diff --git a/crates/engine/src/game/filter.rs b/crates/engine/src/game/filter.rs index daa5e36d6b..98772acb1f 100644 --- a/crates/engine/src/game/filter.rs +++ b/crates/engine/src/game/filter.rs @@ -2386,6 +2386,58 @@ pub fn matches_target_filter_in_owner_zone( ) } +/// CR 400.3: the zones whose membership is keyed by OWNER rather than controller — +/// "If an object would go to any library, graveyard, or hand other than its owner's, +/// it goes to its owner's corresponding zone." The rule enumerates the partition +/// itself; this predicate is that enumeration and nothing more. +/// +/// Why ownership is the correct scope for a `ControllerRef::You` filter there: +/// CR 108.4 + CR 108.4a — a card has a controller only when it represents a +/// permanent or spell; if it has no controller, use its owner instead. A card in a +/// hand, library, or graveyard is neither, so CR 109.5 routes "you"/"your" to its +/// owner. Thus "your graveyard" is an ownership claim even though the parser +/// represents its player scope as `ControllerRef::You`. +/// +/// EXILE IS DELIBERATELY EXCLUDED, and CR 400.3 excludes it too — the rule names +/// library, graveyard, and hand, not exile. The engine matches exiled objects +/// against their AT-EXILE controller via `effective_controller`'s LKI fallback, +/// which the Oversimplify class depends on ("creatures they controlled that were +/// exiled this way" is keyed on who controlled the object when it left, not on who +/// owns it now). Substituting ownership there would break that class. +/// +/// The single authority for this partition: `game::targeting::add_zone_targets` +/// (target enumeration) and `game::off_zone_characteristics` (off-zone keyword +/// grants) both route through it, so the two cannot drift on which zones are +/// owner-scoped. +pub fn is_owner_scoped_zone(zone: Zone) -> bool { + matches!(zone, Zone::Hand | Zone::Library | Zone::Graveyard) +} + +/// CR 400.3 + CR 109.5 + CR 108.4a: match `object_id` against `filter` using the +/// ownership semantics of the zone it is being enumerated from. +/// +/// The single entry point for "evaluate this filter against an object in zone Z". +/// In an owner-scoped zone (see [`is_owner_scoped_zone`]) this delegates to +/// [`matches_target_filter_in_owner_zone`], so a stale `obj.controller` left behind +/// by a control-change effect cannot exclude the object from its own owner's +/// player-scoped query — the state `effects::change_zone` documents for a stolen +/// creature that dies into its owner's graveyard, where `reset_for_battlefield_exit` +/// leaves `controller = thief`. Everywhere else it delegates to the ordinary +/// controller-scoped [`matches_target_filter`]. +pub fn matches_target_filter_for_zone( + state: &GameState, + object_id: ObjectId, + zone: Zone, + filter: &TargetFilter, + ctx: &FilterContext<'_>, +) -> bool { + if is_owner_scoped_zone(zone) { + matches_target_filter_in_owner_zone(state, object_id, filter, ctx) + } else { + matches_target_filter(state, object_id, filter, ctx) + } +} + pub fn matches_target_filter_on_battlefield_entry( state: &GameState, event: &ProposedEvent, diff --git a/crates/engine/src/game/off_zone_characteristics.rs b/crates/engine/src/game/off_zone_characteristics.rs index 77d47ec710..956fa1d618 100644 --- a/crates/engine/src/game/off_zone_characteristics.rs +++ b/crates/engine/src/game/off_zone_characteristics.rs @@ -1,6 +1,4 @@ -use crate::game::filter::{ - matches_target_filter, matches_target_filter_in_owner_zone, FilterContext, -}; +use crate::game::filter::FilterContext; use crate::game::layers::{ active_continuous_effects_from_base_static_source, active_effect_condition_controller, collect_shared_active_continuous_effects, evaluate_condition_with_recipient, @@ -198,6 +196,11 @@ pub(crate) fn collect_applicable_off_zone_keyword_effects( .collect() } +/// CR 109.5 + CR 400.3: "your" cards in hand/library/graveyard are scoped by owner, +/// not by a stale object controller/LKI. Delegates to +/// `filter::matches_target_filter_for_zone`, the single authority for that +/// partition, so this path and target enumeration in `game::targeting` cannot drift +/// on which zones are owner-scoped. fn matches_off_zone_keyword_recipient( state: &GameState, object_id: ObjectId, @@ -205,17 +208,7 @@ fn matches_off_zone_keyword_recipient( filter: &TargetFilter, ctx: &FilterContext<'_>, ) -> bool { - if is_owner_scoped_zone(zone) { - matches_target_filter_in_owner_zone(state, object_id, filter, ctx) - } else { - matches_target_filter(state, object_id, filter, ctx) - } -} - -fn is_owner_scoped_zone(zone: Zone) -> bool { - // CR 109.5 + CR 400.3: "your" cards in hand/library/graveyard are scoped - // by owner, not stale object controller/LKI. - matches!(zone, Zone::Hand | Zone::Library | Zone::Graveyard) + crate::game::filter::matches_target_filter_for_zone(state, object_id, zone, filter, ctx) } fn supports_off_zone_keyword_query(modification: &ContinuousModification) -> bool { diff --git a/crates/engine/src/game/targeting.rs b/crates/engine/src/game/targeting.rs index bbfdcf0aaa..c05a4d9a50 100644 --- a/crates/engine/src/game/targeting.rs +++ b/crates/engine/src/game/targeting.rs @@ -305,6 +305,7 @@ fn find_legal_targets_with_context( } Zone::Exile => add_zone_targets( state, + Zone::Exile, state.exile.iter().copied(), filter, target_ctx, @@ -315,6 +316,7 @@ fn find_legal_targets_with_context( for player in &state.players { add_zone_targets( state, + Zone::Graveyard, player.graveyard.iter().copied(), filter, target_ctx, @@ -327,6 +329,7 @@ fn find_legal_targets_with_context( for player in &state.players { add_zone_targets( state, + Zone::Hand, player.hand.iter().copied(), filter, target_ctx, @@ -339,6 +342,7 @@ fn find_legal_targets_with_context( for player in &state.players { add_zone_targets( state, + Zone::Library, player.library.iter().copied(), filter, target_ctx, @@ -1009,9 +1013,26 @@ fn target_ref_matches_resolved_filter_with_context( TargetRef::Object(id) if state.stack.iter().any(|entry| entry.id == *id) => { super::filter::matches_stack_target_filter(state, *id, target_filter, ctx) } - TargetRef::Object(id) => { - super::filter::matches_target_filter(state, *id, target_filter, ctx) - } + // CR 109.5 + CR 108.4 + CR 108.4a + CR 400.3: RE-VALIDATION must use the same + // ownership semantics as enumeration, or a target that was legal when chosen + // becomes illegal when the spell resolves. Unlike the battlefield scans in + // this file, an explicit target can live in ANY zone, so the zone is read off + // the object rather than assumed — `matches_target_filter_for_zone` then + // owner-scopes hand/library/graveyard and leaves battlefield and exile on + // controller matching, exactly as `add_zone_targets` does at selection time. + // Keeping the two seams on one authority is the point: while enumeration was + // owner-scoped and this check was not, a card in its owner's graveyard with a + // stale controller could be selected and then fizzle on resolution. + TargetRef::Object(id) => match state.objects.get(id) { + Some(obj) => super::filter::matches_target_filter_for_zone( + state, + *id, + obj.zone, + target_filter, + ctx, + ), + None => false, + }, TargetRef::Player(player) => super::filter::player_matches_target_filter_in_state( state, target_filter, @@ -2030,8 +2051,28 @@ fn stack_entry_controller_matches( } } +/// Enumerate legal targets among `object_ids`, all of which are being read out of +/// `zone`. +/// +/// CR 109.5 + CR 108.4 + CR 108.4a + CR 400.3: `zone` is not bookkeeping — it selects +/// the ownership semantics the filter is evaluated under, via +/// `filter::matches_target_filter_for_zone`. A player-scoped query on a hand, +/// library, or graveyard ("target creature card from YOUR graveyard") is an +/// ownership claim as a matter of rule: a card has a controller only when it +/// represents a permanent or spell, and CR 108.4a uses the owner when it has none. +/// Cards in those zones are neither, so CR 109.5 resolves "your" to the owner. +/// CR 400.3 fixes which zones those are. +/// +/// Matching them against `obj.controller` excluded a card from its OWN owner's +/// query whenever a control-change effect left a stale controller behind — the +/// state `effects::change_zone` documents for a creature stolen via Mind Control +/// that dies into its owner's graveyard, where `reset_for_battlefield_exit` does +/// not reset controller and the layer pass that would skips objects off the +/// battlefield. Exile keeps controller matching deliberately; see +/// `filter::is_owner_scoped_zone` for why. fn add_zone_targets( state: &GameState, + zone: Zone, object_ids: impl IntoIterator, filter: &TargetFilter, target_ctx: &super::filter::FilterContext, @@ -2049,7 +2090,7 @@ fn add_zone_targets( let source_ignores_hexproof = require_full_targeting && crate::game::static_abilities::player_ignores_hexproof(state, source_controller); for obj_id in object_ids { - if super::filter::matches_target_filter(state, obj_id, filter, target_ctx) { + if super::filter::matches_target_filter_for_zone(state, obj_id, zone, filter, target_ctx) { let obj = match state.objects.get(&obj_id) { Some(o) => o, None => continue, @@ -5189,6 +5230,111 @@ mod tests { ); } + /// CR 109.5 + CR 108.4 + CR 108.4a + CR 400.3: a player-scoped query on an + /// owner-scoped zone follows OWNERSHIP, and — the point of this test — it does so + /// identically at both seams. + /// + /// Selection (`find_legal_targets`) and resolution-time re-validation + /// (`resolved_object_ids_for_filter`, via + /// `target_ref_matches_resolved_filter_with_context`) are separate code paths that + /// must agree, or a target legally chosen on announcement becomes illegal on + /// resolution and the spell fizzles. Fixing only enumeration would leave exactly + /// that split, so both are asserted here on one state. + /// + /// The fixture stages the divergence CR 400.3 makes reachable: a card goes to its + /// OWNER's graveyard, while `reset_for_battlefield_exit` leaves a stale + /// `controller` behind from a control-change effect. So `mine` (owner P0, + /// controller P1) is in P0's graveyard and must match "creature card in YOUR + /// graveyard"; `theirs` (owner P1, controller P0) is in P1's graveyard and must + /// not — under controller matching the two verdicts invert exactly. + #[test] + fn owner_scoped_zone_query_agrees_across_selection_and_resolution() { + let mut state = GameState::new_two_player(42); + + let mut graveyard_creature = + |card: u64, owner: PlayerId, controller: PlayerId, name: &str| { + let id = create_object( + &mut state, + CardId(card), + owner, + name.to_string(), + Zone::Graveyard, + ); + let obj = state.objects.get_mut(&id).expect("fixture present"); + obj.card_types.core_types.push(CoreType::Creature); + obj.controller = controller; + id + }; + let mine = graveyard_creature(1, PlayerId(0), PlayerId(1), "My Stolen Bear"); + let theirs = graveyard_creature(2, PlayerId(1), PlayerId(0), "Their Stolen Bear"); + + // Premise: owner and controller really do diverge on both fixtures, so + // neither verdict below can be produced by a state where they coincide. + for (id, owner, controller) in [ + (mine, PlayerId(0), PlayerId(1)), + (theirs, PlayerId(1), PlayerId(0)), + ] { + let obj = &state.objects[&id]; + assert_eq!(obj.owner, owner); + assert_eq!(obj.controller, controller); + } + + // "target creature card in your graveyard", as the parser represents it: + // the player scope rides `ControllerRef::You`, and the ZONE decides that it + // is read as ownership. + let filter = TargetFilter::Typed( + TypedFilter::creature() + .controller(ControllerRef::You) + .properties(vec![FilterProp::InZone { + zone: Zone::Graveyard, + }]), + ); + + let source = create_object( + &mut state, + CardId(3), + PlayerId(0), + "Reanimation Spell".to_string(), + Zone::Stack, + ); + + // Seam 1 — selection. + let selectable = find_legal_targets(&state, &filter, PlayerId(0), source); + assert!( + selectable.contains(&TargetRef::Object(mine)), + "a card YOU OWN in your graveyard must be selectable despite a stale \ + opponent controller: {selectable:?}" + ); + assert!( + !selectable.contains(&TargetRef::Object(theirs)), + "a card an OPPONENT OWNS must not be selectable however it is \ + controlled: {selectable:?}" + ); + + // Seam 2 — resolution-time re-validation of an already-chosen target. + let resolved = resolved_object_ids_for_filter( + &state, + &make_resolved_with_targets(vec![TargetRef::Object(mine)], source), + &filter, + ); + assert!( + resolved.contains(&mine), + "the selected owner-scoped target must survive re-validation rather than \ + fizzling: {resolved:?}" + ); + + let resolved_foreign = resolved_object_ids_for_filter( + &state, + &make_resolved_with_targets(vec![TargetRef::Object(theirs)], source), + &filter, + ); + assert!( + !resolved_foreign.contains(&theirs), + "re-validation must not admit an opponent-owned card that selection \ + refused: {resolved_foreign:?}" + ); + } + fn make_resolved_with_targets( targets: Vec, source: ObjectId, diff --git a/crates/engine/src/parser/oracle.rs b/crates/engine/src/parser/oracle.rs index 8edb802c04..beb2a71df3 100644 --- a/crates/engine/src/parser/oracle.rs +++ b/crates/engine/src/parser/oracle.rs @@ -4,8 +4,7 @@ use crate::parser::oracle_nom::error::{OracleError, OracleResult}; use nom::branch::alt; use nom::bytes::complete::{tag, take_until, take_while}; use nom::character::complete::multispace0; -use nom::combinator::{all_consuming, opt, recognize, value}; -use nom::multi::many1; +use nom::combinator::{all_consuming, opt, value}; use nom::sequence::{preceded, terminated}; use nom::Parser; use serde::{Deserialize, Serialize}; @@ -34,8 +33,8 @@ use crate::types::zones::Zone; use super::oracle_nom::bridge::{nom_on_lower, split_once_on_lower}; use super::oracle_nom::condition::parse_graveyard_keyword_grant_sentence; use super::oracle_nom::primitives::{ - parse_number as nom_parse_number, parse_object_recipient_pronoun, scan_at_word_boundaries, - scan_contains, scan_preceded, + parse_number as nom_parse_number, parse_object_recipient_pronoun, parse_period_sentences, + scan_at_word_boundaries, scan_contains, scan_preceded, }; use super::oracle_attraction::parse_attraction_visit_triggers; @@ -54,7 +53,7 @@ use super::oracle_classifier::{ is_instead_replacement_line, is_opening_hand_begin_game, is_pay_life_as_colored_mana_pattern, is_replacement_pattern, is_spells_alternative_cost_pattern, is_static_pattern, is_vehicle_tier_line, lower_starts_with, should_defer_spell_to_effect, - split_flashback_trailing_self_spell_cost_reduction, + split_flashback_trailing_self_spell_cost_reduction, strip_entry_this_way_riders, }; use super::oracle_condition::parse_restriction_condition; use super::oracle_cost::{parse_oracle_cost, parse_single_cost, try_parse_cost_reduction}; @@ -250,16 +249,14 @@ fn parse_replacement_sentence_sequence_ir( Some(replacements) } +/// Split a replacement line into its period-terminated sentences, requiring the +/// line to be fully consumed (a trailing unterminated fragment rejects the whole +/// line, so the multi-sentence replacement path never sees a partial tail). +/// +/// Segmentation itself is delegated to `oracle_nom::primitives::parse_period_sentences`, +/// the single authority shared with `oracle_classifier::strip_entry_this_way_riders`. fn parse_replacement_sentences(input: &str) -> OracleResult<'_, Vec<&str>> { - all_consuming(many1(parse_replacement_sentence)).parse(input) -} - -fn parse_replacement_sentence(input: &str) -> OracleResult<'_, &str> { - preceded( - multispace0, - recognize(terminated(take_until("."), tag("."))), - ) - .parse(input) + all_consuming(parse_period_sentences).parse(input) } // CR 100.2a / CR 903.5b: Deck-construction overrides like "A deck can have @@ -2676,7 +2673,20 @@ fn is_spell_resolution_instruction_line( } else { std::borrow::Cow::Borrowed(effect_lower.as_str()) }; - if is_static_pattern(&static_view) && !should_defer_spell_to_effect(&effect_lower) { + // CR 608.2c: head-scope this gate for the same reason `is_replacement_pattern` + // is head-scoped. `is_static_compound_pattern` classifies on + // `"enters with " && !"counter"` — tokens a reflexive "… this way" rider's + // CONSEQUENT supplies just as readily as the replacement tokens did, and this + // predicate short-circuits the spell path one branch EARLIER than the + // replacement one. Heroic Return survives today only because its rider happens + // to contain the word "counter"; a rider with a non-counter consequent ("… it + // enters with your choice of …", "… it enters with flying") would otherwise + // drop the head reanimation instruction. `None` (text unit is only riders) is + // not a static. + let static_head = strip_entry_this_way_riders(&static_view); + if static_head.as_deref().is_some_and(is_static_pattern) + && !should_defer_spell_to_effect(&effect_lower) + { return false; } @@ -5159,12 +5169,18 @@ pub(crate) fn parse_oracle_ir( // trigger and excludes it from this replacement interceptor. // CR 608.2c: "If a [type] enters this way, it enters with …" is a reflexive // conditional rider on a non-ETB trigger (Winter Soldier, Reborn Avenger), - // not a CR 614.1c enters-with replacement head. Skip the replacement - // interceptor so the line routes through trigger dispatch. + // not a CR 614.1c enters-with replacement head. The "enters with" token must + // therefore be sought in the HEAD instruction only, through the same + // `strip_entry_this_way_riders` authority the classifier uses. A literal + // `"enters this way,"` scan modelled just ONE grammatical voice of the rider + // class (present-tense, comma-terminated), so it still handed a passive-voice + // ("… is put onto the battlefield this way, …") or comma-less rider to the + // replacement interceptor and lost the head instruction. `None` (the line is + // only riders) has no head to intercept either. if has_trigger_prefix(&lower) && !is_enters_with_counter_trigger(&lower) - && scan_contains(&lower, "enters with") - && !scan_contains(&lower, "enters this way,") + && strip_entry_this_way_riders(&lower) + .is_some_and(|head| scan_contains(&head, "enters with")) { // CR 603.1 + CR 603.3 + CR 614.1c/614.12: "Whenever you cast [spell], // that [subject] enters with … counter(s) on it[, where X is …]" diff --git a/crates/engine/src/parser/oracle_classifier.rs b/crates/engine/src/parser/oracle_classifier.rs index eb717ae5b6..cc5e1b3506 100644 --- a/crates/engine/src/parser/oracle_classifier.rs +++ b/crates/engine/src/parser/oracle_classifier.rs @@ -6,6 +6,7 @@ use nom::combinator::{opt, peek, value, verify}; use nom::sequence::{preceded, terminated}; use nom::Parser; +use super::oracle_nom::condition::parse_reflexive_entry_this_way_rider; use super::oracle_nom::primitives as nom_primitives; use super::oracle_nom::primitives::scan_contains; use super::oracle_util::parse_mana_symbols; @@ -753,16 +754,96 @@ const REPLACEMENT_CONTAINS_PATTERNS: &[&str] = &[ "enters under the control of", ]; +/// CR 608.2c + CR 614.1c: return the HEAD instruction of `lower` — the text with +/// every reflexive battlefield-entry "… this way" rider sentence removed. +/// +/// A CR 608.2c rider is a back-reference to an instruction earlier in the same +/// ability. It routinely contributes the exact tokens CR 614.1c classification +/// keys on ("enters", "counter", "enters tapped", "enters under the control of"), +/// but those tokens belong to the back-reference, never to a replacement head. +/// The whole rider sentence is dropped, consequent included: the consequent's +/// tokens are the rider's, not the head's. +/// +/// CONSUMPTION: `parse_reflexive_entry_this_way_rider` is a PREFIX recognizer and +/// is deliberately NOT wrapped in `all_consuming` here. Its remainder is the +/// rider's own consequent (", it enters with two additional +1/+1 counters on +/// it."), so requiring full consumption would reject every real rider — the class +/// exists only because it has a consequent. Fail-closed behavior comes from the +/// recognizer's narrowness instead (an article-or-pronoun subject + a +/// battlefield-entry verb + `" this way"` is not a shape any CR 614.1c head can +/// take), pinned in both directions by +/// `a_rider_prefix_drops_its_whole_sentence_by_contract`. +/// +/// Segmentation uses `oracle_nom::primitives::split_sentence_units`, the total +/// wrapper over `parse_period_sentence` — the SAME combinator that feeds +/// `is_replacement_pattern` at its only sentence-scoped call site (`oracle.rs` via +/// `parse_replacement_sentence_sequence_ir`). A second, `split('.')`-based sentence +/// model would diverge from it in three ways that all matter here: `split` keeps +/// the leading space, drops the terminal '.', and emits an empty tail element — and +/// the residual is then fed to PREFIX-ANCHORED arms below +/// (`lower_starts_with(lower, "as ")`) that a leading space silently kills, and to +/// SUFFIX-anchored arms (`ends_with(" enter tapped")`) that need the period. +/// +/// `None` means the text unit is ONLY riders and therefore has no replacement head +/// at all. That case is reachable at sentence scope: Pharika's Spawn's second +/// sentence ("When it enters this way, each opponent sacrifices a non-Gorgon +/// creature of their choice.") is entirely a rider. +/// +/// `pub(crate)` because head-scoping is a property of CR 608.2c grammar, not of one +/// predicate: `oracle.rs` scopes the spell-line static gate and the Priority 5-pre +/// enters-with interceptor with this same function, so all three classification +/// gates share one model of "what is the head instruction". +pub(crate) fn strip_entry_this_way_riders(lower: &str) -> Option> { + let is_rider = |unit: &str| parse_reflexive_entry_this_way_rider(unit).is_ok(); + + let units = nom_primitives::split_sentence_units(lower); + let kept: Vec<&str> = units.iter().copied().filter(|u| !is_rider(u)).collect(); + if kept.len() == units.len() { + // Hot path: no rider anywhere, hand back the original with no allocation. + return Some(std::borrow::Cow::Borrowed(lower)); + } + + // Residual normalization: a single separating space and no leading/trailing + // whitespace, so prefix- and suffix-anchored arms still see an anchored string. + let joined = kept.join(" ").trim().to_string(); + if joined.is_empty() { + None + } else { + Some(std::borrow::Cow::Owned(joined)) + } +} + +/// CR 614.1c + CR 608.2c: classify the HEAD instruction, not the whole text unit. +/// +/// A reflexive battlefield-entry "… this way" rider (CR 608.2c) contributes +/// "enters"/"counter"/"enters tapped"/"enters under the control of" tokens that +/// belong to a back-reference, never to a replacement head. Scoping here — rather +/// than at one inner predicate — is required because SIX predicates below are +/// equally rider-contaminable: the `REPLACEMENT_CONTAINS_PATTERNS` scan alone +/// carries "enters the battlefield tapped", "enters tapped", "enters untapped" and +/// "enters under the control of". A text unit that is ONLY a rider has no head and +/// is not a replacement. +/// +/// This subsumes and replaces the former +/// `has_trigger_prefix(lower) && scan_contains(lower, "enters this way,")` early +/// return, which was dead at five of this function's six call sites (they all sit +/// behind a `has_trigger_prefix` gate) and, at the one live sentence-scoped site, +/// returned `false` for exactly the inputs the blank-residual rule now returns +/// `false` for. Regression context it carried: Winter Soldier, Reborn Avenger, +/// whose TRIGGER routing is decided one gate earlier by the Priority 5-pre +/// enters-with interceptor in `oracle.rs` — that gate is head-scoped by +/// `strip_entry_this_way_riders` too, so both classification gates now model the +/// rider class identically instead of one of them re-deriving it from a literal. pub(crate) fn is_replacement_pattern(lower: &str) -> bool { - if super::oracle_replacement::is_search_found_replacement_pattern(lower) { - return true; + match strip_entry_this_way_riders(lower) { + None => false, + Some(head) => is_replacement_pattern_head_scoped(&head), } +} - // CR 608.2c: reflexive "enters this way" riders on triggered abilities - // (Winter Soldier, Reborn Avenger) contain "enters" + "counter" but are - // not CR 614.1c ETB replacements. - if has_trigger_prefix(lower) && scan_contains(lower, "enters this way,") { - return false; +fn is_replacement_pattern_head_scoped(lower: &str) -> bool { + if super::oracle_replacement::is_search_found_replacement_pattern(lower) { + return true; } if is_counter_prohibition_replacement_pattern(lower) { @@ -820,17 +901,8 @@ fn is_replacement_compound_pattern(lower: &str) -> bool { if is_as_enters_becomes_choice_pattern(lower) { return true; } - // CR 614.1c: "enters with [counters]" replacement effects. The plural-subject - // forms ("Other creatures you control enter with …", "… creatures escape - // with …") use the bare-verb "enter"/"escape" rather than "enters"/"escapes", - // so accept both at word boundaries. Gated on "counter" so the bare verb - // alone never reclassifies a non-counter line. - if (scan_contains(lower, "enters") - || scan_contains(lower, "escapes") - || scan_contains(lower, "enter with") - || scan_contains(lower, "escape with")) - && scan_contains(lower, "counter") - { + // CR 614.1c: "enters with [counters]" replacement effects. + if has_enters_with_counter_tokens(lower) { return true; } if scan_contains(lower, "tapped for mana") && scan_contains(lower, "instead") { @@ -869,13 +941,30 @@ fn is_replacement_compound_pattern(lower: &str) -> bool { /// carries a fixed `count`), so this recognizer must NOT intercept them. Only /// the per-each *scaled* count, which the static mode cannot represent, routes /// to the dynamic-capable replacement (`PutCounter { count: QuantityExpr }`). +/// The line is head-scoped by `strip_entry_this_way_riders` for the same CR 608.2c +/// reason `is_replacement_pattern` is: a rider's "enters … counter … for each" +/// tokens describe the back-reference, not a replacement head. pub(crate) fn is_enters_with_counter_replacement_line(lower: &str) -> bool { + strip_entry_this_way_riders(lower).is_some_and(|head| { + has_enters_with_counter_tokens(&head) && scan_contains(&head, "for each") + }) +} + +/// CR 614.1c: token signature of an "enters/escapes with counters" replacement. +/// +/// The plural-subject forms ("Other creatures you control enter with …", +/// "… creatures escape with …") use the bare verb "enter"/"escape" rather than +/// "enters"/"escapes", so accept both at word boundaries. Gated on "counter" so +/// the bare verb alone never reclassifies a non-counter line. +/// +/// Shared by `is_replacement_pattern_head_scoped` and +/// `is_enters_with_counter_replacement_line` so the two cannot drift. +fn has_enters_with_counter_tokens(lower: &str) -> bool { (scan_contains(lower, "enters") || scan_contains(lower, "escapes") || scan_contains(lower, "enter with") || scan_contains(lower, "escape with")) && scan_contains(lower, "counter") - && scan_contains(lower, "for each") } /// CR 614.1c + CR 614.12: nom recognizer for the non-self "As a [filter] enters, @@ -1192,4 +1281,291 @@ mod tests { assert!(is_static_pattern(lower)); assert!(is_replacement_pattern(lower)); } + + // ------------------------------------------------------------------- + // CR 608.2c + CR 614.1c: reflexive battlefield-entry rider head-scoping + // ------------------------------------------------------------------- + + /// Heroic Return, printed line index 1 (verbatim, lowercased). + const HEROIC_RETURN_REANIMATION_LINE: &str = + "return target creature card from your graveyard to the battlefield. \ + if a hero enters this way, it enters with two additional +1/+1 counters on it."; + /// Recommission, printed line index 0 (verbatim, lowercased). + const RECOMMISSION_REANIMATION_LINE: &str = + "return target artifact or creature card with mana value 3 or less from your \ + graveyard to the battlefield. if a creature enters this way, it enters with \ + an additional +1/+1 counter on it."; + /// Pharika's Spawn, escape line — sentence 1 IS a genuine CR 614.1c head. + const PHARIKA_SENTENCE_1: &str = "this creature escapes with two +1/+1 counters on it."; + /// Pharika's Spawn — sentence 2 is entirely a rider (the blank-residual case, + /// and the one input the deleted `has_trigger_prefix` guard actually fired on). + const PHARIKA_SENTENCE_2: &str = + "when it enters this way, each opponent sacrifices a non-gorgon creature of their choice."; + /// Silver Surfer, Cosmic Voyager — rider sentence; TRUE today only via the + /// `REPLACEMENT_CONTAINS_PATTERNS` "enters tapped" literal. + const SILVER_SURFER_RIDER_SENTENCE: &str = "if a land enters this way, it enters tapped."; + /// Winter Soldier, Reborn Avenger — rider sentence; TRUE today via enters+counter. + const WINTER_SOLDIER_RIDER_SENTENCE: &str = + "if a hero enters this way, it enters with an additional +1/+1 counter on it."; + + /// V1: the two misparsing spell lines stop being classified as replacements, + /// while a genuine replacement head at the same (sentence) scope does not — + /// so a blanket-`false` regression cannot pass this test. + #[test] + fn reflexive_entry_rider_does_not_make_a_line_a_replacement() { + assert!(!is_replacement_pattern(HEROIC_RETURN_REANIMATION_LINE)); + assert!(!is_replacement_pattern(RECOMMISSION_REANIMATION_LINE)); + + // Non-vacuous positive: the head IS a replacement. + assert!(is_replacement_pattern(PHARIKA_SENTENCE_1)); + + // Rider-only text units have no head at all (blank-residual rule). These + // reproduce, at the one scope where it was live, the verdict of the + // deleted `has_trigger_prefix && "enters this way,"` guard. + assert!(!is_replacement_pattern(PHARIKA_SENTENCE_2)); + assert!(!is_replacement_pattern(SILVER_SURFER_RIDER_SENTENCE)); + assert!(!is_replacement_pattern(WINTER_SOLDIER_RIDER_SENTENCE)); + } + + /// V1: the trigger-prefixed full LINES the deleted guard covered keep their + /// `false` verdict, so no line-scope routing moved for that pair. + #[test] + fn trigger_prefixed_entry_rider_lines_stay_non_replacement() { + assert!(!is_replacement_pattern( + "whenever this creature attacks, return target creature card from your \ + graveyard to the battlefield. if a hero enters this way, it enters with \ + an additional +1/+1 counter on it." + )); + assert!(!is_replacement_pattern( + "when this creature enters, search your library for a land card, put it onto \ + the battlefield, then shuffle. if a land enters this way, it enters tapped." + )); + } + + /// V0c: residual normalization. A dropped rider must leave the surviving head + /// anchored (no leading space), or prefix-anchored arms below silently die. + #[test] + fn stripping_a_rider_leaves_the_head_anchored() { + let line = "as this creature is turned face up, draw a card. \ + if a creature enters this way, it enters tapped."; + let head = strip_entry_this_way_riders(line).expect("head survives"); + assert!( + lower_starts_with(&head, "as "), + "residual must stay prefix-anchored, got {head:?}" + ); + // The line is still a replacement via the "as … is turned face up" arm. + assert!(is_replacement_pattern(line)); + + // Zero-allocation hot path: a rider-free line is handed back borrowed. + assert!(matches!( + strip_entry_this_way_riders(PHARIKA_SENTENCE_1), + Some(std::borrow::Cow::Borrowed(_)) + )); + // A text unit that is ONLY a rider has no head. + assert!(strip_entry_this_way_riders(PHARIKA_SENTENCE_2).is_none()); + } + + /// V1b: Priority-7 routing keeps its class while becoming head-scoped. + #[test] + fn enters_with_counter_replacement_line_is_head_scoped() { + // Gev, Scaled Scorch (verbatim): tokens come from the HEAD, so + // over-stripping fails here. + const GEV_DISTRIBUTIVE_LINE: &str = + "other creatures you control enter with an additional +1/+1 counter on them \ + for each opponent who lost life this turn."; + assert!(is_enters_with_counter_replacement_line( + GEV_DISTRIBUTIVE_LINE + )); + + // Every token comes from the rider sentence — fails on revert to the + // whole-line form. + const RIDER_ONLY_FOR_EACH_LINE: &str = + "return target creature card from your graveyard to the battlefield. \ + if a hero enters this way, it enters with an additional +1/+1 counter on it \ + for each card in your graveyard."; + assert!(!is_enters_with_counter_replacement_line( + RIDER_ONLY_FOR_EACH_LINE + )); + + // The " for each " gate is preserved on the residual: an enters+counter + // head without it must stay out of the Priority-7 reroute. + assert!(!is_enters_with_counter_replacement_line(PHARIKA_SENTENCE_1)); + } + + /// The head-scoper covers the WHOLE rider class the combinator recognizes, + /// not the single present-tense/comma-terminated voice the two retired + /// literals modelled. Both `oracle.rs` gates (spell-line static, Priority + /// 5-pre enters-with) consume this function, so each voice below is a voice + /// those gates now scope off too. + #[test] + fn head_scoping_covers_every_rider_voice_the_literal_missed() { + // Passive voice: the retired literals scanned for "enters this way," and + // this text does not contain it, so both let the rider's tokens through. + const PASSIVE: &str = "return target creature card from your graveyard to the \ + battlefield. if a creature is put onto the battlefield this way, it enters \ + with an additional +1/+1 counter on it."; + assert!( + !scan_contains(PASSIVE, "enters this way,"), + "premise: the retired literal does not match the passive voice" + ); + let head = strip_entry_this_way_riders(PASSIVE).expect("head survives"); + assert!( + !scan_contains(&head, "enters with"), + "the passive rider must be scoped off the head, got {head:?}" + ); + + // Comma-less voice: the rider's clause ends at the period, not a comma — + // the SUBJECT is still clause-initial, which is the position + // `parse_entry_this_way_clause` recognizes. + const COMMA_LESS: &str = "return target creature card from your graveyard to the \ + battlefield. a hero enters this way."; + assert!( + !scan_contains(COMMA_LESS, "enters this way,"), + "premise: the retired literal does not match the comma-less voice" + ); + let head = strip_entry_this_way_riders(COMMA_LESS).expect("head survives"); + assert!( + !scan_contains(&head, "enters this way"), + "the comma-less rider must be scoped off the head, got {head:?}" + ); + + // Active "you put …" voice. + const ACTIVE: &str = "search your library for a land card and put it onto the \ + battlefield. if you put a land onto the battlefield this way, it enters with \ + a +1/+1 counter on it."; + assert!( + !scan_contains(ACTIVE, "enters this way,"), + "premise: the retired literal does not match the active voice" + ); + let head = strip_entry_this_way_riders(ACTIVE).expect("head survives"); + assert!( + !scan_contains(&head, "enters with"), + "the active-voice rider must be scoped off the head, got {head:?}" + ); + + // Non-vacuous: a genuine CR 614.1c head keeps its tokens in every case. + for genuine in [ + "this creature enters with two +1/+1 counters on it.", + "other creatures you control enter with an additional +1/+1 counter on them.", + ] { + let head = strip_entry_this_way_riders(genuine).expect("head survives"); + assert!( + scan_contains(&head, "enters with") || scan_contains(&head, "enter with"), + "a genuine head must keep its tokens, got {head:?}" + ); + } + } + + /// LOW-finding sibling gate: `is_static_pattern` is rider-contaminable through + /// exactly the same `enters with ` token, one branch EARLIER on the spell path + /// than `is_replacement_pattern`. `oracle.rs` head-scopes it with this same + /// function; this pins the verdict flip the head-scoping produces. + #[test] + fn static_classification_is_rider_contaminable_without_head_scoping() { + // A non-counter rider consequent: `is_static_compound_pattern` fires on + // `"enters with " && !"counter"`, which the rider alone supplies. + const NON_COUNTER_RIDER_LINE: &str = "return target creature card from your \ + graveyard to the battlefield. if a hero enters this way, it enters with \ + your choice of flying or vigilance."; + assert!( + is_static_pattern(NON_COUNTER_RIDER_LINE), + "premise: the un-scoped line classifies as a static — this is the gate \ + that dropped the head instruction" + ); + let head = strip_entry_this_way_riders(NON_COUNTER_RIDER_LINE).expect("head survives"); + assert!( + !is_static_pattern(&head), + "the head instruction alone is not a static, got {head:?}" + ); + + // Non-vacuous: a real static keeps its verdict through head-scoping. + const REAL_STATIC: &str = "creatures you control can't block."; + assert!(is_static_pattern(REAL_STATIC)); + assert!( + strip_entry_this_way_riders(REAL_STATIC).is_some_and(|head| is_static_pattern(&head)) + ); + } + + /// POSITION BOUNDARY: `parse_entry_this_way_clause` recognizes a rider only + /// CLAUSE-INITIALLY, so a trailing-position entry rider is deliberately left + /// unscoped. This test states that limit rather than implying coverage the + /// combinator does not have. + /// + /// The limit is safe because the trailing voice is UNPRINTED: a Scryfall regex + /// sweep for a sentence-final battlefield-entry back-reference + /// (`o:/(enters|enter|is put onto the battlefield|are put onto the battlefield) this way\./`) + /// returns zero cards. The shape that DOES print sentence-finally is the + /// second assertion below — a genuine CR 614.1c head whose trailing back-reference + /// is a NON-entry zone change — and that one must keep its tokens. + #[test] + fn head_scoping_leaves_the_unprinted_trailing_rider_voice_alone() { + // Trailing-position entry rider: synthetic, unprinted, and out of scope. + const TRAILING_RIDER: &str = "return target creature card from your graveyard to the \ + battlefield. it enters with an additional +1/+1 counter on it if a hero \ + enters this way."; + let head = strip_entry_this_way_riders(TRAILING_RIDER).expect("head survives"); + assert!( + scan_contains(&head, "enters with"), + "documented limit: a trailing-position rider is NOT scoped off the head, \ + got {head:?}" + ); + + // Arsenal Thresher (verbatim second sentence): a real CR 614.1c head with a + // trailing NON-entry back-reference. `ThisWayVerbScope::BattlefieldEntry` + // withholds "revealed", so this head keeps its tokens — the property a + // position-scanning recognizer would put at risk. + const ARSENAL_THRESHER_HEAD: &str = + "this creature enters with a +1/+1 counter on it for each card revealed this way."; + let head = strip_entry_this_way_riders(ARSENAL_THRESHER_HEAD).expect("head survives"); + assert!( + scan_contains(&head, "enters with"), + "a printed CR 614.1c head with a trailing non-entry back-reference must \ + keep its tokens, got {head:?}" + ); + } + + /// CONSUMPTION CONTRACT: `parse_reflexive_entry_this_way_rider` is a PREFIX + /// recognizer, and this consumer discards the WHOLE sentence on a prefix match. + /// That is the intended contract, not an oversight: the text after the comma is + /// the back-reference's own consequent, so its "enters"/"counter"/"tapped" + /// tokens are the rider's and never a CR 614.1c head's. Wrapping the recognizer + /// in `all_consuming` here would reject every real rider, since the class exists + /// only because it HAS a consequent. + /// + /// Fail-closed behavior comes from the recognizer's narrowness instead, pinned + /// in both directions below. + #[test] + fn a_rider_prefix_drops_its_whole_sentence_by_contract() { + // Prefix match → whole sentence gone, consequent included. + const RIDER_WITH_CONSEQUENT: &str = "return target creature card from your graveyard \ + to the battlefield. if a hero enters this way, it enters tapped and enters \ + under the control of an opponent."; + let head = strip_entry_this_way_riders(RIDER_WITH_CONSEQUENT).expect("head survives"); + assert!( + !scan_contains(&head, "enters tapped") + && !scan_contains(&head, "enters under the control of"), + "the consequent's tokens belong to the rider and must go with it, got {head:?}" + ); + assert!( + scan_contains(&head, "return target creature card"), + "the head instruction must survive, got {head:?}" + ); + + // The other direction: a sentence that merely CONTAINS an entry verb does + // not open with a back-reference, so nothing is dropped. The recognizer's + // narrowness — not full consumption — is what keeps this fail-closed. + for retained in [ + "this creature enters with two +1/+1 counters on it.", + "when this creature enters, draw a card.", + "if a creature card was exiled this way, you may cast it.", + ] { + assert!( + matches!( + strip_entry_this_way_riders(retained), + Some(std::borrow::Cow::Borrowed(_)) + ), + "a non-rider sentence must be handed back untouched: {retained}" + ); + } + } } diff --git a/crates/engine/src/parser/oracle_effect/sequence.rs b/crates/engine/src/parser/oracle_effect/sequence.rs index 7437fa9d2e..d0bec05845 100644 --- a/crates/engine/src/parser/oracle_effect/sequence.rs +++ b/crates/engine/src/parser/oracle_effect/sequence.rs @@ -6621,26 +6621,43 @@ pub(crate) fn is_moved_object_enters_modifier_clause(sentence: &str) -> bool { !matches!(filter, TargetFilter::Any | TargetFilter::None) } -/// CR 122.1 + CR 614.1c + CR 608.2c: returns true when `sentence` is a -/// moved-object put-onto-battlefield-this-way conditional whose counter payoff -/// is represented by `Effect::ChangeZone.conditional_enter_with_counters` -/// ("If you put an artifact onto the battlefield this way, put two +1/+1 counters -/// on it" — Oviya, Automech Artisan). Shared with the `Condition_If` swallow -/// detector so the represented clause can be located and stripped text-scoped, -/// reusing the same `parse_you_put_onto_battlefield_this_way_clause` combinator -/// that produced the gate — not a verbatim Oracle-string match. Mirrors -/// `is_moved_object_enters_modifier_clause`: it keys on the put-this-way condition -/// AND a "counter" payoff tail so it drops only the represented clause. -pub(crate) fn is_moved_object_put_onto_battlefield_counters_clause(sentence: &str) -> bool { +/// CR 122.1 + CR 614.1c + CR 608.2c + CR 400.7: returns true when `sentence` is a +/// moved-object battlefield-entry "this way" conditional whose counter payoff is +/// represented by `Effect::ChangeZone.conditional_enter_with_counters`. Shared +/// with the `Condition_If` swallow detector so the represented clause can be +/// located and stripped text-scoped, reusing the same combinator family that +/// produced the gate — not a verbatim Oracle-string match. Mirrors +/// `is_moved_object_enters_modifier_clause`: it keys on the entry-this-way +/// condition AND a "counter" payoff tail so it drops only the represented clause. +/// +/// CLAUSE VOICES accepted (all three are represented by the same typed slot): +/// * active — "If you put an artifact onto the battlefield this way, put two +/// +1/+1 counters on it" (Oviya, Automech Artisan); +/// * present-tense typed — "If a Hero enters this way, it enters with two +/// additional +1/+1 counters on it" (Heroic Return, Recommission, Winter +/// Soldier Reborn Avenger); +/// * passive typed — "If an Equipment is put onto the battlefield this way, …". +/// +/// CONDITIONAL VOICE is deliberately held at `if ` only, POLARITY at affirmative +/// only, and the SUBJECT at typed-filter-carrying only — all three are enforced by +/// `parse_conditional_entry_this_way_rider`, whose doc comment carries the +/// rationale. A trigger-voiced rider ("When an Equipment enters this way, …" — +/// Adaptive Armorer, Masterpiece Vault), a negated gate, and the bare-pronoun +/// voice ("If it enters this way, …") all stay visible to the audit. The pronoun +/// exclusion is the one that matters most here: no lowering produces +/// `AbilityCondition::ZoneChangedThisWay { filter }` for a filter-less subject, so +/// `fold_enters_this_way_counter_rider` never folds it into the slot — treating it +/// as represented would let a compound card whose OTHER rider populates the slot +/// strip this unrepresented one out of the swallow detector's residual. +/// +/// The "counter" payoff gate is load-bearing and unchanged: it keeps a non-counter +/// entry rider ("if a land enters this way, it enters tapped" — Silver Surfer, +/// Cosmic Voyager), which is genuinely unrepresented, flagged. +pub(crate) fn is_moved_object_entry_this_way_counters_clause(sentence: &str) -> bool { let lower = sentence.to_lowercase(); let lower = lower.trim(); - let Ok((after_if, _)) = tag::<_, _, OracleError<'_>>("if ").parse(lower) else { - return false; - }; let Ok((body, _)) = - crate::parser::oracle_nom::condition::parse_you_put_onto_battlefield_this_way_clause( - after_if, - ) + crate::parser::oracle_nom::condition::parse_conditional_entry_this_way_rider(lower) else { return false; }; diff --git a/crates/engine/src/parser/oracle_nom/condition.rs b/crates/engine/src/parser/oracle_nom/condition.rs index dcfe486df2..9d2e1d3dfc 100644 --- a/crates/engine/src/parser/oracle_nom/condition.rs +++ b/crates/engine/src/parser/oracle_nom/condition.rs @@ -7,7 +7,7 @@ use nom::branch::alt; use nom::bytes::complete::tag; use nom::bytes::complete::take_until; use nom::character::complete::multispace1; -use nom::combinator::{eof, map, opt, value}; +use nom::combinator::{eof, map, opt, peek, value, verify}; use nom::multi::many0; use nom::sequence::{preceded, terminated}; use nom::Parser; @@ -9142,6 +9142,32 @@ pub(crate) fn parse_unless_condition(input: &str) -> OracleResult<'_, StaticCond /// trailing punctuation like ", " or "."). On `wasn't`/`was not` the negation /// is exposed via `negated`. pub fn parse_zone_changed_this_way_clause(input: &str) -> OracleResult<'_, (TargetFilter, bool)> { + parse_zone_changed_this_way_clause_scoped(input, ThisWayVerbScope::AnyZoneChange) +} + +/// CR 400.7 + CR 608.2c: which zone-change verbs a "… this way" back-reference +/// may name. A typed scope rather than a `bool` so the axis stays self-documenting +/// and extensible (per the "never a raw bool" rule); it parameterizes *only* the +/// verb `alt()` of [`parse_zone_changed_this_way_clause_scoped`] — the article, +/// type-phrase, disjunction-fold and tense/negation axes are shared verbatim. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ThisWayVerbScope { + /// Every verb the rider grammar covers: the present-tense "enters"/"enter" + /// branch plus "put onto the battlefield", "destroyed", "exiled", + /// "sacrificed", "returned", "discarded", "milled", "countered". + AnyZoneChange, + /// Battlefield-entry verbs only: the present-tense "enters"/"enter" branch + /// plus "put onto the battlefield". CR 614.1c replacement classification is + /// entry-scoped, so only an entry rider can contaminate it. + BattlefieldEntry, +} + +/// Scoped implementation of [`parse_zone_changed_this_way_clause`]. See that +/// function's doc comment for the grammar; `scope` gates the verb axis only. +pub fn parse_zone_changed_this_way_clause_scoped( + input: &str, + scope: ThisWayVerbScope, +) -> OracleResult<'_, (TargetFilter, bool)> { // CR 608.2c: A "this way" conditional may be quantified. "at least one" / // "one or more" both mean "≥ 1", which the existential `.any()` semantics // of `ZoneChangedThisWay` already encode — they value-discard to unit. The @@ -9210,23 +9236,196 @@ pub fn parse_zone_changed_this_way_clause(input: &str) -> OracleResult<'_, (Targ // verb-phrase: single-word imperatives + the multi-word // "put onto the battlefield". The verb itself is value-discarded; the - // " this way" suffix is the discriminator. - let (rest, _) = alt(( - tag::<_, _, OracleError<'_>>("put onto the battlefield"), - tag("destroyed"), - tag("exiled"), - tag("sacrificed"), - tag("returned"), - tag("discarded"), - tag("milled"), - tag("countered"), - )) - .parse(rest)?; + // " this way" suffix is the discriminator. Under + // `ThisWayVerbScope::BattlefieldEntry` only the battlefield-entry verb is + // offered; the non-entry zone-change verbs are withheld. + let (rest, _) = match scope { + ThisWayVerbScope::AnyZoneChange => alt(( + tag::<_, _, OracleError<'_>>("put onto the battlefield"), + tag("destroyed"), + tag("exiled"), + tag("sacrificed"), + tag("returned"), + tag("discarded"), + tag("milled"), + tag("countered"), + )) + .parse(rest)?, + ThisWayVerbScope::BattlefieldEntry => { + tag::<_, _, OracleError<'_>>("put onto the battlefield").parse(rest)? + } + }; let (rest, _) = tag(" this way").parse(rest)?; Ok((rest, (filter, negated))) } +/// CR 608.2c: the bare pronoun subject of a reflexive battlefield-entry +/// back-reference — "it enters this way" (Pharika's Spawn, Silver Surfer). +/// Delegates the pronoun set to [`parse_object_recipient_pronoun`], the declared +/// single authority, rather than re-listing `it`/`them`/`him`/`her` here. +fn parse_pronoun_enters_this_way_clause(input: &str) -> OracleResult<'_, ()> { + let (rest, _) = parse_object_recipient_pronoun(input)?; + let (rest, _) = tag(" ").parse(rest)?; + let (rest, _) = alt((tag::<_, _, OracleError<'_>>("enters"), tag("enter"))).parse(rest)?; + let (rest, _) = tag(" this way").parse(rest)?; + Ok((rest, ())) +} + +/// CR 608.2c: the core reflexive **battlefield-entry** back-reference — a subject, +/// then an entry verb, then `" this way"`, with no conditional word. +/// +/// Returns `(SUBJECT FILTER, NEGATION FLAG)`. Neither component is value-discarded, +/// because the two wrapper voices below read them differently (see their doc +/// comments). +/// +/// Three subject voices, all delegated to existing combinators: +/// * passive/present typed subject — `parse_zone_changed_this_way_clause_scoped` +/// under [`ThisWayVerbScope::BattlefieldEntry`] ("a Hero enters this way", +/// "an Equipment is put onto the battlefield this way"); +/// * active `you put …` — `parse_you_put_onto_battlefield_this_way_clause`; +/// * bare pronoun — `parse_pronoun_enters_this_way_clause` ("it enters this way"). +/// +/// The subject filter is `None` for EXACTLY the bare-pronoun voice, which names its +/// referent anaphorically and therefore yields no typed filter. That distinction is +/// load-bearing downstream, not cosmetic: `oracle_effect::conditions` lowers only +/// the two filter-carrying voices to `AbilityCondition::ZoneChangedThisWay { filter }`, +/// and `oracle_effect::lower::fold_enters_this_way_counter_rider` folds only that +/// condition into `Effect::ChangeZone.conditional_enter_with_counters`. A +/// filter-less subject is thus never REPRESENTED by that slot, so the swallow-detector +/// voice must not treat it as represented — see +/// [`parse_conditional_entry_this_way_rider`]. +/// +/// POSITION (deliberate): the clause is recognized CLAUSE-INITIALLY only — every +/// subject alternative anchors at the start of `input`. A trailing-position entry +/// rider ("… it enters with a +1/+1 counter on it if a Hero enters this way.") is +/// therefore NOT recognized, and no printed card uses that voice: a Scryfall regex +/// sweep for a sentence-final battlefield-entry back-reference +/// (`o:/(enters|enter|is put onto the battlefield|are put onto the battlefield) this way\./`) +/// returns zero cards. What DOES print sentence-finally is the opposite shape — a +/// genuine CR 614.1c head carrying a trailing NON-entry back-reference ("This creature +/// enters with a +1/+1 counter on it for each card revealed this way" — Arsenal +/// Thresher, Gluttonous Hellkite, Thief of Blood, Mimeoplasm Revered One, Naya +/// Soulbeast, Sin Unending Cataclysm). Those heads must keep their tokens, and +/// [`ThisWayVerbScope::BattlefieldEntry`] already withholds their verbs. Word-boundary +/// scanning is the right tool for a phrase class that genuinely occurs at arbitrary +/// positions; this class does not, so scanning would buy no coverage while widening +/// the blast radius against that printed head class. Pinned by +/// `oracle_classifier::head_scoping_leaves_the_unprinted_trailing_rider_voice_alone`. +/// +/// A trailing word boundary is required so the clause cannot match a prefix of a +/// longer word. +pub fn parse_entry_this_way_clause(input: &str) -> OracleResult<'_, (Option, bool)> { + let (rest, subject) = alt(( + map( + |i| parse_zone_changed_this_way_clause_scoped(i, ThisWayVerbScope::BattlefieldEntry), + |(filter, negated)| (Some(filter), negated), + ), + map( + parse_you_put_onto_battlefield_this_way_clause, + |(filter, negated)| (Some(filter), negated), + ), + map(parse_pronoun_enters_this_way_clause, |()| (None, false)), + )) + .parse(input)?; + let (rest, _) = peek(alt((tag(","), tag(" "), tag("."), eof))).parse(rest)?; + Ok((rest, subject)) +} + +/// CR 608.2c: a "… this way" rider is a back-reference to an instruction EARLIER +/// IN THE SAME ABILITY, never an independent CR 614.1c replacement head — CR 614.1c +/// defines replacement effects as "[This permanent] enters with …" / "As [this +/// permanent] enters …" / "[This permanent] enters as …", none of which a reflexive +/// back-reference can be. +/// +/// SCOPE (deliberate, [`ThisWayVerbScope::BattlefieldEntry`]): only BATTLEFIELD-ENTRY +/// riders. CR 614.1c replacement classification is entry-scoped, so only an entry +/// rider can contaminate it. A "was milled/exiled/destroyed this way" rider +/// (Loafing Giant, Lazav Familiar Stranger, Demonic Junker, Nurturing Pixie) stays +/// visible to the classifier: its incidental "prevent all"/"become a copy of" +/// tokens come from the rider's CONSEQUENT, not from an entry statement. +/// +/// The conditional word is OPTIONAL here, covering "If …", "When …", "Whenever …" +/// and bare subject-initial riders alike. This subsumes BOTH former literal guards +/// that existed for Winter Soldier, Reborn Avenger — the +/// `has_trigger_prefix(lower) && scan_contains(lower, "enters this way,")` early +/// return in `oracle_classifier.rs` and the `!scan_contains(&lower, "enters this +/// way,")` conjunct on the Priority 5-pre enters-with interceptor in `oracle.rs`. +/// Both modelled a single grammatical voice (present tense, comma-terminated); +/// routing them through `oracle_classifier::strip_entry_this_way_riders` covers the +/// whole rider class this combinator recognizes, passive voice included. +/// +/// NEGATION: accepted at THIS (classifier) voice — a negated back-reference is still +/// a back-reference and still cannot be a CR 614.1c head. It is REJECTED at the +/// swallow-detector voice ([`parse_conditional_entry_this_way_rider`]), where +/// `conditional_enter_with_counters` can only represent an AFFIRMATIVE filter match +/// (`enter_with_counters_for_object` pushes counters when `matches_target_filter` is +/// true), so suppressing a negated clause's warning would be coverage dishonesty. +/// No card in `data/card-data.json` currently prints a passive-negated "this way" +/// clause; this is a grammar decision, pinned by unit tests. +/// +/// ALSO EXCLUDED: the cast-permission rider ("if you cast a spell this way, that +/// creature enters with a finality counter on it" — Intrepid Paleontologist, Noctis, +/// Leonardo, Osteomancer Adept, Edgar Master Machinist, Helmut Zemo, Advanced Floral +/// Invocations). That class is owned by +/// `StaticMode::{Graveyard,Exile}CastPermission.enters_with_counter`, and its tokens +/// legitimately participate in classification. +/// +/// CONSUMPTION CONTRACT (deliberate): this is a PREFIX recognizer, not a +/// full-consumption one. The returned remainder is the rider's CONSEQUENT (", it +/// enters with two additional +1/+1 counters on it."), which is exactly why the +/// consumer discards the whole sentence: `oracle_classifier::strip_entry_this_way_riders` +/// asks "does this sentence OPEN with a CR 608.2c back-reference?", and if it does, +/// every token after the comma belongs to that back-reference's consequent rather +/// than to a CR 614.1c head. Wrapping this in `all_consuming` at the consumer would +/// therefore reject every real rider — the class exists only because it HAS a +/// consequent. Fail-closed behavior is supplied instead by the narrowness of the +/// recognizer itself: an article-or-pronoun subject plus a battlefield-entry verb +/// plus `" this way"` is not a shape any CR 614.1c head can take, and the +/// `oracle_classifier` test module pins both directions (a genuine head keeps its +/// tokens; a rider sentence loses them). See +/// `oracle_classifier::a_rider_prefix_drops_its_whole_sentence_by_contract`. +pub fn parse_reflexive_entry_this_way_rider(input: &str) -> OracleResult<'_, ()> { + let (rest, _) = opt(alt((tag("if "), tag("when "), tag("whenever ")))).parse(input)?; + let (rest, _) = parse_entry_this_way_clause(rest)?; + Ok((rest, ())) +} + +/// CR 608.2c + CR 614.1c: the swallow-detector voice of +/// [`parse_entry_this_way_clause`] — the conditional word is MANDATORY and fixed at +/// "if ", negation is REJECTED, and the subject must carry a TYPED FILTER. +/// +/// All three restrictions are load-bearing and deliberately narrower than +/// [`parse_reflexive_entry_this_way_rider`]. Each one names a shape the typed slot +/// `Effect::ChangeZone.conditional_enter_with_counters` cannot represent, and this +/// recognizer decides whether a `Condition_If` swallow warning is SUPPRESSED — so +/// accepting an unrepresentable shape here is coverage dishonesty, not leniency: +/// * `tag("if ")` mandatory — the trigger voice ("When an Equipment enters this +/// way, …" — Adaptive Armorer, Masterpiece Vault) is not lowered into the slot, +/// so accepting it would newly silence a genuinely unrepresented clause. +/// * negation rejected — the slot represents an affirmative existential filter +/// match only (`enter_with_counters_for_object` pushes counters when +/// `matches_target_filter` is TRUE), so a negated gate is not representable. +/// * typed subject required (`filter.is_some()`) — the bare-pronoun voice ("if it +/// enters this way, …") produces no filter, so nothing lowers it to +/// `AbilityCondition::ZoneChangedThisWay { filter }` and +/// `fold_enters_this_way_counter_rider` — which matches on exactly that +/// condition — never folds it into the slot. Accepting it would let a compound +/// card whose OTHER rider populates the slot silently strip this unrepresented +/// one out of the residual. No card prints the conditional pronoun voice (a +/// Scryfall sweep for `o:/(it|they) (enters|enter) this way/` returns only +/// Pharika's Spawn, which is trigger-voiced and already excluded by the `if ` +/// gate), so the restriction closes the hole at zero coverage cost. +pub fn parse_conditional_entry_this_way_rider(input: &str) -> OracleResult<'_, ()> { + let (rest, _) = tag("if ").parse(input)?; + let (rest, _) = verify( + parse_entry_this_way_clause, + |(filter, negated): &(Option, bool)| filter.is_some() && !*negated, + ) + .parse(rest)?; + Ok((rest, ())) +} + /// CR 603.12 + CR 608.2c: Parse "you put [quantifier] [type] onto the battlefield /// this way" — the active-voice reflexive gate (Gilgamesh, Master-at-Arms: /// "When you put one or more Equipment onto the battlefield this way, you may @@ -18580,6 +18779,181 @@ mod tests { } } + // --------------------------------------------------------------------- + // CR 608.2c + CR 614.1c: ThisWayVerbScope + the two reflexive-entry rider voices + // --------------------------------------------------------------------- + + /// V0: the verb-scope narrowing is real. `AnyZoneChange` still accepts a + /// non-entry verb (the non-vacuous positive — a blanket narrowing regression + /// fails here), while `BattlefieldEntry` rejects the same input. + #[test] + fn this_way_verb_scope_separates_entry_from_non_entry_verbs() { + let non_entry = "a creature card was exiled this way, you may cast it"; + assert!( + parse_zone_changed_this_way_clause_scoped(non_entry, ThisWayVerbScope::AnyZoneChange) + .is_ok(), + "AnyZoneChange must keep the full verb set" + ); + assert!( + parse_zone_changed_this_way_clause_scoped( + non_entry, + ThisWayVerbScope::BattlefieldEntry + ) + .is_err(), + "BattlefieldEntry must withhold non-entry zone-change verbs" + ); + + // The battlefield-entry verb is accepted under BOTH scopes. + for scope in [ + ThisWayVerbScope::AnyZoneChange, + ThisWayVerbScope::BattlefieldEntry, + ] { + assert!( + parse_zone_changed_this_way_clause_scoped( + "an equipment is put onto the battlefield this way, attach it", + scope, + ) + .is_ok(), + "entry verb must parse under {scope:?}" + ); + } + } + + /// V0b (classifier voice): the conditional word is optional and passive + /// negation is in scope. + #[test] + fn reflexive_entry_this_way_rider_accepts_the_classifier_voice() { + for accepted in [ + "if a hero enters this way, it enters with two additional +1/+1 counters on it.", + "if a creature enters this way, it enters with an additional +1/+1 counter on it.", + "if a land enters this way, it enters tapped.", + "when an equipment is put onto the battlefield this way, you may attach it", + "when you put one or more equipment onto the battlefield this way, attach one", + "whenever a creature enters this way, draw a card", + "when it enters this way, each opponent sacrifices a creature.", + "it enters this way, so draw a card", + // Passive negation is DELIBERATELY in scope at this voice: a negated + // back-reference is still a back-reference, never a CR 614.1c head. + "if a creature wasn't put onto the battlefield this way, draw a card", + "if a creature is not put onto the battlefield this way, draw a card", + ] { + assert!( + parse_reflexive_entry_this_way_rider(accepted).is_ok(), + "classifier voice must accept: {accepted}" + ); + } + } + + /// V0b (classifier voice): everything structurally outside the CR 608.2c + /// battlefield-entry back-reference stays visible to the classifier. + #[test] + fn reflexive_entry_this_way_rider_rejects_out_of_class_text() { + for rejected in [ + // Cast-permission rider class — owned by StaticMode CastPermission. + "if you cast a spell this way, that creature enters with a finality counter on it", + // Non-entry zone-change verbs (ThisWayVerbScope::BattlefieldEntry). + "if a creature card was exiled this way, you may cast it", + "if a land card was milled this way, draw a card", + "if a permanent was destroyed this way, you gain 1 life", + // Active-voice negation is out of grammar reach by design. + "if you didn't put a card onto the battlefield this way, draw a card", + // `parse_article` rejects the quantifier "fewer". + "if you put fewer than two lands onto the battlefield this way, draw a card", + // Trailing adjuncts: "this way" is not clause-initial here. + "for each creature card exiled this way, create a token", + "this creature enters with a +1/+1 counter on it for each card revealed this way", + "each land played this way enters tapped", + // A real CR 614.1c replacement head must never be mistaken for a rider. + "this creature enters with two +1/+1 counters on it.", + ] { + assert!( + parse_reflexive_entry_this_way_rider(rejected).is_err(), + "classifier voice must reject: {rejected}" + ); + } + } + + /// V0b (swallow-detector voice): differs from the classifier voice on + /// exactly three axes — mandatory "if ", affirmative-only polarity, and a + /// subject that carries a typed filter. + #[test] + fn conditional_entry_this_way_rider_fixes_voice_and_polarity() { + // Shared accept: both voices take the affirmative "if" form. + let shared = + "if a hero enters this way, it enters with two additional +1/+1 counters on it."; + assert!(parse_conditional_entry_this_way_rider(shared).is_ok()); + assert!(parse_reflexive_entry_this_way_rider(shared).is_ok()); + + // Conditional word is MANDATORY here (fail-on-revert pin for `tag("if ")`). + let trigger_voiced = "when an equipment enters this way, attach it to a creature"; + assert!(parse_conditional_entry_this_way_rider(trigger_voiced).is_err()); + assert!(parse_reflexive_entry_this_way_rider(trigger_voiced).is_ok()); + + // Negation is REJECTED here (fail-on-revert pin for the polarity decision): + // `conditional_enter_with_counters` can only represent an affirmative match. + let negated = "if a creature wasn't put onto the battlefield this way, draw a card"; + assert!(parse_conditional_entry_this_way_rider(negated).is_err()); + assert!(parse_reflexive_entry_this_way_rider(negated).is_ok()); + + // Active-voice negation is rejected at BOTH voices (Break Out). + let active_negated = "if you didn't put a card onto the battlefield this way, draw a card"; + assert!(parse_conditional_entry_this_way_rider(active_negated).is_err()); + assert!(parse_reflexive_entry_this_way_rider(active_negated).is_err()); + + // A TYPED SUBJECT is required here (fail-on-revert pin for the + // `filter.is_some()` conjunct): the bare-pronoun voice names its referent + // anaphorically, so `parse_entry_this_way_clause` yields no filter, nothing + // lowers it to `ZoneChangedThisWay { filter }`, and + // `fold_enters_this_way_counter_rider` never folds it into + // `conditional_enter_with_counters`. Suppressing its warning would claim a + // representation that does not exist. + let bare_pronoun = "if it enters this way, it enters with a +1/+1 counter on it"; + assert!(parse_conditional_entry_this_way_rider(bare_pronoun).is_err()); + assert!(parse_reflexive_entry_this_way_rider(bare_pronoun).is_ok()); + + // The subject axis is about the FILTER, not the pronoun spelling: the + // active and passive voices both keep their typed subject and stay accepted. + for typed in [ + "if an equipment is put onto the battlefield this way, put a counter on it", + "if you put a land onto the battlefield this way, put a counter on it", + ] { + assert!( + parse_conditional_entry_this_way_rider(typed).is_ok(), + "typed subject must stay represented: {typed}" + ); + } + } + + /// V0b (subject filter): the value `parse_entry_this_way_clause` now returns + /// is the SUBJECT of the back-reference, and `None` marks exactly the voice + /// that carries no typed filter. Pins the discriminator the swallow detector + /// keys on, at the combinator rather than only through its consumers. + #[test] + fn entry_this_way_clause_reports_its_subject_filter() { + let (_, (filter, negated)) = + parse_entry_this_way_clause("a hero enters this way,").unwrap(); + assert!(!negated); + assert!( + matches!(filter, Some(TargetFilter::Typed(ref t)) + if t.type_filters.contains(&TypeFilter::Subtype("Hero".to_string()))), + "the typed subject must surface its filter, got {filter:?}" + ); + + let (_, (filter, _)) = + parse_entry_this_way_clause("you put a land onto the battlefield this way,").unwrap(); + assert!( + filter.is_some(), + "the active voice carries a typed subject too, got {filter:?}" + ); + + let (_, (filter, negated)) = parse_entry_this_way_clause("it enters this way,").unwrap(); + assert!(!negated); + assert!( + filter.is_none(), + "the bare-pronoun voice must report NO subject filter, got {filter:?}" + ); + } + // --------------------------------------------------------------------- // CR 122.1 + CR 608.2c: parse_there_are_counters_on_source // --------------------------------------------------------------------- diff --git a/crates/engine/src/parser/oracle_nom/primitives.rs b/crates/engine/src/parser/oracle_nom/primitives.rs index 446b60cae0..03640bbfa5 100644 --- a/crates/engine/src/parser/oracle_nom/primitives.rs +++ b/crates/engine/src/parser/oracle_nom/primitives.rs @@ -4,7 +4,7 @@ use std::borrow::Cow; use nom::branch::alt; use nom::bytes::complete::{tag, take_till1, take_until, take_while_m_n}; -use nom::character::complete::{char, digit1, satisfy, space0}; +use nom::character::complete::{char, digit1, multispace0, satisfy, space0}; use nom::combinator::{all_consuming, eof, map, map_res, not, opt, peek, recognize, value}; use nom::multi::{many0, many1}; use nom::sequence::{delimited, preceded, terminated}; @@ -1124,6 +1124,85 @@ where last } +/// Recognize one period-terminated sentence. +/// +/// The recognized slice INCLUDES the trailing '.' and EXCLUDES any leading +/// whitespace (`multispace0` is consumed by `preceded`, outside `recognize`). +/// +/// This is the single authority for period-sentence segmentation, and every +/// consumer that decides "is THIS sentence the represented CR 608.2c rider?" +/// delegates here, directly or through [`parse_period_sentences`] / +/// [`split_sentence_units`], so they cannot develop divergent sentence models. +/// Today's delegating consumers: +/// * the replacement-line dispatcher (`oracle::parse_replacement_sentences`, +/// which feeds `is_replacement_pattern` at its only sentence-scoped call +/// site, `parse_replacement_sentence_sequence_ir`); +/// * the classifier's rider head-scoper +/// (`oracle_classifier::strip_entry_this_way_riders`), itself shared by the +/// replacement, static and Priority 5-pre classification gates; +/// * the entry-rider and cast-rider swallow residual builders +/// (`swallow_check::{conditional_enter_counters_if_is_only_if_marker, +/// enters_with_finality_this_way_is_only_if_marker}`). +/// +/// A `split('.')`-based model would diverge in three ways that all matter to the +/// classifier: it keeps the leading space, drops the terminal '.', and emits an +/// empty tail element. +/// +/// Scope of the claim, stated exactly so it stays checkable: older +/// `swallow_check` strip/residual builders that predate this authority still +/// carry their own `split('.')` model — today +/// `enters_modified_if_is_only_if_marker`, +/// `cast_this_way_alt_cost_is_only_if_marker`, +/// `strip_represented_replacement_instead_sentences`, +/// `strip_cr_implicit_if_phrases` and `strip_represented_tiered_pairs_from_line` +/// (`rg "split\('\.'\)" crates/engine/src/parser/swallow_check.rs` enumerates +/// them). They are the known remaining drift surface, NOT a claim of delegation; +/// converting one is a behavior-affecting change (a newline directly after a +/// period becomes a space, so a post-newline " if " starts being seen) and must +/// be done deliberately, not incidentally. +pub fn parse_period_sentence(input: &str) -> OracleResult<'_, &str> { + preceded( + multispace0, + recognize(terminated(take_until("."), tag("."))), + ) + .parse(input) +} + +/// Recognize a run of one or more period-terminated sentences. +/// +/// Deliberately NOT `all_consuming` — a printed Oracle line need not end in a +/// period. Callers that require full consumption wrap this themselves (see +/// `oracle::parse_replacement_sentences`); callers that must also handle an +/// unterminated tail read it from the returned remainder. +pub fn parse_period_sentences(input: &str) -> OracleResult<'_, Vec<&str>> { + many1(parse_period_sentence).parse(input) +} + +/// Segment a text unit into sentence units, INCLUDING a final unterminated +/// fragment. +/// +/// [`parse_period_sentences`] is deliberately partial (a printed Oracle line need +/// not end in a period); this is the total wrapper every classifier and swallow +/// detector wants, so none of them has to re-derive tail handling — the exact +/// place a second, `split('.')`-shaped sentence model keeps growing back. +/// +/// Units carry their terminal '.' and never carry leading whitespace (that is +/// [`parse_period_sentence`]'s contract); the unterminated tail is trimmed on +/// both ends. Text with no period at all yields exactly one unit, and +/// whitespace-only text yields none. +pub fn split_sentence_units(input: &str) -> Vec<&str> { + let (tail, mut units) = match parse_period_sentences(input) { + Ok((tail, units)) => (tail, units), + // No period at all: the whole input is one unterminated unit. + Err(_) => (input, Vec::new()), + }; + let tail = tail.trim(); + if !tail.is_empty() { + units.push(tail); + } + units +} + /// Check whether `phrase` appears at any word boundary in `text`. /// /// More precise than `str::contains()` — matches complete phrases at word @@ -1384,6 +1463,38 @@ mod tests { use super::*; use nom::bytes::complete::tag; + /// The total wrapper keeps `parse_period_sentence`'s contract (terminal '.' + /// kept, leading whitespace excluded) and adds exactly one thing: the + /// unterminated tail every classifier and swallow detector needs. Those three + /// properties are what make a `split('.')` model non-substitutable. + #[test] + fn split_sentence_units_keeps_periods_and_recovers_the_tail() { + assert_eq!( + split_sentence_units("return it to the battlefield. if a hero enters this way, it enters with a counter on it."), + vec![ + "return it to the battlefield.", + "if a hero enters this way, it enters with a counter on it." + ] + ); + // Unterminated tail is recovered and trimmed, not dropped. + assert_eq!( + split_sentence_units("first. second"), + vec!["first.", "second"] + ); + // No period at all: exactly one unit. + assert_eq!( + split_sentence_units("no period here"), + vec!["no period here"] + ); + // Whitespace-only input has no units at all. + assert!(split_sentence_units(" ").is_empty()); + // A newline separator is leading whitespace of the next unit, not part of it. + assert_eq!( + split_sentence_units("flying.\nreach."), + vec!["flying.", "reach."] + ); + } + #[test] fn strip_double_quoted_spans_no_quote_borrows_unchanged() { let out = strip_double_quoted_spans("creatures you control can't block"); diff --git a/crates/engine/src/parser/oracle_tests.rs b/crates/engine/src/parser/oracle_tests.rs index e83c4c068c..e63b00a2f5 100644 --- a/crates/engine/src/parser/oracle_tests.rs +++ b/crates/engine/src/parser/oracle_tests.rs @@ -23433,14 +23433,43 @@ fn enters_with_n_additional_counters_parses_canonical_type() { assert_eq!(ct, CounterType::Plus1Plus1, "Necromantic Summons type"); assert_eq!(count, QuantityExpr::Fixed { value: 2 }, "count"); - // "it enters with two additional +1/+1 counters on it" (Heroic Return) - let (ct, _) = enters_counter( + // "it enters with two additional +1/+1 counters on it" (Heroic Return). + // + // CR 608.2c: this one is a REFLEXIVE "enters this way" rider, so its canonical + // counter type is carried by `ChangeZone.conditional_enter_with_counters` on the + // reanimation effect — NOT by a standalone `PutCounter` replacement. (It used to + // land in `replacements` with `target: SelfRef`, i.e. counters on the instant + // itself, which is unresolvable; the classifier no longer claims the line.) + // The canonical-type claim under test is unchanged, only its storage location. + let parsed = parse_oracle_text( "Return target creature card from your graveyard to the battlefield. \ If a Hero enters this way, it enters with two additional +1/+1 counters on it.", "Heroic Return", - &["Sorcery"], + &[], + &["Instant".to_string()], + &[], ); - assert_eq!(ct, CounterType::Plus1Plus1, "Heroic Return type"); + assert!( + parsed.replacements.is_empty(), + "Heroic Return: reflexive rider must not become a replacement: {parsed:?}" + ); + let heroic_effect = &parsed + .abilities + .first() + .unwrap_or_else(|| panic!("Heroic Return: reanimation ability missing: {parsed:?}")) + .effect; + let Effect::ChangeZone { + conditional_enter_with_counters, + .. + } = heroic_effect.as_ref() + else { + panic!("Heroic Return: head must be ChangeZone, got {heroic_effect:#?}"); + }; + let [(_, ct, count)] = conditional_enter_with_counters.as_slice() else { + panic!("Heroic Return: expected exactly one conditional entry counter rider: {parsed:?}"); + }; + assert_eq!(*ct, CounterType::Plus1Plus1, "Heroic Return type"); + assert_eq!(*count, QuantityExpr::Fixed { value: 2 }, "count"); // "it enters with three additional +1/+1 counters on it" (Turntimber Symbiosis) let (ct, count) = enters_counter( @@ -23463,6 +23492,168 @@ fn enters_with_n_additional_counters_parses_canonical_type() { assert_eq!(count, QuantityExpr::Fixed { value: 3 }, "count"); } +/// CR 608.2c + CR 614.1c: the Priority 5-pre enters-with interceptor is +/// head-scoped, so EVERY grammatical voice of the reflexive entry rider keeps its +/// line on the trigger path — not just the present-tense, comma-terminated voice +/// the retired `!scan_contains(&lower, "enters this way,")` literal modelled. +/// +/// Winter Soldier, Reborn Avenger is the printed member of the class and uses the +/// present-tense voice; the passive-voice ("… is put onto the battlefield this +/// way, …") and comma-less members are the ones the literal let through. All three +/// must produce a TRIGGER whose head instruction survives, with the rider folded +/// into `conditional_enter_with_counters` — a whole-line "enters with" scan hands +/// the line to `parse_replacement_line_ir` instead, which publishes a replacement +/// and drops the head reanimation. That is the assertion that flips on revert. +#[test] +fn reflexive_entry_rider_voices_all_stay_on_the_trigger_path() { + // Verbatim printed Oracle text (`data/mtgjson/AtomicCards.json`). + const WINTER_SOLDIER: &str = "Whenever Winter Soldier attacks, return target creature card \ + with mana value less than or equal to Winter Soldier's power from your graveyard to \ + the battlefield. If a Hero enters this way, it enters with an additional +1/+1 \ + counter on it."; + // Same class, passive voice — the voice the retired literal missed. + const PASSIVE_VOICE: &str = "Whenever this creature attacks, return target creature card \ + from your graveyard to the battlefield. If a creature is put onto the battlefield \ + this way, it enters with an additional +1/+1 counter on it."; + + for (name, oracle) in [ + ("Winter Soldier, Reborn Avenger", WINTER_SOLDIER), + ("Passive Voice Reanimator", PASSIVE_VOICE), + ] { + // Premise (this is what makes the negative below non-vacuous and pins the + // revert): handed the WHOLE line, the replacement parser really does claim + // it. Only the head-scoped `enters with` gate keeps the interceptor from + // reaching this and dropping the reanimation instruction. + assert!( + crate::parser::oracle_replacement::parse_replacement_line(oracle, name).is_some(), + "{name}: premise — the un-scoped line is claimable by the replacement parser" + ); + + let parsed = parse(oracle, name, &[], &["Creature"], &[]); + + // Reach-guard: the line really produced a trigger, so the + // `replacements.is_empty()` negative cannot pass on a failed parse. + assert_eq!( + parsed.triggers.len(), + 1, + "{name}: the attack trigger must survive: {parsed:?}" + ); + assert!( + parsed.replacements.is_empty(), + "{name}: a CR 608.2c rider must not route the line to the replacement \ + interceptor: {parsed:?}" + ); + + let execute = parsed.triggers[0] + .execute + .as_ref() + .unwrap_or_else(|| panic!("{name}: trigger must carry an effect: {parsed:?}")); + let Effect::ChangeZone { + origin, + destination, + conditional_enter_with_counters, + .. + } = execute.effect.as_ref() + else { + panic!("{name}: head must be the reanimation ChangeZone: {execute:#?}"); + }; + assert_eq!(*origin, Some(Zone::Graveyard), "{name}"); + assert_eq!(*destination, Zone::Battlefield, "{name}"); + assert!( + !conditional_enter_with_counters.is_empty(), + "{name}: the rider must be folded into the typed slot: {execute:#?}" + ); + } +} + +/// CR 608.2c: head-scoping the spell-line STATIC gate is BEHAVIOR-PRESERVING on +/// every input reachable today, and this pins that fact. +/// +/// `is_static_compound_pattern` fires on `"enters with " && !"counter"` — tokens a +/// rider consequent supplies — and it short-circuits +/// `is_spell_resolution_instruction_line` one branch BEFORE the replacement gate. +/// It is now head-scoped for uniformity (see +/// `oracle_classifier::tests::static_classification_is_rider_contaminable_without_head_scoping`, +/// which pins the verdict flip at the seam itself). +/// +/// The *observable* verdict cannot change, though, and that is worth pinning +/// rather than asserting a difference that does not exist: the same function ends +/// in an honest-failure gate (`!has_unimplemented(parse_effect_chain(...))`), and a +/// non-counter entry rider is by construction unrepresentable — the typed slot +/// `conditional_enter_with_counters` only carries counters — so it fails that gate +/// regardless. A *representable* rider always carries the word "counter", which +/// negates the static arm. Both branches below therefore keep the head +/// reanimation instruction and differ only in whether the rider folds in. +#[test] +fn entry_rider_head_scoping_of_the_static_gate_is_behavior_preserving() { + /// True when some ability (or its sub-ability chain) is the reanimation head. + fn has_reanimation(parsed: &ParsedAbilities) -> bool { + fn is_head(effect: &Effect) -> bool { + matches!( + effect, + Effect::ChangeZone { + origin: Some(Zone::Graveyard), + destination: Zone::Battlefield, + .. + } + ) + } + parsed.abilities.iter().any(|a| { + is_head(a.effect.as_ref()) || a.sub_ability.iter().any(|s| is_head(s.effect.as_ref())) + }) + } + + // Representable rider (carries "counter", so the static arm never fired): + // folds into the typed slot and stays in the spell body. + let counter_rider = parse( + "Draw a card.\nReturn target creature card from your graveyard to the battlefield. \ + If a Hero enters this way, it enters with two additional +1/+1 counters on it.", + "Synthetic Counter Rider", + &[], + &["Sorcery"], + &[], + ); + assert!(has_reanimation(&counter_rider), "{counter_rider:?}"); + assert!( + counter_rider.abilities.iter().any(|a| { + let carries_slot = |effect: &Effect| { + matches!( + effect, + Effect::ChangeZone { conditional_enter_with_counters, .. } + if !conditional_enter_with_counters.is_empty() + ) + }; + carries_slot(a.effect.as_ref()) + || a.sub_ability + .iter() + .any(|s| carries_slot(s.effect.as_ref())) + }), + "the representable rider must fold into the typed slot: {counter_rider:?}" + ); + + // Non-counter rider: the head instruction still survives; only the + // unrepresentable rider consequent is left as an honest `Unimplemented` + // residual, which is the coverage-honest outcome. + let non_counter_rider = parse( + "Draw a card.\nReturn target creature card from your graveyard to the battlefield. \ + If a Hero enters this way, it enters with your choice of flying or vigilance.", + "Synthetic Non Counter Rider", + &[], + &["Sorcery"], + &[], + ); + assert!( + has_reanimation(&non_counter_rider), + "the head reanimation must survive even when the rider is unrepresentable: \ + {non_counter_rider:?}" + ); + assert!( + non_counter_rider.statics.is_empty() && non_counter_rider.replacements.is_empty(), + "the rider must not turn the line into a static or a replacement: \ + {non_counter_rider:?}" + ); +} + /// Regression for issue #1272: Violent Urge's Delirium follow-up ("that /// creature gains double strike") must scope to the same single target as /// the base "+1/+0 and first strike" clause, not to every creature. diff --git a/crates/engine/src/parser/swallow_check.rs b/crates/engine/src/parser/swallow_check.rs index 805a2f831d..6eceecf124 100644 --- a/crates/engine/src/parser/swallow_check.rs +++ b/crates/engine/src/parser/swallow_check.rs @@ -3109,23 +3109,35 @@ fn strip_represented_replacement_instead_sentences( out } -/// CR 122.1 + CR 614.1c + CR 608.2c + CR 400.7: "If you put a[n] onto the -/// battlefield this way, put [N] +1/+1 counters on it" (Oviya, Automech Artisan) -/// is represented by the typed `Effect::ChangeZone.conditional_enter_with_counters` -/// gate — the moved object's entry-time counters are applied only when it matches -/// the carried filter (runtime-verified in -/// `change_zone::enter_with_counters_for_object`), so the leading "if" is a -/// representation marker, not a swallowed condition. +/// CR 122.1 + CR 614.1c + CR 608.2c + CR 400.7: a reflexive battlefield-entry +/// "this way" conditional with a counter payoff — "If you put a[n] onto the +/// battlefield this way, put [N] +1/+1 counters on it" (Oviya, Automech Artisan) or +/// the present-tense "If a Hero enters this way, it enters with two additional +/// +1/+1 counters on it" (Heroic Return, Recommission, Winter Soldier Reborn +/// Avenger) — is represented by the typed +/// `Effect::ChangeZone.conditional_enter_with_counters` gate. The moved object's +/// entry-time counters are applied only when it matches the carried filter +/// (runtime-verified in `change_zone::enter_with_counters_for_object`), so the +/// leading "if" is a representation marker, not a swallowed condition. /// /// Mirrors `enters_modified_if_is_only_if_marker`: an inside AST probe /// (`conditional_enter_with_counters` carries `skip_serializing_if = Vec::is_empty`, /// so the key serializes ONLY when non-empty — keying tightly on the /// ChangeScope→Battlefield-with-counters shape the resolver handles) plus -/// text-scoping — the represented put-onto-battlefield-this-way counter clause is -/// located via the shared `is_moved_object_put_onto_battlefield_counters_clause` -/// combinator and dropped sentence-by-sentence, and suppression fires ONLY when no -/// OTHER bare " if " survives, so a compound card carrying the gate AND a separate -/// unrelated " if " still flags. +/// text-scoping — the represented entry-this-way counter clause is located via the +/// shared `is_moved_object_entry_this_way_counters_clause` combinator and dropped +/// sentence-by-sentence, and suppression fires ONLY when no OTHER bare " if " +/// survives, so a compound card carrying the gate AND a separate unrelated " if " +/// still flags. +/// +/// Two axes are deliberately NOT widened, because the typed slot cannot represent +/// what they would newly silence: +/// * conditional voice is fixed at `if ` — a trigger-voiced rider ("When an +/// Equipment enters this way, …") keeps flagging; +/// * polarity is affirmative-only — `enter_with_counters_for_object` pushes +/// counters when `matches_target_filter` is TRUE, so a negated gate ("if a +/// creature wasn't put onto the battlefield this way, …") is unrepresentable +/// and keeps flagging. fn conditional_enter_counters_if_is_only_if_marker( stripped: &str, evidence: &UnitEvidence, @@ -3133,15 +3145,20 @@ fn conditional_enter_counters_if_is_only_if_marker( if !evidence.has_slot("conditional_enter_with_counters") { return false; } - let residual: String = stripped - .split('.') + // Segmentation delegates to `nom_primitives::split_sentence_units`, the single + // period-sentence authority shared with the classifier's rider head-scoper, so + // the two cannot decide "is THIS sentence the represented rider?" with + // divergent sentence models. Units keep their terminal '.' and carry no leading + // whitespace, so the residual is rejoined with a single space. + let residual: String = crate::parser::oracle_nom::primitives::split_sentence_units(stripped) + .into_iter() .filter(|sentence| { - !crate::parser::oracle_effect::sequence::is_moved_object_put_onto_battlefield_counters_clause( + !crate::parser::oracle_effect::sequence::is_moved_object_entry_this_way_counters_clause( sentence, ) }) .collect::>() - .join("."); + .join(" "); let has_other_if = residual.contains(" if ") // allow-noncombinator: swallow detector marker scan on classified text && !residual.contains(" as if ") // allow-noncombinator: swallow detector marker scan on classified text && !residual.contains(" even if "); // allow-noncombinator: swallow detector marker scan on classified text @@ -3171,16 +3188,17 @@ fn enters_with_finality_this_way_is_only_if_marker( return false; } - let residual: String = stripped - .split('.') + // Same single segmentation authority as the sibling detector above; + // `parse_cast_this_way_enters_with_counter` trims its own leading whitespace + // and does not require full consumption, so a unit's terminal '.' is inert. + let residual: String = crate::parser::oracle_nom::primitives::split_sentence_units(stripped) + .into_iter() .filter(|sentence| { - crate::parser::oracle_effect::parse_cast_this_way_enters_with_counter( - sentence.trim_start(), - ) - .is_none() + crate::parser::oracle_effect::parse_cast_this_way_enters_with_counter(sentence) + .is_none() }) .collect::>() - .join("."); + .join(" "); let has_other_if = residual.contains(" if ") // allow-noncombinator: swallow detector marker scan on classified text && !residual.contains(" as if ") // allow-noncombinator: swallow detector marker scan on classified text && !residual.contains(" even if "); // allow-noncombinator: swallow detector marker scan on classified text @@ -3337,9 +3355,14 @@ fn detect_condition_if( if enters_modified_if_is_only_if_marker(&stripped, evidence) { return; } - // CR 122.1 + CR 614.1c + CR 608.2c: "If you put a[n] onto the - // battlefield this way, put [N] +1/+1 counters on it" (Oviya) is represented - // by `Effect::ChangeZone.conditional_enter_with_counters`. + // CR 122.1 + CR 614.1c + CR 608.2c: an affirmative `if `-voiced reflexive + // battlefield-entry "this way" clause with a counter payoff — active + // ("If you put a[n] onto the battlefield this way, put [N] +1/+1 + // counters on it" — Oviya) or present-tense ("If a Hero enters this way, it + // enters with two additional +1/+1 counters on it" — Heroic Return, + // Recommission, Winter Soldier) — is represented by + // `Effect::ChangeZone.conditional_enter_with_counters`. Trigger-voiced and + // negated riders are NOT suppressed; see the function doc. if conditional_enter_counters_if_is_only_if_marker(&stripped, evidence) { return; } @@ -6332,6 +6355,159 @@ mod tests { ); } + /// V5 (CR 122.1 + CR 614.1c + CR 608.2c): the present-tense reflexive + /// battlefield-entry counter rider is represented by + /// `Effect::ChangeZone.conditional_enter_with_counters`, so its leading "if" + /// is a representation marker, not a swallowed condition. + /// + /// Winter Soldier is the independently-reachable member: its head instruction + /// survives the classifier gates on its own (the trigger-voiced head carries no + /// "enters with" of its own once the rider sentence is scoped off), so this + /// assertion fails on revert of Unit 2 alone, without Unit 1. + /// + /// Fixture text is the VERBATIM printed Oracle text from + /// `data/mtgjson/AtomicCards.json` (`"Winter Soldier, Reborn Avenger"`), so the + /// pinned `ChangeZone` shape is the real card's dynamic + /// `Cmc LE Ref(Power{Source})` subject, not a synthetic fixed-mana-value one. + #[test] + fn condition_if_accepts_present_tense_enters_this_way_counter_rider() { + let parsed = parse_named( + "Whenever Winter Soldier attacks, return target creature card with mana value \ + less than or equal to Winter Soldier's power from your graveyard to the \ + battlefield. If a Hero enters this way, it enters with an additional +1/+1 \ + counter on it.", + "Winter Soldier, Reborn Avenger", + &["Creature"], + ); + // Reach-guard: `check_swallowed_clauses` early-returns on Unimplemented, + // so a bare negative would be vacuous. Prove the rider really is + // represented by the typed slot before asserting the absence. + let carries_slot = parsed.triggers.iter().any(|t| { + t.execute.as_ref().is_some_and(|e| { + matches!( + e.effect.as_ref(), + Effect::ChangeZone { + conditional_enter_with_counters, + .. + } if !conditional_enter_with_counters.is_empty() + ) + }) + }); + assert!( + carries_slot, + "premise: the rider must be represented by conditional_enter_with_counters: {parsed:?}" + ); + assert!( + !has_swallowed_detector(&parsed, "Condition_If"), + "a represented present-tense entry rider must not report a swallowed \ + condition: {:?}", + parsed.parse_warnings + ); + } + + /// V5: the two axes Unit 2 deliberately did NOT widen. Both fixtures carry the + /// same represented slot, so a blanket-widening regression flips them silently. + #[test] + fn condition_if_still_flags_unrepresented_entry_this_way_voices() { + // Conditional voice: `tag("if ")` is mandatory. A trigger-voiced rider is + // a different, unrepresented shape and must keep flagging. + assert!( + !crate::parser::oracle_effect::sequence::is_moved_object_entry_this_way_counters_clause( + "When an Equipment enters this way, put a +1/+1 counter on it" + ), + "trigger-voiced rider must not be treated as represented" + ); + // Polarity: `conditional_enter_with_counters` represents an AFFIRMATIVE + // filter match only, so a negated gate is unrepresentable. + assert!( + !crate::parser::oracle_effect::sequence::is_moved_object_entry_this_way_counters_clause( + "If a creature wasn't put onto the battlefield this way, put a +1/+1 counter on it" + ), + "negated gate must not be treated as represented" + ); + // Non-vacuous positive on the same seam: the affirmative `if` voice IS + // represented, so a blanket-`false` regression fails here. + assert!( + crate::parser::oracle_effect::sequence::is_moved_object_entry_this_way_counters_clause( + "If a Hero enters this way, it enters with two additional +1/+1 counters on it" + ) + ); + // The retained "counter" payoff gate: Silver Surfer's `enters tapped` + // rider is genuinely unrepresented and must stay visible to the audit. + assert!( + !crate::parser::oracle_effect::sequence::is_moved_object_entry_this_way_counters_clause( + "If a land enters this way, it enters tapped" + ) + ); + // Subject: the bare-pronoun voice carries no typed filter, so nothing + // lowers it to `ZoneChangedThisWay { filter }` and + // `fold_enters_this_way_counter_rider` never folds it into + // `conditional_enter_with_counters`. Treating it as represented would let + // a compound card whose OTHER rider populates the slot strip this + // unrepresented one out of the residual below. + assert!( + !crate::parser::oracle_effect::sequence::is_moved_object_entry_this_way_counters_clause( + "If it enters this way, it enters with a +1/+1 counter on it" + ), + "the filter-less pronoun subject must not be treated as represented" + ); + } + + /// V5: the pronoun exclusion is not merely a combinator property — it must + /// survive to the detector. A card carrying the represented typed rider AND an + /// unrepresented bare-pronoun rider must still flag, because only the typed one + /// reaches `conditional_enter_with_counters`. Before the subject restriction, + /// the pronoun sentence was stripped from the residual alongside the typed one + /// and its warning vanished with it. + #[test] + fn represented_typed_rider_does_not_hide_an_unrepresented_pronoun_rider() { + let parsed = parse_named( + "Return target creature card from your graveyard to the battlefield. \ + If a Hero enters this way, it enters with two additional +1/+1 counters on it. \ + If it enters this way, draw a card.", + "Pronoun Rider Compound Fixture", + &["Instant"], + ); + // Reach-guard: the typed rider really is represented, so the assertion + // below is about the pronoun sentence and not about a total parse failure. + let carries_slot = parsed.abilities.iter().any(|a| { + matches!( + a.effect.as_ref(), + Effect::ChangeZone { + conditional_enter_with_counters, + .. + } if !conditional_enter_with_counters.is_empty() + ) + }); + assert!( + carries_slot, + "premise: the typed rider must be represented by the slot: {parsed:?}" + ); + assert!( + has_swallowed_detector(&parsed, "Condition_If"), + "the unrepresented pronoun rider must stay visible to Condition_If, got {:?}", + parsed.parse_warnings + ); + } + + /// V5: a card carrying the represented gate PLUS an unrelated bare " if " + /// must still flag — exercising the `has_other_if` residual branch. + #[test] + fn represented_entry_this_way_counter_rider_does_not_hide_unrelated_if() { + let parsed = parse_named( + "Return target creature card from your graveyard to the battlefield. \ + If a Hero enters this way, it enters with two additional +1/+1 counters on it.\n\ + Draw a card if the moon is bright.", + "Heroic Return Compound Fixture", + &["Instant"], + ); + assert!( + has_swallowed_detector(&parsed, "Condition_If"), + "a separate unrelated if line must remain visible to Condition_If, got {:?}", + parsed.parse_warnings + ); + } + /// CR 608.2c: Mister Negative's "If you lost life this way, draw that many /// cards" rider — the "lost life this way" result-reference and "that many" /// draw quantity are jointly represented by `Draw { count: diff --git a/crates/engine/tests/integration/heroic_return_enters_this_way.rs b/crates/engine/tests/integration/heroic_return_enters_this_way.rs new file mode 100644 index 0000000000..b14d94c7bc --- /dev/null +++ b/crates/engine/tests/integration/heroic_return_enters_this_way.rs @@ -0,0 +1,516 @@ +//! Heroic Return / Recommission: a reflexive CR 608.2c "enters this way" rider +//! must not make the spell's reanimation line a CR 614.1c replacement. +//! +//! Before the fix, `is_replacement_pattern` scanned the WHOLE text unit for +//! CR 614.1c classifier tokens. Both cards supply "enters" and "counter" +//! entirely from their rider sentence, so Priority 8 claimed the line, the head +//! "Return target creature card from your graveyard to the battlefield" +//! instruction was dropped on the floor, and the card published a bogus +//! `Moved`/`Battlefield` replacement putting the +1/+1 counters on `SelfRef` — +//! the instant itself, which never enters the battlefield, so the effect was not +//! merely mislocated but unresolvable. `abilities` was empty. +//! +//! The fix head-scopes classification: a CR 608.2c back-reference to an earlier +//! instruction in the same ability contributes no CR 614.1c head tokens. +//! +//! Built via the `/card-test` recipe: `GameScenario` + +//! `GameRunner::cast(..).resolve()` + `CastOutcome` deltas, on verbatim Oracle +//! text from `data/card-data.json`. Every negative assertion is paired with a +//! positive reach-guard in the same test. +//! +//! REVERT DISCRIMINATOR: restore the whole-line token scan in +//! `oracle_classifier::is_replacement_pattern` and `heroic_return_reanimates_hero_with_two_extra_counters` +//! fails at its very first structural guard (`abilities` is empty again), while +//! `heroic_return_parses_to_reanimation_with_conditional_entry_counters` fails on +//! `replacements.is_empty()`. + +use engine::game::casting::legal_target_slots_for_castable_spell; +use engine::game::scenario::{GameScenario, P0, P1}; +use engine::parser::oracle_ir::diagnostic::OracleDiagnostic; +use engine::parser::parse_oracle_text; +use engine::types::ability::{ + ControllerRef, Effect, EffectKind, FilterProp, QuantityExpr, TargetFilter, TargetRef, + TypeFilter, +}; +use engine::types::counter::CounterType; +use engine::types::events::GameEvent; +use engine::types::mana::{ManaCost, ManaCostShard, ManaType, ManaUnit}; +use engine::types::phase::Phase; +use engine::types::zones::Zone; + +/// True when the parse reported a swallowed `Condition_If` clause. +fn has_condition_if_swallow(parsed: &engine::parser::oracle::ParsedAbilities) -> bool { + parsed.parse_warnings.iter().any(|w| { + matches!( + w, + OracleDiagnostic::SwallowedClause { detector, .. } if detector == "Condition_If" + ) + }) +} + +/// Heroic Return {5}{W} Instant — verbatim Oracle text, printed line index 1. +/// (Line index 0 is the cost-reduction static and is included so the fixture is +/// the real card, not a trimmed paraphrase.) +const HEROIC_RETURN: &str = "This spell costs {2} less to cast if a creature is attacking you.\n\ + Return target creature card from your graveyard to the battlefield. If a Hero enters this \ + way, it enters with two additional +1/+1 counters on it."; + +/// Recommission {1}{W} Sorcery — verbatim Oracle text, single printed line. +const RECOMMISSION: &str = "Return target artifact or creature card with mana value 3 or less \ + from your graveyard to the battlefield. If a creature enters this way, it enters with an \ + additional +1/+1 counter on it."; + +fn mana(kind: ManaType, n: usize) -> Vec { + vec![ManaUnit::new(kind, engine::types::identifiers::ObjectId(0), false, vec![]); n] +} + +/// V2 (SHAPE): the head instruction survives as a real `ChangeZone`, and the +/// CR 608.2c rider is folded into `conditional_enter_with_counters` rather than +/// becoming a bogus replacement. +/// +/// The positive shape assertions (exactly one ability, zero `Effect::Unimplemented`, +/// a `ChangeZone{Graveyard -> Battlefield}` head) are what stop the +/// `replacements.is_empty()` negative from passing vacuously on a card that +/// simply failed to parse. +#[test] +fn heroic_return_parses_to_reanimation_with_conditional_entry_counters() { + let parsed = parse_oracle_text( + HEROIC_RETURN, + "Heroic Return", + &[], + &["Instant".to_string()], + &[], + ); + + assert_eq!( + parsed.abilities.len(), + 1, + "the reanimation instruction must survive as the card's one spell ability: {parsed:?}" + ); + let ability = &parsed.abilities[0]; + assert!( + !matches!(ability.effect.as_ref(), Effect::Unimplemented { .. }), + "no Unimplemented residual may survive: {ability:?}" + ); + assert!( + ability.sub_ability.is_none(), + "the rider must be CONSUMED by the fold, not left as a sub-ability: {ability:?}" + ); + + // CR 614.1c: nothing on this card is a replacement effect. Before the fix + // this held a `PutCounter { target: SelfRef }` on the instant itself. + assert!( + parsed.replacements.is_empty(), + "a CR 608.2c back-reference must not classify the line as a CR 614.1c \ + replacement: {parsed:?}" + ); + + let Effect::ChangeZone { + origin, + destination, + target, + conditional_enter_with_counters, + .. + } = ability.effect.as_ref() + else { + panic!("head must be ChangeZone, got {:#?}", ability.effect); + }; + assert_eq!(*origin, Some(Zone::Graveyard)); + assert_eq!(*destination, Zone::Battlefield); + + // CR 109.5 + CR 400.3: "from your graveyard" is a PLAYER-SCOPED query on a + // non-battlefield zone. Its representation is `InZone { Graveyard }` + + // `ControllerRef::You` on the typed filter — the player scope is carried by + // `TypedFilter::controller`, and the owner substitution CR 400.3 requires is an + // evaluator concern (`game::filter::matches_target_filter_in_owner_zone` + // re-evaluates the same filter with ownership standing in for controller), + // not a second filter representation. Pinned here so head-scoping cannot + // silently drop either half of the scope: dropping `InZone` would make every + // graveyard on the table eligible, and dropping `ControllerRef::You` would make + // an opponent's graveyard eligible. + let TargetFilter::Typed(typed) = target else { + panic!("target must be a typed filter, got {target:?}"); + }; + assert!( + typed.type_filters.contains(&TypeFilter::Creature), + "target must be the graveyard creature card, got {target:?}" + ); + assert_eq!( + typed.controller, + Some(ControllerRef::You), + "\"your graveyard\" must survive as a You-scoped player constraint, got {target:?}" + ); + assert!( + typed.properties.contains(&FilterProp::InZone { + zone: Zone::Graveyard + }), + "the graveyard zone constraint must survive head-scoping, got {target:?}" + ); + + // CR 122.1 + CR 614.12: the rider's counters ride the ENTRY, keyed on the + // Hero-ness of the entering object. + let [(filter, counter_type, count)] = conditional_enter_with_counters.as_slice() else { + panic!("expected exactly one conditional entry rider: {conditional_enter_with_counters:?}"); + }; + assert!( + matches!(filter, TargetFilter::Typed(t) + if t.type_filters.contains(&TypeFilter::Subtype("Hero".to_string()))), + "rider filter must be the Hero subtype, got {filter:?}" + ); + assert_eq!(*counter_type, CounterType::Plus1Plus1); + assert_eq!(*count, QuantityExpr::Fixed { value: 2 }); +} + +/// V2 (SHAPE), sibling axis: Recommission exercises the SAME seam with a +/// different filter (disjunctive type + mana-value property) and a different +/// count, so the fix is proven class-level rather than card-shaped. +#[test] +fn recommission_parses_to_reanimation_with_conditional_entry_counters() { + let parsed = parse_oracle_text( + RECOMMISSION, + "Recommission", + &[], + &["Sorcery".to_string()], + &[], + ); + + assert_eq!(parsed.abilities.len(), 1, "{parsed:?}"); + assert!( + parsed.replacements.is_empty(), + "Recommission must not publish a replacement: {parsed:?}" + ); + let ability = &parsed.abilities[0]; + assert!( + !matches!(ability.effect.as_ref(), Effect::Unimplemented { .. }), + "{ability:?}" + ); + + let Effect::ChangeZone { + origin, + destination, + target, + conditional_enter_with_counters, + .. + } = ability.effect.as_ref() + else { + panic!("head must be ChangeZone, got {:#?}", ability.effect); + }; + assert_eq!(*origin, Some(Zone::Graveyard)); + assert_eq!(*destination, Zone::Battlefield); + // The disjunctive artifact-or-creature subject must survive the head parse. + assert!( + matches!(target, TargetFilter::Or { filters } if filters.len() == 2), + "target must be the artifact-or-creature disjunction, got {target:?}" + ); + + let [(filter, counter_type, count)] = conditional_enter_with_counters.as_slice() else { + panic!("expected exactly one conditional entry rider: {conditional_enter_with_counters:?}"); + }; + assert!( + matches!(filter, TargetFilter::Typed(t) if t.type_filters.contains(&TypeFilter::Creature)), + "rider filter must be Creature, got {filter:?}" + ); + assert_eq!(*counter_type, CounterType::Plus1Plus1); + assert_eq!(*count, QuantityExpr::Fixed { value: 1 }); +} + +/// V3 + V4 (RUNTIME): the primary regression. +/// +/// CR 614.12: a replacement effect that modifies how a permanent enters the +/// battlefield applies AT ENTRY, before the object is on the battlefield — so +/// the Hero arrives already carrying its two extra counters, and no separate +/// post-move `PutCounter` effect resolves (V4). +/// +/// Multi-authority hostile fixture, all in ONE game so the branches are +/// distinguished rather than merely exercised: +/// * P0's graveyard holds a Hero AND a non-Hero creature — proves the counter +/// rider binds to the Hero-ness of the ENTERING OBJECT +/// (`matches_target_filter` inside `enter_with_counters_for_object`), not to +/// the spell; +/// * P1's graveyard holds a Hero too — proves the reanimation target binds to +/// `controller: You` + `InZone: Graveyard`, so an opponent's Hero is never a +/// legal target. +#[test] +fn heroic_return_reanimates_hero_with_two_extra_counters() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + let spell = scenario + .add_spell_to_hand_from_oracle(P0, "Heroic Return", true, HEROIC_RETURN) + .with_mana_cost(ManaCost::Cost { + generic: 5, + shards: vec![ManaCostShard::White], + }) + .id(); + let my_hero = scenario + .add_creature_to_graveyard(P0, "Graveyard Hero", 2, 2) + .with_subtypes(vec!["Hero"]) + .id(); + let my_non_hero = scenario + .add_creature_to_graveyard(P0, "Graveyard Bear", 2, 2) + .with_subtypes(vec!["Bear"]) + .id(); + let enemy_hero = scenario + .add_creature_to_graveyard(P1, "Enemy Hero", 2, 2) + .with_subtypes(vec!["Hero"]) + .id(); + scenario.with_mana_pool(P0, { + let mut pool = mana(ManaType::White, 1); + pool.extend(mana(ManaType::Colorless, 5)); + pool + }); + let mut runner = scenario.build(); + + // Structural reach-guard: the card really parsed to the reanimation ability. + // Without this, the "0 counters" and "no PutCounter event" negatives below + // would pass just as well on the pre-fix card, whose `abilities` was empty. + let spell_abilities = &runner.state().objects[&spell].abilities; + assert_eq!( + spell_abilities.len(), + 1, + "premise: Heroic Return must carry its reanimation ability: {spell_abilities:?}" + ); + assert!( + matches!( + spell_abilities[0].effect.as_ref(), + Effect::ChangeZone { + destination: Zone::Battlefield, + .. + } + ), + "premise: the ability must be the reanimation ChangeZone: {spell_abilities:?}" + ); + + // CR 109.5 + CR 400.3: "from YOUR graveyard" restricts the reanimation to P0's + // graveyard — an opponent's graveyard Hero is never a legal target. This proves + // the head's `controller: You` + `InZone: Graveyard` filter survived + // head-scoping and is enforced at enumeration, not merely present in the AST. + // The owner-vs-stale-controller axis is separated in + // `heroic_return_targets_by_owner_not_stale_controller` below. + let slots = legal_target_slots_for_castable_spell(runner.state(), spell); + let slot = slots + .first() + .expect("the reanimation ability must publish a target slot"); + let legal: Vec<_> = slot.legal_targets.iter().collect(); + assert!( + legal + .iter() + .any(|t| matches!(t, TargetRef::Object(id) if *id == my_hero)), + "your own graveyard Hero must be targetable: {legal:?}" + ); + assert!( + legal + .iter() + .any(|t| matches!(t, TargetRef::Object(id) if *id == my_non_hero)), + "your own graveyard non-Hero must be targetable (the Hero filter gates \ + COUNTERS, not targeting): {legal:?}" + ); + assert!( + !legal + .iter() + .any(|t| matches!(t, TargetRef::Object(id) if *id == enemy_hero)), + "an OPPONENT's graveyard Hero must never be a legal target: {legal:?}" + ); + + let outcome = runner.cast(spell).target_objects(&[my_hero]).resolve(); + + // CR 614.12: the Hero enters, already carrying exactly two extra +1/+1 + // counters. Exact, not `>= 1` — the count axis is part of the claim. + outcome.assert_zone(&[my_hero], Zone::Battlefield); + outcome.assert_counters(my_hero, CounterType::Plus1Plus1, 2); + + // The untouched fixtures stay put: the effect returns ONE target. + outcome.assert_zone(&[my_non_hero, enemy_hero], Zone::Graveyard); + + // V4: the counters rode the ENTRY pipeline. A post-move `PutCounter` would + // be a different (and rules-wrong) implementation — it would apply after the + // object is already on the battlefield, so CR 614.12's "as it enters" + // ordering, and anything keyed on the entering characteristics, would differ. + assert!( + !outcome.events().iter().any(|e| matches!( + e, + GameEvent::EffectResolved { + kind: EffectKind::PutCounter, + .. + } + )), + "the rider's counters must ride the entry, not resolve as a separate \ + PutCounter effect: {:?}", + outcome.events() + ); +} + +/// CR 400.3 + CR 109.5 + CR 110.1 (RUNTIME): "your graveyard" is an OWNERSHIP claim, +/// so the reanimation target set follows `obj.owner`, never a stale `obj.controller`. +/// +/// Both fixtures separate the two players on the two fields independently — the +/// preceding test's cards keep owner == controller, so they cannot distinguish the +/// scopes and this one exists to do exactly that: +/// * `mine_stolen` — owner P0, controller P1. Legal: it sits in P0's graveyard. +/// * `theirs_stolen` — owner P1, controller P0. Illegal: it sits in P1's graveyard. +/// +/// The state is not synthetic. `effects::change_zone` documents that a creature +/// stolen via Mind Control retains `obj.controller = thief` after dying into its +/// OWNER's graveyard, because `reset_for_battlefield_exit` does not reset controller +/// and the layer pass that would skips objects off the battlefield. Under +/// controller-scoped enumeration the two assertions below invert exactly: P0's own +/// card vanishes from its own query and the opponent's card becomes targetable. +/// +/// REVERT DISCRIMINATOR: restore `matches_target_filter` in +/// `game::targeting::add_zone_targets` and both assertions fail. +#[test] +fn heroic_return_targets_by_owner_not_stale_controller() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + let spell = scenario + .add_spell_to_hand_from_oracle(P0, "Heroic Return", true, HEROIC_RETURN) + .with_mana_cost(ManaCost::Cost { + generic: 5, + shards: vec![ManaCostShard::White], + }) + .id(); + // Owner P0 — lands in P0's graveyard, so it is in "your graveyard" for P0. + let mine_stolen = scenario + .add_creature_to_graveyard(P0, "My Stolen Hero", 2, 2) + .with_subtypes(vec!["Hero"]) + .id(); + // Owner P1 — lands in P1's graveyard, so it is NOT in "your graveyard" for P0. + let theirs_stolen = scenario + .add_creature_to_graveyard(P1, "Their Stolen Hero", 2, 2) + .with_subtypes(vec!["Hero"]) + .id(); + scenario.with_mana_pool(P0, { + let mut pool = mana(ManaType::White, 1); + pool.extend(mana(ManaType::Colorless, 5)); + pool + }); + let mut runner = scenario.build(); + + // Stamp the divergence AFTER the build so the zone movers cannot normalize it: + // each card's controller is the OTHER player than its owner. + runner + .state_mut() + .objects + .get_mut(&mine_stolen) + .expect("fixture present") + .controller = P1; + runner + .state_mut() + .objects + .get_mut(&theirs_stolen) + .expect("fixture present") + .controller = P0; + + // Premise: the divergence really is staged, so neither assertion below can pass + // by accident on a state where owner and controller still coincide. + for (id, owner, controller) in [(mine_stolen, P0, P1), (theirs_stolen, P1, P0)] { + let obj = &runner.state().objects[&id]; + assert_eq!(obj.owner, owner, "fixture owner for {}", obj.name); + assert_eq!( + obj.controller, controller, + "fixture stale controller for {}", + obj.name + ); + } + + let slots = legal_target_slots_for_castable_spell(runner.state(), spell); + let slot = slots + .first() + .expect("the reanimation ability must publish a target slot"); + let legal: Vec<_> = slot.legal_targets.iter().collect(); + + assert!( + legal + .iter() + .any(|t| matches!(t, TargetRef::Object(id) if *id == mine_stolen)), + "a card YOU OWN in your graveyard must be targetable even while a stale \ + opponent controller clings to it: {legal:?}" + ); + assert!( + !legal + .iter() + .any(|t| matches!(t, TargetRef::Object(id) if *id == theirs_stolen)), + "a card an OPPONENT OWNS must never enter your \"your graveyard\" query, \ + however it is controlled: {legal:?}" + ); + + // End-to-end: the owner-scoped target actually resolves, so the fix holds + // through casting and not merely through enumeration. + let outcome = runner.cast(spell).target_objects(&[mine_stolen]).resolve(); + outcome.assert_zone(&[mine_stolen], Zone::Battlefield); + outcome.assert_counters(mine_stolen, CounterType::Plus1Plus1, 2); + outcome.assert_zone(&[theirs_stolen], Zone::Graveyard); +} + +/// V3 (RUNTIME), the negative branch of the same seam: a non-Hero reanimated by +/// the same spell in the same shape gets NO extra counters. +/// +/// This is the discriminator for `matches_target_filter` inside +/// `enter_with_counters_for_object` — an implementation that applied the counters +/// unconditionally passes the positive test above and fails here. +#[test] +fn heroic_return_gives_a_non_hero_no_extra_counters() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + let spell = scenario + .add_spell_to_hand_from_oracle(P0, "Heroic Return", true, HEROIC_RETURN) + .with_mana_cost(ManaCost::Cost { + generic: 5, + shards: vec![ManaCostShard::White], + }) + .id(); + let bear = scenario + .add_creature_to_graveyard(P0, "Graveyard Bear", 2, 2) + .with_subtypes(vec!["Bear"]) + .id(); + scenario.with_mana_pool(P0, { + let mut pool = mana(ManaType::White, 1); + pool.extend(mana(ManaType::Colorless, 5)); + pool + }); + let mut runner = scenario.build(); + + let outcome = runner.cast(spell).target_objects(&[bear]).resolve(); + + // Positive reach-guard: the reanimation genuinely happened, so the zero-count + // assertion cannot pass because nothing resolved. + outcome.assert_zone(&[bear], Zone::Battlefield); + outcome.assert_counters(bear, CounterType::Plus1Plus1, 0); +} + +/// V5 (COVERAGE HONESTY): the represented `Condition_If` gate must stop being +/// reported as a swallowed clause. +/// +/// `check_swallowed_clauses` early-returns on `Effect::Unimplemented`, so the +/// negative is paired with the positive AST shape in the same test. +#[test] +fn heroic_return_reports_no_swallowed_condition() { + for (name, oracle, types) in [ + ("Heroic Return", HEROIC_RETURN, "Instant"), + ("Recommission", RECOMMISSION, "Sorcery"), + ] { + let parsed = parse_oracle_text(oracle, name, &[], &[types.to_string()], &[]); + // Reach-guards. + assert_eq!(parsed.abilities.len(), 1, "{name}: {parsed:?}"); + assert!( + !matches!( + parsed.abilities[0].effect.as_ref(), + Effect::Unimplemented { .. } + ), + "{name} must parse with zero Unimplemented: {parsed:?}" + ); + assert!( + matches!( + parsed.abilities[0].effect.as_ref(), + Effect::ChangeZone { conditional_enter_with_counters, .. } + if !conditional_enter_with_counters.is_empty() + ), + "{name}'s rider must be represented by the typed slot: {parsed:?}" + ); + assert!( + !has_condition_if_swallow(&parsed), + "{name}: a represented CR 608.2c entry rider must not report a swallowed \ + clause: {:?}", + parsed.parse_warnings + ); + } +} diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index 8f61a29cbd..9adb464879 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -289,6 +289,7 @@ mod hawkeye_avenging_archer_dealt_damage_draw; mod heist_production_path_handoff; mod hellkite_tyrant_steal_artifacts_2906; mod heroic_defiance_recipient_color_4590; +mod heroic_return_enters_this_way; mod hit_the_mother_lode; mod hogaak_cant_spend_mana_1095; mod hollow_one_cost_reduction;