Preserve type variables in closure conversion.

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

 // Copyright (c) 2016, 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 kernel.transformations.closure.converter;
 
 import '../../ast.dart'
     show
         Arguments,
         Block,
         Catch,
         Class,
         ClosureCreation,
         Constructor,
         DartType,
-        DynamicType,
         EmptyStatement,
         Expression,
         ExpressionStatement,
         Field,
         ForInStatement,
         ForStatement,
         FunctionDeclaration,
         FunctionExpression,
         FunctionNode,
         FunctionType,
@@ skipping 226 matching lines @@
     assert(body != null);
 
     rewriter = makeRewriterForBody(function);
 
     VariableDeclaration contextVariable =
         new VariableDeclaration("#contextParameter", type: const VectorType());
     Context parent = context;
     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
     // either dynamic or their bounds. This is to temporarily remove the type
     // variables from closure conversion. They should be returned after the VM
     // changes are done to support vectors and closure creation. See #29181.
     Map<TypeParameter, DartType> replacementTypeSubstitution =
         <TypeParameter, DartType>{};
     for (TypeParameter parameter in typeSubstitution.keys) {
       replacementTypeSubstitution[parameter] = const DynamicType();
     }
     for (TypeParameter parameter in typeSubstitution.keys) {
       if (!isObject(parameter.bound)) {
         replacementTypeSubstitution[parameter] =
             substitute(parameter.bound, replacementTypeSubstitution);
       }
     }
     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
     // class, because the type variables in this function body are already
     // replaced with either dynamic or their bounds. This change should be
     // 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;
   }
 
   TreeNode visitFunctionDeclaration(FunctionDeclaration node) {
     /// Is this closure itself captured by a closure?
     bool isCaptured = capturedVariables.contains(node.variable);
     if (isCaptured) {
       context.extend(node.variable, new InvalidExpression());
     }
     Context parent = context;
     return saveContext(() {
       Expression expression = handleLocalFunction(node.function);
 
       if (isCaptured) {
         parent.update(node.variable, expression);
         return null;
       } else {
         node.variable.initializer = expression;
         expression.parent = node.variable;
         return node.variable;
       }
     });
   }
 
-  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:
   ///
   ///     static method closure#0(Vector #c, /* Parameters of [function]. */)
   ///         → dynamic {
   ///
   ///       /* Context is represented by #c. */
   ///
   ///       /* Body of [function]. */
   ///
   ///     }
   ///
   /// Returns an invocation of the closure creation primitive that binds the
   /// above top-level function to a context represented as Vector.
   Expression addClosure(
       FunctionNode function,
       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(
         new Name(createNameForClosedTopLevelFunction(function)),
         ProcedureKind.Method,
         function,
         isStatic: true,
         fileUri: currentFileUri);
     newLibraryMembers.add(closedTopLevelFunction);
 
     FunctionType closureType = new FunctionType(
         function.positionalParameters
             .skip(1)
             .map((VariableDeclaration decl) => decl.type)
             .toList(),
         function.returnType,
         namedParameters: function.namedParameters
             .map((VariableDeclaration decl) =>
                 new NamedType(decl.name, decl.type))
             .toList(),
         typeParameters: function.typeParameters,
         requiredParameterCount: function.requiredParameterCount - 1);
 
     return new ClosureCreation(
-        closedTopLevelFunction, accessContext, closureType);
+        closedTopLevelFunction, accessContext, closureType, newTypeArgs);
   }
 
   TreeNode visitProcedure(Procedure node) {
     assert(isEmptyContext);
 
     currentMember = node;
 
     FunctionNode function = node.function;
     if (function.body != null) {
       setupContextForFunctionBody(function);
@@ skipping 273 matching lines @@
     return new FunctionNode(new ReturnStatement(invocation),
         typeParameters: typeParameters,
         positionalParameters: positionalParameters,
         namedParameters: namedParameters,
         requiredParameterCount: requiredParameterCount,
         returnType: substitute(function.returnType, cloner.typeSubstitution));
   }
 
   /// 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);
       }
     });
     return substitution;
   }
 
   String createNameForClosedTopLevelFunction(FunctionNode function) {
     return 'closure#${localNames[function]}';
@@ skipping 17 matching lines @@
     copy.function.body.parent = copy.function;
     return copy;
   }
 
   void addGetterForwarder(Name name, Procedure getter) {
     assert(getter.isGetter);
     newClassMembers
         .add(copyWithBody(getter, forwardToThisProperty(getter))..name = name);
   }
 }