Added return handling to FfiConverter

Added the `FfiConverter::lower_return` method.  This is like `lower()`
but specialized for scaffolding returns.  This allows us to always use a
single function to handle scaffolding calls, rather than
`call_with_output` or `call_with_result` depending on if the return type
is a `Result<>` or not.

Having a single code-path for return handling simplifies the code
generation, especially the macros.  We no longer need to try to parse
`Result<>` type names.  This is especially useful for crates that
type-alias their `Result<>` types.

Updated `object_references.md` and removed example code that's no longer
valid.  Replaced it with higher-level descriptions of what's going on,
hopefully this will stay not get out of date as quickly.

docs/manual/src/internals/object_references.md

@@ -56,91 +56,30 @@ interface TodoList {
 };
 ```
 
-On the Rust side of the generated bindings, the instance constructor will create an instance of the
-corresponding `TodoList` Rust struct, wrap it in an `Arc<>` and return the Arc's raw pointer to the
-foreign language code:
-
-```rust
-pub extern "C" fn todolist_12ba_TodoList_new(
- err: &mut uniffi::deps::ffi_support::ExternError,
-) -> *const std::os::raw::c_void /* *const TodoList */ {
- uniffi::deps::ffi_support::call_with_output(err, || {
- let _new = TodoList::new();
- let _arc = std::sync::Arc::new(_new);
- <std::sync::Arc<TodoList> as uniffi::FfiConverter>::lower(_arc)
- })
-}
-```
-
-The UniFFI runtime implements lowering for object instances using `Arc::into_raw`:
-
-```rust
-unsafe impl<T: Sync + Send> FfiConverter for std::sync::Arc<T> {
- type FfiType = *const std::os::raw::c_void;
- fn lower(self) -> Self::FfiType {
- std::sync::Arc::into_raw(self) as Self::FfiType
- }
-}
-```
-
-which does the "arc to pointer" dance for us. Note that this has "leaked" the
-`Arc<>` reference out of Rusts ownership system and given it to the foreign-language code.
-The foreign-language code must pass that pointer back into Rust in order to free it,
-or our instance will leak.
-
-When invoking a method on the instance, the foreign-language code passes the
-raw pointer back to the Rust code, conceptually passing a "borrow" of the `Arc<>` to
-the Rust scaffolding. The Rust side turns it back into a cloned `Arc<>` which
-lives for the duration of the method call:
-
-```rust
-pub extern "C" fn todolist_12ba_TodoList_add_item(
- ptr: *const std::os::raw::c_void,
- todo: uniffi::RustBuffer,
- err: &mut uniffi::deps::ffi_support::ExternError,
-) -> () {
- uniffi::deps::ffi_support::call_with_result(err, || -> Result<_, TodoError> {
- let _retval = TodoList::add_item(
- &<std::sync::Arc<TodoList> as uniffi::FfiConverter>::try_lift(ptr).unwrap(),
- <String as uniffi::FfiConverter>::try_lift(todo).unwrap())?,
- )
- Ok(_retval)
- })
-}
-```
-
-The UniFFI runtime implements lifting for object instances using `Arc::from_raw`:
-
-```rust
-unsafe impl<T: Sync + Send> FfiConverter for std::sync::Arc<T> {
- type FfiType = *const std::os::raw::c_void;
- fn try_lift(v: Self::FfiType) -> Result<Self> {
- 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 { Self::from_raw(v) });
- // Take a clone for our own use.
- Ok(std::sync::Arc::clone(&*foreign_arc))
- }
-```
-
-Notice that we take care to ensure the reference that is owned by the foreign-language
-code remains alive.
+On the Rust side of the generated bindings:
+ - The instance constructor will create an instance of the corresponding `TodoList` Rust struct
+ - The owned value is wrapped it in an `Arc<>`
+ - The `Arc<>` is lowered into the foreign code using `Arc::into_raw` and returned as an object pointer.
+
+This is the "arc to pointer" dance. Note that this has "leaked" the `Arc<>`
+reference out of Rusts ownership system and given it to the foreign-language
+code. The foreign-language code must pass that pointer back into Rust in order
+to free it, or our instance will leak.
+
+When invoking a method on the instance:
+ - The foreign-language code passes the raw pointer back to the Rust code, conceptually passing a "borrow" of the `Arc<>` to the Rust scaffolding.
+ - The Rust side calls `Arc::from_raw` to convert the pointer into an an `Arc<>`
+ - It wraps the `Arc` in `std::mem::ManuallyDrop<>`, which we never actually
+ drop. This is because the Rust side is are borrowing the Arc and shouldn't
+ run the destructor and decrement the reference count.
+ - The `Arc<>` is cloned and returned to the Rust code
 
 Finally, when the foreign-language code frees the instance, it
 passes the raw pointer a special destructor function so that the Rust code can
 drop that initial reference (and if that happens to be the final reference,
-the Rust object will be dropped.)
-
-```rust
-pub extern "C" fn ffi_todolist_12ba_TodoList_object_free(ptr: *const std::os::raw::c_void) {
- if let Err(e) = std::panic::catch_unwind(|| {
- assert!(!ptr.is_null());
- unsafe { std::sync::Arc::from_raw(ptr as *const TodoList) };
- }) {
- uniffi::deps::log::error!("ffi_todolist_12ba_TodoList_object_free panicked: {:?}", e);
- }
-}
-```
+the Rust object will be dropped.). This simply calls `Arc::from_raw`, then
+lets the value drop.
+
 
 Passing instances as arguments and returning them as values works similarly, except that
 UniFFI does not automatically wrap/unwrap the containing `Arc`.

fixtures/keywords/rust/src/lib.rs

@@ -24,7 +24,7 @@ impl r#break {
 }
 
 #[allow(non_camel_case_types)]
-trait r#continue {
+pub trait r#continue {
  fn r#return(&self, v: r#return) -> r#return;
  fn r#continue(&self, v: Vec<Box<dyn r#continue>>) -> Option<Box<dyn r#continue>>;
  fn r#break(&self, _v: Option<Arc<r#break>>) -> HashMap<u8, Arc<r#break>>;

fixtures/uitests/tests/ui/interface_cannot_use_mut_self.stderr

@@ -1,8 +1,8 @@
 error[E0308]: mismatched types
-  --> $OUT_DIR[uniffi_uitests]/counter.uniffi.rs
-  |
-  | Ok(ref val) => val,
-  | ^^^ types differ in mutability
-  |
-  = note: expected mutable reference `&mut Counter`
-  found reference `&Arc<Counter>`
+ --> $OUT_DIR[uniffi_uitests]/counter.uniffi.rs
+ |
+ | Ok(ref val) => val,
+ | ^^^ types differ in mutability
+ |
+ = note: expected mutable reference `&mut Counter`
+ found reference `&Arc<Counter>`

fixtures/uitests/tests/ui/non_hashable_record_key.stderr

@@ -1,60 +1,14 @@
-error[E0277]: the trait bound `f32: std::cmp::Eq` is not satisfied
- --> $OUT_DIR[uniffi_uitests]/records.uniffi.rs
- |
- | uniffi::deps::static_assertions::assert_impl_all!(f32: ::std::cmp::Eq, ::std::hash::Hash); // record<f32, u64>
- | ^^^ the trait `std::cmp::Eq` is not implemented for `f32`
- |
- = help: the following other types implement trait `std::cmp::Eq`:
- i128
- i16
- i32
- i64
- i8
- isize
- u128
- u16
- and $N others
-note: required by a bound in `assert_impl_all`
- --> $OUT_DIR[uniffi_uitests]/records.uniffi.rs
- |
- | uniffi::deps::static_assertions::assert_impl_all!(f32: ::std::cmp::Eq, ::std::hash::Hash); // record<f32, u64>
- | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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]: the trait bound `f32: Hash` is not satisfied
- --> $OUT_DIR[uniffi_uitests]/records.uniffi.rs
- |
- | uniffi::deps::static_assertions::assert_impl_all!(f32: ::std::cmp::Eq, ::std::hash::Hash); // record<f32, u64>
- | ^^^ the trait `Hash` is not implemented for `f32`
- |
- = help: the following other types implement trait `Hash`:
- i128
- i16
- i32
- i64
- i8
- isize
- u128
- u16
- and $N others
-note: required by a bound in `assert_impl_all`
- --> $OUT_DIR[uniffi_uitests]/records.uniffi.rs
- |
- | uniffi::deps::static_assertions::assert_impl_all!(f32: ::std::cmp::Eq, ::std::hash::Hash); // record<f32, u64>
- | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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[E0425]: cannot find function `get_dict` in this scope
- --> $OUT_DIR[uniffi_uitests]/records.uniffi.rs
- |
- | <std::collections::HashMap<f32, u64> as uniffi::FfiConverter<crate::UniFfiTag>>::lower(r#get_dict())
- | ^^^^^^^^^^ not found in this scope
-
 error[E0277]: the trait bound `f32: Hash` is not satisfied
  --> $OUT_DIR[uniffi_uitests]/records.uniffi.rs
  |
- | <std::collections::HashMap<f32, u64> as uniffi::FfiConverter<crate::UniFfiTag>>::lower(r#get_dict())
- | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Hash` is not implemented for `f32`
+ | / pub extern "C" fn r#uitests_82f4_get_dict(
+ | |
+ | | call_status: &mut uniffi::RustCallStatus
+ | | ) -> <std::collections::HashMap<f32, u64> as ::uniffi::FfiConverter<crate::UniFfiTag>>::ReturnType {
+... |
+ | | )
+ | | }
+ | |_^ the trait `Hash` is not implemented for `f32`
  |
  = help: the following other types implement trait `Hash`:
  i128
@@ -71,8 +25,14 @@ error[E0277]: the trait bound `f32: Hash` is not satisfied
 error[E0277]: the trait bound `f32: std::cmp::Eq` is not satisfied
  --> $OUT_DIR[uniffi_uitests]/records.uniffi.rs
  |
- | <std::collections::HashMap<f32, u64> as uniffi::FfiConverter<crate::UniFfiTag>>::lower(r#get_dict())
- | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::cmp::Eq` is not implemented for `f32`
+ | / pub extern "C" fn r#uitests_82f4_get_dict(
+ | |
+ | | call_status: &mut uniffi::RustCallStatus
+ | | ) -> <std::collections::HashMap<f32, u64> as ::uniffi::FfiConverter<crate::UniFfiTag>>::ReturnType {
+... |
+ | | )
+ | | }
+ | |_^ the trait `std::cmp::Eq` is not implemented for `f32`
  |
  = help: the following other types implement trait `std::cmp::Eq`:
  i128
@@ -85,3 +45,9 @@ error[E0277]: the trait bound `f32: std::cmp::Eq` is not satisfied
  u16
  and $N others
  = note: required for `HashMap<f32, u64>` to implement `FfiConverter<UniFfiTag>`
+
+error[E0425]: cannot find function `get_dict` in this scope
+ --> $OUT_DIR[uniffi_uitests]/records.uniffi.rs
+ |
+ | r#get_dict())
+ | ^^^^^^^^^^ not found in this scope

uniffi_bindgen/src/interface/error.rs

@@ -92,9 +92,8 @@ use super::{APIConverter, ComponentInterface};
 /// Represents an Error that might be thrown by functions/methods in the component interface.
 ///
 /// Errors are represented in the UDL as enums with the special `[Error]` attribute, but
-/// they're handled in the FFI very differently. We create them in `uniffi::call_with_result()` if
-/// the wrapped function returns an `Err` value
-/// struct and assign an integer error code to each variant.
+/// they're handled in the FFI very differently. Defining an error means that exported functions
+/// cat return a `Result<R, E>` where `R` is a UniFFI type and `E` is the error type.
 #[derive(Debug, Clone, PartialEq, Eq, Checksum)]
 pub struct Error {
  pub name: String,

uniffi_bindgen/src/interface/function.rs

@@ -247,6 +247,42 @@ impl APIConverter<Argument> for weedle::argument::SingleArgument<'_> {
  }
 }
 
+/// Implemented by function-like types (Function, Method, Constructor)
+pub trait Callable {
+ fn arguments(&self) -> Vec<&Argument>;
+ fn return_type(&self) -> Option<Type>;
+ fn throws_type(&self) -> Option<Type>;
+}
+
+impl Callable for Function {
+ fn arguments(&self) -> Vec<&Argument> {
+ self.arguments()
+ }
+
+ fn return_type(&self) -> Option<Type> {
+ self.return_type().cloned()
+ }
+
+ fn throws_type(&self) -> Option<Type> {
+ self.throws_type()
+ }
+}
+
+// Needed because Askama likes to add extra refs to variables
+impl<T: Callable> Callable for &T {
+ fn arguments(&self) -> Vec<&Argument> {
+ (*self).arguments()
+ }
+
+ fn return_type(&self) -> Option<Type> {
+ (*self).return_type()
+ }
+
+ fn throws_type(&self) -> Option<Type> {
+ (*self).throws_type()
+ }
+}
+
 #[cfg(test)]
 mod test {
  use super::*;

uniffi_bindgen/src/interface/mod.rs

@@ -64,7 +64,7 @@ pub use enum_::Enum;
 mod error;
 pub use error::Error;
 mod function;
-pub use function::{Argument, Function};
+pub use function::{Argument, Callable, Function};
 mod literal;
 pub use literal::{Literal, Radix};
 mod namespace;

uniffi_bindgen/src/interface/object.rs

@@ -65,7 +65,7 @@ use uniffi_meta::Checksum;
 
 use super::attributes::{Attribute, ConstructorAttributes, InterfaceAttributes, MethodAttributes};
 use super::ffi::{FfiArgument, FfiFunction, FfiType};
-use super::function::Argument;
+use super::function::{Argument, Callable};
 use super::types::{Type, TypeIterator};
 use super::{convert_type, APIConverter, ComponentInterface};
 
@@ -211,10 +211,11 @@ impl APIConverter<Object> for weedle::InterfaceDefinition<'_> {
  for member in &self.members.body {
  match member {
  weedle::interface::InterfaceMember::Constructor(t) => {
- let cons: Constructor = t.convert(ci)?;
+ let mut cons: Constructor = t.convert(ci)?;
  if !member_names.insert(cons.name.clone()) {
  bail!("Duplicate interface member name: \"{}\"", cons.name())
  }
+ cons.object_name = object.name.clone();
  object.constructors.push(cons);
  }
  weedle::interface::InterfaceMember::Operation(t) => {
@@ -239,6 +240,7 @@ impl APIConverter<Object> for weedle::InterfaceDefinition<'_> {
 #[derive(Debug, Clone, Checksum)]
 pub struct Constructor {
  pub(super) name: String,
+ pub(super) object_name: String,
  pub(super) arguments: Vec<Argument>,
  // We don't include the FFIFunc in the hash calculation, because:
  // - it is entirely determined by the other fields,
@@ -301,6 +303,8 @@ impl Default for Constructor {
  fn default() -> Self {
  Constructor {
  name: String::from("new"),
+ // We don't know the name of the containing `Object` at this point, fill it in later.
+ object_name: Default::default(),
  arguments: Vec::new(),
  ffi_func: Default::default(),
  attributes: Default::default(),
@@ -316,6 +320,8 @@ impl APIConverter<Constructor> for weedle::interface::ConstructorInterfaceMember
  };
  Ok(Constructor {
  name: String::from(attributes.get_name().unwrap_or("new")),
+ // We don't know the name of the containing `Object` at this point, fill it in later.
+ object_name: Default::default(),
  arguments: self.args.body.list.convert(ci)?,
  ffi_func: Default::default(),
  attributes,
@@ -477,6 +483,34 @@ impl APIConverter<Method> for weedle::interface::OperationInterfaceMember<'_> {
  }
 }
 
+impl Callable for Constructor {
+ fn arguments(&self) -> Vec<&Argument> {
+ self.arguments()
+ }
+
+ fn return_type(&self) -> Option<Type> {
+ Some(Type::Object(self.object_name.clone()))
+ }
+
+ fn throws_type(&self) -> Option<Type> {
+ self.throws_type()
+ }
+}
+
+impl Callable for Method {
+ fn arguments(&self) -> Vec<&Argument> {
+ self.arguments()
+ }
+
+ fn return_type(&self) -> Option<Type> {
+ self.return_type().cloned()
+ }
+
+ fn throws_type(&self) -> Option<Type> {
+ self.throws_type()
+ }
+}
+
 #[cfg(test)]
 mod test {
  use super::*;

uniffi_bindgen/src/macro_metadata/ci.rs

@@ -48,7 +48,7 @@ pub fn add_to_ci(
  Some(ns) => ns,
  None => bail!("Unknown namespace for {item_desc} ({crate_name})"),
  };
- if item_ns != &iface_ns {
+ if item_ns != iface_ns {
  return Err(anyhow!("Found {item_desc} from crate `{crate_name}`.")
  .context(format!(
  "Main crate is expected to be named `{iface_ns}` based on the UDL namespace."