diff --git a/History/dev-diary/2026/2026-08-14-issue-707-lint-keyword-on-tokens.md b/History/dev-diary/2026/2026-08-14-issue-707-lint-keyword-on-tokens.md new file mode 100644 index 00000000..06408650 --- /dev/null +++ b/History/dev-diary/2026/2026-08-14-issue-707-lint-keyword-on-tokens.md @@ -0,0 +1,127 @@ +# 2026-08-14 — LINT-KEYWORD was a substring search, not a lint (#707) + +## Symptom + +```wfl +store s as "MNOP" +display s +``` + +```text +warning[LINT-KEYWORD]: Keyword 'NO' should be lowercase + = Change to 'no' +``` + +The control confirms the mechanism: `"MXYP"` reports nothing. The rule matched +the substring `NO` inside `MNOP`, inside a string literal. + +Comments went the same way — `// Note:` produced `Keyword 'No'`, `// TODO:` +produced `Keyword 'TO'` — as did ordinary words: `"Ineligible"` produced +`Keyword 'In'`. + +Found while porting a PHP minifier, where the program needs the character-class +constant +`"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_"`. That string +contains `MNOP` and cannot be changed — it *is* the definition of `\w`. Combined +with the companion `LINT-INDENT` defect (#706), it meant `--lint` could not be +used as a gate at all. + +## Root cause + +`KeywordCasingRule::apply` took the parsed `Program` and ignored it +(`_program`), then ran `source.find()` over the raw file for each of 26 +keywords, in `UPPERCASE` and `Mixedcase` forms. No tokenization, no word +boundaries, no exclusion of strings or comments. + +The `Mixedcase` half is what made it collide with English: `No`, `To`, `In`, +`For`, `Each`, `End`, `Check`, `If`, `Count`, `From` are ordinary words and +common prefixes, so any file with prose comments was likely to trip several. + +It was also a **false negative**. `source.find` returns only the first hit, so +the rule emitted at most one diagnostic per keyword per casing — 52 for a file +of any size. A file with fifty genuine `STORE` keywords reported one. The rule +could neither avoid reporting non-keywords nor finish reporting real ones. + +## The fact that shaped the fix + +WFL keywords are **case-sensitive**: `src/lexer/token.rs:17` is +`#[token("store")]` with no `ignore(case)`. So `STORE` and `Store` never lex as +keywords — they lex as *identifiers*. `STORE s AS "x"` fails with +`Variable 'STORE s AS' is not defined`. + +That rules out the obvious repair. "Only report where the lexer produced a +keyword token" would have reported **nothing**, silently deleting the lint while +looking like a fix. + +The rule now flags **identifier tokens whose lowercased text is a keyword**, +which is what it was always reaching for. All four defects fall out at once: +string literals lex as string tokens, comments never reach the token stream, +`Ineligible` lowercases to a non-keyword, and iterating tokens reports every +occurrence rather than the first. + +Keyword-ness is decided by lexing the lowercased word and checking it yields a +single non-identifier token — so the rule tracks the lexer instead of a +hardcoded list that drifts. The 26-keyword array is gone. + +## The mechanic that was load-bearing + +The lexer merges adjacent identifier words into one multi-word `Identifier` +token: `STORE counter` lexes as `Identifier("STORE counter")`. Checking the +whole token text would have found no keyword — and silently deleted the lint +again, in a second way. + +So the rule slices `byte_start..byte_end` out of the source and checks each +whitespace-separated word, offsetting the column accordingly. A multi-word +identifier cannot span lines (a newline flushes it), so the line is the token's +and the column is exact. + +## Red evidence + +5 of 10 linter tests failed against the old rule. The incidental one is the +nicest: the three-occurrence case returned two diagnostics — `STORE` once, plus +a bogus `TO` matched *inside the word* `STORE`. + +```text +---- test_keyword_casing_ignores_string_literals stdout ---- +string literal contents must not be linted, got [... message: "Keyword 'NO' should be lowercase" ...] +``` + +Red: `74114dc`. Green: `f6ff1f1`. 10/10 after. + +## Known limitation, unchanged by this work + +`--lint` runs after parsing, so a mis-cased keyword that *breaks* parsing never +reaches the linter — `STORE alpha as 1` exits 2 with a parse error. LINT-KEYWORD +therefore only fires on mis-cased words that still parse, e.g. `store Count as 5`. +That is pre-existing CLI behavior, identical before and after, and is recorded +here so the rule's real reach is not overstated. + +## Review round: the one token that is case-insensitive + +Codex and Devin both caught, independently, that the first cut of this rewrite +lost coverage the old rule had. + +`yes`/`no`/`true`/`false` are the single case-*insensitive* production in the +lexer: + +```rust +#[regex("(?i:yes|no|true|false)", ...)] +BooleanLiteral(bool), +``` + +So `store flag as YES` lexes as a `BooleanLiteral`, never as an `Identifier`, +and an identifier-only rule skipped it silently. The pre-#707 implementation +carried `yes` and `no` in its 26-keyword array and *did* warn on them — so the +claim that "output for genuine hits is identical" was false for exactly those +two keywords, in exactly the way a substring search happened to get right. + +The rule now checks `BooleanLiteral` spans as well. Everything else in the +lexer is case-sensitive — `NothingLiteral` is `#[token("nothing")]`, so +`Nothing` still arrives as an identifier and was always covered. + +Worth recording as a general shape: replacing a crude mechanism with a precise +one silently drops whatever the crude mechanism covered by accident. The +regression was invisible in the six tests written for the reported defect, +because those tests were all about false *positives*. + +Red: `90bfb69`. Green: the boolean-literal branch, 12/12 after. diff --git a/src/linter/mod.rs b/src/linter/mod.rs index 4ef8c01e..53b4231d 100644 --- a/src/linter/mod.rs +++ b/src/linter/mod.rs @@ -1,5 +1,8 @@ use crate::diagnostics::{DiagnosticReporter, Severity, WflDiagnostic}; +use crate::lexer::lex_wfl_with_positions; +use crate::lexer::token::Token; use crate::parser::ast::{Program, Statement}; +use std::collections::HashMap; use std::path::Path; pub trait LintRule { @@ -245,76 +248,63 @@ impl LintRule for KeywordCasingRule { ) -> Vec { let mut diagnostics = Vec::new(); - if let Ok(file) = reporter.files.get(file_id) { - let source = file.source(); + let Ok(file) = reporter.files.get(file_id) else { + return diagnostics; + }; + let source = file.source(); + + // WFL keywords are case-sensitive, so `STORE` never lexes as a keyword — + // it lexes as an identifier. A mis-cased keyword is therefore an + // identifier token whose lowercase form *is* a keyword. Working from the + // token stream (instead of raw substring search over the source) keeps + // string literals and comments out of the rule entirely, never matches + // inside a longer word, and reports every occurrence rather than the first. + let mut keyword_cache: HashMap = HashMap::new(); + + for token in lex_wfl_with_positions(source) { + // The lexer merges adjacent identifier words into one multi-word + // identifier, so check each word separately. Words are ASCII and the + // separators are spaces/tabs, so the byte offset within the token's + // span is also the column offset. Multi-word identifiers never span + // lines (a newline flushes them), so the line is the token's line. + let text = match token.token { + Token::Identifier(ref name) => source + .get(token.byte_start..token.byte_end) + .unwrap_or(name.as_str()), + // `yes`/`no`/`true`/`false` are the one case-*insensitive* + // production in the lexer (`src/lexer/token.rs:446`), so `YES` + // lexes as a boolean literal and never reaches the identifier + // branch. The pre-#707 rule carried `yes`/`no` in its keyword + // array and warned on them, so they are checked here to keep + // that coverage rather than silently dropping it. + Token::BooleanLiteral(_) => { + let Some(text) = source.get(token.byte_start..token.byte_end) else { + continue; + }; + text + } + _ => continue, + }; - let keywords = [ - "store", - "as", - "create", - "change", - "to", - "define", - "action", - "called", - "give", - "back", - "check", - "if", - "otherwise", - "end", - "count", - "from", - "for", - "each", - "in", - "while", - "display", - "yes", - "no", - "nothing", - "missing", - "undefined", - ]; - - for keyword in keywords.iter() { - let uppercase_keyword = keyword.to_uppercase(); - let mixed_case_keyword = keyword - .chars() - .enumerate() - .map(|(i, c)| { - if i == 0 { - c.to_uppercase().next().unwrap() - } else { - c - } - }) - .collect::(); - - if let Some(pos) = source.find(&uppercase_keyword) { - let line_col = line_col_from_pos(source, pos); - diagnostics.push(WflDiagnostic::new( - Severity::Warning, - format!("Keyword '{uppercase_keyword}' should be lowercase"), - Some(format!("Change to '{keyword}'")), - "LINT-KEYWORD".to_string(), - file_id, - line_col.0, - line_col.1, - None, - )); + for (offset, word) in word_positions(text) { + if !word.chars().any(char::is_uppercase) { + continue; } - if let Some(pos) = source.find(&mixed_case_keyword) { - let line_col = line_col_from_pos(source, pos); + let lowercase = word.to_lowercase(); + let is_keyword = *keyword_cache + .entry(lowercase.clone()) + .or_insert_with(|| is_keyword(&lowercase)); + + if is_keyword { diagnostics.push(WflDiagnostic::new( Severity::Warning, - format!("Keyword '{mixed_case_keyword}' should be lowercase"), - Some(format!("Change to '{keyword}'")), + format!("Keyword '{word}' should be lowercase"), + Some(format!("Change to '{lowercase}'")), "LINT-KEYWORD".to_string(), file_id, - line_col.0, - line_col.1, + token.line, + token.column + offset, None, )); } @@ -325,6 +315,39 @@ impl LintRule for KeywordCasingRule { } } +/// Split `text` into whitespace-separated words, paired with each word's byte +/// offset within `text`. +fn word_positions(text: &str) -> Vec<(usize, &str)> { + let mut words = Vec::new(); + let mut start: Option = None; + + for (index, ch) in text.char_indices() { + if ch.is_whitespace() { + if let Some(word_start) = start.take() { + words.push((word_start, &text[word_start..index])); + } + } else if start.is_none() { + start = Some(index); + } + } + + if let Some(word_start) = start { + words.push((word_start, &text[word_start..])); + } + + words +} + +/// Whether `lowercase` (already lowercased) is a WFL keyword. +/// +/// Determined by lexing it: a keyword lexes to exactly one non-identifier token. +/// Asking the lexer keeps this rule in sync with the language automatically, +/// instead of a hardcoded keyword list that drifts. +fn is_keyword(lowercase: &str) -> bool { + let tokens = lex_wfl_with_positions(lowercase); + matches!(tokens.as_slice(), [only] if !matches!(only.token, Token::Identifier(_))) +} + struct TrailingWhitespaceRule; impl LintRule for TrailingWhitespaceRule { @@ -532,26 +555,6 @@ fn to_snake_case(s: &str) -> String { result } -fn line_col_from_pos(source: &str, pos: usize) -> (usize, usize) { - let mut line = 1; - let mut col = 1; - - for (i, c) in source.char_indices() { - if i >= pos { - break; - } - - if c == '\n' { - line += 1; - col = 1; - } else { - col += 1; - } - } - - (line, col) -} - #[cfg(test)] mod tests; diff --git a/src/linter/tests.rs b/src/linter/tests.rs index ed5ea20e..e6c4d313 100644 --- a/src/linter/tests.rs +++ b/src/linter/tests.rs @@ -38,6 +38,96 @@ fn test_is_snake_case() { assert!(!is_snake_case("Mixed_Style")); } +/// Apply only `KeywordCasingRule` to `input` and return its diagnostics. +/// +/// The rule does not consult the AST, so an input that intentionally fails to +/// parse (e.g. `STORE counter as 5`, where `STORE` lexes as an identifier) +/// falls back to an empty program rather than panicking. +fn keyword_casing_diagnostics(input: &str) -> Vec { + let tokens = lex_wfl_with_positions(input); + let program = Parser::new(&tokens).parse().unwrap_or_default(); + + let rule = KeywordCasingRule; + let mut reporter = DiagnosticReporter::new(); + let file_id = reporter.add_file("test.wfl", input); + + rule.apply(&program, &mut reporter, file_id) +} + +/// Regression for #707: keyword casing must not match inside string literals. +#[test] +fn test_keyword_casing_ignores_string_literals() { + let diagnostics = keyword_casing_diagnostics("store s as \"MNOP\""); + assert!( + diagnostics.is_empty(), + "string literal contents must not be linted, got {diagnostics:?}" + ); +} + +/// Regression for #707: keyword casing must not match inside comments. +#[test] +fn test_keyword_casing_ignores_comments() { + let diagnostics = keyword_casing_diagnostics("// Note: this explains the next step"); + assert!( + diagnostics.is_empty(), + "comment text must not be linted, got {diagnostics:?}" + ); +} + +/// Regression for #707: keyword casing must not match inside ordinary words. +#[test] +fn test_keyword_casing_ignores_words_containing_keywords() { + let diagnostics = keyword_casing_diagnostics("store label as \"Ineligible\""); + assert!( + diagnostics.is_empty(), + "substrings of ordinary words must not be linted, got {diagnostics:?}" + ); +} + +/// A genuinely mis-cased keyword must still be reported (backward compatibility). +#[test] +fn test_keyword_casing_flags_uppercase_keyword() { + let diagnostics = keyword_casing_diagnostics("STORE counter as 5"); + + assert_eq!(diagnostics.len(), 1, "got {diagnostics:?}"); + assert_eq!(diagnostics[0].code, "LINT-KEYWORD"); + assert_eq!(diagnostics[0].severity, Severity::Warning); + assert_eq!( + diagnostics[0].message, + "Keyword 'STORE' should be lowercase" + ); + assert_eq!(diagnostics[0].notes, vec!["Change to 'store'".to_string()]); + assert_eq!(diagnostics[0].line, 1); + assert_eq!(diagnostics[0].column, 1); +} + +/// Regression for #707: every occurrence is reported, not just the first. +#[test] +fn test_keyword_casing_reports_every_occurrence() { + let input = "STORE alpha as 1\nSTORE beta as 2\nSTORE gamma as 3"; + let diagnostics = keyword_casing_diagnostics(input); + + assert_eq!(diagnostics.len(), 3, "got {diagnostics:?}"); + let lines: Vec = diagnostics.iter().map(|d| d.line).collect(); + assert_eq!(lines, vec![1, 2, 3]); + assert!( + diagnostics + .iter() + .all(|d| d.message == "Keyword 'STORE' should be lowercase") + ); +} + +/// A correctly written program produces no keyword-casing diagnostics. +#[test] +fn test_keyword_casing_clean_program() { + let input = "store counter as 5\ndisplay counter\n"; + let diagnostics = keyword_casing_diagnostics(input); + assert!( + diagnostics.is_empty(), + "lowercase program must be clean, got {diagnostics:?}" + ); +} + #[test] fn test_linter_integration() { let input = "store Counter as 5\nstore snakecase as 10"; @@ -59,3 +149,39 @@ fn test_linter_integration() { .any(|d| d.code == "LINT-NAME" && d.message.contains("snakecase")) ); } + +/// Regression for #707 review: `yes`/`no`/`true`/`false` lex case-insensitively +/// (`src/lexer/token.rs:446`), so `YES` becomes a `BooleanLiteral` rather than an +/// `Identifier`. The pre-#707 rule carried `yes`/`no` in its keyword array and +/// warned on them, so skipping non-identifier tokens would silently drop that +/// coverage. +#[test] +fn test_keyword_casing_flags_mis_cased_boolean_literals() { + for (source, expected) in [ + ("store flag as YES", "YES"), + ("store flag as No", "No"), + ("store flag as TRUE", "TRUE"), + ("store flag as False", "False"), + ] { + let diagnostics = keyword_casing_diagnostics(source); + assert_eq!( + diagnostics.len(), + 1, + "`{source}` should report exactly one mis-cased literal, got {diagnostics:?}" + ); + assert_eq!( + diagnostics[0].message, + format!("Keyword '{expected}' should be lowercase") + ); + } +} + +/// Correctly-cased boolean literals must stay silent. +#[test] +fn test_keyword_casing_accepts_lowercase_boolean_literals() { + let diagnostics = keyword_casing_diagnostics("store flag as yes\nstore other as false"); + assert!( + diagnostics.is_empty(), + "lowercase boolean literals must not be linted, got {diagnostics:?}" + ); +}