Fix duplicate context creation when closures appear in initializers.

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,
@@ skipping 10 matching lines @@
         ForStatement,
         FunctionDeclaration,
         FunctionExpression,
         FunctionNode,
         FunctionType,
         InterfaceType,
         InvalidExpression,
         InvocationExpression,
         Library,
         LocalInitializer,
+        RedirectingInitializer,
         Member,
         MethodInvocation,
         Name,
         NamedExpression,
         NamedType,
         NullLiteral,
         Procedure,
         ProcedureKind,
         PropertyGet,
         ReturnStatement,
@@ skipping 13 matching lines @@
 import '../../frontend/accessors.dart' show VariableAccessor;
 
 import '../../clone.dart' show CloneVisitor;
 
 import '../../core_types.dart' show CoreTypes;
 
 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;
 
 import 'rewriter.dart' show AstRewriter, BlockRewriter, InitializerListRewriter;
 
 class ClosureConverter extends Transformer {
   final CoreTypes coreTypes;
   final Set<VariableDeclaration> capturedVariables;
   final Map<FunctionNode, Set<TypeParameter>> capturedTypeVariables;
   final Map<FunctionNode, VariableDeclaration> thisAccess;
@@ skipping 123 matching lines @@
       // TODO(karlklose): add a fine-grained analysis of captured parameters.
       node.function.positionalParameters
           .where(capturedVariables.contains)
           .forEach(extendContextWith);
       node.function.namedParameters
           .where(capturedVariables.contains)
           .forEach(extendContextWith);
 
       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;
   }
 
   AstRewriter makeRewriterForBody(FunctionNode function) {
     Statement body = function.body;
     if (body is! Block) {
       body = new Block(<Statement>[body]);
       function.body = function.body.parent = body;
     }
     return new BlockRewriter(body);
@@ skipping 141 matching lines @@
       if (self != null) {
         context.extend(self, new ThisExpression());
       }
       node.transformChildren(this);
       resetContext();
     }
 
     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;
     // Ensure that the body is a block which becomes the current block.
     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() {
     rewriter = null;
     context = null;
     currentMemberFunction = null;
     currentMember = null;
   }
 
   bool get isEmptyContext {
     return rewriter == null && context == null;
   }
 
   TreeNode visitLocalInitializer(LocalInitializer node) {
     assert(!capturedVariables.contains(node.variable));
     node.transformChildren(this);
     return node;
   }
 
   TreeNode visitFunctionNode(FunctionNode node) {
     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;
     return node;
   }
 
   TreeNode visitBlock(Block node) {
     return saveContext(() {
       BlockRewriter blockRewriter = rewriter = rewriter.forNestedBlock(node);
       blockRewriter.transformStatements(this);
@@ skipping 264 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);
   }
 }