simple single use function not inlined

[kzc]

Bug report or feature request?

feature request

ES5 or ES6+ input?

ES5

Uglify version (uglifyjs -V)

uglify-js 3.1.6

JavaScript input

$ cat hmm.js 
function c() { return 1; }
function p() { console.log(c()); }
p();
p();

The single use function c is not inlined:

$ bin/uglifyjs hmm.js -c toplevel,passes=3,unsafe
function c(){return 1}function p(){console.log(c())}p(),p();

Oddly if there's only a single call p(); it is:

$ cat hmmm.js
function c() { return 1; }
function p() { console.log(c()); }
p();

$ bin/uglifyjs hmmm.js -c toplevel,passes=2
console.log(1);

Or if function c is local to function p it is also inlined:

$ cat hmmmm.js
function p() {
    function c() { return 1; }
    console.log(c());
}
p();
p();

$ bin/uglifyjs hmmmm.js -c toplevel,passes=2
function p(){console.log(1)}p(),p();

[alexlamsl]

.single_use is restricted to same-scope as per #2339 (comment):
https://github.com/mishoo/UglifyJS2/blob/c8b6f4733d35db48b5b7e2373264db0d99eb299f/lib/compress.js#L319-L323

With your working example above:

function c() { return 1; }
function p() { console.log(c()); }
p();

reduce_vars on p due to single-use:

function c() { return 1; }
(function p() { console.log(c()); })();

inline:

function c() { return 1; }
console.log(c());

collapse_vars on c:

console.log(function c() { return 1; }());

and finally, inline:

console.log(1);

[kzc]

.single_use is restricted to same-scope as per #2339 (comment):

I write a lot of things. :-)

It's a common pattern. What are the risks to lifting that same-scope restriction?

function x() {
    y();
}
function y() {
    console.log(1);
}
function z() {
    function y() {
        console.log(2);
    }
    x();
}
z();
z();

If function x is inlined in function z it would still retain the reference to the outer function y, would it not?

[alexlamsl]

I think AST_Defun can bypass the same-scope restrictions in this case, because the declaration is hoisted. Otherwise we need to be careful with:

f();
// prints "undefined"
var a = 42;
function f() {
    console.log(a);
}

[alexlamsl]

If function x is inlined in function z it would still retain the reference to the outer function y , would it not?

In this case .is_constant_expression() won't allow it:
https://github.com/mishoo/UglifyJS2/blob/c8b6f4733d35db48b5b7e2373264db0d99eb299f/lib/compress.js#L2154-L2169

[alexlamsl]

And to illustrate the point above:

function x() {
    y();
}
function y() {
    console.log(1);
}
function z() {
    function y() {
        console.log(2);
    }
    x();
}
z();

gives 1, whereas:

function y() {
    console.log(1);
}
function z() {
    function y() {
        console.log(2);
    }
    (function x() {
        y();
    })();
}
z();

gives 2

[kzc]

Well, figure_out_scope() should still have function x retain the definition of the outer function y, should it not? Even if function x is deep cloned you wrote in another post that it keeps a shallow copy of the definitions.

[alexlamsl]

Well, figure_out_scope() should still have function x retain the definition of the outer function y, should it not?

Indeed the AST won't get damaged by the transformation, but the output from the AST would no longer be valid JavaScript as shown in #2423 (comment)

Or have I misunderstood your suggestion somewhere? 😅

[kzc]

I see your point now. In memory the symbol refs would point to the correct definitions, but when output it would be wrong. In the back of my mind I thought the colliding symbol names would be renamed somehow - but obviously compress does not cooperate with mangle.

So what set of new constraints must AST_Defuns adhere to in order to be inlined into another scope?

[alexlamsl]

So what set of new constraints must AST_Defuns adhere to in order to be inlined into another scope?

I think just forgoing the same-scope constraint alone would be fine - and it's a special case anyway since IIRC AST_DefClass does not hoist (the Wonders of ECMAScript... 🙈)

[kzc]

"function declarations are hoisted and class declarations are not"

I didn't know that.

I think just forgoing the same-scope constraint alone would be fine

I'm confused - doesn't the example above disprove that?

[alexlamsl]

I'm confused - doesn't the example above disprove that?

The example above would remain not optimised not because of the d.scope === node.scope constraint, but because function x() won't pass the .is_constant_expression() check 😉

[kzc]

Still confused - function c in the top post doesn't pass the .is_constant_expression() check.

The top post example does not get optimized with:

--- a/lib/compress.js
+++ b/lib/compress.js
@@ -319,7 +319,6 @@ merge(Compressor.prototype, {
                             d.single_use = value
                                 && d.references.length == 1
                                 && loop_ids[d.id] === in_loop
-                                && d.scope === node.scope
                                 && value.is_constant_expression();
                         }
                         if (is_modified(node, value, 0, is_immutable(value))) {

What change are you proposing?

[alexlamsl]

Still confused - function c in the top post doesn't pass the .is_constant_expression() check.

Oops - I've arbitrarily restricted that to AST_Function instead of AST_Lambda:
https://github.com/mishoo/UglifyJS2/blob/c8b6f4733d35db48b5b7e2373264db0d99eb299f/lib/compress.js#L2154-L2169

But yes, basically I proposed relaxing d.scope === node.scope above (only for d.fixed instanceof AST_Defun), then change the .is_constant_expression() to cover AST_Defun as well.

[alexlamsl]

Turns out there are a few other missing pieces with AST_Defun and reduce_vars - working on them now & PR Soon:tm:

[kzc]

Okay, cool. Looking forward to the PR.