Merge #8048

8048: Fix missing unresolved macro diagnostic in function body r=edwin0cheng a=brandondong

This was an issue I found while working on https://github.com/rust-analyzer/rust-analyzer/pull/7970.

**Reproduction:**
1. Call a non-existent macro in a function body.
```
fn main() {
  foo!();
}
```
2. No diagnostics are raised. An unresolved-macro-call diagnostic is expected.
3. If the macro call is instead outside of the function body, this works as expected.

I believe this worked previously and regressed in https://github.com/rust-analyzer/rust-analyzer/pull/7805.

**Behavior prior to https://github.com/rust-analyzer/rust-analyzer/pull/7805:**
- The unresolved-macro-call diagnostic did not exist. Instead, a macro-error diagnostic would be raised with the text "could not resolve macro [path]".
- This was implemented by adding an error to the error sink (https://github.com/rust-analyzer/rust-analyzer/pull/7805/files#diff-50a326c5ae465bd9b31ee4310186380aa06e4fa1f6b41dbc0aed5bcc656a3cb8L657).
- The error was propagated through https://github.com/rust-analyzer/rust-analyzer/blob/1a82af3527e476d52410ff4dfd2fb4c57466abcb/crates/hir_def/src/body.rs#L123 eventually reaching https://github.com/rust-analyzer/rust-analyzer/blob/1a82af3527e476d52410ff4dfd2fb4c57466abcb/crates/hir_def/src/body/lower.rs#L569.

**Behavior after:**
- Instead of writing to the error sink, an UnresolvedMacro error is now returned (https://github.com/rust-analyzer/rust-analyzer/pull/7805/files#diff-50a326c5ae465bd9b31ee4310186380aa06e4fa1f6b41dbc0aed5bcc656a3cb8R631).
- The parent caller throws away the error as its function signature is `Option<MacroCallId>` (https://github.com/rust-analyzer/rust-analyzer/pull/7805/files#diff-50a326c5ae465bd9b31ee4310186380aa06e4fa1f6b41dbc0aed5bcc656a3cb8R604).
- We instead now reach the warn condition (https://github.com/rust-analyzer/rust-analyzer/blob/1a82af3527e476d52410ff4dfd2fb4c57466abcb/crates/hir_def/src/body.rs#L124) and no diagnostics are created in https://github.com/rust-analyzer/rust-analyzer/blob/1a82af3527e476d52410ff4dfd2fb4c57466abcb/crates/hir_def/src/body/lower.rs#L575.

**Fix:**
- Make sure to propagate the UnresolvedMacro error. Report the error using the new unresolved-macro-call diagnostic.


Co-authored-by: Brandon <[email protected]>

8 files changed, 80 insertions, 47 deletions

diff --git a/crates/hir_def/src/body.rs b/crates/hir_def/src/body.rs
index 8bcc350ce..1080d9c2c 100644
--- a/crates/hir_def/src/body.rs
+++ b/crates/hir_def/src/body.rs
@@ -32,6 +32,7 @@ use crate::{
     path::{ModPath, Path},
     src::HasSource,
     AsMacroCall, BlockId, DefWithBodyId, HasModule, LocalModuleId, Lookup, ModuleId,
+    UnresolvedMacro,
 };
 
 /// A subset of Expander that only deals with cfg attributes. We only need it to
@@ -101,10 +102,12 @@ impl Expander {
         &mut self,
         db: &dyn DefDatabase,
         macro_call: ast::MacroCall,
-    ) -> ExpandResult<Option<(Mark, T)>> {
+    ) -> Result<ExpandResult<Option<(Mark, T)>>, UnresolvedMacro> {
         if self.recursion_limit + 1 > EXPANSION_RECURSION_LIMIT {
             cov_mark::hit!(your_stack_belongs_to_me);
-            return ExpandResult::str_err("reached recursion limit during macro expansion".into());
+            return Ok(ExpandResult::str_err(
+                "reached recursion limit during macro expansion".into(),
+            ));
         }
 
         let macro_call = InFile::new(self.current_file_id, &macro_call);
@@ -116,14 +119,11 @@ impl Expander {
         let call_id =
             macro_call.as_call_id_with_errors(db, self.def_map.krate(), resolver, &mut |e| {
                 err.get_or_insert(e);
-            });
+            })?;
         let call_id = match call_id {
-            Some(it) => it,
-            None => {
-                if err.is_none() {
-                    log::warn!("no error despite `as_call_id_with_errors` returning `None`");
-                }
-                return ExpandResult { value: None, err };
+            Ok(it) => it,
+            Err(_) => {
+                return Ok(ExpandResult { value: None, err });
             }
         };
 
@@ -141,9 +141,9 @@ impl Expander {
                     log::warn!("no error despite `parse_or_expand` failing");
                 }
 
-                return ExpandResult::only_err(err.unwrap_or_else(|| {
+                return Ok(ExpandResult::only_err(err.unwrap_or_else(|| {
                     mbe::ExpandError::Other("failed to parse macro invocation".into())
-                }));
+                })));
             }
         };
 
@@ -151,7 +151,7 @@ impl Expander {
             Some(it) => it,
             None => {
                 // This can happen without being an error, so only forward previous errors.
-                return ExpandResult { value: None, err };
+                return Ok(ExpandResult { value: None, err });
             }
         };
 
@@ -167,7 +167,7 @@ impl Expander {
         self.current_file_id = file_id;
         self.ast_id_map = db.ast_id_map(file_id);
 
-        ExpandResult { value: Some((mark, node)), err }
+        Ok(ExpandResult { value: Some((mark, node)), err })
     }
 
     pub(crate) fn exit(&mut self, db: &dyn DefDatabase, mut mark: Mark) {
diff --git a/crates/hir_def/src/body/diagnostics.rs b/crates/hir_def/src/body/diagnostics.rs
index 1de7d30e2..f6992c9a8 100644
--- a/crates/hir_def/src/body/diagnostics.rs
+++ b/crates/hir_def/src/body/diagnostics.rs
@@ -2,13 +2,14 @@
 
 use hir_expand::diagnostics::DiagnosticSink;
 
-use crate::diagnostics::{InactiveCode, MacroError, UnresolvedProcMacro};
+use crate::diagnostics::{InactiveCode, MacroError, UnresolvedMacroCall, UnresolvedProcMacro};
 
 #[derive(Debug, Eq, PartialEq)]
 pub(crate) enum BodyDiagnostic {
     InactiveCode(InactiveCode),
     MacroError(MacroError),
     UnresolvedProcMacro(UnresolvedProcMacro),
+    UnresolvedMacroCall(UnresolvedMacroCall),
 }
 
 impl BodyDiagnostic {
@@ -23,6 +24,9 @@ impl BodyDiagnostic {
             BodyDiagnostic::UnresolvedProcMacro(diag) => {
                 sink.push(diag.clone());
             }
+            BodyDiagnostic::UnresolvedMacroCall(diag) => {
+                sink.push(diag.clone());
+            }
         }
     }
 }
diff --git a/crates/hir_def/src/body/lower.rs b/crates/hir_def/src/body/lower.rs
index 7052058f2..60b25db56 100644
--- a/crates/hir_def/src/body/lower.rs
+++ b/crates/hir_def/src/body/lower.rs
@@ -24,7 +24,7 @@ use crate::{
     body::{Body, BodySourceMap, Expander, LabelSource, PatPtr, SyntheticSyntax},
     builtin_type::{BuiltinFloat, BuiltinInt, BuiltinUint},
     db::DefDatabase,
-    diagnostics::{InactiveCode, MacroError, UnresolvedProcMacro},
+    diagnostics::{InactiveCode, MacroError, UnresolvedMacroCall, UnresolvedProcMacro},
     expr::{
         dummy_expr_id, ArithOp, Array, BinaryOp, BindingAnnotation, CmpOp, Expr, ExprId, Label,
         LabelId, Literal, LogicOp, MatchArm, Ordering, Pat, PatId, RecordFieldPat, RecordLitField,
@@ -33,7 +33,7 @@ use crate::{
     item_scope::BuiltinShadowMode,
     path::{GenericArgs, Path},
     type_ref::{Mutability, Rawness, TypeRef},
-    AdtId, BlockLoc, ModuleDefId,
+    AdtId, BlockLoc, ModuleDefId, UnresolvedMacro,
 };
 
 use super::{diagnostics::BodyDiagnostic, ExprSource, PatSource};
@@ -554,6 +554,17 @@ impl ExprCollector<'_> {
         let macro_call = self.expander.to_source(AstPtr::new(&e));
         let res = self.expander.enter_expand(self.db, e);
 
+        let res = match res {
+            Ok(res) => res,
+            Err(UnresolvedMacro) => {
+                self.source_map.diagnostics.push(BodyDiagnostic::UnresolvedMacroCall(
+                    UnresolvedMacroCall { file: outer_file, node: syntax_ptr.cast().unwrap() },
+                ));
+                collector(self, None);
+                return;
+            }
+        };
+
         match &res.err {
             Some(ExpandError::UnresolvedProcMacro) => {
                 self.source_map.diagnostics.push(BodyDiagnostic::UnresolvedProcMacro(
diff --git a/crates/hir_def/src/body/tests.rs b/crates/hir_def/src/body/tests.rs
index 991a32b15..f8e6f70e8 100644
--- a/crates/hir_def/src/body/tests.rs
+++ b/crates/hir_def/src/body/tests.rs
@@ -175,6 +175,18 @@ fn f() {
 }
 
 #[test]
+fn unresolved_macro_diag() {
+    check_diagnostics(
+        r#"
+fn f() {
+    m!();
+  //^^^^ unresolved macro call
+}
+      "#,
+    );
+}
+
+#[test]
 fn dollar_crate_in_builtin_macro() {
     check_diagnostics(
         r#"
diff --git a/crates/hir_def/src/data.rs b/crates/hir_def/src/data.rs
index 74a2194e5..1a27f7bf2 100644
--- a/crates/hir_def/src/data.rs
+++ b/crates/hir_def/src/data.rs
@@ -267,23 +267,26 @@ fn collect_items(
                 let ast_id_map = db.ast_id_map(file_id);
                 let root = db.parse_or_expand(file_id).unwrap();
                 let call = ast_id_map.get(call.ast_id).to_node(&root);
-
-                if let Some((mark, mac)) = expander.enter_expand(db, call).value {
-                    let src: InFile<ast::MacroItems> = expander.to_source(mac);
-                    let item_tree = db.item_tree(src.file_id);
-                    let iter =
-                        item_tree.top_level_items().iter().filter_map(ModItem::as_assoc_item);
-                    items.extend(collect_items(
-                        db,
-                        module,
-                        expander,
-                        iter,
-                        src.file_id,
-                        container,
-                        limit - 1,
-                    ));
-
-                    expander.exit(db, mark);
+                let res = expander.enter_expand(db, call);
+
+                if let Ok(res) = res {
+                    if let Some((mark, mac)) = res.value {
+                        let src: InFile<ast::MacroItems> = expander.to_source(mac);
+                        let item_tree = db.item_tree(src.file_id);
+                        let iter =
+                            item_tree.top_level_items().iter().filter_map(ModItem::as_assoc_item);
+                        items.extend(collect_items(
+                            db,
+                            module,
+                            expander,
+                            iter,
+                            src.file_id,
+                            container,
+                            limit - 1,
+                        ));
+
+                        expander.exit(db, mark);
+                    }
                 }
             }
         }
diff --git a/crates/hir_def/src/diagnostics.rs b/crates/hir_def/src/diagnostics.rs
index 7188bb299..97abf8653 100644
--- a/crates/hir_def/src/diagnostics.rs
+++ b/crates/hir_def/src/diagnostics.rs
@@ -99,7 +99,7 @@ impl Diagnostic for UnresolvedImport {
 //
 // This diagnostic is triggered if rust-analyzer is unable to resolve the path to a
 // macro in a macro invocation.
-#[derive(Debug)]
+#[derive(Debug, Clone, Eq, PartialEq)]
 pub struct UnresolvedMacroCall {
     pub file: HirFileId,
     pub node: AstPtr<ast::MacroCall>,
diff --git a/crates/hir_def/src/lib.rs b/crates/hir_def/src/lib.rs
index c6655c5fb..6758411a0 100644
--- a/crates/hir_def/src/lib.rs
+++ b/crates/hir_def/src/lib.rs
@@ -58,7 +58,7 @@ use std::{
 use base_db::{impl_intern_key, salsa, CrateId};
 use hir_expand::{
     ast_id_map::FileAstId,
-    eager::{expand_eager_macro, ErrorEmitted},
+    eager::{expand_eager_macro, ErrorEmitted, ErrorSink},
     hygiene::Hygiene,
     AstId, HirFileId, InFile, MacroCallId, MacroCallKind, MacroDefId, MacroDefKind,
 };
@@ -583,7 +583,7 @@ pub trait AsMacroCall {
         krate: CrateId,
         resolver: impl Fn(path::ModPath) -> Option<MacroDefId>,
     ) -> Option<MacroCallId> {
-        self.as_call_id_with_errors(db, krate, resolver, &mut |_| ())
+        self.as_call_id_with_errors(db, krate, resolver, &mut |_| ()).ok()?.ok()
     }
 
     fn as_call_id_with_errors(
@@ -592,7 +592,7 @@ pub trait AsMacroCall {
         krate: CrateId,
         resolver: impl Fn(path::ModPath) -> Option<MacroDefId>,
         error_sink: &mut dyn FnMut(mbe::ExpandError),
-    ) -> Option<MacroCallId>;
+    ) -> Result<Result<MacroCallId, ErrorEmitted>, UnresolvedMacro>;
 }
 
 impl AsMacroCall for InFile<&ast::MacroCall> {
@@ -601,25 +601,28 @@ impl AsMacroCall for InFile<&ast::MacroCall> {
         db: &dyn db::DefDatabase,
         krate: CrateId,
         resolver: impl Fn(path::ModPath) -> Option<MacroDefId>,
-        error_sink: &mut dyn FnMut(mbe::ExpandError),
-    ) -> Option<MacroCallId> {
+        mut error_sink: &mut dyn FnMut(mbe::ExpandError),
+    ) -> Result<Result<MacroCallId, ErrorEmitted>, UnresolvedMacro> {
         let ast_id = AstId::new(self.file_id, db.ast_id_map(self.file_id).ast_id(self.value));
         let h = Hygiene::new(db.upcast(), self.file_id);
         let path = self.value.path().and_then(|path| path::ModPath::from_src(path, &h));
 
-        if path.is_none() {
-            error_sink(mbe::ExpandError::Other("malformed macro invocation".into()));
-        }
+        let path = match error_sink
+            .option(path, || mbe::ExpandError::Other("malformed macro invocation".into()))
+        {
+            Ok(path) => path,
+            Err(error) => {
+                return Ok(Err(error));
+            }
+        };
 
         macro_call_as_call_id(
-            &AstIdWithPath::new(ast_id.file_id, ast_id.value, path?),
+            &AstIdWithPath::new(ast_id.file_id, ast_id.value, path),
             db,
             krate,
             resolver,
             error_sink,
         )
-        .ok()?
-        .ok()
     }
 }
 
@@ -636,7 +639,7 @@ impl<T: ast::AstNode> AstIdWithPath<T> {
     }
 }
 
-struct UnresolvedMacro;
+pub struct UnresolvedMacro;
 
 fn macro_call_as_call_id(
     call: &AstIdWithPath<ast::MacroCall>,
diff --git a/crates/hir_expand/src/eager.rs b/crates/hir_expand/src/eager.rs
index ae7b51a08..dc618a9ee 100644
--- a/crates/hir_expand/src/eager.rs
+++ b/crates/hir_expand/src/eager.rs
@@ -35,7 +35,7 @@ pub struct ErrorEmitted {
     _private: (),
 }
 
-trait ErrorSink {
+pub trait ErrorSink {
     fn emit(&mut self, err: mbe::ExpandError);
 
     fn option<T>(