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
143 changes: 143 additions & 0 deletions crates/engine/src/parser/oracle_effect/imperative.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7407,6 +7407,32 @@ fn filter_targets_stack(filter: &TargetFilter) -> bool {
}
}

/// CR 700.4 (#5649): match "<quantity> cards from your graveyard" and return the
/// exile count. `"that many"` → the dynamic replacement amount
/// (`QuantityRef::EventContextAmount`, Nefarious Lich); a number word → `Fixed`.
/// Anchored on the possessive "your graveyard" so it never overlaps the
/// top-of-library impulse idiom guarded by `parse_dynamic_count_phrase`.
fn parse_exile_count_from_your_graveyard(lower: &str) -> Option<QuantityExpr> {
let trimmed = lower.trim().trim_end_matches('.').trim();
let (rest, qty) = alt((
value(
QuantityExpr::Ref {
qty: crate::types::ability::QuantityRef::EventContextAmount,
},
tag::<_, _, OracleError<'_>>("that many"),
),
map(crate::parser::oracle_nom::primitives::parse_number, |n| {
QuantityExpr::Fixed { value: n as i32 }
}),
))
.parse(trimmed)
.ok()?;
let (rest, _) = tag::<_, _, OracleError<'_>>(" cards from your graveyard")
.parse(rest)
.ok()?;
rest.trim().is_empty().then_some(qty)
}

pub(super) fn parse_exile_ast(
text: &str,
lower: &str,
Expand Down Expand Up @@ -7527,6 +7553,7 @@ pub(super) fn parse_exile_ast(
),
all: true,
enter_with_counters: vec![],
multi_target: None,
});
}
// CR 205.2a + CR 205.3a + CR 608.2c: parse the full target as a
Expand Down Expand Up @@ -7554,6 +7581,7 @@ pub(super) fn parse_exile_ast(
target,
all: true,
enter_with_counters: vec![],
multi_target: None,
});
}

Expand Down Expand Up @@ -7597,6 +7625,7 @@ pub(super) fn parse_exile_ast(
target,
all: true,
enter_with_counters: vec![],
multi_target: None,
});
}

Expand Down Expand Up @@ -7633,6 +7662,7 @@ pub(super) fn parse_exile_ast(
target,
all: false,
enter_with_counters,
multi_target: None,
});
}
}
Expand Down Expand Up @@ -7664,10 +7694,33 @@ pub(super) fn parse_exile_ast(
},
all: false,
enter_with_counters: vec![],
multi_target: None,
});
}
}

// CR 700.4 (#5649): "exile <N> cards from your graveyard" is a COUNTED
// graveyard exile — Nefarious Lich's damage substitution ("exile that many
// cards from your graveyard instead"). `Effect::ChangeZone` has no count
// slot, so capture the quantity here and thread it onto the clause's
// `MultiTargetSpec` at lowering (Forage precedent). The generic tail below
// would otherwise drop the count and bind a bare `ParentTarget`.
if let Some(count) = parse_exile_count_from_your_graveyard(&rest_text.to_ascii_lowercase()) {
let mut filter = crate::types::ability::TypedFilter::default()
.controller(ControllerRef::You)
.properties(vec![crate::types::ability::FilterProp::InZone {
zone: crate::types::zones::Zone::Graveyard,
}]);
filter.type_filters = vec![crate::types::ability::TypeFilter::Card];
return Some(ZoneCounterImperativeAst::Exile {
origin: Some(crate::types::zones::Zone::Graveyard),
target: TargetFilter::Typed(filter),
all: false,
enter_with_counters: vec![],
multi_target: Some(crate::types::ability::MultiTargetSpec::exact(count)),
});
}

// CR 608.2k: thread `ctx` through so bare "it"/"them" anaphors in trigger
// bodies ("Whenever an Elf you control dies, exile it") bind to the
// triggering subject via `resolve_pronoun_target`, not the ability source.
Expand Down Expand Up @@ -7723,6 +7776,7 @@ pub(super) fn parse_exile_ast(
target,
all: false,
enter_with_counters,
multi_target: None,
})
}

Expand Down Expand Up @@ -11035,6 +11089,26 @@ pub(super) fn lower_imperative_family_ast(ast: ImperativeFamilyAst) -> ParsedEff
)));
clause
}
// CR 700.4 (#5649): a COUNTED graveyard exile ("exile that many cards
// from your graveyard", Nefarious Lich) captured a `multi_target`; the
// bare-Effect lowering below produces a `ChangeZone` that cannot carry
// the count, so thread it onto the clause here (mirroring the Tap/Untap
// arm). The runtime resolves the card selection via the shared
// multi-target picker (Forage precedent).
ImperativeFamilyAst::ZoneCounter(
ast @ ZoneCounterImperativeAst::Exile {
multi_target: Some(_),
..
},
) => {
let multi_target = match &ast {
ZoneCounterImperativeAst::Exile { multi_target, .. } => multi_target.clone(),
_ => None,
};
let mut clause = parsed_clause(lower_zone_counter_ast(ast));
clause.multi_target = multi_target;
clause
}
Comment on lines +11098 to +11111

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.

medium

[MEDIUM] Avoid redundant matching with wildcard fallbacks when the variant is already guaranteed by the outer pattern.

Why it matters: The inner match &ast uses a wildcard _ fallback which is technically unreachable and violates the exhaustive matching principle (L5). We can bind multi_target directly in the outer pattern using a reference binding, making the code cleaner and more idiomatic Rust.

        ImperativeFamilyAst::ZoneCounter(
            ast @ ZoneCounterImperativeAst::Exile {
                multi_target: Some(ref multi_target),
                ..
            },
        ) => {
            let mut clause = parsed_clause(lower_zone_counter_ast(ast));
            clause.multi_target = Some(multi_target.clone());
            clause
        }
References
  1. Rule L5: Wildcard _ match arms that should be exhaustive — let the compiler catch missing variants. (link)

// All other arms produce a bare Effect with no sub_ability chain.
other => parsed_clause(lower_imperative_family_effect(other)),
}
Expand Down Expand Up @@ -11611,6 +11685,10 @@ pub(super) fn lower_zone_counter_ast(ast: ZoneCounterImperativeAst) -> Effect {
target,
all,
enter_with_counters,
// The count (for "exile N cards from your graveyard") rides the
// clause's `MultiTargetSpec`, threaded by `lower_imperative_family_ast`
// (#5649); the bare-Effect lowering here ignores it.
multi_target: _,
} => {
if all {
// `ChangeZoneAll` has no counter slot; mass exile never carries
Expand Down Expand Up @@ -13396,6 +13474,7 @@ mod tests {
target: TargetFilter::Typed(filter),
all: false,
enter_with_counters,
multi_target: _,
}) = result
else {
panic!("{input}: expected hand-origin typed exile, got {result:?}");
Expand Down Expand Up @@ -13425,6 +13504,7 @@ mod tests {
target: TargetFilter::Or { filters },
all: true,
enter_with_counters,
multi_target: _,
}) = result
else {
panic!("{input}: expected mass exile type union, got {result:?}");
Expand All @@ -13449,6 +13529,7 @@ mod tests {
target: TargetFilter::Typed(filter),
all: true,
enter_with_counters,
multi_target: _,
}) = result
else {
panic!("{input}: expected mass exile parity filter, got {result:?}");
Expand All @@ -13474,6 +13555,7 @@ mod tests {
target: TargetFilter::Typed(filter),
all: false,
enter_with_counters: _,
multi_target: _,
}) = result
else {
panic!("{input}: expected hand-origin typed exile, got {result:?}");
Expand Down Expand Up @@ -13502,6 +13584,7 @@ mod tests {
target: TargetFilter::Typed(filter),
all: false,
enter_with_counters,
multi_target: _,
}) = result
else {
panic!("{input}: expected hand-origin typed exile, got {result:?}");
Expand Down Expand Up @@ -19497,4 +19580,64 @@ mod tests {
other => panic!("expected AssembleContraptions, got {other:?}"),
}
}

/// #5649 review (matthewevans): the `Fixed`-count arm of
/// `parse_exile_count_from_your_graveyard` fixes 8 of the 9 affected cards
/// (Aegis Sculptor / Bloodcurdler / Egon / Insatiable Frugivore / Kroxa /
/// Emeritus / Ultimecia — "exile <N> cards from your graveyard"), while the
/// "that many" arm covers Nefarious Lich. Pin both arms AND the negative that
/// the possessive "your graveyard" anchor keeps the top-of-library impulse
/// idiom (owned by `parse_dynamic_count_phrase`) untouched.
#[test]
fn exile_count_from_your_graveyard_binds_fixed_and_dynamic_counts() {
// Fixed-count arm — the eight numeric cards.
assert_eq!(
parse_exile_count_from_your_graveyard("two cards from your graveyard"),
Some(QuantityExpr::Fixed { value: 2 })
);
assert_eq!(
parse_exile_count_from_your_graveyard("eight cards from your graveyard"),
Some(QuantityExpr::Fixed { value: 8 })
);
// Dynamic arm — Nefarious Lich.
assert_eq!(
parse_exile_count_from_your_graveyard("that many cards from your graveyard"),
Some(QuantityExpr::Ref {
qty: QuantityRef::EventContextAmount
})
);
// Negatives: the possessive "your graveyard" anchor must never capture the
// top-of-library impulse idiom.
assert_eq!(
parse_exile_count_from_your_graveyard("the top two cards of your library"),
None
);
assert_eq!(
parse_exile_count_from_your_graveyard("two cards from the top of your library"),
None
);

// End-to-end: a fixed graveyard exile rides the clause's `MultiTargetSpec`.
let def = crate::parser::oracle_effect::parse_effect_chain(
"exile two cards from your graveyard",
AbilityKind::Spell,
);
assert!(
matches!(
&*def.effect,
Effect::ChangeZone {
origin: Some(crate::types::zones::Zone::Graveyard),
destination: crate::types::zones::Zone::Exile,
..
}
),
"expected a graveyard->exile ChangeZone, got {:?}",
def.effect
);
assert_eq!(
def.multi_target,
Some(MultiTargetSpec::exact(QuantityExpr::Fixed { value: 2 })),
"the count must ride the clause MultiTargetSpec as exact(Fixed {{ 2 }})"
);
}
}
3 changes: 3 additions & 0 deletions crates/engine/src/parser/oracle_effect/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13893,6 +13893,7 @@ fn try_parse_verb_and_target<'a>(
),
all: true,
enter_with_counters: vec![],
multi_target: None,
},
)),
// The matcher only claims a fully-consumed clause (trailing text
Expand All @@ -13914,6 +13915,7 @@ fn try_parse_verb_and_target<'a>(
target,
all: true,
enter_with_counters: vec![],
multi_target: None,
})),
rem,
));
Expand All @@ -13935,6 +13937,7 @@ fn try_parse_verb_and_target<'a>(
target,
all: false,
enter_with_counters: vec![],
multi_target: None,
})),
rem,
));
Expand Down
7 changes: 7 additions & 0 deletions crates/engine/src/parser/oracle_ir/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1488,6 +1488,13 @@ pub(crate) enum ZoneCounterImperativeAst {
/// ("exile a card … with N <type> counters on it"). Empty for the
/// common no-counter case. Mirrors `Effect::ChangeZone.enter_with_counters`.
enter_with_counters: Vec<(CounterType, QuantityExpr)>,
/// CR 700.4 (#5649): a counted graveyard exile — "exile <N> cards from
/// your graveyard" (Nefarious Lich: "exile that many cards … instead").
/// `Effect::ChangeZone` carries no count, so the quantity rides the
/// clause's `MultiTargetSpec` (mirroring Forage), threaded at lowering by
/// `lower_imperative_family_ast`. `None` for the ordinary single-object
/// exile.
multi_target: Option<MultiTargetSpec>,
},
ExileTop {
player: TargetFilter,
Expand Down
Loading
Loading