fix(parser): recognize "~'s loyalty" as a source loyalty-counter quantity (Nissa Ascended Animist)#6045
Conversation
…tity Closes phase-rs#6012. Nissa, Ascended Animist — '[+1]: Create an X/X green Phyrexian Horror creature token, where X is Nissa's loyalty.' — created no token. After card-name normalization the effect quantity is '~'s loyalty', but the self-possessive quantity leaf had arms only for power and toughness, so parse_cda_quantity returned None, the token clause failed to lower, and the +1 degraded to Effect::Unimplemented. Add parse_self_loyalty_ref, composed from the shared parse_self_possessive prefix + tag(' loyalty'), mapping every self-referent phrasing to the existing QuantityRef::CountersOn { scope: Source, counter_type: Loyalty }. CR 306.5c: a planeswalker's loyalty equals the number of loyalty counters on it. Runtime already resolves counter-on-source quantities and token P/T against the ability source — no new engine infra. Whole-DB regen: 2 cards fixed. Nissa +1 now lowers to Effect::Token (Phyrexian Horror, P/T = source loyalty) instead of Unimplemented; and Nahiri, the Unforgiving [0] ('exile ... with mana value less than Nahiri's loyalty') now keeps its previously-dropped CMC constraint. Runtime regression drives Nissa's +1 at 5 loyalty and asserts a 5/5 Horror token.
superagent-security
Bot
added
the
contributor:flagged
|
🚨 Contributor flagged. Click here for more info: Superagent Dashboard |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for parsing self-possessive loyalty references (e.g., '~'s loyalty') in Oracle text, resolving an issue where Nissa, Ascended Animist's +1 ability failed to create a token. The changes include updates to the quantity parser, a new unit test, and an integration test. The reviewer suggests refactoring the existing self-possessive characteristic parsers into a single parameterized function to improve maintainability and adhere to the repository's 'parameterize, don't proliferate' style rule.
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.
| /// Parse "its loyalty" / "~'s loyalty" / "this creature's loyalty" / | ||
| /// "this card's loyalty" — the loyalty of the ability's source planeswalker. | ||
| /// | ||
| /// CR 306.5c: The loyalty of a planeswalker on the battlefield is equal to the | ||
| /// number of loyalty counters on it, so a source-scoped loyalty reference is the | ||
| /// count of `Loyalty` counters on the source (Nissa, Ascended Animist: | ||
| /// "Create an X/X green Phyrexian Horror creature token, where X is ~'s | ||
| /// loyalty"). Composed from the shared `parse_self_possessive` prefix so every | ||
| /// self-referent phrasing routes to `CountersOn { Source, Loyalty }`, mirroring | ||
| /// `parse_self_power_ref`. | ||
| fn parse_self_loyalty_ref(input: &str) -> OracleResult<'_, QuantityRef> { | ||
| let (rest, _) = parse_self_possessive(input)?; | ||
| let (rest, _) = tag(" loyalty").parse(rest)?; | ||
| Ok(( | ||
| rest, | ||
| QuantityRef::CountersOn { | ||
| scope: crate::types::ability::ObjectScope::Source, | ||
| counter_type: Some(CounterType::Loyalty), | ||
| }, | ||
| )) | ||
| } |
There was a problem hiding this comment.
With the addition of parse_self_loyalty_ref, there are now three similar functions for parsing self-possessive characteristics (..._power_ref, ..._toughness_ref, ..._loyalty_ref). This introduces a 'sibling-cluster smell' as per rule R3 of the repository style guide, which advises to 'Parameterize, don't proliferate'.
Before extending this pattern, consider refactoring these three functions into a single parameterized one. This would reduce code duplication and make it easier to add other self-possessive characteristics in the future.
For example, you could implement a single parser:
fn parse_self_characteristic_ref(input: &str) -> OracleResult<', QuantityRef> {
let (rest, ) = parse_self_possessive(input)?;
let (rest, characteristic) = alt((
map(tag(" power"), || QuantityRef::Power { scope: ObjectScope::Source }),
map(tag(" toughness"), || QuantityRef::Toughness { scope: ObjectScope::Source }),
map(tag(" loyalty"), |_| QuantityRef::CountersOn { scope: ObjectScope::Source, counter_type: Some(CounterType::Loyalty) }),
)).parse(rest)?;
Ok((rest, characteristic))
}
This new function could then replace the alt((...)) block at lines 752-756 with a single call, which would also remove the need for nesting to avoid nom's arity limit.
References
- Avoid verbatim string equality for parsing Oracle phrases. Instead, decompose compound phrases into modular, reusable parsers and compose them using idiomatic combinator aggregates to prevent combinatorial explosion and improve maintainability.
Co-authored-by: dhgoal <153369624+dhgoal@users.noreply.github.com>
Parse changes introduced by this PR · 2 card(s), 3 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved: the self-characteristic nom parser now maps source loyalty to its existing typed CountersOn { Source, Loyalty } representation; the Nissa activation regression exercises the post-cost runtime path.
2 participants
Tier: Frontier
Model: claude-opus-4-8
Thinking: high
Closes #6012.
Summary
Nissa, Ascended Animist — "[+1]: Create an X/X green Phyrexian Horror creature token, where X is Nissa's loyalty." — created no token at all.
After card-name normalization the effect quantity is
~'s loyalty. The self-possessive quantity leaf inparse_quantity_refhad arms for~'s powerand~'s toughnessbut none for loyalty, soparse_cda_quantityreturnedNone,try_parse_tokenbailed (its CR 107.3c guard against a 0/0 dead node), and the whole[+1]degraded toEffect::Unimplemented.Fix: add
parse_self_loyalty_ref, composed from the sharedparse_self_possessiveprefix +tag(" loyalty"), mapping every self-referent phrasing (its loyalty/~'s loyalty/this card's loyalty) to the existingQuantityRef::CountersOn { scope: Source, counter_type: Some(Loyalty) }. Per CR 306.5c, a planeswalker's loyalty equals the number of loyalty counters on it. The runtime already resolves counter-on-source quantities and already resolves token P/T against the ability source — no new engine infra. Class-level, not a one-off.Gate A
./scripts/check-parser-combinators.sh— clean.parse_self_loyalty_refis pure nom (parse_self_possessive+tag); registered by nesting the power/toughness/loyalty arms into a sub-alt(to stay within nom 8.0's 21-item tuple arity). Three-dot diff ofcrates/engine/src/parser/**grepped for forbidden methods: empty.Anchored on
crates/engine/src/parser/oracle_nom/quantity.rs— newparse_self_loyalty_ref+ its registration inparse_quantity_ref.QuantityRef::CountersOn { scope: Source, counter_type: Loyalty }— the existing referent it maps to;resolve_counters_on_scopealready reads loyalty counters on the source, andbuild_token_attrs_from_effect → resolve_pt_valuealready sizes the token from it.Verification
cargo fmt --all— clean.cargo clippy -p engine --tests -- -D warnings— clean.issue_6012_nissa_plus_one_creates_horror_token_sized_to_loyaltyincrates/engine/tests/integration/nissa_ascended_animist_loyalty_token_6012.rs(registered intests/integration/main.rs) — builds Nissa with 5 loyalty, resolves[+1], asserts a Horror token with power/toughness = 5, and guards the body is notUnimplemented. Fails before, passes after. Plus a building-block parser unit test (~'s loyalty/its loyalty/this card's loyalty→CountersOn{Source, Loyalty}).docs/MagicCompRules.txt: 306.5c (loyalty = loyalty counters).oracle-gen, regeneratedcard-data.json, diffed the full database — exactly 2 cards changed, both correct improvements: Nissa, Ascended Animist ([+1]→Effect::Token, P/T = source loyalty, wasUnimplemented); and Nahiri, the Unforgiving[0]— "exile … with mana value less than Nahiri's loyalty" now keeps its previously-dropped CMC constraint. No collateral.Scope Expansion
None.