From a5558cdfe50dc105d31c93edb6848a07005e7d85 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Sun, 16 Apr 2023 14:15:59 +0200 Subject: [PATCH] internal: Report macro definition errors on the definition --- crates/hir-def/src/body.rs | 5 +- crates/hir-def/src/data.rs | 7 ++- crates/hir-def/src/nameres/collector.rs | 2 + crates/hir-def/src/nameres/diagnostics.rs | 2 + crates/hir-expand/src/db.rs | 16 +++++- crates/hir-expand/src/eager.rs | 5 +- crates/hir/src/diagnostics.rs | 8 +++ crates/hir/src/lib.rs | 52 ++++++++++++++++--- .../src/handlers/macro_error.rs | 29 +++++++++++ crates/ide-diagnostics/src/lib.rs | 3 +- 10 files changed, 115 insertions(+), 14 deletions(-) diff --git a/crates/hir-def/src/body.rs b/crates/hir-def/src/body.rs index 6b887bb1b31..928aadfbccc 100644 --- a/crates/hir-def/src/body.rs +++ b/crates/hir-def/src/body.rs @@ -190,8 +190,9 @@ fn enter_expand_inner( 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() { diff --git a/crates/hir-def/src/data.rs b/crates/hir-def/src/data.rs index 98cf69c1689..6d2c88660f9 100644 --- a/crates/hir-def/src/data.rs +++ b/crates/hir-def/src/data.rs @@ -641,7 +641,12 @@ fn collect(&mut self, item_tree: &ItemTree, tree_id: TreeId, assoc_items: &[Asso 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()); diff --git a/crates/hir-def/src/nameres/collector.rs b/crates/hir-def/src/nameres/collector.rs index 84c7a80e150..1d625fa3c7c 100644 --- a/crates/hir-def/src/nameres/collector.rs +++ b/crates/hir-def/src/nameres/collector.rs @@ -1374,6 +1374,8 @@ fn collect_macro_expansion( // 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, diff --git a/crates/hir-def/src/nameres/diagnostics.rs b/crates/hir-def/src/nameres/diagnostics.rs index b024d7c6777..a57896e5460 100644 --- a/crates/hir-def/src/nameres/diagnostics.rs +++ b/crates/hir-def/src/nameres/diagnostics.rs @@ -34,6 +34,8 @@ pub enum DefDiagnosticKind { InvalidDeriveTarget { ast: AstId, id: usize }, MalformedDerive { ast: AstId, id: usize }, + + MacroDefError { ast: AstId, message: String }, } #[derive(Debug, PartialEq, Eq)] diff --git a/crates/hir-expand/src/db.rs b/crates/hir-expand/src/db.rs index 37c36615401..d93f3b08d33 100644 --- a/crates/hir-expand/src/db.rs +++ b/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; + #[salsa::transparent] + fn parse_or_expand_with_err(&self, file_id: HirFileId) -> Option>; /// 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 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> { + 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, diff --git a/crates/hir-expand/src/eager.rs b/crates/hir-expand/src/eager.rs index aca41b11f92..6b646f5e4fa 100644 --- a/crates/hir-expand/src/eager.rs +++ b/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 } } diff --git a/crates/hir/src/diagnostics.rs b/crates/hir/src/diagnostics.rs index f756832f0ff..b735decfcb3 100644 --- a/crates/hir/src/diagnostics.rs +++ b/crates/hir/src/diagnostics.rs @@ -39,6 +39,7 @@ fn from(d: $diag) -> AnyDiagnostic { 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>, + pub message: String, + pub name: Option, +} + #[derive(Debug)] pub struct UnimplementedBuiltinMacro { pub node: InFile, diff --git a/crates/hir/src/lib.rs b/crates/hir/src/lib.rs index 3e568a40215..7e9b89db7a1 100644 --- a/crates/hir/src/lib.rs +++ b/crates/hir/src/lib.rs @@ -46,6 +46,7 @@ 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 @@ 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 @@ pub fn diagnostics(self, db: &dyn HirDatabase, acc: &mut Vec) { } emit_def_diagnostic(db, acc, diag); } + for decl in self.declarations(db) { match decl { ModuleDef::Module(m) => { @@ -601,9 +603,11 @@ pub fn diagnostics(self, db: &dyn HirDatabase, acc: &mut Vec) { } 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 @@ pub fn find_use_path_prefixed( } } +fn emit_macro_def_diagnostics(db: &dyn HirDatabase, acc: &mut Vec, 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, diag: &DefDiagnostic) { - match &diag.kind { + emit_def_diagnostic_(db, acc, &diag.kind) +} + +fn emit_def_diagnostic_( + db: &dyn HirDatabase, + acc: &mut Vec, + 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, 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(), + ); + } } } diff --git a/crates/ide-diagnostics/src/handlers/macro_error.rs b/crates/ide-diagnostics/src/handlers/macro_error.rs index 870c78d1f1e..af74015cf99 100644 --- a/crates/ide-diagnostics/src/handlers/macro_error.rs +++ b/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 + +} "#, ) } diff --git a/crates/ide-diagnostics/src/lib.rs b/crates/ide-diagnostics/src/lib.rs index 70116f15a75..59976ecf29c 100644 --- a/crates/ide-diagnostics/src/lib.rs +++ b/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),