eval() breaks unused argument tree-shaking

[robpalme]

• Rollup Version: 1.26.5 (latest)
• Operating System (or Browser): Win 10
• Node Version: v12.9.1

How Do We Reproduce?

REPL link

function foo(a) { eval("console.log(a)"); }
foo(0);

Expected Behavior

No code change.

function foo(a) { eval("console.log(a)"); }
foo(0);

And perhaps a emit a warning: (!) Use of eval is strongly discouraged because it disables optimizations

Actual Behavior

The argument 0 is incorrectly tree-shaken away.

function foo(a) { eval("console.log(a)"); }
foo();

A warning is emitted: (!) Use of eval is strongly discouraged

Related

• Disable tree shaking for specific symbol #2963
• Guide documentation: Avoiding eval

Discussion

After upgrading Rollup from 1.8.0 to 1.26.2 we found that unused argument tree-shaking introduced in 1.14.0 caused some production code to break. (This has been the only regression so far across a huge codebase, so overall the overall quality level of Rollup seems very high!) The code used an eval'd string to refer to a function parameter. Rollup obviously could not see into the string, assumed the parameter was unused, and incorrectly dropped the argument from the callsites.

I agree that the current warning and guidance to avoid eval is the right advice - most people will be able to rewrite their code in terms of Function constructors and all will be well.

For code that needs to use eval, such as third-party source code that cannot be rewritten, I believe the correct behaviour should be to "de-opt" and disable tree-shaking for all variables accessible from the eval callsite. The current implementation is breaking the important contract that "optimization tools should not change observable semantics", i.e. tree-shaking is currently an unsafe optimization in the presence of eval.

Whilst this issue remains, our workaround will probably be to scan files for eval usage pre-Rollup, and then exclude those files from being processed by Rollup. Therefore the excluded files will not be inlined.

I'd be happy to collaborate on fixing this, if you are happy with the premise and approach.

[lukastaegert]

I see. Admittedly, I am not too happy about addressing this. The reason is that using eval is an extremely risky liability when bundling code:

• As you said, we need to include all visible variables in all surrounding scopes up to the module level
• If variables are renamed for scope-hoisting, code can break as well. This one we cannot really prevent entirely.
• After bundling, new variables will be visible from the evaled code that the code can interact with. Theoretically, it can screw up the whole application.

Nevertheless, what we could do is add a simple algorithm that crawls up the scopes and just includes everything it finds, e.g. ChildScope.includeVisibleVariables. To work properly, it cannot be called in CallExpression.bind where we emit the "do not use eval" warning because it needs to happen after binding all variables. Maybe we call it the first time the CallExpression is included.

Still, I would very much recommend you find a way to improve those third-party dependencies for the reasons I listed above, because you can never be truly safe otherwise.

[lukastaegert]

A PR similar to what I outlined above would be welcome, though.

[robpalme]

Thanks for the quick reply, @lukastaegert . I fully agree eval is dangerous and should be discouraged. In terms of safety, this change will only increase Rollup's safety given the current behaviour is to tolerate eval.

After a bit more research, it seems like for our use-case, disabling Rollup's treeshaking fully solves our problem. We found that running Terser on the output of Rollup in both modes (treeshaking on and off) results in similar final code size. This might be because our codebase is currently does not use named exports. So that removes the urgency for us to work on this.

If anyone else wants to pick this up, I still regard the underlying safety issue as being valuable to solve.