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

`no-unreachable` reports EmptyStatements #9081

Closed
StoneCypher opened this issue Aug 6, 2017 · 20 comments

Comments

@StoneCypher
Copy link

commented Aug 6, 2017

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.

@eslintbot eslintbot added the triage label Aug 6, 2017

@ljharb

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2017

What happens if you remove the unnecessary semicolon after the switch? I wonder if that's creating an extra empty statement that's causing no-unreachable to warn (which it shouldn't, i'd think)

@StoneCypher

This comment has been minimized.

Copy link
Author

commented Aug 6, 2017

holy shit.

@StoneCypher StoneCypher closed this Aug 6, 2017

@StoneCypher StoneCypher reopened this Aug 6, 2017

@StoneCypher

This comment has been minimized.

Copy link
Author

commented Aug 6, 2017

ya i think it shouldn't also

that's legitimately it and i think i just caught on fire

pardon me, calling the burn team

@StoneCypher

This comment has been minimized.

Copy link
Author

commented Aug 6, 2017

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

@ljharb

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2017

Seems like the bug is that no-unreachable shouldn't care about empty statements.

@StoneCypher

This comment has been minimized.

Copy link
Author

commented Aug 6, 2017

test case restored

@platinumazure

This comment has been minimized.

Copy link
Member

commented Aug 6, 2017

@ljharb Yeah, it would be nice for no-unreachable not to care about empty statements, since we do have no-empty for those. I'm not sure how easy that will be to patch, though.

@platinumazure platinumazure added accepted bug rule and removed triage labels Aug 6, 2017

@platinumazure

This comment has been minimized.

Copy link
Member

commented Aug 6, 2017

I'll work on this.

platinumazure added a commit that referenced this issue Aug 7, 2017

Fix: no-unreachable reporting EmptyStatements (fixes #9081)
no-unreachable will only report if there is at least one non-EmptyStatement node in the range

@not-an-aardvark not-an-aardvark changed the title Is this `no-unreachable` difference correct? `no-unreachable` reports EmptyStatements Aug 7, 2017

@mysticatea

This comment has been minimized.

Copy link
Member

commented Aug 7, 2017

Just link to a previous discussion: #6295

@platinumazure

This comment has been minimized.

Copy link
Member

commented Aug 7, 2017

Hmm. Apparently I was in favor of changing the lint message rather than not reporting at that time. Thanks @mysticatea for finding that discussion.

I'd be okay with modifying my PR to just change the lint message if there are only EmptyStatements in the range (and continuing to report entire ranges including EmptyStatements when there is at least one statement that is not an EmptyStatement). Just not sure what's the best approach here.

Ping @ljharb, any thoughts on the discussion in #6295?

@ljharb

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2017

I think it makes no sense at all for no-unreachable to consider EmptyStatements at all. It's not about whether a semi after a switch makes sense (it doesn't), but that's something only no-semi should warn on.

Empty statements contain no code, therefore there's nothing unreachable in them, from an intuition standpoint. I should be able to use ;\n as my blank line terminator if I wanted, without running afoul of this rule.

@StoneCypher

This comment has been minimized.

Copy link
Author

commented Aug 8, 2017

I would be okay with either outcome, but I agree with ljharb: if there isn't any unreachable code (empty statements are not code in my mind,) then this rule should not fire IMO

@platinumazure

This comment has been minimized.

Copy link
Member

commented Aug 8, 2017

@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

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2017

That sounds perfectly reasonable to me!

@StoneCypher

This comment has been minimized.

Copy link
Author

commented Aug 9, 2017

As someone who has never contributed to this codebase, that sounds approximately ideal to me.

Thanks for the inclusion and thanks for the rapid response.

@platinumazure

This comment has been minimized.

Copy link
Member

commented Aug 10, 2017

@ljharb @StoneCypher 👍, I'll request reviews on my pull request with the current behavior.

@kaicataldo

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

@platinumazure Where does this issue stand right now?

@platinumazure

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

I ended up closing my PR as not the desired direction. I am not currently working on this. Contributions would be most welcome. Thanks!

@nzakas nzakas self-assigned this Nov 6, 2018

@nzakas

This comment has been minimized.

Copy link
Member

commented Nov 6, 2018

I'll work on this.

nzakas added a commit that referenced this issue Nov 6, 2018

@mysticatea mysticatea closed this in dcc6233 Nov 8, 2018

@StoneCypher

This comment has been minimized.

Copy link
Author

commented Nov 8, 2018

thanks

@eslint eslint bot locked and limited conversation to collaborators May 8, 2019

@eslint eslint bot added the archived due to age label May 8, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
7 participants
You can’t perform that action at this time.