diff --git a/crates/engine/src/database/synthesis.rs b/crates/engine/src/database/synthesis.rs index 10388e1f4b..e18b8c9a9e 100644 --- a/crates/engine/src/database/synthesis.rs +++ b/crates/engine/src/database/synthesis.rs @@ -9712,7 +9712,7 @@ pub fn synthesize_siege_intrinsics(face: &mut CardFace) { protector_replacement.execute = Some(Box::new(AbilityDefinition::new( AbilityKind::Spell, Effect::Choose { - choice_type: ChoiceType::Opponent { restriction: None }, + choice_type: ChoiceType::opponent(), persist: true, selection: crate::types::ability::TargetSelectionMode::Chosen, }, @@ -9841,7 +9841,7 @@ pub fn synthesize_tribute_intrinsics(face: &mut CardFace) { let choose_stage = AbilityDefinition::new( AbilityKind::Spell, Effect::Choose { - choice_type: ChoiceType::Opponent { restriction: None }, + choice_type: ChoiceType::opponent(), persist: true, selection: crate::types::ability::TargetSelectionMode::Chosen, }, diff --git a/crates/engine/src/game/ability_rw.rs b/crates/engine/src/game/ability_rw.rs index ceaab2f1dc..4db0b41672 100644 --- a/crates/engine/src/game/ability_rw.rs +++ b/crates/engine/src/game/ability_rw.rs @@ -3435,7 +3435,7 @@ fn legacy_guess_subject(subject: &GuessSubject) -> bool { fn legacy_choice_type(choice_type: &crate::types::ability::ChoiceType) -> bool { match choice_type { - crate::types::ability::ChoiceType::Opponent { restriction } => { + crate::types::ability::ChoiceType::Opponent { restriction, .. } => { restriction.as_deref().is_some_and(legacy_player_filter) } crate::types::ability::ChoiceType::CreatureType { .. } @@ -3449,7 +3449,7 @@ fn legacy_choice_type(choice_type: &crate::types::ability::ChoiceType) -> bool { | crate::types::ability::ChoiceType::LandType | crate::types::ability::ChoiceType::CardPredicate { .. } | crate::types::ability::ChoiceType::CardPredicateGuess { .. } - | crate::types::ability::ChoiceType::Player + | crate::types::ability::ChoiceType::Player { .. } | crate::types::ability::ChoiceType::TwoColors | crate::types::ability::ChoiceType::Word | crate::types::ability::ChoiceType::Artist @@ -5728,7 +5728,7 @@ fn rw_guess_subject(subject: &GuessSubject) -> RwProfile { fn rw_choice_type(choice_type: &crate::types::ability::ChoiceType) -> RwProfile { match choice_type { - crate::types::ability::ChoiceType::Opponent { restriction } => match restriction { + crate::types::ability::ChoiceType::Opponent { restriction, .. } => match restriction { Some(filter) => rw_player_filter(filter), None => RwProfile::empty(), }, @@ -5743,7 +5743,7 @@ fn rw_choice_type(choice_type: &crate::types::ability::ChoiceType) -> RwProfile | crate::types::ability::ChoiceType::LandType | crate::types::ability::ChoiceType::CardPredicate { .. } | crate::types::ability::ChoiceType::CardPredicateGuess { .. } - | crate::types::ability::ChoiceType::Player + | crate::types::ability::ChoiceType::Player { .. } | crate::types::ability::ChoiceType::TwoColors | crate::types::ability::ChoiceType::Word | crate::types::ability::ChoiceType::Artist @@ -6824,7 +6824,7 @@ mod tests { #[test] fn b7_choose_persist_member_bound() { let choose = |persist: bool| Effect::Choose { - choice_type: ChoiceType::Opponent { restriction: None }, + choice_type: ChoiceType::opponent(), persist, selection: TargetSelectionMode::default(), }; diff --git a/crates/engine/src/game/coverage.rs b/crates/engine/src/game/coverage.rs index e81afd2fea..285a24f6a9 100644 --- a/crates/engine/src/game/coverage.rs +++ b/crates/engine/src/game/coverage.rs @@ -2030,7 +2030,7 @@ fn fmt_choice_type(ct: &ChoiceType) -> String { ChoiceType::CardPredicate { .. } => "card predicate", ChoiceType::CardPredicateGuess { .. } => "card predicate guess", ChoiceType::Opponent { .. } => "opponent", - ChoiceType::Player => "player", + ChoiceType::Player { .. } => "player", ChoiceType::TwoColors => "two colors", ChoiceType::Word => "word", ChoiceType::Artist => "artist", diff --git a/crates/engine/src/game/effects/choose.rs b/crates/engine/src/game/effects/choose.rs index 452ea02fdd..e218acba8d 100644 --- a/crates/engine/src/game/effects/choose.rs +++ b/crates/engine/src/game/effects/choose.rs @@ -2,8 +2,8 @@ use rand::Rng; use crate::game::players; use crate::types::ability::{ - ChoiceType, ChoiceValue, ChosenAttribute, Effect, EffectError, EffectKind, ResolvedAbility, - SeatDirection, TargetSelectionMode, + ChoiceType, ChoiceValue, ChosenAttribute, Effect, EffectError, EffectKind, + PlayerChoiceDistinctness, ResolvedAbility, SeatDirection, TargetSelectionMode, }; use crate::types::card_type::CoreType; use crate::types::events::GameEvent; @@ -180,7 +180,7 @@ pub(crate) fn resolve_random_in_chain( // it to the sub via `apply_parent_chain_context`. if matches!( choice_type, - ChoiceType::Player | ChoiceType::Opponent { .. } + ChoiceType::Player { .. } | ChoiceType::Opponent { .. } ) { if let Ok(pid) = chosen.parse::() { let mut updated = ability.chosen_players.clone(); @@ -306,7 +306,7 @@ pub(crate) fn bind_named_choice( | ChoiceType::BasicLandType | ChoiceType::Color { .. } | ChoiceType::Keyword { .. } - | ChoiceType::Player + | ChoiceType::Player { .. } | ChoiceType::Opponent { .. } // CR 613.1: A persisted `Label` gates `ChosenLabelIs` // continuous statics — anchor-word modal permanents @@ -505,12 +505,16 @@ const LAND_TYPES: &[&str] = &[ /// casting or resolution. If an option would be illegal, it can't be chosen. /// /// `already_chosen` is the resolution-scoped list of players picked by earlier -/// `Choose(Player)` instructions in this chain. CR 608.2c + the Gluntch card -/// ruling ("three distinct players") require each successive "choose a player" -/// to exclude players already chosen — `ChoiceType::Player` and -/// `ChoiceType::Opponent` filter them out. When fewer eligible players remain -/// than the card asks for, the options list is empty and the choice (and its -/// dependent effect) does nothing — the standard empty-options path. +/// `Choose(Player)` instructions in this chain. `ChoiceType::Player` and +/// `ChoiceType::Opponent` only consult it when their `distinctness` is +/// `DistinctFromPriorChoices` (CR 608.2c + the Gluntch ordinal-cued "choose a +/// second/third player" ruling, "three distinct players"). The default +/// `Independent` distinctness never filters on it — the "Offering" cycle +/// ruling (Benevolent/Infernal/Intellectual/Sylvan Offering) confirms a +/// repeated "Choose an opponent." may pick the same player again. When +/// `DistinctFromPriorChoices` narrows the eligible set below what the card +/// asks for, the options list is empty and the choice (and its dependent +/// effect) does nothing — the standard empty-options path. fn compute_options( state: &GameState, choice_type: &ChoiceType, @@ -612,15 +616,23 @@ fn compute_options( // (in a free-for-all game, every other player). `players::opponents` // already drops eliminated players (CR 104.3a — a player who loses // leaves the game and is no longer an opponent). - // CR 608.2c: Exclude players already chosen earlier in this resolution. + // CR 608.2c: `DistinctFromPriorChoices` excludes players already chosen + // earlier in this resolution; the default `Independent` does not (the + // "Offering" cycle may repeat the same opponent). // CR 102.3 + CR 608.2d: When a `restriction` is present ("with the most // life among your opponents"), narrow the eligible set to opponents // satisfying that `PlayerFilter` — the controller then picks ONE of the // qualifying opponents (CR 608.2d handles ties), keeping it a single // pick rather than fanning the effect out to every tied opponent. - ChoiceType::Opponent { restriction } => players::opponents(state, controller) + ChoiceType::Opponent { + restriction, + distinctness, + } => players::opponents(state, controller) .iter() - .filter(|id| !already_chosen.contains(id)) + .filter(|id| { + *distinctness != PlayerChoiceDistinctness::DistinctFromPriorChoices + || !already_chosen.contains(id) + }) .filter(|id| { restriction.as_ref().is_none_or(|filter| { super::matches_player_scope(state, **id, filter, controller, source_id) @@ -629,11 +641,16 @@ fn compute_options( .map(|id| id.0.to_string()) .collect(), // CR 102.1: A player is one of the people in the game. - // CR 608.2c: Exclude players already chosen earlier in this resolution. - ChoiceType::Player => state + // CR 608.2c: `DistinctFromPriorChoices` (Gluntch's "choose a + // second/third player") excludes players already chosen earlier in + // this resolution; the default `Independent` does not. + ChoiceType::Player { distinctness } => state .seat_order .iter() - .filter(|id| !already_chosen.contains(id)) + .filter(|id| { + *distinctness != PlayerChoiceDistinctness::DistinctFromPriorChoices + || !already_chosen.contains(id) + }) .map(|id| id.0.to_string()) .collect(), ChoiceType::TwoColors => two_color_options(), @@ -1266,7 +1283,7 @@ mod tests { #[test] fn choose_opponent_lists_opponents() { let mut state = GameState::new_two_player(42); - let ability = make_choose_ability(ChoiceType::Opponent { restriction: None }); + let ability = make_choose_ability(ChoiceType::opponent()); let mut events = Vec::new(); resolve(&mut state, &ability, &mut events).unwrap(); match &state.waiting_for { @@ -1278,10 +1295,56 @@ mod tests { } } + /// Issue #6381 (Benevolent Offering): the "Offering" cycle ruling — + /// "You may choose the same opponent for each of the effects, or you may + /// choose different opponents" — means the default `Independent` + /// distinctness must NOT exclude an opponent chosen by an earlier + /// `Choose(Opponent)` in the same resolution. In a two-player game this is + /// the difference between a legal repeat pick (correct) and an impossible + /// no-op second choice (the reported bug). + #[test] + fn choose_opponent_independent_by_default_allows_repeat_choice() { + let mut state = GameState::new_two_player(42); + let mut ability = make_choose_ability(ChoiceType::opponent()); + ability.chosen_players = vec![PlayerId(1)]; + let mut events = Vec::new(); + resolve(&mut state, &ability, &mut events).unwrap(); + match &state.waiting_for { + WaitingFor::NamedChoice { options, .. } => { + assert_eq!( + options, + &["1"], + "the previously-chosen opponent must remain a legal repeat pick" + ); + } + other => panic!("Expected NamedChoice, got {:?}", other), + } + } + #[test] fn choose_player_lists_all_players() { let mut state = GameState::new_two_player(42); - let ability = make_choose_ability(ChoiceType::Player); + let ability = make_choose_ability(ChoiceType::player()); + let mut events = Vec::new(); + resolve(&mut state, &ability, &mut events).unwrap(); + match &state.waiting_for { + WaitingFor::NamedChoice { options, .. } => { + assert_eq!(options.len(), 2); + assert!(options.contains(&"0".to_string())); + assert!(options.contains(&"1".to_string())); + } + other => panic!("Expected NamedChoice, got {:?}", other), + } + } + + #[test] + fn choose_player_independent_by_default_allows_repeat_choice() { + // The default `Independent` distinctness (bare "choose a player") does + // NOT exclude a player already chosen earlier in this resolution — + // only the ordinal-cued `DistinctFromPriorChoices` (Gluntch) does. + let mut state = GameState::new_two_player(42); + let mut ability = make_choose_ability(ChoiceType::player()); + ability.chosen_players = vec![PlayerId(0)]; let mut events = Vec::new(); resolve(&mut state, &ability, &mut events).unwrap(); match &state.waiting_for { @@ -1295,11 +1358,12 @@ mod tests { } #[test] - fn choose_player_excludes_already_chosen_players() { - // CR 608.2c + Gluntch ruling: a successive "choose a player" omits - // players already chosen earlier in the same resolution. + fn choose_player_distinct_from_prior_excludes_already_chosen_players() { + // CR 608.2c + Gluntch ruling ("choose a second/third player"): a + // successive `DistinctFromPriorChoices` pick omits players already + // chosen earlier in the same resolution. let mut state = GameState::new_two_player(42); - let mut ability = make_choose_ability(ChoiceType::Player); + let mut ability = make_choose_ability(ChoiceType::player_distinct_from_prior()); ability.chosen_players = vec![PlayerId(0)]; let mut events = Vec::new(); resolve(&mut state, &ability, &mut events).unwrap(); @@ -1312,7 +1376,7 @@ mod tests { } #[test] - fn choose_player_with_all_players_chosen_resolves_as_no_op() { + fn choose_player_distinct_from_prior_with_all_players_chosen_resolves_as_no_op() { // CR 609.3 (issue #3040): when every eligible player is already chosen, // the engine-enumerated option set is empty — choosing is impossible, so // the choice does nothing and resolution continues. It must NOT raise a @@ -1324,7 +1388,7 @@ mod tests { state.waiting_for = WaitingFor::Priority { player: PlayerId(0), }; - let mut ability = make_choose_ability(ChoiceType::Player); + let mut ability = make_choose_ability(ChoiceType::player_distinct_from_prior()); ability.chosen_players = vec![PlayerId(0), PlayerId(1)]; let mut events = Vec::new(); resolve(&mut state, &ability, &mut events).unwrap(); @@ -1425,7 +1489,7 @@ mod tests { let mut state = GameState::new_two_player(42); let mut ability = ResolvedAbility::new( Effect::Choose { - choice_type: ChoiceType::Player, + choice_type: ChoiceType::player(), persist: false, selection: TargetSelectionMode::Random, }, @@ -1454,7 +1518,7 @@ mod tests { // Building-block regression: a Chosen Choose is left to the interactive // `resolve` path (returns false; raises nothing here). let mut state = GameState::new_two_player(42); - let mut ability = make_choose_ability(ChoiceType::Player); + let mut ability = make_choose_ability(ChoiceType::player()); let mut events = Vec::new(); assert!(!resolve_random_in_chain( &mut state, diff --git a/crates/engine/src/game/engine_resolution_choices.rs b/crates/engine/src/game/engine_resolution_choices.rs index e9ca9781b0..edf3f0a465 100644 --- a/crates/engine/src/game/engine_resolution_choices.rs +++ b/crates/engine/src/game/engine_resolution_choices.rs @@ -5633,7 +5633,7 @@ pub(super) fn handle_resolution_choice( // single GameState slot cleared after every drain. if matches!( choice_type, - ChoiceType::Player | ChoiceType::Opponent { .. } + ChoiceType::Player { .. } | ChoiceType::Opponent { .. } ) { if let Ok(pid) = choice.parse::() { if let Some(frame) = state.active_ability_continuation_frame_mut() { diff --git a/crates/engine/src/game/triggers_ordering_parity_tests.rs b/crates/engine/src/game/triggers_ordering_parity_tests.rs index c0c8b70456..eace11120c 100644 --- a/crates/engine/src/game/triggers_ordering_parity_tests.rs +++ b/crates/engine/src/game/triggers_ordering_parity_tests.rs @@ -1906,7 +1906,7 @@ fn choose_opponent_then_draw() -> ResolvedAbility { target: TargetFilter::Controller, }); ra(Effect::Choose { - choice_type: ChoiceType::Opponent { restriction: None }, + choice_type: ChoiceType::opponent(), persist: false, selection: TargetSelectionMode::default(), }) diff --git a/crates/engine/src/parser/oracle.rs b/crates/engine/src/parser/oracle.rs index 7fb0a1c821..a91e399735 100644 --- a/crates/engine/src/parser/oracle.rs +++ b/crates/engine/src/parser/oracle.rs @@ -1921,7 +1921,7 @@ fn ability_chain_has_player_choice(def: &AbilityDefinition) -> bool { matches!( def.effect.as_ref(), Effect::Choose { - choice_type: ChoiceType::Player | ChoiceType::Opponent { .. }, + choice_type: ChoiceType::Player { .. } | ChoiceType::Opponent { .. }, .. } ) || def @@ -2006,7 +2006,7 @@ fn filter_references_source_chosen_player(filter: &TargetFilter) -> bool { /// sub-ability chain) to `persist: true` so its choice is stored durably. fn persist_player_choice_in_ability(def: &mut AbilityDefinition) { if let Effect::Choose { - choice_type: ChoiceType::Player | ChoiceType::Opponent { .. }, + choice_type: ChoiceType::Player { .. } | ChoiceType::Opponent { .. }, persist, .. } = def.effect.as_mut() diff --git a/crates/engine/src/parser/oracle_effect/mod.rs b/crates/engine/src/parser/oracle_effect/mod.rs index 0a9a92d403..95786e59d7 100644 --- a/crates/engine/src/parser/oracle_effect/mod.rs +++ b/crates/engine/src/parser/oracle_effect/mod.rs @@ -104,14 +104,14 @@ use crate::types::ability::{ EffectScope, FilterProp, GameRestriction, GuessSubject, IntensityScope, IterationKindBinding, KeeperConstraint, LibraryPosition, ManaProduction, ManaSpendPermission, MultiTargetSpec, NumberDistinctness, ObjectProperty, ObjectScope, OriginConstraint, PerpetualModification, - PlayPermissionInvalidation, PlayerFilter, PlayerRelation, PlayerScope, PreventionAmount, - PreventionScope, ProhibitedActivity, PtValue, QuantityExpr, QuantityRef, ReplacementCondition, - ReplacementDefinition, RestrictionExpiry, RestrictionPlayerScope, RevealUntilDisposition, - RoundingMode, SharedQuality, SharedQualityRelation, SiblingCondition, SkipScope, - SpellStackToGraveyardReplacement, StaticCondition, StaticDefinition, StepSkipTarget, - SubAbilityLink, TapStateChange, TargetFilter, TargetSelectionMode, ThisWayCause, - TrackedAnaphorSource, TriggerCondition, TriggerDefinition, TypeFilter, TypedFilter, - UnlessPayModifier, UntilCondition, ZoneOwner, + PlayPermissionInvalidation, PlayerChoiceDistinctness, PlayerFilter, PlayerRelation, + PlayerScope, PreventionAmount, PreventionScope, ProhibitedActivity, PtValue, QuantityExpr, + QuantityRef, ReplacementCondition, ReplacementDefinition, RestrictionExpiry, + RestrictionPlayerScope, RevealUntilDisposition, RoundingMode, SharedQuality, + SharedQualityRelation, SiblingCondition, SkipScope, SpellStackToGraveyardReplacement, + StaticCondition, StaticDefinition, StepSkipTarget, SubAbilityLink, TapStateChange, + TargetFilter, TargetSelectionMode, ThisWayCause, TrackedAnaphorSource, TriggerCondition, + TriggerDefinition, TypeFilter, TypedFilter, UnlessPayModifier, UntilCondition, ZoneOwner, }; #[cfg(test)] use crate::types::ability::{AttackScope, AttackSubject}; @@ -529,7 +529,7 @@ fn finalize_committed_guess_choice_types( _ => unreachable!("matched above"), }; *ability.effect = Effect::Choose { - choice_type: ChoiceType::Opponent { restriction: None }, + choice_type: ChoiceType::opponent(), persist: false, selection: TargetSelectionMode::Chosen, }; @@ -7144,8 +7144,15 @@ fn parse_for_each_object_copy_parts( /// `ChosenPlayer` so a *following* sentence ("They put counters on a creature /// they control") binds its "they"/"that player" anaphora to this choice. /// -/// The ordinal word ("second"/"third") is a parse-time consistency hint only — -/// the engine index is derived from chain position, not the ordinal. +/// The ordinal word ("second"/"third") doubles as the CR 608.2c distinctness +/// signal: its presence marks this pick +/// `PlayerChoiceDistinctness::DistinctFromPriorChoices` (Gluntch, the +/// Bestower — each successive choice must exclude players already chosen +/// earlier in this resolution); its absence keeps the default `Independent`, +/// under which a bare repeated "choose a player"/"choose an opponent" may +/// repeat an earlier choice (confirmed by the "Offering" cycle ruling — +/// Benevolent/Infernal/Intellectual/Sylvan Offering — issue #6381). Either +/// way, the engine index is derived from chain position, not the ordinal. /// CR 102.3 + CR 608.2d: Recognize the "with the most life [among /// your opponents]" qualifier on a "choose an opponent" instruction and lower it /// to the equivalent `PlayerFilter::PlayerAttribute` restriction: each candidate @@ -7208,14 +7215,22 @@ fn try_parse_choose_player_to_verb( // and silently dropped opponent-form choose clauses. let player_arm = |i| { let (i, _) = tag::<_, _, OracleError<'_>>(" ").parse(i)?; - let i = super::oracle_util::parse_ordinal(i) - .map(|(_, rest)| rest) - .unwrap_or(i); + let (i, ordinal_present) = match super::oracle_util::parse_ordinal(i) { + Some((_, rest)) => (rest, true), + None => (i, false), + }; let (i, _) = tag::<_, _, OracleError<'_>>("player").parse(i)?; - Ok::<_, nom::Err>>((i, ChoiceType::Player)) + // CR 608.2c: the ordinal ("second"/"third") is the distinctness signal + // — see the doc comment above. + let distinctness = if ordinal_present { + PlayerChoiceDistinctness::DistinctFromPriorChoices + } else { + PlayerChoiceDistinctness::Independent + }; + Ok::<_, nom::Err>>((i, ChoiceType::Player { distinctness })) }; let opponent_arm = value( - ChoiceType::Opponent { restriction: None }, + ChoiceType::opponent(), tag::<_, _, OracleError<'_>>("n opponent"), ); let (after_player, mut choice_type) = @@ -7226,7 +7241,7 @@ fn try_parse_choose_player_to_verb( // restriction to the `Opponent` choice so it stays a single pick (CR 608.2d // resolves ties) rather than fanning out. Consume the qualifier so it is not // left dangling on the verb tail. - let after_player = if let ChoiceType::Opponent { restriction } = &mut choice_type { + let after_player = if let ChoiceType::Opponent { restriction, .. } = &mut choice_type { match parse_opponent_most_life_restriction(after_player) { Ok((rest, filter)) => { *restriction = Some(Box::new(filter)); @@ -7342,7 +7357,7 @@ fn try_parse_an_opponent_to_verb( } let mut clause = parsed_clause(Effect::Choose { - choice_type: ChoiceType::Opponent { restriction: None }, + choice_type: ChoiceType::opponent(), persist: false, selection: TargetSelectionMode::Chosen, }); @@ -7388,7 +7403,7 @@ fn try_parse_opponent_guesses_chosen_library_kind( }); let mut choose_opponent = parsed_clause(Effect::Choose { - choice_type: ChoiceType::Opponent { restriction: None }, + choice_type: ChoiceType::opponent(), persist: false, selection: TargetSelectionMode::Chosen, }); @@ -14525,13 +14540,25 @@ fn thread_for_each_subject(effect: Effect, original: &str, ctx: &mut ParseContex unless_filter, filter, }, + // CR 119.3 + CR 608.2c (issue #6381): "target player gains N life for + // each X" (issue #1508) needs an actual CR 601.2c target declaration + // (`is_targeted`); "that player gains N life for each X" is instead a + // resolution-scoped anaphor to a player chosen by an earlier "Choose an + // opponent."/"Choose a player." instruction (the "Offering" cycle: + // Benevolent/Infernal/Intellectual/Sylvan Offering) and never sets + // `application.target`. Accept either so both recipient-binding shapes + // rebind away from the no-subject `Controller` default. Effect::GainLife { amount, player: TargetFilter::Controller, - } if is_targeted && target_filter_can_target_player(&target) => Effect::GainLife { - amount, - player: target, - }, + } if target_filter_can_target_player(&target) + && (is_targeted || is_chosen_player_anaphor(&target)) => + { + Effect::GainLife { + amount, + player: target, + } + } // CR 115.1a/c + CR 701.17a + CR 608.2c: "Target opponent/player sacrifices // a [typed] permanent ... for each X" (Urborg Justice, Din of the Fireherd, // Rakdos Riteknife). The for-each interception strips the dynamic count @@ -17837,6 +17864,30 @@ fn try_parse_compound_subject_each( let (consumed_prefix, first_filter, second_filter) = parse_compound_subject_prefix(lower.as_str())?; + // CR 109.4 + CR 608.2c (issue #6381): "that player" in "you and that + // player each ..." is ambiguous prose with two distinct antecedents. The + // grammar's `ScopedPlayer` default is correct for a per-voter/per-opponent + // fan-out body (Master of Ceremonies-style vote bodies), but when a + // PRECEDING "Choose an opponent."/"Choose a player." instruction in the + // same resolution bound `ctx.relative_player_scope` to a + // `ControllerRef::ChosenPlayer { index }` (the "Offering" cycle: + // Benevolent/Infernal/Intellectual/Sylvan Offering), "that player" refers + // to THAT chosen player instead — rebind to match, mirroring + // `rebind_opponent_player_recipient_to_chosen`'s player-only `TargetFilter::Typed` + // encoding. Without this, the second recipient silently defaults to + // `scoped_player_or_controller`'s controller fallback (unset outside a + // fan-out), so every token/effect meant for "that player" was created for + // the caster instead. + let second_filter = match (&second_filter, ctx.relative_player_scope.clone()) { + (TargetFilter::ScopedPlayer, Some(ControllerRef::ChosenPlayer { index })) => { + TargetFilter::Typed(TypedFilter { + controller: Some(ControllerRef::ChosenPlayer { index }), + ..Default::default() + }) + } + _ => second_filter, + }; + // Slice the original-case body text using the consumed offset. let body_text = text[consumed_prefix..].trim(); if body_text.is_empty() { @@ -20474,6 +20525,24 @@ fn target_filter_can_target_player(filter: &TargetFilter) -> bool { } } +/// CR 608.2c: true when `filter` is the resolution-scoped "that player"/"that +/// opponent" anaphor to a player chosen earlier in the same resolution by a +/// `Choose(Player)`/`Choose(Opponent)` instruction, encoded as the player-only +/// `TargetFilter::Typed` carrying `ControllerRef::ChosenPlayer { index }` +/// (mirrors `retarget_effect_to_chosen_player`'s encoding). Distinct from a +/// CR 601.2c cast-time target declaration ("target player"), which sets +/// `SubjectApplication.target` instead. +fn is_chosen_player_anaphor(filter: &TargetFilter) -> bool { + matches!( + filter, + TargetFilter::Typed(TypedFilter { + controller: Some(ControllerRef::ChosenPlayer { .. }), + type_filters, + .. + }) if type_filters.is_empty() + ) +} + fn wrap_target_subject_damage( mut clause: ParsedEffectClause, subject: &SubjectPhraseAst, @@ -20716,6 +20785,27 @@ fn inject_subject_target(effect: &mut Effect, subject: &SubjectPhraseAst) { Effect::AdditionalPhase { target, .. } if *target == TargetFilter::Controller => { *target = subject_filter; } + // CR 119.3 (issue #6381): "that player gains N life" / "target player + // gains N life" — the imperative path defaults `player` to the + // no-subject `Controller` (the "you gain life" reading); inject the + // parsed subject when the sentence actually names a different, + // genuinely player-denoting recipient. Without this arm, EVERY + // "[non-you subject] gains N life" clause silently credited the + // ability's controller instead (the "Offering" cycle's "that player + // gains 2 life for each creature they control" — Benevolent/Infernal/ + // Intellectual/Sylvan Offering). Gated on `target_filter_can_target_player` + // (mirrors the `thread_for_each_subject` GainLife arm) so an + // unresolved compound subject ("you and that player each gain that + // much life" — Angel of Destiny, where the "each"-bodied split + // doesn't cover `GainLife` and falls back to a non-player `Any` + // subject) leaves the safer `Controller` default alone instead of + // rebinding to a nonsensical recipient. + Effect::GainLife { player, .. } + if *player == TargetFilter::Controller + && target_filter_can_target_player(&subject_filter) => + { + *player = subject_filter; + } // CR 400.7: "shuffle [subject]'s graveyard into their library" — inject // subject target for zone-wide changes and shuffles. Effect::ChangeZoneAll { ref mut target, .. } @@ -23325,7 +23415,7 @@ fn opponent_guess_clause(guesser: ControllerRef, subject: GuessSubject) -> Parse // so the chosen player is threaded to the dependent guess as a // same-resolution `ChosenPlayer`. let mut clause = parsed_clause(Effect::Choose { - choice_type: ChoiceType::Opponent { restriction: None }, + choice_type: ChoiceType::opponent(), persist: false, selection: TargetSelectionMode::Chosen, }); @@ -23549,9 +23639,9 @@ pub(crate) fn parse_named_choice_object(rest: &str) -> Option { Some(ChoiceType::LandType) } else if tag::<_, _, E>("an opponent").parse(rest).is_ok() { // CR 800.4a: Choose an opponent from among players in the game. - Some(ChoiceType::Opponent { restriction: None }) + Some(ChoiceType::opponent()) } else if tag::<_, _, E>("a player").parse(rest).is_ok() { - Some(ChoiceType::Player) + Some(ChoiceType::player()) } else if tag::<_, _, E>("two colors").parse(rest).is_ok() { Some(ChoiceType::TwoColors) } else if tag::<_, _, E>("a word").parse(rest).is_ok() { @@ -30264,7 +30354,7 @@ pub(crate) fn parse_effect_chain_ir( if matches!( clause.effect, Effect::Choose { - choice_type: ChoiceType::Player | ChoiceType::Opponent { .. }, + choice_type: ChoiceType::Player { .. } | ChoiceType::Opponent { .. }, .. } ) { diff --git a/crates/engine/src/parser/oracle_effect/snapshot_tests.rs b/crates/engine/src/parser/oracle_effect/snapshot_tests.rs index 44dfe8f139..24045f6569 100644 --- a/crates/engine/src/parser/oracle_effect/snapshot_tests.rs +++ b/crates/engine/src/parser/oracle_effect/snapshot_tests.rs @@ -973,7 +973,10 @@ fn strax_choose_a_player_at_random_records_random_selection() { ); match def.effect.as_ref() { Effect::Choose { - choice_type: ChoiceType::Player, + choice_type: + ChoiceType::Player { + distinctness: PlayerChoiceDistinctness::Independent, + }, selection, .. } => assert_eq!(*selection, TargetSelectionMode::Random), @@ -990,16 +993,20 @@ fn gluntch_choose_player_chain_parses_with_chosen_player_scopes() { AbilityKind::Spell, ); - // Node 0: the first `Choose(Player)`. + // Node 0: the first `Choose(Player)` — no ordinal, so the default + // `Independent` distinctness applies (CR 608.2c; issue #6381 confirms the + // bare "choose a player" must NOT exclude prior choices). assert!( matches!( def.effect.as_ref(), Effect::Choose { - choice_type: ChoiceType::Player, + choice_type: ChoiceType::Player { + distinctness: PlayerChoiceDistinctness::Independent + }, .. } ), - "first node must be Choose(Player), got {:?}", + "first node must be Choose(Player) with Independent distinctness, got {:?}", def.effect ); @@ -1017,17 +1024,21 @@ fn gluntch_choose_player_chain_parses_with_chosen_player_scopes() { "the +1/+1 counters go on a creature the 1st chosen player controls" ); - // Node 2: the second `Choose(Player)`. + // Node 2: the second `Choose(Player)` — "a second player" carries the + // ordinal, so it must be `DistinctFromPriorChoices` (Gluntch's "three + // distinct players" ruling). let node2 = node1.sub_ability.as_ref().expect("2nd Choose node"); assert!( matches!( node2.effect.as_ref(), Effect::Choose { - choice_type: ChoiceType::Player, + choice_type: ChoiceType::Player { + distinctness: PlayerChoiceDistinctness::DistinctFromPriorChoices + }, .. } ), - "node 2 must be Choose(Player) — not Unimplemented — got {:?}", + "node 2 must be Choose(Player) with DistinctFromPriorChoices — not Unimplemented — got {:?}", node2.effect ); @@ -1042,17 +1053,20 @@ fn gluntch_choose_player_chain_parses_with_chosen_player_scopes() { "the 2nd chosen player draws the card" ); - // Node 4: the third `Choose(Player)`. + // Node 4: the third `Choose(Player)` — "a third player" carries the + // ordinal, so it must be `DistinctFromPriorChoices` too. let node4 = node3.sub_ability.as_ref().expect("3rd Choose node"); assert!( matches!( node4.effect.as_ref(), Effect::Choose { - choice_type: ChoiceType::Player, + choice_type: ChoiceType::Player { + distinctness: PlayerChoiceDistinctness::DistinctFromPriorChoices + }, .. } ), - "node 4 must be Choose(Player) — not Unimplemented — got {:?}", + "node 4 must be Choose(Player) with DistinctFromPriorChoices — not Unimplemented — got {:?}", node4.effect ); diff --git a/crates/engine/src/parser/oracle_effect/tests.rs b/crates/engine/src/parser/oracle_effect/tests.rs index 7aecd09b42..2cfbffb6bf 100644 --- a/crates/engine/src/parser/oracle_effect/tests.rs +++ b/crates/engine/src/parser/oracle_effect/tests.rs @@ -24452,7 +24452,10 @@ fn gollum_scheming_guide_guess_sequence_has_no_unimplemented() { && matches!( node.effect.as_ref(), Effect::Choose { - choice_type: ChoiceType::Opponent { restriction: None }, + choice_type: ChoiceType::Opponent { + restriction: None, + .. + }, persist: false, .. } @@ -24665,7 +24668,10 @@ fn committed_choice_guess_chooses_single_opponent_before_guess() { matches!( choose_opponent.effect.as_ref(), Effect::Choose { - choice_type: ChoiceType::Opponent { restriction: None }, + choice_type: ChoiceType::Opponent { + restriction: None, + .. + }, persist: false, .. } @@ -39190,6 +39196,7 @@ fn the_master_most_life_villainous_choice() { choice_type: ChoiceType::Opponent { restriction: Some(restriction), + .. }, persist, .. diff --git a/crates/engine/src/parser/oracle_replacement.rs b/crates/engine/src/parser/oracle_replacement.rs index 3b7bb0a50c..52b57a484d 100644 --- a/crates/engine/src/parser/oracle_replacement.rs +++ b/crates/engine/src/parser/oracle_replacement.rs @@ -2113,7 +2113,7 @@ fn front_opponent_choice_for_nontargeted_look(reveal: &Effect) -> Option<(Effect // observable outcome. let choose_opponent = Effect::Choose { // CR 608.2d + CR 102.3: the controller chooses one opponent. - choice_type: ChoiceType::Opponent { restriction: None }, + choice_type: ChoiceType::opponent(), persist: true, // Same controller-choice selection mode as the fronted card-name choice. selection: crate::types::ability::TargetSelectionMode::Chosen, @@ -14752,7 +14752,10 @@ mod tests { matches!( &*execute.effect, Effect::Choose { - choice_type: ChoiceType::Opponent { restriction: None }, + choice_type: ChoiceType::Opponent { + restriction: None, + .. + }, persist: true, .. } @@ -14863,7 +14866,10 @@ mod tests { matches!( &*mid.effect, Effect::Choose { - choice_type: ChoiceType::Opponent { restriction: None }, + choice_type: ChoiceType::Opponent { + restriction: None, + .. + }, persist: true, .. } diff --git a/crates/engine/src/parser/oracle_vote.rs b/crates/engine/src/parser/oracle_vote.rs index 13fb10298a..45ccb9afc6 100644 --- a/crates/engine/src/parser/oracle_vote.rs +++ b/crates/engine/src/parser/oracle_vote.rs @@ -1210,10 +1210,13 @@ fn parse_vote_for_each_suffix_clause<'a>( opt(tag_no_case("then ")), alt(( value( - ChoiceType::Opponent { restriction: None }, + ChoiceType::opponent(), tag_no_case("choose an opponent at random"), ), - value(ChoiceType::Player, tag_no_case("choose a player at random")), + value( + ChoiceType::player(), + tag_no_case("choose a player at random"), + ), )), tag(". "), )) @@ -2397,7 +2400,10 @@ mod tests { assert_eq!(rest, ""); assert!(matches!( setup, - Some(ChoiceType::Opponent { restriction: None }) + Some(ChoiceType::Opponent { + restriction: None, + .. + }) )); match &*def.effect { Effect::DealDamage { amount, target, .. } => { diff --git a/crates/engine/src/types/ability.rs b/crates/engine/src/types/ability.rs index 22a671cb91..aaf1b5b3f1 100644 --- a/crates/engine/src/types/ability.rs +++ b/crates/engine/src/types/ability.rs @@ -525,6 +525,28 @@ pub enum NumberDistinctness { DistinctFromSourceHistory, } +/// CR 608.2c: whether a "choose a player"/"choose an opponent" instruction +/// must exclude players already chosen earlier in the SAME resolution, or is +/// an independent pick that may repeat an earlier choice. Parse-detected; +/// static; serialized only when non-default so existing `Player`/`Opponent` +/// card-data stays byte-stable. Mirrors `NumberDistinctness`'s axis on the +/// sibling `NumberRange` choice. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default, Serialize, Deserialize)] +pub enum PlayerChoiceDistinctness { + /// The default: repeated "Choose an opponent."/"Choose a player." + /// instructions in one resolution are independent picks — the same + /// player may be chosen more than once. Confirmed by the "Offering" cycle + /// ruling (Benevolent/Infernal/Intellectual/Sylvan Offering): "You may + /// choose the same opponent for each of the effects, or you may choose + /// different opponents." + #[default] + Independent, + /// Ordinal-cued instructions ("choose a second player", "choose a third + /// player" — Gluntch, the Bestower) require each successive choice to + /// exclude every player already chosen earlier in this resolution. + DistinctFromPriorChoices, +} + /// What kind of named choice the player must make at resolution time. #[derive(Debug, Clone, PartialEq, Eq)] pub enum ChoiceType { @@ -590,11 +612,22 @@ pub enum ChoiceType { /// the qualifying opponents (CR 608.2d handles ties) — rather than fanning /// the effect out to every tied opponent. Boxed to avoid inflating the /// `ChoiceType` enum with the recursive `PlayerFilter` payload. + /// + /// `distinctness` (CR 608.2c) governs whether this pick must exclude + /// players already chosen by an earlier `Opponent`/`Player` choice in the + /// same resolution. Defaults to `Independent` — see + /// [`PlayerChoiceDistinctness`]. Opponent { restriction: Option>, + distinctness: PlayerChoiceDistinctness, + }, + /// "Choose a player" — selects any player in the game. `distinctness` + /// (CR 608.2c) governs whether this pick must exclude players already + /// chosen by an earlier `Opponent`/`Player` choice in the same + /// resolution — see [`PlayerChoiceDistinctness`]. + Player { + distinctness: PlayerChoiceDistinctness, }, - /// "Choose a player" — selects any player in the game. - Player, /// "Choose two colors" — selects two distinct mana colors. TwoColors, /// "Choose a word" — names any English word (Un-set and silver-border cards). @@ -702,6 +735,39 @@ impl ChoiceType { Self::CardType { excluded } } + /// Unrestricted "choose an opponent" (CR 102.3), independent of any other + /// choice in the resolution (the "Offering" cycle default). + pub fn opponent() -> Self { + Self::Opponent { + restriction: None, + distinctness: PlayerChoiceDistinctness::Independent, + } + } + + /// "Choose an opponent [with the most life ...]" (CR 608.2d). + pub fn opponent_with_restriction(restriction: PlayerFilter) -> Self { + Self::Opponent { + restriction: Some(Box::new(restriction)), + distinctness: PlayerChoiceDistinctness::Independent, + } + } + + /// "Choose a player" (CR 102.1), independent of any other choice in the + /// resolution. + pub fn player() -> Self { + Self::Player { + distinctness: PlayerChoiceDistinctness::Independent, + } + } + + /// Ordinal-cued "choose a second/third player" (Gluntch, the Bestower): + /// must exclude players already chosen earlier in this resolution. + pub fn player_distinct_from_prior() -> Self { + Self::Player { + distinctness: PlayerChoiceDistinctness::DistinctFromPriorChoices, + } + } + pub fn land_or_nonland_card_predicate_options() -> Vec { vec![CardPredicateChoice::Land, CardPredicateChoice::Nonland] } @@ -840,19 +906,47 @@ impl Serialize for ChoiceType { variant.serialize_field("options", options)?; variant.end() } - // Serialize the unrestricted form as the legacy unit variant - // "Opponent" so existing card-data JSON stays byte-stable; only emit - // the struct form when a restriction is present. - Self::Opponent { restriction } => match restriction { - None => serializer.serialize_unit_variant("ChoiceType", 9, "Opponent"), - Some(restriction) => { - let mut variant = - serializer.serialize_struct_variant("ChoiceType", 9, "Opponent", 1)?; + // Serialize the unrestricted, default-distinctness form as the + // legacy unit variant "Opponent" so existing card-data JSON stays + // byte-stable; only emit the struct form when a restriction + // and/or a non-default `distinctness` is present. + Self::Opponent { + restriction, + distinctness, + } => { + let non_default_distinctness = + *distinctness != PlayerChoiceDistinctness::Independent; + if restriction.is_none() && !non_default_distinctness { + serializer.serialize_unit_variant("ChoiceType", 9, "Opponent") + } else { + let field_count = 1 + non_default_distinctness as usize; + let mut variant = serializer.serialize_struct_variant( + "ChoiceType", + 9, + "Opponent", + field_count, + )?; variant.serialize_field("restriction", restriction)?; + if non_default_distinctness { + variant.serialize_field("distinctness", distinctness)?; + } variant.end() } - }, - Self::Player => serializer.serialize_unit_variant("ChoiceType", 10, "Player"), + } + // Serialize the default-distinctness form as the legacy unit + // variant "Player" so existing card-data JSON stays byte-stable; + // only emit the struct form when `distinctness` is non-default + // (Gluntch, the Bestower's ordinal-cued picks). + Self::Player { distinctness } => { + if *distinctness == PlayerChoiceDistinctness::Independent { + serializer.serialize_unit_variant("ChoiceType", 10, "Player") + } else { + let mut variant = + serializer.serialize_struct_variant("ChoiceType", 10, "Player", 1)?; + variant.serialize_field("distinctness", distinctness)?; + variant.end() + } + } Self::TwoColors => serializer.serialize_unit_variant("ChoiceType", 11, "TwoColors"), Self::Word => serializer.serialize_unit_variant("ChoiceType", 12, "Word"), Self::Artist => serializer.serialize_unit_variant("ChoiceType", 13, "Artist"), @@ -929,6 +1023,12 @@ impl<'de> Deserialize<'de> for ChoiceType { Opponent { #[serde(default)] restriction: Option>, + #[serde(default)] + distinctness: PlayerChoiceDistinctness, + }, + Player { + #[serde(default)] + distinctness: PlayerChoiceDistinctness, }, Keyword { options: Vec, @@ -955,8 +1055,8 @@ impl<'de> Deserialize<'de> for ChoiceType { "CardType" => Ok(Self::card_type()), "CardName" => Ok(Self::CardName), "LandType" => Ok(Self::LandType), - "Opponent" => Ok(Self::Opponent { restriction: None }), - "Player" => Ok(Self::Player), + "Opponent" => Ok(Self::opponent()), + "Player" => Ok(Self::player()), "TwoColors" => Ok(Self::TwoColors), "Word" => Ok(Self::Word), "Artist" => Ok(Self::Artist), @@ -996,7 +1096,14 @@ impl<'de> Deserialize<'de> for ChoiceType { ChoiceTypeData::CardPredicateGuess { options } => { Ok(Self::CardPredicateGuess { options }) } - ChoiceTypeData::Opponent { restriction } => Ok(Self::Opponent { restriction }), + ChoiceTypeData::Opponent { + restriction, + distinctness, + } => Ok(Self::Opponent { + restriction, + distinctness, + }), + ChoiceTypeData::Player { distinctness } => Ok(Self::Player { distinctness }), ChoiceTypeData::Keyword { options, count } => Ok(Self::Keyword { options, count }), ChoiceTypeData::CounterKind { options } => Ok(Self::CounterKind { options }), }, @@ -1379,7 +1486,7 @@ impl ChosenAttribute { distinctness: NumberDistinctness::Repeatable, }, // Player covers both Player and Opponent choice types - Self::Player(_) => ChoiceType::Player, + Self::Player(_) => ChoiceType::player(), Self::TwoColors(_) => ChoiceType::TwoColors, // CR 702.104: Tribute outcome uses a dedicated prompt type rather than // a NamedChoice (two fixed labels: Paid / Declined). Classify under the @@ -1514,7 +1621,7 @@ impl ChoiceValue { } ChoiceType::LandType => Some(Self::LandType(value.to_string())), // CR 800.4a: Parse player ID from string. - ChoiceType::Opponent { .. } | ChoiceType::Player => value + ChoiceType::Opponent { .. } | ChoiceType::Player { .. } => value .parse::() .ok() .map(|id| Self::Player(PlayerId(id))), @@ -24223,14 +24330,14 @@ mod tests { // unrestricted form; it must round-trip to `restriction: None`. let choice_type: ChoiceType = serde_json::from_str("\"Opponent\"").unwrap(); - assert_eq!(choice_type, ChoiceType::Opponent { restriction: None }); + assert_eq!(choice_type, ChoiceType::opponent()); } #[test] fn choice_type_opponent_unrestricted_serializes_as_legacy_unit() { // The hand-rolled Serialize must keep emitting the bare string for the // unrestricted form so existing card-data.json stays byte-stable. - let json = serde_json::to_string(&ChoiceType::Opponent { restriction: None }).unwrap(); + let json = serde_json::to_string(&ChoiceType::opponent()).unwrap(); assert_eq!(json, "\"Opponent\""); } @@ -24291,22 +24398,20 @@ mod tests { // The Master, Gallifrey's End: "choose an opponent with the most life". // The hand-rolled Serialize/Deserialize for the restricted struct form // must be symmetric or card-data.json load corrupts silently. - let original = ChoiceType::Opponent { - restriction: Some(Box::new(PlayerFilter::PlayerAttribute { - relation: PlayerRelation::Opponent, - attr: Box::new(QuantityRef::LifeTotal { - player: PlayerScope::ScopedPlayer, - }), - comparator: Comparator::GE, - value: Box::new(QuantityExpr::Ref { - qty: QuantityRef::LifeTotal { - player: PlayerScope::Opponent { - aggregate: AggregateFunction::Max, - }, + let original = ChoiceType::opponent_with_restriction(PlayerFilter::PlayerAttribute { + relation: PlayerRelation::Opponent, + attr: Box::new(QuantityRef::LifeTotal { + player: PlayerScope::ScopedPlayer, + }), + comparator: Comparator::GE, + value: Box::new(QuantityExpr::Ref { + qty: QuantityRef::LifeTotal { + player: PlayerScope::Opponent { + aggregate: AggregateFunction::Max, }, - }), - })), - }; + }, + }), + }); let json = serde_json::to_string(&original).unwrap(); // Restricted form must use the externally-tagged struct variant so it is @@ -24320,6 +24425,34 @@ mod tests { assert_eq!(round_tripped, original); } + #[test] + fn choice_type_player_unrestricted_serializes_as_legacy_unit() { + // The hand-rolled Serialize must keep emitting the bare "Player" string + // for the default-distinctness form so existing card-data.json (Strax, + // Sontaran Nurse) stays byte-stable. + let json = serde_json::to_string(&ChoiceType::player()).unwrap(); + assert_eq!(json, "\"Player\""); + + let round_tripped: ChoiceType = serde_json::from_str(&json).unwrap(); + assert_eq!(round_tripped, ChoiceType::player()); + } + + #[test] + fn choice_type_player_distinct_from_prior_serde_round_trips() { + // Gluntch, the Bestower's ordinal-cued "choose a second/third player" + // must serialize to the struct form carrying the non-default + // `distinctness`, and round-trip back losslessly. + let original = ChoiceType::player_distinct_from_prior(); + let json = serde_json::to_string(&original).unwrap(); + assert!( + json.starts_with(r#"{"Player":"#), + "non-default distinctness should serialize as a struct variant, got: {json}" + ); + + let round_tripped: ChoiceType = serde_json::from_str(&json).unwrap(); + assert_eq!(round_tripped, original); + } + #[test] fn restricted_color_choice_value_rejects_excluded_color() { assert_eq!( diff --git a/crates/engine/tests/integration/baleful_mastery_regression.rs b/crates/engine/tests/integration/baleful_mastery_regression.rs index 89bfdc70a3..1f8ff20dc9 100644 --- a/crates/engine/tests/integration/baleful_mastery_regression.rs +++ b/crates/engine/tests/integration/baleful_mastery_regression.rs @@ -170,7 +170,7 @@ fn baleful_mastery_alternative_cost_makes_chosen_opponent_draw() { options, .. } => { - assert_eq!(choice_type, ChoiceType::Opponent { restriction: None }); + assert_eq!(choice_type, ChoiceType::opponent()); assert_eq!(options, vec![P1.0.to_string()]); runner .act(GameAction::ChooseOption { diff --git a/crates/engine/tests/integration/gollum_scheming_guide_card_predicate_guess.rs b/crates/engine/tests/integration/gollum_scheming_guide_card_predicate_guess.rs index cb3cceb7b3..70c612bf94 100644 --- a/crates/engine/tests/integration/gollum_scheming_guide_card_predicate_guess.rs +++ b/crates/engine/tests/integration/gollum_scheming_guide_card_predicate_guess.rs @@ -63,7 +63,10 @@ fn gollum_attack_trigger_parser_promotes_choices_to_card_predicates() { assert!(matches!( choose_opponent.effect.as_ref(), Effect::Choose { - choice_type: ChoiceType::Opponent { restriction: None }, + choice_type: ChoiceType::Opponent { + restriction: None, + .. + }, persist: false, .. } @@ -278,7 +281,13 @@ fn choose_opponent( }; assert_eq!(player, expected_chooser, "wrong player choosing opponent"); assert!( - matches!(choice_type, ChoiceType::Opponent { restriction: None }), + matches!( + choice_type, + ChoiceType::Opponent { + restriction: None, + .. + } + ), "expected opponent choice, got {choice_type:?}" ); let choice = opponent.0.to_string(); diff --git a/crates/engine/tests/integration/issue_564_wishclaw_talisman_control.rs b/crates/engine/tests/integration/issue_564_wishclaw_talisman_control.rs index 7f3a001bc7..f2004f5bf7 100644 --- a/crates/engine/tests/integration/issue_564_wishclaw_talisman_control.rs +++ b/crates/engine/tests/integration/issue_564_wishclaw_talisman_control.rs @@ -29,7 +29,7 @@ fn choose_opponent(runner: &mut GameRunner, opponent: engine::types::PlayerId) { options, .. } => { - assert_eq!(*choice_type, ChoiceType::Opponent { restriction: None }); + assert_eq!(*choice_type, ChoiceType::opponent()); assert!( options.contains(&opponent.0.to_string()), "opponent must be legal; options={options:?}" diff --git a/crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs b/crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs new file mode 100644 index 0000000000..35cb715289 --- /dev/null +++ b/crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs @@ -0,0 +1,342 @@ +//! Regression (issue #6381): Benevolent Offering's two independent "Choose an +//! opponent." instructions must each accept the SAME opponent in a two-player +//! game. Official ruling: "You may choose the same opponent for each of the +//! effects, or you may choose different opponents." (Confirmed identically +//! for the "Offering" cycle: Infernal/Intellectual/Sylvan Offering.) +//! +//! Before the fix, `ChoiceType::Opponent`/`ChoiceType::Player` unconditionally +//! excluded players already chosen earlier in the same resolution (correct +//! only for Gluntch, the Bestower's ordinal-cued "choose a second/third +//! player"). In a two-player game that made the SECOND "Choose an opponent." +//! impossible — CR 609.3 turned it into a no-op, so "that player" never got +//! bound for the life-gain clause and the chosen opponent gained 0 life +//! instead of 2 life per creature they control. + +use engine::game::scenario::{GameRunner, GameScenario, P0, P1}; +use engine::types::ability::{ChoiceType, ControllerRef, Effect, TargetFilter, TypedFilter}; +use engine::types::game_state::WaitingFor; +use engine::types::mana::{ManaType, ManaUnit}; +use engine::types::phase::Phase; +use engine::types::player::PlayerId; +use engine::types::triggers::TriggerMode; + +const BENEVOLENT_OFFERING: &str = "Choose an opponent. You and that player each create three 1/1 white Spirit \ + creature tokens with flying.\nChoose an opponent. You gain 2 life for each creature you control and that \ + player gains 2 life for each creature they control."; + +fn floating_mana(color: ManaType, n: usize) -> Vec { + (0..n) + .map(|_| { + ManaUnit::new( + color, + engine::types::identifiers::ObjectId(0), + false, + vec![], + ) + }) + .collect() +} + +fn player_life(runner: &GameRunner, player: PlayerId) -> i32 { + runner + .state() + .players + .iter() + .find(|p| p.id == player) + .unwrap() + .life +} + +/// Assert a `NamedChoice(Opponent)` prompt is showing, that `opponent` is +/// among the legal (non-excluded) options, then answer it. +fn choose_opponent(runner: &mut GameRunner, opponent: PlayerId) { + match &runner.state().waiting_for { + WaitingFor::NamedChoice { + choice_type, + options, + .. + } => { + assert!( + matches!( + choice_type, + ChoiceType::Opponent { + restriction: None, + .. + } + ), + "expected an unrestricted opponent choice, got {choice_type:?}" + ); + assert!( + options.contains(&opponent.0.to_string()), + "opponent P{} must remain a legal pick (Offering cycle ruling allows \ + repeating an earlier choice); options={options:?}", + opponent.0 + ); + } + other => panic!("expected NamedChoice(Opponent), got {other:?}"), + } + runner + .act(engine::types::actions::GameAction::ChooseOption { + choice: opponent.0.to_string(), + }) + .expect("ChooseOption(opponent) must succeed"); +} + +#[test] +fn benevolent_offering_allows_choosing_the_same_opponent_twice() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + scenario.with_mana_pool( + P0, + [ + floating_mana(ManaType::Colorless, 3), + floating_mana(ManaType::White, 1), + ] + .concat(), + ); + + let spell = scenario + .add_spell_to_hand_from_oracle(P0, "Benevolent Offering", true, BENEVOLENT_OFFERING) + .id(); + + let mut runner = scenario.build(); + let life_before_p0 = player_life(&runner, P0); + let life_before_p1 = player_life(&runner, P1); + + runner.cast(spell).resolve(); + + // First "Choose an opponent." (fronting the twin token creation). + choose_opponent(&mut runner, P1); + // Second "Choose an opponent." — must offer P1 again, not exclude it. + choose_opponent(&mut runner, P1); + + for _ in 0..8 { + if matches!(runner.state().waiting_for, WaitingFor::Priority { .. }) + && runner.state().stack.is_empty() + { + break; + } + runner + .act(engine::types::actions::GameAction::PassPriority) + .ok(); + } + + // CR 111.7: each player controls exactly three of the created Spirit tokens. + let p0_spirits = runner + .state() + .objects + .values() + .filter(|o| o.controller == P0 && o.name == "Spirit") + .count(); + let p1_spirits = runner + .state() + .objects + .values() + .filter(|o| o.controller == P1 && o.name == "Spirit") + .count(); + assert_eq!(p0_spirits, 3, "the caster must control three Spirit tokens"); + assert_eq!( + p1_spirits, 3, + "the chosen opponent must control three Spirit tokens" + ); + + // CR 119.3: each player gains 2 life per creature they control (their own + // three Spirit tokens). Under the pre-fix bug, P1's gain was 0 because the + // second Choose(Opponent) resolved as an impossible no-op. + assert_eq!( + player_life(&runner, P0) - life_before_p0, + 6, + "the caster must gain 2 life per creature controlled (3 Spirits)" + ); + assert_eq!( + player_life(&runner, P1) - life_before_p1, + 6, + "the chosen opponent must gain 2 life per creature controlled (3 Spirits) \ + — this is the reported defect: it read 0 before the fix" + ); +} + +const INTELLECTUAL_OFFERING: &str = "Choose an opponent. You and that player each draw three cards.\nChoose an \ + opponent. Untap all nonland permanents you control and all nonland permanents that player controls."; + +fn hand_count(runner: &GameRunner, player: PlayerId) -> usize { + runner + .state() + .objects + .values() + .filter(|o| o.owner == player && o.zone == engine::types::zones::Zone::Hand) + .count() +} + +/// Runtime counterpart to `intellectual_offering_second_draw_binds_to_chosen_opponent` +/// below: drives the real cast/resolution pipeline (not just the parsed AST) +/// so the fix is proven all the way through `game/effects/draw.rs`'s +/// `ChosenPlayer` resolution (`game/effects/mod.rs`'s `resolve_player_for_context_ref`), +/// not just the parser. An AST-only assertion would stay green even if the +/// resolver drew for the wrong player. +#[test] +fn intellectual_offering_draws_three_for_caster_and_chosen_opponent() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + scenario.with_mana_pool( + P0, + [ + floating_mana(ManaType::Colorless, 4), + floating_mana(ManaType::Blue, 1), + ] + .concat(), + ); + // Seed both libraries well past the three cards each side draws. + scenario.with_library_top(P0, &["Forest", "Forest", "Forest", "Forest", "Forest"]); + scenario.with_library_top(P1, &["Island", "Island", "Island", "Island", "Island"]); + + let spell = scenario + .add_spell_to_hand_from_oracle(P0, "Intellectual Offering", true, INTELLECTUAL_OFFERING) + .id(); + + let mut runner = scenario.build(); + // The spell itself occupies P0's hand until it resolves off the stack; + // measure the draw delta from the post-cast baseline, not pre-cast. + let hand_before_p1 = hand_count(&runner, P1); + + runner.cast(spell).resolve(); + let hand_before_p0 = hand_count(&runner, P0); + + // First "Choose an opponent." (fronting the twin three-card draw). + choose_opponent(&mut runner, P1); + // Second "Choose an opponent." — must offer P1 again, not exclude it. + choose_opponent(&mut runner, P1); + + for _ in 0..8 { + if matches!(runner.state().waiting_for, WaitingFor::Priority { .. }) + && runner.state().stack.is_empty() + { + break; + } + runner + .act(engine::types::actions::GameAction::PassPriority) + .ok(); + } + + assert_eq!( + hand_count(&runner, P0) - hand_before_p0, + 3, + "the caster must draw three cards" + ); + assert_eq!( + hand_count(&runner, P1) - hand_before_p1, + 3, + "the chosen opponent must draw three cards — this is the runtime proof \ + that ChosenPlayer{{index: 0}} (not the unrelated ScopedPlayer default) \ + resolves the second Draw's recipient" + ); +} + +/// Intellectual Offering shares Benevolent Offering's "Choose an opponent. +/// You and that player each ." shape, so it exercises the SAME +/// `try_parse_compound_subject_each` fix: "that player" must rebind to the +/// resolution-scoped chosen player (`ChosenPlayer { index }`), not the +/// unrelated vote/fan-out `ScopedPlayer` axis. Locks in that the whole +/// "Offering" cycle — not just Benevolent Offering — benefits. +/// +/// AST-shape companion to `intellectual_offering_draws_three_for_caster_and_chosen_opponent` +/// above; kept as a SHAPE test (see the `card-test` skill) because it pins +/// the exact parser output distinct from the runtime draw-count proof. +#[test] +fn intellectual_offering_second_draw_binds_to_chosen_opponent() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + let spell = scenario + .add_spell_to_hand_from_oracle(P0, "Intellectual Offering", true, INTELLECTUAL_OFFERING) + .id(); + let runner = scenario.build(); + let ability = &runner.state().objects.get(&spell).unwrap().abilities[0]; + assert!( + matches!( + ability.effect.as_ref(), + Effect::Choose { + choice_type: ChoiceType::Opponent { .. }, + .. + } + ), + "head must be Choose(Opponent), got {:?}", + ability.effect + ); + let first_draw = ability.sub_ability.as_ref().expect("first Draw node"); + assert!( + matches!( + first_draw.effect.as_ref(), + Effect::Draw { + target: TargetFilter::OriginalController, + .. + } + ), + "the caster's draw must target OriginalController, got {:?}", + first_draw.effect + ); + let second_draw = first_draw.sub_ability.as_ref().expect("second Draw node"); + assert!( + matches!( + second_draw.effect.as_ref(), + Effect::Draw { + target: TargetFilter::Typed(TypedFilter { + controller: Some(ControllerRef::ChosenPlayer { index: 0 }), + .. + }), + .. + } + ), + "the chosen opponent's draw must bind to ChosenPlayer{{index: 0}}, not ScopedPlayer, got {:?}", + second_draw.effect + ); +} + +/// Regression guard for the fix above: `inject_subject_target`'s new +/// `GainLife` arm must NOT rebind the recipient when the detected subject +/// isn't a genuine player reference. Angel of Destiny's "you and that player +/// each gain that much life" is a compound subject that `GainLife` doesn't +/// support in `rewrite_recipient_on_link` (Token/Draw/Discard/Mill/Pump/ +/// GenericEffect only), so it falls through to a non-player-denoting subject +/// filter; the safe no-subject `Controller` default must survive rather than +/// being corrupted into an incoherent recipient. This is a pre-existing gap +/// (the damaged player still doesn't gain life) — not fixed here — but the +/// `player` field must stay a well-defined `Controller`, not silently swap to +/// something meaningless. +#[test] +fn angel_of_destiny_combat_damage_gain_life_keeps_well_defined_recipient() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + let creature = scenario + .add_creature_from_oracle( + P0, + "Angel of Destiny", + 3, + 4, + "Flying, double strike\nWhenever a creature you control deals combat damage to a player, you and that player each gain that much life.\nAt the beginning of your end step, if you have at least 15 life more than your starting life total, each player this creature attacked this turn loses the game.", + ) + .id(); + let runner = scenario.build(); + let obj = runner.state().objects.get(&creature).unwrap(); + let damage_trigger = obj + .trigger_definitions + .iter_unchecked() + .find(|entry| matches!(entry.definition.mode, TriggerMode::DamageDone)) + .expect("Angel of Destiny must have a DamageDone trigger"); + let execute = damage_trigger + .definition + .execute + .as_ref() + .expect("DamageDone trigger must have an execute body"); + assert!( + matches!( + execute.effect.as_ref(), + Effect::GainLife { + player: TargetFilter::Controller, + .. + } + ), + "GainLife.player must stay the well-defined Controller default, not an \ + unresolved compound-subject filter like Any, got {:?}", + execute.effect + ); +} diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index cee9f2fbb6..0b221f2498 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -613,6 +613,7 @@ mod issue_6092_ability_block_reason; mod issue_6102_ragavan_exile_cast; mod issue_6157_gold_token_auto_mana_payment; mod issue_629_fractured_sanity_cycling; +mod issue_6381_benevolent_offering_repeat_opponent; mod issue_6403_moonmist_mass_transform; mod issue_6405_aang_multicolor_cost_reduction; mod issue_6416_extra_turn_resume_order; diff --git a/crates/engine/tests/integration/rules/tribute.rs b/crates/engine/tests/integration/rules/tribute.rs index 1c12efc64a..2c30e4628f 100644 --- a/crates/engine/tests/integration/rules/tribute.rs +++ b/crates/engine/tests/integration/rules/tribute.rs @@ -64,7 +64,7 @@ fn cast_tribute_creature(count: u32, paid: bool) -> GameRunner { .. } => { assert_eq!(*player, P0, "controller should be choosing the opponent"); - assert_eq!(*choice_type, ChoiceType::Opponent { restriction: None }); + assert_eq!(*choice_type, ChoiceType::opponent()); assert!( options.contains(&P1.0.to_string()), "P1 must be a valid opponent choice, got {options:?}" diff --git a/crates/engine/tests/integration/the_who_opponent_guess_resolution.rs b/crates/engine/tests/integration/the_who_opponent_guess_resolution.rs index 2fed2bc254..f4560960ad 100644 --- a/crates/engine/tests/integration/the_who_opponent_guess_resolution.rs +++ b/crates/engine/tests/integration/the_who_opponent_guess_resolution.rs @@ -126,7 +126,13 @@ fn choose_guessing_opponent(runner: &mut GameRunner, opponent: PlayerId) { "the controller chooses which opponent makes the guess" ); assert!( - matches!(choice_type, ChoiceType::Opponent { restriction: None }), + matches!( + choice_type, + ChoiceType::Opponent { + restriction: None, + .. + } + ), "expected opponent choice before the guess, got {choice_type:?}" ); assert!( diff --git a/crates/mtgish-import/src/convert/action.rs b/crates/mtgish-import/src/convert/action.rs index d4cdb8f421..edec390cce 100644 --- a/crates/mtgish-import/src/convert/action.rs +++ b/crates/mtgish-import/src/convert/action.rs @@ -4441,8 +4441,8 @@ pub fn convert(a: &Action) -> ConvResult { // existing `players_to_controller` bridge for opponent detection. Action::ChooseAPlayer(players) => { let choice_type = match filter_mod::players_to_controller(players.as_ref()) { - Ok(ControllerRef::Opponent) => ChoiceType::Opponent { restriction: None }, - _ => ChoiceType::Player, + Ok(ControllerRef::Opponent) => ChoiceType::opponent(), + _ => ChoiceType::player(), }; Effect::Choose { choice_type, diff --git a/crates/mtgish-import/src/convert/replacement.rs b/crates/mtgish-import/src/convert/replacement.rs index 1b9a2f6319..a11778a778 100644 --- a/crates/mtgish-import/src/convert/replacement.rs +++ b/crates/mtgish-import/src/convert/replacement.rs @@ -1864,15 +1864,15 @@ fn build_replacement_exec( persist: true, selection: engine::types::ability::TargetSelectionMode::Chosen, }, - // CR 800.4a: opponent-scoped player choice when the schema - // filter narrows to opponents; broader player choice + // CR 102.1-102.3 + CR 608.2d: opponent-scoped player choice when the + // schema filter narrows to opponents; broader player choice // otherwise. Re-uses the existing `players_to_controller` // bridge for opponent detection. A::ChooseAPlayer(players) => { let choice_type = match crate::convert::filter::players_to_controller(players.as_ref()) { - Ok(ControllerRef::Opponent) => ChoiceType::Opponent { restriction: None }, - _ => ChoiceType::Player, + Ok(ControllerRef::Opponent) => ChoiceType::opponent(), + _ => ChoiceType::player(), }; Effect::Choose { choice_type,