chore: clippy -D warnings + CI gate#26
Open
saroupille wants to merge 7 commits into
Open
Conversation
Run cargo clippy --workspace --all-targets --fix to flush easily
auto-fixable lints. No logic changes; mechanical reformatting only:
- needless_borrows_for_generic_args / needless_borrow: drop redundant &
- needless_lifetimes: elide explicit 'a where the compiler infers it
- manual_is_multiple_of: x % n == 0 -> x.is_multiple_of(n)
- infallible_destructuring_match: match -> let on single-variant enums
- new_without_default: derive/impl Default for types with new() -> Self
- unused_imports: drop ZERO from places that no longer reference it
- redundant_closure: |f| hex::encode(f) -> hex::encode
- option_map_or -> is_none_or where applicable
Files touched: core/src/lib.rs, apps/wallet/src/lib.rs,
verifier/src/{lib,bundle}.rs, tezos/rollup-kernel/src/lib.rs,
tezos/rollup-kernel/tests/bridge_flow.rs.
Reduces baseline warnings from 64 -> 39 unique. Remaining 39 require
manual judgement (large_enum_variant, too_many_arguments, ptr_arg in
public APIs, doc-comment cleanup, vendored starknet-ff lints) and are
addressed in follow-up commits.
Address the 39 lints that did not auto-fix in the preceding sweep:
- empty_line_after_doc_comments (10): orphaned section-header doc
comments converted to regular `//` comments. They were headers
above test groups, not item docs.
- too_many_arguments (10): suppressed with #[allow] + a one-line
rationale at each site. All occurrences are either protocol-fixed
sighash inputs (`shield_sighash`, `transfer_sighash`,
`unshield_sighash`), CLI command entry points whose arguments
mirror clap flags (`cmd_transfer`, `cmd_unshield`,
`shadownet_profile`, `prepare_transfer_skip_proof`), demo helpers
mirroring the spec (`Chain::shield`, `Chain::transfer`), or test
constructors.
- large_enum_variant (4): `WireKernelInboxMessage` is wire-format and
cannot be boxed without breaking the on-the-wire encoding;
`ParsedRollupMessage` is short-lived per inbox entry; `UserCmd` and
`UserProfileCmd` are clap-driven enums whose variants live on the
stack only while dispatching one CLI invocation. All four
suppressed with #[allow] + comment.
- ptr_arg (3): one site (`hex_f_vec::serialize`) is a serde
`serialize_with` callback whose signature is dictated by serde —
suppressed with #[allow]. The other two
(`encode_verify_bundle_json`) take internal `&Vec<_>` arguments
rewritten to `&[_]`.
- needless_range_loop (2): rewritten with `.iter().enumerate()` /
`.iter().take()`.
- vendored crate `starknet-ff` (2): two warnings (non_local_definitions
and unexpected `cfg(asm)`) emitted by the `MontConfig` derive
expansion. Suppressed at the patched-crate's `lib.rs` with a comment
explaining this is vendored, patched code we do not own.
- dead_code (1): `Chain::verify_memo` in the demo crate documents the
contract-side memo verification and is intentionally unused —
suppressed with #[allow] + comment.
- while_let_loop (1): `loop { let Some(_) = ...; }` rewritten as
`while let Some(_) = ...`.
- unnecessary_to_owned (1): `&PATH_TREE_ROOT.to_vec()` -> `PATH_TREE_ROOT`
- field_reassign_with_default (1): rewritten with struct-update syntax.
- unnecessary_get_then_check (1): `get(k).is_none()` -> `!contains_key(k)`.
- cloned_ref_to_slice_refs (1): `&[n.clone()]` -> `slice::from_ref(&n)`.
- unnecessary_sort_by (1): `.sort_by(|a,b| b.1.cmp(&a.1))` ->
`.sort_by_key(|&(_,v)| Reverse(v))`.
No logic changes. Workspace `cargo clippy --all-targets -- -D warnings`
now exits 0. Wallet/core/services lib tests pass unchanged.
Adds a `clippy` job to the existing Unit Tests workflow that runs: cargo clippy --workspace --all-targets -- -D warnings on stable Rust (matching the channel the bulk of the workspace tests already use). Runs in parallel with the existing `rust-and-cairo` and `ocaml` jobs. The two preceding commits flush the workspace to zero clippy warnings under this gate.
Apply `cargo +nightly-2025-07-14 clippy --workspace --all-targets --fix
-- -W clippy::uninlined_format_args` plus the same on apps/prover so the
repo speaks one format-args dialect (inline) instead of mixing 283
positional vs 2 inline.
Test assertion messages reformat to inline (`assert!(x, "got {}", y)` ->
`assert!(x, "got {y}")`). Behavior is identical; the difference is only
visible in failure output if/when an assertion trips.
Mechanically generated (no token-rewriting macros touched). Verified:
- cargo +nightly-2025-07-14 clippy --workspace --all-targets -- -D warnings (0 errors)
- cargo clippy --workspace --all-targets -- -D warnings (0 errors)
- cargo build --workspace (clean)
- cargo test -p tzel-{core,wallet-app,services} --lib (97 + 103 + 76 ok)
Rust 1.74+ supports `[workspace.lints]`, propagated to each member via `[lints] workspace = true`. Move the uninlined_format_args policy to the workspace root so a single edit re-tunes the gate for every crate, and new members inherit the policy by default. apps/prover is genuinely out-of-workspace (its own rust-toolchain pins nightly), so it gets its own [workspace.lints] block with the same policy.
Three-part clippy gate: 1. clippy-stable -- existing `cargo clippy --workspace --all-targets`. 2. clippy-nightly -- same workspace under nightly-2025-07-14, which is what catches uninlined_format_args (stable does not warn). This is what allows the [workspace.lints] policy to actually deny in CI. 3. apps/prover -- genuinely out-of-workspace (its own rust-toolchain.toml pins nightly-2025-07-14). Run as a separate manifest under the same nightly job. This pulls services/reprover in transitively as a path dep, so we do NOT add a standalone reprover clippy step (and one wouldn't compile anyway -- workspace root would need to mark reprover excluded, which would split the build graph for no gain). Each job uses Swatinem/rust-cache pinned by SHA (v2.7.8) keyed by toolchain so cold builds (~15-20 min today) become incremental.
Reviewer flagged that `services/reprover` is outside the lint gate:
cargo applies `--cap-lints=allow`-equivalent to non-current-workspace
path deps, so the nightly job's
`cargo clippy --manifest-path apps/prover/Cargo.toml` walks through
reprover's compile (the Checking output confirms it) but does NOT
deny on its lints. As proof, reprover line 94 had a `format!("{:#x}",
f)` that should fire `uninlined_format_args` but was never surfaced.
Two changes here:
1. Flush the one site to inline form: `format!("{f:#x}")`. Reprover
now has zero uninlined-format-args sites, matching the rest of
the codebase.
2. Declare `[lints.clippy] uninlined_format_args = "deny"` in
reprover's manifest. Cargo will not enforce this when reprover
is consumed as a path-dep (cap-lints), but it surfaces if anyone
ever clippy's reprover directly (e.g. via a future restructure
that elevates reprover into a workspace), AND it documents the
intent so a contributor reading the manifest sees the policy.
Limitation still present after this change: there is no standalone
`cargo clippy --manifest-path services/reprover/Cargo.toml` step in
CI. We tried wiring one in but cargo fails on the regenerated
Cargo.lock (reprover standalone resolves `ruint v1.18.0` which
requires a const-fn feature that nightly-2025-07-14 doesn't have
stable; the prover-bundled lockfile pins `1.17.2` which works).
Adding standalone clippy would require either committing a
reprover-side lock that diverges from prover's, or re-rooting both
crates into a single hierarchical workspace (impossible because
they live in non-hierarchical sibling paths). Tracked as
follow-up.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
cargo clippy --workspace --all-targets -- -D warningshad no CI gate, and a fresh measurement againstorigin/mainproduced ~50 errors split across stable and nightly toolchains:too_many_arguments,needless_borrows_for_generic_args,empty_line_after_doc_comments,large_enum_variant, etc.)apps/proverpins viarust-toolchain.toml, and the channel several CI test jobs invoke fortzel-verifier,tzel-rollup-kernel,apps/demo): an additional 25+ errors, allclippy::uninlined_format_args(warn-by-default on nightly, allow-by-default on stable).Without a gate, future PRs could re-introduce warnings silently. With the gate on stable only, the half of the codebase that builds on nightly was decoupled.
What this PR does
Mechanical flush (commits
9f64a89,3f815aa) —cargo clippy --fixfor the auto-fixable lints (too_many_arguments,needless_borrows_for_generic_args, etc.), plus narrowly-scoped manual fixes /#[allow]for the rest. Each#[allow]carries a one-line rationale.Stable CI gate (commit
6856d02) — adds aclippyjob to.github/workflows/unit-tests.ymlrunningcargo clippy --workspace --all-targets -- -D warnings. This was v1; the toolchain mismatch below was caught in review.Full repo flush to inline
format!form (commiteab9ad6) — repo-wide grep showed 283 positionalformat!calls vs 2 inline; mechanical sweep viacargo +nightly-2025-07-14 clippy --fix -W clippy::uninlined_format_argsrewrites them to the inline form. 16 files touched, +486 / −632 (net −146). Test-assertion message strings reformat (assert!(x, "got {}", y)→assert!(x, "got {y}")); this is visible only in test failure output.Workspace
[lints.clippy]table (commita6bd01b) — Rust 1.74+[workspace.lints]is the durable home for theuninlined_format_args = "deny"policy. CI-only enforcement is strictly weaker (it doesn't surface in the IDE, doesn't warn on localcargo build). All workspace members opt in via[lints] workspace = true.Dual-toolchain CI gate with caching (commit
b85dbc2) —clippy-stablerunscargo clippy --workspace --all-targets -- -D warningsclippy-nightlyruns the same on nightly-2025-07-14 PLUS a separatecargo clippy --manifest-path apps/prover/Cargo.toml --all-targets -- -D warningsstep (apps/prover is out-of-workspace and pins its own toolchain viarust-toolchain.toml).Swatinem/rust-cache@v2(SHA-pinned per workflow convention) keyed by toolchain so the doubled CI cost is amortised — cold ~15-20 min, warm ~2-3 min.Reprover lint policy + flush (commit
240fb7e, post-review-2 fix) —services/reprover/src/lib.rs:94hadformat!("{:#x}", f)that the v2 gate didn't catch: cargo applies a--cap-lints=allow-equivalent to non-current-workspace path deps, socargo clippy --manifest-path apps/prover/Cargo.tomlwalks reprover's compile but does NOT deny on its lints. The site is flushed toformat!("{f:#x}")here, and[lints.clippy] uninlined_format_args = "deny"is declared in reprover'sCargo.tomlto document intent. Known limitation: cargo will not enforce that policy from the prover-rooted clippy invocation (cap-lints), and a standalonecargo clippy --manifest-path services/reprover/Cargo.tomlis impractical because reprover would need its ownCargo.lock(the prover-shared lock pins compatible versions; a fresh standalone resolve picksruint v1.18.0which trips a const-fn issue on this nightly). Tracked as follow-up.Verification
All four commands exit 0 from a fresh worktree on the head commit:
cargo clippy --workspace --all-targets -- -D warnings(stable rustc 1.95.0)cargo +nightly-2025-07-14 clippy --workspace --all-targets -- -D warningscd apps/prover && cargo clippy --all-targets -- -D warningscargo build --workspaceTest suites (mechanical change, no semantic risk):
tzel-wallet-app --lib103 pass / 3 ignored,tzel-core --lib97 pass,tzel-services --lib76 pass — all matchorigin/mainbaseline.Vendored crates
The
--fixsweep did NOT touchthird_party/patched-crates/starknet-ff-0.3.7. The original PR added two#![allow(...)]at the patched crate's lib.rs root (coveringnon_local_definitionsandunexpected_cfgs) — appropriate posture for vendored upstream code.Follow-ups
cargo clippy --manifest-path services/reprover/Cargo.toml -- -D warningsstep in CI, contingent on either a committed reprover-side lockfile or a re-rooting of the prover/reprover workspace structure.[lints]could broaden beyonduninlined_format_argsonce the codebase is stabilised on the inline form across other lints worth pinning.