Support uniffi 0.32 - #149
Conversation
c1f364e to
9583902
Compare
|
Pushed two commits addressing the 0.32-upgrade CI failures:
Verified on a fork run with Actions enabled: Nix (all dev shells incl. msrv), Lints (1.91 / stable / nightly), and Test Suite (nightly) pass; compilation is clean on 1.91. Two failures remain that look like they need a maintainer call rather than a bindgen change:
Happy to help with either if that would be useful. |
|
@DenisovAV thanks for working on this, i think the downstream tests should be bumped in this PR, but the flakey test needs it's own PR, i think there's an issue for it |
|
Thanks @chavic. Addressed both of your points, plus a codegen fix the downstream turned up: Downstream (
Flaky CI on this branch is otherwise green (Nix, Lints on 1.91/stable/nightly, Test Suite on nightly; the fixture suite incl. the new |
|
Hi @chavic — gentle ping on this when you get a chance 🙂 Both your review points are addressed above: the downstream bump is probed (payjoin generates + runs on 0.32, 24 tests green — which also surfaced a real Since you noted #149 lands first with #152 rebased on top, this one gates the chain — happy to rebase #152 the moment it merges. No rush, just flagging it's ready for another look. Thanks! |
Clippy 1.97 extended useless_borrows_in_formatting to flag these five borrows, and CI runners now ship stable 1.97.1, so the Lints (stable) job fails on main (see the run on #156, which flags these exact lines without touching them). Verified: clippy 1.98.0 reports five 'redundant reference' errors on main and none with this change. Display for &T forwards to Display for T, so output is byte-identical. The same change rode along in #149, #152, #157 (and was reverted from #150 when it could not be reproduced locally on an outdated toolchain). Landing it once on main lets those branches rebase clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDnKiUL8NDSpeaPvyKJeoR
992c32e to
336da8c
Compare
|
Hi @chavic — thanks for merging #157! An update on the downstream tests here, and a question for you. I dug into why both jobs fail against this PR's uniffi 0.32, and it's a genuine chicken-and-egg rather than something I can cleanly fix here:
Neither can be generated against 0.32 until its upstream migrates, and neither has a 0.32 branch yet. Downstreams can't move to 0.32 until there's a 0.32 uniffi-dart release — which this PR is what enables. Forcing them green would mean patching pinned upstream crates in CI (testing a Frankenstein, not the real consumer), which I don't think is worth doing. Worth noting: a real 0.32-native consumer already exists — the qdrant Edge Dart SDK (a vector-database SDK; How would you like to handle the downstream jobs for the transition? A few options:
I'd lean toward (1) now with (3) as a follow-up, but it's your CI — what do you prefer? |
336da8c to
60cce3e
Compare
chavic
left a comment
There was a problem hiding this comment.
Thank you. The 0.32 port is close, and the &[u8] / ForeignBytes catch is a real find that the fixtures could not show.
Two items before merge, and one note:
lowerForeignBytesleaks on each call. See the inline comment.- The MSRV raise from 1.85 to 1.91 is correct and necessary (askama 0.16 needs 1.88, cargo-platform needs 1.91), and it is wired through Cargo.toml, CI, and flake.nix. But it changes a compatibility promise, so it needs an explicit maintainer sign-off in the PR text, not a silent ride-along.
The Test Suite failure is the sleep lower-bound assertion. I measured sleep(200) on this branch and on main: both give 202 to 204 ms, so the 0.32 async path is not the cause. The test measured time with the wall clock, which NTP can move on a CI runner. #160 corrects the test. Please rebase after it merges, and the Test Suite goes green here without a change to this PR.
The downstream jobs are a repo policy question. I leave that to your discussion with @chavic above.
| // position only (never lifted or read back). Dart's GC-managed | ||
| // `Uint8List` has no stable native address, so the bytes are copied | ||
| // into native memory, mirroring `toRustBuffer` above. | ||
| ForeignBytes lowerForeignBytes(Uint8List data) { |
There was a problem hiding this comment.
This function leaks native memory on each call. calloc<ForeignBytes>() and calloc<Uint8>(length) are never freed: Rust only borrows the buffer for the call, and the generated call site does not free it after the return. Each &[u8] argument call leaks its payload. payjoin-ffi has nine call sites of process_response(&self, body: &[u8]), so a polling loop grows without limit.
The Kotlin ByRef converter frees in a finally block after the call. The generated Dart call site needs the same shape. The (null, 0) case for an empty slice and the Int32 guard are good, but the struct allocation leaks in that path also.
Note: toRustBuffer above has the same latent leak. It is older than this PR and out of scope here.
60cce3e to
b06b3bb
Compare
…wer bounds measureTime used DateTime.now(), which is the wall clock. NTP can slew or step the wall clock on a CI runner during a measurement, so the interval it reports can be shorter than the time that actually elapsed. Rust's thread::sleep never returns early, so a lower-bound failure such as the one on Uniffi-Dart#149 (sleep(200) measured as <= 200ms) can only come from the clock, not from the code under test. Stopwatch is monotonic and immune to this. Three lower-bound assertions also used a strict '>' where the rest of the file uses '>='. A sleep of exactly N ms truncates to N milliseconds and fails a strict bound, so they are now inclusive. Local measurements of sleep(200), five runs each with the release cdylib: uniffi 0.31.2 (main) 202.7-204.2ms, uniffi 0.32 (Uniffi-Dart#149) 202.5-203.0ms. The distributions are identical, which rules out a runtime change as the cause. Refs Uniffi-Dart#139. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HDnKiUL8NDSpeaPvyKJeoR
Port the generator from uniffi 0.31.2 to 0.32.
- Bump the workspace uniffi deps to 0.32.
- Repoint `Literal` imports from the now-private
`uniffi_bindgen::pipeline::general::nodes` to `uniffi_bindgen::interface::Literal`.
- Adapt to `ObjectImpl::Trait` becoming a tuple variant `Trait(TraitKind)` (the old
`ObjectImpl::CallbackTrait` variant was removed). Callback-interface converter
naming now mirrors uniffi's own `ObjectImpl::has_callback_interface()`
(`Trait(Both | ForeignOnly)`), which preserves 0.31 behaviour: `with_foreign`
traits (old CallbackTrait, now Both) route to `FfiConverterCallbackInterface*`,
plain trait objects (now RustOnly) use their own class.
- Migrate the legacy `render_literal` off the private pipeline node types onto the
`interface` types; add the missing `Int64` numeric case (an i64 default literal
would otherwise hit the `not a number` panic) and handle the new
`LiteralMetadata::EmptySet` variant (returns no-default, matching
`default_for_interface_type`, until real `Set<T>` rendering exists).
- Align the remaining workspace-member fixtures (benchmarks, docstring-proc-macro,
metadata) to `uniffi = { workspace = true }`, fixing a 0.31-writer / 0.32-reader
metadata skew that broke docstring metadata extraction.
Also add a `map_type` fixture covering HashMap codegen (String->int, String->record,
nested maps, optional values, and the empty-map boundary) — Map is listed under
"Identified Blockers" in the README but already generates correctly.
The full fixture suite passes under uniffi 0.32.
55a0ef5 to
068fb1e
Compare
|
Thanks @chavic — both review points addressed, plus the rebase you suggested. 1. 2. MSRV 1.85 → 1.91: now called out explicitly in the PR description (its own section) for your sign-off — cargo-platform 0.3.3 needs 1.91, askama 0.16 needs 1.88; wired through 3. Rebased onto |
uniffi_bindgen 0.32's `BindingGenerator::new_config`/`get_toml` take a `&toml::Value`, so our impls in src/gen/mod.rs must resolve the same toml version the trait uses or they fail with E0053 "incompatible type for trait". uniffi_bindgen 0.32.0 declares `toml = ">=0.9, <2"` and resolves to toml 1.x, so pin `toml = "1"` to land on the same major. A looser pin lets the resolver put this crate on 0.9 while uniffi stays on 1.x, splitting the crate — and there is no committed Cargo.lock to stabilize it, so CI re-resolves and breaks. The 0.32 dependency tree requires a newer Rust: cargo_metadata pulls cargo-platform 0.3.3 (rustc >= 1.91) and uniffi 0.32 pulls askama 0.16 (rustc >= 1.88). Raise the declared rust-version and the nix msrv shell from 1.85 to 1.91 to match the real floor.
Match the crate rust-version raised for the uniffi 0.32 upgrade; the resolved dependency tree (cargo-platform 0.3.3) requires rustc 1.91.
…uffer
uniffi 0.32 selects `FfiType::ForeignBytes` for borrowed `&[u8]` / `[ByRef]
bytes` arguments (zero-copy, foreign->Rust only) instead of the owned
`RustBuffer` path — see `uniffi_bindgen::interface::Argument::is_borrowed_bytes`.
The generated `@Native` signature already declared the parameter as
`ForeignBytes`, but the call site still lowered the `Uint8List` with
`FfiConverterUint8List.lower` (which returns a `RustBuffer`), so any function
taking `&[u8]` failed to compile:
Error: The argument type 'RustBuffer' can't be assigned to the
parameter type 'ForeignBytes'.
This never surfaced on the existing fixtures (all use owned `Vec<u8>`), but it
broke every downstream consumer with a `&[u8]` argument (e.g. payjoin-ffi's
`process_response(&self, body: &[u8])`, 9 call sites).
Route borrowed-bytes args through a new `lowerForeignBytes` helper, mirroring
the `FfiConverterByRefBytes` in uniffi's Kotlin/Python backends. Dart's
GC-managed `Uint8List` has no stable native address, so it copies into native
memory like the existing `toRustBuffer`; the Rust side only borrows for the
call. Adds `take_bytes_by_ref(&[u8])` coverage to the bytes_types fixture.
…gth guard Review follow-up on the borrowed-bytes lowering: - Empty `Uint8List`: pass `(null, 0)` instead of `calloc<Uint8>(0)`. A zero-size `calloc` is platform-variable — some allocators return null, which makes package:ffi's `calloc.allocate` throw — while Rust's `ForeignBytes::as_slice` reads `(null, 0)` as `&[]`. Mirrors the Kotlin/Python ByRef converters, which special-case zero length to a null pointer for exactly this reason. - Guard against silent truncation: `ForeignBytes.len` is an `Int32`, so a `Uint8List` larger than `0x7fffffff` would wrap to a bogus (or negative) length and make Rust read the wrong slice or panic across the FFI boundary. Throw an `ArgumentError` instead. Also reword the oracle.rs routing comment so "borrowed" (the Rust-side borrow) isn't read as "zero-copy" on the Dart side, which still copies into native memory (a GC `Uint8List` has no stable address).
main's oracle.rs refactor (remove RustBuffer namespace helpers / emit scaffolding once) removed the only user of ExternalFfiMetadata, so after rebasing the 0.32 work onto it the import is unused. TraitKind stays (used by the object-converter arm).
`lowerForeignBytes` copies a `Uint8List` into native memory (a GC-managed
Dart list has no stable address to lend Rust, unlike the direct ByteBuffer
the Kotlin backend borrows zero-copy), but the generated call site never
freed that copy: the `ForeignBytes` struct and the byte buffer leaked on
every `&[u8]` argument call, so a polling loop — e.g. payjoin's
`process_response(&self, body: &[u8])`, nine call sites — grew without bound.
Allocate the copy from an `Arena` the call site scopes around the FFI call
and release it afterwards, so the native memory is freed once Rust is done
borrowing it:
- sync calls wrap the call in `using((Arena arena) { ... })`, which frees on
scope exit including if the call throws;
- async calls hold the borrow until the Rust future settles, so the arena is
released via `whenComplete` after the returned future completes.
Calls with no borrowed-bytes argument are unchanged (`return call;`), so
ordinary code keeps its exact previous shape and zero overhead.
The existing `bytes_types::take_bytes_by_ref` fixture round-trips a `&[u8]`,
but a leak passes a functional round-trip — nothing exercised repeated calls.
Add proc-macro exports `sum_borrowed_bytes`, `sum_borrowed_bytes_checked` and
`Object::borrowed_bytes_len` plus Dart tests covering the free-function,
method, fallible (error-path) and empty-slice call sites and a 50k-call loop
that would surface a leak (unbounded growth) or a double-free (crash).
068fb1e to
da65f67
Compare
The Test Downstream jobs clone payjoin/rust-payjoin and bitcoindevkit/bdk-dart and patch in this crate's 0.32 uniffi-dart — but both still pin uniffi 0.31.2, so their 0.31 metadata cannot be read by the 0.32 bindgen (Unexpected metadata type code). Both compile and their Dart suites pass on 0.32, so bump them in-CI to exercise the 0.32 path end-to-end: - payjoin: bump payjoin-ffi's uniffi to 0.32. - bdk: clone bdk-ffi at its pinned rev, bump it to 0.32, and local-[patch] bdk-dart onto it (+ bump bdk-dart's own uniffi, refresh its lock). - both: pin the native-assets rust-toolchain to an exact 1.91.0 (0.32's MSRV; native_toolchain_rust rejects `stable`) with just the Linux host target. Result on a fork run: payjoin 24 tests pass, bdk 22 pass / 5 skipped. Self-contained in the workflow (no forks); revert once payjoin/bdk ship uniffi 0.32 upstream.
|
Update: I went ahead and greened both downstream jobs rather than leaving them as the chicken-and-egg above — all 12 checks pass now, including Test rust-payjoin (24 tests) and Test bdk-dart (22 pass / 5 skipped). Both payjoin-ffi and bdk-ffi compile and run their Dart suites on 0.32 with only a version bump (no code migration), so
It's self-contained in the workflow (no forks) and commented to revert once payjoin/bdk ship 0.32 upstream. If you'd rather skip them or point the job at a different 0.32-native downstream, easy to reshape — but this keeps them honestly green against the real consumer APIs. That leaves just the MSRV 1.85 → 1.91 sign-off (its own section in the description) — over to you on that + merge. 🙏 |
…] coverage Review follow-ups on the borrowed-&[u8] leak fix: - Rename the generated Arena identifier `arena` -> `_uniffiArena` so it cannot collide with a host-supplied argument named `arena` (which would shadow the injected Arena / redeclare it and fail to compile). - Add a debug_assert in `trait_method_call` (the one call-site generator that lowers args directly, without the Arena wrap) so a future trait-derived method taking `&[u8]` fails loudly instead of silently miscompiling to a leaking RustBuffer lowering. - Document that the async+borrowed-bytes wrap arm is unreachable/defensive: uniffi 0.32 rejects a borrowed `&[u8]` on an async fn (non-Send/'static future). - Expand the proc-macro fixture to exercise the previously-uncompiled call-site shapes: a void-return `&[u8]` fn and a `&[u8]` constructor (initializer-list form), plus a fallible fn that now errors on a NON-empty input so the throw path frees a real data buffer, not just the empty (null,0) struct. - Rewrite the loop test's comment honestly: it is a compile+no-crash smoke test, NOT a leak detector (a leak does not fail a functional test); detecting the leak itself needs an RSS/sanitizer check, tracked as a follow-up.
f4685b4 to
50afc12
Compare
Ports the generator from uniffi 0.31.2 → 0.32. The full fixture suite passes under 0.32 (
cargo test --workspace).The 0.32 dependency tree forces a higher Rust floor:
cargo_metadatapulls cargo-platform 0.3.3 (rustc ≥ 1.91) and uniffi 0.32 pulls askama 0.16 (rustc ≥ 1.88). The bump is wired throughCargo.toml(rust-version), the CI matrix, andflake.nix. This changes uniffi-dart's compatibility promise, so — per your review — flagging it explicitly for your sign-off rather than letting it ride along silently. (tomlis pinned to1:uniffi_bindgen0.32.0 declarestoml = ">=0.9, <2"and resolves to 1.x, sonew_config/get_toml's&toml::Valueneeds this crate on the same major — a looser pin splits us onto 0.9 while uniffi stays on 1.x → E0053, and there is no committedCargo.lockto stabilize resolution.)Rebased onto
main(picks up #152 and #160); the async-timing Test Suite failure was thesleeplower-bound flake #160 fixes, not the 0.32 path.What changed
Literalimports repointed from the now-privateuniffi_bindgen::pipeline::general::nodesto the publicuniffi_bindgen::interface::Literal.ObjectImplbecameStruct+Trait(TraitKind)in 0.32 (theCallbackTraitvariant was removed). Callback-interface converter naming now mirrors uniffi's ownObjectImpl::has_callback_interface()=Trait(Both | ForeignOnly), which preserves 0.31 behaviour:with_foreigntraits (oldCallbackTrait, nowTraitKind::Both) →FfiConverterCallbackInterface*TraitKind::RustOnly) → own classrender_literalmigrated off the private pipeline node types ontointerfacetypes. Adds the missingInt64numeric case (ani64default literal would otherwise hit thenot a numberpanic) and handles the newLiteralMetadata::EmptySetvariant — it returns no-default (matchingdefault_for_interface_type) since the generator does not renderSet<T>types yet.benchmarks,docstring-proc-macro,metadata) still hardcodeduniffi = "0.31"; aligned them toworkspace = true. This fixes a 0.31-writer / 0.32-reader metadata skew that brokedocstring-proc-macroextraction (Unexpected metadata type code).New
map_typefixtureAdds coverage for
HashMapcodegen —String→int,String→record, nested maps, optional values, and the empty-map boundary. Map is listed under "Identified Blockers" in the README but already generates correctly; this fixture demonstrates it and guards the Map converter's offset arithmetic against variable-length value converters.Notes
docstring,keywords,time-types) still pinuniffi = "0.31"; they are not in[workspace] members, so they are never compiled/tested and are left untouched.+v0.31.2build-metadata version suffix is left for a maintainer release bump.Borrowed
&[u8]arguments →ForeignBytes(added)uniffi 0.32 lowers a borrowed
&[u8]/[ByRef] bytesargument throughFfiType::ForeignBytes(zero-copy, foreign→Rust only) instead of the ownedRustBufferpath — gated byArgument::is_borrowed_bytes(). The generator already emitted the@Nativesignature asForeignBytes, but the call site still lowered theUint8ListwithFfiConverterUint8List.lower(aRustBuffer), so any function taking&[u8]failed to compile:The existing fixtures only use owned
Vec<u8>, so nothing caught it, but it broke every downstream consumer with a&[u8]argument (e.g. payjoin-ffi'sprocess_response(&self, body: &[u8]), 9 call sites — verified fixed against payjoin-ffi on 0.32:24 tests passed).Fix: route borrowed-bytes args through a new
lowerForeignByteshelper, mirroring theFfiConverterByRefBytesin uniffi's Kotlin/Python backends. Empty slices pass(null, 0)(Kotlin/Python do the same — a zero-sizecallocis platform-variable), and there's anInt32length guard. Addstake_bytes_by_ref(&[u8])to thebytes_typesfixture (non-empty + empty round-trips).Per-call leak — now fixed (addresses your review):
lowerForeignBytescopies into native memory (Dart's GC-managedUint8Listhas no stable address, so unlike Kotlin's direct ByteBuffer it can't be zero-copy) and that copy was never freed — the struct + buffer leaked on every&[u8]call. It's now allocated from anArenathe call site scopes around the FFI call and released afterwards: sync calls wrap the call inusing((Arena arena) { … })(frees on scope exit, including if the call throws); async calls hold the borrow until the Rust future settles, so the arena is released viawhenComplete. Calls with no borrowed-bytes argument are unchanged. A functional round-trip passes despite a leak, so the fix adds proc-macro fixture exports + Dart tests exercising the free-function, method, fallible (error-path) and empty-slice call sites plus a 50k-call loop.