feat: report errors in macro definition

Reporting macro *definition* error at the macro *call site* is a rather
questionable approach, but at least we don't erase the errors
altogether!

crates/hir_def/src/macro_expansion_tests/mbe.rs

@@ -1,5 +1,6 @@
 mod tt_conversion;
 mod matching;
+mod meta_syntax;
 
 use expect_test::expect;
 

crates/hir_def/src/macro_expansion_tests/mbe/meta_syntax.rs

@@ -0,0 +1,77 @@
+use expect_test::expect;
+
+use crate::macro_expansion_tests::check;
+
+#[test]
+fn well_formed_macro_rules() {
+    check(
+        r#"
+macro_rules! m {
+    ($i:ident) => ();
+    ($(x),*) => ();
+    ($(x)_*) => ();
+    ($(x)i*) => ();
+    ($($i:ident)*) => ($_);
+    ($($true:ident)*) => ($true);
+    ($($false:ident)*) => ($false);
+    ($) => ($);
+}
+m!($);
+"#,
+        expect![[r#"
+macro_rules! m {
+    ($i:ident) => ();
+    ($(x),*) => ();
+    ($(x)_*) => ();
+    ($(x)i*) => ();
+    ($($i:ident)*) => ($_);
+    ($($true:ident)*) => ($true);
+    ($($false:ident)*) => ($false);
+    ($) => ($);
+}
+$
+"#]],
+    )
+}
+
+#[test]
+fn malformed_macro_rules() {
+    check(
+        r#"
+macro_rules! i1 { invalid }
+i1!();
+
+macro_rules! e1 { $i:ident => () }
+e1!();
+macro_rules! e2 { ($i:ident) () }
+e2!();
+macro_rules! e3 { ($(i:ident)_) => () }
+e3!();
+
+macro_rules! f1 { ($i) => ($i) }
+f1!();
+macro_rules! f2 { ($i:) => ($i) }
+f2!();
+macro_rules! f3 { ($i:_) => () }
+f3!();
+"#,
+        expect![[r#"
+macro_rules! i1 { invalid }
+/* error: invalid macro definition: expected subtree */
+
+macro_rules! e1 { $i:ident => () }
+/* error: invalid macro definition: expected subtree */
+macro_rules! e2 { ($i:ident) () }
+/* error: invalid macro definition: expected `=` */
+macro_rules! e3 { ($(i:ident)_) => () }
+/* error: invalid macro definition: invalid repeat */
+
+macro_rules! f1 { ($i) => ($i) }
+/* error: invalid macro definition: bad fragment specifier 1 */
+macro_rules! f2 { ($i:) => ($i) }
+/* error: invalid macro definition: bad fragment specifier 1 */
+macro_rules! f3 { ($i:_) => () }
+/* error: invalid macro definition: bad fragment specifier 1 */
+"#]],
+    )
+}

crates/hir_def/src/macro_expansion_tests/mbe/tt_conversion.rs

@@ -72,7 +72,7 @@ m2!(x
 macro_rules! m1 { ($x:ident) => { ($x } }
 macro_rules! m2 { ($x:ident) => {} }
 
-/* error: Failed to find macro definition */
+/* error: invalid macro definition: expected subtree */
 /* error: Failed to lower macro args to token tree */
 "#]],
     )

crates/hir_expand/src/db.rs

@@ -8,7 +8,7 @@ use mbe::{syntax_node_to_token_tree, ExpandError, ExpandResult};
 use rustc_hash::FxHashSet;
 use syntax::{
     algo::diff,
-    ast::{self, HasAttrs, HasName},
+    ast::{self, HasAttrs},
     AstNode, GreenNode, Parse, SyntaxNode, SyntaxToken, T,
 };
 
@@ -119,7 +119,7 @@ pub trait AstDatabase: SourceDatabase {
     fn macro_arg_text(&self, id: MacroCallId) -> Option<GreenNode>;
     /// Gets the expander for this macro. This compiles declarative macros, and
     /// just fetches procedural ones.
-    fn macro_def(&self, id: MacroDefId) -> Option<Arc<TokenExpander>>;
+    fn macro_def(&self, id: MacroDefId) -> Result<Arc<TokenExpander>, mbe::ParseError>;
 
     /// Expand macro call to a token tree. This query is LRUed (we keep 128 or so results in memory)
     fn macro_expand(&self, macro_call: MacroCallId) -> ExpandResult<Option<Arc<tt::Subtree>>>;
@@ -145,7 +145,7 @@ pub fn expand_speculative(
     token_to_map: SyntaxToken,
 ) -> Option<(SyntaxNode, SyntaxToken)> {
     let loc = db.lookup_intern_macro(actual_macro_call);
-    let macro_def = db.macro_def(loc.def)?;
+    let macro_def = db.macro_def(loc.def).ok()?;
     let token_range = token_to_map.text_range();
 
     // Build the subtree and token mapping for the speculative args
@@ -360,45 +360,39 @@ fn macro_arg_text(db: &dyn AstDatabase, id: MacroCallId) -> Option<GreenNode> {
     Some(arg.green().into())
 }
 
-fn macro_def(db: &dyn AstDatabase, id: MacroDefId) -> Option<Arc<TokenExpander>> {
+fn macro_def(db: &dyn AstDatabase, id: MacroDefId) -> Result<Arc<TokenExpander>, mbe::ParseError> {
     match id.kind {
         MacroDefKind::Declarative(ast_id) => match ast_id.to_node(db) {
             ast::Macro::MacroRules(macro_rules) => {
-                let arg = macro_rules.token_tree()?;
+                let arg = macro_rules
+                    .token_tree()
+                    .ok_or_else(|| mbe::ParseError::Expected("expected a token tree".into()))?;
                 let (tt, def_site_token_map) = mbe::syntax_node_to_token_tree(arg.syntax());
-                let mac = match mbe::MacroRules::parse(&tt) {
+                let mac = mbe::MacroRules::parse(&tt)?;
-                    Ok(it) => it,
+                Ok(Arc::new(TokenExpander::MacroRules { mac, def_site_token_map }))
-                    Err(err) => {
-                        let name = macro_rules.name().map(|n| n.to_string()).unwrap_or_default();
-                        tracing::warn!("fail on macro_def parse ({}): {:?} {:#?}", name, err, tt);
-                        return None;
-                    }
-                };
-                Some(Arc::new(TokenExpander::MacroRules { mac, def_site_token_map }))
             }
             ast::Macro::MacroDef(macro_def) => {
-                let arg = macro_def.body()?;
+                let arg = macro_def
+                    .body()
+                    .ok_or_else(|| mbe::ParseError::Expected("expected a token tree".into()))?;
                 let (tt, def_site_token_map) = mbe::syntax_node_to_token_tree(arg.syntax());
-                let mac = match mbe::MacroDef::parse(&tt) {
+                let mac = mbe::MacroDef::parse(&tt)?;
-                    Ok(it) => it,
+                Ok(Arc::new(TokenExpander::MacroDef { mac, def_site_token_map }))
-                    Err(err) => {
-                        let name = macro_def.name().map(|n| n.to_string()).unwrap_or_default();
-                        tracing::warn!("fail on macro_def parse ({}): {:?} {:#?}", name, err, tt);
-                        return None;
-                    }
-                };
-                Some(Arc::new(TokenExpander::MacroDef { mac, def_site_token_map }))
             }
         },
-        MacroDefKind::BuiltIn(expander, _) => Some(Arc::new(TokenExpander::Builtin(expander))),
+        MacroDefKind::BuiltIn(expander, _) => Ok(Arc::new(TokenExpander::Builtin(expander))),
         MacroDefKind::BuiltInAttr(expander, _) => {
-            Some(Arc::new(TokenExpander::BuiltinAttr(expander)))
+            Ok(Arc::new(TokenExpander::BuiltinAttr(expander)))
         }
         MacroDefKind::BuiltInDerive(expander, _) => {
-            Some(Arc::new(TokenExpander::BuiltinDerive(expander)))
+            Ok(Arc::new(TokenExpander::BuiltinDerive(expander)))
         }
-        MacroDefKind::BuiltInEager(..) => None,
+        MacroDefKind::BuiltInEager(..) => {
-        MacroDefKind::ProcMacro(expander, ..) => Some(Arc::new(TokenExpander::ProcMacro(expander))),
+            // FIXME: Return a random error here just to make the types align.
+            // This obviously should do something real instead.
+            Err(mbe::ParseError::UnexpectedToken("unexpected eager macro".to_string()))
+        }
+        MacroDefKind::ProcMacro(expander, ..) => Ok(Arc::new(TokenExpander::ProcMacro(expander))),
     }
 }
 
@@ -419,8 +413,11 @@ fn macro_expand(db: &dyn AstDatabase, id: MacroCallId) -> ExpandResult<Option<Ar
     };
 
     let expander = match db.macro_def(loc.def) {
-        Some(it) => it,
+        Ok(it) => it,
-        None => return ExpandResult::str_err("Failed to find macro definition".into()),
+        // FIXME: This is weird -- we effectively report macro *definition*
+        // errors lazily, when we try to expand the macro. Instead, they should
+        // be reported at the definition site (when we construct a def map).
+        Err(err) => return ExpandResult::str_err(format!("invalid macro definition: {}", err)),
     };
     let ExpandResult { value: tt, err } = expander.expand(db, id, &macro_arg.0);
     // Set a hard limit for the expanded tt

crates/hir_expand/src/hygiene.rs

@@ -195,7 +195,7 @@ fn make_hygiene_info(
         _ => None,
     });
 
-    let macro_def = db.macro_def(loc.def)?;
+    let macro_def = db.macro_def(loc.def).ok()?;
     let (_, exp_map) = db.parse_macro_expansion(macro_file).value?;
     let macro_arg = db.macro_arg(macro_file.macro_call_id)?;
 

crates/hir_expand/src/lib.rs

@@ -143,7 +143,7 @@ impl HirFileId {
                     _ => None,
                 });
 
-                let macro_def = db.macro_def(loc.def)?;
+                let macro_def = db.macro_def(loc.def).ok()?;
                 let (parse, exp_map) = db.parse_macro_expansion(macro_file).value?;
                 let macro_arg = db.macro_arg(macro_file.macro_call_id)?;
 

crates/mbe/src/lib.rs

@@ -30,7 +30,7 @@ use crate::{
 pub use ::parser::ParserEntryPoint;
 pub use tt::{Delimiter, DelimiterKind, Punct};
 
-#[derive(Debug, PartialEq, Eq)]
+#[derive(Debug, PartialEq, Eq, Clone)]
 pub enum ParseError {
     UnexpectedToken(String),
     Expected(String),
@@ -38,6 +38,17 @@ pub enum ParseError {
     RepetitionEmptyTokenTree,
 }
 
+impl fmt::Display for ParseError {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        match self {
+            ParseError::UnexpectedToken(it) => f.write_str(it),
+            ParseError::Expected(it) => f.write_str(it),
+            ParseError::InvalidRepeat => f.write_str("invalid repeat"),
+            ParseError::RepetitionEmptyTokenTree => f.write_str("empty token tree in repetition"),
+        }
+    }
+}
+
 #[derive(Debug, PartialEq, Eq, Clone)]
 pub enum ExpandError {
     NoMatchingRule,

crates/mbe/src/subtree_source.rs

@@ -17,17 +17,6 @@ pub(crate) struct SubtreeTokenSource {
     curr: (Token, usize),
 }
 
-impl<'a> SubtreeTokenSource {
-    // Helper function used in test
-    #[cfg(test)]
-    pub(crate) fn text(&self) -> SmolStr {
-        match self.cached.get(self.curr.1) {
-            Some(tt) => tt.text.clone(),
-            _ => SmolStr::new(""),
-        }
-    }
-}
-
 impl<'a> SubtreeTokenSource {
     pub(crate) fn new(buffer: &TokenBuffer) -> SubtreeTokenSource {
         let mut current = buffer.begin();

crates/mbe/src/tests.rs

@@ -1,5 +1,4 @@
 mod expand;
-mod rule;
 
 use std::{fmt::Write, iter};
 

crates/mbe/src/tests/rule.rs

@@ -1,48 +0,0 @@
-use syntax::{ast, AstNode};
-
-use super::*;
-
-#[test]
-fn test_valid_arms() {
-    fn check(macro_body: &str) {
-        let m = parse_macro_arm(macro_body);
-        m.unwrap();
-    }
-
-    check("($i:ident) => ()");
-    check("($(x),*) => ()");
-    check("($(x)_*) => ()");
-    check("($(x)i*) => ()");
-    check("($($i:ident)*) => ($_)");
-    check("($($true:ident)*) => ($true)");
-    check("($($false:ident)*) => ($false)");
-    check("($) => ($)");
-}
-
-#[test]
-fn test_invalid_arms() {
-    fn check(macro_body: &str, err: ParseError) {
-        let m = parse_macro_arm(macro_body);
-        assert_eq!(m, Err(err));
-    }
-    check("invalid", ParseError::Expected("expected subtree".into()));
-
-    check("$i:ident => ()", ParseError::Expected("expected subtree".into()));
-    check("($i:ident) ()", ParseError::Expected("expected `=`".into()));
-    check("($($i:ident)_) => ()", ParseError::InvalidRepeat);
-
-    check("($i) => ($i)", ParseError::UnexpectedToken("bad fragment specifier 1".into()));
-    check("($i:) => ($i)", ParseError::UnexpectedToken("bad fragment specifier 1".into()));
-    check("($i:_) => ()", ParseError::UnexpectedToken("bad fragment specifier 1".into()));
-}
-
-fn parse_macro_arm(arm_definition: &str) -> Result<crate::MacroRules, ParseError> {
-    let macro_definition = format!(" macro_rules! m {{ {} }} ", arm_definition);
-    let source_file = ast::SourceFile::parse(&macro_definition).ok().unwrap();
-    let macro_definition =
-        source_file.syntax().descendants().find_map(ast::MacroRules::cast).unwrap();
-
-    let (definition_tt, _) =
-        syntax_node_to_token_tree(macro_definition.token_tree().unwrap().syntax());
-    crate::MacroRules::parse(&definition_tt)
-}