Hopefully the last bit of restructuring between closture classes and loop boxing, etc.

pkg/compiler/lib/src/closure.dart

Index: pkg/compiler/lib/src/closure.dart
diff --git a/pkg/compiler/lib/src/closure.dart b/pkg/compiler/lib/src/closure.dart
index fe91dc9434b41525b105502c0844d2da107ce059..7af93c1d2b61e6e9ba9d1aa1e649cb502b4696b1 100644
--- a/pkg/compiler/lib/src/closure.dart
+++ b/pkg/compiler/lib/src/closure.dart
@@ -52,10 +52,10 @@ abstract class ClosureDataLookup<T> {
 
   /// Look up information about a loop, in case any variables it declares need
   /// to be boxed/snapshotted.
-  LoopClosureRepresentationInfo getClosureRepresentationInfoForLoop(T loopNode);
+  ClosureBase getClosureRepresentationInfoForLoop(T loopNode);
 
   /// Accessor to the information about closures that the SSA builder will use.
-  ClosureAnalysisInfo getClosureAnalysisInfo(T node);
+  ClosureBase getClosureBase(T node);
 }
 
 /// Class that represents one level of scoping information, whether this scope
@@ -70,6 +70,10 @@ abstract class ClosureDataLookup<T> {
 class ScopeInfo {
   const ScopeInfo();
 
+  /// Convenience reference pointer to the element representing `this`.
+  /// If this scope is not in an instance member, it will be null.
+  Local get thisLocal => null;
+
   /// Returns true if this [variable] is used inside a `try` block or a `sync*`
   /// generator (this is important to know because boxing/redirection needs to
   /// happen for those local variables).
@@ -82,16 +86,29 @@ class ScopeInfo {
   /// [ClosureClassMap.useLocal].
   bool variableIsUsedInTryOrSync(Local variable) => false;
 
-  /// Convenience reference pointer to the element representing `this`.
-  /// If this scope is not in an instance member, it will be null.
-  Local get thisLocal => null;
+  /// Loop through each variabe that has been defined in this scope, modified

[Johnni Winther, 2017/06/26 08:31:39]

`variabe` -> `variable`

[Emily Fortuna, 2017/06/26 21:57:19]

Done.

+  /// anywhere (this scope or another scope) and used in another scope . Because

[Johnni Winther, 2017/06/26 08:31:39]

`scope .` -> `scope.`

Start a new paragraph at`Because`

[Emily Fortuna, 2017/06/26 21:57:20]

Done.

+  /// it is used in another scope, these variables need to be "boxed", creating
+  /// a thin wrapper around accesses to these variables so that accesses get
+  /// the correct updated value. The variables in variablesUsedInTryOrSync may
+  /// be included in this set.
+  ///
+  /// In the case of loops, this is the set of iteration variables (or any
+  /// variables declared in the for loop expression (`for (...here...)`) that
+  /// need to be boxed to snapshot their value.
+  void forEachBoxedVariable(f(Local local, FieldEntity field)) {}
+
+  /// True if [variable] has been mutated and is also used in another scope.
+  bool isBoxed(Local variable) => false;
+
+  /// True if this loop declares any variables that need to be boxed.

[Johnni Winther, 2017/06/26 08:31:39]

`this loop` -> `this scope`

[Emily Fortuna, 2017/06/26 21:57:20]

thank you!

+  bool get hasBoxedVariables => false;
 }
 
-/// Class that provides a black-box interface to information gleaned from
-/// analyzing a closure's characteristics, most commonly used to influence how
-/// code should be generated in SSA builder stage.
-class ClosureAnalysisInfo {
-  const ClosureAnalysisInfo();
+/// Class that provides a basic interface for closures. Actual Dart closures
+/// will use this class (see [ClosureRepresentationInfo]) as well as loops.
+class ClosureBase extends ScopeInfo {
+  const ClosureBase();
 
   /// If true, this closure accesses a variable that was defined in an outside
   /// scope and this variable gets modified at some point (sometimes we say that
@@ -105,42 +122,6 @@ class ClosureAnalysisInfo {
   /// scoped into this context from outside. This is an accessor to the
   /// contextBox that [requiresContextBox] is testing is required.
   Local get context => null;
-
-  /// True if the specified variable has been mutated inside the scope of this
-  /// closure.
-  bool isCaptured(Local variable) => false;
-
-  /// Loop through every variable that has been captured in this closure. This
-  /// consists of all the free variables (variables captured *just* in this
-  /// closure) and all variables captured in nested scopes that we may be
-  /// capturing as well.
-  void forEachCapturedVariable(f(Local from, FieldEntity to)) {}
-}
-
-/// Class that describes the actual mechanics of how a loop is
-/// converted/rewritten without closures. Unlike JS, the value of a declared
-/// loop iteration variable in any closure is captured/snapshotted inside at
-/// each iteration point, as if we created a new local variable for that value
-/// inside the loop. For example, for the following loop:
-///
-///     var lst = [];
-///     for (int i = 0; i < 5; i++) lst.add(()=>i);
-///     var result = list.map((f) => f()).toList();
-///
-/// `result` will be [0, 1, 2, 3, 4], whereas were this JS code
-/// the result would be [5, 5, 5, 5, 5]. Because of this difference we need to
-/// create a closure for these sorts of loops to capture the variable's value at
-/// each iteration, by boxing the iteration variable[s].
-class LoopClosureRepresentationInfo extends ClosureAnalysisInfo {
-  const LoopClosureRepresentationInfo();
-
-  /// True if this loop declares any variables that need to be boxed.
-  bool get hasBoxedVariables => false;
-
-  /// The set of iteration variables (or variables declared in the for loop
-  /// expression (`for (...here...)`) that need to be boxed to snapshot their
-  /// value.
-  List<Local> get boxedVariables => const <Local>[];
 }
 
 /// Class that describes the actual mechanics of how the converted, rewritten
@@ -209,7 +190,11 @@ class ClosureRepresentationInfo extends ScopeInfo {
   /// Loop through each variable that has been boxed in this closure class. Only
   /// captured variables that are mutated need to be "boxed" (which basically
   /// puts a thin layer between updates and reads to this variable to ensure
-  /// that every place that accesses it gets the correct updated value).
+  /// that every place that accesses it gets the correct updated value). This
+  /// includes looping over variables that were boxed from other scopes, not
+  /// strictly variables defined in this closure, unlike the behavior in
+  /// the superclass ScopeInfo.
+  @override
   void forEachBoxedVariable(f(Local local, FieldEntity field)) {}
 
   /// Loop through each free variable in this closure. Free variables are the
@@ -245,9 +230,9 @@ class ClosureTask extends ClosureConversionTask<Node> {
     createClosureClasses(closedWorldRefiner);
   }
 
-  ClosureAnalysisInfo getClosureAnalysisInfo(Node node) {
+  ClosureBase getClosureBase(Node node) {
     var value = _closureInfoMap[node];
-    return value == null ? const ClosureAnalysisInfo() : value;
+    return value == null ? const ClosureBase() : value;
   }
 
   ScopeInfo getScopeInfo(Element member) {
@@ -258,10 +243,9 @@ class ClosureTask extends ClosureConversionTask<Node> {
     return getClosureToClassMapping(member);
   }
 
-  LoopClosureRepresentationInfo getClosureRepresentationInfoForLoop(
-      Node loopNode) {
+  ClosureBase getClosureRepresentationInfoForLoop(Node loopNode) {
     var value = _closureInfoMap[loopNode];
-    return value == null ? const LoopClosureRepresentationInfo() : value;
+    return value == null ? const ClosureBase() : value;
   }
 
   /// Returns the [ClosureClassMap] computed for [resolvedAst].
@@ -629,8 +613,7 @@ class SynthesizedCallMethodElementX extends BaseFunctionElementX
 
 // The box-element for a scope, and the captured variables that need to be
 // stored in the box.
-class ClosureScope
-    implements ClosureAnalysisInfo, LoopClosureRepresentationInfo {
+class ClosureScope implements ClosureBase {

[Siggi Cherem (dart-lang), 2017/06/26 16:54:42]

mmm... I sort of like the idea of swapping the two names: make ClosureScope the
public interface and ClosureBase the base implementation for
ClosureRepresentationInfo.

Thoughts?

   final BoxLocal boxElement;
   final Map<Local, BoxFieldElement> capturedVariables;
 
@@ -645,11 +628,29 @@ class ClosureScope
 
   bool get requiresContextBox => capturedVariables.keys.isNotEmpty;
 
-  List<Local> get boxedVariables => boxedLoopVariables;
+  void forEachBoxedVariable(f(Local local, FieldEntity field)) {
+    if (capturedVariables.isNotEmpty) {
+      capturedVariables.forEach(f);

[Johnni Winther, 2017/06/26 08:31:39]

assert(boxedLoopVariables.isEmpty)

+    } else {
+      for (Local l in boxedLoopVariables) {
+        // The boxes for loop variables are constructed on-demand per-iteration
+        // in the locals handler.
+        f(l, null);
+      }
+    }
+  }
 
-  bool get hasBoxedVariables => !boxedLoopVariables.isEmpty;
+  Map<Local, BoxFieldElement> get boxedVariables {

[Johnni Winther, 2017/06/26 08:31:39]

When is this used?

[Emily Fortuna, 2017/06/26 21:57:20]

oops. that was holdover from a previous edit. Removed!

+    Map<Local, BoxFieldElement> temp = <Local, BoxFieldElement>{};
+    boxedLoopVariables.forEach((Local l) {
+      temp[l] = null;
+    });
+    return capturedVariables.isNotEmpty ? capturedVariables : temp;
+  }
+
+  bool get hasBoxedVariables => !capturedVariables.isEmpty;
 
-  bool isCaptured(Local variable) {
+  bool isBoxed(Local variable) {
     return capturedVariables.containsKey(variable);
   }
 
@@ -658,6 +659,13 @@ class ClosureScope
     capturedVariables.forEach(f);
   }
 
+  // Should not be called. Added to make the new interface happy.
+  bool variableIsUsedInTryOrSync(Local variable) =>
+      throw new UnsupportedError("ClosureScope.variableIsUsedInTryOrSync");
+
+  // Should not be called. Added to make the new interface happy.
+  Local get thisLocal => throw new UnsupportedError("ClosureScope.thisLocal");
+
   String toString() {
     String separator = '';
     StringBuffer sb = new StringBuffer();
@@ -726,6 +734,9 @@ class ClosureClassMap implements ClosureRepresentationInfo {
   ClosureClassMap(this.closureEntity, this.closureClassEntity, this.callMethod,
       this.thisLocal);
 
+  bool get hasBoxedVariables =>
+      throw new UnsupportedError("ClosureClassMap.hasBoxedVariables");
+
   List<Local> get createdFieldEntities {
     List<Local> fields = <Local>[];
     if (closureClassEntity == null) return const <Local>[];
@@ -795,6 +806,14 @@ class ClosureClassMap implements ClosureRepresentationInfo {
       scope.forEachCapturedVariable(f);
     });
   }
+
+  bool isBoxed(Local local) {
+    bool variableIsBoxed = false;
+    forEachBoxedVariable((LocalVariableElement element, BoxFieldElement field) {
+      if (element == local) variableIsBoxed = true;
+    });
+    return variableIsBoxed;
+  }
 }
 
 class ClosureTranslator extends Visitor {