Merge #7491

7491: Simplify mbe match error. r=edwin0cheng a=edwin0cheng

Handle parse error in rule parsing instead of matching in mbe.

bors r+

Co-authored-by: Edwin Cheng <edwin0cheng@gmail.com>

crates/mbe/src/expander.rs

@@ -16,13 +16,8 @@ pub(crate) fn expand_rules(
 ) -> ExpandResult<tt::Subtree> {
     let mut match_: Option<(matcher::Match, &crate::Rule)> = None;
     for rule in rules {
-        let new_match = match matcher::match_(&rule.lhs, input) {
+        let new_match = matcher::match_(&rule.lhs, input);
-            Ok(m) => m,
+
-            Err(_e) => {
-                // error in pattern parsing
-                continue;
-            }
-        };
         if new_match.err.is_none() {
             // If we find a rule that applies without errors, we're done.
             // Unconditionally returning the transcription here makes the

crates/mbe/src/expander/matcher.rs

@@ -77,35 +77,26 @@ impl Match {
     }
 }
 
-// General note: These functions have two channels to return errors, a `Result`
+/// Matching errors are added to the `Match`.
-// return value and the `&mut Match`. The returned Result is for pattern parsing
+pub(super) fn match_(pattern: &MetaTemplate, src: &tt::Subtree) -> Match {
-// errors; if a branch of the macro definition doesn't parse, it doesn't make
-// sense to try using it. Matching errors are added to the `Match`. It might
-// make sense to make pattern parsing a separate step?
-
-pub(super) fn match_(pattern: &MetaTemplate, src: &tt::Subtree) -> Result<Match, ExpandError> {
     assert!(pattern.delimiter == None);
 
     let mut res = Match::default();
     let mut src = TtIter::new(src);
 
-    match_subtree(&mut res, pattern, &mut src)?;
+    match_subtree(&mut res, pattern, &mut src);
 
     if src.len() > 0 {
         res.unmatched_tts += src.len();
         res.add_err(err!("leftover tokens"));
     }
 
-    Ok(res)
+    res
 }
 
-fn match_subtree(
+fn match_subtree(res: &mut Match, pattern: &MetaTemplate, src: &mut TtIter) {
-    res: &mut Match,
-    pattern: &MetaTemplate,
-    src: &mut TtIter,
-) -> Result<(), ExpandError> {
     for op in pattern.iter() {
-        match op.as_ref().map_err(|err| err.clone())? {
+        match op {
             Op::Leaf(lhs) => {
                 let rhs = match src.expect_leaf() {
                     Ok(l) => l,
@@ -145,7 +136,7 @@ fn match_subtree(
                     continue;
                 }
                 let mut src = TtIter::new(rhs);
-                match_subtree(res, lhs, &mut src)?;
+                match_subtree(res, lhs, &mut src);
                 if src.len() > 0 {
                     res.add_err(err!("leftover tokens"));
                 }
@@ -172,11 +163,139 @@ fn match_subtree(
                 }
             }
             Op::Repeat { subtree, kind, separator } => {
-                match_repeat(res, subtree, *kind, separator, src)?;
+                match_repeat(res, subtree, *kind, separator, src);
             }
         }
     }
-    Ok(())
+}
+
+pub(super) fn match_repeat(
+    res: &mut Match,
+    pattern: &MetaTemplate,
+    kind: RepeatKind,
+    separator: &Option<Separator>,
+    src: &mut TtIter,
+) {
+    // Dirty hack to make macro-expansion terminate.
+    // This should be replaced by a proper macro-by-example implementation
+    let mut limit = 65536;
+    let mut counter = 0;
+
+    for i in 0.. {
+        let mut fork = src.clone();
+
+        if let Some(separator) = &separator {
+            if i != 0 && !fork.eat_separator(separator) {
+                break;
+            }
+        }
+
+        let mut nested = Match::default();
+        match_subtree(&mut nested, pattern, &mut fork);
+        if nested.err.is_none() {
+            limit -= 1;
+            if limit == 0 {
+                log::warn!(
+                    "match_lhs exceeded repeat pattern limit => {:#?}\n{:#?}\n{:#?}\n{:#?}",
+                    pattern,
+                    src,
+                    kind,
+                    separator
+                );
+                break;
+            }
+            *src = fork;
+
+            if let Err(err) = res.bindings.push_nested(counter, nested.bindings) {
+                res.add_err(err);
+            }
+            counter += 1;
+            if counter == 1 {
+                if let RepeatKind::ZeroOrOne = kind {
+                    break;
+                }
+            }
+        } else {
+            break;
+        }
+    }
+
+    match (kind, counter) {
+        (RepeatKind::OneOrMore, 0) => {
+            res.add_err(ExpandError::UnexpectedToken);
+        }
+        (_, 0) => {
+            // Collect all empty variables in subtrees
+            let mut vars = Vec::new();
+            collect_vars(&mut vars, pattern);
+            for var in vars {
+                res.bindings.push_empty(&var)
+            }
+        }
+        _ => (),
+    }
+}
+
+fn match_meta_var(kind: &str, input: &mut TtIter) -> ExpandResult<Option<Fragment>> {
+    let fragment = match kind {
+        "path" => Path,
+        "expr" => Expr,
+        "ty" => Type,
+        "pat" => Pattern,
+        "stmt" => Statement,
+        "block" => Block,
+        "meta" => MetaItem,
+        "item" => Item,
+        _ => {
+            let tt_result = match kind {
+                "ident" => input
+                    .expect_ident()
+                    .map(|ident| Some(tt::Leaf::from(ident.clone()).into()))
+                    .map_err(|()| err!("expected ident")),
+                "tt" => input.expect_tt().map(Some).map_err(|()| err!()),
+                "lifetime" => input
+                    .expect_lifetime()
+                    .map(|tt| Some(tt))
+                    .map_err(|()| err!("expected lifetime")),
+                "literal" => {
+                    let neg = input.eat_char('-');
+                    input
+                        .expect_literal()
+                        .map(|literal| {
+                            let lit = tt::Leaf::from(literal.clone());
+                            match neg {
+                                None => Some(lit.into()),
+                                Some(neg) => Some(tt::TokenTree::Subtree(tt::Subtree {
+                                    delimiter: None,
+                                    token_trees: vec![neg, lit.into()],
+                                })),
+                            }
+                        })
+                        .map_err(|()| err!())
+                }
+                // `vis` is optional
+                "vis" => match input.eat_vis() {
+                    Some(vis) => Ok(Some(vis)),
+                    None => Ok(None),
+                },
+                _ => Err(ExpandError::UnexpectedToken),
+            };
+            return tt_result.map(|it| it.map(Fragment::Tokens)).into();
+        }
+    };
+    let result = input.expect_fragment(fragment);
+    result.map(|tt| if kind == "expr" { tt.map(Fragment::Ast) } else { tt.map(Fragment::Tokens) })
+}
+
+fn collect_vars(buf: &mut Vec<SmolStr>, pattern: &MetaTemplate) {
+    for op in pattern.iter() {
+        match op {
+            Op::Var { name, .. } => buf.push(name.clone()),
+            Op::Leaf(_) => (),
+            Op::Subtree(subtree) => collect_vars(buf, subtree),
+            Op::Repeat { subtree, .. } => collect_vars(buf, subtree),
+        }
+    }
 }
 
 impl<'a> TtIter<'a> {
@@ -369,134 +488,3 @@ impl<'a> TtIter<'a> {
         }
     }
 }
-
-pub(super) fn match_repeat(
-    res: &mut Match,
-    pattern: &MetaTemplate,
-    kind: RepeatKind,
-    separator: &Option<Separator>,
-    src: &mut TtIter,
-) -> Result<(), ExpandError> {
-    // Dirty hack to make macro-expansion terminate.
-    // This should be replaced by a proper macro-by-example implementation
-    let mut limit = 65536;
-    let mut counter = 0;
-
-    for i in 0.. {
-        let mut fork = src.clone();
-
-        if let Some(separator) = &separator {
-            if i != 0 && !fork.eat_separator(separator) {
-                break;
-            }
-        }
-
-        let mut nested = Match::default();
-        match_subtree(&mut nested, pattern, &mut fork)?;
-        if nested.err.is_none() {
-            limit -= 1;
-            if limit == 0 {
-                log::warn!(
-                    "match_lhs exceeded repeat pattern limit => {:#?}\n{:#?}\n{:#?}\n{:#?}",
-                    pattern,
-                    src,
-                    kind,
-                    separator
-                );
-                break;
-            }
-            *src = fork;
-
-            if let Err(err) = res.bindings.push_nested(counter, nested.bindings) {
-                res.add_err(err);
-            }
-            counter += 1;
-            if counter == 1 {
-                if let RepeatKind::ZeroOrOne = kind {
-                    break;
-                }
-            }
-        } else {
-            break;
-        }
-    }
-
-    match (kind, counter) {
-        (RepeatKind::OneOrMore, 0) => {
-            res.add_err(ExpandError::UnexpectedToken);
-        }
-        (_, 0) => {
-            // Collect all empty variables in subtrees
-            let mut vars = Vec::new();
-            collect_vars(&mut vars, pattern)?;
-            for var in vars {
-                res.bindings.push_empty(&var)
-            }
-        }
-        _ => (),
-    }
-    Ok(())
-}
-
-fn match_meta_var(kind: &str, input: &mut TtIter) -> ExpandResult<Option<Fragment>> {
-    let fragment = match kind {
-        "path" => Path,
-        "expr" => Expr,
-        "ty" => Type,
-        "pat" => Pattern,
-        "stmt" => Statement,
-        "block" => Block,
-        "meta" => MetaItem,
-        "item" => Item,
-        _ => {
-            let tt_result = match kind {
-                "ident" => input
-                    .expect_ident()
-                    .map(|ident| Some(tt::Leaf::from(ident.clone()).into()))
-                    .map_err(|()| err!("expected ident")),
-                "tt" => input.expect_tt().map(Some).map_err(|()| err!()),
-                "lifetime" => input
-                    .expect_lifetime()
-                    .map(|tt| Some(tt))
-                    .map_err(|()| err!("expected lifetime")),
-                "literal" => {
-                    let neg = input.eat_char('-');
-                    input
-                        .expect_literal()
-                        .map(|literal| {
-                            let lit = tt::Leaf::from(literal.clone());
-                            match neg {
-                                None => Some(lit.into()),
-                                Some(neg) => Some(tt::TokenTree::Subtree(tt::Subtree {
-                                    delimiter: None,
-                                    token_trees: vec![neg, lit.into()],
-                                })),
-                            }
-                        })
-                        .map_err(|()| err!())
-                }
-                // `vis` is optional
-                "vis" => match input.eat_vis() {
-                    Some(vis) => Ok(Some(vis)),
-                    None => Ok(None),
-                },
-                _ => Err(ExpandError::UnexpectedToken),
-            };
-            return tt_result.map(|it| it.map(Fragment::Tokens)).into();
-        }
-    };
-    let result = input.expect_fragment(fragment);
-    result.map(|tt| if kind == "expr" { tt.map(Fragment::Ast) } else { tt.map(Fragment::Tokens) })
-}
-
-fn collect_vars(buf: &mut Vec<SmolStr>, pattern: &MetaTemplate) -> Result<(), ExpandError> {
-    for op in pattern.iter() {
-        match op.as_ref().map_err(|e| e.clone())? {
-            Op::Var { name, .. } => buf.push(name.clone()),
-            Op::Leaf(_) => (),
-            Op::Subtree(subtree) => collect_vars(buf, subtree)?,
-            Op::Repeat { subtree, .. } => collect_vars(buf, subtree)?,
-        }
-    }
-    Ok(())
-}

crates/mbe/src/expander/transcriber.rs

@@ -86,13 +86,6 @@ fn expand_subtree(
     let start_elements = arena.len();
     let mut err = None;
     for op in template.iter() {
-        let op = match op {
-            Ok(op) => op,
-            Err(e) => {
-                err = Some(e.clone());
-                break;
-            }
-        };
         match op {
             Op::Leaf(tt) => arena.push(tt.clone().into()),
             Op::Subtree(tt) => {

crates/mbe/src/lib.rs

@@ -24,7 +24,9 @@ use crate::{
 
 #[derive(Debug, PartialEq, Eq)]
 pub enum ParseError {
+    UnexpectedToken(String),
     Expected(String),
+    InvalidRepeat,
     RepetitionEmptyTokenTree,
 }
 
@@ -34,7 +36,6 @@ pub enum ExpandError {
     UnexpectedToken,
     BindingError(String),
     ConversionError,
-    InvalidRepeat,
     ProcMacroError(tt::ExpansionError),
     UnresolvedProcMacro,
     Other(String),
@@ -53,7 +54,6 @@ impl fmt::Display for ExpandError {
             ExpandError::UnexpectedToken => f.write_str("unexpected token in input"),
             ExpandError::BindingError(e) => f.write_str(e),
             ExpandError::ConversionError => f.write_str("could not convert tokens"),
-            ExpandError::InvalidRepeat => f.write_str("invalid repeat expression"),
             ExpandError::ProcMacroError(e) => e.fmt(f),
             ExpandError::UnresolvedProcMacro => f.write_str("unresolved proc macro"),
             ExpandError::Other(e) => f.write_str(e),
@@ -94,11 +94,11 @@ struct Rule {
 #[derive(Clone, Debug, PartialEq, Eq)]
 struct MetaTemplate {
     delimiter: Option<Delimiter>,
-    tokens: Vec<Result<Op, ExpandError>>,
+    tokens: Vec<Op>,
 }
 
 impl<'a> MetaTemplate {
-    fn iter(&self) -> impl Iterator<Item = &Result<Op, ExpandError>> {
+    fn iter(&self) -> impl Iterator<Item = &Op> {
         self.tokens.iter()
     }
 
@@ -288,25 +288,15 @@ impl Rule {
             .expect_subtree()
             .map_err(|()| ParseError::Expected("expected subtree".to_string()))?;
 
-        let lhs = MetaTemplate { tokens: parse_pattern(&lhs), delimiter: None };
+        let lhs = MetaTemplate { tokens: parse_pattern(&lhs)?, delimiter: None };
-        let rhs = MetaTemplate { tokens: parse_template(&rhs), delimiter: None };
+        let rhs = MetaTemplate { tokens: parse_template(&rhs)?, delimiter: None };
 
         Ok(crate::Rule { lhs, rhs })
     }
 }
 
-fn to_parse_error(e: &ExpandError) -> ParseError {
-    let msg = match e {
-        ExpandError::InvalidRepeat => "invalid repeat".to_string(),
-        _ => "invalid macro definition".to_string(),
-    };
-    ParseError::Expected(msg)
-}
-
 fn validate(pattern: &MetaTemplate) -> Result<(), ParseError> {
     for op in pattern.iter() {
-        let op = op.as_ref().map_err(|e| to_parse_error(&e))?;
-
         match op {
             Op::Subtree(subtree) => validate(&subtree)?,
             Op::Repeat { subtree, separator, .. } => {
@@ -315,20 +305,21 @@ fn validate(pattern: &MetaTemplate) -> Result<(), ParseError> {
 
                 if separator.is_none() {
                     if subtree.iter().all(|child_op| {
-                        match child_op.as_ref().map_err(to_parse_error) {
+                        match child_op {
-                            Ok(Op::Var { kind, .. }) => {
+                            Op::Var { kind, .. } => {
                                 // vis is optional
                                 if kind.as_ref().map_or(false, |it| it == "vis") {
                                     return true;
                                 }
                             }
-                            Ok(Op::Repeat { kind, .. }) => {
+                            Op::Repeat { kind, .. } => {
                                 return matches!(
                                     kind,
                                     parser::RepeatKind::ZeroOrMore | parser::RepeatKind::ZeroOrOne
                                 )
                             }
-                            _ => {}
+                            Op::Leaf(_) => {}
+                            Op::Subtree(_) => {}
                         }
                         false
                     }) {

crates/mbe/src/parser.rs

@@ -4,7 +4,7 @@
 use smallvec::SmallVec;
 use syntax::SmolStr;
 
-use crate::{tt_iter::TtIter, ExpandError, MetaTemplate};
+use crate::{tt_iter::TtIter, MetaTemplate, ParseError};
 
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub(crate) enum Op {
@@ -46,12 +46,12 @@ impl PartialEq for Separator {
     }
 }
 
-pub(crate) fn parse_template(template: &tt::Subtree) -> Vec<Result<Op, ExpandError>> {
+pub(crate) fn parse_template(template: &tt::Subtree) -> Result<Vec<Op>, ParseError> {
-    parse_inner(&template, Mode::Template)
+    parse_inner(&template, Mode::Template).into_iter().collect()
 }
 
-pub(crate) fn parse_pattern(pattern: &tt::Subtree) -> Vec<Result<Op, ExpandError>> {
+pub(crate) fn parse_pattern(pattern: &tt::Subtree) -> Result<Vec<Op>, ParseError> {
-    parse_inner(&pattern, Mode::Pattern)
+    parse_inner(&pattern, Mode::Pattern).into_iter().collect()
 }
 
 #[derive(Clone, Copy)]
@@ -60,7 +60,7 @@ enum Mode {
     Template,
 }
 
-fn parse_inner(tt: &tt::Subtree, mode: Mode) -> Vec<Result<Op, ExpandError>> {
+fn parse_inner(tt: &tt::Subtree, mode: Mode) -> Vec<Result<Op, ParseError>> {
     let mut src = TtIter::new(&tt);
     std::iter::from_fn(move || {
         let first = src.next()?;
@@ -71,7 +71,7 @@ fn parse_inner(tt: &tt::Subtree, mode: Mode) -> Vec<Result<Op, ExpandError>> {
 
 macro_rules! err {
     ($($tt:tt)*) => {
-        ExpandError::UnexpectedToken
+        ParseError::UnexpectedToken(($($tt)*).to_string())
     };
 }
 
@@ -81,7 +81,7 @@ macro_rules! bail {
     };
 }
 
-fn next_op<'a>(first: &tt::TokenTree, src: &mut TtIter<'a>, mode: Mode) -> Result<Op, ExpandError> {
+fn next_op<'a>(first: &tt::TokenTree, src: &mut TtIter<'a>, mode: Mode) -> Result<Op, ParseError> {
     let res = match first {
         tt::TokenTree::Leaf(leaf @ tt::Leaf::Punct(tt::Punct { char: '$', .. })) => {
             // Note that the '$' itself is a valid token inside macro_rules.
@@ -93,7 +93,9 @@ fn next_op<'a>(first: &tt::TokenTree, src: &mut TtIter<'a>, mode: Mode) -> Resul
                 tt::TokenTree::Subtree(subtree) => {
                     let (separator, kind) = parse_repeat(src)?;
                     let delimiter = subtree.delimiter;
-                    let tokens = parse_inner(&subtree, mode);
+                    let tokens = parse_inner(&subtree, mode)
+                        .into_iter()
+                        .collect::<Result<Vec<Op>, ParseError>>()?;
                     let subtree = MetaTemplate { tokens, delimiter };
                     Op::Repeat { subtree, separator, kind }
                 }
@@ -102,7 +104,7 @@ fn next_op<'a>(first: &tt::TokenTree, src: &mut TtIter<'a>, mode: Mode) -> Resul
                         static UNDERSCORE: SmolStr = SmolStr::new_inline("_");
 
                         if punct.char != '_' {
-                            return Err(ExpandError::UnexpectedToken);
+                            return Err(ParseError::Expected("_".to_string()));
                         }
                         let name = UNDERSCORE.clone();
                         let kind = eat_fragment_kind(src, mode)?;
@@ -135,7 +137,9 @@ fn next_op<'a>(first: &tt::TokenTree, src: &mut TtIter<'a>, mode: Mode) -> Resul
         tt::TokenTree::Leaf(tt) => Op::Leaf(tt.clone()),
         tt::TokenTree::Subtree(subtree) => {
             let delimiter = subtree.delimiter;
-            let tokens = parse_inner(&subtree, mode);
+            let tokens =
+                parse_inner(&subtree, mode).into_iter().collect::<Result<Vec<Op>, ParseError>>()?;
+
             let subtree = MetaTemplate { tokens, delimiter };
             Op::Subtree(subtree)
         }
@@ -143,7 +147,7 @@ fn next_op<'a>(first: &tt::TokenTree, src: &mut TtIter<'a>, mode: Mode) -> Resul
     Ok(res)
 }
 
-fn eat_fragment_kind<'a>(src: &mut TtIter<'a>, mode: Mode) -> Result<Option<SmolStr>, ExpandError> {
+fn eat_fragment_kind<'a>(src: &mut TtIter<'a>, mode: Mode) -> Result<Option<SmolStr>, ParseError> {
     if let Mode::Pattern = mode {
         src.expect_char(':').map_err(|()| err!("bad fragment specifier 1"))?;
         let ident = src.expect_ident().map_err(|()| err!("bad fragment specifier 1"))?;
@@ -156,12 +160,12 @@ fn is_boolean_literal(lit: &tt::Literal) -> bool {
     matches!(lit.text.as_str(), "true" | "false")
 }
 
-fn parse_repeat(src: &mut TtIter) -> Result<(Option<Separator>, RepeatKind), ExpandError> {
+fn parse_repeat(src: &mut TtIter) -> Result<(Option<Separator>, RepeatKind), ParseError> {
     let mut separator = Separator::Puncts(SmallVec::new());
     for tt in src {
         let tt = match tt {
             tt::TokenTree::Leaf(leaf) => leaf,
-            tt::TokenTree::Subtree(_) => return Err(ExpandError::InvalidRepeat),
+            tt::TokenTree::Subtree(_) => return Err(ParseError::InvalidRepeat),
         };
         let has_sep = match &separator {
             Separator::Puncts(puncts) => !puncts.is_empty(),
@@ -169,7 +173,7 @@ fn parse_repeat(src: &mut TtIter) -> Result<(Option<Separator>, RepeatKind), Exp
         };
         match tt {
             tt::Leaf::Ident(_) | tt::Leaf::Literal(_) if has_sep => {
-                return Err(ExpandError::InvalidRepeat)
+                return Err(ParseError::InvalidRepeat)
             }
             tt::Leaf::Ident(ident) => separator = Separator::Ident(ident.clone()),
             tt::Leaf::Literal(lit) => separator = Separator::Literal(lit.clone()),
@@ -182,11 +186,11 @@ fn parse_repeat(src: &mut TtIter) -> Result<(Option<Separator>, RepeatKind), Exp
                         match &mut separator {
                             Separator::Puncts(puncts) => {
                                 if puncts.len() == 3 {
-                                    return Err(ExpandError::InvalidRepeat);
+                                    return Err(ParseError::InvalidRepeat);
                                 }
                                 puncts.push(punct.clone())
                             }
-                            _ => return Err(ExpandError::InvalidRepeat),
+                            _ => return Err(ParseError::InvalidRepeat),
                         }
                         continue;
                     }
@@ -196,5 +200,5 @@ fn parse_repeat(src: &mut TtIter) -> Result<(Option<Separator>, RepeatKind), Exp
             }
         }
     }
-    Err(ExpandError::InvalidRepeat)
+    Err(ParseError::InvalidRepeat)
 }

crates/mbe/src/tests.rs

@@ -33,19 +33,18 @@ mod rule_parsing {
 
     #[test]
     fn test_invalid_arms() {
-        fn check(macro_body: &str, err: &str) {
+        fn check(macro_body: &str, err: ParseError) {
             let m = parse_macro_arm(macro_body);
-            assert_eq!(m, Err(ParseError::Expected(String::from(err))));
+            assert_eq!(m, Err(err.into()));
         }
+        check("invalid", ParseError::Expected("expected subtree".into()));
 
-        check("invalid", "expected subtree");
+        check("$i:ident => ()", ParseError::Expected("expected subtree".into()));
+        check("($i:ident) ()", ParseError::Expected("expected `=`".into()));
+        check("($($i:ident)_) => ()", ParseError::InvalidRepeat);
 
-        check("$i:ident => ()", "expected subtree");
+        check("($i) => ($i)", ParseError::UnexpectedToken("bad fragment specifier 1".into()));
-        check("($i:ident) ()", "expected `=`");
+        check("($i:) => ($i)", ParseError::UnexpectedToken("bad fragment specifier 1".into()));
-        check("($($i:ident)_) => ()", "invalid repeat");
-
-        check("($i) => ($i)", "invalid macro definition");
-        check("($i:) => ($i)", "invalid macro definition");
     }
 
     fn parse_macro_arm(arm_definition: &str) -> Result<crate::MacroRules, ParseError> {