[ffi] Account for position variance in converting native types

This CL introduces variance-dependent treatment of the `Handle` native
type when converting native types to Dart types. Since `Handle` can
represent any object, it should match any type. To achive that in
cases when `Handle` appears as the subtype in the subtype checks, it's
converted to `Never` in covariant positions and to `Object?` in
covariant ones.

TEST=existing

Issue #49518

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

Author: chloestefantsova

pkg/analyzer/lib/src/generated/ffi_verifier.dart

@@ -729,7 +729,6 @@ class FfiVerifier extends RecursiveAstVisitor<void> {
       dartType,
       nativeType,
       nativeFieldWrappersAsPointer: true,
-      permissiveReturnType: true,
     )) {
       _errorReporter.atToken(
         errorToken,
@@ -1296,14 +1295,11 @@ class FfiVerifier extends RecursiveAstVisitor<void> {
 
   /// Validates that the given [nativeType] is, when native types are converted
   /// to their Dart equivalent, a subtype of [dartType].
-  /// [permissiveReturnType] means that the [direction] is ignored for return
-  /// types, and subtyping is allowed in either direction.
   bool _validateCompatibleFunctionTypes(
     _FfiTypeCheckDirection direction,
     TypeImpl dartType,
     TypeImpl nativeType, {
     bool nativeFieldWrappersAsPointer = false,
-    bool permissiveReturnType = false,
   }) {
     // We require both to be valid function types.
     if (dartType is! FunctionTypeImpl ||
@@ -1334,22 +1330,7 @@ class FfiVerifier extends RecursiveAstVisitor<void> {
     }
 
     // Validate that the return types are compatible.
-    if (permissiveReturnType) {
-      // TODO(dacoharkes): Fix inconsistency between `FfiNative` and
-      // `asFunction`. http://dartbug.com/49518.
-      if (!(_validateCompatibleNativeType(
-            _FfiTypeCheckDirection.nativeToDart,
-            dartType.returnType,
-            nativeType.returnType,
-          ) ||
-          _validateCompatibleNativeType(
-            _FfiTypeCheckDirection.dartToNative,
-            dartType.returnType,
-            nativeType.returnType,
-          ))) {
-        return false;
-      }
-    } else if (!_validateCompatibleNativeType(
+    if (!_validateCompatibleNativeType(
       direction,
       dartType.returnType,
       nativeType.returnType,
@@ -1401,13 +1382,8 @@ class FfiVerifier extends RecursiveAstVisitor<void> {
       // Don't allow other native subtypes if the Dart return type is void.
       return nativeReturnType == _PrimitiveDartType.void_;
     } else if (nativeReturnType == _PrimitiveDartType.handle) {
-      switch (direction) {
-        case _FfiTypeCheckDirection.dartToNative:
-          // Everything is a subtype of `Object?`.
-          return true;
-        case _FfiTypeCheckDirection.nativeToDart:
-          return typeSystem.isSubtypeOf(typeSystem.objectNone, dartType);
-      }
+      // `Handle` matches against any type in positions of any variance.
+      return true;
     } else if (dartType is InterfaceTypeImpl &&
         nativeType is InterfaceTypeImpl) {
       if (nativeFieldWrappersAsPointer &&

pkg/vm/lib/modular/transformations/ffi/common.dart

@@ -172,6 +172,23 @@ enum FfiTypeCheckDirection {
   dartToNative,
 }
 
+/// Converting mode of `Handle` in [convertNativeTypeToDartType].
+enum HandleToDartTypeConversionMode {
+  /// The variance-based conversion mode, where `Handle` is converted to
+  /// `Never` if it appears in covariant positions of the type being converted,
+  /// or to `Object?` when it appears in contravariant positions of the type.
+  /// This mode is used when the type being converted appears as the subtype of
+  /// a type check.
+  varianceBasedBehaviorForSubtype,
+
+  /// The variance-based conversion mode, where `Handle` is converted to
+  /// `Object?` if it appears in covariant positions of the type being
+  /// converted, or to `Never` when it appears in contravariant positions of
+  /// the type. This mode is used when the type being converted appears as the
+  /// supertype of a type check.
+  varianceBasedBehaviorForSupertype,
+}
+
 /// [FfiTransformer] contains logic which is shared between
 /// _FfiUseSiteTransformer and _FfiDefinitionTransformer.
 class FfiTransformer extends Transformer {
@@ -1080,16 +1097,38 @@ class FfiTransformer extends Transformer {
   /// [Pointer]<T>                         -> [Pointer]<T>
   /// T extends [Struct]                   -> T
   /// T extends [Union]                    -> T
-  /// [Handle]                             -> [Object]
+  /// [Handle]                             -> [Object?]|[Never]
   /// [NativeFunction]<T1 Function(T2, T3) -> S1 Function(S2, S3)
   ///    where DartRepresentationOf(Tn) -> Sn
   /// ```
+  ///
+  /// If [mode] is [HandleToDartTypeConversionMode.oldNativeBehavior], `Handle`
+  /// is converted to `Object?`.  Otherwise, `Handle` is converted to either
+  /// `Object?` or `Never`. Since `Handle` represents any possible Dart object,
+  /// it should match any type. To achieve that, in the subtype checks where
+  /// `Handle` appears as the subtype (as indicated by [mode] being
+  /// [HandleToDartTypeConversionMode.varianceBasedBehaviorForSubtype]), it
+  /// should be converted depending on the variance of its position: `Never` for
+  /// covariant positions, and `Object?` for contravariant ones. If `Handle`
+  /// appears as the supertype in the check (as indicated by [mode] being
+  /// [HandleToDartTypeConversionMode.varianceBasedBehaviorForSupertype]), the
+  /// treatment of covariant and contravariant positions is reversed. For more
+  /// details, see https://github.com/dart-lang/sdk/issues/49518.
+  ///
+  /// [isCovariant] is the flag that helps compute the variance of the position
+  /// in [nativeType]. It's updated in the recursive invocations of
+  /// [convertNativeTypeToDartType] and should be omitted in regular call sites.
+  /// [isCovariant] is ignored if [mode] is
+  /// [HandleToDartTypeConversionMode.oldNativeBehavior].
   DartType? convertNativeTypeToDartType(
     DartType nativeType, {
     bool allowStructAndUnion = false,
     bool allowHandle = false,
     bool allowInlineArray = false,
     bool allowVoid = false,
+    HandleToDartTypeConversionMode mode =
+        HandleToDartTypeConversionMode.varianceBasedBehaviorForSubtype,
+    Variance variance = Variance.invariant,
   }) {
     if (nativeType is! InterfaceType) {
       return null;
@@ -1146,7 +1185,22 @@ class FfiTransformer extends Transformer {
       return VoidType();
     }
     if (nativeType_ == NativeType.kHandle && allowHandle) {
-      return coreTypes.objectNonNullableRawType;
+      switch (mode) {
+        case HandleToDartTypeConversionMode.varianceBasedBehaviorForSubtype:
+          return switch (variance) {
+            Variance.contravariant => coreTypes.objectNullableRawType,
+            Variance.covariant ||
+            Variance.invariant ||
+            Variance.unrelated => const NeverType.nonNullable(),
+          };
+        case HandleToDartTypeConversionMode.varianceBasedBehaviorForSupertype:
+          return switch (variance) {
+            Variance.contravariant => const NeverType.nonNullable(),
+            Variance.covariant ||
+            Variance.invariant ||
+            Variance.unrelated => coreTypes.objectNullableRawType,
+          };
+      }
     }
     if (nativeType_ != NativeType.kNativeFunction ||
         native.typeArguments[0] is! FunctionType) {
@@ -1165,6 +1219,8 @@ class FfiTransformer extends Transformer {
       allowStructAndUnion: true,
       allowHandle: true,
       allowVoid: true,
+      mode: mode,
+      variance: variance,
     );
     if (returnType == null) return null;
     final argumentTypes = <DartType>[];
@@ -1174,6 +1230,8 @@ class FfiTransformer extends Transformer {
               paramDartType,
               allowStructAndUnion: true,
               allowHandle: true,
+              mode: mode,
+              variance: variance.combine(Variance.contravariant),
             ) ??
             dummyDartType,
       );
@@ -1856,17 +1914,21 @@ class FfiTransformer extends Transformer {
     bool allowVoid = false,
     bool allowArray = false,
   }) {
-    final DartType correspondingDartType =
-        convertNativeTypeToDartType(
-          nativeType,
-          allowStructAndUnion: true,
-          allowHandle: allowHandle,
-          allowInlineArray: allowArray,
-          allowVoid: allowVoid,
-        )!;
-    if (dartType == correspondingDartType) return correspondingDartType;
+    final DartType correspondingDartType;
     switch (direction) {
       case FfiTypeCheckDirection.nativeToDart:
+        correspondingDartType =
+            convertNativeTypeToDartType(
+              nativeType,
+              allowStructAndUnion: true,
+              allowHandle: allowHandle,
+              allowInlineArray: allowArray,
+              allowVoid: allowVoid,
+              mode:
+                  HandleToDartTypeConversionMode
+                      .varianceBasedBehaviorForSubtype,
+              variance: Variance.covariant,
+            )!;
         if (env.isSubtypeOf(
           correspondingDartType,
           dartType,
@@ -1888,6 +1950,18 @@ class FfiTransformer extends Transformer {
           }
         }
       case FfiTypeCheckDirection.dartToNative:
+        correspondingDartType =
+            convertNativeTypeToDartType(
+              nativeType,
+              allowStructAndUnion: true,
+              allowHandle: allowHandle,
+              allowInlineArray: allowArray,
+              allowVoid: allowVoid,
+              mode:
+                  HandleToDartTypeConversionMode
+                      .varianceBasedBehaviorForSupertype,
+              variance: Variance.covariant,
+            )!;
         if (env.isSubtypeOf(
           dartType,
           correspondingDartType,

pkg/vm/lib/modular/transformations/ffi/definitions.dart

@@ -530,7 +530,15 @@ class _FfiDefinitionTransformer extends FfiTransformer {
           allowStructAndUnion: true,
           allowHandle: false,
         );
+
+        // Since fields induce both setters and getters, the type checks should
+        // be made both ways.
         if (shouldBeDartType == null ||
+            !env.isSubtypeOf(
+              shouldBeDartType,
+              type,
+              SubtypeCheckMode.ignoringNullabilities,
+            ) ||
             !env.isSubtypeOf(
               type,
               shouldBeDartType,

runtime/bin/ffi_test/ffi_test_functions_vmspecific.cc

@@ -989,6 +989,13 @@ DART_EXPORT Dart_Handle PassObjectToC(Dart_Handle h) {
   return return_value;
 }
 
+DART_EXPORT Dart_Handle
+CallClosureWithArgumentViaHandle(Dart_Handle (*callback)(Dart_Handle),
+                                 Dart_Handle argument) {
+  printf("CallClosureWithArgumentViaHandle %p %p\n", callback, argument);
+  return callback(argument);
+}
+
 DART_EXPORT void ClosureCallbackThroughHandle(void (*callback)(Dart_Handle),
                                               Dart_Handle closureHandle) {
   printf("ClosureCallbackThroughHandle %p %p\n", callback, closureHandle);

tests/ffi/static_checks/vmspecific_static_checks_test.dart

@@ -926,13 +926,8 @@ void testHandleVariance() {
   testLibrary.lookupFunction<Handle Function(Handle), Object Function(MyClass)>(
       "PassObjectToC");
 
-  // Requiring a more specific return type is not, this requires a cast from
-  // the user.
+  // Returning a more specific type than Handle is okay too: it will be checked at runtime.
   testLibrary.lookupFunction<Handle Function(Handle), MyClass Function(Object)>(
-      //                                              ^^^^^^^^^^^^^^^^^^^^^^^^
-      // [analyzer] COMPILE_TIME_ERROR.MUST_BE_A_SUBTYPE
-      //      ^
-      // [cfe] Expected type 'MyClass Function(Object)' to be 'Object Function(Object)', which is the Dart type corresponding to 'NativeFunction<Handle Function(Handle)>'.
       "PassObjectToC");
 }
 
@@ -1451,7 +1446,7 @@ void testReturnVoidNotVoid() {
   // Taking a more specific argument is okay.
   testLibrary.lookupFunction<Handle Function(), void Function()>("unused");
   //          ^
-  // [cfe] Expected type 'void Function()' to be 'Object Function()', which is the Dart type corresponding to 'NativeFunction<Handle Function()>'.
+  // [cfe] Expected type 'void Function()' to be 'Never Function()', which is the Dart type corresponding to 'NativeFunction<Handle Function()>'.
   //                                            ^^^^^^^^^^^^^^^
   // [analyzer] COMPILE_TIME_ERROR.MUST_BE_A_SUBTYPE
 }

tests/ffi/vmspecific_ffi_native_handles_test.dart

@@ -7,6 +7,8 @@
 import 'dart:ffi';
 import 'dart:typed_data';
 
+import 'package:expect/expect.dart';
+
 import 'dylib_utils.dart';
 
 final ffiTestFunctions = dlopenPlatformSpecific("ffi_test_functions");
@@ -17,8 +19,94 @@ void main() {
   testMixed();
   testManyHandlesAllDifferent();
   testManyHandlesAllSame();
+  testCallbackHandleCases();
+}
+
+void testHandleInReturn() {
+  final string = 'foo';
+  Expect.identical(string, stringFromString(string));
+  Expect.identical(string, stringFromObject(string));
+  Expect.identical(42, intFromInt(42));
+  Expect.identical(42, intFromObject(42));
+  // TODO(https://dartbug.com/49518): Uncomment the lines below when the
+  // runtime checks are added.
+  //
+  // Expect.throws(() => stringFromObject(Object()));
+  // Expect.throws(() => stringFromObject(42));
+  // Expect.throws(() => intFromObject(Object()));
+  // Expect.throws(() => intFromObject(string));
 }
 
+@Native<Handle Function(Handle)>(symbol: "PassObjectToC")
+external String stringFromString(String arg);
+
+@Native<Handle Function(Handle)>(symbol: "PassObjectToC")
+external String stringFromObject(Object arg);
+
+@Native<Handle Function(Handle)>(symbol: "PassObjectToC")
+external int intFromInt(int arg);
+
+@Native<Handle Function(Handle)>(symbol: "PassObjectToC")
+external int intFromObject(Object arg);
+
+void testCallbackHandleCases() {
+  final o = Object();
+  final s = 'foo';
+
+  Object returnObject(Object obj) => obj;
+  final nc0 = NativeCallable<Handle Function(Handle)>.isolateLocal(returnObject)
+    ..keepIsolateAlive = false;
+  Expect.identical(o, callClosureWithArgumentViaHandle(nc0.nativeFunction, o));
+  Expect.identical(s, callClosureWithArgumentViaHandle(nc0.nativeFunction, s));
+  Expect.identical(
+    42,
+    callClosureWithArgumentViaHandle(nc0.nativeFunction, 42),
+  );
+  // TODO(https://dartbug.com/49518): Uncomment the lines below when the
+  // runtime checks are added.
+  //
+  // Expect.throws(
+  //   () => callClosureWithArgumentViaHandle(nc0.nativeFunction, null),
+  // );
+
+  int returnInt(int obj) => obj;
+  final nc1 = NativeCallable<Handle Function(Handle)>.isolateLocal(returnInt)
+    ..keepIsolateAlive = false;
+  Expect.identical(
+    42,
+    callClosureWithArgumentViaHandle(nc1.nativeFunction, 42),
+  );
+  // TODO(https://dartbug.com/49518): Uncomment the lines below when the
+  // runtime checks are added.
+  //
+  // Expect.throws(() => callClosureWithArgumentViaHandle(nc1.nativeFunction, o));
+  // Expect.throws(() => callClosureWithArgumentViaHandle(nc1.nativeFunction, s));
+  // Expect.throws(
+  //   () => callClosureWithArgumentViaHandle(nc1.nativeFunction, null),
+  // );
+
+  String returnString(String obj) => obj;
+  final nc2 = NativeCallable<Handle Function(Handle)>.isolateLocal(returnString)
+    ..keepIsolateAlive = false;
+  Expect.identical(s, callClosureWithArgumentViaHandle(nc2.nativeFunction, s));
+  // TODO(https://dartbug.com/49518): Uncomment the lines below when the
+  // runtime checks are added.
+  //
+  // Expect.throws(() => callClosureWithArgumentViaHandle(nc2.nativeFunction, o));
+  // Expect.throws(() => callClosureWithArgumentViaHandle(nc2.nativeFunction, 42));
+  // Expect.throws(
+  //   () => callClosureWithArgumentViaHandle(nc2.nativeFunction, null),
+  // );
+}
+
+@Native<
+  Handle Function(Pointer<NativeFunction<Handle Function(Handle)>>, Handle)
+>(symbol: "CallClosureWithArgumentViaHandle")
+external Object? callClosureWithArgumentViaHandle(
+  Pointer<NativeFunction<Handle Function(Handle)>> callback,
+  Object? argument,
+);
+
 void testMixed() {
   callUpdateNode(
     id: 42,