improve reduce_vars (#3112)

fixes #3110

lib/compress.js

@@ -374,6 +374,15 @@ merge(Compressor.prototype, {
             });
         }
 
+        function walk_defuns(tw, compressor, node) {
+            node.functions.each(function(def) {
+                if (def.init instanceof AST_Defun && !(def.id in tw.defun_ids)) {
+                    tw.defun_ids[def.id] = true;
+                    def.init.walk(tw);
+                }
+            });
+        }
+
         function push(tw) {
             tw.safe_ids = Object.create(tw.safe_ids);
         }
@@ -398,7 +407,7 @@ merge(Compressor.prototype, {
             return def.fixed instanceof AST_Defun;
         }
 
-        function safe_to_assign(tw, def, value) {
+        function safe_to_assign(tw, def, scope, value) {
             if (def.fixed === undefined) return true;
             if (def.fixed === null && def.safe_ids) {
                 def.safe_ids[def.id] = false;
@@ -409,6 +418,9 @@ merge(Compressor.prototype, {
             if (!safe_to_read(tw, def)) return false;
             if (def.fixed === false) return false;
             if (def.fixed != null && (!value || def.references.length > def.assignments)) return false;
+            if (def.fixed instanceof AST_Defun) {
+                return value instanceof AST_Node && def.fixed.parent_scope === scope;
+            }
             return all(def.orig, function(sym) {
                 return !(sym instanceof AST_SymbolDefun
                     || sym instanceof AST_SymbolLambda);
@@ -468,6 +480,7 @@ merge(Compressor.prototype, {
             reset_variables(tw, compressor, this);
             descend();
             pop(tw);
+            walk_defuns(tw, compressor, this);
             return true;
         });
         def(AST_Assign, function(tw) {
@@ -476,7 +489,7 @@ merge(Compressor.prototype, {
             var d = node.left.definition();
             var fixed = d.fixed;
             if (!fixed && node.operator != "=") return;
-            if (!safe_to_assign(tw, d, node.right)) return;
+            if (!safe_to_assign(tw, d, node.left.scope, node.right)) return;
             d.references.push(node.left);
             d.assignments++;
             if (node.operator != "=") d.chained = true;
@@ -528,12 +541,15 @@ merge(Compressor.prototype, {
             return true;
         });
         def(AST_Defun, function(tw, descend, compressor) {
+            var id = this.name.definition().id;
+            if (!tw.defun_ids[id]) return true;
+            tw.defun_ids[id] = false;
             this.inlined = false;
-            var save_ids = tw.safe_ids;
+            push(tw);
-            tw.safe_ids = Object.create(null);
             reset_variables(tw, compressor, this);
             descend();
-            tw.safe_ids = save_ids;
+            pop(tw);
+            walk_defuns(tw, compressor, this);
             return true;
         });
         def(AST_Do, function(tw) {
@@ -606,6 +622,7 @@ merge(Compressor.prototype, {
             }
             descend();
             pop(tw);
+            walk_defuns(tw, compressor, node);
             return true;
         });
         def(AST_If, function(tw) {
@@ -659,12 +676,21 @@ merge(Compressor.prototype, {
                 }
             }
             mark_escaped(tw, d, this.scope, this, value, 0, 1);
+            if (d.scope === this.scope && !tw.in_loop && d.fixed instanceof AST_Defun && !(d.id in tw.defun_ids)) {
+                tw.defun_ids[d.id] = true;
+                d.fixed.walk(tw);
+            }
         });
         def(AST_Toplevel, function(tw, descend, compressor) {
             this.globals.each(function(def) {
                 reset_def(compressor, def);
             });
+            push(tw);
             reset_variables(tw, compressor, this);
+            descend();
+            pop(tw);
+            walk_defuns(tw, compressor, this);
+            return true;
         });
         def(AST_Try, function(tw) {
             push(tw);
@@ -681,12 +707,13 @@ merge(Compressor.prototype, {
         def(AST_Unary, function(tw, descend) {
             var node = this;
             if (node.operator != "++" && node.operator != "--") return;
-            if (!(node.expression instanceof AST_SymbolRef)) return;
+            var exp = node.expression;
-            var d = node.expression.definition();
+            if (!(exp instanceof AST_SymbolRef)) return;
+            var d = exp.definition();
             var fixed = d.fixed;
             if (!fixed) return;
-            if (!safe_to_assign(tw, d, true)) return;
+            if (!safe_to_assign(tw, d, exp.scope, true)) return;
-            d.references.push(node.expression);
+            d.references.push(exp);
             d.assignments++;
             d.chained = true;
             d.fixed = function() {
@@ -708,7 +735,7 @@ merge(Compressor.prototype, {
             var node = this;
             var d = node.name.definition();
             if (node.value) {
-                if (safe_to_assign(tw, d, node.value)) {
+                if (safe_to_assign(tw, d, node.name.scope, node.value)) {
                     d.fixed = function() {
                         return node.value;
                     };
@@ -736,19 +763,24 @@ merge(Compressor.prototype, {
     });
 
     AST_Toplevel.DEFMETHOD("reset_opt_flags", function(compressor) {
-        var reduce_vars = compressor.option("reduce_vars");
+        var tw = new TreeWalker(compressor.option("reduce_vars") ? function(node, descend) {
-        var tw = new TreeWalker(function(node, descend) {
+            node._squeezed = false;
+            node._optimized = false;
+            return node.reduce_vars(tw, descend, compressor);
+        } : function(node) {
             node._squeezed = false;
             node._optimized = false;
-            if (reduce_vars) return node.reduce_vars(tw, descend, compressor);
         });
+        // Flow control for visiting `AST_Defun`s
+        tw.defun_ids = Object.create(null);
+        // Record the loop body in which `AST_SymbolDeclaration` is first encountered
+        tw.in_loop = null;
+        tw.loop_ids = Object.create(null);
         // Stack of look-up tables to keep track of whether a `SymbolDef` has been
         // properly assigned before use:
         // - `push()` & `pop()` when visiting conditional branches
         // - backup & restore via `save_ids` when visiting out-of-order sections
         tw.safe_ids = Object.create(null);
-        tw.in_loop = null;
-        tw.loop_ids = Object.create(null);
         this.walk(tw);
     });
 

test/compress/reduce_vars.js

@@ -1476,18 +1476,18 @@ defun_redefine: {
             };
             return g() + h();
         }
+        console.log(f());
     }
     expect:  {
         function f() {
-            function g() {
+            (function() {
-                return 1;
-            }
-            g = function() {
                 return 3;
-            };
+            });
-            return g() + 2;
+            return 3 + 2;
         }
+        console.log(f());
     }
+    expect_stdout: "5"
 }
 
 func_inline: {
@@ -1527,23 +1527,37 @@ func_modified: {
     }
     input: {
         function f(a) {
-            function a() { return 1; }
+            function a() {
-            function b() { return 2; }
+                return 1;
-            function c() { return 3; }
+            }
+            function b() {
+                return 2;
+            }
+            function c() {
+                return 3;
+            }
             b.inject = [];
-            c = function() { return 4; };
+            c = function() {
+                return 4;
+            };
             return a() + b() + c();
         }
+        console.log(f());
     }
     expect: {
         function f(a) {
-            function b() { return 2; }
+            function b() {
-            function c() { return 3; }
+                return 2;
+            }
             b.inject = [];
-            c = function() { return 4; };
+            (function() {
-            return 1 + 2 + c();
+                return 4;
+            });
+            return 1 + 2 + 4;
         }
+        console.log(f());
     }
+    expect_stdout: "7"
 }
 
 defun_label: {
@@ -5054,9 +5068,7 @@ defun_var_1: {
         console.log(typeof a, typeof b);
     }
     expect: {
-        var a = 42;
+        console.log("number", "function");
-        function a() {}
-        console.log(typeof a, "function");
     }
     expect_stdout: "number function"
 }
@@ -5076,9 +5088,7 @@ defun_var_2: {
         console.log(typeof a, typeof b);
     }
     expect: {
-        function a() {}
+        console.log("number", "function");
-        var a = 42;
-        console.log(typeof a, "function");
     }
     expect_stdout: "number function"
 }
@@ -5710,3 +5720,116 @@ issue_3068_2: {
     }
     expect_stdout: true
 }
+
+issue_3110_1: {
+    options = {
+        conditionals: true,
+        evaluate: true,
+        inline: true,
+        passes: 3,
+        properties: true,
+        reduce_vars: true,
+        sequences: true,
+        side_effects: true,
+        unused: true,
+    }
+    input: {
+        (function() {
+            function foo() {
+                return isDev ? "foo" : "bar";
+            }
+            var isDev = true;
+            var obj = {
+                foo: foo
+            };
+            console.log(foo());
+            console.log(obj.foo());
+        })();
+    }
+    expect: {
+        console.log("foo"),
+        console.log("foo");
+    }
+    expect_stdout: [
+        "foo",
+        "foo",
+    ]
+}
+
+issue_3110_2: {
+    options = {
+        conditionals: true,
+        evaluate: true,
+        inline: true,
+        passes: 4,
+        properties: true,
+        reduce_vars: true,
+        sequences: true,
+        side_effects: true,
+        unused: true,
+    }
+    input: {
+        (function() {
+            function foo() {
+                return isDev ? "foo" : "bar";
+            }
+            var isDev = true;
+            console.log(foo());
+            var obj = {
+                foo: foo
+            };
+            console.log(obj.foo());
+        })();
+    }
+    expect: {
+        console.log("foo"),
+        console.log("foo");
+    }
+    expect_stdout: [
+        "foo",
+        "foo",
+    ]
+}
+
+issue_3110_3: {
+    options = {
+        conditionals: true,
+        evaluate: true,
+        inline: true,
+        properties: true,
+        reduce_vars: true,
+        sequences: true,
+        side_effects: true,
+        unused: true,
+    }
+    input: {
+        (function() {
+            function foo() {
+                return isDev ? "foo" : "bar";
+            }
+            console.log(foo());
+            var isDev = true;
+            var obj = {
+                foo: foo
+            };
+            console.log(obj.foo());
+        })();
+    }
+    expect: {
+        (function() {
+            function foo() {
+                return isDev ? "foo" : "bar";
+            }
+            console.log(foo());
+            var isDev = true;
+            var obj = {
+                foo: foo
+            };
+            console.log(obj.foo());
+        })();
+    }
+    expect_stdout: [
+        "bar",
+        "foo",
+    ]
+}

test/compress/typeof.js

@@ -90,17 +90,11 @@ typeof_defun_1: {
         "function" == typeof h && h();
     }
     expect: {
-        function g() {
-            h = 42;
-            console.log("NOPE");
-        }
         function h() {
             console.log("YUP");
         }
-        g = 42;
         console.log("YES");
-        "function" == typeof g && g();
+        h();
-        "function" == typeof h && h();
     }
     expect_stdout: [
         "YES",