dart-lang / native

Dart packages related to FFI and native assets bundling.
BSD 3-Clause "New" or "Revised" License
155 stars 43 forks source link

Obj-C protocol conformance is dropped from types #1462

Open stuartmorgan opened 2 months ago

stuartmorgan commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

@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.

kekland commented 2 months ago

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

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