Use getDartType rather than getFfiDartType in ObjC block codegen (#632)

* Blocks returning proper user facing types * More tests and refactors * fmt * Fix test * Fix vararg test * Partial fix for block ref counts * More block ref counting fixes * fmt * Daco's comments
dart-archive · Oct 27, 2023 · ba94da9 · ba94da9
1 parent de788e3
commit ba94da9
Show file tree

Hide file tree

Showing 14 changed files with 701 additions and 192 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,6 +8,7 @@
 - Fix ObjC methods returning instancetype having the wrong type in sublasses.
 - When generating typedefs for `Pointer<NativeFunction<Function>>`, also
   generate a typedef for the `Function`.
+- Use Dart wrapper types in args and returns of ObjCBlocks.
 - Bump min SDK version to 3.2.0-114.0.dev.
 
 # 9.0.1

diff --git a/lib/src/code_generator/func_type.dart b/lib/src/code_generator/func_type.dart
@@ -23,81 +23,66 @@ class FunctionType extends Type {
     this.varArgParameters = const [],
   });
 
-  String _getCacheKeyString(
-      bool writeArgumentNames, String Function(Type) typeToString) {
-    final sb = StringBuffer();
+  String _getTypeImpl(
+      bool writeArgumentNames, String Function(Type) typeToString,
+      {String? varArgWrapper}) {
+    final params = varArgWrapper != null ? parameters : dartTypeParameters;
+    String? varArgPack;
+    if (varArgWrapper != null && varArgParameters.isNotEmpty) {
+      final varArgPackBuf = StringBuffer();
+      varArgPackBuf.write("$varArgWrapper<(");
+      varArgPackBuf.write((varArgParameters).map<String>((p) {
+        return '${typeToString(p.type)} ${writeArgumentNames ? p.name : ""}';
+      }).join(', '));
+      varArgPackBuf.write(",)>");
+      varArgPack = varArgPackBuf.toString();
+    }
 
     // Write return Type.
+    final sb = StringBuffer();
     sb.write(typeToString(returnType));
 
     // Write Function.
     sb.write(' Function(');
-    sb.write(parameters.map<String>((p) {
-      return '${typeToString(p.type)} ${writeArgumentNames ? p.name : ""}';
-    }).join(', '));
+    sb.write([
+      ...params.map<String>((p) {
+        return '${typeToString(p.type)} ${writeArgumentNames ? p.name : ""}';
+      }),
+      if (varArgPack != null) varArgPack,
+    ].join(', '));
     sb.write(')');
 
     return sb.toString();
   }
 
   @override
-  String getCType(Writer w, {bool writeArgumentNames = true}) {
-    final sb = StringBuffer();
-
-    // Write return Type.
-    sb.write(returnType.getCType(w));
-
-    // Write Function.
-    sb.write(' Function(');
-    sb.write((parameters).map<String>((p) {
-      return '${p.type.getCType(w)} ${writeArgumentNames ? p.name : ""}';
-    }).join(', '));
-    if (varArgParameters.isNotEmpty) {
-      sb.write(", ${w.ffiLibraryPrefix}.VarArgs<(");
-      sb.write((varArgParameters).map<String>((p) {
-        return '${p.type.getCType(w)} ${writeArgumentNames ? p.name : ""}';
-      }).join(', '));
-      sb.write(",)>");
-    }
-    sb.write(')');
-
-    return sb.toString();
-  }
+  String getCType(Writer w, {bool writeArgumentNames = true}) =>
+      _getTypeImpl(writeArgumentNames, (Type t) => t.getCType(w),
+          varArgWrapper: '${w.ffiLibraryPrefix}.VarArgs');
 
   @override
-  String getFfiDartType(Writer w, {bool writeArgumentNames = true}) {
-    final sb = StringBuffer();
+  String getFfiDartType(Writer w, {bool writeArgumentNames = true}) =>
+      _getTypeImpl(writeArgumentNames, (Type t) => t.getFfiDartType(w));
 
-    // Write return Type.
-    sb.write(returnType.getFfiDartType(w));
-
-    // Write Function.
-    sb.write(' Function(');
-    sb.write(dartTypeParameters.map<String>((p) {
-      return '${p.type.getFfiDartType(w)} ${writeArgumentNames ? p.name : ""}';
-    }).join(', '));
-    sb.write(')');
-
-    return sb.toString();
-  }
+  @override
+  String getDartType(Writer w, {bool writeArgumentNames = true}) =>
+      _getTypeImpl(writeArgumentNames, (Type t) => t.getDartType(w));
 
   @override
   bool get sameFfiDartAndCType =>
       returnType.sameFfiDartAndCType &&
-      parameters.every((p) => p.type.sameFfiDartAndCType) &&
-      varArgParameters.every((p) => p.type.sameFfiDartAndCType);
+      dartTypeParameters.every((p) => p.type.sameFfiDartAndCType);
 
   @override
   bool get sameDartAndCType =>
       returnType.sameDartAndCType &&
-      parameters.every((p) => p.type.sameDartAndCType) &&
-      varArgParameters.every((p) => p.type.sameDartAndCType);
+      dartTypeParameters.every((p) => p.type.sameDartAndCType);
 
   @override
-  String toString() => _getCacheKeyString(false, (Type t) => t.toString());
+  String toString() => _getTypeImpl(false, (Type t) => t.toString());
 
   @override
-  String cacheKey() => _getCacheKeyString(false, (Type t) => t.cacheKey());
+  String cacheKey() => _getTypeImpl(false, (Type t) => t.cacheKey());
 
   @override
   void addDependencies(Set<Binding> dependencies) {

diff --git a/lib/src/code_generator/objc_block.dart b/lib/src/code_generator/objc_block.dart
@@ -73,65 +73,67 @@ class ObjCBlock extends BindingType {
     final trampFuncType = FunctionType(
         returnType: returnType,
         parameters: [Parameter(type: blockPtr, name: 'block'), ...params]);
-    final natTrampFnType = NativeFunc(trampFuncType);
+    final trampFuncCType = trampFuncType.getCType(w, writeArgumentNames: false);
+    final trampFuncFfiDartType =
+        trampFuncType.getFfiDartType(w, writeArgumentNames: false);
+    final natTrampFnType = NativeFunc(trampFuncType).getCType(w);
     final nativeCallableType =
-        '${w.ffiLibraryPrefix}.NativeCallable<${trampFuncType.getCType(w)}>';
+        '${w.ffiLibraryPrefix}.NativeCallable<$trampFuncCType>';
+    final funcDartType = funcType.getDartType(w, writeArgumentNames: false);
+    final funcFfiDartType =
+        funcType.getFfiDartType(w, writeArgumentNames: false);
+    final returnFfiDartType = returnType.getFfiDartType(w);
+    final blockCType = blockPtr.getCType(w);
+
+    final paramsNameOnly = params.map((p) => p.name).join(', ');
+    final paramsFfiDartType =
+        params.map((p) => '${p.type.getFfiDartType(w)} ${p.name}').join(', ');
+    final paramsDartType =
+        params.map((p) => '${p.type.getDartType(w)} ${p.name}').join(', ');
 
     // Write the function pointer based trampoline function.
-    s.write(returnType.getFfiDartType(w));
-    s.write(' $funcPtrTrampoline(${blockPtr.getCType(w)} block');
-    for (int i = 0; i < params.length; ++i) {
-      s.write(', ${params[i].type.getFfiDartType(w)} ${params[i].name}');
-    }
-    s.write(') {\n');
-    s.write('  ${isVoid ? '' : 'return '}block.ref.target.cast<'
-        '${natFnType.getFfiDartType(w)}>().asFunction<'
-        '${funcType.getFfiDartType(w)}>()(');
-    for (int i = 0; i < params.length; ++i) {
-      s.write('${i == 0 ? '' : ', '}${params[i].name}');
-    }
-    s.write(');\n');
-    s.write('}\n');
+    s.write('''
+$returnFfiDartType $funcPtrTrampoline($blockCType block, $paramsFfiDartType) =>
+    block.ref.target.cast<${natFnType.getFfiDartType(w)}>()
+        .asFunction<$funcFfiDartType>()($paramsNameOnly);
+''');
 
     // Write the closure registry function.
     s.write('''
-final $closureRegistry = <int, Function>{};
+final $closureRegistry = <int, $funcFfiDartType>{};
 int $closureRegistryIndex = 0;
-$voidPtr $registerClosure(Function fn) {
+$voidPtr $registerClosure($funcFfiDartType fn) {
   final id = ++$closureRegistryIndex;
   $closureRegistry[id] = fn;
   return $voidPtr.fromAddress(id);
 }
 ''');
 
     // Write the closure based trampoline function.
-    s.write(returnType.getFfiDartType(w));
-    s.write(' $closureTrampoline(${blockPtr.getCType(w)} block');
-    for (int i = 0; i < params.length; ++i) {
-      s.write(', ${params[i].type.getFfiDartType(w)} ${params[i].name}');
-    }
-    s.write(') {\n');
-    s.write('  ${isVoid ? '' : 'return '}');
-    s.write('($closureRegistry[block.ref.target.address]');
-    s.write(' as ${returnType.getFfiDartType(w)} Function(');
-    for (int i = 0; i < params.length; ++i) {
-      s.write('${i == 0 ? '' : ', '}${params[i].type.getFfiDartType(w)}');
-    }
-    s.write('))');
-    s.write('(');
-    for (int i = 0; i < params.length; ++i) {
-      s.write('${i == 0 ? '' : ', '}${params[i].name}');
-    }
-    s.write(');\n');
-    s.write('}\n');
+    s.write('''
+$returnFfiDartType $closureTrampoline($blockCType block, $paramsFfiDartType) =>
+    $closureRegistry[block.ref.target.address]!($paramsNameOnly);
+''');
+
+    // Snippet that converts a Dart typed closure to FfiDart type. This snippet
+    // is used below. Note that the closure being converted is called `fn`.
+    final convertedFnArgs = params
+        .map((p) => p.type
+            .convertFfiDartTypeToDartType(w, p.name, 'lib', objCRetain: true))
+        .join(', ');
+    final convFnInvocation = returnType.convertDartTypeToFfiDartType(
+        w, 'fn($convertedFnArgs)',
+        objCRetain: true);
+    final convFn = '($paramsFfiDartType) => $convFnInvocation';
 
     // Write the wrapper class.
     final defaultValue = returnType.getDefaultValue(w, '_lib');
     final exceptionalReturn = defaultValue == null ? '' : ', $defaultValue';
     s.write('''
 class $name extends _ObjCBlockBase {
-  $name._(${blockPtr.getCType(w)} id, ${w.className} lib) :
-      super._(id, lib, retain: false, release: true);
+  $name._($blockCType id, ${w.className} lib,
+      {bool retain = false, bool release = true}) :
+          super._(id, lib, retain: retain, release: release);
 
   /// Creates a block from a C function pointer.
   ///
@@ -141,7 +143,7 @@ class $name extends _ObjCBlockBase {
   $name.fromFunctionPointer(${w.className} lib, $natFnPtr ptr) :
       this._(lib.${builtInFunctions.newBlock.name}(
           _cFuncTrampoline ??= ${w.ffiLibraryPrefix}.Pointer.fromFunction<
-              ${trampFuncType.getCType(w)}>($funcPtrTrampoline
+              $trampFuncCType>($funcPtrTrampoline
                   $exceptionalReturn).cast(), ptr.cast()), lib);
   static $voidPtr? _cFuncTrampoline;
 
@@ -150,11 +152,11 @@ class $name extends _ObjCBlockBase {
   /// 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(${w.className} lib, ${funcType.getFfiDartType(w)} fn) :
+  $name.fromFunction(${w.className} lib, $funcDartType fn) :
       this._(lib.${builtInFunctions.newBlock.name}(
           _dartFuncTrampoline ??= ${w.ffiLibraryPrefix}.Pointer.fromFunction<
-              ${trampFuncType.getCType(w)}>($closureTrampoline
-                  $exceptionalReturn).cast(), $registerClosure(fn)), lib);
+              $trampFuncCType>($closureTrampoline
+                  $exceptionalReturn).cast(), $registerClosure($convFn)), lib);
   static $voidPtr? _dartFuncTrampoline;
 
 ''');
@@ -171,31 +173,30 @@ class $name extends _ObjCBlockBase {
   ///
   /// Note that unlike the default behavior of NativeCallable.listener, listener
   /// blocks do not keep the isolate alive.
-  $name.listener(${w.className} lib, ${funcType.getFfiDartType(w)} fn) :
+  $name.listener(${w.className} lib, $funcDartType fn) :
       this._(lib.${builtInFunctions.newBlock.name}(
-          (_dartFuncListenerTrampoline ??= $nativeCallableType.listener($closureTrampoline
-                  $exceptionalReturn)..keepIsolateAlive = false).nativeFunction.cast(),
-          $registerClosure(fn)), lib);
+          (_dartFuncListenerTrampoline ??= $nativeCallableType.listener(
+              $closureTrampoline $exceptionalReturn)..keepIsolateAlive =
+                  false).nativeFunction.cast(),
+          $registerClosure($convFn)), lib);
   static $nativeCallableType? _dartFuncListenerTrampoline;
 
 ''');
     }
 
     // Call method.
-    s.write('  ${returnType.getFfiDartType(w)} call(');
-    for (int i = 0; i < params.length; ++i) {
-      s.write('${i == 0 ? '' : ', '}${params[i].type.getFfiDartType(w)}');
-      s.write(' ${params[i].name}');
-    }
-    s.write(''') {
-    ${isVoid ? '' : 'return '}_id.ref.invoke.cast<
-        ${natTrampFnType.getCType(w)}>().asFunction<
-            ${trampFuncType.getFfiDartType(w)}>()(_id''');
-    for (int i = 0; i < params.length; ++i) {
-      s.write(', ${params[i].name}');
-    }
-    s.write(''');
-  }''');
+    s.write('  ${returnType.getDartType(w)} call($paramsDartType) =>');
+    final callMethodArgs = params
+        .map((p) =>
+            p.type.convertDartTypeToFfiDartType(w, p.name, objCRetain: false))
+        .join(', ');
+    final callMethodInvocation = '''
+_id.ref.invoke.cast<$natTrampFnType>().asFunction<$trampFuncFfiDartType>()(
+    _id, $callMethodArgs)''';
+    s.write(returnType.convertFfiDartTypeToDartType(
+        w, callMethodInvocation, '_lib',
+        objCRetain: false));
+    s.write(';\n');
 
     s.write('}\n');
     return BindingString(
@@ -227,6 +228,24 @@ class $name extends _ObjCBlockBase {
   @override
   bool get sameDartAndCType => false;
 
+  @override
+  String convertDartTypeToFfiDartType(
+    Writer w,
+    String value, {
+    required bool objCRetain,
+  }) =>
+      ObjCInterface.generateGetId(value, objCRetain);
+
+  @override
+  String convertFfiDartTypeToDartType(
+    Writer w,
+    String value,
+    String library, {
+    required bool objCRetain,
+    String? objCEnclosingClass,
+  }) =>
+      ObjCInterface.generateConstructor(name, value, library, objCRetain);
+
   @override
   String toString() => '($returnType (^)(${argTypes.join(', ')}))';
 }
diff --git a/lib/src/code_generator/objc_built_in_functions.dart b/lib/src/code_generator/objc_built_in_functions.dart
@@ -239,6 +239,11 @@ class $name implements ${w.ffiLibraryPrefix}.Finalizable {
 
   /// Return a pointer to this object.
   $idType get pointer => _id;
+
+  $idType _retainAndReturnId() {
+    _lib.$retain(_id.cast());
+    return _id;
+  }
 }
 ''');
   }