refactor(policy): apply trevarj parser feedback

- Drop BNF grammar from doc comment per Andrew's note that it
  is self-explanatory.
- Wrap parser state in MathParser struct over Peekable<CharIndices>
  to remove byte-indexing and UTF-8 magic numbers.
- Allow arbitrary whitespace between tokens, except inside the
  digraph #{ which remains atomic.
- Replace match_sep helper with From<char> for Op (unreachable! in
  the default arm because the caller verifies the char first).
- Derive PartialEq/Eq on Kind to use == directly.
- Extract consume_while(predicate) helper for digit and identifier
  loops in both parse_math_policy and parse_terminal.

Author: febyeji

src/policy/semantic.rs

@@ -364,123 +364,162 @@ impl<Pk: FromStrKey> str::FromStr for Policy<Pk> {
     }
 }
 
-/// Grammar:
-///
-/// ```text
-///     policy   ::= terminal | "(" group ")" | "#{" thresh_body "}" " = " number
-///     terminal ::= "UNSATISFIABLE" | "TRIVIAL" | name "(" arg ")"
-///     name     ::= "pk" | "after" | "older" | "sha256" | "hash256"
-///                | "ripemd160" | "hash160"
-///     group    ::= policy ((" ∧ " policy)+ | (" ∨ " policy)+)
-///     thresh_body ::= policy ("," " " policy)+
-/// ```
-fn parse_math_policy<Pk: FromStrKey>(s: &str) -> Result<Policy<Pk>, Error> {
-    #[derive(Copy, Clone, PartialEq, Eq)]
-    enum Op {
-        And,
-        Or,
+struct MathParser<'a> {
+    s: &'a str,
+    iter: core::iter::Peekable<core::str::CharIndices<'a>>,
+}
+
+impl<'a> MathParser<'a> {
+    fn new(s: &'a str) -> Self { Self { s, iter: s.char_indices().peekable() } }
+
+    fn peek(&mut self) -> Option<char> { self.iter.peek().map(|&(_, c)| c) }
+
+    fn bump(&mut self) -> Option<char> { self.iter.next().map(|(_, c)| c) }
+
+    fn pos(&mut self) -> usize { self.iter.peek().map(|&(i, _)| i).unwrap_or(self.s.len()) }
+
+    fn skip_ws(&mut self) {
+        while matches!(self.peek(), Some(c) if c.is_whitespace()) {
+            self.bump();
+        }
     }
-    enum Kind {
-        Group(Option<Op>),
-        Thresh,
+
+    fn try_consume(&mut self, want: char) -> bool {
+        if self.peek() == Some(want) {
+            self.bump();
+            true
+        } else {
+            false
+        }
     }
-    struct Frame<Pk: MiniscriptKey> {
-        subs: Vec<Arc<Policy<Pk>>>,
-        kind: Kind,
+
+    fn consume_while(&mut self, pred: impl Fn(char) -> bool) -> &'a str {
+        let start = self.pos();
+        while matches!(self.peek(), Some(c) if pred(c)) {
+            self.bump();
+        }
+        &self.s[start..self.pos()]
     }
+}
 
-    fn match_sep(s: &str) -> Option<Op> {
-        if s.starts_with(" ∧ ") {
-            Some(Op::And)
-        } else if s.starts_with(" ∨ ") {
-            Some(Op::Or)
-        } else {
-            None
+#[derive(Copy, Clone, PartialEq, Eq)]
+enum Op {
+    And,
+    Or,
+}
+
+impl From<char> for Op {
+    fn from(c: char) -> Self {
+        match c {
+            '∧' => Op::And,
+            '∨' => Op::Or,
+            _ => unreachable!(),
         }
     }
+}
 
-    let bytes = s.as_bytes();
+#[derive(PartialEq, Eq)]
+enum Kind {
+    Group(Option<Op>),
+    Thresh,
+}
+
+struct Frame<Pk: MiniscriptKey> {
+    subs: Vec<Arc<Policy<Pk>>>,
+    kind: Kind,
+}
+
+/// Parses the mathematical-notation form produced by `Display`.
+fn parse_math_policy<Pk: FromStrKey>(s: &str) -> Result<Policy<Pk>, Error> {
+    let mut parser = MathParser::new(s);
     let mut frames: Vec<Frame<Pk>> = Vec::new();
     let mut cur: Option<Arc<Policy<Pk>>> = None;
-    let mut i = 0;
 
-    while i < bytes.len() {
+    loop {
+        parser.skip_ws();
+        let Some(c) = parser.peek() else { break };
+
         if cur.is_none() {
-            // Expect: '(', '#{', or a terminal atom.
-            if bytes[i] == b'(' {
-                frames.push(Frame { subs: Vec::new(), kind: Kind::Group(None) });
-                i += 1;
-            } else if s[i..].starts_with("#{") {
-                frames.push(Frame { subs: Vec::new(), kind: Kind::Thresh });
-                i += 2;
-            } else {
-                let (policy, end) = parse_terminal(s, bytes, i)?;
-                cur = Some(Arc::new(policy));
-                i = end;
+            match c {
+                '(' => {
+                    parser.bump();
+                    frames.push(Frame { subs: Vec::new(), kind: Kind::Group(None) });
+                }
+                '#' => {
+                    parser.bump();
+                    if !parser.try_consume('{') {
+                        return Err(malformed_math());
+                    }
+                    frames.push(Frame { subs: Vec::new(), kind: Kind::Thresh });
+                }
+                _ => cur = Some(Arc::new(parse_terminal(&mut parser)?)),
             }
             continue;
         }
 
         let frame = frames.last_mut().ok_or_else(malformed_math)?;
-        if let Some(new_op) = match_sep(&s[i..]) {
-            match &mut frame.kind {
-                Kind::Group(slot @ None) => *slot = Some(new_op),
-                Kind::Group(Some(prev)) if *prev == new_op => {}
-                _ => return Err(malformed_math()),
-            }
-            frame.subs.push(cur.take().unwrap());
-            i += 5; // " ∧ " / " ∨ " in UTF-8
-        } else if bytes[i] == b',' && bytes.get(i + 1) == Some(&b' ') {
-            if !matches!(frame.kind, Kind::Thresh) {
-                return Err(malformed_math());
-            }
-            frame.subs.push(cur.take().unwrap());
-            i += 2;
-        } else if bytes[i] == b')' {
-            let mut frame = frames.pop().unwrap();
-            let op = match frame.kind {
-                Kind::Group(op) => op.ok_or_else(malformed_math)?,
-                Kind::Thresh => return Err(malformed_math()),
-            };
-            frame.subs.push(cur.take().unwrap());
-            if frame.subs.len() < 2 {
-                return Err(malformed_math());
-            }
-            let k = if op == Op::And { frame.subs.len() } else { 1 };
-            cur = Some(Arc::new(Policy::Thresh(
-                Threshold::new(k, frame.subs).map_err(Error::Threshold)?,
-            )));
-            i += 1;
-        } else if bytes[i] == b'}' {
-            let mut frame = match frames.pop() {
-                Some(f) if matches!(f.kind, Kind::Thresh) => f,
-                _ => return Err(malformed_math()),
-            };
-            frame.subs.push(cur.take().unwrap());
-            i += 1;
-            if !s[i..].starts_with(" = ") {
-                return Err(malformed_math());
-            }
-            i += 3;
-            let k_start = i;
-            while i < bytes.len() && bytes[i].is_ascii_digit() {
-                i += 1;
+        match c {
+            '∧' | '∨' => {
+                parser.bump();
+                let op = Op::from(c);
+                match &mut frame.kind {
+                    Kind::Group(slot @ None) => *slot = Some(op),
+                    Kind::Group(Some(prev)) if *prev == op => {}
+                    _ => return Err(malformed_math()),
+                }
+                frame.subs.push(cur.take().unwrap());
             }
-            if i == k_start {
-                return Err(malformed_math());
+            ',' => {
+                if frame.kind != Kind::Thresh {
+                    return Err(malformed_math());
+                }
+                parser.bump();
+                frame.subs.push(cur.take().unwrap());
             }
-            let k = expression::parse_num(&s[k_start..i]).map_err(|_| malformed_math())? as usize;
-            let thresh = Threshold::new(k, frame.subs).map_err(Error::Threshold)?;
-            // k=1 must be `∨`; k=n must be `∧`.
-            if thresh.is_or() {
-                return Err(Error::ParseThreshold(crate::ParseThresholdError::IllegalOr));
+            ')' => {
+                parser.bump();
+                let mut frame = frames.pop().unwrap();
+                let op = match frame.kind {
+                    Kind::Group(op) => op.ok_or_else(malformed_math)?,
+                    Kind::Thresh => return Err(malformed_math()),
+                };
+                frame.subs.push(cur.take().unwrap());
+                if frame.subs.len() < 2 {
+                    return Err(malformed_math());
+                }
+                let k = if op == Op::And { frame.subs.len() } else { 1 };
+                cur = Some(Arc::new(Policy::Thresh(
+                    Threshold::new(k, frame.subs).map_err(Error::Threshold)?,
+                )));
             }
-            if thresh.is_and() {
-                return Err(Error::ParseThreshold(crate::ParseThresholdError::IllegalAnd));
+            '}' => {
+                parser.bump();
+                let mut frame = match frames.pop() {
+                    Some(f) if f.kind == Kind::Thresh => f,
+                    _ => return Err(malformed_math()),
+                };
+                frame.subs.push(cur.take().unwrap());
+                parser.skip_ws();
+                if !parser.try_consume('=') {
+                    return Err(malformed_math());
+                }
+                parser.skip_ws();
+                let k_str = parser.consume_while(|c| c.is_ascii_digit());
+                if k_str.is_empty() {
+                    return Err(malformed_math());
+                }
+                let k = expression::parse_num(k_str).map_err(|_| malformed_math())? as usize;
+                let thresh = Threshold::new(k, frame.subs).map_err(Error::Threshold)?;
+                // k=1 must be `∨`; k=n must be `∧`.
+                if thresh.is_or() {
+                    return Err(Error::ParseThreshold(crate::ParseThresholdError::IllegalOr));
+                }
+                if thresh.is_and() {
+                    return Err(Error::ParseThreshold(crate::ParseThresholdError::IllegalAnd));
+                }
+                cur = Some(Arc::new(Policy::Thresh(thresh)));
             }
-            cur = Some(Arc::new(Policy::Thresh(thresh)));
-        } else {
-            return Err(malformed_math());
+            _ => return Err(malformed_math()),
         }
     }
 
@@ -491,40 +530,26 @@ fn parse_math_policy<Pk: FromStrKey>(s: &str) -> Result<Policy<Pk>, Error> {
     Ok(Arc::try_unwrap(root).unwrap_or_else(|arc| (*arc).clone()))
 }
 
-/// Parses a single terminal starting at `start` and returns the policy and
-/// the byte index immediately after it.
-fn parse_terminal<Pk: FromStrKey>(
-    s: &str,
-    bytes: &[u8],
-    start: usize,
-) -> Result<(Policy<Pk>, usize), Error> {
-    let mut i = start;
-    while i < bytes.len() && bytes[i].is_ascii_alphanumeric() {
-        i += 1;
-    }
-    if i == start {
+fn parse_terminal<Pk: FromStrKey>(parser: &mut MathParser<'_>) -> Result<Policy<Pk>, Error> {
+    let name = parser.consume_while(|c| c.is_ascii_alphanumeric());
+    if name.is_empty() {
         return Err(malformed_math());
     }
-    let name = &s[start..i];
 
-    if i >= bytes.len() || bytes[i] != b'(' {
+    if !parser.try_consume('(') {
         return match name {
-            "UNSATISFIABLE" => Ok((Policy::Unsatisfiable, i)),
-            "TRIVIAL" => Ok((Policy::Trivial, i)),
+            "UNSATISFIABLE" => Ok(Policy::Unsatisfiable),
+            "TRIVIAL" => Ok(Policy::Trivial),
             _ => Err(malformed_math()),
         };
     }
 
-    let arg_start = i + 1;
-    let arg_end = bytes[arg_start..]
-        .iter()
-        .position(|&b| b == b')')
-        .map(|p| p + arg_start)
-        .ok_or_else(malformed_math)?;
-    let arg = &s[arg_start..arg_end];
-    let end = arg_end + 1;
+    let arg = parser.consume_while(|c| c != ')');
+    if !parser.try_consume(')') {
+        return Err(malformed_math());
+    }
 
-    let policy = match name {
+    Ok(match name {
         "pk" => Policy::Key(parse_arg(arg)?),
         "after" => {
             let n = expression::parse_num(arg)
@@ -551,8 +576,7 @@ fn parse_terminal<Pk: FromStrKey>(
         "ripemd160" => Policy::Ripemd160(parse_arg(arg)?),
         "hash160" => Policy::Hash160(parse_arg(arg)?),
         _ => return Err(malformed_math()),
-    };
-    Ok((policy, end))
+    })
 }
 
 fn parse_arg<T: core::str::FromStr>(arg: &str) -> Result<T, Error>
@@ -1016,6 +1040,18 @@ mod tests {
         assert!(StringPolicy::from_str("(pk(A) ∧ pk(B)").is_err());
         assert!(StringPolicy::from_str("#{pk(A), pk(B)} = 1").is_err());
         assert!(StringPolicy::from_str("(and(pk(A),pk(B)) ∧ pk(C))").is_err());
+
+        // Whitespace flexibility: any amount of whitespace between tokens.
+        let canonical = StringPolicy::from_str("(pk(A) ∧ pk(B))").unwrap();
+        assert_eq!(StringPolicy::from_str("(pk(A)∧pk(B))").unwrap(), canonical);
+        assert_eq!(StringPolicy::from_str("( pk(A)   ∧   pk(B) )").unwrap(), canonical);
+
+        let thresh = StringPolicy::from_str("#{pk(A), pk(B), pk(C)} = 2").unwrap();
+        assert_eq!(StringPolicy::from_str("#{pk(A),pk(B),pk(C)}=2").unwrap(), thresh);
+        assert_eq!(StringPolicy::from_str("#{ pk(A) , pk(B) , pk(C) }  =  2").unwrap(), thresh);
+
+        // `#{` is a digraph; whitespace between `#` and `{` is rejected.
+        assert!(StringPolicy::from_str("# {pk(A), pk(B), pk(C)} = 2").is_err());
     }
 
     #[test]