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

Allow Box and Arc for all interfaces #1576

Closed
bendk opened this issue Jun 2, 2023 · 3 comments
Closed

Allow Box and Arc for all interfaces #1576

bendk opened this issue Jun 2, 2023 · 3 comments
Labels
enhancement New feature or request uniffi

Comments

@bendk
Copy link
Contributor

bendk commented Jun 2, 2023

Right now, callback interfaces are always boxed, while trait interfaces and regular interfaces are put inside an Arc. Why not support both options for all types? If I have a #[derive(Object)] around my struct, I should be able to input it as either a Box<T> or an Arc<T> and similarly for trait interfaces and callback interfaces.

Allowing Arc for callback interfaces would allow them to be shared in the Rust code. It would also allow them to be passed back into the foreign side (although as the docs note, this is not a likely use-case).

Allowing Box for objects and trait interfaces would be more efficient if they weren't shared in the Rust code.

Most importantly, this would allow for more flexible APIs. If a function would naturally input a Box<T> then it's not great that UniFFI forces you to change that to Arc<T>. If there are existing Rust consumers of the API that already are passing in a Box<T> then they all need to be changed.

One issue that makes this more complicated is dropping references. Box and Arc require different code when they're dropped, so how can we handle this? One way would be to use some sort of fat pointer -- maybe a pointer to the drop function plus a pointer to the object. Maybe we could avoid that by generalizing the current restriction on callback interfaces: Boxes can't be passed back across the FFI. That way the scaffolding drop function would only be called with a raw Arc pointer. I haven't totally thought this through though.

┆Issue is synchronized with this Jira Task
┆Issue Number: UNIFFI-278

@mhammond
Copy link
Member

mhammond commented Jun 2, 2023

Right now, callback interfaces are always boxed, while trait interfaces and regular interfaces are put inside an Arc.

Trait interfaces are a Box<Arc> (not that it changes what you are saying I don't think)

Allowing Arc for callback interfaces would allow them to be shared in the Rust code.

TBH, I still think that we should deprecate callbacks instead of enhancing them - we'd get the above for free.

Most importantly, this would allow for more flexible APIs. If a function would naturally input a Box then it's not great that UniFFI forces you to change that to Arc

What is the use-case for a Rust API to prefer Box<T> when it's not required? What APIs would specify that if T is a struct?

Box and Arc require different code when they're dropped, so how can we handle this? One way would be to use some sort of fat pointer -- maybe a pointer to the drop function plus a pointer to the object.

Doesn't our trait support already handle that?

@bendk
Copy link
Contributor Author

bendk commented Jun 5, 2023

TBH, I still think that we should deprecate callbacks instead of enhancing them - we'd get the above for free.
What is the use-case for a Rust API to prefer Box when it's not required? What APIs would specify that if T is a struct?

Yeah, this is probably right. I thought there would be some nice symmetry with a callback interface mirroring an object interface and a trait interface being either one. But it's simpler to just deprecate and eventually eliminate callback interfaces.

At that point, this reduces to: it would be nice to be able to use either Arc<dyn T> or Box<dyn T> for trait interfaces in your exported APIs.

@badboy badboy added the enhancement New feature or request label Jun 14, 2023
@bendk
Copy link
Contributor Author

bendk commented Sep 5, 2023

I don't think even that last use case makes sense, let's just close this one.

@bendk bendk closed this as completed Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request uniffi
Projects
None yet
Development

No branches or pull requests

4 participants