fix: disallow all parenthesized pattern except parsing LHS (#12327)

* fix: disallow all parenthesized pattern except parsing LHS * fix: forStatement requires LHS * simplify toAssignable * add more test cases * fix: pass through isLHS on object property and assignment expression * fix: record parenthesized identifier error for LHS * remove duplicated skipped tests * fix: do not record errors on ancestry arrow head * Update packages/babel-parser/src/util/expression-scope.js Co-authored-by: Brian Ng <bng412@gmail.com> Co-authored-by: Brian Ng <bng412@gmail.com>
babel · Nov 10, 2020 · 5bbad89 · 5bbad89
1 parent 08c7280
commit 5bbad89
Show file tree

Hide file tree

Showing 93 changed files with 2,273 additions and 115 deletions.
diff --git a/packages/babel-parser/src/parser/expression.js b/packages/babel-parser/src/parser/expression.js
@@ -278,7 +278,7 @@ export default class ExpressionParser extends LValParser {
       node.operator = operator;
 
       if (this.match(tt.eq)) {
-        node.left = this.toAssignable(left);
+        node.left = this.toAssignable(left, /* isLHS */ true);
         refExpressionErrors.doubleProto = -1; // reset because double __proto__ is valid in assignment expression
       } else {
         node.left = left;
@@ -842,7 +842,6 @@ export default class ExpressionParser extends LValParser {
     nodeForExtra?: ?N.Node,
   ): $ReadOnlyArray<?N.Expression> {
     const elts = [];
-    let innerParenStart;
     let first = true;
     const oldInFSharpPipelineDirectBody = this.state.inFSharpPipelineDirectBody;
     this.state.inFSharpPipelineDirectBody = false;
@@ -875,12 +874,6 @@ export default class ExpressionParser extends LValParser {
         }
       }
 
-      // we need to make sure that if this is an async arrow functions,
-      // that we don't allow inner parens inside the params
-      if (this.match(tt.parenL) && !innerParenStart) {
-        innerParenStart = this.state.start;
-      }
-
       elts.push(
         this.parseExprListItem(
           false,
@@ -891,11 +884,6 @@ export default class ExpressionParser extends LValParser {
       );
     }
 
-    // we found an async arrow function so let's not allow any inner parens
-    if (possibleAsyncArrow && innerParenStart && this.shouldParseAsyncArrow()) {
-      this.unexpected();
-    }
-
     this.state.inFSharpPipelineDirectBody = oldInFSharpPipelineDirectBody;
 
     return elts;
@@ -1421,12 +1409,6 @@ export default class ExpressionParser extends LValParser {
     ) {
       this.expressionScope.validateAsPattern();
       this.expressionScope.exit();
-      for (const param of exprList) {
-        if (param.extra && param.extra.parenthesized) {
-          this.unexpected(param.extra.parenStart);
-        }
-      }
-
       this.parseArrowExpression(arrowNode, exprList, false);
       return arrowNode;
     }
@@ -2056,7 +2038,7 @@ export default class ExpressionParser extends LValParser {
     params: N.Expression[],
     trailingCommaPos: ?number,
   ): void {
-    node.params = this.toAssignableList(params, trailingCommaPos);
+    node.params = this.toAssignableList(params, trailingCommaPos, false);
   }
 
   parseFunctionBodyAndFinish(

diff --git a/packages/babel-parser/src/parser/lval.js b/packages/babel-parser/src/parser/lval.js
@@ -1,5 +1,6 @@
 // @flow
 
+/*:: declare var invariant; */
 import * as charCodes from "charcodes";
 import { types as tt, type TokenType } from "../tokenizer/types";
 import type {
@@ -24,7 +25,7 @@ import { type BindingTypes, BIND_NONE } from "../util/scopeflags";
 import { ExpressionErrors } from "./util";
 import { Errors } from "./error";
 
-const unwrapParenthesizedExpression = (node: Node) => {
+const unwrapParenthesizedExpression = (node: Node): Node => {
   return node.type === "ParenthesizedExpression"
     ? unwrapParenthesizedExpression(node.expression)
     : node;
@@ -51,19 +52,45 @@ export default class LValParser extends NodeUtils {
   +parseDecorator: () => Decorator;
   */
 
-  // Convert existing expression atom to assignable pattern
-  // if possible.
-  // NOTE: There is a corresponding "isAssignable" method in flow.js.
-  // When this one is updated, please check if also that one needs to be updated.
+  /**
+   * Convert existing expression atom to assignable pattern
+   * if possible. Also checks invalid destructuring targets:
 
-  toAssignable(node: Node): Node {
+   - Parenthesized Destructuring patterns
+   - RestElement is not the last element
+   - Missing `=` in assignment pattern
+
+   NOTE: There is a corresponding "isAssignable" method in flow.js.
+   When this one is updated, please check if also that one needs to be updated.
+
+   * @param {Node} node The expression atom
+   * @param {boolean} [isLHS=false] Whether we are parsing a LeftHandSideExpression. If isLHS is `true`, the following cases are allowed:
+                                    `[(a)] = [0]`, `[(a.b)] = [0]`
+
+   * @returns {Node} The converted assignable pattern
+   * @memberof LValParser
+   */
+  toAssignable(node: Node, isLHS: boolean = false): Node {
     let parenthesized = undefined;
     if (node.type === "ParenthesizedExpression" || node.extra?.parenthesized) {
       parenthesized = unwrapParenthesizedExpression(node);
-      if (
-        parenthesized.type !== "Identifier" &&
-        parenthesized.type !== "MemberExpression"
-      ) {
+      if (isLHS) {
+        // an LHS can be reinterpreted to a binding pattern but not vice versa.
+        // therefore a parenthesized identifier is ambiguous until we are sure it is an assignment expression
+        // i.e. `([(a) = []] = []) => {}`
+        // see also `recordParenthesizedIdentifierError` signature in packages/babel-parser/src/util/expression-scope.js
+        if (parenthesized.type === "Identifier") {
+          this.expressionScope.recordParenthesizedIdentifierError(
+            node.start,
+            Errors.InvalidParenthesizedAssignment,
+          );
+        } else if (parenthesized.type !== "MemberExpression") {
+          // A parenthesized member expression can be in LHS but not in pattern.
+          // If the LHS is later interpreted as a pattern, `checkLVal` will throw for member expression binding
+          // i.e. `([(a.b) = []] = []) => {}`
+          this.raise(node.start, Errors.InvalidParenthesizedAssignment);
+        }
+      } else {
         this.raise(node.start, Errors.InvalidParenthesizedAssignment);
       }
     }
@@ -84,7 +111,7 @@ export default class LValParser extends NodeUtils {
         ) {
           const prop = node.properties[i];
           const isLast = i === last;
-          this.toAssignableObjectExpressionProp(prop, isLast);
+          this.toAssignableObjectExpressionProp(prop, isLast, isLHS);
 
           if (
             isLast &&
@@ -97,21 +124,21 @@ export default class LValParser extends NodeUtils {
         break;
 
       case "ObjectProperty":
-        this.toAssignable(node.value);
+        this.toAssignable(node.value, isLHS);
         break;
 
       case "SpreadElement": {
         this.checkToRestConversion(node);
 
         node.type = "RestElement";
         const arg = node.argument;
-        this.toAssignable(arg);
+        this.toAssignable(arg, isLHS);
         break;
       }
 
       case "ArrayExpression":
         node.type = "ArrayPattern";
-        this.toAssignableList(node.elements, node.extra?.trailingComma);
+        this.toAssignableList(node.elements, node.extra?.trailingComma, isLHS);
         break;
 
       case "AssignmentExpression":
@@ -121,11 +148,12 @@ export default class LValParser extends NodeUtils {
 
         node.type = "AssignmentPattern";
         delete node.operator;
-        this.toAssignable(node.left);
+        this.toAssignable(node.left, isLHS);
         break;
 
       case "ParenthesizedExpression":
-        this.toAssignable(((parenthesized: any): Expression));
+        /*::invariant (parenthesized !== undefined) */
+        this.toAssignable(parenthesized, isLHS);
         break;
 
       default:
@@ -135,7 +163,11 @@ export default class LValParser extends NodeUtils {
     return node;
   }
 
-  toAssignableObjectExpressionProp(prop: Node, isLast: boolean) {
+  toAssignableObjectExpressionProp(
+    prop: Node,
+    isLast: boolean,
+    isLHS: boolean,
+  ) {
     if (prop.type === "ObjectMethod") {
       const error =
         prop.kind === "get" || prop.kind === "set"
@@ -148,7 +180,7 @@ export default class LValParser extends NodeUtils {
     } else if (prop.type === "SpreadElement" && !isLast) {
       this.raiseRestNotLast(prop.start);
     } else {
-      this.toAssignable(prop);
+      this.toAssignable(prop, isLHS);
     }
   }
 
@@ -157,6 +189,7 @@ export default class LValParser extends NodeUtils {
   toAssignableList(
     exprList: Expression[],
     trailingCommaPos?: ?number,
+    isLHS: boolean,
   ): $ReadOnlyArray<Pattern> {
     let end = exprList.length;
     if (end) {
@@ -165,8 +198,9 @@ export default class LValParser extends NodeUtils {
         --end;
       } else if (last?.type === "SpreadElement") {
         last.type = "RestElement";
-        const arg = last.argument;
-        this.toAssignable(arg);
+        let arg = last.argument;
+        this.toAssignable(arg, isLHS);
+        arg = unwrapParenthesizedExpression(arg);
         if (
           arg.type !== "Identifier" &&
           arg.type !== "MemberExpression" &&
@@ -186,7 +220,7 @@ export default class LValParser extends NodeUtils {
     for (let i = 0; i < end; i++) {
       const elt = exprList[i];
       if (elt) {
-        this.toAssignable(elt);
+        this.toAssignable(elt, isLHS);
         if (elt.type === "RestElement") {
           this.raiseRestNotLast(elt.start);
         }

diff --git a/packages/babel-parser/src/parser/statement.js b/packages/babel-parser/src/parser/statement.js
@@ -528,7 +528,7 @@ export default class StatementParser extends ExpressionParser {
     const refExpressionErrors = new ExpressionErrors();
     const init = this.parseExpression(true, refExpressionErrors);
     if (this.match(tt._in) || this.isContextual("of")) {
-      this.toAssignable(init);
+      this.toAssignable(init, /* isLHS */ true);
       const description = this.isContextual("of")
         ? "for-of statement"
         : "for-in statement";

diff --git a/packages/babel-parser/src/plugins/estree.js b/packages/babel-parser/src/plugins/estree.js
@@ -341,23 +341,23 @@ export default (superClass: Class<Parser>): Class<Parser> =>
       return (node: any);
     }
 
-    toAssignable(node: N.Node): N.Node {
+    toAssignable(node: N.Node, isLHS: boolean = false): N.Node {
       if (isSimpleProperty(node)) {
         this.toAssignable(node.value);
 
         return node;
       }
 
-      return super.toAssignable(node);
+      return super.toAssignable(node, isLHS);
     }
 
-    toAssignableObjectExpressionProp(prop: N.Node, isLast: boolean) {
+    toAssignableObjectExpressionProp(prop: N.Node, ...args) {
       if (prop.kind === "get" || prop.kind === "set") {
         throw this.raise(prop.key.start, Errors.PatternHasAccessor);
       } else if (prop.method) {
         throw this.raise(prop.key.start, Errors.PatternHasMethod);
       } else {
-        super.toAssignableObjectExpressionProp(prop, isLast);
+        super.toAssignableObjectExpressionProp(prop, ...args);
       }
     }
 

diff --git a/packages/babel-parser/src/plugins/flow.js b/packages/babel-parser/src/plugins/flow.js
@@ -1955,6 +1955,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
         // has not been converted yet.
         ((node.params: any): N.Expression[]),
         node.extra?.trailingComma,
+        /* isLHS */ false,
       );
       // Enter scope, as checkParams defines bindings
       this.scope.enter(SCOPE_FUNCTION | SCOPE_ARROW);
@@ -2192,26 +2193,27 @@ export default (superClass: Class<Parser>): Class<Parser> =>
       }
     }
 
-    toAssignable(node: N.Node): N.Node {
+    toAssignable(node: N.Node, isLHS: boolean = false): N.Node {
       if (node.type === "TypeCastExpression") {
-        return super.toAssignable(this.typeCastToParameter(node));
+        return super.toAssignable(this.typeCastToParameter(node), isLHS);
       } else {
-        return super.toAssignable(node);
+        return super.toAssignable(node, isLHS);
       }
     }
 
     // turn type casts that we found in function parameter head into type annotated params
     toAssignableList(
       exprList: N.Expression[],
       trailingCommaPos?: ?number,
+      isLHS: boolean,
     ): $ReadOnlyArray<N.Pattern> {
       for (let i = 0; i < exprList.length; i++) {
         const expr = exprList[i];
         if (expr?.type === "TypeCastExpression") {
           exprList[i] = this.typeCastToParameter(expr);
         }
       }
-      return super.toAssignableList(exprList, trailingCommaPos);
+      return super.toAssignableList(exprList, trailingCommaPos, isLHS);
     }
 
     // this is a list of nodes, from something like a call expression, we need to filter the

diff --git a/packages/babel-parser/src/plugins/typescript/index.js b/packages/babel-parser/src/plugins/typescript/index.js
@@ -2596,19 +2596,19 @@ export default (superClass: Class<Parser>): Class<Parser> =>
       return param;
     }
 
-    toAssignable(node: N.Node): N.Node {
+    toAssignable(node: N.Node, isLHS: boolean = false): N.Node {
       switch (node.type) {
         case "TSTypeCastExpression":
-          return super.toAssignable(this.typeCastToParameter(node));
+          return super.toAssignable(this.typeCastToParameter(node), isLHS);
         case "TSParameterProperty":
-          return super.toAssignable(node);
+          return super.toAssignable(node, isLHS);
         case "TSAsExpression":
         case "TSNonNullExpression":
         case "TSTypeAssertion":
-          node.expression = this.toAssignable(node.expression);
+          node.expression = this.toAssignable(node.expression, isLHS);
           return node;
         default:
-          return super.toAssignable(node);
+          return super.toAssignable(node, isLHS);
       }
     }
 

diff --git a/packages/babel-parser/src/util/expression-scope.js b/packages/babel-parser/src/util/expression-scope.js
@@ -20,6 +20,7 @@ some expression scopes and thrown later when we know what the ambigous pattern i
 - AwaitBindingIdentifier
 - AwaitExpressionFormalParameter
 - YieldInParameter
+- InvalidParenthesizedAssignment when parenthesized is an identifier
 
 There are four different expression scope
 - Expression
@@ -130,6 +131,41 @@ export default class ExpressionScopeHandler {
     /* eslint-disable @babel/development-internal/dry-error-messages */
     this.raise(pos, message);
   }
+
+  /**
+   * Record parenthesized identifier errors
+   *
+   * A parenthesized identifier in LHS can be ambiguous because the assignment
+   * can be transformed to an assignable later, but not vice versa:
+   * For example, in `([(a) = []] = []) => {}`, we think `(a) = []` is an LHS in `[(a) = []]`,
+   * an LHS within `[(a) = []] = []`. However the LHS chain is then transformed by toAssignable,
+   * and we should throw assignment `(a)`, which is only valid in LHS. Hence we record the
+   * location of parenthesized `(a)` to current scope if it is one of MaybeArrowParameterDeclaration
+   * and MaybeAsyncArrowParameterDeclaration
+   *
+   * Unlike `recordParameterInitializerError`, we don't record to ancestry scope because we
+   * validate arrow head parsing scope before exit, and then the LHS will be unambiguous:
+   * For example, in `( x = ( [(a) = []] = [] ) ) => {}`, we should not record `(a)` in `( x = ... ) =>`
+   * arrow scope because when we finish parsing `( [(a) = []] = [] )`, it is an unambiguous assignment
+   * expression and can not be cast to pattern
+   * @param {number} pos
+   * @param {string} message
+   * @returns {void}
+   * @memberof ExpressionScopeHandler
+   */
+  recordParenthesizedIdentifierError(pos: number, message: string): void {
+    const { stack } = this;
+    const scope: ExpressionScope = stack[stack.length - 1];
+    if (scope.isCertainlyParameterDeclaration()) {
+      this.raise(pos, message);
+    } else if (scope.canBeArrowParameterDeclaration()) {
+      /*:: invariant(scope instanceof ArrowHeadParsingScope) */
+      scope.recordDeclarationError(pos, message);
+    } else {
+      return;
+    }
+  }
+
   /**
    * Record likely async arrow parameter errors
    *

diff --git a/.../create-parenthesized-expressions/invalid-parenthesized-assignment-pattern-3/options.json b/.../create-parenthesized-expressions/invalid-parenthesized-assignment-pattern-3/options.json
diff --git a/.../create-parenthesized-expressions/invalid-parenthesized-assignment-pattern-4/options.json b/.../create-parenthesized-expressions/invalid-parenthesized-assignment-pattern-4/options.json
diff --git a/.../create-parenthesized-expressions/invalid-parenthesized-assignment-pattern-5/options.json b/.../create-parenthesized-expressions/invalid-parenthesized-assignment-pattern-5/options.json