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 parsing ts type casts and nested patterns in destructuring #14500
Fix parsing ts type casts and nested patterns in destructuring #14500
Conversation
nicolo-ribaudo
commented
Apr 27, 2022
•
edited
edited
Q | A |
---|---|
Fixed Issues? | Fixes #14498, fixes #14504 |
Patch: Bug Fix? | Yes |
Major: Breaking Change? | |
Minor: New Feature? | |
Tests Added + Pass? | Yes |
Documentation PR Link | |
Any Dependency Changes? | |
License | MIT |
@@ -625,7 +631,8 @@ export default class LValParser extends NodeUtils { | |||
|
|||
const validity = this.isValidLVal( | |||
expression.type, | |||
hasParenthesizedAncestor || expression.extra?.parenthesized, | |||
!(hasParenthesizedAncestor || expression.extra?.parenthesized) && | |||
ancestor.type === "AssignmentExpression", |
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.
This code was needed to disallow a as b = c
, however we must now make sure to not disallow [a as b] = c
.
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.
Can you add a new test case?
for (a as b of []);
It is disallowed in 7.16.2 but tsc allows it, and it seems this PR also fixes this one, too.
@@ -3,8 +3,7 @@ | |||
"start":0,"end":46,"loc":{"start":{"line":1,"column":0,"index":0},"end":{"line":2,"column":29,"index":46}}, | |||
"errors": [ | |||
"SyntaxError: Invalid left-hand side in assignment expression. (1:0)", | |||
"SyntaxError: Invalid left-hand side in assignment expression. (2:6)", | |||
"SyntaxError: Invalid left-hand side in object destructuring pattern. (2:6)" | |||
"SyntaxError: Invalid left-hand side in assignment expression. (2:6)" |
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.
These two errors were at the same location.
It would be better to only keep the "object destructuring pattern" rather than "assignment expression", but it's quite complex because it requires delaying errors until we know what the outer object is (destructuring or object). I might improve it in a follow-up PR.
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51815/ |
@@ -625,7 +631,8 @@ export default class LValParser extends NodeUtils { | |||
|
|||
const validity = this.isValidLVal( | |||
expression.type, | |||
hasParenthesizedAncestor || expression.extra?.parenthesized, | |||
!(hasParenthesizedAncestor || expression.extra?.parenthesized) && | |||
ancestor.type === "AssignmentExpression", |
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.
Can you add a new test case?
for (a as b of []);
It is disallowed in 7.16.2 but tsc allows it, and it seems this PR also fixes this one, too.
I'm going to push a few more commits for the new comments in #14498
@thorn0 I would appreciate if you could check if there is any missing new test! |
|
|
Thanks, fixed! |
@@ -162,11 +171,10 @@ export default class LValParser extends NodeUtils { | |||
} | |||
|
|||
case "SpreadElement": { |
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.
We can handle SpreadElement
in toAssignableObjectExpressionProp
and toAssignableList
, so we don't need to track isInObjectPattern
anymore.
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.
Done! 69719e4
@@ -23,7 +22,7 @@ | |||
"start":1,"end":13,"loc":{"start":{"line":1,"column":1,"index":1},"end":{"line":1,"column":13,"index":13}}, | |||
"properties": [ | |||
{ | |||
"type": "SpreadElement", | |||
"type": "RestElement", |
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.
This is a slight improvement, since it gives a better shape to the recovered AST (the input code is ({ ...rest, x } = {})
.
4cbc10d
to
69719e4
Compare
The function sometimes mutated its argument, sometimes it returned a new node. However, almost all usages didn't handle the "new node" case: this commit removes it, and the function always mutates its argument.
72cc585
to
8be7c27
Compare