Make sure babel parser throws exactly same recoverable errors when estree plugin is enabled (#12375)

* refactor: introduce isPrivateName and getPrivateNameSV

* feat: check recoverable errors on estree-throw

* fix: pass through all params of parseBlockBody

* fix: set bigInt to null when invalid bigInt value is parsed

e.g. 0.1n

* fix: use string literal value in error message

When estree plugin is enabled, stringLiteral#extra.raw is not accessible. Use StringLiteral#value instead.

* refactor: introduce hasPropertyAsPrivateName

* fix: adapt to ChainExpression

* fix: port checkLVal early return for method in object pattern

* fix: throw new a?.() on estree

* fix: early return for __proto__ in accessors

* fix: test record element via isObjectProperty

* fix: pass through isLHS in toAssignable

* refactor: introduce isObjectMethod methods

packages/babel-parser/src/parser/error-message.js

@@ -51,7 +51,7 @@ export const ErrorMessages = Object.freeze({
   ElementAfterRest: "Rest element must be last element",
   EscapedCharNotAnIdentifier: "Invalid Unicode escape",
   ExportBindingIsString:
-    "A string literal cannot be used as an exported binding without `from`.\n- Did you mean `export { %0 as '%1' } from 'some-module'`?",
+    "A string literal cannot be used as an exported binding without `from`.\n- Did you mean `export { '%0' as '%1' } from 'some-module'`?",
   ExportDefaultFromAsIdentifier:
     "'from' is not allowed as an identifier after 'export default'",
   ForInOfLoopInitializer:

packages/babel-parser/src/parser/expression.js

@@ -94,8 +94,9 @@ export default class ExpressionParser extends LValParser {
   ): void {
     if (
       prop.type === "SpreadElement" ||
-      prop.type === "ObjectMethod" ||
+      this.isObjectMethod(prop) ||
       prop.computed ||
+      // $FlowIgnore
       prop.shorthand
     ) {
       return;
@@ -515,11 +516,7 @@ export default class ExpressionParser extends LValParser {
 
         if (arg.type === "Identifier") {
           this.raise(node.start, Errors.StrictDelete);
-        } else if (
+        } else if (this.hasPropertyAsPrivateName(arg)) {
-          (arg.type === "MemberExpression" ||
-            arg.type === "OptionalMemberExpression") &&
-          arg.property.type === "PrivateName"
-        ) {
           this.raise(node.start, Errors.DeletePrivateField);
         }
       }
@@ -618,12 +615,12 @@ export default class ExpressionParser extends LValParser {
 
     let optional = false;
     if (this.match(tt.questionDot)) {
-      state.optionalChainMember = optional = true;
       if (noCalls && this.lookaheadCharCode() === charCodes.leftParenthesis) {
         // stop at `?.` when parsing `new a?.()`
         state.stop = true;
         return base;
       }
+      state.optionalChainMember = optional = true;
       this.next();
     }
 
@@ -662,11 +659,14 @@ export default class ExpressionParser extends LValParser {
       ? this.parseExpression()
       : this.parseMaybePrivateName(true);
 
-    if (property.type === "PrivateName") {
+    if (this.isPrivateName(property)) {
       if (node.object.type === "Super") {
         this.raise(startPos, Errors.SuperPrivateField);
       }
-      this.classScope.usePrivateName(property.id.name, property.start);
+      this.classScope.usePrivateName(
+        this.getPrivateNameSV(property),
+        property.start,
+      );
     }
     node.property = property;
 
@@ -1496,13 +1496,9 @@ export default class ExpressionParser extends LValParser {
   // https://tc39.es/ecma262/#prod-NewExpression
   parseNew(node: N.Expression): N.NewExpression {
     node.callee = this.parseNoCallExpr();
-
     if (node.callee.type === "Import") {
       this.raise(node.callee.start, Errors.ImportCallNotNewExpression);
-    } else if (
+    } else if (this.isOptionalChain(node.callee)) {
-      node.callee.type === "OptionalMemberExpression" ||
-      node.callee.type === "OptionalCallExpression"
-    ) {
       this.raise(this.state.lastTokEnd, Errors.OptionalChainingNoNew);
     } else if (this.eat(tt.questionDot)) {
       this.raise(this.state.start, Errors.OptionalChainingNoNew);
@@ -1604,7 +1600,7 @@ export default class ExpressionParser extends LValParser {
 
       if (
         isRecord &&
-        prop.type !== "ObjectProperty" &&
+        !this.isObjectProperty(prop) &&
         prop.type !== "SpreadElement"
       ) {
         this.raise(prop.start, Errors.InvalidRecordProperty);
@@ -1923,7 +1919,7 @@ export default class ExpressionParser extends LValParser {
           ? this.parseExprAtom()
           : this.parseMaybePrivateName(isPrivateNameAllowed);
 
-      if (prop.key.type !== "PrivateName") {
+      if (!this.isPrivateName(prop.key)) {
         // ClassPrivateProperty is never computed, so we don't assign in that case.
         prop.computed = false;
       }

packages/babel-parser/src/parser/lval.js

@@ -445,11 +445,11 @@ export default class LValParser extends NodeUtils {
 
       case "ObjectPattern":
         for (let prop of expr.properties) {
-          if (prop.type === "ObjectProperty") prop = prop.value;
+          if (this.isObjectProperty(prop)) prop = prop.value;
           // If we find here an ObjectMethod, it's because this was originally
           // an ObjectExpression which has then been converted.
           // toAssignable already reported this error with a nicer message.
-          else if (prop.type === "ObjectMethod") continue;
+          else if (this.isObjectMethod(prop)) continue;
 
           this.checkLVal(
             prop,

packages/babel-parser/src/parser/statement.js

@@ -1346,7 +1346,7 @@ export default class StatementParser extends ExpressionParser {
       method.kind = "method";
       this.parseClassElementName(method);
 
-      if (method.key.type === "PrivateName") {
+      if (this.isPrivateName(method.key)) {
         // Private generator method
         this.pushClassPrivateMethod(classBody, privateMethod, true, false);
         return;
@@ -1370,7 +1370,7 @@ export default class StatementParser extends ExpressionParser {
 
     const containsEsc = this.state.containsEsc;
     const key = this.parseClassElementName(member);
-    const isPrivate = key.type === "PrivateName";
+    const isPrivate = this.isPrivateName(key);
     // Check the key is not a computed expression or string literal.
     const isSimple = key.type === "Identifier";
     const maybeQuestionTokenStart = this.state.start;
@@ -1431,7 +1431,7 @@ export default class StatementParser extends ExpressionParser {
       this.parseClassElementName(method);
       this.parsePostMemberNameModifiers(publicMember);
 
-      if (method.key.type === "PrivateName") {
+      if (this.isPrivateName(method.key)) {
         // private async method
         this.pushClassPrivateMethod(
           classBody,
@@ -1465,7 +1465,7 @@ export default class StatementParser extends ExpressionParser {
       // The so-called parsed name would have been "get/set": get the real name.
       this.parseClassElementName(publicMethod);
 
-      if (method.key.type === "PrivateName") {
+      if (this.isPrivateName(method.key)) {
         // private getter/setter
         this.pushClassPrivateMethod(classBody, privateMethod, false, false);
       } else {
@@ -1508,7 +1508,10 @@ export default class StatementParser extends ExpressionParser {
       this.raise(key.start, Errors.StaticPrototype);
     }
 
-    if (key.type === "PrivateName" && key.id.name === "constructor") {
+    if (
+      this.isPrivateName(key) &&
+      this.getPrivateNameSV(key) === "constructor"
+    ) {
       this.raise(key.start, Errors.ConstructorClassPrivateField);
     }
 
@@ -1571,7 +1574,7 @@ export default class StatementParser extends ExpressionParser {
     classBody.body.push(node);
 
     this.classScope.declarePrivateName(
-      node.key.id.name,
+      this.getPrivateNameSV(node.key),
       CLASS_ELEMENT_OTHER,
       node.key.start,
     );
@@ -1627,7 +1630,11 @@ export default class StatementParser extends ExpressionParser {
           ? CLASS_ELEMENT_STATIC_SETTER
           : CLASS_ELEMENT_INSTANCE_SETTER
         : CLASS_ELEMENT_OTHER;
-    this.classScope.declarePrivateName(node.key.id.name, kind, node.key.start);
+    this.classScope.declarePrivateName(
+      this.getPrivateNameSV(node.key),
+      kind,
+      node.key.start,
+    );
   }
 
   // Overridden in typescript.js
@@ -1980,7 +1987,7 @@ export default class StatementParser extends ExpressionParser {
               this.raise(
                 specifier.start,
                 Errors.ExportBindingIsString,
-                local.extra.raw,
+                local.value,
                 exportedName,
               );
             } else {

packages/babel-parser/src/parser/util.js

@@ -252,6 +252,51 @@ export default class UtilParser extends Tokenizer {
       this.match(tt.decimal)
     );
   }
+
+  /*
+   * Test if given node is a PrivateName
+   * will be overridden in ESTree plugin
+   */
+  isPrivateName(node: Node): boolean {
+    return node.type === "PrivateName";
+  }
+
+  /*
+   * Return the string value of a given private name
+   * WITHOUT `#`
+   * @see {@link https://tc39.es/proposal-class-fields/#sec-private-names-static-semantics-stringvalue}
+   */
+  getPrivateNameSV(node: Node): string {
+    return node.id.name;
+  }
+
+  /*
+   * Return whether the given node is a member/optional chain that
+   * contains a private name as its property
+   * It is overridden in ESTree plugin
+   */
+  hasPropertyAsPrivateName(node: Node): boolean {
+    return (
+      (node.type === "MemberExpression" ||
+        node.type === "OptionalMemberExpression") &&
+      this.isPrivateName(node.property)
+    );
+  }
+
+  isOptionalChain(node: Node): boolean {
+    return (
+      node.type === "OptionalMemberExpression" ||
+      node.type === "OptionalCallExpression"
+    );
+  }
+
+  isObjectProperty(node: Node): boolean {
+    return node.type === "ObjectProperty";
+  }
+
+  isObjectMethod(node: Node): boolean {
+    return node.type === "ObjectMethod";
+  }
 }
 
 /**

packages/babel-parser/src/plugins/estree.js

@@ -5,18 +5,8 @@ import type Parser from "../parser";
 import type { ExpressionErrors } from "../parser/util";
 import * as N from "../types";
 import type { Position } from "../util/location";
-import { type BindingTypes } from "../util/scopeflags";
 import { Errors } from "../parser/error";
 
-function isSimpleProperty(node: N.Node): boolean {
-  return (
-    node != null &&
-    node.type === "Property" &&
-    node.kind === "init" &&
-    node.method === false
-  );
-}
-
 export default (superClass: Class<Parser>): Class<Parser> =>
   class extends superClass {
     estreeParseRegExpLiteral({ pattern, flags }: N.RegExpLiteral): N.Node {
@@ -35,8 +25,13 @@ export default (superClass: Class<Parser>): Class<Parser> =>
 
     estreeParseBigIntLiteral(value: any): N.Node {
       // https://github.com/estree/estree/blob/master/es2020.md#bigintliteral
-      // $FlowIgnore
+      let bigInt;
-      const bigInt = typeof BigInt !== "undefined" ? BigInt(value) : null;
+      try {
+        // $FlowIgnore
+        bigInt = BigInt(value);
+      } catch {
+        bigInt = null;
+      }
       const node = this.estreeParseLiteral(bigInt);
       node.bigint = String(node.value || value);
 
@@ -98,7 +93,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
     }
 
     checkDeclaration(node: N.Pattern | N.ObjectProperty): void {
-      if (isSimpleProperty(node)) {
+      if (node != null && this.isObjectProperty(node)) {
         this.checkDeclaration(((node: any): N.EstreeProperty).value);
       } else {
         super.checkDeclaration(node);
@@ -110,44 +105,6 @@ export default (superClass: Class<Parser>): Class<Parser> =>
         .params;
     }
 
-    checkLVal(
-      expr: N.Expression,
-      contextDescription: string,
-      ...args: [
-        BindingTypes | void,
-        ?Set<string>,
-        boolean | void,
-        boolean | void,
-      ]
-    ): void {
-      switch (expr.type) {
-        case "ObjectPattern":
-          expr.properties.forEach(prop => {
-            this.checkLVal(
-              prop.type === "Property" ? prop.value : prop,
-              "object destructuring pattern",
-              ...args,
-            );
-          });
-          break;
-        default:
-          super.checkLVal(expr, contextDescription, ...args);
-      }
-    }
-
-    checkProto(
-      prop: N.ObjectMember | N.SpreadElement,
-      isRecord: boolean,
-      protoRef: { used: boolean },
-      refExpressionErrors: ?ExpressionErrors,
-    ): void {
-      // $FlowIgnore: check prop.method and fallback to super method
-      if (prop.method) {
-        return;
-      }
-      super.checkProto(prop, isRecord, protoRef, refExpressionErrors);
-    }
-
     isValidDirective(stmt: N.Statement): boolean {
       return (
         stmt.type === "ExpressionStatement" &&
@@ -170,11 +127,9 @@ export default (superClass: Class<Parser>): Class<Parser> =>
 
     parseBlockBody(
       node: N.BlockStatementLike,
-      allowDirectives: ?boolean,
+      ...args: [?boolean, boolean, TokenType, void | (boolean => void)]
-      topLevel: boolean,
-      end: TokenType,
     ): void {
-      super.parseBlockBody(node, allowDirectives, topLevel, end);
+      super.parseBlockBody(node, ...args);
 
       const directiveStatements = node.directives.map(d =>
         this.directiveToStmt(d),
@@ -337,8 +292,8 @@ export default (superClass: Class<Parser>): Class<Parser> =>
     }
 
     toAssignable(node: N.Node, isLHS: boolean = false): N.Node {
-      if (isSimpleProperty(node)) {
+      if (node != null && this.isObjectProperty(node)) {
-        this.toAssignable(node.value);
+        this.toAssignable(node.value, isLHS);
 
         return node;
       }
@@ -348,9 +303,9 @@ export default (superClass: Class<Parser>): Class<Parser> =>
 
     toAssignableObjectExpressionProp(prop: N.Node, ...args) {
       if (prop.kind === "get" || prop.kind === "set") {
-        throw this.raise(prop.key.start, Errors.PatternHasAccessor);
+        this.raise(prop.key.start, Errors.PatternHasAccessor);
       } else if (prop.method) {
-        throw this.raise(prop.key.start, Errors.PatternHasMethod);
+        this.raise(prop.key.start, Errors.PatternHasMethod);
       } else {
         super.toAssignableObjectExpressionProp(prop, ...args);
       }
@@ -450,4 +405,23 @@ export default (superClass: Class<Parser>): Class<Parser> =>
 
       return node;
     }
+
+    hasPropertyAsPrivateName(node: N.Node): boolean {
+      if (node.type === "ChainExpression") {
+        node = node.expression;
+      }
+      return super.hasPropertyAsPrivateName(node);
+    }
+
+    isOptionalChain(node: N.Node): boolean {
+      return node.type === "ChainExpression";
+    }
+
+    isObjectProperty(node: N.Node): boolean {
+      return node.type === "Property" && node.kind === "init" && !node.method;
+    }
+
+    isObjectMethod(node: N.Node): boolean {
+      return node.method || node.kind === "get" || node.kind === "set";
+    }
   };

packages/babel-parser/test/fixtures/experimental/module-string-names/string-exported-binding-without-from/output.json

@@ -2,8 +2,8 @@
   "type": "File",
   "start":0,"end":45,"loc":{"start":{"line":1,"column":0},"end":{"line":1,"column":45}},
   "errors": [
-    "SyntaxError: A string literal cannot be used as an exported binding without `from`.\n- Did you mean `export { \"學而時習之，不亦說乎？\" as '學而時習之，不亦說乎？' } from 'some-module'`? (1:9)",
+    "SyntaxError: A string literal cannot be used as an exported binding without `from`.\n- Did you mean `export { '學而時習之，不亦說乎？' as '學而時習之，不亦說乎？' } from 'some-module'`? (1:9)",
-    "SyntaxError: A string literal cannot be used as an exported binding without `from`.\n- Did you mean `export { \"吾道一以貫之。\" as '忠恕。' } from 'some-module'`? (1:24)"
+    "SyntaxError: A string literal cannot be used as an exported binding without `from`.\n- Did you mean `export { '吾道一以貫之。' as '忠恕。' } from 'some-module'`? (1:24)"
   ],
   "program": {
     "type": "Program",

packages/babel-parser/test/helpers/runFixtureTests.js

@@ -83,7 +83,13 @@ export function runThrowTestsWithEstree(fixturesPath, parseFunction) {
   Object.keys(fixtures).forEach(function (name) {
     fixtures[name].forEach(function (testSuite) {
       testSuite.tests.forEach(function (task) {
-        if (!task.options.throws) return;
+        if (!task.options.throws) {
+          const hasErrors =
+            !task.disabled && "errors" in JSON.parse(task.expect.code);
+          if (!hasErrors) {
+            return;
+          }
+        }
 
         task.options.plugins = task.options.plugins || [];
         task.options.plugins.push("estree");
@@ -92,7 +98,7 @@ export function runThrowTestsWithEstree(fixturesPath, parseFunction) {
 
         testFn(name + "/" + testSuite.title + "/" + task.title, function () {
           try {
-            runTest(task, parseFunction);
+            runTest(task, parseFunction, true);
           } catch (err) {
             const fixturePath = `${path.relative(
               rootPath,
@@ -126,7 +132,19 @@ function save(test, ast) {
   );
 }
 
-function runTest(test, parseFunction) {
+/**
+ * run parser on given tests
+ *
+ * @param {Test} A {@link packages/babel-helper-fixtures/src/index.js Test} instance
+ generated from `getFixtures`
+ * @param {*} parseFunction A parser with the same interface of `@babel/parser#parse`
+ * @param {boolean} [compareErrorsOnly=false] Whether we should only compare the "errors"
+ * of generated ast against the expected AST. Used for `runThrowTestsWithEstree` where an
+ * ESTree AST is generated but we want to make sure `@babel/parser` still throws expected
+ * recoverable errors on given code locations.
+ * @returns {void}
+ */
+function runTest(test, parseFunction, compareErrorsOnly = false) {
   const opts = test.options;
 
   if (opts.throws && test.expect.code) {
@@ -189,6 +207,11 @@ function runTest(test, parseFunction) {
     throw new Error(
       "Expected error message: " + opts.throws + ". But parsing succeeded.",
     );
+  } else if (compareErrorsOnly) {
+    const mis = misMatch(JSON.parse(test.expect.code).errors, ast.errors);
+    if (mis) {
+      throw new Error(mis);
+    }
   } else {
     const mis = misMatch(JSON.parse(test.expect.code), ast);