Update: Check assignment targets in no-extra-parens

eslint · Oct 24, 2019 · f0a8739 · f0a8739
1 parent 8233873
commit f0a8739
Show file tree

Hide file tree

Showing 2 changed files with 61 additions and 4 deletions.
diff --git a/lib/rules/no-extra-parens.js b/lib/rules/no-extra-parens.js
@@ -320,6 +320,17 @@ module.exports = {
             return node.type === "CallExpression" && node.callee.type === "FunctionExpression";
         }
 
+        /**
+         * Determines if the given node can be the assignment target in destructuring or the LHS of an assignment.
+         * This is to avoid an autofix that could change behavior because parsers mistakenly allow invalid syntax,
+         * such as `(a = b) = c` and `[(a = b) = c] = []`. Ideally, this function shouldn't be necessary.
+         * @param {ASTNode} [node] The node to check
+         * @returns {boolean} `true` if the given node can be a valid assignment target
+         */
+        function canBeAssignmentTarget(node) {
+            return node && (node.type === "Identifier" || node.type === "MemberExpression");
+        }
+
         /**
          * Report the node
          * @param {ASTNode} node node to evaluate
@@ -655,6 +666,12 @@ module.exports = {
                     .forEach(report);
             },
 
+            ArrayPattern(node) {
+                node.elements
+                    .filter(e => canBeAssignmentTarget(e) && hasExcessParens(e))
+                    .forEach(report);
+            },
+
             ArrowFunctionExpression(node) {
                 if (isReturnAssignException(node)) {
                     return;
@@ -681,11 +698,11 @@ module.exports = {
             },
 
             AssignmentExpression(node) {
-                if (isReturnAssignException(node)) {
-                    return;
+                if (canBeAssignmentTarget(node.left) && hasExcessParens(node.left)) {
+                    report(node.left);
                 }
 
-                if (hasExcessParens(node.right) && precedence(node.right) >= precedence(node)) {
+                if (!isReturnAssignException(node) && hasExcessParens(node.right) && precedence(node.right) >= precedence(node)) {
                     report(node.right);
                 }
             },
@@ -918,6 +935,15 @@ module.exports = {
                     }).forEach(property => report(property.value));
             },
 
+            ObjectPattern(node) {
+                node.properties
+                    .filter(property => {
+                        const value = property.value;
+
+                        return canBeAssignmentTarget(value) && hasExcessParens(value);
+                    }).forEach(property => report(property.value));
+            },
+
             Property(node) {
                 if (node.computed) {
                     const { key } = node;
@@ -928,6 +954,14 @@ module.exports = {
                 }
             },
 
+            RestElement(node) {
+                const argument = node.argument;
+
+                if (canBeAssignmentTarget(argument) && hasExcessParens(argument)) {
+                    report(argument);
+                }
+            },
+
             ReturnStatement(node) {
                 const returnToken = sourceCode.getFirstToken(node);
 
@@ -1022,7 +1056,11 @@ module.exports = {
             },
 
             AssignmentPattern(node) {
-                const { right } = node;
+                const { left, right } = node;
+
+                if (canBeAssignmentTarget(left) && hasExcessParens(left)) {
+                    report(left);
+                }
 
                 if (right && hasExcessParens(right) && precedence(right) >= PRECEDENCE_OF_ASSIGNMENT_EXPR) {
                     report(right);

diff --git a/tests/lib/rules/no-extra-parens.js b/tests/lib/rules/no-extra-parens.js
@@ -1362,6 +1362,25 @@ ruleTester.run("no-extra-parens", rule, {
         invalid("const [a = (b)] = []", "const [a = b] = []", "Identifier"),
         invalid("const {a = (b)} = {}", "const {a = b} = {}", "Identifier"),
 
+        // LHS of assigments/Assignment targets
+        invalid("(a) = b", "a = b", "Identifier"),
+        invalid("(a.b) = c", "a.b = c", "MemberExpression"),
+        invalid("(a) += b", "a += b", "Identifier"),
+        invalid("(a.b) >>= c", "a.b >>= c", "MemberExpression"),
+        invalid("[(a) = b] = []", "[a = b] = []", "Identifier"),
+        invalid("[(a.b) = c] = []", "[a.b = c] = []", "MemberExpression"),
+        invalid("({ a: (b) = c } = {})", "({ a: b = c } = {})", "Identifier"),
+        invalid("({ a: (b.c) = d } = {})", "({ a: b.c = d } = {})", "MemberExpression"),
+        invalid("[(a)] = []", "[a] = []", "Identifier"),
+        invalid("[(a.b)] = []", "[a.b] = []", "MemberExpression"),
+        invalid("[,(a),,] = []", "[,a,,] = []", "Identifier"),
+        invalid("[...(a)] = []", "[...a] = []", "Identifier"),
+        invalid("[...(a.b)] = []", "[...a.b] = []", "MemberExpression"),
+        invalid("({ a: (b) } = {})", "({ a: b } = {})", "Identifier"),
+        invalid("({ a: (b.c) } = {})", "({ a: b.c } = {})", "MemberExpression"),
+
+        // TODO: add tests for the RestElement in object patterns when it becomes supported in espree
+
         // https://github.com/eslint/eslint/issues/11706 (also in valid[])
         {
             code: "for ((a = (b in c)); ;);",