Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
165 changes: 136 additions & 29 deletions crates/engine/src/parser/oracle_cost.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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()..],
Expand All @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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!(
Expand Down
5 changes: 4 additions & 1 deletion crates/engine/src/parser/oracle_target.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
7 changes: 6 additions & 1 deletion crates/engine/src/parser/oracle_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
1 change: 1 addition & 0 deletions crates/engine/tests/integration/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
141 changes: 141 additions & 0 deletions crates/engine/tests/integration/tap_cost_another_self_exclusion.rs
Original file line number Diff line number Diff line change
@@ -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<ObjectId>) {
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"
);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

/// 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"
);
}
Loading