feat: add cargo-unused-deps - #102
Conversation
`cargo udeps` resolves the crate graph and asks which declared dependencies go unreferenced, so a `[workspace.dependencies]` entry that no member inherits is invisible to it -- it never enters the graph at all. That blind spot accumulated 48 stale entries before PR #99 swept them out by hand, with udeps green throughout. Adds the design doc for a new sibling gate that closes it, named to match the existing `cargo-ensure-no-cyclic-deps` and `cargo-ensure-no-default-features` check tools. The rule is manifest-only: an entry is unused when no workspace member declares it with `workspace = true`, across dependencies, dev- and build-dependencies and their `[target.'cfg(...)']` forms, in both the inline and dotted spellings. That keeps the gate free of false positives and cheap enough for the text/metadata tier -- no compilation, no toolchain pin, no network. cargo-shear was evaluated and rejected as the vehicle: it does implement an unused-workspace-dependency diagnostic, but derives it from static source-usage analysis, so its verdict inherits that analysis's macro-expansion blind spots -- it reports eight entries here that are inherited and genuinely used through macro arguments. Design only; no crate skeleton and no anvil wiring yet. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
`members` globs `crates/*`, so the new crate directory -- which holds a design doc and no manifest yet -- was read as a workspace member with an unreadable `Cargo.toml`. Every cargo invocation failed at metadata time, which is why the whole check suite went red on a docs-only change. The design-docs-first workflow lands the design before the code, so the gap between doc and manifest is expected rather than accidental. Excludes the directory until the crate lands, at which point the entry goes away. `Cargo.lock` is unaffected: the excluded directory contributes no package. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (99.9%) 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 #102 +/- ##
======================================
Coverage 99.9% 99.9%
======================================
Files 135 139 +4
Lines 17470 17797 +327
======================================
+ Hits 17469 17796 +327
Misses 1 1
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:
|
The design doc alone cannot live under `crates/`: `members` globs `crates/*`, so a directory without a manifest breaks `cargo metadata`, and `cargo sort --workspace` walks the same glob itself -- it ignores the workspace `exclude` that would otherwise paper over it, and 2.1.4 (pinned, and the latest release) has no ignore flag. So the crate lands as a skeleton and the design doc keeps its final path. Following `automation`, the crate is `publish = false` -- there is nothing worth releasing until the implementation exists -- and carries the documented `min-lines-percent = 0.0` coverage opt-out, since a crate with no executable code produces no instrumented regions and would otherwise be graded NO DATA. The implementation change removes both. Replaces the workspace `exclude` added in the previous commit. Verified locally: cargo metadata, cargo sort --check --check-format, clippy -D warnings, rustdoc -D warnings, fmt --check, cargo heather, ensure-no-default-features, ensure-no-cyclic-deps, and cargo-spellcheck all pass, and `cargo anvil --dry-run` reports nothing to write. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
martin-kolinek
changed the title
Fills in the skeleton with the check itself: read the workspace root's `[workspace.dependencies]`, enumerate members with `cargo metadata --no-deps`, and report every catalog entry that no member declares with `workspace = true`. `--fix` removes them through `toml_edit`, carrying a removed entry's comments to the next survivor so a group header keeps labeling the group it introduces. Addresses review feedback on the design: a manifest with no `[workspace]` table is no longer an error. cargo-anvil manages single-crate repositories too, and a generated recipe runs the same command everywhere, so a hard error would make the check unusable in exactly the repositories that never had the problem. It now reports the situation and succeeds -- the property holds vacuously -- with `--require-workspace` restoring the strict reading for callers that know they are pointing at a workspace root. Two unreachable paths were removed rather than left uncovered: `remove` takes the catalog table for granted (callers only fix a catalog they already read entries from) and the write-failure context is formatted eagerly instead of in a closure no test can portably reach. The package is at 100% line and function coverage. Verified end to end against this repository: it reports all 70 catalog entries as inherited, flags an injected entry, and `--fix` removes it and restores the manifest byte for byte. The crate is now publishable, so the skeleton's `publish = false` and its coverage opt-out are gone. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🤖 Clawpilot here! Posted automatically by Clawpilot (an AI agent), not by a human. Please verify before acting. Agreed, and changed — the missing- Your reasoning is the deciding one: the generated anvil recipe runs the same command in every repo it manages, including single-crate ones. A hard error there would fail exactly the repositories that never had the problem, and each would need a local opt-out to get back to green. Succeeding is also the honest answer — "no catalog entry goes uninherited" holds vacuously when there is no catalog. Implemented as you suggested, with the switch inverted so the lenient reading is the default:
The rest of the crate is implemented in the same push ( Leaving this thread unresolved for you to close. |
martin-kolinek
changed the title
|
`cargo mutants` caught a real gap: deleting the `!` in `remove`'s `else if !carried.is_empty()` survived the suite. With that mutation the carry-forward never fires, but the pending comments are not lost -- the trailing-block path still appends them after the last surviving entry, so the comment remains somewhere in the file and a `contains` assertion stays green while the comment has silently left its group. The test now pins the position: the carried comment must precede the next surviving entry's own decor. That is the property the code exists to provide, and it fails under the mutation. 43 mutants, 41 caught, 2 unviable, 0 missed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| // Formatted eagerly rather than in a `with_context` closure: the closure | ||
| // only runs when the write fails, which no test can force portably. | ||
| let failure = format!("failed to write {}", manifest_path.display()); | ||
| std::fs::write(manifest_path, manifest.to_string()).context(failure)?; |
There was a problem hiding this comment.
🤖: std::fs::write truncates the target before writing, so an interrupted --fix (crash, Ctrl-C, full disk) leaves the workspace root manifest truncated — the one file whose loss breaks every other tool in the repo, and the only file this tool ever writes.
That is a fair tradeoff for the single-shot CI path, but the crate is published for use outside this repository, where --fix runs interactively against a possibly dirty tree. The house pattern is already here: cargo-anvil writes temp-then-fs::rename (crates/cargo-anvil/src/manifest.rs:234, crates/cargo-anvil/src/plan.rs:557), tempfile is already a dev-dependency, and NamedTempFile::persist into the manifest's own directory keeps the rename on one filesystem.
Worth adding the unchanged-input guard at the same time: the manifest is read, then cargo metadata runs (a subprocess, so the window is not small), then the document is written back — an editor save landing in that window is silently overwritten. Comparing the bytes read against the bytes on disk before the rename closes it.
There was a problem hiding this comment.
🤖 Clawpilot here! Posted automatically by Clawpilot (an AI agent), not by a human. Please verify before acting.
Fixed in db97acb — --fix no longer truncates in place.
The write now goes to a NamedTempFile in the manifest's own directory and is persisted with a rename over the original, matching the cargo-anvil pattern you pointed at (manifest.rs:234 / plan.rs:557). tempfile moved from dev-dependencies to dependencies.
The unchanged-input guard is in too: the manifest is re-read immediately before the rename and compared against the bytes that were parsed, so an edit landing during the cargo metadata child process aborts the fix instead of being overwritten.
<path>/Cargo.toml changed on disk while the check was running; not writing
That guard is not reachable from an integration test without racing a child process, so it is covered by two unit tests in lib.rs — write_back_replaces_a_manifest_that_is_unchanged and write_back_refuses_a_manifest_that_changed_under_it, the second asserting the competing edit is still on disk afterwards.
There was a problem hiding this comment.
[Copilot speaking]
Align the concurrent-edit guarantee with the actual write guard
The README and crate documentation say that --fix refuses every manifest change made after the initial read and therefore never overwrites a concurrent edit. write_back compares the file once, then reads permissions, creates and writes the staged file, applies permissions, and finally persists it over the target.
Reproducible reasoning: The unchanged-input comparison at lines 267–272 detects edits visible at that moment. It is not coupled atomically to the replacement at line 283, so a writer can change the target after the comparison and before persist; the rename then replaces that newer content. Atomic replacement prevents a partially written manifest, but it does not make the preceding comparison a conditional compare-and-replace.
Consequence: An editor or automation update made in the comparison-to-persist window can be lost despite the published guarantee that concurrent edits are never overwritten.
Recommended action: Align the documented contract and implementation in whichever direction the project intends. Either narrow the documentation to the edits the pre-write comparison can observe, or use a platform-appropriate conditional replacement or coordination mechanism that protects the comparison-to-replacement boundary against non-cooperating writers. State atomic replacement and concurrent-edit detection as separate guarantees.
References:
- crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs
- crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md
- crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md,
--fixreplacement discussion - Prior write-safety review thread
Impacted locations:
crates/cargo-ensure-no-unused-workspace-deps/README.md:65-69crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs:59-63crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs:267-283
Addresses seven review comments. `--fix` no longer truncates the workspace root in place. It writes a temporary file in the manifest's own directory and renames it over the original -- the house pattern from cargo-anvil -- so an interrupted run cannot leave the one file that breaks every other tool in the repo truncated. Before the rename the manifest is re-read and compared against the bytes that were parsed: `cargo metadata` runs in between as a child process, and an editor save landing in that window now aborts the fix instead of being silently overwritten. An empty catalog no longer skips the stale allow-list report. That is the boundary where *every* allowed name suppresses nothing, so it is exactly where the documented contract mattered most. Carried comments are now reported. The carry-forward cannot tell a group header from a note about one specific dependency, so a note about a removed entry lands on the next survivor and reads as if it were about that one -- worse than dropping it, because a dropped comment is visible in the diff and a wrong attribution is not. The relocation is printed on stderr, naming the source entries, the target, and the number of comment lines, and the hazard is documented in the design doc and crate docs. Tests for the two behaviours whose absence was noted: `--fix` keeps an allowed entry while removing the others (the one failure mode that destroys user data rather than printing something wrong), and member globs plus `exclude` follow Cargo, which is the reason the tool shells out to `cargo metadata` at all. The design doc no longer describes the anvil wiring as done; it is a follow-up, because anvil installs pinned tools from crates.io and the crate is unreleased. Adds the three artifacts `scripts/add-crate.ps1` would have produced: the crate's `CHANGELOG.md` scaffold, the root README crates entry, and the root CHANGELOG index entry. Only the scaffold is written -- release tooling owns changelog content. 100% line and function coverage; 53 mutants, 50 caught, 3 unviable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The new `write_back` unit tests do real filesystem work in a temp directory, and the anvil miri leg runs lib unit tests under filesystem isolation, so `mkdir` came back unsupported and `anvil-miri` failed. Guards the module with `#[cfg(not(miri))]`, the same way every other filesystem-touching test in this repo is guarded. Coverage is unaffected: the coverage run does not use miri. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses two review comments; both reproduced against the built binary
before changing anything.
**Replacing by rename brought the temp file's identity with it.**
`NamedTempFile` creates its file at mode 0600 (confirmed in
tempfile-3.20.0 `src/file/imp/unix.rs:24`), and a rename carries the
source mode rather than inheriting the target's, so on Unix a 0644
workspace root came back owner-only after every successful --fix -- a
change git does not track. `persist` also replaced a symlinked manifest
with a regular file, where the previous in-place write followed the link.
Now the manifest's permissions are read up front and applied to the
replacement before the rename, and the path is canonicalized first so the
rename lands on the real file. Both choices are stated in the doc comment.
**The carry report could claim moves that never happened.** With a dotted
last survivor (`serde.version = "1"`) the append is skipped -- only a
plain value has a suffix -- but the `Carry` was pushed regardless, so
stderr claimed the comment had been carried onto `serde` while it was
absent from the output. Reproduced exactly as described. `onto` is now
derived from the operation that actually placed the text, so the
unplaceable cases report a drop, and the message no longer asserts a
reason ("every entry was removed") that is false for the dotted case.
**Comments on removed sub-table entries vanished unreported.** For
`[workspace.dependencies.name]` the decor lives on the table, not the
key, so `comments_of` saw an empty prefix and no `Carry` was recorded.
`decor_prefix` now reads both.
Tests: dotted-last-survivor drop, sub-table carry, and the stderr
assertion the trailing-removal path was missing. Adds symlink terms to
`.spelling`.
100% line and function coverage; 54 mutants, 52 caught, 2 unviable.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Both reported cases reproduced against the built binary first. `toml_edit` keeps an entry's leading comments in one of three places: on the key for a plain value, on the table for a `[workspace.dependencies.name]` sub-table, and -- measured, not assumed -- on the *first inner key* for a dotted `name.version = "1"`. The previous push read the table slot but always wrote the key slot, so a sub-table survivor had its own comment rendered twice: `--fix` put a line into the manifest that the user never wrote. A dotted survivor was worse in the other direction -- setting its outer key decor renders nothing, so the carried comment was lost while stderr still claimed it had been carried. `leading_comments` and `prepend_comments` now resolve the same slot, so what is read is what is written. That removes the duplication, and it also lets a dotted survivor carry the text properly rather than dropping it, which is better than the reported failure mode required. The same lookup fixes a third case neither comment covered: a removed *dotted* entry's comment lived on the inner key, so it used to vanish with no `Carry` recorded at all -- the exact contract violation the reporting exists to prevent. It is now carried and reported. Trailing removals are unchanged: carried text has to land after the final survivor and only a plain value has a suffix, so a sub-table survivor there still drops the comments -- reported, not silent. Also corrects the `Carry::onto` doc, which still described `None` as meaning the table was emptied. 100% line and function coverage; 60 mutants, 58 caught, 2 unviable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a new Ox Tools crate, cargo-ensure-no-unused-workspace-deps, implementing a manifest-only check (with optional --fix) that fails when a [workspace.dependencies] entry is inherited by no workspace member, alongside a design doc and comprehensive integration tests.
Changes:
- Introduces the new
cargo-ensure-no-unused-workspace-depscrate (CLI, detection logic, and--fixmanifest rewrite). - Adds a full design document describing the rule, UX, and
--fixcomment-carry semantics. - Adds integration tests that exercise inheritance detection, allow-list behavior, and
--fixrewriting/reporting.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Adds the new crate to the repository’s crate list. |
| CHANGELOG.md | Adds the new crate to the top-level changelog index. |
| Cargo.lock | Adds the new crate package entry to the lockfile. |
| .spelling | Adds “symlink*” words used in the new docs/messages. |
| crates/cargo-ensure-no-unused-workspace-deps/Cargo.toml | New crate manifest and dependencies. |
| crates/cargo-ensure-no-unused-workspace-deps/src/main.rs | Binary entry point wiring into the library run(). |
| crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs | CLI surface, member enumeration via cargo metadata, reporting, and atomic rewrite logic. |
| crates/cargo-ensure-no-unused-workspace-deps/src/detect.rs | Manifest scanning to detect which catalog entries are inherited. |
| crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs | --fix removal + comment carry/drop behavior using toml_edit. |
| crates/cargo-ensure-no-unused-workspace-deps/tests/integration_tests.rs | End-to-end integration tests running the compiled binary against temp workspaces. |
| crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md | Design doc covering motivation, rule, UX, and CI integration plan. |
| crates/cargo-ensure-no-unused-workspace-deps/README.md | Crate README describing purpose, usage, config, and behavior. |
| crates/cargo-ensure-no-unused-workspace-deps/CHANGELOG.md | New crate changelog file scaffold. |
| crates/cargo-ensure-no-unused-workspace-deps/logo.png | New crate logo asset (LFS pointer). |
| crates/cargo-ensure-no-unused-workspace-deps/favicon.ico | New crate favicon asset (LFS pointer). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The round-three rewrite narrowed the trailing-removal note to "sub-table survivor", but the branch still drops for a dotted survivor too: `Item::as_value_mut` returns `None` for a dotted `Item::Table`, which is what `fix_reports_a_drop_when_the_last_survivor_cannot_carry_comments` pins with a trailing `serde.version = "1"`. Restores both forms in the code comment and design §5, and says why the two directions differ: a dotted survivor can be carried *onto* -- the comments go ahead of it, on its first inner key -- and only appending *after* one is impossible. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The exit-code section still said an empty catalog has "nothing to be stale", which stopped being true when the stale allow-list report moved ahead of that early return. `a_stale_allow_entry_is_reported_against_an_empty_catalog` pins the behaviour the sentence contradicted. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs:16
Outcome::carriesincludes both comment blocks that are successfully carried onto a survivor and comment blocks that are ultimately dropped (onto: None). The current field doc says "moved", which is misleading given theDropped ...reporting andCarry::ontosemantics.
This issue also appears in the following locations of the same file:
- line 20
- line 36
/// Comment blocks that moved off a removed entry.
crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs:23
Carryrecords both successful carries and drops (onto: None), so the doc comment "Only recorded when comments actually moved" is incorrect. It’s still true thatfromis never empty, but the condition is that at least one comment line existed on removed entries (carried or dropped).
/// Comments that belonged to removed entries and had to go somewhere else.
///
/// Only recorded when comments actually moved, so `from` is never empty.
///
crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs:36
Carry::linesis also used when comments are dropped (onto: None), so describing them as lines that "moved" is inaccurate.
/// How many comment lines moved.
|
martin-kolinek Would it be worth expanding the scope of this tool to completely replace cargo-udeps? So simultaneously check whether each project has superfluous dependencies, and then whether the workspace has superfluous dependencies? |
|
Martin Taillefer (@geeknoid), I'm not a huge fan of reimplementing existing tools unless we can fix specific issues with those tools - do we have issues with cargo-udeps? I thought we're pretty happy with how it works. |
Sander Saares (sandersaares)
left a comment
Sander Saares (sandersaares)
left a comment
There was a problem hiding this comment.
[Copilot speaking]
Published 29 findings. One finding follows up on an existing discussion thread.
See diagnostics
| Diagnostic | Value |
|---|---|
| Cache | Hit |
| Members are enumerated from `cargo metadata --no-deps`, which yields the workspace | ||
| member set — including a root that is itself a package — after Cargo has applied | ||
| `members`, globs, `exclude`, and nested-workspace rules. Re-deriving that set from | ||
| the manifest would be cheaper but risks disagreeing with Cargo, and every | ||
| disagreement that drops a member is a false positive. `--no-deps` keeps the call | ||
| free of dependency resolution. |
There was a problem hiding this comment.
[Copilot speaking]
Separate behavioral promises from implementation guidance
The new command has one design document but no implementation guide. docs/design/README.md mixes observable guarantees—Cargo-consistent member selection, comment-aware fixing, and guarded replacement—with replaceable mechanisms such as the exact cargo metadata invocation, toml_edit decor slots, and the temporary-file sequence.
Reproducible reasoning: The implementation spans member discovery, manifest classification, representation-sensitive TOML mutation, diagnostics, and write-back. The behavioral design should remain stable when those mechanisms are refactored, while maintainers still need a durable explanation of how the modules and chosen APIs compose. Keeping both kinds of information in one document makes it difficult to tell which statements are contractual promises and which are implementation choices.
Consequence: A behavior-preserving refactor can appear to violate the design, while a maintainer changing discovery, comment handling, or persistence must reconstruct cross-module invariants from source and tests.
Recommended action: Add docs/implementation.md with the module pipeline, Cargo-enumeration boundary, TOML representation strategy, and guarded replacement invariants. Keep user-visible behavior and enduring design tenets in the existing design document, and move dependency-, parser-, control-flow-, and filesystem-specific rationale into the implementation guide or focused source documentation.
References:
- crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md
- crates/cargo-ensure-no-unused-workspace-deps/src/detect.rs
- crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs
- crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs
- crates/cargo-anvil/docs/implementation.md
Impacted locations:
crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md:62-77crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md:149-212crates/cargo-ensure-no-unused-workspace-deps/src/detect.rs:48-155crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs:44-218crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs:157-307
| `--fix` replaces the manifest atomically – a temporary file in the same | ||
| directory, renamed over the original, carrying the permissions of the | ||
| manifest it replaces and following a symlinked manifest to its target – and | ||
| refuses to write at all if the file changed after it was read, so a | ||
| concurrent edit is never clobbered. |
There was a problem hiding this comment.
[Copilot speaking]
State each --fix safeguard in a direct sentence
The --fix paragraph compresses atomic replacement, same-directory staging, permission preservation, symlink-target handling, and concurrent-edit detection into one heavily parenthesized sentence. It also uses the informal verb “clobbered” for overwriting an edit.
Reproducible reasoning: The participial clauses do not consistently name whether the temporary file, the rename, or the overall operation carries permissions and follows a symlink. These are independent safeguards with different mechanisms, so separate subject-and-verb sentences would make the contract easier to read and would remain clearer if one safeguard changes.
Consequence: Readers can misinterpret which operation preserves each file property, and informal wording makes a publication-facing safety statement less precise.
Recommended action: Rewrite the canonical crate-level documentation as short, direct sentences that name the operation responsible for atomic replacement, permission preservation, symlink-target handling, and change detection. Use “overwritten” instead of “clobbered”, then regenerate the README.
References:
- crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs
Impacted locations:
crates/cargo-ensure-no-unused-workspace-deps/README.md:65-69
| cargo install cargo-ensure-no-unused-workspace-deps | ||
| ``` | ||
|
|
||
| ## Example output | ||
|
|
||
| ```text | ||
| Found 2 unused workspace dependencies in Cargo.toml: | ||
|
|
There was a problem hiding this comment.
[Copilot speaking]
Synchronize the documented example output with the emitted diagnostic
The canonical crate-level example and its generated README show the unused-dependency diagnostic beginning with Found, while report_unused unconditionally emits the same line beginning with the ❌ marker. Because both blocks are presented as command output, they should reproduce the observable output.
Reproducible reasoning: The examples depict the failure path for unused workspace dependencies. The implementation of that path is report_unused, whose first eprintln! has a literal ❌ prefix. The remaining list entries and remediation text match the examples, and there is no terminal or configuration branch that removes the prefix, so both documented examples diverge from every failing invocation for the same root cause.
Consequence: A reader who copies either documented block into an expected-output fixture, snapshot, or output-parsing guidance receives text that the command never emits and may incorrectly conclude that a real invocation differs from the documented behavior.
Recommended action: Add the emitted marker to the canonical example in src/lib.rs and regenerate the README, or deliberately remove the marker from report_unused if plain text is the intended output contract.
References:
- crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs:359-369
Impacted locations:
crates/cargo-ensure-no-unused-workspace-deps/README.md:82-89crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs:79-87crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs:81-88crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs:358-369crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs:359-369
| //! Re-run with --fix to remove them. | ||
| //! ``` | ||
|
|
||
| #![cfg_attr(coverage_nightly, feature(coverage_attribute))] |
There was a problem hiding this comment.
[Copilot speaking]
Enable docs.rs feature annotations at the crate root
The package is publishable and configures docs.rs builds, but the library crate root does not enable doc_cfg under the docsrs configuration. The crate has no feature-gated public API today, so the omission does not change the current rendered documentation; it leaves the support absent when such APIs are added.
Reproducible reasoning: #![cfg_attr(docsrs, feature(doc_cfg))] belongs at crate scope and lets docs.rs display the configuration that exposes conditionally compiled items. Adding it while the publication scaffold is created is a small preventive step that keeps future feature additions from also requiring a documentation infrastructure correction.
Consequence: A later feature-gated public item can reach docs.rs without an availability annotation, leaving readers to infer which Cargo feature exposes it until a subsequent release corrects the crate root.
Recommended action: Add #![cfg_attr(docsrs, feature(doc_cfg))] at the library crate root beside the existing crate-level configuration attributes.
References:
- Rust Unstable Book:
doc_cfg - crates/cargo-ensure-no-unused-workspace-deps/Cargo.toml
Impacted locations:
crates/cargo-ensure-no-unused-workspace-deps/Cargo.toml:19-20crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs:4-11crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md:5crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs:90
|
|
||
| </div> | ||
|
|
||
| A cargo sub-command that ensures every `[workspace.dependencies]` entry is |
There was a problem hiding this comment.
[Copilot speaking]
Standardize the Cargo subcommand terminology
The new package describes the same concept as a "cargo subcommand" in package metadata, a "cargo sub-command" in the README introduction, and a "cargo workspace" in the usage section. Elsewhere in the crate, including its CLI type documentation, the established form is "Cargo subcommand". Using one spelling and capitalization avoids presenting the Cargo product name and its subcommand concept as several different terms.
Reproducible reasoning: Cargo is the name of the Rust package manager, so prose uses it as a proper name, while Cargo's own documentation spells "subcommand" as one word. The reviewed files introduce lowercase and hyphenated variants for that same concept: Cargo.toml line 6 uses "cargo subcommand", README line 16 uses "cargo sub-command", and README line 34 uses "cargo workspace". These are not command invocations, where lowercase cargo would be correct; they are prose naming Cargo and its concepts. The variation is therefore terminology drift rather than a meaningful distinction.
Consequence: Package registry metadata and the README present inconsistent names for the tool category, and readers may reasonably wonder whether "sub-command" denotes something different from the standard Cargo "subcommand" term.
Recommended action: Use "Cargo subcommand" consistently in the package description and README prose, and capitalize Cargo in "Cargo workspace". Preserve lowercase cargo only in command examples and executable names.
References:
Impacted locations:
crates/cargo-ensure-no-unused-workspace-deps/Cargo.toml:6crates/cargo-ensure-no-unused-workspace-deps/README.md:16crates/cargo-ensure-no-unused-workspace-deps/README.md:34
| // The trailing branch reports too: this is the path that used to claim a | ||
| // carry whether or not the append had actually happened. |
There was a problem hiding this comment.
[Copilot speaking]
Describe the trailing-carry invariant without implementation history
The comment above the trailing-carry assertion says this path used to claim a carry whether or not the append had actually happened. That records how an earlier implementation behaved rather than explaining the test solely in terms of the behavior the implementation must provide.
Reproducible reasoning: The present-state documentation guidance applies to code comments and says not to describe how the current state differs from a previous state. The assertion already establishes the current requirement: the trailing branch reports the comment carry. Its rationale can likewise be stated in present tense—for example, that reporting a carry is valid only when the comment was appended—without requiring future readers to know or preserve the history of an earlier defect.
Consequence: The test carries historical narrative that becomes stale as the implementation evolves and makes readers reconstruct a superseded failure mode to understand a current invariant.
Recommended action: Replace the historical sentence with a present-state justification such as The trailing branch must report a carry only when the comment was actually appended, or remove the comment if the assertion message is sufficient.
Impacted locations:
crates/cargo-ensure-no-unused-workspace-deps/tests/integration_tests.rs:306-307
| /// Main entry point for the library, called from the binary crate. | ||
| /// | ||
| /// 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 | ||
| /// (rather than calling `std::process::exit`) lets `main` unwind normally so the | ||
| /// process terminates through the standard runtime path -- important under | ||
| /// coverage instrumentation, where an abrupt `process::exit` skips the profile | ||
| /// flush on some platforms (notably Windows). | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns an error if a manifest cannot be read or parsed, if the workspace | ||
| /// members cannot be enumerated, or if a fixed manifest cannot be written back. | ||
| pub fn run() -> Result<ExitCode> { | ||
| let cli = Cli::parse(); |
There was a problem hiding this comment.
[Copilot speaking]
Expose an in-process application API for integration tests
src/lib.rs exposes only run, but that function is the process-facing adapter: it reads the ambient argument vector through Cli::parse(), can let Clap terminate the caller for help, version, or invalid arguments, and reaches the application logic through the private check. The external integration suite consequently calls Command::new(binary()) for every scenario instead of invoking the application behavior through the library target.
Reproducible reasoning: check already shows the useful boundary by accepting an explicit manifest path and flags, but its privacy means only in-crate unit tests can call it; the external tests/ target cannot. Tests in tests/integration_tests.rs must start the compiled command, communicate through process-global arguments and streams, and reconstruct outcomes from exit status and text. Calling the public run is not an alternative for external in-process tests because it accepts no explicit input and Clap's infallible parser may exit the whole test process. A deliberate public surface with explicit inputs and observable outputs would let complex workspace, fix, allow-list, failure, and comment-preservation scenarios exercise the application from outside its implementation without launching a new copy of the command for each case. The nearby cargo-coverage-gate crate demonstrates this split by exposing evaluation through its library while keeping argument parsing and exit behavior in its binary adapter.
Consequence: Complex integration coverage is tied to per-case process startup and global stdout/stderr, making scenarios harder to compose and inspect directly. Parsing-error cases cannot safely call the existing public function inside the test process, while the private application boundary is unavailable to integration tests.
Recommended action: Keep ambient argument parsing, Clap help and usage rendering, and final exit-code conversion in main or another binary-only adapter. Expose a documented library function that accepts an explicit options value for the manifest path and command flags and returns a structured outcome, or accepts caller-provided output sinks where rendered diagnostics are part of the behavior. Use that API from tests/ for complex scenarios, reserving a small set of spawned-process tests for CLI wiring and process exit behavior. Define and document how the structured result distinguishes successful checks, unexpected dependencies after applying the allow-list, fixes, and operational failures, and keep implementation modules crate-private.
References:
- crates/cargo-ensure-no-unused-workspace-deps/Cargo.toml
- crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs
- crates/cargo-ensure-no-unused-workspace-deps/src/main.rs
- crates/cargo-ensure-no-unused-workspace-deps/src/detect.rs
- crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs
- crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md
- Clap
Parserdocumentation - Clap
Error::exitdocumentation - crates/cargo-coverage-gate/src/error.rs
- crates/cargo-coverage-gate/src/bin/cargo-coverage-gate/run.rs
Impacted locations:
Cargo.lock:446-454crates/cargo-ensure-no-unused-workspace-deps/Cargo.toml:22-32crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs:100-158crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs:100-171crates/cargo-ensure-no-unused-workspace-deps/src/detect.rs:8-49crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md:145-147crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs:154-158crates/cargo-ensure-no-unused-workspace-deps/Cargo.toml:22-25crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs:100crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs:158-171crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs:253-303crates/cargo-ensure-no-unused-workspace-deps/src/detect.rs:9-50crates/cargo-ensure-no-unused-workspace-deps/src/detect.rs:84-92crates/cargo-ensure-no-unused-workspace-deps/src/main.rs:26-31crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md:143-147crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs:158-159crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs:146-148crates/cargo-ensure-no-unused-workspace-deps/src/main.rs:21-22crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs:95-427crates/cargo-ensure-no-unused-workspace-deps/src/detect.rs:19-140crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs:12-44
| let declared = workspace | ||
| .get("dependencies") | ||
| .and_then(Item::as_table_like) | ||
| .map(|table| table.iter().map(|(key, _)| key.to_owned()).collect()) | ||
| .unwrap_or_default(); | ||
|
|
||
| let allowed = workspace | ||
| .get("metadata") | ||
| .and_then(Item::as_table_like) | ||
| .and_then(|metadata| metadata.get(METADATA_KEY)) | ||
| .and_then(Item::as_table_like) | ||
| .and_then(|config| config.get("allowed")) | ||
| .and_then(Item::as_array) | ||
| .map(|names| names.iter().filter_map(Value::as_str).map(ToOwned::to_owned).collect()) | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
[Copilot speaking]
Validate the workspace manifest once and carry that state into fixing
detect::catalog projects a parsed DocumentMut into detached catalog data while treating absent and wrong-shaped TOML as the same successful defaults. fix::remove later walks the original document again and uses expect to assert that the dependency table still has the shape classification previously observed.
Reproducible reasoning: A present non-table workspace value follows the NotAWorkspace path, a present non-table workspace.dependencies value becomes an empty catalog, and non-string elements in the allowed array are silently discarded. The empty-catalog branch returns success before Cargo metadata can reject a Cargo-invalid dependency catalog. On the fixing path, the current caller happens to preserve the required invariant because it mutates the same document immediately after classifying a non-empty catalog, but the WorkspaceCatalog value carries no proof tying it to that document and remove can panic if another caller or refactor violates the implicit sequence. Both problems arise because manifest-shape validation is performed as lossy extraction rather than represented as state that subsequent operations consume.
Consequence: Malformed configuration can be reported as a clean or non-workspace result, while a future reordering or reuse of the fixer can turn an ordinary invalid state into a process-terminating panic.
Recommended action: Make classification return a checked result with explicit non-workspace and validated-workspace states, and carry the validated workspace state into mutation. Present values of the wrong type should become contextual errors unless compatibility requires an explicit, documented lenient variant; fix::remove should mutate through the validated state instead of re-discovering the table with expect. The additional state type is justified here because both detection and fixing depend on the same manifest-shape invariant, and it replaces several silent fallbacks plus the unchecked mutation boundary.
References:
- crates/cargo-ensure-no-unused-workspace-deps/src/detect.rs
- crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs
- crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs
- crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md, Detection rule and Allowed entries
Impacted locations:
crates/cargo-ensure-no-unused-workspace-deps/src/detect.rs:60-74crates/cargo-ensure-no-unused-workspace-deps/src/detect.rs:19-76crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs:44-50crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs:172-218
| let mut carried = String::new(); | ||
| let mut sources: Vec<String> = Vec::new(); | ||
|
|
||
| for name in &order { | ||
| if doomed.contains(name.as_str()) { | ||
| let comments = comments_of(&leading_comments(table, name)); | ||
| if !comments.is_empty() { | ||
| sources.push(name.clone()); | ||
| } | ||
| carried.push_str(&comments); | ||
| table.remove(name); | ||
| outcome.removed += 1; | ||
| } else if !carried.is_empty() { | ||
| // `onto` reports where the comments actually landed. Claiming a |
There was a problem hiding this comment.
[Copilot speaking]
Define the --fix text-preservation contract
The pull request calls removal "lossless", and the design and crate documentation say comments attached to a removed dependency are carried or reported as dropped. The fixer defines attachment only as leading decor, however, so a same-line suffix comment is discarded with the removed item without producing a Carry record or diagnostic.
Reproducible reasoning: toml_edit represents formatting and comments in both prefix and suffix decor. leading_comments reads only prefix locations for plain, dotted, and sub-table entries, comments_of therefore sees no same-line suffix, and table.remove(name) discards the value and its suffix. report_carries can report only the leading blocks that entered Outcome::carries. Separately, the documented behavior intentionally moves or drops some detected leading comments, so removing an uninherited entry is neutral to Cargo's dependency graph and lockfile but is not unconditionally lossless for the human-authored manifest text. The broad terms "attached" and "lossless" conceal that distinction.
Consequence: A dependency rationale written on the same line can disappear silently, while reviewers can reasonably read the published rationale as a guarantee that all manifest information is either preserved or visibly reported.
Recommended action: Choose one explicit preservation boundary and align the pull request description, design, crate documentation, implementation, and focused tests with it. If manifest comments are protected, inspect both prefix and suffix comment decor for every removed dependency representation and preserve or report each block. If only leading group-header decor is supported, narrow "attached" and "lossless" to that scope and state dependency-graph and lockfile neutrality separately. This contract decision is independent of where an already-detected leading comment can be placed on a structured survivor.
References:
- Pull request description, Points to review item 1
- crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md,
--fixand carried-comment sections - crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs
- crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs
- toml_edit
Decordocumentation
Impacted locations:
crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs:59-72crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs:126-218crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs:65-72crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md:151-164crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md:187-213crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs:65-71crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs:93-96
| // Formatted eagerly rather than in a `with_context` closure: the closure | ||
| // only runs when the write fails, which no test can force portably. | ||
| let failure = format!("failed to write {}", manifest_path.display()); | ||
| std::fs::write(manifest_path, manifest.to_string()).context(failure)?; |
There was a problem hiding this comment.
[Copilot speaking]
Align the concurrent-edit guarantee with the actual write guard
The README and crate documentation say that --fix refuses every manifest change made after the initial read and therefore never overwrites a concurrent edit. write_back compares the file once, then reads permissions, creates and writes the staged file, applies permissions, and finally persists it over the target.
Reproducible reasoning: The unchanged-input comparison at lines 267–272 detects edits visible at that moment. It is not coupled atomically to the replacement at line 283, so a writer can change the target after the comparison and before persist; the rename then replaces that newer content. Atomic replacement prevents a partially written manifest, but it does not make the preceding comparison a conditional compare-and-replace.
Consequence: An editor or automation update made in the comparison-to-persist window can be lost despite the published guarantee that concurrent edits are never overwritten.
Recommended action: Align the documented contract and implementation in whichever direction the project intends. Either narrow the documentation to the edits the pre-write comparison can observe, or use a platform-appropriate conditional replacement or coordination mechanism that protects the comparison-to-replacement boundary against non-cooperating writers. State atomic replacement and concurrent-edit detection as separate guarantees.
References:
- crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs
- crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md
- crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md,
--fixreplacement discussion - Prior write-safety review thread
Impacted locations:
crates/cargo-ensure-no-unused-workspace-deps/README.md:65-69crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs:59-63crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs:267-283
`cargo-ensure-no-unused-workspace-deps` names one check. The tool is meant to grow into full unused-dependency validation, and "workspace" is the wrong word for something whose expensive half is per-crate. Renaming now is free -- the crate is unreleased, nothing pins it, and no anvil recipe references it -- and stops being free the moment it publishes, so it is better done before this lands than in a follow-up. Mechanical throughout: crate directory, package name, binary, subcommand (`cargo unused-deps`), the clap variant, the doc-attribute asset URLs, the `CARGO_BIN_EXE_` handle in tests, and Cargo.lock. The manifest metadata key follows the crate name to `[workspace.metadata.unused-deps]`, matching how cargo-coverage-gate drops the `cargo-` prefix for its key. The root README and CHANGELOG entries move to the end of their lists, because the new name sorts after cargo-heather rather than beside the other `ensure-no-` gates. Description now says what the crate does rather than what one check does: it finds unused dependencies, starting with uninherited `[workspace.dependencies]` entries. Verified: build, 30 tests, clippy -D warnings, fmt, cargo-sort, license headers, repo-wide spellcheck, README regeneration check, `cargo anvil --dry-run` clean, and `cargo unused-deps` still reports all 70 catalog entries inherited. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
martin-kolinek
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
The tool’s description currently overpromises scope and the allow-list parsing silently ignores invalid config values, which can lead to confusing behavior for users.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 14/15 changed files
- Comments generated: 3
- Review effort level: Lite
| .and_then(Item::as_table_like) | ||
| .and_then(|config| config.get("allowed")) | ||
| .and_then(Item::as_array) | ||
| .map(|names| names.iter().filter_map(Value::as_str).map(ToOwned::to_owned).collect()) | ||
| .unwrap_or_default(); |
| - [`cargo-ensure-no-cyclic-deps`](./crates/cargo_ensure_no_cyclic_deps/README.md) - A cargo subcommand to detect cyclic dependencies in workspace crates | ||
| - [`cargo-ensure-no-default-features`](./crates/cargo-ensure-no-default-features/README.md) - A cargo subcommand that ensures dependencies are declared with default-features = false | ||
| - [`cargo-heather`](./crates/cargo-heather/README.md) - A cargo subcommand to validate license headers in Rust, TOML, PowerShell, Just, and env source files | ||
| - [`cargo-unused-deps`](./crates/cargo-unused-deps/README.md) - A cargo subcommand that finds unused dependencies, starting with uninherited [workspace.dependencies] entries |
|
|
||
| [package] | ||
| name = "cargo-unused-deps" | ||
| description = "A cargo subcommand that finds unused dependencies, starting with uninherited [workspace.dependencies] entries" |
What
Adds
cargo-unused-deps: a cargo subcommand that fails when a[workspace.dependencies]entry is inherited by no workspace member, plus its design doc.docs/design/README.md— the design.src/— detection,--fix, allow-list. 18 integration tests, 100% line and function coverage.Work item: 7790070.
Why
cargo udepsresolves the crate graph and asks which declared dependencies go unused. A[workspace.dependencies]entry that no member inherits never enters that graph, so it is invisible to udeps — and tocargo machete, for the same structural reason.That blind spot accumulated 48 stale entries in this repo before #99 swept them out by hand, with udeps green throughout. Per review feedback on #99, the check belongs in cargo-anvil so every repo using it benefits, rather than living as a one-off script.
The rule
An entry is unused when no workspace member declares it with
workspace = true— acrossdependencies,dev-dependencies,build-dependenciesand their[target.'cfg(…)']forms, in both the inline and dotted spellings.Manifest-only, so the gate has no false positives and needs no compilation, toolchain pin, or network. It slots into the
modifiedtier next toensure-no-cyclic-depsandensure-no-default-features, and composes with udeps without overlap:udepsPrior art evaluated
cargo-sheardoes implement ashear/unused_workspace_dependencydiagnostic, and it does catch the case — verified by injecting an unused entry into this repo's root manifest. But it derives the verdict from static source-usage analysis, so it inherits that analysis's macro-expansion blind spots: it reports 8 entries here (rustdoc-types-v50..v57) that are inherited and genuinely used, viagenerate_version_support!("50", rustdoc_types_v50). It also skips the check entirely for single-member workspaces. Its other diagnostics remain independently interesting — separate decision.cargo-unused-workspace-depson crates.io does exactly this, but is one release from Sept 2025 with no commits since — not something to pin as an anvil dependency.Points to review
--fixis in scope, unlike the siblingensure-no-default-features("the tool reports; the human edits"). Rationale: removing an entry nobody inherits is mechanical and lossless —Cargo.lockis unaffected by construction — unlike deciding which features to keep. The comment-carrying rules are specified normatively, including the empty-table case.cargo metadata --no-depsrather than re-derivingmembers/globs/excludetextually. Costs a subprocess, but any disagreement with Cargo that drops a member is a false positive.[workspace.metadata.…] allowed = […], not a CLI flag, because the generated recipe invokes the tool with a fixed argument list.Not in this PR
The anvil wiring (
versions.justpin,tools.justinstall/validate,checks/recipe,pr-fastgroup,checks.mdcatalog row). Anvil installs pinned tools from crates.io, so the wiring has nothing to pin until the crate's first release; design §7 says so explicitly.🤖 Authored by Clawpilot (an AI agent), not by a human.