refactor(parser): remove refNeedsArrowPos (#13419)

* fix(parser:expression): remove refNeedsArrowPos in parseExprListItem * feat: use refExpressionErrors to catch optional parameters * feat: delete useless refNeedArrowPos * fix: forgotten deletion * fix: lint and factorize * fix: delete useless comment * fix: review corrections * fix: review corrections pt2 * fix: review correction pt2 * fix: nit correction * fix: update types * Update packages/babel-parser/src/parser/util.js * fix: add invariant comment in flow and ts file Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
babel · Jun 19, 2021 · 101249f · 101249f
1 parent eb22659
commit 101249f
Show file tree

Hide file tree

Showing 4 changed files with 78 additions and 79 deletions.
diff --git a/packages/babel-parser/src/parser/expression.js b/packages/babel-parser/src/parser/expression.js
@@ -30,7 +30,6 @@ import {
   isIdentifierStart,
   canBeReservedWord,
 } from "../util/identifier";
-import type { Pos } from "../util/location";
 import { Position } from "../util/location";
 import * as charCodes from "charcodes";
 import {
@@ -57,6 +56,7 @@ import {
   newExpressionScope,
 } from "../util/expression-scope";
 import { Errors, SourceTypeModuleErrors } from "./error";
+import type { ParsingError } from "./error";
 
 /*::
 import type { SourceType } from "../options";
@@ -222,40 +222,38 @@ export default class ExpressionParser extends LValParser {
   parseMaybeAssignDisallowIn(
     refExpressionErrors?: ?ExpressionErrors,
     afterLeftParse?: Function,
-    refNeedsArrowPos?: ?Pos,
   ) {
     return this.disallowInAnd(() =>
-      this.parseMaybeAssign(
-        refExpressionErrors,
-        afterLeftParse,
-        refNeedsArrowPos,
-      ),
+      this.parseMaybeAssign(refExpressionErrors, afterLeftParse),
     );
   }
 
   // Set [+In] parameter for assignment expression
   parseMaybeAssignAllowIn(
     refExpressionErrors?: ?ExpressionErrors,
     afterLeftParse?: Function,
-    refNeedsArrowPos?: ?Pos,
   ) {
     return this.allowInAnd(() =>
-      this.parseMaybeAssign(
-        refExpressionErrors,
-        afterLeftParse,
-        refNeedsArrowPos,
-      ),
+      this.parseMaybeAssign(refExpressionErrors, afterLeftParse),
     );
   }
 
+  // This method is only used by
+  // the typescript and flow plugins.
+  setOptionalParametersError(
+    refExpressionErrors: ExpressionErrors,
+    resultError?: ParsingError,
+  ) {
+    refExpressionErrors.optionalParameters =
+      resultError?.pos ?? this.state.start;
+  }
+
   // Parse an assignment expression. This includes applications of
   // operators like `+=`.
-
   // https://tc39.es/ecma262/#prod-AssignmentExpression
   parseMaybeAssign(
     refExpressionErrors?: ?ExpressionErrors,
     afterLeftParse?: Function,
-    refNeedsArrowPos?: ?Pos,
   ): N.Expression {
     const startPos = this.state.start;
     const startLoc = this.state.startLoc;
@@ -281,10 +279,7 @@ export default class ExpressionParser extends LValParser {
       this.state.potentialArrowAt = this.state.start;
     }
 
-    let left = this.parseMaybeConditional(
-      refExpressionErrors,
-      refNeedsArrowPos,
-    );
+    let left = this.parseMaybeConditional(refExpressionErrors);
     if (afterLeftParse) {
       left = afterLeftParse.call(this, left, startPos, startLoc);
     }
@@ -319,10 +314,7 @@ export default class ExpressionParser extends LValParser {
   // Parse a ternary conditional (`?:`) operator.
   // https://tc39.es/ecma262/#prod-ConditionalExpression
 
-  parseMaybeConditional(
-    refExpressionErrors: ExpressionErrors,
-    refNeedsArrowPos?: ?Pos,
-  ): N.Expression {
+  parseMaybeConditional(refExpressionErrors: ExpressionErrors): N.Expression {
     const startPos = this.state.start;
     const startLoc = this.state.startLoc;
     const potentialArrowAt = this.state.potentialArrowAt;
@@ -332,16 +324,15 @@ export default class ExpressionParser extends LValParser {
       return expr;
     }
 
-    return this.parseConditional(expr, startPos, startLoc, refNeedsArrowPos);
+    return this.parseConditional(expr, startPos, startLoc, refExpressionErrors);
   }
 
   parseConditional(
     expr: N.Expression,
     startPos: number,
     startLoc: Position,
-    // FIXME: Disabling this for now since can't seem to get it to play nicely
     // eslint-disable-next-line no-unused-vars
-    refNeedsArrowPos?: ?Pos,
+    refExpressionErrors?: ?ExpressionErrors,
   ): N.Expression {
     if (this.eat(tt.question)) {
       const node = this.startNodeAt(startPos, startLoc);
@@ -931,12 +922,7 @@ export default class ExpressionParser extends LValParser {
       }
 
       elts.push(
-        this.parseExprListItem(
-          false,
-          refExpressionErrors,
-          { start: 0 },
-          allowPlaceholder,
-        ),
+        this.parseExprListItem(false, refExpressionErrors, allowPlaceholder),
       );
     }
 
@@ -1449,7 +1435,6 @@ export default class ExpressionParser extends LValParser {
     const innerStartLoc = this.state.startLoc;
     const exprList = [];
     const refExpressionErrors = new ExpressionErrors();
-    const refNeedsArrowPos = { start: 0 };
     let first = true;
     let spreadStart;
     let optionalCommaStart;
@@ -1458,7 +1443,12 @@ export default class ExpressionParser extends LValParser {
       if (first) {
         first = false;
       } else {
-        this.expect(tt.comma, refNeedsArrowPos.start || null);
+        this.expect(
+          tt.comma,
+          refExpressionErrors.optionalParameters === -1
+            ? null
+            : refExpressionErrors.optionalParameters,
+        );
         if (this.match(tt.parenR)) {
           optionalCommaStart = this.state.start;
           break;
@@ -1485,7 +1475,6 @@ export default class ExpressionParser extends LValParser {
           this.parseMaybeAssignAllowIn(
             refExpressionErrors,
             this.parseParenItem,
-            refNeedsArrowPos,
           ),
         );
       }
@@ -1517,7 +1506,6 @@ export default class ExpressionParser extends LValParser {
     if (optionalCommaStart) this.unexpected(optionalCommaStart);
     if (spreadStart) this.unexpected(spreadStart);
     this.checkExpressionErrors(refExpressionErrors, true);
-    if (refNeedsArrowPos.start) this.unexpected(refNeedsArrowPos.start);
 
     this.toReferencedListDeep(exprList, /* isParenthesizedExpr */ true);
     if (exprList.length > 1) {
@@ -2274,7 +2262,6 @@ export default class ExpressionParser extends LValParser {
   parseExprListItem(
     allowEmpty: ?boolean,
     refExpressionErrors?: ?ExpressionErrors,
-    refNeedsArrowPos: ?Pos,
     allowPlaceholder: ?boolean,
   ): ?N.Expression {
     let elt;
@@ -2286,8 +2273,9 @@ export default class ExpressionParser extends LValParser {
     } else if (this.match(tt.ellipsis)) {
       const spreadNodeStartPos = this.state.start;
       const spreadNodeStartLoc = this.state.startLoc;
+
       elt = this.parseParenItem(
-        this.parseSpread(refExpressionErrors, refNeedsArrowPos),
+        this.parseSpread(refExpressionErrors),
         spreadNodeStartPos,
         spreadNodeStartLoc,
       );
@@ -2303,7 +2291,6 @@ export default class ExpressionParser extends LValParser {
       elt = this.parseMaybeAssignAllowIn(
         refExpressionErrors,
         this.parseParenItem,
-        refNeedsArrowPos,
       );
     }
     return elt;

diff --git a/packages/babel-parser/src/parser/util.js b/packages/babel-parser/src/parser/util.js
@@ -14,6 +14,7 @@ import ProductionParameterHandler, {
   PARAM,
 } from "../util/production-parameter";
 import { Errors, type ErrorTemplate, ErrorCodes } from "./error";
+import type { ParsingError } from "./error";
 /*::
 import type ScopeHandler from "../util/scope";
 */
@@ -175,6 +176,7 @@ export default class UtilParser extends Tokenizer {
         template: `Unexpected token, expected "${messageOrType.label}"`,
       };
     }
+
     /* eslint-disable @babel/development-internal/dry-error-messages */
     throw this.raise(pos != null ? pos : this.state.start, messageOrType);
     /* eslint-enable @babel/development-internal/dry-error-messages */
@@ -211,7 +213,7 @@ export default class UtilParser extends Tokenizer {
     oldState: State = this.state.clone(),
   ):
     | TryParse<T, null, false, false, null>
-    | TryParse<T | null, SyntaxError, boolean, false, State>
+    | TryParse<T | null, ParsingError, boolean, false, State>
     | TryParse<T | null, null, false, true, State> {
     const abortSignal: { node: T | null } = { node: null };
     try {
@@ -228,7 +230,7 @@ export default class UtilParser extends Tokenizer {
         this.state.tokensLength = failState.tokensLength;
         return {
           node,
-          error: (failState.errors[oldState.errors.length]: SyntaxError),
+          error: (failState.errors[oldState.errors.length]: ParsingError),
           thrown: false,
           aborted: false,
           failState,
@@ -267,14 +269,22 @@ export default class UtilParser extends Tokenizer {
     andThrow: boolean,
   ) {
     if (!refExpressionErrors) return false;
-    const { shorthandAssign, doubleProto } = refExpressionErrors;
-    if (!andThrow) return shorthandAssign >= 0 || doubleProto >= 0;
+    const { shorthandAssign, doubleProto, optionalParameters } =
+      refExpressionErrors;
+    if (!andThrow) {
+      return (
+        shorthandAssign >= 0 || doubleProto >= 0 || optionalParameters >= 0
+      );
+    }
     if (shorthandAssign >= 0) {
       this.unexpected(shorthandAssign);
     }
     if (doubleProto >= 0) {
       this.raise(doubleProto, Errors.DuplicateProto);
     }
+    if (optionalParameters >= 0) {
+      this.unexpected(optionalParameters);
+    }
   }
 
   /**
@@ -394,17 +404,19 @@ export default class UtilParser extends Tokenizer {
 }
 
 /**
- * The ExpressionErrors is a context struct used to track
- * - **shorthandAssign**: track initializer `=` position when parsing ambiguous
- *   patterns. When we are sure the parsed pattern is a RHS, which means it is
- *   not a pattern, we will throw on this position on invalid assign syntax,
- *   otherwise it will be reset to -1
- * - **doubleProto**: track the duplicate `__proto__` key position when parsing
- *   ambiguous object patterns. When we are sure the parsed pattern is a RHS,
- *   which means it is an object literal, we will throw on this position for
- *   __proto__ redefinition, otherwise it will be reset to -1
+ * The ExpressionErrors is a context struct used to track ambiguous patterns
+ * When we are sure the parsed pattern is a RHS, which means it is not a pattern,
+ * we will throw on this position on invalid assign syntax, otherwise it will be reset to -1
+ *
+ * Types of ExpressionErrors:
+ *
+ * - **shorthandAssign**: track initializer `=` position
+ * - **doubleProto**: track the duplicate `__proto__` key position
+ * - **optionalParameters**: track the optional paramter (`?`).
+ * It's only used by typescript and flow plugins
  */
 export class ExpressionErrors {
   shorthandAssign = -1;
   doubleProto = -1;
+  optionalParameters = -1;
 }
diff --git a/packages/babel-parser/src/plugins/flow/index.js b/packages/babel-parser/src/plugins/flow/index.js
@@ -8,7 +8,7 @@
 import type Parser from "../../parser";
 import { types as tt, type TokenType } from "../../tokenizer/types";
 import * as N from "../../types";
-import type { Pos, Position } from "../../util/location";
+import type { Position } from "../../util/location";
 import { types as tc } from "../../tokenizer/context";
 import * as charCodes from "charcodes";
 import { isIteratorStart, isKeyword } from "../../util/identifier";
@@ -1906,20 +1906,23 @@ export default (superClass: Class<Parser>): Class<Parser> =>
       expr: N.Expression,
       startPos: number,
       startLoc: Position,
-      refNeedsArrowPos?: ?Pos,
+      refExpressionErrors?: ?ExpressionErrors,
     ): N.Expression {
       if (!this.match(tt.question)) return expr;
 
       // only use the expensive "tryParse" method if there is a question mark
       // and if we come from inside parens
-      if (refNeedsArrowPos) {
+      if (this.state.maybeInArrowParameters) {
         const result = this.tryParse(() =>
           super.parseConditional(expr, startPos, startLoc),
         );
 
         if (!result.node) {
-          // $FlowIgnore
-          refNeedsArrowPos.start = result.error.pos || this.state.start;
+          if (result.error) {
+            /*:: invariant(refExpressionErrors != null) */
+            super.setOptionalParametersError(refExpressionErrors, result.error);
+          }
+
           return expr;
         }
 
@@ -1973,7 +1976,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
       node.test = expr;
       node.consequent = consequent;
       node.alternate = this.forwardNoArrowParamsConversionAt(node, () =>
-        this.parseMaybeAssign(undefined, undefined, undefined),
+        this.parseMaybeAssign(undefined, undefined),
       );
 
       return this.finishNode(node, "ConditionalExpression");
@@ -2820,7 +2823,6 @@ export default (superClass: Class<Parser>): Class<Parser> =>
     parseMaybeAssign(
       refExpressionErrors?: ?ExpressionErrors,
       afterLeftParse?: Function,
-      refNeedsArrowPos?: ?Pos,
     ): N.Expression {
       let state = null;
 
@@ -2833,16 +2835,12 @@ export default (superClass: Class<Parser>): Class<Parser> =>
         state = this.state.clone();
 
         jsx = this.tryParse(
-          () =>
-            super.parseMaybeAssign(
-              refExpressionErrors,
-              afterLeftParse,
-              refNeedsArrowPos,
-            ),
+          () => super.parseMaybeAssign(refExpressionErrors, afterLeftParse),
           state,
         );
-        /*:: invariant(!jsx.aborted) */
 
+        /*:: invariant(!jsx.aborted) */
+        /*:: invariant(jsx.node != null) */
         if (!jsx.error) return jsx.node;
 
         // Remove `tc.j_expr` and `tc.j_oTag` from context added
@@ -2870,7 +2868,6 @@ export default (superClass: Class<Parser>): Class<Parser> =>
               const result = super.parseMaybeAssign(
                 refExpressionErrors,
                 afterLeftParse,
-                refNeedsArrowPos,
               );
 
               this.resetStartLocationFromNode(result, typeParameters);
@@ -2950,11 +2947,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
         );
       }
 
-      return super.parseMaybeAssign(
-        refExpressionErrors,
-        afterLeftParse,
-        refNeedsArrowPos,
-      );
+      return super.parseMaybeAssign(refExpressionErrors, afterLeftParse);
     }
 
     // handle return types for arrow functions
@@ -3068,6 +3061,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
           state,
         );
 
+        /*:: invariant(arrow.node != null) */
         if (!arrow.error && !arrow.aborted) return arrow.node;
 
         const result = this.tryParse(