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.

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

fix: disallow all parenthesized pattern except parsing LHS #12327

Merged
merged 9 commits into from Nov 10, 2020
22 changes: 2 additions & 20 deletions packages/babel-parser/src/parser/expression.js
Expand Up @@ -278,7 +278,7 @@ export default class ExpressionParser extends LValParser {
node.operator = operator;

if (this.match(tt.eq)) {
node.left = this.toAssignable(left);
node.left = this.toAssignable(left, /* isLHS */ true);
refExpressionErrors.doubleProto = -1; // reset because double __proto__ is valid in assignment expression
} else {
node.left = left;
Expand Down Expand Up @@ -842,7 +842,6 @@ export default class ExpressionParser extends LValParser {
nodeForExtra?: ?N.Node,
): $ReadOnlyArray<?N.Expression> {
const elts = [];
let innerParenStart;
let first = true;
const oldInFSharpPipelineDirectBody = this.state.inFSharpPipelineDirectBody;
this.state.inFSharpPipelineDirectBody = false;
Expand Down Expand Up @@ -875,12 +874,6 @@ export default class ExpressionParser extends LValParser {
}
}

// we need to make sure that if this is an async arrow functions,
// that we don't allow inner parens inside the params
if (this.match(tt.parenL) && !innerParenStart) {
innerParenStart = this.state.start;
}

elts.push(
this.parseExprListItem(
false,
Expand All @@ -891,11 +884,6 @@ export default class ExpressionParser extends LValParser {
);
}

// we found an async arrow function so let's not allow any inner parens
if (possibleAsyncArrow && innerParenStart && this.shouldParseAsyncArrow()) {
this.unexpected();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of throwing unexpected errors on

var foo = async ((foo)) => {};

we can now recover and raise this error in toAssignable, which I think is more helpful

"SyntaxError: Invalid parenthesized assignment pattern (1:18)"

}

this.state.inFSharpPipelineDirectBody = oldInFSharpPipelineDirectBody;

return elts;
Expand Down Expand Up @@ -1421,12 +1409,6 @@ export default class ExpressionParser extends LValParser {
) {
this.expressionScope.validateAsPattern();
this.expressionScope.exit();
for (const param of exprList) {
if (param.extra && param.extra.parenthesized) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can now recover from var foo = ((foo)) => {};

this.unexpected(param.extra.parenStart);
}
}

this.parseArrowExpression(arrowNode, exprList, false);
return arrowNode;
}
Expand Down Expand Up @@ -2056,7 +2038,7 @@ export default class ExpressionParser extends LValParser {
params: N.Expression[],
trailingCommaPos: ?number,
): void {
node.params = this.toAssignableList(params, trailingCommaPos);
node.params = this.toAssignableList(params, trailingCommaPos, false);
}

parseFunctionBodyAndFinish(
Expand Down
76 changes: 55 additions & 21 deletions packages/babel-parser/src/parser/lval.js
@@ -1,5 +1,6 @@
// @flow

/*:: declare var invariant; */
import * as charCodes from "charcodes";
import { types as tt, type TokenType } from "../tokenizer/types";
import type {
Expand All @@ -24,7 +25,7 @@ import { type BindingTypes, BIND_NONE } from "../util/scopeflags";
import { ExpressionErrors } from "./util";
import { Errors } from "./error";

const unwrapParenthesizedExpression = (node: Node) => {
const unwrapParenthesizedExpression = (node: Node): Node => {
return node.type === "ParenthesizedExpression"
? unwrapParenthesizedExpression(node.expression)
: node;
Expand All @@ -51,19 +52,45 @@ export default class LValParser extends NodeUtils {
+parseDecorator: () => Decorator;
*/

// Convert existing expression atom to assignable pattern
// if possible.
// NOTE: There is a corresponding "isAssignable" method in flow.js.
// When this one is updated, please check if also that one needs to be updated.
/**
* Convert existing expression atom to assignable pattern
* if possible. Also checks invalid destructuring targets:

toAssignable(node: Node): Node {
- Parenthesized Destructuring patterns
- RestElement is not the last element
- Missing `=` in assignment pattern

NOTE: There is a corresponding "isAssignable" method in flow.js.
When this one is updated, please check if also that one needs to be updated.

* @param {Node} node The expression atom
* @param {boolean} [isLHS=false] Whether we are parsing a LeftHandSideExpression. If isLHS is `true`, the following cases are allowed:
`[(a)] = [0]`, `[(a.b)] = [0]`

* @returns {Node} The converted assignable pattern
* @memberof LValParser
*/
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for all the doc comments you are introducing!

toAssignable(node: Node, isLHS: boolean = false): Node {
let parenthesized = undefined;
if (node.type === "ParenthesizedExpression" || node.extra?.parenthesized) {
parenthesized = unwrapParenthesizedExpression(node);
if (
parenthesized.type !== "Identifier" &&
parenthesized.type !== "MemberExpression"
) {
if (isLHS) {
// an LHS can be reinterpreted to a binding pattern but not vice versa.
// therefore a parenthesized identifier is ambiguous until we are sure it is an assignment expression
// i.e. `([(a) = []] = []) => {}`
// see also `recordParenthesizedIdentifierError` signature in packages/babel-parser/src/util/expression-scope.js
if (parenthesized.type === "Identifier") {
this.expressionScope.recordParenthesizedIdentifierError(
node.start,
Errors.InvalidParenthesizedAssignment,
);
} else if (parenthesized.type !== "MemberExpression") {
// A parenthesized member expression can be in LHS but not in pattern.
// If the LHS is later interpreted as a pattern, `checkLVal` will throw for member expression binding
// i.e. `([(a.b) = []] = []) => {}`
this.raise(node.start, Errors.InvalidParenthesizedAssignment);
}
} else {
this.raise(node.start, Errors.InvalidParenthesizedAssignment);
}
}
Expand All @@ -84,7 +111,7 @@ export default class LValParser extends NodeUtils {
) {
const prop = node.properties[i];
const isLast = i === last;
this.toAssignableObjectExpressionProp(prop, isLast);
this.toAssignableObjectExpressionProp(prop, isLast, isLHS);

if (
isLast &&
Expand All @@ -97,21 +124,21 @@ export default class LValParser extends NodeUtils {
break;

case "ObjectProperty":
this.toAssignable(node.value);
this.toAssignable(node.value, isLHS);
break;

case "SpreadElement": {
this.checkToRestConversion(node);

node.type = "RestElement";
const arg = node.argument;
this.toAssignable(arg);
this.toAssignable(arg, isLHS);
break;
}

case "ArrayExpression":
node.type = "ArrayPattern";
this.toAssignableList(node.elements, node.extra?.trailingComma);
this.toAssignableList(node.elements, node.extra?.trailingComma, isLHS);
break;

case "AssignmentExpression":
Expand All @@ -121,11 +148,12 @@ export default class LValParser extends NodeUtils {

node.type = "AssignmentPattern";
delete node.operator;
this.toAssignable(node.left);
this.toAssignable(node.left, isLHS);
break;

case "ParenthesizedExpression":
this.toAssignable(((parenthesized: any): Expression));
/*::invariant (parenthesized !== undefined) */
this.toAssignable(parenthesized, isLHS);
break;

default:
Expand All @@ -135,7 +163,11 @@ export default class LValParser extends NodeUtils {
return node;
}

toAssignableObjectExpressionProp(prop: Node, isLast: boolean) {
toAssignableObjectExpressionProp(
prop: Node,
isLast: boolean,
isLHS: boolean,
) {
if (prop.type === "ObjectMethod") {
const error =
prop.kind === "get" || prop.kind === "set"
Expand All @@ -148,7 +180,7 @@ export default class LValParser extends NodeUtils {
} else if (prop.type === "SpreadElement" && !isLast) {
this.raiseRestNotLast(prop.start);
} else {
this.toAssignable(prop);
this.toAssignable(prop, isLHS);
}
}

Expand All @@ -157,6 +189,7 @@ export default class LValParser extends NodeUtils {
toAssignableList(
exprList: Expression[],
trailingCommaPos?: ?number,
isLHS: boolean,
): $ReadOnlyArray<Pattern> {
let end = exprList.length;
if (end) {
Expand All @@ -165,8 +198,9 @@ export default class LValParser extends NodeUtils {
--end;
} else if (last?.type === "SpreadElement") {
last.type = "RestElement";
const arg = last.argument;
this.toAssignable(arg);
let arg = last.argument;
this.toAssignable(arg, isLHS);
arg = unwrapParenthesizedExpression(arg);
if (
arg.type !== "Identifier" &&
arg.type !== "MemberExpression" &&
Expand All @@ -186,7 +220,7 @@ export default class LValParser extends NodeUtils {
for (let i = 0; i < end; i++) {
const elt = exprList[i];
if (elt) {
this.toAssignable(elt);
this.toAssignable(elt, isLHS);
if (elt.type === "RestElement") {
this.raiseRestNotLast(elt.start);
}
Expand Down
2 changes: 1 addition & 1 deletion packages/babel-parser/src/parser/statement.js
Expand Up @@ -528,7 +528,7 @@ export default class StatementParser extends ExpressionParser {
const refExpressionErrors = new ExpressionErrors();
const init = this.parseExpression(true, refExpressionErrors);
if (this.match(tt._in) || this.isContextual("of")) {
this.toAssignable(init);
this.toAssignable(init, /* isLHS */ true);
const description = this.isContextual("of")
? "for-of statement"
: "for-in statement";
Expand Down
8 changes: 4 additions & 4 deletions packages/babel-parser/src/plugins/estree.js
Expand Up @@ -341,23 +341,23 @@ export default (superClass: Class<Parser>): Class<Parser> =>
return (node: any);
}

toAssignable(node: N.Node): N.Node {
toAssignable(node: N.Node, isLHS: boolean = false): N.Node {
if (isSimpleProperty(node)) {
this.toAssignable(node.value);

return node;
}

return super.toAssignable(node);
return super.toAssignable(node, isLHS);
}

toAssignableObjectExpressionProp(prop: N.Node, isLast: boolean) {
toAssignableObjectExpressionProp(prop: N.Node, ...args) {
if (prop.kind === "get" || prop.kind === "set") {
throw this.raise(prop.key.start, Errors.PatternHasAccessor);
} else if (prop.method) {
throw this.raise(prop.key.start, Errors.PatternHasMethod);
} else {
super.toAssignableObjectExpressionProp(prop, isLast);
super.toAssignableObjectExpressionProp(prop, ...args);
}
}

Expand Down
10 changes: 6 additions & 4 deletions packages/babel-parser/src/plugins/flow.js
Expand Up @@ -1955,6 +1955,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
// has not been converted yet.
((node.params: any): N.Expression[]),
node.extra?.trailingComma,
/* isLHS */ false,
);
// Enter scope, as checkParams defines bindings
this.scope.enter(SCOPE_FUNCTION | SCOPE_ARROW);
Expand Down Expand Up @@ -2192,26 +2193,27 @@ export default (superClass: Class<Parser>): Class<Parser> =>
}
}

toAssignable(node: N.Node): N.Node {
toAssignable(node: N.Node, isLHS: boolean = false): N.Node {
if (node.type === "TypeCastExpression") {
return super.toAssignable(this.typeCastToParameter(node));
return super.toAssignable(this.typeCastToParameter(node), isLHS);
} else {
return super.toAssignable(node);
return super.toAssignable(node, isLHS);
}
}

// turn type casts that we found in function parameter head into type annotated params
toAssignableList(
exprList: N.Expression[],
trailingCommaPos?: ?number,
isLHS: boolean,
): $ReadOnlyArray<N.Pattern> {
for (let i = 0; i < exprList.length; i++) {
const expr = exprList[i];
if (expr?.type === "TypeCastExpression") {
exprList[i] = this.typeCastToParameter(expr);
}
}
return super.toAssignableList(exprList, trailingCommaPos);
return super.toAssignableList(exprList, trailingCommaPos, isLHS);
}

// this is a list of nodes, from something like a call expression, we need to filter the
Expand Down
10 changes: 5 additions & 5 deletions packages/babel-parser/src/plugins/typescript/index.js
Expand Up @@ -2596,19 +2596,19 @@ export default (superClass: Class<Parser>): Class<Parser> =>
return param;
}

toAssignable(node: N.Node): N.Node {
toAssignable(node: N.Node, isLHS: boolean = false): N.Node {
switch (node.type) {
case "TSTypeCastExpression":
return super.toAssignable(this.typeCastToParameter(node));
return super.toAssignable(this.typeCastToParameter(node), isLHS);
case "TSParameterProperty":
return super.toAssignable(node);
return super.toAssignable(node, isLHS);
case "TSAsExpression":
case "TSNonNullExpression":
case "TSTypeAssertion":
node.expression = this.toAssignable(node.expression);
node.expression = this.toAssignable(node.expression, isLHS);
return node;
default:
return super.toAssignable(node);
return super.toAssignable(node, isLHS);
}
}

Expand Down
36 changes: 36 additions & 0 deletions packages/babel-parser/src/util/expression-scope.js
Expand Up @@ -20,6 +20,7 @@ some expression scopes and thrown later when we know what the ambigous pattern i
- AwaitBindingIdentifier
- AwaitExpressionFormalParameter
- YieldInParameter
- InvalidParenthesizedAssignment when parenthesized is an identifier

There are four different expression scope
- Expression
Expand Down Expand Up @@ -130,6 +131,41 @@ export default class ExpressionScopeHandler {
/* eslint-disable @babel/development-internal/dry-error-messages */
this.raise(pos, message);
}

/**
* Record parenthesized identifier errors
*
* A parenthesized identifier in LHS can be ambiguous because the assignment
* can be transformed to an assignable later, but not vice versa:
* For example, in `([(a) = []] = []) => {}`, we think `(a) = []` is an LHS in `[(a) = []]`,
* an LHS within `[(a) = []] = []`. However the LHS chain is then transformed by toAssignable,
* and we should throw assignment `(a)`, which is only valid in LHS. Hence we record the
* location of parenthesized `(a)` to current scope if it is one of MaybeArrowParameterDeclaration
* and MaybeAsyncArrowParameterDeclaration
*
* Unlike `recordParameterInitializerError`, we don't record to ancestry scope because we
* validate arrow head parsing scope before exit, and then the LHS will be unambiguous:
* For example, in `( x = ( [(a) = []] = [] ) ) => {}`, we should not record `(a)` in `( x = ... ) =>`
* arrow scope because when we finish parsing `( [(a) = []] = [] )`, it is an umbiguous assignment
JLHwung marked this conversation as resolved.
Show resolved Hide resolved
* expression and can not be cast to pattern
* @param {number} pos
* @param {string} message
* @returns {void}
* @memberof ExpressionScopeHandler
*/
recordParenthesizedIdentifierError(pos: number, message: string): void {
const { stack } = this;
const scope: ExpressionScope = stack[stack.length - 1];
if (scope.isCertainlyParameterDeclaration()) {
this.raise(pos, message);
} else if (scope.canBeArrowParameterDeclaration()) {
/*:: invariant(scope instanceof ArrowHeadParsingScope) */
scope.recordDeclarationError(pos, message);
} else {
return;
}
}

/**
* Record likely async arrow parameter errors
*
Expand Down

This file was deleted.

This file was deleted.

This file was deleted.