clean up negate_iife

[alexlamsl]

• remove extra tree scanning phase for negate_iife
• negate_iife now only deals with the narrowest form, i.e. IIFE sitting directly under AST_SimpleStatement
• booleans, conditionals etc. will now take care the rest via more accurate accounting

IIRC there was a previous issue raised about some corner cases of negate_iife which the only workaround was to turn the flag off. If anybody remembers what it is I'd like to add it to test cases, since my aim is to improve the correctness of this feature.

Reading material: #640, #1288, #1293

[kzc]

Please add the comments above explaining the clean up into the commit description itself.

Because this PR is a rather invasive change to achieve the same functionality as before (albeit in a more efficient way) it would give reviewers more confidence if as little of the tests are changed as possible. Do not move or reorder tests, change formatting, add or remove semicolons or change names of tests - even if inconsistent. You may add new tests (preferably to the bottom of test files), but do not alter existing tests unless there is a mistake in the test itself.

In general in this code base when changing object literals such as options try to prefer adding a trailing comma. That way new items added to the end of object literals will produce a smaller diff. Never remove trailing commas in existing code.

[kzc]

to improve readability please move is_iife_call function declaration below the return at the end of the function.

[alexlamsl]

Just had a scan through https://github.com/mishoo/UglifyJS2/issues?utf8=%E2%9C%93&q=negate_iife and couldn't find more cases to fix/chase for now.

Currently this PR only strips out ! ~ + -, but as you point out in #1288 (comment) typeof and void can be removed as well.

[kzc]

Your changes seem reasonable, but there are a lot of moving parts.

What does the new argument first_in_statement in AST_Node.negate(compressor, first_in_statement) and best(orig, alt, first_in_statement) do exactly and what's the risk if the caller accidentally neglects to pass it? Is the code resilient enough to handle it and just produce suboptimal output?

[alexlamsl]

AST_Node.negate(compressor, false) (or AST_Node.negate(compressor)) has identical behaviour as before.

AST_Node.negate(compressor, true) differs in the case where extra parentheses are needed for function and object literals appearing as first items in the statement.

Previously there is no tracking of whether an expression under test is first in the statement, e.g. a() in a();, or just part of an expression, e.g. a() in if (a()) {...}, so negate() always assumes the latter and produces sub-optimal results when extra parentheses are not accounted for.

[alexlamsl]

I was actually trying to remove AST_Return from functions which their return values aren't being used.

Spotted an opportunity to insert new logic into negate_iifes(), horror, then back off and decided to clean up negate_iife first.

There is an information loss during optimisations by sequences or conditionals, i.e. turning AST_SimpleStatement into an expression inside AST_Seq or AST_Conditional. Parentheses aside, this also made the return value seemingly used without walking up the tree for a more detailed look.

With this PR statement_to_expression() consolidates those sites of interest so further work can be done.

[kzc]

Thanks for the detailed explanation.

There is an information loss during optimisations by sequences or conditionals, i.e. turning AST_SimpleStatement into an expression inside AST_Seq or AST_Conditional

Yeah, once in sequence form a number of transforms are no longer possible. It would mostly impact suboptimal previously minified expression sequences. But that's beyond the scope of this PR.

LGTM

[alexlamsl]

@kzc I guess you might know about this already, but I just noticed that for test cases with the same name, only the last one will run.

So only the second negate_iife_3 in test/compress/negate-iife.js gets executed.

[kzc]

I just noticed that for test cases with the same name, only the last one will run.

I didn't know that, but it stands to reason because each test is just JS code in the end:

$ echo 'test_name: { options = {}; input: {a(); b();} expected: {a(); b();} }' | uglifyjs -b
test_name: {
    options = {};
    input: {
        a();
        b();
    }
    expected: {
        a();
        b();
    }
}

I would never have looked at the test names for duplication in a review.

When you fix the unintentional duplicate names, do all the tests run correctly?

[alexlamsl]

When you fix the unintentional duplicate names

I only saw this one while doing this PR, and it stuck in my head like a sore thumb so eventually I tested to see if my hypothesis can be proven.

I can do a scan through the tests for duplicate names once I've got this AST_Seq PR done, hopefully soon (writing tests for hoist_vars which seems to be non-existent?!)

[kzc]

Test names are AST_LabeledStatements. test/run-tests.js should be made to error out if a duplicate test name is found within a given test JS file.

tests for hoist_vars which seems to be non-existent?!

As I mentioned in a few PRs, test coverage is lacking for older Uglify functionality.

[kzc]

You have to admit the Uglify test framework is pretty darn clever in its use of JS syntax and its ease of use. The tests don't look like JS, yet they are. You can even run them through uglifyjs -b.

[alexlamsl]

I come to appreciate the beauty of JavaScript as I work on this project, that's for sure 😜

[alexlamsl]

Oh, and I looked at bin/uglifyjs for another issue earlier on today, and saw that -c option values are being parsed by UglifyJS.parser itself. So yeah, very clever indeed. 😅

[kzc]

I come to appreciate the beauty of JavaScript as I work on this project

I learned a lot about JavaScript techniques and the language itself from reading Uglify source code.

You don't truly understand any language until you have to minify it. ;-)