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

[ts] Check if param is assignable when parsing arrow return type #13581

Merged
merged 3 commits into from Jul 26, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 3 additions & 2 deletions packages/babel-parser/src/parser/expression.js
Expand Up @@ -1503,7 +1503,7 @@ export default class ExpressionParser extends LValParser {
let arrowNode = this.startNodeAt(startPos, startLoc);
if (
canBeArrow &&
this.shouldParseArrow() &&
this.shouldParseArrow(exprList) &&
(arrowNode = this.parseArrow(arrowNode))
) {
this.expressionScope.validateAsPattern();
Expand Down Expand Up @@ -1544,7 +1544,8 @@ export default class ExpressionParser extends LValParser {
return parenExpression;
}

shouldParseArrow(): boolean {
// eslint-disable-next-line no-unused-vars -- `params` is used in typescript plugin
shouldParseArrow(params: Array<N.Node>): boolean {
return !this.canInsertSemicolon();
}

Expand Down
47 changes: 46 additions & 1 deletion packages/babel-parser/src/parser/lval.js
Expand Up @@ -60,7 +60,7 @@ export default class LValParser extends NodeUtils {
- RestElement is not the last element
- Missing `=` in assignment pattern

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

Choose a reason for hiding this comment

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

This note can be removed then since it is moved to base parser.

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually like this comment: even if they are in the same file, it's still easy to miss isAssignable when updating toAssignable.


* @param {Node} node The expression atom
Expand Down Expand Up @@ -100,6 +100,7 @@ export default class LValParser extends NodeUtils {
case "ObjectPattern":
case "ArrayPattern":
case "AssignmentPattern":
case "RestElement":
break;

case "ObjectExpression":
Expand Down Expand Up @@ -229,6 +230,50 @@ export default class LValParser extends NodeUtils {
return exprList;
}

isAssignable(node: Node, isBinding?: boolean): boolean {
switch (node.type) {
case "Identifier":
case "ObjectPattern":
case "ArrayPattern":
case "AssignmentPattern":
case "RestElement":
Copy link

Choose a reason for hiding this comment

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

@nicolo-ribaudo What about a lookup table? This switch is slow because of 1) string comparison 2) many cases 3) hard to optimize in v8

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this switch relies on string comparisons: these strings are always constants in our code so V8 will atomize them and just compare them as pointers (but @JLHwung knows more than me about this)

Copy link

Choose a reason for hiding this comment

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

v8 will convert this switch to a lookup, so it's faster if you do it.

return true;

case "ObjectExpression": {
const last = node.properties.length - 1;
return node.properties.every((prop, i) => {
return (
prop.type !== "ObjectMethod" &&
(i === last || prop.type !== "SpreadElement") &&
Copy link
Member Author

Choose a reason for hiding this comment

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

The bug that caused the regression was that this was using === "SpreadElement" instead of !==.

This comment was marked as resolved.

Copy link

Choose a reason for hiding this comment

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

Why use 'every' ? It's slow vs a native for-loop

this.isAssignable(prop)
);
});
}

case "ObjectProperty":
return this.isAssignable(node.value);

case "SpreadElement":
return this.isAssignable(node.argument);

case "ArrayExpression":
return node.elements.every(element => this.isAssignable(element));

case "AssignmentExpression":
return node.operator === "=";

case "ParenthesizedExpression":
return this.isAssignable(node.expression);

case "MemberExpression":
case "OptionalMemberExpression":
return !isBinding;

default:
return false;
}
}

// Convert list of expression atoms to a list of

toReferencedList(
Expand Down
7 changes: 7 additions & 0 deletions packages/babel-parser/src/plugins/estree.js
Expand Up @@ -330,6 +330,13 @@ export default (superClass: Class<Parser>): Class<Parser> =>
return (node: any);
}

isAssignable(node: N.Node, isBinding?: boolean): boolean {
if (node != null && this.isObjectProperty(node)) {
return this.isAssignable(node.value, isBinding);
}
return super.isAssignable(node, isBinding);
}

toAssignable(node: N.Node, isLHS: boolean = false): N.Node {
if (node != null && this.isObjectProperty(node)) {
this.toAssignable(node.value, isLHS);
Expand Down
48 changes: 6 additions & 42 deletions packages/babel-parser/src/plugins/flow/index.js
Expand Up @@ -2257,46 +2257,10 @@ export default (superClass: Class<Parser>): Class<Parser> =>
}

isAssignable(node: N.Node, isBinding?: boolean): boolean {
switch (node.type) {
case "Identifier":
case "ObjectPattern":
case "ArrayPattern":
case "AssignmentPattern":
return true;

case "ObjectExpression": {
const last = node.properties.length - 1;
return node.properties.every((prop, i) => {
return (
prop.type !== "ObjectMethod" &&
(i === last || prop.type === "SpreadElement") &&
this.isAssignable(prop)
);
});
}

case "ObjectProperty":
return this.isAssignable(node.value);

case "SpreadElement":
return this.isAssignable(node.argument);

case "ArrayExpression":
return node.elements.every(element => this.isAssignable(element));

case "AssignmentExpression":
return node.operator === "=";

case "ParenthesizedExpression":
case "TypeCastExpression":
return this.isAssignable(node.expression);

case "MemberExpression":
case "OptionalMemberExpression":
return !isBinding;

default:
return false;
if (node.type === "TypeCastExpression") {
return this.isAssignable(node.expression, isBinding);
} else {
return super.isAssignable(node, isBinding);
}
}

Expand Down Expand Up @@ -2989,8 +2953,8 @@ export default (superClass: Class<Parser>): Class<Parser> =>
return super.parseArrow(node);
}

shouldParseArrow(): boolean {
return this.match(tt.colon) || super.shouldParseArrow();
shouldParseArrow(params: Array<N.Node>): boolean {
return this.match(tt.colon) || super.shouldParseArrow(params);
}

setArrowFunctionParameters(
Expand Down
18 changes: 16 additions & 2 deletions packages/babel-parser/src/plugins/typescript/index.js
Expand Up @@ -2877,6 +2877,17 @@ export default (superClass: Class<Parser>): Class<Parser> =>
return param;
}

isAssignable(node: N.Node, isBinding?: boolean): boolean {
switch (node.type) {
case "TSTypeCastExpression":
return this.isAssignable(node.expression, isBinding);
case "TSParameterProperty":
return true;
default:
return super.isAssignable(node, isBinding);
}
}

toAssignable(node: N.Node, isLHS: boolean = false): N.Node {
switch (node.type) {
case "TSTypeCastExpression":
Expand Down Expand Up @@ -3071,8 +3082,11 @@ export default (superClass: Class<Parser>): Class<Parser> =>
return node.expression;
}

shouldParseArrow() {
return this.match(tt.colon) || super.shouldParseArrow();
shouldParseArrow(params: Array<N.Node>) {
if (this.match(tt.colon)) {
return params.every(expr => this.isAssignable(expr, true));
}
return super.shouldParseArrow(params);
}

shouldParseAsyncArrow(): boolean {
Expand Down
@@ -0,0 +1,2 @@
// https://github.com/babel/babel/issues/11038
0 ? v => (sum += v) : v => 0;
@@ -0,0 +1,100 @@
{
"type": "File",
"start":0,"end":76,"loc":{"start":{"line":1,"column":0},"end":{"line":2,"column":29}},
"program": {
"type": "Program",
"start":0,"end":76,"loc":{"start":{"line":1,"column":0},"end":{"line":2,"column":29}},
"sourceType": "module",
"interpreter": null,
"body": [
{
"type": "ExpressionStatement",
"start":47,"end":76,"loc":{"start":{"line":2,"column":0},"end":{"line":2,"column":29}},
"leadingComments": [
{
"type": "CommentLine",
"value": " https://github.com/babel/babel/issues/11038",
"start":0,"end":46,"loc":{"start":{"line":1,"column":0},"end":{"line":1,"column":46}}
}
],
"expression": {
"type": "ConditionalExpression",
"start":47,"end":75,"loc":{"start":{"line":2,"column":0},"end":{"line":2,"column":28}},
"test": {
"type": "NumericLiteral",
"start":47,"end":48,"loc":{"start":{"line":2,"column":0},"end":{"line":2,"column":1}},
"extra": {
"rawValue": 0,
"raw": "0"
},
"value": 0
},
"consequent": {
"type": "ArrowFunctionExpression",
"start":51,"end":66,"loc":{"start":{"line":2,"column":4},"end":{"line":2,"column":19}},
"id": null,
"generator": false,
"async": false,
"params": [
{
"type": "Identifier",
"start":51,"end":52,"loc":{"start":{"line":2,"column":4},"end":{"line":2,"column":5},"identifierName":"v"},
"name": "v"
}
],
"body": {
"type": "AssignmentExpression",
"start":57,"end":65,"loc":{"start":{"line":2,"column":10},"end":{"line":2,"column":18}},
"extra": {
"parenthesized": true,
"parenStart": 56
},
"operator": "+=",
"left": {
"type": "Identifier",
"start":57,"end":60,"loc":{"start":{"line":2,"column":10},"end":{"line":2,"column":13},"identifierName":"sum"},
"name": "sum"
},
"right": {
"type": "Identifier",
"start":64,"end":65,"loc":{"start":{"line":2,"column":17},"end":{"line":2,"column":18},"identifierName":"v"},
"name": "v"
}
}
},
"alternate": {
"type": "ArrowFunctionExpression",
"start":69,"end":75,"loc":{"start":{"line":2,"column":22},"end":{"line":2,"column":28}},
"id": null,
"generator": false,
"async": false,
"params": [
{
"type": "Identifier",
"start":69,"end":70,"loc":{"start":{"line":2,"column":22},"end":{"line":2,"column":23},"identifierName":"v"},
"name": "v"
}
],
"body": {
"type": "NumericLiteral",
"start":74,"end":75,"loc":{"start":{"line":2,"column":27},"end":{"line":2,"column":28}},
"extra": {
"rawValue": 0,
"raw": "0"
},
"value": 0
}
}
}
}
],
"directives": []
},
"comments": [
{
"type": "CommentLine",
"value": " https://github.com/babel/babel/issues/11038",
"start":0,"end":46,"loc":{"start":{"line":1,"column":0},"end":{"line":1,"column":46}}
}
]
}
@@ -0,0 +1 @@
0 ? v => (sum = v) : v => 0;
@@ -0,0 +1,3 @@
{
"throws": "Unexpected token, expected \":\" (1:27)"
}
@@ -0,0 +1 @@
0 ? v => (sum = v) : v => 0 : v => 0;