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

Destructuring: Fix array unpacking assignments with holes on RHS #9412

Merged

Conversation

motiz88
Copy link
Contributor

@motiz88 motiz88 commented Jan 26, 2019

Q                       A
Fixed Issues? described in PR
Patch: Bug Fix? 👍
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

This fixes an issue where destructuring assignments eligible for the "array unpacking" optimization would fail to compile when the array literal on the right-hand side of the expression contained holes.

Example input:

[a, b] = [, 2];
; // Avoid completion record special case

The error message was Property right of AssignmentExpression expected node to be of a type ["Expression"] but instead got null.

Now the above code compiles to:

a = void 0;
b = 2;
;

This PR also adds a couple of related test cases that were missing, to ensure the change doesn't regress them:

  • Normal assignment expression with unpacking
  • Declaration with unpacking and a hole on RHS

@motiz88 motiz88 force-pushed the fix-destructuring-assign-unpack-holes branch from 0ecb9ca to 741b722 Compare January 26, 2019 16:31
@babel-bot
Copy link
Collaborator

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

1 similar comment
@babel-bot
Copy link
Collaborator

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

@existentialism existentialism added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Jan 26, 2019
var [a, b] = [, 2];
[a, b] = [1, 2];
[a, b] = [, 2];
; // Avoid completion record special case
Copy link
Member

Choose a reason for hiding this comment

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

Not in this PR, but this optimization is wrong: ; keeps the previous completion value:

eval("0;;;;")

Can you use something like 0; instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I've just realised this on another PR. Ultimately it's a false negative in the isCompletionRecord check, which could be quite expensive to correct without memoising somehow.

I'll change this, but FWIW there are several other tests that currently use ; in this way and would need to change similarly.

Copy link
Member

Choose a reason for hiding this comment

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

This can/should be handled in another PR

@nicolo-ribaudo nicolo-ribaudo merged commit f7bfc77 into babel:master Mar 27, 2019
@nicolo-ribaudo
Copy link
Member

Thank you!

NMinhNguyen pushed a commit to NMinhNguyen/babel-plugin-transform-destructuring that referenced this pull request Aug 9, 2019
…el#9412)

This fixes an issue where destructuring assignments eligible for the "array unpacking" optimization would fail to compile when the array literal on the right-hand side of the expression contained holes.

Example input:
```js
[a, b] = [, 2];
; // Avoid completion record special case
```

The error message was `Property right of AssignmentExpression expected node to be of a type ["Expression"] but instead got null`.

Now the above code compiles to:
```js
a = void 0;
b = 2;
;
```

This PR also adds a couple of related test cases that were missing, to ensure the change doesn't regress them:
* Normal assignment expression with unpacking
* Declaration with unpacking and a hole on RHS
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants