Skip to content

[ffigen] Fix protocol inclusion rules #1583

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Sep 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions pkgs/ffigen/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,12 @@
- ObjC objects now include the methods from the protocols they implement. Both
required and optional methods are included. Optional methods will throw an
exception if the method isn't implemented.
- __Breaking change__: Only generate ObjC protocol implementation bindings for
protocols that are included by the config filters. This is breaking because
previously super protocols would automatically get implementation bindings,
rather than just being incorporated into the child protocol. If you want those
implementation bindings, you may need to add the super protocol to your
`objc-protocols` filters.

## 14.0.1

Expand Down
19 changes: 14 additions & 5 deletions pkgs/ffigen/lib/src/code_generator/objc_protocol.dart
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ class ObjCProtocol extends NoLookUpBinding with ObjCMethods {
final superProtocols = <ObjCProtocol>[];
final String lookupName;
late final ObjCInternalGlobal _protocolPointer;
final bool generateBindings;

@override
final ObjCBuiltInFunctions builtInFunctions;
Expand All @@ -23,11 +24,17 @@ class ObjCProtocol extends NoLookUpBinding with ObjCMethods {
String? lookupName,
super.dartDoc,
required this.builtInFunctions,
required this.generateBindings,
}) : lookupName = lookupName ?? originalName,
super(name: name ?? originalName);

@override
BindingString toBindingString(Writer w) {
if (!generateBindings) {
return const BindingString(
type: BindingStringType.objcProtocol, string: '');
}

final protocolMethod = ObjCBuiltInFunctions.protocolMethod.gen(w);
final protocolListenableMethod =
ObjCBuiltInFunctions.protocolListenableMethod.gen(w);
Expand Down Expand Up @@ -154,11 +161,13 @@ ${makeDartDoc(dartDoc ?? originalName)}abstract final class $name {
if (dependencies.contains(this)) return;
dependencies.add(this);

_protocolPointer = ObjCInternalGlobal(
'_protocol_$originalName',
(Writer w) =>
'${ObjCBuiltInFunctions.getProtocol.gen(w)}("$lookupName")')
..addDependencies(dependencies);
if (generateBindings) {
_protocolPointer = ObjCInternalGlobal(
'_protocol_$originalName',
(Writer w) =>
'${ObjCBuiltInFunctions.getProtocol.gen(w)}("$lookupName")')
..addDependencies(dependencies);
}

for (final superProtocol in superProtocols) {
superProtocol.addDependencies(dependencies);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ void _parseSuperType(clang_types.CXCursor cursor, ObjCInterface itf) {

void _parseProtocol(clang_types.CXCursor cursor, ObjCInterface itf) {
final protoCursor = clang.clang_getCursorDefinition(cursor);
final proto = parseObjCProtocolDeclaration(protoCursor);
final proto = parseObjCProtocolDeclaration(protoCursor, ignoreFilter: true);
if (proto != null) {
itf.addProtocol(proto);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,18 +22,19 @@ ObjCProtocol? parseObjCProtocolDeclaration(clang_types.CXCursor cursor,
}

final usr = cursor.usr();
final cachedProtocol = bindingsIndex.getSeenObjCProtocol(usr);
if (cachedProtocol != null) {
return cachedProtocol;
}

final name = cursor.spelling();

final decl = Declaration(usr: usr, originalName: name);
if (!ignoreFilter && !shouldIncludeObjCProtocol(decl)) {
final included = shouldIncludeObjCProtocol(decl);
if (!ignoreFilter && !included) {
return null;
}

final cachedProtocol = bindingsIndex.getSeenObjCProtocol(usr);
if (cachedProtocol != null) {
return cachedProtocol;
}

if (!isApiAvailable(cursor)) {
_logger.info('Omitting deprecated protocol $name');
return null;
Expand All @@ -49,6 +50,12 @@ ObjCProtocol? parseObjCProtocolDeclaration(clang_types.CXCursor cursor,
lookupName: applyModulePrefix(name, config.protocolModule(decl)),
dartDoc: getCursorDocComment(cursor),
builtInFunctions: objCBuiltInFunctions,

// Only generate bindings for the protocol if it is included in the user's
// filters. If this protocol was only parsed because of ignoreFilter, then
// it's being used to add methods to an interface or a child protocol, and
// shouldn't get bindings.
generateBindings: included,
);

// Make sure to add the protocol to the index before parsing the AST, to break
Expand Down
21 changes: 21 additions & 0 deletions pkgs/ffigen/test/native_objc_test/protocol_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ void main() {
// Required instance method from secondary protocol.
final otherIntResult = protocolImpl.otherMethod_b_c_d_(2, 4, 6, 8);
expect(otherIntResult, 20);

// Method from a protocol that isn't included by the filters.
expect(protocolImpl.fooMethod(), 2468);
});

test('Unimplemented method', () {
Expand Down Expand Up @@ -373,5 +376,23 @@ void main() {
expect(blockRetainCount(blockPtr), 0);
}, skip: !canDoGC);
});

test('Filters', () {
// SuperProtocol and FilteredProtocol's methods are included in the
// bindings, but there shouldn't actually be bindings for the protocols
// themselves, because they're not included by the config.
final bindings = File('test/native_objc_test/protocol_bindings.dart')
.readAsStringSync();

expect(bindings, contains('instanceMethod_withDouble_'));
expect(bindings, contains('fooMethod'));

expect(bindings, contains('EmptyProtocol'));
expect(bindings, contains('MyProtocol'));
expect(bindings, contains('SecondaryProtocol'));

expect(bindings, isNot(contains('SuperProtocol')));
expect(bindings, isNot(contains('FilteredProtocol')));
});
});
}
8 changes: 7 additions & 1 deletion pkgs/ffigen/test/native_objc_test/protocol_test.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,11 @@ typedef struct {
@protocol EmptyProtocol
@end

@protocol FilteredProtocol
@required
- (int32_t)fooMethod;
@end


@interface ProtocolConsumer : NSObject
- (NSString*)callInstanceMethod:(id<MyProtocol>)protocol;
Expand All @@ -52,7 +57,8 @@ typedef struct {
@end


@interface ObjCProtocolImpl : NSObject<MyProtocol, SecondaryProtocol>
@interface ObjCProtocolImpl :
NSObject<MyProtocol, SecondaryProtocol, FilteredProtocol>
@end


Expand Down
4 changes: 4 additions & 0 deletions pkgs/ffigen/test/native_objc_test/protocol_test.m
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ - (int32_t)otherMethod:(int32_t)a b:(int32_t)b c:(int32_t)c d:(int32_t)d {
return a + b + c + d;
}

- (int32_t)fooMethod {
return 2468;
}

@end


Expand Down