Fix: no-unreachable reporting EmptyStatements (fixes #9081)

[platinumazure]

no-unreachable will only report if there is at least one non-EmptyStatement node in the range

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

See #9081.

What changes did you make? (Give an overview)

no-unreachable now tracks whether non-EmptyStatement nodes have been included in an unreachable code range.

• If an unreachable code range only includes EmptyStatement nodes, the range is not reported. (Rule no-empty should be used instead.)
• However, if an unreachable code range has at least one EmptyStatement and at least one non-EmptyStatement, the entire range is reported, including the EmptyStatement(s).

Is there anything you'd like reviewers to focus on?

Is there a better way to track empty vs non-empty statements? My preference would have been to iterate on nodes (not tokens) between the start and end node of the reporting range and checking that all are EmptyStatements, but SourceCode doesn't have any methods for node iteration. I think what I have works for now, but will fall apart if the traversal order changes.

[eslintbot]

LGTM

[mention-bot]

@platinumazure, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mysticatea, @vitorbal and @jrfeenst to be potential reviewers.

[not-an-aardvark]

It looks like no-unreachable currently has a listener for every statement type. Would it work to simply remove the listener for EmptyStatement?

[platinumazure]

I thought of that, but I'm afraid of it missing some ranges or parts of ranges that are or contain EmptyStatements. The handler basically does two things: checks to see if this statement is unreachable and tracks it if so; and reporting once non-consecutive code is traversed. The first step still needs to happen for EmptyStatements, I think. All that said, there's nothing stopping me from trying it out now that I have some test cases.

…

On Aug 7, 2017 2:02 AM, "Teddy Katz" ***@***.***> wrote: It looks like no-unreachable currently has a listener for every statement type. Would it work to simply remove the listener for EmptyStatement <https://github.com/eslint/eslint/blob/a113cd3bf831078c3d7d417aa7ab768b2fbd0fe4/lib/rules/no-unreachable.js#L180> ? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#9084 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARWevS_6fBQLmRJG5tc-t0fcC5lr2Ekks5sVrabgaJpZM4Ou5t3> .

[platinumazure]

Adding "do not merge" since there's still some bikeshedding going on.

[platinumazure]

@eslint/eslint-team This should be ready for review. Thanks!

@not-an-aardvark Removing the listener will affect the range reported. I personally like the idea of reporting the entire range of unreachable code, as long as we only report code that has more than just EmptyStatements. (Please feel free to look at the unit tests and look at the start/end line/column in the new test cases to see what I'm after.)

[michaelficarra]

👎 I want this rule to continue warning on empty statements. Also, this is a breaking change and thus needs to be behind an option. Using no-empty is not an option for me because I use empty statements which are not dead code, such as in loop bodies.

[platinumazure]

I misspoke when I said `no-empty`would help report empty statements, sorry. The rule for that is `no-extra-semi`. Sorry for the miscommunication and subsequent confusion.

…

On Aug 10, 2017 10:57 AM, "Michael Ficarra" ***@***.***> wrote: 👎 I want this rule to continue warning on empty statements. Also, this is a breaking change and thus needs to be behind an option. Using no-empty is not an option for me because I use empty statements which are not dead code, such as in loop bodies. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#9084 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARWelmNRaJh81kv-SgRwPoMwG3fkwAWks5sWyhOgaJpZM4Ou5t3> .

[ljharb]

@michaelficarra What specific code patterns would this rule as-is cover, that wouldn't be covered by no-extra-semi + the-rule-after-this-PR?

[platinumazure]

@michaelficarra
Some examples, in case it helps:

// Case 1: Switch with extra EmptyStatement

switch (foo) {
    case true: throw new Error();
    default: throw new Error();
};         // <-- reported by no-extra-semi and no-unreachable

// After this PR: Line above will not be reported by no-unreachable, but will be reported by no-extra-semi

// Case 2: Switch with extra EmptyStatement and unreachable non-empty statement

switch (foo) {
    case true: throw new Error();
    default: throw new Error();
};         // <-- reported by no-extra-semi and no-unreachable
bar();     // <-- reported by no-unreachable

// After this PR: No change-- everything from the extra semicolon to the end of `bar();` is reported by no-unreachable

The goal of this change is to avoid no-unreachable reporting completely frivolous examples (i.e., range contains only EmptyStatement nodes).

[michaelficarra]

I'm okay with this change now that I know it's not no-empty but no-extra-semi which will need to be enabled in its place.

Still, in general, I don't think we should be modifying rule A so that it doesn't report anything that would also be reported by unrelated rule B. It's fine for one section of code to be identified as wrong in more than one way. I care more that I can describe what a rule does without a big list of exceptions.

X: What does the no-unreachable rule do?

Me: It reports any unreachable code. Unless no-extra-semi would also have reported that code.

X: Okay, what does no-extra-semi do?

Me: It reports unnecessary semicolons. Unless ...

(repeat)

[ljharb]

The difference here is that empty statements aren't code, in a conceptual sense - people who don't understand parsing and/or spec details aren't going to expect that warning, nor find it valuable.

[not-an-aardvark]

@not-an-aardvark Removing the listener will affect the range reported. I personally like the idea of reporting the entire range of unreachable code, as long as we only report code that has more than just EmptyStatements. (Please feel free to look at the unit tests and look at the start/end line/column in the new test cases to see what I'm after.)

I'm not sure I agree that this is the best behavior. Consider the following code:

switch (foo) {
  case 1: return x;
  default: return y;
};

baz();

It seems like the motivation behind this change is that users don't consider EmptyStatements to be "code". So as far as the no-unreachable rule is concerned, putting a semicolon after the switch statement is a valid stylistic choice. I think it's misleading to make the report range start at the semicolon, because it's only necessary to remove baz() to fix the problem, and the rule isn't supposed to care about whether the semicolon is present.

In other words, I think that instead of "reporting the entire range of unreachable code, as long as we only report code that has more than just EmptyStatements", the rule should conceptually just ignore unreachable EmptyStatements entirely. That would provide the clearest error message to the user, and as a bonus it would also make the implementation simpler.

[platinumazure]

I'll take another crack at this (just ignoring EmptyStatements entirely) when I get time. Closing this one for now since it's not the desired direction.

EDIT: If someone else wants to take this on, feel free, just ping me so I know.