[vm/ffi] Change asFunction and lookFunction to extension methods

This prevents them from being called dynamically.
Moreover, it prevents asFunction from being called on a non-NativeFunction type argument, simplifying the amount of manual checks.

Note that this CL had to change the CFE and analzyer, and their tests (including mock_sdk) as well.

This can potentially be a breaking change, as the extension methods are only visible when `dart:ffi` is imported, while methods on objects are always visible.

Issue: #35903

Change-Id: I1e291f154228d5d9a34b21a022088bf493f6557d
Cq-Include-Trybots: luci.dart.try:vm-ffi-android-debug-arm-try,vm-ffi-android-debug-arm64-try,app-kernel-linux-debug-x64-try,vm-kernel-linux-debug-ia32-try,vm-kernel-win-debug-x64-try,vm-kernel-win-debug-ia32-try,vm-kernel-precomp-linux-debug-x64-try,vm-dartkb-linux-release-x64-abi-try,vm-kernel-precomp-android-release-arm64-try,vm-kernel-asan-linux-release-x64-try,vm-kernel-linux-release-simarm-try,vm-kernel-linux-release-simarm64-try,vm-kernel-precomp-android-release-arm_x64-try,vm-kernel-precomp-obfuscate-linux-release-x64-try,dart-sdk-linux-try,analyzer-analysis-server-linux-try,analyzer-linux-release-try,front-end-linux-release-x64-try,vm-kernel-precomp-win-release-x64-try,vm-kernel-mac-debug-x64-try,analyzer-nnbd-linux-release-try,dart2js-nnbd-linux-x64-chrome-try,ddc-nnbd-linux-release-chrome-try,front-end-nnbd-linux-release-x64-try,vm-kernel-nnbd-linux-debug-x64-try,vm-kernel-nnbd-linux-release-x64-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/135463
Commit-Queue: Daco Harkes <[email protected]>
Reviewed-by: Martin Kustermann <[email protected]>

Author: dcharkes

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

@@ -126,12 +126,17 @@ class FfiVerifier extends RecursiveAstVisitor<void> {
       Element enclosingElement = element.enclosingElement;
       if (enclosingElement is ClassElement) {
         if (_isPointer(enclosingElement)) {
+          if (element.name == 'fromFunction') {
+            _validateFromFunction(node, element);
+          }
+        }
+      }
+      if (enclosingElement is ExtensionElement) {
+        if (_isNativeFunctionPointerExtension(enclosingElement)) {
           if (element.name == 'asFunction') {
             _validateAsFunction(node, element);
-          } else if (element.name == 'fromFunction') {
-            _validateFromFunction(node, element);
           }
-        } else if (_isDynamicLibrary(enclosingElement) &&
+        } else if (_isDynamicLibraryExtension(enclosingElement) &&
             element.name == 'lookupFunction') {
           _validateLookupFunction(node);
         }
@@ -152,10 +157,10 @@ class FfiVerifier extends RecursiveAstVisitor<void> {
     return element is ClassElement && element.library.name == 'dart.ffi';
   }
 
-  /// Return `true` if the given [element] represents the class
-  /// `DynamicLibrary`.
-  bool _isDynamicLibrary(ClassElement element) =>
-      element.name == 'DynamicLibrary' && element.library.name == 'dart.ffi';
+  /// Return `true` if the given [element] represents the extension
+  /// `LibraryExtension`.
+  bool _isDynamicLibraryExtension(Element element) =>
+      element.name == 'LibraryExtension' && element.library.name == 'dart.ffi';
 
   /// Returns `true` iff [nativeType] is a `ffi.NativeFunction<???>` type.
   bool _isNativeFunctionInterfaceType(DartType nativeType) {
@@ -184,6 +189,10 @@ class FfiVerifier extends RecursiveAstVisitor<void> {
   bool _isPointer(Element element) =>
       element.name == 'Pointer' && element.library.name == 'dart.ffi';
 
+  bool _isNativeFunctionPointerExtension(Element element) =>
+      element.name == 'NativeFunctionPointer' &&
+      element.library.name == 'dart.ffi';
+
   /// Returns `true` iff [nativeType] is a `ffi.Pointer<???>` type.
   bool _isPointerInterfaceType(DartType nativeType) {
     if (nativeType is InterfaceType) {

pkg/analyzer/lib/src/test_utilities/mock_sdk.dart

@@ -532,13 +532,19 @@ class Pointer<T extends NativeType> extends NativeType {
   static Pointer<NativeFunction<T>> fromFunction<T extends Function>(
       @DartRepresentationOf("T") Function f,
       [Object exceptionalReturn]);
-  R asFunction<@DartRepresentationOf("T") R extends Function>();
+}
+extension NativeFunctionPointer<NF extends Function>
+    on Pointer<NativeFunction<NF>> {
+  external DF asFunction<DF extends Function>();
 }
 class Struct extends NativeType {}
 
-abstract class DynamicLibrary {
-  F lookupFunction<T extends Function, F extends Function>(String symbolName);
+abstract class DynamicLibrary {}
+extension LibraryExtension on DynamicLibrary {
+  external F lookupFunction<T extends Function, F extends Function>(
+      String symbolName);
 }
+
 abstract class NativeFunction<T extends Function> extends NativeType {}
 
 class DartRepresentationOf {

pkg/analyzer/test/src/diagnostics/non_native_function_type_argument_to_pointer_test.dart

@@ -3,6 +3,7 @@
 // BSD-style license that can be found in the LICENSE file.
 
 import 'package:analyzer/src/dart/error/ffi_code.dart';
+import 'package:analyzer/src/error/codes.dart';
 import 'package:test_reflective_loader/test_reflective_loader.dart';
 
 import '../dart/resolution/driver_resolution.dart';
@@ -25,7 +26,9 @@ class C {
   }
 }
 ''', [
-      error(FfiCode.NON_NATIVE_FUNCTION_TYPE_ARGUMENT_TO_POINTER, 109, 1),
+      // This changed from a method to a extension method, uses Dart semantics
+      // instead of manual check now.
+      error(StaticTypeWarningCode.UNDEFINED_METHOD, 98, 10),
     ]);
   }
 

pkg/analyzer/test/src/diagnostics/subtype_of_ffi_class_test.dart

@@ -77,9 +77,6 @@ class C extends Int8 {}
 import 'dart:ffi';
 class C extends Pointer {}
 ''', [
-      // TODO(brianwilkerson) The following diagnostic should not be generated.
-      error(StaticWarningCode.NON_ABSTRACT_CLASS_INHERITS_ABSTRACT_MEMBER_ONE,
-          25, 1),
       error(FfiCode.SUBTYPE_OF_FFI_CLASS_IN_EXTENDS, 35, 7),
     ]);
   }
@@ -198,9 +195,6 @@ class C implements Int8 {}
 import 'dart:ffi';
 class C implements Pointer {}
 ''', [
-      // TODO(brianwilkerson) The following diagnostic should not be generated.
-      error(StaticWarningCode.NON_ABSTRACT_CLASS_INHERITS_ABSTRACT_MEMBER_ONE,
-          25, 1),
       error(FfiCode.SUBTYPE_OF_FFI_CLASS_IN_IMPLEMENTS, 38, 7),
     ]);
   }
@@ -333,9 +327,6 @@ class C with Int8 {}
 import 'dart:ffi';
 class C with Pointer {}
 ''', [
-      // TODO(brianwilkerson) The following diagnostic should not be generated.
-      error(StaticWarningCode.NON_ABSTRACT_CLASS_INHERITS_ABSTRACT_MEMBER_ONE,
-          25, 1),
       error(CompileTimeErrorCode.MIXIN_INHERITS_FROM_NOT_OBJECT, 32, 7),
       error(FfiCode.SUBTYPE_OF_FFI_CLASS_IN_WITH, 32, 7),
     ]);

pkg/vm/lib/transformations/ffi.dart

@@ -234,11 +234,12 @@ class FfiTransformer extends Transformer {
         addressOfField = index.getMember('dart:ffi', 'Struct', '_addressOf'),
         structFromPointer =
             index.getMember('dart:ffi', 'Struct', '_fromPointer'),
-        asFunctionMethod = index.getMember('dart:ffi', 'Pointer', 'asFunction'),
+        asFunctionMethod =
+            index.getMember('dart:ffi', 'NativeFunctionPointer', 'asFunction'),
         asFunctionInternal =
             index.getTopLevelMember('dart:ffi', '_asFunctionInternal'),
         lookupFunctionMethod =
-            index.getMember('dart:ffi', 'DynamicLibrary', 'lookupFunction'),
+            index.getMember('dart:ffi', 'LibraryExtension', 'lookupFunction'),
         fromFunctionMethod =
             index.getMember('dart:ffi', 'Pointer', 'fromFunction'),
         libraryLookupMethod =

pkg/vm/lib/transformations/ffi_use_sites.dart

@@ -153,7 +153,30 @@ class _FfiUseSiteTransformer extends FfiTransformer {
 
     final Member target = node.target;
     try {
-      if (target == fromFunctionMethod) {
+      if (target == lookupFunctionMethod && !isFfiLibrary) {
+        final DartType nativeType = InterfaceType(
+            nativeFunctionClass, Nullability.legacy, [node.arguments.types[0]]);
+        final DartType dartType = node.arguments.types[1];
+
+        _ensureNativeTypeValid(nativeType, node);
+        _ensureNativeTypeToDartType(nativeType, dartType, node);
+        return _replaceLookupFunction(node);
+      } else if (target == asFunctionMethod && !isFfiLibrary) {
+        final DartType dartType = node.arguments.types[1];
+        final DartType nativeType = InterfaceType(
+            nativeFunctionClass, Nullability.legacy, [node.arguments.types[0]]);
+
+        _ensureNativeTypeValid(nativeType, node);
+        _ensureNativeTypeToDartType(nativeType, dartType, node);
+
+        final DartType nativeSignature =
+            (nativeType as InterfaceType).typeArguments[0];
+        // Inline function body to make all type arguments instatiated.
+        return StaticInvocation(
+            asFunctionInternal,
+            Arguments([node.arguments.positional[0]],
+                types: [dartType, nativeSignature]));
+      } else if (target == fromFunctionMethod) {
         final DartType nativeType = InterfaceType(
             nativeFunctionClass, Nullability.legacy, [node.arguments.types[0]]);
         final Expression func = node.arguments.positional[0];
@@ -246,15 +269,10 @@ class _FfiUseSiteTransformer extends FfiTransformer {
   // We need to replace calls to 'DynamicLibrary.lookupFunction' with explicit
   // Kernel, because we cannot have a generic call to 'asFunction' in its body.
   //
-  // Below, in 'visitMethodInvocation', we ensure that the type arguments to
+  // Above, in 'visitStaticInvocation', we ensure that the type arguments to
   // 'lookupFunction' are constants, so by inlining the call to 'asFunction' at
   // the call-site, we ensure that there are no generic calls to 'asFunction'.
-  //
-  // We will not detect dynamic invocations of 'asFunction' and
-  // 'lookupFunction': these are handled by the stubs in 'ffi_patch.dart' and
-  // 'dynamic_library_patch.dart'. Dynamic invocations of 'lookupFunction' (and
-  // 'asFunction') are not legal and throw a runtime exception.
-  Expression _replaceLookupFunction(MethodInvocation node) {
+  Expression _replaceLookupFunction(StaticInvocation node) {
     // The generated code looks like:
     //
     // _asFunctionInternal<DS, NS>(lookup<NativeFunction<NS>>(symbolName))
@@ -263,13 +281,16 @@ class _FfiUseSiteTransformer extends FfiTransformer {
     final DartType dartSignature = node.arguments.types[1];
 
     final Arguments args = Arguments([
-      node.arguments.positional.single
+      node.arguments.positional[1]
     ], types: [
       InterfaceType(nativeFunctionClass, Nullability.legacy, [nativeSignature])
     ]);
 
     final Expression lookupResult = MethodInvocation(
-        node.receiver, Name("lookup"), args, libraryLookupMethod);
+        node.arguments.positional[0],
+        Name("lookup"),
+        args,
+        libraryLookupMethod);
 
     return StaticInvocation(asFunctionInternal,
         Arguments([lookupResult], types: [dartSignature, nativeSignature]));
@@ -318,35 +339,7 @@ class _FfiUseSiteTransformer extends FfiTransformer {
 
     final Member target = node.interfaceTarget;
     try {
-      // We will not detect dynamic invocations of 'asFunction' and
-      // 'lookupFunction' -- these are handled by the 'asFunctionInternal' stub
-      // in 'dynamic_library_patch.dart'. Dynamic invocations of 'asFunction'
-      // and 'lookupFunction' are not legal and throw a runtime exception.
-      if (target == lookupFunctionMethod) {
-        final DartType nativeType = InterfaceType(
-            nativeFunctionClass, Nullability.legacy, [node.arguments.types[0]]);
-        final DartType dartType = node.arguments.types[1];
-
-        _ensureNativeTypeValid(nativeType, node);
-        _ensureNativeTypeToDartType(nativeType, dartType, node);
-        return _replaceLookupFunction(node);
-      } else if (target == asFunctionMethod) {
-        final DartType dartType = node.arguments.types[0];
-        final DartType pointerType =
-            node.receiver.getStaticType(_staticTypeContext);
-        final DartType nativeType = _pointerTypeGetTypeArg(pointerType);
-
-        _ensureNativeTypeValid(pointerType, node);
-        _ensureNativeTypeValid(nativeType, node);
-        _ensureNativeTypeToDartType(nativeType, dartType, node);
-
-        final DartType nativeSignature =
-            (nativeType as InterfaceType).typeArguments[0];
-        return StaticInvocation(asFunctionInternal,
-            Arguments([node.receiver], types: [dartType, nativeSignature]));
-      } else if (target == elementAtMethod) {
-        // TODO(37773): When moving to extension methods we can get rid of
-        // this rewiring.
+      if (target == elementAtMethod) {
         final DartType pointerType =
             node.receiver.getStaticType(_staticTypeContext);
         final DartType nativeType = _pointerTypeGetTypeArg(pointerType);

runtime/vm/compiler/frontend/base_flow_graph_builder.cc

@@ -884,7 +884,8 @@ Fragment BaseFlowGraphBuilder::AllocateObject(TokenPosition position,
 
 Fragment BaseFlowGraphBuilder::BuildFfiAsFunctionInternalCall(
     const TypeArguments& signatures) {
-  ASSERT(signatures.IsInstantiated() && signatures.Length() == 2);
+  ASSERT(signatures.IsInstantiated());
+  ASSERT(signatures.Length() == 2);
 
   const AbstractType& dart_type = AbstractType::Handle(signatures.TypeAt(0));
   const AbstractType& native_type = AbstractType::Handle(signatures.TypeAt(1));

sdk/lib/_internal/vm/lib/ffi_dynamic_library_patch.dart

@@ -32,15 +32,6 @@ class DynamicLibrary {
   Pointer<T> lookup<T extends NativeType>(String symbolName)
       native "Ffi_dl_lookup";
 
-  // The real implementation of this function lives in FfiUseSiteTransformer
-  // for interface calls. Only dynamic calls (which are illegal) reach this
-  // implementation.
-  @patch
-  F lookupFunction<T extends Function, F extends Function>(String symbolName) {
-    throw UnsupportedError(
-        "Dynamic invocation of lookupFunction is not supported.");
-  }
-
   // TODO(dacoharkes): Expose this to users, or extend Pointer?
   // https://github.com/dart-lang/sdk/issues/35881
   int getHandle() native "Ffi_dl_getHandle";
@@ -59,3 +50,10 @@ class DynamicLibrary {
   @patch
   Pointer<Void> get handle => Pointer.fromAddress(getHandle());
 }
+
+extension LibraryExtension on DynamicLibrary {
+  @patch
+  DS lookupFunction<NS extends Function, DS extends Function>(
+          String symbolName) =>
+      throw UnsupportedError("The body is inlined in the frontend.");
+}

sdk/lib/_internal/vm/lib/ffi_patch.dart

@@ -106,11 +106,6 @@ class Pointer<T extends NativeType> {
 
   @patch
   Pointer<U> cast<U extends NativeType>() => Pointer.fromAddress(address);
-
-  @patch
-  R asFunction<R extends Function>() {
-    throw UnsupportedError("Pointer.asFunction cannot be called dynamically.");
-  }
 }
 
 /// Returns an integer encoding the ABI used for size and alignment
@@ -229,6 +224,13 @@ Pointer<Pointer<S>> _elementAtPointer<S extends NativeType>(
         Pointer<Pointer<S>> pointer, int index) =>
     Pointer.fromAddress(pointer.address + _intPtrSize * index);
 
+extension NativeFunctionPointer<NF extends Function>
+    on Pointer<NativeFunction<NF>> {
+  @patch
+  DF asFunction<DF extends Function>() =>
+      throw UnsupportedError("The body is inlined in the frontend.");
+}
+
 //
 // The following code is generated, do not edit by hand.
 //

sdk/lib/ffi/dynamic_library.dart

@@ -30,10 +30,6 @@ class DynamicLibrary {
   /// Throws an [ArgumentError] if it fails to lookup the symbol.
   external Pointer<T> lookup<T extends NativeType>(String symbolName);
 
-  /// Helper that combines lookup and cast to a Dart function.
-  external F lookupFunction<T extends Function, F extends Function>(
-      String symbolName);
-
   /// Dynamic libraries are equal if they load the same library.
   external bool operator ==(other);
 
@@ -43,3 +39,10 @@ class DynamicLibrary {
   /// The handle to the dynamic library.
   external Pointer<Void> get handle;
 }
+
+/// Methods which cannot be invoked dynamically.
+extension LibraryExtension on DynamicLibrary {
+  /// Helper that combines lookup and cast to a Dart function.
+  external F lookupFunction<T extends Function, F extends Function>(
+      String symbolName);
+}

sdk/lib/ffi/ffi.dart

@@ -68,13 +68,6 @@ class Pointer<T extends NativeType> extends NativeType {
   /// Cast Pointer<T> to a Pointer<V>.
   external Pointer<U> cast<U extends NativeType>();
 
-  /// Convert to Dart function, automatically marshalling the arguments
-  /// and return value.
-  ///
-  /// Can only be called on [Pointer]<[NativeFunction]>. Does not accept dynamic
-  /// invocations -- where the type of the receiver is [dynamic].
-  external R asFunction<@DartRepresentationOf("T") R extends Function>();
-
   /// Equality for Pointers only depends on their address.
   bool operator ==(other) {
     if (other == null) return false;
@@ -87,6 +80,14 @@ class Pointer<T extends NativeType> extends NativeType {
   }
 }
 
+/// Extension on [Pointer] specialized for the type argument [NativeFunction].
+extension NativeFunctionPointer<NF extends Function>
+    on Pointer<NativeFunction<NF>> {
+  /// Convert to Dart function, automatically marshalling the arguments
+  /// and return value.
+  external DF asFunction<@DartRepresentationOf("NF") DF extends Function>();
+}
+
 //
 // The following code is generated, do not edit by hand.
 //

sdk_nnbd/lib/_internal/vm/lib/ffi_dynamic_library_patch.dart

@@ -30,15 +30,6 @@ class DynamicLibrary {
   Pointer<T> lookup<T extends NativeType>(String symbolName)
       native "Ffi_dl_lookup";
 
-  // The real implementation of this function lives in FfiUseSiteTransformer
-  // for interface calls. Only dynamic calls (which are illegal) reach this
-  // implementation.
-  @patch
-  F lookupFunction<T extends Function, F extends Function>(String symbolName) {
-    throw UnsupportedError(
-        "Dynamic invocation of lookupFunction is not supported.");
-  }
-
   // TODO(dacoharkes): Expose this to users, or extend Pointer?
   // https://github.com/dart-lang/sdk/issues/35881
   int getHandle() native "Ffi_dl_getHandle";
@@ -58,3 +49,10 @@ class DynamicLibrary {
   @patch
   Pointer<Void> get handle => Pointer.fromAddress(getHandle());
 }
+
+extension LibraryExtension on DynamicLibrary {
+  @patch
+  DS lookupFunction<NS extends Function, DS extends Function>(
+          String symbolName) =>
+      throw UnsupportedError("The body is inlined in the frontend.");
+}