feat(cargo-unused-deps): complete dependency analysis - #188
martin-kolinek wants to merge 25 commits into
Conversation
Replace cargo-udeps with the manifest-only cargo-unused-deps check, wire its published 0.1.0 release into setup and impact scoping, and regenerate the dogfood artifacts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3757bb12-095a-4879-9fcd-1cf7a62c8807
There was a problem hiding this comment.
🔵 Needs a closer look
It changes the default dependency-validation contract and regenerates cross-backend CI artifacts, so final human review is warranted.
Pull request overview
Migrates Anvil’s dependency check from cargo-udeps to the published manifest-level cargo-unused-deps tool.
Changes:
- Adds stable, modified-scope
unused-depsrecipes and tool pinning. - Removes legacy
udepsrecipes and updates templates, workflows, snapshots, and docs. - Removes four uninherited workspace dependencies.
File summaries
| File | Description |
|---|---|
README.md |
Documents the new dependency check. |
Cargo.toml |
Removes unused workspace catalog entries. |
.anvil.lock |
Updates generated artifact checksums. |
justfiles/anvil/* |
Regenerates local Anvil recipes. |
crates/cargo-anvil/templates/* |
Updates Anvil source templates. |
crates/cargo-anvil/src/lib.rs |
Updates documented tool list. |
crates/cargo-anvil/src/anvil/artifacts/justfile.rs |
Registers the new check and scope. |
crates/cargo-anvil/tests/recipe_contracts.rs |
Adds recipe contract coverage. |
crates/cargo-anvil/tests/snapshots/* |
Updates emitted-tree snapshots. |
crates/cargo-anvil/README.md |
Regenerates crate documentation. |
crates/cargo-anvil/docs/design/* |
Documents the behavior and scope change. |
crates/cargo-unused-deps/docs/design/README.md |
Marks CI integration as adopted. |
.github/workflows/* |
Regenerates workflow comments and wiring. |
Review details
- Files reviewed: 35/36 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (97.6%) is below the target coverage (100.0%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #188 +/- ##
======================================
Coverage 97.6% 97.6%
======================================
Files 304 307 +3
Lines 69683 70468 +785
======================================
+ Hits 68016 68807 +791
+ Misses 1667 1661 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Builds steps 2 and 3 of the design on top of the catalog check. Compile evidence comes from rustc, not from parsing: the tool enables `unused_crate_dependencies` for its own `cargo check --all-targets` and reads the JSON diagnostics. The lint is set for that invocation only and built into its own target dir, because the raw lint fires per unit -- in the shared lint catalog it would warn on every correct build and fail under `-D warnings`. Aggregation is the part that turns those per-unit warnings into an answer. Cargo compiles a library or binary twice under `--all-targets`, plainly and with cfg(test), and their diagnostics are indistinguishable, so the units are counted instead: the test-profile unit compiles a superset of the plain unit's code, so when only one of them reports it can only be the plain one. That is what lets a dependency used solely from `#[cfg(test)]` code come out as misplaced rather than unused. Doctests are covered by standing in for the compiler rustdoc uses. `--test-builder` points at this binary, which runs the real rustc with the lint on and keeps the diagnostics rustdoc discards on success. Two things learned by measurement: the shim must inherit stdin, because rustdoc feeds it the snippet there, and it must leave the error format alone, because handing rustdoc JSON makes it treat every doctest as failed. Attribution is per package, since a snippet on stdin carries no crate identity -- and bin-only packages are skipped, because asking cargo for their doctests is an error rather than an empty answer. Package selection is cargo's own (-p/--workspace/--exclude), forwarded verbatim to both child builds, while the catalog check always reads every member: a subset cannot answer "no member inherits this". Verified against this repository: the catalog is clean and exactly one finding comes back -- `ohno`, declared by cargo-aprz and named nowhere in its sources. `cargo udeps -p cargo-aprz` reports "All deps seem to have been used", so this is a real defect its filename heuristic misses. Not yet implemented, and marked so in the design: the optional/feature check, and `--fix` for a misplaced dependency. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Review question: with several bin targets, or a lib plus bins, can the counting rule convict a dependency that production code uses? Not as implemented -- counters are keyed per target, and "used" is the union across them -- but the design doc described the fold as one table of dependency x unit kind, which reads exactly like the pooling that would be wrong. Corrected, and pinned with a fixture that would fail under pooling: a package with a library and two binaries where each dependency is used in production by exactly one target, so every other target reports it unused. liponly used by the lib -> reported unused by both binaries binonly used by binary one -> reported unused by the lib and binary two bintest used by one's cfg(test) -> misplaced Only `bintest` is reported. Also verified idempotence: with a warm target directory cargo replays cached diagnostics, and artifacts replay with them, so the ratios and the verdicts are unchanged across runs. The doc now states the rule as counting per target, the monotonicity argument that justifies reading a lone report as the plain unit's, and why pooling across targets would be wrong. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Review question: can multiple test targets skew the counts? The per-target keying already handles that -- several tests, benches and examples each testify separately, verified -- but chasing it turned up a real false positive one step over. "Compiled twice" is not a property of libraries. A test, bench or example declared `test = true` is also built plainly and under cfg(test), and the development branch treated any report at all as "unused". So an example whose cfg(test) code is the only user of a dev-dependency was convicted: the plain unit reports, the test-profile unit does not, and one report was enough. Development targets now use the same rule as libraries -- fewer reports than units means some unit loaded it. Both units of a development target have dev-dependencies in scope, so no scope distinction is needed there; the distinction still matters for libraries and binaries, where a dev-dependency is in scope only for the cfg(test) unit and a lone report is therefore that unit's. Two regression tests: five development targets each used by exactly one dependency, and an `[[example]] test = true` whose cfg(test) code is the sole user. The second fails before this change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Pushing on "one example does not prove the rule" found the assumption that was wrong. The claim was that the cfg(test) unit compiles a superset of the plain unit's code, so a single report out of two units had to be the plain one's. `#[cfg(not(test))]` code is excluded from the test unit, so it is not a superset. A dependency used only from cfg(not(test)) code is used by the library and reported by the cfg(test) unit -- and the tool moved it to [dev-dependencies]. Reproduced before fixing. The inference is gone. Compile evidence is now gathered twice: once over default targets, where each code target has exactly one unit and a report can only be that unit's, and once over --all-targets. "Did the library use it" reads the first; "did anything use it" reads the second, where `reports < scope` is not an assumption but a restatement of the lint's semantics -- a unit reports exactly when the dependency was in scope and went unused. Both passes share a target directory, so the second reuses the first's artifacts. Adds the counter-example as a regression test, and rewrites the design's rule section to separate the two questions, name the run each is read from, and record why the superset argument is false. Verified: 13 evidence tests, 28 catalog tests, and this repository still reports exactly one finding. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Benchmarked against microsoft/oxidizer (72 crates, 618 locked packages) and the doctest phase dominated: one `cargo test --doc -p <pkg>` invocation cost 122s warm, and the tool was running one per package. Doctest evidence can only ever spare a dependency, never accuse one, so it is not needed for a package that produced no finding. The tool now judges once without it, compiles the doctests of only the accused packages, and judges again. Same verdicts, far less work: on oxidizer that is 5 doctest builds instead of 72. Cold run on oxidizer: 1118s -> 431s, identical output. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Applying the tool's own findings to microsoft/oxidizer and running its test suite found a false positive it had produced: removing `uuid` from http_extensions broke a doctest. Same aggregation bug as the one fixed for targets, in the last place it survived. Every doctest is a separate compilation with the package's dev-dependencies in scope, so each doctest that does not mention a dependency reports it unused. http_extensions has about 175 doctests and one of them uses `uuid`; recording "some doctest reported it" as disuse convicted it. Doctest evidence now counts, like everything else: fewer reports than doctests compiled means some doctest used it. Three of the five reported dependency problems on oxidizer were this bug -- uuid, foldhash and prost-types are all doctest-only. The remaining two survive verification: `fetch` uses `bytes` only from tests, and `rest_over_grpc_tests` never uses `http-body`. Verified end to end: with the 14 catalog removals, `bytes` moved to dev-dependencies and `http-body` deleted, oxidizer's full `cargo test --workspace --all-features` passes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
`liponly` was meant to be `libonly` -- the crate in the multi-target fixture that only the library uses. The name appears in the test and in the measured table in the design doc, so both are corrected. No behaviour change; the fixture crate is generated by the test, so the name is arbitrary, but a misspelled one makes the table harder to read than it needs to be. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add compiler and doctest evidence for unused and misplaced declarations. Keep the workspace catalog check global while Cargo-style package selectors scope only the per-package analysis. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3757bb12-095a-4879-9fcd-1cf7a62c8807
This reverts commit 1f031d1.
Always run the workspace catalog check, scope compiler evidence only to explicit Cargo-style package selectors, and cover the zero-package impact case. Harden the evidence tests, preserve strict coverage, and apply the findings to ox-tools. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3757bb12-095a-4879-9fcd-1cf7a62c8807
There was a problem hiding this comment.
🟡 Changes recommended
Package-scoped analysis can judge transitive workspace members, and several diagnostics, documentation, and release paths need correction.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
crates/cargo-unused-deps/src/lib.rs:259
runnow also returns failure when selected compile-backed checks find unused or misplaced dependencies (lines 281-292), but this contract still describes success solely in terms of catalog inheritance. Update the public docs to include the selected source checks so callers do not misinterpret a source-level failure.
/// Returns [`ExitCode::SUCCESS`] when every catalog entry is inherited by at
/// least one member -- or, under `--fix`, once the entries that were not have
/// been removed -- and [`ExitCode::FAILURE`] otherwise. Returning an exit code
- Files reviewed: 14/15 changed files
- Comments generated: 8
- Review effort level: Lite
Track explicitly selected package roots, render compiler failures, include proc-macro doctests, separate source suppressions from stale catalog entries, and keep the stable mutation path meaningful. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3757bb12-095a-4879-9fcd-1cf7a62c8807
Run the repository release script to generate release metadata for the complete dependency analysis release. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3757bb12-095a-4879-9fcd-1cf7a62c8807
There was a problem hiding this comment.
🟡 Changes recommended
Shared evidence artifacts can produce incomplete diagnostics, and doctest evidence does not preserve duplicate package identities.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
crates/cargo-unused-deps/Cargo.toml:7
- This changes the published version to 0.2.0 while
crates/cargo-unused-deps/CHANGELOG.mdstill contains only its header. The repository release procedure saysrelease-crate.ps1bumps the version and generates the changelog (README.md:79-80); please perform that release step, or leave the version bump for the dedicated release PR, so the published 0.2.0 has release notes.
version = "0.2.0"
- Files reviewed: 16/17 changed files
- Comments generated: 3
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Evidence target directories are reused across runs without replayed lint diagnostics, while selector and doctest-wrapper behavior also diverge from the documented contract.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
crates/cargo-unused-deps/src/doctests.rs:90
- The doctest shim invokes
RUSTCdirectly and never chains an existingRUSTC_WRAPPER. The compile-evidence path explicitly preserves that wrapper incrates/cargo-unused-deps/src/evidence.rs:166-169, so a caller with a custom compiler wrapper gets different compiler behavior (or loses required wrapper setup) during doctest collection. Preserve the wrapper chain here as well.
crates/cargo-unused-deps/src/lib.rs:365 - These selectors only match a literal name or an exact full
name@version. The repository's existing Cargo-style selector implementation accepts Unix globs and partial version qualifiers (crates/cargo-each/src/select.rs:102-111and:134-151), so valid selectors such astokio-*orfoo@0.1are rejected here even though Cargo accepts them. Match the same package-spec semantics, with corresponding integration coverage.
- Files reviewed: 16/17 changed files
- Comments generated: 3
- Review effort level: Lite
Force the rustdoc test-builder shim to emit stable human, no-color diagnostics before parsing unused dependency names. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3757bb12-095a-4879-9fcd-1cf7a62c8807
Regenerate the 0.2.0 changelog after diagnostic normalization. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3757bb12-095a-4879-9fcd-1cf7a62c8807
There was a problem hiding this comment.
🟡 Changes recommended
Reused evidence artifacts can suppress findings on repeated runs, and selector/documentation contracts are inconsistent.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
crates/cargo-unused-deps/src/lib.rs:365
resolve_selectorsonly matches literal names or a completename@versionstring. This rejects the repository's existing Cargo-style selector forms such astokio-*and partial versions likefoo@0.1, even though the siblingcargo-eachcontract documents and implements them (crates/cargo-each/src/cli.rs:33-45,crates/cargo-each/src/select.rs:99-145). An impact or user invocation using these selectors fails before Cargo runs; implement the same matching semantics or narrow/document this CLI as non-Cargo-style.
crates/cargo-unused-deps/src/lib.rs:385
- These target directories are stable for the workspace, so a second
source_checksinvocation reuses the first run's artifacts. The design explicitly notes that Cargo emitscompiler-artifactfor cached units without replaying lint diagnostics (docs/design/README.md:422-426);parsestill incrementsunits, whileEvidence::used_by_plain_unittreats zero reports as use (src/evidence.rs:96-99). Thus the repeated-analysis case intests/evidence_tests.rs:652-658can lose an unused finding. Use a fresh target directory per analysis (or force fresh compiler diagnostics), not just separate directories for the two passes.
let plain_target_dir = workspace.evidence_target_dir.join("plain");
let all_target_dir = workspace.evidence_target_dir.join("all-targets");
crates/cargo-unused-deps/docs/design/README.md:276
- This section says the tool has no stable-toolchain mode, but the CLI deliberately supports a stable catalog-only invocation when no selector is supplied (
README.md:35-39, 56-60; design:556-564). That makes the design contract self-contradictory and can lead users to run nightly unnecessarily; qualify the statement to the compiler-backed checks.
There is exactly one source of truth about source-level use: **what the compiler
loaded**. The tool requires a nightly toolchain and gathers that evidence for every
unit, doctests included. It does not parse Rust, and it has no stable-toolchain mode —
a second, weaker analysis to fall back on would double the code and halve the
confidence in the result.
- Files reviewed: 16/17 changed files
- Comments generated: 1
- Review effort level: Lite
Use fresh target directories across invocations, key doctest evidence by package identity, narrow selector semantics, filter failure output to errors, and align public documentation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3757bb12-095a-4879-9fcd-1cf7a62c8807
Regenerate the 0.2.0 changelog after evidence isolation fixes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3757bb12-095a-4879-9fcd-1cf7a62c8807
There was a problem hiding this comment.
🟡 Changes recommended
Target-specific dependency findings lose their declaration path and are reported as unconditional [dependencies] entries.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
crates/cargo-unused-deps/docs/design/README.md:458
- This rationale is internally contradictory: it claims the
cfg(test)unit is a superset of the plain unit, but the later explanation correctly notes that#[cfg(not(test))]code is absent from the test unit. The false superset claim makes the one-report attribution rule misleading; the separate default-target pass, not monotonicity, is what identifies the plain unit.
crates/cargo-unused-deps/docs/design/README.md:617 - The exit-code table immediately above still describes only catalog-level unused entries, but this implementation also returns failure for selected packages' unused/misplaced findings and returns an error when compile or doctest evidence collection fails. Update the table so the design documents the new package-check failure paths introduced by this PR.
crates/cargo-unused-deps/src/doctests.rs:105 - This comment says the error format is left alone, but
stable_diagnostic_argsremoves inherited--error-format,--color, and--jsonoptions andshimappends--error-format=human --color=never. Reword the comment to describe that normalization; otherwise it contradicts the behavior the parser relies on.
- Files reviewed: 16/17 changed files
- Comments generated: 1
- Review effort level: Lite
Preserve target-table predicates in findings, align shim documentation, and avoid the spellcheck tokenization failure in package-selector help. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3757bb12-095a-4879-9fcd-1cf7a62c8807
Regenerate the 0.2.0 changelog after target-context reporting. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3757bb12-095a-4879-9fcd-1cf7a62c8807
There was a problem hiding this comment.
🔵 Needs a closer look
Selected-check filtering can trigger unrelated doctest failures, and the --fix documentation overstates concurrent-edit safety.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
crates/cargo-unused-deps/src/lib.rs:94
write_backonly re-reads the target and then performspersist/rename; an edit that lands between those operations can still be overwritten. The new documentation therefore overstates the guarantee by saying a concurrent edit is never clobbered. Restore the prior race-window caveat or add an OS-level lock/conditional replacement before making this claim.
crates/cargo-unused-deps/src/lib.rs:406candidatescontains findings of both kinds before the laterchecksfilter, so--check misplacedstill runscargo test --docfor packages whose only candidates are unused dev/build declarations. Becausedoctest_evidencepropagates doctest failures, an unselected unused finding can make this invocation fail and it also defeats the requested check's cost boundary. Gather all candidates whenunusedis selected, but otherwise only normal declarations, since doctests can only turn those intoMisplaced.
crates/cargo-unused-deps/docs/design/README.md:561- The new package-selection contract is documented here, but the design's exit/reporting contract below still describes code 1 only for uninherited catalog entries and manifest errors.
runnow also returns failure for selected unused or misplaced declarations (and evidence-collection errors), so the design should update its exit-code and reporting sections to match the implemented source checks.
- Files reviewed: 16/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
Prevent PowerShell#26940 assembly-loader crashes by serializing recipe-contract interpreter startups under libtest and nextest while preserving intentional concurrency inside individual fixtures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3757bb12-095a-4879-9fcd-1cf7a62c8807
There was a problem hiding this comment.
🔵 Needs a closer look
Documentation currently overstates concurrent-edit protection and JSON validation behavior compared with the implementation.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
crates/cargo-unused-deps/src/evidence.rs:161
- This error contract is broader than the implementation:
parsefilters to lines beginning with{at lines 262–263 and the test at lines 359–368 explicitly accepts anot jsonline. Narrow the documentation to malformed JSON message lines (or change the parser if all non-JSON output should be rejected), so callers are not promised strict validation that the code does not perform.
crates/cargo-unused-deps/src/lib.rs:94 - The new wording promises that a concurrent edit is never clobbered, but
write_backonly compares the manifest at lines 598–603 and renames the replacement at line 623. An edit after that final read (or after a member recheck) can still be overwritten, so please narrow this to a best-effort pre-write check and keep the generated README/design wording consistent.
- Files reviewed: 18/19 changed files
- Comments generated: 0 new
- Review effort level: Lite
Apply the generated nightly rustfmt output required by the fast check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3757bb12-095a-4879-9fcd-1cf7a62c8807
There was a problem hiding this comment.
🔵 Needs a closer look
Public and design documentation still omit the new compiler-backed error, reporting, and exit behavior.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
crates/cargo-unused-deps/docs/design/README.md:561
- This new package-level contract is not reflected in the exit/reporting contract below: the table still describes code 1 only for uninherited catalog entries, and the reporting text still promises one-line unused-entry output.
source_checksalso returns failure for selectedunused/misplacedfindings and emits multi-line remediation blocks, so update those sections to cover the compiler-backed checks.
crates/cargo-unused-deps/src/lib.rs:299 - The new
source_checkscall makesrunpropagate compiler and doctest evidence-collection errors, but the publicrun# Errorscontract above still lists only manifest, workspace-enumeration, and fix failures. Document thecargo check/cargo test --docfailure paths so callers can distinguish evidence errors from ordinary findings.
- Files reviewed: 18/19 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
|
🤖 Pato's Pull Request Agent: [design/correctness] Feature-forwarding optional dependencies are reported as unused.
[dependencies]
serde = { version = "1", optional = true }
[features]
derive = ["serde/derive"] # or a bare `dep:serde`the The README/design frame the allow-list around side-effect deps ("an allocator or |
🤖
Summary
cargo-unused-depsfrom the workspace-catalog check into a replacement forcargo-udepsandcargo-macheteunused_crate_dependenciesdiagnostics from default-target and all-target passes to detect unused and misplaced declarations-p/--package,--workspace, and--excludeselection for package-level checkscargo-unused-depsto 0.2.0 and apply its findings to the repositoryPackage selection contract
Package selectors scope only compiler-backed checks. With no selector, the tool runs the catalog check without compiling packages. This lets a future Anvil recipe invoke the tool even when impact returns
--skip; a nonemptyrequiredimpact set can be forwarded as repeated--packagearguments, while an unscoped run uses--workspace.Release sequencing
Anvil adoption is intentionally deferred until 0.2.0 is published. Generated Anvil setup installs its pinned tool from crates.io, so adopting the complete behavior in this same PR would make dogfood CI execute the catalog-only 0.1.0 release.
Validation
cargo-unused-depsand the adjustedcargo-aprzmanifest