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

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

BUG=

[Emily Fortuna, 2017-05-31 05:56:15]

Patchset #2 (id:20001) has been deleted

[Emily Fortuna, 2017-05-31 06:05:19]

Patchset #2 (id:40001) has been deleted

[Emily Fortuna, 2017-05-31 06:05:58]

efortuna@google.com changed reviewers:
+ johnniwinther@google.com, sigmund@google.com

[Emily Fortuna, 2017-05-31 06:05:58]

Definitely more to do. These interfaces are very much a work in progress. :-/

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

lgtm

https://codereview.chromium.org/2915523003/diff/60001/pkg/compiler/lib/src/cl...
File pkg/compiler/lib/src/closure.dart (right):

https://codereview.chromium.org/2915523003/diff/60001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:28: abstract class ClosureClassMaps<T> {
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].

https://codereview.chromium.org/2915523003/diff/60001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:40: /// variables that the closure has
"captured. Return the actual local
"captured -> captured

https://codereview.chromium.org/2915523003/diff/60001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:44: /// The method representing calling the
closure to execute, provided the
Maybe rephrase as:

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

(and rename [member] to [local])

https://codereview.chromium.org/2915523003/diff/60001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:57: /// Look up information about what
variables are have been mutated that are
are have ... inside the -> have been mutated inside the

https://codereview.chromium.org/2915523003/diff/60001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:519: bool isCaptured(VariableElement variable)
{
[VariableElement] -> [LocalVariableElement] to make a valid override.

https://codereview.chromium.org/2915523003/diff/60001/pkg/compiler/lib/src/du...
File pkg/compiler/lib/src/dump_info.dart (right):

https://codereview.chromium.org/2915523003/diff/60001/pkg/compiler/lib/src/du...
pkg/compiler/lib/src/dump_info.dart:218:
.getCallEntity(element.methodElement.resolvedAst));
`element.methodElement.resolvedAst` -> `element.methodElement`  (getCallEntity
takes a LocalFunctionElement)

https://codereview.chromium.org/2915523003/diff/60001/pkg/compiler/lib/src/ke...
File pkg/compiler/lib/src/kernel/kernel_backend_strategy.dart (right):

https://codereview.chromium.org/2915523003/diff/60001/pkg/compiler/lib/src/ke...
pkg/compiler/lib/src/kernel/kernel_backend_strategy.dart:220: ClassEntity
getClosureClassEntity(Local member);
Add a `=> null;` body to avoid warnings.

https://codereview.chromium.org/2915523003/diff/60001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2915523003/diff/60001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:333:
.getCapturedVariableInfo(constructorElement.resolvedAst.node);
The uses of `constructorElement.resolvedAst.node` needs to go away. We can avoid
it by using a per member ClosureClassMap<T>.

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

We talked offline so, the comments below are likely repetitive :)

While on the whiteboard we thought of a couple ideas, I think Johnni would
approve too :)

- do go back to have an intermediate object that collects everything about a
method
- possibly split that in two sub objects.

Some name ideas we had:

- ClosureConverter => the main class that you query for individual closure data

- ClosureAnalysisData => lets you query the captureVariableInfo for loops and
closures

- ClosureRepresentationData => provides mapping to entities that are used to
reify closures as classes (or in the future as a different representation if we
wish).

https://codereview.chromium.org/2915523003/diff/60001/pkg/compiler/lib/src/cl...
File pkg/compiler/lib/src/closure.dart (right):

https://codereview.chromium.org/2915523003/diff/60001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:29: /// Currently, closures are rewritten in
the form of classes that
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 ...
///

https://codereview.chromium.org/2915523003/diff/60001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:33: ClassEntity getClosureClassEntity(Local
member);
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)

https://codereview.chromium.org/2915523003/diff/60001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:36: // forEachCapturedVariable or something
that's less implementation specific.
+1 I like that, also possible `forEachCapturedVariableIn` (`in` refers to the
[entity])

https://codereview.chromium.org/2915523003/diff/60001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:39: /// Closures are rewritten as classes with
fields representing the local
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].

https://codereview.chromium.org/2915523003/diff/60001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:50: /// scoped into this context from outside.
is "scoped" the same as captured here? or something else?

https://codereview.chromium.org/2915523003/diff/60001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:51: Local getExecutableContext(T node);
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).

https://codereview.chromium.org/2915523003/diff/60001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:53: // TODO(efortuna): Finish removing this
method by exposing remining info in
remining => remaining

https://codereview.chromium.org/2915523003/diff/60001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:493: bool hasCapturedVariables() => false;
nit: turn these two into getters:

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

https://codereview.chromium.org/2915523003/diff/60001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/builder_kernel.dart (right):

https://codereview.chromium.org/2915523003/diff/60001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/builder_kernel.dart:333:
.getCapturedVariableInfo(constructorElement.resolvedAst.node);
On 2017/05/31 12:39:51, Johnni Winther wrote:
> The uses of `constructorElement.resolvedAst.node` needs to go away. We can
avoid
> it by using a per member ClosureClassMap<T>.

Another idea:
I noticed that we have 2 kinds of calls to getCapturedVariableInfo:
 - when the key node is a loop
 - when the key node is a function

What if we have 2 APIs one for each:

  CapturedVariableInfo capturedVariableInfoForFunction(
      FunctionEntity entity);

  CapturedVariableInfo capturedVariableInfoForLoop(T node);

Then here, the parameter will be constructorElement and we can drop
.resolvedAst.node

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

just realized I have one more comment below that I had drafted on my laptop and
haven't save it yet :)

https://codereview.chromium.org/2915523003/diff/60001/pkg/compiler/lib/src/cl...
File pkg/compiler/lib/src/closure.dart (right):

https://codereview.chromium.org/2915523003/diff/60001/pkg/compiler/lib/src/cl...
pkg/compiler/lib/src/closure.dart:78: if (scopeData == null) return new
CapturedVariableInfo();
consider adding a const constructor to CapturedVariableInfo, that way we can
reuse the same instance here.