Skip to content

feat(parser): parse "for each <type> you control with <keyword>" counts#5210

Merged
matthewevans merged 3 commits into
phase-rs:mainfrom
real-venus:feat/for-each-controlled-type-with-keyword
Jul 6, 2026
Merged

feat(parser): parse "for each <type> you control with <keyword>" counts#5210
matthewevans merged 3 commits into
phase-rs:mainfrom
real-venus:feat/for-each-controlled-type-with-keyword

Conversation

@real-venus

Copy link
Copy Markdown
Contributor

Summary

Closes #5018

Adds parse_for_each_controlled_type_with_keyword to the "for each" quantity dispatch — the controller-scoped counterpart to the existing any-controller parse_for_each_battlefield_type_with_keyword.

The for each grammar could parse <type> on the battlefield with <keyword> (any controller) and <type> you control with a <kind> counter on it (controller-scoped, counter predicate), but had no arm for <type> you control with <keyword>. The bare parse_for_each_controlled_type arm matched creature you control and stranded with flying as an unconsumed remainder; because the for each clause requires full consumption, the whole quantity — and the effect depending on it — was silently dropped.

Cards fixed (class, not one card)

Card Oracle text Effect that was dropped
Skycat Sovereign "…+1/+1 for each other creature you control with flying." dynamic P/T pump
Aven Gagglemaster "…you gain 2 life for each creature you control with flying." life-gain amount
Aerial Assault "You gain 1 life for each creature you control with flying." life-gain amount
Alert Heedbonder "…you gain 1 life for each creature you control with vigilance." life-gain amount
Overgrown Battlement "{T}: Add {G} for each creature you control with defender." mana amount

Root cause

The for each dispatch (crates/engine/src/parser/oracle_nom/quantity.rs) already had a combinator for the any-controller form (parse_for_each_battlefield_type_with_keyword, <type> on the battlefield with <keyword>) and a controller-scoped counter form (parse_for_each_controlled_type_with_counter), but no controller-scoped keyword form. The bare parse_for_each_controlled_type arm consumed <type> you control and left with <keyword> unconsumed, so the clause failed full-consumption and the dependent effect was silently dropped — exactly the failure mode the battlefield-wide sibling already fixed, but for the you control scope.

Implementation

The new combinator composes only existing building blocks:

  • parse_type_filter_word for the type head,
  • the shared controller phrase (mirroring parse_for_each_controlled_type_with_counter, tolerating the already adverb in "you already control"),
  • parse_keyword_name + FilterProp::WithKeyword, generalized over the whole evergreen keyword table.

The optional other/another prefix lowers to FilterProp::Another (Skycat's "other creature you control"). The count binds to the source controller via ControllerRef::You (CR 109.4 — only battlefield/stack objects have a controller), which is the discriminator from the any-controller battlefield form. No new enum variants and no engine/runtime changes — the FilterProp::WithKeyword runtime evaluation path already backs the battlefield sibling.

It is registered immediately before the bare parse_for_each_controlled_type arm so the keyword suffix is consumed before the shorter you control tag can match.

Tests

parse_for_each_controlled_type_with_keyword_scoped_count exercises the class across the keyword table (flying / vigilance / defender), the other/another self-exclusion, and the already adverb, asserting full consumption and controller == Some(You) (the discriminator vs. the any-controller battlefield form).

CR annotations

  • CR 109.4 — only objects on the battlefield/stack have a controller (verified in docs/MagicCompRules.txt).
  • CR 702 — Keyword Abilities (the with <keyword> predicate; verified).

Add `parse_for_each_controlled_type_with_keyword` to the "for each" quantity dispatch, the controller-scoped counterpart to the existing any-controller `parse_for_each_battlefield_type_with_keyword`.

The bare `parse_for_each_controlled_type` arm matched "<type> you control" and stranded " with <keyword>" as an unconsumed remainder, failing the for-each full-consumption requirement so the whole quantity (and its dependent P/T pump / life-gain / mana amount) was silently dropped. The new arm composes the shared `parse_type_filter_word`, controller phrase, and `parse_keyword_name` + `FilterProp::WithKeyword` building blocks over the whole evergreen keyword table, with the optional other/another self-exclusion, binding the count to the source controller (CR 109.4).

Covers the class: Skycat Sovereign, Aven Gagglemaster, Aerial Assault, Alert Heedbonder, Overgrown Battlement.

Closes phase-rs#5018
@real-venus real-venus requested a review from matthewevans as a code owner July 6, 2026 21:30

@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 introduces the parse_for_each_controlled_type_with_keyword parser to support parsing controller-scoped quantity counts gated on keyword predicates (e.g., 'for each other creature you control with flying'), along with corresponding unit tests. The feedback highlights a potential runtime panic where .unwrap() is used on keyword parsing, suggesting a safer approach using map_res to handle conversion errors gracefully.

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/parser/oracle_nom/quantity.rs Outdated
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

Parse changes introduced by this PR

✓ No card-parse changes detected.

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
@real-venus

Copy link
Copy Markdown
Contributor Author

@matthewevans
Please kindly review.

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

[MED] The PR claims production card coverage, but the review evidence only reaches the quantity helper and the parse-diff shows no card-level change. Evidence: crates/engine/src/parser/oracle_nom/quantity.rs:4339 names Skycat Sovereign/Aven Gagglemaster/Aerial Assault/Alert Heedbonder/Overgrown Battlement as the backed class, while the only new test calls parse_for_each_clause_ref directly at crates/engine/src/parser/oracle_nom/quantity.rs:4946; the current parse-diff sticky says No card-parse changes detected. Why it matters: a helper-only test can pass even if the production static/effect parser path never feeds these Oracle lines through the new arm, so the claimed card fixes may still be unsupported or already handled elsewhere. Suggested fix: add a production-path parser test for at least one representative card line through parse_static_line/parse_effect as appropriate, and reconcile why the card-level parse diff is empty if the PR is intended to change supported card output.

… keyword"

Address review: prove the new controller-scoped keyword `for each` arm is
reached by the real production parser, not only the `parse_for_each_clause_ref`
helper in isolation.

Add two `parse_effect` end-to-end tests:
- Aven Gagglemaster / Aerial Assault: "You gain 1 life for each creature you
  control with flying." lowers to `GainLife` whose amount is the
  controller-scoped (`ControllerRef::You`) count of creatures with flying.
- Skycat Sovereign: "gets +1/+1 for each other creature you control with
  flying" lowers to a `Pump` whose dynamic count carries both the `Another`
  self-exclusion and the flying `WithKeyword` predicate.

Both fail if reverted to before the new arm, when the stranded " with flying"
made the for-each clause fail full consumption and the dynamic quantity was
dropped.
@real-venus

Copy link
Copy Markdown
Contributor Author

[MED] The PR claims production card coverage, but the review evidence only reaches the quantity helper and the parse-diff shows no card-level change. Evidence: crates/engine/src/parser/oracle_nom/quantity.rs:4339 names Skycat Sovereign/Aven Gagglemaster/Aerial Assault/Alert Heedbonder/Overgrown Battlement as the backed class, while the only new test calls parse_for_each_clause_ref directly at crates/engine/src/parser/oracle_nom/quantity.rs:4946; the current parse-diff sticky says No card-parse changes detected. Why it matters: a helper-only test can pass even if the production static/effect parser path never feeds these Oracle lines through the new arm, so the claimed card fixes may still be unsupported or already handled elsewhere. Suggested fix: add a production-path parser test for at least one representative card line through parse_static_line/parse_effect as appropriate, and reconcile why the card-level parse diff is empty if the PR is intended to change supported card output.

@matthewevans
Done.
please check this.

@matthewevans matthewevans self-assigned this Jul 6, 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 latest head adds production-path parser coverage for the previously requested for each <type> you control with <keyword> path, parse-diff reports no card-level drift, and CI is green. Approving for merge queue.

@matthewevans matthewevans added the enhancement New feature or request label Jul 6, 2026
@matthewevans matthewevans added this pull request to the merge queue Jul 6, 2026
@matthewevans matthewevans removed their assignment Jul 6, 2026
Merged via the queue into phase-rs:main with commit 42fc668 Jul 6, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Parser drops "for each <type> you control with <keyword>" counts (Skycat Sovereign, Aven Gagglemaster, Overgrown Battlement)

2 participants