Uglifyjs: [ES6] Function hoisting + if-return optimisation breaks let scope

[AshleyScirra]

Input:

"use strict";
(function () {
    if (window.featureDisabled)
        return;

    let foo = "bar";

    function doSomething()
    {
        console.log(foo);
    };

    window.addEventListener("dummy", doSomething);

})();

Compile with --compress --beautify:

"use strict";
!function() {
    function doSomething() {
        console.log(foo); // OOPS: out of scope!
    }
    if (!window.featureDisabled) {
        let foo = "bar";
        window.addEventListener("dummy", doSomething);
    }
}();

Using var works because it is scoped to the whole function, and disabling if_return also fixes it, but disabling hoist_funs does not appear to change the output code.

[kzc]

Most uglify compress and mangle optimizations predated ES6 and as such would be incorrect in ES6 code. Harmony users should disable compress and mangle.

As a stop-gap measure the harmony branch ought to scan the tree for let and other ES6 constructs and if found disable many of the optimizations.

The same scoping issues apply to const. This will be trickier because it is quasi supported in uglify in ES5 mode but with the wrong scope semantics.

[avdg]

Tracked as https://github.com/avdg/UglifyJS2/projects/1#card-606543

[avdg]

Going to take a look into this issue.

[avdg]

There are no functions to be moved and es5 isn't aware of block scoping, so why disabling hoist_funs doesn't work makes sense. Especially when the if_return setting is able to solve all the things on it's own.

[avdg]

For now I might be forced to disable the optimization, even though the code can be transformed into something that doesn't use block variables or doesn't affect code context.

[avdg]

If statement and return statements can't have var/let/scope in their statements. Use cases are limited to code within statement bodies that have at least 1 non-return statements.

[avdg]

Runnable test case:

"use strict";
var foo = function(n, m) {
    function getResult() {
        return [n, m, bar];
    }
    if (n === m) {
        return;
    }
    let bar = n % m;
    return getResult();
}

foo(0, 0);
foo(1, 0);
foo(0, 1);

[avdg]

Related to the if_return, I'm not sure why we have the case at https://github.com/mishoo/UglifyJS2/blob/48284844a461e6113bb9911cdcdad7ab8a3d85de/lib/compress.js#L609-L619

Disabling these lines results in code that seems slightly shorter

    Running test [if_return_7]
!!! failed
---INPUT---
function f(x) {
    if (x) {
        return true;
    }
    foo();
    bar();
}
---OUTPUT---
function f(x){if(x)return!0;foo(),bar()}
---EXPECTED---
function f(x){return!!x||(foo(),void bar())}

    Running test [issue979_test_negated_is_best]
!!! failed
---INPUT---
{
    function f3() {
        if (a == 1 | b == 2) {
            foo();
        }
    }
    function f4() {
        if (!(a == 1 | b == 2)) {} else {
            foo();
        }
    }
    function f5() {
        if (a == 1 && b == 2) {
            foo();
        }
    }
    function f6() {
        if (!(a == 1 && b == 2)) {} else {
            foo();
        }
    }
    function f7() {
        if (a == 1 || b == 2) {
            foo();
        } else {
            return bar();
        }
    }
}
---OUTPUT---
function f3(){1==a|2==b&&foo()}function f4(){1==a|2==b&&foo()}function f5(){1==a&&2==b&&foo()}function f6(){1!=a||2!=b||foo()}function f7(){if(1!=a&&2!=b)return bar();foo()}
---EXPECTED---
function f3(){1==a|2==b&&foo()}function f4(){1==a|2==b&&foo()}function f5(){1==a&&2==b&&foo()}function f6(){1!=a||2!=b||foo()}function f7(){return 1!=a&&2!=b?bar():void foo()}

[kzc]

Both cases deal with a situation where one branch returns a result and the other branch does not return anything (a.k.a. returns void). This is why the resultant code is longer when transformed into a return of a further optimized ternary operator with a void expression.

I'm not the author of the code but I can only surmise that the hope was there would be optimization opportunities later in some cases when statement chains were transformed into expressions. That's not true in those particular cases. Perhaps there exists a case where it is true?

[avdg]

I think I am going to remove this optimization case, as it adds undefined (or void something()) to the tail of the expression list of the alternative case and expressions following, and makes the expression longer than this.

[avdg]

@kzc yeah, I wanted to be careful with removing that optimisations, but there are no tests that prove this so far :/

[avdg]

Also @mishoo is the only author on that specific code and he did added that comment (a bit later) as well that he had no clue where it is for. The code was the result of a big refactoring on the if_return stuff, and actually was good for a big part as it does optimization backwards, to prevent transformation cycles (which is genius).

This is just a slight case that was probably overlooked I guess.

[avdg]

Going to remove that optimization on master soon.

[kzc]

No need for guesswork - run uglifyjs -m -c on dozen popular unminified libraries to see what the resultant code size would be with and without the code in question. If the result is smaller without that code, then drop it.

[kzc]

I qualified that statement with "unminified" because once transformed into expression sequence form uglify cannot revert them back to regular statements.

[avdg]

Patch on #1437. Feel free to battle out for a better solution.

[kzc]

Counter case working better with [email protected]: https://github.com/mishoo/UglifyJS2/pull/1437#issuecomment-275162526

[kzc]

@alexlamsl This bug is presently the biggest showstopper in uglify-es. Can be fixed by not hoisting functions with references to let and const variables outside of the function (and any child lambdas it may have). Affects the compress options if_return and hoist_funs.

[alexlamsl]

hoist_funs is fine even in the presence of block-scoped variables, as moving AST_Defun before AST_Let won't affect correctness of the program.

The previous fix for if_return defangs its overlap with hoist_funs, which fix half of the problem. What's left is a problem of the introduction of new block scope which is confusing ES6 constructs:

"use strict";
(function (a) {
    if (!a) return;
    let foo = "bar";
    function doSomething() {
        console.log(foo);
    };
    doSomething.call(null);
})(process);

This is currently optimised into, with hoist_funs=0:

"use strict";

!function(a) {
    if (a) {
        let foo = "bar";
        doSomething.call(null);
    }
    function doSomething() {
        console.log(foo);
    }
}(process);

So one might think we can just stuff all the trailing AST_Defun into the if-block. Unfortunately, if doSomething() is referenced by something before if(a){... say, then this will now break.

[alexlamsl]

May be extracting the block-scoped declarations and shove them back up?

"use strict";

!function(a) {
    let foo;
    if (a) {
        foo = "bar";
        doSomething.call(null);
    }
    function doSomething() {
        console.log(foo);
    }
}(process);

This will create larger code, especially if we get a bunch of these let and const.

[kzc]

hoist_funs is fine even in the presence of block-scoped variables, as moving AST_Defun before AST_Let won't affect correctness of the program.

That's not correct. Consider:

$ cat 1317.js 
!function() {
    if (!Math) return;
    let x = 1;
    function foo() {
        console.log(x);
    }
    foo();
    foo();
}();

$ cat 1317.js | node
1
1



md5-9b08acee94d2962f58a956be206b3a31



$ cat 1317.js | bin/uglifyjs -cb | node
        console.log(x);
                    ^

ReferenceError: x is not defined



md5-9b08acee94d2962f58a956be206b3a31



$ cat 1317.js | bin/uglifyjs -cb
!function() {
    function foo() {
        console.log(x);
    }
    if (Math) {
        let x = 1;
        foo(), foo();
    }
}();



md5-9b08acee94d2962f58a956be206b3a31



$ cat 1317.js | bin/uglifyjs -b -c hoist_funs=0,if_return=0 | node
1
1



md5-9b08acee94d2962f58a956be206b3a31



$ cat 1317.js | bin/uglifyjs -b -c hoist_funs=0,if_return=0
!function() {
    if (!Math) return;
    let x = 1;
    function foo() {
        console.log(x);
    }
    foo(), foo();
}();

Unlike var, let and const variables are temporal with respect to functions making use of them. (correction: @alexlamsl pointed out that only variable scope matters.)

By the way, it was difficult to create a test case because reduce_vars always optimized away things I did not want optimized away - which is why I used Math above. :-)

[alexlamsl]

The culprit of your example is if_return, not hoist_funs:

$ cat 1317.js | node
1
1

$ uglifyjs 1317.js -c if_return=0 | node
1
1

$ uglifyjs 1317.js -c if_return=1 | node
[stdin]:1
!function(){function foo(){console.log(x)}if(Math){let x=1;foo(),foo()}}();
                                       ^

ReferenceError: x is not defined
    at foo ([stdin]:1:40)

[alexlamsl]

$ uglifyjs 1317.js -c if_return=0 -b bracketize
!function() {
    function foo() {
        console.log(x);
    }
    if (!Math) {
        return;
    }
    let x = 1;
    foo(), foo();
}();

[kzc]

The culprit of your example is if_return, not hoist_funs

Fair enough - I wasn't terribly concerned which option was the problem. Default compress options produced an incorrect program, which was my main point.

[kzc]

So the problem is actually moving the let and const declarations into a different block where the function will not have visibility to them.

[alexlamsl]

That I agree - but in order to fix the issue I need to know where to change the code... :sweat_smile:

Anyway, in the process of looking around I've discovered these two blocks of code have overlapping functionality:
https://github.com/mishoo/UglifyJS2/blob/7b13159cda54b6e2dc5faef02c87ae87daa59237/lib/compress.js#L962-L978
https://github.com/mishoo/UglifyJS2/blob/7b13159cda54b6e2dc5faef02c87ae87daa59237/lib/compress.js#L998-L1018

So my fix in #2010 only applies to the first block, and if I inject false && the second block would take over and undo my fix. Great times.

[alexlamsl]

So the problem is actually moving the let and const declarations into a different block where the function will not have visibility to them.

Sorry for my rumblings in https://github.com/mishoo/UglifyJS2/issues/1317#issuecomment-304527021 where I was trying to point that out :sweat_smile:

[kzc]

Sorry for my rumblings in #1317 (comment) where I was trying to point that out

Not at all - if I'm wrong, I'm wrong. I just needed to write a program to better understand the issue.

[kzc]

You know what, this statement still holds true:

Can be fixed by not hoisting functions with references to let and const variables outside of the function (and any child lambdas it may have).

[alexlamsl]

You know what, this statement still holds true:

That's the route I'm taking, as it won't produce larger code under any circumstances.

But then I hit https://github.com/mishoo/UglifyJS2/issues/1317#issuecomment-304529592