From c7b34e4873183d052f277ee7dda1f9927b66e154 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Mon, 31 Jul 2023 17:12:17 +0200 Subject: [PATCH 1/4] fix: Strip unused token ids from eager macro input token maps --- crates/hir-expand/src/eager.rs | 9 ++++++--- .../test_data/highlight_macros.html | 14 ++++++++++++-- crates/ide/src/syntax_highlighting/tests.rs | 15 ++++++++++++++- crates/mbe/src/token_map.rs | 4 ++++ crates/tt/src/lib.rs | 2 +- 5 files changed, 37 insertions(+), 7 deletions(-) diff --git a/crates/hir-expand/src/eager.rs b/crates/hir-expand/src/eager.rs index 91aa49d1e2..3ca2e295d7 100644 --- a/crates/hir-expand/src/eager.rs +++ b/crates/hir-expand/src/eager.rs @@ -19,7 +19,7 @@ //! //! See the full discussion : use base_db::CrateId; -use rustc_hash::FxHashMap; +use rustc_hash::{FxHashMap, FxHashSet}; use syntax::{ted, Parse, SyntaxNode, TextRange, TextSize, WalkEvent}; use triomphe::Arc; @@ -83,10 +83,11 @@ pub fn expand_eager_macro_input( mbe::syntax_node_to_token_tree(&expanded_eager_input); let og_tmap = if let Some(tt) = macro_call.value.token_tree() { - let og_tmap = mbe::syntax_node_to_token_map(tt.syntax()); + let mut ids_used = FxHashSet::default(); + let mut og_tmap = mbe::syntax_node_to_token_map(tt.syntax()); // The tokenmap and ids of subtree point into the expanded syntax node, but that is inaccessible from the outside // so we need to remap them to the original input of the eager macro. - subtree.visit_ids(&|id| { + subtree.visit_ids(&mut |id| { // Note: we discard all token ids of braces and the like here, but that's not too bad and only a temporary fix if let Some(range) = expanded_eager_input_token_map @@ -97,6 +98,7 @@ pub fn expand_eager_macro_input( // remap from eager input expansion to original eager input if let Some(&og_range) = ws_mapping.get(og_range) { if let Some(og_token) = og_tmap.token_by_range(og_range) { + ids_used.insert(id); return og_token; } } @@ -104,6 +106,7 @@ pub fn expand_eager_macro_input( } tt::TokenId::UNSPECIFIED }); + og_tmap.filter(|id| ids_used.contains(&id)); og_tmap } else { Default::default() diff --git a/crates/ide/src/syntax_highlighting/test_data/highlight_macros.html b/crates/ide/src/syntax_highlighting/test_data/highlight_macros.html index c5fcec7568..06b66b302a 100644 --- a/crates/ide/src/syntax_highlighting/test_data/highlight_macros.html +++ b/crates/ide/src/syntax_highlighting/test_data/highlight_macros.html @@ -90,8 +90,18 @@ pre { color: #DCDCCC; background: #3F3F3F; font-size: 22px; padd } } +#[rustc_builtin_macro] +macro_rules! concat {} +#[rustc_builtin_macro] +macro_rules! include {} +#[rustc_builtin_macro] +macro_rules! format_args {} + +include!(concat!("foo/", "foo.rs")); + fn main() { - println!("Hello, {}!", 92); + format_args!("Hello, {}!", 92); dont_color_me_braces!(); noop!(noop!(1)); -} \ No newline at end of file +} + \ No newline at end of file diff --git a/crates/ide/src/syntax_highlighting/tests.rs b/crates/ide/src/syntax_highlighting/tests.rs index 696aa59002..8747071807 100644 --- a/crates/ide/src/syntax_highlighting/tests.rs +++ b/crates/ide/src/syntax_highlighting/tests.rs @@ -48,6 +48,7 @@ fn macros() { check_highlighting( r#" //- proc_macros: mirror +//- /lib.rs crate:lib proc_macros::mirror! { { ,i32 :x pub @@ -95,11 +96,23 @@ macro without_args { } } +#[rustc_builtin_macro] +macro_rules! concat {} +#[rustc_builtin_macro] +macro_rules! include {} +#[rustc_builtin_macro] +macro_rules! format_args {} + +include!(concat!("foo/", "foo.rs")); + fn main() { - println!("Hello, {}!", 92); + format_args!("Hello, {}!", 92); dont_color_me_braces!(); noop!(noop!(1)); } +//- /foo/foo.rs crate:foo +mod foo {} +use self::foo as bar; "#, expect_file!["./test_data/highlight_macros.html"], false, diff --git a/crates/mbe/src/token_map.rs b/crates/mbe/src/token_map.rs index 9b2df89f9c..73a27df5db 100644 --- a/crates/mbe/src/token_map.rs +++ b/crates/mbe/src/token_map.rs @@ -117,4 +117,8 @@ impl TokenMap { TokenTextRange::Delimiter(_) => None, }) } + + pub fn filter(&mut self, id: impl Fn(tt::TokenId) -> bool) { + self.entries.retain(|&(tid, _)| id(tid)); + } } diff --git a/crates/tt/src/lib.rs b/crates/tt/src/lib.rs index 1b8d4ba42a..ab398726b8 100644 --- a/crates/tt/src/lib.rs +++ b/crates/tt/src/lib.rs @@ -70,7 +70,7 @@ pub mod token_id { } impl Subtree { - pub fn visit_ids(&mut self, f: &impl Fn(TokenId) -> TokenId) { + pub fn visit_ids(&mut self, f: &mut impl FnMut(TokenId) -> TokenId) { self.delimiter.open = f(self.delimiter.open); self.delimiter.close = f(self.delimiter.close); self.token_trees.iter_mut().for_each(|tt| match tt { From d999d34e39ca9787f376e22d5f079a9d89267692 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Tue, 1 Aug 2023 11:20:15 +0200 Subject: [PATCH 2/4] Don't bail eager expansion when inner macros fail to resolve --- crates/hir-def/src/lib.rs | 2 +- .../hir-def/src/macro_expansion_tests/mbe.rs | 24 ++++++++++++ crates/hir-expand/src/eager.rs | 38 ++++++++++--------- 3 files changed, 46 insertions(+), 18 deletions(-) diff --git a/crates/hir-def/src/lib.rs b/crates/hir-def/src/lib.rs index 9270cbfe89..fdab7382d3 100644 --- a/crates/hir-def/src/lib.rs +++ b/crates/hir-def/src/lib.rs @@ -1160,7 +1160,7 @@ fn macro_call_as_call_id_( let macro_call = InFile::new(call.ast_id.file_id, call.ast_id.to_node(db)); expand_eager_macro_input(db, krate, macro_call, def, &|path| { resolver(path).filter(MacroDefId::is_fn_like) - })? + }) } _ if def.is_fn_like() => ExpandResult { value: Some(def.as_lazy_macro( diff --git a/crates/hir-def/src/macro_expansion_tests/mbe.rs b/crates/hir-def/src/macro_expansion_tests/mbe.rs index b26f986758..0364904fcd 100644 --- a/crates/hir-def/src/macro_expansion_tests/mbe.rs +++ b/crates/hir-def/src/macro_expansion_tests/mbe.rs @@ -99,6 +99,30 @@ fn#19 main#20(#21)#21 {#22 ); } +#[test] +fn eager_expands_with_unresolved_within() { + check( + r#" +#[rustc_builtin_macro] +#[macro_export] +macro_rules! format_args {} + +fn main(foo: ()) { + format_args!("{} {} {}", format_args!("{}", 0), foo, identity!(10), "bar") +} +"#, + expect![[r##" +#[rustc_builtin_macro] +#[macro_export] +macro_rules! format_args {} + +fn main(foo: ()) { + /* error: unresolved macro identity */::core::fmt::Arguments::new_v1(&["", " ", " ", ], &[::core::fmt::ArgumentV1::new(&(::core::fmt::Arguments::new_v1(&["", ], &[::core::fmt::ArgumentV1::new(&(0), ::core::fmt::Display::fmt), ])), ::core::fmt::Display::fmt), ::core::fmt::ArgumentV1::new(&(foo), ::core::fmt::Display::fmt), ::core::fmt::ArgumentV1::new(&(identity!(10)), ::core::fmt::Display::fmt), ]) +} +"##]], + ); +} + #[test] fn token_mapping_eager() { check( diff --git a/crates/hir-expand/src/eager.rs b/crates/hir-expand/src/eager.rs index 3ca2e295d7..4110f28475 100644 --- a/crates/hir-expand/src/eager.rs +++ b/crates/hir-expand/src/eager.rs @@ -29,7 +29,7 @@ use crate::{ hygiene::Hygiene, mod_path::ModPath, EagerCallInfo, ExpandError, ExpandResult, ExpandTo, InFile, MacroCallId, MacroCallKind, - MacroCallLoc, MacroDefId, MacroDefKind, UnresolvedMacro, + MacroCallLoc, MacroDefId, MacroDefKind, }; pub fn expand_eager_macro_input( @@ -38,7 +38,7 @@ pub fn expand_eager_macro_input( macro_call: InFile, def: MacroDefId, resolver: &dyn Fn(ModPath) -> Option, -) -> Result>, UnresolvedMacro> { +) -> ExpandResult> { let ast_map = db.ast_id_map(macro_call.file_id); // the expansion which the ast id map is built upon has no whitespace, so the offsets are wrong as macro_call is from the token tree that has whitespace! let call_id = InFile::new(macro_call.file_id, ast_map.ast_id(¯o_call.value)); @@ -71,12 +71,12 @@ pub fn expand_eager_macro_input( InFile::new(arg_id.as_file(), arg_exp.syntax_node()), krate, resolver, - )? + ) }; let err = parse_err.or(err); let Some((expanded_eager_input, mapping)) = expanded_eager_input else { - return Ok(ExpandResult { value: None, err }); + return ExpandResult { value: None, err }; }; let (mut subtree, expanded_eager_input_token_map) = @@ -98,7 +98,7 @@ pub fn expand_eager_macro_input( // remap from eager input expansion to original eager input if let Some(&og_range) = ws_mapping.get(og_range) { if let Some(og_token) = og_tmap.token_by_range(og_range) { - ids_used.insert(id); + ids_used.insert(og_token); return og_token; } } @@ -124,7 +124,7 @@ pub fn expand_eager_macro_input( kind: MacroCallKind::FnLike { ast_id: call_id, expand_to }, }; - Ok(ExpandResult { value: Some(db.intern_macro_call(loc)), err }) + ExpandResult { value: Some(db.intern_macro_call(loc)), err } } fn lazy_expand( @@ -150,13 +150,13 @@ fn eager_macro_recur( curr: InFile, krate: CrateId, macro_resolver: &dyn Fn(ModPath) -> Option, -) -> Result)>>, UnresolvedMacro> { +) -> ExpandResult)>> { let original = curr.value.clone_for_update(); let mut mapping = FxHashMap::default(); let mut replacements = Vec::new(); - // Note: We only report a single error inside of eager expansions + // FIXME: We only report a single error inside of eager expansions let mut error = None; let mut offset = 0i32; let apply_offset = |it: TextSize, offset: i32| { @@ -187,7 +187,14 @@ fn eager_macro_recur( } }; let def = match call.path().and_then(|path| ModPath::from_src(db, path, hygiene)) { - Some(path) => macro_resolver(path.clone()).ok_or(UnresolvedMacro { path })?, + Some(path) => match macro_resolver(path.clone()) { + Some(def) => def, + None => { + error = + Some(ExpandError::other(format!("unresolved macro {}", path.display(db)))); + continue; + } + }, None => { error = Some(ExpandError::other("malformed macro invocation")); continue; @@ -195,16 +202,13 @@ fn eager_macro_recur( }; let ExpandResult { value, err } = match def.kind { MacroDefKind::BuiltInEager(..) => { - let ExpandResult { value, err } = match expand_eager_macro_input( + let ExpandResult { value, err } = expand_eager_macro_input( db, krate, curr.with_value(call.clone()), def, macro_resolver, - ) { - Ok(it) => it, - Err(err) => return Err(err), - }; + ); match value { Some(call_id) => { let ExpandResult { value, err: err2 } = @@ -254,7 +258,7 @@ fn eager_macro_recur( parse.as_ref().map(|it| it.syntax_node()), krate, macro_resolver, - )?; + ); let err = err.or(error); if let Some(tt) = call.token_tree() { @@ -284,7 +288,7 @@ fn eager_macro_recur( } // check if the whole original syntax is replaced if call.syntax() == &original { - return Ok(ExpandResult { value: value.zip(Some(mapping)), err: error }); + return ExpandResult { value: value.zip(Some(mapping)), err: error }; } if let Some(insert) = value { @@ -295,5 +299,5 @@ fn eager_macro_recur( } replacements.into_iter().rev().for_each(|(old, new)| ted::replace(old.syntax(), new)); - Ok(ExpandResult { value: Some((original, mapping)), err: error }) + ExpandResult { value: Some((original, mapping)), err: error } } From e14d84d0a6b532c683ca3e3e1fedbd94796c9604 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Tue, 1 Aug 2023 12:38:53 +0200 Subject: [PATCH 3/4] Skip out on single-segment immediate macro resolution when there are errors --- crates/hir-def/src/lib.rs | 21 +++++++------ crates/hir-def/src/nameres/collector.rs | 39 +++++++++++++++++-------- 2 files changed, 39 insertions(+), 21 deletions(-) diff --git a/crates/hir-def/src/lib.rs b/crates/hir-def/src/lib.rs index fdab7382d3..e8187e05ad 100644 --- a/crates/hir-def/src/lib.rs +++ b/crates/hir-def/src/lib.rs @@ -1083,7 +1083,7 @@ pub trait AsMacroCall { &self, db: &dyn ExpandDatabase, krate: CrateId, - resolver: impl Fn(path::ModPath) -> Option, + resolver: impl Fn(path::ModPath) -> Option + Copy, ) -> Option { self.as_call_id_with_errors(db, krate, resolver).ok()?.value } @@ -1092,7 +1092,7 @@ pub trait AsMacroCall { &self, db: &dyn ExpandDatabase, krate: CrateId, - resolver: impl Fn(path::ModPath) -> Option, + resolver: impl Fn(path::ModPath) -> Option + Copy, ) -> Result>, UnresolvedMacro>; } @@ -1101,7 +1101,7 @@ impl AsMacroCall for InFile<&ast::MacroCall> { &self, db: &dyn ExpandDatabase, krate: CrateId, - resolver: impl Fn(path::ModPath) -> Option, + resolver: impl Fn(path::ModPath) -> Option + Copy, ) -> Result>, 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)); @@ -1112,12 +1112,13 @@ impl AsMacroCall for InFile<&ast::MacroCall> { return Ok(ExpandResult::only_err(ExpandError::other("malformed macro invocation"))); }; - macro_call_as_call_id_( + macro_call_as_call_id_with_eager( db, &AstIdWithPath::new(ast_id.file_id, ast_id.value, path), expands_to, krate, resolver, + resolver, ) } } @@ -1140,17 +1141,19 @@ fn macro_call_as_call_id( call: &AstIdWithPath, expand_to: ExpandTo, krate: CrateId, - resolver: impl Fn(path::ModPath) -> Option, + resolver: impl Fn(path::ModPath) -> Option + Copy, ) -> Result, UnresolvedMacro> { - macro_call_as_call_id_(db, call, expand_to, krate, resolver).map(|res| res.value) + macro_call_as_call_id_with_eager(db, call, expand_to, krate, resolver, resolver) + .map(|res| res.value) } -fn macro_call_as_call_id_( +fn macro_call_as_call_id_with_eager( db: &dyn ExpandDatabase, call: &AstIdWithPath, expand_to: ExpandTo, krate: CrateId, - resolver: impl Fn(path::ModPath) -> Option, + resolver: impl FnOnce(path::ModPath) -> Option, + eager_resolver: impl Fn(path::ModPath) -> Option, ) -> Result>, UnresolvedMacro> { let def = resolver(call.path.clone()).ok_or_else(|| UnresolvedMacro { path: call.path.clone() })?; @@ -1159,7 +1162,7 @@ fn macro_call_as_call_id_( MacroDefKind::BuiltInEager(..) => { let macro_call = InFile::new(call.ast_id.file_id, call.ast_id.to_node(db)); expand_eager_macro_input(db, krate, macro_call, def, &|path| { - resolver(path).filter(MacroDefId::is_fn_like) + eager_resolver(path).filter(MacroDefId::is_fn_like) }) } _ if def.is_fn_like() => ExpandResult { diff --git a/crates/hir-def/src/nameres/collector.rs b/crates/hir-def/src/nameres/collector.rs index c048716d74..61915504b6 100644 --- a/crates/hir-def/src/nameres/collector.rs +++ b/crates/hir-def/src/nameres/collector.rs @@ -38,7 +38,7 @@ use crate::{ self, ExternCrate, Fields, FileItemTreeId, ImportKind, ItemTree, ItemTreeId, ItemTreeNode, MacroCall, MacroDef, MacroRules, Mod, ModItem, ModKind, TreeId, }, - macro_call_as_call_id, macro_id_to_def_id, + macro_call_as_call_id, macro_call_as_call_id_with_eager, macro_id_to_def_id, nameres::{ diagnostics::DefDiagnostic, mod_resolution::ModDir, @@ -2187,7 +2187,7 @@ impl ModCollector<'_, '_> { // scopes without eager expansion. // Case 1: try to resolve macro calls with single-segment name and expand macro_rules - if let Ok(res) = macro_call_as_call_id( + if let Ok(res) = macro_call_as_call_id_with_eager( db.upcast(), &ast_id, mac.expand_to, @@ -2210,19 +2210,34 @@ impl ModCollector<'_, '_> { .map(|it| macro_id_to_def_id(self.def_collector.db, it)) }) }, - ) { - // Legacy macros need to be expanded immediately, so that any macros they produce - // are in scope. - if let Some(val) = res { - self.def_collector.collect_macro_expansion( + |path| { + let resolved_res = self.def_collector.def_map.resolve_path_fp_with_macro( + db, + ResolveMode::Other, self.module_id, - val, - self.macro_depth + 1, - container, + &path, + BuiltinShadowMode::Module, + Some(MacroSubNs::Bang), ); - } + resolved_res.resolved_def.take_macros().map(|it| macro_id_to_def_id(db, it)) + }, + ) { + // FIXME: if there were errors, this mightve been in the eager expansion from an + // unresolved macro, so we need to push this into late macro resolution. see fixme above + if res.err.is_none() { + // Legacy macros need to be expanded immediately, so that any macros they produce + // are in scope. + if let Some(val) = res.value { + self.def_collector.collect_macro_expansion( + self.module_id, + val, + self.macro_depth + 1, + container, + ); + } - return; + return; + } } // Case 2: resolve in module scope, expand during name resolution. From a5059da57afd32a545e346394b1f058a6a40f244 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Tue, 1 Aug 2023 12:53:45 +0200 Subject: [PATCH 4/4] Update test fixture --- crates/ide-diagnostics/src/handlers/macro_error.rs | 2 +- .../src/syntax_highlighting/test_data/highlight_strings.html | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ide-diagnostics/src/handlers/macro_error.rs b/crates/ide-diagnostics/src/handlers/macro_error.rs index 3af5f94eeb..f54cdd63bb 100644 --- a/crates/ide-diagnostics/src/handlers/macro_error.rs +++ b/crates/ide-diagnostics/src/handlers/macro_error.rs @@ -80,7 +80,7 @@ macro_rules! m { fn f() { m!(); - //^^^^ error: unresolved macro `$crate::private::concat!` + //^^^^ error: unresolved macro $crate::private::concat } //- /core.rs crate:core diff --git a/crates/ide/src/syntax_highlighting/test_data/highlight_strings.html b/crates/ide/src/syntax_highlighting/test_data/highlight_strings.html index dcac8eb736..3ac8aa9cc9 100644 --- a/crates/ide/src/syntax_highlighting/test_data/highlight_strings.html +++ b/crates/ide/src/syntax_highlighting/test_data/highlight_strings.html @@ -178,5 +178,5 @@ pre { color: #DCDCCC; background: #3F3F3F; font-size: 22px; padd toho!("{}fmt", 0); asm!("mov eax, {0}"); format_args!(concat!("{}"), "{}"); - format_args!("{} {} {} {} {} {}", backslash, format_args!("{}", 0), foo, "bar", toho!(), backslash); + format_args!("{} {} {} {} {} {}", backslash, format_args!("{}", 0), foo, "bar", toho!(), backslash); } \ No newline at end of file