[dart:js_interop] Restrict external members from using type parameters that don't extend static interop types

Since external APIs can only use primitives and JS types in static interop,
we should require that all type parameters on static interop APIs extend
another static interop type. This is the minimum required to ensure all
type parameters can be erased to JSValue. This only affects dart:js_interop
users and replaces the previous type parameter static error check.

Change-Id: Ia546874da73c808aa25deb8d54d581db783987df
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/316140
Reviewed-by: Joshua Litt <[email protected]>
Reviewed-by: Johnni Winther <[email protected]>
Commit-Queue: Srujan Gaddam <[email protected]>

Author: srujzs

CHANGELOG.md

@@ -16,6 +16,11 @@
   `ObjectToJSExportedDartObject` and `JSExportedDartObjectToObject` are renamed
   to `ObjectToJSBoxedDartObject` and `JSBoxedDartObjectToObject` in order to
   avoid confusion with `@JSExport`.
+- **Type parameters in external APIs**:
+  Type parameters must now be bound to a static interop type or one of the
+  `dart:js_interop` types like `JSNumber` when used in an external API. This
+  only affects `dart:js_interop` classes and not `package:js` or other forms of
+  JS interop.
 
 ## 3.1.0
 

pkg/_fe_analyzer_shared/lib/src/messages/codes_generated.dart

@@ -8182,18 +8182,18 @@ const MessageCode messageJsInteropOperatorsNotSupported = const MessageCode(
 
 // DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
 const Code<Null>
-    codeJsInteropStaticInteropExternalExtensionMembersWithTypeParameters =
-    messageJsInteropStaticInteropExternalExtensionMembersWithTypeParameters;
+    codeJsInteropStaticInteropExternalMemberWithInvalidTypeParameters =
+    messageJsInteropStaticInteropExternalMemberWithInvalidTypeParameters;
 
 // DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
 const MessageCode
-    messageJsInteropStaticInteropExternalExtensionMembersWithTypeParameters =
+    messageJsInteropStaticInteropExternalMemberWithInvalidTypeParameters =
     const MessageCode(
-        "JsInteropStaticInteropExternalExtensionMembersWithTypeParameters",
+        "JsInteropStaticInteropExternalMemberWithInvalidTypeParameters",
         problemMessage:
-            r"""`@staticInterop` classes cannot have external extension members with type parameters.""",
+            r"""External static interop members can only use type parameters that extend either a static interop type or one of the 'dart:js_interop' types.""",
         correctionMessage:
-            r"""Try using a Dart extension member if you need type parameters instead.""");
+            r"""Try adding a valid bound to the type parameters used in this member.""");
 
 // DO NOT EDIT. THIS FILE IS GENERATED. SEE TOP OF FILE.
 const Code<Null> codeJsInteropStaticInteropGenerativeConstructor =

pkg/_js_interop_checks/lib/js_interop_checks.dart

@@ -22,7 +22,7 @@ import 'package:_fe_analyzer_shared/src/messages/codes.dart'
         messageJsInteropNonExternalMember,
         messageJsInteropOperatorCannotBeRenamed,
         messageJsInteropOperatorsNotSupported,
-        messageJsInteropStaticInteropExternalExtensionMembersWithTypeParameters,
+        messageJsInteropStaticInteropExternalMemberWithInvalidTypeParameters,
         messageJsInteropStaticInteropGenerativeConstructor,
         messageJsInteropStaticInteropParameterInitializersAreIgnored,
         messageJsInteropStaticInteropSyntheticConstructor,
@@ -68,7 +68,7 @@ class JsInteropChecks extends RecursiveVisitor {
   final Map<String, Class> _nativeClasses;
   final JsInteropDiagnosticReporter _reporter;
   final StatefulStaticTypeContext _staticTypeContext;
-  final _TypeParameterVisitor _typeParameterVisitor = _TypeParameterVisitor();
+  late _TypeParameterBoundChecker _typeParameterBoundChecker;
   bool _classHasJSAnnotation = false;
   bool _classHasAnonymousAnnotation = false;
   bool _classHasStaticInteropAnnotation = false;
@@ -151,6 +151,8 @@ class JsInteropChecks extends RecursiveVisitor {
             TypeEnvironment(_coreTypes, hierarchy)) {
     _inlineExtensionIndex =
         InlineExtensionIndex(_coreTypes, _staticTypeContext.typeEnvironment);
+    _typeParameterBoundChecker =
+        _TypeParameterBoundChecker(_inlineExtensionIndex);
   }
 
   /// Verifies given [member] is an external extension member on a static
@@ -385,23 +387,29 @@ class JsInteropChecks extends RecursiveVisitor {
               (hasDartJSInteropAnnotation(node) ||
                   _libraryHasDartJSInteropAnnotation)) {
         _checkNoParamInitializersForStaticInterop(node.function);
-        if (node.isExtensionMember) {
-          final annotatable =
-              _inlineExtensionIndex.getExtensionAnnotatable(node);
+        late Annotatable? annotatable;
+        if (node.isInlineClassMember) {
+          annotatable = _inlineExtensionIndex.getInlineClass(node);
+        } else if (node.isExtensionMember) {
+          annotatable = _inlineExtensionIndex.getExtensionAnnotatable(node);
           if (annotatable != null) {
-            // If a @staticInterop member, check that it uses no type
-            // parameters.
-            if (hasStaticInteropAnnotation(annotatable) &&
-                !isAllowedCustomStaticInteropImplementation(node)) {
-              _checkStaticInteropMemberUsesNoTypeParameters(node);
-            }
             // We do not support external extension members with the 'static'
             // keyword currently.
             if (_inlineExtensionIndex.getExtensionDescriptor(node)!.isStatic) {
               report(
                   messageJsInteropExternalExtensionMemberWithStaticDisallowed);
             }
           }
+        } else {
+          annotatable = node.enclosingClass;
+        }
+        if (!isAllowedCustomStaticInteropImplementation(node)) {
+          if (annotatable == null ||
+              ((hasDartJSInteropAnnotation(annotatable) ||
+                  annotatable is InlineClass))) {
+            // Only restrict type parameters for dart:js_interop.
+            _checkStaticInteropMemberUsesValidTypeParameters(node);
+          }
         }
       }
     }
@@ -839,18 +847,10 @@ class JsInteropChecks extends RecursiveVisitor {
     }
   }
 
-  void _checkStaticInteropMemberUsesNoTypeParameters(Procedure node) {
-    // If the extension has type parameters of its own, it copies those type
-    // parameters to the procedure's type parameters (in the front) as well.
-    // Ignore these for the analysis.
-    final extensionTypeParams =
-        _inlineExtensionIndex.getExtension(node)!.typeParameters;
-    final procedureTypeParams = List.from(node.function.typeParameters);
-    procedureTypeParams.removeRange(0, extensionTypeParams.length);
-    if (procedureTypeParams.isNotEmpty ||
-        _typeParameterVisitor.usesTypeParameters(node)) {
+  void _checkStaticInteropMemberUsesValidTypeParameters(Procedure node) {
+    if (_typeParameterBoundChecker.containsInvalidTypeBound(node)) {
       _reporter.report(
-          messageJsInteropStaticInteropExternalExtensionMembersWithTypeParameters,
+          messageJsInteropStaticInteropExternalMemberWithInvalidTypeParameters,
           node.fileOffset,
           node.name.text.length,
           node.fileUri);
@@ -935,7 +935,8 @@ class JsInteropChecks extends RecursiveVisitor {
             (type is InterfaceType &&
                 hasStaticInteropAnnotation(type.classNode)) ||
             (type is InlineType &&
-                hasDartJSInteropAnnotation(type.inlineClass)))) {
+                _inlineExtensionIndex
+                    .isInteropInlineClass(type.inlineClass)))) {
       _reporter.report(
           templateJsInteropStrictModeViolation.withArguments(type, true),
           node.fileOffset,
@@ -952,19 +953,49 @@ class JsInteropChecks extends RecursiveVisitor {
       _reportIfNotJSType(type, node, node.name, node.location?.file);
 }
 
-/// Visitor used to check if a particular node uses a type parameter type.
-class _TypeParameterVisitor extends RecursiveVisitor {
-  bool _visitedTypeParameterType = false;
+/// Visitor used to check that all usages of type parameter types of an external
+/// static interop member is a valid static interop type.
+class _TypeParameterBoundChecker extends RecursiveVisitor {
+  final InlineExtensionIndex _inlineExtensionIndex;
+
+  _TypeParameterBoundChecker(this._inlineExtensionIndex);
+
+  bool _containsInvalidTypeBound = false;
+
+  bool containsInvalidTypeBound(Procedure node) {
+    _containsInvalidTypeBound = false;
+    final function = node.function;
+    for (final param in function.positionalParameters) {
+      param.accept(this);
+    }
+    function.returnType.accept(this);
+    return _containsInvalidTypeBound;
+  }
+
+  @override
+  void visitInterfaceType(InterfaceType node) {
+    final cls = node.classNode;
+    if (hasStaticInteropAnnotation(cls)) return;
+    super.visitInterfaceType(node);
+  }
 
-  bool usesTypeParameters(Node node) {
-    _visitedTypeParameterType = false;
-    node.accept(this);
-    return _visitedTypeParameterType;
+  @override
+  void visitInlineType(InlineType node) {
+    if (_inlineExtensionIndex.isInteropInlineClass(node.inlineClass)) return;
+    super.visitInlineType(node);
   }
 
   @override
   void visitTypeParameterType(TypeParameterType node) {
-    _visitedTypeParameterType = true;
+    final bound = node.bound;
+    if (bound is InlineType &&
+        !_inlineExtensionIndex.isInteropInlineClass(bound.inlineClass)) {
+      _containsInvalidTypeBound = true;
+    }
+    if (bound is InterfaceType &&
+        !hasStaticInteropAnnotation(bound.classNode)) {
+      _containsInvalidTypeBound = true;
+    }
   }
 }
 

pkg/front_end/messages.status

@@ -619,8 +619,8 @@ JsInteropOperatorsNotSupported/analyzerCode: Fail # Web compiler specific
 JsInteropOperatorsNotSupported/example: Fail # Web compiler specific
 JsInteropObjectLiteralConstructorPositionalParameters/analyzerCode: Fail # Web compiler specific
 JsInteropObjectLiteralConstructorPositionalParameters/example: Fail # Web compiler specific
-JsInteropStaticInteropExternalExtensionMembersWithTypeParameters/analyzerCode: Fail # Web compiler specific
-JsInteropStaticInteropExternalExtensionMembersWithTypeParameters/example: Fail # Web compiler specific
+JsInteropStaticInteropExternalMemberWithInvalidTypeParameters/analyzerCode: Fail # Web compiler specific
+JsInteropStaticInteropExternalMemberWithInvalidTypeParameters/example: Fail # Web compiler specific
 JsInteropStaticInteropGenerativeConstructor/analyzerCode: Fail # Web compiler specific
 JsInteropStaticInteropGenerativeConstructor/example: Fail # Web compiler specific
 JsInteropStaticInteropMockMissingGetterOrSetter/analyzerCode: Fail # Web compiler specific

pkg/front_end/messages.yaml

@@ -5552,9 +5552,9 @@ JsInteropNonStaticWithStaticInteropSupertype:
   problemMessage: "Class '#name' does not have an `@staticInterop` annotation, but has supertype '#name2', which does."
   correctionMessage: "Try marking '#name' as a `@staticInterop` class, or don't inherit '#name2'."
 
-JsInteropStaticInteropExternalExtensionMembersWithTypeParameters:
-  problemMessage: "`@staticInterop` classes cannot have external extension members with type parameters."
-  correctionMessage: "Try using a Dart extension member if you need type parameters instead."
+JsInteropStaticInteropExternalMemberWithInvalidTypeParameters:
+  problemMessage: "External static interop members can only use type parameters that extend either a static interop type or one of the 'dart:js_interop' types."
+  correctionMessage: "Try adding a valid bound to the type parameters used in this member."
 
 JsInteropStaticInteropGenerativeConstructor:
   problemMessage: "`@staticInterop` classes should not contain any generative constructors."

pkg/front_end/testcases/dart2js/inline_class/external.dart

@@ -23,7 +23,9 @@ inline class B {
 
   external A method();
 
-  external T genericMethod<T>(T t);
+  // TODO: Once https://github.com/dart-lang/sdk/issues/53046 is resolved,
+  // uncomment this, the static variant below, and the usage of the members.
+  // external T genericMethod<T extends B>(T t);
 
   external B get getter;
 
@@ -33,7 +35,7 @@ inline class B {
 
   external static A staticMethod();
 
-  external static T staticGenericMethod<T>(T t);
+  // external static T staticGenericMethod<T extends B>(T t);
 
   external static B get staticGetter;
 
@@ -46,13 +48,13 @@ void method(A a) {
   a = b1.field;
   b1.field = a;
   a = b1.method();
-  b2 = b2.genericMethod(b2);
+  // b2 = b2.genericMethod(b2);
   b1 = b2.getter;
   b1.setter = b2;
   a = B.staticField;
   B.staticField = a;
   a = B.staticMethod();
-  b2 = B.staticGenericMethod(b2);
+  // b2 = B.staticGenericMethod(b2);
   b1 = B.staticGetter;
   B.staticSetter = b2;
 }

pkg/front_end/testcases/dart2js/inline_class/external.dart.strong.expect

@@ -22,13 +22,10 @@ inline class B /* declaredRepresentationType = self::A */ {
   set field = self::B|set#field;
   method method = self::B|method;
   tearoff method = self::B|get#method;
-  method genericMethod = self::B|genericMethod;
-  tearoff genericMethod = self::B|get#genericMethod;
   get getter = self::B|get#getter;
   static get staticField = get self::B|staticField;
   static set staticField = set self::B|staticField;
   static method staticMethod = self::B|staticMethod;
-  static method staticGenericMethod = self::B|staticGenericMethod;
   static get staticGetter = get self::B|staticGetter;
   set setter = self::B|set#setter;
   static set staticSetter = set self::B|staticSetter;
@@ -48,15 +45,11 @@ external static inline-class-member method B|set#field(lowered self::A #this, se
 external static inline-class-member method B|method(lowered final self::B #this) → self::A;
 static inline-class-member method B|get#method(lowered final self::B #this) → () → self::A
   return () → self::A => self::B|method(#this);
-external static inline-class-member method B|genericMethod<T extends core::Object? = dynamic>(lowered final self::B #this, self::B|genericMethod::T% t) → self::B|genericMethod::T%;
-static inline-class-member method B|get#genericMethod(lowered final self::B #this) → <T extends core::Object? = dynamic>(T%) → T%
-  return <T extends core::Object? = dynamic>(T% t) → T% => self::B|genericMethod<T%>(#this, t);
 external static inline-class-member method B|get#getter(lowered final self::B #this) → self::B;
 external static inline-class-member method B|set#setter(lowered final self::B #this, self::B b) → void;
 external static inline-class-member get B|staticField() → self::A;
 external static inline-class-member set B|staticField(self::A #externalFieldValue) → void;
 external static inline-class-member method B|staticMethod() → self::A;
-external static inline-class-member method B|staticGenericMethod<T extends core::Object? = dynamic>(self::B|staticGenericMethod::T% t) → self::B|staticGenericMethod::T%;
 external static inline-class-member get B|staticGetter() → self::B;
 external static inline-class-member set B|staticSetter(self::B b) → void;
 static method method(self::A a) → void {
@@ -65,13 +58,11 @@ static method method(self::A a) → void {
   a = self::B|get#field(b1);
   self::B|set#field(b1, a);
   a = self::B|method(b1);
-  b2 = self::B|genericMethod<self::B>(b2, b2);
   b1 = self::B|get#getter(b2);
   self::B|set#setter(b1, b2);
   a = self::B|staticField;
   self::B|staticField = a;
   a = self::B|staticMethod();
-  b2 = self::B|staticGenericMethod<self::B>(b2);
   b1 = self::B|staticGetter;
   self::B|staticSetter = b2;
 }

pkg/front_end/testcases/dart2js/inline_class/external.dart.strong.transformed.expect

@@ -23,13 +23,10 @@ inline class B /* declaredRepresentationType = self::A */ {
   set field = self::B|set#field;
   method method = self::B|method;
   tearoff method = self::B|get#method;
-  method genericMethod = self::B|genericMethod;
-  tearoff genericMethod = self::B|get#genericMethod;
   get getter = self::B|get#getter;
   static get staticField = get self::B|staticField;
   static set staticField = set self::B|staticField;
   static method staticMethod = self::B|staticMethod;
-  static method staticGenericMethod = self::B|staticGenericMethod;
   static get staticGetter = get self::B|staticGetter;
   set setter = self::B|set#setter;
   static set staticSetter = set self::B|staticSetter;
@@ -49,15 +46,11 @@ external static inline-class-member method B|set#field(lowered self::A #this, se
 external static inline-class-member method B|method(lowered final self::B #this) → self::A;
 static inline-class-member method B|get#method(lowered final self::B #this) → () → self::A
   return () → self::A => js_2::_callMethodUnchecked0<self::A>(#this, "method");
-external static inline-class-member method B|genericMethod<T extends core::Object? = dynamic>(lowered final self::B #this, self::B|genericMethod::T% t) → self::B|genericMethod::T%;
-static inline-class-member method B|get#genericMethod(lowered final self::B #this) → <T extends core::Object? = dynamic>(T%) → T%
-  return <T extends core::Object? = dynamic>(T% t) → T% => js_2::callMethod<T%>(#this, "genericMethod", <dynamic>[t]);
 external static inline-class-member method B|get#getter(lowered final self::B #this) → self::B;
 external static inline-class-member method B|set#setter(lowered final self::B #this, self::B b) → void;
 external static inline-class-member get B|staticField() → self::A;
 external static inline-class-member set B|staticField(self::A #externalFieldValue) → void;
 external static inline-class-member method B|staticMethod() → self::A;
-external static inline-class-member method B|staticGenericMethod<T extends core::Object? = dynamic>(self::B|staticGenericMethod::T% t) → self::B|staticGenericMethod::T%;
 external static inline-class-member get B|staticGetter() → self::B;
 external static inline-class-member set B|staticSetter(self::B b) → void;
 static method method(self::A a) → void {
@@ -66,13 +59,11 @@ static method method(self::A a) → void {
   a = js_2::getProperty<self::A>(b1, "field");
   js_2::setProperty<self::A>(b1, "field", a);
   a = js_2::_callMethodUnchecked0<self::A>(b1, "method");
-  b2 = js_2::callMethod<self::B>(b2, "genericMethod", <dynamic>[b2]);
   b1 = js_2::getProperty<self::B>(b2, "getter");
   js_2::setProperty<self::B>(b1, "setter", b2);
   a = js_2::getProperty<self::A>(js_2::_getPropertyTrustType<core::Object>(js_2::globalThis, "B"), "staticField");
   js_2::setProperty<self::A>(js_2::_getPropertyTrustType<core::Object>(js_2::globalThis, "B"), "staticField", a);
   a = js_2::_callMethodUnchecked0<self::A>(js_2::_getPropertyTrustType<core::Object>(js_2::globalThis, "B"), "staticMethod");
-  b2 = js_2::callMethod<self::B>(js_2::_getPropertyTrustType<core::Object>(js_2::globalThis, "B"), "staticGenericMethod", <dynamic>[b2]);
   b1 = js_2::getProperty<self::B>(js_2::_getPropertyTrustType<core::Object>(js_2::globalThis, "B"), "staticGetter");
   js_2::setProperty<self::B>(js_2::_getPropertyTrustType<core::Object>(js_2::globalThis, "B"), "staticSetter", b2);
 }

pkg/front_end/testcases/dart2js/inline_class/external.dart.textual_outline.expect

@@ -1,27 +1,21 @@
 @JS()
 library static_interop;
-
 import 'dart:js_interop';
-
 @JS()
 @staticInterop
 class A {}
-
 @JS()
 inline class B {
   final A a;
   external B(A a);
   external B.named(int i);
   external A field;
   external A method();
-  external T genericMethod<T>(T t);
   external B get getter;
   external void set setter(B b);
   external static A staticField;
   external static A staticMethod();
-  external static T staticGenericMethod<T>(T t);
   external static B get staticGetter;
   external static void set staticSetter(B b);
 }
-
 void method(A a) {}

pkg/front_end/testcases/dart2js/inline_class/external.dart.textual_outline_modelled.expect

@@ -14,11 +14,11 @@ inline class B {
   external B(A a);
   external B.named(int i);
   external B get getter;
-  external T genericMethod<T>(T t);
+  external T genericMethod<T extends B>(T t);
   external static A staticField;
   external static A staticMethod();
   external static B get staticGetter;
-  external static T staticGenericMethod<T>(T t);
+  external static T staticGenericMethod<T extends B>(T t);
   external static void set staticSetter(B b);
   external void set setter(B b);
   final A a;