[ffigen] Fix block inheritance issue (#1433)

pkgs/ffigen/CHANGELOG.md

@@ -9,6 +9,11 @@
 - Global variables using ObjC types (interfaces or blocks) will now use the
   correct Dart wrapper types, instead of the raw C-style pointers.
 - Rename `assetId` under *ffi-native* to `asset-id` to follow dash-case.
+- __Breaking change__: ObjC blocks are now passed through all ObjC APIs as
+  `ObjCBlock<Ret Function(Args...)>`, instead of the codegenned
+  `ObjCBlock_...` wrapper. The wrapper is now a non-constructible set of util
+  methods for constructing `ObjCBlock`.
+
 
 ## 13.0.0
 

pkgs/ffigen/lib/src/code_generator/imports.dart

@@ -129,4 +129,4 @@ final objCObjectType =
 final objCSelType =
     ImportedType(objcPkgImport, 'ObjCSelector', 'ObjCSelector', 'void');
 final objCBlockType =
-    ImportedType(objcPkgImport, 'ObjCBlock', 'ObjCBlock', 'id');
+    ImportedType(objcPkgImport, 'ObjCBlockImpl', 'ObjCBlockImpl', 'id');

pkgs/ffigen/lib/src/code_generator/objc_block.dart

@@ -66,6 +66,12 @@ class ObjCBlock extends BindingType {
 
   bool get hasListener => returnType == voidType;
 
+  String _blockType(Writer w) {
+    final args = argTypes.map((t) => t.getObjCBlockSignatureType(w)).join(', ');
+    final func = '${returnType.getObjCBlockSignatureType(w)} Function($args)';
+    return '${ObjCBuiltInFunctions.blockType.gen(w)}<$func>';
+  }
+
   @override
   BindingString toBindingString(Writer w) {
     final s = StringBuffer();
@@ -84,6 +90,8 @@ class ObjCBlock extends BindingType {
         w.topLevelUniqueNamer.makeUnique('_${name}_fnPtrTrampoline');
     final closureTrampoline =
         w.topLevelUniqueNamer.makeUnique('_${name}_closureTrampoline');
+    final callExtension =
+        w.topLevelUniqueNamer.makeUnique('${name}_CallExtension');
     final newPointerBlock = ObjCBuiltInFunctions.newPointerBlock.gen(w);
     final newClosureBlock = ObjCBuiltInFunctions.newClosureBlock.gen(w);
     final getBlockClosure = ObjCBuiltInFunctions.getBlockClosure.gen(w);
@@ -101,6 +109,7 @@ class ObjCBlock extends BindingType {
         funcType.getFfiDartType(w, writeArgumentNames: false);
     final returnFfiDartType = returnType.getFfiDartType(w);
     final blockCType = blockPtr.getCType(w);
+    final blockType = _blockType(w);
 
     final paramsNameOnly = params.map((p) => p.name).join(', ');
     final paramsFfiDartType =
@@ -136,41 +145,38 @@ $returnFfiDartType $closureTrampoline($blockCType block, $paramsFfiDartType) =>
     final defaultValue = returnType.getDefaultValue(w);
     final exceptionalReturn = defaultValue == null ? '' : ', $defaultValue';
     s.write('''
-class $name extends ${ObjCBuiltInFunctions.blockBase.gen(w)} {
-  $name._($blockCType pointer,
-      {bool retain = false, bool release = true}) :
-          super(pointer, retain: retain, release: release);
 
+/// Construction methods for `$blockType`.
+abstract final class $name {
   /// Returns a block that wraps the given raw block pointer.
-  static $name castFromPointer($blockCType pointer,
-      {bool retain = false, bool release = false}) {
-    return $name._(pointer, retain: retain, release: release);
-  }
+  static $blockType castFromPointer($blockCType pointer,
+      {bool retain = false, bool release = false}) =>
+      $blockType(pointer, retain: retain, release: release);
 
   /// Creates a block from a C function pointer.
   ///
   /// This block must be invoked by native code running on the same thread as
   /// the isolate that registered it. Invoking the block on the wrong thread
   /// will result in a crash.
-  $name.fromFunctionPointer($natFnPtr ptr) :
-      this._($newPointerBlock(
+  static $blockType fromFunctionPointer($natFnPtr ptr) =>
+      $blockType($newPointerBlock(
           _cFuncTrampoline ??= ${w.ffiLibraryPrefix}.Pointer.fromFunction<
               $trampFuncCType>($funcPtrTrampoline
-                  $exceptionalReturn).cast(), ptr.cast()));
+                  $exceptionalReturn).cast(), ptr.cast()),
+          retain: false, release: true);
   static $voidPtr? _cFuncTrampoline;
 
   /// Creates a block from a Dart function.
   ///
   /// This block must be invoked by native code running on the same thread as
   /// the isolate that registered it. Invoking the block on the wrong thread
   /// will result in a crash.
-  $name.fromFunction($funcDartType fn) :
-      this._($newClosureBlock(
+  static $blockType fromFunction($funcDartType fn) =>
+      $blockType($newClosureBlock(
           _dartFuncTrampoline ??= ${w.ffiLibraryPrefix}.Pointer.fromFunction<
               $trampFuncCType>($closureTrampoline $exceptionalReturn).cast(),
-          $convFn));
+          $convFn), retain: false, release: true);
   static $voidPtr? _dartFuncTrampoline;
-
 ''');
 
     // Listener block constructor is only available for void blocks.
@@ -189,6 +195,7 @@ class $name extends ${ObjCBuiltInFunctions.blockBase.gen(w)} {
           '($paramsFfiDartType) => $listenerConvFnInvocation';
 
       s.write('''
+
   /// Creates a listener block from a Dart function.
   ///
   /// This is based on FFI's NativeCallable.listener, and has the same
@@ -198,18 +205,22 @@ class $name extends ${ObjCBuiltInFunctions.blockBase.gen(w)} {
   ///
   /// Note that unlike the default behavior of NativeCallable.listener, listener
   /// blocks do not keep the isolate alive.
-  $name.listener($funcDartType fn) :
-      this._(${_wrapListenerBlock?.name ?? ''}($newClosureBlock(
+  static $blockType listener($funcDartType fn) =>
+      $blockType(${_wrapListenerBlock?.name ?? ''}($newClosureBlock(
           (_dartFuncListenerTrampoline ??= $nativeCallableType.listener(
               $closureTrampoline $exceptionalReturn)..keepIsolateAlive =
-                  false).nativeFunction.cast(), $listenerConvFn)));
+                  false).nativeFunction.cast(), $listenerConvFn)),
+          retain: false, release: true);
   static $nativeCallableType? _dartFuncListenerTrampoline;
-
 ''');
     }
+    s.write('}\n\n');
 
-    // Call method.
-    s.write('  ${returnType.getDartType(w)} call($paramsDartType) =>');
+    // Call operator extension method.
+    s.write('''
+/// Call operator for `$blockType`.
+extension $callExtension on $blockType {
+  ${returnType.getDartType(w)} call($paramsDartType) =>''');
     final callMethodArgs = params
         .map((p) =>
             p.type.convertDartTypeToFfiDartType(w, p.name, objCRetain: false))
@@ -221,7 +232,7 @@ pointer.ref.invoke.cast<$natTrampFnType>().asFunction<$trampFuncFfiDartType>()(
         objCRetain: false));
     s.write(';\n');
 
-    s.write('}\n');
+    s.write('}\n\n');
     return BindingString(
         type: BindingStringType.objcBlock, string: s.toString());
   }
@@ -284,8 +295,14 @@ $blockTypedef $fnName($blockTypedef block) {
   @override
   String getCType(Writer w) => PointerType(objCBlockType).getCType(w);
 
+  // We return `ObjCBlockBase<T>` here instead of the code genned wrapper, so
+  // that the subtyping rules work as expected.
+  // See https://github.com/dart-lang/native/issues/1416 for details.
+  @override
+  String getDartType(Writer w) => _blockType(w);
+
   @override
-  String getDartType(Writer w) => name;
+  String getObjCBlockSignatureType(Writer w) => getDartType(w);
 
   @override
   String getNativeType({String varName = ''}) {

pkgs/ffigen/lib/src/code_generator/objc_built_in_functions.dart

@@ -26,7 +26,7 @@ class ObjCBuiltInFunctions {
       ObjCImport('getProtocolMethodSignature');
   static const getProtocol = ObjCImport('getProtocol');
   static const objectBase = ObjCImport('ObjCObjectBase');
-  static const blockBase = ObjCImport('ObjCBlockBase');
+  static const blockType = ObjCImport('ObjCBlock');
   static const protocolMethod = ObjCImport('ObjCProtocolMethod');
   static const protocolListenableMethod =
       ObjCImport('ObjCProtocolListenableMethod');

pkgs/ffigen/lib/src/code_generator/objc_interface.dart

@@ -74,6 +74,7 @@ class ObjCInterface extends BindingType with ObjCMethods {
     }
 
     final s = StringBuffer();
+    s.write('\n');
     s.write(makeDartDoc(dartDoc ?? originalName));
 
     final methodNamer = createMethodRenamer(w);
@@ -108,7 +109,6 @@ class ObjCInterface extends BindingType with ObjCMethods {
       [_classObject.name],
     )};
   }
-
 ''');
 
     // Methods.
@@ -125,6 +125,7 @@ class ObjCInterface extends BindingType with ObjCMethods {
       }
 
       // The method declaration.
+      s.write('\n  ');
       s.write(makeDartDoc(m.dartDoc ?? m.originalName));
       s.write('  ');
       if (isStatic) {
@@ -203,7 +204,7 @@ class ObjCInterface extends BindingType with ObjCMethods {
         s.write('    return $result;');
       }
 
-      s.write('  }\n\n');
+      s.write('\n  }\n');
     }
 
     s.write('}\n\n');
@@ -298,6 +299,9 @@ class ObjCInterface extends BindingType with ObjCMethods {
   @override
   String getNativeType({String varName = ''}) => '$originalName* $varName';
 
+  @override
+  String getObjCBlockSignatureType(Writer w) => getDartType(w);
+
   @override
   bool get sameFfiDartAndCType => true;
 

pkgs/ffigen/lib/src/code_generator/objc_protocol.dart

@@ -48,7 +48,7 @@ class ObjCProtocol extends NoLookUpBinding with ObjCMethods {
       final fieldName = methodName;
       final argName = methodName;
       final block = method.protocolBlock;
-      final blockType = block.getDartType(w);
+      final blockUtils = block.name;
       final methodClass =
           block.hasListener ? protocolListenableMethod : protocolMethod;
 
@@ -75,7 +75,7 @@ class ObjCProtocol extends NoLookUpBinding with ObjCMethods {
       var listenerBuilder = '';
       var maybeImplementAsListener = 'implement';
       if (block.hasListener) {
-        listenerBuilder = '($funcType func) => $blockType.listener($wrapper),';
+        listenerBuilder = '($funcType func) => $blockUtils.listener($wrapper),';
         maybeImplementAsListener = 'implementAsListener';
         anyListeners = true;
       }
@@ -94,7 +94,7 @@ class ObjCProtocol extends NoLookUpBinding with ObjCMethods {
           isRequired: ${method.isRequired},
           isInstanceMethod: ${method.isInstanceMethod},
       ),
-      ($funcType func) => $blockType.fromFunction($wrapper),
+      ($funcType func) => $blockUtils.fromFunction($wrapper),
       $listenerBuilder
     );
 ''');

pkgs/ffigen/lib/src/code_generator/type.dart

@@ -49,6 +49,12 @@ abstract class Type {
   /// as getFfiDartType. For ObjC bindings this refers to the wrapper object.
   String getDartType(Writer w) => getFfiDartType(w);
 
+  /// Returns the type to be used if this type appears in an ObjC block
+  /// signature. By default it's the same as [getCType]. But for some types
+  /// that's not enough to distinguish them (eg all ObjC objects have a C type
+  /// of `Pointer<objc.ObjCObject>`), so we use [getDartType] instead.
+  String getObjCBlockSignatureType(Writer w) => getCType(w);
+
   /// Returns the C/ObjC type of the Type. This is the type as it appears in
   /// C/ObjC source code. It should not be used in Dart source code.
   ///
@@ -150,6 +156,9 @@ abstract class BindingType extends NoLookUpBinding implements Type {
   @override
   String getDartType(Writer w) => getFfiDartType(w);
 
+  @override
+  String getObjCBlockSignatureType(Writer w) => getCType(w);
+
   @override
   String getNativeType({String varName = ''}) =>
       throw UnsupportedError('No native mapping for type: $this');

pkgs/ffigen/lib/src/code_generator/writer.dart

@@ -405,7 +405,7 @@ class Writer {
     }
 
     // If it's a framework header, use a <> style import.
-    return '#import <$frameworkHeader>';
+    return '#import <$frameworkHeader>\n';
   }
 
   /// Writes the Objective C code needed for the bindings, if any. Returns null

pkgs/ffigen/test/native_objc_test/block_inherit_config.yaml

@@ -0,0 +1,24 @@
+name: BlockInheritTestObjCLibrary
+description: 'Tests inheritance rules for blocks.'
+language: objc
+output:
+  bindings: 'block_inherit_bindings.dart'
+  objc-bindings: 'block_inherit_bindings.m'
+exclude-all-by-default: true
+objc-interfaces:
+  include:
+    - Mammal
+    - Platypus
+    - BlockInheritTestBase
+    - BlockInheritTestChild
+typedefs:
+  include:
+    - ReturnMammal
+    - ReturnPlatypus
+    - AcceptMammal
+    - AcceptPlatypus
+headers:
+  entry-points:
+    - 'block_inherit_test.m'
+preamble: |
+  // ignore_for_file: camel_case_types, non_constant_identifier_names, unnecessary_non_null_assertion, unused_element, unused_field