feat(parser): compound-subject land type-change static (CR 305.7 / CR 611.3)#5301
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements parsing support for compound-subject land type-change static abilities (e.g., "All Mountains and all Forests are Plains") under CR 611.3 and CR 305.7. It refactors single-subject land type-change parsing to share predicate modification logic with the new compound-subject parser and adds extensive unit tests. The review feedback points out a violation of Repository Style Guide Rule R6, noting that the new helper function parse_compound_all_subjects_land_filter is missing a mandatory CR annotation comment.
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 "all `<X>` and all `<Y>`[ and all `<Z>`…]" into an `Or` of land-only | ||
| /// per-subject filters. Each conjunct must resolve through | ||
| /// [`parse_land_type_change_subject`] — mixed land/creature compounds (Life and | ||
| /// Limb's "Forests and Saprolings") return `None` so animation handlers keep | ||
| /// ownership. |
There was a problem hiding this comment.
[MEDIUM] Missing mandatory CR annotation on rules-touching parser helper. Evidence: crates/engine/src/parser/oracle_static/type_change.rs:2135. 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>. Suggested fix: Add a CR 611.3 annotation to the doc comment.
| /// Parse "all `<X>` and all `<Y>`[ and all `<Z>`…]" into an `Or` of land-only | |
| /// per-subject filters. Each conjunct must resolve through | |
| /// [`parse_land_type_change_subject`] — mixed land/creature compounds (Life and | |
| /// Limb's "Forests and Saprolings") return `None` so animation handlers keep | |
| /// ownership. | |
| /// CR 611.3: Parse "all <X> and all <Y>[ and all <Z>...]" into an Or of land-only | |
| /// per-subject filters. Each conjunct must resolve through | |
| /// [parse_land_type_change_subject] — mixed land/creature compounds (Life and | |
| /// Limb's "Forests and Saprolings") return None so animation handlers keep | |
| /// ownership. |
References
- Rule R6: Every rules-touching line of engine code must carry a comment of the form CR : . (link)
Parse changes introduced by this PR✓ No card-parse changes detected. |
… 611.3) Mirror the compound-subject animation handler structure for land type-change predicates: distribute dual-subject lines into an Or filter and parse the shared land-type predicate once via parse_land_type_change_predicate_modifications. Closes phase-rs#5299 Co-authored-by: Cursor <cursoragent@cursor.com>
d477982 to
833d402
Compare
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer review: parser-only compound land type-change support lands at the existing static type-change seam, reuses the land-type predicate parser, and has focused parser coverage for replacement, additive, multi-type, triple-subject, mixed-subject decline, and single-subject fallthrough cases. Parse-diff Equipment entries are from already-merged main changes between b1a9f9b and origin/main, not this PR diff.
Summary
Completes the compound-subject static class started in #5219 / #5293 by adding the land type-change sibling handler. The parallel grammar \All and all are \ previously strict-failed because \parse_land_type_change\ only resolves single-subject filters.
\parse_compound_all_subjects_land_type_change\ mirrors the #5219 structure:
Closes #5299
Test plan
Made with Cursor