Replace handrolled scan with AST - #4
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change adds AST-based Zig import analysis, sentinel-terminated file reading, alias-aware sorting, AST-based banned-pattern detection, and tests for typed imports, aliases, C imports, comments, and unchanged files. ChangesAST import scanning and sorting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Worker
participant compat.readFileAllocZ
participant ast_scan.analyze
participant zsort.processSource
participant buildSortedImportText
Worker->>compat.readFileAllocZ: read sentinel-terminated source
compat.readFileAllocZ->>zsort.processSource: source buffer
zsort.processSource->>ast_scan.analyze: analyze declarations and import calls
ast_scan.analyze->>zsort.processSource: imports, aliases, and block boundary
zsort.processSource->>buildSortedImportText: sorted imports and aliases
buildSortedImportText->>Worker: updated source
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
src/zsort.zig (2)
54-59: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winParse the source once per file.
processSourcecallsast_scan.analyze, which duplicates the source and parses it. Line 450 then callshasBannedPatterns, which duplicates the source again and parses it again. Each file therefore pays twodupeZallocations and two full parses.Return the
Ast(or theImportCalllist) fromanalyze, or accept a parsed tree inhasBannedPatterns. The worker inprocessFileJobalready reads a[:0]u8buffer, soprocessSourcecan accept[:0]const u8and remove both copies.Also applies to: 450-450
🤖 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 `@src/zsort.zig` around lines 54 - 59, Update the processSource/analyze/hasBannedPatterns flow to parse each file only once: reuse the parsed Ast or ImportCall list produced by analyze when checking banned patterns, and avoid duplicating the source again. Propagate the worker’s existing [:0]u8 buffer through processSource as [:0]const u8, removing redundant dupeZ allocations while preserving current analysis and validation behavior.
20-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the production wrappers.
collectImportsandfindImportBlockEndare called only bysrc/zsort_test.zig. Useast_scan.analyzedirectly in the tests, then delete both wrappers.🤖 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 `@src/zsort.zig` around lines 20 - 40, Remove the production wrappers collectImports and findImportBlockEnd from src/zsort.zig. Update the corresponding usages in zsort_test.zig to call ast_scan.analyze directly, preserving the existing allocator setup, source duplication, import collection, block_end handling, and fallback behavior required by the tests.src/ast_scan.zig (2)
147-173: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueNote the quadratic line-versus-span scan.
lineBlockEndscans every leading line against every import and alias span. For a large import block the cost is O(lines × imports). Both slices are already ordered by source position for imports appended in decl order, so a single merge-style walk is possible.This is not a correctness problem. Consider it only if large files become slow.
🤖 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 `@src/ast_scan.zig` around lines 147 - 173, Optimize lineBlockEnd by replacing the per-line scans across imports and aliases with a merge-style walk over their source-ordered spans, maintaining advancing indices and skipping spans that end before the current line. Preserve the existing handling of blank/comment lines and return the first uncovered line position.
255-260: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
stringPathreturns the raw literal body without unescaping.The function strips the quotes only. A path written as
@import("a\x2fb.zig")yields the raw text, soclassifyand the banned-prefix check see the escaped form. The result is a missedclass_localclassification and a missed banned-prefix match.Escaped import paths are rare. Consider
std.zig.string_literal.parseAllocif you want exact handling.🤖 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 `@src/ast_scan.zig` around lines 255 - 260, Update stringPath to unescape the string literal body before returning it, using std.zig.string_literal.parseAlloc or the project’s established equivalent; ensure escaped paths such as \x2f are decoded so classify and banned-prefix checks receive the actual path text, while preserving null handling for non-string or invalid literals.
🤖 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.
Inline comments:
In `@src/ast_scan.zig`:
- Around line 286-296: Update spanEnd so its semicolon search cannot reach a
later declaration: bound scanning to lastToken(node) + 1 and stop when a token
begins on a line later than the declaration’s last token. Preserve the existing
line-end calculation for a semicolon found within the allowed range and for the
fallback token.
In `@src/compat.zig`:
- Around line 26-31: Update the is_v016 branch in readFileAllocZ to call
std.Io.Dir.readFileAllocOptions without passing io, using the Zig 0.16 argument
order: dir, path, allocator, .limited(max), .of(u8), and sentinel 0. Leave the
Zig 0.15 branch unchanged.
In `@src/zsort_test.zig`:
- Around line 721-723: Strengthen the test assertions around late_pos and
joined_pos so both the multiline import and its trailing comment are verified to
appear before the pub fn main() declaration. Preserve the existing ordering
assertion that joined_pos follows late_pos, and locate the declaration position
from result.new_text for comparison.
In `@src/zsort.zig`:
- Around line 61-70: Replace the line-prefix logic in the call scan with an
AST-based check using the declaration classification from ast_scan: expose the
set of const declarations whose initializer is an `@import` call, then allow only
calls belonging to that set and classify every other import call as inline.
Remove the findLineStart/findLineEnd text checks so multiline declarations,
nested struct-body calls, and var initializers are handled by their enclosing
declaration rather than source-line formatting.
- Around line 145-151: Update the import-buffer loop around all_imports so the
prev_class transition check only applies to the sorted-import section, not
aliases that follow it. Preserve the single separator inserted when entering the
alias section and keep aliases in declaration order without adding additional
blank lines for class changes.
- Around line 436-448: Keep the dup buffer allocated for the entire
processSource flow instead of freeing it immediately after ast_scan.analyze;
remove the allocator.free(dup) call while preserving cleanup on analysis
failure, and free dup only after all uses of analysis.imports and
analysis.aliases have completed.
---
Nitpick comments:
In `@src/ast_scan.zig`:
- Around line 147-173: Optimize lineBlockEnd by replacing the per-line scans
across imports and aliases with a merge-style walk over their source-ordered
spans, maintaining advancing indices and skipping spans that end before the
current line. Preserve the existing handling of blank/comment lines and return
the first uncovered line position.
- Around line 255-260: Update stringPath to unescape the string literal body
before returning it, using std.zig.string_literal.parseAlloc or the project’s
established equivalent; ensure escaped paths such as \x2f are decoded so
classify and banned-prefix checks receive the actual path text, while preserving
null handling for non-string or invalid literals.
In `@src/zsort.zig`:
- Around line 54-59: Update the processSource/analyze/hasBannedPatterns flow to
parse each file only once: reuse the parsed Ast or ImportCall list produced by
analyze when checking banned patterns, and avoid duplicating the source again.
Propagate the worker’s existing [:0]u8 buffer through processSource as [:0]const
u8, removing redundant dupeZ allocations while preserving current analysis and
validation behavior.
- Around line 20-40: Remove the production wrappers collectImports and
findImportBlockEnd from src/zsort.zig. Update the corresponding usages in
zsort_test.zig to call ast_scan.analyze directly, preserving the existing
allocator setup, source duplication, import collection, block_end handling, and
fallback behavior required by the tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59b48592-1e46-40c1-978f-7422f0463769
📒 Files selected for processing (5)
README.mdsrc/ast_scan.zigsrc/compat.zigsrc/zsort.zigsrc/zsort_test.zig
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/ast_scan.zig (2)
62-78: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve comments at the start of the file.
At Line 69, the trim set does not include
\n, so a blank line does not satisfy the intended empty-line check. At Line 72, a comment that starts at offset0is discarded by returningnull.processSourceusescomment_startto remove stray imports, so these comments can remain detached from their import.Proposed fix
- const prev_trimmed = std.mem.trimStart(u8, source[prev_start..prev_end], " \t\r"); + const prev_trimmed = std.mem.trim(u8, source[prev_start..prev_end], " \t\r\n"); if (prev_trimmed.len == 0 or std.mem.startsWith(u8, prev_trimmed, "//")) { comment_start = prev_start; - if (prev_start == 0) return null; + if (prev_start == 0) break; back = prev_start - 1;🤖 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 `@src/ast_scan.zig` around lines 62 - 78, Update findCommentStart to trim newline characters when detecting blank preceding lines, and preserve a comment beginning at source offset 0 by returning its start instead of null. Keep processSource’s existing comment_start handling intact so leading comments remain attached to the related import.
302-306: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDecode escaped import paths before classification and banned-prefix checks.
stringPathcurrently returns raw token text, so@import("\x66oo/bar")is analyzed as\x66oo/barinstead offoo/bar. Decode the literal intoAnalysis-owned storage and use the decoded path forImport.pathandImportCall.path. Add coverage for classification, sorting, and banned-prefix matching.🤖 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 `@src/ast_scan.zig` around lines 302 - 306, Update stringPath to decode escaped string-literal contents into Analysis-owned storage before returning the path, rather than slicing raw token text. Propagate the decoded value through Import.path and ImportCall.path so classification, sorting, and banned-prefix checks operate on the decoded path; add coverage for escaped imports across those behaviors.
♻️ Duplicate comments (1)
src/ast_scan.zig (1)
333-345: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBound the semicolon lookup to the adjacent token.
The Line 342 guard only stops at a later line. If malformed input has
const a =@import("a") const b = value;on one line,spanEndreachesb's semicolon and absorbs the later declaration. Check only the token immediately afterlast_node_tokenfor a semicolon. Otherwise, end the span atlast_node_token.🤖 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 `@src/ast_scan.zig` around lines 333 - 345, The semicolon search in spanEnd must only inspect the token immediately following last_node_token, preventing later declarations on the same line from extending the span. Replace the loop-based lookup with adjacent-token validation: return that token’s line end when it is a semicolon; otherwise end the span at last_node_token.
🧹 Nitpick comments (2)
src/zsort_test.zig (2)
601-606: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the alias test exercise a reorder.
The input already places
const barbeforeconst Debug. Therefore,bar_pos < debug_poscan pass without rearranging the alias. Start with the alias before an import, then assert that all imports precede the alias and that the alias remains beforeconst rest. (raw.githubusercontent.com)Proposed test update
const source = + \\const std = `@import`("std"); + \\const Debug = std.debug; \\const bar = `@import`("bar"); - \\const Debug = std.debug; \\ \\const rest = 1; @@ + const std_pos = std.mem.indexOf(u8, result, "const std") orelse return error.TestUnexpectedResult; const bar_pos = std.mem.indexOf(u8, result, "const bar") orelse return error.TestUnexpectedResult; const debug_pos = std.mem.indexOf(u8, result, "const Debug = std.debug;") orelse return error.TestUnexpectedResult; + const rest_pos = std.mem.indexOf(u8, result, "const rest = 1;") orelse return error.TestUnexpectedResult; + try std.testing.expect(std_pos < bar_pos); try std.testing.expect(bar_pos < debug_pos); + try std.testing.expect(debug_pos < rest_pos);🤖 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 `@src/zsort_test.zig` around lines 601 - 606, Update the alias-order test around collectImportsForTest, collectAliasesForTest, and buildSortedImportText so the input places the alias before at least one import, forcing a reorder. Assert that every import appears before the alias and that the alias still appears before const rest, rather than only checking the original bar/debug ordering.
580-591: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert immediate comment attachment.
Both tests only require the comment to occur before the declaration. A misplaced comment can satisfy that condition. Assert that the output immediately before each declaration ends with its expected comment line. This verifies attachment during sorting. (raw.githubusercontent.com)
Proposed test update
- const comment_pos = std.mem.indexOf(u8, result, "// std comment") orelse return error.TestUnexpectedResult; - try std.testing.expect(comment_pos < std_pos); + try std.testing.expect(std.mem.endsWith(u8, result[0..std_pos], "// std comment\n")); @@ - const comment_pos = std.mem.indexOf(u8, result, "// Debug alias") orelse return error.TestUnexpectedResult; const debug_pos = std.mem.indexOf(u8, result, "const Debug") orelse return error.TestUnexpectedResult; - try std.testing.expect(comment_pos < debug_pos); + try std.testing.expect(std.mem.endsWith(u8, result[0..debug_pos], "// Debug alias\n"));Also applies to: 787-805
🤖 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 `@src/zsort_test.zig` around lines 580 - 591, Strengthen the assertions in the test around buildSortedImportText, and apply the same change to the corresponding test near the second referenced block: verify the text immediately preceding each declaration ends with its expected comment line, rather than only checking comment_pos < declaration_pos. Preserve the existing ordering assertions while adding exact comment attachment checks for both std and bar declarations.
🤖 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.
Outside diff comments:
In `@src/ast_scan.zig`:
- Around line 62-78: Update findCommentStart to trim newline characters when
detecting blank preceding lines, and preserve a comment beginning at source
offset 0 by returning its start instead of null. Keep processSource’s existing
comment_start handling intact so leading comments remain attached to the related
import.
- Around line 302-306: Update stringPath to decode escaped string-literal
contents into Analysis-owned storage before returning the path, rather than
slicing raw token text. Propagate the decoded value through Import.path and
ImportCall.path so classification, sorting, and banned-prefix checks operate on
the decoded path; add coverage for escaped imports across those behaviors.
---
Duplicate comments:
In `@src/ast_scan.zig`:
- Around line 333-345: The semicolon search in spanEnd must only inspect the
token immediately following last_node_token, preventing later declarations on
the same line from extending the span. Replace the loop-based lookup with
adjacent-token validation: return that token’s line end when it is a semicolon;
otherwise end the span at last_node_token.
---
Nitpick comments:
In `@src/zsort_test.zig`:
- Around line 601-606: Update the alias-order test around collectImportsForTest,
collectAliasesForTest, and buildSortedImportText so the input places the alias
before at least one import, forcing a reorder. Assert that every import appears
before the alias and that the alias still appears before const rest, rather than
only checking the original bar/debug ordering.
- Around line 580-591: Strengthen the assertions in the test around
buildSortedImportText, and apply the same change to the corresponding test near
the second referenced block: verify the text immediately preceding each
declaration ends with its expected comment line, rather than only checking
comment_pos < declaration_pos. Preserve the existing ordering assertions while
adding exact comment attachment checks for both std and bar declarations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e5692a6-95b1-47be-9d78-178565382279
📒 Files selected for processing (3)
src/ast_scan.zigsrc/zsort.zigsrc/zsort_test.zig
🚧 Files skipped from review as they are similar to previous changes (1)
- src/zsort.zig
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
zsort fixorganizes explicitly typed imports like regular imports.