refactor(parser): promote rider SpecialClause variants to ClauseDisposition::ModifyPrior (U5-M2)#5566
Merged
Merged
Conversation
…sition::ModifyPrior (U5-M2)
Fourth U5-M2 increment. AltCostRider + ManaRetention + EntersTappedAttacking
(CR 118.9 / CR 106.4 / CR 508.4) share the structural role "modify a field of the
prior emitted def, emit no sibling" and fold into
`ClauseDisposition::ModifyPrior { modifier: PriorModifier }`, where `PriorModifier`
(AltCost | ManaRetention | EntersTappedAttacking) keeps each distinct rules concept
typed. The three lower.rs handler bodies (including the EntersTappedAttacking
pop+patch that stashes the unpatched original in else_ability) move verbatim under
`match modifier`. All three SpecialClause variants are deleted.
Faithful-by-construction (ClauseDisposition never reaches a serialized snapshot).
Parity: Nashi, Moon Sage's Scion (AltCost, alt_ability_cost folded onto the prior
CastFromZone) and Karn, Legacy Reforged (ManaRetention, expiry folded onto the mana
effect), zero snapshot drift. The third kind (EntersTappedAttacking) has no card in
the 568-card fixture, so its verbatim-moved pop+patch body is pinned by a direct
handler unit test instead.
clippy::large_enum_variant is allowed on ClauseDisposition (matching the sibling
oracle_ir/doc.rs convention): shrinking SpecialClause exposed the pre-existing large
`Emit` variant (two ContinuationAst option channels); the per-clause IR enum is
short-lived and Vec-allocated, so the size gap is acceptable.
matthewevans
enabled auto-merge
July 11, 2026 09:28
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors how field-level modifications to prior definitions are handled by introducing a unified ClauseDisposition::ModifyPrior variant and a PriorModifier enum, replacing several separate SpecialClause variants. It also adds corresponding snapshot and unit tests to verify the new behavior. The review feedback highlights a violation of Rule R6, noting that the newly introduced AltCost and ManaRetention match arms in lower.rs are missing mandatory CR annotations.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+1455
to
1460
| PriorModifier::AltCost(cost) => { | ||
| attach_alt_cost_to_prior_cast_from_zone(&mut defs, cost.clone()); | ||
| true | ||
| } | ||
| SpecialClause::DigInsteadAlt(alt_def) => { | ||
| if let Some(last_def) = defs.pop() { | ||
| let mut new_def = *alt_def.clone(); | ||
| apply_where_x_ability_expression( | ||
| &mut new_def, | ||
| clause_ir.where_x_expression.as_deref(), | ||
| ); | ||
| new_def.else_ability = Some(Box::new(last_def)); | ||
| defs.push(new_def); | ||
| } | ||
| true | ||
| } | ||
| SpecialClause::InsteadClause(instead_def) => { | ||
| // CR 614.1a + CR 608.2c: assemble a multi-clause base + an | ||
| // "instead" override so the runtime can produce both | ||
| // branches. Clause 1 becomes the root and is the Cow-swap | ||
| // target — when the override's `ConditionInstead` fires, | ||
| // `effects/mod.rs` swaps the root's effect with the | ||
| // override's at parent resolution, and the override branch | ||
| // returns terminally (see the `ConditionInstead` arm at | ||
| // ~line 2713 in `effects/mod.rs`). To make the tail clauses | ||
| // (2..N) conditional on the override NOT firing, we stash | ||
| // them in the override's `else_ability`: the runtime only | ||
| // walks `else_ability` when the swap did not happen. Net: | ||
| // condition true → only the override's effect runs (clause | ||
| // 1 swapped away, tail bypassed); condition false → clause | ||
| // 1 runs as printed, then the tail runs from | ||
| // `else_ability`. Single-clause bases collapse to the | ||
| // prior shape (empty tail → no `else_ability`). | ||
| if !defs.is_empty() { | ||
| let mut chain_defs = std::mem::take(&mut defs); | ||
| let mut root = chain_defs.remove(0); | ||
| for next in chain_defs { | ||
| append_to_deepest_sub_ability(&mut root, Some(Box::new(next))); | ||
| } | ||
| let mut instead = *instead_def.clone(); | ||
| // CR 702.33d + CR 707.10: Resolve "create N of those | ||
| // tokens" anaphor against the root (the antecedent | ||
| // for a multi-clause base is the first printed clause). | ||
| rewrite_those_tokens_from_antecedent(&mut instead.effect, &root.effect); | ||
| if rewrite_counter_instead_target_from_antecedent( | ||
| &mut instead.effect, | ||
| &root.effect, | ||
| ) { | ||
| instead.target_choice_timing = root.target_choice_timing; | ||
| } | ||
| if has_explicit_player_target(root.effect.as_ref()) { | ||
| rewrite_player_anaphor_targets_in_definition(&mut instead); | ||
| } | ||
| instead.else_ability = root.sub_ability.take(); | ||
| root.sub_ability = Some(Box::new(instead)); | ||
| defs.push(root); | ||
| } | ||
| true | ||
| PriorModifier::ManaRetention(expiry) => { | ||
| attach_mana_retention_to_prior_mana(&mut defs, *expiry); | ||
| } |
Contributor
There was a problem hiding this comment.
[MEDIUM] Missing CR annotations for AltCost and ManaRetention modifiers. Evidence: crates/engine/src/parser/oracle_effect/lower.rs:1455-1460.
Why it matters: Every rules-touching line of engine code must carry a comment of the form CR <number>: <description> to ensure strict fidelity to the MTG Comprehensive Rules.
Suggested fix: Add the corresponding CR annotations to the AltCost and ManaRetention match arms.
PriorModifier::AltCost(cost) => {
// CR 118.9: fold an alternative cost onto the prior CastFromZone.
attach_alt_cost_to_prior_cast_from_zone(&mut defs, cost.clone());
}
PriorModifier::ManaRetention(expiry) => {
// CR 106.4: fold a mana-retention expiry onto the prior Mana effect.
attach_mana_retention_to_prior_mana(&mut defs, *expiry);
}References
- Every rules-touching line of engine code must carry a comment of the form CR : (regex CR \d{3}(.\d+[a-z]?)?). (link)
Parse changes introduced by this PR✓ No card-parse changes detected. |
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.
Fourth U5-M2 increment. AltCostRider + ManaRetention + EntersTappedAttacking
(CR 118.9 / CR 106.4 / CR 508.4) share the structural role "modify a field of the
prior emitted def, emit no sibling" and fold into
ClauseDisposition::ModifyPrior { modifier: PriorModifier }, wherePriorModifier(AltCost | ManaRetention | EntersTappedAttacking) keeps each distinct rules concept
typed. The three lower.rs handler bodies (including the EntersTappedAttacking
pop+patch that stashes the unpatched original in else_ability) move verbatim under
match modifier. All three SpecialClause variants are deleted.Faithful-by-construction (ClauseDisposition never reaches a serialized snapshot).
Parity: Nashi, Moon Sage's Scion (AltCost, alt_ability_cost folded onto the prior
CastFromZone) and Karn, Legacy Reforged (ManaRetention, expiry folded onto the mana
effect), zero snapshot drift. The third kind (EntersTappedAttacking) has no card in
the 568-card fixture, so its verbatim-moved pop+patch body is pinned by a direct
handler unit test instead.
clippy::large_enum_variant is allowed on ClauseDisposition (matching the sibling
oracle_ir/doc.rs convention): shrinking SpecialClause exposed the pre-existing large
Emitvariant (two ContinuationAst option channels); the per-clause IR enum isshort-lived and Vec-allocated, so the size gap is acceptable.