Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: reorder checkLVal parameters #12346

Merged
merged 3 commits into from Nov 17, 2020
+87 −123
Merged

refactor: reorder checkLVal parameters #12346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2a9bccb
refactor: reorder checkLVal parameters
JLHwung Nov 11, 2020
c7e06cc
update allow list
JLHwung Nov 11, 2020
e830de5
Apply suggestions from code review
JLHwung Nov 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 9 additions & 10 deletions packages/babel-parser/src/parser/expression.js
View file
Open in desktop
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
Open in desktop
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}`;
Copy link
Contributor Author

@JLHwung JLHwung Nov 11, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implement checkClashes with Set so we don't have to deal with old V8 bug.


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
Open in desktop
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
Open in desktop
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
Open in desktop
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