Emit runtime scaffolding once into a shared module - #150
Conversation
1egoman
changed the title
|
I just pushed ab28306 to attempt to fix some clippy errors, but it looks like I can't be sure that lets that ci job pass until a maintainer approves the ci run for this commit. There was also a failing test in another ci job - from what I can tell, this seems to be a known flaky one (#139) so I'll leave this unaddressed. |
|
@1egoman, thanks for working on this. I'm taking a look now |
chavic
left a comment
chavic
left a comment
There was a problem hiding this comment.
Thank you for this change. The method is correct.
I tested the change. I made two crates:
demoextgives a record and an error.demotopgives a function that returns the record or the error.
I made the bindings in library mode. Then I did an analysis of the Dart code.
On main, the Dart code has four errors. These are the errors in issue #84:
error - lib/demotop.dart:423:5 - The argument type 'ExtExceptionErrorHandler' can't be
assigned to the parameter type 'UniffiRustCallStatusErrorHandler?'
error - lib/demotop.dart:433:5 - ... 'RustBuffer' is defined in demoext.dart / demotop.dart
With this change, the Dart code has no errors.
The Dart tests are satisfactory. The tests include these operations:
- A call across two namespaces.
- An error across two namespaces.
- 2000 RustBuffer allocations across two namespaces.
- An async call across two namespaces.
I found seven items. One item stops the compilation. The other six are small, and most of them are cleanups in the same area. Six items are in the code, below.
The tests
I agree with your note about the fixtures.
The risk of this change occurs only when one generation makes two components. Because of this, a test with two components gives more protection than the other fixtures give.
My test setup is small:
- Two crates with proc-macro exports.
- One call to
generate_dart_bindingsin library mode. - A test that
dart analyzefinds no error.
I am happy to make this fixture and send it to you, if that helps.
|
|
||
| fn ffi_converter_name(&self) -> String { | ||
| format!("FfiConverter{}", &DartCodeOracle::class_name(&self.id)) | ||
| format!("FfiConverter{}", DartCodeOracle::class_name(&self.id)) |
There was a problem hiding this comment.
The changes to the format arguments are correct, and they are safe.
I tried to reproduce the clippy failure and I could not. I ran cargo clippy -- -D warnings, which is the command in CI. On main, clippy reported no problem. My rustc version is 1.94.0-nightly. The CI for main is also green.
Maybe your toolchain has a different configuration. Can you give the clippy version and the output? If the failure is not on main, then this commit can go in a different PR, or the message can change.
There is one more item, and it is not related to this commit. cargo clippy --all-targets -- -D warnings fails on main and on this branch with items_after_test_module. This change moves mod tests to a higher position in types.rs. Because of this, generate_type and runtime_scaffolding come after the test module. CI does not use --all-targets and does not find this problem. A move of the test module to the end of the file corrects it.
There was a problem hiding this comment.
I moved the test module to the end of types.rs. cargo clippy --all-targets -- -D warnings is now clean for me - fixed in 31951aa.
Going back and forth with a LLM, it looks like the reason you can't reproduce this failure is due to toolchain version. CI seems to use 1.97.0 which causes it to occur, and if I use 1.94.0-nightly (or what I was using initially in development, rustc 1.94.1 / clippy 0.94.1) it all looks fine to me locally as well. The reason it seems to not fail on main is because the last commit was ~2 months ago (which ran on rustc 1.96.0), and I suspect that if you re-ran the CI again now it would fail.
What version of rust should these lints be expected to pass on? Should that be the version that is currently used by ci (1.97.0), or should the version running in ci be changed to be fixed at 1.96.0 / 1.94.0-nightly release and this extra ab28306 commit dropped?
2babbc6 to
4296714
Compare
|
@chavic Thanks for the thorough review! I believe I've addressed all of your notes and left a question on the lint related comment, would you be able to take another look? |
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
Every component file re-declared the whole runtime, so `UniffiRustCallStatusErrorHandler` and friends were nominally distinct types per file and values could not cross a crate boundary: an error handler declared in a dependency would not type-check against the importing crate's `uniffiRustCallAsync`. The scaffolding now goes out once, into `uniffi_runtime.dart`, which every component imports and re-exports. The `rustbuffer_*` extern declarations move with it, since `RustBuffer`'s own methods call them, so `uniffi_function_definitions` takes a predicate partitioning the FFI functions between the runtime file and the component files. Two consequences: - `RustBuffer` is now a single type, so external records no longer need namespace-qualifying. - The foreign-future helpers had to lose their leading underscore. Generated component code references them, and a private name is not visible across a library boundary.
`is_rustbuffer_fn` tested for the `_rustbuffer_` substring, so an exported user function such as `to_rustbuffer_bytes` was also treated as a runtime helper. Its extern declaration was then filtered out of the component file while the runtime file -- which only sees the first component -- never declared it either, and the generated Dart failed to compile. Compare against `ComponentInterface::iter_rust_buffer_ffi_function_definitions` instead. That is the same set `runtime_scaffolding` calls, so the two lists cannot drift apart.
Each external namespace was imported twice, once plain and once under a library prefix, so that an external `RustBuffer` could be qualified. The shared runtime module makes `RustBuffer` a single type and the prefix went away with it, which left the prefixed import unused. `dart analyze` reported it as a warning.
`rust_buffer_name_with_path` returned a constant after the runtime module made `RustBuffer` a single type, so the call sites now say `quote!(RustBuffer)` directly and the helper -- with `rust_buffer_name` above it -- is gone. The `ComponentInterface` argument that only served the namespace lookup is gone as well, from `ffi_dart_type_label`, `ffi_native_type_label`, `native_type_label`, `native_dart_type_label` and `callback_param_type`. Two callback-interface helpers lost their renderer argument for the same reason. No behaviour changes; the generated Dart is identical.
Moving the runtime scaffolding out of the component files left some of their imports unused, and `dart analyze` fails on a warning. Which imports go unused depends on the interface, so each generated file now carries an `// ignore_for_file: unused_import` directive; tracking usage per import costs more than the result is worth. The test harness build hook imported `dart:io` without using it, so that import goes too. `dart analyze` on the `hello_world` output now reports no issues, down from 3 on `main`.
The runtime module holds one asset id and one set of `rustbuffer_*` externs, taken from the first component, and every other component uses them. That holds today because only library mode generates more than one component, every component there links into one cdylib, and UniFFI gives each the same cdylib name -- but only a comment said so. `write_bindings` now bails if two components resolve to different asset ids, so a configuration that breaks the assumption gives an error instead of silently wrong code. It also bails on a component whose namespace is `uniffi_runtime`, which would otherwise write its component file over the runtime file.
`cargo clippy --all-targets -- -D warnings` failed with `items_after_test_module`, on this branch and on `main`: `generate_type` and `runtime_scaffolding` followed `mod tests`. CI does not pass `--all-targets` and so does not see it. With the test module last, `--all-targets` is clean.
076aa82 to
1730000
Compare
Removing the `ComponentInterface` and renderer parameters left several calls wrapped across lines that now fit on one, and rustfmt wants them collapsed. `cargo fmt --all -- --check` is clean again, on stable and on 1.85.
The multi-crate codegen fixes (Uniffi-Dart/uniffi-dart#150, #154, #156) are all merged upstream, so the temporary pin to the contributor fork is no longer needed. Pin the current upstream main rev, as no release tag has been cut since v0.2.1. Upstream codegen now emits ignore_for_file hints and drops unused imports, but it introduces unused locals in async callback trampolines, so add unused_local_variable to the generated-code lint ignores.
2 participants
Problem
Issue #84 reports a type error in generated Dart code due to a Rust function returning an error type from a different UniFFI namespace. The generated Dart code then does not compile:
Cause
The generator wrote the runtime scaffolding into every component file - each file declared its own
UniffiRustCallStatusErrorHandler.Dart creates a separate type for each declaration, and two component files therefore held two different types with the same name. An error handler from one file cannot pass touniffiRustCallAsyncin another file.RustBufferhad the same problem, and #50 correctedRustBufferonly. It imported each external namespace two times, and one import used a library prefix. The generator then wrote that prefix before each externalRustBuffer. This method corrects one type. It does not correct the error handler types, or any other runtime type.Change
The generator now emits the runtime scaffolding one time, into a new file named
uniffi_runtime.dart. Every component file imports this file, and every component file also re-exports it. The re-export keeps the public API complete for a consumer that imports only the component file.The
rustbuffer_*extern declarations move into the new file, because the methods of RustBuffer call them.uniffi_function_definitionstherefore takes a predicate. The predicate divides the FFI functions between the runtime file and the component files. A symbol must not have a declaration in two files.The change has two further results:
RustBufferis now a single type. The namespace prefix from Support multiple namespaces #50 is no longer necessary, so this PR removes it.Tests
Please note the limit of the coverage of existing tests. The fixtures run in UDL mode, and each fixture generates one component. No fixture generates two components together. The fixtures therefore show that this change causes no regression. They do not prove the correction.
I tested the multi-component case by hand. I generated bindings for two crates, livekit-uniffi and livekit-datatrack. The second crate re-exports the first. Before this change, the generated Dart code did not compile. After this change, it compiles.
A fixture with two components is a good follow-up task - let me know and I can try to add something here. Such a fixture would protect this behavior and would also help #72.
Fixes #84.