Better boolean conversion tests.

lib/src/compiler/code_generator.dart

 // Copyright (c) 2015, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 import 'dart:collection' show HashMap, HashSet;
 import 'dart:math' show min, max;
 
 import 'package:analyzer/analyzer.dart' hide ConstantEvaluator;
 import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/dart/ast/token.dart' show Token, TokenType;
@@ skipping 438 matching lines @@
       if (export is ClassElement && export.typeParameters.isNotEmpty) {
         // Export the generic name as well.
         // TODO(jmesserly): revisit generic classes
         emitExport(export, suffix: r'$');
       }
       emitExport(export);
     }
   }
 
   @override
-  visitAsExpression(AsExpression node) =>
-      _emitCast(node.expression, to: node.type.type);
+  visitAsExpression(AsExpression node) {
+    Expression fromExpr = node.expression;
+    var from = getStaticType(fromExpr);
+    var 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);
-    var from = getStaticType(fromExpr);
-
-    JS.Expression maybeCheckNull(JS.Expression jsExpr) {
-      if (checkNull && isNullable(fromExpr)) {
-        return js.call('dart.notNull(#)', jsExpr);
-      }
-      return jsExpr;
-    }
 
     // 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);
+      return jsFrom;
     }
 
-    if (to == types.boolType && checkNull) {
-      return js.call('dart.test(#)', _visit(fromExpr));
-    }
-
-    return maybeCheckNull(js.call('dart.as(#, #)', [jsFrom, _emitType(to)]));
+    return js.call('dart.as(#, #)', [jsFrom, _emitType(to)]);
   }
 
   @override
   visitIsExpression(IsExpression node) {
     // Generate `is` as `dart.is` or `typeof` depending on the RHS type.
     JS.Expression result;
     var type = node.type.type;
     var lhs = _visit(node.expression);
     var typeofName = _jsTypeofName(type);
     if (typeofName != null) {
@@ skipping 2614 matching lines @@
   bool typeIsPrimitiveInJS(DartType t) =>
       _isNumberInJS(t) || t == types.boolType;
 
   bool binaryOperationIsPrimitive(DartType leftT, DartType rightT) =>
       typeIsPrimitiveInJS(leftT) && typeIsPrimitiveInJS(rightT);
 
   bool unaryOperationIsPrimitive(DartType t) => typeIsPrimitiveInJS(t);
 
   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
   JS.Expression visitBinaryExpression(BinaryExpression node) {
     var op = node.operator;
     var left = node.leftOperand;
     var right = node.rightOperand;
 
     var leftType = getStaticType(left);
     var rightType = getStaticType(right);
@@ skipping 687 matching lines @@
   Expression _getTarget(node) {
     assert(node is IndexExpression ||
         node is PropertyAccess ||
         node is MethodInvocation);
     return node.isCascaded ? _cascadeTarget : node.target;
   }
 
   @override
   visitConditionalExpression(ConditionalExpression node) {
     return js.call('# ? # : #', [
-      notNull(node.condition),
+      _visitTest(node.condition),
       _visit(node.thenExpression),
       _visit(node.elseExpression)
     ]);
   }
 
   @override
   visitThrowExpression(ThrowExpression node) {
     var expr = _visit(node.expression);
     if (node.parent is ExpressionStatement) {
       return js.statement('dart.throw(#);', expr);
@@ skipping 21 matching lines @@
     var result = _visit(stmt);
     if (result is JS.ExpressionStatement &&
         result.expression is JS.VariableDeclarationList) {
       return new JS.Block([result]);
     }
     return result;
   }
 
   @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
   JS.For visitForStatement(ForStatement node) {
     var init = _visit(node.initialization);
     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
   JS.Statement visitForEachStatement(ForEachStatement node) {
     if (node.awaitKeyword != null) {
       return _emitAwaitFor(node);
     }
 
     var init = _visit(node.identifier);
     if (init == null) {
@@ skipping 316 matching lines @@
     return result;
   }
 
   /// Visits a list of expressions, creating a comma expression if needed in JS.
   JS.Expression _visitListToBinary(List<Expression> nodes, String operator) {
     if (nodes == null || nodes.isEmpty) return null;
     return new JS.Expression.binary(
         _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
   /// directly rather than computing the relevant options for [_emitMemberName].
   JS.Expression _elementMemberName(ExecutableElement e, {bool useExtension}) {
     String name;
     if (e is PropertyAccessorElement) {
       name = e.variable.name;
     } else {
       name = e.name;
@@ skipping 237 matching lines @@
 }
 
 bool isLibraryPrefix(Expression node) =>
     node is SimpleIdentifier && node.staticElement is PrefixElement;
 
 LibraryElement _getLibrary(AnalysisContext c, String uri) =>
     c.computeLibraryElement(c.sourceFactory.forUri(uri));
 
 bool _isDartRuntime(LibraryElement l) =>
     l.isInSdk && l.source.uri.toString() == 'dart:_runtime';