From ecda34fcdc3cb20bef68f98d3913efdc9a3ae7b0 Mon Sep 17 00:00:00 2001 From: Mark Hammond Date: Tue, 30 Mar 2021 15:21:38 +1100 Subject: [PATCH] Allow passing object instances as args, fields and return values. This is a significant re-working of how object instances are handled, replacing our previous use of handlemaps with direct use of an `Arc` pointer. This both reduces overhead and allows us to must more easily generate code for dealing with object references. On the Rust side of the world, code that needs to deal with an object reference will typically take an `Arc`, in the spirit of being as explicit as possible. The one exception is when passing objects as arguments, where (like other types) you can annotate the argument with `[ByRef]` and have UniFFI hand you an `&T`. For an example of how this might be used in practice, the "todolist" example has grown the ability to manage a global default list by setting and retreiving a shared object reference. Co-authored-by: Ryan Kelly --- CHANGELOG.md | 10 + .../src/internals/lifting_and_lowering.md | 4 +- .../manual/src/internals/object_references.md | 124 ++++-- docs/manual/src/udl/functions.md | 1 + docs/manual/src/udl/interfaces.md | 102 ++++- docs/manual/src/udl/structs.md | 43 ++- examples/todolist/Cargo.toml | 1 + examples/todolist/src/lib.rs | 30 +- examples/todolist/src/todolist.udl | 5 + .../todolist/tests/bindings/test_todolist.kts | 30 +- .../todolist/tests/bindings/test_todolist.py | 19 + .../todolist/tests/bindings/test_todolist.rb | 17 + .../tests/bindings/test_todolist.swift | 20 +- fixtures/coverall/src/coverall.udl | 35 ++ fixtures/coverall/src/lib.rs | 66 +++- .../coverall/tests/bindings/test_coverall.kts | 109 ++++-- .../coverall/tests/bindings/test_coverall.py | 64 ++- .../coverall/tests/bindings/test_coverall.rb | 92 ++++- .../tests/bindings/test_coverall.swift | 99 +++++ .../coverall/tests/test_generated_bindings.rs | 1 + uniffi/Cargo.toml | 2 +- uniffi/src/ffi/handle_maps.rs | 363 ------------------ uniffi/src/ffi/mod.rs | 2 - uniffi/src/lib.rs | 62 ++- .../src/bindings/gecko_js/gen_gecko_js.rs | 1 + .../src/bindings/kotlin/gen_kotlin.rs | 1 + .../bindings/kotlin/templates/EnumTemplate.kt | 21 +- .../src/bindings/kotlin/templates/Helpers.kt | 73 ++-- .../kotlin/templates/ObjectTemplate.kt | 36 +- .../kotlin/templates/RecordTemplate.kt | 16 +- .../src/bindings/python/gen_python.rs | 7 +- .../python/templates/ObjectTemplate.py | 36 +- .../python/templates/RustBufferBuilder.py | 10 +- .../python/templates/RustBufferStream.py | 6 +- uniffi_bindgen/src/bindings/ruby/gen_ruby.rs | 5 +- .../bindings/ruby/templates/ObjectTemplate.rb | 53 ++- .../ruby/templates/RustBufferBuilder.rb | 6 +- .../ruby/templates/RustBufferStream.rb | 4 +- .../src/bindings/swift/gen_swift.rs | 1 + .../swift/templates/ErrorTemplate.swift | 2 + .../swift/templates/ObjectTemplate.swift | 64 ++- uniffi_bindgen/src/interface/attributes.rs | 10 +- uniffi_bindgen/src/interface/enum_.rs | 7 + uniffi_bindgen/src/interface/ffi.rs | 4 + uniffi_bindgen/src/interface/function.rs | 13 +- uniffi_bindgen/src/interface/mod.rs | 33 ++ uniffi_bindgen/src/interface/object.rs | 36 +- uniffi_bindgen/src/interface/record.rs | 7 + uniffi_bindgen/src/interface/types/mod.rs | 4 +- uniffi_bindgen/src/scaffolding.rs | 6 +- .../src/templates/ObjectTemplate.rs | 37 +- uniffi_bindgen/src/templates/macros.rs | 40 +- 52 files changed, 1232 insertions(+), 608 deletions(-) create mode 100644 fixtures/coverall/tests/bindings/test_coverall.swift delete mode 100644 uniffi/src/ffi/handle_maps.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index d6e11db7d4..d19d4bc30b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,16 @@ will be removed in a future release. More details on the motivation for this change can be found in [ADR-0004](https://github.com/mozilla/uniffi-rs/blob/main/docs/adr/0004-only-threadsafe-interfaces.md). +### What's New + +- It is now possible to use Object instances as fields in Records or Enums, to pass them as arguments, + and to return them from function and method calls. They should for the most part behave just like + a host language object, and their lifecycle is managed transparently using Rust's `Arc` type. + - Reference cycles that include Rust objects will not be garbage collected; if you cannot avoid + creating reference cycles you may need to use Rust's `Weak` type to help break them. + - In the **Kotlin** bindings, Object instances must be manually freed by calling their `destroy()` + method or by using their `.use` block helper method. Records or Enums that *contain* an Object + instance now also have a `destroy()` method and must be similarly disposed of after use. ### What's Changed diff --git a/docs/manual/src/internals/lifting_and_lowering.md b/docs/manual/src/internals/lifting_and_lowering.md index 3f3e14cf18..0743fb0681 100644 --- a/docs/manual/src/internals/lifting_and_lowering.md +++ b/docs/manual/src/internals/lifting_and_lowering.md @@ -66,7 +66,7 @@ Calling this function from foreign language code involves the following steps: | `record` | `RustBuffer` struct pointing to serialized bytes | | `enum` and `[Enum] interface` | `RustBuffer` struct pointing to serialized bytes | | `dictionary` | `RustBuffer` struct pointing to serialized bytes | -| `interface` | `uint64_t` opaque integer handle | +| `interface` | `void*` opaque pointer to object on the heap | ## Serialization Format @@ -88,7 +88,7 @@ The details of this format are internal only and may change between versions of | `record` | Serialized `i32` item count followed by serialized items; each item is a serialized `string` followed by a serialized `T` | | `enum` and `[Enum] interface` | Serialized `i32` indicating variant, numbered in declaration order starting from 1, followed by the serialized values of the variant's fields in declaration order | | `dictionary` | The serialized value of each field, in declaration order | -| `interface` | *Cannot currently be serialized* | +| `interface` | Fixed-width 8-byte unsigned integer encoding a pointer to the object on the heap | Note that length fields in this format are serialized as *signed* integers despite the fact that they will always be non-negative. This is to help diff --git a/docs/manual/src/internals/object_references.md b/docs/manual/src/internals/object_references.md index b9836dfd7c..6ceeff4379 100644 --- a/docs/manual/src/internals/object_references.md +++ b/docs/manual/src/internals/object_references.md @@ -1,7 +1,7 @@ # Managing Object References UniFFI [interfaces](../udl/interfaces.md) represent instances of objects -that have methods and contain shared mutable state. One of Rust's core innovations +that have methods and contain state. One of Rust's core innovations is its ability to provide compile-time guarantees about working with such instances, including: @@ -16,6 +16,8 @@ system. UniFFI itself tries to take a hands-off approach as much as possible and depends on the Rust compiler itself to uphold safety guarantees, without assuming that foreign-language callers will be "well behaved". +## Concurrency + UniFFI's hands-off approach means that all object instances exposed by UniFFI must be safe to access concurrently. In Rust terminology, they must be `Send+Sync` and must be useable without taking any `&mut` references. @@ -27,18 +29,22 @@ of the component - as much as possible, UniFFI tries to stay out of your way, si that the object implementation is `Send+Sync` and letting the Rust compiler ensure that this is so. -## Handle Maps +## Lifetimes + +In order to allow for instances to be used as flexibly as possible from foreign-language code, +UniFFI wraps all object instances in an `Arc` and leverages their reference-count based lifetimes, +allowing UniFFI to largely stay out of handling lifetimes entirely for these objects. -For additional typechecking safety, UniFFI indirects all object access through a -"handle map", a mapping from opaque integer handles to object instances. This indirection -imposes a small runtime cost but helps us guard against errors or oversights -in the generated bindings. +When constructing a new object, UniFFI is able to add the `Arc` automatically, because it +knows that the return type of the Rust constructor must be a new uniquely-owned struct of +the corresponding type. -For each interface declared in the UDL, the UniFFI-generated Rust scaffolding -will create a global handlemap that is responsible for owning all instances -of that interface, and handing out references to them when methods are called. -The handlemap requires that its contents be `Send+Sync`, helping enforce requirements -around thread-safety. +When you want to return object instances from functions or methods, or store object instances +as fields in records, the underlying Rust code will need to work with `Arc` directly, to ensure +that the code behaves in the way that UniFFI expects. + +When accepting instances as arguments, the underlying Rust code can choose to accept it as an `Arc` +or as the underlying struct `T`, as there are different use-cases for each scenario. For example, given a interface definition like this: @@ -50,53 +56,91 @@ interface TodoList { }; ``` -The Rust scaffolding would define a lazyily-initialized global static like: +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 -lazy_static! { - static ref UNIFFI_HANDLE_MAP_TODOLIST: ArcHandleMap = ArcHandleMap::new(); +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); + as uniffi::ViaFfi>::lower(_arc) + }) } ``` -On the Rust side of the generated bindings, the instance constructor will create an instance of the -corresponding `TodoList` Rust struct, insert it into the handlemap, and return the resulting integer -handle to the foreign language code: +The UniFFI runtime implements lowering for object instances using `Arc::into_raw`: ```rust -pub extern "C" fn todolist_TodoList_new(err: &mut ExternError) -> u64 { - // Give ownership of the new instance to the handlemap. - // We will only ever operate on borrowed references to it. - UNIFFI_HANDLE_MAP_TODOLIST.insert_with_output(err, || TodoList::new()) +unsafe impl ViaFfi for std::sync::Arc { + type FfiType = *const std::os::raw::c_void; + fn lower(self) -> Self::FfiType { + std::sync::Arc::into_raw(self) as Self::FfiType + } } ``` -When invoking a method on the instance, the foreign-language code passes the integer handle back -to the Rust code, which borrows a reference to the instance from the handlemap for the duration -of the method call: +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_TodoList_add_item(handle: u64, todo: RustBuffer, err: &mut ExternError) -> () { - let todo = ::try_lift(todo).unwrap() - // Borrow a reference to the instance so that we can call a method on it. - UNIFFI_HANDLE_MAP_TODOLIST.call_with_result_mut(err, handle, |obj| -> Result<(), TodoError> { - TodoList::add_item(obj, todo) +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( + & as uniffi::ViaFfi>::try_lift(ptr).unwrap(), + ::try_lift(todo).unwrap())?, + ) + Ok(_retval) }) } ``` -Finally, when the foreign-language code frees the instance, it passes the integer handle to -a special destructor function so that the Rust code can delete it from the handlemap: +The UniFFI runtime implements lifting for object instances using `Arc::from_raw`: ```rust -pub extern "C" fn ffi_todolist_TodoList_object_free(handle: u64) { - UNIFFI_HANDLE_MAP_TODOLIST.delete_u64(handle); -} +unsafe impl ViaFfi for std::sync::Arc { + type FfiType = *const std::os::raw::c_void; + fn try_lift(v: Self::FfiType) -> Result { + let v = v as *const T; + // We musn't drop the `Arc` 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)) + } ``` -This indirection gives us some important safety properties: +Notice that we take care to ensure the reference that is owned by the foreign-language +code remains alive. + +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); + } +} +``` -* If the generated bindings incorrectly pass an invalid handle, or a handle for a different type of object, - then the handlemap will throw an error with high probability, providing some amount of run-time typechecking - for correctness of the generated bindings. -* The handlemap can ensure we uphold Rust's requirements around unique mutable references and threadsafey, - by specifying that the contained type must be `Send+Sync`, and by refusing to hand out any mutable references. +Passing instances as arguments and returning them as values works similarly, except that +UniFFI does not automatically wrap/unwrap the containing `Arc`. diff --git a/docs/manual/src/udl/functions.md b/docs/manual/src/udl/functions.md index 9766b1875a..f86273e6d0 100644 --- a/docs/manual/src/udl/functions.md +++ b/docs/manual/src/udl/functions.md @@ -15,3 +15,4 @@ The UDL file will look like: namespace Example { string hello_world(); } +``` \ No newline at end of file diff --git a/docs/manual/src/udl/interfaces.md b/docs/manual/src/udl/interfaces.md index cc54a77f40..e654e024d9 100644 --- a/docs/manual/src/udl/interfaces.md +++ b/docs/manual/src/udl/interfaces.md @@ -93,6 +93,104 @@ For each alternate constructor, UniFFI will expose an appropriate static-method, in the foreign language binding, and will connect it to the Rust method of the same name on the underlying Rust struct. +## Managing Shared References + +To the foreign-language consumer, UniFFI object instances are designed to behave as much like +regular language objects as possible. They can be freely passed as arguments or returned as values, +like this: + +```idl +interface TodoList { + ... + + // Copy the items from another TodoList into this one. + void import_items(TodoList other); + + // Make a copy of this TodoList as a new instance. + TodoList duplicate(); +}; +``` + +To ensure that this is safe, UniFFI allocates every object instance on the heap using +[`Arc`](https://doc.rust-lang.org/std/sync/struct.Arc.html), Rust's built-in smart pointer +type for managing shared references at runtime. + +The use of `Arc` is transparent to the foreign-language code, but sometimes shows up +in the function signatures of the underlying Rust code. For example, the Rust code implementing +the `TodoList::duplicate` method would need to explicitly return an `Arc`, since UniFFI +doesn't know whether it will be returning a new object or an existing one: + +```rust +impl TodoList { + fn duplicate(&self) -> Arc { + Arc::new(TodoList { + items: RwLock::new(self.items.read().unwrap().clone()) + }) + } +} +``` + +By default, object instances passed as function arguments will also be passed as an `Arc`, so the +Rust implementation of `TodoList::import_items` would also need to accept an `Arc`: + +```rust +impl TodoList { + fn import_items(&self, other: Arc) { + self.items.write().unwrap().append(other.get_items()); + } +} +``` + +If the Rust code does not need an owned reference to the `Arc`, you can use the `[ByRef]` UDL attribute +to signal that a function accepts a borrowed reference: + +```idl +interface TodoList { + ... + // +-- indicate that we only need to borrow the other list + // V + void import_items([ByRef] TodoList other); + ... +}; +``` + +```rust +impl TodoList { + // +-- don't need to care about the `Arc` here + // V + fn import_items(&self, other: &TodoList) { + self.items.write().unwrap().append(other.get_items()); + } +} +``` + +Conversely, if the Rust code explicitly *wants* to deal with an `Arc` in the special case of +the `self` parameter, it can signal this using the `[Self=ByArc]` UDL attribute on the method: + + +```idl +interface TodoList { + ... + // +-- indicate that we want the `Arc` containing `self` + // V + [Self=ByArc] + void import_items(TodoList other); + ... +}; +``` + +```rust +impl TodoList { + // `Arc`s everywhere! --+-----------------+ + // V V + fn import_items(self: Arc, other: Arc) { + self.items.write().unwrap().append(other.get_items()); + } +} +``` + +You can read more about the technical details in the docs on the +[internal details of managing object references](../internals/object_references.md). ## Concurrent Access @@ -132,7 +230,7 @@ impl Counter { Self { value: 0 } } - // No mutable references to self allowed in in UniFFI interfaces. + // No mutable references to self allowed in UniFFI interfaces. fn increment(&mut self) { self.value = self.value + 1; } @@ -194,4 +292,4 @@ impl Counter { ``` You can read more about the technical details in the docs on the -[internal details of managing object references](../internals/object_references.md). \ No newline at end of file +[internal details of managing object references](../internals/object_references.md). diff --git a/docs/manual/src/udl/structs.md b/docs/manual/src/udl/structs.md index 7dd583bb1a..5bfaa4f2f2 100644 --- a/docs/manual/src/udl/structs.md +++ b/docs/manual/src/udl/structs.md @@ -2,6 +2,8 @@ Dictionaries can be compared to POJOs in the Java world: just a data structure holding some data. +A Rust struct like this: + ```rust struct TodoEntry { done: bool, @@ -10,7 +12,7 @@ struct TodoEntry { } ``` -can be converted in UDL to: +Can be exposed via UniFFI using UDL like this: ```idl dictionary TodoEntry { @@ -18,7 +20,44 @@ dictionary TodoEntry { u64 due_date; string text; }; +``` + +The fields in a dictionary can be of almost any type, including objects or other dictionaries. +The current limitations are: + +* They cannot recursively contain another instance of the *same* dictionary. +* They cannot contain references to callback interfaces. + +## Fields holding Object References + +If a dictionary contains a field whose type is an [interface](./interfaces.md), then that +field will hold a *reference* to an underlying instance of a Rust struct. The Rust code for +working with such fields must store them as an `Arc` in order to help properly manage the +lifetime of the instance. So if the UDL interface looked like this: + +```idl +interface User { + // Some sort of "user" object that can own todo items +}; + +dictionary TodoEntry { + User owner; + string text; +} +``` + +Then the corresponding Rust code would need to look like this: +```rust +struct TodoEntry { + owner: std::sync::Arc, + text: String, +} ``` -Dictionaries can contain each other and every other data type available, except objects. +Depending on the languge, the foreign-language bindings may also need to be aware of +these embedded references. For example in Kotlin, each Object instance must be explicitly +destroyed to avoid leaking the underlying memory, and this also applies to Objects stored +in record fields. + +You can read more about managing object references in the section on [interfaces](./interfaces.md). diff --git a/examples/todolist/Cargo.toml b/examples/todolist/Cargo.toml index 73a52daf0f..ef58034a60 100644 --- a/examples/todolist/Cargo.toml +++ b/examples/todolist/Cargo.toml @@ -14,6 +14,7 @@ name = "uniffi_todolist" uniffi_macros = {path = "../../uniffi_macros"} uniffi = {path = "../../uniffi", features=["builtin-bindgen"]} thiserror = "1.0" +lazy_static = "1.4" [build-dependencies] uniffi_build = {path = "../../uniffi_build", features=["builtin-bindgen"]} diff --git a/examples/todolist/src/lib.rs b/examples/todolist/src/lib.rs index e36a440a19..254132310d 100644 --- a/examples/todolist/src/lib.rs +++ b/examples/todolist/src/lib.rs @@ -2,13 +2,21 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -use std::sync::RwLock; +use std::sync::{Arc, RwLock}; #[derive(Debug, Clone)] pub struct TodoEntry { text: String, } +lazy_static::lazy_static! { + // There is a single "default" TodoList that can be shared + // by all consumers of this component. Depending on requirements, + // a real app might like to use a `Weak<>` rather than an `Arc<>` + // here to reduce the rist of circular references. + static ref DEFAULT_LIST: RwLock>> = RwLock::new(None); +} + #[derive(Debug, thiserror::Error)] enum TodoError { #[error("The todo does not exist!")] @@ -23,6 +31,22 @@ enum TodoError { DeligatedError(#[from] std::io::Error), } +/// Get a reference to the global default TodoList, if set. +/// +fn get_default_list() -> Option> { + DEFAULT_LIST.read().unwrap().clone() +} + +/// Set the global default TodoList. +/// +/// This will silently drop any previously set value. +/// +fn set_default_list(list: Arc) { + *DEFAULT_LIST.write().unwrap() = Some(list); +} + +/// Create a new TodoEntry from the given string. +/// fn create_entry_with>(item: S) -> Result { let text = item.into(); if text.is_empty() { @@ -117,6 +141,10 @@ impl TodoList { items.remove(idx); Ok(()) } + + fn make_default(self: Arc) { + set_default_list(self); + } } include!(concat!(env!("OUT_DIR"), "/todolist.uniffi.rs")); diff --git a/examples/todolist/src/todolist.udl b/examples/todolist/src/todolist.udl index 5120fb70af..5c923314cd 100644 --- a/examples/todolist/src/todolist.udl +++ b/examples/todolist/src/todolist.udl @@ -1,4 +1,7 @@ namespace todolist { + TodoList? get_default_list(); + undefined set_default_list(TodoList list); + [Throws=TodoError] TodoEntry create_entry_with(string todo); }; @@ -30,4 +33,6 @@ interface TodoList { string get_first(); [Throws=TodoError] void clear_item(string todo); + [Self=ByArc] + undefined make_default(); }; diff --git a/examples/todolist/tests/bindings/test_todolist.kts b/examples/todolist/tests/bindings/test_todolist.kts index e419337ccd..1e2984138d 100644 --- a/examples/todolist/tests/bindings/test_todolist.kts +++ b/examples/todolist/tests/bindings/test_todolist.kts @@ -50,6 +50,34 @@ todo.addItems(listOf("bobo", "fofo")) assert(todo.getItems().size == 9) assert(todo.getItems()[7] == "bobo") +assert(getDefaultList() == null) + +// Note that each individual object instance needs to be explicitly destroyed, +// either by using the `.use` helper or explicitly calling its `.destroy` method. +// Failure to do so will leak the underlying Rust object. +TodoList().use { todo2 -> + setDefaultList(todo) + getDefaultList()!!.use { default -> + assert(todo.getEntries() == default.getEntries()) + assert(todo2.getEntries() != default.getEntries()) + } + + todo2.makeDefault() + getDefaultList()!!.use { default -> + assert(todo.getEntries() != default.getEntries()) + assert(todo2.getEntries() == default.getEntries()) + } + + todo.addItem("Test liveness after being demoted from default") + assert(todo.getLast() == "Test liveness after being demoted from default") + + todo2.addItem("Test shared state through local vs default reference") + getDefaultList()!!.use { default -> + assert(default.getLast() == "Test shared state through local vs default reference") + } +} + // Ensure the kotlin version of deinit doesn't crash, and is idempotent. todo.destroy() -todo.destroy() \ No newline at end of file +todo.destroy() + diff --git a/examples/todolist/tests/bindings/test_todolist.py b/examples/todolist/tests/bindings/test_todolist.py index 0e3b5b434b..017e999fb2 100644 --- a/examples/todolist/tests/bindings/test_todolist.py +++ b/examples/todolist/tests/bindings/test_todolist.py @@ -23,3 +23,22 @@ entry2 = TodoEntry("Test Ünicode hàndling in an entry can't believe I didn't test this at first 🤣") todo.add_entry(entry2) assert(todo.get_last_entry().text == "Test Ünicode hàndling in an entry can't believe I didn't test this at first 🤣") + +todo2 = TodoList() +assert(todo != todo2) +assert(todo is not todo2) + +assert(get_default_list() is None) + +set_default_list(todo) +assert(todo.get_items() == get_default_list().get_items()) + +todo2.make_default() +assert(todo2.get_items() == get_default_list().get_items()) + +todo.add_item("Test liveness after being demoted from default") +assert(todo.get_last() == "Test liveness after being demoted from default") + +todo2.add_item("Test shared state through local vs default reference") +assert(get_default_list().get_last() == "Test shared state through local vs default reference") + diff --git a/examples/todolist/tests/bindings/test_todolist.rb b/examples/todolist/tests/bindings/test_todolist.rb index 2bdfb42ceb..d9e04f92e7 100644 --- a/examples/todolist/tests/bindings/test_todolist.rb +++ b/examples/todolist/tests/bindings/test_todolist.rb @@ -28,3 +28,20 @@ entry2 = TodoEntry.new("Test Ünicode hàndling in an entry can't believe I didn't test this at first 🤣") todo.add_entry(entry2) assert_equal todo.get_last_entry.text, "Test Ünicode hàndling in an entry can't believe I didn't test this at first 🤣" + +todo2 = TodoList.new +assert todo2.get_items != todo.get_items + +assert Todolist.get_default_list == nil + +Todolist.set_default_list todo +assert todo.get_items == Todolist.get_default_list.get_items + +todo2.make_default +assert todo2.get_items == Todolist.get_default_list.get_items + +todo.add_item "Test liveness after being demoted from default" +assert todo.get_last == "Test liveness after being demoted from default" + +todo2.add_item "Test shared state through local vs default reference" +assert Todolist.get_default_list.get_last == "Test shared state through local vs default reference" \ No newline at end of file diff --git a/examples/todolist/tests/bindings/test_todolist.swift b/examples/todolist/tests/bindings/test_todolist.swift index 781c1c6ad0..e66b30b5e9 100644 --- a/examples/todolist/tests/bindings/test_todolist.swift +++ b/examples/todolist/tests/bindings/test_todolist.swift @@ -47,4 +47,22 @@ assert(todo.getItems()[7] == "bobo") for _ in 0..<10 { let list = TodoList() try! list.addItem(todo: "todo") -} \ No newline at end of file +} + +let todo2 = TodoList() +assert(todo != todo2) + +assert(getDefaultList() == nil) + +setDefaultList(list: todo) +assert(todo.getItems() == getDefaultList()!.getItems()) + +todo2.makeDefault() +assert(todo2.getItems() == getDefaultList()!.getItems()) + +try! todo.addItem(todo: "Test liveness after being demoted from default") +assert(try! todo.getLast() == "Test liveness after being demoted from default") + +try! todo2.addItem(todo: "Test shared state through local vs default reference") +assert(try! getDefaultList()!.getLast() == "Test shared state through local vs default reference") + diff --git a/fixtures/coverall/src/coverall.udl b/fixtures/coverall/src/coverall.udl index 49cfdaeddf..4f7cef4e60 100644 --- a/fixtures/coverall/src/coverall.udl +++ b/fixtures/coverall/src/coverall.udl @@ -22,6 +22,13 @@ dictionary SimpleDict { float? maybe_float32; double float64; double? maybe_float64; + Coveralls? coveralls; +}; + +[Enum] +interface MaybeSimpleDict { + Yeah(SimpleDict d); + Nah(); }; [Error] @@ -52,6 +59,34 @@ interface Coveralls { /// Calls `Arc::strong_count()` on the `Arc` containing `self`. [Self=ByArc] u64 strong_count(); + + /// Takes an `Arc` and stores it in `self`, dropping the existing + /// reference. Note you can create circular references by passing `self`. + void take_other(Coveralls? other); + + /// Returns what was previously set via `take_other()`, or null. + Coveralls? get_other(); + + /// Same signature as `take_other` but always fails. + [Self=ByArc, Throws=CoverallError] + void take_other_fallible(); + + /// Same signature as `take_other` but with an extra string arg - always + /// panics with that message.. + [Self=ByArc] + void take_other_panic(string message); + + // can't name it `clone` as it conflicts with the Clone trait and ours has a different signature + Coveralls clone_me(); +}; + +// All coveralls end up with a patch. +enum Color {"Red", "Blue", "Green"}; + +interface Patch { + constructor(Color color); + + Color get_color(); }; interface ThreadsafeCounter { diff --git a/fixtures/coverall/src/lib.rs b/fixtures/coverall/src/lib.rs index 2741ab8c06..a5989db55c 100644 --- a/fixtures/coverall/src/lib.rs +++ b/fixtures/coverall/src/lib.rs @@ -3,7 +3,7 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ use std::sync::atomic::{AtomicBool, AtomicI32, Ordering}; -use std::sync::{Arc, RwLock}; +use std::sync::{Arc, Mutex, RwLock}; lazy_static::lazy_static! { static ref NUM_ALIVE: RwLock = { @@ -35,6 +35,13 @@ pub struct SimpleDict { maybe_float32: Option, float64: f64, maybe_float64: Option, + coveralls: Option>, +} + +#[derive(Debug, Clone)] +pub enum MaybeSimpleDict { + Yeah { d: SimpleDict }, + Nah, } fn create_some_dict() -> SimpleDict { @@ -55,6 +62,7 @@ fn create_some_dict() -> SimpleDict { maybe_float32: Some(22.0 / 7.0), float64: 0.0, maybe_float64: Some(1.0), + coveralls: Some(Arc::new(Coveralls::new("some_dict".to_string()))), } } @@ -76,6 +84,7 @@ fn create_none_dict() -> SimpleDict { maybe_float32: None, float64: 0.0, maybe_float64: None, + coveralls: None, } } @@ -88,12 +97,18 @@ type Result = std::result::Result; #[derive(Debug)] pub struct Coveralls { name: String, + // A reference to another Coveralls. Currently will be only a reference + // to `self`, so will create a circular reference. + other: Mutex>>, } impl Coveralls { fn new(name: String) -> Self { *NUM_ALIVE.write().unwrap() += 1; - Self { name } + Self { + name, + other: Mutex::new(None), + } } fn fallible_new(name: String, should_fail: bool) -> Result { @@ -127,6 +142,32 @@ impl Coveralls { fn strong_count(self: Arc) -> u64 { Arc::strong_count(&self) as u64 } + + fn take_other(&self, other: Option>) { + *self.other.lock().unwrap() = other.map(|arc| Arc::clone(&arc)) + } + + fn get_other(&self) -> Option> { + (*self.other.lock().unwrap()).as_ref().map(|other| Arc::clone(other)) + } + + fn take_other_fallible(self: Arc) -> Result<()> { + Err(CoverallError::TooManyHoles) + } + + fn take_other_panic(self: Arc, message: String) { + panic!("{}", message); + } + + fn clone_me(&self) -> Arc { + let other = self.other.lock().unwrap(); + let new_other = Mutex::new(other.clone()); + *NUM_ALIVE.write().unwrap() += 1; + Arc::new(Self { + name: self.name.clone(), + other: new_other, + }) + } } impl Drop for Coveralls { @@ -134,6 +175,27 @@ impl Drop for Coveralls { *NUM_ALIVE.write().unwrap() -= 1; } } +#[derive(Debug, Clone, Copy)] +enum Color { + Red, + Blue, + Green, +} + +#[derive(Debug)] +struct Patch { + color: Color, +} + +impl Patch { + fn new(color: Color) -> Self { + Self { color } + } + + fn get_color(&self) -> Color { + self.color + } +} // This is a small implementation of a counter that allows waiting on one thread, // and counting on another thread. We use it to test that the UniFFI generated scaffolding diff --git a/fixtures/coverall/tests/bindings/test_coverall.kts b/fixtures/coverall/tests/bindings/test_coverall.kts index ea90e63c0e..8f97deb9f6 100644 --- a/fixtures/coverall/tests/bindings/test_coverall.kts +++ b/fixtures/coverall/tests/bindings/test_coverall.kts @@ -6,31 +6,94 @@ import java.util.concurrent.* import uniffi.coverall.* -// Test some_dict(). // TODO: use an actual test runner. -val d = createSomeDict(); -assert(d.text == "text"); -assert(d.maybeText == "maybe_text"); -assert(d.aBool); -assert(d.maybeABool == false); -assert(d.unsigned8 == 1.toUByte()) -assert(d.maybeUnsigned8 == 2.toUByte()) -assert(d.unsigned64 == 18446744073709551615UL) -assert(d.maybeUnsigned64 == 0UL) -assert(d.signed8 == 8.toByte()) -assert(d.maybeSigned8 == 0.toByte()) -assert(d.signed64 == 9223372036854775807L) -assert(d.maybeSigned64 == 0L) - -// floats should be "close enough". -fun Float.almostEquals(other: Float) = Math.abs(this - other) < 0.000001 -fun Double.almostEquals(other: Double) = Math.abs(this - other) < 0.000001 - -assert(d.float32.almostEquals(1.2345F)) -assert(d.maybeFloat32!!.almostEquals(22.0F/7.0F)) -assert(d.float64.almostEquals(0.0)) -assert(d.maybeFloat64!!.almostEquals(1.0)) +// Test some_dict(). +// N.B. we need to `use` here to clean up the contained `Coveralls` reference. +createSomeDict().use { d -> + assert(d.text == "text"); + assert(d.maybeText == "maybe_text"); + assert(d.aBool); + assert(d.maybeABool == false); + assert(d.unsigned8 == 1.toUByte()) + assert(d.maybeUnsigned8 == 2.toUByte()) + assert(d.unsigned64 == 18446744073709551615UL) + assert(d.maybeUnsigned64 == 0UL) + assert(d.signed8 == 8.toByte()) + assert(d.maybeSigned8 == 0.toByte()) + assert(d.signed64 == 9223372036854775807L) + assert(d.maybeSigned64 == 0L) + + // floats should be "close enough". + fun Float.almostEquals(other: Float) = Math.abs(this - other) < 0.000001 + fun Double.almostEquals(other: Double) = Math.abs(this - other) < 0.000001 + + assert(d.float32.almostEquals(1.2345F)) + assert(d.maybeFloat32!!.almostEquals(22.0F/7.0F)) + assert(d.float64.almostEquals(0.0)) + assert(d.maybeFloat64!!.almostEquals(1.0)) + + assert(d.coveralls!!.getName() == "some_dict") +} + + +// Test arcs. + +Coveralls("test_arcs").use { coveralls -> + assert(getNumAlive() == 1UL); + // One ref held by the foreign-language code, one created for this method call. + assert(coveralls.strongCount() == 2UL); + assert(coveralls.getOther() == null); + coveralls.takeOther(coveralls); + // Should now be a new strong ref, held by the object's reference to itself. + assert(coveralls.strongCount() == 3UL); + // But the same number of instances. + assert(getNumAlive() == 1UL); + // Careful, this makes a new Kotlin object which must be separately destroyed. + coveralls.getOther()!!.use { other -> + // It's the same Rust object. + assert(other.getName() == "test_arcs") + } + try { + coveralls.takeOtherFallible() + throw RuntimeException("Should have thrown a IntegerOverflow exception!") + } catch (e: CoverallErrorException.TooManyHoles) { + // It's okay! + } + try { + coveralls.takeOtherPanic("expected panic: with an arc!") + throw RuntimeException("Should have thrown a IntegerOverflow exception!") + } catch (e: InternalException) { + // No problemo! + } + coveralls.takeOther(null); + assert(coveralls.strongCount() == 2UL); +} +assert(getNumAlive() == 0UL); + +// Test return objects + +Coveralls("test_return_objects").use { coveralls -> + assert(getNumAlive() == 1UL) + assert(coveralls.strongCount() == 2UL) + coveralls.cloneMe().use { c2 -> + assert(c2.getName() == coveralls.getName()) + assert(getNumAlive() == 2UL) + assert(c2.strongCount() == 2UL) + + coveralls.takeOther(c2) + // same number alive but `c2` has an additional ref count. + assert(getNumAlive() == 2UL) + assert(coveralls.strongCount() == 2UL) + assert(c2.strongCount() == 3UL) + } + // Here we've dropped Kotlin's reference to `c2`, but the rust struct will not + // be dropped as coveralls hold an `Arc<>` to it. + assert(getNumAlive() == 2UL) +} +// Destroying `coveralls` will kill both. +assert(getNumAlive() == 0UL); + // This tests that the UniFFI-generated scaffolding doesn't introduce any unexpected locking. // We have one thread busy-wait for a some period of time, while a second thread repeatedly diff --git a/fixtures/coverall/tests/bindings/test_coverall.py b/fixtures/coverall/tests/bindings/test_coverall.py index d7fb1accd4..28822ee4ee 100644 --- a/fixtures/coverall/tests/bindings/test_coverall.py +++ b/fixtures/coverall/tests/bindings/test_coverall.py @@ -6,6 +6,11 @@ from coverall import * class TestCoverall(unittest.TestCase): + # Any test not terminating with zero objects alive will cause others to + # fail - this helps us work out which test kept things alive. + def tearDown(self): + self.assertEqual(get_num_alive(), 0) + def test_some_dict(self): d = create_some_dict() self.assertEqual(d.text, "text") @@ -20,6 +25,7 @@ def test_some_dict(self): self.assertEqual(d.maybe_signed8, 0) self.assertEqual(d.signed64, 9223372036854775807) self.assertEqual(d.maybe_signed64, 0) + self.assertEqual(d.coveralls.get_name(), "some_dict") # floats should be "close enough" - although it's mildly surprising that # we need to specify `places=6` whereas the default is 7. @@ -47,6 +53,7 @@ def test_none_dict(self): self.assertIsNone(d.maybe_float32) self.assertAlmostEqual(d.float64, 0.0) self.assertIsNone(d.maybe_float64) + self.assertIsNone(d.coveralls) def test_constructors(self): self.assertEqual(get_num_alive(), 0) @@ -67,7 +74,7 @@ def test_constructors(self): with self.assertRaisesRegex(InternalError, "expected panic: woe is me"): Coveralls.panicing_new("expected panic: woe is me") - # in the absence of cycles Python is deterministic killing refs + # in the absence of cycles Python is deterministic in killing refs coveralls2 = None self.assertEqual(get_num_alive(), 1) coveralls = None @@ -89,5 +96,60 @@ def test_self_by_arc(self): # One reference is held by the handlemap, and one by the `Arc` method receiver. self.assertEqual(coveralls.strong_count(), 2) + def test_arcs(self): + coveralls = Coveralls("test_arcs") + self.assertEqual(get_num_alive(), 1) + self.assertEqual(coveralls.strong_count(), 2) + self.assertIsNone(coveralls.get_other()) + coveralls.take_other(coveralls) + # should now be a new strong ref. + self.assertEqual(coveralls.strong_count(), 3) + # but the same number of instances. + self.assertEqual(get_num_alive(), 1) + # and check it's the correct object. + self.assertEqual(coveralls.get_other().get_name(), "test_arcs") + + with self.assertRaises(CoverallError.TooManyHoles): + coveralls.take_other_fallible() + + with self.assertRaisesRegex(InternalError, "expected panic: with an arc!"): + coveralls.take_other_panic("expected panic: with an arc!") + + coveralls.take_other(None) + self.assertEqual(coveralls.strong_count(), 2) + coveralls = None + self.assertEqual(get_num_alive(), 0) + + def test_return_objects(self): + coveralls = Coveralls("test_return_objects") + self.assertEqual(get_num_alive(), 1) + self.assertEqual(coveralls.strong_count(), 2) + c2 = coveralls.clone_me() + self.assertEqual(c2.get_name(), coveralls.get_name()) + self.assertEqual(get_num_alive(), 2) + self.assertEqual(c2.strong_count(), 2) + + coveralls.take_other(c2) + # same number alive but `c2` has an additional ref count. + self.assertEqual(get_num_alive(), 2) + self.assertEqual(coveralls.strong_count(), 2) + self.assertEqual(c2.strong_count(), 3) + + # We can drop Python's reference to `c2`, but the rust struct will not + # be dropped as coveralls hold an `Arc<>` to it. + c2 = None + self.assertEqual(get_num_alive(), 2) + + # Dropping `coveralls` will kill both. + coveralls = None + self.assertEqual(get_num_alive(), 0) + + 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) + if __name__=='__main__': unittest.main() diff --git a/fixtures/coverall/tests/bindings/test_coverall.rb b/fixtures/coverall/tests/bindings/test_coverall.rb index ae23faa1d5..81d996b2c4 100644 --- a/fixtures/coverall/tests/bindings/test_coverall.rb +++ b/fixtures/coverall/tests/bindings/test_coverall.rb @@ -8,6 +8,7 @@ require 'coverall' class TestCoverall < Test::Unit::TestCase + def test_some_dict d = Coverall.create_some_dict assert_equal(d.text, 'text') @@ -28,6 +29,8 @@ def test_some_dict assert_equal(d.float64, 0.0) assert_equal(d.maybe_float64, 1.0) + + assert_equal(d.coveralls.get_name(), "some_dict") end def test_none_dict @@ -52,6 +55,7 @@ def test_none_dict end def test_constructors + GC.start assert_equal(Coverall.get_num_alive, 0) # must work. coveralls = Coverall::Coveralls.new 'c1' @@ -77,9 +81,9 @@ def test_constructors end begin - obejcts = 10.times.map { Coverall::Coveralls.new 'c1' } + objects = 10.times.map { Coverall::Coveralls.new 'c1' } assert_equal 12, Coverall.get_num_alive - obejcts = nil + objects = nil GC.start end @@ -109,4 +113,88 @@ def test_self_by_arc # One reference is held by the handlemap, and one by the `Arc` method receiver. assert_equal coveralls.strong_count, 2 end + + def test_arcs + GC.start + coveralls = Coverall::Coveralls.new 'test_arcs' + assert_equal 1, Coverall.get_num_alive + + assert_equal 2, coveralls.strong_count + assert_equal nil, coveralls.get_other + + coveralls.take_other coveralls + # should now be a new strong ref. + assert_equal 3, coveralls.strong_count + # but the same number of instances. + assert_equal 1, Coverall.get_num_alive + # and check it's the correct object. + assert_equal "test_arcs", coveralls.get_other.get_name + + # Using `assert_raise` here would keep a reference to `coveralls` alive + # by capturing it in a closure, which would interfere with the tests. + begin + coveralls.take_other_fallible + rescue Coverall::CoverallError::TooManyHoles + # OK + else + raise 'should have thrown' + end + + begin + coveralls.take_other_panic "expected panic: with an arc!" + rescue Coverall::InternalError => err + assert_match /expected panic: with an arc!/, err.message + else + raise 'should have thrown' + end + + coveralls.take_other nil + GC.start + assert_equal 2, coveralls.strong_count + + # Reference cleanup includes the cached most recent exception. + coveralls = nil + GC.start + assert_equal 0, Coverall.get_num_alive + + end + + def test_return_objects + GC.start + coveralls = Coverall::Coveralls.new "test_return_objects" + assert_equal Coverall.get_num_alive, 1 + assert_equal coveralls.strong_count, 2 + c2 = coveralls.clone_me() + assert_equal c2.get_name(), coveralls.get_name() + assert_equal Coverall.get_num_alive(), 2 + assert_equal c2.strong_count(), 2 + + coveralls.take_other(c2) + # same number alive but `c2` has an additional ref count. + assert_equal Coverall.get_num_alive(), 2 + assert_equal coveralls.strong_count(), 2 + assert_equal c2.strong_count(), 3 + + # We can drop Ruby's reference to `c2`, but the Rust struct will not + # be dropped as coveralls hold an `Arc<>` to it. + c2 = nil + GC.start + assert_equal Coverall.get_num_alive(), 2 + + # Dropping `coveralls` will kill both. + coveralls = nil + GC.start + assert_equal Coverall.get_num_alive(), 0 + 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 intance, got.*Patch/ do + coveralls.take_other patch + end + end + + end diff --git a/fixtures/coverall/tests/bindings/test_coverall.swift b/fixtures/coverall/tests/bindings/test_coverall.swift new file mode 100644 index 0000000000..be6d539445 --- /dev/null +++ b/fixtures/coverall/tests/bindings/test_coverall.swift @@ -0,0 +1,99 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +import coverall + +// TODO: use an actual test runner. + + +// Floats should be "close enough" for testing purposes. +fileprivate extension Double { + func almostEquals(_ other: Double) -> Bool { + return abs(self - other) < 0.000001 + } +} + +fileprivate extension Float { + func almostEquals(_ other: Float) -> Bool { + return abs(self - other) < 0.000001 + } +} + +// Test some_dict(). +do { + let d = createSomeDict() + assert(d.text == "text") + assert(d.maybeText == "maybe_text") + assert(d.aBool) + assert(d.maybeABool == false); + assert(d.unsigned8 == 1) + assert(d.maybeUnsigned8 == 2) + assert(d.unsigned64 == 18446744073709551615) + assert(d.maybeUnsigned64 == 0) + assert(d.signed8 == 8) + assert(d.maybeSigned8 == 0) + assert(d.signed64 == 9223372036854775807) + assert(d.maybeSigned64 == 0) + assert(d.float32.almostEquals(1.2345)) + assert(d.maybeFloat32!.almostEquals(22.0/7.0)) + assert(d.float64.almostEquals(0.0)) + assert(d.maybeFloat64!.almostEquals(1.0)) + assert(d.coveralls!.getName() == "some_dict") +} + +// Test arcs. +do { + let coveralls = Coveralls(name: "test_arcs") + assert(getNumAlive() == 1) + // One ref held by the foreign-language code, one created for this method call. + assert(coveralls.strongCount() == 2) + assert(coveralls.getOther() == nil) + coveralls.takeOther(other: coveralls) + // Should now be a new strong ref, held by the object's reference to itself. + assert(coveralls.strongCount() == 3) + // But the same number of instances. + assert(getNumAlive() == 1) + // It's the same Rust object. + assert(coveralls.getOther()!.getName() == "test_arcs") + do { + try coveralls.takeOtherFallible() + fatalError("Should have thrown") + } catch CoverallError.TooManyHoles { + // It's okay! + } + // TODO: kinda hard to test this, as it triggers a fatal error. + // coveralls!.takeOtherPanic(message: "expected panic: with an arc!") + coveralls.takeOther(other: nil); + assert(coveralls.strongCount() == 2); +} +// Swift GC is deterministic, `coveralls` is freed when it goes out of scope. +assert(getNumAlive() == 0); + + +// Test return objects +do { + let coveralls = Coveralls(name: "test_return_objects") + assert(getNumAlive() == 1) + assert(coveralls.strongCount() == 2) + do { + let c2 = coveralls.cloneMe() + assert(c2.getName() == coveralls.getName()) + assert(c2 != coveralls) + assert(getNumAlive() == 2) + assert(c2.strongCount() == 2) + + coveralls.takeOther(other: c2) + // same number alive but `c2` has an additional ref count. + assert(getNumAlive() == 2) + assert(coveralls.strongCount() == 2) + assert(c2.strongCount() == 3) + + assert(coveralls.getOther() == c2) + } + // We can drop Swifts's reference to `c2`, but the rust struct will not + // be dropped as coveralls hold an `Arc<>` to it. + assert(getNumAlive() == 2) +} +// Dropping `coveralls` will kill both. +assert(getNumAlive() == 0) diff --git a/fixtures/coverall/tests/test_generated_bindings.rs b/fixtures/coverall/tests/test_generated_bindings.rs index a65f29c67a..777324c290 100644 --- a/fixtures/coverall/tests/test_generated_bindings.rs +++ b/fixtures/coverall/tests/test_generated_bindings.rs @@ -4,6 +4,7 @@ uniffi_macros::build_foreign_language_testcases!( "tests/bindings/test_coverall.py", "tests/bindings/test_coverall.kts", "tests/bindings/test_coverall.rb", + "tests/bindings/test_coverall.swift", "tests/bindings/test_handlerace.kts", ] ); diff --git a/uniffi/Cargo.toml b/uniffi/Cargo.toml index a6d1f33d5e..83c75ee178 100644 --- a/uniffi/Cargo.toml +++ b/uniffi/Cargo.toml @@ -14,7 +14,7 @@ keywords = ["ffi", "bindgen"] # Re-exported dependencies used in generated Rust scaffolding files. anyhow = "1" bytes = "1.0" -ffi-support = "~0.4.2" +ffi-support = "~0.4.3" lazy_static = "1.4" log = "0.4" # Regular dependencies diff --git a/uniffi/src/ffi/handle_maps.rs b/uniffi/src/ffi/handle_maps.rs deleted file mode 100644 index a538ea79b6..0000000000 --- a/uniffi/src/ffi/handle_maps.rs +++ /dev/null @@ -1,363 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ - -use ffi_support::{ExternError, Handle, HandleError, HandleMap, IntoFfi}; -use std::sync::{Arc, RwLock}; - -/// `ArcHandleMap` is a relatively thin wrapper around `RwLock>>`. -/// This is only suitable for objects that implement `Sync` and `Send` and is -/// for objects that are able to look after their own locking, and need to be called -/// from multiple foreign language threads at the same time. -/// -/// This is in contrast to the `ConcurrentHandleMap` struct from `ffi-support`, which -/// allows objects with `mut` methods but which only lets the objects be accessed from one -/// thread at a time. -/// -// Some care is taken to protect that handlemap itself being read and written concurrently, and -// that the lock is held for the least amount of time; however, if it is ever poisoned, it will -// panic on both read and write. -pub struct ArcHandleMap -where - T: Sync + Send, -{ - /// The underlying map. Public so that more advanced use-cases - /// may use it as they please. - pub map: RwLock>>, -} - -impl ArcHandleMap { - /// Construct a new `ArcHandleMap`. - pub fn new() -> Self { - ArcHandleMap { - map: RwLock::new(HandleMap::new()), - } - } - - /// Get the number of entries in the `ArcHandleMap`. - /// - /// This takes the map's `read` lock. - #[inline] - pub fn len(&self) -> usize { - let map = self.map.read().unwrap(); - map.len() - } - - /// Returns true if the `ArcHandleMap` is empty. - /// - /// This takes the map's `read` lock. - #[inline] - pub fn is_empty(&self) -> bool { - self.len() == 0 - } - - /// Insert an item into the map, returning the newly allocated handle to the - /// item. - /// - /// This takes the map's `write` lock which the object is inserted. - pub fn insert(&self, v: T) -> Handle { - let mut map = self.map.write().unwrap(); - map.insert(Arc::new(v)) - } - - /// Remove an item from the map. - /// - /// This takes the map's `write` lock while removing from the map, but - /// not while the object is being dropped. - pub fn delete(&self, h: Handle) -> Result<(), HandleError> { - // We use `remove` and not delete (and use the inner block) to ensure - // that if `v`'s destructor panics, we aren't holding the write lock - // when it happens, so that the map itself doesn't get poisoned. - let v = { - let mut map = self.map.write().unwrap(); - map.remove(h) - }; - v.map(drop) - } - - /// Convenient wrapper for `delete` which takes a `u64` that it will - /// convert to a handle. - /// - /// The main benefit (besides convenience) of this over the version - /// that takes a [`Handle`] is that it allows handling handle-related errors - /// in one place. - pub fn delete_u64(&self, h: u64) -> Result<(), HandleError> { - self.delete(Handle::from_u64(h)?) - } - - /// Remove an item from the map, returning either the item, - /// or None if its guard mutex got poisoned at some point. - /// - /// This takes the map's `write` lock, and unwraps the `Arc`. - pub fn remove(&self, h: Handle) -> Result, HandleError> { - let mut map = self.map.write().unwrap(); - let arc = map.remove(h)?; - match Arc::try_unwrap(arc) { - Ok(obj) => Ok(Some(obj)), - _ => Ok(None), - } - } - - /// Convenient wrapper for `remove` which takes a `u64` that it will - /// convert to a handle. - /// - /// The main benefit (besides convenience) of this over the version - /// that takes a [`Handle`] is that it allows handling handle-related errors - /// in one place. - pub fn remove_u64(&self, h: u64) -> Result, HandleError> { - self.remove(Handle::from_u64(h)?) - } - - /// Call `callback` with a non-mutable reference to the item from the map, - /// after acquiring the necessary locks. - /// - /// This takes the map's `read` lock for as long as needed to clone the inner `Arc`. - /// This is so the lock isn't held while the callback is in use. - pub fn get(&self, h: Handle, callback: F) -> Result - where - F: FnOnce(&T) -> Result, - E: From, - { - let obj = { - let map = self.map.read().unwrap(); - let obj = map.get(h)?; - Arc::clone(&obj) - }; - callback(&*obj) - } - - /// Convenient wrapper for `get` which takes a `u64` that it will convert to - /// a handle. - /// - /// The other benefit (besides convenience) of this over the version - /// that takes a [`Handle`] is that it allows handling handle-related errors - /// in one place. - /// - /// This takes the map's `read` lock for as long as needed to clone the inner `Arc`. - /// This is so the lock isn't held while the callback is in use. - pub fn get_u64(&self, u: u64, callback: F) -> Result - where - F: FnOnce(&T) -> Result, - E: From, - { - self.get(Handle::from_u64(u)?, callback) - } - - /// Helper that performs both a [`call_with_result`] and [`get`](ArcHandleMap::get). - /// - /// The callback is called with a clone of the inner `Arc`, returning a `Result`. - /// - /// This takes the map's `read` lock for as long as needed to clone the inner `Arc`. - /// This is so the lock isn't held while the callback is in use. - /// - /// This is the most general of the `call_` helper methods; all other call helpers - /// can be implemented in terms of this one. - pub fn call_by_arc_with_result( - &self, - out_error: &mut ExternError, - h: u64, - callback: F, - ) -> R::Value - where - F: std::panic::UnwindSafe + FnOnce(Arc) -> Result, - ExternError: From, - R: IntoFfi, - { - use ffi_support::call_with_result; - call_with_result(out_error, || -> Result<_, ExternError> { - // We can't reuse `get` here because it would require E: - // From, which is inconvenient... - let h = Handle::from_u64(h)?; - let obj_arc = { - let map = self.map.read().unwrap(); - let obj = map.get(h)?; - Arc::clone(&obj) - }; - Ok(callback(obj_arc)?) - }) - } - - /// Helper that performs both a [`call_with_result`] and [`get`](ArcHandleMap::get). - /// - /// The callback is called with a reference to the contained struct, returning a `Result`. - /// - /// This takes the map's `read` lock for as long as needed to clone the inner `Arc`. - /// This is so the lock isn't held while the callback is in use. - pub fn call_with_result( - &self, - out_error: &mut ExternError, - h: u64, - callback: F, - ) -> R::Value - where - F: std::panic::UnwindSafe + FnOnce(&T) -> Result, - ExternError: From, - R: IntoFfi, - { - self.call_by_arc_with_result(out_error, h, |obj_arc: Arc| -> Result<_, E> { - let obj = &*obj_arc; - callback(obj) - }) - } - - /// Helper that performs both a [`call_with_output`] and [`get`](ArcHandleMap::get). - /// - /// The callback is called with a clone of the inner `Arc`, returning a value. - pub fn call_by_arc_with_output( - &self, - out_error: &mut ExternError, - h: u64, - callback: F, - ) -> R::Value - where - F: std::panic::UnwindSafe + FnOnce(Arc) -> R, - R: IntoFfi, - { - self.call_by_arc_with_result(out_error, h, |obj_arc: Arc| -> Result<_, HandleError> { - Ok(callback(obj_arc)) - }) - } - - /// Helper that performs both a [`call_with_output`] and [`get`](ArcHandleMap::get). - /// - /// The callback is called with a reference to the contained struct, returning a value. - pub fn call_with_output( - &self, - out_error: &mut ExternError, - h: u64, - callback: F, - ) -> R::Value - where - F: std::panic::UnwindSafe + FnOnce(&T) -> R, - R: IntoFfi, - { - self.call_with_result(out_error, h, |obj| -> Result<_, HandleError> { - Ok(callback(obj)) - }) - } - - /// Use `constructor` to create and insert a `T`, while inside a - /// [`call_with_result`] call (to handle panics and map errors onto an - /// `ExternError`). - /// - /// This takes the map's `write` lock for as long as needed to insert into the map. - /// This is so the lock isn't held while the constructor is being called. - pub fn insert_with_result(&self, out_error: &mut ExternError, constructor: F) -> u64 - where - F: std::panic::UnwindSafe + FnOnce() -> Result, - ExternError: From, - { - use ffi_support::call_with_result; - call_with_result(out_error, || -> Result<_, ExternError> { - // Note: it's important that we don't call the constructor while - // we're holding the write lock, because we don't want to poison - // the entire map if it panics! - let to_insert = constructor()?; - Ok(self.insert(to_insert)) - }) - } - - /// Equivalent to - /// [`insert_with_result`](ArcHandleMap::insert_with_result) for the - /// case where the constructor cannot produce an error. - /// - /// The name is somewhat dubious, since there's no `output`, but it's intended to make it - /// clear that it contains a [`call_with_output`] internally. - pub fn insert_with_output(&self, out_error: &mut ExternError, constructor: F) -> u64 - where - F: std::panic::UnwindSafe + FnOnce() -> T, - { - // The Err type isn't important here beyond being convertable to ExternError - self.insert_with_result(out_error, || -> Result<_, HandleError> { - Ok(constructor()) - }) - } -} - -impl Default for ArcHandleMap { - #[inline] - fn default() -> Self { - Self::new() - } -} - -/// Tests that check our behavior when panicking. -/// -/// Naturally these require panic=unwind, which means we can't run them when -/// generating coverage (well, `-Zprofile`-based coverage can't -- although -/// ptrace-based coverage like tarpaulin can), and so we turn them off. -/// -/// (For clarity, `cfg(coverage)` is not a standard thing. We add it in -/// `automation/emit_coverage_info.sh`, and you can force it by adding -/// "--cfg coverage" to your RUSTFLAGS manually if you need to do so). -/// -/// Note: these tests are derived directly from ffi_support::ConcurrentHandleMap. -#[cfg(not(coverage))] -#[allow(unused_imports)] -mod panic_tests { - use super::ArcHandleMap; - use ffi_support::{call_with_result, ErrorCode, ExternError}; - - #[derive(PartialEq, Debug)] - pub(super) struct Foobar(usize); - - struct PanicOnDrop(()); - impl Drop for PanicOnDrop { - fn drop(&mut self) { - panic!("intentional panic (drop)"); - } - } - - #[test] - fn test_panicking_drop() { - let map = ArcHandleMap::new(); - let h = map.insert(PanicOnDrop(())).into_u64(); - let mut e = ExternError::success(); - call_with_result(&mut e, || map.delete_u64(h)); - assert_eq!(e.get_code(), ErrorCode::PANIC); - let _ = unsafe { e.get_and_consume_message() }; - assert!(!map.map.is_poisoned()); - let inner = map.map.read().unwrap(); - assert_eq!(inner.len(), 0); - } - - #[test] - fn test_panicking_call_with() { - let map = ArcHandleMap::new(); - let h = map.insert(Foobar(0)).into_u64(); - let mut e = ExternError::success(); - map.call_with_output(&mut e, h, |_thing| { - panic!("intentional panic (call_with_output)"); - }); - - assert_eq!(e.get_code(), ErrorCode::PANIC); - let _ = unsafe { e.get_and_consume_message() }; - { - assert!(!map.map.is_poisoned()); - let inner = map.map.read().unwrap(); - assert_eq!(inner.len(), 1); - } - assert!(map.delete_u64(h).is_ok()); - assert!(!map.map.is_poisoned()); - let inner = map.map.read().unwrap(); - assert_eq!(inner.len(), 0); - } - - #[test] - fn test_panicking_insert_with() { - let map = ArcHandleMap::new(); - let mut e = ExternError::success(); - let res = map.insert_with_output(&mut e, || { - panic!("intentional panic (insert_with_output)"); - }); - - assert_eq!(e.get_code(), ErrorCode::PANIC); - let _ = unsafe { e.get_and_consume_message() }; - - assert_eq!(res, 0); - - assert!(!map.map.is_poisoned()); - let inner = map.map.read().unwrap(); - assert_eq!(inner.len(), 0); - } -} diff --git a/uniffi/src/ffi/mod.rs b/uniffi/src/ffi/mod.rs index 3edf2eb4a8..03c12fabcb 100644 --- a/uniffi/src/ffi/mod.rs +++ b/uniffi/src/ffi/mod.rs @@ -4,10 +4,8 @@ pub mod foreignbytes; pub mod foreigncallbacks; -pub mod handle_maps; pub mod rustbuffer; pub use foreignbytes::*; pub use foreigncallbacks::*; -pub use handle_maps::*; pub use rustbuffer::*; diff --git a/uniffi/src/lib.rs b/uniffi/src/lib.rs index 7d87636e7d..88bb535fcc 100644 --- a/uniffi/src/lib.rs +++ b/uniffi/src/lib.rs @@ -49,7 +49,6 @@ pub mod deps { pub use anyhow; pub use bytes; pub use ffi_support; - pub use lazy_static; pub use log; pub use static_assertions; } @@ -566,6 +565,67 @@ unsafe impl ViaFfi for HashMap { } } +/// Support for passing reference-counted shared objects via the FFI. +/// +/// To avoid dealing with complex lifetime semantics over the FFI, any data passed +/// by reference must be encapsulated in an `Arc`, and must be safe to share +/// across threads. +unsafe impl ViaFfi for std::sync::Arc { + // Don't use a pointer to as that requires a `pub ` + type FfiType = *const std::os::raw::c_void; + + /// When lowering, we have an owned `Arc` and we transfer that ownership + /// to the foreign-language code, "leaking" it out of Rust's ownership system + /// as a raw pointer. This works safely because we have unique ownership of `self`. + /// The foreign-language code is responsible for freeing this by calling the + /// `ffi_object_free` FFI function provided by the corresponding UniFFI type. + /// + /// Safety: when freeing the resulting pointer, the foreign-language code must + /// call the destructor function specific to the type `T`. Calling the destructor + /// function for other types may lead to undefined behaviour. + fn lower(self) -> Self::FfiType { + std::sync::Arc::into_raw(self) as Self::FfiType + } + + /// When lifting, we receive a "borrow" of the `Arc` that is owned by + /// the foreign-language code, and make a clone of it for our own use. + /// + /// Safety: the provided value must be a pointer previously obtained by calling + /// the `lower()` or `write()` method of this impl. + fn try_lift(v: Self::FfiType) -> Result { + let v = v as *const T; + // We musn't drop the `Arc` 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)) + } + + /// When writing as a field of a complex structure, make a clone and transfer ownership + /// of it to the foreign-language code by writing its pointer into the buffer. + /// The foreign-language code is responsible for freeing this by calling the + /// `ffi_object_free` FFI function provided by the corresponding UniFFI type. + /// + /// Safety: when freeing the resulting pointer, the foreign-language code must + /// call the destructor function specific to the type `T`. Calling the destructor + /// function for other types may lead to undefined behaviour. + fn write(&self, buf: &mut B) { + static_assertions::const_assert!(std::mem::size_of::<*const std::ffi::c_void>() <= 8); + let ptr = std::sync::Arc::clone(self).lower(); + buf.put_u64(ptr as u64); + } + + /// When reading as a field of a complex structure, we receive a "borrow" of the `Arc` + /// that is owned by the foreign-language code, and make a clone for our own use. + /// + /// Safety: the buffer must contain a pointer previously obtained by calling + /// the `lower()` or `write()` method of this impl. + fn try_read(buf: &mut B) -> Result { + static_assertions::const_assert!(std::mem::size_of::<*const std::ffi::c_void>() <= 8); + check_remaining(buf, 8)?; + Self::try_lift(buf.get_u64() as Self::FfiType) + } +} + #[cfg(test)] mod test { use super::*; diff --git a/uniffi_bindgen/src/bindings/gecko_js/gen_gecko_js.rs b/uniffi_bindgen/src/bindings/gecko_js/gen_gecko_js.rs index dbf2da9cc3..6a3295f4c1 100644 --- a/uniffi_bindgen/src/bindings/gecko_js/gen_gecko_js.rs +++ b/uniffi_bindgen/src/bindings/gecko_js/gen_gecko_js.rs @@ -332,6 +332,7 @@ mod filters { FFIType::Float32 => "float".into(), FFIType::Float64 => "double".into(), FFIType::RustCString => "const char*".into(), + FFIType::RustArcPtr => unimplemented!("object pointers are not implemented"), FFIType::RustBuffer => context.ffi_rustbuffer_type(), FFIType::RustError => context.ffi_rusterror_type(), FFIType::ForeignBytes => context.ffi_foreignbytes_type(), diff --git a/uniffi_bindgen/src/bindings/kotlin/gen_kotlin.rs b/uniffi_bindgen/src/bindings/kotlin/gen_kotlin.rs index c62098fc10..28ef1b5425 100644 --- a/uniffi_bindgen/src/bindings/kotlin/gen_kotlin.rs +++ b/uniffi_bindgen/src/bindings/kotlin/gen_kotlin.rs @@ -114,6 +114,7 @@ mod filters { FFIType::Float32 => "Float".to_string(), FFIType::Float64 => "Double".to_string(), FFIType::RustCString => "Pointer".to_string(), + FFIType::RustArcPtr => "Pointer".to_string(), FFIType::RustBuffer => "RustBuffer.ByValue".to_string(), FFIType::RustError => "RustError".to_string(), FFIType::ForeignBytes => "ForeignBytes.ByValue".to_string(), diff --git a/uniffi_bindgen/src/bindings/kotlin/templates/EnumTemplate.kt b/uniffi_bindgen/src/bindings/kotlin/templates/EnumTemplate.kt index b5b55b1095..332a881a2a 100644 --- a/uniffi_bindgen/src/bindings/kotlin/templates/EnumTemplate.kt +++ b/uniffi_bindgen/src/bindings/kotlin/templates/EnumTemplate.kt @@ -35,7 +35,7 @@ enum class {{ e.name()|class_name_kt }} { {% else %} -sealed class {{ e.name()|class_name_kt }} { +sealed class {{ e.name()|class_name_kt }}{% if e.contains_object_references(ci) %}: Destroyable {% endif %} { {% for variant in e.variants() -%} {% if !variant.has_fields() -%} object {{ variant.name()|class_name_kt }} : {{ e.name()|class_name_kt }}() @@ -78,11 +78,28 @@ sealed class {{ e.name()|class_name_kt }} { buf.putInt({{ loop.index }}) {% for field in variant.fields() -%} {{ "(this.{})"|format(field.name())|write_kt("buf", field.type_()) }} - {% endfor -%} + {% endfor %} } {%- endfor %} }.let { /* this makes the `when` an expression, which ensures it is exhaustive */ } } + + {% if e.contains_object_references(ci) %} + @Suppress("UNNECESSARY_SAFE_CALL") // codegen is much simpler if we unconditionally emit safe calls here + override fun destroy() { + when(this) { + {%- for variant in e.variants() %} + is {{ e.name()|class_name_kt }}.{{ variant.name()|class_name_kt }} -> { + {% for field in variant.fields() -%} + {%- if ci.type_contains_object_references(field.type_()) -%} + this.{{ field.name() }}?.destroy() + {% endif -%} + {%- endfor %} + } + {%- endfor %} + }.let { /* this makes the `when` an expression, which ensures it is exhaustive */ } + } + {% endif %} } {% endif %} diff --git a/uniffi_bindgen/src/bindings/kotlin/templates/Helpers.kt b/uniffi_bindgen/src/bindings/kotlin/templates/Helpers.kt index 8ffb86a80a..ad0391afde 100644 --- a/uniffi_bindgen/src/bindings/kotlin/templates/Helpers.kt +++ b/uniffi_bindgen/src/bindings/kotlin/templates/Helpers.kt @@ -2,20 +2,44 @@ // This would be a good candidate for isolating in its own ffi-support lib. {% if ci.iter_object_definitions().len() > 0 %} + +// Interface implemented by anything that can contain an object reference. +// +// Such types expose a `destroy()` method that must be called to cleanly +// dispose of the contained objects. Failure to call this method may result +// in memory leaks. +// +// The easiest way to ensure this method is called is to use the `.use` +// helper method to execute a block and destroy the object at the end. +interface Destroyable { + fun destroy() +} + +inline fun T.use(block: (T) -> R) = + try { + block(this) + } finally { + try { + this?.destroy() + } catch (e: Throwable) { + // swallow + } + } + // The base class for all UniFFI Object types. // -// This class provides core operations for working with the integer handle that -// maps to the live Rust struct on the other side of the FFI. +// This class provides core operations for working with the Rust `Arc` pointer to +// the live Rust struct on the other side of the FFI. // // There's some subtlety here, because we have to be careful not to operate on a Rust // struct after it has been dropped, and because we must expose a public API for freeing // the Kotlin wrapper object in lieu of reliable finalizers. The core requirements are: // -// * Each `FFIObject` instance has an opaque integer "handle" that is a reference -// to the underlying Rust struct. Method calls need to read this handle from the -// object's state and pass it in to the Rust FFI. +// * Each `FFIObject` instance holds an opaque pointer to the underlying Rust struct. +// Method calls need to read this pointer from the object's state and pass it in to +// the Rust FFI. // -// * When an `FFIObject` is no longer needed, its handle should be passed to a +// * When an `FFIObject` is no longer needed, its pointer should be passed to a // special destructor function provided by the Rust FFI, which will drop the // underlying Rust struct. // @@ -32,13 +56,13 @@ // the destructor has been called, and must never call the destructor more than once. // Doing so may trigger memory unsafety. // -// If we try to implement this with mutual exclusion on access to the handle, there is the +// If we try to implement this with mutual exclusion on access to the pointer, there is the // possibility of a race between a method call and a concurrent call to `destroy`: // -// * Thread A starts a method call, reads the value of the handle, but is interrupted -// before it can pass the handle over the FFI to Rust. +// * Thread A starts a method call, reads the value of the pointer, but is interrupted +// before it can pass the pointer over the FFI to Rust. // * Thread B calls `destroy` and frees the underlying Rust struct. -// * Thread A resumes, passing the already-read handle value to Rust and triggering +// * Thread A resumes, passing the already-read pointer value to Rust and triggering // a use-after-free. // // One possible solution would be to use a `ReadWriteLock`, with each method call taking @@ -84,28 +108,28 @@ // [1] https://stackoverflow.com/questions/24376768/can-java-finalize-an-object-when-it-is-still-in-scope/24380219 // abstract class FFIObject( - protected val handle: Long -) { + protected val pointer: Pointer +): Destroyable { val wasDestroyed = AtomicBoolean(false) val callCounter = AtomicLong(1) - open protected fun freeHandle() { + open protected fun freeRustArcPtr() { // To be overridden in subclasses. } - open fun destroy() { + override fun destroy() { // Only allow a single call to this method. // TODO: maybe we should log a warning if called more than once? if (this.wasDestroyed.compareAndSet(false, true)) { // This decrement always matches the initial count of 1 given at creation time. if (this.callCounter.decrementAndGet() == 0L) { - this.freeHandle() + this.freeRustArcPtr() } } } - internal inline fun callWithHandle(block: (handle: Long) -> R): R { + internal inline fun callWithPointer(block: (ptr: Pointer) -> R): R { // Check and increment the call counter, to keep the object alive. // This needs a compare-and-set retry loop in case of concurrent updates. do { @@ -117,28 +141,17 @@ abstract class FFIObject( throw IllegalStateException("${this.javaClass.simpleName} call counter would overflow") } } while (! this.callCounter.compareAndSet(c, c + 1L)) - // Now we can safely do the method call without the handle being freed concurrently. + // Now we can safely do the method call without the pointer being freed concurrently. try { - return block(handle) + return block(this.pointer) } finally { // This decrement aways matches the increment we performed above. if (this.callCounter.decrementAndGet() == 0L) { - this.freeHandle() + this.freeRustArcPtr() } } } } - -inline fun T.use(block: (T) -> R) = - try { - block(this) - } finally { - try { - this.destroy() - } catch (e: Throwable) { - // swallow - } - } {% endif %} {% if ci.iter_callback_interface_definitions().len() > 0 %} diff --git a/uniffi_bindgen/src/bindings/kotlin/templates/ObjectTemplate.kt b/uniffi_bindgen/src/bindings/kotlin/templates/ObjectTemplate.kt index 21b32d512c..1894b76033 100644 --- a/uniffi_bindgen/src/bindings/kotlin/templates/ObjectTemplate.kt +++ b/uniffi_bindgen/src/bindings/kotlin/templates/ObjectTemplate.kt @@ -9,8 +9,8 @@ public interface {{ obj.name()|class_name_kt }}Interface { } class {{ obj.name()|class_name_kt }}( - handle: Long -) : FFIObject(handle), {{ obj.name()|class_name_kt }}Interface { + pointer: Pointer +) : FFIObject(pointer), {{ obj.name()|class_name_kt }}Interface { {%- match obj.primary_constructor() %} {%- when Some with (cons) %} @@ -22,23 +22,33 @@ class {{ obj.name()|class_name_kt }}( /** * Disconnect the object from the underlying Rust object. * - * It can be called more than once, but once called, interacting with the object + * It can be called more than once, but once called, interacting with the object * causes an `IllegalStateException`. * * Clients **must** call this method once done with the object, or cause a memory leak. */ - override protected fun freeHandle() { + override protected fun freeRustArcPtr() { rustCall(InternalError.ByReference()) { err -> - _UniFFILib.INSTANCE.{{ obj.ffi_object_free().name() }}(this.handle, err) + _UniFFILib.INSTANCE.{{ obj.ffi_object_free().name() }}(this.pointer, err) } } + internal fun lower(): Pointer { + callWithPointer { ptr -> return ptr } + } + + internal fun write(buf: RustBufferBuilder) { + // The Rust code always expects pointers written as 8 bytes, + // and will fail to compile if they don't fit. + buf.putLong(Pointer.nativeValue(this.lower())) + } + {% for meth in obj.methods() -%} {%- match meth.return_type() -%} {%- when Some with (return_type) -%} override fun {{ meth.name()|fn_name_kt }}({% call kt::arg_list_protocol(meth) %}): {{ return_type|type_kt }} = - callWithHandle { + callWithPointer { {%- call kt::to_ffi_call_with_prefix("it", meth) %} }.let { {{ "it"|lift_kt(return_type) }} @@ -46,18 +56,26 @@ class {{ obj.name()|class_name_kt }}( {%- when None -%} override fun {{ meth.name()|fn_name_kt }}({% call kt::arg_list_protocol(meth) %}) = - callWithHandle { + callWithPointer { {%- call kt::to_ffi_call_with_prefix("it", meth) %} } {% endmatch %} {% endfor %} - {% if obj.constructors().len() > 1 -%} companion object { + internal fun lift(ptr: Pointer): {{ obj.name()|class_name_kt }} { + return {{ obj.name()|class_name_kt }}(ptr) + } + + internal fun read(buf: ByteBuffer): {{ obj.name()|class_name_kt }} { + // The Rust code always writes pointers as 8 bytes, and will + // fail to compile if they don't fit. + return {{ obj.name()|class_name_kt }}.lift(Pointer(buf.getLong())) + } + {% for cons in obj.alternate_constructors() -%} fun {{ cons.name()|fn_name_kt }}({% call kt::arg_list_decl(cons) %}): {{ obj.name()|class_name_kt }} = {{ obj.name()|class_name_kt }}({% call kt::to_ffi_call(cons) %}) {% endfor %} } - {%- endif %} } diff --git a/uniffi_bindgen/src/bindings/kotlin/templates/RecordTemplate.kt b/uniffi_bindgen/src/bindings/kotlin/templates/RecordTemplate.kt index 2455db2abf..ce6992850c 100644 --- a/uniffi_bindgen/src/bindings/kotlin/templates/RecordTemplate.kt +++ b/uniffi_bindgen/src/bindings/kotlin/templates/RecordTemplate.kt @@ -7,9 +7,8 @@ data class {{ rec.name()|class_name_kt }} ( {%- endmatch -%} {% if !loop.last %}, {% endif %} {%- endfor %} -) { +) {% if rec.contains_object_references(ci) %}: Destroyable {% endif %}{ companion object { - // XXX TODO: put this in a superclass maybe? internal fun lift(rbuf: RustBuffer.ByValue): {{ rec.name()|class_name_kt }} { return liftFromRustBuffer(rbuf) { buf -> {{ rec.name()|class_name_kt }}.read(buf) } } @@ -30,6 +29,17 @@ data class {{ rec.name()|class_name_kt }} ( internal fun write(buf: RustBufferBuilder) { {%- for field in rec.fields() %} {{ "(this.{})"|format(field.name())|write_kt("buf", field.type_()) }} + {% endfor %} + } + + {% if rec.contains_object_references(ci) %} + @Suppress("UNNECESSARY_SAFE_CALL") // codegen is much simpler if we unconditionally emit safe calls here + override fun destroy() { + {% for field in rec.fields() %} + {%- if ci.type_contains_object_references(field.type_()) -%} + this.{{ field.name() }}?.destroy() + {% endif -%} {%- endfor %} } -} \ No newline at end of file + {% endif %} +} diff --git a/uniffi_bindgen/src/bindings/python/gen_python.rs b/uniffi_bindgen/src/bindings/python/gen_python.rs index e41f67c2c0..854df819da 100644 --- a/uniffi_bindgen/src/bindings/python/gen_python.rs +++ b/uniffi_bindgen/src/bindings/python/gen_python.rs @@ -72,7 +72,8 @@ mod filters { FFIType::UInt64 => "ctypes.c_uint64".to_string(), FFIType::Float32 => "ctypes.c_float".to_string(), FFIType::Float64 => "ctypes.c_double".to_string(), - FFIType::RustCString => "ctypes.c_voidp".to_string(), + FFIType::RustCString => "ctypes.c_void_p".to_string(), + FFIType::RustArcPtr => "ctypes.c_void_p".to_string(), FFIType::RustBuffer => "RustBuffer".to_string(), FFIType::RustError => "ctypes.POINTER(RustError)".to_string(), FFIType::ForeignBytes => "ForeignBytes".to_string(), @@ -174,7 +175,7 @@ mod filters { | Type::Float64 => nm.to_string(), Type::Boolean => format!("(1 if {} else 0)", nm), Type::String => format!("RustBuffer.allocFromString({})", nm), - Type::Object(_) => format!("({}._handle)", nm), + Type::Object(_) => format!("({}._pointer)", nm), Type::CallbackInterface(_) => panic!("No support for lowering callback interfaces yet"), Type::Error(_) => panic!("No support for lowering errors, yet"), Type::Enum(_) @@ -204,7 +205,7 @@ mod filters { Type::Float32 | Type::Float64 => format!("float({})", nm), Type::Boolean => format!("(True if {} else False)", nm), Type::String => format!("{}.consumeIntoString()", nm), - Type::Object(_) => panic!("No support for lifting objects, yet"), + Type::Object(name) => format!("{}._make_instance_({})", class_name_py(name)?, nm), Type::CallbackInterface(_) => panic!("No support for lifting callback interfaces, yet"), Type::Error(_) => panic!("No support for lowering errors, yet"), Type::Enum(_) diff --git a/uniffi_bindgen/src/bindings/python/templates/ObjectTemplate.py b/uniffi_bindgen/src/bindings/python/templates/ObjectTemplate.py index cec5a695c0..b917a338af 100644 --- a/uniffi_bindgen/src/bindings/python/templates/ObjectTemplate.py +++ b/uniffi_bindgen/src/bindings/python/templates/ObjectTemplate.py @@ -3,28 +3,36 @@ class {{ obj.name()|class_name_py }}(object): {%- when Some with (cons) %} def __init__(self, {% call py::arg_list_decl(cons) -%}): {%- call py::coerce_args_extra_indent(cons) %} - self._handle = {% call py::to_ffi_call(cons) %}\ + self._pointer = {% call py::to_ffi_call(cons) %} {%- when None %} {%- endmatch %} def __del__(self): - rust_call_with_error( - InternalError, - _UniFFILib.{{ obj.ffi_object_free().name() }}, - self._handle - ) + # In case of partial initialization of instances. + pointer = getattr(self, "_pointer", None) + if pointer is not None: + rust_call_with_error( + InternalError, + _UniFFILib.{{ obj.ffi_object_free().name() }}, + pointer + ) + + # Used by alternative constructors or any methods which return this type. + @classmethod + def _make_instance_(cls, pointer): + # Lightly yucky way to bypass the usual __init__ logic + # and just create a new instance with the required pointer. + inst = cls.__new__(cls) + inst._pointer = pointer + return inst {% for cons in obj.alternate_constructors() -%} @classmethod def {{ cons.name()|fn_name_py }}(cls, {% call py::arg_list_decl(cons) %}): {%- call py::coerce_args_extra_indent(cons) %} # Call the (fallible) function before creating any half-baked object instances. - handle = {% call py::to_ffi_call(cons) %} - # Lightly yucky way to bypass the usual __init__ logic - # and just create a new instance with the required handle. - inst = cls.__new__(cls) - inst._handle = handle - return inst + pointer = {% call py::to_ffi_call(cons) %} + return cls._make_instance_(pointer) {% endfor %} {% for meth in obj.methods() -%} @@ -33,12 +41,12 @@ def {{ cons.name()|fn_name_py }}(cls, {% call py::arg_list_decl(cons) %}): {%- when Some with (return_type) -%} def {{ meth.name()|fn_name_py }}(self, {% call py::arg_list_decl(meth) %}): {%- call py::coerce_args_extra_indent(meth) %} - _retval = {% call py::to_ffi_call_with_prefix("self._handle", meth) %} + _retval = {% call py::to_ffi_call_with_prefix("self._pointer", meth) %} return {{ "_retval"|lift_py(return_type) }} {%- when None -%} def {{ meth.name()|fn_name_py }}(self, {% call py::arg_list_decl(meth) %}): {%- call py::coerce_args_extra_indent(meth) %} - {% call py::to_ffi_call_with_prefix("self._handle", meth) %} + {% call py::to_ffi_call_with_prefix("self._pointer", meth) %} {% endmatch %} {% endfor %} diff --git a/uniffi_bindgen/src/bindings/python/templates/RustBufferBuilder.py b/uniffi_bindgen/src/bindings/python/templates/RustBufferBuilder.py index c4a74bff91..798bb937e5 100644 --- a/uniffi_bindgen/src/bindings/python/templates/RustBufferBuilder.py +++ b/uniffi_bindgen/src/bindings/python/templates/RustBufferBuilder.py @@ -133,10 +133,14 @@ def write{{ canonical_type_name }}(self, v): {% when Type::Object with (object_name) -%} # The Object type {{ object_name }}. - # Objects cannot currently be serialized, but we can produce a helpful error. + # We write the pointer value directly - what could possibly go wrong? - def write{{ canonical_type_name }}(self): - raise InternalError("RustBufferStream.write() not implemented yet for {{ canonical_type_name }}") + def write{{ canonical_type_name }}(self, v): + if not isinstance(v, {{ object_name|class_name_py }}): + raise TypeError("Expected {{ object_name|class_name_py }} instance, {} found".format(v.__class__.__name__)) + # The Rust code always expects pointers written as 8 bytes, + # and will fail to compile if they don't fit in that size. + self.writeU64(v._pointer) {% when Type::CallbackInterface with (object_name) -%} # The Callback Interface type {{ object_name }}. diff --git a/uniffi_bindgen/src/bindings/python/templates/RustBufferStream.py b/uniffi_bindgen/src/bindings/python/templates/RustBufferStream.py index 6daaca4309..4c90e713c6 100644 --- a/uniffi_bindgen/src/bindings/python/templates/RustBufferStream.py +++ b/uniffi_bindgen/src/bindings/python/templates/RustBufferStream.py @@ -129,10 +129,12 @@ def read{{ canonical_type_name }}(self): {% when Type::Object with (object_name) -%} # The Object type {{ object_name }}. - # Objects cannot currently be serialized, but we can produce a helpful error. def read{{ canonical_type_name }}(self): - raise InternalError("RustBufferStream.read not implemented yet for {{ canonical_type_name }}") + # The Rust code always expects pointers written as 8 bytes, + # and will fail to compile if they don't fit in that size. + pointer = self._unpack_from(8, ">Q") + return {{ object_name|class_name_py }}._make_instance_(pointer) {% when Type::CallbackInterface with (object_name) -%} # The Callback Interface type {{ object_name }}. diff --git a/uniffi_bindgen/src/bindings/ruby/gen_ruby.rs b/uniffi_bindgen/src/bindings/ruby/gen_ruby.rs index 620a44560a..dfdc4d423c 100644 --- a/uniffi_bindgen/src/bindings/ruby/gen_ruby.rs +++ b/uniffi_bindgen/src/bindings/ruby/gen_ruby.rs @@ -93,6 +93,7 @@ mod filters { FFIType::Float32 => ":float".to_string(), FFIType::Float64 => ":double".to_string(), FFIType::RustCString => ":string".to_string(), + FFIType::RustArcPtr => ":pointer".to_string(), FFIType::RustBuffer => "RustBuffer.by_value".to_string(), FFIType::RustError => "RustError.by_ref".to_string(), FFIType::ForeignBytes => "ForeignBytes".to_string(), @@ -210,7 +211,7 @@ mod filters { Type::String => format!("RustBuffer.allocFromString({})", nm), Type::Timestamp => panic!("No support for timestamps in Ruby, yet"), Type::Duration => panic!("No support for durations in Ruby, yet"), - Type::Object(_) => format!("({}._handle)", nm), + Type::Object(name) => format!("({}._uniffi_lower {})", class_name_rb(name)?, nm), Type::CallbackInterface(_) => panic!("No support for lowering callback interfaces yet"), Type::Error(_) => panic!("No support for lowering errors, yet"), Type::Enum(_) @@ -240,7 +241,7 @@ mod filters { Type::String => format!("{}.consumeIntoString", nm), Type::Timestamp => panic!("No support for timestamps in Ruby, yet"), Type::Duration => panic!("No support for durations in Ruby, yet"), - Type::Object(_) => panic!("No support for lifting objects, yet"), + Type::Object(name) => format!("{}._uniffi_allocate({})", class_name_rb(name)?, nm), Type::CallbackInterface(_) => panic!("No support for lifting callback interfaces, yet"), Type::Error(_) => panic!("No support for lowering errors, yet"), Type::Enum(_) diff --git a/uniffi_bindgen/src/bindings/ruby/templates/ObjectTemplate.rb b/uniffi_bindgen/src/bindings/ruby/templates/ObjectTemplate.rb index af47374a89..7acef0c7e2 100644 --- a/uniffi_bindgen/src/bindings/ruby/templates/ObjectTemplate.rb +++ b/uniffi_bindgen/src/bindings/ruby/templates/ObjectTemplate.rb @@ -1,22 +1,46 @@ class {{ obj.name()|class_name_rb }} - {%- match obj.primary_constructor() %} - {%- when Some with (cons) %} - def initialize({% call rb::arg_list_decl(cons) -%}) - {%- call rb::coerce_args_extra_indent(cons) %} - handle = {% call rb::to_ffi_call(cons) %} - ObjectSpace.define_finalizer(self, self.class.define_finalizer_by_handle(handle)) - @handle = handle + + # A private helper for initializing instances of the class from a raw pointer, + # bypassing any initialization logic and ensuring they are GC'd properly. + def self._uniffi_allocate(pointer) + pointer.autorelease = false + inst = allocate + inst.instance_variable_set :@pointer, pointer + ObjectSpace.define_finalizer(inst, _uniffi_define_finalizer_by_pointer(pointer, inst.object_id)) + return inst end - def {{ obj.name()|class_name_rb }}.define_finalizer_by_handle(handle) + # A private helper for registering an object finalizer. + # N.B. it's important that this does not capture a reference + # to the actual instance, only its underlying pointer. + def self._uniffi_define_finalizer_by_pointer(pointer, object_id) Proc.new do |_id| {{ ci.namespace()|class_name_rb }}.rust_call_with_error( InternalError, :{{ obj.ffi_object_free().name() }}, - handle + pointer ) end end + + # A private helper for lowering instances into a raw pointer. + # This does an explicit typecheck, because accidentally lowering a different type of + # object in a place where this type is expected, could lead to memory unsafety. + def self._uniffi_lower(inst) + if not inst.is_a? self + raise TypeError.new "Expected a {{ obj.name()|class_name_rb }} intance, got #{inst}" + end + return inst.instance_variable_get :@pointer + end + + {%- match obj.primary_constructor() %} + {%- when Some with (cons) %} + def initialize({% call rb::arg_list_decl(cons) -%}) + {%- call rb::coerce_args_extra_indent(cons) %} + pointer = {% call rb::to_ffi_call(cons) %} + @pointer = pointer + ObjectSpace.define_finalizer(self, self.class._uniffi_define_finalizer_by_pointer(pointer, self.object_id)) + end {%- when None %} {%- endmatch %} @@ -25,11 +49,8 @@ def self.{{ cons.name()|fn_name_rb }}({% call rb::arg_list_decl(cons) %}) {%- call rb::coerce_args_extra_indent(cons) %} # Call the (fallible) function before creating any half-baked object instances. # Lightly yucky way to bypass the usual "initialize" logic - # and just create a new instance with the required handle. - inst = allocate - inst.instance_variable_set :@handle, {% call rb::to_ffi_call(cons) %} - - return inst + # and just create a new instance with the required pointer. + return _uniffi_allocate({% call rb::to_ffi_call(cons) %}) end {% endfor %} @@ -39,14 +60,14 @@ def self.{{ cons.name()|fn_name_rb }}({% call rb::arg_list_decl(cons) %}) {%- when Some with (return_type) -%} def {{ meth.name()|fn_name_rb }}({% call rb::arg_list_decl(meth) %}) {%- call rb::coerce_args_extra_indent(meth) %} - result = {% call rb::to_ffi_call_with_prefix("@handle", meth) %} + result = {% call rb::to_ffi_call_with_prefix("@pointer", meth) %} return {{ "result"|lift_rb(return_type) }} end {%- when None -%} def {{ meth.name()|fn_name_rb }}({% call rb::arg_list_decl(meth) %}) {%- call rb::coerce_args_extra_indent(meth) %} - {% call rb::to_ffi_call_with_prefix("@handle", meth) %} + {% call rb::to_ffi_call_with_prefix("@pointer", meth) %} end {% endmatch %} {% endfor %} diff --git a/uniffi_bindgen/src/bindings/ruby/templates/RustBufferBuilder.rb b/uniffi_bindgen/src/bindings/ruby/templates/RustBufferBuilder.rb index 530b833580..0a1ac079f5 100644 --- a/uniffi_bindgen/src/bindings/ruby/templates/RustBufferBuilder.rb +++ b/uniffi_bindgen/src/bindings/ruby/templates/RustBufferBuilder.rb @@ -123,10 +123,10 @@ def write_{{ canonical_type_name }} {% when Type::Object with (object_name) -%} # The Object type {{ object_name }}. - # Objects cannot currently be serialized, but we can produce a helpful error. - def write_{{ canonical_type_name }} - raise InternalError('RustBufferStream.write() not implemented yet for {{ canonical_type_name }}') + def write_{{ canonical_type_name }}(obj) + pointer = {{ object_name|class_name_rb}}._uniffi_lower obj + pack_into(8, 'Q>', pointer.address) end {% when Type::CallbackInterface with (object_name) -%} diff --git a/uniffi_bindgen/src/bindings/ruby/templates/RustBufferStream.rb b/uniffi_bindgen/src/bindings/ruby/templates/RustBufferStream.rb index 9e6e700398..d4bef0c61d 100644 --- a/uniffi_bindgen/src/bindings/ruby/templates/RustBufferStream.rb +++ b/uniffi_bindgen/src/bindings/ruby/templates/RustBufferStream.rb @@ -124,10 +124,10 @@ def read{{ canonical_type_name }} {% when Type::Object with (object_name) -%} # The Object type {{ object_name }}. - # Objects cannot currently be serialized, but we can produce a helpful error. def read{{ canonical_type_name }} - raise InternalError, 'RustBufferStream.read not implemented yet for {{ canonical_type_name }}' + pointer = FFI::Pointer.new unpack_from 8, 'Q>' + return {{ object_name|class_name_rb }}._uniffi_allocate(pointer) end {% when Type::CallbackInterface with (object_name) -%} diff --git a/uniffi_bindgen/src/bindings/swift/gen_swift.rs b/uniffi_bindgen/src/bindings/swift/gen_swift.rs index 6adde71a98..8d0f264e67 100644 --- a/uniffi_bindgen/src/bindings/swift/gen_swift.rs +++ b/uniffi_bindgen/src/bindings/swift/gen_swift.rs @@ -166,6 +166,7 @@ mod filters { FFIType::Float32 => "float".into(), FFIType::Float64 => "double".into(), FFIType::RustCString => "const char*_Nonnull".into(), + FFIType::RustArcPtr => "void*_Nonnull".into(), FFIType::RustBuffer => "RustBuffer".into(), FFIType::RustError => "NativeRustError".into(), FFIType::ForeignBytes => "ForeignBytes".into(), diff --git a/uniffi_bindgen/src/bindings/swift/templates/ErrorTemplate.swift b/uniffi_bindgen/src/bindings/swift/templates/ErrorTemplate.swift index 4532fce681..0ad77b379f 100644 --- a/uniffi_bindgen/src/bindings/swift/templates/ErrorTemplate.swift +++ b/uniffi_bindgen/src/bindings/swift/templates/ErrorTemplate.swift @@ -16,6 +16,7 @@ fileprivate enum UniffiInternalError: RustError { case incompleteData case unexpectedOptionalTag case unexpectedEnumCase + case unexpectedNullPointer case emptyResult case unknown(_ message: String) @@ -25,6 +26,7 @@ fileprivate enum UniffiInternalError: RustError { case .incompleteData: return "The buffer still has data after lifting its containing value" case .unexpectedOptionalTag: return "Unexpected optional tag; should be 0 or 1" case .unexpectedEnumCase: return "Raw enum value doesn't match any cases" + case .unexpectedNullPointer: return "Raw pointer value was null" case .emptyResult: return "Unexpected nil returned from FFI function" case let .unknown(message): return "FFI function returned unknown error: \(message)" } diff --git a/uniffi_bindgen/src/bindings/swift/templates/ObjectTemplate.swift b/uniffi_bindgen/src/bindings/swift/templates/ObjectTemplate.swift index 36ddcbb8b3..60680ce9fc 100644 --- a/uniffi_bindgen/src/bindings/swift/templates/ObjectTemplate.swift +++ b/uniffi_bindgen/src/bindings/swift/templates/ObjectTemplate.swift @@ -9,30 +9,43 @@ public protocol {{ obj.name() }}Protocol { {% endfor %} } -public class {{ obj.name()|class_name_swift }}: {{ obj.name() }}Protocol { - private let handle: UInt64 +public class {{ obj.name()|class_name_swift }}: {{ obj.name() }}Protocol, Equatable, Hashable { + private let pointer: UnsafeMutableRawPointer - private init(fromRawHandle handle: UInt64) { - self.handle = handle + // TODO: We'd like this to be `private` but for Swifty reasons, + // we can't implement `ViaFfi` without making this `required` and we can't + // make it `required` without making it `public`. + required init(unsafeFromRawPointer pointer: UnsafeMutableRawPointer) { + self.pointer = pointer } {%- match obj.primary_constructor() %} {%- when Some with (cons) %} public convenience init({% call swift::arg_list_decl(cons) -%}) {% call swift::throws(cons) %} { - self.init(fromRawHandle: {% call swift::to_ffi_call(cons) %}) + self.init(unsafeFromRawPointer: {% call swift::to_ffi_call(cons) %}) } {%- when None %} {%- endmatch %} deinit { try! rustCall(UniffiInternalError.unknown("deinit")) { err in - {{ obj.ffi_object_free().name() }}(handle, err) + {{ obj.ffi_object_free().name() }}(pointer, err) } } + // Object equality is pointer identity. + // TODO: perhaps we should delegate to Rust for this sort of thing? + public static func ==(lhs: {{ obj.name()|class_name_swift }}, rhs: {{ obj.name()|class_name_swift }}) -> Bool { + return lhs.pointer == rhs.pointer + } + + public func hash(into hasher: inout Hasher) { + hasher.combine(self.pointer) + } + {% for cons in obj.alternate_constructors() %} public static func {{ cons.name()|fn_name_swift }}({% call swift::arg_list_decl(cons) %}) {% call swift::throws(cons) %} -> {{ obj.name()|class_name_swift }} { - return {{ obj.name()|class_name_swift }}(fromRawHandle: {% call swift::to_ffi_call(cons) %}) + return {{ obj.name()|class_name_swift }}(unsafeFromRawPointer: {% call swift::to_ffi_call(cons) %}) } {% endfor %} @@ -42,14 +55,45 @@ public class {{ obj.name()|class_name_swift }}: {{ obj.name() }}Protocol { {%- when Some with (return_type) -%} public func {{ meth.name()|fn_name_swift }}({% call swift::arg_list_decl(meth) %}) {% call swift::throws(meth) %} -> {{ return_type|type_swift }} { - let _retval = {% call swift::to_ffi_call_with_prefix("self.handle", meth) %} + let _retval = {% call swift::to_ffi_call_with_prefix("self.pointer", meth) %} return {% call swift::try(meth) %} {{ "_retval"|lift_swift(return_type) }} } {%- when None -%} public func {{ meth.name()|fn_name_swift }}({% call swift::arg_list_decl(meth) %}) {% call swift::throws(meth) %} { - {% call swift::to_ffi_call_with_prefix("self.handle", meth) %} + {% call swift::to_ffi_call_with_prefix("self.pointer", meth) %} } {%- endmatch %} {% endfor %} -} \ No newline at end of file +} + +fileprivate extension {{ obj.name()|class_name_swift }} { + fileprivate typealias FfiType = UnsafeMutableRawPointer + + fileprivate static func read(from buf: Reader) throws -> Self { + let v: UInt64 = try buf.readInt() + // The Rust code won't compile if a pointer won't fit in a UInt64. + // We have to go via `UInt` because that's the thing that's the size of a pointer. + let ptr = UnsafeMutableRawPointer(bitPattern: UInt(truncatingIfNeeded: v)) + if (ptr == nil) { + throw UniffiInternalError.unexpectedNullPointer + } + return try self.lift(ptr!) + } + + fileprivate func write(into buf: Writer) { + // This fiddling is because `Int` is the thing that's the same size as a pointer. + // The Rust code won't compile if a pointer won't fit in a `UInt64`. + buf.writeInt(UInt64(bitPattern: Int64(Int(bitPattern: self.lower())))) + } + + fileprivate static func lift(_ pointer: UnsafeMutableRawPointer) throws -> Self { + return Self(unsafeFromRawPointer: pointer) + } + + fileprivate func lower() -> UnsafeMutableRawPointer { + return self.pointer + } +} + +extension {{ obj.name()|class_name_swift }} : ViaFfi, Serializable {} diff --git a/uniffi_bindgen/src/interface/attributes.rs b/uniffi_bindgen/src/interface/attributes.rs index 7ca005c3f8..7e599c04bd 100644 --- a/uniffi_bindgen/src/interface/attributes.rs +++ b/uniffi_bindgen/src/interface/attributes.rs @@ -330,7 +330,7 @@ impl MethodAttributes { }) } - pub(super) fn get_by_arc(&self) -> bool { + pub(super) fn get_self_by_arc(&self) -> bool { self.0 .iter() .any(|attr| matches!(attr, Attribute::SelfType(SelfType::ByArc))) @@ -525,23 +525,23 @@ mod test { fn test_method_attributes() { let (_, node) = weedle::attribute::ExtendedAttributeList::parse("[Throws=Error]").unwrap(); let attrs = MethodAttributes::try_from(&node).unwrap(); - assert!(!attrs.get_by_arc()); + assert!(!attrs.get_self_by_arc()); assert!(matches!(attrs.get_throws_err(), Some("Error"))); let (_, node) = weedle::attribute::ExtendedAttributeList::parse("[]").unwrap(); let attrs = MethodAttributes::try_from(&node).unwrap(); - assert!(!attrs.get_by_arc()); + assert!(!attrs.get_self_by_arc()); assert!(attrs.get_throws_err().is_none()); let (_, node) = weedle::attribute::ExtendedAttributeList::parse("[Self=ByArc, Throws=Error]").unwrap(); let attrs = MethodAttributes::try_from(&node).unwrap(); - assert!(attrs.get_by_arc()); + assert!(attrs.get_self_by_arc()); assert!(attrs.get_throws_err().is_some()); let (_, node) = weedle::attribute::ExtendedAttributeList::parse("[Self=ByArc]").unwrap(); let attrs = MethodAttributes::try_from(&node).unwrap(); - assert!(attrs.get_by_arc()); + assert!(attrs.get_self_by_arc()); assert!(attrs.get_throws_err().is_none()); } diff --git a/uniffi_bindgen/src/interface/enum_.rs b/uniffi_bindgen/src/interface/enum_.rs index e790f1e9cf..53418f13c0 100644 --- a/uniffi_bindgen/src/interface/enum_.rs +++ b/uniffi_bindgen/src/interface/enum_.rs @@ -99,6 +99,7 @@ impl Enum { pub fn name(&self) -> &str { &self.name } + pub fn variants(&self) -> Vec<&Variant> { self.variants.iter().collect() } @@ -106,6 +107,12 @@ impl Enum { pub fn is_flat(&self) -> bool { self.flat } + + pub fn contains_object_references(&self, ci: &ComponentInterface) -> bool { + // *sigh* at the clone here, the relationship between a ComponentInterace + // and its contained types could use a bit of a cleanup. + ci.type_contains_object_references(&Type::Enum(self.name.clone())) + } } // Note that we have two `APIConverter` impls here - one for the `enum` case diff --git a/uniffi_bindgen/src/interface/ffi.rs b/uniffi_bindgen/src/interface/ffi.rs index 24b4e24380..b84c99b4c7 100644 --- a/uniffi_bindgen/src/interface/ffi.rs +++ b/uniffi_bindgen/src/interface/ffi.rs @@ -35,6 +35,10 @@ pub enum FFIType { /// If you've got one of these, you must call the appropriate rust function to free it. /// This is currently only used for error messages, and may go away in future. RustCString, + /// A `*const c_void` pointer to a rust-owned `Arc`. + /// If you've got one of these, you must call the appropriate rust function to free it. + /// The templates will generate a unique `free` function for each T. + RustArcPtr, /// A byte buffer allocated by rust, and owned by whoever currently holds it. /// If you've got one of these, you must either call the appropriate rust function to free it /// or pass it to someone that will. diff --git a/uniffi_bindgen/src/interface/function.rs b/uniffi_bindgen/src/interface/function.rs index 056534fd5f..1954164f3a 100644 --- a/uniffi_bindgen/src/interface/function.rs +++ b/uniffi_bindgen/src/interface/function.rs @@ -61,12 +61,19 @@ impl Function { pub fn name(&self) -> &str { &self.name } + pub fn arguments(&self) -> Vec<&Argument> { self.arguments.iter().collect() } + + pub fn all_arguments(&self) -> Vec { + self.arguments.to_vec() + } + pub fn return_type(&self) -> Option<&Type> { self.return_type.as_ref() } + pub fn ffi_func(&self) -> &FFIFunction { &self.ffi_func } @@ -112,9 +119,6 @@ impl APIConverter for weedle::namespace::NamespaceMember<'_> { impl APIConverter for weedle::namespace::OperationNamespaceMember<'_> { fn convert(&self, ci: &mut ComponentInterface) -> Result { let return_type = ci.resolve_return_type_expression(&self.return_type)?; - if let Some(Type::Object(_)) = return_type { - bail!("Objects cannot currently be returned from functions"); - } Ok(Function { name: match self.identifier { None => bail!("anonymous functions are not supported {:?}", self), @@ -176,9 +180,6 @@ impl APIConverter for weedle::argument::Argument<'_> { impl APIConverter for weedle::argument::SingleArgument<'_> { fn convert(&self, ci: &mut ComponentInterface) -> Result { let type_ = ci.resolve_type_expression(&self.type_)?; - if let Type::Object(_) = type_ { - bail!("Objects cannot currently be passed as arguments"); - } let default = match self.default { None => None, Some(v) => Some(convert_default_value(&v.value, &type_)?), diff --git a/uniffi_bindgen/src/interface/mod.rs b/uniffi_bindgen/src/interface/mod.rs index 28ecc60c5b..9d97dd003e 100644 --- a/uniffi_bindgen/src/interface/mod.rs +++ b/uniffi_bindgen/src/interface/mod.rs @@ -204,10 +204,43 @@ impl<'ci> ComponentInterface { self.errors.iter().find(|e| e.name == name) } + /// Iterate over all known types in the interface. pub fn iter_types(&self) -> Vec { self.types.iter_known_types().collect() } + /// Check whether the given type contains any (possibly nested) Type::Object references. + /// + /// This is important to know in language bindings that cannot integrate object types + /// tightly with the host GC, and hence need to perform manual destruction of objects. + pub fn type_contains_object_references(&self, type_: &Type) -> bool { + match type_ { + Type::Object(_) => true, + Type::Optional(t) | Type::Sequence(t) | Type::Map(t) => { + self.type_contains_object_references(t) + } + Type::Record(name) => self + .get_record_definition(name) + .map(|rec| { + rec.fields() + .iter() + .any(|f| self.type_contains_object_references(&f.type_)) + }) + .unwrap_or(false), + Type::Enum(name) => self + .get_enum_definition(name) + .map(|e| { + e.variants().iter().any(|v| { + v.fields() + .iter() + .any(|f| self.type_contains_object_references(&f.type_)) + }) + }) + .unwrap_or(false), + _ => false, + } + } + /// Calculate a numeric checksum for this ComponentInterface. /// /// The checksum can be used to guard against accidentally using foreign-language bindings diff --git a/uniffi_bindgen/src/interface/object.rs b/uniffi_bindgen/src/interface/object.rs index ee69facef2..a95c5be2fa 100644 --- a/uniffi_bindgen/src/interface/object.rs +++ b/uniffi_bindgen/src/interface/object.rs @@ -95,6 +95,10 @@ impl Object { &self.name } + pub fn type_(&self) -> Type { + Type::Object(self.name.clone()) + } + pub fn constructors(&self) -> Vec<&Constructor> { self.constructors.iter().collect() } @@ -127,8 +131,8 @@ impl Object { pub fn derive_ffi_funcs(&mut self, ci_prefix: &str) -> Result<()> { self.ffi_func_free.name = format!("ffi_{}_{}_object_free", ci_prefix, self.name); self.ffi_func_free.arguments = vec![FFIArgument { - name: "handle".to_string(), - type_: FFIType::UInt64, + name: "ptr".to_string(), + type_: FFIType::RustArcPtr, }]; self.ffi_func_free.return_type = None; for cons in self.constructors.iter_mut() { @@ -198,7 +202,7 @@ impl APIConverter for weedle::InterfaceDefinition<'_> { // Represents a constructor for an object type. // -// In the FFI, this will be a function that returns a handle for an instance +// In the FFI, this will be a function that returns a pointer to an instance // of the corresponding object type. #[derive(Debug, Clone)] pub struct Constructor { @@ -217,6 +221,10 @@ impl Constructor { self.arguments.iter().collect() } + pub fn all_arguments(&self) -> Vec { + self.arguments.to_vec() + } + pub fn ffi_func(&self) -> &FFIFunction { &self.ffi_func } @@ -232,7 +240,7 @@ impl Constructor { self.ffi_func.name.push('_'); self.ffi_func.name.push_str(&self.name); self.ffi_func.arguments = self.arguments.iter().map(Into::into).collect(); - self.ffi_func.return_type = Some(FFIType::UInt64); + self.ffi_func.return_type = Some(FFIType::RustArcPtr); } fn is_primary_constructor(&self) -> bool { @@ -282,8 +290,8 @@ impl APIConverter for weedle::interface::ConstructorInterfaceMember // Represents an instance method for an object type. // -// The in FFI, this will be a function whose first argument is a handle for an -// instance of the corresponding object type. +// The FFI will represent this as a function whose first/self argument is a +// `FFIType::RustArcPtr` to the instance. #[derive(Debug, Clone)] pub struct Method { pub(super) name: String, @@ -313,22 +321,29 @@ impl Method { pub fn first_argument(&self) -> Argument { Argument { - name: "handle".to_string(), + name: "ptr".to_string(), // TODO: ideally we'd get this via `ci.resolve_type_expression` so that it // is contained in the proper `TypeUniverse`, but this works for now. type_: Type::Object(self.object_name.clone()), - by_ref: false, + by_ref: !self.attributes.get_self_by_arc(), optional: false, default: None, } } + pub fn all_arguments(&self) -> Vec { + vec![self.first_argument()] + .into_iter() + .chain(self.arguments.iter().cloned()) + .collect() + } + pub fn throws(&self) -> Option<&str> { self.attributes.get_throws_err() } pub fn takes_self_by_arc(&self) -> bool { - self.attributes.get_by_arc() + self.attributes.get_self_by_arc() } pub fn derive_ffi_func(&mut self, ci_prefix: &str, obj_prefix: &str) -> Result<()> { @@ -372,9 +387,6 @@ impl APIConverter for weedle::interface::OperationInterfaceMember<'_> { bail!("method modifiers are not supported") } let return_type = ci.resolve_return_type_expression(&self.return_type)?; - if let Some(Type::Object(_)) = return_type { - bail!("Objects cannot currently be returned from functions"); - } Ok(Method { name: match self.identifier { None => bail!("anonymous methods are not supported {:?}", self), diff --git a/uniffi_bindgen/src/interface/record.rs b/uniffi_bindgen/src/interface/record.rs index a51b92a18b..07b417f7e3 100644 --- a/uniffi_bindgen/src/interface/record.rs +++ b/uniffi_bindgen/src/interface/record.rs @@ -65,9 +65,16 @@ impl Record { pub fn name(&self) -> &str { &self.name } + pub fn fields(&self) -> Vec<&Field> { self.fields.iter().collect() } + + pub fn contains_object_references(&self, ci: &ComponentInterface) -> bool { + // *sigh* at the clone here, the relationship between a ComponentInterace + // and its contained types could use a bit of a cleanup. + ci.type_contains_object_references(&Type::Record(self.name.clone())) + } } impl APIConverter for weedle::DictionaryDefinition<'_> { diff --git a/uniffi_bindgen/src/interface/types/mod.rs b/uniffi_bindgen/src/interface/types/mod.rs index c3c2e2ca28..80bbb3a8db 100644 --- a/uniffi_bindgen/src/interface/types/mod.rs +++ b/uniffi_bindgen/src/interface/types/mod.rs @@ -133,8 +133,8 @@ impl From<&Type> for FFIType { // Strings are always owned rust values. // We might add a separate type for borrowed strings in future. Type::String => FFIType::RustBuffer, - // Objects are passed as opaque integer handles. - Type::Object(_) => FFIType::UInt64, + // Objects are pointers to an Arc<> + Type::Object(_) => FFIType::RustArcPtr, // Callback interfaces are passed as opaque integer handles. Type::CallbackInterface(_) => FFIType::UInt64, // Errors have their own special type. diff --git a/uniffi_bindgen/src/scaffolding.rs b/uniffi_bindgen/src/scaffolding.rs index 1534b29de5..445f7ed3c0 100644 --- a/uniffi_bindgen/src/scaffolding.rs +++ b/uniffi_bindgen/src/scaffolding.rs @@ -42,9 +42,8 @@ mod filters { Type::String => "String".into(), Type::Timestamp => "std::time::SystemTime".into(), Type::Duration => "std::time::Duration".into(), - Type::Enum(name) | Type::Record(name) | Type::Object(name) | Type::Error(name) => { - name.clone() - } + Type::Enum(name) | Type::Record(name) | Type::Error(name) => name.clone(), + Type::Object(name) => format!("std::sync::Arc<{}>", name), Type::CallbackInterface(name) => format!("Box", name), Type::Optional(t) => format!("Option<{}>", type_rs(t)?), Type::Sequence(t) => format!("Vec<{}>", type_rs(t)?), @@ -65,6 +64,7 @@ mod filters { FFIType::Float32 => "f32".into(), FFIType::Float64 => "f64".into(), FFIType::RustCString => "*mut std::os::raw::c_char".into(), + FFIType::RustArcPtr => "*const std::os::raw::c_void".into(), FFIType::RustBuffer => "uniffi::RustBuffer".into(), FFIType::RustError => "uniffi::deps::ffi_support::ExternError".into(), FFIType::ForeignBytes => "uniffi::ForeignBytes".into(), diff --git a/uniffi_bindgen/src/templates/ObjectTemplate.rs b/uniffi_bindgen/src/templates/ObjectTemplate.rs index 8cd303a9f4..e5c8e19786 100644 --- a/uniffi_bindgen/src/templates/ObjectTemplate.rs +++ b/uniffi_bindgen/src/templates/ObjectTemplate.rs @@ -1,13 +1,14 @@ -// For each Object definition, we assume the caller has provided an appropriately-shaped `struct` -// with an `impl` for each method on the object. We create a `ConcurrentHandleMap` for safely handing -// out references to these structs to foreign language code, and we provide a `pub extern "C"` function +// For each Object definition, we assume the caller has provided an appropriately-shaped `struct T` +// with an `impl` for each method on the object. We create an `Arc` for "safely" handing out +// references to these structs to foreign language code, and we provide a `pub extern "C"` function // corresponding to each method. // +// (Note that "safely" is in "scare quotes" - that's because we use functions on an `Arc` that +// that are inherently unsafe, but the code we generate is safe in practice.) +// // If the caller's implementation of the struct does not match with the methods or types specified // in the UDL, then the rust compiler will complain with a (hopefully at least somewhat helpful!) // error message when processing this generated code. -{% let handle_map = format!("UNIFFI_HANDLE_MAP_{}", obj.name().to_uppercase()) -%} - {% if obj.uses_deprecated_threadsafe_attribute() %} // We want to mark this as `deprecated` - long story short, the only way to @@ -23,25 +24,27 @@ fn uniffi_note_threadsafe_deprecation_{{ obj.name() }}() {} {% endif %} -uniffi::deps::lazy_static::lazy_static! { - #[doc(hidden)] - static ref {{ handle_map }}: uniffi::ffi::handle_maps::ArcHandleMap<{{ obj.name() }}> - = Default::default(); -} - - {% let ffi_free = obj.ffi_object_free() -%} - #[doc(hidden)] - #[no_mangle] - pub extern "C" fn {{ ffi_free.name() }}(handle: u64) { - let _ = {{ handle_map }}.delete_u64(handle); +{% let ffi_free = obj.ffi_object_free() -%} +#[doc(hidden)] +#[no_mangle] +pub extern "C" fn {{ ffi_free.name() }}(ptr: *const std::os::raw::c_void) { + // We mustn't panic across the FFI, but also can't report it anywhere. + // The best we can do it catch, warn and ignore. + if let Err(e) = std::panic::catch_unwind(|| { + assert!(!ptr.is_null()); + {#- turn it into an Arc and explicitly drop it. #} + drop(unsafe { std::sync::Arc::from_raw(ptr as *const {{ obj.name() }}) }) + }) { + uniffi::deps::log::error!("{{ ffi_free.name() }} panicked: {:?}", e); } +} {%- for cons in obj.constructors() %} #[allow(clippy::all)] #[doc(hidden)] #[no_mangle] pub extern "C" fn {{ cons.ffi_func().name() }}( - {%- call rs::arg_list_ffi_decl(cons.ffi_func()) %}) -> u64 { + {%- call rs::arg_list_ffi_decl(cons.ffi_func()) %}) -> *const std::os::raw::c_void /* *const {{ obj.name() }} */ { uniffi::deps::log::debug!("{{ cons.ffi_func().name() }}"); {% if obj.uses_deprecated_threadsafe_attribute() %} uniffi_note_threadsafe_deprecation_{{ obj.name() }}(); diff --git a/uniffi_bindgen/src/templates/macros.rs b/uniffi_bindgen/src/templates/macros.rs index bc2e43cbfa..26527daf38 100644 --- a/uniffi_bindgen/src/templates/macros.rs +++ b/uniffi_bindgen/src/templates/macros.rs @@ -6,14 +6,8 @@ {{ func.name() }}({% call _arg_list_rs_call(func) -%}) {%- endmacro -%} -{%- macro to_rs_call_with_prefix(prefix, func) -%} - {{ func.name() }}( - {{- prefix }}{% if func.arguments().len() > 0 %}, {% call _arg_list_rs_call(func) -%}{% endif -%} -) -{%- endmacro -%} - {%- macro _arg_list_rs_call(func) %} - {%- for arg in func.arguments() %} + {%- for arg in func.all_arguments() %} {%- if arg.by_ref() %}&{% endif %} {{- arg.name()|lift_rs(arg.type_()) }} {%- if !loop.last %}, {% endif %} @@ -44,31 +38,37 @@ {% macro ret(func) %}{% match func.return_type() %}{% when Some with (return_type) %}{{ "_retval"|lower_rs(return_type) }}{% else %}_retval{% endmatch %}{% endmacro %} +{% macro construct(obj, cons) %} + {{- obj.name() }}::{% call to_rs_call(cons) -%} +{% endmacro %} + {% macro to_rs_constructor_call(obj, cons) %} {% match cons.throws() %} {% when Some with (e) %} -UNIFFI_HANDLE_MAP_{{ obj.name()|upper }}.insert_with_result(err, || -> Result<{{obj.name()}}, {{e}}> { - let _retval = {{ obj.name() }}::{% call to_rs_call(cons) %}?; - Ok(_retval) -}) + uniffi::deps::ffi_support::call_with_result(err, || -> Result<_, {{ e }}> { + let _new = {% call construct(obj, cons) %}?; + let _arc = std::sync::Arc::new(_new); + Ok({{ "_arc"|lower_rs(obj.type_()) }}) + }) {% else %} -UNIFFI_HANDLE_MAP_{{ obj.name()|upper }}.insert_with_output(err, || { - {{ obj.name() }}::{% call to_rs_call(cons) %} -}) + uniffi::deps::ffi_support::call_with_output(err, || { + let _new = {% call construct(obj, cons) %}; + let _arc = std::sync::Arc::new(_new); + {{ "_arc"|lower_rs(obj.type_()) }} + }) {% endmatch %} {% endmacro %} {% macro to_rs_method_call(obj, meth) -%} -{% let this_handle_map = format!("UNIFFI_HANDLE_MAP_{}", obj.name().to_uppercase()) -%} {% match meth.throws() -%} {% when Some with (e) -%} -{{ this_handle_map }}.call_{% if meth.takes_self_by_arc() %}by_arc_{% endif %}with_result(err, {{ meth.first_argument().name() }}, |obj| -> Result<{% call return_type_func(meth) %}, {{e}}> { - let _retval = {{ obj.name() }}::{%- call to_rs_call_with_prefix("obj", meth) -%}?; +uniffi::deps::ffi_support::call_with_result(err, || -> Result<{% call return_type_func(meth) %}, {{e}}> { + let _retval = {{ obj.name() }}::{% call to_rs_call(meth) %}?; Ok({% call ret(meth) %}) }) -{% else -%} -{{ this_handle_map }}.call_{% if meth.takes_self_by_arc() %}by_arc_{% endif %}with_output(err, {{ meth.first_argument().name() }}, |obj| { - let _retval = {{ obj.name() }}::{%- call to_rs_call_with_prefix("obj", meth) -%}; +{% else %} +uniffi::deps::ffi_support::call_with_output(err, || { + let _retval = {{ obj.name() }}::{% call to_rs_call(meth) %}; {% call ret(meth) %} }) {% endmatch -%}