consolidate single-use function reduction

[alexlamsl]

fixes #2423

@kzc this combines #2053 from distant past with .single_use and should cover existing cases and a few more - please give it a spin 😎

[kzc]

The previously discussed example:

$ cat scope0.js 
function x() {
    y();
}
function y() {
    console.log(1);
}
function z() {
    function y() {
        console.log(2);
    }
    x();
}
z();
z();

does not inline function x or the outer function y in this PR:

$ cat scope0.js | bin/uglifyjs -c toplevel,passes=9 -b
function x() {
    y();
}

function y() {
    console.log(1);
}

function z() {
    x();
}

z(), z();

Notice that the inner function y was not used and was dropped which ought to permit the inlining of function x and the outer function y.

[alexlamsl]

It'd be quite expensive to do a two-way look-up for variable collision - you need to walk up all the parent scopes in between, i.e. consider:

function x() {
    y();
}
function y() {
    console.log(1);
}
function z() {
    function y() {
        console.log(2);
    }
    return function z1() {
        function y() {
            console.log(3);
        }
        return function z2() {
            function y() {
                console.log(4);
            }
            x();
        }();
    }();
}
z();
z();

However, if you focus on your example above, I'd expect that top-level function y() be inlined into function x() - let me have a look.

[alexlamsl]

let me have a look.

Okay well, console.log(1) isn't .is_constant_expression() - duh!

[alexlamsl]

Enhancing .is_constant_expression() to take into account standard global variables under unsafe:

--- a/lib/compress.js
+++ b/lib/compress.js
@@ -319,11 +319,11 @@ merge(Compressor.prototype, {
                             if (value instanceof AST_Lambda) {
                                 d.single_use = d.scope === node.scope
                                         && !(d.orig[0] instanceof AST_SymbolFunarg)
-                                    || value.is_constant_expression();
+                                    || value.is_constant_expression(compressor);
                             } else {
                                 d.single_use = d.scope === node.scope
                                     && loop_ids[d.id] === in_loop
-                                    && value.is_constant_expression();
+                                    && value.is_constant_expression(compressor);
                                 }
                         } else {
                             d.single_use = false;
@@ -385,7 +385,7 @@ merge(Compressor.prototype, {
                         mark(d, true);
                         if (unused && d.references.length == 1) {
                             d.single_use = d.scope === d.references[0].scope
-                                || node.is_constant_expression();
+                                || node.is_constant_expression(compressor);
                         }
                     }
                     var save_ids = safe_ids;
@@ -2168,19 +2168,20 @@ merge(Compressor.prototype, {

     // determine if expression is constant
     (function(def){
-        function all(list) {
+        function all(list, compressor) {
             for (var i = list.length; --i >= 0;)
-                if (!list[i].is_constant_expression())
+                if (!list[i].is_constant_expression(compressor))
                     return false;
             return true;
         }
         def(AST_Node, return_false);
         def(AST_Constant, return_true);
-        def(AST_Lambda, function(){
+        def(AST_Lambda, function(compressor){
             var self = this;
             var result = true;
             self.walk(new TreeWalker(function(node) {
                 if (!result) return true;
+                if (is_undeclared_ref(node) && node.is_declared(compressor)) return true;
                 if (node instanceof AST_SymbolRef) {
                     var def = node.definition();
                     if (self.enclosed.indexOf(def) >= 0
@@ -2192,20 +2193,21 @@ merge(Compressor.prototype, {
             }));
             return result;
         });
-        def(AST_Unary, function(){
-            return this.expression.is_constant_expression();
+        def(AST_Unary, function(compressor){
+            return this.expression.is_constant_expression(compressor);
         });
-        def(AST_Binary, function(){
-            return this.left.is_constant_expression() && this.right.is_constant_expression();
+        def(AST_Binary, function(compressor){
+            return this.left.is_constant_expression(compressor)
+                && this.right.is_constant_expression(compressor);
         });
-        def(AST_Array, function(){
-            return all(this.elements);
+        def(AST_Array, function(compressor){
+            return all(this.elements, compressor);
         });
-        def(AST_Object, function(){
-            return all(this.properties);
+        def(AST_Object, function(compressor){
+            return all(this.properties, compressor);
         });
-        def(AST_ObjectProperty, function(){
-            return this.value.is_constant_expression();
+        def(AST_ObjectProperty, function(compressor){
+            return this.value.is_constant_expression(compressor);
         });
     })(function(node, func){
         node.DEFMETHOD("is_constant_expression", func);
@@ -3501,7 +3503,7 @@ merge(Compressor.prototype, {
         var stat = fn instanceof AST_Function && fn.body[0];
         if (compressor.option("inline") && stat instanceof AST_Return) {
             var value = stat.value;
-            if (!value || value.is_constant_expression()) {
+            if (!value || value.is_constant_expression(compressor)) {
                 var args = self.args.concat(value || make_node(AST_Undefined, self));
                 return make_sequence(self, args).optimize(compressor);
             }

gives:

$ uglifyjs scope0.js -b bracketize -c toplevel,passes=2
function x() {
    y();
}

function y() {
    console.log(1);
}

function z() {
    x();
}

z(), z();

$ uglifyjs scope0.js -b bracketize -c toplevel,passes=2,unsafe
function z() {
    console.log(1);
}

z(), z();

What do you think?

[kzc]

It'd be quite expensive to do a two-way look-up for variable collision - you need to walk up all the parent scopes in between

Unfortunately all the interesting inline-able functions fall into that category.

[kzc]

function z(){console.log(1)}z(),z();
What do you think?

That clearly works fine. What's inherently unsafe about this particular transform?

[alexlamsl]

Unfortunately all the interesting inline-able functions fall into that category.

Any real-life examples? The existing single-use function inlining certainly works on many instances within test/benchmark.js already.

[alexlamsl]

What's inherently unsafe about this particular transform?

console being available as a global variable - though now that I think about it, it's rather easily defeated by introducing var console = 42; within function z(), so let's come up with a better solution...

[kzc]

I was playing with react@16 and found a few single-use function cases that were not inlined when building the library from source. They've switched to using Google Closure.

[alexlamsl]

Well, if they don't mind following all the restrictions of ADVANCED_OPTIMIZATIONS, that's certainly one way of doing it 😉

[kzc]

They were using SIMPLE_OPTIMIZATIONS which is more conservative. It's like advanced but excludes property mangling and retains toplevel symbols - similar to uglify's defaults.

Here's a simplified version of something I found in react@16:

!function(){
    function bar(o) {
        return {
            a: o.x,
            b: o.y
        };
    }
    function foo(obj) {
        console.log(bar(obj));
    }
    foo({x: 1});
    foo({y: 2});
}();

[alexlamsl]

Second iteration of .is_constant_expression() refinement - no unsafe required:

--- a/lib/compress.js
+++ b/lib/compress.js
@@ -319,7 +319,7 @@ merge(Compressor.prototype, {
                             if (value instanceof AST_Lambda) {
                                 d.single_use = d.scope === node.scope
                                         && !(d.orig[0] instanceof AST_SymbolFunarg)
-                                    || value.is_constant_expression();
+                                    || value.is_constant_expression(node.scope);
                             } else {
                                 d.single_use = d.scope === node.scope
                                     && loop_ids[d.id] === in_loop
@@ -2176,7 +2176,7 @@ merge(Compressor.prototype, {
         }
         def(AST_Node, return_false);
         def(AST_Constant, return_true);
-        def(AST_Lambda, function(){
+        def(AST_Lambda, function(scope){
             var self = this;
             var result = true;
             self.walk(new TreeWalker(function(node) {
@@ -2184,7 +2184,8 @@ merge(Compressor.prototype, {
                 if (node instanceof AST_SymbolRef) {
                     var def = node.definition();
                     if (self.enclosed.indexOf(def) >= 0
-                        && self.variables.get(def.name) !== def) {
+                        && self.variables.get(def.name) !== def
+                        && (!scope || self.find_variable(def) !== scope.find_variable(def))) {
                         result = false;
                         return true;
                     }

will need some help testing, but at least the example works:

$ uglifyjs scope0.js -b bracketize -c toplevel,passes=2
function z() {
    console.log(1);
}

z(), z();

[alexlamsl]

For your react.js example above:

$ uglifyjs react.js -b bracketize -c
!function() {
    function foo(obj) {
        console.log(function(o) {
            return {
                a: o.x,
                b: o.y
            };
        }(obj));
    }
    foo({
        x: 1
    }), foo({
        y: 2
    });
}();

[alexlamsl]

... btw your react.js example works without #2427 (comment)

[kzc]

It'd nice to be able to inline functions that are not constant expressions, such as:

!function(){
    function bar(k) {
        console.log(k);
    }
    function foo(x) {
        return bar(x);
    }
    function baz(a) {
        foo(a);
    }
    baz(Math.random());
    baz(Math.random());
}();

which should result in:

function baz(a) {
    console.log(a);
}
baz(Math.random());
baz(Math.random());

or better yet:

console.log(Math.random());
console.log(Math.random());

That optimization is performed by Closure in simple mode.

[kzc]

... btw your react.js example works without #2427 (comment)

It almost works - it did indeed inline the function:

        console.log(function(o) {
            return {
                a: o.x,
                b: o.y
            };
        }(obj));

but would like to see:

        console.log({
            a: obj.x,
            b: obj.y
        });

[kzc]

Second iteration of .is_constant_expression() refinement - no unsafe required:

That's good progress!

[kzc]

I recognize that inlining functions that are not constant expressions is beyond the scope of this PR. Should open another Issue for that.

Yet another Issue can be opened to inline single-statement functions used more than once if the function symbol is solely referenced via AST_Call.expressions. Would have to deep clone the original function.

[alexlamsl]

Agreed - I'll incorporate #2427 (comment) and merge this PR first, then we can address the remaining problems. That way we can start ufuzz and the release process.

Would be helpful if you can summarise and/or separate them into new issue reports 😉

[kzc]

@alexlamsl Could you summarize what specific scenarios this PR will optimize? We went on a lot of discussion tangents above.

[alexlamsl]

Right, I've changed my mind about incorporating #2427 (comment) as it needs more work.

So #2427 (comment) at the very top summarises this PR.

[kzc]

In a followup commit could you please add expect_stdout: true to these tests?

[alexlamsl]

Done in #2430

[kzc]

@alexlamsl In light of the recent compress optimizations with AST_This and inlining do you think the next release warrants a minor version bump?

[alexlamsl]

In light of the recent compress optimizations with AST_This and inlining do you think the next release warrants a minor version bump?

I don't think it's user-visible, so let's bump minor version when we enable hoist_props by default.