[ffigen] Protocol polish (#2060)

Author: liamappelbe

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

@@ -43,6 +43,7 @@ class ObjCBuiltInFunctions {
   static const protocolMethod = ObjCImport('ObjCProtocolMethod');
   static const protocolListenableMethod =
       ObjCImport('ObjCProtocolListenableMethod');
+  static const protocolClass = ObjCImport('Protocol');
   static const protocolBuilder = ObjCImport('ObjCProtocolBuilder');
   static const unimplementedOptionalMethodException =
       ObjCImport('UnimplementedOptionalMethodException');

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

@@ -62,6 +62,7 @@ class ObjCProtocol extends BindingType with ObjCMethods {
 
   @override
   BindingString toBindingString(Writer w) {
+    final protocolClass = ObjCBuiltInFunctions.protocolClass.gen(w);
     final protocolBase = ObjCBuiltInFunctions.protocolBase.gen(w);
     final protocolMethod = ObjCBuiltInFunctions.protocolMethod.gen(w);
     final protocolListenableMethod =
@@ -180,6 +181,10 @@ interface class $name extends $protocolBase $impls{
       buildArgs.add('bool \$keepIsolateAlive = true');
       final args = '{${buildArgs.join(', ')}}';
       final builders = '''
+  /// Returns the [$protocolClass] object for this protocol.
+  static $protocolClass get \$protocol =>
+      $protocolClass.castFromPointer(${_protocolPointer.name}.cast());
+
   /// Builds an object that implements the $originalName protocol. To implement
   /// multiple protocols, use [addToBuilder] or [$protocolBuilder] directly.
   ///
@@ -188,6 +193,7 @@ interface class $name extends $protocolBase $impls{
   static $name implement($args) {
     final builder = $protocolBuilder(debugName: '$originalName');
     $buildImplementations
+    builder.addProtocol(\$protocol);
     return $name.castFrom(builder.build(keepIsolateAlive: \$keepIsolateAlive));
   }
 
@@ -197,6 +203,7 @@ interface class $name extends $protocolBase $impls{
   /// Note: You cannot call this method after you have called `builder.build`.
   static void addToBuilder($protocolBuilder builder, $args) {
     $buildImplementations
+    builder.addProtocol(\$protocol);
   }
 ''';
 
@@ -212,6 +219,7 @@ interface class $name extends $protocolBase $impls{
   static $name implementAsListener($args) {
     final builder = $protocolBuilder(debugName: '$originalName');
     $buildListenerImplementations
+    builder.addProtocol(\$protocol);
     return $name.castFrom(builder.build(keepIsolateAlive: \$keepIsolateAlive));
   }
 
@@ -222,6 +230,7 @@ interface class $name extends $protocolBase $impls{
   /// Note: You cannot call this method after you have called `builder.build`.
   static void addToBuilderAsListener($protocolBuilder builder, $args) {
     $buildListenerImplementations
+    builder.addProtocol(\$protocol);
   }
 
   /// Builds an object that implements the $originalName protocol. To implement
@@ -233,6 +242,7 @@ interface class $name extends $protocolBase $impls{
   static $name implementAsBlocking($args) {
     final builder = $protocolBuilder(debugName: '$originalName');
     $buildBlockingImplementations
+    builder.addProtocol(\$protocol);
     return $name.castFrom(builder.build(keepIsolateAlive: \$keepIsolateAlive));
   }
 
@@ -243,6 +253,7 @@ interface class $name extends $protocolBase $impls{
   /// Note: You cannot call this method after you have called `builder.build`.
   static void addToBuilderAsBlocking($protocolBuilder builder, $args) {
     $buildBlockingImplementations
+    builder.addProtocol(\$protocol);
   }
 ''';
       }

pkgs/ffigen/test/native_objc_test/protocol_config.yaml

@@ -5,6 +5,12 @@ output:
   bindings: 'protocol_bindings.dart'
   objc-bindings: 'protocol_bindings.m'
 exclude-all-by-default: true
+functions:
+  include:
+    - getClass
+    - getClassName
+    - objc_autoreleasePoolPop
+    - objc_autoreleasePoolPush
 objc-interfaces:
   include:
     - ProtocolConsumer

pkgs/ffigen/test/native_objc_test/protocol_test.dart

@@ -12,6 +12,7 @@ import 'dart:isolate';
 
 import 'package:ffi/ffi.dart';
 import 'package:objective_c/objective_c.dart';
+import 'package:objective_c/src/internal.dart' show getProtocol;
 import 'package:test/test.dart';
 
 import '../test_utils.dart';
@@ -24,13 +25,15 @@ typedef VoidMethodBlock = ObjCBlock_ffiVoid_ffiVoid_Int32;
 typedef OtherMethodBlock = ObjCBlock_Int32_ffiVoid_Int32_Int32_Int32_Int32;
 
 void main() {
+  late ProtocolTestObjCLibrary lib;
+
   group('protocol', () {
     setUpAll(() {
       // TODO(https://github.com/dart-lang/native/issues/1068): Remove this.
       DynamicLibrary.open('../objective_c/test/objective_c.dylib');
       final dylib = File('test/native_objc_test/objc_test.dylib');
       verifySetupFile(dylib);
-      DynamicLibrary.open(dylib.absolute.path);
+      lib = ProtocolTestObjCLibrary(DynamicLibrary.open(dylib.absolute.path));
       generateBindingsForCoverage('protocol');
     });
 
@@ -126,6 +129,9 @@ void main() {
           },
         );
 
+        expect(MyProtocol.conformsTo(myProtocol), isTrue);
+        expect(SecondaryProtocol.conformsTo(myProtocol), isFalse);
+
         // Required instance method.
         final result = consumer.callInstanceMethod_(myProtocol);
         expect(result.toDartString(), 'MyProtocol: Hello from ObjC: 3.14');
@@ -157,6 +163,9 @@ void main() {
         final SecondaryProtocol asSecondaryProtocol =
             SecondaryProtocol.castFrom(protocolImpl);
 
+        expect(MyProtocol.conformsTo(protocolImpl), isTrue);
+        expect(SecondaryProtocol.conformsTo(protocolImpl), isTrue);
+
         // Required instance method.
         final result = consumer.callInstanceMethod_(asMyProtocol);
         expect(result.toDartString(), 'ProtocolBuilder: Hello from ObjC: 3.14');
@@ -413,6 +422,71 @@ void main() {
       expect(UnusedProtocol.conformsTo(inst), isFalse);
     });
 
+    test('Threading stress test', () async {
+      final consumer = ProtocolConsumer();
+      final completer = Completer<void>();
+      int count = 0;
+
+      final protocolBuilder = ObjCProtocolBuilder();
+      MyProtocol.voidMethod_.implementAsListener(protocolBuilder, (int x) {
+        expect(x, 123);
+        ++count;
+        if (count == 1000) completer.complete();
+      });
+
+      final protocol = protocolBuilder.build();
+      final MyProtocol asMyProtocol = MyProtocol.castFrom(protocol);
+
+      for (int i = 0; i < 1000; ++i) {
+        consumer.callMethodOnRandomThread_(asMyProtocol);
+      }
+      await completer.future;
+      expect(count, 1000);
+    });
+
+    (NSObject, Pointer<ObjCBlockImpl>) blockRefCountTestInner() {
+      final pool = lib.objc_autoreleasePoolPush();
+      final protocolBuilder = ObjCProtocolBuilder();
+
+      final block = InstanceMethodBlock.fromFunction(
+          (Pointer<Void> p, NSString s, double x) => 'Hello'.toNSString());
+      MyProtocol.instanceMethod_withDouble_
+          .implementWithBlock(protocolBuilder, block);
+      final protocol = protocolBuilder.build();
+      lib.objc_autoreleasePoolPop(pool);
+
+      final blockPtr = block.ref.pointer;
+
+      // There are 2 references to the block. One owned by the Dart wrapper
+      // object, and the other owned by the protocol.
+      doGC();
+      expect(blockRetainCount(blockPtr), 2);
+
+      return (protocol, blockPtr);
+    }
+
+    Pointer<ObjCBlockImpl> blockRefCountTest() {
+      final (protocol, blockPtr) = blockRefCountTestInner();
+
+      // The Dart side block pointer has gone out of scope, but the protocol
+      // still owns a reference to it.
+      doGC();
+      expect(blockRetainCount(blockPtr), 1);
+
+      expect(protocol, isNotNull); // Force protocol to stay in scope.
+
+      return blockPtr;
+    }
+
+    test('Block ref counting', () {
+      final blockPtr = blockRefCountTest();
+
+      // The protocol object has gone out of scope, so it should be cleaned up.
+      // So should the block.
+      doGC();
+      expect(blockRetainCount(blockPtr), 0);
+    }, skip: !canDoGC);
+
     test('keepIsolateAlive', () async {
       final isolateSendPort = Completer<SendPort>();
       final protosCreated = Completer<void>();
@@ -475,5 +549,70 @@ void main() {
 
       receivePort.close();
     }, skip: !canDoGC);
+
+    test('class disposal, builder first', () {
+      final pool = lib.objc_autoreleasePoolPush();
+      ObjCProtocolBuilder? protocolBuilder =
+          ObjCProtocolBuilder(debugName: 'Foo');
+
+      NSObject? protocol = protocolBuilder.build();
+      final clazz = lib.getClass(protocol);
+      expect(lib.getClassName(clazz).cast<Utf8>().toDartString(),
+          startsWith('Foo'));
+      expect(isValidClass(clazz), isTrue);
+      lib.objc_autoreleasePoolPop(pool);
+
+      protocolBuilder = null;
+      doGC();
+      expect(isValidClass(clazz), isTrue);
+
+      protocol = null;
+      doGC();
+      expect(isValidClass(clazz), isFalse);
+    }, skip: !canDoGC);
+
+    test('class disposal, instance first', () {
+      final pool = lib.objc_autoreleasePoolPush();
+      ObjCProtocolBuilder? protocolBuilder =
+          ObjCProtocolBuilder(debugName: 'Foo');
+
+      NSObject? protocol = protocolBuilder.build();
+      final clazz = lib.getClass(protocol);
+      expect(lib.getClassName(clazz).cast<Utf8>().toDartString(),
+          startsWith('Foo'));
+      expect(isValidClass(clazz), isTrue);
+      lib.objc_autoreleasePoolPop(pool);
+
+      protocolBuilder = null;
+      doGC();
+      expect(isValidClass(clazz), isTrue);
+
+      protocol = null;
+      doGC();
+      expect(isValidClass(clazz), isFalse);
+    }, skip: !canDoGC);
+
+    test('adding more methods after build', () {
+      final protocolBuilder = ObjCProtocolBuilder();
+
+      MyProtocol.addToBuilder(
+        protocolBuilder,
+        instanceMethod_withDouble_: (NSString s, double x) {
+          return 'ProtocolBuilder: ${s.toDartString()}: $x'.toNSString();
+        },
+        optionalMethod_: (SomeStruct s) {
+          return s.y - s.x;
+        },
+      );
+
+      final protocolImpl = protocolBuilder.build();
+
+      expect(
+          () => SecondaryProtocol.addToBuilder(protocolBuilder,
+                  otherMethod_b_c_d_: (int a, int b, int c, int d) {
+                return a * b * c * d;
+              }),
+          throwsA(isA<StateError>()));
+    });
   });
 }

pkgs/ffigen/test/native_objc_test/protocol_test.h

@@ -5,6 +5,11 @@
 #import <Foundation/NSObject.h>
 #import <Foundation/NSString.h>
 
+const char* getClassName(void* cls);
+void* getClass(id object);
+void objc_autoreleasePoolPop(void *pool);
+void *objc_autoreleasePoolPush();
+
 typedef struct {
   int32_t x;
   int32_t y;

pkgs/ffigen/test/native_objc_test/protocol_test.m

@@ -8,6 +8,16 @@
 
 #include "protocol_test.h"
 
+const char* class_getName(Class cls);
+
+const char* getClassName(void* cls) {
+  return class_getName((__bridge Class)cls);
+}
+
+void* getClass(id object) {
+  return (__bridge void*)[object class];
+}
+
 @implementation ProtocolConsumer : NSObject
 - (NSString*)callInstanceMethod:(id<SuperProtocol>)protocol {
   return [protocol instanceMethod:@"Hello from ObjC" withDouble:3.14];

pkgs/ffigen/test/native_objc_test/util.dart

@@ -93,3 +93,6 @@ int objectRetainCount(Pointer<ObjCObject> object) {
   if (!internal_for_testing.isValidClass(clazz)) return 0;
   return _getObjectRetainCount(object.cast());
 }
+
+bool isValidClass(Pointer<Void> clazz) =>
+    internal_for_testing.isValidClass(clazz.cast(), forceReloadClasses: true);

pkgs/objective_c/ffigen_c.yaml

@@ -22,7 +22,6 @@ functions:
     - 'protocol_getMethodDescription'
     - 'protocol_getName'
     - 'newFinalizableHandle'
-    - 'class_addMethod'
     - 'DOBJC_.*'
   leaf:
     include:
@@ -56,10 +55,7 @@ functions:
     'objc_msgSend_stret': 'msgSendStret'
     'object_getClass': 'getObjectClass'
     'objc_copyClassList': 'copyClassList'
-    'objc_allocateClassPair': 'allocateClassPair'
-    'objc_registerClassPair': 'registerClassPair'
     'objc_getProtocol': 'getProtocol'
-    'class_addMethod': 'addMethod'
     'protocol_getMethodDescription': 'getMethodDescription'
     'protocol_getName': 'getProtocolName'
 globals:

pkgs/objective_c/lib/src/c_bindings_generated.dart

@@ -56,30 +56,6 @@ external ffi.Array<ffi.Pointer<ffi.Void>> NSConcreteMallocBlock;
 @ffi.Native<ffi.Array<ffi.Pointer<ffi.Void>>>(symbol: '_NSConcreteStackBlock')
 external ffi.Array<ffi.Pointer<ffi.Void>> NSConcreteStackBlock;
 
-@ffi.Native<
-    ffi.Bool Function(
-        ffi.Pointer<ObjCObject>,
-        ffi.Pointer<ObjCSelector>,
-        ffi.Pointer<ffi.Void>,
-        ffi.Pointer<ffi.Char>)>(symbol: 'class_addMethod', isLeaf: true)
-external bool addMethod(
-  ffi.Pointer<ObjCObject> cls,
-  ffi.Pointer<ObjCSelector> name,
-  ffi.Pointer<ffi.Void> imp,
-  ffi.Pointer<ffi.Char> types,
-);
-
-@ffi.Native<
-    ffi.Pointer<ObjCObject> Function(
-        ffi.Pointer<ObjCObject>,
-        ffi.Pointer<ffi.Char>,
-        ffi.Size)>(symbol: 'objc_allocateClassPair', isLeaf: true)
-external ffi.Pointer<ObjCObject> allocateClassPair(
-  ffi.Pointer<ObjCObject> superclass,
-  ffi.Pointer<ffi.Char> name,
-  int extraBytes,
-);
-
 @ffi.Native<ffi.Void Function(ffi.Pointer<ffi.Void>)>(
     symbol: 'DOBJC_awaitWaiter')
 external void awaitWaiter(
@@ -211,12 +187,6 @@ external ffi.Pointer<ObjCObject> objectRetain(
   ffi.Pointer<ObjCObject> object,
 );
 
-@ffi.Native<ffi.Void Function(ffi.Pointer<ObjCObject>)>(
-    symbol: 'objc_registerClassPair', isLeaf: true)
-external void registerClassPair(
-  ffi.Pointer<ObjCObject> cls,
-);
-
 @ffi.Native<ffi.Pointer<ObjCSelector> Function(ffi.Pointer<ffi.Char>)>(
     symbol: 'sel_registerName', isLeaf: true)
 external ffi.Pointer<ObjCSelector> registerName(

pkgs/objective_c/lib/src/internal.dart

@@ -295,8 +295,8 @@ bool _isValidObject(ObjectPtr ptr) {
 
 final _allClasses = <ObjectPtr>{};
 
-bool _isValidClass(ObjectPtr clazz) {
-  if (_allClasses.contains(clazz)) return true;
+bool _isValidClass(ObjectPtr clazz, {bool forceReloadClasses = false}) {
+  if (!forceReloadClasses && _allClasses.contains(clazz)) return true;
 
   // If the class is missing from the list, it either means we haven't created
   // the set yet, or more classes have been loaded since we created the set, or
@@ -446,5 +446,6 @@ BlockPtr wrapBlockingBlock(
 bool blockHasRegisteredClosure(BlockPtr block) =>
     _blockClosureRegistry.containsKey(block.ref.target.address);
 bool isValidBlock(BlockPtr block) => c.isValidBlock(block);
-bool isValidClass(ObjectPtr clazz) => _isValidClass(clazz);
+bool isValidClass(ObjectPtr clazz, {bool forceReloadClasses = false}) =>
+    _isValidClass(clazz, forceReloadClasses: forceReloadClasses);
 bool isValidObject(ObjectPtr object) => _isValidObject(object);