Fix duplicate context creation when closures appear in initializers.

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 713ab38cdc6995e812fabb5dad3932f2fabd491d..d89cb843388febee2dc3dda254fb5c5983f3c182 100644
--- a/pkg/kernel/lib/transformations/closure/converter.dart
+++ b/pkg/kernel/lib/transformations/closure/converter.dart
@@ -28,6 +28,7 @@ import '../../ast.dart'
         InvocationExpression,
         Library,
         LocalInitializer,
+        RedirectingInitializer,
         Member,
         MethodInvocation,
         Name,
@@ -61,7 +62,7 @@ import '../../type_algebra.dart' show substitute;
 
 import 'clone_without_body.dart' show CloneWithoutBody;
 
-import 'context.dart' show Context, NoContext;
+import 'context.dart' show Context, NoContext, LocalContext;
 
 import 'info.dart' show ClosureInfo;
 
@@ -205,20 +206,80 @@ class ClosureConverter extends Transformer {
 
       transformList(node.initializers, this, node);
       node.initializers.insertAll(0, initRewriter.prefix);
-      context = rewriter = null;
+      rewriter = null;
     }
 
     // Transform constructor body.
     FunctionNode function = node.function;
     if (function.body != null && function.body is! EmptyStatement) {
-      setupContextForFunctionBody(function);
+      // If we created a context for the initializers, we need to re-use that
+      // context in the body of the function. Unfortunately, the context is
+      // declared in a local initializer and local initializers aren't visible
+      // in the body of the constructor. To work around this issue, we move the
+      // body into a new constructor and make this constructor redirect to that
+      // one, passing the context as an argument to the new constructor.
+      var movingCtor = context != null && context is! NoContext;
+
+      setupContextForFunctionBody(function, context);
       VariableDeclaration self = thisAccess[currentMemberFunction];
       if (self != null) {
         context.extend(self, new ThisExpression());
       }
       node.function.accept(this);
-      resetContext();
+
+      if (movingCtor) {
+        var newCtorName = new Name("${node.name.name}#redir");
+        var newCtor = new Constructor(node.function, name: newCtorName);
+        newClassMembers.add(newCtor);
+
+        for (var i = 1; i < node.initializers.length; ++i)

[Dmitry Stefantsov, 2017/07/28 12:36:16]

I guess here we make some assumptions about the structure of node.initializers,
that is, that the first initializer is different from the others.  Can we make
it more explicit in the code?  So that if something changes in the structure of
the initializers above, the code here won't be broken.  At the very least, a
comment is required.

[sjindel, 2017/07/31 12:06:20]

I rewrote it to look for the context's initializer specifically.

+          newCtor.initializers.add(node.initializers[i]);
+
+        node.initializers = node.initializers.take(1).toList();
+
+        var cv = new CloneVisitor();
+        var oldCtorParams = function.positionalParameters
+            .map(cv.visitVariableDeclaration)
+            .toList();
+        var oldCtorNamedParams =
+            function.namedParameters.map(cv.visitVariableDeclaration).toList();
+
+        function.positionalParameters.addAll(function.namedParameters);
+        function.namedParameters = [];
+
+        var args = <Expression>[];
+        assert(function.typeParameters.length == 0);

[Dmitry Stefantsov, 2017/07/28 12:36:15]

I think we can't supply additional type arguments to constructors, only those
that are specified at the class level.  So, I think function.typeParameters
should be empty anyway.  But we may need additional clarification for that.

[sjindel, 2017/07/31 12:06:20]

The comment above the [Constructor] class confirms this:

/// Constructors do not take type parameters.  Type arguments from a constructor
/// invocation should be matched with the type parameters declared in the class.

+        args.addAll(oldCtorParams.map((decl) => new VariableGet(decl)));
+        args.addAll(oldCtorNamedParams.map((decl) => new VariableGet(decl)));
+
+        node.function = new FunctionNode(new EmptyStatement(),
+            typeParameters: [],
+            positionalParameters: oldCtorParams,
+            namedParameters: oldCtorNamedParams,
+            requiredParameterCount: function.requiredParameterCount,
+            returnType: function.returnType,
+            asyncMarker: function.asyncMarker,
+            dartAsyncMarker: function.dartAsyncMarker);
+        node.function.parent = node;
+
+        var contextDecl = (context as LocalContext).self;
+        var oldCtorDecl =
+            (new CloneVisitor()).visitVariableDeclaration(contextDecl);

[Dmitry Stefantsov, 2017/07/28 12:36:16]

Can we use [cv] from above here?

[sjindel, 2017/07/31 12:06:20]

Done.

+        contextDecl.initializer = null;
+        function.positionalParameters.add(contextDecl);
+        function.requiredParameterCount++;
+
+        var contextInit = node.initializers[0] as LocalInitializer;

[Dmitry Stefantsov, 2017/07/28 12:36:16]

Same note about treating 0-th initializer as special from above applies here.

[sjindel, 2017/07/31 12:06:20]

Done.

+        contextInit.variable = oldCtorDecl;
+        oldCtorDecl.parent = contextInit;
+
+        args.add(new VariableGet(oldCtorDecl));
+        var redirInit =
+            new RedirectingInitializer(newCtor, new Arguments(args));
+        node.initializers.add(redirInit);
+      }
     }
+    resetContext();
     return node;
   }
 
@@ -380,7 +441,8 @@ class ClosureConverter extends Transformer {
     return node;
   }
 
-  void setupContextForFunctionBody(FunctionNode function) {
+  void setupContextForFunctionBody(FunctionNode function,
+      [Context reuse = null]) {

[Dmitry Stefantsov, 2017/07/28 12:36:16]

Why do we need this optional parameter here?

[sjindel, 2017/07/31 12:06:20]

So we don't reset the function body's context when an initializer has already
been set up to hold the context.

[Dmitry Stefantsov, 2017/07/31 15:05:35]

I asked this question because [reuse] wasn't mentioned anywhere in the method
body.

[sjindel, 2017/07/31 15:32:16]

I see, I suppose it wasn't actually needed then.

     Statement body = function.body;
     assert(body != null);
     currentMemberFunction = function;
@@ -388,7 +450,7 @@ class ClosureConverter extends Transformer {
     rewriter = makeRewriterForBody(function);
     // Start with no context.  This happens after setting up _currentBlock
     // so statements can be emitted into _currentBlock if necessary.
-    context = new NoContext(this);
+    if (context == null) context = new NoContext(this);
   }
 
   void resetContext() {
@@ -412,12 +474,17 @@ class ClosureConverter extends Transformer {
     transformList(node.typeParameters, this, node);
     // TODO: Can parameters contain initializers (e.g., for optional ones) that
     // need to be closure converted?
-    node.positionalParameters
-        .where(capturedVariables.contains)
-        .forEach(extendContextWith);
-    node.namedParameters
-        .where(capturedVariables.contains)
-        .forEach(extendContextWith);
+    //
+    // Answer: no, initializers for optional parameters must be compile-time
+    // constants, which excludes closures.

[Dmitry Stefantsov, 2017/07/28 12:36:15]

I would suggest rewriting this comment, so that it is not a TODO-comment.

[sjindel, 2017/07/31 12:06:20]

Done.

+    if (context is NoContext) {

[Dmitry Stefantsov, 2017/07/28 12:36:15]

Why do we need this check here?

[sjindel, 2017/07/31 12:06:20]

We don't want to re-visit the function's parameters if they were already handled
in "visitConstructor".

[Dmitry Stefantsov, 2017/07/31 15:05:35]

Yes, I meant that if this check makes distinction between two cases, they should
be more clearly and descriptively separated.

[sjindel, 2017/07/31 15:32:16]

The case is removed.

+      node.positionalParameters
+          .where(capturedVariables.contains)
+          .forEach(extendContextWith);
+      node.namedParameters
+          .where(capturedVariables.contains)
+          .forEach(extendContextWith);
+    }
     assert(node.body != null);
     node.body = node.body.accept(this);
     node.body.parent = node;