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

pkg/compiler/lib/src/js_model/closure.dart

Index: pkg/compiler/lib/src/js_model/closure.dart
diff --git a/pkg/compiler/lib/src/js_model/closure.dart b/pkg/compiler/lib/src/js_model/closure.dart
index 3ba9ec542cdd60ada05161b50796b654f43bf572..bb83905c3beb5f02c0683279d010be55374dded4 100644
--- a/pkg/compiler/lib/src/js_model/closure.dart
+++ b/pkg/compiler/lib/src/js_model/closure.dart
@@ -1,4 +1,4 @@
-// Copyright (c) 2016, the Dart project authors.  Please see the AUTHORS file
+// Copyright (c) 2017, 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.
 
@@ -9,54 +9,10 @@ import '../common/tasks.dart';
 import '../elements/entities.dart';
 import '../kernel/element_map.dart';
 import '../world.dart';
+import '../js_model/elements.dart';

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

I might be misreading it, but isn't this also under `js_model` (remove the
../js_model?)

[Emily Fortuna, 2017/06/30 23:48:09]

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

+import 'closure_visitors.dart' as visitor;
 import 'locals.dart';
 
-class KernelClosureDataBuilder extends ir.Visitor {
-  final KernelToLocalsMap _localsMap;
-  final KernelClosureRepresentationInfo info;
-
-  bool _inTry = false;
-
-  KernelClosureDataBuilder(this._localsMap, ThisLocal thisLocal)
-      : info = new KernelClosureRepresentationInfo(thisLocal);
-
-  @override
-  defaultNode(ir.Node node) {
-    node.visitChildren(this);
-  }
-
-  @override
-  visitTryCatch(ir.TryCatch node) {
-    bool oldInTry = _inTry;
-    _inTry = true;
-    node.visitChildren(this);
-    _inTry = oldInTry;
-  }
-
-  @override
-  visitTryFinally(ir.TryFinally node) {
-    bool oldInTry = _inTry;
-    _inTry = true;
-    node.visitChildren(this);
-    _inTry = oldInTry;
-  }
-
-  @override
-  visitVariableGet(ir.VariableGet node) {
-    if (_inTry) {
-      info.registerUsedInTryOrSync(_localsMap.getLocal(node.variable));
-    }
-  }
-
-  @override
-  visitVariableSet(ir.VariableSet node) {
-    if (_inTry) {
-      info.registerUsedInTryOrSync(_localsMap.getLocal(node.variable));
-    }
-    node.visitChildren(this);
-  }
-}
-
 /// Closure conversion code using our new Entity model. Closure conversion is
 /// necessary because the semantics of closures are slightly different in Dart
 /// than JavaScript. Closure conversion is separated out into two phases:
@@ -69,14 +25,23 @@ class KernelClosureDataBuilder extends ir.Visitor {
 /// check out:
 /// http://siek.blogspot.com/2012/07/essence-of-closure-conversion.html or
 /// http://matt.might.net/articles/closure-conversion/.
+// TODO(efortuna): Change inheritance hierarchy so that the
+// ClosureConversionTask doesn't inherit from ClosureTask because it's just a
+// glorified timer.
 class KernelClosureConversionTask extends ClosureConversionTask<ir.Node> {
   final KernelToElementMapForBuilding _elementMap;
   final GlobalLocalsMap _globalLocalsMap;
-  Map<Entity, ClosureRepresentationInfo> _infoMap =
+  Map<ir.Node, ClosureScope> _closureScopeMap = <ir.Node, ClosureScope>{};
+
+  Map<Entity, ClosureRepresentationInfo> _closureRepresentationCache =

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

nit: Cache => Map (to be consistent)?

[Emily Fortuna, 2017/06/30 23:48:08]

Done.

       <Entity, ClosureRepresentationInfo>{};
 
-  KernelClosureConversionTask(
-      Measurer measurer, this._elementMap, this._globalLocalsMap)
+  /// Should only be used at the very beginning to ensure we are looking at the
+  /// right kind of elements.
+  final JsToFrontendMap _kToJElementMap;

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

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?

[Emily Fortuna, 2017/06/30 23:48:09]

Done. 

Johnni, please see Siggi's comment, too.

+
+  KernelClosureConversionTask(Measurer measurer, this._elementMap,
+      this._kToJElementMap, this._globalLocalsMap)
       : super(measurer);
 
   /// The combined steps of generating our intermediate representation of
@@ -88,55 +53,142 @@ class KernelClosureConversionTask extends ClosureConversionTask<ir.Node> {
   @override
   void convertClosures(Iterable<MemberEntity> processedEntities,
       ClosedWorldRefiner closedWorldRefiner) {
-    // TODO(efortuna): implement.
+    // The key in flaggedClosures is either a Local or a MemberEntity.
+    Map<ir.Node, ScopeInfo> flaggedClosures = <ir.Node, ScopeInfo>{};

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

nit: I'd use the type only once (either on the left or right), in strong-mode
inference will fill the other one.

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

Seems we have 3 names for this :) - flaggedClosures, closuresToRewrite, and
closuresToGenerate. Maybe rename them all to closuresToGenerate?

[Emily Fortuna, 2017/06/30 23:48:08]

Done and done!

+    processedEntities.forEach((MemberEntity kEntity) {
+      MemberEntity entity = kEntity;
+      if (_kToJElementMap != null) {
+        entity = _kToJElementMap.toBackendMember(kEntity);
+      }
+      if (entity.isAbstract) return;
+      if (entity.isField && !entity.isInstanceMember) {
+        ir.Field field = _elementMap.getMemberNode(entity);
+        // Skip top-level/static fields without an initializer.
+        if (field.initializer == null) return;
+      }
+      _buildClosureModel(entity, flaggedClosures, closedWorldRefiner);
+    });
+
+    for (ir.Node node in flaggedClosures.keys) {
+      _produceSyntheticElements(
+          node, flaggedClosures[node], closedWorldRefiner);
+    }
   }
 
-  /// TODO(johnniwinther,efortuna): Implement this.
-  @override
-  ClosureScope getClosureScope(ir.Node node) {
-    return const ClosureScope();
+  /// Inspect members and mark if those members capture any state that needs to
+  /// be marked as free variables.
+  void _buildClosureModel(
+      MemberEntity entity,
+      Map<ir.Node, ScopeInfo> closuresToRewrite,
+      ClosedWorldRefiner closedWorldRefiner) {
+    ir.Node node = _elementMap.getMemberNode(entity);
+    if (_closureScopeMap.keys.contains(node)) return;
+    visitor.ClosureScopeBuilder translator = new visitor.ClosureScopeBuilder(

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

nit: I'd probably drop the prefix, maybe use a `show` in the import if you want
to highlight where this type is defined.

[Emily Fortuna, 2017/06/30 23:48:09]

Done.

+        _closureScopeMap,
+        closuresToRewrite,
+        _globalLocalsMap.getLocalsMap(entity),
+        _elementMap);
+    if (entity.isField) {
+      if (node is ir.Field && node.initializer != null) {
+        translator.translateLazyInitializer(node);
+      }
+    } else {
+      assert(node is ir.Procedure || node is ir.Constructor);
+      translator.translateConstructorOrProcedure(node);
+    }
+  }
+
+  /// Given what variables are captured at each point, construct closure classes
+  /// with fields containing the captured variables to replicate the Dart
+  /// closure semantics in JS.
+  void _produceSyntheticElements(
+      ir.Node node, ScopeInfo info, ClosedWorldRefiner closedWorldRefiner) {
+    Entity entity;
+    KernelClosureClass closureClass =
+        new KernelClosureClass.fromScopeInfo(info);
+    if (node is ir.FunctionNode) {
+      // We want the original declaration where that function is used to point
+      // to the correct closure class.
+      // TODO(efortuna): entity equivalent of element.declaration?
+      node = (node as ir.FunctionNode).parent;
+      _closureRepresentationCache[closureClass.callMethod] = closureClass;
+    }
+
+    if (node is ir.Member) {
+      entity = _elementMap.getMember(node);
+    } else {
+      entity = _elementMap.getLocalFunction(node);
+    }
+    assert(entity != null);
+
+    _closureRepresentationCache[entity] = closureClass;
   }
 
   @override
   ScopeInfo getScopeInfo(Entity entity) {
-    // TODO(efortuna): Specialize this function from the one below.
     return getClosureRepresentationInfo(entity);
   }
 
-  /// TODO(johnniwinther,efortuna): Implement this.
+  @override
+  ClosureScope getClosureScope(ir.Node node) {
+    ClosureScope closureScope = _closureScopeMap[node];
+    if (closureScope != null) {
+      // TODO(efortuna): Eventually closureScope should always be non-null, and
+      // we should just test that with an assert.
+      return closureScope;
+    }
+    return const ClosureScope();

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

nit: you can also write the entire body as:

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

[Emily Fortuna, 2017/06/30 23:48:09]

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

+  }
+
   @override
   LoopClosureScope getLoopClosureScope(ir.Node loopNode) {
-    return const LoopClosureScope();
+    ClosureScope scope = getClosureScope(loopNode);
+    if (scope is! KernelLoopClosureScope) {
+      // TODO(efortuna): Eventually scope should always be non-null, and

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

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();

?

[Emily Fortuna, 2017/06/30 23:48:08]

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.

+      // we should just test that with an assert.
+      return const LoopClosureScope();
+    }
+    return scope;
   }
 
   @override
   ClosureRepresentationInfo getClosureRepresentationInfo(Entity entity) {
-    return _infoMap.putIfAbsent(entity, () {
-      if (entity is MemberEntity) {
-        ir.Member node = _elementMap.getMemberNode(entity);
-        ThisLocal thisLocal;
-        if (entity.isInstanceMember) {
-          thisLocal = new ThisLocal(entity);
-        }
-        KernelClosureDataBuilder builder = new KernelClosureDataBuilder(
-            _globalLocalsMap.getLocalsMap(entity), thisLocal);
-        node.accept(builder);
-        return builder.info;
-      }
+    var closureImpl = _closureRepresentationCache[entity];

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

same compact version here

[Emily Fortuna, 2017/06/30 23:48:09]

Done.

+    if (closureImpl != null) {
+      // TODO(efortuna): Eventually closureImpl should always be non-null, and
+      // we should just test that with an assert.
+      return closureImpl;
+    }
 
-      /// TODO(johnniwinther,efortuna): Implement this.
-      return const ClosureRepresentationInfo();
-    });
+    return const ClosureRepresentationInfo();
   }
 }
 
-// TODO(johnniwinther): Add unittest for the computed
-// [ClosureRepresentationInfo].
-class KernelClosureRepresentationInfo extends ClosureRepresentationInfo {
-  final ThisLocal thisLocal;
-  final Set<Local> _localsUsedInTryOrSync = new Set<Local>();
+class KernelScopeInfo extends ScopeInfo {
+  Set<Local> _localsUsedInTryOrSync = new Set<Local>();
+  final Local thisLocal;
+  Set<Local> boxedVariables = new Set<Local>();
 
-  KernelClosureRepresentationInfo(this.thisLocal);
+  /// The set of variables that were defined in another scope, but are used in
+  /// this scope.
+  Set<ir.VariableDeclaration> freeVariables = new Set<ir.VariableDeclaration>();
+
+  KernelScopeInfo(this.thisLocal);
+
+  KernelScopeInfo.from(this.thisLocal, KernelScopeInfo info)
+      : this._localsUsedInTryOrSync = info.variablesUsedInTryOrSync,
+        this.boxedVariables = info.boxedVariables;
+
+  KernelScopeInfo.withBoxedVariables(this.boxedVariables, this.thisLocal);
+
+  Set<Local> get variablesUsedInTryOrSync => _localsUsedInTryOrSync;

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

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.

[Emily Fortuna, 2017/06/30 23:48:09]

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

+
+  void forEachBoxedVariable(f(Local local, FieldEntity field)) {
+    boxedVariables.forEach((Local l) {
+      // TODO(efortuna): add FieldEntities as created.
+      f(l, null);
+    });
+  }
 
   void registerUsedInTryOrSync(Local local) {
     _localsUsedInTryOrSync.add(local);
@@ -151,4 +203,64 @@ class KernelClosureRepresentationInfo extends ClosureRepresentationInfo {
     sb.write('localsUsedInTryOrSync={${_localsUsedInTryOrSync.join(', ')}}');
     return sb.toString();
   }
+
+  bool isBoxed(Local variable) => boxedVariables.contains(variable);
+}
+
+class KernelClosureScope extends KernelScopeInfo implements ClosureScope {
+  final Local context;
+
+  KernelClosureScope(Set<Local> boxedVariables, this.context, Local thisLocal)
+      : super.withBoxedVariables(boxedVariables, thisLocal);
+
+  bool get requiresContextBox => boxedVariables.isNotEmpty;
+}
+
+class KernelLoopClosureScope extends KernelClosureScope
+    implements LoopClosureScope {
+  final List<Local> boxedLoopVariables;
+
+  KernelLoopClosureScope(Set<Local> boxedVariables, this.boxedLoopVariables,
+      Local context, Local thisLocal)
+      : super(boxedVariables, context, thisLocal);
+
+  bool get hasBoxedLoopVariables => boxedLoopVariables.isNotEmpty;
+}
+
+// TODO(johnniwinther): Add unittest for the computed [ClosureClass].
+class KernelClosureClass extends KernelScopeInfo
+    implements ClosureRepresentationInfo {
+  KernelClosureClass.fromScopeInfo(ScopeInfo info)
+      : super.from(info.thisLocal, info);
+
+  // TODO(efortuna): Implement.
+  Local get closureEntity => null;
+
+  // TODO(efortuna): Implement.
+  ClassEntity get closureClassEntity => null;
+
+  // TODO(efortuna): Implement.
+  FunctionEntity get callMethod => null;
+
+  // TODO(efortuna): Implement.
+  List<Local> get createdFieldEntities => const <Local>[];
+
+  // TODO(efortuna): Implement.
+  FieldEntity get thisFieldEntity => null;
+
+  // TODO(efortuna): Implement.
+  void forEachCapturedVariable(f(Local from, FieldEntity to)) {}
+
+  // TODO(efortuna): Implement.
+  @override
+  void forEachBoxedVariable(f(Local local, FieldEntity field)) {}
+
+  // TODO(efortuna): Implement.
+  void forEachFreeVariable(f(Local variable, FieldEntity field)) {}
+
+  // TODO(efortuna): Implement.
+  bool isVariableBoxed(Local variable) => false;
+
+  // TODO(efortuna): Implement.
+  bool get isClosure => false;

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

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.

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

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

[Emily Fortuna, 2017/06/30 23:48:09]

Done.

 }