diff --git a/CHANGELOG.md b/CHANGELOG.md index d3bb08b..948df20 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,43 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Added + +- `stray-const`, an opt-in rule that reports `SCREAMING_SNAKE_CASE` constants + **declared** outside the files named by `const-files`. A constant is a + decision the program has made -- a limit, a path, a key, a magic number + somebody named -- and scattered across the tree those decisions cannot be + read as a set, so the same one gets made twice under two names. + `const-files` is `theme-files` for constants: a declaration inside one is + what the rule asks for, everywhere else is an error. Enable with + `stray-const = true` or `--stray-const`; enabling it without naming a file + is refused, because every constant would be a finding with nowhere to move + it. + + The analysis is [beamte](https://github.com/PowderworksCode/beamte)'s + `const-declaration`, and it parses rather than matching text because the + whole content of the rule is the difference between declaring a name and + using one. Text cannot tell those apart without a table of declaration + keywords per language; the node vocabulary answers it in one form for every + grammar, so an import binds a name without declaring it, a parameter is not + a constant, a function's locals cannot be moved to another file, and a use + is not a binding at all. Ten languages, the ones treebank publishes a + grammar for. Opt-in twice over: the first scan of a language downloads its + grammar, and the rule has nothing to say until the files are named. + +### Changed + +- The pack cache moves to `src/pack.rs`, shared, so two rules meeting the same + language in one scan JIT its grammar once between them rather than each + keeping a copy. +- `test-quality` renders instructions for the test-scoped rules only. beamte's + catalogue now holds rules it does not run, and advertising one would promise + a check `test-quality` never makes. +- beamte 0.3: `Rule::property` and `Rule::citation` are `Option`, so a rule may + state a structural fact rather than restate a published argument. + `severity_of` maps a rule with no test property to a warning, since it makes + no claim that mapping is about. + ## [0.2.0] - 2026-08-30 ### Added diff --git a/Cargo.lock b/Cargo.lock index e3cdf4b..b439e49 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -151,9 +151,8 @@ checksum = "ac07cdecf99051d9a5238b80f35af32cdeba5b336e55d957b318b50137e18da5" [[package]] name = "beamte" -version = "0.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d369373b6cfa7fd9d2e8c8716ff29a31b111aabdb3ecaf26d07d60b218df553b" +version = "0.3.0" +source = "git+https://github.com/PowderworksCode/beamte?rev=2aa372ecec972e9b3535885c4615968d6a0878bc#2aa372ecec972e9b3535885c4615968d6a0878bc" dependencies = [ "treebank", ] @@ -405,7 +404,7 @@ version = "3.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "faf9468729b8cbcea668e36183cb69d317348c2e08e994829fb56ebfdfbaac34" dependencies = [ - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -880,7 +879,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb" dependencies = [ "libc", - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -1570,7 +1569,7 @@ version = "0.50.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7957b9740744892f114936ab4a57b3f487491bbeafaf8083688b16841a4240e5" dependencies = [ - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -1960,7 +1959,7 @@ dependencies = [ "bitflags", "libc", "mach2 0.4.3", - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -2047,7 +2046,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys", - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -2113,7 +2112,7 @@ dependencies = [ "security-framework", "security-framework-sys", "webpki-root-certs", - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -2528,7 +2527,7 @@ dependencies = [ "getrandom 0.4.3", "once_cell", "rustix", - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] @@ -3114,7 +3113,7 @@ version = "0.1.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c2a7b1c03c876122aa43f3020e6c3c3ee5c05081c9a00739faf7503aeba10d22" dependencies = [ - "windows-sys 0.61.2", + "windows-sys 0.59.0", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index 0d5b42c..38c1a5a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -34,7 +34,7 @@ wasmer = { version = "7", default-features = false, features = ["sys", "cranelif # `pure` selects the Rust implementation instead, and naming blake3 here is what # unifies that feature across the whole graph. blake3 = { version = "1", default-features = false, features = ["pure"] } -beamte = "0.1" +beamte = "0.3" clap = { version = "4", features = ["derive"] } ignore = "0.4" inventory = "0.3" @@ -64,3 +64,15 @@ let_underscore_must_use = "deny" [profile.release] lto = true strip = true + +# beamte 0.3 (`const-declaration`, and `Rule::property`/`citation` as +# `Option`) is not on crates.io yet. Until it is, the requirement above +# resolves through this patch to the commit that carries it. +# +# A commit rather than a branch, because a branch name stops resolving the +# moment the branch is merged and deleted, which is how #63 broke: the +# dependency vanished under a PR that had not changed. A merged commit stays +# reachable. Drop this table the moment 0.3 is published -- the requirement +# above is already written for the registry. +[patch.crates-io] +beamte = { git = "https://github.com/PowderworksCode/beamte", rev = "2aa372ecec972e9b3535885c4615968d6a0878bc" } diff --git a/README.md b/README.md index 2059a3d..6fb9341 100644 --- a/README.md +++ b/README.md @@ -27,8 +27,12 @@ src/theme/button.css:12:14 [color] #ff6600 straitjacket: 1 error(s), 0 warning(s) across 84 file(s); 0 suppressed ``` -Eleven rules ship; nine run at the first invocation. The other two you opt -into: `no-comments`, and `test-quality`, which reads your tests the way the +Twelve rules ship; nine run at the first invocation. The other three you opt +into: `no-comments`; `stray-const`, which reports `SCREAMING_SNAKE_CASE` +constants *declared* anywhere but the files you designate as their home, so +the decisions a program has made can be read as a set rather than hunted for +— it parses, because telling a declaration from a use is a question about the +tree; and `test-quality`, which reads your tests the way the language writes them — `#[test]`, `@Test`, `it(...)`, `TEST(...)`, `test "..."` — and flags the ones that weaken what they prove, such as a loop or a conditional in a test body. It parses with a diff --git a/site/content/getting-started.md b/site/content/getting-started.md index 10849d0..5b5b957 100644 --- a/site/content/getting-started.md +++ b/site/content/getting-started.md @@ -37,7 +37,7 @@ straitjacket ``` With no arguments, Straitjacket scans the current directory, honoring your -`.gitignore`. Nine of the eleven rules are on by default — it runs near its +`.gitignore`. Nine of the twelve rules are on by default — it runs near its max and you ratchet down later. The other two are modes you opt into: `no-comments`, and `test-quality`, which parses your tests with a downloaded grammar. diff --git a/site/content/index.md b/site/content/index.md index 11efb30..8409ffd 100644 --- a/site/content/index.md +++ b/site/content/index.md @@ -18,7 +18,7 @@ Run `straitjacket` at the root of any project. It honors your `.gitignore`, prints one line per finding as `path:line:col [rule] matched`, and exits non-zero on any error — so CI fails the moment slop lands. -Nine of the eleven rules are on at the first run, so the strictest +Nine of the twelve rules are on at the first run, so the strictest Straitjacket gets by default takes no configuration to reach. What you disagree with, you turn off — `--skip` for a run, `straitjacket.toml` for good, `straitjacket-allow` on the one line you meant. The other two you opt diff --git a/site/content/reference/config-file.md b/site/content/reference/config-file.md index 5b9faca..18b2e2f 100644 --- a/site/content/reference/config-file.md +++ b/site/content/reference/config-file.md @@ -55,6 +55,8 @@ Every key mirrors a [CLI flag](/reference/cli) one-for-one, in | `test-rules` | list of [test rule](/reference/rules#test-quality) ids; unset runs all | — | | `test-quality` | boolean | `--test-quality` ([test quality](/reference/rules#test-quality)) | | `no-comments` | boolean | `--no-comments` ([no-comments mode](/reference/rules#no-comments-mode)) | +| `stray-const` | boolean | `--stray-const` ([stray constants](/reference/rules#stray-constants)) | +| `const-files` | list of files constants are declared in — required when `stray-const` is on | — | | `include-json` | boolean | `--include-json` | | `no-ignore` | boolean | `--no-ignore` | | `no-fail` | boolean | `--no-fail` | diff --git a/site/content/reference/rules.md b/site/content/reference/rules.md index 592d2a0..4efdce2 100644 --- a/site/content/reference/rules.md +++ b/site/content/reference/rules.md @@ -33,6 +33,7 @@ installed version ever disagree. | `stray-todo` | on | deferred-work markers left in comments — `TODO`, `TBD`, `FIXME`, `WIP`. Do the work now, or record it in an issue the repository tracks. Exempt path prefixes with `todo-exclude`. | | `unused-marker` | on | a suppression marker that did not suppress anything — the finding it was written for is gone, so the marker is stale. Turn it off with `--no-fail-on-unused-markers`. | | `no-comments` | **opt-in** | every comment, in every language it knows (`//`, `/* */`, `#`, `--`, ``). See [no-comments mode](#no-comments-mode) below. | +| `stray-const` | **opt-in** | `SCREAMING_SNAKE_CASE` constants **declared** outside the files named by `const-files` — a limit, a path, a key or a magic number named where it happens to be used rather than where the program keeps its decisions. Parses with a treebank grammar, so it can tell a declaration from a use. See [stray constants](#stray-constants) below. | | `test-quality` | **opt-in** | tests that weaken what they prove — currently a loop or a conditional in a test body. Parses the file with a treebank grammar, so it reads a test the way the language writes one: `#[test]`, `@Test`, `it(...)`, `TEST(...)`, `test "..."`. See [test quality](#test-quality) below. | ### `deep-nesting` and embedded DSLs @@ -211,3 +212,76 @@ existing repository. | respect `.gitignore` | on | `--no-ignore` | | fail on unused markers | on | `--no-fail-on-unused-markers` | | fail on findings | on | `--no-fail` | + +## stray constants + +A constant is a decision the program has made: a limit, a retry count, a path, +a key, a magic number somebody named. Scattered across the tree those decisions +cannot be read as a set, nobody can tell which are still true, and the same one +gets made twice under two names. `stray-const` reports a +`SCREAMING_SNAKE_CASE` declaration anywhere but the files you designate: + +```sh +straitjacket --stray-const +``` + +or in [`straitjacket.toml`](/reference/config-file): + +```toml +stray-const = true +const-files = ["src/consts.rs", "src/env.rs"] +``` + +`const-files` is `theme-files` for constants: a declaration inside one is what +the rule is asking for, and everywhere else is an error. Enabling the rule +without naming a file is refused rather than obeyed — every constant would be +a finding with nowhere to move it, which is a configuration nobody means. + +**Declarations, not uses.** `MAX_SIZE` mentioned in an expression is the whole +point of having a constant; only the line that introduces the name is a +finding. A rule that flagged uses could not be satisfied. + +That distinction is the reason this rule parses. Telling a declaration from a +use is a question about the tree, and answering it from text needs a table of +declaration keywords per language — a parser written badly, which is what the +first version of this rule was. The analysis is +[beamte](https://github.com/PowderworksCode/beamte)'s `const-declaration`, +which asks the node vocabulary instead: + +| shape | what the tree says | verdict | +|---|---|---| +| `MAX_SIZE = 3` | a binding | declared | +| `const MAX_SIZE: u8 = 3` | a binding | declared | +| `from settings import MAX_SIZE` | a binding, and a directive | imported, not declared | +| `def f(MAX_SIZE)` | a binding, and a parameter | a parameter, not a constant | +| `n > MAX_SIZE` | neither | a use | + +A name bound inside a function is a local — it cannot be moved to another +file, so it is not reported. Because the rule reads the vocabulary rather than +any language's syntax, there is no per-language table to drift: a constant is +recognised the same way in every grammar. + +**It is opt-in for two reasons**, either enough alone. The grammar for a +language is downloaded the first time a file in that language is scanned, so +the rule reaches the network; and it has nothing to say until you name the +files constants belong in, which is a decision no default can make. A file +whose grammar cannot be fetched is reported as **not read** rather than +passing quietly. + +### What counts as a constant + +A name of at least two words joined by underscores — `MAX_SIZE`, +`DEFAULT_PATH`, `API_BASE_URL`. A single all-caps word is deliberately left +alone: `PI`, `OK`, `HTTP`, a Go export, a C header guard and a type parameter +are all spelled that way, and flagging them would bury the constants among +them. + +Ten languages, being the ones treebank publishes a grammar for: Python, Ruby, +Rust, Java, TypeScript, JavaScript, C, C++, Shell and Zig. + +One miss is worth naming. An enum member written as an assignment in a class +body — Python's `RED_ONE = 1` inside `class Colour(Enum)` — is a binding +outside any function and is reported, though it cannot be moved either. +Telling an enum from a class needs its base class, which is a fact about a +library rather than about the tree; name those files in `const-files`, or +suppress with a marker. diff --git a/site/content/rules.json b/site/content/rules.json index d9f8c6f..3db3bdd 100644 --- a/site/content/rules.json +++ b/site/content/rules.json @@ -42,6 +42,11 @@ "summary": "ordinary comment outside the leading file header", "default_enabled": false }, + { + "id": "stray-const", + "summary": "a constant is declared outside the designated constant files", + "default_enabled": false + }, { "id": "stray-todo", "summary": "deferred-work marker left in a comment", diff --git a/src/config.rs b/src/config.rs index 158e72f..c76a3f4 100644 --- a/src/config.rs +++ b/src/config.rs @@ -22,6 +22,13 @@ pub struct FileConfig { pub theme_files: Option>, pub max_nesting: Option, pub no_comments: Option, + /// Turn on `stray-const`, which reports SCREAMING_SNAKE_CASE constants + /// declared outside the files named by `const-files`. + pub stray_const: Option, + /// The files constants are declared in. `theme-files` for constants: a + /// declaration inside one is what the rule asks for, everywhere else it + /// is a finding. + pub const_files: Option>, pub test_quality: Option, pub include_json: Option, pub no_ignore: Option, @@ -55,6 +62,8 @@ pub struct Settings { pub max_nesting: usize, pub no_comments: bool, pub test_quality: bool, + pub stray_const: bool, + pub const_files: Vec, pub include_json: bool, pub no_ignore: bool, pub no_fail: bool, @@ -77,6 +86,8 @@ impl Default for Settings { max_nesting: DEFAULT_MAX_NESTING, no_comments: false, test_quality: false, + stray_const: false, + const_files: Vec::new(), include_json: false, no_ignore: false, no_fail: false, @@ -126,6 +137,12 @@ impl Settings { if let Some(value) = file.no_comments { self.no_comments = value; } + if let Some(value) = file.stray_const { + self.stray_const = value; + } + if let Some(paths) = file.const_files { + self.const_files = paths.into_iter().map(PathBuf::from).collect(); + } if let Some(value) = file.test_quality { self.test_quality = value; } diff --git a/src/main.rs b/src/main.rs index f2c36ff..4fd93f1 100644 --- a/src/main.rs +++ b/src/main.rs @@ -52,6 +52,9 @@ struct Cli { #[arg(long, help = "Enable the opt-in `no-comments` rule")] no_comments: bool, + + #[arg(long, help = "Enable the opt-in `stray-const` rule")] + stray_const: bool, #[arg(long, help = "Enable the opt-in `test-quality` rule")] test_quality: bool, @@ -333,6 +336,7 @@ fn resolve(cli: &Cli) -> anyhow::Result { settings.max_nesting = value; } settings.no_comments |= cli.no_comments; + settings.stray_const |= cli.stray_const; settings.test_quality |= cli.test_quality; settings.include_json |= cli.include_json; settings.no_ignore |= cli.no_ignore; diff --git a/src/pack.rs b/src/pack.rs index b7fb8a3..d656b09 100644 --- a/src/pack.rs +++ b/src/pack.rs @@ -9,8 +9,10 @@ //! `tb_*` ABI, statically linked, importing only WASI. It is not linked C, so //! it cannot break the musl cross-build the release depends on. +use std::cell::RefCell; use std::collections::HashMap; use std::path::Path; +use std::rc::Rc; use std::sync::Mutex; use anyhow::{Context, Result, bail}; @@ -399,6 +401,45 @@ impl Loaded { } } +thread_local! { + /// Loaded packs, and the reasons for the ones that would not load. + /// Shared by every rule that parses, so two rules meeting the same + /// language in one scan JIT its grammar once between them. + /// + /// A `FileRule` must be `Send + Sync` and a wasmer `Store` is neither, so + /// the packs cannot live in a rule. They live beside the rules instead, + /// which costs nothing today -- the walk in `src/walk.rs` is a single + /// sequential iterator -- and stays correct rather than unsound if that + /// ever changes. A parallel walk would pay one JIT per thread per grammar. + /// + /// The failure is cached with the same weight as the success: a machine + /// with no network pays one failed fetch, not one per file. + static CACHED: RefCell, String>>> = + RefCell::new(HashMap::new()); +} + +/// The pack for a grammar, fetched once and then reused. +/// +/// Fetched per language, and only once a rule has already decided a file is +/// worth parsing, so a Python repository never downloads the Java grammar. +pub fn cached(grammar: &'static str) -> std::result::Result, String> { + CACHED.with_borrow_mut(|packs| { + packs + .entry(grammar) + .or_insert_with(|| { + acquire(grammar) + .map(Rc::new) + .map_err(|error| format!("{error:#}")) + }) + .clone() + }) +} + +fn acquire(grammar: &'static str) -> Result { + let bytes = treebank::fetch::fetch_bytes(grammar)?; + Pack::from_bytes(&bytes, &format!("the treebank {grammar} pack")) +} + fn intern(table: &mut Vec, ids: &mut HashMap, value: String) -> u32 { if let Some(id) = ids.get(&value) { return *id; diff --git a/src/rules/mod.rs b/src/rules/mod.rs index 9eaa4d2..ac18cd0 100644 --- a/src/rules/mod.rs +++ b/src/rules/mod.rs @@ -9,6 +9,7 @@ mod key; mod motion; mod no_comments; mod regex_rule; +mod stray_const; mod stray_todo; mod test_quality; mod unused_marker; @@ -136,6 +137,23 @@ pub fn resolve(names: &[String]) -> anyhow::Result> { /// Straitjacket carries one rule key for all of them, so these names never /// reach [`resolve`] and would otherwise be accepted silently -- a typo in /// `test-rules` would quietly turn a rule off rather than say so. +/// `stray-const` with nowhere to put a constant. +/// +/// Every constant in the repository would be a finding and none of them could +/// be fixed, which is a configuration nobody means. `deny_unknown_fields` +/// cannot catch this one -- both keys are spelled right -- so it is caught +/// here, where the rule is known to be running. +pub fn check_const_files(enabled: bool, settings: &Settings) -> anyhow::Result<()> { + if enabled && settings.const_files.is_empty() { + bail!( + "`stray-const` is on but `const-files` names no file, so every \ + constant would be a finding with nowhere to move it. Name the \ + file(s) constants belong in." + ); + } + Ok(()) +} + pub fn resolve_test_rules(names: &[String]) -> anyhow::Result<()> { let unknown: Vec<&str> = names .iter() @@ -157,5 +175,6 @@ pub fn resolve_test_rules(names: &[String]) -> anyhow::Result<()> { } pub use no_comments::KEY as NO_COMMENTS; +pub use stray_const::KEY as STRAY_CONST; pub use test_quality::KEY as TEST_QUALITY; pub use unused_marker::{KEY as UNUSED_MARKER, descriptor as unused_marker_descriptor}; diff --git a/src/rules/stray_const.rs b/src/rules/stray_const.rs new file mode 100644 index 0000000..e5cb80d --- /dev/null +++ b/src/rules/stray_const.rs @@ -0,0 +1,318 @@ +//! Constants declared outside the files that hold them, found by beamte's +//! `const-declaration` rule over a treebank pack. +//! +//! Straitjacket implements none of the analysis and must not start. Beamte +//! owns what a constant declaration *is*; this file owns everything beamte +//! refuses to: getting a grammar, parsing, deciding a severity, and -- the +//! part that is policy about a repository rather than a fact about a tree -- +//! naming the files that hold the constants. `const-files` in +//! `straitjacket.toml` names them, and a declaration inside one is what the +//! rule is asking for rather than something to report. +//! +//! The analysis is beamte's because the question is a tree question. The +//! whole content of the rule is the difference between *declaring* a name and +//! *using* one, and text cannot tell those apart without a per-language table +//! of declaration keywords -- which is a parser written badly, and was the +//! first attempt at this rule. Beamte asks the vocabulary instead: a +//! `_binding` that is neither an import nor a parameter, outside any +//! callable. That needs no language table at all, so unlike `test-quality` +//! there is nothing here to keep in step per language beyond which pack +//! serves which grammar. +//! +//! A finding is an error rather than a mapping of beamte's property -- the +//! rule has no property, being a fact about how code is arranged rather than +//! a claim about a test. It is opt-in, and a repository that turned it on +//! wants the declaration moved, not mentioned. + +use beamte::node::Unit; +use beamte::{RuleId, Selection}; + +use crate::Settings; +use crate::finding::{Finding, Location, Severity}; +use crate::language::LanguageProfile; +use crate::rule::{Candidate, FileRule, RuleDescriptor, RuleKey, SourceFile}; +use crate::rules::RuleRegistration; + +pub const KEY: RuleKey = RuleKey::new("stray-const"); + +/// Off unless a configuration asks for it. +/// +/// Two reasons, either enough on its own. The first scan of a language +/// downloads its grammar, and a scan that reaches the network because the +/// tool was upgraded is not a surprise anyone should get for free. And the +/// rule has nothing to say until somebody says where constants belong, which +/// is a decision no default can make. +const DEFAULT_ENABLED: bool = false; + +/// A language this rule can read: the ten treebank publishes a grammar for. +/// +/// Only the pack name is needed. beamte's rule reads the node vocabulary +/// rather than any language's declaration syntax, so there is no per-language +/// table to drift -- which is the difference between this rule and both of +/// straitjacket's other beamte hosts. +const SUPPORTED: &[(&str, &str)] = &[ + ("c", "c"), + ("cpp", "cpp"), + ("java", "java"), + ("javascript", "typescript"), + ("python", "python"), + ("ruby", "ruby"), + ("rust", "rust"), + ("shell", "bash"), + ("typescript", "typescript"), + ("zig", "zig"), +]; + +fn supported(language: &LanguageProfile) -> Option<&'static str> { + SUPPORTED + .iter() + .find(|(id, _)| *id == language.id) + .map(|(_, pack)| *pack) +} + +pub struct StrayConstRule { + /// The designated homes. A declaration inside one is what the rule is + /// asking for, so it is not reported. Same matching as + /// `file-size-exclude`, so one notion of "this path" covers both. + allow: Vec, + /// `Only(const-declaration)`, resolved through beamte's catalogue so a + /// name this crate no longer has cannot reach a scan. + only: Vec, +} + +impl StrayConstRule { + pub fn new(allow: Vec) -> Self { + let only = beamte::catalogue() + .iter() + .filter(|rule| rule.id.as_str() == "const-declaration") + .map(|rule| rule.id) + .collect(); + Self { allow, only } + } + + fn designated(&self, path: &str) -> bool { + let path = std::path::Path::new(path); + self.allow.iter().any(|allowed| { + path == allowed + || path.starts_with(allowed) + || allowed.is_relative() && path.is_absolute() && path.ends_with(allowed) + }) + } + + fn hint(&self) -> String { + match self.allow.first() { + Some(home) => format!( + "declare it in {} and reference it from here", + home.display() + ), + None => "declare it in one of the files named by `const-files`".to_string(), + } + } +} + +fn build(settings: &Settings) -> Box { + Box::new(StrayConstRule::new(settings.const_files.clone())) +} + +fn instruction(settings: &Settings) -> String { + let sentence = beamte::rule("const-declaration") + .map(|rule| rule.instruction.to_string()) + .unwrap_or_default(); + if settings.const_files.is_empty() { + return sentence; + } + let designated = settings + .const_files + .iter() + .map(|path| path.display().to_string()) + .collect::>() + .join(", "); + format!("{sentence} The files that hold them are {designated}.") +} + +inventory::submit! { + RuleRegistration { + key: KEY, + factory: Some(build), + instruction, + } +} + +impl FileRule for StrayConstRule { + fn descriptor(&self) -> RuleDescriptor { + RuleDescriptor { + id: KEY, + summary: "a constant is declared outside the designated constant files", + default_enabled: DEFAULT_ENABLED, + } + } + + fn applies_to(&self, language: &LanguageProfile) -> bool { + supported(language).is_some() + } + + fn check(&self, file: SourceFile<'_>, candidates: &mut Vec) { + let Some(pack_name) = supported(file.language) else { + return; + }; + if self.designated(file.path) { + return; + } + if !looks_like_a_constant(file.text) { + return; + } + let Some(model) = beamte::TestModel::for_language(file.language.id) + .or_else(|| beamte::TestModel::for_language(pack_name)) + else { + return; + }; + + let pack = match crate::pack::cached(pack_name) { + Ok(pack) => pack, + Err(reason) => { + candidates.push(not_read( + file.path, + format!( + "not read: the {pack_name} grammar could not be loaded, so this \ + file was not checked for constant declarations ({reason})" + ), + Some( + "Packs are downloaded once and cached. Check network access, or \ + skip `stray-const` if this environment is offline by design." + .to_string(), + ), + )); + return; + } + }; + + let tree = match pack.parse(file.text) { + Ok(tree) => tree, + Err(error) => { + candidates.push(not_read( + file.path, + format!("not read: this file did not parse as {pack_name} ({error:#})"), + None, + )); + return; + } + }; + + let unit = Unit::new(file.path, tree.source(), tree.root()); + for finding in beamte::inspect_with(&unit, &model, Selection::Only(&self.only)) { + let line = finding.span.line; + let column = finding.span.column; + let mut owned = Finding::new( + KEY, + Severity::Error, + Location::point(file.path, line, column), + matched_text(file.text, line), + finding.message, + ); + owned.help = Some(self.hint()); + candidates.push(Candidate::line(owned)); + } + } +} + +/// Whether the text holds anything shaped like a screaming-snake name. +/// +/// The cheap half of the decision, and the reason a file that cannot produce +/// a finding is never handed to a grammar. It over-reports on purpose: `A_B` inside a +/// comment passes here and is dismissed by the parse, which is the right way +/// round for a prefilter. +fn looks_like_a_constant(text: &str) -> bool { + let mut upper_run = 0usize; + let mut seen_underscore = false; + for character in text.chars() { + if character.is_ascii_uppercase() || character.is_ascii_digit() { + upper_run += 1; + } else if character == '_' && upper_run > 0 { + seen_underscore = true; + } else { + if seen_underscore && upper_run > 1 { + return true; + } + upper_run = 0; + seen_underscore = false; + } + } + seen_underscore && upper_run > 0 +} + +/// A file that could not be checked, said out loud. +/// +/// Beamte's DESIGN.md §7.3: a file that was not read is reported as unread, +/// never as clean. Returning nothing would mean a failed pack fetch reads +/// exactly like a file with nothing wrong in it. +fn not_read(path: &str, message: String, help: Option) -> Candidate { + let mut finding = Finding::new( + KEY, + Severity::Warning, + Location::point(path, 1, 1), + String::new(), + message, + ); + finding.help = help; + Candidate::file(finding) +} + +/// The line a finding sits on, trimmed, for the `matched` field every other +/// rule fills in from its own regex. +fn matched_text(text: &str, line: usize) -> String { + text.lines() + .nth(line.saturating_sub(1)) + .unwrap_or_default() + .trim() + .to_string() +} + +#[cfg(test)] +mod tests { + use super::{KEY, SUPPORTED, StrayConstRule, looks_like_a_constant}; + + #[test] + fn every_supported_language_is_one_straitjacket_knows() { + for (id, _) in SUPPORTED { + assert!( + crate::language::language_profile(id).is_some(), + "{id} is not a language straitjacket has a profile for" + ); + } + } + + #[test] + fn every_supported_language_has_a_beamte_model() { + for (id, pack) in SUPPORTED { + assert!( + beamte::TestModel::for_language(id).is_some() + || beamte::TestModel::for_language(pack).is_some(), + "{id} maps to no beamte model" + ); + } + } + + #[test] + fn the_prefilter_keeps_what_could_be_a_constant_and_drops_what_could_not() { + assert!(looks_like_a_constant("const MAX_SIZE = 3;")); + assert!(looks_like_a_constant("A_B")); + assert!(!looks_like_a_constant("let max_size = 3;")); + assert!(!looks_like_a_constant("fn main() {}")); + assert!(!looks_like_a_constant("PI")); + } + + #[test] + fn a_designated_file_is_designated_however_the_walk_spells_it() { + let rule = StrayConstRule::new(vec!["src/consts.rs".into(), "src/env/".into()]); + + assert!(rule.designated("src/consts.rs")); + assert!(rule.designated("/repo/src/consts.rs")); + assert!(rule.designated("src/env/keys.rs")); + assert!(!rule.designated("src/main.rs")); + } + + #[test] + fn the_key_is_the_one_the_registry_carries() { + assert_eq!(KEY.as_str(), "stray-const"); + } +} diff --git a/src/rules/test_quality.rs b/src/rules/test_quality.rs index 81a1911..f215a42 100644 --- a/src/rules/test_quality.rs +++ b/src/rules/test_quality.rs @@ -16,17 +16,12 @@ //! and a project that dislikes one rule are both real. `test-rules` in //! `straitjacket.toml` names them; unset means all of them. -use std::cell::RefCell; -use std::collections::HashMap; -use std::rc::Rc; - use beamte::node::Unit; use beamte::{Property, RuleId, Selection}; use crate::Settings; use crate::finding::{EvidenceStep, Finding, Location, Severity}; use crate::language::LanguageProfile; -use crate::pack::Pack; use crate::rule::{Candidate, FileRule, RuleDescriptor, RuleKey, SourceFile}; use crate::rules::RuleRegistration; @@ -133,44 +128,6 @@ fn path_names_a_test(path: &str) -> bool { lowered.contains("test") || lowered.contains("spec") } -thread_local! { - /// Loaded packs, and the reasons for the ones that would not load. - /// - /// A `FileRule` must be `Send + Sync` and a wasmer `Store` is neither, so - /// the packs cannot live in the rule. They live beside it instead, which - /// costs nothing today -- the walk in `src/walk.rs` is a single sequential - /// iterator -- and stays correct rather than unsound if that ever changes. - /// A parallel walk would pay one JIT per thread per grammar. - /// - /// The failure is cached with the same weight as the success: a machine - /// with no network pays one failed fetch, not one per file. - static PACKS: RefCell, String>>> = - RefCell::new(HashMap::new()); -} - -/// The pack for a grammar, fetched once and then reused. -/// -/// Fetched per language, and only once a file of that language has already -/// looked like a test, so a Python repository never downloads the Java -/// grammar. -fn pack(grammar: &'static str) -> Result, String> { - PACKS.with_borrow_mut(|packs| { - packs - .entry(grammar) - .or_insert_with(|| { - acquire(grammar) - .map(Rc::new) - .map_err(|error| format!("{error:#}")) - }) - .clone() - }) -} - -fn acquire(grammar: &'static str) -> anyhow::Result { - let bytes = treebank::fetch::fetch_bytes(grammar)?; - Pack::from_bytes(&bytes, &format!("the treebank {grammar} pack")) -} - pub struct TestQualityRule { /// Empty means every rule beamte has, which is also what it will mean /// after beamte grows one. @@ -205,10 +162,19 @@ fn build(settings: &Settings) -> Box { Box::new(TestQualityRule::new(only)) } +/// The catalogue holds rules this one does not run. A file-scoped rule +/// belongs to whichever rule reads whole files, and advertising it here would +/// promise a check `test-quality` never makes. fn instruction(_settings: &Settings) -> String { let mut sentences = Vec::new(); for rule in beamte::catalogue() { - sentences.push(format!("{} ({})", rule.instruction, rule.citation.title)); + if rule.scope != beamte::Scope::Tests { + continue; + } + match rule.citation { + Some(citation) => sentences.push(format!("{} ({})", rule.instruction, citation.title)), + None => sentences.push(rule.instruction.to_string()), + } } sentences.join(" ") } @@ -224,14 +190,17 @@ inventory::submit! { /// Beamte states a property; a severity is a policy about a repository, and /// straitjacket is the one holding the policy. /// +/// A rule defending no test property makes no claim this mapping is about, so +/// it takes the milder of the two rather than a guess. +/// /// A test that does not fail when the code is broken is the failure that /// costs something -- the suite reports green over a bug. The other two make /// a suite harder to trust and to read, which is worth saying and not worth /// failing a build over on its own. -fn severity_of(property: Property) -> Severity { +fn severity_of(property: Option) -> Severity { match property { - Property::Fidelity => Severity::Error, - Property::Resilience | Property::Precision => Severity::Warning, + Some(Property::Fidelity) => Severity::Error, + Some(Property::Resilience | Property::Precision) | None => Severity::Warning, } } @@ -259,7 +228,7 @@ impl FileRule for TestQualityRule { return; }; - let pack = match pack(entry.pack) { + let pack = match crate::pack::cached(entry.pack) { Ok(pack) => pack, Err(reason) => { candidates.push(not_read( @@ -354,7 +323,7 @@ fn message_of(finding: &beamte::Finding) -> String { /// and makes the rule set auditable rather than one person's taste. fn help_of(finding: &beamte::Finding) -> Option { let citation = beamte::rule(finding.rule.as_str()).map(|rule| rule.citation); - match (&finding.help, citation) { + match (&finding.help, citation.flatten()) { (Some(help), Some(citation)) => { Some(format!("{help} — {} ({})", citation.title, citation.url)) } @@ -426,13 +395,18 @@ mod tests { #[test] fn a_broken_suite_is_an_error_and_a_muddled_one_is_a_warning() { assert_eq!( - severity_of(Property::Fidelity), + severity_of(Some(Property::Fidelity)), crate::finding::Severity::Error ); assert_eq!( - severity_of(Property::Precision), + severity_of(Some(Property::Precision)), crate::finding::Severity::Warning ); + assert_eq!( + severity_of(None), + crate::finding::Severity::Warning, + "a rule defending no test property makes no claim this maps" + ); } #[test] diff --git a/src/scanner.rs b/src/scanner.rs index 4960c52..0e607fb 100644 --- a/src/scanner.rs +++ b/src/scanner.rs @@ -33,6 +33,13 @@ struct RegisteredRule { enabled: bool, } +/// Whether one rule survived `only`/`skip` and is going to run. +fn rules_enabled(rules: &[RegisteredRule], key: crate::rules::RuleKey) -> bool { + rules + .iter() + .any(|registered| registered.enabled && registered.rule.descriptor().id == key) +} + pub struct Scanner { rules: Vec, include_json: bool, @@ -45,13 +52,14 @@ impl Scanner { let builtins = rules::builtins(settings)?; let only: HashSet<_> = rules::resolve(&settings.only)?.into_iter().collect(); let skip: HashSet<_> = rules::resolve(&settings.skip)?.into_iter().collect(); - let rules = builtins + let rules: Vec = builtins .into_iter() .map(|rule| { let descriptor = rule.descriptor(); let mut enabled = if only.is_empty() { descriptor.default_enabled || (descriptor.id == rules::NO_COMMENTS && settings.no_comments) + || (descriptor.id == rules::STRAY_CONST && settings.stray_const) || (descriptor.id == rules::TEST_QUALITY && settings.test_quality) } else { only.contains(&descriptor.id) @@ -61,6 +69,8 @@ impl Scanner { }) .collect(); + rules::check_const_files(rules_enabled(&rules, rules::STRAY_CONST), settings)?; + Ok(Self { rules, include_json: settings.include_json, diff --git a/tests/stray_const.rs b/tests/stray_const.rs new file mode 100644 index 0000000..b4eb2cb --- /dev/null +++ b/tests/stray_const.rs @@ -0,0 +1,219 @@ +//! `stray-const` against the languages treebank publishes a grammar for. +//! +//! The point of this file is breadth, as `tests/test_quality.rs` is for its +//! rule: a constant declared the way each language declares one. beamte's +//! analysis reads the node vocabulary rather than any language's declaration +//! syntax, so a language going quiet here is a grammar problem rather than a +//! missing table — which is the difference between this rule and the regex it +//! replaced. +//! +//! These fetch real packs, for the reason `tests/pack_host.rs` gives at +//! length: a test that passes because it found no grammar is worse than no +//! test. + +use straitjacket::config::Settings; +use straitjacket::finding::Severity; +use straitjacket::scanner::Scanner; + +/// A scanner with `stray-const` on and one designated home, the way a +/// configuration turns it on. +fn scanner(const_files: Vec) -> Scanner { + let settings = Settings { + stray_const: true, + const_files, + ..Settings::default() + }; + Scanner::new(&settings).expect("the scanner builds") +} + +fn findings(path: &str, source: &str) -> Vec { + let extension = path.rsplit('.').next().unwrap_or(""); + scanner(vec!["src/consts.rs".into()]) + .scan(source, path, extension) + .findings + .into_iter() + .map(|finding| finding.message) + .collect() +} + +/// One constant declaration per language, written the way that language +/// writes one, and the name the finding must carry. +const CASES: &[(&str, &str, &str, &str)] = &[ + ("rust", "src/a.rs", "const MAX_SIZE: u8 = 3;\n", "MAX_SIZE"), + ( + "rust-static", + "src/b.rs", + "static DEFAULT_PATH: &str = \"/tmp\";\n", + "DEFAULT_PATH", + ), + ("python", "src/a.py", "MAX_SIZE = 3\n", "MAX_SIZE"), + ("ruby", "lib/a.rb", "MAX_SIZE = 3\n", "MAX_SIZE"), + ( + "typescript", + "src/a.ts", + "const MAX_SIZE: number = 3;\n", + "MAX_SIZE", + ), + ( + "javascript", + "src/a.js", + "const MAX_SIZE = 3;\n", + "MAX_SIZE", + ), + ( + "java", + "src/A.java", + "class A {\n static final int MAX_SIZE = 3;\n}\n", + "MAX_SIZE", + ), + ("zig", "src/a.zig", "const MAX_SIZE = 3;\n", "MAX_SIZE"), +]; + +#[test] +fn every_language_reports_its_constant_declaration() { + for (language, path, source, name) in CASES { + let found = findings(path, source); + assert!( + found.iter().any(|message| message.contains(name)), + "{language}: expected a finding naming {name} in {path}, got {found:?}" + ); + } +} + +#[test] +fn a_use_is_not_a_declaration() { + let source = "fn f(n: u8) -> bool {\n n > MAX_SIZE && n < OTHER_LIMIT\n}\n"; + + assert_eq!( + findings("src/main.rs", source), + Vec::::new(), + "flagging uses would make the rule impossible to satisfy" + ); +} + +#[test] +fn an_import_binds_a_name_without_declaring_it() { + assert_eq!( + findings("src/loader.py", "from settings import MAX_SIZE\n"), + Vec::::new() + ); +} + +#[test] +fn a_local_inside_a_body_is_nobodys_to_gather() { + assert_eq!( + findings( + "src/loader.py", + "def f():\n LOCAL_MAX = 4\n return LOCAL_MAX\n" + ), + Vec::::new() + ); +} + +#[test] +fn a_single_word_name_is_too_ambiguous_to_flag() { + assert_eq!( + findings("src/a.rs", "const MAX: u8 = 3;\nconst PI: f64 = 3.0;\n"), + Vec::::new() + ); +} + +#[test] +fn a_declaration_that_is_not_code_is_not_a_declaration() { + assert_eq!( + findings("src/main.rs", "// const MAX_SIZE: u8 = 3;\nfn f() {}\n"), + Vec::::new(), + "commented out" + ); + assert_eq!( + findings( + "src/main.rs", + "fn f() {\n let s = \"const MAX_SIZE = 3\";\n}\n" + ), + Vec::::new(), + "quoted in a string" + ); +} + +#[test] +fn a_declaration_is_an_error_and_says_where_it_belongs() { + let result = scanner(vec!["src/consts.rs".into()]).scan( + "const MAX_SIZE: u8 = 3;\n", + "src/main.rs", + "rs", + ); + + assert_eq!(result.findings.len(), 1); + assert_eq!(result.findings[0].severity, Severity::Error); + let help = result.findings[0].help.as_deref().unwrap_or_default(); + assert!( + help.contains("src/consts.rs"), + "the help should name the designated file, got: {help}" + ); +} + +#[test] +fn the_designated_file_may_declare_and_everything_else_may_not() { + let scanner = scanner(vec!["src/consts.rs".into()]); + let source = "const MAX_SIZE: u8 = 3;\n"; + + assert_eq!( + scanner.scan(source, "src/consts.rs", "rs").findings, + Vec::new(), + "the designated file is where constants are supposed to be" + ); + assert_eq!(scanner.scan(source, "src/main.rs", "rs").findings.len(), 1); +} + +#[test] +fn data_and_prose_files_are_not_this_rules_to_read() { + assert_eq!( + findings("config/a.yaml", "MAX_SIZE: 3\n"), + Vec::::new() + ); + assert_eq!( + findings("docs/a.md", "MAX_SIZE = 3\n"), + Vec::::new() + ); +} + +#[test] +fn turning_the_rule_on_with_nowhere_to_put_a_constant_is_refused() { + let settings = Settings { + stray_const: true, + ..Settings::default() + }; + + let error = match Scanner::new(&settings) { + Ok(_) => panic!("a rule with no designated file should be refused"), + Err(error) => error.to_string(), + }; + assert!( + error.contains("const-files"), + "the error should name the key that fixes it: {error}" + ); +} + +#[test] +fn the_rule_is_off_until_a_configuration_asks_for_it() { + let quiet = Scanner::new(&Settings::default()) + .expect("the default scanner builds") + .scan("const MAX_SIZE: u8 = 3;\n", "src/main.rs", "rs"); + + assert_eq!( + quiet.findings, + Vec::new(), + "an opt-in rule must stay silent until it is opted into" + ); +} + +/// The analysis is beamte's, and the split is the point: beamte reports every +/// declaration, straitjacket decides which are licensed. +#[test] +fn the_analysis_belongs_to_beamte() { + let rule = beamte::rule("const-declaration").expect("beamte carries the rule"); + + assert_eq!(rule.scope, beamte::Scope::File); + assert_eq!(rule.property, None); + assert!(rule.citation.is_none()); +}