fix(parser): accept "of your choice ... in addition to its other types" (Navigator's Compass)#5860
Conversation
…s" (Navigator's Compass) try_parse_become_choice anchored the choice phrase on literally ending in "of your choice", so a trailing "in addition to its other types" retention marker (Navigator's Compass: "becomes the basic land type of your choice in addition to its other types") made the whole predicate fall through unparsed — the fixed-value sibling path already handles this exact marker via has_in_addition_to_other_types/has_in_addition_to_other_colors, but the choice form never did. Reuse the existing split_in_addition_tail splitter to peel the marker off before the anchor check; AddChosenSubtype is additive by construction (CR 205.1b) so accepting it changes only whether the line parses, not the emitted modification. Fixes phase-rs#5859. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![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 updates the Oracle parser to support the "in addition to its other types" retention marker within choice phrases by exposing and utilizing the split_in_addition_tail helper. The reviewer feedback correctly points out that this manual string-splitting approach violates the repository's hard architectural rule (R1) requiring nom combinators for parsing, and suggests refactoring the logic into modular, reusable nom-based parsers.
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.
| // CR 205.1b: the choice phrase may itself carry the "in addition to its | ||
| // other types" retention marker (Navigator's Compass: "becomes the basic | ||
| // land type of your choice in addition to its other types") — the same | ||
| // marker the fixed-value sibling path already recognizes via | ||
| // `has_in_addition_to_other_types` (see `build_become_clause`'s | ||
| // `parse_animation_spec` fallback). Peel it off with the shared | ||
| // `split_in_addition_tail` splitter before the "of your choice" anchor | ||
| // check below, so the choice phrase underneath is still recognized instead | ||
| // of the whole predicate falling through unparsed. `AddChosenSubtype` (the | ||
| // land/creature-type modification below) is additive by construction | ||
| // regardless of the marker, so no branching on the match is needed — it | ||
| // only needs to be accepted, not interpreted. | ||
| let choice_text = match split_in_addition_tail(choice_text) { | ||
| Some((prefix, _matched)) => prefix.trim(), | ||
| None => choice_text, | ||
| }; |
There was a problem hiding this comment.
[HIGH] Fragile manual string splitting and parsing of Oracle phrases.
The current implementation relies on manual string manipulation (split_around, split_in_addition_tail) to parse complex choice phrases and keyword grants. This approach is highly fragile and prone to parsing failures, such as losing trailing keyword grants when combined with the 'in addition to its other types' marker.
Instead of patching this with additional manual string splitting (which bypasses the robust parser and creates fragile matches), we should decompose these compound phrases into modular, reusable nom-based parsers. Use idiomatic combinator aggregates (like nested alt and tag sequences) to parse the constituent parts (subjects, conjunctions, and keyword grants) to prevent combinatorial explosion and improve maintainability.
| // CR 205.1b: the choice phrase may itself carry the "in addition to its | |
| // other types" retention marker (Navigator's Compass: "becomes the basic | |
| // land type of your choice in addition to its other types") — the same | |
| // marker the fixed-value sibling path already recognizes via | |
| // `has_in_addition_to_other_types` (see `build_become_clause`'s | |
| // `parse_animation_spec` fallback). Peel it off with the shared | |
| // `split_in_addition_tail` splitter before the "of your choice" anchor | |
| // check below, so the choice phrase underneath is still recognized instead | |
| // of the whole predicate falling through unparsed. `AddChosenSubtype` (the | |
| // land/creature-type modification below) is additive by construction | |
| // regardless of the marker, so no branching on the match is needed — it | |
| // only needs to be accepted, not interpreted. | |
| let choice_text = match split_in_addition_tail(choice_text) { | |
| Some((prefix, _matched)) => prefix.trim(), | |
| None => choice_text, | |
| }; | |
| use nom::{ | |
| branch::alt, | |
| bytes::complete::tag, | |
| combinator::opt, | |
| sequence::tuple, | |
| IResult, | |
| }; | |
| fn parse_choice_and_grant(input: &str) -> IResult<&str, (Choice, Option<Grant>)> { | |
| // Decompose compound phrases into modular, reusable parsers | |
| let (input, (choice, _, _, grant)) = tuple(( | |
| parse_choice_type, | |
| tag(" in addition to its other types"), | |
| opt(tag(" and ")), | |
| opt(parse_keyword_grant), | |
| ))(input)?; | |
| Ok((input, (choice, grant))) | |
| } |
References
- R6. CR annotations are mandatory and verified. Every rules-touching line of engine code must carry a comment of the form CR : . (link)
- Avoid verbatim string equality or manual string splitting for parsing Oracle phrases as it bypasses the robust nom-based parser and creates fragile matches. Instead, decompose compound phrases into modular, reusable parsers for constituent parts and compose them using idiomatic combinator aggregates.
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 after current-head implementation review.
2 participants
Summary
Fixes #5859.
try_parse_become_choice(the "becomes the [type] of your choice" effect-clause parser) anchored the choice phrase on literally ending in "of your choice" — any trailing text after that, other than the one" and <keyword grant>"shape it already special-cased, made the whole predicate fall through unparsed.Navigator's Compass (Oracle text verified against Scryfall,
dom/225):{T}: Until end of turn, target land you control becomes the basic land type of your choice in addition to its other types.The "in addition to its other types" retention marker made this strict-fail — the activated ability was a complete no-op.The fixed-value sibling path (
build_become_clause'sparse_animation_specfallback, for "becomes a<type>" predicates) already recognizes this exact marker viahas_in_addition_to_other_types/has_in_addition_to_other_colors(Possessed Goat: "becomes a black Demon in addition to its other colors and types").try_parse_become_choice— the sibling for a chosen rather than fixed type — never picked up the same recognition.Fix
Reuse the marker splitter the fixed-value path already relies on (
split_in_addition_tail, exposed aspub(crate)fromoracle_effect/animation.rs) insidetry_parse_become_choice: peel the trailing marker off the choice phrase before theends_with_of_your_choiceanchor check, the same way the existing" and <grant>"split already runs first.AddChosenSubtype(the modification for both the creature-type and basic-land-type choice) is additive by construction (CR 205.1b) — it never clears prior subtypes — so accepting the marker changes only whether the line parses, not what modification is emitted. The color choice keeps its CR 105.3Setdefault unconditionally; no printed card pairs it with the marker, so no behavior was invented there.No new runtime: every modification (
AddChosenSubtype,AddChosenColor) and combinator (split_in_addition_tail,has_in_addition_to_other_types) already existed and was already wired for the fixed-value sibling — this only makes an existing sibling function recognize a marker its neighbor already understands.Files changed
crates/engine/src/parser/oracle_effect/animation.rs— exposesplit_in_addition_tailaspub(crate)for reuse (previously private, single in-file caller).crates/engine/src/parser/oracle_effect/subject.rs—try_parse_become_choicepeels the marker before the "of your choice" anchor check.crates/engine/src/parser/oracle_effect/tests.rs— new tests.CR references
CR 205.1b— additive type/subtype grants ("in addition to its other types").CR 305.7— land-subtype-changing effects (the basic-land-type choice).CR 205.3— choosing a creature type or basic land type.Verification
cargo fmt --package engine -- --checkon the touched files — clean../scripts/check-parser-combinators.sh(Gate A) — none of the touched files appear in its forbidden-pattern report; exit 0.split_in_addition_tail→parse_in_addition_other_types_marker) for both the Navigator's Compass predicate and the pre-existing Mondo Gecko ("...of your choice and gains hexproof...") regression case, confirming the marker is consumed correctly and the "and " path is unaffected.cargo test/clippylocally: Tilt is unreachable in this sandbox and there is no local MSVC linker or WSL distribution available as a fallback, so this repo's native toolchain cannot link on this machine. Relying on CI to run the full suite; happy to address any failures it surfaces.Tests added
become_basic_land_type_of_choice_in_addition_to_other_types— Navigator's Compass's exact predicate now parses toChoose { BasicLandType }chained toAddChosenSubtype { BasicLandType }(no spuriousRemoveAllSubtypes).become_creature_type_of_choice_in_addition_to_other_types— same marker on the creature-type axis, proving the fix generalizes rather than special-casing the land branch.become_color_of_choice_still_chains_trailing_keyword_grant— no-regression guard for the pre-existing Mondo Gecko" and <grant>"shape.🤖 Generated with Claude Code