Name and hoist types

lib/src/compiler/code_generator.dart

Index: lib/src/compiler/code_generator.dart
diff --git a/lib/src/compiler/code_generator.dart b/lib/src/compiler/code_generator.dart
index ba65adf354cb69cffa88c9b4bcc8fb43bc2759e8..bbe4e74655aeb14bb5730a87b8760c4abd8a602d 100644
--- a/lib/src/compiler/code_generator.dart
+++ b/lib/src/compiler/code_generator.dart
@@ -66,6 +66,9 @@ class CodeGenerator extends GeneralizingAstVisitor
   /// The list of output module items, in the order they need to be emitted in.
   final _moduleItems = <JS.ModuleItem>[];
 
+  /// Table of named and possibly hoisted types.
+  final _typeTable = new _TypeTable();
+
   /// The global extension type table.
   final ExtensionTypeSet _extensionTypes;
 
@@ -252,6 +255,10 @@ class CodeGenerator extends GeneralizingAstVisitor
     // Declare imports
     _finishImports(items);
 
+    // Discharge the type table cache variables and
+    // hoisted definitions.
+    items.addAll(_typeTable.discharge());
+
     // Add the module's code (produced by visiting compilation units, above)
     _copyAndFlattenBlocks(items, _moduleItems);
 
@@ -568,7 +575,7 @@ class CodeGenerator extends GeneralizingAstVisitor
     JS.Expression body = annotate(
         js.call('dart.typedef(#, () => #)', [
           js.string(element.name, "'"),
-          _emitType(element.type, lowerTypedef: true)
+          _emitType(element.type, nameType: false, lowerTypedef: true)
         ]),
         node,
         element);
@@ -840,8 +847,12 @@ class CodeGenerator extends GeneralizingAstVisitor
   JS.Statement _defineClassTypeArguments(TypeDefiningElement element,
       List<TypeParameterElement> formals, JS.Statement body) {
     assert(formals.isNotEmpty);
-    var genericCall = js.call('dart.generic((#) => { #; return #; })',
-        [_emitTypeFormals(formals), body, element.name]);
+    var genericCall = js.call('dart.generic((#) => { #; #; return #; })', [
+      _emitTypeFormals(formals),
+      _typeTable.discharge(formals),
+      body,
+      element.name
+    ]);
     if (element.library.isDartAsync &&
         (element.name == "Future" || element.name == "_Future")) {
       genericCall = js.call('dart.flattenFutures(#)', [genericCall]);
@@ -894,10 +905,13 @@ class CodeGenerator extends GeneralizingAstVisitor
       supertype = fillDynamicTypeArgs(supertype.element.type);
       _hasDeferredSupertype.add(element);
     }
-    heritage = _emitType(supertype);
+    // 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(_emitType).toList();
+      var mixins =
+          type.mixins.map((t) => _emitType(t, nameType: false)).toList();
       mixins.insert(0, heritage);
       heritage = js.call('dart.mixin(#)', [mixins]);
     }
@@ -1159,7 +1173,7 @@ class CodeGenerator extends GeneralizingAstVisitor
           'dart.setExtensionBaseClass(#, #);', [className, newBaseClass]));
     } else if (_hasDeferredSupertype.contains(classElem)) {
       var newBaseClass = _emitType(classElem.type.superclass,
-          subClass: classElem, className: className);
+          nameType: false, subClass: classElem, className: className);
       body.add(
           js.statement('dart.setBaseClass(#, #);', [className, newBaseClass]));
     }
@@ -1259,9 +1273,12 @@ class CodeGenerator extends GeneralizingAstVisitor
         }
         var memberName = _elementMemberName(element,
             useExtension: _extensionTypes.isNativeClass(classElem));
-        var parts = _emitFunctionTypeParts(element.type);
-        var property =
-            new JS.Property(memberName, new JS.ArrayInitializer(parts));
+        // TODO(leafp):  Settle on a policy for this.  Naming these makes the
+        // signature more compact, but it generates a lot of noise at the
+        // top level, and it's not clear that there's much benefit.
+        var type =
+            _emitFunctionType(element.type, nameType: false, definite: true);
+        var property = new JS.Property(memberName, type);
         if (node.isStatic) {
           tStatics.add(property);
           sNames.add(memberName);
@@ -1275,10 +1292,11 @@ class CodeGenerator extends GeneralizingAstVisitor
     for (ConstructorDeclaration node in ctors) {
       var memberName = _constructorName(node.element);
       var element = node.element;
-      var parts = _emitFunctionTypeParts(element.type,
-          parameters: node.parameters.parameters);
-      var property =
-          new JS.Property(memberName, new JS.ArrayInitializer(parts));
+      var type = _emitFunctionType(element.type,
+          parameters: node.parameters.parameters,
+          nameType: false,
+          definite: true);
+      var property = new JS.Property(memberName, type);
       tCtors.add(property);
     }
 
@@ -1918,25 +1936,12 @@ class CodeGenerator extends GeneralizingAstVisitor
 
   JS.Expression _emitFunctionTagged(JS.Expression fn, DartType type,
       {topLevel: false}) {
-    var name = type.name;
     var lazy = topLevel && !_typeIsLoaded(type);
-
-    if (type is FunctionType && (name == '' || name == null)) {
-      if (type.typeFormals.isEmpty &&
-          type.returnType.isDynamic &&
-          type.optionalParameterTypes.isEmpty &&
-          type.namedParameterTypes.isEmpty &&
-          type.normalParameterTypes.every((t) => t.isDynamic)) {
-        return js.call('dart.fn(#)', [fn]);
-      }
-
-      var parts = _emitFunctionTypeParts(type);
-      if (lazy) {
-        return js.call(
-            'dart.lazyFn(#, () => #)', [fn, new JS.ArrayInitializer(parts)]);
-      } else {
-        return js.call('dart.fn(#, #)', [fn, parts]);
-      }
+    var typeRep = _emitFunctionType(type, definite: true);
+    if (lazy) {
+      return js.call('dart.lazyFn(#, () => #)', [fn, typeRep]);
+    } else {
+      return js.call('dart.fn(#, #)', [fn, typeRep]);
     }
     throw 'Function has non function type: $type';
   }
@@ -2012,21 +2017,21 @@ class CodeGenerator extends GeneralizingAstVisitor
       FormalParameterList parameters, FunctionBody body) {
     FunctionType type = element.type;
 
-    // sync*, async, async*
-    if (element.isAsynchronous || element.isGenerator) {
-      return new JS.Fun(
-          visitFormalParameterList(parameters, destructure: false),
-          new JS.Block([
-            _emitGeneratorFunctionBody(element, parameters, body).toReturn()
-          ]),
-          typeParams: _emitTypeFormals(type.typeFormals),
-          returnType: emitTypeRef(type.returnType));
+    // normal function (sync), vs (sync*, async, async*)
+    var stdFn = !(element.isAsynchronous || element.isGenerator);
+    var formals = visitFormalParameterList(parameters, destructure: stdFn);
+    var code = (stdFn)
+        ? _visit(body)
+        : new JS.Block(
+            [_emitGeneratorFunctionBody(element, parameters, body).toReturn()]);
+    var typeFormals = _emitTypeFormals(type.typeFormals);
+    var returnType = emitTypeRef(type.returnType);
+    if (type.typeFormals.isNotEmpty) {
+      code = new JS.Block(
+          [new JS.Block(_typeTable.discharge(type.typeFormals)), code]);
     }
-
-    // normal function (sync)
-    return new JS.Fun(visitFormalParameterList(parameters), _visit(body),
-        typeParams: _emitTypeFormals(type.typeFormals),
-        returnType: emitTypeRef(type.returnType));
+    return new JS.Fun(formals, code,
+        typeParams: typeFormals, returnType: returnType);
   }
 
   JS.Expression _emitGeneratorFunctionBody(ExecutableElement element,
@@ -2249,13 +2254,15 @@ class CodeGenerator extends GeneralizingAstVisitor
           ? p.metadata
           : (p as DefaultFormalParameter).parameter.metadata;
 
-  JS.ArrayInitializer _emitTypeNames(List<DartType> types,
-      [List<FormalParameter> parameters]) {
+  JS.ArrayInitializer _emitTypeNames(
+      List<DartType> types, List<FormalParameter> parameters,
+      {bool nameType: true, bool hoistType: true}) {
     var result = <JS.Expression>[];
     for (int i = 0; i < types.length; ++i) {
       var metadata =
           parameters != null ? _parameterMetadata(parameters[i]) : [];
-      var typeName = _emitType(types[i]);
+      var typeName =
+          _emitType(types[i], nameType: nameType, hoistType: hoistType);
       var value = typeName;
       if (options.emitMetadata && metadata.isNotEmpty) {
         metadata = metadata.map(_instantiateAnnotation).toList();
@@ -2278,13 +2285,37 @@ class CodeGenerator extends GeneralizingAstVisitor
 
   /// Emit the pieces of a function type, as an array of return type,
   /// regular args, and optional/named args.
+  JS.Expression _emitFunctionType(FunctionType type,
+      {List<FormalParameter> parameters,
+      bool lowerTypedef: false,
+      bool nameType: true,
+      bool hoistType: true,

[Jennifer Messerly, 2016/05/24 21:56:39]

do we ever set this to false? I wasn't able to find it, seems like we always let
the default value kick in or just pass it along.

[Leaf, 2016/05/25 00:56:13]

It was here so that we can play with different strategies.  Added flags so its
controllable from the command line, plus test.

+      definite: false}) {
+    var parts = _emitFunctionTypeParts(type,
+        parameters: parameters,
+        lowerTypedef: lowerTypedef,
+        nameType: nameType,
+        hoistType: hoistType);
+    var helper = (definite) ? 'definiteFunctionType' : 'functionType';
+    var fullType = js.call('dart.${helper}(#)', [parts]);
+    if (!nameType) return fullType;
+    return _typeTable.nameType(type, fullType, hoistType, definite: definite);
+  }
+
+  /// Emit the pieces of a function type, as an array of return type,
+  /// regular args, and optional/named args.
   List<JS.Expression> _emitFunctionTypeParts(FunctionType type,
-      {List<FormalParameter> parameters, bool lowerTypedef: false}) {
+      {List<FormalParameter> parameters,
+      bool lowerTypedef: false,
+      bool nameType: true,
+      bool hoistType: true}) {
     var parameterTypes = type.normalParameterTypes;
     var optionalTypes = type.optionalParameterTypes;
     var namedTypes = type.namedParameterTypes;
-    var rt = _emitType(type.returnType);
-    var ra = _emitTypeNames(parameterTypes, parameters);
+    var rt =
+        _emitType(type.returnType, nameType: nameType, hoistType: hoistType);
+    var ra = _emitTypeNames(parameterTypes, parameters,
+        nameType: nameType, hoistType: hoistType);
 
     List<JS.Expression> typeParts;
     if (namedTypes.isNotEmpty) {
@@ -2295,7 +2326,8 @@ class CodeGenerator extends GeneralizingAstVisitor
     } else if (optionalTypes.isNotEmpty) {
       assert(namedTypes.isEmpty);
       var oa = _emitTypeNames(
-          optionalTypes, parameters?.sublist(parameterTypes.length));
+          optionalTypes, parameters?.sublist(parameterTypes.length),
+          nameType: nameType, hoistType: hoistType);
       typeParts = [rt, ra, oa];
     } else {
       typeParts = [rt, ra];
@@ -2307,7 +2339,17 @@ class CodeGenerator extends GeneralizingAstVisitor
       // function types (as callable functions). See discussion at:
       // https://github.com/dart-lang/dev_compiler/issues/526
       var tf = _emitTypeFormals(typeFormals);
-      typeParts = [new JS.ArrowFun(tf, new JS.ArrayInitializer(typeParts))];
+      var names = _typeTable.discharge(typeFormals);
+      var parts = new JS.ArrayInitializer(typeParts);
+      if (names.isEmpty) {
+        typeParts = [
+          js.call('(#) => #', [tf, parts])
+        ];
+      } else {
+        typeParts = [
+          js.call('(#) => {#; return #;}', [tf, names, parts])
+        ];
+      }
     }
     return typeParts;
   }
@@ -2325,6 +2367,8 @@ class CodeGenerator extends GeneralizingAstVisitor
   JS.Expression _emitType(DartType type,
       {bool lowerTypedef: false,
       bool lowerGeneric: false,
+      bool nameType: true,
+      bool hoistType: true,
       ClassElement subClass,
       JS.Expression className}) {
     // The void and dynamic types are not defined in core.
@@ -2347,9 +2391,8 @@ class CodeGenerator extends GeneralizingAstVisitor
       // TODO(jmesserly): should we change how typedefs work? They currently
       // go through use similar logic as generic classes. This makes them
       // different from universal function types.
-      var ft = type as FunctionType;
-      var parts = _emitFunctionTypeParts(ft, lowerTypedef: lowerTypedef);
-      return js.call('dart.functionType(#)', [parts]);
+      return _emitFunctionType(type as FunctionType,
+          lowerTypedef: lowerTypedef, nameType: nameType, hoistType: hoistType);
     }
 
     if (type is TypeParameterType) {
@@ -2365,14 +2408,20 @@ class CodeGenerator extends GeneralizingAstVisitor
       var args = type.typeArguments;
       Iterable jsArgs = null;
       if (args.any((a) => !a.isDynamic)) {
-        jsArgs = args
-            .map((x) => _emitType(x, subClass: subClass, className: className));
+        jsArgs = args.map((x) => _emitType(x,
+            nameType: nameType,
+            hoistType: hoistType,
+            subClass: subClass,
+            className: className));
       } else if (lowerGeneric) {
         jsArgs = [];
       }
       if (jsArgs != null) {
         var genericName = _emitTopLevelName(element, suffix: '\$');
-        return js.call('#(#)', [genericName, jsArgs]);
+        var typeRep = js.call('#(#)', [genericName, jsArgs]);
+        return nameType
+            ? _typeTable.nameType(type, typeRep, hoistType)
+            : typeRep;
       }
     }
 
@@ -4565,3 +4614,210 @@ LibraryElement _getLibrary(AnalysisContext c, String uri) =>
 
 bool _isDartRuntime(LibraryElement l) =>
     l.isInSdk && l.source.uri.toString() == 'dart:_runtime';
+
+Set<TypeParameterElement> _freeTypeParameters(DartType t) {
+  var result = new HashSet<TypeParameterElement>();
+  void find(DartType t) {
+    if (t is TypeParameterType) {
+      result.add(t.element);
+    } else if (t is FunctionType) {
+      find(t.returnType);
+      t.parameters.forEach((p) => find(p.type));
+      t.typeFormals.forEach((p) => find(p.bound));
+      t.typeFormals.forEach(result.remove);
+    } else if (t is InterfaceType) {
+      t.typeArguments.forEach(find);
+    }
+  }
+  find(t);
+  return result;
+}
+
+/// _CacheTable tracks cache variables for variables that
+/// are emitted in place with a hoisted variable for a cache.

[Jennifer Messerly, 2016/05/24 21:56:39]

Consider: including an example here of what it looks like in the code. Might
make it easier to visualize.

[Leaf, 2016/05/25 00:56:13]

Done below.

+class _CacheTable {

[Jennifer Messerly, 2016/05/24 21:56:39]

consider: putting these new classes into their own file(s)? They look pretty
self contained (which is good!)

[Leaf, 2016/05/25 00:56:13]

Done.

+  /// Mapping from types to their canonical names.
+  final _names = new Map<DartType, JS.TemporaryId>();
+  Iterable<DartType> get keys => _names.keys.toList();
+
+  JS.Statement _dischargeType(DartType type) {
+    var name = _names.remove(type);
+    if (name != null) {
+      return (js.statement('let #;', [name]));

[Jennifer Messerly, 2016/05/24 21:56:39]

nit: parens not needed around return value

[Leaf, 2016/05/25 00:56:13]

Done.

[Leaf, 2016/05/25 00:56:13]

Done.

+    }
+    return null;
+  }
+
+  /// Emit a list of statements declaring the cache variables for

[Jennifer Messerly, 2016/05/24 21:56:39]

doc comment style nit -- first sentence should be its own paragraph:
https://www.dartlang.org/effective-dart/documentation/#do-make-the-first-sent...
(applies to all of them)

That said content is more important and these are private APIs so I wouldn't
worry about it, but as a general FYI.

[Leaf, 2016/05/25 00:56:13]

Acknowledged.

+  /// types tracked by this table.  If [typeFilter] is given,
+  /// only emit the types listed in the filter.
+  List<JS.Statement> discharge([Iterable<DartType> typeFilter]) {
+    var decls = <JS.Statement>[];
+    var types = typeFilter ?? keys;
+    for (var t in types) {
+      var stmt = _dischargeType(t);
+      if (stmt != null) decls.add(stmt);
+    }
+    return decls;
+  }
+
+  bool isNamed(DartType type) => _names.containsKey(type);
+
+  /// If [type] is not already in the table, choose a new canonical
+  /// variable to contain it. Emit an expression which uses [typeRep] to
+  /// lazily initialize the cache in place.
+  JS.Expression nameType(DartType type, JS.Expression typeRep) {
+    var temp = _names[type];

[Jennifer Messerly, 2016/05/24 21:56:39]

you could also do

    var temp = _names.putIfAbsent(type, () => chooseTypeName(type));

[Leaf, 2016/05/25 00:56:13]

Acknowledged.

+    if (temp == null) {
+      ;

[Jennifer Messerly, 2016/05/24 21:56:39]

unintended empty statement?

[Leaf, 2016/05/25 00:56:13]

Done.

+      _names[type] = temp = chooseTypeName(type);
+    }
+    return js.call('# || (# = #)', [temp, temp, typeRep]);
+  }
+
+  String _typeString(DartType type, {bool flat: false}) {
+    if (type is ParameterizedType && type.name != null) {
+      var clazz = type.name;
+      var params = type.typeArguments;
+      if (params == null) return clazz;
+      if (params.every((p) => p.isDynamic)) return clazz;
+      var paramStrings = params.map(_typeString);
+      var paramString = paramStrings.join("\$");
+      return "${clazz}Of${paramString}";
+    }
+    if (type is FunctionType) {
+      if (flat) return "Fn";
+      var rType = _typeString(type.returnType, flat: true);
+      var paramStrings = type.normalParameterTypes
+          .take(3)
+          .map((p) => _typeString(p, flat: true));
+      var paramString = paramStrings.join("And");
+      var count = type.normalParameterTypes.length;
+      if (count > 3 ||
+          type.namedParameterTypes.isNotEmpty ||
+          type.optionalParameterTypes.isNotEmpty) {
+        paramString = "${paramString}__";
+      } else if (count == 0) {
+        paramString = "Void";
+      }
+      return "${paramString}To${rType}";
+    }
+    if (type is TypeParameterType) return type.name;
+    if (type.name != null) return type.name;
+    return "type";
+  }
+
+  /// Heuristically choose a good name for the cache and generator
+  /// variables.
+  JS.Identifier chooseTypeName(DartType type) {
+    return new JS.TemporaryId(_typeString(type));
+  }
+}
+
+/// _GeneratorTable tracks types which have been
+/// named and hoisted.
+class _GeneratorTable extends _CacheTable {
+  final _defs = new Map<DartType, JS.Expression>();
+
+  _GeneratorTable();

[Jennifer Messerly, 2016/05/24 21:56:39]

note: this is not needed

[Leaf, 2016/05/25 00:56:13]

Done.

+
+  JS.Statement _dischargeType(DartType t) {
+    var name = _names.remove(t);
+    if (name != null) {
+      JS.Expression init = _defs.remove(t);
+      assert(init != null);
+      return js.statement(
+          'let # = () => ((# = dart.constFn(#))());', [name, name, init]);
+    }
+    return null;
+  }
+
+  /// If [type] does not already have a generator name chosen for it,
+  /// assign it one, using [typeRep] as the initializer for it.
+  /// Emit an expression which calls the generator name.
+  JS.Expression nameType(DartType type, JS.Expression typeRep) {

[Jennifer Messerly, 2016/05/24 21:56:39]

this looks mostly identical to the super method, might be worth sharing

[Leaf, 2016/05/25 00:56:13]

It's 3 of 5 lines that are duplicated.  I don't see an easy way to share that
doesn't just add ceremony.

+    var temp = _names[type];
+    if (temp == null) {
+      _names[type] = temp = chooseTypeName(type);
+      _defs[type] = typeRep;
+    }
+    return js.call('#()', [temp]);
+  }
+}
+
+class _TypeTable {
+  /// Cache variable names for types emitted in place.
+  final _cacheNames = new _CacheTable();
+
+  /// Cache variable names for definite function types emitted in place.
+  final _definiteCacheNames = new _CacheTable();
+
+  /// Generator variable names for hoisted types.
+  final _generators = new _GeneratorTable();
+
+  /// Generator variable names for hoisted definite function types.
+  final _definiteGenerators = new _GeneratorTable();
+
+  /// Mapping from type parameters to the types which must have their
+  /// cache/generator variables discharged at the binding site for the
+  /// type variable since the type definition depends on the type
+  /// parameter.
+  final _scopeDependencies = new Map<TypeParameterElement, List<DartType>>();
+
+  /// Emit a list of statements declaring the cache variables and generator
+  /// definitions tracked by the table.  If [formals] is present, only
+  /// emit the definitions which depend on the formals.
+  List<JS.Statement> discharge([List<TypeParameterElement> formals]) {
+    var filter = formals?.expand((p) => _scopeDependencies[p] ?? []);
+    var stmts = [
+      _cacheNames,
+      _definiteCacheNames,
+      _generators,
+      _definiteGenerators
+    ].expand((c) => c.discharge(filter)).toList();
+    formals?.forEach(_scopeDependencies.remove);
+    return stmts;
+  }
+
+  /// Record the dependencies of the type on its free variables
+  bool recordScopeDependencies(DartType type) {
+    var fvs = _freeTypeParameters(type);
+    // TODO(leafp): This is a hack to avoid trying to hoist out of
+    // generic functions and generic function types.  This often degrades
+    // readability to little or no benefit.  It would be good to do this
+    // when we know that we can hoist it to an outer scope, but for
+    // now we just disable it.
+    if (fvs.any((i) => i.enclosingElement is FunctionTypedElement)) {
+      return true;
+    }
+    void addScope(TypeParameterElement i) {
+      List<DartType> types = _scopeDependencies[i];
+      if (types == null) _scopeDependencies[i] = types = [];
+      types.add(type);
+    }
+    fvs.forEach(addScope);
+    return false;
+  }
+
+  /// Given a type [type], and a JS expression [typeRep] which implements it,
+  /// Add the type and its representation to the table, returning an

[Jennifer Messerly, 2016/05/24 21:56:39]

nit: lowercase "add"

[Leaf, 2016/05/25 00:56:13]

Done.

+  /// expression which implements the type (but which caches the value).
+  /// If [hoist] is true, then the JS representation will be hoisted up

[Jennifer Messerly, 2016/05/24 21:56:39]

consider: splitting this into a few paragraphs?

[Leaf, 2016/05/25 00:56:13]

Done.

+  /// as far as possible and shared between instances of the type.
+  /// If [hoist] is false, the cache variable will be hoisted up as
+  /// far as possible and shared between instances of the type, but the
+  /// initializer expression will be emitted in place.
+  /// The boolean parameter [definite] distinguishes between definite function
+  /// types and other types (since the same DartType may have different
+  /// representations as definite and indefinite function types).

[Jennifer Messerly, 2016/05/24 21:56:39]

I seem to recall at one point we had considered putting the arrow type (fuzzy or
definite) on FunctionTypeImpl in Analyzer. Makes me wonder about that again.
Maybe that's something we should look into at some point?

[Leaf, 2016/05/25 00:56:13]

Acknowledged.

+  JS.Expression nameType(DartType type, JS.Expression typeRep, bool hoist,

[Jennifer Messerly, 2016/05/24 21:56:39]

any reason to make "hoist" required and "definite" a named arg?

usual rule is to avoid required booleans:
https://www.dartlang.org/effective-dart/design/#avoid-positional-boolean-para...

[Leaf, 2016/05/25 00:56:13]

Made them both named, asserted hoist is set.  Definite has an obvious default
(almost no types are definite), but hoist is something that is specific to every
call site and shouldn't be omitted.

+      {bool definite: false}) {
+    var table = hoist
+        ? (definite ? _definiteGenerators : _generators)
+        : (definite ? _definiteCacheNames : _cacheNames);
+    if (!table.isNamed(type)) {
+      if (recordScopeDependencies(type)) return typeRep;
+    }
+    return table.nameType(type, typeRep);
+  }
+}