From d5e0a022c97e96de96b9cdb1edd787dba6ea858c Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 16:11:13 +0000 Subject: [PATCH 1/3] stray-const: the constant declared where it happens to be used 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 say which are still true, and the same one gets made twice under two names. const-files names where they live and every declaration outside is an error. It is theme-files for constants. Deliberately not a parser. A rule that has to tell one expression from another needs a tree, which is why test-quality fetches a grammar. This one asks whether a line *declares* a screaming-snake name, and declaration syntax answers that alone -- so it covers all eighteen languages straitjacket calls structured code rather than the nine with a pack, and never reaches the network. It is opt-in only because designating the files is a decision no default can make, and enabling it with no const-files is refused rather than obeyed: every constant would be a finding with nowhere to move it. Three decisions that keep it a rule rather than a nuisance. Declarations, not uses -- referencing a constant is the point of having one, and flagging that would make the rule unsatisfiable. At least two words joined by an underscore, because PI, OK, HTTP, a Go export and a C header guard are all single all-caps words and flagging them would bury the constants among them. And a bare NAME = value counts as a declaration only where the language spells it that way -- in Python, Ruby and Shell only at the left margin, which is what keeps every enum member and every shell local from being a finding. To read code rather than text, rules/comments.rs now yields the comments and a code-only view from one traversal. Masking preserves byte offsets, so a column in the masked line is the column in the real one, and a declaration commented out or quoted inside a string is not one. One lexer answers where the code stops and the prose starts; answering it in two places is how the two answers come to disagree. Run against this repository it reports twenty constants across Rust, TypeScript and shell -- every one a real declaration, with imports, uses and indented shell locals correctly left alone. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01WrSzGnURZoupdEfVdwk9pg --- CHANGELOG.md | 28 ++ README.md | 7 +- site/content/getting-started.md | 2 +- site/content/index.md | 2 +- site/content/reference/config-file.md | 2 + site/content/reference/rules.md | 65 ++++ site/content/rules.json | 5 + src/config.rs | 17 + src/main.rs | 4 + src/rules/comments.rs | 402 ++++++++++++++++-------- src/rules/mod.rs | 19 ++ src/rules/stray_const.rs | 434 ++++++++++++++++++++++++++ src/scanner.rs | 12 +- tests/stray_const.rs | 234 ++++++++++++++ 14 files changed, 1106 insertions(+), 127 deletions(-) create mode 100644 src/rules/stray_const.rs create mode 100644 tests/stray_const.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index d3bb08b..de3d93b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,34 @@ 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 rule needs no grammar, so unlike `test-quality` it covers + all eighteen languages straitjacket calls structured code and never reaches + the network. It reports declarations rather than uses -- referencing a + constant is the point of having one -- and it reads code rather than text, + so a declaration commented out or quoted inside a string is not one. A bare + `NAME = value` counts as a declaration only in the languages that spell it + that way, and in Python, Ruby and Shell only at the left margin, which is + what keeps every `enum` member from being a finding. + +### Changed + +- `rules::comments` now yields the file's comments and a code-only view from + one traversal (`comments::code`), rather than only the comments. Masking + preserves byte offsets, so a column found in the masked line is the column + in the real one. One lexer answers where the code stops and the prose + starts; answering it in two places is how the two answers come to disagree. + ## [0.2.0] - 2026-08-30 ### Added diff --git a/README.md b/README.md index 2059a3d..8112aa1 100644 --- a/README.md +++ b/README.md @@ -27,8 +27,11 @@ 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; 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..300c392 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. Needs no grammar, so it covers every structured language. 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,67 @@ 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. + +**No grammar, so every structured language.** Unlike +[`test-quality`](#test-quality), this rule reads declaration syntax rather +than a parse tree, which means all eighteen languages straitjacket calls +structured code, no download, and no network. It is opt-in only because designating the files is a decision no +default can make. + +It reads code, not text: a declaration that is commented out or quoted inside +a string is not a declaration, and those are the two places source most often +appears without being code. + +### 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. + +What declares one depends on the language: + +| shape | looks like | languages | +|---|---|---| +| a keyword | `const MAX_SIZE`, `static final int MAX_SIZE`, `const val MAX_SIZE` | Rust, C, C++, C#, Java, Kotlin, JS, TS, Swift, Scala, PHP, Zig, Go | +| a preprocessor directive | `#define MAX_RETRIES 5` | C, C++ | +| a bare assignment at the left margin | `MAX_SIZE = 3`, `MAX_SIZE=3` | Python, Ruby, Shell | +| a bare assignment at any depth | `const (` … `MAX_SIZE = 100` … `)` | Go | + +The last two rows are why an **enum member is not a finding**: indented +`RED_ONE = 1` inside a Python `class Colour(Enum)` or a C `enum` body is not a +constant anyone could move to another file, so only the left margin counts in +the languages that declare by assignment. + +Two misses are worth naming. A constant whose name is built at run time is +invisible, as is PHP's `define('MAX_SIZE', 3)`, where the name lives inside a +string this rule has deliberately blanked. 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/rules/comments.rs b/src/rules/comments.rs index 3880f0a..c1fc47d 100644 --- a/src/rules/comments.rs +++ b/src/rules/comments.rs @@ -75,164 +75,322 @@ struct OpenBlock { parts: Vec, } +/// A comment that begins and ends on one line. +/// +/// Three of the four places a comment is recorded build exactly this, from +/// three levels inside the traversal. Naming it keeps those sites flat enough +/// to read. +fn one_line(line: usize, col: usize, text: String) -> Comment { + Comment { + parts: vec![CommentPart { line, col, text }], + } +} + +/// The comments in a file. pub fn scan(text: &str, language: &LanguageProfile) -> Vec { + walk(text, language).comments +} + +/// The file with every comment and string literal blanked to spaces, one +/// entry per line. +/// +/// Byte offsets and line lengths are preserved, so a column found in a masked +/// line is the column in the real one. This is what lets a rule match a +/// declaration without also matching it commented out or quoted inside a +/// string -- the two places source code most often appears without being +/// code. A language with no comment syntax comes back unmasked, having +/// nothing this can recognise. +pub fn code(text: &str, language: &LanguageProfile) -> Vec { + walk(text, language).code +} + +/// Both views, from one traversal. +/// +/// They are produced together because they answer one question -- where the +/// code stops and the prose starts -- and answering it in two places is how +/// the two answers come to disagree. +struct Walk { + comments: Vec, + code: Vec, +} + +/// An unterminated single-quote string is taken to run to the end of its +/// line. The traversal does not carry one to the next line, and the mask does +/// not either, so the two always agree about where such a string stops. +fn walk(text: &str, language: &LanguageProfile) -> Walk { let Some(syntax) = language.comments else { - return Vec::new(); + return Walk { + comments: Vec::new(), + code: text.lines().map(str::to_owned).collect(), + }; }; let mut comments = Vec::new(); + let mut code = Vec::new(); let mut open_block: Option = None; let mut open_string: Option = None; for (line_index, line) in text.lines().enumerate() { let line_number = line_index + 1; - if line_number == 1 && line.starts_with("#!") && syntax.line.contains(&"#") { - continue; - } + let mut holes: Vec<(usize, usize)> = Vec::new(); - let mut cursor = 0; - if let Some(block) = open_block.as_mut() { - match line.find(block.end) { - Some(position) => { - let end = position + block.end.len(); - block.parts.push(CommentPart { - line: line_number, - col: 1, - text: line[..end].to_owned(), - }); - let block = open_block.take().expect("open block exists"); - comments.push(Comment { parts: block.parts }); - cursor = end; - } - None => { - block.parts.push(CommentPart { - line: line_number, - col: 1, - text: line.to_owned(), - }); - continue; - } + 'line_done: { + if line_number == 1 && line.starts_with("#!") && syntax.line.contains(&"#") { + break 'line_done; } - } else if let Some(delimiter) = open_string.as_deref() { - match line.find(delimiter) { - Some(position) => { - cursor = position + delimiter.len(); - open_string = None; + + let mut cursor = 0; + if let Some(block) = open_block.as_mut() { + match line.find(block.end) { + Some(position) => { + let end = position + block.end.len(); + block.parts.push(CommentPart { + line: line_number, + col: 1, + text: line[..end].to_owned(), + }); + let block = open_block.take().expect("open block exists"); + comments.push(Comment { parts: block.parts }); + holes.push((0, end)); + cursor = end; + } + None => { + block.parts.push(CommentPart { + line: line_number, + col: 1, + text: line.to_owned(), + }); + holes.push((0, line.len())); + break 'line_done; + } + } + } else if let Some(delimiter) = open_string.as_deref() { + match line.find(delimiter) { + Some(position) => { + cursor = position + delimiter.len(); + holes.push((0, cursor)); + open_string = None; + } + None => { + holes.push((0, line.len())); + break 'line_done; + } } - None => continue, } - } - let mut previous = line[..cursor].chars().next_back(); - let mut quote = None; - 'line: while cursor < line.len() { - let rest = &line[cursor..]; - let character = rest.chars().next().expect("cursor is a char boundary"); + let mut previous = line[..cursor].chars().next_back(); + let mut quote = None; + let mut quote_start = 0; + 'line: while cursor < line.len() { + let rest = &line[cursor..]; + let character = rest.chars().next().expect("cursor is a char boundary"); - if let Some(delimiter) = quote { - if character == '\\' { + if let Some(delimiter) = quote { + if character == '\\' { + cursor += character.len_utf8(); + if let Some(escaped) = line[cursor..].chars().next() { + cursor += escaped.len_utf8(); + } + continue; + } cursor += character.len_utf8(); - if let Some(escaped) = line[cursor..].chars().next() { - cursor += escaped.len_utf8(); + if character == delimiter { + quote = None; + holes.push((quote_start, cursor)); } continue; } - if character == delimiter { - quote = None; - } - cursor += character.len_utf8(); - continue; - } - if language.id == "rust" - && let Some((opening_len, delimiter)) = rust_raw_string(rest) - { - let after = cursor + opening_len; - match line[after..].find(&delimiter) { - Some(position) => cursor = after + position + delimiter.len(), - None => { - open_string = Some(delimiter); - break 'line; + if language.id == "rust" + && let Some((opening_len, delimiter)) = rust_raw_string(rest) + { + let after = cursor + opening_len; + match line[after..].find(&delimiter) { + Some(position) => { + let end = after + position + delimiter.len(); + holes.push((cursor, end)); + cursor = end; + } + None => { + open_string = Some(delimiter); + holes.push((cursor, line.len())); + break 'line; + } } + previous = line[..cursor].chars().next_back(); + continue; } - previous = line[..cursor].chars().next_back(); - continue; - } - if let Some(delimiter) = syntax - .multi_quotes - .iter() - .find(|delimiter| rest.starts_with(**delimiter)) - { - let after = cursor + delimiter.len(); - match line[after..].find(delimiter) { - Some(position) => cursor = after + position + delimiter.len(), - None => { - open_string = Some((*delimiter).to_owned()); - break 'line; + if let Some(delimiter) = syntax + .multi_quotes + .iter() + .find(|delimiter| rest.starts_with(**delimiter)) + { + let after = cursor + delimiter.len(); + match line[after..].find(delimiter) { + Some(position) => { + let end = after + position + delimiter.len(); + holes.push((cursor, end)); + cursor = end; + } + None => { + open_string = Some((*delimiter).to_owned()); + holes.push((cursor, line.len())); + break 'line; + } } + previous = delimiter.chars().next_back(); + continue; } - previous = delimiter.chars().next_back(); - continue; - } - if syntax.quotes.contains(&character) { - quote = Some(character); - cursor += character.len_utf8(); - continue; - } + if syntax.quotes.contains(&character) { + quote = Some(character); + quote_start = cursor; + cursor += character.len_utf8(); + continue; + } - if let Some((open, close)) = - syntax.block.iter().find(|(open, _)| rest.starts_with(open)) - { - let after = cursor + open.len(); - match line[after..].find(close) { - Some(position) => { - let end = after + position + close.len(); - comments.push(Comment { - parts: vec![CommentPart { - line: line_number, - col: cursor + 1, - text: line[cursor..end].to_owned(), - }], - }); - cursor = end; - previous = close.chars().next_back(); - continue; - } - None => { - open_block = Some(OpenBlock { - end: close, - parts: vec![CommentPart { - line: line_number, - col: cursor + 1, - text: rest.to_owned(), - }], - }); - break 'line; + if let Some((open, close)) = + syntax.block.iter().find(|(open, _)| rest.starts_with(open)) + { + let after = cursor + open.len(); + match line[after..].find(close) { + Some(position) => { + let end = after + position + close.len(); + comments.push(one_line( + line_number, + cursor + 1, + line[cursor..end].to_owned(), + )); + holes.push((cursor, end)); + cursor = end; + previous = close.chars().next_back(); + continue; + } + None => { + let opening = one_line(line_number, cursor + 1, rest.to_owned()); + open_block = Some(OpenBlock { + end: close, + parts: opening.parts, + }); + holes.push((cursor, line.len())); + break 'line; + } } } - } - if let Some(marker) = syntax.line.iter().find(|marker| rest.starts_with(**marker)) - && boundary_ok(marker, previous) - { - comments.push(Comment { - parts: vec![CommentPart { - line: line_number, - col: cursor + 1, - text: rest.to_owned(), - }], - }); - break 'line; + if let Some(marker) = syntax.line.iter().find(|marker| rest.starts_with(**marker)) + && boundary_ok(marker, previous) + { + comments.push(one_line(line_number, cursor + 1, rest.to_owned())); + holes.push((cursor, line.len())); + break 'line; + } + + previous = Some(character); + cursor += character.len_utf8(); } - previous = Some(character); - cursor += character.len_utf8(); + if quote.is_some() { + holes.push((quote_start, line.len())); + } } + + code.push(mask(line, &holes)); } if let Some(block) = open_block { comments.push(Comment { parts: block.parts }); } - comments + Walk { comments, code } +} + +/// The line with the given byte ranges replaced by spaces. +/// +/// Ranges always cover whole characters, so replacing their bytes with ASCII +/// spaces leaves valid UTF-8 and leaves every column where it was. +fn mask(line: &str, holes: &[(usize, usize)]) -> String { + if holes.is_empty() { + return line.to_owned(); + } + let mut bytes = line.as_bytes().to_vec(); + for &(from, to) in holes { + let from = from.min(bytes.len()); + let to = to.min(bytes.len()); + for byte in &mut bytes[from..to.max(from)] { + *byte = b' '; + } + } + String::from_utf8(bytes).expect("blanking whole characters leaves valid UTF-8") +} + +/// The masked view keeps the shape of the line and drops only what is not +/// code. Every case here is one a rule reading `code` would otherwise get +/// wrong: a declaration commented out, quoted, or spanning a block. +#[cfg(test)] +mod tests { + use super::code; + use crate::language::language_profile; + + fn masked(source: &str, language: &str) -> Vec { + code( + source, + language_profile(language).expect("a language straitjacket knows"), + ) + } + + #[test] + fn code_survives_and_comments_do_not() { + let source = "const MAX_SIZE: u8 = 3; // MAX_OTHER"; + let lines = masked(&format!("{source}\n"), "rust"); + + assert!(lines[0].starts_with("const MAX_SIZE: u8 = 3;")); + assert!(!lines[0].contains("MAX_OTHER")); + assert_eq!(lines[0].len(), source.len(), "columns must not move"); + } + + #[test] + fn a_string_literal_is_blanked_and_its_columns_are_kept() { + let source = "let s = \"const MAX_SIZE = 3\";"; + let lines = masked(&format!("{source}\n"), "rust"); + + assert!(lines[0].starts_with("let s = ")); + assert!(!lines[0].contains("MAX_SIZE")); + assert!(lines[0].ends_with(';'), "the code after a string survives"); + assert_eq!(lines[0].len(), source.len(), "columns must not move"); + } + + #[test] + fn a_block_comment_is_blanked_on_every_line_it_spans() { + let lines = masked("a\n/* const MAX_SIZE = 1\n still inside */ b\n", "rust"); + + assert_eq!(lines[0], "a"); + assert_eq!(lines[1].trim(), ""); + assert_eq!(lines[2].trim(), "b"); + } + + #[test] + fn a_rust_raw_string_is_blanked_hashes_and_all() { + let source = "let s = r#\"const MAX_SIZE = 3\"#;"; + let lines = masked(&format!("{source}\n"), "rust"); + + assert!(lines[0].starts_with("let s = ")); + assert!(!lines[0].contains("MAX_SIZE")); + assert_eq!(lines[0].len(), source.len(), "columns must not move"); + } + + #[test] + fn a_python_docstring_is_blanked_across_its_lines() { + let lines = masked("x = 1\n\"\"\"\nMAX_SIZE = 3\n\"\"\"\ny = 2\n", "python"); + + assert_eq!(lines[0], "x = 1"); + assert_eq!(lines[2].trim(), ""); + assert_eq!(lines[4], "y = 2"); + } + + #[test] + fn a_language_with_no_comment_syntax_is_returned_whole() { + let lines = masked("{\"MAX_SIZE\": 3}\n", "json"); + + assert_eq!(lines, ["{\"MAX_SIZE\": 3}"]); + } } 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..0049756 --- /dev/null +++ b/src/rules/stray_const.rs @@ -0,0 +1,434 @@ +//! `SCREAMING_SNAKE_CASE` constants declared outside the files that hold +//! them. +//! +//! A constant is a decision about the program: a limit, a path, a key, a +//! magic number somebody named. Scattered through the tree, those decisions +//! cannot be read as a set, and the same one gets made twice under two names. +//! Gathering them is not tidiness -- it is what makes the list of decisions +//! reviewable. `const-files` names where they live and everything outside is +//! reported. +//! +//! Deliberately not a parser. A rule that has to tell one expression from +//! another needs a tree, which is why `test-quality` fetches a treebank +//! grammar. This one asks a much narrower question -- does a line *declare* a +//! screaming-snake name -- and declaration syntax answers that on its own. So +//! it runs over all eighteen languages straitjacket calls structured code +//! rather than the nine with a pack, needs no network, and is off by default +//! only because designating the files is a decision no default can make. +//! +//! It reads [`comments::code`], not the raw text: a declaration commented out +//! or quoted inside a string is not a declaration, and those are the two +//! places source code most often appears without being code. +//! +//! **Declarations, not uses.** `MAX_SIZE` mentioned in an expression is the +//! point of having a constant, and flagging it would make the rule +//! unsatisfiable. Only the site that introduces the name is a finding. + +use std::path::{Path, PathBuf}; + +use regex::Regex; + +use crate::Settings; +use crate::finding::{Finding, Location, Severity}; +use crate::language::{LanguageProfile, STRUCTURED_CODE}; +use crate::rule::{Candidate, FileRule, RuleDescriptor, RuleKey, SourceFile}; +use crate::rules::RuleRegistration; +use crate::rules::comments; + +pub const KEY: RuleKey = RuleKey::new("stray-const"); + +/// Off unless a configuration asks for it. +/// +/// Unlike every default-on rule, this one has nothing to say until somebody +/// says where constants belong. A default would be a guess about a layout +/// straitjacket cannot see. +const DEFAULT_ENABLED: bool = false; + +/// A screaming-snake name: at least two words, joined by underscores. +/// +/// The underscore is required, which is the whole difference between a rule +/// and a nuisance. A single all-caps word is ambiguous in every language that +/// has one -- `PI`, `OK`, `HTTP`, a Go export, a C macro guard, a type +/// parameter -- and flagging those would bury the constants among them. +const NAME: &str = r"[A-Z][A-Z0-9]*(?:_[A-Z0-9]+)+"; + +/// Whether a bare `NAME = value` declares a constant in this language. +/// +/// In the C family and its descendants a declaration carries a keyword, so a +/// bare assignment is either a write to something already declared or an enum +/// member -- neither of which is a constant anyone can move. Reading it as a +/// declaration there is how this rule would come to flag every `enum` body in +/// the repository. +#[derive(Clone, Copy, PartialEq, Eq)] +enum Bare { + /// Never; the language spells declarations with a keyword. + No, + /// Only at the left margin, which is where a module-level constant sits. + /// Indented, the same line is a class attribute, an enum member or a + /// local, and none of those belongs in another file. + TopLevel, + /// At any indentation: Go writes its constants inside a `const (` block. + AnyIndent, +} + +fn bare_form(language: &LanguageProfile) -> Bare { + match language.id { + "python" | "ruby" | "shell" => Bare::TopLevel, + "go" => Bare::AnyIndent, + _ => Bare::No, + } +} + +pub struct StrayConstRule { + /// The designated homes. A declaration inside one is what the rule is + /// asking for, so it is not reported. + allow: Vec, + /// `const NAME`, `static final int NAME`, `let NAME` -- a declaration + /// keyword, an optional type, then the name. + /// + /// The group between the keyword and the name is that type: + /// `static final int MAX_SIZE`, `const char *MAX_NAME`. Where the keyword + /// sits directly against the name it matches nothing, and where a second + /// keyword intervenes the scan simply starts again at that keyword -- + /// which is how `static final int` finds its name on the third attempt + /// rather than needing a rule of its own. + keyword: Regex, + /// `#define NAME`. + define: Regex, + /// `NAME = value` at the left margin. + bare_top: Regex, + /// `NAME = value` at any indentation. + bare_any: Regex, + /// A screaming-snake name anywhere at all. + /// + /// The prefilter, and deliberately not one of the patterns above: those + /// are anchored to a line, so asking one of them about a whole file + /// answers for its first line only. That is how an indented Go `const (` + /// block came to be skipped, caught by the test that covers Go. A bare + /// name is both cheaper to look for and free of anchors. + name: Regex, +} + +impl StrayConstRule { + pub fn new(allow: Vec) -> Self { + let compile = + |pattern: String| Regex::new(&pattern).expect("built-in rule patterns must compile"); + Self { + allow, + keyword: compile(format!( + r"\b(?:const|constexpr|static|final|readonly|let|var|val)\s+(?:mut\s+)?(?:[A-Za-z_][A-Za-z0-9_:<>\[\].]*\s+)?(?:[*&]+\s*)?({NAME})\b" + )), + define: compile(format!(r"^\s*#\s*define\s+({NAME})\b")), + bare_top: compile(format!( + r"^(?:export\s+|readonly\s+)?({NAME})\s*(?::[^=]*)?=" + )), + bare_any: compile(format!( + r"^\s*(?:export\s+|readonly\s+)?({NAME})\s*(?::[^=]*)?=" + )), + name: compile(NAME.to_owned()), + } + } + + fn designated(&self, path: &str) -> bool { + let 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) + }) + } + + /// Every declaration on one line, as (byte offset, name). + /// + /// The line is already masked, so anything found here is code. + fn declarations(&self, line: &str, bare: Bare) -> Vec<(usize, String)> { + let mut found: Vec<(usize, String)> = Vec::new(); + let mut take = |regex: &Regex, assignment: bool| { + for captures in regex.captures_iter(line) { + let Some(name) = captures.get(1) else { + continue; + }; + if assignment && !assigns(line, captures.get(0).map_or(0, |whole| whole.end())) { + continue; + } + if found.iter().any(|(at, _)| *at == name.start()) { + continue; + } + found.push((name.start(), name.as_str().to_owned())); + } + }; + + take(&self.keyword, false); + take(&self.define, false); + match bare { + Bare::No => {} + Bare::TopLevel => take(&self.bare_top, true), + Bare::AnyIndent => take(&self.bare_any, true), + } + + found.sort_by_key(|(at, _)| *at); + found + } +} + +/// Whether the `=` a bare pattern stopped on is really an assignment. +/// +/// The regex crate has no lookahead, so the character after the match is +/// checked here instead. `==` is a comparison, `=>` a hash rocket or a match +/// arm, and `=~` a Ruby match -- three ways to write a screaming-snake name +/// beside an equals sign without declaring anything. +fn assigns(line: &str, end: usize) -> bool { + !matches!(line.as_bytes().get(end), Some(b'=' | b'>' | b'~')) +} + +fn build(settings: &Settings) -> Box { + Box::new(StrayConstRule::new(settings.const_files.clone())) +} + +fn instruction(settings: &Settings) -> String { + if settings.const_files.is_empty() { + return "Declare SCREAMING_SNAKE_CASE constants in the designated constant \ + files named by `const-files` in straitjacket.toml, and reference them \ + from everywhere else." + .to_string(); + } + let designated = settings + .const_files + .iter() + .map(|path| path.display().to_string()) + .collect::>() + .join(", "); + format!( + "SCREAMING_SNAKE_CASE constants are declared in {designated} and nowhere else. \ + Reference them from there rather than declaring one where it is used." + ) +} + +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 { + language.has_facet(&STRUCTURED_CODE) + } + + fn check(&self, file: SourceFile<'_>, candidates: &mut Vec) { + if self.designated(file.path) { + return; + } + if !self.name.is_match(file.text) { + return; + } + + let bare = bare_form(file.language); + for (line_index, line) in comments::code(file.text, file.language).iter().enumerate() { + for (offset, name) in self.declarations(line, bare) { + let mut finding = Finding::new( + KEY, + Severity::Error, + Location::point(file.path, line_index + 1, offset + 1), + name.clone(), + format!("`{name}` is declared here rather than in a constant file"), + ); + finding.help = Some(self.hint()); + candidates.push(Candidate::line(finding)); + } + } + } +} + +impl StrayConstRule { + 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(), + } + } +} + +#[cfg(test)] +mod tests { + use super::StrayConstRule; + use crate::language::language_profile; + use crate::rule::{Candidate, FileRule, SourceFile}; + + fn findings(source: &str, language: &str) -> Vec<(usize, usize, String)> { + at(source, language, "src/thing.x") + } + + fn at(source: &str, language: &str, path: &str) -> Vec<(usize, usize, String)> { + rule(Vec::new(), source, language, path) + } + + fn rule( + allow: Vec, + source: &str, + language: &str, + path: &str, + ) -> Vec<(usize, usize, String)> { + let mut candidates: Vec = Vec::new(); + StrayConstRule::new(allow).check( + SourceFile { + path, + language: language_profile(language).expect("a language straitjacket knows"), + text: source, + }, + &mut candidates, + ); + candidates + .into_iter() + .map(|candidate| { + ( + candidate.finding.location.line, + candidate.finding.location.col, + candidate.finding.matched, + ) + }) + .collect() + } + + #[test] + fn flags_a_keyword_declaration_and_points_at_the_name() { + let hits = findings("const MAX_SIZE: u8 = 3;\n", "rust"); + + assert_eq!(hits.len(), 1); + assert_eq!(hits[0].0, 1); + assert_eq!(hits[0].1, 7); + assert_eq!(hits[0].2, "MAX_SIZE"); + } + + #[test] + fn flags_a_declaration_behind_a_type_and_a_pointer() { + assert_eq!( + findings("static final int MAX_SIZE = 3;\n", "java")[0].2, + "MAX_SIZE" + ); + assert_eq!( + findings("static const char *DEFAULT_NAME = \"x\";\n", "c")[0].2, + "DEFAULT_NAME" + ); + assert_eq!(findings("#define MAX_RETRIES 5\n", "c")[0].2, "MAX_RETRIES"); + } + + #[test] + fn a_use_is_not_a_declaration() { + assert!(findings("if size > MAX_SIZE { return; }\n", "rust").is_empty()); + assert!(findings("foo(MAX_SIZE, OTHER_THING);\n", "rust").is_empty()); + } + + #[test] + fn a_single_word_name_is_left_alone() { + assert!(findings("const MAX: u8 = 3;\n", "rust").is_empty()); + assert!(findings("const PI: f64 = 3.14;\n", "rust").is_empty()); + } + + #[test] + fn a_commented_out_or_quoted_declaration_is_not_one() { + assert!(findings("// const MAX_SIZE: u8 = 3;\n", "rust").is_empty()); + assert!(findings("let s = \"const MAX_SIZE = 3\";\n", "rust").is_empty()); + assert!(findings("/* const MAX_SIZE = 3 */\n", "rust").is_empty()); + } + + /// Rust spells a declaration with a keyword, so a bare `MAX_SIZE = 3` + /// there is a write to something declared elsewhere rather than a + /// declaration this rule could ask anyone to move. + #[test] + fn a_bare_assignment_declares_only_where_the_language_says_so() { + assert_eq!(findings("MAX_SIZE = 3\n", "python")[0].2, "MAX_SIZE"); + assert_eq!(findings("MAX_SIZE = 3\n", "ruby")[0].2, "MAX_SIZE"); + assert_eq!(findings("MAX_SIZE=3\n", "shell")[0].2, "MAX_SIZE"); + assert!(findings("MAX_SIZE = 3;\n", "rust").is_empty()); + } + + #[test] + fn an_indented_assignment_is_a_member_rather_than_a_constant() { + let source = "class Colour(Enum):\n RED_ONE = 1\n GREEN_TWO = 2\n"; + + assert!( + findings(source, "python").is_empty(), + "an enum member cannot be moved to another file" + ); + } + + #[test] + fn go_declares_inside_an_indented_const_block() { + let hits = findings("const (\n\tMAX_SIZE = 100\n)\n", "go"); + + assert_eq!(hits.len(), 1); + assert_eq!(hits[0].2, "MAX_SIZE"); + } + + #[test] + fn a_comparison_is_not_an_assignment() { + assert!(findings("MAX_SIZE == 3\n", "python").is_empty()); + assert!(findings("MAX_SIZE != 3\n", "python").is_empty()); + assert!(findings("MAX_SIZE >= 3\n", "python").is_empty()); + assert!(findings("MAX_SIZE => 3\n", "ruby").is_empty()); + } + + #[test] + fn a_designated_file_may_declare_freely() { + let source = "const MAX_SIZE: u8 = 3;\n"; + + assert!( + rule( + vec!["src/config.rs".into()], + source, + "rust", + "src/config.rs" + ) + .is_empty() + ); + assert_eq!( + rule(vec!["src/config.rs".into()], source, "rust", "src/main.rs").len(), + 1 + ); + } + + #[test] + fn a_designated_directory_covers_what_is_under_it() { + let source = "const MAX_SIZE: u8 = 3;\n"; + + assert!( + rule( + vec!["src/consts/".into()], + source, + "rust", + "src/consts/a.rs" + ) + .is_empty() + ); + } + + #[test] + fn several_declarations_on_one_line_are_reported_once_each() { + let hits = findings("const A_ONE: u8 = 1; const B_TWO: u8 = 2;\n", "rust"); + + assert_eq!(hits.len(), 2); + assert_eq!(hits[0].2, "A_ONE"); + assert_eq!(hits[1].2, "B_TWO"); + } + + #[test] + fn a_python_docstring_full_of_declarations_is_prose() { + let source = "\"\"\"\nMAX_SIZE = 3\n\"\"\"\nx = 1\n"; + + assert!(findings(source, "python").is_empty()); + } +} 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..3a9be79 --- /dev/null +++ b/tests/stray_const.rs @@ -0,0 +1,234 @@ +//! `stray-const` against the languages straitjacket calls structured code. +//! +//! The point of this file is breadth, as `tests/test_quality.rs` and +//! `tests/env_vars.rs` are for their rules -- but where those two reach for a +//! grammar and cover nine languages, this one needs no parser and so has to +//! answer for eighteen. A declaration written the way each language writes +//! one is what keeps a whole language from going quietly silent. +//! +//! Nothing here touches the network: the rule reads declaration syntax, not +//! trees, which is the trade it exists to make. + +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| format!("{}:{}", finding.location.line, finding.matched)) + .collect() +} + +/// One constant declaration per language, written the way that language +/// writes one: the label, the file, the source, and the name the finding must +/// carry. Each `source` declares exactly one constant, so exactly one finding +/// is correct in every row. +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", + ), + ("c", "src/a.c", "#define MAX_RETRIES 5\n", "MAX_RETRIES"), + ( + "cpp", + "src/a.cc", + "constexpr int MAX_BUFFER = 1024;\n", + "MAX_BUFFER", + ), + ( + "c-sharp", + "src/A.cs", + "private const int MAX_ITEMS = 10;\n", + "MAX_ITEMS", + ), + ( + "go", + "src/a.go", + "const (\n\tMAX_SIZE = 100\n)\n", + "MAX_SIZE", + ), + ( + "java", + "src/A.java", + "public static final int MAX_SIZE = 3;\n", + "MAX_SIZE", + ), + ( + "javascript", + "src/a.js", + "const MAX_SIZE = 3;\n", + "MAX_SIZE", + ), + ("kotlin", "src/a.kt", "const val MAX_SIZE = 3\n", "MAX_SIZE"), + ("php", "src/a.php", "const MAX_SIZE = 3;\n", "MAX_SIZE"), + ("python", "src/a.py", "MAX_SIZE = 3\n", "MAX_SIZE"), + ("ruby", "src/a.rb", "MAX_SIZE = 3\n", "MAX_SIZE"), + ("scala", "src/a.scala", "val MAX_SIZE = 3\n", "MAX_SIZE"), + ("shell", "src/a.sh", "MAX_SIZE=3\n", "MAX_SIZE"), + ("swift", "src/a.swift", "let MAX_SIZE = 3\n", "MAX_SIZE"), + ( + "typescript", + "src/a.ts", + "const MAX_SIZE: number = 3;\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_eq!( + found.len(), + 1, + "{language}: expected exactly one finding in {path}, got {found:?}" + ); + assert!( + found[0].ends_with(name), + "{language}: the finding should name {name}, got {found:?}" + ); + } +} + +#[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 using_a_constant_everywhere_is_the_point_of_having_one() { + 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 a_single_word_name_is_too_ambiguous_to_flag() { + assert_eq!( + findings("src/main.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;\n"), + Vec::::new(), + "commented out" + ); + assert_eq!( + findings("src/main.rs", "let s = \"const MAX_SIZE = 3\";\n"), + Vec::::new(), + "quoted in a string" + ); +} + +#[test] +fn an_enum_member_is_not_a_constant_anyone_can_move() { + assert_eq!( + findings( + "src/colour.py", + "class Colour(Enum):\n RED_ONE = 1\n GREEN_TWO = 2\n" + ), + Vec::::new() + ); + assert_eq!( + findings("src/colour.c", "enum Colour {\n RED_ONE = 1,\n};\n"), + Vec::::new() + ); +} + +#[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("data/a.json", "{\"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" + ); +} From 00326d56ac0b85a291ad7173a9c1716078595e63 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 20:21:46 +0000 Subject: [PATCH 2/3] stray-const: ask a parser what a declaration is, not a regex The first cut matched declaration syntax with a per-language table of keyword patterns. That table is a parser written badly: it cannot tell `MAX_SIZE` in `const MAX_SIZE = 3` from `MAX_SIZE` in `n > MAX_SIZE` without knowing each language's declaration grammar, and every language added means another pattern to get subtly wrong. The question is a tree question, so it moves to beamte, which owns what a construct *is*. `const-declaration` asks the node vocabulary instead: a `_binding` that is neither an import nor a parameter, outside any callable, whose bound name is SCREAMING_SNAKE. That needs no language table at all -- only which pack serves which grammar. What is left here is what beamte refuses to do: fetch a grammar, parse, pick a severity, and name the files constants belong in. The last of those is the policy half, and it stays policy: beamte reports every declaration and `const-files` decides which are licensed. Costs the change accepts: ten languages instead of eighteen, and a grammar download on first scan, which is why the rule is off by default twice over. Gains: no false positive on a use, and shell reassignment in `publish.sh` now reads as the declaration it is. `comments.rs` reverts to main -- the masking it grew existed only to keep the regex off commented-out code, and a parse does that for free. `pack::cached` moves out of `test_quality` so both hosts share one cache rather than each keeping a private one. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01WrSzGnURZoupdEfVdwk9pg --- CHANGELOG.md | 51 +-- Cargo.lock | 21 +- Cargo.toml | 10 +- README.md | 7 +- site/content/reference/rules.md | 59 ++-- src/pack.rs | 41 +++ src/rules/comments.rs | 402 ++++++++---------------- src/rules/stray_const.rs | 536 +++++++++++++------------------- src/rules/test_quality.rs | 76 ++--- tests/stray_const.rs | 171 +++++----- 10 files changed, 563 insertions(+), 811 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index de3d93b..948df20 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,30 +9,39 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### 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 rule needs no grammar, so unlike `test-quality` it covers - all eighteen languages straitjacket calls structured code and never reaches - the network. It reports declarations rather than uses -- referencing a - constant is the point of having one -- and it reads code rather than text, - so a declaration commented out or quoted inside a string is not one. A bare - `NAME = value` counts as a declaration only in the languages that spell it - that way, and in Python, Ruby and Shell only at the left margin, which is - what keeps every `enum` member from being a finding. + **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 -- `rules::comments` now yields the file's comments and a code-only view from - one traversal (`comments::code`), rather than only the comments. Masking - preserves byte offsets, so a column found in the masked line is the column - in the real one. One lexer answers where the code stops and the prose - starts; answering it in two places is how the two answers come to disagree. +- 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 diff --git a/Cargo.lock b/Cargo.lock index e3cdf4b..e565ae6 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?branch=claude%2Fsj-const-files-2g6g2t#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..f0970ee 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,11 @@ 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 branch that carries it. Deliberately not +# a branch pin that can vanish: drop this table the moment 0.3 is published, +# since the requirement is already written for the registry. +[patch.crates-io] +beamte = { git = "https://github.com/PowderworksCode/beamte", branch = "claude/sj-const-files-2g6g2t" } diff --git a/README.md b/README.md index 8112aa1..6fb9341 100644 --- a/README.md +++ b/README.md @@ -29,9 +29,10 @@ straitjacket: 1 error(s), 0 warning(s) across 84 file(s); 0 suppressed 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; and -`test-quality`, which reads your tests the way the +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/reference/rules.md b/site/content/reference/rules.md index 300c392..4efdce2 100644 --- a/site/content/reference/rules.md +++ b/site/content/reference/rules.md @@ -33,7 +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. Needs no grammar, so it covers every structured language. See [stray constants](#stray-constants) 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 @@ -241,15 +241,32 @@ a finding with nowhere to move it, which is a configuration nobody means. point of having a constant; only the line that introduces the name is a finding. A rule that flagged uses could not be satisfied. -**No grammar, so every structured language.** Unlike -[`test-quality`](#test-quality), this rule reads declaration syntax rather -than a parse tree, which means all eighteen languages straitjacket calls -structured code, no download, and no network. It is opt-in only because designating the files is a decision no -default can make. +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: -It reads code, not text: a declaration that is commented out or quoted inside -a string is not a declaration, and those are the two places source most often -appears without being code. +| 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 @@ -259,20 +276,12 @@ 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. -What declares one depends on the language: +Ten languages, being the ones treebank publishes a grammar for: Python, Ruby, +Rust, Java, TypeScript, JavaScript, C, C++, Shell and Zig. -| shape | looks like | languages | -|---|---|---| -| a keyword | `const MAX_SIZE`, `static final int MAX_SIZE`, `const val MAX_SIZE` | Rust, C, C++, C#, Java, Kotlin, JS, TS, Swift, Scala, PHP, Zig, Go | -| a preprocessor directive | `#define MAX_RETRIES 5` | C, C++ | -| a bare assignment at the left margin | `MAX_SIZE = 3`, `MAX_SIZE=3` | Python, Ruby, Shell | -| a bare assignment at any depth | `const (` … `MAX_SIZE = 100` … `)` | Go | - -The last two rows are why an **enum member is not a finding**: indented -`RED_ONE = 1` inside a Python `class Colour(Enum)` or a C `enum` body is not a -constant anyone could move to another file, so only the left margin counts in -the languages that declare by assignment. - -Two misses are worth naming. A constant whose name is built at run time is -invisible, as is PHP's `define('MAX_SIZE', 3)`, where the name lives inside a -string this rule has deliberately blanked. +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/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/comments.rs b/src/rules/comments.rs index c1fc47d..3880f0a 100644 --- a/src/rules/comments.rs +++ b/src/rules/comments.rs @@ -75,322 +75,164 @@ struct OpenBlock { parts: Vec, } -/// A comment that begins and ends on one line. -/// -/// Three of the four places a comment is recorded build exactly this, from -/// three levels inside the traversal. Naming it keeps those sites flat enough -/// to read. -fn one_line(line: usize, col: usize, text: String) -> Comment { - Comment { - parts: vec![CommentPart { line, col, text }], - } -} - -/// The comments in a file. pub fn scan(text: &str, language: &LanguageProfile) -> Vec { - walk(text, language).comments -} - -/// The file with every comment and string literal blanked to spaces, one -/// entry per line. -/// -/// Byte offsets and line lengths are preserved, so a column found in a masked -/// line is the column in the real one. This is what lets a rule match a -/// declaration without also matching it commented out or quoted inside a -/// string -- the two places source code most often appears without being -/// code. A language with no comment syntax comes back unmasked, having -/// nothing this can recognise. -pub fn code(text: &str, language: &LanguageProfile) -> Vec { - walk(text, language).code -} - -/// Both views, from one traversal. -/// -/// They are produced together because they answer one question -- where the -/// code stops and the prose starts -- and answering it in two places is how -/// the two answers come to disagree. -struct Walk { - comments: Vec, - code: Vec, -} - -/// An unterminated single-quote string is taken to run to the end of its -/// line. The traversal does not carry one to the next line, and the mask does -/// not either, so the two always agree about where such a string stops. -fn walk(text: &str, language: &LanguageProfile) -> Walk { let Some(syntax) = language.comments else { - return Walk { - comments: Vec::new(), - code: text.lines().map(str::to_owned).collect(), - }; + return Vec::new(); }; let mut comments = Vec::new(); - let mut code = Vec::new(); let mut open_block: Option = None; let mut open_string: Option = None; for (line_index, line) in text.lines().enumerate() { let line_number = line_index + 1; - let mut holes: Vec<(usize, usize)> = Vec::new(); - - 'line_done: { - if line_number == 1 && line.starts_with("#!") && syntax.line.contains(&"#") { - break 'line_done; - } + if line_number == 1 && line.starts_with("#!") && syntax.line.contains(&"#") { + continue; + } - let mut cursor = 0; - if let Some(block) = open_block.as_mut() { - match line.find(block.end) { - Some(position) => { - let end = position + block.end.len(); - block.parts.push(CommentPart { - line: line_number, - col: 1, - text: line[..end].to_owned(), - }); - let block = open_block.take().expect("open block exists"); - comments.push(Comment { parts: block.parts }); - holes.push((0, end)); - cursor = end; - } - None => { - block.parts.push(CommentPart { - line: line_number, - col: 1, - text: line.to_owned(), - }); - holes.push((0, line.len())); - break 'line_done; - } + let mut cursor = 0; + if let Some(block) = open_block.as_mut() { + match line.find(block.end) { + Some(position) => { + let end = position + block.end.len(); + block.parts.push(CommentPart { + line: line_number, + col: 1, + text: line[..end].to_owned(), + }); + let block = open_block.take().expect("open block exists"); + comments.push(Comment { parts: block.parts }); + cursor = end; } - } else if let Some(delimiter) = open_string.as_deref() { - match line.find(delimiter) { - Some(position) => { - cursor = position + delimiter.len(); - holes.push((0, cursor)); - open_string = None; - } - None => { - holes.push((0, line.len())); - break 'line_done; - } + None => { + block.parts.push(CommentPart { + line: line_number, + col: 1, + text: line.to_owned(), + }); + continue; } } + } else if let Some(delimiter) = open_string.as_deref() { + match line.find(delimiter) { + Some(position) => { + cursor = position + delimiter.len(); + open_string = None; + } + None => continue, + } + } - let mut previous = line[..cursor].chars().next_back(); - let mut quote = None; - let mut quote_start = 0; - 'line: while cursor < line.len() { - let rest = &line[cursor..]; - let character = rest.chars().next().expect("cursor is a char boundary"); + let mut previous = line[..cursor].chars().next_back(); + let mut quote = None; + 'line: while cursor < line.len() { + let rest = &line[cursor..]; + let character = rest.chars().next().expect("cursor is a char boundary"); - if let Some(delimiter) = quote { - if character == '\\' { - cursor += character.len_utf8(); - if let Some(escaped) = line[cursor..].chars().next() { - cursor += escaped.len_utf8(); - } - continue; - } + if let Some(delimiter) = quote { + if character == '\\' { cursor += character.len_utf8(); - if character == delimiter { - quote = None; - holes.push((quote_start, cursor)); + if let Some(escaped) = line[cursor..].chars().next() { + cursor += escaped.len_utf8(); } continue; } + if character == delimiter { + quote = None; + } + cursor += character.len_utf8(); + continue; + } - if language.id == "rust" - && let Some((opening_len, delimiter)) = rust_raw_string(rest) - { - let after = cursor + opening_len; - match line[after..].find(&delimiter) { - Some(position) => { - let end = after + position + delimiter.len(); - holes.push((cursor, end)); - cursor = end; - } - None => { - open_string = Some(delimiter); - holes.push((cursor, line.len())); - break 'line; - } + if language.id == "rust" + && let Some((opening_len, delimiter)) = rust_raw_string(rest) + { + let after = cursor + opening_len; + match line[after..].find(&delimiter) { + Some(position) => cursor = after + position + delimiter.len(), + None => { + open_string = Some(delimiter); + break 'line; } - previous = line[..cursor].chars().next_back(); - continue; } + previous = line[..cursor].chars().next_back(); + continue; + } - if let Some(delimiter) = syntax - .multi_quotes - .iter() - .find(|delimiter| rest.starts_with(**delimiter)) - { - let after = cursor + delimiter.len(); - match line[after..].find(delimiter) { - Some(position) => { - let end = after + position + delimiter.len(); - holes.push((cursor, end)); - cursor = end; - } - None => { - open_string = Some((*delimiter).to_owned()); - holes.push((cursor, line.len())); - break 'line; - } + if let Some(delimiter) = syntax + .multi_quotes + .iter() + .find(|delimiter| rest.starts_with(**delimiter)) + { + let after = cursor + delimiter.len(); + match line[after..].find(delimiter) { + Some(position) => cursor = after + position + delimiter.len(), + None => { + open_string = Some((*delimiter).to_owned()); + break 'line; } - previous = delimiter.chars().next_back(); - continue; } + previous = delimiter.chars().next_back(); + continue; + } - if syntax.quotes.contains(&character) { - quote = Some(character); - quote_start = cursor; - cursor += character.len_utf8(); - continue; - } + if syntax.quotes.contains(&character) { + quote = Some(character); + cursor += character.len_utf8(); + continue; + } - if let Some((open, close)) = - syntax.block.iter().find(|(open, _)| rest.starts_with(open)) - { - let after = cursor + open.len(); - match line[after..].find(close) { - Some(position) => { - let end = after + position + close.len(); - comments.push(one_line( - line_number, - cursor + 1, - line[cursor..end].to_owned(), - )); - holes.push((cursor, end)); - cursor = end; - previous = close.chars().next_back(); - continue; - } - None => { - let opening = one_line(line_number, cursor + 1, rest.to_owned()); - open_block = Some(OpenBlock { - end: close, - parts: opening.parts, - }); - holes.push((cursor, line.len())); - break 'line; - } + if let Some((open, close)) = + syntax.block.iter().find(|(open, _)| rest.starts_with(open)) + { + let after = cursor + open.len(); + match line[after..].find(close) { + Some(position) => { + let end = after + position + close.len(); + comments.push(Comment { + parts: vec![CommentPart { + line: line_number, + col: cursor + 1, + text: line[cursor..end].to_owned(), + }], + }); + cursor = end; + previous = close.chars().next_back(); + continue; + } + None => { + open_block = Some(OpenBlock { + end: close, + parts: vec![CommentPart { + line: line_number, + col: cursor + 1, + text: rest.to_owned(), + }], + }); + break 'line; } } - - if let Some(marker) = syntax.line.iter().find(|marker| rest.starts_with(**marker)) - && boundary_ok(marker, previous) - { - comments.push(one_line(line_number, cursor + 1, rest.to_owned())); - holes.push((cursor, line.len())); - break 'line; - } - - previous = Some(character); - cursor += character.len_utf8(); } - if quote.is_some() { - holes.push((quote_start, line.len())); + if let Some(marker) = syntax.line.iter().find(|marker| rest.starts_with(**marker)) + && boundary_ok(marker, previous) + { + comments.push(Comment { + parts: vec![CommentPart { + line: line_number, + col: cursor + 1, + text: rest.to_owned(), + }], + }); + break 'line; } - } - code.push(mask(line, &holes)); + previous = Some(character); + cursor += character.len_utf8(); + } } if let Some(block) = open_block { comments.push(Comment { parts: block.parts }); } - Walk { comments, code } -} - -/// The line with the given byte ranges replaced by spaces. -/// -/// Ranges always cover whole characters, so replacing their bytes with ASCII -/// spaces leaves valid UTF-8 and leaves every column where it was. -fn mask(line: &str, holes: &[(usize, usize)]) -> String { - if holes.is_empty() { - return line.to_owned(); - } - let mut bytes = line.as_bytes().to_vec(); - for &(from, to) in holes { - let from = from.min(bytes.len()); - let to = to.min(bytes.len()); - for byte in &mut bytes[from..to.max(from)] { - *byte = b' '; - } - } - String::from_utf8(bytes).expect("blanking whole characters leaves valid UTF-8") -} - -/// The masked view keeps the shape of the line and drops only what is not -/// code. Every case here is one a rule reading `code` would otherwise get -/// wrong: a declaration commented out, quoted, or spanning a block. -#[cfg(test)] -mod tests { - use super::code; - use crate::language::language_profile; - - fn masked(source: &str, language: &str) -> Vec { - code( - source, - language_profile(language).expect("a language straitjacket knows"), - ) - } - - #[test] - fn code_survives_and_comments_do_not() { - let source = "const MAX_SIZE: u8 = 3; // MAX_OTHER"; - let lines = masked(&format!("{source}\n"), "rust"); - - assert!(lines[0].starts_with("const MAX_SIZE: u8 = 3;")); - assert!(!lines[0].contains("MAX_OTHER")); - assert_eq!(lines[0].len(), source.len(), "columns must not move"); - } - - #[test] - fn a_string_literal_is_blanked_and_its_columns_are_kept() { - let source = "let s = \"const MAX_SIZE = 3\";"; - let lines = masked(&format!("{source}\n"), "rust"); - - assert!(lines[0].starts_with("let s = ")); - assert!(!lines[0].contains("MAX_SIZE")); - assert!(lines[0].ends_with(';'), "the code after a string survives"); - assert_eq!(lines[0].len(), source.len(), "columns must not move"); - } - - #[test] - fn a_block_comment_is_blanked_on_every_line_it_spans() { - let lines = masked("a\n/* const MAX_SIZE = 1\n still inside */ b\n", "rust"); - - assert_eq!(lines[0], "a"); - assert_eq!(lines[1].trim(), ""); - assert_eq!(lines[2].trim(), "b"); - } - - #[test] - fn a_rust_raw_string_is_blanked_hashes_and_all() { - let source = "let s = r#\"const MAX_SIZE = 3\"#;"; - let lines = masked(&format!("{source}\n"), "rust"); - - assert!(lines[0].starts_with("let s = ")); - assert!(!lines[0].contains("MAX_SIZE")); - assert_eq!(lines[0].len(), source.len(), "columns must not move"); - } - - #[test] - fn a_python_docstring_is_blanked_across_its_lines() { - let lines = masked("x = 1\n\"\"\"\nMAX_SIZE = 3\n\"\"\"\ny = 2\n", "python"); - - assert_eq!(lines[0], "x = 1"); - assert_eq!(lines[2].trim(), ""); - assert_eq!(lines[4], "y = 2"); - } - - #[test] - fn a_language_with_no_comment_syntax_is_returned_whole() { - let lines = masked("{\"MAX_SIZE\": 3}\n", "json"); - - assert_eq!(lines, ["{\"MAX_SIZE\": 3}"]); - } + comments } diff --git a/src/rules/stray_const.rs b/src/rules/stray_const.rs index 0049756..e5cb80d 100644 --- a/src/rules/stray_const.rs +++ b/src/rules/stray_const.rs @@ -1,136 +1,97 @@ -//! `SCREAMING_SNAKE_CASE` constants declared outside the files that hold -//! them. +//! Constants declared outside the files that hold them, found by beamte's +//! `const-declaration` rule over a treebank pack. //! -//! A constant is a decision about the program: a limit, a path, a key, a -//! magic number somebody named. Scattered through the tree, those decisions -//! cannot be read as a set, and the same one gets made twice under two names. -//! Gathering them is not tidiness -- it is what makes the list of decisions -//! reviewable. `const-files` names where they live and everything outside is -//! reported. +//! 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. //! -//! Deliberately not a parser. A rule that has to tell one expression from -//! another needs a tree, which is why `test-quality` fetches a treebank -//! grammar. This one asks a much narrower question -- does a line *declare* a -//! screaming-snake name -- and declaration syntax answers that on its own. So -//! it runs over all eighteen languages straitjacket calls structured code -//! rather than the nine with a pack, needs no network, and is off by default -//! only because designating the files is a decision no default can make. +//! 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. //! -//! It reads [`comments::code`], not the raw text: a declaration commented out -//! or quoted inside a string is not a declaration, and those are the two -//! places source code most often appears without being code. -//! -//! **Declarations, not uses.** `MAX_SIZE` mentioned in an expression is the -//! point of having a constant, and flagging it would make the rule -//! unsatisfiable. Only the site that introduces the name is a finding. - -use std::path::{Path, PathBuf}; +//! 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 regex::Regex; +use beamte::node::Unit; +use beamte::{RuleId, Selection}; use crate::Settings; use crate::finding::{Finding, Location, Severity}; -use crate::language::{LanguageProfile, STRUCTURED_CODE}; +use crate::language::LanguageProfile; use crate::rule::{Candidate, FileRule, RuleDescriptor, RuleKey, SourceFile}; use crate::rules::RuleRegistration; -use crate::rules::comments; pub const KEY: RuleKey = RuleKey::new("stray-const"); /// Off unless a configuration asks for it. /// -/// Unlike every default-on rule, this one has nothing to say until somebody -/// says where constants belong. A default would be a guess about a layout -/// straitjacket cannot see. +/// 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 screaming-snake name: at least two words, joined by underscores. -/// -/// The underscore is required, which is the whole difference between a rule -/// and a nuisance. A single all-caps word is ambiguous in every language that -/// has one -- `PI`, `OK`, `HTTP`, a Go export, a C macro guard, a type -/// parameter -- and flagging those would bury the constants among them. -const NAME: &str = r"[A-Z][A-Z0-9]*(?:_[A-Z0-9]+)+"; - -/// Whether a bare `NAME = value` declares a constant in this language. +/// A language this rule can read: the ten treebank publishes a grammar for. /// -/// In the C family and its descendants a declaration carries a keyword, so a -/// bare assignment is either a write to something already declared or an enum -/// member -- neither of which is a constant anyone can move. Reading it as a -/// declaration there is how this rule would come to flag every `enum` body in -/// the repository. -#[derive(Clone, Copy, PartialEq, Eq)] -enum Bare { - /// Never; the language spells declarations with a keyword. - No, - /// Only at the left margin, which is where a module-level constant sits. - /// Indented, the same line is a class attribute, an enum member or a - /// local, and none of those belongs in another file. - TopLevel, - /// At any indentation: Go writes its constants inside a `const (` block. - AnyIndent, -} - -fn bare_form(language: &LanguageProfile) -> Bare { - match language.id { - "python" | "ruby" | "shell" => Bare::TopLevel, - "go" => Bare::AnyIndent, - _ => Bare::No, - } +/// 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. - allow: Vec, - /// `const NAME`, `static final int NAME`, `let NAME` -- a declaration - /// keyword, an optional type, then the name. - /// - /// The group between the keyword and the name is that type: - /// `static final int MAX_SIZE`, `const char *MAX_NAME`. Where the keyword - /// sits directly against the name it matches nothing, and where a second - /// keyword intervenes the scan simply starts again at that keyword -- - /// which is how `static final int` finds its name on the third attempt - /// rather than needing a rule of its own. - keyword: Regex, - /// `#define NAME`. - define: Regex, - /// `NAME = value` at the left margin. - bare_top: Regex, - /// `NAME = value` at any indentation. - bare_any: Regex, - /// A screaming-snake name anywhere at all. - /// - /// The prefilter, and deliberately not one of the patterns above: those - /// are anchored to a line, so asking one of them about a whole file - /// answers for its first line only. That is how an indented Go `const (` - /// block came to be skipped, caught by the test that covers Go. A bare - /// name is both cheaper to look for and free of anchors. - name: Regex, + /// 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 compile = - |pattern: String| Regex::new(&pattern).expect("built-in rule patterns must compile"); - Self { - allow, - keyword: compile(format!( - r"\b(?:const|constexpr|static|final|readonly|let|var|val)\s+(?:mut\s+)?(?:[A-Za-z_][A-Za-z0-9_:<>\[\].]*\s+)?(?:[*&]+\s*)?({NAME})\b" - )), - define: compile(format!(r"^\s*#\s*define\s+({NAME})\b")), - bare_top: compile(format!( - r"^(?:export\s+|readonly\s+)?({NAME})\s*(?::[^=]*)?=" - )), - bare_any: compile(format!( - r"^\s*(?:export\s+|readonly\s+)?({NAME})\s*(?::[^=]*)?=" - )), - name: compile(NAME.to_owned()), - } + 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 = Path::new(path); + let path = std::path::Path::new(path); self.allow.iter().any(|allowed| { path == allowed || path.starts_with(allowed) @@ -138,59 +99,27 @@ impl StrayConstRule { }) } - /// Every declaration on one line, as (byte offset, name). - /// - /// The line is already masked, so anything found here is code. - fn declarations(&self, line: &str, bare: Bare) -> Vec<(usize, String)> { - let mut found: Vec<(usize, String)> = Vec::new(); - let mut take = |regex: &Regex, assignment: bool| { - for captures in regex.captures_iter(line) { - let Some(name) = captures.get(1) else { - continue; - }; - if assignment && !assigns(line, captures.get(0).map_or(0, |whole| whole.end())) { - continue; - } - if found.iter().any(|(at, _)| *at == name.start()) { - continue; - } - found.push((name.start(), name.as_str().to_owned())); - } - }; - - take(&self.keyword, false); - take(&self.define, false); - match bare { - Bare::No => {} - Bare::TopLevel => take(&self.bare_top, true), - Bare::AnyIndent => take(&self.bare_any, true), + 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(), } - - found.sort_by_key(|(at, _)| *at); - found } } -/// Whether the `=` a bare pattern stopped on is really an assignment. -/// -/// The regex crate has no lookahead, so the character after the match is -/// checked here instead. `==` is a comparison, `=>` a hash rocket or a match -/// arm, and `=~` a Ruby match -- three ways to write a screaming-snake name -/// beside an equals sign without declaring anything. -fn assigns(line: &str, end: usize) -> bool { - !matches!(line.as_bytes().get(end), Some(b'=' | b'>' | b'~')) -} - 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 "Declare SCREAMING_SNAKE_CASE constants in the designated constant \ - files named by `const-files` in straitjacket.toml, and reference them \ - from everywhere else." - .to_string(); + return sentence; } let designated = settings .const_files @@ -198,10 +127,7 @@ fn instruction(settings: &Settings) -> String { .map(|path| path.display().to_string()) .collect::>() .join(", "); - format!( - "SCREAMING_SNAKE_CASE constants are declared in {designated} and nowhere else. \ - Reference them from there rather than declaring one where it is used." - ) + format!("{sentence} The files that hold them are {designated}.") } inventory::submit! { @@ -222,213 +148,171 @@ impl FileRule for StrayConstRule { } fn applies_to(&self, language: &LanguageProfile) -> bool { - language.has_facet(&STRUCTURED_CODE) + 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 !self.name.is_match(file.text) { + 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 bare = bare_form(file.language); - for (line_index, line) in comments::code(file.text, file.language).iter().enumerate() { - for (offset, name) in self.declarations(line, bare) { - let mut finding = Finding::new( - KEY, - Severity::Error, - Location::point(file.path, line_index + 1, offset + 1), - name.clone(), - format!("`{name}` is declared here rather than in a constant file"), - ); - finding.help = Some(self.hint()); - candidates.push(Candidate::line(finding)); + 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)); } } } -impl StrayConstRule { - 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(), +/// 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 } -#[cfg(test)] -mod tests { - use super::StrayConstRule; - use crate::language::language_profile; - use crate::rule::{Candidate, FileRule, SourceFile}; - - fn findings(source: &str, language: &str) -> Vec<(usize, usize, String)> { - at(source, language, "src/thing.x") - } - - fn at(source: &str, language: &str, path: &str) -> Vec<(usize, usize, String)> { - rule(Vec::new(), source, language, path) - } - - fn rule( - allow: Vec, - source: &str, - language: &str, - path: &str, - ) -> Vec<(usize, usize, String)> { - let mut candidates: Vec = Vec::new(); - StrayConstRule::new(allow).check( - SourceFile { - path, - language: language_profile(language).expect("a language straitjacket knows"), - text: source, - }, - &mut candidates, - ); - candidates - .into_iter() - .map(|candidate| { - ( - candidate.finding.location.line, - candidate.finding.location.col, - candidate.finding.matched, - ) - }) - .collect() - } - - #[test] - fn flags_a_keyword_declaration_and_points_at_the_name() { - let hits = findings("const MAX_SIZE: u8 = 3;\n", "rust"); - - assert_eq!(hits.len(), 1); - assert_eq!(hits[0].0, 1); - assert_eq!(hits[0].1, 7); - assert_eq!(hits[0].2, "MAX_SIZE"); - } - - #[test] - fn flags_a_declaration_behind_a_type_and_a_pointer() { - assert_eq!( - findings("static final int MAX_SIZE = 3;\n", "java")[0].2, - "MAX_SIZE" - ); - assert_eq!( - findings("static const char *DEFAULT_NAME = \"x\";\n", "c")[0].2, - "DEFAULT_NAME" - ); - assert_eq!(findings("#define MAX_RETRIES 5\n", "c")[0].2, "MAX_RETRIES"); - } - - #[test] - fn a_use_is_not_a_declaration() { - assert!(findings("if size > MAX_SIZE { return; }\n", "rust").is_empty()); - assert!(findings("foo(MAX_SIZE, OTHER_THING);\n", "rust").is_empty()); - } - - #[test] - fn a_single_word_name_is_left_alone() { - assert!(findings("const MAX: u8 = 3;\n", "rust").is_empty()); - assert!(findings("const PI: f64 = 3.14;\n", "rust").is_empty()); - } - - #[test] - fn a_commented_out_or_quoted_declaration_is_not_one() { - assert!(findings("// const MAX_SIZE: u8 = 3;\n", "rust").is_empty()); - assert!(findings("let s = \"const MAX_SIZE = 3\";\n", "rust").is_empty()); - assert!(findings("/* const MAX_SIZE = 3 */\n", "rust").is_empty()); - } - - /// Rust spells a declaration with a keyword, so a bare `MAX_SIZE = 3` - /// there is a write to something declared elsewhere rather than a - /// declaration this rule could ask anyone to move. - #[test] - fn a_bare_assignment_declares_only_where_the_language_says_so() { - assert_eq!(findings("MAX_SIZE = 3\n", "python")[0].2, "MAX_SIZE"); - assert_eq!(findings("MAX_SIZE = 3\n", "ruby")[0].2, "MAX_SIZE"); - assert_eq!(findings("MAX_SIZE=3\n", "shell")[0].2, "MAX_SIZE"); - assert!(findings("MAX_SIZE = 3;\n", "rust").is_empty()); - } - - #[test] - fn an_indented_assignment_is_a_member_rather_than_a_constant() { - let source = "class Colour(Enum):\n RED_ONE = 1\n GREEN_TWO = 2\n"; - - assert!( - findings(source, "python").is_empty(), - "an enum member cannot be moved to another file" - ); - } +/// 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) +} - #[test] - fn go_declares_inside_an_indented_const_block() { - let hits = findings("const (\n\tMAX_SIZE = 100\n)\n", "go"); +/// 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() +} - assert_eq!(hits.len(), 1); - assert_eq!(hits[0].2, "MAX_SIZE"); - } +#[cfg(test)] +mod tests { + use super::{KEY, SUPPORTED, StrayConstRule, looks_like_a_constant}; #[test] - fn a_comparison_is_not_an_assignment() { - assert!(findings("MAX_SIZE == 3\n", "python").is_empty()); - assert!(findings("MAX_SIZE != 3\n", "python").is_empty()); - assert!(findings("MAX_SIZE >= 3\n", "python").is_empty()); - assert!(findings("MAX_SIZE => 3\n", "ruby").is_empty()); + 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 a_designated_file_may_declare_freely() { - let source = "const MAX_SIZE: u8 = 3;\n"; - - assert!( - rule( - vec!["src/config.rs".into()], - source, - "rust", - "src/config.rs" - ) - .is_empty() - ); - assert_eq!( - rule(vec!["src/config.rs".into()], source, "rust", "src/main.rs").len(), - 1 - ); + 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 a_designated_directory_covers_what_is_under_it() { - let source = "const MAX_SIZE: u8 = 3;\n"; - - assert!( - rule( - vec!["src/consts/".into()], - source, - "rust", - "src/consts/a.rs" - ) - .is_empty() - ); + 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 several_declarations_on_one_line_are_reported_once_each() { - let hits = findings("const A_ONE: u8 = 1; const B_TWO: u8 = 2;\n", "rust"); + fn a_designated_file_is_designated_however_the_walk_spells_it() { + let rule = StrayConstRule::new(vec!["src/consts.rs".into(), "src/env/".into()]); - assert_eq!(hits.len(), 2); - assert_eq!(hits[0].2, "A_ONE"); - assert_eq!(hits[1].2, "B_TWO"); + 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 a_python_docstring_full_of_declarations_is_prose() { - let source = "\"\"\"\nMAX_SIZE = 3\n\"\"\"\nx = 1\n"; - - assert!(findings(source, "python").is_empty()); + 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/tests/stray_const.rs b/tests/stray_const.rs index 3a9be79..b4eb2cb 100644 --- a/tests/stray_const.rs +++ b/tests/stray_const.rs @@ -1,13 +1,15 @@ -//! `stray-const` against the languages straitjacket calls structured code. +//! `stray-const` against the languages treebank publishes a grammar for. //! -//! The point of this file is breadth, as `tests/test_quality.rs` and -//! `tests/env_vars.rs` are for their rules -- but where those two reach for a -//! grammar and cover nine languages, this one needs no parser and so has to -//! answer for eighteen. A declaration written the way each language writes -//! one is what keeps a whole language from going quietly silent. +//! 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. //! -//! Nothing here touches the network: the rule reads declaration syntax, not -//! trees, which is the trade it exists to make. +//! 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; @@ -30,14 +32,12 @@ fn findings(path: &str, source: &str) -> Vec { .scan(source, path, extension) .findings .into_iter() - .map(|finding| format!("{}:{}", finding.location.line, finding.matched)) + .map(|finding| finding.message) .collect() } /// One constant declaration per language, written the way that language -/// writes one: the label, the file, the source, and the name the finding must -/// carry. Each `source` declares exactly one constant, so exactly one finding -/// is correct in every row. +/// 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"), ( @@ -46,29 +46,12 @@ const CASES: &[(&str, &str, &str, &str)] = &[ "static DEFAULT_PATH: &str = \"/tmp\";\n", "DEFAULT_PATH", ), - ("c", "src/a.c", "#define MAX_RETRIES 5\n", "MAX_RETRIES"), - ( - "cpp", - "src/a.cc", - "constexpr int MAX_BUFFER = 1024;\n", - "MAX_BUFFER", - ), - ( - "c-sharp", - "src/A.cs", - "private const int MAX_ITEMS = 10;\n", - "MAX_ITEMS", - ), - ( - "go", - "src/a.go", - "const (\n\tMAX_SIZE = 100\n)\n", - "MAX_SIZE", - ), + ("python", "src/a.py", "MAX_SIZE = 3\n", "MAX_SIZE"), + ("ruby", "lib/a.rb", "MAX_SIZE = 3\n", "MAX_SIZE"), ( - "java", - "src/A.java", - "public static final int MAX_SIZE = 3;\n", + "typescript", + "src/a.ts", + "const MAX_SIZE: number = 3;\n", "MAX_SIZE", ), ( @@ -77,17 +60,10 @@ const CASES: &[(&str, &str, &str, &str)] = &[ "const MAX_SIZE = 3;\n", "MAX_SIZE", ), - ("kotlin", "src/a.kt", "const val MAX_SIZE = 3\n", "MAX_SIZE"), - ("php", "src/a.php", "const MAX_SIZE = 3;\n", "MAX_SIZE"), - ("python", "src/a.py", "MAX_SIZE = 3\n", "MAX_SIZE"), - ("ruby", "src/a.rb", "MAX_SIZE = 3\n", "MAX_SIZE"), - ("scala", "src/a.scala", "val MAX_SIZE = 3\n", "MAX_SIZE"), - ("shell", "src/a.sh", "MAX_SIZE=3\n", "MAX_SIZE"), - ("swift", "src/a.swift", "let MAX_SIZE = 3\n", "MAX_SIZE"), ( - "typescript", - "src/a.ts", - "const MAX_SIZE: number = 3;\n", + "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"), @@ -97,63 +73,47 @@ const CASES: &[(&str, &str, &str, &str)] = &[ fn every_language_reports_its_constant_declaration() { for (language, path, source, name) in CASES { let found = findings(path, source); - assert_eq!( - found.len(), - 1, - "{language}: expected exactly one finding in {path}, got {found:?}" - ); assert!( - found[0].ends_with(name), - "{language}: the finding should name {name}, got {found:?}" + found.iter().any(|message| message.contains(name)), + "{language}: expected a finding naming {name} in {path}, got {found:?}" ); } } #[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", - ); +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!(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}" + assert_eq!( + findings("src/main.rs", source), + Vec::::new(), + "flagging uses would make the rule impossible to satisfy" ); } #[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"; - +fn an_import_binds_a_name_without_declaring_it() { assert_eq!( - scanner.scan(source, "src/consts.rs", "rs").findings, - Vec::new(), - "the designated file is where constants are supposed to be" + findings("src/loader.py", "from settings import MAX_SIZE\n"), + Vec::::new() ); - assert_eq!(scanner.scan(source, "src/main.rs", "rs").findings.len(), 1); } #[test] -fn using_a_constant_everywhere_is_the_point_of_having_one() { - let source = "fn f(n: u8) -> bool {\n n > MAX_SIZE && n < OTHER_LIMIT\n}\n"; - +fn a_local_inside_a_body_is_nobodys_to_gather() { assert_eq!( - findings("src/main.rs", source), - Vec::::new(), - "flagging uses would make the rule impossible to satisfy" + 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/main.rs", "const MAX: u8 = 3;\nconst PI: f64 = 3.0;\n"), + findings("src/a.rs", "const MAX: u8 = 3;\nconst PI: f64 = 3.0;\n"), Vec::::new() ); } @@ -161,30 +121,48 @@ fn a_single_word_name_is_too_ambiguous_to_flag() { #[test] fn a_declaration_that_is_not_code_is_not_a_declaration() { assert_eq!( - findings("src/main.rs", "// const MAX_SIZE: u8 = 3;\n"), + findings("src/main.rs", "// const MAX_SIZE: u8 = 3;\nfn f() {}\n"), Vec::::new(), "commented out" ); assert_eq!( - findings("src/main.rs", "let s = \"const MAX_SIZE = 3\";\n"), + findings( + "src/main.rs", + "fn f() {\n let s = \"const MAX_SIZE = 3\";\n}\n" + ), Vec::::new(), "quoted in a string" ); } #[test] -fn an_enum_member_is_not_a_constant_anyone_can_move() { - assert_eq!( - findings( - "src/colour.py", - "class Colour(Enum):\n RED_ONE = 1\n GREEN_TWO = 2\n" - ), - Vec::::new() +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!( - findings("src/colour.c", "enum Colour {\n RED_ONE = 1,\n};\n"), - Vec::::new() + 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] @@ -193,10 +171,6 @@ fn data_and_prose_files_are_not_this_rules_to_read() { findings("config/a.yaml", "MAX_SIZE: 3\n"), Vec::::new() ); - assert_eq!( - findings("data/a.json", "{\"MAX_SIZE\": 3}\n"), - Vec::::new() - ); assert_eq!( findings("docs/a.md", "MAX_SIZE = 3\n"), Vec::::new() @@ -232,3 +206,14 @@ fn the_rule_is_off_until_a_configuration_asks_for_it() { "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()); +} From 6da1335475313d8c5dabe66f859fed9c7c76d834 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 20:41:38 +0000 Subject: [PATCH 3/3] Pin the beamte patch to a commit rather than a branch A `[patch.crates-io]` entry naming a branch stops resolving the moment that branch is merged and deleted, and the failure lands on whoever pushes next rather than on whoever merged. That is how #63's CI broke: beamte's branch went away under a PR that had not changed, and cargo could not resolve the dependency before a single check ran. A merged commit stays reachable, so the rev survives the merge. The table is temporary either way -- `beamte = "0.3"` above it is already written for the registry, and this whole block goes when 0.3 publishes. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01WrSzGnURZoupdEfVdwk9pg --- Cargo.lock | 2 +- Cargo.toml | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e565ae6..b439e49 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -152,7 +152,7 @@ checksum = "ac07cdecf99051d9a5238b80f35af32cdeba5b336e55d957b318b50137e18da5" [[package]] name = "beamte" version = "0.3.0" -source = "git+https://github.com/PowderworksCode/beamte?branch=claude%2Fsj-const-files-2g6g2t#2aa372ecec972e9b3535885c4615968d6a0878bc" +source = "git+https://github.com/PowderworksCode/beamte?rev=2aa372ecec972e9b3535885c4615968d6a0878bc#2aa372ecec972e9b3535885c4615968d6a0878bc" dependencies = [ "treebank", ] diff --git a/Cargo.toml b/Cargo.toml index f0970ee..38c1a5a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -67,8 +67,12 @@ 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 branch that carries it. Deliberately not -# a branch pin that can vanish: drop this table the moment 0.3 is published, -# since the requirement is already written for the registry. +# 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", branch = "claude/sj-const-files-2g6g2t" } +beamte = { git = "https://github.com/PowderworksCode/beamte", rev = "2aa372ecec972e9b3535885c4615968d6a0878bc" }