diff --git a/.gitignore b/.gitignore index b4ac787d7..3f04de14f 100644 --- a/.gitignore +++ b/.gitignore @@ -9,6 +9,7 @@ memories/ vtcode.toml .memdb/ .grepai/ +.netsuke/ build.ninja *:Zone.Identifier /graph.dot @@ -18,6 +19,7 @@ __pycache__/ .uv-cache/ .uv-tools/ .pytest_cache/ +.ruff_cache/ .typos-oxendict-base.json .typos-oxendict-base.toml *.swo diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 6b35e0fdf..54e24acc8 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -4417,6 +4417,286 @@ Three dispositions are in use: Scope an expectation as tightly as the site allows — a function where one call is involved, a module only where the whole file is pending migration. +#### The suppression contract that backs the rule + +`expect`-not-`allow` is a convention the compiler enforces only on the outer +form. `clippy::allow_attributes` does not fire on an *inner* attribute, so + +```rust +#![allow(clippy::disallowed_methods, reason = "escape hatch probe")] +``` + +at the top of a file switches the environment-access policy off for everything +below it and passes `make lint` with every other contract green: `clippy.toml` +still lists the methods, the workspace still denies the lint, and the lint +target still runs across the workspace. Each of those asserts a true statement +about a different thing, and none observes that a source has opted out. + +`tests/env_access_suppressions.rs` closes that gap. It reads the compiled +sources — `src`, `build_l10n_audit`, `test_support/src`, `tests`, `benches`, +`examples`, and `build.rs` — and fails when an `#[allow(...)]` or +`#![allow(...)]` attribute names a lint that carries the policy. The roots are +the ones the workspace lints rather than the ones a convention calls source. +`tests`, `benches`, and `examples` are in scope because Cargo discovers targets +in all three and `--all-targets` compiles and lints them, and the modules they +wire in, exactly as it lints the library, so an inner attribute there silences +the policy for a whole test, benchmark, or example binary just the same. + +Within a root the scan reads every file that is not dot-prefixed, rather than +only the `*.rs` files, because a `.rs` name is not something the compiler +requires. A module is read from whatever `#[path = "..."]` names — +`#[path = "suppressed.inc"] mod suppressed;` compiles — and such a file may +open with the innermost form of the policy suppression, which +`allow_attributes` does not report. Naming the compiled sources by an extension +the language does not require is therefore a filter the one reader who cares +about it would rather have on. The breadth errs towards reading too much on +purpose: a file read but never compiled costs a failure message naming a real +file, while a file compiled but not read hides a suppression. A file that will +not decode is declined rather than fatal, since Rust source is UTF-8 by +definition; every other read error still propagates, and that half is the +load-bearing one, because a file that is text and could not be read is a source +that went unscanned. A dot-file under a source root stays out, being tooling +state — `.gitignore`, `.editorconfig`, `.rustfmt.toml` — rather than anything a +`#[path]` names. + +The root list is not trusted to stay complete on its own, because that is how a +scan silently stops covering something. A second test walks every Rust source +in the workspace, skipping only the named machine-local directories — `target`, +the tool caches, and the other entries `.gitignore` declares — and fails when +one of them is not in the scanned roots, naming each. So a root that is renamed +or misspelled, or a target location added later, reports itself instead of +quietly excusing its sources. Extending the roots stays safe: the invariant is +what says the set is complete, rather than a reviewer re-deriving it. + +A root is a directory, and every directory name beneath it comes along, so a +cache can sit inside a scanned root: `tests/.uv-cache` is under `tests`. The +scan therefore skips the same machine-local names the workspace walk skips, at +any depth, rather than reading a cache as though it were repository content. It +did not always, and the gap was worth closing for a reason other than tidiness: +a vendored source under a scanned root that carried the banned `allow` failed +the gate on a machine where the tool had run and passed on a fresh clone. A +verdict that depends on a machine is worse than no verdict, and this one would +have been near-impossible to diagnose, because the name is git-ignored and so +appears in no diff and in no `git status`. Both walks skip by name now, and a +self-test pins that they agree on what is governed, since the coverage +invariant only means something while they do. + +The skip list is named rather than "anything dot-prefixed", and the difference +matters. A dot-directory is not evidence of a cache: `.config`, `.github`, and +`.rules` are tracked repository content, and Cargo compiles a target declared +under any directory at all, hidden or not. A walk that skipped every +dot-prefixed name would neither scan a target sitting in one nor report it, +which is exactly the silent non-coverage the invariant exists to prevent. Where +the two rules could disagree, the tie breaks towards reporting: an entry +missing from the list costs a false failure naming a real file, while an entry +present but wrong hides a source. + +The skip is justified by an appeal to `.gitignore` — a name git will not track +is not one a compiled source can live under — and that appeal is enforced +rather than trusted, because it stopped being true once. `.netsuke` is +netsuke's own runtime state, but it was skipped while `git check-ignore` +declined it; every sibling tool cache is listed and it had been missed. A `.rs` +file placed there would have been tracked, compiled, skipped by the walk, and +reported by nobody, which is the failure the invariant exists to catch arriving +through the list rather than the walk. The name is now in `.gitignore`, and the +walk's self-test requires each skipped name to be one git would not track. +`.git` is the single named exception: git refuses to track anything beneath it +whatever the ignore files say. + +That self-test asks the *repository's* rules rather than the working tree, +because a working tree answers with more than those. Ruff writes a `.gitignore` +holding `*` into `.ruff_cache` as a side effect of running, so +`git check-ignore` in the live tree agreed that `.ruff_cache` was ignored while +the repository's own `.gitignore` said nothing about it — every sibling cache +has an entry and this one had been missed, the same defect as `.netsuke` +arriving one level down. A fresh clone, or a `coverage-main` lane that runs +`make test` without `make lint` first, has no such file, so the answer would +have depended on which tools had already run. The test therefore copies +`.gitignore` into a scratch repository and puts the question there: the rule +has to hold on every checkout, before any tool runs, and `.ruff_cache` is now in +`.gitignore` beside its siblings. Asking its own repository also means the +test needs no guard for the copies cargo-mutants makes, since it brings one. + +The machine's git configuration is a third place an answer can come from, and +it is switched off for the same reason. A global ignore file naming one of +these directories would make the test pass while the repository said nothing +about the name — the original defect wearing a different hat, and just as +invisible. An empty `core.excludesFile` covers both spellings a global ignore +can take: it overrides a configured path and also suppresses the default +`~/.config/git/ignore`, measured against both. The value is empty rather than a +device path, since the test also runs on the Windows lane. + +A template directory is a fourth, and it is closed at `git init` instead: +`--template=` keeps a template from seeding the scratch repository's +`info/exclude`, which `check-ignore` would otherwise read. That is the whole of +its job, and it is worth stating narrowly: a template can seed `.git/config` +too, but the empty `core.excludesFile` above already neutralizes an ignore file +configured there, measured with a template seeding only that. The flag has to +be on `git init` rather than on the `check-ignore` call, because `info/exclude` +is written at init time and no later call could unpin it. Note that +`-c init.templateDir=` on the same command does *not* close it: +`GIT_TEMPLATE_DIR` outranks it, measured, so the empty `--template` argument is +the form that works for a contributor with that variable set. + +A fifth route does not go through a file at all: `GIT_DIR` repoints git at +another repository's metadata, so `check-ignore` answers from there. Measured +at a false pass, with the hostile repository's `info/exclude` holding `*` while +the honest answer for an unignored name was "not ignored". Both `git` calls +clear `GIT_DIR`, `GIT_WORK_TREE`, and `GIT_COMMON_DIR`. This one needed a +mutation to verify rather than a green suite, because a false pass is also a +pass: a name absent from `.gitignore` is added to the skip list, and the test +must fail naming it even under that environment, which it does only with the +pin in place. + +It reads the attribute as source text, because that is what an attribute is: +there is no execution to model, and the assertion is exactly "this text does +not appear in an `allow` attribute". An attribute nested in a `cfg_attr` is +read too, since that is the same suppression written one token differently. The +scan first blanks comments and string and character literals, because that is +where quoted text lives — a byte or C string escapes like any other, so its +body ends at an unescaped quote, and reading one as raw would end it early at +an escaped quote and blank the code after it. Masking is what keeps the scan +off prose that quotes an attribute, including this section and the mutation +records that quote the form they prohibit: a quoted attribute never reaches the +matcher at all, whatever line it sits on. It then matches an attribute by its +tokens — `#`, an optional `!`, `[`, a name, `(` — with whitespace permitted +between them, and reads it to its matching parenthesis, so one that `rustfmt` +has wrapped across several lines is read whole rather than truncated. The +scanner lives beside the contract in `tests/env_access_suppressions/` +(`scanner.rs`, `mask.rs`, `policy.rs`), and its self-tests in +`scanner_tests.rs` and `spelling_tests.rs` pin each shape it must report and +each innocent source it must not. + +Matching tokens rather than lines is the one design decision here that was +reached the hard way, and it is worth recording why the obvious alternative +fails. The scan once anchored at the start of a line, reasoning that `rustfmt` +normalizes an attribute's spelling and `make check-fmt` enforces that, so a +spelling the anchor declined to read could not reach the compiler. **That +reasoning is false.** Each of these compiles, silences the policy outright +(`clippy` exits 0 where the same file without the attribute exits 101), and +passed the anchored scan, and every one is pinned by a test in +`spelling_tests.rs` against a real probe file: + +- `#[rustfmt::skip]` freezing a split marker: `#[allow` with its `(` on a later + line, a newline between `#[allow(` and the lint list, a newline between the + `#` and the `[`, or spaces around the `::` of the path. `rustfmt` would + normally join or normalize all of these, which is the premise that failed — + but a skip attribute is a request to be shown nothing, so the gate passes a + spelling it never inspected. +- `r#allow(...)` and `r#clippy::disallowed_methods`, raw identifiers denoting + exactly what the unprefixed names denote. These need no skip attribute at all: + `rustfmt` leaves them byte-for-byte as written, so they were reachable on a + clean `make check-fmt` run and are the more dangerous of the two groups. +- the deprecated bare `disallowed_methods` beside its enabler, likewise + untouched by `rustfmt`. + +A layout gate is not a proof about spelling — it normalizes what it is shown, +and it is not shown what a skip attribute covers — so the matcher tolerates +whitespace between tokens and reads the raw prefix instead of trusting a gate +to have removed them. + +The banned set names lints rather than spelling one form, and it follows the +lint hierarchy where the hierarchy applies. `disallowed_methods` is declared in +Clippy's `style` group, so allowing that group silences the policy just as +naming the lint does, and `clippy::all` sits above it; both were measured at +exit 0 under the gate's own flags. `warnings` is banned as well, but not +because it sits above them — it does not. The `warnings` group is the set of +lints *currently at* `warn`, and Cargo passes `[workspace.lints]` as +command-line denies, so the policy lint is at `deny` and outside the group: +`#![allow(warnings)]` alone leaves it firing, measured at exit 101 bare and +gated. It stays in the set because it silences every warn-level lint under the +gate, because it silences `unfulfilled_lint_expectations` — the self-removal +mechanism `clippy.toml` relies on when it says the backlog "removes itself +instead of rotting" — and because it is half of the only measured way past the +gate's flags. The two guard lints are included because silencing the reporter +is the one suppression nothing else would report. + +That second job sets the membership rule, and it is not "can this reach the +policy lint". `clippy::restriction` is in the set although it cannot reach the +policy lint at all — `disallowed_methods` sits in `all` and `style`, and +allowing `restriction` leaves the policy lint firing at exit 101. It is in the +set because it is the *group* of both guard lints, so a single crate-level +`#![allow(clippy::restriction, reason = "…")]` silences them together and an +item-level bare `allow` further down then passes unreported: measured at exit 0 +where the same file without the crate attribute exits 101. That is the escape +hatch this module exists to close, since `allow_attributes` does not fire on +the inner form, and `blanket_clippy_restriction_lints` — denied in the +workspace — does not cover the attribute route either, firing only on a +group-level `-W clippy::restriction` and reporting nothing for an attribute +naming the group. So the rule is: a name is banned when a measurement shows it +silencing something this module protects, and that is decided per name. + +A `warn` attribute is deliberately *not* matched, and the reason is measured +rather than assumed, because it is the obvious next question. A `warn` of the +policy lint does lower it — bare `cargo clippy` exits 0 where the same file +exits 101 — so it is a real suppression and not a no-op. It is not a *silent* +one: every lint and test target passes `-D warnings`, which re-promotes the +lint to an error and exits 101. Twelve spellings were probed (inner and outer, +`cfg_attr`-wrapped, group and alias names, the guard-lint forms) and every one +is caught by the gate while the bare run silences the same file. Reporting a +shape that cannot pass a gate would be a rule the code cannot justify, which is +the reasoning that also leaves `unknown_lints` out. + +The one exception is the shape that pairs the two, and it is worth stating +because it is what makes the `warnings` entry load-bearing rather than +decorative. `#![warn(clippy::disallowed_methods)]` lowers the policy lint to +`warn`, which is precisely what puts it *into* the `warnings` group; +`#![allow(warnings)]` then suppresses that group. Measured at exit 0 under +`RUSTFLAGS=-D warnings`, in either order, where neither half escapes alone. The +scan catches it on the `allow` half, because that is the only half it matches. +Should the lint target ever stop passing `-D warnings`, re-measure the `warn` +family before trusting this reasoning: the re-promotion is the only thing +holding that side of the pair. + +The set also bans a second route in, which is worth stating because it is not +obvious. Clippy keeps the old spelling of a renamed lint, and a renamed name +still selects the lint it was renamed to, so `clippy::disallowed_method` — the +alias of the policy lint — silences the policy exactly as the current name +does. On its own that is harmless: `renamed_and_removed_lints` is denied in +`[workspace.lints.rust]`, so the rename is reported and the alias is an error +rather than a suppression. Allow that lint as well and the rename goes +unreported, and the alias suppresses the policy in silence — measured at exit +0, where the same file without the attribute exits 101. The deprecated bare +`disallowed_methods` is the same alias under its shorter name, and measurement +says it behaves identically: it silences the policy beside the enabler, and +errors without it. Three entries close the class: `renamed_and_removed_lints`, +because no alias suppresses anything while the rename naming it is still +reported, plus `clippy::disallowed_method` and the bare `disallowed_methods`, +so the pair stays honest if a future Clippy stops reporting renames. + +`unknown_lints` is deliberately *not* banned, though it looks as if it should +be: it hides the report that a name does not exist, which reads like the rename +mechanism above. It was measured and it is not one. A misspelled lint name is a +no-op whether or not the report is allowed, so suppressing `unknown_lints` +cannot silence the policy — `#![allow(unknown_lints, disallowed_methods)]` +without the rename enabler still exits 101 — and banning a name that cannot +suppress anything would be a rule the code cannot justify. The workspace denies +`unknown_lints` anyway, which is where that concern belongs. The lesson is the +one this section keeps relearning: measure the mechanism before writing the +rule, and do not add a name because it looks like it belongs. + +The measurement has to be applied to the name and not to the category, or the +rule cuts the other way and takes out entries it should keep. "Cannot suppress +anything" is the test — not "cannot suppress the policy lint", which would also +excuse the two guard lints and `clippy::restriction`, all three of which +silence something. Reading the rule as the second form is what kept +`restriction` out of the set for a round of review, and it would have been a +real hole: it is the group of the guard lints, and nothing else reports a crate +that has silenced the reporter. + +Three files are exempt, and only for those two guard lints: +`src/runner/error.rs`, `src/manifest/diagnostics/mod.rs`, and +`src/manifest/diagnostics/yaml.rs`. Each isolates `thiserror`/`miette` derive +expansions where `unused_assignments` fires on some Rust versions and not +others. `#[expect]` fails when the lint does not fire and +`unfulfilled_lint_expectations` cannot itself be expected, so the module must +carry an `allow` — which the guard lints then reject, leaving the module no way +to state the suppression they require it to state. The exemption is scoped to +those lints on those paths: an `allow` of `clippy::disallowed_methods`, +`clippy::style`, or `warnings` is a finding there too. Remove an entry from +`SCOPED_ALLOWLIST` when its workaround goes, or the exemption outlives its +reason. See . + ### `LocaleLocalizer` `test_support::localizer::locale_localizer` installs a test locale under diff --git a/tests/env_access_suppressions.rs b/tests/env_access_suppressions.rs new file mode 100644 index 000000000..019b44382 --- /dev/null +++ b/tests/env_access_suppressions.rs @@ -0,0 +1,259 @@ +//! Contract test: no compiled source suppresses the environment-access policy. +//! +//! `clippy.toml` disallows the process-environment entry points, and the +//! workspace denies `clippy::disallowed_methods`, but the two together do not +//! close the door. An inner attribute — an `allow` of the policy lint, at the +//! top of a file — switches the lint off for everything below it and passes +//! `make lint` with every other contract green: `clippy.toml` still lists the +//! methods, the workspace still denies the lint, and the lint target still runs +//! across the workspace. None of them observes that a source opted out, because +//! each asserts a true statement about something else. A green tick means the +//! gate ran, not that it was allowed to see anything. +//! +//! Clippy cannot close this itself: `clippy::allow_attributes` does not fire on +//! inner attributes, so the seam taxonomy's "an `#[expect]` carrying a reason, +//! never an `allow`" rule has no mechanical enforcement there. The assertion is +//! therefore about source text, which is normally the wrong shape for a +//! contract — but an attribute *is* source text, there is no execution to +//! model, and "this file does not opt out" is exactly a statement about what +//! the file says. +//! +//! [`scanner`] finds the attributes, [`mask`] keeps quoted text out of its way, +//! and [`policy`] decides which names they may carry; [`roots`] decides which +//! sources are read at all, and this crate holds the assertions over both. +//! +//! See `docs/adr-008-environment-seam-taxonomy.md` for the taxonomy, and the +//! developers' guide for the sanctioned forms and the scoped exemption. + +use anyhow::{Context, Result, ensure}; +use camino::Utf8Path; +use cap_std::{ambient_authority, fs_utf8::Dir}; + +#[path = "env_access_suppressions/mask.rs"] +mod mask; +#[path = "env_access_suppressions/policy.rs"] +mod policy; +#[path = "env_access_suppressions/roots.rs"] +mod roots; +#[path = "env_access_suppressions/scanner.rs"] +mod scanner; + +use roots::{MINIMUM_WORKSPACE_SOURCES, collect_all_sources, compiled_sources, is_scanned}; +use scanner::scan_source; + +/// Render every finding as one failure message, so one run names them all. +/// +/// Collecting before asserting means a contributor sees every offending file in +/// one run rather than fixing them one gate at a time. The message is assembled +/// from a joined list rather than pushed into a growing `String`, because the +/// helper sits outside a `#[test]` body, where neither `expect` nor the +/// `std::fmt::Write` result may be discarded. +/// +/// The findings are sorted first, so the same sources produce the same message. +/// The walk hands them over in the directory's order, which `read_dir` does not +/// promise to keep — filesystem order can change between runs, and between one +/// machine and another, without any source changing. A message that reorders +/// itself makes a genuine failure look like it moved and a fixed one look like +/// it stayed, which is the kind of difference a reader has to spend time on +/// precisely when they are looking at something that has already gone wrong. +/// The coverage invariant sorts its own list for the same reason; see +/// [`every_rust_source_in_the_workspace_is_scanned`]. +fn build_error_message(findings: &[(String, String)]) -> String { + let mut ordered = findings.to_vec(); + ordered.sort(); + let listed = ordered + .iter() + .map(|(path, lint)| format!("{path}: {lint}")) + .collect::>() + .join("\n- "); + format!( + "the environment-access policy is suppressed in compiled sources; \ + remove the `allow` and state the site as `#[expect(..., reason = \"...\")]`, \ + or route the access through a seam:\n- {listed}" + ) +} + +/// Render the `(path, lint)` pairs a table case expects the scan to report. +/// +/// Every finding for one source carries that source's path, so a case states +/// the lints and the path once and the pairs are assembled here. That keeps a +/// row down to the three things that distinguish it — the source, where it +/// sits, and what must be found — rather than repeating the pair shape at every +/// row. +fn expected_findings(path: &str, lints: &[&str]) -> Vec<(String, String)> { + lints + .iter() + .map(|lint| (path.to_owned(), (*lint).to_owned())) + .collect() +} + +/// Fail if any compiled source suppresses the environment-access policy. +#[test] +fn compiled_sources_never_suppress_the_environment_policy() -> Result<()> { + let sources = compiled_sources()?; + // A sweep that silently matched nothing would make the assertion vacuous. + ensure!( + !sources.is_empty(), + "the compiled source roots should contain at least one Rust source" + ); + let findings: Vec<(String, String)> = sources + .iter() + .flat_map(|(path, contents)| scan_source(path, contents)) + .collect(); + + ensure!(findings.is_empty(), "{}", build_error_message(&findings)); + Ok(()) +} + +/// Fail if any Rust source in the workspace falls outside the scanned roots. +/// +/// The scan above can only be as good as the roots it lists, and a root list +/// is exactly the kind of thing that ages badly: Cargo discovers targets in +/// `src/bin`, `examples`, `tests`, and `benches`, a contributor can add a +/// fourth location, and a root that is renamed or misspelled silently excuses +/// its sources rather than reporting itself. So the roots are not trusted on +/// their own. This walk enumerates every Rust source the workspace holds and +/// fails when one of them is not scanned, which turns the silent failure into a +/// named one and makes the root list safe to extend rather than something a +/// reviewer has to keep re-deriving. +/// +/// Caches and `target` are skipped, not because their sources do not matter, +/// but because they are generated or vendored rather than written here, and a +/// gate that read them would depend on what a cache happened to hold. Writes +/// under `target/` are the compiler's, and the `.uv-cache` and friends are +/// tooling state; neither is a place a contributor edits. +#[test] +fn every_rust_source_in_the_workspace_is_scanned() -> Result<()> { + let crate_root = Dir::open_ambient_dir(env!("CARGO_MANIFEST_DIR"), ambient_authority()) + .context("open the workspace root")?; + let mut present = Vec::new(); + collect_all_sources(&crate_root, Utf8Path::new("."), &mut present)?; + + // A walk that silently found nothing would pass while inspecting nothing. + ensure!( + !present.is_empty(), + "the workspace walk should find at least one Rust source" + ); + ensure!( + present.len() >= MINIMUM_WORKSPACE_SOURCES, + "the workspace walk found {} Rust sources, fewer than the {} the workspace \ + holds; the walk is probably not descending", + present.len(), + MINIMUM_WORKSPACE_SOURCES + ); + + let mut unscanned: Vec<&String> = present.iter().filter(|path| !is_scanned(path)).collect(); + unscanned.sort(); + ensure!( + unscanned.is_empty(), + "these Rust sources would not be scanned for policy suppressions; add each \ + source's root to `COMPILED_SOURCE_ROOTS` (or `STANDALONE_COMPILED_SOURCES` \ + if it is a file), so that the suppression contract covers it:\n- {}", + unscanned + .iter() + .map(|path| path.as_str()) + .collect::>() + .join("\n- ") + ); + Ok(()) +} + +/// Fail if the scan reads a different set of sources than the invariant governs. +/// +/// The two tests above are each other's blind spot, and the gap between them is +/// where a whole class of defect hides. `compiled_sources_never_suppress_the_ +/// environment_policy` asserts that the sources it was handed hold no findings, +/// which is true and worthless if it was handed almost nothing; the coverage +/// invariant asserts that no *governed* source is outside the roots, which is a +/// statement about the root list rather than about the walk that reads it. A +/// `collect_rust_sources` that never recursed would leave both green: the scan +/// would find nothing because it read nothing, and the coverage walk is a +/// different function that would still enumerate the tree correctly. +/// +/// So the two are compared here. Every source the invariant calls governed must +/// be one the scan really read, with the contents the walk found, which is what +/// makes "no findings" a statement about the repository rather than about how +/// little was read. Measured against the mutation this exists for — returning +/// early in `collect_rust_sources` instead of descending — the assertion fails +/// naming hundreds of omitted sources where the tests above stay green. +/// +/// The comparison is one-directional, and the direction is the point. The scan +/// reads more than the invariant governs: every non-`.rs` file under a root is +/// read, because the compiler reaches a module through `#[path]` whatever the +/// file is named, while the invariant asks only about the `.rs` files Cargo +/// discovers. Reading extra files cannot hide a suppression in a governed one, +/// so it is the governed set that has to be accounted for. +#[test] +fn the_scan_reads_every_governed_source() -> Result<()> { + let crate_root = Dir::open_ambient_dir(env!("CARGO_MANIFEST_DIR"), ambient_authority()) + .context("open the workspace root")?; + let mut present = Vec::new(); + collect_all_sources(&crate_root, Utf8Path::new("."), &mut present)?; + + let read: std::collections::BTreeMap = + compiled_sources()?.into_iter().collect(); + + let mut omitted: Vec<&String> = present + .iter() + .filter(|path| is_scanned(path) && !read.contains_key(*path)) + .collect(); + omitted.sort(); + // A comparison that silently had nothing to compare would prove nothing, so + // the governed side is required to be the bulk of what the walk found. + let governed = present.iter().filter(|path| is_scanned(path)).count(); + ensure!( + governed >= MINIMUM_WORKSPACE_SOURCES, + "only {governed} of the {} walked sources are governed, fewer than the \ + {MINIMUM_WORKSPACE_SOURCES} the workspace holds; the comparison is vacuous", + present.len() + ); + ensure!( + omitted.is_empty(), + "the coverage invariant governs these sources, but the scan never read \ + them, so a suppression in one would go unreported:\n- {}", + omitted + .iter() + .map(|path| path.as_str()) + .collect::>() + .join("\n- ") + ); + + // Reading a source is not the same as reading *it*: a walk that paired each + // path with the wrong contents would satisfy the check above and still let a + // suppression through, because the text the matcher saw would be another + // file's. Each governed source is therefore re-read here and compared, which + // is the strongest statement available without re-implementing the walk. + let mismatched: Vec<&String> = present + .iter() + .filter(|path| { + read.get(*path).is_some_and(|contents| { + crate_root + .read_to_string(path) + .map_or(true, |on_disk| on_disk != *contents) + }) + }) + .collect(); + ensure!( + mismatched.is_empty(), + "the scan read these sources with contents that do not match the file on \ + disk, so the text it matched was not the text the compiler sees:\n- {}", + mismatched + .iter() + .map(|path| path.as_str()) + .collect::>() + .join("\n- ") + ); + Ok(()) +} + +#[path = "env_access_suppressions/scanner_tests.rs"] +mod scanner_tests; + +#[path = "env_access_suppressions/read_tests.rs"] +mod read_tests; + +#[path = "env_access_suppressions/spelling_tests.rs"] +mod spelling_tests; + +#[path = "env_access_suppressions/walk_tests.rs"] +mod walk_tests; diff --git a/tests/env_access_suppressions/mask.rs b/tests/env_access_suppressions/mask.rs new file mode 100644 index 000000000..fac3ba067 --- /dev/null +++ b/tests/env_access_suppressions/mask.rs @@ -0,0 +1,396 @@ +//! Blanking of comments and literals before the suppression scan reads text. +//! +//! The scan reads tokens rather than lines, so it cannot tell from its own +//! position whether text is code. This module is what tells it: comments and +//! string and character literals are where quoted text lives, so their contents +//! are replaced with spaces first, and something that looks like an attribute +//! is then one only when it is really in code. That is the whole mechanism +//! keeping prose — a doc comment quoting an attribute, a fixture snapshot — out +//! of the findings, and it is a stronger one than the line anchor it replaced, +//! which a `#[rustfmt::skip]` could hold open across lines. +//! +//! Blanking preserves byte offsets and newlines, so the masked text indexes +//! exactly as the source does, and only bytes belonging to a comment or literal +//! are replaced, so the result is valid UTF-8 whenever the input is. That is +//! what lets the scan run over the masked text unchanged. + +/// Return `source` with every comment and string or char literal blanked. +pub(super) fn mask_non_code(source: &str) -> String { + let bytes = source.as_bytes(); + let mut masked = bytes.to_vec(); + let mut index = 0_usize; + while let Some(byte) = bytes.get(index) { + index = match byte { + b'/' if bytes.get(index + 1) == Some(&b'/') => { + blank_line_comment(bytes, &mut masked, index) + } + b'/' if bytes.get(index + 1) == Some(&b'*') => { + blank_block_comment(bytes, &mut masked, index) + } + b'\'' => blank_char_literal(source, &mut masked, index), + b'"' | b'r' | b'b' | b'c' => blank_string_or_advance(bytes, &mut masked, index), + _ => index + 1, + }; + } + String::from_utf8_lossy(&masked).into_owned() +} + +/// Replace the byte at `index` with a space, leaving any newline in place. +fn blank_byte(masked: &mut [u8], index: usize) { + if let Some(slot) = masked.get_mut(index).filter(|slot| **slot != b'\n') { + *slot = b' '; + } +} + +/// Replace every byte of `[start, end)` with a space, leaving newlines in place. +fn blank_span(masked: &mut [u8], start: usize, end: usize) { + for index in start..end { + blank_byte(masked, index); + } +} + +/// Blank a `//` comment, returning the index of its newline or the input's end. +fn blank_line_comment(bytes: &[u8], masked: &mut [u8], start: usize) -> usize { + let mut index = start; + while let Some(byte) = bytes.get(index) { + if *byte == b'\n' { + return index; + } + blank_byte(masked, index); + index += 1; + } + index +} + +/// Blank a `/* */` comment, returning the index just past its terminator. +/// +/// Rust block comments nest, so the terminator is the one matching the opening +/// delimiter rather than the first one seen. The opening delimiter is known and +/// blanked before the scan starts, which leaves the loop one job: decide whether +/// each byte opens a nested comment, closes the innermost one, or is content. +/// The depth therefore starts at one and cannot go below it, since the loop +/// stops the moment it reaches zero — an empty `/**/` is blanked and closed by +/// the same path as any other comment. +fn blank_block_comment(bytes: &[u8], masked: &mut [u8], start: usize) -> usize { + blank_span(masked, start, start + 2); + let mut depth = 1_usize; + let mut index = start + 2; + while depth != 0 { + let Some(byte) = bytes.get(index) else { + return index; + }; + let next = bytes.get(index + 1).copied(); + index = match (*byte, next) { + (b'/', Some(b'*')) => { + depth += 1; + blank_span(masked, index, index + 2); + index + 2 + } + (b'*', Some(b'/')) => { + depth -= 1; + blank_span(masked, index, index + 2); + index + 2 + } + _ => { + blank_byte(masked, index); + index + 1 + } + }; + } + index +} + +/// Blank a char literal, or step over a quote that opens no literal. +fn blank_char_literal(source: &str, masked: &mut [u8], start: usize) -> usize { + char_literal_end(source, start).map_or_else( + || start + 1, + |end| { + blank_span(masked, start, end); + end + }, + ) +} + +/// Blank a string literal, or step over a byte that opens none. +fn blank_string_or_advance(bytes: &[u8], masked: &mut [u8], start: usize) -> usize { + match string_quote(bytes, start) { + Some((quote, raw)) => blank_string(bytes, masked, quote, raw), + None => start + 1, + } +} + +/// Blank a string body, returning the index just past its terminator. +/// +/// The two spellings end differently, so each is read by its own scan: a raw +/// string ends at the first `"` followed by as many `#` as opened it, while any +/// other string honours a backslash escape. +/// +/// What is blanked differs between them, and the difference is not cosmetic. An +/// escaped string's contents are blanked and its closing `"` is left, because a +/// `"` cannot begin anything the scan reads. A raw string's closing `"` and its +/// hashes are blanked with its body: those hashes are a delimiter the language +/// wrote, but to a reader of the masked text they are a `#` sitting directly +/// before a `[`, which is the opening of an attribute. See +/// [`blank_raw_string`]. +fn blank_string(bytes: &[u8], masked: &mut [u8], quote: usize, raw: Option) -> usize { + match raw { + Some(hashes) => blank_raw_string(bytes, masked, quote, hashes), + None => blank_escaped_string(bytes, masked, quote), + } +} + +/// Blank a raw string's body and closing delimiter, returning the index past it. +/// +/// A raw string's hashes are part of its delimiter rather than its contents, +/// and they are blanked with it. Leaving them would let the closing `#` of +/// `r#"abc"#` stand as the marker of an attribute when the literal is indexed — +/// `&r#"abc"#[allow(warnings)]` is `r#"abc"#` indexed by a call to a function +/// named `allow`, and it compiles and runs. Left in place, that `#` plus the +/// `[` behind it is token-for-token the opening of an `allow` attribute, so +/// harmless code was reported as a suppression. Measured on this shape before +/// the delimiter was blanked, at one false finding. +/// +/// The escaped form needs no equivalent change, and the asymmetry is the +/// reason it is worth stating: a `#` there would have to sit in the body, which +/// is already blanked, and the closing `"` that remains cannot open a marker on +/// its own. A raw string is the only spelling that leaves a `#` in the text +/// after the contents are removed. +fn blank_raw_string(bytes: &[u8], masked: &mut [u8], quote: usize, hashes: usize) -> usize { + let mut index = quote + 1; + while let Some(byte) = bytes.get(index) { + if *byte == b'"' && closes_raw_string(bytes, index, hashes) { + blank_span(masked, index, index + hashes + 1); + return index + hashes + 1; + } + blank_byte(masked, index); + index += 1; + } + index +} + +/// Blank an escaped string's body, returning the index just past its terminator. +fn blank_escaped_string(bytes: &[u8], masked: &mut [u8], quote: usize) -> usize { + let mut escaped = false; + let mut index = quote + 1; + while let Some(byte) = bytes.get(index) { + if escaped { + escaped = false; + } else if *byte == b'\\' { + escaped = true; + } else if *byte == b'"' { + return index + 1; + } + blank_byte(masked, index); + index += 1; + } + index +} + +/// Return whether the `"` at `index` carries the `hashes` that close a raw string. +fn closes_raw_string(bytes: &[u8], index: usize, hashes: usize) -> bool { + (1..=hashes).all(|offset| bytes.get(index + offset) == Some(&b'#')) +} + +/// Return the offset of the `"` opening a string at `start`, and its raw hashes. +/// +/// A prefix counts only where it stands as a token of its own, so an identifier +/// ending in `r` does not turn the string after it into a raw one. The hash +/// count is `Some` for a raw string and `None` for one that honours escapes, +/// including a byte string such as `b"..."`, which escapes like any other: a +/// raw string closes at the first `"` followed by its opening hashes, so +/// reading `b"a \" b"` as raw would end it at the escaped quote and blank +/// whatever followed it, hiding an attribute from the scan. +fn string_quote(bytes: &[u8], start: usize) -> Option<(usize, Option)> { + let byte = *bytes.get(start)?; + if byte == b'"' { + return Some((start, None)); + } + if start > 0 && bytes.get(start - 1).is_some_and(|it| is_ident_byte(*it)) { + return None; + } + let (prefix, raw) = string_prefix(byte, bytes.get(start + 1).copied())?; + let body = start + prefix; + if raw { + return raw_string_quote(bytes, body); + } + (bytes.get(body) == Some(&b'"')).then_some((body, None)) +} + +/// Return the prefix length of the string opening at `start` and whether it is raw. +const fn string_prefix(byte: u8, next: Option) -> Option<(usize, bool)> { + match (byte, next) { + // `br"..."` and `cr"..."` are raw; the `r` is the prefix's last byte. + (b'b' | b'c', Some(b'r')) => Some((2, true)), + // `b"..."` and `c"..."` escape like any other string. + (b'b' | b'c', Some(b'"')) => Some((1, false)), + // A bare `r` opens a raw string; any other byte opens none. + (b'r', _) => Some((1, true)), + _ => None, + } +} + +/// Return the quote offset and hash count of the raw string opening at `quote`. +/// +/// A raw string carries any number of hashes, including none, so the body is +/// located by counting them rather than by assuming at least one. +fn raw_string_quote(bytes: &[u8], quote: usize) -> Option<(usize, Option)> { + let mut index = quote; + let mut hashes = 0_usize; + while bytes.get(index) == Some(&b'#') { + hashes += 1; + index += 1; + } + (bytes.get(index) == Some(&b'"')).then_some((index, Some(hashes))) +} + +/// Return the index just past the `'` that closes the char literal at `start`. +/// +/// A quote opens a literal only where exactly one character — or one escape — +/// sits between it and a closing quote, which is what tells `'a'` from the +/// lifetime `'a`. Reading a lifetime as an unterminated literal would send the +/// scan hunting for a close and blank real code on the way, hiding whatever +/// attribute followed it. +fn char_literal_end(source: &str, start: usize) -> Option { + let bytes = source.as_bytes(); + if bytes.get(start) != Some(&b'\'') { + return None; + } + let after = start + 1; + let end = if bytes.get(after) == Some(&b'\\') { + escape_end(bytes, after + 1)? + } else { + after + source.get(after..)?.chars().next()?.len_utf8() + }; + (bytes.get(end) == Some(&b'\'')).then(|| end + 1) +} + +/// Return the index just past the `}` closing the `\u{...}` escape at `start`. +/// +/// The escape opens with `u{`; only that much anchors it, since the contents +/// are not validated here. A char literal is read to decide where it ends, not +/// to judge whether the escape is well formed. +/// +/// The precision of the returned offset is not observable through the scan, and +/// this was measured rather than assumed. `char_literal_end` accepts it only +/// when a closing quote sits at exactly that index, so an offset that is too +/// small, too large, or derived from the wrong brace makes it return `None` — +/// which leaves the literal unblanked and its contents merely become text the +/// matcher finds nothing in. Four mutations of this function, including one +/// that hunts the last `}` in the whole input, all left all 47 tests passing, +/// where disabling masking outright fails three. The behaviour here is pinned +/// by inspection and by the literal-boundary cases in the spelling suite; the +/// offset itself has no test that could distinguish it. +fn unicode_escape_end(bytes: &[u8], start: usize) -> Option { + bytes.get(start + 1).copied().filter(|byte| *byte == b'{')?; + bytes + .get(start + 2..)? + .iter() + .position(|byte| *byte == b'}') + .map(|offset| start + offset + 3) +} + +/// Return the index just past the escape sequence whose body begins at `start`. +/// +/// The forms follow the language: `\u{...}` runs to its closing brace, `\x` +/// takes the two hex digits after it, and anything else escapes a single +/// character, which may occupy several bytes. +fn escape_end(bytes: &[u8], start: usize) -> Option { + match bytes.get(start)? { + b'u' => unicode_escape_end(bytes, start), + b'x' => { + let digits = bytes.get(start + 1..start + 3)?; + digits + .iter() + .all(u8::is_ascii_hexdigit) + .then_some(start + 3) + } + _ => Some(start + 1), + } +} + +/// Return whether `byte` continues a Rust identifier. +const fn is_ident_byte(byte: u8) -> bool { + byte.is_ascii_alphanumeric() || byte == b'_' +} + +#[cfg(test)] +mod tests { + //! Direct and generated cases for what masking must preserve. + //! + //! The scan reads the masked text and slices it at offsets derived from the + //! source, so masking has to be an in-place edit: same length, same + //! newlines, and nothing but comment or literal bytes replaced. That is + //! stated in the module header and everything else here depends on it, but + //! no row of the spelling table can observe it — each pins a *finding*, and + //! an offset that drifted would show up only as some other shape's answer + //! changing. A generated search is what makes the claim falsifiable rather + //! than merely documented, since a counterexample is exactly the input whose + //! masked form is not a same-length edit. + //! + //! The generator is deliberately small and structured: fragments drawn from + //! the spellings masking has to classify, concatenated to a bounded depth, + //! rather than arbitrary bytes. Arbitrary input would be dominated by cases + //! that are not Rust at all, and the property would pass on them for the + //! wrong reason — masking never panics on a byte it does not recognise, so + //! the interesting failures are the ones where a recognised spelling leaves + //! a byte behind. `proptest` shrinks any counterexample it finds, so a + //! failure arrives as the smallest fragment sequence that still trips it. + + use super::mask_non_code; + use proptest::prelude::*; + + /// Spellings masking must classify, each with its counterpart left open. + const FRAGMENTS: [&str; 15] = [ + "//", "\n", "/*", "*/", "/* /*", "\"", "\\", "'", "r\"", "r#\"", "\"#", "b\"", "c\"", "#[", + "!", + ]; + + fn fragment() -> impl Strategy { + prop::sample::select(FRAGMENTS.as_slice()) + } + + proptest! { + #![proptest_config(ProptestConfig::with_cases(256))] + + /// Masking is an in-place edit: length and newline positions hold. + #[test] + fn masking_preserves_length_and_newlines(fragments in prop::collection::vec(fragment(), 0..24)) { + let source = fragments.concat(); + let masked = mask_non_code(&source); + prop_assert_eq!(masked.len(), source.len(), "masking changed the length of {:?}", source); + let newlines = |text: &str| { + text.bytes().enumerate().filter(|(_, b)| *b == b'\n').map(|(i, _)| i).collect::>() + }; + prop_assert_eq!( + newlines(&masked), + newlines(&source), + "masking moved a newline in {:?}", + source + ); + } + + /// Nothing outside a comment or literal is blanked. + /// + /// The complement of the property above, and the one that matters for + /// the scan: masking that ate a `#` in real code would hide the very + /// attribute the contract exists to report. A byte is only expected to + /// be blank where the source put whitespace, or where masking replaced + /// it — so each masked byte is checked to be either unchanged or a + /// space standing in the source's own place. + #[test] + fn masking_replaces_bytes_in_place(fragments in prop::collection::vec(fragment(), 0..24)) { + let source = fragments.concat(); + let masked = mask_non_code(&source); + for (index, (before, after)) in source.bytes().zip(masked.bytes()).enumerate() { + prop_assert!( + before == after || (after == b' ' && before != b'\n'), + "byte {} of {:?} changed from {:?} to {:?}", + index, + source, + before as char, + after as char + ); + } + } + } +} diff --git a/tests/env_access_suppressions/policy.rs b/tests/env_access_suppressions/policy.rs new file mode 100644 index 000000000..9c10cc0f9 --- /dev/null +++ b/tests/env_access_suppressions/policy.rs @@ -0,0 +1,244 @@ +//! What counts as a suppression of the environment-access policy. +//! +//! [`scanner`](super::scanner) finds the `allow` attributes; this module +//! decides which carried names constitute an offence, given the path the +//! attribute sits in. + +/// Lint names an `allow` attribute may not carry in a compiled source. +/// +/// The list follows the lint hierarchy rather than spelling one name, because +/// allowing a parent of the policy lint silences it just as naming it does. +/// `disallowed_methods` is declared in Clippy's `style` group, and `clippy::all` +/// sits above that; both were measured to suppress the policy outright under +/// this repository's configuration, `-D warnings` included. +/// +/// `warnings` is banned too, but *not* because it sits above them — it does not. +/// The `warnings` group is the set of lints currently at `warn`, and Cargo +/// passes `[workspace.lints]` as command-line denies, so the policy lint is at +/// `deny` and therefore *outside* the group: `#![allow(warnings)]` on its own +/// leaves the policy lint firing, measured at exit 101 both bare and under the +/// gate's flags. Three measurements say it still belongs in the set. It +/// silences every warn-level lint under the gate, a file with four diagnostics +/// compiling clean; it silences `unfulfilled_lint_expectations`, which is the +/// self-removal mechanism `clippy.toml` relies on when it says the backlog +/// "removes itself instead of rotting"; and it is one half of the only measured +/// way to defeat the gate's own flags, described below. +/// +/// That combination is worth stating precisely, because neither half is an +/// evasion alone and the pair is. `#![warn(clippy::disallowed_methods)]` lowers +/// the policy lint from `deny` to `warn`, which *puts it into the `warnings` +/// group*; `#![allow(warnings)]` then suppresses it. Measured at exit 0 under +/// `RUSTFLAGS=-D warnings`, in either order. A `warn` of the policy lint alone +/// is re-promoted by `-D warnings` and exits 101, and the `allow` alone cannot +/// reach the lint; only together do they escape. The ban below is what closes +/// it — the scanner reports the `allow(warnings)` half — and it is the reason +/// this entry is load-bearing rather than decorative. Should the lint target +/// ever stop passing `-D warnings`, the `warn` family must be re-measured +/// before the ban list is trusted: the re-promotion is the only thing holding +/// that side of the pair. +/// +/// The two guard lints are what make the seam taxonomy's `expect`-not-`allow` +/// rule enforceable, and they are cheap to protect: a scan that reads the +/// attributes reports an item-level `allow` of the policy lint wherever it +/// sits, but nothing else reports a crate that has silenced the reporter. See +/// "Enforcing the environment mandate" in the developers' guide. +/// +/// That second job is why `clippy::restriction` is banned even though it cannot +/// reach the policy lint. It is the *group* of both guard lints — measured from +/// `cargo clippy -- -W help`, which lists `clippy::allow-attributes` and +/// `clippy::allow-attributes-without-reason` as its members — so one crate-level +/// `#![allow(clippy::restriction, reason = "...")]` silences them together, and +/// an item-level bare `allow` further down then passes unreported. Measured on a +/// file whose only offence is that item-level `allow`: exit 101 with no crate +/// attribute, exit 0 with one. The `allow_attributes` entry above does not do +/// this — it leaves both diagnostics firing, exit 101 — because +/// `allow_attributes` does not fire on the *inner* form, which is the whole +/// reason this module reads source text. `blanket_clippy_restriction_lints` is +/// denied in the workspace but does not cover the attribute route either: it +/// fires on a group-level `-W clippy::restriction`, and reports nothing for an +/// attribute naming the group, measured at zero diagnostics. +/// +/// The criterion for membership is therefore "can suppress something this +/// module exists to protect", not "can suppress the policy lint", and the two +/// are different sets. Membership is decided by measurement, per name. +/// +/// The last three entries close a second way in, measured rather than assumed. +/// Clippy keeps the old spelling of a renamed lint, and a renamed name still +/// selects the lint it was renamed to, so `clippy::disallowed_method` — an +/// alias of the policy lint — silences the policy exactly as the current name +/// does. The same is true of the bare `disallowed_methods`, the name the lint +/// carried in a set of toolchain versions and still accepts. Ordinarily this is +/// harmless, because the rename is reported and `renamed_and_removed_lints` is +/// denied, so an alias is an error rather than a suppression. Allowing that +/// lint as well hides the rename, and the alias then silences the policy in +/// silence: measured at exit 0 where the same file without the attribute exits +/// 101. Banning the enabler closes the whole class of alias evasions, since no +/// alias suppresses anything while the rename that names it is still reported; +/// banning each alias too keeps the pair honest if a future Clippy stops +/// reporting renames. +/// +/// `unknown_lints` is deliberately *not* in the set, and the reason is worth +/// stating because it looks like it belongs. It hides the report that an +/// attribute names a lint that does not exist, which sounds like the rename +/// mechanism above. It was measured and it is not: a misspelled name is a no-op +/// whether or not the report is allowed, so suppressing `unknown_lints` cannot +/// silence the policy, and `#![allow(unknown_lints, disallowed_methods)]` +/// without the rename enabler still exits 101. Banning a name that cannot +/// suppress anything would be a rule the code cannot justify. The workspace +/// still denies `unknown_lints`, so a misspelled name remains an error at the +/// lint level, which is where that concern belongs. +/// +/// The test is applied to the name, not to the category it belongs to. An +/// earlier reading of this paragraph took "cannot suppress anything" to excuse +/// every name that leaves the policy lint firing, which would also excuse +/// `clippy::allow_attributes` and `clippy::allow_attributes_without_reason` +/// above — both in the set, both unable to reach the policy lint. What +/// distinguishes `unknown_lints` is that no measurement shows it silencing +/// *anything*; the guard lints can be silenced, and `clippy::restriction` does +/// it. So each name here is measured against what it can actually reach. +const FORBIDDEN_ALLOW_LINTS: [&str; 10] = [ + "clippy::disallowed_methods", + "clippy::style", + "clippy::all", + "clippy::restriction", + "warnings", + "clippy::allow_attributes", + "clippy::allow_attributes_without_reason", + "clippy::disallowed_method", + "renamed_and_removed_lints", + "disallowed_methods", +]; + +/// Paths permitted to suppress the two guard lints, and which of those they may. +/// +/// This is a scoped exemption, not a general one: a file listed here may still +/// not suppress the policy lint itself, its group, or `warnings`. The three +/// files are the derive-isolation modules documented in the developers' guide. +/// Each isolates `thiserror`/`miette` derive expansions, where +/// `unused_assignments` fires on some Rust versions and not others. `#[expect]` +/// fails when the lint does not fire, and `unfulfilled_lint_expectations` +/// cannot itself be expected, so the module must carry an `allow` — which the +/// guard lints then reject, leaving the module no way to state the suppression +/// that the guard lints themselves require it to state. +/// +/// A future reader who removes the workaround should delete the entry for that +/// file here at the same time, or this exemption outlives its reason. +/// See . +const SCOPED_ALLOWLIST: [(&str, [&str; 2]); 3] = [ + ( + "src/runner/error.rs", + [ + "clippy::allow_attributes", + "clippy::allow_attributes_without_reason", + ], + ), + ( + "src/manifest/diagnostics/mod.rs", + [ + "clippy::allow_attributes", + "clippy::allow_attributes_without_reason", + ], + ), + ( + "src/manifest/diagnostics/yaml.rs", + [ + "clippy::allow_attributes", + "clippy::allow_attributes_without_reason", + ], + ), +]; + +/// Return whether a clause of an attribute body is its `reason = "..."` argument. +fn is_reason_clause(clause: &str) -> bool { + clause + .strip_prefix("reason") + .is_some_and(|rest| rest.trim_start().starts_with('=')) +} + +/// Split an attribute body on the commas that separate its clauses. +/// +/// Only commas at the top level separate clauses: one inside a `reason` +/// string, or inside a nested group, is part of the clause being read. +fn split_clauses(body: &str) -> Vec { + let mut clauses = Vec::new(); + let mut current = String::new(); + let mut depth = 0_usize; + let mut in_string = false; + let mut escaped = false; + for character in body.chars() { + if in_string { + if escaped { + escaped = false; + } else if character == '\\' { + escaped = true; + } else if character == '"' { + in_string = false; + } + current.push(character); + continue; + } + match character { + '"' => { + in_string = true; + current.push(character); + } + '(' => { + depth += 1; + current.push(character); + } + ')' => { + depth = depth.saturating_sub(1); + current.push(character); + } + ',' if depth == 0 => clauses.push(std::mem::take(&mut current)), + _ => current.push(character), + } + } + clauses.push(current); + clauses +} + +/// Return `name` with the spellings that do not change what it denotes removed. +/// +/// Two spellings reach the compiler without reaching a reader's eye, and both +/// were measured to silence the policy outright while passing the scan: +/// `r#clippy::disallowed_methods`, where a raw identifier denotes whatever the +/// name without the prefix denotes, and `clippy :: disallowed_methods`, where +/// whitespace separates the segments of one path. Comparing the normalized name +/// rather than the written one is what makes the ban a statement about which +/// lints are named instead of about how they are spelled. +fn canonical_lint(name: &str) -> String { + name.replace("r#", "") + .chars() + .filter(|character| !character.is_whitespace()) + .collect() +} + +/// Return the lint names an attribute body carries, excluding its reason. +/// +/// Each name is normalized by [`canonical_lint`], so a spelling that means the +/// same lint is compared as that lint. The reason clause is recognized on the +/// written text, before normalization, since it is the `reason` token that +/// identifies it rather than any lint it names. +pub(super) fn named_lints(body: &str) -> Vec { + split_clauses(body) + .into_iter() + .map(|clause| clause.trim().to_owned()) + .filter(|clause| !clause.is_empty() && !is_reason_clause(clause)) + .map(|clause| canonical_lint(&clause)) + .collect() +} + +/// Return whether `path` suppressing `lint` is an offence. +/// +/// A path on the scoped allowlist is excused the two guard lints it names and +/// nothing else: an `allow` of the policy lint, of its group, or of `warnings` +/// is a finding wherever it appears, exemption or not. +pub(super) fn is_offence(path: &str, lint: &str) -> bool { + if !FORBIDDEN_ALLOW_LINTS.contains(&lint) { + return false; + } + !SCOPED_ALLOWLIST + .iter() + .any(|(allowed_path, allowed_lints)| *allowed_path == path && allowed_lints.contains(&lint)) +} diff --git a/tests/env_access_suppressions/read_tests.rs b/tests/env_access_suppressions/read_tests.rs new file mode 100644 index 000000000..86fd2eb0a --- /dev/null +++ b/tests/env_access_suppressions/read_tests.rs @@ -0,0 +1,150 @@ +//! Self-tests for which sources the scan reads. +//! +//! The contract's scan and the coverage invariant each walk the workspace, and +//! they have to agree about what a source is. They do not agree by +//! construction — they are two functions with two filters — so the agreement +//! is pinned here, against a synthetic tree rather than against the repository: +//! a shape the repository does not happen to contain today is exactly the shape +//! that would go unnoticed if the test read the real tree. +//! +//! The walk's own reach is pinned in `walk_tests.rs`; this module is about the +//! read set the reach is used for. + +use super::roots::collect_rust_sources; +use super::scanner::scan_source; +use anyhow::{Context, Result, ensure}; +use camino::Utf8Path; +use cap_std::{ambient_authority, fs_utf8::Dir}; +use tempfile::tempdir; + +/// Fail if the scan reads only the files a `.rs` name points at. +/// +/// The compiler reaches a module through whatever `#[path = "..."]` names, with +/// no extension test of its own: `#[path = "suppressed.inc"] mod suppressed;` +/// compiles, and the module may open with +/// `#![allow(clippy::disallowed_methods)]`, which `clippy::allow_attributes` +/// does not report because it does not fire on the inner form. An extension +/// filter therefore names the compiled sources by a spelling the language does +/// not require, and the shape is invisible to every other test here: the file +/// is under a scanned root, so [`is_scanned`] would say it is governed, and it +/// is simply never asked because the walk filtered it out first. +/// +/// The tree is synthetic for the usual reason — the repository holds no such +/// file today, which is exactly the shape that would go unnoticed. The binary +/// is here too, because the wider read set is what admits it and a walk that +/// failed on it would report an I/O error rather than a suppression. +#[test] +fn a_source_without_an_rs_extension_is_still_read() -> Result<()> { + let scratch = tempdir().context("create a scratch directory for the walk")?; + let scratch_path = Utf8Path::from_path(scratch.path()) + .context("a temporary directory path should be valid UTF-8")?; + let root = Dir::open_ambient_dir(scratch_path, ambient_authority()) + .context("open the scratch directory")?; + let directory = "src"; + root.create_dir_all(directory) + .with_context(|| format!("create `{directory}`"))?; + root.write( + "src/suppressed.inc", + b"#![allow(clippy::disallowed_methods)]\n", + ) + .context("write the module reached by `#[path]`")?; + root.write("src/kept.rs", b"fn kept() {}\n") + .context("write the kept source")?; + // Not UTF-8, so not a module: the walk must pass over it rather than fail + // on a read it cannot decode. + root.write("src/blob.bin", b"\xff\xfe\x00\x01") + .context("write the binary")?; + // A dot-file is tooling state rather than a module, and stays out. + root.write("src/.hidden", b"#![allow(clippy::disallowed_methods)]\n") + .context("write the dot-file")?; + + let mut read = Vec::new(); + collect_rust_sources(&root, Utf8Path::new("src"), &mut read)?; + let mut names: Vec<&str> = read.iter().map(|(path, _)| path.as_str()).collect(); + names.sort_unstable(); + + ensure!( + names == ["src/kept.rs", "src/suppressed.inc"], + "the scan must read a source the compiler reaches through `#[path]`, \ + whatever the file is named, and must pass over a file that is not text \ + and a name a `#[path]` cannot be written as; got {names:?}" + ); + // Reading it is only half of it: the file the compiler compiles must be one + // the scan reports on, which is what turns the read into a finding. + let findings: Vec = read + .iter() + .flat_map(|(path, contents)| scan_source(path, contents)) + .map(|(path, lint)| format!("{path}: {lint}")) + .collect(); + ensure!( + findings == ["src/suppressed.inc: clippy::disallowed_methods"], + "the module the compiler reaches through `#[path]` carries the policy \ + suppression, so the scan must report it; got {findings:?}" + ); + Ok(()) +} + +/// Fail if the walk treats a source it could not read as one that is absent. +/// +/// The walk's wider read set is what makes the distinction matter: it now reads +/// files whose being Rust this walk cannot see, so a file that will not decode +/// has to be declined rather than fatal. But declining has to stay the narrow +/// case. A file that *is* text and could not be read is a source the gate did +/// not scan, which is the silent non-coverage the contract exists to prevent, +/// and calling that "nothing to see" is that same silence wearing an +/// error-handling hat. +/// +/// The two are indistinguishable from the walk the suite performs, since every +/// path it touches is readable — so a filter that swallowed *every* error would +/// leave the whole suite green, measured exactly that way. Asserting the +/// predicate's own output is not enough to close that: it pins which kinds are +/// named, but the filter that consults it could still be a catch-all and the +/// suite would not notice, which was measured by replacing the guard with +/// `Err(_) => Ok(None)` and watching all 57 tests pass. So the read is taken +/// through [`read_source`] itself, and the kind that must not be swallowed is +/// produced by a path that really reports it. +/// +/// A directory is that path, and it is the one shape here that behaves the same +/// way on every machine: the read fails with `IsADirectory`, which no filter may +/// classify as "not text". A permissions-based version was written and rejected +/// — it fails spuriously when the suite runs as root, which is how a container +/// lane runs it, and the mode bits it relies on are Unix-only. A missing file +/// and a file that will not decode are read here too, so that the pair is +/// asserted at the same level rather than one of them by proxy. +#[test] +fn only_a_file_that_is_not_text_may_be_passed_over() -> Result<()> { + let scratch = tempdir().context("create a scratch directory for the walk")?; + let scratch_path = + Utf8Path::from_path(scratch.path()).context("a temporary path should be valid UTF-8")?; + let root = Dir::open_ambient_dir(scratch_path, ambient_authority()) + .context("open the scratch directory")?; + root.create_dir_all("sub") + .context("create a directory to read as a file")?; + // Not UTF-8, so there is no source text here and the walk may pass it over. + root.write("blob.bin", b"\xff\xfe\x00\x01") + .context("write a file that will not decode")?; + for absent in ["missing.file", "blob.bin"] { + ensure!( + super::roots::read_source(&root, absent) + .with_context(|| format!("read `{absent}`"))? + .is_none(), + "`{absent}` holds no source text, so the walk may pass it over" + ); + } + // The load-bearing half: the file is there and could not be read as text, + // so it is a source this gate did not scan and must fail rather than be + // reported as absent. A catch-all filter passes every assertion above and + // fails this one. + let error = super::roots::read_source(&root, "sub") + .expect_err("a directory is not text, so reading it must be an error"); + ensure!( + !super::roots::is_not_text( + error + .downcast_ref::() + .map_or(std::io::ErrorKind::Other, std::io::Error::kind) + ), + "a directory that could not be read is a source that went unscanned, so \ + the walk must report it rather than let it go silently unread; got {error:?}" + ); + Ok(()) +} diff --git a/tests/env_access_suppressions/roots.rs b/tests/env_access_suppressions/roots.rs new file mode 100644 index 000000000..6c45a4b2e --- /dev/null +++ b/tests/env_access_suppressions/roots.rs @@ -0,0 +1,366 @@ +//! Which sources the suppression scan reads, and how the walk finds them. +//! +//! The roots are the crate's answer to "what is a compiled source here", and +//! both the scan and the coverage invariant are built on that answer, so the +//! two must agree. They are kept together for that reason: a change to the root +//! list, to the skip list, or to either walk is a change to the same rule, and +//! a reader who has only one half of it cannot tell whether the gate still +//! covers what it claims to. +//! +//! The crate root holds the assertions; this module holds the traversal they +//! are made of. + +use anyhow::{Context, Result}; +use camino::Utf8Path; +use cap_std::{ambient_authority, fs_utf8::Dir}; + +/// Roots, relative to the workspace root, whose Rust sources the crate compiles. +/// +/// The list is the roots the workspace lints rather than the ones a convention +/// calls source. `tests` is included because `cargo clippy --workspace +/// --all-targets` lints integration-test targets and the modules they wire in, +/// exactly as it lints the library, so an inner attribute there silences the +/// policy for the whole test binary. Measured: with an integration target that +/// reads the environment, the target fails to compile without the attribute and +/// compiles clean with it. Leaving `tests` out would hand the evasion a second +/// home. `benches` and `examples` are included for the same reason: Cargo +/// discovers targets in both, and a benchmark or example target is compiled and +/// linted like any other, so its own environment reads are governed by the same +/// policy. `examples` holds no Rust source today; it is listed because the +/// directory Cargo discovers is governed whether or not it is currently +/// occupied, and the coverage assertion below is what makes that safe to say — +/// it fails if any Rust source anywhere in the workspace is outside these +/// roots, so a root that is misspelled, or a target location nobody predicted, +/// is caught rather than silently excusing its sources. +pub(super) const COMPILED_SOURCE_ROOTS: [&str; 6] = [ + "src", + "build_l10n_audit", + "test_support/src", + "tests", + "benches", + "examples", +]; + +/// The fewest Rust sources the workspace can hold while the walk still works. +/// +/// Well below the count a healthy tree carries, so ordinary growth and pruning +/// never trip it; a walk that descended nowhere, or stopped after one directory, +/// would. +pub(super) const MINIMUM_WORKSPACE_SOURCES: usize = 100; + +/// Compiled sources that sit outside every [`COMPILED_SOURCE_ROOTS`] root. +/// +/// `build.rs` is compiled and linted by `cargo clippy --all-targets`, and it is +/// where a build script's own environment reads live, so an inner attribute +/// there silences the policy for the build script exactly as it would in a +/// library source. It is listed separately because the walk is over directories +/// and this is a file. +pub(super) const STANDALONE_COMPILED_SOURCES: [&str; 1] = ["build.rs"]; + +/// Append every source the scan reads beneath `directory`, with its contents. +/// +/// The order is the directory's, not this function's: `read_dir` reports +/// entries as the filesystem lists them and std promises nothing about that +/// order. Nothing here depends on it — the assertion over the result is about +/// the set of findings, and each source is read in full — so no caller should +/// either. The failure message sorts its findings for the same reason; see +/// [`build_error_message`](super::build_error_message). +/// +/// [`MACHINE_LOCAL_DIRECTORIES`] is skipped by name, at whatever depth it +/// appears, exactly as the coverage walk below skips it. A scanned root is a +/// directory a contributor edits, but a cache inside one is still a cache: the +/// entry names are relative to a machine, so a `.uv-cache` under `tests/` holds +/// third-party or generated sources that are not repository content. Reading +/// them would make the verdict depend on which tools had run — a vendored crate +/// that suppressed the policy would fail the gate on one machine and pass on +/// another — and the failure would be near-impossible to diagnose, because such +/// a path is git-ignored, so it appears in no diff and in no `git status`. +/// +/// Skipping here does not open a hole, because this walk is not what decides +/// which sources are governed: [`is_scanned`] does. A machine-local name is one +/// the repository ignores, so a source under it is not one this gate is +/// answerable for, and the walk's self-test fails if that ever stops holding +/// for a name in the list. +/// +/// An absent directory is an empty one rather than an error. A root that does +/// not exist holds no sources, so there is nothing here to miss, and the +/// coverage invariant is what keeps that from becoming a hole: the walk below +/// finds every Rust source in the workspace and fails on any that is not +/// scanned, so a root whose sources moved elsewhere — renamed, misspelled, +/// deleted — is reported there by name. Failing here instead would mean +/// reporting the same fact twice, in the less useful form of an I/O error that +/// does not say which source went uncovered. Every other I/O error still +/// propagates: an unreadable directory is a real failure, and the distinction +/// between "holds nothing" and "could not be read" is worth keeping. +pub(super) fn collect_rust_sources( + root: &Dir, + directory: &Utf8Path, + sources: &mut Vec<(String, String)>, +) -> Result<()> { + let entries = match root.read_dir(directory) { + Ok(entries) => entries, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(error) => return Err(error).with_context(|| format!("read `{directory}`")), + }; + for entry_result in entries { + let entry = entry_result.with_context(|| format!("read an entry of `{directory}`"))?; + let name = entry + .file_name() + .with_context(|| format!("read an entry name in `{directory}`"))?; + if MACHINE_LOCAL_DIRECTORIES.contains(&name.as_str()) { + continue; + } + let path = format!("{directory}/{name}"); + let file_type = entry + .file_type() + .with_context(|| format!("read the file type of `{path}`"))?; + if file_type.is_dir() { + collect_rust_sources(root, Utf8Path::new(&path), sources)?; + } else if is_readable_source(&name) { + sources.extend(read_source(root, &path)?.map(|text| (path, text))); + } + } + Ok(()) +} + +/// Return whether an entry name is a Rust source. +pub(super) fn is_rust_source(name: &str) -> bool { + Utf8Path::new(name).extension().is_some_and(|it| it == "rs") +} + +/// Read one source, returning `None` when the file is not text at all. +/// +/// The wider read set admits files whose being Rust is not something this walk +/// can see — anything named by a `#[path]` — and a directory under a scanned +/// root may hold a binary that no `#[path]` names. Rust source is UTF-8 by +/// definition, so a file that will not decode cannot be a module the compiler +/// reads, and skipping it is the honest classification rather than a failure. +/// +/// Every other error propagates, and that half is the load-bearing one: a file +/// that *is* text but could not be read is a source that went unscanned, which +/// is the silent non-coverage this whole contract exists to prevent, so it must +/// fail rather than be passed over. The two cases are distinguished by +/// [`is_not_text`] rather than by a catch-all, and the distinction is pinned by +/// a test — a filter that swallowed every error would be indistinguishable from +/// this one on the walk the suite actually performs, so it is measured against +/// an unreadable path directly. +pub(super) fn read_source(root: &Dir, path: &str) -> Result> { + match root.read_to_string(path) { + Ok(contents) => Ok(Some(contents)), + Err(error) if is_not_text(error.kind()) => Ok(None), + Err(error) => Err(error).with_context(|| format!("read {path}")), + } +} + +/// Return whether an I/O error means the file is not text this walk can read. +/// +/// `InvalidData` is what a decode failure reports, and `NotFound` is the file +/// that was listed at the start of the walk and is gone by the time it is read +/// — a concurrent build writing under a scanned root, or a lint run that +/// deleted a fixture. Both mean "there is no source text here", which is a +/// classification rather than a failure. Anything else means the file is there +/// and could not be read, which is a source this gate did not scan, and that +/// must fail. The list is a function of its own so the error filter is a named +/// rule rather than a pattern inside a `match`, and so it can be exercised +/// directly by a test: a catch-all here would look identical from the walk the +/// suite performs, since every path that walk touches is readable. +pub(super) const fn is_not_text(kind: std::io::ErrorKind) -> bool { + matches!( + kind, + std::io::ErrorKind::InvalidData | std::io::ErrorKind::NotFound + ) +} + +/// Return whether an entry name is one the scan reads. +/// +/// A `.rs` file is read by name, and so is every other file that is not +/// dot-prefixed — because a module's file need not be named `.rs`. Rust reads +/// a module from whatever `#[path = "..."]` names, with no extension test of +/// its own: `#[path = "suppressed.inc"] mod suppressed;` compiles, the module +/// may open with `#![allow(clippy::disallowed_methods)]`, and that inner form +/// is exactly what `clippy::allow_attributes` does not report. Measured on a +/// probe crate: the module compiles where the same file without the attribute +/// exits 101, so an extension filter names the compiled sources by a spelling +/// the language does not require, and the one reader who cares about that +/// filter is the one who would rather the source went unread. +/// +/// Reading every file is the safer direction because the two mistakes are not +/// symmetric: a file read but never compiled costs a failure message naming a +/// real file, while a file compiled but not read hides a suppression, which is +/// the failure this contract exists to prevent. The file's other rule breaks +/// the same way for the same reason — see [`MACHINE_LOCAL_DIRECTORIES`]. +/// +/// The breadth is bounded rather than open-ended. The walk is already confined +/// to [the scanned roots](COMPILED_SOURCE_ROOTS), the skip list still removes +/// the caches and `target`, and a name beginning with a dot stays out because a +/// dot-file under a source root is tooling state — `.gitignore`, +/// `.editorconfig`, `.rustfmt.toml` — rather than anything a `#[path]` names. +/// A `.rs` file is read whatever its name, so the dot rule can only ever narrow +/// the files that were never Rust sources to begin with. +/// +/// Measured against the tree this was written on: 216 files across the scanned +/// roots are read by the wider rule, and the scan finds nothing in any of them. +/// The scan is token-wise over source text, so prose that quotes an attribute +/// in a comment, a string, or a snapshot of either is blanked before the +/// matcher sees it; only a file that really opens with the attribute would be +/// reported, and such a file is one `#[path]` away from being compiled. +pub(super) fn is_readable_source(name: &str) -> bool { + is_rust_source(name) || !name.starts_with('.') +} + +/// Directories the coverage walk does not descend into. +/// +/// These are the machine-local directories the repository declares in +/// `.gitignore`, plus the compiler's output and the caches of the tools this +/// repository runs. Each belongs to a machine rather than to the repository, so +/// each may hold a Rust source that is not a source here: `target` holds +/// generated and vendored output, and a package cache holds the extracted +/// sources of third-party crates — a Python distribution with a Rust extension +/// ships `.rs` files with it. Descending into one would make the gate turn on +/// what a cache happened to contain on one machine, which is the thing it must +/// not do. +/// +/// An entry is skipped by *name*, at whatever depth it appears, rather than by +/// comparing the whole workspace-relative path against a list of root +/// directories. That is the rule `.gitignore` already states — its patterns +/// carry no leading slash, so `target/`, `memories/`, and `__pycache__/` are +/// ignored at every level, and the list is drawn from them. Matching by name +/// keeps the two in step: a name git will not track is not a name a compiled +/// source can live under without `git add -f`, which is deliberate +/// circumvention rather than an accident this invariant is shaped to catch. +/// Verified both ways: `git ls-files` finds no tracked path beneath any of the +/// fifteen names at any depth, and every nested occurrence in the tree sits +/// inside another skipped directory or a cache. +/// +/// That appeal to `.gitignore` is only sound while it holds for every name, so +/// it is enforced rather than trusted. It did not hold twice: `.netsuke` is +/// netsuke's own runtime state, and `.ruff_cache` a tool cache, but both were +/// skipped while the repository's `.gitignore` declined them — every sibling +/// cache is listed, and they were missed — so a `.rs` file placed under either +/// would have been tracked, compiled, skipped by the walk, and reported by +/// nobody. Both names are now in `.gitignore` like their siblings, and the +/// walk's self-test requires each skipped name to be one the repository's own +/// rules ignore, with `.git` as the single named exception. +/// +/// The list is named rather than "anything dot-prefixed", and that distinction +/// is the point. A dot-directory is not evidence of a cache: `.config`, +/// `.github`, and `.rules` are tracked repository content, and Cargo will +/// compile a target declared under any directory at all, hidden or not. A walk +/// that skipped every dot-prefixed name would therefore neither scan nor report +/// a target sitting in one, which is precisely the silent non-coverage this +/// invariant exists to prevent. Walking them instead turns that into a loud +/// failure that names the source. Where the two rules disagree, the tie breaks +/// towards reporting: an entry that should have been here but is missing costs +/// a false failure that names a real file, while an entry that should not be +/// here hides a source. +pub(super) const MACHINE_LOCAL_DIRECTORIES: [&str; 15] = [ + "__pycache__", + ".claude", + ".crush", + ".git", + ".grepai", + ".hypothesis", + ".memdb", + ".netsuke", + ".pytest_cache", + ".ruff_cache", + ".uv-cache", + ".uv-tools", + ".vtcode", + "memories", + "target", +]; + +/// Append every Rust source the coverage invariant governs, in no set order. +/// +/// The walk descends everything but [`MACHINE_LOCAL_DIRECTORIES`], which is +/// where the generated output and the third-party sources live. Everything +/// else is repository content until proven otherwise — including a +/// dot-directory — so a compiled source anywhere in the workspace is found and +/// named rather than passed over. +pub(super) fn collect_all_sources( + root: &Dir, + directory: &Utf8Path, + found: &mut Vec, +) -> Result<()> { + for entry_result in root + .read_dir(directory) + .with_context(|| format!("read `{directory}`"))? + { + let entry = entry_result.with_context(|| format!("read an entry of `{directory}`"))?; + collect_source_entry(root, directory, found, &entry)?; + } + Ok(()) +} + +/// Classify one entry of `directory`, recursing where the walk must descend. +/// +/// The early returns are the whole classification: a machine-local name is +/// skipped, a directory is descended, a Rust source is kept, and anything else +/// is passed over. Only the last two touch `found`, and only a directory +/// recurses, so the caller above does nothing but open the directory and hand +/// its entries here. +fn collect_source_entry( + root: &Dir, + directory: &Utf8Path, + found: &mut Vec, + entry: &cap_std::fs_utf8::DirEntry, +) -> Result<()> { + let name = entry + .file_name() + .with_context(|| format!("read an entry name in `{directory}`"))?; + if MACHINE_LOCAL_DIRECTORIES.contains(&name.as_str()) { + return Ok(()); + } + let path = join_path(directory, &name); + let file_type = entry + .file_type() + .with_context(|| format!("read the file type of `{path}`"))?; + if file_type.is_dir() { + return collect_all_sources(root, Utf8Path::new(&path), found); + } + if is_rust_source(&name) { + found.push(path); + } + Ok(()) +} + +/// Join a directory and an entry name, keeping the walk root's paths bare. +pub(super) fn join_path(directory: &Utf8Path, name: &str) -> String { + match directory.as_str() { + "." => name.to_owned(), + _ => format!("{directory}/{name}"), + } +} + +/// Return whether `path` is one of the sources [`compiled_sources`] reads. +/// +/// A source counts as covered when it sits beneath a scanned root or is one of +/// the standalone sources. Coverage is what makes the scan's silence mean +/// something: a source outside this set is not "clean", it is unread, and the +/// difference is the whole point of the invariant that calls this. +pub(super) fn is_scanned(path: &str) -> bool { + STANDALONE_COMPILED_SOURCES.contains(&path) + || COMPILED_SOURCE_ROOTS.iter().any(|root| { + path.strip_prefix(root) + .is_some_and(|rest| rest.starts_with('/')) + }) +} + +/// Read every compiled source the scan governs, with its workspace-relative path. +pub(super) fn compiled_sources() -> Result> { + let crate_root = Dir::open_ambient_dir(env!("CARGO_MANIFEST_DIR"), ambient_authority()) + .context("open the workspace root")?; + let mut sources = Vec::new(); + for root_path in COMPILED_SOURCE_ROOTS { + collect_rust_sources(&crate_root, Utf8Path::new(root_path), &mut sources) + .with_context(|| format!("walk the `{root_path}` source root"))?; + } + for path in STANDALONE_COMPILED_SOURCES { + let contents = crate_root + .read_to_string(path) + .with_context(|| format!("read {path}"))?; + sources.push((path.to_owned(), contents)); + } + Ok(sources) +} diff --git a/tests/env_access_suppressions/scanner.rs b/tests/env_access_suppressions/scanner.rs new file mode 100644 index 000000000..a20dbab7e --- /dev/null +++ b/tests/env_access_suppressions/scanner.rs @@ -0,0 +1,383 @@ +//! Detection of `allow` attributes that switch off a policy-carrying lint. +//! +//! The scan reads source text because that is what an attribute is: there is no +//! execution to model, and "this file does not opt out" is exactly a statement +//! about what the file says. Two properties keep it off innocent sources. +//! Comments and string or char literals are blanked by [`super::mask`] first, +//! because that is where quoted text lives and a line inside either is not a +//! line of code. An attribute is then recognized by its tokens — `#`, an +//! optional `!`, `[`, a name, `(` — so prose that quotes an attribute is not a +//! finding, since only masking decides what is prose. Finally the attribute is +//! read to its matching parenthesis, so one that `rustfmt` has wrapped across +//! several lines is read whole rather than truncated. +//! +//! An `allow` nested in a `cfg_attr` is read too. It is the same suppression +//! written one token differently, and `clippy::allow_attributes` does not fire +//! on the inner form, so nothing else reports it. +//! +//! # Why matching is token-wise and not line-wise +//! +//! The scan once anchored at the start of a line, on the reasoning that +//! `rustfmt` normalizes an attribute's spelling and `make check-fmt` enforces +//! that, so any spelling the anchor declined to read could not reach the +//! compiler. That reasoning is false, and it was falsified by measurement +//! rather than argument. `#[rustfmt::skip]` freezes the very spelling `rustfmt` +//! would otherwise normalize, and every shape below then compiles, silences the +//! policy outright (`clippy` exit 0 where the same file without the attribute +//! exits 101), and passed the line-anchored scan: +//! +//! - `#[allow` and its `(` on separate lines, under a `#[rustfmt::skip]`; +//! - a newline between `#[allow(` and the lint list; +//! - a newline between the `#` and the `[`; +//! - `r#allow(...)`, and `r#clippy::disallowed_methods`, raw identifiers; +//! - `clippy :: disallowed_methods`, with spaces around the path separator; +//! - the deprecated bare name `disallowed_methods` beside its enabler. +//! +//! Each is the same suppression one token differently, so the matcher reads +//! tokens and tolerates whitespace between them rather than trusting a layout +//! gate to have fixed the spelling first. What keeps it off prose is masking, +//! which is the mechanism that was always doing that work: a comment or a +//! string is blanked before the matcher sees it, so an attribute quoted inside +//! one is never read, whatever line it sits on. `#[expect(...)]` and +//! `.expect(...)` are excluded by the name the matcher looks for — it accepts +//! `allow` and `cfg_attr` and nothing else — rather than by their position. +//! +//! The lesson is recorded here because it is the reason the code looks the way +//! it does: a layout gate is not a proof about spelling. It normalizes what it +//! is shown, and a skip attribute is a request to be shown nothing. +//! +//! # Why `warn` is not read +//! +//! A reviewer's natural next question is why the matcher accepts `allow` and +//! `cfg_attr` but not `warn`, so the answer is measured rather than asserted. A +//! `warn` of the policy lint *does* lower it from the workspace's `deny` to +//! `warn` — bare `cargo clippy` exits 0 where the same file exits 101 — so it +//! is a real suppression and not a no-op. It is not a *silent* one: every lint +//! and test target passes `-D warnings` (`Makefile:206` among others), and that +//! re-promotes the lint to an error. Twelve spellings were probed — inner and +//! outer, `cfg_attr`-wrapped, the group and alias names, and the guard-lint +//! forms — and every one exits 101 under the gate's flags while the bare run +//! silences the same file. Reporting a shape that cannot pass a gate would be a +//! rule the code cannot justify. Whether a name belongs in the banned set is +//! decided per name by what it can reach, not by which category it looks like it +//! belongs to — the criterion [`policy`](super::policy) states and the one that +//! admits `clippy::restriction` while leaving `unknown_lints` out. +//! +//! The exception is a `warn` of the policy lint seated beside an +//! `allow(warnings)`, and it is the one measured way past the gate's flags. The +//! `warn` lowers the policy lint to `warn`, which is what puts it *into* the +//! `warnings` group — the group is the set of lints currently at `warn`, not a +//! parent of the hierarchy — and the `allow` then suppresses that group: exit 0 +//! under `-D warnings`, in either order. Neither half escapes alone. The scan +//! reports this pair, because the `allow` half is what it matches and +//! `warnings` is in the banned set. + +use super::mask::mask_non_code; +use super::policy::{is_offence, named_lints}; + +/// Which attribute form a marker opens. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum AttributeKind { + /// An `#[allow(...)]` or `#![allow(...)]` attribute. + Allow, + /// An `#[cfg_attr(...)]` or `#![cfg_attr(...)]` attribute. + CfgAttr, +} + +impl AttributeKind { + /// Return the attribute form an attribute name selects. + fn of(name: &str) -> Option { + match name { + "allow" => Some(Self::Allow), + "cfg_attr" => Some(Self::CfgAttr), + _ => None, + } + } +} + +/// Return the offset of every attribute's `(`, with the form it opens. +/// +/// The search is over the whole text rather than a line at a time, because +/// nothing about an attribute is line-shaped: its marker may be split across +/// lines, and so may the gap before its parenthesis. Masking has already +/// removed the comments and literals where quoted text lives, so a candidate +/// here is a candidate in code. +/// +/// Scanning resumes just inside the parenthesis it found, so an `allow` nested +/// in a `cfg_attr` is not read twice — that inner form has no `#` of its own, +/// and [`cfg_attr_allow_bodies`] is what reads it. +fn attribute_opens(text: &str) -> Vec<(usize, AttributeKind)> { + let mut opens = Vec::new(); + let mut index = 0_usize; + while let Some(byte) = text.as_bytes().get(index) { + if *byte != b'#' { + index += 1; + continue; + } + if let Some((open, kind)) = attribute_at(text, index) { + opens.push((open, kind)); + index = open + 1; + } else { + index += 1; + } + } + opens +} + +/// Parse the attribute whose `#` sits at `hash`, returning its `(` offset. +/// +/// The tokens are `#`, an optional `!`, `[`, a name, and `(`, with whitespace +/// permitted between any two of them — including a newline, which is what +/// makes this independent of where a line ends. The name is read through an +/// optional `r#` prefix, so a raw identifier names the same attribute; see +/// [`identifier`]. +/// +/// Returns `None` for anything else, which is what keeps `#[expect(...)]`, +/// `.expect(...)`, `#[derive(...)]`, and a bare `#` in ordinary code out of the +/// result. `#[cfg_attr(...)]` is matched too, so that its body can be searched +/// for the `allow` it may wrap. +fn attribute_at(text: &str, hash: usize) -> Option<(usize, AttributeKind)> { + let after_hash = text.get(hash..)?.strip_prefix('#')?.trim_start(); + let after_bang = after_hash + .strip_prefix('!') + .unwrap_or(after_hash) + .trim_start(); + let after_bracket = after_bang.strip_prefix('[')?.trim_start(); + let (name, after_name) = identifier(after_bracket)?; + let kind = AttributeKind::of(name)?; + let after_paren = after_name.trim_start().strip_prefix('(')?; + Some((text.len() - after_paren.len() - 1, kind)) +} + +/// Split the Rust identifier `text` opens with, and return what follows it. +/// +/// A leading `r#` is stepped over: a raw identifier denotes whatever the name +/// without the prefix denotes, so `r#allow` is the `allow` attribute and +/// `r#clippy::disallowed_methods` is the policy lint. Being blind to the prefix +/// is the whole point — it is a spelling that reaches the compiler but not a +/// reader's eye, which is what makes it worth evading with. +/// +/// Returns `None` when no identifier opens the text. +fn identifier(text: &str) -> Option<(&str, &str)> { + let body = text.strip_prefix("r#").unwrap_or(text); + let end = body + .find(|character| !is_identifier_char(character)) + .unwrap_or(body.len()); + (end > 0).then(|| body.split_at(end)) +} + +/// Whether the byte being read sits inside a double-quoted string. +#[derive(Clone, Copy)] +enum StringState { + /// Reading code, outside every string. + Outside, + /// Reading a string's body, tracking whether the last byte escaped the next. + Quoted { + /// Whether the previous byte was an unescaped `\`. + escaped: bool, + }, +} + +impl StringState { + /// Consume one byte, returning the next state and whether it was string text. + /// + /// The flag is what lets the caller skip its own handling: every byte of a + /// string, its opening and closing quotes included, is consumed here, so a + /// parenthesis counter downstream never sees a `)` the string quoted. A + /// backslash escapes the byte after it and is cleared by whatever it + /// escaped, which keeps an escaped quote from closing the string and an + /// escaped backslash from opening an escape. + const fn consume(self, byte: u8) -> (Self, bool) { + match (self, byte) { + // A backslash in a string escapes whatever follows it. + (Self::Quoted { escaped: false }, b'\\') => (Self::Quoted { escaped: true }, true), + // An unescaped quote is the only byte that closes the string. + (Self::Quoted { escaped: false }, b'"') => (Self::Outside, true), + // Every other quoted byte is content, and so is the quote that + // opens a string from code: both leave a string being read with no + // escape pending. An escaped byte clears the escape it was the + // target of, and a quote behind an escape is content rather than a + // close, which is why the two share an arm. + (Self::Quoted { .. }, _) | (Self::Outside, b'"') => { + (Self::Quoted { escaped: false }, true) + } + // Anything else is code, and the caller must handle it. + (Self::Outside, _) => (Self::Outside, false), + } + } +} + +/// Read the parenthesized body of the attribute whose `(` sits at `open`. +/// +/// Scanning tracks parenthesis depth so an attribute that `rustfmt` has wrapped +/// across several lines is read whole. Parentheses inside a double-quoted +/// string are stepped over, and a backslash escape is honoured, so a `)` inside +/// a `reason` string does not end the scan early. Both are already unreachable +/// on [`mask_non_code`]'s output, but the tracking is what makes the function +/// correct on its own terms rather than only on that caller's. +/// +/// Returns `None` when the parenthesis is never closed, which leaves the +/// malformed attribute unread rather than reporting the remainder of the file +/// as its body. +fn read_attribute_body(source: &str, open: usize) -> Option { + // `open` is the known opening parenthesis, so the body starts inside it. + let mut depth = 1_usize; + let mut string_state = StringState::Outside; + for (offset, byte) in source.as_bytes().iter().enumerate().skip(open + 1) { + let (next_state, consumed) = string_state.consume(*byte); + string_state = next_state; + // A byte the string state consumed is not code, so naming that flag in + // each pattern leaves the loop one decision rather than a branch that + // screens a second one. + match (*byte, consumed) { + (b'(', false) => depth += 1, + (b')', false) if depth == 1 => return source.get(open + 1..offset).map(str::to_owned), + (b')', false) => depth = depth.checked_sub(1)?, + _ => {} + } + } + None +} + +/// Return the body of every `allow` nested in a `cfg_attr` body. +/// +/// The nesting may be several `cfg_attr`s deep, so the whole body is searched +/// rather than its first argument. The inner form is written without a `#`, so +/// it cannot be found the way [`attribute_at`] finds the outer one; the name is +/// located instead and must be followed by a `(`. A name that merely contains +/// `allow` — such as `clippy::allow_attributes`, or the word inside a `reason` +/// string — is skipped by the test for an identifier character on either side. +fn cfg_attr_allow_bodies(body: &str) -> Vec { + let mut bodies = Vec::new(); + let mut index = 0_usize; + while let Some(offset) = body.get(index..).and_then(|rest| rest.find("allow")) { + let at = index + offset; + let after = at + "allow".len(); + index = after; + if body + .get(..at) + .and_then(|prefix| prefix.chars().next_back()) + .is_some_and(is_identifier_char) + { + continue; + } + let Some(tail) = body.get(after..) else { + continue; + }; + if tail.chars().next().is_some_and(is_identifier_char) { + continue; + } + let Some(open) = tail + .trim_start() + .strip_prefix('(') + .map(|rest| after + tail.len() - rest.len() - 1) + else { + continue; + }; + bodies.extend(read_attribute_body(body, open)); + } + bodies +} + +/// Return the body of every `allow` attribute in `source`. +fn allow_attribute_bodies(source: &str) -> Vec { + let masked = mask_non_code(source); + let mut bodies = Vec::new(); + for (open, kind) in attribute_opens(&masked) { + match kind { + AttributeKind::Allow => bodies.extend(read_attribute_body(&masked, open)), + AttributeKind::CfgAttr => bodies.extend( + read_attribute_body(&masked, open) + .map(|body| cfg_attr_allow_bodies(&body)) + .unwrap_or_default(), + ), + } + } + bodies +} + +/// Return every suppression of the policy `source` contains, as `(path, lint)`. +pub(super) fn scan_source(path: &str, source: &str) -> Vec<(String, String)> { + let mut findings = Vec::new(); + for body in allow_attribute_bodies(source) { + for lint in named_lints(&body) { + if is_offence(path, &lint) { + findings.push((path.to_owned(), lint)); + } + } + } + findings +} + +/// Return whether `character` continues a Rust identifier. +fn is_identifier_char(character: char) -> bool { + character.is_alphanumeric() || character == '_' +} + +#[cfg(test)] +mod tests { + //! Direct cases for the reading that masking hides from the scan. + //! + //! Masking blanks a literal's contents before the scan sees them, so the + //! quoted-string tracking in [`read_attribute_body`] is unreachable through + //! [`scan_source`] and no table row can pin it: measured, the masked form of + //! `reason = "before \" after"` is `reason = " "`, with the + //! escaped quote and both its neighbours replaced by spaces. The function + //! still documents and preserves standalone correctness, so its contract is + //! asserted here, where the raw text reaches it. + + use super::read_attribute_body; + use anyhow::{Result, ensure}; + + /// The attribute body is read past a parenthesis the string merely quotes. + #[test] + fn a_parenthesis_inside_a_string_does_not_close_the_body() -> Result<()> { + let source = r#"(reason = "closing ) paren", flag)"#; + ensure!( + read_attribute_body(source, 0).as_deref() + == Some(r#"reason = "closing ) paren", flag"#), + "a `)` inside a string must not close the body" + ); + Ok(()) + } + + /// An escaped quote does not close the string that quotes it. + /// + /// This is the case the mask makes unreachable through the scan, and the one + /// a reader is most likely to think is untested. Without the escape bit the + /// `\"` would close the string early, the following `)` would close the + /// body, and the body would be truncated — which is exactly what the + /// mutation of that bit measures. + #[test] + fn an_escaped_quote_does_not_close_the_string() -> Result<()> { + let source = r#"(reason = "before \" after ) still inside", tail)"#; + ensure!( + read_attribute_body(source, 0).as_deref() + == Some(r#"reason = "before \" after ) still inside", tail"#), + "an escaped quote must not close the string, nor let its `)` close the body" + ); + Ok(()) + } + + /// Nested parentheses are counted rather than treated as the body's end. + #[test] + fn nested_parentheses_are_counted() -> Result<()> { + let source = "(cfg_attr(all(), allow(warnings)), tail)"; + ensure!( + read_attribute_body(source, 0).as_deref() + == Some("cfg_attr(all(), allow(warnings)), tail"), + "the body ends at the parenthesis matching the one that opened it" + ); + Ok(()) + } + + /// An unterminated body is unread rather than the rest of the file. + #[test] + fn an_unterminated_body_is_none() -> Result<()> { + ensure!( + read_attribute_body("(allow(warnings)", 0).is_none(), + "a body that never closes must not be returned" + ); + Ok(()) + } +} diff --git a/tests/env_access_suppressions/scanner_tests.rs b/tests/env_access_suppressions/scanner_tests.rs new file mode 100644 index 000000000..0389ef329 --- /dev/null +++ b/tests/env_access_suppressions/scanner_tests.rs @@ -0,0 +1,222 @@ +//! Self-tests for the suppression scanner. +//! +//! Each row of the table below pins one shape the scan must classify: a +//! suppression it must report, or an innocent source it must not. They run +//! against synthetic text rather than the repository, so the gate's own +//! behaviour is asserted directly instead of being inferred from a clean walk +//! over sources that happen to be clean. +//! +//! The rows share one function because they share one assertion: the findings +//! for a source equal the lints listed for it, and an innocent shape is a row +//! whose list is empty. Split into separate tests the two directions could +//! drift apart, and a shape that stopped being reported would look like a shape +//! that was never expected to be. Each case is named for the shape it pins, so +//! a failure names the shape rather than a row number. + +use super::expected_findings; +use super::scanner::scan_source; +use anyhow::{Result, ensure}; +use rstest::rstest; + +/// Every shape the scan must classify, as path, source, and expected lints. +/// +/// An empty list means the source suppresses nothing and the scan must find +/// nothing in it. The path varies because it is not merely a label: the scoped +/// exemption is keyed on it, and two rows below pin that the exemption covers +/// the derive-isolation modules and nothing else. +#[rstest] +// The evasion this contract exists for: an inner attribute, at the top of a +// file, that switches the policy off for everything below it. +#[case::inner_allow( + "src/lib.rs", + "#![allow(clippy::disallowed_methods, reason = \"escape hatch probe\")]\n", + &["clippy::disallowed_methods"] +)] +// The item-level form, and the blanket spelling a reader reaches for first. +#[case::item_allow_of_warnings( + "src/lib.rs", + "#[allow(warnings, reason = \"escape hatch probe\")]\nfn probe() {}\n", + &["warnings"] +)] +// The group above the policy lint silences it just as naming it does. +#[case::enclosing_group( + "src/lib.rs", + "#![allow(clippy::style, reason = \"escape hatch probe\")]\n", + &["clippy::style"] +)] +// `clippy::restriction` is banned for a different reason than the groups above: +// it does not reach the policy lint at all. It is the group of the two guard +// lints, so one crate-level attribute silences the reporter and an item-level +// bare `allow` further down then passes unreported. Measured: exit 101 with no +// crate attribute, exit 0 with this one. This row pins the entry, which a +// reader measuring only against the policy lint would otherwise remove. +#[case::guard_lint_group( + "src/lib.rs", + "#![allow(clippy::restriction, reason = \"escape hatch probe\")]\n", + &["clippy::restriction"] +)] +// A wrapped attribute is read whole, as `rustfmt` writes a long one. +#[case::wrapped( + "src/lib.rs", + "#![allow(\n clippy::disallowed_methods,\n reason = \"escape hatch probe\"\n)]\n", + &["clippy::disallowed_methods"] +)] +// A `)` inside the reason string does not end the attribute early. +#[case::parenthesis_in_the_reason( + "src/lib.rs", + "#[allow(warnings, reason = \"closing ) paren\")]\nfn probe() {}\n", + &["warnings"] +)] +// The old spelling of the policy lint still selects it, so it is banned too. +#[case::renamed_spelling( + "src/lib.rs", + "#![allow(clippy::disallowed_method, reason = \"escape hatch probe\")]\n", + &["clippy::disallowed_method"] +)] +// The enabler that hides the rename is the ingredient that makes it silent. +#[case::rename_enabler_with_alias( + "src/lib.rs", + "#![allow(\n renamed_and_removed_lints,\n clippy::disallowed_method,\n reason = \"escape hatch probe\"\n)]\n", + &["renamed_and_removed_lints", "clippy::disallowed_method"] +)] +// A `cfg_attr`-wrapped allow suppresses the policy just as a direct one does. +#[case::cfg_attr_wrapped( + "src/lib.rs", + "#![cfg_attr(all(), allow(clippy::disallowed_methods, reason = \"escape hatch probe\"))]\n", + &["clippy::disallowed_methods"] +)] +// An `allow` nested several `cfg_attr`s deep is still read. +#[case::nested_cfg_attr( + "src/lib.rs", + "#[cfg_attr(all(), cfg_attr(all(), allow(clippy::style, reason = \"escape hatch probe\")))]\n", + &["clippy::style"] +)] +// An inner attribute inside a macro body is read, wherever it sits. +#[case::inner_attribute_in_a_macro_body( + "src/lib.rs", + "macro_rules! probe_macro {\n () => {\n #![allow(clippy::disallowed_methods, reason = \"escape hatch probe\")]\n };\n}\n", + &["clippy::disallowed_methods"] +)] +// A guard lint named off an exempt path is a finding. Nothing else reports a +// crate that has silenced the reporter, so the scan is the only thing standing +// between this attribute and a silenced `allow_attributes`. +#[case::path_segment_named_allow( + "src/lib.rs", + "#![allow(clippy::allow_attributes, reason = \"escape hatch probe\")]\n", + &["clippy::allow_attributes"] +)] +// A lifetime is not an unterminated char literal that blanks the code after it. +#[case::attribute_after_a_lifetime( + "src/lib.rs", + "fn probe<'a>(value: &'a str) {}\n#[allow(warnings, reason = \"escape hatch probe\")]\n", + &["warnings"] +)] +// A `\u{...}` escape opens a char literal that the masker must not overrun. +// The offset the escape reader returns is only accepted when a closing quote +// sits exactly there, so an overrun makes the literal unread and leaves its +// contents in code; the attribute below is what fails if the reader walks past +// the literal's end into it. Measured: the behaviour this pins is the literal +// boundary, not the escape offset, which no row can distinguish. +#[case::attribute_after_a_unicode_escape( + "src/lib.rs", + r#"const LETTER: char = '\u{61}'; +#[allow(clippy::disallowed_methods, reason = "escape hatch probe")] +fn probe() {} +"#, + &["clippy::disallowed_methods"] +)] +// A `\x` escape takes its hex digits and no more, so the literal after it +// closes where the language says and the attribute below stays code. +#[case::attribute_after_a_hex_escape( + "src/lib.rs", + r#"const LETTER: char = '\x61'; +#[allow(clippy::disallowed_methods, reason = "escape hatch probe")] +fn probe() {} +"#, + &["clippy::disallowed_methods"] +)] +// Suppression in general is not the offence: an unrelated lint still passes. +#[case::unrelated_allow( + "test_support/src/lib.rs", + "#![allow(dead_code, reason = \"shared test-support module\")]\n", + &[] +)] +// The exemption covers the derive-isolation modules ... +#[case::exempt_isolation_module( + "src/runner/error.rs", + "#![allow(\n clippy::allow_attributes,\n clippy::allow_attributes_without_reason,\n unused_assignments\n)]\n", + &[] +)] +// ... and only those: the same attribute elsewhere still names the guard lints. +#[case::guard_lints_off_the_exempt_path( + "src/elsewhere.rs", + "#![allow(\n clippy::allow_attributes,\n clippy::allow_attributes_without_reason,\n unused_assignments\n)]\n", + &[ + "clippy::allow_attributes", + "clippy::allow_attributes_without_reason" + ] +)] +// An empty block comment is the shortest comment there is: `/**/` is its own +// opening and closing delimiter, so it must be blanked and closed by the same +// path as any other comment rather than leaving a `*/` in code or reading on to +// a terminator that never comes. The attribute behind it is what fails if the +// comment swallows text past its end. +#[case::attribute_after_an_empty_block_comment( + "src/lib.rs", + "/**/#[allow(clippy::disallowed_methods, reason = \"escape hatch probe\")]\nfn probe() {}\n", + &["clippy::disallowed_methods"] +)] +// An attribute-looking line inside a block comment suppresses nothing. +#[case::attribute_in_a_block_comment( + "src/lib.rs", + "/*\n#[allow(clippy::disallowed_methods, reason = \"commented example\")]\n*/\npub fn probe() {}\n", + &[] +)] +// An attribute-looking line inside a raw string suppresses nothing. +#[case::attribute_in_a_raw_string( + "src/lib.rs", + "const FIXTURE: &str = r#\"\n#[allow(warnings, reason = \"quoted example\")]\n\"#;\n", + &[] +)] +// An escaped quote in a byte string does not hide the attribute after it. +#[case::escaped_quote_in_a_byte_string( + "src/lib.rs", + r#"fn probe() { let sample = b"a \" b"; } +#[allow(clippy::disallowed_methods, reason = "escape hatch probe")] +fn probe2() {} +"#, + &["clippy::disallowed_methods"] +)] +// An attribute-looking line inside a byte string is quoted text, not code. +#[case::attribute_in_a_byte_string( + "src/lib.rs", + r#"const SAMPLE: &[u8] = b"one \" two +#[allow(warnings, reason = \"sample\")] +three"; +"#, + &[] +)] +// A `cfg_attr` naming a lint but not suppressing it is not an offence. +#[case::cfg_attr_denies( + "src/lib.rs", + "#![cfg_attr(test, deny(clippy::disallowed_methods))]\n", + &[] +)] +// A group name containing `allow` is not itself an `allow` attribute. +#[case::lint_name_containing_allow( + "src/lib.rs", + "#[cfg_attr(all(), expect(clippy::allow_attributes, reason = \"probe\"))]\nfn probe() {}\n", + &[] +)] +fn the_scan_reports_the_expected_lints( + #[case] path: &str, + #[case] source: &str, + #[case] expected_lints: &[&str], +) -> Result<()> { + let findings = scan_source(path, source); + ensure!( + findings == expected_findings(path, expected_lints), + "expected {expected_lints:?} in {path}, got {findings:?}" + ); + Ok(()) +} diff --git a/tests/env_access_suppressions/spelling_tests.rs b/tests/env_access_suppressions/spelling_tests.rs new file mode 100644 index 000000000..4ba551a72 --- /dev/null +++ b/tests/env_access_suppressions/spelling_tests.rs @@ -0,0 +1,230 @@ +//! Self-tests for the spellings an attribute may be written in. +//! +//! `mask` blanks comments and literals, so what remains is code and the scan can +//! match an attribute token by token rather than by the shape of the line it +//! sits on. These cases pin the spellings that matter: `rustc` accepts each of +//! them, `#[rustfmt::skip]` freezes several past `make check-fmt`, and every one +//! of them silences the policy exactly as the canonical spelling does. Each was +//! measured against a real probe file before it was pinned here, so the set is +//! evidence rather than guesswork. +//! +//! The negative cases sit beside them because the anchor that used to exclude +//! them is gone. Prose that quotes an attribute is still not an attribute — not +//! because of where the line begins, but because quoted text never reaches the +//! matcher at all. +//! +//! `malformed_input_is_not_a_panic` stays a test of its own: it asserts +//! something weaker than the rest — that the matcher returns at all — over +//! inputs that are not shapes so much as the absence of one. + +use super::expected_findings; +use super::scanner::scan_source; +use anyhow::{Result, ensure}; +use rstest::rstest; + +/// The evading spellings, and the innocent ones the anchor used to exclude. +/// +/// An empty list means the source is not an offence. The raw-identifier rows +/// are the dangerous group: they need no `#[rustfmt::skip]`, so `rustfmt` +/// leaves them byte-for-byte and they were reachable on a clean +/// `make check-fmt` run even while the anchor stood. +#[rstest] +// A `#[rustfmt::skip]` freezes the spelling, so the scan must read it anyway. +// `rustfmt` would join the marker to its parenthesis, but the skip attribute +// tells it not to, and `make check-fmt` then passes a file whose attribute is +// split. Measured: the file compiles, the policy is silenced, and both clippy +// exit codes are 0. +#[case::skipped_split_attribute( + "#[rustfmt::skip]\n#[allow\n (clippy::disallowed_methods, reason = \"escape hatch probe\")]\nfn probe() {}\n", + &["clippy::disallowed_methods"] +)] +// A newline between the marker and its parenthesis is the same attribute. +#[case::newline_inside_the_marker( + "#![allow(\nclippy::disallowed_methods, reason = \"escape hatch probe\")]\n", + &["clippy::disallowed_methods"] +)] +// A newline between the `#` and the `[` is legal, and silences the policy. +#[case::newline_before_the_bracket( + "#\n[allow(warnings, reason = \"escape hatch probe\")]\nfn probe() {}\n", + &["warnings"] +)] +// A blank line between the marker and its parenthesis is still one attribute. +// Whitespace between tokens is not limited to a single newline, and a +// `#[rustfmt::skip]` can hold the gap open however wide it likes. +#[case::blank_line_before_the_parenthesis( + "#[rustfmt::skip]\n#[allow\n\n (clippy::disallowed_methods, reason = \"escape hatch probe\")]\nfn probe() {}\n", + &["clippy::disallowed_methods"] +)] +// A raw identifier names the same attribute. +#[case::raw_identifier_attribute_name( + "#![r#allow(clippy::disallowed_methods, reason = \"escape hatch probe\")]\n", + &["clippy::disallowed_methods"] +)] +// A raw identifier names the same lint path. +#[case::raw_identifier_lint_path( + "#![allow(r#clippy::disallowed_methods, reason = \"escape hatch probe\")]\n", + &["clippy::disallowed_methods"] +)] +// Whitespace around a path separator does not rename the lint. +#[case::spaces_around_the_path_separator( + "#![allow(clippy :: disallowed_methods, reason = \"escape hatch probe\")]\n", + &["clippy::disallowed_methods"] +)] +// The deprecated bare name is an alias, and is reported beside its enabler. +// Measured twice: beside `renamed_and_removed_lints` the bare name silences the +// policy at clippy exit 0, and without it the same attribute exits 101. So it +// is the enabler that closes the class and the alias that keeps the pair +// honest, exactly as with the path-qualified spelling. +#[case::deprecated_bare_name_with_enabler( + "#![allow(renamed_and_removed_lints, disallowed_methods, reason = \"escape hatch probe\")]\n", + &["renamed_and_removed_lints", "disallowed_methods"] +)] +// `unknown_lints` cannot suppress anything, so it is not banned. It looks as +// though it belongs in the set — it hides the report that a name does not exist +// — but measurement says a misspelled name is a no-op either way, so allowing +// the report silences nothing. A rule the code cannot justify is worse than an +// absent one; this row is what keeps the entry from being added back on the +// strength of a plausible-sounding rationale. The test is "silences nothing", +// not "does not reach the policy lint": the latter would also excuse the two +// guard lints and `clippy::restriction`, all of which do silence something. +#[case::unknown_lints_enabler_alone( + "#![allow(unknown_lints, reason = \"escape hatch probe\")]\n", + &[] +)] +// An attribute quoted inside a line comment is prose, not code. The line anchor +// used to be what excluded this; the masked text excludes it now, which is what +// lets the matcher read attributes split across lines. +#[case::attribute_in_a_line_comment( + "let probe = 1; // #[allow(warnings, reason = \"quoted example\")]\n", + &[] +)] +// A raw string's closing hashes are part of its delimiter rather than a `#` in +// code, but masking is what has to say so. Indexing the literal puts those +// hashes directly before a `[`, and that pair is token-for-token the opening of +// an attribute: `&r#"abc"#[allow(warnings)]` is the literal `r#"abc"#` indexed +// by a call to a function named `allow`, and it compiles and runs with `allow` +// and `warnings` in scope as ordinary items. Measured at one false finding +// before the closing delimiter was blanked with the body. +#[case::raw_string_closing_hashes_before_an_index( + "fn probe() { let s: &str = &r#\"abc\"#[allow(warnings)]; }\n", + &[] +)] +// The escaped form carries no equivalent hazard, and the asymmetry is measured +// rather than assumed: a `#` there can only sit inside the body, which is +// already blanked, and the closing `"` left in place cannot open a marker. +#[case::escaped_string_before_an_index( + "fn probe() { let s: &str = &\"abc\"[allow(warnings)]; }\n", + &[] +)] +// The blanking is the delimiter and not the code behind it, so a real inner +// attribute after a raw string is still read. Blanking one token too many would +// make this row pass while the contract went blind, which is why the pair is +// asserted together. +#[case::real_attribute_after_a_raw_string( + "fn probe() { let s = r#\"abc\"#; }\n#[allow(warnings, reason = \"escape hatch probe\")]\nfn other() {}\n", + &["warnings"] +)] +// A `cfg_attr` whose wrapped body holds the `allow` is read whole. +#[case::wrapped_cfg_attr_allow( + "#[cfg_attr(\n all(),\n allow(clippy::disallowed_methods, reason = \"escape hatch probe\"),\n)]\nfn probe() {}\n", + &["clippy::disallowed_methods"] +)] +// An `#[expect]` carrying the policy lint is the sanctioned form, not an +// offence. The seam taxonomy asks for an `expect` with a reason precisely so +// that the suppression is tied to a site that still exists. The scan must not +// read it as an `allow`, and `.expect(...)` method calls must not be read at +// all. +#[case::expect_carrying_the_policy_lint( + "#![expect(clippy::disallowed_methods, reason = \"sanctioned site\")]\n\ + fn probe() {\n\ + \x20 let value = std::env::var(\"X\");\n\ + \x20 assert!(value.is_err());\n\ + }\n", + &[] +)] +// A `warn` of the policy lint is not read, and this row is what pins that the +// omission is deliberate. It lowers the lint from the workspace's `deny` to +// `warn` — bare `cargo clippy` exits 0 — but every lint target passes +// `-D warnings`, which re-promotes it: measured at exit 101 under the gate's +// flags. Reporting a shape that cannot pass a gate would be a rule the code +// cannot justify, the same reasoning that leaves `unknown_lints` out of the +// banned set. +#[case::warn_of_the_policy_lint_alone( + "#![warn(clippy::disallowed_methods)]\n", + &[] +)] +// ... and the same holds for the item-level and `cfg_attr`-wrapped spellings, +// so the omission is about the `warn` marker rather than one layout. +#[case::warn_wrapped_in_a_cfg_attr( + "#![cfg_attr(all(), warn(clippy::disallowed_methods))]\n", + &[] +)] +// The pair that *does* escape the gate's flags, and the reason `warnings` +// stays in the banned set. The `warn` lowers the policy lint to `warn`, which +// is what puts it *into* the `warnings` group — the group is the set of lints +// currently at `warn`, not a parent of the hierarchy — and the `allow` then +// suppresses that group. Measured at exit 0 under `RUSTFLAGS=-D warnings`, in +// either order, where neither half escapes alone. The scan catches it on the +// `allow` half, which is the only half it can see. +#[case::warn_of_the_policy_lint_beside_allow_warnings( + "#![warn(clippy::disallowed_methods)]\n\ + #![allow(warnings, reason = \"escape hatch probe\")]\n", + &["warnings"] +)] +fn the_scan_reads_each_spelling_the_same_way( + #[case] source: &str, + #[case] expected_lints: &[&str], +) -> Result<()> { + let findings = scan_source("src/lib.rs", source); + ensure!( + findings == expected_findings("src/lib.rs", expected_lints), + "expected {expected_lints:?}, got {findings:?} for {source:?}" + ); + Ok(()) +} + +/// Malformed input yields findings or silence, never a panic. +/// +/// The matcher reads bytes and slices at the offsets it derives from them, so +/// the shapes that could send it out of bounds — a `#` with nothing behind it, +/// a marker that never closes, a stray byte that is not a character boundary — +/// are pinned here. A panic would be a worse failure than a finding: it would +/// take the gate down rather than report it. +#[test] +fn malformed_input_is_not_a_panic() -> Result<()> { + let cases = [ + "#", + "#!", + "#[", + "#![]", + "#[]", + "#(", + "#!(", + "#[allow(", + "#[allow(warnings", + "#![allow(clippy::disallowed_methods", + "#[cfg_attr(all(),", + "#[cfg_attr(all(), allow(cfg_attr_allow_is_unterminated", + "const C: &str = \"unterminated", + // An escape that opens a char literal and never closes it: the escape + // reader looks for a `}` that never comes, and must return nothing + // rather than run off the end. + "const C: char = '\\u{61", + "/* unterminated comment\n#[allow(warnings, reason = \"x\")]", + "let \u{00e9} = 1; #[allow(warnings, reason = \"non-ascii before\")]", + "#\u{00e9}[allow(warnings, reason = \"non-ascii after\")]", + "#[allow(warnings, reason = \"\u{1f600}\")]", + ]; + for case in cases { + // The assertion is that this returns at all; each case is malformed or + // harmless, so any finding is acceptable and a panic is not. + let findings = scan_source("src/lib.rs", case); + ensure!( + findings + .iter() + .all(|(path, lint)| path == "src/lib.rs" && !lint.is_empty()), + "a finding should name a path and a lint, got {findings:?} for {case:?}" + ); + } + Ok(()) +} diff --git a/tests/env_access_suppressions/walk_tests.rs b/tests/env_access_suppressions/walk_tests.rs new file mode 100644 index 000000000..467be9374 --- /dev/null +++ b/tests/env_access_suppressions/walk_tests.rs @@ -0,0 +1,325 @@ +//! Self-tests for the workspace walk behind the coverage invariant. +//! +//! The invariant above it asserts that every Rust source in the workspace is +//! scanned, and it can only assert that about the sources its walk reaches. So +//! the walk's own reach is pinned here, against a synthetic tree rather than +//! against the repository: a shape the repository does not happen to contain +//! today is exactly the shape that would go unnoticed if the test read the real +//! tree. +//! +//! The same reasoning governs the skip list's own justification, which is +//! checked against a scratch repository holding the repository's `.gitignore` +//! rather than against the working tree. A working tree answers with more than +//! the repository's rules — nested tools write ignore files of their own — so +//! asking it would make the answer depend on which tools had run. + +use super::is_scanned; +use super::roots::{MACHINE_LOCAL_DIRECTORIES, collect_all_sources, collect_rust_sources}; +use anyhow::{Context, Result, bail, ensure}; +use camino::Utf8Path; +use cap_std::{ambient_authority, fs_utf8::Dir}; +use tempfile::tempdir; + +/// Fail if an `.rs` file under a non-cache dot-directory is walked past. +/// +/// This is the shape the machine-local list must not swallow. A dot-prefix is +/// not evidence of a cache — `.config`, `.github`, and `.rules` are tracked +/// content, and a manifest can declare a target under any directory, hidden or +/// not — so a walk that skipped every dot-prefixed name would neither scan a +/// target in one nor report it, which is exactly the silent non-coverage the +/// invariant exists to prevent. The tree is synthetic so the assertion does not +/// depend on what this repository happens to contain today: a cache directory +/// and `target` still hold nothing, and a hidden directory holds a source that +/// must be named. +#[test] +fn a_source_under_a_dot_directory_is_walked_and_reported() -> Result<()> { + let scratch = tempdir().context("create a scratch directory for the walk")?; + let scratch_path = Utf8Path::from_path(scratch.path()) + .context("a temporary directory path should be valid UTF-8")?; + let root = Dir::open_ambient_dir(scratch_path, ambient_authority()) + .context("open the scratch directory")?; + for directory in [".hidden", "src", ".uv-cache", "target"] { + root.create_dir_all(directory) + .with_context(|| format!("create `{directory}`"))?; + } + root.write(".hidden/probe.rs", b"fn probe() {}\n") + .context("write the hidden probe")?; + root.write("src/kept.rs", b"fn kept() {}\n") + .context("write the kept source")?; + root.write(".uv-cache/vendored.rs", b"fn vendored() {}\n") + .context("write the cached source")?; + root.write("target/generated.rs", b"fn generated() {}\n") + .context("write the generated source")?; + + let mut found = Vec::new(); + collect_all_sources(&root, Utf8Path::new("."), &mut found)?; + found.sort(); + + ensure!( + found == [".hidden/probe.rs", "src/kept.rs"], + "the walk should descend a non-cache dot-directory and skip the machine-local ones, \ + got {found:?}" + ); + // The walk finding it is not enough; the invariant must also name it, which + // is what fails the gate rather than silently excusing the source. + ensure!( + !is_scanned(".hidden/probe.rs"), + "a source under a dot-directory is outside the scanned roots, so the invariant \ + must report it" + ); + Ok(()) +} + +/// Fail if a skipped name is only skipped at the workspace root. +/// +/// The skip is keyed on the entry name rather than on a workspace-relative +/// path, because that is the rule `.gitignore` states: its patterns carry no +/// leading slash, so `target/` and `memories/` are ignored at every depth. The +/// distinction is not cosmetic — a walk that skipped `target` only at the root +/// would descend a nested one, and if a nested `target` ever held a generated +/// `.rs` file the gate would turn on the compiler's output, which is the thing +/// it must not do. The tree is synthetic so the assertion does not depend on +/// the repository happening to have no nested cache today. +#[test] +fn a_machine_local_name_is_skipped_at_any_depth() -> Result<()> { + let scratch = tempdir().context("create a scratch directory for the walk")?; + let scratch_path = Utf8Path::from_path(scratch.path()) + .context("a temporary directory path should be valid UTF-8")?; + let root = Dir::open_ambient_dir(scratch_path, ambient_authority()) + .context("open the scratch directory")?; + for directory in ["tools/memories", "vendor/target", "src"] { + root.create_dir_all(directory) + .with_context(|| format!("create `{directory}`"))?; + } + root.write("tools/memories/probe.rs", b"fn probe() {}\n") + .context("write the nested memories source")?; + root.write("vendor/target/generated.rs", b"fn generated() {}\n") + .context("write the nested target source")?; + root.write("src/kept.rs", b"fn kept() {}\n") + .context("write the kept source")?; + + let mut found = Vec::new(); + collect_all_sources(&root, Utf8Path::new("."), &mut found)?; + found.sort(); + + ensure!( + found == ["src/kept.rs"], + "the skip is by entry name, so a nested machine-local directory is skipped too; \ + got {found:?}" + ); + Ok(()) +} + +/// Fail if a skipped name is ignored only by a cache's own ignore file. +/// +/// The skip is justified by an appeal to `.gitignore`: a name git will not +/// track is not one a compiled source can live under, so skipping it cannot +/// hide anything. That appeal is only sound while it is *true* for every name in +/// the list, and it was not — `.netsuke` was skipped while `git check-ignore` +/// declined it, so a `.rs` file placed there would have been tracked, compiled, +/// skipped by the walk, and reported by nobody. That is the precise silent +/// non-coverage this invariant exists to prevent, and it arrived through the +/// list rather than through the walk. +/// +/// The question is asked of the *repository's* rules rather than of the working +/// tree, because a working tree answers with more than those: `git check-ignore` +/// also reads ignore files that nested tools write. Ruff drops a `.gitignore` +/// holding `*` into `.ruff_cache` as a side effect of running, so asking the +/// live tree made `.ruff_cache` pass on a machine where ruff had run and fail on +/// a fresh clone — and `make test` can precede `make lint`, so the answer would +/// have depended on the gate order. The rule has to hold on every checkout, so +/// the repository's `.gitignore` is copied into a scratch repository and the +/// question is put there. Nothing under the workspace is touched, and the answer +/// no longer depends on which tools have run or on whether the sources are a +/// checkout at all, which is also why no `git rev-parse` guard is needed for the +/// copies cargo-mutants makes: this test brings its own repository. +/// +/// The machine's own git configuration is a third source of answers and is +/// switched off for the same reason. A contributor with a global ignore file +/// listing a name here would otherwise see the test pass while the repository +/// says nothing about that name, which is the original defect wearing a +/// different hat. An empty `core.excludesFile` covers both spellings a global +/// ignore can take: it overrides a configured path, and it also suppresses the +/// default `~/.config/git/ignore`, measured against both. One flag is enough: +/// an earlier version of this comment named a second key for the default path, +/// and that key does not exist in git. +/// +/// A template directory is a fourth, and it is closed at `git init` above +/// rather than here, because the `info/exclude` it seeds is written before this +/// runs and no later call could unpin it. +/// +/// Git's own environment variables are a fifth, and they are the one route that +/// does not go through a file. `GIT_DIR` repoints git at another repository's +/// metadata, so `check-ignore` answers from that repository instead of the +/// scratch one: measured at a false pass, where the hostile repository's +/// `info/exclude` held `*` and the honest answer was "not ignored". Both calls +/// clear `GIT_DIR`, `GIT_WORK_TREE`, and `GIT_COMMON_DIR`, which is why the +/// helper below removes them as well as the caller above. +/// +/// `.git` is the one legitimate exception: git refuses to track anything +/// beneath it whatever the ignore files say, so the appeal still holds even +/// though `check-ignore` reports it as unignored. It is named here rather than +/// excluded by a pattern, so a future name added to the list without an ignore +/// rule is caught rather than grandfathered in. +#[test] +fn every_skipped_name_is_one_git_would_not_track() -> Result<()> { + let root = Dir::open_ambient_dir(env!("CARGO_MANIFEST_DIR"), ambient_authority()) + .context("open the workspace root")?; + let scratch = tempdir().context("create a scratch repository")?; + let scratch_path = Utf8Path::from_path(scratch.path()) + .context("a temporary directory path should be valid UTF-8")?; + let scratch_root = Dir::open_ambient_dir(scratch_path, ambient_authority()) + .context("open the scratch directory")?; + let ignore_rules = root + .read(".gitignore") + .context("read the repository's `.gitignore`")?; + scratch_root + .write(".gitignore", ignore_rules) + .context("copy the repository's `.gitignore` into the scratch repository")?; + // `--template=` is what makes the scratch repository answer from the + // `.gitignore` alone. A template directory can hold an `info/exclude`, and + // git writes it into the new repository, where `check-ignore` reads it; + // measured at a false pass once seeded. An empty value suppresses the + // template, and it is the only form that does: `GIT_TEMPLATE_DIR` outranks + // a `-c init.templateDir=` given to the same command, measured, so a + // contributor with that variable set would otherwise see the false pass + // survive. + // + // The flag belongs here rather than on `check-ignore` because + // `info/exclude` is written at *init* time, so there is no later call that + // could unpin it. That is the whole of its job, and it is worth stating + // narrowly: a template can seed `.git/config` too, but the helper's own + // `-c core.excludesFile=` already neutralizes an ignore file configured + // there — measured with a template seeding only `.git/config`, unpinned and + // pinned both leaving the name unignored. An earlier version of this + // comment claimed the config half as well and was wrong about it. + let init = std::process::Command::new("git") + .args(["init", "--quiet", "--template="]) + .env_remove("GIT_DIR") + .env_remove("GIT_WORK_TREE") + .env_remove("GIT_COMMON_DIR") + .current_dir(scratch_path) + .status() + .context("run git init in the scratch repository")?; + ensure!(init.success(), "git init failed in the scratch repository"); + + let mut unexplained = Vec::new(); + for name in MACHINE_LOCAL_DIRECTORIES { + if name == ".git" { + continue; + } + if !is_ignored(scratch_path, name)? { + unexplained.push(name); + } + } + ensure!( + unexplained.is_empty(), + "these names are skipped by the walk, but the repository's own `.gitignore` does \ + not ignore them, so git would track a source under them and the walk would hide \ + it rather than report it; add each to `.gitignore` beside its sibling caches, or \ + reconsider the skip: {unexplained:?}" + ); + Ok(()) +} + +/// Fail if a cache inside a scanned root is read rather than skipped. +/// +/// The scan descends a root list, and a scan is not a walk of the repository: +/// a root covers nested directory names too, so the roots shipped without any +/// skip rule and a `.uv-cache` under `tests/` would have been read. That is how +/// the scan and this walk came apart — the walk has always skipped by name, so +/// the cache was invisible to it and ungoverned by it, while the scan read it. +/// Measured before the fix: a vendored source carrying the banned `allow` under +/// `tests/.uv-cache/` failed the scan contract, which is red on a machine where +/// a tool had run and green on a fresh clone, with the offending path in no +/// diff and in no `git status` because the name is git-ignored. A verdict that +/// depends on a machine is worse than no verdict, so the two walks must agree. +/// +/// The tree is synthetic for the usual reason: the disagreement needs a cache +/// inside a scanned root, and the repository has none today, which is the shape +/// that would otherwise go unnoticed until a contributor's tooling created one. +#[test] +fn a_cache_inside_a_scanned_root_is_skipped_by_the_scan() -> Result<()> { + let scratch = tempdir().context("create a scratch directory for the walk")?; + let scratch_path = Utf8Path::from_path(scratch.path()) + .context("a temporary directory path should be valid UTF-8")?; + let root = Dir::open_ambient_dir(scratch_path, ambient_authority()) + .context("open the scratch directory")?; + for directory in ["tests/.uv-cache", "tests/nested/target", "tests"] { + root.create_dir_all(directory) + .with_context(|| format!("create `{directory}`"))?; + } + root.write("tests/.uv-cache/vendored.rs", b"fn vendored() {}\n") + .context("write the cached source")?; + root.write("tests/nested/target/generated.rs", b"fn generated() {}\n") + .context("write the nested generated source")?; + root.write("tests/kept.rs", b"fn kept() {}\n") + .context("write the kept source")?; + + let mut read = Vec::new(); + collect_rust_sources(&root, Utf8Path::new("tests"), &mut read)?; + let mut scanned: Vec<&str> = read.iter().map(|(path, _)| path.as_str()).collect(); + scanned.sort_unstable(); + + let mut walked = Vec::new(); + collect_all_sources(&root, Utf8Path::new("."), &mut walked)?; + walked.sort(); + + // `collect_rust_sources` is the function the scan actually reads through, so + // this is the assertion the missing skip failed. Asserting on + // `collect_all_sources` alone would not have caught it: that walk has always + // skipped by name, so it was the one behaving correctly. + ensure!( + scanned == ["tests/kept.rs"], + "the scan must skip a machine-local name inside a scanned root, at any \ + depth, or its verdict depends on which tools have run on this machine; \ + got {scanned:?}" + ); + // And the two walks agree, which is the property that keeps the coverage + // invariant meaningful: it reports a source as ungoverned only when the scan + // really would not read it. + ensure!( + walked == scanned, + "the scan and the workspace walk must agree on what is governed, or the \ + coverage invariant excuses a source the scan reads or reports one it \ + does not; scan {scanned:?}, walk {walked:?}" + ); + Ok(()) +} + +/// Return whether the repository at `root` ignores a source under `name`. +/// +/// The machine's global ignore file is disabled first, so the answer comes from +/// the repository copied into `root` and from nothing else. `core.excludesFile` +/// is the only key that needs setting: an empty value overrides a configured +/// path and equally suppresses the default `~/.config/git/ignore`, so one flag +/// covers both ways a contributor's machine can answer for the repository. +/// Measured against both configurations. The value is empty rather than +/// `/dev/null` because a device path is a Unix spelling and this test runs on +/// the Windows lane too; the empty form needs no filesystem path and was +/// measured to behave identically. +/// +/// `check-ignore -q` reports by exit status: 0 ignored, 1 not ignored. Every +/// other status is a real failure and propagates, so "git could not answer" is +/// never read as "git would track this". +fn is_ignored(root: &Utf8Path, name: &str) -> Result { + let status = std::process::Command::new("git") + .args([ + "-c", + "core.excludesFile=", + "check-ignore", + "-q", + &format!("{name}/probe.rs"), + ]) + .env_remove("GIT_DIR") + .env_remove("GIT_WORK_TREE") + .env_remove("GIT_COMMON_DIR") + .current_dir(root) + .status() + .context("run git check-ignore")?; + match status.code() { + Some(0) => Ok(true), + Some(1) => Ok(false), + _ => bail!("git check-ignore failed ({status})"), + } +}