fix multiple nested function substitutions

[alexlamsl]

fixes #2449

[alexlamsl]

@kzc so one thing I thought was safe back in #2427 which turns out not to be the case:

function collectDirectives(node, directives, attrs, maxPriority, ignoreDirective) {
    // ...
    return directives.sort(byPriority);
}
function byPriority(a, b) {
    // ...
}

i.e. byPriority cannot be inlined.

[kzc]

byPriority cannot be inlined.

That's surprising to me. Clearly my understanding of closures is wrong.

What heuristic do you now use to replace AST_Defun with AST_Function? (In words rather than code).

[alexlamsl]

This subtle difference from the example above could be substituted without issue:

function collectDirectives(node, directives, attrs, maxPriority, ignoreDirective) {
    // ...
    return directives.sort(byPriority);
    function byPriority(a, b) {
        // ...
    }
}

The reason should be clear when you consider multiple invocations of collectDirectives().

What heuristic do you now use to replace AST_Defun with AST_Function? (In words rather than code).

(function ↔️ AST_Defun, reference ↔️ AST_SymbolRef)

Common criteria:

• reduce_vars & unused are enabled
• no eval or with
• single reference
• function to reference does not cross any loop boundaries

In addition, if function to reference crosses any scope boundaries:

• reference does not escape its local scope, i.e. no return f or grab(f) etc.
• reference does not reside within function (recursive reference)
• function does not contain any out-of-scope variable references that would conflict with reference's scope

[kzc]

Wow. I have to do some reading on closures.

How confident are you with the current state of this function symbol substitution logic?

[kzc]

function does not contain any out-of-scope variable references that would conflict with reference's scope

That's not immediately obvious and certainly worthy of a comment in the code. Could you point to the place in the code where that is now determined?

[alexlamsl]

How confident are you with the current state of this function symbol substitution logic?

Fairly so.

That's not immediately obvious and certainly worthy of a comment in the code. Could you point to the place in the code where that is now determined?

That would be AST_Lambda.is_constant_expression(scope):
https://github.com/mishoo/UglifyJS2/blob/1127a2caf305ad1df370c34faf7283e53423cc10/lib/compress.js#L2192-L2214

[kzc]

That would be AST_Lambda.is_constant_expression

That's an interesting place for it. So a constant lambda is one that does not have any out of scope references. That makes sense.

Could the fuzzer technically generate test cases for this in its present form?

Are there any implications for harmony block scopes within functions? I guess it'd just have AST_Lambda.is_constant_expression() report false in such cases which would mean no substitution would take place. So be it.

[alexlamsl]

Could the fuzzer technically generate test cases for this in its present form?

I'd think so, seeing that we are getting [Function XXXXXX] in the output, i.e. AST_Lambda.names are being supplied to other expressions.

Are there any implications for harmony block scopes within functions?

We utilise AST_Scope.enclosed array here to detect if a reference is going out of scope, so block-scoped variables should not hinder anything here if AST_Toplevel.figure_out_scope() is doing its job correctly.

[th317erd]

Any idea when this will be published? This is affecting me as well

[kzc]

workaround until next release: -c reduce_vars=false

[JumalDB]

Can you release a new version with this fix? When it will be released?