Added for-loop variable tracking and regular closures/initializers captured variable tracking.

pkg/compiler/lib/src/js_model/closure_visitors.dart

Index: pkg/compiler/lib/src/js_model/closure_visitors.dart
diff --git a/pkg/compiler/lib/src/js_model/closure_visitors.dart b/pkg/compiler/lib/src/js_model/closure_visitors.dart
new file mode 100644
index 0000000000000000000000000000000000000000..f86c168f77ae538971ba32d60076a762051800b5
--- /dev/null
+++ b/pkg/compiler/lib/src/js_model/closure_visitors.dart
@@ -0,0 +1,274 @@
+// Copyright (c) 2017, 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.
+
+import 'package:kernel/ast.dart' as ir;
+
+import '../closure.dart';
+import '../elements/entities.dart';
+import 'closure.dart';
+import '../kernel/element_map.dart';
+
+/// This builder walks the code to determine what variables are captured/free at
+/// various points to build ClosureScope that can respond to queries
+/// about how a particular variable is being used at any point in the code.
+class ClosureScopeBuilder extends ir.Visitor {
+  /// A map of each visited call node with the associated information about what

[Siggi Cherem (dart-lang), 2017/06/30 22:02:09]

I am not sure what the keys of the map are, maybe you can clarify in the
comment? It appears like you have a key for every scope in the program?

[Emily Fortuna, 2017/06/30 23:48:09]

Done.

+  /// variables are captured/used.
+  Map<ir.Node, ClosureScope> _closureInfoMap = <ir.Node, ClosureScope>{};
+
+  /// A map of the elements corresponding to nodes that we have flagged as

[Siggi Cherem (dart-lang), 2017/06/30 22:02:09]

now that you are using ir.Node as keys, maybe remove "elements corresponding to
..."? For instance:

  /// Maps nodes that require the compiler to create a closure class.

[Emily Fortuna, 2017/06/30 23:48:10]

Done.

+  /// necessary to generate closure classes for in a later stage. We map that
+  /// node to information ascertained about variable usage in the surrounding

[Siggi Cherem (dart-lang), 2017/06/30 22:02:09]

I had to look it up in the dictionary :)

+  /// scope.
+  Map<ir.Node, ScopeInfo> _closuresToGenerate = <ir.Node, ScopeInfo>{};

[Siggi Cherem (dart-lang), 2017/06/30 22:02:09]

to better document what kind of nodes, sometimes we write in comments the union
of types that we expect here. For example:

Map<ir.Node /*ir.Field | ir.FunctionNode*/, ScopeInfo> ...;

It might also be worth mentioning in the doc, for instance:

 /// ...
 /// Keys can be either function nodes (explicit Dart closures) or
 /// static fields (closures are created for lazy initialization).

[Emily Fortuna, 2017/06/30 23:48:10]

Done.

+
+  /// The local variables that have been declared in the current scope.
+  List<ir.VariableDeclaration> _scopeVariables;
+
+  /// Pointer to the context in which this closure is executed.

[Siggi Cherem (dart-lang), 2017/06/30 22:02:09]

maybe include an example of what that context can be? the enclosing method?

[Emily Fortuna, 2017/06/30 23:48:10]

Done.

+  ir.Node _executableContext;
+
+  /// A flag to indicate if we are currently inside a closure.
+  bool _isInsideClosure = false;
+
+  /// Pointer to the original node where this closure builder started.
+  ir.Node _outermostNode;
+
+  /// Keep track of the mutated local variables so that we don't need to box
+  /// non-mutated variables.
+  Set<ir.VariableDeclaration> _mutatedVariables =
+      new Set<ir.VariableDeclaration>();
+
+  /// The set of variables that are accessed in some form, whether they are
+  /// mutated or not.
+  Set<ir.VariableDeclaration> _capturedVariables =
+      new Set<ir.VariableDeclaration>();
+
+  /// If true, the visitor is currently traversing some nodes that are inside a
+  /// try block.
+  bool _inTry = false;
+
+  /// Lookup the local entity that corresponds to a particular kernel node.

[Siggi Cherem (dart-lang), 2017/06/30 22:02:09]

nit: "particular kernel node" => "kernel variable declaration"?

[Emily Fortuna, 2017/06/30 23:48:09]

Done.

+  final KernelToLocalsMap _localsMap;
+
+  /// The current scope we are in.
+  KernelScopeInfo _currentScopeInfo;
+
+  final KernelToElementMap _kernelToElementMap;
+
+  ClosureScopeBuilder(this._closureInfoMap, this._closuresToGenerate,
+      this._localsMap, this._kernelToElementMap);
+
+  /// Update the [ClosureScope] object corresponding to
+  /// this node if any variables are captured.
+  void attachCapturedScopeVariables(ir.Node node) {
+    Set<Local> capturedVariablesForScope = new Set<Local>();
+
+    for (ir.VariableDeclaration variable in _scopeVariables) {
+      // No need to box non-assignable elements.
+      // TODO(efortuna): Need to figure out how to test isAssignable with Local

[Siggi Cherem (dart-lang), 2017/06/30 22:02:09]

you could directly inspect this on the ir.VariableDeclaration (asking if it's
final or const)

[Emily Fortuna, 2017/06/30 23:48:10]

oh DUH! This comment was from when I was looking at entities.

+      //if (!variable.isAssignable) continue;
+      if (!_mutatedVariables.contains(variable)) continue;
+      if (_capturedVariables.contains(variable)) {
+        capturedVariablesForScope.add(_localsMap.getLocal(variable));

[Siggi Cherem (dart-lang), 2017/06/30 22:02:09]

<no action here, just a general comment> - here it is important that we are
using the kernel-2-j-entities map on this part of the algorithm, (and same
further below when creating the ThisLocal) all of these are going to be used by
inference and ssa assuming they are part of the J world.

If we wanted to fully make this analysis agnostic of the J/K, we'd have to
replicate the ClosureScope class hierarchy using a similar structure with
ir.Nodes instead, and then convert it to ClosureScopes. IMO - not worth doing
that change as this time.

[Emily Fortuna, 2017/06/30 23:48:10]

agreed. I was seeing that as I was making the ir.Node switch, but pure-ir
version looked to be further in the future.

+      }
+    }
+    if (!capturedVariablesForScope.isEmpty) {
+      ThisLocal thisLocal = null;
+      if (node is ir.Member && node.isInstanceMember) {
+        if (node is ir.Procedure) {
+          thisLocal = new ThisLocal(_kernelToElementMap.getMethod(node));
+        } else if (node is ir.Field) {
+          thisLocal = new ThisLocal(_kernelToElementMap.getField(node));
+        }
+      } else if (node is ir.Constructor) {
+        thisLocal = new ThisLocal(_kernelToElementMap.getConstructor(node));
+      }
+
+      Entity context;
+      if (_executableContext is ir.Member) {
+        context = _kernelToElementMap.getMember(_executableContext);
+      } else {
+        context = _kernelToElementMap.getLocalFunction(_executableContext);
+      }
+      _closureInfoMap[node] =
+          new KernelClosureScope(capturedVariablesForScope, context, thisLocal);
+    }
+  }
+
+  /// Perform book-keeping with the current set of local variables that have
+  /// been seen thus far before entering this new scope.
+  void enterNewScope(ir.Node node, Function visitNewScope) {
+    List<ir.VariableDeclaration> oldScopeVariables = _scopeVariables;
+    _scopeVariables = <ir.VariableDeclaration>[];
+    visitNewScope();
+    attachCapturedScopeVariables(node);
+    _mutatedVariables.removeAll(_scopeVariables);
+    _scopeVariables = oldScopeVariables;
+  }
+
+  @override
+  void defaultNode(ir.Node node) {
+    node.visitChildren(this);
+  }
+
+  @override
+  visitTryCatch(ir.TryCatch node) {
+    bool oldInTry = _inTry;
+    _inTry = true;
+    node.visitChildren(this);
+    _inTry = oldInTry;
+  }
+
+  @override
+  visitTryFinally(ir.TryFinally node) {
+    bool oldInTry = _inTry;
+    _inTry = true;
+    node.visitChildren(this);
+    _inTry = oldInTry;
+  }
+
+  @override
+  visitVariableGet(ir.VariableGet node) {
+    _markVariableAsUsed(node.variable);
+  }
+
+  @override
+  visitVariableSet(ir.VariableSet node) {
+    _markVariableAsUsed(node.variable);

[Siggi Cherem (dart-lang), 2017/06/30 22:02:09]

don't we need to also add node.variable as a mutated variable here?

[Emily Fortuna, 2017/06/30 23:48:09]

thank you. you're finding all the missing pieces for where the testing needs to
go. :-P

+    node.visitChildren(this);
+  }
+
+  /// Add this variable to the set of free variables if appropriate and add to
+  /// the tally of variables used in try or sync blocks.
+  void _markVariableAsUsed(ir.VariableDeclaration variable) {
+    if (_isInsideClosure && !_inCurrentContext(variable)) {
+      _currentScopeInfo.freeVariables.add(variable);
+    }
+    if (_inTry) {
+      _currentScopeInfo.registerUsedInTryOrSync(_localsMap.getLocal(variable));
+    }
+  }
+
+  @override
+  void visitForStatement(ir.ForStatement node) {
+    List<Local> boxedLoopVariables = <Local>[];
+    enterNewScope(node, () {
+      // First visit initialized variables and update steps so we can easily
+      // check if a loop variable was captured in one of these subexpressions.
+      node.variables
+          .forEach((ir.VariableDeclaration variable) => variable.accept(this));
+      node.updates
+          .forEach((ir.Expression expression) => expression.accept(this));
+
+      // Loop variables that have not been captured yet can safely be flagged as
+      // non-mutated, because no nested function can observe the mutation.
+      for (ir.VariableDeclaration variable in node.variables) {
+        Local local = _localsMap.getLocal(variable);
+        if (!_capturedVariables.contains(variable)) {
+          _mutatedVariables.remove(local);

[Siggi Cherem (dart-lang), 2017/06/30 22:02:09]

local => variable?

In that case, it appears you can drop the call to `getLocal` too?

[Emily Fortuna, 2017/06/30 23:48:09]

oh goodness, yes, that was a holdover from the Locals-as-keys time from before. 

+        }
+      }
+
+      // Visit condition and body.
+      // This must happen after the above, so any loop variables mutated in the
+      // condition or body are indeed flagged as mutated.
+      if (node.condition != null) node.condition.accept(this);
+      node.body.accept(this);
+
+      // See if we have declared loop variables that need to be boxed.
+      if (node.variables.isEmpty) return;

[Siggi Cherem (dart-lang), 2017/06/30 22:02:09]

minor nit: delete this line? (since the next loop would be empty and is the last
thing on this method)

[Emily Fortuna, 2017/06/30 23:48:10]

Done.

+      for (ir.VariableDeclaration variable in node.variables) {
+        Local local = _localsMap.getLocal(variable);

[Siggi Cherem (dart-lang), 2017/06/30 22:02:09]

move getLocal inside the branch below?

[Emily Fortuna, 2017/06/30 23:48:09]

Done. Yeah, that was a holdover from when the entities were keys in more places.

+        // Non-mutated variables should not be boxed.  The _mutatedVariables set
+        // gets cleared when `enterNewScope` returns, so check it here.
+        if (_capturedVariables.contains(variable) &&
+            _mutatedVariables.contains(variable)) {
+          boxedLoopVariables.add(local);
+        }
+      }
+    });
+    KernelClosureScope scope = _closureInfoMap[node];
+    if (scope == null) return;
+    _closureInfoMap[node] = new KernelLoopClosureScope(scope.boxedVariables,
+        boxedLoopVariables, scope.context, scope.thisLocal);
+  }
+
+  void visitInvokable(ir.Node node) {
+    bool oldIsInsideClosure = _isInsideClosure;
+    ir.Node oldExecutableContext = _executableContext;
+    KernelScopeInfo oldScopeInfo = _currentScopeInfo;
+
+    _isInsideClosure = _outermostNode != null;
+    _executableContext = node;
+    if (!_isInsideClosure) {

[Siggi Cherem (dart-lang), 2017/06/30 22:02:09]

consider adding a comment saying that outermostNode is only null when we first
enter the body of the field/ctor/method that is being analyzed.

[Emily Fortuna, 2017/06/30 23:48:09]

Done.

+      _outermostNode = node;
+    }
+    _closuresToGenerate[node] = _currentScopeInfo;
+
+    enterNewScope(node, () {
+      node.visitChildren(this);
+    });
+
+    KernelScopeInfo savedScopeInfo = _currentScopeInfo;
+    bool savedIsInsideClosure = _isInsideClosure;
+
+    // Restore old values.
+    _isInsideClosure = oldIsInsideClosure;
+    _currentScopeInfo = oldScopeInfo;
+    _executableContext = oldExecutableContext;
+
+    // Mark all free variables as captured and expect to encounter them in the
+    // outer function.
+    Iterable<ir.VariableDeclaration> freeVariables =
+        savedScopeInfo.freeVariables;
+    assert(freeVariables.isEmpty || savedIsInsideClosure);
+    for (ir.VariableDeclaration freeVariable in freeVariables) {
+      assert(!_capturedVariables.contains(freeVariable));
+      _capturedVariables.add(freeVariable);
+      // If the element is not declared in the current function and the element
+      // is not the closure itself we need to mark the element as free variable.
+      // Note that the check on [insideClosure] is not just an

[Siggi Cherem (dart-lang), 2017/06/30 22:02:09]

seems like this comment should move next to where you check for insideClosure,
maybe move to markVariableAsUsed?

[Emily Fortuna, 2017/06/30 23:48:10]

Done.

+      // optimization: factories have type parameters as function
+      // parameters, and type parameters are declared in the class, not
+      // the factory.
+      _markVariableAsUsed(freeVariable);
+    }
+  }
+
+  /// Return true if [variable]'s context is the same as the current executable
+  /// context.
+  bool _inCurrentContext(ir.VariableDeclaration variable) {
+    ir.TreeNode node = variable;
+    while (node.parent != null) {
+      if (variable == node) return true;

[Siggi Cherem (dart-lang), 2017/06/30 22:02:09]

Did you mean to check here against _exectuableContext (rather than variable)?

[Emily Fortuna, 2017/06/30 23:48:10]

oops, good catch!

+      node = node.parent;

[Siggi Cherem (dart-lang), 2017/06/30 22:02:09]

since ir nodes are connected all the way to the library, it might be worth
stopping the loop sooner, maybe when node == _outermostNode?

[Emily Fortuna, 2017/06/30 23:48:09]

Done.

+    }
+    return variable == node;
+  }
+
+  void translateLazyInitializer(ir.Field field) {
+    _currentScopeInfo =
+        new KernelScopeInfo(new ThisLocal(_kernelToElementMap.getField(field)));
+    visitInvokable(field);
+  }
+
+  void translateConstructorOrProcedure(ir.Node constructorOrProcedure) {
+    Entity element;
+    if (constructorOrProcedure is ir.Constructor ||
+        (constructorOrProcedure is ir.Procedure &&
+            constructorOrProcedure.kind == ir.ProcedureKind.Factory)) {
+      element = _kernelToElementMap.getConstructor(constructorOrProcedure);
+    } else {
+      assert(constructorOrProcedure is ir.Procedure);
+      element = _kernelToElementMap.getMethod(constructorOrProcedure);
+    }
+    _currentScopeInfo = new KernelScopeInfo(new ThisLocal(element));
+    constructorOrProcedure.accept(this);
+  }
+
+  void visitFunctionNode(ir.FunctionNode functionNode) {
+    visitInvokable(functionNode);
+  }
+}