Use handles for better trait interfaces passing

Check if the trait interface handle originates from the other side of
the FFI.  If it does, clone the handle, rather than returning a handle
to the wrapped object.  Before, each time the trait object was passed
across the FFI, we wrapped it another time

On the foreign side, this can be accomplished by a type check.  On the
Rust side, this requires an extra, `#[doc(hidden)]` method on the trait.
This means that UDL-defined trait interfaces need to be wrapped with an
attribute macro that inserts that method.

Reworked the changelog so that breaking changes for external bindings is
its own section.

Removed the reexport-scaffolding-macro fixture which often requires
changes when the FFI changes.  I don't think it's giving us enough value
at this point to justify continuing to update it.

CHANGELOG.md

@@ -16,13 +16,22 @@
 
 ### What's changed?
 
+- Trait interfaces defined in UDL need to be wrapped with `#[uniffi::trait_interface]`.
+- Trait interfaces performance has been improved.  If a trait interface handle is passed across the
+  FFI multiple times, UniFFI will only wrap the object once rather than each time it's passed.
+
+### Breaking changes for external bindings:
+
 - The `rust_future_continuation_callback_set` FFI function was removed.  `rust_future_poll` now
-  inputs the callback pointer.  External bindings authors will need to update their code.
-- The object handle FFI has changed.  External bindings generators will need to update their code to
-  use the new handle system:
-    * A single `FfiType::Handle` is used for all object handles.
-    * `FfiType::Handle` is always a 64-bit int.
-    * Foreign handles must always set the lowest bit of that int.
+  inputs the callback pointer.
+- The object handle FFI has changed.
+  * A single `FfiType::Handle` is used for all object handles.
+  * `FfiType::Handle` is always a 64-bit int.
+  * Foreign handles must always set the lowest bit of that int.
+  * Handles are now cloned before they are returned:
+    * Clone Rust handles before lowering them
+    * If you implement trait interfaces: implement the `IDX_CALLBACK_CLONE` special method for
+      callback interfaces so that Rust can clone your handles and consume the handle in `lift`.
 
 ## v0.25.0 (backend crates: v0.25.0) - (_2023-10-18_)
 

Cargo.toml

@@ -37,7 +37,6 @@ members = [
   "fixtures/keywords/swift",
   "fixtures/metadata",
   "fixtures/proc-macro",
-  "fixtures/reexport-scaffolding-macro",
   "fixtures/regressions/enum-without-i32-helpers",
   "fixtures/regressions/fully-qualified-types",
   "fixtures/regressions/kotlin-experimental-unsigned-types",

docs/manual/src/foreign_traits.md

@@ -22,6 +22,7 @@ a [compatible error type](./udl/errors.md) - see below for more on error handlin
 For example:
 
 ```rust,no_run
+#[uniffi::trait_interface]
 pub trait Keychain: Send + Sync + Debug {
   fn get(&self, key: String) -> Result<Option<String>, KeyChainError>;
   fn put(&self, key: String, value: String) -> Result<(), KeyChainError>;

docs/manual/src/proc_macro/index.md

@@ -84,6 +84,8 @@ impl MyObject {
 // Corresponding UDL:
 // [Trait]
 // interface MyTrait {};
+//
+// Note: `[uniffi::trait_interface]` is not needed when the trait is exported via a proc-macro.
 #[uniffi::export]
 trait MyTrait {
     // ...

docs/manual/src/udl/interfaces.md

@@ -87,9 +87,10 @@ interface Button {
 
 ```
 
-With the following Rust implementation:
+The Rust implementation needs to be wrapped in `#[uniffi::trait_interface]`:
 
 ```rust
+#[uniffi::trait_interface]
 pub trait Button: Send + Sync {
     fn name(&self) -> String;
 }

examples/callbacks/src/lib.rs

@@ -21,6 +21,7 @@ impl From<uniffi::UnexpectedUniFFICallbackError> for TelephoneError {
 }
 
 // SIM cards.
+#[uniffi::trait_interface]
 pub trait SimCard: Send + Sync {
     fn name(&self) -> String;
 }

examples/traits/src/lib.rs

@@ -13,6 +13,7 @@ fn press(button: Arc<dyn Button>) -> Arc<dyn Button> {
     button
 }
 
+#[uniffi::trait_interface]
 pub trait Button: Send + Sync {
     fn name(&self) -> String;
 }

fixtures/coverall/src/traits.rs

@@ -10,6 +10,7 @@ pub fn get_traits() -> Vec<Arc<dyn NodeTrait>> {
     vec![Arc::new(Trait1::default()), Arc::new(Trait2::default())]
 }
 
+#[uniffi::trait_interface]
 pub trait NodeTrait: Send + Sync + std::fmt::Debug {
     fn name(&self) -> String;
 
@@ -35,6 +36,7 @@ pub fn ancestor_names(node: Arc<dyn NodeTrait>) -> Vec<String> {
 /// Test trait
 ///
 /// The goal here is to test all possible arg, return, and error types.
+#[uniffi::trait_interface]
 pub trait Getters: Send + Sync {
     fn get_bool(&self, v: bool, arg2: bool) -> bool;
     fn get_string(&self, v: String, arg2: bool) -> Result<String, CoverallError>;

fixtures/coverall/tests/bindings/test_coverall.kts

@@ -353,12 +353,10 @@ getTraits().let { traits ->
     assert(traits[1].name() == "node-2")
     assert(traits[1].strongCount() == 2UL)
 
-    // Note: this doesn't increase the Rust strong count, since we wrap the Rust impl with a
-    // Swift impl before passing it to `setParent()`
     traits[0].setParent(traits[1])
     assert(ancestorNames(traits[0]) == listOf("node-2"))
     assert(ancestorNames(traits[1]).isEmpty())
-    assert(traits[1].strongCount() == 2UL)
+    assert(traits[1].strongCount() == 3UL)
     assert(traits[0].getParent()!!.name() == "node-2")
 
     val ktNode = KotlinNode()
@@ -367,6 +365,10 @@ getTraits().let { traits ->
     assert(ancestorNames(traits[1]) == listOf("node-kt"))
     assert(ancestorNames(ktNode) == listOf<String>())
 
+    // If we get the node back from Rust, we should get the original object, not an object that's
+    // been wrapped again.
+    assert(traits[1].getParent() === ktNode)
+
     traits[1].setParent(null)
     ktNode.setParent(traits[0])
     assert(ancestorNames(ktNode) == listOf("node-1", "node-2"))

fixtures/coverall/tests/bindings/test_coverall.py

@@ -335,14 +335,14 @@ def strong_count(self):
 
 class TraitsTest(unittest.TestCase):
     # Test traits implemented in Rust
-    # def test_rust_getters(self):
-    #     test_getters(None)
-    #     self.check_getters_from_python(make_rust_getters())
+    def test_rust_getters(self):
+        test_getters(make_rust_getters())
+        self.check_getters_from_python(make_rust_getters())
 
-    # Test traits implemented in Rust
+    # Test traits implemented in Python
     def test_python_getters(self):
         test_getters(PyGetters())
-        #self.check_getters_from_python(PyGetters())
+        self.check_getters_from_python(PyGetters())
 
     def check_getters_from_python(self, getters):
         self.assertEqual(getters.get_bool(True, True), False);
@@ -373,7 +373,11 @@ def check_getters_from_python(self, getters):
         with self.assertRaises(ComplexError.UnknownError):
             getters.get_option("unknown-error", True)
 
-        with self.assertRaises(InternalError):
+        # If the trait is implmented in Rust, we should see an `InternalError`.
+
+        # If it's implemented in Python, we see a `RuntimeError` since it's a direct call with no
+        # UniFFI wrapping.
+        with self.assertRaises((InternalError, RuntimeError)):
             getters.get_string("unexpected-error", True)
 
     def test_path(self):
@@ -389,9 +393,7 @@ def test_path(self):
 
         # Let's try connecting them together
         traits[0].set_parent(traits[1])
-        # Note: this doesn't increase the Rust strong count, since we wrap the Rust impl with a
-        # python impl before passing it to `set_parent()`
-        self.assertEqual(traits[1].strong_count(), 2)
+        self.assertEqual(traits[1].strong_count(), 3)
         self.assertEqual(ancestor_names(traits[0]), ["node-2"])
         self.assertEqual(ancestor_names(traits[1]), [])
         self.assertEqual(traits[0].get_parent().name(), "node-2")
@@ -404,6 +406,10 @@ def test_path(self):
         self.assertEqual(ancestor_names(traits[1]), ["node-py"])
         self.assertEqual(ancestor_names(py_node), [])
 
+        # If we get the node back from Rust, we should get the original object, not an object that's
+        # been wrapped again.
+        self.assertIs(traits[1].get_parent(), py_node)
+
         # Rotating things.
         # The ancestry chain now goes py_node -> traits[0] -> traits[1]
         traits[1].set_parent(None)

fixtures/coverall/tests/bindings/test_coverall.swift

@@ -388,12 +388,10 @@ do {
     assert(traits[1].name() == "node-2")
     assert(traits[1].strongCount() == 2)
 
-    // Note: this doesn't increase the Rust strong count, since we wrap the Rust impl with a
-    // Swift impl before passing it to `set_parent()`
     traits[0].setParent(parent: traits[1])
     assert(ancestorNames(node: traits[0]) == ["node-2"])
     assert(ancestorNames(node: traits[1]) == [])
-    assert(traits[1].strongCount() == 2)
+    assert(traits[1].strongCount() == 3)
     assert(traits[0].getParent()!.name() == "node-2")
 
     // Throw in a Swift implementation of the trait
@@ -404,6 +402,10 @@ do {
     assert(ancestorNames(node: traits[1]) == ["node-swift"])
     assert(ancestorNames(node: swiftNode) == [])
 
+    // If we get the node back from Rust, we should get the original object, not an object that's
+    // been wrapped again.
+    assert(traits[1].getParent() === swiftNode)
+
     // Rotating things.
     // The ancestry chain now goes swiftNode -> traits[0] -> traits[1]
     traits[1].setParent(parent: nil)