Skip to content

Commit c3bab96

Browse files
borsbors
committed
Auto merge of #14989 - lowr:fix/nested-macro-in-assoc-item-panic, r=Veykril
fix: derive source scope from syntax node to be transformed Fixes #14534 When we use `PathTransform` for associated items of a trait, we have been feeding `SemanticsScope` for the trait definition to it as source scope. `PathTransform` uses the source scope to resolve paths in associated items to find which path to transform. In the course of path resolution, the scope is responsible for lowering `ast::MacroType`s (because they can be written within a path) using `AstIdMap` for the scope's `HirFileId`. The problem here is that when an associated item is generated by a macro, the scope for the trait is different from the scope for that associated item. The former can only resolve the top-level macros within the trait definition but not the macro calls generated by those top-level macros. We need the latter to resolve such nested macros. This PR makes sure that we pass `SemanticsScope` for each associated item we're applying path transformation to.
2 parents 68bdf60 + d091991 commit c3bab96

File tree

5 files changed

+225
-72
lines changed

5 files changed

+225
-72
lines changed

‎crates/hir/src/semantics.rs‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -483,10 +483,6 @@ impl<'db, DB: HirDatabase> Semantics<'db, DB> {
483483
self.imp.scope_at_offset(node, offset)
484484
}
485485

486-
pub fn scope_for_def(&self, def: Trait) -> SemanticsScope<'db> {
487-
self.imp.scope_for_def(def)
488-
}
489-
490486
pub fn assert_contains_node(&self, node: &SyntaxNode) {
491487
self.imp.assert_contains_node(node)
492488
}
@@ -1307,12 +1303,6 @@ impl<'db> SemanticsImpl<'db> {
13071303
)
13081304
}
13091305

1310-
fn scope_for_def(&self, def: Trait) -> SemanticsScope<'db> {
1311-
let file_id = self.db.lookup_intern_trait(def.id).id.file_id();
1312-
let resolver = def.id.resolver(self.db.upcast());
1313-
SemanticsScope { db: self.db, file_id, resolver }
1314-
}
1315-
13161306
fn source<Def: HasSource>(&self, def: Def) -> Option<InFile<Def::Ast>>
13171307
where
13181308
Def::Ast: AstNode,

‎crates/ide-assists/src/handlers/add_missing_impl_members.rs‎

Lines changed: 74 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
use hir::HasSource;
2-
use ide_db::syntax_helpers::insert_whitespace_into_node::insert_ws_into;
32
use syntax::ast::{self, make, AstNode};
43

54
use crate::{
@@ -129,20 +128,9 @@ fn add_missing_impl_members_inner(
129128
let target = impl_def.syntax().text_range();
130129
acc.add(AssistId(assist_id, AssistKind::QuickFix), label, target, |edit| {
131130
let new_impl_def = edit.make_mut(impl_def.clone());
132-
let missing_items = missing_items
133-
.into_iter()
134-
.map(|it| {
135-
if ctx.sema.hir_file_for(it.syntax()).is_macro() {
136-
if let Some(it) = ast::AssocItem::cast(insert_ws_into(it.syntax().clone())) {
137-
return it;
138-
}
139-
}
140-
it.clone_for_update()
141-
})
142-
.collect();
143131
let first_new_item = add_trait_assoc_items_to_impl(
144132
&ctx.sema,
145-
missing_items,
133+
&missing_items,
146134
trait_,
147135
&new_impl_def,
148136
target_scope,
@@ -1730,4 +1718,77 @@ impl m::Foo for S {
17301718
}"#,
17311719
)
17321720
}
1721+
1722+
#[test]
1723+
fn nested_macro_should_not_cause_crash() {
1724+
check_assist(
1725+
add_missing_impl_members,
1726+
r#"
1727+
macro_rules! ty { () => { i32 } }
1728+
trait SomeTrait { type Output; }
1729+
impl SomeTrait for i32 { type Output = i64; }
1730+
macro_rules! define_method {
1731+
() => {
1732+
fn method(&mut self, params: <ty!() as SomeTrait>::Output);
1733+
};
1734+
}
1735+
trait AnotherTrait { define_method!(); }
1736+
impl $0AnotherTrait for () {
1737+
}
1738+
"#,
1739+
r#"
1740+
macro_rules! ty { () => { i32 } }
1741+
trait SomeTrait { type Output; }
1742+
impl SomeTrait for i32 { type Output = i64; }
1743+
macro_rules! define_method {
1744+
() => {
1745+
fn method(&mut self, params: <ty!() as SomeTrait>::Output);
1746+
};
1747+
}
1748+
trait AnotherTrait { define_method!(); }
1749+
impl AnotherTrait for () {
1750+
$0fn method(&mut self,params: <ty!()as SomeTrait>::Output) {
1751+
todo!()
1752+
}
1753+
}
1754+
"#,
1755+
);
1756+
}
1757+
1758+
// FIXME: `T` in `ty!(T)` should be replaced by `PathTransform`.
1759+
#[test]
1760+
fn paths_in_nested_macro_should_get_transformed() {
1761+
check_assist(
1762+
add_missing_impl_members,
1763+
r#"
1764+
macro_rules! ty { ($me:ty) => { $me } }
1765+
trait SomeTrait { type Output; }
1766+
impl SomeTrait for i32 { type Output = i64; }
1767+
macro_rules! define_method {
1768+
($t:ty) => {
1769+
fn method(&mut self, params: <ty!($t) as SomeTrait>::Output);
1770+
};
1771+
}
1772+
trait AnotherTrait<T: SomeTrait> { define_method!(T); }
1773+
impl $0AnotherTrait<i32> for () {
1774+
}
1775+
"#,
1776+
r#"
1777+
macro_rules! ty { ($me:ty) => { $me } }
1778+
trait SomeTrait { type Output; }
1779+
impl SomeTrait for i32 { type Output = i64; }
1780+
macro_rules! define_method {
1781+
($t:ty) => {
1782+
fn method(&mut self, params: <ty!($t) as SomeTrait>::Output);
1783+
};
1784+
}
1785+
trait AnotherTrait<T: SomeTrait> { define_method!(T); }
1786+
impl AnotherTrait<i32> for () {
1787+
$0fn method(&mut self,params: <ty!(T)as SomeTrait>::Output) {
1788+
todo!()
1789+
}
1790+
}
1791+
"#,
1792+
);
1793+
}
17331794
}

‎crates/ide-assists/src/handlers/replace_derive_with_manual_impl.rs‎

Lines changed: 18 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,5 @@
11
use hir::{InFile, ModuleDef};
2-
use ide_db::{
3-
helpers::mod_path_to_ast, imports::import_assets::NameToImport, items_locator,
4-
syntax_helpers::insert_whitespace_into_node::insert_ws_into,
5-
};
2+
use ide_db::{helpers::mod_path_to_ast, imports::import_assets::NameToImport, items_locator};
63
use itertools::Itertools;
74
use syntax::{
85
ast::{self, AstNode, HasName},
@@ -202,19 +199,24 @@ fn impl_def_from_trait(
202199
node
203200
};
204201

205-
let trait_items = trait_items
206-
.into_iter()
207-
.map(|it| {
208-
if sema.hir_file_for(it.syntax()).is_macro() {
209-
if let Some(it) = ast::AssocItem::cast(insert_ws_into(it.syntax().clone())) {
210-
return it;
211-
}
212-
}
213-
it.clone_for_update()
214-
})
215-
.collect();
202+
// <<<<<<< HEAD
203+
// let trait_items = trait_items
204+
// .into_iter()
205+
// .map(|it| {
206+
// if sema.hir_file_for(it.syntax()).is_macro() {
207+
// if let Some(it) = ast::AssocItem::cast(insert_ws_into(it.syntax().clone())) {
208+
// return it;
209+
// }
210+
// }
211+
// it.clone_for_update()
212+
// })
213+
// .collect();
214+
// let first_assoc_item =
215+
// add_trait_assoc_items_to_impl(sema, trait_items, trait_, &impl_def, target_scope);
216+
// =======
216217
let first_assoc_item =
217-
add_trait_assoc_items_to_impl(sema, trait_items, trait_, &impl_def, target_scope);
218+
add_trait_assoc_items_to_impl(sema, &trait_items, trait_, &impl_def, target_scope);
219+
// >>>>>>> fix(assist): derive source scope from syntax node to be transformed
218220

219221
// Generate a default `impl` function body for the derived trait.
220222
if let ast::AssocItem::Fn(ref func) = first_assoc_item {

‎crates/ide-assists/src/utils.rs‎

Lines changed: 54 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,11 @@
33
use std::ops;
44

55
pub(crate) use gen_trait_fn_body::gen_trait_fn_body;
6-
use hir::{db::HirDatabase, HirDisplay, Semantics};
7-
use ide_db::{famous_defs::FamousDefs, path_transform::PathTransform, RootDatabase, SnippetCap};
6+
use hir::{db::HirDatabase, HirDisplay, InFile, Semantics};
7+
use ide_db::{
8+
famous_defs::FamousDefs, path_transform::PathTransform,
9+
syntax_helpers::insert_whitespace_into_node::insert_ws_into, RootDatabase, SnippetCap,
10+
};
811
use stdx::format_to;
912
use syntax::{
1013
ast::{
@@ -91,30 +94,21 @@ pub fn filter_assoc_items(
9194
sema: &Semantics<'_, RootDatabase>,
9295
items: &[hir::AssocItem],
9396
default_methods: DefaultMethods,
94-
) -> Vec<ast::AssocItem> {
95-
fn has_def_name(item: &ast::AssocItem) -> bool {
96-
match item {
97-
ast::AssocItem::Fn(def) => def.name(),
98-
ast::AssocItem::TypeAlias(def) => def.name(),
99-
ast::AssocItem::Const(def) => def.name(),
100-
ast::AssocItem::MacroCall(_) => None,
101-
}
102-
.is_some()
103-
}
104-
105-
items
97+
) -> Vec<InFile<ast::AssocItem>> {
98+
return items
10699
.iter()
107100
// Note: This throws away items with no source.
108-
.filter_map(|&i| {
109-
let item = match i {
110-
hir::AssocItem::Function(i) => ast::AssocItem::Fn(sema.source(i)?.value),
111-
hir::AssocItem::TypeAlias(i) => ast::AssocItem::TypeAlias(sema.source(i)?.value),
112-
hir::AssocItem::Const(i) => ast::AssocItem::Const(sema.source(i)?.value),
101+
.copied()
102+
.filter_map(|assoc_item| {
103+
let item = match assoc_item {
104+
hir::AssocItem::Function(it) => sema.source(it)?.map(ast::AssocItem::Fn),
105+
hir::AssocItem::TypeAlias(it) => sema.source(it)?.map(ast::AssocItem::TypeAlias),
106+
hir::AssocItem::Const(it) => sema.source(it)?.map(ast::AssocItem::Const),
113107
};
114108
Some(item)
115109
})
116110
.filter(has_def_name)
117-
.filter(|it| match it {
111+
.filter(|it| match &it.value {
118112
ast::AssocItem::Fn(def) => matches!(
119113
(default_methods, def.body()),
120114
(DefaultMethods::Only, Some(_)) | (DefaultMethods::No, None)
@@ -125,26 +119,55 @@ pub fn filter_assoc_items(
125119
),
126120
_ => default_methods == DefaultMethods::No,
127121
})
128-
.collect::<Vec<_>>()
122+
.collect();
123+
124+
fn has_def_name(item: &InFile<ast::AssocItem>) -> bool {
125+
match &item.value {
126+
ast::AssocItem::Fn(def) => def.name(),
127+
ast::AssocItem::TypeAlias(def) => def.name(),
128+
ast::AssocItem::Const(def) => def.name(),
129+
ast::AssocItem::MacroCall(_) => None,
130+
}
131+
.is_some()
132+
}
129133
}
130134

135+
/// Given `original_items` retrieved from the trait definition (usually by
136+
/// [`filter_assoc_items()`]), clones each item for update and applies path transformation to it,
137+
/// then inserts into `impl_`. Returns the modified `impl_` and the first associated item that got
138+
/// inserted.
131139
pub fn add_trait_assoc_items_to_impl(
132140
sema: &Semantics<'_, RootDatabase>,
133-
items: Vec<ast::AssocItem>,
141+
original_items: &[InFile<ast::AssocItem>],
134142
trait_: hir::Trait,
135143
impl_: &ast::Impl,
136144
target_scope: hir::SemanticsScope<'_>,
137145
) -> ast::AssocItem {
138-
let source_scope = sema.scope_for_def(trait_);
139-
140-
let transform = PathTransform::trait_impl(&target_scope, &source_scope, trait_, impl_.clone());
141-
142146
let new_indent_level = IndentLevel::from_node(impl_.syntax()) + 1;
143-
let items = items.into_iter().map(|assoc_item| {
144-
transform.apply(assoc_item.syntax());
145-
assoc_item.remove_attrs_and_docs();
146-
assoc_item.reindent_to(new_indent_level);
147-
assoc_item
147+
let items = original_items.into_iter().map(|InFile { file_id, value: original_item }| {
148+
let cloned_item = {
149+
if file_id.is_macro() {
150+
if let Some(formatted) =
151+
ast::AssocItem::cast(insert_ws_into(original_item.syntax().clone()))
152+
{
153+
return formatted;
154+
} else {
155+
stdx::never!("formatted `AssocItem` could not be cast back to `AssocItem`");
156+
}
157+
}
158+
original_item.clone_for_update()
159+
};
160+
161+
if let Some(source_scope) = sema.scope(original_item.syntax()) {
162+
// FIXME: Paths in nested macros are not handled well. See
163+
// `add_missing_impl_members::paths_in_nested_macro_should_get_transformed` test.
164+
let transform =
165+
PathTransform::trait_impl(&target_scope, &source_scope, trait_, impl_.clone());
166+
transform.apply(cloned_item.syntax());
167+
}
168+
cloned_item.remove_attrs_and_docs();
169+
cloned_item.reindent_to(new_indent_level);
170+
cloned_item
148171
});
149172

150173
let assoc_item_list = impl_.get_or_create_assoc_item_list();

0 commit comments

Comments
 (0)