Uglifyjs: `if_return` incorrectly drops block-scope variable declarations

[peterkooijmans]

Bug report or feature request?

Bug report

ES5 or ES6+ input?

ES6

Uglify version (uglifyjs -V)

uglify-es 3.3.5

JavaScript input

class foo {
    bar(baz) {
        if (baz === 0) {
            return null;
        }

        let r;
        if (baz > 2) {
            r = 4;
        } else {
            r = 5;
        }

        return r;
    }
}

new foo().bar(1);

The uglifyjs CLI command executed or minify() options used.

uglifyjs -c -b -o output.js input.js

JavaScript output or error produced.

class foo {
    bar(baz) {
        return 0 === baz ? null : r = baz > 2 ? 4 : 5;
    }
}

new foo().bar(1);

Executing this code results in a reference error, because r is used but never declared.

[email protected] gives the following, correct, output:

class foo {
    bar(baz) {
        if (0 === baz) return null;
        let r;
        return r = baz > 2 ? 4 : 5;
    }
}

new foo().bar(1);

[kzc]

Temporary workaround - disable the compress option if_return:

$ bin/uglifyjs t2747.js -bc if_return=false
class foo {
    bar(baz) {
        if (0 === baz) return null;
        let r;
        return r = baz > 2 ? 4 : 5;
    }
}

new foo().bar(1);

[alexlamsl]

Confirmed that this only affects harmony - works if it was var r.

[kzc]

Bug is not specific to class members:

$ cat t2747b.js
"use strict";
function f(baz) {
    if (baz === 0) {
        return null;
    }
    let r;
    if (baz > 2) {
        r = 4;
    } else {
        r = 5;
    }
    return r;
}
console.log(f(0), f(1), f(3));

```
$ cat t2747b.js | node
null 5 4

```js
$ cat t2747b.js | bin/uglifyjs -c | node
[stdin]:1
"use strict";function f(baz){return 0===baz?null:r=baz>2?4:5}console.log(f(0),f(1),f(3));
                                                  ^
ReferenceError: r is not defined

[alexlamsl]

In case there is any doubt unused may be hiding a potential bug on master:

"use strict";
function f(baz) {
    if (baz === 0) {
        return null;
    }
    var r;
    if (baz > 2) {
        r = 4;
    } else {
        r = 5;
    }
    return Math.pow(r, r);
}
console.log(f(0), f(1), f(3));

$ cat test.js | node
null 3125 256

$ uglifyjs test.js -bc | node
null 3125 256

$ uglifyjs test.js -bc
"use strict";

function f(baz) {
    return 0 === baz ? null : (r = baz > 2 ? 4 : 5, Math.pow(r, r));
    var r;
}

console.log(f(0), f(1), f(3));

[kzc]

I don't follow - output is null 3125 256 in both cases above.

[alexlamsl]

Hence master is behaving correctly - this is a test case of success rather than failure :sweat_smile:

[kzc]

@alexlamsl Can you give this issue a suitable title? Bug in if_return related to let scoping?

[kzc]

Unlike var, the let declaration cannot follow its use...

$ cat t2747b.js
"use strict";
function f(baz) {
    if (baz === 0) {
        return null;
    }
    let r;
    if (baz > 2) {
        r = 4;
    } else {
        r = 5;
    }
    return r;
}
console.log(f(0), f(1), f(3));

$ cat t2747b.js | bin/uglifyjs -bc dead_code=0
"use strict";

function f(baz) {
    return 0 === baz ? null : r = baz > 2 ? 4 : 5;
    let r;
}

console.log(f(0), f(1), f(3));



md5-6155b77b992b1c1ec27d364063e877b1



If `if_return` were to generate the `let` declaration prior to use, it'd work fine with the rest of the `compress` transforms:



md5-4c33c4e8a22252e5822ce6a3386d80c5

$ cat let.js | bin/uglifyjs -bc passes=9 | node
null 5 4
```

[alexlamsl]

I think if you attempt to move that let and the statement above uses r somehow, then we will be breaking GIGO. Probably easier just to bail on any block-scoped declarations.

[kzc]

I agree that the let shouldn't be moved after the fact. It shouldn't have been dropped from its original location in the first place.

[kzc]

What I don't understand is why the assignment to r was retained when r is known to be a write-only variable.

[alexlamsl]

What I don't understand is why the assignment to r was retained when r is known to be a write-only variable.

Because unused assignment doesn't get optimised for block-scoped variables:
https://github.com/mishoo/UglifyJS2/blob/4b1799ecddbcb4994346b6f03b0fcaf7118ab8f2/lib/compress.js#L3194

[alexlamsl]

... which is a fix introduced by #1969

[kzc]

Both this issue and that PR's test case appears to work when the condition above is removed. Can you provide an ES6 test case that would generate bad code without that condition?

[alexlamsl]

Well, it didn't work before, so the rewrite to that keep_assign logic on master must have improved enough to render it redundant.

Feel free to remove it and see what breaks in the wild.

[kzc]

Feel free to remove it and see what breaks in the wild.

:-)

That's what I'm afraid of. If we had an ES6 fuzzer or a large ES6 test suite we could revisit that.

The other option you mentioned to "bail on any block-scoped declarations" might be the safer way to go.

[kzc]

Noticed that for this issue as well as #2757 if the let is initialized, then the bug disappears:

$ cat t2747c.js
"use strict";
function f(baz) {
    if (baz === 0) {
        return null;
    }
    let r = void 0;
    if (baz > 2) {
        r = 4;
    } else {
        r = 5;
    }
    return r;
}
console.log(f(0), f(1), f(3));

$ cat t2747c.js | bin/uglifyjs -bc
"use strict";

function f(baz) {
    if (0 === baz) return null;
    let r = void 0;
    return r = baz > 2 ? 4 : 5;
}

console.log(f(0), f(1), f(3));



md5-de875bbc621502b4c704f9e25b92074f



$ cat t2747c.js | bin/uglifyjs -bc | node
null 5 4

[kzc]

Issue fixed with hack from https://github.com/mishoo/UglifyJS2/issues/2757#issuecomment-356529719:

$ cat t2747b.js
"use strict";
function f(baz) {
    if (baz === 0) {
        return null;
    }
    let r;
    if (baz > 2) {
        r = 4;
    } else {
        r = 5;
    }
    return r;
}
console.log(f(0), f(1), f(3));

```
$ cat t2747b.js | node
null 5 4

$ cat t2747b.js | bin/uglifyjs -bc passes=3 | node
null 5 4

```js
$ cat t2747b.js | bin/uglifyjs -bc passes=3
"use strict";

function f(baz) {
    return 0 === baz ? null : baz > 2 ? 4 : 5;
}

console.log(f(0), f(1), f(3));