Skip to content

Commit

Permalink
Make the proc-macro FfiConverter impls canonical
Browse files Browse the repository at this point in the history
* 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.
  • Loading branch information
bendk committed Jan 31, 2023
1 parent 1fe90bb commit c87b2fd
Show file tree
Hide file tree
Showing 16 changed files with 447 additions and 363 deletions.
Original file line number Diff line number Diff line change
@@ -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)
44 changes: 7 additions & 37 deletions fixtures/uitests/tests/ui/interface_not_sync_and_send.stderr
Original file line number Diff line number Diff line change
@@ -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)
2 changes: 1 addition & 1 deletion uniffi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")]
Expand Down
42 changes: 15 additions & 27 deletions uniffi/tests/ui/proc_macro_arc.stderr
Original file line number Diff line number Diff line change
@@ -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)
42 changes: 9 additions & 33 deletions uniffi_bindgen/src/scaffolding/templates/EnumTemplate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 %}
}
92 changes: 12 additions & 80 deletions uniffi_bindgen/src/scaffolding/templates/ErrorTemplate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 %}
}
2 changes: 2 additions & 0 deletions uniffi_bindgen/src/scaffolding/templates/ObjectTemplate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
24 changes: 5 additions & 19 deletions uniffi_bindgen/src/scaffolding/templates/RecordTemplate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 %}
}
5 changes: 3 additions & 2 deletions uniffi_core/src/ffi_converter_impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;

Expand Down
6 changes: 6 additions & 0 deletions uniffi_core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
Loading

0 comments on commit c87b2fd

Please sign in to comment.