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/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/Cargo.lock b/Cargo.lock index 1f04e50b..3fef2265 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -442,6 +442,17 @@ dependencies = [ "toml", ] +[[package]] +name = "cargo-ensure-no-unused-workspace-deps" +version = "0.1.0" +dependencies = [ + "anyhow", + "cargo_metadata", + "clap", + "tempfile", + "toml_edit", +] + [[package]] name = "cargo-heather" version = "0.3.0" 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 new file mode 100644 index 00000000..61467fd5 --- /dev/null +++ b/crates/cargo-ensure-no-unused-workspace-deps/Cargo.toml @@ -0,0 +1,41 @@ +# 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.1.0" +readme = "README.md" +keywords = ["oxidizer", "cargo", "subcommand", "dependencies", "ci"] +categories = ["command-line-utilities", "development-tools::cargo-plugins"] + +edition.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" + +[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"] } +tempfile = { workspace = true } +toml_edit = { workspace = true } + +# >>> anvil-managed: anvil-lints +[lints] +workspace = true +# <<< anvil-managed: anvil-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..bc1c51ac --- /dev/null +++ b/crates/cargo-ensure-no-unused-workspace-deps/README.md @@ -0,0 +1,101 @@ +
+ 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. + +## Fixing + +`--fix` replaces the manifest atomically – a temporary file in the same +directory, renamed over the original, carrying the permissions of the +manifest it replaces and following a symlinked manifest to its target – and +refuses to write at all if the file changed after it was read, so a +concurrent edit is never clobbered. + +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 – the removal emptied the table, +or left a trailing survivor with nothing to append to – are reported as +dropped. + +## 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 new file mode 100644 index 00000000..ddaeb689 --- /dev/null +++ b/crates/cargo-ensure-no-unused-workspace-deps/docs/design/README.md @@ -0,0 +1,256 @@ +# cargo-ensure-no-unused-workspace-deps — Design + +> Status: **Adopted**. +> 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] [--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. | +| `--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 + +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 `[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 +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. + +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. + +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. 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. 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 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 + +| 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 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 +pass without configuration. + +## 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. 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..be18e4fc --- /dev/null +++ b/crates/cargo-ensure-no-unused-workspace-deps/src/detect.rs @@ -0,0 +1,156 @@ +// 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 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 { + 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..4fb56428 --- /dev/null +++ b/crates/cargo-ensure-no-unused-workspace-deps/src/fix.rs @@ -0,0 +1,218 @@ +// 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, TableLike}; + +/// 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 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. + 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]) -> Outcome { + 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 outcome = Outcome { + removed: 0, + carries: Vec::new(), + }; + let mut carried = String::new(); + let mut sources: Vec = Vec::new(); + + for name in &order { + if doomed.contains(name.as_str()) { + let comments = comments_of(&leading_comments(table, name)); + if !comments.is_empty() { + sources.push(name.clone()); + } + carried.push_str(&comments); + table.remove(name); + outcome.removed += 1; + } else if !carried.is_empty() { + // `onto` reports where the comments actually landed. Claiming a + // 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 = prepend_comments(table, name, &carried); + + outcome.carries.push(Carry { + from: std::mem::take(&mut sources), + onto, + lines: comment_lines(&carried), + }); + carried.clear(); + } + } + + if !carried.is_empty() { + // The removed entries were the last in the table, so there is no + // 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 + // 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| { + 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, + lines: comment_lines(&carried), + }); + } + + outcome +} + +/// The name of the inner key that renders first inside a dotted entry. +/// +/// `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()) +} + +/// 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() + .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()); + } + + 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. +/// +/// 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() + } +} + +/// 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 new file mode 100644 index 00000000..540a53df --- /dev/null +++ b/crates/cargo-ensure-no-unused-workspace-deps/src/lib.rs @@ -0,0 +1,427 @@ +// 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. +#![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. +//! +//! 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. +//! +//! # Fixing +//! +//! `--fix` replaces the manifest atomically -- a temporary file in the same +//! directory, renamed over the original, carrying the permissions of the +//! manifest it replaces and following a symlinked manifest to its target -- and +//! refuses to write at all if the file changed after it was read, so a +//! concurrent edit is never clobbered. +//! +//! 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 -- the removal emptied the table, +//! or left a trailing survivor with nothing to append to -- are reported as +//! dropped. +//! +//! # 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::io::Write; +use std::path::{Path, PathBuf}; +use std::process::ExitCode; + +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)) + .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 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, + 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() { + // An empty catalog is the boundary where *every* allowed name + // suppresses nothing, so the stale report is due here too. + let (_, stale) = detect::partition(&catalog, &BTreeSet::new()); + report_stale(&stale); + + 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 outcome = fix::remove(&mut manifest, &unused); + write_back(manifest_path, &original, &manifest.to_string())?; + + println!( + "🧹 Removed {} unused workspace {} from {}.", + outcome.removed, + entries(outcome.removed), + manifest_path.display() + ); + report_carries(&outcome.carries); + + Ok(ExitCode::SUCCESS) +} + +/// Replace `manifest_path` with `contents`, atomically and only if the file +/// still holds what was read. +/// +/// The workspace root manifest 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 same directory and renamed over the +/// original, which is atomic on one filesystem. `cargo metadata` runs between +/// the read and the write, and it is a child process, so that window is wide +/// enough for an editor to save into it -- hence the unchanged-input guard. +/// +/// 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: +/// +/// - **Permissions.** A temporary file is created owner-only, and a rename +/// carries its mode rather than inheriting the target's, so the manifest's +/// own permissions are read first and applied to the replacement. Without +/// that, a world-readable manifest silently comes back owner-only, which git +/// does not track and the next differently-owned reader discovers the hard +/// way. +/// - **Symlinks.** A symlinked manifest is resolved first, so the rename lands +/// on the file the link points at and the indirection survives. Replacing the +/// link itself would quietly turn it into a regular file. +fn write_back(manifest_path: &Path, original: &str, contents: &str) -> 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()); + + // 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 permissions = std::fs::metadata(&target).context(metadata_failure)?.permissions(); + let directory = target + .parent() + .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.as_file().set_permissions(permissions).context(permissions_failure)?; + staged.persist(&target).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 +/// 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 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}': no surviving entry could carry them.", + 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(); + 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" } +} + +/// Pluralize `line` for `count`. +fn lines(count: usize) -> &'static str { + if count == 1 { "line" } else { "lines" } +} + +#[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; + + 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/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..95e28407 --- /dev/null +++ b/crates/cargo-ensure-no-unused-workspace-deps/tests/integration_tests.rs @@ -0,0 +1,686 @@ +// 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"); + 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!( + 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] +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}" + ); + + // 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] +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 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': 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_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, + // 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"); + 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}" + ); +}