fix(linter): match LINT-KEYWORD on identifier tokens, not raw substrings (#707) - #714
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ngs (#707) KeywordCasingRule scanned the raw source with `source.find()` for each of 26 keywords in UPPERCASE and Mixedcase forms. That matched inside string literals ("MNOP" reported `NO`), inside comments (`// Note:` reported `No`), and inside ordinary words ("Ineligible" reported `In`) — and because `find` returns only the first hit, it reported at most one diagnostic per keyword per casing. WFL keywords are case-sensitive, so a mis-cased keyword lexes as an identifier. The rule now walks the token stream and flags identifier words whose lowercase form is a keyword, determined by lexing that lowercase form (so the rule stays in sync with the lexer rather than a hardcoded list). Comments and string literals are never identifiers, whole-word matching is inherent, and every occurrence is reported. Message, hint, code and severity are unchanged, so genuine hits produce identical output. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 7 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2aa19bec03
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let Token::Identifier(ref name) = token.token else { | ||
| continue; |
There was a problem hiding this comment.
For inputs such as store answer as YES or store answer as No, the lexer emits Token::BooleanLiteral because its boolean regex is case-insensitive (src/lexer/token.rs:446-453), so this identifier-only branch skips the token entirely. The previous rule explicitly checked yes and no, meaning these valid, parseable inputs have now silently lost their LINT-KEYWORD warning despite the change's backward-compatibility claim; inspect non-lowercase boolean literal tokens before continuing.
AGENTS.md reference: AGENTS.md:L21-L21
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| for token in lex_wfl_with_positions(source) { | ||
| let Token::Identifier(ref name) = token.token else { | ||
| continue; | ||
| }; |
There was a problem hiding this comment.
🟡 Wrongly capitalized yes/no values are no longer reported by the style checker
Only words the language reader treats as names are examined (Token::Identifier filter at src/linter/mod.rs:265-267), while YES, NO, Yes, No are read as true/false values, so genuinely mis-capitalized yes/no words are silently skipped.
Impact: Programs written with YES/NO instead of yes/no used to get a style warning and now get none, so the style check quietly stops covering those words.
Case-insensitive boolean literal regex keeps YES/NO out of the identifier stream
src/lexer/token.rs:446 matches booleans case-insensitively (#[regex("(?i:yes|no|true|false)")]), confirmed by src/lexer/tests.rs:615-645 where "YES", "NO", "True", "False" all lex to Token::BooleanLiteral. The new rule iterates tokens and skips anything that is not Token::Identifier, so those tokens never reach the word check. The old implementation had "yes" and "no" in its 26-keyword array (src/linter/mod.rs prior lines 273-274) and did report store flag as YES as Keyword 'YES' should be lowercase. So the PR claim that "output for genuine hits is identical" does not hold for these two keywords. A fix would additionally inspect Token::BooleanLiteral (and NothingLiteral-style literal) spans, comparing the source slice against its lowercase form.
Prompt for agents
KeywordCasingRule in src/linter/mod.rs now only inspects Token::Identifier tokens. Because src/lexer/token.rs:446 matches yes/no/true/false case-insensitively, inputs like `store flag as YES` lex to Token::BooleanLiteral and are never examined, so a genuinely mis-cased `YES`/`NO` produces no LINT-KEYWORD warning — a regression versus the removed substring implementation, which listed `yes` and `no` among its keywords. Consider also examining non-identifier tokens whose source slice (source[byte_start..byte_end]) contains uppercase characters and whose lowercase form differs, reporting them with the same message/hint, and add a regression test for `store flag as YES`.
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn is_keyword(lowercase: &str) -> bool { | ||
| let tokens = lex_wfl_with_positions(lowercase); | ||
| matches!(tokens.as_slice(), [only] if !matches!(only.token, Token::Identifier(_))) | ||
| } |
There was a problem hiding this comment.
🔍 Keyword set widened from 26 hardcoded words to the lexer's full ~181 keywords
Deriving keyword-ness from the lexer (is_keyword at src/linter/mod.rs:335-338) is much broader than the removed 26-word array: src/lexer/token.rs defines tokens for common English nouns such as list, text, data, time, date, status, output, error, request, response, empty, test, start, new, one, any. Any capitalized word inside a variable name whose lowercase form is one of these now warns, e.g. store User Data as 5 yields "Keyword 'Data' should be lowercase" with the hint "Change to 'data'" — applying that hint would turn part of an identifier into a keyword and break the program. This is the rule's intended semantics (the tests use store Count as 5), but it materially expands the set of legitimate identifiers that now produce warnings, which matters because --lint is used as a gate. Worth confirming against TestPrograms//examples/ that no shipped program starts emitting new LINT-KEYWORD warnings.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Pull request overview
Fixes LINT-KEYWORD false positives/negatives by switching from raw source.find() substring matching to token-based inspection, aligning the lint with what the lexer actually recognizes and allowing it to report every occurrence.
Changes:
- Reimplemented
KeywordCasingRuleto scan lexer tokens (identifiers) and detect mis-cased keywords via lowercasing + re-lexing, with correct per-occurrence positioning. - Added focused regression tests for the issue #707 cases (strings/comments/ordinary words) plus multi-occurrence reporting.
- Added a dev-diary entry documenting the bug, the root cause, and the rationale behind the new approach.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/linter/mod.rs | Reworks LINT-KEYWORD implementation to operate on lexer output (and adds helper utilities). |
| src/linter/tests.rs | Adds targeted unit tests covering #707 regressions and multi-occurrence reporting. |
| History/dev-diary/2026/2026-08-14-issue-707-lint-keyword-on-tokens.md | Documents the defect and the design constraints that shaped the fix. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// 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); | ||
| } | ||
|
|
| for token in lex_wfl_with_positions(source) { | ||
| let Token::Identifier(ref name) = token.token else { | ||
| continue; | ||
| }; | ||
|
|
||
| // 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 = source | ||
| .get(token.byte_start..token.byte_end) | ||
| .unwrap_or(name.as_str()); | ||
|
|
Review findings — all addressed@chatgpt-codex-connector and @devin-ai-integration — confirmed and fixed. Both of you independently found a real regression, and you were right.
Fixed in I checked whether anything else was affected: New tests: Worth naming the general shape, since it is the interesting part of this review: replacing a crude mechanism with a precise one silently drops whatever the crude mechanism covered by accident. Every test written for the reported defect was about false positives, so none of them could have caught a lost true positive. Recorded in the dev diary. |
Summary
Testing
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/linter/mod.rs:298
keyword_cache.entry(lowercase.clone())clones/allocates the lowercased string even on cache hits, doubling work for files with many repeated identifiers (e.g. manySTOREoccurrences). You can avoid the extra clone by checkingget()first and only cloning when inserting a new key.
let lowercase = word.to_lowercase();
let is_keyword = *keyword_cache
.entry(lowercase.clone())
.or_insert_with(|| is_keyword(&lowercase));
src/linter/tests.rs:103
- The rule’s new
token.column + offsetcalculation is load-bearing for multi-word identifiers, but the tests don’t currently cover the case where the mis-cased keyword appears later in the identifier (to validate the offset math). Adding a regression that asserts the reported column forstore My Store as 5would protect against future changes to identifier merging/whitespace handling.
/// 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");
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/linter/mod.rs:287
Token::Identifierfalls back toname.as_str()if slicing the original source span fails. That fallback can produce incorrect word offsets (and therefore incorrect diagnostic columns) because the linter later adds byte offsets computed from the sliced text. Since diagnostics depend on exact source positions, it’s safer and consistent with theBooleanLiteralarm to skip the token when the slice can’t be recovered.
let text = match token.token {
Token::Identifier(ref name) => source
.get(token.byte_start..token.byte_end)
.unwrap_or(name.as_str()),
Summary
Closes #707.
LINT-KEYWORDransource.find()over the raw file for 26 keywords inUPPERCASEandMixedcaseforms, ignoring the parsedProgramit was handed.It therefore fired on text inside string literals, inside comments, and inside
ordinary English words:
store s as "MNOP"Keyword 'NO' should be lowercase// Note: this explains the next stepKeyword 'No' should be lowercasestore label as "Ineligible"Keyword 'In' should be lowercaseControl:
store s as "MXYP"reports nothing, confirming substring matching.It was also a false negative:
source.findreturns only the first hit, sothe rule emitted at most one diagnostic per keyword per casing. A file with
fifty genuine
STOREkeywords reported one.The fact that determined the fix
WFL keywords are case-sensitive —
src/lexer/token.rs:17is#[token("store")]with noignore(case).STOREandStoredo not lex askeywords; they lex as identifiers (
STORE s AS "x"fails withVariable 'STORE s AS' is not defined).So the obvious repair — "only report where the lexer produced a keyword token" —
would have reported nothing, silently deleting the lint while appearing to
fix it.
What the rule does now
Flags identifier tokens whose lowercased text is a keyword. All four defects
fall out at once: string literals lex as string tokens, comments never reach the
token stream,
Ineligiblelowercases to a non-keyword, and iterating tokensreports every occurrence instead of 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 rather than a
hardcoded list that drifts. The 26-keyword array, both
source.findblocks, andthe now-dead
line_col_from_posare removed. Message, hint, code and severitywording are byte-for-byte unchanged, so output for genuine hits is identical.
One mechanic worth calling out in review: the lexer merges adjacent identifier
words into a single multi-word
Identifiertoken (STORE counter→ oneIdentifier("STORE counter")). Checking the whole token text would have foundno keyword and deleted the lint a second way, so the rule slices
byte_start..byte_endfrom the source and checks each whitespace-separatedword, offsetting the column. A multi-word identifier cannot span lines, so the
line is the token's and the column is exact.
Test evidence
Risk class: R2 —
--lintoutput and exit code are CLI behavior and apublic contract. Backward compatibility verified: a genuine mis-cased keyword
still warns.
Acceptance criteria → tests (all in
src/linter/tests.rs):test_keyword_casing_ignores_string_literals(the exact repro from the issue)test_keyword_casing_ignores_commentstest_keyword_casing_ignores_words_containing_keywordstest_keyword_casing_flags_uppercase_keyword(exactly 1, correct line and column)test_keyword_casing_reports_every_occurrence(3 occurrences → 3 diagnostics)test_keyword_casing_clean_programRed evidence:
74114dc(test-only, ancestor of the fix). 5 of 10 failed:The three-occurrence case is the most telling — it returned 2:
STOREonce, plus a bogus
TOmatched inside the wordSTORE.Green evidence:
f6ff1f1— 10/10.Unit/component:
cargo test --lib linter::→ 10 passed, 0 failed.Integration/contract:
cargo test --allpasses, excluding two pre-existingflaky suites unrelated to this change (see Residual risk).
End-to-end: the issue's repro now lints with 0 issues, exit 0;
store Count as 5/store Display as 6still yields both warnings, 2issues, exit 1.
Static:
cargo fmt --all -- --checkclean;cargo clippy --all-targets --all-features -- -D warningsclean.Coverage: six cases — three false-positive classes, one true positive, the
truncation case, and a clean program.
Platforms: authored and run on Windows; CI covers Ubuntu and Windows.
Not applicable, with reason: no
Docs/change. The only mention of therule is
Docs/06-best-practices/code-style-guide.md:333, the table row| LINT-KEYWORD | Lowercase keywords |, which remains accurate — the rule'scontract did not change, only its matching mechanism. Nothing under
Docs/documented the substring behavior, so there is no stale claim to correct.
Rollback/recovery: revert; no persistent state involved.
Residual risk:
--lintruns after parsing, so a mis-cased keyword thatbreaks parsing never reaches the linter (
STORE alpha as 1exits 2 with aparse 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, identicalbefore and after this change, and is documented in the dev diary so the rule's
reach is not overstated. Separately,
tests/file_io_concurrent_test.rsandtests/file_io_performance_test.rsare flaky on this machine and also writeoutput files into the repo root rather than
target/test-artifacts/; both wereconfirmed pre-existing by re-running against the unmodified tree, and are
reported separately rather than fixed here.
🤖 Generated with Claude Code