Better casting of for-in loops.

pkg/analyzer/lib/src/task/strong/checker.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.
 
 // TODO(jmesserly): this was ported from package:dev_compiler, and needs to be
 // refactored to fit into analyzer.
 library analyzer.src.task.strong.checker;
 
 import 'package:analyzer/analyzer.dart';
 import 'package:analyzer/dart/ast/ast.dart';
@@ skipping 309 matching lines @@
         var staticInfo =
             new InvalidParameterDeclaration(rules, node, fieldType);
         _recordMessage(staticInfo);
       }
     }
     node.visitChildren(this);
   }
 
   @override
   void visitForEachStatement(ForEachStatement node) {
-    // Check that the expression is an Iterable.
-    var expr = node.iterable;
-    var iterableType = node.awaitKeyword != null
-        ? typeProvider.streamType
-        : typeProvider.iterableType;
     var loopVariable = node.identifier != null
         ? node.identifier
         : node.loopVariable?.identifier;
+
+    // Safely handle malformed statements.
     if (loopVariable != null) {
-      var iteratorType = loopVariable.staticType;
-      var checkedType = iterableType.instantiate([iteratorType]);
-      checkAssignment(expr, checkedType);
+      // Find up the element type of the sequence.

[vsm, 2016/03/21 21:25:20]

s/up//

[Bob Nystrom, 2016/03/21 22:44:55]

Done.

+      var sequenceInterface = node.awaitKeyword != null
+          ? typeProvider.streamType
+          : typeProvider.iterableType;
+      var iterableType = _getStaticType(node.iterable);
+      var elementType =
+          rules.mostSpecificTypeArgument(iterableType, sequenceInterface);
+
+      // If the sequence is dynamic, cast it to Iterable<dynamic> or
+      // Stream<dynamic>. Then we'll do a separate cast of the dynamic element
+      // to the variable's type.
+      if (iterableType.isDynamic) {

[vsm, 2016/03/21 21:25:20]

Should we just do checkAssignment(node.iterable, sequenceInterface)?  That will
cover the case where sequence has type Object as well.  The previous code
allowed that.

[Bob Nystrom, 2016/03/21 22:44:55]

I don't think we want that because we don't want a cast on the iterable itself
in cases like:

for (int i in <num>[]) { ... }

Casting <num>[] to Iterable<int> is redundant since we'll be adding a num->int
cast to i itself. Instead, we just want to do an isSubtypeOf() check to see if
the type is a supertype of Iterable. Did that and added some tests.

[vsm, 2016/03/21 22:50:35]

It'd be a cast to Iterable, not Iterable<int> - i.e., using sequenceType
(instantiated with dynamic is that's needed here).

[Bob Nystrom, 2016/03/21 23:00:44]

OK, then we're thinking the same thing. That's what I did in the revised code. I
just didn't go through checkAssignment() because that will also report an error,
which isn't what we want here.

+        _recordMessage(DownCast.create(
+            rules, node.iterable, iterableType, sequenceInterface));
+        elementType = DynamicTypeImpl.instance;
+      }
+
+      // If the sequence doesn't implement the interface at all, [ErrorVerifier]
+      // will report the error, so ignore it here.
+      if (elementType != null) {
+        // Insert a cast from the sequence's element type to the loop variable's
+        // if needed.
+        _checkDowncast(loopVariable, _getStaticType(loopVariable),
+            from: elementType);
+      }
     }
+
     node.visitChildren(this);
   }
 
   @override
   void visitForStatement(ForStatement node) {
     if (node.condition != null) {
       checkBoolean(node.condition);
     }
     node.visitChildren(this);
   }
@@ skipping 235 matching lines @@
       var returnType = _specializedBinaryReturnType(
           op, lhsType, rhsType, functionType.returnType);
 
       if (!rules.isSubtypeOf(returnType, lhsType)) {
         final numType = typeProvider.numType;
         // Try to fix up the numerical case if possible.
         if (rules.isSubtypeOf(lhsType, numType) &&
             rules.isSubtypeOf(lhsType, rhsType)) {
           // This is also slightly different from spec, but allows us to keep
           // compound operators in the int += num and num += dynamic cases.
-          staticInfo = DownCast.create(
-              rules, expr.rightHandSide, rhsType, lhsType);
+          staticInfo =
+              DownCast.create(rules, expr.rightHandSide, rhsType, lhsType);
           rhsType = lhsType;
         } else {
           staticInfo = new StaticTypeError(rules, expr, lhsType);
         }
         _recordMessage(staticInfo);
       }
 
       // Check the rhs type
       if (staticInfo is! CoercionInfo) {
         var paramType = paramTypes.first;
@@ skipping 46 matching lines @@
         op.type == TokenType.MINUS_MINUS) {
       if (_isDynamicTarget(node.operand)) {
         _recordDynamicInvoke(node, node.operand);
       }
       // For ++ and --, even if it is not dynamic, we still need to check
       // that the user defined method accepts an `int` as the RHS.
       // We assume Analyzer has done this already.
     }
   }
 
-  /// Records a [DownCast] of [expr] to [toT], if there is one.
+  /// Records a [DownCast] of [expr] from [from] to [to], if there is one.
   ///
-  /// If [expr] does not require a downcast because it is not related to [toT]
+  /// If [from] is omitted, uses the static type of [expr].
+  ///
+  /// If [expr] does not require a downcast because it is not related to [to]
   /// or is already a subtype of it, does nothing.
-  void _checkDowncast(Expression expr, DartType toT) {
-    DartType fromT = _getStaticType(expr);
+  void _checkDowncast(Expression expr, DartType to, {DartType from}) {
+    if (from == null) {
+      from = _getStaticType(expr);
+    }
 
     // We can use anything as void.
-    if (toT.isVoid) return;
+    if (to.isVoid) return;
 
     // fromT <: toT, no coercion needed.
-    if (rules.isSubtypeOf(fromT, toT)) return;
+    if (rules.isSubtypeOf(from, to)) return;
 
     // TODO(vsm): We can get rid of the second clause if we disallow
     // all sideways casts - see TODO below.
     // -------
     // Note: a function type is never assignable to a class per the Dart
     // spec - even if it has a compatible call method.  We disallow as
     // well for consistency.
-    if ((fromT is FunctionType && rules.getCallMethodType(toT) != null) ||
-        (toT is FunctionType && rules.getCallMethodType(fromT) != null)) {
+    if ((from is FunctionType && rules.getCallMethodType(to) != null) ||
+        (to is FunctionType && rules.getCallMethodType(from) != null)) {
       return;
     }
 
     // Downcast if toT <: fromT
-    if (rules.isSubtypeOf(toT, fromT)) {
-      _recordMessage(DownCast.create(rules, expr, fromT, toT));
+    if (rules.isSubtypeOf(to, from)) {
+      _recordMessage(DownCast.create(rules, expr, from, to));
       return;
     }
 
     // TODO(vsm): Once we have generic methods, we should delete this
     // workaround.  These sideways casts are always ones we warn about
     // - i.e., we think they are likely to fail at runtime.
     // -------
     // Downcast if toT <===> fromT
     // The intention here is to allow casts that are sideways in the restricted
     // type system, but allowed in the regular dart type system, since these
     // are likely to succeed.  The canonical example is List<dynamic> and
     // Iterable<T> for some concrete T (e.g. Object).  These are unrelated
     // in the restricted system, but List<dynamic> <: Iterable<T> in dart.
-    if (fromT.isAssignableTo(toT)) {
-      _recordMessage(DownCast.create(rules, expr, fromT, toT));
+    if (from.isAssignableTo(to)) {
+      _recordMessage(DownCast.create(rules, expr, from, to));
     }
   }
 
   // Produce a coercion which coerces something of type fromT
   // to something of type toT.
   // Returns the error coercion if the types cannot be coerced
   // according to our current criteria.
   /// Gets the expected return type of the given function [body], either from
   /// a normal return/yield, or from a yield*.
   DartType _getExpectedReturnType(FunctionBody body, {bool yieldStar: false}) {
@@ skipping 553 matching lines @@
     } while (!current.isObject && !visited.contains(current));
   }
 
   void _recordMessage(StaticInfo info) {
     if (info == null) return;
     var error = info.toAnalysisError();
     if (error.errorCode.errorSeverity == ErrorSeverity.ERROR) _failure = true;
     _reporter.onError(error);
   }
 }