fix: reduce MODICUM lint false positives#9
Conversation
Teach the linter to walk switch nodes, preserve syntactic imported-type fallbacks, ignore exported function names for Z001, and avoid Z024 reports for comments/raw strings/string literals. This fixes stale false positives seen when validating MODICUM against the local Zig 0.16 QC stack. Validation: git diff --check; zig fmt --check src/Linter.zig src/main.zig; zig build --summary failures; zig build test --summary failures --test-timeout 30s.
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
🚫 Excluded labels (none allowed) (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR reduces MODICUM-facing false positives by improving AST traversal (so nested declarations inside switch/case bodies are visible), preserving syntactic fallbacks when semantic type resolution is inconclusive, and refining several heuristics for name/type/line-length related rules. It also adds targeted tests and updates Z024 documentation to reflect the new behavior around literal/comment lines.
Changes:
- Extend AST child-walking to include
switchandswitch_case*nodes and refine public/imported type classification logic. - Reduce false positives in rules around exported function names, container
Self/enclosing type names, callable struct-init call arguments, and deinit poisoning patterns. - Adjust Z024 to skip comment-only, raw-string, and string-literal lines, with expanded tests and docs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/main.zig | Skip linting non-.zig file paths and add a focused unit test for the new path filter. |
| src/Linter.zig | Main linting behavior changes: improved AST traversal, semantic/syntactic fallback handling, and multiple false-positive reductions with added tests. |
| docs/rules/Z024.md | Document updated Z024 semantics (skipping comments/raw strings/string-literal lines) and add an example rationale. |
Comments suppressed due to low confidence (1)
src/Linter.zig:385
- ChildList was expanded to 32 items, but append() still hard-caps at
self.len < 8, so only the first 8 children are tracked. This undermines the new switch/switch-case traversal and can leave parent_map incomplete for nodes with >8 children.
fn append(self: *ChildList, item: Ast.Node.Index) void {
if (self.len < 8) {
self.items[self.len] = item;
self.len += 1;
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn isDestroySelfCall(self: *Linter, node: Ast.Node.Index, param_name: []const u8) bool { | ||
| const source = self.getNodeSource(node); | ||
| if (std.mem.indexOf(u8, source, ".destroy(") == null) return false; | ||
| if (std.mem.indexOf(u8, source, param_name) == null) return false; | ||
| return true; | ||
| } |
| fn hasStringLiteral(line: []const u8) bool { | ||
| var escaped = false; | ||
| for (line) |c| { | ||
| if (escaped) { | ||
| escaped = false; | ||
| continue; | ||
| } | ||
| if (c == '\\') { | ||
| escaped = true; | ||
| continue; | ||
| } | ||
| if (c == '"') return true; | ||
| } | ||
| return false; | ||
| } |
| fn typeDeclHasCallableMembers(self: *Linter, type_node: Ast.Node.Index) bool { | ||
| const type_name = self.getTypeNodeName(type_node) orelse return false; | ||
|
|
||
| for (0..self.tree.nodes.len) |i| { | ||
| const node: Ast.Node.Index = @enumFromInt(i); |
![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: 11f96a156e
ℹ️ 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".
| items: [32]Ast.Node.Index = undefined, | ||
| len: usize = 0, | ||
|
|
||
| fn append(self: *ChildList, item: Ast.Node.Index) void { |
There was a problem hiding this comment.
Switch traversal added in this patch appends the condition plus every case through ChildList, but append still stops at self.len < 8. A switch with 8+ arms (or a case with many values) silently drops later children, so parent_map/visitNode won't reach declarations in those arms and the MODICUM false positives can still occur past the seventh arm; use the actual array length or handle switch cases as an unbounded child list.
Useful? React with 👍 / 👎.
| if (std.mem.indexOf(u8, source, ".destroy(") == null) return false; | ||
| if (std.mem.indexOf(u8, source, param_name) == null) return false; | ||
| return true; |
There was a problem hiding this comment.
This exemption accepts any final statement whose source contains .destroy( and the receiver name as a substring. In a deinit such as self.* = undefined; allocator.destroy(self.child); (or allocator.destroy(not_self)), Z030 is suppressed even though the final call is not destroying the receiver and may use the receiver after poisoning; parse the call and require the destroy argument to be exactly the receiver parameter.
Useful? React with 👍 / 👎.
| var buf: [2]Ast.Node.Index = undefined; | ||
| const struct_init = self.tree.fullStructInit(&buf, node) orelse return; | ||
| const type_node = struct_init.ast.type_expr.unwrap() orelse return; | ||
| if (in_call_arg and self.typeDeclHasCallableMembers(type_node)) return; |
There was a problem hiding this comment.
With this early return, bar(Foo{}) is no longer reported whenever Foo declares any function, even if bar has a concrete Foo parameter and bar(.{}) is valid. The callable-member exception only applies to generic/interface-style call sites, so suppressing based solely on the type declaration creates broad Z010 false negatives for ordinary structs with methods.
Useful? React with 👍 / 👎.
Code Review Roast 🔥Verdict: 5 Issues Found | Recommendation: Merge was already completed; issues remain unresolved Overview
Issue Details (click to expand)
🏆 Best part: Oh wait, the switch-case walking logic is actually well-structured. I had my flamethrower warmed up but that part's clean. 💀 Worst part: The 📊 Overall: Like serving a cake with missing ingredients—good intentions but the Files Reviewed (3 files)
Reviewed by laguna-m.1-20260312:free · Input: 822.4K · Output: 11.2K · Cached: 2M |
Summary
Self/enclosing container types, exported function names, callable struct init arguments, and comment/raw-string/string-literal line-length cases.Validation
git diff --checkzig fmt --check src/Linter.zig src/main.zigzig build --summary failureszig build test --summary failures --test-timeout 30sMODICUM integration context
This is the linter-side fix needed before MODICUM can replace its stale pinned
zig-pkg/ziglint-0.5.2dependency with the updated EugOT/ziglint mainline.