From 45e2411d8e112e9c3fa6c07f05422de2898051bc Mon Sep 17 00:00:00 2001 From: matthewevans Date: Sun, 12 Jul 2026 21:04:28 -0700 Subject: [PATCH] fix(parser): bind the where-X QUANTITY channel or fail honestly (CR 107.3c) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sibling of #5706, which fixed the P/T channel of the same defect. A "where X is " clause DEFINES the value of X in an ability's text (CR 107.3c) — the controller does not choose it. When the parser could not represent that definition it fabricated a well-typed placeholder holding the raw Oracle text and carried on: QuantityRef::Variable { name: "" } -> game/quantity.rs:1841, the non-"X" Variable arm -> state.last_named_choice -> .unwrap_or(0) Every such node renders as a supported dynamic quantity in the coverage report while resolving to 0 — or, worse, to an unrelated number left behind by some earlier "choose a number". Porcuparrot dealt 0 damage. Abby, Merciless Soldier made 0 tokens. Arek, False Goldwarden drained for nothing. All reported as supported. #5706 fixed the P/T half of two of these functions and left the quantity half fabricating in the same functions; it also missed the CopyTokenOf where-X path 20 lines above the one it fixed. This removes the fabrication at all five sites (oracle_effect/lower.rs x2, mana.rs, token.rs x2). The where-X rewriter family is now Option-returning and routes failure through the existing unbound_where_x -> Effect::unimplemented("where_x_binding", ...) seam — no new machinery. The explicit `return None` for "the number of times ..." now surfaces as an honest red instead of being re-fabricated by the caller. Binds four expression classes whose typed home AND live resolver both already existed; zero new enum variants: "~'s intensity" / "this spell's intensity" -> Intensity { scope: Source } "the chosen number" -> ChosenNumber "the amount of mana spent to cast it/her" -> ManaSpentToCast { Total } "the amount of {S} spent to cast ..." -> ManaSpentToCast { FromSource } CR 107.4h: {S} refers to mana of any type produced by a snow source spent to pay a cost. FromSource filters payment-time source snapshots, so the snow-ness of the producing source is read at payment time. Two sibling-cluster duplicates retired in the process: - oracle_static/anthem.rs carried a private where-X intensity combinator that was never wired into the shared interpreter. It now delegates; the grammar lives once, in the shared parse_quantity_ref. - parse_mana_spent_to_cast_amount re-listed the self-anaphora pronouns instead of using parse_mana_spent_self_subject. That duplicate list is exactly why Toph, Greatest Earthbender ("...spent to cast her") fell through. Full-pool measured on 35,396 faces (base c281a86abb). Probe anchored on the `name` key: the export is internally tagged and PtValue::Variable collides on the variant name ({"type":"Variable","value":"*"}). Non-vacuity control: name:"X" = 647 faces. Census re-derived as 93 faces / 74 expressions (supersedes the 94/75 measured on an older base — drift, not contradiction). DEFECT LEDGER (93 faces; gains and reds separate, never netted): 13 gained binding (a real typed quantity) 80 gained honest-red (an Effect::unimplemented gap — harvest yield) 0 still fabricating Invariant: zero raw-text QuantityRef::Variable{name} survive pool-wide. Aliases preserved: "X" (647 faces), "guessed" (The Toymaker's Trap). The leaked "count" sentinel (Abby) was a real fabrication leak, not a legitimate channel instance — it is cleared by the ManaSpentToCast bind. COLLATERAL LEDGER (10 faces outside the defect set; the binds went into the shared combinator, so they also reached inline "equal to its intensity" and "for each {S} spent" contexts): 9 gains — and three were SILENT WRONG BEHAVIOR, not mere gaps: Expel the Interlopers destroyed EVERY creature regardless of power (the chosen-number filter bound was empty) Search for Glory always gained exactly 1 life (Fixed{1}) Jarsyl, Dark Age Scion had a swallowed mana-value constraint 1 regression — Piece It Together. Its Draw{1} genuinely worked before and is now absorbed into a gap. This is a GENUINE FUNCTIONAL REGRESSION, not a lying-green conversion, and it is named as such. Cause attributed by toggle probe, not inference: disabling only the intensity arm brings the Draw back. Once "~'s intensity" parses, the sentence becomes a parseable conditional-"instead" whose effect is unsupported, and the line merges into the preceding gap. The bind did not create that defect — it made one more card reach the CR 614 "instead" defect already under active repair. Absorbed on unit economics. No pinning test is added: that would cement the wrong output. It is recorded as a recovery witness on the CR 614 work instead — when branch lowering lands, the Draw must return. Tests: 6 parser witnesses + 3 runtime witnesses, each WATCHED FAILING before the fix (runtime reds produced by disabling the bind arms, then restored). The first draft of the convert witness was VACUOUS — green pre-fix, because its Draw phrasing never reached the fabricating path; it was replaced with Porcuparrot's real DealDamage shape, whose pre-fix failure prints the fabrication verbatim. Co-authored-by: matthewevans --- .../src/parser/oracle_effect/imperative.rs | 3 +- .../engine/src/parser/oracle_effect/lower.rs | 293 ++++++++++++------ .../engine/src/parser/oracle_effect/mana.rs | 31 +- crates/engine/src/parser/oracle_effect/mod.rs | 19 +- .../src/parser/oracle_effect/sequence.rs | 6 +- .../engine/src/parser/oracle_effect/tests.rs | 162 +++++++++- .../engine/src/parser/oracle_effect/token.rs | 31 +- .../engine/src/parser/oracle_nom/quantity.rs | 84 +++++ crates/engine/src/parser/oracle_quantity.rs | 72 +++-- .../engine/src/parser/oracle_static/anthem.rs | 31 +- crates/engine/tests/integration/main.rs | 1 + .../where_x_quantity_channel_binds.rs | 141 +++++++++ 12 files changed, 691 insertions(+), 183 deletions(-) create mode 100644 crates/engine/tests/integration/where_x_quantity_channel_binds.rs diff --git a/crates/engine/src/parser/oracle_effect/imperative.rs b/crates/engine/src/parser/oracle_effect/imperative.rs index b8d1ee1bbb..2941669160 100644 --- a/crates/engine/src/parser/oracle_effect/imperative.rs +++ b/crates/engine/src/parser/oracle_effect/imperative.rs @@ -8241,7 +8241,8 @@ pub(super) fn parse_counter_ast(text: &str, lower: &str) -> Option Option }) } +/// CR 107.3c: A "where X is …" clause DEFINES the value of X in the ability's +/// text — the controller does not choose it. Bind every X reference in the +/// quantity channel to the typed quantity the clause names. +/// +/// Returns `None` when the clause defines X but the parser cannot represent that +/// definition. That is a PARSE FAILURE and callers MUST surface it through +/// `Effect::unimplemented`; they must never fabricate a substitute value. +/// +/// This function previously fell back to +/// `QuantityRef::Variable { name: "" }`. That fallback was a +/// silent lie, and it is the quantity-channel twin of the `PtValue::Variable` +/// lie removed in the P/T channel: `game/quantity.rs` dispatches the non-`"X"` +/// `Variable` arm through `state.last_named_choice` and `.unwrap_or(0)`, so the +/// quantity read 0 — or, worse, an unrelated number left behind by some earlier +/// "choose a number" — while the raw text still rendered as a supported dynamic +/// quantity in the coverage report. Porcuparrot dealt 0 damage; Abby made 0 +/// tokens. Every such node was well-typed and completely dead. Honest failure is +/// the only correct answer here. +/// +/// Note that `None` is returned ONLY when the node actually carries an X +/// reference (bare `Variable("X")` or `CostXPaid`) that this clause was supposed +/// to bind. A node with no X reference is returned unchanged as `Some`, so an +/// unrepresentable where-X clause on an ability that never uses X cannot poison +/// that ability. pub(super) fn apply_where_x_quantity_expression( value: QuantityExpr, where_x_expression: Option<&str>, -) -> QuantityExpr { - match value { +) -> Option { + Some(match value { // CR 107.3i: Generic "X is N or more" condition parsing defaults to // CostXPaid for X-cost spells, but a surrounding "where X is ..." clause // is the more specific binding and must own every X reference in the @@ -7706,21 +7730,13 @@ pub(super) fn apply_where_x_quantity_expression( qty: QuantityRef::CostXPaid, } if where_x_expression.is_some() => { let expression = where_x_expression.expect("checked is_some above"); - parse_where_x_quantity_expression(expression).unwrap_or_else(|| QuantityExpr::Ref { - qty: QuantityRef::Variable { - name: expression.to_string(), - }, - }) + parse_where_x_quantity_expression(expression)? } QuantityExpr::Ref { qty: QuantityRef::Variable { name }, } if where_x_expression.is_some() && name.eq_ignore_ascii_case("X") => { let expression = where_x_expression.expect("checked is_some above"); - parse_where_x_quantity_expression(expression).unwrap_or_else(|| QuantityExpr::Ref { - qty: QuantityRef::Variable { - name: expression.to_string(), - }, - }) + parse_where_x_quantity_expression(expression)? } // CR 107.3i: "search ... for up to X ..., where X is …" wraps the X // count in `UpTo`. Recurse into `max` so the defining clause rewrites @@ -7728,20 +7744,20 @@ pub(super) fn apply_where_x_quantity_expression( // must bind X to the where-clause population, not stay at 0). `up_to` // re-asserts the non-nesting invariant. QuantityExpr::UpTo { max } => { - QuantityExpr::up_to(apply_where_x_quantity_expression(*max, where_x_expression)) + QuantityExpr::up_to(apply_where_x_quantity_expression(*max, where_x_expression)?) } QuantityExpr::Offset { inner, offset } => QuantityExpr::Offset { inner: Box::new(apply_where_x_quantity_expression( *inner, where_x_expression, - )), + )?), offset, }, QuantityExpr::ClampMin { inner, minimum } => QuantityExpr::ClampMin { inner: Box::new(apply_where_x_quantity_expression( *inner, where_x_expression, - )), + )?), minimum, }, QuantityExpr::Multiply { factor, inner } => QuantityExpr::Multiply { @@ -7749,7 +7765,7 @@ pub(super) fn apply_where_x_quantity_expression( inner: Box::new(apply_where_x_quantity_expression( *inner, where_x_expression, - )), + )?), }, QuantityExpr::DivideRounded { inner, @@ -7759,7 +7775,7 @@ pub(super) fn apply_where_x_quantity_expression( inner: Box::new(apply_where_x_quantity_expression( *inner, where_x_expression, - )), + )?), divisor, rounding, }, @@ -7767,29 +7783,51 @@ pub(super) fn apply_where_x_quantity_expression( exprs: exprs .into_iter() .map(|expr| apply_where_x_quantity_expression(expr, where_x_expression)) - .collect(), + .collect::>>()?, }, QuantityExpr::Max { exprs } => QuantityExpr::Max { exprs: exprs .into_iter() .map(|expr| apply_where_x_quantity_expression(expr, where_x_expression)) - .collect(), + .collect::>>()?, }, QuantityExpr::Difference { left, right } => QuantityExpr::Difference { - left: Box::new(apply_where_x_quantity_expression(*left, where_x_expression)), + left: Box::new(apply_where_x_quantity_expression( + *left, + where_x_expression, + )?), right: Box::new(apply_where_x_quantity_expression( *right, where_x_expression, - )), + )?), }, QuantityExpr::Power { base, exponent } => QuantityExpr::Power { base, exponent: Box::new(apply_where_x_quantity_expression( *exponent, where_x_expression, - )), + )?), }, other => other, + }) +} + +/// Bind an X-bearing quantity slot in place, recording an unrepresentable +/// where-X definition instead of fabricating one (CR 107.3c). +/// +/// This is the single authority for the "rewrite a quantity slot under a +/// where-X clause" operation: every call site in the where-X rewriter family +/// routes through it so that a failed bind is reported exactly once, in one +/// way — as `unbound`, which the caller converts to `Effect::unimplemented`. +/// Callers must never inspect the binding themselves or supply a default. +fn bind_where_x_quantity( + slot: &mut QuantityExpr, + where_x_expression: Option<&str>, + unbound: &mut Option, +) { + match apply_where_x_quantity_expression(slot.clone(), where_x_expression) { + Some(bound) => *slot = bound, + None => *unbound = where_x_expression.map(str::to_string), } } @@ -7818,7 +7856,7 @@ pub(super) fn apply_where_x_effect_expression( .. } | Effect::Incubate { count: amount } => { - *amount = apply_where_x_quantity_expression(amount.clone(), where_x_expression); + bind_where_x_quantity(amount, where_x_expression, &mut unbound_where_x); } Effect::Token { count, @@ -7826,7 +7864,7 @@ pub(super) fn apply_where_x_effect_expression( toughness, .. } => { - *count = apply_where_x_quantity_expression(count.clone(), where_x_expression); + bind_where_x_quantity(count, where_x_expression, &mut unbound_where_x); match ( apply_where_x_expression(power.clone(), where_x_expression), apply_where_x_expression(toughness.clone(), where_x_expression), @@ -7842,8 +7880,8 @@ pub(super) fn apply_where_x_effect_expression( // is …" binds the search count (Oreskos Explorer). Eldritch Evolution // binds the filter's `Cmc` bound when X appears in the card filter. Effect::SearchLibrary { filter, count, .. } | Effect::Seek { filter, count, .. } => { - *filter = apply_where_x_to_filter(filter.clone(), where_x_expression); - *count = apply_where_x_quantity_expression(count.clone(), where_x_expression); + bind_where_x_filter(filter, where_x_expression, &mut unbound_where_x); + bind_where_x_quantity(count, where_x_expression, &mut unbound_where_x); } // CR 107.3i + CR 400.7: "return/put up to one target creature card with // mana value X or less ..., where X is " binds the @@ -7854,13 +7892,13 @@ pub(super) fn apply_where_x_effect_expression( // breaking the trigger's intended behavior. Mirrors the // `SearchLibrary`/`Seek` filter rewrite above. Effect::ChangeZone { target, .. } => { - *target = apply_where_x_to_filter(target.clone(), where_x_expression); + bind_where_x_filter(target, where_x_expression, &mut unbound_where_x); } Effect::Destroy { target, .. } | Effect::Bounce { target, .. } | Effect::BounceAll { target, .. } | Effect::CastFromZone { target, .. } => { - *target = apply_where_x_to_filter(target.clone(), where_x_expression); + bind_where_x_filter(target, where_x_expression, &mut unbound_where_x); } Effect::Dig { count, @@ -7868,12 +7906,12 @@ pub(super) fn apply_where_x_effect_expression( filter, .. } => { - *count = apply_where_x_quantity_expression(count.clone(), where_x_expression); - *player = apply_where_x_to_filter(player.clone(), where_x_expression); - *filter = apply_where_x_to_filter(filter.clone(), where_x_expression); + bind_where_x_quantity(count, where_x_expression, &mut unbound_where_x); + bind_where_x_filter(player, where_x_expression, &mut unbound_where_x); + bind_where_x_filter(filter, where_x_expression, &mut unbound_where_x); } Effect::Scry { count, .. } => { - *count = apply_where_x_quantity_expression(count.clone(), where_x_expression); + bind_where_x_quantity(count, where_x_expression, &mut unbound_where_x); } Effect::Pump { power, toughness, .. @@ -7922,9 +7960,9 @@ pub(super) fn apply_where_x_effect_expression( // CR 118.1 + CR 118.5: per-object scaled mana (`scale`) tracks the // surrounding where-X binding before the cost amount itself. if let Some(times) = scale { - *times = apply_where_x_quantity_expression(times.clone(), where_x_expression); + bind_where_x_quantity(times, where_x_expression, &mut unbound_where_x); } - apply_where_x_to_ability_cost(cost, where_x_expression); + apply_where_x_to_ability_cost(cost, where_x_expression, &mut unbound_where_x); } Effect::GenericEffect { static_abilities, @@ -7948,7 +7986,11 @@ pub(super) fn apply_where_x_effect_expression( target_based && where_x_is_demonstrative_target_creature_stat(where_x_expression); for static_def in static_abilities.iter_mut() { if let Some(condition) = static_def.condition.as_mut() { - apply_where_x_static_condition(condition, where_x_expression); + apply_where_x_static_condition( + condition, + where_x_expression, + &mut unbound_where_x, + ); } // CR 107.3i + CR 611.2c: A continuous "gets +X/+X … where X is // " grant lowers to dynamic P/T modifications whose @@ -7963,7 +8005,11 @@ pub(super) fn apply_where_x_effect_expression( // control") tracks the bound quantity instead of the cost-X // fallback. Mirrors the `Pump`/`SearchLibrary` arms above. for modification in static_def.modifications.iter_mut() { - apply_where_x_continuous_modification(modification, where_x_expression); + apply_where_x_continuous_modification( + modification, + where_x_expression, + &mut unbound_where_x, + ); if rebind_target_anaphor { rebind_target_anaphor_continuous_modification(modification); } @@ -8032,6 +8078,7 @@ fn rebind_dynamic_keyword_value_to_recipient( fn apply_where_x_continuous_modification( modification: &mut ContinuousModification, where_x_expression: Option<&str>, + unbound: &mut Option, ) { match modification { ContinuousModification::SetDynamicPower { value, .. } @@ -8040,10 +8087,10 @@ fn apply_where_x_continuous_modification( | ContinuousModification::SetToughnessDynamic { value, .. } | ContinuousModification::AddDynamicPower { value, .. } | ContinuousModification::AddDynamicToughness { value, .. } => { - *value = apply_where_x_quantity_expression(value.clone(), where_x_expression); + bind_where_x_quantity(value, where_x_expression, unbound); } ContinuousModification::AddDynamicKeyword { value, .. } => { - *value = apply_where_x_quantity_expression(value.clone(), where_x_expression); + bind_where_x_quantity(value, where_x_expression, unbound); // CR 613.4c + CR 702: a GRANTED keyword's "where X is its // power/toughness/mana value" refers to the keyword's RECIPIENT (the // creature that has the keyword), not the grant's source object. The @@ -8257,26 +8304,30 @@ fn rebind_cost_paid_object_pt_to_target(expr: &mut QuantityExpr) { /// (`Loyalty`, `Mill`, `Blight`, counts on Sacrifice/Exile/TapCreatures/…) or a /// static `ManaCost`/object filter that the where-X mana-value clause does not /// bind (X-in-mana-cost is concretized at announcement, not by this rewrite). -fn apply_where_x_to_ability_cost(cost: &mut AbilityCost, where_x_expression: Option<&str>) { +fn apply_where_x_to_ability_cost( + cost: &mut AbilityCost, + where_x_expression: Option<&str>, + unbound: &mut Option, +) { match cost { AbilityCost::PayLife { amount } | AbilityCost::PaySpeed { amount } | AbilityCost::PayEnergy { amount } | AbilityCost::ManaDynamic { quantity: amount } => { - *amount = apply_where_x_quantity_expression(amount.clone(), where_x_expression); + bind_where_x_quantity(amount, where_x_expression, unbound); } // CR 701.9: "discard X cards, where X is …" — the discard count is a // `QuantityExpr` and must track the same where-X binding. AbilityCost::Discard { count, .. } => { - *count = apply_where_x_quantity_expression(count.clone(), where_x_expression); + bind_where_x_quantity(count, where_x_expression, unbound); } AbilityCost::Composite { costs } | AbilityCost::OneOf { costs } => { for sub in costs.iter_mut() { - apply_where_x_to_ability_cost(sub, where_x_expression); + apply_where_x_to_ability_cost(sub, where_x_expression, unbound); } } AbilityCost::PerCounter { base, .. } => { - apply_where_x_to_ability_cost(base, where_x_expression); + apply_where_x_to_ability_cost(base, where_x_expression, unbound); } // CR 107.3i + CR 118.1: An effect performed as a cost nests an `Effect` // (e.g. `PutCounter { count: QuantityExpr }`), whose own quantity can @@ -8287,6 +8338,9 @@ fn apply_where_x_to_ability_cost(cost: &mut AbilityCost, where_x_expression: Opt AbilityCost::EffectCost { effect } => { apply_where_x_effect_expression(effect, where_x_expression); } + // (the nested effect reports its own unrepresentable where-X binding by + // rewriting itself to `Effect::unimplemented`, so no `unbound` plumbing + // is needed here) // No X-bearing `QuantityExpr` amount to bind: fixed integer counts // (`Loyalty`, `Mill`, `Blight`, counts on Sacrifice/Exile/…) or a static // `ManaCost`/object filter that this where-X mana-value clause does not @@ -8331,6 +8385,20 @@ pub(super) fn apply_where_x_to_latest_def( } } +/// Bind an X-bearing `TargetFilter` in place, recording an unrepresentable +/// where-X definition instead of fabricating one (CR 107.3c). Filter twin of +/// [`bind_where_x_quantity`]. +fn bind_where_x_filter( + slot: &mut TargetFilter, + where_x_expression: Option<&str>, + unbound: &mut Option, +) { + match apply_where_x_to_filter(slot.clone(), where_x_expression) { + Some(bound) => *slot = bound, + None => *unbound = where_x_expression.map(str::to_string), + } +} + /// CR 202.3 + CR 107.3i: Substitute the literal `X` inside a `TargetFilter`'s /// `FilterProp::Cmc` bounds with a trailing "where X is " defining /// clause. A `Cmc` bound parsed as `QuantityRef::Variable("X")` carries no @@ -8342,36 +8410,41 @@ pub(super) fn apply_where_x_to_latest_def( /// Walks typed-filter property lists and target-filter compositions, recursing /// through `AnyOf` nesting so composite "mana value N or M" bounds are /// covered. Non-`Cmc` props and non-typed filters pass through unchanged. +/// +/// Returns `None` when the where-X clause defines X but that definition has no +/// typed home (CR 107.3c) — the filter bound would otherwise carry a raw-text +/// `QuantityRef::Variable`, which resolves to 0 and silently narrows the filter +/// to "mana value 0 or less" while still reading as supported. pub(crate) fn apply_where_x_to_filter( filter: TargetFilter, where_x_expression: Option<&str>, -) -> TargetFilter { +) -> Option { if where_x_expression.is_none() { - return filter; + return Some(filter); } - match filter { + Some(match filter { TargetFilter::Typed(mut typed) => { typed.properties = typed .properties .into_iter() .map(|prop| apply_where_x_to_filter_prop(prop, where_x_expression)) - .collect(); + .collect::>>()?; TargetFilter::Typed(typed) } TargetFilter::And { filters } => TargetFilter::And { filters: filters .into_iter() .map(|filter| apply_where_x_to_filter(filter, where_x_expression)) - .collect(), + .collect::>>()?, }, TargetFilter::Or { filters } => TargetFilter::Or { filters: filters .into_iter() .map(|filter| apply_where_x_to_filter(filter, where_x_expression)) - .collect(), + .collect::>>()?, }, TargetFilter::Not { filter } => TargetFilter::Not { - filter: Box::new(apply_where_x_to_filter(*filter, where_x_expression)), + filter: Box::new(apply_where_x_to_filter(*filter, where_x_expression)?), }, TargetFilter::TrackedSetFiltered { id, @@ -8379,11 +8452,11 @@ pub(crate) fn apply_where_x_to_filter( caused_by, } => TargetFilter::TrackedSetFiltered { id, - filter: Box::new(apply_where_x_to_filter(*filter, where_x_expression)), + filter: Box::new(apply_where_x_to_filter(*filter, where_x_expression)?), caused_by, }, other => other, - } + }) } /// CR 107.3i + CR 202.3: Substitute the X binding into a target-set constraint's @@ -8395,17 +8468,21 @@ pub(crate) fn apply_where_x_to_filter( fn apply_where_x_to_target_constraint( constraint: &mut TargetSelectionConstraint, where_x_expression: Option<&str>, + unbound: &mut Option, ) { if let TargetSelectionConstraint::TotalManaValue { value, .. } = constraint { - *value = apply_where_x_quantity_expression(value.clone(), where_x_expression); + bind_where_x_quantity(value, where_x_expression, unbound); } } -fn apply_where_x_to_filter_prop(prop: FilterProp, where_x_expression: Option<&str>) -> FilterProp { - match prop { +fn apply_where_x_to_filter_prop( + prop: FilterProp, + where_x_expression: Option<&str>, +) -> Option { + Some(match prop { FilterProp::Cmc { comparator, value } => FilterProp::Cmc { comparator, - value: apply_where_x_quantity_expression(value, where_x_expression), + value: apply_where_x_quantity_expression(value, where_x_expression)?, }, FilterProp::Counters { counters, @@ -8414,7 +8491,7 @@ fn apply_where_x_to_filter_prop(prop: FilterProp, where_x_expression: Option<&st } => FilterProp::Counters { counters, comparator, - count: apply_where_x_quantity_expression(count, where_x_expression), + count: apply_where_x_quantity_expression(count, where_x_expression)?, }, FilterProp::PtComparison { stat, @@ -8425,13 +8502,13 @@ fn apply_where_x_to_filter_prop(prop: FilterProp, where_x_expression: Option<&st stat, scope, comparator, - value: apply_where_x_quantity_expression(value, where_x_expression), + value: apply_where_x_quantity_expression(value, where_x_expression)?, }, FilterProp::CanEnchant { target } => FilterProp::CanEnchant { - target: Box::new(apply_where_x_to_filter(*target, where_x_expression)), + target: Box::new(apply_where_x_to_filter(*target, where_x_expression)?), }, FilterProp::DifferentNameFrom { filter } => FilterProp::DifferentNameFrom { - filter: Box::new(apply_where_x_to_filter(*filter, where_x_expression)), + filter: Box::new(apply_where_x_to_filter(*filter, where_x_expression)?), }, FilterProp::SharesQuality { quality, @@ -8439,29 +8516,34 @@ fn apply_where_x_to_filter_prop(prop: FilterProp, where_x_expression: Option<&st relation, } => FilterProp::SharesQuality { quality, - reference: reference - .map(|filter| Box::new(apply_where_x_to_filter(*filter, where_x_expression))), + reference: match reference { + Some(filter) => Some(Box::new(apply_where_x_to_filter( + *filter, + where_x_expression, + )?)), + None => None, + }, relation, }, FilterProp::TargetsOnly { filter } => FilterProp::TargetsOnly { - filter: Box::new(apply_where_x_to_filter(*filter, where_x_expression)), + filter: Box::new(apply_where_x_to_filter(*filter, where_x_expression)?), }, FilterProp::Targets { filter } => FilterProp::Targets { - filter: Box::new(apply_where_x_to_filter(*filter, where_x_expression)), + filter: Box::new(apply_where_x_to_filter(*filter, where_x_expression)?), }, FilterProp::AnyOf { props } => FilterProp::AnyOf { props: props .into_iter() .map(|p| apply_where_x_to_filter_prop(p, where_x_expression)) - .collect(), + .collect::>>()?, }, // CR 608.2c: Descend into the negated inner prop so X-substitution // reaches it (mirrors the AnyOf transform). FilterProp::Not { prop } => FilterProp::Not { - prop: Box::new(apply_where_x_to_filter_prop(*prop, where_x_expression)), + prop: Box::new(apply_where_x_to_filter_prop(*prop, where_x_expression)?), }, other => other, - } + }) } pub(super) fn apply_where_x_ability_expression( @@ -8472,17 +8554,28 @@ pub(super) fn apply_where_x_ability_expression( // time. Substitute X in this AbilityDefinition's condition before walking // into effect/sub_ability/etc. The recursion below visits every chained // SequentialSibling node, so each node's own `condition` is reached here. + // CR 107.3c: set when this ability's where-X clause DEFINES X but the + // definition has no typed home. Converted to a gap node after the walk (the + // rewrites below hold mutable borrows of `def`'s fields). + let mut unbound_where_x: Option = None; if let Some(cond) = def.condition.as_mut() { - apply_where_x_ability_condition(cond, where_x_expression); + apply_where_x_ability_condition(cond, where_x_expression, &mut unbound_where_x); } if let Some(repeat_for) = def.repeat_for.take() { - def.repeat_for = Some(apply_where_x_quantity_expression( - repeat_for, - where_x_expression, - )); + match apply_where_x_quantity_expression(repeat_for, where_x_expression) { + Some(bound) => def.repeat_for = Some(bound), + None => unbound_where_x = where_x_expression.map(str::to_string), + } } if let Some(spec) = def.multi_target.as_mut() { - spec.map_quantities(|expr| apply_where_x_quantity_expression(expr, where_x_expression)); + // `map_quantities` is infallible, so bind each quantity through the + // shared authority and record an unrepresentable definition out-of-band + // rather than fabricating one. + spec.map_quantities(|expr| { + let mut slot = expr; + bind_where_x_quantity(&mut slot, where_x_expression, &mut unbound_where_x); + slot + }); } // CR 107.3i + CR 202.3: Rebind X in the target-set constraints (e.g. the // `TotalManaValue` cap on Ancient Brass Dragon, whose bound is the @@ -8490,9 +8583,15 @@ pub(super) fn apply_where_x_ability_expression( // inherits `Variable("X")` with no defining expression and the cap is // effectively unbounded. for constraint in def.target_constraints.iter_mut() { - apply_where_x_to_target_constraint(constraint, where_x_expression); + apply_where_x_to_target_constraint(constraint, where_x_expression, &mut unbound_where_x); } apply_where_x_effect_expression(def.effect.as_mut(), where_x_expression); + // CR 107.3c: the clause defines X, but we cannot represent that definition. + // Report the gap instead of keeping a raw-text placeholder that resolves to + // 0 while still reading as a supported dynamic quantity. + if let Some(expression) = unbound_where_x { + *def.effect = Effect::unimplemented("where_x_binding", format!("where X is {expression}")); + } if let Some(sub) = def.sub_ability.as_mut() { apply_where_x_ability_expression(sub, where_x_expression); } @@ -8509,22 +8608,26 @@ pub(super) fn apply_where_x_ability_expression( /// `apply_where_x_quantity_expression`; recurses through compound arms /// (`And`/`Or`/`Not`/`ConditionInstead`). Leaf arms without quantity fields /// fall through to the no-op `_` arm. -fn apply_where_x_ability_condition(cond: &mut AbilityCondition, where_x_expression: Option<&str>) { +fn apply_where_x_ability_condition( + cond: &mut AbilityCondition, + where_x_expression: Option<&str>, + unbound: &mut Option, +) { match cond { AbilityCondition::QuantityCheck { lhs, rhs, .. } => { - *lhs = apply_where_x_quantity_expression(lhs.clone(), where_x_expression); - *rhs = apply_where_x_quantity_expression(rhs.clone(), where_x_expression); + bind_where_x_quantity(lhs, where_x_expression, unbound); + bind_where_x_quantity(rhs, where_x_expression, unbound); } AbilityCondition::And { conditions } | AbilityCondition::Or { conditions } => { for c in conditions.iter_mut() { - apply_where_x_ability_condition(c, where_x_expression); + apply_where_x_ability_condition(c, where_x_expression, unbound); } } AbilityCondition::Not { condition } => { - apply_where_x_ability_condition(condition, where_x_expression); + apply_where_x_ability_condition(condition, where_x_expression, unbound); } AbilityCondition::ConditionInstead { inner } => { - apply_where_x_ability_condition(inner, where_x_expression); + apply_where_x_ability_condition(inner, where_x_expression, unbound); } _ => {} } @@ -8533,19 +8636,20 @@ fn apply_where_x_ability_condition(cond: &mut AbilityCondition, where_x_expressi fn apply_where_x_static_condition( condition: &mut StaticCondition, where_x_expression: Option<&str>, + unbound: &mut Option, ) { match condition { StaticCondition::QuantityComparison { lhs, rhs, .. } => { - *lhs = apply_where_x_quantity_expression(lhs.clone(), where_x_expression); - *rhs = apply_where_x_quantity_expression(rhs.clone(), where_x_expression); + bind_where_x_quantity(lhs, where_x_expression, unbound); + bind_where_x_quantity(rhs, where_x_expression, unbound); } StaticCondition::And { conditions } | StaticCondition::Or { conditions } => { for condition in conditions { - apply_where_x_static_condition(condition, where_x_expression); + apply_where_x_static_condition(condition, where_x_expression, unbound); } } StaticCondition::Not { condition } => { - apply_where_x_static_condition(condition, where_x_expression); + apply_where_x_static_condition(condition, where_x_expression, unbound); } _ => {} } @@ -10072,7 +10176,16 @@ mod where_x_tests { qty: QuantityRef::Variable { name: "X".into() }, }, }; - super::apply_where_x_to_target_constraint(&mut constraint, Some("the result")); + let mut unbound = None; + super::apply_where_x_to_target_constraint( + &mut constraint, + Some("the result"), + &mut unbound, + ); + assert_eq!( + unbound, None, + "\"the result\" is representable, so no gap is recorded" + ); assert_eq!( constraint, TargetSelectionConstraint::TotalManaValue { @@ -10118,7 +10231,12 @@ mod where_x_tests { use crate::types::game_state::TargetSelectionConstraint; let mut constraint = TargetSelectionConstraint::DifferentObjectControllers; - super::apply_where_x_to_target_constraint(&mut constraint, Some("the result")); + let mut unbound = None; + super::apply_where_x_to_target_constraint( + &mut constraint, + Some("the result"), + &mut unbound, + ); assert_eq!( constraint, TargetSelectionConstraint::DifferentObjectControllers @@ -10151,7 +10269,8 @@ mod where_x_tests { ], }; - let rewritten = super::apply_where_x_quantity_expression(expression, Some("the result")); + let rewritten = super::apply_where_x_quantity_expression(expression, Some("the result")) + .expect("\"the result\" is representable, so the bind must succeed"); let QuantityExpr::Sum { exprs } = rewritten else { panic!("expected Sum"); }; diff --git a/crates/engine/src/parser/oracle_effect/mana.rs b/crates/engine/src/parser/oracle_effect/mana.rs index 89ac9a06fd..96cfd71d81 100644 --- a/crates/engine/src/parser/oracle_effect/mana.rs +++ b/crates/engine/src/parser/oracle_effect/mana.rs @@ -218,7 +218,7 @@ pub(super) fn try_parse_add_mana_effect_with_context( // `parse_mana_production_clause` so the where-X count is resolved here, // co-located with `apply_where_x_count_expression`. if let Some((count, color_options)) = parse_repeated_count_color_choice(clause) { - let (count, target) = apply_where_x_count_expression(count, where_x_expression.as_deref()); + let (count, target) = apply_where_x_count_expression(count, where_x_expression.as_deref())?; return Some(Effect::Mana { produced: ManaProduction::AnyOneColor { count, @@ -297,7 +297,7 @@ pub(super) fn try_parse_add_mana_effect_with_context( if let Some((count, rest)) = parse_mana_count_prefix(clause) { let (count, where_x_target) = - apply_where_x_count_expression(count, where_x_expression.as_deref()); + apply_where_x_count_expression(count, where_x_expression.as_deref())?; let rest = rest.trim().trim_end_matches(['.', '"']).trim(); let rest_lower = rest.to_lowercase(); @@ -645,7 +645,7 @@ pub(super) fn try_parse_add_mana_effect_with_context( .map(|(count, _)| count) .unwrap_or(QuantityExpr::Fixed { value: 1 }); let (fallback_count, fallback_target) = - apply_where_x_count_expression(fallback_count, where_x_expression.as_deref()); + apply_where_x_count_expression(fallback_count, where_x_expression.as_deref())?; // Scan for mana production type at word boundaries using nom combinators. let produced = scan_mana_production_type(&clause_lower, fallback_count.clone(), contribution)?; @@ -1048,10 +1048,13 @@ pub(super) fn parse_mana_count_prefix(text: &str) -> Option<(QuantityExpr, &str) )) } +/// CR 107.3c: Bind a "where X is …" mana count, or FAIL (`None`) when the +/// definition has no typed home. Never fabricates a raw-text placeholder — see +/// `apply_where_x_quantity_expression` for why such a node is dead at runtime. pub(super) fn apply_where_x_count_expression( count: QuantityExpr, where_x_expression: Option<&str>, -) -> (QuantityExpr, Option) { +) -> Option<(QuantityExpr, Option)> { match (&count, where_x_expression) { ( QuantityExpr::Ref { @@ -1059,19 +1062,15 @@ pub(super) fn apply_where_x_count_expression( }, Some(expression), ) if name.eq_ignore_ascii_case("X") => { - if let Some(count) = super::parse_where_x_quantity_expression(expression) { - return (count, where_x_expression_target_filter(expression)); - } - ( - QuantityExpr::Ref { - qty: QuantityRef::Variable { - name: expression.to_string(), - }, - }, - None, - ) + // CR 107.3c: the clause DEFINES X. An unrepresentable definition is a + // PARSE FAILURE (`None`), never a raw-text placeholder: the fabricated + // `QuantityRef::Variable { name: "" }` is dead at runtime + // (game/quantity.rs resolves a non-`X` variable name to 0), so the mana + // clause produced ZERO mana while still reading as supported. + let count = super::parse_where_x_quantity_expression(expression)?; + Some((count, where_x_expression_target_filter(expression))) } - _ => (count, None), + _ => Some((count, None)), } } diff --git a/crates/engine/src/parser/oracle_effect/mod.rs b/crates/engine/src/parser/oracle_effect/mod.rs index c101b65b7e..a353284b89 100644 --- a/crates/engine/src/parser/oracle_effect/mod.rs +++ b/crates/engine/src/parser/oracle_effect/mod.rs @@ -7960,7 +7960,18 @@ fn parse_effect_clause_inner(text: &str, ctx: &mut ParseContext) -> ParsedEffect let (discover_tp, discover_where_x) = strip_trailing_where_x(tp); if let Some((discover_player, limit, rest_orig)) = parse_discover_with_player(discover_tp) { if rest_orig.trim().is_empty() { - let limit = apply_where_x_quantity_expression(limit, discover_where_x.as_deref()); + // CR 107.3c: "discover X, where X is " — the clause DEFINES X. + // If the definition has no typed home, report the gap instead of + // fabricating a raw-text placeholder that resolves to 0 (a discover + // for mana value 0) while still reading as supported. + let Some(limit) = apply_where_x_quantity_expression(limit, discover_where_x.as_deref()) + else { + let expression = discover_where_x.unwrap_or_default(); + return parsed_clause(Effect::unimplemented( + "where_x_binding", + format!("where X is {expression}"), + )); + }; return parsed_clause(Effect::Discover { mana_value_limit: limit, player: discover_player, @@ -10719,7 +10730,8 @@ fn try_parse_reveal_until(tp: TextPair, player: TargetFilter) -> Option Option Option" defining clause. - let filter = apply_where_x_to_filter(filter, where_x_expression.as_deref()); + // CR 107.3c: fail honestly instead of fabricating a raw-text placeholder. + let filter = apply_where_x_to_filter(filter, where_x_expression.as_deref())?; // CR 110.2a + CR 708.2a/708.3: detect "under your control" / "face down" on // the full clause for the from-among put-step. diff --git a/crates/engine/src/parser/oracle_effect/tests.rs b/crates/engine/src/parser/oracle_effect/tests.rs index 61827fef50..4bf1bf5674 100644 --- a/crates/engine/src/parser/oracle_effect/tests.rs +++ b/crates/engine/src/parser/oracle_effect/tests.rs @@ -3,7 +3,8 @@ use crate::parser::parse_oracle_text; use crate::types::ability::CardPlayMode::{Cast, Play}; use crate::types::ability::CastFromZoneDriver::{DuringResolution, LingeringPermission}; use crate::types::ability::{ - AttachmentKind, ExcessRecipient, ForEachCategoryAction, PerpetualModification, + AttachmentKind, CastManaObjectScope, CastManaSpentMetric, ExcessRecipient, + ForEachCategoryAction, PerpetualModification, }; use crate::types::card_type::CoreType; use crate::types::mana::{ManaCost, ManaCostShard}; @@ -29143,6 +29144,165 @@ fn where_x_power_of_the_exiled_card_binds_exiled_card_power() { } } +/// CR 107.3c: the QUANTITY channel of the same "where X is …" defect #5706 fixed +/// for P/T. When the where-clause defined X with an expression the parser could +/// not type, `apply_where_x_quantity_expression` fabricated +/// `QuantityRef::Variable { name: "" }`. That node is well-typed +/// and renders as a supported dynamic quantity in the coverage report, but +/// `game/quantity.rs` (non-`"X"` `Variable` arm) resolves it through +/// `state.last_named_choice` and `.unwrap_or(0)` — so the effect read 0 (or an +/// unrelated stale number left by some earlier "choose a number"). +/// +/// These four expression classes each have a typed home AND a live resolver arm +/// that both already existed; the where-X interpreter simply never delegated to +/// them. Each assertion below is a regression pin: the raw-text fallback must +/// never come back. +#[test] +fn where_x_intensity_binds_source_intensity() { + // Arek, False Goldwarden: "Target opponent loses X life and you gain X life, + // where X is Arek's intensity." Self-reference is normalized to `~` upstream. + let def = parse_effect_chain( + "target opponent loses X life, where X is ~'s intensity", + AbilityKind::Spell, + ); + let expected = QuantityExpr::Ref { + qty: QuantityRef::Intensity { + scope: ObjectScope::Source, + }, + }; + let Effect::LoseLife { amount, .. } = &*def.effect else { + panic!("expected LoseLife, got {:?}", def.effect); + }; + assert_eq!( + amount, &expected, + "X must bind to Intensity{{Source}}; a raw-text Variable resolves to 0" + ); +} + +/// Mycelic Ballad spells the same quantity as "this spell's intensity" — the +/// possessive is the only axis that differs, so it must reach the same binding. +#[test] +fn where_x_this_spells_intensity_binds_source_intensity() { + let def = parse_effect_chain( + "you gain X life, where X is this spell's intensity", + AbilityKind::Spell, + ); + let Effect::GainLife { amount, .. } = &*def.effect else { + panic!("expected GainLife, got {:?}", def.effect); + }; + assert_eq!( + amount, + &QuantityExpr::Ref { + qty: QuantityRef::Intensity { + scope: ObjectScope::Source, + }, + }, + "\"this spell's intensity\" must reach the same Intensity binding as \"~'s intensity\"" + ); +} + +/// Liquid Fire / Fluros of Myra's Marvels: "where X is the chosen number". +/// `QuantityRef::ChosenNumber` reads `ChosenAttribute::Number` off the source +/// object (game/quantity.rs) — the value the player actually chose. +#[test] +fn where_x_the_chosen_number_binds_chosen_number() { + let def = parse_effect_chain( + "~ deals X damage to target creature, where X is the chosen number", + AbilityKind::Spell, + ); + let Effect::DealDamage { amount, .. } = &*def.effect else { + panic!("expected DealDamage, got {:?}", def.effect); + }; + assert_eq!( + amount, + &QuantityExpr::Ref { + qty: QuantityRef::ChosenNumber + }, + "X must bind to ChosenNumber, not a raw-text Variable" + ); +} + +/// Toph, Greatest Earthbender: "where X is the amount of mana spent to cast her". +/// CR 107.3c + the existing `ManaSpentToCast{SelfObject, Total}` typed home. +#[test] +fn where_x_mana_spent_to_cast_binds_mana_spent_to_cast() { + for text in [ + "you gain X life, where X is the amount of mana spent to cast her", + "you gain X life, where X is the amount of mana spent to cast it", + "you gain X life, where X is the amount of mana spent to cast this spell", + ] { + let def = parse_effect_chain(text, AbilityKind::Spell); + let Effect::GainLife { amount, .. } = &*def.effect else { + panic!("expected GainLife for {text:?}, got {:?}", def.effect); + }; + assert_eq!( + amount, + &QuantityExpr::Ref { + qty: QuantityRef::ManaSpentToCast { + scope: CastManaObjectScope::SelfObject, + metric: CastManaSpentMetric::Total, + }, + }, + "X must bind to ManaSpentToCast{{Total}} for {text:?}" + ); + } +} + +/// CR 107.4h: "{S} … can also be used to refer to mana of any type produced by a +/// snow source spent to pay a cost." Graven Lore / Blessing of Frost / Blood on +/// the Snow: "where X is the amount of {S} spent to cast this spell". +/// `CastManaSpentMetric::FromSource` counts the mana whose PRODUCING source +/// matches the filter, so a Snow-supertype source filter is the exact model. +#[test] +fn where_x_snow_mana_spent_binds_mana_spent_from_snow_source() { + let def = parse_effect_chain( + "scry X, where X is the amount of {S} spent to cast this spell", + AbilityKind::Spell, + ); + let Effect::Scry { count, .. } = &*def.effect else { + panic!("expected Scry, got {:?}", def.effect); + }; + let QuantityExpr::Ref { + qty: + QuantityRef::ManaSpentToCast { + scope: CastManaObjectScope::SelfObject, + metric: CastManaSpentMetric::FromSource { source_filter }, + }, + } = count + else { + panic!("X must bind to ManaSpentToCast{{FromSource}}, got {count:?}"); + }; + assert!( + format!("{source_filter:?}").contains("Snow"), + "the source filter must select snow sources (CR 107.4h), got {source_filter:?}" + ); +} + +/// The convert half of the same defect: a where-X definition with NO typed home +/// must FAIL HONESTLY, not fabricate. Porcuparrot's "{T}: This creature deals X +/// damage to any target, where X is the number of times this creature has +/// mutated" has no QuantityRef and no resolver — mutation count is not modeled. +/// +/// Pre-fix this lowered to `QuantityRef::Variable { name: "the number of times ~ +/// has mutated" }`, which `game/quantity.rs` resolves through `last_named_choice` +/// to 0: Porcuparrot dealt ZERO damage while the coverage report called it +/// supported. The ability must lower to an Unimplemented gap so the report shows +/// red instead. +#[test] +fn where_x_unrepresentable_quantity_fails_honestly_instead_of_fabricating() { + let def = parse_effect_chain( + "~ deals X damage to any target, where X is the number of times ~ has mutated", + AbilityKind::Activated, + ); + assert!( + matches!(&*def.effect, Effect::Unimplemented { .. }), + "an unrepresentable where-X definition must lower to Unimplemented (honest red), \ + never to a raw-text Variable that resolves to 0 while reading as supported; \ + got {:?}", + def.effect + ); +} + #[test] fn duration_preserved_with_for_each_suffix() { // Goblin Piledriver pattern: "gets +2/+0 until end of turn for each other attacking Goblin" diff --git a/crates/engine/src/parser/oracle_effect/token.rs b/crates/engine/src/parser/oracle_effect/token.rs index 70cb02eefe..33c6bc0e38 100644 --- a/crates/engine/src/parser/oracle_effect/token.rs +++ b/crates/engine/src/parser/oracle_effect/token.rs @@ -106,15 +106,18 @@ pub(crate) fn try_parse_token(_lower: &str, text: &str, ctx: &mut ParseContext) if matches!(&count, QuantityExpr::Ref { qty: QuantityRef::Variable { ref name } } if name == "X") { if let Some(where_expression) = extract_token_where_x_expression(&text) { - count = super::parse_where_x_quantity_expression(&where_expression) - .or_else(|| { + // CR 107.3c: the clause DEFINES X. If the definition is not + // representable, this copy-token clause does not lower — fail the + // parse instead of fabricating a raw-text placeholder. The + // fabricated `QuantityRef::Variable { name: "" }` is + // well-typed but DEAD (game/quantity.rs resolves a non-`X` variable + // name to 0), so the effect copied ZERO tokens while the raw text + // still rendered as a supported dynamic quantity. This mirrors the + // sibling non-copy token path below. + count = + super::parse_where_x_quantity_expression(&where_expression).or_else(|| { crate::parser::oracle_quantity::parse_cda_quantity(&where_expression) - }) - .unwrap_or(QuantityExpr::Ref { - qty: QuantityRef::Variable { - name: where_expression, - }, - }); + })?; } } return Some(Effect::CopyTokenOf { @@ -703,15 +706,17 @@ fn parse_token_description_with_context( // `parse_event_context_quantity` only fires when `parse_cda_quantity` // returns None and itself returns None for unrecognized phrases, so // it strictly widens coverage without disturbing existing matches. + // CR 122.1 + CR 608.2c: bind the deferred "a number of" count to the + // quantity its "equal to " clause names. An unrepresentable + // expression FAILS the token clause — the raw-text placeholder it used + // to fabricate is dead at runtime (game/quantity.rs resolves a non-`X` + // variable name to 0), so the card created ZERO tokens while still + // reading as supported. count = crate::parser::oracle_quantity::parse_cda_quantity(&count_expression) .or_else(|| { crate::parser::oracle_quantity::parse_event_context_quantity(&count_expression) }) - .unwrap_or(QuantityExpr::Ref { - qty: QuantityRef::Variable { - name: count_expression, - }, - }); + .or_else(|| super::parse_where_x_quantity_expression(&count_expression))?; } } diff --git a/crates/engine/src/parser/oracle_nom/quantity.rs b/crates/engine/src/parser/oracle_nom/quantity.rs index 2c9150dcee..a4a961f9f7 100644 --- a/crates/engine/src/parser/oracle_nom/quantity.rs +++ b/crates/engine/src/parser/oracle_nom/quantity.rs @@ -592,11 +592,48 @@ fn parse_guessed_number_ref(input: &str) -> OracleResult<'_, QuantityRef> { .parse(input) } +/// Alchemy (digital-only) intensity: " intensity". +/// +/// The self-reference is normalized to `~` upstream, so Arek, False +/// Goldwarden's "where X is Arek's intensity" arrives as "~'s intensity"; a +/// spell reading its own counter says "this spell's intensity" (Mycelic +/// Ballad). Both denote the SOURCE object, which is what +/// `QuantityRef::Intensity { scope: Source }` resolves against (game/quantity.rs). +/// +/// Without this arm the phrase fell through to the raw-text +/// `QuantityRef::Variable`, which resolves to 0 — every intensity card silently +/// did nothing while reading as supported. +fn parse_intensity_ref(input: &str) -> OracleResult<'_, QuantityRef> { + value( + QuantityRef::Intensity { + scope: ObjectScope::Source, + }, + terminated( + // "this spell's" is a leaf variant of the same self-possessive axis; + // it is kept local rather than pushed into `parse_self_possessive`, + // whose many other callers do not expect a stack-only possessive. + alt((parse_self_possessive, value((), tag("this spell's")))), + tag(" intensity"), + ), + ) + .parse(input) +} + +/// CR 107.3: "the chosen number" — the number a player named for this object +/// (Liquid Fire's additional cost; Fluros of Myra's Marvels' as-enters choice). +/// `QuantityRef::ChosenNumber` reads `ChosenAttribute::Number` off the source +/// object (game/quantity.rs), which is where the choice is recorded. +fn parse_chosen_number_ref(input: &str) -> OracleResult<'_, QuantityRef> { + value(QuantityRef::ChosenNumber, tag("the chosen number")).parse(input) +} + pub fn parse_quantity_ref(input: &str) -> OracleResult<'_, QuantityRef> { alt(( alt(( parse_guessed_number_ref, parse_object_count_by_shared_quality, + parse_chosen_number_ref, + parse_intensity_ref, )), parse_the_number_of, parse_object_property_aggregate_ref, @@ -3590,6 +3627,27 @@ fn parse_mana_spent_to_cast_ref(input: &str) -> OracleResult<'_, QuantityRef> { )); } + // CR 107.4h: "{S} spent to cast " — the snow mana symbol "can also be + // used to refer to mana of any type produced by a snow source spent to pay a + // cost". That is exactly `FromSource`, whose filter selects the PRODUCING + // source (game/quantity.rs counts each spent-mana snapshot whose source + // matches), so a Snow-supertype filter is the precise model. Graven Lore, + // Blessing of Frost, Blood on the Snow. The symbol is matched case-insensitively + // because this combinator runs on both original and lowercased text. + if let Ok((rest, _)) = parse_snow_mana_symbol(input) { + let (rest, _) = tag(" spent to cast ").parse(rest)?; + let (rest, _scope) = parse_mana_spent_self_subject(rest)?; + return Ok(( + rest, + QuantityRef::ManaSpentToCast { + scope: CastManaObjectScope::SelfObject, + metric: CastManaSpentMetric::FromSource { + source_filter: snow_source_filter(), + }, + }, + )); + } + let (rest, _) = tag("mana spent to cast ").parse(input)?; // SelfObject literal retained: this ref form never accepts "that" subjects. let (rest, _scope) = parse_mana_spent_self_subject(rest)?; @@ -3626,6 +3684,26 @@ pub(crate) fn parse_mana_source_filter(input: &str) -> OracleResult<'_, TargetFi Ok((rest, source_filter)) } +/// CR 107.4h: The snow mana symbol `{S}`. Matched case-insensitively because +/// this combinator runs on both original-case and lowercased text. +pub(crate) fn parse_snow_mana_symbol(input: &str) -> OracleResult<'_, ()> { + value((), alt((tag::<_, _, OracleError<'_>>("{s}"), tag("{S}")))).parse(input) +} + +/// CR 106.3 + CR 107.4h: The filter that selects a SNOW SOURCE — any object with +/// the Snow supertype. Single authority for the `{S}` model, shared by every +/// "mana produced by a snow source" reading so the two entry points +/// (`parse_mana_spent_to_cast_ref` here and `parse_mana_spent_to_cast_amount` in +/// `oracle_quantity`) cannot drift apart. +pub(crate) fn snow_source_filter() -> TargetFilter { + TargetFilter::Typed(TypedFilter { + properties: vec![FilterProp::HasSupertype { + value: crate::types::card_type::Supertype::Snow, + }], + ..Default::default() + }) +} + /// CR 400.7d: Parse the subject anaphora of a "mana spent to cast " /// clause and report which `CastManaObjectScope` it selects. /// @@ -3647,6 +3725,12 @@ pub(crate) fn parse_mana_spent_self_subject(input: &str) -> OracleResult<'_, Cas value(CastManaObjectScope::SelfObject, tag("this permanent")), value(CastManaObjectScope::SelfObject, tag("it")), value(CastManaObjectScope::SelfObject, tag("them")), + // CR 400.7d: gendered self-anaphora — Oracle text for a legendary + // creature refers to the spell as "her"/"him" (Toph, Greatest + // Earthbender: "where X is the amount of mana spent to cast her"). + // Same self-object axis as "it"/"them"; only the pronoun differs. + value(CastManaObjectScope::SelfObject, tag("her")), + value(CastManaObjectScope::SelfObject, tag("him")), value(CastManaObjectScope::SelfObject, tag("~")), )) .parse(input) diff --git a/crates/engine/src/parser/oracle_quantity.rs b/crates/engine/src/parser/oracle_quantity.rs index 546e40dc29..9438753d8e 100644 --- a/crates/engine/src/parser/oracle_quantity.rs +++ b/crates/engine/src/parser/oracle_quantity.rs @@ -1832,46 +1832,52 @@ pub(crate) fn parse_event_context_quantity(text: &str) -> Option { /// effect reading the triggering spell's cost; Wildgrowth Archaic, /// Expressive Firedancer rider, Mana Sculpt rider). fn parse_mana_spent_to_cast_amount(input: &str) -> Option { - // Consume optional leading "the ". + // Consume optional leading "the ", then the shared "amount of " head. let rest = tag::<_, _, OracleError<'_>>("the ") .parse(input) .map_or(input, |(r, _)| r); - // Consume the core phrase. Accept both "mana you spent" and "mana spent". - let rest = alt(( - value( - (), - tag::<_, _, OracleError<'_>>("amount of mana you spent to cast "), - ), - value((), tag("amount of mana spent to cast ")), + let rest = tag::<_, _, OracleError<'_>>("amount of ") + .parse(rest) + .ok()? + .0; + + // CR 107.4h: "{S} spent to cast " — the snow mana symbol refers to + // mana produced by a snow source. `FromSource` counts exactly the spent-mana + // snapshots whose PRODUCING source matches the filter (game/quantity.rs). + // Graven Lore, Blessing of Frost, Blood on the Snow. + if let Ok((subject, _)) = nom_quantity::parse_snow_mana_symbol(rest) { + let subject = tag::<_, _, OracleError<'_>>(" spent to cast ") + .parse(subject) + .ok()? + .0; + let (_, scope) = nom_quantity::parse_mana_spent_self_subject(subject).ok()?; + return Some(QuantityRef::ManaSpentToCast { + scope, + metric: crate::types::ability::CastManaSpentMetric::FromSource { + source_filter: nom_quantity::snow_source_filter(), + }, + }); + } + + // "mana [you] spent to cast " — total mana paid. + let subject = alt(( + value((), tag::<_, _, OracleError<'_>>("mana you spent to cast ")), + value((), tag("mana spent to cast ")), )) .parse(rest) .ok()? .0; - // Dispatch on subject: self-referential vs triggering-spell anaphora. - alt(( - value( - QuantityRef::ManaSpentToCast { - scope: crate::types::ability::CastManaObjectScope::SelfObject, - metric: crate::types::ability::CastManaSpentMetric::Total, - }, - alt(( - tag::<_, _, OracleError<'_>>("this spell"), - tag("this creature"), - tag("it"), - tag("~"), - )), - ), - value( - QuantityRef::ManaSpentToCast { - scope: crate::types::ability::CastManaObjectScope::TriggeringSpell, - metric: crate::types::ability::CastManaSpentMetric::Total, - }, - alt((tag("that spell"), tag("that creature"))), - ), - )) - .parse(rest) - .ok() - .map(|(_, qty)| qty) + + // The subject anaphora IS the scope signal (CR 400.7d). Delegate to the + // shared `parse_mana_spent_self_subject` combinator — the single authority — + // instead of re-listing the pronouns here. That duplicate list is why Toph, + // Greatest Earthbender ("...spent to cast her") fell through to a raw-text + // `QuantityRef::Variable` and resolved to 0. + let (_, scope) = nom_quantity::parse_mana_spent_self_subject(subject).ok()?; + Some(QuantityRef::ManaSpentToCast { + scope, + metric: crate::types::ability::CastManaSpentMetric::Total, + }) } /// CR 603.7c: Classify the prefix of a `"'s "` possessive diff --git a/crates/engine/src/parser/oracle_static/anthem.rs b/crates/engine/src/parser/oracle_static/anthem.rs index 22427938ed..609b75ccb1 100644 --- a/crates/engine/src/parser/oracle_static/anthem.rs +++ b/crates/engine/src/parser/oracle_static/anthem.rs @@ -1189,9 +1189,10 @@ pub(crate) fn parse_dynamic_pt_in_text( // Run whose effect text has no binding clause — the X is bound to the // cost, not to a derived quantity. let quantity = match where_x_expression { - Some(wx) => parse_cda_quantity(wx) - .or_else(|| parse_event_context_quantity(wx)) - .or_else(|| parse_source_intensity_where_x(wx))?, + // Intensity now lives in the shared `parse_quantity_ref` combinator + // (oracle_nom/quantity.rs), which `parse_cda_quantity` delegates to — so + // the local duplicate this arm used to carry is gone. + Some(wx) => parse_cda_quantity(wx).or_else(|| parse_event_context_quantity(wx))?, None => QuantityExpr::Ref { qty: QuantityRef::CostXPaid, }, @@ -1235,30 +1236,6 @@ pub(crate) fn parse_dynamic_pt_in_text( Some(mods) } -/// Digital-only Alchemy (Arena): "where X is 's intensity" — a dynamic -/// P/T pump that scales by the STATIC's own source object's intensity (Minthara -/// of the Absolute, Teysa of the Ghost Council, Quickbeast Amulet). The -/// self-possessive axis reuses [`nom_quantity::parse_self_possessive`] -/// (its / ~'s / this creature's / …) rather than enumerating verbatim strings; -/// `normalize_card_name_refs` funnels every card-name and permanent-type -/// self-reference to `~` upstream, so `~'s intensity` is the form the static -/// parser actually receives. `QuantityRef::Intensity { scope: Source }` is fully -/// evaluated at runtime by `game::quantity`, so this is a grammar-only seam that -/// reuses existing modeling. -fn parse_source_intensity_where_x(wx: &str) -> Option { - nom_parse_lower(wx.trim().trim_end_matches('.').trim(), |i| { - value( - QuantityExpr::Ref { - qty: QuantityRef::Intensity { - scope: ObjectScope::Source, - }, - }, - terminated(nom_quantity::parse_self_possessive, tag(" intensity")), - ) - .parse(i) - }) -} - pub(crate) fn parse_base_pt_mod(text: &str) -> Option<(i32, i32)> { let lower = text.to_lowercase(); let tp = TextPair::new(text, &lower); diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index 9d02e16616..910c15f5bb 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -935,6 +935,7 @@ mod vanille_meld_optional_cost; mod vannifar_cloak_from_hand; mod veteran_bodyguard_tap_redirect; mod weeping_angel_combat_prevention; +mod where_x_quantity_channel_binds; mod winding_way_reveal_partition_2931; mod witchs_oven_food_tokens; mod xantid_swarm_defending_player_cant_cast; diff --git a/crates/engine/tests/integration/where_x_quantity_channel_binds.rs b/crates/engine/tests/integration/where_x_quantity_channel_binds.rs new file mode 100644 index 0000000000..c614d2d4ac --- /dev/null +++ b/crates/engine/tests/integration/where_x_quantity_channel_binds.rs @@ -0,0 +1,141 @@ +//! CR 107.3c — the QUANTITY channel of "where X is …" must resolve to the real +//! value, not 0. +//! +//! Sibling of `bishop_of_binding_where_x_exiled_card_power` (harvest #48, which +//! covered the P/T channel). When a "where X is " clause defined X with an +//! expression the parser could not type, the quantity channel fabricated +//! `QuantityRef::Variable { name: "" }` and carried on. That +//! node is well-typed and renders as a supported dynamic quantity in the +//! coverage report, but `game/quantity.rs` dispatches the non-`"X"` `Variable` +//! arm through `state.last_named_choice` and `.unwrap_or(0)` — so the quantity +//! read 0 (or, worse, an unrelated number left behind by some earlier "choose a +//! number"). 93 faces / 74 distinct expressions carried such a node pool-wide. +//! +//! These are RUNTIME witnesses, not AST-shape assertions: each one parses the +//! card's real Oracle clause through the production parser, then hands the +//! resulting `QuantityExpr` to the live resolver (`game::quantity::resolve_quantity`) +//! against a game state where the referenced value is actually set. If the +//! binding is dropped, the resolver returns 0 — which is exactly what each +//! assertion discriminates against. +//! +//! Oracle text below is read from the card export, not from memory. + +use engine::game::quantity::resolve_quantity; +use engine::game::scenario::{GameScenario, P0}; +use engine::parser::parse_oracle_text; +use engine::types::ability::{ChosenAttribute, Effect, QuantityExpr}; +use engine::types::phase::Phase; + +/// Pull the single quantity a parsed one-clause ability carries. +/// +/// Deliberately matches only the effects these witnesses use, so a parse that +/// silently lowers to something else (an `Unimplemented` gap, say) fails loudly +/// here instead of being skipped. +fn only_quantity(oracle: &str) -> QuantityExpr { + let parsed = parse_oracle_text(oracle, "~", &[], &["Creature".to_string()], &[]); + let def = parsed + .abilities + .first() + .unwrap_or_else(|| panic!("no ability parsed from {oracle:?}")); + match &*def.effect { + Effect::GainLife { amount, .. } + | Effect::LoseLife { amount, .. } + | Effect::DealDamage { amount, .. } => amount.clone(), + Effect::Scry { count, .. } | Effect::Draw { count, .. } => count.clone(), + other => panic!("unexpected effect for {oracle:?}: {other:?}"), + } +} + +/// Alchemy intensity (Arek, False Goldwarden; Legion's Chant; Mycelic Ballad). +/// `QuantityRef::Intensity { scope: Source }` reads the source object's live +/// intensity counter. Pre-fix this was `Variable("~'s intensity")` → 0, so Arek +/// drained for nothing. +#[test] +fn where_x_intensity_resolves_to_the_sources_intensity_not_zero() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + let source = scenario + .add_creature(P0, "Arek, False Goldwarden", 2, 2) + .id(); + let mut runner = scenario.build(); + + // Starting intensity 0, intensified three times. + runner + .state_mut() + .objects + .get_mut(&source) + .expect("source on battlefield") + .intensity = 3; + + let expr = only_quantity("Target opponent loses X life, where X is ~'s intensity."); + let resolved = resolve_quantity(runner.state(), &expr, P0, source); + + assert_eq!( + resolved, 3, + "X must resolve to the source's intensity (3). A raw-text \ + QuantityRef::Variable resolves to 0 — a silent no-op that still reads as \ + supported in the coverage report. Got {resolved} from {expr:?}" + ); +} + +/// "the chosen number" (Liquid Fire; Fluros of Myra's Marvels). +/// `QuantityRef::ChosenNumber` reads `ChosenAttribute::Number` off the source. +#[test] +fn where_x_chosen_number_resolves_to_the_chosen_number_not_zero() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + let source = scenario.add_creature(P0, "Liquid Fire", 1, 1).id(); + let mut runner = scenario.build(); + + // "As an additional cost to cast this spell, choose a number between 0 and 5." + runner + .state_mut() + .objects + .get_mut(&source) + .expect("source on battlefield") + .chosen_attributes = vec![ChosenAttribute::Number(4)]; + + let expr = only_quantity("~ deals X damage to any target, where X is the chosen number."); + let resolved = resolve_quantity(runner.state(), &expr, P0, source); + + assert_eq!( + resolved, 4, + "X must resolve to the number the player actually chose (4), not 0. \ + Got {resolved} from {expr:?}" + ); +} + +/// "the amount of mana spent to cast her" (Toph, Greatest Earthbender). +/// The gendered self-anaphora is the same self-object axis as "it"/"them" +/// (CR 400.7d) — it was simply missing from the subject list, so the clause fell +/// through to a raw-text placeholder and Toph earthbent 0. +#[test] +fn where_x_mana_spent_to_cast_resolves_to_the_mana_paid_not_zero() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + let source = scenario + .add_creature(P0, "Toph, Greatest Earthbender", 3, 3) + .id(); + let mut runner = scenario.build(); + + runner + .state_mut() + .objects + .get_mut(&source) + .expect("source on battlefield") + .mana_spent_to_cast_amount = 5; + + for oracle in [ + "You gain X life, where X is the amount of mana spent to cast her.", + "You gain X life, where X is the amount of mana spent to cast it.", + "You gain X life, where X is the amount of mana spent to cast this spell.", + ] { + let expr = only_quantity(oracle); + let resolved = resolve_quantity(runner.state(), &expr, P0, source); + assert_eq!( + resolved, 5, + "X must resolve to the mana actually paid (5) for {oracle:?}, not 0. \ + Got {resolved} from {expr:?}" + ); + } +}