Stabilize debug_closure_helpers#146099
Conversation
rustbot
added
S-waiting-on-review
coolreader18
changed the title
debug_closure_helpers
tgross35
added
the
I-libs-api-nominated
Amanieu
added
I-libs-api-nominated
|
☔ The latest upstream changes (presumably #146636) made this pull request unmergeable. Please resolve the merge conflicts. |
Amanieu
removed
the
I-libs-api-nominated
|
Once the FCP is complete, I think it might make sense to merge #145915 first and then merge this on top of that. |
| pub fn field_with<F>(&mut self, name: &str, value_fmt: F) -> &mut Self | ||
| where | ||
| F: FnOnce(&mut fmt::Formatter<'_>) -> fmt::Result, |
There was a problem hiding this comment.
Having the closure as a generic parameter (and not type-erasing it internally) leads to a lot of code bloat compared to the corresponding stable APIs that take &dyn Debug as value to format. According to cargo +nightly llvm-lines a simple program formatting a struct with two fields generates 694 lines of LLVM IR while the same program using field() generates 42 lines. So at least with the current implementation, these functions have an annoying downside compared to e.g. .field("foo", &fmt::from_fn(|f| ...)
I think this can be addressed after stabilization, without changing the signatures. But I wanted to flag it so the reviewer can think about it as well. It's not entirely obvious to me for the helpers that deal in FnOnce. I'm aware of one workaround (put the closure into an Option, then pass a &dyn FnMut that unwraps this option to call the original impl FnOnce) but it's not quite zero cost in several dimensions.
There was a problem hiding this comment.
Could this be solved by putting the main logic back into field(), and having field_with be an #[inline] wrapper around it?
I don't want to send in any PRs that might cause Git conflicts with the stabilization, but if the public API is good then I'd be happy to experiment with internal reorganization / optimization after both have landed.
There was a problem hiding this comment.
field takes a &dyn Debug. How do you invoke a FnOnce or FnMut from the &self argument of Debug::fmt?
There was a problem hiding this comment.
Cell<Option<F>> , then .replace(None).unwrap()? It doesn't need to survive more than one call.
There was a problem hiding this comment.
I think if field_with were changed to use dyn, it'd be important to ensure that it doesn't cause undue stack usage, since that was brought up as an issue with the existing functions in #117729 (comment)
There was a problem hiding this comment.
That report is in the context of a total stack size of 4 KiB. The DebugStruct::field function in my nightly toolchain's libcore.rlib allocates a very reasonable amount of stack space (add $0x48,%rsp). It then goes on to call other functions, but those seem to be functions shared with most of the formatting infrastructure. It's frankly a miracle that any variation of the code fits within a 4 KiB stack, and the cases that work probably depends on the code being duplicated and specialized a bit for the particular usage, which is fundamentally at odds with optimizing for code size (as core::fmt generally does). So I wouldn't give much weight to that report.
There was a problem hiding this comment.
@hanna-kruppe would you mind creating an issue for this? And/or a PR to change it? I think you're probably the best equipped to do so.
I'm ready to approve this but would like your confirmation that we have room to improve this within the current design.
There was a problem hiding this comment.
I can extract my above comment into an issue but I probably won't have time to try out a fix in the next 2-3 weeks. There's definitely room for improvement (e.g., the Cell<Option<F>>-via-&dyn Fn() approach discussed above). But without trying it out, I can't quantify how much improvement it is.
|
The stabilization PR for gentle ping @the8472 for review |
coolreader18
force-pushed
the
stabilize-debug_closure_helpers
branch
from
a51fcbe to
b1bde94
Compare
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #148544) made this pull request unmergeable. Please resolve the merge conflicts. |
coolreader18
force-pushed
the
stabilize-debug_closure_helpers
branch
from
b1bde94 to
1e39a61
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@bors retry |
|
@jmillikin: 🔑 Insufficient privileges: not in try users |
|
@the8472 gentle ping? |
|
r? libs-api |
rustbot
added
the
T-libs-api
|
☔ The latest upstream changes (presumably #149704) made this pull request unmergeable. Please resolve the merge conflicts. |
|
r? tgross35 Fyi you can The diff here LGTM, but I'd just like to make sure we have room to improve on what Hanna mentioned. |
|
Also this needs a rebase, of course |
coolreader18
force-pushed
the
stabilize-debug_closure_helpers
branch
from
1e39a61 to
52ef2cf
Compare
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #149941) made this pull request unmergeable. Please resolve the merge conflicts. |
coolreader18
force-pushed
the
stabilize-debug_closure_helpers
branch
from
52ef2cf to
044818c
Compare
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #150068) made this pull request unmergeable. Please resolve the merge conflicts. |
coolreader18
force-pushed
the
stabilize-debug_closure_helpers
branch
from
044818c to
d7204e5
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
Status update: waiting for team feedback at #117729 |
tgross35
added
S-waiting-on-t-libs-api
|
Per #117729 (comment), blocked on #149745 for now. @rustbot blocked |
rustbot
added
the
S-blocked
|
☔ The latest upstream changes (presumably #151381) made this pull request unmergeable. Please resolve the merge conflicts. |
Amanieu
removed
the
S-waiting-on-t-libs-api
|
@coolreader18 any chance you'd be willing to do the changes mentioned at #117729 (comment) in a separate PR? It should be pretty easy, and that would unblock this. |
|
Regarding that comment, it's a bit unclear (to me at least) how an implementation based on let mut writer = PadAdapter::wrap(self.fmt, &mut slot, &mut self.state);
key_fmt(&mut writer)?;... I think? |
|
Haven't looked into it closely but I think it's a pattern to call (You should probably ask for followup at #149745 or #117729, I don't think the relevant people are subscribed to this PR) |
Scaffolds the Rust ↔ Vue IPC boundary every P10.2+ business command
will plug into. No WPF parity mapping (the C# tree dispatches UI
events directly through the WPF view-model); this chunk lands the
plumbing tauri-specta derives bindings off and the CommandError
contract every later command returns.
commands/error (CommandError DTO + domain conversions):
- CommandError { code: String, message: String, details:
Option<serde_json::Value> } derives Serialize + specta::Type so
every fallible command returns the same shape across the IPC wire.
Builder helpers new(code, message) and with_details(v) keep call
sites readable.
- `From` for all 7 domain errors (LoginError 42 variants,
StorageError 8, ConfigError 4, ProcessError 8, RegistryError 2,
GameError 7, UpdaterError 4) — one CommandError code per variant
using the <domain>.<variant_snake_case> convention so the
compiler's exhaustive match enforces that every new variant gets
a stable wire code (renaming a variant won't silently change the
frontend i18n key). Structural fields (url, pid, path, http
status, ShellExecute code, …) flow into `details` as JSON; non-
Serialize embeddings (reqwest::Error, serde_json::Error,
io::Error) use display-string + side-channel flags (is_timeout,
line/column, io_kind) so the frontend still gets structured
context without dragging foreign types into the specta graph.
- Helper fns io_kind_str / reqwest_details / serde_json_details
centralise the display-string shape so every From impl that
embeds a foreign error writes the same `details` contract.
- Module docs include the full code registry (7 domains + the
command-layer system.* codes for app_data_missing /
spawn_blocking_failed) so the frontend can cross-reference
without grepping source.
commands/state (AppState shared runtime state):
- AppState { storage_root: PathBuf, session:
tokio::sync::RwLock<Option<Session>> } injected through
Builder::manage so every command receives it via State<'_,
AppState>. Session is an empty placeholder (P10.2 fills avatar /
token / account_list). http_client deliberately deferred to P10.2
— holding a reqwest::Client the P10.1 smoke commands never call
would surface as a dead_code warning now and a premature design
commitment later.
- tokio::sync::RwLock (not std) so guards are `Send` and survive
.await points inside async command bodies.
commands/system (smoke commands):
- #[tauri::command] pub fn version() -> VersionInfo { app, tauri }
exercises the synchronous-infallible IPC path and returns a
struct DTO so tauri-specta's TS emission is tested against
non-trivial shape, not just a scalar.
- #[tauri::command] pub async fn ping(message: String) -> Result<
String, CommandError> exercises the async + tokio::task::
spawn_blocking path every P10.2+ Win32-wrapping command will
follow. JoinError maps to system.spawn_blocking_failed. The 60ms
sleep is long enough to prove the work truly ran off-runtime
without slowing the suite.
commands/mod (build_specta_builder helper):
- Single-source-of-truth `pub fn build_specta_builder<R:
tauri::Runtime>() -> Builder<R>` wraps the collect_commands!
call so adding a command is a one-line edit rather than three
(runtime invoke_handler, bindings export, future mock-invoke
integration test). Generic over R so a P10.2+ mock-invoke test
can instantiate it with tauri::test::MockRuntime without
retrofitting the signature.
- Module-level docs carry the IPC architecture diagram and the
add-a-command howto so the expected workflow survives the next
contributor.
lib.rs (boot sequence + bindings export):
- resolve_storage_root() pulls %APPDATA%\Beanfun on Windows
(matches the WPF client's SpecialFolder.ApplicationData
convention so on-disk state lands in the same place) and falls
back to std::env::temp_dir().join("Beanfun") for non-Windows dev
builds. Missing APPDATA surfaces as CommandError with code
system.app_data_missing; run() exits with a concise diagnostic
rather than panicking when the env var really isn't set.
- export_specta_bindings<R: Runtime> writes bindings.ts to
<CARGO_MANIFEST_DIR>/../src/types on every debug boot (auto-
keeping the frontend in lock-step with renamed commands). Gated
behind #[cfg(debug_assertions)] so release installers — which
often live under Program Files and would error out on a source-
tree write — skip the path; failures inside the helper are
non-fatal and logged to stderr so a broken dev checkout still
boots with whatever bindings.ts is already on disk.
- greet scaffold removed (not part of the Beanfun feature set).
App.vue:
- Scaffold `greet` form + ref + invoke call stripped to match the
backend cleanup. Left a blank app shell for P11 UI work.
Cargo.toml:
- tauri-specta pinned to =2.0.0-rc.21 (with =2.0.0-rc.22 specta +
=0.0.9 specta-typescript). rc.24 pulls specta rc.24 which uses
fmt::from_fn from the unstable debug_closure_helpers feature
(rust-lang/rust#117729), breaking the stable rustc 1.92 build.
Section comment pins the rationale + rust-lang/rust#146099 as
the stabilisation watchpoint for a future upgrade.
- tauri gains "specta" feature (wires Tauri's State / Window
types into the specta type graph).
- specta gains "derive" + "serde_json" (the latter needed for
CommandError's Option<serde_json::Value> field to derive
specta::Type).
Tests (430 → 462, +32):
- 20 error From-impl cases (≥1 per domain, covering Option<String>
fields, non-Serialize embeddings, and structured details JSON).
- 3 AppState cases (new / session lifecycle / storage_root).
- 3 system cases (version fields, ping echo, ping Unicode).
- 1 bindings_file_tests::bindings_file_contains_all_p101_symbols
lib-level guard that reads the committed bindings.ts and grep-
asserts `version` / `ping` / `CommandError` / `VersionInfo`
appear on `export`-prefixed lines; skips with a stderr hint on
fresh clones where bindings.ts doesn't yet exist.
- 5 mod.rs doc examples compile-verified by rustdoc.
D11 scope note: the original plan was
`tests/ipc_smoke.rs::mock_invoke`, but any test binary that
statically instantiates `tauri_specta::Builder<R>` (for any R,
including MockRuntime) pulls `tauri-runtime-wry` →
`webview2-com-sys` into the link graph, which declares
WebView2Loader.dll as a regular (non-delay-loaded) import. Windows
then refuses to load the test .exe with STATUS_ENTRYPOINT_NOT_FOUND
whenever the DLL isn't discoverable on PATH, even with the WebView2
runtime itself installed. The production `cargo tauri dev` path
works because Tauri's own build setup places the DLL next to the
main binary. Rather than duplicate the specta export pipeline or
fight the native-DLL story with a workaround, bindings_file_tests
treats bindings.ts as a committed artefact (the frontend imports
from it anyway) and asserts on disk — catching drift on every CI
`cargo test` without needing a Tauri runtime.
Quality gates: cargo fmt --check / cargo clippy --all-targets --
-D warnings / cargo test --lib 462/462 / cargo test --tests (all
existing integration files 0 failed; no new integration binary) /
RUSTDOCFLAGS="-D warnings" cargo doc --no-deps.
10 participants
Resolves #117729. The tracking issue still needs an FCP, but I'm hoping that creating a stabilization PR will prompt one.
r? libs-api