Better casting of for-in loops.

pkg/analyzer/lib/src/task/strong/checker.dart

Index: pkg/analyzer/lib/src/task/strong/checker.dart
diff --git a/pkg/analyzer/lib/src/task/strong/checker.dart b/pkg/analyzer/lib/src/task/strong/checker.dart
index 53164a3323806b8b8ef5967fc34baf2a1a27210e..dfbff50b7a4ce36c2ceff49147a39a2d46c74061 100644
--- a/pkg/analyzer/lib/src/task/strong/checker.dart
+++ b/pkg/analyzer/lib/src/task/strong/checker.dart
@@ -327,19 +327,39 @@ class CodeChecker extends RecursiveAstVisitor {
 
   @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);
   }
 
@@ -595,8 +615,8 @@ class CodeChecker extends RecursiveAstVisitor {
             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);
@@ -663,18 +683,22 @@ class CodeChecker extends RecursiveAstVisitor {
     }
   }
 
-  /// 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 [from] is omitted, uses the static type of [expr].
   ///
-  /// If [expr] does not require a downcast because it is not related to [toT]
+  /// 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.
@@ -682,14 +706,14 @@ class CodeChecker extends RecursiveAstVisitor {
     // 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;
     }
 
@@ -703,8 +727,8 @@ class CodeChecker extends RecursiveAstVisitor {
     // 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));
     }
   }