Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 5 additions & 21 deletions crates/engine/src/game/effects/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7511,31 +7511,15 @@ fn effect_consumes_event_context_amount(effect: &Effect) -> bool {
consumes
}

/// Walks every `QuantityRef` reachable through `quantity`'s composition forms
/// and reports whether any satisfies `pred`. Single traversal authority for the
/// resolution-local back-reference predicates, so a new `QuantityExpr`
/// composition form is threaded in exactly one place instead of once per
/// predicate.
/// Delegates to `QuantityExpr::any_ref` (`types/ability.rs`) — the single
/// traversal authority, relocated there so the parser layer can consult it
/// too without reaching into game internals. Kept as a thin free-function
/// wrapper here since this module's call sites (below) predate the move.
fn quantity_expr_any_ref(
quantity: &QuantityExpr,
pred: &mut dyn FnMut(&QuantityRef) -> bool,
) -> bool {
match quantity {
QuantityExpr::Ref { qty } => pred(qty),
QuantityExpr::Offset { inner, .. }
| QuantityExpr::ClampMin { inner, .. }
| QuantityExpr::Multiply { inner, .. }
| QuantityExpr::DivideRounded { inner, .. } => quantity_expr_any_ref(inner, pred),
QuantityExpr::Sum { exprs } | QuantityExpr::Max { exprs } => {
exprs.iter().any(|expr| quantity_expr_any_ref(expr, pred))
}
QuantityExpr::UpTo { max } => quantity_expr_any_ref(max, pred),
QuantityExpr::Power { exponent, .. } => quantity_expr_any_ref(exponent, pred),
QuantityExpr::Difference { left, right } => {
quantity_expr_any_ref(left, pred) || quantity_expr_any_ref(right, pred)
}
QuantityExpr::Fixed { .. } => false,
}
quantity.any_ref(pred)
}

fn quantity_expr_references_event_context_amount(quantity: &QuantityExpr) -> bool {
Expand Down
8 changes: 7 additions & 1 deletion crates/engine/src/game/engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15605,6 +15605,12 @@ mod stage2_injector_tests {
// at all, both re-read and sha256-confirmed in place. (`engine.rs`'s entry
// has since moved to `:11619` — see the item-2 note on that entry below;
// `scoped_library_search.rs:452` still has not moved.)
// Valakut #7047 fix round: `:9458 ⇒ :9442`, −16, and only that
// effects/mod.rs entry moved relative to current main. The
// `QuantityExpr::any_ref` relocation replaces the 16-line traversal match
// with a delegation; it sits above this producer and below the first two.
// The merge tree therefore retains main's first two coordinates
// (`:6177`/`:6254`) and shifts this one by −16 to `:9442`.
//
// ⚠ THIS ROW FAILS IN CI BEFORE IT FAILS LOCALLY, and that is not a bug in the
// row. CI checks out `refs/pull/<n>/merge` — this branch merged with CURRENT
Expand All @@ -15619,7 +15625,7 @@ mod stage2_injector_tests {
// and is offered as a follow-up rather than taken unannounced mid-review.
"game/effects/mod.rs:6177".to_string(),
"game/effects/mod.rs:6254".to_string(),
"game/effects/mod.rs:9458".to_string(),
"game/effects/mod.rs:9442".to_string(),
// UNMOVED across the rebase, and that is itself evidence the SET did not
// move: a census that had gained or lost a producer would not leave this
// entry both byte-identical AND at the same coordinate.
Expand Down
22 changes: 22 additions & 0 deletions crates/engine/src/game/triggers_ordering_parity_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,28 @@ const DOCUMENTED_OVER_PROMPT: &[&str] = &[
// MISaligned copies go to a different graveyard and are correctly prompted by
// the same conflict — see the impl-report soundness note.)
"skyfisher spider",
// ---- unprofiled-effect commute, batch: the effect node has no RW profile, so
// the fail-closed `RwProfile::conservative()` catch-all prompts ----
// dies-batch optional PutOnTopOrBottom{SelfRef, chooser: Controller} (AST
// measured): each co-departing copy moves ONLY its own graveyard card (CR 603.6c
// first-zone check) to its OWN owner's library, so the members' writes are
// disjoint — owner-misaligned copies route to different libraries entirely, and
// same-owner copies contend only for the top/bottom slot, where identical
// same-name cards commute up to relabeling. The "may" and the top-or-bottom pick
// are resolution-time choices (CR 603.5) made by the batch's one controller in
// either order: production ordering groups are partitioned per-controller
// (`trigger_order_controller` / `begin_trigger_ordering`, triggers.rs; CR 603.3b
// — each player orders only the triggers THEY control, cross-controller
// placement being APNAP-fixed, not chosen), the premise the §5 batch rows model
// as `ControllerUniformity::Uniform` — single-controller BY CONSTRUCTION, not an
// artifact of the Phase+OnlyDuringYourTurn privacy predicate (same-event S2
// only). The prompt is conservative: `PutOnTopOrBottom` is unprofiled — it
// lands in `RwProfile::conservative()` (maximal reads/writes + fail-closed
// `reads_member_bound`), refusing batch-T1 and tripping the feed rows, so no
// in-scope recognizer proves the self-scoped move context-free (Valakut
// Exploration misparse-fix fixture regen surfaced this card in the sweep
// corpus; same adjudication as the #7031 branch).
"arashin sovereign",
];

/// Batch-depth GENUINE order-dependence (kept SEPARATE from the same-event
Expand Down
59 changes: 57 additions & 2 deletions crates/engine/src/parser/oracle_effect/assembly.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ use crate::parser::oracle_nom::bridge::nom_on_lower;
use crate::parser::oracle_nom::error::OracleError;
use crate::types::ability::{
AbilityCondition, AbilityCost, AbilityDefinition, AbilityKind, CastFromZoneDriver,
CastingPermission, ControllerRef, Effect, PlayerFilter, QuantityExpr, StaticCondition,
SubAbilityLink, TapStateChange, TargetFilter,
CastingPermission, ControllerRef, Effect, PlayerFilter, QuantityExpr, QuantityRef,
StaticCondition, SubAbilityLink, TapStateChange, TargetFilter,
};
use crate::types::game_state::TargetSelectionConstraint;
use crate::types::zones::Zone;
Expand Down Expand Up @@ -1218,6 +1218,41 @@ impl AssemblyEnv {
}
}

/// Coverage-honesty marker (issue #7046): a `DamageEachPlayer` reading "that
/// much/that many" (`Ref(EventContextAmount)`) chained IMMEDIATELY after a mass
/// zone move cannot be faithfully executed today. CR 608.2c requires the
/// completed move's TOTAL for every recipient, but the runtime hand-off
/// (`install_previous_effect_counts_by_player`) publishes a per-OWNER table that
/// `DamageEachPlayer`'s per-recipient resolution consults first — each opponent
/// reads their OWN swept-card count (0 in the Valakut Exploration native
/// pattern). Emitting the parsed shape would be silently wrong at runtime,
/// which is strictly worse than an honest residual gap (the Winnowing-class
/// precedent in `imperative.rs`). Scoped to the immediately-chained pairing
/// because only the next resolution step can see the table (any intermediate
/// effect clears it), and only `DamageEachPlayer` resolves per-recipient —
/// scalar consumers (Draw / DealDamage-to-object / Mill / LoseLife /
/// SearchLibrary / Token) read max-over-owners, which equals the true total for
/// every corpus pool (single-owner by CR 400.3). DELETE this gate when #7046
/// provides the completed-sweep scalar-total channel; the T1 zero-delta
/// assertions and the P2 marker assertion are the tripwires that force that
/// deletion to be a conscious, tested change.
pub(crate) const MASS_MOVE_TOTAL_DAMAGE_GAP: &str = "mass_move_total_damage";

/// CR 608.2c: does `effect`'s amount expression read `EventContextAmount`
/// ("that much"/"that many")? Leaf helper for the `MASS_MOVE_TOTAL_DAMAGE_GAP`
/// gate above. Mirrors the game-side
/// `quantity_expr_references_event_context_amount` (`game/effects/mod.rs`),
/// which this parser module cannot call directly — the parser layer never
/// reaches into game internals; both consult the single traversal authority
/// `QuantityExpr::any_ref`.
fn damage_amount_reads_event_context(effect: &Effect) -> bool {
let mut reads = false;
effect.for_each_quantity_expr(&mut |quantity| {
reads |= quantity.any_ref(&mut |qty| matches!(qty, QuantityRef::EventContextAmount));
});
reads
}

pub(crate) fn assemble_effect_chain(ir: &EffectChainIr) -> AbilityDefinition {
let kind = ir.kind;
let continuation_kind = ir.continuation_kind.unwrap_or(AbilityKind::Spell);
Expand Down Expand Up @@ -1950,6 +1985,26 @@ pub(crate) fn assemble_effect_chain(ir: &EffectChainIr) -> AbilityDefinition {
// boundary→link authority (`oracle_ir::ast::sub_link_after_boundary`),
// which the referent walk in `oracle_effect::mod` also consults.
def.sub_link = sub_link_after_boundary(prev_boundary);
// Coverage-honesty gate (issue #7046, see `MASS_MOVE_TOTAL_DAMAGE_GAP`
// doc comment above `assemble_effect_chain`): a `DamageEachPlayer`
// reading "that much/that many" chained IMMEDIATELY after a mass zone
// move cannot be faithfully executed today. `prev.sub_ability.is_none()`
// matches the runtime scope exactly — only the immediately-next
// resolution step can see the per-owner count table
// (`install_previous_effect_counts_by_player` clears it for any other
// consumer), and only `DamageEachPlayer` resolves per-recipient.
if let Some(prev) = defs.last() {
if prev.sub_ability.is_none()
&& matches!(&*prev.effect, Effect::ChangeZoneAll { .. })
&& matches!(&*def.effect, Effect::DamageEachPlayer { .. })
&& damage_amount_reads_event_context(&def.effect)
{
*def.effect = Effect::unimplemented(
MASS_MOVE_TOTAL_DAMAGE_GAP,
clause_ir.source.fragment().unwrap_or_default(),
);
}
}
Comment thread
matthewevans marked this conversation as resolved.
// CR 615.5: A "(When|Whenever|If) damage [from a <type> source] is
// prevented this way, …" rider is printed as its own sentence but is not
// an independent instruction — its "this way" back-reference binds to the
Expand Down
45 changes: 43 additions & 2 deletions crates/engine/src/parser/oracle_effect/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,14 @@ pub(crate) fn is_bare_object_pronoun(text: &str) -> bool {
)
}

/// CR 608.2c (rules of English): the plural subset of the bare object-pronoun
/// family. A plural antecedent (a set-valued noun phrase) binds these and
/// only these — a singular "it" must never be captured by a plural pool
/// (see `ParseContext::plural_object_pronoun_ref`).
pub(crate) fn is_bare_plural_object_pronoun(text: &str) -> bool {
matches!(text, "them" | "themselves")
}

/// CR 608.2c anaphora: substitute `replacement` for the FIRST bare object
/// pronoun word ("it"/"them"/…) in `body`, leaving any later pronouns intact so
/// a downstream "and it gains …" still chains to the now-declared target via
Expand Down Expand Up @@ -31545,6 +31553,12 @@ pub(crate) fn parse_effect_chain_ir(
// self-reference so a `"that creature"` copy-token anaphor in any
// chunk of an Aura/bestow card remaps to the enchanted host.
host_self_reference: ctx.host_self_reference.clone(),
// CR 608.2c + CR 608.2k + CR 406.6: the plural-anaphor antecedent
// introduced by the trigger's intervening-if ("if there are cards
// exiled with ~") is a property of the whole trigger body, not of
// an individual chunk — the "put THEM …" sweep chunk needs it to
// bind the bare plural pronoun to the linked-exile pool.
plural_object_pronoun_ref: ctx.plural_object_pronoun_ref.clone(),
// CR 608.2k: propagate the enclosing ability's exile-cost source
// zone so a `"the exiled card"` anaphor in any effect chunk
// disambiguates to `CostPaidObject` (Jhoira of the Ghitu).
Expand Down Expand Up @@ -33352,7 +33366,28 @@ fn try_parse_put_zone_change_parts(
(false, target_text)
};
let up_to = parse_up_to_one_target_prefix(before.lower) || choice_count.is_some();
let (target, _) = parse_target(target_text);
// CR 608.2c + CR 608.2k + CR 406.6 + CR 607.2a: a bare plural
// anaphor whose antecedent is the trigger's linked-exile pool
// ("if there are cards exiled with ~, put THEM into their owner's
// graveyard") is a mandatory sweep of the whole pool — a mass move
// (Bomat Courier's `ChangeZoneAll` shape, emitted through the
// existing mass branch below), never the single-object
// resolution-choice path. When `plural_object_pronoun_ref` is
// `None` — every card outside the class — behavior is unchanged
// (the ctx-free `parse_target` fallback below).
let plural_pool = if is_bare_plural_object_pronoun(&target_text.to_ascii_lowercase()) {
ctx.plural_object_pronoun_ref
.clone()
.filter(|pool| matches!(pool, TargetFilter::ExiledBySource))
} else {
None
};
let pool_bound = plural_pool.is_some();
let is_mass = is_mass || pool_bound;
let target = match plural_pool {
Some(pool) => pool,
None => parse_target(target_text).0,
};
let multi_origin_zones = put_hand_graveyard_origin_zones(before.lower);
let target = match multi_origin_zones.as_ref() {
Some(zones) => add_put_multi_origin_constraint(target, zones),
Expand Down Expand Up @@ -33445,7 +33480,13 @@ fn try_parse_put_zone_change_parts(
// graveyard"); "into your graveyard" then yields `origin: None`,
// matching the hand branch and letting the injected reveal target
// drive the move uniformly across the whole destination class.
let origin = if is_tracked_anaphor || multi_origin_zones.is_some() {
let origin = if pool_bound {
// CR 607.2a: the linked pool's members are in exile by
// definition, so the sweep scans Exile (Bomat Courier's proven
// `origin: Some(Exile)` shape — `resolve_all` with
// `origin: None` would fall back to a Battlefield scan).
Some(Zone::Exile)
} else if is_tracked_anaphor || multi_origin_zones.is_some() {
None
} else {
let origin_text = format!("{}{}", before.lower, after.lower);
Expand Down
10 changes: 10 additions & 0 deletions crates/engine/src/parser/oracle_ir/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,16 @@ pub(crate) struct ParseContext {
/// set this to `TriggeringSource` so "Whenever you cast a spell, put it ..."
/// moves the spell on the stack, not the trigger source or a parent target.
pub object_pronoun_ref: Option<TargetFilter>,
/// CR 608.2c (rules of English — number agreement) + CR 608.2k + CR 406.6:
/// Antecedent for bare PLURAL object pronouns ("them"/"themselves") in the
/// current trigger body, introduced by a plural noun phrase in the trigger's
/// intervening-if ("if there are cards exiled with ~, put THEM …" → the
/// linked-exile pool). Deliberately separate from the singular
/// `object_pronoun_ref`: a plural antecedent must never capture a singular
/// "it", whose antecedent is the nearer chained object (River Song's Diary:
/// "choose one of them at random. You may cast IT" — "it" is the chosen card,
/// not the pool).
pub plural_object_pronoun_ref: Option<TargetFilter>,
/// Accumulated diagnostics for the current card parse (Phase 52, D-07).
/// Replaces thread-local oracle_warnings accumulator.
pub diagnostics: Vec<OracleDiagnostic>,
Expand Down
19 changes: 19 additions & 0 deletions crates/engine/src/parser/oracle_ir/snapshot_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2334,6 +2334,25 @@ fn edgewall_innkeeper() {
insta::assert_json_snapshot!("edgewall_innkeeper_lowered", &lowered);
}

// ---------------------------------------------------------------------------
// Valakut Exploration (existential exiled-with intervening-if + plural-pool
// sweep + chained "that much" damage — CR 603.4 + CR 406.6 + CR 607.2a +
// CR 608.2c/608.2k)
// ---------------------------------------------------------------------------

#[test]
fn valakut_exploration() {
let (ir, lowered) = parse_two_layer_with_keywords(
"Landfall — Whenever a land you control enters, exile the top card of your library. You may play that card for as long as it remains exiled.\nAt the beginning of your end step, if there are cards exiled with this enchantment, put them into their owner's graveyard, then this enchantment deals that much damage to each opponent.",
"Valakut Exploration",
&["Landfall"],
&["Enchantment"],
&[],
);
insta::assert_json_snapshot!("valakut_exploration_ir", &ir);
insta::assert_json_snapshot!("valakut_exploration_lowered", &lowered);
}

// ---------------------------------------------------------------------------
// Bomat Courier (exile + activated with complex costs)
// ---------------------------------------------------------------------------
Expand Down
Loading
Loading