fix: register syntax option implicit locals as lowercase#166
Conversation
Stata's `syntax` command uses uppercase letters in option names to declare a minimum abbreviation (e.g. `Cache(string)` accepts `cache(...)`, `Cac(...)`, ..., `C(...)`), but the implicit local it creates at runtime is always the lowercase form. The analyzer was registering the local under the original casing, so references like `` `cache' `` inside the program body were flagged as undefined for any program declaring `Cache(...)` in its syntax line.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughSyntax option names registered as implicit local macros are now lowercased during registration. Documentation is updated to clarify case-sensitive matching elsewhere. Tests validate that mixed-case and uppercase syntax options create lowercase implicit locals and preserve case-sensitivity in macro references. ChangesSyntax Option Implicit Local Registration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
- syntax-option-capitalization.test.ts: assert that a wrong-case reference (`Cache') still produces an undefined-macro diagnostic. Locks in the semantics in both directions and prevents accidentally registering both casings. - syntax-command-analyzer.prop.test.ts: deduplicate generated option names by their lowercase form before asserting, and confirm every distinct lowercase name maps to exactly one entry. Previously the generator could produce inputs that collapse to one runtime local (e.g. `Foo`, `foo`), and the assertion looked stronger than it was. - analyzer/index.ts: document why extract_syntax_option_names preserves original casing — full case-insensitive matching of macro-creating options also requires lowercasing call-site option matching, which is broader than the implicit-local fix.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/analyzer/syntax-option-capitalization.test.ts (1)
16-43: ⚡ Quick winAlign scoped variable names with the repo’s
my_naming convention.Please rename scoped variables like
analyzer,lexer, andparserto the project’s prefixed form for consistency in this new test file.As per coding guidelines, "
**/*.ts: ... Usemy_prefix for loop iterators and scoped variables (except single lettersi,j,k)."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/analyzer/syntax-option-capitalization.test.ts` around lines 16 - 43, Scoped variables in this test file (analyzer, lexer, parser) don't follow the repo naming convention—rename analyzer to my_analyzer, lexer to my_lexer, and parser to my_parser and update every reference (the beforeEach initializer and usages inside analyze_document and undefined_macro_messages) so the test uses my_analyzer, my_lexer, my_parser consistently; ensure function analyze_document uses my_lex_result.tokenize(my_source) and parser.parse calls are updated to use my_parser and pass my_lex_result.tokens, and that analyzer.analyze is replaced with my_analyzer.analyze in the return path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/analyzer/syntax-option-capitalization.test.ts`:
- Around line 16-43: Scoped variables in this test file (analyzer, lexer,
parser) don't follow the repo naming convention—rename analyzer to my_analyzer,
lexer to my_lexer, and parser to my_parser and update every reference (the
beforeEach initializer and usages inside analyze_document and
undefined_macro_messages) so the test uses my_analyzer, my_lexer, my_parser
consistently; ensure function analyze_document uses
my_lex_result.tokenize(my_source) and parser.parse calls are updated to use
my_parser and pass my_lex_result.tokens, and that analyzer.analyze is replaced
with my_analyzer.analyze in the return path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8c9eb630-e59d-47a9-89c8-752f6b235bd6
📒 Files selected for processing (3)
src/analyzer/index.tstests/property/syntax-command-analyzer.prop.test.tstests/unit/analyzer/syntax-option-capitalization.test.ts
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
1 participant
Summary
syntaxcommand uses uppercase letters in option names to declare a minimum abbreviation (Cache(string)acceptscache(...),Cac(...), ...,C(...)), but the implicit local Stata creates at runtime is always lowercased. The analyzer was registering the local under the original casing, so references like`cache'inside the program body were flagged as undefined.register_implicit_locals: lowercaseopt.namewhen registering and looking up the implicit-local symbol.OptionSpec.nameitself is left intact so hover and signature display continue to show the user's original casing (which conveys the abbreviation hint).format_birth_data.do, which declaressyntax, Cache(string) Prefix(string) Outpath(string) [Suffix(string)]and then references`cache',`prefix',`outpath',`suffix'in the program body.Test plan
tests/unit/analyzer/syntax-option-capitalization.test.tscovering mixed-case (Cache), already-lowercase, and fully-uppercase (OUT) options.tests/property/syntax-command-analyzer.prop.test.ts— the existing property assertion encoded the old buggy round-trip; it now asserts the correct Stata semantics (lowercase runtime name).bun run test— 5530 pass / 0 fail.bun run typecheck— clean.Summary by CodeRabbit