Simplify eager macro error handling

6 files changed, 120 insertions, 211 deletions

diff --git a/crates/hir-def/src/body.rs b/crates/hir-def/src/body.rs
index 928aadfbccc..f4304ae7e8a 100644
--- a/crates/hir-def/src/body.rs
+++ b/crates/hir-def/src/body.rs
@@ -138,6 +138,7 @@ impl Expander {
         db: &dyn DefDatabase,
         macro_call: ast::MacroCall,
     ) -> Result<ExpandResult<Option<(Mark, T)>>, UnresolvedMacro> {
+        // FIXME: within_limit should support this, instead of us having to extract the error
         let mut unresolved_macro_err = None;
 
         let result = self.within_limit(db, |this| {
@@ -146,22 +147,13 @@ impl Expander {
             let resolver =
                 |path| this.resolve_path_as_macro(db, &path).map(|it| macro_id_to_def_id(db, it));
 
-            let mut err = None;
-            let call_id = match macro_call.as_call_id_with_errors(
-                db,
-                this.def_map.krate(),
-                resolver,
-                &mut |e| {
-                    err.get_or_insert(e);
-                },
-            ) {
+            match macro_call.as_call_id_with_errors(db, this.def_map.krate(), resolver) {
                 Ok(call_id) => call_id,
                 Err(resolve_err) => {
                     unresolved_macro_err = Some(resolve_err);
-                    return ExpandResult { value: None, err: None };
+                    ExpandResult { value: None, err: None }
                 }
-            };
-            ExpandResult { value: call_id.ok(), err }
+            }
         });
 
         if let Some(err) = unresolved_macro_err {
diff --git a/crates/hir-def/src/lib.rs b/crates/hir-def/src/lib.rs
index 5a0c1b66b9c..55e1e91220f 100644
--- a/crates/hir-def/src/lib.rs
+++ b/crates/hir-def/src/lib.rs
@@ -65,11 +65,11 @@ use hir_expand::{
     builtin_attr_macro::BuiltinAttrExpander,
     builtin_derive_macro::BuiltinDeriveExpander,
     builtin_fn_macro::{BuiltinFnLikeExpander, EagerExpander},
-    eager::{expand_eager_macro, ErrorEmitted, ErrorSink},
+    eager::expand_eager_macro,
     hygiene::Hygiene,
     proc_macro::ProcMacroExpander,
-    AstId, ExpandError, ExpandTo, HirFileId, InFile, MacroCallId, MacroCallKind, MacroDefId,
-    MacroDefKind, UnresolvedMacro,
+    AstId, ExpandError, ExpandResult, ExpandTo, HirFileId, InFile, MacroCallId, MacroCallKind,
+    MacroDefId, MacroDefKind, UnresolvedMacro,
 };
 use item_tree::ExternBlock;
 use la_arena::Idx;
@@ -795,7 +795,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 |_| ()).ok()?.ok()
+        self.as_call_id_with_errors(db, krate, resolver).ok()?.value
     }
 
     fn as_call_id_with_errors(
@@ -803,8 +803,7 @@ pub trait AsMacroCall {
         db: &dyn db::DefDatabase,
         krate: CrateId,
         resolver: impl Fn(path::ModPath) -> Option<MacroDefId>,
-        error_sink: &mut dyn FnMut(ExpandError),
-    ) -> Result<Result<MacroCallId, ErrorEmitted>, UnresolvedMacro>;
+    ) -> Result<ExpandResult<Option<MacroCallId>>, UnresolvedMacro>;
 }
 
 impl AsMacroCall for InFile<&ast::MacroCall> {
@@ -813,21 +812,15 @@ impl AsMacroCall for InFile<&ast::MacroCall> {
         db: &dyn db::DefDatabase,
         krate: CrateId,
         resolver: impl Fn(path::ModPath) -> Option<MacroDefId>,
-        mut error_sink: &mut dyn FnMut(ExpandError),
-    ) -> Result<Result<MacroCallId, ErrorEmitted>, UnresolvedMacro> {
+    ) -> Result<ExpandResult<Option<MacroCallId>>, UnresolvedMacro> {
         let expands_to = hir_expand::ExpandTo::from_call_site(self.value);
         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(db.upcast(), path, &h));
 
-        let path = match error_sink
-            .option(path, || ExpandError::Other("malformed macro invocation".into()))
-        {
-            Ok(path) => path,
-            Err(error) => {
-                return Ok(Err(error));
-            }
+        let Some(path) = path else {
+            return Ok(ExpandResult::only_err(ExpandError::Other("malformed macro invocation".into())));
         };
 
         macro_call_as_call_id(
@@ -836,7 +829,6 @@ impl AsMacroCall for InFile<&ast::MacroCall> {
             expands_to,
             krate,
             resolver,
-            error_sink,
         )
     }
 }
@@ -860,21 +852,23 @@ fn macro_call_as_call_id(
     expand_to: ExpandTo,
     krate: CrateId,
     resolver: impl Fn(path::ModPath) -> Option<MacroDefId>,
-    error_sink: &mut dyn FnMut(ExpandError),
-) -> Result<Result<MacroCallId, ErrorEmitted>, UnresolvedMacro> {
+) -> Result<ExpandResult<Option<MacroCallId>>, UnresolvedMacro> {
     let def =
         resolver(call.path.clone()).ok_or_else(|| UnresolvedMacro { path: call.path.clone() })?;
 
     let res = if let MacroDefKind::BuiltInEager(..) = def.kind {
         let macro_call = InFile::new(call.ast_id.file_id, call.ast_id.to_node(db.upcast()));
 
-        expand_eager_macro(db.upcast(), krate, macro_call, def, &resolver, error_sink)?
+        expand_eager_macro(db.upcast(), krate, macro_call, def, &resolver)?
     } else {
-        Ok(def.as_lazy_macro(
-            db.upcast(),
-            krate,
-            MacroCallKind::FnLike { ast_id: call.ast_id, expand_to },
-        ))
+        ExpandResult {
+            value: Some(def.as_lazy_macro(
+                db.upcast(),
+                krate,
+                MacroCallKind::FnLike { ast_id: call.ast_id, expand_to },
+            )),
+            err: None,
+        }
     };
     Ok(res)
 }
diff --git a/crates/hir-def/src/macro_expansion_tests/mod.rs b/crates/hir-def/src/macro_expansion_tests/mod.rs
index 314bf22b95e..73a495d89bf 100644
--- a/crates/hir-def/src/macro_expansion_tests/mod.rs
+++ b/crates/hir-def/src/macro_expansion_tests/mod.rs
@@ -125,21 +125,15 @@ pub fn identity_when_valid(_attr: TokenStream, item: TokenStream) -> TokenStream
 
     for macro_call in source_file.syntax().descendants().filter_map(ast::MacroCall::cast) {
         let macro_call = InFile::new(source.file_id, &macro_call);
-        let mut error = None;
-        let macro_call_id = macro_call
-            .as_call_id_with_errors(
-                &db,
-                krate,
-                |path| {
-                    resolver.resolve_path_as_macro(&db, &path).map(|it| macro_id_to_def_id(&db, it))
-                },
-                &mut |err| error = Some(err),
-            )
-            .unwrap()
+        let res = macro_call
+            .as_call_id_with_errors(&db, krate, |path| {
+                resolver.resolve_path_as_macro(&db, &path).map(|it| macro_id_to_def_id(&db, it))
+            })
             .unwrap();
+        let macro_call_id = res.value.unwrap();
         let macro_file = MacroFile { macro_call_id };
         let mut expansion_result = db.parse_macro_expansion(macro_file);
-        expansion_result.err = expansion_result.err.or(error);
+        expansion_result.err = expansion_result.err.or(res.err);
         expansions.push((macro_call.value.clone(), expansion_result, db.macro_arg(macro_call_id)));
     }
 
diff --git a/crates/hir-def/src/nameres/collector.rs b/crates/hir-def/src/nameres/collector.rs
index 1d625fa3c7c..8ab0b3dbd14 100644
--- a/crates/hir-def/src/nameres/collector.rs
+++ b/crates/hir-def/src/nameres/collector.rs
@@ -16,8 +16,8 @@ use hir_expand::{
     builtin_fn_macro::find_builtin_macro,
     name::{name, AsName, Name},
     proc_macro::ProcMacroExpander,
-    ExpandTo, HirFileId, InFile, MacroCallId, MacroCallKind, MacroCallLoc, MacroDefId,
-    MacroDefKind,
+    ExpandResult, ExpandTo, HirFileId, InFile, MacroCallId, MacroCallKind, MacroCallLoc,
+    MacroDefId, MacroDefKind,
 };
 use itertools::{izip, Itertools};
 use la_arena::Idx;
@@ -1116,9 +1116,8 @@ impl DefCollector<'_> {
                         *expand_to,
                         self.def_map.krate,
                         resolver_def_id,
-                        &mut |_err| (),
                     );
-                    if let Ok(Ok(call_id)) = call_id {
+                    if let Ok(ExpandResult { value: Some(call_id), .. }) = call_id {
                         push_resolved(directive, call_id);
                         res = ReachedFixedPoint::No;
                         return false;
@@ -1414,7 +1413,6 @@ impl DefCollector<'_> {
                                 .take_macros()
                                 .map(|it| macro_id_to_def_id(self.db, it))
                         },
-                        &mut |_| (),
                     );
                     if let Err(UnresolvedMacro { path }) = macro_call_as_call_id {
                         self.def_map.diagnostics.push(DefDiagnostic::unresolved_macro_call(
@@ -2112,7 +2110,6 @@ impl ModCollector<'_, '_> {
         let ast_id = AstIdWithPath::new(self.file_id(), mac.ast_id, ModPath::clone(&mac.path));
 
         // Case 1: try to resolve in legacy scope and expand macro_rules
-        let mut error = None;
         match macro_call_as_call_id(
             self.def_collector.db,
             &ast_id,
@@ -2133,21 +2130,20 @@ impl ModCollector<'_, '_> {
                     )
                 })
             },
-            &mut |err| {
-                error.get_or_insert(err);
-            },
         ) {
-            Ok(Ok(macro_call_id)) => {
+            Ok(res) => {
                 // Legacy macros need to be expanded immediately, so that any macros they produce
                 // are in scope.
-                self.def_collector.collect_macro_expansion(
-                    self.module_id,
-                    macro_call_id,
-                    self.macro_depth + 1,
-                    container,
-                );
+                if let Some(val) = res.value {
+                    self.def_collector.collect_macro_expansion(
+                        self.module_id,
+                        val,
+                        self.macro_depth + 1,
+                        container,
+                    );
+                }
 
-                if let Some(err) = error {
+                if let Some(err) = res.err {
                     self.def_collector.def_map.diagnostics.push(DefDiagnostic::macro_error(
                         self.module_id,
                         MacroCallKind::FnLike { ast_id: ast_id.ast_id, expand_to: mac.expand_to },
@@ -2157,16 +2153,6 @@ impl ModCollector<'_, '_> {
 
                 return;
             }
-            Ok(Err(_)) => {
-                // Built-in macro failed eager expansion.
-
-                self.def_collector.def_map.diagnostics.push(DefDiagnostic::macro_error(
-                    self.module_id,
-                    MacroCallKind::FnLike { ast_id: ast_id.ast_id, expand_to: mac.expand_to },
-                    error.unwrap().to_string(),
-                ));
-                return;
-            }
             Err(UnresolvedMacro { .. }) => (),
         }
 
diff --git a/crates/hir-expand/src/db.rs b/crates/hir-expand/src/db.rs
index d93f3b08d33..d7aff6b02f4 100644
--- a/crates/hir-expand/src/db.rs
+++ b/crates/hir-expand/src/db.rs
@@ -279,25 +279,28 @@ fn parse_macro_expansion(
     let mbe::ValueResult { value, err } = db.macro_expand(macro_file.macro_call_id);
 
     if let Some(err) = &err {
-        // Note:
-        // The final goal we would like to make all parse_macro success,
-        // such that the following log will not call anyway.
-        let loc: MacroCallLoc = db.lookup_intern_macro_call(macro_file.macro_call_id);
-        let node = loc.kind.to_node(db);
-
-        // collect parent information for warning log
-        let parents =
-            std::iter::successors(loc.kind.file_id().call_node(db), |it| it.file_id.call_node(db))
-                .map(|n| format!("{:#}", n.value))
-                .collect::<Vec<_>>()
-                .join("\n");
-
-        tracing::debug!(
-            "fail on macro_parse: (reason: {:?} macro_call: {:#}) parents: {}",
-            err,
-            node.value,
-            parents
-        );
+        if tracing::enabled!(tracing::Level::DEBUG) {
+            // Note:
+            // The final goal we would like to make all parse_macro success,
+            // such that the following log will not call anyway.
+            let loc: MacroCallLoc = db.lookup_intern_macro_call(macro_file.macro_call_id);
+            let node = loc.kind.to_node(db);
+
+            // collect parent information for warning log
+            let parents = std::iter::successors(loc.kind.file_id().call_node(db), |it| {
+                it.file_id.call_node(db)
+            })
+            .map(|n| format!("{:#}", n.value))
+            .collect::<Vec<_>>()
+            .join("\n");
+
+            tracing::debug!(
+                "fail on macro_parse: (reason: {:?} macro_call: {:#}) parents: {}",
+                err,
+                node.value,
+                parents
+            );
+        }
     }
     let tt = match value {
         Some(tt) => tt,
@@ -466,7 +469,8 @@ fn macro_expand(
         Ok(it) => it,
         // 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).
+        // be reported at the definition site when we construct a def map.
+        // (Note we do report them also at the definition site in the late diagnostic pass)
         Err(err) => {
             return ExpandResult::only_err(ExpandError::Other(
                 format!("invalid macro definition: {err}").into(),
diff --git a/crates/hir-expand/src/eager.rs b/crates/hir-expand/src/eager.rs
index 6b646f5e4fa..74adced8c68 100644
--- a/crates/hir-expand/src/eager.rs
+++ b/crates/hir-expand/src/eager.rs
@@ -32,77 +32,16 @@ use crate::{
     MacroCallLoc, MacroDefId, MacroDefKind, UnresolvedMacro,
 };
 
-#[derive(Debug)]
-pub struct ErrorEmitted {
-    _private: (),
-}
-
-pub trait ErrorSink {
-    fn emit(&mut self, err: ExpandError);
-
-    fn option<T>(
-        &mut self,
-        opt: Option<T>,
-        error: impl FnOnce() -> ExpandError,
-    ) -> Result<T, ErrorEmitted> {
-        match opt {
-            Some(it) => Ok(it),
-            None => {
-                self.emit(error());
-                Err(ErrorEmitted { _private: () })
-            }
-        }
-    }
-
-    fn option_with<T>(
-        &mut self,
-        opt: impl FnOnce() -> Option<T>,
-        error: impl FnOnce() -> ExpandError,
-    ) -> Result<T, ErrorEmitted> {
-        self.option(opt(), error)
-    }
-
-    fn result<T>(&mut self, res: Result<T, ExpandError>) -> Result<T, ErrorEmitted> {
-        match res {
-            Ok(it) => Ok(it),
-            Err(e) => {
-                self.emit(e);
-                Err(ErrorEmitted { _private: () })
-            }
-        }
-    }
-
-    fn expand_result_option<T>(&mut self, res: ExpandResult<Option<T>>) -> Result<T, ErrorEmitted> {
-        match (res.value, res.err) {
-            (None, Some(err)) => {
-                self.emit(err);
-                Err(ErrorEmitted { _private: () })
-            }
-            (Some(value), opt_err) => {
-                if let Some(err) = opt_err {
-                    self.emit(err);
-                }
-                Ok(value)
-            }
-            (None, None) => unreachable!("`ExpandResult` without value or error"),
-        }
-    }
-}
-
-impl ErrorSink for &'_ mut dyn FnMut(ExpandError) {
-    fn emit(&mut self, err: ExpandError) {
-        self(err);
-    }
-}
-
 pub fn expand_eager_macro(
     db: &dyn ExpandDatabase,
     krate: CrateId,
     macro_call: InFile<ast::MacroCall>,
     def: MacroDefId,
     resolver: &dyn Fn(ModPath) -> Option<MacroDefId>,
-    diagnostic_sink: &mut dyn FnMut(ExpandError),
-) -> Result<Result<MacroCallId, ErrorEmitted>, UnresolvedMacro> {
+) -> Result<ExpandResult<Option<MacroCallId>>, UnresolvedMacro> {
+    let MacroDefKind::BuiltInEager(eager, _) = def.kind else {
+        panic!("called `expand_eager_macro` on non-eager macro def {def:?}")
+    };
     let hygiene = Hygiene::new(db, macro_call.file_id);
     let parsed_args = macro_call
         .value
@@ -129,40 +68,34 @@ pub fn expand_eager_macro(
     });
 
     let parsed_args = mbe::token_tree_to_syntax_node(&parsed_args, mbe::TopEntryPoint::Expr).0;
-    let result = match eager_macro_recur(
+    let ExpandResult { value, mut err } = eager_macro_recur(
         db,
         &hygiene,
         InFile::new(arg_id.as_file(), parsed_args.syntax_node()),
         krate,
         resolver,
-        diagnostic_sink,
-    ) {
-        Ok(Ok(it)) => it,
-        Ok(Err(err)) => return Ok(Err(err)),
-        Err(err) => return Err(err),
+    )?;
+    let Some(value ) = value else {
+        return Ok(ExpandResult { value: None, err })
     };
-    let subtree = to_subtree(&result);
+    let subtree = to_subtree(&value);
 
-    if let MacroDefKind::BuiltInEager(eager, _) = def.kind {
-        let res = eager.expand(db, arg_id, &subtree);
-        if let Some(err) = res.err {
-            diagnostic_sink(err);
-        }
+    let res = eager.expand(db, arg_id, &subtree);
+    if err.is_none() {
+        err = res.err;
+    }
 
-        let loc = MacroCallLoc {
-            def,
-            krate,
-            eager: Some(EagerCallInfo {
-                arg_or_expansion: Arc::new(res.value.subtree),
-                included_file: res.value.included_file,
-            }),
-            kind: MacroCallKind::FnLike { ast_id: call_id, expand_to },
-        };
+    let loc = MacroCallLoc {
+        def,
+        krate,
+        eager: Some(EagerCallInfo {
+            arg_or_expansion: Arc::new(res.value.subtree),
+            included_file: res.value.included_file,
+        }),
+        kind: MacroCallKind::FnLike { ast_id: call_id, expand_to },
+    };
 
-        Ok(Ok(db.intern_macro_call(loc)))
-    } else {
-        panic!("called `expand_eager_macro` on non-eager macro def {def:?}");
-    }
+    Ok(ExpandResult { value: Some(db.intern_macro_call(loc)), err })
 }
 
 fn to_subtree(node: &SyntaxNode) -> crate::tt::Subtree {
@@ -201,23 +134,25 @@ fn eager_macro_recur(
     curr: InFile<SyntaxNode>,
     krate: CrateId,
     macro_resolver: &dyn Fn(ModPath) -> Option<MacroDefId>,
-    mut diagnostic_sink: &mut dyn FnMut(ExpandError),
-) -> Result<Result<SyntaxNode, ErrorEmitted>, UnresolvedMacro> {
+) -> Result<ExpandResult<Option<SyntaxNode>>, UnresolvedMacro> {
     let original = curr.value.clone_for_update();
 
     let children = original.descendants().filter_map(ast::MacroCall::cast);
     let mut replacements = Vec::new();
 
+    // Note: We only report a single error inside of eager expansions
+    let mut error = None;
+
     // Collect replacement
     for child in children {
         let def = match child.path().and_then(|path| ModPath::from_src(db, path, hygiene)) {
             Some(path) => macro_resolver(path.clone()).ok_or(UnresolvedMacro { path })?,
             None => {
-                diagnostic_sink(ExpandError::Other("malformed macro invocation".into()));
+                error = Some(ExpandError::Other("malformed macro invocation".into()));
                 continue;
             }
         };
-        let insert = match def.kind {
+        let ExpandResult { value, err } = match def.kind {
             MacroDefKind::BuiltInEager(..) => {
                 let id = match expand_eager_macro(
                     db,
@@ -225,45 +160,49 @@ fn eager_macro_recur(
                     curr.with_value(child.clone()),
                     def,
                     macro_resolver,
-                    diagnostic_sink,
                 ) {
-                    Ok(Ok(it)) => it,
-                    Ok(Err(err)) => return Ok(Err(err)),
+                    Ok(it) => it,
                     Err(err) => return Err(err),
                 };
-                db.parse_or_expand(id.as_file())
-                    .expect("successful macro expansion should be parseable")
-                    .clone_for_update()
+                id.map(|call| {
+                    call.and_then(|call| db.parse_or_expand(call.as_file()))
+                        .map(|it| it.clone_for_update())
+                })
             }
             MacroDefKind::Declarative(_)
             | MacroDefKind::BuiltIn(..)
             | MacroDefKind::BuiltInAttr(..)
             | MacroDefKind::BuiltInDerive(..)
             | MacroDefKind::ProcMacro(..) => {
-                let res = lazy_expand(db, &def, curr.with_value(child.clone()), krate);
-                let val = match diagnostic_sink.expand_result_option(res) {
-                    Ok(it) => it,
-                    Err(err) => return Ok(Err(err)),
-                };
-
-                // replace macro inside
-                let hygiene = Hygiene::new(db, val.file_id);
-                match eager_macro_recur(db, &hygiene, val, krate, macro_resolver, diagnostic_sink) {
-                    Ok(Ok(it)) => it,
-                    Ok(Err(err)) => return Ok(Err(err)),
-                    Err(err) => return Err(err),
+                let ExpandResult { value, err } =
+                    lazy_expand(db, &def, curr.with_value(child.clone()), krate);
+
+                match value {
+                    Some(val) => {
+                        // replace macro inside
+                        let hygiene = Hygiene::new(db, val.file_id);
+                        let ExpandResult { value, err: error } =
+                            eager_macro_recur(db, &hygiene, val, krate, macro_resolver)?;
+                        let err = if err.is_none() { error } else { err };
+                        ExpandResult { value, err }
+                    }
+                    None => ExpandResult { value: None, err },
                 }
             }
         };
-
+        if err.is_some() {
+            error = err;
+        }
         // check if the whole original syntax is replaced
         if child.syntax() == &original {
-            return Ok(Ok(insert));
+            return Ok(ExpandResult { value, err: error });
         }
 
-        replacements.push((child, insert));
+        if let Some(insert) = value {
+            replacements.push((child, insert));
+        }
     }
 
     replacements.into_iter().rev().for_each(|(old, new)| ted::replace(old.syntax(), new));
-    Ok(Ok(original))
+    Ok(ExpandResult { value: Some(original), err: error })
 }