-
-
Notifications
You must be signed in to change notification settings - Fork 153
fix(parser): scope count-form "would draw N or more" draw replacements (Alms Collector #5678) #5867
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
c88f2b6
b4e2438
b0cea68
fd86d17
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -446,20 +446,33 @@ fn parse_replacement_line_inner(text: &str, card_name: &str) -> Option<Replaceme | |
| // "would draw a card" hooks one individual draw; a count-form "would draw one | ||
| // or more cards" hooks the instruction, which CR 121.2a modifies "before | ||
| // considering any of the individual card draws". | ||
| let draw_scope = nom_primitives::scan_at_word_boundaries(&lower, |i| { | ||
| // The count-form arm captures N via `parse_number` (build for the class: any | ||
| // "<N> or more cards" threshold, not a "two or more" special case). A count | ||
| // form scopes the whole instruction (InstructionCount); "N >= 2" additionally | ||
| // carries a typed threshold (Alms Collector) wired below as an OnlyIfQuantity | ||
| // over the pending draw count. "one or more" (N == 1) is vacuously true, so it | ||
| // takes no threshold — its InstructionCount comes from the antecedent alone. | ||
| let draw_antecedent = nom_primitives::scan_at_word_boundaries(&lower, |i| { | ||
| alt(( | ||
| value( | ||
| DrawReplacementScope::IndividualDraw, | ||
| (DrawReplacementScope::IndividualDraw, None), | ||
| tag::<_, _, OracleError<'_>>("would draw a card"), | ||
| ), | ||
| value( | ||
| DrawReplacementScope::InstructionCount, | ||
| tag("would draw one or more cards"), | ||
| ), | ||
| ( | ||
| tag("would draw "), | ||
| nom_primitives::parse_number, | ||
| tag(" or more cards"), | ||
| ) | ||
| .map(|(_, n, _)| { | ||
| ( | ||
| DrawReplacementScope::InstructionCount, | ||
| if n >= 2 { Some(n) } else { None }, | ||
| ) | ||
| }), | ||
| )) | ||
| .parse(i) | ||
| }); | ||
| if let Some(draw_scope) = draw_scope { | ||
| if let Some((draw_scope, threshold_n)) = draw_antecedent { | ||
| // CR 614.1a: An "As long as <state>, if you would draw a | ||
| // card, ..." gate (Archmage Ascension) precedes the draw antecedent with | ||
| // its own comma clause. Split it off so effect extraction anchors on the | ||
|
|
@@ -581,6 +594,29 @@ fn parse_replacement_line_inner(text: &str, card_name: &str) -> Option<Replaceme | |
| WhileAntecedent::Absent => {} | ||
| } | ||
| } | ||
| // CR 121.2a: a "draw N or more cards" antecedent (N >= 2) gates the | ||
| // replacement on the pending draw *instruction* being for at least N | ||
| // cards. Carry N as a typed `OnlyIfQuantity` over the event's draw count | ||
| // (`EventContextAmount`), evaluated at the instruction stage before the | ||
| // draw decomposes into individual card draws — composed (And) with any | ||
| // as-long-as / while / except-first gate already set. Alms Collector: | ||
| // "If an opponent would draw two or more cards, ...". | ||
| if let Some(n) = threshold_n { | ||
| let threshold = ReplacementCondition::OnlyIfQuantity { | ||
| lhs: QuantityExpr::Ref { | ||
| qty: QuantityRef::EventContextAmount, | ||
| }, | ||
| comparator: Comparator::GE, | ||
| rhs: QuantityExpr::Fixed { value: n as i32 }, | ||
| active_player_req: None, | ||
| }; | ||
| def.condition = Some(match def.condition.take() { | ||
| Some(existing) => ReplacementCondition::And { | ||
| conditions: vec![existing, threshold], | ||
| }, | ||
| None => threshold, | ||
| }); | ||
| } | ||
|
Comment on lines
+597
to
+619
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Preserve Line 568 returns after applying an Also applies to: 18804-18838 🤖 Prompt for AI AgentsSource: Path instructions |
||
| return Some(def); | ||
| } | ||
|
|
||
|
|
@@ -18765,6 +18801,42 @@ mod tests { | |
| )); | ||
| } | ||
|
|
||
| #[test] | ||
| fn alms_collector_count_form_threshold_gates_on_instruction_draw_count() { | ||
| // #5678 / CR 121.2a: "If an opponent would draw two or more cards, instead | ||
| // you and that player each draw a card." The count-form antecedent must | ||
| // (1) scope the whole instruction (InstructionCount), (2) carry N=2 as a | ||
| // typed OnlyIfQuantity over the pending draw count (EventContextAmount) so | ||
| // a one-card draw does not match, and (3) apply only to an opponent's draw. | ||
| let def = parse_replacement_line( | ||
| "If an opponent would draw two or more cards, instead you and that player each draw a card.", | ||
| "Alms Collector", | ||
| ) | ||
| .expect("Alms Collector's count-form antecedent must lower to a Draw replacement"); | ||
| assert_eq!(def.event, ReplacementEvent::Draw); | ||
| assert_eq!(def.draw_scope, Some(DrawReplacementScope::InstructionCount)); | ||
| assert_eq!(def.valid_player, Some(ReplacementPlayerScope::Opponent)); | ||
| assert_eq!( | ||
| def.condition, | ||
| Some(ReplacementCondition::OnlyIfQuantity { | ||
| lhs: QuantityExpr::Ref { | ||
| qty: QuantityRef::EventContextAmount, | ||
| }, | ||
| comparator: Comparator::GE, | ||
| rhs: QuantityExpr::Fixed { value: 2 }, | ||
| active_player_req: None, | ||
| }), | ||
| "N=2 must lower to OnlyIfQuantity(EventContextAmount >= 2)" | ||
| ); | ||
| // The substitute is a fixed per-player draw (you + the drawing opponent), | ||
| // not a count-modifier -- InstructionCount comes from the antecedent | ||
| // threshold, not the execute shape (the discipline the maintainer required). | ||
| assert!(matches!( | ||
| def.execute.as_deref().map(|a| &*a.effect), | ||
| Some(Effect::Draw { .. }) | ||
| )); | ||
| } | ||
|
|
||
| #[test] | ||
| fn draw_replacement_leading_instead_prefix_blood_scrivener() { | ||
| // CR 614.1a: "instead you draw two cards" — leading "instead" form with | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19947,6 +19947,30 @@ pub enum PlaneswalkReplacementScope { | |
| PlanarDieOnly, | ||
| } | ||
|
|
||
| /// CR 121.2a + CR 121.6b: which [`DrawReplacementScope`] the in-progress draw | ||
| /// replacement consult is eligible to match. A draw instruction resolves in two | ||
| /// seams — the whole-instruction consult that runs *before* the instruction | ||
| /// splits into individual card draws, and the per-card consult that runs for | ||
| /// each individual draw — and a shield is scoped to exactly one of them. | ||
|
Comment on lines
+19950
to
+19954
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Locate the relevant code and documentation.
git ls-files 'crates/engine/src/types/ability.rs' 'docs/MagicCompRules.txt' | cat
echo
echo '--- ability.rs excerpt ---'
sed -n '19930,19970p' crates/engine/src/types/ability.rs
echo
echo '--- relevant CR search ---'
rg -n -C 2 '121\.2a|121\.6b|draw replacement|replacement' docs/MagicCompRules.txtRepository: phase-rs/phase Length of output: 2471 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Read the exact rules text around the cited sections.
python3 - <<'PY'
from pathlib import Path
path = Path('docs/MagicCompRules.txt')
text = path.read_text()
for needle in ['121.2a', '121.6b']:
idx = text.find(needle)
print(f'=== {needle} @ {idx} ===')
if idx != -1:
start = max(0, idx - 500)
end = min(len(text), idx + 1200)
print(text[start:end])
print()
PYRepository: phase-rs/phase Length of output: 702 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Find nearby parser/test code that may explain the intended seam.
rg -n -C 3 'DrawConsultScope|DrawReplacementScope|instruction.*draw|individual.*draw' crates/engine/src/types/ability.rs crates/engine/src -g '!target'Repository: phase-rs/phase Length of output: 50370 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Inspect the file around the cited lines and any nearby comments.
sed -n '19945,19960p' crates/engine/src/types/ability.rs | cat -nRepository: phase-rs/phase Length of output: 1105 🌐 Web query:
💡 Result: In the Magic: The Gathering Comprehensive Rules, rules 121.2a and 121.6b address how replacement effects interact with drawing cards [1][2]. Rule 121.2a states: An instruction to draw multiple cards can be modified by replacement effects that refer to the number of cards drawn. This modification occurs before considering any of the individual card draws [1][2]. Rule 121.6b states: If an effect replaces a draw within a sequence of card draws, the replacement effect is completed before resuming the sequence [1][2]. Citations: Drop CR 121.6b from this consult-scope doc. It covers completing a replacement before resuming a draw sequence; the pre-split consult seam here is supported by CR 121.2a, not 121.6b. 🤖 Prompt for AI AgentsSource: Path instructions |
||
| /// | ||
| /// The default, [`Individual`](Self::Individual), is the per-card seam: an | ||
| /// `IndividualDraw` shield hooks each card, and a count-form | ||
| /// `InstructionCount` shield hooks a non-split whole-count draw (the turn-based | ||
| /// draw step, connive, gift) at or above its printed threshold. | ||
| /// [`Instruction`](Self::Instruction) is set only by | ||
| /// `game::replacement::replace_draw_instruction` for the pre-split consult, so | ||
| /// only `InstructionCount` shields see the whole instruction there — an | ||
| /// `IndividualDraw` shield must wait for its individual card. | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Default, Serialize, Deserialize)] | ||
| pub enum DrawConsultScope { | ||
| /// The per-card / non-split draw seam. See the type-level docs. | ||
| #[default] | ||
| Individual, | ||
| /// The pre-split whole-instruction seam (CR 121.2a). Only `InstructionCount` | ||
| /// count-form shields are eligible. | ||
| Instruction, | ||
| } | ||
|
|
||
| /// CR 614.1a: Which player(s) a replacement effect applies to, scoped relative | ||
| /// to the replacement source player. For permanents/spells this is the source's | ||
| /// controller; for cards outside the battlefield/stack, CR 109.4 + CR 108.4a | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
Repository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
Repository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
Repository: phase-rs/phase
Length of output: 50370
🏁 Script executed:
Repository: phase-rs/phase
Length of output: 3930
🏁 Script executed:
Repository: phase-rs/phase
Length of output: 30058
🏁 Script executed:
Repository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
Repository: phase-rs/phase
Length of output: 5247
🏁 Script executed:
Repository: phase-rs/phase
Length of output: 968
🏁 Script executed:
Repository: phase-rs/phase
Length of output: 27577
🏁 Script executed:
Repository: phase-rs/phase
Length of output: 5571
🏁 Script executed:
Repository: phase-rs/phase
Length of output: 50370
🏁 Script executed:
Repository: phase-rs/phase
Length of output: 8659
🏁 Script executed:
Repository: phase-rs/phase
Length of output: 3963
🏁 Script executed:
Repository: phase-rs/phase
Length of output: 13133
🏁 Script executed:
Repository: phase-rs/phase
Length of output: 14154
🏁 Script executed:
Repository: phase-rs/phase
Length of output: 10516
🏁 Script executed:
Repository: phase-rs/phase
Length of output: 9185
🏁 Script executed:
Repository: phase-rs/phase
Length of output: 8976
🏁 Script executed:
Repository: phase-rs/phase
Length of output: 7375
Use a fallible conversion for the threshold.
threshold_nisu32, son as i32can wrap abovei32::MAXand flip this gate negative; usei32::try_from(n)and fail closed on overflow.🤖 Prompt for AI Agents
Source: Path instructions