diff --git a/crates/engine/src/parser/oracle_effect/mod.rs b/crates/engine/src/parser/oracle_effect/mod.rs index df5584790f..4c0f1b263f 100644 --- a/crates/engine/src/parser/oracle_effect/mod.rs +++ b/crates/engine/src/parser/oracle_effect/mod.rs @@ -18653,11 +18653,11 @@ fn rewrite_recipient_on_link(def: &mut AbilityDefinition, filter: &TargetFilter) /// `player_scope_from_parent_target_subject`), reused here rather than /// duplicated. /// -/// Total and FAIL-CLOSED: only a recipient an existing `PlayerFilter` can name -/// is bound. A recipient naming a TARGETED player ("target opponent") or an -/// object ("that creature") returns `false`, so the caller falls through to -/// `Effect::Unimplemented` instead of silently letting the printed controller -/// act in someone else's place. +/// Total and FAIL-CLOSED on BOTH sides of the binding. A recipient naming a +/// TARGETED player ("target opponent") or an object ("that creature") returns +/// `false`; so does a body that already carries its own scope. Either way the +/// caller falls through to `Effect::Unimplemented` instead of silently letting +/// the printed controller act in someone else's place. fn bind_recipient_without_recipient_slot( def: &mut AbilityDefinition, filter: &TargetFilter, @@ -18668,6 +18668,15 @@ fn bind_recipient_without_recipient_slot( if matches!(filter, TargetFilter::OriginalController) { return true; } + // The body may already carry a printed iteration scope of its own. Both + // halves are rewritten from clones of ONE parsed body, so an unguarded + // stamp would replace that fan-out with a single recipient — and with a + // DIFFERENT one on each half, since each half binds its own conjunct. The + // printed per-player iteration would be silently lost. Refuse instead: an + // honest gap beats resolving for the wrong set of players. + if def.player_scope.is_some() { + return false; + } let Some(scope) = distribution_recipient_player_scope(filter) else { return false; }; diff --git a/crates/engine/src/parser/oracle_effect/tests.rs b/crates/engine/src/parser/oracle_effect/tests.rs index 62ea126b5c..6da5390c94 100644 --- a/crates/engine/src/parser/oracle_effect/tests.rs +++ b/crates/engine/src/parser/oracle_effect/tests.rs @@ -559,11 +559,13 @@ fn possessive_actor_compound_subject_distributes_a_recipient_bearing_body() { ); } -/// CR 109.4 + CR 608.2d: the recipient-less binding channel generalizes past the +/// CR 608.2d: the recipient-less binding channel generalizes past the /// possessive axis. Infernal Offering's "You and that player each sacrifice a /// creature" has no `TargetFilter` recipient slot on `Effect::Sacrifice` either, /// and its second conjunct is the opponent a preceding "Choose an opponent." -/// picked — so the second half binds `player_scope: ChosenPlayer`. +/// picked — a choice announced while applying the effect, not an object whose +/// controller is being read — so the second half binds +/// `player_scope: ChosenPlayer`. #[test] fn recipient_less_body_binds_a_chosen_player_conjunct_by_scope() { let parsed = parse_oracle_text( @@ -602,23 +604,55 @@ fn recipient_less_body_binds_a_chosen_player_conjunct_by_scope() { ); } -/// CR 109.4: FAIL-CLOSED contract for the recipient-less binding channel. No -/// `PlayerFilter` can name a TARGETED player, so "you and target opponent each +/// CR 115.1: FAIL-CLOSED contract for the recipient-less binding channel. The +/// second conjunct is a TARGETED player — declared as the spell goes on the +/// stack — and no `PlayerFilter` names one, so "you and target opponent each /// flip a coin" (Mana Clash) / "… each secretly choose 1, 2, or 3" -/// (Expert-Level Safe) must stay an honest `Unimplemented` — binding the body to +/// (Expert-Level Safe) must stay an honest `Unimplemented`. Binding the body to /// `PlayerFilter::Opponent` would make EVERY opponent act in a multiplayer game, /// and leaving it unbound would make the caster act twice. #[test] fn recipient_less_body_with_a_targeted_player_conjunct_fails_closed() { + // Positive reach guard: the SAME grammar with a NAMEABLE conjunct must still + // parse. Without it, every assertion below would also hold if the + // compound-subject distributor had stopped running altogether — a dead path + // fails closed on everything, including cases it should bind. + // + // "that player" needs its antecedent: the binding comes from the preceding + // "Choose an opponent.", so the guard has to carry that sentence. A bare + // "You and that player each …" is itself an unbound subject, which is why + // this guard uses the full Infernal Offering text. + let reachable = parse_oracle_text( + "Choose an opponent. You and that player each sacrifice a creature.", + "Infernal Offering", + &[], + &["Sorcery".to_string()], + &[], + ); + let bound_half = reachable + .abilities + .first() + .and_then(|ability| ability.sub_ability.as_deref()) + .expect("reach-guard: the caster half must exist after the Choose"); + assert!( + matches!(&*bound_half.effect, Effect::Sacrifice { .. }), + "reach-guard: the compound-subject distributor must still bind a nameable \ + conjunct, else the fail-closed assertions below are vacuous, got {:#?}", + bound_half.effect + ); + for text in [ "You and target opponent each flip a coin.", "You and target opponent each secretly choose 1, 2, or 3.", ] { let ability = parse_effect_chain(text, AbilityKind::Spell); - assert!( - matches!(&*ability.effect, Effect::Unimplemented { .. }), - "{text:?} must fail closed, got {:#?}", - ability.effect + let Effect::Unimplemented { name, .. } = &*ability.effect else { + panic!("{text:?} must fail closed, got {:#?}", ability.effect); + }; + assert_eq!( + name, "unbound_subject", + "{text:?} must fail closed AT THE SUBJECT — any other gap name means the \ + clause died earlier and this case stopped covering the targeted-player path" ); assert_eq!( ability.player_scope, None, diff --git a/crates/engine/src/parser/oracle_ir/ast.rs b/crates/engine/src/parser/oracle_ir/ast.rs index c95052cbc3..c58de09d2b 100644 --- a/crates/engine/src/parser/oracle_ir/ast.rs +++ b/crates/engine/src/parser/oracle_ir/ast.rs @@ -193,11 +193,15 @@ pub(crate) struct SubjectPhraseAst { /// BOARD-WIDE effect — the grant landed on every permanent, lands and /// artifacts included, while coverage still reported the card as supported. /// Encoding the unbound state in the type makes that fail-open - /// unrepresentable: every consumer must say what it does with `None`, and - /// the one consumer that actually reads this field - /// (`lower_subject_predicate_ast`'s `ImperativeFallback` arm, the only - /// predicate kind that applies the subject filter) fails closed to - /// `Effect::unimplemented`. Same shape, same reason, as + /// unrepresentable: every consumer must say what it does with `None`. + /// `lower_subject_predicate_ast`'s `ImperativeFallback` arm — the only + /// predicate kind that applies the subject filter — is the only consumer + /// that treats `None` as a coverage GAP, failing closed to + /// `Effect::unimplemented`. The other readers + /// (`sync_subject_into_nested_shuffle_sub`, `inject_subject_target`) reach + /// it through `target.or(affected)` and treat `None` as "nothing to + /// rebind", returning early. `None` is therefore reachable in all three — + /// do not assume otherwise when editing them. Same shape, same reason, as /// [`EntersUnderSpec::UnboundAnaphor`]. pub(crate) affected: Option, pub(crate) target: Option, diff --git a/crates/engine/tests/integration/wand_of_orcus_compound_subject_6965.rs b/crates/engine/tests/integration/wand_of_orcus_compound_subject_6965.rs index a507c58787..186c1121de 100644 --- a/crates/engine/tests/integration/wand_of_orcus_compound_subject_6965.rs +++ b/crates/engine/tests/integration/wand_of_orcus_compound_subject_6965.rs @@ -21,13 +21,16 @@ //! //! CR 611.2c: one continuous effect naming several subjects determines the set //! each part applies to independently — i.e. the UNION of the named subjects. -//! CR 301.5f: an Equipment attaches to a creature. +//! CR 301.5a: an Equipment is attached to a creature, which is then the +//! "equipped creature". CR 301.5f: an ability referring to the "equipped +//! creature" means whatever creature the permanent is attached to. //! CR 702.2b: deathtouch. CR 702.11b: hexproof. use engine::game::combat::AttackTarget; use engine::game::game_object::AttachTarget; use engine::game::layers::evaluate_layers; use engine::game::scenario::{GameRunner, GameScenario, P0, P1}; +use engine::types::ability::Effect; use engine::types::card_type::CoreType; use engine::types::identifiers::ObjectId; use engine::types::keywords::Keyword; @@ -76,8 +79,9 @@ fn wand_of_orcus_unbindable_subject_grants_nothing_board_wide() { let mut runner = scenario.build(); - // CR 301.5f: make the Wand a real Equipment attached to `host`, so its - // "equipped creature attacks" trigger has a subject to fire on. + // CR 301.5a: attach the Wand to `host` so it is a real Equipment with an + // equipped creature. CR 301.5f: that is what its "equipped creature + // attacks" trigger resolves against, so the trigger has a subject to fire on. { let obj = runner.state_mut().objects.get_mut(&wand).unwrap(); obj.card_types.core_types = vec![CoreType::Artifact]; @@ -104,6 +108,38 @@ fn wand_of_orcus_unbindable_subject_grants_nothing_board_wide() { "the attack trigger must be on the stack, or nothing below is exercised" ); + // ...and the trigger must carry the SPECIFIC gap this test is about. Stack + // presence alone proves only that a trigger was created: a regression that + // dropped the execute effect entirely would also grant no deathtouch and + // leave every assertion below green. Pin the reason, not just the silence. + { + let wand_obj = runner.state().objects.get(&wand).unwrap(); + let gap = wand_obj + .trigger_definitions + .iter_unchecked() + .filter_map(|entry| entry.definition.execute.as_ref()) + .find_map(|exec| match exec.effect.as_ref() { + Effect::Unimplemented { name, description } => Some((name, description)), + _ => None, + }) + .expect( + "the attack trigger's execute chain must be an Unimplemented gap — if it \ + parsed, or vanished, the deathtouch assertions below prove nothing", + ); + assert_eq!( + gap.0, "unbound_subject", + "the gap must name the SUBJECT as the unbound part; another name means the \ + clause failed elsewhere and this test stopped covering the fail-closed path" + ); + assert!( + gap.1 + .as_deref() + .is_some_and(|text| text.contains("Zombies you control")), + "reach-guard: the gap must quote the unbindable conjunct, got {:?}", + gap.1 + ); + } + runner.advance_until_stack_empty(); runner.state_mut().layers_dirty.mark_full(); evaluate_layers(runner.state_mut());