[dart:js_interop] Make isA<JSBoxedDartObject> check if it's a boxed object

Fixes #60644

Previously, we made no special case for JSBoxedDartObjects, so isA
would always return true for any object (as JSBoxedDartObject contains
an @js('Object') annotation). This is not obvious however, and it's
much more useful to check that the value is a result of a previous
`toJSBox` call. Documentation is updated/cleaned up to make note of
the various exceptions in `isA`.

CoreLibraryReviewExempt: Web-only library.
Change-Id: Ibd8873d3f862f4c950101f5f049327a1aa5c7bf2
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/445522
Commit-Queue: Srujan Gaddam <[email protected]>
Reviewed-by: Ömer Ağacan <[email protected]>
Reviewed-by: Leaf Petersen <[email protected]>

Author: srujzs

CHANGELOG.md

@@ -75,6 +75,8 @@ To learn more about the feature, check out the
 - `JSArray.add` is added to avoid cases where during migration from `List` to
   `JSArray`, `JSAnyOperatorExtension.add` is accidentally used. See [#59830][]
   for more details.
+- `isA<JSBoxedDartObject>` now checks that the value was the result of a
+  `toJSBox` operation instead of returning true for all objects.
 
 [#59830]: https://github.com/dart-lang/sdk/issues/59830
 

pkg/_js_interop_checks/lib/src/transformations/shared_interop_transformer.dart

@@ -41,6 +41,7 @@ class SharedInteropTransformer extends Transformer {
   late StaticInvocation? _invocation;
   final Procedure _isA;
   final Procedure _isATearoff;
+  final Procedure _isJSBoxedDartObject;
   final ExtensionTypeDeclaration _jsAny;
   final ExtensionTypeDeclaration _jsFunction;
   final ExtensionTypeDeclaration _jsObject;
@@ -100,6 +101,8 @@ class SharedInteropTransformer extends Transformer {
         'dart:js_interop',
         'JSAnyUtilityExtension|${LibraryIndex.tearoffPrefix}isA',
       ),
+      _isJSBoxedDartObject = _typeEnvironment.coreTypes.index
+          .getTopLevelProcedure('dart:js_interop', '_isJSBoxedDartObject'),
       _jsAny = _typeEnvironment.coreTypes.index.getExtensionType(
         'dart:js_interop',
         'JSAny',
@@ -549,20 +552,30 @@ class SharedInteropTransformer extends Transformer {
         typeofString = 'symbol';
         break;
       case 'JSTypedArray' when interopTypeDecl == jsType:
-        // Only do this special case when users are referring directly to
-        // the `dart:js_interop` type and not some wrapper.
+        // Only do this special case when users are referring directly to the
+        // `dart:js_interop` type and not some wrapper.
 
         // `TypedArray` doesn't exist as a property in JS, but rather as a
         // superclass of all typed arrays. In order to do the most sensible
-        // thing here, we can use the prototype of some typed array, and
-        // check that the receiver is an `instanceof` that prototype.
-        // See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray#description
+        // thing here, we can use the prototype of some typed array, and check
+        // that the receiver is an `instanceof` that prototype. See
+        // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray#description
         // for more details.
         check = StaticInvocation(
           _instanceof,
           Arguments([VariableGet(any), getInt8ArrayPrototype()]),
         );
         break;
+      case 'JSBoxedDartObject' when interopTypeDecl == jsType:
+        // Only do this special case when users are referring directly to the
+        // `dart:js_interop` type and not some wrapper.
+
+        // Check whether the given value is the result of a previous call to
+        // `toJSBox`.
+        check = StaticInvocation(
+          _isJSBoxedDartObject,
+          Arguments([VariableGet(any)]),
+        );
       default:
         for (final descriptor in interopTypeDecl.memberDescriptors) {
           final descriptorNode = descriptor.memberReference!.node;

sdk/lib/_internal/js_shared/lib/js_interop_patch.dart

@@ -99,14 +99,23 @@ final Object _jsBoxedDartObjectProperty = foreign_helper.JS(
   'Symbol("jsBoxedDartObjectProperty")',
 );
 
+// Returns the value of the property we embed in every `JSBoxedDartObject` in
+// `any`.
+Object? _getJSBoxedDartObjectPropertyValue(JSAny any) =>
+    js_util.getProperty<Object?>(any, _jsBoxedDartObjectProperty);
+
+// Used in the `isA` transform.
+bool _isJSBoxedDartObject(JSAny any) =>
+    _getJSBoxedDartObjectPropertyValue(any) != null;
+
 // -----------------------------------------------------------------------------
 // JSBoxedDartObject <-> Object
 @patch
 extension JSBoxedDartObjectToObject on JSBoxedDartObject {
   @patch
   @pragma('dart2js:prefer-inline')
   Object get toDart {
-    final val = js_util.getProperty<Object?>(this, _jsBoxedDartObjectProperty);
+    final val = _getJSBoxedDartObjectPropertyValue(this);
     if (val == null) {
       throw 'Expected a wrapped Dart object, but got a JS object or a wrapped '
           'Dart object from a separate runtime instead.';

sdk/lib/_internal/wasm/lib/js_interop_patch.dart

@@ -119,17 +119,26 @@ extension FunctionToJSExportedDartFunction on Function {
 final WasmExternRef? _jsBoxedDartObjectPropertyExternRef = js_helper
     .JS<WasmExternRef?>('() => Symbol("jsBoxedDartObjectProperty")');
 
+// Returns the value of the property we embed in every `JSBoxedDartObject` in
+// `any`.
+WasmExternRef? _getJSBoxedDartObjectPropertyValue(JSAny any) =>
+    js_helper.JS<WasmExternRef?>(
+      '(o,s) => o[s]',
+      any.toExternRef,
+      _jsBoxedDartObjectPropertyExternRef,
+    );
+
+// Used in the `isA` transform.
+bool _isJSBoxedDartObject(JSAny any) =>
+    !isDartNull(_getJSBoxedDartObjectPropertyValue(any));
+
 // -----------------------------------------------------------------------------
 // JSBoxedDartObject <-> Object
 @patch
 extension JSBoxedDartObjectToObject on JSBoxedDartObject {
   @patch
   Object get toDart {
-    final val = js_helper.JS<WasmExternRef?>(
-      '(o,s) => o[s]',
-      this.toExternRef,
-      _jsBoxedDartObjectPropertyExternRef,
-    );
+    final val = _getJSBoxedDartObjectPropertyValue(this);
     if (isDartNull(val)) {
       throw 'Expected a wrapped Dart object, but got a JS object or a wrapped '
           'Dart object from a separate runtime instead.';

sdk/lib/js_interop/js_interop.dart

@@ -62,8 +62,8 @@ export 'dart:js_util' show NullRejectionException;
 /// - Renaming external declarations by parameterizing the annotation on the
 ///   member with [name].
 ///
-/// In the case where [name] is not specified, is `null`, or is empty, the Dart
-/// name of the extension type or external declaration is used as the default.
+/// In the case where [name] is not specified, the Dart name of the extension
+/// type or external declaration is used as the default.
 ///
 /// See https://dart.dev/interop/js-interop/usage#js for more details on how to
 /// use this annotation.
@@ -567,17 +567,19 @@ extension JSAnyUtilityExtension on JSAny? {
   /// this does an `instanceOfString('JSClass')` check and not an
   /// `instanceOfString('Array')` check.
   ///
-  /// There are two exceptions to this rule. The first exception is
-  /// `JSTypedArray`. As `TypedArray` does not exist as a property in
-  /// JavaScript, this does some prototype checking to make `isA<JSTypedArray>`
-  /// do the right thing. The other exception is `JSAny`. If you do a
-  /// `isA<JSAny>` check, it will only do a `null` check.
-  ///
-  /// Using this method with a [T] that has an object literal constructor will
-  /// result in an error as you likely want to use [JSObject] instead.
-  ///
-  /// Using this method with a [T] that wraps a primitive JS type will result in
-  /// an error telling you to use the primitive JS type instead.
+  /// There are a few values for [T] that are exceptions to this rule:
+  /// - `JSTypedArray`: As `TypedArray` does not exist as a class in JavaScript,
+  ///   this does some prototype checking to make `isA<JSTypedArray>` do the
+  ///   right thing.
+  /// - `JSBoxedDartObject`: `isA<JSBoxedDartObject>` will check if the value is
+  ///   a result of a previous [ObjectToJSBoxedDartObject.toJSBox] call.
+  /// - `JSAny`: If you do an `isA<JSAny>` check, it will only check for `null`.
+  /// - User interop types whose representation types are JS primitive types:
+  ///   This will result in an error to avoid confusion on whether the user
+  ///   interop type is used in the type-check. Use the primitive JS type as the
+  ///   value for [T] instead.
+  /// - User interop types that have an object literal constructor: This will
+  ///   result in an error as you likely want to use [JSObject] instead.
   @Since('3.4')
   external bool isA<T extends JSAny?>();
 

tests/lib/js/static_interop_test/isa/functional_test.dart

@@ -32,6 +32,15 @@ extension type ArraySubtype._(JSArray _) implements JSObject {
   external ArraySubtype();
 }
 
+extension type WrapJSBoxedDartObject(JSBoxedDartObject _)
+    implements JSBoxedDartObject {}
+
+@JS('WrapJSBoxedDartObject.prototype')
+external JSObject get wrapJSBoxedDartObjectPrototype;
+
+@JS('Object.setPrototypeOf')
+external void setPrototypeOf(JSObject obj, JSObject prototype);
+
 @JS()
 external JSBigInt BigInt(String val);
 
@@ -61,9 +70,11 @@ void main() {
   Expect.isTrue(nil.isA<JSString?>());
   Expect.isTrue(nil.isA<JSObject?>());
   Expect.isTrue(nil.isA<JSAny?>());
+  Expect.isTrue(nil.isA<JSBoxedDartObject?>());
   Expect.isFalse(nil.isA<JSString>());
   Expect.isFalse(nil.isA<JSObject>());
   Expect.isFalse(nil.isA<JSAny>());
+  Expect.isFalse(nil.isA<JSBoxedDartObject>());
 
   // Test inheritance chain, nullability, and one false case.
 
@@ -102,16 +113,22 @@ void main() {
   final jsObject = ObjectLiteral(a: 0);
   testIsJSObject(jsObject);
   Expect.isFalse(jsObject.isA<JSBoolean>());
-  // Note that this is true even though it's a supertype - we can't
-  // differentiate between the two types. This is okay because users will get a
-  // consistent error when trying to internalize it.
-  Expect.isTrue(jsObject.isA<JSBoxedDartObject>());
 
   final jsBox = ''.toJSBox;
   Expect.isTrue(jsBox.isA<JSBoxedDartObject>());
   Expect.isTrue(jsBox.isA<JSBoxedDartObject?>());
   testIsJSObject(jsBox);
   Expect.isFalse(jsBox.isA<JSString>());
+  // While `JSBoxedDartObject`s are just `Object`s, we do additional checking to
+  // make sure `isA` checks this actually is a `JSBoxedDartObject`.
+  Expect.isFalse(jsObject.isA<JSBoxedDartObject>());
+  Expect.isFalse(JSArray().isA<JSBoxedDartObject>());
+  // Check that property access on primitives isn't invalid.
+  Expect.isFalse(jsNum.isA<JSBoxedDartObject>());
+  Expect.isFalse(jsBool.isA<JSBoxedDartObject>());
+  Expect.isFalse(jsString.isA<JSBoxedDartObject>());
+  Expect.isFalse(jsBigInt.isA<JSBoxedDartObject>());
+  Expect.isFalse(jsSymbol.isA<JSBoxedDartObject>());
 
   final jsArray = <JSAny?>[].toJS;
   Expect.isTrue(jsArray.isA<JSArray>());
@@ -220,6 +237,8 @@ void main() {
   eval('''
     class SubtypeArray extends Array {}
     globalThis.SubtypeArray = SubtypeArray;
+    class WrapJSBoxedDartObject {}
+    globalThis.WrapJSBoxedDartObject = WrapJSBoxedDartObject;
   ''');
 
   final customJsAny = CustomJSAny(jsArray);
@@ -252,6 +271,15 @@ void main() {
   Expect.isTrue(customTypedArray.isA<JSTypedArray>());
   Expect.isTrue(customTypedArray.isA<JSTypedArray?>());
 
+  final wrapJsBox = WrapJSBoxedDartObject(jsBox);
+  Expect.isFalse(wrapJsBox.isA<WrapJSBoxedDartObject>());
+  Expect.isFalse(wrapJsBox.isA<WrapJSBoxedDartObject?>());
+  Expect.isTrue(nil.isA<WrapJSBoxedDartObject?>());
+  Expect.isFalse(nil.isA<WrapJSBoxedDartObject>());
+  setPrototypeOf(wrapJsBox, wrapJSBoxedDartObjectPrototype);
+  Expect.isTrue(wrapJsBox.isA<WrapJSBoxedDartObject>());
+  Expect.isTrue(wrapJsBox.isA<WrapJSBoxedDartObject?>());
+
   final arraySubtype = ArraySubtype();
   Expect.isTrue(arraySubtype.isA<ArraySubtype>());
   Expect.isTrue(arraySubtype.isA<ArraySubtype?>());