fix(parser): scope spell-cast triggers to a required spell keyword (#4754)#5886
Conversation
…hase-rs#4754) Slitherwisp ("Whenever you cast another spell that has flash, ...") dropped the "that has flash" clause: parse_type_phrase consumed "spell" and discarded the keyword tail, so the trigger's valid_card carried only FilterProp::Another and fired on every other spell — a non-flash counterspell wrongly triggered it. Add a parse_post_spell_modifier arm that recognizes "that has <keyword>" / "with <keyword>" and emits FilterProp::WithKeyword, gated on a fully consumed recognized keyword (parse_keyword_line_core with an empty remainder) so the numeric/colored "with mana value ..." / "with ... mana symbol" qualifiers keep their earlier arms and non-keyword tails still decline. The object matcher already honors WithKeyword via obj.has_keyword, so no runtime change is needed. Tests: a parser test (Slitherwisp -> WithKeyword(Flash) + Another) and a runtime pair proving the trigger's observable effect (each opponent loses 1 life + controller draws) fires only for a flash spell, not a plain spell.
![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 addresses issue #4754 by implementing parsing support for spell modifiers containing 'that has ' or 'with ' clauses (such as 'that has flash' on Slitherwisp), preventing spell-cast triggers from over-firing. The changes include parser updates, unit tests, and a runtime integration test. The review feedback suggests simplifying the parser logic in oracle_trigger.rs by removing redundant value combinators in favor of a simpler alt setup.
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.
| if let Ok((keyword_text, ())) = alt(( | ||
| value((), tag::<_, _, OracleError<'_>>("that has ")), | ||
| value((), tag("with ")), | ||
| )) | ||
| .parse(modifier) |
There was a problem hiding this comment.
We can simplify the parser by removing the redundant value combinators. Since both tag parsers return &str (the matched prefix), alt can directly compose them and return the matched prefix, which we can discard with a wildcard pattern _. This makes the code more concise and idiomatic.
| if let Ok((keyword_text, ())) = alt(( | |
| value((), tag::<_, _, OracleError<'_>>("that has ")), | |
| value((), tag("with ")), | |
| )) | |
| .parse(modifier) | |
| if let Ok((keyword_text, _)) = alt(( | |
| tag::<_, _, OracleError<'_>>("that has "), | |
| tag("with "), | |
| )) | |
| .parse(modifier) |
References
- Decompose compound phrases into modular, reusable parsers for constituent parts and compose them using idiomatic combinator aggregates (like nested
altandtagsequences) to prevent combinatorial explosion and improve maintainability.
Parse changes introduced by this PR · 8 card(s), 6 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Verdict: Approved
🔴 Blocker
- None.
🟡 Non-blocking
- Gemini's
value-combinator suggestion is not actionable: this parser module already usesvalue((), tag(...))to normalize alternative prefixes, which is the same explicit discard pattern used here.
✅ Clean
- The keyword predicate is added at the post-spell-modifier parser seam and reuses the strict all-consuming keyword parser.
- The eight-card parse diff is the intended shared grammar class, with no unexplained cards.
- The Slitherwisp integration tests use the real cast/trigger pipeline and discriminate both the allowed Flash and rejected non-Flash branches.
Recommendation: Merge via the queue.
2 participants
Fixes #4754
Problem
Slitherwisp reads "Flash\nWhenever you cast another spell that has flash, you draw a card and each opponent loses 1 life." The spell-cast trigger dropped the "that has flash" clause: for the payload
spell that has flash,parse_spell_qualifier_payload→parse_post_spell_modifier("that has flash")returnedNone, so the parse fell back toparse_type_phrase, which consumed "spell" and discarded "that has flash" as leftover. The trigger'svalid_cardended up with onlyFilterProp::Another, so it fired on every other spell — the reported bug: casting a counterspell (no flash) wrongly triggered Slitherwisp.Fix
Add a
parse_post_spell_modifierarm that recognizes a"that has <keyword>"/"with <keyword>"suffix and emitsFilterProp::WithKeyword { value }. It is gated on a fully consumed recognized keyword (viaparse_keyword_line_core, checking the remainder is empty), so:"with mana value N","with {X} in its mana cost","with one or more <color> mana symbol"— keep their arms (they run first and return early), and"with power 3 or greater","with an {X}") still declines here and falls through unchanged.The
Anotherprop is still appended by the caller'sadd_another_prop, so Slitherwisp's filter becomes[WithKeyword(Flash), Another]. The object matcher already honorsFilterProp::WithKeywordviaobj.has_keyword(game/filter.rs), so no runtime change is required — the existingvalid_card_matches→target_filter_matches_objectpath now filters cast spells by keyword.Tests
slitherwisp_cast_another_flash_spell_scopes_to_flash): the trigger'svalid_cardcarries bothWithKeyword(Flash)andAnother.slitherwisp_flash_spell_cast_trigger.rs, 2 cases): drives the real cast pipeline and asserts Slitherwisp's observable effect (each opponent loses 1 life + controller draws) fires only for a flash spell — a plain spell leaves opponent life and hand untouched. This proves the fix is not merely parse-correct-but-inert.CR references: 702.8a (flash), 603.2 (trigger events).