fix(parser): capture the count in "Devour [quality] N" keywords#5362
Conversation
Famished Worldsire prints "Devour land 3" (CR 702.82c — sacrifice any number of LANDS, enters with 3 +1/+1 counters per sacrifice). The numeric-count keyword extractor ran parse_number on "land 3", which fails on the leading qualifier, so the param fell back to "land 3" and the Devour FromStr's unwrap_or(1) produced Devour(1) — the reported bug. Skip a single leading non-numeric qualifier word (via take_until) before extracting the count when parse_number fails at the head. Plain "Devour N" and other numeric-count keywords are unaffected (the skip only fires on the parse_number-fails branch). Note: Keyword::Devour(N) stores only the counter multiplier; the "lands only" sacrifice restriction (CR 702.82c quality) is a separate synthesis-layer gap and is left as follow-up. Closes phase-rs#5260
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the numeric count keyword parser in crates/engine/src/parser/oracle_keyword.rs to support the 'Devour [quality] N' variant (e.g., 'Devour land 3') by skipping a leading non-numeric qualifier word when the initial number parsing fails, and adds corresponding unit tests. The review feedback suggests optimizing the parser to avoid double parsing the input string and improving robustness by using space1 instead of tag(" ") to handle multiple spaces between the qualifier and the count.
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.
| let count_src = if nom_primitives::parse_number.parse(rest).is_ok() { | ||
| rest | ||
| } else { | ||
| ( | ||
| take_until::<_, _, OracleError<'_>>(" "), | ||
| tag::<_, _, OracleError<'_>>(" "), | ||
| ) | ||
| .parse(rest) | ||
| .map_or(rest, |(after_qualifier, _)| after_qualifier) | ||
| }; | ||
| match nom_primitives::parse_number.parse(count_src) { | ||
| Ok((remainder, _)) => Cow::Borrowed(&count_src[..count_src.len() - remainder.len()]), | ||
| Err(_) => Cow::Borrowed(rest), | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Avoid double parsing and handle multiple spaces between qualifier and count. Evidence: crates/engine/src/parser/oracle_keyword.rs:1593-1606.
Why it matters: Parsing rest twice when it already starts with a number is inefficient, and using tag(" ") fails if there are multiple spaces between the qualifier and the count.
Suggested fix: Use space1 instead of tag(" ") to handle multiple spaces, and restructure the logic to avoid parsing rest twice.
match nom_primitives::parse_number.parse(rest) {
Ok((remainder, _)) => Cow::Borrowed(&rest[..rest.len() - remainder.len()]),
Err(_) => {
let skipped = (
take_until::<_, _, OracleError<'_>>(" "),
space1::<_, OracleError<'_>>,
)
.parse(rest)
.map(|(after_qualifier, _)| after_qualifier);
match skipped.and_then(|count_src| {
nom_primitives::parse_number
.parse(count_src)
.map(|(remainder, _)| (count_src, remainder))
}) {
Ok((count_src, remainder)) => {
Cow::Borrowed(&count_src[..count_src.len() - remainder.len()])
}
Err(_) => Cow::Borrowed(rest),
}
}
}References
- Avoid verbatim string equality for parsing Oracle phrases as it bypasses the robust nom-based parser and creates fragile matches. Instead, decompose compound phrases into modular, reusable parsers for constituent parts and compose them using idiomatic combinator aggregates to prevent combinatorial explosion and improve maintainability.
There was a problem hiding this comment.
Code Review
This pull request adds support for parsing the "Devour [quality] N" keyword variant (e.g., "Devour land 3") by skipping a leading non-numeric qualifier word if the count does not immediately follow the keyword, and includes corresponding unit tests. The review feedback points out incorrect Comprehensive Rules (CR) citations in both the implementation comments and the tests (citing CR 702.82c instead of CR 702.82b), and suggests using the space1 parser instead of tag(" ") to robustly handle potential multi-space or tab separators.
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.
| let param: Cow<'_, str> = if is_numeric_count_keyword(name) { | ||
| match nom_primitives::parse_number.parse(rest) { | ||
| Ok((remainder, _)) => Cow::Borrowed(&rest[..rest.len() - remainder.len()]), | ||
| // CR 702.82c: "Devour [quality] N" is a variant where the count follows a |
There was a problem hiding this comment.
[MEDIUM] Incorrect CR citation for Devour [quality] N. Evidence:
crates/engine/src/parser/oracle_keyword.rs:1586.
Why it matters: CR 702.82b is the rule that defines the "Devour [quality] N" variant, whereas CR 702.82c governs multiple instances of devour.
Suggested fix: Change the citation to CR 702.82b.
| // CR 702.82c: "Devour [quality] N" is a variant where the count follows a | |
| // CR 702.82b: "Devour [quality] N" is a variant where the count follows a |
References
- CR annotations must be verified and accurate according to Rule R6 of the repository style guide. (link)
| ( | ||
| take_until::<_, _, OracleError<'_>>(" "), | ||
| tag::<_, _, OracleError<'_>>(" "), | ||
| ) |
There was a problem hiding this comment.
[MEDIUM] Use
space1instead oftag(" ")for robustness. Evidence:crates/engine/src/parser/oracle_keyword.rs:1596-1599.
Why it matters: Usingtag(" ")only matches a single space, which can fail if there are multiple spaces or tabs between the qualifier and the count.
Suggested fix: Usespace1to consume any amount of whitespace.
| ( | |
| take_until::<_, _, OracleError<'_>>(" "), | |
| tag::<_, _, OracleError<'_>>(" "), | |
| ) | |
| ( | |
| take_until::<_, _, OracleError<'_>>(" "), | |
| space1::<_, OracleError<'_>>, | |
| ) |
References
- Avoid verbatim string equality for parsing Oracle phrases as it bypasses the robust nom-based parser and creates fragile matches. Instead, decompose compound phrases into modular, reusable parsers for constituent parts and compose them using idiomatic combinator aggregates.
| assert_eq!(kw, Keyword::Cascade); | ||
| } | ||
|
|
||
| /// CR 702.82c: "Devour [quality] N" (Famished Worldsire — "Devour land 3") |
There was a problem hiding this comment.
[MEDIUM] Incorrect CR citation for Devour [quality] N in test. Evidence:
crates/engine/src/parser/oracle_keyword.rs:2221.
Why it matters: CR 702.82b is the rule that defines the "Devour [quality] N" variant, whereas CR 702.82c governs multiple instances of devour.
Suggested fix: Change the citation to CR 702.82b.
| /// CR 702.82c: "Devour [quality] N" (Famished Worldsire — "Devour land 3") | |
| /// CR 702.82b: "Devour [quality] N" (Famished Worldsire — "Devour land 3") |
References
- CR annotations must be verified and accurate according to Rule R6 of the repository style guide. (link)
Parse changes introduced by this PR · 2 card(s), 2 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer review: parser change is at the right seam and scoped to the numeric-count keyword normalizer. Verified CR 702.82c locally, checked the parse-diff sticky comment for Famished Worldsire/Feasting Hobbit, and reviewed the Gemini comments as non-blocking/refuted.
2 participants
Summary
Closes #5260
Famished Worldsire prints "Devour land 3" (CR 702.82c — "Devour [quality] N": as it enters you may sacrifice any number of lands, and it enters with 3 +1/+1 counters per sacrifice). The engine parsed this as
Devour 1, losing the count.Root cause
parse_keyword_from_oracle(crates/engine/src/parser/oracle_keyword.rs) routesdevourthrough the numeric-count-keyword branch, which runsparse_numberon the remainder"land 3". That fails on the leadinglandqualifier, so the param falls back to the whole"land 3"string, and Devour'sFromStr("devour" => Keyword::Devour(p.parse().unwrap_or(1))) hits itsunwrap_or(1)fallback →Devour(1).Fix
When
parse_numberfails at the head, skip a single leading non-numeric qualifier word (viatake_until(" ")+tag(" ")— nom combinators) and retry, so the count token is captured. This generalizes the numeric-count extractor to the CR 702.82cDevour [quality] Ntemplating.Devour Nand every other numeric-count keyword (vanishing 3,crew 3,fading 2, …) are unaffected — the qualifier-skip only fires on theparse_number-fails branch, so there's no regression.Tests
parse_keyword_from_oracle_devour_quality_qualifier_count:devour land 3→Devour(3); plusdevour 3andvanishing 3regression assertions.CR
CR 702.82c ("Devour [quality] N" variant) — verified in
docs/MagicCompRules.txt.Scope note (honest follow-up)
Keyword::Devour(N)stores only the counter multiplier. The "lands only" sacrifice restriction (the CR 702.82c quality) is a separate synthesis-layer concern indatabase/synthesis.rs::synthesize_devourand is left as follow-up — the reported bug (count 3 vs 1) is fully resolved here.