Defer all recursive mixins

pkg/dev_compiler/lib/src/compiler/code_generator.dart

Index: pkg/dev_compiler/lib/src/compiler/code_generator.dart
diff --git a/pkg/dev_compiler/lib/src/compiler/code_generator.dart b/pkg/dev_compiler/lib/src/compiler/code_generator.dart
index deb528af8607798c265d03bb753f4b880d096c10..0832ffa1a02809793c744ae3069c4a8ea89b317b 100644
--- a/pkg/dev_compiler/lib/src/compiler/code_generator.dart
+++ b/pkg/dev_compiler/lib/src/compiler/code_generator.dart
@@ -1248,24 +1248,21 @@ class CodeGenerator extends GeneralizingAstVisitor
 
     _loader.startTopLevel(element);
 
-    // Find the super type
-    JS.Expression heritage;
-    var supertype = type.superclass;
-    if (_deferIfNeeded(supertype, element)) {
-      // Fall back to raw type.
-      supertype = fillDynamicTypeArgs(supertype.element.type);
+    // List of "direct" supertypes (supertype + mixins)
+    var basetypes = [ type.superclass ]..addAll(type.mixins);

[Jennifer Messerly, 2017/01/11 18:11:47]

nit: run dart format

[vsm, 2017/01/11 18:33:00]

Done.

+
+    // If any of these are recursive (via type parameter), defer setting
+    // the real superclass.
+    if (basetypes.any((t) => _deferIfNeeded(t, element))) {

[Jennifer Messerly, 2017/01/11 18:11:47]

alternatively, could do this:

var baseTypes = <DartType>[];
for (var t in [type.superclass]..addAll(type.mixins)) {
  if (_deferIfNeeded(t, element)) {
    baseTypes.add(fillDynamicTypeArgs(t.element.type));
    _hasDeferredSupertype.add(element);
  } else {
    baseTypes.add(t);
  }
}


... slightly more precise on which types are being deferred. Not sure it's worth
it tho.

[vsm, 2017/01/11 18:33:00]

We also don't need to fill in dynamic for all type params on a single type -
just where it's recursive.  Thought about doing this, but didn't seem worth it.

+      // Fall back to raw type
+      basetypes = basetypes.map((t) => fillDynamicTypeArgs(t.element.type)).toList();
       _hasDeferredSupertype.add(element);
     }
-    // We could choose to name the superclasses, but it's
-    // not clear that there's much benefit
-    heritage = _emitType(supertype, nameType: false);
 
-    if (type.mixins.isNotEmpty) {
-      var mixins =
-          type.mixins.map((t) => _emitType(t, nameType: false)).toList();
-      mixins.insert(0, heritage);
-      heritage = _callHelper('mixin(#)', [mixins]);
-    }
+    // List of "direct" JS superclasses
+    var baseclasses = basetypes.map((t) => _emitType(t, nameType: false)).toList();
+    assert(baseclasses.isNotEmpty);
+    var heritage = (baseclasses.length == 1) ? baseclasses.first :  _callHelper('mixin(#)', [baseclasses]);
 
     _loader.finishTopLevel(element);
 
@@ -1684,6 +1681,12 @@ class CodeGenerator extends GeneralizingAstVisitor
     } else if (_hasDeferredSupertype.contains(classElem)) {

[Jennifer Messerly, 2017/01/11 18:11:47]

incidentally ... this isn't caused by your change, but I'm never sure why we
used _hasDeferredSupertype like a global Set. It should be quite possible to
return the value from _emitClassHeritage and pass it back down to this method.

We're literally using a Set on the class rather than pass a return value and
parameter. It's strange and I've been meaning to refactor it at some point.

[vsm, 2017/01/11 18:33:00]

Added a comment.  _emitClassHeritage/_emitClassExpression are called a handful
of places - wasn't obvious if we'd need to thread all of those.

       var newBaseClass = _emitType(classElem.type.superclass,
           nameType: false, subClass: classElem, className: className);
+      if (classElem.type.mixins.isNotEmpty) {
+        var mixins =
+            classElem.type.mixins.map((t) => _emitType(t, nameType: false)).toList();
+        mixins.insert(0, newBaseClass);
+        newBaseClass = _callHelper('mixin(#)', [mixins]);
+      }
       var deferredBaseClass = _callHelperStatement(
           'setBaseClass(#, #);', [className, newBaseClass]);
       if (typeFormals.isNotEmpty) return deferredBaseClass;