Create new interface instead of ClosureClassMap for variable usage information that is not Element-…

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 f7a0af62c9a1992e5d94f3c2630e72c066970a9e..9075e92c861527f3cb874b52f1dbe167a76bfe1f 100644
--- a/pkg/compiler/lib/src/closure.dart
+++ b/pkg/compiler/lib/src/closure.dart
@@ -23,14 +23,47 @@ import 'tree/tree.dart';
 import 'util/util.dart';
 import 'world.dart' show ClosedWorldRefiner;
 
-abstract class ClosureClassMaps {
-  ClosureClassMap getMemberMap(MemberEntity member);
-  ClosureClassMap getLocalFunctionMap(Local localFunction);
+/// Where T is ir.Node or Node.
+// TODO(efortuna): Rename this class.
+abstract class ClosureClassMaps<T> {

[Johnni Winther, 2017/05/31 12:39:51]

I'd prefer if these new methods (eventually) are on a per member
`ClosureClassMap<T>` so that we can purge each ClosureClassMap when we have
built the corresponding ssa graph.

This would also eliminate the need for passing the current local function to
[forEachClosureClassFieldEntity] and [getLocalVarForClosureField].

+  /// Currently, closures are rewritten in the form of classes that

[Siggi Cherem (dart-lang), 2017/05/31 20:44:38]

Small suggestion: to make the docs a bit more complete, let's add one sentence
to this and the other docs that describes the function API. I generally assume
you have the context of what this algorithm does on that sentence, but include
extra paragraphs (like the one you have here) to clarify further. 

It would also be helpful to mention what is [member] expected to be (since so
many things can be Local) or maybe rename it to be more explicit. Is it always a
[localFunction] or an actual method?

For example:

/// Fetch the class used to represent a [localFunction].
///
/// Closures are rewritten in the form of classes ...
///

+  /// have fields to control the redirection and editing of variables that are
+  /// "captured" inside a scope (declared in an outer scope but used in an
+  /// inside scope).
+  ClassEntity getClosureClassEntity(Local member);

[Siggi Cherem (dart-lang), 2017/05/31 20:44:39]

minor naming ideas: these are totally optional, so feel free to submit and later
we can revisit them.

For background: a style we have used a lot in dart2js is to use names like
`xForSomething` instead of `getX`. 

My suggestion for these APIs would be:

 - closureClassFor(...)  // I'd drop Entity from the method name too
 - forEachCapturedVariable(...)  // your idea :)
 - localVariableForClosureField(...)
 - callFunctionFor(...)   // Function instead of Entity
 - executableContextFor(T)   
 - capturedVariableInfoFor(T) 

+
+  // TODO(efortuna): This should probably be renamed to something like
+  // forEachCapturedVariable or something that's less implementation specific.

[Siggi Cherem (dart-lang), 2017/05/31 20:44:39]

+1 I like that, also possible `forEachCapturedVariableIn` (`in` refers to the
[entity])

+  void forEachClosureClassFieldEntity(Local entity, void f(FieldEntity field));
+
+  /// Closures are rewritten as classes with fields representing the local

[Siggi Cherem (dart-lang), 2017/05/31 20:44:39]

Another though - we can explore moving the docs up to the class level to avoid
repeating information, and then just refer to it on all these methods.

Then this can be a one-liner like:
/// Returns the original captured variable in [member] that is represented by
[field].

+  /// variables that the closure has "captured. Return the actual local

[Johnni Winther, 2017/05/31 12:39:51]

"captured -> captured

+  /// variable that a particular closure field is emulating.
+  Local getLocalVarForClosureField(Local member, FieldEntity field);
+
+  /// The method representing calling the closure to execute, provided the

[Johnni Winther, 2017/05/31 12:39:51]

Maybe rephrase as:

The function that implements the [local] function as a `call` method on the
closure class.

(and rename [member] to [local])

+  /// original local entity.
+  FunctionEntity getCallEntity(Local member);
+
+  /// 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.

[Siggi Cherem (dart-lang), 2017/05/31 20:44:39]

is "scoped" the same as captured here? or something else?

+  Local getExecutableContext(T node);

[Siggi Cherem (dart-lang), 2017/05/31 20:44:38]

Open question: I wonder if we should introduce this name now or a bit later
(since we use still use 'box' elsewhere in the compiler, we could wait until we
are ready to fix all other uses.)

Johnni - when we talked offline we realize why "Box" is confusing, is that we
use it to mean two things: the place where we wrap a captured variable (what we
normally think as a box) and the argument that collects many of those boxed
variables to pass to the constructor (which we call the box element).

+
+  // TODO(efortuna): Finish removing this method by exposing remining info in

[Siggi Cherem (dart-lang), 2017/05/31 20:44:39]

remining => remaining

+  // this interface.
+  ClosureClassMap getMemberMap(MemberElement member);
+
+  /// Look up information about what variables are have been mutated that are

[Johnni Winther, 2017/05/31 12:39:51]

are have ... inside the -> have been mutated inside the

+  /// used inside the scope of [node].
+  CapturedVariableInfo getCapturedVariableInfo(T node);
 }
 
-class ClosureTask extends CompilerTask implements ClosureClassMaps {
+class ClosureTask extends CompilerTask implements ClosureClassMaps<Node> {
   Map<Element, ClosureClassMap> _closureMappingCache =
       <Element, ClosureClassMap>{};
+  Map<Node, Local> _executableContextCache = <Node, Local>{};
+  Map<Node, ClosureScope> _closureScopeCache = <Node, ClosureScope>{};
   Compiler compiler;
   ClosureTask(Compiler compiler)
       : compiler = compiler,
@@ -40,14 +73,52 @@ class ClosureTask extends CompilerTask implements ClosureClassMaps {
 
   DiagnosticReporter get reporter => compiler.reporter;
 
+  CapturedVariableInfo getCapturedVariableInfo(Node node) {
+    ClosureScope scopeData = _closureScopeCache[node];
+    if (scopeData == null) return new CapturedVariableInfo();

[Siggi Cherem (dart-lang), 2017/06/01 15:40:30]

consider adding a const constructor to CapturedVariableInfo, that way we can
reuse the same instance here.

+    return scopeData;
+  }
+
   ClosureClassMap getMemberMap(MemberElement member) {
     return getClosureToClassMapping(member.resolvedAst);
   }
 
-  ClosureClassMap getLocalFunctionMap(LocalFunctionElement localFunction) {
+  ClosureClassMap _getLocalFunctionMap(LocalFunctionElement localFunction) {
     return getClosureToClassMapping(localFunction.resolvedAst);
   }
 
+  void forEachClosureClassFieldEntity(
+      LocalFunctionElement entity, void f(FieldEntity field)) {
+    ClosureClassElement classEntity = getClosureClassEntity(entity);
+    classEntity.closureFields.forEach(f);
+  }
+
+  Local getExecutableContext(Node node) {
+    return _executableContextCache[node];
+  }
+
+  Local getLocalVarForClosureField(
+      LocalFunctionElement member, FieldEntity field) {
+    ClosureClassMap memberMap = _getLocalFunctionMap(member);
+    assert(memberMap != null);
+    assert(memberMap.closureClassElement != null);
+    return memberMap.getLocalVariableForClosureField(field);
+  }
+
+  ClassEntity getClosureClassEntity(LocalFunctionElement member) {
+    ClosureClassMap memberMap = _getLocalFunctionMap(member);
+    assert(memberMap != null);
+    assert(memberMap.closureClassElement != null);
+    return memberMap.closureClassElement;
+  }
+
+  FunctionEntity getCallEntity(LocalFunctionElement member) {
+    ClosureClassMap memberMap = _getLocalFunctionMap(member);
+    assert(memberMap != null);
+    assert(memberMap.closureClassElement != null);
+    return memberMap.callElement;
+  }
+
   /// Returns the [ClosureClassMap] computed for [resolvedAst].
   ClosureClassMap getClosureToClassMapping(ResolvedAst resolvedAst) {
     return measure(() {
@@ -94,7 +165,12 @@ class ClosureTask extends CompilerTask implements ClosureClassMaps {
         TreeElements elements = resolvedAst.elements;
 
         ClosureTranslator translator = new ClosureTranslator(
-            compiler, closedWorldRefiner, elements, _closureMappingCache);
+            compiler,
+            closedWorldRefiner,
+            elements,
+            _closureMappingCache,
+            _executableContextCache,
+            _closureScopeCache);
 
         // The translator will store the computed closure-mappings inside the
         // cache. One for given node and one for each nested closure.
@@ -411,9 +487,21 @@ class SynthesizedCallMethodElementX extends BaseFunctionElementX
   }
 }
 
+/// Interface external classes can use to query information about what variables
+/// are mutated inside a scope.
+class CapturedVariableInfo {
+  bool hasCapturedVariables() => false;

[Siggi Cherem (dart-lang), 2017/05/31 20:44:39]

nit: turn these two into getters:

bool get hasCapturedVariables => false;
bool get hasBoxedLoopVariables => false;

+  bool hasBoxedLoopVariables() => false;
+
+  /// True if the specified variable has been mutated inside the scope of this
+  /// closure.
+  bool isCaptured(Local variable) => false;
+  void forEachCapturedVariable(f(Entity from, Entity to)) {}
+}
+
 // The box-element for a scope, and the captured variables that need to be
 // stored in the box.
-class ClosureScope {
+class ClosureScope implements CapturedVariableInfo {
   final BoxLocal boxElement;
   final Map<Local, BoxFieldElement> capturedVariables;
 
@@ -424,9 +512,11 @@ class ClosureScope {
 
   ClosureScope(this.boxElement, this.capturedVariables);
 
+  bool hasCapturedVariables() => capturedVariables.keys.isNotEmpty;
+
   bool hasBoxedLoopVariables() => !boxedLoopVariables.isEmpty;
 
-  bool isCapturedVariable(VariableElement variable) {
+  bool isCaptured(VariableElement variable) {

[Johnni Winther, 2017/05/31 12:39:51]

[VariableElement] -> [LocalVariableElement] to make a valid override.

     return capturedVariables.containsKey(variable);
   }
 
@@ -591,8 +681,16 @@ class ClosureTranslator extends Visitor {
 
   bool insideClosure = false;
 
-  ClosureTranslator(this.compiler, this.closedWorldRefiner, this.elements,
-      this.closureMappingCache);
+  Map<Node, Local> executableContextCache;
+  Map<Node, ClosureScopeCache> closureScopeCache;
+
+  ClosureTranslator(
+      this.compiler,
+      this.closedWorldRefiner,
+      this.elements,
+      this.closureMappingCache,
+      this.executableContextCache,
+      this.closureScopeCache);
 
   DiagnosticReporter get reporter => compiler.reporter;
 
@@ -976,6 +1074,10 @@ class ClosureTranslator extends Visitor {
     if (!scopeMapping.isEmpty) {
       ClosureScope scope = new ClosureScope(box, scopeMapping);
       closureData.capturingScopes[node] = scope;
+      assert(executableContextCache[node] == null);
+      executableContextCache[node] = box;
+      assert(closureScopeCache[node] == null);
+      closureScopeCache[node] = scope;
     }
   }