diff --git a/crates/engine/src/parser/oracle_effect/mod.rs b/crates/engine/src/parser/oracle_effect/mod.rs index 0250608674..df5584790f 100644 --- a/crates/engine/src/parser/oracle_effect/mod.rs +++ b/crates/engine/src/parser/oracle_effect/mod.rs @@ -17370,7 +17370,7 @@ fn try_parse_target_subject_multi_target_damage_chain( let application = parse_subject_application(subject_text, &mut tentative_ctx)?; application.target.as_ref()?; let subject = SubjectPhraseAst { - affected: application.affected, + affected: Some(application.affected), target: application.target, multi_target: application.multi_target, inherits_parent: application.inherits_parent, @@ -18302,17 +18302,77 @@ fn type_phrase_has_compound_conjunction(type_phrase: &str) -> bool { .is_ok() } -fn parse_static_compound_subject_prefix( - lower: &str, +/// CR 109.4 + CR 115.1 + CR 608.2c: Second-subject axis for a POSSESSIVE-ACTOR +/// conjunct — "target <filter>'s controller each " / "target <filter>'s owner +/// each " (Life at Stake: "You and target creature's controller each secretly +/// choose a number 0 or greater"; Eye to Eye; the same conjunct shape over any +/// object noun). +/// +/// The conjunct is delegated to the single-subject grammar +/// ([`subject::parse_subject_application`]), the established authority for this +/// possessive form: it resolves the acting player to +/// `ParentTargetController`/`ParentTargetOwner` (CR 109.4) while preserving the +/// announced object as the ability's TARGET (CR 115.1). Both halves are +/// load-bearing — an application with no announced target is a different +/// subject class (a class filter, an anaphor) and is left to the other axes, so +/// this axis never fabricates a target slot. +/// +/// The declared target is returned to the caller, which emits it as a leading +/// [`Effect::TargetOnly`] slot; without that slot the returned +/// `ParentTargetController` recipient would have no target to read (CR 601.2c). +pub(super) fn parse_possessive_actor_each_second_subject( + rest: &str, ) -> Option<(usize, TargetFilter, TargetFilter)> { - let (remaining, (first_filter, second_filter)) = ( - alt(( - value( - TargetFilter::OriginalController, - tag::<_, _, OracleError<'_>>("you and "), - ), - value(TargetFilter::SelfRef, tag("~ and ")), - )), + let (remaining, subject) = terminated( + take_until::<_, _, OracleError<'_>>(" each "), + tag::<_, _, OracleError<'_>>(" each "), + ) + .parse(rest) + .ok()?; + let application = + subject::parse_subject_application(subject.trim(), &mut ParseContext::default())?; + if !matches!( + application.affected, + TargetFilter::ParentTargetController | TargetFilter::ParentTargetOwner + ) { + return None; + } + let declared_target = application.target?; + Some(( + rest.len() - remaining.len(), + application.affected, + declared_target, + )) +} + +/// CR 109.5: First-subject axis shared by every compound-subject prefix form — +/// "you" (the printed ability controller, CR 109.5) or "~" (the ability source +/// itself, the object axis). +fn parse_compound_first_subject(lower: &str) -> OracleResult<'_, TargetFilter> { + alt(( + value( + TargetFilter::OriginalController, + tag::<_, _, OracleError<'_>>("you and "), + ), + value(TargetFilter::SelfRef, tag("~ and ")), + )) + .parse(lower) +} + +/// CR 109.5 + CR 115.1 + CR 608.2c: A parsed "<A> and <B> each " distribution +/// prefix. `declares_target` is `Some` only for a second subject that ANNOUNCES +/// its own object target (the possessive-actor axis); every other axis binds a +/// recipient that needs no new target slot. +struct CompoundSubjectPrefix { + consumed: usize, + first: TargetFilter, + second: TargetFilter, + declares_target: Option, +} + +fn parse_static_compound_subject_prefix(lower: &str) -> Option { + let (remaining, (first, second)) = ( + parse_compound_first_subject, alt(( value( TargetFilter::ScopedPlayer, @@ -18328,34 +18388,47 @@ fn parse_static_compound_subject_prefix( ) .parse(lower) .ok()?; - Some((lower.len() - remaining.len(), first_filter, second_filter)) + Some(CompoundSubjectPrefix { + consumed: lower.len() - remaining.len(), + first, + second, + declares_target: None, + }) } -fn parse_dynamic_compound_subject_prefix( - lower: &str, -) -> Option<(usize, TargetFilter, TargetFilter)> { - let (remaining, first_filter) = alt(( - value( - TargetFilter::OriginalController, - tag::<_, _, OracleError<'_>>("you and "), - ), - value(TargetFilter::SelfRef, tag("~ and ")), +fn parse_dynamic_compound_subject_prefix(lower: &str) -> Option { + let (remaining, first) = alt(( + parse_compound_first_subject, value(TargetFilter::SelfRef, tag("it and ")), )) .parse(lower) .ok()?; - let (second_consumed, second_filter) = parse_controlled_creature_each_second_subject(remaining) + let (second_consumed, second) = parse_controlled_creature_each_second_subject(remaining) .or_else(|| parse_other_creatures_share_type_each_second_subject(remaining))?; - Some(( - lower.len() - remaining.len() + second_consumed, - first_filter, - second_filter, - )) + Some(CompoundSubjectPrefix { + consumed: lower.len() - remaining.len() + second_consumed, + first, + second, + declares_target: None, + }) +} + +fn parse_possessive_actor_compound_subject_prefix(lower: &str) -> Option { + let (remaining, first) = parse_compound_first_subject(lower).ok()?; + let (second_consumed, second, declared_target) = + parse_possessive_actor_each_second_subject(remaining)?; + Some(CompoundSubjectPrefix { + consumed: lower.len() - remaining.len() + second_consumed, + first, + second, + declares_target: Some(declared_target), + }) } -fn parse_compound_subject_prefix(lower: &str) -> Option<(usize, TargetFilter, TargetFilter)> { +fn parse_compound_subject_prefix(lower: &str) -> Option { parse_static_compound_subject_prefix(lower) .or_else(|| parse_dynamic_compound_subject_prefix(lower)) + .or_else(|| parse_possessive_actor_compound_subject_prefix(lower)) } /// CR 109.5 + CR 608.2c: True when `text` opens with a compound-subject @@ -18377,8 +18450,12 @@ fn try_parse_compound_subject_each( // Compose the prefix through the shared grammar in // `parse_compound_subject_prefix` (kept in lockstep with the chunk-loop guard // `text_is_compound_subject_distribution`). - let (consumed_prefix, first_filter, second_filter) = - parse_compound_subject_prefix(lower.as_str())?; + let CompoundSubjectPrefix { + consumed: consumed_prefix, + first: first_filter, + second: second_filter, + declares_target, + } = 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 @@ -18447,7 +18524,7 @@ fn try_parse_compound_subject_each( } tail.sub_ability = Some(Box::new(half_b)); - Some(ParsedEffectClause { + let distributed = ParsedEffectClause { effect: *half_a.effect, duration: half_a.duration, sub_ability: half_a.sub_ability, @@ -18456,6 +18533,30 @@ fn try_parse_compound_subject_each( condition: half_a.condition, optional: false, unless_pay: None, + }; + + // CR 115.1 + CR 601.2c: a possessive-actor second subject ("target + // creature's controller") names its player THROUGH an announced object + // target. Declare that target with a leading `Effect::TargetOnly` — the + // same slot-only head the single-subject grammar emits for "target + // creature's controller s" — so the recipient's + // `ParentTargetController` reference (and any later "that creature" + // anaphor) has a target slot to resolve against. + let Some(target) = declares_target else { + return Some(distributed); + }; + Some(ParsedEffectClause { + effect: Effect::TargetOnly { target }, + duration: None, + sub_ability: Some(Box::new(ability_definition_from_clause( + AbilityKind::Spell, + distributed, + ))), + distribute: None, + multi_target: None, + condition: None, + optional: false, + unless_pay: None, }) } @@ -18529,12 +18630,72 @@ fn rewrite_recipient_on_link(def: &mut AbilityDefinition, filter: &TargetFilter) } true } - // Any other effect family is out of scope for compound-subject - // distribution at this entry point. Returning false keeps the - // detector tight and prevents silent misparse on bodies whose - // recipient binding is encoded differently (e.g. nested filter props). - _ => false, + // No `TargetFilter`-typed recipient slot on this effect family. The + // acting player of such an effect is the resolving ability's + // controller, so the recipient binds on the ABILITY rather than the + // effect — see `bind_recipient_without_recipient_slot`. Recipients no + // `PlayerFilter` can name still return `false`, keeping the detector + // tight against bodies whose recipient binding is encoded differently + // (e.g. nested filter props). + _ => bind_recipient_without_recipient_slot(def, filter), + } +} + +/// CR 109.4 + CR 608.2d: Bind a distribution recipient on a link whose effect +/// has NO `TargetFilter`-typed recipient slot — the "who decides / who acts" +/// class (`Effect::Choose`, `Effect::Sacrifice`, …), where the acting player is +/// the resolving ability's controller rather than a field on the effect. +/// +/// The binding channel is `AbilityDefinition.player_scope`, whose fan-out +/// rebinds the acting controller per matching player (`resolve_ability_chain`). +/// This is the same lift the single-subject grammar already performs for a +/// slot-less predicate ("its controller investigates" — +/// `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. +fn bind_recipient_without_recipient_slot( + def: &mut AbilityDefinition, + filter: &TargetFilter, +) -> bool { + // CR 109.5: "you" — the printed controller already IS the acting player of + // an unscoped ability, so this half needs no scope. Stamping one would be a + // redundant single-player fan-out. + if matches!(filter, TargetFilter::OriginalController) { + return true; } + let Some(scope) = distribution_recipient_player_scope(filter) else { + return false; + }; + def.player_scope = Some(scope); + true +} + +/// CR 109.4: Map a distribution recipient to the `PlayerFilter` iteration scope +/// that names exactly that one player. Composed from the existing +/// parent-target subject mapping plus the resolution-chosen player, the two +/// recipient forms this distributor produces that a `PlayerFilter` can name. +/// Everything else is `None` — see the fail-closed contract on +/// `bind_recipient_without_recipient_slot`. +fn distribution_recipient_player_scope(filter: &TargetFilter) -> Option { + player_scope_from_parent_target_subject(filter).or_else(|| match filter { + // CR 608.2d: "that player" already rebound by `try_parse_compound_subject_each` + // to the opponent a preceding "Choose an opponent." picked (the Offering + // cycle) — a resolution-scoped single player, not a player class. + TargetFilter::Typed(tf) if tf.type_filters.is_empty() && tf.properties.is_empty() => { + match tf.controller { + Some(ControllerRef::ChosenPlayer { index }) => { + Some(PlayerFilter::ChosenPlayer { index }) + } + _ => None, + } + } + _ => None, + }) } /// CR 614.1a + CR 701.5 + CR 608.2c: Detect the "exile-after-cast/counter rider" @@ -20055,6 +20216,18 @@ fn lower_subject_predicate_ast( unless_pay: None, }, PredicateAst::ImperativeFallback { text } => { + // Issue #6965: this is the ONLY predicate kind that applies the + // subject filter — the `Continuous` / `Become` / `Restriction` arms + // above lower an effect their own clause parser already bound. So + // this is the one place an unbound subject (`affected == None`) must + // fail closed. It used to arrive as `TargetFilter::Any`, which + // matches unconditionally (`game/filter.rs`), turning a parse + // FAILURE into a board-wide grant. `Effect::unimplemented` is the + // repo's single authority for "the parser couldn't handle this"; + // see `subject::UNBOUND_SUBJECT_GAP` for the recorded decision. + let Some(affected) = subject.affected.clone() else { + return parsed_clause(Effect::unimplemented(subject::UNBOUND_SUBJECT_GAP, text)); + }; let pred_lower = text.to_lowercase(); // CR 120.1 + CR 601.2c: Native IR has already separated an explicit // target damage source from its predicate here ("Target creature … @@ -20072,29 +20245,22 @@ fn lower_subject_predicate_ast( } } if matches!(pred_lower.as_str(), "shuffle" | "shuffles") - && matches!( - subject.affected, - TargetFilter::Player | TargetFilter::Controller - ) + && matches!(affected, TargetFilter::Player | TargetFilter::Controller) { - return parsed_clause(Effect::Shuffle { - target: subject.affected, - }); + return parsed_clause(Effect::Shuffle { target: affected }); } // CR 701.20a: " reveals cards from the top of their library // until they reveal a [filter]" — third-person form. The subject // (e.g., "its controller", "that player", "target opponent") was - // extracted into `subject.affected` and identifies whose library is + // extracted into `affected` and identifies whose library is // revealed. Must be checked BEFORE the RevealTop fallback below, // which would otherwise greedy-match the "reveals/top/library" verbs. { let pred_tp = TextPair::new(text.as_str(), pred_lower.as_str()); - if let Some(clause) = - try_parse_exile_from_top_until(pred_tp, subject.affected.clone()) - { + if let Some(clause) = try_parse_exile_from_top_until(pred_tp, affected.clone()) { return clause; } - if let Some(clause) = try_parse_reveal_until(pred_tp, subject.affected.clone()) { + if let Some(clause) = try_parse_reveal_until(pred_tp, affected.clone()) { return clause; } } @@ -20115,7 +20281,7 @@ fn lower_subject_predicate_ast( 1 }; return parsed_clause(Effect::RevealTop { - player: subject.affected, + player: affected, count, }); } @@ -20133,7 +20299,7 @@ fn lower_subject_predicate_ast( // moved object's `face_down` flag so `visibility.rs` redacts it. let face_down = scan_contains_phrase(&pred_lower, "face down"); return parsed_clause(Effect::ExileTop { - player: subject.affected, + player: affected, count, position: crate::types::ability::LibraryPosition::Top, face_down, @@ -20142,7 +20308,7 @@ fn lower_subject_predicate_ast( // CR 701.40a + CR 608.2c: " manifests the top [N] card(s) of // their library" — Reality Shift's "its controller manifests the top // card of their library" routes through this arm so the acting - // player is bound to `subject.affected` (e.g., ParentTargetController) + // player is bound to `affected` (e.g., ParentTargetController) // rather than the default Controller. if alt((tag::<_, _, OracleError<'_>>("manifest "), tag("manifests "))) .parse(pred_lower.as_str()) @@ -20160,7 +20326,7 @@ fn lower_subject_predicate_ast( QuantityExpr::Fixed { value: 1 } }; return parsed_clause(Effect::Manifest { - target: subject.affected, + target: affected, count, profile: None, enters_under: None, @@ -20189,7 +20355,7 @@ fn lower_subject_predicate_ast( // following clause ("that player may …") — resolve against it. Mirrors // the player-target ChangeZone / Explore wrapping just below. if matches!( - subject.affected, + affected, TargetFilter::ParentTargetController | TargetFilter::ParentTargetOwner ) { if let Some(object_target) = subject.target.clone() { @@ -20225,7 +20391,7 @@ fn lower_subject_predicate_ast( // `recipient = ParentTarget` ("It" = the +1/+1 target); do NOT // clobber that explicit binding with the bare-pronoun subject. if matches!(recipient, TargetFilter::SelfRef) { - *recipient = subject.affected.clone(); + *recipient = affected.clone(); } return clause; } @@ -20256,7 +20422,7 @@ fn lower_subject_predicate_ast( if let Effect::ChooseFromZone { chooser, .. } = &mut clause.effect { if subject.target.is_none() && matches!( - &subject.affected, + &affected, TargetFilter::Typed(tf) if tf.controller == Some(ControllerRef::Opponent) && tf.type_filters.is_empty() @@ -20273,10 +20439,8 @@ fn lower_subject_predicate_ast( filter: Some(_), .. } - ) && !matches!( - subject.affected, - TargetFilter::Controller | TargetFilter::SelfRef - ) { + ) && !matches!(affected, TargetFilter::Controller | TargetFilter::SelfRef) + { return parsed_clause(Effect::Unimplemented { name: "choose".to_string(), description: Some(text.clone()), @@ -20393,12 +20557,10 @@ fn lower_subject_predicate_ast( let subject_filter = if subject.inherits_parent { TargetFilter::ParentTarget } else { - subject.target.as_ref().unwrap_or(&subject.affected).clone() + subject.target.as_ref().unwrap_or(&affected).clone() }; - if subject.target.is_some() - || matches!(subject.affected, TargetFilter::TriggeringSource) - { + if subject.target.is_some() || matches!(affected, TargetFilter::TriggeringSource) { let mut explore = AbilityDefinition::new(AbilityKind::Spell, Effect::Explore); explore.sub_ability = clause.sub_ability; return ParsedEffectClause { @@ -20418,7 +20580,7 @@ fn lower_subject_predicate_ast( return clause; } - if !matches!(subject.affected, TargetFilter::SelfRef) { + if !matches!(affected, TargetFilter::SelfRef) { return ParsedEffectClause { effect: Effect::ExploreAll { filter: subject_filter, @@ -20449,7 +20611,7 @@ fn lower_subject_predicate_ast( // explicit parent-target player anaphor; a bare "investigate" leaves // `affected == SelfRef`/`Controller` and is untouched (caster default). if matches!(clause.effect, Effect::Investigate) { - if let Some(scope) = player_scope_from_parent_target_subject(&subject.affected) { + if let Some(scope) = player_scope_from_parent_target_subject(&affected) { ctx.pending_player_scope = Some(scope); } // CR 701.16a + CR 608.2c + CR 400.7: "investigate FOR EACH nontoken @@ -20474,7 +20636,7 @@ fn lower_subject_predicate_ast( // this for explicit graveyard-to-hand `Effect::ChangeZone`; the // rebind tree-walks the filter and is a no-op when the filter has // no `ScopedPlayer` ref. - if let TargetFilter::Typed(tf) = &subject.affected { + if let TargetFilter::Typed(tf) = &affected { if let Some(ControllerRef::ChosenPlayer { index }) = tf.controller { match &mut clause.effect { Effect::Bounce { target, .. } | Effect::ChangeZone { target, .. } => { @@ -20486,7 +20648,7 @@ fn lower_subject_predicate_ast( } if let Effect::PayCost { payer, .. } = &mut clause.effect { if matches!( - subject.affected, + affected, TargetFilter::Controller | TargetFilter::Player | TargetFilter::ParentTargetController @@ -20501,7 +20663,7 @@ fn lower_subject_predicate_ast( | TargetFilter::Owner | TargetFilter::SpecificPlayer { .. } ) { - *payer = subject.affected.clone(); + *payer = affected.clone(); } } // CR 113.10 + CR 702.16j: When the subject is a player-scope filter @@ -20510,7 +20672,7 @@ fn lower_subject_predicate_ast( // Protection), retarget the static's `affected` from SelfRef (the // spell object) to the subject filter so the keyword is granted to // the player. This is the Teferi's-Protection-clause-2 hook. - retarget_player_scoped_keyword_grant(&mut clause.effect, &subject.affected); + retarget_player_scoped_keyword_grant(&mut clause.effect, &affected); if clause.multi_target.is_none() { clause.multi_target = subject.multi_target; } @@ -21428,7 +21590,11 @@ fn sync_subject_into_nested_shuffle_sub( clause: &mut ParsedEffectClause, subject: &SubjectPhraseAst, ) { - let subject_filter = subject.target.as_ref().unwrap_or(&subject.affected); + // Issue #6965: no bound subject and no target means there is nothing to + // rebind — return rather than fabricate a filter. + let Some(subject_filter) = subject.target.as_ref().or(subject.affected.as_ref()) else { + return; + }; if !target_filter_can_target_player(subject_filter) { return; } @@ -21469,7 +21635,11 @@ fn sync_subject_into_nested_shuffle_sub( } fn inject_subject_target(effect: &mut Effect, subject: &SubjectPhraseAst) { - let subject_filter = subject.target.as_ref().unwrap_or(&subject.affected).clone(); + // Issue #6965: no bound subject and no target means there is nothing to + // rebind — return rather than fabricate a filter. + let Some(subject_filter) = subject.target.clone().or_else(|| subject.affected.clone()) else { + return; + }; // CR 603.6 + CR 120.1: "that creature/permanent deals damage equal to // its power..." in an ETB trigger makes the triggering object, not the // trigger source permanent, the damage source. Keep the parsed damage @@ -31850,7 +32020,7 @@ pub(crate) fn parse_effect_chain_ir( // NOT a fresh copy of the player filter, which would surface a // second target slot and prompt the player again (#2344). let subject = SubjectPhraseAst { - affected: TargetFilter::ParentTarget, + affected: Some(TargetFilter::ParentTarget), target: Some(TargetFilter::ParentTarget), multi_target: None, inherits_parent: true, diff --git a/crates/engine/src/parser/oracle_effect/sequence.rs b/crates/engine/src/parser/oracle_effect/sequence.rs index 090aa21fb7..ca12c8844d 100644 --- a/crates/engine/src/parser/oracle_effect/sequence.rs +++ b/crates/engine/src/parser/oracle_effect/sequence.rs @@ -2352,6 +2352,10 @@ fn is_inside_temporal_prefix(lower: &str) -> bool { /// - "that creature each" — the object-axis form (CR 115.1 parent-target /// binding; e.g. Gogo, Mysterious Mime's "~ and that creature each get /// +2/+0 and gain haste ... and attack this turn if able"). +/// - "target <filter>'s controller/owner each" — the possessive-actor form +/// (CR 109.4; Life at Stake's "You and target creature's controller each +/// secretly choose a number 0 or greater"), delegated to the shared axis +/// combinator so the two sites cannot drift. fn remainder_trimmed_starts_with_compound_subject_each(remainder: &str) -> bool { let lower = remainder.to_ascii_lowercase(); let result: nom::IResult<&str, (), OracleError<'_>> = alt(( @@ -2365,6 +2369,7 @@ fn remainder_trimmed_starts_with_compound_subject_each(remainder: &str) -> bool return true; } controlled_creature_each_subject_starts(&lower) + || super::parse_possessive_actor_each_second_subject(&lower).is_some() } fn controlled_creature_each_subject_starts(lower: &str) -> bool { diff --git a/crates/engine/src/parser/oracle_effect/subject.rs b/crates/engine/src/parser/oracle_effect/subject.rs index d5d180f6fa..50c2f31af6 100644 --- a/crates/engine/src/parser/oracle_effect/subject.rs +++ b/crates/engine/src/parser/oracle_effect/subject.rs @@ -42,9 +42,68 @@ use super::super::oracle_static::{ }; use super::super::oracle_target::{parse_target, parse_target_with_ctx, parse_type_phrase}; use super::super::oracle_util::{ - parse_number, TextPair, SELF_REF_PARSE_ONLY_PHRASES, SELF_REF_TYPE_PHRASES, + merge_or_filters, parse_number, TextPair, SELF_REF_PARSE_ONLY_PHRASES, SELF_REF_TYPE_PHRASES, }; +/// Coverage category key for "this sentence printed a subject, and the subject +/// grammar could not bind it". +/// +/// **Recorded decision (issue #6965).** The two subject-predicate sites that +/// re-derive a subject phrase used to substitute +/// +/// ```text +/// SubjectApplication { affected: TargetFilter::Any, .. } +/// ``` +/// +/// when [`parse_subject_application`] returned `None`. `TargetFilter::Any` +/// matches unconditionally (`game/filter.rs`), so a parse FAILURE produced a +/// BOARD-WIDE effect: the grant landed on every permanent, lands and artifacts +/// included, while coverage still reported the card as supported. That is a +/// fail-open default in a rules engine, and it was unbounded — every phrasing +/// the subject grammar does not yet cover inherited it. +/// +/// The chosen replacement is `Effect::unimplemented` (issue #6965 option 1), +/// the repo's single authority for "the parser couldn't handle this". The card +/// then reports as unsupported, which is TRUE, rather than supported-but-wrong. +/// Deliberately NOT chosen: +/// - a silent no-op (conservative, but it still fabricates a successful parse +/// and hides the gap from coverage); +/// - a per-call-site permissive default (nothing in this parser has a +/// legitimate need to broadcast an unbound subject). +/// +/// The state itself is carried by [`SubjectPhraseAst::affected`] being `None`, +/// so the fail-open cannot be reintroduced by adding another call site; the +/// gap effect is emitted at the single consumer that applies the filter +/// (`lower_subject_predicate_ast`). +/// +/// CR 608.2c ("read the whole text and apply the rules of English to the +/// text") is the rules-side statement of the same rule: a printed subject the +/// parser cannot bind must not be silently widened. +pub(super) const UNBOUND_SUBJECT_GAP: &str = "unbound_subject"; + +/// Build the IR subject phrase from an optional [`SubjectApplication`], +/// propagating "the subject grammar could not bind this phrase" as +/// [`SubjectPhraseAst::affected`] `== None` (issue #6965) rather than as a +/// fabricated filter. +fn subject_phrase_ast(application: Option) -> SubjectPhraseAst { + match application { + Some(application) => SubjectPhraseAst { + affected: Some(application.affected), + target: application.target, + multi_target: application.multi_target, + inherits_parent: application.inherits_parent, + is_optional: application.is_optional, + }, + None => SubjectPhraseAst { + affected: None, + target: None, + multi_target: None, + inherits_parent: false, + is_optional: false, + }, + } +} + pub(super) fn try_parse_subject_predicate_ast( text: &str, ctx: &mut ParseContext, @@ -232,23 +291,26 @@ pub(super) fn try_parse_subject_predicate_ast( if let Some(stripped) = strip_subject_clause(text) { let subject_text = extract_subject_text(text)?; - let application = - parse_subject_application(&subject_text, ctx).unwrap_or(SubjectApplication { - affected: TargetFilter::Any, - target: None, - multi_target: None, - inherits_parent: false, - is_optional: false, - }); + // Issue #6965: an unbindable subject stays UNBOUND. It used to become + // `TargetFilter::Any` here, which broadcast the predicate over every + // permanent; see `SubjectPhraseAst::affected`. This is the arm that + // matters — `ImperativeFallback` is the only predicate kind that applies + // the subject filter, so it is the one that fails closed on `None`. + let application = parse_subject_application(&subject_text, ctx); + // Diagnostics: when the subject is unbound the whole clause is the gap, + // so carry the WHOLE printed clause as the fragment. The stripped + // predicate alone would hide the subject that actually failed, which is + // the one thing a reader of the coverage report needs to see. + let predicate_text = if application.is_some() { + stripped + } else { + text.to_string() + }; return Some(ClauseAst::SubjectPredicate { - subject: Box::new(SubjectPhraseAst { - affected: application.affected, - target: application.target, - multi_target: application.multi_target, - inherits_parent: application.inherits_parent, - is_optional: application.is_optional, + subject: Box::new(subject_phrase_ast(application)), + predicate: Box::new(PredicateAst::ImperativeFallback { + text: predicate_text, }), - predicate: Box::new(PredicateAst::ImperativeFallback { text: stripped }), }); } @@ -264,23 +326,25 @@ fn subject_predicate_ast_from_clause( where F: FnOnce(Effect, Option, Option>) -> PredicateAst, { - let subject_text = extract_subject_text(text).unwrap_or_default(); - let application = parse_subject_application(&subject_text, ctx).unwrap_or(SubjectApplication { - affected: TargetFilter::Any, - target: None, - multi_target: None, - inherits_parent: false, - is_optional: false, - }); + // Issue #6965: an unbindable subject stays UNBOUND (see + // `SubjectPhraseAst::affected`); it used to become `TargetFilter::Any`. + // Both halves can fail: `extract_subject_text` returns `None` when + // `find_predicate_start` found no verb at all, and the previous + // `.unwrap_or_default()` then handed `parse_subject_application` an EMPTY + // string, which it rejects — so that path reached the same fabricated + // filter by a second route. + // + // `build_predicate` here only ever produces `Continuous` / `Become` / + // `Restriction` (every caller in this module does), and those three lower + // the effect their own clause parser already built — they never read + // `affected`. So `None` is inert on this path rather than a new gap; the + // point of carrying it is that a future predicate kind which DOES read the + // filter cannot silently inherit a permissive default. + let application = extract_subject_text(text) + .and_then(|subject_text| parse_subject_application(&subject_text, ctx)); ClauseAst::SubjectPredicate { - subject: Box::new(SubjectPhraseAst { - affected: application.affected, - target: application.target, - multi_target: application.multi_target, - inherits_parent: application.inherits_parent, - is_optional: application.is_optional, - }), + subject: Box::new(subject_phrase_ast(application)), predicate: Box::new(build_predicate( clause.effect, clause.duration, @@ -325,7 +389,7 @@ fn try_parse_subject_additive_type_clause(text: &str, ctx: &mut ParseContext) -> Some(ClauseAst::SubjectPredicate { subject: Box::new(SubjectPhraseAst { - affected: application.affected, + affected: Some(application.affected), target: application.target, multi_target: application.multi_target, inherits_parent: application.inherits_parent, @@ -382,7 +446,7 @@ fn try_parse_contracted_subject_additive_type_clause( ) { return Some(ClauseAst::SubjectPredicate { subject: Box::new(SubjectPhraseAst { - affected: application.affected.clone(), + affected: Some(application.affected.clone()), target: application.target.clone(), multi_target: application.multi_target.clone(), inherits_parent: application.inherits_parent, @@ -410,7 +474,7 @@ fn try_parse_contracted_subject_additive_type_clause( if let Some(clause) = build_additive_type_continuous_clause(&application, &predicate) { return Some(ClauseAst::SubjectPredicate { subject: Box::new(SubjectPhraseAst { - affected: application.affected, + affected: Some(application.affected), target: application.target, multi_target: application.multi_target, inherits_parent: application.inherits_parent, @@ -453,7 +517,7 @@ fn try_parse_contracted_subject_additive_type_clause( let clause = build_become_clause(application.clone(), &become_predicate, ctx)?; Some(ClauseAst::SubjectPredicate { subject: Box::new(SubjectPhraseAst { - affected: application.affected, + affected: Some(application.affected), target: application.target, multi_target: application.multi_target, inherits_parent: application.inherits_parent, @@ -824,7 +888,7 @@ fn try_parse_subject_supertype_removal_clause( }; Some(ClauseAst::SubjectPredicate { subject: Box::new(SubjectPhraseAst { - affected: application.affected, + affected: Some(application.affected), target: application.target, multi_target: application.multi_target, inherits_parent: application.inherits_parent, @@ -1241,7 +1305,7 @@ fn try_parse_subject_base_pt_set_clause_ast( Some(ClauseAst::SubjectPredicate { subject: Box::new(SubjectPhraseAst { - affected: application.affected, + affected: Some(application.affected), target: application.target, multi_target: application.multi_target, inherits_parent: application.inherits_parent, @@ -1406,7 +1470,7 @@ fn try_parse_source_and_other_restriction_clause( Some(ClauseAst::SubjectPredicate { subject: Box::new(SubjectPhraseAst { - affected: primary_application.affected, + affected: Some(primary_application.affected), target: primary_application.target, multi_target: None, inherits_parent: primary_application.inherits_parent, @@ -1504,7 +1568,7 @@ fn try_parse_target_and_same_name_pump_clause( Some(ClauseAst::SubjectPredicate { subject: Box::new(SubjectPhraseAst { - affected: primary.affected, + affected: Some(primary.affected), target: primary.target, multi_target: None, inherits_parent: primary.inherits_parent, @@ -2152,26 +2216,11 @@ pub(super) fn parse_subject_application( let lower = subject.to_lowercase(); - if let Ok((_, _)) = all_consuming(( - tag::<_, _, OracleError<'_>>("you"), - tag(" and "), - tag("permanents you control"), - )) - .parse(lower.as_str()) - { - let (permanents, rest) = parse_target("all permanents you control"); - if rest.trim().is_empty() { - return Some(SubjectApplication { - affected: TargetFilter::Or { - filters: vec![TargetFilter::Controller, permanents], - }, - target: None, - multi_target: None, - inherits_parent: false, - is_optional: false, - }); - } - } + // NOTE (issue #6965): the literal `"you" + " and " + "permanents you + // control"` arm that used to sit here is now handled by the general + // `parse_conjoined_subject_application` union arm at the end of this + // function, which parses each conjunct with this same grammar instead of + // matching one printed phrase. // CR 115.10a: "another target X" — target with Another filter property, // excluding the source object from legal targets. @@ -2530,6 +2579,14 @@ pub(super) fn parse_subject_application( ("that player", true), tag::<_, _, OracleError<'_>>("that player may"), ), + // CR 608.2c: "that opponent" is the same anaphoric back-reference as + // "that player" with the noun narrowed — `parse_event_context_ref` already + // maps it (via `parse_attacked_opponent_event_ref`), and the + // `relative_player_scope` dispatch below resolves it exactly as it does + // "that player" (e.g. to `ScopedPlayer` inside a villainous choice, + // Sycorax Commander's "That opponent discards all the cards in their + // hand"). Longest-match: the `may` form precedes the bare one. + value(("that opponent", true), tag("that opponent may")), value(("the player", true), tag("the player may")), value( ("that attacking player", false), @@ -2537,6 +2594,7 @@ pub(super) fn parse_subject_application( ), value(("the attacking player", false), tag("the attacking player")), value(("that player", false), tag("that player")), + value(("that opponent", false), tag("that opponent")), value(("the player", false), tag("the player")), ))) .parse(lower.as_str()); @@ -2615,7 +2673,24 @@ pub(super) fn parse_subject_application( // bare player subject (e.g., "you phase out", "you draw a card"). The // imperative resolvers map `TargetFilter::Controller` → the ability's // controller player at resolution time. - if lower == "you" { + // + // The "you may " form is the CONTROLLER's own permission grant + // ("you may cast sorcery spells as though they had flash" — Teferi, Time + // Raveler [+1]; "you may look at face-down creatures you don't control any + // time" — Lumbering Laundry). It completes the may-modal family that + // already covers every OTHER player subject ("that player may", "they may", + // "its controller may", "its owner may", "'s controller may"). + // + // Unlike those siblings this does NOT set `is_optional` (CR 608.2d, the + // "effect offers a choice" rule, does not apply): the permission itself IS + // the opt-in — the granted static is what the player may later use — so + // marking the ability optional would prompt a redundant yes/no before a + // grant that asks nothing of its controller. `swallow_check`'s + // `Optional_YouMay` exemption records the same reading. + if all_consuming(alt((tag::<_, _, OracleError<'_>>("you may"), tag("you")))) + .parse(lower.as_str()) + .is_ok() + { return Some(SubjectApplication { affected: TargetFilter::Controller, target: None, @@ -2842,8 +2917,15 @@ pub(super) fn parse_subject_application( // {2}" (Smothering Tithe, Mind Whip); both must set `is_optional` so // `lower_subject_predicate_ast` marks the lowered ability optional and // `resolve_they_pronoun`'s existing player/object dispatch is unchanged. + // CR 608.2k: a trailing distributive "each" on an already-plural pronoun + // ("They each deal damage equal to their power to target creature an + // opponent controls") is emphasis, not a second axis — the predicate grammar + // owns the per-object application. Same reading as + // `oracle_static/anthem.rs::strip_trailing_distributive_each` takes for + // multi-subject static lists. Longest form first. if let Ok((_, is_optional)) = all_consuming(alt(( value(true, tag::<_, _, OracleError<'_>>("they may")), + value(false, tag("they each")), value(false, tag("they")), ))) .parse(lower.as_str()) @@ -2956,7 +3038,147 @@ pub(super) fn parse_subject_application( return subject_filter_application(TargetFilter::ParentTarget, false); } - None + // CR 611.2c: a single effect may name SEVERAL subjects sharing one + // predicate. Runs LAST: every conjunct phrasing that reaches here has + // already declined every single-subject arm above, so this arm only ever + // converts a `None` (which issue #6965 used to widen to `TargetFilter::Any`) + // into a bound union. + parse_conjoined_subject_application(TextPair::new(subject, lower.as_str()), ctx) +} + +/// CR 611.2c: parse `" and [and …]"` into the +/// UNION of its conjuncts. +/// +/// CR 611.2c settles the semantics — "If a single continuous effect has parts +/// that modify the characteristics or changes the controller of any objects and +/// other parts that don't, the set of objects each part applies to is determined +/// independently" — so a shared predicate applies to each named subject on its +/// own terms. `TargetFilter::Or` is that union, and it is the same shape +/// `oracle_static/anthem.rs` already emits for the static-ability form of this +/// construction (Sylvan Advocate → `Or[SelfRef, Typed(Creature+Land, You)]`). +/// +/// Each conjunct is parsed by [`parse_subject_application`] itself, so the +/// conjunct grammar IS the single-subject grammar — no phrase list, no per-card +/// arm — and recursion on the right-hand side gives N-ary lists for free. +/// Covers "it and Zombies you control" (Wand of Orcus), "you and planeswalkers +/// you control" (Eon Frolicker), "you and each permanent you control" (Faith's +/// Shield), and the "you and permanents you control" form that previously had +/// its own hardcoded literal arm. +/// +/// Fails closed unless EVERY conjunct is a plain, non-targeting subject filter +/// (see [`conjunct_subject_filter`]). Distributive lists ("you and target +/// opponent EACH draw a card") decline by construction: the trailing "each …" +/// leaves the last conjunct unparseable. Their per-player semantics are not a +/// union and belong to the distributive grammar, not here. +fn parse_conjoined_subject_application( + subject: TextPair<'_>, + ctx: &mut ParseContext, +) -> Option { + // Word-boundary scan for the conjunction, so "and" inside a conjunct's own + // noun phrase cannot split mid-word. + let (before, _, after) = nom_primitives::scan_preceded(subject.lower, |input| { + value((), tag::<_, _, OracleError<'_>>("and ")).parse(input) + })?; + // `scan_preceded` hands back the post-match remainder, so the conjunction + // itself is already consumed by the combinator — the two offsets below just + // project its result onto the paired original-case view. + let left = subject.split_at(before.len()).0.trim_end(); + let right = subject + .split_at(subject.lower.len() - after.len()) + .1 + .trim_start(); + if left.is_empty() || right.is_empty() { + return None; + } + + // Parse the conjuncts against a TENTATIVE context and commit it only on + // success. `parse_subject_application` takes `&mut ParseContext` and several + // of its arms record state on it (pronoun antecedents, relative player + // scope); leaking those from a conjunct probe that then DECLINES would + // silently change how the caller re-parses the same clause. Mirrors + // `try_parse_multi_target_damage_chain`'s tentative-context discipline. + let mut tentative = ctx.clone(); + let left_filter = conjunct_subject_filter(left, &mut tentative)?; + // Recurse first so "A and B and C" unions all three; fall back to treating + // the whole remainder as one conjunct ("Zombies you control"). + let right_filter = parse_conjoined_subject_application(right, &mut tentative) + .map(|application| application.affected) + .or_else(|| conjunct_subject_filter(right, &mut tentative))?; + *ctx = tentative; + + Some(SubjectApplication { + // `merge_or_filters` flattens, so a three-way list is one `Or` of three + // filters rather than an `Or` nested inside an `Or`. + affected: merge_or_filters(left_filter, right_filter), + target: None, + multi_target: None, + inherits_parent: false, + is_optional: false, + }) +} + +/// The filter for one conjunct of a compound subject, or `None` when that +/// conjunct is not a plain non-targeting subject. +/// +/// Rejected, deliberately (issue #6965 — these must fail closed rather than +/// widen): +/// * a conjunct that TARGETS ("you and target opponent …") needs its own +/// target slot, which one shared subject phrase cannot express; +/// * a conjunct carrying a cardinality or a `may` modal belongs to the +/// targeting grammar for the same reason; +/// * a conjunct that is not UNIONABLE — see [`filter_is_unionable`]. +fn conjunct_subject_filter(conjunct: TextPair<'_>, ctx: &mut ParseContext) -> Option { + let application = parse_subject_application(conjunct.original, ctx)?; + let plain = application.target.is_none() + && application.multi_target.is_none() + && !application.is_optional + && filter_is_unionable(&application.affected); + plain.then_some(application.affected) +} + +/// Issue #6965: true when `filter` is a self-contained subject DESCRIPTION — +/// one the runtime evaluates by matching an object or player against it, which +/// is the only channel a `TargetFilter::Or` union has. +/// +/// Deliberately an allowlist with a fail-CLOSED wildcard, so a future +/// `TargetFilter` variant is rejected from unions until someone decides it +/// belongs. Two classes are excluded, for two different reasons: +/// +/// * **Non-discriminating filters.** `TargetFilter::Any` matches +/// unconditionally (`game/filter.rs`), and a fully default `TypedFilter` +/// is what the type-phrase parsers hand back when they recognised nothing +/// in particular. Both are legitimate results for a WHOLE subject +/// elsewhere (the bare-"players" arm above deliberately yields the default +/// `TypedFilter`), but as a CONJUNCT they are indistinguishable from a +/// failed parse — unioning one re-widens the whole subject, reproducing +/// the pre-fix fail-open inside an `Or` wrapper. Model of Unity ("you and +/// each opponent WHO VOTED FOR A CHOICE YOU VOTED FOR may scry 2") is the +/// worked example: its restrictive relative clause is not modelled, so the +/// conjunct collapses to the default filter and `Or[Controller, ]` +/// would let every player scry. +/// +/// * **Event-context anaphors** (`TriggeringSource`, `ParentTarget`, …). +/// These resolve through the TARGET/binding channel, not by object +/// matching — `game/filter.rs::filter_inner_for_object` maps every one of +/// them to `false` by design. Unioning one produces an `Or` whose branch is +/// inert, so the effect silently applies to only PART of the printed +/// subject. Wand of Orcus ("it and Zombies you control gain deathtouch") +/// is exactly this: the Zombies branch applies and the equipped creature's +/// does not. That is still a misparse, so it fails closed here. Carrying an +/// anaphor conjunct correctly needs the primary-subject + chained +/// `sub_ability` split that +/// `try_parse_source_and_other_restriction_clause` already uses for +/// " and up to N other target creatures", not a filter union. +fn filter_is_unionable(filter: &TargetFilter) -> bool { + match filter { + TargetFilter::Typed(typed) => *typed != TypedFilter::default(), + // Static player scopes (CR 109.5 / CR 102.2): "you", "an opponent", + // "each player". + TargetFilter::Controller | TargetFilter::Opponent | TargetFilter::AllPlayers => true, + // A nested union is already made of unionable conjuncts by construction. + TargetFilter::Or { .. } => true, + _ => false, + } } pub(super) fn parse_leading_subject_application( @@ -9053,4 +9275,168 @@ mod tests { "must not fall through to Unimplemented" ); } + + // --- issue #6965: fail-closed subject binding + general compound subjects --- + + /// CR 611.2c: a compound subject applies to the UNION of its conjuncts. + /// + /// Building-block level, three real phrasings across three axes — one arm, + /// no per-card branch: + /// * PLAYER + typed filter — Eon Frolicker; + /// * PLAYER + quantified typed filter — Faith's Shield; + /// * player SCOPE + property-qualified typed filter — Detection Tower. + /// + /// All three fail on the pre-fix parser, which had a single compound arm + /// hardcoded to the literal phrase "you and permanents you control". + #[test] + fn compound_subject_parses_to_union_of_conjuncts() { + for (subject, expected) in [ + ( + // Eon Frolicker. + "you and planeswalkers you control", + vec![ + TargetFilter::Controller, + TargetFilter::Typed( + TypedFilter::default() + .with_type(TypeFilter::Planeswalker) + .controller(ControllerRef::You), + ), + ], + ), + ( + // Faith's Shield (fateful hour). + "you and each permanent you control", + vec![ + TargetFilter::Controller, + TargetFilter::Typed( + TypedFilter::default() + .with_type(TypeFilter::Permanent) + .controller(ControllerRef::You), + ), + ], + ), + ( + // Detection Tower. + "your opponents and creatures your opponents control with hexproof", + vec![ + TargetFilter::Typed(TypedFilter::default().controller(ControllerRef::Opponent)), + TargetFilter::Typed( + TypedFilter::default() + .with_type(TypeFilter::Creature) + .controller(ControllerRef::Opponent) + .properties(vec![FilterProp::WithKeyword { + value: crate::types::keywords::Keyword::Hexproof, + }]), + ), + ], + ), + ] { + let mut ctx = ParseContext::default(); + let application = parse_subject_application(subject, &mut ctx) + .unwrap_or_else(|| panic!("{subject:?} must bind to a subject")); + assert_eq!( + application.affected, + TargetFilter::Or { filters: expected }, + "{subject:?} must union its conjuncts" + ); + // A compound SUBJECT declares no target slot of its own. + assert!(application.target.is_none(), "{subject:?} does not target"); + } + } + + /// Issue #6965: a conjunct that is an event-context ANAPHOR resolves through + /// the target/binding channel, not by object matching + /// (`game/filter.rs::filter_inner_for_object` maps it to `false`). Unioning + /// one yields an `Or` whose anaphor branch is inert, so the grant applies to + /// only PART of the printed subject while still reporting as supported. It + /// must fail closed instead — Wand of Orcus, "it and Zombies you control". + #[test] + fn compound_subject_declines_an_anaphor_conjunct() { + let mut ctx = ParseContext::default(); + // Reach-guard: the OTHER conjunct parses fine on its own, so the decline + // below is caused by the anaphor and not by a broken right-hand side. + assert!( + parse_subject_application("Zombies you control", &mut ctx).is_some(), + "the typed conjunct must parse on its own" + ); + assert!( + parse_subject_application("it and Zombies you control", &mut ctx).is_none(), + "an anaphor conjunct must fail closed, not produce a half-inert union" + ); + } + + /// The generalized arm must reproduce the literal `"you and permanents you + /// control"` arm it replaced, byte for byte (Lazotep Plating, Veil of + /// Summer, Surge of Salvation, Dawn's Truce, ...). + #[test] + fn compound_subject_reproduces_the_replaced_literal_arm() { + let mut ctx = ParseContext::default(); + let application = parse_subject_application("you and permanents you control", &mut ctx) + .expect("the previously hardcoded phrase must still bind"); + let (permanents, rest) = parse_target("all permanents you control"); + assert!(rest.trim().is_empty()); + assert_eq!( + application.affected, + TargetFilter::Or { + filters: vec![TargetFilter::Controller, permanents], + } + ); + } + + /// Issue #6965: conjuncts that TARGET, carry a cardinality, or carry a + /// `may` modal are not a shared-predicate union — they must fail closed + /// rather than be widened into one. + /// + /// "you and target opponent each draw a card" is the distributive form: it + /// declares its own target slot and acts per player. Unioning it would both + /// drop the target slot and misapply the predicate. + #[test] + fn compound_subject_declines_targeting_and_distributive_conjuncts() { + for subject in [ + "you and target opponent each", + "you and target creature's controller", + "you and each opponent who voted for a choice you voted for may", + ] { + let mut ctx = ParseContext::default(); + assert!( + parse_subject_application(subject, &mut ctx).is_none(), + "{subject:?} must fail closed, not widen into a union" + ); + } + } + + /// Issue #6965 — the headline regression. A subject the grammar cannot bind + /// must produce an honest `Effect::Unimplemented`, NEVER a filter that + /// matches every permanent. + /// + /// Fixture is By Elspeth's Command mode 2, VERBATIM. `"It perpetually"` is + /// the real stranded-adverb shape: `find_predicate_start` splits at the verb + /// `gets`, leaving the Alchemy permanence marker on the subject side, which + /// no subject arm binds. Before the fix this clause emitted a static with + /// `affected: TargetFilter::Any` — the grant landed on every permanent. + #[test] + fn unbindable_subject_fails_closed_instead_of_going_board_wide() { + const CLAUSE: &str = "It perpetually gets +1/+1 and gains vigilance"; + + let mut ctx = ParseContext::default(); + // Reach-guard: prove the subject really is unbindable, so the assertion + // below exercises the fail-closed path and not some other arm. + assert!( + parse_subject_application("It perpetually", &mut ctx).is_none(), + "\"It perpetually\" must be an unbindable subject" + ); + + let effect = super::super::parse_effect(CLAUSE); + let Effect::Unimplemented { name, description } = &effect else { + // The pre-fix output was a `GenericEffect` whose static carried + // `affected: TargetFilter::Any` — a board-wide P/T + keyword grant. + panic!("an unbindable subject must lower to a gap, got {effect:?}"); + }; + assert_eq!(name, UNBOUND_SUBJECT_GAP); + assert_eq!( + description.as_deref(), + Some(CLAUSE), + "the gap must quote the WHOLE printed clause, subject included" + ); + } } diff --git a/crates/engine/src/parser/oracle_effect/tests.rs b/crates/engine/src/parser/oracle_effect/tests.rs index 587f70633e..62ea126b5c 100644 --- a/crates/engine/src/parser/oracle_effect/tests.rs +++ b/crates/engine/src/parser/oracle_effect/tests.rs @@ -413,25 +413,28 @@ fn two_target_fight_pump_keeps_both_slots_and_buffs_slot_zero() { } } -/// #4751 review (matthewevans): part 1's sentence-bounding drops only a STRAY -/// `Effect::unimplemented("you")` head from Life at Stake — it does NOT remove a -/// functional chooser. "You and target creature's controller each secretly -/// choose a number" is unimplemented on BOTH `main` and this branch: the real -/// mechanic is a `Choose { NumberRange }` (with the exile + lose-life tail), -/// byte-identical either way. On `main` the leading "You" was split off its "and" -/// by a coincidental " loses " in a LATER sentence into a bare -/// `Unimplemented { name: "you", description: "You" }`; bounding the verb scan to -/// the first sentence stops that coincidental split, so the chain now heads at -/// the real `Choose` node instead of the stray "you" fragment. This pins that the -/// card's actual choose-a-number mechanic survives (no regression) — the -/// controller was never a *parsed* chooser to lose (that stays a pre-existing gap -/// on both, orthogonal to this PR). -#[test] -fn life_at_stake_keeps_choose_mechanic_and_drops_only_the_stray_you_stub() { +/// CR 109.4 + CR 115.1 + CR 608.2c + CR 608.2d: Life at Stake — "You and target +/// creature's controller each secretly choose a number 0 or greater." +/// +/// The compound subject's second conjunct names its player THROUGH an announced +/// object target, so the parse must produce three things, not one: +/// 1. a `TargetOnly { creature }` head declaring the CR 115.1 target slot the +/// possessive reference (and the later "exile that creature" anaphor) read; +/// 2. a `Choose { NumberRange }` whose chooser is the printed controller +/// ("you", CR 109.5 — the unscoped resolver default); +/// 3. a SECOND `Choose { NumberRange }` bound to a DISTINCT chooser via +/// `player_scope: ParentObjectTargetController` (CR 109.4). +/// +/// The two choosers being distinct is the whole mechanic — one `Choose`, or two +/// that both prompt the caster, is the bug. Predecessor of this test pinned a +/// single `Choose` head while the controller was an unparsed chooser; that gap +/// is now closed. +#[test] +fn life_at_stake_binds_both_number_choosers_to_distinct_players() { use crate::types::ability::ChoiceType; - fn collect(a: &AbilityDefinition, out: &mut Vec) { - out.push((*a.effect).clone()); + fn collect<'a>(a: &'a AbilityDefinition, out: &mut Vec<&'a AbilityDefinition>) { + out.push(a); if let Some(s) = a.sub_ability.as_deref() { collect(s, out); } @@ -446,22 +449,47 @@ fn life_at_stake_keeps_choose_mechanic_and_drops_only_the_stray_you_stub() { let parsed = parse_oracle_text(text, "Life at Stake", &[], &["Instant".to_string()], &[]); let ability = parsed.abilities.first().expect("expected a spell ability"); - // The real mechanic — Choose a number — heads the chain (not a stray - // `Unimplemented("you")` fragment). - assert!( - matches!( - &*ability.effect, - Effect::Choose { - choice_type: ChoiceType::NumberRange { .. }, - .. - } - ), - "Life at Stake must head at a NumberRange Choose, not a stray you-stub; got {:#?}", - ability.effect + // CR 115.1: the announced creature is the ability's target — declared by a + // slot-only head so the possessive chooser and the "that creature" exile + // anaphor both have something to resolve against. + let Effect::TargetOnly { target } = &*ability.effect else { + panic!( + "Life at Stake must head at a TargetOnly declaring the creature target, got {:#?}", + ability.effect + ); + }; + assert_eq!( + *target, + TargetFilter::Typed(TypedFilter::creature()), + "the declared target must be the announced creature" ); - let mut effects = Vec::new(); - collect(ability, &mut effects); + let mut links = Vec::new(); + collect(ability, &mut links); + + // Both halves of the compound subject choose a number, and their choosers + // are DIFFERENT players: the printed controller (no scope) and the targeted + // creature's controller (CR 109.4). + let choosers: Vec> = links + .iter() + .filter(|link| { + matches!( + &*link.effect, + Effect::Choose { + choice_type: ChoiceType::NumberRange { .. }, + .. + } + ) + }) + .map(|link| link.player_scope.clone()) + .collect(); + assert_eq!( + choosers, + vec![None, Some(PlayerFilter::ParentObjectTargetController)], + "both conjuncts must choose a number, bound to distinct choosers" + ); + + let effects: Vec<&Effect> = links.iter().map(|link| &*link.effect).collect(); assert!( effects.iter().any(|e| matches!( e, @@ -479,9 +507,124 @@ fn life_at_stake_keeps_choose_mechanic_and_drops_only_the_stray_you_stub() { assert!( !effects .iter() - .any(|e| matches!(e, Effect::Unimplemented { name, .. } if name == "you")), - "the stray Unimplemented(\"you\") head must be gone: {effects:#?}" + .any(|e| matches!(e, Effect::Unimplemented { .. })), + "no clause of the choose-a-number sentence may fall back to Unimplemented: {effects:#?}" + ); +} + +/// CR 109.4 + CR 115.1 + CR 608.2c: the possessive-actor second-subject axis is +/// a CLASS, not Life at Stake's card. The same "you and target <filter>'s +/// controller each <body>" shape must distribute a body that DOES carry a +/// recipient slot, binding it through the effect's own field rather than +/// `player_scope` — and the announced target must be declared exactly once. +#[test] +fn possessive_actor_compound_subject_distributes_a_recipient_bearing_body() { + let ability = parse_effect_chain( + "You and target creature's controller each draw a card.", + AbilityKind::Spell, ); + + let Effect::TargetOnly { target } = &*ability.effect else { + panic!( + "expected a TargetOnly target declaration, got {:#?}", + ability.effect + ); + }; + assert_eq!(*target, TargetFilter::Typed(TypedFilter::creature())); + + let you = ability + .sub_ability + .as_deref() + .expect("expected the caster half"); + match &*you.effect { + Effect::Draw { target, .. } => assert_eq!(*target, TargetFilter::OriginalController), + other => panic!("expected the caster half to Draw, got {other:?}"), + } + assert_eq!( + you.player_scope, None, + "a recipient-bearing body binds \"you\" on the effect, not as a fan-out" + ); + + let them = you + .sub_ability + .as_deref() + .expect("expected the possessive half"); + match &*them.effect { + Effect::Draw { target, .. } => assert_eq!(*target, TargetFilter::ParentTargetController), + other => panic!("expected the possessive half to Draw, got {other:?}"), + } + assert_eq!( + them.player_scope, None, + "a recipient-bearing body must not ALSO fan out — that would double-apply" + ); +} + +/// CR 109.4 + 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`. +#[test] +fn recipient_less_body_binds_a_chosen_player_conjunct_by_scope() { + let parsed = parse_oracle_text( + "Choose an opponent. You and that player each sacrifice a creature.", + "Infernal Offering", + &[], + &["Sorcery".to_string()], + &[], + ); + let ability = parsed.abilities.first().expect("expected a spell ability"); + + let you = ability + .sub_ability + .as_deref() + .expect("expected the caster half after the Choose"); + assert!( + matches!(&*you.effect, Effect::Sacrifice { .. }), + "the caster half must sacrifice, got {:#?}", + you.effect + ); + assert_eq!(you.player_scope, None, "\"you\" is the unscoped default"); + + let them = you + .sub_ability + .as_deref() + .expect("expected the chosen-player half"); + assert!( + matches!(&*them.effect, Effect::Sacrifice { .. }), + "the chosen-player half must sacrifice, got {:#?}", + them.effect + ); + assert_eq!( + them.player_scope, + Some(PlayerFilter::ChosenPlayer { index: 0 }), + "the chosen opponent must be the acting player of the second half" + ); +} + +/// 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 +/// flip a coin" (Mana Clash) / "… each secretly choose 1, 2, or 3" +/// (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() { + 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 + ); + assert_eq!( + ability.player_scope, None, + "{text:?} must not fabricate a fan-out scope" + ); + } } /// Recursively walk an ability chain (root effect + `sub_ability` + `else_ability`) diff --git a/crates/engine/src/parser/oracle_ir/ast.rs b/crates/engine/src/parser/oracle_ir/ast.rs index 5bc933e909..c95052cbc3 100644 --- a/crates/engine/src/parser/oracle_ir/ast.rs +++ b/crates/engine/src/parser/oracle_ir/ast.rs @@ -182,7 +182,24 @@ pub(crate) enum ClauseAst { #[derive(Debug, Clone, PartialEq, Eq, Serialize)] pub(crate) struct SubjectPhraseAst { - pub(crate) affected: TargetFilter, + /// CR 608.2c ("read the whole text and apply the rules of English to the + /// text"): the subject the predicate applies to, or `None` when the + /// sentence printed a subject the subject grammar could not bind. + /// + /// **`Option`, not a permissive default (issue #6965).** Both sites that + /// re-derive a subject phrase used to substitute `TargetFilter::Any` when + /// [`super::SubjectApplication`] could not be produced. `TargetFilter::Any` + /// matches unconditionally (`game/filter.rs`), so a parse FAILURE emitted a + /// 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 + /// [`EntersUnderSpec::UnboundAnaphor`]. + pub(crate) affected: Option, pub(crate) target: Option, pub(crate) multi_target: Option, pub(crate) inherits_parent: bool, diff --git a/crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__karn_legacy_reforged_ir.snap b/crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__karn_legacy_reforged_ir.snap index b1badd777f..775c9f7cbe 100644 --- a/crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__karn_legacy_reforged_ir.snap +++ b/crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__karn_legacy_reforged_ir.snap @@ -228,8 +228,8 @@ expression: "&ir" "parsed": { "effect": { "type": "Unimplemented", - "name": "can't", - "description": "can't be spent to cast nonartifact spells" + "name": "unbound_subject", + "description": "This mana can't be spent to cast nonartifact spells" }, "duration": null, "sub_ability": null, 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 index 35cb715289..f616eca2cf 100644 --- a/crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs +++ b/crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs @@ -291,19 +291,28 @@ fn intellectual_offering_second_draw_binds_to_chosen_opponent() { ); } -/// 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. +/// Regression guard for the fix above, restated by #6965. `inject_subject_target`'s +/// `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 `rewrite_recipient_on_link` has no arm for +/// (Token/Draw/Discard/Mill/Pump/GenericEffect only). +/// +/// This used to assert the clause survived as `GainLife { player: Controller }`, and +/// the comment conceded the gap in the same breath: the damaged player never gained +/// life. That is a half-applied effect the caster benefits from, and it counted as +/// SUPPORTED in coverage — the silent-misparse class #6965 exists to remove. It only +/// reached `GainLife` at all because the unbindable subject fell open. With the +/// fail-open gone the clause is an honest `unbound_subject` gap: still not playable, +/// but now visible to coverage instead of masquerading as a working trigger. +/// +/// The original intent is preserved and strengthened — the point was that a subject +/// the parser cannot resolve must never be laundered into a concrete recipient. An +/// `Unimplemented` gap satisfies that more completely than a `Controller` default did. +/// +/// Forward-red: binding "that player" to the damage-event player will red this test, +/// which is the intended prompt to assert the real two-recipient shape. #[test] -fn angel_of_destiny_combat_damage_gain_life_keeps_well_defined_recipient() { +fn angel_of_destiny_compound_subject_fails_closed_rather_than_half_applying() { let mut scenario = GameScenario::new(); scenario.at_phase(Phase::PreCombatMain); let creature = scenario @@ -327,16 +336,22 @@ fn angel_of_destiny_combat_damage_gain_life_keeps_well_defined_recipient() { .execute .as_ref() .expect("DamageDone trigger must have an execute body"); + let Effect::Unimplemented { name, description } = execute.effect.as_ref() else { + panic!( + "`you and that player each gain that much life` must fail closed rather than \ + bind half the clause to a recipient it cannot name, got {:?}", + execute.effect + ); + }; + assert_eq!( + name, "unbound_subject", + "the gap must name the SUBJECT as the unbound part — a different name means the \ + clause failed somewhere else and this test stopped covering the fail-closed path" + ); 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 + description + .as_deref() + .is_some_and(|text| text.contains("that player")), + "reach-guard: the gap must quote the conjunct it could not bind, got {description:?}" ); } diff --git a/crates/engine/tests/integration/life_at_stake_both_choosers_6965.rs b/crates/engine/tests/integration/life_at_stake_both_choosers_6965.rs new file mode 100644 index 0000000000..ad4cc04071 --- /dev/null +++ b/crates/engine/tests/integration/life_at_stake_both_choosers_6965.rs @@ -0,0 +1,151 @@ +//! Issue #6965 (follow-up): Life at Stake — *"You and target creature's +//! controller each secretly choose a number 0 or greater."* +//! +//! The compound subject's second conjunct TARGETS: it names a player through an +//! announced object. Unioning it into one subject filter would lose the target +//! binding, so the parser instead splits the shared predicate into two chained +//! halves and declares the announced creature with a slot-only +//! `Effect::TargetOnly` head (CR 115.1). The half for "you" is the unscoped +//! resolver default (CR 109.5); the half for "target creature's controller" +//! binds its acting player on the ABILITY via +//! `player_scope: ParentObjectTargetController` (CR 109.4), because +//! `Effect::Choose` carries no recipient field of its own. +//! +//! This test drives the REAL parse → cast → resolution pipeline and asserts the +//! only thing that matters at runtime: TWO number choices are raised, and the +//! second one prompts the TARGETED creature's controller, not the caster. +//! +//! Fail-on-revert: before the fix, the whole sentence lowered to +//! `Effect::Unimplemented { name: "unbound_subject" }` — no target slot, no +//! `NamedChoice` at all, so both assertions below fail. +//! +//! CR 109.4: only objects on the stack or battlefield have a controller — the +//! anchor "target creature's controller" reads. +//! CR 109.5: "you" on an object refers to that object's controller. +//! CR 115.1: targets are declared as the spell is put on the stack. +//! CR 601.2c: the caster announces a legal object for each target the spell +//! requires. +//! CR 608.2c: the controller follows the instructions in the order written. +//! CR 608.2d: a choice offered by a resolving spell is announced while applying +//! the effect. + +use engine::game::scenario::{GameScenario, P0, P1}; +use engine::types::ability::{ChoiceType, TargetRef}; +use engine::types::actions::GameAction; +use engine::types::game_state::CastPaymentMode; +use engine::types::game_state::WaitingFor; +use engine::types::mana::{ManaCost, ManaCostShard, ManaType, ManaUnit}; +use engine::types::phase::Phase; +use engine::types::player::PlayerId; + +/// Verbatim Oracle text (MTGJSON). A paraphrase can take a different parser +/// branch and go green while the real card stays broken. +const LIFE_AT_STAKE: &str = "You and target creature's controller each secretly choose a number 0 or greater. Then, reveal the chosen numbers. If your number was highest or tied for the highest, exile that creature. Each player who chose the highest number loses that much life."; + +#[test] +fn life_at_stake_prompts_the_caster_then_the_targets_controller() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + scenario.with_library_top(P0, &["Lib A", "Lib B", "Lib C"]); + scenario.with_library_top(P1, &["Lib D", "Lib E", "Lib F"]); + + // The announced target is a creature P1 controls, so "target creature's + // controller" is a player DIFFERENT from the caster — the discriminating + // setup. A caster-controlled creature would let a wrong `Controller` + // binding pass. + let victim = scenario.add_creature(P1, "Grizzly Bears", 2, 2).id(); + + let mut spell_builder = + scenario.add_spell_to_hand_from_oracle(P0, "Life at Stake", true, LIFE_AT_STAKE); + spell_builder.with_mana_cost(ManaCost::Cost { + generic: 0, + shards: vec![ManaCostShard::Black], + }); + let spell = spell_builder.id(); + scenario.with_mana_pool( + P0, + vec![ManaUnit::new(ManaType::Black, spell, false, vec![])], + ); + + let mut runner = scenario.build(); + let card_id = runner.state().objects[&spell].card_id; + runner + .act(GameAction::CastSpell { + object_id: spell, + card_id, + targets: vec![], + payment_mode: CastPaymentMode::Auto, + }) + .expect("casting Life at Stake must start"); + + // Every player prompted for a number, in the order the engine raised the + // prompts. This is the whole assertion surface: who chooses. + let mut number_choosers: Vec = Vec::new(); + + for _ in 0..64 { + match runner.state().waiting_for.clone() { + WaitingFor::TargetSelection { target_slots, .. } => { + // CR 115.1 + CR 601.2c: the possessive subject must have + // declared a real creature slot. Pre-fix there was none. + assert_eq!( + target_slots.len(), + 1, + "the announced creature must be exactly one target slot" + ); + assert!( + target_slots[0] + .legal_targets + .contains(&TargetRef::Object(victim)), + "P1's creature must be a legal target; got {:?}", + target_slots[0].legal_targets + ); + runner + .act(GameAction::SelectTargets { + targets: vec![TargetRef::Object(victim)], + }) + .expect("selecting P1's creature must succeed"); + } + WaitingFor::ManaPayment { .. } => { + runner + .act(GameAction::PassPriority) + .expect("mana payment must auto-finalize"); + } + WaitingFor::NamedChoice { + player, + choice_type, + options, + .. + } => { + if matches!(choice_type, ChoiceType::NumberRange { .. }) { + number_choosers.push(player); + } + let choice = options + .first() + .cloned() + .expect("a number choice must offer options"); + runner + .act(GameAction::ChooseOption { choice }) + .expect("answering the number choice must succeed"); + if number_choosers.len() == 2 { + break; + } + } + WaitingFor::Priority { .. } => { + if runner.act(GameAction::PassPriority).is_err() { + break; + } + } + _ => break, + } + } + + // CR 109.4 + CR 109.5: both conjuncts choose, and they are DIFFERENT + // players — the caster, then the targeted creature's controller. A single + // prompt (the pre-fix gap) or two prompts both aimed at P0 (an unbound + // chooser) both fail here. + assert_eq!( + number_choosers, + vec![P0, P1], + "Life at Stake must prompt the caster and then the target creature's controller" + ); +} diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index 2fa5487c13..26be00a6a9 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -723,6 +723,7 @@ mod leeching_sliver; mod leyline_taps_for_mana_repro; mod lictor_opponent_entered_this_turn; mod life_and_limb_sylvan_advocate; +mod life_at_stake_both_choosers_6965; mod lightning_dart_disjunctive_color_instead; mod liliana_dreadhorde_multi_dies; mod liliana_waker_cross_scope_decline; @@ -1242,6 +1243,7 @@ mod vanille_meld_optional_cost; mod vannifar_cloak_from_hand; mod veteran_bodyguard_tap_redirect; mod vohar_discard_drain; +mod wand_of_orcus_compound_subject_6965; mod weeping_angel_combat_prevention; mod wheel_and_deal; mod where_x_coverage_runtime; diff --git a/crates/engine/tests/integration/parker_luck.rs b/crates/engine/tests/integration/parker_luck.rs index f8127d9758..d0fd8e9f23 100644 --- a/crates/engine/tests/integration/parker_luck.rs +++ b/crates/engine/tests/integration/parker_luck.rs @@ -119,37 +119,50 @@ fn parker_luck_lose_parses_to_other_revealed_card_mana_value() { ); } -/// §6.5 HONEST-RED PIN (Daretti-INVERSE): Keen Duelist's reveal is single-subject -/// (`RevealTop { player: Any }`, no `multi_target`), so the B3 anchor-precondition -/// gate rewrites its `OtherRevealedCard`-bearing lose back to `Effect::Unimplemented` -/// (coverage `supported == false`) EVEN after the §2.5 combinator lands. Revert-red: -/// remove the §2.6 gate → the combinator makes this lose parse to `LoseLife` → KD -/// flips supported → this test reds. Reach-guards: the root is a RevealTop with NO -/// multi_target and the lose still chains the `put`, so the Unimplemented assertion -/// is not vacuous on a parse failure. +/// §6.5 HONEST-RED PIN, deepened by #6965. This test previously asserted that KD's +/// reveal parsed to a `RevealTop` root and only its lose clause was gapped. That root +/// parsed solely because the subject `you and target opponent each` fell open to +/// `RevealTop { player: Any }` — a filter that matches unconditionally, i.e. the exact +/// unsound binding #6965 removes. The old assertion named that filter in its own doc +/// comment, so it was pinning the fail-open, not surviving it. +/// +/// `target opponent` is a TARGETED player, and no `PlayerFilter` names one: +/// `PlayerFilter::Opponent` would make EVERY opponent reveal in multiplayer. So the +/// subject now fails closed and the whole trigger is an honest `unbound_subject` gap, +/// which is strictly more truthful than a reveal bound to "anyone". The lose clause is +/// unreachable behind it, so this no longer goes through `lose_node`. +/// +/// Forward-red: whoever teaches the parser to bind a targeted player (giving KD a real +/// `RevealTop` again) will red this test, which is the intended prompt to revisit the +/// §2.6 lose gate that the old assertion guarded. #[test] -fn keen_duelist_lose_stays_unimplemented_without_multiplayer_reveal() { +fn keen_duelist_targeted_opponent_subject_fails_closed() { let parsed = parse_keen_duelist(); - let root = parsed.triggers[0].execute.as_ref().unwrap(); + let root = parsed.triggers[0] + .execute + .as_ref() + .expect("reach-guard: KD's upkeep trigger must still carry an execute chain"); assert!( root.multi_target.is_none(), "reach-guard: Keen Duelist's single-subject reveal carries no multi_target" ); - let lose = lose_node(&parsed); - assert!( - matches!(&*lose.effect, Effect::Unimplemented { name, .. } if name == "lose"), - "the gate must keep KD's lose an honest Unimplemented gap, got {:?}", - lose.effect + let Effect::Unimplemented { name, description } = &*root.effect else { + panic!( + "`you and target opponent each ...` must fail closed rather than bind a \ + reveal to an unconditional filter, got {:?}", + root.effect + ); + }; + assert_eq!( + name, "unbound_subject", + "the gap must name the SUBJECT as the unbound part — a different name means the \ + clause failed somewhere else and this test stopped covering the fail-closed path" ); assert!( - matches!( - &*lose.sub_ability.as_ref().unwrap().effect, - Effect::ChangeZone { - destination: Zone::Hand, - .. - } - ), - "reach-guard: the chain still parsed the 'put into hand' sub (only the lose is gapped)" + description + .as_deref() + .is_some_and(|text| text.contains("target opponent")), + "reach-guard: the gap must quote the targeting conjunct that caused it, got {description:?}" ); } 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 new file mode 100644 index 0000000000..a507c58787 --- /dev/null +++ b/crates/engine/tests/integration/wand_of_orcus_compound_subject_6965.rs @@ -0,0 +1,183 @@ +//! Issue #6965: an unparseable subject must not become a board-wide effect. +//! +//! Two production-path regressions, one per half of the fix. +//! +//! **1. Fail closed.** Wand of Orcus — *"Whenever equipped creature attacks or +//! blocks, it and Zombies you control gain deathtouch until end of turn."* Both +//! subject-predicate sites that re-derive a subject used to substitute +//! `TargetFilter::Any` when the subject grammar returned `None`, and +//! `TargetFilter::Any` matches unconditionally (`game/filter.rs`). So the parse +//! FAILURE produced a BOARD-WIDE grant: every permanent the controller had — +//! lands and artifacts included — gained deathtouch. It now produces an honest +//! `Effect::Unimplemented` gap, and the runtime grants nothing. +//! +//! **2. Bind the compound subject.** Lazotep Plating — *"You and permanents you +//! control gain hexproof until end of turn."* This construction used to be +//! recognised by a single hardcoded literal arm matching exactly the phrase +//! `"you" + " and " + "permanents you control"`. It is now parsed by the general +//! CR 611.2c union arm, which parses each conjunct with the ordinary +//! single-subject grammar. This test is the regression guard for deleting the +//! literal arm: it fails if the generalization does not reproduce it. +//! +//! 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 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::card_type::CoreType; +use engine::types::identifiers::ObjectId; +use engine::types::keywords::Keyword; +use engine::types::mana::{ManaColor, ManaCost, ManaCostShard, ManaType, ManaUnit}; +use engine::types::phase::Phase; + +/// Verbatim Oracle text (MTGJSON). A paraphrase can take a different parser +/// branch and go green while the real card stays broken. +const WAND_OF_ORCUS: &str = "Whenever equipped creature attacks or blocks, it and Zombies you control gain deathtouch until end of turn.\nWhenever equipped creature deals combat damage to a player, create that many 2/2 black Zombie creature tokens.\nEquip {3}"; + +/// Verbatim Oracle text (MTGJSON), reminder text included. +const LAZOTEP_PLATING: &str = "Amass Zombies 1. (Put a +1/+1 counter on an Army you control. It's also a Zombie. If you don't control an Army, create a 0/0 black Zombie Army creature token first.)\nYou and permanents you control gain hexproof until end of turn. (You and they can't be the targets of spells or abilities your opponents control.)"; + +fn keywords(runner: &GameRunner, id: ObjectId) -> Vec { + runner.state().objects[&id].keywords.clone() +} + +#[test] +fn wand_of_orcus_unbindable_subject_grants_nothing_board_wide() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + + let host = scenario + .add_creature(P0, "Wandbearer", 2, 2) + .with_subtypes(vec!["Human", "Soldier"]) + .id(); + // The conjunct a PARTIAL union would reach: under an `Or` whose anaphor + // branch is inert, this Zombie gains deathtouch while the equipped creature + // does not — a half-applied grant that still reports as supported. That must + // not happen either. + let zombie = scenario + .add_creature(P0, "Shambler", 2, 2) + .with_subtypes(vec!["Zombie"]) + .id(); + let bear = scenario + .add_creature(P0, "Grizzly Bears", 2, 2) + .with_subtypes(vec!["Bear"]) + .id(); + // Not even a creature. Pre-fix, `TargetFilter::Any` reached lands too — the + // most visible symptom of the fail-open. + let land = scenario.add_basic_land(P0, ManaColor::Black); + + let wand = scenario + .add_creature_from_oracle(P0, "Wand of Orcus", 0, 1, WAND_OF_ORCUS) + .id(); + + 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. + { + let obj = runner.state_mut().objects.get_mut(&wand).unwrap(); + obj.card_types.core_types = vec![CoreType::Artifact]; + obj.card_types.subtypes = vec!["Equipment".to_string()]; + obj.base_card_types = obj.card_types.clone(); + obj.power = None; + obj.toughness = None; + obj.base_power = None; + obj.base_toughness = None; + obj.attached_to = Some(AttachTarget::Object(host)); + } + evaluate_layers(runner.state_mut()); + + runner.advance_to_combat(); + runner + .declare_attackers(&[(host, AttackTarget::Player(P1))]) + .expect("declare the equipped creature as an attacker"); + + // Reach-guard: the trigger really did fire and go on the stack. Without it + // the assertions below would pass vacuously on a card that never triggered. + assert_eq!( + runner.stack_names(), + vec!["Wand of Orcus".to_string()], + "the attack trigger must be on the stack, or nothing below is exercised" + ); + + runner.advance_until_stack_empty(); + runner.state_mut().layers_dirty.mark_full(); + evaluate_layers(runner.state_mut()); + + // The printed subject ("it and Zombies you control") carries an anaphor + // conjunct the union cannot bind, so the whole clause fails closed. Nothing + // is granted — most importantly, NOT everything. + for (id, label) in [ + (host, "the equipped creature"), + (zombie, "a Zombie you control"), + (bear, "an unrelated creature you control"), + (land, "a LAND you control"), + ] { + assert!( + !keywords(&runner, id).contains(&Keyword::Deathtouch), + "{label} must not gain deathtouch: the printed subject could not be \ + bound, so the clause is an honest gap (issue #6965 — it used to \ + become TargetFilter::Any and grant to every permanent)" + ); + } +} + +#[test] +fn lazotep_plating_grants_hexproof_to_the_union_of_both_conjuncts() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + + let ally = scenario + .add_creature(P0, "Grizzly Bears", 2, 2) + .with_subtypes(vec!["Bear"]) + .id(); + // The second conjunct is "permanents you control", not "creatures" — a land + // you control must be covered too. + let ally_land = scenario.add_basic_land(P0, ManaColor::White); + // Excluded by "you control". + let foe = scenario + .add_creature(P1, "Runeclaw Bear", 2, 2) + .with_subtypes(vec!["Bear"]) + .id(); + + let plating = scenario + .add_spell_to_hand_from_oracle(P0, "Lazotep Plating", true, LAZOTEP_PLATING) + .with_mana_cost(ManaCost::Cost { + generic: 1, + shards: vec![ManaCostShard::Blue], + }) + .id(); + scenario.with_mana_pool( + P0, + vec![ + ManaUnit::new(ManaType::Colorless, ObjectId(0), false, vec![]), + ManaUnit::new(ManaType::Blue, ObjectId(0), false, vec![]), + ], + ); + + let mut runner = scenario.build(); + runner.cast(plating).resolve(); + runner.state_mut().layers_dirty.mark_full(); + evaluate_layers(runner.state_mut()); + + // CR 611.2c: both named subjects are covered. + assert!( + keywords(&runner, ally).contains(&Keyword::Hexproof), + "a creature you control is inside \"permanents you control\" and must \ + gain hexproof" + ); + assert!( + keywords(&runner, ally_land).contains(&Keyword::Hexproof), + "a LAND you control is a permanent you control and must gain hexproof" + ); + // The negative arm is non-vacuous: the two positives above prove the grant + // resolved at all. + assert!( + !keywords(&runner, foe).contains(&Keyword::Hexproof), + "an opponent's permanent is excluded by \"you control\"" + ); +}