Added for-loop variable tracking and regular closures/initializers captured variable tracking.

Added for-loop variable tracking and regular closures/initializers captured variable tracking.


This whole thing is hanging together by a thread. A BUNCH of tests and more implementation coming next once the KernelClosureClass is truly functional.

BUG=
R=sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/d40f84fbf8fbe13ab37a62959ec20a924bd82d88

[Emily Fortuna, 2017-06-30 17:55:29]

Patchset #1 (id:1) has been deleted

[Emily Fortuna, 2017-06-30 17:58:39]

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

[Emily Fortuna, 2017-06-30 17:58:40]

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_model/closure.dart (right):

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure.dart:265: bool get isClosure => false;
what's this? you may ask. Well, to properly call ourselves a closure, we need to
register the new closure class with the ClosedWorldRefiner, which currently only
takes elements. The change to that (and the subsequent adjustment here) will
follow soon in a subsequent CL.

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

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/ke...
pkg/compiler/lib/src/kernel/kernel_backend_strategy.dart:81: _compiler.measurer,
elementMap, null, _globalLocalsMap);
This seems... super sketchy. Johnni, I don't understand the distinction between
this strategy and the js_strategy? I mean, I understand conceptually, but is
this particular strategy actually being exercised anywhere? It just seemed less
implemented.

[Siggi Cherem (dart-lang), 2017-06-30 22:02:10]

looking nice! Just a few comments below

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_model/closure.dart (right):

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure.dart:12: import
'../js_model/elements.dart';
I might be misreading it, but isn't this also under `js_model` (remove the
../js_model?)

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure.dart:36: Map<Entity,
ClosureRepresentationInfo> _closureRepresentationCache =
nit: Cache => Map (to be consistent)?

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure.dart:41: final JsToFrontendMap
_kToJElementMap;
Let's add a TODO to eliminate this mapping.

Johnni - Emily and I talked offine and we realized this is related to the
distinction with KernelBackendStrategyImpl and JsBackendStrategy. Is the idea
that we will have JsBackendStrategy only in the future? It seems we want to pass
here the kElementMap (KernelToElementMapForBuilding) and not the jElementMap,
correct?

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure.dart:57: Map<ir.Node, ScopeInfo>
flaggedClosures = <ir.Node, ScopeInfo>{};
Seems we have 3 names for this :) - flaggedClosures, closuresToRewrite, and
closuresToGenerate. Maybe rename them all to closuresToGenerate?

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure.dart:57: Map<ir.Node, ScopeInfo>
flaggedClosures = <ir.Node, ScopeInfo>{};
nit: I'd use the type only once (either on the left or right), in strong-mode
inference will fill the other one.

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure.dart:86: visitor.ClosureScopeBuilder
translator = new visitor.ClosureScopeBuilder(
nit: I'd probably drop the prefix, maybe use a `show` in the import if you want
to highlight where this type is defined.

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure.dart:140: return const ClosureScope();
nit: you can also write the entire body as:

ClosureScope getClosureScope(ir.Node node) =>
  _closureScopeMap[node] ?? const ClosureScope();

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure.dart:147: // TODO(efortuna): Eventually
scope should always be non-null, and
is it not null and not KernelLoopClosureScope because of the default value in
`getClosureScope` above?

Maybe change to directly call _closureScopeMap instead?

  => _closureScopeMap[loopNode] ?? const LoopClosureScope();

?

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure.dart:156: var closureImpl =
_closureRepresentationCache[entity];
same compact version here

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure.dart:184: Set<Local> get
variablesUsedInTryOrSync => _localsUsedInTryOrSync;
since you are not updating the private field, I think we can get rid of that
field and make this getter a final field instead:

final Set<Local> variablesUsedInTryOrSync;
KernelScopeInfo(this.thisLocal)
  : variablesUsedInTryOrSync = new Set<Local>();

KernelScopeInfo.from(this.thisLocal, info)
  : variablesUsedInTryOrSync = info.variablesUsedInSync;

Also - if we don't intend to update the `boxedVariables` field, let's make it
final as well.

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure.dart:265: bool get isClosure => false;
On 2017/06/30 17:58:40, Emily Fortuna wrote:
> what's this? you may ask. Well, to properly call ourselves a closure, we need
to
> register the new closure class with the ClosedWorldRefiner, which currently
only
> takes elements. The change to that (and the subsequent adjustment here) will
> follow soon in a subsequent CL.

I'm ok copying this comment into the code if you like :)

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_model/closure_visitors.dart (right):

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:16: /// A map of each
visited call node with the associated information about what
I am not sure what the keys of the map are, maybe you can clarify in the
comment? It appears like you have a key for every scope in the program?

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:20: /// A map of the
elements corresponding to nodes that we have flagged as
now that you are using ir.Node as keys, maybe remove "elements corresponding to
..."? For instance:

  /// Maps nodes that require the compiler to create a closure class.

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:22: /// node to information
ascertained about variable usage in the surrounding
I had to look it up in the dictionary :)

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:24: Map<ir.Node, ScopeInfo>
_closuresToGenerate = <ir.Node, ScopeInfo>{};
to better document what kind of nodes, sometimes we write in comments the union
of types that we expect here. For example:

Map<ir.Node /*ir.Field | ir.FunctionNode*/, ScopeInfo> ...;

It might also be worth mentioning in the doc, for instance:

 /// ...
 /// Keys can be either function nodes (explicit Dart closures) or
 /// static fields (closures are created for lazy initialization).

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:29: /// Pointer to the
context in which this closure is executed.
maybe include an example of what that context can be? the enclosing method?

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:52: /// Lookup the local
entity that corresponds to a particular kernel node.
nit: "particular kernel node" => "kernel variable declaration"?

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:70: // TODO(efortuna): Need
to figure out how to test isAssignable with Local
you could directly inspect this on the ir.VariableDeclaration (asking if it's
final or const)

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:74:
capturedVariablesForScope.add(_localsMap.getLocal(variable));
<no action here, just a general comment> - here it is important that we are
using the kernel-2-j-entities map on this part of the algorithm, (and same
further below when creating the ThisLocal) all of these are going to be used by
inference and ssa assuming they are part of the J world.

If we wanted to fully make this analysis agnostic of the J/K, we'd have to
replicate the ClosureScope class hierarchy using a similar structure with
ir.Nodes instead, and then convert it to ClosureScopes. IMO - not worth doing
that change as this time.

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:139:
_markVariableAsUsed(node.variable);
don't we need to also add node.variable as a mutated variable here?

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:170:
_mutatedVariables.remove(local);
local => variable?

In that case, it appears you can drop the call to `getLocal` too?

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:181: if
(node.variables.isEmpty) return;
minor nit: delete this line? (since the next loop would be empty and is the last
thing on this method)

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:183: Local local =
_localsMap.getLocal(variable);
move getLocal inside the branch below?

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:205: if (!_isInsideClosure)
{
consider adding a comment saying that outermostNode is only null when we first
enter the body of the field/ctor/method that is being analyzed.

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:232: // Note that the check
on [insideClosure] is not just an
seems like this comment should move next to where you check for insideClosure,
maybe move to markVariableAsUsed?

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:245: if (variable == node)
return true;
Did you mean to check here against _exectuableContext (rather than variable)?

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:246: node = node.parent;
since ir nodes are connected all the way to the library, it might be worth
stopping the loop sooner, maybe when node == _outermostNode?

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/ke...
File pkg/compiler/lib/src/kernel/element_map_impl.dart (right):

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/ke...
pkg/compiler/lib/src/kernel/element_map_impl.dart:697: ir.Member
getKernelMember(KMember entity) =>
@override?

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/locals_handler.dart (right):

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/locals_handler.dart:181: void updateCaptureBox(Local
context, List<Local> toBeCopiedElements) {
nit: maybe rename context or update the comment to undersdtand the connection
here. For example `context` => `currentBox`?

https://codereview.chromium.org/2961253005/diff/20001/tests/compiler/dart2js/...
File tests/compiler/dart2js/forloop_box_test.dart (right):

https://codereview.chromium.org/2961253005/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/forloop_box_test.dart:45: 'for-loop variable was boxed');
while we are here, let's update the failure reason also to say what it should
do, instead of what happened.

(e.g. here "... variable should not be boxed")

[Emily Fortuna, 2017-06-30 23:48:10]

PTAL

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_model/closure.dart (right):

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure.dart:12: import
'../js_model/elements.dart';
On 2017/06/30 22:02:08, Siggi Cherem (dart-lang) wrote:
> I might be misreading it, but isn't this also under `js_model` (remove the
> ../js_model?)

ah, yup. this was written before Johnni moved more stuff around.

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure.dart:36: Map<Entity,
ClosureRepresentationInfo> _closureRepresentationCache =
On 2017/06/30 22:02:08, Siggi Cherem (dart-lang) wrote:
> nit: Cache => Map (to be consistent)?

Done.

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure.dart:41: final JsToFrontendMap
_kToJElementMap;
On 2017/06/30 22:02:08, Siggi Cherem (dart-lang) wrote:
> Let's add a TODO to eliminate this mapping.
> 
> Johnni - Emily and I talked offine and we realized this is related to the
> distinction with KernelBackendStrategyImpl and JsBackendStrategy. Is the idea
> that we will have JsBackendStrategy only in the future? It seems we want to
pass
> here the kElementMap (KernelToElementMapForBuilding) and not the jElementMap,
> correct?

Done. 

Johnni, please see Siggi's comment, too.

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure.dart:57: Map<ir.Node, ScopeInfo>
flaggedClosures = <ir.Node, ScopeInfo>{};
On 2017/06/30 22:02:09, Siggi Cherem (dart-lang) wrote:
> Seems we have 3 names for this :) - flaggedClosures, closuresToRewrite, and
> closuresToGenerate. Maybe rename them all to closuresToGenerate?

Done and done!

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure.dart:86: visitor.ClosureScopeBuilder
translator = new visitor.ClosureScopeBuilder(
On 2017/06/30 22:02:08, Siggi Cherem (dart-lang) wrote:
> nit: I'd probably drop the prefix, maybe use a `show` in the import if you
want
> to highlight where this type is defined.

Done.

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure.dart:140: return const ClosureScope();
On 2017/06/30 22:02:08, Siggi Cherem (dart-lang) wrote:
> nit: you can also write the entire body as:
> 
> ClosureScope getClosureScope(ir.Node node) =>
>   _closureScopeMap[node] ?? const ClosureScope();

ah, thanks! This was the product of code evolution, where I first tried to just
always look it up in the map, only to discover we were not ready for that sort
of "prime-time" yet. :-P

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure.dart:147: // TODO(efortuna): Eventually
scope should always be non-null, and
On 2017/06/30 22:02:09, Siggi Cherem (dart-lang) wrote:
> is it not null and not KernelLoopClosureScope because of the default value in
> `getClosureScope` above?
> 
> Maybe change to directly call _closureScopeMap instead?
> 
>   => _closureScopeMap[loopNode] ?? const LoopClosureScope();
> 
> ?

To your first question, yes. It was written this way so that when this stuff
goes away, we can always just call getClosureScope directly, but it's fine this
way, too.

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure.dart:156: var closureImpl =
_closureRepresentationCache[entity];
On 2017/06/30 22:02:08, Siggi Cherem (dart-lang) wrote:
> same compact version here

Done.

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure.dart:184: Set<Local> get
variablesUsedInTryOrSync => _localsUsedInTryOrSync;
On 2017/06/30 22:02:08, Siggi Cherem (dart-lang) wrote:
> since you are not updating the private field, I think we can get rid of that
> field and make this getter a final field instead:
> 
> final Set<Local> variablesUsedInTryOrSync;
> KernelScopeInfo(this.thisLocal)
>   : variablesUsedInTryOrSync = new Set<Local>();
> 
> KernelScopeInfo.from(this.thisLocal, info)
>   : variablesUsedInTryOrSync = info.variablesUsedInSync;
> 
> Also - if we don't intend to update the `boxedVariables` field, let's make it
> final as well.

Yeah... this was a holdover from Johnni's design. I'm totally fine changing it
tho.

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure.dart:265: bool get isClosure => false;
On 2017/06/30 22:02:08, Siggi Cherem (dart-lang) wrote:
> On 2017/06/30 17:58:40, Emily Fortuna wrote:
> > what's this? you may ask. Well, to properly call ourselves a closure, we
need
> to
> > register the new closure class with the ClosedWorldRefiner, which currently
> only
> > takes elements. The change to that (and the subsequent adjustment here) will
> > follow soon in a subsequent CL.
> 
> I'm ok copying this comment into the code if you like :)

Done.

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_model/closure_visitors.dart (right):

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:16: /// A map of each
visited call node with the associated information about what
On 2017/06/30 22:02:09, Siggi Cherem (dart-lang) wrote:
> I am not sure what the keys of the map are, maybe you can clarify in the
> comment? It appears like you have a key for every scope in the program?

Done.

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:20: /// A map of the
elements corresponding to nodes that we have flagged as
On 2017/06/30 22:02:09, Siggi Cherem (dart-lang) wrote:
> now that you are using ir.Node as keys, maybe remove "elements corresponding
to
> ..."? For instance:
> 
>   /// Maps nodes that require the compiler to create a closure class.
> 

Done.

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:24: Map<ir.Node, ScopeInfo>
_closuresToGenerate = <ir.Node, ScopeInfo>{};
On 2017/06/30 22:02:09, Siggi Cherem (dart-lang) wrote:
> to better document what kind of nodes, sometimes we write in comments the
union
> of types that we expect here. For example:
> 
> Map<ir.Node /*ir.Field | ir.FunctionNode*/, ScopeInfo> ...;
> 
> It might also be worth mentioning in the doc, for instance:
> 
>  /// ...
>  /// Keys can be either function nodes (explicit Dart closures) or
>  /// static fields (closures are created for lazy initialization).

Done.

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:29: /// Pointer to the
context in which this closure is executed.
On 2017/06/30 22:02:09, Siggi Cherem (dart-lang) wrote:
> maybe include an example of what that context can be? the enclosing method?

Done.

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:52: /// Lookup the local
entity that corresponds to a particular kernel node.
On 2017/06/30 22:02:09, Siggi Cherem (dart-lang) wrote:
> nit: "particular kernel node" => "kernel variable declaration"?

Done.

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:70: // TODO(efortuna): Need
to figure out how to test isAssignable with Local
On 2017/06/30 22:02:09, Siggi Cherem (dart-lang) wrote:
> you could directly inspect this on the ir.VariableDeclaration (asking if it's
> final or const)

oh DUH! This comment was from when I was looking at entities.

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:74:
capturedVariablesForScope.add(_localsMap.getLocal(variable));
On 2017/06/30 22:02:09, Siggi Cherem (dart-lang) wrote:
> <no action here, just a general comment> - here it is important that we are
> using the kernel-2-j-entities map on this part of the algorithm, (and same
> further below when creating the ThisLocal) all of these are going to be used
by
> inference and ssa assuming they are part of the J world.
> 
> If we wanted to fully make this analysis agnostic of the J/K, we'd have to
> replicate the ClosureScope class hierarchy using a similar structure with
> ir.Nodes instead, and then convert it to ClosureScopes. IMO - not worth doing
> that change as this time.

agreed. I was seeing that as I was making the ir.Node switch, but pure-ir
version looked to be further in the future.

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:139:
_markVariableAsUsed(node.variable);
On 2017/06/30 22:02:09, Siggi Cherem (dart-lang) wrote:
> don't we need to also add node.variable as a mutated variable here?

thank you. you're finding all the missing pieces for where the testing needs to
go. :-P

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:170:
_mutatedVariables.remove(local);
On 2017/06/30 22:02:09, Siggi Cherem (dart-lang) wrote:
> local => variable?
> 
> In that case, it appears you can drop the call to `getLocal` too?

oh goodness, yes, that was a holdover from the Locals-as-keys time from before.

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:181: if
(node.variables.isEmpty) return;
On 2017/06/30 22:02:09, Siggi Cherem (dart-lang) wrote:
> minor nit: delete this line? (since the next loop would be empty and is the
last
> thing on this method)

Done.

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:183: Local local =
_localsMap.getLocal(variable);
On 2017/06/30 22:02:09, Siggi Cherem (dart-lang) wrote:
> move getLocal inside the branch below?

Done. Yeah, that was a holdover from when the entities were keys in more places.

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:205: if (!_isInsideClosure)
{
On 2017/06/30 22:02:09, Siggi Cherem (dart-lang) wrote:
> consider adding a comment saying that outermostNode is only null when we first
> enter the body of the field/ctor/method that is being analyzed.

Done.

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:232: // Note that the check
on [insideClosure] is not just an
On 2017/06/30 22:02:09, Siggi Cherem (dart-lang) wrote:
> seems like this comment should move next to where you check for insideClosure,
> maybe move to markVariableAsUsed?

Done.

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:245: if (variable == node)
return true;
On 2017/06/30 22:02:09, Siggi Cherem (dart-lang) wrote:
> Did you mean to check here against _exectuableContext (rather than variable)?

oops, good catch!

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:246: node = node.parent;
On 2017/06/30 22:02:09, Siggi Cherem (dart-lang) wrote:
> since ir nodes are connected all the way to the library, it might be worth
> stopping the loop sooner, maybe when node == _outermostNode?

Done.

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/ke...
File pkg/compiler/lib/src/kernel/element_map_impl.dart (right):

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/ke...
pkg/compiler/lib/src/kernel/element_map_impl.dart:697: ir.Member
getKernelMember(KMember entity) =>
On 2017/06/30 22:02:10, Siggi Cherem (dart-lang) wrote:
> @override?

no, in this case, like the "getKernelClass" above, it's only exposed in the Impl
class, so it's not overriding anything.

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/locals_handler.dart (right):

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/locals_handler.dart:181: void updateCaptureBox(Local
context, List<Local> toBeCopiedElements) {
On 2017/06/30 22:02:10, Siggi Cherem (dart-lang) wrote:
> nit: maybe rename context or update the comment to undersdtand the connection
> here. For example `context` => `currentBox`?

Done.

https://codereview.chromium.org/2961253005/diff/20001/tests/compiler/dart2js/...
File tests/compiler/dart2js/forloop_box_test.dart (right):

https://codereview.chromium.org/2961253005/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/forloop_box_test.dart:45: 'for-loop variable was boxed');
On 2017/06/30 22:02:10, Siggi Cherem (dart-lang) wrote:
> while we are here, let's update the failure reason also to say what it should
> do, instead of what happened.
> 
> (e.g. here "... variable should not be boxed")

Done.

[Siggi Cherem (dart-lang), 2017-07-01 00:39:43]

lgtm! woohoo!

https://codereview.chromium.org/2961253005/diff/40001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_model/closure_visitors.dart (right):

https://codereview.chromium.org/2961253005/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_model/closure_visitors.dart:212: // field, constsructor,
or method that is being analyzed.
typo: constructor (extra `s`)

https://codereview.chromium.org/2961253005/diff/40001/tests/compiler/dart2js/...
File tests/compiler/dart2js/forloop_box_test.dart (right):

https://codereview.chromium.org/2961253005/diff/40001/tests/compiler/dart2js/...
tests/compiler/dart2js/forloop_box_test.dart:53: 'for-loop variable was not
boxed');
maybe this one too :)

[Emily Fortuna, 2017-07-01 14:12:43]

Description was changed from

==========
Added for-loop variable tracking and regular closures/initializers captured
variable tracking.


This whole thing is hanging together by a thread. A BUNCH of tests and more
implementation coming next once the KernelClosureClass is truly functional.

BUG=
==========

to

==========
Added for-loop variable tracking and regular closures/initializers captured
variable tracking.


This whole thing is hanging together by a thread. A BUNCH of tests and more
implementation coming next once the KernelClosureClass is truly functional.

BUG=
R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/d40f84fbf8fbe13ab37a62959ec20a924bd82d88
==========

[Emily Fortuna, 2017-07-01 14:12:44]

Committed patchset #2 (id:40001) manually as
d40f84fbf8fbe13ab37a62959ec20a924bd82d88 (presubmit successful).

[Johnni Winther, 2017-07-03 07:36:57]

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

https://codereview.chromium.org/2961253005/diff/20001/pkg/compiler/lib/src/ke...
pkg/compiler/lib/src/kernel/kernel_backend_strategy.dart:81: _compiler.measurer,
elementMap, null, _globalLocalsMap);
On 2017/06/30 17:58:40, Emily Fortuna wrote:
> This seems... super sketchy. Johnni, I don't understand the distinction
between
> this strategy and the js_strategy? I mean, I understand conceptually, but is
> this particular strategy actually being exercised anywhere? It just seemed
less
> implemented.

KernelBackendStrategy is to be replaced by JsStrategy