Breaking: Check assignment targets in no-extra-parens (#12490)

* Update: Check assignment targets in no-extra-parens * Add TODO tests
eslint · Jan 17, 2020 · b50179d · b50179d
1 parent de4fa7c
commit b50179d
Show file tree

Hide file tree

Showing 2 changed files with 67 additions and 1 deletion.
diff --git a/lib/rules/no-extra-parens.js b/lib/rules/no-extra-parens.js
@@ -342,6 +342,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
@@ -679,6 +690,12 @@ module.exports = {
                     .forEach(report);
             },
 
+            ArrayPattern(node) {
+                node.elements
+                    .filter(e => canBeAssignmentTarget(e) && hasExcessParens(e))
+                    .forEach(report);
+            },
+
             ArrowFunctionExpression(node) {
                 if (isReturnAssignException(node)) {
                     return;
@@ -705,6 +722,10 @@ module.exports = {
             },
 
             AssignmentExpression(node) {
+                if (canBeAssignmentTarget(node.left) && hasExcessParens(node.left)) {
+                    report(node.left);
+                }
+
                 if (!isReturnAssignException(node) && hasExcessParensWithPrecedence(node.right, precedence(node))) {
                     report(node.right);
                 }
@@ -947,6 +968,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;
@@ -957,6 +987,14 @@ module.exports = {
                 }
             },
 
+            RestElement(node) {
+                const argument = node.argument;
+
+                if (canBeAssignmentTarget(argument) && hasExcessParens(argument)) {
+                    report(argument);
+                }
+            },
+
             ReturnStatement(node) {
                 const returnToken = sourceCode.getFirstToken(node);
 
@@ -1054,7 +1092,11 @@ module.exports = {
             },
 
             AssignmentPattern(node) {
-                const { right } = node;
+                const { left, right } = node;
+
+                if (canBeAssignmentTarget(left) && hasExcessParens(left)) {
+                    report(left);
+                }
 
                 if (right && hasExcessParensWithPrecedence(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
@@ -1403,6 +1403,30 @@ 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 these tests for RestElement's parenthesized arguments in object patterns when that becomes supported by Espree.
+         *
+         * invalid("({ ...(a) } = {})", "({ ...a } = {})", "Identifier"),
+         * invalid("({ ...(a.b) } = {})", "({ ...a.b } = {})", "MemberExpression")
+         */
+
         // https://github.com/eslint/eslint/issues/11706 (also in valid[])
         {
             code: "for ((a = (b in c)); ;);",