Skip to content

Mark FOO in "var { x: FOO }˝ as a binding, not as a reference#9492

Merged
nicolo-ribaudo merged 2 commits intobabel:masterfrom
nicolo-ribaudo:obj-destr-binding
Feb 26, 2019
Merged

Mark FOO in "var { x: FOO }˝ as a binding, not as a reference#9492
nicolo-ribaudo merged 2 commits intobabel:masterfrom
nicolo-ribaudo:obj-destr-binding

Conversation

@nicolo-ribaudo
Copy link
Copy Markdown
Member

@nicolo-ribaudo nicolo-ribaudo commented Feb 11, 2019

Q                       A
Patch: Bug Fix? Yes
Major: Breaking Change? Maybe? I'd consider this as a bug fix
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

Before this PR, babel incorrectly marks FOO as a ReferencedIdentifier in var { a: FOO } and as a binding identifier in ({ a: FOO }): this is because we needed to know not only the parent but also its grandparent to know it. Array patterns don't have this problem, since there isn't something like an ArrayElement node between the identifier and the pattern.

This code shows the bug; you can test the old version on ASTExplorer (https://astexplorer.net/#/gist/3dc1c7fd69ee5f7478c2229e4bdce428/latest)

Plugin 
function ({ types: t, template }) {
  let result = "";
  return {
    visitor: {
      Identifier(path) {
        if (path.node.name === "key") return;

        result += path.node.name +
          " - referenced: " + path.isReferencedIdentifier() +
          ", binding: " + path.isBindingIdentifier() + "\n";
      },
      Program: {
        exit() {
          alert(result);
        }
      }
    },
  };
}
Input code 
var a = 1;
var [ b ] = 1;
var { key: c } = 1;

x;
[ y ];
({ key: z });
Old output 
a - referenced: false, binding: true
b - referenced: false, binding: true
c - referenced: true, binding: true
x - referenced: true, binding: false
y - referenced: true, binding: false
z - referenced: true, binding: true
New output 
a - referenced: false, binding: true
b - referenced: false, binding: true
c - referenced: false, binding: true
x - referenced: true, binding: false
y - referenced: true, binding: false
z - referenced: true, binding: false

@nicolo-ribaudo nicolo-ribaudo added PR: Bug Fix 🐛 A type of pull request used for our changelog categories pkg: types labels Feb 11, 2019
@babel-bot
Copy link
Copy Markdown
Collaborator

babel-bot commented Feb 11, 2019 •
edited
Loading

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

nicolo-ribaudo
nicolo-ribaudo commented Feb 11, 2019
View reviewed changes
packages/babel-plugin-proposal-object-rest-spread/src/index.js
const pattern = t.objectPattern([property]);

return t.isReferenced(node, property, pattern) ? 1 : 0;
})();
Copy link
Copy Markdown
Member Author

@nicolo-ribaudo nicolo-ribaudo Feb 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This is why this might be considered a breaking change: we were relying on the wrong behavior.
This code is needed to make new versions of the plugin work with new and old versions of @babel/types.

...ages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/import-const-throw/output.js
Foo
} = ({}, function () {
throw new Error('"' + "Foo" + '" is read-only.');
}()));
Copy link
Copy Markdown
Member Author

@nicolo-ribaudo nicolo-ribaudo Feb 11, 2019

Choose a reason for hiding this comment

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

This change is correct. You can see the input file at https://github.com/babel/babel/blob/93ec6ee62a10ea022c902962dc989f1045fb1637/packages/babel-plugin-transform-modules-commonjs/test/fixtures/misc/import-const-throw/input.mjs

import Foo from "foo";

// ...

({Foo} = {});

@nicolo-ribaudo nicolo-ribaudo mentioned this pull request Feb 11, 2019
Merged
danez
danez approved these changes Feb 18, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@danez danez left a comment

Choose a reason for hiding this comment

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

looks good, I'm not sure what you are saying about the breaking change. Even with the compat code it could be still breaking?

@nicolo-ribaudo
Copy link
Copy Markdown
Member Author

nicolo-ribaudo commented Feb 18, 2019 •
edited
Loading

With the compact mode it shouldn't break, but as we did rely on the wrong behavior someone else could have done the same thing.

@danez danez added the PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release label Feb 26, 2019
@nicolo-ribaudo nicolo-ribaudo merged commit 5c8cc0d into babel:master Feb 26, 2019
@nicolo-ribaudo nicolo-ribaudo deleted the obj-destr-binding branch February 26, 2019 23:17
@kevinji kevinji mentioned this pull request Apr 7, 2019
Merged
@nicolo-ribaudo nicolo-ribaudo mentioned this pull request May 14, 2019
Closed
@danez
Copy link
Copy Markdown
Member

danez commented May 16, 2019 •
edited
Loading

@nicolo-ribaudo this fixes #4694 ?

@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 3, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@existentialism existentialism existentialism approved these changes

+1 more reviewer

@danez danez danez approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: types PR: Bug Fix 🐛 A type of pull request used for our changelog categories PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nicolo-ribaudo @babel-bot @danez @existentialism