fix(parser): parse repeated named control clauses#5114
Conversation
![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 enhances the condition parser in crates/engine/src/parser/oracle_nom/condition.rs to support disjunctions in 'you control [type] named [Name1] or a [type] named [Name2]' phrases, resolving backlog issues for cards like High Marshal Arguel and Liu Bei. The feedback suggests expanding the list of condition terminators in parse_control_named_condition_terminator to prevent name parser overruns, and adding mandatory CR annotations to the new rules-touching helper functions per Rule R6.
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_control_named_condition_terminator( | ||
| input: &str, | ||
| ) -> Result<(&str, ()), nom::Err<OracleError<'_>>> { | ||
| alt(( | ||
| value((), tag(".")), | ||
| value( | ||
| (), | ||
| ( | ||
| tag(", "), | ||
| alt(( | ||
| value((), eof), | ||
| value((), tag("instead")), | ||
| value((), tag("then")), | ||
| value((), tag("do")), | ||
| value((), tag("draw")), | ||
| value((), tag("create")), | ||
| value((), tag("put")), | ||
| value((), tag("sacrifice")), | ||
| value((), tag("transform")), | ||
| value((), tag("you ")), | ||
| value((), tag("target ")), | ||
| )), | ||
| ), | ||
| ), | ||
| )) | ||
| .parse(input) | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Expand the list of condition terminators to prevent overrunning the name parser.
Why it matters: The current list of terminators in parse_control_named_condition_terminator is missing several common MTG action verbs (like exile, destroy, return, discard) and pronouns (like it , its , their , each , all , choose ). If a card's effect starts with one of these words after the comma, the name parser will overrun and consume the effect as part of the card name.
Suggested fix: Add these common keywords to the inner alt block.
fn parse_control_named_condition_terminator(
input: &str,
) -> Result<(&str, ()), nom::Err<OracleError<'_>>> {
alt((
value((), tag(".")),
value(
(),
(
tag(", "),
alt((
value((), eof),
value((), tag("instead")),
value((), tag("then")),
value((), tag("do")),
value((), tag("draw")),
value((), tag("create")),
value((), tag("put")),
value((), tag("sacrifice")),
value((), tag("transform")),
value((), tag("you ")),
value((), tag("target ")),
value((), tag("exile")),
value((), tag("destroy")),
value((), tag("return")),
value((), tag("discard")),
value((), tag("it ")),
value((), tag("its ")),
value((), tag("their ")),
value((), tag("each ")),
value((), tag("all ")),
value((), tag("choose ")),
)),
),
),
))
.parse(input)
}References
- Avoid verbatim string equality 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. (link)
| /// Consume a named-card member up to a guarded condition boundary. Bare commas | ||
| /// stay inside the name so legendary names such as "Guan Yu, Sainted Warrior" | ||
| /// survive, while a comma followed by an effect lead remains available to the | ||
| /// caller as the condition terminator. | ||
| fn parse_control_named_final_name(input: &str) -> OracleResult<'_, &str> { |
There was a problem hiding this comment.
[MEDIUM] Add CR annotations to the new rules-touching helper functions.
Why it matters: Rule R6 of the repository style guide requires every rules-touching line of engine code to carry a comment of the form CR <number>: <description>. The new helper functions (like parse_control_named_final_name, find_repeated_typed_control_named_pair, etc.) implement card name and condition clause parsing under CR 201.2 and CR 603.4 but lack these annotations.
Suggested fix: Add CR 201.2 or CR 603.4 annotations to the documentation comments of these new helper functions.
References
- Rule R6. CR annotations are mandatory and verified. Every rules-touching line of engine code must carry a comment of the form CR : . (link)
Parse changes introduced by this PR · 3 card(s), 3 signature(s) (baseline: main
|
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
…named-clauses # Conflicts: # docs/parser-misparse-backlog.md
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Thanks for the update. I found three remaining parser/scope blockers:
[MED] Comma-led add effects now overrun into the second named card. Evidence: crates/engine/src/parser/oracle_nom/condition.rs:745 terminator list lacks add , while Tower Worker has If you control creatures named Mine Worker and Power Plant Worker, add {C}{C}{C} instead. Why it matters: the new guarded comma boundary regresses a printed shared-type named-control clause that the old comma stop would have split. Suggested fix: terminate on , add and add a discriminating Tower Worker-style parser test.
[MED] The named-control parser still materializes exactly two named predicates, so three-name lists collapse names. Evidence: crates/engine/src/parser/oracle_nom/condition.rs:692 returns only (first, second); Helm of Kaldra has Equipment named Helm of Kaldra, Sword of Kaldra, and Shield of Kaldra. Why it matters: the touched seam handles the class as a pair parser, leaving a same-class printed clause misparsed. Suggested fix: parse a named-member list into N predicates, preserving shared-type and repeated-typed members.
[MED] The sticky parse-diff contains an unexplained out-of-scope parser change. Evidence: the coverage-parse-diff sticky reports Say Its Name added cost/two other cards named Say Its Name from your graveyard, while the PR body claims only High Marshal Arguel and Liu Bei changed. Why it matters: parser PRs need the card-level diff reconciled; unexplained gained/changed signatures are possible blast radius. Suggested fix: reconcile the full sticky diff by explaining/regenerating the baseline or eliminating the unrelated Say Its Name change.
2 participants
Summary
you control ... named ...condition parser so repeated typed named clauses keep their own type/name pairs.Root cause
parse_control_named_pairhandledyou control artifacts named A and Bby sharing one parsed type across both names. That same path over-consumed repeated typed clauses:Enchantmentnamed"a land named temple of aclazotz".Namedstring.Parse audit
Focused before/after export for root #30 changed exactly these cards:
high marshal arguelliu bei, lord of shuRaw JSON deltas are intentional:
Landnamed"temple of aclazotz"instead ofEnchantmentnamed"a land named temple of aclazotz".Or(IsPresent permanent named Guan Yu, IsPresent permanent named Zhang Fei)instead of one overrunNamedvalue.coverage-parse-diffon the same focused exports reported no card-parse changes detected.Review cleanup
Addressed current Gemini review comments in
9b89e5533:CR 201.2,CR 603.4).Verification
cargo fmt --all./scripts/check-parser-combinators.shcargo test -p engine --features cli --lib control_named -- --nocapturecargo test -p engine --features cli --lib repeated_typed_named_pair -- --nocapturecargo clippy -p engine --all-targets -- -D warningsoracle-genbefore/after export for root chore: update coverage stats and badges #30coverage-parse-diff /tmp/phase-root30-search-named-or-before.json /tmp/phase-root30-control-named-after.jsoncargo fmt --all,./scripts/check-parser-combinators.sh,cargo test -p engine --features cli --lib control_named -- --nocapture,cargo test -p engine --features cli --lib repeated_typed_named_pair -- --nocapture,cargo clippy -p engine --all-targets -- -D warnings,git diff --checkNote: full workspace
cargo clippy --all-targets -- -D warningsis blocked on this host by missingpkg-config/OpenSSL for non-engine targets; the engine crate clippy gate passed.