Merge pull request #2093 from bendk/lower-error

Adding the `LowerError` trait

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

docs/manual/src/udl/errors.md

@@ -83,9 +83,9 @@ impl From<anyhow::Error> for MyError {
 You can't yet use `anyhow` directly in your exposed functions - you need a wrapper:
 
 ```rs
-fn oops() -> Result<(), Arc<MyError>> {
+fn oops() -> Result<(), MyError> {
     let e = anyhow::Error::msg("oops");
-    Err(Arc::new(e.into()))
+    Err(e.into())
 }
 ```
 then in Python:

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]

fixtures/error-types/src/lib.rs

@@ -27,15 +27,24 @@ impl From<anyhow::Error> for ErrorInterface {
     }
 }
 
-// must do explicit conversion...
+// Test an interface as the error type
 fn oops() -> Result<(), Arc<ErrorInterface>> {
+    // must do explicit conversion to convert anyhow::Error into ErrorInterface
     Err(Arc::new(
         anyhow::Error::msg("oops")
             .context("because uniffi told me so")
             .into(),
     ))
 }
 
+// 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<dyn ErrorTrait>> {
     Err(Arc::new(ErrorTraitImpl {

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")

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()

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)
+assert(Error.self != Swift.Error.self)

uniffi_core/src/ffi_converter_impls.rs

@@ -23,16 +23,16 @@
 /// "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};
 use paste::paste;
 use std::{
     collections::HashMap,
     convert::TryFrom,
-    error::Error,
+    fmt::{Debug, Display},
     sync::Arc,
     time::{Duration, SystemTime},
 };
@@ -425,21 +425,25 @@ derive_ffi_traits!(blanket SystemTime);
 // Note that this means we don't get specialized return handling.  For example, if we could return
 // an `Option<Result<>>` we would always return that type directly and never throw.
 derive_ffi_traits!(impl<T, UT> LowerReturn<UT> for Option<T> where Option<T>: Lower<UT>);
+derive_ffi_traits!(impl<T, UT> LowerError<UT> for Option<T> where Option<T>: Lower<UT>);
 derive_ffi_traits!(impl<T, UT> LiftReturn<UT> for Option<T> where Option<T>: Lift<UT>);
 derive_ffi_traits!(impl<T, UT> LiftRef<UT> for Option<T> where Option<T>: Lift<UT>);
 
 derive_ffi_traits!(impl<T, UT> LowerReturn<UT> for Vec<T> where Vec<T>: Lower<UT>);
+derive_ffi_traits!(impl<T, UT> LowerError<UT> for Vec<T> where Vec<T>: Lower<UT>);
 derive_ffi_traits!(impl<T, UT> LiftReturn<UT> for Vec<T> where Vec<T>: Lift<UT>);
 derive_ffi_traits!(impl<T, UT> LiftRef<UT> for Vec<T> where Vec<T>: Lift<UT>);
 
 derive_ffi_traits!(impl<K, V, UT> LowerReturn<UT> for HashMap<K, V> where HashMap<K, V>: Lower<UT>);
+derive_ffi_traits!(impl<K, V, UT> LowerError<UT> for HashMap<K, V> where HashMap<K, V>: Lower<UT>);
 derive_ffi_traits!(impl<K, V, UT> LiftReturn<UT> for HashMap<K, V> where HashMap<K, V>: Lift<UT>);
 derive_ffi_traits!(impl<K, V, UT> LiftRef<UT> for HashMap<K, V> where HashMap<K, V>: Lift<UT>);
 
 // For Arc we derive all the traits, but have to write it all out because we need an unsized T bound
 derive_ffi_traits!(impl<T, UT> Lower<UT> for Arc<T> where Arc<T>: FfiConverter<UT>, T: ?Sized);
 derive_ffi_traits!(impl<T, UT> Lift<UT> for Arc<T> where Arc<T>: FfiConverter<UT>, T: ?Sized);
 derive_ffi_traits!(impl<T, UT> LowerReturn<UT> for Arc<T> where Arc<T>: Lower<UT>, T: ?Sized);
+derive_ffi_traits!(impl<T, UT> LowerError<UT> for Arc<T> where Arc<T>: Lower<UT>, T: ?Sized);
 derive_ffi_traits!(impl<T, UT> LiftReturn<UT> for Arc<T> where Arc<T>: Lift<UT>, T: ?Sized);
 derive_ffi_traits!(impl<T, UT> LiftRef<UT> for Arc<T> where Arc<T>: Lift<UT>, T: ?Sized);
 derive_ffi_traits!(impl<T, UT> TypeId<UT> for Arc<T> where Arc<T>: FfiConverter<UT>, T: ?Sized);
@@ -472,14 +476,14 @@ impl<UT> TypeId<UT> for () {
 unsafe impl<UT, R, E> LowerReturn<UT> for Result<R, E>
 where
     R: LowerReturn<UT>,
-    E: Lower<UT> + Error + Send + Sync + 'static,
+    E: LowerError<UT> + Display + Debug + Send + Sync + 'static,
 {
     type ReturnType = R::ReturnType;
 
     fn lower_return(v: Self) -> Result<Self::ReturnType, RustBuffer> {
         match v {
             Ok(r) => R::lower_return(r),
-            Err(e) => Err(E::lower_into_rust_buffer(e)),
+            Err(e) => Err(E::lower_error(e)),
         }
     }
 

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<UT>: 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<UT>: Sized {
     }
 }
 
+/// Return Rust error values
+///
+/// This is implemented for types that can be the `E` param in `Result<T, E>`.
+/// 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<UT>: 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<UT> Lower<UT> for $ty);
         $crate::derive_ffi_traits!(impl<UT> Lift<UT> for $ty);
         $crate::derive_ffi_traits!(impl<UT> LowerReturn<UT> for $ty);
+        $crate::derive_ffi_traits!(impl<UT> LowerError<UT> for $ty);
         $crate::derive_ffi_traits!(impl<UT> LiftReturn<UT> for $ty);
         $crate::derive_ffi_traits!(impl<UT> LiftRef<UT> for $ty);
         $crate::derive_ffi_traits!(impl<UT> ConvertError<UT> for $ty);
@@ -477,6 +498,7 @@ macro_rules! derive_ffi_traits {
         $crate::derive_ffi_traits!(impl Lower<crate::UniFfiTag> for $ty);
         $crate::derive_ffi_traits!(impl Lift<crate::UniFfiTag> for $ty);
         $crate::derive_ffi_traits!(impl LowerReturn<crate::UniFfiTag> for $ty);
+        $crate::derive_ffi_traits!(impl LowerError<crate::UniFfiTag> for $ty);
         $crate::derive_ffi_traits!(impl LiftReturn<crate::UniFfiTag> for $ty);
         $crate::derive_ffi_traits!(impl LiftRef<crate::UniFfiTag> for $ty);
         $crate::derive_ffi_traits!(impl ConvertError<crate::UniFfiTag> 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 {
+                <Self as $crate::Lower<$ut>>::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)*)*
         {

uniffi_core/src/lib.rs

@@ -48,7 +48,7 @@ pub use ffi::ffiserialize::FfiBufferElement;
 pub use ffi::*;
 pub use ffi_converter_traits::{
     ConvertError, FfiConverter, FfiConverterArc, HandleAlloc, Lift, LiftRef, LiftReturn, Lower,
-    LowerReturn, TypeId,
+    LowerError, LowerReturn, TypeId,
 };
 pub use metadata::*;
 

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(),

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<crate::UniFfiTag>>::lower_error
+    }
+}
+
 /// Lift return type
 pub fn lift_return_type(ty: impl ToTokens) -> TokenStream {
     quote! {

uniffi_macros/src/object.rs

@@ -99,18 +99,20 @@ 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() {
         Ok(p) => p,
         Err(e) => return e.into_compile_error(),
     };
     let arc_self_type = quote! { ::std::sync::Arc<Self> };
-    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<Self>, buf: &mut Vec<u8>) {
                 ::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<Self>> {
                 ::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<Self::ReturnType, ::uniffi::RustBuffer> {
-                #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;
         }
     }
 }