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<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;
 
   /// Records place-holders for cloning contexts. See [visitForStatement].
   final Set<InvalidExpression> contextClonePlaceHolders =
       new Set<InvalidExpression>();
 
   final CloneVisitor cloner = new CloneWithoutBody();
 
@@ skipping 91 matching lines @@
   TreeNode visitClass(Class node) {
     assert(newClassMembers.isEmpty);
     currentClass = node;
     node = super.visitClass(node);
     newClassMembers.forEach(node.addMember);
     newClassMembers.clear();
     currentClass = null;
     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;
   }
 
   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 35 matching lines @@
 
     Expression result = addClosure(function, contextVariable, parent.expression,
         typeSubstitution, enclosingTypeSubstitution);
     currentFunction = enclosingFunction;
     typeSubstitution = enclosingTypeSubstitution;
     return result;
   }
 
   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());
     }
     Context parent = context;
     return saveContext(() {
       Expression expression = handleLocalFunction(node.function);
 
       if (isCaptured) {
         parent.update(node.variable, expression);
         return null;
@@ skipping 74 matching lines @@
   }
 
   TreeNode visitProcedure(Procedure node) {
     assert(isEmptyContext);
 
     currentMember = node;
 
     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());
       }
       node.transformChildren(this);
       resetContext();
     }
 
     return node;
   }
 
   void setupContextForFunctionBody(FunctionNode function) {

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

Now this function name is misleading, after actual context initialization was
moved somewhere else.  Please, consider refactoring or renaming.

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

Done.

     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() {
     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));
+    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;
     return node;
   }
 
   TreeNode visitBlock(Block node) {
     return saveContext(() {
       BlockRewriter blockRewriter = rewriter = rewriter.forNestedBlock(node);
       blockRewriter.transformStatements(this);
       return node;
     });
   }
 
   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.
       context.extend(node, new VariableGet(node));
     } else {
       context.extend(node, node.initializer ?? new NullLiteral());
     }
 
     if (node.parent == currentFunction) {
       return node;
     } else {
       assert(node.parent is Block);
       // When returning null, the parent block will remove
       // this node from its list of statements.
       return null;
     }
   }
 
   TreeNode visitVariableGet(VariableGet node) {
-    return capturedVariables.contains(node.variable)
+    return capturedVariables.containsKey(node.variable)
         ? context.lookup(node.variable)
         : node;
   }
 
   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;
   }
 
   bool isInVoidContext(Expression node) {
     TreeNode parent = node.parent;
     return parent is ExpressionStatement ||
         parent is ForStatement && parent.condition != node;
   }
@@ skipping 15 matching lines @@
     InvalidExpression placeHolder = new InvalidExpression();
     contextClonePlaceHolders.add(placeHolder);
     return placeHolder;
   }
 
   TreeNode visitInvalidExpression(InvalidExpression node) {
     return contextClonePlaceHolders.remove(node) ? context.clone() : node;
   }
 
   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
       // loop.
       //
       // Consider the following example, where `x` is `node.variables.first`,
       // and `#t1` is a temporary variable:
       //
       //     for (var x = 0; x < 10; x++) body;
       //
@@ skipping 16 matching lines @@
         node.updates.insert(0, cloneContext());
         Block block = new Block(statements);
         rewriter = new BlockRewriter(block);
         return block.accept(this);
       });
     }
     return super.visitForStatement(node);
   }
 
   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`
       // is captured.
       //
       // Consider the following example, where `x` is `node.variable`, and
       // `#t1` is a temporary variable:
       //
       //     for (var x in expr) body;
       //
@@ skipping 16 matching lines @@
 
   TreeNode visitThisExpression(ThisExpression node) {
     return isOuterMostContext
         ? node
         : context.lookup(thisAccess[currentMemberFunction]);
   }
 
   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);
       node.stackTrace.parent = node;
       stackTrace.initializer = new VariableGet(node.stackTrace);
       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);
       node.exception.parent = node;
       exception.initializer = new VariableGet(node.exception);
       block.statements.insert(0, exception);
       exception.parent = block;
     }
     return super.visitCatch(node);
   }
@@ skipping 99 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);
   }
 }