New system for remote / custom / external types #2087

bendk · 2024-05-01T18:36:02Z

As discussed in #1865, this is how I think we should update our code for remote / custom / external types.

The doc changes are the first commit so that we can agree on how things should work from the user's perspective.

Changes:

Make remote types a first-class feature.
Make UDL generate blanket ffi trait impls for all UniFfiTags. Remove the use_udl_* since we don't need them anymore.
Add use_remote_type! to handle the one case where we do need to forward ffi traits impls to another crate's tag.
Use a macro to define custom types. This way we have more freedom to change how things are implemented behind the scenes.
Update the UDL external syntax so that [Extern] can work with any type.

Benefits are:

UDL and proc-macros will be consistent in their handling of UniFfiTag.
First-class remote types would enable things like using anyhow::Error in interfaces.
The custom type macro is easier to use then the current code. It's also easier for us to make changes behind the scenes.
External types get a little less hacky.
3rd party crates can provide built-in UniFFI support and implement the ffi traits for all their consumers.

jplatte · 2024-05-01T19:20:02Z

In my mind the biggest benefit is actually that using types from a crate with builtin UniFFI support doesn't require 'importing' the trait impls at all with this, regardless of UDL vs. proc-macros. Maybe add that to the list?

bendk · 2024-05-02T12:11:16Z

In my mind the biggest benefit is actually that using types from a crate with builtin UniFFI support doesn't require 'importing' the trait impls at all with this, regardless of UDL vs. proc-macros. Maybe add that to the list?

Added and I agree that it's a benefit in many cases, but I think crates should think carefully before they do that since they lock users in to their choices. I think this is especially true if they use a custom type conversion. For example, should UUIDs be a string or a struct of 4 U32s? Should URLs be a string or an interface?

Each derive is now based on 2 things: - The item, which is the parsed DeriveInput - The context, which specifies how we want to render the item. Do we want to generate metadata, which UniFfiTags should we implement traits on, etc. Storing all the parsed DeriveInput parts in 1 type makes it easier to change what/how we parse. We only need to update the struct not the signatures of all the functions. Adding the context will help make the UDL / proc-macro code more consistent (mozilla#1865). We can update the `udl_derive` method and have it affect all UDL-based generation. I also want to add a couple more modes for remote types and remote UDL types. Consolidated the `derive_*_for_udl` macros into a single `udl_derive` macro. I think this is simpler and I also want to use the same basic structure for the `remote_type` macro described in mozilla#2087. Made EnumItem also parse error attributes. I think this is simpler than creating a separate EnumItem vs ErrorItem, at least for now. Removed the `#[uniffi(non_exhaustive)]` attribute and parse the real `#[non_exhaustive]` attribute instead. This is easy to do now with the new system and it will be simpler for the user when we implement remote types.

This is prep work for implementing the system for remote / custom / external types as described in mozilla#2087. Consolidated the `derive_*_for_udl` macros into a single `udl_derive` macro and added the `DeriveOptions` struct. My plan is to re-use this pattern remote types (with `generate_metadata: true, local_tag: true`). This also simplifies the task of switching UDL to generate blanket FFI trait impls. Changed the specific derive code to use `DeriveOptions` plus an item struct. For example RecordItem, which represents the parsed DeriveInput for the record. This will make future changes easier since we only need to modify one of these structs, not update all the function signatures. Changed the UDL derives to parse attributes exactly like regular derives. For example, we now wrap enums with `#[uniffi(flat_error)]` rather than putting `flat_error` in the `derive_error_for_udl` attribute macro. This is prep work for remote types -- in that case I think we should tell the user to define the item exactly like it was a regular derive and not special case the attributes. Removed the `#[uniffi(non_exhaustive)]` attribute and parse the real `#[non_exhaustive]` attribute instead. This is easy to do now with the new system and also will be good for remote types. Made EnumItem also parse error attributes. I think this is simpler than creating a separate EnumItem vs ErrorItem, at least for now.

This is prep work for implementing the system for remote / custom / external types as described in mozilla#2087. The generated code should stay exactly the same, this just refactors how it's generated. Consolidated the `derive_*_for_udl` macros into a single `udl_derive` macro and added the `DeriveOptions` struct. My plan is to re-use this pattern remote types (with `generate_metadata: true, local_tag: true`). This also simplifies the task of switching UDL to generate blanket FFI trait impls. Changed the specific derive code to use `DeriveOptions` plus an item struct. For example RecordItem, which represents the parsed DeriveInput for the record. This will make future changes easier since we only need to modify one of these structs, not update all the function signatures. Changed the UDL derives to parse attributes exactly like regular derives. For example, we now wrap enums with `#[uniffi(flat_error)]` rather than putting `flat_error` in the `derive_error_for_udl` attribute macro. This is prep work for remote types -- in that case I think we should tell the user to define the item exactly like it was a regular derive and not special case the attributes. Removed the `#[uniffi(non_exhaustive)]` attribute and parse the real `#[non_exhaustive]` attribute instead. This is easy to do now with the new system and also will be good for remote types. Made EnumItem also parse error attributes. I think this is simpler than creating a separate EnumItem vs ErrorItem, at least for now.

This is prep work for implementing the system for remote / custom / external types as described in mozilla#2087. The generated code should stay exactly the same, this just refactors how it's generated. Consolidated the `derive_*_for_udl` macros into a single `udl_derive` macro and added the `DeriveOptions` struct. I think that we can update UDL-generation to create blanket FFI trait impls by just tweaking which flags we set. We can also re-use the same pattern to implement remote types (which will have `generate_metadata: true, local_tag: true`). Changed the specific derive code to use `DeriveOptions` plus an item struct. For example RecordItem, which represents the parsed DeriveInput for the record. This will make future changes easier since we only need to modify one of these structs, not update all the function signatures. Changed the UDL derives to parse attributes exactly like regular derives. For example, we now wrap enums with `#[uniffi(flat_error)]` rather than putting `flat_error` in the `derive_error_for_udl` attribute macro. This is prep work for remote types -- in that case I think we should tell the user to define the item exactly like it was a regular derive and not special case the attributes. Removed the `#[uniffi(non_exhaustive)]` attribute and parse the real `#[non_exhaustive]` attribute instead. This is easy to do now with the new system and also will be good for remote types. Made EnumItem also parse error attributes. I think this is simpler than creating a separate EnumItem vs ErrorItem, at least for now.

This is prep work for implementing the system for remote / custom / external types as described in mozilla#2087. The generated code should stay exactly the same, this just refactors how it's generated. Consolidated the `derive_*_for_udl` macros into a single `udl_derive` macro and added the `DeriveOptions` struct. I think that we can update UDL-generation to create blanket FFI trait impls by just tweaking which flags we set. We can also re-use the same pattern to implement remote types (which will have `generate_metadata: true, local_tag: true`). Changed the specific derive code to use `DeriveOptions` plus an item struct. For example RecordItem, which represents the parsed DeriveInput for the record. This will make future changes easier since we only need to modify one of these structs, not update all the function signatures. Changed the UDL derives to parse attributes exactly like regular derives. For example, we now wrap enums with `#[uniffi(flat_error)]` rather than calling `derive_error_for_udl!(flat_error)`. This is prep work for remote types -- in that case I think we should tell the user to define the item exactly like it was a regular derive and not special case the attributes. Removed the `#[uniffi(non_exhaustive)]` attribute and parse the real `#[non_exhaustive]` attribute instead. This is easy to do now with the new system and also will be good for remote types. Made EnumItem also parse error attributes. I think this is simpler than creating a separate EnumItem vs ErrorItem, at least for now.

mhammond · 2024-05-03T00:39:19Z

That description is just a huge list of positives with not many draw-backs, so what's not to love :) These types have all been one hack on another...

I'm twice confused though :)

doesn't require 'importing' the trait impls at all with this

That sounds great too, but I don't know what kind of imports you mean here - what will it allow users to avoid doing?

Added and I agree that it's a benefit in many cases, but I think crates should think carefully before they do

And I don't get what you mean by this - how/why does what a crate choose?

bendk · 2024-05-03T15:32:22Z

I'm twice confused though :)

doesn't require 'importing' the trait impls at all with this

That sounds great too, but I don't know what kind of imports you mean here - what will it allow users to avoid doing?

If we were in a future where a 3rd-party crate defined FFI traits for their types, then those traits impls would apply to all downstream crates automatically. For some of the other solutions described in #1865, the downstream crates would have call uniffi::use_remove_type! to get the impls.

Added and I agree that it's a benefit in many cases, but I think crates should think carefully before they do

And I don't get what you mean by this - how/why does what a crate choose?

Great question, I think it's something we have to figure out as we go forward. But to take a couple examples:

Suppose fairybridge (a UniFFI-enabled async HTTP bridge). If that ended up being wildly successful and other crates wanted to use it as a standalone library, then fairybridge should define blanket FFI trait impls for its types (otherwise users would need to call uniffi::use_remote_type! for each type they wanted to use).
I don't think it would make sense for uuid to do the same for the Uuid type, since different consumers might want to pass uuids across the FFI in different ways. Maybe it would make sense behind a feature flag, but I don't think there should always be a blanket impl.

mhammond · 2024-05-03T18:29:15Z

then fairybridge should define blanket FFI trait impls for its types

Thanks, but I'm still mildly confused - in the description, you mention "Make UDL generate blanket ffi trait impls for all UniFfiTags" and IIUC, that's what procmacros do today, right?

So I assumed from that description that we'd always and unconditionally generate blanket impls. It's still not clear to me from the above how crates would opt-out of this. proc-macro users can't opt out of this now, right?

Add use_remote_type! to handle the one case where we do need to forward ffi traits impls to another crate's tag.

Maybe this is part of my confusion - what exactly is the one case?

bendk · 2024-05-03T19:08:01Z

then fairybridge should define blanket FFI trait impls for its types

Thanks, but I'm still mildly confused - in the description, you mention "Make UDL generate blanket ffi trait impls for all UniFfiTags" and IIUC, that's what procmacros do today, right?

So I assumed from that description that we'd always and unconditionally generate blanket impls. It's still not clear to me from the above how crates would opt-out of this. proc-macro users can't opt out of this now, right?

Yes, that's 100% correct. The only way to "opt-out" is to not use uniffi::derive on your crates.

I'm just saying that if you're the author of a crate like url and your users are asking you to add UniFFI support, you should probably think twice before just wrapping everything with uniffi::derive, since that forces all downstream crates to use your impls.

Add use_remote_type! to handle the one case where we do need to forward ffi traits impls to another crate's tag.

Maybe this is part of my confusion - what exactly is the one case?

This is the current case with url . If one crate implements the FFI traits for Url, either through custom_type! or remote_type!, then other crates will need to call use_remote_type!.

bendk · 2024-05-04T14:43:23Z

Just pushed the implementation of this. I split it up into different commits for readability, but I'll probably squash them together when we merge.

All tests are passing and I'm really liking the new syntax and the concepts behind it, but I'd love to know what others think. As a follow-up I'd like to to enable methods on remote interfaces, but even without them it allows for using anyhow::Error which is pretty cool. If we merge this plus #2093 then we can use anyhow::Error directly as the error type, no wrapping by the user needed.

Also TODO is implementing the new [External] syntax for UDL. I think this should be an easy followup.

bendk · 2024-05-04T15:11:01Z

uniffi_meta/src/reader.rs

@@ -339,6 +341,7 @@ impl<'a> MetadataReader<'a> {
        Ok(ObjectMetadata {
            module_path: self.read_string()?,
            name: self.read_string()?,
+            remote: false, // only used when generating scaffolding from UDL


These comments are a bit sad. At some point, I think we should separate the metadata that is used to generate the scaffolding from the metadata used to generate the bindings.

…an be used in other crates than declaring crate.

Sajjon · 2024-05-21T08:40:38Z

@bendk I've been testing your source branch quite heavily and it works really well, bravo, well done! I'm using (at time of writing, the last commit - 4b65c37 of your source branch)

I perfectly understand this is in flux :) so I'm happy and impressed that it works so well given that it has not been merged yet!

I found a bug in Kotlin which you might be interested in. It is in my small "Lemon Merringue Pie" repo (a "playground" for understanding how I can migrate Sargon into Workspace multi crate setup), and this PR (source branch) contains the bug - bug if that use case is intended to be supported, but I don't see why not:
Sajjon/lemon-merringue-pie#2

(Swift works like a charm though)

Sajjon · 2024-05-23T06:17:22Z

We probably want my PR into Bens fork: bendk#3 - without it it is not possible to have a single error type defined in crate X but used in crate Y (and Y2, Y3, ...) - which I think is a quite natural setup.

Let me know if I should create a standalone PR into this target repo instead, "standalone" as in "probably can be based on main branch".

bendk · 2024-05-23T13:45:57Z

We probably want my PR into Bens fork: bendk#3 - without it it is not possible to have a single error type defined in crate X but used in crate Y (and Y2, Y3, ...) - which I think is a quite natural setup.

Let me know if I should create a standalone PR into this target repo instead, "standalone" as in "probably can be based on main branch".

Thanks for that one, just merged it in.

As discussed in mozilla#1865, this is how I think we should update our code for remote / custom / external types: - Make remote types a first-class feature. - Make UDL generate blanket ffi trait impls for all UniFfiTags. Remove the `use_udl_*` since we don't need them anymore. - Add `use_remote_type!` to handle the one case where we do need to forward ffi traits impls to another crate's tag. - Use a macro to define custom types. - Update the UDL external syntax so that `[Extern]` can work with any type. Benefits are: - UDL and proc-macros will be consistent in their handling of UniFfiTag. - First-class remote types would enable things like using anyhow::Error in interfaces. - The custom type macro is easier for users to use then the current code. It allows us to hide the complexity of things like the `UniFffiTag`. - External types get a little less hacky. - 3rd party crates can provide built-in UniFFI support and implement the FFI traits for all their consumers.

This can be used from proc-macros to specify that an item is remote and therefore the FFI traits should only be implemnted for the local `UniFfiTag`. Added an example where this is used to use anyhow::Error in as your error type. Also added the `[Remote]` tag for UDL. It doesn't do anything yes because all UDL-based generation currently uses the local tag. I'm going to change that in the next commit though. Require that error types to implement `Display + Debug` rather than `Error`. For some reason `anyhow::Error` doesn't actually implement error.

Implemented the custom type system descibed in the new docs. This is working for single crates, but multi-crate setup is currently broken. It should start working again once we change how UDL generation handles UniFfiTag. Removed the nested-module-import fixture. The custom type code will no longer test it, since it's not using `UniffiCustomTypeConverter`. I couldn't think of a way to make it work again.

This means it works exactly like the proc-macro code which simplifies things significantly. Also, this fixes the ext-types fixture to work with types that are custom, remote, and external. Removed the `use_udl_*` macros, they're not needed anymore. Added the `remote_type!` macro, it's now needed for using remote types who's FFI trait impls are defined in another crate (which is now possible from using UDL and proc-macros) One feature that we've lost in defining callback/trait interfaces using traits defined in a remote crate. I doubt anyone is using that feature, but I think the plan should be to re-implement it before the next release. As part of the same work, I think we can also support methods on remote interfaces.

Sajjon · 2024-05-24T06:53:04Z

(Using Bens fork, commit: bendk@f4fae82 )

Context: Asked for help on Matrix about compilation errors in Swift bindings when multi-crates are used:
For Multi-crate Cargo Workspace setup : Rust crates - each using UniFFI - that are dependent on each other each will generate multiple Swift files - trying to use lift/lower on types from each create (if such dependency exists in the Rust code, i.e. CrateX using (UniFFI exported) types from CrateY).

Given CrateX being dependent on UniFFI code in CrateY, this will generate broken Swift code:
CrateX.swift will contain swift code calling lift / lower etc o types in CrateY.swift - but those functions are private, so this does not compile. I'm unsure about status of Kotlin (and other language bindings), I have not progress that far yet....

I think I have an OK idea on how to solve this - maybe there is some detail I miss which makes my naive idea not break, but here goes nothing :
(this assumes on single call to generate (Swift) bindings - aka library mode)

All generated Converters will be placed in separate files, in lets call it generated/ folder (as specified by --out-dir), and only be generated once, i.e. the contents of ptemplate/StringHelper.swift](https://github.com/bendk/uniffi-rs/blob/custom-type-rework/uniffi_bindgen/src/bindings/swift/templates/StringHelper.swift) will be put generated/ folder
The actual type - fileprivate struct FfiConverterString: FfiConverter - gets a changed visibility to internal struct instead of fileprivate.
All protocols, types, functions and methods in RustBufferTemplate (and similar tempates) get changed visibility to internal too
Bindgen code for a single crate only contain the author written UniFFI code in one file per crate, places in generated/ "next to" the UniFFI generated above mentioned converters.

The result is many - smaller - Swift files with the converters, all with internal visibility so that these Swift files can see each all symbols within this Swift target, and CrateX.swift and CrateY.swift will also see each others lift and lower methods.

I think that will work. And I think most UniFFI consumers build scripts will "just work" (at least if they used cp / mv with *.swift ) or easily fixed.

I started doing this myself but I will take me days, but probably done by @bendk in a couple of hours tops!

WDYT? I think this will make this PR work with Multi-crate solution!

Here is commit showcasing the broken bindgen in my small Workspace demo project LemonMerringuePie. If you run cargo test the Swift bindings test will fail.

Screenshot of problem:

(Using Bens fork, commit: bendk@f4fae82 )

EDIT: Or alternatively make the library mode generate Swift code to go into a single file, not multiple Swift files, which would also fix the Multi-crate issues... but i think using separate files is better. It makes CMD clicking on Symbols from Xcode on types/functions/methods faster, because the generated file(s) will not be as big! Actually, I've been meaning to suggest that we change the bindgen to generate separate files for each type! This will offer much greater DX (Developer Experience), since CMD clicking types does not take you to a 30,000 lines of code scary file with strange "RustBuffers" but rather a single small file with just the Swift generated type - we can even put the FFIConverters for those CustomTypes (e.g. #[uniffi::Record]\nstruct MyRecord ) in a separate file! Great DX!

bendk requested a review from a team as a code owner May 1, 2024 18:36

bendk requested review from badboy, mhammond and jplatte and removed request for a team May 1, 2024 18:36

bendk added the discuss label May 1, 2024

bendk changed the title ~~Updating the docs for remote / custom / external types~~ Discuss: new system for remote / custom / external types May 1, 2024

bendk force-pushed the custom-type-rework branch 2 times, most recently from da041c8 to 76b8c0a Compare May 2, 2024 12:07

bendk mentioned this pull request May 2, 2024

Refactor the derive macro code #2090

Merged

bendk force-pushed the custom-type-rework branch 2 times, most recently from 5681d55 to c2c0fca Compare May 4, 2024 14:38

bendk removed the discuss label May 4, 2024

bendk force-pushed the custom-type-rework branch from c2c0fca to 8169b6e Compare May 4, 2024 14:40

bendk force-pushed the custom-type-rework branch from 8169b6e to 268453c Compare May 4, 2024 14:45

bendk commented May 4, 2024

View reviewed changes

bendk force-pushed the custom-type-rework branch from 268453c to 4b65c37 Compare May 6, 2024 19:59

bendk changed the title ~~Discuss: new system for remote / custom / external types~~ New system for remote / custom / external types May 20, 2024

Sajjon added a commit to Sajjon/lemon-merringue-pie that referenced this pull request May 20, 2024

use Bens fork with mozilla/uniffi-rs#2087 and the BagOfBytes works! c…

8d3eb60

…an be used in other crates than declaring crate.

Sajjon added a commit to Sajjon/lemon-merringue-pie that referenced this pull request May 20, 2024

use Bens fork with mozilla/uniffi-rs#2087 and the BagOfBytes works! c…

2582a00

…an be used in other crates than declaring crate.

CyonAlexRDX mentioned this pull request May 22, 2024

WIP: Convert to workspace (multi crate) radixdlt/sargon#143

Draft

bendk and others added 5 commits May 23, 2024 09:47

Allow 'External' errors from crate X to be used in crate Y

f4fae82

bendk force-pushed the custom-type-rework branch from bd33c25 to f4fae82 Compare May 23, 2024 13:47

This was referenced May 24, 2024

Remote interface types #2126

Open

Remote types: interfaces and proc macros ADR #2130

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New system for remote / custom / external types #2087

New system for remote / custom / external types #2087

bendk commented May 1, 2024 •

edited

jplatte commented May 1, 2024 •

edited

bendk commented May 2, 2024

mhammond commented May 3, 2024

bendk commented May 3, 2024

mhammond commented May 3, 2024

bendk commented May 3, 2024

bendk commented May 4, 2024 •

edited

bendk May 4, 2024

Sajjon commented May 21, 2024 •

edited

Sajjon commented May 23, 2024

bendk commented May 23, 2024

Sajjon commented May 24, 2024 •

edited

New system for remote / custom / external types #2087

Are you sure you want to change the base?

New system for remote / custom / external types #2087

Conversation

bendk commented May 1, 2024 • edited

jplatte commented May 1, 2024 • edited

bendk commented May 2, 2024

mhammond commented May 3, 2024

bendk commented May 3, 2024

mhammond commented May 3, 2024

bendk commented May 3, 2024

bendk commented May 4, 2024 • edited

bendk May 4, 2024

Choose a reason for hiding this comment

Sajjon commented May 21, 2024 • edited

Sajjon commented May 23, 2024

bendk commented May 23, 2024

Sajjon commented May 24, 2024 • edited

bendk commented May 1, 2024 •

edited

jplatte commented May 1, 2024 •

edited

bendk commented May 4, 2024 •

edited

Sajjon commented May 21, 2024 •

edited

Sajjon commented May 24, 2024 •

edited