refactor(lint): replace crate-wide lint allowances with scoped exceptions - #830
refactor(lint): replace crate-wide lint allowances with scoped exceptions#830TheWeirdDee wants to merge 3 commits into
Conversation
|
@TheWeirdDee Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
Update: CI's clippy job runs whatever Rust/Clippy is currently
|
|
@TheWeirdDee Please fix CI issues |
…all) Same fix as Nanle-code#759/Nanle-code#830: database.rs used thiserror::Error/#[error(...)] without thiserror being a declared dependency, and passed &mut Transaction where the Migration trait expects &mut Connection (Transaction has no DerefMut). Switched the trait to &Connection. commands/mod.rs and utils/mod.rs were both missing `pub mod ai_doc_qa;` from the ai-documentation-Q&A merge (Nanle-code#718), so main.rs referenced modules that didn't exist. Without this, no CI job on this PR can even attempt to compile the crate.
…ions src/lib.rs and src/main.rs carried a blanket matching [lints] table listing 16 individually-named lints. Together these silenced every default Clippy lint group (correctness, suspicious, style, complexity, perf) plus dead_code/unused_imports/ unused_variables across the entire crate — not just the handful of patterns the comments claimed to justify. Removing both surfaced ~2021 warnings under `cargo clippy --all-features`. Of those: - 94% (1658) were the single mechanical `uninlined_format_args` style lint, applied via `cargo clippy --fix` (two passes to converge, after fixing one clippy suggestion that didn't actually compile: removing `.to_string()` from a String/&str comparison in audit.rs that only worked because it allocated a matching String). - ~160 more were fixed by hand: manual_clamp, ptr_arg (with the attendant &PathBuf/&Path .clone() semantics fix at each call site), needless_range_loop, vec_init_then_push, if_same_then_else (two genuine redundant-branch merges), a should_implement_trait rename, a handful of type_complexity type aliases, and other one-off style fixes. - One of those fixes was a real bug, not just style: a constructed template changelog entry was built and then silently discarded (`changelog: None` instead of `Some(changelog)`) in templates.rs — vec_init_then_push flagged the construction, but the value was never used at all. - The remaining 55 (26 too_many_arguments, 29 dead_code) are now scoped #[allow(...)] on the specific function/struct/field that needs it, each with a comment explaining why, instead of a crate-wide suppression that hid everything else behind them. Also fixes two default-build-only warnings that were only ever hidden by the same blanket: hardware_wallet.rs's Ledger APDU codec helpers are dead when built without --features hardware-wallet and without tests (now an honest, scoped cfg_attr instead of silence), and profiler.rs had a genuinely pointless `#[cfg(not(feature = "memory-profiling"))] let memory_tracker = None;` plus its supporting placeholder struct, neither of which anything ever used — deleted outright rather than allowed. `cargo clippy --all-features --locked -- -D warnings` (exactly what CI runs) now passes with zero warnings, with every remaining exception scoped and documented rather than silenced project-wide. Updates CODE_STYLE_STANDARDS.md's "Project-Specific Allowances" section, which previously described the now-removed blanket as if it only covered a few narrow cases.
CI's clippy job pulled Rust/Clippy 1.98.0 (dtolnay/rust-toolchain@stable
tracks whatever's current), ~9 minor versions ahead of the 1.89.0
toolchain this branch was originally validated against locally.
Clippy periodically adds new lints to its default groups between
releases, and clippy::all being removed (previous commit) means every
one of those newly-added lints is now live on this crate too.
Installed the matching stable toolchain locally and fixed everything
it flagged that 1.89 didn't know about: unnecessary_sort_by (7,
mostly `.sort_by(|a,b| b.x.cmp(&a.x))` → `.sort_by_key(|a|
Reverse(a.x))` after auto-fix declined the ones needing Reverse),
manual_checked_ops (2, `if total > 0 {x/total} else {0}` →
`x.checked_div(total).unwrap_or(0)`), explicit_counter_loop (a
manually-incremented line counter replaced with `(1u32..).zip(lines)`),
and unnecessary_unwrap (an `if x.is_some() { x.unwrap() }` replaced
with `if let Some(x) = x`). The rest (useless_borrows_in_formatting,
collapsible_match, derivable_impls, filter_next, manual_is_multiple_of,
useless_format) were machine-applicable via `cargo clippy --fix`.
`cargo clippy --all-features --locked -- -D warnings` now passes clean
under both the 1.89.0 toolchain this branch started on and the
1.98.0 stable CI actually runs.
The should_implement_trait rename (SkillLevel::from_str -> parse_lenient, in the first commit of this branch) only searched src/ for call sites. tests/template_recommendation.rs — a top-level integration test file — had two more, which broke its compilation. Found via CI's deploy-verify job actually attempting to compile it.
a02bab9 to
f316e97
Compare
Closes #651.
Objective
Restore useful compiler and Clippy signals while documenting necessary local exceptions.
What was there before
src/lib.rsandsrc/main.rsboth carried:#![allow(dead_code, unused, clippy::all)]clippy::alldisables essentially every default Clippy lint group (correctness, suspicious, style, complexity, perf) for the whole crate.unuseddisablesunused_imports/unused_variables/etc. crate-wide too. On top of that,Cargo.tomlhad a[lints]table individually naming 16 more lints as"allow", with a comment claiming they were "intentionally relaxed... to keepcargo clippy --all-targets -- -D warningsgreen" — but theCargo.tomltable was almost entirely redundant with the much bigger hammer already swinging inlib.rs/main.rs.Net effect: nobody had real Clippy signal on this crate.
cargo clippy --all-featureslooked clean not because the code was clean, but because nearly everything was silenced.What changed
Removed both the crate-root
#![allow(...)]and theCargo.toml[lints]table entirely. That surfaced 2021 warnings undercargo clippy --all-features. Here's what happened to them:94% (1658) were
uninlined_format_args—format!("{}", x)vsformat!("{x}"), purely cosmetic. Applied viacargo clippy --fix --all-features(took two passes to converge, and one manual fix first: the initial--fixattempt failed wholesale because one of its own suggestions didn't actually compile — removing.to_string()from aString < &strcomparison inaudit.rsthat only worked because it allocated a matchingString;Stringdoesn't implementPartialOrd<&str>directly. Fixed that one by hand with.as_str()first, then the rest of the batch applied cleanly).~160 more fixed by hand, grouped by lint:
manual_clamp(8) —.max(a).min(b)→.clamp(a, b).ptr_arg(8) —&PathBuf/&mut Vec<T>params narrowed to&Path/&mut [T]. This one bit back: two call sites didpath.clone()expecting an ownedPathBuf(which works on&PathBufvia method-resolution autoderef) — after narrowing to&Path,.clone()silently started returning&Pathinstead (copying the reference, sincePathisn'tClone), which the compiler caught immediately as a type mismatch. Fixed with.to_path_buf().needless_range_loop(6) —for j in a..b { lines[j] }→ iterate a slice directly.vec_init_then_push(3) — one of these was a real bug, not just style:templates.rsbuilt achangelog: Vec<ChangelogEntry>with an "Initial release" entry viaVec::new()+.push(), then setTemplateEntry { changelog: None, ... }right below it, completely ignoring the local variable. Fixed tochangelog: Some(changelog). (The other two multi-push sites got a scoped#[allow(clippy::vec_init_then_push)]instead — 8 and 2 sequential multi-line struct-literal pushes read far more clearly than one giantvec![]literal.)if_same_then_else(2) — both were "two different conditions, same intentional outcome," not copy-paste bugs; merged with||.should_implement_trait(1) —SkillLevel::from_str(&str) -> Option<Self>was shadowing/confusable withFromStr::from_str(which returnsResult). Renamed toparse_lenientand updated its 2 call sites.type_complexity(4) — addedtypealiases for aRwLock<HashMap<...Box<dyn AIService>...>>struct field, a tuple-keyed telemetry aggregation map, a security-sensitive encrypted-bundle-parsing tuple return, and a publicVec<(String, String, i64, i64, i64, f64)>API return type.manual_strip,wildcard_in_or_patterns,doc_overindented_list_items,cloned_ref_to_slice_refs,only_used_in_recursion(dropped an unused&selffrom a recursive helper, converting it toSelf::topological_sort(...)), and 2 genuinely unused imports.The remaining 55 (26
too_many_arguments, 29dead_code) are now scoped#[allow(...)]on the exact function/struct/field that needs it, each with a comment explaining why — not a crate-wide suppression hiding everything else behind them.too_many_argumentsmostly affects CLI command handlers where each parameter is an independent named flag; bundling them into a config struct wouldn't reduce real complexity.dead_codeis honestly annotated as "not currently called from any code path in this crate; kept rather than removed since deleting it is a product decision, not a lint-scoping one" — I did not go delete two dozen functions I don't have full context on as part of a lint-policy PR.Two more warnings only surfaced in a default build (no
--all-features, no tests) and needed their own fix rather than--fix:hardware_wallet.rs's Ledger APDU codec helpers (constants +build_apdu/parse_hd_path/etc.) are only used by thehardware-walletfeature's transport code and by this module's own tests — genuinely unused in the one configuration that has neither. Added a module-level#![cfg_attr(not(any(test, feature = "hardware-wallet")), allow(dead_code))]with a comment explaining exactly that, instead of silence.profiler.rshad#[cfg(not(feature = "memory-profiling"))] let memory_tracker: Option<MemoryTracker> = None;plus a placeholderstruct MemoryTracker;that existed solely to give that dead assignment a type — neither was ever read when the feature was off. Deleted both outright rather than allowing them, since they served no purpose at all.Documentation
Rewrote
CODE_STYLE_STANDARDS.md's "Project-Specific Allowances" section, which previously described the (now-removed) blanket allowlist as if it only covered a handful of specific, narrow patterns. It now explains the scoped-exception policy, what to do (and not do) when adding one, and — as a concrete illustration of why blanket allows are dangerous — the changelog bug this cleanup found.Testing / verification
cargo clippy --all-features --locked -- -D warnings— exactly what CI'sclippyjob runs — passes with zero warnings.cargo build --locked(default features, matchingbuild-and-test's first step) passes with zero warnings (previously would have shown 15+, now that the blanket's gone — see thehardware_wallet.rs/profiler.rsfixes above).cargo fmt --checkon every touched file is clean (ranrustfmtacross the full touched-file set as part of this change, since a lint-quality PR is exactly the right place to also normalize formatting drift in the same files).cargo clippy --all-features --message-format=jsonand diffing the remaining warning list, to make sure nothing regressed and no fix introduced a new warning class.Scope note: master's pre-existing build breakage
Before any of the above could be verified, I had to get the crate compiling at all —
mastercurrently failscargo buildfor two small, unrelated, pre-existing reasons (already fixed standalone in #759, reapplied here as a prerequisite commit since this issue's work is fundamentally impossible to validate without a workingcargo clippy):database.rsusedthiserror::Error/#[error(...)]withoutthiserrorbeing a declared dependency, and passed&mut Transactionwhere theMigrationtrait expects&mut Connection(Transactionhas noDerefMut).commands/mod.rsandutils/mod.rswere both missingpub mod ai_doc_qa;from a recent merge (feat(ai): implement AI Documentation Q&A (#512) #718), somain.rsreferenced modules that didn't exist.Separately — and out of scope for this PR —
cargo clippy --all-targets(i.e. including test code) still hits a large, unrelated wave of pre-existing test-compile errors acrosstemplate_recommender.rs/template_analytics.rs/plugin registry code/etc. (missing struct fields, type mismatches). None of that is reachable bycargo clippy --all-features(no--all-targets, matching what CI'sclippyjob actually runs), so it doesn't block this PR, but it does meancargo test/cargo clippy --all-targetswon't be clean until that separate issue is addressed.