Fix Zig 0.16 quality gates#2
Merged
Merged
Conversation
WHY: fork and upstream did independent Zig 0.16 migrations (1 ahead / 1 behind, no auto-merge). Upstream's later update has better core mechanics; the fork's ziglint quality gate and arg-parser rigor must survive the sync. WHAT: - Merge upstream/main (52b8c43). src/Walk.zig + src/main.zig resolved to upstream (correct absolute-path realPathFileAbsoluteAlloc handling, init.arena Juicy Main); build.zig.zon + build_readme.zig resolved to ours (pinned EugOT/ziglint dependency; ParsedArgs with named ParseError set + strict positional validation + tests). - Restore the ziglint integration upstream removed: ziglint import, `lint` step via addLint (exit 0, strict) wired into `zig build test`, covering src/, build.zig, build_readme.zig. - Fix all 10 lint findings on the merged tree: Z023 io-before-other on six *ArenaAllocator fns + call sites (upstream had regressed the order); Z011 indexOf -> find (2); Z011 StringArrayHashMapUnmanaged -> std.array_hash_map.String (2, verified present in 0.16.0 stable stdlib). IMPACT: fork is upstream + ziglint gate + arg-parser tests; default `zig build test` enforces fmt, unit tests, build_readme invariants, and strict self-applied ziglint. VALIDATION: zig build test 11/11 steps, 5/5 tests; zig build lint 0 findings; zig build run -- --help works; zig fmt clean.
|
Warning Review limit reached
More reviews will be available in 10 minutes and 29 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThis PR performs a comprehensive migration to Zig 0.16 and integrates a new ziglint linting tool. The changes span build configuration, core module refactoring with resource cleanup, CLI entry point updates, and a complete linting implementation with semantic type analysis and configuration support. ChangesZig 0.16 Migration with ZigLint Integration
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 55447798ec
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
Updates zigdoc (and its vendored ziglint gate) to be compatible with Zig 0.16 by adopting std.Io APIs, adding/adjusting cleanup paths for global state, and wiring additional tests into the build to keep quality gates green.
Changes:
- Vendor/migrate
ziglintsources to Zig 0.16-stylestd.Io+ semantic-analysis helpers (ModuleGraph/TypeResolver/doc tests). - Improve
zigdoc’s global cleanup behavior inWalkand register symbol-resolution tests inbuild.zig. - Add Zig 0.16 build runner support and update project templates / tooling defaults for Zig 0.16.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| zig-pkg/ziglint-0.5.2-t0bwLz2XBQCgnA0PTY0KM9YgrmdjLU7wwjXablNijOuq/src/TypeResolver.zig | Adds semantic type resolution for ziglint rules; includes unit tests. |
| zig-pkg/ziglint-0.5.2-t0bwLz2XBQCgnA0PTY0KM9YgrmdjLU7wwjXablNijOuq/src/rules.zig | Defines lint rule IDs, config struct generation, and formatted diagnostic messages. |
| zig-pkg/ziglint-0.5.2-t0bwLz2XBQCgnA0PTY0KM9YgrmdjLU7wwjXablNijOuq/src/ModuleGraph.zig | Implements a module import graph loader for semantic analysis. |
| zig-pkg/ziglint-0.5.2-t0bwLz2XBQCgnA0PTY0KM9YgrmdjLU7wwjXablNijOuq/src/main.zig | Updates ziglint CLI entrypoint to Zig 0.16 std.process.Init + std.Io patterns. |
| zig-pkg/ziglint-0.5.2-t0bwLz2XBQCgnA0PTY0KM9YgrmdjLU7wwjXablNijOuq/src/doc_tests.zig | Adds executable doc tests runner for rule docs in docs/rules/. |
| zig-pkg/ziglint-0.5.2-t0bwLz2XBQCgnA0PTY0KM9YgrmdjLU7wwjXablNijOuq/src/doc_comments.zig | Adds doc-comment extraction utility for semantic analysis. |
| zig-pkg/ziglint-0.5.2-t0bwLz2XBQCgnA0PTY0KM9YgrmdjLU7wwjXablNijOuq/src/Config.zig | Adds .ziglint.zon config loading + per-rule enablement settings. |
| zig-pkg/ziglint-0.5.2-t0bwLz2XBQCgnA0PTY0KM9YgrmdjLU7wwjXablNijOuq/build.zig.zon | Declares ziglint package metadata for the vendored dependency. |
| zig-pkg/ziglint-0.5.2-t0bwLz2XBQCgnA0PTY0KM9YgrmdjLU7wwjXablNijOuq/build.zig | Adds build/test/lint steps for ziglint itself, including self-lint smoke run. |
| src/Walk.zig | Updates Walk globals/maps for Zig 0.16 containers; adds explicit deinit + scope cleanup. |
| src/test_symbol_resolution.zig | Converts tests to arena-based setup and uses Walk.deinit() cleanup. |
| src/templates/main.zig.template | Updates template app entrypoint to Zig 0.16 std.process.Init + std.Io. |
| src/templates/build.zig.zon.template | Updates template zon for Zig 0.16 minimum version; removes ziglint dependency. |
| src/templates/build.zig.template | Removes ziglint integration from template build file. |
| src/templates/AGENTS.md.template | Updates docs examples to Zig 0.16 std.Io writer patterns. |
| src/main.zig | Updates zigdoc CLI to new args iteration + Zig-not-found diagnostic + 0.16 runner selection. |
| src/build_runner_0.16.zig | Adds Zig 0.16 build runner implementation for module import dumping. |
| mise.toml | Pins Zig toolchain version to 0.16. |
| build.zig | Registers symbol-resolution tests in the build’s test step. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Code Review Roast 🔥Verdict: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)
🏆 Best part: The arena-based cleanup in 💀 Worst part: Line 152 in 📊 Overall: Like a car that runs on both gasoline and hope — the Zig 0.16 migration spirit is there, but you'll need a tow truck when the JSON parser meets backslashes. Files Reviewed (20 files)
Reviewed by step-3.7-flash-20260528 · 464,351 tokens |
…d-runner failures, plug initProject leak WHY: Review on #2 (Codex P2 ×2, Copilot ×1) flagged correctness + resource issues that let malformed input or build failures masquerade as missing docs. WHAT: - build_runner_0.16.zig: encode module/import names and root paths via std.json.Stringify.encodeJsonString instead of raw "{s}" interpolation, so Windows backslash paths (C:\...) no longer emit invalid JSON that breaks parseBuildOutput. - main.zig processBuildZig: return error.BuildRunnerFailed (with stderr) on non-zero build-runner exit instead of silently continuing with zero modules. - main.zig initProject: cap run() stdout/stderr at .limited(1MiB) and free the caller-owned buffers (was leaking on success path). - Walk.zig: migrate StringArrayHashMapUnmanaged -> array_hash_map.String (0.16). - build.zig.zon: repoint ziglint dep ref fix/0.16-build-compat -> main. IMPACT: zigdoc module discovery on Windows and on failing builds; no public API change. VALIDATION: mise x zig@0.16.0 zig build test --summary all => 13/13 steps, 15/15 tests, zig fmt --check clean, ziglint self-lint gate clean.
Owner
Author
|
Addressed all review picks in 8b72ad4 (signed):
Validation: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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/build_runner_0.16.zig`:
- Around line 29-43: Guard the CLI positional indexing by verifying args.len
before using args[arg_idx]; specifically ensure there are at least 6 entries
(program name + five positional args) prior to reading zig_exe, zig_lib_dir,
build_root, cache_root, and global_cache_root. Add a check right after const
args = try init.args.toSlice(arena); e.g. assert(args.len >= 6) or return a
clear error if too few arguments, and use that guarded path before accessing
args[arg_idx] (symbols: init.args, args, arg_idx, zig_exe, zig_lib_dir,
build_root, cache_root, global_cache_root).
- Around line 116-126: Change the managed hashmaps to allocator-explicit
unmanaged variants: replace the std.StringHashMap.init(our_allocator) usage for
all_modules with std.StringHashMapUnmanaged = .empty and likewise replace
std.AutoHashMap.init(our_allocator) for visited_steps with
std.AutoHashMapUnmanaged = .empty, add defer all_modules.deinit(our_allocator)
and defer visited_steps.deinit(our_allocator), and update collectStepModules
signature to accept the unmanaged map types; also update calls inside
collectStepModules to use modules.put(our_allocator, ...) and
visited.put(our_allocator, ...) instead of put(...) so the allocator is passed
explicitly.
In
`@zig-pkg/ziglint-0.5.2-t0bwLz2XBQCgnA0PTY0KM9YgrmdjLU7wwjXablNijOuq/src/Config.zig`:
- Around line 10-15: Config currently owns heap allocations created by
parseConfigSource (specifically Config.paths and each path string) but provides
no deinitializer; add a public deinit method (e.g., pub fn deinit(self: *Config,
allocator: *std.mem.Allocator) void) that frees each path entry and then the
paths array and resets fields to empty/default. Update parseConfigSource usage
docs or signature so callers know to call Config.deinit with the same allocator
that was used to allocate, or store the allocator in Config at creation time so
deinit can use it; implement freeing by iterating over self.paths, calling
allocator.free for each path slice, then allocator.free for the paths slice
itself and setting self.paths = &.{} to avoid double-free/leak. Ensure symbols
referenced: Config, Config.paths, parseConfigSource.
In
`@zig-pkg/ziglint-0.5.2-t0bwLz2XBQCgnA0PTY0KM9YgrmdjLU7wwjXablNijOuq/src/doc_comments.zig`:
- Around line 26-28: The loop that scans tokens for doc comments currently only
skips the .keyword_pub token (the variable tag in doc_comments.zig), which
causes it to stop when encountering multi-qualifier declarations like "pub
inline fn"; update the logic to treat a set of declaration qualifiers as
skippable instead of just .keyword_pub. Add or use a helper (e.g.,
isDeclarationQualifier(tag)) and include qualifiers such as .keyword_inline,
.keyword_export, .keyword_comptime, etc., so the loop continues past any
declaration qualifiers before checking the doc-comment token.
In
`@zig-pkg/ziglint-0.5.2-t0bwLz2XBQCgnA0PTY0KM9YgrmdjLU7wwjXablNijOuq/src/doc_tests.zig`:
- Around line 127-156: The test currently only ensures expected rules exist
(using linter.diagnosticCount and doc_test.expected_rules) but ignores extra
diagnostics when expected_rules is non-empty; update doc_tests.zig to also
reject any unexpected diagnostics by comparing linter.diagnostics.items against
doc_test.expected_rules (e.g., iterate linter.diagnostics.items and for each
diagnostic check membership in doc_test.expected_rules by rule.code() or
similar) and if any diagnostic is not listed, print the same diagnostic/Code
context and return error.UnexpectedDiagnostic; keep the existing no-expectations
branch for the empty case.
In
`@zig-pkg/ziglint-0.5.2-t0bwLz2XBQCgnA0PTY0KM9YgrmdjLU7wwjXablNijOuq/src/main.zig`:
- Around line 88-94: findProjectRoot returns a heap allocation that is never
freed in the main loop; after calling findProjectRoot (project_root) and after
lintPath uses it, free that allocation (e.g., call
std.heap.page_allocator.free(project_root) or the allocator used inside
findProjectRoot) — best is to add a defer or an explicit free immediately after
the lintPath call to release project_root in the loop (refer to findProjectRoot,
project_root, and the lintPath(...) call to locate where to free).
In
`@zig-pkg/ziglint-0.5.2-t0bwLz2XBQCgnA0PTY0KM9YgrmdjLU7wwjXablNijOuq/src/rules.zig`:
- Line 42: Rename the PascalCase function identifiers to camelCase: change
ConfigType to configType and RuleConfig to ruleConfig; update their declarations
(e.g., fn ConfigType(comptime self: Rule) type { ... } and the RuleConfig
function at the other occurrence) and update all call sites and references to
use the new names so the comptime signature (comptime self: Rule) and return
types remain unchanged.
In
`@zig-pkg/ziglint-0.5.2-t0bwLz2XBQCgnA0PTY0KM9YgrmdjLU7wwjXablNijOuq/src/TypeResolver.zig`:
- Around line 523-528: The alias resolution in findMethodInFileAsStruct can
recurse forever for cyclic aliases; modify it to track visited aliases (e.g., a
HashSet or Zig.Set of identifier strings) or add a recursion depth limit before
calling itself when init_tag == .identifier so you check if alias_target (from
tree.tokenSlice(tree.nodeMainToken(init_node))) is already visited or depth
exceeded and bail out (return null or an error) instead of recursing; propagate
the visited set or depth counter through recursive calls of
findMethodInFileAsStruct so subsequent calls honor the check.
- Around line 818-822: The .user_type match arm incorrectly returns a .std_type
value, conflating user namespaces with stdlib; change the arm to return the
appropriate user-type variant instead of .std_type (i.e., return the user_type
representation that your resolver uses), and if necessary replace or wrap the
call to self.buildStdTypePath with a dedicated builder (e.g., buildUserTypePath)
so the returned value matches the .user_type variant expected by downstream
semantic resolution; update the .user_type arm (and any helper used there) to
construct and return the correct user-type payload rather than .{ .std_type =
... }.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 692ded1d-4b6e-4353-914e-b777a9410cb2
📒 Files selected for processing (21)
build.zigbuild.zig.zonmise.tomlsrc/Walk.zigsrc/build_runner_0.16.zigsrc/main.zigsrc/templates/AGENTS.md.templatesrc/templates/build.zig.templatesrc/templates/build.zig.zon.templatesrc/templates/main.zig.templatesrc/test_symbol_resolution.zigzig-pkg/ziglint-0.5.2-t0bwLz2XBQCgnA0PTY0KM9YgrmdjLU7wwjXablNijOuq/build.zigzig-pkg/ziglint-0.5.2-t0bwLz2XBQCgnA0PTY0KM9YgrmdjLU7wwjXablNijOuq/build.zig.zonzig-pkg/ziglint-0.5.2-t0bwLz2XBQCgnA0PTY0KM9YgrmdjLU7wwjXablNijOuq/src/Config.zigzig-pkg/ziglint-0.5.2-t0bwLz2XBQCgnA0PTY0KM9YgrmdjLU7wwjXablNijOuq/src/Linter.zigzig-pkg/ziglint-0.5.2-t0bwLz2XBQCgnA0PTY0KM9YgrmdjLU7wwjXablNijOuq/src/ModuleGraph.zigzig-pkg/ziglint-0.5.2-t0bwLz2XBQCgnA0PTY0KM9YgrmdjLU7wwjXablNijOuq/src/TypeResolver.zigzig-pkg/ziglint-0.5.2-t0bwLz2XBQCgnA0PTY0KM9YgrmdjLU7wwjXablNijOuq/src/doc_comments.zigzig-pkg/ziglint-0.5.2-t0bwLz2XBQCgnA0PTY0KM9YgrmdjLU7wwjXablNijOuq/src/doc_tests.zigzig-pkg/ziglint-0.5.2-t0bwLz2XBQCgnA0PTY0KM9YgrmdjLU7wwjXablNijOuq/src/main.zigzig-pkg/ziglint-0.5.2-t0bwLz2XBQCgnA0PTY0KM9YgrmdjLU7wwjXablNijOuq/src/rules.zig
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
EugOT/dotfiles(manual)
💤 Files with no reviewable changes (1)
- src/templates/build.zig.template
WHY: CodeRabbit threads on PR #2 flagged 7 findings inside the vendored ziglint copy; the fixes landed upstream (EugOT/ziglint#4) — patching the package cache in-place would desync it from its content hash. WHAT: build.zig.zon repinned to main@a2fa56a (hash t0bwLzCwBQA...); vendored zig-pkg snapshot refreshed, stale t0bwLz2XBQC... removed. Upstream brings: Config.deinit + pub Rule re-export, doc-comment qualifier scan (pub inline fn), strict doc-test gate, alias-recursion cap, user_type no longer coerced to std_type, findProjectRoot caller-allocator fix. IMPACT: zigdoc lint gate now runs the hardened ziglint. VALIDATION: mise x zig@0.16.0 zig build test --summary all => all steps green (tests, zig fmt --check, ziglint self-lint via new pin).
Owner
Author
|
All 10 review threads addressed across 2183881 + 28b1c9f: zigdoc source (fixed in 2183881):
Vendored Gate: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Upstream PR review
Verification