Skip to content

chore: lint test and bench targets with clippy - #770

Merged
jdx merged 1 commit into
jdx:mainfrom
JamBalaya56562:chore-clippy-all-targets
Aug 2, 2026
Merged

chore: lint test and bench targets with clippy#770
jdx merged 1 commit into
jdx:mainfrom
JamBalaya56562:chore-clippy-all-targets

Conversation

@JamBalaya56562

@JamBalaya56562 JamBalaya56562 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #763, which fixed the lint but deliberately left the gate alone.

Problem

lint:clippy runs

cargo clippy --all --all-features -- -D warnings

which covers lib and bin targets only. Tests and benches have never been linted. Nothing was watching them, so lint quietly accumulated: adding --all-targets surfaced 17 findings — 13 deprecated criterion::black_box calls in lib/benches/parse.rs, two clippy::unnecessary_get_then_check and two clippy::manual_contains in tests. #763 fixed all of them, but the same drift starts again the moment someone writes a test.

The #[cfg(test)] mod tests blocks are the sharpest case: the lib target does not enable cfg(test), so unit tests are invisible to the current command no matter which crate they live in.

Fix

One flag:

 [tasks."lint:clippy"]
-run = 'cargo clippy --all --all-features -- -D warnings'
+run = 'cargo clippy --all --all-features --all-targets -- -D warnings'

Newly covered: lib/benches/parse.rs, the integration tests under lib/tests/, cli/tests/ and clap_usage/tests/, and every #[cfg(test)] mod tests in the workspace.

That is the entire diff — the current main already passes --all-targets clean, so this changes no source.

Precedent

mise runs both forms in CI (.github/workflows/test.yml):

- name: Run Clippy
  run: |
    cargo clippy -- -D warnings
    cargo clippy --all-features --all-targets -- -D warnings

and its AGENTS.md states it as a rule: refactor until cargo clippy --workspace --all-features --all-targets -- -D warnings passes, without #[allow(clippy::…)] exclusions. This brings usage to the same bar.

lint-fix left alone, on purpose

render:usage-cli-completions ends with mise run lint-fix, and what render needs from it is cargo fmt and prettier -w — the clippy --fix is along for the ride. Widening it would add a full test and criterion build to every mise run render, including the CI step that follows it with a no-diff assertion, for nothing render cares about. cargo fix also has a known habit of bailing with "file has changed on disk" when one file is compiled as both a lib and a test target.

mise makes the same split: xtasks/lint-fix.ps1 is cargo clippy --fix --allow-staged --allow-dirty -- -Dwarnings, without --all-targets or even --all-features. Check wide, fix narrow.

Cost

mise r lint is the last step of the test job, after mise r build and mise r test. Clippy does not share fingerprints with cargo build/cargo test, so the extra targets are compiled rather than reused. The genuinely new dependency is criterion, for the bench — coverage.yml already builds it via cargo test --all-features --all-targets in tasks/coverage.sh, but that is a different job with a different cache.

Measured locally: 14 s warm on Windows, 1 m 22 s on Linux from a cache without criterion.

Verified

That the widened gate actually catches something, by planting a clippy::clone_on_copy in cli/tests/clap_sort.rs:

# before — passes
$ cargo clippy --all --all-features -- -D warnings
0 errors

# after — fails
$ cargo clippy --all --all-features --all-targets -- -D warnings
error: using `clone` on type `i32` which implements the `Copy` trait
  --> cli\tests\clap_sort.rs:12:17

Reverted afterwards. mise run lint:clippy and mise run lint:fmt pass on the unmodified tree, on Windows and on Linux.

Possible follow-up

mise runs a second, bare cargo clippy -- -D warnings alongside the --all-features one, which catches code that only compiles with a feature enabled. usage has three features on usage-lib, so the same gap exists here. Left out because it is a question about features rather than targets.


This pull request was generated by Claude Code.

Summary by CodeRabbit

  • Chores
    • Expanded lint checks to cover all project targets, improving consistency and code quality validation.

`lint:clippy` ran `cargo clippy --all --all-features -- -D warnings`, which
covers lib and bin targets only. Tests and benches were never linted, so lint
accumulated there unnoticed: turning `--all-targets` on surfaced 17 findings —
13 deprecated `criterion::black_box` calls in the bench, plus
`clippy::unnecessary_get_then_check` and `clippy::manual_contains` in tests.
Those were fixed in jdx#763, but nothing stops the same thing happening again
while the gate stays narrow.

mise runs both forms in CI (.github/workflows/test.yml):

    cargo clippy -- -D warnings
    cargo clippy --all-features --all-targets -- -D warnings

and its AGENTS.md makes `cargo clippy --workspace --all-features --all-targets
-- -D warnings` passing without `#[allow]` exclusions an explicit rule. This
brings usage to the same bar.

Newly covered: `lib/benches/parse.rs`, the integration tests under
`lib/tests/`, `cli/tests/` and `clap_usage/tests/`, and every `#[cfg(test)]
mod tests` — the last of which the lib target never enabled `cfg(test)` for.

`lint-fix` is deliberately left alone. `render:usage-cli-completions` calls it
for `cargo fmt` and `prettier -w`, and widening the `clippy --fix` inside it
would add a full test and criterion build to `mise run render` for no benefit.
mise keeps its fix side narrower than its check side for the same reason.
@coderabbitai

coderabbitai Bot commented Aug 2, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The lint:clippy task in mise.toml now runs Clippy across all Cargo targets while preserving the existing workspace feature and warning-denial settings.

Changes

Clippy target coverage

Layer / File(s) Summary
Clippy task configuration
mise.toml
The lint:clippy task now includes all Cargo targets in its Clippy check.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Possibly related PRs

  • jdx/usage#763: Fixes warnings in test and benchmark targets covered by the expanded Clippy check.

Suggested reviewers: jdx

Poem

A rabbit checks each target bright,
Clippy hops through code tonight.
Warnings tucked in tests appear,
The lint task makes them clear.
One small change, the burrow gleams.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: extending Clippy linting to test and benchmark targets.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@JamBalaya56562
JamBalaya56562 marked this pull request as ready for review August 2, 2026 02:17
@greptile-apps

greptile-apps Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR broadens the Clippy lint gate to include test and benchmark targets.

  • Adds --all-targets to the lint:clippy Cargo invocation.
  • Leaves the narrower automatic-fix workflow unchanged as documented.

Confidence Score: 5/5

The PR appears safe to merge because the lint-only change introduces no actionable failure.

The changed command broadens static-analysis coverage to workspace tests and benchmarks, while the repository configuration provides the corresponding targets and development dependencies.

Important Files Changed

Filename Overview
mise.toml Extends Clippy coverage to all Cargo targets without introducing a concrete correctness or workflow defect.

Reviews (1): Last reviewed commit: "chore: lint test and bench targets with ..." | Re-trigger Greptile

@jdx
jdx merged commit d4034a7 into jdx:main Aug 2, 2026
8 checks passed
@JamBalaya56562
JamBalaya56562 deleted the chore-clippy-all-targets branch August 2, 2026 02:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants