Allow Rust traits to be exposed as an interface. #1457

mhammond · 2023-01-23T02:58:15Z

This PR allows Rust traits to be exposed as interfaces, including when they have associated types. It supersedes #1308 (the memory leaks discussed there were easy to resolve once the other shoe dropped for me)

Instead of repeating what I hope are comprehensive docs and tests, please see the docs for this, see the new example (and the coveralls fixture exercises them quite thoroughly).

It has required zero changes to the generated bindings and it all fell out quite cleanly IMO. I've also got a POC in application-services for my specific use-case there, and it works perfectly!

bendk

Very cool and I'm excited to start using it.

I think there's one FfiConverter issue to figure out before we can merge this, but everything else looks good to me.

docs/manual/src/udl/interfaces.md

uniffi_bindgen/src/interface/attributes.rs

bendk · 2023-01-25T20:29:40Z

uniffi_bindgen/src/interface/mod.rs

@@ -999,7 +1007,12 @@ fn convert_type(s: &uniffi_meta::Type) -> Type {
            convert_type(key_type).into(),
            convert_type(value_type).into(),
        ),
-        Ty::ArcObject { object_name } => Type::Object(object_name.clone()),
+        Ty::ArcObject { object_name } => Type::Object {
+            // XXX - needs love for traits and associated types.


Can this comment line be removed?

uniffi_bindgen/src/scaffolding/templates/ObjectTemplate.rs

uniffi_core/src/lib.rs

* Split out the proc-macro code to generate `FfiConverter` implementations and added standalone macros for that. * Removed the `FfiConverter` implementations from the Askama template code. Instead, the templates now invoke the macros. * Removed the blanket `Arc<T>` impl from `uniffi_core` and made it so we invoke a macro for each interface. The reason for this is: * I want to add some more functionality to `FfiConverter`, but don't want to implement it twice and test it two different ways. * Removing the blanket `Arc<T>` implementation allows us to customize the `FfiConverter` code for each interface. I think this makes mozilla#1457 easier. * Everything goes through one code path, which should give us some more confidence as we migrate to proc-macros. This change caused me to fix some bugs and add some features to them.

* Split out the proc-macro code to generate `FfiConverter` implementations and added standalone macros for that. * Removed the `FfiConverter` implementations from the Askama template code. Instead, the templates now invoke the macros. * Made the blanket `Arc<T>` impl in `uniffi_core` depend on a separate `Interface` trait that gets defined by macros. * Added attribute to generate a `try_lift` method for flat errors. This is required to support callback interface errors. The reason for this is: * I want to add some more functionality to `FfiConverter`, but don't want to implement it twice and test it two different ways. * The Interface trait allows us to customize the `Arc<T>` implementation. I plan to push some code that takes advantage of this and I also think it could be useful for mozilla#1457. * Everything goes through one code path, which should give us some more confidence as we migrate to proc-macros. This change caused me to fix some bugs with the macro-code and add some features.

mhammond · 2023-03-16T07:33:14Z

This seems so so close - cargo test -p uniffi-example-traits works, but somehow it has broken most other tests - eg, cargo test -p uniffi-fixture-coverall is failing with errors for plain old structs, eg:

error[E0277]: the trait bound `Patch: Interface<UniFfiTag>` is not satisfied
   --> /mnt/c/src/moz/uniffi-rs/target/debug/build/uniffi-fixture-coverall-82c9c761f5ae2c4d/out/coverall.uniffi.rs:166:1
    |
166 | #[::uniffi::ffi_converter_record(tag = crate::UniFfiTag)]
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Interface<UniFfiTag>` is not implemented for `Patch`
    |

Which confuses me. Out of time for today, but if anyone has a clue please share it with me :)

mhammond · 2023-03-17T03:03:30Z

So this all works and now uses a macro to implement FfiConverter - however, it's a bit different from how objects are done and I haven't even tried to support traits with proc-macros yet as I still need to understand how that works for structs! But it does seem to be in a fairly good place.

bendk

This looks great to me. I left a couple of small bikeshedding comments, but nothing big. I'd be happy to merge this as-is.

uniffi_bindgen/src/interface/object.rs

uniffi_bindgen/src/scaffolding/templates/ObjectTemplate.rs

mhammond

I think this is ready to roll! Ben already approved it, but I think it's worth taking another look given the number of changes which happened underneath me since the last version.

mhammond · 2023-04-24T22:26:19Z

uniffi_bindgen/src/interface/types/mod.rs

+    /// an object with the given name.
+    /// Includes `r#`, traits get a leading `dyn`. If we ever supported associated types, then
+    /// this would also include them.
+    pub fn rust_name_for(&self, name: &str) -> String {


This seems oddly named and vaguely mis-placed, but all other names and places seem worse :)

mhammond requested a review from a team as a code owner January 23, 2023 02:58

mhammond requested review from badboy and removed request for a team January 23, 2023 02:58

mhammond mentioned this pull request Jan 23, 2023

Allow Rust traits to be exposed as an interface #1308

Closed

mhammond force-pushed the traits-types branch from 939624b to b62138a Compare January 23, 2023 03:01

mhammond requested review from bendk and jplatte January 23, 2023 03:02

mhammond force-pushed the traits-types branch from b62138a to 16f6b52 Compare January 23, 2023 03:49

bendk requested changes Jan 25, 2023

View reviewed changes

bendk mentioned this pull request Jan 31, 2023

Make the proc-macro FfiConverter impls canonical #1464

Merged

mhammond force-pushed the traits-types branch 2 times, most recently from b93fcb4 to d44dc2c Compare March 16, 2023 07:30

mhammond force-pushed the traits-types branch 2 times, most recently from 37431e7 to edf92de Compare March 16, 2023 23:29

mhammond marked this pull request as draft March 16, 2023 23:41

mhammond force-pushed the traits-types branch 2 times, most recently from f73f4bc to 0dec0a6 Compare March 17, 2023 03:02

mhammond force-pushed the traits-types branch 2 times, most recently from dcf9e41 to b77c626 Compare March 17, 2023 03:06

badboy mentioned this pull request Mar 22, 2023

Implement docstrings #1493

Merged

bendk approved these changes Mar 30, 2023

View reviewed changes

uniffi_bindgen/src/interface/object.rs Outdated Show resolved Hide resolved

uniffi_bindgen/src/scaffolding/templates/ObjectTemplate.rs Outdated Show resolved Hide resolved

mhammond force-pushed the traits-types branch 3 times, most recently from b380a52 to 73f2428 Compare April 24, 2023 22:14

mhammond marked this pull request as ready for review April 24, 2023 22:14

mhammond commented Apr 24, 2023

View reviewed changes

mhammond mentioned this pull request Apr 25, 2023

[discuss] next steps for interface definition language? #385

Closed

mhammond force-pushed the traits-types branch 2 times, most recently from 0870081 to fdd8f1d Compare April 30, 2023 23:08

Allow Rust traits to be exposed as an interface.

cf4c1cd

mhammond force-pushed the traits-types branch from fdd8f1d to cf4c1cd Compare April 30, 2023 23:17

mhammond merged commit b982143 into mozilla:main Apr 30, 2023

mhammond deleted the traits-types branch April 30, 2023 23:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Rust traits to be exposed as an interface. #1457

Allow Rust traits to be exposed as an interface. #1457

mhammond commented Jan 23, 2023

bendk left a comment

bendk Jan 25, 2023 •

edited

Loading

mhammond commented Mar 16, 2023

mhammond commented Mar 17, 2023

bendk left a comment

mhammond left a comment

mhammond Apr 24, 2023

Allow Rust traits to be exposed as an interface. #1457

Allow Rust traits to be exposed as an interface. #1457

Conversation

mhammond commented Jan 23, 2023

bendk left a comment

Choose a reason for hiding this comment

bendk Jan 25, 2023 • edited Loading

Choose a reason for hiding this comment

mhammond commented Mar 16, 2023

mhammond commented Mar 17, 2023

bendk left a comment

Choose a reason for hiding this comment

mhammond left a comment

Choose a reason for hiding this comment

mhammond Apr 24, 2023

Choose a reason for hiding this comment

bendk Jan 25, 2023 •

edited

Loading