(Travis CI timed out.)

Nice.

LGTM

Discovered bug while testing with test/benchmark.js - investigating.

It would be good to run uglifyjs -c -m on well known benchmarks that verify their results.

This one uses real life code rather than synthetic and/or unit tests:

http://browserbench.org/JetStream/

So I stepped on a landmine which is make_node_from_constant() calling AST_Node.optimize() without any regards for the compressor's context 😅

Given the intertwined nature of this bug, I guess I should fix it alongside this PR. If I'm correct, it will also trigger in the case of [].join() optimisation as well.

Phew, I think this is sorted and ready to go now 😉

It would be good to run uglifyjs -c -m on well known benchmarks that verify their results.

I couldn't find those files in a VCS repository - should I just download a copy of everything from http://browserbench.org/JetStream/in-depth.html and work from there?

Edit: #1479

I see you've changed optimize() to transform() in a few places - what's the difference?

Also, you've dropped optimize() altogether in some spots? ¿Qué pasa?

I couldn't find those files in a VCS repository - should I just download a copy of everything from http://browserbench.org/JetStream/in-depth.html and work from there?

I tried running Jetstream locally with node but errored out because it can't find the source for the function BenchmarkSuite, nor could I find a source repo. Then I gave up.

AST_Node.optimize(compressor) does not reset TreeWalker.stack and just continue from where make_node_from_constant() is called - so when you tried to evaluate() AST_Call.args and the call happens to be in a Boolean context, e.g. c(a) ? 1 : 2, a would be told to be safe to transform into AST_Boolean. Obviously bad things then happen.

AST_Node.transform(compressor) start a fresh tree transformation so this won't be an issue. I basically took out the AST_Constant calls to .optimize(compressor) where there are no transformation defined within compress.js, then convert the ones that do to .transform(compressor) to avoid the aforementioned issue.

@alexlamsl Regarding your explanation above - were you able to create a such a failing test case against uglify-js@2.7.5?

@kzc the cases I run into where .in_boolean_context() would cause confusion applies to AST_Array, AST_Object and AST_RegExp. The other place I spotted this to have a context mismatch, [].join(), only calls make_node_from_string() that will hit the AST_String path.

I could continue to investigate further to see if other evaluate() / make_node_from_constant() cases would have this issue, but so far I haven't got any other cases where this would lead to incorrect results on master yet.