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

Do not unpack array patterns that update a referenced binding #8535

Merged
merged 1 commit into from Sep 28, 2018

Conversation

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Aug 25, 2018

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

Do not unpack array patterns that update a referenced binding

We used this.scope.traverse to check if the array initializer referenced
a binding which is present in the array pattern. It doesn't work because,
in order to check if an identifier is a reference, we need to check its
parent.
this.scope.binding can't set the correct parentPath, because the only
path that it knows is the root scope's (e.g. the function path).
Use t.traverse which gives the correct parent.

This regression has been brought to the surface by 29d249e because it made the isReferenced check stricter, but it is unrelated.

@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Aug 25, 2018

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

@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:regression-8528 branch 2 times, most recently from 3395ab5 to ce8931e Aug 25, 2018
state.bindings[node.name]
) {
state.deopt = true;
return true;

This comment has been minimized.

Copy link
@loganfsmyth

loganfsmyth Aug 25, 2018

Member

I'm not a huge fan of return true in traversal-like APIs because it's not clear if the behavior is .skip()-like or .stop()-like.

My preference would probably be for it to either:

  • Explicitly return a STOP constant
  • Skip the return value and throw an exception instead, e.g.
const STOP = {};
// ...
throw STOP;

and do

try {
  t.traverseFast(arr, arrayUnpackVisitor, state);
} catch (err) {
  if (err !== STOP) throw err;
}
@nicolo-ribaudo nicolo-ribaudo force-pushed the nicolo-ribaudo:regression-8528 branch from 3ce9686 to ba94037 Aug 30, 2018
@JSteunou

This comment has been minimized.

Copy link

JSteunou commented Sep 25, 2018

Any step missing for this PR to be accepted?

@hzoo hzoo merged commit 626f479 into babel:master Sep 28, 2018
4 checks passed
4 checks passed
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 80.58% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nicolo-ribaudo nicolo-ribaudo deleted the nicolo-ribaudo:regression-8528 branch Sep 28, 2018
NMinhNguyen pushed a commit to NMinhNguyen/babel-plugin-transform-destructuring that referenced this pull request Aug 9, 2019
@lock lock bot added the outdated 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.
Projects
None yet
6 participants
You can’t perform that action at this time.