Stabilize debug_closure_helpers - #146099
coolreader18 wants to merge 1 commit into
Conversation
rustbot
added
S-waiting-on-review
coolreader18
changed the title
debug_closure_helpers
Amanieu
added
I-libs-api-nominated
|
☔ The latest upstream changes (presumably #146636) made this pull request unmergeable. Please resolve the merge conflicts. |
|
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 |
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. |
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 |
|
☔ 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. |
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.
d7204e5 to
c47f842
Compare
|
The parser was modified, potentially altering the grammar of (stable) Rust cc @fmease |
|
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. |
clarfonthey
added
S-waiting-on-review
This comment was marked as outdated.
This comment was marked as outdated.
|
@rfcbot fcp merge libs |
|
@clarfonthey has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
rust-rfcbot
added
proposed-final-comment-period
clarfonthey
added
S-waiting-on-fcp
|
Clarification on FCP: the FCP was a long while ago and stabilisation was ultimately blocked on the code size issues being fixed. We've now decided those issues are substantially better now, but want to redo FCP anyway given all that. |
|
@coolreader18 could you rebase this? Making sure there aren't new failures since this has been around for a while |
|
@bors try |
This comment has been minimized.
This comment has been minimized.
… r=<try> Stabilize `debug_closure_helpers`
|
(I mean, we can still rebase, but it seems fine.) |
|
I just rebased it 2 weeks ago, to fix merge conflicts. |
rust-rfcbot
added
final-comment-period
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
| /// # Panics | ||
| /// | ||
| /// `key` must be called before `value` and each call to `key` must be followed | ||
| /// by a corresponding call to `value`. Otherwise this method will panic. |
There was a problem hiding this comment.
key and value have these panic sections. They should be updated here to mention key_with and value_with, and key_with and value_with should get the same thing.
Expect a minor conflict with #162825 if that merges first.
|
Wish I had noticed this earlier: we specify Nominating since this is already in FCP. @rustbot label +I-libs-nominated |
rustbot
added
the
I-libs-nominated
|
(Note: if you want to make that concern blocking, would need someone with checkbox privs to register it, but you can do that as a libs member.) |
|
☔ The latest upstream changes (presumably #162907) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
13 participants
View all comments
Resolves #117729. The tracking issue still needs an FCP, but I'm hoping that creating a stabilization PR will prompt one.
r? libs-api