Skip to content

Commit

Permalink
refactor: reorder checkLVal parameters (#12346)
Browse files Browse the repository at this point in the history 
* refactor: reorder checkLVal parameters

* update allow list

* Apply suggestions from code review
  • Loading branch information
@JLHwung
JLHwung committed Nov 17, 2020
1 parent 0f838be commit b564368
Show file tree
Hide file tree
Showing 7 changed files with 87 additions and 123 deletions.
19 changes: 9 additions & 10 deletions packages/babel-parser/src/parser/expression.js
View file
Expand Up @@ -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();
Expand Down Expand Up @@ -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");
}

Expand All @@ -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");
}
Expand Down Expand Up @@ -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,
);
Expand Down Expand Up @@ -2139,14 +2139,13 @@ export default class ExpressionParser extends LValParser {
isArrowFunction: ?boolean,
strictModeChanged?: boolean = true,
): void {
// $FlowIssue
const nameHash: {} = Object.create(null);
for (let i = 0; i < node.params.length; i++) {
const checkClashes = new Set();
for (const param of node.params) {
this.checkLVal(
node.params[i],
BIND_VAR,
allowDuplicates ? null : nameHash,
param,
"function parameter list",
BIND_VAR,
allowDuplicates ? null : checkClashes,
undefined,
strictModeChanged,
);
Expand Down
64 changes: 33 additions & 31 deletions packages/babel-parser/src/parser/lval.js
View file
Expand Up @@ -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)
: isStrictBindOnlyReservedWord(expr.name))
? isStrictBindReservedWord(name, this.inModule)
: 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
// `__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]) {
if (checkClashes.has(name)) {
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) {
Expand All @@ -451,9 +453,9 @@ export default class LValParser extends NodeUtils {

this.checkLVal(
prop,
"object destructuring pattern",
bindingType,
checkClashes,
"object destructuring pattern",
disallowLetBinding,
);
}
Expand All @@ -464,9 +466,9 @@ export default class LValParser extends NodeUtils {
if (elem) {
this.checkLVal(
elem,
"array destructuring pattern",
bindingType,
checkClashes,
"array destructuring pattern",
disallowLetBinding,
);
}
Expand All @@ -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;

Expand Down
22 changes: 6 additions & 16 deletions packages/babel-parser/src/parser/statement.js
View file
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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",
);
}
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -2173,12 +2173,7 @@ export default class StatementParser extends ExpressionParser {
contextDescription: string,
): void {
specifier.local = this.parseIdentifier();
this.checkLVal(
specifier.local,
BIND_LEXICAL,
undefined,
contextDescription,
);
this.checkLVal(specifier.local, contextDescription, BIND_LEXICAL);
node.specifiers.push(this.finishNode(specifier, type));
}
Expand Down Expand Up @@ -2383,12 +2378,7 @@ export default class StatementParser extends ExpressionParser {
this.checkReservedWord(imported.name, specifier.start, true, true);
specifier.local = imported.__clone();
}
this.checkLVal(
specifier.local,
BIND_LEXICAL,
undefined,
"import specifier",
);
this.checkLVal(specifier.local, "import specifier", BIND_LEXICAL);
node.specifiers.push(this.finishNode(specifier, "ImportSpecifier"));
}
}
23 changes: 9 additions & 14 deletions packages/babel-parser/src/plugins/estree.js
View file
Expand Up @@ -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 {
Expand Down Expand Up @@ -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(
expr,
bindingType,
checkClashes,
contextDescription,
disallowLetBinding,
);
super.checkLVal(expr, contextDescription, ...args);
}
}

Expand Down
34 changes: 12 additions & 22 deletions packages/babel-parser/src/plugins/flow.js
View file
Expand Up @@ -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,
Expand Down Expand Up @@ -2264,17 +2263,18 @@ export default (superClass: Class<Parser>): Class<Parser> =>

checkLVal(
expr: N.Expression,
bindingType: BindingTypes = BIND_NONE,
checkClashes: ?{ [key: string]: boolean },
contextDescription: string,
...args:
| [string, BindingTypes | void]
| [
string,
BindingTypes | void,
?Set<string>,
boolean | void,
boolean | void,
]
): void {
if (expr.type !== "TypeCastExpression") {
return super.checkLVal(
expr,
bindingType,
checkClashes,
contextDescription,
);
return super.checkLVal(expr, ...args);
}
}

Expand Down Expand Up @@ -2481,12 +2481,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
)
: this.parseIdentifier();

this.checkLVal(
specifier.local,
BIND_LEXICAL,
undefined,
contextDescription,
);
this.checkLVal(specifier.local, contextDescription, BIND_LEXICAL);
node.specifiers.push(this.finishNode(specifier, type));
}

Expand Down Expand Up @@ -2608,12 +2603,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
);
}

this.checkLVal(
specifier.local,
BIND_LEXICAL,
undefined,
"import specifier",
);
this.checkLVal(specifier.local, "import specifier", BIND_LEXICAL);
node.specifiers.push(this.finishNode(specifier, "ImportSpecifier"));
}

Expand Down

0 comments on commit b564368

Please sign in to comment.