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

pkg/compiler/lib/src/closure.dart

 // Copyright (c) 2012, 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 'common/names.dart' show Identifiers;
 import 'common/resolution.dart' show ParsingContext, Resolution;
 import 'common/tasks.dart' show CompilerTask, Measurer;
 import 'common.dart';
 import 'compiler.dart' show Compiler;
 import 'constants/expressions.dart';
@@ skipping 34 matching lines @@
   // one for local functions.
   ScopeInfo getScopeInfo(covariant Entity member);
 
   /// This returns the same information as ScopeInfo, but can be called in
   /// situations when you are sure you are dealing with a closure specifically.
   ClosureRepresentationInfo getClosureRepresentationInfo(
       covariant Entity member);
 
   /// 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
 /// is a closure or not. This is specifically used to store information
 /// about the usage of variables in try or sync blocks, because they need to be
 /// boxed.
 ///
 /// Variables that are used in a try must be treated as boxed because the
 /// control flow can be non-linear. Also parameters to a `sync*` generator must
 /// be boxed, because of the way we rewrite sync* functions. See also comments
 /// in [ClosureClassMap.useLocal].
 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).
   ///
   /// Variables that are used in a try must be treated as boxed because the
   /// control flow can be non-linear.
   ///
   /// Also parameters to a `sync*` generator must be boxed, because of the way
   /// we rewrite sync* functions. See also comments in
   /// [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
   /// variable has been "captured"). In this situation, access to this variable
   /// is controlled via a wrapper (box) so that updates to this variable
   /// are done in a way that is in line with Dart's closure rules.
   bool get requiresContextBox => false;
 
   /// Accessor to the local environment in which a particular closure node is
   /// executed. This will encapsulate the value of any variables that have been
   /// 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
 /// closure is implemented. For example, for the following closure (named foo
 /// for convenience):
 ///
 ///   var foo = (x) => y + x;
 ///
 /// We would produce the following class to control access to these variables in
 /// the following way (modulo naming of variables, assuming that y is modified
@@ skipping 48 matching lines @@
   /// 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. These nested scopes hold "boxes" to hold the executable
   /// context for that scope.
   void forEachCapturedVariable(f(Local from, FieldEntity to)) {}
 
   /// 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
   /// variables that have been captured *just* in this closure, not in nested
   /// scopes.
   void forEachFreeVariable(f(Local variable, FieldEntity field)) {}
 
   /// Return true if [variable] has been captured and mutated (all other
   /// variables do not require boxing).
   bool isVariableBoxed(Local variable) => false;
@@ skipping 15 matching lines @@
 
   String get name => "Closure Simplifier";
 
   DiagnosticReporter get reporter => compiler.reporter;
 
   void convertClosures(Iterable<MemberEntity> processedEntities,
       ClosedWorldRefiner closedWorldRefiner) {
     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) {
     return getClosureToClassMapping(member);
   }
 
   ClosureRepresentationInfo getClosureRepresentationInfo(Element member) {
     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].
   ClosureClassMap getClosureToClassMapping(Element element) {
     return measure(() {
       if (element.isGenerativeConstructorBody) {
         ConstructorBodyElement constructorBody = element;
         element = constructorBody.constructor;
       }
       ClosureClassMap closureClassMap = _closureMappingCache[element];
@@ skipping 347 matching lines @@
         expression.compilationUnit.script.resourceUri);
   }
 
   accept(ElementVisitor visitor, arg) {
     return visitor.visitMethodElement(this, arg);
   }
 }
 
 // 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;
 
   // If the scope is attached to a [For] contains the variables that are
   // declared in the initializer of the [For] and that need to be boxed.
   // Otherwise contains the empty List.
   List<Local> boxedLoopVariables = const <Local>[];
 
   ClosureScope(this.boxElement, this.capturedVariables);
 
   Local get context => boxElement;
 
   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 isCaptured(Local variable) {
+  bool get hasBoxedVariables => !capturedVariables.isEmpty;
+
+  bool isBoxed(Local variable) {
     return capturedVariables.containsKey(variable);
   }
 
   void forEachCapturedVariable(
       f(LocalVariableElement variable, BoxFieldElement boxField)) {
     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();
     sb.write('ClosureScope(');
     if (boxElement != null) {
       sb.write('box=$boxElement');
       separator = ',';
     }
     if (boxedLoopVariables.isNotEmpty) {
       sb.write(separator);
@@ skipping 48 matching lines @@
   /// control flow can be non-linear.
   ///
   /// Also parameters to a `sync*` generator must be boxed, because of the way
   /// we rewrite sync* functions. See also comments in [useLocal].
   // TODO(johnniwinther): Add variables to this only if the variable is mutated.
   final Set<Local> variablesUsedInTryOrSync = new Set<Local>();
 
   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>[];
     closureClassEntity.closureFields.forEach((field) {
       fields.add(field.local);
     });
     return fields;
   }
 
   void addFreeVariable(Local element) {
@@ skipping 49 matching lines @@
   void forEachBoxedVariable(
       void f(LocalVariableElement local, BoxFieldElement field)) {
     freeVariableMap.forEach((variable, copy) {
       if (!isVariableBoxed(variable)) return;
       f(variable, copy);
     });
     capturingScopes.values.forEach((ClosureScope scope) {
       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 {
   final Compiler compiler;
   final ClosedWorldRefiner closedWorldRefiner;
   final TreeElements elements;
   int closureFieldCounter = 0;
   int boxedFieldCounter = 0;
   bool inTryStatement = false;
 
@@ skipping 685 matching lines @@
 ///
 /// Move the below classes to a JS model eventually.
 ///
 abstract class JSEntity implements MemberEntity {
   Local get declaredEntity;
 }
 
 abstract class PrivatelyNamedJSEntity implements JSEntity {
   Entity get rootOfScope;
 }