Auto merge of #14583 - Veykril:macro-def-err, r=Veykril

internal: Report macro definition errors on the definition

We still report them on the call site as well for the time being, and the diagnostic doesn't know where the error in the definition comes from, but that can be done later on

crates/hir-def/src/body.rs

@@ -190,8 +190,9 @@ impl Expander {
 
         let file_id = call_id.as_file();
 
-        let raw_node = match db.parse_or_expand(file_id) {
+        let raw_node = match db.parse_or_expand_with_err(file_id) {
-            Some(it) => it,
+            // FIXME: report parse errors
+            Some(it) => it.syntax_node(),
             None => {
                 // Only `None` if the macro expansion produced no usable AST.
                 if err.is_none() {

crates/hir-def/src/data.rs

@@ -641,7 +641,12 @@ impl<'a> AssocItemCollector<'a> {
                     self.items.push((item.name.clone(), def.into()));
                 }
                 AssocItem::MacroCall(call) => {
-                    if let Some(root) = self.db.parse_or_expand(self.expander.current_file_id()) {
+                    if let Some(root) =
+                        self.db.parse_or_expand_with_err(self.expander.current_file_id())
+                    {
+                        // FIXME: report parse errors
+                        let root = root.syntax_node();
+
                         let call = &item_tree[call];
 
                         let ast_id_map = self.db.ast_id_map(self.expander.current_file_id());

crates/hir-def/src/nameres/collector.rs

@@ -1374,6 +1374,8 @@ impl DefCollector<'_> {
 
         // 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 {
             def_collector: &mut *self,

crates/hir-def/src/nameres/diagnostics.rs

@@ -34,6 +34,8 @@ pub enum DefDiagnosticKind {
     InvalidDeriveTarget { ast: AstId<ast::Item>, id: usize },
 
     MalformedDerive { ast: AstId<ast::Adt>, id: usize },
+
+    MacroDefError { ast: AstId<ast::Macro>, message: String },
 }
 
 #[derive(Debug, PartialEq, Eq)]

crates/hir-expand/src/db.rs

@@ -99,6 +99,8 @@ pub trait ExpandDatabase: SourceDatabase {
     /// file or a macro expansion.
     #[salsa::transparent]
     fn parse_or_expand(&self, file_id: HirFileId) -> Option<SyntaxNode>;
+    #[salsa::transparent]
+    fn parse_or_expand_with_err(&self, file_id: HirFileId) -> Option<Parse<SyntaxNode>>;
     /// Implementation for the macro case.
     fn parse_macro_expansion(
         &self,
@@ -252,13 +254,23 @@ fn parse_or_expand(db: &dyn ExpandDatabase, file_id: HirFileId) -> Option<Syntax
     match file_id.repr() {
         HirFileIdRepr::FileId(file_id) => Some(db.parse(file_id).tree().syntax().clone()),
         HirFileIdRepr::MacroFile(macro_file) => {
-            // FIXME: Note how we convert from `Parse` to `SyntaxNode` here,
-            // forgetting about parse errors.
             db.parse_macro_expansion(macro_file).value.map(|(it, _)| it.syntax_node())
         }
     }
 }
 
+fn parse_or_expand_with_err(
+    db: &dyn ExpandDatabase,
+    file_id: HirFileId,
+) -> Option<Parse<SyntaxNode>> {
+    match file_id.repr() {
+        HirFileIdRepr::FileId(file_id) => Some(db.parse(file_id).to_syntax()),
+        HirFileIdRepr::MacroFile(macro_file) => {
+            db.parse_macro_expansion(macro_file).value.map(|(parse, _)| parse)
+        }
+    }
+}
+
 fn parse_macro_expansion(
     db: &dyn ExpandDatabase,
     macro_file: MacroFile,

crates/hir-expand/src/eager.rs

@@ -187,7 +187,10 @@ fn lazy_expand(
     );
 
     let err = db.macro_expand_error(id);
-    let value = db.parse_or_expand(id.as_file()).map(|node| InFile::new(id.as_file(), node));
+    let value =
+        db.parse_or_expand_with_err(id.as_file()).map(|node| InFile::new(id.as_file(), node));
+    // FIXME: report parse errors
+    let value = value.map(|it| it.map(|it| it.syntax_node()));
 
     ExpandResult { value, err }
 }

crates/hir/src/diagnostics.rs

@@ -39,6 +39,7 @@ diagnostics![
     InvalidDeriveTarget,
     IncoherentImpl,
     MacroError,
+    MacroDefError,
     MalformedDerive,
     MismatchedArgCount,
     MissingFields,
@@ -131,6 +132,13 @@ pub struct MacroError {
     pub message: String,
 }
 
+#[derive(Debug, Clone, Eq, PartialEq)]
+pub struct MacroDefError {
+    pub node: InFile<AstPtr<ast::Macro>>,
+    pub message: String,
+    pub name: Option<TextRange>,
+}
+
 #[derive(Debug)]
 pub struct UnimplementedBuiltinMacro {
     pub node: InFile<SyntaxNodePtr>,

crates/hir/src/lib.rs

@@ -46,6 +46,7 @@ use hir_def::{
     item_tree::ItemTreeNode,
     lang_item::{LangItem, LangItemTarget},
     layout::ReprOptions,
+    macro_id_to_def_id,
     nameres::{self, diagnostics::DefDiagnostic, ModuleOrigin},
     per_ns::PerNs,
     resolver::{HasResolver, Resolver},
@@ -86,12 +87,12 @@ pub use crate::{
     attrs::{HasAttrs, Namespace},
     diagnostics::{
         AnyDiagnostic, BreakOutsideOfLoop, ExpectedFunction, InactiveCode, IncoherentImpl,
-        IncorrectCase, InvalidDeriveTarget, MacroError, MalformedDerive, MismatchedArgCount,
+        IncorrectCase, InvalidDeriveTarget, MacroDefError, MacroError, MalformedDerive,
-        MissingFields, MissingMatchArms, MissingUnsafe, NeedMut, NoSuchField, PrivateAssocItem,
+        MismatchedArgCount, MissingFields, MissingMatchArms, MissingUnsafe, NeedMut, NoSuchField,
-        PrivateField, ReplaceFilterMapNextWithFindMap, TypeMismatch, UndeclaredLabel,
+        PrivateAssocItem, PrivateField, ReplaceFilterMapNextWithFindMap, TypeMismatch,
-        UnimplementedBuiltinMacro, UnreachableLabel, UnresolvedExternCrate, UnresolvedField,
+        UndeclaredLabel, UnimplementedBuiltinMacro, UnreachableLabel, UnresolvedExternCrate,
-        UnresolvedImport, UnresolvedMacroCall, UnresolvedMethodCall, UnresolvedModule,
+        UnresolvedField, UnresolvedImport, UnresolvedMacroCall, UnresolvedMethodCall,
-        UnresolvedProcMacro, UnusedMut,
+        UnresolvedModule, UnresolvedProcMacro, UnusedMut,
     },
     has_source::HasSource,
     semantics::{PathResolution, Semantics, SemanticsScope, TypeInfo, VisibleTraits},
@@ -563,6 +564,7 @@ impl Module {
             }
             emit_def_diagnostic(db, acc, diag);
         }
+
         for decl in self.declarations(db) {
             match decl {
                 ModuleDef::Module(m) => {
@@ -601,9 +603,11 @@ impl Module {
                     }
                     acc.extend(decl.diagnostics(db))
                 }
+                ModuleDef::Macro(m) => emit_macro_def_diagnostics(db, acc, m),
                 _ => acc.extend(decl.diagnostics(db)),
             }
         }
+        self.legacy_macros(db).into_iter().for_each(|m| emit_macro_def_diagnostics(db, acc, m));
 
         let inherent_impls = db.inherent_impls_in_crate(self.id.krate());
 
@@ -685,8 +689,31 @@ impl Module {
     }
 }
 
+fn emit_macro_def_diagnostics(db: &dyn HirDatabase, acc: &mut Vec<AnyDiagnostic>, m: Macro) {
+    let id = macro_id_to_def_id(db.upcast(), m.id);
+    if let Err(e) = db.macro_def(id) {
+        let Some(ast) = id.ast_id().left() else {
+                never!("MacroDefError for proc-macro: {:?}", e);
+                return;
+            };
+        emit_def_diagnostic_(
+            db,
+            acc,
+            &DefDiagnosticKind::MacroDefError { ast, message: e.to_string() },
+        );
+    }
+}
+
 fn emit_def_diagnostic(db: &dyn HirDatabase, acc: &mut Vec<AnyDiagnostic>, diag: &DefDiagnostic) {
-    match &diag.kind {
+    emit_def_diagnostic_(db, acc, &diag.kind)
+}
+
+fn emit_def_diagnostic_(
+    db: &dyn HirDatabase,
+    acc: &mut Vec<AnyDiagnostic>,
+    diag: &DefDiagnosticKind,
+) {
+    match diag {
         DefDiagnosticKind::UnresolvedModule { ast: declaration, candidates } => {
             let decl = declaration.to_node(db.upcast());
             acc.push(
@@ -794,6 +821,17 @@ fn emit_def_diagnostic(db: &dyn HirDatabase, acc: &mut Vec<AnyDiagnostic>, diag:
                 None => stdx::never!("derive diagnostic on item without derive attribute"),
             }
         }
+        DefDiagnosticKind::MacroDefError { ast, message } => {
+            let node = ast.to_node(db.upcast());
+            acc.push(
+                MacroDefError {
+                    node: InFile::new(ast.file_id, AstPtr::new(&node)),
+                    name: node.name().map(|it| it.syntax().text_range()),
+                    message: message.clone(),
+                }
+                .into(),
+            );
+        }
     }
 }
 

crates/ide-diagnostics/src/handlers/macro_error.rs

@@ -9,6 +9,16 @@ pub(crate) fn macro_error(ctx: &DiagnosticsContext<'_>, d: &hir::MacroError) ->
     Diagnostic::new("macro-error", d.message.clone(), display_range).experimental()
 }
 
+// Diagnostic: macro-error
+//
+// This diagnostic is shown for macro expansion errors.
+pub(crate) fn macro_def_error(ctx: &DiagnosticsContext<'_>, d: &hir::MacroDefError) -> Diagnostic {
+    // Use more accurate position if available.
+    let display_range =
+        ctx.resolve_precise_location(&d.node.clone().map(|it| it.syntax_node_ptr()), d.name);
+    Diagnostic::new("macro-def-error", d.message.clone(), display_range).experimental()
+}
+
 #[cfg(test)]
 mod tests {
     use crate::{
@@ -188,6 +198,7 @@ fn f() {
       "#,
         );
     }
+
     #[test]
     fn dollar_crate_in_builtin_macro() {
         check_diagnostics(
@@ -209,6 +220,24 @@ macro_rules! outer {
 fn f() {
     outer!();
 } //^^^^^^^^ error: leftover tokens
+"#,
+        )
+    }
+
+    #[test]
+    fn def_diagnostic() {
+        check_diagnostics(
+            r#"
+macro_rules! foo {
+           //^^^ error: expected subtree
+    f => {};
+}
+
+fn f() {
+    foo!();
+  //^^^ error: invalid macro definition: expected subtree
+
+}
 "#,
         )
     }

crates/ide-diagnostics/src/lib.rs

@@ -249,7 +249,7 @@ pub fn diagnostics(
 
     let mut diags = Vec::new();
     if let Some(m) = module {
-        m.diagnostics(db, &mut diags)
+        m.diagnostics(db, &mut diags);
     }
 
     for diag in diags {
@@ -263,6 +263,7 @@ pub fn diagnostics(
             AnyDiagnostic::IncoherentImpl(d) => handlers::incoherent_impl::incoherent_impl(&ctx, &d),
             AnyDiagnostic::IncorrectCase(d) => handlers::incorrect_case::incorrect_case(&ctx, &d),
             AnyDiagnostic::InvalidDeriveTarget(d) => handlers::invalid_derive_target::invalid_derive_target(&ctx, &d),
+            AnyDiagnostic::MacroDefError(d) => handlers::macro_error::macro_def_error(&ctx, &d),
             AnyDiagnostic::MacroError(d) => handlers::macro_error::macro_error(&ctx, &d),
             AnyDiagnostic::MalformedDerive(d) => handlers::malformed_derive::malformed_derive(&ctx, &d),
             AnyDiagnostic::MismatchedArgCount(d) => handlers::mismatched_arg_count::mismatched_arg_count(&ctx, &d),