fix(parser): scope 'creatures that died under your control' count to the controller (#1129) - #3154
Conversation
There was a problem hiding this comment.
Code Review
This pull request addresses issue #1129 by updating the parser to correctly scope the controller (e.g., Some(ControllerRef::You)) for phrases like "creatures that died under your control" or "put into your graveyard", while leaving unqualified phrases unscoped. It also adds corresponding unit and integration tests. The reviewer noted that parse_creatures_died_this_turn_tail violates the repository style guide by enumerating the Cartesian product of phrase variants as separate tag arms instead of composing them using alt() per axis. A refactoring suggestion was provided to compose these axes modularly.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| fn parse_creatures_died_this_turn_tail(input: &str) -> OracleResult<'_, Option<ControllerRef>> { | ||
| alt(( | ||
| value((), tag("creatures that died under your control this turn")), | ||
| value((), tag("creatures that died under your control")), | ||
| value((), tag("creatures that died this turn")), | ||
| value((), tag("creatures that died")), | ||
| value((), tag("creature that died under your control this turn")), | ||
| value((), tag("creature that died under your control")), | ||
| value((), tag("creature that died this turn")), | ||
| value((), tag("creature that died")), | ||
| value( | ||
| Some(ControllerRef::You), | ||
| tag("creatures that died under your control this turn"), | ||
| ), | ||
| value( | ||
| Some(ControllerRef::You), | ||
| tag("creatures that died under your control"), | ||
| ), | ||
| value(None, tag("creatures that died this turn")), | ||
| value(None, tag("creatures that died")), | ||
| value( | ||
| Some(ControllerRef::You), | ||
| tag("creature that died under your control this turn"), | ||
| ), | ||
| value( | ||
| Some(ControllerRef::You), | ||
| tag("creature that died under your control"), | ||
| ), | ||
| value(None, tag("creature that died this turn")), | ||
| value(None, tag("creature that died")), | ||
| // CR 700.4: "creature put into [a/your] graveyard from the battlefield" | ||
| // is the long form of "died" — both reference the same battlefield→ | ||
| // graveyard transition tracked in `zone_changes_this_turn`. | ||
| // graveyard transition tracked in `zone_changes_this_turn`. CR 109.5: | ||
| // "your" graveyard scopes the count to the source's controller. | ||
| value( | ||
| (), | ||
| Some(ControllerRef::You), | ||
| tag("creatures put into your graveyard from the battlefield this turn"), | ||
| ), | ||
| value( | ||
| (), | ||
| Some(ControllerRef::You), | ||
| tag("creatures put into your graveyard from the battlefield"), | ||
| ), | ||
| value( | ||
| (), | ||
| None, | ||
| tag("creatures put into a graveyard from the battlefield this turn"), | ||
| ), | ||
| value( | ||
| (), | ||
| None, | ||
| tag("creatures put into a graveyard from the battlefield"), | ||
| ), | ||
| value( | ||
| (), | ||
| Some(ControllerRef::You), | ||
| tag("creature put into your graveyard from the battlefield this turn"), | ||
| ), | ||
| value( | ||
| (), | ||
| Some(ControllerRef::You), | ||
| tag("creature put into your graveyard from the battlefield"), | ||
| ), | ||
| value( | ||
| (), | ||
| None, | ||
| tag("creature put into a graveyard from the battlefield this turn"), | ||
| ), | ||
| value( | ||
| (), | ||
| None, | ||
| tag("creature put into a graveyard from the battlefield"), | ||
| ), | ||
| )) |
There was a problem hiding this comment.
[HIGH] Avoid enumerating the Cartesian product of phrase variants as separate tag arms.
Why it matters: The Repository Style Guide (Parser section) strictly prohibits enumerating N-dimensional patterns as separate tag("full string") arms. Instead, they should be composed using alt() per axis to prevent combinatorial explosion and maintain idiomatic, modular parsers.
Suggested fix: Refactor parse_creatures_died_this_turn_tail to compose the subject number, verb phrasing, controller qualification, and temporal qualification axes.
fn parse_creatures_died_this_turn_tail(input: &str) -> OracleResult<'_, Option<ControllerRef>> {
preceded(
alt((tag("creatures"), tag("creature"))),
terminated(
alt((
map(
preceded(
tag(" that died"),
opt(tag(" under your control")),
),
|opt_control| opt_control.map(|_| ControllerRef::You),
),
preceded(
tag(" put into "),
terminated(
alt((
value(Some(ControllerRef::You), tag("your")),
value(None, tag("a")),
)),
tag(" graveyard from the battlefield"),
),
),
)),
opt(tag(" this turn")),
),
)References
- Parser rules require N-dimensional patterns to compose
alt()per axis instead of enumerating the Cartesian product as separatetagarms. (link) - Avoid verbatim string equality for parsing Oracle phrases. Decompose compound phrases into modular, reusable parsers and compose them using idiomatic combinator aggregates to prevent combinatorial explosion.
c491a88 to
8e66066
Compare
8e66066 to
46bd78c
Compare
Summary
Closes #1129.
Priest of the Crossing's "for each creature that died under your control this turn" counted every player's creature deaths.
creatures_died_this_turn_ref()(oracle_nom/quantity.rs) always emittedZoneChangeCountThisTurn{ filter: TypedFilter::creature() }with no controller restriction, becauseparse_creatures_died_this_turn_tailcollapsed both the controller-qualified ("under your control", "your graveyard") and unqualified ("creatures that died this turn", "a graveyard") phrasings into the same unit value.Class fix (mirrors the already-correct
parse_for_each_creature_left_battlefield_this_turn): threadOption<ControllerRef>through the tail so qualified forms emit.controller(ControllerRef::You)and unqualified forms keep no restriction (counts all). The runtime resolver already honors the controller filter (zone_change_filter_inner, CR 109.5) — only the parser was dropping it. ~16 "died under your control" + ~8 "your graveyard" cards corrected; unqualified all-players cards unchanged.Tests
priest_of_the_crossing_died_under_control.rs: P0's + P1's creatures both die; Priest counts only P0's (1, not 2). Fails on revert.controller == Some(You), unqualified →None(bothfor eachandthe number ofsurfaces).CR 700.4, 109.5 (grep-verified). Verified locally via the parser combinator gate; full verification deferred to CI (local main checkout was transiently stale).