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

5.0.0 - Crash with prefer-const rule where code contains array destructuring assignment with 'ignored values' #10520

Closed
mjmasn opened this issue Jun 25, 2018 · 6 comments · Fixed by #10527
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules

Comments

@mjmasn
Copy link
Contributor

mjmasn commented Jun 25, 2018

This is a minimal reproduction for a bug I've found in eslint@5.0.0. As far as I can tell the example code below is valid JavaScript (see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment#Ignoring_some_returned_values).

Tell us about your environment

  • ESLint Version: 5.0.0
  • Node Version: 8.11.0
  • npm Version: 5.6.0
  • yarn Version: 1.7.0

What parser (default, Babel-ESLint, etc.) are you using? default

Please show your full configuration:

{
    "parserOptions": {
        "ecmaVersion": 2018
    },
    "rules": {
      "prefer-const": 1
    }
}

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

const b = [1, 2];
let a;

[, a] = b;
cd eslint-5-array-destructuring-assignment-bug
yarn eslint .

What did you expect to happen?
eslint would lint my project

What actually happened? Please include the actual, raw output from ESLint.
eslint did not lint my project and threw a TypeError from one of its built-in rules (prefer-const)

Cannot read property 'name' of null
TypeError: Cannot read property 'name' of null
    at elements.map.element (/node_modules/eslint/lib/rules/prefer-const.js:173:49)
    at Array.map (<anonymous>)
    at getIdentifierIfShouldBeConst (/node_modules/eslint/lib/rules/prefer-const.js:173:26)
    at groupByDestructuring (/node_modules/eslint/lib/rules/prefer-const.js:227:28)
    at Program:exit (/node_modules/eslint/lib/rules/prefer-const.js:362:17)
    at listeners.(anonymous function).forEach.listener (/node_modules/eslint/lib/util/safe-emitter.js:45:58)
    at Array.forEach (<anonymous>)
    at Object.emit (/node_modules/eslint/lib/util/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (/node_modules/eslint/lib/util/node-event-generator.js:251:26)
    at NodeEventGenerator.applySelectors (/node_modules/eslint/lib/util/node-event-generator.js:280:22)
@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Jun 25, 2018
@platinumazure
Copy link
Member

platinumazure commented Jun 25, 2018

Hi @mjmasn, thanks for the issue!

I can reproduce in the demo. I'll mark this as a bug.

@platinumazure platinumazure added bug ESLint is working incorrectly rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Jun 25, 2018
@mjmasn
Copy link
Contributor Author

mjmasn commented Jun 25, 2018

👍 BTW changing

.map(element => element.name)
to

.map(element => element ? element.name : null)

fixes the crash but I'm not sure if there are any wider implications...

@platinumazure
Copy link
Member

Thanks @mjmasn, that makes sense. Would you like to create a PR (maybe with a new test that would trigger the crash)?

@platinumazure
Copy link
Member

Just out of curiosity, anyone know if this can be recreated in the 4.x release line?

mjmasn pushed a commit to mjmasn/eslint that referenced this issue Jun 26, 2018
…10520)

The crash is triggered by using array destructuring assignment with an
array containing empty (ignored) elements due to attempting to read the
name property of null.
mjmasn pushed a commit to mjmasn/eslint that referenced this issue Jun 26, 2018
…10520)

The crash is triggered by using array destructuring assignment with an
array containing empty (ignored) elements due to attempting to read the
name property of null.
@mjmasn
Copy link
Contributor Author

mjmasn commented Jun 26, 2018

@platinumazure I don't think so, the code was introduced in 240c1a4 (first released in 5.0.0-alpha.4)

@aladdin-add
Copy link
Member

aladdin-add commented Jun 26, 2018

should we release a patch 5.0.2, since seems it was introduced in 5.0.0-alpha.4? cc @eslint/eslint-team

facebook-github-bot pushed a commit to facebookarchive/nuclide that referenced this issue Jun 29, 2018
Summary:
This fixes an issue with the null-coalescing operator (??).

Unfortunately there's an issue with the array destructure syntax when you omit variables: e.g. `[,a,b] = [1,2,3]`. But we've only used that once so far so it's not a big deal (and it should be merged soon.)

eslint/eslint#10520

Reviewed By: aaronabramov

Differential Revision: D8673675

fbshipit-source-id: ce3954c24568d398b421db446fda77145c8adfcb
facebook-github-bot pushed a commit to facebook/metro that referenced this issue Jul 3, 2018
Summary:
Upgrades eslint to v5.0.1

Updated `eslint-plugin-eslint-comments`, which was necessary for eslint 5

Disabled the `prefer-const` rule while we wait for eslint/eslint#10520 to be fixed/published.

Reviewed By: zertosh

Differential Revision: D8692838

fbshipit-source-id: fa0cae3e299af2350c8c30ceb94d70740ee84eab
facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Jul 3, 2018
Summary:
@public

Upgrades eslint to v5.0.1

Updated `eslint-plugin-eslint-comments`, which was necessary for eslint 5

Disabled the `prefer-const` rule while we wait for eslint/eslint#10520 to be fixed/published.

Reviewed By: zertosh

Differential Revision: D8692838

fbshipit-source-id: fa0cae3e299af2350c8c30ceb94d70740ee84eab
facebook-github-bot pushed a commit to facebook/relay that referenced this issue Jul 3, 2018
Summary:
Upgrades eslint to v5.0.1

Updated `eslint-plugin-eslint-comments`, which was necessary for eslint 5

Disabled the `prefer-const` rule while we wait for eslint/eslint#10520 to be fixed/published.

Reviewed By: zertosh

Differential Revision: D8692838

fbshipit-source-id: fa0cae3e299af2350c8c30ceb94d70740ee84eab
not-an-aardvark pushed a commit that referenced this issue Jul 8, 2018
…#10527)

* Chore: Add more tests for prefer-const rule

Adds coverage for the case where the code contains array-destructuring
assignment with 'ignored values'.

* Fix: prefer-const rule crashing on array destructuring (fixes #10520)

The crash is triggered by using array destructuring assignment with an
array containing empty (ignored) elements due to attempting to read the
name property of null.

* Chore: Add more invalid test cases for prefer-const
hansonw added a commit to facebookarchive/atom-ide-ui that referenced this issue Jul 18, 2018
Summary:
This fixes an issue with the null-coalescing operator (??).

Unfortunately there's an issue with the array destructure syntax when you omit variables: e.g. `[,a,b] = [1,2,3]`. But we've only used that once so far so it's not a big deal (and it should be merged soon.)

eslint/eslint#10520

Reviewed By: aaronabramov

Differential Revision: D8673675

fbshipit-source-id: ce3954c24568d398b421db446fda77145c8adfcb
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jan 5, 2019
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jan 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants