Support marking a call as pure #1448

kzc · 2017-01-29T01:58:19Z

A function call or IIFE with an immediately preceding comment containing @__PURE__ or #__PURE__ is deemed to be a side-effect-free pure function call and can potentially be dropped.

Related: microsoft/TypeScript#13721

alexlamsl · 2017-01-29T04:53:44Z

lib/compress.js

@@ -1273,7 +1273,21 @@ merge(Compressor.prototype, {
        def(AST_Constant, return_false);
        def(AST_This, return_false);

+        var pure_regex = /[@#]pure/;


The devil in me thinks https://docs.read.me/notes#pure_in_title could trip on this.

Yeah, there's always a risk of an unintentional comment collision.

I'm going to change it to /[@#]__PURE__/.

Or may be just /^\s*[@#]pure/, so forcing it to be at the start of the comment?

I think the directive should be allowed to appear anywhere in the comment. The directive doesn't have to pretty - it's uglify, after all. ;-)

/[@#]__PURE__/ should lower the odds of accidental comment collision.

A function call or IIFE with an immediately preceding comment containing `@__PURE__` or `#__PURE__` is deemed to be a side-effect-free pure function call and can potentially be dropped.

kzc · 2017-01-29T18:36:50Z

Note: pure function call annotations work well with the new toplevel compress option in PR #1450.

When this PR and PR #1450 are applied then toplevel IIFEs assigned to an otherwise unused var will be dropped:

$ echo 'var t = /*#__PURE__*/function(){ console.log("iife"); }();' | uglifyjs -c
var t=function(){console.log("iife")}();

$ echo 'var t = /*#__PURE__*/function(){ console.log("iife"); }();' | uglifyjs -c toplevel
WARN: Dropping unused variable t [-:1,4]

A function call or IIFE with an immediately preceding comment containing `@__PURE__` or `#__PURE__` is deemed to be a side-effect-free pure function call and can potentially be dropped. closes mishoo#1448

kzc · 2017-02-21T02:47:44Z

In the event that all comments are retained, the pure call hint should be dropped. Otherwise this potential bug could happen if uglify output is run through uglify a second time:

$ echo '/*#__PURE__*/ foo(), bar();' | bin/uglifyjs -c --comments=all
/*#__PURE__*/bar();

$ echo '/*#__PURE__*/ foo(), bar();' | bin/uglifyjs -c --comments=all | bin/uglifyjs -c
WARN: Dropping side-effect-free statement [-:1,13]

A fix will be pushed shortly.

kzc · 2017-02-21T03:19:32Z

@alexlamsl d35b92c passes on all versions of node except for 0.10.x. It's not a timeout. Do you see anything that's suspect?

kzc · 2017-02-21T03:29:05Z

Apparently something about the g flag in the regex that node 0.10.x doesn't like:

var pure_regex = /[@#]__PURE__/g;

kzc · 2017-02-21T03:46:40Z

Using a RegExp literal instead of a reused regex did the trick. Some regexp state was being mutated causing it to fail on node 0.10.x.

alexlamsl · 2017-02-21T04:53:37Z

Yeah, using the regex literals this way is definitely safer. I guess some states that needs resetting between String.replace() calls are not being done correctly on Node.js 0.10.x

alexlamsl · 2017-02-21T05:00:39Z

Hmm. Looking at this, wouldn't it make more sense if the comment is removed if/when the annotated function is being removed? I'm thinking of cases where .has_side_effects() is called for some other optimisation but the pure function isn't being dropped, but then the annotation would be dropped regardless

Having said that, this would only cause sub-optimal output AFAICT. So I guess it's not a major issue.

kzc · 2017-02-21T05:14:59Z

wouldn't it make more sense if the comment is removed if/when the annotated function is being removed?

I believe it's correct because it checks the previously set pure property:

            if (node.pure !== undefined) return node.pure;

which outlives the removed __PURE__ comment hint. The call will be removed eventually by the side_effects option.

alexlamsl · 2017-02-21T05:23:13Z

Not related to this PR, but we have a similar issue with /*@const*/:

$ echo 'function f(){/*@const*/var a=1;var b=2;return a+b;}' | bin/uglifyjs -c --comments=all
WARN: Dropping unused variable a [-:1,27]
function f(){/*@const*/var b=2;return 1+b}

$ echo 'function f(){/*@const*/var a=1;var b=2;return a+b;}' | bin/uglifyjs -c --comments=all | bin/uglifyjs -c
WARN: Dropping unused variable a [-:1,27]
WARN: Dropping unused variable b [-:1,27]
function f(){return 3}

kzc · 2017-02-21T05:25:41Z

Regarding your point about __PURE__, if the side_effects option is disabled, the warning should not be emitted. Will push a fix shortly.

kzc · 2017-02-21T05:26:18Z

Not related to this PR, but we have a similar issue with /*@const*/

I had suspected that as well.

…nabled

kzc · 2017-02-21T05:34:55Z

With latest PR:

$ echo '/*#__PURE__*/ foo(), bar();' | bin/uglifyjs -c side_effects=true --comments=all
WARN: Dropping __PURE__ call [-:1,14]
/* */bar();

$ echo '/*#__PURE__*/ foo(), bar();' | bin/uglifyjs -c side_effects=false --comments=all
/*#__PURE__*/foo(),bar();

alexlamsl · 2017-02-21T05:45:27Z

With the latest commit, collapse_vars won't be able to recognise /*@__PURE__*/ with side_effects disabled, even though all the other side-effects calculation, including pure_funcs, would continue to function.

_{(Stop me if I'm starting to sound like picking bones in a chicken egg 😅 )}

kzc · 2017-02-21T05:55:25Z

collapse_vars doesn't work very well without side_effects enabled anyway. Probably a few other optimizations have the same problem.

But the case you describe still works, as far as I can tell:

$ echo 'function foo(){var b = /*#__PURE__*/bar(); return b;}' | bin/uglifyjs -c side_effects=false,collapse_vars --comments=all
WARN: Replacing variable b [-:1,50]
function foo(){/*#__PURE__*/return bar()}

It doesn't drop the hint, but that's fine. It's a no-op in that context.

alexlamsl · 2017-02-21T06:08:58Z

LGTM - let me fold all of this onto #1485 now.

alexlamsl · 2017-02-21T07:42:50Z

Yes, that's what I designed reduce_vars to do, with the difference being that it would analyse usage to ensure safety instead of requiring explicit marking of /*@const*/.

kzc · 2017-02-21T07:43:39Z

So /*@const*/ is obsolete then.

alexlamsl · 2017-02-21T07:45:37Z

To be clear, I wasn't saying the example above gives the wrong output, just a surprising/unexpected behaviour.

What I'm puzzling over at the moment is given the comment is attached to var a the AST_Node, why dropping that as unused would somehow retain the comment before it.

kzc · 2017-02-21T07:50:44Z

What I'm puzzling over at the moment is given the comment is attached to var a the AST_Node, why dropping that as unused would somehow retain the comment before it.

If I'm not mistaken, the same comment can be attached to more than one node. A child and its parent can share the same comment depending on the statement. Ownership of a comment is a tricky thing and is ambiguous. OutputStream makes sure a given comment is output only once by marking the AST, despite multiple comment owners.

alexlamsl · 2017-02-21T07:58:38Z

You may be right in general, but in this case it's due to:

/*@const*/var a;

var b;

getting transformed into:

/*@const*/var a, b;

before drop_unused() is called. So basically that pragma will pollute other variables as well:

$ echo 'function f(){/*@const*/var a=1;var b=2;b=3;return a+b}' | bin/uglifyjs -c --comments=all
WARN: Dropping unused variable a [-:1,27]
function f(){/*@const*/var b=2;return b=3,1+b}

$ echo 'function f(){/*@const*/var a=1;var b=2;b=3;return a+b}' | bin/uglifyjs -c --comments=all | bin/uglifyjs -c
WARN: Dropping unused variable a [-:1,27]
WARN: Dropping unused variable b [-:1,27]
function f(){return 3,3}

Now this looks like a bug to me.

(TODO: investigate hoist_vars)

kzc · 2017-02-21T08:05:19Z

If you try the dump AST PR #769 and modify this line to remove the exclamation marks:

https://github.com/mishoo/UglifyJS2/pull/769/files#diff-da84828c4bd7834c0040b0bbb51acdecR105

to emit the start and end tokens and execute this command you can see multiple nodes with the same comment "Hello":

$ echo '/*Hello*/foo();' | bin/uglifyjs --dump-ast | egrep 'Hello|_class'
  "_class": "AST_Toplevel",
      "_class": "AST_SimpleStatement",
        "_class": "AST_Call",
          "_class": "AST_Token",
          "_class": "AST_SymbolRef",
            "_class": "AST_Token",
                "_class": "AST_Token",
                "value": "Hello"
            "_class": "AST_Token",
                "_class": "AST_Token",
                "value": "Hello"
          "_class": "AST_Token",
              "_class": "AST_Token",
              "value": "Hello"
        "_class": "AST_Token",
        "_class": "AST_Token",
            "_class": "AST_Token",
            "value": "Hello"
    "_class": "AST_Token",
    "_class": "AST_Token",
        "_class": "AST_Token",
        "value": "Hello"

kzc · 2017-02-21T08:08:43Z

Now this looks like a bug to me

It would only be a bug if the const hint was harmful, which it is not.

That transform happens because several nodes share the same comment. It's not easy to determine which node a given comment ought to belong to - particularly after a transform.

alexlamsl · 2017-02-21T08:12:37Z

But in my example above, var b; would never have inherited /*@const*/ if it didn't get fold into var a;. And there is a b=3; somewhere down the line, so marking b as const will generate incorrect code in this case.

kzc · 2017-02-21T08:16:04Z

And there is a b=3; somewhere down the line, so marking b as const will generate incorrect code in this case.

It's harmless.

$ echo '/*@const*/ var b = 1; b = 2; console.log(b);' | node
2

$ echo '/*@const*/ var b = 1; b = 2; console.log(b);' | uglifyjs -c
var b=1;b=2,console.log(b);

$ echo '/*@const*/ var b = 1; b = 2; console.log(b);' | uglifyjs -c | node
2

It's just a hint, it's still a var.

alexlamsl · 2017-02-21T08:21:05Z

Erm, console.log(b) won't get optimised until #1477 😅

So to modify your example a little:

$ echo '/*@const*/ var b = 1; b = 2; console.log(1+b);' | node
3

$ echo '/*@const*/ var b = 1; b = 2; console.log(1+b);' | uglifyjs -c
var b=1;b=2,console.log(2);

$ echo '/*@const*/ var b = 1; b = 2; console.log(1+b);' | uglifyjs -c | node
2

join_vars is responsible for folding those together:

$ echo 'function f(){/*@const*/var a=1;var b=2;b=3;return a+b}' | bin/uglifyjs -c join_vars=0 --comments=all
WARN: Dropping unused variable a [-:1,27]
function f(){var b=2;return b=3,1+b}

$ echo 'function f(){/*@const*/var a=1;var b=2;b=3;return a+b}' | bin/uglifyjs -c join_vars=0 --comments=all | bin/uglifyjs -c
WARN: Dropping unused variable a [-:1,27]
function f(){var b=2;return b=3,1+b}

kzc · 2017-02-21T08:28:33Z

Fair enough. I see a couple of possible solutions:

have the parser or figure_out_scope raise an error that either const or a pseudo-const var is being reassigned.
or revert the code for @const altogether and suggest that reduce_vars be used instead.

kzc · 2017-02-21T08:29:46Z

@alexlamsl Can we move the /*@const*/ bug discussion to a new Issue? It's unrelated to pure calls.

alexlamsl · 2017-02-21T08:32:37Z

moving @const discussions to #1497

kzc · 2017-03-28T18:08:02Z

Some interest in the use of this feature:

angular/tsickle#452

alexlamsl · 2017-03-28T19:15:07Z

Would be good to know when somebody starts to use this actively in the wild.

IgorMinar · 2017-03-30T06:16:47Z

@alexlamsl I can confirm that this is feature pure gold and allows all classes downleveled by TypeScript to be removed by Uglify. This will have a huge impact for most Angular developers. Thank you for adding this feature.

Now I'm onto figuring out how to roll this out to all TypeScript users. We'll either get our TypeScript friends to implement microsoft/TypeScript#13721 or if there is pushback against that, we'll implement it in our tsickle tool or some kind of webpack plugin within @angular/cli. We'll see. In the meantime I created this super hacky tool. Thanks again.

kzc · 2017-05-01T19:24:24Z

Proof-of-concept Babel plugin to annotate down-levelled classes: babel/babel#5632 (comment)

kzc · 2017-06-29T20:17:22Z

An article explaining how uglify /*@__PURE__*/ annotations were used in library code to reduce the size of user applications:

https://iamakulov.com/notes/polished-webpack/

Andarist · 2017-09-08T14:14:17Z

lib/compress.js

+                && (comments = node.start.comments_before)
+                && comments.length
+                && /[@#]__PURE__/.test((last_comment = comments[comments.length - 1]).value)) {
+                last_comment.value = last_comment.value.replace(/[@#]__PURE__/g, ' ');


@kzc any reason why the #__PURE__ comment has to be the last one? why this isnt checking all node.start.comments_before?

If im gonna write i.e. a babel plugin which annotates calls for me it might interfere with other annotations causing i.e. such situation

var A = /*#__PURE__*/ /* @class */ function() { /* ... */ }();

and thus preventing it from being dropped as unused.

any reason why the #__PURE__ comment has to be the last one? why this isnt checking all node.start.comments_before?

No particular reason other than efficiency. It probably should check all comments. PR is welcome.

Be aware that it's unlikely to be back-ported to uglify-js@2.x which is widely used but whose code base is frozen. So you'd probably want to list #__PURE__ in the last comment to aid legacy uglify users.

No particular reason other than efficiency. It probably should check all comments. PR is welcome.

PR is comming then. :)

Be aware that it's unlikely to be back-ported to uglify-js@2.x which is widely used but whose code base is frozen. So you'd probably want to list #PURE in the last comment to aid legacy uglify users.

While at the moment of adding the comment I can ensure its added as the last comment, I cannot be sure that no other transformation change it. I wouldnt be overly concern about old version of the uglify. Life goes on and library's consumers should upgrade sooner or later :)

I should mention that the following would still work with current uglify versions:

var A = /* @__PURE__ @class */ function() { /* ... */ }();

That's all the proposed babel plugin would have to do to retain backwards compatibility with uglify.

This was referenced Jan 29, 2017

Class declaration in IIFE considered as side effect #1261

Closed

Next release after v2.7.5 #1383

Closed

alexlamsl reviewed Jan 29, 2017

View reviewed changes

Support marking a call as pure

58036cd

A function call or IIFE with an immediately preceding comment containing `@__PURE__` or `#__PURE__` is deemed to be a side-effect-free pure function call and can potentially be dropped.

Pauan mentioned this pull request Feb 2, 2017

Proposal for marking functions as pure rollup/rollup#1293

Closed

rickmed mentioned this pull request Feb 11, 2017

Migrate to ES6 modules ramda/ramda#1968

Closed

jetpacmonkey mentioned this pull request Feb 15, 2017

Tree shaking completely broken? webpack/webpack#2867

Closed

alexlamsl mentioned this pull request Feb 17, 2017

fix pure_funcs & improve side_effects #1494

Closed

alexlamsl mentioned this pull request Feb 18, 2017

2.8.0 staging #1485

Merged

alexlamsl added a commit to alexlamsl/UglifyJS that referenced this pull request Feb 18, 2017

add test for mishoo#1448 & mishoo#1450

391a25a

alexlamsl added a commit to alexlamsl/UglifyJS that referenced this pull request Feb 19, 2017

add test for mishoo#1448 & mishoo#1450

01b8313

remove [#@]__PURE__ hint from comment when pure call dropped

d35b92c

have [@#]__PURE__ regex work with node 0.10.x

883ce81

have #__PURE__ hint only work when compress side_effects option e…

b943c0d

…nabled

alexlamsl mentioned this pull request Feb 21, 2017

@const creeping #1497

Closed

alexlamsl closed this in 1e51586 Feb 23, 2017

alexlamsl mentioned this pull request Feb 26, 2017

Harmony 2.8.0 #1509

Merged

sonicoder86 mentioned this pull request Apr 14, 2017

Pure annotation in downlevel emits babel/babel#5632

Closed

nicolo-ribaudo mentioned this pull request Aug 8, 2017

Remove unused calls to pure functions babel/minify#647

Closed

Andarist reviewed Sep 8, 2017

View reviewed changes

billiegoose mentioned this pull request Dec 1, 2017

Tree shaking class methods rollup/rollup#349

Closed

kzc deleted the pure_call_annotation branch November 21, 2018 16:09

evanw mentioned this pull request Dec 23, 2021

Pure annotation for functions evanw/esbuild#1883

Closed

aleclarson mentioned this pull request Jun 4, 2022

Extending the pure annotation for functions #5234

Closed

willson-wang mentioned this pull request Sep 3, 2022

webpack4+ Tree shaking原理 willson-wang/Blog#108

Open

timostamm mentioned this pull request May 1, 2023

Add /*@__PURE__*/ annotation for better tree-shaking support bufbuild/protobuf-es#470

Merged

Support marking a call as pure #1448

Support marking a call as pure #1448

Conversation

kzc commented Jan 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzc commented Jan 29, 2017 • edited Loading

kzc commented Feb 21, 2017

kzc commented Feb 21, 2017

kzc commented Feb 21, 2017

kzc commented Feb 21, 2017

alexlamsl commented Feb 21, 2017

alexlamsl commented Feb 21, 2017

kzc commented Feb 21, 2017 • edited Loading

alexlamsl commented Feb 21, 2017

kzc commented Feb 21, 2017

kzc commented Feb 21, 2017 • edited Loading

kzc commented Feb 21, 2017

alexlamsl commented Feb 21, 2017

kzc commented Feb 21, 2017 • edited Loading

alexlamsl commented Feb 21, 2017

alexlamsl commented Feb 21, 2017

kzc commented Feb 21, 2017

alexlamsl commented Feb 21, 2017

kzc commented Feb 21, 2017

alexlamsl commented Feb 21, 2017 • edited Loading

kzc commented Feb 21, 2017

kzc commented Feb 21, 2017 • edited Loading

alexlamsl commented Feb 21, 2017

kzc commented Feb 21, 2017

alexlamsl commented Feb 21, 2017 • edited Loading

kzc commented Feb 21, 2017 • edited Loading

kzc commented Feb 21, 2017

alexlamsl commented Feb 21, 2017

kzc commented Mar 28, 2017

alexlamsl commented Mar 28, 2017

IgorMinar commented Mar 30, 2017

kzc commented May 1, 2017

kzc commented Jun 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzc commented Jan 29, 2017 •

edited

Loading

kzc commented Jan 29, 2017 •

edited

Loading

kzc commented Feb 21, 2017 •

edited

Loading

kzc commented Feb 21, 2017 •

edited

Loading

kzc commented Feb 21, 2017 •

edited

Loading

alexlamsl commented Feb 21, 2017 •

edited

Loading

kzc commented Feb 21, 2017 •

edited

Loading

alexlamsl commented Feb 21, 2017 •

edited

Loading

kzc commented Feb 21, 2017 •

edited

Loading