Make the proc-macro FfiConverter impls canonical #1464

bendk · 2023-01-31T15:59:00Z

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 Allow Rust traits to be exposed as an interface. #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.

bendk · 2023-01-31T15:59:28Z

fixtures/uitests/tests/ui/fieldless_errors_used_in_callbacks_cant_have_fields.stderr

+  | | );
+  | |_^
+  |
+  = note: this error originates in the macro `::uniffi::ffi_converter_flat_error_with_read` (in Nightly builds, run with -Z macro-backtrace for more info)


These error messages aren't very good, but they don't seem any worse that the previous ones.

jplatte

Took a quick peek at the macro code, this is not a comprehensive review.

uniffi_macros/src/lib.rs

bendk · 2023-01-31T16:09:01Z

uniffi_macros/src/error.rs

-        unsafe impl<T> ::uniffi::FfiConverter<T> for #ident {
-            ::uniffi::ffi_converter_rust_buffer_lift_and_lower!(T);
+        unsafe impl ::uniffi::FfiConverter<crate::UniFfiTag> for #ident {
+            ::uniffi::ffi_converter_rust_buffer_lift_and_lower!(crate::UniFfiTag);


This changes the code to only implement FfiConverter<crate::UniFffiTag>, meaning other crates don't get an automatic implementation. I made the change because:

I want to move the interface FfiConverter impls to the generate code, but that can't be a blanket impl since Arc<T> is not a local type

The blanket impl doesn't work for remote types and this is currently a requirement for the UDL-based code

I think we can still make sharing types between crates ergonomic. What if the requirement was that users needed to write uniffi_use!(external_crate::ExternalType)? That seems reasonable to me. I hope to have another PR, based on top of this, that implements this.

I'm still against changing the default. How about having UDL codegen communicate to the proc-macro that it wants this sort of impl? Something like

#[derive(uniffi::Enum)] #[uniffi(tag = crate::UniFfiTag)] enum Foo { ... }

as discussed previously.

Yes, we can do that.

bendk · 2023-01-31T21:10:50Z

uniffi_macros/src/error.rs

+            })
+        } else {
+            Err(lookahead.error())
+        }


This is my best guess at how to handle more attributes here, please tell me if there's a better way to do this.

Exactly what I would have written 👍🏼

bendk · 2023-01-31T21:16:37Z

Pushed an update based on the comments from @jplatte:

Switched the macros to use DeriveInput, which removes the need for parsing code
Added code to control when the macros generate FfiConverter implementations for all <T> vs a specific UniFfiTag.

jplatte · 2023-02-03T08:49:11Z

uniffi_macros/src/error.rs

+            })
+        } else {
+            Err(lookahead.error())
+        }


Exactly what I would have written 👍🏼

uniffi_macros/src/object.rs

jplatte · 2023-02-03T08:53:22Z

uniffi_macros/src/util.rs

+    }
+}
+
+impl TryFrom<AttributeArgs> for FfiConverterTagHandler {


Why use syn's Meta types instead of parsing the attributes using syn::parse::Parse like elsewhere? I already added a bunch of helpers to make that easier.

I guess this is also the reason you didn't do tag = crate::Tag syntax, because syn::Meta doesn't support that?

I tried, but couldn't figure out a good way to get from a TokenStream to an [Attribute] slicte to use with parse_uniffi_attributes(). Maybe we could handle this as a follow-up.

Yeah, I think I can take care of that next week.

mhammond

This is a great improvement, thanks.

mhammond · 2023-02-07T05:47:20Z

uniffi/tests/ui/proc_macro_arc.stderr

+   = note: required for `Arc<Foo>` to implement `FfiConverter<UniFfiTag>`
+   = note: this error originates in the attribute macro `uniffi::export` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error[E0277]: the trait bound `child::Foo: Interface<UniFfiTag>` is not satisfied


This change is a little unfortunate as the compiler gave much better diagnostics previously. However, at least it will lists the incorrect type name, so this seems OK if there's really nothing that can be done to improve it (and if you think there might be, maybe open a new issue?)

I think I'm missing something because both error messages don't seem very clear to me. I'm happy to work on improving these as a followup though.

mhammond · 2023-02-07T05:50:13Z

uniffi_core/src/lib.rs

+///
+/// Like, FfiConverter this has a generic parameter, that's filled in with a type local to the
+/// UniFFI consumer crate.
+pub trait Interface<UT>: Send + Sync + Sized {}


Interface will end up being an unfortunate name in the future if macros end up being the preferred/only way of exposing items. I'm fine with crossing that bridge when we come to it, mainly because I can't quickly think of a better name :)

* 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.

bendk requested a review from a team as a code owner January 31, 2023 15:59

bendk requested review from mhammond, jplatte and a team and removed request for a team January 31, 2023 15:59

bendk commented Jan 31, 2023

View reviewed changes

jplatte reviewed Jan 31, 2023

View reviewed changes

uniffi_macros/src/lib.rs Outdated Show resolved Hide resolved

uniffi_macros/src/lib.rs Outdated Show resolved Hide resolved

uniffi_macros/src/lib.rs Outdated Show resolved Hide resolved

bendk commented Jan 31, 2023

View reviewed changes

bendk force-pushed the ffi-converter-macros branch 2 times, most recently from 4925ba9 to a7d26b1 Compare January 31, 2023 21:06

bendk commented Jan 31, 2023

View reviewed changes

bendk force-pushed the ffi-converter-macros branch from a7d26b1 to c87b2fd Compare January 31, 2023 21:13

bendk force-pushed the ffi-converter-macros branch 4 times, most recently from bfbd1ae to 99a71e9 Compare February 2, 2023 20:40

jplatte reviewed Feb 3, 2023

View reviewed changes

bendk force-pushed the ffi-converter-macros branch from 99a71e9 to 407aed6 Compare February 3, 2023 19:45

bendk mentioned this pull request Feb 3, 2023

Use FfiConverter for metadata generation/storage #1469

Merged

2 tasks

jplatte approved these changes Feb 3, 2023

View reviewed changes

mhammond approved these changes Feb 7, 2023

View reviewed changes

bendk force-pushed the ffi-converter-macros branch from 407aed6 to 1d6c384 Compare February 7, 2023 22:02

bendk merged commit 1d6c384 into mozilla:main Feb 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the proc-macro FfiConverter impls canonical #1464

Make the proc-macro FfiConverter impls canonical #1464

bendk commented Jan 31, 2023

bendk Jan 31, 2023

jplatte left a comment

bendk Jan 31, 2023

jplatte Jan 31, 2023 •

edited

Loading

bendk Jan 31, 2023

bendk Jan 31, 2023

jplatte Feb 3, 2023

bendk commented Jan 31, 2023

jplatte Feb 3, 2023

jplatte Feb 3, 2023

jplatte Feb 3, 2023

bendk Feb 3, 2023

jplatte Feb 3, 2023

mhammond left a comment

mhammond Feb 7, 2023

bendk Feb 7, 2023

mhammond Feb 7, 2023

Make the proc-macro FfiConverter impls canonical #1464

Make the proc-macro FfiConverter impls canonical #1464

Conversation

bendk commented Jan 31, 2023

Choose a reason for hiding this comment

jplatte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jplatte Jan 31, 2023 • 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

bendk commented Jan 31, 2023

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

Choose a reason for hiding this comment

jplatte Jan 31, 2023 •

edited

Loading