-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Changes from all commits
b14317c
fb8883f
ff6740b
8e809d9
8a8e3f5
00a776c
e9c2070
64b9236
46de321
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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; | ||
|
@@ -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, | ||
|
@@ -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(); | ||
} | ||
|
||
this.state.inFSharpPipelineDirectBody = oldInFSharpPipelineDirectBody; | ||
|
||
return elts; | ||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can now recover from |
||
this.unexpected(param.extra.parenStart); | ||
} | ||
} | ||
|
||
this.parseArrowExpression(arrowNode, exprList, false); | ||
return arrowNode; | ||
} | ||
|
@@ -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( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
// @flow | ||
|
||
/*:: declare var invariant; */ | ||
import * as charCodes from "charcodes"; | ||
import { types as tt, type TokenType } from "../tokenizer/types"; | ||
import type { | ||
|
@@ -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; | ||
|
@@ -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 | ||
*/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
} | ||
|
@@ -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 && | ||
|
@@ -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": | ||
|
@@ -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: | ||
|
@@ -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" | ||
|
@@ -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); | ||
} | ||
} | ||
|
||
|
@@ -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) { | ||
|
@@ -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" && | ||
|
@@ -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); | ||
} | ||
|
This file was deleted.
This file was deleted.
This file was deleted.
There was a problem hiding this comment.
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
we can now recover and raise this error in
toAssignable
, which I think is more helpful