fix infinite recursion in function inlining

[kzc]

fixes #2442

[kzc]

@alexlamsl Something is odd here. The tests recursive_inlining_3, recursive_inlining_4 and recursive_inlining_5 pass without this PR on current master and v3.1.7. That's fine, but unexpected. The tests recursive_inlining_1 and recursive_inlining_2 do require this PR to pass. I wonder if there are recursive scenarios not covered.

Feel free to either take over this PR, or close and replace it with a fix of your own. In either case, the new tests can be used.

[alexlamsl]

@kzc thanks for taking a stab at this. If I understand the issue correctly, before #2427, function inlining only works within the same scope, thus avoiding this infinite recursion problem beautifully.

Let's see if I can fix that root cause, but in any case I truly appreciate all the test cases here and shall be stealing borrowing them if I do come up with a better solution 😉

[kzc]

The more I think about it, the AST must be damaged or cross linked in some way from function inlining, as ASTs are finite and infinite recursion should not be an issue in AST_Node.clone(). This PR does not address the underlying AST problem.

[kzc]

In the meantime I think it'd be a good idea to npm deprecate uglify-js@3.1.7 and uglify-uglify-es@3.1.7.

[alexlamsl]

In the meantime I think it'd be a good idea to npm deprecate uglify-js@3.1.7 and uglify-uglify-es@3.1.7.

Fair point - let me dig up the docs and do that before resume work on the new PR.

Edit: done.

[alexlamsl]

Something is odd here. The tests recursive_inlining_3, recursive_inlining_4 and recursive_inlining_5 pass without this PR on current master and v3.1.7.

You need to have a reference cycle of functions that are all .single_use to trigger this bug, which those three cases do not possess. No harm keeping them around though 😉

[kzc]

You need to have a reference cycle of functions that are all .single_use to trigger this bug, which those three cases do not possess.

Ah - of course! That riddle is solved.

Can single-use function cycles exist in non-dead code? I suspect not.

No harm keeping them around though

Indeed - I was impressed by how the generated code changed in the tests when calling 1, 2 and 3 of the recursive functions from main.

[alexlamsl]

Can single-use function cycles exist in non-dead code? I suspect not.

I don't think so either - see #2446 (comment)

[kzc]

It makes you wonder what the code in the wild triggering this issue actually did or how it was bundled in the first place.

[alexlamsl]

It could be something like:

function factorial(n) {
    return n < 1 ? 1 : factorial(n - 1);
}
if (DEBUG) console.log(factorial(10));

and then compress with global_defs: { DEBUG: false }? 🤔

[kzc]

Yeah, unlikely, but I guess. It's a real head-scratcher.

[alexlamsl]

Yeah, unlikely, but I guess. It's a real head-scratcher.

You've saved my hair - please don't make me feel bad by losing any of yours 😅