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;
+
+  // 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;
 
   /// 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}) {
+    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;
   }
 
   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 73 matching lines @@
         closedTopLevelFunction, accessContext, closureType, fnTypeArgs);
   }
 
   TreeNode visitProcedure(Procedure node) {
     assert(isEmptyContext);
 
     currentMember = node;
 
     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());
       }
       node.transformChildren(this);
       resetContext();
     }
 
     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() {
     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?
+    // 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;
     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);
   }
 }