Merge pull request #19731 from Veykril/push-mmvowomkpwxy

refactor: Simplify macro call id construction

Cargo.toml

@@ -131,6 +131,7 @@ process-wrap = { version = "8.2.0", features = ["std"] }
 pulldown-cmark-to-cmark = "10.0.4"
 pulldown-cmark = { version = "0.9.6", default-features = false }
 rayon = "1.10.0"
+rowan = "=0.15.15"
 salsa = { version = "0.21.1", default-features = false, features = ["rayon","salsa_unstable"] }
 salsa-macros = "0.21.1"
 semver = "1.0.26"

crates/hir-def/src/expr_store/expander.rs

@@ -5,20 +5,22 @@ use std::mem;
 use base_db::Crate;
 use cfg::CfgOptions;
 use drop_bomb::DropBomb;
+use hir_expand::AstId;
 use hir_expand::{
     ExpandError, ExpandErrorKind, ExpandResult, HirFileId, InFile, Lookup, MacroCallId,
     eager::EagerCallBackFn, mod_path::ModPath, span_map::SpanMap,
 };
 use span::{AstIdMap, Edition, SyntaxContext};
 use syntax::ast::HasAttrs;
-use syntax::{Parse, ast};
+use syntax::{AstNode, Parse, ast};
 use triomphe::Arc;
 use tt::TextRange;
 
 use crate::attr::Attrs;
 use crate::expr_store::HygieneId;
+use crate::macro_call_as_call_id;
 use crate::nameres::DefMap;
-use crate::{AsMacroCall, MacroId, UnresolvedMacro, db::DefDatabase};
+use crate::{MacroId, UnresolvedMacro, db::DefDatabase};
 
 #[derive(Debug)]
 pub(super) struct Expander {
@@ -92,8 +94,31 @@ impl Expander {
 
         let result = self.within_limit(db, |this| {
             let macro_call = this.in_file(&macro_call);
-            match macro_call.as_call_id_with_errors(
+
+            let expands_to = hir_expand::ExpandTo::from_call_site(macro_call.value);
+            let ast_id = AstId::new(macro_call.file_id, this.ast_id_map().ast_id(macro_call.value));
+            let path = macro_call.value.path().and_then(|path| {
+                let range = path.syntax().text_range();
+                let mod_path = ModPath::from_src(db, path, &mut |range| {
+                    this.span_map.span_for_range(range).ctx
+                })?;
+                let call_site = this.span_map.span_for_range(range);
+                Some((call_site, mod_path))
+            });
+
+            let Some((call_site, path)) = path else {
+                return ExpandResult::only_err(ExpandError::other(
+                    this.span_map.span_for_range(macro_call.value.syntax().text_range()),
+                    "malformed macro invocation",
+                ));
+            };
+
+            match macro_call_as_call_id(
                 db,
+                ast_id,
+                &path,
+                call_site.ctx,
+                expands_to,
                 krate,
                 |path| resolver(path).map(|it| db.macro_def(it)),
                 eager_callback,

crates/hir-def/src/lib.rs

@@ -64,8 +64,8 @@ use std::hash::{Hash, Hasher};
 
 use base_db::{Crate, impl_intern_key};
 use hir_expand::{
-    AstId, ExpandError, ExpandResult, ExpandTo, HirFileId, InFile, MacroCallId, MacroCallKind,
+    AstId, ExpandResult, ExpandTo, HirFileId, InFile, MacroCallId, MacroCallKind, MacroDefId,
-    MacroDefId, MacroDefKind,
+    MacroDefKind,
     builtin::{BuiltinAttrExpander, BuiltinDeriveExpander, BuiltinFnLikeExpander, EagerExpander},
     db::ExpandDatabase,
     eager::expand_eager_macro_input,
@@ -79,7 +79,7 @@ use la_arena::Idx;
 use nameres::DefMap;
 use span::{AstIdNode, Edition, FileAstId, SyntaxContext};
 use stdx::impl_from;
-use syntax::{AstNode, ast};
+use syntax::ast;
 
 pub use hir_expand::{Intern, Lookup, tt};
 
@@ -1166,66 +1166,6 @@ impl ModuleDefId {
         })
     }
 }
-
-// FIXME: Replace this with a plain function, it only has one impl
-/// A helper trait for converting to MacroCallId
-trait AsMacroCall {
-    fn as_call_id_with_errors(
-        &self,
-        db: &dyn ExpandDatabase,
-        krate: Crate,
-        resolver: impl Fn(&ModPath) -> Option<MacroDefId> + Copy,
-        eager_callback: &mut dyn FnMut(
-            InFile<(syntax::AstPtr<ast::MacroCall>, span::FileAstId<ast::MacroCall>)>,
-            MacroCallId,
-        ),
-    ) -> Result<ExpandResult<Option<MacroCallId>>, UnresolvedMacro>;
-}
-
-impl AsMacroCall for InFile<&ast::MacroCall> {
-    fn as_call_id_with_errors(
-        &self,
-        db: &dyn ExpandDatabase,
-        krate: Crate,
-        resolver: impl Fn(&ModPath) -> Option<MacroDefId> + Copy,
-        eager_callback: &mut dyn FnMut(
-            InFile<(syntax::AstPtr<ast::MacroCall>, span::FileAstId<ast::MacroCall>)>,
-            MacroCallId,
-        ),
-    ) -> Result<ExpandResult<Option<MacroCallId>>, UnresolvedMacro> {
-        let expands_to = hir_expand::ExpandTo::from_call_site(self.value);
-        let ast_id = AstId::new(self.file_id, db.ast_id_map(self.file_id).ast_id(self.value));
-        let span_map = db.span_map(self.file_id);
-        let path = self.value.path().and_then(|path| {
-            let range = path.syntax().text_range();
-            let mod_path = ModPath::from_src(db, path, &mut |range| {
-                span_map.as_ref().span_for_range(range).ctx
-            })?;
-            let call_site = span_map.span_for_range(range);
-            Some((call_site, mod_path))
-        });
-
-        let Some((call_site, path)) = path else {
-            return Ok(ExpandResult::only_err(ExpandError::other(
-                span_map.span_for_range(self.value.syntax().text_range()),
-                "malformed macro invocation",
-            )));
-        };
-
-        macro_call_as_call_id_with_eager(
-            db,
-            ast_id,
-            &path,
-            call_site.ctx,
-            expands_to,
-            krate,
-            resolver,
-            resolver,
-            eager_callback,
-        )
-    }
-}
-
 /// Helper wrapper for `AstId` with `ModPath`
 #[derive(Clone, Debug, Eq, PartialEq)]
 struct AstIdWithPath<T: AstIdNode> {
@@ -1239,41 +1179,14 @@ impl<T: AstIdNode> AstIdWithPath<T> {
     }
 }
 
-fn macro_call_as_call_id(
+pub fn macro_call_as_call_id(
-    db: &dyn ExpandDatabase,
-    call: &AstIdWithPath<ast::MacroCall>,
-    call_site: SyntaxContext,
-    expand_to: ExpandTo,
-    krate: Crate,
-    resolver: impl Fn(&ModPath) -> Option<MacroDefId> + Copy,
-    eager_callback: &mut dyn FnMut(
-        InFile<(syntax::AstPtr<ast::MacroCall>, span::FileAstId<ast::MacroCall>)>,
-        MacroCallId,
-    ),
-) -> Result<Option<MacroCallId>, UnresolvedMacro> {
-    macro_call_as_call_id_with_eager(
-        db,
-        call.ast_id,
-        &call.path,
-        call_site,
-        expand_to,
-        krate,
-        resolver,
-        resolver,
-        eager_callback,
-    )
-    .map(|res| res.value)
-}
-
-fn macro_call_as_call_id_with_eager(
     db: &dyn ExpandDatabase,
     ast_id: AstId<ast::MacroCall>,
     path: &ModPath,
     call_site: SyntaxContext,
     expand_to: ExpandTo,
     krate: Crate,
-    resolver: impl FnOnce(&ModPath) -> Option<MacroDefId>,
+    resolver: impl Fn(&ModPath) -> Option<MacroDefId> + Copy,
-    eager_resolver: impl Fn(&ModPath) -> Option<MacroDefId>,
     eager_callback: &mut dyn FnMut(
         InFile<(syntax::AstPtr<ast::MacroCall>, span::FileAstId<ast::MacroCall>)>,
         MacroCallId,
@@ -1289,7 +1202,7 @@ fn macro_call_as_call_id_with_eager(
             ast_id,
             def,
             call_site,
-            &|path| eager_resolver(path).filter(MacroDefId::is_fn_like),
+            &|path| resolver(path).filter(MacroDefId::is_fn_like),
             eager_callback,
         ),
         _ if def.is_fn_like() => ExpandResult {

crates/hir-def/src/macro_expansion_tests/mbe.rs

@@ -2001,8 +2001,9 @@ macro_rules! bug {
         true
     };
 }
-
+fn f() {
-let _ = bug!(a;;;test);
+    let _ = bug!(a;;;test);
+}
     "#,
         expect![[r#"
 macro_rules! bug {
@@ -2022,8 +2023,9 @@ macro_rules! bug {
         true
     };
 }
-
+fn f() {
-let _ = true;
+    let _ = true;
+}
     "#]],
     );
 }

crates/hir-def/src/macro_expansion_tests/mod.rs

@@ -19,7 +19,7 @@ use std::{iter, ops::Range, sync};
 use base_db::RootQueryDb;
 use expect_test::Expect;
 use hir_expand::{
-    InFile, MacroCallKind, MacroKind,
+    AstId, InFile, MacroCallId, MacroCallKind, MacroKind,
     db::ExpandDatabase,
     proc_macro::{ProcMacro, ProcMacroExpander, ProcMacroExpansionError, ProcMacroKind},
     span_map::SpanMapRef,
@@ -29,7 +29,7 @@ use itertools::Itertools;
 use span::{Edition, Span};
 use stdx::{format_to, format_to_acc};
 use syntax::{
-    AstNode,
+    AstNode, AstPtr,
     SyntaxKind::{COMMENT, EOF, IDENT, LIFETIME_IDENT},
     SyntaxNode, T,
     ast::{self, edit::IndentLevel},
@@ -37,10 +37,9 @@ use syntax::{
 use test_fixture::WithFixture;
 
 use crate::{
-    AdtId, AsMacroCall, Lookup, ModuleDefId,
+    AdtId, Lookup, ModuleDefId,
     db::DefDatabase,
-    nameres::{DefMap, MacroSubNs, ModuleSource},
+    nameres::{DefMap, ModuleSource},
-    resolver::HasResolver,
     src::HasSource,
     test_db::TestDB,
     tt::TopSubtree,
@@ -78,7 +77,6 @@ fn check_errors(#[rust_analyzer::rust_fixture] ra_fixture: &str, expect: Expect)
     expect.assert_eq(&errors);
 }
 
-#[track_caller]
 fn check(#[rust_analyzer::rust_fixture] ra_fixture: &str, mut expect: Expect) {
     let extra_proc_macros = vec![(
         r#"
@@ -95,54 +93,59 @@ pub fn identity_when_valid(_attr: TokenStream, item: TokenStream) -> TokenStream
             disabled: false,
         },
     )];
+
+    fn resolve(
+        db: &dyn DefDatabase,
+        def_map: &DefMap,
+        ast_id: AstId<ast::MacroCall>,
+        ast_ptr: InFile<AstPtr<ast::MacroCall>>,
+    ) -> Option<MacroCallId> {
+        def_map.modules().find_map(|module| {
+            for decl in
+                module.1.scope.declarations().chain(module.1.scope.unnamed_consts().map(Into::into))
+            {
+                let body = match decl {
+                    ModuleDefId::FunctionId(it) => it.into(),
+                    ModuleDefId::ConstId(it) => it.into(),
+                    ModuleDefId::StaticId(it) => it.into(),
+                    _ => continue,
+                };
+
+                let (body, sm) = db.body_with_source_map(body);
+                if let Some(it) =
+                    body.blocks(db).find_map(|block| resolve(db, &block.1, ast_id, ast_ptr))
+                {
+                    return Some(it);
+                }
+                if let Some((_, res)) = sm.macro_calls().find(|it| it.0 == ast_ptr) {
+                    return Some(res);
+                }
+            }
+            module.1.scope.macro_invoc(ast_id)
+        })
+    }
+
     let db = TestDB::with_files_extra_proc_macros(ra_fixture, extra_proc_macros);
     let krate = db.fetch_test_crate();
     let def_map = db.crate_def_map(krate);
     let local_id = DefMap::ROOT;
-    let module = def_map.module_id(local_id);
-    let resolver = module.resolver(&db);
     let source = def_map[local_id].definition_source(&db);
     let source_file = match source.value {
         ModuleSource::SourceFile(it) => it,
         ModuleSource::Module(_) | ModuleSource::BlockExpr(_) => panic!(),
     };
 
-    // What we want to do is to replace all macros (fn-like, derive, attr) with
-    // their expansions. Turns out, we don't actually store enough information
-    // to do this precisely though! Specifically, if a macro expands to nothing,
-    // it leaves zero traces in def-map, so we can't get its expansion after the
-    // fact.
-    //
-    // This is the usual
-    // <https://github.com/rust-lang/rust-analyzer/issues/3407>
-    // resolve/record tension!
-    //
-    // So here we try to do a resolve, which is necessary a heuristic. For macro
-    // calls, we use `as_call_id_with_errors`. For derives, we look at the impls
-    // in the module and assume that, if impls's source is a different
-    // `HirFileId`, than it came from macro expansion.
-
     let mut text_edits = Vec::new();
     let mut expansions = Vec::new();
 
-    for macro_call in source_file.syntax().descendants().filter_map(ast::MacroCall::cast) {
+    for macro_call_node in source_file.syntax().descendants().filter_map(ast::MacroCall::cast) {
-        let macro_call = InFile::new(source.file_id, &macro_call);
+        let ast_id = db.ast_id_map(source.file_id).ast_id(&macro_call_node);
-        let res = macro_call
+        let ast_id = InFile::new(source.file_id, ast_id);
-            .as_call_id_with_errors(
+        let ptr = InFile::new(source.file_id, AstPtr::new(&macro_call_node));
-                &db,
+        let macro_call_id = resolve(&db, &def_map, ast_id, ptr)
-                krate,
+            .unwrap_or_else(|| panic!("unable to find semantic macro call {macro_call_node}"));
-                |path| {
+        let expansion_result = db.parse_macro_expansion(macro_call_id);
-                    resolver
+        expansions.push((macro_call_node.clone(), expansion_result));
-                        .resolve_path_as_macro(&db, path, Some(MacroSubNs::Bang))
-                        .map(|(it, _)| db.macro_def(it))
-                },
-                &mut |_, _| (),
-            )
-            .unwrap();
-        let macro_call_id = res.value.unwrap();
-        let mut expansion_result = db.parse_macro_expansion(macro_call_id);
-        expansion_result.err = expansion_result.err.or(res.err);
-        expansions.push((macro_call.value.clone(), expansion_result));
     }
 
     for (call, exp) in expansions.into_iter().rev() {

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

@@ -259,7 +259,8 @@ impl<'a> AssocItemCollector<'a> {
                 };
                 match macro_call_as_call_id(
                     self.db,
-                    &AstIdWithPath::new(tree_id.file_id(), ast_id, Clone::clone(path)),
+                    InFile::new(tree_id.file_id(), ast_id),
+                    path,
                     ctxt,
                     expand_to,
                     self.module_id.krate(),
@@ -268,12 +269,15 @@ impl<'a> AssocItemCollector<'a> {
                         self.macro_calls.push((ptr.map(|(_, it)| it.upcast()), call_id))
                     },
                 ) {
-                    Ok(Some(call_id)) => {
+                    // FIXME: Expansion error?
-                        self.macro_calls
+                    Ok(call_id) => match call_id.value {
-                            .push((InFile::new(tree_id.file_id(), ast_id.upcast()), call_id));
+                        Some(call_id) => {
-                        self.collect_macro_items(call_id);
+                            self.macro_calls
-                    }
+                                .push((InFile::new(tree_id.file_id(), ast_id.upcast()), call_id));
-                    Ok(None) => (),
+                            self.collect_macro_items(call_id);
+                        }
+                        None => (),
+                    },
                     Err(_) => {
                         self.diagnostics.push(DefDiagnostic::unresolved_macro_call(
                             self.module_id.local_id,

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

@@ -39,7 +39,7 @@ use crate::{
         ItemTreeId, ItemTreeNode, Macro2, MacroCall, MacroRules, Mod, ModItem, ModKind, TreeId,
         UseTreeKind,
     },
-    macro_call_as_call_id, macro_call_as_call_id_with_eager,
+    macro_call_as_call_id,
     nameres::{
         BuiltinShadowMode, DefMap, LocalDefMap, MacroSubNs, ModuleData, ModuleOrigin, ResolveMode,
         attr_resolution::{attr_macro_as_call_id, derive_macro_as_call_id},
@@ -1256,7 +1256,8 @@ impl DefCollector<'_> {
                 MacroDirectiveKind::FnLike { ast_id, expand_to, ctxt: call_site } => {
                     let call_id = macro_call_as_call_id(
                         self.db,
-                        ast_id,
+                        ast_id.ast_id,
+                        &ast_id.path,
                         *call_site,
                         *expand_to,
                         self.def_map.krate,
@@ -1265,15 +1266,18 @@ impl DefCollector<'_> {
                             eager_callback_buffer.push((directive.module_id, ptr, call_id));
                         },
                     );
-                    if let Ok(Some(call_id)) = call_id {
+                    if let Ok(call_id) = call_id {
-                        self.def_map.modules[directive.module_id]
+                        // FIXME: Expansion error
-                            .scope
+                        if let Some(call_id) = call_id.value {
-                            .add_macro_invoc(ast_id.ast_id, call_id);
+                            self.def_map.modules[directive.module_id]
+                                .scope
+                                .add_macro_invoc(ast_id.ast_id, call_id);
 
-                        push_resolved(directive, call_id);
+                            push_resolved(directive, call_id);
 
-                        res = ReachedFixedPoint::No;
+                            res = ReachedFixedPoint::No;
-                        return Resolved::Yes;
+                            return Resolved::Yes;
+                        }
                     }
                 }
                 MacroDirectiveKind::Derive {
@@ -1542,7 +1546,8 @@ impl DefCollector<'_> {
                     // FIXME: we shouldn't need to re-resolve the macro here just to get the unresolved error!
                     let macro_call_as_call_id = macro_call_as_call_id(
                         self.db,
-                        ast_id,
+                        ast_id.ast_id,
+                        &ast_id.path,
                         *call_site,
                         *expand_to,
                         self.def_map.krate,
@@ -2420,7 +2425,7 @@ impl ModCollector<'_, '_> {
 
         let mut eager_callback_buffer = vec![];
         // Case 1: try to resolve macro calls with single-segment name and expand macro_rules
-        if let Ok(res) = macro_call_as_call_id_with_eager(
+        if let Ok(res) = macro_call_as_call_id(
             db,
             ast_id.ast_id,
             &ast_id.path,
@@ -2445,21 +2450,6 @@ impl ModCollector<'_, '_> {
                         .map(|it| self.def_collector.db.macro_def(it))
                 })
             },
-            |path| {
-                let resolved_res = self.def_collector.def_map.resolve_path_fp_with_macro(
-                    self.def_collector
-                        .crate_local_def_map
-                        .as_deref()
-                        .unwrap_or(&self.def_collector.local_def_map),
-                    db,
-                    ResolveMode::Other,
-                    self.module_id,
-                    path,
-                    BuiltinShadowMode::Module,
-                    Some(MacroSubNs::Bang),
-                );
-                resolved_res.resolved_def.take_macros().map(|it| db.macro_def(it))
-            },
             &mut |ptr, call_id| eager_callback_buffer.push((ptr, call_id)),
         ) {
             for (ptr, call_id) in eager_callback_buffer {

crates/syntax/Cargo.toml

@@ -14,7 +14,7 @@ rust-version.workspace = true
 [dependencies]
 either.workspace = true
 itertools.workspace = true
-rowan = "=0.15.15"
+rowan.workspace = true
 rustc-hash.workspace = true
 rustc-literal-escaper.workspace = true
 smol_str.workspace = true