Fix duplicate eager expansion errors

10 files changed, 70 insertions, 50 deletions

diff --git a/crates/hir-def/src/data.rs b/crates/hir-def/src/data.rs
index 95581727add..668b436e019 100644
--- a/crates/hir-def/src/data.rs
+++ b/crates/hir-def/src/data.rs
@@ -638,7 +638,6 @@ impl<'a> AssocItemCollector<'a> {
                     self.items.push((item.name.clone(), def.into()));
                 }
                 AssocItem::MacroCall(call) => {
-                    // TODO: These are the wrong errors to report, report in collect_macro_items instead
                     let file_id = self.expander.current_file_id();
                     let root = self.db.parse_or_expand(file_id);
                     if let Some(root) = root {
diff --git a/crates/hir-def/src/item_tree.rs b/crates/hir-def/src/item_tree.rs
index 8546d36d798..48c1baf3081 100644
--- a/crates/hir-def/src/item_tree.rs
+++ b/crates/hir-def/src/item_tree.rs
@@ -101,6 +101,7 @@ pub struct ItemTree {
     top_level: SmallVec<[ModItem; 1]>,
     attrs: FxHashMap<AttrOwner, RawAttrs>,
 
+    // FIXME: Remove this indirection, an item tree is almost always non-empty?
     data: Option<Box<ItemTreeData>>,
 }
 
diff --git a/crates/hir-def/src/lib.rs b/crates/hir-def/src/lib.rs
index 55e1e91220f..34d704942ab 100644
--- a/crates/hir-def/src/lib.rs
+++ b/crates/hir-def/src/lib.rs
@@ -823,7 +823,7 @@ impl AsMacroCall for InFile<&ast::MacroCall> {
             return Ok(ExpandResult::only_err(ExpandError::Other("malformed macro invocation".into())));
         };
 
-        macro_call_as_call_id(
+        macro_call_as_call_id_(
             db,
             &AstIdWithPath::new(ast_id.file_id, ast_id.value, path),
             expands_to,
@@ -852,6 +852,16 @@ fn macro_call_as_call_id(
     expand_to: ExpandTo,
     krate: CrateId,
     resolver: impl Fn(path::ModPath) -> Option<MacroDefId>,
+) -> Result<Option<MacroCallId>, UnresolvedMacro> {
+    macro_call_as_call_id_(db, call, expand_to, krate, resolver).map(|res| res.value)
+}
+
+fn macro_call_as_call_id_(
+    db: &dyn db::DefDatabase,
+    call: &AstIdWithPath<ast::MacroCall>,
+    expand_to: ExpandTo,
+    krate: CrateId,
+    resolver: impl Fn(path::ModPath) -> Option<MacroDefId>,
 ) -> Result<ExpandResult<Option<MacroCallId>>, UnresolvedMacro> {
     let def =
         resolver(call.path.clone()).ok_or_else(|| UnresolvedMacro { path: call.path.clone() })?;
diff --git a/crates/hir-def/src/nameres/collector.rs b/crates/hir-def/src/nameres/collector.rs
index 8ab0b3dbd14..6592c6b9024 100644
--- a/crates/hir-def/src/nameres/collector.rs
+++ b/crates/hir-def/src/nameres/collector.rs
@@ -1117,8 +1117,9 @@ impl DefCollector<'_> {
                         self.def_map.krate,
                         resolver_def_id,
                     );
-                    if let Ok(ExpandResult { value: Some(call_id), .. }) = call_id {
+                    if let Ok(Some(call_id)) = call_id {
                         push_resolved(directive, call_id);
+
                         res = ReachedFixedPoint::No;
                         return false;
                     }
@@ -1354,26 +1355,30 @@ impl DefCollector<'_> {
         let file_id = macro_call_id.as_file();
 
         // First, fetch the raw expansion result for purposes of error reporting. This goes through
-        // `macro_expand_error` to avoid depending on the full expansion result (to improve
+        // `parse_macro_expansion_error` to avoid depending on the full expansion result (to improve
         // incrementality).
-        let loc: MacroCallLoc = self.db.lookup_intern_macro_call(macro_call_id);
-        let err = self.db.macro_expand_error(macro_call_id);
+        let ExpandResult { value, err } = self.db.parse_macro_expansion_error(macro_call_id);
         if let Some(err) = err {
+            let loc: MacroCallLoc = self.db.lookup_intern_macro_call(macro_call_id);
             let diag = match err {
+                // why is this reported here?
                 hir_expand::ExpandError::UnresolvedProcMacro(krate) => {
                     always!(krate == loc.def.krate);
-                    // Missing proc macros are non-fatal, so they are handled specially.
                     DefDiagnostic::unresolved_proc_macro(module_id, loc.kind.clone(), loc.def.krate)
                 }
-                _ => DefDiagnostic::macro_error(module_id, loc.kind, err.to_string()),
+                _ => DefDiagnostic::macro_error(module_id, loc.kind.clone(), err.to_string()),
             };
 
             self.def_map.diagnostics.push(diag);
         }
+        if let Some(errors) = value {
+            let loc: MacroCallLoc = self.db.lookup_intern_macro_call(macro_call_id);
+            let diag = DefDiagnostic::macro_expansion_parse_error(module_id, loc.kind, &errors);
+            self.def_map.diagnostics.push(diag);
+        }
 
         // Then, fetch and process the item tree. This will reuse the expansion result from above.
         let item_tree = self.db.file_item_tree(file_id);
-        // FIXME: report parse errors for the macro expansion here
 
         let mod_dir = self.mod_dirs[&module_id].clone();
         ModCollector {
@@ -1395,6 +1400,7 @@ impl DefCollector<'_> {
         for directive in &self.unresolved_macros {
             match &directive.kind {
                 MacroDirectiveKind::FnLike { ast_id, expand_to } => {
+                    // FIXME: we shouldn't need to re-resolve the macro here just to get the unresolved error!
                     let macro_call_as_call_id = macro_call_as_call_id(
                         self.db,
                         ast_id,
@@ -2110,7 +2116,7 @@ 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
-        match macro_call_as_call_id(
+        if let Ok(res) = macro_call_as_call_id(
             self.def_collector.db,
             &ast_id,
             mac.expand_to,
@@ -2131,29 +2137,18 @@ impl ModCollector<'_, '_> {
                 })
             },
         ) {
-            Ok(res) => {
-                // Legacy macros need to be expanded immediately, so that any macros they produce
-                // are in scope.
-                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) = 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 },
-                        err.to_string(),
-                    ));
-                }
-
-                return;
+            // Legacy macros need to be expanded immediately, so that any macros they produce
+            // are in scope.
+            if let Some(val) = res {
+                self.def_collector.collect_macro_expansion(
+                    self.module_id,
+                    val,
+                    self.macro_depth + 1,
+                    container,
+                );
             }
-            Err(UnresolvedMacro { .. }) => (),
+
+            return;
         }
 
         // Case 2: resolve in module scope, expand during name resolution.
diff --git a/crates/hir-def/src/nameres/tests/incremental.rs b/crates/hir-def/src/nameres/tests/incremental.rs
index 13e6825f821..b07462cde0f 100644
--- a/crates/hir-def/src/nameres/tests/incremental.rs
+++ b/crates/hir-def/src/nameres/tests/incremental.rs
@@ -140,7 +140,7 @@ m!(Z);
         let n_recalculated_item_trees = events.iter().filter(|it| it.contains("item_tree")).count();
         assert_eq!(n_recalculated_item_trees, 6);
         let n_reparsed_macros =
-            events.iter().filter(|it| it.contains("parse_macro_expansion")).count();
+            events.iter().filter(|it| it.contains("parse_macro_expansion(")).count();
         assert_eq!(n_reparsed_macros, 3);
     }
 
diff --git a/crates/hir-expand/src/db.rs b/crates/hir-expand/src/db.rs
index 1749698387d..afc2be07418 100644
--- a/crates/hir-expand/src/db.rs
+++ b/crates/hir-expand/src/db.rs
@@ -9,7 +9,7 @@ use mbe::syntax_node_to_token_tree;
 use rustc_hash::FxHashSet;
 use syntax::{
     ast::{self, HasAttrs, HasDocComments},
-    AstNode, GreenNode, Parse, SyntaxNode, SyntaxToken, T,
+    AstNode, GreenNode, Parse, SyntaxError, SyntaxNode, SyntaxToken, T,
 };
 
 use crate::{
@@ -132,15 +132,18 @@ pub trait ExpandDatabase: SourceDatabase {
     /// just fetches procedural ones.
     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)
+    /// Expand macro call to a token tree.
     fn macro_expand(&self, macro_call: MacroCallId) -> ExpandResult<Option<Arc<tt::Subtree>>>;
     /// Special case of the previous query for procedural macros. We can't LRU
     /// proc macros, since they are not deterministic in general, and
     /// non-determinism breaks salsa in a very, very, very bad way. @edwin0cheng
     /// heroically debugged this once!
     fn expand_proc_macro(&self, call: MacroCallId) -> ExpandResult<tt::Subtree>;
-    /// Firewall query that returns the error from the `macro_expand` query.
-    fn macro_expand_error(&self, macro_call: MacroCallId) -> Option<ExpandError>;
+    /// Firewall query that returns the errors from the `parse_macro_expansion` query.
+    fn parse_macro_expansion_error(
+        &self,
+        macro_call: MacroCallId,
+    ) -> ExpandResult<Option<Box<[SyntaxError]>>>;
 
     fn hygiene_frame(&self, file_id: HirFileId) -> Arc<HygieneFrame>;
 }
@@ -448,6 +451,7 @@ fn macro_def(
 fn macro_expand(
     db: &dyn ExpandDatabase,
     id: MacroCallId,
+    // FIXME: Remove the OPtion if possible
 ) -> ExpandResult<Option<Arc<tt::Subtree>>> {
     let _p = profile::span("macro_expand");
     let loc: MacroCallLoc = db.lookup_intern_macro_call(id);
@@ -498,8 +502,12 @@ fn macro_expand(
     ExpandResult { value: Some(Arc::new(tt)), err }
 }
 
-fn macro_expand_error(db: &dyn ExpandDatabase, macro_call: MacroCallId) -> Option<ExpandError> {
-    db.macro_expand(macro_call).err
+fn parse_macro_expansion_error(
+    db: &dyn ExpandDatabase,
+    macro_call_id: MacroCallId,
+) -> ExpandResult<Option<Box<[SyntaxError]>>> {
+    db.parse_macro_expansion(MacroFile { macro_call_id })
+        .map(|it| it.map(|(it, _)| it.errors().to_vec().into_boxed_slice()))
 }
 
 fn expand_proc_macro(db: &dyn ExpandDatabase, id: MacroCallId) -> ExpandResult<tt::Subtree> {
diff --git a/crates/hir/src/db.rs b/crates/hir/src/db.rs
index 0935b5ea519..7ec27af04b9 100644
--- a/crates/hir/src/db.rs
+++ b/crates/hir/src/db.rs
@@ -6,8 +6,8 @@
 pub use hir_def::db::*;
 pub use hir_expand::db::{
     AstIdMapQuery, ExpandDatabase, ExpandDatabaseStorage, ExpandProcMacroQuery, HygieneFrameQuery,
-    InternMacroCallQuery, MacroArgTextQuery, MacroDefQuery, MacroExpandErrorQuery,
-    MacroExpandQuery, ParseMacroExpansionQuery,
+    InternMacroCallQuery, MacroArgTextQuery, MacroDefQuery, MacroExpandQuery,
+    ParseMacroExpansionQuery,
 };
 pub use hir_ty::db::*;
 
diff --git a/crates/ide-db/src/apply_change.rs b/crates/ide-db/src/apply_change.rs
index 3b8458980c6..8d14371d034 100644
--- a/crates/ide-db/src/apply_change.rs
+++ b/crates/ide-db/src/apply_change.rs
@@ -80,7 +80,6 @@ impl RootDatabase {
             hir::db::MacroDefQuery
             hir::db::MacroExpandQuery
             hir::db::ExpandProcMacroQuery
-            hir::db::MacroExpandErrorQuery
             hir::db::HygieneFrameQuery
 
             // DefDatabase
diff --git a/crates/ide-db/src/lib.rs b/crates/ide-db/src/lib.rs
index 11b2a5e1c2e..12b6e3c4bb8 100644
--- a/crates/ide-db/src/lib.rs
+++ b/crates/ide-db/src/lib.rs
@@ -152,7 +152,6 @@ impl RootDatabase {
         let lru_capacity = lru_capacity.unwrap_or(base_db::DEFAULT_LRU_CAP);
         base_db::ParseQuery.in_db_mut(self).set_lru_capacity(lru_capacity);
         hir::db::ParseMacroExpansionQuery.in_db_mut(self).set_lru_capacity(lru_capacity);
-        hir::db::MacroExpandQuery.in_db_mut(self).set_lru_capacity(lru_capacity);
     }
 
     pub fn update_lru_capacities(&mut self, lru_capacities: &FxHashMap<Box<str>, usize>) {
@@ -167,12 +166,6 @@ impl RootDatabase {
                 .copied()
                 .unwrap_or(base_db::DEFAULT_LRU_CAP),
         );
-        hir_db::MacroExpandQuery.in_db_mut(self).set_lru_capacity(
-            lru_capacities
-                .get(stringify!(MacroExpandQuery))
-                .copied()
-                .unwrap_or(base_db::DEFAULT_LRU_CAP),
-        );
 
         macro_rules! update_lru_capacity_per_query {
             ($( $module:ident :: $query:ident )*) => {$(
@@ -201,7 +194,6 @@ impl RootDatabase {
             hir_db::MacroDefQuery
             // hir_db::MacroExpandQuery
             hir_db::ExpandProcMacroQuery
-            hir_db::MacroExpandErrorQuery
             hir_db::HygieneFrameQuery
 
             // DefDatabase
diff --git a/crates/ide-diagnostics/src/handlers/macro_error.rs b/crates/ide-diagnostics/src/handlers/macro_error.rs
index af74015cf99..7547779a95c 100644
--- a/crates/ide-diagnostics/src/handlers/macro_error.rs
+++ b/crates/ide-diagnostics/src/handlers/macro_error.rs
@@ -241,4 +241,20 @@ fn f() {
 "#,
         )
     }
+
+    #[test]
+    fn expansion_syntax_diagnostic() {
+        check_diagnostics(
+            r#"
+macro_rules! foo {
+    () => { struct; };
+}
+
+fn f() {
+    foo!();
+  //^^^ error: Syntax Error in Expansion: expected a name
+}
+"#,
+        )
+    }
 }