[dart2js] Rti-need fix for generic instantiations

The rti-need of an instantiation is not the same as the rti-need of the
instantiated generic function. The generic function might not use its
type arguments but the instantiation still needs the type arguments for
equality.

dart2js implements generic function instantiation by creating instances
of a helper class that is a subclass of `Closure`. The helper class needs
its type parameters since they are used in `==` and `toString()` methods.
The fix for #47054 is to add the dependency of the helper class type
parameters on the instantiation type arguments.

Change-Id: I9ecb18e1b61a8ad6549f35b572476b1ed7ebb88d
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/212037
Reviewed-by: Sigmund Cherem <sigmund@google.com>
Reviewed-by: Mayank Patke <fishythefish@google.com>
Commit-Queue: Stephen Adams <sra@google.com>

pkg/compiler/lib/src/js_backend/runtime_types_resolution.dart

@@ -513,13 +513,27 @@ class TypeVariableTests {
     });
 
     for (GenericInstantiation instantiation in _genericInstantiations) {
+      ParameterStructure instantiationParameterStructure =
+          ParameterStructure.fromType(instantiation.functionType);
+      ClassEntity implementationClass = _commonElements
+          .getInstantiationClass(instantiation.typeArguments.length);
+
       void processEntity(Entity entity) {
         MethodNode node = _getMethodNode(entity);
-        if (node.parameterStructure ==
-            ParameterStructure.fromType(instantiation.functionType)) {
+        // TODO(sra,johnniwinther): Use more information from the instantiation
+        // site. At many sites the instantiated element known, and for other
+        // sites the static type could filter more entities.
+        if (node.parameterStructure == instantiationParameterStructure) {
           _instantiationMap.putIfAbsent(entity, () => {}).add(instantiation);
           for (DartType type in instantiation.typeArguments) {
             registerDependenciesForInstantiation(node, type);
+            // The instantiation is implemented by a generic class (a subclass
+            // of 'Closure'). The implementation of generic instantiation
+            // equality places a need on the type parameters of the generic
+            // class. Making the class a dependency on the instantiation's
+            // parameters allows the dependency to propagate back to the helper
+            // function that is called to create the instantiation.
+            registerDependencies(_getClassNode(implementationClass), type);
           }
         }
       }
@@ -1337,6 +1351,7 @@ class RuntimeTypesNeedBuilderImpl implements RuntimeTypesNeedBuilder {
       }
       if (neededOnAll) break;
     }
+
     Set<ClassEntity> allClassesNeedingRuntimeType;
     if (neededOnAll) {
       neededOnFunctions = true;
@@ -1399,13 +1414,19 @@ class RuntimeTypesNeedBuilderImpl implements RuntimeTypesNeedBuilder {
     Set<int> instantiationsNeedingTypeArguments = {};
     typeVariableTests.forEachInstantiatedEntity(
         (Entity target, Set<GenericInstantiation> instantiations) {
-      if (methodsNeedingTypeArguments.contains(target) ||
-          localFunctionsNeedingTypeArguments.contains(target)) {
-        // TODO(johnniwinther): Use the static type of the instantiated
-        // expression.
-        instantiationsNeedingTypeArguments
-            .add(instantiations.first.typeArguments.length);
-        if (retainDataForTesting) {
+      // An instantiation needs type arguments if the class implementing the
+      // instantiation needs type arguments.
+      int arity = instantiations.first.typeArguments.length;
+      if (!instantiationsNeedingTypeArguments.contains(arity)) {
+        if (classesNeedingTypeArguments
+            .contains(commonElements.getInstantiationClass(arity))) {
+          instantiationsNeedingTypeArguments.add(arity);
+        }
+      }
+
+      if (retainDataForTesting) {
+        if (methodsNeedingTypeArguments.contains(target) ||
+            localFunctionsNeedingTypeArguments.contains(target)) {
           _instantiatedEntitiesNeedingTypeArgumentsForTesting ??= {};
           _instantiatedEntitiesNeedingTypeArgumentsForTesting
               .putIfAbsent(target, () => {})

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

@@ -5134,25 +5134,10 @@ class KernelSsaGraphBuilder extends ir.Visitor<void> with ir.VisitorVoidMixin {
     List<HInstruction> arguments = [];
     node.expression.accept(this);
     arguments.add(pop());
-    StaticType expressionType = _getStaticType(node.expression);
-    FunctionType functionType = expressionType.type.withoutNullability;
-    bool typeArgumentsNeeded = _rtiNeed.instantiationNeedsTypeArguments(
-        functionType, node.typeArguments.length);
-    List<DartType> typeArguments = node.typeArguments
-        .map((type) => typeArgumentsNeeded
-            ? _elementMap.getDartType(type)
-            : _commonElements.dynamicType)
-        .toList();
-    registry.registerGenericInstantiation(
-        GenericInstantiation(functionType, typeArguments));
-    // TODO(johnniwinther): Can we avoid creating the instantiation object?
-    for (DartType type in typeArguments) {
-      HInstruction instruction =
-          _typeBuilder.analyzeTypeArgument(type, sourceElement);
-      arguments.add(instruction);
-    }
+
+    // A generic function instantiation is created by calling a helper function
+    // which takes the arguments.
     int typeArgumentCount = node.typeArguments.length;
-    bool targetCanThrow = false; // TODO(sra): Is this true?
     FunctionEntity target =
         _commonElements.getInstantiateFunction(typeArgumentCount);
     if (target == null) {
@@ -5163,10 +5148,36 @@ class KernelSsaGraphBuilder extends ir.Visitor<void> with ir.VisitorVoidMixin {
       stack.add(graph.addConstantNull(closedWorld));
       return;
     }
+
+    StaticType expressionType = _getStaticType(node.expression);
+    FunctionType functionType = expressionType.type.withoutNullability;
+    bool typeArgumentsNeeded = _rtiNeed.methodNeedsTypeArguments(target);
+
+    List<DartType> typeArguments = node.typeArguments
+        .map((type) => typeArgumentsNeeded
+            ? _elementMap.getDartType(type)
+            : _commonElements.dynamicType)
+        .toList();
+    registry.registerGenericInstantiation(
+        GenericInstantiation(functionType, typeArguments));
+
+    // TODO(sra): Add instantiations to SourceInformationBuilder.
+    SourceInformation sourceInformation = null;
+
+    // TODO(47484): Allow callee to have different calling convention for type
+    // arguments.
+    if (typeArgumentsNeeded) {
+      _addTypeArguments(arguments, typeArguments, sourceInformation);
+    }
+
+    bool targetCanThrow = false; // TODO(sra): Is this true?
+
+    // TODO(sra): Use [_pushStaticInvocation] to allow inlining. We don't now
+    // because inference can't tell that the call has no side-effects.
     HInstruction instruction = HInvokeStatic(
         target, arguments, _abstractValueDomain.functionType, <DartType>[],
         targetCanThrow: targetCanThrow);
-    // TODO(sra): ..sourceInformation = sourceInformation
+    instruction.sourceInformation = sourceInformation;
     instruction.sideEffects
       ..clearAllDependencies()
       ..clearAllSideEffects();

pkg/compiler/test/closure/data/instantiation.dart

@@ -7,7 +7,7 @@
 T id<T>(T t) => t;
 
 method<S>(S s) {
-  /*spec.fields=[S],free=[S]*/
+  /*fields=[S],free=[S]*/
   S Function(S) getId() => id;
   return getId();
 }

pkg/compiler/test/closure/data/instantiation1.dart

@@ -13,8 +13,7 @@ class B<S> {
   /*member: B.method:hasThis*/
   method() {
     return
-        /*spec.fields=[this],free=[this],hasThis*/
-        /*prod.hasThis*/
+        /*fields=[this],free=[this],hasThis*/
         () {
       F<S> c = f;
       return c;

pkg/compiler/test/closure/data/instantiation3.dart

@@ -10,7 +10,7 @@ typedef int F<R>(R a);
 
 method<S>() {
   return
-      /*spec.fields=[S],free=[S]*/
+      /*fields=[S],free=[S]*/
       () {
     F<S> c = f;
     return c;

pkg/compiler/test/rti/data/instantiation1.dart

@@ -11,6 +11,7 @@ int f<T>(T a) => null;
 typedef int F<R>(R a);
 
 /*spec.class: B:explicit=[int* Function(B.S*)*],implicit=[B.S],indirect,needsArgs*/
+/*prod.class: B:needsArgs*/
 class B<S> {
   F<S> c;
 

pkg/compiler/test/rti/data/instantiation3.dart

@@ -11,6 +11,7 @@ int f<T>(T a) => null;
 typedef int F<R>(R a);
 
 /*spec.class: B:direct,explicit=[int* Function(B.S*)*],implicit=[B.S],needsArgs*/
+/*prod.class: B:needsArgs*/
 class B<S> {
   F<S> c;
 

pkg/compiler/test/rti/data/instantiation5.dart

@@ -11,6 +11,7 @@ int f<T>(T a) => null;
 typedef int F<R>(R a);
 
 /*spec.member: method:implicit=[method.S],indirect,needsArgs*/
+/*prod.member: method:needsArgs*/
 method<S>() {
   F<S> c;
 

pkg/compiler/test/rti/data/instantiation7.dart

@@ -21,6 +21,7 @@ typedef int F2<R, P>(R a, P b, P c);
 typedef int F3<R, P, Q>(R a, P b, Q c);
 
 /*spec.member: method:implicit=[method.X,method.Y,method.Z],indirect,needsArgs*/
+/*prod.member: method:needsArgs*/
 method<X, Y, Z>() {
   F1<X> c1;
   F2<X, Y> c2;

pkg/compiler/test/rti/data/instantiation8.dart

@@ -14,6 +14,7 @@ equals(a, b) {
   if (a != b) throw '$a != $b';
 }
 
+/*member: test:needsArgs*/
 test<T>(f) {
   Class<T> Function() g = create;
   equals(f, g);

tests/web/lax_runtime_type_instantiate_to_string_test.dart

@@ -14,7 +14,10 @@ main() {
     // `true` if non-minified.
     // The signature of `id` is not otherwise needed so the instantiation
     // wrapper doesn't have a function type.
-    Expect.equals("Instantiation1<dynamic>", toString);
+    // The type parameter is present since it is required because `==`
+    // distinguishes instantiations of the same generic function with different
+    // types.
+    Expect.equals("Instantiation1<int>", toString);
   }
   print(toString);
 }

tests/web_2/lax_runtime_type_instantiate_to_string_test.dart

@@ -16,7 +16,10 @@ main() {
     // `true` if non-minified.
     // The signature of `id` is not otherwise needed so the instantiation
     // wrapper doesn't have a function type.
-    Expect.equals("Instantiation1<dynamic>", toString);
+    // The type parameter is present since it is required because `==`
+    // distinguishes instantiations of the same generic function with different
+    // types.
+    Expect.equals("Instantiation1<int>", toString);
   }
   print(toString);
 }