Defer all recursive mixins

pkg/dev_compiler/lib/src/compiler/code_generator.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.
 
 import 'dart:collection' show HashMap, HashSet;
 import 'dart:math' show min, max;
 
 import 'package:analyzer/analyzer.dart' hide ConstantEvaluator;
 import 'package:analyzer/dart/ast/ast.dart';
@@ skipping 1230 matching lines @@
     return new JS.ClassExpression(new JS.Identifier(name), heritage, methods,
         typeParams: typeParams, fields: jsFields);
   }
 
   JS.Expression _emitClassHeritage(ClassElement element) {
     var type = element.type;
     if (type.isObject) return null;
 
     _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);
 
     return heritage;
   }
 
   /// Provide Dart getters and setters that forward to the underlying native
   /// field.  Note that the Dart names are always symbolized to avoid
   /// conflicts.  They will be installed as extension methods on the underlying
   /// native type.
@@ skipping 395 matching lines @@
   JS.Statement _setBaseClass(ClassElement classElem, JS.Expression className,
       List<String> jsPeerNames, List<JS.Statement> body) {
     var typeFormals = classElem.typeParameters;
     if (jsPeerNames.isNotEmpty && typeFormals.isNotEmpty) {
       for (var peer in jsPeerNames) {
         // TODO(jmesserly): we should just extend Array in the first place
         var newBaseClass = _callHelper('global.#', [peer]);
         body.add(_callHelperStatement(
             'setExtensionBaseClass(#, #);', [className, newBaseClass]));
       }
     } 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;
       body.add(deferredBaseClass);
     }
     return null;
   }
 
   void _defineNamedConstructors(List<ConstructorDeclaration> ctors,
       List<JS.Statement> body, JS.Expression className, bool isCallable) {
@@ skipping 4116 matching lines @@
   if (targetIdentifier.staticElement is! PrefixElement) return false;
   var prefix = targetIdentifier.staticElement as PrefixElement;
 
   // The library the prefix is referring to must come from a deferred import.
   var containingLibrary = resolutionMap
       .elementDeclaredByCompilationUnit(target.root as CompilationUnit)
       .library;
   var imports = containingLibrary.getImportsWithPrefix(prefix);
   return imports.length == 1 && imports[0].isDeferred;
 }