More work on signatures

Change-Id: Ifd8c20f1947149d60cca7c69e40ad85533519746
Reviewed-on: https://dart-review.googlesource.com/45700
Reviewed-by: Emily Fortuna <efortuna@google.com>

pkg/compiler/lib/src/js_emitter/runtime_type_generator.dart

@@ -175,6 +175,10 @@ class RuntimeTypeGenerator {
         }
       }
 
+      // TODO(johnniwinther): Avoid unneeded function type indices or
+      // signatures. We either need them for mirrors or because [type] is
+      // potentially a subtype of a checked function. Currently we eagerly
+      // generate a function type index or signature for all callable classes.
       if (storeFunctionTypeInMetadata && !type.containsTypeVariables) {
         // TODO(sigmund): use output unit of `method` (Issue #31032)
         OutputUnit outputUnit = _outputUnitData.mainOutputUnit;
@@ -191,9 +195,7 @@ class RuntimeTypeGenerator {
           encoding = generatedCode[signature];
         } else {
           // TODO(efortuna): Reinsert assertion.
-          // TODO(johnniwinther): Avoid unneeded signatures from closure
-          // classes.
-          // Use shared signature function.
+          // Generate the signature on the fly.
           encoding = _rtiEncoder.getSignatureEncoding(
               emitterTask.emitter, type, thisAccess);
         }

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

@@ -116,7 +116,6 @@ class KernelClosureConversionTask extends ClosureConversionTask<ir.Node> {
 
   void _updateScopeBasedOnRtiNeed(
       KernelScopeInfo scope,
-      ir.Node node,
       bool Function(ClassEntity) classNeedsTypeArguments,
       bool Function(MemberEntity) methodNeedsTypeArguments,
       bool Function(ir.Node) localFunctionNeedsTypeArguments,
@@ -189,7 +188,7 @@ class KernelClosureConversionTask extends ClosureConversionTask<ir.Node> {
           .forEach((ir.Node node, KernelCapturedScope scope) {
         Map<Local, JRecordField> boxedVariables =
             _elementMap.makeRecordContainer(scope, member, localsMap);
-        _updateScopeBasedOnRtiNeed(scope, node, classNeedsTypeArguments,
+        _updateScopeBasedOnRtiNeed(scope, classNeedsTypeArguments,
             methodNeedsTypeArguments, localFunctionNeedsTypeArguments, member);
 
         if (scope is KernelCapturedLoopScope) {
@@ -222,7 +221,8 @@ class KernelClosureConversionTask extends ClosureConversionTask<ir.Node> {
             classNeedsTypeArguments,
             methodNeedsTypeArguments,
             localFunctionNeedsTypeArguments,
-            needsSignature: localFunctionNeedsSignature(functionNode));
+            createSignatureMethod:
+                localFunctionNeedsSignature(functionNode.parent));
         // Add also for the call method.
         _scopeMap[closureClassInfo.callMethod] = closureClassInfo;
         _scopeMap[closureClassInfo.signatureMethod] = closureClassInfo;
@@ -247,14 +247,14 @@ class KernelClosureConversionTask extends ClosureConversionTask<ir.Node> {
       bool Function(ClassEntity) classNeedsTypeArguments,
       bool Function(FunctionEntity) methodNeedsTypeArguments,
       bool Function(ir.Node) localFunctionNeedsTypeArguments,
-      {bool needsSignature}) {
-    _updateScopeBasedOnRtiNeed(info, node.parent, classNeedsTypeArguments,
+      {bool createSignatureMethod}) {
+    _updateScopeBasedOnRtiNeed(info, classNeedsTypeArguments,
         methodNeedsTypeArguments, localFunctionNeedsTypeArguments, member);
     KernelToLocalsMap localsMap = _globalLocalsMap.getLocalsMap(member);
     KernelClosureClassInfo closureClassInfo =
         closedWorldBuilder.buildClosureClass(
             member, node, member.library, boxedVariables, info, localsMap,
-            needsSignature: needsSignature);
+            createSignatureMethod: createSignatureMethod);
 
     // We want the original declaration where that function is used to point
     // to the correct closure class.
@@ -263,7 +263,7 @@ class KernelClosureConversionTask extends ClosureConversionTask<ir.Node> {
     _memberClosureRepresentationMap[closureClassInfo.signatureMethod] =
         closureClassInfo;
     _globalLocalsMap.setLocalsMap(closureClassInfo.callMethod, localsMap);
-    if (needsSignature) {
+    if (createSignatureMethod) {
       _globalLocalsMap.setLocalsMap(
           closureClassInfo.signatureMethod, localsMap);
     }

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

@@ -291,26 +291,40 @@ class JsClosedWorldBuilder {
       Set<ir.Node> localFunctionsNodesNeedingSignature = new Set<ir.Node>();
       for (KLocalFunction localFunction
           in kernelRtiNeed.localFunctionsNeedingSignature) {
-        localFunctionsNodesNeedingSignature.add(localFunction.node);
+        ir.Node node = localFunction.node;
+        assert(node is ir.FunctionDeclaration || node is ir.FunctionExpression,
+            "Unexpected local function node: $node");
+        localFunctionsNodesNeedingSignature.add(node);
       }
       Set<ir.Node> localFunctionsNodesNeedingTypeArguments = new Set<ir.Node>();
       for (KLocalFunction localFunction
           in kernelRtiNeed.localFunctionsNeedingTypeArguments) {
-        localFunctionsNodesNeedingTypeArguments.add(localFunction.node);
+        ir.Node node = localFunction.node;
+        assert(node is ir.FunctionDeclaration || node is ir.FunctionExpression,
+            "Unexpected local function node: $node");
+        localFunctionsNodesNeedingTypeArguments.add(node);
       }
 
       RuntimeTypesNeedImpl jRtiNeed =
           _convertRuntimeTypesNeed(map, backendUsage, kernelRtiNeed);
       callMethods = _closureConversionTask.createClosureEntities(
           this, map.toBackendMemberMap(closureModels, identity),
-          localFunctionNeedsSignature: backendUsage.isRuntimeTypeUsed
-              ? (_) => true
-              : localFunctionsNodesNeedingSignature.contains,
+          localFunctionNeedsSignature: (ir.Node node) {
+            assert(node is ir.FunctionDeclaration ||
+                node is ir.FunctionExpression);
+            return backendUsage.isRuntimeTypeUsed
+                ? true
+                : localFunctionsNodesNeedingSignature.contains(node);
+          },
           classNeedsTypeArguments: jRtiNeed.classNeedsTypeArguments,
           methodNeedsTypeArguments: jRtiNeed.methodNeedsTypeArguments,
-          localFunctionNeedsTypeArguments: backendUsage.isRuntimeTypeUsed
-              ? (_) => true
-              : localFunctionsNodesNeedingTypeArguments.contains);
+          localFunctionNeedsTypeArguments: (ir.Node node) {
+            assert(node is ir.FunctionDeclaration ||
+                node is ir.FunctionExpression);
+            return backendUsage.isRuntimeTypeUsed
+                ? true
+                : localFunctionsNodesNeedingTypeArguments.contains(node);
+          });
 
       List<FunctionEntity> callMethodsNeedingSignature = <FunctionEntity>[];
       for (ir.Node node in localFunctionsNodesNeedingSignature) {
@@ -536,7 +550,7 @@ class JsClosedWorldBuilder {
       Map<Local, JRecordField> boxedVariables,
       KernelScopeInfo info,
       KernelToLocalsMap localsMap,
-      {bool needsSignature}) {
+      {bool createSignatureMethod}) {
     ClassEntity superclass = _commonElements.closureClass;
 
     KernelClosureClassInfo closureClassInfo = _elementMap.constructClosureClass(
@@ -547,7 +561,7 @@ class JsClosedWorldBuilder {
         info,
         localsMap,
         new InterfaceType(superclass, const []),
-        needsSignature: needsSignature);
+        createSignatureMethod: createSignatureMethod);
 
     // Tell the hierarchy that this is the super class. then we can use
     // .getSupertypes(class)

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

@@ -2461,7 +2461,7 @@ class JsKernelToElementMap extends KernelToElementMapBase
       KernelScopeInfo info,
       KernelToLocalsMap localsMap,
       InterfaceType supertype,
-      {bool needsSignature}) {
+      {bool createSignatureMethod}) {
     InterfaceType memberThisType = member.enclosingClass != null
         ? _elementEnvironment.getThisType(member.enclosingClass)
         : null;
@@ -2530,7 +2530,7 @@ class JsKernelToElementMap extends KernelToElementMapBase
     _buildClosureClassFields(closureClassInfo, member, memberThisType, info,
         localsMap, recordFieldsVisibleInScope, memberMap);
 
-    if (needsSignature) {
+    if (createSignatureMethod) {
       _constructSignatureMethod(closureClassInfo, memberMap, node,
           memberThisType, location, typeVariableAccess);
     }

tests/compiler/dart2js/rti/data/closure_unneeded.dart

@@ -0,0 +1,20 @@
+// Copyright (c) 2018, 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.
+
+import 'package:expect/expect.dart';
+
+/*class: A:*/
+class A<T> {
+  @NoInline()
+  m() {
+    return /**/ (T t, String s) {};
+  }
+}
+
+@NoInline()
+test(o) => o is void Function(int);
+
+main() {
+  test(new A<int>().m());
+}

tests/compiler/dart2js/rti/data/type_variable_function_type.dart

@@ -0,0 +1,29 @@
+// Copyright (c) 2018, 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.
+
+// Based on tests\language_2\type_variable_function_type_test.dart
+
+import 'package:expect/expect.dart';
+
+typedef T Func<T>();
+
+/*class: Foo:explicit=[Foo.S Function()],needsArgs*/
+class Foo<S> {
+  m(x) => x is Func<S>;
+}
+
+/*class: Bar:needsArgs*/
+class Bar<T> {
+  f() {
+    /*needsSignature*/ T local() => null;
+    return local;
+  }
+}
+
+void main() {
+  dynamic x = new Foo<List<String>>();
+  if (new DateTime.now().millisecondsSinceEpoch == 42) x = new Foo<int>();
+  Expect.isFalse(x.m(new Bar<String>().f()));
+  Expect.isTrue(x.m(new Bar<List<String>>().f()));
+}

tests/compiler/dart2js/rti/emission/closure_signature.dart

@@ -0,0 +1,26 @@
+// Copyright (c) 2018, 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.
+
+import 'package:expect/expect.dart';
+
+/*class: A:checks=[],instance*/
+class A<T> {
+  @NoInline()
+  m() {
+    return /*checks=[$signature],instance*/ (T t) {};
+  }
+
+  @NoInline()
+  f() {
+    return /*checks=[],functionType,instance*/ (int t) {};
+  }
+}
+
+@NoInline()
+test(o) => o is void Function(int);
+
+main() {
+  test(new A<int>().m());
+  test(new A<int>().f());
+}

tests/compiler/dart2js/rti/emission/closure_signature_unneeded.dart

@@ -0,0 +1,22 @@
+// Copyright (c) 2018, 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.
+
+import 'package:expect/expect.dart';
+
+/*class: A:checks=[],instance*/
+class A<T> {
+  @NoInline()
+  m() {
+    // TODO(johnniwinther): The signature is not needed since the type isn't a
+    // potential subtype of the checked function types.
+    return /*checks=[$signature],instance*/ (T t, String s) {};
+  }
+}
+
+@NoInline()
+test(o) => o is void Function(int);
+
+main() {
+  test(new A<int>().m());
+}

tests/compiler/dart2js/rti/emission/type_variable_function_type.dart

@@ -0,0 +1,30 @@
+// Copyright (c) 2018, 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.
+
+// Based on tests\language_2\type_variable_function_type_test.dart
+
+import 'package:expect/expect.dart';
+
+typedef T Func<T>();
+
+/*class: Foo:checks=[],instance*/
+class Foo<S> {
+  m(x) => x is Func<S>;
+}
+
+/*class: Bar:checks=[],instance*/
+class Bar<T> {
+  f() {
+    /*checks=[$signature],instance*/
+    T local() => null;
+    return local;
+  }
+}
+
+void main() {
+  dynamic x = new Foo<List<String>>();
+  if (new DateTime.now().millisecondsSinceEpoch == 42) x = new Foo<int>();
+  Expect.isFalse(x.m(new Bar<String>().f()));
+  Expect.isTrue(x.m(new Bar<List<String>>().f()));
+}

tests/compiler/dart2js_extra/closure_signature_unneeded_test.dart

@@ -0,0 +1,19 @@
+// Copyright (c) 2018, 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.
+
+import 'package:expect/expect.dart';
+
+class A<T> {
+  @NoInline()
+  m() {
+    return (T t, String s) {};
+  }
+}
+
+@NoInline()
+test(o) => o is void Function(int);
+
+main() {
+  Expect.isFalse(test(new A<int>().m()));
+}

tests/compiler/dart2js_extra/dart2js_extra.status

@@ -12,6 +12,7 @@ mirrors_used_closure_test: Fail # Issue 17939
 no_such_method_test: Fail # Wrong Invocation.memberName.
 statements_test: Fail
 typed_locals_test: Pass, Fail
+closure_signature_unneeded_test: RuntimeError # Too eager signature generation.
 
 [ $compiler != dart2js ]
 dummy_compiler_test: SkipByDesign # Issue 30773. Test should be migrated as a unit test of dart2js, is only intended to test self-hosting.