From be0b298c094b2c86323c543071d8fc95cdedade9 Mon Sep 17 00:00:00 2001 From: Ben Dean-Kawamura Date: Fri, 3 May 2024 16:29:02 -0400 Subject: [PATCH] Adding the `LowerError` trait This specifies how types are lowered when they are the `E` side of `Result`. The main reason for the trait is that it lets us specialize the handling for interfaces by auto-wrapping the value in an Arc. This makes using interfaces as errors much more ergonomic. Also, replace the `Error` bound on the `E` types with `Display + Debug`. I just found out that `anyhow::Error` doesn't actually implement `Error` for some obscure reason. --- CHANGELOG.md | 4 ++ docs/manual/src/udl/errors.md | 4 +- fixtures/error-types/src/error_types.udl | 3 ++ fixtures/error-types/src/lib.rs | 11 +++- fixtures/error-types/tests/bindings/test.kts | 9 ++++ fixtures/error-types/tests/bindings/test.py | 7 +++ .../error-types/tests/bindings/test.swift | 8 ++- uniffi_core/src/ffi_converter_impls.rs | 14 +++-- uniffi_core/src/ffi_converter_traits.rs | 51 +++++++++++++++---- uniffi_core/src/lib.rs | 2 +- uniffi_macros/src/error.rs | 2 +- uniffi_macros/src/ffiops.rs | 7 +++ uniffi_macros/src/object.rs | 28 ++++++---- 13 files changed, 119 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4efd98d343..5e44eaa238 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ ## [[UnreleasedUniFFIVersion]] (backend crates: [[UnreleasedBackendVersion]]) - (_[[ReleaseDate]]_) +### What's new? + +- Objects error types can now be as `Result<>` error type without wrapping them in `Arc<>`. + ### What's fixed? - Custom Type names are now treated as type names by all bindings. This means if they will work if they happen to be keywords in the language. There's a very small risk of this being a breaking change if you used a type name which diff --git a/docs/manual/src/udl/errors.md b/docs/manual/src/udl/errors.md index 3b64628272..5ee67cc660 100644 --- a/docs/manual/src/udl/errors.md +++ b/docs/manual/src/udl/errors.md @@ -83,9 +83,9 @@ impl From for MyError { You can't yet use `anyhow` directly in your exposed functions - you need a wrapper: ```rs -fn oops() -> Result<(), Arc> { +fn oops() -> Result<(), MyError> { let e = anyhow::Error::msg("oops"); - Err(Arc::new(e.into())) + Err(e.into()) } ``` then in Python: diff --git a/fixtures/error-types/src/error_types.udl b/fixtures/error-types/src/error_types.udl index 3356a7e0a0..ad01a41f55 100644 --- a/fixtures/error-types/src/error_types.udl +++ b/fixtures/error-types/src/error_types.udl @@ -2,6 +2,9 @@ namespace error_types { [Throws=ErrorInterface] void oops(); + [Throws=ErrorInterface] + void oops_nowrap(); + ErrorInterface get_error(string message); [Throws=RichError] diff --git a/fixtures/error-types/src/lib.rs b/fixtures/error-types/src/lib.rs index 90645e617d..4076cadd11 100644 --- a/fixtures/error-types/src/lib.rs +++ b/fixtures/error-types/src/lib.rs @@ -27,8 +27,9 @@ impl From for ErrorInterface { } } -// must do explicit conversion... +// Test an interface as the error type fn oops() -> Result<(), Arc> { + // must do explicit conversion to convert anyhow::Error into ErrorInterface Err(Arc::new( anyhow::Error::msg("oops") .context("because uniffi told me so") @@ -36,6 +37,14 @@ fn oops() -> Result<(), Arc> { )) } +// Like `oops`, but let UniFFI handle wrapping the interface with an arc +fn oops_nowrap() -> Result<(), ErrorInterface> { + // must do explicit conversion to convert anyhow::Error into ErrorInterface + Err(anyhow::Error::msg("oops") + .context("because uniffi told me so") + .into()) +} + #[uniffi::export] fn toops() -> Result<(), Arc> { Err(Arc::new(ErrorTraitImpl { diff --git a/fixtures/error-types/tests/bindings/test.kts b/fixtures/error-types/tests/bindings/test.kts index b70998f2f5..32f0d1bc01 100644 --- a/fixtures/error-types/tests/bindings/test.kts +++ b/fixtures/error-types/tests/bindings/test.kts @@ -14,6 +14,15 @@ try { assert(e.link(0U) == "because uniffi told me so") } +try { + oopsNowrap() + throw RuntimeException("Should have failed") +} catch (e: ErrorInterface) { + assert(e.toString() == "because uniffi told me so\n\nCaused by:\n oops") + assert(e.chain().size == 2) + assert(e.link(0U) == "because uniffi told me so") +} + try { toops() throw RuntimeException("Should have failed") diff --git a/fixtures/error-types/tests/bindings/test.py b/fixtures/error-types/tests/bindings/test.py index efb95c609a..60ed3afdce 100644 --- a/fixtures/error-types/tests/bindings/test.py +++ b/fixtures/error-types/tests/bindings/test.py @@ -14,6 +14,13 @@ def test_normal_catch(self): except ErrorInterface as e: self.assertEqual(str(e), "because uniffi told me so\n\nCaused by:\n oops") + def test_normal_catch_with_implit_arc_wrapping(self): + try: + oops_nowrap() + self.fail("must fail") + except ErrorInterface as e: + self.assertEqual(str(e), "because uniffi told me so\n\nCaused by:\n oops") + def test_error_interface(self): with self.assertRaises(ErrorInterface) as cm: oops() diff --git a/fixtures/error-types/tests/bindings/test.swift b/fixtures/error-types/tests/bindings/test.swift index 2421df5863..cc409c4ad0 100644 --- a/fixtures/error-types/tests/bindings/test.swift +++ b/fixtures/error-types/tests/bindings/test.swift @@ -5,6 +5,12 @@ do { } catch let e as ErrorInterface { assert(String(describing: e) == "because uniffi told me so\n\nCaused by:\n oops") } +do { + try oopsNowrap() + fatalError("Should have thrown") +} catch let e as ErrorInterface { + assert(String(describing: e) == "because uniffi told me so\n\nCaused by:\n oops") +} do { try toops() @@ -17,4 +23,4 @@ let e = getError(message: "the error") assert(String(describing: e) == "the error") assert(String(reflecting: e) == "ErrorInterface { e: the error }") assert(Error.self is Swift.Error.Type) -assert(Error.self != Swift.Error.self) \ No newline at end of file +assert(Error.self != Swift.Error.self) diff --git a/uniffi_core/src/ffi_converter_impls.rs b/uniffi_core/src/ffi_converter_impls.rs index caed7235ac..34ef5c6074 100644 --- a/uniffi_core/src/ffi_converter_impls.rs +++ b/uniffi_core/src/ffi_converter_impls.rs @@ -23,8 +23,8 @@ /// "UT" means an arbitrary `UniFfiTag` type. use crate::{ check_remaining, derive_ffi_traits, ffi_converter_rust_buffer_lift_and_lower, metadata, - ConvertError, FfiConverter, Lift, LiftRef, LiftReturn, Lower, LowerReturn, MetadataBuffer, - Result, RustBuffer, TypeId, UnexpectedUniFFICallbackError, + ConvertError, FfiConverter, Lift, LiftRef, LiftReturn, Lower, LowerError, LowerReturn, + MetadataBuffer, Result, RustBuffer, TypeId, UnexpectedUniFFICallbackError, }; use anyhow::bail; use bytes::buf::{Buf, BufMut}; @@ -32,7 +32,7 @@ use paste::paste; use std::{ collections::HashMap, convert::TryFrom, - error::Error, + fmt::{Debug, Display}, sync::Arc, time::{Duration, SystemTime}, }; @@ -425,14 +425,17 @@ derive_ffi_traits!(blanket SystemTime); // Note that this means we don't get specialized return handling. For example, if we could return // an `Option>` we would always return that type directly and never throw. derive_ffi_traits!(impl LowerReturn for Option where Option: Lower); +derive_ffi_traits!(impl LowerError for Option where Option: Lower); derive_ffi_traits!(impl LiftReturn for Option where Option: Lift); derive_ffi_traits!(impl LiftRef for Option where Option: Lift); derive_ffi_traits!(impl LowerReturn for Vec where Vec: Lower); +derive_ffi_traits!(impl LowerError for Vec where Vec: Lower); derive_ffi_traits!(impl LiftReturn for Vec where Vec: Lift); derive_ffi_traits!(impl LiftRef for Vec where Vec: Lift); derive_ffi_traits!(impl LowerReturn for HashMap where HashMap: Lower); +derive_ffi_traits!(impl LowerError for HashMap where HashMap: Lower); derive_ffi_traits!(impl LiftReturn for HashMap where HashMap: Lift); derive_ffi_traits!(impl LiftRef for HashMap where HashMap: Lift); @@ -440,6 +443,7 @@ derive_ffi_traits!(impl LiftRef for HashMap where HashMap Lower for Arc where Arc: FfiConverter, T: ?Sized); derive_ffi_traits!(impl Lift for Arc where Arc: FfiConverter, T: ?Sized); derive_ffi_traits!(impl LowerReturn for Arc where Arc: Lower, T: ?Sized); +derive_ffi_traits!(impl LowerError for Arc where Arc: Lower, T: ?Sized); derive_ffi_traits!(impl LiftReturn for Arc where Arc: Lift, T: ?Sized); derive_ffi_traits!(impl LiftRef for Arc where Arc: Lift, T: ?Sized); derive_ffi_traits!(impl TypeId for Arc where Arc: FfiConverter, T: ?Sized); @@ -472,14 +476,14 @@ impl TypeId for () { unsafe impl LowerReturn for Result where R: LowerReturn, - E: Lower + Error + Send + Sync + 'static, + E: LowerError + Display + Debug + Send + Sync + 'static, { type ReturnType = R::ReturnType; fn lower_return(v: Self) -> Result { match v { Ok(r) => R::lower_return(r), - Err(e) => Err(E::lower_into_rust_buffer(e)), + Err(e) => Err(E::lower_error(e)), } } diff --git a/uniffi_core/src/ffi_converter_traits.rs b/uniffi_core/src/ffi_converter_traits.rs index a0cf23d97f..1b4fdb7333 100644 --- a/uniffi_core/src/ffi_converter_traits.rs +++ b/uniffi_core/src/ffi_converter_traits.rs @@ -12,15 +12,15 @@ //! The main traits form a sort-of tree structure from general to specific: //! ```ignore //! -//! [FfiConverter] -//! | -//! ----------------------------- -//! | | -//! [Lower] [Lift] -//! | | -//! | -------------- -//! | | | -//! [LowerReturn] [LiftRef] [LiftReturn] +//! [FfiConverter] +//! | +//! ----------------------------------------- +//! | | +//! [Lower] [Lift] +//! | | +//! ----------------- -------------- +//! | | | | +//! [LowerReturn] [LowerError] [LiftRef] [LiftReturn] //! ``` //! //! There's also: @@ -253,7 +253,7 @@ pub unsafe trait Lower: Sized { /// Return Rust values to the foreign code /// -/// This is usually derived from [Lift], but we special case types like `Result<>` and `()`. +/// This is usually derived from [Lower], but we special case types like `Result<>` and `()`. /// /// ## Safety /// @@ -288,6 +288,26 @@ pub unsafe trait LowerReturn: Sized { } } +/// Return Rust error values +/// +/// This is implemented for types that can be the `E` param in `Result`. +/// It's is usually derived from [Lower], but we sometimes special case it. +/// +/// ## Safety +/// +/// All traits are unsafe (implementing it requires `unsafe impl`) because we can't guarantee +/// that it's safe to pass your type out to foreign-language code and back again. Buggy +/// implementations of this trait might violate some assumptions made by the generated code, +/// or might not match with the corresponding code in the generated foreign-language bindings. +/// These traits should not be used directly, only in generated code, and the generated code should +/// have fixture tests to test that everything works correctly together. +pub unsafe trait LowerError: Sized { + /// Lower this value for scaffolding function return + /// + /// Lower the type into a RustBuffer. `RustCallStatus.error_buf` will be set to this. + fn lower_error(obj: Self) -> RustBuffer; +} + /// Return foreign values to Rust /// /// This is usually derived from [Lower], but we special case types like `Result<>` and `()`. @@ -467,6 +487,7 @@ macro_rules! derive_ffi_traits { $crate::derive_ffi_traits!(impl Lower for $ty); $crate::derive_ffi_traits!(impl Lift for $ty); $crate::derive_ffi_traits!(impl LowerReturn for $ty); + $crate::derive_ffi_traits!(impl LowerError for $ty); $crate::derive_ffi_traits!(impl LiftReturn for $ty); $crate::derive_ffi_traits!(impl LiftRef for $ty); $crate::derive_ffi_traits!(impl ConvertError for $ty); @@ -477,6 +498,7 @@ macro_rules! derive_ffi_traits { $crate::derive_ffi_traits!(impl Lower for $ty); $crate::derive_ffi_traits!(impl Lift for $ty); $crate::derive_ffi_traits!(impl LowerReturn for $ty); + $crate::derive_ffi_traits!(impl LowerError for $ty); $crate::derive_ffi_traits!(impl LiftReturn for $ty); $crate::derive_ffi_traits!(impl LiftRef for $ty); $crate::derive_ffi_traits!(impl ConvertError for $ty); @@ -524,6 +546,15 @@ macro_rules! derive_ffi_traits { } }; + (impl $(<$($generic:ident),*>)? $(::uniffi::)? LowerError<$ut:path> for $ty:ty $(where $($where:tt)*)?) => { + unsafe impl $(<$($generic),*>)* $crate::LowerError<$ut> for $ty $(where $($where)*)* + { + fn lower_error(obj: Self) -> $crate::RustBuffer { + >::lower_into_rust_buffer(obj) + } + } + }; + (impl $(<$($generic:ident),*>)? $(::uniffi::)? LiftReturn<$ut:path> for $ty:ty $(where $($where:tt)*)?) => { unsafe impl $(<$($generic),*>)* $crate::LiftReturn<$ut> for $ty $(where $($where)*)* { diff --git a/uniffi_core/src/lib.rs b/uniffi_core/src/lib.rs index 53fa6641c3..6df0cf2e91 100644 --- a/uniffi_core/src/lib.rs +++ b/uniffi_core/src/lib.rs @@ -46,7 +46,7 @@ pub mod metadata; pub use ffi::*; pub use ffi_converter_traits::{ ConvertError, FfiConverter, FfiConverterArc, HandleAlloc, Lift, LiftRef, LiftReturn, Lower, - LowerReturn, TypeId, + LowerError, LowerReturn, TypeId, }; pub use metadata::*; diff --git a/uniffi_macros/src/error.rs b/uniffi_macros/src/error.rs index 0ef023423a..04100521b7 100644 --- a/uniffi_macros/src/error.rs +++ b/uniffi_macros/src/error.rs @@ -60,7 +60,7 @@ fn flat_error_ffi_converter_impl(item: &EnumItem, options: &DeriveOptions) -> To let lower_impl_spec = options.ffi_impl_header("Lower", ident); let lift_impl_spec = options.ffi_impl_header("Lift", ident); let type_id_impl_spec = options.ffi_impl_header("TypeId", ident); - let derive_ffi_traits = options.derive_ffi_traits(ident, &["ConvertError"]); + let derive_ffi_traits = options.derive_ffi_traits(ident, &["LowerError", "ConvertError"]); let mod_path = match mod_path() { Ok(p) => p, Err(e) => return e.into_compile_error(), diff --git a/uniffi_macros/src/ffiops.rs b/uniffi_macros/src/ffiops.rs index 416318e9e5..f43befd812 100644 --- a/uniffi_macros/src/ffiops.rs +++ b/uniffi_macros/src/ffiops.rs @@ -67,6 +67,13 @@ pub fn lower_return(ty: impl ToTokens) -> TokenStream { } } +/// Lower error function +pub fn lower_error(ty: impl ToTokens) -> TokenStream { + quote! { + <#ty as ::uniffi::LowerError>::lower_error + } +} + /// Lift return type pub fn lift_return_type(ty: impl ToTokens) -> TokenStream { quote! { diff --git a/uniffi_macros/src/object.rs b/uniffi_macros/src/object.rs index 95476ee1da..280a673d4d 100644 --- a/uniffi_macros/src/object.rs +++ b/uniffi_macros/src/object.rs @@ -99,6 +99,7 @@ fn interface_impl(object: &ObjectItem, options: &DeriveOptions) -> TokenStream { let ident = object.ident(); let impl_spec = options.ffi_impl_header("FfiConverterArc", ident); let lower_return_impl_spec = options.ffi_impl_header("LowerReturn", ident); + let lower_error_impl_spec = options.ffi_impl_header("LowerError", ident); let type_id_impl_spec = options.ffi_impl_header("TypeId", ident); let lift_ref_impl_spec = options.ffi_impl_header("LiftRef", ident); let mod_path = match mod_path() { @@ -106,11 +107,12 @@ fn interface_impl(object: &ObjectItem, options: &DeriveOptions) -> TokenStream { Err(e) => return e.into_compile_error(), }; let arc_self_type = quote! { ::std::sync::Arc }; - let lower = ffiops::lower(&arc_self_type); - let type_id_meta = ffiops::type_id_meta(&arc_self_type); - let try_lift = ffiops::try_lift(&arc_self_type); - let lower_return_type = ffiops::lower_return_type(&arc_self_type); - let lower_return = ffiops::lower_return(&arc_self_type); + let lower_arc = ffiops::lower(&arc_self_type); + let type_id_meta_arc = ffiops::type_id_meta(&arc_self_type); + let try_lift_arc = ffiops::try_lift(&arc_self_type); + let lower_return_type_arc = ffiops::lower_return_type(&arc_self_type); + let lower_return_arc = ffiops::lower_return(&arc_self_type); + let lower_error_arc = ffiops::lower_error(&arc_self_type); quote! { // All Object structs must be `Sync + Send`. The generated scaffolding will fail to compile @@ -159,7 +161,7 @@ fn interface_impl(object: &ObjectItem, options: &DeriveOptions) -> TokenStream { /// function for other types may lead to undefined behaviour. fn write(obj: ::std::sync::Arc, buf: &mut Vec) { ::uniffi::deps::static_assertions::const_assert!(::std::mem::size_of::<*const ::std::ffi::c_void>() <= 8); - ::uniffi::deps::bytes::BufMut::put_u64(buf, #lower(obj) as u64); + ::uniffi::deps::bytes::BufMut::put_u64(buf, #lower_arc(obj) as u64); } /// When reading as a field of a complex structure, we receive a "borrow" of the `Arc` @@ -170,7 +172,7 @@ fn interface_impl(object: &ObjectItem, options: &DeriveOptions) -> TokenStream { fn try_read(buf: &mut &[u8]) -> ::uniffi::Result<::std::sync::Arc> { ::uniffi::deps::static_assertions::const_assert!(::std::mem::size_of::<*const ::std::ffi::c_void>() <= 8); ::uniffi::check_remaining(buf, 8)?; - #try_lift(::uniffi::deps::bytes::Buf::get_u64(buf) as Self::FfiType) + #try_lift_arc(::uniffi::deps::bytes::Buf::get_u64(buf) as Self::FfiType) } const TYPE_ID_META: ::uniffi::MetadataBuffer = ::uniffi::MetadataBuffer::from_code(::uniffi::metadata::codes::TYPE_INTERFACE) @@ -179,10 +181,16 @@ fn interface_impl(object: &ObjectItem, options: &DeriveOptions) -> TokenStream { } unsafe #lower_return_impl_spec { - type ReturnType = #lower_return_type; + type ReturnType = #lower_return_type_arc; fn lower_return(obj: Self) -> ::std::result::Result { - #lower_return(::std::sync::Arc::new(obj)) + #lower_return_arc(::std::sync::Arc::new(obj)) + } + } + + unsafe #lower_error_impl_spec { + fn lower_error(obj: Self) -> ::uniffi::RustBuffer { + #lower_error_arc(::std::sync::Arc::new(obj)) } } @@ -191,7 +199,7 @@ fn interface_impl(object: &ObjectItem, options: &DeriveOptions) -> TokenStream { } #type_id_impl_spec { - const TYPE_ID_META: ::uniffi::MetadataBuffer = #type_id_meta; + const TYPE_ID_META: ::uniffi::MetadataBuffer = #type_id_meta_arc; } } }