bug in unused on labelled for-loop

[alexlamsl]

I hope I haven't gone crazy, but this seems to be a pretty old bug:

$ echo 'function f(){out:for(var i=x();;)continue out;}' | node

$ echo 'function f(){out:for(var i=x();;)continue out;}' | bin/uglifyjs -c
function f(){out:{for(x();;)continue out}}

$ echo 'function f(){out:for(var i=x();;)continue out;}' | bin/uglifyjs -c | node
[stdin]:1
function f(){out:{for(x();;)continue out}}
                                     ^^^
SyntaxError: Undefined label 'out'
    at createScript (vm.js:53:10)
    at Object.runInThisContext (vm.js:95:10)

$ git checkout v2.7.5
$ echo 'function f(){out:for(var i=x();;)continue out;}' | bin/uglifyjs -c | node
WARN: Side effects in initialization of unused variable i [-:1,25]
[stdin]:1
function f(){out:{for(x();;)continue out}}
                                     ^^^
SyntaxError: Undefined label 'out'
    at createScript (vm.js:53:10)
    at Object.runInThisContext (vm.js:95:10)

[kzc]

Doesn't surprise me - labelled loops are not very commonly used in the wild.

Labelled loops/breaks/continues could be added to the fuzzer easily enough.

[alexlamsl]

Indeed - the attempt to fix #44 turns out to be incomplete thus causing this issue.

The straightforward way is to wrap another layer of if (node instanceof AST_... around that workaround to take care of this particular case.

There is only one reason why AST_BlockStatement needs to be generated though, i.e. when the last variable on var statement is unused but its value has side-effects. If I can somehow make that go away, then we can remove this workaround altogether.

// Original
var a = x(), b = y(), c = z();
console.log(b);
// Uglified
{
    var b = (x(), y());
    z(); <== if only I can pack this back up there
}
console.log(b);

[kzc]

Sorry for being daft, but why is a new block needed at all?

[alexlamsl]

You can only replace AST_Node with another, single AST_Node unless you are in_list (which you won't be with AST_For.init).

That example wasn't accurate but merely illustrative, as it would be in_list within AST_Block in that case. But I put it down like that so it's easier to think about/around it.

[kzc]

I see.

OT 1: Nearly 300 lines for drop_unused - yikes.

OT 2: Would save 2 bytes if var b=(x(),y());f(b); were transformed into x();var b=y();f(b);

[alexlamsl]

OT 1: Nearly 300 lines for drop_unused - yikes.

I know - I had a hard time parsing this section of the codebase as well, especially with how it is called within Compressor.before() 😅

OT 2: Would save 2 bytes if var b=(x(),y());f(b); were transformed into x();var b=y();f(b);

Fair point - let me see if I can make that happen with the clean up PR I was originally working on (which is another spin-off from #1821)