Uglifyjs: `let` bug in `reduce_vars`

[kzc]

Bug report or feature request?

bug

ES5 or ES6+ input?

ES6

Uglify version (uglifyjs -V)

uglify-es 3.3.5

JavaScript input

Bug found running npm test on uglified [email protected] source code before they switched from ES6 to TypeScript:

$ curl -s https://raw.githubusercontent.com/rollup/rollup/v0.53.0/src/utils/transform.js > transform.js
$ bin/uglifyjs transform.js -bc reduce_vars=0 > 0.js
$ bin/uglifyjs transform.js -bc reduce_vars=1 > 1.js
$ diff -u 0.js 1.js
--- 0.js    2018-01-09 19:27:23.000000000 -0500
+++ 1.js    2018-01-09 19:27:34.000000000 -0500
@@ -49,7 +49,7 @@
             try {
                 transformed = plugin.transform.call(context, previous, id);
             } catch (err) {
-                throwing || context.error(err), error(err);
+                0, error(err);
             }
             return Promise.resolve(transformed).then(result => null == result ? previous : ("string" == typeof result ? result = {
                 code: result,

Reduced test case:

$ cat let_reduce_vars.js 
(function() {
    let bar;
    const unused = function() {
        bar = true;
    };
    if (!bar) {
        console.log(1);
    }
    console.log(2);
}());

$ cat let_reduce_vars.js | node
1
2



md5-1d72dae677dd8ee6f450369dd428b9b5



$ cat let_reduce_vars.js | bin/uglifyjs -bc reduce_vars=0 | node
1
2



md5-1d72dae677dd8ee6f450369dd428b9b5



$ cat let_reduce_vars.js | bin/uglifyjs -bc reduce_vars=1 | node
2



md5-1d72dae677dd8ee6f450369dd428b9b5



$ cat let_reduce_vars.js | bin/uglifyjs -bc reduce_vars=1
console.log(2);

[kzc]

Worked correctly in last release:

$ bin/uglifyjs -V
uglify-es 3.3.4

```
$ cat let_reduce_vars.js | bin/uglifyjs -bc | node
1
2

```js
$ cat let_reduce_vars.js | bin/uglifyjs -bc
!function() {
    let bar;
    bar || console.log(1), console.log(2);
}();

[kzc]

related issue: #2747

[kzc]

Noticed if the let variable is initialized then the bug goes away:

$ bin/uglifyjs -V
uglify-es 3.3.5

$ cat let_reduce_vars_2.js 
(function() {
    let bar = void 0;
    const unused = function() {
        bar = true;
    };
    if (!bar) {
        console.log(1);
    }
    console.log(2);
}());



md5-1d72dae677dd8ee6f450369dd428b9b5



$ cat let_reduce_vars_2.js | bin/uglifyjs -bc | node
1
2

[kzc]

@alexlamsl This hack will make your skin crawl. It fixes both this issue and #2747 and passes all tests.

diff --git a/lib/ast.js b/lib/ast.js
index 7407bac..dc958a6 100644
--- a/lib/ast.js
+++ b/lib/ast.js
@@ -1086,8 +1086,11 @@ var AST_NaN = DEFNODE("NaN", null, {
     value: 0/0
 }, AST_Atom);

-var AST_Undefined = DEFNODE("Undefined", null, {
+var AST_Undefined = DEFNODE("Undefined", "synthetic", {
     $documentation: "The `undefined` value",
+    $propdoc: {
+        synthetic: "[Boolean] is this a synthetic `undefined` value",
+    },
     value: (function(){}())
 }, AST_Atom);

diff --git a/lib/compress.js b/lib/compress.js
index e80d7a9..2ad784a 100644
--- a/lib/compress.js
+++ b/lib/compress.js
@@ -5325,6 +5325,7 @@ merge(Compressor.prototype, {
     }

     OPT(AST_Undefined, function(self, compressor){
+        if (self.synthetic) return self;
         if (compressor.option("unsafe")) {
             var undef = find_variable(compressor, "undefined");
             if (undef) {
diff --git a/lib/output.js b/lib/output.js
index b2353a4..dc86920 100644
--- a/lib/output.js
+++ b/lib/output.js
@@ -1474,7 +1474,7 @@ function OutputStream(options) {

     DEFPRINT(AST_VarDef, function(self, output){
         self.name.print(output);
-        if (self.value) {
+        if (self.value && !self.value.synthetic) {
             output.space();
             output.print("=");
             output.space();
diff --git a/lib/parse.js b/lib/parse.js
index 9387ad9..67a88d7 100644
--- a/lib/parse.js
+++ b/lib/parse.js
@@ -1863,7 +1863,8 @@ function parse($TEXT, options) {
                     value : is("operator", "=")
                         ? (next(), expression(false, no_in))
                         : !no_in && kind === "const"
-                            ? croak("Missing initializer in const declaration") : null,
+                            ? croak("Missing initializer in const declaration")
+                            : kind == "let" ? new AST_Undefined({synthetic: true}) : null,
                     end   : prev()
                 });
                 if (def.name.name == "import") croak("Unexpected token: import");

```js
$ cat let_reduce_vars.js | bin/uglifyjs -bc passes=1
!function() {
let bar;
bar || console.log(1), console.log(2);
}();

```js
$ cat let_reduce_vars.js | bin/uglifyjs -bc passes=2
console.log(1), console.log(2);

I welcome a better fix.

[alexlamsl]

Interesting concept - you may be able to achieve the same effects with SymbolDef.init = null:

--- a/lib/scope.js
+++ b/lib/scope.js
@@ -202,7 +202,7 @@ AST_Toplevel.DEFMETHOD("figure_out_scope", function(options){
             || node instanceof AST_SymbolConst) {
             var def;
             if (node instanceof AST_SymbolBlockDeclaration) {
-                def = scope.def_variable(node);
+                def = scope.def_variable(node, null);
             } else {
                 def = defun.def_variable(node, node.TYPE == "SymbolVar" ? null : undefined);
             }

[kzc]

Your proposed patch fixes this issue:

$ cat let_reduce_vars.js | bin/uglifyjs -bc passes=2
console.log(1), console.log(2);

but not #2747:

$ cat t2747b.js | bin/uglifyjs -bc passes=3 | node
[stdin]:4
    return 0 === baz ? null : r = baz > 2 ? 4 : 5;
                                ^
ReferenceError: r is not defined

[alexlamsl]

Thanks for validation - I'll put that one-liner & your test case into a new PR in a moment.

[kzc]

@alexlamsl I'll leave these issues with you.

The uninitialized lets seen in both these issues are rather common in ES6 code. Rollup is a case in point.

[alexlamsl]

The combo of PRs are up - let's hope Travis NAT fixes lead to CI testing of this project instead of test-compiling node from scratch...

[alexlamsl]

@kzc thanks a lot for https://github.com/mishoo/UglifyJS2/issues/2757#issuecomment-356529719 - it gives vital clues towards the root cause.

[kzc]

The combination of PRs #2759 and #2760 allow Rollup to pass "npm test" after its ES6 sources were uglified.

[kzc]

@alexlamsl Why did changing the init value from undefined to null in this scope patch work anyway?

-                def = scope.def_variable(node);
+                def = scope.def_variable(node, null);

Where is SymbolDef.init used in a non-boolean context where the undefined versus null distinction would make a difference?

[alexlamsl]

SymbolDef.init is used to prime SymbolDef.fixed:
https://github.com/mishoo/UglifyJS2/blob/6a0af85c8bf8673498e89ad7226d6e8cc80b2c15/lib/compress.js#L319-L320

Setting it to null signals the variable is safe for use without initialisation:
https://github.com/mishoo/UglifyJS2/blob/6a0af85c8bf8673498e89ad7226d6e8cc80b2c15/lib/compress.js#L625-L626

Upon first read, it will materialise into AST_Undefined:
https://github.com/mishoo/UglifyJS2/blob/6a0af85c8bf8673498e89ad7226d6e8cc80b2c15/lib/compress.js#L357-L361

[kzc]

Ok, I see. Thanks.