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

Avoid adding #__PURE__ annotation to .bind(this)() expressions #7043

Merged

Conversation

Jessidhia
Copy link
Member

This improves on the fix done on #6999.

Fixes the #__PURE__ annotation getting added to (async () => {})() IIFEs when the arrow function transform is running with "spec": true.

The arrow function transform will change the function to be wrapped with a call to .bind(this), which throws off the naïve isIIFE check. This adds a fancier isIIFE check that will also catch immediately invoking the result of .bind(this).

Fixes the #__PURE__ annotation getting added to (async () => {})() IIFEs when the arrow function transform is running with spec: true.
@Jessidhia Jessidhia added 7.x: regression PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Dec 18, 2017
@nicolo-ribaudo
Copy link
Member

@Jessidhia
Copy link
Member Author

Jessidhia commented Dec 18, 2017

@nicolo-ribaudo that would, unfortunately, then break the not-IIFE case for spec arrow functions ((async () => {}) would not get annotated because it has a .bind(this) call) 🤔

EDIT: though I guess that could be worked around by specifically whitelisting .bind(this) as pure? Will be incorrect if someone actually writes a .bind(this) call before invoking super(), though.

@babel-bot
Copy link
Collaborator

babel-bot commented Dec 18, 2017

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

@Jessidhia
Copy link
Member Author

@nicolo-ribaudo looks like it's not possible as the annotateAsPure helper can receive Nodes without a NodePath

["transform-arrow-functions", { "spec": true }],
"transform-regenerator",
"transform-async-to-generator",
"external-helpers"
Copy link
Member

Choose a reason for hiding this comment

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

indentation is wrong here

@Andarist
Copy link
Member

We could build version accepting NodePaths exclusively on top of the existing helper.

@Jessidhia
Copy link
Member Author

I am trying a different approach by wrapping the calls on ParenthesizedExpressions.

return _context4.stop();
}
}, _callee4, this);
})).bind(this);
Copy link
Member Author

Choose a reason for hiding this comment

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

The call to .bind(this) makes this not eligible for DCE by Uglify, even with the #__PURE__ annotation.

Maybe changing this to just check for MemberExpression at all is sufficient; or should we stay optimistic and add the #__PURE__ annotation in this case? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Is .bind impure only when the user modifies Function.prototype.bind? If yes, I vote for being optimistic (since Babel has always assumed that builtins are unchanged), otherwise we should remove it.

@Jessidhia
Copy link
Member Author

I'll merge it just so this bug is not on the next beta anymore, and I'm going to see if using sequence expressions in #7044 instead works out better.

@Jessidhia Jessidhia merged commit 0f60d42 into babel:master Dec 28, 2017
@Jessidhia Jessidhia deleted the fix-pure-annotation-on-async-spec-arrow branch December 28, 2017 08:02
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
7.x: 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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants