"pureFunctions" are not actually pure, and are not safe to remove.

[ljharb]

tl;dr: the vast majority of the language builtins listed in https://github.com/rollup/rollup/blob/master/src/ast/nodes/shared/pureFunctions.ts can throw exceptions, which makes them not "pure", and decidedly unsafe to remove.

Specifically, the es6-shim - something that is widely deployed on a not-insignificant percentage of the internet - has many things like this line that rely on a thrown exception, or lack thereof, to indicate if a builtin implementation is broken and needs replacement. We pre-build our shims at @airbnb with rollup, and this resulted in our website being broken for, in particular, IE 11 users - because rollup transpiled this:

var objectKeysAcceptsPrimitives = !throwsError(function () { Object.keys('foo'); });

into this:

var objectKeysAcceptsPrimitives = !throwsError(function () {  });

Anything that can possibly throw an exception under any circumstances (Object.keys(null), for example) is not something that's necessarily safe to remove (depending on if the parameters are all inline literals that meet the appropriate criteria), and although I haven't exhaustively gone through the list of pureFunctions, at a glance, almost all of them have a path in the spec that can throw.

I've patched around this issue in v0.35.4 of es6-shim by adding return so that rollup is not fooled into thinking the statements are no-ops, but it'd be ideal to fix this in rollup itself - there may be other cases I've not yet discovered..

[ljharb]

any update? This seems like a critical flaw in one of the core heuristics of this project - it'd be nice to have some kind of response from maintainers.

[lukastaegert]

Feature detection has always been a problem as one of the core assumptions is that the software is running in a well-behaved, spec-compliant environment.

My best idea to allow feature detection workflows would be to deactive any assumptions on globals inside try-catch blocks. Would this work for you?

The easy solution here would be to just deactivate them for calls to globals directly inside such a block, the complicated and much more wasteful solution would be to also deoptimize functions called from such a scope.

Would the simple solution help you?

[lukastaegert]

I would rather avoid removing the pure function logic as most users are profiting hugely here.

[lukastaegert]

It would also be possible to add a flag to deactivate the pure function logic altogether.

[ljharb]

Note that relying on the exceptions these methods can throw is well-behaved and spec-compliant.

es6-shim’s usage has the thrower in a function, that’s separately wrapped, so you wouldn’t be able to detect this.

Users may be profiting in terms of bundle size, but that’s much less important than how they’re losing in terms of correctness. The logic simply isn’t a safe default - it’d be fine to make it opt-in with a flag tho.

[robpalme]

The common case is that built-in methods exist and are unmodified for some version of the spec. The proof is the happy users.

Maybe a compromise would be to accept a boolean to express "builtins might be overridden and may not exist", or perhaps an array of explicit strings for unreliable builtins so you can customize based on your lowest targeted browser version.

It feels like this is an important issue to address because it's about correctness, as @ljharb says, but I am more ambivalent on which way the default goes.

[lukastaegert]

I still have hope that we could build some automatism that

• inside try-catch, pure functions are considered impure
• each function called from such a context receives the same deoptimization

To my understanding, this should cover es6-shim's approach. If we go for this, then this will definitely go with a test suite that contains important real-world polyfills.

[lukastaegert]

Also wondering if it would make sense to make this configurable on a per-module basis, ideally by plugins.

[ljharb]

@lukastaegert i'm all for whatever works! however function tryFn(fn) { try { return fn(); } catch (e) { } } is a pattern i tend to use, particularly because many engines (including v8 until recently) deoptimize inline try/catches but don't deopt with that pattern.

Whatever the choice, I hope that the default is made maximally safe, not maximally small.

[lukastaegert]

#2892 will finally provide a solution for this that should work for you, comments welcome.