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..3db6e15b20968168331e1b819e6bc48806870f9d 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;
 
@@ -69,7 +70,11 @@ import 'rewriter.dart' show AstRewriter, BlockRewriter, InitializerListRewriter;
 
 class ClosureConverter extends Transformer {
   final CoreTypes coreTypes;
-  final Set<VariableDeclaration> capturedVariables;
+
+  // This map pairs variables that are captured with flags indicating whether
+  // they are captured inside or outside an initializer.

[Dmitry Stefantsov, 2017/08/01 09:36:13]

Please, add a reference to the field in [ClosureInfo] with the explanation of
the flags values.

[sjindel, 2017/08/01 14:58:25]

Done.

+  final Map<VariableDeclaration, int> capturedVariables;
+
   final Map<FunctionNode, Set<TypeParameter>> capturedTypeVariables;
   final Map<FunctionNode, VariableDeclaration> thisAccess;
   final Map<FunctionNode, String> localNames;
@@ -181,44 +186,126 @@ class ClosureConverter extends Transformer {
     return node;
   }
 
-  void extendContextWith(VariableDeclaration parameter) {
-    context.extend(parameter, new VariableGet(parameter));
+  extendContextConditionally({bool inInitializer}) {
+    return (VariableDeclaration parameter) {
+      int flags = capturedVariables[parameter];
+
+      // When moving variables into the context while scanning initializers,
+      // we need to add the variable if it's captured in an initializer,
+      // whether or not it's used/captured in the body. However, in the body,
+      // we only need to add the variable into the context if it's *not*
+      // captured in an initializer.
+      if (flags != null && inInitializer
+          ? (flags & ClosureInfo.INSIDE_INITIALIZER) > 0
+          : flags == ClosureInfo.OUTSIDE_INITIALIZER) {

[Dmitry Stefantsov, 2017/08/01 09:36:13]

How about breaking this condition into many, for example, this way:

```
if (flags == null) {
  // The variable is not captured.
  return 0;
}
if (inInitializer && (flags & ClosureInfo.INSIDE_INITIALIZER) == 0) {
  // The variable is not captured in an initializer, so we don't need
  // to extend the context in initializers with it.
  return flags;
}
if (!inInitializer && flags != ClosureInfo.OUTSIDE_INITIALIZER) {
  // The variable wasn't used in the body of a constructor or a function,
  // so we don't need to extend the context in a body with it.
  return flags;
}
context.extend(parameter, new VariableGet(parameter));
return flags;
```

Feel free to leave the code as it is though.

+        context.extend(parameter, new VariableGet(parameter));
+      }
+      return flags ?? 0;
+    };
   }
 
   TreeNode visitConstructor(Constructor node) {
     assert(isEmptyContext);
     currentMember = node;
 
+    // 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 = false;
+
     // Transform initializers.
     if (node.initializers.length > 0) {
       var initRewriter = new InitializerListRewriter(node);
       rewriter = initRewriter;
       context = new NoContext(this);
 
+      int capturedBoth =

[Dmitry Stefantsov, 2017/08/01 09:36:13]

I think using `final` modifier makes sense for this variable.

[sjindel, 2017/08/01 14:58:25]

Done.

+          ClosureInfo.OUTSIDE_INITIALIZER | ClosureInfo.INSIDE_INITIALIZER;
+
       // 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);
+      movingCtor = node.function.positionalParameters
+              .map(extendContextConditionally(inInitializer: true))
+              .any((flags) => flags == capturedBoth) ||
+          node.function.namedParameters
+              .map(extendContextConditionally(inInitializer: true))
+              .any((flags) => flags == capturedBoth);
 
       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);
+      setupRewriterForFunctionBody(function);
+      if (!movingCtor) context = new NoContext(this);
       VariableDeclaration self = thisAccess[currentMemberFunction];
       if (self != null) {
         context.extend(self, new ThisExpression());
       }
       node.function.accept(this);
-      resetContext();
+
+      if (movingCtor) {
+        var contextDecl = (context as LocalContext).self;
+        var newCtorName = new Name("${node.name.name}#redir");
+        var newCtor = new Constructor(node.function, name: newCtorName);
+        newClassMembers.add(newCtor);
+
+        LocalInitializer contextDeclInit = null;
+        for (var init in node.initializers) {
+          if (init is LocalInitializer && init.variable == contextDecl) {
+            contextDeclInit = init;
+          } else {
+            newCtor.initializers.add(init);
+          }
+        }
+
+        node.initializers = node.initializers.take(1).toList();

[Dmitry Stefantsov, 2017/08/01 09:36:13]

Here we still treat the first initializer as special, although we now have it in
a local variable with a descriptive name.  How about doing the following here?

```
node.initializers = <Initializer>[contextDeclInit];
```

[sjindel, 2017/08/01 14:58:25]

Done.

+
+        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/08/01 09:36:13]

If we are now sure that the [FunctionNode] of the constructor may not have its
own type parameters, how about removing this line?

[sjindel, 2017/08/01 14:58:25]

Done.

+        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 oldCtorDecl = cv.visitVariableDeclaration(contextDecl);
+        contextDecl.initializer = null;
+        function.positionalParameters.add(contextDecl);
+        function.requiredParameterCount++;
+
+        contextDeclInit.variable = oldCtorDecl;
+        oldCtorDecl.parent = contextDeclInit;
+
+        args.add(new VariableGet(oldCtorDecl));
+        var redirInit =
+            new RedirectingInitializer(newCtor, new Arguments(args));
+        node.initializers.add(redirInit);
+      }
     }
+    resetContext();
     return node;
   }
 
@@ -274,7 +361,7 @@ class ClosureConverter extends Transformer {
 
   TreeNode visitFunctionDeclaration(FunctionDeclaration node) {
     /// Is this closure itself captured by a closure?
-    bool isCaptured = capturedVariables.contains(node.variable);
+    bool isCaptured = capturedVariables.containsKey(node.variable);
     if (isCaptured) {
       context.extend(node.variable, new InvalidExpression());
     }
@@ -368,7 +455,11 @@ class ClosureConverter extends Transformer {
 
     FunctionNode function = node.function;
     if (function.body != null) {
-      setupContextForFunctionBody(function);
+      setupRewriterForFunctionBody(function);
+      // Start with no context.  This happens after setting up _currentBlock
+      // so statements can be emitted into _currentBlock if necessary.
+      context = new NoContext(this);
+
       VariableDeclaration self = thisAccess[currentMemberFunction];
       if (self != null) {
         context.extend(self, new ThisExpression());
@@ -380,15 +471,12 @@ class ClosureConverter extends Transformer {
     return node;
   }
 
-  void setupContextForFunctionBody(FunctionNode function) {
+  void setupRewriterForFunctionBody(FunctionNode function) {
     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);
   }
 
   void resetContext() {
@@ -403,21 +491,19 @@ class ClosureConverter extends Transformer {
   }
 
   TreeNode visitLocalInitializer(LocalInitializer node) {
-    assert(!capturedVariables.contains(node.variable));
+    assert(!capturedVariables.containsKey(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?
+    // Initializers for optional parameters must be compile-time constants,
+    // which excludes closures.

[Dmitry Stefantsov, 2017/08/01 09:36:13]

Looks like the cause for this comment was lost during rewriting :) How about
adding the following?

"Therefore, we can avoid looking for closures in initializers of the
parameters."

[sjindel, 2017/08/01 14:58:25]

Done.

     node.positionalParameters
-        .where(capturedVariables.contains)
-        .forEach(extendContextWith);
+        .forEach(extendContextConditionally(inInitializer: false));
     node.namedParameters
-        .where(capturedVariables.contains)
-        .forEach(extendContextWith);
+        .forEach(extendContextConditionally(inInitializer: false));
     assert(node.body != null);
     node.body = node.body.accept(this);
     node.body.parent = node;
@@ -435,7 +521,7 @@ class ClosureConverter extends Transformer {
   TreeNode visitVariableDeclaration(VariableDeclaration node) {
     node.transformChildren(this);
 
-    if (!capturedVariables.contains(node)) return node;
+    if (!capturedVariables.containsKey(node)) return node;
     if (node.initializer == null && node.parent is FunctionNode) {
       // If the variable is a function parameter and doesn't have an
       // initializer, just use this variable name to put it into the context.
@@ -455,7 +541,7 @@ class ClosureConverter extends Transformer {
   }
 
   TreeNode visitVariableGet(VariableGet node) {
-    return capturedVariables.contains(node.variable)
+    return capturedVariables.containsKey(node.variable)
         ? context.lookup(node.variable)
         : node;
   }
@@ -463,7 +549,7 @@ class ClosureConverter extends Transformer {
   TreeNode visitVariableSet(VariableSet node) {
     node.transformChildren(this);
 
-    return capturedVariables.contains(node.variable)
+    return capturedVariables.containsKey(node.variable)
         ? context.assign(node.variable, node.value,
             voidContext: isInVoidContext(node))
         : node;
@@ -499,7 +585,7 @@ class ClosureConverter extends Transformer {
   }
 
   TreeNode visitForStatement(ForStatement node) {
-    if (node.variables.any(capturedVariables.contains)) {
+    if (node.variables.any(capturedVariables.containsKey)) {
       // In Dart, loop variables are new variables on each iteration of the
       // loop. This is only observable when a loop variable is captured by a
       // closure, which is the situation we're in here. So we transform the
@@ -536,7 +622,7 @@ class ClosureConverter extends Transformer {
   }
 
   TreeNode visitForInStatement(ForInStatement node) {
-    if (capturedVariables.contains(node.variable)) {
+    if (capturedVariables.containsKey(node.variable)) {
       // In Dart, loop variables are new variables on each iteration of the
       // loop. This is only observable when the loop variable is captured by a
       // closure, so we need to transform the for-in loop when `node.variable`
@@ -573,7 +659,7 @@ class ClosureConverter extends Transformer {
   TreeNode visitCatch(Catch node) {
     VariableDeclaration exception = node.exception;
     VariableDeclaration stackTrace = node.stackTrace;
-    if (stackTrace != null && capturedVariables.contains(stackTrace)) {
+    if (stackTrace != null && capturedVariables.containsKey(stackTrace)) {
       Block block = node.body = ensureBlock(node.body);
       block.parent = node;
       node.stackTrace = new VariableDeclaration(null);
@@ -582,7 +668,7 @@ class ClosureConverter extends Transformer {
       block.statements.insert(0, stackTrace);
       stackTrace.parent = block;
     }
-    if (exception != null && capturedVariables.contains(exception)) {
+    if (exception != null && capturedVariables.containsKey(exception)) {
       Block block = node.body = ensureBlock(node.body);
       block.parent = node;
       node.exception = new VariableDeclaration(null);