[vm/ffi] Fix direct interaction with Pointer<Native>

This CL changes the static checks and runtime behavior of Pointer<T>.store<T>(T v) and T Pointer<T>.load() behavior to be consistent with the way Dart handles instances methods with generics. For more details see the issue.

Fixes: #37254

Change-Id: Ifcf89646f8e357d8592c38bb340942d522dac941
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/107404
Commit-Queue: Daco Harkes <[email protected]>
Reviewed-by: Martin Kustermann <[email protected]>

Author: dcharkes

pkg/vm/lib/transformations/ffi_use_sites.dart

@@ -192,17 +192,20 @@ class _FfiUseSiteTransformer extends FfiTransformer {
   }
 
   void _ensureNativeTypeToDartType(
-      DartType nativeType, DartType dartType, Expression node) {
-    DartType shouldBeDartType = convertNativeTypeToDartType(nativeType);
-    if (dartType != shouldBeDartType) {
-      diagnosticReporter.report(
-          templateFfiTypeMismatch.withArguments(
-              dartType, shouldBeDartType, nativeType),
-          node.fileOffset,
-          1,
-          node.location.file);
-      throw _FfiStaticTypeError();
-    }
+      DartType containerTypeArg, DartType elementType, Expression node) {
+    final DartType shouldBeElementType =
+        convertNativeTypeToDartType(containerTypeArg);
+    if (elementType == shouldBeElementType) return;
+    // Both subtypes and implicit downcasts are allowed statically.
+    if (env.isSubtypeOf(shouldBeElementType, elementType)) return;
+    if (env.isSubtypeOf(elementType, shouldBeElementType)) return;
+    diagnosticReporter.report(
+        templateFfiTypeMismatch.withArguments(
+            elementType, shouldBeElementType, containerTypeArg),
+        node.fileOffset,
+        1,
+        node.location.file);
+    throw _FfiStaticTypeError();
   }
 
   void _ensureNativeTypeValid(DartType nativeType, Expression node) {

runtime/lib/ffi.cc

@@ -26,14 +26,6 @@ namespace dart {
 // Some checks are only performed at runtime to allow for generic code, these
 // throw ArgumentExceptions.
 
-static void ThrowTypeArgumentError(const AbstractType& type_arg,
-                                   const char* expected) {
-  const String& error = String::Handle(String::NewFormatted(
-      "Type argument (%s) should be a %s",
-      String::Handle(type_arg.UserVisibleName()).ToCString(), expected));
-  Exceptions::ThrowArgumentError(error);
-}
-
 static bool IsPointerType(const AbstractType& type) {
   // Do a fast check for predefined types.
   classid_t type_cid = type.type_class_id();
@@ -54,38 +46,15 @@ static bool IsPointerType(const AbstractType& type) {
   return type.IsSubtypeOf(pointer_type, Heap::kNew);
 }
 
-static bool IsConcreteNativeType(const AbstractType& type) {
-  // Do a fast check for predefined types.
-  classid_t type_cid = type.type_class_id();
-  if (RawObject::IsFfiNativeTypeTypeClassId(type_cid)) {
-    return false;
-  }
-  if (RawObject::IsFfiTypeClassId(type_cid)) {
-    return true;
-  }
-
-  // Do a slow check for subtyping.
-  const Class& native_type_class = Class::Handle(
-      Isolate::Current()->object_store()->ffi_native_type_class());
-  AbstractType& native_type_type =
-      AbstractType::Handle(native_type_class.DeclarationType());
-  return type.IsSubtypeOf(native_type_type, Heap::kNew);
-}
-
-static void CheckIsConcreteNativeType(const AbstractType& type) {
-  if (!IsConcreteNativeType(type)) {
-    ThrowTypeArgumentError(type, "concrete sub type of NativeType");
-  }
-}
-
 static bool IsNativeFunction(const AbstractType& type_arg) {
   classid_t type_cid = type_arg.type_class_id();
   return RawObject::IsFfiTypeNativeFunctionClassId(type_cid);
 }
 
 static void CheckSized(const AbstractType& type_arg) {
-  classid_t type_cid = type_arg.type_class_id();
-  if (RawObject::IsFfiTypeVoidClassId(type_cid) ||
+  const classid_t type_cid = type_arg.type_class_id();
+  if (RawObject::IsFfiNativeTypeTypeClassId(type_cid) ||
+      RawObject::IsFfiTypeVoidClassId(type_cid) ||
       RawObject::IsFfiTypeNativeFunctionClassId(type_cid)) {
     const String& error = String::Handle(String::NewFormatted(
         "%s does not have a predefined size (@unsized). "
@@ -98,6 +67,8 @@ static void CheckSized(const AbstractType& type_arg) {
   }
 }
 
+enum class FfiVariance { kInvariant = 0, kCovariant = 1, kContravariant = 2 };
+
 // Checks that a dart type correspond to a [NativeType].
 // Because this is checked already in a kernel transformation, it does not throw
 // an ArgumentException but a boolean which should be asserted.
@@ -118,7 +89,8 @@ static void CheckSized(const AbstractType& type_arg) {
 // [NativeFunction]<T1 Function(T2, T3) -> S1 Function(S2, S3)
 //    where DartRepresentationOf(Tn) -> Sn
 static bool DartAndCTypeCorrespond(const AbstractType& native_type,
-                                   const AbstractType& dart_type) {
+                                   const AbstractType& dart_type,
+                                   FfiVariance variance) {
   classid_t native_type_cid = native_type.type_class_id();
   if (RawObject::IsFfiTypeIntClassId(native_type_cid)) {
     return dart_type.IsSubtypeOf(AbstractType::Handle(Type::IntType()),
@@ -129,7 +101,13 @@ static bool DartAndCTypeCorrespond(const AbstractType& native_type,
                                  Heap::kNew);
   }
   if (RawObject::IsFfiPointerClassId(native_type_cid)) {
-    return native_type.Equals(dart_type) || dart_type.IsNullType();
+    return (variance == FfiVariance::kInvariant &&
+            dart_type.Equals(native_type)) ||
+           (variance == FfiVariance::kCovariant &&
+            dart_type.IsSubtypeOf(native_type, Heap::kNew)) ||
+           (variance == FfiVariance::kContravariant &&
+            native_type.IsSubtypeOf(dart_type, Heap::kNew)) ||
+           dart_type.IsNullType();
   }
   if (RawObject::IsFfiTypeNativeFunctionClassId(native_type_cid)) {
     if (!dart_type.IsFunctionType()) {
@@ -161,20 +139,34 @@ static bool DartAndCTypeCorrespond(const AbstractType& native_type,
     }
     if (!DartAndCTypeCorrespond(
             AbstractType::Handle(nativefunction_function.result_type()),
-            AbstractType::Handle(dart_function.result_type()))) {
+            AbstractType::Handle(dart_function.result_type()), variance)) {
       return false;
     }
     for (intptr_t i = 0; i < dart_function.NumParameters(); i++) {
       if (!DartAndCTypeCorrespond(
               AbstractType::Handle(nativefunction_function.ParameterTypeAt(i)),
-              AbstractType::Handle(dart_function.ParameterTypeAt(i)))) {
+              AbstractType::Handle(dart_function.ParameterTypeAt(i)),
+              variance)) {
         return false;
       }
     }
   }
   return true;
 }
 
+static void CheckDartAndCTypeCorrespond(const AbstractType& native_type,
+                                        const AbstractType& dart_type,
+                                        FfiVariance variance) {
+  if (!DartAndCTypeCorrespond(native_type, dart_type, variance)) {
+    const String& error = String::Handle(String::NewFormatted(
+        "Expected type '%s' to be different, it should be "
+        "DartRepresentationOf('%s').",
+        String::Handle(dart_type.UserVisibleName()).ToCString(),
+        String::Handle(native_type.UserVisibleName()).ToCString()));
+    Exceptions::ThrowArgumentError(error);
+  }
+}
+
 // The following functions are runtime checks on arguments.
 
 // Note that expected_from and expected_to are inclusive.
@@ -224,7 +216,6 @@ DEFINE_NATIVE_ENTRY(Ffi_allocate, 1, 1) {
   // Pointer. https://github.com/dart-lang/sdk/issues/35782
   GET_NATIVE_TYPE_ARGUMENT(type_arg, arguments->NativeTypeArgAt(0));
 
-  CheckIsConcreteNativeType(type_arg);
   CheckSized(type_arg);
 
   GET_NON_NULL_NATIVE_ARGUMENT(Integer, argCount, arguments->NativeArgAt(0));
@@ -251,7 +242,6 @@ DEFINE_NATIVE_ENTRY(Ffi_fromAddress, 1, 1) {
   TypeArguments& type_args = TypeArguments::Handle(type_arg.arguments());
   AbstractType& native_type = AbstractType::Handle(
       type_args.TypeAtNullSafe(Pointer::kNativeTypeArgPos));
-  CheckIsConcreteNativeType(native_type);
   GET_NON_NULL_NATIVE_ARGUMENT(Integer, arg_ptr, arguments->NativeArgAt(0));
 
   // TODO(dacoharkes): should this return NULL if address is 0?
@@ -298,7 +288,6 @@ DEFINE_NATIVE_ENTRY(Ffi_cast, 1, 1) {
   TypeArguments& type_args = TypeArguments::Handle(type_arg.arguments());
   AbstractType& native_type = AbstractType::Handle(
       type_args.TypeAtNullSafe(Pointer::kNativeTypeArgPos));
-  CheckIsConcreteNativeType(native_type);
 
   const Integer& address = Integer::Handle(zone, pointer.GetCMemoryAddress());
   RawPointer* result =
@@ -379,7 +368,8 @@ DEFINE_NATIVE_ENTRY(Ffi_load, 1, 1) {
   AbstractType& pointer_type_arg =
       AbstractType::Handle(pointer.type_argument());
   CheckSized(pointer_type_arg);
-  ASSERT(DartAndCTypeCorrespond(pointer_type_arg, type_arg));
+  CheckDartAndCTypeCorrespond(pointer_type_arg, type_arg,
+                              FfiVariance::kContravariant);
 
   uint8_t* address = reinterpret_cast<uint8_t*>(
       Integer::Handle(pointer.GetCMemoryAddress()).AsInt64Value());
@@ -460,7 +450,8 @@ DEFINE_NATIVE_ENTRY(Ffi_store, 0, 2) {
   AbstractType& pointer_type_arg =
       AbstractType::Handle(pointer.type_argument());
   CheckSized(pointer_type_arg);
-  ASSERT(DartAndCTypeCorrespond(pointer_type_arg, arg_type));
+  CheckDartAndCTypeCorrespond(pointer_type_arg, arg_type,
+                              FfiVariance::kCovariant);
 
   classid_t type_cid = pointer_type_arg.type_class_id();
   StoreValue(zone, pointer, type_cid, new_value);
@@ -469,7 +460,6 @@ DEFINE_NATIVE_ENTRY(Ffi_store, 0, 2) {
 
 DEFINE_NATIVE_ENTRY(Ffi_sizeOf, 1, 0) {
   GET_NATIVE_TYPE_ARGUMENT(type_arg, arguments->NativeTypeArgAt(0));
-  CheckIsConcreteNativeType(type_arg);
   CheckSized(type_arg);
 
   classid_t type_cid = type_arg.type_class_id();
@@ -523,7 +513,8 @@ DEFINE_NATIVE_ENTRY(Ffi_asFunction, 1, 1) {
       AbstractType::Handle(pointer.type_argument());
   ASSERT(IsNativeFunction(pointer_type_arg));
   GET_NATIVE_TYPE_ARGUMENT(type_arg, arguments->NativeTypeArgAt(0));
-  ASSERT(DartAndCTypeCorrespond(pointer_type_arg, type_arg));
+  CheckDartAndCTypeCorrespond(pointer_type_arg, type_arg,
+                              FfiVariance::kInvariant);
 
   Function& dart_signature = Function::Handle(Type::Cast(type_arg).signature());
   TypeArguments& nativefunction_type_args =

tests/ffi/data_test.dart

@@ -125,9 +125,7 @@ void testCastGeneric2() {
 
 void testCastNativeType() {
   ffi.Pointer<ffi.Int64> p = ffi.allocate();
-  Expect.throws(() {
-    p.cast<ffi.Pointer>();
-  });
+  p.cast<ffi.Pointer>();
   p.free();
 }
 

tests/ffi/ffi.status

@@ -2,8 +2,6 @@
 # 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.
 
-regress_37254_test:Skip # dartbug.com/37254
-
 [ $arch == simdbc ]
 *: Skip # FFI not yet supported on SimDBC32: dartbug.com/36809
 

tests/ffi/regress_37254_test.dart

@@ -83,7 +83,6 @@ void store2() {
 
   // Successful implicit downcast of argument at runtime.
   // Should succeed now, should statically be rejected when NNBD lands.
-  // TODO(37254): Currently rejected by the frontend.
   a.store(b);
 
   a.free();
@@ -92,12 +91,10 @@ void store2() {
 
 void store3() {
   final Pointer<Pointer<Int8>> a = allocate<Pointer<Int8>>();
-  // TODO(37254): We currently disallow obtaining a Pointer<NativeType>.
   final Pointer<NativeType> b = allocate<Int8>().cast<Pointer<NativeType>>();
 
   // Failing implicit downcast of argument at runtime.
   // Should fail now at runtime, should statically be rejected when NNBD lands.
-  // TODO(37254): Currently rejected by the frontend.
   Expect.throws(() {
     a.store(b);
   });
@@ -111,7 +108,6 @@ void store4() {
       Pointer<Int8>>(); // Reified as Pointer<Pointer<Int8>> at runtime.
   final Pointer<Int8> b = allocate<Int8>();
 
-  // TODO(37254): Currently rejected by the frontend.
   a.store(b);
 
   a.free();
@@ -133,7 +129,6 @@ void store5() {
 void store6() {
   final Pointer<Pointer<NativeType>> a = allocate<
       Pointer<Int8>>(); // Reified as Pointer<Pointer<Int8>> at runtime.
-  // TODO(37254): We currently disallow obtaining a Pointer<NativeType>.
   final Pointer<NativeType> b = allocate<Int8>().cast<Pointer<NativeType>>();
 
   // Fails on type check of argument.
@@ -149,7 +144,6 @@ void store7() {
   final Pointer<Pointer<NativeType>> a = allocate<Pointer<NativeType>>();
   final Pointer<Int8> b = allocate<Int8>();
 
-  // TODO(37254): Currently rejected by the frontend.
   a.store(b);
 
   a.free();
@@ -161,7 +155,6 @@ void store8() {
   final Pointer<NativeType> b =
       allocate<Int8>(); // Reified as Pointer<Int8> at runtime.
 
-  // TODO(37254): Currently hits assertion in VM.
   a.store(b);
 
   a.free();
@@ -170,7 +163,6 @@ void store8() {
 
 void store9() {
   final Pointer<Pointer<NativeType>> a = allocate<Pointer<NativeType>>();
-  // TODO(37254): We currently disallow obtaining a Pointer<NativeType>.
   final Pointer<NativeType> b = allocate<Int8>().cast<Pointer<NativeType>>();
 
   a.store(b);
@@ -203,7 +195,6 @@ void load3() {
   final Pointer<Pointer<NativeType>> a = allocate<
       Pointer<Int8>>(); // Reified as Pointer<Pointer<Int8>> at runtime.
 
-  // TODO(37254): Currently hits assertion in VM.
   Pointer<Int8> b = a.load<Pointer<NativeType>>();
   Expect.type<Pointer<Int8>>(b);
 
@@ -215,7 +206,6 @@ void load4() {
       Pointer<Int8>>(); // Reified as Pointer<Pointer<Int8>> at runtime.
 
   // Return value runtime type is Pointer<Int8>.
-  // TODO(37254): Currently hits assertion in VM.
   Pointer<NativeType> b = a.load();
   Expect.type<Pointer<Int8>>(b);