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.
* 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 #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.
  • Loading branch information
bendk committed Jan 31, 2023
1 parent 1fe90bb commit 4925ba9
Show file tree
Hide file tree
Showing 14 changed files with 289 additions and 298 deletions.
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
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_flat_error_with_read!(
| | r#ArithmeticError
| | {
| | r#IntegerOverflow {
... |
| | }
| | );
| |_^
|
= 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)
30 changes: 0 additions & 30 deletions fixtures/uitests/tests/ui/interface_not_sync_and_send.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -16,33 +16,3 @@ note: required by a bound in `assert_impl_all`
| 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
|
= 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>`
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
58 changes: 31 additions & 27 deletions uniffi/tests/ui/proc_macro_arc.stderr
Original file line number Diff line number Diff line change
@@ -1,31 +1,35 @@
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 `Arc<Foo>: FfiConverter<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 `FfiConverter<UniFfiTag>` is not implemented for `Arc<Foo>`
|
note: expected this to be `Arc<Foo>`
--> tests/ui/proc_macro_arc.rs:21:22
= help: the following other types implement trait `FfiConverter<UT>`:
()
Duration
HashMap<K, V>
Option<T>
String
SystemTime
Vec<T>
bool
and $N others
= 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 `Arc<child::Foo>: FfiConverter<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 `FfiConverter<UniFfiTag>` is not implemented for `Arc<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 following other types implement trait `FfiConverter<UT>`:
()
Duration
HashMap<K, V>
Option<T>
String
SystemTime
Vec<T>
bool
and $N others
= note: this error originates in the attribute macro `uniffi::export` (in Nightly builds, run with -Z macro-backtrace for more info)
41 changes: 9 additions & 32 deletions uniffi_bindgen/src/scaffolding/templates/EnumTemplate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,37 +7,14 @@
// 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 %},
::uniffi::ffi_converter_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 %}
v => uniffi::deps::anyhow::bail!("Invalid {{ e.name() }} enum value: {}", v),
})
},
{%- endfor %}
}
}
);
95 changes: 16 additions & 79 deletions uniffi_bindgen/src/scaffolding/templates/ErrorTemplate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,84 +7,21 @@
// 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 %},
{%- if e.is_flat() && ci.should_generate_error_read(e) %}
::uniffi::ffi_converter_flat_error_with_read!(
{%- else if e.is_flat() && !ci.should_generate_error_read(e) %}
::uniffi::ffi_converter_flat_error!(
{%- else %}
::uniffi::ffi_converter_enum!(
{%- endif %}
r#{{ e.name() }}
{
{%- for variant in e.variants() %}
r#{{ variant.name() }} {
{%- for field in variant.fields() %}
r#{{ field.name() }}: {{ field.type_()|type_rs }},
{%- endfor %}
v => uniffi::deps::anyhow::bail!("Invalid {{ e.name() }} enum value: {}", v),
})
},
{%- endfor %}
}
{% endif %}
}
);
1 change: 1 addition & 0 deletions uniffi_bindgen/src/scaffolding/templates/ObjectTemplate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
fn uniffi_note_threadsafe_deprecation_{{ obj.name() }}() {}
{% endif %}

::uniffi::ffi_converter_interface!(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
21 changes: 4 additions & 17 deletions uniffi_bindgen/src/scaffolding/templates/RecordTemplate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,23 +8,10 @@
// 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.
::uniffi::ffi_converter_record!(
r#{{ rec.name() }} {
{%- for field in rec.fields() %}
{{ field.type_().borrow()|ffi_converter }}::write(obj.r#{{ field.name() }}, buf);
r#{{ field.name() }}: {{ field.type_()|type_rs }},
{%- 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 %}
})
}
}
);
60 changes: 0 additions & 60 deletions uniffi_core/src/ffi_converter_impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -343,63 +343,3 @@ where
Ok(map)
}
}

/// Support for passing reference-counted shared objects via the FFI.
///
/// 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> {
// Don't use a pointer to <T> as that requires a `pub <T>`
type FfiType = *const std::os::raw::c_void;

/// When lowering, we have an owned `Arc<T>` and we transfer that ownership
/// to the foreign-language code, "leaking" it out of Rust's ownership system
/// as a raw pointer. This works safely because we have unique ownership of `self`.
/// The foreign-language code is responsible for freeing this by calling the
/// `ffi_object_free` FFI function provided by the corresponding UniFFI type.
///
/// Safety: when freeing the resulting pointer, the foreign-language code must
/// call the destructor function specific to the type `T`. Calling the destructor
/// function for other types may lead to undefined behaviour.
fn lower(obj: std::sync::Arc<T>) -> Self::FfiType {
std::sync::Arc::into_raw(obj) as Self::FfiType
}

/// When lifting, we receive a "borrow" of the `Arc<T>` that is owned by
/// the foreign-language code, and make a clone of it for our own use.
///
/// Safety: the provided value must be a pointer previously obtained by calling
/// the `lower()` or `write()` method of this impl.
fn try_lift(v: Self::FfiType) -> Result<std::sync::Arc<T>> {
let v = v as *const T;
// We musn't drop the `Arc<T>` that is owned by the foreign-language code.
let foreign_arc = std::mem::ManuallyDrop::new(unsafe { std::sync::Arc::<T>::from_raw(v) });
// Take a clone for our own use.
Ok(std::sync::Arc::clone(&*foreign_arc))
}

/// When writing as a field of a complex structure, make a clone and transfer ownership
/// of it to the foreign-language code by writing its pointer into the buffer.
/// The foreign-language code is responsible for freeing this by calling the
/// `ffi_object_free` FFI function provided by the corresponding UniFFI type.
///
/// Safety: when freeing the resulting pointer, the foreign-language code must
/// call the destructor function specific to the type `T`. Calling the destructor
/// function for other types may lead to undefined behaviour.
fn write(obj: std::sync::Arc<T>, buf: &mut Vec<u8>) {
static_assertions::const_assert!(std::mem::size_of::<*const std::ffi::c_void>() <= 8);
buf.put_u64(<Self as FfiConverter<UT>>::lower(obj) as u64);
}

/// When reading as a field of a complex structure, we receive a "borrow" of the `Arc<T>`
/// that is owned by the foreign-language code, and make a clone for our own use.
///
/// Safety: the buffer must contain a pointer previously obtained by calling
/// the `lower()` or `write()` method of this impl.
fn try_read(buf: &mut &[u8]) -> Result<std::sync::Arc<T>> {
static_assertions::const_assert!(std::mem::size_of::<*const std::ffi::c_void>() <= 8);
check_remaining(buf, 8)?;
<Self as FfiConverter<UT>>::try_lift(buf.get_u64() as Self::FfiType)
}
}
6 changes: 3 additions & 3 deletions uniffi_macros/src/enum_.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,8 @@ pub(crate) fn enum_ffi_converter_impl(

quote! {
#[automatically_derived]
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);

fn write(obj: Self, buf: &mut ::std::vec::Vec<u8>) {
#write_impl
Expand Down Expand Up @@ -155,6 +155,6 @@ fn write_field(f: &Field) -> TokenStream {
let ty = &f.ty;

quote! {
<Self as ::uniffi::FfiConverter<#ty>>::write(#ident, buf);
<#ty as ::uniffi::FfiConverter<crate::UniFfiTag>>::write(#ident, buf);
}
}
Loading

0 comments on commit 4925ba9

Please sign in to comment.