Skip to content

refactor: Simplify macro call id construction #19731

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 2, 2025
Merged

refactor: Simplify macro call id construction #19731

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we bumped rowan?

Copy link
Member Author

@Veykril Veykril May 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No we did not, in fact we can't given the new assertion within rowan that will fail on our code base still

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I thought we had fixed that. My bad.

salsa = { version = "0.21.1", default-features = false, features = ["rayon","salsa_unstable"] }
salsa-macros = "0.21.1"
semver = "1.0.26"
Expand Down
31 changes: 28 additions & 3 deletions crates/hir-def/src/expr_store/expander.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down
99 changes: 6 additions & 93 deletions crates/hir-def/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
MacroDefId, MacroDefKind,
AstId, ExpandResult, ExpandTo, HirFileId, InFile, MacroCallId, MacroCallKind, MacroDefId,
MacroDefKind,
builtin::{BuiltinAttrExpander, BuiltinDeriveExpander, BuiltinFnLikeExpander, EagerExpander},
db::ExpandDatabase,
eager::expand_eager_macro_input,
Expand All @@ -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};

Expand Down Expand Up @@ -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> {
Expand All @@ -1239,41 +1179,14 @@ impl<T: AstIdNode> AstIdWithPath<T> {
}
}

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(
pub fn macro_call_as_call_id(
db: &dyn ExpandDatabase,
ast_id: AstId<ast::MacroCall>,
path: &ModPath,
call_site: SyntaxContext,
expand_to: ExpandTo,
krate: Crate,
resolver: impl FnOnce(&ModPath) -> Option<MacroDefId>,
eager_resolver: impl Fn(&ModPath) -> Option<MacroDefId>,
resolver: impl Fn(&ModPath) -> Option<MacroDefId> + Copy,
eager_callback: &mut dyn FnMut(
InFile<(syntax::AstPtr<ast::MacroCall>, span::FileAstId<ast::MacroCall>)>,
MacroCallId,
Expand All @@ -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 {
Expand Down
10 changes: 6 additions & 4 deletions crates/hir-def/src/macro_expansion_tests/mbe.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2001,8 +2001,9 @@ macro_rules! bug {
true
};
}

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

let _ = true;
fn f() {
let _ = true;
}
"#]],
);
}
85 changes: 44 additions & 41 deletions crates/hir-def/src/macro_expansion_tests/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -29,18 +29,17 @@ 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},
};
use test_fixture::WithFixture;

use crate::{
AdtId, AsMacroCall, Lookup, ModuleDefId,
AdtId, Lookup, ModuleDefId,
db::DefDatabase,
nameres::{DefMap, MacroSubNs, ModuleSource},
resolver::HasResolver,
nameres::{DefMap, ModuleSource},
src::HasSource,
test_db::TestDB,
tt::TopSubtree,
Expand Down Expand Up @@ -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#"
Expand All @@ -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) {
let macro_call = InFile::new(source.file_id, &macro_call);
let res = macro_call
.as_call_id_with_errors(
&db,
krate,
|path| {
resolver
.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 macro_call_node in source_file.syntax().descendants().filter_map(ast::MacroCall::cast) {
let ast_id = db.ast_id_map(source.file_id).ast_id(&macro_call_node);
let ast_id = InFile::new(source.file_id, ast_id);
let ptr = InFile::new(source.file_id, AstPtr::new(&macro_call_node));
let macro_call_id = resolve(&db, &def_map, ast_id, ptr)
.unwrap_or_else(|| panic!("unable to find semantic macro call {macro_call_node}"));
let expansion_result = db.parse_macro_expansion(macro_call_id);
expansions.push((macro_call_node.clone(), expansion_result));
}

for (call, exp) in expansions.into_iter().rev() {
Expand Down
18 changes: 11 additions & 7 deletions crates/hir-def/src/nameres/assoc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand All @@ -268,12 +269,15 @@ impl<'a> AssocItemCollector<'a> {
self.macro_calls.push((ptr.map(|(_, it)| it.upcast()), call_id))
},
) {
Ok(Some(call_id)) => {
self.macro_calls
.push((InFile::new(tree_id.file_id(), ast_id.upcast()), call_id));
self.collect_macro_items(call_id);
}
Ok(None) => (),
// FIXME: Expansion error?
Ok(call_id) => match call_id.value {
Some(call_id) => {
self.macro_calls
.push((InFile::new(tree_id.file_id(), ast_id.upcast()), call_id));
self.collect_macro_items(call_id);
}
None => (),
},
Err(_) => {
self.diagnostics.push(DefDiagnostic::unresolved_macro_call(
self.module_id.local_id,
Expand Down
Loading