Analyzer: no implicit cast errors for most downcasts to iterable

[MichaelRFairhurst]

  Object o;                                                                      
  dynamic d;                                                                     
  for (var x in o) {} // no error                                                
  for (int x in d) {} // no error                                                
  for (int x in o) {} // error                                                   
  for (var x in d) {} // no error

The same behavior occurs for spread collections.

I would expect that all of these would be flagged based on the fact that all four of these assignments are flagged:

  Iterable<dynamic> idd = d;                                                     
  Iterable<int> iid = d;                                                         
  Iterable<dynamic> ido = o;                                                     
  Iterable<int> iio = o;

[eernstg]

@lrhn and I just discussed the extent to which we can insist that a desugaring specification can be used to determine compile-time errors. I would like to say that the rewritten code shows whether there are any errors, but the original code should (of course) be the sole reference that error messages can refer to.

But for-in desugars to a while statement, and in this case we would get the following outcome:

  Object o;
  dynamic d;
  for (var x in o) {} // An error exists, because `o.iterator` is an error.
  for (int x in d) {} // OK; dynamic lookups `d.iterator`, `n0.current` and cast to `int`.
  for (int x in o) {} // Error exists, `o.iterator` again.
  for (var x in d) {} // OK, has `d.iterator`, `n0.current`, `x` has type `dynamic`.

The error messages would not be able to refer to o.iterator, but we don't specify exactly how to weasel around that. ;-)

PS: The title seems to imply that --no-implicit-casts is used, in which case the cast to int will of course be flagged as well.

[MichaelRFairhurst]

PS: The title seems to imply that --no-implicit-casts is used, in which case the cast to int will of course be flagged as well.

Yes, that's the issue here, is that turning that flag on doesn't do anything here.

I'm not so sure about

  for (var x in d) {} // OK, has `d.iterator`, `n0.current`, `x` has type `dynamic`.

I suppose I can see this being OK because it's simply pure dynamic code. But I don't like it! :)

I would expect the desugared code to do:

Iteratable i = d;

after all, this code aborts because NotIterable is not an Iterable.

void main() {                                                                    
  dynamic notIterable = NotIterable();                                           
  for (var i in notIterable) {                                                   
    print(i);                                                                    
  }                                                                              
}                                                                                
                                                                                 
class NotIterable {                                                              
  List<int> innerList = [1, 2, 3];                                               
  Iterator<int> get iterator => innerList.iterator;                              
}

[eernstg]

I would expect the desugared code to do: .. Iterable i = d;

We discussed that approach in the language team, but for-in is actually specified to desugar to a snippet of code where the type of the iterable is not required to be Iterable<T> for any T, it just has to support an invocation of iterator, and the returned object must support moveNext() and current. So if that iterator has type dynamic and those methods/getters exist then we're good to go. It's been a while, but I believe it wasn't done because of worries about potential breakage, or maybe it just wasn't pushed hard enough to get to a conclusion.

But I can see that both dart2js and dart generate code where it is actually required that the iterable is an Iterable, so there wouldn't be any breakage to worry about in practice.

Created a pull request in order to make the spec say that it must be an Iterable.

The discussion about --no-implicit-casts is separate, but with this change we would get an error for for (var x in d) {} because of the downcast from dynamic to Iterable<dynamic> at the beginning of the desugared code.

[srawlins]

I'm closing this as WAI. The spec is now pretty clear, and analyzer's new strict-casts: true reports these cases.