Negated IIFE in if body produces superfluous !

[rvanvelzen]

Test case (that currently fails):

regression_negate_iife_in_if: {
    options = {
        negate_iife: true,
        conditionals: true,
    }
    input: {
        if (!x) {
            (function () {
                x = {};
            })();
        }
    }
    expect: {
        x || function () {
            x = {};
        }();
    }
}

[kzc]

$ echo 'if(!x){(function(){x={}})()}' | bin/uglifyjs -c
x||!function(){x={}}();

Luckily the code produced still functions correctly.

Seems simple enough to remedy. The IIFE ! transform should only be applied if the IIFE AST_Call is a statement. Not a subexpression.

[kzc]

I see what's going on. The IIFE ! transform in the previous example was already applied before the if transform. Case in point, it's not applied here:

$ echo 'x||function(){x={}}();' | bin/uglifyjs -c
x||function(){x={}}();

So any unary could be stripped only if it is on the right side of a || or a && operator that happens to be a statement since its result is not used.

[kzc]

So any unary could be stripped only if it is on the right side of a || or a && operator that happens to be a statement since its result is not used.

To be more specific: some AST_UnaryPrefixes could be stripped in that situation:

!
~
-
+

[rvanvelzen]

At first I was thinking about ways to skip adding the ! at all, but soon came up with the same idea.

In locations where the result of such an expression is not used, those prefix operators could be dropped. This has a some complications (e.g. which locations?) but is entirely tractable.

[kzc]

Edit: patch was corrected.

--- a/lib/compress.js
+++ b/lib/compress.js
@@ -2430,6 +2430,14 @@ merge(Compressor.prototype, {
             break;
         }
         if (compressor.option("conditionals")) {
+            if (compressor.parent() instanceof AST_SimpleStatement
+                && (self.operator == "&&" || self.operator == "||")
+                && self.right instanceof AST_UnaryPrefix
+                && self.right.operator != "delete") {
+                // x && +y;  --->  x && y;
+                // x || +y;  --->  x || y;
+                self.right = self.right.expression;
+            }
             if (self.operator == "&&") {
                 var ll = self.left.evaluate(compressor);
                 if (ll.length > 1) {

Seems to work:

$ echo 'if(!x){(function(){x={}})()}' | bin/uglifyjs -c
x||function(){x={}}();

$ echo 'if(x){(function(){x={}})()}' | bin/uglifyjs -c
x&&function(){x={}}();

$ echo 'x && void y();' | bin/uglifyjs -c
x&&y();

$ echo 'x || ~y();' | bin/uglifyjs -c
x||y();

$ echo 'var a = x || ~y();' | bin/uglifyjs -c
var a=x||~y();

$ echo 'x || delete y();' | bin/uglifyjs -c
x||delete y();

[kzc]

@rvanvelzen The fix probably belongs in OPT(AST_SimpleStatement) rather than OPT(AST_Binary). It's sort of arbitrary where the code goes, but I think that would make it clearer. Should still be within if (compressor.option("conditionals") though.

If you want to put together a PR with the test cases above, go ahead.

[kzc]

I missed the fact that operators "--" and "++" modify the expression.

Corrected patch:

--- a/lib/compress.js
+++ b/lib/compress.js
@@ -2355,6 +2355,7 @@ merge(Compressor.prototype, {
     });

     var commutativeOperators = makePredicate("== === != !== * & | ^");
+    var UNARY_PREFIX_WITHOUT_SIDE_EFFECTS_ON_EXPRESSION = makePredicate("! ~ - + void typeof");

     OPT(AST_Binary, function(self, compressor){
         function reverse(op, force) {
@@ -2430,6 +2431,14 @@ merge(Compressor.prototype, {
             break;
         }
         if (compressor.option("conditionals")) {
+            if (compressor.parent() instanceof AST_SimpleStatement
+                && (self.operator == "&&" || self.operator == "||")
+                && self.right instanceof AST_UnaryPrefix
+                && UNARY_PREFIX_WITHOUT_SIDE_EFFECTS_ON_EXPRESSION(self.right.operator)) {
+                // x && +y;  --->  x && y;
+                // x || +y;  --->  x || y;
+                self.right = self.right.expression;
+            }
             if (self.operator == "&&") {
                 var ll = self.left.evaluate(compressor);
                 if (ll.length > 1) {

New test cases:

$ echo 'x && ++y.p;' | bin/uglifyjs -c
x&&++y.p;

$ echo 'x && --y.p;' | bin/uglifyjs -c
x&&--y.p;

$ echo 'x && void y.p;' | bin/uglifyjs -c
x&&y.p;

$ echo 'x && typeof y.p;' | bin/uglifyjs -c
x&&y.p;

$ echo 'x && delete y.p;' | bin/uglifyjs -c
x&&delete y.p;

$ echo 'x && +y.p;' | bin/uglifyjs -c
x&&y.p;

$ echo 'x && -y.p;' | bin/uglifyjs -c
x&&y.p;

$ echo 'x && !y.p;' | bin/uglifyjs -c
x&&y.p;

$ echo 'x && ~y.p;' | bin/uglifyjs -c
x&&y.p;

[rvanvelzen]

Strictly speaking, + and - have the side-effect of calling valueOf() on the subject. Not that I believe that to be a problem (since side-effects in such methods are generally avoided) but it's worth mentioning.

[kzc]

You're right of course, but I'll just point to the assumptions that Google Closure makes:

https://github.com/google/closure-compiler/wiki/toString-and-valueOf

https://github.com/google/closure-compiler/wiki/Compiler-Assumptions

[kzc]

Just so there's no confusion, I'm not putting together a PR.

[kzc]

Off topic...

Regarding the valueOf potential side effect thing, I wonder if uglify should support a comment directive to not run the compressor on some code or a function.