Throw error when objects are passed instead of traits (#2649)

The corner case here is when an object implements a Rust-only trait.
In the updated coverall fixture we have the following Python classes:

  - StringUtilProtocol - interface for the trait
  - StringUtil - object that implements the trait via rust calls
  - StringUtilObject - exported Rust object that imprements the trait

Before `StringUtilObject` would inherit from `StringUtil`, which isn't
correct conceptually and in practice it means that the Python code will
try to lower the handle into a function that expects `Arc<dyn
StructUtil>` when the handle was actually for `Arc<StringUtilObject>`,
once Rust tries to use the arc a segfault will happen soon after.

Now `StringUtilObject` inherits from `StringUtilProtocol` and we don't
have the segfault.

I tried to add Kotlin and Swift tests here, but it doesn't work because
they throw a type error as soon as you try making that call.

Author: bendk

CHANGELOG.md

@@ -32,6 +32,7 @@
   * Methods now have typing annotations for return values ([#2625](https://github.com/mozilla/uniffi-rs/issues/2625))
   * Fix relative imports ([#2657](https://github.com/mozilla/uniffi-rs/pull/2657))
   * Fix shadowing param names with internal variables in Python (#2628/#2645)
+  * Don't allow objects to be passed as arguments when traits are expected (#2649)
 - Swift:
   * All object protocol conformances are public ([#2671](https://github.com/mozilla/uniffi-rs/pull/2671))
   * Initialization functions now have a stable ordering when using external types.

fixtures/coverall/src/traits.rs

@@ -261,3 +261,33 @@ impl StringUtil for StringUtilImpl2 {
         format!("{a}{b}")
     }
 }
+
+/// Object that implements `StringUtil`
+/// This lets us test what happens if foreign bindings pass in `StringUtilImpl` when `StringUtil` was expected.
+/// Ideally, languages should coerce the object to it's trait, however that's not supported at this time.
+/// The current test is simply that this doesn't cause a segfault (#2649)
+#[derive(uniffi::Object)]
+pub struct StringUtilObject {
+    separator: String,
+}
+
+#[uniffi::export]
+impl StringUtilObject {
+    #[uniffi::constructor]
+    pub fn new(separator: String) -> Self {
+        Self { separator }
+    }
+}
+
+#[uniffi::export]
+impl StringUtil for StringUtilObject {
+    fn concat(&self, a: &str, b: &str) -> String {
+        let separator = &self.separator;
+        format!("{a}{separator}{b}")
+    }
+}
+
+#[uniffi::export]
+pub fn concat_with_string_util(string_util: Arc<dyn StringUtil>, a: &str, b: &str) -> String {
+    string_util.concat(a, b)
+}

fixtures/coverall/tests/bindings/test_coverall.py

@@ -502,6 +502,17 @@ def test_rust_only_traits(self):
         self.assertEqual(traits[0].concat("cow", "boy"), "cowboy")
         self.assertEqual(traits[1].concat("cow", "boy"), "cowboy")
 
+    def test_pass_object_to_function_that_input_trait(self):
+        string_util_obj = StringUtilObject("--")
+
+        # StringUtilObject implements StringUtil.
+        # We currently don't support passing a `StringUtilObject` to a function that inputs a `dyn
+        # StringUtil`.
+        # This test checks that the result is a TypeError rather than a segfault (#2649)
+
+        with self.assertRaises(TypeError):
+            concat_with_string_util(string_util_obj, "cow", "boy"),
+
     def test_html_error(self):
         with self.assertRaises(HtmlError):
             validate_html("test")

fixtures/proc-macro/tests/bindings/test_proc_macro.py

@@ -30,7 +30,6 @@
 assert trait_impl.concat_strings("foo", "bar") == "foobar"
 assert obj.get_trait(trait_impl).concat_strings("foo", "bar") == "foobar"
 assert concat_strings_by_ref(trait_impl, "foo", "bar") == "foobar"
-assert issubclass(StructWithTrait, Trait)
 assert StructWithTrait("me").concat_strings("foo", "bar") == "me: foobar"
 
 trait_impl2 = obj.get_trait_with_foreign(None)

uniffi_bindgen/src/bindings/python/pipeline/interfaces.rs

@@ -43,13 +43,25 @@ pub fn pass(int: &mut Interface) -> Result<()> {
             Type::Interface {
                 name,
                 external_package_name,
+                imp,
                 ..
-            } => (name, external_package_name),
+            } => {
+                // For trait interfaces implement in Rust-only, the protocol has `Protocol` appended.
+                // Trait interfaces with foreign implementations don't have that
+                match imp {
+                    ObjectImpl::Trait => (format!("{name}Protocol"), external_package_name),
+                    ObjectImpl::CallbackTrait => (name.to_string(), external_package_name),
+                    ObjectImpl::Struct => {
+                        bail!("Objects can only inherit from traits, not other objects")
+                    }
+                }
+            }
+
             Type::CallbackInterface {
                 name,
                 external_package_name,
                 ..
-            } => (name, external_package_name),
+            } => (name.to_string(), external_package_name),
             _ => bail!("trait_ty {:?} isn't a trait", t),
         };
         let fq = match external_package_name {

uniffi_bindgen/src/bindings/python/pipeline/mod.rs

@@ -28,8 +28,8 @@ pub fn pipeline() -> Pipeline<initial::Root, Root> {
         .pass(config::pass)
         .pass(external_types::pass)
         .pass(names::pass)
-        .pass(modules::pass)
         .pass(interfaces::pass)
+        .pass(modules::pass)
         .pass(callback_interfaces::pass)
         .pass(types::pass)
         .pass(default::pass)

uniffi_bindgen/src/bindings/python/pipeline/modules.rs

@@ -33,7 +33,10 @@ pub fn pass(root: &mut Root) -> Result<()> {
         namespace.visit(|e: &Enum| exported_names.push(e.name.clone()));
         namespace.visit(|r: &Record| exported_names.push(r.name.clone()));
         namespace.visit(|f: &Function| exported_names.push(f.callable.name.clone()));
-        namespace.visit(|i: &Interface| exported_names.push(i.name.clone()));
+        namespace.visit(|i: &Interface| {
+            exported_names.push(i.name.clone());
+            exported_names.push(i.protocol.name.clone());
+        });
         namespace.visit(|c: &CallbackInterface| exported_names.push(c.name.clone()));
         namespace.exported_names = exported_names;