diff --git a/src/lib.rs b/src/lib.rs index 7c15601..2e4c04e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -25,6 +25,7 @@ use abnf::types::{Kind, Node, Repeat, Rule, TerminalValues}; use indexmap::map::IndexMap; use itertools::Itertools; use pretty::BoxDoc; +use std::cmp::Ordering; use std::collections::HashSet; mod core_rules; @@ -38,10 +39,16 @@ impl Pretty for Node { fn pretty(&self) -> BoxDoc<'static> { use Node::*; match self { - Alternatives(nodes) => BoxDoc::intersperse( - nodes.iter().map(|x| x.pretty().nest(2).group()), - BoxDoc::space().append(BoxDoc::text("| ")), - ), + Alternatives(nodes) => { + // ABNF alternatives are an unordered set, while pest choice + // is ordered, so render them in a PEG-safe order. + let mut ordered: Vec<&Node> = nodes.iter().collect(); + ordered.sort_by(|a, b| choice_order(a, b)); + BoxDoc::intersperse( + ordered.iter().map(|x| x.pretty().nest(2).group()), + BoxDoc::space().append(BoxDoc::text("| ")), + ) + } Concatenation(nodes) => BoxDoc::intersperse( nodes.iter().map(|x| x.pretty()), BoxDoc::space().append(BoxDoc::text("~ ")), @@ -65,6 +72,39 @@ impl Pretty for Node { } } +/// Whether the pest expression rendered from `node` can succeed without +/// consuming any input. In an ordered choice, such an expression shadows +/// every alternative that follows it, and pest_derive rejects it anywhere +/// but last. +fn cannot_fail(node: &Node) -> bool { + use Node::*; + match node { + String(s) => s.as_str().is_empty(), + Optional(_) => true, + Repetition { repeat, .. } => repeat.min().unwrap_or(0) == 0, + Group(n) => cannot_fail(n), + Concatenation(v) => v.iter().all(cannot_fail), + Alternatives(v) => v.iter().any(cannot_fail), + Rulename(_) | TerminalValues(_) | Prose(_) => false, + } +} + +/// Order alternatives for pest's ordered choice: can't-fail expressions +/// last, and string literals longest first (a literal would otherwise +/// shadow any alternative it is a prefix of). Anything else keeps its +/// written order. +fn choice_order(a: &Node, b: &Node) -> Ordering { + match (cannot_fail(a), cannot_fail(b)) { + (false, true) => return Ordering::Less, + (true, false) => return Ordering::Greater, + _ => {} + } + match (a, b) { + (Node::String(x), Node::String(y)) => y.as_str().len().cmp(&x.as_str().len()), + _ => Ordering::Equal, + } +} + impl Pretty for Repeat { fn pretty(&self) -> BoxDoc<'static> { BoxDoc::text(match (self.min().unwrap_or(0), self.max()) { diff --git a/tests/parse_abnf.rs b/tests/parse_abnf.rs index a3090e6..d543ec2 100644 --- a/tests/parse_abnf.rs +++ b/tests/parse_abnf.rs @@ -44,3 +44,44 @@ fn incremental_without_base_is_initial() { assert!(rules.contains_key("rule")); assert_eq!(rendered_single(&rules, "rule"), "rule = { A | B }"); } + +#[test] +fn empty_alternative_renders_last() { + // pest implements ordered choice and pest_derive rejects an + // alternative that cannot fail unless it comes last: + // + // = expression cannot fail; following choices cannot be reached + // + // The empty alternative must render at the end, whether the + // alternation was written in one rule or assembled with =/. + let direct = parse_abnf("rule = \"\" / A / B / C\n").unwrap(); + let merged = parse_abnf("rule = \"\"\nrule =/ A / B\nrule =/ C\n").unwrap(); + assert_eq!( + rendered_single(&direct, "rule"), + "rule = { A | B | C | ^\"\" }" + ); + assert_eq!( + rendered_single(&merged, "rule"), + "rule = { A | B | C | ^\"\" }" + ); +} + +#[test] +fn longer_string_literals_render_first() { + // Ordered choice also shadows later alternatives that merely extend an + // earlier one: with { ^"foo" | ^"foobar" }, input "foobar" matches + // "foo" and leaves "bar". pest_derive accepts such a grammar without + // complaint, so the emitted order alone decides whether the generated + // parser is correct. For plain string literals, longer-first is + // PEG-safe, however the alternation was written. + let direct = parse_abnf("rule = \"foo\" / \"foobar\"\n").unwrap(); + let merged = parse_abnf("rule = \"foo\"\nrule =/ \"foobar\"\n").unwrap(); + assert_eq!( + rendered_single(&direct, "rule"), + "rule = { ^\"foobar\" | ^\"foo\" }" + ); + assert_eq!( + rendered_single(&merged, "rule"), + "rule = { ^\"foobar\" | ^\"foo\" }" + ); +}