Add ScopeInfo class for variable information that doesn't actually involve closures.

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 16e04cdb17dea8463ba9e8511640ab16f6d41f40..c0ef9a41d5a62833904a26037970afe3410c8e20 100644
--- a/pkg/compiler/lib/src/closure.dart
+++ b/pkg/compiler/lib/src/closure.dart
@@ -22,6 +22,7 @@ import 'package:front_end/src/fasta/scanner.dart' show Token;
 import 'tree/tree.dart';
 import 'util/util.dart';
 import 'world.dart' show ClosedWorldRefiner;
+import 'package:collection/collection.dart' show UnmodifiableSetView;
 
 // TODO(johnniwinther,efortuna): Split [ClosureConversionTask] from
 // [ClosureDataLookup].
@@ -43,6 +44,10 @@ abstract class ClosureDataLookup<T> {
   /// used inside the scope of [node].
   // TODO(johnniwinther): Split this up into two functions, one for members and
   // one for local functions.
+  ScopeInfo getScopeInfo(covariant Entity member);
+
+  /// This returns the same information as ScopeInfo, but can be called in
+  /// situations when you are sure you are dealing with a closure specifically.
   ClosureRepresentationInfo getClosureRepresentationInfo(
       covariant Entity member);
 
@@ -54,6 +59,36 @@ abstract class ClosureDataLookup<T> {
   ClosureAnalysisInfo getClosureAnalysisInfo(T node);
 }
 
+/// Class that represents one level of scoping information, whether this scope
+/// is a closure or not. This is specifically used to store information
+/// about the usage of variables in try or sync blocks, because they need to be
+/// boxed.
+///
+/// Variables that are used in a try must be treated as boxed because the
+/// control flow can be non-linear. Also parameters to a `sync*` generator must
+/// be boxed, because of the way we rewrite sync* functions. See also comments
+/// in [ClosureClassMap.useLocal].
+class ScopeInfo {
+  const ScopeInfo();
+
+  /// Set of [variable]s referenced in this scope that are used inside a
+  /// `try` block or a `sync*` generator (this is important to know because
+  /// boxing/redirection needs to happen for those local variables).
+  ///
+  /// Variables that are used in a try must be treated as boxed because the
+  /// control flow can be non-linear.
+  ///
+  /// Also parameters to a `sync*` generator must be boxed, because of the way
+  /// we rewrite sync* functions. See also comments in
+  /// [ClosureClassMap.useLocal].
+  // TODO(johnniwinther): Add variables to this only if the variable is mutated.
+  final Set<Local> variablesUsedInTryOrSync = const UnmodifiableSetView.empty();

[Siggi Cherem (dart-lang), 2017/06/23 19:11:56]

I lean towards simply using `new Set<Local>()` here instead and drop the
dependency on package:collection.

[Emily Fortuna, 2017/06/23 21:20:06]

switched to old API!

+
+  /// Convenience reference pointer to the element representing `this`.
+  /// If this scope is not in an instance member, it will be null.
+  Local get thisLocal => null;
+}
+
 /// Class that provides a black-box interface to information gleaned from
 /// analyzing a closure's characteristics, most commonly used to influence how
 /// code should be generated in SSA builder stage.
@@ -138,7 +173,7 @@ class LoopClosureRepresentationInfo extends ClosureAnalysisInfo {
 /// used in the inner scope of this closure, we say `y` is a "captured"
 /// variable.
 /// TODO(efortuna): Make interface simpler in subsequent refactorings.
-class ClosureRepresentationInfo {
+class ClosureRepresentationInfo extends ScopeInfo {
   const ClosureRepresentationInfo();
 
   /// The original local function before any translation.
@@ -162,26 +197,10 @@ class ClosureRepresentationInfo {
   /// accessor to that set of fields.
   List<Local> get createdFieldEntities => const <Local>[];
 
-  /// Convenience reference pointer to the element representing `this`.
-  /// It is only set for instance-members.
-  Local get thisLocal => null;
-
   /// Convenience pointer to the field entity representation in the closure
   /// class of the element representing `this`.
   FieldEntity get thisFieldEntity => null;
 
-  /// Returns true if this [variable] is used inside a `try` block or a `sync*`
-  /// generator (this is important to know because boxing/redirection needs to
-  /// happen for those local variables).
-  ///
-  /// Variables that are used in a try must be treated as boxed because the
-  /// control flow can be non-linear.
-  ///
-  /// Also parameters to a `sync*` generator must be boxed, because of the way
-  /// we rewrite sync* functions. See also comments in
-  /// [ClosureClassMap.useLocal].
-  bool variableIsUsedInTryOrSync(Local variable) => false;

[Siggi Cherem (dart-lang), 2017/06/23 19:11:56]

I think it's fine either way, but what I liked about having this bool-API is
that the `Set` implementation was hidden from consumers of this info.

It makes sense to expose the set so you can directly add to it in the
kernel-specific implementation though. So I can see that this API provides less
value now that the set is exposed.

[Emily Fortuna, 2017/06/23 21:20:06]

Moved back to the API it was!

-
   /// Loop through every variable that has been captured in this closure. This
   /// consists of all the free variables (variables captured *just* in this
   /// closure) and all variables captured in nested scopes that we may be
@@ -233,6 +252,10 @@ class ClosureTask extends ClosureConversionTask<Node> {
     return value == null ? const ClosureAnalysisInfo() : value;
   }
 
+  ScopeInfo getScopeInfo(Element member) {
+    return getClosureToClassMapping(member);
+  }
+
   ClosureRepresentationInfo getClosureRepresentationInfo(Element member) {
     return getClosureToClassMapping(member);
   }
@@ -690,13 +713,7 @@ class ClosureClassMap implements ClosureRepresentationInfo {
   /// not contain any nested closure.
   final Map<Node, ClosureScope> capturingScopes = new Map<Node, ClosureScope>();
 
-  /// Variables that are used in a try must be treated as boxed because the
-  /// control flow can be non-linear.
-  ///
-  /// Also parameters to a `sync*` generator must be boxed, because of the way
-  /// we rewrite sync* functions. See also comments in [useLocal].
-  // TODO(johnniwinther): Add variables to this only if the variable is mutated.
-  final Set<Local> variablesUsedInTryOrGenerator = new Set<Local>();
+  final Set<Local> variablesUsedInTryOrSync = new Set<Local>();

[Siggi Cherem (dart-lang), 2017/06/23 19:11:56]

alternatively, we could change "implements ClosureRepresentationInfo" to
"extends ...", and use the field from the supersuperclass

[Emily Fortuna, 2017/06/23 21:20:06]

Acknowledged.

 
   ClosureClassMap(this.closureEntity, this.closureClassEntity, this.callMethod,
       this.thisLocal);
@@ -727,9 +744,6 @@ class ClosureClassMap implements ClosureRepresentationInfo {
 
   FieldEntity get thisFieldEntity => freeVariableMap[thisLocal];
 
-  bool variableIsUsedInTryOrSync(Local variable) =>
-      variablesUsedInTryOrGenerator.contains(variable);
-
   Local getLocalVariableForClosureField(ClosureFieldElement field) {
     return field.local;
   }
@@ -969,13 +983,13 @@ class ClosureTranslator extends Visitor {
       if (variable != closureData.thisLocal &&
           variable != closureData.closureEntity &&
           variable is! TypeVariableLocal) {
-        closureData.variablesUsedInTryOrGenerator.add(variable);
+        closureData.variablesUsedInTryOrSync.add(variable);
       }
     } else if (variable is LocalParameterElement &&
         variable.functionDeclaration.asyncMarker == AsyncMarker.SYNC_STAR) {
       // Parameters in a sync* function are shared between each Iterator created
       // by the Iterable returned by the function, therefore they must be boxed.
-      closureData.variablesUsedInTryOrGenerator.add(variable);
+      closureData.variablesUsedInTryOrSync.add(variable);
     }
   }