Ffi converter #1008

bendk · 2021-08-06T13:46:02Z

Okay, I think this code is actually ready to merge. How are you all feeling about it?

Couple notes here:

I wrote up an ADR which reflects my thinking on the main design choice here. Then I read over jhugman's PR, I realized that we currently use the other option for the Kotlin bindings (I think all the bindings except swift) and it's working okay. Let's consider the ADR a work in progress. I don't think that needs to block the merge though.
I think there's actually a better way of implementing the pattern here, where we define a struct that can convert between a Rust type and a FFI type. It basically does the same thing, but without the weirdness of the associated types or needing to generate so many unit structs. I also don't think that needs to block the merge, this code can be a stepping block to there.

No functional changes yet -- this change is mostly just replacing `Self` with `Self::RustType`. The only actual code change was to shift the responsibility of lowering rust errors from the `rustcalls` module to the code that calls into it. This opens the door for handling external types because we can define our own struct that implements ViaFfi for the external type.

Before, the system was that if you wanted to lift/lower type T, then you would use <T as ViaFfi>. This change implements a more flexible mapping: - The `Type` enum defines `viaffi_impl_name()`, which determines which ViaFfi implementation to use for that type. Sometimes it's the type, sometimes it's something else. - With the help of some template code, we can map any `Type` to Rust code to do the lowering/lifting (e.g. `Type` -> `<ViaFfiImplForType as ViaFfi>::lift(val)`) This gives us some flexibility and moves us towards composable ViaFfi types. One nice feature is all of the smarts are in the ViaFfi impl, which can use whatever Rust techniques it wants to to implement composition. It's also just nice to have almost all of the code in one place, rather than spread out and mixed up between template code, rust code, the component interface, etc. Some things are already nicer: - For Records/Enums/Errors, we define a new unit-struct to implement ViaFfi for the type. This allows us to support external types without hitting the orphan rule. - We can implement Option<Type> with `Option<{{ Type.viaffi_impl_name()}}>` and a generic implementation and this should work with any adapter-style ViaFfi implementations we write in the future. - We can move the code to wrap CallbackInteface in a `Box<>` out of the template code and into the CallbackInterface template. Before it was split up between that template code and the scaffolding filters. Also, Option<CallbackInteface> was not working, this adds support for that (maybe Vec<CallbackInteface> too, although I haven't tested that one).

This was Ryan's suggestion and I think it's a good one. The two traits accomplish the same task, but they're fundamentally different so we should use a different name. - Didn't change the name on swift, since its code still follows the ViaFfi model. - Removed a line from the upcoming changelog. It's kind of weird to have it reference a name that's been change. I also realize that it's not possible for consumer code to use a custom ViaFfi/FfiConverter yet, so that wasn't really a visible change to them.

bendk · 2021-08-06T13:48:52Z

docs/adr/0006-external-types.md

+* Status: proposed
+* Deciders: bendk, rfkelly, mhammond
+* Consulted: jhugman, jan-erik, travis
+* Date: 2021-08-06


I'm never sure what to put for "deciders" and "consulted". I put Ryan and Mark on for deciders since I remember them saying I should make a real PR for the code. I put James, Jan-Erik, and Travis as consulted since I did some demoing of the code to them yesterday. Tell me if I should change this and sorry if I forgot anyone's contribution, there's been a lot going on this week!

It's easier to read and to type. The main reason that I used "viaffi_impl" was to hint that we had a new system and that it wasn't always going to be `<RustType as ViaFfi>`. Renaming the trait to `FfiConverter` accomplishes the same thing better.

bendk · 2021-08-06T15:46:18Z

fixtures/external-types/lib/src/lib.rs

+pub struct CombinedType {
+    pub cot: CrateOneType,
+    pub ctt: CrateTwoType,
+}


mhammond mentioned that these fields don't have the greatest names and that he has a similar fixture overlaps with this one. My proposal is that we merge this code to main, then one of us can create a PR that merges/improves the fixture.

skhamis

I'll let someone else closer to this do the final approval but after rereading it a couple of times I definitely think this is a great approach to what is essentially sidestepping the orphan rule (but it seems, without creating a whole nest of other problems).

My only issue is (if we're still using udl in the near future) that I personally think there might need to be some way denote that this type is coming from an external crate but otherwise looks awesome! Really nice job with this!

skhamis · 2021-08-06T20:42:18Z

fixtures/external-types/lib/src/external-types-lib.udl

+    CombinedType get_combined_type(optional CombinedType? cval);
+};
+
+dictionary CrateOneType {


Hmm if we stick with the UDL approach I wonder if we'll need something to denote that this is indeed external from the current crate (even though it's awesome we can do this now) even if functionally it does nothing. Something like [ExternalType] or even [Crate=<ExternalCrateName>] feels like it'd help the consumer/viewer quickly understand (imagine if we're using types from 5+ crates the mental juggling would become unwieldy).

However this just might be me -- open to what other people think

This is interesting because Rust doesn't actually need to be told where it is, because it's just a "plain old type" - so it's actually the including Rust code that must have the use crate_one::CrateOneType - I was actually surprised this didn't work on main today, but forgot that uniffi in the lib crate will try to generate impl uniffi::RustBufferViaFfi for CrateOneType { - which fails for the reasons Ben mentions in the adr.

In practice, I doubt that this will end up being a common use-case (ie, that the .udl for a type will be in a different crate than where the type is defined) - I think we'll end up with crate-one having its own .udl, and lib will want to refer to that via the typedef approach we are experimenting with elsewhere. But this is still a good step on the path to that.

I'm 50/50 on if it should be tagged in the UDL. On one hand, it's nice to make it clear where the thing is coming from. On the other hand, it's kind of redundant. I went this way because it was just easier to implement, but either is fine with me.

skhamis · 2021-08-06T20:47:32Z

uniffi/src/lib.rs

    type FfiType = RustBuffer;

    // This returns a struct with a raw pointer to the underlying bytes, so it's very
    // important that it consume ownership of the String, which is relinquished to the
    // foreign language code (and can be restored by it passing the pointer back).
-    fn lower(self) -> Self::FfiType {
-        RustBuffer::from_vec(self.into_bytes())
+    fn lower(obj: Self::RustType) -> Self::FfiType {


Self::RustTypeinitially took my head for a spin but after seeing the implementation and understanding all the context behind it I have to say it's a really interesting approach!

mhammond

I love this! Let's wait for Ryan, but take my upvote ;)

mhammond · 2021-08-08T08:19:10Z

fixtures/external-types/lib/src/external-types-lib.udl

+    CombinedType get_combined_type(optional CombinedType? cval);
+};
+
+dictionary CrateOneType {


This is interesting because Rust doesn't actually need to be told where it is, because it's just a "plain old type" - so it's actually the including Rust code that must have the use crate_one::CrateOneType - I was actually surprised this didn't work on main today, but forgot that uniffi in the lib crate will try to generate impl uniffi::RustBufferViaFfi for CrateOneType { - which fails for the reasons Ben mentions in the adr.

In practice, I doubt that this will end up being a common use-case (ie, that the .udl for a type will be in a different crate than where the type is defined) - I think we'll end up with crate-one having its own .udl, and lib will want to refer to that via the typedef approach we are experimenting with elsewhere. But this is still a good step on the path to that.

mhammond · 2021-08-08T08:42:49Z

uniffi_bindgen/src/scaffolding/templates/EnumTemplate.rs

 #}
+
+struct {{ e.type_()|ffi_converter_name }};


it's wild to me how this all hangs together without a self!

rfk

This is a very nice refactor, rock on! 👍🏻

rfk · 2021-08-09T02:37:15Z

docs/adr/0006-external-types.md

+
+## Context and Problem Statement
+
+UniFFI currently cannot support types from external crates because of Rust's


nit: "because of" -> "because".

rfk · 2021-08-09T02:38:42Z

docs/adr/0006-external-types.md

+
+  * In the filters functions we generate code to wrap lift/lower/read/write.
+    For example the lower_rs filter could output `External_type(x).lower()` to
+    lower `WrapperType`.


I don't understand use of External_type(x) here...should this be WrapperType(x).lower()?

rfk · 2021-08-09T02:41:28Z

docs/adr/0006-external-types.md

+    new unit struct and set `RustType` to the type.  This handles external types
+    without issues since we're implementing `FfiConverter` on our on struct.
+    The orphan rule doesn't apply to associated types.
+


Another option here (similar to something that @mhammond suggested previously) would be to define a new trait that's specific to our crate. We could have the trait delegate to ViaFfi for anything that implements it while also providing out own implementation for the types defined in this crate. The "unit struct" pattern is probably easier to understand, but I wanted to note this for completeness.

That's an interesting idea. I actually have 1 more idea that comes from your suggestion of trying to leverage generics that I think could be really neat.

This ADR needs some work. I think I should change the focus away from the specifics of FfiConverter and focus on the decision to do the wrapping in the target language rather than in the template code.

rfk · 2021-08-09T02:42:09Z

docs/adr/0006-external-types.md

+
+* We believe it will make it easier to implement the other wrapping-style
+  features mentioned above.  One sign of this was the CallbackInterface code,
+  which converts it's type to `Box<dyn CallbackInterfaceTrait>`.  The


nit: "it's" -> "its"

rfk · 2021-08-09T03:28:14Z

fixtures/callbacks/src/lib.rs

@@ -33,6 +33,15 @@ impl RustGetters {
    fn get_list(&self, callback: Box<dyn ForeignGetters>, v: Vec<i32>, arg2: bool) -> Vec<i32> {
        callback.get_list(v, arg2)
    }
+
+    fn get_string_optional_callback<'a>(


FWIW, this seemed to work correctly for me without the explicit 'a lifetime annotation, is there a reason it is required?

rfk · 2021-08-09T03:34:15Z

uniffi_bindgen/src/scaffolding/mod.rs

+    /// Get the name of the FfiConverter implementation for this type
+    ///
+    /// - For primitives / standard types this is the type itself.
+    /// - For user-defined types, this is a unique generated name.  We then generate a unit-struct


nit: This "We then generate a unit-struct" feels like the first half of some larger thought, is it something that needs to be fleshed out more, or a leftover to be removed?

rfk · 2021-08-09T03:36:31Z

uniffi_bindgen/src/scaffolding/templates/CallbackInterfaceTemplate.rs

-    // impedence mismatches between Rust and foreign languages, and our uncertainty around implementations
-    // of concurrent handlemaps.
+    // Lower and write are tricky to implement because we have a dyn trait as our type.  There's
+    // probably a way to, but but this carries lots of thread safety risks, down to impedence


nit: "but but" -> "but"

rfk · 2021-08-09T03:39:11Z

uniffi_bindgen/src/scaffolding/templates/EnumTemplate.rs

@@ -2,31 +2,38 @@
 // For each enum declared in the UDL, we assume the caller has provided a corresponding
 // rust `enum`. We provide the traits for sending it across the FFI, which will fail to
 // compile if the provided struct has a different shape to the one declared in the UDL.
+//
+// We define a unit-struct to implement the trait to sidestep Rust's orphan rule (ADR-0006).


Please consider adding a little more to this comment to help future intrepid debuggers who may be working through the generated Rust code. Suggestion:

"""
We define a unit-struct to implement the trait rather than implementing it directly on the enum.
This sidesteps Rust's orphan rules in the case where the underlying type has been imported from
a separate crate (ADR-0006).
"""

rfk · 2021-08-09T03:44:37Z

uniffi_bindgen/src/scaffolding/mod.rs

+            // inner type.
+            Type::Optional(inner) => format!("Option<{}>", ffi_converter_name(inner)?),
+            Type::Sequence(inner) => format!("Vec<{}>", ffi_converter_name(inner)?),
+            Type::Map(inner) => format!("HashMap<String, {}>", ffi_converter_name(inner)?),


Aha! Coming back to this work, this is the bit that made things click for me. We're using the generated unit structs to construct an expression in the typesystem that knows how to convert nested structures containing imported types, without having to actually materialize any values of that type. So we might end up with Vec<Option<FfiConverterForImportedType>> which is not a value at runtime, but which knows how to convert a corresponding Vec<Option<ImportedType>> value back-and-forth across the FFI. Neat.

rfk · 2021-08-09T03:47:41Z

CHANGELOG.md

- Replaced `lower_into_buffer()` and `try_lift_from_buffer()` with the
-  `RustBufferViaFfi` trait.  If you use those functions in your custom ViaFfi
-  implementation then you'll need to update the code.  Check out the `Option<>`
-  implementation in uniffi/src/lib.rs for an example.


Please consider adding a changelog entry about the new trait here, I think it'll be to our longer-term benefit if we keep ourselves accountable for communicating these changes outward.

badboy

Couple of nits.

I like this approach.

uniffi/src/ffi/rustcalls.rs

uniffi/src/lib.rs

Thanks badboy! I've never used the github suggestion UI before, that was great. Co-authored-by: Jan-Erik Rediger <badboy@archlinux.us>

bhearsum added 5 commits August 5, 2021 12:59

Let's actually test that external types work (and they do!)

820d456

Added an ADR describing what's going on in this branch.

1d5a63d

bendk requested a review from a team August 6, 2021 13:46

bendk commented Aug 6, 2021

View reviewed changes

bhearsum added 2 commits August 6, 2021 10:01

Cargo fmt

83c62ed

bendk commented Aug 6, 2021

View reviewed changes

skhamis reviewed Aug 6, 2021

View reviewed changes

mhammond approved these changes Aug 8, 2021

View reviewed changes

rfk approved these changes Aug 9, 2021

View reviewed changes

rfk reviewed Aug 9, 2021

View reviewed changes

badboy approved these changes Aug 9, 2021

View reviewed changes

bendk and others added 3 commits August 9, 2021 09:31

Apply suggestions from code review

35fe355

Thanks badboy! I've never used the github suggestion UI before, that was great. Co-authored-by: Jan-Erik Rediger <badboy@archlinux.us>

Changes based on rfk`s review

ea6d91c

Merge branch 'main' into ffi-converter

716ebe9

bendk merged commit 82b8b19 into mozilla:main Aug 9, 2021

rfk mentioned this pull request Aug 10, 2021

Use fully-qualified type names in scaffolding #1015

Closed

skhamis mentioned this pull request Aug 20, 2021

Update uniffi 0.14.0 mozilla/application-services#4389

Merged

4 tasks

bendk deleted the ffi-converter branch September 10, 2021 17:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ffi converter #1008

Ffi converter #1008

bendk commented Aug 6, 2021 •

edited

Loading

bendk Aug 6, 2021

bendk Aug 6, 2021 •

edited

Loading

skhamis left a comment

skhamis Aug 6, 2021

mhammond Aug 8, 2021

bendk Aug 9, 2021

skhamis Aug 6, 2021

mhammond left a comment

mhammond Aug 8, 2021

mhammond Aug 8, 2021

rfk left a comment

rfk Aug 9, 2021

rfk Aug 9, 2021

rfk Aug 9, 2021

bendk Aug 9, 2021 •

edited

Loading

rfk Aug 9, 2021

rfk Aug 9, 2021

rfk Aug 9, 2021

rfk Aug 9, 2021

rfk Aug 9, 2021

rfk Aug 9, 2021

rfk Aug 9, 2021

badboy left a comment


		## Context and Problem Statement

		UniFFI currently cannot support types from external crates because of Rust's

Ffi converter #1008

Ffi converter #1008

Conversation

bendk commented Aug 6, 2021 • edited Loading

Choose a reason for hiding this comment

bendk Aug 6, 2021 • edited Loading

Choose a reason for hiding this comment

skhamis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhammond left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendk Aug 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

badboy left a comment

Choose a reason for hiding this comment

bendk commented Aug 6, 2021 •

edited

Loading

bendk Aug 6, 2021 •

edited

Loading

bendk Aug 9, 2021 •

edited

Loading