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

Make /*#__PURE__*/ not only for call, but also for callable value? #2960

Closed
LongTengDao opened this issue Jun 22, 2019 · 11 comments · Fixed by #5024
Closed

Make /*#__PURE__*/ not only for call, but also for callable value? #2960

LongTengDao opened this issue Jun 22, 2019 · 11 comments · Fixed by #5024

Comments

@LongTengDao
Copy link
Contributor

LongTengDao commented Jun 22, 2019

Feature Use Case

For treeshaking purpose especially for pollyfill, these workarounds are troublesome:

// lib
var create = Object.create || function polyfill () { /* ... */ };

// main
export var a = /*#__PURE__*/ create(null);
export var b = /*#__PURE__*/ create(null);
export var c = /*#__PURE__*/ create(null);
// lib
var create = Object.create || function polyfill () { /* ... */ };
function __PURE__create  (o, p) { return /*#__PURE__*/ create(o, p); };

// main
export var a = $__PURE__create(null);
export var b = $__PURE__create(null);
export var c = $__PURE__create(null);

Feature Proposal

Allow to mark functions as PURE to call like native ones:

// lib
var create = Object.create || /*#__PURE__*/ function polyfill () { /* ... */ };

// main
export var a = create(null);
export var b = create(null);
export var c = create(null);
@LongTengDao LongTengDao changed the title Make /*#__PURE__*/ not only for call, but also for value and set? Make /*#__PURE__*/ not only for call, but also for callable value? Jun 23, 2019
@lukastaegert
Copy link
Member

The intention at the moment is to match what UglifyJS does as closely as possible, as it is basically an Uglify syntax we are using here. So if we extend semantics from our side, the annotations will only work for Rollup builds but not be respected by any other bundler or minifier. That being said, it surely is possible.

@kzc
Copy link
Contributor

kzc commented Jun 23, 2019

I wouldn't recommend this. The uglify pure call semantics are well defined, easily understood and has been stable for years. Such a proposed extension by rollup would not only be not respected by uglify and its derivatives, it might cause them to generate incorrect code due to their aggressive inlining of functions and variables with unknown effects for shifted accompanying comments.

If you wish to pursue this path, I'd suggest to come up with a differently named comment annotation and lobby the other projects to support it. There was a related proposal to come up with yet another annotation to mark code as containing side effects and keep it in its original form. There's an opportunity to unify these related proposals with a new mechanism.

@LongTengDao
Copy link
Contributor Author

@fabiosantoscode Hi, how do you feel this?

@fabiosantoscode
Copy link

I've given thought to this before, and I think it is a pretty good idea. I also want to implement something that prevents inlining and it wouldn't make sense to reproduce this on every call. So what makes the most sense to me is to add it to functions and also declarations of function values.

@LongTengDao
Copy link
Contributor Author

If rollup and terser (the es6 support version of uglify) both support that, things will work perfectly. @lukastaegert

@LongTengDao
Copy link
Contributor Author

LongTengDao commented Aug 31, 2019

If we can't use PURE, is /*#__Pure__*/ ok?

Differentiate from PURE, and not too different, and it means "create a new PURE function".

@kzc

@kzc
Copy link
Contributor

kzc commented Aug 31, 2019

Annotating functions would not work in all cases because of the dynamic nature of ECMAScript. As soon as a function is passed through a variable, call argument or complex expression its pure provenance would be lost.

I added pure annotation support for function calls in uglify primarily to solve the "IIFE with no logical side effects" problem. It works well for that purpose. Altering its behavior will create unknown effects on code in the wild. So I am not in favor of extending its behavior - particularly when major tools still do not handle basic pure annotation functionality correctly with default settings.

@lukastaegert
Copy link
Member

I see the same issues, though I am not sure if it will cause problems. What WILL happen is that different tools with different quality of reference resolution will produce different results, and it will open doors for a lot of edge case scenarios where a wrong reference resolution will mark something as pure that is not pure. Rollup still has some of them open...

@kzc
Copy link
Contributor

kzc commented Sep 1, 2019

I see the same issues, though I am not sure if it will cause problems.

@lukastaegert It's not hypothetical - it's a genuine bug in babel + regenerator. This is explained in detail here: babel/babel#10306

@LongTengDao
Copy link
Contributor Author

LongTengDao commented Sep 1, 2019

I see. This feature maybe useful, but we couldn't avoid to produce bugs incidentally, which will exist for some time.

It seems that only when babel webpack/rollup uglify/terser implement it together, a new syntax will be given birth safely.

@fabiosantoscode
Copy link

Thanks for clearing this up @kzc. It makes sense not to do this. In the end pure annotations are meant more for compilers/macros than they are for humans, so I feel like annotating functions themselves shouldn't be done.

Probably this is off-topic, but noinline should behave in the same way to save some some work for non-terser implementers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants