diff --git a/crates/engine/src/parser/oracle.rs b/crates/engine/src/parser/oracle.rs index 859f68fee2..b502c24bf8 100644 --- a/crates/engine/src/parser/oracle.rs +++ b/crates/engine/src/parser/oracle.rs @@ -1,3 +1,5 @@ +use std::borrow::Cow; + use crate::parser::oracle_nom::error::{OracleError, OracleResult}; use nom::branch::alt; use nom::bytes::complete::{tag, take_until, take_while}; @@ -1158,9 +1160,33 @@ fn item_replacement(item: &OracleItemIr) -> Option<&ReplacementDefinition> { } } -fn item_ability(item: &OracleItemIr) -> Option<&AbilityDefinition> { +/// CR 607.2d: the ability side of document-relation discovery. +/// +/// Returns `Cow` because the two spell-bearing node shapes own their definition +/// at different times. A pre-lowered item already holds an `AbilityDefinition` +/// and lends it out; an IR-native item holds only an `EffectChainIr` and owns no +/// definition at all until lowering builds one, so it must lower and hand back +/// the result owned. A plain `&AbilityDefinition` cannot express the second case +/// — there is nothing to borrow from — and a plain `AbilityDefinition` would +/// clone the first case at all seven call sites, most of which scan every item +/// on the card. +/// +/// This is where the trigger side's reasoning stops applying. +/// `item_trigger` keeps a borrow and pushes exhaustiveness down onto +/// `TriggerNodeIr::definition()`, because both of its shapes own a +/// `TriggerDefinition` — `TriggerNodeIr::Assembled` carries one directly. There +/// is no equivalent layer to push this obligation onto: a spell node's IR +/// payload is an `EffectChainIr`, not an enum of definition-owning +/// representations, so both shapes are named here. `_ => None` is safe for the +/// same reason it is safe there: every remaining variant is genuinely `None`. +/// +/// Lowering is the identity conversion `lower_oracle_ir` (the `Spell` arm) will +/// perform for the same item, so a relation predicate sees exactly the +/// definition the relation will later be applied to. +fn item_ability(item: &OracleItemIr) -> Option> { match &item.node { - OracleNodeIr::PreLoweredSpell(def) => Some(def), + OracleNodeIr::PreLoweredSpell(def) => Some(Cow::Borrowed(def)), + OracleNodeIr::Spell(effect_ir) => Some(Cow::Owned(lower_effect_chain_ir(effect_ir))), _ => None, } } @@ -1421,7 +1447,7 @@ fn detect_linked_choice_type_statics( } ) }); - let is_dig = item_ability(item).is_some_and(ability_chain_has_dig) + let is_dig = item_ability(item).is_some_and(|def| ability_chain_has_dig(&def)) || item_trigger(item) .and_then(|trigger| trigger.execute.as_deref()) .is_some_and(ability_chain_has_dig); @@ -1542,7 +1568,7 @@ fn chosen_subtype_kind_from_persisted_choice_items( items .iter() .filter_map(item_ability) - .find_map(chosen_subtype_kind_from_ability) + .find_map(|def| chosen_subtype_kind_from_ability(&def)) }) .or_else(|| { items @@ -1643,7 +1669,7 @@ fn detect_linked_choice_persisted_player( ) { let has_durable_reader = items.iter().any(|item| { item_static(item).is_some_and(static_references_source_chosen_player) - || item_ability(item).is_some_and(ability_references_source_chosen_player) + || item_ability(item).is_some_and(|def| ability_references_source_chosen_player(&def)) || item_trigger(item).is_some_and(trigger_references_source_chosen_player) }); if !has_durable_reader { @@ -1652,7 +1678,7 @@ fn detect_linked_choice_persisted_player( let choosers: Vec = items .iter() .filter(|item| { - item_ability(item).is_some_and(ability_chain_has_player_choice) + item_ability(item).is_some_and(|def| ability_chain_has_player_choice(&def)) || item_trigger(item) .and_then(|trigger| trigger.execute.as_deref()) .is_some_and(ability_chain_has_player_choice) @@ -1703,9 +1729,9 @@ fn detect_linked_choice_copy_chosen_host( items: &[OracleItemIr], relations: &mut Vec, ) { - let chooser = items - .iter() - .find(|item| item_ability(item).is_some_and(ability_is_as_enters_choose_permanent_gap)); + let chooser = items.iter().find(|item| { + item_ability(item).is_some_and(|def| ability_is_as_enters_choose_permanent_gap(&def)) + }); let copy_static = items.iter().find(|item| { item_static(item).is_some_and(|s| { s.modifications @@ -3075,12 +3101,12 @@ fn ability_is_active_player_punisher(def: &AbilityDefinition) -> bool { fn detect_active_player_punisher(items: &[OracleItemIr], relations: &mut Vec) { let Some(coerce) = items .iter() - .find(|item| item_ability(item).is_some_and(ability_is_active_player_coerce)) + .find(|item| item_ability(item).is_some_and(|def| ability_is_active_player_coerce(&def))) else { return; }; for item in items { - if item_ability(item).is_some_and(ability_is_active_player_punisher) { + if item_ability(item).is_some_and(|def| ability_is_active_player_punisher(&def)) { relations.push(DocumentRelationIr::ActivePlayerPunisher { coerce: coerce.id, punisher: item.id, @@ -3699,13 +3725,19 @@ impl<'a> DocEmitter<'a> { /// /// `take_last_spell` pops the emission-ordered spell stack, which equals /// `abilities.last_mut()` regardless of triggers/statics emitted in between. + /// + /// Reads the popped item through `lower_spell_node`, which accepts BOTH + /// spell node shapes. It must: `emit` pushes either shape onto + /// `spells_emitted` and `take_last_spell` pops it with no variant filter, so + /// a destructure that names one shape panics the parser the first time an + /// IR-native spell precedes a mutating line. fn mutate_last_spell(&mut self, f: impl FnOnce(&mut AbilityDefinition)) { let Some(item) = self.builder.take_last_spell() else { return; }; let OracleItemIr { source, node, .. } = item; - let OracleNodeIr::PreLoweredSpell(mut def) = node else { - unreachable!("take_last_spell returns only PreLoweredSpell items"); + let Some(mut def) = lower_spell_node(&node) else { + unreachable!("`spells_emitted` holds only spell nodes, and both spell shapes lower"); }; f(&mut def); self.reemit_spell(&source, def); @@ -5852,8 +5884,13 @@ pub(crate) fn parse_oracle_ir( node: base_node, .. } = base_item; - let OracleNodeIr::PreLoweredSpell(mut base) = base_node else { - unreachable!("pop_last_spell returns only PreLoweredSpell items"); + // Both spell node shapes, via the shared reader: + // `pop_last_spell` pops `spells_emitted`, which `emit` + // fills from either shape with no variant filter. + let Some(mut base) = lower_spell_node(&base_node) else { + unreachable!( + "`spells_emitted` holds only spell nodes, and both spell shapes lower" + ); }; // Save the base ability's continuation chain in else_ability // so the engine can run it when the condition is NOT met. diff --git a/crates/engine/src/parser/oracle_ir/doc.rs b/crates/engine/src/parser/oracle_ir/doc.rs index 10c158a5b1..98ed7bd8a5 100644 --- a/crates/engine/src/parser/oracle_ir/doc.rs +++ b/crates/engine/src/parser/oracle_ir/doc.rs @@ -914,14 +914,13 @@ impl OracleDocBuilder { // dispatch loop baked in. // // Match is EXHAUSTIVE over `OracleNodeIr` (no `_`), mirroring `emit`'s - // printed-slot match above: a future node variant — or the still - // never-constructed `Spell` IR variant once a later commit emits it — - // must fail to compile here until its slot behavior is decided, rather - // than being silently skipped (which would mis-index every later - // trigger/ability). `Trigger` is destructured to its one payload variant - // for the same reason: adding an IR-native trigger payload must break - // this match, because the stamp targets `execute`, which a decomposition - // does not have until lowering builds it. + // printed-slot match above: a future node variant must fail to compile + // here until its slot behavior is decided, rather than being silently + // skipped (which would mis-index every later trigger/ability). + // `Trigger` is destructured to its one payload variant for the same + // reason: adding an IR-native trigger payload must break this match, + // because the stamp targets `execute`, which a decomposition does not + // have until lowering builds it. let mut trigger_slot = 0usize; let mut ability_slot = 0usize; for item in self.items.values_mut() { @@ -938,12 +937,29 @@ impl OracleDocBuilder { stamp_retained_printed_slot(def, ability_slot, PrintedItemKind::Ability); ability_slot += 1; } - // No retain modification can reach these today: the `*` IR variants - // are never constructed in unit 3a, and the remaining categories do - // not carry a copy-except body. Left explicit (not `_`) so a new - // slot-bearing node is a compile error, per the note above. - OracleNodeIr::Spell(_) - | OracleNodeIr::Static(_) + // CR 707.9a printed slots count PRINTED abilities. The set of + // variants that advance `ability_slot` here must equal the set + // `lower_oracle_ir` (`oracle.rs`) pushes into `result.abilities`, + // which is in turn the set `emit` pushes onto `spells_emitted` + // above: both spell shapes. Three matches over one set. + // + // Advances WITHOUT stamping, on purpose — do not try to make this + // arm symmetric with the arm above. An IR-native spell body has no + // `AbilityDefinition` until lowering builds one, and + // `stamp_retained_printed_slot` takes `&mut AbilityDefinition`, so + // there is nothing here to stamp. The printed slot is consumed all + // the same: skipping the advance stamps every LATER ability one + // slot low, silently binding a copy's "except it has this ability" + // to the wrong ability. + OracleNodeIr::Spell(_) => { + ability_slot += 1; + } + // Neither stamps nor counts: `RetainPrinted*FromSource` exists only + // for the trigger and ability categories (CR 707.9a), so these + // carry no slot to rewrite and consume neither counter this walk + // maintains. Left explicit (not `_`) so a new slot-bearing node is + // a compile error, per the note above. + OracleNodeIr::Static(_) | OracleNodeIr::PreLoweredStatic(_) | OracleNodeIr::Replacement(_) | OracleNodeIr::PreLoweredReplacement(_) diff --git a/crates/engine/tests/integration/ir_spell_node_readers.rs b/crates/engine/tests/integration/ir_spell_node_readers.rs new file mode 100644 index 0000000000..4ed67ff36f --- /dev/null +++ b/crates/engine/tests/integration/ir_spell_node_readers.rs @@ -0,0 +1,255 @@ +//! Readers of an IR-native spell item (`OracleNodeIr::Spell`) — Plan 05b T7. +//! +//! That node shape has a live producer today (the instant/sorcery +//! prevent-damage recognizer in `parser/oracle.rs`), but several of its readers +//! were written when it had none, and each assumed the pre-lowered shape. The +//! defects they carried are silent by construction: a mis-stamped printed slot +//! and a missed document relation both produce a card that parses successfully +//! into the wrong thing, so full-pool byte-identity cannot see them. +//! +//! These tests drive the real `parse_oracle_text` pipeline and assert on the +//! parsed AST, which is the layer the defects live in. + +use engine::parser::oracle::ParsedAbilities; +use engine::parser::parse_oracle_text; +use engine::types::ability::{ + AbilityDefinition, ContinuousModification, ControllerRef, Effect, FilterProp, TargetFilter, +}; + +/// Every CR 707.9a printed-ability slot a card's copy-except clauses resolve to. +fn retained_ability_slots(parsed: &ParsedAbilities) -> Vec { + fn from_mods(mods: &[ContinuousModification], out: &mut Vec) { + for m in mods { + if let ContinuousModification::RetainPrintedAbilityFromSource { + source_ability_index, + } = m + { + out.push(*source_ability_index); + } + } + } + fn walk(def: &AbilityDefinition, out: &mut Vec) { + match def.effect.as_ref() { + Effect::CopySpell { + additional_modifications, + .. + } + | Effect::CopyTokenOf { + additional_modifications, + .. + } + | Effect::BecomeCopy { + additional_modifications, + .. + } => from_mods(additional_modifications, out), + Effect::AddPendingEntersModifications { modifications } => { + from_mods(modifications, out) + } + _ => {} + } + if let Some(sub) = def.sub_ability.as_deref() { + walk(sub, out); + } + } + let mut out = Vec::new(); + for ability in &parsed.abilities { + walk(ability, &mut out); + } + out +} + +/// CR 707.9a: a printed slot is consumed by the printed ability that occupies +/// it, whether or not that ability has a definition yet to stamp. +/// +/// `OracleDocBuilder::finish()` resolves every "…except it has this ability" +/// clause by walking items in source order and counting each category +/// separately. An IR-native spell node holds only an effect chain, so there is +/// nothing to stamp into — but it still occupies an ability slot, because +/// `lower_oracle_ir` pushes it into `result.abilities` exactly like a +/// pre-lowered one does. +/// +/// DISCRIMINATING: with the `Spell` arm parked in `finish()`'s no-op list this +/// reads `0`, and the copy grafts the FIRST printed ability — the prevention +/// spell — instead of itself. +/// +/// Line 1 is the shape the only live `OracleNodeIr::Spell` producer emits: an +/// instant/sorcery prevention line. Line 2 is a synthetic activated ability, +/// which is the sole source of `RetainPrintedAbilityFromSource`. No printing +/// pairs the two today — which is exactly why this defect had no corpus witness +/// and why the full-pool byte gate is silent on it. +#[test] +fn an_ir_native_spell_consumes_the_printed_ability_slot_it_occupies() { + let parsed = parse_oracle_text( + "Prevent all damage that would be dealt to you this turn.\n{2}: Create a token that's a copy of target creature, except it has this ability.", + "Probe", + &[], + &["Instant".to_string()], + &[], + ); + + // Reach-guard: both printed abilities must be present, or the slot + // assertion below is vacuous. + assert_eq!( + parsed.abilities.len(), + 2, + "expected the prevention spell and the copy ability, got {:?}", + parsed.abilities + ); + assert!( + matches!( + parsed.abilities[0].effect.as_ref(), + Effect::PreventDamage { .. } + ), + "the first printed ability must be the IR-native prevention spell, got {:?}", + parsed.abilities[0].effect + ); + + assert_eq!( + retained_ability_slots(&parsed), + vec![1], + "the copy-except clause must resolve to printed slot 1 — the IR-native \ + prevention spell consumed slot 0 despite carrying no definition to stamp" + ); +} + +// --------------------------------------------------------------------------- +// CR 607.2d document relations and the `item_ability` reader. +// +// `item_ability` is the ability side of cross-item relation discovery. It used +// to return `Option<&AbilityDefinition>` and recognize exactly one spell node +// shape; it now returns `Option>` and recognizes +// both, lowering the IR-native shape on demand because that shape owns no +// definition to borrow. +// +// A relation that stops being discovered fails SILENTLY — no error, no panic, +// just a card that parses to its unrelated line-local shape, with the symptom +// surfacing on a different card from the one that was edited. The borrowed path +// therefore needs an explicit non-regression witness across the signature +// change. +// +// SCOPE, stated honestly: this witnesses the BORROWED arm. The new IR-native +// arm has no witness, because no text can currently reach it — the single live +// `Spell` producer is a prevent-damage spell line, and no relation predicate +// matches a prevention chain. Verified by construction (removing the new arm +// leaves these tests green) and by attempting to synthesize a card that pairs +// the two, which the line splitter refuses to produce. The arm is forward +// hardening for T8, when `Spell` producers rise from 1 to ~14. +// --------------------------------------------------------------------------- + +/// Verified against Scryfall 2026-07-27 (`cards/named?exact=Siren's Call`). +const SIRENS_CALL: &str = "Cast this spell only during an opponent's turn, before attackers are declared.\nCreatures the active player controls attack this turn if able.\nAt the beginning of the next end step, destroy all non-Wall creatures that player controls that didn't attack this turn. Ignore this effect for each creature the player didn't control continuously since the beginning of the turn."; + +/// The delayed punisher's destroyed set, plus whether its exemption sibling is +/// still hanging off the delayed ability. +fn punisher_destroy_target(parsed: &ParsedAbilities) -> (TargetFilter, bool) { + for ability in &parsed.abilities { + if let Effect::CreateDelayedTrigger { effect, .. } = ability.effect.as_ref() { + if let Effect::DestroyAll { target, .. } = effect.effect.as_ref() { + return (target.clone(), effect.sub_ability.is_some()); + } + } + } + panic!("expected a delayed DestroyAll punisher ability, got {parsed:?}"); +} + +fn typed_controllers(filter: &TargetFilter, out: &mut Vec>) { + match filter { + TargetFilter::Typed(tf) => out.push(tf.controller.clone()), + TargetFilter::And { filters } | TargetFilter::Or { filters } => { + filters.iter().for_each(|f| typed_controllers(f, out)) + } + TargetFilter::Not { filter } => typed_controllers(filter, out), + _ => {} + } +} + +fn typed_props(filter: &TargetFilter, out: &mut Vec) { + match filter { + TargetFilter::Typed(tf) => out.extend(tf.properties.iter().cloned()), + TargetFilter::And { filters } | TargetFilter::Or { filters } => { + filters.iter().for_each(|f| typed_props(f, out)) + } + TargetFilter::Not { filter } => typed_props(filter, out), + _ => {} + } +} + +/// CR 102.1 + CR 603.7c + CR 608.2c: the `ActivePlayerPunisher` relation pairs +/// the mass-attack coerce clause with its sibling delayed punisher, then +/// rebinds the punisher's "that player controls" anaphor from the line-local +/// `You` default to `ActivePlayer`. +/// +/// Siren's Call is the witness because BOTH sides of the relation are ability +/// items — two printed abilities on one instant — so a single card drives +/// `item_ability` twice, once per predicate. +/// +/// DISCRIMINATING: the rebind is reachable ONLY through the relation, and the +/// relation is discovered only if `item_ability` returns a definition for both +/// participating items. Blind the reader on either side and this sees the +/// line-local `You` — the wrong player, which destroys the caster's own +/// creatures. +#[test] +fn active_player_punisher_relation_survives_the_item_ability_reader() { + let parsed = parse_oracle_text( + SIRENS_CALL, + "Siren's Call", + &[], + &["Instant".to_string()], + &[], + ); + let (target, exemption_sibling_present) = punisher_destroy_target(&parsed); + + let mut controllers = Vec::new(); + typed_controllers(&target, &mut controllers); + assert!( + !controllers.is_empty(), + "reach-guard: the destroyed set must carry at least one typed node to rebind, got {target:?}" + ); + assert!( + controllers + .iter() + .all(|c| *c == Some(ControllerRef::ActivePlayer)), + "the punisher's destroyed set must be rebound to ActivePlayer by the CR 607.2d relation; \ + a `You` here means the relation was not discovered, got {controllers:?}" + ); + + // CR 302.6 + CR 508.1a: the same relation folds the continuous-control + // exemption into the destroyed set and CONSUMES the redundant sibling. Both + // halves are asserted so a partial application cannot pass. + let mut props = Vec::new(); + typed_props(&target, &mut props); + assert!( + props.contains(&FilterProp::ControlledContinuouslySinceTurnBegan), + "the exemption must be folded into the destroyed set as a filter prop, got {props:?}" + ); + assert!( + !exemption_sibling_present, + "the redundant exemption sibling must be consumed once folded" + ); +} + +/// Reach-guard for the assertion above: `ActivePlayer` must not be something +/// the punisher line parses to on its own. With no sibling coerce clause there +/// is no relation to discover, so the anaphor keeps its line-local `You`. +/// +/// Without this, the test above would still pass against an `item_ability` that +/// returned `Some` for every item, or against a parser that hardcoded +/// `ActivePlayer` into the punisher line. +#[test] +fn the_punisher_line_alone_keeps_its_line_local_controller() { + let parsed = parse_oracle_text( + "At the beginning of the next end step, destroy all non-Wall creatures that player controls that didn't attack this turn.", + "Probe", + &[], + &["Instant".to_string()], + &[], + ); + let (target, _) = punisher_destroy_target(&parsed); + let mut controllers = Vec::new(); + typed_controllers(&target, &mut controllers); + assert!( + !controllers.contains(&Some(ControllerRef::ActivePlayer)), + "with no coerce sibling there is no relation, so the anaphor must NOT be \ + rebound to ActivePlayer, got {controllers:?}" + ); +} diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index b6e3460020..954f6bbe45 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -241,6 +241,7 @@ mod integration_bending; mod integration_landfall; mod interaction_contract; mod invoke_calamity_free_cast; +mod ir_spell_node_readers; mod issue_1005_suffer_the_past; mod issue_1007_fractal_harness_attach; mod issue_1008_korvold_sacrifice_triggers; diff --git a/scripts/prelowered-ratchet.txt b/scripts/prelowered-ratchet.txt index af991b803b..148876cb2c 100644 --- a/scripts/prelowered-ratchet.txt +++ b/scripts/prelowered-ratchet.txt @@ -12,7 +12,7 @@ # real IR nodes. Every tranche that converts a producer must lower its number # in the same commit, which is what makes the burn-down visible in `git log` # on this one file. -crates/engine/src/parser/oracle.rs 20 +crates/engine/src/parser/oracle.rs 16 crates/engine/src/parser/oracle_class.rs 6 # --- infrastructure: NOT burn-down targets ----------------------------------