diff --git a/crates/engine/src/ai_support/shortcut_efficacy.rs b/crates/engine/src/ai_support/shortcut_efficacy.rs index f7a6caff94..40faa2a91b 100644 --- a/crates/engine/src/ai_support/shortcut_efficacy.rs +++ b/crates/engine/src/ai_support/shortcut_efficacy.rs @@ -625,6 +625,7 @@ fn ability_window_reach(def: &AbilityDefinition) -> WindowReach { repeat_for, announced_x, repeat_until, + optional_player, optional_for, iteration_kind_binding, // ---- read-free ---- @@ -691,6 +692,7 @@ fn ability_window_reach(def: &AbilityDefinition) -> WindowReach { || repeat_for.is_some() || announced_x.is_some() || repeat_until.is_some() + || optional_player.is_some() || optional_for.is_some() || iteration_kind_binding.is_some(); acc.or(WindowReach::of(!conservative_when_present)) diff --git a/crates/engine/src/game/ability_rw.rs b/crates/engine/src/game/ability_rw.rs index 4bc7653a5d..20e03d1221 100644 --- a/crates/engine/src/game/ability_rw.rs +++ b/crates/engine/src/game/ability_rw.rs @@ -3753,6 +3753,7 @@ fn walk_ability( context: _, optional_targeting: _, optional: _, + optional_player, optional_for: _, target_choice_timing: _, description: _, @@ -3832,6 +3833,9 @@ fn walk_ability( if let Some(tc) = target_chooser { acc.merge(rw_target_filter(tc)); } + if let Some(player) = optional_player { + acc.merge(rw_target_filter(player)); + } if let Some(ru) = repeat_until { acc.merge(rw_repeat_continuation(ru)); } @@ -3886,6 +3890,7 @@ fn walk_definition( ability_tag: _, optional_targeting: _, optional: _, + optional_player, optional_for: _, target_choice_timing: _, distribute: _, @@ -3951,6 +3956,9 @@ fn walk_definition( if let Some(tc) = target_chooser { acc.merge(rw_target_filter(tc)); } + if let Some(player) = optional_player { + acc.merge(rw_target_filter(player)); + } if let Some(ru) = repeat_until { acc.merge(rw_repeat_continuation(ru)); } diff --git a/crates/engine/src/game/ability_scan.rs b/crates/engine/src/game/ability_scan.rs index 3a32bab3d8..fbff5eccad 100644 --- a/crates/engine/src/game/ability_scan.rs +++ b/crates/engine/src/game/ability_scan.rs @@ -242,27 +242,28 @@ fn resolved_ability_axes(a: &ResolvedAbility, mode: ScanMode) -> Axes { context: _, // SpellContext: cast-time fact snapshot, not a live read optional_targeting: _, // bool optional: _, // bool - optional_for: _, // OpponentMayScope: AnyOpponent/AnyPlayer, no read - target_choice_timing: _, // Stack/Resolution tag - description: _, // display string - selected_mode_labels: _, // display strings, no dynamic read - min_x_value: _, // u32 - cant_be_copied: _, // bool - copy_count_status: _, // status tag - forward_result: _, // bool - distribution: _, // concrete pre-assigned (TargetRef, u32) portions - chosen_x: _, // concrete cast-time X - cost_paid_object: _, // concrete captured-object snapshot - cost_paid_object_ids: _, // concrete captured-object ids (issue #4948) - effect_context_object: _, // concrete captured-object snapshot - amassed_army_object: _, // concrete captured-object snapshot - ability_index: _, // usize provenance - may_trigger_origin: _, // provenance tag - target_selection_mode: _, // Chosen/Random tag - chosen_players: _, // concrete chosen player ids - replacement_applied: _, // replacement provenance set, no dynamic read - sub_link: _, // SubAbilityLink kind tag - sibling_condition: _, // SiblingCondition replication marker, no dynamic read + optional_player, + optional_for: _, // OpponentMayScope: AnyOpponent/AnyPlayer, no read + target_choice_timing: _, // Stack/Resolution tag + description: _, // display string + selected_mode_labels: _, // display strings, no dynamic read + min_x_value: _, // u32 + cant_be_copied: _, // bool + copy_count_status: _, // status tag + forward_result: _, // bool + distribution: _, // concrete pre-assigned (TargetRef, u32) portions + chosen_x: _, // concrete cast-time X + cost_paid_object: _, // concrete captured-object snapshot + cost_paid_object_ids: _, // concrete captured-object ids (issue #4948) + effect_context_object: _, // concrete captured-object snapshot + amassed_army_object: _, // concrete captured-object snapshot + ability_index: _, // usize provenance + may_trigger_origin: _, // provenance tag + target_selection_mode: _, // Chosen/Random tag + chosen_players: _, // concrete chosen player ids + replacement_applied: _, // replacement provenance set, no dynamic read + sub_link: _, // SubAbilityLink kind tag + sibling_condition: _, // SiblingCondition replication marker, no dynamic read parent_target_missing_reason: _, // seam flag } = a; @@ -324,6 +325,13 @@ fn resolved_ability_axes(a: &ResolvedAbility, mode: ScanMode) -> Axes { mode, )); } + if let Some(player) = optional_player { + acc = acc.or(scan_target_filter( + player, + FilterReadContext::SnapshotOrEvent, + mode, + )); + } // CR 608.2c / CR 107.1c: a "repeat this process while " predicate is // re-evaluated against freshly-resolved state each iteration — a resolution read. if let Some(repeat_until) = repeat_until { @@ -4353,6 +4361,7 @@ fn ability_definition_axes(def: &AbilityDefinition, mode: ScanMode) -> Axes { ability_tag: _, optional_targeting: _, optional: _, + optional_player, optional_for: _, target_choice_timing: _, min_x_value: _, @@ -4413,6 +4422,13 @@ fn ability_definition_axes(def: &AbilityDefinition, mode: ScanMode) -> Axes { mode, )); } + if let Some(player) = optional_player { + acc = acc.or(scan_target_filter( + player, + FilterReadContext::SnapshotOrEvent, + mode, + )); + } if let Some(ru) = repeat_until { acc = acc.or(scan_repeat_continuation(ru, mode)); } diff --git a/crates/engine/src/game/ability_utils.rs b/crates/engine/src/game/ability_utils.rs index 8c686b9d40..89f272b282 100644 --- a/crates/engine/src/game/ability_utils.rs +++ b/crates/engine/src/game/ability_utils.rs @@ -119,6 +119,7 @@ pub fn build_resolved_from_def_with_targets( } resolved.optional_targeting = def.optional_targeting; resolved.optional = def.optional; + resolved.optional_player = def.optional_player.clone(); resolved.optional_for = def.optional_for; resolved.multi_target = def.multi_target.clone(); // CR 115.1 + CR 601.2c: Carry the target-set constraints (e.g. combined diff --git a/crates/engine/src/game/effects/additional_phase.rs b/crates/engine/src/game/effects/additional_phase.rs index db6c404301..b5a2554305 100644 --- a/crates/engine/src/game/effects/additional_phase.rs +++ b/crates/engine/src/game/effects/additional_phase.rs @@ -282,6 +282,7 @@ mod tests { context: SpellContext::default(), optional_targeting: false, optional: false, + optional_player: None, optional_for: None, multi_target: None, target_constraints: Vec::new(), diff --git a/crates/engine/src/game/effects/double.rs b/crates/engine/src/game/effects/double.rs index 149061ef06..0c7d856f1c 100644 --- a/crates/engine/src/game/effects/double.rs +++ b/crates/engine/src/game/effects/double.rs @@ -352,6 +352,7 @@ mod tests { may_trigger_origin: None, optional_targeting: false, optional: false, + optional_player: None, optional_for: None, multi_target: None, target_constraints: Vec::new(), diff --git a/crates/engine/src/game/effects/extra_turn.rs b/crates/engine/src/game/effects/extra_turn.rs index be995ef991..2de6a6c7d8 100644 --- a/crates/engine/src/game/effects/extra_turn.rs +++ b/crates/engine/src/game/effects/extra_turn.rs @@ -87,6 +87,7 @@ mod tests { context: SpellContext::default(), optional_targeting: false, optional: false, + optional_player: None, optional_for: None, multi_target: None, target_constraints: Vec::new(), diff --git a/crates/engine/src/game/effects/grant_extra_loyalty_activations.rs b/crates/engine/src/game/effects/grant_extra_loyalty_activations.rs index 1dc37343fd..7f8542b864 100644 --- a/crates/engine/src/game/effects/grant_extra_loyalty_activations.rs +++ b/crates/engine/src/game/effects/grant_extra_loyalty_activations.rs @@ -105,6 +105,7 @@ mod tests { context: SpellContext::default(), optional_targeting: false, optional: false, + optional_player: None, optional_for: None, multi_target: None, target_constraints: Vec::new(), diff --git a/crates/engine/src/game/effects/mod.rs b/crates/engine/src/game/effects/mod.rs index 82b2d0d8a1..82d464bedf 100644 --- a/crates/engine/src/game/effects/mod.rs +++ b/crates/engine/src/game/effects/mod.rs @@ -7123,6 +7123,16 @@ pub(crate) fn optional_prompt_player(state: &GameState, ability: &ResolvedAbilit return player; } } + // CR 608.2d: a parser-stamped subject such as "they may" names the player + // who receives this choice. The reference resolves from the trigger event, + // preserving the event-time controller rather than inferring from effect shape. + if let Some(optional_player) = &ability.optional_player { + if let Some(player) = + crate::game::targeting::resolve_effect_player_ref(state, ability, optional_player) + { + return player; + } + } if let Effect::Sacrifice { target, .. } = &ability.effect { if target_filter_controller_scope(target) == Some(ControllerRef::ParentTargetController) { if let Some(player) = crate::game::targeting::resolve_effect_player_ref( diff --git a/crates/engine/src/game/effects/player_counter.rs b/crates/engine/src/game/effects/player_counter.rs index b564934717..26af405939 100644 --- a/crates/engine/src/game/effects/player_counter.rs +++ b/crates/engine/src/game/effects/player_counter.rs @@ -455,6 +455,7 @@ mod tests { context: SpellContext::default(), optional_targeting: false, optional: false, + optional_player: None, optional_for: None, multi_target: None, target_constraints: Vec::new(), @@ -652,6 +653,7 @@ mod tests { context: SpellContext::default(), optional_targeting: false, optional: false, + optional_player: None, optional_for: None, multi_target: None, target_constraints: Vec::new(), diff --git a/crates/engine/src/game/effects/reverse_turn_order.rs b/crates/engine/src/game/effects/reverse_turn_order.rs index b851afa6d9..48e429ce4c 100644 --- a/crates/engine/src/game/effects/reverse_turn_order.rs +++ b/crates/engine/src/game/effects/reverse_turn_order.rs @@ -60,6 +60,7 @@ mod tests { context: SpellContext::default(), optional_targeting: false, optional: false, + optional_player: None, optional_for: None, multi_target: None, target_constraints: Vec::new(), diff --git a/crates/engine/src/game/effects/skip_next_step.rs b/crates/engine/src/game/effects/skip_next_step.rs index 37bfb8c4ac..33549edca1 100644 --- a/crates/engine/src/game/effects/skip_next_step.rs +++ b/crates/engine/src/game/effects/skip_next_step.rs @@ -123,6 +123,7 @@ mod tests { context: SpellContext::default(), optional_targeting: false, optional: false, + optional_player: None, optional_for: None, multi_target: None, target_constraints: Vec::new(), diff --git a/crates/engine/src/game/effects/skip_next_turn.rs b/crates/engine/src/game/effects/skip_next_turn.rs index 0f6d74987a..608b7c2caf 100644 --- a/crates/engine/src/game/effects/skip_next_turn.rs +++ b/crates/engine/src/game/effects/skip_next_turn.rs @@ -101,6 +101,7 @@ mod tests { context: SpellContext::default(), optional_targeting: false, optional: false, + optional_player: None, optional_for: None, multi_target: None, target_constraints: Vec::new(), diff --git a/crates/engine/src/game/effects/vote.rs b/crates/engine/src/game/effects/vote.rs index 78d00004d8..a5457009f7 100644 --- a/crates/engine/src/game/effects/vote.rs +++ b/crates/engine/src/game/effects/vote.rs @@ -373,6 +373,7 @@ pub fn resolve_tally( context: Default::default(), optional_targeting: per_choice_effect[idx].optional_targeting, optional: per_choice_effect[idx].optional, + optional_player: per_choice_effect[idx].optional_player.clone(), optional_for: None, multi_target: None, target_constraints: Vec::new(), @@ -443,6 +444,7 @@ pub fn resolve_tally( context: Default::default(), optional_targeting: per_choice_effect[idx].optional_targeting, optional: per_choice_effect[idx].optional, + optional_player: per_choice_effect[idx].optional_player.clone(), optional_for: None, multi_target: None, target_constraints: Vec::new(), @@ -687,6 +689,7 @@ fn resolved_from_def( context: Default::default(), optional_targeting: def.optional_targeting, optional: def.optional, + optional_player: def.optional_player.clone(), optional_for: None, multi_target: None, target_constraints: Vec::new(), @@ -949,6 +952,7 @@ mod tests { context: Default::default(), optional_targeting: false, optional: false, + optional_player: None, optional_for: None, multi_target: None, target_constraints: Vec::new(), @@ -1060,6 +1064,7 @@ mod tests { context: Default::default(), optional_targeting: false, optional: false, + optional_player: None, optional_for: None, multi_target: None, target_constraints: Vec::new(), @@ -1496,6 +1501,7 @@ mod tests { context: Default::default(), optional_targeting: false, optional: false, + optional_player: None, optional_for: None, multi_target: None, target_constraints: Vec::new(), @@ -1664,6 +1670,7 @@ mod tests { context: Default::default(), optional_targeting: false, optional: false, + optional_player: None, optional_for: None, multi_target: None, target_constraints: Vec::new(), diff --git a/crates/engine/src/game/engine.rs b/crates/engine/src/game/engine.rs index 765dcc7a9d..056adb4ca2 100644 --- a/crates/engine/src/game/engine.rs +++ b/crates/engine/src/game/engine.rs @@ -16115,7 +16115,7 @@ mod stage2_injector_tests { // in the merged source, still in their named production functions. "game/effects/mod.rs:6640".to_string(), "game/effects/mod.rs:6717".to_string(), - "game/effects/mod.rs:9922".to_string(), + "game/effects/mod.rs:9932".to_string(), // UNMOVED across the rebase, and that is itself evidence the SET did not // move: a census that had gained or lost a producer would not leave this // entry both byte-identical AND at the same coordinate. diff --git a/crates/engine/src/game/resolution_prompt.rs b/crates/engine/src/game/resolution_prompt.rs index a995e32e86..96449f475c 100644 --- a/crates/engine/src/game/resolution_prompt.rs +++ b/crates/engine/src/game/resolution_prompt.rs @@ -529,6 +529,7 @@ pub(crate) fn chain_offers_choice(a: &ResolvedAbility) -> bool { sub_ability, else_ability, optional, + optional_player: _, // selects the optional actor; `optional` already records the choice optional_for, optional_targeting, unless_pay, diff --git a/crates/engine/src/game/stack.rs b/crates/engine/src/game/stack.rs index ef74710c04..2cb2a5136a 100644 --- a/crates/engine/src/game/stack.rs +++ b/crates/engine/src/game/stack.rs @@ -3084,6 +3084,7 @@ fn self_counter_ability_is_batch_candidate(ability: &ResolvedAbility) -> bool { context, optional_targeting, optional, + optional_player, optional_for, multi_target, target_constraints, @@ -3144,6 +3145,7 @@ fn self_counter_ability_is_batch_candidate(ability: &ResolvedAbility) -> bool { && *context == SpellContext::default() && !*optional_targeting && !*optional + && optional_player.is_none() && optional_for.is_none() && multi_target.is_none() && target_constraints.is_empty() @@ -3297,6 +3299,7 @@ fn fixed_controller_gain_life_ability_is_batch_candidate(ability: &ResolvedAbili context, optional_targeting, optional, + optional_player, optional_for, multi_target, target_constraints, @@ -3352,6 +3355,7 @@ fn fixed_controller_gain_life_ability_is_batch_candidate(ability: &ResolvedAbili && *context == SpellContext::default() && !*optional_targeting && !*optional + && optional_player.is_none() && optional_for.is_none() && multi_target.is_none() && target_constraints.is_empty() @@ -3490,6 +3494,7 @@ fn fixed_opponent_lose_life_ability_is_batch_candidate(ability: &ResolvedAbility context, optional_targeting, optional, + optional_player, optional_for, multi_target, target_constraints, @@ -3545,6 +3550,7 @@ fn fixed_opponent_lose_life_ability_is_batch_candidate(ability: &ResolvedAbility && *context == SpellContext::default() && !*optional_targeting && !*optional + && optional_player.is_none() && optional_for.is_none() && multi_target.is_none() && target_constraints.is_empty() @@ -4130,6 +4136,7 @@ fn inert_trigger_abilities_eq_ignoring_provenance( context: a_context, optional_targeting: a_optional_targeting, optional: a_optional, + optional_player: a_optional_player, optional_for: a_optional_for, multi_target: a_multi_target, target_constraints: a_target_constraints, @@ -4186,6 +4193,7 @@ fn inert_trigger_abilities_eq_ignoring_provenance( context: b_context, optional_targeting: b_optional_targeting, optional: b_optional, + optional_player: b_optional_player, optional_for: b_optional_for, multi_target: b_multi_target, target_constraints: b_target_constraints, @@ -4254,6 +4262,7 @@ fn inert_trigger_abilities_eq_ignoring_provenance( && a_context == b_context && a_optional_targeting == b_optional_targeting && a_optional == b_optional + && a_optional_player == b_optional_player && a_optional_for == b_optional_for && a_multi_target == b_multi_target && a_target_constraints == b_target_constraints diff --git a/crates/engine/src/parser/oracle_ir/trigger.rs b/crates/engine/src/parser/oracle_ir/trigger.rs index 1dadec2617..cd1d8ce3e6 100644 --- a/crates/engine/src/parser/oracle_ir/trigger.rs +++ b/crates/engine/src/parser/oracle_ir/trigger.rs @@ -291,6 +291,10 @@ pub(crate) struct TriggerModifiers { /// CR 603.5: Some triggered abilities' effects are optional (they contain /// "may"). They go on the stack regardless; the choice is made on resolution. pub(crate) optional: bool, + /// CR 608.2d: Event-relative player explicitly named by the root optional + /// subject, after any intervening-if wrapper has been removed. + #[serde(skip_serializing_if = "Option::is_none")] + pub(crate) optional_player: Option, /// CR 118.12: "unless [player] pays {cost}" tax modifier. pub(crate) unless_pay: Option, /// Intervening-if condition extracted from effect text. diff --git a/crates/engine/src/parser/oracle_trigger.rs b/crates/engine/src/parser/oracle_trigger.rs index 22c4509637..04e4d5e9b2 100644 --- a/crates/engine/src/parser/oracle_trigger.rs +++ b/crates/engine/src/parser/oracle_trigger.rs @@ -51,12 +51,13 @@ use crate::types::ability::{ AdditionalCostOrigin, AdditionalCostPaymentSource, AggregateFunction, AttachmentKind, AttackersDeclaredCountSubject, CastManaObjectScope, CastManaSpentMetric, CastVariantPaid, CoinFlipResult, Comparator, ControllerRef, CountScope, CounterTriggerFilter, DamageKindFilter, - DestinationConstraint, DieResultFilter, Effect, FilterProp, ManaAbilityProducedFilter, - ObjectScope, OriginConstraint, ParsedCondition, PlayerFilter, PlayerScope, PtStat, - PtValueScope, QuantityExpr, QuantityRef, RenownSubject, SacrificeAggregateStat, SacrificeCost, - SacrificeRequirement, SharedQuality, StaticCondition, SubAbilityLink, TapCreaturesRequirement, - TargetFilter, TriggerCondition, TriggerConstraint, TriggerDefinition, TypeFilter, TypedFilter, - UnlessPayModifier, ZoneChangeClause, + DestinationConstraint, DieResultFilter, Effect, EffectScope, FilterProp, + ManaAbilityProducedFilter, ObjectScope, OriginConstraint, ParsedCondition, PlayerFilter, + PlayerScope, PtStat, PtValueScope, QuantityExpr, QuantityRef, RenownSubject, + SacrificeAggregateStat, SacrificeCost, SacrificeRequirement, SharedQuality, StaticCondition, + SubAbilityLink, TapCreaturesRequirement, TapStateChange, TargetFilter, TriggerCondition, + TriggerConstraint, TriggerDefinition, TypeFilter, TypedFilter, UnlessPayModifier, + ZoneChangeClause, }; use crate::types::card_type::{is_land_subtype, CoreType}; use crate::types::counter::CounterType; @@ -296,6 +297,16 @@ fn effect_adds_mana_to_triggering_player(effect_lower: &str) -> bool { .is_ok() } +/// CR 608.2d + CR 603.2: A leading "they may" in a normalized trigger body +/// names the player recorded by that trigger event, rather than the ability's +/// controller. Call after stripping an intervening-if wrapper so the actor is +/// retained for both direct and conditional root modals. +fn optional_player_from_effect_body(effect_text: &str) -> Option { + let lower = effect_text.to_lowercase(); + let parsed = tag::<_, _, OracleError<'_>>("they may ").parse(lower.trim_start()); + parsed.ok().map(|_| TargetFilter::TriggeringPlayer) +} + /// CR 113.6 + CR 113.6b: Collect every zone the trigger's /// source must occupy for the condition to be satisfiable. Returns the /// deduplicated union of `SourceInZone { zone }` references across @@ -1353,6 +1364,10 @@ pub(crate) fn parse_trigger_line_with_index_ir( let cond_lower = condition_text.to_lowercase(); let effect_lower = effect_text.to_lowercase(); + let after_structural_if = effect_lower + .strip_prefix("if ") // allow-noncombinator: structural if-clause skip when condition is unrecognized + .and_then(|rest| rest.split_once(", ")) + .map(|(_cond, body)| body); // CR 701.42b: A meld instigator's effect text opens with the own/control // gate ("if you both own and control ~ and a [type] named [partner], exile // them, then meld them into [result]"). Recognize it as a unit: the gate @@ -1376,6 +1391,8 @@ pub(crate) fn parse_trigger_line_with_index_ir( (without_if, cond, None) } }; + let optional_player = optional_player_from_effect_body(&effect_without_if) + .or_else(|| after_structural_if.and_then(optional_player_from_effect_body)); // CR 608.2c (resolution-order instructions): "You may" at the start of // the effect text makes the triggered effect optional at resolution. @@ -1394,11 +1411,8 @@ pub(crate) fn parse_trigger_line_with_index_ir( // The detection below only fires when the `you may` is the FIRST token // (modulo an intervening-if), which excludes the multi-sentence case. let starts_with_you_may = |s: &str| tag::<_, _, OracleError<'_>>("you may ").parse(s).is_ok(); - let after_structural_if = effect_lower - .strip_prefix("if ") // allow-noncombinator: structural if-clause skip when condition is unrecognized - .and_then(|rest| rest.split_once(", ")) - .map(|(_cond, body)| body); - let mut optional = starts_with_you_may(effect_lower.as_str()) + let mut optional = optional_player.is_some() + || starts_with_you_may(effect_lower.as_str()) || starts_with_you_may(effect_without_if.trim_start()) || after_structural_if.is_some_and(starts_with_you_may); @@ -1606,6 +1620,7 @@ pub(crate) fn parse_trigger_line_with_index_ir( body, modifiers: TriggerModifiers { optional, + optional_player, unless_pay, intervening_if: if_condition, trigger_subject, @@ -1805,6 +1820,11 @@ pub(crate) fn lower_trigger_ir(ir: &TriggerIr) -> TriggerDefinition { // quantities to `PlayerScope::ScopedPlayer` so they resolve against the // damaged/attacked player rather than an absent chosen target. let mut execute = execute; + if let Some(optional_player) = &modifiers.optional_player { + if let Some(ability) = execute.as_deref_mut() { + ability.optional_player = Some(optional_player.clone()); + } + } // CR 603.2c: A `TrackedSetAggregate { source: TriggeringBatch }` reduces the // objects of THIS trigger's event, read back through // `extract_sources_from_event`. That only yields anything for the events that @@ -2123,7 +2143,7 @@ pub(crate) fn lower_trigger_ir(ir: &TriggerIr) -> TriggerDefinition { } } - // CR 608.2k + CR 603.7c: For event-source-bearing trigger modes, the "that + // CR 603.2 + CR 603.6 + CR 608.2k: For event-source-bearing trigger modes, the "that // card / that creature / that permanent" anaphor in the effect body // refers to the *triggering object* carried by the event (the just- // discarded card, sacrificed permanent, drawn card, etc.) — not a chosen @@ -2225,7 +2245,7 @@ fn valid_target_blocks_event_source_lift( /// TargetFilter` and whose runtime semantics make sense against the event /// object (e.g. `ChangeZone` operating on the just-discarded card). Other /// effect variants are left untouched. -fn lift_parent_target_to_triggering_source(effect: &mut Effect) { +fn lift_parent_target_to_triggering_source(effect: &mut Effect, allow_set_tap_lift: bool) { // CR 608.2k: each variant carries a top-level `target` that, when the // surface anaphor was "that ", refers to the event object. let target = match effect { @@ -2234,6 +2254,15 @@ fn lift_parent_target_to_triggering_source(effect: &mut Effect) { // "create a token that's a copy of that creature" (Necroduality) — the // copy source is the entering object, not the trigger's own source. Effect::CopyTokenOf { target, .. } => target, + // CR 608.2k + CR 701.26a: on a single-object zone-change trigger, + // "they may tap that permanent" refers to the entering object. The + // caller limits this to the trigger's top-level, untargeted Tap effect; + // a reflexive or targeted tap has its own chosen referent instead. + Effect::SetTapState { + target, + scope: EffectScope::Single, + state: TapStateChange::Tap, + } if allow_set_tap_lift => target, _ => return, }; if matches!(target, TargetFilter::ParentTarget) { @@ -2260,7 +2289,8 @@ fn first_independent_sibling_after_search( None } -/// CR 608.2k + CR 603.7c: Recurse `lift_parent_target_to_triggering_source` +/// CR 603.2 + CR 603.6 + CR 608.2k: Recurse +/// `lift_parent_target_to_triggering_source` /// through an ability's effect AND every chained `sub_ability`. Required /// for the punisher-trigger class: a chained Tergrid-shape ability like /// "...exile that card, then create a token" carries the "that card" @@ -2268,6 +2298,14 @@ fn first_independent_sibling_after_search( /// Without the descent, the second link would silently bind to the trigger /// source object instead of the just-acted-on event object. fn lift_parent_target_to_triggering_source_in_ability(ability: &mut AbilityDefinition) { + // CR 608.2c + CR 608.2k: An inline modal stores each mode outside the + // ordinary sub-ability chain. Each mode is nevertheless a root instruction + // of this event-source trigger, so it needs the same narrow rewrite before + // the modal choice selects one; a chosen target inside a mode remains + // protected by this walk's existing boundary. + for mode in &mut ability.mode_abilities { + lift_parent_target_to_triggering_source_in_ability(mode); + } // CR 608.2c + CR 608.2k: Stop the descent as soon as a link introduces a // player-*chosen* object target. A later `ParentTarget` then refers to // *that* choice, not the trigger event — the enters-flicker class @@ -2276,6 +2314,7 @@ fn lift_parent_target_to_triggering_source_in_ability(ability: &mut AbilityDefin // Necroduality (top-level `CopyTokenOf` with no prior choice) and Tergrid // ("put that card …, then create a token") still lift correctly. let mut node = Some(ability); + let mut is_top_level = true; while let Some(link) = node { // CR 701.23a: A library search's continuation receives the found cards // as its parent targets. In an event-source-bearing trigger, the @@ -2294,8 +2333,11 @@ fn lift_parent_target_to_triggering_source_in_ability(ability: &mut AbilityDefin if introduces_chosen_object_target(link.effect.as_ref()) { break; } - lift_parent_target_to_triggering_source(link.effect.as_mut()); + let allow_set_tap_lift = + is_top_level && link.multi_target.is_none() && !link.optional_targeting; + lift_parent_target_to_triggering_source(link.effect.as_mut(), allow_set_tap_lift); node = link.sub_ability.as_deref_mut(); + is_top_level = false; } } diff --git a/crates/engine/src/parser/oracle_trigger_tests.rs b/crates/engine/src/parser/oracle_trigger_tests.rs index 1e48219cf4..e391b32196 100644 --- a/crates/engine/src/parser/oracle_trigger_tests.rs +++ b/crates/engine/src/parser/oracle_trigger_tests.rs @@ -11,10 +11,10 @@ use crate::types::ability::{ Comparator, ContinuousModification, ControllerRef, CopyChooseScope, CopyRetargetPermission, CountScope, DamageChannel, DamageModification, DamageSource, DelayedTriggerCondition, DiscardSelfScope, Duration, Effect, EffectScope, FilterProp, ManaContribution, ManaProduction, - ManaSpendPermission, ObjectScope, PerpetualModification, PlayerFilter, PlayerScope, PtStat, - PtValue, PtValueScope, QuantityExpr, QuantityRef, SeatDirection, SharedQuality, - SiblingCondition, SubAbilityLink, TapStateChange, TargetFilter, TriggerCondition, TypeFilter, - TypedFilter, ZoneRef, + ManaSpendPermission, ModalChoice, ObjectScope, PerpetualModification, PlayerFilter, + PlayerScope, PtStat, PtValue, PtValueScope, QuantityExpr, QuantityRef, SeatDirection, + SharedQuality, SiblingCondition, SubAbilityLink, TapStateChange, TargetFilter, + TriggerCondition, TypeFilter, TypedFilter, ZoneRef, }; use crate::types::card_type::Supertype; use crate::types::counter::{CounterMatch, CounterType}; @@ -2472,6 +2472,100 @@ fn trigger_etb_subject_enters_untapped_attaches_negated_condition() { condition: Box::new(TriggerCondition::ZoneChangeObjectIsTapped) }) ); + let execute = def.execute.as_deref().expect("Charismatic execute ability"); + assert!(execute.optional, "they may tap must remain optional"); + assert_eq!( + execute.optional_player, + Some(TargetFilter::TriggeringPlayer), + "the parsed `they` subject, not the tap shape, names the optional actor" + ); + assert!(matches!( + execute.effect.as_ref(), + Effect::SetTapState { + target: TargetFilter::TriggeringSource, + scope: EffectScope::Single, + state: TapStateChange::Tap, + } + )); + let decline = execute + .sub_ability + .as_deref() + .expect("decline token continuation"); + assert_eq!( + decline.condition, + Some(AbilityCondition::Not { + condition: Box::new(AbilityCondition::effect_performed()), + }), + "the Vampire token must remain the optional tap's decline branch" + ); +} + +/// CR 608.2d: The controller's "you may" modal must not acquire the +/// event-relative actor provenance reserved for an explicit "they may" subject. +#[test] +fn trigger_you_may_tap_does_not_stamp_triggering_player_as_optional_actor() { + let def = parse_trigger_line( + "Whenever a creature enters, you may tap that permanent.", + "Controller's Tap", + ); + let execute = def.execute.as_deref().expect("execute ability"); + assert!(execute.optional); + assert_eq!(execute.optional_player, None); +} + +/// CR 603.4 + CR 608.2d: Actor provenance survives a supported intervening-if +/// wrapper, so its `they may` body still prompts the player from the event. +#[test] +fn conditional_they_may_tap_stamps_triggering_player_as_optional_actor() { + let def = parse_trigger_line( + "Whenever a creature enters, if that creature is white, they may tap that permanent.", + "Conditional Tap", + ); + let execute = def.execute.as_deref().expect("execute ability"); + assert!(execute.optional); + assert_eq!( + execute.optional_player, + Some(TargetFilter::TriggeringPlayer) + ); +} + +/// CR 603.2 + CR 603.6 + CR 608.2k: Only a trigger's direct, untargeted +/// "tap that permanent" instruction is rebound to the zone-change object. +/// A reflexive selected tap and an untap anaphor retain their own referents. +#[test] +fn event_source_tap_lift_preserves_reflexive_and_untap_referents() { + fn first_tap(ability: &AbilityDefinition) -> Option<&Effect> { + if matches!(ability.effect.as_ref(), Effect::SetTapState { .. }) { + return Some(ability.effect.as_ref()); + } + ability.sub_ability.as_deref().and_then(first_tap) + } + + let snare = parse_trigger_line( + "When Snaremaster Sprite enters, you may pay {2}. When you do, tap target creature an opponent controls and put a stun counter on it.", + "Snaremaster Sprite", + ); + assert!(matches!( + snare.execute.as_deref().and_then(first_tap), + Some(Effect::SetTapState { + target: TargetFilter::ParentTarget, + state: TapStateChange::Tap, + .. + }) + )); + + let howl = parse_trigger_line( + "When Howl of the Hunt enters, if enchanted creature is a Wolf or Werewolf, untap that creature.", + "Howl of the Hunt", + ); + assert!(matches!( + howl.execute.as_deref().and_then(first_tap), + Some(Effect::SetTapState { + target: TargetFilter::ParentTarget, + state: TapStateChange::Untap, + .. + }) + )); } // Guard: a bare "enters" (no tapped-state rider) must NOT attach a @@ -28872,3 +28966,44 @@ fn synthetic_sentence_separated_mass_move_damage_keeps_event_context_amount() { other => panic!("expected DamageEachPlayer(EventContextAmount, Opponent), got {other:?}"), } } + +/// SHAPE — inline modal roots are independent of `sub_ability`, but a +/// targetless top-level tap in each mode still refers to the zone-change event +/// source. This pins the parser's event-source rewrite without widening it +/// through an explicitly chosen target. +#[test] +fn event_source_lift_rewrites_inline_modal_tap_mode_roots() { + let mode = AbilityDefinition::new( + AbilityKind::Spell, + Effect::SetTapState { + target: TargetFilter::ParentTarget, + scope: EffectScope::Single, + state: TapStateChange::Tap, + }, + ); + let mut root = AbilityDefinition::new( + AbilityKind::Spell, + Effect::unimplemented("modal marker", "Choose one"), + ) + .with_modal( + ModalChoice { + min_choices: 1, + max_choices: 1, + mode_count: 1, + mode_descriptions: vec!["Tap that permanent.".to_string()], + ..Default::default() + }, + vec![mode], + ); + + lift_parent_target_to_triggering_source_in_ability(&mut root); + + assert!(matches!( + root.mode_abilities[0].effect.as_ref(), + Effect::SetTapState { + target: TargetFilter::TriggeringSource, + scope: EffectScope::Single, + state: TapStateChange::Tap, + } + )); +} diff --git a/crates/engine/src/types/ability.rs b/crates/engine/src/types/ability.rs index 44a5493940..cce1415deb 100644 --- a/crates/engine/src/types/ability.rs +++ b/crates/engine/src/types/ability.rs @@ -18940,8 +18940,13 @@ pub struct AbilityDefinition { pub optional_targeting: bool, /// CR 608.2d: When true, the controller chooses whether to perform this effect ("You may X"). pub optional: bool, + /// CR 608.2d: Event-relative player named by an optional subject (for example, + /// "they may"). Unlike `optional_for` and `target_chooser`, this selects the + /// resolution-time optional actor; `None` uses this ability's controller. + pub optional_player: Option, /// CR 608.2d: When set, an opponent (not the controller) chooses whether to perform this - /// optional effect. Requires `optional: true`. Opponents are prompted in APNAP order. + /// optional effect. Unlike `optional_player` and `target_chooser`, this is an + /// any-opponent permission. Requires `optional: true`; prompts use APNAP order. pub optional_for: Option, /// Variable-count targeting: min/max targets the player can choose. /// When present, resolution enters MultiTargetSelection instead of immediate resolve. @@ -19021,8 +19026,8 @@ pub struct AbilityDefinition { /// CR 601.2c + CR 603.3d: When set, this player (not the controller) announces /// this ability's target(s) at stack placement. `None` = controller chooses /// (default). Mirrors `target_selection_mode` (the same "by-whom are targets - /// selected" axis). Distinct from CR 608.2d resolution-time "of their choice" - /// sacrifices. + /// selected" axis). Unlike `optional_player` and `optional_for`, this is a + /// stack-placement target choice, not a resolution-time optional actor. pub target_chooser: Option, /// CR 608.2c + CR 107.1c: per-iteration loop-continuation predicate, the /// non-count companion to `repeat_for`. When `Some`, the resolution chain @@ -19084,6 +19089,8 @@ struct AbilityDefinitionRepr<'a> { optional_targeting: bool, optional: bool, #[serde(skip_serializing_if = "Option::is_none")] + optional_player: &'a Option, + #[serde(skip_serializing_if = "Option::is_none")] optional_for: &'a Option, #[serde(skip_serializing_if = "Option::is_none")] multi_target: &'a Option, @@ -19150,6 +19157,7 @@ impl Serialize for AbilityDefinition { condition, optional_targeting, optional, + optional_player, optional_for, multi_target, target_constraints, @@ -19191,6 +19199,7 @@ impl Serialize for AbilityDefinition { condition, optional_targeting: *optional_targeting, optional: *optional, + optional_player, optional_for, multi_target, target_constraints, @@ -19282,6 +19291,8 @@ struct AbilityDefinitionDe { #[serde(default)] optional: bool, #[serde(default)] + optional_player: Option, + #[serde(default)] optional_for: Option, #[serde(default)] multi_target: Option, @@ -19354,6 +19365,7 @@ impl<'de> Deserialize<'de> for AbilityDefinition { condition: de.condition, optional_targeting: de.optional_targeting, optional: de.optional, + optional_player: de.optional_player, optional_for: de.optional_for, multi_target: de.multi_target, target_constraints: de.target_constraints, @@ -19550,6 +19562,7 @@ impl AbilityDefinition { condition: None, optional_targeting: false, optional: false, + optional_player: None, optional_for: None, multi_target: None, target_constraints: Vec::new(), @@ -24402,6 +24415,9 @@ pub struct ResolvedAbility { /// CR 608.2d: Optional effect — controller prompted before execution. #[serde(default)] pub optional: bool, + /// CR 608.2d: Event-relative player explicitly named by an optional subject. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub optional_player: Option, /// CR 608.2d: When set, an opponent chooses whether to perform this optional effect. #[serde(default, skip_serializing_if = "Option::is_none")] pub optional_for: Option, @@ -24642,6 +24658,7 @@ impl ResolvedAbility { context: SpellContext::default(), optional_targeting: false, optional: false, + optional_player: None, optional_for: None, multi_target: None, target_constraints: Vec::new(), diff --git a/crates/engine/tests/integration/issue_4963_charismatic_conqueror.rs b/crates/engine/tests/integration/issue_4963_charismatic_conqueror.rs new file mode 100644 index 0000000000..d08b6d2403 --- /dev/null +++ b/crates/engine/tests/integration/issue_4963_charismatic_conqueror.rs @@ -0,0 +1,120 @@ +//! Regression for issue #4963: Charismatic Conqueror's optional tap belongs to +//! the player who controlled the permanent as it entered. + +use engine::game::scenario::{GameRunner, GameScenario, P0, P1}; +use engine::types::actions::GameAction; +use engine::types::game_state::{CastPaymentMode, WaitingFor}; +use engine::types::identifiers::ObjectId; +use engine::types::keywords::Keyword; +use engine::types::mana::ManaCost; +use engine::types::phase::Phase; + +const CHARISMATIC_CONQUEROR_ORACLE: &str = "Whenever an artifact or creature an opponent controls enters untapped, they may tap that permanent. If they don't, you create a 1/1 white Vampire creature token with lifelink."; +const CONTROLLER_MAY_TAP_ORACLE: &str = "Whenever a creature enters, you may tap that permanent."; + +fn scenario_with_optional_tapper(oracle: &str) -> (GameRunner, ObjectId) { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + scenario + .add_creature_from_oracle(P0, "Charismatic Conqueror", 2, 2, oracle) + .id(); + let entrant = scenario + .add_creature_to_hand_from_oracle(P1, "Untapped Entrant", 1, 1, "") + .with_mana_cost(ManaCost::zero()) + .id(); + let mut runner = scenario.build(); + { + let state = runner.state_mut(); + state.active_player = P1; + state.priority_player = P1; + state.waiting_for = WaitingFor::Priority { player: P1 }; + } + (runner, entrant) +} + +fn resolve_entrant_to_optional(runner: &mut GameRunner, entrant: ObjectId) { + let card_id = runner.state().objects[&entrant].card_id; + runner + .act(GameAction::CastSpell { + object_id: entrant, + card_id, + targets: vec![], + payment_mode: CastPaymentMode::Auto, + }) + .expect("cast the zero-cost entrant through the production pipeline"); + runner.advance_until_stack_empty(); +} + +/// CR 603.2 + CR 603.6a + CR 608.2d: P1's untapped creature ETB triggers +/// Conqueror, but P1—not the Conqueror controller—makes the optional choice. +/// Accepting taps the entrant and suppresses the decline token branch. +#[test] +fn charismatic_conqueror_accept_prompts_entering_controller_and_taps_entrant() { + let (mut runner, entrant) = scenario_with_optional_tapper(CHARISMATIC_CONQUEROR_ORACLE); + resolve_entrant_to_optional(&mut runner, entrant); + + assert!(matches!( + runner.state().waiting_for, + WaitingFor::OptionalEffectChoice { player: P1, .. } + )); + runner + .act(GameAction::DecideOptionalEffect { accept: true }) + .expect("P1 accepts the optional tap"); + runner.advance_until_stack_empty(); + + assert!( + runner.state().objects[&entrant].tapped, + "P1 accepted the tap" + ); + assert!( + !runner.state().objects.values().any(|object| { + object.is_token && object.name == "Vampire" && object.controller == P0 + }), + "accepting must not execute the 'If they don't' Vampire branch" + ); +} + +/// CR 608.2c + CR 109.5: declining leaves P1's entrant untapped and creates +/// the Vampire for P0, the controller of Conqueror when its trigger fired. +#[test] +fn charismatic_conqueror_decline_keeps_entrant_untapped_and_creates_p0_vampire() { + let (mut runner, entrant) = scenario_with_optional_tapper(CHARISMATIC_CONQUEROR_ORACLE); + resolve_entrant_to_optional(&mut runner, entrant); + + assert!(matches!( + runner.state().waiting_for, + WaitingFor::OptionalEffectChoice { player: P1, .. } + )); + runner + .act(GameAction::DecideOptionalEffect { accept: false }) + .expect("P1 declines the optional tap"); + runner.advance_until_stack_empty(); + + assert!( + !runner.state().objects[&entrant].tapped, + "declining must leave P1's entrant untapped" + ); + let vampires: Vec<_> = runner + .state() + .objects + .values() + .filter(|object| object.is_token && object.name == "Vampire") + .collect(); + assert_eq!(vampires.len(), 1, "declining creates one Vampire token"); + assert_eq!(vampires[0].controller, P0); + assert!(vampires[0].keywords.contains(&Keyword::Lifelink)); +} + +/// CR 608.2d: A controller's "you may tap that permanent" uses the same +/// event-object referent but must prompt P0, proving the `they may` actor stamp +/// is not inferred from the lowered tap effect. +#[test] +fn controller_may_tap_that_permanent_prompts_ability_controller() { + let (mut runner, entrant) = scenario_with_optional_tapper(CONTROLLER_MAY_TAP_ORACLE); + resolve_entrant_to_optional(&mut runner, entrant); + + assert!(matches!( + runner.state().waiting_for, + WaitingFor::OptionalEffectChoice { player: P0, .. } + )); +} diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index 24257508a7..07bb4c7e6a 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -590,6 +590,7 @@ mod issue_4955_greenbelt_rampager; mod issue_4956_gift_of_immortality_reattach; mod issue_4960_nova_flame; mod issue_4962_volo_guide_to_monsters; +mod issue_4963_charismatic_conqueror; mod issue_4966_waterbenders_ascension; mod issue_4991_vigorous_farming; mod issue_4999_treasure_cruise_delve_tokens; diff --git a/crates/engine/tests/integration/the_chain_veil_loyalty_grants.rs b/crates/engine/tests/integration/the_chain_veil_loyalty_grants.rs index 5f118cea0d..d5355c4176 100644 --- a/crates/engine/tests/integration/the_chain_veil_loyalty_grants.rs +++ b/crates/engine/tests/integration/the_chain_veil_loyalty_grants.rs @@ -168,6 +168,7 @@ fn make_grant_ability(controller: PlayerId, source: ObjectId) -> ResolvedAbility replacement_applied: Default::default(), optional_targeting: false, optional: false, + optional_player: None, optional_for: None, multi_target: None, target_constraints: Vec::new(),