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 parsing ts type casts and nested patterns in destructuring #14500

Merged
merged 14 commits into from May 1, 2022

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Apr 27, 2022

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

@nicolo-ribaudo nicolo-ribaudo added PR: Bug Fix 🐛 pkg: parser area: typescript labels Apr 27, 2022
@@ -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",
Copy link
Member Author

@nicolo-ribaudo nicolo-ribaudo Apr 27, 2022

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.

Copy link
Contributor

@JLHwung JLHwung Apr 28, 2022

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)"
Copy link
Member Author

@nicolo-ribaudo nicolo-ribaudo Apr 27, 2022

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.

@babel-bot
Copy link
Collaborator

@babel-bot babel-bot commented Apr 27, 2022

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51815/

JLHwung
JLHwung previously approved these changes Apr 28, 2022
@@ -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",
Copy link
Contributor

@JLHwung JLHwung Apr 28, 2022

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.

existentialism
existentialism previously approved these changes Apr 28, 2022
@nicolo-ribaudo nicolo-ribaudo dismissed stale reviews from existentialism and JLHwung Apr 28, 2022

I'm going to push a few more commits for the new comments in #14498

@nicolo-ribaudo
Copy link
Member Author

@nicolo-ribaudo nicolo-ribaudo commented Apr 28, 2022

@thorn0 I would appreciate if you could check if there is any missing new test!

@nicolo-ribaudo nicolo-ribaudo changed the title [ts] Fix AST for type assertions in destructuring Fix parsing for type assertions and nested patterns in destructuring Apr 28, 2022
@nicolo-ribaudo nicolo-ribaudo changed the title Fix parsing for type assertions and nested patterns in destructuring Fix parsing of ts type casts and nested patterns in destruct Apr 28, 2022
@thorn0
Copy link
Contributor

@thorn0 thorn0 commented Apr 28, 2022

({ ...[] }=x); and ({ ...{} }=x); should be errors, but they're parsed successfully if the typescript plugin is enabled. REPL

@thorn0
Copy link
Contributor

@thorn0 thorn0 commented Apr 28, 2022

(a as T)! = b: TS, Babel (this PR)
Same with a!! = b.

@nicolo-ribaudo
Copy link
Member Author

@nicolo-ribaudo nicolo-ribaudo commented Apr 29, 2022

Thanks, fixed!

@@ -162,11 +171,10 @@ export default class LValParser extends NodeUtils {
}

case "SpreadElement": {
Copy link
Contributor

@JLHwung JLHwung Apr 29, 2022

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.

Copy link
Member Author

@nicolo-ribaudo nicolo-ribaudo Apr 29, 2022

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",
Copy link
Member Author

@nicolo-ribaudo nicolo-ribaudo Apr 29, 2022

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 } = {}).

@nicolo-ribaudo nicolo-ribaudo force-pushed the ts-ast-as-in-destructuring branch from 4cbc10d to 69719e4 Compare Apr 29, 2022
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.
@nicolo-ribaudo nicolo-ribaudo force-pushed the ts-ast-as-in-destructuring branch from 72cc585 to 8be7c27 Compare May 1, 2022
@nicolo-ribaudo nicolo-ribaudo changed the title Fix parsing of ts type casts and nested patterns in destruct Fix parsing ts type casts and nested patterns in destructuring May 1, 2022
@nicolo-ribaudo nicolo-ribaudo merged commit 0e6900d into babel:main May 1, 2022
36 checks passed
@nicolo-ribaudo nicolo-ribaudo deleted the ts-ast-as-in-destructuring branch May 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: typescript pkg: parser PR: Bug Fix 🐛
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants