Skip to content
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

Obj-C protocol conformance is dropped from types #1462

Open
stuartmorgan opened this issue Aug 26, 2024 · 7 comments
Open

Obj-C protocol conformance is dropped from types #1462

stuartmorgan opened this issue Aug 26, 2024 · 7 comments
Labels
lang-objective_c Related to Objective C support package:ffigen

Comments

@stuartmorgan
Copy link

While migrating plugin code from native to Dart, at one intermediate stage I had this:

- (instancetype)initWithPlayerItem:(AVPlayerItem *)item
                      viewProvider:(id<FVPViewProvider>)viewProvider
                         AVFactory:(id<FVPAVFactory>)avFactory
                displayLinkFactory:(id<FVPDisplayLinkFactory>)displayLinkFactory;

which was turned into:

FVPVideoPlayer initWithPlayerItem_viewProvider_AVFactory_displayLinkFactory_(
      AVPlayerItem item,
      objc.ObjCObjectBase viewProvider,
      objc.ObjCObjectBase avFactory,
      objc.ObjCObjectBase displayLinkFactory)

Without any of the protocols in the types, it's incredibly easy to call incorrectly, and also much less self-documenting. Ideally Obj-C protocols would be reflected as Dart abstract interfaces that are implements'd.

@liamappelbe liamappelbe added package:ffigen lang-objective_c Related to Objective C support labels Aug 26, 2024
@liamappelbe
Copy link
Contributor

liamappelbe commented Aug 26, 2024

We can experiment with this, but I held off doing it in the initial implementation for a few reasons:

  1. Dart's interface checking is way stricter than ObjC's, so there will probably be use cases that would compile fine in ObjC that don't compile in Dart. I'd need to make sure there's an escape hatch that lets you ignore the check.
  2. Not all interfaces declare which protocols they implement, so I'd need to proactively check which protocols are implemented
  3. I'm not sure how to handle protocols that have optional methods.
    • I suppose I could just make the codegenned protocol empty, so we get some type checking without actually verifying that the methods exist. You still wouldn't be able to call methods on the protocol from Dart land.
  4. Also not sure how to handle args that implement multiple protocols: - (void)method:(id<Foo, Bar>)obj;
    • We could codegen a FooBar interface, but that gets complicated because now that's another interface that the implementation needs to implement. Better solution is probably to just take the first protocol and ignore the rest.

@stuartmorgan
Copy link
Author

1. Dart's interface checking is way stricter than ObjC's, so there will probably be use cases that would compile fine in ObjC that don't compile in Dart.

Do you have an example of this to illustrate the kind of problem that would happen?

2. Not all interfaces declare which protocols they implement, so I'd need to proactively check which protocols are implemented

Can you elaborate on this one? Are you talking about conformsToProtocol: runtime checks? If so, I wouldn't expect that to be reflected in the type system.

3. I'm not sure how to handle protocols that have optional methods.

  • I suppose I could just make the codegenned protocol empty, so we get some type checking without actually verifying that the methods exist. You still wouldn't be able to call methods on the protocol from Dart land.

I think starting with just the type would be a big improvement. (Type and required methods would be even better, for better code completion and Dart-side IDE experience.)

4. Also not sure how to handle args that implement multiple protocols: - (void)method:(id<Foo, Bar>)obj;

I can't remember ever seeing that used in practice, so it probably wouldn't matter much what happened.

@liamappelbe
Copy link
Contributor

An example for both 1 and 2:

@protocol SomeProtocol<NSObject>
@required
- (int32_t)instanceMethod;
@end


@interface UndeclaredImplementation : NSObject
- (int32_t)instanceMethod;
@end

@implementation UndeclaredImplementation : NSObject
- (int32_t)instanceMethod {
  return 123;
}
@end


@interface Consumer : NSObject
- (int32_t)callInstanceMethod:(id<SomeProtocol>)protocol;
@end

@implementation Consumer : NSObject
- (int32_t)callInstanceMethod:(id<SomeProtocol>)protocol {
  return [protocol instanceMethod];
}
@end

int32_t foo() {
  return [[Consumer new] callInstanceMethod: [UndeclaredImplementation new]];
}

This compiles and runs, and foo returns 123 as expected. But the corresponding Dart bindings would not be invocable, (you couldn't write foo in Dart).

All I get from clang is a warning:

protocol_test.m:139:46: warning: sending 'UndeclaredImplementation *' to parameter of incompatible type 'id<SomeProtocol>'
  return [[Consumer new] callInstanceMethod: [UndeclaredImplementation new]];
                                             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
protocol_test.m:129:49: note: passing argument to parameter 'protocol' here
- (int32_t)callInstanceMethod:(id<SomeProtocol>)protocol;
                                                ^

Whether or not this matters depends on whether there are any APIs that do this in practice. I suppose we can just put this behind a config flag, so users can opt out if necessary.

@stuartmorgan
Copy link
Author

stuartmorgan commented Aug 27, 2024

All I get from clang is a warning:

protocol_test.m:139:46: warning: sending 'UndeclaredImplementation *' to parameter of incompatible type 'id<SomeProtocol>'
  return [[Consumer new] callInstanceMethod: [UndeclaredImplementation new]];

It's Objective-C; almost any type violation will just give you a warning. I would not expect to be able to write foo in Dart because foo is wrong.

If "possible to write/run in Obj-C" were our criteria for mirroring, then we would need to throw away the whole type system, not just protocols. This code also builds and runs with nothing but warnings, for instance:

NSArray* notAnArray = @"actually a string";
[self thisParamTypeIsString:notAnArray];

Whether or not this matters depends on whether there are any APIs that do this in practice.

The code in your example isn't an API, it's an implementation. I don't see any problems in the APIs in that code. Unless you mean that UndeclaredImplementation is third-party code that was supposed to explicitly conform to SomeProtocol? If that's the case, couldn't that be trivially worked around by just subclassing UndeclaredImplementation on either side of the language divide, and adding the protocol to the subclass, then downcasting? Or just casting to dynamic?

@kekland
Copy link

kekland commented Aug 27, 2024

I got around this issue by creating a "proxy" class which takes the protocol object in the init function and calls the methods on that object. For example, for FlutterTextureRegistry:

@objc public class FlutterTextureRegistryProxy: NSObject {
    public init(textures: FlutterTextureRegistry) {
        self.textures = textures
    }
    
    let textures: FlutterTextureRegistry;
    
    @objc public func registerTexture(texture: any FlutterTexture) -> Int64 {
        return self.textures.register(texture);
    }
    
    @objc public func textureFrameAvailable(textureId: Int64) {
        return self.textures.textureFrameAvailable(textureId);
    }
    
    @objc public func unregisterTexture(textureId: Int64) {
        return self.textures.unregisterTexture(textureId);
    }
}

But it would be great to have the protocols be generated to avoid this hassle

@liamappelbe
Copy link
Contributor

@kekland I think your work around is solving a different problem. Are you trying to make a protocol invocable? My fix probably won't help with that.

The fix I'm planning will add a Dart interface for each protocol, have the classes implement those interfaces, and then pass around those interfaces instead of ObjCObjectBase. These protocol interfaces would be empty placeholders (at least initially).

I guess if we want to support methods that return protocols, the protocol interface can't actually be abstract. I'll have to make it also be a concrete implementation of the protocol, so that I have something to construct and return. This will also act as an escape hatch for users to work around the type restrictions if necessary. I suppose if we code gen all the required methods, then this would make the protocol invocable.

@liamappelbe liamappelbe added this to the ffigen 15.0.0 milestone Aug 27, 2024
@kekland
Copy link

kekland commented Aug 28, 2024

@liamappelbe ah, you're right, was a long day yesterday :D

but yes, it would be great to have both of those things

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang-objective_c Related to Objective C support package:ffigen
Projects
Status: Todo
Development

No branches or pull requests

4 participants
@stuartmorgan @liamappelbe @kekland and others