fixes #130, accept anything with `iterator` in for loops

lib/src/codegen/js_codegen.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.
 
 library dev_compiler.src.codegen.js_codegen;
 
 import 'dart:collection' show HashSet, HashMap;
 
 import 'package:analyzer/analyzer.dart' hide ConstantEvaluator;
 import 'package:analyzer/src/generated/ast.dart' hide ConstantEvaluator;
@@ skipping 555 matching lines @@
     var isObject = type.isObject;
     var name = node.name.name;
 
     // Iff no constructor is specified for a class C, it implicitly has a
     // default constructor `C() : super() {}`, unless C is class Object.
     var jsMethods = <JS.Method>[];
     if (ctors.isEmpty && !isObject) {
       jsMethods.add(_emitImplicitConstructor(node, name, fields));
     }
 
-    for (var member in node.members) {
-      if (member is ConstructorDeclaration) {
-        jsMethods.add(_emitConstructor(member, name, fields, isObject));
-      } else if (member is MethodDeclaration) {
-        jsMethods.add(_emitMethodDeclaration(type, member));
+    bool hasJsPeer = getAnnotationValue(node, _isJsPeerInterface) != null;
+
+    bool hasIterator = false;
+    for (var m in node.members) {
+      if (m is ConstructorDeclaration) {
+        jsMethods.add(_emitConstructor(m, name, fields, isObject));
+      } else if (m is MethodDeclaration) {
+        jsMethods.add(_emitMethodDeclaration(type, m));
+
+        if (!hasJsPeer && m.isGetter && m.name.name == 'iterator') {
+          hasIterator = true;
+          jsMethods.add(_emitIterable(type));

[vsm, 2015/05/05 14:19:28]

Should we check the return type of iterator here?  We could either:

(a) Only generate a JS iterator if it is Iterable or a subtype.

or

(b) Only generate a JS iterator if the return type has a moveNext and current
with the right signatures.

On Dart for-in loops, we currently require (a), but the Dart spec allows (b).

[Jennifer Messerly, 2015/05/05 15:38:25]

First off, just confirming: most of this is the job of static checker, right?
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.

there's a bunch of issues with the dynamic case, all of which existed before
this CL, and span various parts of our checker/reifier/codegen/runtime, so I'd
vote to fix those at some later point in time :). Some of these seem rather
obscure, too.

Anyway, lots more to say on this in
https://github.com/dart-lang/dev_compiler/issues/166

+        }
       }
     }
 
-    // Support for adapting dart:core Iterator/Iterable to ES6 versions.
-    // This lets them use for-of loops transparently.
-    // https://github.com/lukehoban/es6features#iterators--forof
-    if (element.library.isDartCore && element.name == 'Iterable') {
-      JS.Fun body = js.call('''function() {
-        var iterator = this.iterator;
-        return {
-          next() {
-            var done = iterator.moveNext();
-            return { done: done, current: done ? void 0 : iterator.current };
-          }
-        };
-      }''');
-      jsMethods.add(new JS.Method(js.call('$_SYMBOL.iterator'), body));
+    // If the type doesn't have an `iterator`, but claims to implement Iterable,
+    // we inject the adaptor method here, as it's less code size to put the
+    // helper on a parent class. This pattern is common in the core libraries
+    // (e.g. IterableMixin<E> and IterableBase<E>).
+    //
+    // (We could do this same optimization for any interface with an `iterator`
+    // method, but that's more expensive to check for, so it doesn't seem worth
+    // it. The above case for an explicit `iterator` method will catch those.)
+    if (!hasJsPeer && !hasIterator && _implementsIterable(type)) {
+      jsMethods.add(_emitIterable(type));
     }
+
     return jsMethods.where((m) => m != null).toList(growable: false);
   }
 
+  bool _implementsIterable(InterfaceType t) =>
+      t.interfaces.any((i) => i.element.type == types.iterableType);
+
+  /// Support for adapting dart:core Iterable to ES6 versions.
+  ///
+  /// This lets them use for-of loops transparently:
+  /// <https://github.com/lukehoban/es6features#iterators--forof>
+  ///
+  /// This will return `null` if the adapter was already added on a super type,
+  /// otherwise it returns the adapter code.
+  // TODO(jmesserly): should we adapt `Iterator` too?
+  JS.Method _emitIterable(InterfaceType t) {
+    // If a parent had an `iterator` (concrete or abstract) or implements
+    // Iterable, we know the adapter is already there, so we can skip it as a
+    // simple code size optimization.
+    var parent = t.lookUpGetterInSuperclass('iterator', t.element.library);
+    if (parent != null) return null;
+    parent = findSupertype(t, _implementsIterable);
+    if (parent != null) return null;
+
+    // Otherwise, emit the adapter method, which wraps the Dart iterator in
+    // an ES6 iterator.
+    return new JS.Method(js.call('$_SYMBOL.iterator'), js.call(
+        'function() { return new dart.JsIterator(this.#); }',
+        [_emitMemberName('iterator', type: t)]));
+  }
+
   /// Emit class members that need to come after the class declaration, such
   /// as static fields. See [_emitClassMethods] for things that are emitted
   /// inside the ES6 `class { ... }` node.
   JS.Statement _finishClassMembers(ClassElement classElem,
       JS.ClassExpression cls, List<ConstructorDeclaration> ctors,
       List<FieldDeclaration> fields, List<FieldDeclaration> staticFields,
       String jsPeerName) {
     var name = classElem.name;
     var body = <JS.Statement>[];
     body.add(new JS.ClassDeclaration(cls));
@@ skipping 1926 matching lines @@
 // TODO(jmesserly): validate the library. See issue #135.
 bool _isJsNameAnnotation(DartObjectImpl value) => value.type.name == 'JsName';
 
 bool _isJsPeerInterface(DartObjectImpl value) =>
     value.type.name == 'JsPeerInterface';
 
 // TODO(jacobr): we would like to do something like the following
 // but we don't have summary support yet.
 // bool _supportJsExtensionMethod(AnnotatedNode node) =>
 //    _getAnnotation(node, "SupportJsExtensionMethod") != null;