Skip to content

Commit

Permalink
Fix: prefer-const object destructuring false positive (fixes #9108) (#…
Browse files Browse the repository at this point in the history
…10368)

<!--
    ESLint adheres to the [JS Foundation Code of Conduct](https://js.foundation/community/code-of-conduct).
-->
close #9108

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

[ ] Documentation update
[x] Bug fix ([template](https://raw.githubusercontent.com/eslint/eslint/master/templates/bug-report.md))
[ ] New rule ([template](https://raw.githubusercontent.com/eslint/eslint/master/templates/rule-proposal.md))
[ ] Changes an existing rule ([template](https://raw.githubusercontent.com/eslint/eslint/master/templates/rule-change-proposal.md))
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

<!--
    If the item you've checked above has a template, please paste the template questions below and answer them. (If this pull request is addressing an issue, you can just paste a link to the issue here instead.)
-->
#9108

<!--
    Please ensure your pull request is ready:

    - Read the pull request guide (https://eslint.org/docs/developer-guide/contributing/pull-requests)
    - Include tests for this change
    - Update documentation for this change (if appropriate)
-->

<!--
    The following is required for all pull requests:
-->

**What changes did you make? (Give an overview)**
Allow:
```js
(function ({ a }) {
  let b;
  ({ a, b } = obj);
})();
```
or
```js
let a;
{
  let b;
  ({ a, b } = obj);
}
```
  • Loading branch information
g-plane authored and btmills committed May 28, 2018
1 parent 93c9a52 commit 240c1a4
Show file tree
Hide file tree
Showing 2 changed files with 93 additions and 27 deletions.
104 changes: 77 additions & 27 deletions lib/rules/prefer-const.js
Expand Up @@ -5,6 +5,8 @@

"use strict";

const astUtils = require("../ast-utils");

//------------------------------------------------------------------------------
// Helpers
//------------------------------------------------------------------------------
Expand Down Expand Up @@ -55,6 +57,54 @@ function canBecomeVariableDeclaration(identifier) {
);
}

/**
* Checks if an property or element is from outer scope or function parameters
* in destructing pattern.
*
* @param {string} name - A variable name to be checked.
* @param {eslint-scope.Scope} initScope - A scope to start find.
* @returns {boolean} Indicates if the variable is from outer scope or function parameters.
*/
function isOuterVariableInDestructing(name, initScope) {
if (initScope.through.find(ref => ref.resolved && ref.resolved.name === name)) {
return true;
}

const variable = astUtils.getVariableByName(initScope, name);

if (variable !== null) {
return variable.defs.some(def => def.type === "Parameter");
}

return false;
}

/**
* Gets the VariableDeclarator/AssignmentExpression node that a given reference
* belongs to.
* This is used to detect a mix of reassigned and never reassigned in a
* destructuring.
*
* @param {eslint-scope.Reference} reference - A reference to get.
* @returns {ASTNode|null} A VariableDeclarator/AssignmentExpression node or
* null.
*/
function getDestructuringHost(reference) {
if (!reference.isWrite()) {
return null;
}
let node = reference.identifier.parent;

while (PATTERN_TYPE.test(node.type)) {
node = node.parent;
}

if (!DESTRUCTURING_HOST_TYPE.test(node.type)) {
return null;
}
return node;
}

/**
* Gets an identifier node of a given variable.
*
Expand Down Expand Up @@ -102,6 +152,32 @@ function getIdentifierIfShouldBeConst(variable, ignoreReadBeforeAssign) {
if (isReassigned) {
return null;
}

const destructuringHost = getDestructuringHost(reference);

if (destructuringHost !== null && destructuringHost.left !== void 0) {
const leftNode = destructuringHost.left;
let hasOuterVariables = false;

if (leftNode.type === "ObjectPattern") {
const properties = leftNode.properties;

hasOuterVariables = properties
.filter(prop => prop.value)
.map(prop => prop.value.name)
.some(name => isOuterVariableInDestructing(name, variable.scope));
} else if (leftNode.type === "ArrayPattern") {
const elements = leftNode.elements;

hasOuterVariables = elements
.map(element => element.name)
.some(name => isOuterVariableInDestructing(name, variable.scope));
}
if (hasOuterVariables) {
return null;
}
}

writer = reference;

} else if (reference.isRead() && writer === null) {
Expand Down Expand Up @@ -131,32 +207,6 @@ function getIdentifierIfShouldBeConst(variable, ignoreReadBeforeAssign) {
return writer.identifier;
}

/**
* Gets the VariableDeclarator/AssignmentExpression node that a given reference
* belongs to.
* This is used to detect a mix of reassigned and never reassigned in a
* destructuring.
*
* @param {eslint-scope.Reference} reference - A reference to get.
* @returns {ASTNode|null} A VariableDeclarator/AssignmentExpression node or
* null.
*/
function getDestructuringHost(reference) {
if (!reference.isWrite()) {
return null;
}
let node = reference.identifier.parent;

while (PATTERN_TYPE.test(node.type)) {
node = node.parent;
}

if (!DESTRUCTURING_HOST_TYPE.test(node.type)) {
return null;
}
return node;
}

/**
* Groups by the VariableDeclarator/AssignmentExpression node that each
* reference of given variables belongs to.
Expand Down Expand Up @@ -290,7 +340,7 @@ module.exports = {
(varDeclParent.parent.type === "ForInStatement" || varDeclParent.parent.type === "ForOfStatement" || varDeclParent.declarations[0].init) &&

/*
* If options.destucturing is "all", then this warning will not occur unless
* If options.destructuring is "all", then this warning will not occur unless
* every assignment in the destructuring should be const. In that case, it's safe
* to apply the fix.
*/
Expand Down
16 changes: 16 additions & 0 deletions tests/lib/rules/prefer-const.js
Expand Up @@ -69,6 +69,22 @@ ruleTester.run("prefer-const", rule, {
"/*exported a*/ let a; function init() { a = foo(); }",
"/*exported a*/ let a = 1",
"let a; if (true) a = 0; foo(a);",
`
(function (a) {
let b;
({ a, b } = obj);
})();
`,
`
(function (a) {
let b;
([ a, b ] = obj);
})();
`,
"var a; { var b; ({ a, b } = obj); }",
"let a; { let b; ({ a, b } = obj); }",
"var a; { var b; ([ a, b ] = obj); }",
"let a; { let b; ([ a, b ] = obj); }",

/*
* The assignment is located in a different scope.
Expand Down

0 comments on commit 240c1a4

Please sign in to comment.