diff --git a/crates/engine/src/game/mana_abilities.rs b/crates/engine/src/game/mana_abilities.rs index 97fb7f57a2..5795d85d37 100644 --- a/crates/engine/src/game/mana_abilities.rs +++ b/crates/engine/src/game/mana_abilities.rs @@ -9947,6 +9947,170 @@ mod tests { ); } + // Issue #6507 integration follow-up: `make_gemstone_mine` above omits the + // trailing "If there are no mining counters on this land, sacrifice it." + // sub-ability entirely, so it never exercised the reported bug or its fix + // through the real activation pipeline — only the parser-level AST shape + // is covered by `oracle::tests::gemstone_mine_conditional_sacrifice_binds_to_self_ref`. + // This fixture mirrors the actual end-to-end parsed shape (mana effect + + // conditional `Sacrifice { target: SelfRef }` sub-ability gated on zero + // mining counters) so activation itself proves the fix: mana is produced + // regardless, and the land is sacrificed only on the activation that + // removes its LAST counter. + fn make_gemstone_mine_with_sacrifice_sub_ability( + state: &mut GameState, + player: PlayerId, + initial_mining_counters: u32, + ) -> ObjectId { + let land = create_object( + state, + CardId(8003), + player, + "Gemstone Mine".to_string(), + Zone::Battlefield, + ); + let obj = state.objects.get_mut(&land).unwrap(); + obj.card_types + .core_types + .push(crate::types::card_type::CoreType::Land); + let mining_key = crate::types::counter::parse_counter_type("MINING"); + obj.counters.insert(mining_key, initial_mining_counters); + + let sacrifice_if_depleted = AbilityDefinition::new( + AbilityKind::Spell, + Effect::Sacrifice { + target: TargetFilter::SelfRef, + count: QuantityExpr::Fixed { value: 1 }, + min_count: 0, + }, + ) + .condition(AbilityCondition::QuantityCheck { + lhs: QuantityExpr::Ref { + qty: QuantityRef::CountersOn { + scope: ObjectScope::Source, + counter_type: Some(CounterType::Generic("mining".to_string())), + }, + }, + comparator: Comparator::EQ, + rhs: QuantityExpr::Fixed { value: 0 }, + }); + + let ability = AbilityDefinition::new( + AbilityKind::Activated, + Effect::Mana { + produced: ManaProduction::AnyOneColor { + count: QuantityExpr::Fixed { value: 1 }, + color_options: vec![ + ManaColor::White, + ManaColor::Blue, + ManaColor::Black, + ManaColor::Red, + ManaColor::Green, + ], + contribution: ManaContribution::Base, + }, + restrictions: Vec::new(), + grants: Vec::new(), + expiry: None, + target: None, + }, + ) + .cost(AbilityCost::Composite { + costs: vec![ + AbilityCost::Tap, + AbilityCost::RemoveCounter { + count: 1, + counter_type: CounterMatch::OfType(CounterType::Generic("mining".to_string())), + target: None, + selection: crate::types::ability::CounterCostSelection::SingleObject, + }, + ], + }) + .sub_ability(sacrifice_if_depleted); + Arc::make_mut(&mut obj.abilities).push(ability); + land + } + + #[test] + fn gemstone_mine_survives_activation_with_counters_remaining() { + let mut state = GameState::new_two_player(42); + let player = PlayerId(0); + let land = make_gemstone_mine_with_sacrifice_sub_ability(&mut state, player, 2); + + let def = state + .objects + .get(&land) + .unwrap() + .abilities + .first() + .cloned() + .unwrap(); + let mut events = Vec::new(); + resolve_mana_ability( + &mut state, + land, + player, + &def, + &mut events, + Some(ProductionOverride::SingleColor(ManaType::Green)), + ) + .expect("Gemstone Mine activation must not fail with counters present"); + + assert_eq!( + state.players[player.0 as usize] + .mana_pool + .count_color(ManaType::Green), + 1, + "mana must be produced regardless of the trailing conditional sacrifice" + ); + assert!( + state.battlefield.contains(&land), + "Gemstone Mine must remain on the battlefield while a mining counter remains after activation" + ); + } + + #[test] + fn gemstone_mine_sacrifices_itself_on_last_counter_removed() { + let mut state = GameState::new_two_player(42); + let player = PlayerId(0); + let land = make_gemstone_mine_with_sacrifice_sub_ability(&mut state, player, 1); + + let def = state + .objects + .get(&land) + .unwrap() + .abilities + .first() + .cloned() + .unwrap(); + let mut events = Vec::new(); + resolve_mana_ability( + &mut state, + land, + player, + &def, + &mut events, + Some(ProductionOverride::SingleColor(ManaType::Green)), + ) + .expect("Gemstone Mine activation must not fail on its last counter"); + + assert_eq!( + state.players[player.0 as usize] + .mana_pool + .count_color(ManaType::Green), + 1, + "mana must still be produced on the activation that empties the last counter" + ); + assert!( + !state.battlefield.contains(&land), + "Gemstone Mine must be sacrificed once its last mining counter is removed (issue #6507)" + ); + assert!( + state.players[player.0 as usize].graveyard.contains(&land), + "the sacrificed Gemstone Mine must land in its controller's graveyard" + ); + } + #[test] fn cabal_coffers_pays_generic_taps_and_counts_swamps() { let mut state = GameState::new_two_player(42); diff --git a/crates/engine/src/parser/oracle_effect/mod.rs b/crates/engine/src/parser/oracle_effect/mod.rs index 46ce49b490..4dc65025ef 100644 --- a/crates/engine/src/parser/oracle_effect/mod.rs +++ b/crates/engine/src/parser/oracle_effect/mod.rs @@ -30668,6 +30668,14 @@ pub(crate) fn parse_effect_chain_ir( // which is exactly why a head-only rewrite corrupts the chain. let is_distributed_chunk = text_is_compound_subject_distribution(&text); // Kicker clauses referencing "that creature"/"it" inherit the parent's target. + // CR 608.2c: untouched — its `!builder.is_empty()` gate is intentionally broad + // (many antecedent shapes reach this without a `target_filter()`, e.g. a typed + // trigger subject rebind chain), and narrowing it here regressed dozens of + // unrelated cards (Feather, the Redeemed's exile-return rider; Managorger + // Phoenix; dropped/added parent-target bindings across PutCounter/Pump/grant/ + // ChangeZone/DealDamage/Destroy/GainControl) that a full parse-diff against + // main surfaced. See the Sacrifice-specific fixup immediately below instead, + // which is scoped to exactly the reported bug class. if condition.is_some() && !is_distributed_chunk && !condition.as_ref().is_some_and(condition_refs_source_object) @@ -30683,6 +30691,56 @@ pub(crate) fn parse_effect_chain_ir( { replace_target_with_parent(&mut clause.effect); } + // CR 608.2c (issue #6507): the sacrifice imperative parser + // (`parse_targeted_action_ast`) defaults a bare "it"/"them" pronoun with no + // trigger subject to `ParentTarget`, on the assumption that SOME earlier + // clause chose a real target for it to inherit — the common case + // (`bare_it_without_trigger_subject_preserves_parent_target`). That assumption + // is false, and the fallback must not fire, when ANY of the established + // antecedent authorities finds a referent: + // - `parent_target_available` (computed above) — a chosen typed target + // anywhere in this chain, an `if you do` object anchor, OR an outer + // chain's referent inherited into a recursively-parsed `Otherwise` + // else-branch via `ctx.parent_target_available` (Brilliance Unleashed's + // class). Omitting this last case would let an else-branch whose own + // internal clauses are targetless rebind a valid OUTER `ParentTarget` to + // `SelfRef`, sacrificing the ability's source instead of the object + // selected before the paired conditional. + // - `chain_prior_referent_is_created_token` — a token/copy publisher + // (`Effect::Populate` has no `target` field but DOES publish a + // created-token referent; `Token`/`CopyTokenOf` are covered by + // `parent_target_available`'s typed-referent scan for free since both DO + // have a `target_filter()`). + // - the nearest non-`Continue` clause (an absorbed rider carries no + // independent referent, so it must not be mistaken for a targetless + // antecedent — mirrors `if_you_do_object_anchor`'s same lookback) + // exposing SOME target concept (`target_filter().is_some()`). + // With none of those present, there is nothing for `ParentTarget` to resolve + // to, so a conditional "sacrifice it" tail (Gemstone Mine's "{T}, Remove a + // mining counter: Add one mana of any color. If there are no mining counters + // on this land, sacrifice it.") silently never sacrifices anything. Rebind it + // to the ability's own source, the only object "it" can plausibly name here. + // + // Deliberately scoped to `Effect::Sacrifice` only (not folded into the + // Kicker-clause rewrite's gate above): that gate is shared by every + // `replace_target_with_parent`-eligible effect kind, and narrowing it there + // regressed unrelated cards — see the comment above. + let nearest_semantic_clause = builder + .clauses() + .iter() + .rev() + .find(|clause| !matches!(clause.disposition, ClauseDisposition::Continue { .. })); + let prior_clause_has_no_target_concept = !parent_target_available + && !chain_prior_referent_is_created_token(builder.clauses()) + && nearest_semantic_clause + .is_some_and(|prev| prev.parsed.effect.target_filter().is_none()); + if condition.is_some() && !is_distributed_chunk && prior_clause_has_no_target_concept { + if let Effect::Sacrifice { target, .. } = &mut clause.effect { + if matches!(target, TargetFilter::ParentTarget) { + *target = TargetFilter::SelfRef; + } + } + } // CR 608.2c: Pronoun clause following a conditional targeted effect. if condition.is_none() && !is_distributed_chunk diff --git a/crates/engine/src/parser/oracle_effect/tests.rs b/crates/engine/src/parser/oracle_effect/tests.rs index 76a8e8b96d..f76c015079 100644 --- a/crates/engine/src/parser/oracle_effect/tests.rs +++ b/crates/engine/src/parser/oracle_effect/tests.rs @@ -18715,6 +18715,81 @@ fn kicker_instead_chain_produces_correct_condition() { )); } +/// Issue #6507 review follow-up: `Effect::Populate` has no `target` field +/// (`target_filter()` is `None`, just like a mana ability's untargeted `Add`), +/// but it DOES publish a created-token referent via +/// `chain_prior_referent_is_created_token` — the same "populate anaphor +/// chain" class documented at the sacrifice imperative's bare-"it" binding. +/// A conditional "sacrifice it" tail after Populate must keep inheriting the +/// populated token (`ParentTarget`, rewritten to `LastCreated` by +/// `rewrite_parent_target_to_last_created`) — NOT get rebound to `SelfRef`, +/// which would sacrifice the ability's source instead of the populated copy. +#[test] +fn populate_conditional_sacrifice_keeps_created_token_not_self_ref() { + let ability = parse_effect_chain( + "Populate. If it was kicked, sacrifice it.", + AbilityKind::Spell, + ); + assert!(matches!(&*ability.effect, Effect::Populate)); + let sub = ability.sub_ability.as_ref().expect("expected sub_ability"); + assert!( + sub.condition.is_some(), + "expected the 'if it was kicked' gate to survive as a condition" + ); + let Effect::Sacrifice { target, .. } = &*sub.effect else { + panic!("expected Effect::Sacrifice, got {:?}", sub.effect); + }; + assert_eq!( + *target, + TargetFilter::LastCreated, + "sacrifice target must bind to the populated token (LastCreated), \ + not fall back to SelfRef (the ability's own source)" + ); +} + +/// Issue #6507 review follow-up (HIGH): an `Otherwise` else-branch is parsed +/// as its own recursive `parse_effect_chain_ir` call whose own `clauses` start +/// empty (see `parse_effect_chain_ir`'s "Otherwise" handler) — so a naive +/// "does the immediately-preceding INTERNAL clause expose a target?" check +/// can never see an outer referent established BEFORE the paired conditional +/// (Brilliance Unleashed's class). That outer referent is exactly what +/// `ctx.parent_target_available` carries across the recursive call for. Drive +/// the recursive parser directly with `parent_target_available: true` seeded +/// (mirroring what the "Otherwise" handler seeds from a real outer typed +/// target) to prove the no-antecedent Sacrifice fallback defers to it: a +/// targetless inner clause (`create an emblem` has no `target` field, so it +/// looks exactly like Gemstone Mine's untargeted mana ability in isolation) +/// followed by a conditional "sacrifice it" must still resolve to +/// `ParentTarget` — inheriting the outer referent — not get misrouted to +/// `SelfRef` (the ability's own source). +#[test] +fn conditional_sacrifice_defers_to_seeded_parent_target_available() { + let mut ctx = ParseContext { + parent_target_available: true, + ..Default::default() + }; + let ability = parse_effect_chain_with_context( + "create an emblem. If it wasn't kicked, sacrifice it.", + AbilityKind::Spell, + &mut ctx, + ); + assert!(matches!(&*ability.effect, Effect::Unimplemented { .. })); + let sac = ability + .sub_ability + .as_ref() + .expect("expected the conditional sacrifice chained after the targetless clause"); + let Effect::Sacrifice { target, .. } = &*sac.effect else { + panic!("expected Effect::Sacrifice, got {:?}", sac.effect); + }; + assert_eq!( + *target, + TargetFilter::ParentTarget, + "with parent_target_available seeded (simulating an inherited outer \ + referent from an enclosing Otherwise chain), sacrifice must keep \ + ParentTarget, not fall back to SelfRef" + ); +} + #[test] fn kicker_leading_instead_produces_correct_condition() { // CR 608.2c: "if kicked, instead [effect]" — leading "instead" variant. diff --git a/crates/engine/src/parser/oracle_tests.rs b/crates/engine/src/parser/oracle_tests.rs index ee50378e7f..5c49fcd37e 100644 --- a/crates/engine/src/parser/oracle_tests.rs +++ b/crates/engine/src/parser/oracle_tests.rs @@ -8214,6 +8214,50 @@ fn mox_pearl_mana_ability() { assert_eq!(r.abilities[0].kind, AbilityKind::Activated); } +/// Issue #6507: Gemstone Mine's mana ability is targetless ("Add one mana of +/// any color" chooses no target), so the trailing "If there are no mining +/// counters on this land, sacrifice it" conditional has no parent target to +/// inherit. The bare "it" pronoun must bind to the ability's own source +/// (`TargetFilter::SelfRef`), not `ParentTarget` — `ParentTarget` resolves to +/// nothing here and the land was never sacrificed. This is the +/// depletion-land class, not a one-off: any targetless activated ability +/// whose trailing conditional says "sacrifice it" shares the exposure. +#[test] +fn gemstone_mine_conditional_sacrifice_binds_to_self_ref() { + let r = parse( + "This land enters with three mining counters on it.\n\ + {T}, Remove a mining counter from this land: Add one mana of any color. \ + If there are no mining counters on this land, sacrifice it.", + "Gemstone Mine", + &[], + &["Land"], + &[], + ); + assert_eq!(r.abilities.len(), 1); + let ability = &r.abilities[0]; + assert!( + matches!(*ability.effect, Effect::Mana { .. }), + "expected Effect::Mana, got {:?}", + ability.effect + ); + let sub_ability = ability + .sub_ability + .as_ref() + .expect("expected a conditional sub_ability for the trailing sacrifice sentence"); + assert!( + sub_ability.condition.is_some(), + "expected the 'if there are no mining counters' gate to survive as a condition" + ); + let Effect::Sacrifice { target, .. } = &*sub_ability.effect else { + panic!("expected Effect::Sacrifice, got {:?}", sub_ability.effect); + }; + assert_eq!( + *target, + TargetFilter::SelfRef, + "sacrifice target must bind to the source land, not an unestablished ParentTarget" + ); +} + #[test] fn parses_return_forest_cost_untap_activated_ability() { let r = parse(