refactor(parser): retain generic activated abilities as native IR - #6792
Conversation
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughActivated-ability parsing now builds and emits ChangesActivated ability IR migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OracleParser
participant ActivatedAbilityIR
participant ShellStages
participant Lowerer
participant Emitter
OracleParser->>ActivatedAbilityIR: parse activated ability
ActivatedAbilityIR->>ShellStages: configure normalization and extraction stages
OracleParser->>Lowerer: lower AbilityIr for guards and fallback checks
Lowerer-->>OracleParser: lowered ability and diagnostics
OracleParser->>Emitter: emit ability_ir_at(item_line, ir)
Possibly related PRs
Suggested labels: 🚥 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
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.rs (1)
8474-8512: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
normalize_activated_mana_instead_deltasilently drops a non-ConditionInsteadsub-ability condition, and this function is now reachable much more broadly.At line 8488,
sub.condition.take()unconditionally clearssub.condition. When the pattern match againstAbilityCondition::ConditionInsteadfails (i.e.sub.conditionwasSome(some_other_condition)), theelse { return; }branch returns without restoring it — the condition is permanently lost. Contrast with the two later failure branches in this same function (lines 8502 and 8508), which correctly restoresub.condition = Some(AbilityCondition::ConditionInstead { inner })before returning.This function's body is unchanged by this PR, but its visibility just widened to
pub(crate)specifically so it can be invoked via the newShellStage::NormalizeActivatedManaInsteadvariant, whichparse_activated_ability_irnow unconditionally adds toir.shell.stagesfor every generic and ability-word-prefixed activated ability. That is a materially larger call surface than before this PR (per the PR's own description ofNormalizeActivatedManaInsteadas a new addition), so any activated mana ability (Effect::Manawith aColorlessfixed count) that has asub_abilitygated by some condition OTHER than an "instead" alternative will now silently lose that condition during lowering.🐛 Proposed fix: restore the condition on match failure
- let Some(AbilityCondition::ConditionInstead { inner }) = sub.condition.take() else { - return; - }; + let taken_condition = sub.condition.take(); + let Some(AbilityCondition::ConditionInstead { inner }) = taken_condition else { + sub.condition = taken_condition; + return; + };🤖 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.rs` around lines 8474 - 8512, Update normalize_activated_mana_instead_delta so a sub-ability condition that is not AbilityCondition::ConditionInstead is restored before returning. Preserve the existing early-return behavior for absent conditions and keep the later restoration paths unchanged.
🤖 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 192-196: Strengthen the assertions for the first lowered ability
in the snapshot test to verify its complete effect chain, including both the
roll and draw instructions in order rather than only matching Effect::Mana.
Snapshot both effect layers as needed so a nonterminal roll cannot omit either
clause.
---
Outside diff comments:
In `@crates/engine/src/parser/oracle.rs`:
- Around line 8474-8512: Update normalize_activated_mana_instead_delta so a
sub-ability condition that is not AbilityCondition::ConditionInstead is restored
before returning. Preserve the existing early-return behavior for absent
conditions and keep the later restoration paths unchanged.
🪄 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: f772103d-f09b-4a17-b91c-06b9dbbe42d7
⛔ Files ignored due to path filters (21)
crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__aetherling_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__barbarian_ring_activated_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__barbarian_ring_activated_lowered.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__batterskull_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__birds_of_paradise_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__bomat_courier_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__boseiju_who_endures_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__component_pouch_activated_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__component_pouch_activated_lowered.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__experiment_one_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__figure_of_destiny_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__ghost_lit_stalker_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__jade_mage_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__llanowar_elves_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__mother_of_runes_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__repeat_offender_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__stoneforge_mystic_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__sylvan_safekeeper_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__thespians_stage_generic_activated_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__thespians_stage_generic_activated_lowered.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__walking_ballista_ir.snapis excluded by!**/*.snap,!**/snapshots/**
📒 Files selected for processing (6)
crates/engine/src/parser/oracle.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_ir/context.rscrates/engine/src/parser/oracle_ir/effect_chain.rscrates/engine/src/parser/oracle_ir/snapshot_tests.rscrates/engine/src/parser/oracle_pipeline_snapshot_tests.rs
Parse changes introduced by this PRBaseline pending for |
1 participant
Summary
AbilityIrVerification
cargo fmt --all --checkcargo clippy --all-targets -- -D warnings: PASScmpexit 0Witnesses
The full-pool exports used the same checked AtomicCards input; neither generated export is committed.
Summary by CodeRabbit