chore: drop unused imports and bindings in the crypto crates - #599
chore: drop unused imports and bindings in the crypto crates#599blacks1ne wants to merge 3 commits into
Conversation
Review feedback on QuilibriumNetwork#599: an unused variable can indicate a bug, so silencing the lint with an `_` prefix throws away the signal. It also does not remove the work — `let _x = expensive()` still evaluates. Three of the five underscores in this PR were "compute a value, then discard it". Each is now deleted, with the reason recorded: - time_reel.rs: the clone was a duplicate of the lookup+clone that `send_head_event` performs on the very next line, and its `.unwrap()` panicked where `send_head_event` returns gracefully. Removing it makes this branch identical in shape to the sibling branch below it. - membership.rs: `d_l` is only needed by the verifier, which builds it itself in `verify_combined_ml`. The prover paid a matvec, a sub and a concat for a value it never used. - range.rs: `let _q = params.q;` was a plain leftover. Deleting `d_l` then exposed a second one the binding had been hiding: `tvec` was `prove_combined`'s only remaining use of its target-vector parameter, so the compiler immediately reported the parameter as unused. That is correct — `d_l = (C.t1, L·C.t2 − t)` is public and only the verifier needs it; the prover's response `z = y + c·r` does not involve `t` at all. The parameter is dropped and the two call sites now discard the `tvec` half of `build_relation`'s result. `verify_combined` keeps it. This is the review comment's point demonstrated: the discarded binding was masking a second unused item one level up, and would have kept masking it indefinitely. The two remaining `_` prefixes are closure parameters in prover_registry.rs, which cannot be deleted — a positional parameter has to stay. `_p2p_handle` in dht_node.rs also stays: that binding exists to hold an RAII guard alive for the scope, and `let _ =` would drop it immediately.
|
@dazthecorgi You're right in TG comment, and the The line you linked is a real leftover, though not a live bug: Same treatment for the other compute-and-discard binding here: Pushed as d3dc2f8. I've applied the same review to the rest of the series and |
d3dc2f8 to
511436d
Compare
Mechanical cleanup of 45 rustc warnings in the uniffi-exported crypto
crates (`bls48581`, `channel`, `ferret`, `vdf`, `verenc`), which are
compiled by all three of `task build_node_amd64_linux`,
`task build_qclient_amd64_linux` and `task test_rust_amd64_linux`.
- `unused_imports`: the `use` item is deleted.
- `unused_variables`: the binding is `_`-prefixed rather than removed, so
any evaluation side effect is preserved. This deliberately keeps the
`for (_i, x) in ....enumerate()` shape instead of dropping `.enumerate()`,
to stay a rename and nothing more.
- `unused_mut`: the `mut` is dropped.
One class needed real thought rather than the mechanical rewrite. Five
`non_fmt_panics` warnings (`ferret/build.rs` x4, `vdf/src/lib.rs` x1) are
cases where a single-argument `panic!`/`assert!` message uses `{ident}`
captures. Both crates are edition 2018, where a one-argument `panic!`
takes the string verbatim -- so today these messages would print the
literal text `unsupported target {target}` rather than the target. The
rustc-suggested `panic!("{}", "...")` rewrite preserves that broken
output; instead each call gets an explicit argument so it becomes a real
format string and prints what it was clearly meant to print.
The `bls48581` `non_snake_case` and `ferret` `non_camel_case_types`
warnings are intentionally NOT touched here -- those identifiers mirror
their MIRACL / C originals and are handled separately.
One `unused_variables` site that would otherwise belong here is left to
the dead-code bucket instead: `json_to_metadata`'s `ratchet_state`. That
whole function is unused and is annotated there, so keeping the rename
next to the annotation keeps the site in a single PR rather than having
two of them touch adjacent lines. Its live counterpart `metadata_to_json`
is renamed here as normal.
Review feedback: an unused variable can indicate a bug, so silencing the lint with an `_` prefix throws away the signal. It also does not remove the work — `let _x = expensive()` still evaluates. tripleratchet.rs, the site raised in review: the binding was a copy of the block in `advance_and_decrypt`, minus the `self.`. It is genuinely dead, but not because the state update is missing — the caller performs it at the `should_advance_dkg_ratchet` branch that this function returns `true` for. Deleted, and replaced with a comment saying so, because the tempting "fix" of adding `self.` would write the field twice. bls48581/src/lib.rs: `let _M = BIG::new_ints(&rom::CURVE_ORDER)` was allocated on every recursion level of `recurse_fft` and never read. Deleted. The eight `for (_i, x) in ....enumerate()` loops drop the `.enumerate()` instead, since only the index was unused. The two tuple patterns keep their shape — `zip` bounds the iteration length, so the operands must stay — but bind `_` rather than a named placeholder. crates/bls48581/src/bls48581/ecp.rs keeps its `_mask`: that file is MIRACL Core under its own copyright header and is genuinely vendored.
Extends this bucket to `channel-wasm`, the wasm binding over `channel`. Its two dead `use` items sit in a `#[cfg(test)]` module and the workspace audit command excluded the crate, so they were never listed; with the rest of the series applied they are the only two rustc warnings left in the workspace. Both are dead: the module's single test reaches `ed448_rust`, `BASE64_STANDARD` and `serde_json` through `use super::*`, and nothing in it references `HashMap`, `Scalar`, `EdwardsPoint` or `elliptic_curve::Group`. Deleted rather than `_`-prefixed, per the convention used for `unused_imports` in the rest of the series.
Review feedback on QuilibriumNetwork#599: an unused variable can indicate a bug, so silencing the lint with an `_` prefix throws away the signal. It also does not remove the work — `let _x = expensive()` still evaluates. Three of the five underscores in this PR were "compute a value, then discard it". Each is now deleted, with the reason recorded: - time_reel.rs: the clone was a duplicate of the lookup+clone that `send_head_event` performs on the very next line, and its `.unwrap()` panicked where `send_head_event` returns gracefully. Removing it makes this branch identical in shape to the sibling branch below it. - membership.rs: `d_l` is only needed by the verifier, which builds it itself in `verify_combined_ml`. The prover paid a matvec, a sub and a concat for a value it never used. - range.rs: `let _q = params.q;` was a plain leftover. Deleting `d_l` then exposed a second one the binding had been hiding: `tvec` was `prove_combined`'s only remaining use of its target-vector parameter, so the compiler immediately reported the parameter as unused. That is correct — `d_l = (C.t1, L·C.t2 − t)` is public and only the verifier needs it; the prover's response `z = y + c·r` does not involve `t` at all. The parameter is dropped and the two call sites now discard the `tvec` half of `build_relation`'s result. `verify_combined` keeps it. This is the review comment's point demonstrated: the discarded binding was masking a second unused item one level up, and would have kept masking it indefinitely. The two remaining `_` prefixes are closure parameters in prover_registry.rs, which cannot be deleted — a positional parameter has to stay. `_p2p_handle` in dht_node.rs also stays: that binding exists to hold an RAII guard alive for the scope, and `let _ =` would drop it immediately.
511436d to
a265c00
Compare
The same mechanical class as #598 but in
bls48581,channel,ferret,vdf,verencand the four wasm wrappers — 46 warnings — plus fivenon_fmt_panicsthat are real message bugs.ferretandvdfare edition 2018, where a single-argumentpanic!takes thestring verbatim. So
panic!("unsupported target {target}")prints the literal{target}rather than the value — the panic message that is supposed to tell youwhich target failed does not.
Worth flagging:
cargo fix's suggestion for this lint ispanic!("{}", "unsupported target {target}"), which silences the warning andkeeps the broken output. These are fixed with explicit format arguments
instead, so the messages actually interpolate.
Two changes since this was first pushed
channel-wasmis now in scope (+2). Its two deaduseitems sit in a#[cfg(test)]module and the audit command excluded the crate, so they werenever listed. With the rest of the series applied they were the last two
warnings in the workspace.
json_to_metadata's unusedratchet_stateparameter is renamed there instead of here, so that the wholeof that dead function — annotation, comment and parameter — is described by a
single PR. Previously the two PRs touched adjacent lines of the same signature
and conflicted on whichever merged second; they now merge in either order and
produce an identical tree. The warning is still fixed exactly once across the
series.
metadata_to_json, which is live, keeps its rename here.Series
Part of the warning-cleanup series that starts with #597. The eight PRs are
disjoint and each stands on its own, but they are meant to be read in order —
please take #597 first: it is the only one of the eight that fixes a bug
rather than a warning, and it is the shortest.
932045a3the 16 being
classgroup's GMP FFI glue, deliberately left visible rather thansilenced. Update: those 16 are now fixed rather than documented, in fix: correct classgroup GMP FFI declarations and uninitialized values #605,
which corrects the declarations and the uninitialized values instead of
annotating them. With fix: correct classgroup GMP FFI declarations and uninitialized values #605 and the channelwasm commit in this PR, the combined tree
checks with zero warnings.
--exclude channelwasm. That crateis in scope now — it is the last commit here — so the exclusion is gone.)
Happy to re-pace these, drop any of them, or squash the set into a single PR if
you would rather review it in one pass — just say which.
Drafted with Claude Code; every site was read individually and the reasoning is
in the commit message.