fix(parser): split bare-"and" "... and each opponent/player <verb>" conjunct - #3584
Conversation
…junct
A second clause of the form "each opponent/player <conjugated-verb> …" joined
to a "you …" clause by a bare " and " (no comma) was swallowed by the first
effect and silently dropped — Slitherwisp dealt no life loss, Curry Favor's
each-opponent drain vanished, Bad Deal's each-opponent discard was lost.
starts_bare_and_clause_lower had no "each opponent"/"each player" + verb arm
("each opponent" is intentionally not a bare clause starter so Goblin
Chainwhirler's "each opponent and each creature you control" stays one
DamageAll). Add a verb-scoped helper starts_each_player_predicate_clause_lower
mirroring starts_target_continuous_clause_lower: it splits only when the
"each opponent/player " subject is immediately followed by a conjugated
player-action verb, so a bare-noun continuation never matches. Once peeled,
the existing strip_each_player_subject + per-player scope lowering handles it.
Parser-only, no new engine variant. Covers Slitherwisp, Curry Favor,
Disinformation Campaign, Bad Deal, Clockwork Fox.
There was a problem hiding this comment.
Code Review
This pull request introduces a parser helper starts_each_player_predicate_clause_lower to correctly split player-scoped predicate clauses joined by a bare " and " (such as in Slitherwisp or Curry Favor), preventing them from being swallowed by preceding effects. It also adds comprehensive unit and end-to-end integration tests. The feedback suggests expanding the list of recognized player-action verbs (e.g., adding "creates", "puts", "chooses", etc.) to ensure complete sibling coverage for other common card effects.
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.
| alt(( | ||
| tag("loses "), | ||
| tag("gains "), | ||
| tag("draws "), | ||
| tag("discards "), | ||
| tag("mills "), | ||
| tag("sacrifices "), | ||
| tag("exiles "), | ||
| )), |
There was a problem hiding this comment.
[MEDIUM] Missing conjugated player-action verbs in starts_each_player_predicate_clause_lower.
Why it matters: Common player-action clauses like each opponent creates..., each opponent puts..., each opponent chooses..., or each opponent reveals... will be swallowed and dropped because their verbs are not matched by this parser arm.
Suggested fix: Add creates , puts , chooses , reveals , shuffles , and returns to the alt combinator.
alt((
tag("loses "),
tag("gains "),
tag("draws "),
tag("discards "),
tag("mills "),
tag("sacrifices "),
tag("exiles "),
tag("creates "),
tag("puts "),
tag("chooses "),
tag("reveals "),
tag("shuffles "),
tag("returns "),
)),References
- L2. Sibling coverage: If a parser arm or string was extended, ensure all sibling variants/verbs are covered. (link)
- Avoid verbatim string equality for parsing Oracle phrases. Instead, decompose compound phrases into modular, reusable parsers and compose them using idiomatic combinator aggregates like nested
altandtagsequences.
matthewevans
left a comment
There was a problem hiding this comment.
Approved after deep review. Splits the bare-"and" "X and each opponent/player " conjunct (Slitherwisp, Curry Favor, Bad Deal, Oloro, Gray Merchant — 18-card class) that was being swallowed into the first effect. This is a high-blast-radius change to sequence.rs's clause splitter, so the gating concern was over-split regression — and the guard is tight: starts_each_player_predicate_clause_lower (sequence.rs:1519) fires ONLY when the post-and remainder re-states a fresh player subject (each opponent/each player) + a conjugated player-verb, so same-subject continuations ("gets +1/+1 and gains flying") structurally can't reach it, and Goblin Chainwhirler ("...and each creature you control") is excluded. Three regression negatives prove the over-split doesn't fire (bare-noun continuation, GCW compound, possessive). Plugs into the canonical bare-and .or() cluster (inherits #3509's quote/paren boundary guards); swallowed-clause ratchet moves safe (only adds a split). End-to-end test drives Slitherwisp/Curry Favor/Bad Deal through parse_effect_chain, both conjuncts real + opponent-scoped. Subject parameterized (each opponent/each player). CR 102.2/119.3/121.1/608.2c verified. Enqueuing.
|
Approved & enqueued — nice, tightly-guarded work. One follow-up (non-blocking, for a future PR): re: Gemini's verb-expansion suggestion, I checked the card pool. The speculative verbs (creates/puts/chooses) match zero cards in this structure, so leaving them out is correct scoping. But "returns" is a real 2-card sibling gap: |
Fixes #3582.
Summary
When a "you …" clause is joined to a second "each opponent/player
<verb>…" clause by a bare " and " (no comma), the second conjunct was silently dropped — only the first half resolved:Root cause
starts_bare_and_clause_lowerdid not treat "each opponent "/"each player " + a conjugated player verb as a fresh clause start, so the second conjunct never began a new chunk and was swallowed by the first effect. The comma splitter already lists those subjects (so comma-joined / separate-sentence forms work). "each opponent" is intentionally not a bare clause starter so a bare-noun continuation like Goblin Chainwhirler's "deals 1 damage to each opponent and each creature you control" stays a singleDamageAll.Fix (parser-only; no new engine variant)
Add
starts_each_player_predicate_clause_lower, modeled on the existingstarts_target_continuous_clause_lowersibling, wired as one trailing.or()arm instarts_bare_and_clause_lower. It matches "each opponent "/"each player " only when immediately followed by a conjugated player-action verb (loses/gains/draws/discards/mills/sacrifices/exiles) — an immediatetagon the verb (nottake_until), so a bare-noun continuation (no verb) never matches and Goblin Chainwhirler stays one chunk. Once the conjunct is peeled, the existingstrip_each_player_subject+ per-player scope lowering handles it end-to-end; for Curry Favor the bare-and split emits aClauseBoundary::Comma, so the "where X is the number of Knights" binding reaches both conjuncts.No runtime change — the player-scoped effects (
LoseLife/GainLife/Draw/Discard/Mill/Sacrifice/ExileunderPlayerFilter::Opponent/All) already exist and lower today. CR 102.2 (opponent), CR 119.3 (gain/lose life), CR 121.1 (draw), CR 608.2c (follow the whole text).Tests
starts_bare_and_clause): positive for "each opponent loses 1 life" / "discards a card" / "discards two cards" / "draws a card" / "loses x life" / "each player loses 2 life"; negative for "each creature you control" / "each opponent and each creature you control" / "each opponent's creatures".clause_texts): "you draw a card and each opponent loses 1 life" → two chunks; Goblin Chainwhirler stays a single chunk.parse_effect_chain): Slitherwisp has bothDrawand opponent-scopedLoseLife(amount 1); Curry Favor has bothGainLifeand opponentLoseLifebound to the same X; Bad Deal has the opponentDiscard(count 2). Revert-discriminating — without the new arm the conjunct is dropped and these fail.Verification
cargo +nightly test -p engine --lib: 0 failures (152 sequence + 51 bare_and tests pass, incl. Goblin Chainwhirler no-split and Skulduggery).clippy -D warnings: clean.rustfmt --check: clean. Parser-combinator gate: clean for this diff.