ABI Mapper Improvements#67
Conversation
…ve a proper document format instead of just a list of strings
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces the flat ChangesABI Mapper Rework
CI Package Cache Key Automation
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches🧪 Generate unit tests (beta)
|
…de ashet.abi, adds broken links for code references.
…resolution of FQNs
…om context + fqn inside the docs
…pes are now also rendered as locally qualified names to simplify display.
…ff the declaration.
…t for the abi-parser.
…l problems identified with the tool
…to get from the unpatched abi file to a working solution
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (4)
src/tools/abi-mapper/tests/testsuite.zig (1)
1-13: ⚡ Quick winAdd a UID database invariant test module to this test aggregator.
Since
src/abi/db/abi-id-db.jsonis now a committed stability contract, include one test module here that validates at least uniqueness offqnanduid(and optionally deterministic load/save roundtrip). This catches accidental mapping drift early in CI.src/tools/abi-mapper/tests/doc_ref_emission.zig (1)
59-62: ⚡ Quick winFail fast when semantic diagnostics are emitted on successful analysis.
errorsis collected but never asserted on success, so tests may pass while analyzer diagnostics are still produced.Suggested change
- return abi_parser.sema.analyze(allocator, ast_document, null, &errors); + const analyzed = try abi_parser.sema.analyze(allocator, ast_document, null, &errors); + try std.testing.expectEqual(`@as`(usize, 0), errors.items.len); + return analyzed;src/tools/abi-mapper/tests/optional_type_handling.zig (1)
30-33: ⚡ Quick winAssert optional type preservation explicitly in the positive-path test.
This test currently validates only the native input name. Add a type-level assertion so it fails if
?Callbackis flattened or rewritten incorrectly.src/tools/abi-mapper/tests/syscall_output_count.zig (1)
4-14: ⚡ Quick winExtract
parse_and_analyzeinto a shared test utility.This helper is duplicated across the new test modules, which increases drift risk when parser/analyzer call signatures change.
♻️ Proposed refactor
+// src/tools/abi-mapper/tests/test_utils.zig +const std = `@import`("std"); +const abi_parser = `@import`("abi-parser"); + +pub fn parse_and_analyze(allocator: std.mem.Allocator, source: []const u8) !abi_parser.model.Document { + var tokenizer: abi_parser.syntax.Tokenizer = .init(source, "test"); + var parser: abi_parser.syntax.Parser = .{ + .allocator = allocator, + .core = .init(&tokenizer), + }; + const ast = try parser.accept_document(); + var errors: std.ArrayList(abi_parser.sema.AnalysisError) = .empty; + defer errors.deinit(allocator); + return abi_parser.sema.analyze(allocator, ast, null, &errors); +}-const abi_parser = `@import`("abi-parser"); +const abi_parser = `@import`("abi-parser"); +const test_utils = `@import`("test_utils.zig"); -fn parse_and_analyze(allocator: std.mem.Allocator, source: []const u8) !abi_parser.model.Document { - ... -} - -const doc = try parse_and_analyze(allocator, source); +const doc = try test_utils.parse_and_analyze(allocator, source);
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 55966225-af50-47c2-8051-621b74724b09
📒 Files selected for processing (35)
build.zigbuild.zig.zonsrc/abi/build.zigsrc/abi/db/abi-id-db.jsonsrc/abi/src/ashet.abisrc/abi/tests/escaping.abisrc/abi/utility/render_zig_code.zigsrc/tools/abi-mapper/build.zigsrc/tools/abi-mapper/rework/abi-doc-format.mdsrc/tools/abi-mapper/rework/findings.mdsrc/tools/abi-mapper/src/abi-parser.zigsrc/tools/abi-mapper/src/doc_comment.zigsrc/tools/abi-mapper/src/model.zigsrc/tools/abi-mapper/src/sema.zigsrc/tools/abi-mapper/src/syntax.zigsrc/tools/abi-mapper/src/uid_db.zigsrc/tools/abi-mapper/tests/bitstruct_array_field.zigsrc/tools/abi-mapper/tests/constant_ordering.zigsrc/tools/abi-mapper/tests/digit_separator.zigsrc/tools/abi-mapper/tests/doc_parser.zigsrc/tools/abi-mapper/tests/doc_ref_emission.abisrc/tools/abi-mapper/tests/doc_ref_emission.zigsrc/tools/abi-mapper/tests/doc_ref_resolution.abisrc/tools/abi-mapper/tests/doc_ref_resolution.zigsrc/tools/abi-mapper/tests/fnptr_named_params.zigsrc/tools/abi-mapper/tests/nonstandard_backing_type.zigsrc/tools/abi-mapper/tests/optional_type_handling.zigsrc/tools/abi-mapper/tests/stress/ashet-1.0.abisrc/tools/abi-mapper/tests/stress/ashet-1.0.abi.patchsrc/tools/abi-mapper/tests/syscall_output_count.zigsrc/tools/abi-mapper/tests/testsuite.zigsrc/tools/abi-mapper/tests/unknown_named_type.zigsrc/website/build.zigsrc/website/src/syscalls-gen.zigsrc/website/www/theme.css
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 (1)
src/tools/abi-mapper/tests/doc_parser.zig (1)
283-304:⚠️ Potential issue | 🟡 MinorAdd
LEARNto the admonition kinds test cases.The
Section.Kindenum inmodel.zigincludeslearnas a valid variant, andparse_admonitionindoc_comment.zigalready handles it. The test loop should cover all admonition kinds for completeness. Add.{ .tag = "LEARN", .kind = .learn }to the cases array.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2840e6cf-ebf5-4e18-85d8-58c5698e9e54
📒 Files selected for processing (9)
src/os/build.zigsrc/tools/abi-mapper/src/doc_comment.zigsrc/tools/abi-mapper/src/sema.zigsrc/tools/abi-mapper/src/syntax.zigsrc/tools/abi-mapper/tests/digit_separator.zigsrc/tools/abi-mapper/tests/doc_parser.zigsrc/tools/abi-mapper/tests/doc_ref_resolution.zigsrc/tools/abi-mapper/tests/stress/ashet-1.0.abisrc/website/src/syscalls-gen.zig
🚧 Files skipped from review as they are similar to previous changes (4)
- src/tools/abi-mapper/src/syntax.zig
- src/tools/abi-mapper/src/doc_comment.zig
- src/website/src/syscalls-gen.zig
- src/tools/abi-mapper/src/sema.zig
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 927e7989-e470-4059-a999-417dff21d138
📒 Files selected for processing (4)
.github/workflows/build.yml.github/workflows/pages.yml.github/workflows/smoketest.ymlscripts/zig_package_cache_key.py
ikskuh
force-pushed
the
work/abi-mapper-improvements
branch
from
eb4cc10 to
11e553b
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
scripts/zig_package_cache_key.py (1)
41-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInline trailing comments on
.url/.hashentries still hard-fail parsing.At Line 42,
assert value.endswith('",')rejects valid Zig like.hash = "...", // comment, which can break CI cache-key generation.Proposed fix
import hashlib import pathlib +import re import subprocess import sys +ENTRY_RE = re.compile(r'^(\.url|\.hash)\s*=\s*"(.*)"\s*,') + @@ - for key in (".url", ".hash"): - prefix = f"{key} =" - if not stripped.startswith(prefix): - continue - - _, value = stripped.split("=", 1) - value = value.strip() - - assert value.startswith('"'), f"{path}: malformed {key} entry: {line!r}" - assert value.endswith('",'), f"{path}: malformed {key} entry: {line!r}" - - entries.add(f"{key}={value}") + m = ENTRY_RE.match(stripped) + if not m: + continue + key, raw_value = m.groups() + entries.add(f'{key}="{raw_value}",')
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 86d2545e-76e9-4f37-a4fc-5be2a0677cfd
📒 Files selected for processing (5)
.gitattributes.github/workflows/build.yml.github/workflows/pages.yml.github/workflows/smoketest.ymlscripts/zig_package_cache_key.py
✅ Files skipped from review due to trivial changes (1)
- .gitattributes
ikskuh
force-pushed
the
work/abi-mapper-improvements
branch
from
3b2a23e to
8fb6c61
Compare
ikskuh
force-pushed
the
work/abi-mapper-improvements
branch
from
8fb6c61 to
d765ff7
Compare
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Improvements