Reland "[ddc] Don't emit type args on interop invocations"

This is a reland of commit ebc96ecb83

Angular codegen emits members that are external so DDC's external
checks need to be more specific to only focus on JS interop. This
also modifies some code logic around whether a member is JS interop
to be less permissive. Before, it claimed that any member is JS
interop if it's external and the enclosing library has a @JS
annotation, but this is only true if there is no surrounding class.

Original change's description:
> [ddc] Don't emit type args on interop invocations
>
> DDC passes type args as the first args for a generic function
> invocation, but interop functions should be excluded from doing
> so, as this leads to passing type args to JS functions.
>
> Change-Id: Ia011beca8c7a0ebb3a7e6c81cd960096045c3a06
> Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/323941
> Reviewed-by: Nicholas Shahan <nshahan@google.com>
> Commit-Queue: Srujan Gaddam <srujzs@google.com>

Change-Id: Id48337ac4980d9de5de03d895d93cad3ab3a1808
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/324542
Reviewed-by: Nicholas Shahan <nshahan@google.com>
Commit-Queue: Srujan Gaddam <srujzs@google.com>

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

@@ -4327,9 +4327,11 @@ class ProgramCompiler extends ComputeOnceConstantVisitor<js_ast.Expression>
 
   bool _reifyGenericFunction(Member? m) =>
       m == null ||
-      !m.enclosingLibrary.importUri.isScheme('dart') ||
-      !m.annotations
-          .any((a) => isBuiltinAnnotation(a, '_js_helper', 'NoReifyGeneric'));
+      // JS interop members should not pass type arguments.
+      !isJsMember(m) &&
+          !(m.enclosingLibrary.importUri.isScheme('dart') &&
+              m.annotations.any((a) =>
+                  isBuiltinAnnotation(a, '_js_helper', 'NoReifyGeneric')));
 
   js_ast.Statement _nullParameterCheck(js_ast.Expression param) {
     var call = runtimeCall('argumentError((#))', [param]);

pkg/dev_compiler/lib/src/kernel/js_interop.dart

@@ -47,11 +47,13 @@ bool isAllowInterop(Expression node) {
 }
 
 bool isJsMember(Member member) {
-  // TODO(vsm): If we ever use external outside the SDK for non-JS interop,
-  // we're need to fix this.
-  return !_isLibrary(member.enclosingLibrary, ['dart:*']) &&
-      member.isExternal &&
-      !isNative(member);
+  return member.isExternal &&
+      !_isLibrary(member.enclosingLibrary, ['dart:*']) &&
+      !isNative(member) &&
+      // Non-JS interop external members might exist e.g. the ones generated by
+      // Angular, so we should check to see that this member actually uses JS
+      // interop.
+      usesJSInterop(member);
 }
 
 bool _annotationIsFromJSLibrary(String expectedName, Expression value) {
@@ -120,7 +122,7 @@ bool isStaticInteropType(Class namedClass) {
 bool isUndefinedAnnotation(Expression value) =>
     isBuiltinAnnotation(value, '_js_helper', '_Undefined');
 
-/// Returns true iff the class has an `@JS(...)` annotation from
+/// Returns true iff the annotatable has an `@JS(...)` annotation from
 /// `package:js`, `dart:_js_annotations`, or `dart:js_interop`.
 ///
 /// Note: usually [usesJSInterop] should be used instead of this.
@@ -132,8 +134,11 @@ bool isUndefinedAnnotation(Expression value) =>
 // class itself, other places we require it on the library. Also members are
 // inconsistent: sometimes they need to have `@JS` on them, other times they
 // need to be `external` in an `@JS` class.
-bool hasJSInteropAnnotation(Class c) =>
-    c.annotations.any(isJSInteropAnnotation);
+//
+// TODO(srujzs): We should replace many of these helpers with the ones defined
+// in pkg:_js_interop_checks.
+bool hasJSInteropAnnotation(Annotatable a) =>
+    a.annotations.any(isJSInteropAnnotation);
 
 /// Returns true iff this element is a JS interop member.
 ///
@@ -144,9 +149,10 @@ bool hasJSInteropAnnotation(Class c) =>
 /// the class or library.
 bool usesJSInterop(NamedNode n) {
   if (n is Member && n.isExternal) {
-    return n.enclosingLibrary.annotations.any(isJSInteropAnnotation) ||
-        n.annotations.any(isJSInteropAnnotation) ||
-        (n.enclosingClass?.annotations.any(isJSInteropAnnotation) ?? false);
+    return hasJSInteropAnnotation(n) ||
+        (n.enclosingClass == null
+            ? hasJSInteropAnnotation(n.enclosingLibrary)
+            : hasJSInteropAnnotation(n.enclosingClass!));
   } else if (n is Class) {
     return n.annotations.any(isJSInteropAnnotation);
   }

tests/lib/js/type_parameter_lowering_test.dart

@@ -0,0 +1,50 @@
+// Copyright (c) 2023, 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.
+
+// Check that the compilers lower interop calls that use type parameters do not
+// pass the type parameter.
+
+@JS()
+library type_parameter_lowering_test;
+
+import 'package:js/js.dart';
+
+@JS()
+external void eval(String code);
+
+@JS()
+external void topLevelMethod<T extends int>(T t);
+
+@JS()
+class TypeParam<T extends int> {
+  external TypeParam(T t);
+  external static void staticMethod<U extends int>(U u);
+  external static void staticMethodShadow<T extends int>(T t);
+  external void genericMethod<U extends int>(U u);
+  external void genericMethodShadow<T extends int>(T t);
+}
+
+void main() {
+  eval('''
+    const checkValue = function(value) {
+      if (value != 0) {
+        throw new Error(`Expected value to be 0, but got \${value}.`);
+      }
+    }
+    globalThis.topLevelMethod = checkValue;
+    globalThis.TypeParam = function (value) {
+      checkValue(value);
+      this.genericMethod = checkValue;
+      this.genericMethodShadow = checkValue;
+    }
+    globalThis.TypeParam.staticMethod = checkValue;
+    globalThis.TypeParam.staticMethodShadow = checkValue;
+  ''');
+  topLevelMethod(0);
+  final typeParam = TypeParam(0);
+  TypeParam.staticMethod(0);
+  TypeParam.staticMethodShadow(0);
+  typeParam.genericMethod(0);
+  typeParam.genericMethodShadow(0);
+}

tests/lib/lib.status

@@ -86,6 +86,7 @@ js/static_interop_test/jsobject_type_test: SkipByDesign # Issue 42085. CSP polic
 js/static_interop_test/native_error_test: SkipByDesign # Issue 42085. CSP policy disallows injected JS code
 js/static_interop_test/typed_data_test: SkipByDesign # Issue 42085. CSP policy disallows injected JS code
 js/trust_types_test: SkipByDesign # Issue 42085. CSP policy disallows injected JS code
+js/type_parameter_lowering_test: SkipByDesign # Issue 42085. CSP policy disallows injected JS code
 
 [ $simulator ]
 convert/utf85_test: Skip # Pass, Slow Issue 20111.

tests/lib_2/js/type_parameter_lowering_test.dart

@@ -0,0 +1,50 @@
+// Copyright (c) 2023, 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.
+
+// Check that the compilers lower interop calls that use type parameters do not
+// pass the type parameter.
+
+@JS()
+library type_parameter_lowering_test;
+
+import 'package:js/js.dart';
+
+@JS()
+external void eval(String code);
+
+@JS()
+external void topLevelMethod<T extends int>(T t);
+
+@JS()
+class TypeParam<T extends int> {
+  external TypeParam(T t);
+  external static void staticMethod<U extends int>(U u);
+  external static void staticMethodShadow<T extends int>(T t);
+  external void genericMethod<U extends int>(U u);
+  external void genericMethodShadow<T extends int>(T t);
+}
+
+void main() {
+  eval('''
+    const checkValue = function(value) {
+      if (value != 0) {
+        throw new Error(`Expected value to be 0, but got \${value}.`);
+      }
+    }
+    globalThis.topLevelMethod = checkValue;
+    globalThis.TypeParam = function (value) {
+      checkValue(value);
+      this.genericMethod = checkValue;
+      this.genericMethodShadow = checkValue;
+    }
+    globalThis.TypeParam.staticMethod = checkValue;
+    globalThis.TypeParam.staticMethodShadow = checkValue;
+  ''');
+  topLevelMethod(0);
+  final typeParam = TypeParam(0);
+  TypeParam.staticMethod(0);
+  TypeParam.staticMethodShadow(0);
+  typeParam.genericMethod(0);
+  typeParam.genericMethodShadow(0);
+}

tests/lib_2/lib_2.status

@@ -67,6 +67,7 @@ js/static_interop_test/external_static_member_lowerings_test: SkipByDesign # Iss
 js/static_interop_test/external_static_member_lowerings_trusttypes_test: SkipByDesign # Issue 42085. CSP policy disallows injected JS code
 js/static_interop_test/external_static_member_lowerings_with_namespaces_test: SkipByDesign # Issue 42085. CSP policy disallows injected JS code
 js/trust_types_test: SkipByDesign # Issue 42085. CSP policy disallows injected JS code
+js/type_parameter_lowering_test: SkipByDesign # Issue 42085. CSP policy disallows injected JS code
 
 [ $simulator ]
 convert/utf85_test: Skip # Pass, Slow Issue 20111.