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

Class declaration in IIFE considered as side effect #1261

Closed
blacksonic opened this Issue Aug 21, 2016 · 73 comments

Comments

Projects
None yet
@blacksonic
Copy link

blacksonic commented Aug 21, 2016

I'm sending a bug report

When i declare a class inside an IIFE, but class is not used, it isn't stripped by UglifyJS, because it is considered as a side effect.

var V6Engine = (function () {
    function V6Engine() {
    }
    V6Engine.prototype.toString = function () {
        return 'V6';
    };
    return V6Engine;
}());

The warning i get:

WARN: Side effects in initialization of unused variable V6Engine [./dist/car.bundle.js:74,4]

I suspect it is because it assigns values to the prototype.
The biggest pain point with it that it blocks tree-shaking of exported classes generated by Typescript.

Can it be fixed or is their a class declaration that is not considered a side effect?
Here is an example repository where i use UglifyJS through Webpack with Typescript.

@TheLarkInn

This comment has been minimized.

Copy link

TheLarkInn commented Aug 21, 2016

cc @DanielRosenwasser This may be of interest to you. This prevents TS users from being able to have tree shaking/dead code elim with UglifyJs

@mishoo if we can help resolve this, please point us in the right direction.

@kzc

This comment has been minimized.

Copy link
Contributor

kzc commented Aug 22, 2016

@blacksonic Uglify doesn't perform program flow analysis. It won't drop the code because of the the side effects you've noted.

You probably want to use something like rollup which supports that sort of speculative unused code dropping.

@TheLarkInn

This comment has been minimized.

Copy link

TheLarkInn commented Aug 22, 2016

Since I don't understand specifically what you mean by speculative code dropping, could you elaborate on this please?

@kzc

This comment has been minimized.

Copy link
Contributor

kzc commented Aug 22, 2016

One truly doesn't truly know the global impact of any program unless you run it or use SSA. Barring that you can trap all assignments and make some assumptions as to what is likely going on. That seems to be what rollup is doing. But one could craft code to make such a scheme fail. That's why it's speculative.

@Jessidhia

This comment has been minimized.

Copy link

Jessidhia commented Aug 22, 2016

True, it is possible one could write something like export const impure = (() => window.foo = 'bar')(), which would have different global effects depending on whether it is removed or not.

The problem is the interaction with pure declarative code that ends up using IIFE by accident of transpilation...

Speaking of "pure declarative", assuming uglify got support for this use case, how would decorators sit in this? Is there something to prevent, say, a class decorator from having global side-effects? I presume class and static member decorators have to execute at class declaration time, so if they can have side-effects, tree-shaking would affect the actual runtime result of the code.

@kzc

This comment has been minimized.

Copy link
Contributor

kzc commented Aug 22, 2016

Certain transforms are simpler to perform at the higher ES6 level, rather than with the lower level transpiled ES5 where it's more difficult to reason about side effects.

The following is using Uglify's harmony branch. Notice that the ES6 class Foo will be dropped by uglify harmony if not referenced:

$ echo 'class Foo { method(){console.log("method");} };' | bin/uglifyjs -c

WARN: Dropping unused function Foo [-:1,6]

Uglify is pretty conservative in dropping code. Rollup appears to have more aggressive code dropping heuristics.

@kzc

This comment has been minimized.

Copy link
Contributor

kzc commented Aug 22, 2016

Uglify's harmony branch for ES6 is still a work in progress and doesn't support class decorators as far as I know. @avdg could better answer what's supported in harmony.

@avdg

This comment has been minimized.

Copy link
Contributor

avdg commented Aug 22, 2016

On es 6 I'm more focused on getting stuff working (I prefer wide support
and less compression rather than supporting only few es6 features and have
scripts without any compression).

But we still haven't good destructuring or modules support for example.
There is a lot to be done there on the parser and AST side.

We have to make decisions on where to spend our time if we can't spend our
time on implementing all features. (Although, fixes are mostly relative
small, maybe we should keep more obvious things in the repo so others who
want can fix them instead?).

Although where possible, there is some constant evaluation being done, for
objects and classes that applies to computed properties (still needs to be
merged with harmony though).

Feel free to open issues with clear examples where UglifyJS could do a
better job, but UglifyJS like nearly (or) all compilers will have
limitations and some transformations may be unsafe due to a lack of agreed
concensus with the compiler on what is safe to compile.

Op ma 22 aug. 2016 05:20 schreef kzc notifications@github.com:

Uglify's harmony branch for ES6 is still a work in progress and doesn't
support class decorators as far as I know. @avdg https://github.com/avdg
could better answer what's supported in harmony.


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#1261 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAM9GPsIdPloMwdaQWy-0vyvL6bbtQyXks5qiRVigaJpZM4JpWFl
.

@TheLarkInn

This comment has been minimized.

Copy link

TheLarkInn commented Aug 22, 2016

Would it be possible to add this to the list of 'unsafe' compress features. These wrapped IIEF's are generated from transpilers? This is a pretty important optimization to both babel and Typescript users. I can't think of a code example where a transpiler would output cases like the one @kzc described due to the generative nature of the code.

@kzc

This comment has been minimized.

Copy link
Contributor

kzc commented Aug 22, 2016

It's a non-trivial amount of work to recognize code for ES6 classes converted to ES5 IIFEs and drop them if the side effects are collectively deemed to be acceptable to ignore. Uglify does not have any program flow analysis. Pull requests are welcome.

As I mentioned before, dealing with this issue at a higher level with rollup is probably your best bet.

@blacksonic

This comment has been minimized.

Copy link

blacksonic commented Aug 22, 2016

@kzc Is it a trivial amount of work to detect an IIFE or is it also considered program flow analysis? If not, with the flag one can decide if the codebase and the packages used can be trusted and no global variable manipulation occurs there.

@kzc

This comment has been minimized.

Copy link
Contributor

kzc commented Aug 22, 2016

IIFEs are trivial to detect. Deciding whether the code in them can be considered pure and side-effect free in the context of the entire program is not.

Then there's the exports assignment issue that I mentioned in the other thread that would prevent the IIFE from being dropped anyway.

If you want to put together a PR to address these issues, go for it.

@blacksonic

This comment has been minimized.

Copy link

blacksonic commented Aug 22, 2016

But it would be the developers choice to decide wether IIFEs can be considered side effect free. The transpiler won't produce side effects.

The exports variable is not interfering with this statement, which causes the side effect detection.

var V6Engine = (function () {
    function V6Engine() {
    }
    V6Engine.prototype.toString = function () {
        return 'V6';
    };
    return V6Engine;
}());
@blacksonic

This comment has been minimized.

Copy link

blacksonic commented Aug 22, 2016

If you consider it trivial can you give us hints on how and where to implement it? At first sight to understanding 1k+ files seemed hard for me who is not familiar with the codebase.

@blacksonic

This comment has been minimized.

Copy link

blacksonic commented Aug 22, 2016

This would be a big help for everyone using Webpack with Babel or Typescript, it would be not a trivial nor easy thing to migrate to Rollup.

@kzc

This comment has been minimized.

Copy link
Contributor

kzc commented Aug 22, 2016

Uglify consists of a dozen or so JS files in lib, tools and bin. It's pretty easy to follow.

@avdg

This comment has been minimized.

Copy link
Contributor

avdg commented Aug 22, 2016

Some important api docs can be found on http://lisperator.net/uglifyjs/

@blacksonic

This comment has been minimized.

Copy link

blacksonic commented Aug 22, 2016

Is it ok if i add a new option parameter to skip the check on this line?

if (def.value && def.value.has_side_effects(compressor)) {

@blacksonic

This comment has been minimized.

Copy link

blacksonic commented Aug 22, 2016

@avdg thanks, those docs are really helpful for newcomers

@kzc

This comment has been minimized.

Copy link
Contributor

kzc commented Aug 22, 2016

@blacksonic wrote:

Is it ok if i add a new option parameter to skip the check on this line?

I wouldn't recommend it. It would break a lot of uglified code.

@blacksonic

This comment has been minimized.

Copy link

blacksonic commented Aug 22, 2016

How can this break existing things if it is turned off by default?

@kzc

This comment has been minimized.

Copy link
Contributor

kzc commented Aug 22, 2016

Just because you want to drop code with a side effect in a very specific case does not mean it should be done in the general case.

$ echo '(function(){ var x = console.log("whatever"); })();' | uglifyjs -c | node
WARN: Side effects in initialization of unused variable x [-:1,17]
whatever
@blacksonic

This comment has been minimized.

Copy link

blacksonic commented Aug 22, 2016

Maybe put a check there for IIFE also besides the config check?
If so this is where i would ask for your or anyone's help, because i'm not familiar with AST.

@kzc

This comment has been minimized.

Copy link
Contributor

kzc commented Aug 22, 2016

I don't think you understand the nature of the problem you're trying to solve. Please re-read: #1261 (comment)

@blacksonic

This comment has been minimized.

Copy link

blacksonic commented Aug 22, 2016

Can you explain it in detail?

var V6Engine = (function () {
    function V6Engine() {
    }
    V6Engine.prototype.toString = function () {
        return 'V6';
    };
    return V6Engine;
}());

This part has nothing to do with the exports assignment. It is not assigned to exports.

@kzc

This comment has been minimized.

Copy link
Contributor

kzc commented Aug 22, 2016

I've tried to explain that the problem is not as simple as you believe it to be. Just examine the uglify code and you'll figure it out. Good luck.

@Jessidhia

This comment has been minimized.

Copy link

Jessidhia commented Aug 23, 2016

This is probably a bad idea due to the additional coupling, but could there be, say, an _uglifyPureIIFE "global meta-function" that, when given a function, in the default case, would be transpiled by uglify to become its argument (_uglifyPureIIFE(function () {})(args) -> +function(){}(args)), but would also allow uglify to treat that function as safe to remove when the result is unused?

@kzc

This comment has been minimized.

Copy link
Contributor

kzc commented Feb 28, 2017

For what it's worth, the pure function call comment annotation feature has been released in uglify-js@2.8.1:

$ echo 'foo(); var a=/*#__PURE__*/(function(){console.log("Hello");}()); bar();' | bin/uglifyjs -c toplevel
WARN: Dropping __PURE__ call [-:1,26]
WARN: Dropping unused variable a [-:1,11]
foo();bar();
@bhovhannes

This comment has been minimized.

Copy link

bhovhannes commented Feb 28, 2017

@sokra any idea when this will land into webpack ?

fongandrew added a commit to esperco/lutra-redux that referenced this issue Mar 8, 2017

Upgrade to Webpack2.
Also switches TypeScript to emit ES2015 modules instead of CommonJS
so we can take advantage of tree shaking to reduce bundle sizes.
The gains here are barely noticeable though (there's an outstanding
issue with how TypeScript emits classes and how Uglify interacts
with that that may help once that's fixed), but every little bit
helps.

mishoo/UglifyJS2#1261
Microsoft/TypeScript#13721
webpack/webpack#2899

TODO: Look to see if we can convert more of our more imports to
ES2015 to impove tree shaking.
@jods4

This comment has been minimized.

Copy link

jods4 commented Mar 8, 2017

Before TS might release this in class emit, I am wondering if we should ask them some support for decorators.

Decorators being arbitrary functions, it's nearly impossible to detect if they have side effects or not. So any class with a decorator is going to be kept, even when unused.

Unfortunately, frameworks like Angular or Aurelia make extensive use of decorators, since they are a very natural syntax to attach metadata.

Typescript generates this code when using decorators:

// class emit:
var ClassX = /*#__PURE__*/(...);
// decorators emit:
ClassX = /*(1)*/__decorate([deco1, deco2, deco3], ClassX);

Question: if there was a __PURE__ annotation in (1), would uglify completely drop the ClassX?

My idea behind this is to ask TS to also move a pure comment before decorators in this spot.

Concretely, if I know my decorators are harmless and I want my class to be dismissable (particularly in large frameworks/libs) I would write this:

/*#__PURE__*/
@cacheable
class X { }

And TS could maybe move that pure comment in spot (1) above, and my class could be dropped completely...

An even better option but depending on TS goodwill would be to annotate the decorators themselves and have the annotation inserted automatically when applicable. :/

@kzc

This comment has been minimized.

Copy link
Contributor

kzc commented Mar 8, 2017

@jods4 Sorry, decorators are out of scope for Uglify and should be implemented at the higher level language level.

By the way, dropping unused classes is easy at the ES6 level and uglify harmony already does this without any need for annotations. It's only at the ES5 generated code level does this problem become difficult because any ES5 IIFE can potentially have side effects:

$ echo 'class Foo{ bar(){} } baz();' | bin/uglifyjs -c toplevel
WARN: Dropping unused function Foo [-:1,6]
baz();
@jods4

This comment has been minimized.

Copy link

jods4 commented Mar 8, 2017

@kzc I know.
The poorly expressed question was:
If TS emitted the following for a class with decorators:

var ClassX = /*#__PURE__*/(function() { ... }());
ClassX = /*#__PURE__*/__decorate([deco1, deco2, deco3], ClassX);

Would it be completely dropped ?

@kzc

This comment has been minimized.

Copy link
Contributor

kzc commented Mar 8, 2017

With uglify-js@2.8.10 neither statement above would be dropped because ClassX is assigned to twice. There is no data flow analysis in Uglify, so the worst case was assumed and the code was retained.

However, the following would work:

$ cat q.js 

    var ClassX = /*#__PURE__*/(function() { whatever(); }());
    var DecoratedClassX = /*#__PURE__*/__decorate([deco1, deco2, deco3], ClassX);

$ bin/uglifyjs q.js -c toplevel,passes=2

    WARN: Dropping __PURE__ call [q.js:3,39]
    WARN: Side effects in initialization of unused variable DecoratedClassX [q.js:3,8]
    WARN: Dropping __PURE__ call [q.js:2,30]
    WARN: Dropping unused variable ClassX [q.js:2,8]

    deco1,deco2,deco3;

as would:

$ cat r.js

    var ClassX = /*#__PURE__*/__decorate([deco1, deco2, deco3],
        /*#__PURE__*/(function(){ whatever(); })()
    );

$ bin/uglifyjs r.js -c toplevel

    WARN: Dropping __PURE__ call [r.js:2,30]
    WARN: Dropping __PURE__ call [r.js:3,21]
    WARN: Side effects in initialization of unused variable ClassX [r.js:2,8]

    deco1,deco2,deco3;

The reason that the arguments deco1,deco2,deco3 are not dropped is because they are not declared and considered global and could throw if not defined. Had they been declared or been constants, then all code would be dropped:

    var deco1, deco2, deco3;
    var ClassX = /*#__PURE__*/__decorate([deco1, deco2, deco3],
        /*#__PURE__*/(function(){ whatever(); })()
    );
@jods4

This comment has been minimized.

Copy link

jods4 commented Mar 8, 2017

interesting, thanks!

@kzc

This comment has been minimized.

Copy link
Contributor

kzc commented Mar 9, 2017

Furthermore, if the decorators were functions not used elsewhere then they would also be dropped accordingly:

$ cat s.js

    function deco1(obj) { obj.one   = 1; return obj; }
    function deco2(obj) { obj.two   = 2; return obj; }
    function deco3(obj) { obj.three = 3; return obj; }

    var ClassX = /*#__PURE__*/__decorate([deco1, deco2, deco3],
        /*#__PURE__*/(function(){ whatever(); })()
    );

    var ClassY = /*#__PURE__*/__decorate([deco1, deco3],
        /*#__PURE__*/(function(){ whatever(); })()
    );

    var ClassZ = /*#__PURE__*/__decorate([deco1],
        /*#__PURE__*/(function(){ whatever(); })()
    );

    foo( new ClassZ );

$ bin/uglifyjs s.js -c toplevel,passes=2 -b

    WARN: Dropping unused function deco2 [s.js:3,13]
    WARN: Dropping unused function deco3 [s.js:4,13]
    WARN: Dropping __PURE__ call [s.js:6,30]
    WARN: Dropping __PURE__ call [s.js:7,21]
    WARN: Dropping unused variable ClassX [s.js:6,8]
    WARN: Dropping __PURE__ call [s.js:10,30]
    WARN: Dropping __PURE__ call [s.js:11,21]
    WARN: Dropping unused variable ClassY [s.js:10,8]

    function deco1(obj) {
        return obj.one = 1, obj;
    }

    var ClassZ = __decorate([ deco1 ], function() {
        whatever();
    }());

    foo(new ClassZ());
@kzc

This comment has been minimized.

Copy link
Contributor

kzc commented Mar 9, 2017

The __decorate function can be declared to be a pure function in an Uglify option so it does not have to be annotated:

$ cat t.js

    function deco1(obj) { obj.one   = 1; return obj; }
    function deco2(obj) { obj.two   = 2; return obj; }
    function deco3(obj) { obj.three = 3; return obj; }

    var ClassX = __decorate([deco1, deco2, deco3],
        /*#__PURE__*/(function(){ whatever(); })()
    );

    var ClassY = __decorate([deco1, deco3],
        /*#__PURE__*/(function(){ whatever(); })()
    );

    var ClassZ = __decorate([deco1],
        /*#__PURE__*/(function(){ whatever(); })()
    );

    foo( new ClassZ );

$ bin/uglifyjs t.js -c toplevel,passes=2 -b --pure-funcs __decorate

    WARN: Dropping unused function deco2 [t.js:3,13]
    WARN: Dropping unused function deco3 [t.js:4,13]
    WARN: Dropping __PURE__ call [t.js:7,21]
    WARN: Dropping unused variable ClassX [t.js:6,8]
    WARN: Dropping __PURE__ call [t.js:11,21]
    WARN: Dropping unused variable ClassY [t.js:10,8]

    function deco1(obj) {
        return obj.one = 1, obj;
    }

    var ClassZ = __decorate([ deco1 ], function() {
        whatever();
    }());

    foo(new ClassZ());
@jods4

This comment has been minimized.

Copy link

jods4 commented Mar 9, 2017

@kzc but this is highly dependent of what the invoked decorators actually do!

Some decorators may have wanted side-effects and removing the __decorate call can be plain wrong.
Unlike the class code gen, this is not an always-or-never situation, more a case-by-case basis.

@kzc

This comment has been minimized.

Copy link
Contributor

kzc commented Mar 9, 2017

Marking a function as pure via --pure-funcs does not mean it will be necessarily be removed. It will only be removed if its result is not used. Study the example above.

@jods4

This comment has been minimized.

Copy link

jods4 commented Mar 9, 2017

@kzc the point is that even if its result (the class) is not used, you may want to keep it because of decorators side-effects. My point is that you can't say globally __decorate is pure, remove it when the class is not used. It depends on each instantiation.

For example:

// Here the decorators are just metadata, you can drop the class if it's not used
@cacheable
class A {}

const A = /*#__PURE__*/__decorate([cacheable], class { });

// Here the decorator registers the class in a pub-sub event bus, you should NOT remove it!
@subscribe('login')
class B { }

const B = __decorate([subscribe('login')], class { });
@kzc

This comment has been minimized.

Copy link
Contributor

kzc commented Mar 9, 2017

Okay, you have the flexibility to annotate specific __decorate calls with /*#__PURE__*/ if you choose.

@curran

This comment has been minimized.

Copy link

curran commented Mar 17, 2017

Related discussion about introducing this annotation in Rollup and Webpack to enable correct (non-speculative) tree shaking: https://github.com/d3/d3/issues/3076

@kzc

This comment has been minimized.

Copy link
Contributor

kzc commented Feb 2, 2018

@alexlamsl It appears that you quietly implemented this feature - dropping unused side-effect-free class IIFEs in 0b0eac1:

$ bin/uglifyjs -V
uglify-js 3.3.9
$ cat t1261.js
var Foo = (function() {
    function Foo() {
    }
    Foo.prototype.toString = function() {
        return 'V6';
    };
    return Foo;
}());
var Bar = (function() {
    function Bar() {
    }
    Bar.prototype.toString = function() {
        return 'V8';
    };
    return Bar;
}());
console.log(new Foo().toString());
$ cat t1261.js | bin/uglifyjs -bc toplevel,passes=3
var Foo = function() {
    function Foo() {}
    return Foo.prototype.toString = function() {
        return "V6";
    }, Foo;
}();

console.log(new Foo().toString());

Nice!

@alexlamsl

This comment has been minimized.

Copy link
Collaborator

alexlamsl commented Feb 2, 2018

@kzc I was staring at those (function(){}).prototype.destroy = ... under test/benchmark.js and thought we can compress those.

I don't think #2612 is general enough – a second Bar.prototype.prop=... would defeat it.

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