refactor(parser): emit temporal delayed triggers as native ir - #6807
refactor(parser): emit temporal delayed triggers as native ir#6807matthewevans wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughTemporal delayed spell triggers now produce ChangesTemporal delayed trigger routing
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant parse_oracle_ir
participant try_parse_temporal_delayed_trigger_ability
participant ability_ir_at
participant lower_oracle_ir
parse_oracle_ir->>try_parse_temporal_delayed_trigger_ability: parse temporal delayed trigger
try_parse_temporal_delayed_trigger_ability-->>parse_oracle_ir: return AbilityIr
parse_oracle_ir->>ability_ir_at: emit AbilityIr
ability_ir_at->>lower_oracle_ir: lower during later IR phase
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/engine/src/parser/oracle_ir/snapshot_tests.rs`:
- Around line 1001-1023: Expand the temporal recognizer fixture table to include
a distinct card case matching the `try_parse_whenever_this_turn` arm, while
retaining separate fixtures for the existing recognizers and adding any missing
fourth-arm coverage. Keep each fixture representative of a reusable temporal
pattern rather than a card-specific special case.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dc50c1cb-22b8-45c8-ae20-546fe9f2dcae
⛔ Files ignored due to path filters (6)
crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__temporal_delayed_trigger@full_throttle_temporal_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__temporal_delayed_trigger@full_throttle_temporal_lowered.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__temporal_delayed_trigger@galvanic_iteration_temporal_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__temporal_delayed_trigger@galvanic_iteration_temporal_lowered.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__temporal_delayed_trigger@pact_of_negation_temporal_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__temporal_delayed_trigger@pact_of_negation_temporal_lowered.snapis excluded by!**/*.snap,!**/snapshots/**
📒 Files selected for processing (3)
crates/engine/src/parser/oracle.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_ir/snapshot_tests.rs
Parse changes introduced by this PR✓ No card-parse changes detected. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/engine/src/parser/oracle_ir/snapshot_tests.rs (1)
1064-1073: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove the card-name-specific assertion.
Add an expected payload-kind field to each fixture row and assert it generically. This avoids extending the test with additional card-name branches for future spell payload cases.
As per path instructions, tests must exercise reusable building blocks instead of one-card special cases.
🤖 Prompt for 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. In `@crates/engine/src/parser/oracle_ir/snapshot_tests.rs` around lines 1064 - 1073, Replace the Pact of Negation-specific branch in the snapshot tests with an expected payload-kind field on each fixture row, then assert the parsed delayed-trigger effect’s kind against that field generically. Update all fixture rows to provide the expected value and remove the card-name check and unreachable branch.Source: Path instructions
🧹 Nitpick comments (1)
crates/engine/src/parser/oracle_effect/tests.rs (1)
11135-11145: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the delayed-trigger payload, not only its outer variant.
This test would pass if parsing dropped the second sentence or produced the wrong trigger duration, event, or copy effect:
source_textis copied from the original input by the upstream constructor, andmatches!discards allCreateDelayedTriggerfields. Add payload-level assertions or a focused IR snapshot that proves the complete clause was parsed.🤖 Prompt for 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. In `@crates/engine/src/parser/oracle_effect/tests.rs` around lines 11135 - 11145, Strengthen the test for try_parse_temporal_delayed_trigger_ability by inspecting the fields inside Effect::CreateDelayedTrigger rather than only matching its outer variant. Assert the complete delayed-trigger payload, including its duration, triggering event, and copy effect, so both sentences and the intended behavior are verified; retain the existing source_text and clause-count assertions.
🤖 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 `@crates/engine/src/parser/oracle_ir/snapshot_tests.rs`:
- Around line 1001-1010: Strengthen the `Glimpse of Nature` case in the `cases`
fixture within the snapshot test by either supplying expected IR and lowered
snapshot values or explicitly asserting its `Whenever … this turn` condition and
draw-card payload. Ensure the test exercises the fourth recognizer arm’s
semantic parsing and lowering, not only the generic `CreateDelayedTrigger`
shape.
---
Outside diff comments:
In `@crates/engine/src/parser/oracle_ir/snapshot_tests.rs`:
- Around line 1064-1073: Replace the Pact of Negation-specific branch in the
snapshot tests with an expected payload-kind field on each fixture row, then
assert the parsed delayed-trigger effect’s kind against that field generically.
Update all fixture rows to provide the expected value and remove the card-name
check and unreachable branch.
---
Nitpick comments:
In `@crates/engine/src/parser/oracle_effect/tests.rs`:
- Around line 11135-11145: Strengthen the test for
try_parse_temporal_delayed_trigger_ability by inspecting the fields inside
Effect::CreateDelayedTrigger rather than only matching its outer variant. Assert
the complete delayed-trigger payload, including its duration, triggering event,
and copy effect, so both sentences and the intended behavior are verified;
retain the existing source_text and clause-count assertions.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2164a2b1-93eb-448c-8e2c-41737c22d9f0
📒 Files selected for processing (2)
crates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/parser/oracle_ir/snapshot_tests.rs
| // The three established grammar representatives remain the stable IR/lowered | ||
| // snapshot fixtures. The direct `Whenever … this turn` arm uses the same | ||
| // structural assertions without a fourth snapshot pair. | ||
| let cases = [ | ||
| ( | ||
| None, | ||
| "Whenever you cast a creature spell this turn, draw a card.", | ||
| "Glimpse of Nature", | ||
| &["Sorcery"][..], | ||
| ), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Verify the fourth recognizer arm semantically.
The Glimpse of Nature case uses None for snapshots and therefore only reaches the generic CreateDelayedTrigger shape checks. A regression in its condition, draw payload, lowering, or arm dispatch could still pass. Add a two-layer snapshot pair or assert the expected condition and payload explicitly.
As per path instructions, test the reusable parser building block across its parameter range rather than leaving one recognizer arm with only a generic wrapper check.
🤖 Prompt for 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.
In `@crates/engine/src/parser/oracle_ir/snapshot_tests.rs` around lines 1001 -
1010, Strengthen the `Glimpse of Nature` case in the `cases` fixture within the
snapshot test by either supplying expected IR and lowered snapshot values or
explicitly asserting its `Whenever … this turn` condition and draw-card payload.
Ensure the test exercises the fourth recognizer arm’s semantic parsing and
lowering, not only the generic `CreateDelayedTrigger` shape.
Source: Path instructions
Pull request was closed
Pull request was closed
Pull request was closed
1 participant
Converts the narrow instant/sorcery temporal delayed-trigger route to emit native
AbilityIrwhile preserving its four recognizer arms and the Pact payload’s separately lowered boxed definition.\n\nVerification: focused two-layer snapshots (Pact of Negation, Full Throttle, Galvanic Iteration), formatting and parser structural gates. Independent review approved. Strict clippy/full-pool parity deferred locally because the isolated test target consumed available disk; CI will provide that coverage.Summary by CodeRabbit
Bug Fixes
Tests