Uglifyjs: [ES6] many bugs related to for-of loops

[eight04]

Bug report or feature request?

Bug report.

ES5 or ES6+ input?

ES6+

Uglify version (uglifyjs -V)

uglify-es 3.3.7

JavaScript input

function foo() {
    for (const a of func(value)) {}
    function func(va) {
        return doSomething(va);
    }
}

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

uglifyjs test.js -c

JavaScript output or error produced.

function foo(){for(const a of va=value,doSomething(va));var va}

The error:

(function (exports, require, module, __filename, __dirname) { function foo(){for
(const a of va=value,doSomething(va));var va}

                    ^

SyntaxError: Unexpected token ,
    at createScript (vm.js:80:10)
    at Object.runInThisContext (vm.js:152:10)
    at Module._compile (module.js:624:28)
    at Object.Module._extensions..js (module.js:671:10)
    at Module.load (module.js:573:32)
    at tryModuleLoad (module.js:513:12)
    at Function.Module._load (module.js:505:3)
    at Function.Module.runMain (module.js:701:10)
    at startup (bootstrap_node.js:194:16)
    at bootstrap_node.js:618:3

The result is weirder with mangle option:

uglifyjs test.js -c -m

function foo(){for(const o of o(value));}

[kzc]

Last working version for compress and mangle was [email protected].

This issue is chock-full-o-bugs.

First test case - errors in compress and mangle

$ cat t2794.js
function foo() {
    for (const a of func(value)) {
        console.log(a);
    }
    function func(va) {
        return doSomething(va);
    }
}
function doSomething(x) {
    return [ x, 2 * x, 3 * x ];
}
const value = 10;
foo();

Expected:

$ cat t2794.js | node
10
20
30

Actual:

$ cat t2794.js | bin/uglifyjs -bc | node
[stdin]:2
    for (const a of va = value, doSomething(va)) console.log(a);
                              ^
SyntaxError: Unexpected token ,

$ cat t2794.js | bin/uglifyjs -bcm | node
    for (const o of o(value)) console.log(o);
                    ^
ReferenceError: o is not defined



md5-ac7164be691105bf8f3dfb62340aadef



**Second test case - `for-of` parse error not detected by uglify**

Expected:



md5-a38ba6512ae6ea80d658abae86239bf0



Actual:



md5-67703bc0e75f467faf55830714bb8666



Curiously, a paren-less sequence in a `for`-`in` loop is valid for both `node` and `uglify`:



md5-eec0ffba0006b1955253fb29c54e7b7d

$ echo 'for (var x in [1, 2], [3, 4, 5]) console.log(x);' | bin/uglifyjs -b
for (var x in [ 1, 2 ], [ 3, 4, 5 ]) console.log(x);

$ echo 'for (var x in ([1, 2], [3, 4, 5])) console.log(x);' | bin/uglifyjs -b
for (var x in [ 1, 2 ], [ 3, 4, 5 ]) console.log(x);

$ echo 'for (var x in [1, 2], [3, 4, 5]) console.log(x);' | bin/uglifyjs | node
0
1
2

**Third test case - output of `for-of` loop lacks parentheses around sequence**

Expected:
```js
$ echo 'for (var x of ([1, 2], [3, 4])) console.log(x);' | node
3
4

Actual:

$ echo 'for (var x of ([1, 2], [3, 4])) console.log(x);' | bin/uglifyjs -b | node
[stdin]:1
for (var x of [ 1, 2 ], [ 3, 4 ]) console.log(x);
                      ^
SyntaxError: Unexpected token ,

[kzc]

Could this issue be retitled?

[ES6] many bugs related to for-of loops

[alexlamsl]

@kzc just to confirm - nothing here affects master?

[kzc]

for-in loops on master appear to work correctly.

[kzc]

The for-of compress bug is only apparent when -c inline >= 2:

$ cat t2794.js | bin/uglifyjs -bc inline=1 | node
10
20
30

$ cat t2794.js | bin/uglifyjs -bc inline=2 | node
    for (const a of va = value, doSomething(va)) console.log(a);
                              ^
SyntaxError: Unexpected token ,



md5-3fd9dbee0bbb578163ca692de4686269

$ cat t2794c.js | node
10
20
30

`mangle` appears to be a different issue however:

$ cat t2794.js | bin/uglifyjs -mc inline=1 | node
function foo(){for(const o of o(value))console.log(o)}function doSomething(o){return[o,2o,3o]}const value=10;foo();
^
ReferenceError: o is not defined
```

[kzc]

uglify-es 3.3.7 workaround for the compress and mangle bugs in the top post:

$ cat t2794.js | bin/uglifyjs -bmc inline=1 --no-rename | node
10
20
30

```js
$ cat t2794.js | bin/uglifyjs -bmc inline=1 --no-rename
function foo() {
for (const o of function(o) {
return doSomething(o);
}(value)) console.log(o);
}

function doSomething(o) {
return [ o, 2 * o, 3 * o ];
}

const value = 10;

foo();

It also works with higher levels of optimization:

$ cat t2794.js | bin/uglifyjs -bmc inline=1,passes=2,toplevel --no-rename | node
10
20
30

```js
$ cat t2794.js | bin/uglifyjs -bmc inline=1,passes=2,toplevel --no-rename
!function() {
    for (const o of [ 10, 20, 30 ]) console.log(o);
}();

[kzc]

@alexlamsl Is there an issue with rename and for-of?

Ignore the fact that parens are not emitted around sequences which is easily fixed...

$ cat t2794.js | bin/uglifyjs -bmc
function foo() {
    for (const o of o(value)) console.log(o);
}

function doSomething(o) {
    return [ o, 2 * o, 3 * o ];
}

const value = 10;

foo();

$ cat t2794.js | bin/uglifyjs --no-rename -bmc
function foo() {
    for (const n of o = value, doSomething(o)) console.log(n);
    var o;
}

function doSomething(o) {
    return [ o, 2 * o, 3 * o ];
}

const value = 10;

foo();

[alexlamsl]

Is there an issue with rename and for-of?

rename gives globally unique names to each AST_SymbolDef, so I can't see anything that would specifically target AST_ForOf.

[kzc]

Hacking rename=true in bin/uglify:

--- a/bin/uglifyjs
+++ b/bin/uglifyjs
@@ -206,6 +206,7 @@ function run() {
     } catch (ex) {
         fatal(ex);
     }
+options.rename = true;
     var result = UglifyJS.minify(files, options);
     if (result.error) {
         var ex = result.error;

verifies that there's a bug in rename:

$ cat t2794.js | bin/uglifyjs -b 
function foo() {
    for (const a of a(value)) {
        console.log(a);
    }
    function a(b) {
        return doSomething(b);
    }
}

function doSomething(c) {
    return [ c, 2 * c, 3 * c ];
}

const value = 10;

foo();

$ cat t2794.js | bin/uglifyjs -b | node
    for (const a of a(value)) {
                    ^
ReferenceError: a is not defined

[alexlamsl]

What you are most likely seeing here is the prevention of inline due to namespace collision, which rename is designed to resolve.

[kzc]

See test case above.

Can we please get the ability to enable rename in bin/uglifyjs? It would make debugging easier at the very least.

[alexlamsl]

Oh I see - block-scoped variable strikes, which cannot be scanned thus renamed by rename.

I'd just disable rename on harmony instead of worrying about it.

[alexlamsl]

Can we please get the ability to enable rename in bin/uglifyjs? It would make debugging easier at the very least.

If you can find a way of getting CLI to behave correctly, be my guest. I can't get it to work properly when I tried the straightforward case of introducing --rename.

[kzc]

I'd just disable rename on harmony instead of worrying about it.

Yeah, it'd be safer to have harmony minify() ignore the rename option altogether. There could be other undiscovered ES6 scope issues with rename.

One downside is that the harmony tests would diverge from master if that was done.

Would the minify code quality be suboptimal in some cases compared to master?

If you can find a way of getting CLI to behave correctly, be my guest. I can't get it to work properly when I tried the straightforward case of introducing --rename.

Okay, I'll take a look at it later.

[alexlamsl]

Would the minify code quality be suboptimal in some cases compared to master?

The compression ratio would suffer, mainly due to lost opportunities by inline - obviously that has knock-on effects. But I'd prefer lower maintenance overhead than quality of ES6+ at this point.

Okay, I'll take a look at it later.

Many thanks :+1:

[kzc]

Oh I see - block-scoped variable strikes, which cannot be scanned thus renamed by rename.

Is there a technical obstacle to getting rename to work properly in harmony with block scopes or you'd simply rather not support it? Do you think other ES6 scope problems exist in rename?

[alexlamsl]

There is no theoretical obstacle to getting rename to work on harmony - I simply have had enough of ES6 "features" for the time being.

If you'd like to get it to work, the recent fix for mangle_names() may contain helpful pointers.

[kzc]

That's cool. I'll just disable rename on harmony, and fix for-in parse and output.

[alexlamsl]

Fun fact - AST_ForIn.object does not reside within the block scope:

$ echo 'for(var a in console.log(a));' | node
undefined

$ echo 'for(let a in console.log(a));' | node
[stdin]:1
for(let a in console.log(a));
                         ^

ReferenceError: a is not defined
    at [stdin]:1:26

Run into this as I'm thinking about making sequences hide stuff inside AST_ForIn.object like we do with AST_For.init

[kzc]

@alexlamsl This CLI patch serves my purposes to enable rename without compress nor mangle. Could you please incorporate it into master?

--- a/bin/uglifyjs
+++ b/bin/uglifyjs
@@ -45,7 +45,8 @@ program.option("--ie8", "Support non-standard Internet Explorer 8.");
 program.option("--keep-classnames", "Do not mangle/drop class names.");
 program.option("--keep-fnames", "Do not mangle/drop function names. Useful for code relying on Function.prototype.name.");
 program.option("--name-cache <file>", "File to hold mangled name mappings.");
-program.option("--no-rename", "Disable symbol expansion.");
+program.option("--force-rename", "Rename symbols for debugging.");
+program.option("--no-rename", "Do not rename symbols prior to `compress` + `mangle`.");
 program.option("--safari10", "Support non-standard Safari 10.");
 program.option("--self", "Build UglifyJS as a library (implies --wrap UglifyJS)");
 program.option("--source-map [options]", "Enable source map/specify source map options.", parse_source_map());
@@ -76,6 +77,9 @@ if (!program.output && program.sourceMap && program.sourceMap.url != "inline") {
         options[name] = program[name];
     }
 });
+if (program.forceRename) {
+    options.rename = true;
+}
 if ("ecma" in program) {
     if (program.ecma != (program.ecma | 0)) fatal("ERROR: ecma must be an integer");
     options.ecma = program.ecma | 0;

[kzc]

This patch incorporates --rename using an undocumented commander property:

diff --git a/bin/uglifyjs b/bin/uglifyjs
index 26fd8ef..9a28cd7 100755
--- a/bin/uglifyjs
+++ b/bin/uglifyjs
@@ -45,6 +45,7 @@ program.option("--ie8", "Support non-standard Internet Explorer 8.");
 program.option("--keep-classnames", "Do not mangle/drop class names.");
 program.option("--keep-fnames", "Do not mangle/drop function names. Useful for code relying on Function.prototype.name.");
 program.option("--name-cache <file>", "File to hold mangled name mappings.");
+program.option("--rename", "Force symbol expansion.");
 program.option("--no-rename", "Disable symbol expansion.");
 program.option("--safari10", "Support non-standard Safari 10.");
 program.option("--self", "Build UglifyJS as a library (implies --wrap UglifyJS)");
@@ -65,17 +66,21 @@ if (!program.output && program.sourceMap && program.sourceMap.url != "inline") {
     "compress",
     "ie8",
     "mangle",
-    "rename",
     "safari10",
     "sourceMap",
     "toplevel",
     "wrap"
 ].forEach(function(name) {
     if (name in program) {
-        if (name == "rename" && program[name]) return;
         options[name] = program[name];
     }
 });
+if ("rename" in program) {
+    if (program.rawArgs && program.rawArgs.indexOf("--rename") >= 0)
+        options.rename = true;
+    else if (!program.rename)
+        options.rename = false;
+}
 if ("ecma" in program) {
     if (program.ecma != (program.ecma | 0)) fatal("ERROR: ecma must be an integer");
     options.ecma = program.ecma | 0;
@@ -206,6 +211,7 @@ function run() {
     } catch (ex) {
         fatal(ex);
     }
+    console.error("*** minify options:", options); // FOR DEBUGGING ONLY
     var result = UglifyJS.minify(files, options);
     if (result.error) {
         var ex = result.error;

$ echo "" | bin/uglifyjs
*** minify options: { compress: false, mangle: false }

$ echo "" | bin/uglifyjs --no-rename
*** minify options: { compress: false, mangle: false, rename: false }

$ echo "" | bin/uglifyjs --rename
*** minify options: { compress: false, mangle: false, rename: true }