Bug in compress transformation of !typeof side_effect()

[kzc]

Incorrect optimizations:

$ echo '!typeof console.log(1);' | uglifyjs -c
WARN: Boolean expression always true [-:1,1]
WARN: Dropping side-effect-free statement [-:1,0]

$ echo 'var a = !typeof console.log(1);' | uglifyjs -c
WARN: Boolean expression always true [-:1,9]
var a=!1;

[kzc]

Likely fix:

--- a/lib/compress.js
+++ b/lib/compress.js
@@ -2310,10 +2310,12 @@ merge(Compressor.prototype, {
                 }
                 break;
               case "typeof":
-                // typeof always returns a non-empty string, thus it's
-                // always true in booleans
-                compressor.warn("Boolean expression always true [{file}:{line},{col}]", self.start);
-                return make_node(AST_True, self);
+                if (!self.expression.has_side_effects(compressor)) {
+                    // typeof always returns a non-empty string, thus it's
+                    // always true in booleans
+                    compressor.warn("Boolean expression always true [{file}:{line},{col}]", self.start);
+                    return make_node(AST_True, self);
+                }
             }
             if (e instanceof AST_Binary && self.operator == "!") {
                 self = best_of(self, e.negate(compressor));

After patch applied:

$ echo '!typeof console.log(1);' | bin/uglifyjs -c
!typeof console.log(1);

$ echo 'var a = !typeof console.log(1);' | bin/uglifyjs -c
var a=!typeof console.log(1);

It's not optimal, but not incorrect.

[rvanvelzen]

Different solution:

diff --git a/lib/compress.js b/lib/compress.js
index 8a08572..11ea86d 100644
--- a/lib/compress.js
+++ b/lib/compress.js
@@ -2313,6 +2313,12 @@ merge(Compressor.prototype, {
                 // typeof always returns a non-empty string, thus it's
                 // always true in booleans
                 compressor.warn("Boolean expression always true [{file}:{line},{col}]", self.start);
+                if (self.expression.has_side_effects(compressor)) {
+                  return make_node(AST_Seq, self, {
+                      car: self.expression,
+                      cdr: make_node(AST_True, self)
+                  });
+                }
                 return make_node(AST_True, self);
             }
             if (e instanceof AST_Binary && self.operator == "!") {

$ echo '!typeof console.log(1);' | bin/uglifyjs -c
WARN: Boolean expression always true [-:1,1]
console.log(1),!1;

$ echo 'var a = !typeof console.log(1);' | bin/uglifyjs -c
WARN: Boolean expression always true [-:1,9]
var a=(console.log(1),!1);

These could probably be optimized further later, but at least the operator is dropped, which is something?

[kzc]

Is it worth optimizing a typeof of an expression with a side effect in a boolean context?

This is so rare in practise and is almost always incorrect code.

Look at the cost/benefit ratio versus the risk of an incorrect assumption.

[rvanvelzen]

Except for typos, I don't believe this case would ever appear in the wild. That having been said, I don't see any reason not to optimize this case.

[kzc]

@rvanvelzen I don't think it's worth the complexity for this rare case, but please feel free to fix as you see fit.