Trailing comma after rest - The final fix (#10491)

* [parser] Track trailing commas in extras instead of state * Update existing tests * Update test262 whitelist * Improve error message and location * nit * Use lookaheadCharCode
2019-10-08 23:08:50 +02:00
parent c7add11fdc
commit 34937f13d5
46 changed files with 226 additions and 100 deletions
--- a/packages/babel-parser/src/parser/expression.js
+++ b/packages/babel-parser/src/parser/expression.js
@@ -168,9 +168,6 @@ export default class ExpressionParser extends LValParser {
      }
    }

-    const oldCommaAfterSpreadAt = this.state.commaAfterSpreadAt;
-    this.state.commaAfterSpreadAt = -1;
-
    let failOnShorthandAssign;
    if (refShorthandDefaultPos) {
      failOnShorthandAssign = false;
@@ -230,9 +227,6 @@ export default class ExpressionParser extends LValParser {
        );
      }

-      if (patternErrorMsg) this.checkCommaAfterRestFromSpread();
-      this.state.commaAfterSpreadAt = oldCommaAfterSpreadAt;
-
      this.next();
      node.right = this.parseMaybeAssign(noIn);
      return this.finishNode(node, "AssignmentExpression");
@@ -240,8 +234,6 @@ export default class ExpressionParser extends LValParser {
      this.unexpected(refShorthandDefaultPos.start);
    }

-    this.state.commaAfterSpreadAt = oldCommaAfterSpreadAt;
-
    return left;
  }

@@ -680,14 +672,12 @@ export default class ExpressionParser extends LValParser {
      let node = this.startNodeAt(startPos, startLoc);
      node.callee = base;

-      const oldCommaAfterSpreadAt = this.state.commaAfterSpreadAt;
-      this.state.commaAfterSpreadAt = -1;
-
      node.arguments = this.parseCallExpressionArguments(
        tt.parenR,
        state.maybeAsyncArrow,
        base.type === "Import",
        base.type !== "Super",
+        node,
      );
      if (!state.optionalChainMember) {
        this.finishCallExpression(node);
@@ -698,8 +688,6 @@ export default class ExpressionParser extends LValParser {
      if (state.maybeAsyncArrow && this.shouldParseAsyncArrow()) {
        state.stop = true;

-        this.checkCommaAfterRestFromSpread();
-
        node = this.parseAsyncArrowFromCallExpression(
          this.startNodeAt(startPos, startLoc),
          node,
@@ -743,7 +731,6 @@ export default class ExpressionParser extends LValParser {
      }

      this.state.maybeInArrowParameters = oldMaybeInArrowParameters;
-      this.state.commaAfterSpreadAt = oldCommaAfterSpreadAt;

      return node;
    } else if (this.match(tt.backQuote)) {
@@ -825,6 +812,7 @@ export default class ExpressionParser extends LValParser {
    possibleAsyncArrow: boolean,
    dynamicImport?: boolean,
    allowPlaceholder?: boolean,
+    nodeForExtra?: ?N.Node,
  ): $ReadOnlyArray<?N.Expression> {
    const elts = [];
    let innerParenStart;
@@ -837,13 +825,21 @@ export default class ExpressionParser extends LValParser {
        first = false;
      } else {
        this.expect(tt.comma);
-        if (this.eat(close)) {
+        if (this.match(close)) {
          if (dynamicImport) {
            this.raise(
              this.state.lastTokStart,
              "Trailing comma is disallowed inside import(...) arguments",
            );
          }
+          if (nodeForExtra) {
+            this.addExtra(
+              nodeForExtra,
+              "trailingComma",
+              this.state.lastTokStart,
+            );
+          }
+          this.next();
          break;
        }
      }
@@ -883,7 +879,12 @@ export default class ExpressionParser extends LValParser {
    call: N.CallExpression,
  ): N.ArrowFunctionExpression {
    this.expect(tt.arrow);
-    this.parseArrowExpression(node, call.arguments, true);
+    this.parseArrowExpression(
+      node,
+      call.arguments,
+      true,
+      call.extra?.trailingComma,
+    );
    return node;
  }

@@ -1046,6 +1047,7 @@ export default class ExpressionParser extends LValParser {
          tt.bracketR,
          true,
          refShorthandDefaultPos,
+          node,
        );
        if (!this.state.maybeInArrowParameters) {
          // This could be an array pattern:
@@ -1289,7 +1291,7 @@ export default class ExpressionParser extends LValParser {
          ),
        );

-        this.checkCommaAfterRest();
+        this.checkCommaAfterRest(charCodes.rightParenthesis);

        break;
      } else {
@@ -1504,7 +1506,11 @@ export default class ExpressionParser extends LValParser {
        first = false;
      } else {
        this.expect(tt.comma);
-        if (this.eat(tt.braceR)) break;
+        if (this.match(tt.braceR)) {
+          this.addExtra(node, "trailingComma", this.state.lastTokStart);
+          this.next();
+          break;
+        }
      }

      const prop = this.parseObjectMember(isPattern, refShorthandDefaultPos);
@@ -1572,7 +1578,7 @@ export default class ExpressionParser extends LValParser {
        this.next();
        // Don't use parseRestBinding() as we only allow Identifier here.
        prop.argument = this.parseIdentifier();
-        this.checkCommaAfterRest();
+        this.checkCommaAfterRest(charCodes.rightCurlyBrace);
        return this.finishNode(prop, "RestElement");
      }

@@ -1856,6 +1862,7 @@ export default class ExpressionParser extends LValParser {
    node: N.ArrowFunctionExpression,
    params: ?(N.Expression[]),
    isAsync: boolean,
+    trailingCommaPos: ?number,
  ): N.ArrowFunctionExpression {
    this.scope.enter(functionFlags(isAsync, false) | SCOPE_ARROW);
    this.initFunction(node, isAsync);
@@ -1867,7 +1874,7 @@ export default class ExpressionParser extends LValParser {
    this.state.yieldPos = -1;
    this.state.awaitPos = -1;

-    if (params) this.setArrowFunctionParameters(node, params);
+    if (params) this.setArrowFunctionParameters(node, params, trailingCommaPos);
    this.parseFunctionBody(node, true);

    this.scope.exit();
@@ -1881,11 +1888,13 @@ export default class ExpressionParser extends LValParser {
  setArrowFunctionParameters(
    node: N.ArrowFunctionExpression,
    params: N.Expression[],
+    trailingCommaPos: ?number,
  ): void {
    node.params = this.toAssignableList(
      params,
      true,
      "arrow function parameters",
+      trailingCommaPos,
    );
  }

@@ -2012,6 +2021,7 @@ export default class ExpressionParser extends LValParser {
    close: TokenType,
    allowEmpty?: boolean,
    refShorthandDefaultPos?: ?Pos,
+    nodeForExtra?: ?N.Node,
  ): $ReadOnlyArray<?N.Expression> {
    const elts = [];
    let first = true;
@@ -2021,7 +2031,17 @@ export default class ExpressionParser extends LValParser {
        first = false;
      } else {
        this.expect(tt.comma);
-        if (this.eat(close)) break;
+        if (this.match(close)) {
+          if (nodeForExtra) {
+            this.addExtra(
+              nodeForExtra,
+              "trailingComma",
+              this.state.lastTokStart,
+            );
+          }
+          this.next();
+          break;
+        }
      }

      elts.push(this.parseExprListItem(allowEmpty, refShorthandDefaultPos));
--- a/packages/babel-parser/src/parser/lval.js
+++ b/packages/babel-parser/src/parser/lval.js
@@ -1,5 +1,6 @@
 // @flow

+import * as charCodes from "charcodes";
 import { types as tt, type TokenType } from "../tokenizer/types";
 import type {
  TSParameterProperty,
@@ -60,6 +61,14 @@ export default class LValParser extends NodeUtils {
            const prop = node.properties[i];
            const isLast = i === last;
            this.toAssignableObjectExpressionProp(prop, isBinding, isLast);
+
+            if (
+              isLast &&
+              prop.type === "RestElement" &&
+              node.extra?.trailingComma
+            ) {
+              this.raiseRestNotLast(node.extra.trailingComma);
+            }
          }
          break;

@@ -78,7 +87,12 @@ export default class LValParser extends NodeUtils {

        case "ArrayExpression":
          node.type = "ArrayPattern";
-          this.toAssignableList(node.elements, isBinding, contextDescription);
+          this.toAssignableList(
+            node.elements,
+            isBinding,
+            contextDescription,
+            node.extra?.trailingComma,
+          );
          break;

        case "AssignmentExpression":
@@ -142,6 +156,7 @@ export default class LValParser extends NodeUtils {
    exprList: Expression[],
    isBinding: ?boolean,
    contextDescription: string,
+    trailingCommaPos?: ?number,
  ): $ReadOnlyArray<Pattern> {
    let end = exprList.length;
    if (end) {
@@ -160,6 +175,11 @@ export default class LValParser extends NodeUtils {
        ) {
          this.unexpected(arg.start);
        }
+
+        if (trailingCommaPos) {
+          this.raiseTrailingCommaAfterRest(trailingCommaPos);
+        }
+
        --end;
      }
    }
@@ -213,11 +233,6 @@ export default class LValParser extends NodeUtils {
      undefined,
      refNeedsArrowPos,
    );
-
-    if (this.state.commaAfterSpreadAt === -1 && this.match(tt.comma)) {
-      this.state.commaAfterSpreadAt = this.state.start;
-    }
-
    return this.finishNode(node, "SpreadElement");
  }

@@ -234,7 +249,11 @@ export default class LValParser extends NodeUtils {
      case tt.bracketL: {
        const node = this.startNode();
        this.next();
-        node.elements = this.parseBindingList(tt.bracketR, true);
+        node.elements = this.parseBindingList(
+          tt.bracketR,
+          charCodes.rightSquareBracket,
+          true,
+        );
        return this.finishNode(node, "ArrayPattern");
      }

@@ -247,6 +266,7 @@ export default class LValParser extends NodeUtils {

  parseBindingList(
    close: TokenType,
+    closeCharCode: $Values<typeof charCodes>,
    allowEmpty?: boolean,
    allowModifiers?: boolean,
  ): $ReadOnlyArray<Pattern | TSParameterProperty> {
@@ -265,7 +285,7 @@ export default class LValParser extends NodeUtils {
        break;
      } else if (this.match(tt.ellipsis)) {
        elts.push(this.parseAssignableListItemTypes(this.parseRestBinding()));
-        this.checkCommaAfterRest();
+        this.checkCommaAfterRest(closeCharCode);
        this.expect(close);
        break;
      } else {
@@ -455,19 +475,21 @@ export default class LValParser extends NodeUtils {
    }
  }

-  checkCommaAfterRest(): void {
+  checkCommaAfterRest(close: $Values<typeof charCodes>): void {
    if (this.match(tt.comma)) {
-      this.raiseRestNotLast(this.state.start);
-    }
-  }
-
-  checkCommaAfterRestFromSpread(): void {
-    if (this.state.commaAfterSpreadAt > -1) {
-      this.raiseRestNotLast(this.state.commaAfterSpreadAt);
+      if (this.lookaheadCharCode() === close) {
+        this.raiseTrailingCommaAfterRest(this.state.start);
+      } else {
+        this.raiseRestNotLast(this.state.start);
+      }
    }
  }

  raiseRestNotLast(pos: number) {
    this.raise(pos, `Rest element must be last element`);
  }
+
+  raiseTrailingCommaAfterRest(pos: number) {
+    this.raise(pos, `Unexpected trailing comma after rest element`);
+  }
 }
--- a/packages/babel-parser/src/parser/statement.js
+++ b/packages/babel-parser/src/parser/statement.js
@@ -1105,6 +1105,7 @@ export default class StatementParser extends ExpressionParser {
    this.expect(tt.parenL);
    node.params = this.parseBindingList(
      tt.parenR,
+      charCodes.rightParenthesis,
      /* allowEmpty */ false,
      allowModifiers,
    );
--- a/packages/babel-parser/src/plugins/flow.js
+++ b/packages/babel-parser/src/plugins/flow.js
@@ -1798,6 +1798,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
              ((node.params: any): N.Expression[]),
              true,
              "arrow function parameters",
+              node.extra?.trailingComma,
            );
            // Enter scope, as checkParams defines bindings
            this.scope.enter(functionFlags(false, false) | SCOPE_ARROW);
@@ -1820,6 +1821,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
            ((node.params: any): N.Expression[]),
            true,
            "arrow function parameters",
+            node.extra?.trailingComma,
          );
        }
        return [arrows, []];
@@ -1831,6 +1833,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
            ((node.params: any): N.Expression[]),
            true,
            "arrow function parameters",
+            node.extra?.trailingComma,
          );
          return true;
        } catch (err) {
@@ -2005,6 +2008,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
      exprList: N.Expression[],
      isBinding: ?boolean,
      contextDescription: string,
+      trailingCommaPos?: ?number,
    ): $ReadOnlyArray<N.Pattern> {
      for (let i = 0; i < exprList.length; i++) {
        const expr = exprList[i];
@@ -2012,7 +2016,12 @@ export default (superClass: Class<Parser>): Class<Parser> =>
          exprList[i] = this.typeCastToParameter(expr);
        }
      }
-      return super.toAssignableList(exprList, isBinding, contextDescription);
+      return super.toAssignableList(
+        exprList,
+        isBinding,
+        contextDescription,
+        trailingCommaPos,
+      );
    }

    // this is a list of nodes, from something like a call expression, we need to filter the
--- a/packages/babel-parser/src/plugins/typescript/index.js
+++ b/packages/babel-parser/src/plugins/typescript/index.js
@@ -363,21 +363,23 @@ export default (superClass: Class<Parser>): Class<Parser> =>
    tsParseBindingListForSignature(): $ReadOnlyArray<
      N.Identifier | N.RestElement | N.ObjectPattern | N.ArrayPattern,
    > {
-      return this.parseBindingList(tt.parenR).map(pattern => {
-        if (
-          pattern.type !== "Identifier" &&
-          pattern.type !== "RestElement" &&
-          pattern.type !== "ObjectPattern" &&
-          pattern.type !== "ArrayPattern"
-        ) {
-          throw this.unexpected(
-            pattern.start,
-            "Name in a signature must be an Identifier, ObjectPattern or ArrayPattern," +
-              `instead got ${pattern.type}`,
-          );
-        }
-        return pattern;
-      });
+      return this.parseBindingList(tt.parenR, charCodes.rightParenthesis).map(
+        pattern => {
+          if (
+            pattern.type !== "Identifier" &&
+            pattern.type !== "RestElement" &&
+            pattern.type !== "ObjectPattern" &&
+            pattern.type !== "ArrayPattern"
+          ) {
+            throw this.unexpected(
+              pattern.start,
+              "Name in a signature must be an Identifier, ObjectPattern or ArrayPattern," +
+                `instead got ${pattern.type}`,
+            );
+          }
+          return pattern;
+        },
+      );
    }

    tsParseTypeMemberSemicolon(): void {
@@ -586,7 +588,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
        const restNode: N.TsRestType = this.startNode();
        this.next(); // skips ellipsis
        restNode.typeAnnotation = this.tsParseType();
-        this.checkCommaAfterRest();
+        this.checkCommaAfterRest(charCodes.rightSquareBracket);
        return this.finishNode(restNode, "TSRestType");
      }

@@ -2363,11 +2365,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
      }
    }

-    toAssignableList(
-      exprList: N.Expression[],
-      isBinding: ?boolean,
-      contextDescription: string,
-    ): $ReadOnlyArray<N.Pattern> {
+    toAssignableList(exprList: N.Expression[]): $ReadOnlyArray<N.Pattern> {
      for (let i = 0; i < exprList.length; i++) {
        const expr = exprList[i];
        if (!expr) continue;
@@ -2384,7 +2382,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
            break;
        }
      }
-      return super.toAssignableList(exprList, isBinding, contextDescription);
+      return super.toAssignableList(...arguments);
    }

    typeCastToParameter(node: N.TsTypeCastExpression): N.Node {
--- a/packages/babel-parser/src/tokenizer/state.js
+++ b/packages/babel-parser/src/tokenizer/state.js
@@ -55,11 +55,6 @@ export default class State {
  //          ^
  noArrowParamsConversionAt: number[] = [];

-  // A comma after "...a" is only allowed in spread, but not in rest.
-  // Since we parse destructuring patterns as array/object literals
-  // and then convert them, we need to track it.
-  commaAfterSpreadAt: number = -1;
-
  // Flags to track
  inParameters: boolean = false;
  maybeInArrowParameters: boolean = false;