Skip to content

fix(parser): harden trailing if-gate splitter against "as if" riders#4933

Open
nickmopen wants to merge 1 commit into
phase-rs:mainfrom
nickmopen:fix/issue-4876-trailing-gate-as-if
Open

fix(parser): harden trailing if-gate splitter against "as if" riders#4933
nickmopen wants to merge 1 commit into
phase-rs:mainfrom
nickmopen:fix/issue-4876-trailing-gate-as-if

Conversation

@nickmopen

Copy link
Copy Markdown
Contributor

Closes #4876.

Problem

split_trailing_gate_condition (added in #4841 for Rock Jockey's can't play lands if … gate) used take_until(" if "), which anchors on the first if in the line. On oracle text containing as if — e.g. a permission rider like "cast as if it had flash if you control a Swamp" — it mis-splits inside as if, attaching the wrong condition or dropping the real trailing gate when the building block is reused on other restriction classes.

Fix

  • Promoted split_trailing_gate_condition and its split_trailing_as_long_as building block from oracle_static/dispatch.rs to oracle_static/shared.rs (parity with parse_if_static_condition / extract_cant_untap_condition), so the shared trailing-gate splitter lives with the other condition building blocks.
  • The bare-if fallback now walks each if boundary (combinator-based take_until/tag scan) and keeps the last one whose preceding word is not "as" — skipping as if riders. " as long as " is still tried first; an unrecognized condition still leaves the line unsupported rather than enforcing unconditionally.

Verified behavior:

Input (lowercased) Gate condition
… can't play lands if this creature was cast this turn this creature was cast this turn
… can't play lands as if it had flash if you control a swamp you control a swamp
… can't play lands as long as ten or more lands are on the battlefield ten or more lands are on the battlefield
… can't play lands None
cast this as if it had flash (no real gate) None

Tests

  • New cant_play_lands_gate_skips_as_if_rider — an as if + trailing if line on can't play lands must bind the trailing gate.
  • Extended the Rock Jockey dispatch test with card-name-normalized text ("Rock Jockey was cast this turn").

cargo test -p engine --lib14,572 passed / 0 failed; cargo fmt --all --check, cargo clippy -p engine --all-targets -- -D warnings, and scripts/check-parser-combinators.sh all clean.

CR 611.3a (conditional statics) + CR 305.1 (land plays).

@nickmopen nickmopen requested a review from matthewevans as a code owner July 2, 2026 19:23
@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.

[MED] The new as if regression test drives the production CantPlayLand parser with a rules-bearing middle rider that the parser does not model or reject. Evidence: the test parses "You can't play lands as if it had flash if this creature was cast this turn." through parse_static_line_multi (crates/engine/src/parser/oracle_static/tests.rs:345-348), while the production arm accepts any line containing can't play lands (crates/engine/src/parser/oracle_static/dispatch.rs:2789-2795) and only attaches the trailing gate afterward. That means the test succeeds while silently dropping as if it had flash. Why it matters: this makes the parser look like it supports a rules-bearing rider it does not actually represent, which is coverage-dishonest support. Please either test split_trailing_gate_condition directly as a splitter unit, or use a production static whose middle as if rider is fully modeled; production parsing should not accept and erase an unmodeled rider.

This parser-surface PR also still needs the current <!-- coverage-parse-diff --> sticky before acceptance, so the card-level blast radius can be checked against the claimed scope.

Promote `split_trailing_gate_condition` (and its `split_trailing_as_long_as`
building block) from `oracle_static/dispatch.rs` to `oracle_static/shared.rs`,
and match the LAST trailing ` if ` while skipping `as if` riders instead of
anchoring on the first ` if `.

The old `take_until(" if ")` anchored on the first occurrence, so a line like
"You can't play lands as if it had flash if you control a Swamp" mis-split
inside `as if` — attaching the wrong condition or dropping the real trailing
gate. The splitter now walks each ` if ` boundary (combinator-based) and keeps
the last one whose preceding word is not "as".

Adds regression coverage for an `as if` + trailing `if` line on can't-play-lands
and extends the Rock Jockey dispatch test with card-name-normalized text.

Closes phase-rs#4876
@nickmopen nickmopen force-pushed the fix/issue-4876-trailing-gate-as-if branch from f30a54c to fdaf7f6 Compare July 2, 2026 20:24
@nickmopen

Copy link
Copy Markdown
Contributor Author

Good catch — fixed. Replaced the production-parser test with a direct splitter-unit test (split_trailing_gate_condition_skips_as_if_rider) that calls split_trailing_gate_condition on the building block itself, so nothing drives the can't play lands production arm with an as if it had flash rider it doesn't model. The unit test asserts the splitter's full contract:

  • "…can't play lands as if it had flash if you control a swamp"Some("you control a swamp") (skips as if, takes the last if)
  • plain trailing if → unchanged
  • as long as still precedes a later bare if
  • an as if-only line (no real gate) → None
  • no gate → None

The Rock Jockey dispatch test (with card-name-normalized text) still exercises the real production path on a fully-modeled line.

Also rebased onto current main so the coverage-parse-diff sticky recomputes against the current base. cargo test -p engine --lib, fmt --check, clippy -D warnings, and check-parser-combinators.sh all clean.

@matthewevans

Copy link
Copy Markdown
Member

Thanks for the update. The previous false-green concern is addressed on this head: the as if case now tests split_trailing_gate_condition directly, which is the right building block for this fix.

Holding this for current-head parse-diff evidence before approval. This PR changes parser source (crates/engine/src/parser/oracle_static/shared.rs and dispatch.rs), but I do not see a <!-- coverage-parse-diff --> sticky for head fdaf7f6d3663a126a11390b1ef37c41810cfa07f. Once the parse-diff sticky is present for this head, I can verify the card-level blast radius and finish the review.

@nickmopen

Copy link
Copy Markdown
Contributor Author

Parse changes introduced by this PR · head fdaf7f6d3

No card-parse changes detected.

{ "oracle_changed": 1, "added_cards": [], "removed_cards": [], "clusters": [] }

Surfacing this manually because the auto-post workflow_run sticky job didn't fire for this fork head — the Card data (generate, validate, coverage) check did run and uploaded the parse-diff artifact, so this is the CI-generated result for fdaf7f6d3, not a hand-computed one:

Zero card blast radius is expected here: the change is a behavior-preserving move of split_trailing_gate_condition/split_trailing_as_long_as into shared.rs plus defensive as if hardening on the bare-if fallback. No card in the pool has a can't play lands … as if … if … line, and the existing can't play lands cards (Rock Jockey single trailing if, Limited Resources as long as) resolve to the same condition under both the old first-if and new last-if logic — so no card's parse output changes.

@matthewevans matthewevans self-assigned this Jul 2, 2026
@matthewevans

Copy link
Copy Markdown
Member

Thanks for the follow-up. I’m keeping this on hold for the current head because the parse-diff block here was supplied by the PR author, and the review loop only treats trusted workflow- or maintainer-generated parse-diff evidence as authoritative for parser-source PRs.

The no-change result may be correct, but I need trusted evidence for this head before I can clear the prior review. This is especially important for parser splitter changes where the failure mode is silent parse-scope drift.

@matthewevans matthewevans added the bug Bug fix label Jul 2, 2026
@matthewevans matthewevans removed their assignment Jul 2, 2026
@matthewevans

Copy link
Copy Markdown
Member

Parse changes introduced by this PR

✓ No card-parse changes detected.

@matthewevans matthewevans self-assigned this 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, the parser shape looks otherwise clean. One remaining repo-policy issue blocks this for me:

[LOW] New test uses an inline import despite the repo-wide no-inline-import rule. Evidence: crates/engine/src/parser/oracle_static/tests.rs:346 introduces use super::shared::split_trailing_gate_condition; inside the test body. Why it matters: project instructions require imports at file scope for readability/static analysis, and this is directly avoidable. Suggested fix: move the import to the test module imports or call super::shared::split_trailing_gate_condition(...) fully qualified.

@matthewevans matthewevans removed their assignment Jul 5, 2026
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.

fix(parser): harden trailing if-gate combinator on static restrictions (as-if safe)

2 participants