Better boolean conversion tests.

lib/src/compiler/code_generator.dart

Index: lib/src/compiler/code_generator.dart
diff --git a/lib/src/compiler/code_generator.dart b/lib/src/compiler/code_generator.dart
index 8a97a278c6114a34e3a048de9a9dd36cedc82799..9f551c7b9cb338febf22a170989001a6dc5dd23c 100644
--- a/lib/src/compiler/code_generator.dart
+++ b/lib/src/compiler/code_generator.dart
@@ -456,44 +456,28 @@ class CodeGenerator extends GeneralizingAstVisitor
   }
 
   @override
-  visitAsExpression(AsExpression node) =>
-      _emitCast(node.expression, to: node.type.type);
-
-  /// Emits a cast and/or a null check (i.e. a cast to a non-null type).
-  JS.Expression _emitCast(Expression fromExpr,
-      {DartType to, bool checkNull: false}) {
-    var jsFrom = _visit(fromExpr);
+  visitAsExpression(AsExpression node) {
+    Expression fromExpr = node.expression;
     var from = getStaticType(fromExpr);
+    var to = node.type.type;
 
-    JS.Expression maybeCheckNull(JS.Expression jsExpr) {
-      if (checkNull && isNullable(fromExpr)) {
-        return js.call('dart.notNull(#)', jsExpr);
-      }
-      return jsExpr;
-    }
+    var jsFrom = _visit(fromExpr);
 
     // Skip the cast if it's not needed.
-    if (to == null || rules.isSubtypeOf(from, to)) {
-      return maybeCheckNull(jsFrom);
-    }
+    if (rules.isSubtypeOf(from, to)) return jsFrom;
 
     // All Dart number types map to a JS double.
     if (_isNumberInJS(from) && _isNumberInJS(to)) {
       // Make sure to check when converting to int.
       if (from != types.intType && to == types.intType) {
-        // TODO(jmesserly): fuse this with notNull check.

[Jennifer Messerly, 2016/05/19 17:45:38]

Why remove this TODO? We still want to be smarter with numeric checks, right?
Rather than emit `notNull(asInt(`

[sra1, 2016/05/19 18:57:56]

I'll add it back.

-        return maybeCheckNull(js.call('dart.asInt(#)', [jsFrom]));
+        return js.call('dart.asInt(#)', jsFrom);
       }
 
       // A no-op in JavaScript.
-      return maybeCheckNull(jsFrom);
-    }
-
-    if (to == types.boolType && checkNull) {
-      return js.call('dart.test(#)', _visit(fromExpr));
+      return jsFrom;
     }
 
-    return maybeCheckNull(js.call('dart.as(#, #)', [jsFrom, _emitType(to)]));
+    return js.call('dart.as(#, #)', [jsFrom, _emitType(to)]);
   }
 
   @override
@@ -3128,10 +3112,9 @@ class CodeGenerator extends GeneralizingAstVisitor
 
   JS.Expression notNull(Expression expr) {
     if (expr == null) return null;
-    if (expr is AsExpression) {
-      return _emitCast(expr.expression, to: expr.type.type, checkNull: true);
-    }
-    return _emitCast(expr, checkNull: true);
+    var jsExpr = _visit(expr);
+    if (!isNullable(expr)) return jsExpr;
+    return js.call('dart.notNull(#)', jsExpr);
   }
 
   @override
@@ -3839,7 +3822,7 @@ class CodeGenerator extends GeneralizingAstVisitor
   @override
   visitConditionalExpression(ConditionalExpression node) {
     return js.call('# ? # : #', [
-      notNull(node.condition),
+      _visitTest(node.condition),
       _visit(node.thenExpression),
       _visit(node.elseExpression)
     ]);
@@ -3881,8 +3864,8 @@ class CodeGenerator extends GeneralizingAstVisitor
 
   @override
   JS.If visitIfStatement(IfStatement node) {
-    return new JS.If(notNull(node.condition), _visitScope(node.thenStatement),
-        _visitScope(node.elseStatement));
+    return new JS.If(_visitTest(node.condition),
+        _visitScope(node.thenStatement), _visitScope(node.elseStatement));
   }
 
   @override
@@ -3891,18 +3874,18 @@ class CodeGenerator extends GeneralizingAstVisitor
     if (init == null) init = _visit(node.variables);
     var update = _visitListToBinary(node.updaters, ',');
     if (update != null) update = update.toVoidExpression();
-    return new JS.For(
-        init, notNull(node.condition), update, _visitScope(node.body));
+    var condition = node.condition == null ? null : _visitTest(node.condition);
+    return new JS.For(init, condition, update, _visitScope(node.body));
   }
 
   @override
   JS.While visitWhileStatement(WhileStatement node) {
-    return new JS.While(notNull(node.condition), _visitScope(node.body));
+    return new JS.While(_visitTest(node.condition), _visitScope(node.body));
   }
 
   @override
   JS.Do visitDoStatement(DoStatement node) {
-    return new JS.Do(_visitScope(node.body), notNull(node.condition));
+    return new JS.Do(_visitScope(node.body), _visitTest(node.condition));
   }
 
   @override
@@ -4239,6 +4222,39 @@ class CodeGenerator extends GeneralizingAstVisitor
         _visitList(nodes) as List<JS.Expression>, operator);
   }
 
+  /// Generates an expression for a boolean conversion context (if, while, &&,
+  /// etc.), where conversions and null checks are implemented via `dart.test`
+  /// to give a more helpful message.
+  // TODO(sra): When nullablility is available earlier, it would be cleaner to
+  // build an input AST where the boolean conversion is a single AST node.
+  JS.Expression _visitTest(Expression node) {
+    JS.Expression finish(JS.Expression result) {
+      return annotate(result, node);
+    }
+    if (node is PrefixExpression && node.operator.lexeme == '!') {
+      return finish(js.call('!#', _visitTest(node.operand)));
+    }
+    if (node is BinaryExpression) {
+      JS.Expression shortCircuit(String code) {
+        return finish(js.call(code,
+            [_visitTest(node.leftOperand), _visitTest(node.rightOperand)]));
+      }
+      var op = node.operator.type.lexeme;
+      if (op == '&&') return shortCircuit('# && #');
+      if (op == '||') return shortCircuit('# || #');
+    }
+    // Offset 0 from start of file is syntactically impossible for normal code.
+    // TODO(sra): Find a better way to recognize reified coercion, since we
+    // can't set the isSynthetic attribute.

[Jennifer Messerly, 2016/05/19 17:45:38]

FYI -- we can send a patch to Analyzer if we need a tweak to how isSynthetic
works. That's always an option.

(But definitely good to work around it first from DDC side first, as patches to
Analyzer take longer to propagate)

[sra1, 2016/05/19 18:57:56]

Acknowledged.

+    if (node is AsExpression && node.asOperator.offset == 0) {
+      assert(node.staticType == types.boolType);
+      return js.call('dart.test(#)', _visit(node.expression));
+    }
+    JS.Expression result = _visit(node);
+    if (isNullable(node)) result = js.call('dart.test(#)', result);
+    return result;
+  }
+
   /// Like [_emitMemberName], but for declaration sites.
   ///
   /// Unlike call sites, we always have an element available, so we can use it