v0.49.0 DCE/treeshaking regression

[leeoniya]

function repaint(node) {
   node && node.el && node.el.offsetHeight;
}

which forces a DOM repaint by accessing offsetHeight gets improperly culled to a noop:

function repaint(node) {

}

...and is then removed from all call sites.

probably introduced by #1582

[leeoniya]

this [1] could be useful for a whitelist if you guys can assert that the property owner is a dom element.

i could probably work around this (eg el.getBoundingClientRect()), but i suspect the few element methods that can be used without side-effects are more expensive than any single property access.

[1] https://gist.github.com/paulirish/5d52fb081b3570c81e3a

[kzc]

Even if Rollup does not know it's a DOM property with side effects, the property read should still be preserved in the event that the object may be undefined:

$ bin/rollup -v
rollup version 0.49.1

$ cat prop.js
function f(o) {
  o.prop;
}
var u = global.no_such_variable;
try {
  f(u);
  console.log("FAIL");
} catch (x) {
  console.log("PASS");
}

$ cat prop.js | node
PASS

$ bin/rollup prop.js -f es --silent | node
FAIL

$ bin/rollup prop.js -f es --silent
try {
  console.log("FAIL");
} catch (x) {
  console.log("PASS");
}

For that matter, Rollup drops globals that could throw:

$ cat global.js 
try {
  g;
  console.log("FAIL");
} catch (x) {
  console.log("PASS");
}

$ cat global.js | node
PASS

$ bin/rollup global.js -f es --silent | node
FAIL

$ bin/rollup global.js -f es --silent
try {
  console.log("FAIL");
} catch (x) {
  console.log("PASS");
}

[mjeanroy]

Same problem here, here is a simple repository to reproduce the issue.

[lukastaegert]

This is really bad. Rollup was never able to handle getters properly but I think no-one noticed because tree-shaking failed for so many other reasons. The SAFE way would be to assume that any property access is actually a getter unless we are REALLY sure it is not. Basically, this would cripple the algorithm and we are back to square one 😢

HOWEVER looking at your examples @leeoniya, the way you access this getter is via calling an expression as a statement. So maybe this is something we can work with as this is something one would not usually do unless they expect an effect.

@kzc Looking at your example, I actually assumed that people would not use something as horrific as a try-catch to catch an undefined-is-not-an-object and make their code rely on that. So unless someone is really complaining I would not change code to fix your example because, again, this would certainly prevent tree-shaking except for the most basic cases.

Forget what I wrote, maybe even your code might be fixed by this. Let's see.

[lukastaegert]

@mjeanroy Since you are calling a constructor for side effects, your issue might be addressed by this as well. Will look into this tomorrow.

[kzc]

I actually assumed that people would not use something as horrific as a try-catch to catch an undefined

The try/catch is not relevant to the two issues demonstrated. They were just put in there to show unambiguous test output.

Uglify has an optional pure_getters mode for property reads:

$ echo 'function foo(bar){ bar.prop; }' | uglifyjs -c
function foo(bar){bar.prop}

$ echo 'function foo(bar){ bar.prop; }' | uglifyjs -c pure_getters
function foo(bar){}

Perhaps Rollup could offer something similar.

[lukastaegert]

@kzc I think you are right, this is exactly what we need. This sounds like the best way to address the issue of handling getters. However, changing the interface of rollup and introducing options like these would probably require a little more discussion, maybe we should make a separate issue for this? I would certainly offer my help in wiring this up, but right now I do not want to make possibly breaking interface decisions.
As such a flag would still not cover @mjeanroy 's case, I will at the meantime go ahead and implement special treatment for certain expressions-as-statements for which I now seem to have a working implementation that does not break any tests and covers all cases mentioned here.

[lukastaegert]

Ok, I added my code to #1591. Let's see if this fixes your issues once it gets merged.
By the way, it seems directly installing from pull requests is kind of broken in NPM 5–if you manage to pull this of, please drop a note to tell me how you did it so I can tell others 😉

[Rich-Harris]

Released 0.49.2 with the fix — thanks. Ultimately I think @kzc is right, we're forced to default to assuming that there are getters with side-effects everywhere. In the longer term it'd be nice to keep track of objects and properties so that this...

foo = { bar: 1 };
foo.bar;

...is understood to not have side-effects. Tricky because you need to start tracing values as they flow through the program (aka the big rewrite I aborted a while back 😬), but certainly possible in many scenarios.

[lukastaegert]

I agree and I already got some ideas on how to handle this. Basically, I would not track values in a general sense but instead assignments of nodes to other nodes and really beef up what .assignedExpressions can do. But the ideas are still a little vague, for now I want to refactor call expression handling first to complete what I have started with the first refactoring (and finally add tree shaking to ClassDeclarations).

[mjeanroy]

@lukastaegert Thanks for the hard work!

[leeoniya]

thanks @lukastaegert

[leeoniya]

Ultimately I think @kzc is right, we're forced to default to assuming that there are getters with side-effects everywhere.

getters with side-effects is an extremely shitty pattern i have never encountered in real code. unfortunately we're stuck with the DOM api in this case. i don't think this is worth supporting generically if it makes the codebase slower and more complex. it's also highly unlikely that real code would ever have eg elem.offsetHeight; statement that wasnt there for side-effects.

the implemented fix is sufficient, imo.

[lukastaegert]

Well, there is of course pixi.js–works really well, but they REALLY love their getters.
I think a flag like pure_getters will be the sweet spot. The only question will be what we pick as default–true or false. I fear for convenience reasons it should be false.

[leeoniya]

the issue here is getters with side-effects, right? there's no problem removing getters otherwise.

in the file you linked there is one getter where the comment says it "sets" [1], but the comment appears to be wrong. there's another one that may initialize a DisplayObject but it's unlikely anyone accesses that getter simply for that side-effect without using the return value.

[1] https://github.com/pixijs/pixi.js/blob/dev/src/core/display/DisplayObject.js#L589
[2] https://github.com/pixijs/pixi.js/blob/dev/src/core/display/DisplayObject.js#L144

[kzc]

the issue here is getters with side-effects, right? there's no problem removing getters otherwise.

Rollup cannot know which getters have side effects so it has to be conservative by default. Or whether obj is undefined or null in obj.prop. There's a lot of zany javascript out there in the wild with all sorts of crazy side effects.

[leeoniya]

i understand.

i'm postulating that getters with side-effects are quite rare, because they're shit pattern - no different than a function called getTime() that also silently resets some global timezone. the pixi.js example does not disprove this assertion by virtue of using a lot of getters, none of which have side-effects.

[localvoid]

Chai getters have side-effects.

[leeoniya]

Chai getters have side-effects.

where?

https://github.com/chaijs/chai/search?utf8=%E2%9C%93&q=get&type=

[lukastaegert]

Not sure where but chai is usually used like

expect(value).to.be.true

I assume they are creating the getters programmatically. It would certainly make chai unusable if these statements were removed. Luckily I think we got this one covered with the latest update.
In fact, I assume expect would probably never be removed anyways.

[leeoniya]

interesting. it seems linters didn't particularly appreciate this either:

https://github.com/chaijs/chai/blob/master/ReleaseNotes.md#noop-function-for-terminating-assertion-properties

In fact, I assume expect would probably never be removed anyways.

right, only .to.be.true would have been dropped as it appears to be a useless property access.

i stand by my previous statement that the implemented fix is sufficient, even for these oddball cases :p