diff --git a/crates/engine/src/parser/oracle_cost.rs b/crates/engine/src/parser/oracle_cost.rs index 9a23ee7b84..0ac10f3b6c 100644 --- a/crates/engine/src/parser/oracle_cost.rs +++ b/crates/engine/src/parser/oracle_cost.rs @@ -14,11 +14,12 @@ use super::oracle_nom::primitives::{scan_contains, split_once_on}; use super::oracle_nom::quantity as nom_quantity; use super::oracle_nom::target::parse_cost_self_reference; use super::oracle_static::parse_dynamic_x_clause; -use super::oracle_target::{parse_target, parse_type_phrase}; +use super::oracle_target::{distribute_shared_properties, parse_target, parse_type_phrase}; use super::oracle_util::parse_count_expr; use super::oracle_util::parse_creature_subtype; use super::oracle_util::parse_mana_symbols; use super::oracle_util::parse_number; +use super::oracle_util::CountWord; use super::oracle_util::TextPair; use crate::types::ability::{ AbilityCost, AggregateFunction, BeholdCostAction, ChoiceType, Comparator, ControllerRef, @@ -1031,19 +1032,32 @@ pub fn parse_single_cost(text: &str) -> AbilityCost { value((), alt((tag("tap "), tag("tapped ")))).parse(i) }) { let tap_lower = tap_rest.to_lowercase(); - let (count, filter_text) = if let Some(((), r)) = nom_on_lower(tap_rest, &tap_lower, |i| { - value( - (), - alt((tag("another untapped "), tag("an untapped "), tag("an "))), - ) - .parse(i) - }) { - (1u32, r.to_lowercase()) - } else if let Some(((), r)) = nom_on_lower(tap_rest, &tap_lower, |i| { + // The leading quantifier reports a typed `CountWord` alongside the + // count. "Another"/"other" is not merely a quantity of one: it is the + // source-exclusion qualifier, and this branch CONSUMES it, so the + // remainder handed to `parse_target` no longer carries it. Without the + // signal the exclusion is lost and the source pays its own cost + // (Spire Mechcycle, #7522). Same failure and same typed remedy as the + // sacrifice imperative's `parse_count_expr_with_exclusion` (#4513). + let (count, filter_text, count_word) = if let Some((word, r)) = + nom_on_lower(tap_rest, &tap_lower, |i| { + alt(( + value(CountWord::SourceExclusion, tag("another untapped ")), + value(CountWord::Plain, tag("an untapped ")), + value(CountWord::Plain, tag("an ")), + )) + .parse(i) + }) { + (1u32, r.to_lowercase(), word) + } else if let Some((word, r)) = nom_on_lower(tap_rest, &tap_lower, |i| { // "X untapped [type]" — variable count, use u32::MAX as sentinel. - value((), alt((tag("x untapped "), tag("x other untapped ")))).parse(i) + alt(( + value(CountWord::Plain, tag("x untapped ")), + value(CountWord::SourceExclusion, tag("x other untapped ")), + )) + .parse(i) }) { - (u32::MAX, r.to_lowercase()) + (u32::MAX, r.to_lowercase(), word) } else if let Some((n, r)) = super::oracle_util::parse_number(&tap_lower) { let r = nom_on_lower( &tap_rest[tap_lower.len() - r.len()..], @@ -1052,15 +1066,26 @@ pub fn parse_single_cost(text: &str) -> AbilityCost { ) .map(|((), rest)| rest.to_lowercase()) .unwrap_or_else(|| r.trim_start().to_string()); - (n, r) + // The numeric branch does NOT consume "other" ("tap two other + // untapped artifacts you control"): the `untapped ` tag fails on + // the "other " lead, so the phrase reaches `parse_target` intact + // and `parse_type_phrase` supplies `FilterProp::Another` itself. + // Nothing to re-apply here. + (n, r, CountWord::Plain) } else { - (0, String::new()) + (0, String::new(), CountWord::Plain) }; if count > 0 { let target_text = format!("target {filter_text}"); let (filter, remainder) = parse_target(&target_text); if remainder.trim().is_empty() { + let filter = match count_word { + CountWord::SourceExclusion => { + distribute_shared_properties(filter, &[FilterProp::Another]) + } + CountWord::Plain => filter, + }; return AbilityCost::TapCreatures { requirement: TapCreaturesRequirement::count(count), filter, @@ -1535,21 +1560,7 @@ fn ensure_another_sacrifice_filter(filter: TargetFilter, phrase: &str) -> Target if !has_another_prefix { return filter; } - match filter { - TargetFilter::Typed(mut typed) => { - if !typed.properties.contains(&FilterProp::Another) { - typed.properties.push(FilterProp::Another); - } - TargetFilter::Typed(typed) - } - TargetFilter::Or { filters } => TargetFilter::Or { - filters: filters - .into_iter() - .map(|f| ensure_another_sacrifice_filter(f, phrase)) - .collect(), - }, - other => other, - } + distribute_shared_properties(filter, &[FilterProp::Another]) } /// CR 117.1 + CR 601.2b + CR 107.4a/107.4e/202.1: Parse Baron Helmut Zemo's @@ -2346,6 +2357,102 @@ mod tests { ); } + /// CR 602.2b + CR 601.2h + CR 118.3: the source-exclusion "another" in a + /// `TapCreatures` activation cost must survive into the cost filter, so the + /// ability's source cannot pay an activation cost that requires another + /// untapped creature (Spire Mechcycle, #7522). + /// + /// Table-driven over the printed shapes the tap-cost grammar distinguishes, + /// with both counter-directions: an ordinary article and a plain numeric + /// count must NOT gain the exclusion (a standalone tap cost with no + /// "another" does include the source). + /// + /// The "two other untapped" row was already green before this fix: the + /// numeric branch never consumes "other", so the phrase reaches + /// `parse_type_phrase` intact and it supplies the property. That row pins + /// the path; it is not evidence for the fix. + #[test] + fn tap_cost_another_carries_the_source_exclusion() { + let cases: &[(&str, bool)] = &[ + ("Tap another untapped Merfolk you control", true), + ( + "Tap another untapped creature you control with flying", + true, + ), + ("Tap two other untapped artifacts you control", true), + ("Tap an untapped Merfolk you control", false), + ("Tap three untapped Merfolk you control", false), + ]; + for (text, excluded) in cases { + let AbilityCost::TapCreatures { filter, .. } = parse_oracle_cost(text) else { + panic!("{text:?} must parse to a TapCreatures cost"); + }; + let TargetFilter::Typed(typed) = &filter else { + panic!("{text:?} must parse to a typed filter, got {filter:?}"); + }; + assert_eq!( + typed.properties.contains(&FilterProp::Another), + *excluded, + "{text:?} exclusion mismatch, got {:?}", + typed.properties + ); + } + } + + /// Spire Mechcycle (#7522): "Tap another untapped Mount or Vehicle you + /// control" — the exclusion must land on EVERY leg of the disjunction. The + /// Mechcycle is itself a Vehicle, so it matches the SECOND leg; marking + /// only the first would still let it pay its own cost. + #[test] + fn tap_cost_another_marks_every_leg_of_a_disjunction() { + let AbilityCost::TapCreatures { filter, .. } = + parse_oracle_cost("Tap another untapped Mount or Vehicle you control") + else { + panic!("expected a TapCreatures cost"); + }; + let TargetFilter::Or { filters } = &filter else { + panic!("expected an Or filter for 'Mount or Vehicle', got {filter:?}"); + }; + assert_eq!(filters.len(), 2, "expected two legs, got {filters:?}"); + for leg in filters { + let TargetFilter::Typed(typed) = leg else { + panic!("expected typed legs, got {leg:?}"); + }; + assert!( + typed.properties.contains(&FilterProp::Another), + "every leg must carry the exclusion, got {typed:?}" + ); + } + } + + /// CR 602.2b + CR 118.3: shared source exclusion must flow through the + /// `And` shape for "creature you control but don't own" without changing + /// the negated ownership leg. + #[test] + fn tap_cost_another_preserves_exclusion_in_conjunctive_filter() { + let AbilityCost::TapCreatures { filter, .. } = + parse_oracle_cost("Tap another untapped creature you control but don't own") + else { + panic!("expected a TapCreatures cost"); + }; + let TargetFilter::And { filters } = filter else { + panic!("expected an And filter, got {filter:?}"); + }; + assert!(matches!( + filters.first(), + Some(TargetFilter::Typed(TypedFilter { properties, .. })) + if properties.contains(&FilterProp::Another) + )); + assert!(matches!( + filters.get(1), + Some(TargetFilter::Not { filter }) if matches!( + filter.as_ref(), + TargetFilter::Typed(TypedFilter { properties, .. }) + if !properties.contains(&FilterProp::Another) + ) + )); + } + #[test] fn cost_unattach_this_equipment() { assert_eq!( diff --git a/crates/engine/src/parser/oracle_target.rs b/crates/engine/src/parser/oracle_target.rs index a8af054692..58839ab654 100644 --- a/crates/engine/src/parser/oracle_target.rs +++ b/crates/engine/src/parser/oracle_target.rs @@ -4274,7 +4274,10 @@ fn stack_spell_filter(mut typed: TypedFilter) -> TargetFilter { } } -fn distribute_shared_properties(filter: TargetFilter, shared_props: &[FilterProp]) -> TargetFilter { +pub(super) fn distribute_shared_properties( + filter: TargetFilter, + shared_props: &[FilterProp], +) -> TargetFilter { match filter { TargetFilter::Typed(mut typed) => { for prop in shared_props { diff --git a/crates/engine/src/parser/oracle_util.rs b/crates/engine/src/parser/oracle_util.rs index 1ec8ad617b..814e32db8e 100644 --- a/crates/engine/src/parser/oracle_util.rs +++ b/crates/engine/src/parser/oracle_util.rs @@ -708,7 +708,7 @@ pub(crate) fn rewrite_quantity_expr_rounding(expr: &mut QuantityExpr, mode: Roun } } -/// Typed signal distinguishing which count-word `parse_count_expr` consumed. +/// Typed signal distinguishing which count-word a quantifier grammar consumed. /// /// The numeric value of a count is the same whether the text said "a", "an", /// "1", "any", or "another" — all yield `QuantityExpr::Fixed { value: 1 }`. But @@ -718,6 +718,11 @@ pub(crate) fn rewrite_quantity_expr_rounding(expr: &mut QuantityExpr, mode: Roun /// distinguish the exclusion word from an ordinary article without re-matching /// the raw string at the call site (CLAUDE.md forbids stringly-typed dispatch). /// This enum is that typed signal. +/// +/// Every grammar that consumes the qualifier reports it, not just +/// [`parse_count_expr_with_exclusion`]: `parse_oracle_cost`'s tap-cost branch +/// reports it from its own leading-quantifier `alt` ("tap another untapped +/// Merfolk you control", #7522). #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) enum CountWord { /// The count word was the source-exclusion "another" — the consuming caller diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index a688071dd7..03a2090598 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -1021,6 +1021,7 @@ mod swans_prevention_followup; mod swarm_combat_witness; mod tales_of_the_ancestors_catch_up_draw; mod talon_gates_from_hand_activation; +mod tap_cost_another_self_exclusion; mod targeted_exchange_preview_budget; mod tchaka_venerable_king; mod teamwork_aggregate_legal_actions; diff --git a/crates/engine/tests/integration/tap_cost_another_self_exclusion.rs b/crates/engine/tests/integration/tap_cost_another_self_exclusion.rs new file mode 100644 index 0000000000..a2c9c0f354 --- /dev/null +++ b/crates/engine/tests/integration/tap_cost_another_self_exclusion.rs @@ -0,0 +1,141 @@ +//! CR 602.2b + CR 601.2h + CR 118.3: a `"Tap another untapped … you control"` activation +//! cost excludes the ability's own source (#7522). +//! +//! Spire Mechcycle's exhaust cost reads "Tap another untapped Mount or Vehicle +//! you control", and the Mechcycle is itself a Vehicle — before the fix the +//! parsed cost filter carried no `FilterProp::Another`, so the source was an +//! eligible payment for its own ability. +//! +//! The runtime was never the defect. `has_enough_tap_creatures` +//! (`game/cost_payability.rs`) evaluates the cost filter against a +//! `FilterContext::from_source`, so `FilterProp::Another` is honoured the +//! moment the parser emits it; its separate `exclude_source` flag belongs to +//! composite `{T}` costs and is untouched here. These tests therefore drive the +//! real payability gate (`ai_support::legal_actions`), not the parser. +//! +//! Card text is built from Oracle text rather than named cards, so the tests +//! run in CI (which has no card database). +//! +//! Not covered: "other than this creature" tail forms (Impelled Giant, +//! Mossbridge Troll) reach the exclusion through a different grammar and were +//! already correct; the subtype disjunction (Spire Mechcycle's "Mount or +//! Vehicle") is pinned at the parser layer in +//! `parser::oracle_cost::tests::tap_cost_another_marks_every_leg_of_a_disjunction`, +//! because `GameScenario` has no helper that stamps a Vehicle subtype. + +use engine::game::scenario::{GameRunner, GameScenario, P0}; +use engine::types::ability::{AbilityCost, Effect}; +use engine::types::actions::GameAction; +use engine::types::identifiers::ObjectId; +use engine::types::phase::Phase; + +/// The reported shape: the cost excludes the source. +const ANOTHER: &str = + "Tap another untapped creature you control: This creature gains indestructible until end of turn."; + +/// Counter-direction: no "another", so the source remains eligible. +const PLAIN: &str = + "Tap an untapped creature you control: This creature gains indestructible until end of turn."; + +/// One creature carrying `oracle` plus `helpers` vanilla untapped creatures, +/// all controlled by P0, with P0 holding priority in its precombat main phase. +fn board(oracle: &str, helper_count: usize) -> (GameRunner, ObjectId, Vec) { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + let source = scenario + .add_creature_from_oracle(P0, "Tapper", 2, 2, oracle) + .id(); + let helpers = (0..helper_count) + .map(|i| scenario.add_creature(P0, &format!("Helper {i}"), 1, 1).id()) + .collect(); + (scenario.build(), source, helpers) +} + +/// Does the engine offer `source`'s first activated ability right now? +fn offers_activation(runner: &GameRunner, source: ObjectId) -> bool { + engine::ai_support::legal_actions(runner.state()) + .iter() + .any(|action| { + matches!( + action, + GameAction::ActivateAbility { + source_id, + ability_index: 0, + } if *source_id == source + ) + }) +} + +/// The defect: alone on the battlefield, the source matched its own cost filter +/// and the ability was offered — it would have paid by tapping itself. +/// +/// The reach guards below prove that ability 0 was published as a concrete +/// `TapCreatures` cost. Without them, a parser failure could make both negative +/// assertions pass without exercising the source-exclusion behavior. +/// +/// Reverting the fix flips this test to red: `assert!(!offers_activation(…))` +/// fails, and the `Err` expectation below becomes `Ok`. +#[test] +fn the_source_alone_cannot_pay_its_own_tap_another_cost() { + let (mut runner, source, _) = board(ANOTHER, 0); + let ability = runner.state().objects[&source] + .abilities + .first() + .expect("the source must publish ability 0"); + assert!( + matches!(&ability.cost, Some(AbilityCost::TapCreatures { .. })), + "ability 0 must publish a TapCreatures cost, got {:?}", + ability.cost + ); + assert!( + !matches!(ability.effect.as_ref(), Effect::Unimplemented { .. }), + "ability 0 must not lower to Effect::Unimplemented, got {:?}", + ability.effect + ); + assert!( + !offers_activation(&runner, source), + "a lone source must not be offered its own \"tap another untapped creature\" ability" + ); + assert!( + runner + .act(GameAction::ActivateAbility { + source_id: source, + ability_index: 0, + }) + .is_err(), + "activating anyway must be rejected — the source may not tap itself for the cost" + ); +} + +/// Positive counter-direction: with a second untapped creature the cost is +/// payable and the ability is offered. Guards against over-suppression, the +/// expensive collateral of a self-exclusion fix. +#[test] +fn a_second_untapped_creature_pays_the_tap_another_cost() { + let (mut runner, source, helpers) = board(ANOTHER, 1); + assert!( + offers_activation(&runner, source), + "another untapped creature makes the cost payable" + ); + let helper = helpers[0]; + runner.activate(source, 0).pay_with(&[helper]).resolve(); + assert!( + runner.state().objects[&helper].tapped, + "the selected helper must be tapped by the real activation cost payment" + ); + assert!( + !runner.state().objects[&source].tapped, + "the source must remain untapped; only another creature pays this cost" + ); +} + +/// A standalone `TapCreatures` cost with no "another" includes the source. +/// This is the behaviour the fix must not break. +#[test] +fn a_plain_tap_cost_still_includes_the_source() { + let (runner, source, _) = board(PLAIN, 0); + assert!( + offers_activation(&runner, source), + "\"tap an untapped creature you control\" is payable by the source itself" + ); +}