fix replacement logic in collapse_vars (#2475)

lib/compress.js

@@ -839,30 +839,7 @@ merge(Compressor.prototype, {
             var args;
             var candidates = [];
             var stat_index = statements.length;
-            while (--stat_index >= 0) {
+            var scanner = new TreeTransformer(function(node, descend) {
-                // Treat parameters as collapsible in IIFE, i.e.
-                //   function(a, b){ ... }(x());
-                // would be translated into equivalent assignments:
-                //   var a = x(), b = undefined;
-                if (stat_index == 0 && compressor.option("unused")) extract_args();
-                // Find collapsible assignments
-                extract_candidates(statements[stat_index]);
-                while (candidates.length > 0) {
-                    var candidate = candidates.pop();
-                    var lhs = get_lhs(candidate);
-                    if (!lhs || is_lhs_read_only(lhs) || lhs.has_side_effects(compressor)) continue;
-                    // Locate symbols which may execute code outside of scanning range
-                    var lvalues = get_lvalues(candidate);
-                    if (lhs instanceof AST_SymbolRef) lvalues[lhs.name] = false;
-                    var replace_all = candidate.multiple;
-                    if (!replace_all && lhs instanceof AST_SymbolRef) {
-                        var def = lhs.definition();
-                        replace_all = def.references.length - def.replaced == 1;
-                    }
-                    var side_effects = value_has_side_effects(candidate);
-                    var hit = candidate.name instanceof AST_SymbolFunarg;
-                    var abort = false, replaced = 0, can_replace = !args || !hit;
-                    var tt = new TreeTransformer(function(node, descend) {
                 if (abort) return node;
                 // Skip nodes before `candidate` as quickly as possible
                 if (!hit) {
@@ -873,7 +850,7 @@ merge(Compressor.prototype, {
                     return;
                 }
                 // Stop immediately if these node types are encountered
-                        var parent = tt.parent();
+                var parent = scanner.parent();
                 if (node instanceof AST_Assign && node.operator != "=" && lhs.equivalent_to(node.left)
                     || node instanceof AST_Call && lhs instanceof AST_PropAccess && lhs.equivalent_to(node.expression)
                     || node instanceof AST_Debugger
@@ -888,8 +865,11 @@ merge(Compressor.prototype, {
                 // Replace variable with assignment when found
                 if (can_replace
                     && !(node instanceof AST_SymbolDeclaration)
-                            && !is_lhs(node, parent)
                     && lhs.equivalent_to(node)) {
+                    if (is_lhs(node, parent)) {
+                        if (candidate.multiple) replaced++;
+                        return node;
+                    }
                     CHANGED = abort = true;
                     replaced++;
                     compressor.info("Collapsing {name} [{file}:{line},{col}]", {
@@ -937,32 +917,16 @@ merge(Compressor.prototype, {
                             || parent instanceof AST_Case
                             || parent instanceof AST_Conditional
                             || parent instanceof AST_If)) {
-                            if (!(node instanceof AST_Scope)) descend(node, tt);
+                    if (!(node instanceof AST_Scope)) descend(node, scanner);
                     abort = true;
                     return node;
                 }
                 // Skip (non-executed) functions and (leading) default case in switch statements
                 if (node instanceof AST_Default || node instanceof AST_Scope) return node;
             });
-                    if (!can_replace) {
+            var multi_replacer = new TreeTransformer(function(node) {
-                        for (var j = compressor.self().argnames.lastIndexOf(candidate.name) + 1; !abort && j < args.length; j++) {
-                            args[j].transform(tt);
-                        }
-                        can_replace = true;
-                    }
-                    for (var i = stat_index; !abort && i < statements.length; i++) {
-                        statements[i].transform(tt);
-                    }
-                    if (candidate.multiple) {
-                        var def = candidate.name.definition();
-                        if (abort && def.references.length > replaced) replaced = false;
-                        else {
-                            abort = false;
-                            hit = candidate.name instanceof AST_SymbolFunarg;
-                            var value_def = candidate.value.definition();
-                            for (var i = stat_index; !abort && i < statements.length; i++) {
-                                statements[i].transform(new TreeTransformer(function(node) {
                 if (abort) return node;
+                // Skip nodes before `candidate` as quickly as possible
                 if (!hit) {
                     if (node === candidate) {
                         hit = true;
@@ -970,13 +934,59 @@ merge(Compressor.prototype, {
                     }
                     return;
                 }
-                                    if (node instanceof AST_SymbolRef && node.name == def.name) {
+                // Replace variable when found
+                if (node instanceof AST_SymbolRef
+                    && node.name == def.name) {
+                    if (!--replaced) abort = true;
+                    if (is_lhs(node, multi_replacer.parent())) return node;
                     def.replaced++;
                     value_def.replaced--;
-                                        if (!--replaced) abort = true;
                     return candidate.value;
                 }
-                                }));
+                // Skip (non-executed) functions and (leading) default case in switch statements
+                if (node instanceof AST_Default || node instanceof AST_Scope) return node;
+            });
+            while (--stat_index >= 0) {
+                // Treat parameters as collapsible in IIFE, i.e.
+                //   function(a, b){ ... }(x());
+                // would be translated into equivalent assignments:
+                //   var a = x(), b = undefined;
+                if (stat_index == 0 && compressor.option("unused")) extract_args();
+                // Find collapsible assignments
+                extract_candidates(statements[stat_index]);
+                while (candidates.length > 0) {
+                    var candidate = candidates.pop();
+                    var lhs = get_lhs(candidate);
+                    if (!lhs || is_lhs_read_only(lhs) || lhs.has_side_effects(compressor)) continue;
+                    // Locate symbols which may execute code outside of scanning range
+                    var lvalues = get_lvalues(candidate);
+                    if (lhs instanceof AST_SymbolRef) lvalues[lhs.name] = false;
+                    var replace_all = candidate.multiple;
+                    if (!replace_all && lhs instanceof AST_SymbolRef) {
+                        var def = lhs.definition();
+                        replace_all = def.references.length - def.replaced == 1;
+                    }
+                    var side_effects = value_has_side_effects(candidate);
+                    var hit = candidate.name instanceof AST_SymbolFunarg;
+                    var abort = false, replaced = 0, can_replace = !args || !hit;
+                    if (!can_replace) {
+                        for (var j = compressor.self().argnames.lastIndexOf(candidate.name) + 1; !abort && j < args.length; j++) {
+                            args[j].transform(scanner);
+                        }
+                        can_replace = true;
+                    }
+                    for (var i = stat_index; !abort && i < statements.length; i++) {
+                        statements[i].transform(scanner);
+                    }
+                    if (candidate.multiple) {
+                        var def = candidate.name.definition();
+                        if (abort && def.references.length - def.replaced > replaced) replaced = false;
+                        else {
+                            abort = false;
+                            hit = candidate.name instanceof AST_SymbolFunarg;
+                            var value_def = candidate.value.definition();
+                            for (var i = stat_index; !abort && i < statements.length; i++) {
+                                statements[i].transform(multi_replacer);
                             }
                         }
                     }
@@ -2549,12 +2559,14 @@ merge(Compressor.prototype, {
                             var def = tail.pop();
                             compressor.warn("Converting duplicated definition of variable {name} to assignment [{file}:{line},{col}]", template(def.name));
                             remove(var_defs, def);
-                            drop_decl(def.name.definition());
                             side_effects.unshift(make_node(AST_Assign, def, {
                                 operator: "=",
                                 left: make_node(AST_SymbolRef, def.name, def.name),
                                 right: def.value
                             }));
+                            def = def.name.definition();
+                            drop_decl(def);
+                            def.replaced--;
                         }
                     }
                     if (head.length > 0 || tail.length > 0) {

test/compress/collapse_vars.js

@@ -3518,3 +3518,63 @@ issue_2436_12: {
         }
     }
 }
+
+issue_2436_13: {
+    options = {
+        collapse_vars: true,
+        reduce_vars: true,
+        unused: true,
+    }
+    input: {
+        var a = "PASS";
+        (function() {
+            function f(b) {
+                (function g(b) {
+                    var b = b && (b.null = "FAIL");
+                })(a);
+            }
+            f();
+        })();
+        console.log(a);
+    }
+    expect: {
+        var a = "PASS";
+        (function() {
+            (function(b) {
+                (function(b) {
+                    a && (a.null = "FAIL");
+                })();
+            })();
+        })();
+        console.log(a);
+    }
+    expect_stdout: "PASS"
+}
+
+issue_2436_14: {
+    options = {
+        collapse_vars: true,
+        reduce_vars: true,
+        unused: true,
+    }
+    input: {
+        var a = "PASS";
+        var b = {};
+        (function() {
+            var c = a;
+            c && function(c, d) {
+                console.log(c, d);
+            }(b, c);
+        })();
+    }
+    expect: {
+        var a = "PASS";
+        var b = {};
+        (function() {
+            a && function(c, d) {
+                console.log(c, d);
+            }(b, a);
+        })();
+    }
+    expect_stdout: true
+}