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

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

Merged
merged 2 commits into from Feb 26, 2019

Conversation

Projects
None yet
4 participants
@nicolo-ribaudo
Copy link
Member

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

This comment has been minimized.

Copy link
Collaborator

commented Feb 11, 2019

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

const pattern = t.objectPattern([property]);

return t.isReferenced(node, property, pattern) ? 1 : 0;
})();

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Feb 11, 2019

Author Member

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.

Foo
} = ({}, function () {
throw new Error('"' + "Foo" + '" is read-only.');
}()));

This comment has been minimized.

Copy link
@nicolo-ribaudo
@danez

danez approved these changes Feb 18, 2019

Copy link
Member

left a comment

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

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2019

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.

@nicolo-ribaudo nicolo-ribaudo merged commit 5c8cc0d into babel:master Feb 26, 2019

4 checks passed

babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 86.97% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nicolo-ribaudo nicolo-ribaudo deleted the nicolo-ribaudo:obj-destr-binding branch Feb 26, 2019

@danez

This comment has been minimized.

Copy link
Member

commented May 16, 2019

@nicolo-ribaudo this fixes #4694 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.