Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion crates/engine/src/parser/oracle_effect/imperative.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1817,7 +1817,13 @@ pub(super) fn parse_targeted_action_ast(
let (target, target_syntax, _count_for_shape) = match counted_return {
Some((target, c)) => (target, TargetSyntax::TargetKeyword, c),
None => {
let (target, _rem, syntax) = parse_target_with_syntax(target_text, ctx);
let (target, rem, syntax) = parse_target_with_syntax(target_text, ctx);
// CR 608.2c: "... a non-Avatar creature card or a planeswalker card"
// names two alternatives; `parse_target_with_syntax` stops at the
// article-led connector, so fold the remaining disjuncts in rather
// than dropping them (Overlord of the Balemurk).
let (target, _rem) =
crate::parser::oracle_target::fold_article_led_or_type_phrases(target, rem);
#[cfg(debug_assertions)]
assert_no_compound_remainder(_rem, text);
(target, syntax, QuantityExpr::Fixed { value: 0 })
Expand Down
120 changes: 120 additions & 0 deletions crates/engine/src/parser/oracle_target.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1866,6 +1866,65 @@
parse_type_phrase_with_ctx(text, &mut ParseContext::default())
}

/// Consume an `"or"` connector whose right-hand side is article-led
/// (`"or a "` / `"or an "`) or bare (`"or "`), returning the text after it.
fn parse_or_connector_with_optional_article(input: &str) -> OracleResult<'_, ()> {
let (input, _) = nom::character::complete::space0.parse(input)?;
let (input, _) = tag("or ").parse(input)?;
let (input, _) = opt(nom_primitives::parse_article).parse(input)?;
Ok((input, ()))
}

/// CR 608.2c: A filter phrase may name its alternatives as an article-led
/// disjunction — "a non-Avatar creature card **or a** planeswalker card"
/// (Overlord of the Balemurk). `parse_type_phrase` stops at the connector and
/// hands the right-hand side back as remainder (see
/// `parse_type_phrase_leaves_article_led_or_rhs_as_remainder`), because a bare
/// `"or <type>"` union and an article-led `"or a <type>"` continuation are
/// different productions.
///
/// Callers that would otherwise discard that remainder must fold it in, or the
/// spell silently loses an alternative the Oracle text named — the planeswalker
/// half of Overlord's return was unreachable this way. Each disjunct is parsed
/// by the full `parse_type_phrase` grammar and unioned into `TargetFilter::Or`,
/// so this composes for any number of alternatives rather than one card's shape.
///
/// Returns `head` untouched (and the remainder unconsumed) when the remainder
/// does not open an article-led disjunct, so it is safe to call unconditionally.
pub(crate) fn fold_article_led_or_type_phrases<'a>(

Check failure on line 1894 in crates/engine/src/parser/oracle_target.rs

View workflow job for this annotation

GitHub Actions / Rust lint (fmt, clippy, parser gate)

the following explicit lifetimes could be elided: 'a
head: TargetFilter,
remainder: &'a str,
) -> (TargetFilter, &'a str) {
let mut filters = vec![head];
let mut rest = remainder;

loop {
let lower = rest.to_lowercase();
let Ok((after_connector, ())) = parse_or_connector_with_optional_article(&lower) else {
break;
};
// Map the lowercase offset back onto the original-case slice so subtype
// capitalization ("Avatar", "Plan") survives into the parsed filter.
let rhs = &rest[rest.len() - after_connector.len()..];
let (filter, next) = parse_type_phrase(rhs);
// A leg that consumed nothing is not a type phrase (e.g. "or a card in
// your hand"); leave it for the caller rather than looping forever.
if next.len() >= rhs.len() {
break;
}
filters.push(filter);
rest = next;
}

match filters.len() {
1 => (
filters.pop().expect("head filter is always present"),
remainder,
),
_ => (TargetFilter::Or { filters }, rest),
}
}
Comment on lines +1869 to +1926

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.

high

Calling to_lowercase() inside the loop allocates a new String on every iteration, which is inefficient. More importantly, using rest.len() - after_connector.len() to slice the original string rest is unsafe: because converting a string to lowercase can change its byte length in UTF-8 (e.g., with certain Unicode characters), this offset calculation can point to an invalid byte boundary and cause a panic at runtime.

Using tag_no_case allows parsing the connector case-insensitively directly on the original rest slice, completely avoiding the allocation and the dangerous slicing logic.

fn parse_or_connector_with_optional_article(input: &str) -> OracleResult<'_, ()> {
    let (input, _) = nom::character::complete::space0.parse(input)?;
    let (input, _) = nom::bytes::complete::tag_no_case("or ").parse(input)?;
    let (input, _) = opt(nom_primitives::parse_article).parse(input)?;
    Ok((input, ()))
}

/// CR 608.2c: A filter phrase may name its alternatives as an article-led
/// disjunction - "a non-Avatar creature card or a planeswalker card"
/// (Overlord of the Balemurk). parse_type_phrase stops at the connector and
/// hands the right-hand side back as remainder (see
/// parse_type_phrase_leaves_article_led_or_rhs_as_remainder), because a bare
/// "or <type>" union and an article-led "or a <type>" continuation are
/// different productions.
///
/// Callers that would otherwise discard that remainder must fold it in, or the
/// spell silently loses an alternative the Oracle text named - the planeswalker
/// half of Overlord's return was unreachable this way. Each disjunct is parsed
/// by the full parse_type_phrase grammar and unioned into TargetFilter::Or,
/// so this composes for any number of alternatives rather than one card's shape.
///
/// Returns head untouched (and the remainder unconsumed) when the remainder
/// does not open an article-led disjunct, so it is safe to call unconditionally.
pub(crate) fn fold_article_led_or_type_phrases<'a>(
    head: TargetFilter,
    remainder: &'a str,
) -> (TargetFilter, &'a str) {
    let mut filters = vec![head];
    let mut rest = remainder;

    loop {
        let Ok((next_rest, ())) = parse_or_connector_with_optional_article(rest) else {
            break;
        };
        let (filter, next) = parse_type_phrase(next_rest);
        // A leg that consumed nothing is not a type phrase (e.g. "or a card in
        // your hand"); leave it for the caller rather than looping forever.
        if next.len() >= next_rest.len() {
            break;
        }
        filters.push(filter);
        rest = next;
    }

    match filters.len() {
        1 => (
            filters.pop().expect("head filter is always present"),
            remainder,
        ),
        _ => (TargetFilter::Or { filters }, rest),
    }
}
References
  1. 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 (e.g., subjects, conjunctions) and compose them using idiomatic combinator aggregates (like nested alt and tag sequences) to prevent combinatorial explosion and improve maintainability.


/// CR 608.2c: separator byte length for a mass-target union continuation
/// ("…, all artifacts, and all enchantments"). Longest-match-first over the
/// comma / "and" / "or" connectors. Returns `None` when `lower` does not start
Expand Down Expand Up @@ -15385,6 +15444,67 @@
assert!(tf.type_filters.contains(&TypeFilter::Creature));
}

/// CR 608.2c: the fold turns the article-led RHS that `parse_type_phrase`
/// leaves behind into a real `Or` union — Overlord of the Balemurk's
/// "a non-Avatar creature card or a planeswalker card".
#[test]
fn fold_article_led_or_unions_planeswalker_disjunct() {
let (head, rest) = parse_type_phrase("non-Avatar creature card or a planeswalker card");
assert_eq!(
rest, "or a planeswalker card",
"the disjunct is left unparsed"
);

let (folded, rest) = fold_article_led_or_type_phrases(head, rest);
assert!(
rest.trim().is_empty(),
"disjunct must be consumed: {rest:?}"
);
let TargetFilter::Or { filters } = folded else {
panic!("expected Or union, got {folded:?}");
};
assert_eq!(filters.len(), 2, "one leg per named alternative");
assert!(
filters.iter().any(|f| matches!(
f,
TargetFilter::Typed(tf) if tf.type_filters.contains(&TypeFilter::Planeswalker)
)),
"the planeswalker alternative must survive: {filters:?}"
);
assert!(
filters.iter().any(|f| matches!(
f,
TargetFilter::Typed(tf)
if tf.type_filters.contains(&TypeFilter::Creature)
&& tf.type_filters.contains(&TypeFilter::Non(Box::new(
TypeFilter::Subtype("Avatar".to_string())
)))
)),
"the non-Avatar creature alternative must survive: {filters:?}"
);
}

/// The fold is a no-op when the remainder is not an article-led disjunct,
/// so callers may invoke it unconditionally.
#[test]
fn fold_article_led_or_is_noop_without_disjunct() {
let (head, rest) = parse_type_phrase("creature card");
let head_clone = head.clone();
let (folded, rest2) = fold_article_led_or_type_phrases(head, rest);
assert_eq!(folded, head_clone, "filter must be returned untouched");
assert_eq!(rest2, rest, "remainder must be left unconsumed");
}

/// A trailing "or" clause that is not a type phrase must be left for the
/// caller rather than swallowed (and must not loop).
#[test]
fn fold_article_led_or_leaves_non_type_phrase_rhs() {
let head = TargetFilter::Typed(TypedFilter::new(TypeFilter::Creature));
let (folded, rest) = fold_article_led_or_type_phrases(head.clone(), " or you lose 2 life");
assert_eq!(folded, head);
assert_eq!(rest, " or you lose 2 life");
}

/// Regression: a bare (no-article) connector RHS still parses to an Or via
/// the existing non-comma separator branch (unchanged by this work).
#[test]
Expand Down
Loading