Skip to content

fix(parser): parse supertype-qualified enchant targets - #5109

Merged
matthewevans merged 4 commits into
phase-rs:mainfrom
ntindle:codex/root31-enchant-supertype-targets
Jul 8, 2026
Merged

fix(parser): parse supertype-qualified enchant targets#5109
matthewevans merged 4 commits into
phase-rs:mainfrom
ntindle:codex/root31-enchant-supertype-targets

Conversation

@ntindle

@ntindle ntindle commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Parse supertype-qualified Enchant targets such as snow land you control, basic land you control, and legendary creature.
  • Reuse the shared target supertype parser so Aura legality emits FilterProp::HasSupertype instead of dropping the Enchant keyword payload.
  • Keep multi-leg Enchant lists per-leg, so legendary creature or planeswalker does not leak Legendary onto the planeswalker leg.
  • Remove On Thin Ice from root cause chore: update coverage stats and badges #31 in docs/parser-misparse-backlog.md.

Root Cause

parse_type_phrase already understood supertype prefixes, but the Enchant keyword path used the narrower parse_enchant_type_leg directly. As a result, Enchant:snow land you control and sibling forms failed to produce a typed Keyword::Enchant(...) target filter.

Parse Audit

Focused before/after export set:

  • Root chore: update coverage stats and badges #31: Cabal Stronghold, Flaccify, On Thin Ice, Open the Omenpaths, Rainbow Vale, The Great Mound
  • Expected sibling Aura cards: Dimensional Exile, Ossification, Leyline Immersion
  • The name filter also matched Colossification; it was included in the audit and did not change.

Raw JSON changed cards:

  • On Thin Ice: keywords: [] -> Enchant(Typed Land, controller You, HasSupertype Snow)
  • Dimensional Exile: keywords: [] -> Enchant(Typed Land, controller You, HasSupertype Basic)
  • Ossification: keywords: [] -> Enchant(Typed Land, controller You, HasSupertype Basic)
  • Leyline Immersion: keywords: [] -> Enchant(Typed Creature, HasSupertype Legendary)

After deleting keywords, all four changed card objects were byte-equivalent to baseline. The other focused cards did not change.

coverage-parse-diff reports no parse-tree changes, which is expected because this PR changes Enchant keyword payloads rather than parse_details ability trees.

Review cleanup

Addressed current Gemini review comments in 8a0cddb39:

  • Move EnchantTypeLeg properties directly instead of cloning them.
  • Use filters.pop().unwrap() for the single-filter Enchant case.

Validation

  • cargo fmt --all
  • ./scripts/check-parser-combinators.sh
  • env -u RUSTC_WRAPPER ZIG_GLOBAL_CACHE_DIR=/tmp/phase-zig-cache cargo --config 'build.rustc-wrapper=""' test -p engine --features cli --lib enchant (217 passed)
  • target/debug/card-data-validate /tmp/phase-enchant-supertype-baseline.json
  • target/debug/card-data-validate /tmp/phase-enchant-supertype-after.json
  • target/debug/coverage-parse-diff /tmp/phase-enchant-supertype-baseline.json /tmp/phase-enchant-supertype-after.json --markdown /tmp/phase-enchant-supertype-diff.md --json /tmp/phase-enchant-supertype-diff.json
  • review cleanup rerun: cargo fmt --all, ./scripts/check-parser-combinators.sh, cargo test -p engine --features cli --lib enchant -- --nocapture (217 passed), git diff --check

CR references verified locally with grep against docs/MagicCompRules.txt for 205.4a, 303.4a, and 702.5a.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request implements support for supertype-qualified Aura targets (such as "snow land", "basic land", or "legendary creature") in the MTG rules engine parser. It introduces the EnchantTypeLeg struct to preserve supertype properties per leg, ensuring they are scoped correctly and do not leak to sibling legs in multi-leg inline Enchant phrases (e.g., "Enchant legendary creature or planeswalker"), resolving a misparse backlog item for "On Thin Ice". The review feedback suggests two performance and idiomatic improvements: destructuring type_leg in parse_enchant_target to avoid cloning FilterProp elements, and using pop().unwrap() instead of remove(0) when extracting a single element from a vector of length 1.

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.

Comment thread crates/engine/src/types/keywords.rs
Comment thread crates/engine/src/parser/oracle_nom/enchant.rs
@github-actions

github-actions Bot commented Jul 5, 2026

Copy link
Copy Markdown

Parse changes introduced by this PR · 4 card(s), 1 signature(s) (baseline: main ddad0c5d2e39)

4 card(s) · keyword/Enchant · added: Enchant

Examples: Dimensional Exile, Leyline Immersion, On Thin Ice (+1 more)

@ntindle
ntindle marked this pull request as ready for review July 5, 2026 00:57
@ntindle
ntindle requested a review from matthewevans as a code owner July 5, 2026 00:57
@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!

…supertype-targets

# Conflicts:
#	docs/parser-misparse-backlog.md
@matthewevans matthewevans self-assigned this Jul 5, 2026
@matthewevans matthewevans added the bug Bug fix label Jul 5, 2026

@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.

Thanks for the update. The parser shape needs one more parse-diff/scope pass before I can approve:

[MED] Parse-diff includes an unexplained out-of-scope card change. Evidence: the coverage-parse-diff sticky reports Say Its Name gaining cost/two other cards named ~ from your graveyard, while this PR's diff is for supertype-qualified enchant targets. Why it matters: an enchant-target parser PR should not silently change an unrelated graveyard-cost parse signature. Suggested fix: regenerate/explain the parse-diff; if that change is real, split it or cover the Say Its Name parser change with its own discriminating evidence.

@matthewevans matthewevans removed their assignment Jul 5, 2026
@ntindle

ntindle commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

Updated this branch with current origin/main (a267d9b). The two Gemini inline suggestions were already present on the merged head, and I resolved both threads after confirming the current code matches them. Local checks: cargo fmt --all; git diff --check.

@matthewevans matthewevans self-assigned this Jul 8, 2026

@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.

Maintainer re-review: the current head keeps the supertype-qualified Enchant change at the shared enchant parser seam, reuses the existing supertype parser, scopes properties per list leg, and resolves the prior Gemini suggestions. I verified the CR citations and the relevant parser gates locally; leaving enqueue for when the pending Rust test shards finish.

@matthewevans
matthewevans added this pull request to the merge queue Jul 8, 2026
@matthewevans matthewevans removed their assignment Jul 8, 2026
Merged via the queue into phase-rs:main with commit d0200a7 Jul 8, 2026
12 checks passed
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