clean up collapse_vars (#1826)

- remove overlap in functionality of singular, consecutive reference of constant value
- remove workarounds for previous bugs in `lib/scope.js`
- distribute recursive `collapse_single_use_vars()` calls to their respective `OPT(AST_Node)`
- enable collapsing of variables within a single `AST_Definitions`

lib/compress.js

@@ -642,151 +642,134 @@ merge(Compressor.prototype, {
             // and if it has exactly one reference then attempt to replace its reference
             // in the statement with the var value and then erase the var definition.
 
-            var self = compressor.self();
+            var scope = compressor.find_parent(AST_Scope);
-            var var_defs_removed = false;
             var toplevel = compressor.option("toplevel");
             for (var stat_index = statements.length; --stat_index >= 0;) {
                 var stat = statements[stat_index];
-                if (stat instanceof AST_Definitions) continue;
 
-                // Process child blocks of statement if present.
+                var var_names_seen = Object.create(null);
-                [stat, stat.body, stat.alternative, stat.bcatch, stat.bfinally].forEach(function(node) {
-                    node && node.body && collapse_single_use_vars(node.body, compressor);
-                });
-
-                // The variable definition must precede a statement.
-                if (stat_index <= 0) break;
-                var prev_stat_index = stat_index - 1;
-                var prev_stat = statements[prev_stat_index];
-                if (!(prev_stat instanceof AST_Definitions)) continue;
-                var var_defs = prev_stat.definitions;
-                if (var_defs == null) continue;
-
-                var var_names_seen = {};
                 var side_effects_encountered = false;
                 var lvalues_encountered = false;
-                var lvalues = {};
+                var lvalues = Object.create(null);
+                var prev_stat_index, var_defs, var_defs_index;
 
                 // Scan variable definitions from right to left.
-                for (var var_defs_index = var_defs.length; --var_defs_index >= 0;) {
+                if (stat instanceof AST_Definitions) {
-
+                    prev_stat_index = stat_index;
-                    // Obtain var declaration and var name with basic sanity check.
+                    var_defs = stat.definitions;
-                    var var_decl = var_defs[var_defs_index];
+                    for (var_defs_index = var_defs.length - 1; --var_defs_index >= 0;) {
-                    if (var_decl.value == null) break;
+                        if (collapse(var_defs[var_defs_index + 1])) break;
-                    var var_name = var_decl.name.name;
-                    if (!var_name || !var_name.length) break;
-
-                    // Bail if we've seen a var definition of same name before.
-                    if (var_name in var_names_seen) break;
-                    var_names_seen[var_name] = true;
-
-                    // 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" || (!toplevel && def.global)) {
-                        side_effects_encountered = true;
-                        continue;
                     }
-                    var ref = def.references[0];
+                } else if (stat_index > 0) {
-
+                    // The variable definition must precede a statement.
-                    // Don't replace ref if eval() or with statement in scope.
+                    prev_stat_index = stat_index - 1;
-                    if (ref.scope.uses_eval || ref.scope.uses_with) break;
+                    var prev_stat = statements[prev_stat_index];
-
+                    if (!(prev_stat instanceof AST_Definitions)) continue;
-                    // Constant single use vars can be replaced in any scope.
+                    var_defs = prev_stat.definitions;
-                    if (var_decl.value.is_constant()) {
+                    for (var_defs_index = var_defs.length; --var_defs_index >= 0;) {
-                        var ctt = new TreeTransformer(function(node) {
+                        if (collapse(stat)) break;
-                            var parent = ctt.parent();
-                            if (parent instanceof AST_IterationStatement
-                                && (parent.condition === node || parent.init === node)) {
-                                return node;
-                            }
-                            if (node === ref)
-                                return replace_var(node, parent, true);
-                        });
-                        stat.transform(ctt);
-                        continue;
                     }
-
-                    // Restrict var replacement to constants if side effects encountered.
-                    if (side_effects_encountered |= lvalues_encountered) continue;
-
-                    var value_has_side_effects = var_decl.value.has_side_effects(compressor);
-                    // Non-constant single use vars can only be replaced in same scope.
-                    if (ref.scope !== self) {
-                        side_effects_encountered |= value_has_side_effects;
-                        continue;
-                    }
-
-                    // Detect lvalues in var value.
-                    var tw = new TreeWalker(function(node){
-                        if (node instanceof AST_SymbolRef && is_lvalue(node, tw.parent())) {
-                            lvalues[node.name] = lvalues_encountered = true;
-                        }
-                    });
-                    var_decl.value.walk(tw);
-
-                    // Replace the non-constant single use var in statement if side effect free.
-                    var unwind = false;
-                    var tt = new TreeTransformer(
-                        function preorder(node) {
-                            if (unwind) return node;
-                            var parent = tt.parent();
-                            if (node instanceof AST_Lambda
-                                || node instanceof AST_Try
-                                || node instanceof AST_With
-                                || node instanceof AST_Case
-                                || node instanceof AST_IterationStatement
-                                || (parent instanceof AST_If          && node !== parent.condition)
-                                || (parent instanceof AST_Conditional && node !== parent.condition)
-                                || (node instanceof AST_SymbolRef
-                                    && value_has_side_effects
-                                    && !are_references_in_scope(node.definition(), self))
-                                || (parent instanceof AST_Binary
-                                    && (parent.operator == "&&" || parent.operator == "||")
-                                    && node === parent.right)
-                                || (parent instanceof AST_Switch && node !== parent.expression)) {
-                                return side_effects_encountered = unwind = true, node;
-                            }
-                            function are_references_in_scope(def, scope) {
-                                if (def.orig.length === 1
-                                    && def.orig[0] instanceof AST_SymbolDefun) return true;
-                                if (def.scope !== scope) return false;
-                                var refs = def.references;
-                                for (var i = 0, len = refs.length; i < len; i++) {
-                                    if (refs[i].scope !== scope) return false;
-                                }
-                                return true;
-                            }
-                        },
-                        function postorder(node) {
-                            if (unwind) return node;
-                            if (node === ref)
-                                return unwind = true, replace_var(node, tt.parent(), false);
-                            if (side_effects_encountered |= node.has_side_effects(compressor))
-                                return unwind = true, node;
-                            if (lvalues_encountered && node instanceof AST_SymbolRef && node.name in lvalues) {
-                                side_effects_encountered = true;
-                                return unwind = true, node;
-                            }
-                        }
-                    );
-                    stat.transform(tt);
                 }
             }
 
-            // Remove extraneous empty statments in block after removing var definitions.
-            // Leave at least one statement in `statements`.
-            if (var_defs_removed) for (var i = statements.length; --i >= 0;) {
-                if (statements.length > 1 && statements[i] instanceof AST_EmptyStatement)
-                    statements.splice(i, 1);
-            }
-
             return statements;
 
+            function collapse(stat) {
+                var var_decl = var_defs[var_defs_index];
+                // `drop_unused()` shuffles variables without values to the top,
+                // so we can terminate upon first sighting as an optimization.
+                if (var_decl.value == null) return true;
+                var var_name = var_decl.name.name;
+
+                // Bail if we've seen a var definition of same name before.
+                if (var_name in var_names_seen) return true;
+                var_names_seen[var_name] = true;
+
+                // Only interested in non-constant values.
+                if (var_decl.value.is_constant()) return;
+
+                // Only interested in cases with just one reference to the variable.
+                var def = var_decl.name.definition();
+                if (def.references.length !== 1
+                    || var_name == "arguments" || (!toplevel && def.global)) {
+                    side_effects_encountered = true;
+                    return;
+                }
+                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) return true;
+
+                // Restrict var replacement to constants if side effects encountered.
+                if (side_effects_encountered |= lvalues_encountered) return;
+
+                var value_has_side_effects = var_decl.value.has_side_effects(compressor);
+                // Non-constant single use vars can only be replaced in same scope.
+                if (ref.scope !== scope) {
+                    side_effects_encountered |= value_has_side_effects;
+                    return;
+                }
+
+                // Detect lvalues in var value.
+                var tw = new TreeWalker(function(node){
+                    if (node instanceof AST_SymbolRef && is_lvalue(node, tw.parent())) {
+                        lvalues[node.name] = lvalues_encountered = true;
+                    }
+                });
+                var_decl.value.walk(tw);
+
+                // Replace the non-constant single use var in statement if side effect free.
+                var unwind = false;
+                var tt = new TreeTransformer(
+                    function preorder(node) {
+                        if (unwind || node instanceof AST_Scope && node !== scope) return node;
+                        var parent = tt.parent();
+                        if (node instanceof AST_Try
+                            || node instanceof AST_With
+                            || node instanceof AST_Case
+                            || node instanceof AST_IterationStatement
+                            || (parent instanceof AST_If          && node !== parent.condition)
+                            || (parent instanceof AST_Conditional && node !== parent.condition)
+                            || (node instanceof AST_SymbolRef
+                                && value_has_side_effects
+                                && !are_references_in_scope(node.definition(), scope))
+                            || (parent instanceof AST_Binary
+                                && (parent.operator == "&&" || parent.operator == "||")
+                                && node === parent.right)
+                            || (parent instanceof AST_Switch && node !== parent.expression)) {
+                            return side_effects_encountered = unwind = true, node;
+                        }
+                        function are_references_in_scope(def, scope) {
+                            if (def.orig.length === 1
+                                && def.orig[0] instanceof AST_SymbolDefun) return true;
+                            if (def.scope !== scope) return false;
+                            var refs = def.references;
+                            for (var i = 0, len = refs.length; i < len; i++) {
+                                if (refs[i].scope !== scope) return false;
+                            }
+                            return true;
+                        }
+                    },
+                    function postorder(node) {
+                        if (unwind) return node;
+                        if (node === ref)
+                            return unwind = true, replace_var(var_decl, node, tt.parent(), false);
+                        if (side_effects_encountered |= node.has_side_effects(compressor))
+                            return unwind = true, node;
+                        if (lvalues_encountered && node instanceof AST_SymbolRef && node.name in lvalues) {
+                            side_effects_encountered = true;
+                            return unwind = true, node;
+                        }
+                    }
+                );
+                stat.transform(tt);
+            }
+
             function is_lvalue(node, parent) {
                 return node instanceof AST_SymbolRef && is_lhs(node, parent);
             }
-            function replace_var(node, parent, is_constant) {
+
+            function replace_var(var_decl, node, parent, is_constant) {
                 if (is_lvalue(node, parent)) return node;
 
                 // Remove var definition and return its value to the TreeTransformer to replace.
@@ -795,14 +778,19 @@ merge(Compressor.prototype, {
 
                 var_defs.splice(var_defs_index, 1);
                 if (var_defs.length === 0) {
-                    statements[prev_stat_index] = make_node(AST_EmptyStatement, self);
+                    statements.splice(prev_stat_index, 1);
-                    var_defs_removed = true;
+                    stat_index--;
                 }
                 // Further optimize statement after substitution.
                 stat.reset_opt_flags(compressor);
 
-                compressor.info("Collapsing " + (is_constant ? "constant" : "variable") +
+                compressor.info("Collapsing {type} {name} [{file}:{line},{col}]", {
-                    " " + var_name + " [{file}:{line},{col}]", node.start);
+                    type: is_constant ? "constant" : "variable",
+                    name: var_decl.name.name,
+                    file: node.start.file,
+                    line: node.start.line,
+                    col: node.start.col
+                });
                 CHANGED = true;
                 return value;
             }
@@ -1746,6 +1734,7 @@ merge(Compressor.prototype, {
         def(AST_SymbolRef, function(compressor){
             return this.undeclared();
         });
+        def(AST_SymbolDeclaration, return_false);
         def(AST_Object, function(compressor){
             return any(this.properties, compressor);
         });

test/compress/collapse_vars.js

@@ -2,7 +2,7 @@ collapse_vars_side_effects_1: {
     options = {
         collapse_vars:true, sequences:true, properties:true, dead_code:true, conditionals:true,
         comparisons:true, evaluate:true, booleans:true, loops:true, unused:true, hoist_funs:true,
-        keep_fargs:true, if_return:true, join_vars:true, cascade:true, side_effects:true
+        keep_fargs:true, if_return:true, join_vars:true, cascade:true, side_effects:true, reduce_vars:true
     }
     input: {
         function f1() {
@@ -151,7 +151,7 @@ collapse_vars_issue_721: {
     options = {
         collapse_vars:true, sequences:true, properties:true, dead_code:true, conditionals:true,
         comparisons:true, evaluate:true, booleans:true, loops:true, unused:true, hoist_funs:true,
-        keep_fargs:true, if_return:true, join_vars:true, cascade:true, side_effects:true
+        keep_fargs:true, if_return:true, join_vars:true, cascade:true, side_effects:true, reduce_vars:true
     }
     input: {
         define(["require", "exports", 'handlebars'], function (require, exports, hb) {
@@ -217,7 +217,7 @@ collapse_vars_properties: {
     options = {
         collapse_vars:true, sequences:true, properties:true, dead_code:true, conditionals:true,
         comparisons:true, evaluate:true, booleans:true, loops:true, unused:true, hoist_funs:true,
-        keep_fargs:true, if_return:true, join_vars:true, cascade:true, side_effects:true
+        keep_fargs:true, if_return:true, join_vars:true, cascade:true, side_effects:true, reduce_vars:true
     }
     input: {
         function f1(obj) {
@@ -244,7 +244,7 @@ collapse_vars_if: {
     options = {
         collapse_vars:true, sequences:true, properties:true, dead_code:true, conditionals:true,
         comparisons:true, evaluate:true, booleans:true, loops:true, unused:true, hoist_funs:true,
-        keep_fargs:true, if_return:true, join_vars:true, cascade:true, side_effects:true
+        keep_fargs:true, if_return:true, join_vars:true, cascade:true, side_effects:true, reduce_vars:true
     }
     input: {
         function f1() {
@@ -294,7 +294,7 @@ collapse_vars_while: {
     options = {
         collapse_vars:true, sequences:true, properties:true, dead_code:true, conditionals:true,
         comparisons:true, evaluate:true, booleans:true, loops:false, unused:true, hoist_funs:true,
-        keep_fargs:true, if_return:true, join_vars:true, cascade:true, side_effects:true
+        keep_fargs:true, if_return:true, join_vars:true, cascade:true, side_effects:true, reduce_vars:true
     }
     input: {
         function f1(y) {
@@ -393,9 +393,9 @@ collapse_vars_do_while: {
         }
         function f3(y) {
             function fn(n) { console.log(n); }
-            var a = 2;
+            var a = 2, x = 7;
             do {
-                fn(a = 7);
+                fn(a = x);
                 break;
             } while (y);
         }
@@ -468,8 +468,9 @@ collapse_vars_do_while_drop_assign: {
         }
         function f3(y) {
             function fn(n) { console.log(n); }
+            var x = 7;
             do {
-                fn(7);
+                fn(x);
                 break;
             } while (y);
         }
@@ -670,7 +671,7 @@ collapse_vars_lvalues: {
         function f4(x) { var a = (x -= 3); return x + a; }
         function f5(x) { var w = e1(), v = e2(), c = v = --x; return (w = x) - c; }
         function f6(x) { var w = e1(), v = e2(); return (v = --x) - (w = x); }
-        function f7(x) { var w = e1(), v = e2(), c = v - x; return (w = x) - c; }
+        function f7(x) { var w = e1(), c = e2() - x; return (w = x) - c; }
         function f8(x) { var w = e1(), v = e2(); return (w = x) - (v - x); }
         function f9(x) { var w = e1(); return e2() - x - (w = x); }
     }
@@ -702,7 +703,7 @@ collapse_vars_lvalues_drop_assign: {
         function f4(x) { var a = (x -= 3); return x + a; }
         function f5(x) { var v = (e1(), e2()), c = v = --x; return x - c; }
         function f6(x) { e1(), e2(); return --x - x; }
-        function f7(x) { var v = (e1(), e2()), c = v - x; return x - c; }
+        function f7(x) { var c = (e1(), e2() - x); return x - c; }
         function f8(x) { var v = (e1(), e2()); return x - (v - x); }
         function f9(x) { e1(); return e2() - x - x; }
     }
@@ -712,7 +713,7 @@ collapse_vars_misc1: {
     options = {
         collapse_vars:true, sequences:true, properties:true, dead_code:true, conditionals:true,
         comparisons:true, evaluate:true, booleans:true, loops:true, unused:true, hoist_funs:true,
-        keep_fargs:true, if_return:true, join_vars:true, cascade:true, side_effects:true
+        keep_fargs:true, if_return:true, join_vars:true, cascade:true, side_effects:true, reduce_vars:true
     }
     input: {
         function f0(o, a, h) {
@@ -789,7 +790,7 @@ collapse_vars_repeated: {
     options = {
         collapse_vars:true, sequences:true, properties:true, dead_code:true, conditionals:true,
         comparisons:true, evaluate:true, booleans:true, loops:true, unused:true, hoist_funs:true,
-        keep_fargs:true, if_return:true, join_vars:true, cascade:true, side_effects:true
+        keep_fargs:true, if_return:true, join_vars:true, cascade:true, side_effects:true, reduce_vars:true
     }
     input: {
         function f1() {
@@ -812,19 +813,17 @@ collapse_vars_repeated: {
     }
     expect: {
         function f1() {
-            return -3
+            return -3;
         }
         function f2(x) {
-            return x
+            return x;
         }
         (function(x){
-             var a = "GOOD" + x, e = "BAD", e = a;
+             console.log("GOOD!!");
-             console.log(e + "!");
+        })(),
-        })("!"),
         (function(x){
-            var a = "GOOD" + x, e = "BAD" + x, e = a;
+             console.log("GOOD!!");
-            console.log(e + "!");
+        })();
-        })("!");
     }
     expect_stdout: true
 }
@@ -833,7 +832,7 @@ collapse_vars_closures: {
     options = {
         collapse_vars:true, sequences:true, properties:true, dead_code:true, conditionals:true,
         comparisons:true, evaluate:true, booleans:true, loops:true, unused:true, hoist_funs:true,
-        keep_fargs:true, if_return:true, join_vars:true, cascade:true, side_effects:true
+        keep_fargs:true, if_return:true, join_vars:true, cascade:true, side_effects:true, reduce_vars:true
     }
     input: {
         function constant_vars_can_be_replaced_in_any_scope() {
@@ -923,7 +922,7 @@ collapse_vars_try: {
     options = {
         collapse_vars:true, sequences:true, properties:true, dead_code:true, conditionals:true,
         comparisons:true, evaluate:true, booleans:true, loops:true, unused:true, hoist_funs:true,
-        keep_fargs:true, if_return:true, join_vars:true, cascade:true, side_effects:true
+        keep_fargs:true, if_return:true, join_vars:true, cascade:true, side_effects:true, reduce_vars:true
     }
     input: {
         function f1() {
@@ -1121,7 +1120,7 @@ collapse_vars_constants: {
     options = {
         collapse_vars:true, sequences:true, properties:true, dead_code:true, conditionals:true,
         comparisons:true, evaluate:true, booleans:true, loops:true, unused:true, hoist_funs:true,
-        keep_fargs:true, if_return:true, join_vars:true, cascade:true, side_effects:true
+        keep_fargs:true, if_return:true, join_vars:true, cascade:true, side_effects:true, reduce_vars:true
     }
     input: {
         function f1(x) {
@@ -1159,7 +1158,7 @@ collapse_vars_arguments: {
         collapse_vars:true, sequences:true, properties:true, dead_code:true, conditionals:true,
         comparisons:true, evaluate:true, booleans:true, loops:true, unused:true, hoist_funs:true,
         keep_fargs:true, if_return:true, join_vars:true, cascade:true, side_effects:true,
-        toplevel:true
+        toplevel:true, reduce_vars:true
     }
     input: {
         var outer = function() {
@@ -1286,6 +1285,7 @@ collapse_vars_regexp: {
         join_vars:     true,
         cascade:       true,
         side_effects:  true,
+        reduce_vars:   true,
     }
     input: {
         function f1() {
@@ -1319,8 +1319,8 @@ collapse_vars_regexp: {
             };
         }
         (function(){
-            var result, s = "acdabcdeabbb", rx = /ab*/g;
+            var result, rx = /ab*/g;
-            while (result = rx.exec(s))
+            while (result = rx.exec("acdabcdeabbb"))
                 console.log(result[0]);
         })();
     }
@@ -1344,7 +1344,10 @@ issue_1537: {
 issue_1562: {
     options = {
         collapse_vars: true,
+        evaluate: true,
+        reduce_vars: true,
         toplevel: true,
+        unused: true,
     }
     input: {
         var v = 1, B = 2;
@@ -1363,14 +1366,11 @@ issue_1562: {
         var v = 1;
         for (v in objs) f(2);
 
-        var x = 3;
+        while(5) bar(10);
-        while(x + 2) bar(10);
 
-        var y = 4;
+        do bar(20); while(6);
-        do bar(20); while(y + 2);
 
-        var z = 5;
+        for (; f(7) ;) bar(30);
-        for (; f(z + 2) ;) bar(30);
     }
 }
 
@@ -1614,3 +1614,43 @@ reduce_vars_assign: {
     }
     expect_stdout: "0"
 }
+
+iife_1: {
+    options = {
+        collapse_vars: true,
+        reduce_vars: true,
+        toplevel: true,
+        unused: true,
+    }
+    input: {
+        var log = function(x) {
+            console.log(x);
+        }, foo = bar();
+        log(foo);
+    }
+    expect: {
+        (function(x) {
+            console.log(x);
+        })(bar());
+    }
+}
+
+iife_2: {
+    options = {
+        collapse_vars: true,
+        reduce_vars: false,
+        toplevel: true,
+        unused: false,
+    }
+    input: {
+        var foo = bar();
+        !function(x) {
+            console.log(x);
+        }(foo);
+    }
+    expect: {
+        !function(x) {
+            console.log(x);
+        }(bar());
+    }
+}