From 05729fd3c4aa542d162b54e7352c0d4bade62684 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Sun, 28 Feb 2021 14:12:11 +0300 Subject: [PATCH] For unresolved macros, hightlight only the last segment --- crates/hir/src/diagnostics.rs | 4 +- crates/hir/src/semantics.rs | 8 +- crates/hir_def/src/body.rs | 2 +- crates/hir_def/src/diagnostics.rs | 28 ++++++ crates/hir_def/src/lib.rs | 112 ++++++++++++------------ crates/hir_def/src/nameres.rs | 14 +++ crates/hir_def/src/nameres/collector.rs | 81 ++++++++++------- crates/ide/src/diagnostics.rs | 67 +++++++++++--- 8 files changed, 208 insertions(+), 108 deletions(-) diff --git a/crates/hir/src/diagnostics.rs b/crates/hir/src/diagnostics.rs index 5343a036c0..b1ebba5164 100644 --- a/crates/hir/src/diagnostics.rs +++ b/crates/hir/src/diagnostics.rs @@ -1,5 +1,7 @@ //! FIXME: write short doc here -pub use hir_def::diagnostics::{InactiveCode, UnresolvedModule, UnresolvedProcMacro}; +pub use hir_def::diagnostics::{ + InactiveCode, UnresolvedMacroCall, UnresolvedModule, UnresolvedProcMacro, +}; pub use hir_expand::diagnostics::{ Diagnostic, DiagnosticCode, DiagnosticSink, DiagnosticSinkBuilder, }; diff --git a/crates/hir/src/semantics.rs b/crates/hir/src/semantics.rs index 59292d5a2f..144851f834 100644 --- a/crates/hir/src/semantics.rs +++ b/crates/hir/src/semantics.rs @@ -16,13 +16,12 @@ use rustc_hash::{FxHashMap, FxHashSet}; use syntax::{ algo::find_node_at_offset, ast::{self, GenericParamsOwner, LoopBodyOwner}, - match_ast, AstNode, SyntaxNode, SyntaxToken, TextSize, + match_ast, AstNode, SyntaxNode, SyntaxNodePtr, SyntaxToken, TextSize, }; use crate::{ code_model::Access, db::HirDatabase, - diagnostics::Diagnostic, semantics::source_to_def::{ChildContainer, SourceToDefCache, SourceToDefCtx}, source_analyzer::{resolve_hir_path, SourceAnalyzer}, AssocItem, Callable, ConstParam, Crate, Field, Function, HirFileId, Impl, InFile, Label, @@ -141,7 +140,7 @@ impl<'db, DB: HirDatabase> Semantics<'db, DB> { self.imp.original_range(node) } - pub fn diagnostics_display_range(&self, diagnostics: &dyn Diagnostic) -> FileRange { + pub fn diagnostics_display_range(&self, diagnostics: InFile) -> FileRange { self.imp.diagnostics_display_range(diagnostics) } @@ -385,8 +384,7 @@ impl<'db> SemanticsImpl<'db> { node.as_ref().original_file_range(self.db.upcast()) } - fn diagnostics_display_range(&self, diagnostics: &dyn Diagnostic) -> FileRange { - let src = diagnostics.display_source(); + fn diagnostics_display_range(&self, src: InFile) -> FileRange { let root = self.db.parse_or_expand(src.file_id).unwrap(); let node = src.value.to_node(&root); self.cache(root, src.file_id); diff --git a/crates/hir_def/src/body.rs b/crates/hir_def/src/body.rs index 9a432f7d12..ff4b4a0cf2 100644 --- a/crates/hir_def/src/body.rs +++ b/crates/hir_def/src/body.rs @@ -123,7 +123,7 @@ impl Expander { Some(it) => it, None => { if err.is_none() { - eprintln!("no error despite `as_call_id_with_errors` returning `None`"); + log::warn!("no error despite `as_call_id_with_errors` returning `None`"); } return ExpandResult { value: None, err }; } diff --git a/crates/hir_def/src/diagnostics.rs b/crates/hir_def/src/diagnostics.rs index ab3f059cef..ac7474f63f 100644 --- a/crates/hir_def/src/diagnostics.rs +++ b/crates/hir_def/src/diagnostics.rs @@ -95,6 +95,34 @@ impl Diagnostic for UnresolvedImport { } } +// Diagnostic: unresolved-macro-call +// +// This diagnostic is triggered if rust-analyzer is unable to resolove path to a +// macro in a macro invocation. +#[derive(Debug)] +pub struct UnresolvedMacroCall { + pub file: HirFileId, + pub node: AstPtr, +} + +impl Diagnostic for UnresolvedMacroCall { + fn code(&self) -> DiagnosticCode { + DiagnosticCode("unresolved-macro-call") + } + fn message(&self) -> String { + "unresolved macro call".to_string() + } + fn display_source(&self) -> InFile { + InFile::new(self.file, self.node.clone().into()) + } + fn as_any(&self) -> &(dyn Any + Send + 'static) { + self + } + fn is_experimental(&self) -> bool { + true + } +} + // Diagnostic: inactive-code // // This diagnostic is shown for code with inactive `#[cfg]` attributes. diff --git a/crates/hir_def/src/lib.rs b/crates/hir_def/src/lib.rs index b50923747c..6802bc250c 100644 --- a/crates/hir_def/src/lib.rs +++ b/crates/hir_def/src/lib.rs @@ -57,8 +57,10 @@ use std::{ use base_db::{impl_intern_key, salsa, CrateId}; use hir_expand::{ - ast_id_map::FileAstId, eager::expand_eager_macro, hygiene::Hygiene, AstId, HirFileId, InFile, - MacroCallId, MacroCallKind, MacroDefId, MacroDefKind, + ast_id_map::FileAstId, + eager::{expand_eager_macro, ErrorEmitted}, + hygiene::Hygiene, + AstId, HirFileId, InFile, MacroCallId, MacroCallKind, MacroDefId, MacroDefKind, }; use la_arena::Idx; use nameres::DefMap; @@ -592,8 +594,15 @@ impl AsMacroCall for InFile<&ast::MacroCall> { error_sink(mbe::ExpandError::Other("malformed macro invocation".into())); } - AstIdWithPath::new(ast_id.file_id, ast_id.value, path?) - .as_call_id_with_errors(db, krate, resolver, error_sink) + macro_call_as_call_id( + &AstIdWithPath::new(ast_id.file_id, ast_id.value, path?), + db, + krate, + resolver, + error_sink, + ) + .ok()? + .ok() } } @@ -610,61 +619,50 @@ impl AstIdWithPath { } } -impl AsMacroCall for AstIdWithPath { - fn as_call_id_with_errors( - &self, - db: &dyn db::DefDatabase, - krate: CrateId, - resolver: impl Fn(path::ModPath) -> Option, - error_sink: &mut dyn FnMut(mbe::ExpandError), - ) -> Option { - let def: MacroDefId = resolver(self.path.clone()).or_else(|| { - error_sink(mbe::ExpandError::Other(format!("could not resolve macro `{}`", self.path))); - None - })?; +struct UnresolvedMacro; - if let MacroDefKind::BuiltInEager(_) = def.kind { - let macro_call = InFile::new(self.ast_id.file_id, self.ast_id.to_node(db.upcast())); - let hygiene = Hygiene::new(db.upcast(), self.ast_id.file_id); +fn macro_call_as_call_id( + call: &AstIdWithPath, + db: &dyn db::DefDatabase, + krate: CrateId, + resolver: impl Fn(path::ModPath) -> Option, + error_sink: &mut dyn FnMut(mbe::ExpandError), +) -> Result, UnresolvedMacro> { + let def: MacroDefId = resolver(call.path.clone()).ok_or(UnresolvedMacro)?; - Some( - expand_eager_macro( - db.upcast(), - krate, - macro_call, - def, - &|path: ast::Path| resolver(path::ModPath::from_src(path, &hygiene)?), - error_sink, - ) - .ok()? - .into(), - ) - } else { - Some(def.as_lazy_macro(db.upcast(), krate, MacroCallKind::FnLike(self.ast_id)).into()) - } - } -} + 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())); + let hygiene = Hygiene::new(db.upcast(), call.ast_id.file_id); -impl AsMacroCall for AstIdWithPath { - fn as_call_id_with_errors( - &self, - db: &dyn db::DefDatabase, - krate: CrateId, - resolver: impl Fn(path::ModPath) -> Option, - error_sink: &mut dyn FnMut(mbe::ExpandError), - ) -> Option { - let def: MacroDefId = resolver(self.path.clone()).or_else(|| { - error_sink(mbe::ExpandError::Other(format!("could not resolve macro `{}`", self.path))); - None - })?; - - Some( - def.as_lazy_macro( - db.upcast(), - krate, - MacroCallKind::Attr(self.ast_id, self.path.segments().last()?.to_string()), - ) - .into(), + expand_eager_macro( + db.upcast(), + krate, + macro_call, + def, + &|path: ast::Path| resolver(path::ModPath::from_src(path, &hygiene)?), + error_sink, ) - } + .map(MacroCallId::from) + } else { + Ok(def.as_lazy_macro(db.upcast(), krate, MacroCallKind::FnLike(call.ast_id)).into()) + }; + Ok(res) +} + +fn item_attr_as_call_id( + item_attr: &AstIdWithPath, + db: &dyn db::DefDatabase, + krate: CrateId, + resolver: impl Fn(path::ModPath) -> Option, +) -> Result { + let def: MacroDefId = resolver(item_attr.path.clone()).ok_or(UnresolvedMacro)?; + let last_segment = item_attr.path.segments().last().ok_or(UnresolvedMacro)?; + let res = def + .as_lazy_macro( + db.upcast(), + krate, + MacroCallKind::Attr(item_attr.ast_id, last_segment.to_string()), + ) + .into(); + Ok(res) } diff --git a/crates/hir_def/src/nameres.rs b/crates/hir_def/src/nameres.rs index f92232eb32..6a3456f2e9 100644 --- a/crates/hir_def/src/nameres.rs +++ b/crates/hir_def/src/nameres.rs @@ -417,6 +417,8 @@ mod diagnostics { UnresolvedProcMacro { ast: MacroCallKind }, + UnresolvedMacroCall { ast: AstId }, + MacroError { ast: MacroCallKind, message: String }, } @@ -477,6 +479,13 @@ mod diagnostics { Self { in_module: container, kind: DiagnosticKind::MacroError { ast, message } } } + pub(super) fn unresolved_macro_call( + container: LocalModuleId, + ast: AstId, + ) -> Self { + Self { in_module: container, kind: DiagnosticKind::UnresolvedMacroCall { ast } } + } + pub(super) fn add_to( &self, db: &dyn DefDatabase, @@ -589,6 +598,11 @@ mod diagnostics { }); } + DiagnosticKind::UnresolvedMacroCall { ast } => { + let node = ast.to_node(db.upcast()); + sink.push(UnresolvedMacroCall { file: ast.file_id, node: AstPtr::new(&node) }); + } + DiagnosticKind::MacroError { ast, message } => { let (file, ast) = match ast { MacroCallKind::FnLike(ast) => { diff --git a/crates/hir_def/src/nameres/collector.rs b/crates/hir_def/src/nameres/collector.rs index 9996a08072..e51d89b436 100644 --- a/crates/hir_def/src/nameres/collector.rs +++ b/crates/hir_def/src/nameres/collector.rs @@ -13,7 +13,7 @@ use hir_expand::{ builtin_macro::find_builtin_macro, name::{AsName, Name}, proc_macro::ProcMacroExpander, - HirFileId, MacroCallId, MacroCallKind, MacroDefId, MacroDefKind, + HirFileId, MacroCallId, MacroDefId, MacroDefKind, }; use hir_expand::{InFile, MacroCallLoc}; use rustc_hash::{FxHashMap, FxHashSet}; @@ -24,11 +24,13 @@ use tt::{Leaf, TokenTree}; use crate::{ attr::Attrs, db::DefDatabase, + item_attr_as_call_id, item_scope::{ImportType, PerNsGlobImports}, item_tree::{ self, FileItemTreeId, ItemTree, ItemTreeId, MacroCall, MacroRules, Mod, ModItem, ModKind, StructDefKind, }, + macro_call_as_call_id, nameres::{ diagnostics::DefDiagnostic, mod_resolution::ModDir, path_resolution::ReachedFixedPoint, BuiltinShadowMode, DefMap, ModuleData, ModuleOrigin, ResolveMode, @@ -36,9 +38,9 @@ use crate::{ path::{ImportAlias, ModPath, PathKind}, per_ns::PerNs, visibility::{RawVisibility, Visibility}, - AdtId, AsMacroCall, AstId, AstIdWithPath, ConstLoc, ContainerId, EnumLoc, EnumVariantId, - FunctionLoc, ImplLoc, Intern, LocalModuleId, ModuleDefId, StaticLoc, StructLoc, TraitLoc, - TypeAliasLoc, UnionLoc, + AdtId, AstId, AstIdWithPath, ConstLoc, ContainerId, EnumLoc, EnumVariantId, FunctionLoc, + ImplLoc, Intern, LocalModuleId, ModuleDefId, StaticLoc, StructLoc, TraitLoc, TypeAliasLoc, + UnionLoc, UnresolvedMacro, }; const GLOB_RECURSION_LIMIT: usize = 100; @@ -790,8 +792,11 @@ impl DefCollector<'_> { return false; } - if let Some(call_id) = - directive.ast_id.as_call_id(self.db, self.def_map.krate, |path| { + match macro_call_as_call_id( + &directive.ast_id, + self.db, + self.def_map.krate, + |path| { let resolved_res = self.def_map.resolve_path_fp_with_macro( self.db, ResolveMode::Other, @@ -800,24 +805,29 @@ impl DefCollector<'_> { BuiltinShadowMode::Module, ); resolved_res.resolved_def.take_macros() - }) - { - resolved.push((directive.module_id, call_id, directive.depth)); - res = ReachedFixedPoint::No; - return false; + }, + &mut |_err| (), + ) { + Ok(Ok(call_id)) => { + resolved.push((directive.module_id, call_id, directive.depth)); + res = ReachedFixedPoint::No; + return false; + } + Err(UnresolvedMacro) | Ok(Err(_)) => {} } true }); attribute_macros.retain(|directive| { - if let Some(call_id) = - directive.ast_id.as_call_id(self.db, self.def_map.krate, |path| { - self.resolve_attribute_macro(&directive, &path) - }) - { - resolved.push((directive.module_id, call_id, 0)); - res = ReachedFixedPoint::No; - return false; + match item_attr_as_call_id(&directive.ast_id, self.db, self.def_map.krate, |path| { + self.resolve_attribute_macro(&directive, &path) + }) { + Ok(call_id) => { + resolved.push((directive.module_id, call_id, 0)); + res = ReachedFixedPoint::No; + return false; + } + Err(UnresolvedMacro) => (), } true @@ -902,7 +912,8 @@ impl DefCollector<'_> { for directive in &self.unexpanded_macros { let mut error = None; - directive.ast_id.as_call_id_with_errors( + match macro_call_as_call_id( + &directive.ast_id, self.db, self.def_map.krate, |path| { @@ -918,15 +929,15 @@ impl DefCollector<'_> { &mut |e| { error.get_or_insert(e); }, - ); - - if let Some(err) = error { - self.def_map.diagnostics.push(DefDiagnostic::macro_error( - directive.module_id, - MacroCallKind::FnLike(directive.ast_id.ast_id), - err.to_string(), - )); - } + ) { + Ok(_) => (), + Err(UnresolvedMacro) => { + self.def_map.diagnostics.push(DefDiagnostic::unresolved_macro_call( + directive.module_id, + directive.ast_id.ast_id, + )); + } + }; } // Emit diagnostics for all remaining unresolved imports. @@ -1446,8 +1457,11 @@ impl ModCollector<'_, '_> { let mut ast_id = AstIdWithPath::new(self.file_id, mac.ast_id, mac.path.clone()); // Case 1: try to resolve in legacy scope and expand macro_rules - if let Some(macro_call_id) = - ast_id.as_call_id(self.def_collector.db, self.def_collector.def_map.krate, |path| { + if let Ok(Ok(macro_call_id)) = macro_call_as_call_id( + &ast_id, + self.def_collector.db, + self.def_collector.def_map.krate, + |path| { path.as_ident().and_then(|name| { self.def_collector.def_map.with_ancestor_maps( self.def_collector.db, @@ -1455,8 +1469,9 @@ impl ModCollector<'_, '_> { &mut |map, module| map[module].scope.get_legacy_macro(&name), ) }) - }) - { + }, + &mut |_err| (), + ) { self.def_collector.unexpanded_macros.push(MacroDirective { module_id: self.module_id, ast_id, diff --git a/crates/ide/src/diagnostics.rs b/crates/ide/src/diagnostics.rs index 8607139ba3..fe32f39b64 100644 --- a/crates/ide/src/diagnostics.rs +++ b/crates/ide/src/diagnostics.rs @@ -10,15 +10,16 @@ mod field_shorthand; use std::cell::RefCell; use hir::{ + db::AstDatabase, diagnostics::{Diagnostic as _, DiagnosticCode, DiagnosticSinkBuilder}, - Semantics, + InFile, Semantics, }; use ide_db::{base_db::SourceDatabase, RootDatabase}; use itertools::Itertools; use rustc_hash::FxHashSet; use syntax::{ ast::{self, AstNode}, - SyntaxNode, TextRange, + SyntaxNode, SyntaxNodePtr, TextRange, }; use text_edit::TextEdit; @@ -147,20 +148,38 @@ pub(crate) fn diagnostics( // Override severity and mark as unused. res.borrow_mut().push( - Diagnostic::hint(sema.diagnostics_display_range(d).range, d.message()) - .with_unused(true) - .with_code(Some(d.code())), + Diagnostic::hint( + sema.diagnostics_display_range(d.display_source()).range, + d.message(), + ) + .with_unused(true) + .with_code(Some(d.code())), ); }) .on::(|d| { // Use more accurate position if available. - let display_range = - d.precise_location.unwrap_or_else(|| sema.diagnostics_display_range(d).range); + let display_range = d + .precise_location + .unwrap_or_else(|| sema.diagnostics_display_range(d.display_source()).range); // FIXME: it would be nice to tell the user whether proc macros are currently disabled res.borrow_mut() .push(Diagnostic::hint(display_range, d.message()).with_code(Some(d.code()))); }) + .on::(|d| { + let last_path_segment = sema.db.parse_or_expand(d.file).and_then(|root| { + d.node + .to_node(&root) + .path() + .and_then(|it| it.segment()) + .and_then(|it| it.name_ref()) + .map(|it| InFile::new(d.file, SyntaxNodePtr::new(it.syntax()))) + }); + let diagnostics = last_path_segment.unwrap_or_else(|| d.display_source()); + let display_range = sema.diagnostics_display_range(diagnostics).range; + res.borrow_mut() + .push(Diagnostic::error(display_range, d.message()).with_code(Some(d.code()))); + }) // Only collect experimental diagnostics when they're enabled. .filter(|diag| !(diag.is_experimental() && config.disable_experimental)) .filter(|diag| !config.disabled.contains(diag.code().as_str())); @@ -170,8 +189,11 @@ pub(crate) fn diagnostics( // Diagnostics not handled above get no fix and default treatment. .build(|d| { res.borrow_mut().push( - Diagnostic::error(sema.diagnostics_display_range(d).range, d.message()) - .with_code(Some(d.code())), + Diagnostic::error( + sema.diagnostics_display_range(d.display_source()).range, + d.message(), + ) + .with_code(Some(d.code())), ); }); @@ -183,13 +205,13 @@ pub(crate) fn diagnostics( } fn diagnostic_with_fix(d: &D, sema: &Semantics) -> Diagnostic { - Diagnostic::error(sema.diagnostics_display_range(d).range, d.message()) + Diagnostic::error(sema.diagnostics_display_range(d.display_source()).range, d.message()) .with_fix(d.fix(&sema)) .with_code(Some(d.code())) } fn warning_with_fix(d: &D, sema: &Semantics) -> Diagnostic { - Diagnostic::hint(sema.diagnostics_display_range(d).range, d.message()) + Diagnostic::hint(sema.diagnostics_display_range(d.display_source()).range, d.message()) .with_fix(d.fix(&sema)) .with_code(Some(d.code())) } @@ -645,6 +667,29 @@ fn test_fn() { ); } + #[test] + fn test_unresolved_macro_range() { + check_expect( + r#"foo::bar!(92);"#, + expect![[r#" + [ + Diagnostic { + message: "unresolved macro call", + range: 5..8, + severity: Error, + fix: None, + unused: false, + code: Some( + DiagnosticCode( + "unresolved-macro-call", + ), + ), + }, + ] + "#]], + ); + } + #[test] fn range_mapping_out_of_macros() { // FIXME: this is very wrong, but somewhat tricky to fix.