Request: add option for no-fallthrough with newlines

[SheetJSDev]

Environment

Node version: v16.14.0
npm version: v8.3.1
Local ESLint version: v8.11.0 (Currently used)
Global ESLint version: v7.23.0
Operating System: darwin 21.1.0

What parser are you using?

Default (Espree)

What did you do?

var x = 3;
switch(x){
  case 0:
  case 1: break;

  case 2:
    
  case 3:
    break;
}

ESLint Demo Link

What did you expect to happen?

No errors

What actually happened?

  8:3  error  Expected a 'break' statement before 'case'  no-fallthrough

Participation

• I am willing to submit a pull request for this issue.

Additional comments

It might also make sense to support comments:

switch(x) {
  // currently accepted
  case 0:
  case 1:
    break;

  // this issue
  case 2:

  case 3:
    break;

  // general proposal
  /* a note about 4 */
  case 4:
  /* a note about 5 */
  case 5:
    break;
}

[nzakas]

Thanks for the report. I’m curious why you believe this is a bug? ESLint is reporting that there should be a break before case 3, which is correct. Because of the extra beeline after case 2, ESLint doesn’t know if you intend for it to fall through or not.

[SheetJSDev]

https://eslint.org/docs/rules/no-fallthrough does not actually define "unintentional fallthrough", but each of the bad examples look like

switch(x) {
  case 1:
    // ...
    doSomething(); // there is at least one statement 

  case 2:
    // ...
}

There is no example in the docs page with zero statements in a case clause and blank lines before the next statement, so this question focuses on an undocumented corner case.

IMHO "unintentional fallthrough" requires at least one statement in the case clause.

The spec does not use the phrase "fall through", but other languages do have definitions for this. C++17 / C23 formalized the "fallthrough attribute" and GCC/Clang support -Wimplicit-fallthrough. For example:

int main() {
  int i = 0;
  switch(3) {
    case 0:
    case 1:
      break;

    /* one blank line */
    case 2:

    case 3:
      break;

    /* two blank lines */
    case 4:


    case 5:
      break;

    /* one statement */
    case 6:

      i = 1;

    case 7: break;
  }
  return 0;
}

Running this through clang only shows a warning in the last part (one statement):

% clang++ -Wimplicit-fallthrough test.cc
test.cc:26:5: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
    case 7: break;
    ^
test.cc:26:5: note: insert 'break;' to avoid fall-through
    case 7: break;
    ^
    break; 

[LvChengbin]

I encountered the same problem after I added comments before a case statement.

switch( true ) {
    case 'a' :
    case 'b' :
}


switch( true ) {
    case 'a' :
    /**
     * 
     */
    case 'b' :
}

And you can see the demo here: https://eslint.org/demo#eyJ0ZXh0Ijoic3dpdGNoKCB0cnVlICkge1xuY2FzZSAnYScgOlxuY2FzZSAnYicgOlxuXHRcbn1cblxuXG5zd2l0Y2goIHRydWUgKSB7XG5jYXNlICdhJyA6XG4vKipcbiAqIFxuICovXG5jYXNlICdiJyA6XG5cdFxufSIsIm9wdGlvbnMiOnsicGFyc2VyT3B0aW9ucyI6eyJlY21hVmVyc2lvbiI6MTIsInNvdXJjZVR5cGUiOiJzY3JpcHQiLCJlY21hRmVhdHVyZXMiOnt9fSwicnVsZXMiOnsiY29uc3RydWN0b3Itc3VwZXIiOjIsImZvci1kaXJlY3Rpb24iOjIsImdldHRlci1yZXR1cm4iOjIsIm5vLWFzeW5jLXByb21pc2UtZXhlY3V0b3IiOjIsIm5vLWNhc2UtZGVjbGFyYXRpb25zIjoyLCJuby1jbGFzcy1hc3NpZ24iOjIsIm5vLWNvbXBhcmUtbmVnLXplcm8iOjIsIm5vLWNvbmQtYXNzaWduIjoyLCJuby1jb25zdC1hc3NpZ24iOjIsIm5vLWNvbnN0YW50LWNvbmRpdGlvbiI6Miwibm8tY29udHJvbC1yZWdleCI6Miwibm8tZGVidWdnZXIiOjIsIm5vLWRlbGV0ZS12YXIiOjIsIm5vLWR1cGUtYXJncyI6Miwibm8tZHVwZS1jbGFzcy1tZW1iZXJzIjoyLCJuby1kdXBlLWVsc2UtaWYiOjIsIm5vLWR1cGUta2V5cyI6Miwibm8tZHVwbGljYXRlLWNhc2UiOjIsIm5vLWVtcHR5IjoyLCJuby1lbXB0eS1jaGFyYWN0ZXItY2xhc3MiOjIsIm5vLWVtcHR5LXBhdHRlcm4iOjIsIm5vLWV4LWFzc2lnbiI6Miwibm8tZXh0cmEtYm9vbGVhbi1jYXN0IjoyLCJuby1leHRyYS1zZW1pIjoyLCJuby1mYWxsdGhyb3VnaCI6Miwibm8tZnVuYy1hc3NpZ24iOjIsIm5vLWdsb2JhbC1hc3NpZ24iOjIsIm5vLWltcG9ydC1hc3NpZ24iOjIsIm5vLWlubmVyLWRlY2xhcmF0aW9ucyI6Miwibm8taW52YWxpZC1yZWdleHAiOjIsIm5vLWlycmVndWxhci13aGl0ZXNwYWNlIjoyLCJuby1taXNsZWFkaW5nLWNoYXJhY3Rlci1jbGFzcyI6Miwibm8tbWl4ZWQtc3BhY2VzLWFuZC10YWJzIjoyLCJuby1uZXctc3ltYm9sIjoyLCJuby1vYmotY2FsbHMiOjIsIm5vLW9jdGFsIjoyLCJuby1wcm90b3R5cGUtYnVpbHRpbnMiOjIsIm5vLXJlZGVjbGFyZSI6Miwibm8tcmVnZXgtc3BhY2VzIjoyLCJuby1zZWxmLWFzc2lnbiI6Miwibm8tc2V0dGVyLXJldHVybiI6Miwibm8tc2hhZG93LXJlc3RyaWN0ZWQtbmFtZXMiOjIsIm5vLXNwYXJzZS1hcnJheXMiOjIsIm5vLXRoaXMtYmVmb3JlLXN1cGVyIjoyLCJuby11bmRlZiI6Miwibm8tdW5leHBlY3RlZC1tdWx0aWxpbmUiOjIsIm5vLXVucmVhY2hhYmxlIjoyLCJuby11bnNhZmUtZmluYWxseSI6Miwibm8tdW5zYWZlLW5lZ2F0aW9uIjoyLCJuby11bnVzZWQtbGFiZWxzIjoyLCJuby11bnVzZWQtdmFycyI6Miwibm8tdXNlbGVzcy1jYXRjaCI6Miwibm8tdXNlbGVzcy1lc2NhcGUiOjIsIm5vLXdpdGgiOjIsInJlcXVpcmUteWllbGQiOjIsInVzZS1pc25hbiI6MiwidmFsaWQtdHlwZW9mIjoyLCJpbmRlbnQiOjJ9LCJlbnYiOnt9fX0=

[nzakas]

In all of the cases mentioned, the rule is behaving as expected. Only cases without empty lines or comments between them are considered valid. As soon as they are separated, this will trigger the warning.

We can update the docs to show these examples.

[thw0rted]

Just for trackback, there are bunch of old locked issues about inconsistent no-fallthrough behavior:

#6741
#7889
#9080
#9559
#10608
#13339
#14701 (finally actually got implemented)

I came here looking for a way to handle commentPattern where it isn't the last line immediately before the next case, and found there's a closed PR to implement this:

#11186

IMHO, it would be an objective improvement to the rule to strictly define fallthrough as a case that executes at least one statement, then continues execution to a subsequent case, without a matching comment between the last executed line under the first case and the first executed line under the next case. This would immediately address all the above linked issues by implementing the behavior they have requested. The current behavior is surprising (and thus, "bad") given a plain-English understanding of the meaning of "fallthrough".

[TriMoon]

It's indeed a bit weird that it only reacts on empty lines, see demos with and without a blank line in between. (where only line 8 is deleted)

[reviewher]

Coming from other programming languages, ESLint's current definition of fallthrough is inconsistent. @SheetJSDev talked about C/C++. Other languages:

PHP: PSR2 guidelines define fallthrough as requiring at least one statement. This is the same as C/C++

Java: Sun style defines a fallthrough as "doesn't include a break statement", so technically you always need a fallthrough comment between cases, no matter how much whitespace is in between.

Go: forced breaks at the end of each case clause, even if there is no whitespace. Special keyword fallthrough to fall through.

Perl: given / when statements do not fall through. Special keyword continue to fall through.

C#: compilation error if there is no break between case clauses, irrespective of the number of lines. Special statement goto case ____ to fall through

Ruby: case / when statements do not fall through

Rust: match does not have fallthrough support.

Pascal: case statement has no fallthrough

In summary, "everything but eslint" defines fallthrough in one of two ways:

"intrinsic": each case is separate. case a: case b: are seen as two distinct parts irrespective of the amount of whitespace and newlines between them.

"statement": at least one statement between case clauses with no final break.

ESLint's behavior is surprising and feels inconsistent.

[divinity76]

i hope there's a way to tell the linter that the passthrough is intentional tho? for example in the Eclipse Java linter,

	case FIOC_READ:
		is_read = 1;
	case FIOC_WRITE:
		fioc_do_rw(req, arg, in_buf, in_bufsz, out_bufsz, is_read);
		break;

will cause a linter warning, but

	case FIOC_READ:
		is_read = 1;
		/* no break */
	case FIOC_WRITE:
		fioc_do_rw(req, arg, in_buf, in_bufsz, out_bufsz, is_read);
		break;

tells the linter that the passthru is intentional

[nzakas]

@divinity76 please check out the documentation for the rule:
https://eslint.org/docs/rules/no-fallthrough

[reviewher]

@LvChengbin Here's a truly funny workaround that passes eslint:

switch( true ) {
  case 'a' /*

    Put a message here

  */:
  case 'b' : break;

  case 'c':
  case /*

    Put a message here

  */ 'd':
}

Both workarounds pass because eslint does not check whitespace between case, the value of the case clause, and the colon.

Is this more readable / less bug prone? IMHO no but it at least passes the rule check

[mdjermanovic]

This works as intended. The rule considers only cases that appear in consecutive lines as an intentional fallthrough.

I don't think we should change the default behavior because we don't know if most users would really prefer the suggested one.

However, since there are different opinions on what looks and what does not look like intentional fallthrough, I wouldn't mind adding an option to change the behavior. The new option, when enabled, would allow empty cases regardless of blank lines and comments inside.

[thw0rted]

Linters are always going to wind up favoring one style or construct over another. I agree that knowing what "most users would prefer" sounds useful, but also difficult to achieve in practice. How does the team make decisions like this, in the absence of a poll of X number of ES programmers? Maybe the standard should be like Wikipedia, "no original research" -- if you want to make an assertion (like "fallthrough should only include case-statements on consecutive lines", or "fallthrough can include whitespace"), you need to bring a citation from a reputable source backing it up.

A few posts back, @reviewher did a pretty good job of covering how other languages define fallthrough, which I think makes a good case for making whitespace-counts as default behavior. Has anyone put together a counter-argument in favor of the current (no-whitespace) behavior, perhaps when the rule was originally added?

[reviewher]

I genuinely tried to find an example outside of ESLint and came up short. Even in JS land:

JSHint takes the "statement" interpretation (you can verify on https://jshint.com/ )

JSLint takes the "statement" interpretation (you can verify on https://www.jslint.com/ )

ESLint "blazing a new trail" is not inherently bad, but there arguably should be a very compelling reason.

[SheetJSDev]

@mdjermanovic "most users" probably never use switch statements, and "most switch users" probably never use the fallthrough behavior intentionally (commented or otherwise). The users who do use switch statements liberally in JS (the relevant demographic for the rule) likely have deep experience with system languages like C and expect the concept to work in the same way.

That said, as discussed in this thread, ESLint stands alone in the current behavior. IMHO the current behavior encourages unreadable patterns.

[nzakas]

The team is in agreement that we can add a new option for this behavior but we won’t be changing the default behavior. Anyone is free to submit a pull request for this change.

[Gautam-Arora24]

I would like to work on this issue.

[nzakas]

@Gautam-Arora24 go for it 👍

[amareshsm]

I am interested to work on this issue and i am looking into it.

[Gautam-Arora24]

@amareshsm Go for it :)

[snitin315]

@amareshsm Can I finish this if you are not working on it actively?

[amareshsm]

@snitin315 Yes, I will complete this today.