From 01df98aee9aece5c9fa1f212ba1bffde6ff33fd9 Mon Sep 17 00:00:00 2001 From: Abhinav Chordia Date: Wed, 14 Oct 2026 20:04:32 +0000 Subject: [PATCH 1/2] archcheck: block new catch-all crate names (common, utils, core, ...) Add a crate_names rule. Each workspace member's package name, and its library target's name, is lowercased and split on - and _; the crate matches if a whole part is a banned token (common, shared, util, utils, helper, helpers, misc, core), unless `allow` in [rules.crate_names] names the package exactly. `allow` starts empty; its names must be lowercase and unique, and a name both allowed and baselined exits 2. - no_new (required): a matching crate that the new [crate_names] baseline table does not list at its path, reported at the name = (or [lib] name) line of its Cargo.toml; and a [crate_names] entry a pull request may not list, reported at the entry's line. With --base-baseline, that is an entry whose name the base branch's table lacks; updating an existing entry's path is not a new entry. Without a base copy of the table (the base predates the rule, or can't be resolved), only a fixed set compiled into archcheck, the 16 crates that matched when the rule was added, may be listed. A base table that doesn't parse, or a base copy that isn't TOML, counts every entry as added. - stale_entries (advisory): an entry whose crate is gone, renamed or no longer matches, and an allow name that no longer applies. - policy_changed (advisory): with the new --base-architecture flag, every change to [rules.crate_names] compared with the base branch's ARCHITECTURE.toml (read leniently), on its line, so loosening the policy is visible in review. Workspace members now come from `cargo metadata --no-deps --offline`, run from the root, for every rule: Cargo also makes an in-workspace path dependency a member, so a crate added only that way was never checked. If cargo metadata fails, archcheck exits 2. This adds pplx-space-credential-injection-store, whose files are all under the file_size limit, so file_size findings are unchanged; pattern_ratchet reads the same inventory and finds nothing new either. The baseline lists the 16 crates that match today, in a [crate_names] table after the other tables. --write-baseline never adds an entry: it removes stale ones, leaves an entry whose crate moved for a deliberate edit, lists no entries to paste (a hand-added one fails no_new), and always writes the table, even when empty. --write-baseline=crate_names updates that table alone. crate_names fits the shared baseline structure of the count ratchet: it implements Rule directly, not CountRule, since its table maps names to member paths rather than capping a count. Rule::tables names its crate_names table, so the configured-table check and --write-baseline= accept it, and Rule::baseline_rule makes Baseline::load_base read it: the base reader takes the [crate_names] key from the parsed document on its own, beside [file_size] and each [pattern_ratchet.] table, and records it as read. The in-memory Baseline keeps it beside the count tables and writes it last. NotAdded's path and count become an optional entry, so the crate_names lines offer nothing to paste. Rule::other_skips reports the --base-architecture skip line. Config is fail-closed: an empty token list, or a token that is empty, uppercase or contains - or _, exits 2. Module names are out of scope; the README explains why. --- ARCHITECTURE.toml | 15 + architecture-baseline.toml | 20 + crates/tools/archcheck/README.md | 128 +- crates/tools/archcheck/src/baseline.rs | 172 ++- crates/tools/archcheck/src/baseline/tests.rs | 17 +- crates/tools/archcheck/src/config.rs | 218 ++- crates/tools/archcheck/src/context.rs | 267 +++- crates/tools/archcheck/src/finding.rs | 4 +- crates/tools/archcheck/src/lib.rs | 30 +- crates/tools/archcheck/src/main.rs | 43 +- crates/tools/archcheck/src/rules.rs | 23 +- .../tools/archcheck/src/rules/crate_names.rs | 462 ++++++ crates/tools/archcheck/src/rules/ratchet.rs | 3 +- .../archcheck/src/rules/ratchet/tests.rs | 10 +- crates/tools/archcheck/tests/cli.rs | 1289 +++++++++++++++-- 15 files changed, 2450 insertions(+), 251 deletions(-) create mode 100644 crates/tools/archcheck/src/rules/crate_names.rs diff --git a/ARCHITECTURE.toml b/ARCHITECTURE.toml index 7f164fbeef..4429232e66 100644 --- a/ARCHITECTURE.toml +++ b/ARCHITECTURE.toml @@ -77,3 +77,18 @@ mode = "required" [rules.pattern_ratchet.rootfs_copy.stale_entries] mode = "required" + +[rules.crate_names] +# A crate matches if a part of its package or library name, lowercased and +# split on `-` and `_`, is one of these. +banned_tokens = ["common", "shared", "util", "utils", "helper", "helpers", "misc", "core"] +allow = [] # exact package names exempt from the rule + +[rules.crate_names.no_new] # no new catch-all crate, and no new baseline entries +mode = "required" + +[rules.crate_names.stale_entries] # entries and allow names that no longer apply +mode = "advisory" + +[rules.crate_names.policy_changed] # changes to this table, compared with the base branch +mode = "advisory" diff --git a/architecture-baseline.toml b/architecture-baseline.toml index dbdcf08606..7c260f3432 100644 --- a/architecture-baseline.toml +++ b/architecture-baseline.toml @@ -139,3 +139,23 @@ "crates/libs/template/mgr/src/service.rs" = 3 "crates/libs/vol/mgr/server/src/staging.rs" = 2 +# [crate_names] maps each workspace package with a catch-all name (a part such +# as `common`, `utils` or `core`) to its member path. Only remove entries: a pull +# request that adds one is reported, and fails the check. +[crate_names] +pplx-space-common-cli = "crates/libs/common/cli" +pplx-space-common-connection-admission = "crates/libs/common/connection/admission" +pplx-space-common-core = "crates/libs/common/core" +pplx-space-common-grpc = "crates/libs/common/grpc" +pplx-space-common-grpc-lb = "crates/libs/common/grpc-lb" +pplx-space-common-io = "crates/libs/common/io" +pplx-space-common-jobs = "crates/libs/common/jobs" +pplx-space-common-lru-reaper = "crates/libs/common/lru-reaper" +pplx-space-common-proto-build = "crates/libs/common/proto-build" +pplx-space-common-svc = "crates/libs/common/svc" +pplx-space-common-work-coalescer = "crates/libs/common/work-coalescer" +pplx-space-credential-injection-core = "crates/libs/credential/injection/core" +pplx-space-execution-common = "crates/libs/execution/common" +pplx-space-net-common = "crates/libs/net/common" +pplx-space-node-runtime-core = "crates/libs/node/runtime/core" +pplx-space-spaced-common = "crates/libs/spaced/common" diff --git a/crates/tools/archcheck/README.md b/crates/tools/archcheck/README.md index 99e58c9d2f..abc0c7cb04 100644 --- a/crates/tools/archcheck/README.md +++ b/crates/tools/archcheck/README.md @@ -6,22 +6,26 @@ tolerates for now are listed in [`architecture-baseline.toml`](../../../architecture-baseline.toml). ```sh -cargo arch # human-readable report -cargo arch --format rdjson # reviewdog Diagnostic Format, for CI -cargo arch --base-baseline # also compare with the base branch's baseline -cargo arch --write-baseline # update the baseline's entries; never adds one -cargo arch --write-baseline=
# update one table's entries, keeping the others -cargo arch --root # check another checkout +cargo arch # human-readable report +cargo arch --format rdjson # reviewdog Diagnostic Format, for CI +cargo arch --base-baseline # also compare with the base branch's baseline +cargo arch --base-architecture # ... and with its ARCHITECTURE.toml +cargo arch --write-baseline # update the baseline's entries; never adds one +cargo arch --write-baseline=
# update one table's entries, keeping the others +cargo arch --root # check another checkout ``` Without `--root`, it checks the Cargo workspace containing the current -directory. Exit status: 0 without required findings, 1 with one, 2 for a -configuration, usage or I/O error. +directory. The workspace members are the ones `cargo metadata --no-deps +--offline` reports, run from the root: the root `Cargo.toml`'s `members`, and +every path dependency inside the workspace, which Cargo makes a member too. If +`cargo metadata` fails, archcheck exits 2. Exit status: 0 without required +findings, 1 with one, 2 for a configuration, usage or I/O error. ## What is checked - **Production Rust file:** a `.rs` file under `/src/` of a workspace - member in the root `Cargo.toml`, except `tests.rs`, `*_tests.rs` and + member, except `tests.rs`, `*_tests.rs` and anything below a `tests/` directory. Symlinks under `src/` are skipped; a member directory or `src` that is itself a symlink is followed. Other files may have non-UTF-8 names; a `.rs` file may not (exit 2). @@ -129,14 +133,73 @@ code changes: entry, since the base branch's copy has no table for the instance: that is expected for a deliberate new instance. +## Crate names + +`crate_names` keeps new crates from getting catch-all names. For each +workspace member, it lowercases the package name and the library target's +name (from `[lib] name`, or Cargo's default from the package name) and splits +them on `-` and `_`. The crate matches if a part of either is in +`banned_tokens`: `common`, `shared`, `util`, `utils`, `helper`, `helpers`, +`misc` and `core`. Only whole parts match, so `pplx-space-common-io`, +`foo_core` and a library named `utils` do, and `database`, `coreutils` and +`encore` don't. An empty token list, or a token that is empty, uppercase or +contains `-` or `_`, can't match anything and exits 2. + +`allow` lists exact package names exempt from the rule, such as a +`pplx-space-shared-memory` whose `shared` is not a catch-all. It starts empty. +Its names must be lowercase and listed once, and a name both allowed and in the +baseline exits 2. + +The baseline's `[crate_names]` table maps each tolerated crate's package name +to its member path. An entry covers only the crate at that path: a crate with a +listed name at another path is reported, until the entry's path is updated, +so a listed crate that moves needs a one-line baseline edit, and a removed +crate's name can't be reused elsewhere. + +- `no_new` (required): a matching crate the baseline doesn't list at its path, + shown on the `name =` line of its `Cargo.toml` (or the `[lib] name` line, if + only the library's name matches); and an entry a pull request may not list, + shown on the entry's line, so it can't list its own new crate. With + `--base-baseline`, entries are compared with the base branch's + `[crate_names]`: an entry whose name it lacks is reported, and updating the + path of an existing entry is not. Without a copy of that table, because the + base predates the rule or wasn't given, only the 16 crates that matched when + the rule was added (`BOOTSTRAP` in `src/rules/crate_names.rs`) may be + listed. A table that doesn't parse counts as empty, with a warning, as does + a copy that isn't TOML at all, under the one warning for that copy. +- `stale_entries` (advisory): an entry whose crate is gone or renamed, or no + longer matches; an `allow` name that no workspace member has, or that + doesn't match. +- `policy_changed` (advisory): every difference in `[rules.crate_names]` from + the base branch's `ARCHITECTURE.toml`, given with `--base-architecture`: a + token added or removed, an `allow` name added or removed, a mode changed, + or the whole table new. Each is shown on its line in `ARCHITECTURE.toml`, so + a loosened policy is visible in review. That copy is read leniently: other + tables and unknown keys are ignored, and one that can't be parsed skips the + behaviour with a warning. Without the flag, it is skipped too, and one line + after the summary says so. + +`--write-baseline` keeps each entry whose crate still matches at the entry's +path, removes the other entries, and never adds one: it prints `not added: …` +for each matching crate without an entry, and `not updated: …` for an entry +whose crate is at another path, which it leaves for a deliberate edit. Unlike +for the other tables, it lists no entries to paste: a hand-added entry fails +`no_new`. It always writes the `[crate_names]` table, even when empty, after +the other tables; `--write-baseline=crate_names` updates it alone. + +Module names aren't checked. Catch-all modules haven't been appearing in the +history, a module's reach is limited to its crate, `file_size` already bounds +how much one can hold, and a check on module names is easy to evade (`util.rs` +becomes `support.rs`) without changing what the module holds. + ## Configuration and modes Each behaviour has a `mode`: `advisory` findings are reported, `required` ones also fail the check, so behaviours can graduate one at a time. Unknown keys, sections or modes, a missing file, bad TOML, an invalid regex, glob or instance name, an empty `patterns` list, a baseline table for an instance that is not -configured, and `members` with globs or with empty, `.` or `..` components -exit 2. So does a `--base-baseline` that can't be read, or one combined with +configured, and a failing `cargo metadata` exit 2. So does a `--base-baseline` +or `--base-architecture` that can't be read, or one combined with `--write-baseline`. ## Baseline @@ -145,15 +208,18 @@ Each table maps a repository-relative path to its cap, the count it had when listed: under `[file_size]`, each production file over the limit, with its code-line count; under `[pattern_ratchet.]`, each production file outside the instance's allowed paths with a matching line, with its count of -matching lines. Entries should only be removed or lowered. +matching lines. `[crate_names]`, written last, maps a package name to its +member path instead (see [Crate names](#crate-names)). Entries should only be +removed or lowered. `cargo arch --write-baseline` rewrites `architecture-baseline.toml` from its entries and the tree. An entry still in violation gets its current count as its cap: under `[file_size]`, a file still over the limit; under `[pattern_ratchet.]`, a file outside the allowed paths that still has a matching line. The other entries are removed. It never adds an entry, or a -`[pattern_ratchet.]` table the file lacks (`[file_size]` is always -written, empty if the file had none). It prints `not added: (N code lines); …` for each +`[pattern_ratchet.]` table the file lacks (`[file_size]` and +`[crate_names]` are always written, empty if the file had none; for +`[crate_names]`, see [Crate names](#crate-names)). It prints `not added: (N code lines); …` for each file over the limit without an entry instead, and `not added: (N code lines matching the patterns); …` for each matching file without one, then, after its summary line, the entries they would need, as TOML, one @@ -168,8 +234,9 @@ caps are written too, and `no_raised_caps` reports each of them. It reads the existing file with the strict parser, so a missing or invalid baseline exits 2: it can't create one from scratch. -`cargo arch --write-baseline=
`, with `
` either `file_size` or -`pattern_ratchet.` for a configured instance, does the same for that +`cargo arch --write-baseline=
`, with `
` `file_size`, +`crate_names` or `pattern_ratchet.` for a configured instance, does +the same for that table alone and keeps every other table as the existing baseline has it, printing only that table's `not added` lines and entries. Use it when adding or changing one rule, so that other tables are left alone. If the file has no @@ -191,15 +258,19 @@ counted too: check `git status` before `--write-baseline`. On pull requests, the Rust clippy job in [`rust-static-ci.yml`](../../../.github/workflows/rust-static-ci.yml) runs -`cargo arch --format rdjson --base-baseline ` and passes the findings to -[reviewdog](https://github.com/reviewdog/reviewdog) with -`-reporter=github-pr-annotations`. The file is the baseline of the synthetic -merge's first parent, the base branch, or an empty file if the base has none. -If the base can't be resolved, the step warns and runs without the flag. Each -finding is a diagnostic with code `.`, or -`..` for a rule with instances, on line 1 of its -file, with the path exactly as it is. A new entry, a raised cap, and a stale -entry for a deleted file point at the entry's line in the baseline instead. +`cargo arch --format rdjson --base-baseline --base-architecture ` +and passes the findings to [reviewdog](https://github.com/reviewdog/reviewdog) +with `-reporter=github-pr-annotations`. The files are the baseline and +`ARCHITECTURE.toml` of the synthetic merge's first parent, the base branch, or +an empty file for one the base lacks. If the base can't be resolved, the step +warns and runs without either flag: the `no_new_entries` and `no_raised_caps` +behaviours and `crate_names.policy_changed` are skipped, and +`crate_names.no_new` allows only the bootstrap entries. Each finding is a +diagnostic with code `.`, or `..` +for a rule with instances, on line 1 of its file, with the path exactly as it +is. A new catch-all crate points at the `name =` line of its `Cargo.toml` +instead, and a new entry, a raised cap, a stale entry for a deleted file and a +stale `[crate_names]` entry at the entry's line in the baseline. `pattern_ratchet`'s `no_crossing`, `listed_no_growth` and shrank `stale_entries` findings point at the file's first matching line; one for a file with no matches left, at its line 1. Required findings are `ERROR`, @@ -230,6 +301,13 @@ advisory ones `WARNING`. most 10 are annotated. Annotations are not the full inventory: every finding is in the step's log and its rdjson output, or run `cargo arch`. +A new catch-all crate's finding is on its own `Cargo.toml`, a file the pull +request adds, so it is annotated in either mode and its `ERROR` fails the job. +The same holds for a crate moved to a listed name's new path or a renamed +library, whose manifest the pull request changes; a self-listed entry or a +changed `allow` changes the baseline or `ARCHITECTURE.toml`, which switches to +`nofilter`. + Only annotations are filtered. Once a behaviour is `required`, `tests/repository.rs` fails on any required finding anywhere in the tree. diff --git a/crates/tools/archcheck/src/baseline.rs b/crates/tools/archcheck/src/baseline.rs index fd75eeea61..a38ee162f8 100644 --- a/crates/tools/archcheck/src/baseline.rs +++ b/crates/tools/archcheck/src/baseline.rs @@ -1,18 +1,19 @@ //! `architecture-baseline.toml`: existing violations a rule tolerates for now. //! -//! Each rule that needs a baseline owns a table: `[file_size]`, and one -//! `[pattern_ratchet.]` per configured instance. `cargo arch -//! --write-baseline` updates and removes the entries of every table, or of one -//! with `--write-baseline=
`, but never adds an entry or creates a -//! `pattern_ratchet` table: those are hand edits, which the base branch -//! comparison then reports. (`[file_size]` is always written, empty if the -//! file had none.) A missing table means the rule tolerates nothing, which can -//! only make checks stricter. The base branch's copy, which a pull request's baseline is +//! Each rule that needs a baseline owns a table: `[file_size]`, one +//! `[pattern_ratchet.]` per configured instance, and +//! `[crate_names]`. `cargo arch --write-baseline` updates and removes the +//! entries of every table, or of one with `--write-baseline=
`, but +//! never adds an entry or creates a `pattern_ratchet` table: those are hand +//! edits, which the base branch comparison then reports. (`[file_size]` and +//! `[crate_names]` are always written, empty if the file had none.) A missing +//! table means the rule tolerates nothing, which can only make checks +//! stricter. The base branch's copy, which a pull request's baseline is //! compared with, is read leniently and table by table, so that a change to //! the schema or a renamed instance does not break the comparison. use std::{ - collections::{BTreeMap, btree_map}, + collections::{BTreeMap, BTreeSet, btree_map}, io, path::Path, path::PathBuf, @@ -27,7 +28,7 @@ use toml::{ use crate::{ context::{LoadError, parse_toml_with_text, read}, - rules::{self, CountRule, FileSize, PatternRatchet}, + rules::{self, CountRule, CrateNames, FileSize, PatternRatchet}, }; #[cfg(test)] @@ -47,24 +48,48 @@ const HEADER: &str = "\ # that adds an entry or raises a cap is reported. "; -/// Every table, by the name `--write-baseline=
` takes: ``, or -/// `.` for a rule with instances. The default is the -/// baseline of an empty file: a `[file_size]` table without entries. +/// Written above `[crate_names]`, which follows the other tables. +const CRATE_NAMES_COMMENT: &str = "\ +# [crate_names] maps each workspace package with a catch-all name (a part such +# as `common`, `utils` or `core`) to its member path. Only remove entries: a pull +# request that adds one is reported, and fails the check. +"; + +/// Every table of caps, by the name `--write-baseline=
` takes: +/// ``, or `.` for a rule with instances; and +/// `[crate_names]`, which maps names to paths. The default is the baseline +/// of an empty file: a `[file_size]` and a `[crate_names]` table without +/// entries. #[derive(Debug, Clone, PartialEq, Eq)] pub struct Baseline { tables: BTreeMap, + /// Package name to its member path, as `cargo metadata` reports it. + /// Written by [`Baseline::write`] after the other tables. + pub crate_names: BTreeMap, + /// The 1-based line of each `[crate_names]` key in the file the baseline + /// was loaded from. + pub crate_names_lines: BTreeMap, } /// One table: repository-relative path to the file's cap, its count when it /// was listed. -#[derive(Debug, Clone, Default, PartialEq, Eq)] -struct Table { - caps: BTreeMap, +#[derive(Debug, Clone, PartialEq, Eq)] +struct Table { + caps: BTreeMap, /// The 1-based line of each key in the file the baseline was loaded /// from, where a finding about the entry itself points. lines: BTreeMap, } +impl Default for Table { + fn default() -> Self { + Self { + caps: BTreeMap::new(), + lines: BTreeMap::new(), + } + } +} + /// The baseline as written, with the position of each key. #[derive(Default, Deserialize)] #[serde(deny_unknown_fields)] @@ -73,6 +98,8 @@ struct BaselineFile { file_size: BTreeMap, usize>, #[serde(default)] pattern_ratchet: BTreeMap, usize>>, + #[serde(default)] + crate_names: BTreeMap, String>, } /// The base branch's copy, for `[file_size]`: other tables and keys ignored. @@ -101,8 +128,12 @@ pub struct BaseTables { /// not. tables: BTreeMap, /// Each rule whose tables were read: `Ok`, or, for a rule with instances - /// whose key is not a table of tables, why none of them can be read. + /// whose key is not a table of tables, or for `[crate_names]`, why none + /// of them can be read. rules: BTreeMap<&'static str, Result<(), Box>>, + /// The package names `[crate_names]` lists; `None` when the copy has no + /// such table, as before the rule existed. + crate_names: Option>, } /// One table of the base branch's copy, or why it can't be compared. @@ -190,6 +221,17 @@ impl BaseBaseline { .flat_map(|tables| tables.tables.keys().map(String::as_str)) } + /// The package names the base branch's `[crate_names]` lists, or `None` + /// if it has no such table. + pub fn crate_names(&self) -> Result>, BaseSkip<'_>> { + let tables = self.tables()?; + match tables.rules.get(CrateNames::NAME) { + None => Err(BaseSkip::NotRead), + Some(Err(unusable)) => Err(BaseSkip::Unusable(unusable)), + Some(Ok(())) => Ok(tables.crate_names.as_ref()), + } + } + fn tables(&self) -> Result<&BaseTables, BaseSkip<'_>> { match self { Self::NotGiven => Err(BaseSkip::NotGiven), @@ -239,7 +281,12 @@ impl Baseline { with_lines(text, table), ); } - Self { tables } + let crate_names = with_lines(text, file.crate_names); + Self { + tables, + crate_names: crate_names.caps, + crate_names_lines: crate_names.lines, + } } /// The base branch's copy at `path`. Only an unreadable file is an @@ -267,18 +314,32 @@ impl Baseline { .map_err(unusable); tables.insert(FileSize::NAME.to_owned(), file_size); read.insert(FileSize::NAME, Ok(())); - let instances = match document - .into_iter() - .find(|(key, _)| key.get_ref() == PatternRatchet::NAME) - { - None => Ok(()), - Some((_, value)) => base_instances(value, &unusable).map(|instances| { - tables.extend(instances.into_iter().map(|(instance, table)| { - (format!("{}.{instance}", PatternRatchet::NAME), table) - })); - }), - }; + let mut instances = Ok(()); + let mut crate_names = Ok(None); + for (key, value) in document { + match key.get_ref().as_ref() { + PatternRatchet::NAME => { + instances = base_instances(value, &unusable).map(|instances| { + tables.extend(instances.into_iter().map(|(instance, table)| { + (format!("{}.{instance}", PatternRatchet::NAME), table) + })); + }); + } + CrateNames::NAME => crate_names = base_crate_names(value).map(Some), + _ => {} + } + } read.insert(PatternRatchet::NAME, instances); + let crate_names = match crate_names { + Ok(names) => { + read.insert(CrateNames::NAME, Ok(())); + names + } + Err(error) => { + read.insert(CrateNames::NAME, Err(unusable(error))); + None + } + }; if let Some(rule) = rules::registered() .iter() .filter_map(|rule| rule.baseline_rule()) @@ -292,21 +353,35 @@ impl Baseline { Ok(BaseBaseline::Loaded(BaseTables { tables, rules: read, + crate_names, })) } /// Every table, by the name `--write-baseline=
` takes, in the /// order they are written: `file_size`, then `pattern_ratchet.` - /// for each instance. + /// for each instance, then `crate_names`. pub fn table_names(&self) -> Vec { - self.tables.keys().cloned().collect() + let tables = self.tables.keys().cloned(); + tables + .chain(std::iter::once(CrateNames::NAME.to_owned())) + .collect() } - /// The table `name` names, if this baseline has it. + /// The table of caps `name` names, if this baseline has it. `crate_names`, + /// which maps names to paths, is not one. pub fn table(&self, name: &str) -> Option<&BTreeMap> { self.tables.get(name).map(|table| &table.caps) } + /// The number of entries in the table `name` names, if this baseline has + /// it. + pub fn table_len(&self, name: &str) -> Option { + if name == CrateNames::NAME { + return Some(self.crate_names.len()); + } + self.table(name).map(BTreeMap::len) + } + /// The 1-based line of `path`'s entry in table `name` of the file this /// baseline was loaded from. pub fn entry_line(&self, name: &str, path: &str) -> Option { @@ -323,6 +398,10 @@ impl Baseline { /// table as it is, and says whether it did: if `fresh` lacks the table, /// nothing changes. pub fn replace_table(&mut self, fresh: &Self, name: &str) -> bool { + if name == CrateNames::NAME { + self.crate_names = fresh.crate_names.clone(); + return true; + } let Some(caps) = fresh.table(name) else { return false; }; @@ -331,11 +410,16 @@ impl Baseline { } /// Writes the header and every table in generated form: sorted keys, one - /// table after another. A baseline loaded from a generated file and - /// written back is byte-identical; hand-added comments, quoting or order - /// are not kept. + /// table after another, `[crate_names]` last after its comment. A + /// baseline loaded from a generated file and written back is + /// byte-identical; hand-added comments, quoting or order are not kept. pub fn write(&self, path: &Path) -> Result<(), WriteBaselineError> { - let text = format!("{HEADER}\n{}", toml::to_string(self)?); + let crate_names = + toml::to_string(&BTreeMap::from([(CrateNames::NAME, &self.crate_names)]))?; + let text = format!( + "{HEADER}\n{}\n{CRATE_NAMES_COMMENT}{crate_names}", + toml::to_string(self)? + ); std::fs::write(path, text).map_err(|source| WriteBaselineError::Write { path: path.to_path_buf(), source, @@ -368,13 +452,20 @@ fn base_instances( .collect()) } +/// The package names the base copy's `[crate_names]` lists; its values are +/// ignored. +fn base_crate_names(value: Spanned>) -> Result, toml::de::Error> { + BTreeMap::::deserialize(ValueDeserializer::from(value)) + .map(|names| names.into_keys().collect()) +} + /// The rule a table name belongs to: `` of `.`. pub(crate) fn rule_of(table: &str) -> &str { table.split_once('.').map_or(table, |(rule, _)| rule) } -/// A table's caps, and the 1-based line of each key in `text`. -fn with_lines(text: &str, entries: BTreeMap, usize>) -> Table { +/// A table's values, and the 1-based line of each key in `text`. +fn with_lines(text: &str, entries: BTreeMap, V>) -> Table { let mut table = Table::default(); for (key, cap) in entries { let line = line_of(text, key.span().start); @@ -388,7 +479,8 @@ fn with_lines(text: &str, entries: BTreeMap, usize>) -> Table { /// The tables as the file has them: `[]`, then each /// `[.]` under the rule's key, which is left out if the rule /// has no tables. A rule can't have both: TOML could not tell its entries -/// from its instances. +/// from its instances. `[crate_names]` is not among them: [`Baseline::write`] +/// writes it last, after its comment. impl Serialize for Baseline { fn serialize(&self, serializer: S) -> Result { #[derive(Serialize)] @@ -430,6 +522,6 @@ fn both_forms(rule: &str) -> E { } /// The 1-based line of byte `offset` in `text`. -fn line_of(text: &str, offset: usize) -> usize { +pub(crate) fn line_of(text: &str, offset: usize) -> usize { text.get(..offset).unwrap_or_default().matches('\n').count() + 1 } diff --git a/crates/tools/archcheck/src/baseline/tests.rs b/crates/tools/archcheck/src/baseline/tests.rs index 7259ef301d..c833cc1dde 100644 --- a/crates/tools/archcheck/src/baseline/tests.rs +++ b/crates/tools/archcheck/src/baseline/tests.rs @@ -3,8 +3,8 @@ use std::fs; use super::*; use crate::{config::ArchitectureConfig, rules}; -/// `file_size` and two `pattern_ratchet` instances, every behaviour advisory: -/// a table of every registered rule with a baseline. +/// `file_size`, two `pattern_ratchet` instances and `crate_names`, every +/// behaviour advisory: a table of every registered rule with a baseline. fn config() -> ArchitectureConfig { let behaviours = |table: &str| -> String { [ @@ -24,8 +24,13 @@ fn config() -> ArchitectureConfig { behaviours(&format!("pattern_ratchet.{name}")) ) }; + let crate_names: String = ["no_new", "stale_entries", "policy_changed"] + .iter() + .map(|behaviour| format!("[rules.crate_names.{behaviour}]\nmode = \"advisory\"\n")) + .collect(); let text = format!( - "[rules.file_size]\nlimit = 5\n{}{}{}", + "[rules.file_size]\nlimit = 5\n{}{}{}\ + [rules.crate_names]\nbanned_tokens = [\"common\"]\nallow = []\n{crate_names}", behaviours("file_size"), instance("alpha"), instance("beta"), @@ -59,6 +64,12 @@ fn every_configured_table_is_read_from_a_base_copy() { let base = Baseline::load_base(&path).expect("every registered rule's tables are read"); for (cap, table) in tables.iter().enumerate() { + // `[crate_names]` maps names to paths: only its names are read. + if table == CrateNames::NAME { + let names = BTreeSet::from([format!("{table}.rs")]); + assert_eq!(base.crate_names().ok(), Some(Some(&names)), "{table}"); + continue; + } let entries = base.table(table).ok(); let expected = BTreeMap::from([(format!("{table}.rs"), cap)]); assert_eq!(entries, Some(&expected), "{table}"); diff --git a/crates/tools/archcheck/src/config.rs b/crates/tools/archcheck/src/config.rs index b765c14a22..5251f1d9da 100644 --- a/crates/tools/archcheck/src/config.rs +++ b/crates/tools/archcheck/src/config.rs @@ -4,12 +4,23 @@ //! Every table denies unknown keys so a typo or a section for a rule that does //! not exist is a configuration error rather than a silently skipped check. -use std::{borrow::Borrow, collections::BTreeMap, fmt}; +use std::{ + borrow::Borrow, + collections::{BTreeMap, BTreeSet}, + fmt, + path::Path, +}; use globset::{GlobBuilder, GlobSet, GlobSetBuilder}; use regex::RegexSet; use serde::Deserialize; use thiserror::Error; +use toml::Spanned; + +use crate::{ + baseline::{UnusableBase, line_of}, + context::{LoadError, read}, +}; pub const CONFIG_FILE: &str = "ARCHITECTURE.toml"; @@ -27,6 +38,7 @@ pub struct RulesConfig { /// `[rules.pattern_ratchet.]` tables; none when absent. #[serde(default)] pub pattern_ratchet: BTreeMap, + pub crate_names: CrateNamesConfig, } #[derive(Debug, Clone, Deserialize)] @@ -157,6 +169,210 @@ impl TryFrom> for AllowList { } } +#[derive(Debug, Clone, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct CrateNamesConfig { + pub banned_tokens: BannedTokens, + pub allow: AllowedNames, + pub no_new: BehaviourConfig, + pub stale_entries: BehaviourConfig, + pub policy_changed: BehaviourConfig, +} + +/// Parts of a package name, split on `-` and `_`, that make it a catch-all +/// name. Each can match a part: non-empty, lowercase, without `-` or `_`. +#[derive(Debug, Clone, Deserialize)] +#[serde(try_from = "Vec")] +pub struct BannedTokens(Vec); + +#[derive(Debug, Error)] +pub enum BannedTokensError { + #[error("banned_tokens is empty, so crate_names would check nothing")] + Empty, + #[error( + "banned token {0:?} can never match: tokens are matched against the parts of a \ + lowercased name split on `-` and `_`, so each must be non-empty, lowercase, and \ + without `-` or `_`" + )] + Unmatchable(String), +} + +impl TryFrom> for BannedTokens { + type Error = BannedTokensError; + + fn try_from(tokens: Vec) -> Result { + if tokens.is_empty() { + return Err(BannedTokensError::Empty); + } + if let Some(token) = tokens.iter().find(|token| { + token.is_empty() || token.contains(['-', '_']) || token.to_lowercase() != **token + }) { + return Err(BannedTokensError::Unmatchable(token.clone())); + } + Ok(Self(tokens)) + } +} + +impl BannedTokens { + pub fn contains(&self, part: &str) -> bool { + self.0.iter().any(|token| token == part) + } +} + +/// Exact package names exempt from `crate_names` although a part is banned. +#[derive(Debug, Clone, Deserialize)] +#[serde(try_from = "Vec")] +pub struct AllowedNames(BTreeSet); + +#[derive(Debug, Error)] +pub enum AllowedNamesError { + #[error("allow entry {0:?} is not a lowercase package name")] + NotLowercase(String), + #[error("allow lists {0:?} twice")] + Duplicate(String), +} + +impl TryFrom> for AllowedNames { + type Error = AllowedNamesError; + + fn try_from(names: Vec) -> Result { + let mut allow = BTreeSet::new(); + for name in names { + if name.is_empty() || name.to_lowercase() != name { + return Err(AllowedNamesError::NotLowercase(name)); + } + if allow.contains(&name) { + return Err(AllowedNamesError::Duplicate(name)); + } + allow.insert(name); + } + Ok(Self(allow)) + } +} + +impl AllowedNames { + pub fn contains(&self, name: &str) -> bool { + self.0.contains(name) + } + + pub fn iter(&self) -> impl Iterator { + self.0.iter() + } +} + +/// `[rules.crate_names]` as `crate_names.policy_changed` compares it: each +/// part, and the 1-based line it is written on. Read leniently, so that the +/// base branch's copy can have another schema. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct CrateNamesPolicy { + /// The line of the `banned_tokens` key, if any. + pub banned_tokens_line: Option, + pub banned_tokens: BTreeMap, + /// The line of the `allow` key, if any. + pub allow_line: Option, + pub allow: BTreeMap, + /// Behaviour name to its mode, as written, and the mode's line. + pub modes: BTreeMap, +} + +#[derive(Deserialize)] +struct PolicyFile { + #[serde(default)] + rules: PolicyRules, +} + +#[derive(Default, Deserialize)] +struct PolicyRules { + crate_names: Option, +} + +/// The base branch's `[rules.crate_names]`, given with `--base-architecture`. +#[derive(Debug, Default)] +pub enum BaseArchitecture { + #[default] + NotGiven, + /// Not TOML, or a `[rules.crate_names]` of another shape. + Unusable(UnusableBase), + /// `None` when the copy has no `[rules.crate_names]`. + Loaded(Option), +} + +/// `flatten` and `untagged` would lose the spans, so each behaviour is a +/// field. +#[derive(Deserialize)] +struct PolicyTable { + banned_tokens: Option>>>, + allow: Option>>>, + no_new: Option, + stale_entries: Option, + policy_changed: Option, +} + +#[derive(Deserialize)] +struct PolicyBehaviour { + mode: Option>, +} + +impl CrateNamesPolicy { + /// `[rules.crate_names]` of `text`, or `None` without one. Other tables + /// and unknown keys are ignored. + pub fn parse(text: &str) -> Result, toml::de::Error> { + let file: PolicyFile = toml::from_str(text)?; + let Some(table) = file.rules.crate_names else { + return Ok(None); + }; + let line = |span: std::ops::Range| line_of(text, span.start); + let list = |list: Option>>>| match list { + None => (None, BTreeMap::new()), + Some(list) => ( + Some(line(list.span())), + list.into_inner() + .into_iter() + .map(|item| (item.get_ref().clone(), line(item.span()))) + .collect(), + ), + }; + let (banned_tokens_line, banned_tokens) = list(table.banned_tokens); + let (allow_line, allow) = list(table.allow); + let modes = [ + ("no_new", table.no_new), + ("stale_entries", table.stale_entries), + ("policy_changed", table.policy_changed), + ] + .into_iter() + .filter_map(|(behaviour, entry)| { + let mode = entry?.mode?; + Some(( + behaviour.to_owned(), + (mode.get_ref().clone(), line(mode.span())), + )) + }) + .collect(); + Ok(Some(Self { + banned_tokens_line, + banned_tokens, + allow_line, + allow, + modes, + })) + } + + /// The base branch's `ARCHITECTURE.toml` at `path`, given with + /// `--base-architecture`. Only an unreadable file is an error; one that + /// does not parse is [`BaseArchitecture::Unusable`]. + pub fn load_base(path: &Path) -> Result { + let text = read(path)?; + Ok(match Self::parse(&text) { + Ok(policy) => BaseArchitecture::Loaded(policy), + Err(error) => BaseArchitecture::Unusable(UnusableBase { + path: path.to_path_buf(), + line: error.span().map(|span| line_of(&text, span.start)), + error, + }), + }) + } +} + #[derive(Debug, Clone, Copy, Deserialize)] #[serde(deny_unknown_fields)] pub struct BehaviourConfig { diff --git a/crates/tools/archcheck/src/context.rs b/crates/tools/archcheck/src/context.rs index 451318d2ff..6b3de60200 100644 --- a/crates/tools/archcheck/src/context.rs +++ b/crates/tools/archcheck/src/context.rs @@ -1,26 +1,28 @@ //! Everything a rule may inspect, loaded once before any rule runs. use std::{ + env, ffi::OsStr, io, path::{Path, PathBuf}, + process::{Command, ExitStatus}, }; use serde::Deserialize; use thiserror::Error; +use toml::Spanned; use crate::{ - baseline::{BASELINE_FILE, BaseBaseline, Baseline}, - config::{ArchitectureConfig, CONFIG_FILE}, + baseline::{BASELINE_FILE, BaseBaseline, Baseline, line_of}, + config::{ArchitectureConfig, BaseArchitecture, CONFIG_FILE, CrateNamesPolicy}, rules, }; -/// The root manifest, whose `[workspace] members` decide which files are -/// checked. +/// The root manifest, the workspace `cargo metadata` reads. const ROOT_MANIFEST: &str = "Cargo.toml"; -/// Characters Cargo expands as globs in `[workspace] members`. -const GLOB_CHARACTERS: &[char] = &['*', '?', '[', ']', '{', '}']; +/// Cargo target kinds that build a library crate, whose name code imports. +const LIB_KINDS: &[&str] = &["lib", "rlib", "dylib", "cdylib", "staticlib", "proc-macro"]; #[derive(Debug, Error)] pub enum LoadError { @@ -36,19 +38,31 @@ pub enum LoadError { #[source] source: toml::de::Error, }, - #[error("{path} has no [workspace] members list")] - NoWorkspaceMembers { path: PathBuf }, - #[error( - "{path}: workspace member {member} contains glob characters; list every member explicitly" - )] - GlobMember { path: PathBuf, member: String }, + #[error("could not run `cargo metadata` in {root}: {source}")] + CargoMetadataSpawn { + root: PathBuf, + #[source] + source: io::Error, + }, + #[error("`cargo metadata` failed in {root} ({status}):\n{stderr}")] + CargoMetadataFailed { + root: PathBuf, + status: ExitStatus, + stderr: String, + }, + #[error("could not read the output of `cargo metadata` in {root}: {source}")] + CargoMetadataOutput { + root: PathBuf, + #[source] + source: serde_json::Error, + }, + #[error("workspace member {manifest} is not under the workspace root {root}")] + MemberOutsideRoot { manifest: PathBuf, root: PathBuf }, #[error( - "{path}: workspace member {member:?} has an empty, `.` or `..` component; write it as a \ - plain relative path" + "{name} is in both [rules.crate_names] allow in {CONFIG_FILE} and [crate_names] in \ + {BASELINE_FILE}; an allowed crate needs no entry, so remove one of them" )] - UnnormalizedMember { path: PathBuf, member: String }, - #[error("workspace member {member} has no Cargo.toml at {manifest}")] - MissingMember { member: String, manifest: PathBuf }, + AllowedAndListed { name: String }, /// Debug-formatted, so the invalid bytes are shown escaped. #[error("{path:?} is not valid UTF-8")] NonUtf8Path { path: PathBuf }, @@ -80,16 +94,52 @@ pub struct ProductionFile { pub code_lines: usize, } +/// A workspace member's package. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Crate { + /// `[package] name`. + pub name: String, + /// The name of its library target, which Cargo derives from the package + /// name unless `[lib] name` sets it; `None` without a library. + pub lib: Option, + /// The member's directory relative to the workspace root, `/`-separated; + /// empty for a root package. + pub member: String, + /// The 1-based line of `name` in the member's `Cargo.toml`. + pub name_line: usize, + /// The 1-based line of `[lib] name`, when the manifest sets it. + pub lib_name_line: Option, +} + +impl Crate { + /// Repository-relative, `/`-separated. + pub fn manifest(&self) -> String { + if self.member.is_empty() { + ROOT_MANIFEST.to_owned() + } else { + format!("{}/{ROOT_MANIFEST}", self.member) + } + } +} + #[derive(Debug)] pub struct Context { pub config: ArchitectureConfig, pub baseline: Baseline, /// Behaviours that compare with the base branch's baseline are skipped - /// unless it is [`BaseBaseline::Loaded`]. + /// unless it is [`BaseBaseline::Loaded`]; `crate_names.no_new` is limited + /// instead. pub base_baseline: BaseBaseline, - /// Every production Rust file of the root `Cargo.toml`'s workspace - /// members, sorted by path. + /// `[rules.crate_names]` of `ARCHITECTURE.toml`, with the line of each + /// part. + pub crate_names_policy: CrateNamesPolicy, + /// The same table of the base branch's `ARCHITECTURE.toml`, given with + /// `--base-architecture`. + pub base_architecture: BaseArchitecture, + /// Every production Rust file of the workspace members, sorted by path. pub files: Vec, + /// The package of each workspace member, sorted by member path. + pub crates: Vec, } impl Context { @@ -99,7 +149,9 @@ impl Context { } pub fn load_with_baseline(root: &Path, baseline: Baseline) -> Result { - let config: ArchitectureConfig = parse_toml(&root.join(CONFIG_FILE))?; + let config_path = root.join(CONFIG_FILE); + let (config, config_text): (ArchitectureConfig, String) = + parse_toml_with_text(&config_path)?; // A table for an instance that no longer exists would never be // checked, so it cannot silently outlive its configuration. let configured = rules::tables(&config.rules); @@ -113,13 +165,29 @@ impl Context { table, }); } - let members = load_members(&root.join(ROOT_MANIFEST))?; - let files = production_files(root, &members)?; + let crate_names_policy = CrateNamesPolicy::parse(&config_text) + .map_err(|source| LoadError::Parse { + path: config_path, + source, + })? + .unwrap_or_default(); + if let Some(name) = baseline + .crate_names + .keys() + .find(|name| config.rules.crate_names.allow.contains(name)) + { + return Err(LoadError::AllowedAndListed { name: name.clone() }); + } + let crates = workspace_crates(root)?; + let files = production_files(root, &crates)?; Ok(Self { config, baseline, - base_baseline: BaseBaseline::NotGiven, + base_baseline: BaseBaseline::default(), + crate_names_policy, + base_architecture: BaseArchitecture::NotGiven, files, + crates, }) } @@ -205,63 +273,126 @@ pub(crate) fn parse_toml_with_text Deserialize<'de>>( } } -/// Only the part of the root manifest the inventory needs; other keys are -/// Cargo's business. +/// The part of `cargo metadata --format-version 1` the inventory needs. #[derive(Deserialize)] -struct RootManifest { - workspace: Option, +struct Metadata { + packages: Vec, + workspace_members: Vec, + workspace_root: PathBuf, } #[derive(Deserialize)] -struct WorkspaceManifest { - members: Option>, +struct MetadataPackage { + id: String, + name: String, + manifest_path: PathBuf, + targets: Vec, } -fn load_members(path: &Path) -> Result, LoadError> { - let manifest: RootManifest = parse_toml(path)?; - let members = manifest - .workspace - .and_then(|workspace| workspace.members) - .ok_or_else(|| LoadError::NoWorkspaceMembers { - path: path.to_path_buf(), +#[derive(Deserialize)] +struct MetadataTarget { + name: String, + kind: Vec, +} + +/// Every workspace member as Cargo sees it: the root manifest's `members`, +/// expanded, and every path dependency inside the workspace, which Cargo makes +/// a member too. `cargo` is the one running archcheck under `cargo run`. +fn workspace_crates(root: &Path) -> Result, LoadError> { + let cargo = env::var_os("CARGO").unwrap_or_else(|| "cargo".into()); + let output = Command::new(cargo) + .args([ + "metadata", + "--no-deps", + "--format-version", + "1", + "--offline", + ]) + .current_dir(root) + .output() + .map_err(|source| LoadError::CargoMetadataSpawn { + root: root.to_path_buf(), + source, })?; - for member in &members { - let error = if member.contains(GLOB_CHARACTERS) { - LoadError::GlobMember { - path: path.to_path_buf(), - member: member.clone(), - } - } else if member - .split('/') - .any(|part| part.is_empty() || part == "." || part == "..") - { - // Files are keyed by the path they are found under, so `a/../a` - // would not match the baseline's `a`. - LoadError::UnnormalizedMember { - path: path.to_path_buf(), - member: member.clone(), - } - } else { - continue; - }; - return Err(error); + if !output.status.success() { + return Err(LoadError::CargoMetadataFailed { + root: root.to_path_buf(), + status: output.status, + stderr: String::from_utf8_lossy(&output.stderr) + .trim_end() + .to_owned(), + }); } - Ok(members) + let metadata: Metadata = serde_json::from_slice(&output.stdout).map_err(|source| { + LoadError::CargoMetadataOutput { + root: root.to_path_buf(), + source, + } + })?; + let mut crates = metadata + .packages + .into_iter() + .filter(|package| metadata.workspace_members.contains(&package.id)) + .map(|package| load_crate(&metadata.workspace_root, package)) + .collect::, _>>()?; + crates.sort_by(|a, b| a.member.cmp(&b.member)); + Ok(crates) +} + +/// Only the names of a member's manifest, for their lines; Cargo has read +/// the rest. +#[derive(Deserialize)] +struct MemberManifest { + package: Option, + lib: Option, } -fn production_files(root: &Path, members: &[String]) -> Result, LoadError> { +#[derive(Deserialize)] +struct NameTable { + name: Option>, +} + +fn load_crate(workspace_root: &Path, package: MetadataPackage) -> Result { + let manifest = package.manifest_path; + let dir = manifest.parent().unwrap_or(&manifest); + let Ok(member) = dir.strip_prefix(workspace_root) else { + return Err(LoadError::MemberOutsideRoot { + manifest, + root: workspace_root.to_path_buf(), + }); + }; + let member = relative_path(Path::new(""), member)?; + let (parsed, text): (MemberManifest, String) = parse_toml_with_text(&manifest)?; + let line = |table: Option| { + table + .and_then(|table| table.name) + .map(|name| line_of(&text, name.span().start)) + }; + let lib = package + .targets + .into_iter() + .find(|target| { + target + .kind + .iter() + .any(|kind| LIB_KINDS.contains(&kind.as_str())) + }) + .map(|target| target.name); + Ok(Crate { + name: package.name, + lib, + member, + name_line: line(parsed.package).unwrap_or(1), + lib_name_line: line(parsed.lib), + }) +} + +fn production_files(root: &Path, crates: &[Crate]) -> Result, LoadError> { let mut paths = Vec::new(); - for member in members { - let manifest = root.join(member).join("Cargo.toml"); - if !manifest.is_file() { - return Err(LoadError::MissingMember { - member: member.clone(), - manifest, - }); - } + for krate in crates { // A member directory or `src` that is itself a symlink is followed; // symlinks below `src/` are not. - let src = root.join(member).join("src"); + let src = root.join(&krate.member).join("src"); if src.is_dir() { collect_rust_files(&src, &mut paths)?; } diff --git a/crates/tools/archcheck/src/finding.rs b/crates/tools/archcheck/src/finding.rs index 72a0d1189f..cbb212717c 100644 --- a/crates/tools/archcheck/src/finding.rs +++ b/crates/tools/archcheck/src/finding.rs @@ -1,6 +1,8 @@ use crate::config::Mode; -/// One reported problem. `path` is repository-relative with `/` separators. +/// One reported problem. `path` is what it is about: a repository-relative +/// path with `/` separators, or the key of a baseline entry such as a crate +/// name. #[derive(Debug, Clone, PartialEq, Eq)] pub struct Finding { pub rule: &'static str, diff --git a/crates/tools/archcheck/src/lib.rs b/crates/tools/archcheck/src/lib.rs index 0a22bd41f4..7b1e3a1828 100644 --- a/crates/tools/archcheck/src/lib.rs +++ b/crates/tools/archcheck/src/lib.rs @@ -14,7 +14,7 @@ pub mod rules; pub use baseline::{BASELINE_FILE, BaseBaseline, Baseline, WriteBaselineError}; pub use config::{ArchitectureConfig, CONFIG_FILE, Mode}; -pub use context::{Context, LoadError, ProductionFile}; +pub use context::{Context, Crate, LoadError, ProductionFile}; pub use finding::{Finding, Location}; use std::{collections::BTreeMap, path::Path}; @@ -35,16 +35,17 @@ pub fn check(ctx: &Context) -> Vec { /// one reason, so it gets one line, not one per table. pub fn skipped(ctx: &Context) -> Vec { let rules = rules::registered(); - if let BaseBaseline::Unusable(unusable) = &ctx.base_baseline { + let mut lines = if let BaseBaseline::Unusable(unusable) = &ctx.base_baseline { let comparisons: Vec = rules .iter() .flat_map(|rule| rule.base_comparisons(ctx)) .collect(); - return vec![ - BaseSkip::Unusable(unusable).message(&format!("{} skipped", comparisons.join(", "))), - ]; - } - rules.iter().flat_map(|rule| rule.skipped(ctx)).collect() + vec![BaseSkip::Unusable(unusable).message(&format!("{} skipped", comparisons.join(", ")))] + } else { + rules.iter().flat_map(|rule| rule.skipped(ctx)).collect() + }; + lines.extend(rules.iter().flat_map(|rule| rule.other_skips(ctx))); + lines } /// What [`write_baseline`] or [`write_baseline_table`] wrote, and the @@ -62,7 +63,8 @@ pub struct WrittenBaseline { /// Updates every table of `architecture-baseline.toml` from its entries and /// the current tree: entries still in violation are kept and updated, the /// rest are removed, and none are added. A `pattern_ratchet` table the -/// existing baseline lacks is not created; `[file_size]` is always written. +/// existing baseline lacks is not created; `[file_size]` and `[crate_names]` +/// are always written. pub fn write_baseline(root: &Path) -> Result { let ctx = Context::load(root)?; let (baseline, not_added) = update_baseline(&ctx); @@ -79,7 +81,7 @@ pub fn write_baseline(root: &Path) -> Result) -> Self { let mut not_added_entries: BTreeMap> = BTreeMap::new(); for entry in ¬_added { - not_added_entries - .entry(entry.table.clone()) - .or_default() - .insert(entry.path.clone(), entry.count); + if let Some((path, count)) = &entry.entry { + not_added_entries + .entry(entry.table.clone()) + .or_default() + .insert(path.clone(), *count); + } } Self { baseline, diff --git a/crates/tools/archcheck/src/main.rs b/crates/tools/archcheck/src/main.rs index 14e38ecd7d..bcb8453ca3 100644 --- a/crates/tools/archcheck/src/main.rs +++ b/crates/tools/archcheck/src/main.rs @@ -14,6 +14,7 @@ use std::{ use pplx_space_archcheck::{ BASELINE_FILE, Baseline, Context, LoadError, WriteBaselineError, check, + config::CrateNamesPolicy, context::find_workspace_root, report::{Format, UnknownFormatError, render, summary}, skipped, write_baseline, write_baseline_table, @@ -22,7 +23,7 @@ use thiserror::Error; const USAGE: &str = "\ usage: archcheck [--root ] [--format human|rdjson] - [--base-baseline ] + [--base-baseline ] [--base-architecture ] archcheck [--root ] --write-baseline[=
] --root repository root (default: the Cargo workspace containing @@ -31,14 +32,21 @@ usage: archcheck [--root ] [--format human|rdjson] summary on stderr --base-baseline the base branch's architecture-baseline.toml, which the - no_new_entries and no_raised_caps behaviours compare - with (skipped without it) + no_new_entries and no_raised_caps behaviours (skipped + without it) and crate_names.no_new (limited to the + crates listed when it was added without it) compare + with + --base-architecture + the base branch's ARCHITECTURE.toml, which + crate_names.policy_changed compares with (skipped + without it) --write-baseline[=
] update or remove the entries of architecture-baseline.toml - to match the current tree, or with =
(file_size or - pattern_ratchet.) only that table's, keeping - the others. It lists each violation without an entry, - with the entries to add by hand, and never adds one"; + to match the current tree, or with =
(file_size, + crate_names or pattern_ratchet.) only that + table's, keeping the others. It lists each violation + without an entry, with the entries to add by hand, and + never adds one"; const EXIT_REQUIRED_FINDINGS: u8 = 1; const EXIT_ERROR: u8 = 2; @@ -54,8 +62,8 @@ enum UsageError { UnexpectedArgument(String), #[error("unexpected argument {0:?}; name a table as --write-baseline={0}")] WriteBaselineTable(String), - #[error("--base-baseline cannot be combined with --write-baseline")] - BaseBaselineWithWriteBaseline, + #[error("{0} cannot be combined with --write-baseline")] + BaseWithWriteBaseline(&'static str), #[error(transparent)] Format(#[from] UnknownFormatError), } @@ -78,6 +86,7 @@ struct Options { root: Option, format: Format, base_baseline: Option, + base_architecture: Option, write_baseline: Option, } @@ -133,7 +142,7 @@ fn run(options: Options) -> Result { let counts: Vec = tables .iter() .filter_map(|table| { - let entries = written.baseline.table(table)?.len(); + let entries = written.baseline.table_len(table)?; Some(format!("{entries} {table}")) }) .collect(); @@ -167,6 +176,9 @@ fn run(options: Options) -> Result { if let Some(path) = &options.base_baseline { ctx.base_baseline = Baseline::load_base(path)?; } + if let Some(path) = &options.base_architecture { + ctx.base_architecture = CrateNamesPolicy::load_base(path)?; + } let findings = check(&ctx); let report = render(options.format, &findings, ctx.files.len()); // A line per skip, or warning about a skip, after the summary line. @@ -209,6 +221,7 @@ fn parse_args(args: impl Iterator) -> Result, U root: None, format: Format::Human, base_baseline: None, + base_architecture: None, write_baseline: None, }; let mut args = args @@ -231,6 +244,7 @@ fn parse_args(args: impl Iterator) -> Result, U "--root" => options.root = Some(PathBuf::from(value()?)), "--format" => options.format = value()?.parse()?, "--base-baseline" => options.base_baseline = Some(PathBuf::from(value()?)), + "--base-architecture" => options.base_architecture = Some(PathBuf::from(value()?)), // A table is only ever given after `=`, so a following argument // is never taken as one; after a bare `--write-baseline`, the // error says how to name it. @@ -247,8 +261,13 @@ fn parse_args(args: impl Iterator) -> Result, U other => return Err(UsageError::UnexpectedArgument(other.to_owned())), } } - if options.write_baseline.is_some() && options.base_baseline.is_some() { - return Err(UsageError::BaseBaselineWithWriteBaseline); + if options.write_baseline.is_some() { + if options.base_baseline.is_some() { + return Err(UsageError::BaseWithWriteBaseline("--base-baseline")); + } + if options.base_architecture.is_some() { + return Err(UsageError::BaseWithWriteBaseline("--base-architecture")); + } } Ok(Some(options)) } diff --git a/crates/tools/archcheck/src/rules.rs b/crates/tools/archcheck/src/rules.rs index da49b7b835..bd837e1498 100644 --- a/crates/tools/archcheck/src/rules.rs +++ b/crates/tools/archcheck/src/rules.rs @@ -1,9 +1,11 @@ //! The rule interface and the registry every check runs from. +mod crate_names; mod file_size; mod pattern_ratchet; mod ratchet; +pub(crate) use crate_names::CrateNames; pub(crate) use file_size::FileSize; pub(crate) use pattern_ratchet::PatternRatchet; pub(crate) use ratchet::CountRule; @@ -45,6 +47,14 @@ pub trait Rule { None } + /// Lines like [`Rule::skipped`]'s for inputs other than the base + /// branch's baseline, such as `--base-architecture`, which + /// [`crate::skipped`] reports however that baseline reads. `skipped` + /// does not repeat them. + fn other_skips(&self, _ctx: &Context) -> Vec { + Vec::new() + } + /// Fills this rule's tables for `--write-baseline` from the entries in /// `ctx.baseline`, and returns each violation it did not add. It never /// adds an entry, and a table of a rule with instances that `ctx.baseline` @@ -60,9 +70,10 @@ pub trait Rule { pub struct NotAdded { /// The table it is missing from, as `--write-baseline=
` names it. pub table: String, - /// The entry it would need: its file's path and current count. - pub path: String, - pub count: usize, + /// The entry it would need, its file's path and current count, to add by + /// hand; `None` where the table takes no hand-added entry, as for + /// `crate_names`, whose `no_new` would fail one. + pub entry: Option<(String, usize)>, /// `not added: …`, with what to do instead. pub line: String, } @@ -77,5 +88,9 @@ pub fn tables(config: &RulesConfig) -> Vec { /// Every rule, in reporting order. Register a new rule here. pub fn registered() -> Vec> { - vec![Box::new(FileSize), Box::new(PatternRatchet)] + vec![ + Box::new(FileSize), + Box::new(PatternRatchet), + Box::new(CrateNames), + ] } diff --git a/crates/tools/archcheck/src/rules/crate_names.rs b/crates/tools/archcheck/src/rules/crate_names.rs new file mode 100644 index 0000000000..c0cd946e6e --- /dev/null +++ b/crates/tools/archcheck/src/rules/crate_names.rs @@ -0,0 +1,462 @@ +//! No new catch-all crate names, such as `common`, `utils` or `core`. +//! +//! A name matches when one of its parts, once lowercased and split on `-` and +//! `_`, is a banned token; a crate matches when its package name or its +//! library's name does, unless `allow` names it. `no_new` reports a matching +//! crate that the baseline does not list at its path, and a baseline entry +//! the base branch's copy lacks, so the baseline is not a way around the rule; +//! without that copy's table, only [`BOOTSTRAP`] may be listed. +//! `stale_entries` asks for an entry or an `allow` name to be removed once it +//! no longer applies. `policy_changed` shows every change to +//! `[rules.crate_names]` compared with the base branch's `ARCHITECTURE.toml`, +//! so a pull request can't loosen the rule unnoticed. + +use std::collections::BTreeSet; + +use crate::{ + baseline::{BASELINE_FILE, BaseSkip, Baseline}, + config::{ + BaseArchitecture, CONFIG_FILE, CrateNamesConfig, CrateNamesPolicy, Mode, RulesConfig, + }, + context::{Context, Crate}, + finding::{Finding, Location}, + rules::{NotAdded, Rule}, +}; + +pub struct CrateNames; + +impl CrateNames { + /// The rule's name, as in its finding codes and baseline table. + pub const NAME: &'static str = "crate_names"; +} + +const RULE: &str = CrateNames::NAME; +const NO_NEW: &str = "no_new"; +const STALE_ENTRIES: &str = "stale_entries"; +const POLICY_CHANGED: &str = "policy_changed"; + +/// The crates that matched when the rule was added. Without a base branch +/// copy of `[crate_names]` to compare with (before the table reached the +/// base branch, or when the base can't be resolved), these are the only +/// names the baseline may list. +pub const BOOTSTRAP: [&str; 16] = [ + "pplx-space-common-cli", + "pplx-space-common-connection-admission", + "pplx-space-common-core", + "pplx-space-common-grpc", + "pplx-space-common-grpc-lb", + "pplx-space-common-io", + "pplx-space-common-jobs", + "pplx-space-common-lru-reaper", + "pplx-space-common-proto-build", + "pplx-space-common-svc", + "pplx-space-common-work-coalescer", + "pplx-space-credential-injection-core", + "pplx-space-execution-common", + "pplx-space-net-common", + "pplx-space-node-runtime-core", + "pplx-space-spaced-common", +]; + +/// Which name of a crate matched, and on which banned token. +struct CatchAll { + token: String, + /// The library's name, when it matched and the package name did not. + lib: Option, +} + +/// The names a pull request's baseline may list. +enum Listable<'a> { + /// The base branch's copy has the table. + Base(&'a BTreeSet), + /// No copy of the table to compare with. + Bootstrap, + /// The base branch's table, or its whole copy, does not parse. + Nothing, +} + +impl Listable<'_> { + fn contains(&self, name: &str) -> bool { + match self { + Self::Base(names) => names.contains(name), + Self::Bootstrap => BOOTSTRAP.contains(&name), + Self::Nothing => false, + } + } + + fn reason(&self) -> String { + match self { + Self::Base(_) => "not in the base branch's copy".to_owned(), + Self::Bootstrap => format!( + "not one of the {} crates listed when the rule was added, and there is no base \ + branch copy of [crate_names] to compare with", + BOOTSTRAP.len() + ), + Self::Nothing => { + format!("the base branch's {BASELINE_FILE} or its [crate_names] does not parse") + } + } + } +} + +impl Rule for CrateNames { + fn check(&self, ctx: &Context) -> Vec { + let config = &ctx.config.rules.crate_names; + let listed = &ctx.baseline.crate_names; + let mut findings = Vec::new(); + + let mode = config.no_new.mode; + for krate in &ctx.crates { + let Some(catch_all) = catch_all(config, krate) else { + continue; + }; + let manifest = krate.manifest(); + let (detail, line) = match (listed.get(&krate.name), &catch_all.lib) { + (Some(path), _) if *path == krate.member => continue, + (Some(path), _) => ( + format!( + ": `{}` is listed in {BASELINE_FILE} at {path}, but this crate is at {}. \ + An entry only covers the crate at its path: if the listed crate moved \ + here, update the entry's path; otherwise name this crate for what it \ + does.", + krate.name, krate.member + ), + krate.name_line, + ), + (None, None) => ( + format!( + ": `{}` uses `{}`, a catch-all name. {}", + krate.name, + catch_all.token, + advice() + ), + krate.name_line, + ), + (None, Some(lib)) => ( + format!( + ": the library of `{}` is named `{lib}`, which uses `{}`, a catch-all \ + name. {}", + krate.name, + catch_all.token, + advice() + ), + krate.lib_name_line.unwrap_or(krate.name_line), + ), + }; + findings.push(Finding { + rule: RULE, + instance: None, + behaviour: NO_NEW, + path: manifest.clone(), + detail, + mode, + location: Location { + path: manifest, + line, + }, + }); + } + + let listable = match ctx.base_baseline.crate_names() { + Ok(Some(names)) => Listable::Base(names), + Ok(None) | Err(BaseSkip::NotGiven | BaseSkip::NotRead) => Listable::Bootstrap, + Err(BaseSkip::Unusable(_)) => Listable::Nothing, + }; + for name in listed.keys().filter(|name| !listable.contains(name)) { + findings.push(entry_finding( + BASELINE_FILE, + ctx.baseline.crate_names_lines.get(name).copied(), + NO_NEW, + mode, + name, + format!( + " is in [crate_names] in {BASELINE_FILE} but {}; catch-all crate names can't \ + be added. Name the crate for what it does instead.", + listable.reason() + ), + )); + } + + let mode = config.stale_entries.mode; + for name in listed.keys() { + let detail = match ctx.crates.iter().find(|krate| krate.name == *name) { + None => format!( + ": listed in {BASELINE_FILE} but no workspace member has this name. Remove the \ + entry; a renamed crate is checked under its new name." + ), + Some(krate) if catch_all(config, krate).is_none() => format!( + ": listed in {BASELINE_FILE} but no longer a catch-all name. Remove the entry." + ), + // At another path, it is a `no_new` finding. + Some(_) => continue, + }; + findings.push(entry_finding( + BASELINE_FILE, + ctx.baseline.crate_names_lines.get(name).copied(), + STALE_ENTRIES, + mode, + name, + detail, + )); + } + for name in config.allow.iter() { + let detail = match ctx.crates.iter().find(|krate| krate.name == *name) { + None => format!( + ": in [rules.crate_names] allow in {CONFIG_FILE} but no workspace member has \ + this name. Remove it from allow." + ), + Some(krate) if catch_all_unless_allowed(config, krate).is_none() => format!( + ": in [rules.crate_names] allow in {CONFIG_FILE} but not a catch-all name. \ + Remove it from allow." + ), + Some(_) => continue, + }; + let line = ctx.crate_names_policy.allow.get(name).copied(); + findings.push(entry_finding( + CONFIG_FILE, + line, + STALE_ENTRIES, + mode, + name, + detail, + )); + } + + if let BaseArchitecture::Loaded(base) = &ctx.base_architecture { + let mode = config.policy_changed.mode; + findings.extend( + policy_changes(base.as_ref(), &ctx.crate_names_policy) + .into_iter() + .map(|(line, detail)| Finding { + rule: RULE, + instance: None, + behaviour: POLICY_CHANGED, + path: CONFIG_FILE.to_owned(), + detail, + mode, + location: Location { + path: CONFIG_FILE.to_owned(), + line, + }, + }), + ); + } + + findings + } + + /// Not called for a base branch's copy that is not TOML at all; `no_new` + /// then counts every entry as added, which its findings say. + fn skipped(&self, ctx: &Context) -> Vec { + let bootstrap = format!( + "{RULE}.{NO_NEW} allows only the {} crates listed when the rule was added in \ + [crate_names]", + BOOTSTRAP.len() + ); + match ctx.base_baseline.crate_names() { + Ok(Some(_)) => Vec::new(), + Ok(None) => vec![format!( + "the base branch's {BASELINE_FILE} has no [crate_names] table; {bootstrap}" + )], + Err(skip @ (BaseSkip::NotGiven | BaseSkip::NotRead)) => vec![skip.message(&bootstrap)], + Err(skip @ BaseSkip::Unusable(_)) => vec![skip.message(&format!( + "{RULE}.{NO_NEW} counts every [crate_names] entry as added" + ))], + } + } + + fn tables(&self, _config: &RulesConfig) -> Vec { + vec![RULE.to_owned()] + } + + fn baseline_rule(&self) -> Option<&'static str> { + Some(RULE) + } + + fn other_skips(&self, ctx: &Context) -> Vec { + let policy = format!("{RULE}.{POLICY_CHANGED} skipped"); + match &ctx.base_architecture { + BaseArchitecture::Loaded(_) => Vec::new(), + BaseArchitecture::NotGiven => { + vec![format!("{policy} (no --base-architecture given)")] + } + BaseArchitecture::Unusable(unusable) => { + vec![BaseSkip::Unusable(unusable).message(&policy)] + } + } + } + + /// Keeps each entry whose crate still matches at the entry's path. An + /// entry whose crate is at another path is kept as it is: updating the + /// path is a deliberate edit. A matching crate without an entry is not + /// added: that takes a hand-written entry, which `no_new` then reports. + fn update_baseline(&self, ctx: &Context, baseline: &mut Baseline) -> Vec { + let config = &ctx.config.rules.crate_names; + let listed = &ctx.baseline.crate_names; + let mut not_added = Vec::new(); + for krate in &ctx.crates { + let Some(catch_all) = catch_all(config, krate) else { + continue; + }; + match listed.get(&krate.name) { + Some(path) => { + if *path != krate.member { + not_added.push(not_added_line(format!( + "not updated: {} is listed at {path} but is at {}; update the entry's \ + path by hand if it moved", + krate.name, krate.member + ))); + } + baseline + .crate_names + .insert(krate.name.clone(), path.clone()); + } + None => not_added.push(not_added_line(format!( + "not added: {} ({}) uses `{}`, a catch-all name; name the crate for what it \ + does", + krate.name, + krate.manifest(), + catch_all.token + ))), + } + } + not_added + } +} + +/// A line for `--write-baseline` without an entry to paste: one added by +/// hand would be a `no_new` finding. +fn not_added_line(line: String) -> NotAdded { + NotAdded { + table: RULE.to_owned(), + entry: None, + line, + } +} + +fn advice() -> String { + format!( + "Name the crate for what it does, and split it if it holds unrelated things. Existing \ + catch-all crates are listed in {BASELINE_FILE}; new ones can't be added." + ) +} + +/// How `krate` matches, unless `allow` names it. +fn catch_all(config: &CrateNamesConfig, krate: &Crate) -> Option { + if config.allow.contains(&krate.name) { + return None; + } + catch_all_unless_allowed(config, krate) +} + +/// How `krate` matches: its package name first, then its library's. +fn catch_all_unless_allowed(config: &CrateNamesConfig, krate: &Crate) -> Option { + if let Some(token) = banned_token(config, &krate.name) { + return Some(CatchAll { token, lib: None }); + } + let lib = krate.lib.as_ref()?; + banned_token(config, lib).map(|token| CatchAll { + token, + lib: Some(lib.clone()), + }) +} + +/// The first part of `name`, lowercased and split on `-` and `_`, that is a +/// banned token. +fn banned_token(config: &CrateNamesConfig, name: &str) -> Option { + name.to_lowercase() + .split(['-', '_']) + .find(|part| config.banned_tokens.contains(part)) + .map(str::to_owned) +} + +/// Each difference of `policy` from `base` (`None`: the base has no table), +/// as the line it is shown on and the finding's detail. +fn policy_changes( + base: Option<&CrateNamesPolicy>, + policy: &CrateNamesPolicy, +) -> Vec<(usize, String)> { + let table = "[rules.crate_names]"; + let tokens_line = policy.banned_tokens_line.unwrap_or(1); + let Some(base) = base else { + return vec![( + tokens_line, + format!(": {table} is not in the base branch's copy; the whole policy is new."), + )]; + }; + let mut changes = Vec::new(); + for (token, &line) in &policy.banned_tokens { + if !base.banned_tokens.contains_key(token) { + changes.push(( + line, + format!(": `{token}` was added to {table} banned_tokens."), + )); + } + } + for token in base.banned_tokens.keys() { + if !policy.banned_tokens.contains_key(token) { + changes.push(( + tokens_line, + format!( + ": `{token}` was removed from {table} banned_tokens, so crate names using it \ + are no longer checked." + ), + )); + } + } + for (name, &line) in &policy.allow { + if !base.allow.contains_key(name) { + changes.push(( + line, + format!(": `{name}` was added to {table} allow, exempting it from the rule."), + )); + } + } + for name in base.allow.keys() { + if !policy.allow.contains_key(name) { + changes.push(( + policy.allow_line.unwrap_or(1), + format!(": `{name}` was removed from {table} allow."), + )); + } + } + let behaviours: BTreeSet<&String> = base.modes.keys().chain(policy.modes.keys()).collect(); + for behaviour in behaviours { + let old = base.modes.get(behaviour).map(|(mode, _)| mode.as_str()); + let new = policy.modes.get(behaviour); + if old != new.map(|(mode, _)| mode.as_str()) { + changes.push(( + new.map_or(1, |(_, line)| *line), + format!( + ": {RULE}.{behaviour} mode changed from {} to {}.", + old.unwrap_or("unset"), + new.map_or("unset", |(mode, _)| mode.as_str()) + ), + )); + } + } + changes +} + +/// A finding about an entry of `file`, shown on its line. +fn entry_finding( + file: &str, + line: Option, + behaviour: &'static str, + mode: Mode, + name: &str, + detail: String, +) -> Finding { + Finding { + rule: RULE, + instance: None, + behaviour, + path: name.to_owned(), + detail, + mode, + location: Location { + path: file.to_owned(), + line: line.unwrap_or(1), + }, + } +} diff --git a/crates/tools/archcheck/src/rules/ratchet.rs b/crates/tools/archcheck/src/rules/ratchet.rs index d29d0be38e..448c5ca3bb 100644 --- a/crates/tools/archcheck/src/rules/ratchet.rs +++ b/crates/tools/archcheck/src/rules/ratchet.rs @@ -350,8 +350,7 @@ fn update_baseline( } else { not_added.push(NotAdded { table: table.clone(), - path: file.path.clone(), - count, + entry: Some((file.path.clone(), count)), line: format!( "not added: {} ({}); {advice}", file.path, diff --git a/crates/tools/archcheck/src/rules/ratchet/tests.rs b/crates/tools/archcheck/src/rules/ratchet/tests.rs index e9a53945d0..b36db58a80 100644 --- a/crates/tools/archcheck/src/rules/ratchet/tests.rs +++ b/crates/tools/archcheck/src/rules/ratchet/tests.rs @@ -7,6 +7,7 @@ use std::{collections::BTreeMap, fs}; use super::*; use crate::{ baseline::BaseBaseline, + config::{BaseArchitecture, CrateNamesPolicy}, context::{code_lines, count_code_lines}, }; @@ -73,7 +74,11 @@ fn context(baseline: Baseline, files: &[(&str, &str)]) -> Context { [rules.file_size.no_new_entries]\nmode = \"advisory\"\n\ [rules.file_size.no_raised_caps]\nmode = \"advisory\"\n\ [rules.file_size.listed_no_growth]\nmode = \"advisory\"\n\ - [rules.file_size.stale_entries]\nmode = \"advisory\"\n"; + [rules.file_size.stale_entries]\nmode = \"advisory\"\n\ + [rules.crate_names]\nbanned_tokens = [\"common\"]\nallow = []\n\ + [rules.crate_names.no_new]\nmode = \"advisory\"\n\ + [rules.crate_names.stale_entries]\nmode = \"advisory\"\n\ + [rules.crate_names.policy_changed]\nmode = \"advisory\"\n"; let mut files: Vec = files .iter() .map(|(path, text)| ProductionFile { @@ -87,7 +92,10 @@ fn context(baseline: Baseline, files: &[(&str, &str)]) -> Context { config: toml::from_str(config).expect("the configuration parses"), baseline, base_baseline: BaseBaseline::NotGiven, + crate_names_policy: CrateNamesPolicy::default(), + base_architecture: BaseArchitecture::NotGiven, files, + crates: Vec::new(), } } diff --git a/crates/tools/archcheck/tests/cli.rs b/crates/tools/archcheck/tests/cli.rs index e3ce10f76a..e71afb1c8f 100644 --- a/crates/tools/archcheck/tests/cli.rs +++ b/crates/tools/archcheck/tests/cli.rs @@ -6,6 +6,7 @@ use std::{fs, path::Path, process::Command}; use pplx_space_archcheck::{ BaseBaseline, Baseline, Context, check, + config::CrateNamesPolicy, context::count_code_lines, report::{Format, render}, skipped, write_baseline, @@ -22,12 +23,37 @@ fn code(lines: usize) -> String { const ADVISORY: [&str; 5] = ["advisory"; 5]; -/// The summary's line when no base baseline is given. +/// The summary's line when no base architecture is given. +const POLICY_SKIPPED: &str = + "archcheck: crate_names.policy_changed skipped (no --base-architecture given)\n"; + +/// The summary's lines when neither base is given. const SKIPPED: &str = "archcheck: file_size.no_new_entries and file_size.no_raised_caps skipped \ - (no --base-baseline given)\n"; + (no --base-baseline given)\n\ + archcheck: crate_names.no_new allows only the 16 crates listed when the \ + rule was added in [crate_names] (no --base-baseline given)\n\ + archcheck: crate_names.policy_changed skipped (no --base-architecture \ + given)\n"; + +/// `crate_names`' lines after the other skip lines, for a base baseline +/// without `[crate_names]` and no base architecture. +const CRATE_NAMES_SKIPPED: [&str; 2] = [ + "the base branch's architecture-baseline.toml has no [crate_names] table; crate_names.no_new \ + allows only the 16 crates listed when the rule was added in [crate_names]", + "crate_names.policy_changed skipped (no --base-architecture given)", +]; -/// `modes` of no_crossing, no_new_entries, no_raised_caps, listed_no_growth -/// and stale_entries. +/// `[rules.crate_names]` as in the repository's `ARCHITECTURE.toml`. +const CRATE_NAMES: &str = "[rules.crate_names]\n\ + banned_tokens = [\"common\", \"shared\", \"util\", \"utils\", \ + \"helper\", \"helpers\", \"misc\", \"core\"]\n\ + allow = []\n\ + [rules.crate_names.no_new]\nmode = \"required\"\n\ + [rules.crate_names.stale_entries]\nmode = \"advisory\"\n\ + [rules.crate_names.policy_changed]\nmode = \"advisory\"\n"; + +/// `modes` of file_size's no_crossing, no_new_entries, no_raised_caps, +/// listed_no_growth and stale_entries, followed by [`CRATE_NAMES`]. fn config(modes: [&str; 5]) -> String { let [ no_crossing, @@ -42,18 +68,46 @@ fn config(modes: [&str; 5]) -> String { [rules.file_size.no_new_entries]\nmode = \"{no_new_entries}\"\n\ [rules.file_size.no_raised_caps]\nmode = \"{no_raised_caps}\"\n\ [rules.file_size.listed_no_growth]\nmode = \"{listed_no_growth}\"\n\ - [rules.file_size.stale_entries]\nmode = \"{stale_entries}\"\n" + [rules.file_size.stale_entries]\nmode = \"{stale_entries}\"\n\ + {CRATE_NAMES}" ) } +/// [`config`] with file_size's behaviours advisory, and the `key` line of +/// `[rules.crate_names]` replaced with `key = `. +fn with_crate_names(key: &str, value: &str) -> String { + let advisory = config(ADVISORY); + let line = advisory + .lines() + .find(|line| line.starts_with(&format!("{key} = "))) + .expect("CRATE_NAMES has the key"); + advisory.replace(line, &format!("{key} = {value}")) +} + +/// `[file_size]` with `entries` and an empty `[crate_names]`. fn baseline(entries: &[(&str, usize)]) -> String { + tables(entries, &[]) +} + +/// `[file_size]` and `[crate_names]` with these entries. +fn tables(file_size: &[(&str, usize)], crate_names: &[(&str, &str)]) -> String { let mut text = String::from("[file_size]\n"); - for (path, cap) in entries { + for (path, cap) in file_size { text.push_str(&format!("{path:?} = {cap}\n")); } + text.push_str("[crate_names]\n"); + for (name, member) in crate_names { + text.push_str(&format!("{name:?} = {member:?}\n")); + } text } +/// A member manifest for package `name`. Its library's source is outside +/// `src/`, so that it is not a production file, and need not exist. +fn package(name: &str) -> String { + format!("[package]\nname = {name:?}\n[lib]\npath = \"lib.rs\"\n") +} + /// A workspace with members `crates/a` and `crates/b`, every behaviour /// advisory and an empty baseline, with `files` written over those defaults. fn workspace(files: &[(&str, String)]) -> TempDir { @@ -63,8 +117,8 @@ fn workspace(files: &[(&str, String)]) -> TempDir { "Cargo.toml", "[workspace]\nmembers = [\"crates/a\", \"crates/b\"]\n".to_owned(), ), - ("crates/a/Cargo.toml", "[package]\n".to_owned()), - ("crates/b/Cargo.toml", "[package]\n".to_owned()), + ("crates/a/Cargo.toml", package("a")), + ("crates/b/Cargo.toml", package("b")), ("ARCHITECTURE.toml", config(ADVISORY)), ("architecture-baseline.toml", baseline(&[])), ]; @@ -76,15 +130,50 @@ fn workspace(files: &[(&str, String)]) -> TempDir { dir } +/// A workspace with members `crates/a`, `crates/b` and one per +/// `(member, package name)` in `packages`, and `files` written over the +/// defaults. +fn crate_workspace(packages: &[(&str, &str)], files: &[(&str, String)]) -> TempDir { + let mut members = vec!["crates/a", "crates/b"]; + members.extend(packages.iter().map(|(member, _)| *member)); + let manifests: Vec<(String, String)> = packages + .iter() + .map(|(member, name)| (format!("{member}/Cargo.toml"), package(name))) + .collect(); + let mut all: Vec<(&str, String)> = vec![( + "Cargo.toml", + format!("[workspace]\nmembers = {members:?}\n"), + )]; + all.extend( + manifests + .iter() + .map(|(path, text)| (path.as_str(), text.clone())), + ); + all.extend(files.iter().cloned()); + workspace(&all) +} + /// The workspace's context, with `base` written to `base-baseline.toml` and /// read as the base branch's baseline. fn load(root: &Path, base: Option<&str>) -> Context { + load_with(root, base, None) +} + +/// [`load`], also with `architecture` written to `base-architecture.toml` +/// and read as the base branch's `ARCHITECTURE.toml`. +fn load_with(root: &Path, base: Option<&str>, architecture: Option<&str>) -> Context { let mut ctx = Context::load(root).expect("the workspace loads"); if let Some(text) = base { let path = root.join("base-baseline.toml"); fs::write(&path, text).expect("write the base baseline"); ctx.base_baseline = Baseline::load_base(&path).expect("the base baseline is readable"); } + if let Some(text) = architecture { + let path = root.join("base-architecture.toml"); + fs::write(&path, text).expect("write the base architecture"); + ctx.base_architecture = + CrateNamesPolicy::load_base(&path).expect("the base architecture is readable"); + } ctx } @@ -172,8 +261,8 @@ fn inventory() { ]; let mut files: Vec<(&str, String)> = cases.iter().map(|(path, _)| (*path, code(1))).collect(); files.extend([ - ("crates/a/src/inner/Cargo.toml", "[package]\n".to_owned()), - ("crates/c/Cargo.toml", "[package]\n".to_owned()), + ("crates/a/src/inner/Cargo.toml", package("inner")), + ("crates/c/Cargo.toml", package("c")), ]); let mut members = vec!["crates/a", "crates/b", "crates/a/src/inner"]; let mut expected: Vec<&str> = cases @@ -185,7 +274,7 @@ fn inventory() { { // `crates/s/src` is a symlink to `shared/src`, made below. files.extend([ - ("crates/s/Cargo.toml", "[package]\n".to_owned()), + ("crates/s/Cargo.toml", package("s")), ("shared/src/lib.rs", code(1)), ]); members.push("crates/s"); @@ -478,7 +567,10 @@ fn lenient_base_baseline() { .map(|finding| finding.behaviour) .collect(); assert_eq!(found, *expected, "{name}"); - let skipped = skipped(&ctx); + let skipped: Vec = skipped(&ctx) + .into_iter() + .filter(|line| line.contains("file_size.")) + .collect(); match problem { None => { assert!( @@ -503,6 +595,962 @@ fn lenient_base_baseline() { } } +/// Pairs such as `(member, package name)` or `(behaviour, message)`. +type Pairs<'a> = &'a [(&'a str, &'a str)]; + +/// The base branch's `[crate_names]`, as a column of the tables below. +#[derive(Clone, Copy)] +enum Base<'a> { + /// No `--base-baseline`, as when the base can't be resolved. + Unresolved, + /// A copy without the table, from before the rule. + NoTable, + Names(&'a [&'a str]), +} + +impl Base<'_> { + fn text(self) -> Option { + match self { + Self::Unresolved => None, + Self::NoTable => Some("[file_size]\n".to_owned()), + Self::Names(names) => { + let entries: Vec<(&str, &str)> = + names.iter().map(|name| (*name, "crates/x")).collect(); + Some(tables(&[], &entries)) + } + } + } +} + +/// `(behaviour, message)` of each `crate_names` finding. +fn crate_findings(ctx: &Context) -> Vec<(&'static str, String)> { + check(ctx) + .into_iter() + .filter(|finding| finding.rule == "crate_names") + .map(|finding| (finding.behaviour, finding.message())) + .collect() +} + +fn assert_findings(name: &str, found: &[(&str, String)], expected: &[(&str, &str)]) { + assert_eq!(found.len(), expected.len(), "{name}: {found:#?}"); + for ((behaviour, message), (want_behaviour, want_message)) in found.iter().zip(expected) { + assert_eq!(behaviour, want_behaviour, "{name}"); + assert!(message.starts_with(want_message), "{name}: {message}"); + } +} + +/// One row per situation: the base branch's `[crate_names]`, the baseline's +/// entries, the extra members as `(member, package name)`, and the expected +/// `(behaviour, start of message)` of each `crate_names` finding. +#[test] +fn crate_names_behaviours() { + const LISTED: &str = "pplx-space-common-io"; + type Case<'a> = ( + &'a str, + Base<'a>, + &'a [(&'a str, &'a str)], + &'a [(&'a str, &'a str)], + &'a [(&'a str, &'a str)], + ); + let new = |member: &str, name: &str, token: &str| { + format!("{member}/Cargo.toml: `{name}` uses `{token}`, a catch-all name.") + }; + let underscores = new("crates/x", "pplx_space_foo_utils", "utils"); + let mixed_case = new("crates/x", "Foo-Utils", "utils"); + let mixed_separators = new("crates/x", "foo_Shared-io", "shared"); + let unprefixed = new("crates/x", "foo-core", "core"); + let first = new("crates/x", "pplx-space-common-core", "common"); + let renamed_to_utils = new("crates/x", "pplx-space-io-utils", "utils"); + let not_bootstrap = "pplx-space-foo-utils is in [crate_names] in architecture-baseline.toml \ + but not one of the 16 crates listed when the rule was added, and there \ + is no base branch copy of [crate_names] to compare with; catch-all crate \ + names can't be added. Name the crate for what it does instead."; + let elsewhere = "crates/new/Cargo.toml: `pplx-space-common-io` is listed in \ + architecture-baseline.toml at crates/old, but this crate is at crates/new. \ + An entry only covers the crate at its path: if the listed crate moved here, \ + update the entry's path; otherwise name this crate for what it does."; + let foo_utils = [("crates/libs/foo-utils", "pplx-space-foo-utils")]; + let foo_utils_entry = [("pplx-space-foo-utils", "crates/libs/foo-utils")]; + let cases: &[Case] = &[ + ( + "a new catch-all crate", + Base::Unresolved, + &[], + &foo_utils, + &[( + "no_new", + "crates/libs/foo-utils/Cargo.toml: `pplx-space-foo-utils` uses `utils`, a \ + catch-all name. Name the crate for what it does, and split it if it holds \ + unrelated things. Existing catch-all crates are listed in \ + architecture-baseline.toml; new ones can't be added.", + )], + ), + ( + "underscores", + Base::Unresolved, + &[], + &[("crates/x", "pplx_space_foo_utils")], + &[("no_new", &underscores)], + ), + ( + "mixed case", + Base::Unresolved, + &[], + &[("crates/x", "Foo-Utils")], + &[("no_new", &mixed_case)], + ), + ( + "mixed separators", + Base::Unresolved, + &[], + &[("crates/x", "foo_Shared-io")], + &[("no_new", &mixed_separators)], + ), + ( + "a name without the pplx-space- prefix", + Base::Unresolved, + &[], + &[("crates/x", "foo-core")], + &[("no_new", &unprefixed)], + ), + ( + "the first matching part is named", + Base::Unresolved, + &[], + &[("crates/x", "pplx-space-common-core")], + &[("no_new", &first)], + ), + ( + "whole parts only", + Base::Unresolved, + &[], + &[ + ("crates/p0", "pplx-space-database"), + ("crates/p1", "pplx-space-coreutils"), + ("crates/p2", "encore"), + ("crates/p3", "pplx-space-uncommon"), + ("crates/p4", "utility_helpersets"), + ], + &[], + ), + ( + "a listed crate", + Base::Names(&[LISTED]), + &[(LISTED, "crates/x")], + &[("crates/x", LISTED)], + &[], + ), + ( + "an entry was added", + Base::Names(&[]), + &[(LISTED, "crates/x")], + &[("crates/x", LISTED)], + &[( + "no_new", + "pplx-space-common-io is in [crate_names] in architecture-baseline.toml but not \ + in the base branch's copy; catch-all crate names can't be added. Name the crate \ + for what it does instead.", + )], + ), + ( + "a new crate with an entry of its own", + Base::Names(&[]), + &foo_utils_entry, + &foo_utils, + &[("no_new", "pplx-space-foo-utils is in [crate_names]")], + ), + ( + "an entry in the base, though not a bootstrap crate", + Base::Names(&["pplx-space-foo-utils"]), + &foo_utils_entry, + &foo_utils, + &[], + ), + ( + "no base table: a bootstrap entry", + Base::NoTable, + &[(LISTED, "crates/x")], + &[("crates/x", LISTED)], + &[], + ), + ( + "no base table: any other entry", + Base::NoTable, + &foo_utils_entry, + &foo_utils, + &[("no_new", not_bootstrap)], + ), + ( + "an unresolved base: a bootstrap entry", + Base::Unresolved, + &[(LISTED, "crates/x")], + &[("crates/x", LISTED)], + &[], + ), + ( + "an unresolved base: any other entry", + Base::Unresolved, + &foo_utils_entry, + &foo_utils, + &[("no_new", not_bootstrap)], + ), + ( + "an entry was removed", + Base::Names(&[LISTED, "pplx-space-misc"]), + &[(LISTED, "crates/x")], + &[("crates/x", LISTED)], + &[], + ), + ( + "a listed name at another path", + Base::Names(&[LISTED]), + &[(LISTED, "crates/old")], + &[("crates/new", LISTED)], + &[("no_new", elsewhere)], + ), + ( + "a listed crate moved, and its entry's path with it", + Base::Names(&[LISTED]), + &[(LISTED, "crates/new")], + &[("crates/new", LISTED)], + &[], + ), + ( + "a listed crate was renamed to a clean name", + Base::Names(&[LISTED]), + &[(LISTED, "crates/x")], + &[("crates/x", "pplx-space-io-buffers")], + &[( + "stale_entries", + "pplx-space-common-io: listed in architecture-baseline.toml but no workspace \ + member has this name. Remove the entry; a renamed crate is checked under its new \ + name.", + )], + ), + ( + "a listed crate was removed", + Base::Names(&[LISTED]), + &[(LISTED, "crates/x")], + &[], + &[("stale_entries", "pplx-space-common-io: listed in")], + ), + ( + "a listed crate was renamed to another catch-all name", + Base::Names(&[LISTED]), + &[(LISTED, "crates/x")], + &[("crates/x", "pplx-space-io-utils")], + &[ + ("no_new", &renamed_to_utils), + ("stale_entries", "pplx-space-common-io: listed in"), + ], + ), + ( + "a listed name that is not a catch-all name", + Base::Names(&["pplx-space-io-buffers"]), + &[("pplx-space-io-buffers", "crates/x")], + &[("crates/x", "pplx-space-io-buffers")], + &[( + "stale_entries", + "pplx-space-io-buffers: listed in architecture-baseline.toml but no longer a \ + catch-all name. Remove the entry.", + )], + ), + ]; + for (name, base, entries, packages, expected) in cases { + let ws = crate_workspace( + packages, + &[("architecture-baseline.toml", tables(&[], entries))], + ); + + let found = crate_findings(&load(ws.path(), base.text().as_deref())); + + assert_findings(name, &found, expected); + } +} + +/// Cargo makes a path dependency inside the workspace a member, though the +/// root manifest's `members` does not list it: both rules check it. +#[test] +fn implicit_members() { + let ws = workspace(&[ + ( + "crates/a/Cargo.toml", + format!( + "{}[dependencies]\nfoo = {{ path = \"../../libs/foo-utils\", package = \ + \"pplx-space-foo-utils\" }}\n", + package("a") + ), + ), + ("libs/foo-utils/Cargo.toml", package("pplx-space-foo-utils")), + ("libs/foo-utils/src/big.rs", code(LIMIT + 1)), + ("libs/foo-utils/src/ok.rs", code(LIMIT)), + ]); + + let ctx = load(ws.path(), None); + + let files: Vec<&str> = ctx.files.iter().map(|file| file.path.as_str()).collect(); + assert_eq!( + files, + ["libs/foo-utils/src/big.rs", "libs/foo-utils/src/ok.rs"] + ); + let found: Vec<_> = check(&ctx) + .into_iter() + .map(|finding| (finding.behaviour, finding.location.path)) + .collect(); + assert_eq!( + found, + [ + ("no_crossing", "libs/foo-utils/src/big.rs".to_owned()), + ("no_new", "libs/foo-utils/Cargo.toml".to_owned()), + ] + ); + assert_eq!(run(ws.path(), &[]).code, 1); +} + +/// The library's name is checked too, as written in `[lib] name` or derived +/// from the package name; the package name is reported first. +#[test] +fn crate_names_library_names() { + let cases: &[(&str, &str, Pairs)] = &[ + ( + "pplx-space-io", + "name = \"utils\"\n", + &[( + "no_new", + "crates/x/Cargo.toml: the library of `pplx-space-io` is named `utils`, which uses \ + `utils`, a catch-all name. Name the crate", + )], + ), + ( + "pplx-space-io", + "name = \"io_Helpers\"\n", + &[( + "no_new", + "crates/x/Cargo.toml: the library of `pplx-space-io` is named", + )], + ), + ("pplx-space-io", "name = \"io_buffers\"\n", &[]), + ("pplx-space-io", "", &[]), + ( + "pplx-space-misc-io", + "", + &[( + "no_new", + "crates/x/Cargo.toml: `pplx-space-misc-io` uses `misc`", + )], + ), + ( + "pplx-space-misc-io", + "name = \"utils\"\n", + &[( + "no_new", + "crates/x/Cargo.toml: `pplx-space-misc-io` uses `misc`", + )], + ), + ]; + for (package, lib, expected) in cases { + let manifest = format!("[package]\nname = {package:?}\n[lib]\npath = \"lib.rs\"\n{lib}"); + let ws = crate_workspace( + &[("crates/x", package)], + &[("crates/x/Cargo.toml", manifest)], + ); + + let found = crate_findings(&load(ws.path(), None)); + + assert_findings(&format!("{package} {lib}"), &found, expected); + } + + // Shown on the `[lib] name` line. + let ws = crate_workspace( + &[("crates/x", "pplx-space-io")], + &[( + "crates/x/Cargo.toml", + "[package]\nname = \"pplx-space-io\"\n\n[lib]\npath = \"lib.rs\"\nname = \"utils\"\n" + .to_owned(), + )], + ); + let document = rdjson(ws.path(), None); + assert_eq!( + document["diagnostics"][0]["location"]["range"]["start"]["line"], + 6 + ); +} + +/// `allow` exempts exact package names; one that is also in the baseline is +/// a configuration error, and one that no longer applies is stale. +#[test] +fn crate_names_allow() { + let shared = "pplx-space-shared-memory"; + let cases: &[(&str, &str, Pairs, Pairs)] = &[ + ("[\"pplx-space-shared-memory\"]", shared, &[], &[]), + ( + "[\"pplx-space-shared\"]", + shared, + &[], + &[ + ( + "no_new", + "crates/x/Cargo.toml: `pplx-space-shared-memory` uses `shared`", + ), + ( + "stale_entries", + "pplx-space-shared: in [rules.crate_names] allow in ARCHITECTURE.toml but no \ + workspace member has this name. Remove it from allow.", + ), + ], + ), + ( + "[\"pplx-space-memory\"]", + "pplx-space-memory", + &[], + &[( + "stale_entries", + "pplx-space-memory: in [rules.crate_names] allow in ARCHITECTURE.toml but not a \ + catch-all name. Remove it from allow.", + )], + ), + ]; + for (allow, package, entries, expected) in cases { + let ws = crate_workspace( + &[("crates/x", package)], + &[ + ("ARCHITECTURE.toml", with_crate_names("allow", allow)), + ("architecture-baseline.toml", tables(&[], entries)), + ], + ); + + let found = crate_findings(&load(ws.path(), None)); + + assert_findings(allow, &found, expected); + } + + let ws = crate_workspace( + &[("crates/x", shared)], + &[ + ( + "ARCHITECTURE.toml", + with_crate_names("allow", "[\"pplx-space-shared-memory\"]"), + ), + ( + "architecture-baseline.toml", + tables(&[], &[(shared, "crates/x")]), + ), + ], + ); + let error = Context::load(ws.path()).unwrap_err().to_string(); + assert!( + error.starts_with( + "pplx-space-shared-memory is in both [rules.crate_names] allow in ARCHITECTURE.toml \ + and [crate_names] in architecture-baseline.toml" + ), + "{error}" + ); + assert_eq!(run(ws.path(), &[]).code, 2); +} + +/// `policy_changed` compares `[rules.crate_names]` with the base branch's +/// `ARCHITECTURE.toml` and shows each difference on its line there; the +/// base copy is read leniently. +#[test] +fn crate_names_policy_changed() { + let base = config(ADVISORY); + let without_misc = base.replace(", \"misc\"", ""); + let allowing = base.replace( + "allow = []", + "allow = [\n \"pplx-space-shared-memory\",\n]", + ); + let advisory_no_new = base.replace( + "[rules.crate_names.no_new]\nmode = \"required\"", + "[rules.crate_names.no_new]\nmode = \"advisory\"", + ); + let line_of = + |text: &str, needle: &str| text.lines().position(|line| line.contains(needle)).unwrap() + 1; + let tokens = line_of(&base, "banned_tokens"); + type Case<'a> = (&'a str, &'a str, String, &'a [(usize, &'a str)]); + let cases: &[Case] = &[ + ("unchanged", &base, base.clone(), &[]), + ( + "a token removed", + &base, + without_misc.clone(), + &[( + tokens, + "ARCHITECTURE.toml: `misc` was removed from [rules.crate_names] banned_tokens, so \ + crate names using it are no longer checked.", + )], + ), + ( + "a token added", + &without_misc, + base.clone(), + &[( + tokens, + "ARCHITECTURE.toml: `misc` was added to [rules.crate_names] banned_tokens.", + )], + ), + ( + "an allow entry added", + &base, + allowing.clone(), + &[( + line_of(&allowing, "\"pplx-space-shared-memory\""), + "ARCHITECTURE.toml: `pplx-space-shared-memory` was added to [rules.crate_names] \ + allow, exempting it from the rule.", + )], + ), + ( + "an allow entry removed", + &allowing, + base.clone(), + &[( + line_of(&base, "allow"), + "ARCHITECTURE.toml: `pplx-space-shared-memory` was removed from \ + [rules.crate_names] allow.", + )], + ), + ( + "a mode changed", + &base, + advisory_no_new.clone(), + &[( + line_of(&advisory_no_new, "[rules.crate_names.no_new]") + 1, + "ARCHITECTURE.toml: crate_names.no_new mode changed from required to advisory.", + )], + ), + ( + "a base from before the rule", + "[rules.file_size]\nlimit = 5\n", + base.clone(), + &[( + tokens, + "ARCHITECTURE.toml: [rules.crate_names] is not in the base branch's copy; the \ + whole policy is new.", + )], + ), + ( + "unknown keys and behaviours in the base", + &format!("{base}extra = 1\n[rules.crate_names.retired]\nmode = \"advisory\"\n"), + base.clone(), + &[], + ), + ]; + for (name, base, policy, expected) in cases { + let ws = crate_workspace( + &[("crates/x", "pplx-space-shared-memory")], + &[("ARCHITECTURE.toml", policy.clone())], + ); + + let ctx = load_with(ws.path(), None, Some(base)); + + let found: Vec<_> = check(&ctx) + .into_iter() + .filter(|finding| finding.behaviour == "policy_changed") + .map(|finding| (finding.location.line, finding.message(), finding.mode)) + .collect(); + assert_eq!(found.len(), expected.len(), "{name}: {found:#?}"); + for ((line, message, mode), (want_line, want)) in found.iter().zip(*expected) { + assert_eq!(line, want_line, "{name}"); + assert_eq!(message, want, "{name}"); + assert_eq!(*mode, pplx_space_archcheck::Mode::Advisory, "{name}"); + } + assert!( + !skipped(&ctx) + .iter() + .any(|line| line.contains("policy_changed")), + "{name}" + ); + } + + // A copy that does not parse skips the comparison with a warning, and so + // does the lack of one; neither fails the check. + let ws = workspace(&[("base.toml", "[rules\n".to_owned())]); + let base = ws.path().join("base.toml"); + let run_unusable = run(ws.path(), &["--base-architecture", base.to_str().unwrap()]); + assert_eq!(run_unusable.code, 0, "{}", run_unusable.stderr); + let warning = format!( + "archcheck: warning: could not parse {} at line 1: ", + base.display() + ); + let last = run_unusable.stdout.lines().last().unwrap(); + assert!( + last.starts_with(&warning) && last.ends_with("; crate_names.policy_changed skipped"), + "{}", + run_unusable.stdout + ); + assert!(run(ws.path(), &[]).stdout.ends_with(POLICY_SKIPPED)); + let missing = ws.path().join("missing.toml"); + let run_missing = run( + ws.path(), + &["--base-architecture", missing.to_str().unwrap()], + ); + assert_eq!(run_missing.code, 2); +} + +/// The base branch's `[crate_names]` is read on its own: other tables and the +/// values are ignored. Without the table, only the bootstrap crates may be +/// listed; a table that does not parse counts as empty, with a warning. The +/// baseline lists `pplx-space-common-io`, a bootstrap crate. +#[test] +fn crate_names_lenient_base_baseline() { + /// Name, the base's copy, the `crate_names` behaviours found, the + /// summary's line, and the exit status. + type Case<'a> = (&'a str, String, &'a [&'a str], Option<&'a str>, i32); + let listed = "[crate_names]\n\"pplx-space-common-io\" = \"crates/x\"\n"; + let missing = CRATE_NAMES_SKIPPED[0]; + let cases: &[Case] = &[ + ("the table", listed.to_owned(), &[], None, 0), + ( + "other tables and keys", + format!("version = 2\n[other]\nx = 1\n{listed}"), + &[], + None, + 0, + ), + ( + "values of another shape", + "[crate_names]\n\"pplx-space-common-io\" = { path = 1 }\n".to_owned(), + &[], + None, + 0, + ), + ( + "a [file_size] that does not parse", + format!("[file_size]\n\"x\" = \"9\"\n{listed}"), + &[], + None, + 0, + ), + ("an empty file", String::new(), &[], Some(missing), 0), + ( + "no [crate_names] table", + "[file_size]\n".to_owned(), + &[], + Some(missing), + 0, + ), + ( + "[crate_names] is not a table", + "crate_names = 3\n".to_owned(), + &["no_new"], + Some("at line 1: "), + 1, + ), + ( + "invalid TOML", + format!("{listed}[file_size\n"), + &["no_new"], + Some("at line 3: "), + 1, + ), + ]; + for (name, base, expected, line, code) in cases { + let ws = crate_workspace( + &[("crates/x", "pplx-space-common-io")], + &[ + ( + "architecture-baseline.toml", + format!("[file_size]\n{listed}"), + ), + ("base-baseline.toml", base.clone()), + ], + ); + + let ctx = load(ws.path(), Some(base)); + + let found: Vec<_> = check(&ctx) + .iter() + .filter(|finding| finding.rule == "crate_names") + .map(|finding| finding.behaviour) + .collect(); + assert_eq!(found, *expected, "{name}"); + // However the copy reads, policy_changed's skip is reported once. + let policy: Vec = skipped(&ctx) + .into_iter() + .filter(|line| line.contains("crate_names.policy_changed")) + .collect(); + assert_eq!( + policy, + [POLICY_SKIPPED.trim_start_matches("archcheck: ").trim_end()], + "{name}" + ); + let skipped: Vec = skipped(&ctx) + .into_iter() + .filter(|line| { + line.contains("crate_names.no_new") + || matches!(ctx.base_baseline, BaseBaseline::Unusable(_)) + && line.starts_with("warning: ") + }) + .collect(); + match line { + None => assert_eq!(skipped, Vec::::new(), "{name}"), + Some(at) if at.starts_with("at line") => { + let path = ws.path().join("base-baseline.toml"); + let [line] = skipped.as_slice() else { + panic!("{name}: {skipped:?}"); + }; + let start = format!("warning: could not parse {} {at}", path.display()); + // A copy that is not TOML at all gets the one warning for + // every comparison skipped; no_new's findings say why. + let end = if matches!(ctx.base_baseline, BaseBaseline::Unusable(_)) { + "; file_size.no_new_entries and file_size.no_raised_caps skipped" + } else { + "; crate_names.no_new counts every [crate_names] entry as added" + }; + assert!( + line.starts_with(&start) && line.ends_with(end) && !line.contains('\n'), + "{name}: {line}" + ); + } + Some(line) => assert_eq!(skipped, [*line], "{name}"), + } + let base = ws.path().join("base-baseline.toml"); + let run = run(ws.path(), &["--base-baseline", base.to_str().unwrap()]); + assert_eq!(run.code, *code, "{name}: {}{}", run.stdout, run.stderr); + } +} + +/// `--write-baseline` keeps each entry whose crate still matches, removes +/// the others, adds none, and leaves an entry whose crate is at another path +/// as it is. `[crate_names]` is written even when empty. +#[test] +fn crate_names_write_baseline() { + let rows: &[(Pairs, Pairs, &str, &[&str])] = &[ + ( + &[ + ("crates/x", "pplx-space-common-io"), + ("crates/new", "pplx-space-common-jobs"), + ("crates/y", "pplx-space-io-buffers"), + ("crates/libs/foo-utils", "pplx-space-foo-utils"), + ], + &[ + ("pplx-space-common-io", "crates/x"), + ("pplx-space-common-jobs", "crates/old"), + ("pplx-space-io-buffers", "crates/y"), + ("pplx-space-gone-core", "crates/gone"), + ], + "[file_size]\n[crate_names]\npplx-space-common-io = \"crates/x\"\n\ + pplx-space-common-jobs = \"crates/old\"\n", + &[ + "not added: pplx-space-foo-utils (crates/libs/foo-utils/Cargo.toml) uses \ + `utils`, a catch-all name; name the crate for what it does", + "not updated: pplx-space-common-jobs is listed at crates/old but is at \ + crates/new; update the entry's path by hand if it moved", + ], + ), + ( + &[("crates/y", "pplx-space-io-buffers")], + &[("pplx-space-gone-core", "crates/gone")], + "[file_size]\n[crate_names]\n", + &[], + ), + ]; + for (packages, entries, expected, expected_not_added) in rows { + let ws = crate_workspace( + packages, + &[("architecture-baseline.toml", tables(&[], entries))], + ); + + let written = write_baseline(ws.path()).unwrap(); + + let mut not_added = written.not_added.clone(); + not_added.sort(); + assert_eq!(not_added, *expected_not_added); + let text = fs::read_to_string(ws.path().join("architecture-baseline.toml")).unwrap(); + assert!( + text.contains("\n\n# [crate_names] maps each workspace package"), + "{text}" + ); + let body: String = text + .lines() + .filter(|line| !line.starts_with('#') && !line.is_empty()) + .map(|line| format!("{line}\n")) + .collect(); + assert_eq!(body, *expected); + + let run = run(ws.path(), &["--write-baseline"]); + assert_eq!(run.code, 0, "{}", run.stderr); + assert_eq!( + fs::read_to_string(ws.path().join("architecture-baseline.toml")).unwrap(), + text, + "a generated baseline is written back unchanged" + ); + } +} + +/// `--write-baseline=crate_names` updates only `[crate_names]`, and lists no +/// entries to paste for a crate it did not add; `--write-baseline=file_size` +/// keeps `[crate_names]` as it is. +#[test] +fn crate_names_scoped_write_baseline() { + const BASELINE: &str = "architecture-baseline.toml"; + let ws = crate_workspace( + &[ + ("crates/x", "pplx-space-common-io"), + ("crates/libs/foo-utils", "pplx-space-foo-utils"), + ], + &[ + ( + BASELINE, + "[file_size]\n\"crates/a/src/big.rs\" = 9\n\n[crate_names]\n\ + pplx-space-common-io = \"crates/x\"\npplx-space-gone-core = \"crates/gone\"\n" + .to_owned(), + ), + ("crates/a/src/big.rs", code(LIMIT + 1)), + ], + ); + + let wrote = run(ws.path(), &["--write-baseline=crate_names"]); + + assert_eq!(wrote.code, 0, "{}", wrote.stderr); + let output = write_output(&wrote.stdout); + assert_eq!( + output.not_added, + [ + "not added: pplx-space-foo-utils (crates/libs/foo-utils/Cargo.toml) uses `utils`, a \ + catch-all name; name the crate for what it does" + ] + ); + assert!( + output + .summary + .starts_with("archcheck: wrote 1 crate_names entries to ") + && output.summary.ends_with("; other tables kept"), + "{}", + wrote.stdout + ); + assert_eq!(output.entries, ""); + let after = fs::read_to_string(ws.path().join(BASELINE)).unwrap(); + assert!( + after.contains("[file_size]\n\"crates/a/src/big.rs\" = 9\n"), + "{after}" + ); + assert!( + after.ends_with("[crate_names]\npplx-space-common-io = \"crates/x\"\n"), + "{after}" + ); + + let wrote = run(ws.path(), &["--write-baseline=file_size"]); + + assert_eq!(wrote.code, 0, "{}", wrote.stderr); + let output = write_output(&wrote.stdout); + assert!( + output + .summary + .starts_with("archcheck: wrote 1 file_size entries to "), + "{}", + wrote.stdout + ); + let kept = fs::read_to_string(ws.path().join(BASELINE)).unwrap(); + assert!( + kept.contains("[file_size]\n\"crates/a/src/big.rs\" = 6\n"), + "{kept}" + ); + assert!( + kept.ends_with("[crate_names]\npplx-space-common-io = \"crates/x\"\n"), + "{kept}" + ); +} + +/// A new catch-all crate is an `ERROR` on its manifest's `name =` line and +/// exits 1; a new baseline entry is one on the entry's line; a stale entry is +/// a `WARNING` there. +#[test] +fn crate_names_rdjson_and_exit_codes() { + let packages = [ + ("crates/x", "pplx-space-common-io"), + ("crates/libs/foo-utils", "pplx-space-foo-utils"), + ]; + let baseline = "[file_size]\n\n[crate_names]\n# A comment.\n\"pplx-space-common-io\" = \ + \"crates/x\"\n'pplx-space-gone-utils' = \"crates/gone\"\n"; + let ws = crate_workspace( + &packages, + &[ + ( + "crates/libs/foo-utils/Cargo.toml", + format!("# A comment.\n\n{}", package("pplx-space-foo-utils")), + ), + ("architecture-baseline.toml", baseline.to_owned()), + ], + ); + let diagnostic = |path: &str, line: u64, severity: &str, behaviour: &str| { + json!({ + "path": path, + "line": line, + "severity": severity, + "code": format!("crate_names.{behaviour}"), + }) + }; + + let document = rdjson( + ws.path(), + Base::Names(&["pplx-space-gone-utils"]).text().as_deref(), + ); + + let diagnostics: Vec = document["diagnostics"] + .as_array() + .unwrap() + .iter() + .map(|d| { + json!({ + "path": d["location"]["path"], + "line": d["location"]["range"]["start"]["line"], + "severity": d["severity"], + "code": d["code"]["value"], + }) + }) + .collect(); + assert_eq!( + diagnostics, + [ + diagnostic("crates/libs/foo-utils/Cargo.toml", 4, "ERROR", "no_new"), + diagnostic("architecture-baseline.toml", 5, "ERROR", "no_new"), + diagnostic("architecture-baseline.toml", 6, "WARNING", "stale_entries"), + ] + ); + + let run_human = run(ws.path(), &[]); + assert_eq!(run_human.code, 1, "{}", run_human.stderr); + assert!( + run_human.stdout.contains( + "crate_names.no_new (required):\n crates/libs/foo-utils/Cargo.toml: \ + `pplx-space-foo-utils` uses `utils`" + ), + "{}", + run_human.stdout + ); + + // Only the advisory stale entry left. + let ws = crate_workspace( + &packages[..1], + &[ + ("architecture-baseline.toml", baseline.to_owned()), + ( + "base.toml", + tables( + &[], + &[ + ("pplx-space-common-io", "crates/x"), + ("pplx-space-gone-utils", "crates/gone"), + ], + ), + ), + ], + ); + let base = ws.path().join("base.toml"); + let run_stale = run( + ws.path(), + &[ + "--format", + "rdjson", + "--base-baseline", + base.to_str().unwrap(), + ], + ); + assert_eq!(run_stale.code, 0, "{}", run_stale.stderr); + let document: Value = serde_json::from_str(&run_stale.stdout).unwrap(); + assert_eq!(document["diagnostics"][0]["severity"], "WARNING"); +} + /// One finding per behaviour, each reported in its own mode; only required /// findings exit 1. Without `--base-baseline`, no_new_entries is skipped and /// the summary says so. @@ -594,10 +1642,20 @@ fn modes_and_exit_codes() { let skipped = SKIPPED; let summary = "archcheck: no findings (0 production Rust files checked)\n"; let base = clean.path().join("architecture-baseline.toml"); + let architecture = clean.path().join("ARCHITECTURE.toml"); for (args, stdout) in [ (vec![], format!("{summary}{skipped}")), ( vec!["--base-baseline", base.to_str().unwrap()], + format!("{summary}{POLICY_SKIPPED}"), + ), + ( + vec![ + "--base-baseline", + base.to_str().unwrap(), + "--base-architecture", + architecture.to_str().unwrap(), + ], summary.to_owned(), ), ] { @@ -690,8 +1748,9 @@ fn configuration_errors() { }; let valid_instance = instance(&[r"\bold_table\b"], &["crates/a/**"]); let parse = "could not parse"; - let glob = "contains glob characters"; - let unnormalized = "has an empty, `.` or `..` component"; + let cargo = "`cargo metadata` failed"; + let crate_names = |key: &str, value: &str| Some(with_crate_names(key, value)); + let tokens = |list: &str| crate_names("banned_tokens", list); let cases: &[(&str, &str, Option, &str)] = &[ ( "unknown behaviour key", @@ -708,7 +1767,7 @@ fn configuration_errors() { ( "unknown rule", CONFIG, - Some(format!("{advisory}[rules.crate_names]\n")), + Some(format!("{advisory}[rules.module_names]\n")), parse, ), ( @@ -723,6 +1782,69 @@ fn configuration_errors() { Some(advisory.replace("[rules.file_size.stale_entries]\nmode = \"advisory\"\n", "")), parse, ), + ( + "empty banned tokens", + CONFIG, + tokens("[]"), + "banned_tokens is empty", + ), + ( + "banned token with -", + CONFIG, + tokens("[\"util\", \"foo-bar\"]"), + "banned token \"foo-bar\" can never match", + ), + ( + "banned token with _", + CONFIG, + tokens("[\"foo_bar\"]"), + "banned token \"foo_bar\" can never match", + ), + ( + "uppercase banned token", + CONFIG, + tokens("[\"Common\"]"), + "banned token \"Common\" can never match", + ), + ( + "empty banned token", + CONFIG, + tokens("[\"\"]"), + "banned token \"\" can never match", + ), + ( + "strip_prefix", + CONFIG, + Some(advisory.replace("allow = []", "allow = []\nstrip_prefix = \"x-\"")), + "unknown field `strip_prefix`", + ), + ( + "missing allow", + CONFIG, + Some(advisory.replace("allow = []\n", "")), + "missing field `allow`", + ), + ( + "uppercase allow entry", + CONFIG, + crate_names("allow", "[\"pplx-space-Shared\"]"), + "allow entry \"pplx-space-Shared\" is not a lowercase package name", + ), + ( + "duplicate allow entry", + CONFIG, + crate_names("allow", "[\"x-shared\", \"y\", \"x-shared\"]"), + "allow lists \"x-shared\" twice", + ), + ( + "missing policy_changed", + CONFIG, + Some(advisory.replace( + "[rules.crate_names.policy_changed]\nmode = \"advisory\"\n", + "", + )), + "missing field `policy_changed`", + ), ("unparseable", CONFIG, text("[rules.file_size\n"), parse), ( "bad regex", @@ -791,6 +1913,7 @@ fn configuration_errors() { parse, ), ("missing baseline", BASELINE, None, "could not read"), + ("missing member", MANIFEST, members("crates/c"), cargo), ( "baseline table for no instance", BASELINE, @@ -804,58 +1927,34 @@ fn configuration_errors() { parse, ), ( - "glob member", - MANIFEST, - members("crates/*"), - "member crates/* contains glob characters", - ), - ("glob ?", MANIFEST, members("crates/b?"), glob), - ("glob [", MANIFEST, members("crates/[b"), glob), - ("glob ]", MANIFEST, members("crates/b]"), glob), - ("glob {", MANIFEST, members("crates/{b"), glob), - ("glob }", MANIFEST, members("crates/b}"), glob), - ( - "member with ..", - MANIFEST, - members("crates/b/../b"), - unnormalized, - ), - ( - "member with .", - MANIFEST, - members("crates/./b"), - unnormalized, - ), - ( - "member with //", + "unparseable root manifest", MANIFEST, - members("crates//b"), - unnormalized, + text("[workspace\n"), + cargo, ), ( - "member ending in /", + "root manifest without a workspace", MANIFEST, - members("crates/b/"), - unnormalized, + text("[x]\n"), + cargo, ), ( - "absolute member", - MANIFEST, - members("/crates/b"), - unnormalized, + "member without a package name", + "crates/a/Cargo.toml", + text("[package]\nversion = \"0.1.0\"\n"), + cargo, ), - ("empty member", MANIFEST, members(""), unnormalized), ( - "missing member", - MANIFEST, - members("crates/c"), - "member crates/c has no Cargo.toml", + "member without [package]", + "crates/b/Cargo.toml", + text("[lib]\n"), + cargo, ), ( - "no members", - MANIFEST, - text("[workspace]\n"), - "has no [workspace] members list", + "member without a target", + "crates/b/Cargo.toml", + text("[package]\nname = \"b\"\n"), + "no targets specified", ), ]; for (name, file, contents, problem) in cases { @@ -890,8 +1989,8 @@ fn configuration_errors() { ), ( "Cargo.toml", - "[workspace]\nmembers = [\"crates/a/../a\"]\n", - "Cargo.toml: workspace member \"crates/a/../a\" has", + "[workspace]\nmembers = [\"crates/zz\"]\n", + "`cargo metadata` failed in ", ), ]; for (file, contents, problem) in cases { @@ -961,13 +2060,13 @@ fn write_baseline_output() { ("crates/a/src/tests.rs", LIMIT + 9), ], "[file_size]\n\"crates/a/src/grew.rs\" = 8\n\"crates/a/src/same.rs\" = 7\n\ - \"crates/a/src/shrank.rs\" = 6\n", + \"crates/a/src/shrank.rs\" = 6\n[crate_names]\n", vec![not_added("crates/b/src/new.rs", 12)], ), ( &[], &[("crates/a/src/ok.rs", LIMIT)], - "[file_size]\n", + "[file_size]\n[crate_names]\n", vec![], ), ]; @@ -1018,7 +2117,7 @@ fn write_baseline_output() { let (listed, wrote) = run.stdout.split_once('\n').unwrap(); assert_eq!(listed, not_added("crates/a/src/big.rs", 6)); assert!( - wrote.starts_with("archcheck: wrote 1 file_size entries to "), + wrote.starts_with("archcheck: wrote 1 file_size, 0 crate_names entries to "), "{}", run.stdout ); @@ -1074,7 +2173,10 @@ fn base_baseline_problems() { serde_json::from_str::(&run.stdout).expect("one JSON document"); run.stderr }; - let warning = summary.lines().last().unwrap_or_default(); + let warning = summary + .lines() + .find(|line| line.contains("file_size.")) + .unwrap_or_default(); assert!( warning.starts_with(&format!("archcheck: warning: {problem} {path} at line ")) && warning.ends_with("file_size.no_raised_caps skipped"), @@ -1240,6 +2342,16 @@ fn arguments() { 2, "archcheck: --base-baseline cannot be combined", ), + ( + &["--write-baseline", "--base-architecture", "base.toml"], + 2, + "archcheck: --base-architecture cannot be combined with --write-baseline", + ), + ( + &["--base-architecture"], + 2, + "archcheck: --base-architecture needs a value", + ), ( &["--bogus"], 2, @@ -1924,6 +3036,7 @@ fn pattern_ratchet_base_baseline() { let path = ws.path().join("base-baseline.toml"); let (info, skipped): (Vec, Vec) = skipped(&ctx) .into_iter() + .filter(|line| !line.contains("crate_names.")) .partition(|line| line.starts_with("base baseline has ")); let expected_info: Vec = ignored .iter() @@ -2052,12 +3165,16 @@ fn pattern_ratchet_base_table_that_does_not_parse_skips_only_its_instance() { let path = ws.path().join("base-baseline.toml"); assert_eq!( skipped(&ctx), - [format!( - "warning: could not parse {} at line 2: invalid type: string \"1\", expected usize; \ - pattern_ratchet.alpha.no_new_entries and pattern_ratchet.alpha.no_raised_caps \ - skipped", - path.display() - )] + [ + format!( + "warning: could not parse {} at line 2: invalid type: string \"1\", expected \ + usize; pattern_ratchet.alpha.no_new_entries and \ + pattern_ratchet.alpha.no_raised_caps skipped", + path.display() + ), + CRATE_NAMES_SKIPPED[0].to_owned(), + CRATE_NAMES_SKIPPED[1].to_owned(), + ] ); } @@ -2114,6 +3231,8 @@ fn skip_lines_come_in_rule_then_instance_order() { "base baseline has [pattern_ratchet.delta], which is not configured; skipped" .to_owned(), "base baseline has [pattern_ratchet.zeta], which is not configured; skipped".to_owned(), + CRATE_NAMES_SKIPPED[0].to_owned(), + CRATE_NAMES_SKIPPED[1].to_owned(), ] ); } @@ -2279,8 +3398,8 @@ fn pattern_ratchet_write_baseline() { ); assert!( output.summary.starts_with( - "archcheck: wrote 1 file_size, 3 pattern_ratchet.old, 0 pattern_ratchet.unused \ - entries to " + "archcheck: wrote 1 file_size, 3 pattern_ratchet.old, 0 pattern_ratchet.unused, \ + 0 crate_names entries to " ), "{}", wrote.stdout @@ -2302,7 +3421,7 @@ fn pattern_ratchet_write_baseline() { body, "[file_size]\n\"crates/b/src/sub/big.rs\" = 7\n[pattern_ratchet.old]\n\ \"crates/a/src/one.rs\" = 1\n\"crates/a/src/z.rs\" = 2\n\ - \"crates/b/src/sub/big.rs\" = 2\n[pattern_ratchet.unused]\n" + \"crates/b/src/sub/big.rs\" = 2\n[pattern_ratchet.unused]\n[crate_names]\n" ); // Only the file left out is still a finding. let checked = run(ws.path(), &[]); @@ -2359,7 +3478,7 @@ fn pattern_ratchet_write_creates_no_table() { ) } else { format!( - "archcheck: wrote 0 file_size, 1 pattern_ratchet.old entries to {}", + "archcheck: wrote 0 file_size, 1 pattern_ratchet.old, 0 crate_names entries to {}", ws.path().join(BASELINE).display() ) }; @@ -2524,8 +3643,8 @@ fn pattern_ratchet_scoped_write_baseline() { assert_eq!(generated.code, 0, "{}", generated.stderr); assert!( generated.stdout.starts_with( - "archcheck: wrote 1 file_size, 1 pattern_ratchet.old, 1 pattern_ratchet.other \ - entries to " + "archcheck: wrote 1 file_size, 1 pattern_ratchet.old, 1 pattern_ratchet.other, \ + 0 crate_names entries to " ), "{}", generated.stdout @@ -2727,7 +3846,12 @@ fn pattern_ratchet_scoped_write_keeps_an_empty_table() { table_section(&after, "pattern_ratchet.old"), "[pattern_ratchet.old]\n\n" ); - assert!(after.ends_with("[pattern_ratchet.other]\n"), "{after}"); + // Only [crate_names], always written last, follows it. + assert!( + after.contains("[pattern_ratchet.other]\n\n# [crate_names] ") + && after.ends_with("\n[crate_names]\n"), + "{after}" + ); assert_eq!(codes(ws.path()), ["pattern_ratchet.old.no_crossing"]); // Pasted inside the existing table, as the comment says, the entries @@ -2809,7 +3933,8 @@ fn write_baseline_entries_paste_into_their_tables() { "file_size", "pattern_ratchet.fresh", "pattern_ratchet.old", - "pattern_ratchet.zz" + "pattern_ratchet.zz", + "crate_names" ] ); assert_eq!( @@ -2942,7 +4067,7 @@ fn write_baseline_takes_a_table_only_after_equals() { assert_eq!(run.code, 0, "{}", run.stderr); assert!( run.stdout - .starts_with("archcheck: wrote 0 file_size entries to ") + .starts_with("archcheck: wrote 0 file_size, 0 crate_names entries to ") && !run.stdout.contains("other tables kept"), "{}", run.stdout @@ -3045,9 +4170,11 @@ fn pattern_ratchet_errors_point_at_the_line() { config(ADVISORY), ratchet("old", patterns, allow, ADVISORY), ); - let line = 1 + text - .lines() - .position(|line| line.starts_with(&format!("{key} = "))) + // The last: [rules.crate_names], before the instance, has an allow too. + let lines: Vec<&str> = text.lines().collect(); + let line = 1 + lines + .iter() + .rposition(|line| line.starts_with(&format!("{key} = "))) .unwrap(); let column = key.len() + " = ".len() + 1; let ws = workspace(&[("ARCHITECTURE.toml", text)]); From 1eb58d8d0374df8057aea1454d2a27cf09911af2 Mon Sep 17 00:00:00 2001 From: Abhinav Chordia Date: Wed, 14 Oct 2026 03:36:26 +0000 Subject: [PATCH 2/2] ci: pass the base ARCHITECTURE.toml to archcheck The archcheck step now also copies the base's ARCHITECTURE.toml, the same way as its baseline, for crate_names.policy_changed. Its warnings say exactly what is skipped when a base file can't be read: the no_new_entries and no_raised_caps comparisons and policy_changed are skipped, and crate_names.no_new allows only the crates listed when it was added. order-archcheck-findings.sh orders crate_names findings like the others. policy_changed compares with the base's ARCHITECTURE.toml, so it leads with no_new_entries and no_raised_caps instead of coming last in report order. A stale [crate_names] entry is the pull request's own when the pull request changes the Cargo.toml of the member path the entry maps to (the key is a package name, not a file). Required findings are still annotated from archcheck's own report, in the run that fails the job. The README says so, and that every listed crate is a root `members` entry, so moving one switches reviewdog to nofilter. --- .github/workflows/rust-static-ci.yml | 49 +++++++++++++------- ci/scripts/order-archcheck-findings.sh | 51 ++++++++++++++------- ci/scripts/test-order-archcheck-findings.sh | 46 +++++++++++++++++-- crates/tools/archcheck/README.md | 27 +++++++---- 4 files changed, 128 insertions(+), 45 deletions(-) diff --git a/.github/workflows/rust-static-ci.yml b/.github/workflows/rust-static-ci.yml index 8b067c5c42..3952234702 100644 --- a/.github/workflows/rust-static-ci.yml +++ b/.github/workflows/rust-static-ci.yml @@ -168,11 +168,14 @@ jobs: # Pull requests only: archcheck writes its findings as rdjson, offline # from the clippy step's caches, even when clippy failed. Exit 1 means # required findings, which reviewdog decides on; anything else fails. - # no_new_entries and no_raised_caps compare with the baseline of the - # base: the first parent of the synthetic merge (fetch-depth 0 above), - # once resolve-migration-base.sh has checked the second is this pull - # request. If the base has no baseline, an empty one; if it can't be - # resolved, a warning and no comparison. + # The base is the first parent of the synthetic merge (fetch-depth 0 + # above), once resolve-migration-base.sh has checked the second is this + # pull request. no_new_entries, no_raised_caps and crate_names.no_new + # compare with its architecture-baseline.toml, and + # crate_names.policy_changed with its ARCHITECTURE.toml; an empty file + # stands in for one the base lacks. If the base can't be resolved, a + # warning: the first two and policy_changed are skipped, and no_new + # allows only the crates listed when it was added. - name: Check repository structure id: archcheck if: >- @@ -192,19 +195,33 @@ jobs: --env CARGO_TARGET_DIR=target/buildkite-lint --env PULL_REQUEST_HEAD "space-ci:${GITHUB_SHA}" bash -c ' sccache --start-server >/dev/null 2>&1 && export RUSTC_WRAPPER=sccache mkdir -p target && git config --global --add safe.directory /workspace - base_file=target/archcheck-base-baseline.toml base_args=() - if base="$(bash ci/scripts/resolve-migration-base.sh "" "${PULL_REQUEST_HEAD}")"; then - if ! git cat-file -e "${base}:architecture-baseline.toml" 2>/dev/null; then - echo "archcheck: ${base} has no architecture-baseline.toml; comparing with an empty one" - : >"${base_file}" && base_args=(--base-baseline "${base_file}") - elif git show "${base}:architecture-baseline.toml" >"${base_file}"; then - echo "archcheck: comparing with architecture-baseline.toml from ${base}" - base_args=(--base-baseline "${base_file}") + base_args=() missing=() + # Passes the base copy of a file, or an empty file if the base has none. + base_copy() { + local file="target/archcheck-base-$2" + if ! git cat-file -e "${base}:$2" 2>/dev/null; then + echo "archcheck: ${base} has no $2; comparing with an empty one" + : >"${file}" && base_args+=("--$1" "${file}") + elif git show "${base}:$2" >"${file}"; then + echo "archcheck: comparing with $2 from ${base}" + base_args+=("--$1" "${file}") + else + missing+=("$2") fi + } + if base="$(bash ci/scripts/resolve-migration-base.sh "" "${PULL_REQUEST_HEAD}")"; then + base_copy base-baseline architecture-baseline.toml + base_copy base-architecture ARCHITECTURE.toml + else + missing=(architecture-baseline.toml ARCHITECTURE.toml) fi - if ((${#base_args[@]} == 0)); then - echo "::warning title=archcheck::Could not resolve the base baseline; no_new_entries and no_raised_caps are skipped." - fi + for file in "${missing[@]}"; do + case "${file}" in + architecture-baseline.toml) skipped="no_new_entries and no_raised_caps are skipped, and crate_names.no_new allows only the crates listed when it was added" ;; + ARCHITECTURE.toml) skipped="crate_names.policy_changed is skipped" ;; + esac + echo "::warning title=archcheck::Could not read the base ${file}; ${skipped}." + done exec cargo arch --format rdjson "${base_args[@]}" >target/archcheck.rdjson' || status=$? if ((status > 1)); then echo "::error title=archcheck::cargo arch exited ${status}; see above."; exit 1; fi diff --git a/ci/scripts/order-archcheck-findings.sh b/ci/scripts/order-archcheck-findings.sh index 702a1e0d37..1f688627e2 100755 --- a/ci/scripts/order-archcheck-findings.sh +++ b/ci/scripts/order-archcheck-findings.sh @@ -16,16 +16,23 @@ # this a pull request's own findings can be pushed out by the tree's. # # The pull request's own findings, in this order, each in report order: -# - no_new_entries and no_raised_caps findings. archcheck reports these only -# for entries that differ from the base branch's copy of -# architecture-baseline.toml, by table and key, so they include the -# entries of a renamed or split table. +# - Findings that compare with the base branch: no_new_entries and +# no_raised_caps, which archcheck reports only for entries that differ from +# the base's copy of architecture-baseline.toml, by table and key, so they +# include the entries of a renamed or split table; and +# crate_names.policy_changed, which it reports only for differences from the +# base's ARCHITECTURE.toml. # - Findings in a file the pull request changes, under its old or new name. -# archcheck's only other findings in architecture-baseline.toml are stale -# entries for files that are gone, so one there is the pull request's when -# its entry names a file it changes. An entry's key may be indented and a -# basic or literal string; a basic string with an escape JSON lacks (\e, -# \U) names no file. +# archcheck's other findings in architecture-baseline.toml are stale +# entries, and crate_names.no_new findings on [crate_names] entries. One +# there is the pull request's when its entry names a file it changes: the +# key of a file entry, or, in [crate_names], the Cargo.toml of the member +# path the entry maps to. A key or path may be indented and a basic or +# literal string; a basic string with an escape JSON lacks (\e, \U) names +# no file. +# +# Required (ERROR) findings are ordered too, but the workflow annotates them +# from archcheck's own report, in a run of their own that fails the job. # # reviewdog's -filter-mode=file keeps findings in files whose diff has a # +++ line: those with changes, under their new name. A pure rename, a mode @@ -53,14 +60,26 @@ jq --arg baseline "${baseline}" --arg own "${own}" --arg q "'" \ [$pages[][]] as $listed | ([$listed[] | .filename, .previous_filename // empty] | INDEX(.)) as $files | ([$listed[] | select(.changes > 0) | .filename] | INDEX(.)) as $diffed - | def entry_path: - capture("^\\s*(?:\"(?(?:[^\"\\\\]|\\\\.)*)\"|\($q)(?[^\($q)]*)\($q))\\s*=") - | if .basic != null then (try ("\"" + .basic + "\"" | fromjson) catch null) else .literal end; - ([$head_baseline | split("\n") | to_entries[] - | select([.value | entry_path | select(. != null and $files[.] != null)] | length > 0) - | .key + 1 | tostring] + | def string_value: + if .basic != null then (try ("\"" + .basic + "\"" | fromjson) catch null) else .literal end; + def quoted: "(?:\"(?(?:[^\"\\\\]|\\\\.)*)\"|\($q)(?[^\($q)]*)\($q))"; + def entry_path: capture("^\\s*\(quoted)\\s*=") | string_value; + # The Cargo.toml of the member path a [crate_names] entry maps to. + def member_manifest: + capture("^[^=]+=\\s*\(quoted)\\s*(?:#.*)?$") | string_value + | select(. != null) | if . == "" then "Cargo.toml" else . + "/Cargo.toml" end; + def table_header: capture("^\\s*\\[\\s*(?[^\\]\\s]+)\\s*\\]\\s*(?:#.*)?$").name; + ([foreach ($head_baseline | split("\n") | to_entries[]) as $line (null; + first($line.value | table_header) // .; + {table: ., number: ($line.key + 1), text: $line.value}) + | select(.table as $table + | [.text | if $table == "crate_names" then member_manifest else entry_path end + | select(. != null and $files[.] != null)] + | length > 0) + | .number | tostring] | INDEX(.)) as $named_lines - | def compares_with_base: (.code.value // "") | test("\\.(no_new_entries|no_raised_caps)$"); + | def compares_with_base: + (.code.value // "") | test("\\.(no_new_entries|no_raised_caps)$|^crate_names\\.policy_changed$"); def own: compares_with_base or if .location.path == $baseline then $named_lines[.location.range.start.line // 0 | tostring] != null diff --git a/ci/scripts/test-order-archcheck-findings.sh b/ci/scripts/test-order-archcheck-findings.sh index bb0570877c..505fe9545a 100755 --- a/ci/scripts/test-order-archcheck-findings.sh +++ b/ci/scripts/test-order-archcheck-findings.sh @@ -1,8 +1,9 @@ #!/usr/bin/env bash # Regression test for ci/scripts/order-archcheck-findings.sh: a pull request's -# own findings come first, those comparing with the base's baseline leading, -# each group in report order; a stale baseline entry is its own when it names -# a changed file; and --own counts only what reviewdog shows. +# own findings come first, those comparing with the base's baseline or +# ARCHITECTURE.toml leading, each group in report order; a stale baseline +# entry is its own when it names a changed file, or a [crate_names] entry the +# changed Cargo.toml of its member; and --own counts only what reviewdog shows. set -euo pipefail SCRIPT="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/order-archcheck-findings.sh" @@ -177,6 +178,45 @@ expect "file list" "new name.rs:1 ${B}:2 second.rs:1 third.rs:1 $(numbered tree expect "pure rename" "moved.rs:1 edited.rs:1 $(numbered tree 11)" moved.rdjson moved.json expect "pure rename, file mode" "edited.rs:1" --own=file moved.rdjson moved.json +# crate_names.policy_changed compares ARCHITECTURE.toml with the base's, so +# it leads with the other base comparisons; a required no_new finding on a new +# crate's Cargo.toml is the pull request's, and kept. +printf '[file_size]\n' >"${B}" +{ + tree_findings + diagnostic own.rs 1 file_size.listed_no_growth + diagnostic crates/new-utils/Cargo.toml 2 crate_names.no_new ERROR + diagnostic ARCHITECTURE.toml 80 crate_names.policy_changed + diagnostic ARCHITECTURE.toml 81 crate_names.stale_entries +} | report >policy.rdjson +pages ARCHITECTURE.toml own.rs crates/new-utils/Cargo.toml >policy.json +expect "crate_names policy" \ + "ARCHITECTURE.toml:80 own.rs:1 crates/new-utils/Cargo.toml:2 ARCHITECTURE.toml:81" \ + --own=nofilter policy.rdjson policy.json + +# A stale [crate_names] entry is the pull request's when it changes the +# Cargo.toml of the member path the entry maps to (old or new name), however +# the key and path are written; an empty path is the root package. +cat >"${B}" <<'EOF' +[file_size] +"crates/z/src/lib.rs" = 1300 + +[crate_names] +pplx-space-x-common = "crates/x" +"pplx-space-y-utils" = 'crates/y' # moved +pplx-space-z-core = "crates/z" +pplx-space-root-misc = "" +EOF +{ + tree_findings + for line in 5 6 7 8; do diagnostic "${B}" "${line}" crate_names.stale_entries; done +} | report >crates.rdjson +{ + printf '[{"filename": "crates/y2/Cargo.toml", "previous_filename": "crates/y/Cargo.toml", "status": "renamed", "changes": 0}]' + pages crates/x/Cargo.toml Cargo.toml crates/z/src/lib.rs +} >crates.json +expect "crate_names entries" "${B}:5 ${B}:6 ${B}:8 $(numbered tree 11) ${B}:7" crates.rdjson crates.json + if bash "${SCRIPT}" source.rdjson 2>/dev/null; then fail "the script accepted a missing argument" fi diff --git a/crates/tools/archcheck/README.md b/crates/tools/archcheck/README.md index abc0c7cb04..dc0d9c42a4 100644 --- a/crates/tools/archcheck/README.md +++ b/crates/tools/archcheck/README.md @@ -292,21 +292,28 @@ advisory ones `WARNING`. rest. First come all `no_new_entries` and `no_raised_caps` findings: they compare with the base's copy by table and key, so they include the entries of a renamed or split pattern table even though those lines are unchanged. + `crate_names.policy_changed` findings, which compare with the base's + `ARCHITECTURE.toml`, come with them, although the report lists them last. Then come findings in the files the pull request changes (old or new name), and stale entries in the baseline for files it changes, such as files it - deletes. Each group keeps report order. With `file`, this changes nothing - that is shown. The step logs how many own advisory findings reviewdog will - show under the step's filter mode (with `file`, none in a file renamed - without edits); with more than 10, it says in the step summary that at - most 10 are annotated. Annotations are not the full inventory: every - finding is in the step's log and its rdjson output, or run `cargo arch`. + deletes, or, in `[crate_names]`, for a crate whose `Cargo.toml` it changes, + at the entry's member path. Each group keeps report order. With `file`, this + changes nothing that is shown. Required findings are annotated from the + report as it is, in their own run. The step logs how many own advisory + findings reviewdog will show under the step's filter mode (with `file`, + none in a file renamed without edits); with more than 10, it says in the + step summary that at most 10 are annotated. Annotations are not the full + inventory: every finding is in the step's log and its rdjson output, or run + `cargo arch`. A new catch-all crate's finding is on its own `Cargo.toml`, a file the pull request adds, so it is annotated in either mode and its `ERROR` fails the job. -The same holds for a crate moved to a listed name's new path or a renamed -library, whose manifest the pull request changes; a self-listed entry or a -changed `allow` changes the baseline or `ARCHITECTURE.toml`, which switches to -`nofilter`. +So is a renamed library's, whose manifest the pull request edits. Moving a +listed crate changes the root `Cargo.toml`'s `members`, where every listed +crate is today, and a self-listed entry or a changed `allow` changes the +baseline or `ARCHITECTURE.toml`: each switches to `nofilter`. A listed crate +that is only a path dependency, moved without editing its manifest, would not +be annotated with `file`; `tests/repository.rs` still fails on it. Only annotations are filtered. Once a behaviour is `required`, `tests/repository.rs` fails on any required finding anywhere in the tree.