Auto merge of #16540 - Veykril:macro-arg, r=Veykril

internal: macro_arg query always returns a TokenTree

12 files changed, 139 insertions, 131 deletions

diff --git a/crates/hir-def/src/body/lower.rs b/crates/hir-def/src/body/lower.rs
index 29ac666277d..8a589a6cf1b 100644
--- a/crates/hir-def/src/body/lower.rs
+++ b/crates/hir-def/src/body/lower.rs
@@ -1000,10 +1000,6 @@ impl ExprCollector<'_> {
                         krate: *krate,
                     });
                 }
-                Some(ExpandError::RecursionOverflowPoisoned) => {
-                    // Recursion limit has been reached in the macro expansion tree, but not in
-                    // this very macro call. Don't add diagnostics to avoid duplication.
-                }
                 Some(err) => {
                     self.source_map.diagnostics.push(BodyDiagnostic::MacroError {
                         node: InFile::new(outer_file, syntax_ptr),
diff --git a/crates/hir-def/src/expander.rs b/crates/hir-def/src/expander.rs
index b83feeedc34..b99df1ed593 100644
--- a/crates/hir-def/src/expander.rs
+++ b/crates/hir-def/src/expander.rs
@@ -140,13 +140,11 @@ impl Expander {
             // The overflow error should have been reported when it occurred (see the next branch),
             // so don't return overflow error here to avoid diagnostics duplication.
             cov_mark::hit!(overflow_but_not_me);
-            return ExpandResult::only_err(ExpandError::RecursionOverflowPoisoned);
+            return ExpandResult::ok(None);
         } else if self.recursion_limit.check(self.recursion_depth as usize + 1).is_err() {
             self.recursion_depth = u32::MAX;
             cov_mark::hit!(your_stack_belongs_to_me);
-            return ExpandResult::only_err(ExpandError::other(
-                "reached recursion limit during macro expansion",
-            ));
+            return ExpandResult::only_err(ExpandError::RecursionOverflow);
         }
 
         let ExpandResult { value, err } = op(self);
diff --git a/crates/hir-def/src/macro_expansion_tests/mbe/matching.rs b/crates/hir-def/src/macro_expansion_tests/mbe/matching.rs
index 0909d8c8354..63f211022c9 100644
--- a/crates/hir-def/src/macro_expansion_tests/mbe/matching.rs
+++ b/crates/hir-def/src/macro_expansion_tests/mbe/matching.rs
@@ -33,7 +33,7 @@ m!(&k");
 "#,
         expect![[r#"
 macro_rules! m { ($i:literal) => {}; }
-/* error: invalid token tree */"#]],
+/* error: mismatched delimiters */"#]],
     );
 }
 
diff --git a/crates/hir-def/src/macro_expansion_tests/mbe/meta_syntax.rs b/crates/hir-def/src/macro_expansion_tests/mbe/meta_syntax.rs
index e875950e4e5..2d289b76833 100644
--- a/crates/hir-def/src/macro_expansion_tests/mbe/meta_syntax.rs
+++ b/crates/hir-def/src/macro_expansion_tests/mbe/meta_syntax.rs
@@ -68,26 +68,26 @@ m2!();
 "#,
         expect![[r#"
 macro_rules! i1 { invalid }
-/* error: invalid macro definition: expected subtree */
+/* error: macro definition has parse errors */
 
 macro_rules! e1 { $i:ident => () }
-/* error: invalid macro definition: expected subtree */
+/* error: macro definition has parse errors */
 macro_rules! e2 { ($i:ident) () }
-/* error: invalid macro definition: expected `=` */
+/* error: macro definition has parse errors */
 macro_rules! e3 { ($(i:ident)_) => () }
-/* error: invalid macro definition: invalid repeat */
+/* error: macro definition has parse errors */
 
 macro_rules! f1 { ($i) => ($i) }
-/* error: invalid macro definition: missing fragment specifier */
+/* error: macro definition has parse errors */
 macro_rules! f2 { ($i:) => ($i) }
-/* error: invalid macro definition: missing fragment specifier */
+/* error: macro definition has parse errors */
 macro_rules! f3 { ($i:_) => () }
-/* error: invalid macro definition: missing fragment specifier */
+/* error: macro definition has parse errors */
 
 macro_rules! m1 { ($$i) => () }
-/* error: invalid macro definition: `$$` is not allowed on the pattern side */
+/* error: macro definition has parse errors */
 macro_rules! m2 { () => ( ${invalid()} ) }
-/* error: invalid macro definition: invalid metavariable expression */
+/* error: macro definition has parse errors */
 "#]],
     )
 }
@@ -137,18 +137,18 @@ macro_rules! m9 { ($($($($i:ident)?)*)+) => {}; }
 macro_rules! mA { ($($($($i:ident)+)?)*) => {}; }
 macro_rules! mB { ($($($($i:ident)+)*)?) => {}; }
 
-/* error: invalid macro definition: empty token tree in repetition */
-/* error: invalid macro definition: empty token tree in repetition */
-/* error: invalid macro definition: empty token tree in repetition */
-/* error: invalid macro definition: empty token tree in repetition */
-/* error: invalid macro definition: empty token tree in repetition */
-/* error: invalid macro definition: empty token tree in repetition */
-/* error: invalid macro definition: empty token tree in repetition */
-/* error: invalid macro definition: empty token tree in repetition */
-/* error: invalid macro definition: empty token tree in repetition */
-/* error: invalid macro definition: empty token tree in repetition */
-/* error: invalid macro definition: empty token tree in repetition */
-/* error: invalid macro definition: empty token tree in repetition */
+/* error: macro definition has parse errors */
+/* error: macro definition has parse errors */
+/* error: macro definition has parse errors */
+/* error: macro definition has parse errors */
+/* error: macro definition has parse errors */
+/* error: macro definition has parse errors */
+/* error: macro definition has parse errors */
+/* error: macro definition has parse errors */
+/* error: macro definition has parse errors */
+/* error: macro definition has parse errors */
+/* error: macro definition has parse errors */
+/* error: macro definition has parse errors */
     "#]],
     );
 }
diff --git a/crates/hir-def/src/macro_expansion_tests/mbe/metavar_expr.rs b/crates/hir-def/src/macro_expansion_tests/mbe/metavar_expr.rs
index 6560d0ec466..bf701198387 100644
--- a/crates/hir-def/src/macro_expansion_tests/mbe/metavar_expr.rs
+++ b/crates/hir-def/src/macro_expansion_tests/mbe/metavar_expr.rs
@@ -275,9 +275,9 @@ macro_rules! depth_too_large {
 }
 
 fn test() {
-    /* error: invalid macro definition: invalid metavariable expression */;
-    /* error: invalid macro definition: invalid metavariable expression */;
-    /* error: invalid macro definition: invalid metavariable expression */;
+    /* error: macro definition has parse errors */;
+    /* error: macro definition has parse errors */;
+    /* error: macro definition has parse errors */;
 }
 "#]],
     );
diff --git a/crates/hir-def/src/macro_expansion_tests/mbe/tt_conversion.rs b/crates/hir-def/src/macro_expansion_tests/mbe/tt_conversion.rs
index ae56934f632..362c189f6a7 100644
--- a/crates/hir-def/src/macro_expansion_tests/mbe/tt_conversion.rs
+++ b/crates/hir-def/src/macro_expansion_tests/mbe/tt_conversion.rs
@@ -97,8 +97,8 @@ m2!(x
 macro_rules! m1 { ($x:ident) => { ($x } }
 macro_rules! m2 { ($x:ident) => {} }
 
-/* error: invalid macro definition: expected subtree */
-/* error: invalid token tree */
+/* error: macro definition has parse errors */
+/* error: mismatched delimiters */
 "#]],
     )
 }
diff --git a/crates/hir-expand/src/builtin_fn_macro.rs b/crates/hir-expand/src/builtin_fn_macro.rs
index 6d3de0e55d2..90cd3af7578 100644
--- a/crates/hir-expand/src/builtin_fn_macro.rs
+++ b/crates/hir-expand/src/builtin_fn_macro.rs
@@ -446,7 +446,7 @@ fn compile_error_expand(
 ) -> ExpandResult<tt::Subtree> {
     let err = match &*tt.token_trees {
         [tt::TokenTree::Leaf(tt::Leaf::Literal(it))] => match unquote_str(it) {
-            Some(unquoted) => ExpandError::other(unquoted),
+            Some(unquoted) => ExpandError::other(unquoted.into_boxed_str()),
             None => ExpandError::other("`compile_error!` argument must be a string"),
         },
         _ => ExpandError::other("`compile_error!` argument must be a string"),
diff --git a/crates/hir-expand/src/db.rs b/crates/hir-expand/src/db.rs
index 6a288cf9197..7b62eaa0289 100644
--- a/crates/hir-expand/src/db.rs
+++ b/crates/hir-expand/src/db.rs
@@ -108,7 +108,7 @@ pub trait ExpandDatabase: SourceDatabase {
     fn macro_arg(
         &self,
         id: MacroCallId,
-    ) -> ValueResult<Option<(Arc<tt::Subtree>, SyntaxFixupUndoInfo)>, Arc<Box<[SyntaxError]>>>;
+    ) -> ValueResult<(Arc<tt::Subtree>, SyntaxFixupUndoInfo), Arc<Box<[SyntaxError]>>>;
     /// Fetches the expander for this macro.
     #[salsa::transparent]
     #[salsa::invoke(TokenExpander::macro_expander)]
@@ -326,58 +326,77 @@ fn macro_arg(
     db: &dyn ExpandDatabase,
     id: MacroCallId,
     // FIXME: consider the following by putting fixup info into eager call info args
-    // ) -> ValueResult<Option<Arc<(tt::Subtree, SyntaxFixupUndoInfo)>>, Arc<Box<[SyntaxError]>>> {
-) -> ValueResult<Option<(Arc<tt::Subtree>, SyntaxFixupUndoInfo)>, Arc<Box<[SyntaxError]>>> {
-    let mismatched_delimiters = |arg: &SyntaxNode| {
-        let first = arg.first_child_or_token().map_or(T![.], |it| it.kind());
-        let last = arg.last_child_or_token().map_or(T![.], |it| it.kind());
-        let well_formed_tt =
-            matches!((first, last), (T!['('], T![')']) | (T!['['], T![']']) | (T!['{'], T!['}']));
-        if !well_formed_tt {
-            // Don't expand malformed (unbalanced) macro invocations. This is
-            // less than ideal, but trying to expand unbalanced  macro calls
-            // sometimes produces pathological, deeply nested code which breaks
-            // all kinds of things.
-            //
-            // Some day, we'll have explicit recursion counters for all
-            // recursive things, at which point this code might be removed.
-            cov_mark::hit!(issue9358_bad_macro_stack_overflow);
-            Some(Arc::new(Box::new([SyntaxError::new(
-                "unbalanced token tree".to_owned(),
-                arg.text_range(),
-            )]) as Box<[_]>))
-        } else {
-            None
-        }
-    };
+    // ) -> ValueResult<Arc<(tt::Subtree, SyntaxFixupUndoInfo)>, Arc<Box<[SyntaxError]>>> {
+) -> ValueResult<(Arc<tt::Subtree>, SyntaxFixupUndoInfo), Arc<Box<[SyntaxError]>>> {
     let loc = db.lookup_intern_macro_call(id);
     if let Some(EagerCallInfo { arg, .. }) = matches!(loc.def.kind, MacroDefKind::BuiltInEager(..))
         .then(|| loc.eager.as_deref())
         .flatten()
     {
-        ValueResult::ok(Some((arg.clone(), SyntaxFixupUndoInfo::NONE)))
+        ValueResult::ok((arg.clone(), SyntaxFixupUndoInfo::NONE))
     } else {
         let (parse, map) = parse_with_map(db, loc.kind.file_id());
         let root = parse.syntax_node();
 
         let syntax = match loc.kind {
             MacroCallKind::FnLike { ast_id, .. } => {
+                let dummy_tt = |kind| {
+                    (
+                        Arc::new(tt::Subtree {
+                            delimiter: tt::Delimiter {
+                                open: loc.call_site,
+                                close: loc.call_site,
+                                kind,
+                            },
+                            token_trees: Box::default(),
+                        }),
+                        SyntaxFixupUndoInfo::default(),
+                    )
+                };
+
                 let node = &ast_id.to_ptr(db).to_node(&root);
                 let offset = node.syntax().text_range().start();
-                match node.token_tree() {
-                    Some(tt) => {
-                        let tt = tt.syntax();
-                        if let Some(e) = mismatched_delimiters(tt) {
-                            return ValueResult::only_err(e);
-                        }
-                        tt.clone()
-                    }
-                    None => {
-                        return ValueResult::only_err(Arc::new(Box::new([
-                            SyntaxError::new_at_offset("missing token tree".to_owned(), offset),
-                        ])));
-                    }
+                let Some(tt) = node.token_tree() else {
+                    return ValueResult::new(
+                        dummy_tt(tt::DelimiterKind::Invisible),
+                        Arc::new(Box::new([SyntaxError::new_at_offset(
+                            "missing token tree".to_owned(),
+                            offset,
+                        )])),
+                    );
+                };
+                let first = tt.left_delimiter_token().map(|it| it.kind()).unwrap_or(T!['(']);
+                let last = tt.right_delimiter_token().map(|it| it.kind()).unwrap_or(T![.]);
+
+                let mismatched_delimiters = !matches!(
+                    (first, last),
+                    (T!['('], T![')']) | (T!['['], T![']']) | (T!['{'], T!['}'])
+                );
+                if mismatched_delimiters {
+                    // Don't expand malformed (unbalanced) macro invocations. This is
+                    // less than ideal, but trying to expand unbalanced  macro calls
+                    // sometimes produces pathological, deeply nested code which breaks
+                    // all kinds of things.
+                    //
+                    // So instead, we'll return an empty subtree here
+                    cov_mark::hit!(issue9358_bad_macro_stack_overflow);
+
+                    let kind = match first {
+                        _ if loc.def.is_proc_macro() => tt::DelimiterKind::Invisible,
+                        T!['('] => tt::DelimiterKind::Parenthesis,
+                        T!['['] => tt::DelimiterKind::Bracket,
+                        T!['{'] => tt::DelimiterKind::Brace,
+                        _ => tt::DelimiterKind::Invisible,
+                    };
+                    return ValueResult::new(
+                        dummy_tt(kind),
+                        Arc::new(Box::new([SyntaxError::new_at_offset(
+                            "mismatched delimiters".to_owned(),
+                            offset,
+                        )])),
+                    );
                 }
+                tt.syntax().clone()
             }
             MacroCallKind::Derive { ast_id, .. } => {
                 ast_id.to_ptr(db).to_node(&root).syntax().clone()
@@ -427,15 +446,15 @@ fn macro_arg(
 
         if matches!(loc.def.kind, MacroDefKind::BuiltInEager(..)) {
             match parse.errors() {
-                [] => ValueResult::ok(Some((Arc::new(tt), undo_info))),
+                [] => ValueResult::ok((Arc::new(tt), undo_info)),
                 errors => ValueResult::new(
-                    Some((Arc::new(tt), undo_info)),
+                    (Arc::new(tt), undo_info),
                     // Box::<[_]>::from(res.errors()), not stable yet
                     Arc::new(errors.to_vec().into_boxed_slice()),
                 ),
             }
         } else {
-            ValueResult::ok(Some((Arc::new(tt), undo_info)))
+            ValueResult::ok((Arc::new(tt), undo_info))
         }
     }
 }
@@ -519,21 +538,20 @@ fn macro_expand(
             expander.expand(db, macro_call_id, &node, map.as_ref())
         }
         _ => {
-            let ValueResult { value, err } = db.macro_arg(macro_call_id);
-            let Some((macro_arg, undo_info)) = value else {
-                return ExpandResult {
-                    value: CowArc::Owned(tt::Subtree {
-                        delimiter: tt::Delimiter::invisible_spanned(loc.call_site),
-                        token_trees: Box::new([]),
-                    }),
-                    // FIXME: We should make sure to enforce an invariant that invalid macro
-                    // calls do not reach this call path!
-                    err: Some(ExpandError::other("invalid token tree")),
-                };
+            let ValueResult { value: (macro_arg, undo_info), err } = db.macro_arg(macro_call_id);
+            let format_parse_err = |err: Arc<Box<[SyntaxError]>>| {
+                let mut buf = String::new();
+                for err in &**err {
+                    use std::fmt::Write;
+                    _ = write!(buf, "{}, ", err);
+                }
+                buf.pop();
+                buf.pop();
+                ExpandError::other(buf)
             };
 
             let arg = &*macro_arg;
-            match loc.def.kind {
+            let res = match loc.def.kind {
                 MacroDefKind::Declarative(id) => {
                     db.decl_macro_expander(loc.def.krate, id).expand(db, arg.clone(), macro_call_id)
                 }
@@ -549,16 +567,7 @@ fn macro_expand(
                 MacroDefKind::BuiltInEager(..) if loc.eager.is_none() => {
                     return ExpandResult {
                         value: CowArc::Arc(macro_arg.clone()),
-                        err: err.map(|err| {
-                            let mut buf = String::new();
-                            for err in &**err {
-                                use std::fmt::Write;
-                                _ = write!(buf, "{}, ", err);
-                            }
-                            buf.pop();
-                            buf.pop();
-                            ExpandError::other(buf)
-                        }),
+                        err: err.map(format_parse_err),
                     };
                 }
                 MacroDefKind::BuiltInEager(it, _) => {
@@ -570,6 +579,11 @@ fn macro_expand(
                     res
                 }
                 _ => unreachable!(),
+            };
+            ExpandResult {
+                value: res.value,
+                // if the arg had parse errors, show them instead of the expansion errors
+                err: err.map(format_parse_err).or(res.err),
             }
         }
     };
@@ -597,17 +611,7 @@ fn macro_expand(
 
 fn expand_proc_macro(db: &dyn ExpandDatabase, id: MacroCallId) -> ExpandResult<Arc<tt::Subtree>> {
     let loc = db.lookup_intern_macro_call(id);
-    let Some((macro_arg, undo_info)) = db.macro_arg(id).value else {
-        return ExpandResult {
-            value: Arc::new(tt::Subtree {
-                delimiter: tt::Delimiter::invisible_spanned(loc.call_site),
-                token_trees: Box::new([]),
-            }),
-            // FIXME: We should make sure to enforce an invariant that invalid macro
-            // calls do not reach this call path!
-            err: Some(ExpandError::other("invalid token tree")),
-        };
-    };
+    let (macro_arg, undo_info) = db.macro_arg(id).value;
 
     let expander = match loc.def.kind {
         MacroDefKind::ProcMacro(expander, ..) => expander,
diff --git a/crates/hir-expand/src/declarative.rs b/crates/hir-expand/src/declarative.rs
index 37084ee8b93..345e5cee266 100644
--- a/crates/hir-expand/src/declarative.rs
+++ b/crates/hir-expand/src/declarative.rs
@@ -44,9 +44,9 @@ impl DeclarativeMacroExpander {
             )
         });
         match self.mac.err() {
-            Some(e) => ExpandResult::new(
+            Some(_) => ExpandResult::new(
                 tt::Subtree::empty(tt::DelimSpan { open: loc.call_site, close: loc.call_site }),
-                ExpandError::other(format!("invalid macro definition: {e}")),
+                ExpandError::MacroDefinition,
             ),
             None => self
                 .mac
@@ -80,9 +80,9 @@ impl DeclarativeMacroExpander {
             )
         });
         match self.mac.err() {
-            Some(e) => ExpandResult::new(
+            Some(_) => ExpandResult::new(
                 tt::Subtree::empty(tt::DelimSpan { open: call_site, close: call_site }),
-                ExpandError::other(format!("invalid macro definition: {e}")),
+                ExpandError::MacroDefinition,
             ),
             None => self.mac.expand(&tt, |_| (), new_meta_vars, call_site).map_err(Into::into),
         }
diff --git a/crates/hir-expand/src/lib.rs b/crates/hir-expand/src/lib.rs
index 42a8864c6a7..3a7febc2eb8 100644
--- a/crates/hir-expand/src/lib.rs
+++ b/crates/hir-expand/src/lib.rs
@@ -44,7 +44,6 @@ use crate::{
     builtin_derive_macro::BuiltinDeriveExpander,
     builtin_fn_macro::{BuiltinFnLikeExpander, EagerExpander},
     db::{ExpandDatabase, TokenExpander},
-    fixup::SyntaxFixupUndoInfo,
     hygiene::SyntaxContextData,
     mod_path::ModPath,
     proc_macro::{CustomProcMacroExpander, ProcMacroKind},
@@ -131,8 +130,9 @@ pub enum ExpandError {
     UnresolvedProcMacro(CrateId),
     /// The macro expansion is disabled.
     MacroDisabled,
+    MacroDefinition,
     Mbe(mbe::ExpandError),
-    RecursionOverflowPoisoned,
+    RecursionOverflow,
     Other(Box<Box<str>>),
     ProcMacroPanic(Box<Box<str>>),
 }
@@ -154,15 +154,14 @@ impl fmt::Display for ExpandError {
         match self {
             ExpandError::UnresolvedProcMacro(_) => f.write_str("unresolved proc-macro"),
             ExpandError::Mbe(it) => it.fmt(f),
-            ExpandError::RecursionOverflowPoisoned => {
-                f.write_str("overflow expanding the original macro")
-            }
+            ExpandError::RecursionOverflow => f.write_str("overflow expanding the original macro"),
             ExpandError::ProcMacroPanic(it) => {
                 f.write_str("proc-macro panicked: ")?;
                 f.write_str(it)
             }
             ExpandError::Other(it) => f.write_str(it),
             ExpandError::MacroDisabled => f.write_str("macro disabled"),
+            ExpandError::MacroDefinition => f.write_str("macro definition has parse errors"),
         }
     }
 }
@@ -761,15 +760,7 @@ impl ExpansionInfo {
         let (parse, exp_map) = db.parse_macro_expansion(macro_file).value;
         let expanded = InMacroFile { file_id: macro_file, value: parse.syntax_node() };
 
-        let (macro_arg, _) = db.macro_arg(macro_file.macro_call_id).value.unwrap_or_else(|| {
-            (
-                Arc::new(tt::Subtree {
-                    delimiter: tt::Delimiter::invisible_spanned(loc.call_site),
-                    token_trees: Box::new([]),
-                }),
-                SyntaxFixupUndoInfo::NONE,
-            )
-        });
+        let (macro_arg, _) = db.macro_arg(macro_file.macro_call_id).value;
 
         let def = loc.def.ast_id().left().and_then(|id| {
             let def_tt = match id.to_node(db) {
diff --git a/crates/ide-completion/src/tests/expression.rs b/crates/ide-completion/src/tests/expression.rs
index 556d3872d7e..7749fac40b9 100644
--- a/crates/ide-completion/src/tests/expression.rs
+++ b/crates/ide-completion/src/tests/expression.rs
@@ -549,7 +549,26 @@ fn quux(x: i32) {
     m!(x$0
 }
 "#,
-        expect![[r#""#]],
+        expect![[r#"
+            fn quux(…)   fn(i32)
+            lc x         i32
+            lc y         i32
+            ma m!(…)     macro_rules! m
+            bt u32       u32
+            kw crate::
+            kw false
+            kw for
+            kw if
+            kw if let
+            kw loop
+            kw match
+            kw return
+            kw self::
+            kw true
+            kw unsafe
+            kw while
+            kw while let
+        "#]],
     );
 }
 
diff --git a/crates/ide-diagnostics/src/handlers/macro_error.rs b/crates/ide-diagnostics/src/handlers/macro_error.rs
index e4cb53f3a2f..6a957ac1c97 100644
--- a/crates/ide-diagnostics/src/handlers/macro_error.rs
+++ b/crates/ide-diagnostics/src/handlers/macro_error.rs
@@ -242,7 +242,7 @@ macro_rules! foo {
 
 fn f() {
     foo!();
-  //^^^ error: invalid macro definition: expected subtree
+  //^^^ error: macro definition has parse errors
 
 }
 "#,