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..3772328961a711496ee484e0ed36f20e3d5df43f 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,8 @@ import 'rewriter.dart' show AstRewriter, BlockRewriter, InitializerListRewriter;
 
 class ClosureConverter extends Transformer {
   final CoreTypes coreTypes;
-  final Set<VariableDeclaration> capturedVariables;
+  final Map<VariableDeclaration, int /*capture flags -- see info.dart*/ >

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

Please, add put this comment on its own line above the variable declaration. 
Also, it would be nice to have a small note about how the flag values are
interpreted by the converter.

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

Done.

+      capturedVariables;
   final Map<FunctionNode, Set<TypeParameter>> capturedTypeVariables;
   final Map<FunctionNode, VariableDeclaration> thisAccess;
   final Map<FunctionNode, String> localNames;
@@ -181,44 +183,125 @@ class ClosureConverter extends Transformer {
     return node;
   }
 
-  void extendContextWith(VariableDeclaration parameter) {
-    context.extend(parameter, new VariableGet(parameter));
-  }
+  extendContextConditionally(bool inInitializer) =>

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

As Karl pointed out, the arrow notation is usually used when the expression can
be put on the same line.  Personally, I don't mind this, but I think it would be
nice to have the same style throughout the code.

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

I would suggest to make [inInitializer] a named parameter.  Having it a
positional parameter makes invocations like `extendContextConditionally(true)`
read as `true` is the condition on which we extend the context.  I think it
would be less ambiguous to write `extendContextConditionally(inInitializer:
true)` implying that the condition is determined somewhere else.

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

Done.

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

Done.

+      (VariableDeclaration parameter) {
+        var captured = capturedVariables[parameter];

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

Also I think that the name [captured] is a bit misleading.  We don't capture the
flags; the flags describe some conditions on the captured variable.  How about
the name "capturingFlags" or just "flags" here?

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

I think using type annotation `int` here helps with readability.  We know that
we expect a flag here.  Types are nullable in Dart, so using `int` is perfectly
fine.

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

Done.

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

Done.

+
+        // 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 (captured != null && inInitializer
+            ? (captured & ClosureInfo.INSIDE_INITIALIZER) > 0
+            : captured == ClosureInfo.OUTSIDE_INITIALIZER) {
+          context.extend(parameter, new VariableGet(parameter));
+        }
+        return captured ?? 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 =
+          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(true))
+              .any((flags) => flags == capturedBoth) ||
+          node.function.namedParameters
+              .map(extendContextConditionally(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);
+      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) {

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

This version is definitely better.  I was thinking about keeping the context
declaration and other initializers in separate places from the beginning, with
descriptive names.  This way we may avoid the loop here.

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

True, but I feel that this approach keeps the logic associated with this
particular transformation mostly isolated in this function.

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

But it's not completely isolated.  I think that subclassing [AstRewriter] is a
better way to do that.  This way we would save some time for guessing to the
future reader.  Also, writing a sublclass of [AstRewriter] for redirecting
constructor makes sense, because the redirecting technique may be seen as the
special case for creating contexts, and this is exactly why we have the already
existing subclasses of [AstRewriter], that is, to treat different cases for
context creation differently.

However, feel free to leave the code as it is.

+          if (init is LocalInitializer && init.variable == contextDecl) {
+            contextDeclInit = init;
+          } else {
+            newCtor.initializers.add(init);
+          }
+        }
+
+        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);
+        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 +357,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());
     }
@@ -369,6 +452,10 @@ class ClosureConverter extends Transformer {
     FunctionNode function = node.function;
     if (function.body != null) {
       setupContextForFunctionBody(function);
+      // Start with no context.  This happens after setting up _currentBlock
+      // so statements can be emitted into _currentBlock if necessary.
+      context = new NoContext(this);

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

Why was [context] initialization moved here?

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

It didn't really make sense to do it in the function
"setupContextForFunctionBody" (now called "setupRewriterForFunctionBody")
because I needed to execute this line 
 conditionally at the other call side, and there are only two callers.

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

If we had a special kind of rewriter for the case when we need the redirecting
technique, we may have used it in, for example, a factory of [Context] like
this:

```
// In [setupContextForFunctionBody], converter.dart.
context = new Context.emptyContextForConverter(this);
```

```
// In [Context], context.dart.
factory Context.emptyContextForConverter(Converter converter) {
  if (converter.rewriter is RedirectingRewriter) {
    // In case of [RedirectingRewriter] the new context chain was
    // created while visiting the constructor that makes the redirection.
    return converter.context;
  }
  return new NoContext(converter);
}
```

That would save some guessing about some special context treatment.  Feel free
to leave the code as it is though.

+
       VariableDeclaration self = thisAccess[currentMemberFunction];
       if (self != null) {
         context.extend(self, new ThisExpression());
@@ -386,9 +473,6 @@ class ClosureConverter extends Transformer {
     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 +487,17 @@ 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?
-    node.positionalParameters
-        .where(capturedVariables.contains)
-        .forEach(extendContextWith);
-    node.namedParameters
-        .where(capturedVariables.contains)
-        .forEach(extendContextWith);
+    // Initializers for optional parameters must be compile-time constants,
+    // which excludes closures.
+    node.positionalParameters.forEach(extendContextConditionally(false));
+    node.namedParameters.forEach(extendContextConditionally(false));
     assert(node.body != null);
     node.body = node.body.accept(this);
     node.body.parent = node;
@@ -435,7 +515,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 +535,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 +543,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 +579,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 +616,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 +653,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 +662,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);