Make foreign trait implementations opt-in (#1827) #1902

bendk · 2023-12-14T21:49:18Z

This changes the default behavior for exported trait interfaces to not generate a foreign callback interface implementation for them. Users can opt-in to this using the TraitWithCallbackInterface attribute for UDL and the with_callback_interface for proc-macros.

The main reason for this change is that callback interfaces and trait interfaces aren't completely compatible. For example, ByRef is not supported for callback interfaces, but it is supported for trait interfaces that only have Rust implementations.

bendk · 2023-12-14T21:50:53Z

I think this is the simplest way to fix the issue and making a distinction between these two things will be good going forward. Totally open to bikeshedding on the attribute names.

mhammond · 2023-12-23T03:49:48Z

This patch is fantastic, loads of tests and exactly what needs to be done here, thanks!

And sorry for the slow followup to your question in #1827, but as we discussed, I'm allowed to bike-shed names etc :)

For the UniFFI consumer: I think we should move away from "callback". That's not really accurate for many use-cases with generic traits. Maybe just "foreign"? I propose for UDL:

[ForeignTrait]
interface SimCard { ... };

and something like #[uniffi::export(with_foreign)] for procmacros? There's no good reason to be consistent with #[uniffi::export(callback_interface)]; that should fade into obscurity and maybe formally deprecated - callbacks and procmacros are kinda "recent" anyway and I suspect not widely used - @jplatte, do you use them?

Internally: "callback" seems fine, "foreign" might marginally be better for consistency, whatever :) But it still seems very verbose - what about if you dropped the word "interface" everywhere if comes after "callback" in this patch - eg, all CallBackInterface/callback_interface/CALLBACK_INTERFACE etc just need "callback"? (or "foreign" ;)

Internally is just nit-picking though, the "public" spelling is the important bit. I'd like to see other bike-sheds :)

bendk · 2023-12-27T16:30:53Z

For the UniFFI consumer: I think we should move away from "callback". That's not really accurate for many use-cases with generic traits. Maybe just "foreign"?

This sounds good to me. The one tweak I'd suggest is with the UDL keyword. [ForeignTrait] could imply that it's foreign-only. I'd rather something that feels more additive. What about [AllowForeign] or [WithForeign]?

mhammond · 2023-12-27T16:34:13Z

Sounds great! I think I mildly prefer with over allow - the latter tends to imply some other action is required to actually enable it. Regardless though, it would be obviously ideal if procmacros and udl attributes used identical terminology.

mhammond · 2023-12-27T16:36:42Z

OTOH, I'm not sure plain-old [WithForeign] is perfect stand-alone in a UDL as there's no indication it's a trait - were you thinking [Trait, WithForeign]? If not, maybe [TraitWithForeign]?

bendk · 2023-12-27T16:49:15Z

OTOH, I'm not sure plain-old [WithForeign] is perfect stand-alone in a UDL as there's no indication it's a trait - were you thinking [Trait, WithForeign]? If not, maybe [TraitWithForeign]?

Good point. I think either would work, but I'm currently liking [Trait, WithForeign] the best.

skeet70 · 2024-01-03T18:29:48Z

uniffi_macros/src/export/trait_interface.rs

+    let try_lift = if with_callback_interface {
+        let trait_impl_ident = callback_interface::trait_impl_ident(&trait_name);
+        quote! {
+            fn try_lift(v: Self::FfiType) -> ::uniffi::deps::anyhow::Result<::std::sync::Arc<Self>> {
+                Ok(::std::sync::Arc::new(<#trait_impl_ident>::new(v as u64)))
+            }
+        }
+    } else {
+        quote! {
+            fn try_lift(v: Self::FfiType) -> ::uniffi::deps::anyhow::Result<::std::sync::Arc<Self>> {
+                unsafe {
+                    Ok(*::std::boxed::Box::from_raw(v as *mut ::std::sync::Arc<Self>))
+                }
+            }
+        }
+    };


This is definitely the part I wasn't going to figure out how to do :P.

skeet70 · 2024-01-03T18:36:22Z

Any of the names seem like they work to me. The one that'd make it the clearest to me would be [Trait, AllowForeignImpl] and uniffi::export(allow_foreign_impl).

bendk · 2024-01-04T21:35:33Z

I ended up going with [WithForeign]. I don't think there's a big difference between the two, but it's shorter and also users might get confused and think that [AllowForeignImpl] would not have an effect if they didn't actually implement anything on the foreign side.

jplatte · 2024-01-04T21:39:42Z

callbacks and procmacros are kinda "recent" anyway and I suspect not widely used - @jplatte, do you use them?

Not sure if this Q is still relevant but yes, #[uniffi::export(callback_interface)] is used by the Matrix Rust SDK (though I am no longer working on it much as my employment at Element is ending).

mhammond

mhammond · 2024-01-08T19:27:29Z

uniffi_bindgen/src/bindings/python/templates/ObjectTemplate.py

@@ -1,3 +1,4 @@
+# IMP: {{ "{:?}"|format(obj.imp()) }}


nit: either remove this line or make it a little less terse :)

This changes the default behavior for exported trait interfaces to not generate a foreign callback interface implementation for them. Users can opt-in to this using the `WithForeign` attribute for UDL and the `with_foreign` for proc-macros. The main reason for this change is that callback interfaces and trait interfaces aren't completely compatible. For example, `ByRef` is not supported for callback interfaces, but it is supported for trait interfaces that only have Rust implementations.

bendk requested a review from a team as a code owner December 14, 2023 21:49

bendk requested review from jhugman and mhammond and removed request for a team December 14, 2023 21:49

bendk force-pushed the foreign-trait-interfaces-opt-in branch 2 times, most recently from 8e13514 to 5588e5b Compare December 19, 2023 00:27

mhammond added the v0.26 label Dec 21, 2023

bendk mentioned this pull request Dec 21, 2023

0.26.0 Release #1911

Closed

2 tasks

skeet70 approved these changes Jan 3, 2024

View reviewed changes

bendk force-pushed the foreign-trait-interfaces-opt-in branch from 5588e5b to 1010792 Compare January 4, 2024 21:33

bendk force-pushed the foreign-trait-interfaces-opt-in branch from 1010792 to dab13db Compare January 8, 2024 14:52

mhammond approved these changes Jan 8, 2024

View reviewed changes

bendk force-pushed the foreign-trait-interfaces-opt-in branch from dab13db to e70164e Compare January 9, 2024 14:33

bendk merged commit b788e71 into mozilla:main Jan 9, 2024
5 checks passed

mgeisler mentioned this pull request Feb 26, 2024

[uniffi] Add support for custom GroupStateStorage interface awslabs/mls-rs#86

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make foreign trait implementations opt-in (#1827) #1902

Make foreign trait implementations opt-in (#1827) #1902

bendk commented Dec 14, 2023

bendk commented Dec 14, 2023

mhammond commented Dec 23, 2023

bendk commented Dec 27, 2023

mhammond commented Dec 27, 2023

mhammond commented Dec 27, 2023

bendk commented Dec 27, 2023

skeet70 Jan 3, 2024

skeet70 commented Jan 3, 2024 •

edited

Loading

bendk commented Jan 4, 2024

jplatte commented Jan 4, 2024

mhammond left a comment

mhammond Jan 8, 2024

Make foreign trait implementations opt-in (#1827) #1902

Make foreign trait implementations opt-in (#1827) #1902

Conversation

bendk commented Dec 14, 2023

bendk commented Dec 14, 2023

mhammond commented Dec 23, 2023

bendk commented Dec 27, 2023

mhammond commented Dec 27, 2023

mhammond commented Dec 27, 2023

bendk commented Dec 27, 2023

skeet70 Jan 3, 2024

Choose a reason for hiding this comment

skeet70 commented Jan 3, 2024 • edited Loading

bendk commented Jan 4, 2024

jplatte commented Jan 4, 2024

mhammond left a comment

Choose a reason for hiding this comment

mhammond Jan 8, 2024

Choose a reason for hiding this comment

skeet70 commented Jan 3, 2024 •

edited

Loading