Uglifyjs: [ES6] Compress emits invalid code in ForIn and ForOf loops with destructuring

[ckm2k1]

Input:

var k = '';
for (k in {prop: 'val'}) {}

Output:

for(""in{prop:"val"});

This results in a syntax error. The issue was present on the Harmony branch, and after issue #1533 got merged, this is now an issue in master as well.

I've spent some time debugging the issue, and traced it down to the collapse_single_use_vars
pass. Specifically this snippet:

// Only interested in cases with just one reference to the variable.
var def = self.find_variable && self.find_variable(var_name);
if (!def || !def.references || def.references.length !== 1 || var_name == "arguments") {
    side_effects_encountered = true;
    continue;
}
var ref = def.references[0];

// Don't replace ref if eval() or with statement in scope.
if (ref.scope.uses_eval || ref.scope.uses_with) break;

// Constant single use vars can be replaced in any scope.
if (var_decl.value.is_constant()) {
    var ctt = new TreeTransformer(function(node) {
        if (node === ref)
            return replace_var(node, ctt.parent(), true);
    });
    stat.transform(ctt);
    continue;
}

Before #1533, reference marking wasn't working correctly for SymbolRef in a ForIn node, which meant that if (!def || !def.references || def.references.length !== 1 || var_name == "arguments") { would result in true and exit the optimization. Since the marking now works correctly, the compressor replaces the k variable in the ForIn node with it's constant value ("") which results in invalid JS output.

This is also a problem for ForOf nodes, and is reproduced even with destructuring statements of the form for ([k] of [[1], [3]]) {}

Hope this helps!

[kzc]

This affects ES5 as well. Could you please remove [ES6] from the Issue title?

$ echo "var k = ''; for (k in {prop: 'val'}) {}" | uglifyjs -c collapse_vars=true
WARN: Replacing constant k [-:1,17]
for(""in{prop:"val"});

As a workaround you can disable collapse_vars until this issue is fixed.

$ echo "var k = ''; for (k in {prop: 'val'}) {}" | uglifyjs -c collapse_vars=false
var k="";for(k in{prop:"val"});

[alexlamsl]

I believe this is the same as #1533 and fix is at #1535

[kzc]

@alexlamsl This is a new collapse_vars bug with for-in.

[alexlamsl]

@kzc reduce_vars caused collapse_vars to fail, see https://github.com/mishoo/UglifyJS2/pull/1535/files#diff-25caad7de41e394d6c7ba323e8ec5a6dR595

[kzc]

Latest master:

$ echo "var k = ''; for (k in {prop: 'val'}) {}" | bin/uglifyjs -c
WARN: Replacing constant k [-:1,17]
for(""in{prop:"val"});

[alexlamsl]

I stand corrected. This one is surprising:

$ echo "var k = ''; for (k in {prop: 'val'}) {}" | bin/uglifyjs -c reduce_vars=false
WARN: Replacing constant k [-:1,17]
for(""in{prop:"val"});

[kzc]

I think this fixes the for-in case for collapse_vars=true on master:

--- a/lib/compress.js
+++ b/lib/compress.js
@@ -480,8 +480,11 @@ merge(Compressor.prototype, {
                     // Constant single use vars can be replaced in any scope.
                     if (var_decl.value.is_constant()) {
                         var ctt = new TreeTransformer(function(node) {
-                            if (node === ref)
-                                return replace_var(node, ctt.parent(), true);
+                            var parent = ctt.parent();
+                            if (node === ref
+                                && !(parent instanceof AST_ForIn && parent.init === node)) {
+                                return replace_var(node, parent, true);
+                            }
                         });
                         stat.transform(ctt);
                         continue;

[alexlamsl]

@kzc any idea why this isn't breaking on 2.7.5?

[ckm2k1]

@alexlamsl
As i wrote in the issue description, the fix you introduced in #1533 made reference marking work correctly, which exposed this bug. You can checkout 2.7.5 and apply the patch from #1533 and you'll get the same issue as here.

[alexlamsl]

@ckm2k1 Ah, that makes sense - thanks!

[kzc]

any idea why this isn't breaking on 2.7.5?

It is breaking on 2.7.5 as well:

$ uglifyjs -V
uglify-js 2.7.5

$ echo "var k = ''; for (k in {prop: 'val'}) {}" | uglifyjs -c collapse_vars=true,reduce_vars=false
WARN: Replacing constant k [-:1,17]
for(""in{prop:"val"});

PR shortly.

[alexlamsl]

@kzc since you are picking this up - this is the test I was adding to test/compress/collapse_vars.js:

issue_1537: {
    options = {
        collapse_vars: true,
    }
    input: {
        var k = "";
        for (k in {prop: "val"});
    }
    expect: {
        var k = "";
        for (k in {prop: "val"});
    }
}

[kzc]

@alexlamsl Thanks. I have a similar test.

[kzc]

This bug was not discovered earlier because the for-in loop variable was not used in the loop body, which is uncommon.

Notice how this works:

$ uglifyjs -V
uglify-js 2.7.5

$ echo "var k = ''; for (k in {prop: 'val'}) console.log(k);" | uglifyjs -c collapse_vars=true
var k="";for(k in{prop:"val"})console.log(k);

[kzc]

1538 fixes for-in loops on master.

Another PR will be needed for for-of loops on harmony.

[kzc]

As luck would have it, #1538 will fix harmony for-of loops as well:

var AST_ForOf = DEFNODE("ForOf", null, {
    $documentation: "A `for ... of` statement",
}, AST_ForIn);

because AST_ForOf is derived from AST_ForIn.

[kzc]

@alexlamsl I spoke too soon about harmony/ES6 for-(in|of) loop init sections being okay...

this works:

$ echo 'var x = 1; for (x of a);' | bin/uglifyjs -c
var x=1;for(x of a);

but this does not:

$ echo 'var x = 1; for ([[x]] of a);' | bin/uglifyjs -c
WARN: Collapsing constant x [-:1,18]
for([[1]]of a);

Because destructuring can be of arbitrary depth would have to check if AST_ForIn.init is not the parent of the constant variable to replace.

[kzc]

@alexlamsl Is there a utility function along the lines of AST_Node.has_parent(node) or AST_Node.has_child(node)? Seems like a waste to make a tree walker every time this is needed.

[alexlamsl]

compressor.find_parent(AST_NodeType) for types. But for a AST_Node instance I think you'll need to go up via compress.parent(level) to find a match.

[kzc]

@alexlamsl I found a much simpler solution - just disallow collapse_var constant replacement in AST_ForIn statements. reduce_vars will pick up the slack.

--- a/lib/compress.js
+++ b/lib/compress.js
@@ -492,13 +492,10 @@ merge(Compressor.prototype, {
                     if (ref.scope.uses_eval || ref.scope.uses_with) break;

                     // Constant single use vars can be replaced in any scope.
-                    if (var_decl.value.is_constant()) {
+                    if (!(stat instanceof AST_ForIn) && var_decl.value.is_constant()) {
                         var ctt = new TreeTransformer(function(node) {
                             if (node === ref) {
-                                var parent = ctt.parent();
-                                if (!(parent instanceof AST_ForIn && parent.init === node)) {
-                                    return replace_var(node, parent, true);
-                                }
+                                return replace_var(node, ctt.parent(), true);
                             }
                         });
                         stat.transform(ctt);

with this patch applied to harmony it produces correct code:

$ echo 'var x = 1, y = 2; for ([[x],y] of a);' | bin/uglifyjs -c
var x=1,y=2;for([[x],y]of a);

$ echo 'var x = 1; for ([[x]] of a);' | bin/uglifyjs -c
var x=1;for([[x]]of a);

Will make a PR for master shortly to be (eventually) applied to harmony.

[kzc]

compressor.find_parent(AST_NodeType) for types

@alexlamsl That's a good suggestion. I learned it's TreeWalker.find_parent(AST_NodeType), but it appears to do the trick.

[alexlamsl]

I learned it's TreeWalker.find_parent(AST_NodeType)

Indeed - Compressor is a subclass of TreeTransformer, which in turn is a subclass of TreeWalker

[kzc]

Okay, the following patch works on harmony:

--- a/lib/compress.js
+++ b/lib/compress.js
@@ -494,11 +494,10 @@ merge(Compressor.prototype, {
                     // Constant single use vars can be replaced in any scope.
                     if (var_decl.value.is_constant()) {
                         var ctt = new TreeTransformer(function(node) {
-                            if (node === ref) {
-                                var parent = ctt.parent();
-                                if (!(parent instanceof AST_ForIn && parent.init === node)) {
-                                    return replace_var(node, parent, true);
-                                }
+                            if (node === ref
+                                && !ctt.find_parent(AST_Destructuring)
+                                && !ctt.find_parent(AST_ForIn)) {
+                                return replace_var(node, ctt.parent(), true);
                             }
                         });
                         stat.transform(ctt);

$ echo 'var x = 1, y = 2; [x] = [y];' | bin/uglifyjs -c collapse_vars=1,reduce_vars=0
WARN: Collapsing constant y [-:1,25]
var x=1;[x]=[2];



md5-fe87426594bf6a297b69f42e86bfdfdf



$ echo 'var x=1, y=2; (function(){for ([[x],y] of a);})();' | bin/uglifyjs -c collapse_vars=1,reduce_vars=0
var x=1,y=2;!function(){for([[x],y]of a);}();



md5-fe87426594bf6a297b69f42e86bfdfdf



$ echo 'var x=1, y=2; (function(){for ([[x],y] in a);})();' | bin/uglifyjs -c collapse_vars=1,reduce_vars=0
var x=1,y=2;!function(){for([[x],y]in a);}();

Just won't have collapse_vars bother with constants in AST_ForIn, AST_ForOf and AST_Destructuring - leave that for reduce_vars to handle.

I'll just remove the AST_Destructuring line and update PR #1543 for master.

[alexlamsl]

Keeping this PR open until the next harmony merge to remind myself of adding that line of AST_Destructuring (plus may be more tests)

[kzc]

@alexlamsl harmony also requires this patch to avoid non-constant replacement in collapse_vars in destructuring:

--- a/lib/compress.js
+++ b/lib/compress.js
@@ -529,6 +529,7 @@ merge(Compressor.prototype, {
                             if (unwind) return node;
                             var parent = tt.parent();
                             if (node instanceof AST_Lambda
+                                || node instanceof AST_Destructuring
                                 || node instanceof AST_Try
                                 || node instanceof AST_With
                                 || node instanceof AST_Case

to avoid bugs like this in ES6 code:

$ echo 'var x = foo(); [x] = [1];' | bin/uglifyjs -c collapse_vars=1,reduce_vars=0
WARN: Collapsing variable x [-:1,16]
[foo()]=[1];

and

$ echo 'var x = Math.random(); ({p: x = 9} = {v:1});' | bin/uglifyjs -c collapse_vars=1,reduce_vars=0
WARN: Collapsing variable x [-:1,28]
({p:Math.random()=9}={v:1});

Perhaps AST_DefaultAssign could also be added to the patch as a class to avoid, but it should not occur outside of an AST_Destructuring node.

[alexlamsl]

All patched up now.