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
41 changes: 41 additions & 0 deletions crates/engine/src/parser/oracle_effect/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36801,6 +36801,47 @@ fn intensify_parser_maps_source_and_owned_scopes() {
));
}

/// The controller-specific "if you discard a card this way" continuation shares
/// the generic reflexive connector with the player-anaphor forms used by Chains.
/// Great Desert Hellion must retain its conditioned intensify rider.
#[test]
fn great_desert_hellion_unless_discard_keeps_conditioned_intensify() {
let parsed = parse_oracle_text(
"Starting intensity 1\nMenace\nAt the beginning of your upkeep, sacrifice Great Desert Hellion unless you discard a card. If you discard a card this way, Great Desert Hellion intensifies by 1.",
"Great Desert Hellion",
&[],
&["Creature".to_string()],
&["Hellion".to_string()],
);
let upkeep = parsed
.triggers
.iter()
.find(|trigger| matches!(trigger.phase, Some(Phase::Upkeep)))
.expect("expected Great Desert Hellion's upkeep trigger");
let intensify = upkeep
.execute
.as_deref()
.and_then(|ability| ability.sub_ability.as_deref())
.expect("expected an intensify rider after the unless-discard action");
assert!(
matches!(
intensify.effect.as_ref(),
Effect::Intensify {
scope: IntensityScope::Source,
amount: QuantityExpr::Fixed { value: 1 },
}
),
"upkeep rider must remain source intensify, got {:?}",
intensify.effect
);
assert_eq!(
intensify.condition,
Some(AbilityCondition::effect_performed()),
"intensify must be gated on the optional discard succeeding, got {:?}",
intensify.condition
);
}

#[test]
fn intensify_parser_preserves_variable_x_amount() {
let e = parse_effect("This Equipment intensifies by X, where X is that creature's power.");
Expand Down
68 changes: 68 additions & 0 deletions crates/engine/src/parser/oracle_nom/condition.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9422,6 +9422,44 @@ pub(crate) fn parse_affirmative_reflexive_connector(
tag("if the player does, "),
),
value(AbilityCondition::effect_performed(), tag("if you do, ")),
parse_discard_this_way_affirmative_connector,
))
.parse(input)
}

/// CR 608.2c + CR 701.9a: the echoed-verb affirmative form — "if [subject]
/// discard(s) a card this way, " (Chains of Mephistopheles / Magus of the
/// Chains: "If the player discards a card this way, they draw a card."). This
/// is the untyped-object sibling of `parse_you_discard_this_way_clause` (which
/// requires a typed filter and lowers to `ZoneChangedThisWay`): an unqualified
/// "a card" carries no type information to check, so the condition collapses
/// to the same "did the preceding discard occur" gate as the bare "if you
/// do, " connector. The controller-specific "if you discard ..." form shares
/// that gate: its preceding optional discard publishes `OptionalEffectPerformed`,
/// so the follow-up stays attached to the discard outcome rather than executing
/// after the sacrifice alternative.
fn parse_discard_this_way_affirmative_connector(input: &str) -> OracleResult<'_, AbilityCondition> {
alt((
value(
AbilityCondition::effect_performed(),
tag("if you discard a card this way, "),
),
value(
AbilityCondition::effect_performed(),
tag("if a player discards a card this way, "),
),
value(
AbilityCondition::effect_performed(),
tag("if they discard a card this way, "),
),
value(
AbilityCondition::effect_performed(),
tag("if that player discards a card this way, "),
),
value(
AbilityCondition::effect_performed(),
tag("if the player discards a card this way, "),
),
))
Comment on lines +9425 to 9463

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Compose the subject dimension instead of repeating full tag() strings per subject.

parse_discard_this_way_affirmative_connector and parse_discard_this_way_negated_connector each hard-code one tag() per subject ("you", "a player", "they", "that player", "the player"), duplicating the exact subject list already enumerated in parse_affirmative_reflexive_connector and parse_negated_reflexive_connector. This repeats the full compound phrase ("if a player discards a card this way, ") instead of composing an existing subject-alternative with a shared verb-phrase suffix.

If a new subject/anaphor form is added later, it must be updated in two independent places (the bare "does"/"doesn't" list and the "discard(s) a card this way" list), risking silent drift between them.

Compose the subject alternative (with its matching verb form) once, then append the fixed " discard(s)/discard(s)n't a card this way, " suffix, so both lists share one subject-to-verb-form source.

♻️ Suggested composition approach
-fn parse_discard_this_way_affirmative_connector(input: &str) -> OracleResult<'_, AbilityCondition> {
-    alt((
-        value(
-            AbilityCondition::effect_performed(),
-            tag("if you discard a card this way, "),
-        ),
-        value(
-            AbilityCondition::effect_performed(),
-            tag("if a player discards a card this way, "),
-        ),
-        value(
-            AbilityCondition::effect_performed(),
-            tag("if they discard a card this way, "),
-        ),
-        value(
-            AbilityCondition::effect_performed(),
-            tag("if that player discards a card this way, "),
-        ),
-        value(
-            AbilityCondition::effect_performed(),
-            tag("if the player discards a card this way, "),
-        ),
-    ))
-    .parse(input)
-}
+fn parse_discard_this_way_subject(input: &str) -> OracleResult<'_, &str> {
+    alt((
+        tag("you discard"),
+        tag("a player discards"),
+        tag("they discard"),
+        tag("that player discards"),
+        tag("the player discards"),
+    ))
+    .parse(input)
+}
+
+fn parse_discard_this_way_affirmative_connector(input: &str) -> OracleResult<'_, AbilityCondition> {
+    value(
+        AbilityCondition::effect_performed(),
+        (tag("if "), parse_discard_this_way_subject, tag(" a card this way, ")),
+    )
+    .parse(input)
+}

Based on learnings, the referenced documentation for this file states: "The discard-this-way condition additions should therefore be generalized across supported subject/anaphor forms and wired into the existing condition dispatcher, not implemented as card-name or full-string special cases." As per coding guidelines, crates/engine/src/parser/**/*.rs requires composing nom combinators across independent dimensions "instead of enumerating full-string permutations."

Also applies to: 9493-9523

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/engine/src/parser/oracle_nom/condition.rs` around lines 9425 - 9463,
Refactor parse_discard_this_way_affirmative_connector and
parse_discard_this_way_negated_connector to compose the existing subject
alternatives from parse_affirmative_reflexive_connector and
parse_negated_reflexive_connector with the shared discard-this-way verb suffix,
preserving each subject’s correct verb form. Remove the duplicated full tag()
permutations so both bare and discard-specific connectors derive their
subject/anaphor coverage from one source.

Sources: Coding guidelines, Path instructions

.parse(input)
}
Expand Down Expand Up @@ -9452,6 +9490,36 @@ fn parse_negated_reflexive_connector(input: &str) -> OracleResult<'_, AbilityCon
},
tag("if they don't, "),
),
parse_discard_this_way_negated_connector,
))
.parse(input)
}

/// CR 608.2c + CR 701.9a: the echoed-verb negated form — "if [subject]
/// doesn't/don't discard a card this way, " (Chains of Mephistopheles / Magus
/// of the Chains: "If the player doesn't discard a card this way, they mill a
/// card."). Untyped-object sibling of the affirmative echoed connector above;
/// mirrors the same subject set already covered by the bare negated connector.
fn parse_discard_this_way_negated_connector(input: &str) -> OracleResult<'_, AbilityCondition> {
alt((
value(
AbilityCondition::Not {
condition: Box::new(AbilityCondition::effect_performed()),
},
tag("if that player doesn't discard a card this way, "),
),
value(
AbilityCondition::Not {
condition: Box::new(AbilityCondition::effect_performed()),
},
tag("if the player doesn't discard a card this way, "),
),
value(
AbilityCondition::Not {
condition: Box::new(AbilityCondition::effect_performed()),
},
tag("if they don't discard a card this way, "),
),
))
.parse(input)
}
Expand Down
62 changes: 60 additions & 2 deletions crates/engine/src/parser/oracle_replacement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11448,8 +11448,8 @@ mod tests {
use super::*;
use crate::parser::oracle::parse_oracle_text;
use crate::types::ability::{
Comparator, ControllerRef, CountScope, QuantityExpr, QuantityModification, QuantityRef,
ReplacementCondition, ShieldKind, ZoneRef,
AbilityCondition, Comparator, ControllerRef, CountScope, QuantityExpr,
QuantityModification, QuantityRef, ReplacementCondition, ShieldKind, ZoneRef,
};
use crate::types::card_type::{CoreType, Supertype};
use crate::types::keywords::Keyword;
Expand Down Expand Up @@ -20466,6 +20466,64 @@ mod tests {
);
}

/// Issue #5653 + CR 608.2c: the full Chains of Mephistopheles / Magus of
/// the Chains text carries two trailing sentences after the "discards a
/// card instead" antecedent — "If the player discards a card this way,
/// they draw a card. If the player doesn't discard a card this way, they
/// mill a card." Both must attach as typed `EffectOutcome` gates on the
/// execute chain's Draw/Mill sub-abilities, not run unconditionally.
#[test]
fn parses_chains_full_text_gates_draw_and_mill_on_discard_outcome() {
let def = parse_replacement_line(
"If a player would draw a card except the first one they draw in each of their draw steps, that player discards a card instead. If the player discards a card this way, they draw a card. If the player doesn't discard a card this way, they mill a card.",
"Chains of Mephistopheles",
)
.expect("Chains draw replacement must parse");

let discard = def.execute.as_deref().expect("execute chain present");
assert!(
matches!(&*discard.effect, Effect::Discard { .. }),
"the antecedent effect must be Discard, got {:?}",
discard.effect
);

let draw = discard
.sub_ability
.as_deref()
.expect("Draw sub-ability must be present");
assert!(
matches!(&*draw.effect, Effect::Draw { .. }),
"the first follow-on effect must be Draw, got {:?}",
draw.effect
);
assert_eq!(
draw.condition,
Some(AbilityCondition::effect_performed()),
"\"if the player discards a card this way\" must gate Draw on \
whether the discard actually happened, got {:?}",
draw.condition
);

let mill = draw
.sub_ability
.as_deref()
.expect("Mill sub-ability must be present");
assert!(
matches!(&*mill.effect, Effect::Mill { .. }),
"the second follow-on effect must be Mill, got {:?}",
mill.effect
);
assert_eq!(
mill.condition,
Some(AbilityCondition::Not {
condition: Box::new(AbilityCondition::effect_performed())
}),
"\"if the player doesn't discard a card this way\" must gate Mill \
on the discard having failed, got {:?}",
mill.condition
);
}

#[test]
fn parses_opponent_mill_replacement_with_multiplier() {
let text =
Expand Down
2 changes: 1 addition & 1 deletion crates/engine/tests/fixtures/integration_cards.json

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
@@ -0,0 +1,174 @@
//! Runtime pipeline regression for issue #5653 — Chains of Mephistopheles /
//! Magus of the Chains.
//!
//! Oracle text: "If a player would draw a card except the first one they draw
//! in each of their draw steps, that player discards a card instead. If the
//! player discards a card this way, they draw a card. If the player doesn't
//! discard a card this way, they mill a card."
//!
//! The draw and mill branches are mutually exclusive on whether the discard
//! actually removed a card: a nonempty hand discards then draws (no mill); an
//! empty hand fails to discard, so the player mills instead of drawing (no
//! draw). Each scenario stocks the library with TWO cards so an unconditional
//! chain (both Draw and Mill firing regardless of the discard's outcome) is
//! distinguishable from the correct either/or behavior — with only one library
//! card, an unconditional Draw would exhaust the library before Mill could act,
//! making the bug invisible.

use engine::game::scenario::{GameScenario, P0, P1};
use engine::game::scenario_db::GameScenarioDbExt;
use engine::types::actions::{DebugAction, GameAction};
use engine::types::phase::Phase;
use engine::types::zones::Zone;

use crate::support::shared_card_db as load_db;

fn card_names(
state: &engine::types::game_state::GameState,
ids: impl Iterator<Item = engine::types::identifiers::ObjectId>,
) -> Vec<String> {
ids.filter_map(|id| state.objects.get(&id).map(|o| o.name.clone()))
.collect()
}

/// CR 121.1 + CR 614.1a: with a card in hand, the replaced draw discards it,
/// then — because the discard succeeded — the player draws a replacement
/// card, and the mill branch must NOT also run. The library carries a second
/// card so an unconditional chain (Draw AND Mill both firing) would leave the
/// library empty and put a second card in the graveyard; only the correct
/// either/or behavior leaves "Sol Ring" untouched in the library.
#[test]
fn chains_nonempty_hand_discards_then_draws_no_mill() {
let db = load_db().expect("shared card database must be available for this integration test");

let mut scenario = GameScenario::new();
scenario.at_phase(Phase::PreCombatMain);
scenario.add_real_card(P0, "Chains of Mephistopheles", Zone::Battlefield, db);
scenario.add_real_card(P0, "Grizzly Bears", Zone::Hand, db);
// Library front-to-back: [Hill Giant, Sol Ring]. A correct single draw
// takes only Hill Giant, leaving Sol Ring in the library.
scenario.add_real_card(P0, "Hill Giant", Zone::Library, db);
scenario.add_real_card(P0, "Sol Ring", Zone::Library, db);
// P1 needs *some* library so SBAs don't fire.
for _ in 0..5 {
scenario.add_real_card(P1, "Plains", Zone::Library, db);
}
let mut runner = scenario.build();
engine::game::rehydrate_game_from_card_db(runner.state_mut(), db);
runner.state_mut().debug_mode = true;

runner
.act(GameAction::Debug(DebugAction::DrawCards {
player_id: P0,
count: 1,
}))
.expect("debug draw must succeed");
runner.advance_until_stack_empty();

let hand_names = card_names(
runner.state(),
runner.state().players[0].hand.iter().copied(),
);
assert_eq!(
hand_names,
vec!["Hill Giant".to_string()],
"the discard-then-draw branch must swap Grizzly Bears for the drawn \
Hill Giant (net hand size unchanged); got {hand_names:?}"
);

let graveyard_names = card_names(
runner.state(),
runner.state().players[0].graveyard.iter().copied(),
);
assert_eq!(
graveyard_names,
vec!["Grizzly Bears".to_string()],
"exactly the discarded card must be in the graveyard — a second card \
here would mean the mill branch ran alongside the draw branch \
instead of being mutually exclusive with it; got {graveyard_names:?}"
);

let library_names = card_names(
runner.state(),
runner.state().players[0].library.iter().copied(),
);
assert_eq!(
library_names,
vec!["Sol Ring".to_string()],
"only Hill Giant may leave the library (the single draw) — Sol Ring \
must remain; a missing Sol Ring would mean mill also fired and \
consumed it; got {library_names:?}"
);
}

/// CR 121.1 + CR 614.1a: with an empty hand, the replaced draw has nothing to
/// discard, so the discard fails — and because it failed, the player mills a
/// card instead of drawing. The library carries a second card so an
/// unconditional chain (Draw AND Mill both firing) would draw one card into
/// hand AND mill the other; only the correct either/or behavior leaves the
/// hand empty and "Sol Ring" untouched in the library.
#[test]
fn chains_empty_hand_fails_discard_then_mills_no_draw() {
let db = load_db().expect("shared card database must be available for this integration test");

let mut scenario = GameScenario::new();
scenario.at_phase(Phase::PreCombatMain);
scenario.add_real_card(P0, "Chains of Mephistopheles", Zone::Battlefield, db);
// Library front-to-back: [Hill Giant, Sol Ring]. A correct mill takes only
// Hill Giant (the top card), leaving Sol Ring in the library.
scenario.add_real_card(P0, "Hill Giant", Zone::Library, db);
scenario.add_real_card(P0, "Sol Ring", Zone::Library, db);
// P1 needs *some* library so SBAs don't fire.
for _ in 0..5 {
scenario.add_real_card(P1, "Plains", Zone::Library, db);
}
let mut runner = scenario.build();
engine::game::rehydrate_game_from_card_db(runner.state_mut(), db);
runner.state_mut().debug_mode = true;

assert!(
runner.state().players[0].hand.is_empty(),
"test setup must start with an empty P0 hand"
);

runner
.act(GameAction::Debug(DebugAction::DrawCards {
player_id: P0,
count: 1,
}))
.expect("debug draw must succeed");
runner.advance_until_stack_empty();

let hand_names = card_names(
runner.state(),
runner.state().players[0].hand.iter().copied(),
);
assert!(
hand_names.is_empty(),
"an empty hand cannot discard, so the draw branch must NOT fire \
either — the hand must stay empty; got {hand_names:?}"
);

let graveyard_names = card_names(
runner.state(),
runner.state().players[0].graveyard.iter().copied(),
);
assert_eq!(
graveyard_names,
vec!["Hill Giant".to_string()],
"the mill branch must move exactly the top library card to the \
graveyard; got {graveyard_names:?}"
);

let library_names = card_names(
runner.state(),
runner.state().players[0].library.iter().copied(),
);
assert_eq!(
library_names,
vec!["Sol Ring".to_string()],
"only Hill Giant may leave the library (the single mill) — Sol Ring \
must remain; a missing Sol Ring would mean the draw branch also \
fired and consumed it; got {library_names:?}"
);
}
1 change: 1 addition & 0 deletions crates/engine/tests/integration/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ mod case_solve_condition;
mod cast_during_resolution_pipeline;
mod cda_counted_quantities_pt;
mod chain_of_smog_copy;
mod chains_of_mephistopheles_discard_draw_or_mill;
mod chandra_revolution_doesnt_untap_slot;
mod charging_cinderhorn_issue_2868;
mod chatterstorm_storm;
Expand Down
Loading