Updated lower validation for Python/Ruby

The dynamic languages performed various checks in lower(): type checks, bounds
checks, etc.  This presented an issue when we wanted to call `inc_ref`
for object types.  If we did this, then another value failed its check,
we would leak the reference.

Prevent this by adding a `check()` method.  We do the check first then
once we get to `lower()`, nothing can fail.

Author: bendk

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;
 }
@@ -39,6 +40,7 @@ fn get_sim_cards() -> Vec<Arc<dyn SimCard>> {
 }
 
 // The call-answer, callback interface.
+#[uniffi::trait_interface]
 pub trait CallAnswerer {
     fn answer(&self) -> Result<String, TelephoneError>;
 }

examples/sprites/tests/bindings/test_sprites.kts

@@ -16,7 +16,7 @@ s.destroy()
 try {
     s.moveBy(Vector(0.0, 0.0))
     assert(false) { "Should not be able to call anything after `destroy`" }
-} catch(e: IllegalStateException) {
+} catch(e: InternalException) {
     assert(true)
 }
 

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/tests/bindings/test_coverall.py

@@ -242,16 +242,13 @@ def test_return_objects(self):
         coveralls = None
         self.assertEqual(get_num_alive(), 0)
 
-    # FIXME: since we're now inc-refing the handle, Python will leak objects if another lower fails.
-    # For now, let's disable this test.
-    #
-    # def test_bad_objects(self):
-    #     coveralls = Coveralls("test_bad_objects")
-    #     patch = Patch(Color.RED)
-    #     # `coveralls.take_other` wants `Coveralls` not `Patch`
-    #     with self.assertRaisesRegex(TypeError, "Coveralls.*Patch"):
-    #         coveralls.take_other(patch)
-    #
+    def test_bad_objects(self):
+        coveralls = Coveralls("test_bad_objects")
+        patch = Patch(Color.RED)
+        # `coveralls.take_other` wants `Coveralls` not `Patch`
+        with self.assertRaisesRegex(TypeError, "Coveralls.*Patch"):
+            coveralls.take_other(patch)
+
     def test_dict_with_defaults(self):
         """ This does not call Rust code. """
 

fixtures/coverall/tests/bindings/test_coverall.rb

@@ -244,17 +244,14 @@ def test_return_objects
     assert_equal Coverall.get_num_alive(), 0
   end
 
-  # FIXME: since we're now inc-refing the handle, Ruby will leak objects if another lower fails.
-  # For now, let's disable this test.
-  #
-  # def test_bad_objects
-  #   coveralls = Coverall::Coveralls.new "test_bad_objects"
-  #   patch = Coverall::Patch.new Coverall::Color::RED
-  #   # `coveralls.take_other` wants `Coveralls` not `Patch`
-  #   assert_raise_message /Expected a Coveralls instance, got.*Patch/ do
-  #     coveralls.take_other patch
-  #   end
-  # end
+  def test_bad_objects
+    coveralls = Coverall::Coveralls.new "test_bad_objects"
+    patch = Coverall::Patch.new Coverall::Color::RED
+    # `coveralls.take_other` wants `Coveralls` not `Patch`
+    assert_raise_message /Expected a Coveralls instance, got.*Patch/ do
+      coveralls.take_other patch
+    end
+  end
 
   def test_bytes
     coveralls = Coverall::Coveralls.new "test_bytes"

fixtures/reexport-scaffolding-macro/src/lib.rs

@@ -6,9 +6,10 @@ mod tests {
     use cargo_metadata::Message;
     use libloading::{Library, Symbol};
     use std::ffi::CString;
-    use std::os::raw::c_void;
     use std::process::{Command, Stdio};
-    use uniffi::{FfiConverter, ForeignCallback, RustBuffer, RustCallStatus, RustCallStatusCode};
+    use uniffi::{
+        FfiConverter, ForeignCallback, Handle, RustBuffer, RustCallStatus, RustCallStatusCode,
+    };
     use uniffi_bindgen::ComponentInterface;
 
     struct UniFfiTag;
@@ -149,34 +150,38 @@ mod tests {
                 .ffi_func()
                 .name(),
         );
-        let coveralls_new: Symbol<
-            unsafe extern "C" fn(RustBuffer, &mut RustCallStatus) -> *const c_void,
-        > = get_symbol(
-            &library,
-            object_def.primary_constructor().unwrap().ffi_func().name(),
-        );
+        let coveralls_new: Symbol<unsafe extern "C" fn(RustBuffer, &mut RustCallStatus) -> Handle> =
+            get_symbol(
+                &library,
+                object_def.primary_constructor().unwrap().ffi_func().name(),
+            );
         let coveralls_get_name: Symbol<
-            unsafe extern "C" fn(*const c_void, &mut RustCallStatus) -> RustBuffer,
+            unsafe extern "C" fn(Handle, &mut RustCallStatus) -> RustBuffer,
         > = get_symbol(
             &library,
             object_def.get_method("get_name").ffi_func().name(),
         );
-        let coveralls_free: Symbol<unsafe extern "C" fn(*const c_void, &mut RustCallStatus) -> ()> =
+        let coveralls_inc_ref: Symbol<unsafe extern "C" fn(Handle, &mut RustCallStatus) -> ()> =
+            get_symbol(&library, object_def.ffi_object_inc_ref().name());
+        let coveralls_free: Symbol<unsafe extern "C" fn(Handle, &mut RustCallStatus) -> ()> =
             get_symbol(&library, object_def.ffi_object_free().name());
 
         let num_alive = unsafe { get_num_alive(&mut call_status) };
         assert_eq!(call_status.code, RustCallStatusCode::Success);
         assert_eq!(num_alive, 0);
 
-        let obj_id = unsafe {
+        let obj_handle = unsafe {
             coveralls_new(
                 <String as FfiConverter<UniFfiTag>>::lower("TestName".into()),
                 &mut call_status,
             )
         };
         assert_eq!(call_status.code, RustCallStatusCode::Success);
 
-        let name_buf = unsafe { coveralls_get_name(obj_id, &mut call_status) };
+        unsafe { coveralls_inc_ref(obj_handle, &mut call_status) };
+        assert_eq!(call_status.code, RustCallStatusCode::Success);
+
+        let name_buf = unsafe { coveralls_get_name(obj_handle, &mut call_status) };
         assert_eq!(call_status.code, RustCallStatusCode::Success);
         assert_eq!(
             <String as FfiConverter<UniFfiTag>>::try_lift(name_buf).unwrap(),
@@ -187,7 +192,7 @@ mod tests {
         assert_eq!(call_status.code, RustCallStatusCode::Success);
         assert_eq!(num_alive, 1);
 
-        unsafe { coveralls_free(obj_id, &mut call_status) };
+        unsafe { coveralls_free(obj_handle, &mut call_status) };
         assert_eq!(call_status.code, RustCallStatusCode::Success);
 
         let num_alive = unsafe { get_num_alive(&mut call_status) };

fixtures/uitests/tests/ui/interface_trait_not_sync_and_send.rs

@@ -4,6 +4,7 @@ uniffi_macros::generate_and_include_scaffolding!("../../../../fixtures/uitests/s
 fn main() { /* empty main required by `trybuild` */}
 
 // This will fail to compile, because the trait is not explicit Send+Sync
+#[uniffi::trait_interface]
 pub trait Trait {
 }
 

fixtures/uitests/tests/ui/interface_trait_not_sync_and_send.stderr

@@ -27,9 +27,9 @@ note: required by a bound in `FfiConverterArc`
   = note: this error originates in the attribute macro `::uniffi::export_for_udl` (in Nightly builds, run with -Z macro-backtrace for more info)
 
 error[E0277]: `(dyn ProcMacroTrait + 'static)` cannot be shared between threads safely
-  --> tests/ui/interface_trait_not_sync_and_send.rs:11:1
+  --> tests/ui/interface_trait_not_sync_and_send.rs:12:1
    |
-11 | #[uniffi::export]
+12 | #[uniffi::export]
    | ^^^^^^^^^^^^^^^^^ `(dyn ProcMacroTrait + 'static)` cannot be shared between threads safely
    |
    = help: the trait `Sync` is not implemented for `(dyn ProcMacroTrait + 'static)`
@@ -41,9 +41,9 @@ note: required by a bound in `FfiConverterArc`
    = note: this error originates in the attribute macro `uniffi::export` (in Nightly builds, run with -Z macro-backtrace for more info)
 
 error[E0277]: `(dyn ProcMacroTrait + 'static)` cannot be sent between threads safely
-  --> tests/ui/interface_trait_not_sync_and_send.rs:11:1
+  --> tests/ui/interface_trait_not_sync_and_send.rs:12:1
    |
-11 | #[uniffi::export]
+12 | #[uniffi::export]
    | ^^^^^^^^^^^^^^^^^ `(dyn ProcMacroTrait + 'static)` cannot be sent between threads safely
    |
    = help: the trait `Send` is not implemented for `(dyn ProcMacroTrait + 'static)`

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

@@ -413,6 +413,10 @@ pub mod filters {
         Ok(format!("{}.lift", ffi_converter_name(as_ct)?))
     }
 
+    pub fn check_fn(as_ct: &impl AsCodeType) -> Result<String, askama::Error> {
+        Ok(format!("{}.check", ffi_converter_name(as_ct)?))
+    }
+
     pub fn lower_fn(as_ct: &impl AsCodeType) -> Result<String, askama::Error> {
         Ok(format!("{}.lower", ffi_converter_name(as_ct)?))
     }

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

@@ -1,16 +1,20 @@
-class _UniffiConverterBool(_UniffiConverterPrimitive):
+class _UniffiConverterBool:
     @classmethod
     def check(cls, value):
-        return not not value
+        pass
+
+    @classmethod
+    def lower(cls, value):
+        return 1 if value else 0
+
+    @staticmethod
+    def lift(value):
+        return value != 0
 
     @classmethod
     def read(cls, buf):
         return cls.lift(buf.read_u8())
 
     @classmethod
-    def write_unchecked(cls, value, buf):
+    def write(cls, value, buf):
         buf.write_u8(value)
-
-    @staticmethod
-    def lift(value):
-        return value != 0

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

@@ -7,10 +7,13 @@ def read(buf):
         return buf.read(size)
 
     @staticmethod
-    def write(value, buf):
+    def check(value):
         try:
             memoryview(value)
         except TypeError:
             raise TypeError("a bytes-like object is required, not {!r}".format(type(value).__name__))
+
+    @staticmethod
+    def write(value, buf):
         buf.write_i32(len(value))
         buf.write(value)

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

@@ -24,6 +24,10 @@ def read(cls, buf):
         handle = buf.read_u64()
         cls.lift(handle)
 
+    @classmethod
+    def check(cls, cb):
+        pass
+
     @classmethod
     def lower(cls, cb):
         handle = cls._slab.insert(cb)

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

@@ -17,6 +17,10 @@ def read(buf):
     def lift(value):
         return {{ builtin|ffi_converter_name }}.lift(value)
 
+    @staticmethod
+    def check(value):
+        return {{ builtin|ffi_converter_name }}.check(value)
+
     @staticmethod
     def lower(value):
         return {{ builtin|ffi_converter_name }}.lower(value)
@@ -51,6 +55,11 @@ def lift(value):
         builtin_value = {{ builtin|lift_fn }}(value)
         return {{ config.into_custom.render("builtin_value") }}
 
+    @staticmethod
+    def check(value):
+        builtin_value = {{ config.from_custom.render("value") }}
+        return {{ builtin|check_fn }}(builtin_value)
+
     @staticmethod
     def lower(value):
         builtin_value = {{ config.from_custom.render("value") }}

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

@@ -12,10 +12,14 @@ def read(buf):
         return datetime.timedelta(seconds=seconds, microseconds=microseconds)
 
     @staticmethod
-    def write(value, buf):
+    def check(value):
         seconds = value.seconds + value.days * 24 * 3600
-        nanoseconds = value.microseconds * 1000
         if seconds < 0:
             raise ValueError("Invalid duration, must be non-negative")
+
+    @staticmethod
+    def write(value, buf):
+        seconds = value.seconds + value.days * 24 * 3600
+        nanoseconds = value.microseconds * 1000
         buf.write_i64(seconds)
         buf.write_u32(nanoseconds)

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

@@ -81,6 +81,23 @@ def read(buf):
         {%- endfor %}
         raise InternalError("Raw enum value doesn't match any cases")
 
+    def check(value):
+        {%- if e.variants().is_empty() %}
+        pass
+        {%- else %}
+        {%- for variant in e.variants() %}
+        {%- if e.is_flat() %}
+        if value == {{ type_name }}.{{ variant.name()|enum_variant_py }}:
+        {%- else %}
+        if value.is_{{ variant.name()|var_name }}():
+        {%- endif %}
+            {%- for field in variant.fields() %}
+            {{ field|check_fn }}(value.{{ field.name()|var_name }})
+            {%- endfor %}
+            return
+        {%- endfor %}
+        {%- endif %}
+
     def write(value, buf):
         {%- for variant in e.variants() %}
         {%- if e.is_flat() %}

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

@@ -59,6 +59,20 @@ def read(buf):
         {%- endfor %}
         raise InternalError("Raw enum value doesn't match any cases")
 
+    @staticmethod
+    def check(value):
+        {%- if e.variants().is_empty() %}
+        pass
+        {%- else %}
+        {%- for variant in e.variants() %}
+        if isinstance(value, {{ type_name }}.{{ variant.name()|class_name }}):
+            {%- for field in variant.fields() %}
+            {{ field|check_fn }}(value.{{ field.name()|var_name }})
+            {%- endfor %}
+            return
+        {%- endfor %}
+        {%- endif %}
+
     @staticmethod
     def write(value, buf):
         {%- for variant in e.variants() %}

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

@@ -4,5 +4,5 @@ def read(buf):
         return buf.read_float()
 
     @staticmethod
-    def write_unchecked(value, buf):
+    def write(value, buf):
         buf.write_float(value)

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

@@ -4,5 +4,5 @@ def read(buf):
         return buf.read_double()
 
     @staticmethod
-    def write_unchecked(value, buf):
+    def write(value, buf):
         buf.write_double(value)

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

@@ -12,9 +12,12 @@ class {{ ffi_converter_name }}:
     _slab = UniffiSlab()
 
     @classmethod
-    def lower(cls, eventloop):
+    def check(cls, eventloop):
         if not isinstance(eventloop, asyncio.BaseEventLoop):
             raise TypeError("_uniffi_executor_callback: Expected EventLoop instance")
+
+    @classmethod
+    def lower(cls, eventloop):
         return cls._slab.insert(eventloop)
 
     @classmethod

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

@@ -8,5 +8,5 @@ def read(buf):
         return buf.read_i16()
 
     @staticmethod
-    def write_unchecked(value, buf):
+    def write(value, buf):
         buf.write_i16(value)

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

@@ -8,5 +8,5 @@ def read(buf):
         return buf.read_i32()
 
     @staticmethod
-    def write_unchecked(value, buf):
+    def write(value, buf):
         buf.write_i32(value)

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

@@ -8,5 +8,5 @@ def read(buf):
         return buf.read_i64()
 
     @staticmethod
-    def write_unchecked(value, buf):
+    def write(value, buf):
         buf.write_i64(value)

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

@@ -8,5 +8,5 @@ def read(buf):
         return buf.read_i8()
 
     @staticmethod
-    def write_unchecked(value, buf):
+    def write(value, buf):
         buf.write_i8(value)