Unused constructor call is unsafely stripped out

[gaearon]

Example here:

var hasBadPolyfill = false;
try {
	var nonExtensibleObject = Object.preventExtensions({});
	new Map([[nonExtensibleObject, null]]);
	new Set([nonExtensibleObject]);
} catch (err) {
	hasBadPolyfill = true;
}
console.log(hasBadPolyfill);

Result:

'use strict';

var hasBadPolyfill = false;
try {
	var nonExtensibleObject = Object.preventExtensions({});
	
} catch (err) {
	hasBadPolyfill = true;
}
console.log(hasBadPolyfill);

However this breaks the feature detection we were trying to do.
(In our case, this led to a crash: facebook/react#11745.)

[kzc]

Polyfill detection code and polyfills shouldn't be run through Rollup. It should be prepended to the bundle or loaded separately.

Such feature detection is at odds with the goals of Rollup - it assumes an ES6 environment and that Map and Set exist and those statements are side effect free.

You could hack out those classes, but that's not a great idea:

--- a/src/ast/nodes/shared/pureFunctions.js
+++ b/src/ast/nodes/shared/pureFunctions.js
@@ -22,7 +22,6 @@ simdTypes.forEach( t => {
        'Date', 'Date.UTC', 'Date.now', 'Date.parse',
        'String', 'String.fromCharCode', 'String.fromCodePoint', 'String.raw',
        'RegExp',
-       'Map', 'Set', 'WeakMap', 'WeakSet',
        'ArrayBuffer', 'ArrayBuffer.isView',
        'DataView',
        'JSON.parse', 'JSON.stringify',

If you must run such code through Rollup, I'd suggest calling a bogus function with new Map and new Set as arguments:

function detect(x) {
	if (x === detect) throw detect;
	return x;
}
var hasBadPolyfill = false;
try {
	var nonExtensibleObject = Object.preventExtensions({});
	detect(new Map([[nonExtensibleObject, null]]));
        detect(new Set([nonExtensibleObject]));
} catch (err) {
        //console.log("caught:", err);
	hasBadPolyfill = true;
}
console.log(hasBadPolyfill);

[lukastaegert]

H @gaearon ,
yes, this is a design decision. Rollup assumes core JavaScript globals always work as specified and it is maintaining a list of pure functions to be able to permit users to use things like Object.keys or Map in their module initialisation code without preventing tree-shaking.

I must admit, though, that feature detection is something I did not have on my radar as much as I should have. For the short term, I would actually suggest to do what @kzc mentions but is not happy about–to remove the functions in question from the "pure functions" list. This is not unprecedented:
In this commit, I removed Function from this list to enable some feature detection (I must admit I cannot find the relevant issue number now see this example). Nevertheless, I would also love to have a plan for a more permanent solution.

1. @kzc 's suggestion to not run polyfills through rollup would work without any changes to rollup. However, I think this is not ideal:

   • Not bundling them will make the initialisation of the polyfills less efficient
   • This will put the burden of understanding these issues on end users which I see at odds with rollup's goal of being easy to use

2. The simplest approach on our side would be to allow a config option to specify a list of "impure functions" to override the defaults. Again, I think this is not ideal:

   • Now, end users actually need to know how the feature detection of their polyfills works!
   • Also, this would break tree-shaking for other uses of the global functions in question.

3. We could implement a new comment syntax, something like /*#__IMPURE__*/ or /*#__KEEP__*/. This would put the burden of adding comments on the polyfill makers. Which I very much prefer to the previous option, at least those people actually KNOW what is important for their polyfills to work and this would only need to be done once. Also, this would not break tree-shaking in any other places.

I am very open to feedback and further suggestions regarding this. If we go with option 3, which I very much recommend, we should also

• Have a word with the maintainers of other projects that might want to consume these comments–mainly UglifyJS and possibly Webpack. We should not go alone here and risk further ecosystem fragmentation.
• Try to get the most important polyfills on board. I am mainly thinking core-js here, but it would not hurt if other more specialised polyfills were on board as well.

What do you think?

[gaearon]

To be 100% clear, in my case I'm not adding a polyfill. I am trying to detect a bad polyfill from my library (by feature testing) so I can avoid using it.

I don't think it's practical for us to separate this code from Rollup. I don't see a non-convoluted way to do this while keeping the source modular.

An annotation to prevent stripping out seemingly pure expressions would indeed be helpful.

[kzc]

This may be pedantic, but I wouldn't label this a bug - Rollup is working as designed. This is a feature request.

When the code in the top post is run through Google Closure in advanced mode it produces:

var a = !1;
try {
  Object.preventExtensions({});
} catch (b) {
  a = !0;
}
console.log(a);

Perhaps Closure has some similar annotation to prevent side-effect-free code from being dropped.

[gaearon]

Fair enough :-) I agree it's a feature request from the context above.

[kzc]

A zero overhead production workaround with existing tools:

$ cat keep.js
var hasBadPolyfill = false;
try {
	var nonExtensibleObject = Object.preventExtensions({});
	keep(new Map([[nonExtensibleObject, null]]));
        keep(new Set([nonExtensibleObject]));
} catch (err) {
	hasBadPolyfill = true;
}
console.log(hasBadPolyfill);

$ rollup -v
rollup version 0.52.0

$ uglifyjs -V
uglify-es 3.2.1

$ rollup keep.js -f es --silent | uglifyjs --toplevel -mc pure_funcs=[keep],passes=3 -b
var e = !1;

try {
    var n = Object.preventExtensions({});
    new Map([ [ n, null ] ]), new Set([ n ]);
} catch (n) {
    e = !0;
}

console.log(e);

In dev mode the calls to keep could be replaced in a rollup.config.js file with Object to produce:

	Object(new Map([[nonExtensibleObject, null]]));
        Object(new Set([nonExtensibleObject]));

which is retained by both uglify and Closure (in simple mode).

Edit: Or just have rollup remove keep in the rollup config in dev mode.

[lukastaegert]

I guess I have to agree this is currently an enhancement. I thought that this might be a potentially more wide-spread issue that had just gone under the radar but browsing through core-js I think they are–for the time being–still safe from being broken by our algorithm.

As for GCC, it seems that even though they too encountered these issues, no annotation syntax was introduced. Uglify does not seem to have a syntax for this as well so if we introduce this, this might actually be a first.

As for myself, I would very much like to introduce such a syntax now because

• This is not the first time we had such an issue, see v0.49.0 DCE/treeshaking regression #1592
• The more we teach rollup about the default JavaScript globals, the more of these issues w are going to have, so having might want to have something like this in place
• This might also prove useful as a debugging feature

I would suggest introducing an annotation that can be placed in front of any statement and that will prevent this statement AND ALL ITS CHILDREN from being removed (that way, people will not need to know too much about AST semantics). As for a name, /*#__KEEP__*/ (or /*@_KEEP_*/) is what for now sounds best to me. More thoughts?

[guybedford]

Perhaps we could apply a heuristic here and detect if a side-effect-free statement that also has the chance of not being supported in legacy browsers, that is also within a try-catch block indicates a "feature detection" and that it shouldn't be treeshaken.

[guybedford]

(personally I'd prefer to avoid annotations wherever we can help it as a guiding philosophy of a "do-the-right-thing js compiler")

[lukastaegert]

I am not a huge fan of heuristics that

1. force you to write code in a certain style
2. have the potential to make tree-shaking worse for many that do not rely on this
3. are easily broken by future refactorings as it is hard to implement them in a localized and clean manner, in contrast to annotations. I.e. heuristics tend to make the code base harder to maintain. We already had a similar heuristic to deal with getters at some point and I am very glad we finally could get rid of it.

I.e. why is it "the right thing" to keep side-effect free (by spec) statements in try...catch blocks? What about side-effect-free statements in functions that are called from within try...catch blocks, do we need to trace this "being called from a try-catch blocked"? What if I want to do it in a Promise instead? Which statements do I want to keep? Do we want to maintain a second list of pure functions that "are impure when used inside a try-catch block"?

Annotations are the cleanest approach that even works when re-bundling bundled code.

[ghost]

Why even warn about bad polyfills? Most users use @babel/polyfill which are spec-compliant anyway nowadays.

[gaearon]

Because if you do get a bad one you're gonna blame React for being slow.

[ghost]

@gaearon Why not recommend a specific polyfill in the README?

[AndrewLeedham]

I ran into this issue recently when using @babel/preset-env's useBuiltIns: "usage" option to bundle core-js polyfills. I created a minimal repro. As well as a more involved recreation: https://codesandbox.io/s/r0vj2k7znn. I reported the issue in the core-js repo zloirock/core-js#494, so a workaround has been added.

[lukastaegert]

I know there has been a long silence around this issue but #2892 will finally provide a solution for this that should work for you, comments welcome.

[lukastaegert]

Citing myself:

I.e. why is it "the right thing" to keep side-effect free (by spec) statements in try...catch blocks?

I am growing older and starting to accept things.