Share locals between members

R=efortuna@google.com

Review-Url: https://codereview.chromium.org/2995113002 .

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

@@ -143,6 +143,7 @@ class KernelClosureConversionTask extends ClosureConversionTask<ir.Node> {
     // We want the original declaration where that function is used to point
     // to the correct closure class.
     _memberClosureRepresentationMap[closureClass.callMethod] = closureClass;
+    _globalLocalsMap.setLocalsMap(closureClass.callMethod, localsMap);
     if (node.parent is ir.Member) {
       assert(_elementMap.getMember(node.parent) == member);
       _memberClosureRepresentationMap[member] = closureClass;
@@ -425,13 +426,7 @@ class NodeBox {
 }
 
 class JClosureClass extends JClass {
-  // TODO(efortuna): Storing this map here is so horrible. Instead store this on
-  // the ScopeModel (because all of the closures share that localsMap) and then
-  // set populate the getLocalVariable lookup with this localsMap for all the
-  // closures.
-  final KernelToLocalsMap localsMap;
-
-  JClosureClass(this.localsMap, JLibrary library, int classIndex, String name)
+  JClosureClass(JLibrary library, int classIndex, String name)
       : super(library, classIndex, name, isAbstract: false);
 
   @override

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

@@ -6,7 +6,6 @@ library dart2js.js_model.locals;
 
 import 'package:kernel/ast.dart' as ir;
 
-import 'closure.dart' show JClosureClass;
 import '../closure.dart';
 import '../common.dart';
 import '../elements/entities.dart';
@@ -17,10 +16,20 @@ class GlobalLocalsMap {
   Map<MemberEntity, KernelToLocalsMap> _localsMaps =
       <MemberEntity, KernelToLocalsMap>{};
 
+  /// Returns the [KernelToLocalsMap] for [member].
   KernelToLocalsMap getLocalsMap(MemberEntity member) {
     return _localsMaps.putIfAbsent(
         member, () => new KernelToLocalsMapImpl(member));
   }
+
+  /// Associates [localsMap] with [member].
+  ///
+  /// Use this for sharing maps between members that share IR nodes.
+  void setLocalsMap(MemberEntity member, KernelToLocalsMap localsMap) {
+    assert(!_localsMaps.containsKey(member),
+        "Locals map already created for $member.");
+    _localsMaps[member] = localsMap;
+  }
 }
 
 class KernelToLocalsMapImpl implements KernelToLocalsMap {
@@ -126,15 +135,7 @@ class KernelToLocalsMapImpl implements KernelToLocalsMap {
   }
 
   @override
-  Local getLocalVariable(ir.VariableDeclaration node,
-      {bool isClosureCallMethod = false}) {
-    if (isClosureCallMethod && !_map.containsKey(node)) {
-      // Node might correspond to a free variable in the closure class.
-      assert(currentMember.enclosingClass is JClosureClass);
-      return (currentMember.enclosingClass as JClosureClass)
-          .localsMap
-          .getLocalVariable(node);
-    }
+  Local getLocalVariable(ir.VariableDeclaration node) {
     return _map.putIfAbsent(node, () {
       return new JLocal(
           node.name, currentMember, node.parent is ir.FunctionNode);

pkg/compiler/lib/src/kernel/element_map.dart

@@ -389,8 +389,7 @@ abstract class KernelToLocalsMap {
   /// variables involved with a closure class.
   // TODO(efortuna, johnniwinther): convey this information without a boolean
   // parameter.
-  Local getLocalVariable(ir.VariableDeclaration node,
-      {bool isClosureCallMethod = false});
+  Local getLocalVariable(ir.VariableDeclaration node);
 
   /// Returns the [Local] corresponding to the [node]. The node must be either
   /// a [ir.FunctionDeclaration] or [ir.FunctionExpression].

pkg/compiler/lib/src/kernel/element_map_impl.dart

@@ -2177,7 +2177,7 @@ class JsKernelToElementMap extends KernelToElementMapBase
       InterfaceType supertype) {
     String name = _computeClosureName(node);
     JClass classEntity =
-        new JClosureClass(localsMap, enclosingLibrary, _classEnvs.length, name);
+        new JClosureClass(enclosingLibrary, _classEnvs.length, name);
     _classList.add(classEntity);
     Map<String, MemberEntity> memberMap = <String, MemberEntity>{};
     _classEnvs.add(new ClosureClassEnv(memberMap));

pkg/compiler/lib/src/ssa/builder_kernel.dart

@@ -2237,10 +2237,7 @@ class KernelSsaGraphBuilder extends ir.Visitor
       return;
     }
 
-    Local local = localsMap.getLocalVariable(variableGet.variable,
-        isClosureCallMethod:
-            _elementMap.getMemberDefinition(targetElement).kind ==
-                MemberKind.closureCall);
+    Local local = localsMap.getLocalVariable(variableGet.variable);
     stack.add(localsHandler.readLocal(local));
   }
 

pkg/compiler/lib/src/ssa/locals_handler.dart

@@ -422,7 +422,8 @@ class LocalsHandler {
       HRef ref = value;
       value = ref.value;
     }
-    assert(!isStoredInClosureField(local));
+    assert(!isStoredInClosureField(local),
+        "Local $local is stored in a closure field.");
     if (isAccessedDirectly(local)) {
       directLocals[local] = value;
     } else if (isBoxed(local)) {

tests/compiler/dart2js_extra/dart2js_extra.status

@@ -182,20 +182,18 @@ regress/4562_test/01: Crash # Issue 27394
 23828_test: Crash
 assert_with_message_test: Crash
 async_stacktrace_test: Crash
+big_allocation_expression_test: Crash # 'file:///c:/Users/johnniwinther/dart-repo1/sdk/pkg/compiler/lib/src/ssa/locals_handler.dart': Failed assertion: line 425 pos 12: '!isStoredInClosureField(local)': Local local(Maps.mapToString#first) is stored in a closure field.
 bound_closure_interceptor_methods_test: Crash
 bound_closure_interceptor_type_test: Crash
 break_test: Crash
 closure3_test: Crash
-closure4_test: Crash
+closure4_test: RuntimeError
 closure6_test: Crash
 closure7_test: Crash
-closure_capture2_test: Crash
 closure_capture2_test: RuntimeError
-closure_capture3_test: Crash
-closure_capture4_test: Crash
+closure_capture3_test: Crash # Assertion failure: Cannot find value Instance of 'ThisLocal' in (local(Closure.nestedClosure#)) for j:closure_call(Closure_nestedClosure_closure.call).
 closure_capture4_test: RuntimeError
-closure_capture5_test: Crash
-closure_capture_test: Crash
+closure_capture5_test: Crash # 'file:///c:/Users/johnniwinther/dart-repo1/sdk/pkg/compiler/lib/src/ssa/locals_handler.dart': Failed assertion: line 425 pos 12: '!isStoredInClosureField(local)': Local local(closure4#g) is stored in a closure field.
 closure_capture_test: RuntimeError
 closure_type_reflection2_test: Crash
 closure_type_reflection_test: Crash
@@ -207,9 +205,7 @@ compile_time_constant4_test/04: MissingCompileTimeError
 compile_time_constant4_test/05: MissingCompileTimeError
 compile_time_constant4_test/06: Crash
 conditional_send_test: Crash
-consistent_add_error_test: Crash
 consistent_add_error_test: RuntimeError
-consistent_subtract_error_test: Crash
 consistent_subtract_error_test: RuntimeError
 constant_javascript_semantics_test/01: MissingCompileTimeError
 constant_javascript_semantics_test/02: MissingCompileTimeError
@@ -229,16 +225,18 @@ deferred_fail_and_retry_worker_test: CompileTimeError
 deferred_inheritance_test: CompileTimeError
 deferred_split_test: CompileTimeError
 dummy_compiler_test: CompileTimeError
+first_class_types_hashcode_test: Crash # 'file:///c:/Users/johnniwinther/dart-repo1/sdk/pkg/compiler/lib/src/ssa/locals_handler.dart': Failed assertion: line 425 pos 12: '!isStoredInClosureField(local)': Local local(Maps.mapToString#first) is stored in a closure field.
 for_in_test: Crash
 hash_code_test: Crash
+if_null2_test: Crash # 'file:///c:/Users/johnniwinther/dart-repo1/sdk/pkg/compiler/lib/src/ssa/locals_handler.dart': Failed assertion: line 425 pos 12: '!isStoredInClosureField(local)': Local local(Maps.mapToString#first) is stored in a closure field.
+if_null3_test: Crash # 'file:///c:/Users/johnniwinther/dart-repo1/sdk/pkg/compiler/lib/src/ssa/locals_handler.dart': Failed assertion: line 425 pos 12: '!isStoredInClosureField(local)': Local local(Maps.mapToString#first) is stored in a closure field.
 if_null_test: Crash
 inference_nsm_mirrors_test: Crash
 inference_super_set_call_test: RuntimeError
 inline_position_crash_test: Crash
 interceptor_named_arguments_test: Crash
 invalid_annotation2_test/none: Crash
 is_check_instanceof_test: Crash
-js_array_index_error_test: Crash
 js_dispatch_property_test: RuntimeError
 label_test/06: Crash
 lookup_map/dead_entry_single_nested_pairs_test: Crash
@@ -288,8 +286,6 @@ private_symbol_literal_test/05: MissingCompileTimeError
 private_symbol_literal_test/06: MissingCompileTimeError
 recursive_import_test: CompileTimeError
 reflect_native_types_test: Crash
-closure4_test: RuntimeError
-closure_capture5_test: RuntimeError
 regress/4562_test/none: CompileTimeError
 regress/4639_test: Crash
 string_interpolation_dynamic_test: RuntimeError
@@ -324,13 +320,13 @@ bound_closure_interceptor_methods_test: Crash # NoSuchMethodError: The getter 'e
 bound_closure_interceptor_type_test: Crash # NoSuchMethodError: The getter 'classIndex' was called on null.
 break_test: Crash # RangeError (index): Invalid value: Valid value range is empty: 0
 closure3_test: Crash # NoSuchMethodError: Class 'KernelClosureClass' has no instance getter 'supertype'.
-closure4_test: Crash # NoSuchMethodError: The getter 'classIndex' was called on null.
+closure4_test: RuntimeError
 closure6_test: Crash # NoSuchMethodError: Class 'KernelClosureClass' has no instance getter 'supertype'.
 closure7_test: Crash # NoSuchMethodError: Class 'KernelClosureClass' has no instance getter 'supertype'.
 closure_capture2_test: RuntimeError
 closure_capture3_test: Crash # Assertion failure: Cannot find value local(Closure_nestedClosure_closure.call#f) in (local(Closure.nestedClosure#)) for j:closure_call(Closure_nestedClosure_closure.call).
 closure_capture4_test: RuntimeError
-closure_capture5_test: Crash # Assertion failure: Cannot find value local(closure3_closure.call#fs) in (local(closure3#)) for j:closure_call(closure3_closure.call).
+closure_capture5_test: RuntimeError
 closure_capture_test: RuntimeError
 closure_type_reflection2_test: Crash # UnimplementedError: KernelClosedWorldMixin.getAppliedMixin
 closure_type_reflection_test: Crash # UnimplementedError: KernelClosedWorldMixin.getAppliedMixin
@@ -372,7 +368,6 @@ inline_position_crash_test: Crash # NoSuchMethodError: The getter 'classIndex' w
 interceptor_named_arguments_test: Crash # NoSuchMethodError: The getter 'memberIndex' was called on null.
 invalid_annotation2_test/none: Crash # NoSuchMethodError: Class 'DynamicType' has no instance getter 'element'.
 is_check_instanceof_test: Crash # NoSuchMethodError: The getter 'enclosingClass' was called on null.
-js_array_index_error_test: Crash # Assertion failure: Cannot find value local(variableIndexSetNonempty_closure.call#index) in (local(variableIndexSetNonempty#fault3), local(variableIndexSetNonempty_closure.call#a)) for j:closure_call(variableIndexSetNonempty_closure.call).
 js_dispatch_property_test: RuntimeError
 label_test/06: Crash # RangeError (index): Invalid value: Valid value range is empty: 0
 lookup_map/dead_entry_single_nested_pairs_test: Crash # NoSuchMethodError: Class 'KField' has no instance getter 'constant'.
@@ -422,8 +417,6 @@ private_symbol_literal_test/05: MissingCompileTimeError
 private_symbol_literal_test/06: MissingCompileTimeError
 recursive_import_test: CompileTimeError
 reflect_native_types_test: Crash # UnimplementedError: KernelClosedWorldMixin.getAppliedMixin
-closure4_test: RuntimeError
-closure_capture5_test: RuntimeError
 regress/4562_test/none: CompileTimeError
 regress/4639_test: Crash # NoSuchMethodError: The getter 'enclosingClass' was called on null.
 runtime_type_test: Crash

tests/compiler/dart2js_native/dart2js_native.status

@@ -150,6 +150,7 @@ field_type2_test: RuntimeError
 field_type_test: RuntimeError
 fixup_get_tag_test: RuntimeError
 hash_code_test: RuntimeError
+inference_of_helper_methods_test: Crash # 'file:///c:/Users/johnniwinther/dart-repo1/sdk/pkg/compiler/lib/src/ssa/locals_handler.dart': Failed assertion: line 425 pos 12: '!isStoredInClosureField(local)': Local local(main#value) is stored in a closure field.
 internal_library_test: Crash
 is_check_test: CompileTimeError
 issue9182_test: RuntimeError
@@ -175,8 +176,8 @@ native_class_with_dart_methods_frog_test: Crash
 native_closure_identity_frog_test: Crash
 native_constructor_name_test: RuntimeError
 native_equals_frog_test: RuntimeError
-native_exception2_test: RuntimeError
-native_exception_test: RuntimeError
+native_exception2_test: Crash # 'file:///c:/Users/johnniwinther/dart-repo1/sdk/pkg/compiler/lib/src/ssa/locals_handler.dart': Failed assertion: line 425 pos 12: '!isStoredInClosureField(local)': Local local(main#previous) is stored in a closure field.
+native_exception_test: Crash # 'file:///c:/Users/johnniwinther/dart-repo1/sdk/pkg/compiler/lib/src/ssa/locals_handler.dart': Failed assertion: line 425 pos 12: '!isStoredInClosureField(local)': Local local(main#previous) is stored in a closure field.
 native_exceptions1_frog_test: Crash
 native_field_invocation2_test: RuntimeError
 native_field_invocation3_test: RuntimeError
@@ -230,7 +231,7 @@ subclassing_constructor_2_test: RuntimeError
 subclassing_super_call_test: RuntimeError
 subclassing_super_field_1_test: RuntimeError
 subclassing_super_field_2_test: RuntimeError
-subclassing_type_test: Crash
+subclassing_type_test: RuntimeError
 super_call_test: RuntimeError
 super_property_test: RuntimeError
 
@@ -331,7 +332,7 @@ subclassing_constructor_2_test: RuntimeError
 subclassing_super_call_test: RuntimeError
 subclassing_super_field_1_test: RuntimeError
 subclassing_super_field_2_test: RuntimeError
-subclassing_type_test: Crash # NoSuchMethodError: The getter 'closureClassEntity' was called on null.
+subclassing_type_test: RuntimeError
 super_call_test: RuntimeError
 super_property_test: RuntimeError