for-in codegen for dynamic case

[jmesserly]

First off, we should verify that the static checker is working. Dart for-in loops should do the right static checks, and issue errors, e.g. if my Iterator is missing moveNext or current. So all statically typed cases will be fine, and caught at compile time.

The two problematic cases that I see are the dynamic ones:

    case1() {
      dynamic bogus = 123;
      for (var x in bogus) { ... }
    }

    class BogusIterable {
      get iterator {
        dynamic bogus = 123;
        return bogus;
      }
    }
    case2() {
      for (var x in new BogusIterable()) { ... }
    }

For case 1, we should call bogus.noSuchMethod with #iterator as the invocation symbol. Instead it will fail like:

Uncaught TypeError: Cannot read property 'Symbol(Symbol.iterator)' of ...

For case 2 we have a similar problem. It will fail inside dart_runtime JsIterator, in either the i.moveNext() or i.current calls.

Both of these need different fixes, I think. For case1, we need a dynamic variant of for-in loop. Probably we can insert a call like:

    for (let x of dart.diterable(bogus)) { ... }

this will return an object that does checks:

function diterable(obj) {
  // TODO(jmesserly): we only use this immediately in for-of loops and discard, so we could
  // cache and reuse the same instance for perf.
  return { [Symbol.iterator]() { return new JsIteratorChecked(dart.dload(obj, 'iterable')); } }
}

For case2, we need only JsIteratorChecked ... in codegen, if we don't think this.iterable is typed correctly--it doesn't have valid moveNext and current methods--we use JsIteratorChecked instead of JsIterator. JsIteratorChecked is identical to JsIterator, but uses dynamic dispatch for moveNext and current. We could even optimize it a little bit, and have JsIteratorChecked decide up-front if moveNext and current are there and have the right return types, and if so, use the faster JsIterator.

Last but not least, case3: we need checking on x in the for loop body, I suspect. Might be there already. Checker/coercion reifier should be the ones handling these.

[jmesserly]

CC @vsmenon

[jmesserly]

Note, personally I don't think this is super high priority. Error cases will still fail, with an easy to understand TypeError, either iterator, moveNext or current is missing. What won't work is noSuchMethod ... which we don't implement yet anyway: #59, and checking the result of moveNext is actually a bool (instead we'll do a JS boolean conversion). It's not ideal, but on the stack rank of all the things broken right now, it wouldn't be really high on my list. Pretty sure language tests will catch all of these corners for us. (and many more we aren't thinking of)

[vsmenon]

Note, we currently don't have a dynamic case. E.g., given:

void main() {
  dynamic l = <int>[1, 2, 3];
  for (int i in l) {
    print(i);
  }
}

we generate:

  function main() {
    let l = dart.setType([1, 2, 3], core.List$(core.int));
    for (let i of dart.as(l, core.Iterable$(core.int))) {
      core.print(i);
    }
  }

I.e., the dynamic is cast to an Iterable - this is inserted by the checker / reifier. This is arguably too strong a requirement. It's certainly strongly than spec.

[jmesserly]

oh wow. yeah, multiple aspects of the system are implicated here :)

what happens if "i" is dynamic? would that become Iterable<dynamic>? take my case2 above, and make it case3:

    class BogusIterable2 implements Iterable {
      get iterator {
        dynamic bogus = 123;
        return bogus;
      }
    }
    case3() {
      for (var x in new BogusIterable2()) { ... }
    }

now we'll fail inside the iterator at runtime I think.

[munificent]

This patch along with my previous patch for better for-in checking should, I hope, ensure the right errors are reported and casts are added to the AST. Here's what analyzer should give you:

case1() {
  dynamic bogus = 123;
  for (var x in bogus) { ... }
}

This does a dynamic cast of bogus to Iterable<dynamic>. No cast for x since it's dynamic.

class BogusIterable {
  get iterator {
    dynamic bogus = 123;
    return bogus;
  }
}
case2() {
  for (var x in new BogusIterable()) { ... }
}

Static error since we do nominal checking of for-in now. BogusIterable has to declare it implements Iterable.

function main() {
  let l = dart.setType([1, 2, 3], core.List$(core.int));
  for (let i of dart.as(l, core.Iterable$(core.int))) {
    core.print(i);
  }
}

Now that we do nominal checking, I think this cast is correct.

class BogusIterable2 implements Iterable {
  get iterator {
    dynamic bogus = 123;
    return bogus;
  }
}
case3() {
  for (var x in new BogusIterable2()) { ... }
}

Oh, wow. Now that's nasty. It declares it implements Iterable but makes iterator return dynamic in order to cheat and not really implement it fully.

My CLs don't handle this case. Handling it correctly would require us to generate a series of casts to fully match the structural desugaring of for-in. We'd need to cast the sequence to Iterable. Cast the result of iterator to Iterator. Cast the result of moveNext() to bool. Ugh.

What do you think we should do?

[jmesserly]

Oh, wow. Now that's nasty. It declares it implements Iterable but makes iterator return dynamic in order to cheat and not really implement it fully.

Hmm, nowadays shouldn't we push the type down via member override inference? Otherwise this would be an error:

class BogusIterable2 implements Iterable {
  // Would be ERROR: () -> dynamic not a subtype of () -> Iterator<dynamic>
  // But instead we should infer the type and then ...
  get iterator {
    dynamic bogus = 123;
    return bogus; // Strong Mode should insert a cast here.
  }
}

[leafpetersen]

Yeah, I think we're inferring the return type of iterator for you. So there is (or should be) an implicit cast being inserted around the return of bogus.

[munificent]

Ah, well, that's handy. Though I suppose you could go out of your way to confound that by using an explicit dynamic annotation on iterator?

[leafpetersen]

That would be an invalid override.

[jmesserly]

I think this bug is obsolete with Bob's Analyzer changes, but we can reopen if something like this comes up again.