refactor: copy 0.14.1 parser from zig std#330
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a versioned Zig bundle at Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Areas to focus review on:
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
github-actions
bot
added
A-semantic
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #330 +/- ##
==========================================
- Coverage 91.67% 89.96% -1.72%
==========================================
Files 91 94 +3
Lines 7808 11478 +3670
==========================================
+ Hits 7158 10326 +3168
- Misses 650 1152 +502 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/linter/rules/useless_error_return.zig (1)
334-336: Bug: Incorrect method call passesselftwice.The method call
self.visit_switch_case_one(self, node)incorrectly passesselfas an explicit argument when it's already bound via the method call syntax.Apply this diff to fix the bug:
pub fn visit_switch_case_one_inline(self: *Visitor, node: Node.Index) VisitError!walk.WalkState { - return self.visit_switch_case_one(self, node); + return self.visit_switch_case_one(node); }src/linter/rules/case_convention.zig (1)
150-155: <!-- [scratchpad_start]
[task verify_review_comment_1/1]
[observations]
- Symbol has a field
decl: Node.Index(line 37) - this is the AST node index- Symbol.Id is a NominalId(u32) (line 53) - this is a symbol's unique identifier
- Symbol table and AST nodes are two separate data structures
- When code does
symbols.items(.decl)[id], it's looking up the node index from the symbol table using symbol id- The critical point: symbol indices and node indices are DIFFERENT and stored in different structures
[analysis]
- The bug is definitively confirmed
id(symbol index) anddecl(node index) are fundamentally different- They should NEVER be used interchangeably
- Using
idto index into ast.nodes is incorrect- The fix to use
declis correct[done]
- Verification complete: the review comment is valid
- The issue is real and the suggested fix is correct
</scratchpad_end] -->Based on my verification, I can now provide the rewritten review comment:
Use declaration node index instead of symbol index for AST node lookup
At lines 152-153, you're indexing
ast.nodes.items(.main_token)with the symbol-table indexid, when you should use the declaration node indexdecl. Symbol indices and node indices are distinct and stored in separate data structures—usingidrisks pointing at the wrong node or going out of bounds, producing incorrect diagnostic spans.The declaration node index is already available from line 134:
const decl: Node.Index = symbols.items(.decl)[id].- const ast = ctx.ast(); - const fn_keyword_token_idx: Ast.TokenIndex = ast.nodes.items(.main_token)[id]; + const ast = ctx.ast(); + const fn_keyword_token_idx: Ast.TokenIndex = ast.nodes.items(.main_token)[decl];
🧹 Nitpick comments (3)
src/linter/rules/duplicate_case.zig (1)
62-64: Consider consolidating Ast imports.The file imports
zigdirectly (line 53) and also getsAstviaSemantic(line 63). SinceSemantic.Astalready re-exports from the versioned zig module, the directzigimport is only used forLocat line 55. This works correctly but creates two paths to essentially the same types.src/zig/0.14.1/tokenizer.zig (2)
341-344: Consider project linting rules forstd.debug.print.The
dumpfunction usesstd.debug.printwhich is flagged by the project's linter. Since this is vendored code from Zig stdlib, you may want to either:
- Add a lint suppression comment if the project supports it
- Accept this as intentional for debugging purposes
404-411: Consider adding safety comments forundefinedvalues.The static analysis tool flags the
undefinedvalues fortagandend. While this is safe because both are guaranteed to be set before any return path, the project's linting rules require safety comments.pub fn next(self: *Tokenizer) Token { var result: Token = .{ - .tag = undefined, + .tag = undefined, // Set in all state branches before return .loc = .{ .start = self.index, - .end = undefined, + .end = undefined, // Set at line 1105 before return }, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
src/Semantic.zig(1 hunks)src/Semantic/Parse.zig(1 hunks)src/Semantic/ast.zig(1 hunks)src/Semantic/tokenizer.zig(2 hunks)src/linter/lint_context.zig(1 hunks)src/linter/rules/allocator_first_param.zig(1 hunks)src/linter/rules/avoid_as.zig(1 hunks)src/linter/rules/case_convention.zig(1 hunks)src/linter/rules/duplicate_case.zig(1 hunks)src/linter/rules/homeless_try.zig(1 hunks)src/linter/rules/must_return_ref.zig(1 hunks)src/linter/rules/no_catch_return.zig(1 hunks)src/linter/rules/no_print.zig(1 hunks)src/linter/rules/no_return_try.zig(1 hunks)src/linter/rules/no_unresolved.zig(1 hunks)src/linter/rules/returned_stack_reference.zig(1 hunks)src/linter/rules/suppressed_errors.zig(1 hunks)src/linter/rules/unsafe_undefined.zig(1 hunks)src/linter/rules/useless_error_return.zig(1 hunks)src/printer/AstPrinter.zig(1 hunks)src/printer/Printer.zig(0 hunks)src/printer/SemanticPrinter.zig(1 hunks)src/root.zig(2 hunks)src/source.zig(0 hunks)src/span.zig(4 hunks)src/visit/walk.zig(6 hunks)src/visit/walk_test.zig(1 hunks)src/zig.zig(1 hunks)src/zig/0.14.1/LICENSE(1 hunks)src/zig/0.14.1/primitives.zig(1 hunks)src/zig/0.14.1/string_literal.zig(1 hunks)src/zig/0.14.1/tokenizer.zig(1 hunks)
💤 Files with no reviewable changes (2)
- src/source.zig
- src/printer/Printer.zig
🧰 Additional context used
🪛 GitHub Actions: autofix.ci
src/zig/0.14.1/tokenizer.zig
[warning] 1462-1462: ba should be by, be.
🪛 GitHub Check: autofix
src/zig/0.14.1/tokenizer.zig
[warning] 1462-1462:
"ba" should be "by" or "be".
🪛 GitHub Check: Lint Changed
src/zig/0.14.1/tokenizer.zig
[warning] 409-409: unsafe-undefined
undefined is missing a safety comment
[warning] 406-406: unsafe-undefined
undefined is missing a safety comment
[warning] 343-343: no-print
Using std.debug.print is not allowed.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Collect and upload test coverage
🔇 Additional comments (39)
src/zig/0.14.1/LICENSE (1)
1-21: LGTM! Proper license attribution for copied Zig standard library code.Including the MIT license file is the correct approach when copying code from the Zig standard library.
src/zig/0.14.1/primitives.zig (1)
1-64: LGTM! Well-structured primitive type detection.The implementation correctly handles:
- Named primitives via the static map
- Integer types (i/u prefix + digits) via character checking
- Edge cases like empty strings and underscores
The tests cover the key cases appropriately.
src/linter/rules/useless_error_return.zig (1)
69-70: Import change aligns with the versioned Zig module pattern.The switch from
std.zig.Asttozig.Astvia the versioned module is consistent with the PR's goal.src/linter/rules/avoid_as.zig (1)
41-46: Import change aligns with the versioned Zig module pattern.The switch to the versioned
zig.zigmodule is consistent with the rest of the PR.src/linter/rules/homeless_try.zig (1)
69-70: Import change aligns with the versioned Zig module pattern.Consistent with the PR's approach to centralize versioned Zig imports.
src/linter/rules/returned_stack_reference.zig (1)
57-58: Import change aligns with the versioned Zig module pattern.Consistent with the PR's centralized versioned Zig imports approach.
src/linter/rules/allocator_first_param.zig (1)
56-58: Import change aligns with the versioned Zig module pattern.Consistent with the PR's centralized versioned Zig imports approach.
src/visit/walk_test.zig (1)
3-5: LGTM!The import migration to the versioned Zig module is correct and consistent with the PR's approach. The
@"0.14.1"syntax properly accesses the versioned namespace from the centralizedzig.zigmodule.src/linter/rules/no_return_try.zig (1)
46-48: LGTM!The versioned import migration is correct. The relative path
../../zig.zigproperly resolves from thesrc/linter/rules/directory.src/linter/rules/must_return_ref.zig (1)
57-59: LGTM!The import migration is consistent with the rest of the PR. The rule's functionality for detecting potentially leaked allocations via returned copies remains unchanged.
src/Semantic/ast.zig (1)
6-20: LGTM!This is a well-structured centralization of AST type re-exports. The
RawTokenstruct correctly mirrors the non-public token struct used in Zig's AST SOA, and the newNodeIndexandMaybeTokenIdtype aliases provide clean abstractions for dependent modules.src/linter/rules/duplicate_case.zig (1)
53-55: Note the inconsistent AI summary.The enriched summary references
no-print.zigbut this file isduplicate_case.zig. The import changes themselves are correct and consistent with the PR's migration pattern.src/Semantic/Parse.zig (1)
45-48: Ast now sourced from zig.zig 0.14.1; wiring looks consistentRedirecting
Astto@import("../zig.zig").@"0.14.1".Astis consistent with the rest of the PR, and the existing uses (Ast.parse,ast.deinit, field type) remain valid as long as the copied 0.14.1AstAPI matches whatSemanticexposes.Please run the existing semantic/linter tests to confirm spans, token indices, and parse results are unchanged with the new
Astimplementation.src/linter/rules/no_unresolved.zig (1)
40-41: no-unresolved now using versioned Ast; no logic changeBinding
Astfromzig.zig(0.14.1) keeps the rule’s use ofAst.Node.Tagandctx.ast()consistent with the new semantic context wiring; no control-flow or diagnostic behavior changes here.It’s still worth re-running this rule’s tests to ensure the new
Astdefinition doesn’t affect tag values or node indices.src/linter/rules/no_catch_return.zig (1)
44-49: Swapping to zig.zig Ast/Token is consistent with semantic contextUsing
zig.Astandzig.Tokenhere aligns this rule with the rest of the refactor; the existing logic overNode.Tag,Token.Tag, andTokenIndexremains structurally identical.Please confirm that
Semantic’sast.tokensis built from the samezig.Tokentype so the tag array indexing remains valid.src/linter/lint_context.zig (1)
285-287: Central Ast alias now bound to zig.zig 0.14.1Pointing
Astat../zig.zig.@"0.14.1".Ast keeps the linter context in sync with the new parser bundle;ast(),spanN,spanT, andcommentsBeforecontinue to operate on the same tree asSemantic.parse.ast.Double-check that
Semantic.parse.astis also typed as this sameAstto avoid subtle pointer/ABI mismatches.src/printer/AstPrinter.zig (1)
196-200: AstPrinter now targets versioned Ast; API usage remains validRebinding
Asttozig.Astmatches the parser refactor, and all uses (rootDecls,fullVarDecl,fullCall,fullContainerDecl, token accessors) remain compatible with the 0.14.1 AST API.Running the AST printer tests (or snapshot outputs) will help confirm there are no differences in tag names or slice boundaries with the new Ast source.
src/linter/rules/case_convention.zig (1)
39-41: case-convention hooked up to versioned Ast; behavior unchangedImporting
zig.zig0.14.1 and aliasingAst = zig.Astkeeps this rule aligned with the new parser types;Ast.TokenIndex,Ast.full.FnProto, and node access all stay on the same tree as the rest of the linter.Please ensure
LinterContext.ast()also returns this sameAsttype sofullFnProtoand token index math remain valid.src/linter/rules/suppressed_errors.zig (1)
64-66: suppressed-errors now using versioned Ast; rule logic intactBinding
Astfromzig.zig(0.14.1) cleanly aligns this rule with the new parser; uses ofNode,TokenIndex,nodeToSpan, token starts, and tags remain structurally the same.Re-run this rule’s tests to confirm spans around
catchbodies andunreachabletokens are unchanged with the new Ast implementation.src/printer/SemanticPrinter.zig (1)
222-223: SemanticPrinter now keys off zig.zig Node; tag printing remains consistentSwitching
Nodetozig.Ast.Nodekeeps thePrintableReference.nodefield and tag extraction (ast.nodes.items(.tag)) aligned with the new 0.14.1 AST definitions without changing output structure.Please re-run the semantic printer output tests (if any) to ensure node tag names and values still match expectations after the move to
zig.Ast.Node.src/Semantic/tokenizer.zig (1)
4-4: LGTM! Clean migration to versioned imports.The updates to use the centralized
zig.zigversioned module for Token and Tokenizer types are consistent and maintain the existing API surface.Also applies to: 9-9, 58-58
src/linter/rules/no_print.zig (1)
70-72: LGTM! Consistent with versioned import strategy.The Loc type alias now properly references the versioned Zig module.
src/root.zig (2)
3-3: LGTM! Good centralization of versioned imports.Exporting the versioned Zig module as a public constant provides a clean access point for downstream modules.
24-25: LGTM! Test references ensure proper symbol tracking.src/Semantic.zig (1)
146-147: LGTM! Public API now uses versioned Ast.The shift from
std.zig.Astto the versionedzig.Astimproves version control and sets up the foundation for future migration to 0.15.src/visit/walk.zig (1)
31-31: LGTM! Comprehensive update to versioned imports.All references to AST types—in code, documentation, and tests—have been consistently updated to use the versioned
zig.Ast. The mechanical nature of these changes maintains correctness.Also applies to: 402-402, 1187-1190, 1231-1231, 1252-1252
src/span.zig (1)
3-3: LGTM! Type conversions updated for versioned module.The updates to
Span.from()andLabeledSpan.from()properly handle the versionedzig.Ast.Spanandzig.Token.Loctypes. Documentation is also kept in sync.Also applies to: 65-66, 157-158, 231-231
src/linter/rules/unsafe_undefined.zig (1)
141-142: LGTM! Consistent with versioned import pattern.src/zig.zig (1)
1-11: LGTM! Well-structured versioned module wrapper.The centralized wrapper with explicit version namespacing (
@"0.14.1") is a solid design that will facilitate the future migration to 0.15. The module cleanly exposes the key components (Ast, Parse, Token, Tokenizer, primitives, string_literal) needed by the rest of the codebase.src/zig/0.14.1/string_literal.zig (6)
1-118: Well-structured error types and formatting.The error union types (
ParsedCharLiteral,Result,Error) follow Zig idioms nicely. TheError.fmtmethod provides good diagnostics with contextual error messages.
120-153: LGTM!The
parseCharLiteralfunction correctly handles UTF-8 codepoints and escape sequences with proper validation.
155-240: LGTM!The escape sequence parsing handles all standard Zig escapes with proper validation and error reporting.
325-361: LGTM!The
parseWritefunction correctly handles UTF-8 encoding for unicode escapes and rejects invalid embedded newlines.
363-373: LGTM!Memory management is correct with
defer buf.deinit()ensuring cleanup on all paths.
242-391: Good test coverage.Tests comprehensively cover success cases (ASCII, UTF-8, hex escapes, unicode escapes) and error cases (invalid escapes, missing digits, malformed unicode).
src/zig/0.14.1/tokenizer.zig (4)
3-335: LGTM!The
Tokenstruct provides a comprehensive and well-organized token model with proper keyword lookup viaStaticStringMap.
1451-1488: The "0ba" test case is intentional, not a typo.The pipeline warning about "ba" on line 1462 is a false positive.
0bais intentionally testing how the tokenizer handles an invalid binary literal (0bprefix followed by the non-binary digita). This is correct test coverage.
1714-1776: Excellent property-based test coverage.The
testPropertiesUpheldfunction verifies critical tokenizer invariants including location consistency, EOF handling, and control character restrictions.
412-1103: LGTM!The tokenizer state machine is comprehensive and handles all Zig syntax correctly, including edge cases like UTF-8 BOM, CR/LF normalization, and various numeric literal formats.
1 participant
Part of #323
This PR copies over the source code for
Ast,Parse, etc. from zig 0.14.1's standard library. Migrating to 0.15 is a large lift since both writers and the parser both got re-written. Copying this over will let us break the migration into smaller pieces.Summary by CodeRabbit
Refactor
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.