Add ClosureRepresentationInfo, the new public face of ClosureClassMap

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 1b0cb8bd625c69a2a3f10ded6f11bcdd22d00176..53dc89cf027fa73d272dee7d433cf9a79e64d814 100644
--- a/pkg/compiler/lib/src/closure.dart
+++ b/pkg/compiler/lib/src/closure.dart
@@ -26,9 +26,13 @@ import 'util/util.dart';
 import 'world.dart' show ClosedWorldRefiner;
 
 /// Where T is ir.Node or Node.
+// TODO(efortuna): Rename this class.
 abstract class ClosureClassMaps<T> {
-  ClosureClassMap getMemberMap(MemberEntity member);
-  ClosureClassMap getLocalFunctionMap(Local localFunction);
+  /// Look up information about the variables that have been mutated and are
+  /// used inside the scope of [node].
+  // TODO(johnniwinther): Split this up into two functions, one for members and
+  // one for local functions.
+  ClosureRepresentationInfo getClosureRepresentationInfo(Entity member);
 
   /// Look up information about a loop, in case any variables it declares need
   /// to be boxed/snapshotted.
@@ -76,8 +80,9 @@ class ClosureAnalysisInfo {
 ///
 ///     var lst = [];
 ///     for (int i = 0; i < 5; i++) lst.add(()=>i);
+///     var result = list.map((f) => f()).toList();
 ///
-/// The result of `lst` will be [0, 1, 2, 3, 4], whereas were this JS code
+/// `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].
@@ -93,6 +98,102 @@ class LoopClosureRepresentationInfo extends ClosureAnalysisInfo {
   List<Local> get boxedVariables => const <Local>[];
 }
 
+/// Class that describes the actual mechanics of how the converted, rewritten

[Siggi Cherem (dart-lang), 2017/06/14 22:56:36]

brainstorming here: in a way I think "convertion/rewriting" is the
implementation :), so I wonder about what noun we should use to refer to the
rewritten closure. I'm not opposed to keep it as "rewritten closure " like you
have it, just thinking of ideas. For example, other ideas:
- emitter representation of Dart closures
- JavaScript representation of a Dart closure
- closure's implementation representation.

If we change the terminology, I'd consider updating the docs, for example, this
sentence here could be:

Class that describes the actual mechanics of how closures are implemented. This
includes information about how the closure is represented in the emitted
JavaScript in terms of classes and fields. ....

[Emily Fortuna, 2017/06/15 00:11:22]

"Closure Conversion" is the standard term used to describe the process, so I'm
inclined to keep it. Can discuss more, tho.

+/// 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
+/// elsewhere in its scope):
+///
+///    class FooClosure {
+///       int y;
+///       FooClosure(this.y);
+///       call(x) => this.y + x;
+///    }
+///
+///  and then to execute this closure, for example:
+///
+///     var foo = new FooClosure(1);
+///     foo.call(2);
+///
+/// if y is modified elsewhere within its scope, accesses to y anywhere in the
+/// code will be controlled via a box object.
+/// TODO(efortuna): Make interface simpler in subsequent refactorings.
+class ClosureRepresentationInfo {
+  const ClosureRepresentationInfo();
+
+  /// The original local function before any translation.
+  ///
+  /// Will be null for methods.
+  Local get closureEntity => null;
+
+  /// Closures are rewritten in the form of classes that have fields to control

[Siggi Cherem (dart-lang), 2017/06/14 22:56:36]

minor nit: consider moving up the sentence that just describes what this
property is (since there is context already in the class comment), and then add
more details. For example:

/// The entity for the class used to represent the closure in the emitted
JavaScript.
///
/// Closures are represented as a class, where each field represents a captured
variable.
/// **A capture variable is a variable declared in an scope outside the closure
and used in an 
/// scope within the closure.

** - I'd probably not include the definition of captured varaibles here, but
move it to the class comment and other methods below that use the term.

[Emily Fortuna, 2017/06/15 00:11:22]

Cool. I updated the docs in this CL: https://codereview.chromium.org/2937143002

+  /// the redirection and editing of variables that are "captured" inside a
+  /// scope (declared in an outer scope but used in an inside scope). So this
+  /// returns the class entity that represents this particular rewritten
+  /// closure.
+  ClassEntity get closureClassEntity => null;
+
+  /// The function that implements the [local] function as a `call` method on
+  /// the closure class.
+  FunctionEntity get callMethod => null;
+
+  /// As shown in the example in the comments at the top of this class, we
+  /// create fields in the closure class for each captured variable. This is an
+  /// accessor to that set of fields.
+  List<Local> get createdFieldEntities => const <Local>[];
+
+  /// Convenience reference pointer to the element representing `this`.
+  /// It is only set for instance-members.
+  Local get thisLocal => null;
+
+  /// Convenience pointer to the field entity representation in the closure
+  /// class of the element representing `this`.
+  FieldEntity get thisFieldEntity => 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;
+
+  /// 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).
+  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;
+
+  // TODO(efortuna): Remove this method. The old system was using
+  // ClosureClassMaps for situations other than closure class maps, and that's
+  // just confusing.
+  bool get isClosure => false;
+}
+
 class ClosureTask extends CompilerTask implements ClosureClassMaps<Node> {
   Map<Node, ClosureScope> _closureInfoMap = <Node, ClosureScope>{};
   Map<Element, ClosureClassMap> _closureMappingCache =
@@ -111,20 +212,16 @@ class ClosureTask extends CompilerTask implements ClosureClassMaps<Node> {
     return value == null ? const ClosureAnalysisInfo() : value;
   }
 
+  ClosureRepresentationInfo getClosureRepresentationInfo(Element member) {
+    return getClosureToClassMapping(member);
+  }
+
   LoopClosureRepresentationInfo getClosureRepresentationInfoForLoop(
       Node loopNode) {
     var value = _closureInfoMap[loopNode];
     return value == null ? const LoopClosureRepresentationInfo() : value;
   }
 
-  ClosureClassMap getMemberMap(MemberElement member) {
-    return getClosureToClassMapping(member);
-  }
-
-  ClosureClassMap getLocalFunctionMap(LocalFunctionElement localFunction) {
-    return getClosureToClassMapping(localFunction);
-  }
-
   /// Returns the [ClosureClassMap] computed for [resolvedAst].
   ClosureClassMap getClosureToClassMapping(Element element) {
     return measure(() {
@@ -537,24 +634,24 @@ class ClosureScope
   }
 }
 
-class ClosureClassMap {
+class ClosureClassMap implements ClosureRepresentationInfo {
   /// The local function element before any translation.
   ///
   /// Will be null for methods.
-  final LocalFunctionElement closureElement;
+  final LocalFunctionElement closureEntity;
 
-  /// The synthesized closure class for [closureElement].
+  /// The synthesized closure class for [closureEntity].
   ///
-  /// The closureClassElement will be null for methods that are not local
+  /// The closureClassEntity will be null for methods that are not local
   /// closures.
-  final ClosureClassElement closureClassElement;
+  final ClosureClassElement closureClassEntity;
 
-  /// The synthesized `call` method of the [ closureClassElement].
+  /// The synthesized `call` method of the [closureClassEntity].
   ///
-  /// The callElement will be null for methods that are not local closures.
-  final MethodElement callElement;
+  /// The callMethod will be null for methods that are not local closures.
+  final MethodElement callMethod;
 
-  /// The [thisElement] makes handling 'this' easier by treating it like any
+  /// The [thisLocal] makes handling 'this' easier by treating it like any
   /// other argument. It is only set for instance-members.
   final ThisLocal thisLocal;
 
@@ -576,8 +673,17 @@ class ClosureClassMap {
   // TODO(johnniwinther): Add variables to this only if the variable is mutated.
   final Set<Local> variablesUsedInTryOrGenerator = new Set<Local>();
 
-  ClosureClassMap(this.closureElement, this.closureClassElement,
-      this.callElement, this.thisLocal);
+  ClosureClassMap(this.closureEntity, this.closureClassEntity, this.callMethod,
+      this.thisLocal);
+
+  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) {
     assert(freeVariableMap[element] == null);
@@ -594,14 +700,16 @@ class ClosureClassMap {
     freeVariableMap.forEach(f);
   }
 
-  bool isVariableUsedInTryOrSync(Local variable) =>
+  FieldEntity get thisFieldEntity => freeVariableMap[thisLocal];
+
+  bool variableIsUsedInTryOrSync(Local variable) =>
       variablesUsedInTryOrGenerator.contains(variable);
 
   Local getLocalVariableForClosureField(ClosureFieldElement field) {
     return field.local;
   }
 
-  bool get isClosure => closureElement != null;
+  bool get isClosure => closureEntity != null;
 
   bool capturingScopesBox(Local variable) {
     return capturingScopes.values.any((scope) {
@@ -778,7 +886,7 @@ class ClosureTranslator extends Visitor {
           boxes.add(boxFieldElement.box);
         }
       });
-      ClosureClassElement closureClass = data.closureClassElement;
+      ClosureClassElement closureClass = data.closureClassEntity;
       assert(closureClass != null || (fieldCaptures.isEmpty && boxes.isEmpty));
 
       void addClosureField(Local local, String name) {
@@ -834,7 +942,7 @@ class ClosureTranslator extends Visitor {
       // things in the builder.
       // Note that nested (named) functions are immutable.
       if (variable != closureData.thisLocal &&
-          variable != closureData.closureElement &&
+          variable != closureData.closureEntity &&
           variable is! TypeVariableLocal) {
         closureData.variablesUsedInTryOrGenerator.add(variable);
       }
@@ -1221,8 +1329,8 @@ class ClosureTranslator extends Visitor {
     }
     closureMappingCache[element] = closureData;
     closureMappingCache[element.declaration] = closureData;
-    if (closureData.callElement != null) {
-      closureMappingCache[closureData.callElement] = closureData;
+    if (closureData.callMethod != null) {
+      closureMappingCache[closureData.callMethod] = closureData;
     }
 
     inNewScope(node, () {