feat(tribute-passes): implement arena-based evidence passes (#442)#463
Open
feat(tribute-passes): implement arena-based evidence passes (#442)#463
Conversation
Migrate `add_evidence_params` and `transform_evidence_calls` to the arena IR via the Salsa-Arena bridge pattern, as part of the "Bridge & Shared Pass Migration" milestone. - `trunk-ir`: add `prepend_block_arg` method to `IrContext` for inserting a block argument at position 0 - `tribute-ir`: add arena type helpers to the ability dialect — `marker_adt_type_ref`, `evidence_adt_type_ref`, `is_marker_type_ref`, and `is_evidence_type_ref` - `tribute-passes`: implement `arena_add_evidence_params` and `arena_transform_evidence_calls` in `evidence::arena`; update the Salsa entry points to bridge into arena IR, run the arena passes, and bridge back; remove the old Salsa pattern-based implementations; add tests for all new arena functionality Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughPorts evidence insertion to arena IR: adds arena ability ADT helpers, ports evidence passes to operate on ArenaModule with Salsa↔Arena bridging, updates pipeline to invoke arena passes, and adds IrContext::prepend_block_arg for block-arg prepends. Changes
Sequence Diagram(s)sequenceDiagram
participant Pipeline as Pipeline
participant SalsaDB as Salsa DB
participant Bridge as Salsa↔Arena Bridge
participant ArenaMod as ArenaModule
participant ArenaPass as Arena Evidence Passes
Pipeline->>SalsaDB: hold core Module
Pipeline->>Bridge: import_salsa_module(Module)
Bridge->>ArenaMod: create ArenaModule
Pipeline->>ArenaPass: arena_add_evidence_params(ArenaModule)
ArenaPass->>ArenaMod: mutate module (add evidence params)
Pipeline->>Bridge: export_to_salsa(ArenaModule)
Bridge->>SalsaDB: produce modified Module'
alt evidence call transforms required
Pipeline->>Bridge: import_salsa_module(Module')
Bridge->>ArenaMod: create ArenaModule
Pipeline->>ArenaPass: arena_transform_evidence_calls(ArenaModule)
ArenaPass->>ArenaMod: mutate module (transform calls)
Pipeline->>Bridge: export_to_salsa(ArenaModule)
Bridge->>SalsaDB: produce Module''
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #463 +/- ##
==========================================
+ Coverage 78.54% 78.65% +0.11%
==========================================
Files 10 10
Lines 4935 4947 +12
==========================================
+ Hits 3876 3891 +15
+ Misses 1059 1056 -3
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
….rs to pipeline.rs Keep evidence.rs as a pure arena pass library with no Salsa bridging. pipeline.rs now owns all import_salsa_module/export_to_salsa calls, early-exit checks, and the stage_evidence_params/stage_evidence_calls wrappers, matching the pattern used by other pipeline stages. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The legacy combined evidence pass had no callers after the pipeline was split into stage_evidence_params and stage_evidence_calls. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tribute-passes/src/evidence.rs`:
- Around line 309-331: The code currently skips pure functions and also assumes
an evidence first-arg when it may be absent, risking silent corruption; change
the logic so you always traverse function bodies (remove the early continue that
skips pure functions) but only attempt to extract ev_value and call
transform_calls_in_region when has_evidence_first_param(ctx, func_ty) is true;
additionally, if a function is marked in all_effectful but
has_evidence_first_param returns false (i.e., func_name ∈ all_effectful &&
!has_ev_param), fail fast by emitting an explicit error/diagnostic or returning
Err (rather than reading ctx.block_arg) to prevent silent rewrites — adjust the
block that computes is_explicitly_effectful, has_ev_param, ev_value and the call
to transform_calls_in_region accordingly using the symbols all_effectful,
has_evidence_first_param, func_op, body, entry_block, ev_value, and
transform_calls_in_region.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/tribute-passes/src/evidence.rscrates/tribute-passes/src/lib.rssrc/pipeline.rs
Remove the `pub mod arena { ... }` wrapper from `evidence.rs`, promoting
all arena functions and tests to the top level of the module. Merge arena
imports into the module-level use declarations, rename `mod arena_tests`
to `mod tests` (dropping the old Salsa-only tests), and rename the two
public arena pass functions:
- `arena::arena_add_evidence_params` → `add_evidence_params`
- `arena::arena_transform_evidence_calls` → `transform_evidence_calls`
Update call sites in `src/pipeline.rs` accordingly.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/tribute-passes/src/evidence.rs (1)
296-315:⚠️ Potential issue | 🔴 CriticalFail fast when an effectful function is missing the prepended evidence parameter.
At Line 300 onward,
is_explicitly_effectful && !has_ev_paramstill reaches Line 315 and treatsarg0as evidence. That can silently rewrite calls with a non-evidence value if phase 1 was skipped/partial for any function.🔧 Suggested guard
if !is_explicitly_effectful && !has_ev_param { continue; } + assert!( + !is_explicitly_effectful || has_ev_param, + "transform_evidence_calls: `{}` is effectful but missing evidence param; run add_evidence_params first", + func_name + ); // Get the evidence value from first block arg let body = func_op.body(ctx); let blocks = &ctx.region(body).blocks; if blocks.is_empty() { continue; } let entry_block = blocks[0]; let block_args = ctx.block_args(entry_block); - if block_args.is_empty() { - continue; - } + assert!( + !block_args.is_empty(), + "transform_evidence_calls: expected entry block arg[0] evidence in `{}`", + func_name + ); let ev_value = ctx.block_arg(entry_block, 0);Based on learnings: when a pass depends on invariants established by earlier passes, prefer fail-fast instead of silent fallback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tribute-passes/src/evidence.rs` around lines 296 - 315, The code currently proceeds to treat the first block argument as evidence even when is_explicitly_effectful is true but has_ev_param is false; change this to fail-fast: after computing is_explicitly_effectful and has_ev_param, detect the case where is_explicitly_effectful && !has_ev_param and immediately surface a hard failure (emit a diagnostic/error or return Err) including the function identifier (func_name) so the pass cannot silently rewrite a non-evidence value as ev_value; ensure this check is done before accessing body/block args (i.e., before using func_op, entry_block, ctx.block_arg).
🤖 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/tribute-passes/src/evidence.rs`:
- Around line 296-315: The code currently proceeds to treat the first block
argument as evidence even when is_explicitly_effectful is true but has_ev_param
is false; change this to fail-fast: after computing is_explicitly_effectful and
has_ev_param, detect the case where is_explicitly_effectful && !has_ev_param and
immediately surface a hard failure (emit a diagnostic/error or return Err)
including the function identifier (func_name) so the pass cannot silently
rewrite a non-evidence value as ev_value; ensure this check is done before
accessing body/block args (i.e., before using func_op, entry_block,
ctx.block_arg).
Replace manual op traversal and mutation in `add_evidence_params` and `transform_evidence_calls` with `AddEvidenceParamPattern` and `TransformEvidenceCallPattern` driven by `PatternApplicator`. Add `find_enclosing_evidence` helper that walks the parent chain to locate the evidence block argument, eliminating the need for the recursive `transform_calls_in_region` / `transform_calls_in_block` functions. Public API signatures are unchanged. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/tribute-passes/src/evidence.rs (1)
338-340:⚠️ Potential issue | 🟠 MajorFail fast when an effectful call has no enclosing evidence context.
At Line 338, returning
falsesilently skips rewriting a call that is known effectful (calleealready matched), which can leave mismatched call signatures and defer the failure to later passes.Suggested fix
- let Some(ev_value) = find_enclosing_evidence(ctx, op) else { - return false; - }; + let Some(ev_value) = find_enclosing_evidence(ctx, op) else { + panic!( + "transform_evidence_calls: effectful call to `{}` has no enclosing evidence arg; run add_evidence_params first", + callee + ); + };Based on learnings: when a pass relies on invariants established by a prior pass, prefer fail-fast over silent fallback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tribute-passes/src/evidence.rs` around lines 338 - 340, The code silently returns false when find_enclosing_evidence(ctx, op) yields None, which hides a violated invariant for effectful calls; change this to a fail-fast error: replace the early "return false" in the block that reads "let Some(ev_value) = find_enclosing_evidence(ctx, op) else { return false; }" with an explicit panic/reporting call that includes the operation span and a clear message (e.g., via panic!, span_bug!, or ctx's diagnostic API) so the pass fails immediately when an effectful callee lacks an enclosing evidence context.
🤖 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/tribute-passes/src/evidence.rs`:
- Around line 338-340: The code silently returns false when
find_enclosing_evidence(ctx, op) yields None, which hides a violated invariant
for effectful calls; change this to a fail-fast error: replace the early "return
false" in the block that reads "let Some(ev_value) =
find_enclosing_evidence(ctx, op) else { return false; }" with an explicit
panic/reporting call that includes the operation span and a clear message (e.g.,
via panic!, span_bug!, or ctx's diagnostic API) so the pass fails immediately
when an effectful callee lacks an enclosing evidence context.
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
Ports
add_evidence_paramsandtransform_evidence_callsto the arena IR via the Salsa-Arena bridge pattern, closing #442 ("Port Evidence Passes to Arena IR") in the "Bridge & Shared Pass Migration" milestone.trunk-ir: Addprepend_block_argtoIrContext— inserts a new block argument at position 0, shifting all existing arg indices and updating their uses via RAUWtribute-ir: Add arena type helpers to the ability dialect —marker_adt_type_ref,evidence_adt_type_ref,is_marker_type_ref,is_evidence_type_ref(mirrors the Salsa versions)tribute-passes: Implementarena_add_evidence_paramsandarena_transform_evidence_callsinevidence::arena; update the Salsa entry points (add_evidence_params,transform_evidence_calls) to bridge into arena IR viaimport_salsa_module/export_to_salsa, run the arena passes, and bridge back; remove the old Salsa pattern-based implementations (AddEvidenceParamPattern,TransformCallsPattern); add unit and integration tests for all new arena functionalityTest plan
prepend_block_argincrates/trunk-ir/src/arena/context.rsmarker_adt_type_ref,evidence_adt_type_ref,is_marker_type_ref,is_evidence_type_ref) incrates/tribute-ir/src/arena/dialect/ability.rsis_effectful_type_arenahelperarena_add_evidence_params+arena_transform_evidence_calls)cargo nextest run --workspace)Closes #442
🤖 Generated with Claude Code
Summary by CodeRabbit
Refactor
Tests