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

Define default value for vars shadowing params #11307

Merged
merged 6 commits into from Mar 22, 2020

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Mar 21, 2020

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

The best way to handle params's horrible shadowing behavior is by... relying on it 😁

@nicolo-ribaudo nicolo-ribaudo added PR: Bug Fix 🐛 A type of pull request used for our changelog categories i: regression labels Mar 21, 2020
@@ -1,4 +1,3 @@
import callDelegate from "@babel/helper-call-delegate";
Copy link
Member Author

Choose a reason for hiding this comment

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

This is now unused and can be removed from the repo.

@nicolo-ribaudo nicolo-ribaudo added this to the v7.9.3 milestone Mar 21, 2020
This prevents this plugin from running after the destructuring
transform, which causes babel#11256
@nicolo-ribaudo
Copy link
Member Author

I ended up also pushing a workaround for #11256 (a fix would require major changes to how we handle the scope)

@@ -1,5 +1,5 @@
function foo(a = 2) {
for (var a, i = 0; i < 1; i++);
expect(a).toBe(undefined);
expect(a).toBe(2);
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!


for (let i = 0; i < params.length; i++) {
const param = params[i];
let isSimpleParameterList = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

What about

const isSimpleParameterList = params.every(param => param.isIdentifier());

It will return true when params is empty.

scope: scope,
stop: false,
needsOuterBinding: false,
shadowedParams: new Set(),
Copy link
Contributor

Choose a reason for hiding this comment

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

We can move shadowedParams out of state if we don't intend it accessed by visitIdentifier.

@@ -0,0 +1,6 @@
function test(date, defValue = 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: lol they don't have to be date and defValue.


const iifeVisitor = {
ReferencedIdentifier(path, state) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can simplify to

const iifeVisitor = {
  "ReferencedIdentifier|BindingIdentifier": visitIdentifier,
}

If there is a BindingIdentifier in the function scope, it should not satisfy scope.getBinding(name) === state.scope.parent.getBinding(name) so essentially it will be noop.

Copy link
Member

@existentialism existentialism left a comment

Choose a reason for hiding this comment

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

👍 quiz ftw :)

!isSafeBinding(scope, node) ||
!isSafeBinding(state.scope, node)
name === "eval" ||
(scope.getBinding(name) === state.scope.parent.getBinding(name) &&
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this just a !scope.hasOwnBinding check?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, state.scope does not equal to scope.

… ci]

Co-Authored-By: Brian Ng <bng412@gmail.com>
@nicolo-ribaudo nicolo-ribaudo merged commit ceb54dd into babel:master Mar 22, 2020
@nicolo-ribaudo nicolo-ribaudo deleted the regression-params branch March 22, 2020 09:45
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jun 22, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
i: regression 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
5 participants