Fix: handle array-destructuring with obj assignment in prefer const #8994

VictorHom · 2017-07-24T01:56:46Z

(fixes #8308)

NPM version: 3.10.10
node: v6.9.5

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

What is the purpose of this pull request? (put an "X" next to item)
[X] Bug fix (template)

What changes did you make? (Give an overview)
Added a check in prefer_const where it counts the number of nodes in a destructure group and checks with the number of elements in the parent of the first element in the group. This is how I am checking if everything else in the group can be converted to const.

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

eslintbot · 2017-07-24T01:56:48Z

LGTM

eslintbot · 2017-07-24T01:58:56Z

LGTM

eslintbot · 2017-07-24T01:59:16Z

LGTM

not-an-aardvark

Thanks for the PR! I left a few suggestions for additional tests -- there are some cases that I think aren't handled correctly right now.

not-an-aardvark · 2017-07-24T02:50:04Z

tests/lib/rules/prefer-const.js

+        {
+            code: "let predicate; [typeNode.returnType, predicate] = foo();",
+            options: [{ destructuring: "all" }]
+        },


Can you add a test where the MemberExpression is not the first value in the destructuring group? From looking at the code, I'm not sure this case is handled correctly.

let predicate; [predicate, typeNode.returnType] = foo();

not-an-aardvark · 2017-07-24T02:51:13Z

tests/lib/rules/prefer-const.js

@@ -56,6 +56,12 @@ ruleTester.run("prefer-const", rule, {
        "let a; function foo() { if (a) {} a = bar(); }",
        "let a; function foo() { a = a || bar(); baz(a); }",
        "let a; function foo() { bar(++a); }",
+        "let predicate; [typeNode.returnType, predicate] = foo();",
+        "let predicate; let rest; [typeNode.returnType, predicate, ...rest] = foo();",


Shouldn't these be invalid since they have the default destructuring: "any" option?

That's a good point @not-an-aardvark

In that case, would it make sense to say that the example in #8308 is actually behaving as expected? If destructuring: "any" is on by default, it should report that predicate is not reassigned then.

Hmm, it seems like you're right. However, the bug still exists with destructuring: all, right?

not-an-aardvark · 2017-07-24T02:53:24Z

tests/lib/rules/prefer-const.js

+        {
+            code: "let predicate; [typeNode.returnType, predicate] = foo();",
+            options: [{ destructuring: "all" }]
+        },


Can you add a test where the destructuring uses an ObjectPattern rather than an ArrayPattern? From looking at the code, I'm not sure this case is handled correctly.

let foo; ({ x: bar.baz, y: foo } = qux);

not-an-aardvark · 2017-07-24T02:57:21Z

tests/lib/rules/prefer-const.js

+        {
+            code: "let predicate; [typeNode.returnType, predicate] = foo();",
+            options: [{ destructuring: "all" }]
+        },


Can you add a test where the declarations use nested destructuring? From looking at the code, I'm not sure this case is handled correctly.

let foo; [foo, [bar.baz]] = qux;

eslintbot · 2017-07-26T02:50:38Z

LGTM

eslintbot · 2017-07-29T16:33:33Z

LGTM

not-an-aardvark · 2017-07-30T01:22:24Z

lib/rules/prefer-const.js

@@ -199,6 +200,28 @@ function groupByDestructuring(variables, ignoreReadBeforeAssign) {
        }
    }

+    if (!checkingMixedDestructuring) {


Can you move this out of the groupByDestructuring function (maybe into a different helper function)? It seems like a separate concern from grouping by destructuring.

not-an-aardvark · 2017-07-30T01:30:34Z

lib/rules/prefer-const.js

+            const destructureElement = destructureGroup[0];
+            let elementsInHost;
+
+            if (destructureElement && destructureElement.parent) {


Is this if statement necessary? It seems like destructureElement and destructureElement.parent will always exist.

It looks like one test case let {a = 0, b} = obj, c = a; b = a; has destrucutreGroup as [null]

Not clear on the reason.

not-an-aardvark · 2017-07-30T02:21:35Z

lib/rules/prefer-const.js

+                }
+            }
+
+            if (elementsInHost && elementsInHost.length && elementsInHost.length !== destructureGroup.length) {


I think this strategy will fail for things like

let foo; [foo, []] = qux;

or like this:

let foo, bar; [foo, [bar, baz.qux]] = qux;

With the desctructuring: all option, the first case should be invalid, and the second case should be valid.

I think this is going to be a problem with the approach of checking the number of elements in a destructuring pattern, because it won't be able to account for nested destructuring.

kaicataldo · 2017-09-10T22:07:50Z

Friendly ping - is there anything we can help with here?

(fixes eslint#8308)

VictorHom · 2017-09-23T20:31:37Z

totally forgot about this pr. I will take another look from comments.

eslintbot · 2017-09-24T03:25:06Z

LGTM

VictorHom · 2017-10-05T03:15:24Z

@not-an-aardvark when you have some time, could you review this? I updated base on your feedback by going through the nested structure recursively. Let me know if that could work

not-an-aardvark · 2017-10-06T04:29:33Z

lib/rules/prefer-const.js

+        const numberOfElements = node.elements.length;
+
+        for (let i = 0; i < numberOfElements; i++) {
+            if (!node.elements[i].elements && node.elements[i].type === "Identifier") {


I'm a bit confused about this line -- is the !node.elements[i].elements part redundant? It seems like an Identifier node would never have an elements property.

not-an-aardvark · 2017-10-06T04:34:12Z

lib/rules/prefer-const.js

+
+                verifyAllDestructuring(identifierMap, checkingMixedDestructuring, destructureCount, destructureIdentifier).forEach(checkGroup);
+            },
+            "ExpressionStatement[expression.left.type = /ArrayPattern|ObjectPattern/]"(node) {


Should this be AssignmentExpression rather than ExpressionStatement? Ostensibly we also want to catch assignments that aren't in an ExpressionStatement, e.g. foo([bar] = 1).

not-an-aardvark · 2017-10-06T04:36:24Z

lib/rules/prefer-const.js

+ *      in IdentifierMap
+ * @returns {Map<ASTNode, ASTNode[]>} Grouped identifier nodes.
+ */
+function verifyAllDestructuring(identifierMap, checkingMixedDestructuring, destructureCount, destructureIdentifier) {


Would it be possible for this function to return a new Map instead? In general I don't think it's a good idea for functions to mutate their arguments.

not-an-aardvark · 2017-10-06T04:40:36Z

lib/rules/prefer-const.js

+ * @param {integer|null} destructureCount count of destructure terms.
+ * @param {ASTNode[]|null} destructureIdentifier list of Identifier nodes to check 
+ *      in IdentifierMap
+ * @returns {Map<ASTNode, ASTNode[]>} Grouped identifier nodes.


Can you make this description of the return type more detailed? For example, how are the nodes grouped?

not-an-aardvark · 2017-10-06T04:43:55Z

lib/rules/prefer-const.js

-                groupByDestructuring(variables, ignoreReadBeforeAssign).forEach(checkGroup);
+                const identifierMap = groupByDestructuring(variables, ignoreReadBeforeAssign);
+
+                verifyAllDestructuring(identifierMap, checkingMixedDestructuring, destructureCount, destructureIdentifier).forEach(checkGroup);


This doesn't seem right to me -- destructureCount is getting reassigned on every ExpressionStatement, so only the final value is getting used in the Program:exit listener. It seems like it doesn't make sense for the previous values of destructureCount to get discarded.

not-an-aardvark · 2017-10-06T04:46:05Z

Thanks! This mostly seems good -- I left a few more comments. If it would be helpful, I'm planning to look into this a bit more deeply when I have time (so that I can make more concrete implementation suggestions rather than pointing out things that look incorrect).

VictorHom · 2017-10-07T00:57:52Z

@not-an-aardvark I think you looking into it when you have time will help immensely 👍

platinumazure · 2018-01-20T00:30:44Z

@not-an-aardvark Where are we on this?

not-an-aardvark · 2018-01-20T00:35:33Z

I still plan to look into it, sorry about the delay.

not-an-aardvark suggested changes Jul 24, 2017

View reviewed changes

VictorHom changed the title ~~(WIP) Fix: handle array-destructuring with obj assignment in prefer const~~ Fix: handle array-destructuring with obj assignment in prefer const Jul 29, 2017

not-an-aardvark suggested changes Jul 30, 2017

View reviewed changes

not-an-aardvark added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly rule Relates to ESLint's core rules labels Aug 1, 2017

VictorHom added 5 commits September 23, 2017 16:30

Fix: handle array-destructuring with obj assignment in prefer const

08616bf

(fixes eslint#8308)

add a blank space to trigger github tests

0048eb2

remove a blank line

c21dad6

add tests; one still failing

bd70c1c

handle object destructuring

da70054

update for destructuring cases in prefer-const

695b006

VictorHom force-pushed the destructuring_prefer_const_err branch from f9f6582 to 695b006 Compare September 24, 2017 03:25

not-an-aardvark reviewed Oct 6, 2017

View reviewed changes

VictorHom closed this May 26, 2018

VictorHom deleted the destructuring_prefer_const_err branch May 26, 2018 22:16

eslint-deprecated bot locked and limited conversation to collaborators Nov 24, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Nov 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: handle array-destructuring with obj assignment in prefer const #8994

Fix: handle array-destructuring with obj assignment in prefer const #8994

VictorHom commented Jul 24, 2017

eslintbot commented Jul 24, 2017

eslintbot commented Jul 24, 2017

eslintbot commented Jul 24, 2017

not-an-aardvark left a comment

not-an-aardvark Jul 24, 2017

not-an-aardvark Jul 24, 2017

VictorHom Jul 24, 2017

not-an-aardvark Jul 24, 2017

not-an-aardvark Jul 24, 2017

not-an-aardvark Jul 24, 2017

eslintbot commented Jul 26, 2017

eslintbot commented Jul 29, 2017

not-an-aardvark Jul 30, 2017

not-an-aardvark Jul 30, 2017

VictorHom Sep 23, 2017

not-an-aardvark Jul 30, 2017

kaicataldo commented Sep 10, 2017

VictorHom commented Sep 23, 2017

eslintbot commented Sep 24, 2017

VictorHom commented Oct 5, 2017

not-an-aardvark Oct 6, 2017

not-an-aardvark Oct 6, 2017

not-an-aardvark Oct 6, 2017

not-an-aardvark Oct 6, 2017

not-an-aardvark Oct 6, 2017

not-an-aardvark commented Oct 6, 2017

VictorHom commented Oct 7, 2017

platinumazure commented Jan 20, 2018

not-an-aardvark commented Jan 20, 2018

Fix: handle array-destructuring with obj assignment in prefer const #8994

Fix: handle array-destructuring with obj assignment in prefer const #8994

Conversation

VictorHom commented Jul 24, 2017

eslintbot commented Jul 24, 2017

eslintbot commented Jul 24, 2017

eslintbot commented Jul 24, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Jul 26, 2017

eslintbot commented Jul 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaicataldo commented Sep 10, 2017

VictorHom commented Sep 23, 2017

eslintbot commented Sep 24, 2017

VictorHom commented Oct 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

not-an-aardvark commented Oct 6, 2017

VictorHom commented Oct 7, 2017

platinumazure commented Jan 20, 2018

not-an-aardvark commented Jan 20, 2018