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

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.
 
 library closureToClassMapper;
 
 import 'common/names.dart' show Identifiers;
 import 'common/resolution.dart' show ParsingContext, Resolution;
 import 'common/tasks.dart' show CompilerTask;
 import 'common.dart';
 import 'compiler.dart' show Compiler;
 import 'constants/expressions.dart';
 import 'elements/resolution_types.dart';
 import 'elements/elements.dart';
 import 'elements/entities.dart';
 import 'elements/modelx.dart'
     show BaseFunctionElementX, ClassElementX, ElementX;
 import 'elements/visitor.dart' show ElementVisitor;
 import 'js_backend/js_backend.dart' show JavaScriptBackend;
 import 'resolution/tree_elements.dart' show TreeElements;
 import 'package:front_end/src/fasta/scanner.dart' show Token;
 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,
         super(compiler.measurer);
 
   String get name => "Closure Simplifier";
 
   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(() {
       Element element = resolvedAst.element;
       if (element.isGenerativeConstructorBody) {
         ConstructorBodyElement constructorBody = element;
         element = constructorBody.constructor;
       }
       ClosureClassMap closureClassMap = _closureMappingCache[element];
       assert(invariant(resolvedAst.element, closureClassMap != null,
@@ skipping 26 matching lines @@
       if (cached != null) return cached;
       if (resolvedAst.kind != ResolvedAstKind.PARSED) {
         return _closureMappingCache[element] =
             new ClosureClassMap(null, null, null, new ThisLocal(element));
       }
       return reporter.withCurrentElement(element.implementation, () {
         Node node = resolvedAst.node;
         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.
         if (node is FunctionExpression) {
           translator.translateFunction(element, node);
         } else if (element.isSynthesized) {
           reporter.internalError(
               element, "Unexpected synthesized element: $element");
           _closureMappingCache[element] =
               new ClosureClassMap(null, null, null, new ThisLocal(element));
@@ skipping 296 matching lines @@
   ResolvedAst get resolvedAst {
     return new ParsedResolvedAst(this, node, node.body, treeElements,
         expression.compilationUnit.script.resourceUri);
   }
 
   accept(ElementVisitor visitor, arg) {
     return visitor.visitMethodElement(this, arg);
   }
 }
 
+/// 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;
 
   // 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<VariableElement> boxedLoopVariables = const <VariableElement>[];
 
   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);
   }
 
   void forEachCapturedVariable(
       f(LocalVariableElement variable, BoxFieldElement boxField)) {
     capturedVariables.forEach(f);
   }
 
   String toString() {
     String separator = '';
@@ skipping 144 matching lines @@
   Set<LocalVariableElement> mutatedVariables = new Set<LocalVariableElement>();
 
   MemberElement outermostElement;
   ExecutableElement executableContext;
 
   // The closureData of the currentFunctionElement.
   ClosureClassMap closureData;
 
   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;
 
   /// Generate a unique name for the [id]th closure field, with proposed name
   /// [name].
   ///
   /// The result is used as the name of [ClosureFieldElement]s, and must
   /// therefore be unique to avoid breaking an invariant in the element model
   /// (classes cannot declare multiple fields with the same name).
   ///
@@ skipping 363 matching lines @@
 
     for (LocalVariableElement variable in scopeVariables) {
       // No need to box non-assignable elements.
       if (!variable.isAssignable) continue;
       if (!mutatedVariables.contains(variable)) continue;
       boxCapturedVariable(variable);
     }
     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;
     }
   }
 
   void inNewScope(Node node, Function action) {
     List<LocalVariableElement> oldScopeVariables = scopeVariables;
     scopeVariables = <LocalVariableElement>[];
     action();
     attachCapturedScopeVariables(node);
     mutatedVariables.removeAll(scopeVariables);
     scopeVariables = oldScopeVariables;
@@ skipping 246 matching lines @@
 ///
 /// Move the below classes to a JS model eventually.
 ///
 abstract class JSEntity implements Entity {
   Entity get declaredEntity;
 }
 
 abstract class PrivatelyNamedJSEntity implements JSEntity {
   Entity get rootOfScope;
 }