From 2b05c3d14fa51439c0489caaf821d2c764f8f8d9 Mon Sep 17 00:00:00 2001 From: Ben Dean-Kawamura Date: Thu, 31 Oct 2024 16:04:43 -0400 Subject: [PATCH] Fix some more Swift concurrency issues Converted some more vars to lets. Swift still complains about async calls. I believe the next step would be to make all UniFFI objects conform to `Sendable`. Split the `FfiType::Reference` variant into `Reference` and `MutReference`. This allows us to define some more variables using `let` in swift. See #2279. --- CHANGELOG.md | 4 ++++ uniffi_bindgen/src/bindings/kotlin/gen_kotlin/mod.rs | 4 +++- uniffi_bindgen/src/bindings/python/gen_python/mod.rs | 4 +++- uniffi_bindgen/src/bindings/ruby/gen_ruby/mod.rs | 2 +- uniffi_bindgen/src/bindings/swift/gen_swift/mod.rs | 8 +++++++- uniffi_bindgen/src/bindings/swift/templates/Async.swift | 2 +- .../bindings/swift/templates/CallbackInterfaceImpl.swift | 9 ++++++--- .../swift/templates/CallbackInterfaceTemplate.swift | 2 +- .../src/bindings/swift/templates/HandleMap.swift | 5 +++-- .../src/bindings/swift/templates/ObjectTemplate.swift | 2 +- uniffi_bindgen/src/bindings/swift/test.rs | 2 ++ uniffi_bindgen/src/interface/callbacks.rs | 6 ++++-- uniffi_bindgen/src/interface/ffi.rs | 8 +++++++- 13 files changed, 43 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 71405bf7d1..aa2918c4d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,10 @@ ### What's changed? - Switching jinja template engine from askama to rinja. +- +### ⚠️ Breaking Changes for external bindings authors ⚠️ + +- Added the `FfiType::MutReference` variant. ## v0.28.2 (backend crates: v0.28.2) - (_2024-10-08_) diff --git a/uniffi_bindgen/src/bindings/kotlin/gen_kotlin/mod.rs b/uniffi_bindgen/src/bindings/kotlin/gen_kotlin/mod.rs index bf5178a557..61ea737575 100644 --- a/uniffi_bindgen/src/bindings/kotlin/gen_kotlin/mod.rs +++ b/uniffi_bindgen/src/bindings/kotlin/gen_kotlin/mod.rs @@ -439,7 +439,9 @@ impl KotlinCodeOracle { FfiType::ForeignBytes => "ForeignBytes.ByValue".to_string(), FfiType::Callback(name) => self.ffi_callback_name(name), FfiType::Struct(name) => self.ffi_struct_name(name), - FfiType::Reference(inner) => self.ffi_type_label_by_reference(inner), + FfiType::Reference(inner) | FfiType::MutReference(inner) => { + self.ffi_type_label_by_reference(inner) + } FfiType::VoidPointer => "Pointer".to_string(), } } diff --git a/uniffi_bindgen/src/bindings/python/gen_python/mod.rs b/uniffi_bindgen/src/bindings/python/gen_python/mod.rs index c083ecf046..078f29e7e3 100644 --- a/uniffi_bindgen/src/bindings/python/gen_python/mod.rs +++ b/uniffi_bindgen/src/bindings/python/gen_python/mod.rs @@ -401,7 +401,9 @@ impl PythonCodeOracle { FfiType::Callback(name) => self.ffi_callback_name(name), FfiType::Struct(name) => self.ffi_struct_name(name), // Pointer to an `asyncio.EventLoop` instance - FfiType::Reference(inner) => format!("ctypes.POINTER({})", self.ffi_type_label(inner)), + FfiType::Reference(inner) | FfiType::MutReference(inner) => { + format!("ctypes.POINTER({})", self.ffi_type_label(inner)) + } FfiType::VoidPointer => "ctypes.c_void_p".to_string(), } } diff --git a/uniffi_bindgen/src/bindings/ruby/gen_ruby/mod.rs b/uniffi_bindgen/src/bindings/ruby/gen_ruby/mod.rs index 7fceb3ac1b..6a58ae513e 100644 --- a/uniffi_bindgen/src/bindings/ruby/gen_ruby/mod.rs +++ b/uniffi_bindgen/src/bindings/ruby/gen_ruby/mod.rs @@ -139,7 +139,7 @@ mod filters { // definitions use references. Those FFI functions aren't actually used, so we just // pick something that runs and makes some sense. Revisit this once the references // are actually implemented. - FfiType::Reference(_) => ":pointer".to_string(), + FfiType::Reference(_) | FfiType::MutReference(_) => ":pointer".to_string(), FfiType::VoidPointer => ":pointer".to_string(), FfiType::Struct(_) => { unimplemented!("Structs are not implemented") diff --git a/uniffi_bindgen/src/bindings/swift/gen_swift/mod.rs b/uniffi_bindgen/src/bindings/swift/gen_swift/mod.rs index 5afaa0598d..93b353a08d 100644 --- a/uniffi_bindgen/src/bindings/swift/gen_swift/mod.rs +++ b/uniffi_bindgen/src/bindings/swift/gen_swift/mod.rs @@ -560,6 +560,9 @@ impl SwiftCodeOracle { FfiType::Callback(name) => format!("@escaping {}", self.ffi_callback_name(name)), FfiType::Struct(name) => self.ffi_struct_name(name), FfiType::Reference(inner) => { + format!("UnsafePointer<{}>", self.ffi_type_label(inner)) + } + FfiType::MutReference(inner) => { format!("UnsafeMutablePointer<{}>", self.ffi_type_label(inner)) } FfiType::VoidPointer => "UnsafeMutableRawPointer".into(), @@ -708,7 +711,10 @@ pub mod filters { format!("{} _Nonnull", SwiftCodeOracle.ffi_callback_name(name)) } FfiType::Struct(name) => SwiftCodeOracle.ffi_struct_name(name), - FfiType::Reference(inner) => format!("{}* _Nonnull", header_ffi_type_name(inner)?), + FfiType::Reference(inner) => { + format!("const {}* _Nonnull", header_ffi_type_name(inner)?) + } + FfiType::MutReference(inner) => format!("{}* _Nonnull", header_ffi_type_name(inner)?), FfiType::VoidPointer => "void* _Nonnull".into(), }) } diff --git a/uniffi_bindgen/src/bindings/swift/templates/Async.swift b/uniffi_bindgen/src/bindings/swift/templates/Async.swift index c26518783f..2d88afb9f0 100644 --- a/uniffi_bindgen/src/bindings/swift/templates/Async.swift +++ b/uniffi_bindgen/src/bindings/swift/templates/Async.swift @@ -84,7 +84,7 @@ private func uniffiTraitInterfaceCallAsyncWithError( // Borrow the callback handle map implementation to store foreign future handles // TODO: consolidate the handle-map code (https://github.com/mozilla/uniffi-rs/pull/1823) -fileprivate var UNIFFI_FOREIGN_FUTURE_HANDLE_MAP = UniffiHandleMap() +fileprivate let UNIFFI_FOREIGN_FUTURE_HANDLE_MAP = UniffiHandleMap() // Protocol for tasks that handle foreign futures. // diff --git a/uniffi_bindgen/src/bindings/swift/templates/CallbackInterfaceImpl.swift b/uniffi_bindgen/src/bindings/swift/templates/CallbackInterfaceImpl.swift index 2f4a12761d..124433e4a9 100644 --- a/uniffi_bindgen/src/bindings/swift/templates/CallbackInterfaceImpl.swift +++ b/uniffi_bindgen/src/bindings/swift/templates/CallbackInterfaceImpl.swift @@ -6,7 +6,10 @@ fileprivate struct {{ trait_impl }} { // Create the VTable using a series of closures. // Swift automatically converts these into C callback functions. - static var vtable: {{ vtable|ffi_type_name }} = {{ vtable|ffi_type_name }}( + // + // This creates 1-element array, since this seems to be the only way to construct a const + // pointer that we can pass to the Rust code. + static let vtable: [{{ vtable|ffi_type_name }}] = [{{ vtable|ffi_type_name }}( {%- for (ffi_callback, meth) in vtable_methods %} {{ meth.name()|fn_name }}: { ( {%- for arg in ffi_callback.arguments() %} @@ -101,9 +104,9 @@ fileprivate struct {{ trait_impl }} { print("Uniffi callback interface {{ name }}: handle missing in uniffiFree") } } - ) + )] } private func {{ callback_init }}() { - {{ ffi_init_callback.name() }}(&{{ trait_impl }}.vtable) + {{ ffi_init_callback.name() }}({{ trait_impl }}.vtable) } diff --git a/uniffi_bindgen/src/bindings/swift/templates/CallbackInterfaceTemplate.swift b/uniffi_bindgen/src/bindings/swift/templates/CallbackInterfaceTemplate.swift index 2ea0d35a5f..b13a3aee33 100644 --- a/uniffi_bindgen/src/bindings/swift/templates/CallbackInterfaceTemplate.swift +++ b/uniffi_bindgen/src/bindings/swift/templates/CallbackInterfaceTemplate.swift @@ -16,7 +16,7 @@ @_documentation(visibility: private) #endif fileprivate struct {{ ffi_converter_name }} { - fileprivate static var handleMap = UniffiHandleMap<{{ type_name }}>() + fileprivate static let handleMap = UniffiHandleMap<{{ type_name }}>() } #if swift(>=5.8) diff --git a/uniffi_bindgen/src/bindings/swift/templates/HandleMap.swift b/uniffi_bindgen/src/bindings/swift/templates/HandleMap.swift index 6de9f085d6..e4ef8ecb55 100644 --- a/uniffi_bindgen/src/bindings/swift/templates/HandleMap.swift +++ b/uniffi_bindgen/src/bindings/swift/templates/HandleMap.swift @@ -1,6 +1,7 @@ -fileprivate class UniffiHandleMap { - private var map: [UInt64: T] = [:] +fileprivate final class UniffiHandleMap: @unchecked Sendable { + // All mutation happens with this lock held, which is why we implement @unchecked Sendable. private let lock = NSLock() + private var map: [UInt64: T] = [:] private var currentHandle: UInt64 = 1 func insert(obj: T) -> UInt64 { diff --git a/uniffi_bindgen/src/bindings/swift/templates/ObjectTemplate.swift b/uniffi_bindgen/src/bindings/swift/templates/ObjectTemplate.swift index efeb4fe8fc..2fe22186be 100644 --- a/uniffi_bindgen/src/bindings/swift/templates/ObjectTemplate.swift +++ b/uniffi_bindgen/src/bindings/swift/templates/ObjectTemplate.swift @@ -132,7 +132,7 @@ open class {{ impl_class_name }}: #endif public struct {{ ffi_converter_name }}: FfiConverter { {%- if obj.has_callback_interface() %} - fileprivate static var handleMap = UniffiHandleMap<{{ type_name }}>() + fileprivate static let handleMap = UniffiHandleMap<{{ type_name }}>() {%- endif %} typealias FfiType = UnsafeMutableRawPointer diff --git a/uniffi_bindgen/src/bindings/swift/test.rs b/uniffi_bindgen/src/bindings/swift/test.rs index 9a2a5a7de9..afb5a6bd52 100644 --- a/uniffi_bindgen/src/bindings/swift/test.rs +++ b/uniffi_bindgen/src/bindings/swift/test.rs @@ -96,6 +96,8 @@ fn compile_swift_module>( command .current_dir(out_dir) .arg("-emit-module") + // TODO(2279): Fix concurrency issues and uncomment this + //.arg("-strict-concurrency=complete") .arg("-module-name") .arg(module_name) .arg("-o") diff --git a/uniffi_bindgen/src/interface/callbacks.rs b/uniffi_bindgen/src/interface/callbacks.rs index b0e978e8b4..c864a79566 100644 --- a/uniffi_bindgen/src/interface/callbacks.rs +++ b/uniffi_bindgen/src/interface/callbacks.rs @@ -153,7 +153,9 @@ pub fn method_ffi_callback(trait_name: &str, method: &Method, index: usize) -> F arguments: iter::once(FfiArgument::new("uniffi_handle", FfiType::UInt64)) .chain(method.arguments().into_iter().map(Into::into)) .chain(iter::once(match method.return_type() { - Some(t) => FfiArgument::new("uniffi_out_return", FfiType::from(t).reference()), + Some(t) => { + FfiArgument::new("uniffi_out_return", FfiType::from(t).mut_reference()) + } None => FfiArgument::new("uniffi_out_return", FfiType::VoidPointer), })) .collect(), @@ -175,7 +177,7 @@ pub fn method_ffi_callback(trait_name: &str, method: &Method, index: usize) -> F FfiArgument::new("uniffi_callback_data", FfiType::UInt64), FfiArgument::new( "uniffi_out_return", - FfiType::Struct("ForeignFuture".to_owned()).reference(), + FfiType::Struct("ForeignFuture".to_owned()).mut_reference(), ), ]) .collect(), diff --git a/uniffi_bindgen/src/interface/ffi.rs b/uniffi_bindgen/src/interface/ffi.rs index ab2e8eb6bd..c406dd5ee3 100644 --- a/uniffi_bindgen/src/interface/ffi.rs +++ b/uniffi_bindgen/src/interface/ffi.rs @@ -58,8 +58,10 @@ pub enum FfiType { /// These are used to pass objects across the FFI. Handle, RustCallStatus, - /// Pointer to an FfiType. + /// Const pointer to an FfiType. Reference(Box), + /// Mutable pointer to an FfiType. + MutReference(Box), /// Opaque pointer VoidPointer, } @@ -69,6 +71,10 @@ impl FfiType { FfiType::Reference(Box::new(self)) } + pub fn mut_reference(self) -> FfiType { + FfiType::MutReference(Box::new(self)) + } + /// Unique name for an FFI return type pub fn return_type_name(return_type: Option<&FfiType>) -> String { match return_type {