Preserve type variables in closure conversion.

pkg/kernel/lib/transformations/closure/converter.dart

Index: pkg/kernel/lib/transformations/closure/converter.dart
diff --git a/pkg/kernel/lib/transformations/closure/converter.dart b/pkg/kernel/lib/transformations/closure/converter.dart
index 5ee224172f0006729dda2786a0733ec62dadfdff..dd7b644ea159d8b6ac361e011e1f7ada266c7da7 100644
--- a/pkg/kernel/lib/transformations/closure/converter.dart
+++ b/pkg/kernel/lib/transformations/closure/converter.dart
@@ -13,7 +13,6 @@ import '../../ast.dart'
         ClosureCreation,
         Constructor,
         DartType,
-        DynamicType,
         EmptyStatement,
         Expression,
         ExpressionStatement,
@@ -260,12 +259,12 @@ class ClosureConverter extends Transformer {
     context = context.toNestedContext(
         new VariableAccessor(contextVariable, null, TreeNode.noOffset));
 
-    Set<TypeParameter> captured = capturedTypeVariables[currentFunction];
-    if (captured != null) {
-      typeSubstitution = copyTypeVariables(captured);
-    } else {
-      typeSubstitution = const <TypeParameter, DartType>{};
-    }
+    Set<TypeParameter> captured =
+        capturedTypeVariables[currentFunction] ?? new Set<TypeParameter>();
+    var newParams = <TypeParameter>[], newArgs = <TypeParameterType>[];

[karlklose, 2017/07/25 09:38:23]

Use one variable declaration per variable.

[sjindel, 2017/07/25 13:11:48]

Code removed.

+    typeSubstitution = copyTypeVariables(captured, newParams, newArgs);
+
+    /****

[Dmitry Stefantsov, 2017/07/25 09:13:34]

Please, remove the code if it's no longer needed.

[sjindel, 2017/07/25 13:11:48]

Done.

 
     // TODO(29181): remove replacementTypeSubstitution variable and its usages.
     // All the type variables used in this function body are replaced with
@@ -284,7 +283,6 @@ class ClosureConverter extends Transformer {
       }
     }
     typeSubstitution = replacementTypeSubstitution;
-    function.transformChildren(this);
 
     // TODO(29181): don't replace typeSubstitution with an empty map.
     // Information about captured type variables is deleted from the closure
@@ -293,8 +291,12 @@ class ClosureConverter extends Transformer {
     // undone after the VM support for vectors and closure creation is
     // implemented. See #29181.
     typeSubstitution = <TypeParameter, DartType>{};
+
+    ****/
+    function.transformChildren(this);
+
     Expression result = addClosure(function, contextVariable, parent.expression,
-        typeSubstitution, enclosingTypeSubstitution);
+        typeSubstitution, enclosingTypeSubstitution, newParams, newArgs);
     currentFunction = enclosingFunction;
     typeSubstitution = enclosingTypeSubstitution;
     return result;
@@ -321,11 +323,8 @@ class ClosureConverter extends Transformer {
     });
   }
 
-  TreeNode visitFunctionExpression(FunctionExpression node) {
-    return saveContext(() {
-      return handleLocalFunction(node.function);
-    });
-  }
+  TreeNode visitFunctionExpression(FunctionExpression node) =>

[karlklose, 2017/07/25 09:38:23]

I prefer using {} and a return statement instead of '=>' if the expression does
not fit on the same line.

[sjindel, 2017/07/25 13:11:48]

Done.

+      saveContext(() => handleLocalFunction(node.function));
 
   /// Add a new procedure to the current library that looks like this:
   ///
@@ -345,7 +344,10 @@ class ClosureConverter extends Transformer {
       VariableDeclaration contextVariable,
       Expression accessContext,
       Map<TypeParameter, DartType> substitution,
-      Map<TypeParameter, DartType> enclosingTypeSubstitution) {
+      Map<TypeParameter, DartType> enclosingTypeSubstitution,
+      List<TypeParameter> newTypeParams,
+      List<TypeParameterType> newTypeArgs) {
+    function.typeParameters.insertAll(0, newTypeParams);
     function.positionalParameters.insert(0, contextVariable);
     ++function.requiredParameterCount;
     Procedure closedTopLevelFunction = new Procedure(
@@ -370,7 +372,7 @@ class ClosureConverter extends Transformer {
         requiredParameterCount: function.requiredParameterCount - 1);
 
     return new ClosureCreation(
-        closedTopLevelFunction, accessContext, closureType);
+        closedTopLevelFunction, accessContext, closureType, newTypeArgs);
   }
 
   TreeNode visitProcedure(Procedure node) {
@@ -664,14 +666,24 @@ class ClosureConverter extends Transformer {
 
   /// Creates copies of the type variables in [original] and returns a
   /// substitution that can be passed to [substitute] to substitute all uses of
-  /// [original] with their copies.
+  /// [original] with their copies. Additionally returns a list of new type

[karlklose, 2017/07/25 09:38:23]

If you keep the "output parameters", please change this comment to say that the
method will append the new type parameters and type arguments to the lists given
as arguments.

[sjindel, 2017/07/25 13:11:48]

Removed code.

+  /// parameters to prefix to the enclosing function's type parameters and the
+  /// arguments to be passed for those parameters.
+  ///
   Map<TypeParameter, DartType> copyTypeVariables(
-      Iterable<TypeParameter> original) {
+      Iterable<TypeParameter> original,
+      List<TypeParameter> typeParams_out,

[Dmitry Stefantsov, 2017/07/25 09:13:34]

As I understand it, we don't use underscores in variable names.

[Dmitry Stefantsov, 2017/07/25 09:13:34]

I think it's better to write a separate procedure to extract type parameters and
type arguments from a substitution map than to have output type parameters.  As
an alternative, you may just extract them at the place they're needed.  I think
there's only one such place.

[sjindel, 2017/07/25 13:11:48]

Done.
There needed in two places, and the issue with this is it requires downcasting
the values in the map.

[sjindel, 2017/07/25 13:11:48]

Done.

[Dmitry Stefantsov, 2017/07/26 09:12:09]

Thanks for the explanation.  Tuples would look nice in this situation, I guess
:)

Originally, I was confused that some values are returned, and some are put into
empty data structs passed as arguments.  It also brought those "_out" suffixes
into existence.

+      List<TypeParameterType> typeArgs_out) {
     if (original.isEmpty) return const <TypeParameter, DartType>{};
+
     Map<TypeParameter, DartType> substitution = <TypeParameter, DartType>{};
     for (TypeParameter t in original) {
-      substitution[t] = new TypeParameterType(new TypeParameter(t.name));
+      typeArgs_out.add(new TypeParameterType(t));
+      var newParam = new TypeParameter(t.name);
+      typeParams_out.add(newParam);
+      substitution[t] = new TypeParameterType(newParam);
     }
+
     substitution.forEach((TypeParameter t, DartType copy) {
       if (copy is TypeParameterType) {
         copy.parameter.bound = substitute(t.bound, substitution);