[ffigen] Migrate method filtering to the visitor (#1684)

Author: liamappelbe

pkgs/ffigen/CHANGELOG.md

@@ -2,6 +2,10 @@
 
 - Ensure all protocols referenced in bindings are available at runtime.
 - Use package:objective_c 4.0.0
+- Fix various small bugs todo with config filters:
+  - https://github.com/dart-lang/native/issues/1582
+  - https://github.com/dart-lang/native/issues/1594
+  - https://github.com/dart-lang/native/issues/1595
 
 ## 15.0.0
 

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

@@ -135,6 +135,6 @@ final wCharType = ImportedType(ffiImport, 'WChar', 'int', 'wchar_t', '0');
 final objCObjectType =
     ImportedType(objcPkgImport, 'ObjCObject', 'ObjCObject', 'void');
 final objCSelType = ImportedType(
-    objcPkgImport, 'ObjCSelector', 'ObjCSelector', 'objc_selector');
+    objcPkgImport, 'ObjCSelector', 'ObjCSelector', 'struct objc_selector');
 final objCBlockType =
     ImportedType(objcPkgImport, 'ObjCBlockImpl', 'ObjCBlockImpl', 'id');

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

@@ -154,10 +154,18 @@ class ObjCInterface extends BindingType with ObjCMethods {
       s.write(' {\n');
 
       // Implementation.
+      final target = isStatic
+          ? _classObject.name
+          : convertDartTypeToFfiDartType(
+              w,
+              'this',
+              objCRetain: m.consumesSelf,
+              objCAutorelease: false,
+            );
       final sel = m.selObject.name;
       if (m.isOptional) {
         s.write('''
-    if (!${ObjCBuiltInFunctions.respondsToSelector.gen(w)}(ref.pointer, $sel)) {
+    if (!${ObjCBuiltInFunctions.respondsToSelector.gen(w)}($target, $sel)) {
       throw ${ObjCBuiltInFunctions.unimplementedOptionalMethodException.gen(w)}(
           '$originalName', '${m.originalName}');
     }
@@ -166,14 +174,6 @@ class ObjCInterface extends BindingType with ObjCMethods {
       final convertReturn = m.kind != ObjCMethodKind.propertySetter &&
           !returnType.sameDartAndFfiDartType;
 
-      final target = isStatic
-          ? _classObject.name
-          : convertDartTypeToFfiDartType(
-              w,
-              'this',
-              objCRetain: m.consumesSelf,
-              objCAutorelease: false,
-            );
       final msgSendParams =
           m.params.map((p) => p.type.convertDartTypeToFfiDartType(
                 w,

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

@@ -15,8 +15,8 @@ import 'writer.dart';
 final _logger = Logger('ffigen.code_generator.objc_methods');
 
 mixin ObjCMethods {
-  final _methods = <String, ObjCMethod>{};
-  final _order = <String>[];
+  Map<String, ObjCMethod> _methods = <String, ObjCMethod>{};
+  List<String> _order = <String>[];
 
   Iterable<ObjCMethod> get methods =>
       _order.map((name) => _methods[name]).nonNulls;
@@ -97,6 +97,20 @@ mixin ObjCMethods {
       parent: w.topLevelUniqueNamer);
 
   void sortMethods() => _order.sort();
+
+  void filterMethods(bool Function(ObjCMethod method) predicate) {
+    final newOrder = <String>[];
+    final newMethods = <String, ObjCMethod>{};
+    for (final name in _order) {
+      final method = _methods[name];
+      if (method != null && predicate(method)) {
+        newMethods[name] = method;
+        newOrder.add(name);
+      }
+    }
+    _order = newOrder;
+    _methods = newMethods;
+  }
 }
 
 enum ObjCMethodKind {

pkgs/ffigen/lib/src/header_parser/sub_parsers/objcinterfacedecl_parser.dart

@@ -131,10 +131,6 @@ void _parseProperty(
     return;
   }
 
-  if (!config.objcInterfaces.shouldIncludeMember(itfDecl, fieldName)) {
-    return;
-  }
-
   final dartDoc = getCursorDocComment(cursor);
 
   final propertyAttributes =
@@ -221,10 +217,6 @@ ObjCMethod? parseObjCMethod(clang_types.CXCursor cursor, Declaration itfDecl,
     return null;
   }
 
-  if (!filters.shouldIncludeMember(itfDecl, methodName)) {
-    return null;
-  }
-
   final method = ObjCMethod(
     builtInFunctions: objCBuiltInFunctions,
     originalName: methodName,

pkgs/ffigen/lib/src/visitor/apply_config_filters.dart

@@ -36,12 +36,25 @@ class ApplyConfigFiltersVisitation extends Visitation {
       _visitImpl(node, config.macroDecl);
 
   @override
-  void visitObjCInterface(ObjCInterface node) =>
-      _visitImpl(node, config.objcInterfaces);
+  void visitObjCInterface(ObjCInterface node) {
+    node.filterMethods(
+        (m) => config.objcInterfaces.shouldIncludeMember(node, m.originalName));
+    _visitImpl(node, config.objcInterfaces);
+  }
 
   @override
-  void visitObjCProtocol(ObjCProtocol node) =>
-      _visitImpl(node, config.objcProtocols);
+  void visitObjCProtocol(ObjCProtocol node) {
+    node.filterMethods((m) {
+      // TODO(https://github.com/dart-lang/native/issues/1149): Support class
+      // methods on protocols if there's a use case. For now filter them. We
+      // filter here instead of during parsing so that these methods are still
+      // copied to any interfaces that implement the protocol.
+      if (m.isClassMethod) return false;
+
+      return config.objcProtocols.shouldIncludeMember(node, m.originalName);
+    });
+    _visitImpl(node, config.objcProtocols);
+  }
 
   @override
   void visitUnnamedEnumConstant(UnnamedEnumConstant node) =>

pkgs/ffigen/test/native_objc_test/block_config.yaml

@@ -14,6 +14,7 @@ typedefs:
     - FloatBlock
     - DoubleBlock
     - Vec4Block
+    - SelectorBlock
     - VoidBlock
     - ObjectBlock
     - NullableObjectBlock

pkgs/ffigen/test/native_objc_test/block_test.dart

@@ -27,6 +27,7 @@ typedef ListenerBlock = ObjCBlock_ffiVoid_IntBlock;
 typedef FloatBlock = ObjCBlock_ffiFloat_ffiFloat;
 typedef DoubleBlock = ObjCBlock_ffiDouble_ffiDouble;
 typedef Vec4Block = ObjCBlock_Vec4_Vec4;
+typedef SelectorBlock = ObjCBlock_ffiVoid_objcObjCSelector;
 typedef ObjectBlock = ObjCBlock_DummyObject_DummyObject;
 typedef NullableObjectBlock = ObjCBlock_DummyObject_DummyObject1;
 typedef NullableStringBlock = ObjCBlock_NSString_NSString;
@@ -162,6 +163,19 @@ void main() {
       });
     });
 
+    test('Selector block', () {
+      late String sel;
+      final block = SelectorBlock.fromFunction((Pointer<ObjCSelector> x) {
+        sel = x.toDartString();
+      });
+
+      block('Hello'.toSelector());
+      expect(sel, 'Hello');
+
+      BlockTester.callSelectorBlock_(block);
+      expect(sel, 'Select');
+    });
+
     test('Object block', () {
       bool isCalled = false;
       final block = ObjectBlock.fromFunction((DummyObject x) {

pkgs/ffigen/test/native_objc_test/block_test.h

@@ -35,6 +35,7 @@ typedef float (^FloatBlock)(float);
 typedef double (^DoubleBlock)(double);
 typedef Vec4 (^Vec4Block)(Vec4);
 typedef void (^VoidBlock)();
+typedef void (^SelectorBlock)(SEL);
 typedef DummyObject* (^ObjectBlock)(DummyObject*);
 typedef DummyObject* _Nullable (^NullableObjectBlock)(DummyObject* _Nullable);
 typedef NSString* _Nullable (^NullableStringBlock)(NSString* _Nullable);
@@ -64,6 +65,7 @@ typedef void (^NoTrampolineListenerBlock)(int32_t, Vec4, const char*);
 + (float)callFloatBlock:(FloatBlock)block;
 + (double)callDoubleBlock:(DoubleBlock)block;
 + (Vec4)callVec4Block:(Vec4Block)block;
++ (void)callSelectorBlock:(SelectorBlock)block;
 + (DummyObject*)callObjectBlock:(ObjectBlock)block NS_RETURNS_RETAINED;
 + (nullable DummyObject*)callNullableObjectBlock:(NullableObjectBlock)block
     NS_RETURNS_RETAINED;

pkgs/ffigen/test/native_objc_test/block_test.m

@@ -153,6 +153,10 @@ + (Vec4)callVec4Block:(Vec4Block)block {
   return block(vec4);
 }
 
++ (void)callSelectorBlock:(SelectorBlock)block {
+  block(sel_registerName("Select"));
+}
+
 + (DummyObject*)callObjectBlock:(ObjectBlock)block NS_RETURNS_RETAINED {
   return block([DummyObject new]);
 }

pkgs/ffigen/test/native_objc_test/method_filtering_test.dart

@@ -45,6 +45,11 @@ void main() {
         expect(bindings, contains('includedProtocolMethod'));
         expect(bindings, isNot(contains('excludedProtocolMethod')));
       });
+
+      test('transitive deps', () {
+        expect(bindings, isNot(contains('TransitiveInterface')));
+        expect(bindings, isNot(contains('someTransitiveMethod')));
+      });
     });
   });
 }

pkgs/ffigen/test/native_objc_test/method_filtering_test.m

@@ -4,11 +4,16 @@
 
 #import <Foundation/NSObject.h>
 
+@interface TransitiveInterface : NSObject {}
++ (instancetype)someTransitiveMethod: (double)arg;
+@end
+
 @interface MethodFilteringTestInterface : NSObject {}
 + (instancetype)includedStaticMethod;
 + (instancetype)excludedStaticMethod;
 - (instancetype)includedInstanceMethod: (int32_t)arg with: (int32_t)otherArg;
-- (instancetype)excludedInstanceMethod: (int32_t)arg with: (int32_t)otherArg;
+- (instancetype)excludedInstanceMethod: (int32_t)arg
+    with: (TransitiveInterface*)otherArg;
 @property (assign) NSObject* includedProperty;
 @property (assign) NSObject* excludedProperty;
 @end

pkgs/ffigen/test/native_objc_test/protocol_test.dart

@@ -73,6 +73,10 @@ void main() {
 
         // Method from a protocol that isn't included by the filters.
         expect(protocolImpl.fooMethod(), 2468);
+
+        // Class methods.
+        expect(ObjCProtocolImpl.requiredClassMethod(), 9876);
+        expect(ObjCProtocolImpl.optionalClassMethod(), 5432);
       });
 
       test('Unimplemented method', () {
@@ -94,6 +98,9 @@ void main() {
         expect(() => protocolImpl.optionalMethod_(structPtr.ref),
             throwsA(isA<UnimplementedOptionalMethodException>()));
         calloc.free(structPtr);
+
+        expect(() => ObjCProtocolImpl.unimplementedOtionalClassMethod(),
+            throwsA(isA<UnimplementedOptionalMethodException>()));
       });
     });
 

pkgs/ffigen/test/native_objc_test/protocol_test.h

@@ -25,6 +25,17 @@ typedef struct {
 @optional
 - (void)voidMethod:(int32_t)x;
 
+// Class methods aren't supported in protocol implementation from Dart, but they
+// are still codegenned for any native interfaces that implement this protocol.
+@required
++ (int32_t)requiredClassMethod;
+
+@optional
++ (int32_t)optionalClassMethod;
+
+@optional
++ (int32_t)unimplementedOtionalClassMethod;
+
 @end
 
 

pkgs/ffigen/test/native_objc_test/protocol_test.m

@@ -49,6 +49,14 @@ - (int32_t)fooMethod {
   return 2468;
 }
 
++ (int32_t)requiredClassMethod {
+  return 9876;
+}
+
++ (int32_t)optionalClassMethod {
+  return 5432;
+}
+
 @end