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>

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,14 +222,9 @@ export default class ExpressionParser extends LValParser {
   parseMaybeAssignDisallowIn(
     refExpressionErrors?: ?ExpressionErrors,
     afterLeftParse?: Function,
-    refNeedsArrowPos?: ?Pos,
   ) {
     return this.disallowInAnd(() =>
-      this.parseMaybeAssign(
+      this.parseMaybeAssign(refExpressionErrors, afterLeftParse),
-        refExpressionErrors,
-        afterLeftParse,
-        refNeedsArrowPos,
-      ),
     );
   }
 
@@ -237,25 +232,28 @@ export default class ExpressionParser extends LValParser {
   parseMaybeAssignAllowIn(
     refExpressionErrors?: ?ExpressionErrors,
     afterLeftParse?: Function,
-    refNeedsArrowPos?: ?Pos,
   ) {
     return this.allowInAnd(() =>
-      this.parseMaybeAssign(
+      this.parseMaybeAssign(refExpressionErrors, afterLeftParse),
-        refExpressionErrors,
-        afterLeftParse,
-        refNeedsArrowPos,
-      ),
     );
   }
 
+  // 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(
+    let left = this.parseMaybeConditional(refExpressionErrors);
-      refExpressionErrors,
-      refNeedsArrowPos,
-    );
     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(
+  parseMaybeConditional(refExpressionErrors: ExpressionErrors): N.Expression {
-    refExpressionErrors: ExpressionErrors,
-    refNeedsArrowPos?: ?Pos,
-  ): 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(
+        this.parseExprListItem(false, refExpressionErrors, allowPlaceholder),
-          false,
-          refExpressionErrors,
-          { start: 0 },
-          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;

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;
+    const { shorthandAssign, doubleProto, optionalParameters } =
-    if (!andThrow) return shorthandAssign >= 0 || doubleProto >= 0;
+      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
+ * The ExpressionErrors is a context struct used to track ambiguous patterns
- * - **shorthandAssign**: track initializer `=` position when parsing ambiguous
+ * When we are sure the parsed pattern is a RHS, which means it is not a pattern,
- *   patterns. When we are sure the parsed pattern is a RHS, which means it is
+ * we will throw on this position on invalid assign syntax, otherwise it will be reset to -1
- *   not a pattern, we will throw on this position on invalid assign syntax,
+ *
- *   otherwise it will be reset to -1
+ * Types of ExpressionErrors:
- * - **doubleProto**: track the duplicate `__proto__` key position when parsing
+ *
- *   ambiguous object patterns. When we are sure the parsed pattern is a RHS,
+ * - **shorthandAssign**: track initializer `=` position
- *   which means it is an object literal, we will throw on this position for
+ * - **doubleProto**: track the duplicate `__proto__` key position
- *   __proto__ redefinition, otherwise it will be reset to -1
+ * - **optionalParameters**: track the optional paramter (`?`).
+ * It's only used by typescript and flow plugins
  */
 export class ExpressionErrors {
   shorthandAssign = -1;
   doubleProto = -1;
+  optionalParameters = -1;
 }

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
+          if (result.error) {
-          refNeedsArrowPos.start = result.error.pos || this.state.start;
+            /*:: 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),
-            super.parseMaybeAssign(
-              refExpressionErrors,
-              afterLeftParse,
-              refNeedsArrowPos,
-            ),
           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(
+      return super.parseMaybeAssign(refExpressionErrors, afterLeftParse);
-        refExpressionErrors,
-        afterLeftParse,
-        refNeedsArrowPos,
-      );
     }
 
     // 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(

packages/babel-parser/src/plugins/typescript/index.js

@@ -10,7 +10,7 @@ import type State from "../../tokenizer/state";
 import { types as tt } from "../../tokenizer/types";
 import { types as ct } from "../../tokenizer/context";
 import * as N from "../../types";
-import type { Pos, Position } from "../../util/location";
+import type { Position } from "../../util/location";
 import type Parser from "../../parser";
 import {
   type BindingTypes,
@@ -2467,16 +2467,16 @@ export default (superClass: Class<Parser>): Class<Parser> =>
       expr: N.Expression,
       startPos: number,
       startLoc: Position,
-      refNeedsArrowPos?: ?Pos,
+      refExpressionErrors?: ?ExpressionErrors,
     ): N.Expression {
       // only do the expensive clone if there is a question mark
       // and if we come from inside parens
-      if (!refNeedsArrowPos || !this.match(tt.question)) {
+      if (!this.state.maybeInArrowParameters || !this.match(tt.question)) {
         return super.parseConditional(
           expr,
           startPos,
           startLoc,
-          refNeedsArrowPos,
+          refExpressionErrors,
         );
       }
 
@@ -2485,8 +2485,11 @@ export default (superClass: Class<Parser>): Class<Parser> =>
       );
 
       if (!result.node) {
-        // $FlowIgnore
+        if (result.error) {
-        refNeedsArrowPos.start = result.error.pos || this.state.start;
+          /*:: invariant(refExpressionErrors != null) */
+          super.setOptionalParametersError(refExpressionErrors, result.error);
+        }
+
         return expr;
       }
       if (result.error) this.state = result.failState;
@@ -2734,8 +2737,9 @@ export default (superClass: Class<Parser>): Class<Parser> =>
         state = this.state.clone();
 
         jsx = this.tryParse(() => super.parseMaybeAssign(...args), 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
@@ -2778,6 +2782,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
         return expr;
       }, state);
 
+      /*:: invariant(arrow.node != null) */
       if (!arrow.error && !arrow.aborted) return arrow.node;
 
       if (!jsx) {
@@ -2790,6 +2795,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
         // But don't directly call `this.tsParseTypeAssertion` because we want to handle any binary after it.
         typeCast = this.tryParse(() => super.parseMaybeAssign(...args), state);
         /*:: invariant(!typeCast.aborted) */
+        /*:: invariant(typeCast.node != null) */
         if (!typeCast.error) return typeCast.node;
       }