Skip to content

feat(parser): peel a leading supertype off "<Supertype> <Subtype>" subjects#5549

Merged
matthewevans merged 2 commits into
phase-rs:mainfrom
minion1227:minion_legendary_subtype
Jul 11, 2026
Merged

feat(parser): peel a leading supertype off "<Supertype> <Subtype>" subjects#5549
matthewevans merged 2 commits into
phase-rs:mainfrom
minion1227:minion_legendary_subtype

Conversation

@minion1227

Copy link
Copy Markdown
Contributor

CR 205.4a + CR 205.3m

Bug

A subject phrase like "Legendary Humans you control" names the legendary supertype plus the Human subtype — but the shared subtype authority typed_filter_for_subtype treated the whole two-word string as one subtype and fabricated Subtype("Legendary Human"), a filter that matches no card in existence. The anthems therefore applied to nothing at runtime:

  • General's EnforcerLegendary Humans you control have indestructible.
  • Kashi-Tribe EliteLegendary Snakes you control have shroud.

Before: affected = [Creature, Subtype("Legendary Human")] (zero matches).
After: affected = [Creature, Subtype("Human")] + HasSupertype(Legendary).

Fix

At the single authority typed_filter_for_subtype (grammar.rs): peel a leading supertype word ("Legendary"/"Snow"/"Basic"/…) via a new split_leading_supertype helper, attach it as FilterProp::HasSupertype, and resolve the remainder as the real subtype (recursing so the core-type inference still runs on the true subtype).

Because every capitalized-subtype fallback in the subject parsers routes through this one function, all four call sites — parse_typed_you_control's creature-arm and combat-status-prefix arm, plus the parse_creature_subject_filter fallback — inherit the fix from one place.

Why it's the right seam / minimal blast radius

  • One authority, not four sibling patches — the fix lands where the fabrication happens, so the whole <supertype> <subtype> subject class is covered in a single change.
  • A bare supertype ("Legendary creatures") already routes through its own path (descriptor_is_supertype) and is untouched; a plain subtype ("Goblins") gains no supertype property (regression-tested).
  • split_leading_supertype returns None for a bare supertype (no following subtype), so it never strips a legitimate lone supertype.

Tests

  • legendary_subtype_subject_peels_supertype_and_keeps_subtype — end-to-end via parse_static_line: General's Enforcer → Subtype("Human") + HasSupertype(Legendary) + AddKeyword(Indestructible).
  • typed_filter_for_subtype_peels_leading_supertype — building-block: "Legendary Human" → Human + HasSupertype(Legendary); "Goblin" → Goblin, no supertype (regression).
  • Full parser::oracle_static::tests (1039 passed, 0 failed), including the existing bare-supertype "Legendary creatures" / Jodah tests.

CR verification

  • CR 205.4a — the supertypes (basic, legendary, ongoing, snow, world) — verified in docs/MagicCompRules.txt.
  • CR 205.3m — subtype recognition on the remainder.

AI-contributor notes

  • Rules-correct: the anthem now targets legendary creatures of the named subtype per CR 205.4a; the enchanted/controlled set is real, not empty.
  • Builds the class: the fix is at the shared subtype authority, so any <supertype> <subtype> subject (current or future) is covered.
  • Verified: live-parsed General's Enforcer and Kashi-Tribe Elite before/after against Scryfall Oracle text; cargo fmt, CI-exact clippy -p engine --all-targets --features proptest -D warnings, and the full oracle_static test module are green.

🤖 Generated with Claude Code

@minion1227
minion1227 requested a review from matthewevans as a code owner July 11, 2026 04:25
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Architecturally clean, correct building-block fix — holding formal approval for CI.

✅ Clean

  • Right seam. The supertype-peel lives in typed_filter_for_subtype (grammar.rs), the single subtype-filter authority, so every <supertype> <subtype> compound benefits — not just the two cards cited. Recursing into typed_filter_for_subtype(rest) composes cleanly rather than special-casing.
  • Reuses the existing combinator. split_leading_supertype delegates to nom_target::parse_supertype_word instead of hand-rolling supertype matching; the original-case remainder uses the documented descriptor[len − rest_lower.len()..] offset idiom.
  • CR + cards verified. CR 205.4a grep-confirmed verbatim ("supertypes are basic, legendary, ongoing, snow, and world"); CR 205.3m confirms Human/Snake are subtypes. General's Enforcer — "Legendary Humans you control have indestructible." — and Kashi-Tribe Elite — "Legendary Snakes you control have shroud." — both match Scryfall. This is the fabricated zero-match Subtype("Legendary Human") class fixed at the source.
  • Discriminating tests at both levels: the building-block test asserts the peel plus a plain-Goblin negative (no fabricated supertype), and the integration test locks HasSupertype(Legendary) + Subtype(Human) + the anthem AddKeyword modification.

Recommendation: approve as bug once CI settles green on a rebased head (currently BEHIND, CI pending). No changes requested.

@github-actions

github-actions Bot commented Jul 11, 2026

Copy link
Copy Markdown

Parse changes introduced by this PR · 2 card(s), 2 signature(s) (baseline: main 67607fd8e8b8)

1 card(s) · static/Continuous · field affects: you control creature Legendary Humanlegendary you control creature Human

Examples: General's Enforcer

1 card(s) · static/Continuous · field affects: you control creature Legendary Snakelegendary you control creature Snake

Examples: Kashi-Tribe Elite

1 card(s) had Oracle-text changes (errata/reprint) — excluded as non-parser.

minion1227 and others added 2 commits July 10, 2026 21:40
…bjects

CR 205.4a + CR 205.3m: A subject phrase like "Legendary Humans you control"
names the legendary supertype plus the Human subtype, but the shared subtype
authority `typed_filter_for_subtype` treated the whole two-word string as one
subtype and fabricated `Subtype("Legendary Human")` — a filter that matches no
card in existence. So General's Enforcer ("Legendary Humans you control have
indestructible") and Kashi-Tribe Elite ("Legendary Snakes you control have
shroud") silently applied their anthem to nothing.

Fix at the single authority: `typed_filter_for_subtype` now peels a leading
supertype word ("Legendary"/"Snow"/"Basic"/...) via a new `split_leading_supertype`
helper, attaching it as a `FilterProp::HasSupertype` and resolving the remainder
("Human", "Snake") as the real subtype. Because every capitalized-subtype
fallback in the subject parsers routes through this one function, all four call
sites (`parse_typed_you_control`'s creature/combat-prefix arms and the
`parse_creature_subject_filter` fallback) inherit the fix. A bare supertype
("Legendary creatures") still routes through its own path unchanged, and a plain
subtype ("Goblins") gains no supertype property.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…combinator gate)

`split_leading_supertype` peeled the supertype word with a nom combinator but
then consumed the separating space with `strip_prefix(' ')`, tripping the parser
combinator gate. Consume the word and its trailing space atomically via
`terminated(parse_supertype_word, tag(" "))` — a bare supertype (no following
subtype) now fails the trailing space and declines, identical to the prior
`strip_prefix(' ')?` behavior, with no string-method dispatch.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@minion1227
minion1227 force-pushed the minion_legendary_subtype branch from c15df99 to f4439c7 Compare July 11, 2026 04:40

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Approving — CI resolved green, merge state CLEAN. Correctness verified in the prior review (CR 205.4a supertype handling at the single subtype/type authority, reuses parse_supertype_word, discriminating tests). The head that went green is the reviewed head; enqueuing as bug.

@matthewevans matthewevans added the bug Bug fix label Jul 11, 2026
@matthewevans
matthewevans added this pull request to the merge queue Jul 11, 2026
Merged via the queue into phase-rs:main with commit f213193 Jul 11, 2026
13 checks passed
@minion1227
minion1227 deleted the minion_legendary_subtype branch July 18, 2026 06:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants