Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error/warn on switch case fallthough if there is more than one newline? #1849

Open
cbuttner opened this issue Jan 15, 2025 · 6 comments
Open
Assignees
Labels
Accepted Accepted Request Implemented Needs Verification Check if this issue is resolved
Milestone

Comments

@cbuttner
Copy link
Contributor

I ran into a case recently where I had commented out the body of a switch case which made it fall through and messing up the program logic, and it took me a quite while to realize.

switch (a) {
  case FOO:
    // commented out code... 
    // now it falls through unexpectedly
  case BAR:
    ...
}

Would it perhaps make sense to emit an error or warning if there is more than one newline between the end of case FOO: and case BAR:?

@cbuttner
Copy link
Contributor Author

Correction: More than one newline and no statements.

@lerno lerno added the Enhancement Request New feature or request label Jan 15, 2025
@lerno
Copy link
Collaborator

lerno commented Jan 15, 2025

Yes, I think it's reasonable, in fact this problem has actually entered my mind several times and then I consequently forgot about.

There is also this example:

switch (x)
{
    case 0:
        $if SOME_CONDITION:
            foo();
        $endif;
    case 1:
        ...
}

Here even the intent is unclear: did the programmer intend fallthrough if SOME_CONDITION is false – it has no statements afterwards – or what is the intention?

I can somewhat lean towards treating the $if as a ; - that is a NOP statement but maybe this type of statement should require being ended either by break or nextcase?

@lerno
Copy link
Collaborator

lerno commented Jan 15, 2025

This is the downside of allowing this special case of fallthrough by the way.

@alexveden
Copy link
Contributor

Totally agree with proposal, faced it many times when commented something inside case, and getting implied fallthrough.

@lerno
Copy link
Collaborator

lerno commented Jan 17, 2025

So what should the exact behaviour be? One thing I could do is to require explicit break or nextcase as soon as it isn't completely empty.

@lerno lerno self-assigned this Jan 20, 2025
@lerno lerno added the Accepted Accepted Request label Jan 20, 2025
@lerno lerno added this to the 0.6.7 milestone Jan 20, 2025
@lerno lerno added Implemented Needs Verification Check if this issue is resolved and removed Enhancement Request New feature or request labels Jan 20, 2025
@BWindey
Copy link
Contributor

BWindey commented Jan 21, 2025

@lerno What's the reason behind implementing this as an error rather than a warning?

Imagine the case :

switch (myEnum) {
  // Some comment explaining why it belongs to case 2
  case ENUM1:
  // Some comment about case2
  case ENUM2:
    do_something()
  default:
    say_hi()
}

That now gives an error while I wanted that comment.
I feel like this should be just a warning?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted Request Implemented Needs Verification Check if this issue is resolved
Projects
None yet
Development

No branches or pull requests

4 participants