From 0cab26f85c4b54e9c2facd327cc0a5bd039a35a5 Mon Sep 17 00:00:00 2001 From: "Martin Kolinek (from Dev Box)" Date: Tue, 25 Aug 2026 18:12:24 +0200 Subject: [PATCH 01/11] docs: design for cargo-ensure-no-unused-workspace-deps `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> --- .../docs/design/README.md | 189 ++++++++++++++++++ 1 file changed, 189 insertions(+) create mode 100644 crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md diff --git a/crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md b/crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md new file mode 100644 index 00000000..5930050c --- /dev/null +++ b/crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md @@ -0,0 +1,189 @@ +# cargo-ensure-no-unused-workspace-deps — Design + +> Status: **Proposed**. +> Crate name: `cargo-ensure-no-unused-workspace-deps`. +> Home: `github.com/microsoft/ox-tools`, published to crates.io. + +## 1. Problem + +A workspace root declares a dependency catalog in `[workspace.dependencies]`, and +members draw from it with `dep = { workspace = true }`. Nothing requires an entry to +be drawn from. An entry that no member inherits stays in the manifest forever: it is +syntactically valid, it never enters the dependency graph, and no build ever fails +because of it. + +Such entries are not inert. They carry version requirements, so they surface in +dependency review, attract Dependabot bumps, and add lines that `cargo sort` and +every manifest diff must carry. They also mislead: a reader takes the catalog as the +list of things this repository depends on. + +The existing unused-dependency gate cannot see them. `cargo udeps` resolves the crate +graph and asks which *declared* dependencies go unreferenced; an entry that no member +declares is absent from that graph entirely. The same blind spot applies to +`cargo machete`. In this repository the gap accumulated 48 stale entries before a +manual sweep removed them — with `udeps` green throughout. + +The missing check is a different question from the one `udeps` answers, and a much +cheaper one: *is this catalog entry inherited by anybody?* That is a fact about the +manifests, decidable by reading them. + +## 2. Goals + +1. **Close the catalog blind spot.** Fail when `[workspace.dependencies]` holds an + entry that no workspace member inherits. +2. **No false positives.** The verdict rests only on manifest text. A gate that + occasionally accuses a load-bearing dependency would be turned off. +3. **Cheap.** No compilation, no toolchain pin, no network. Fast enough for the + text/metadata tier that runs on every pull request. +4. **Cargo-native UX.** Ship as `cargo ensure-no-unused-workspace-deps`, matching its + sibling gates `cargo ensure-no-cyclic-deps` and `cargo ensure-no-default-features`. +5. **Mechanical remediation.** Removing an entry nobody inherits is lossless, so the + tool offers `--fix` rather than leaving a 48-entry sweep to hand editing. + +## 3. Non-goals + +- **Judging whether an inherited dependency is used in code.** That is the + compile-accurate question `udeps` already answers. This tool stops at inheritance. +- **Editing member manifests.** Only the workspace root is ever written. +- **Feature or version opinions.** `ensure-no-default-features` covers the catalog's + feature hygiene; version policy belongs to `cargo-aprz` and `deny`. + +## 4. Detection rule + +An entry `name` in `[workspace.dependencies]` is **unused** when no workspace member +manifest contains a dependency declaration whose *key* is `name` and which sets +`workspace = true`. + +Keying on the declaration name rather than the resolved package is exact, not an +approximation: inheritance is by catalog key. A member writing +`rustdoc-types-v57 = { workspace = true }` can only be served by the catalog key +`rustdoc-types-v57`, whatever `package = "…"` rename the catalog entry carries. + +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. + +Each member manifest is scanned for: + +- `[dependencies]`, `[dev-dependencies]`, `[build-dependencies]`; +- the same three tables under any `[target.'cfg(…)']`; +- both the inline form `dep = { workspace = true }` and the dotted form + `dep.workspace = true`. + +Anything that inherits, anywhere, in any target, marks the entry as used. The scan is +deliberately permissive: it errs toward *used*. + +## 5. User-visible shape + +### Invocation + +```bash +cargo ensure-no-unused-workspace-deps [--manifest-path ] [--fix] +``` + +| Option | Default | Meaning | +|-------------------|--------------|----------------------------------------------------------------------| +| `--manifest-path` | `Cargo.toml` | Workspace root manifest to check, relative to the current directory. | +| `--fix` | *(off)* | Remove the unused entries instead of only reporting them. | + +### Allowed entries + +A deliberate exception is declared in the workspace manifest, not on the command +line, because the generated CI recipe invokes the tool with a fixed argument list: + +```toml +[workspace.metadata.ensure-no-unused-workspace-deps] +allowed = ["kept-on-purpose"] +``` + +An allowed name is neither reported nor removed. An `allowed` entry that matches no +unused entry produces a warning on stderr without changing the exit code — a stale +exception is a maintenance smell, and failing the build for one would punish the act +of fixing the underlying problem. + +### Reporting + +Unused entries are written to stderr, one per line, naming the entry and the +manifest that declares it. Order follows the manifest so the report reads alongside +the file. The success line goes to stdout. + +### Exit codes + +| Code | Meaning | +|------|--------------------------------------------------------------------------------------------------| +| 0 | No unused entries — or, under `--fix`, all unused entries were removed and the manifest written. | +| 1 | Unused entries found without `--fix`, or a manifest could not be read, parsed, or enumerated. | + +A manifest with no `[workspace]` table is an error: it means the wrong file was +pointed at. A `[workspace]` table with no `dependencies` catalog is a clean pass — +there is nothing to be stale. + +The exit code is returned from `run` as an `ExitCode` rather than raised with +`std::process::exit`, so `main` unwinds normally. That matters under coverage +instrumentation, where an abrupt exit can skip the profile flush on some platforms. + +### `--fix` + +The manifest is rewritten with `toml_edit`, so formatting, ordering, and comments on +surviving entries are preserved. Comment handling follows the manifest's own reading +order: + +- Comments attached to a removed entry are carried forward to the next surviving + entry, so a group header such as `# --- external dependencies ---` keeps labeling + the group it introduces. +- When the removed entries are the last in the table, the carried comments are + appended after the final surviving entry's *value*, keeping them at the end of the + table where they were written. Attaching them to that entry's key prefix would + hoist them above it and relabel a surviving dependency. +- When every entry is removed, the carried comments go with them. A header for a + group that no longer exists is not worth preserving. + +Only comment-bearing decor is carried; blank-line padding from a removed entry is +dropped. + +`Cargo.lock` is unaffected by construction: an entry no member inherits never +contributed a node to the dependency graph. The tool never touches the lockfile, and +a lockfile that changes after a fix indicates unrelated drift. + +## 6. Relationship to the other dependency checks + +| Question | Answered by | +|---------------------------------------------------------|------------------------------| +| Is this catalog entry inherited by any member? | this tool | +| Is an inherited dependency actually referenced in code? | `udeps` | +| Is it declared with explicit features? | `ensure-no-default-features` | + +The first two compose without overlap and without gaps: this tool is manifest-only +and cannot be fooled by macro-hidden imports; `udeps` is compile-accurate and cannot +see uninherited entries. + +**`cargo-shear` was evaluated and rejected as the vehicle.** It does implement a +`shear/unused_workspace_dependency` diagnostic, but derives it from static +source-usage analysis, so its verdict inherits that analysis's macro-expansion blind +spots — on this repository it reports eight entries that are inherited and genuinely +used through macro arguments. It also skips the check for single-member workspaces. +Its other diagnostics remain independently interesting; that is a separate decision. +The dormant `cargo-unused-workspace-deps` crate (one release in 2025, no commits +since) was likewise rejected as a pinned dependency. + +## 7. CI integration + +The check joins the `modified` tier and runs in the `pr-fast` group as +`cargo ensure-no-unused-workspace-deps`, alongside `ensure-no-cyclic-deps` and +`ensure-no-default-features`. It is a text/metadata check: one platform is enough, +no toolchain pin is required, and it is wired through cargo-anvil like its siblings — +a pinned version in `versions.just`, install and validate recipes in `tools.just`, +and a check recipe in `checks/`. + +Because the tool reads the workspace root, it runs once from the repository root +rather than per affected package. + +## 8. Out of scope + +- Unused entries in the `[workspace.dependencies]` of a *nested* workspace. Each + workspace root is checked on its own terms by its own invocation. +- `[patch]`, `[replace]`, and `[profile]` tables. +- Any judgment about whether an inherited dependency *should* be inherited. From 51b999d139201ac418c42f81fe48d9a27cedd27f Mon Sep 17 00:00:00 2001 From: "Martin Kolinek (from Dev Box)" Date: Tue, 25 Aug 2026 18:18:26 +0200 Subject: [PATCH 02/11] fix: exclude the design-only crate directory from the workspace `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> --- Cargo.toml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index bc5fab38..2fab9e72 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -4,6 +4,11 @@ [workspace] resolver = "2" members = ["crates/*"] +# `members` globs `crates/*`, so a crate directory holding only a design doc -- +# the design-docs-first workflow lands the design before the code -- would be +# read as a member with no manifest and break every cargo invocation. Excluded +# until the crate itself lands, which removes this entry. +exclude = ["crates/cargo-ensure-no-unused-workspace-deps"] [workspace.package] edition = "2024" From d240f35d42653418f32ed99a3e936f61c4f61dbf Mon Sep 17 00:00:00 2001 From: "Martin Kolinek (from Dev Box)" Date: Tue, 25 Aug 2026 19:19:16 +0200 Subject: [PATCH 03/11] feat: add the cargo-ensure-no-unused-workspace-deps crate skeleton 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> --- Cargo.lock | 4 +++ Cargo.toml | 5 --- .../Cargo.toml | 35 +++++++++++++++++++ .../src/lib.rs | 22 ++++++++++++ 4 files changed, 61 insertions(+), 5 deletions(-) create mode 100644 crates/cargo-ensure-no-unused-workspace-deps/Cargo.toml create mode 100644 crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs diff --git a/Cargo.lock b/Cargo.lock index 2b96212c..206085aa 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -442,6 +442,10 @@ dependencies = [ "toml", ] +[[package]] +name = "cargo-ensure-no-unused-workspace-deps" +version = "0.0.0" + [[package]] name = "cargo-heather" version = "0.3.0" diff --git a/Cargo.toml b/Cargo.toml index 2fab9e72..bc5fab38 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -4,11 +4,6 @@ [workspace] resolver = "2" members = ["crates/*"] -# `members` globs `crates/*`, so a crate directory holding only a design doc -- -# the design-docs-first workflow lands the design before the code -- would be -# read as a member with no manifest and break every cargo invocation. Excluded -# until the crate itself lands, which removes this entry. -exclude = ["crates/cargo-ensure-no-unused-workspace-deps"] [workspace.package] edition = "2024" diff --git a/crates/cargo-ensure-no-unused-workspace-deps/Cargo.toml b/crates/cargo-ensure-no-unused-workspace-deps/Cargo.toml new file mode 100644 index 00000000..d0fcd402 --- /dev/null +++ b/crates/cargo-ensure-no-unused-workspace-deps/Cargo.toml @@ -0,0 +1,35 @@ +# Copyright (c) Microsoft Corporation. +# Licensed under the MIT License. + +[package] +name = "cargo-ensure-no-unused-workspace-deps" +description = "A cargo subcommand that ensures every [workspace.dependencies] entry is inherited by a workspace member" +version = "0.0.0" +keywords = ["oxidizer", "cargo", "subcommand", "dependencies", "ci"] +categories = ["command-line-utilities", "development-tools::cargo-plugins"] + +# Skeleton. The design is under review and the implementation has not +# landed, so there is nothing worth publishing yet. The implementation +# change drops this line and takes the crate to its first release. +publish = false + +authors.workspace = true +edition.workspace = true +homepage.workspace = true +license.workspace = true +rust-version.workspace = true +repository = "https://github.com/microsoft/ox-tools/tree/main/crates/cargo-ensure-no-unused-workspace-deps" + +# Coverage opt-out. The crate carries no executable code yet, so the +# nightly anvil-llvm-cov run attributes no instrumented regions to it and +# cargo-coverage-gate would classify it as NO DATA (exit 2). +# `min-lines-percent = 0.0` is the gate's documented opt-out and resolves +# to Status::Ok even with no attributed data. The implementation change +# removes this opt-out along with the reason for it. +[package.metadata.coverage-gate] +min-lines-percent = 0.0 + +# >>> anvil-managed: anvil-lints +[lints] +workspace = true +# <<< anvil-managed: anvil-lints diff --git a/crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs b/crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs new file mode 100644 index 00000000..6b913958 --- /dev/null +++ b/crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs @@ -0,0 +1,22 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//! A cargo sub-command that ensures every `[workspace.dependencies]` entry is +//! inherited by at least one workspace member. +//! +//! A workspace root declares a dependency catalog that members draw from with +//! `dep = { workspace = true }`. Nothing requires an entry to be drawn from, so +//! an entry nobody inherits stays in the manifest forever: it never enters the +//! dependency graph, and no build fails because of it. It still carries a +//! version requirement, so it keeps attracting dependency-bump traffic and +//! keeps misleading readers about what the workspace depends on. +//! +//! Unused-dependency tools resolve the crate graph and ask which *declared* +//! dependencies go unused, so an entry that no member declares is invisible to +//! them. This crate answers the prior question -- is the entry inherited at +//! all? -- from the manifests alone. +//! +//! # Status +//! +//! Skeleton. The design is under review and the implementation has not landed, +//! so this crate exposes no API and installs no binary yet. From 4d7b09720d9ba86b1d337eab634c96061aa990ad Mon Sep 17 00:00:00 2001 From: "Martin Kolinek (from Dev Box)" Date: Wed, 26 Aug 2026 12:21:25 +0200 Subject: [PATCH 04/11] feat: implement cargo-ensure-no-unused-workspace-deps 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> --- Cargo.lock | 9 +- .../Cargo.toml | 42 +- .../README.md | 85 ++++ .../docs/design/README.md | 38 +- .../favicon.ico | 3 + .../logo.png | 3 + .../src/detect.rs | 147 +++++++ .../src/fix.rs | 83 ++++ .../src/lib.rs | 262 +++++++++++- .../src/main.rs | 32 ++ .../tests/integration_tests.rs | 395 ++++++++++++++++++ 11 files changed, 1064 insertions(+), 35 deletions(-) create mode 100644 crates/cargo-ensure-no-unused-workspace-deps/README.md create mode 100644 crates/cargo-ensure-no-unused-workspace-deps/favicon.ico create mode 100644 crates/cargo-ensure-no-unused-workspace-deps/logo.png create mode 100644 crates/cargo-ensure-no-unused-workspace-deps/src/detect.rs create mode 100644 crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs create mode 100644 crates/cargo-ensure-no-unused-workspace-deps/src/main.rs create mode 100644 crates/cargo-ensure-no-unused-workspace-deps/tests/integration_tests.rs diff --git a/Cargo.lock b/Cargo.lock index 206085aa..2d380371 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -444,7 +444,14 @@ dependencies = [ [[package]] name = "cargo-ensure-no-unused-workspace-deps" -version = "0.0.0" +version = "0.1.0" +dependencies = [ + "anyhow", + "cargo_metadata", + "clap", + "tempfile", + "toml_edit", +] [[package]] name = "cargo-heather" diff --git a/crates/cargo-ensure-no-unused-workspace-deps/Cargo.toml b/crates/cargo-ensure-no-unused-workspace-deps/Cargo.toml index d0fcd402..971635ab 100644 --- a/crates/cargo-ensure-no-unused-workspace-deps/Cargo.toml +++ b/crates/cargo-ensure-no-unused-workspace-deps/Cargo.toml @@ -4,30 +4,38 @@ [package] name = "cargo-ensure-no-unused-workspace-deps" description = "A cargo subcommand that ensures every [workspace.dependencies] entry is inherited by a workspace member" -version = "0.0.0" +version = "0.1.0" +readme = "README.md" keywords = ["oxidizer", "cargo", "subcommand", "dependencies", "ci"] categories = ["command-line-utilities", "development-tools::cargo-plugins"] -# Skeleton. The design is under review and the implementation has not -# landed, so there is nothing worth publishing yet. The implementation -# change drops this line and takes the crate to its first release. -publish = false - -authors.workspace = true edition.workspace = true -homepage.workspace = true -license.workspace = true rust-version.workspace = true +authors.workspace = true +license.workspace = true +homepage.workspace = true repository = "https://github.com/microsoft/ox-tools/tree/main/crates/cargo-ensure-no-unused-workspace-deps" -# Coverage opt-out. The crate carries no executable code yet, so the -# nightly anvil-llvm-cov run attributes no instrumented regions to it and -# cargo-coverage-gate would classify it as NO DATA (exit 2). -# `min-lines-percent = 0.0` is the gate's documented opt-out and resolves -# to Status::Ok even with no attributed data. The implementation change -# removes this opt-out along with the reason for it. -[package.metadata.coverage-gate] -min-lines-percent = 0.0 +[package.metadata.docs.rs] +all-features = true + +[package.metadata.cargo_check_external_types] +# `anyhow::Error` leaks through the `run` entry point that `main` calls. Mirrors the +# allowlist entry in `cargo-ensure-no-default-features`, which has the same shape. +allowed_external_types = ["anyhow::Result"] + +[[bin]] +name = "cargo-ensure-no-unused-workspace-deps" +path = "src/main.rs" + +[dependencies] +anyhow = { workspace = true, features = ["std"] } +cargo_metadata = { workspace = true } +clap = { workspace = true, features = ["std", "derive", "color", "help", "error-context", "usage"] } +toml_edit = { workspace = true } + +[dev-dependencies] +tempfile = { workspace = true } # >>> anvil-managed: anvil-lints [lints] diff --git a/crates/cargo-ensure-no-unused-workspace-deps/README.md b/crates/cargo-ensure-no-unused-workspace-deps/README.md new file mode 100644 index 00000000..3e9924bc --- /dev/null +++ b/crates/cargo-ensure-no-unused-workspace-deps/README.md @@ -0,0 +1,85 @@ +
+ Cargo-Ensure-No-Unused-Workspace-Deps Logo + +# Cargo-Ensure-No-Unused-Workspace-Deps + +[![crates.io](https://img.shields.io/crates/v/cargo-ensure-no-unused-workspace-deps.svg)](https://crates.io/crates/cargo-ensure-no-unused-workspace-deps) +[![docs.rs](https://docs.rs/cargo-ensure-no-unused-workspace-deps/badge.svg)](https://docs.rs/cargo-ensure-no-unused-workspace-deps) +[![MSRV](https://img.shields.io/crates/msrv/cargo-ensure-no-unused-workspace-deps)](https://crates.io/crates/cargo-ensure-no-unused-workspace-deps) +[![CI](https://github.com/microsoft/ox-tools/actions/workflows/main.yml/badge.svg?event=push)](https://github.com/microsoft/ox-tools/actions/workflows/main.yml) +[![Coverage](https://codecov.io/gh/microsoft/ox-tools/graph/badge.svg?token=FCUG0EL5TI)](https://codecov.io/gh/microsoft/ox-tools) +[![License](https://img.shields.io/badge/license-MIT-blue.svg)](../../LICENSE) +This crate was developed as part of the Oxidizer project + +
+ +A cargo sub-command that ensures every `[workspace.dependencies]` entry is +inherited by at least one workspace member. + +A workspace root declares a dependency catalog that members draw from with +`dep = { workspace = true }`. Nothing requires an entry to be drawn from, so +an entry nobody inherits stays in the manifest forever: it never enters the +dependency graph, and no build fails because of it. It still carries a +version requirement, so it keeps attracting dependency-bump traffic and keeps +misleading readers about what the workspace depends on. + +Unused-dependency tools resolve the crate graph and ask which *declared* +dependencies go unused, so an entry that no member declares is invisible to +them. This tool answers the prior question – is the entry inherited at all? +– from the manifests alone, which makes it free of false positives and cheap +enough to run on every pull request. + +## Usage + +Run in a cargo workspace: + +```bash +cargo ensure-no-unused-workspace-deps +``` + +Remove what it finds: + +```bash +cargo ensure-no-unused-workspace-deps --fix +``` + +`--manifest-path` points at an explicit workspace root, defaulting to the +`Cargo.toml` in the current directory. A manifest with no `[workspace]` table +declares no catalog and passes with a note; `--require-workspace` turns that +into an error for callers that know they are pointing at a workspace root. + +## Configuration + +An entry kept on purpose is exempted in the workspace manifest: + +```toml +[workspace.metadata.ensure-no-unused-workspace-deps] +allowed = ["kept-on-purpose"] +``` + +An `allowed` name that suppresses nothing is reported as stale, on stderr, +without failing the run. + +## Installation + +```bash +cargo install cargo-ensure-no-unused-workspace-deps +``` + +## Example output + +```text +Found 2 unused workspace dependencies in Cargo.toml: + + - once_cell + - smallvec + +Re-run with --fix to remove them. +``` + + +
+ +This crate was developed as part of The Oxidizer Project. Browse this crate's source code. + + diff --git a/crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md b/crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md index 5930050c..41d96376 100644 --- a/crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md +++ b/crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md @@ -1,6 +1,6 @@ # cargo-ensure-no-unused-workspace-deps — Design -> Status: **Proposed**. +> Status: **Adopted**. > Crate name: `cargo-ensure-no-unused-workspace-deps`. > Home: `github.com/microsoft/ox-tools`, published to crates.io. @@ -81,13 +81,30 @@ deliberately permissive: it errs toward *used*. ### Invocation ```bash -cargo ensure-no-unused-workspace-deps [--manifest-path ] [--fix] +cargo ensure-no-unused-workspace-deps [--manifest-path ] [--fix] [--require-workspace] ``` -| Option | Default | Meaning | -|-------------------|--------------|----------------------------------------------------------------------| -| `--manifest-path` | `Cargo.toml` | Workspace root manifest to check, relative to the current directory. | -| `--fix` | *(off)* | Remove the unused entries instead of only reporting them. | +| Option | Default | Meaning | +|-----------------------|--------------|----------------------------------------------------------------------| +| `--manifest-path` | `Cargo.toml` | Workspace root manifest to check, relative to the current directory. | +| `--fix` | *(off)* | Remove the unused entries instead of only reporting them. | +| `--require-workspace` | *(off)* | Treat a manifest with no `[workspace]` table as an error. | + +### Manifests that are not workspace roots + +A manifest with no `[workspace]` table has no catalog, so there is nothing this +check can be wrong about. It reports that on stderr and succeeds. + +That default exists because the check is invoked from cargo-anvil, which manages +single-crate repositories as well as workspaces. A generated recipe runs the same +command everywhere, so a hard error here would make the check unusable in exactly +the repositories that never had the problem, and each of them would need a local +opt-out. Succeeding is also the honest answer: the property "no catalog entry goes +uninherited" holds vacuously. + +`--require-workspace` restores the strict reading for callers that know they are +pointing at a workspace root and want a misdirected `--manifest-path` to fail rather +than pass quietly. ### Allowed entries @@ -117,9 +134,9 @@ the file. The success line goes to stdout. | 0 | No unused entries — or, under `--fix`, all unused entries were removed and the manifest written. | | 1 | Unused entries found without `--fix`, or a manifest could not be read, parsed, or enumerated. | -A manifest with no `[workspace]` table is an error: it means the wrong file was -pointed at. A `[workspace]` table with no `dependencies` catalog is a clean pass — -there is nothing to be stale. +A `[workspace]` table with no `dependencies` catalog is a clean pass — there is +nothing to be stale. A manifest with no `[workspace]` table at all is a pass with a +note on stderr, or an error under `--require-workspace`. The exit code is returned from `run` as an `ExitCode` rather than raised with `std::process::exit`, so `main` unwinds normally. That matters under coverage @@ -179,7 +196,8 @@ a pinned version in `versions.just`, install and validate recipes in `tools.just and a check recipe in `checks/`. Because the tool reads the workspace root, it runs once from the repository root -rather than per affected package. +rather than per affected package. Single-crate repositories run the same command and +pass without configuration. ## 8. Out of scope diff --git a/crates/cargo-ensure-no-unused-workspace-deps/favicon.ico b/crates/cargo-ensure-no-unused-workspace-deps/favicon.ico new file mode 100644 index 00000000..18acb282 --- /dev/null +++ b/crates/cargo-ensure-no-unused-workspace-deps/favicon.ico @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:82caca17fd4d08a23a9d76f5c895029ff6bb0d13b0e2693ef229a4ea691ffccd +size 46496 diff --git a/crates/cargo-ensure-no-unused-workspace-deps/logo.png b/crates/cargo-ensure-no-unused-workspace-deps/logo.png new file mode 100644 index 00000000..20aaf26c --- /dev/null +++ b/crates/cargo-ensure-no-unused-workspace-deps/logo.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:fc17b32e4b6b8c4f0c1d443b680a25b68e0cd8913954dfd047a7659a6bd0baff +size 131471 diff --git a/crates/cargo-ensure-no-unused-workspace-deps/src/detect.rs b/crates/cargo-ensure-no-unused-workspace-deps/src/detect.rs new file mode 100644 index 00000000..08fc4d8a --- /dev/null +++ b/crates/cargo-ensure-no-unused-workspace-deps/src/detect.rs @@ -0,0 +1,147 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//! Detection: which `[workspace.dependencies]` entries no member inherits. + +use std::collections::BTreeSet; +use std::path::{Path, PathBuf}; + +use anyhow::{Context, Result}; +use toml_edit::{DocumentMut, Item, TableLike, Value}; + +/// Dependency tables a member manifest can inherit workspace dependencies from. +const DEP_TABLES: [&str; 3] = ["dependencies", "dev-dependencies", "build-dependencies"]; + +/// Key under `[workspace.metadata]` holding this tool's configuration. +const METADATA_KEY: &str = "ensure-no-unused-workspace-deps"; + +/// What a manifest turned out to be. +pub enum Catalog { + /// The manifest has no `[workspace]` table, so it declares no catalog. + NotAWorkspace, + + /// The manifest is a workspace root. The catalog may still be empty. + Workspace(WorkspaceCatalog), +} + +/// The `[workspace.dependencies]` catalog of a workspace root, with the +/// allow-list that accompanies it. +pub struct WorkspaceCatalog { + /// Catalog entry names, in the order the manifest declares them. + pub declared: Vec, + + /// Names configured as deliberate exceptions. + pub allowed: BTreeSet, +} + +/// Read and parse a manifest. +pub fn read_manifest(path: &Path) -> Result { + let text = std::fs::read_to_string(path).with_context(|| format!("failed to read {}", path.display()))?; + text.parse::() + .with_context(|| format!("failed to parse {}", path.display())) +} + +/// Classify a parsed manifest and, when it is a workspace root, collect its +/// catalog and allow-list. +pub fn catalog(manifest: &DocumentMut) -> Catalog { + let Some(workspace) = manifest.get("workspace").and_then(Item::as_table_like) else { + return Catalog::NotAWorkspace; + }; + + 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(); + + Catalog::Workspace(WorkspaceCatalog { declared, allowed }) +} + +/// Collect the catalog keys that member manifests inherit. +/// +/// `members` are manifest paths as reported by `cargo metadata`; a manifest that +/// cannot be read or parsed fails the run rather than being silently treated as +/// inheriting nothing, which would turn a read error into false accusations. +pub fn inherited(members: &[PathBuf]) -> Result> { + let mut used = BTreeSet::new(); + + for member in members { + let doc = read_manifest(member)?; + collect_inherited(&doc, &mut used); + } + + Ok(used) +} + +/// Record every catalog key a single manifest inherits. +fn collect_inherited(doc: &DocumentMut, used: &mut BTreeSet) { + for name in DEP_TABLES { + if let Some(table) = doc.get(name).and_then(Item::as_table_like) { + collect_from_dep_table(table, used); + } + } + + // `[target.'cfg(...)'.dependencies]` and its dev/build siblings. + let Some(targets) = doc.get("target").and_then(Item::as_table_like) else { + return; + }; + + for target in targets.iter().filter_map(|(_, target)| target.as_table_like()) { + for name in DEP_TABLES { + if let Some(table) = target.get(name).and_then(Item::as_table_like) { + collect_from_dep_table(table, used); + } + } + } +} + +/// Record the inheriting declarations of one dependency table. +fn collect_from_dep_table(table: &dyn TableLike, used: &mut BTreeSet) { + for (name, spec) in table.iter() { + if inherits_from_workspace(spec) { + used.insert(name.to_owned()); + } + } +} + +/// True for `dep = { workspace = true, .. }` and the dotted `dep.workspace = true` +/// form. Both are table-like to `toml_edit`, so one lookup covers each. +fn inherits_from_workspace(spec: &Item) -> bool { + spec.as_table_like() + .and_then(|table| table.get("workspace")) + .and_then(Item::as_value) + .and_then(Value::as_bool) + .unwrap_or(false) +} + +/// Split the catalog into the entries nobody inherits and the allow-list entries +/// that suppressed nothing. +/// +/// Declaration order is preserved so the report reads alongside the manifest. +pub fn partition(catalog: &WorkspaceCatalog, used: &BTreeSet) -> (Vec, Vec) { + let uninherited: Vec = catalog + .declared + .iter() + .filter(|name| !used.contains(name.as_str())) + .cloned() + .collect(); + + let unused = uninherited + .iter() + .filter(|name| !catalog.allowed.contains(name.as_str())) + .cloned() + .collect(); + let stale = catalog.allowed.iter().filter(|name| !uninherited.contains(name)).cloned().collect(); + + (unused, stale) +} diff --git a/crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs b/crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs new file mode 100644 index 00000000..f639ff7a --- /dev/null +++ b/crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs @@ -0,0 +1,83 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//! Removal of unused `[workspace.dependencies]` entries, preserving the +//! formatting and comments of everything that survives. + +use std::collections::BTreeSet; + +use toml_edit::{DocumentMut, Item}; + +/// Remove `names` from the catalog and return how many entries went away. +/// +/// Comments attached to a removed entry are carried forward to the next +/// surviving entry, so a group header keeps labeling the group it introduces. +pub fn remove(manifest: &mut DocumentMut, names: &[String]) -> usize { + let table = manifest + .get_mut("workspace") + .and_then(Item::as_table_like_mut) + .and_then(|workspace| workspace.get_mut("dependencies")) + .and_then(Item::as_table_like_mut) + .expect("callers only fix a catalog they already read entries from, so the table is present"); + + let order: Vec = table.iter().map(|(key, _)| key.to_owned()).collect(); + let doomed: BTreeSet<&str> = names.iter().map(String::as_str).collect(); + + let mut removed = 0; + let mut carried = String::new(); + + for name in &order { + let prefix = table + .key(name) + .and_then(|key| key.leaf_decor().prefix()) + .and_then(toml_edit::RawString::as_str) + .unwrap_or_default() + .to_owned(); + + if doomed.contains(name.as_str()) { + carried.push_str(&comments_of(&prefix)); + table.remove(name); + removed += 1; + } else if !carried.is_empty() { + if let Some(mut key) = table.key_mut(name) { + key.leaf_decor_mut().set_prefix(format!("{carried}{prefix}")); + } + carried.clear(); + } + } + + if !carried.is_empty() { + // The removed entries were the last in the table, so there is no + // following key to carry the comments to. Append them after the final + // surviving entry's *value* instead: attaching them to that entry's key + // prefix would hoist them above it and relabel a surviving dependency. + // + // When every entry was removed there is no surviving entry at all, and + // the comments go with the group they introduced. + if let Some(last) = table.iter().last().map(|(key, _)| key.to_owned()) + && let Some(value) = table.get_mut(&last).and_then(Item::as_value_mut) + { + let suffix = value + .decor() + .suffix() + .and_then(toml_edit::RawString::as_str) + .unwrap_or_default() + .to_owned(); + value.decor_mut().set_suffix(format!("{suffix}{carried}")); + } + } + + removed +} + +/// The comment-bearing part of a removed entry's decor. +/// +/// Blank-line padding is dropped: only comments such as a `# --- group ---` +/// header are worth carrying to another entry. +fn comments_of(prefix: &str) -> String { + if prefix.lines().any(|line| line.trim_start().starts_with('#')) { + prefix.to_owned() + } else { + String::new() + } +} diff --git a/crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs b/crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs index 6b913958..26312988 100644 --- a/crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs +++ b/crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs @@ -3,20 +3,268 @@ //! A cargo sub-command that ensures every `[workspace.dependencies]` entry is //! inherited by at least one workspace member. +#![doc( + html_logo_url = "https://media.githubusercontent.com/media/microsoft/ox-tools/refs/heads/main/crates/cargo-ensure-no-unused-workspace-deps/logo.png" +)] +#![doc( + html_favicon_url = "https://media.githubusercontent.com/media/microsoft/ox-tools/refs/heads/main/crates/cargo-ensure-no-unused-workspace-deps/favicon.ico" +)] //! //! A workspace root declares a dependency catalog that members draw from with //! `dep = { workspace = true }`. Nothing requires an entry to be drawn from, so //! an entry nobody inherits stays in the manifest forever: it never enters the //! dependency graph, and no build fails because of it. It still carries a -//! version requirement, so it keeps attracting dependency-bump traffic and -//! keeps misleading readers about what the workspace depends on. +//! version requirement, so it keeps attracting dependency-bump traffic and keeps +//! misleading readers about what the workspace depends on. //! //! Unused-dependency tools resolve the crate graph and ask which *declared* //! dependencies go unused, so an entry that no member declares is invisible to -//! them. This crate answers the prior question -- is the entry inherited at -//! all? -- from the manifests alone. +//! them. This tool answers the prior question -- is the entry inherited at all? +//! -- from the manifests alone, which makes it free of false positives and cheap +//! enough to run on every pull request. //! -//! # Status +//! # Usage //! -//! Skeleton. The design is under review and the implementation has not landed, -//! so this crate exposes no API and installs no binary yet. +//! Run in a cargo workspace: +//! +//! ```bash +//! cargo ensure-no-unused-workspace-deps +//! ``` +//! +//! Remove what it finds: +//! +//! ```bash +//! cargo ensure-no-unused-workspace-deps --fix +//! ``` +//! +//! `--manifest-path` points at an explicit workspace root, defaulting to the +//! `Cargo.toml` in the current directory. A manifest with no `[workspace]` table +//! declares no catalog and passes with a note; `--require-workspace` turns that +//! into an error for callers that know they are pointing at a workspace root. +//! +//! # Configuration +//! +//! An entry kept on purpose is exempted in the workspace manifest: +//! +//! ```toml +//! [workspace.metadata.ensure-no-unused-workspace-deps] +//! allowed = ["kept-on-purpose"] +//! ``` +//! +//! An `allowed` name that suppresses nothing is reported as stale, on stderr, +//! without failing the run. +//! +//! # Installation +//! +//! ```bash +//! cargo install cargo-ensure-no-unused-workspace-deps +//! ``` +//! +//! # Example output +//! +//! ```text +//! Found 2 unused workspace dependencies in Cargo.toml: +//! +//! - once_cell +//! - smallvec +//! +//! Re-run with --fix to remove them. +//! ``` + +#![cfg_attr(coverage_nightly, feature(coverage_attribute))] + +mod detect; +mod fix; + +use std::collections::BTreeSet; +use std::path::{Path, PathBuf}; +use std::process::ExitCode; + +use anyhow::{Context, Result}; +use cargo_metadata::MetadataCommand; +use clap::builder::Styles; +use clap::builder::styling::{AnsiColor, Effects}; +use clap::{Parser, Subcommand}; + +use crate::detect::{Catalog, WorkspaceCatalog}; + +const CLAP_STYLES: Styles = Styles::styled() + .header(AnsiColor::Green.on_default().effects(Effects::BOLD)) + .usage(AnsiColor::Green.on_default().effects(Effects::BOLD)) + .literal(AnsiColor::Cyan.on_default().effects(Effects::BOLD)) + .placeholder(AnsiColor::Cyan.on_default()); + +/// Cargo subcommand to ensure every workspace dependency is inherited. +#[derive(Parser, Debug)] +#[command(bin_name = "cargo", version, about, author)] +#[command(styles = CLAP_STYLES)] +struct Cli { + #[command(subcommand)] + command: Commands, +} + +#[derive(Subcommand, Debug)] +enum Commands { + /// Ensure every `[workspace.dependencies]` entry is inherited by a member + #[command(version, display_name = "cargo-ensure-no-unused-workspace-deps")] + EnsureNoUnusedWorkspaceDeps { + /// Path to the workspace root Cargo.toml + #[arg(long, default_value = "Cargo.toml", value_name = "PATH")] + manifest_path: PathBuf, + + /// Remove the unused entries instead of only reporting them + #[arg(long)] + fix: bool, + + /// Treat a manifest with no [workspace] table as an error + #[arg(long)] + require_workspace: bool, + }, +} + +/// 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 { + let cli = Cli::parse(); + let Commands::EnsureNoUnusedWorkspaceDeps { + manifest_path, + fix, + require_workspace, + } = cli.command; + + check(&manifest_path, fix, require_workspace) +} + +/// The check itself, split from [`run`] so tests can drive it without a process +/// boundary or a parsed command line. +fn check(manifest_path: &Path, fix: bool, require_workspace: bool) -> Result { + let mut manifest = detect::read_manifest(manifest_path)?; + + let catalog = match detect::catalog(&manifest) { + Catalog::Workspace(catalog) => catalog, + Catalog::NotAWorkspace => { + if require_workspace { + eprintln!("❌ {} has no [workspace] table.", manifest_path.display()); + return Ok(ExitCode::FAILURE); + } + + eprintln!( + "ℹ️ {} has no [workspace] table; there is no dependency catalog to check.", + manifest_path.display() + ); + return Ok(ExitCode::SUCCESS); + } + }; + + if catalog.declared.is_empty() { + println!("✅ {} declares no workspace dependencies.", manifest_path.display()); + return Ok(ExitCode::SUCCESS); + } + + let members = members_of(manifest_path)?; + let used = detect::inherited(&members)?; + let (unused, stale) = detect::partition(&catalog, &used); + + report_stale(&stale); + + if unused.is_empty() { + report_clean(manifest_path, &catalog, &used, members.len()); + return Ok(ExitCode::SUCCESS); + } + + if !fix { + report_unused(manifest_path, &unused); + return Ok(ExitCode::FAILURE); + } + + let removed = fix::remove(&mut manifest, &unused); + // 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)?; + + println!( + "🧹 Removed {removed} unused workspace {} from {}.", + entries(removed), + manifest_path.display() + ); + + Ok(ExitCode::SUCCESS) +} + +/// Manifest paths of every workspace member, as Cargo resolves them. +/// +/// Deferring to `cargo metadata` rather than re-deriving `members`, its globs +/// and `exclude` keeps this in step with Cargo itself; a member missed here +/// would look like an entry nobody inherits. +fn members_of(manifest_path: &Path) -> Result> { + let metadata = MetadataCommand::new() + .manifest_path(manifest_path) + .no_deps() + .exec() + .with_context(|| format!("failed to enumerate the workspace members of {}", manifest_path.display()))?; + + Ok(metadata + .workspace_packages() + .into_iter() + .map(|package| package.manifest_path.clone().into_std_path_buf()) + .collect()) +} + +/// Report allow-list entries that suppressed nothing. +fn report_stale(stale: &[String]) { + for name in stale { + eprintln!("⚠️ '{name}' is allowed but is inherited or not declared; the allow-list entry can be removed."); + } +} + +/// Report a catalog in which every entry is inherited or allowed. +fn report_clean(manifest_path: &Path, catalog: &WorkspaceCatalog, used: &BTreeSet, members: usize) { + let declared = catalog.declared.len(); + let inherited = catalog.declared.iter().filter(|name| used.contains(name.as_str())).count(); + + if declared == inherited { + println!( + "✅ All {declared} workspace {} in {} are inherited by one of {members} members.", + entries(declared), + manifest_path.display() + ); + } else { + println!( + "✅ All {declared} workspace {} in {} are inherited by one of {members} members or explicitly allowed.", + entries(declared), + manifest_path.display() + ); + } +} + +/// Report the entries no member inherits. +fn report_unused(manifest_path: &Path, unused: &[String]) { + eprintln!( + "❌ Found {} unused workspace {} in {}:\n", + unused.len(), + entries(unused.len()), + manifest_path.display() + ); + for name in unused { + eprintln!(" - {name}"); + } + eprintln!("\nRe-run with --fix to remove them."); +} + +/// Pluralize `dependency` for `count`. +fn entries(count: usize) -> &'static str { + if count == 1 { "dependency" } else { "dependencies" } +} diff --git a/crates/cargo-ensure-no-unused-workspace-deps/src/main.rs b/crates/cargo-ensure-no-unused-workspace-deps/src/main.rs new file mode 100644 index 00000000..d886cfc4 --- /dev/null +++ b/crates/cargo-ensure-no-unused-workspace-deps/src/main.rs @@ -0,0 +1,32 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//! A cargo sub-command that ensures every `[workspace.dependencies]` entry is +//! inherited by at least one workspace member. +//! +//! # Usage +//! +//! After installation, run in any cargo workspace: +//! +//! ```bash +//! cargo ensure-no-unused-workspace-deps +//! ``` +//! +//! Or point at an explicit workspace root: +//! +//! ```bash +//! cargo ensure-no-unused-workspace-deps --manifest-path path/to/Cargo.toml +//! ``` +//! +//! The tool exits with code 0 when every catalog entry is inherited by a member, +//! and code 1 otherwise. `--fix` removes the entries that are not. + +use std::process::ExitCode; + +use anyhow::Result; + +fn main() -> Result { + // TODO: This could be a main.rs only crate, but CI complains when processing bin-only crates: + // https://github.com/rust-lang/cargo/issues/15231. + cargo_ensure_no_unused_workspace_deps::run() +} diff --git a/crates/cargo-ensure-no-unused-workspace-deps/tests/integration_tests.rs b/crates/cargo-ensure-no-unused-workspace-deps/tests/integration_tests.rs new file mode 100644 index 00000000..b0c12b58 --- /dev/null +++ b/crates/cargo-ensure-no-unused-workspace-deps/tests/integration_tests.rs @@ -0,0 +1,395 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//! Integration tests for cargo-ensure-no-unused-workspace-deps. +//! +//! Each test builds a throwaway workspace on disk and runs the compiled binary +//! against it, so the behaviour under test is the one users get, including the +//! `cargo metadata` call that enumerates members. + +// Miri cannot run these tests because they spawn subprocesses and use temp directories. +#![cfg(not(miri))] +#![allow( + clippy::expect_used, + clippy::unwrap_used, + reason = "panic-on-failure idioms are appropriate in tests" +)] + +use std::fs; +use std::path::{Path, PathBuf}; +use std::process::{Command, Output}; + +use tempfile::TempDir; + +/// Path to the binary under test. +fn binary() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_cargo-ensure-no-unused-workspace-deps")) +} + +/// Write a workspace root manifest plus one member per entry in `members`. +/// +/// Each member is `(name, manifest body)`; the body is appended to a generated +/// `[package]` section so tests only spell out the dependency tables they care +/// about. +fn workspace(root: &str, members: &[(&str, &str)]) -> TempDir { + let dir = TempDir::new().expect("failed to create temp dir"); + fs::write(dir.path().join("Cargo.toml"), root).expect("failed to write workspace manifest"); + + for (name, body) in members { + let member = dir.path().join(name); + fs::create_dir_all(member.join("src")).expect("failed to create member dir"); + fs::write(member.join("src").join("lib.rs"), "").expect("failed to write member source"); + + let manifest = format!("[package]\nname = \"{name}\"\nversion = \"0.1.0\"\nedition = \"2021\"\n\n{body}"); + fs::write(member.join("Cargo.toml"), manifest).expect("failed to write member manifest"); + } + + dir +} + +/// Run the tool against `manifest_path` with `args`. +fn run(manifest_path: &Path, args: &[&str]) -> Output { + Command::new(binary()) + .arg("ensure-no-unused-workspace-deps") + .arg("--manifest-path") + .arg(manifest_path) + .args(args) + // Keep the run hermetic: `--no-deps` never resolves, so nothing should + // reach the network, and this makes a regression there fail loudly. + .env("CARGO_NET_OFFLINE", "true") + .output() + .expect("failed to execute the binary") +} + +/// `(success, stdout, stderr)` of a run. +fn outcome(output: &Output) -> (bool, String, String) { + ( + output.status.success(), + String::from_utf8_lossy(&output.stdout).into_owned(), + String::from_utf8_lossy(&output.stderr).into_owned(), + ) +} + +#[test] +fn passes_when_every_entry_is_inherited() { + let dir = workspace( + "[workspace]\nmembers = [\"member\"]\n\n[workspace.dependencies]\nserde = \"1\"\n", + &[("member", "[dependencies]\nserde = { workspace = true }\n")], + ); + let manifest = dir.path().join("Cargo.toml"); + + let (success, stdout, _) = outcome(&run(&manifest, &[])); + + assert!(success, "an inherited catalog should pass"); + assert!(stdout.contains("All 1 workspace dependency"), "unexpected stdout: {stdout}"); +} + +#[test] +fn fails_and_lists_entries_nobody_inherits() { + let dir = workspace( + "[workspace]\nmembers = [\"member\"]\n\n[workspace.dependencies]\nserde = \"1\"\nonce_cell = \"1\"\nsmallvec = \"1\"\n", + &[("member", "[dependencies]\nserde = { workspace = true }\n")], + ); + let manifest = dir.path().join("Cargo.toml"); + + let (success, _, stderr) = outcome(&run(&manifest, &[])); + + assert!(!success, "uninherited entries should fail the run"); + assert!( + stderr.contains("Found 2 unused workspace dependencies"), + "unexpected stderr: {stderr}" + ); + assert!(stderr.contains("- once_cell"), "unexpected stderr: {stderr}"); + assert!(stderr.contains("- smallvec"), "unexpected stderr: {stderr}"); + assert!(!stderr.contains("- serde"), "the inherited entry should not be reported: {stderr}"); +} + +#[test] +fn counts_a_single_entry_in_the_singular() { + let dir = workspace( + "[workspace]\nmembers = [\"member\"]\n\n[workspace.dependencies]\nonce_cell = \"1\"\n", + &[("member", "")], + ); + let manifest = dir.path().join("Cargo.toml"); + + let (success, _, stderr) = outcome(&run(&manifest, &[])); + + assert!(!success); + assert!( + stderr.contains("Found 1 unused workspace dependency in"), + "unexpected stderr: {stderr}" + ); +} + +#[test] +fn recognizes_dev_build_and_target_tables() { + let root = + "[workspace]\nmembers = [\"member\"]\n\n[workspace.dependencies]\ntempfile = \"3\"\ncc = \"1\"\nlibc = \"0.2\"\nwinapi = \"0.3\"\n"; + let body = concat!( + "[dev-dependencies]\ntempfile = { workspace = true }\n\n", + "[build-dependencies]\ncc = { workspace = true }\n\n", + "[target.'cfg(unix)'.dependencies]\nlibc = { workspace = true }\n\n", + "[target.'cfg(windows)'.dev-dependencies]\nwinapi = { workspace = true }\n", + ); + let dir = workspace(root, &[("member", body)]); + let manifest = dir.path().join("Cargo.toml"); + + let (success, stdout, stderr) = outcome(&run(&manifest, &[])); + + assert!(success, "every table form should count as inheritance: {stderr}"); + assert!(stdout.contains("All 4 workspace dependencies"), "unexpected stdout: {stdout}"); +} + +#[test] +fn recognizes_the_dotted_inheritance_form() { + let dir = workspace( + "[workspace]\nmembers = [\"member\"]\n\n[workspace.dependencies]\nserde = \"1\"\n", + &[("member", "[dependencies]\nserde.workspace = true\n")], + ); + let manifest = dir.path().join("Cargo.toml"); + + let (success, _, stderr) = outcome(&run(&manifest, &[])); + + assert!(success, "the dotted form is inheritance too: {stderr}"); +} + +#[test] +fn a_declaration_that_does_not_inherit_does_not_count() { + // The member declares `serde` itself rather than drawing it from the + // catalog, so the catalog entry is still inherited by nobody. + let dir = workspace( + "[workspace]\nmembers = [\"member\"]\n\n[workspace.dependencies]\nserde = \"1\"\n", + &[("member", "[dependencies]\nserde = { version = \"1\" }\n")], + ); + let manifest = dir.path().join("Cargo.toml"); + + let (success, _, stderr) = outcome(&run(&manifest, &[])); + + assert!(!success, "a self-declared dependency does not inherit"); + assert!(stderr.contains("- serde"), "unexpected stderr: {stderr}"); +} + +#[test] +fn counts_the_root_package_as_a_member() { + let dir = TempDir::new().expect("failed to create temp dir"); + fs::create_dir_all(dir.path().join("src")).expect("failed to create src"); + fs::write(dir.path().join("src").join("lib.rs"), "").expect("failed to write source"); + fs::write( + dir.path().join("Cargo.toml"), + concat!( + "[workspace]\n\n", + "[workspace.dependencies]\nserde = \"1\"\n\n", + "[package]\nname = \"root\"\nversion = \"0.1.0\"\nedition = \"2021\"\n\n", + "[dependencies]\nserde = { workspace = true }\n", + ), + ) + .expect("failed to write manifest"); + + let (success, _, stderr) = outcome(&run(&dir.path().join("Cargo.toml"), &[])); + + assert!(success, "a workspace root that is itself a package inherits too: {stderr}"); +} + +#[test] +fn honors_the_allow_list_and_reports_stale_entries() { + let root = concat!( + "[workspace]\nmembers = [\"member\"]\n\n", + "[workspace.metadata.ensure-no-unused-workspace-deps]\nallowed = [\"kept\", \"stale\"]\n\n", + "[workspace.dependencies]\nkept = \"1\"\nstale = \"1\"\n", + ); + let dir = workspace(root, &[("member", "[dependencies]\nstale = { workspace = true }\n")]); + let manifest = dir.path().join("Cargo.toml"); + + let (success, stdout, stderr) = outcome(&run(&manifest, &[])); + + assert!(success, "an allowed entry does not fail the run: {stderr}"); + assert!(stdout.contains("explicitly allowed"), "unexpected stdout: {stdout}"); + assert!( + stderr.contains("'stale' is allowed but is inherited or not declared"), + "unexpected stderr: {stderr}" + ); + assert!( + !stderr.contains("'kept' is allowed"), + "the load-bearing allow entry is not stale: {stderr}" + ); +} + +#[test] +fn fix_removes_the_entries_and_keeps_the_rest_intact() { + let root = concat!( + "[workspace]\nmembers = [\"member\"]\n\n", + "[workspace.dependencies]\n", + "# --- kept ---\n", + "serde = { version = \"1\", default-features = false }\n", + "once_cell = \"1\"\n", + ); + let dir = workspace(root, &[("member", "[dependencies]\nserde = { workspace = true }\n")]); + let manifest = dir.path().join("Cargo.toml"); + + let (success, stdout, stderr) = outcome(&run(&manifest, &["--fix"])); + + assert!(success, "--fix should succeed: {stderr}"); + assert!( + stdout.contains("Removed 1 unused workspace dependency"), + "unexpected stdout: {stdout}" + ); + + let fixed = fs::read_to_string(&manifest).expect("failed to read the fixed manifest"); + assert!(!fixed.contains("once_cell"), "the unused entry should be gone: {fixed}"); + assert!( + fixed.contains("serde = { version = \"1\", default-features = false }"), + "the survivor keeps its formatting: {fixed}" + ); + assert!(fixed.contains("# --- kept ---"), "comments on survivors are preserved: {fixed}"); + + // A fixed manifest is clean on the next run. + let (success, _, _) = outcome(&run(&manifest, &[])); + assert!(success, "the fixed manifest should pass"); +} + +#[test] +fn fix_carries_a_removed_group_header_to_the_next_survivor() { + let root = concat!( + "[workspace]\nmembers = [\"member\"]\n\n", + "[workspace.dependencies]\n", + "# --- unused group ---\n", + "once_cell = \"1\"\n", + "# --- kept group ---\n", + "serde = \"1\"\n", + ); + let dir = workspace(root, &[("member", "[dependencies]\nserde = { workspace = true }\n")]); + let manifest = dir.path().join("Cargo.toml"); + + let (success, _, stderr) = outcome(&run(&manifest, &["--fix"])); + assert!(success, "--fix should succeed: {stderr}"); + + let fixed = fs::read_to_string(&manifest).expect("failed to read the fixed manifest"); + let header = fixed.find("# --- kept group ---").expect("the surviving header is kept"); + let survivor = fixed.find("serde =").expect("the survivor is kept"); + assert!(header < survivor, "the header still introduces its group: {fixed}"); + assert!( + fixed.contains("# --- unused group ---"), + "the removed entry's comment is carried: {fixed}" + ); +} + +#[test] +fn fix_keeps_a_trailing_header_behind_the_last_survivor() { + let root = concat!( + "[workspace]\nmembers = [\"member\"]\n\n", + "[workspace.dependencies]\n", + "serde = \"1\"\n", + "# --- external dependencies ---\n", + "once_cell = \"1\"\n", + ); + let dir = workspace(root, &[("member", "[dependencies]\nserde = { workspace = true }\n")]); + let manifest = dir.path().join("Cargo.toml"); + + let (success, _, stderr) = outcome(&run(&manifest, &["--fix"])); + assert!(success, "--fix should succeed: {stderr}"); + + let fixed = fs::read_to_string(&manifest).expect("failed to read the fixed manifest"); + let survivor = fixed.find("serde =").expect("the survivor is kept"); + let header = fixed.find("# --- external dependencies ---").expect("the carried header is kept"); + assert!( + survivor < header, + "the trailing header must not be hoisted above the survivor: {fixed}" + ); +} + +#[test] +fn fix_on_a_fully_unused_catalog_empties_the_table() { + let root = "[workspace]\nmembers = [\"member\"]\n\n[workspace.dependencies]\n# --- all of it ---\nonce_cell = \"1\"\n"; + let dir = workspace(root, &[("member", "")]); + let manifest = dir.path().join("Cargo.toml"); + + let (success, stdout, stderr) = outcome(&run(&manifest, &["--fix"])); + + assert!(success, "--fix should succeed: {stderr}"); + assert!( + stdout.contains("Removed 1 unused workspace dependency"), + "unexpected stdout: {stdout}" + ); + + let fixed = fs::read_to_string(&manifest).expect("failed to read the fixed manifest"); + assert!(!fixed.contains("once_cell"), "the entry should be gone: {fixed}"); + assert!(fixed.contains("[workspace.dependencies]"), "the empty table stays: {fixed}"); +} + +#[test] +fn a_manifest_without_a_workspace_table_passes_with_a_note() { + let dir = TempDir::new().expect("failed to create temp dir"); + let manifest = dir.path().join("Cargo.toml"); + fs::write(&manifest, "[package]\nname = \"solo\"\nversion = \"0.1.0\"\nedition = \"2021\"\n").expect("failed to write manifest"); + + let (success, _, stderr) = outcome(&run(&manifest, &[])); + + assert!(success, "a non-workspace manifest has no catalog to be wrong about"); + assert!(stderr.contains("has no [workspace] table"), "unexpected stderr: {stderr}"); +} + +#[test] +fn require_workspace_rejects_a_manifest_without_a_workspace_table() { + let dir = TempDir::new().expect("failed to create temp dir"); + let manifest = dir.path().join("Cargo.toml"); + fs::write(&manifest, "[package]\nname = \"solo\"\nversion = \"0.1.0\"\nedition = \"2021\"\n").expect("failed to write manifest"); + + let (success, _, stderr) = outcome(&run(&manifest, &["--require-workspace"])); + + assert!(!success, "--require-workspace makes the missing table an error"); + assert!(stderr.contains("has no [workspace] table"), "unexpected stderr: {stderr}"); +} + +#[test] +fn an_empty_catalog_passes() { + let dir = workspace("[workspace]\nmembers = [\"member\"]\n", &[("member", "")]); + let manifest = dir.path().join("Cargo.toml"); + + let (success, stdout, _) = outcome(&run(&manifest, &[])); + + assert!(success, "no catalog means nothing to check"); + assert!(stdout.contains("declares no workspace dependencies"), "unexpected stdout: {stdout}"); +} + +#[test] +fn a_missing_manifest_is_an_error() { + let dir = TempDir::new().expect("failed to create temp dir"); + let manifest = dir.path().join("Cargo.toml"); + + let (success, _, stderr) = outcome(&run(&manifest, &[])); + + assert!(!success, "a missing manifest is a real failure"); + assert!(stderr.contains("failed to read"), "unexpected stderr: {stderr}"); +} + +#[test] +fn an_unparsable_manifest_is_an_error() { + let dir = TempDir::new().expect("failed to create temp dir"); + let manifest = dir.path().join("Cargo.toml"); + fs::write(&manifest, "[workspace\n").expect("failed to write manifest"); + + let (success, _, stderr) = outcome(&run(&manifest, &[])); + + assert!(!success, "a broken manifest is a real failure"); + assert!(stderr.contains("failed to parse"), "unexpected stderr: {stderr}"); +} + +#[test] +fn a_workspace_cargo_cannot_load_is_an_error() { + // `members` names a directory that has no manifest, so `cargo metadata` + // fails. Reporting that beats silently treating the member as inheriting + // nothing, which would accuse every entry it inherits. + let dir = workspace( + "[workspace]\nmembers = [\"member\", \"ghost\"]\n\n[workspace.dependencies]\nserde = \"1\"\n", + &[("member", "[dependencies]\nserde = { workspace = true }\n")], + ); + let manifest = dir.path().join("Cargo.toml"); + + let (success, _, stderr) = outcome(&run(&manifest, &[])); + + assert!(!success, "an unloadable workspace is a real failure"); + assert!( + stderr.contains("failed to enumerate the workspace members"), + "unexpected stderr: {stderr}" + ); +} From c132cf5b1cc1f76cdd4901dac7a2b332a58a2620 Mon Sep 17 00:00:00 2001 From: "Martin Kolinek (from Dev Box)" Date: Wed, 26 Aug 2026 12:39:47 +0200 Subject: [PATCH 05/11] test: assert where a carried comment lands, not that it exists `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> --- .../tests/integration_tests.rs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/crates/cargo-ensure-no-unused-workspace-deps/tests/integration_tests.rs b/crates/cargo-ensure-no-unused-workspace-deps/tests/integration_tests.rs index b0c12b58..d1137b47 100644 --- a/crates/cargo-ensure-no-unused-workspace-deps/tests/integration_tests.rs +++ b/crates/cargo-ensure-no-unused-workspace-deps/tests/integration_tests.rs @@ -266,11 +266,18 @@ fn fix_carries_a_removed_group_header_to_the_next_survivor() { let fixed = fs::read_to_string(&manifest).expect("failed to read the fixed manifest"); let header = fixed.find("# --- kept group ---").expect("the surviving header is kept"); let survivor = fixed.find("serde =").expect("the survivor is kept"); - assert!(header < survivor, "the header still introduces its group: {fixed}"); + let carried = fixed + .find("# --- unused group ---") + .expect("the removed entry's comment is carried"); + + // Position matters, not mere presence: the carried comment has to land + // ahead of the next surviving entry. Leaving it at the end of the table + // would also keep it in the file, while silently moving it out of place. assert!( - fixed.contains("# --- unused group ---"), - "the removed entry's comment is carried: {fixed}" + carried < header, + "the carried comment must precede the next entry's own decor: {fixed}" ); + assert!(header < survivor, "the header still introduces its group: {fixed}"); } #[test] From db97acb7dbe1478e06918bebd0d29d627206818b Mon Sep 17 00:00:00 2001 From: "Martin Kolinek (from Dev Box)" Date: Wed, 26 Aug 2026 20:20:16 +0200 Subject: [PATCH 06/11] fix: harden --fix and close the review gaps 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> --- CHANGELOG.md | 1 + README.md | 1 + .../CHANGELOG.md | 1 + .../Cargo.toml | 4 +- .../README.md | 12 ++ .../docs/design/README.md | 36 ++++- .../src/detect.rs | 15 +- .../src/fix.rs | 67 +++++++- .../src/lib.rs | 146 ++++++++++++++++-- .../tests/integration_tests.rs | 127 +++++++++++++++ 10 files changed, 382 insertions(+), 28 deletions(-) create mode 100644 crates/cargo-ensure-no-unused-workspace-deps/CHANGELOG.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a7bd45b..3f602d7d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,4 +9,5 @@ Please see each crate's change log below: - [`cargo-each`](./crates/cargo-each/CHANGELOG.md) - [`cargo-ensure-no-cyclic-deps`](./crates/cargo_ensure_no_cyclic_deps/CHANGELOG.md) - [`cargo-ensure-no-default-features`](./crates/cargo-ensure-no-default-features/CHANGELOG.md) +- [`cargo-ensure-no-unused-workspace-deps`](./crates/cargo-ensure-no-unused-workspace-deps/CHANGELOG.md) - [`cargo-heather`](./crates/cargo-heather/CHANGELOG.md) diff --git a/README.md b/README.md index ae8afa3b..88a027a7 100644 --- a/README.md +++ b/README.md @@ -31,6 +31,7 @@ These are the crates built out of this repo: - [`cargo-each`](./crates/cargo-each/README.md) - A cargo subcommand that runs a command over a cargo-style selection of workspace members - [`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-ensure-no-unused-workspace-deps`](./crates/cargo-ensure-no-unused-workspace-deps/README.md) - A cargo subcommand that ensures every [workspace.dependencies] entry is inherited by a workspace member - [`cargo-heather`](./crates/cargo-heather/README.md) - A cargo subcommand to validate license headers in Rust, TOML, PowerShell, Just, and env source files ## About this Repo diff --git a/crates/cargo-ensure-no-unused-workspace-deps/CHANGELOG.md b/crates/cargo-ensure-no-unused-workspace-deps/CHANGELOG.md new file mode 100644 index 00000000..825c32f0 --- /dev/null +++ b/crates/cargo-ensure-no-unused-workspace-deps/CHANGELOG.md @@ -0,0 +1 @@ +# Changelog diff --git a/crates/cargo-ensure-no-unused-workspace-deps/Cargo.toml b/crates/cargo-ensure-no-unused-workspace-deps/Cargo.toml index 971635ab..61467fd5 100644 --- a/crates/cargo-ensure-no-unused-workspace-deps/Cargo.toml +++ b/crates/cargo-ensure-no-unused-workspace-deps/Cargo.toml @@ -32,10 +32,8 @@ path = "src/main.rs" anyhow = { workspace = true, features = ["std"] } cargo_metadata = { workspace = true } clap = { workspace = true, features = ["std", "derive", "color", "help", "error-context", "usage"] } -toml_edit = { workspace = true } - -[dev-dependencies] tempfile = { workspace = true } +toml_edit = { workspace = true } # >>> anvil-managed: anvil-lints [lints] diff --git a/crates/cargo-ensure-no-unused-workspace-deps/README.md b/crates/cargo-ensure-no-unused-workspace-deps/README.md index 3e9924bc..f0e65387 100644 --- a/crates/cargo-ensure-no-unused-workspace-deps/README.md +++ b/crates/cargo-ensure-no-unused-workspace-deps/README.md @@ -60,6 +60,18 @@ allowed = ["kept-on-purpose"] An `allowed` name that suppresses nothing is reported as stale, on stderr, without failing the run. +## Fixing + +`--fix` replaces the manifest atomically – a temporary file in the same +directory, renamed over the original – and refuses to write at all if the +file changed after it was read, so a concurrent edit is never clobbered. + +Comments on a removed entry are carried to the next surviving entry, which +keeps a group header attached to the group it introduces. A note about one +specific dependency is indistinguishable from such a header, so every move +is reported on stderr: check that carried text still describes the entry it +landed on. + ## Installation ```bash diff --git a/crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md b/crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md index 41d96376..4402cc8f 100644 --- a/crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md +++ b/crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md @@ -165,6 +165,27 @@ dropped. contributed a node to the dependency graph. The tool never touches the lockfile, and a lockfile that changes after a fix indicates unrelated drift. +The workspace root is the one file whose loss breaks every other tool in the +repository, so it is never truncated in place. The replacement is written to a +temporary file in the manifest's own directory and renamed over the original, which +is atomic on a single filesystem. Before that rename the file is re-read and compared +against the bytes that were parsed: `cargo metadata` runs in between as a subprocess, +which is a wide enough window for an editor to save into, and an edit that lands there +aborts the fix rather than being overwritten. + +#### Carried comments can be misattributed + +A group header and a note about one specific dependency are the same thing to the +parser — comment lines in an entry's decor. When the noted entry is the one removed, +its note lands on the next surviving entry and reads as if it were written about that +one, which is worse than dropping it: a dropped comment shows up in the `--fix` diff, +a wrong attribution outlives it. + +The carry-forward still earns its keep for headers, so it stays, and the relocation is +made visible instead: every move is reported on stderr, naming the entries the +comments came from and the entry they landed on, so whoever reviews the diff knows +which lines to check. + ## 6. Relationship to the other dependency checks | Question | Answered by | @@ -188,12 +209,15 @@ since) was likewise rejected as a pinned dependency. ## 7. CI integration -The check joins the `modified` tier and runs in the `pr-fast` group as -`cargo ensure-no-unused-workspace-deps`, alongside `ensure-no-cyclic-deps` and -`ensure-no-default-features`. It is a text/metadata check: one platform is enough, -no toolchain pin is required, and it is wired through cargo-anvil like its siblings — -a pinned version in `versions.just`, install and validate recipes in `tools.just`, -and a check recipe in `checks/`. +The check belongs in the `modified` tier, running in the `pr-fast` group as +`cargo ensure-no-unused-workspace-deps` alongside `ensure-no-cyclic-deps` and +`ensure-no-default-features`. It is a text/metadata check: one platform is enough and +no toolchain pin is required. + +**Not yet wired.** Anvil installs pinned tools from crates.io, so the wiring — a +pinned version in `versions.just`, install and validate recipes in `tools.just`, a +check recipe in `checks/`, and the `pr-fast` entry — follows the crate's first +release. Until then the tool is published and runnable but enforces nothing here. Because the tool reads the workspace root, it runs once from the repository root rather than per affected package. Single-crate repositories run the same command and diff --git a/crates/cargo-ensure-no-unused-workspace-deps/src/detect.rs b/crates/cargo-ensure-no-unused-workspace-deps/src/detect.rs index 08fc4d8a..be18e4fc 100644 --- a/crates/cargo-ensure-no-unused-workspace-deps/src/detect.rs +++ b/crates/cargo-ensure-no-unused-workspace-deps/src/detect.rs @@ -34,13 +34,22 @@ pub struct WorkspaceCatalog { pub allowed: BTreeSet, } -/// Read and parse a manifest. -pub fn read_manifest(path: &Path) -> Result { - let text = std::fs::read_to_string(path).with_context(|| format!("failed to read {}", path.display()))?; +/// Read a manifest's text. +pub fn read_manifest_text(path: &Path) -> Result { + std::fs::read_to_string(path).with_context(|| format!("failed to read {}", path.display())) +} + +/// Parse manifest text that came from `path`. +pub fn parse_manifest(text: &str, path: &Path) -> Result { text.parse::() .with_context(|| format!("failed to parse {}", path.display())) } +/// Read and parse a manifest. +pub fn read_manifest(path: &Path) -> Result { + parse_manifest(&read_manifest_text(path)?, path) +} + /// Classify a parsed manifest and, when it is a workspace root, collect its /// catalog and allow-list. pub fn catalog(manifest: &DocumentMut) -> Catalog { diff --git a/crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs b/crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs index f639ff7a..04524009 100644 --- a/crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs +++ b/crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs @@ -8,11 +8,39 @@ use std::collections::BTreeSet; use toml_edit::{DocumentMut, Item}; -/// Remove `names` from the catalog and return how many entries went away. +/// What a `--fix` did. +pub struct Outcome { + /// How many entries were removed. + pub removed: usize, + + /// Comment blocks that moved off a removed entry. + pub carries: Vec, +} + +/// Comments that belonged to removed entries and had to go somewhere else. +/// +/// Only recorded when comments actually moved, so `from` is never empty. +/// +/// Reported so the relocation is visible: the carry-forward cannot tell a group +/// header from a note about one specific dependency, and a note that lands on +/// the next entry reads as if it were about that one. +pub struct Carry { + /// Entries whose comments were carried, in manifest order. + pub from: Vec, + + /// The entry the comments landed on, or `None` when the table was emptied + /// and they were dropped. + pub onto: Option, + + /// How many comment lines moved. + pub lines: usize, +} + +/// Remove `names` from the catalog. /// /// Comments attached to a removed entry are carried forward to the next /// surviving entry, so a group header keeps labeling the group it introduces. -pub fn remove(manifest: &mut DocumentMut, names: &[String]) -> usize { +pub fn remove(manifest: &mut DocumentMut, names: &[String]) -> Outcome { let table = manifest .get_mut("workspace") .and_then(Item::as_table_like_mut) @@ -23,8 +51,12 @@ pub fn remove(manifest: &mut DocumentMut, names: &[String]) -> usize { let order: Vec = table.iter().map(|(key, _)| key.to_owned()).collect(); let doomed: BTreeSet<&str> = names.iter().map(String::as_str).collect(); - let mut removed = 0; + let mut outcome = Outcome { + removed: 0, + carries: Vec::new(), + }; let mut carried = String::new(); + let mut sources: Vec = Vec::new(); for name in &order { let prefix = table @@ -35,13 +67,22 @@ pub fn remove(manifest: &mut DocumentMut, names: &[String]) -> usize { .to_owned(); if doomed.contains(name.as_str()) { - carried.push_str(&comments_of(&prefix)); + let comments = comments_of(&prefix); + if !comments.is_empty() { + sources.push(name.clone()); + } + carried.push_str(&comments); table.remove(name); - removed += 1; + outcome.removed += 1; } else if !carried.is_empty() { if let Some(mut key) = table.key_mut(name) { key.leaf_decor_mut().set_prefix(format!("{carried}{prefix}")); } + outcome.carries.push(Carry { + from: std::mem::take(&mut sources), + onto: Some(name.clone()), + lines: comment_lines(&carried), + }); carried.clear(); } } @@ -54,7 +95,8 @@ pub fn remove(manifest: &mut DocumentMut, names: &[String]) -> usize { // // When every entry was removed there is no surviving entry at all, and // the comments go with the group they introduced. - if let Some(last) = table.iter().last().map(|(key, _)| key.to_owned()) + let last = table.iter().last().map(|(key, _)| key.to_owned()); + if let Some(last) = last.clone() && let Some(value) = table.get_mut(&last).and_then(Item::as_value_mut) { let suffix = value @@ -65,9 +107,15 @@ pub fn remove(manifest: &mut DocumentMut, names: &[String]) -> usize { .to_owned(); value.decor_mut().set_suffix(format!("{suffix}{carried}")); } + + outcome.carries.push(Carry { + from: std::mem::take(&mut sources), + onto: last, + lines: comment_lines(&carried), + }); } - removed + outcome } /// The comment-bearing part of a removed entry's decor. @@ -81,3 +129,8 @@ fn comments_of(prefix: &str) -> String { String::new() } } + +/// How many lines of `decor` are comments. +fn comment_lines(decor: &str) -> usize { + decor.lines().filter(|line| line.trim_start().starts_with('#')).count() +} diff --git a/crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs b/crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs index 26312988..2a8a38d6 100644 --- a/crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs +++ b/crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs @@ -54,6 +54,18 @@ //! An `allowed` name that suppresses nothing is reported as stale, on stderr, //! without failing the run. //! +//! # Fixing +//! +//! `--fix` replaces the manifest atomically -- a temporary file in the same +//! directory, renamed over the original -- and refuses to write at all if the +//! file changed after it was read, so a concurrent edit is never clobbered. +//! +//! Comments on a removed entry are carried to the next surviving entry, which +//! keeps a group header attached to the group it introduces. A note about one +//! specific dependency is indistinguishable from such a header, so every move +//! is reported on stderr: check that carried text still describes the entry it +//! landed on. +//! //! # Installation //! //! ```bash @@ -77,16 +89,19 @@ mod detect; mod fix; use std::collections::BTreeSet; +use std::io::Write; use std::path::{Path, PathBuf}; use std::process::ExitCode; -use anyhow::{Context, Result}; +use anyhow::{Context, Result, ensure}; use cargo_metadata::MetadataCommand; use clap::builder::Styles; use clap::builder::styling::{AnsiColor, Effects}; use clap::{Parser, Subcommand}; +use tempfile::NamedTempFile; use crate::detect::{Catalog, WorkspaceCatalog}; +use crate::fix::Carry; const CLAP_STYLES: Styles = Styles::styled() .header(AnsiColor::Green.on_default().effects(Effects::BOLD)) @@ -150,7 +165,8 @@ pub fn run() -> Result { /// The check itself, split from [`run`] so tests can drive it without a process /// boundary or a parsed command line. fn check(manifest_path: &Path, fix: bool, require_workspace: bool) -> Result { - let mut manifest = detect::read_manifest(manifest_path)?; + let original = detect::read_manifest_text(manifest_path)?; + let mut manifest = detect::parse_manifest(&original, manifest_path)?; let catalog = match detect::catalog(&manifest) { Catalog::Workspace(catalog) => catalog, @@ -169,6 +185,11 @@ fn check(manifest_path: &Path, fix: bool, require_workspace: bool) -> Result Result Result<()> { + // Eagerly formatted rather than built in `with_context` closures: those + // closures only run on failures no test can force portably. + let read_failure = format!("failed to re-read {} before writing it", manifest_path.display()); + let write_failure = format!("failed to write {}", manifest_path.display()); + let persist_failure = format!("failed to replace {}", manifest_path.display()); + + let current = std::fs::read_to_string(manifest_path).context(read_failure)?; + ensure!( + current == original, + "{} changed on disk while the check was running; not writing", + manifest_path.display() + ); + + let directory = manifest_path + .parent() + .expect("the manifest path always names a file, so it always has a parent directory"); + + // Same directory as the manifest, so the rename stays on one filesystem. + let mut staged = NamedTempFile::new_in(directory).context(write_failure.clone())?; + staged.write_all(contents.as_bytes()).context(write_failure)?; + staged.persist(manifest_path).context(persist_failure)?; + + Ok(()) +} + /// Manifest paths of every workspace member, as Cargo resolves them. /// /// Deferring to `cargo metadata` rather than re-deriving `members`, its globs @@ -230,6 +285,30 @@ fn report_stale(stale: &[String]) { } } +/// Report comments that moved off a removed entry. +/// +/// A group header and a note about one specific dependency have identical +/// decor, so carrying the note onto the next entry makes it read as if it were +/// about that one. Naming what moved and where puts the reviewer of the `--fix` +/// diff on the right lines. +fn report_carries(carries: &[Carry]) { + for carry in carries { + let sources = carry.from.join("', '"); + match carry.onto.as_ref() { + Some(onto) => eprintln!( + "⚠️ Carried {} comment {} from '{sources}' onto '{onto}'; check that the text still describes '{onto}'.", + carry.lines, + lines(carry.lines) + ), + None => eprintln!( + "⚠️ Dropped {} comment {} from '{sources}': every entry in the table was removed.", + carry.lines, + lines(carry.lines) + ), + } + } +} + /// Report a catalog in which every entry is inherited or allowed. fn report_clean(manifest_path: &Path, catalog: &WorkspaceCatalog, used: &BTreeSet, members: usize) { let declared = catalog.declared.len(); @@ -268,3 +347,52 @@ fn report_unused(manifest_path: &Path, unused: &[String]) { fn entries(count: usize) -> &'static str { if count == 1 { "dependency" } else { "dependencies" } } + +/// Pluralize `line` for `count`. +fn lines(count: usize) -> &'static str { + if count == 1 { "line" } else { "lines" } +} + +#[cfg(test)] +#[cfg_attr(coverage_nightly, coverage(off))] +mod tests { + use std::fs; + + use tempfile::TempDir; + + use super::write_back; + + /// The unchanged-input guard cannot be driven from an integration test: the + /// window it protects is between the read and the write of a single run, so + /// forcing a change inside it would mean racing a child process. Exercised + /// directly instead. + #[test] + fn write_back_replaces_a_manifest_that_is_unchanged() { + let dir = TempDir::new().expect("failed to create temp dir"); + let path = dir.path().join("Cargo.toml"); + fs::write(&path, "original").expect("failed to seed the manifest"); + + write_back(&path, "original", "replacement").expect("an unchanged manifest is replaced"); + + assert_eq!(fs::read_to_string(&path).expect("failed to read back"), "replacement"); + } + + #[test] + fn write_back_refuses_a_manifest_that_changed_under_it() { + let dir = TempDir::new().expect("failed to create temp dir"); + let path = dir.path().join("Cargo.toml"); + fs::write(&path, "edited by someone else").expect("failed to seed the manifest"); + + let error = write_back(&path, "original", "replacement").expect_err("a changed manifest is refused"); + + assert!( + error.to_string().contains("changed on disk while the check was running"), + "unexpected error: {error}" + ); + assert_eq!( + fs::read_to_string(&path).expect("failed to read back"), + "edited by someone else", + "the competing edit must survive" + ); + } +} diff --git a/crates/cargo-ensure-no-unused-workspace-deps/tests/integration_tests.rs b/crates/cargo-ensure-no-unused-workspace-deps/tests/integration_tests.rs index d1137b47..4c1e8e6a 100644 --- a/crates/cargo-ensure-no-unused-workspace-deps/tests/integration_tests.rs +++ b/crates/cargo-ensure-no-unused-workspace-deps/tests/integration_tests.rs @@ -323,6 +323,133 @@ fn fix_on_a_fully_unused_catalog_empties_the_table() { assert!(fixed.contains("[workspace.dependencies]"), "the empty table stays: {fixed}"); } +#[test] +fn fix_keeps_an_allowed_entry_while_removing_the_others() { + // The destructive path must honour the allow-list too: reporting suppresses + // an allowed finding, and `--fix` has to leave that entry on disk. + let root = concat!( + "[workspace]\nmembers = [\"member\"]\n\n", + "[workspace.metadata.ensure-no-unused-workspace-deps]\nallowed = [\"kept\"]\n\n", + "[workspace.dependencies]\nkept = \"1\"\nonce_cell = \"1\"\nserde = \"1\"\n", + ); + let dir = workspace(root, &[("member", "[dependencies]\nserde = { workspace = true }\n")]); + let manifest = dir.path().join("Cargo.toml"); + + let (success, stdout, stderr) = outcome(&run(&manifest, &["--fix"])); + + assert!(success, "--fix should succeed: {stderr}"); + assert!( + stdout.contains("Removed 1 unused workspace dependency"), + "unexpected stdout: {stdout}" + ); + + let fixed = fs::read_to_string(&manifest).expect("failed to read the fixed manifest"); + assert!(fixed.contains("kept = \"1\""), "an allowed entry must survive --fix: {fixed}"); + assert!(fixed.contains("serde = \"1\""), "an inherited entry must survive --fix: {fixed}"); + assert!(!fixed.contains("once_cell"), "the ordinary unused entry should be gone: {fixed}"); +} + +#[test] +fn member_globs_and_exclusions_follow_cargo() { + // The reason this tool shells out to `cargo metadata` rather than reading + // `members` itself: globs expand and `exclude` subtracts. A literal reading + // of `members` would treat the excluded package as a member and call + // `only_excluded_uses` inherited. + let dir = TempDir::new().expect("failed to create temp dir"); + fs::write( + dir.path().join("Cargo.toml"), + concat!( + "[workspace]\nmembers = [\"crates/*\"]\nexclude = [\"crates/excluded\"]\n\n", + "[workspace.dependencies]\nboth_use = \"1\"\nonly_excluded_uses = \"1\"\n", + ), + ) + .expect("failed to write workspace manifest"); + + for (name, dep) in [("included", "both_use"), ("excluded", "only_excluded_uses")] { + let member = dir.path().join("crates").join(name); + fs::create_dir_all(member.join("src")).expect("failed to create member dir"); + fs::write(member.join("src").join("lib.rs"), "").expect("failed to write member source"); + fs::write( + member.join("Cargo.toml"), + format!( + "[package]\nname = \"{name}\"\nversion = \"0.1.0\"\nedition = \"2021\"\n\n[dependencies]\n{dep} = {{ workspace = true }}\n" + ), + ) + .expect("failed to write member manifest"); + } + + let (success, _, stderr) = outcome(&run(&dir.path().join("Cargo.toml"), &[])); + + assert!(!success, "the entry only the excluded package inherits is unused"); + assert!(stderr.contains("- only_excluded_uses"), "unexpected stderr: {stderr}"); + assert!( + !stderr.contains("- both_use"), + "the glob-expanded member's entry is inherited: {stderr}" + ); +} + +#[test] +fn a_stale_allow_entry_is_reported_against_an_empty_catalog() { + // The degenerate boundary: with no catalog at all, every allowed name + // suppresses nothing, so every one of them is stale. + let root = concat!( + "[workspace]\nmembers = [\"member\"]\n\n", + "[workspace.metadata.ensure-no-unused-workspace-deps]\nallowed = [\"old\"]\n", + ); + let dir = workspace(root, &[("member", "")]); + let manifest = dir.path().join("Cargo.toml"); + + let (success, stdout, stderr) = outcome(&run(&manifest, &[])); + + assert!(success, "an empty catalog still passes"); + assert!(stdout.contains("declares no workspace dependencies"), "unexpected stdout: {stdout}"); + assert!( + stderr.contains("'old' is allowed but"), + "a stale allow entry must still be reported: {stderr}" + ); +} + +#[test] +fn fix_reports_which_comments_moved_and_where() { + // A note about one entry and a group header are indistinguishable, so the + // relocation has to be visible in the run's output. + let root = concat!( + "[workspace]\nmembers = [\"member\"]\n\n", + "[workspace.dependencies]\n", + "# pinned to 1.2 until upstream #42 is fixed\n", + "# revisit after the 2.0 release\n", + "once_cell = \"1\"\n", + "serde = \"1\"\n", + ); + let dir = workspace(root, &[("member", "[dependencies]\nserde = { workspace = true }\n")]); + let manifest = dir.path().join("Cargo.toml"); + + let (success, _, stderr) = outcome(&run(&manifest, &["--fix"])); + + assert!(success, "--fix should succeed: {stderr}"); + // The count has to be the real number of comment lines, not a placeholder: + // it tells the reviewer how much text to re-read. + assert!( + stderr.contains("Carried 2 comment lines from 'once_cell' onto 'serde'"), + "the move must be reported with its size: {stderr}" + ); +} + +#[test] +fn fix_reports_comments_dropped_with_an_emptied_table() { + let root = "[workspace]\nmembers = [\"member\"]\n\n[workspace.dependencies]\n# --- all of it ---\nonce_cell = \"1\"\n"; + let dir = workspace(root, &[("member", "")]); + let manifest = dir.path().join("Cargo.toml"); + + let (success, _, stderr) = outcome(&run(&manifest, &["--fix"])); + + assert!(success, "--fix should succeed: {stderr}"); + assert!( + stderr.contains("Dropped 1 comment line from 'once_cell'"), + "the drop must be reported: {stderr}" + ); +} + #[test] fn a_manifest_without_a_workspace_table_passes_with_a_note() { let dir = TempDir::new().expect("failed to create temp dir"); From f704af90905309ba27671328ffc837900d3a4acc Mon Sep 17 00:00:00 2001 From: "Martin Kolinek (from Dev Box)" Date: Wed, 26 Aug 2026 20:35:14 +0200 Subject: [PATCH 07/11] fix: exclude the write-guard unit tests from miri 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> --- crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs b/crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs index 2a8a38d6..61a14e36 100644 --- a/crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs +++ b/crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs @@ -354,6 +354,9 @@ fn lines(count: usize) -> &'static str { } #[cfg(test)] +// Miri runs with filesystem isolation, and these tests need real files in a +// real temp directory. +#[cfg(not(miri))] #[cfg_attr(coverage_nightly, coverage(off))] mod tests { use std::fs; From d4a95e230363b13865a49a867387478828ea1fa1 Mon Sep 17 00:00:00 2001 From: "Martin Kolinek (from Dev Box)" Date: Thu, 27 Aug 2026 19:20:41 +0200 Subject: [PATCH 08/11] fix: preserve manifest identity on --fix and report carries honestly 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> --- .spelling | 3 + .../README.md | 10 ++- .../docs/design/README.md | 24 +++++- .../src/fix.rs | 75 +++++++++++++------ .../src/lib.rs | 42 +++++++++-- .../tests/integration_tests.rs | 63 +++++++++++++++- 6 files changed, 177 insertions(+), 40 deletions(-) diff --git a/.spelling b/.spelling index b1af0376..390292bb 100644 --- a/.spelling +++ b/.spelling @@ -521,3 +521,6 @@ deprecations parallelization remediate recency +symlink +symlinked +symlinks diff --git a/crates/cargo-ensure-no-unused-workspace-deps/README.md b/crates/cargo-ensure-no-unused-workspace-deps/README.md index f0e65387..806c8032 100644 --- a/crates/cargo-ensure-no-unused-workspace-deps/README.md +++ b/crates/cargo-ensure-no-unused-workspace-deps/README.md @@ -63,14 +63,18 @@ without failing the run. ## Fixing `--fix` replaces the manifest atomically – a temporary file in the same -directory, renamed over the original – and refuses to write at all if the -file changed after it was read, so a concurrent edit is never clobbered. +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. Comments on a removed entry are carried to the next surviving entry, which keeps a group header attached to the group it introduces. A note about one specific dependency is indistinguishable from such a header, so every move is reported on stderr: check that carried text still describes the entry it -landed on. +landed on. Comments that cannot be placed – nothing survives, or the last +survivor is a dotted key or sub-table with no value to append to – are +reported as dropped. ## Installation diff --git a/crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md b/crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md index 4402cc8f..2d293ddc 100644 --- a/crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md +++ b/crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md @@ -173,19 +173,35 @@ against the bytes that were parsed: `cargo metadata` runs in between as a subpro which is a wide enough window for an editor to save into, and an edit that lands there aborts the fix rather than being overwritten. +Replacing a file by rename brings the temporary file's identity with it, so two +properties an in-place write would have kept are restored deliberately. The manifest's +permissions are read first and applied to the replacement, because a temporary file is +created owner-only and a rename carries its mode rather than inheriting the target's — +otherwise a world-readable manifest silently comes back owner-only, which git does not +track. A symlinked manifest is resolved first, so the rename lands on the file the link +points at instead of replacing the link with a regular file. + #### Carried comments can be misattributed A group header and a note about one specific dependency are the same thing to the -parser — comment lines in an entry's decor. When the noted entry is the one removed, -its note lands on the next surviving entry and reads as if it were written about that -one, which is worse than dropping it: a dropped comment shows up in the `--fix` diff, -a wrong attribution outlives it. +parser — comment lines in an entry's decor, whether that decor sits on the key or, for +a `[workspace.dependencies.name]` entry, on the table. When the noted entry is the one +removed, its note lands on the next surviving entry and reads as if it were written +about that one, which is worse than dropping it: a dropped comment shows up in the +`--fix` diff, a wrong attribution outlives it. The carry-forward still earns its keep for headers, so it stays, and the relocation is made visible instead: every move is reported on stderr, naming the entries the comments came from and the entry they landed on, so whoever reviews the diff knows which lines to check. +Comments cannot always be placed. Only a plain value has a suffix to append to, so when +the removed entries are last in the table and the final survivor is a dotted key or a +sub-table — and when every entry is removed and nothing survives — the comments go with +the group they introduced, reported as a drop. The report describes what happened +rather than what was attempted: claiming a move that did not happen would send the +reviewer hunting for text that is not in the diff. + ## 6. Relationship to the other dependency checks | Question | Answered by | diff --git a/crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs b/crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs index 04524009..00fb8ad3 100644 --- a/crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs +++ b/crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs @@ -6,7 +6,7 @@ use std::collections::BTreeSet; -use toml_edit::{DocumentMut, Item}; +use toml_edit::{DocumentMut, Item, TableLike}; /// What a `--fix` did. pub struct Outcome { @@ -59,12 +59,7 @@ pub fn remove(manifest: &mut DocumentMut, names: &[String]) -> Outcome { let mut sources: Vec = Vec::new(); for name in &order { - let prefix = table - .key(name) - .and_then(|key| key.leaf_decor().prefix()) - .and_then(toml_edit::RawString::as_str) - .unwrap_or_default() - .to_owned(); + let prefix = decor_prefix(table, name); if doomed.contains(name.as_str()) { let comments = comments_of(&prefix); @@ -75,12 +70,18 @@ pub fn remove(manifest: &mut DocumentMut, names: &[String]) -> Outcome { table.remove(name); outcome.removed += 1; } else if !carried.is_empty() { - if let Some(mut key) = table.key_mut(name) { + // `onto` reports where the comments actually landed. Claiming a + // move that did not happen sends the reviewer of the `--fix` diff + // hunting for text that is not there, which is the failure this + // reporting exists to prevent. + let onto = table.key_mut(name).map(|mut key| { key.leaf_decor_mut().set_prefix(format!("{carried}{prefix}")); - } + name.clone() + }); + outcome.carries.push(Carry { from: std::mem::take(&mut sources), - onto: Some(name.clone()), + onto, lines: comment_lines(&carried), }); carried.clear(); @@ -93,24 +94,28 @@ pub fn remove(manifest: &mut DocumentMut, names: &[String]) -> Outcome { // surviving entry's *value* instead: attaching them to that entry's key // prefix would hoist them above it and relabel a surviving dependency. // - // When every entry was removed there is no surviving entry at all, and - // the comments go with the group they introduced. + // Only a plain value has a suffix to append to. When the last survivor + // is a dotted key or a sub-table, and when every entry was removed and + // there is no survivor at all, the comments go with the group they + // introduced -- reported as a drop, because that is what happened. let last = table.iter().last().map(|(key, _)| key.to_owned()); - if let Some(last) = last.clone() - && let Some(value) = table.get_mut(&last).and_then(Item::as_value_mut) - { - let suffix = value - .decor() - .suffix() - .and_then(toml_edit::RawString::as_str) - .unwrap_or_default() - .to_owned(); - value.decor_mut().set_suffix(format!("{suffix}{carried}")); - } + let onto = last.and_then(|last| { + let appended = table.get_mut(&last).and_then(Item::as_value_mut).map(|value| { + let suffix = value + .decor() + .suffix() + .and_then(toml_edit::RawString::as_str) + .unwrap_or_default() + .to_owned(); + value.decor_mut().set_suffix(format!("{suffix}{carried}")); + }); + + appended.map(|()| last) + }); outcome.carries.push(Carry { from: std::mem::take(&mut sources), - onto: last, + onto, lines: comment_lines(&carried), }); } @@ -118,6 +123,28 @@ pub fn remove(manifest: &mut DocumentMut, names: &[String]) -> Outcome { outcome } +/// The decor preceding one entry, from wherever `toml_edit` keeps it. +/// +/// A plain entry carries its comments on the key. A sub-table entry -- +/// `[workspace.dependencies.name]` -- carries them on the table instead, and +/// reading only the key would let those comments disappear unremarked. +fn decor_prefix(table: &dyn TableLike, name: &str) -> String { + let on_table = table + .get(name) + .and_then(Item::as_table) + .and_then(|nested| nested.decor().prefix()) + .and_then(toml_edit::RawString::as_str) + .unwrap_or_default(); + + let on_key = table + .key(name) + .and_then(|key| key.leaf_decor().prefix()) + .and_then(toml_edit::RawString::as_str) + .unwrap_or_default(); + + format!("{on_table}{on_key}") +} + /// The comment-bearing part of a removed entry's decor. /// /// Blank-line padding is dropped: only comments such as a `# --- group ---` diff --git a/crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs b/crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs index 61a14e36..fb381cfc 100644 --- a/crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs +++ b/crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs @@ -57,14 +57,18 @@ //! # Fixing //! //! `--fix` replaces the manifest atomically -- a temporary file in the same -//! directory, renamed over the original -- and refuses to write at all if the -//! file changed after it was read, so a concurrent edit is never clobbered. +//! 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. //! //! Comments on a removed entry are carried to the next surviving entry, which //! keeps a group header attached to the group it introduces. A note about one //! specific dependency is indistinguishable from such a header, so every move //! is reported on stderr: check that carried text still describes the entry it -//! landed on. +//! landed on. Comments that cannot be placed -- nothing survives, or the last +//! survivor is a dotted key or sub-table with no value to append to -- are +//! reported as dropped. //! //! # Installation //! @@ -233,28 +237,50 @@ fn check(manifest_path: &Path, fix: bool, require_workspace: bool) -> Result Result<()> { // Eagerly formatted rather than built in `with_context` closures: those // closures only run on failures no test can force portably. + let resolve_failure = format!("failed to resolve {}", manifest_path.display()); let read_failure = format!("failed to re-read {} before writing it", manifest_path.display()); + let metadata_failure = format!("failed to read the permissions of {}", manifest_path.display()); let write_failure = format!("failed to write {}", manifest_path.display()); + let permissions_failure = format!("failed to apply the permissions of {} to its replacement", manifest_path.display()); let persist_failure = format!("failed to replace {}", manifest_path.display()); - let current = std::fs::read_to_string(manifest_path).context(read_failure)?; + // Follow a symlinked manifest through to its target, the way an in-place + // write would have. + let target = std::fs::canonicalize(manifest_path).context(resolve_failure)?; + + let current = std::fs::read_to_string(&target).context(read_failure)?; ensure!( current == original, "{} changed on disk while the check was running; not writing", manifest_path.display() ); - let directory = manifest_path + let permissions = std::fs::metadata(&target).context(metadata_failure)?.permissions(); + let directory = target .parent() - .expect("the manifest path always names a file, so it always has a parent directory"); + .expect("a canonicalized file path always names a file, so it always has a parent directory"); // Same directory as the manifest, so the rename stays on one filesystem. let mut staged = NamedTempFile::new_in(directory).context(write_failure.clone())?; staged.write_all(contents.as_bytes()).context(write_failure)?; - staged.persist(manifest_path).context(persist_failure)?; + staged.as_file().set_permissions(permissions).context(permissions_failure)?; + staged.persist(&target).context(persist_failure)?; Ok(()) } @@ -301,7 +327,7 @@ fn report_carries(carries: &[Carry]) { lines(carry.lines) ), None => eprintln!( - "⚠️ Dropped {} comment {} from '{sources}': every entry in the table was removed.", + "⚠️ Dropped {} comment {} from '{sources}': no surviving entry could carry them.", carry.lines, lines(carry.lines) ), diff --git a/crates/cargo-ensure-no-unused-workspace-deps/tests/integration_tests.rs b/crates/cargo-ensure-no-unused-workspace-deps/tests/integration_tests.rs index 4c1e8e6a..040bc450 100644 --- a/crates/cargo-ensure-no-unused-workspace-deps/tests/integration_tests.rs +++ b/crates/cargo-ensure-no-unused-workspace-deps/tests/integration_tests.rs @@ -302,6 +302,13 @@ fn fix_keeps_a_trailing_header_behind_the_last_survivor() { survivor < header, "the trailing header must not be hoisted above the survivor: {fixed}" ); + + // The trailing branch reports too: this is the path that used to claim a + // carry whether or not the append had actually happened. + assert!( + stderr.contains("Carried 1 comment line from 'once_cell' onto 'serde'"), + "the trailing carry must be reported: {stderr}" + ); } #[test] @@ -445,11 +452,65 @@ fn fix_reports_comments_dropped_with_an_emptied_table() { assert!(success, "--fix should succeed: {stderr}"); assert!( - stderr.contains("Dropped 1 comment line from 'once_cell'"), + stderr.contains("Dropped 1 comment line from 'once_cell': no surviving entry could carry them."), "the drop must be reported: {stderr}" ); } +#[test] +fn fix_reports_a_drop_when_the_last_survivor_cannot_carry_comments() { + // Only a plain value has a suffix to append to. A dotted survivor is an + // `Item::Table`, so the comment cannot be attached and is dropped -- and + // the report has to say so rather than claim a move that did not happen. + let root = concat!( + "[workspace]\nmembers = [\"member\"]\n\n", + "[workspace.dependencies]\n", + "serde.version = \"1\"\n", + "# pinned to 1.2 until upstream #42 is fixed\n", + "once_cell = \"1\"\n", + ); + let dir = workspace(root, &[("member", "[dependencies]\nserde = { workspace = true }\n")]); + let manifest = dir.path().join("Cargo.toml"); + + let (success, _, stderr) = outcome(&run(&manifest, &["--fix"])); + + assert!(success, "--fix should succeed: {stderr}"); + + let fixed = fs::read_to_string(&manifest).expect("failed to read the fixed manifest"); + assert!(!fixed.contains("pinned to 1.2"), "the comment really is gone: {fixed}"); + assert!( + stderr.contains("Dropped 1 comment line from 'once_cell'"), + "a comment that was dropped must not be reported as carried: {stderr}" + ); + assert!(!stderr.contains("Carried"), "nothing was carried here: {stderr}"); +} + +#[test] +fn fix_carries_a_comment_from_a_removed_sub_table_entry() { + // A `[workspace.dependencies.name]` entry keeps its comments on the table, + // not on the key, so reading only the key decor would drop them unremarked. + let root = concat!( + "[workspace]\nmembers = [\"member\"]\n\n", + "[workspace.dependencies]\nserde = \"1\"\n\n", + "# pinned to 1.2 until upstream #42 is fixed\n", + "[workspace.dependencies.once_cell]\nversion = \"1\"\n", + ); + let dir = workspace(root, &[("member", "[dependencies]\nserde = { workspace = true }\n")]); + let manifest = dir.path().join("Cargo.toml"); + + let (success, _, stderr) = outcome(&run(&manifest, &["--fix"])); + + assert!(success, "--fix should succeed: {stderr}"); + + let fixed = fs::read_to_string(&manifest).expect("failed to read the fixed manifest"); + assert!(!fixed.contains("once_cell"), "the unused entry should be gone: {fixed}"); + assert!(fixed.contains("pinned to 1.2"), "the sub-table's comment must survive: {fixed}"); + assert!( + stderr.contains("Carried 1 comment line from 'once_cell' onto 'serde'"), + "the move must be reported: {stderr}" + ); +} + #[test] fn a_manifest_without_a_workspace_table_passes_with_a_note() { let dir = TempDir::new().expect("failed to create temp dir"); From 2f8c34e691de183ff2f1af3cbde7da4d8fcc6a8b Mon Sep 17 00:00:00 2001 From: "Martin Kolinek (from Dev Box)" Date: Fri, 28 Aug 2026 10:36:15 +0200 Subject: [PATCH 09/11] fix: read and write carried comments through the same decor slot 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> --- .../README.md | 6 +- .../docs/design/README.md | 28 +++-- .../src/fix.rs | 110 +++++++++++++----- .../src/lib.rs | 6 +- .../tests/integration_tests.rs | 96 +++++++++++++++ 5 files changed, 200 insertions(+), 46 deletions(-) diff --git a/crates/cargo-ensure-no-unused-workspace-deps/README.md b/crates/cargo-ensure-no-unused-workspace-deps/README.md index 806c8032..bc1c51ac 100644 --- a/crates/cargo-ensure-no-unused-workspace-deps/README.md +++ b/crates/cargo-ensure-no-unused-workspace-deps/README.md @@ -72,9 +72,9 @@ Comments on a removed entry are carried to the next surviving entry, which keeps a group header attached to the group it introduces. A note about one specific dependency is indistinguishable from such a header, so every move is reported on stderr: check that carried text still describes the entry it -landed on. Comments that cannot be placed – nothing survives, or the last -survivor is a dotted key or sub-table with no value to append to – are -reported as dropped. +landed on. Comments that cannot be placed – the removal emptied the table, +or left a trailing survivor with nothing to append to – are reported as +dropped. ## Installation diff --git a/crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md b/crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md index 2d293ddc..c23b8086 100644 --- a/crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md +++ b/crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md @@ -184,23 +184,29 @@ points at instead of replacing the link with a regular file. #### Carried comments can be misattributed A group header and a note about one specific dependency are the same thing to the -parser — comment lines in an entry's decor, whether that decor sits on the key or, for -a `[workspace.dependencies.name]` entry, on the table. When the noted entry is the one -removed, its note lands on the next surviving entry and reads as if it were written -about that one, which is worse than dropping it: a dropped comment shows up in the -`--fix` diff, a wrong attribution outlives it. +parser — comment lines in an entry's decor. Where that decor lives depends on how the +entry is written: on the key for a plain value, on the table for a +`[workspace.dependencies.name]` sub-table, and on the first inner key for a dotted +`name.version = "1"`. Comments are read from, and written to, the same slot; reading +one slot and writing another would render both and put text in the manifest that +nobody wrote. + +When the noted entry is the one removed, its note lands on the next surviving entry +and reads as if it were written about that one, which is worse than dropping it: a +dropped comment shows up in the `--fix` diff, a wrong attribution outlives it. The carry-forward still earns its keep for headers, so it stays, and the relocation is made visible instead: every move is reported on stderr, naming the entries the comments came from and the entry they landed on, so whoever reviews the diff knows which lines to check. -Comments cannot always be placed. Only a plain value has a suffix to append to, so when -the removed entries are last in the table and the final survivor is a dotted key or a -sub-table — and when every entry is removed and nothing survives — the comments go with -the group they introduced, reported as a drop. The report describes what happened -rather than what was attempted: claiming a move that did not happen would send the -reviewer hunting for text that is not in the diff. +Comments cannot always be placed. When the removed entries are last in the table the +carried text has to go *after* the final survivor rather than ahead of it, and only a +plain value has a suffix to append to — so a sub-table survivor there, and an emptied +table with no survivor at all, lose the comments with the group they introduced. That +is reported as a drop. The report describes what happened rather than what was +attempted: claiming a move that did not happen would send the reviewer hunting for +text that is not in the diff. ## 6. Relationship to the other dependency checks diff --git a/crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs b/crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs index 00fb8ad3..82bee442 100644 --- a/crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs +++ b/crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs @@ -28,8 +28,9 @@ pub struct Carry { /// Entries whose comments were carried, in manifest order. pub from: Vec, - /// The entry the comments landed on, or `None` when the table was emptied - /// and they were dropped. + /// The entry the comments landed on, or `None` when they could not be + /// placed and were dropped: nothing survived the removal, or the only + /// surviving anchor was a value the comments cannot attach to. pub onto: Option, /// How many comment lines moved. @@ -59,10 +60,8 @@ pub fn remove(manifest: &mut DocumentMut, names: &[String]) -> Outcome { let mut sources: Vec = Vec::new(); for name in &order { - let prefix = decor_prefix(table, name); - if doomed.contains(name.as_str()) { - let comments = comments_of(&prefix); + let comments = comments_of(&leading_comments(table, name)); if !comments.is_empty() { sources.push(name.clone()); } @@ -74,10 +73,7 @@ pub fn remove(manifest: &mut DocumentMut, names: &[String]) -> Outcome { // move that did not happen sends the reviewer of the `--fix` diff // hunting for text that is not there, which is the failure this // reporting exists to prevent. - let onto = table.key_mut(name).map(|mut key| { - key.leaf_decor_mut().set_prefix(format!("{carried}{prefix}")); - name.clone() - }); + let onto = prepend_comments(table, name, &carried); outcome.carries.push(Carry { from: std::mem::take(&mut sources), @@ -90,14 +86,14 @@ pub fn remove(manifest: &mut DocumentMut, names: &[String]) -> Outcome { if !carried.is_empty() { // The removed entries were the last in the table, so there is no - // following key to carry the comments to. Append them after the final - // surviving entry's *value* instead: attaching them to that entry's key - // prefix would hoist them above it and relabel a surviving dependency. + // following entry to carry the comments to. Append them after the final + // surviving entry's *value* instead: attaching them ahead of that entry + // would hoist them above it and relabel a surviving dependency. // // Only a plain value has a suffix to append to. When the last survivor - // is a dotted key or a sub-table, and when every entry was removed and - // there is no survivor at all, the comments go with the group they - // introduced -- reported as a drop, because that is what happened. + // is a sub-table, and when every entry was removed and nothing survives + // at all, the comments go with the group they introduced -- reported as + // a drop, because that is what happened. let last = table.iter().last().map(|(key, _)| key.to_owned()); let onto = last.and_then(|last| { let appended = table.get_mut(&last).and_then(Item::as_value_mut).map(|value| { @@ -123,26 +119,82 @@ pub fn remove(manifest: &mut DocumentMut, names: &[String]) -> Outcome { outcome } -/// The decor preceding one entry, from wherever `toml_edit` keeps it. +/// The name of the inner key that renders first inside a dotted entry. /// -/// A plain entry carries its comments on the key. A sub-table entry -- -/// `[workspace.dependencies.name]` -- carries them on the table instead, and -/// reading only the key would let those comments disappear unremarked. -fn decor_prefix(table: &dyn TableLike, name: &str) -> String { - let on_table = table - .get(name) - .and_then(Item::as_table) - .and_then(|nested| nested.decor().prefix()) - .and_then(toml_edit::RawString::as_str) - .unwrap_or_default(); +/// `dep.version = "1"` is a dotted table whose leading comments hang off the +/// inner `version` key, not off `dep`. +fn dotted_leaf(table: &dyn TableLike, name: &str) -> Option { + let nested = table.get(name).and_then(Item::as_table).filter(|nested| nested.is_dotted())?; + + nested.iter().next().map(|(inner, _)| inner.to_owned()) +} - let on_key = table +/// The decor preceding one entry, read from whichever slot renders it. +/// +/// `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 on the first inner key for a dotted `name.version = "1"`. +/// Reading only the key lets the other two disappear unremarked. +fn leading_comments(table: &dyn TableLike, name: &str) -> String { + if let Some(leaf) = dotted_leaf(table, name) { + return table + .get(name) + .and_then(Item::as_table_like) + .and_then(|nested| nested.key(&leaf)) + .and_then(|key| key.leaf_decor().prefix()) + .and_then(toml_edit::RawString::as_str) + .unwrap_or_default() + .to_owned(); + } + + if let Some(nested) = table.get(name).and_then(Item::as_table) { + return nested + .decor() + .prefix() + .and_then(toml_edit::RawString::as_str) + .unwrap_or_default() + .to_owned(); + } + + table .key(name) .and_then(|key| key.leaf_decor().prefix()) .and_then(toml_edit::RawString::as_str) - .unwrap_or_default(); + .unwrap_or_default() + .to_owned() +} + +/// Prepend `carried` to an entry's leading comments, in the same slot +/// [`leading_comments`] reads from. +/// +/// Writing to a different slot than the one that holds the entry's own decor +/// would render both, duplicating text the user never wrote. Returns the entry +/// name when the comments were placed, and `None` when the entry offers no such +/// slot, so the caller can report a drop instead of an imagined move. +fn prepend_comments(table: &mut dyn TableLike, name: &str, carried: &str) -> Option { + let existing = leading_comments(table, name); + let combined = format!("{carried}{existing}"); + + if let Some(leaf) = dotted_leaf(table, name) { + return table + .get_mut(name) + .and_then(Item::as_table_like_mut) + .and_then(|nested| nested.key_mut(&leaf)) + .map(|mut key| { + key.leaf_decor_mut().set_prefix(combined); + name.to_owned() + }); + } + + if let Some(nested) = table.get_mut(name).and_then(Item::as_table_mut) { + nested.decor_mut().set_prefix(combined); + return Some(name.to_owned()); + } - format!("{on_table}{on_key}") + table.key_mut(name).map(|mut key| { + key.leaf_decor_mut().set_prefix(combined); + name.to_owned() + }) } /// The comment-bearing part of a removed entry's decor. diff --git a/crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs b/crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs index fb381cfc..540a53df 100644 --- a/crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs +++ b/crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs @@ -66,9 +66,9 @@ //! keeps a group header attached to the group it introduces. A note about one //! specific dependency is indistinguishable from such a header, so every move //! is reported on stderr: check that carried text still describes the entry it -//! landed on. Comments that cannot be placed -- nothing survives, or the last -//! survivor is a dotted key or sub-table with no value to append to -- are -//! reported as dropped. +//! landed on. Comments that cannot be placed -- the removal emptied the table, +//! or left a trailing survivor with nothing to append to -- are reported as +//! dropped. //! //! # Installation //! diff --git a/crates/cargo-ensure-no-unused-workspace-deps/tests/integration_tests.rs b/crates/cargo-ensure-no-unused-workspace-deps/tests/integration_tests.rs index 040bc450..95e28407 100644 --- a/crates/cargo-ensure-no-unused-workspace-deps/tests/integration_tests.rs +++ b/crates/cargo-ensure-no-unused-workspace-deps/tests/integration_tests.rs @@ -485,6 +485,102 @@ fn fix_reports_a_drop_when_the_last_survivor_cannot_carry_comments() { assert!(!stderr.contains("Carried"), "nothing was carried here: {stderr}"); } +#[test] +fn fix_does_not_duplicate_a_sub_table_survivors_own_comment() { + // The survivor's own comment lives on its table decor. Reading it there and + // writing it back onto the key would leave both in the document, so `--fix` + // would emit a line the user never wrote. + let root = concat!( + "[workspace]\nmembers = [\"member\"]\n\n", + "[workspace.dependencies]\n", + "# --- group header ---\n", + "once_cell = \"1\"\n\n", + "# note about serde\n", + "[workspace.dependencies.serde]\nversion = \"1\"\n", + ); + let dir = workspace(root, &[("member", "[dependencies]\nserde = { workspace = true }\n")]); + let manifest = dir.path().join("Cargo.toml"); + + let (success, _, stderr) = outcome(&run(&manifest, &["--fix"])); + + assert!(success, "--fix should succeed: {stderr}"); + + let fixed = fs::read_to_string(&manifest).expect("failed to read the fixed manifest"); + assert_eq!( + fixed.matches("# note about serde").count(), + 1, + "the survivor's own comment must appear exactly once: {fixed}" + ); + assert!(fixed.contains("# --- group header ---"), "the carried header survives: {fixed}"); + assert!( + stderr.contains("Carried 1 comment line from 'once_cell' onto 'serde'"), + "the move must be reported: {stderr}" + ); +} + +#[test] +fn fix_carries_onto_a_dotted_survivor() { + // A dotted entry keeps its leading comments on its first inner key, so that + // is where a carry has to land. Writing to the outer key renders nothing + // and would lose the text while still reporting a move. + let root = concat!( + "[workspace]\nmembers = [\"member\"]\n\n", + "[workspace.dependencies]\n", + "# pinned to 1.2 until upstream #42 is fixed\n", + "old = \"1\"\n", + "serde.version = \"1\"\n", + "zzz = \"1\"\n", + ); + let dir = workspace( + root, + &[( + "member", + "[dependencies]\nserde = { workspace = true }\nzzz = { workspace = true }\n", + )], + ); + let manifest = dir.path().join("Cargo.toml"); + + let (success, _, stderr) = outcome(&run(&manifest, &["--fix"])); + + assert!(success, "--fix should succeed: {stderr}"); + + let fixed = fs::read_to_string(&manifest).expect("failed to read the fixed manifest"); + let comment = fixed.find("# pinned to 1.2").expect("the carried comment survives"); + let survivor = fixed.find("serde.version").expect("the dotted survivor is kept"); + assert!(comment < survivor, "the comment introduces the entry it landed on: {fixed}"); + assert!( + stderr.contains("Carried 1 comment line from 'old' onto 'serde'"), + "the move must be reported: {stderr}" + ); +} + +#[test] +fn fix_carries_a_comment_from_a_removed_dotted_entry() { + // The removed entry is dotted, so its comment hangs off the inner key. + // Reading only the outer key or the table decor would drop it unreported. + let root = concat!( + "[workspace]\nmembers = [\"member\"]\n\n", + "[workspace.dependencies]\n", + "# note about old\n", + "old.version = \"1\"\n", + "serde = \"1\"\n", + ); + let dir = workspace(root, &[("member", "[dependencies]\nserde = { workspace = true }\n")]); + let manifest = dir.path().join("Cargo.toml"); + + let (success, _, stderr) = outcome(&run(&manifest, &["--fix"])); + + assert!(success, "--fix should succeed: {stderr}"); + + let fixed = fs::read_to_string(&manifest).expect("failed to read the fixed manifest"); + assert!(!fixed.contains("old.version"), "the unused entry should be gone: {fixed}"); + assert!(fixed.contains("# note about old"), "its comment must not vanish: {fixed}"); + assert!( + stderr.contains("Carried 1 comment line from 'old' onto 'serde'"), + "the move must be reported: {stderr}" + ); +} + #[test] fn fix_carries_a_comment_from_a_removed_sub_table_entry() { // A `[workspace.dependencies.name]` entry keeps its comments on the table, From 5656b50db04b6b4de3eb1e249a46043b65f5cf3b Mon Sep 17 00:00:00 2001 From: "Martin Kolinek (from Dev Box)" Date: Fri, 28 Aug 2026 13:05:00 +0200 Subject: [PATCH 10/11] docs: keep dotted survivors in the trailing-drop description MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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> --- .../docs/design/README.md | 11 ++++++----- .../cargo-ensure-no-unused-workspace-deps/src/fix.rs | 9 ++++++--- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md b/crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md index c23b8086..af7ac79a 100644 --- a/crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md +++ b/crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md @@ -202,11 +202,12 @@ which lines to check. Comments cannot always be placed. When the removed entries are last in the table the carried text has to go *after* the final survivor rather than ahead of it, and only a -plain value has a suffix to append to — so a sub-table survivor there, and an emptied -table with no survivor at all, lose the comments with the group they introduced. That -is reported as a drop. The report describes what happened rather than what was -attempted: claiming a move that did not happen would send the reviewer hunting for -text that is not in the diff. +plain value has a suffix to append to — so a dotted key or a sub-table survivor there, +and an emptied table with no survivor at all, lose the comments with the group they +introduced. That is reported as a drop. (A dotted survivor elsewhere in the table is +carried onto normally; it is only appending *after* one that has nowhere to go.) The +report describes what happened rather than what was attempted: claiming a move that +did not happen would send the reviewer hunting for text that is not in the diff. ## 6. Relationship to the other dependency checks diff --git a/crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs b/crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs index 82bee442..4fb56428 100644 --- a/crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs +++ b/crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs @@ -91,9 +91,12 @@ pub fn remove(manifest: &mut DocumentMut, names: &[String]) -> Outcome { // would hoist them above it and relabel a surviving dependency. // // Only a plain value has a suffix to append to. When the last survivor - // is a sub-table, and when every entry was removed and nothing survives - // at all, the comments go with the group they introduced -- reported as - // a drop, because that is what happened. + // is a dotted key or a sub-table, and when every entry was removed and + // nothing survives at all, the comments go with the group they + // introduced -- reported as a drop, because that is what happened. + // + // A dotted survivor can be carried *onto* earlier in the loop, where + // the comments go ahead of it; only appending after it is impossible. let last = table.iter().last().map(|(key, _)| key.to_owned()); let onto = last.and_then(|last| { let appended = table.get_mut(&last).and_then(Item::as_value_mut).map(|value| { From b573896ea961e4785348929b6bb5fca264b65bb2 Mon Sep 17 00:00:00 2001 From: "Martin Kolinek (from Dev Box)" Date: Fri, 28 Aug 2026 14:02:24 +0200 Subject: [PATCH 11/11] docs: record the stale allow-list report at the empty-catalog boundary 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> --- .../docs/design/README.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md b/crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md index af7ac79a..ddaeb689 100644 --- a/crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md +++ b/crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md @@ -134,9 +134,11 @@ the file. The success line goes to stdout. | 0 | No unused entries — or, under `--fix`, all unused entries were removed and the manifest written. | | 1 | Unused entries found without `--fix`, or a manifest could not be read, parsed, or enumerated. | -A `[workspace]` table with no `dependencies` catalog is a clean pass — there is -nothing to be stale. A manifest with no `[workspace]` table at all is a pass with a -note on stderr, or an error under `--require-workspace`. +A `[workspace]` table with no `dependencies` catalog is a pass: no entry can be +uninherited when none is declared. It is not silent, though — that is the boundary +where *every* configured `allowed` name suppresses nothing, so each one is reported +as stale on stderr before the run succeeds. A manifest with no `[workspace]` table at +all is a pass with a note on stderr, or an error under `--require-workspace`. The exit code is returned from `run` as an `ExitCode` rather than raised with `std::process::exit`, so `main` unwinds normally. That matters under coverage