refactor: reorder checkLVal parameters (#12346)

* refactor: reorder checkLVal parameters

* update allow list

* Apply suggestions from code review

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

@@ -288,7 +288,7 @@ export default class ExpressionParser extends LValParser {
         refExpressionErrors.shorthandAssign = -1; // reset because shorthand default was used correctly
       }
 
-      this.checkLVal(left, undefined, undefined, "assignment expression");
+      this.checkLVal(left, "assignment expression");
 
       this.next();
       node.right = this.parseMaybeAssign();
@@ -539,7 +539,7 @@ export default class ExpressionParser extends LValParser {
     refExpressionErrors: ?ExpressionErrors,
   ): N.Expression {
     if (update) {
-      this.checkLVal(node.argument, undefined, undefined, "prefix operation");
+      this.checkLVal(node.argument, "prefix operation");
       return this.finishNode(node, "UpdateExpression");
     }
 
@@ -552,7 +552,7 @@ export default class ExpressionParser extends LValParser {
       node.operator = this.state.value;
       node.prefix = false;
       node.argument = expr;
-      this.checkLVal(expr, undefined, undefined, "postfix operation");
+      this.checkLVal(expr, "postfix operation");
       this.next();
       expr = this.finishNode(node, "UpdateExpression");
     }
@@ -2108,9 +2108,9 @@ export default class ExpressionParser extends LValParser {
           if (this.state.strict && node.id) {
             this.checkLVal(
               node.id,
+              "function name",
               BIND_OUTSIDE,
               undefined,
-              "function name",
               undefined,
               strictModeChanged,
             );
@@ -2139,14 +2139,13 @@ export default class ExpressionParser extends LValParser {
     isArrowFunction: ?boolean,
     strictModeChanged?: boolean = true,
   ): void {
-    // $FlowIssue
+    const checkClashes = new Set();
-    const nameHash: {} = Object.create(null);
+    for (const param of node.params) {
-    for (let i = 0; i < node.params.length; i++) {
       this.checkLVal(
-        node.params[i],
+        param,
-        BIND_VAR,
-        allowDuplicates ? null : nameHash,
         "function parameter list",
+        BIND_VAR,
+        allowDuplicates ? null : checkClashes,
         undefined,
         strictModeChanged,
       );

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

@@ -376,64 +376,66 @@ export default class LValParser extends NodeUtils {
     return this.finishNode(node, "AssignmentPattern");
   }
 
-  // Verify that a node is an lval — something that can be assigned
+  /**
-  // to.
+   * Verify that if a node is an lval - something that can be assigned to.
-
+   *
+   * @param {Expression} expr The given node
+   * @param {string} contextDescription The auxiliary context information printed when error is thrown
+   * @param {BindingTypes} [bindingType=BIND_NONE] The desired binding type. If the given node is an identifier and `bindingType` is not
+                                                   BIND_NONE, `checkLVal` will register binding to the parser scope
+                                                   See also src/util/scopeflags.js
+   * @param {?Set<string>} checkClashes An optional string set to check if an identifier name is included. `checkLVal` will add checked
+                                        identifier name to `checkClashes` It is used in tracking duplicates in function parameter lists. If
+                                        it is nullish, `checkLVal` will skip duplicate checks
+   * @param {boolean} [disallowLetBinding] Whether an identifier named "let" should be disallowed
+   * @param {boolean} [strictModeChanged=false] Whether an identifier has been parsed in a sloppy context but should be reinterpreted as
+                                                strict-mode. e.g. `(arguments) => { "use strict "}`
+   * @memberof LValParser
+   */
   checkLVal(
     expr: Expression,
-    bindingType: BindingTypes = BIND_NONE,
-    checkClashes: ?{ [key: string]: boolean },
     contextDescription: string,
+    bindingType: BindingTypes = BIND_NONE,
+    checkClashes: ?Set<string>,
     disallowLetBinding?: boolean,
     strictModeChanged?: boolean = false,
   ): void {
     switch (expr.type) {
-      case "Identifier":
+      case "Identifier": {
+        const { name } = expr;
         if (
           this.state.strict &&
           // "Global" reserved words have already been checked by parseIdentifier,
           // unless they have been found in the id or parameters of a strict-mode
           // function in a sloppy context.
           (strictModeChanged
-            ? isStrictBindReservedWord(expr.name, this.inModule)
+            ? isStrictBindReservedWord(name, this.inModule)
-            : isStrictBindOnlyReservedWord(expr.name))
+            : isStrictBindOnlyReservedWord(name))
         ) {
           this.raise(
             expr.start,
             bindingType === BIND_NONE
               ? Errors.StrictEvalArguments
               : Errors.StrictEvalArgumentsBinding,
-            expr.name,
+            name,
           );
         }
 
         if (checkClashes) {
-          // we need to prefix this with an underscore for the cases where we have a key of
+          if (checkClashes.has(name)) {
-          // `__proto__`. there's a bug in old V8 where the following wouldn't work:
-          //
-          //   > var obj = Object.create(null);
-          //   undefined
-          //   > obj.__proto__
-          //   null
-          //   > obj.__proto__ = true;
-          //   true
-          //   > obj.__proto__
-          //   null
-          const key = `_${expr.name}`;
-
-          if (checkClashes[key]) {
             this.raise(expr.start, Errors.ParamDupe);
           } else {
-            checkClashes[key] = true;
+            checkClashes.add(name);
           }
         }
-        if (disallowLetBinding && expr.name === "let") {
+        if (disallowLetBinding && name === "let") {
           this.raise(expr.start, Errors.LetInLexicalBinding);
         }
         if (!(bindingType & BIND_NONE)) {
-          this.scope.declareName(expr.name, bindingType, expr.start);
+          this.scope.declareName(name, bindingType, expr.start);
         }
         break;
+      }
 
       case "MemberExpression":
         if (bindingType !== BIND_NONE) {
@@ -451,9 +453,9 @@ export default class LValParser extends NodeUtils {
 
           this.checkLVal(
             prop,
+            "object destructuring pattern",
             bindingType,
             checkClashes,
-            "object destructuring pattern",
             disallowLetBinding,
           );
         }
@@ -464,9 +466,9 @@ export default class LValParser extends NodeUtils {
           if (elem) {
             this.checkLVal(
               elem,
+              "array destructuring pattern",
               bindingType,
               checkClashes,
-              "array destructuring pattern",
               disallowLetBinding,
             );
           }
@@ -476,27 +478,27 @@ export default class LValParser extends NodeUtils {
       case "AssignmentPattern":
         this.checkLVal(
           expr.left,
+          "assignment pattern",
           bindingType,
           checkClashes,
-          "assignment pattern",
         );
         break;
 
       case "RestElement":
         this.checkLVal(
           expr.argument,
+          "rest element",
           bindingType,
           checkClashes,
-          "rest element",
         );
         break;
 
       case "ParenthesizedExpression":
         this.checkLVal(
           expr.expression,
+          "parenthesized expression",
           bindingType,
           checkClashes,
-          "parenthesized expression",
         );
         break;
 

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

@@ -532,7 +532,7 @@ export default class StatementParser extends ExpressionParser {
       const description = this.isContextual("of")
         ? "for-of statement"
         : "for-in statement";
-      this.checkLVal(init, undefined, undefined, description);
+      this.checkLVal(init, description);
       return this.parseForIn(node, init, awaitAt);
     } else {
       this.checkExpressionErrors(refExpressionErrors, true);
@@ -648,7 +648,7 @@ export default class StatementParser extends ExpressionParser {
 
     const simple = param.type === "Identifier";
     this.scope.enter(simple ? SCOPE_SIMPLE_CATCH : 0);
-    this.checkLVal(param, BIND_LEXICAL, null, "catch clause");
+    this.checkLVal(param, "catch clause", BIND_LEXICAL);
 
     return param;
   }
@@ -1050,9 +1050,9 @@ export default class StatementParser extends ExpressionParser {
     decl.id = this.parseBindingAtom();
     this.checkLVal(
       decl.id,
+      "variable declaration",
       kind === "var" ? BIND_VAR : BIND_LEXICAL,
       undefined,
-      "variable declaration",
       kind !== "var",
     );
   }
@@ -1675,7 +1675,7 @@ export default class StatementParser extends ExpressionParser {
     if (this.match(tt.name)) {
       node.id = this.parseIdentifier();
       if (isStatement) {
-        this.checkLVal(node.id, bindingType, undefined, "class name");
+        this.checkLVal(node.id, "class name", bindingType);
       }
     } else {
       if (optionalId || !isStatement) {
@@ -2173,12 +2173,7 @@ export default class StatementParser extends ExpressionParser {
     contextDescription: string,
   ): void {
     specifier.local = this.parseIdentifier();
-    this.checkLVal(
+    this.checkLVal(specifier.local, contextDescription, BIND_LEXICAL);
-      specifier.local,
-      BIND_LEXICAL,
-      undefined,
-      contextDescription,
-    );
     node.specifiers.push(this.finishNode(specifier, type));
   }
 
@@ -2383,12 +2378,7 @@ export default class StatementParser extends ExpressionParser {
       this.checkReservedWord(imported.name, specifier.start, true, true);
       specifier.local = imported.__clone();
     }
-    this.checkLVal(
+    this.checkLVal(specifier.local, "import specifier", BIND_LEXICAL);
-      specifier.local,
-      BIND_LEXICAL,
-      undefined,
-      "import specifier",
-    );
     node.specifiers.push(this.finishNode(specifier, "ImportSpecifier"));
   }
 }

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

@@ -5,7 +5,7 @@ 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, BIND_NONE } from "../util/scopeflags";
+import { type BindingTypes } from "../util/scopeflags";
 import { Errors } from "../parser/error";
 
 function isSimpleProperty(node: N.Node): boolean {
@@ -112,31 +112,26 @@ export default (superClass: Class<Parser>): Class<Parser> =>
 
     checkLVal(
       expr: N.Expression,
-      bindingType: BindingTypes = BIND_NONE,
-      checkClashes: ?{ [key: string]: boolean },
       contextDescription: string,
-      disallowLetBinding?: boolean,
+      ...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,
-              bindingType,
-              checkClashes,
               "object destructuring pattern",
-              disallowLetBinding,
+              ...args,
             );
           });
           break;
         default:
-          super.checkLVal(
+          super.checkLVal(expr, contextDescription, ...args);
-            expr,
-            bindingType,
-            checkClashes,
-            contextDescription,
-            disallowLetBinding,
-          );
       }
     }
 

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

@@ -16,7 +16,6 @@ import * as charCodes from "charcodes";
 import { isIteratorStart, isKeyword } from "../util/identifier";
 import {
   type BindingTypes,
-  BIND_NONE,
   BIND_LEXICAL,
   BIND_VAR,
   BIND_FUNCTION,
@@ -2264,17 +2263,18 @@ export default (superClass: Class<Parser>): Class<Parser> =>
 
     checkLVal(
       expr: N.Expression,
-      bindingType: BindingTypes = BIND_NONE,
+      ...args:
-      checkClashes: ?{ [key: string]: boolean },
+        | [string, BindingTypes | void]
-      contextDescription: string,
+        | [
+            string,
+            BindingTypes | void,
+            ?Set<string>,
+            boolean | void,
+            boolean | void,
+          ]
     ): void {
       if (expr.type !== "TypeCastExpression") {
-        return super.checkLVal(
+        return super.checkLVal(expr, ...args);
-          expr,
-          bindingType,
-          checkClashes,
-          contextDescription,
-        );
       }
     }
 
@@ -2481,12 +2481,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
           )
         : this.parseIdentifier();
 
-      this.checkLVal(
+      this.checkLVal(specifier.local, contextDescription, BIND_LEXICAL);
-        specifier.local,
-        BIND_LEXICAL,
-        undefined,
-        contextDescription,
-      );
       node.specifiers.push(this.finishNode(specifier, type));
     }
 
@@ -2608,12 +2603,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
         );
       }
 
-      this.checkLVal(
+      this.checkLVal(specifier.local, "import specifier", BIND_LEXICAL);
-        specifier.local,
-        BIND_LEXICAL,
-        undefined,
-        "import specifier",
-      );
       node.specifiers.push(this.finishNode(specifier, "ImportSpecifier"));
     }
 

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

@@ -14,7 +14,6 @@ import type { Pos, Position } from "../../util/location";
 import type Parser from "../../parser";
 import {
   type BindingTypes,
-  BIND_NONE,
   SCOPE_TS_MODULE,
   SCOPE_OTHER,
   BIND_TS_ENUM,
@@ -1221,9 +1220,8 @@ export default (superClass: Class<Parser>): Class<Parser> =>
       node.id = this.parseIdentifier();
       this.checkLVal(
         node.id,
-        BIND_TS_INTERFACE,
-        undefined,
         "typescript interface declaration",
+        BIND_TS_INTERFACE,
       );
       node.typeParameters = this.tsTryParseTypeParameters();
       if (this.eat(tt._extends)) {
@@ -1239,7 +1237,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
       node: N.TsTypeAliasDeclaration,
     ): N.TsTypeAliasDeclaration {
       node.id = this.parseIdentifier();
-      this.checkLVal(node.id, BIND_TS_TYPE, undefined, "typescript type alias");
+      this.checkLVal(node.id, "typescript type alias", BIND_TS_TYPE);
 
       node.typeParameters = this.tsTryParseTypeParameters();
       node.typeAnnotation = this.tsInType(() => {
@@ -1325,9 +1323,8 @@ export default (superClass: Class<Parser>): Class<Parser> =>
       node.id = this.parseIdentifier();
       this.checkLVal(
         node.id,
-        isConst ? BIND_TS_CONST_ENUM : BIND_TS_ENUM,
-        undefined,
         "typescript enum declaration",
+        isConst ? BIND_TS_CONST_ENUM : BIND_TS_ENUM,
       );
 
       this.expect(tt.braceL);
@@ -1364,9 +1361,8 @@ export default (superClass: Class<Parser>): Class<Parser> =>
       if (!nested) {
         this.checkLVal(
           node.id,
-          BIND_TS_NAMESPACE,
-          null,
           "module or namespace declaration",
+          BIND_TS_NAMESPACE,
         );
       }
 
@@ -1414,12 +1410,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
     ): N.TsImportEqualsDeclaration {
       node.isExport = isExport || false;
       node.id = this.parseIdentifier();
-      this.checkLVal(
+      this.checkLVal(node.id, "import equals declaration", BIND_LEXICAL);
-        node.id,
-        BIND_LEXICAL,
-        undefined,
-        "import equals declaration",
-      );
       this.expect(tt.eq);
       node.moduleReference = this.tsParseModuleReference();
       this.semicolon();
@@ -1805,7 +1796,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
       if (!node.body && node.id) {
         // Function ids are validated after parsing their body.
         // For bodyless function, we need to do it here.
-        this.checkLVal(node.id, BIND_TS_AMBIENT, null, "function name");
+        this.checkLVal(node.id, "function name", BIND_TS_AMBIENT);
       } else {
         super.registerFunctionStatementId(...arguments);
       }
@@ -2615,9 +2606,10 @@ export default (superClass: Class<Parser>): Class<Parser> =>
 
     checkLVal(
       expr: N.Expression,
-      bindingType: BindingTypes = BIND_NONE,
-      checkClashes: ?{ [key: string]: boolean },
       contextDescription: string,
+      ...args:
+        | [BindingTypes | void]
+        | [BindingTypes | void, ?Set<string>, boolean | void, boolean | void]
     ): void {
       switch (expr.type) {
         case "TSTypeCastExpression":
@@ -2626,25 +2618,15 @@ export default (superClass: Class<Parser>): Class<Parser> =>
           // e.g. `const f = (foo: number = 0) => foo;`
           return;
         case "TSParameterProperty":
-          this.checkLVal(
+          this.checkLVal(expr.parameter, "parameter property", ...args);
-            expr.parameter,
-            bindingType,
-            checkClashes,
-            "parameter property",
-          );
           return;
         case "TSAsExpression":
         case "TSNonNullExpression":
         case "TSTypeAssertion":
-          this.checkLVal(
+          this.checkLVal(expr.expression, contextDescription, ...args);
-            expr.expression,
-            bindingType,
-            checkClashes,
-            contextDescription,
-          );
           return;
         default:
-          super.checkLVal(expr, bindingType, checkClashes, contextDescription);
+          super.checkLVal(expr, contextDescription, ...args);
           return;
       }
     }

scripts/parser-tests/typescript/allowlist.txt

@@ -295,6 +295,12 @@ letDeclarations-scopes-duplicates6.ts
 letDeclarations-scopes-duplicates7.ts
 letDeclarations-scopes.ts
 letDeclarations-validContexts.ts
+letInConstDeclarations_ES5.ts
+letInConstDeclarations_ES6.ts
+letInLetConstDeclOfForOfAndForIn_ES5.ts
+letInLetConstDeclOfForOfAndForIn_ES6.ts
+letInLetDeclarations_ES5.ts
+letInLetDeclarations_ES6.ts
 mergeWithImportedType.ts
 mergedDeclarations6.ts
 metadataOfClassFromAlias.ts