Foreign trait interface implementations can break when libraries expect a trait to be Rust-only

[bendk]

In 0.25.0 this worked and the bar argument was treated as a normal ByRef argument.

#[uniffi::export]
pub trait Foo: Send + Sync {
     fn foo(&self, bar: &Bar);
}

However this currently fails in main because we now also generate a callback interface for each trait. ByRef arguments are not supported there.

[bendk]

Maybe we could add support for ByRef args in callback interfaces:

• Primitive types can be copied
• Types that are serialized to RustBuffer, probably don't need any special treatment.
• For objects/trait interfaces, could we clone the Arc?

If we could get this to work, then there's still a question of if the semantics are correct. If the Rust code is passing a reference they find copying the data into buffer or cloning the Arc surprising.

[mhammond]

a question of if the semantics are correct.

Yeah, I'm a bit torn here. I don't like breaking consumers, but OTOH, I don't think we ever promised this would work, did we? It seems we didn't have test coverage for it otherwise we'd have noticed it before now. I agree that making it look like we are passing by reference just for us to turn it into a copy behind their back isn't ideal and "un-rusty". Is supporting this something we will regret as a foot-gun in a few years?

[bendk]

Maybe we could add an attribute that makes a trait interface Rust-only?

It's hard to anticipate, but if we created more differences between object interfaces and callback interfaces, allowing consumers to opt-in a Rust-only trait interface could be a good solution.

[skeet70]

We have code that does this, and don't care about external consumers being able to implement our traits. They're used internally to cement the API and externally they let a consumer specify they need that interface (which our library presents multiple implementers of). If supporting callbacks with ByRef isn't a semantically good direction, we would want the ability to label the trait so a callback interface isn't generated for it.

[mhammond]

yeah, we do need an attribute here.

[mhammond]

Maybe we could add support for ByRef args in callback interfaces: ... If we could get this to work, then there's still a question of if the semantics are correct. If the Rust code is passing a reference they find copying the data into buffer or cloning the Arc surprising.

Actually I think this is fine and preferable. Rust code calling the trait will not be impacted at all. Foreign code calling in will mean we have a newly created Bar we pass a reference to, but I don't see how that's surprising to anyone. Am I missing other cases?

What's involved in supporting byrefs there?

[skeet70]

I pushed a use case fixture byref-trait to my branch for this. I was originally trying to add an attribute to disable the automatic callback trait (while wondering why it was added in the first place vs using the existing callback_interface in places people wanted foreign implementations), but hadn't made it very far. Looking now into what's required to support ByRef in the current auto-callback interface process.

[mhammond]

Actually I think this is fine and preferable.

Sorry I didn't note this before, but I was a bit confused here. I'm not sure it does make sense for a callback interface to be able to take references - these are only implemented on the foreign side, so I agree with Ben's comment that it would be odd for a callback interface to take an reference when it must then clone the referent. OTOH though, I guess that given we appear to be moving towards callback interfaces being avoided in favour of traits, I guess I also can't see why it would be particularly bad for that to happen if it helps this scenario. I'll try and have a look at the branch over the weekend.

[mhammond]

A complication here is that traits are always lifted as callbacks - trying to round-trip traits ends up with foreign wrappers each time. This is quite unlike interfaces which manage to round-trip the Arc correctly, and isn't ideal :)

It seems like this attribute will need to change the entire impl of the ffi for the trait - to either be a "callback" or an "object" as a "short-term" fix?

[mhammond]

Ben and I chatted about this briefly and agreed the comment I wrote above is what we should do in the short term, and longer term we might be able to implement something better for both cases and make the new attribute a no-op or similar.

@skeet70 does the comment above make sense and is this still something you are looking to pursue? Let us know if we can help.

[skeet70]

It seems like this attribute will need to change the entire impl of the ffi for the trait - to either be a "callback" or an "object" as a "short-term" fix?

Those words make sense conceptually to me but don't connect to code changes immediately for me yet. It is something I'm still trying to work on but I've had other things take priority at work for the moment. I'll try to come back to it in a few days and see if I can figure out what code changes that statement implies.

[mhammond]

Great! I think it will entail that for traits, instead of generating https://github.com/mozilla/uniffi-rs/blob/main/uniffi_macros/src/export/trait_interface.rs#L17 for the scaffolding we instead will generate closer to https://github.com/mozilla/uniffi-rs/blob/main/uniffi_macros/src/object.rs#L9. I'm sure there will be some devil in the detail though.

[bendk]

When Mark and I discussed this, we thought there should be 3 options for your exporting traits:

1. Only support Rust implementations (trait interfaces in 0.25.x)
2. Only support foreign implementations (callback interfaces)
3. Support both (something new that we would find a new name for)

This means that the new interface could would be a new feature that users would opt-in to rather than one that we automatically assume for all trait interfaces.

One question is the ergonomics of it all. Maybe we could use the words "with callback interface", meaning you're taking an existing trait interface and adding callback interface functionality to it:

UDL:

[TraitWithCallbackInteface]
interface Foo {
   ... 
};

proc-macros:

#[uniffi::export(with_callback_interface)]
pub trait Foo {
  ...
}