dart2js cps: Use JS semantics for logical operators in the tree IR.

pkg/compiler/lib/src/tree_ir/optimization/logical_rewriter.dart

Index: pkg/compiler/lib/src/tree_ir/optimization/logical_rewriter.dart
diff --git a/pkg/compiler/lib/src/tree_ir/optimization/logical_rewriter.dart b/pkg/compiler/lib/src/tree_ir/optimization/logical_rewriter.dart
index aba10f4fc9967a4bee301471a4a6d7d3d50a6999..bf31cd56464800d77b7487408e2cfc452a201557 100644
--- a/pkg/compiler/lib/src/tree_ir/optimization/logical_rewriter.dart
+++ b/pkg/compiler/lib/src/tree_ir/optimization/logical_rewriter.dart
@@ -8,8 +8,6 @@ import '../tree_ir_nodes.dart';
 import 'optimization.dart' show Pass;
 import '../../constants/values.dart' as values;
 
-// TODO(asgerf): Update this class to use JS semantics for && and ||.
-
 /// Rewrites logical expressions to be more compact in the Tree IR.
 ///
 /// In this class an expression is said to occur in "boolean context" if
@@ -49,16 +47,10 @@ import '../../constants/values.dart' as values;
 ///
 ///   x ? true : false  ==>  !!x
 ///
-/// If an operand is known to be a boolean, we can introduce a logical operator:
-///
-///   x ? y : false  ==>  x && y   (if y is known to be a boolean)
+/// If the possible falsy values of the condition are known, we can sometimes
+/// introduce a logical operator:
 ///
-/// The following sequence of rewrites demonstrates the merit of these rules:
-///
-///   x ? (y ? true : false) : false
-///   x ? !!y : false   (double negation introduced by [toBoolean])
-///   x && !!y          (!!y validated by [isBooleanValued])
-///   x && y            (double negation removed by [putInBooleanContext])
+///   !x ? y : false  ==>  !x && y
 ///
 class LogicalRewriter extends RecursiveTransformer
                       implements Pass {
@@ -94,7 +86,7 @@ class LogicalRewriter extends RecursiveTransformer
 
   Statement visitIf(If node) {
     // If one of the branches is empty (i.e. just a fallthrough), then that
-    // branch should preferrably be the 'else' so we won't have to print it.
+    // branch should preferably be the 'else' so we won't have to print it.
     // In other words, we wish to perform this rewrite:
     //   if (E) {} else {S}
     //     ==>
@@ -143,6 +135,31 @@ class LogicalRewriter extends RecursiveTransformer
     return toBoolean(makeCondition(node.operand, false, liftNots: false));
   }
 
+  /// True if the only possible falsy return value of [condition] is [value].
+  ///
+  /// If [value] is `null` or a truthy value, false is returned. This is to make
+  /// pattern matching more convenient.
+  bool isOnlyFalsyValue(Expression condition, values.ConstantValue value) {

[Kevin Millikin (Google), 2015/06/16 12:30:54]

"is" is weird with the extra argument.  Maybe matchOnlyFalsyValue or the like is
a better clue that something more complicated is happening.

[asgerf, 2015/06/16 12:52:12]

"matchOnly" seems wrong, it could match other "patterns" too, so I settled for
"matchesFalsyValue".

+    if (value == null) return false;
+    // TODO(asgerf): Here we could really use some more type information,
+    //               this is just the best we can do at the moment.
+    return isBooleanValued(condition) && value.isFalse;
+  }
+
+  /// True if the only possible truthy return value of [condition] is [value].
+  ///
+  /// If [value] is `null` or a falsy value, false is returned. This is to make
+  /// pattern matching more convenient.
+  bool isOnlyTruthyValue(Expression condition, values.ConstantValue value) {
+    if (value == null) return false;
+    // TODO(asgerf): Again, more type information could really beef this up.
+    return isBooleanValued(condition) && value.isTrue;
+  }
+
+  values.ConstantValue getConstant(Expression exp) {
+    return exp is Constant ? exp.value : null;
+  }
+
   Expression visitConditional(Conditional node) {
     // node.condition will be visited after the then and else parts, because its
     // polarity depends on what rewrite we use.
@@ -161,29 +178,32 @@ class LogicalRewriter extends RecursiveTransformer
       return toBoolean(makeCondition(node.condition, false, liftNots: false));
     }
 
-    // x ? y : false ==> x && y  (if y is known to be a boolean)
-    if (isBooleanValued(node.thenExpression) && isFalse(node.elseExpression)) {
+    // x ? y : false ==> x && y  (if x is truthy or false)
+    // x ? y : null  ==> x && y  (if x is truthy or null)
+    // x ? y : 0     ==> x && y  (if x is truthy or zero) (and so on...)
+    if (isOnlyFalsyValue(node.condition, getConstant(node.elseExpression))) {
       return new LogicalOperator.and(
-          makeCondition(node.condition, true, liftNots:false),
-          putInBooleanContext(node.thenExpression));
+          visitExpression(node.condition),
+          node.thenExpression);
     }
-    // x ? y : true ==> !x || y  (if y is known to be a boolean)
-    if (isBooleanValued(node.thenExpression) && isTrue(node.elseExpression)) {
+    // x ? true : y ==> x || y  (if x is falsy or true)
+    // x ? 1    : y ==> x || y  (if x is falsy or one) (and so on...)
+    if (isOnlyTruthyValue(node.condition, getConstant(node.thenExpression))) {
       return new LogicalOperator.or(
-          makeCondition(node.condition, false, liftNots: false),
-          putInBooleanContext(node.thenExpression));
+          visitExpression(node.condition),
+          node.elseExpression);
     }
-    // x ? true : y ==> x || y  (if y if known to be boolean)
-    if (isBooleanValued(node.elseExpression) && isTrue(node.thenExpression)) {
+    // x ? y : true ==> !x || y
+    if (isTrue(node.elseExpression)) {
       return new LogicalOperator.or(
-          makeCondition(node.condition, true, liftNots: false),
-          putInBooleanContext(node.elseExpression));
+          toBoolean(makeCondition(node.condition, false, liftNots: false)),
+          node.thenExpression);
     }
-    // x ? false : y ==> !x && y  (if y is known to be a boolean)
-    if (isBooleanValued(node.elseExpression) && isFalse(node.thenExpression)) {
+    // x ? false : y ==> !x && y
+    if (isFalse(node.thenExpression)) {
       return new LogicalOperator.and(
-          makeCondition(node.condition, false, liftNots: false),
-          putInBooleanContext(node.elseExpression));
+          toBoolean(makeCondition(node.condition, false, liftNots: false)),
+          node.elseExpression);
     }
 
     node.condition = makeCondition(node.condition, true);
@@ -200,8 +220,8 @@ class LogicalRewriter extends RecursiveTransformer
   }
 
   Expression visitLogicalOperator(LogicalOperator node) {
-    node.left = makeCondition(node.left, true);
-    node.right = makeCondition(node.right, true);
+    node.left = visitExpression(node.left);
+    node.right = visitExpression(node.right);
     return node;
   }
 
@@ -253,16 +273,6 @@ class LogicalRewriter extends RecursiveTransformer
     }
   }
 
-  /// Rewrite an expression that was originally processed in a non-boolean
-  /// context.
-  Expression putInBooleanContext(Expression e) {
-    if (e is Not && e.operand is Not) {
-      return (e.operand as Not).operand;
-    } else {
-      return e;
-    }
-  }
-
   /// Forces a boolean conversion of the given expression.
   Expression toBoolean(Expression e) {
     if (isBooleanValued(e))
@@ -275,7 +285,7 @@ class LogicalRewriter extends RecursiveTransformer
   /// context where its result is immediately subject to boolean conversion.
   /// If [polarity] if false, the negated condition will be created instead.
   /// If [liftNots] is true (default) then Not expressions will be lifted toward
-  /// the root the condition so they can be eliminated by the caller.
+  /// the root of the condition so they can be eliminated by the caller.
   Expression makeCondition(Expression e, bool polarity, {bool liftNots:true}) {
     if (e is Not) {
       // !!E ==> E