NotificationsYou must be signed in to change notification settings
Fork5.7k
Star43.7k

Fix parsing ts type casts and nested patterns in destructuring#14500

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

nicolo-ribaudo merged 14 commits intobabel:mainfromnicolo-ribaudo:ts-ast-as-in-destructuring

May 1, 2022

Merged

Fix parsing ts type casts and nested patterns in destructuring#14500

nicolo-ribaudo merged 14 commits intobabel:mainfromnicolo-ribaudo:ts-ast-as-in-destructuring

May 1, 2022

Conversation

Copy link

Member

nicolo-ribaudo commentedApr 27, 2022•
edited
Loading

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 added PR: Bug Fix 🐛

A type of pull request used for our changelog categories

pkg: parser area: typescript labels

Apr 27, 2022

nicolo-ribaudo commented

Apr 27, 2022

View reviewed changes

packages/babel-parser/src/parser/lval.js

		@@ -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

MemberAuthor

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 disallowa as b = c, however we must now make sure tonot disallow[a as b] = c.

Copy link

Contributor

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(aasbof[]);

It is disallowed in 7.16.2 but tsc allows it, and it seems this PR also fixes this one, too.

...ges/babel-parser/test/fixtures/typescript/cast/unparenthesized-assert-and-assign/output.json

		@@ -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

MemberAuthor

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.

Copy link

Collaborator

babel-bot commentedApr 27, 2022•
edited
Loading

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

JLHwung previously approved these changes

Apr 28, 2022

View reviewed changes

packages/babel-parser/src/parser/lval.js

		@@ -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

JLHwungApr 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(aasbof[]);

It is disallowed in 7.16.2 but tsc allows it, and it seems this PR also fixes this one, too.

existentialism previously approved these changes

Apr 28, 2022

View reviewed changes

thorn0 mentioned this pull request

Apr 28, 2022

[Bug]: Weird AST foras inArrayPattern#14498

Closed

1 task

nicolo-ribaudo dismissed stale reviews fromexistentialism andJLHwung

April 28, 2022 14:12

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

nicolo-ribaudo requested review fromexistentialism andJLHwung

April 28, 2022 21:41

Copy link

MemberAuthor

nicolo-ribaudo commentedApr 28, 2022•
edited
Loading

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

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 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

Copy link

Contributor

thorn0 commentedApr 28, 2022

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

Copy link

Contributor

thorn0 commentedApr 28, 2022•
edited
Loading

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

Copy link

MemberAuthor

nicolo-ribaudo commentedApr 29, 2022

Thanks, fixed!

JLHwung approved these changes

Apr 29, 2022

View reviewed changes

packages/babel-parser/src/parser/lval.js

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

		case "SpreadElement": {

Copy link

Contributor

JLHwungApr 29, 2022

Choose a reason for hiding this comment

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

We can handleSpreadElement intoAssignableObjectExpressionProp andtoAssignableList, so we don't need to trackisInObjectPattern anymore.

Copy link

MemberAuthor

nicolo-ribaudoApr 29, 2022

Choose a reason for hiding this comment

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

Done!69719e4

nicolo-ribaudo commented

Apr 29, 2022

View reviewed changes

...-parser/test/fixtures/es2018/object-rest-spread/expression-rest-not-last-invalid/output.json

		@@ -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

MemberAuthor

nicolo-ribaudoApr 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 force-pushed thets-ast-as-in-destructuring branch from4cbc10d to69719e4Compare

April 29, 2022 23:17

JLHwung approved these changes

Apr 30, 2022

View reviewed changes

existentialism approved these changes

Apr 30, 2022

View reviewed changes

nicolo-ribaudo added7 commits

May 1, 2022 13:02

[ts] Fix AST for type assertions in destructuring

e7747b3

Add for-of test

408db9b

Always returnvoid fromtoAssignable

35e9ee9

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 functionalways mutates its argument.

Fix handling of nested type casts

1de2335

Update comment

bd5e86d

Add test forbabel#14504

f4f12aa

Fix flow

1dd0481

nicolo-ribaudo added7 commits

May 1, 2022 13:02

Disallow{...{}} also with flow/ts plugins

66f28e8

Support in@babel/types

f3402e9

Update generated files

bea42da

RemoveisInObjectPattern param fromtoAssignable

0a2d3a3

Fix ts error

cdbd0a3

Fix typo

d86a2b5

Fix Babel 8

8be7c27

nicolo-ribaudo force-pushed thets-ast-as-in-destructuring branch from72cc585 to8be7c27Compare

May 1, 2022 11:02

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 merged commit0e6900d intobabel:main

May 1, 2022

nicolo-ribaudo deleted the ts-ast-as-in-destructuring branch

May 1, 2022 12:39

JLHwung reviewed

May 2, 2022

View reviewed changes

packages/babel-types/src/definitions/core.tsShow resolvedHide resolved