diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml index b39e799..3207097 100644 --- a/.github/workflows/tests.yaml +++ b/.github/workflows/tests.yaml @@ -29,7 +29,7 @@ env: jobs: # --------------------------------------------------------------------------- - # Lint (fmt + clippy + rustdoc) + # Lint (fmt + clippy + rustdoc + extended-lint) # --------------------------------------------------------------------------- lint: @@ -47,6 +47,13 @@ jobs: - name: Documentation run: make doc + - name: Fetch extended-lint diff base + run: | + git fetch origin "${GITHUB_BASE_REF:-main}":"refs/remotes/origin/${GITHUB_BASE_REF:-main}" --depth=1 + + - name: Extended lint + run: make extended-lint + # --------------------------------------------------------------------------- # Build + test # --------------------------------------------------------------------------- diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4b25cc6..f150770 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -23,6 +23,7 @@ make build # workspace build make test # all tests make fmt # format with nightly rustfmt make lint # clippy + nightly fmt check +make extended-lint # diff-scoped heuristic checks (TODOs, comment slop, repetition), via xtask make audit # cargo audit + cargo deny check ``` diff --git a/Cargo.lock b/Cargo.lock index 0f04d6d..fd05012 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -51,6 +51,12 @@ version = "1.0.14" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "940b3a0ca603d1eade50a4846a2afffd5ef57a9feac2c0e2ec2e14f9ead76000" +[[package]] +name = "anyhow" +version = "1.0.104" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "330a5ed07fa54e4702c9d6c4174f74427fc0ef6e214bbd677ae50a5099946470" + [[package]] name = "async-broadcast" version = "0.7.2" @@ -2785,6 +2791,14 @@ version = "0.6.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3ad82d2a33cdc9674dc7465672f271e096168fcdbe0f799d9e6db8c5892679dc" +[[package]] +name = "xtask" +version = "0.1.0" +dependencies = [ + "anyhow", + "regex", +] + [[package]] name = "yaml_serde" version = "0.10.6" diff --git a/Cargo.toml b/Cargo.toml index 36037ab..ab2f0f3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -6,6 +6,9 @@ rust-version = "1.96" license = "MIT" publish = false +[workspace] +members = ["xtask"] + [features] integration = [] diff --git a/Containerfile b/Containerfile index b98c436..250fead 100644 --- a/Containerfile +++ b/Containerfile @@ -21,12 +21,18 @@ WORKDIR /src # The manifest declares an explicit [[bench]], so cargo refuses to parse # it unless that file exists. Stub it alongside src/ or this layer fails -# before a single dependency is compiled. +# before a single dependency is compiled. The `xtask` workspace member is +# a dev-only tool never shipped in this image; `-p praxis-operator` keeps +# it out of the build target, but cargo still needs its manifest and a +# target file present to resolve the workspace, so it gets a permanent +# stub rather than being swapped for real source later. COPY Cargo.toml Cargo.lock ./ -RUN mkdir -p src benches \ +COPY xtask/Cargo.toml xtask/Cargo.toml +RUN mkdir -p src benches xtask/src \ && printf '//! stub\nfn main() {}\n' > src/main.rs \ && printf 'fn main() {}\n' > benches/config_generation.rs \ - && cargo build --release --locked \ + && printf 'fn main() {}\n' > xtask/src/main.rs \ + && cargo build --release --locked -p praxis-operator \ && rm -rf src # --------------------------------------------------------------------------- @@ -39,7 +45,7 @@ RUN mkdir -p src benches \ COPY src src COPY benches benches RUN touch src/main.rs \ - && cargo build --release --locked \ + && cargo build --release --locked -p praxis-operator \ && cp target/release/praxis-operator /usr/local/bin/ # --------------------------------------------------------------------------- diff --git a/Makefile b/Makefile index 9aec22b..fccc9c4 100644 --- a/Makefile +++ b/Makefile @@ -1,5 +1,5 @@ .PHONY: all build release check test lint fmt doc audit clean -.PHONY: coverage-check +.PHONY: coverage-check extended-lint .PHONY: require-container-engine images container praxis-image .PHONY: kind-up kind-down kind-reset conformance smoke-test .PHONY: dev-env dev-conformance dev-cycle dev-integration dev-push @@ -46,6 +46,9 @@ lint: cargo clippy --all-targets -- -D warnings cargo +nightly fmt --all -- --check +extended-lint: + cargo run -p xtask -- lint-extended + fmt: cargo +nightly fmt --all @@ -197,6 +200,7 @@ help: @echo "" @echo "Quality:" @echo " lint clippy + nightly rustfmt check" + @echo " extended-lint diff-scoped heuristic checks (TODOs, comment slop, repetition)" @echo " fmt format with nightly rustfmt" @echo " doc build docs with warnings denied" @echo " audit cargo audit + cargo deny" diff --git a/docs/conventions.md b/docs/conventions.md index 4096a65..b89d32f 100644 --- a/docs/conventions.md +++ b/docs/conventions.md @@ -25,6 +25,12 @@ - **cargo-audit**: Check for vulnerable dependencies - **cargo-deny**: Enforce supply chain safety policies - **rustdoc**: Generate the API documentation +- **`xtask lint-extended`** (`make extended-lint`): diff-scoped + heuristic checks for patterns clippy can't catch structurally -- + leftover `TODO`/`FIXME` markers, commented-out code, narrating "what" + comments, repeated literals that should be named constants, weak + identifier names, and new clippy suppressions. Only scans lines added + since the diff base, so pre-existing code is never relitigated. ### Comments vs Tracing diff --git a/xtask/Cargo.toml b/xtask/Cargo.toml new file mode 100644 index 0000000..75e82e0 --- /dev/null +++ b/xtask/Cargo.toml @@ -0,0 +1,33 @@ +[package] +name = "xtask" +version = "0.1.0" +edition = "2024" +rust-version = "1.96" +license = "MIT" +publish = false + +[dependencies] +anyhow = "1.0.100" +regex = "1.13.1" + +[lints.rust] +dead_code = "deny" +unsafe_code = "deny" +missing_docs = "deny" +unreachable_pub = "deny" +unused_imports = "deny" +unused_variables = "deny" +trivial_casts = "deny" +trivial_numeric_casts = "deny" +unused_qualifications = "deny" + +[lints.clippy] +# unwrap_used/expect_used/panic are intentionally not denied here: this is a +# dev-only build tool (not shipped production code), and its regexes are +# compile-time-constant literals compiled once into `LazyLock` statics, where +# `.unwrap()` is the standard idiom (a bad pattern would fail on first run). +todo = "deny" +unimplemented = "deny" +dbg_macro = "deny" +print_stdout = "allow" +print_stderr = "allow" diff --git a/xtask/src/lint_extended.rs b/xtask/src/lint_extended.rs new file mode 100644 index 0000000..5d5795e --- /dev/null +++ b/xtask/src/lint_extended.rs @@ -0,0 +1,325 @@ +//! Extended lint: diff-scoped heuristic checks for common low-quality-code +//! patterns that automated compiler lints can't catch structurally. +//! +//! Clippy already denies the machine-checkable half of this class of issue +//! (unwrap/expect, panic, todo!()/unimplemented!(), dead_code, missing_docs, +//! print/dbg macros, and more, depending on the crate's own lint config). +//! What lint tooling structurally cannot check is comment *content* and +//! diff-local *repetition* -- two common low-effort-code tells. This checks +//! only lines added/changed versus the diff base so pre-existing code is +//! never relitigated. +//! +//! Checks (Block = fails; Warn = printed, does not fail): +//! - Block: leftover TODO/FIXME/XXX/HACK markers in comments +//! - Block: commented-out code +//! - Warn: narrating "what the code does" comments +//! - Warn: the same numeric/string literal repeated 3+ times without a named constant +//! - Warn: weak/generic identifier names introduced by a new let/fn binding +//! - Warn: new clippy lint suppressions added +//! +//! Diff base resolution: CLI arg, else `$EXTENDED_LINT_BASE`, else +//! `origin/$GITHUB_BASE_REF` in a GitHub Actions PR, else `origin/main`. +//! +//! Known, deliberate limitation: the `xtask` crate excludes itself from its +//! own scan (see [`run_diff`]), so a genuine leftover TODO added to this +//! crate specifically won't be caught by this tool; everything else in the +//! workspace is scanned normally. + +use std::{ + collections::{HashMap, HashSet}, + process::Command, + sync::LazyLock, +}; + +use anyhow::{Context, Result}; +use regex::Regex; + +static TODO_MARKER_RE: LazyLock = LazyLock::new(|| Regex::new(r"(?i)//.*\b(TODO|FIXME|XXX|HACK)\b").unwrap()); +static COMMENTED_CODE_RE: LazyLock = LazyLock::new(|| { + Regex::new( + r#"^//+\s*(let\s+\w|fn\s+\w|if\s*\(|for\s*\(|match\s+\w|return\b|\w+\s*\([^)]*\)\s*;?\s*$|\w+\.\w+\(.*\)\s*;?\s*$|[\w:<>]+\s*=\s*.+;\s*$)"#, + ) + .unwrap() +}); +static WEAK_NAME_RE: LazyLock = + LazyLock::new(|| Regex::new(r"^(let(?:\s+mut)?|fn)\s+(temp|tmp|foo|bar|thing|val|obj|stuff)\b").unwrap()); +static LIT_RE: LazyLock = LazyLock::new(|| Regex::new(r#"(?:^|[^\w.])(\d{2,}|"[^"]{4,}")(?:$|[^\w])"#).unwrap()); +static CONST_LINE_RE: LazyLock = LazyLock::new(|| Regex::new(r"\b(const|static)\s+\w+").unwrap()); +static SUPPRESSION_RE: LazyLock = LazyLock::new(|| Regex::new(r"#\[(allow|expect)\(clippy::").unwrap()); +static TEST_MODULE_RE: LazyLock = LazyLock::new(|| Regex::new(r"^(#\[cfg\(test\)\]|mod tests\b)").unwrap()); + +const NARRATING_OPENERS: &[&str] = &[ + "increment", + "decrement", + "loop through", + "iterate over", + "iterate through", + "return the", + "returns the", + "create a", + "creates a", + "initialize", + "set the", + "sets the", + "get the", + "gets the", + "parse the", + "parses the", + "convert ", + "converts ", + "check if", + "checks if", + "validate that", + "validates that", + "call ", + "calls ", + "define ", + "defines ", + "import ", + "imports ", + "declare ", + "declares ", + "instantiate", + "loop over", + "append ", + "appends ", + "remove ", + "removes ", + "add ", + "adds ", +]; + +struct AddedLine { + file: String, + lineno: usize, + content: String, +} + +/// Maps a `(file, literal)` pair to the sites (source line text, line number) +/// where that literal was added. +type LiteralSites = HashMap<(String, String), Vec<(String, usize)>>; + +fn resolve_diff_base(cli_arg: Option<&str>) -> String { + if let Some(base) = cli_arg { + return base.to_string(); + } + if let Ok(base) = std::env::var("EXTENDED_LINT_BASE") { + return base; + } + if let Ok(base_ref) = std::env::var("GITHUB_BASE_REF") { + return format!("origin/{base_ref}"); + } + "origin/main".to_string() +} + +fn run_diff(diff_base: &str) -> Result> { + // Exclude this tool's own crate: its doc comments and test fixtures + // necessarily spell out the literal marker words and comment shapes + // these checks look for, so scanning them would trip the checks on + // examples rather than violations. The original `scripts/extended-lint.py` + // never had this problem since `.py` files never matched the `*.rs` glob. + let output = Command::new("git") + .args(["diff", "--unified=0", diff_base, "--", "*.rs", ":(exclude)xtask/**"]) + .output() + .context("failed to run git diff")?; + let stdout = String::from_utf8_lossy(&output.stdout); + + let mut added = Vec::new(); + let mut current_file = String::new(); + let mut new_lineno: usize = 0; + let hunk_re = Regex::new(r"^@@ -\d+(?:,\d+)? \+(\d+)").unwrap(); + + for line in stdout.lines() { + if let Some(path) = line.strip_prefix("+++ b/") { + current_file = path.to_string(); + continue; + } + if let Some(caps) = hunk_re.captures(line) { + new_lineno = caps[1].parse().unwrap_or(0); + continue; + } + if line.starts_with("+++") || line.starts_with("---") { + continue; + } + if let Some(content) = line.strip_prefix('+') { + added.push(AddedLine { + file: current_file.clone(), + lineno: new_lineno, + content: content.to_string(), + }); + new_lineno += 1; + } else if !line.starts_with('-') { + new_lineno += 1; + } + } + Ok(added) +} + +fn test_module_start_line(file: &str) -> usize { + let Ok(text) = std::fs::read_to_string(file) else { + return usize::MAX; + }; + for (i, line) in text.lines().enumerate() { + if TEST_MODULE_RE.is_match(line) { + return i + 1; + } + } + usize::MAX +} + +/// Runs the check; returns `Ok(true)` if clean, `Ok(false)` if blocking +/// findings exist (caller should exit non-zero in that case). +pub(crate) fn run(cli_arg: Option<&str>) -> Result { + let diff_base = resolve_diff_base(cli_arg); + let added = run_diff(&diff_base)?; + if added.is_empty() { + println!("[extended-lint] no added Rust lines vs {diff_base}; nothing to check."); + return Ok(true); + } + + let mut blocking = Vec::new(); + let mut warnings = Vec::new(); + let mut literal_sites: LiteralSites = HashMap::new(); + let mut const_declared: HashMap> = HashMap::new(); + + for line in &added { + let stripped = line.content.trim(); + let comment_text = line + .content + .find("//") + .map(|i| line.content[i..].trim().to_string()) + .unwrap_or_default(); + + if !comment_text.is_empty() && TODO_MARKER_RE.is_match(&comment_text) { + blocking.push(format!( + "{}:{}: leftover TODO/FIXME/XXX/HACK marker: {stripped:?}", + line.file, line.lineno + )); + } + + if !comment_text.is_empty() + && !comment_text.starts_with("///") + && !comment_text.starts_with("//!") + && COMMENTED_CODE_RE.is_match(&comment_text) + { + blocking.push(format!( + "{}:{}: looks like commented-out code: {stripped:?}", + line.file, line.lineno + )); + } + + if comment_text.starts_with("//") && !comment_text.starts_with("///") && !comment_text.starts_with("//!") { + let body = comment_text.trim_start_matches('/').trim().to_lowercase(); + if NARRATING_OPENERS.iter().any(|opener| body.starts_with(opener)) { + warnings.push(format!( + "{}:{}: narrating 'what' comment, prefer self-explanatory code or a doc comment on why: {stripped:?}", + line.file, line.lineno + )); + } + } + + if let Some(caps) = WEAK_NAME_RE.captures(stripped) { + let weak_name = &caps[2]; + warnings.push(format!( + "{}:{}: weak/generic identifier name {weak_name:?}: {stripped:?}", + line.file, line.lineno + )); + } + + if SUPPRESSION_RE.is_match(stripped) { + warnings.push(format!( + "{}:{}: new clippy suppression added, double-check the reason: {stripped:?}", + line.file, line.lineno + )); + } + + if CONST_LINE_RE.is_match(stripped) { + for caps in LIT_RE.captures_iter(stripped) { + const_declared + .entry(line.file.clone()) + .or_default() + .insert(caps[1].to_string()); + } + } + + if line.lineno < test_module_start_line(&line.file) && !stripped.starts_with("#[") { + for caps in LIT_RE.captures_iter(stripped) { + literal_sites + .entry((line.file.clone(), caps[1].to_string())) + .or_default() + .push((stripped.to_string(), line.lineno)); + } + } + } + + for ((file, literal), sites) in &literal_sites { + let declared = const_declared.get(file).is_some_and(|s| s.contains(literal)); + if sites.len() >= 3 && !declared { + let lines: Vec = sites.iter().map(|(_, l)| l.to_string()).collect(); + warnings.push(format!( + "{file}: literal {literal} repeated {}x at lines {} without a named constant -- consider hoisting it", + sites.len(), + lines.join(", ") + )); + } + } + + if !warnings.is_empty() { + eprintln!("[extended-lint] warnings (review, does not block):"); + for w in &warnings { + eprintln!(" - {w}"); + } + eprintln!(); + } + + if !blocking.is_empty() { + eprintln!("[extended-lint] BLOCKING findings:"); + for b in &blocking { + eprintln!(" - {b}"); + } + eprintln!(); + eprintln!("[extended-lint] fix the above, or if a match is a false positive, note why in the PR description."); + return Ok(false); + } + + eprintln!("[extended-lint] no blocking findings."); + Ok(true) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn detects_todo_marker() { + assert!(TODO_MARKER_RE.is_match("// TODO: fix this later")); + assert!(!TODO_MARKER_RE.is_match("// this is fine")); + } + + #[test] + fn detects_commented_out_code_but_not_doc_comments() { + assert!(COMMENTED_CODE_RE.is_match("// let x = compute();")); + assert!(!COMMENTED_CODE_RE.is_match("/// Returns the computed value.")); + } + + #[test] + fn detects_weak_names() { + let caps = WEAK_NAME_RE.captures("let temp = 5;").unwrap(); + assert_eq!(&caps[2], "temp"); + assert!(WEAK_NAME_RE.captures("let value = 5;").is_none()); + } + + #[test] + fn detects_narrating_comment_openers() { + assert!( + NARRATING_OPENERS + .iter() + .any(|o| "increment the counter by one".starts_with(o)) + ); + assert!( + !NARRATING_OPENERS + .iter() + .any(|o| "guards against a torn write".starts_with(o)) + ); + } +} diff --git a/xtask/src/main.rs b/xtask/src/main.rs new file mode 100644 index 0000000..40da54f --- /dev/null +++ b/xtask/src/main.rs @@ -0,0 +1,38 @@ +//! Development-time task runner for the `praxis-operator` workspace. +//! +//! Run `cargo run -p xtask -- [ARGS]`. See [`lint_extended`] +//! for the `lint-extended` subcommand. + +mod lint_extended; + +use std::process::ExitCode; + +const USAGE: &str = "Usage: cargo run -p xtask -- [ARGS]\n\n\ +Subcommands:\n \ +lint-extended [DIFF_BASE] diff-scoped heuristic checks for comment/repetition smells\n"; + +fn main() -> ExitCode { + let mut args = std::env::args().skip(1); + match args.next().as_deref() { + Some("lint-extended") => run_lint_extended(args.next().as_deref()), + Some(other) => { + eprintln!("xtask: unknown subcommand '{other}'\n\n{USAGE}"); + ExitCode::FAILURE + }, + None => { + eprintln!("{USAGE}"); + ExitCode::FAILURE + }, + } +} + +fn run_lint_extended(diff_base: Option<&str>) -> ExitCode { + match lint_extended::run(diff_base) { + Ok(true) => ExitCode::SUCCESS, + Ok(false) => ExitCode::FAILURE, + Err(err) => { + eprintln!("xtask: lint-extended failed: {err:#}"); + ExitCode::FAILURE + }, + } +}