eslint
eslint copied to clipboard
eslint
Request: add option for no-fallthrough with newlines
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;
}
What did you expect to happen?
No errors
What actually happened?
8:3 error Expected a 'break' statement before 'case' no-fallthrough
Participation
- [X] 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;
}
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.
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;
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=
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.
Just for trackback, there are bunch of old locked issues about inconsistent no-fallthrough behavior:
https://github.com/eslint/eslint/issues/6741 https://github.com/eslint/eslint/issues/7889 https://github.com/eslint/eslint/issues/9080 https://github.com/eslint/eslint/issues/9559 https://github.com/eslint/eslint/issues/10608 https://github.com/eslint/eslint/issues/13339 https://github.com/eslint/eslint/issues/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:
https://github.com/eslint/eslint/pull/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".
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)
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.
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
@divinity76 please check out the documentation for the rule: https://eslint.org/docs/rules/no-fallthrough
@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
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.
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?
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.
@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.
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.
