eslint: `no-unreachable` reports EmptyStatements
Thank you for a wonderful tool. Most times I think I find a bug, instead I learn a JS horror detail.
Can’t figure this one out yet. Don’t think it’s env related.
- ESLint - 4.3.0
- Node - 8.1.3
- NPM - 5.3.0
- Parser - babel-eslint
- Exhibits on Mac, Win10, Travis CI’s debian
Please show your full configuration:
Configuration
John-Haugeland:jssm johnhaugeland$ cat .eslintrc
{
"parser" : "babel-eslint",
"plugins" : [ ],
"env" : { "es6": true, "commonjs": true },
"extends" : "eslint-config-stonecypher"
}
eslint-config-stonecypher
in turn configures every current eslint rule, plus eslint-plugin-
… flowtype
, fp
, jsdoc
, ava
, unicorn
, new-with-error
, and promise
What did you do? Please include the actual source code causing the issue.
The following code triggers “no-unreachable” at the end of the switch, even though it’s the last action in a default, ostensibly because of the throw
.
const arrow_direction = arrow => {
switch (arrow) {
case '->': case '=>': case '~>':
return 'right';
case '<-': case '<=': case '<~':
return 'left';
case '<->': case '<-=>': case '<-~>':
case '<=>': case '<=->': case '<=~>':
case '<~>': case '<~->': case '<~=>':
return 'both';
default:
throw new Error(`arrow_direction: unknown arrow type ${arrow}`);
};
}
What did you expect to happen?
Nothing. No code follows the throw
.
This similar code does not trigger the warning, I expect because the throw is now outside the switch.
const arrow_direction = arrow => {
switch (arrow) {
case '->': case '=>': case '~>':
return 'right';
case '<-': case '<=': case '<~':
return 'left';
case '<->': case '<-=>': case '<-~>':
case '<=>': case '<=->': case '<=~>':
case '<~>': case '<~->': case '<~=>':
return 'both';
};
throw new Error(`arrow_direction: unknown arrow type ${arrow}`);
}
What actually happened? Please include the actual, raw output from ESLint.
/Users/johnhaugeland/projects/jssm/src/js/jssm.js
22:7 warning 'arrow_direction' is assigned a value but never used no-unused-vars
22:7 warning Missing "arrow_direction" variable type annotation flowtype/require-variable-type
22:25 warning Missing return type annotation flowtype/require-return-type
22:25 warning Missing "arrow" parameter type annotation flowtype/require-parameter-type
44:4 warning Unnecessary semicolon no-extra-semi
44:4 error Unreachable code no-unreachable
...
It’s not like it’s a big deal, or anything. I moved the throws outside of the switch afterwards anyway, because in retrospect it’s sorta gross.
It’s just surprising, and I wonder whether it’s legitimately a subtle bug.
Please have a nice day.
About this issue
- Original URL
- State: closed
- Created 7 years ago
- Comments: 20 (13 by maintainers)
Commits related to this issue
- Fix: no-unreachable reporting EmptyStatements (fixes #9081) no-unreachable will only report if there is at least one non-EmptyStatement node in the range — committed to eslint/eslint by platinumazure 7 years ago
- Fix: Ignore empty statements in no-unreachable (fixes #9081) — committed to eslint/eslint by nzakas 6 years ago
@ljharb @StoneCypher 👍, I’ll request reviews on my pull request with the current behavior.
I’ll work on this.
I’ll work on this.
I ended up closing my PR as not the desired direction. I am not currently working on this. Contributions would be most welcome. Thanks!
@ljharb @StoneCypher You may wish to check out my pull request (see #9084). Basically, what happens is, EmptyStatements are included in a reported range as unreachable code, but if the entire block of “unreachable code” consists solely of EmptyStatements, then it’s not reported at all. So basically, my approach is a good balance of practicality (don’t report code that won’t do anything) and accuracy (but do report all of the unreachable code when some or all of that code is non-empty).
If you think that behavior is unreasonable, feel free to leave a comment here or there. But I’m fairly sure this approach gets you what you want, in terms of the clear priority.
@ljharb Yeah, it would be nice for
no-unreachable
not to care about empty statements, since we do haveno-empty
for those. I’m not sure how easy that will be to patch, though.note: had removed the semicolon @ljharb is talking about as a cleanliness detail before he spoke up, without retesting it
if he hadn’t responded so ridiculously quickly i would have actually accidentally blown the test case
What happens if you remove the unnecessary semicolon after the
switch
? I wonder if that’s creating an extra empty statement that’s causingno-unreachable
to warn (which it shouldn’t, i’d think)