Conversation
Contributor
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (12)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughThis PR introduces a complete textual parser for Arena IR that transforms text input into arena IR structures through a two-stage process (raw parsing to intermediate representation, then IR building to arena references), along with a comprehensive validation module for checking value scope integrity and use-chain consistency. The existing parser module is refactored to separate raw parsing concerns. Function type representation is migrated from Changes
Sequence Diagram(s)sequenceDiagram
participant Input as Textual Input
participant RawParser as Raw Parser
participant Builder as ArenaIrBuilder
participant Context as IrContext
participant Arena as Arena Refs
Input->>RawParser: parse text
RawParser->>RawParser: lex and parse (Raw AST)
RawParser-->>Builder: RawType, RawAttribute, RawRegion, RawBlock, RawOperation
Builder->>Context: interning types/attributes
Context-->>Builder: TypeRef, AttributeRef
Builder->>Builder: construct regions & blocks
Builder->>Builder: resolve operands & successors
Builder->>Context: build arena operations
Context-->>Arena: OpRef, RegionRef, BlockRef, ValueRef
Builder-->>Arena: fully resolved IR
sequenceDiagram
participant Module as ArenaModule
participant Validator as Validator
participant Scope as Scope Checker
participant UseChain as Use-Chain Checker
participant Result as ValidationResult
Module->>Validator: validate_all
Validator->>Scope: collect visible values per region
Scope->>Scope: recursively verify operands in scope
Scope-->>Result: stale_errors (if any)
Validator->>UseChain: enumerate operand uses
UseChain->>UseChain: verify use-chain entries match operands
UseChain-->>Result: use_chain_errors (if any)
Result-->>Validator: combined ValidationResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/trunk-ir/src/arena/parser.rs`:
- Around line 157-167: The code only checks duplicates within a single block
using seen_names but then inserts block-arg names into the shared value_map
without checking for preexisting keys, allowing later blocks to overwrite
earlier bindings; in the block-args insertion site (the loop over all_args and
the shared value_map insert code), add a guard: before inserting into value_map
check if value_map.contains_key(name) and, if it already exists, return a
ParseError (similar shape to the existing duplicate error) or otherwise
disambiguate (e.g. scope the key by block id); update the insertion logic that
currently calls value_map.insert(...) so it either errors on cross-block
collisions or uses a block-scoped key to prevent silent overwrites.
- Around line 129-132: build_region is leaking outer block_map entries into
nested regions because block entries created during build_region_inner are
resolved later against the shared block_map (successor resolution), causing
`^label` in a nested region to match outer blocks; fix by making region parsing
truly scoped: inside build_region (and similarly in the other region entry),
push a fresh scope or clear/shadow block_map after calling save_scopes, then run
build_region_inner so all block registrations go into that isolated scope, and
finally call restore_scopes before any successor resolution runs; ensure
functions named save_scopes, restore_scopes, build_region, and
build_region_inner are adjusted so successor resolution consults only the
current (restored) scope rather than the outer/shared block_map.
In `@crates/trunk-ir/src/arena/rewrite/signature_conversion.rs`:
- Around line 41-43: The current guard only accepts exactly dialect ==
Symbol::new("func") and name == Symbol::new("fn"), which misses synthesized
dialects like "core.func"; update the check around type_data to accept any
dialect that is "func" or ends with ".func" while still requiring the name "fn"
(e.g. replace the strict equality on type_data.dialect with a predicate that
checks type_data.dialect == "func" ||
type_data.dialect.as_str().ends_with(".func")). Apply the same relaxed check in
the other identical block that currently uses Symbol::new("func") /
Symbol::new("fn") so parsed modules like core.func are handled.
In `@crates/trunk-ir/src/arena/validation.rs`:
- Around line 227-233: validate_functions_in_region currently only recognizes
function ops with dialect "func" and name "func" so bodies of "wasm.func" are
skipped; update the check inside the loop that examines ctx.op(op) in
validate_functions_in_region to also treat wasm.func as a function root by
creating a wasm_dialect Symbol::new("wasm") (or equivalent) and including a
condition that accepts either (data.dialect == func_dialect && data.name ==
func_name_sym) or (data.dialect == wasm_dialect && data.name == func_name_sym)
so validate_value_integrity/validate_all will inspect wasm.func bodies as well.
- Around line 330-340: For each value in checked_values, detect duplicate
entries returned by ctx.uses(val) by adding a per-value HashSet (or similar) of
seen (u.user, u.operand_index) pairs: when iterating u in ctx.uses(val) first
check if the pair is already in the seen set and if so push a UseChainError
describing the duplicate entry for that val; otherwise insert the pair into seen
and then continue the existing check against actual_uses (the current membership
test that pushes UseChainError when (val, u.user, u.operand_index) is missing).
This ensures duplicate use-chain entries are reported in addition to missing
ones.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
crates/trunk-ir/src/parser/snapshots/trunk_ir__parser__tests__roundtrip_adt_struct.snapis excluded by!**/*.snapcrates/trunk-ir/src/parser/snapshots/trunk_ir__parser__tests__roundtrip_arith_ops.snapis excluded by!**/*.snapcrates/trunk-ir/src/parser/snapshots/trunk_ir__parser__tests__roundtrip_func_call.snapis excluded by!**/*.snapcrates/trunk-ir/src/parser/snapshots/trunk_ir__parser__tests__roundtrip_func_effects.snapis excluded by!**/*.snapcrates/trunk-ir/src/parser/snapshots/trunk_ir__parser__tests__roundtrip_scf_if.snapis excluded by!**/*.snapcrates/trunk-ir/src/parser/snapshots/trunk_ir__parser__tests__roundtrip_scf_loop.snapis excluded by!**/*.snapcrates/trunk-ir/src/parser/snapshots/trunk_ir__parser__tests__roundtrip_scf_switch.snapis excluded by!**/*.snap
📒 Files selected for processing (12)
crates/trunk-ir/src/arena/context.rscrates/trunk-ir/src/arena/mod.rscrates/trunk-ir/src/arena/parser.rscrates/trunk-ir/src/arena/printer.rscrates/trunk-ir/src/arena/rewrite/applicator.rscrates/trunk-ir/src/arena/rewrite/mod.rscrates/trunk-ir/src/arena/rewrite/pattern.rscrates/trunk-ir/src/arena/rewrite/rewriter.rscrates/trunk-ir/src/arena/rewrite/signature_conversion.rscrates/trunk-ir/src/arena/validation.rscrates/trunk-ir/src/parser/mod.rscrates/trunk-ir/src/parser/raw.rs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/trunk-ir/src/arena/validation.rs (1)
333-343:⚠️ Potential issue | 🟠 MajorDuplicate use-chain entries are still not reported.
Direction-2 only checks set membership in
actual_uses, so repeated identical entries inctx.uses(val)are silently accepted. That breaks the “exact match” guarantee for use-chain consistency.Suggested patch
for &val in &checked_values { + let mut seen_uses: HashSet<(OpRef, u32)> = HashSet::new(); for u in ctx.uses(val) { + if !seen_uses.insert((u.user, u.operand_index)) { + errors.push(UseChainError { + message: format!( + "duplicate use-chain entry for {:?}: user {:?} operand #{}", + val, u.user, u.operand_index, + ), + }); + continue; + } if !actual_uses.contains(&(val, u.user, u.operand_index)) { errors.push(UseChainError { message: format!( "use-chain entry for {:?} claims use by {:?} operand #{}, but no such operand exists", val, u.user, u.operand_index, ), }); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/trunk-ir/src/arena/validation.rs` around lines 333 - 343, The loop over ctx.uses(val) in validation.rs currently only checks membership in actual_uses and thus ignores duplicate identical entries; modify the logic to detect and report duplicate use-chain entries by keeping a seen set (e.g., a HashSet of tuples like (val, u.user, u.operand_index)) and for each u first check if that tuple was already seen—if so, push a UseChainError describing a duplicate use-chain entry for that (val, user, operand_index); otherwise insert into seen and continue to the existing membership check against actual_uses so both missing and duplicate entries are reported.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/trunk-ir/src/arena/validation.rs`:
- Around line 333-343: The loop over ctx.uses(val) in validation.rs currently
only checks membership in actual_uses and thus ignores duplicate identical
entries; modify the logic to detect and report duplicate use-chain entries by
keeping a seen set (e.g., a HashSet of tuples like (val, u.user,
u.operand_index)) and for each u first check if that tuple was already seen—if
so, push a UseChainError describing a duplicate use-chain entry for that (val,
user, operand_index); otherwise insert into seen and continue to the existing
membership check against actual_uses so both missing and duplicate entries are
reported.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/trunk-ir/src/arena/validation.rs`:
- Around line 197-200: The doc comment that currently states the validation is
for `func.func` only is inaccurate; update the comment to state that
value-integrity validation applies to both `func.func` and `wasm.func`
operations (the same change should be made for the similar comment later).
Locate the docstring above the validation implementation (the comment block
describing validation of value integrity) and revise the wording to mention both
`func.func` and `wasm.func`, keeping the rest of the description (checking that
every operand references a value defined within the function's region tree)
unchanged.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
crates/trunk-ir/src/arena/dialect/mod.rscrates/trunk-ir/src/arena/printer.rscrates/trunk-ir/src/arena/rewrite/signature_conversion.rscrates/trunk-ir/src/arena/transforms/dce.rscrates/trunk-ir/src/arena/transforms/global_dce.rscrates/trunk-ir/src/arena/transforms/scf_to_cf.rscrates/trunk-ir/src/arena/validation.rscrates/trunk-ir/src/arena/walk.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/trunk-ir/src/arena/rewrite/signature_conversion.rs
Add arena IR text format parser (arena/parser.rs) with ArenaIrBuilder that converts Raw* structures from the shared winnow combinators into arena-based IR. Includes 14 tests covering round-trips, error detection, and validation. Migrate the Salsa-based parser from parser.rs into parser/mod.rs and extract shared winnow combinators into parser/raw.rs so both the Salsa and arena parsers can reuse them. Reorder successor_list parsing in raw.rs to appear before return_type/effects/attrs/result_types, matching the arena printer output order. Add arena/validation.rs with walk_region type annotation fixes and corrected OperationDataBuilder API usage. Extend arena/printer.rs to decompose core.func types and emit effects inline (` -> T effects E`) instead of printing the opaque type. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Reject duplicate SSA names across block arguments within the same region in the arena IR parser, returning a clear error message - Clear block_map at the start of build_region to isolate block label scope per region, preventing nested regions from resolving outer block labels as successors - Extend validate_functions_in_region to recognise wasm.func as a function root alongside func.func, so its body is included in value integrity validation - Add tests covering all three fixes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Align the arena IR function type representation with the canonical dialect naming convention. All occurrences of (dialect="func", name="fn") are replaced with (dialect="core", name="func"), covering the production guard and builder in signature_conversion.rs as well as test helpers across printer, validation, walk, dce, global_dce, and scf_to_cf. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The function already validated both `func.func` and `wasm.func` operations, but the public doc comment and an inline comment only mentioned `func.func`. Updated both to accurately reflect the actual behavior. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…to outer scopes The old `collect_defined_in_region` recursively collected all values from all nested sub-regions into a flat set, then validated operands against that flat set. This incorrectly allowed outer scopes to reference values defined only inside inner regions (e.g. a `func.return` consuming a value produced inside an `scf.if` branch). Replace the flat-collection approach with visibility-based scoping: - Remove `collect_defined_in_region` and `collect_defined_in_block`. - Add `collect_region_top_level`: shallow collection of block args + op results at one region level only, without descending into sub-regions. - Rewrite `check_operands_in_region` to accept an `outer_visible` set, extend it with the current region's top-level values, propagate the extended set into nested sub-regions, and never leak sub-region values back to parent scopes. - Each function body check starts with an empty outer visible set. - Add regression test `inner_value_not_visible_in_outer_scope` that constructs a `func.return` using a value only defined inside an `scf.if` branch and asserts the validator rejects it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Kroisse
force-pushed
the
feature/arena-signature-conversion
branch
from
be8abc1 to
30bdb85
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #461 +/- ##
=======================================
Coverage 78.54% 78.54%
=======================================
Files 10 10
Lines 4935 4935
=======================================
Hits 3876 3876
Misses 1059 1059
🚀 New features to boost your workflow:
|
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
1 participant
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
Implements arena IR validation, parser, and SignatureConversion patterns, closing #440 and #438.
arena/parser.rs): Full text-format parser producingIrContextusing shared winnow combinators. Includes 14 tests covering round-trips, error detection, and validation. The Salsa-based parser is migrated toparser/mod.rs, with shared raw combinators extracted toparser/raw.rsso both parsers reuse them.arena/validation.rs): Ports IR validation rules to the arena IR, including use-chain consistency checks (every use points to a valid op, every value's uses list is accurate).arena/rewrite/signature_conversion.rs): MLIR-styleFuncSignatureConversionPatternandWasmFuncSignatureConversionPatternthat convertfunc.func/wasm.funcsignatures via a type converter, update entry block arg types in-place, and replace the op with a rebuilt function carrying the new signature and original body.set_block_arg_typeanddetach_regionadded toIrContext;ArenaTypeConverterlifetime threaded throughPatternRewriter<'a>;rewrite_function_signaturehelper extracted to eliminate duplication between the two conversion patterns.update_entry_block_argsnow returnsbooland bails out on arity mismatch, preventing partial IR corruption.Test plan
cargo nextest run -p trunk-irpasses all existing tests🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor