internal: Report macro definition errors on the definition

Author: Veykril

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) {
-            Some(it) => it,
+        let raw_node = match db.parse_or_expand_with_err(file_id) {
+            // 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,
-        MissingFields, MissingMatchArms, MissingUnsafe, NeedMut, NoSuchField, PrivateAssocItem,
-        PrivateField, ReplaceFilterMapNextWithFindMap, TypeMismatch, UndeclaredLabel,
-        UnimplementedBuiltinMacro, UnreachableLabel, UnresolvedExternCrate, UnresolvedField,
-        UnresolvedImport, UnresolvedMacroCall, UnresolvedMethodCall, UnresolvedModule,
-        UnresolvedProcMacro, UnusedMut,
+        IncorrectCase, InvalidDeriveTarget, MacroDefError, MacroError, MalformedDerive,
+        MismatchedArgCount, MissingFields, MissingMatchArms, MissingUnsafe, NeedMut, NoSuchField,
+        PrivateAssocItem, PrivateField, ReplaceFilterMapNextWithFindMap, TypeMismatch,
+        UndeclaredLabel, UnimplementedBuiltinMacro, UnreachableLabel, UnresolvedExternCrate,
+        UnresolvedField, UnresolvedImport, UnresolvedMacroCall, UnresolvedMethodCall,
+        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),