Left-value parsing cleanup (babel#17160)

* cleanup * refactor: simplify checkProto implementation * rename test * refactor: avoid second scan in type cast transform * refactor: simplify ts parseConditional The current logic is taken from the flow plugin * add benchmark cases
liuxingbaoyu · Mar 3, 2025 · fb1e134 · fb1e134
1 parent 61a6b5e
commit fb1e134
Show file tree

Hide file tree

Showing 11 changed files with 109 additions and 66 deletions.
diff --git a/benchmark/babel-parser/many-nested-conditional-expressions/bench.mjs b/benchmark/babel-parser/many-nested-conditional-expressions/bench.mjs
@@ -0,0 +1,20 @@
+import Benchmark from "benchmark";
+import { baselineParser, currentParser, report } from "../../util.mjs";
+
+const suite = new Benchmark.Suite();
+function createInput(length) {
+  return "(" + "x ?".repeat(length) + "0" + ": x".repeat(length) + ");";
+}
+function benchCases(name, implementation, options) {
+  for (const length of [256, 512, 1024, 2048]) {
+    const input = createInput(length);
+    suite.add(`${name} ${length} nested conditional expressions`, () => {
+      implementation.parse(input, options);
+    });
+  }
+}
+
+benchCases("baseline", baselineParser);
+benchCases("current", currentParser);
+
+suite.on("cycle", report).run();
diff --git a/benchmark/babel-parser/many-nested-conditional-expressions/with-typescript-plugin.mjs b/benchmark/babel-parser/many-nested-conditional-expressions/with-typescript-plugin.mjs
@@ -0,0 +1,23 @@
+import Benchmark from "benchmark";
+import { baselineParser, currentParser, report } from "../../util.mjs";
+
+const suite = new Benchmark.Suite();
+function createInput(length) {
+  return "(" + "x ?".repeat(length) + "0" + ": x".repeat(length) + ");";
+}
+function benchCases(name, implementation, options) {
+  for (const length of [256, 512, 1024, 2048]) {
+    const input = createInput(length);
+    suite.add(
+      `${name} ${length} nested conditional expressions with typescript plugin`,
+      () => {
+        implementation.parse(input, options);
+      }
+    );
+  }
+}
+
+benchCases("baseline", baselineParser, { plugins: ["typescript"] });
+benchCases("current", currentParser, { plugins: ["typescript"] });
+
+suite.on("cycle", report).run();
diff --git a/packages/babel-parser/src/parser/expression.ts b/packages/babel-parser/src/parser/expression.ts
@@ -55,7 +55,7 @@ import {
   newAsyncArrowScope,
   newExpressionScope,
 } from "../util/expression-scope.ts";
-import { Errors, type ParseError } from "../parse-error.ts";
+import { Errors } from "../parse-error.ts";
 import {
   UnparenthesizedPipeBodyDescriptions,
   type UnparenthesizedPipeBodyTypes,
@@ -108,18 +108,16 @@ export default abstract class ExpressionParser extends LValParser {
   checkProto(
     prop: N.ObjectMember | N.SpreadElement,
     isRecord: boolean | undefined | null,
-    protoRef: {
-      used: boolean;
-    },
+    sawProto: boolean,
     refExpressionErrors?: ExpressionErrors | null,
-  ): void {
+  ): boolean {
     if (
       prop.type === "SpreadElement" ||
       this.isObjectMethod(prop) ||
       prop.computed ||
       prop.shorthand
     ) {
-      return;
+      return sawProto;
     }
 
     const key = prop.key as
@@ -133,9 +131,9 @@ export default abstract class ExpressionParser extends LValParser {
     if (name === "__proto__") {
       if (isRecord) {
         this.raise(Errors.RecordNoProto, key);
-        return;
+        return true;
       }
-      if (protoRef.used) {
+      if (sawProto) {
         if (refExpressionErrors) {
           // Store the first redefinition's position, otherwise ignore because
           // we are parsing ambiguous pattern
@@ -147,8 +145,10 @@ export default abstract class ExpressionParser extends LValParser {
         }
       }
 
-      protoRef.used = true;
+      return true;
     }
+
+    return sawProto;
   }
 
   shouldExitDescending(
@@ -255,12 +255,8 @@ export default abstract class ExpressionParser extends LValParser {
 
   // This method is only used by
   // the typescript and flow plugins.
-  setOptionalParametersError(
-    refExpressionErrors: ExpressionErrors,
-    resultError?: ParseError<any>,
-  ) {
-    refExpressionErrors.optionalParametersLoc =
-      resultError?.loc ?? this.state.startLoc;
+  setOptionalParametersError(refExpressionErrors: ExpressionErrors) {
+    refExpressionErrors.optionalParametersLoc = this.state.startLoc;
   }
 
   // Parse an assignment expression. This includes applications of
@@ -2019,7 +2015,7 @@ export default abstract class ExpressionParser extends LValParser {
     }
     const oldInFSharpPipelineDirectBody = this.state.inFSharpPipelineDirectBody;
     this.state.inFSharpPipelineDirectBody = false;
-    const propHash: any = Object.create(null);
+    let sawProto = false;
     let first = true;
     const node = this.startNode<
       N.ObjectExpression | N.ObjectPattern | N.RecordExpression
@@ -2044,7 +2040,12 @@ export default abstract class ExpressionParser extends LValParser {
         prop = this.parseBindingProperty();
       } else {
         prop = this.parsePropertyDefinition(refExpressionErrors);
-        this.checkProto(prop, isRecord, propHash, refExpressionErrors);
+        sawProto = this.checkProto(
+          prop,
+          isRecord,
+          sawProto,
+          refExpressionErrors,
+        );
       }
 
       if (

diff --git a/packages/babel-parser/src/parser/lval.ts b/packages/babel-parser/src/parser/lval.ts
@@ -21,7 +21,7 @@ import type {
   AssignmentProperty,
   Assignable,
 } from "../types.ts";
-import type { Pos, Position } from "../util/location.ts";
+import type { Position } from "../util/location.ts";
 import {
   isStrictBindOnlyReservedWord,
   isStrictBindReservedWord,
@@ -50,13 +50,11 @@ export default abstract class LValParser extends NodeUtils {
   abstract parseMaybeAssign(
     refExpressionErrors?: ExpressionErrors | null,
     afterLeftParse?: Function,
-    refNeedsArrowPos?: Pos | null,
   ): Expression;
 
   abstract parseMaybeAssignAllowIn(
     refExpressionErrors?: ExpressionErrors | null,
     afterLeftParse?: Function,
-    refNeedsArrowPos?: Pos | null,
   ): Expression;
 
   abstract parseObjectLike<T extends ObjectPattern | ObjectExpression>(
@@ -232,7 +230,7 @@ export default abstract class LValParser extends NodeUtils {
   // Convert list of expression atoms to binding list.
 
   toAssignableList(
-    exprList: (Expression | SpreadElement | RestElement)[],
+    exprList: (Expression | SpreadElement | RestElement | null)[],
     trailingCommaLoc: Position | undefined | null,
     isLHS: boolean,
   ): void {
@@ -242,14 +240,7 @@ export default abstract class LValParser extends NodeUtils {
       const elt = exprList[i];
       if (!elt) continue;
 
-      if (elt.type === "SpreadElement") {
-        (elt as unknown as RestElement).type = "RestElement";
-        const arg = elt.argument;
-        this.checkToRestConversion(arg, /* allowPattern */ true);
-        this.toAssignable(arg, isLHS);
-      } else {
-        this.toAssignable(elt, isLHS);
-      }
+      this.toAssignableListItem(exprList, i, isLHS);
 
       if (elt.type === "RestElement") {
         if (i < end) {
@@ -261,6 +252,22 @@ export default abstract class LValParser extends NodeUtils {
     }
   }
 
+  toAssignableListItem(
+    exprList: (Expression | SpreadElement | RestElement)[],
+    index: number,
+    isLHS: boolean,
+  ): void {
+    const node = exprList[index];
+    if (node.type === "SpreadElement") {
+      (node as unknown as RestElement).type = "RestElement";
+      const arg = node.argument;
+      this.checkToRestConversion(arg, /* allowPattern */ true);
+      this.toAssignable(arg, isLHS);
+    } else {
+      this.toAssignable(node, isLHS);
+    }
+  }
+
   isAssignable(node: Node, isBinding?: boolean): boolean {
     switch (node.type) {
       case "Identifier":

diff --git a/packages/babel-parser/src/parser/util.ts b/packages/babel-parser/src/parser/util.ts
@@ -374,15 +374,15 @@ export default abstract class UtilParser extends Tokenizer {
 /**
  * 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
+ * we will throw on this position on invalid assign syntax, otherwise it will be reset to null
  *
  * Types of ExpressionErrors:
  *
  * - **shorthandAssignLoc**: track initializer `=` position
  * - **doubleProtoLoc**: track the duplicate `__proto__` key position
- * - **privateKey**: track private key `#p` position
+ * - **privateKeyLoc**: track private key `#p` position
  * - **optionalParametersLoc**: track the optional parameter (`?`).
- * It's only used by typescript and flow plugins
+ *   It's only used by typescript and flow plugins
  */
 export class ExpressionErrors {
   shorthandAssignLoc: Position | undefined | null = null;

diff --git a/packages/babel-parser/src/plugins/typescript/index.ts b/packages/babel-parser/src/plugins/typescript/index.ts
@@ -24,7 +24,6 @@ import { ParamKind } from "../../util/production-parameter.ts";
 import { Errors, ParseErrorEnum } from "../../parse-error.ts";
 import { cloneIdentifier, type Undone } from "../../parser/node.ts";
 import type { Pattern } from "../../types.ts";
-import type { Expression } from "../../types.ts";
 import type { ClassWithMixin, IJSXParserMixin } from "../jsx/index.ts";
 import { ParseBindingListFlags } from "../../parser/lval.ts";
 import { OptionFlags } from "../../options.ts";
@@ -3261,31 +3260,26 @@ export default (superClass: ClassWithMixin<typeof Parser, IJSXParserMixin>) =>
       startLoc: Position,
       refExpressionErrors?: ExpressionErrors | null,
     ): N.Expression {
-      // only do the expensive clone if there is a question mark
-      // and if we come from inside parens
-      if (!this.state.maybeInArrowParameters || !this.match(tt.question)) {
-        return super.parseConditional(
-          expr,
+      if (!this.match(tt.question)) return expr;
 
-          startLoc,
-          refExpressionErrors,
-        );
-      }
-
-      const result = this.tryParse(() =>
-        super.parseConditional(expr, startLoc),
-      );
-
-      if (!result.node) {
-        if (result.error) {
+      if (this.state.maybeInArrowParameters) {
+        const nextCh = this.lookaheadCharCode();
+        // These tokens cannot start an expression, so if one of them follows
+        // ? then we are probably in an arrow function parameters list and we
+        // don't parse the conditional expression.
+        if (
+          nextCh === charCodes.comma || // (a?, b) => c
+          nextCh === charCodes.equalsTo || // (a? = b) => c
+          nextCh === charCodes.colon || // (a?: b) => c
+          nextCh === charCodes.rightParenthesis // (a?) => c
+        ) {
           /*:: invariant(refExpressionErrors != null) */
-          super.setOptionalParametersError(refExpressionErrors, result.error);
+          this.setOptionalParametersError(refExpressionErrors);
+          return expr;
         }
-
-        return expr;
       }
-      if (result.error) this.state = result.failState;
-      return result.node;
+
+      return super.parseConditional(expr, startLoc, refExpressionErrors);
     }
 
     // Note: These "type casts" are *not* valid TS expressions.
@@ -4032,18 +4026,16 @@ export default (superClass: ClassWithMixin<typeof Parser, IJSXParserMixin>) =>
       return type;
     }
 
-    toAssignableList(
-      exprList: Expression[],
-      trailingCommaLoc: Position | undefined | null,
+    toAssignableListItem(
+      exprList: (N.Expression | N.SpreadElement | N.RestElement)[],
+      index: number,
       isLHS: boolean,
     ): void {
-      for (let i = 0; i < exprList.length; i++) {
-        const expr = exprList[i];
-        if (expr?.type === "TSTypeCastExpression") {
-          exprList[i] = this.typeCastToParameter(expr);
-        }
+      const node = exprList[index];
+      if (node.type === "TSTypeCastExpression") {
+        exprList[index] = this.typeCastToParameter(node);
       }
-      super.toAssignableList(exprList, trailingCommaLoc, isLHS);
+      super.toAssignableListItem(exprList, index, isLHS);
     }
 
     typeCastToParameter(node: N.TsTypeCastExpression): N.Expression {

diff --git a/packages/babel-parser/src/util/expression-scope.ts b/packages/babel-parser/src/util/expression-scope.ts
@@ -45,7 +45,7 @@ There are four different expression scope
   recorded in this scope will be thrown immediately. No errors will be recorded in
   this scope.
 
-// @see {@link https://docs.google.com/document/d/1FAvEp9EUK-G8kHfDIEo_385Hs2SUBCYbJ5H-NnLvq8M|V8 Expression Scope design docs}
+// @see {@link https://docs.google.com/document/d/1FAvEp9EUK-G8kHfDIEo_385Hs2SUBCYbJ5H-NnLvq8M | V8 Expression Scope design docs}
  */
 
 const enum ExpressionScopeType {

diff --git a/...renthesizedExpressions-true copy/input.ts → ...ateParenthesizedExpressions-true/input.ts b/...renthesizedExpressions-true copy/input.ts → ...ateParenthesizedExpressions-true/input.ts
diff --git a/...hesizedExpressions-true copy/options.json → ...arenthesizedExpressions-true/options.json b/...hesizedExpressions-true copy/options.json → ...arenthesizedExpressions-true/options.json
diff --git a/...thesizedExpressions-true copy/output.json → ...ParenthesizedExpressions-true/output.json b/...thesizedExpressions-true copy/output.json → ...ParenthesizedExpressions-true/output.json
diff --git a/...ges/babel-parser/test/fixtures/typescript/async-call/with-optional-parameter/options.json b/...ges/babel-parser/test/fixtures/typescript/async-call/with-optional-parameter/options.json
@@ -1,3 +1,3 @@
 {
-  "throws": "Unexpected token (1:8)"
-}
+  "throws": "Unexpected token (1:7)"
+}