From f4a670e68e28c127d80148b920064fafe8fe995d Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 26 Jul 2026 19:12:05 +0200 Subject: [PATCH 1/6] Broaden static-regex lint to cover once_cell Lazy (#410) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `check-static-regexes` guard only rejected `LazyLock::new(... Regex::new(...))`, so a hand-rolled static regex wrapped in `once_cell::sync::Lazy::new(|| Regex::new(...))` slipped past `make lint`. Extract the scan into `scripts/check-static-regexes.sh` as the single source of truth and broaden its pattern to reject both supported lazy-wrapper constructors — `std::sync::LazyLock::new` and `once_cell::sync::Lazy::new` — whether spelled directly or fully qualified. `lazy_regex!` remains the sole sanctioned idiom. Add `tests/static_regex_lint.rs`, which drives the script against fixtures for every supported wrapper form, asserts a clean source passes, and asserts a ripgrep scan failure propagates its exit status. Fixtures live under `tests/data/static_regex/` with a `.rs.txt` extension so the guard does not match them in place. Co-Authored-By: Claude Opus 4.8 (1M context) --- Makefile | 8 +- docs/developers-guide.md | 5 + scripts/check-static-regexes.sh | 37 +++++ tests/data/static_regex/clean.rs.txt | 2 + .../data/static_regex/lazylock_direct.rs.txt | 1 + .../static_regex/lazylock_qualified.rs.txt | 1 + .../static_regex/once_cell_lazy_direct.rs.txt | 1 + .../once_cell_lazy_qualified.rs.txt | 1 + tests/static_regex_lint.rs | 134 ++++++++++++++++++ 9 files changed, 183 insertions(+), 7 deletions(-) create mode 100755 scripts/check-static-regexes.sh create mode 100644 tests/data/static_regex/clean.rs.txt create mode 100644 tests/data/static_regex/lazylock_direct.rs.txt create mode 100644 tests/data/static_regex/lazylock_qualified.rs.txt create mode 100644 tests/data/static_regex/once_cell_lazy_direct.rs.txt create mode 100644 tests/data/static_regex/once_cell_lazy_qualified.rs.txt create mode 100644 tests/static_regex_lint.rs diff --git a/Makefile b/Makefile index fe353d71..2737cce9 100644 --- a/Makefile +++ b/Makefile @@ -42,13 +42,7 @@ check-ripgrep: ## Verify ripgrep is available } check-static-regexes: check-ripgrep ## Reject hand-rolled static regular expressions - @status=0; \ - $(RG) -U --glob '*.rs' '\bstatic\b[^;=]*=\s*(?:[[:alnum:]_]+::)*LazyLock::new\s*\(\s*\|\|\s*(\{\s*)?(?:[[:alnum:]_]+::)*Regex::new' . || status=$$?; \ - case $$status in \ - 0) echo "static regular expressions must use lazy_regex!"; exit 1 ;; \ - 1) ;; \ - *) echo "failed to scan Rust sources (rg exit $$status)" >&2; exit $$status ;; \ - esac + @RG='$(RG)' scripts/check-static-regexes.sh . markdownlint: ## Lint Markdown files $(MDLINT) "**/*.md" diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 10431ae2..06e192ec 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -67,6 +67,11 @@ restores the separator row with widths derived from the final table body. - `check-static-regexes`: Runs before Clippy as part of `make lint` and uses ripgrep (`rg`) to reject hand-rolled static regular expression declarations. + The scan lives in `scripts/check-static-regexes.sh` and rejects any `static` + that wraps `Regex::new` directly in a supported lazy-wrapper constructor — + `std::sync::LazyLock::new` or `once_cell::sync::Lazy::new`, whether spelled + directly or fully qualified — so `lazy_regex!` remains the sole sanctioned + idiom. `tests/static_regex_lint.rs` exercises every supported form. Contributors must install ripgrep locally; Continuous Integration (CI) installs the pinned version before running the lint gate. diff --git a/scripts/check-static-regexes.sh b/scripts/check-static-regexes.sh new file mode 100755 index 00000000..b6f7a349 --- /dev/null +++ b/scripts/check-static-regexes.sh @@ -0,0 +1,37 @@ +#!/usr/bin/env bash +# Reject hand-rolled static regular expressions that bypass the `lazy_regex!` +# convention. +# +# The guard scans Rust sources for `static` declarations that wrap `Regex::new` +# directly in a supported lazy-wrapper constructor. Two wrapper families are +# supported, each matched whether spelled directly or fully qualified: +# +# * `std::sync::LazyLock::new` +# * `once_cell::sync::Lazy::new` +# +# Usage: check-static-regexes.sh [SCAN_DIR] +# +# SCAN_DIR defaults to the current directory. The RG environment variable +# overrides the ripgrep executable (default: `rg`). +# +# Exit status: +# 0 no prohibited declaration found +# 1 a prohibited declaration was found (diagnostic on stdout) +# * ripgrep failed to scan (diagnostic on stderr; rg's status propagated) +set -euo pipefail + +RG="${RG:-rg}" +scan_dir="${1:-.}" + +# `(?:[[:alnum:]_]+::)*` absorbs any module qualification (for example the +# `once_cell::sync::` in `once_cell::sync::Lazy::new`), so both the direct and +# fully qualified spellings of each supported constructor are rejected. +pattern='\bstatic\b[^;=]*=\s*(?:[[:alnum:]_]+::)*(?:LazyLock|Lazy)::new\s*\(\s*\|\|\s*(\{\s*)?(?:[[:alnum:]_]+::)*Regex::new' + +status=0 +"$RG" -U --glob '*.rs' "$pattern" "$scan_dir" || status=$? +case $status in + 0) echo "static regular expressions must use lazy_regex!"; exit 1 ;; + 1) exit 0 ;; + *) echo "failed to scan Rust sources (rg exit $status)" >&2; exit "$status" ;; +esac diff --git a/tests/data/static_regex/clean.rs.txt b/tests/data/static_regex/clean.rs.txt new file mode 100644 index 00000000..783f83b6 --- /dev/null +++ b/tests/data/static_regex/clean.rs.txt @@ -0,0 +1,2 @@ +static RE: LazyLock = lazy_regex!("clean"); +fn build() { let _ = Regex::new("local").unwrap(); } diff --git a/tests/data/static_regex/lazylock_direct.rs.txt b/tests/data/static_regex/lazylock_direct.rs.txt new file mode 100644 index 00000000..3760b95e --- /dev/null +++ b/tests/data/static_regex/lazylock_direct.rs.txt @@ -0,0 +1 @@ +static RE: LazyLock = LazyLock::new(|| Regex::new("a").unwrap()); diff --git a/tests/data/static_regex/lazylock_qualified.rs.txt b/tests/data/static_regex/lazylock_qualified.rs.txt new file mode 100644 index 00000000..758fb742 --- /dev/null +++ b/tests/data/static_regex/lazylock_qualified.rs.txt @@ -0,0 +1 @@ +static RE: LazyLock = std::sync::LazyLock::new(|| Regex::new("b").unwrap()); diff --git a/tests/data/static_regex/once_cell_lazy_direct.rs.txt b/tests/data/static_regex/once_cell_lazy_direct.rs.txt new file mode 100644 index 00000000..a601be38 --- /dev/null +++ b/tests/data/static_regex/once_cell_lazy_direct.rs.txt @@ -0,0 +1 @@ +static RE: Lazy = Lazy::new(|| Regex::new("c").unwrap()); diff --git a/tests/data/static_regex/once_cell_lazy_qualified.rs.txt b/tests/data/static_regex/once_cell_lazy_qualified.rs.txt new file mode 100644 index 00000000..9b2879df --- /dev/null +++ b/tests/data/static_regex/once_cell_lazy_qualified.rs.txt @@ -0,0 +1 @@ +static RE: Lazy = once_cell::sync::Lazy::new(|| Regex::new("d").unwrap()); diff --git a/tests/static_regex_lint.rs b/tests/static_regex_lint.rs new file mode 100644 index 00000000..c1cad98a --- /dev/null +++ b/tests/static_regex_lint.rs @@ -0,0 +1,134 @@ +//! Regression coverage for the `check-static-regexes` lint guard. +//! +//! The guard lives in `scripts/check-static-regexes.sh` (invoked by the +//! Makefile's `check-static-regexes` target). It rejects hand-rolled static +//! regular expressions that bypass the `lazy_regex!` convention by wrapping +//! `Regex::new` directly in a supported lazy-wrapper constructor. +//! +//! These tests exercise the script directly so the Makefile and the tests +//! share a single source of truth for the scan. Every supported wrapper form +//! is asserted to be rejected, a clean fixture is asserted to pass, and a +//! ripgrep scan failure is asserted to propagate. +//! +//! Fixtures live under `tests/data/static_regex/` with a `.rs.txt` extension +//! so the guard (which scans `*.rs`) does not match the fixtures in place; the +//! tests copy each fixture into a temporary directory as a `.rs` file before +//! scanning. + +use std::{ + path::{Path, PathBuf}, + process::Command, +}; + +use rstest::rstest; +use tempfile::TempDir; + +/// The diagnostic emitted when a prohibited declaration is found. +const PROHIBITED_DIAGNOSTIC: &str = "static regular expressions must use lazy_regex!"; + +/// Every lazy-wrapper constructor form the guard must reject when it directly +/// wraps `Regex::new`. Each label maps to a fixture under +/// `tests/data/static_regex/` exercising one supported spelling: +/// +/// * `lazylock_direct` — `LazyLock::new(|| Regex::new(...))` +/// * `lazylock_qualified` — `std::sync::LazyLock::new(|| Regex::new(...))` +/// * `once_cell_lazy_direct` — `Lazy::new(|| Regex::new(...))` +/// * `once_cell_lazy_qualified` — `once_cell::sync::Lazy::new(|| Regex::new(...))` +const PROHIBITED_FORMS: &[&str] = &[ + "lazylock_direct", + "lazylock_qualified", + "once_cell_lazy_direct", + "once_cell_lazy_qualified", +]; + +fn manifest_dir() -> PathBuf { PathBuf::from(env!("CARGO_MANIFEST_DIR")) } + +fn script_path() -> PathBuf { manifest_dir().join("scripts/check-static-regexes.sh") } + +fn fixture(label: &str) -> String { + let path = manifest_dir().join(format!("tests/data/static_regex/{label}.rs.txt")); + std::fs::read_to_string(&path) + .unwrap_or_else(|e| panic!("failed to read fixture {}: {e}", path.display())) +} + +/// Materialise `label`'s fixture as a `.rs` file inside a fresh temp directory. +fn scan_dir_with(label: &str) -> TempDir { + let dir = TempDir::new().expect("failed to create temp dir"); + std::fs::write(dir.path().join(format!("{label}.rs")), fixture(label)) + .expect("failed to write fixture into temp dir"); + dir +} + +/// Run the guard against `scan_dir`, optionally overriding the ripgrep binary +/// via the `RG` environment variable. +fn run_guard(scan_dir: &Path, rg: Option<&Path>) -> std::process::Output { + let mut cmd = Command::new(script_path()); + cmd.arg(scan_dir); + if let Some(rg) = rg { + cmd.env("RG", rg); + } + cmd.output() + .expect("failed to execute check-static-regexes.sh") +} + +#[rstest] +fn rejects_prohibited_lazy_wrapper_form(#[values(0, 1, 2, 3)] index: usize) { + let label = PROHIBITED_FORMS[index]; + let dir = scan_dir_with(label); + + let output = run_guard(dir.path(), None); + + assert_eq!( + output.status.code(), + Some(1), + "form `{label}` should be rejected with status 1" + ); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains(PROHIBITED_DIAGNOSTIC), + "form `{label}` should emit the prohibited diagnostic, got: {stdout}" + ); +} + +#[test] +fn accepts_clean_sources() { + // The sanctioned `lazy_regex!` idiom plus an unrelated non-static + // `Regex::new` call that must not trip the guard. + let dir = scan_dir_with("clean"); + + let output = run_guard(dir.path(), None); + + assert_eq!( + output.status.code(), + Some(0), + "clean sources should pass; stdout: {}", + String::from_utf8_lossy(&output.stdout) + ); +} + +#[test] +fn propagates_ripgrep_scan_failure() { + let dir = TempDir::new().expect("failed to create temp dir"); + // A stub standing in for ripgrep that fails with a distinctive status. + let stub = dir.path().join("rg-stub.sh"); + std::fs::write(&stub, "#!/bin/sh\nexit 3\n").expect("failed to write stub"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&stub, std::fs::Permissions::from_mode(0o755)) + .expect("failed to chmod stub"); + } + + let output = run_guard(dir.path(), Some(&stub)); + + assert_eq!( + output.status.code(), + Some(3), + "a ripgrep scan failure should propagate its exit status" + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("failed to scan Rust sources (rg exit 3)"), + "scan failure should emit the scan-failure diagnostic, got: {stderr}" + ); +} From 53bf71358d2d5db4d2e60c0889798d57e9dcb284 Mon Sep 17 00:00:00 2001 From: leynos Date: Mon, 27 Jul 2026 20:04:06 +0200 Subject: [PATCH 2/6] Harden static-regex guard: RG args and move closures (#410) Address review findings on the broadened static-regex lint: - Restore support for `RG` overrides that carry arguments (for example `RG='rg --pcre2'`). Extracting the scan into the shell script replaced the Makefile's word-splitting `$(RG)` expansion with a quoted `"$RG"`, which treated the whole value as one executable name. Split `RG` into an array so arguments are preserved, matching `check-ripgrep`'s `firstword` handling. - Reject `move` closures. The pattern only matched `|| Regex::new(...)`, so a hand-rolled `LazyLock::new(move || Regex::new(...))` (or the `once_cell` `Lazy` equivalent) slipped through. Add `(?:move\s+)?` and fixtures for both wrapper families. - Make the regression tests deterministic: `run_guard` now clears any ambient `RG` on default-path runs so the guard's own `rg` default is exercised. Skipped the suggestion to migrate the test to camino/cap-std path types: it would add two dependencies absent from the tree and diverge from the existing test suite, which uses std::fs and tempfile. Co-Authored-By: Claude Opus 4.8 (1M context) --- scripts/check-static-regexes.sh | 11 +++++++---- tests/data/static_regex/lazylock_move.rs.txt | 1 + .../data/static_regex/once_cell_lazy_move.rs.txt | 1 + tests/static_regex_lint.rs | 15 +++++++++++---- 4 files changed, 20 insertions(+), 8 deletions(-) create mode 100644 tests/data/static_regex/lazylock_move.rs.txt create mode 100644 tests/data/static_regex/once_cell_lazy_move.rs.txt diff --git a/scripts/check-static-regexes.sh b/scripts/check-static-regexes.sh index b6f7a349..987311eb 100755 --- a/scripts/check-static-regexes.sh +++ b/scripts/check-static-regexes.sh @@ -12,7 +12,9 @@ # Usage: check-static-regexes.sh [SCAN_DIR] # # SCAN_DIR defaults to the current directory. The RG environment variable -# overrides the ripgrep executable (default: `rg`). +# overrides the ripgrep command (default: `rg`). It is split on whitespace, so +# it may carry arguments — for example `RG='rg --pcre2'` — matching the way the +# Makefile's `$(RG)` expansion behaved before the scan was extracted here. # # Exit status: # 0 no prohibited declaration found @@ -20,16 +22,17 @@ # * ripgrep failed to scan (diagnostic on stderr; rg's status propagated) set -euo pipefail -RG="${RG:-rg}" +read -r -a rg_cmd <<<"${RG:-rg}" scan_dir="${1:-.}" # `(?:[[:alnum:]_]+::)*` absorbs any module qualification (for example the # `once_cell::sync::` in `once_cell::sync::Lazy::new`), so both the direct and # fully qualified spellings of each supported constructor are rejected. -pattern='\bstatic\b[^;=]*=\s*(?:[[:alnum:]_]+::)*(?:LazyLock|Lazy)::new\s*\(\s*\|\|\s*(\{\s*)?(?:[[:alnum:]_]+::)*Regex::new' +# `(?:move\s+)?` covers `move` closures such as `LazyLock::new(move || ...)`. +pattern='\bstatic\b[^;=]*=\s*(?:[[:alnum:]_]+::)*(?:LazyLock|Lazy)::new\s*\(\s*(?:move\s+)?\|\|\s*(\{\s*)?(?:[[:alnum:]_]+::)*Regex::new' status=0 -"$RG" -U --glob '*.rs' "$pattern" "$scan_dir" || status=$? +"${rg_cmd[@]}" -U --glob '*.rs' "$pattern" "$scan_dir" || status=$? case $status in 0) echo "static regular expressions must use lazy_regex!"; exit 1 ;; 1) exit 0 ;; diff --git a/tests/data/static_regex/lazylock_move.rs.txt b/tests/data/static_regex/lazylock_move.rs.txt new file mode 100644 index 00000000..75a1b363 --- /dev/null +++ b/tests/data/static_regex/lazylock_move.rs.txt @@ -0,0 +1 @@ +static RE: LazyLock = LazyLock::new(move || Regex::new("e").unwrap()); diff --git a/tests/data/static_regex/once_cell_lazy_move.rs.txt b/tests/data/static_regex/once_cell_lazy_move.rs.txt new file mode 100644 index 00000000..59ab1dd7 --- /dev/null +++ b/tests/data/static_regex/once_cell_lazy_move.rs.txt @@ -0,0 +1 @@ +static RE: Lazy = once_cell::sync::Lazy::new(move || Regex::new("f").unwrap()); diff --git a/tests/static_regex_lint.rs b/tests/static_regex_lint.rs index c1cad98a..21771321 100644 --- a/tests/static_regex_lint.rs +++ b/tests/static_regex_lint.rs @@ -32,13 +32,17 @@ const PROHIBITED_DIAGNOSTIC: &str = "static regular expressions must use lazy_re /// /// * `lazylock_direct` — `LazyLock::new(|| Regex::new(...))` /// * `lazylock_qualified` — `std::sync::LazyLock::new(|| Regex::new(...))` +/// * `lazylock_move` — `LazyLock::new(move || Regex::new(...))` /// * `once_cell_lazy_direct` — `Lazy::new(|| Regex::new(...))` /// * `once_cell_lazy_qualified` — `once_cell::sync::Lazy::new(|| Regex::new(...))` +/// * `once_cell_lazy_move` — `once_cell::sync::Lazy::new(move || Regex::new(...))` const PROHIBITED_FORMS: &[&str] = &[ "lazylock_direct", "lazylock_qualified", + "lazylock_move", "once_cell_lazy_direct", "once_cell_lazy_qualified", + "once_cell_lazy_move", ]; fn manifest_dir() -> PathBuf { PathBuf::from(env!("CARGO_MANIFEST_DIR")) } @@ -64,15 +68,18 @@ fn scan_dir_with(label: &str) -> TempDir { fn run_guard(scan_dir: &Path, rg: Option<&Path>) -> std::process::Output { let mut cmd = Command::new(script_path()); cmd.arg(scan_dir); - if let Some(rg) = rg { - cmd.env("RG", rg); - } + // Control the ripgrep dependency explicitly: override it for `Some`, and + // clear any ambient `RG` for `None` so default-path runs are deterministic. + match rg { + Some(rg) => cmd.env("RG", rg), + None => cmd.env_remove("RG"), + }; cmd.output() .expect("failed to execute check-static-regexes.sh") } #[rstest] -fn rejects_prohibited_lazy_wrapper_form(#[values(0, 1, 2, 3)] index: usize) { +fn rejects_prohibited_lazy_wrapper_form(#[values(0, 1, 2, 3, 4, 5)] index: usize) { let label = PROHIBITED_FORMS[index]; let dir = scan_dir_with(label); From ccc83dcbae3c1aef737fdd067501410616b0ab6f Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 31 Jul 2026 02:16:53 +0200 Subject: [PATCH 3/6] Cover argument-bearing RG overrides (#410) Address review findings on the static-regex lint regression tests: - Widen `run_guard`'s `rg` parameter from `Option<&Path>` to `Option<&str>` so tests can supply an `RG` value that carries arguments, and add `preserves_arguments_supplied_through_rg`. It drives the guard with `RG=' --pcre2'` and asserts, via a stub that records its argv, that the override's arguments are forwarded ahead of the guard's own and that the scan directory remains last. Reverting the array split makes this test fail with exit 127, the exact symptom the fix removed. - Extract the executable-stub setup into a `write_stub` helper shared by the scan-failure and argument-preservation tests. - Spell the `scan_dir_with` doc comment "Materialize", matching the repository's en-GB-oxendict Oxford-spelling convention. Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/static_regex_lint.rs | 89 +++++++++++++++++++++++++++++++------- 1 file changed, 74 insertions(+), 15 deletions(-) diff --git a/tests/static_regex_lint.rs b/tests/static_regex_lint.rs index 21771321..f91d86a0 100644 --- a/tests/static_regex_lint.rs +++ b/tests/static_regex_lint.rs @@ -55,7 +55,7 @@ fn fixture(label: &str) -> String { .unwrap_or_else(|e| panic!("failed to read fixture {}: {e}", path.display())) } -/// Materialise `label`'s fixture as a `.rs` file inside a fresh temp directory. +/// Materialize `label`'s fixture as a `.rs` file inside a fresh temp directory. fn scan_dir_with(label: &str) -> TempDir { let dir = TempDir::new().expect("failed to create temp dir"); std::fs::write(dir.path().join(format!("{label}.rs")), fixture(label)) @@ -63,13 +63,16 @@ fn scan_dir_with(label: &str) -> TempDir { dir } -/// Run the guard against `scan_dir`, optionally overriding the ripgrep binary -/// via the `RG` environment variable. -fn run_guard(scan_dir: &Path, rg: Option<&Path>) -> std::process::Output { +/// Run the guard against `scan_dir`, optionally overriding the `RG` ripgrep +/// command. +/// +/// `rg` is the raw `RG` value, so it may carry arguments (for example +/// `rg --pcre2`); the guard splits it on whitespace. Passing `None` clears any +/// ambient `RG` so default-path runs exercise the guard's own `rg` default +/// deterministically. +fn run_guard(scan_dir: &Path, rg: Option<&str>) -> std::process::Output { let mut cmd = Command::new(script_path()); cmd.arg(scan_dir); - // Control the ripgrep dependency explicitly: override it for `Some`, and - // clear any ambient `RG` for `None` so default-path runs are deterministic. match rg { Some(rg) => cmd.env("RG", rg), None => cmd.env_remove("RG"), @@ -78,6 +81,19 @@ fn run_guard(scan_dir: &Path, rg: Option<&Path>) -> std::process::Output { .expect("failed to execute check-static-regexes.sh") } +/// Write `script` to `/` and mark it executable. +fn write_stub(dir: &Path, name: &str, script: &str) -> PathBuf { + let path = dir.join(name); + std::fs::write(&path, script).expect("failed to write stub"); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o755)) + .expect("failed to chmod stub"); + } + path +} + #[rstest] fn rejects_prohibited_lazy_wrapper_form(#[values(0, 1, 2, 3, 4, 5)] index: usize) { let label = PROHIBITED_FORMS[index]; @@ -117,16 +133,9 @@ fn accepts_clean_sources() { fn propagates_ripgrep_scan_failure() { let dir = TempDir::new().expect("failed to create temp dir"); // A stub standing in for ripgrep that fails with a distinctive status. - let stub = dir.path().join("rg-stub.sh"); - std::fs::write(&stub, "#!/bin/sh\nexit 3\n").expect("failed to write stub"); - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - std::fs::set_permissions(&stub, std::fs::Permissions::from_mode(0o755)) - .expect("failed to chmod stub"); - } + let stub = write_stub(dir.path(), "rg-stub.sh", "#!/bin/sh\nexit 3\n"); - let output = run_guard(dir.path(), Some(&stub)); + let output = run_guard(dir.path(), Some(&stub.display().to_string())); assert_eq!( output.status.code(), @@ -139,3 +148,53 @@ fn propagates_ripgrep_scan_failure() { "scan failure should emit the scan-failure diagnostic, got: {stderr}" ); } + +/// An `RG` override may carry arguments — the Makefile's `$(RG)` expansion +/// supported this before the scan moved into the script, and `check-ripgrep` +/// still validates only `$(firstword $(RG))`. The guard must therefore split +/// `RG` on whitespace and forward the extra arguments to ripgrep ahead of its +/// own, rather than treating the whole value as one executable name. +#[test] +fn preserves_arguments_supplied_through_rg() { + let dir = TempDir::new().expect("failed to create temp dir"); + let argv_log = dir.path().join("argv.txt"); + // A stub that records its argv, then reports "no matches" so the guard + // takes its clean-scan path. + let stub = write_stub( + dir.path(), + "rg-stub.sh", + &format!( + "#!/bin/sh\nfor a in \"$@\"; do printf '%s\\n' \"$a\"; done > '{}'\nexit 1\n", + argv_log.display() + ), + ); + + let output = run_guard(dir.path(), Some(&format!("{} --pcre2", stub.display()))); + + assert_eq!( + output.status.code(), + Some(0), + "an argument-bearing RG override must still run; stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let argv: Vec = std::fs::read_to_string(&argv_log) + .expect("stub should have recorded its argv") + .lines() + .map(str::to_owned) + .collect(); + assert_eq!( + argv.first().map(String::as_str), + Some("--pcre2"), + "the RG override's own arguments must be forwarded first, got: {argv:?}" + ); + assert!( + argv.contains(&"-U".to_owned()) && argv.contains(&"--glob".to_owned()), + "the guard's own ripgrep arguments must follow, got: {argv:?}" + ); + assert_eq!( + argv.last().map(String::as_str), + Some(dir.path().to_str().expect("temp dir path should be UTF-8")), + "the scan directory must remain the final argument, got: {argv:?}" + ); +} From e496331a16e7234e53f357fa120c91888fcee563 Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 31 Jul 2026 12:26:05 +0200 Subject: [PATCH 4/6] Scope integration-test filesystem access (#418) Use UTF-8 paths and directory capabilities throughout file-backed integration tests. Centralize temporary-directory setup while keeping ambient access at explicit repository and process boundaries. Preserve existing command execution, fixtures, snapshots, and behavioural assertions. --- tests/cli.rs | 31 +++++------ tests/cli_frontmatter.rs | 20 +++++-- tests/cli_matrix.rs | 9 ++-- tests/cli_matrix/fixture_io.rs | 36 +++++++++++++ tests/cli_matrix/invariants.rs | 10 ++-- tests/cli_matrix/support.rs | 90 +++++++++++-------------------- tests/cli_matrix/support_tests.rs | 47 ++++++++++++++++ tests/code_emphasis.rs | 83 +++++++++------------------- tests/common/fs.rs | 35 ++++++++++++ tests/parallel.rs | 51 +++++++----------- tests/static_regex_lint.rs | 77 +++++++++++++++----------- tests/wrap/cli_files.rs | 14 ++--- tests/wrap_cli.rs | 16 +++--- 13 files changed, 294 insertions(+), 225 deletions(-) create mode 100644 tests/cli_matrix/fixture_io.rs create mode 100644 tests/cli_matrix/support_tests.rs create mode 100644 tests/common/fs.rs diff --git a/tests/cli.rs b/tests/cli.rs index b110114e..fea9a289 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -7,14 +7,15 @@ //! - Processing of Markdown files through the CLI interface use assert_cmd::Command; -use camino::{Utf8Path, Utf8PathBuf}; -use cap_std::{ambient_authority, fs_utf8::Dir}; +use camino::Utf8Path; use rstest::rstest; -use tempfile::{TempDir, tempdir}; #[macro_use] #[path = "common/mod.rs"] mod common; +#[path = "common/fs.rs"] +mod test_fs; +use test_fs::TestDir; #[path = "cli/ellipsis.rs"] mod ellipsis; #[path = "support/fixtures.rs"] @@ -52,8 +53,9 @@ fn test_cli_version_flag() { /// and asserts that the output is the expected fixed table. #[rstest] fn test_cli_process_file(broken_table: Vec) { - let dir = tempdir().expect("failed to create temporary directory"); - let (directory, parent_path) = capability_directory(&dir); + let dir = TestDir::new().expect("failed to create temporary directory"); + let directory = dir.directory(); + let parent_path = dir.path(); let file_name = Utf8Path::new("sample.md"); directory .write(file_name, format!("{}\n", broken_table.join("\n"))) @@ -71,8 +73,9 @@ fn test_cli_process_file(broken_table: Vec) { fn cli_output_modes_snapshot_table_prose() { let input = include_str!("data/cli-output-parity.dat"); let expected = include_str!("data/cli-output-parity.expected.md"); - let dir = tempdir().expect("failed to create temporary directory"); - let (directory, parent_path) = capability_directory(&dir); + let dir = TestDir::new().expect("failed to create temporary directory"); + let directory = dir.directory(); + let parent_path = dir.path(); let stdout_path = parent_path.join("stdout.md"); let in_place_path = parent_path.join("in-place.md"); directory @@ -305,8 +308,9 @@ fn test_cli_footnotes_option() { /// Executes an in-place rewrite with the provided flags and asserts idempotence. fn run_in_place(flags: &[&str], input: &str, expected: &str) { - let dir = tempdir().expect("failed to create temporary directory"); - let (directory, parent_path) = capability_directory(&dir); + let dir = TestDir::new().expect("failed to create temporary directory"); + let directory = dir.directory(); + let parent_path = dir.path(); let file_name = Utf8Path::new("sample.md"); let file_path = parent_path.join(file_name); directory @@ -353,15 +357,6 @@ fn run_in_place(flags: &[&str], input: &str, expected: &str) { assert_eq!(out2, out); } -/// Opens a temporary directory as the capability-scoped I/O boundary for a test. -fn capability_directory(tempdir: &TempDir) -> (Dir, Utf8PathBuf) { - let path = Utf8PathBuf::from_path_buf(tempdir.path().to_path_buf()) - .expect("temporary directory path is UTF-8"); - let directory = Dir::open_ambient_dir(&path, ambient_authority()) - .expect("failed to open temporary directory"); - (directory, path) -} - /// Ensures `--in-place` rewrites files correctly for multiple flag combinations. #[rstest] #[case(&["--fences"], "Rust\n```\nfn main() {}\n```\n", "```rust\nfn main() {}\n```\n")] diff --git a/tests/cli_frontmatter.rs b/tests/cli_frontmatter.rs index 4b9eb694..39648a8b 100644 --- a/tests/cli_frontmatter.rs +++ b/tests/cli_frontmatter.rs @@ -3,18 +3,30 @@ use assert_cmd::Command; use rstest::{fixture, rstest}; +#[path = "common/fs.rs"] +mod test_fs; +use test_fs::TestDir; + /// Fixture providing an in-place test runner closure. #[fixture] fn in_place_runner() -> impl Fn(&[&str], &str, &str) { |args: &[&str], input: &str, expected: &str| { - let temp = tempfile::NamedTempFile::new().expect("create temp file"); - std::fs::write(temp.path(), input).expect("write temp file"); + let dir = TestDir::new().expect("create temp directory"); + dir.directory() + .write("input.md", input) + .expect("write temp file"); + let file_path = dir.path().join("input.md"); let mut cmd = Command::cargo_bin("mdtablefix").expect("find binary"); - cmd.arg("--in-place").args(args).arg(temp.path()); + cmd.arg("--in-place") + .args(args) + .arg(file_path.as_std_path()); cmd.assert().success(); - let actual = std::fs::read_to_string(temp.path()).expect("read temp file"); + let actual = dir + .directory() + .read_to_string("input.md") + .expect("read temp file"); assert_eq!(actual, expected, "in-place content mismatch"); } } diff --git a/tests/cli_matrix.rs b/tests/cli_matrix.rs index 14dc6e68..5f62b5d6 100644 --- a/tests/cli_matrix.rs +++ b/tests/cli_matrix.rs @@ -12,6 +12,7 @@ use support::{ PhysicalCase, WrapVariant, assert_transform_invariants, + fixture_exists, fixture_path, has_flag, is_case_id, @@ -39,11 +40,11 @@ fn matrix_case_ids_accept_documented_characters() { fn matrix_case_fixtures_are_dat_files() { for case in BASE_MATRIX_CASES { let fixture = fixture_path(case.fixture); - assert!(fixture.exists(), "missing fixture {}", fixture.display()); - assert_eq!( - fixture.extension().and_then(|ext| ext.to_str()), - Some("dat") + assert!( + fixture_exists(case.fixture).expect("inspect matrix fixture"), + "missing fixture {fixture}" ); + assert_eq!(fixture.extension(), Some("dat")); } } diff --git a/tests/cli_matrix/fixture_io.rs b/tests/cli_matrix/fixture_io.rs new file mode 100644 index 00000000..e8c98398 --- /dev/null +++ b/tests/cli_matrix/fixture_io.rs @@ -0,0 +1,36 @@ +//! Capability-scoped fixture access for CLI matrix tests. + +use anyhow::{Context as _, Result}; +use camino::{Utf8Path, Utf8PathBuf}; +use cap_std::{ambient_authority, fs_utf8::Dir}; + +/// Returns the repository-relative path to a matrix fixture. +pub(crate) fn fixture_path(file_name: &str) -> Utf8PathBuf { + Utf8Path::new("tests") + .join("data") + .join("cli-matrix") + .join(file_name) +} + +pub(super) fn manifest_directory() -> Result { + Dir::open_ambient_dir( + Utf8Path::new(env!("CARGO_MANIFEST_DIR")), + ambient_authority(), + ) + .context("open repository root capability") +} + +/// Returns whether a repository fixture is accessible through its directory capability. +pub(crate) fn fixture_exists(file_name: &str) -> Result { + Ok(manifest_directory()? + .metadata(fixture_path(file_name)) + .is_ok()) +} + +/// Reads a matrix fixture through the repository directory capability. +pub(super) fn read_fixture(file_name: &str) -> Result { + let path = fixture_path(file_name); + manifest_directory()? + .read_to_string(&path) + .with_context(|| format!("read matrix fixture '{path}'")) +} diff --git a/tests/cli_matrix/invariants.rs b/tests/cli_matrix/invariants.rs index 3b74f42c..45aad08a 100644 --- a/tests/cli_matrix/invariants.rs +++ b/tests/cli_matrix/invariants.rs @@ -1,10 +1,8 @@ //! Independent output invariants for CLI matrix snapshots. -use std::fs; +use anyhow::Result; -use anyhow::{Context as _, Result}; - -use super::{LogicalCase, TransformFlag, fixture_path}; +use super::{LogicalCase, TransformFlag, read_fixture}; const ELLIPSIS_UTF8: &[u8] = b"\xE2\x80\xA6"; @@ -15,9 +13,7 @@ struct OrderedMarker { /// Asserts output properties that prove enabled transforms changed matching input. pub(crate) fn assert_transform_invariants(logical: &LogicalCase, stdout: &[u8]) -> Result<()> { - let fixture_path = fixture_path(logical.fixture); - let fixture = fs::read_to_string(&fixture_path) - .with_context(|| format!("read matrix fixture '{}'", fixture_path.display()))?; + let fixture = read_fixture(logical.fixture)?; let output = String::from_utf8_lossy(stdout); if logical.flags.contains(&TransformFlag::Ellipsis) && fixture.contains("...") { diff --git a/tests/cli_matrix/support.rs b/tests/cli_matrix/support.rs index 419a9d6e..522711d5 100644 --- a/tests/cli_matrix/support.rs +++ b/tests/cli_matrix/support.rs @@ -1,17 +1,20 @@ //! Support types and runners for the CLI matrix integration test. -use std::{ - fs, - path::{Path, PathBuf}, - process::Output, -}; +use std::process::Output; use anyhow::{Context as _, Result}; use assert_cmd::Command; -use tempfile::tempdir; +use camino::{Utf8Path, Utf8PathBuf}; +#[path = "fixture_io.rs"] +mod fixture_io; #[path = "invariants.rs"] mod invariants; +#[path = "../common/fs.rs"] +mod test_fs; +pub(crate) use fixture_io::{fixture_exists, fixture_path}; +use fixture_io::{manifest_directory, read_fixture}; +use test_fs::TestDir; /// Represents a non-wrap CLI transform flag. #[derive(Clone, Copy, Debug, Eq, Ord, PartialEq, PartialOrd)] @@ -293,30 +296,30 @@ pub(crate) fn assert_transform_invariants(logical: &LogicalCase, stdout: &[u8]) /// Copies a matrix fixture into the temporary command directory. /// /// The staged input preserves the fixture extension for debugging clarity. -pub(crate) fn stage_fixture(case: &PhysicalCase, dir: &Path) -> Result { +pub(crate) fn stage_fixture(case: &PhysicalCase, dir: &TestDir) -> Result { let fixture = fixture_path(case.logical.fixture); - let file_path = dir - .join("input") - .with_extension(fixture.extension().unwrap_or_default()); - fs::copy(&fixture, &file_path).with_context(|| { - format!( - "copy fixture '{}' to '{}'", - case.logical.fixture, - file_path.display(), - ) - })?; + let file_path = Utf8Path::new("input").with_extension(fixture.extension().unwrap_or_default()); + let contents = manifest_directory()? + .read(&fixture) + .with_context(|| format!("read fixture '{fixture}'"))?; + dir.directory() + .write(&file_path, contents) + .with_context(|| format!("copy fixture '{}' to '{file_path}'", case.logical.fixture))?; Ok(file_path) } /// Builds a run result from process output and the temporary input file. pub(crate) fn collect_result( output: Output, - file_path: &Path, + dir: &TestDir, + file_path: &Utf8Path, mode: ExecutionMode, ) -> Result { let file_content = match mode { - ExecutionMode::Stdout | ExecutionMode::InPlace => fs::read(file_path) - .with_context(|| format!("read file '{}' after {:?} run", file_path.display(), mode))?, + ExecutionMode::Stdout | ExecutionMode::InPlace => dir + .directory() + .read(file_path) + .with_context(|| format!("read file '{file_path}' after {mode:?} run"))?, }; Ok(RunResult { output, @@ -326,27 +329,19 @@ pub(crate) fn collect_result( /// Runs a physical matrix case through the real `mdtablefix` binary. pub(crate) fn run_physical_case(case: &PhysicalCase) -> Result { - let dir = tempdir().context("create temporary directory for matrix case")?; - let file_path = stage_fixture(case, dir.path())?; + let dir = TestDir::new().context("create temporary directory for matrix case")?; + let file_path = stage_fixture(case, &dir)?; + let command_path = dir.path().join(&file_path); let mut command = Command::cargo_bin("mdtablefix").context("create mdtablefix test command")?; - command.args(case.args()).arg(&file_path); + command.args(case.args()).arg(command_path.as_std_path()); let output = command.output().with_context(|| { format!( "execute mdtablefix for matrix case '{}'", case.snapshot_name() ) })?; - collect_result(output, &file_path, case.mode) -} - -/// Returns the repository-relative path to a matrix fixture. -pub(crate) fn fixture_path(file_name: &str) -> PathBuf { - Path::new(env!("CARGO_MANIFEST_DIR")) - .join("tests") - .join("data") - .join("cli-matrix") - .join(file_name) + collect_result(output, &dir, &file_path, case.mode) } /// Returns whether a matrix case identifier uses the documented character set. @@ -371,30 +366,5 @@ pub(crate) fn non_wrap_signature(fixture: &str, flags: &[TransformFlag]) -> Stri pub(crate) fn has_flag(case: &BaseCase, flag: TransformFlag) -> bool { case.flags.contains(&flag) } #[cfg(test)] -#[rustfmt::skip] -mod tests { - //! Unit tests for CLI-matrix support helpers. - - use super::{BaseCase, TransformFlag, has_flag, is_case_id, non_wrap_signature}; - use rstest::rstest; - - #[rstest] - #[case("row_001", true)] #[case("row-001", true)] #[case("abc123", true)] - #[case("", false)] #[case("Row_001", false)] #[case("row 001", false)] - fn is_case_id_returns_expected_value(#[case] id: &str, #[case] expected: bool) { - assert_eq!(is_case_id(id), expected); - } - - #[test] fn non_wrap_signature_ignores_wrap_variant() { - let flags = [TransformFlag::Renumber, TransformFlag::Fences]; let (unwrapped, wrapped) = (false, true); - assert_ne!(unwrapped, wrapped); assert_eq!(non_wrap_signature("fixture.dat", &flags), non_wrap_signature("fixture.dat", &flags)); } - #[test] fn non_wrap_signature_distinguishes_flag_lists() { - assert_ne!(non_wrap_signature("fixture.dat", &[TransformFlag::Renumber]), non_wrap_signature("fixture.dat", &[TransformFlag::Fences])); } - - #[rstest] - #[case(TransformFlag::Renumber, true)] #[case(TransformFlag::Fences, false)] - fn has_flag_returns_expected_value(#[case] flag: TransformFlag, #[case] expected: bool) { - let case = BaseCase { id: "row_001", fixture: "fixture.dat", flags: &[TransformFlag::Renumber] }; - assert_eq!(has_flag(&case, flag), expected); - } -} +#[path = "support_tests.rs"] +mod tests; diff --git a/tests/cli_matrix/support_tests.rs b/tests/cli_matrix/support_tests.rs new file mode 100644 index 00000000..755127a8 --- /dev/null +++ b/tests/cli_matrix/support_tests.rs @@ -0,0 +1,47 @@ +//! Unit tests for CLI-matrix support helpers. + +use rstest::rstest; + +use super::{BaseCase, TransformFlag, has_flag, is_case_id, non_wrap_signature}; + +#[rstest] +#[case("row_001", true)] +#[case("row-001", true)] +#[case("abc123", true)] +#[case("", false)] +#[case("Row_001", false)] +#[case("row 001", false)] +fn is_case_id_returns_expected_value(#[case] id: &str, #[case] expected: bool) { + assert_eq!(is_case_id(id), expected); +} + +#[test] +fn non_wrap_signature_ignores_wrap_variant() { + let flags = [TransformFlag::Renumber, TransformFlag::Fences]; + let (unwrapped, wrapped) = (false, true); + assert_ne!(unwrapped, wrapped); + assert_eq!( + non_wrap_signature("fixture.dat", &flags), + non_wrap_signature("fixture.dat", &flags) + ); +} + +#[test] +fn non_wrap_signature_distinguishes_flag_lists() { + assert_ne!( + non_wrap_signature("fixture.dat", &[TransformFlag::Renumber]), + non_wrap_signature("fixture.dat", &[TransformFlag::Fences]) + ); +} + +#[rstest] +#[case(TransformFlag::Renumber, true)] +#[case(TransformFlag::Fences, false)] +fn has_flag_returns_expected_value(#[case] flag: TransformFlag, #[case] expected: bool) { + let case = BaseCase { + id: "row_001", + fixture: "fixture.dat", + flags: &[TransformFlag::Renumber], + }; + assert_eq!(has_flag(&case, flag), expected); +} diff --git a/tests/code_emphasis.rs b/tests/code_emphasis.rs index 511a11bd..fe6d4b56 100644 --- a/tests/code_emphasis.rs +++ b/tests/code_emphasis.rs @@ -2,17 +2,36 @@ //! //! Verifies that emphasis markers adjacent to inline code are normalized. -use std::fs; - use rstest::rstest; #[path = "support/cli_args.rs"] mod cli_args; #[path = "support/cli_stdin.rs"] mod cli_stdin; +#[path = "common/fs.rs"] +mod test_fs; use cli_args::run_cli_with_args; use cli_stdin::run_cli_with_stdin; -use tempfile::tempdir; +use test_fs::TestDir; + +fn assert_in_place_result(file_name: &str, input: &str, expected: &str) { + let dir = TestDir::new().expect("failed to create temporary directory"); + dir.directory() + .write(file_name, input) + .expect("failed to write test file"); + let file_path = dir.path().join(file_name); + + run_cli_with_args(&["--code-emphasis", "--in-place", file_path.as_str()]) + .expect("failed to run mdtablefix") + .success() + .stdout(""); + + let output = dir + .directory() + .read_to_string(file_name) + .expect("failed to read output file"); + assert_eq!(output, expected); +} #[test] fn cli_stdin_code_emphasis() -> Result<(), Box> { @@ -44,75 +63,25 @@ fn cli_preserves_emphasised_code( #[test] fn cli_in_place_code_emphasis() { - let dir = tempdir().expect("failed to create temporary directory"); - let file_path = dir.path().join("sample.md"); let input = "`StepContext`** Enhancement (in **`crates/rstest-bdd/src/context.rs`**)**\n"; let expected = "**`StepContext` Enhancement (in `crates/rstest-bdd/src/context.rs`)**\n"; - fs::write(&file_path, input).expect("failed to write test file"); - run_cli_with_args(&[ - "--code-emphasis", - "--in-place", - file_path.to_str().expect("path is not valid UTF-8"), - ]) - .unwrap() - .success() - .stdout(""); - let out = fs::read_to_string(&file_path).expect("failed to read output file"); - assert_eq!(out, expected); + assert_in_place_result("sample.md", input, expected); } #[test] -fn cli_in_place_code_emphasis_empty_file() { - let dir = tempdir().expect("failed to create temporary directory"); - let file_path = dir.path().join("empty.md"); - fs::write(&file_path, "").expect("failed to write test file"); - run_cli_with_args(&[ - "--code-emphasis", - "--in-place", - file_path.to_str().expect("path is not valid UTF-8"), - ]) - .unwrap() - .success() - .stdout(""); - let out = fs::read_to_string(&file_path).expect("failed to read output file"); - assert_eq!(out, ""); -} +fn cli_in_place_code_emphasis_empty_file() { assert_in_place_result("empty.md", "", ""); } #[test] fn cli_in_place_code_emphasis_whitespace_file() { - let dir = tempdir().expect("failed to create temporary directory"); - let file_path = dir.path().join("whitespace.md"); let input = " \n\t "; let expected = " \n\t \n"; - fs::write(&file_path, input).expect("failed to write test file"); - run_cli_with_args(&[ - "--code-emphasis", - "--in-place", - file_path.to_str().expect("path is not valid UTF-8"), - ]) - .unwrap() - .success() - .stdout(""); - let out = fs::read_to_string(&file_path).expect("failed to read output file"); - assert_eq!(out, expected); + assert_in_place_result("whitespace.md", input, expected); } #[test] fn cli_in_place_preserves_inner_backticks() { - let dir = tempdir().expect("failed to create temporary directory"); - let file_path = dir.path().join("inner.md"); let input = "```` ``a`b`` ````\n"; - fs::write(&file_path, input).expect("failed to write test file"); - run_cli_with_args(&[ - "--code-emphasis", - "--in-place", - file_path.to_str().expect("path is not valid UTF-8"), - ]) - .unwrap() - .success() - .stdout(""); - let out = fs::read_to_string(&file_path).expect("failed to read output file"); - assert_eq!(out, input); + assert_in_place_result("inner.md", input, input); } #[test] diff --git a/tests/common/fs.rs b/tests/common/fs.rs new file mode 100644 index 00000000..adf72e04 --- /dev/null +++ b/tests/common/fs.rs @@ -0,0 +1,35 @@ +//! Capability-scoped filesystem support for integration tests. + +use anyhow::{Context as _, Result, anyhow}; +use camino::{Utf8Path, Utf8PathBuf}; +use cap_std::{ambient_authority, fs_utf8::Dir}; +use tempfile::TempDir; + +/// Owns a temporary directory and its capability-scoped filesystem handle. +pub(crate) struct TestDir { + directory: Dir, + path: Utf8PathBuf, + _guard: TempDir, +} + +impl TestDir { + /// Creates a temporary directory with a UTF-8 absolute path. + pub(crate) fn new() -> Result { + let guard = TempDir::new().context("create temporary test directory")?; + let path = Utf8PathBuf::from_path_buf(guard.path().to_path_buf()) + .map_err(|path| anyhow!("temporary directory path is not UTF-8: {}", path.display()))?; + let directory = Dir::open_ambient_dir(&path, ambient_authority()) + .context("open temporary directory capability")?; + Ok(Self { + directory, + path, + _guard: guard, + }) + } + + /// Returns the capability used for relative filesystem operations. + pub(crate) fn directory(&self) -> &Dir { &self.directory } + + /// Returns the UTF-8 absolute path used at process boundaries. + pub(crate) fn path(&self) -> &Utf8Path { &self.path } +} diff --git a/tests/parallel.rs b/tests/parallel.rs index 7f49029c..745542dd 100644 --- a/tests/parallel.rs +++ b/tests/parallel.rs @@ -1,14 +1,14 @@ //! Tests for parallel CLI processing of multiple files. -use std::{fs::File, io::Write}; - use assert_cmd::Command; use rstest::rstest; -use tempfile::tempdir; #[macro_use] #[path = "common/mod.rs"] mod common; +#[path = "common/fs.rs"] +mod test_fs; +use test_fs::TestDir; #[path = "support/cli_args.rs"] mod cli_args; @@ -25,48 +25,40 @@ fn test_cli_parallel_empty_file_list() -> Result<(), Box> #[rstest] fn test_cli_parallel_multiple_files() -> Result<(), Box> { - let dir = tempdir().expect("failed to create temporary directory"); + let dir = TestDir::new().expect("failed to create temporary directory"); let mut files = Vec::new(); let mut expected = String::new(); for i in 0..4 { - let path = dir.path().join(format!("file{i}.md")); + let file_name = format!("file{i}.md"); + let path = dir.path().join(&file_name); let table = vec![ format!("| A{i} | B{i} | |"), format!("| {i} | {i} | | {i} | {i} |"), ]; - let mut f = File::create(&path).expect("failed to create temporary file"); - for line in &table { - writeln!(f, "{line}").expect("failed to write line"); - } - f.flush().expect("failed to flush file"); - drop(f); + dir.directory() + .write(&file_name, format!("{}\n", table.join("\n"))) + .expect("failed to write temporary file"); expected.push_str(&mdtablefix::reflow_table(&table).join("\n")); expected.push('\n'); files.push(path); } - let args: Vec<&str> = files - .iter() - .map(|p| p.to_str().expect("path is not valid UTF-8")) - .collect(); + let args: Vec<&str> = files.iter().map(|path| path.as_str()).collect(); run_cli_with_args(&args)?.success().stdout(expected); Ok(()) } #[rstest] fn test_cli_parallel_missing_file_error() { - let dir = tempdir().expect("failed to create temporary directory"); + let dir = TestDir::new().expect("failed to create temporary directory"); let good = dir.path().join("good.md"); let table = vec![ "| Q | R | |".to_string(), "| 1 | 2 | | 3 | 4 |".to_string(), ]; - let mut f = File::create(&good).expect("failed to create file"); - for line in &table { - writeln!(f, "{line}").expect("failed to write line"); - } - f.flush().expect("failed to flush file"); - drop(f); + dir.directory() + .write("good.md", format!("{}\n", table.join("\n"))) + .expect("failed to write file"); let expected = mdtablefix::reflow_table(&table).join("\n") + "\n"; let missing = dir.path().join("missing.md"); @@ -84,18 +76,15 @@ fn test_cli_parallel_missing_file_error() { fn test_cli_parallel_missing_file_in_place( broken_table: Vec, ) -> Result<(), Box> { - let dir = tempdir().expect("failed to create temporary directory"); + let dir = TestDir::new().expect("failed to create temporary directory"); + dir.directory() + .write("good.md", format!("{}\n", broken_table.join("\n"))) + .expect("failed to write file"); let good = dir.path().join("good.md"); - let mut f = File::create(&good).expect("failed to create file"); - for line in &broken_table { - writeln!(f, "{line}").expect("failed to write line"); - } - f.flush().expect("failed to flush file"); - drop(f); let missing = dir.path().join("missing.md"); - let good_str = good.to_str().expect("path is not valid UTF-8"); - let missing_str = missing.to_str().expect("path is not valid UTF-8"); + let good_str = good.as_str(); + let missing_str = missing.as_str(); run_cli_with_args(&["--in-place", good_str, missing_str])? .failure() .stderr(predicates::str::contains("missing.md")); diff --git a/tests/static_regex_lint.rs b/tests/static_regex_lint.rs index f91d86a0..aac68de0 100644 --- a/tests/static_regex_lint.rs +++ b/tests/static_regex_lint.rs @@ -15,13 +15,18 @@ //! tests copy each fixture into a temporary directory as a `.rs` file before //! scanning. -use std::{ - path::{Path, PathBuf}, - process::Command, -}; +use std::process::Command; +use camino::{Utf8Path, Utf8PathBuf}; +use cap_std::{ + ambient_authority, + fs_utf8::{Dir, PermissionsExt as _}, +}; use rstest::rstest; -use tempfile::TempDir; + +#[path = "common/fs.rs"] +mod test_fs; +use test_fs::TestDir; /// The diagnostic emitted when a prohibited declaration is found. const PROHIBITED_DIAGNOSTIC: &str = "static regular expressions must use lazy_regex!"; @@ -45,20 +50,27 @@ const PROHIBITED_FORMS: &[&str] = &[ "once_cell_lazy_move", ]; -fn manifest_dir() -> PathBuf { PathBuf::from(env!("CARGO_MANIFEST_DIR")) } +fn manifest_path() -> &'static Utf8Path { Utf8Path::new(env!("CARGO_MANIFEST_DIR")) } + +fn manifest_directory() -> Dir { + Dir::open_ambient_dir(manifest_path(), ambient_authority()) + .expect("failed to open repository root") +} -fn script_path() -> PathBuf { manifest_dir().join("scripts/check-static-regexes.sh") } +fn script_path() -> Utf8PathBuf { manifest_path().join("scripts/check-static-regexes.sh") } fn fixture(label: &str) -> String { - let path = manifest_dir().join(format!("tests/data/static_regex/{label}.rs.txt")); - std::fs::read_to_string(&path) - .unwrap_or_else(|e| panic!("failed to read fixture {}: {e}", path.display())) + let path = Utf8PathBuf::from(format!("tests/data/static_regex/{label}.rs.txt")); + manifest_directory() + .read_to_string(&path) + .unwrap_or_else(|error| panic!("failed to read fixture {path}: {error}")) } /// Materialize `label`'s fixture as a `.rs` file inside a fresh temp directory. -fn scan_dir_with(label: &str) -> TempDir { - let dir = TempDir::new().expect("failed to create temp dir"); - std::fs::write(dir.path().join(format!("{label}.rs")), fixture(label)) +fn scan_dir_with(label: &str) -> TestDir { + let dir = TestDir::new().expect("failed to create temp dir"); + dir.directory() + .write(format!("{label}.rs"), fixture(label)) .expect("failed to write fixture into temp dir"); dir } @@ -70,8 +82,8 @@ fn scan_dir_with(label: &str) -> TempDir { /// `rg --pcre2`); the guard splits it on whitespace. Passing `None` clears any /// ambient `RG` so default-path runs exercise the guard's own `rg` default /// deterministically. -fn run_guard(scan_dir: &Path, rg: Option<&str>) -> std::process::Output { - let mut cmd = Command::new(script_path()); +fn run_guard(scan_dir: &Utf8Path, rg: Option<&str>) -> std::process::Output { + let mut cmd = Command::new(script_path().as_std_path()); cmd.arg(scan_dir); match rg { Some(rg) => cmd.env("RG", rg), @@ -82,16 +94,18 @@ fn run_guard(scan_dir: &Path, rg: Option<&str>) -> std::process::Output { } /// Write `script` to `/` and mark it executable. -fn write_stub(dir: &Path, name: &str, script: &str) -> PathBuf { - let path = dir.join(name); - std::fs::write(&path, script).expect("failed to write stub"); +fn write_stub(dir: &TestDir, name: &str, script: &str) -> Utf8PathBuf { + dir.directory() + .write(name, script) + .expect("failed to write stub"); #[cfg(unix)] { - use std::os::unix::fs::PermissionsExt; - std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o755)) + let permissions = cap_std::fs_utf8::Permissions::from_mode(0o755); + dir.directory() + .set_permissions(name, permissions) .expect("failed to chmod stub"); } - path + dir.path().join(name) } #[rstest] @@ -131,11 +145,11 @@ fn accepts_clean_sources() { #[test] fn propagates_ripgrep_scan_failure() { - let dir = TempDir::new().expect("failed to create temp dir"); + let dir = TestDir::new().expect("failed to create temp dir"); // A stub standing in for ripgrep that fails with a distinctive status. - let stub = write_stub(dir.path(), "rg-stub.sh", "#!/bin/sh\nexit 3\n"); + let stub = write_stub(&dir, "rg-stub.sh", "#!/bin/sh\nexit 3\n"); - let output = run_guard(dir.path(), Some(&stub.display().to_string())); + let output = run_guard(dir.path(), Some(stub.as_str())); assert_eq!( output.status.code(), @@ -156,20 +170,19 @@ fn propagates_ripgrep_scan_failure() { /// own, rather than treating the whole value as one executable name. #[test] fn preserves_arguments_supplied_through_rg() { - let dir = TempDir::new().expect("failed to create temp dir"); + let dir = TestDir::new().expect("failed to create temp dir"); let argv_log = dir.path().join("argv.txt"); // A stub that records its argv, then reports "no matches" so the guard // takes its clean-scan path. let stub = write_stub( - dir.path(), + &dir, "rg-stub.sh", &format!( - "#!/bin/sh\nfor a in \"$@\"; do printf '%s\\n' \"$a\"; done > '{}'\nexit 1\n", - argv_log.display() + "#!/bin/sh\nfor a in \"$@\"; do printf '%s\\n' \"$a\"; done > '{argv_log}'\nexit 1\n" ), ); - let output = run_guard(dir.path(), Some(&format!("{} --pcre2", stub.display()))); + let output = run_guard(dir.path(), Some(&format!("{stub} --pcre2"))); assert_eq!( output.status.code(), @@ -178,7 +191,9 @@ fn preserves_arguments_supplied_through_rg() { String::from_utf8_lossy(&output.stderr) ); - let argv: Vec = std::fs::read_to_string(&argv_log) + let argv: Vec = dir + .directory() + .read_to_string("argv.txt") .expect("stub should have recorded its argv") .lines() .map(str::to_owned) @@ -194,7 +209,7 @@ fn preserves_arguments_supplied_through_rg() { ); assert_eq!( argv.last().map(String::as_str), - Some(dir.path().to_str().expect("temp dir path should be UTF-8")), + Some(dir.path().as_str()), "the scan directory must remain the final argument, got: {argv:?}" ); } diff --git a/tests/wrap/cli_files.rs b/tests/wrap/cli_files.rs index e3735f81..8aa0a1dd 100644 --- a/tests/wrap/cli_files.rs +++ b/tests/wrap/cli_files.rs @@ -1,12 +1,13 @@ //! File-backed CLI regression tests for the parameterless wrapping flag. -use std::fs; - use assert_cmd::Command; use mdtablefix::process::WRAP_COLS; -use tempfile::NamedTempFile; use unicode_width::UnicodeWidthStr; +#[path = "../common/fs.rs"] +mod test_fs; +use test_fs::TestDir; + /// Ensures a path after `--wrap` remains a positional input file. #[test] fn cli_wrap_processes_positional_file() -> Result<(), Box> { @@ -17,13 +18,14 @@ fn cli_wrap_processes_positional_file() -> Result<(), Box "漢字🙂 漢字🙂 漢字🙂 漢字🙂 漢字🙂 漢字🙂 漢字🙂 漢字🙂 漢字🙂 漢字🙂 ", "漢字🙂 漢字🙂 漢字🙂 漢字🙂 漢字🙂 漢字🙂 漢字🙂 漢字🙂 漢字🙂 漢字🙂.\n", ); - let file = NamedTempFile::new()?; - fs::write(file.path(), input)?; + let dir = TestDir::new()?; + dir.directory().write("input.md", input)?; + let file_path = dir.path().join("input.md"); let mut command = Command::cargo_bin("mdtablefix")?; let output = command .arg("--wrap") - .arg(file.path()) + .arg(file_path.as_std_path()) .assert() .success() .get_output() diff --git a/tests/wrap_cli.rs b/tests/wrap_cli.rs index f086d370..d1b8c266 100644 --- a/tests/wrap_cli.rs +++ b/tests/wrap_cli.rs @@ -1,25 +1,27 @@ //! CLI regression tests for wrap behaviour around verbatim code blocks. -use std::fs; - use assert_cmd::Command; use rstest::rstest; -use tempfile::NamedTempFile; + +#[path = "common/fs.rs"] +mod test_fs; +use test_fs::TestDir; fn run_wrap_in_place_and_read_back(input: &str) -> Result> { - let temp = NamedTempFile::new()?; - fs::write(temp.path(), input)?; + let dir = TestDir::new()?; + dir.directory().write("input.md", input)?; + let file_path = dir.path().join("input.md"); let mut command = Command::cargo_bin("mdtablefix")?; command .args(["--wrap", "--in-place"]) - .arg(temp.path()) + .arg(file_path.as_std_path()) .assert() .success() .stdout("") .stderr(""); - Ok(fs::read_to_string(temp.path())?) + Ok(dir.directory().read_to_string("input.md")?) } /// Guards issue #261 by asserting `--wrap --in-place` leaves shell code blocks From 82dec427d084ab204819a137150623004dfa28ab Mon Sep 17 00:00:00 2001 From: leynos Date: Fri, 31 Jul 2026 12:31:30 +0200 Subject: [PATCH 5/6] Document capability-scoped test paths (#418) Record the integration suite's UTF-8 path and directory-capability convention, including the ownership boundary for shared test support. Explain why the production dependencies are reused by tests and import the current Whitaker user's guide for path-level lint exclusion guidance. --- Cargo.toml | 1 + docs/contents.md | 2 + docs/developers-guide.md | 61 ++- docs/repository-layout.md | 8 +- docs/whitaker-users-guide.md | 750 +++++++++++++++++++++++++++++++++++ 5 files changed, 819 insertions(+), 3 deletions(-) create mode 100644 docs/whitaker-users-guide.md diff --git a/Cargo.toml b/Cargo.toml index cb3a3a3a..59fe5673 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -18,6 +18,7 @@ pkg-fmt = "tgz" [dependencies] anyhow = "1" +# Production file output and integration tests share UTF-8 capability paths. camino = "1.2.4" cap-std = { version = "4.0.2", features = ["fs_utf8"] } clap = { version = "4", features = ["derive"] } diff --git a/docs/contents.md b/docs/contents.md index 6ed4430b..902578df 100644 --- a/docs/contents.md +++ b/docs/contents.md @@ -40,6 +40,8 @@ Rust documentation tests that avoid brittle or misleading examples. - [Rust testing with rstest fixtures](rust-testing-with-rstest-fixtures.md): Reference for the fixture and parameterization patterns used in tests. +- [Whitaker user's guide](whitaker-users-guide.md): Imported reference for + Whitaker lints, including path-level `no_std_fs_operations` exclusions. - [Trailing spaces](trailing-spaces.md): Notes on preserving Markdown hard line breaks and other trailing-space-sensitive content. diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 06e192ec..f8679eb3 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -990,7 +990,66 @@ integration-test binary crates. The `#[expect(unused_macros)]` suppressions that previously guarded them were replaced by the export attribute when it became clear that multiple test binaries depend on them. -### 2.3. `test-macros` crate + +### 2.3. Capability-scoped test filesystem access + +All integration tests under `tests/` use `camino::Utf8Path` and +`camino::Utf8PathBuf` for runtime paths and perform filesystem operations +through `cap_std::fs_utf8::Dir`. Absolute paths are retained only for process +boundaries such as `std::process::Command`; reads, writes, metadata queries, +copies, and permission changes use paths relative to an opened directory +capability. + +The `camino` and `cap-std` crates are normal dependencies because the +production CLI already uses them for file output. The integration suite shares +those dependencies instead of declaring redundant dev-dependencies. This keeps +production and test filesystem semantics aligned while avoiding two independent +version declarations. + +`tests/common/fs.rs` owns `TestDir`, the reusable temporary-directory boundary. +It converts `tempfile::TempDir`'s platform path to a UTF-8 path once, opens the +directory capability, and retains the `TempDir` guard for the test's lifetime. +Only integration tests may include this helper. Callers compose it by using +relative paths with `TestDir::directory()` and by deriving an absolute +`Utf8PathBuf` from `TestDir::path()` only when invoking a child process. It +must not absorb fixture-specific logic or production filesystem behaviour. + +Repository fixture access remains an explicit ambient boundary. Focused modules +such as `tests/cli_matrix/fixture_io.rs` open `CARGO_MANIFEST_DIR` once and +expose only capability-scoped fixture queries. New tests must follow the same +pattern instead of calling `std::fs`, storing `std::path::PathBuf`, or relying +on `Utf8Path::exists`, which performs an ambient metadata query. + +Whitaker's `no_std_fs_operations` lint supports `excluded_paths` for genuinely +unavoidable ambient boundaries. Prefer the capability pattern above; use a +path-level exclusion only when the boundary cannot be expressed through +`cap_std`, and record the reason beside the configuration. See the imported +[Whitaker user's guide](whitaker-users-guide.md#no_std_fs_operations) for the +configuration syntax and segment-matching rules. + + +### 2.4. `test-macros` crate + +The `test-macros` workspace crate provides the `allow_fixture_expansion_lints` +proc-macro attribute. It suppresses the `unused_braces` lint that `rstest` +fixture expansion triggers when `fn_single_line = true` is set in +`rustfmt.toml`. + +The macro emits `#[allow(unused_braces, …)]` rather than `#[expect(…)]` because +the Rust proc-macro API delivers a pre-parsed token stream; the emitted lint +attribute applies to code that the compiler has not yet expanded, making +`#[expect]` semantically unusable at that site. This is a known consequence of +the `rstest` fixture expansion and is not a lint-integrity violation. + +Apply it to any fixture function whose single-expression body triggers the lint: + +```rust +#[test_macros::allow_fixture_expansion_lints] +#[rstest::fixture] +pub fn broken_table() -> Vec { … } +``` + +### 2.4. `test-macros` crate The `test-macros` workspace crate provides the `allow_fixture_expansion_lints` proc-macro attribute. It suppresses the `unused_braces` lint that `rstest` diff --git a/docs/repository-layout.md b/docs/repository-layout.md index 260379af..b0ef2d50 100644 --- a/docs/repository-layout.md +++ b/docs/repository-layout.md @@ -26,6 +26,7 @@ output and most test fixtures. │ ├── developers-guide.md │ ├── documentation-style-guide.md │ ├── repository-layout.md +│ ├── whitaker-users-guide.md │ └── users-guide.md ├── src/ │ ├── fences/ @@ -69,6 +70,8 @@ _Figure 1: Simplified repository tree._ the Markdown processing pipeline. - `docs/documentation-style-guide.md`: Documentation style rules imported from the shared Rust agent template. +- `docs/whitaker-users-guide.md`: Imported Whitaker lint reference, including + crate-level and path-level filesystem exclusion configuration. - `docs/adrs/`: Accepted architecture decision records. Add narrow decision records here when design choices need a durable audit trail. - `docs/execplans/`: Living execution plans and roadmap documents for larger @@ -98,8 +101,9 @@ _Figure 1: Simplified repository tree._ - `tests/*.rs`: Integration and behavioural tests for command-line workflows and Markdown transformations. -- `tests/common/`: Shared integration test support. Keep helpers explicit and - avoid direct environment mutation. +- `tests/common/`: Shared integration test support. `fs.rs` owns temporary + directory capability setup; keep other helpers explicit and avoid direct + environment mutation. - `tests/cli_matrix/`: Matrix-test support for command-line option combinations and invariants. - `tests/data/`: Input and expected-output fixtures. Treat these as reviewable diff --git a/docs/whitaker-users-guide.md b/docs/whitaker-users-guide.md new file mode 100644 index 00000000..a84c8333 --- /dev/null +++ b/docs/whitaker-users-guide.md @@ -0,0 +1,750 @@ +# Whitaker User's Guide + +Whitaker is a collection of opinionated Dylint lints for Rust. This guide +explains how to integrate the lints into a project and configure them. + +For contributors who want to develop new lints or work on Whitaker itself, see +the [Developer's Guide](developers-guide.md). + +## Quick Setup + +### Prerequisites + +Install `cargo-dylint` and `dylint-link`: + +```sh +cargo install cargo-dylint dylint-link +``` + +### Standalone installation (recommended) + +The simplest way to use Whitaker is via the standalone installer, which handles +setup automatically: + +```sh +cargo install whitaker-installer +whitaker-installer +whitaker --all +``` + +This: + +1. Installs `cargo-dylint` and `dylint-link` if not present + The installer first attempts to download pre-built dependency binaries from + Whitaker's GitHub Releases page for the current platform. If the release + asset is absent (HTTP 404 or 410), the installer skips `cargo binstall` and + falls back directly to building from source with `cargo install`, pinned to + the version recorded in the dependency manifest. If the release asset is + present but the download fails for another reason, the installer falls back + to `cargo binstall` when available and then to `cargo install`. When the + source-build path is taken, the installer reports that it is falling back to + Cargo, and on success it prints + `Installed from source with cargo install.`. After installation, + `cargo-dylint` is verified by running `cargo dylint --version`. + `dylint-link` is never executed: it is a linker wrapper that forwards its + arguments to the underlying linker, so it has no reliable self-reporting + subcommand. A release artefact is trusted once its checksum, extraction, and + executable permissions have been established, and a Cargo-managed copy is + checked by resolving it on `PATH` and comparing the version Cargo recorded. +2. Clones the Whitaker repository to a platform-specific data directory +3. Builds the lint libraries +4. Creates `whitaker` and `whitaker-ls` wrapper scripts. `whitaker` invokes + `cargo dylint` with the correct `DYLINT_LIBRARY_PATH`, and `whitaker-ls` + lists installed Whitaker suite libraries +5. Ensures the pinned Rust toolchain and components are installed via rustup + +After installation, run `whitaker --all` in any Rust project to lint it. Use +`whitaker-ls` to list the installed Whitaker suite libraries. + +On Windows, the installer's `PATH` check honours `PATHEXT` and falls back to +the usual executable suffixes when `PATHEXT` is unset, so a normal +Cargo-installed executable such as `dylint-link.exe` in +`%USERPROFILE%\.cargo\bin` is located correctly and then matched against the +version Cargo recorded for it, without needing a separate wrapper or manual +environment-variable workaround. + +**Options:** + +- `--cranelift` — Tell the installer to add the + `rustc-codegen-cranelift` component via `rustup component add`. The + `rustc-codegen-cranelift` component is not included in the standard nightly + toolchain, so enable `--cranelift` when a project or CI pipeline requires the + Cranelift back-end and would otherwise need an explicit + `rustc-codegen-cranelift` component-add step before running the installer. +- `--skip-deps` — Skip `cargo-dylint`/`dylint-link` installation check +- `--skip-wrapper` — Skip wrapper script generation (prints + `DYLINT_LIBRARY_PATH` instructions instead) +- `--no-update` — Don't update existing repository clone + +### Adding Whitaker to a project + +Add the following to the workspace `Cargo.toml`: + +```toml +[workspace.metadata.dylint] +libraries = [ + { git = "https://github.com/leynos/whitaker", pattern = "whitaker_suite" } +] +``` + +Then run the lints: + +```sh +cargo dylint --all +``` + +### Version pinning + +For reproducible builds, pin to a specific release tag or commit: + +```toml +[workspace.metadata.dylint] +libraries = [ + { git = "https://github.com/leynos/whitaker", pattern = "whitaker_suite", tag = "v0.1.0" } +] +``` + +Or pin to a specific commit: + +```toml +[workspace.metadata.dylint] +libraries = [ + { git = "https://github.com/leynos/whitaker", pattern = "whitaker_suite", rev = "abc123def456" } +] +``` + +### Rolling release downloads + +Whitaker publishes a `rolling` pre-release tag that is continuously updated and +overwritten on every push to `main`. It is intended for early adopters who want +the latest available build outputs before the next stable release is cut. + +Rolling releases are best-effort builds. If some matrix legs fail, Whitaker +still publishes the artefacts that were built successfully. For example, a +target-specific `cargo-dylint` archive may be missing from one rolling release +even though other target archives were updated successfully. Do not assume that +every supported target is present in every rolling release. + +Stable releases differ from `rolling`: a stable tag is expected to contain the +complete artefact set for the release. For production installs, pin to a stable +release tag rather than consuming `rolling`. + +Scripts or CI pipelines that consume rolling-release archives should verify +that the required target archive exists before proceeding. Treat missing +archives as an expected condition for rolling releases rather than assuming the +artefact set is complete. + +### Selecting individual lints + +To load specific lints instead of the full suite, specify each lint explicitly: + +```toml +[workspace.metadata.dylint] +libraries = [ + { git = "https://github.com/leynos/whitaker", pattern = "crates/module_max_lines" }, + { git = "https://github.com/leynos/whitaker", pattern = "crates/no_expect_outside_tests" } +] +``` + +### Standard vs Experimental Lints + +Whitaker lints are divided into two categories: + +- **Standard lints** are stable, well-tested, and included in the default suite. + They are recommended for general use and have predictable behaviour. +- **Experimental lints** are newer or more aggressive checks that may produce + false positives or undergo breaking changes between releases. They must be + explicitly enabled. + +The default `whitaker_suite` pattern includes only standard lints. Whitaker +currently ships one experimental lint, `rstest_helper_should_be_fixture`, which +is available only when experimental lints are enabled. + +### Enabling experimental lints + +#### Via standalone installer + +```sh +whitaker-installer --experimental +``` + +This enables experimental suite features when building `whitaker_suite`. To +build experimental lints as individual libraries, combine it with +`--individual-lints`. Explicit `--lint` requests for experimental lints also +require `--experimental`; without that opt-in the installer rejects the request +before building anything. + +## Lint Configuration + +Configure lint behaviour in `dylint.toml` at the workspace root: + +```toml +# Diagnostic language (default: en-GB) +locale = "cy" + +# Module size threshold (default: 400) +[module_max_lines] +max_lines = 500 + +# Conditional branch limit (default: 2) +[conditional_max_n_branches] +max_branches = 3 + +# Custom test attributes +[no_expect_outside_tests] +additional_test_attributes = ["my_framework::test", "wasm_bindgen_test"] + +# Additional test markers for `test_must_not_have_example` +[test_must_not_have_example] +additional_test_attributes = ["actix_rt::test", "my_framework::test"] + +# Allow panics in main +[no_unwrap_or_else_panic] +allow_in_main = true + +# Experimental rstest fixture extraction lint +[rstest_helper_should_be_fixture] +min_calls = 2 +min_distinct_tests = 2 +require_identical_fixture_arg_names = false +provider_param_attributes = ["case", "values", "files", "future", "context"] +use_source_callee_fallback = false +``` + +## Localized Diagnostics + +Whitaker supports multiple languages for diagnostic messages. Set the locale +via the `DYLINT_LOCALE` environment variable or in `dylint.toml`: + +```toml +locale = "cy" +``` + +Available locales: + +- `en-GB` (default) - English +- `cy` - Welsh (Cymraeg) +- `gd` - Scottish Gaelic (Gàidhlig) + +______________________________________________________________________ + +## Available Lints + +### `bumpy_road_function` + +#### Purpose + +Detects functions with multiple distinct clusters of nested conditional +complexity. + +#### Scope and behaviour + +Flags a function when peak detection finds two or more separated complexity +regions above the configured threshold. Detection smooths the local complexity +signal with the configured `window` and only considers peaks spanning at least +`min_bump_lines`. + +The default threshold was lowered from 3.0 to 2.5 to detect bumpy road patterns +in match expressions with nested conditionals. The moving-average smoothing +(window=3) reduces raw peaks by approximately 15–20%, so a threshold of 3.0 can +mask genuine two-bump patterns in match arms with nested `if` guards. + +#### Configuration + +```toml +[bumpy_road_function] +threshold = 2.5 # Raise to 3.0 or higher to reduce false positives +window = 3 +min_bump_lines = 2 +``` + +#### What is allowed + +- A single complexity peak in a function. +- Simple predicates that remain below the configured threshold. + +#### What is denied + +- Two or more separated complexity peaks above the configured threshold. + +#### How to fix + +Split complex regions into helper functions and simplify branch-heavy +predicates. + +______________________________________________________________________ + +### `conditional_max_n_branches` + +Limits the complexity of conditional predicates by enforcing a maximum number +of boolean branches. + +**Configuration:** + +```toml +[conditional_max_n_branches] +max_branches = 2 +``` + +The default threshold is 2 branches. A predicate like `a && b && c` has three +branches and would trigger the lint. + +**How to fix:** Extract complex conditions into helper functions: + +```rust +// Before: Too many branches +if condition_a && condition_b && condition_c { + // action +} + +// After: Extract to helper function +fn should_proceed() -> bool { + condition_a && condition_b && condition_c +} + +if should_proceed() { + // action +} +``` + +______________________________________________________________________ + +### `function_attrs_follow_docs` + + +#### Purpose + +Ensures doc comments appear before other outer attributes on functions, +methods, and trait methods. + + +#### Scope and behaviour + +When attributes are generated or reordered by a procedural macro (for example, +`rstest` or `derive`), the lint recovers the original source span from the +macro expansion chain. Attributes whose spans cannot be traced back to any +user-written source location (macro-only glue) are silently excluded from the +ordering check, so the lint never fires on compiler- or macro-generated code +that the developer cannot edit. + + +#### Configuration + +`function_attrs_follow_docs` has no configuration knobs. + + +#### What is allowed + +- Doc comments that appear before every other outer attribute on the same + function, method, or trait method. +- Macro-generated attributes whose spans are excluded because they are + macro-only. +- Inner attributes, which are outside the lint's scope. + + +#### What is denied + +- Outer attributes that appear before a doc comment on the same function, + method, or trait method. +- Macro-expanded attributes that recover to a user-editable source span and + sort before the doc comment. + + +#### How to fix + +Move doc comments so they appear before other outer attributes: + +```rust +// Wrong +#[inline] +/// This function does something. +fn example() {} + +// Correct +/// This function does something. +#[inline] +fn example() {} +``` + +With `rstest`, place the doc comment before all attributes, including the test +annotation: + +```rust +// Wrong +#[rstest] +#[case(1, 2, 3)] +/// Verifies addition. +fn adds(#[case] a: i32, #[case] b: i32, #[case] expected: i32) { + assert_eq!(a + b, expected); +} + +// Correct +/// Verifies addition. +#[rstest] +#[case(1, 2, 3)] +fn adds(#[case] a: i32, #[case] b: i32, #[case] expected: i32) { + assert_eq!(a + b, expected); +} +``` + +______________________________________________________________________ + +### `module_max_lines` + +Warns when modules exceed a configurable line count threshold. + +**Configuration:** + +```toml +[module_max_lines] +max_lines = 400 +``` + +**How to fix:** Split large modules into smaller, focused submodules. + +______________________________________________________________________ + +### `module_must_have_inner_docs` + +Enforces that every module begins with an inner documentation comment (`//!`). + +**How to fix:** + +```rust +mod my_module { + //! Explain the module's purpose here. + pub fn value() {} +} +``` + +______________________________________________________________________ + +### `no_expect_outside_tests` + + +#### Purpose + +Detect test attributes correctly so `no_expect_outside_tests` can allow +`.expect()` in recognized test-only code while still flagging production use. + + +#### Scope and behaviour + +Whitaker recognizes `#[test]`, prelude-qualified `#[test]` forms, +`#[tokio::test]`, `#[async_std::test]`, `#[gpui::test]`, `#[rstest]`, +`#[rstest::rstest]`, `#[rstest_parametrize]`, `#[rstest::rstest_parametrize]`, +`#[case]`, and `#[rstest::case]` by default. The `additional_test_attributes` +setting extends that matching list with project-specific markers, so the lint +treats those annotated functions as tests too. + + +#### Configuration + +```toml +[no_expect_outside_tests] +additional_test_attributes = ["my_framework::test", "wasm_bindgen_test"] +``` + +Set `additional_test_attributes` to an array of attribute paths written as +strings. Each entry should match the path Whitaker sees on the test function, +for example `my_framework::test` or `wasm_bindgen_test`. + + +#### Ancestor context propagation + +`additional_test_attributes` now apply during ancestor context detection as +well as direct annotation matching. If a parent function is annotated with a +configured custom test attribute, Whitaker treats nested code within that +function as test context too, so `.expect()` remains allowed throughout that +ancestry chain. + +```rust +// dylint.toml +// [no_expect_outside_tests] +// additional_test_attributes = ["my_framework::test"] + +#[my_framework::test] +async fn my_test() { + helper(); // allowed — ancestor is a recognized test function +} + +fn helper() { + let v: Option = Some(1); + let _ = v.expect("value present"); // allowed — called from within test ancestry +} +``` + + +#### What is allowed + +- Default markers such as `#[test]`, `#[::test]`, + `#[::std::prelude::v1::test]`, `#[tokio::test]`, `#[async_std::test]`, + `#[gpui::test]`, `#[rstest]`, `#[rstest::rstest]`, `#[rstest_parametrize]`, + `#[rstest::rstest_parametrize]`, `#[case]`, and `#[rstest::case]` +- Project-specific markers listed in `additional_test_attributes`, such as + `#[wasm_bindgen_test]` + + +#### What is denied + +Functions using `.expect()` will still be flagged when their test attribute is +not in Whitaker's default list and is not listed in +`additional_test_attributes`. + + +#### How to fix + +- Add the missing test marker to `additional_test_attributes` if the function is + genuinely part of a supported test framework +- Change the attribute usage to a recognized form such as `#[test]`, + `#[::test]`, `#[::std::prelude::v1::test]`, `#[tokio::test]`, + `#[async_std::test]`, `#[gpui::test]`, `#[rstest]`, `#[rstest::rstest]`, + `#[rstest_parametrize]`, `#[rstest::rstest_parametrize]`, `#[case]`, or + `#[rstest::case]` where appropriate +- If the function is not test-only code, replace `.expect()` with explicit error + handling such as `?` or `map_err` + +______________________________________________________________________ + +### `rstest_helper_should_be_fixture` + + +#### Purpose + +Bootstraps the experimental lint that will recommend converting repeated helper +calls inside `#[rstest]` tests into injected `#[fixture]` parameters. + + +#### Scope and behaviour + +This lint is experimental. The current implementation registers the lint, loads +configuration defaults, and passively collects local helper calls inside strict +`#[rstest]` tests, fingerprinting fixture-local, literal, `const`, and `static` +arguments for later aggregation. The lint remains diagnostic-silent: threshold +evaluation and actionable diagnostics are tracked by 8.2.3, while UI pass/fail +coverage is tracked by 8.2.4. + + +#### Configuration + +```toml +[rstest_helper_should_be_fixture] +min_calls = 2 +min_distinct_tests = 2 +require_identical_fixture_arg_names = false +provider_param_attributes = ["case", "values", "files", "future", "context"] +use_source_callee_fallback = false +``` + +`provider_param_attributes` lists `rstest` parameter attributes that should be +treated as data providers rather than fixture-local bindings. Entries may be +written either as bare names such as `case` or qualified names such as +`rstest::case`; Whitaker normalizes them to the shared detection policy. + + +#### What is allowed + +- Single-use helper calls inside `#[rstest]` tests +- Helper calls whose totals stay below `min_calls` or `min_distinct_tests` +- Parameter-provider uses covered by `provider_param_attributes`, such as + `case`, `values`, `files`, `future`, and `context` + + +#### What is denied + +When diagnostic phases are implemented, `rstest_helper_should_be_fixture` will +deny repeated non-provider helper invocations across `#[rstest]` tests when +they meet or exceed both `min_calls` and `min_distinct_tests`. The +`require_identical_fixture_arg_names` setting controls whether candidate +fixture arguments must use the same names, and `use_source_callee_fallback` +controls whether source-callsite recovery may be used for macro-expanded callee +locations. + + +#### How to fix + +- Replace repeated helper calls with a shared `#[fixture]` parameter. +- If `require_identical_fixture_arg_names` is enabled, rename helper arguments + so repeated calls use the same fixture argument names. +- Prefer provider attributes listed in `provider_param_attributes` for + parameterized data inputs rather than modelling them as fixture-local helper + calls. +- Tune `min_calls`, `min_distinct_tests`, and `use_source_callee_fallback` when + repository conventions need stricter or looser matching. + +______________________________________________________________________ + +### `test_must_not_have_example` + +Warns when test function documentation includes example headings (for example +`# Examples`) or fenced code blocks. + +**Configuration:** + +```toml +[test_must_not_have_example] +additional_test_attributes = ["actix_rt::test", "my_framework::test"] +``` + +Use `additional_test_attributes` for frameworks not covered by default test +markers such as `#[test]`, `#[tokio::test]`, `#[async_std::test]`, +`#[gpui::test]`, and `#[rstest]`. + +**How to fix:** Keep test docs focused on intent and assertions, and move +example/tutorial snippets into user-facing documentation. + +```rust +// Before +#[test] +/// # Examples +/// ```rust +/// assert_eq!(sum(2, 2), 4); +/// ``` +fn sums_values() { /* ... */ } + +// After +#[test] +/// Verifies summation handles two positive integers. +fn sums_values() { /* ... */ } +``` + +______________________________________________________________________ + +### `no_std_fs_operations` + +Enforces capability-based filesystem access by forbidding direct use of +`std::fs` operations. + +**Configuration:** + +```toml +[no_std_fs_operations] +excluded_crates = ["my_cli_entrypoint", "my_test_utilities"] +excluded_paths = ["my_app::legacy_io", "my_app::bin::migrate"] +``` + +The `excluded_crates` option allows specified crates to use `std::fs` +operations without triggering diagnostics. This is useful for: + +- CLI entry points where ambient filesystem access is the intended boundary +- Test support utilities that manage fixtures with ambient access +- Build scripts or code generators that require direct filesystem operations + +The `excluded_paths` option narrows an exclusion to individual modules rather +than a whole crate. Each entry is a fully qualified path anchored at the crate +identifier, and it matches on segment boundaries: `my_app::legacy_io` exempts +that module and everything nested beneath it (for example +`my_app::legacy_io::reader`), but never a sibling such as +`my_app::legacy_io_utils`. Reach for this when only a bounded corner of a crate +needs ambient filesystem access, while the rest stays under the capability +policy. + +> **Note:** For both options, use Rust identifiers (underscores), not Cargo +> package names (hyphens). For example, use `my_cli_app` rather than +> `my-cli-app`, and `my_app::legacy_io` rather than `my-app::legacy_io`. +> +> **Tip:** For an ad hoc, single-site exemption that travels with the code, a +> standard `#[allow(no_std_fs_operations)]` attribute on the item or module +> also works, since the lint honours Rust's lint-level attributes. + +**How to fix:** Replace `std::fs` with `cap_std`: + +```rust +// Before +use std::fs; +fn read_config() -> std::io::Result { + fs::read_to_string("config.toml") +} + +// After +use cap_std::fs::Dir; +use camino::Utf8Path; +fn read_config(config_dir: &Dir, path: &Utf8Path) -> std::io::Result { + config_dir.read_to_string(path) +} +``` + +______________________________________________________________________ + +### `no_unwrap_or_else_panic` + +Denies panicking `unwrap_or_else` fallbacks on `Option`/`Result`, including +tests. Doctest runs remain exempt. + +**Configuration:** + +```toml +[no_unwrap_or_else_panic] +allow_in_main = true +``` + +**What is allowed:** + +- Panicking `unwrap_or_else` fallbacks inside doctests +- Panicking `unwrap_or_else` fallbacks inside `main` when + `allow_in_main = true` +- `unwrap_or_else(|| panic!("value was {:?}", value))` inside test code when + the closure interpolates a runtime value into the panic message +- Non-panicking `unwrap_or_else` fallbacks + +**What is denied:** + +- `unwrap_or_else(|| panic!(..))` outside tests, subject to the standard + denial +- `unwrap_or_else(|| panic!(..))` in tests unless the `panic!` message + interpolates runtime values +- `unwrap_or_else(|| panic!("static message"))` in tests; use + `.expect("static message")` instead +- `unwrap_or_else(|| value.unwrap())` + +**How to fix:** Propagate errors with `?` or use `.expect()` with a clear +message if a panic is truly intended. In tests, replace +`unwrap_or_else(|| panic!("msg"))` with `.expect("msg")` for clarity and +brevity, unless the closure needs to interpolate runtime state for a more +useful diagnostic. The rule denies only closures whose `panic!` message does +not meet the interpolated-only test exception. When the closure contains a +static string literal in tests, prefer `.expect("static message")`; only +interpolated-only `panic!` fallbacks are permitted there. + +## Clone Detection: AST Feature Extraction + +Whitaker's experimental clone detector runs in two passes. Pass A is a token +scan over the workspace; Pass B lifts each candidate span into an abstract +syntax tree (AST) substrate that later scoring will consume. This release ships +the Pass B substrate only — it does not yet report clones (see below). + +**What the substrate does.** Given a source file and a candidate byte range, +`lower_span` validates a non-empty, UTF-8-aligned `ByteSpan`, parses the +supplied source, and lowers the smallest syntax subtree that covers the span +into a parser-independent `NormalizedTree`. Working from the smallest covering +subtree keeps the representation tied to the candidate rather than the whole +file. + +**Feature vectors.** From a `NormalizedTree`, extraction derives a +deterministic set of features: + +- exact counts of each syntax kind; +- dyadic fixed-point depth weighting (`2^63 >> depth`), so weights halve with + depth and collapse to zero beyond depth 63; +- production bigrams and trigrams (parent-to-child and + grandparent-to-parent-to-child kind sequences); and +- an opaque hexadecimal canonical hash of the normalized subtree. + +**Schema-versioned hashes.** Every canonical hash mixes in +`PARSER_SCHEMA_VERSION`, which is tied to the pinned parser snapshot. A +parser-schema change therefore changes every hash by design, intentionally +invalidating persisted snapshots and caches so stale AST fingerprints are never +reused across incompatible parser versions. + +**Parser feature.** `whitaker_clones_core` enables its exact-pinned +`ra_ap_syntax` parser adapter through the default `parser` feature. The feature +is on by default; building the crate with it disabled makes `lower_span` return +`AstError::ParserUnavailable` instead of lowering anything. + +**Not yet emitted.** Type-3 clone scoring and SARIF Run 1 emission are deferred +to roadmap item 7.3.2. This release builds and exposes the AST substrate only; +it does not score clones or emit AST-based SARIF results. + +Contributors maintaining the pinned parser should follow the +[`ra_ap_syntax` re-pinning runbook](developers-guide.md#ra_ap_syntax-re-pinning-runbook) +in the Developer's Guide. From 239c7f64b8d1a07e2a2a7a0b727a908abc788f55 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 2 Aug 2026 09:56:47 +0200 Subject: [PATCH 6/6] Remove duplicate rebase documentation (#418) Retain main's tracing-test guidance and the branch's capability-scoped filesystem convention while removing the duplicate test-macros section introduced by semantic replay. --- docs/developers-guide.md | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index f8679eb3..017b1122 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -990,7 +990,6 @@ integration-test binary crates. The `#[expect(unused_macros)]` suppressions that previously guarded them were replaced by the export attribute when it became clear that multiple test binaries depend on them. - ### 2.3. Capability-scoped test filesystem access All integration tests under `tests/` use `camino::Utf8Path` and @@ -1027,28 +1026,6 @@ path-level exclusion only when the boundary cannot be expressed through [Whitaker user's guide](whitaker-users-guide.md#no_std_fs_operations) for the configuration syntax and segment-matching rules. - -### 2.4. `test-macros` crate - -The `test-macros` workspace crate provides the `allow_fixture_expansion_lints` -proc-macro attribute. It suppresses the `unused_braces` lint that `rstest` -fixture expansion triggers when `fn_single_line = true` is set in -`rustfmt.toml`. - -The macro emits `#[allow(unused_braces, …)]` rather than `#[expect(…)]` because -the Rust proc-macro API delivers a pre-parsed token stream; the emitted lint -attribute applies to code that the compiler has not yet expanded, making -`#[expect]` semantically unusable at that site. This is a known consequence of -the `rstest` fixture expansion and is not a lint-integrity violation. - -Apply it to any fixture function whose single-expression body triggers the lint: - -```rust -#[test_macros::allow_fixture_expansion_lints] -#[rstest::fixture] -pub fn broken_table() -> Vec { … } -``` - ### 2.4. `test-macros` crate The `test-macros` workspace crate provides the `allow_fixture_expansion_lints`