expression: pull MultiColon error into parsing logic, drop AtOutsideOr

In miniscript and in policy we had near-identical logic dealing with :
and @ separators on certain nodes. The differences were:

* In Miniscript we had special handling for aliases, where we would
  synthetically munge the wrappers (characters before ':'). This was
  unnecessary since we can just handle the aliases directly. (Because
  of our munging code, we did some extra error-checking to ensure that
  a PkK fragment always fits into a Check. It does. This checking is
  completely unnecessary.)
* In Policy we forbade the @ character if we were outside of an Or
  context. Also unnecessary. The @ character does not appear in any
  other fragment, so the "unknown fragment" error is already sufficient.

Removes two variants from the giant Error enum.

src/expression/error.rs

@@ -76,6 +76,13 @@ pub enum ParseTreeError {
         /// The position of the opening curly brace.
         pos: usize,
     },
+    ///  Multiple separators (':' or '@') appeared in a node name.
+    MultipleSeparators {
+        /// The separator in question.
+        separator: char,
+        /// The position of the second separator.
+        pos: usize,
+    },
     /// Data occurred after the final ).
     TrailingCharacter {
         /// The first trailing character.
@@ -149,6 +156,13 @@ impl fmt::Display for ParseTreeError {
                 }?;
                 write!(f, ", but found {}", n_children)
             }
+            ParseTreeError::MultipleSeparators { separator, pos } => {
+                write!(
+                    f,
+                    "separator '{}' occured multiple times (second time at position {})",
+                    separator, pos
+                )
+            }
             ParseTreeError::TrailingCharacter { ch, pos } => {
                 write!(f, "trailing data `{}...` (position {})", ch, pos)
             }
@@ -169,6 +183,7 @@ impl std::error::Error for ParseTreeError {
             | ParseTreeError::IllegalCurlyBrace { .. }
             | ParseTreeError::IncorrectName { .. }
             | ParseTreeError::IncorrectNumberOfChildren { .. }
+            | ParseTreeError::MultipleSeparators { .. }
             | ParseTreeError::TrailingCharacter { .. }
             | ParseTreeError::UnknownName { .. } => None,
         }

src/expression/mod.rs

@@ -79,6 +79,25 @@ pub trait FromTree: Sized {
 }
 
 impl<'a> Tree<'a> {
+    /// Split the name by a separating character.
+    ///
+    /// If the separator is present, returns the prefix before the separator and
+    /// the suffix after the separator. Otherwise returns the whole name.
+    ///
+    /// If the separator occurs multiple times, returns an error.
+    pub fn name_separated(&self, separator: char) -> Result<(Option<&str>, &str), ParseTreeError> {
+        let mut name_split = self.name.splitn(3, separator);
+        match (name_split.next(), name_split.next(), name_split.next()) {
+            (None, _, _) => unreachable!("'split' always yields at least one element"),
+            (Some(_), None, _) => Ok((None, self.name)),
+            (Some(prefix), Some(name), None) => Ok((Some(prefix), name)),
+            (Some(_), Some(_), Some(suffix)) => Err(ParseTreeError::MultipleSeparators {
+                separator,
+                pos: self.children_pos - suffix.len() - 1,
+            }),
+        }
+    }
+
     /// Check that a tree node has the given number of children.
     ///
     /// The `description` argument is only used to populate the error return,

src/lib.rs

@@ -427,10 +427,6 @@ pub enum Error {
     UnexpectedStart,
     /// Got something we were not expecting
     Unexpected(String),
-    /// Name of a fragment contained `:` multiple times
-    MultiColon(String),
-    /// Name of a fragment contained `@` but we were not parsing an OR
-    AtOutsideOr(String),
     /// Encountered a wrapping character that we don't recognize
     UnknownWrapper(char),
     /// Parsed a miniscript and the result was not of type T
@@ -500,8 +496,6 @@ impl fmt::Display for Error {
             Error::AddrP2shError(ref e) => fmt::Display::fmt(e, f),
             Error::UnexpectedStart => f.write_str("unexpected start of script"),
             Error::Unexpected(ref s) => write!(f, "unexpected «{}»", s),
-            Error::MultiColon(ref s) => write!(f, "«{}» has multiple instances of «:»", s),
-            Error::AtOutsideOr(ref s) => write!(f, "«{}» contains «@» in non-or() context", s),
             Error::UnknownWrapper(ch) => write!(f, "unknown wrapper «{}:»", ch),
             Error::NonTopLevel(ref s) => write!(f, "non-T miniscript: {}", s),
             Error::Trailing(ref s) => write!(f, "trailing tokens: {}", s),
@@ -553,8 +547,6 @@ impl std::error::Error for Error {
             | InvalidPush(_)
             | UnexpectedStart
             | Unexpected(_)
-            | MultiColon(_)
-            | AtOutsideOr(_)
             | UnknownWrapper(_)
             | NonTopLevel(_)
             | Trailing(_)

src/miniscript/astelem.rs

@@ -37,12 +37,28 @@ impl<Pk: FromStrKey, Ctx: ScriptContext> crate::expression::FromTree for Termina
                     })
             };
 
-        let (frag_name, frag_wrap) = super::split_expression_name(top.name)?;
+        let (frag_wrap, frag_name) = top
+            .name_separated(':')
+            .map_err(From::from)
+            .map_err(Error::Parse)?;
+        // "pk" and "pkh" are aliases for "c:pk_k" and "c:pk_h" respectively.
         let unwrapped = match frag_name {
             "expr_raw_pkh" => top
                 .verify_terminal_parent("expr_raw_pkh", "public key hash")
                 .map(Terminal::RawPkH)
                 .map_err(Error::Parse),
+            "pk" => top
+                .verify_terminal_parent("pk", "public key")
+                .map(Terminal::PkK)
+                .map_err(Error::Parse)
+                .and_then(|term| Miniscript::from_ast(term))
+                .map(|ms| Terminal::Check(Arc::new(ms))),
+            "pkh" => top
+                .verify_terminal_parent("pkh", "public key")
+                .map(Terminal::PkH)
+                .map_err(Error::Parse)
+                .and_then(|term| Miniscript::from_ast(term))
+                .map(|ms| Terminal::Check(Arc::new(ms))),
             "pk_k" => top
                 .verify_terminal_parent("pk_k", "public key")
                 .map(Terminal::PkK)
@@ -132,7 +148,13 @@ impl<Pk: FromStrKey, Ctx: ScriptContext> crate::expression::FromTree for Termina
                 name: x.to_owned(),
             }))),
         }?;
-        let ms = super::wrap_into_miniscript(unwrapped, frag_wrap)?;
+
+        if frag_wrap == Some("") {
+            return Err(Error::Parse(crate::ParseError::Tree(
+                crate::ParseTreeError::UnknownName { name: top.name.to_owned() },
+            )));
+        }
+        let ms = super::wrap_into_miniscript(unwrapped, frag_wrap.unwrap_or(""))?;
         Ok(ms.node)
     }
 }

src/miniscript/mod.rs

@@ -670,71 +670,13 @@ impl<Pk: MiniscriptKey, Ctx: ScriptContext> Miniscript<Pk, Ctx> {
     }
 }
 
-/// Utility function used when parsing a script from an expression tree.
-///
-/// Checks that the name of each fragment has at most one `:`, splits
-/// the name at the `:`, and implements aliases for the old `pk`/`pk_h`
-/// fragments.
-///
-/// Returns the fragment name (right of the `:`) and a list of wrappers
-/// (left of the `:`).
-fn split_expression_name(full_name: &str) -> Result<(&str, Cow<str>), Error> {
-    let mut aliased_wrap;
-    let frag_name;
-    let frag_wrap;
-    let mut name_split = full_name.split(':');
-    match (name_split.next(), name_split.next(), name_split.next()) {
-        (None, _, _) => {
-            frag_name = "";
-            frag_wrap = "".into();
-        }
-        (Some(name), None, _) => {
-            if name == "pk" {
-                frag_name = "pk_k";
-                frag_wrap = "c".into();
-            } else if name == "pkh" {
-                frag_name = "pk_h";
-                frag_wrap = "c".into();
-            } else {
-                frag_name = name;
-                frag_wrap = "".into();
-            }
-        }
-        (Some(wrap), Some(name), None) => {
-            if wrap.is_empty() {
-                return Err(Error::Parse(crate::ParseError::Tree(
-                    crate::ParseTreeError::UnknownName { name: full_name.to_owned() },
-                )));
-            }
-            if name == "pk" {
-                frag_name = "pk_k";
-                aliased_wrap = wrap.to_owned();
-                aliased_wrap.push('c');
-                frag_wrap = aliased_wrap.into();
-            } else if name == "pkh" {
-                frag_name = "pk_h";
-                aliased_wrap = wrap.to_owned();
-                aliased_wrap.push('c');
-                frag_wrap = aliased_wrap.into();
-            } else {
-                frag_name = name;
-                frag_wrap = wrap.into();
-            }
-        }
-        (Some(_), Some(_), Some(_)) => {
-            return Err(Error::MultiColon(full_name.to_owned()));
-        }
-    }
-    Ok((frag_name, frag_wrap))
-}
-
 /// Utility function used when parsing a script from an expression tree.
 ///
 /// Once a Miniscript fragment has been parsed into a terminal, apply any
 /// wrappers that were included in its name.
 fn wrap_into_miniscript<Pk, Ctx>(
     term: Terminal<Pk, Ctx>,
-    frag_wrap: Cow<str>,
+    frag_wrap: &str,
 ) -> Result<Miniscript<Pk, Ctx>, Error>
 where
     Pk: MiniscriptKey,

src/policy/concrete.rs

@@ -850,31 +850,22 @@ impl<Pk: FromStrKey> Policy<Pk> {
         top: &expression::Tree,
         allow_prob: bool,
     ) -> Result<(usize, Policy<Pk>), Error> {
-        let frag_prob;
-        let frag_name;
-        let mut name_split = top.name.split('@');
-        match (name_split.next(), name_split.next(), name_split.next()) {
-            (None, _, _) => {
-                frag_prob = 1;
-                frag_name = "";
-            }
-            (Some(name), None, _) => {
-                frag_prob = 1;
-                frag_name = name;
-            }
-            (Some(prob), Some(name), None) => {
-                if !allow_prob {
-                    return Err(Error::AtOutsideOr(top.name.to_owned()));
-                }
-                frag_prob = expression::parse_num(prob)
-                    .map_err(crate::ParseError::Num)
-                    .map_err(Error::Parse)? as usize;
-                frag_name = name;
-            }
-            (Some(_), Some(_), Some(_)) => {
-                return Err(Error::MultiColon(top.name.to_owned()));
-            }
-        }
+        // When 'allow_prob' is true we parse '@' signs out of node names.
+        let (frag_prob, frag_name) = if allow_prob {
+            top.name_separated('@')
+                .map_err(From::from)
+                .map_err(Error::Parse)?
+        } else {
+            (None, top.name)
+        };
+
+        let frag_prob = match frag_prob {
+            None => 1,
+            Some(s) => expression::parse_num(s)
+                .map_err(From::from)
+                .map_err(Error::Parse)? as usize,
+        };
+
         match frag_name {
             "UNSATISFIABLE" => {
                 top.verify_n_children("UNSATISFIABLE", 0..=0)