Fix: do not fail on unknown operators from custom parsers (fixes #10475)

[rubennorte]

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

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

What changes did you make? (Give an overview)

I modified the code path analysis code to explicitly check the operator of the logical expressions in order to create new code paths. This will allow users to use a custom parser that adds a new operator (like ??) without breaking the code. The error users see now is

Error: unreachable
      at CodePathState.popChoiceContext (node_modules/eslint/lib/code-path-analysis/code-path-state.js:444:23)
      at processCodePathToExit (node_modules/eslint/lib/code-path-analysis/code-path-analyzer.js:434:19)
      at CodePathAnalyzer.leaveNode (node_modules/eslint/lib/code-path-analysis/code-path-analyzer.js:627:9)
      at Traverser.leave (node_modules/eslint/lib/linter.js:959:32)
      at Traverser.__execute (node_modules/estraverse/estraverse.js:397:31)
      at Traverser.traverse (node_modules/estraverse/estraverse.js:491:28)
      at Traverser.traverse (node_modules/eslint/lib/util/traverser.js:31:22)
      at Linter._verifyWithoutProcessors (node_modules/eslint/lib/linter.js:953:19)
      at preprocess.map.textBlock (node_modules/eslint/lib/linter.js:994:35)
      at Array.map (<anonymous>)
      at Linter.verify (node_modules/eslint/lib/linter.js:993:42)
      ...

Fixes #10475 (see for more details) and provides a more generic solution to #10399 (which is against the ESLint policy about providing support for pre-Stage 3 proposals).

[jsf-clabot]

All committers have signed the CLA.

[rubennorte]

I manually modified the operator to make sure this solution was generic (and also to make this still relevant when ESLint adds official support for ?? in the future).

[platinumazure]

This approach looks promising, thanks for writing this up! Hopefully the rest of the team (and probably the TSC-- I assume the TSC will need to review) will agree.

[not-an-aardvark]

LGTM, thanks!

Looking at the existing code, it seems like we're not actually treating && and || any differently from each other. I'm wondering if it would work to treat unknown logical operators the same as && and || rather than not forking the code path for unknown operators -- the advantage could be that the resulting codepath might be more correct in common cases. (However, that's just a minor suggestion and shouldn't block this from being merged.)

[rubennorte]

@not-an-aardvark I considered merging them into a "logical" kind, but this required less modifications in the code and looked cleaner (as it didn't need to modify anything inside code-path-state).

[mysticatea]

Thank you for this PR.

LGTM.

But I have a small concern: the token's type property must be a string (AST spec). It's better if the added test uses a valid AST.

[rubennorte]

@mysticatea I just copied the output from babel-eslint (the version is in the file comment). If that's wrong I think we should fix it there.

Also the goal of this PR is to allow us to use new operators with babel-eslint (or other custom parsers), so it seems better to directly use the same output it provides.

Edit: would you rather fix it by hand (adding a comment in the file) or leave it like this? I'm going to fix it in babel-eslint but I'd rather not wait for it to get this merged (as this specific fix is the only thing preventing us from rolling out ?? at Facebook).

Edit 2: created PR to fix it in babel-eslint: babel/babel-eslint#632

[mysticatea]

I'd like to fix it by hand to adhere the AST spec; to "Punctuator" or something like.

[rubennorte]

I finally committed an intermediate solution (updated the AST with the fix I sent to babel-eslint).

Do you think we can have this merged soon? Also, would it be possible to cut a v4.20.0 release with this?

[not-an-aardvark]

To answer your question about release timing: we have scheduled releases every 2 weeks, and our next release is scheduled for next Friday, June 22nd. That release is expected to be the first stable release of ESLint v5.0. We've been doing prereleases for ESLint v5.0 for the last couple months, and unfortunately we don't really have the manpower to maintain multiple release branches simultaneously, so we only backport changes if a critical bug is found.

That said, since the upcoming release is expected to be stable, it should be possible to use it relatively soon.

[rubennorte]

That seems reasonable. I'll test the last prerelease in Facebook to upgrade it as soon as this gets merged. If I can do something to accelerate the merge of this PR, please let me know. Thanks!

[platinumazure]

After some discussion with other team members, I've been persuaded that this is a bug fix, not an enhancement. Relabeling accordingly. Sorry for the inconvenience/confusion!

[platinumazure]

@rubennorte Not from my perspective. I just want to leave this open a bit longer so more team members could review and potentially request changes. Whether we merge today or next week, it will definitely make next week's release (barring a catastrophe).

[kaicataldo]

LGTM. Thanks for contributing to ESLint!

[not-an-aardvark]

Thanks!