Allow Rust traits to be exposed as an interface #1308
Conversation
|
Uhh... This already works without |
The goal would be to support a Trait that's implemented by multiple types and have allow the foreign code to have something that works like |
|
In the spirit of early feedback, here's a patch that makes the FfiConverter code add the box so user's don't have to: https://github.com/bendk/uniffi-rs/pull/new/traits-unboxed. I also wonder about avoiding the box altogether. AFAICT, the reason we need it is because The idea of combining this new thing with
How could that last part work? We already have code that inputs a callback interface handle from the foreign side and creates a This makes me think that maybe the |
Yeah - I think there is real scope here - ultimately, I think the only real difference will be whether the code expects the trait to already exist, or whether uniffi needs to generate it. Thanks for that tweak - I'll play with it more later next week. |
I tweaked the callbacks example to try and move towards this - there's now a "SimCard" trait (and I think that probably means my new fixture can die). My next step is to work out how to have the foreign code also implement this new trait, which IIUC, is what you described above.
I suspect it might play out slightly different to that and we might even be able to just remove CallbackInterface entirely - but that should become clearer once we flesh out some examples. |
mhammond
force-pushed
the
traits
branch
2 times, most recently
from
ded3a02 to
7fa707d
Compare
Slight tweak on Mark's original code to remove the requirement that users return `Arc<Box<dyn Trait>>` for UniFFI functions that return trait objects. Now they only need to return `Arc<dyn Trait>`, which corresponds to how regular objects work. We still add a box to avoid the fat pointer issue, but now it's added by the FfiConverter code instead. Also, the Box is now wrapping the Arc instead of the other way around, I'm not sure if that makes any difference. I think this basic approach should work, but I'm not completely confident in how the pointers are being handled. I also think there's a a way to cleanup the way we handle pointer types slightly that would consolidate some redundant code between regular objects and trait objects.
| /// the `lower()` or `write()` method of this impl. | ||
| fn try_lift(v: Self::FfiType) -> Result<Self::RustType> { | ||
| // Use Box::leak to avoid dropping the `Arc<T>` that is owned by the foreign-language code. | ||
| let foreign_arc = Box::leak(unsafe { Box::from_raw(v as *mut std::sync::Arc<T>) }); |
There was a problem hiding this comment.
I guess this is where the leak comes from? This really makes me think Arc<Box<T>> is the way to go and maybe it's more precise to call this Arc<Box<dyn Trait>>. I think I can even wrap my head around it because it's closer the the way we both intuitively thought that things worked. Box<dyn Trait> is a fat pointer, where one points to the underlying T and the other points to the vtable. Then Arc<Box<dyn trait>> let's you share that between the foreign and Rust code, with the nice side benefit it's just a regular, skinny, pointer.
In fact, could you then just use the existing Arc<T> impl?
I think whatever way we choose to implement this, we need good documentation about how everything is layed out. I know I'm not going to remember in a couple months.
There was a problem hiding this comment.
I'm still a bit confused by this TBH - I think I need to better understand the leak.
My only real problem with Arc<Box<dyn T>> is the ergonomics, and I'm not even sure how much of a problem I think it is :) But having an argument be foo: Arc<Box<impl Foo>> seems like it's going to wind up with lots of &**foo and other gymnastics which makes the code look messy and just surfaces the limitations of UniFFI rather than looking "natural".
So I'm still trying to think about ways we can avoid this and use the slightly more natural Arc<impl Foo> in the signatures of functions being exposed. Although we discussed how ugly and hacky it would be, and I expressed hostility to the idea, I am starting to wonder how bad it would be to try and pull some of the tricks mentioned in this discussion about std::raw::TraitObject? Or whether we can make the current model not leak (which seems like it should be possible, although might end up being very heavy on many small, short-lived allocations). Or something :) WDYT?
I think whatever way we choose to implement this, we need good documentation about how everything is layed out.
Agreed - mermaid FTW I guess :)
There was a problem hiding this comment.
I think I need to better understand the leak.
I can't actually reproduce a leak in that lower/lift implementation - there is a leak in my branch, but it doesn't look like it's there - so let me get more info before we start exploring the more radical options.
There was a problem hiding this comment.
My only real problem with
Arc<Box<dyn T>>is the ergonomics, and I'm not even sure how much of a problem I think it is :) But having an argument befoo: Arc<Box<impl Foo>>seems like it's going to wind up with lots of&**fooand other gymnastics which makes the code look messy and just surfaces the limitations of UniFFI rather than looking "natural".
That's a good point. I wonder if we could write our own wrapper type that uses Arc<Box<dyn T>> internally and has Deref impl that makes it easy to use. Or maybe the type should wrap Box<dyn T> in a way that makes it easy to use when wrapped in an Arc.
|
This is now in #1457 |
We've a use-case where we'd like to have a .udl describe an interface that's actually implemented on the Rust side of the world as a trait rather than a struct. Hopefully the motivation will be clear when looking at the new fixture.
This actually turned out reasonably well, but there is still quite a bit of cleanup to do, and also other edge-cases to explore. I'd also welcome alternative strategies, and something tells me that there's some synergy now between traits and callback interfaces, but I'm yet to explore that. In the meantime, I thought I'd get this up for early feedback.