Make the proc-macro FfiConverter impls canonical

* 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 #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.
mozilla · Feb 2, 2023 · 99a71e9 · 99a71e9
1 parent b50a66f
commit 99a71e9
Show file tree

Hide file tree

Showing 17 changed files with 465 additions and 365 deletions.
diff --git a/fixtures/uitests/tests/ui/fieldless_errors_used_in_callbacks_cant_have_fields.stderr b/fixtures/uitests/tests/ui/fieldless_errors_used_in_callbacks_cant_have_fields.stderr
@@ -1,11 +1,7 @@
-error[E0063]: missing field `numerator` in initializer of `ArithmeticError`
-  --> $OUT_DIR[uniffi_uitests]/errors.uniffi.rs
-   |
-   |             2 => r#ArithmeticError::r#DivisionByZero{ },
-   |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `numerator`
-
-error[E0063]: missing field `0` in initializer of `ArithmeticError`
-  --> $OUT_DIR[uniffi_uitests]/errors.uniffi.rs
-   |
-   |             3 => r#ArithmeticError::r#UnexpectedError{ },
-   |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `0`
+error[E0533]: expected unit struct, unit variant or constant, found struct variant `Self::DivisionByZero`
+ --> $OUT_DIR[uniffi_uitests]/errors.uniffi.rs
+  |
+  | #[::uniffi::ffi_converter_error(crate::UniFfiTag)]
+  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+  |
+  = note: this error originates in the attribute macro `::uniffi::ffi_converter_error` (in Nightly builds, run with -Z macro-backtrace for more info)
diff --git a/fixtures/uitests/tests/ui/interface_not_sync_and_send.stderr b/fixtures/uitests/tests/ui/interface_not_sync_and_send.stderr
@@ -1,48 +1,18 @@
 error[E0277]: `Cell<u32>` cannot be shared between threads safely
  --> $OUT_DIR[uniffi_uitests]/counter.uniffi.rs
   |
-  | uniffi::deps::static_assertions::assert_impl_all!(r#Counter: Sync, Send);
-  |                                                   ^^^^^^^^^ `Cell<u32>` cannot be shared between threads safely
+  | #[::uniffi::ffi_converter_interface(crate::UniFfiTag)]
+  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `Cell<u32>` cannot be shared between threads safely
   |
   = help: within `Counter`, the trait `Sync` is not implemented for `Cell<u32>`
 note: required because it appears within the type `Counter`
  --> tests/ui/interface_not_sync_and_send.rs:9:12
   |
 9 | pub struct Counter {
   |            ^^^^^^^
-note: required by a bound in `assert_impl_all`
- --> $OUT_DIR[uniffi_uitests]/counter.uniffi.rs
-  |
-  | uniffi::deps::static_assertions::assert_impl_all!(r#Counter: Sync, Send);
-  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `assert_impl_all`
-  = note: this error originates in the macro `uniffi::deps::static_assertions::assert_impl_all` (in Nightly builds, run with -Z macro-backtrace for more info)
-
-error[E0277]: `Cell<u32>` cannot be shared between threads safely
- --> $OUT_DIR[uniffi_uitests]/counter.uniffi.rs
-  |
-  |         <std::sync::Arc<r#Counter> as uniffi::FfiConverter<crate::UniFfiTag>>::lower(_arc)
-  |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `Cell<u32>` cannot be shared between threads safely
+note: required by a bound in `Interface`
+ --> $WORKSPACE/uniffi_core/src/lib.rs
   |
-  = help: within `Counter`, the trait `Sync` is not implemented for `Cell<u32>`
-  = help: the trait `FfiConverter<UT>` is implemented for `Arc<T>`
-note: required because it appears within the type `Counter`
- --> tests/ui/interface_not_sync_and_send.rs:9:12
-  |
-9 | pub struct Counter {
-  |            ^^^^^^^
-  = note: required for `Arc<Counter>` to implement `FfiConverter<UniFfiTag>`
-
-error[E0277]: `Cell<u32>` cannot be shared between threads safely
- --> $OUT_DIR[uniffi_uitests]/counter.uniffi.rs
-  |
-  |         match<std::sync::Arc<r#Counter> as uniffi::FfiConverter<crate::UniFfiTag>>::try_lift(r#ptr) {
-  |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `Cell<u32>` cannot be shared between threads safely
-  |
-  = help: within `Counter`, the trait `Sync` is not implemented for `Cell<u32>`
-  = help: the trait `FfiConverter<UT>` is implemented for `Arc<T>`
-note: required because it appears within the type `Counter`
- --> tests/ui/interface_not_sync_and_send.rs:9:12
-  |
-9 | pub struct Counter {
-  |            ^^^^^^^
-  = note: required for `Arc<Counter>` to implement `FfiConverter<UniFfiTag>`
+  | pub trait Interface<UT>: Send + Sync + Sized {}
+  |                                 ^^^^ required by this bound in `Interface`
+  = note: this error originates in the attribute macro `::uniffi::ffi_converter_interface` (in Nightly builds, run with -Z macro-backtrace for more info)
diff --git a/uniffi/src/lib.rs b/uniffi/src/lib.rs
@@ -4,7 +4,7 @@
 
 /// Reexport items from other uniffi creates
 pub use uniffi_core::*;
-pub use uniffi_macros::{export, include_scaffolding, Enum, Error, Object, Record};
+pub use uniffi_macros::*;
 #[cfg(feature = "cli")]
 mod cli;
 #[cfg(feature = "bindgen-tests")]

diff --git a/uniffi/tests/ui/proc_macro_arc.stderr b/uniffi/tests/ui/proc_macro_arc.stderr
@@ -1,31 +1,19 @@
-error[E0271]: type mismatch resolving `<Arc<child::Foo> as child::_::_::{closure#0}::TypeEq>::This == Arc<Foo>`
-  --> tests/ui/proc_macro_arc.rs:21:22
+error[E0277]: the trait bound `Foo: Interface<UniFfiTag>` is not satisfied
+  --> tests/ui/proc_macro_arc.rs:10:1
    |
-21 |     fn take_foo(foo: Arc<Foo>) {
-   |                      ^^^^^^^^ type mismatch resolving `<Arc<child::Foo> as child::_::_::{closure#0}::TypeEq>::This == Arc<Foo>`
+10 | #[uniffi::export]
+   | ^^^^^^^^^^^^^^^^^ the trait `Interface<UniFfiTag>` is not implemented for `Foo`
    |
-note: expected this to be `Arc<Foo>`
-  --> tests/ui/proc_macro_arc.rs:21:22
+   = help: the trait `FfiConverter<UT>` is implemented for `Arc<T>`
+   = 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
+  --> tests/ui/proc_macro_arc.rs:20:5
    |
-21 |     fn take_foo(foo: Arc<Foo>) {
-   |                      ^^^
-   = note: enum `child::Foo` and struct `Foo` have similar names, but are actually distinct types
-note: enum `child::Foo` is defined in module `crate::child` of the current crate
-  --> tests/ui/proc_macro_arc.rs:18:5
+20 |     #[uniffi::export]
+   |     ^^^^^^^^^^^^^^^^^ the trait `Interface<UniFfiTag>` is not implemented for `child::Foo`
    |
-18 |     enum Foo {}
-   |     ^^^^^^^^
-note: struct `Foo` is defined in module `crate` of the current crate
-  --> tests/ui/proc_macro_arc.rs:8:1
-   |
-8  | pub struct Foo;
-   | ^^^^^^^^^^^^^^
-note: required by a bound in `child::_::_::{closure#0}::assert_type_eq_all`
-  --> tests/ui/proc_macro_arc.rs:21:22
-   |
-21 |     fn take_foo(foo: Arc<Foo>) {
-   |                      ^^^
-   |                      |
-   |                      required by a bound in this
-   |                      required by this bound in `child::_::_::{closure#0}::assert_type_eq_all`
-   = note: this error originates in the macro `::uniffi::deps::static_assertions::assert_type_eq_all` (in Nightly builds, run with -Z macro-backtrace for more info)
+   = help: the trait `FfiConverter<UT>` is implemented for `Arc<T>`
+   = note: required for `Arc<child::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)
diff --git a/uniffi_bindgen/src/scaffolding/templates/EnumTemplate.rs b/uniffi_bindgen/src/scaffolding/templates/EnumTemplate.rs
@@ -7,37 +7,13 @@
 // public so other crates can refer to it via an `[External='crate'] typedef`
 #}
 
-
-#[doc(hidden)]
-unsafe impl ::uniffi::FfiConverter<crate::UniFfiTag> for r#{{ e.name() }} {
-    ::uniffi::ffi_converter_rust_buffer_lift_and_lower!(crate::UniFfiTag);
-
-    fn write(obj: r#{{ e.name() }}, buf: &mut std::vec::Vec<u8>) {
-        use uniffi::deps::bytes::BufMut;
-        match obj {
-            {%- for variant in e.variants() %}
-            r#{{ e.name() }}::r#{{ variant.name() }} { {% for field in variant.fields() %}r#{{ field.name() }}, {%- endfor %} } => {
-                buf.put_i32({{ loop.index }});
-                {% for field in variant.fields() -%}
-                {{ field.type_().borrow()|ffi_converter }}::write(r#{{ field.name() }}, buf);
-                {%- endfor %}
-            },
-            {%- endfor %}
-        };
-    }
-
-    fn try_read(buf: &mut &[u8]) -> uniffi::deps::anyhow::Result<r#{{ e.name() }}> {
-        use uniffi::deps::bytes::Buf;
-        uniffi::check_remaining(buf, 4)?;
-        Ok(match buf.get_i32() {
-            {%- for variant in e.variants() %}
-            {{ loop.index }} => r#{{ e.name() }}::r#{{ variant.name() }}{% if variant.has_fields() %} {
-                {% for field in variant.fields() %}
-                r#{{ field.name() }}: {{ field.type_().borrow()|ffi_converter }}::try_read(buf)?,
-                {%- endfor %}
-            }{% endif %},
-            {%- endfor %}
-            v => uniffi::deps::anyhow::bail!("Invalid {{ e.name() }} enum value: {}", v),
-        })
-    }
+#[::uniffi::ffi_converter_enum(crate::UniFfiTag)]
+enum r#{{ e.name() }} {
+    {%- for variant in e.variants() %}
+    r#{{ variant.name() }} {
+        {%- for field in variant.fields() %}
+        r#{{ field.name() }}: {{ field.type_()|type_rs }},
+        {%- endfor %}
+    },
+    {%- endfor %}
 }
diff --git a/uniffi_bindgen/src/scaffolding/templates/ErrorTemplate.rs b/uniffi_bindgen/src/scaffolding/templates/ErrorTemplate.rs
@@ -7,84 +7,16 @@
 // public so other crates can refer to it via an `[External='crate'] typedef`
 #}
 
-#[doc(hidden)]
-unsafe impl ::uniffi::FfiConverter<crate::UniFfiTag> for r#{{ e.name() }} {
-    uniffi::ffi_converter_rust_buffer_lift_and_lower!(crate::UniFfiTag);
-
-    {% if e.is_flat() %}
-
-    // For "flat" error enums, we stringify the error on the Rust side and surface that
-    // as the error message in the foreign language.
-
-
-    fn write(obj: r#{{ e.name() }}, buf: &mut std::vec::Vec<u8>) {
-        use uniffi::deps::bytes::BufMut;
-        let msg = obj.to_string();
-        match obj {
-            {%- for variant in e.variants() %}
-            r#{{ e.name() }}::r#{{ variant.name() }}{..} => {
-                buf.put_i32({{ loop.index }});
-                {{ Type::String.borrow()|ffi_converter }}::write(msg, buf);
-            },
-            {%- endfor %}
-        };
-    }
-
-    {%- if ci.should_generate_error_read(e) %}
-    fn try_read(buf: &mut &[u8]) -> uniffi::deps::anyhow::Result<r#{{ e.name() }}> {
-        use uniffi::deps::bytes::Buf;
-        uniffi::check_remaining(buf, 4)?;
-        Ok(match buf.get_i32() {
-            {%- for variant in e.variants() %}
-            {{ loop.index }} => r#{{ e.name() }}::r#{{ variant.name() }}{ },
-            {%- endfor %}
-            v => uniffi::deps::anyhow::bail!("Invalid {{ e.name() }} enum value: {}", v),
-        })
-    }
-    {%- else %}
-    fn try_read(_buf: &mut &[u8]) -> uniffi::deps::anyhow::Result<r#{{ e.name() }}> {
-        panic!("try_read not supported for flat errors");
-    }
-    {%- endif %}
-
-    {% else %}
-
-    // For rich structured enums, we map individual fields on the Rust side over to
-    // corresponding fields on the foreign-language side.
-    //
-    // If a variant doesn't have fields defined in the UDL, it's currently still possible that
-    // the Rust enum has fields and they're just not listed. In that case we use the `Variant{..}`
-    // syntax to match the variant while ignoring its fields.
-
-    fn write(obj: r#{{ e.name() }}, buf: &mut std::vec::Vec<u8>) {
-        use uniffi::deps::bytes::BufMut;
-        match obj {
-            {%- for variant in e.variants() %}
-            r#{{ e.name() }}::r#{{ variant.name() }}{% if variant.has_fields() %} { {% for field in variant.fields() %}r#{{ field.name() }}, {%- endfor %} }{% else %}{..}{% endif %} => {
-                buf.put_i32({{ loop.index }});
-                {% for field in variant.fields() -%}
-                {{ field.type_().borrow()|ffi_converter }}::write(r#{{ field.name() }}, buf);
-                {%- endfor %}
-            },
-            {%- endfor %}
-        };
-    }
-
-    fn try_read(buf: &mut &[u8]) -> uniffi::deps::anyhow::Result<r#{{ e.name() }}> {
-        // Note: no need to call should_generate_error_read here, since it is always true for
-        // non-flat errors
-        use uniffi::deps::bytes::Buf;
-        uniffi::check_remaining(buf, 4)?;
-        Ok(match buf.get_i32() {
-            {%- for variant in e.variants() %}
-            {{ loop.index }} => r#{{ e.name() }}::r#{{ variant.name() }}{% if variant.has_fields() %} {
-                {% for field in variant.fields() %}
-                r#{{ field.name() }}: {{ field.type_().borrow()|ffi_converter }}::try_read(buf)?,
-                {%- endfor %}
-            }{% endif %},
-            {%- endfor %}
-            v => uniffi::deps::anyhow::bail!("Invalid {{ e.name() }} enum value: {}", v),
-        })
-    }
-    {% endif %}
+#[::uniffi::ffi_converter_error(crate::UniFfiTag)]
+{%- if e.is_flat() %}
+#[uniffi(flat_error{% if ci.should_generate_error_read(e) %},with_try_read{% endif %})]
+{%- endif %}
+enum r#{{ e.name() }} {
+    {%- for variant in e.variants() %}
+    r#{{ variant.name() }} {
+        {%- for field in variant.fields() %}
+        r#{{ field.name() }}: {{ field.type_()|type_rs }},
+        {%- endfor %}
+    },
+    {%- endfor %}
 }
diff --git a/uniffi_bindgen/src/scaffolding/templates/ExternalTypesTemplate.rs b/uniffi_bindgen/src/scaffolding/templates/ExternalTypesTemplate.rs
@@ -2,12 +2,12 @@
 
 // Types with an external `FfiConverter`...
 {% for (name, crate_name, kind) in ci.iter_external_types() %}
-{# For non-interface types, we need to import the FfiConverter for them.  Interface types use the generic `Arc<T>` impl. #}
+// The FfiConverter for `{{ name }}` is defined in `{{ crate_name }}`
 {%- match kind %}
 {%- when ExternalKind::DataClass %}
-// `{{ name }}` is defined in `{{ crate_name }}`
 ::uniffi::ffi_converter_forward!(r#{{ name }}, ::{{ crate_name|crate_name_rs }}::UniFfiTag, crate::UniFfiTag);
-{%- else %}
+{%- when ExternalKind::Interface %}
+::uniffi::ffi_converter_forward!(::std::sync::Arc<r#{{ name }}>, ::{{ crate_name|crate_name_rs }}::UniFfiTag, crate::UniFfiTag);
 {%- endmatch %}
 {%- endfor %}
 

diff --git a/uniffi_bindgen/src/scaffolding/templates/ObjectTemplate.rs b/uniffi_bindgen/src/scaffolding/templates/ObjectTemplate.rs
@@ -24,6 +24,8 @@
 fn uniffi_note_threadsafe_deprecation_{{ obj.name() }}() {}
 {% endif %}
 
+#[::uniffi::ffi_converter_interface(crate::UniFfiTag)]
+struct r#{{ obj.name() }} { }
 
 // All Object structs must be `Sync + Send`. The generated scaffolding will fail to compile
 // if they are not, but unfortunately it fails with an unactionably obscure error message.

diff --git a/uniffi_bindgen/src/scaffolding/templates/RecordTemplate.rs b/uniffi_bindgen/src/scaffolding/templates/RecordTemplate.rs
@@ -8,23 +8,9 @@
 // public so other crates can refer to it via an `[External='crate'] typedef`
 #}
 
-#[doc(hidden)]
-unsafe impl ::uniffi::FfiConverter<crate::UniFfiTag> for r#{{ rec.name() }} {
-    ::uniffi::ffi_converter_rust_buffer_lift_and_lower!(crate::UniFfiTag);
-
-    fn write(obj: r#{{ rec.name() }}, buf: &mut std::vec::Vec<u8>) {
-        // If the provided struct doesn't match the fields declared in the UDL, then
-        // the generated code here will fail to compile with somewhat helpful error.
-        {%- for field in rec.fields() %}
-        {{ field.type_().borrow()|ffi_converter }}::write(obj.r#{{ field.name() }}, buf);
-        {%- endfor %}
-    }
-
-    fn try_read(buf: &mut &[u8]) -> uniffi::deps::anyhow::Result<r#{{ rec.name() }}> {
-        Ok(r#{{ rec.name() }} {
-            {%- for field in rec.fields() %}
-                r#{{ field.name() }}: {{ field.type_().borrow()|ffi_converter }}::try_read(buf)?,
-            {%- endfor %}
-        })
-    }
+#[::uniffi::ffi_converter_record(crate::UniFfiTag)]
+struct r#{{ rec.name() }} {
+    {%- for field in rec.fields() %}
+    r#{{ field.name() }}: {{ field.type_()|type_rs }},
+    {%- endfor %}
 }
diff --git a/uniffi_core/src/ffi_converter_impls.rs b/uniffi_core/src/ffi_converter_impls.rs
@@ -3,7 +3,8 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 use crate::{
-    check_remaining, ffi_converter_rust_buffer_lift_and_lower, FfiConverter, Result, RustBuffer,
+    check_remaining, ffi_converter_rust_buffer_lift_and_lower, FfiConverter, Interface, Result,
+    RustBuffer,
 };
 /// This module contains builtin `FFIConverter` implementations.  These cover:
 ///   - Simple privitive types: u8, i32, String, Arc<T>, etc
@@ -349,7 +350,7 @@ where
 /// To avoid dealing with complex lifetime semantics over the FFI, any data passed
 /// by reference must be encapsulated in an `Arc`, and must be safe to share
 /// across threads.
-unsafe impl<UT, T: Sync + Send> FfiConverter<UT> for std::sync::Arc<T> {
+unsafe impl<UT, T: Interface<UT>> FfiConverter<UT> for std::sync::Arc<T> {
     // Don't use a pointer to <T> as that requires a `pub <T>`
     type FfiType = *const std::os::raw::c_void;
 

diff --git a/uniffi_core/src/lib.rs b/uniffi_core/src/lib.rs
@@ -198,6 +198,12 @@ pub unsafe trait FfiConverter<UT>: Sized {
     fn try_read(buf: &mut &[u8]) -> Result<Self>;
 }
 
+/// Implemented for exported interface types
+///
+/// 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 {}
+
 /// Struct to use when we want to lift/lower/serialize types inside the `uniffi` crate.
 struct UniFfiTag;