Foreign-implemented trait interfaces

This adds support for the foreign bindings side to implement Rust traits
and for both sides to pass `Arc<dyn [Trait]>` objects back and forth.

I thought this would be a good time to redo the callback interface code.
The trait interface code uses a foreign-supplied vtable rather than a
single callback and method indexes like the old code used.

Renamed `CALL_PANIC` to `CALL_UNEXPECTED_ERROR` in the foreign bindings
templates.  This matches the name in the Rust code and makes more sense
for foreign trait implementations.

-------------------- TODO -----------------------------

This currently requires "wrapping" the object every time it's passed
across the FFI.  If the foreign code receives a trait object, then
passes it back to Rust.  Rust now has a handle to the foreign impl and
that foreign impl just calls back into Rust.  I think mozilla#1730 could help
solve this.

I think there should be better tests for reference counts, but I'm
waiting until we address the previous issue to implement them.

Python requires a hack to return RustBuffer from callbacks
(RubiconCtypesPatch.py) because of an old ctypes bug
(https://bugs.python.org/issue5710).  The options I can see are:
  - Go back to the callback interface style, where we input a RustBuffer
    and write the output to it.
  - Pass the return type as an out pointer.  This requires some special
    handling for void returns though.
  - Implement mozilla#1779, which changes RustBuffer to a simple `*mut u8` and
    then Python could return it.  This is my preference, I don't like
    the idea of making the ABI worse just because of Python.

TODO: If I make the Python tests fail, I see `RuntimeWarning: memory
leak in callback function` in the console.  I believe this is because of
the rubicon hack, but I'm not sure.  We should double check this before
merging.

Kotlin is not implemented yet.  I think when we do implement it, we're
going to want to face mozilla#1394 and make a decision there.  I don't think
it's reasonable to have consumers have to call a `close()` method on
these interfaces.  I also don't know how we would deal with this in
Rust.  ISTM that the same logic that says Kotlin objects need to be
manually closed dictates that if you use a Kotlin object in Rust, there
should be a `close()` method or something analogous to a to
`AutoCloseable`.

The Ruby coverall tests are now failing because they don't implement
trait interfaces.  We need to figure out a way for them to pass the test
suite.

Remove all the debugging printouts

fixtures/coverall/src/lib.rs

@@ -20,6 +20,12 @@ pub enum CoverallError {
     TooManyHoles,
 }
 
+impl From<uniffi::UnexpectedUniFFICallbackError> for CoverallError {
+    fn from(_e: uniffi::UnexpectedUniFFICallbackError) -> Self {
+        panic!("Saw UnexpectedUniFFICallbackError when a CoverallError was expected")
+    }
+}
+
 #[derive(Debug, thiserror::Error)]
 pub enum CoverallFlatError {
     #[error("Too many variants: {num}")]
@@ -90,6 +96,12 @@ pub enum ComplexError {
     UnknownError,
 }
 
+impl From<uniffi::UnexpectedUniFFICallbackError> for ComplexError {
+    fn from(_e: uniffi::UnexpectedUniFFICallbackError) -> Self {
+        Self::UnknownError
+    }
+}
+
 #[derive(Debug, thiserror::Error, uniffi::Error)]
 pub enum ComplexMacroError {
     #[error("OsError: {code} ({extended_code})")]

fixtures/coverall/tests/bindings/test_coverall.py

@@ -278,11 +278,69 @@ def test_bytes(self):
         coveralls = Coveralls("test_bytes")
         self.assertEqual(coveralls.reverse(b"123"), b"321")
 
+class PyGetters:
+    def get_bool(self, v, arg2):
+        print(f"get_bool {v} {arg2}", v ^ arg2)
+        return v ^ arg2
+
+    def get_string(self, v, arg2):
+        if v == "too-many-holes":
+            raise CoverallError.TooManyHoles
+        elif v == "unexpected-error":
+            raise RuntimeError("unexpected error")
+        elif arg2:
+            return v.upper()
+        else:
+            return v
+
+    def get_option(self, v, arg2):
+        if v == "os-error":
+            raise ComplexError.OsError(100, 200)
+        elif v == "unknown-error":
+            raise ComplexError.UnknownError
+        elif arg2:
+            if v:
+                return v.upper()
+            else:
+                return None
+        else:
+            return v
+
+    def get_list(self, v, arg2):
+        if arg2:
+            return v
+        else:
+            return []
+
+    def get_nothing(self, _v):
+        return None
+
+class PyNode:
+    def __init__(self):
+        self.parent = None
+
+    def name(self):
+        return "node-py"
+
+    def set_parent(self, parent):
+        self.parent = parent
+
+    def get_parent(self):
+        return self.parent
+
+    def strong_count(self):
+        return 0 # TODO
+
 class TraitsTest(unittest.TestCase):
     # Test traits implemented in Rust
-    def test_rust_getters(self):
-        test_getters(make_rust_getters())
-        self.check_getters_from_python(make_rust_getters())
+    # def test_rust_getters(self):
+    #     test_getters(None)
+    #     self.check_getters_from_python(make_rust_getters())
+
+    # Test traits implemented in Rust
+    def test_python_getters(self):
+        test_getters(PyGetters())
+        #self.check_getters_from_python(PyGetters())
 
     def check_getters_from_python(self, getters):
         self.assertEqual(getters.get_bool(True, True), False);
@@ -317,6 +375,7 @@ def check_getters_from_python(self, getters):
             getters.get_string("unexpected-error", True)
 
     def test_path(self):
+        # Get traits creates 2 objects that implement the trait
         traits = get_traits()
         self.assertEqual(traits[0].name(), "node-1")
         # Note: strong counts are 1 more than you might expect, because the strong_count() method
@@ -326,11 +385,33 @@ def test_path(self):
         self.assertEqual(traits[1].name(), "node-2")
         self.assertEqual(traits[1].strong_count(), 2)
 
+        # 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(ancestor_names(traits[0]), ["node-2"])
         self.assertEqual(ancestor_names(traits[1]), [])
-        self.assertEqual(traits[1].strong_count(), 3)
         self.assertEqual(traits[0].get_parent().name(), "node-2")
+
+        # Throw in a Python implementation of the trait
+        # The ancestry chain now goes traits[0] -> traits[1] -> py_node
+        py_node = PyNode()
+        traits[1].set_parent(py_node)
+        self.assertEqual(ancestor_names(traits[0]), ["node-2", "node-py"])
+        self.assertEqual(ancestor_names(traits[1]), ["node-py"])
+        self.assertEqual(ancestor_names(py_node), [])
+
+        # Rotating things.
+        # The ancestry chain now goes py_node -> traits[0] -> traits[1]
+        traits[1].set_parent(None)
+        py_node.set_parent(traits[0])
+        self.assertEqual(ancestor_names(py_node), ["node-1", "node-2"])
+        self.assertEqual(ancestor_names(traits[0]), ["node-2"])
+        self.assertEqual(ancestor_names(traits[1]), [])
+
+        # Make sure we don't crash when undoing it all
+        py_node.set_parent(None)
         traits[0].set_parent(None)
 
 if __name__=='__main__':

fixtures/coverall/tests/bindings/test_coverall.swift

@@ -319,10 +319,12 @@ 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
+    // python 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() == 3)
+    assert(traits[1].strongCount() == 2)
     assert(traits[0].getParent()!.name() == "node-2")
     traits[0].setParent(parent: nil)
 }

uniffi_bindgen/src/bindings/kotlin/gen_kotlin/mod.rs

@@ -313,6 +313,7 @@ impl KotlinCodeOracle {
                 "UniFffiRustFutureContinuationCallbackType".to_string()
             }
             FfiType::RustFutureContinuationData => "USize".to_string(),
+            FfiType::VTable(_name) => unimplemented!(),
         }
     }
 }

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

@@ -314,6 +314,33 @@ impl PythonCodeOracle {
             FfiType::RustFutureHandle => "ctypes.c_void_p".to_string(),
             FfiType::RustFutureContinuationCallback => "_UNIFFI_FUTURE_CONTINUATION_T".to_string(),
             FfiType::RustFutureContinuationData => "ctypes.c_size_t".to_string(),
+            // VTables are passed as a pointer to the struct.  Use a c_void_p since we want this to
+            // work before the struct is defined.
+            FfiType::VTable(_) => format!("ctypes.c_void_p"),
+        }
+    }
+
+    fn return_type_default(callable: impl Callable) -> String {
+        match callable.return_type() {
+            None => "None".to_owned(),
+            Some(t) => match t.clone().into() {
+                FfiType::Int8
+                | FfiType::UInt8
+                | FfiType::Int16
+                | FfiType::UInt16
+                | FfiType::Int32
+                | FfiType::UInt32
+                | FfiType::Int64
+                | FfiType::UInt64 => "0".to_owned(),
+                | FfiType::Float32
+                | FfiType::Float64 => "0.0".to_owned(),
+                FfiType::RustArcPtr(_) => "None".to_owned(),
+                FfiType::RustBuffer(maybe_suffix) => match maybe_suffix {
+                    Some(suffix) => format!("_UniffiRustBuffer{suffix}.default()"),
+                    None => "_UniffiRustBuffer.default()".to_owned(),
+                },
+                _ => panic!("Don't know how to return {t:?}"),
+            },
         }
     }
 }
@@ -412,6 +439,26 @@ pub mod filters {
         Ok(type_.into())
     }
 
+    pub fn return_type_default(callable: impl Callable) -> Result<String, askama::Error> {
+        Ok(PythonCodeOracle::return_type_default(callable))
+    }
+
+    pub fn ctypes_ffi_prototype(ffi_func: &FfiFunction) -> Result<String, askama::Error> {
+        Ok(format!(
+            "ctypes.CFUNCTYPE({}, {}, ctypes.POINTER(_UniffiRustCallStatus))",
+            match &ffi_func.return_type() {
+                Some(v) => PythonCodeOracle::ffi_type_label(v),
+                None => "None".to_owned(),
+            },
+            ffi_func
+                .arguments()
+                .into_iter()
+                .map(|a| PythonCodeOracle::ffi_type_label(&a.type_()))
+                .collect::<Vec<_>>()
+                .join(", ")
+        ))
+    }
+
     /// Get the Python syntax for representing a given low-level `FfiType`.
     pub fn ffi_type_name(type_: &FfiType) -> Result<String, askama::Error> {
         Ok(PythonCodeOracle::ffi_type_label(type_))

uniffi_bindgen/src/bindings/python/templates/Helpers.py

@@ -16,18 +16,22 @@ class _UniffiRustCallStatus(ctypes.Structure):
     # These match the values from the uniffi::rustcalls module
     CALL_SUCCESS = 0
     CALL_ERROR = 1
-    CALL_PANIC = 2
+    CALL_UNEXPECTED_ERROR = 2
 
     def __str__(self):
         if self.code == _UniffiRustCallStatus.CALL_SUCCESS:
             return "_UniffiRustCallStatus(CALL_SUCCESS)"
         elif self.code == _UniffiRustCallStatus.CALL_ERROR:
             return "_UniffiRustCallStatus(CALL_ERROR)"
-        elif self.code == _UniffiRustCallStatus.CALL_PANIC:
-            return "_UniffiRustCallStatus(CALL_PANIC)"
+        elif self.code == _UniffiRustCallStatus.CALL_UNEXPECTED_ERROR:
+            return "_UniffiRustCallStatus(CALL_UNEXPECTED_ERROR)"
         else:
             return "_UniffiRustCallStatus(<invalid code>)"
 
+    @classmethod
+    def default(cls):
+        cls(_UniffiRustCallStatus.CALL_SUCCESS, _UniffiRustBuffer.default())
+
 def _rust_call(fn, *args):
     # Call a rust function
     return _rust_call_with_error(None, fn, *args)
@@ -53,7 +57,7 @@ def _uniffi_check_call_status(error_ffi_converter, call_status):
             raise InternalError("_rust_call_with_error: CALL_ERROR, but error_ffi_converter is None")
         else:
             raise error_ffi_converter.lift(call_status.error_buf)
-    elif call_status.code == _UniffiRustCallStatus.CALL_PANIC:
+    elif call_status.code == _UniffiRustCallStatus.CALL_UNEXPECTED_ERROR:
         # When the rust code sees a panic, it tries to construct a _UniffiRustBuffer
         # with the message.  But if that code panics, then it just sends back
         # an empty buffer.
@@ -66,6 +70,30 @@ def _uniffi_check_call_status(error_ffi_converter, call_status):
         raise InternalError("Invalid _UniffiRustCallStatus code: {}".format(
             call_status.code))
 
+def _uniffi_trait_interface_call(call_status, make_call, lower_fn, default_return):
+    try:
+        return lower_fn(make_call())
+    except Exception as e:
+        import traceback
+        print(f"Unexpected error in _uniffi_trait_interface_call: {e}")
+        call_status.code = _UniffiRustCallStatus.CALL_UNEXPECTED_ERROR
+        call_status.error_buf = {{ Type::String.borrow()|lower_fn }}(str(e))
+        return default_return
+
+def _uniffi_trait_interface_call_with_error(call_status, make_call, lower_fn, default_return, error_type, lower_error):
+    try:
+        try:
+            return lower_fn(make_call())
+        except error_type as e:
+            call_status.code = _UniffiRustCallStatus.CALL_ERROR
+            call_status.error_buf = lower_error(e)
+            return default_return
+    except Exception as e:
+        print(f"Unexpected error in _uniffi_trait_interface_call_with_error: {e}")
+        call_status.code = _UniffiRustCallStatus.CALL_UNEXPECTED_ERROR
+        call_status.error_buf = {{ Type::String.borrow()|lower_fn }}(str(e))
+        return default_return
+
 # A function pointer for a callback as defined by UniFFI.
 # Rust definition `fn(handle: u64, method: u32, args: _UniffiRustBuffer, buf_ptr: *mut _UniffiRustBuffer) -> int`
 _UNIFFI_FOREIGN_CALLBACK_T = ctypes.CFUNCTYPE(ctypes.c_int, ctypes.c_ulonglong, ctypes.c_ulong, ctypes.POINTER(ctypes.c_char), ctypes.c_int, ctypes.POINTER(_UniffiRustBuffer))

uniffi_bindgen/src/bindings/python/templates/ObjectTemplate.py

@@ -63,7 +63,8 @@ def __ne__(self, other: object) -> {{ ne.return_type().unwrap()|type_name }}:
 {%      endmatch %}
 {% endfor %}
 
-
+{%- match imp %}
+{%- when ObjectImpl::Struct %}
 class {{ ffi_converter_name }}:
     @classmethod
     def read(cls, buf):
@@ -72,16 +73,46 @@ def read(cls, buf):
             raise InternalError("Raw pointer value was null")
         return cls.lift(ptr)
 
+    @staticmethod
+    def lift(value):
+        return {{ type_name }}._make_instance_(value)
+
     @classmethod
     def write(cls, value, buf):
         if not isinstance(value, {{ type_name }}):
             raise TypeError("Expected {{ type_name }} instance, {} found".format(type(value).__name__))
         buf.write_u64(cls.lower(value))
 
+    @staticmethod
+    def lower(value):
+        return value._pointer
+{%- when ObjectImpl::Trait %}
+{%- let trait_impl="UniffiTraitImpl{name}"|format %}
+{%- include "TraitObjectImpl.py" %}
+class {{ ffi_converter_name }}:
+    # Lift and read Rust-backed trait objects
     @staticmethod
     def lift(value):
-        return {{ type_name }}._make_instance_(value)
+        obj = {{ type_name }}._make_instance_(value)
+        print(f"lift: {obj} {value:x}", flush=False)
+        return obj
+
+    @classmethod
+    def read(cls, buf):
+        ptr = buf.read_u64()
+        if ptr == 0:
+            raise InternalError("Raw pointer value was null")
+        return cls.lift(ptr)
 
+    # Lower and write Python-backed trait objects
     @staticmethod
     def lower(value):
-        return value._pointer
+        ptr = {{ trait_impl }}._UniffiPointerManager.new_pointer(value)
+        print(f"lower: {value} {ptr:x}", flush=False)
+        return ptr
+
+    @classmethod
+    def write(cls, value, buf):
+        buf.write_u64(cls.lower(value))
+
+{%- endmatch %}

uniffi_bindgen/src/bindings/python/templates/PointerManager.py

@@ -44,7 +44,7 @@ class _UniffiPointerManagerGeneral:
     def __init__(self):
         self._map = {}
         self._lock = threading.Lock()
-        self._current_handle = 0
+        self._current_handle = 1
 
     def new_pointer(self, obj):
         with self._lock:
@@ -62,7 +62,10 @@ def lookup(self, handle):
             return self._map[handle]
 
 # Pick an pointer manager implementation based on the platform
-if platform.python_implementation() == 'CPython':
-    _UniffiPointerManager = _UniffiPointerManagerCPython # type: ignore
-else:
-    _UniffiPointerManager = _UniffiPointerManagerGeneral # type: ignore
+# Unfortunately, the _UniffiPointerManagerCPython is not compatible with the rubicon hack, so we
+# always use the slower _UniffiPointerManagerGeneral
+# if platform.python_implementation() == 'CPython':
+#     _UniffiPointerManager = _UniffiPointerManagerCPython # type: ignore
+# else:
+#     _UniffiPointerManager = _UniffiPointerManagerGeneral # type: ignore
+_UniffiPointerManager = _UniffiPointerManagerGeneral # type: ignore