Don't require objects to always be wrapped in Arc<>
#1672
Conversation
uniffi_macros/src/object.rs
Outdated
| } | ||
|
|
||
| fn try_lift(v: Self::FfiType) -> ::uniffi::Result<Self> { | ||
| panic!("Can't lift") |
There was a problem hiding this comment.
This is the main issue with this change. If we implement FfiConverter on a type, then UniFFI-exported functions can input that type as an argument. However, it's not possible to input the object type directly if it's already wrapped in an arc (into_inner doesn't seem right).
Right now this is just implemented as a panic, which is unfortunate because if a library does try to input an object type it would be better to catch the issue at compile time. I don't think there's a static assertion to check that a function is never called though.
One solution would be to split up FfiConverter into 2 traits: one for lifting and one for lowering. That seems like a big change though, is there anything easier?
bendk
force-pushed
the
objects-without-arcs
branch
from
980eaf5
to
cf1bffe
Compare
|
I agree that this should be supported, but I don't think anything should change / needs to change about the traits. This can be 100% a function of the scaffolding code generation: If a |
That's a neat idea, but I'm also concerned about non-constructurs that return interfaces. In particular, if we want to use interfaces as errors, it's going to be annoying to have to add |
|
I see what you mean. Well in that case, I'm fully in support of splitting the trait in two, which I wanted anyways. We could allow users to opt into this for regular types as well, to reduce the amount of generated code when a certain type is either only ever used as (part of) a parameter, or only ever used as (part of) a return value. |
|
I wonder what the right split is. In addition to |
|
|
bendk
force-pushed
the
objects-without-arcs
branch
from
cf1bffe
to
0be1589
Compare
bendk
changed the title
Arc<>Arc<>
|
We have split up the A step further would be to implement |
bendk
force-pushed
the
objects-without-arcs
branch
from
0be1589
to
373cac6
Compare
|
Another option would be to restrict this even more and only support it for constructors. That feels like the least magical of all, but it would require adding a new FFI trait, maybe we could call it |
I'm torn here. Allowing all methods to return an object and have the arc wrapping done does seem convenient and might even mean consumers need to make fewer changes to their existing APIs to allow UniFFI to expose it. I don't think it will mis-lead too many people - our docs make it quite clear everything is an Arc. OTOH, I am sympathetic to "explicit is better than implicit", so expect people will disagree and respect that. However, I am struggling a little with the concept that is makes sense for constructors but not elsewhere - both arguments above apply equally in both cases IMO. (See also #1063 where there was strong objection to doing this for constructors. My memory is faulty, because for some reason I thought we supported that everywhere except constructors, but I don't think that's true) |
I don't disagree, but one detail is that if we want to support returning That's an argument for the two extreme cases: either limit the auto-arc-wrapping to constructors or support it anytime we lift a value. |
| @@ -75,7 +75,7 @@ impl MyObject { | |||
| // ... | |||
| } | |||
|
|
|||
| // `Arc<Self>` is also supported | |||
| // Returning objects is also supported, either as `Self` or `Arc<Self>` | |||
bendk
force-pushed
the
objects-without-arcs
branch
from
373cac6
to
383360e
Compare
I don't know, I found it useful. In fact, it made me think I was doing the same thing. Why only implement The only drawback I can see is that it allows users to pass in objects into callback interface methods without wrapping them in an arc, but why stop that? I can't see any real footguns here. Maybe some users won't understand what UniFFI is doing behind the scenes, but that seems fine to me. |
bendk
force-pushed
the
objects-without-arcs
branch
from
383360e
to
8040ed9
Compare
|
So whether we can is different to whether we should :) A big part of that for me is how magic it seems and whether it helps or hurts the mental model users have with uniffi. IMO, the direct question here (returning objects from constructors without the Arc<>) has clear utility and IMO isn't going to confuse anyone's mental model of the world. The fact functions also get this "for free" unless we take additional action to prevent it can be classed a "happy accident" - it might come in useful and I don't really see footguns there, so I'm not sure it's worth additional effort to not support it. In the same way, I doubt I'd be that keen on spending additional effort to support it if it didn't come for free. It could certainly be argued that these limitations are going to be confusing for users, and I suspect a good argument could persuade me if anyone feels strongly about it - but at face value I currently thing it's fine. However, moving beyond this starts to move into uncertain territory for me. So without thinking too much about it, I'm kinda +1 on adding this capability to constructors, +1 on leaving a happy accident that other functions get the simple case for free too, and -0.5 on expanding this too far at this stage, because it's much easier to add this capability in the future than it would be to take it away if it turns out to be confusing for people. |
For constructors and other methods that return an object, UniFFI can wrap the object in an `Arc<>` just fine, the library doesn't need to do this. Allowing the library to skip the wrapping can improve ergonomics, especially if we start to allow interfaces as errors.
bendk
force-pushed
the
objects-without-arcs
branch
from
8040ed9
to
f76f5a3
Compare
|
After talking with Mark, I'm not sure if implementing Going to merge this one since that change was already approved. |
Up until uniffi 0.26 it was not possible to send objects across the boundary unless they were wrapped in an `Arc<>`, see mozilla/uniffi-rs#1672 The bindings generator used in complement-crypto only supports up to uniffi 0.25, meaning having a function which returns objects ends up erroring with: ``` error[E0277]: the trait bound `TaskHandle: LowerReturn<UniFfiTag>` is not satisfied --> bindings/matrix-sdk-ffi/src/room_directory_search.rs:109:10 | 109 | ) -> TaskHandle { | ^^^^^^^^^^ the trait `LowerReturn<UniFfiTag>` is not implemented for `TaskHandle` | = help: the following other types implement trait `LowerReturn<UT>`: <bool as LowerReturn<UT>> <i8 as LowerReturn<UT>> <i16 as LowerReturn<UT>> <i32 as LowerReturn<UT>> <i64 as LowerReturn<UT>> <u8 as LowerReturn<UT>> <u16 as LowerReturn<UT>> <u32 as LowerReturn<UT>> and 133 others error[E0277]: the trait bound `TaskHandle: LowerReturn<_>` is not satisfied --> bindings/matrix-sdk-ffi/src/room_directory_search.rs:82:1 | 82 | #[uniffi::export(async_runtime = "tokio")] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `LowerReturn<_>` is not implemented for `TaskHandle` | = help: the following other types implement trait `LowerReturn<UT>`: <bool as LowerReturn<UT>> <i8 as LowerReturn<UT>> <i16 as LowerReturn<UT>> <i32 as LowerReturn<UT>> <i64 as LowerReturn<UT>> <u8 as LowerReturn<UT>> <u16 as LowerReturn<UT>> <u32 as LowerReturn<UT>> and 133 others ``` This PR wraps the offending function in an `Arc<>` to make it uniffi 0.25 compatible, which unbreaks complement crypto.
Up until uniffi 0.26 it was not possible to send objects across the boundary unless they were wrapped in an `Arc<>`, see mozilla/uniffi-rs#1672 The bindings generator used in complement-crypto only supports up to uniffi 0.25, meaning having a function which returns objects ends up erroring with: ``` error[E0277]: the trait bound `TaskHandle: LowerReturn<UniFfiTag>` is not satisfied --> bindings/matrix-sdk-ffi/src/room_directory_search.rs:109:10 | 109 | ) -> TaskHandle { | ^^^^^^^^^^ the trait `LowerReturn<UniFfiTag>` is not implemented for `TaskHandle` | = help: the following other types implement trait `LowerReturn<UT>`: <bool as LowerReturn<UT>> <i8 as LowerReturn<UT>> <i16 as LowerReturn<UT>> <i32 as LowerReturn<UT>> <i64 as LowerReturn<UT>> <u8 as LowerReturn<UT>> <u16 as LowerReturn<UT>> <u32 as LowerReturn<UT>> and 133 others error[E0277]: the trait bound `TaskHandle: LowerReturn<_>` is not satisfied --> bindings/matrix-sdk-ffi/src/room_directory_search.rs:82:1 | 82 | #[uniffi::export(async_runtime = "tokio")] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `LowerReturn<_>` is not implemented for `TaskHandle` | = help: the following other types implement trait `LowerReturn<UT>`: <bool as LowerReturn<UT>> <i8 as LowerReturn<UT>> <i16 as LowerReturn<UT>> <i32 as LowerReturn<UT>> <i64 as LowerReturn<UT>> <u8 as LowerReturn<UT>> <u16 as LowerReturn<UT>> <u32 as LowerReturn<UT>> and 133 others ``` This PR wraps the offending function in an `Arc<>` to make it uniffi 0.25 compatible, which unbreaks complement crypto.
Since mozilla#1672 was merged, we support both `Arc<Self>` and `Self` return types.
When reviewing #1662 I felt like I would be very annoyed to always have to wrap error objects with
Arc::new(), this is an attempt to see if can avoid that. It contains a generalFfiConverterimplementation for the struct itself, that simply wraps the value inArc::new()then forwards the call to the normalArc<>impl.For constructors and other methods that return an object, UniFFI can wrap the object in an
Arc<>just fine, the library doesn't need to do this. Allowing the library to skip the wrapping can improve ergonomics, especially if we start to allow interfaces as errors.