-
Notifications
You must be signed in to change notification settings - Fork 36
feat(uniffi): fully upgrade to 0.30 #322
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Major changes: - Update workspace dependencies to UniFFI 0.30.0 - Replace uniffi_bindgen::backend with uniffi_bindgen::interface - Replace FfiType::RustArcPtr with FfiType::Handle (u64) - Fix toml::value::Value type compatibility - Update templates: takes_self() -> self_type().is_some() - Update C++ type mapping for Handle (void* -> uint64_t) Remaining work: - Fix Method construction (no more MethodMetadata.into()) - Handle new UniffiTrait::Ord variant - Fix Literal type mismatches - Update C++ includes (RustArcPtr.h) - Regenerate fixtures for new trait interface - Run full test suite See UniFFI 0.30 changelog for breaking changes.
- Updated uniffi, uniffi_bindgen, uniffi_meta dependencies to 0.30.0 - Fixed uniffi_bindgen::backend -> uniffi_bindgen::interface migration - Replaced FfiType::RustArcPtr with FfiType::Handle (u64 handles) - Updated Callable.takes_self() -> Callable.self_type().is_some() - Fixed toml dependency version conflicts - Added UniffiTrait::Ord pattern match - Updated FfiFunction construction for private fields - Fixed DefaultValueMetadata handling in render_literal - Updated C++ and TypeScript FFI type mappings for u64 handles
- Removed duplicate Handle pattern matches in gen_rust/mod.rs - Fixed toml::value::Value import path consistency - Cleaned up unreachable pattern warnings
|
@Larkooo Wow, thank you so much. I was/am rather dreading this upgrade. Is there anything you need from me while you chug through this? I have enabled this PR to run on CI in the mean time. |
|
Nope all good, will try to get this ready. |
- Fix callback functions to return correct types (uint64_t for clone callbacks) - Fix lambda return type declarations to match callback signatures - Add proper return value handling when rsLambda is null - Fix free functions to pass uint64_t directly instead of void* cast - Add makeCallbackFunction support for ForeignFuture callbacks Fixes three UniFFI 0.30 C++ codegen issues: 1. Callback return type mismatch (void vs uint64_t) 2. Missing ForeignFutureDroppedCallback namespace 3. Free function wrong signature (void* vs uint64_t)
- Build TypeScript sources to typescript/dist - Add dist to git for GitHub package usage - Allows using as GitHub dependency in pnpm/npm - Previously dist was gitignored
jhugman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you're still not done with this yet, so this is a drive by review. Thank you so much for all the work you have put in so far.
There is a temptation to refactor in order to understand code. I'm happy to see that in different PRs; for this upgrade/version bump, I think we should be aiming to make it as tight as possible. What do you think?
| pub trait SimCard: Send + Sync { | ||
| fn name(&self) -> String; | ||
|
|
||
| #[doc(hidden)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file comes verbatim from https://github.com/mozilla/uniffi-rs/blob/main/examples/callbacks/src/lib.rs
The uniffi_foreign_handle looks like it's a leak of a detail in your implementation. Is there a way to avoid this?
| using CallInvoker = uniffi_runtime::UniffiCallInvoker; | ||
|
|
||
| {%- for type_suffix in ["U8", "I8", "U16", "I16", "U32", "I32", "U64", "I64", "F32", "F64"] %} | ||
| {%- let struct_name = "UniffiForeignFutureResult" ~ type_suffix %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woah. TIL.
| {%- let cb_name = callback.name()|ffi_callback_name %} | ||
| {%- let guard_name = cb_name|fmt("BRIDGING_{}") %} | ||
| #ifndef {{ guard_name }}_DEFINED | ||
| #define {{ guard_name }}_DEFINED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have added guards in a lot of files here.
- Are they newly needed? What has changed from prior releases?
- Were they needed before, and you're adding them because they're a best practice? How was it working before?
As you may have worked out, C++ isn't my strongest language. :)
| operator UniffiRustFutureContinuationCallback() const { return callback; } | ||
| }; | ||
|
|
||
| template <> struct Bridging<UniffiRustFutureContinuationCallback{{ future_ns|capitalize }}Wrapper> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: extract UniffiRustFutureContinuationCallback{{ future_ns|capitalize }}Wrapper into a variable, and use it in the multiple places you're using it now.
| rt, | ||
| callInvoker, | ||
| value | ||
| return UniffiRustFutureContinuationCallback{{ future_ns|capitalize }}Wrapper( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused about the need for this wrapper.
| pub(crate) fn ffi_type_label_for_cpp(&self, ffi_type: &FfiType) -> String { | ||
| match ffi_type { | ||
| FfiType::RustArcPtr(_) => "UniffiRustArcPtr".to_string(), | ||
| FfiType::Handle => "uint64_t".to_string(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I think this should be bigint.
The method name is confusing: we're in the gen_typescript module, so this is the type—used in typescript—that corresponds to a type in cpp.
I think it could be renamed to ffi_type_label, but I'm sure there is another method of that name. But let's not do that in this PR.
| // Create FfiFunction for the bless pointer function | ||
| // In UniFFI 0.30, objects use u64 handles instead of raw pointers | ||
| // We use the default() builder pattern since fields are private | ||
| let mut ffi_func = FfiFunction::default(); | ||
| ffi_func.rename(format!("ffi_{}__bless_pointer", self.name())); | ||
| // Note: We can't set arguments/return_type/flags directly in 0.30 | ||
| // This is a limitation - the bless_pointer function may not work correctly | ||
| // TODO: Find proper API or file issue with uniffi-bindgen-react-native | ||
| ffi_func |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀 😟
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bless pointer method happens between the Typescript and C++, but doesn't go as far as Rust.
Pointer blessing (thanks Perl for the nomenclature) is a way of tying the hermes' garbage collection into Rust's reference counting.
If you can't get this running, then I think file a bug with mozilla/uniffi-rs to get FfiFunction to derive a builder.
|
|
||
| pub fn cpp_module(&self) -> String { | ||
| format!("Native{}", self.namespace.to_upper_camel_case()) | ||
| // Explicitly capitalize the first letter to ensure proper casing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand this. Wasn't heck doing the right thing here?
| serde = { workspace = true } | ||
| textwrap = "0.16.1" | ||
| toml = "0.5" | ||
| toml = { workspace = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice.
| uniffi_meta = "=0.29.3" | ||
| uniffi = "=0.30.0" | ||
| uniffi_bindgen = "=0.30.0" | ||
| uniffi_meta = "=0.30.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😅
|
Awesome to see someone working on this @Larkooo ! It seems to mostly work in my project, with the addition of a few fixes I pushed here https://github.com/spacebear21/uniffi-bindgen-react-native/commits/update-uniffi-0.30-wasm/. Please let me know if you need any help with testing or review, I'd love to see 0.30 support added to this library. |
No description provided.