Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ES6] Function hoisting + if-return optimisation breaks let scope #1317

Closed
AshleyScirra opened this issue Oct 5, 2016 · 30 comments
Closed

Comments

@AshleyScirra
Copy link
Contributor

AshleyScirra commented Oct 5, 2016

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
Copy link
Contributor

kzc commented Oct 5, 2016

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
Copy link
Contributor

avdg commented Oct 27, 2016

@avdg
Copy link
Contributor

avdg commented Oct 31, 2016

Going to take a look into this issue.

@avdg
Copy link
Contributor

avdg commented Oct 31, 2016

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
Copy link
Contributor

avdg commented Oct 31, 2016

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
Copy link
Contributor

avdg commented Oct 31, 2016

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
Copy link
Contributor

avdg commented Jan 24, 2017

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
Copy link
Contributor

avdg commented Jan 25, 2017

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
Copy link
Contributor

kzc commented Jan 25, 2017

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
Copy link
Contributor

avdg commented Jan 25, 2017

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
Copy link
Contributor

avdg commented Jan 25, 2017

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

@avdg
Copy link
Contributor

avdg commented Jan 25, 2017

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
Copy link
Contributor

avdg commented Jan 25, 2017

Going to remove that optimization on master soon.

@kzc
Copy link
Contributor

kzc commented Jan 25, 2017

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
Copy link
Contributor

kzc commented Jan 25, 2017

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

@avdg
Copy link
Contributor

avdg commented Jan 25, 2017

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

@kzc
Copy link
Contributor

kzc commented Jan 25, 2017

Counter case working better with uglify-js@2.7.5: #1437 (comment)

@kzc
Copy link
Contributor

kzc commented May 28, 2017

@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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Contributor

kzc commented May 28, 2017

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
$ cat 1317.js | bin/uglifyjs -cb | node
[stdin]:3
        console.log(x);
                    ^

ReferenceError: x is not defined
$ cat 1317.js | bin/uglifyjs -cb
!function() {
    function foo() {
        console.log(x);
    }
    if (Math) {
        let x = 1;
        foo(), foo();
    }
}();
$ cat 1317.js | bin/uglifyjs -b -c hoist_funs=0,if_return=0 | node
1
1
$ 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
Copy link
Collaborator

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
Copy link
Collaborator

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

@kzc
Copy link
Contributor

kzc commented May 28, 2017

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
Copy link
Contributor

kzc commented May 28, 2017

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
Copy link
Collaborator

alexlamsl commented May 28, 2017

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

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
Copy link
Collaborator

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 #1317 (comment) where I was trying to point that out 😅

@kzc
Copy link
Contributor

kzc commented May 28, 2017

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
Copy link
Contributor

kzc commented May 28, 2017

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
Copy link
Collaborator

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 #1317 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants