Merge #7805

7805: For unresolved macros, hightlight only the last segment r=matklad a=matklad

bors r+
🤖

Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

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,
 };

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<SyntaxNodePtr>) -> 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<SyntaxNodePtr>) -> 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);

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 };
             }

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<ast::MacroCall>,
+}
+
+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<SyntaxNodePtr> {
+        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.

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<T: ast::AstNode> AstIdWithPath<T> {
     }
 }
 
-impl AsMacroCall for AstIdWithPath<ast::MacroCall> {
-    fn as_call_id_with_errors(
-        &self,
-        db: &dyn db::DefDatabase,
-        krate: CrateId,
-        resolver: impl Fn(path::ModPath) -> Option<MacroDefId>,
-        error_sink: &mut dyn FnMut(mbe::ExpandError),
-    ) -> Option<MacroCallId> {
-        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<ast::MacroCall>,
+    db: &dyn db::DefDatabase,
+    krate: CrateId,
+    resolver: impl Fn(path::ModPath) -> Option<MacroDefId>,
+    error_sink: &mut dyn FnMut(mbe::ExpandError),
+) -> Result<Result<MacroCallId, ErrorEmitted>, 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<ast::Item> {
-    fn as_call_id_with_errors(
-        &self,
-        db: &dyn db::DefDatabase,
-        krate: CrateId,
-        resolver: impl Fn(path::ModPath) -> Option<MacroDefId>,
-        error_sink: &mut dyn FnMut(mbe::ExpandError),
-    ) -> Option<MacroCallId> {
-        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<ast::Item>,
+    db: &dyn db::DefDatabase,
+    krate: CrateId,
+    resolver: impl Fn(path::ModPath) -> Option<MacroDefId>,
+) -> Result<MacroCallId, UnresolvedMacro> {
+    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)
 }

crates/hir_def/src/nameres.rs

@@ -417,6 +417,8 @@ mod diagnostics {
 
         UnresolvedProcMacro { ast: MacroCallKind },
 
+        UnresolvedMacroCall { ast: AstId<ast::MacroCall> },
+
         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<ast::MacroCall>,
+        ) -> 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) => {

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,

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::<hir::diagnostics::UnresolvedProcMacro, _>(|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::<hir::diagnostics::UnresolvedMacroCall, _>(|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: DiagnosticWithFix>(d: &D, sema: &Semantics<RootDatabase>) -> 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: DiagnosticWithFix>(d: &D, sema: &Semantics<RootDatabase>) -> 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.