Unnamed repository; edit this file 'description' to name the repository.
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.
| -rw-r--r-- | crates/hir/src/semantics.rs | 10 | ||||
| -rw-r--r-- | crates/ide-assists/src/handlers/add_missing_impl_members.rs | 87 | ||||
| -rw-r--r-- | crates/ide-assists/src/handlers/replace_derive_with_manual_impl.rs | 34 | ||||
| -rw-r--r-- | crates/ide-assists/src/utils.rs | 85 | ||||
| -rw-r--r-- | crates/ide-completion/src/completions/item_list/trait_impl.rs | 81 |
5 files changed, 225 insertions, 72 deletions
diff --git a/crates/hir/src/semantics.rs b/crates/hir/src/semantics.rs index 2d2b00b147..36ded659b4 100644 --- a/crates/hir/src/semantics.rs +++ b/crates/hir/src/semantics.rs @@ -483,10 +483,6 @@ impl<'db, DB: HirDatabase> Semantics<'db, DB> { self.imp.scope_at_offset(node, offset) } - pub fn scope_for_def(&self, def: Trait) -> SemanticsScope<'db> { - self.imp.scope_for_def(def) - } - pub fn assert_contains_node(&self, node: &SyntaxNode) { self.imp.assert_contains_node(node) } @@ -1307,12 +1303,6 @@ impl<'db> SemanticsImpl<'db> { ) } - fn scope_for_def(&self, def: Trait) -> SemanticsScope<'db> { - let file_id = self.db.lookup_intern_trait(def.id).id.file_id(); - let resolver = def.id.resolver(self.db.upcast()); - SemanticsScope { db: self.db, file_id, resolver } - } - fn source<Def: HasSource>(&self, def: Def) -> Option<InFile<Def::Ast>> where Def::Ast: AstNode, diff --git a/crates/ide-assists/src/handlers/add_missing_impl_members.rs b/crates/ide-assists/src/handlers/add_missing_impl_members.rs index 0cf7a848d8..c7f9fa5bd6 100644 --- a/crates/ide-assists/src/handlers/add_missing_impl_members.rs +++ b/crates/ide-assists/src/handlers/add_missing_impl_members.rs @@ -1,5 +1,4 @@ use hir::HasSource; -use ide_db::syntax_helpers::insert_whitespace_into_node::insert_ws_into; use syntax::ast::{self, make, AstNode}; use crate::{ @@ -129,20 +128,9 @@ fn add_missing_impl_members_inner( let target = impl_def.syntax().text_range(); acc.add(AssistId(assist_id, AssistKind::QuickFix), label, target, |edit| { let new_impl_def = edit.make_mut(impl_def.clone()); - let missing_items = missing_items - .into_iter() - .map(|it| { - if ctx.sema.hir_file_for(it.syntax()).is_macro() { - if let Some(it) = ast::AssocItem::cast(insert_ws_into(it.syntax().clone())) { - return it; - } - } - it.clone_for_update() - }) - .collect(); let first_new_item = add_trait_assoc_items_to_impl( &ctx.sema, - missing_items, + &missing_items, trait_, &new_impl_def, target_scope, @@ -1730,4 +1718,77 @@ impl m::Foo for S { }"#, ) } + + #[test] + fn nested_macro_should_not_cause_crash() { + check_assist( + add_missing_impl_members, + r#" +macro_rules! ty { () => { i32 } } +trait SomeTrait { type Output; } +impl SomeTrait for i32 { type Output = i64; } +macro_rules! define_method { + () => { + fn method(&mut self, params: <ty!() as SomeTrait>::Output); + }; +} +trait AnotherTrait { define_method!(); } +impl $0AnotherTrait for () { +} +"#, + r#" +macro_rules! ty { () => { i32 } } +trait SomeTrait { type Output; } +impl SomeTrait for i32 { type Output = i64; } +macro_rules! define_method { + () => { + fn method(&mut self, params: <ty!() as SomeTrait>::Output); + }; +} +trait AnotherTrait { define_method!(); } +impl AnotherTrait for () { + $0fn method(&mut self,params: <ty!()as SomeTrait>::Output) { + todo!() + } +} +"#, + ); + } + + // FIXME: `T` in `ty!(T)` should be replaced by `PathTransform`. + #[test] + fn paths_in_nested_macro_should_get_transformed() { + check_assist( + add_missing_impl_members, + r#" +macro_rules! ty { ($me:ty) => { $me } } +trait SomeTrait { type Output; } +impl SomeTrait for i32 { type Output = i64; } +macro_rules! define_method { + ($t:ty) => { + fn method(&mut self, params: <ty!($t) as SomeTrait>::Output); + }; +} +trait AnotherTrait<T: SomeTrait> { define_method!(T); } +impl $0AnotherTrait<i32> for () { +} +"#, + r#" +macro_rules! ty { ($me:ty) => { $me } } +trait SomeTrait { type Output; } +impl SomeTrait for i32 { type Output = i64; } +macro_rules! define_method { + ($t:ty) => { + fn method(&mut self, params: <ty!($t) as SomeTrait>::Output); + }; +} +trait AnotherTrait<T: SomeTrait> { define_method!(T); } +impl AnotherTrait<i32> for () { + $0fn method(&mut self,params: <ty!(T)as SomeTrait>::Output) { + todo!() + } +} +"#, + ); + } } diff --git a/crates/ide-assists/src/handlers/replace_derive_with_manual_impl.rs b/crates/ide-assists/src/handlers/replace_derive_with_manual_impl.rs index 1b0a8e0a8d..0469343dbf 100644 --- a/crates/ide-assists/src/handlers/replace_derive_with_manual_impl.rs +++ b/crates/ide-assists/src/handlers/replace_derive_with_manual_impl.rs @@ -1,8 +1,5 @@ use hir::{InFile, ModuleDef}; -use ide_db::{ - helpers::mod_path_to_ast, imports::import_assets::NameToImport, items_locator, - syntax_helpers::insert_whitespace_into_node::insert_ws_into, -}; +use ide_db::{helpers::mod_path_to_ast, imports::import_assets::NameToImport, items_locator}; use itertools::Itertools; use syntax::{ ast::{self, AstNode, HasName}, @@ -202,19 +199,24 @@ fn impl_def_from_trait( node }; - let trait_items = trait_items - .into_iter() - .map(|it| { - if sema.hir_file_for(it.syntax()).is_macro() { - if let Some(it) = ast::AssocItem::cast(insert_ws_into(it.syntax().clone())) { - return it; - } - } - it.clone_for_update() - }) - .collect(); + // <<<<<<< HEAD + // let trait_items = trait_items + // .into_iter() + // .map(|it| { + // if sema.hir_file_for(it.syntax()).is_macro() { + // if let Some(it) = ast::AssocItem::cast(insert_ws_into(it.syntax().clone())) { + // return it; + // } + // } + // it.clone_for_update() + // }) + // .collect(); + // let first_assoc_item = + // add_trait_assoc_items_to_impl(sema, trait_items, trait_, &impl_def, target_scope); + // ======= let first_assoc_item = - add_trait_assoc_items_to_impl(sema, trait_items, trait_, &impl_def, target_scope); + add_trait_assoc_items_to_impl(sema, &trait_items, trait_, &impl_def, target_scope); + // >>>>>>> fix(assist): derive source scope from syntax node to be transformed // Generate a default `impl` function body for the derived trait. if let ast::AssocItem::Fn(ref func) = first_assoc_item { diff --git a/crates/ide-assists/src/utils.rs b/crates/ide-assists/src/utils.rs index cc4a7f3c0a..03d8553506 100644 --- a/crates/ide-assists/src/utils.rs +++ b/crates/ide-assists/src/utils.rs @@ -3,8 +3,11 @@ use std::ops; pub(crate) use gen_trait_fn_body::gen_trait_fn_body; -use hir::{db::HirDatabase, HirDisplay, Semantics}; -use ide_db::{famous_defs::FamousDefs, path_transform::PathTransform, RootDatabase, SnippetCap}; +use hir::{db::HirDatabase, HirDisplay, InFile, Semantics}; +use ide_db::{ + famous_defs::FamousDefs, path_transform::PathTransform, + syntax_helpers::insert_whitespace_into_node::insert_ws_into, RootDatabase, SnippetCap, +}; use stdx::format_to; use syntax::{ ast::{ @@ -91,30 +94,21 @@ pub fn filter_assoc_items( sema: &Semantics<'_, RootDatabase>, items: &[hir::AssocItem], default_methods: DefaultMethods, -) -> Vec<ast::AssocItem> { - fn has_def_name(item: &ast::AssocItem) -> bool { - match item { - ast::AssocItem::Fn(def) => def.name(), - ast::AssocItem::TypeAlias(def) => def.name(), - ast::AssocItem::Const(def) => def.name(), - ast::AssocItem::MacroCall(_) => None, - } - .is_some() - } - - items +) -> Vec<InFile<ast::AssocItem>> { + return items .iter() // Note: This throws away items with no source. - .filter_map(|&i| { - let item = match i { - hir::AssocItem::Function(i) => ast::AssocItem::Fn(sema.source(i)?.value), - hir::AssocItem::TypeAlias(i) => ast::AssocItem::TypeAlias(sema.source(i)?.value), - hir::AssocItem::Const(i) => ast::AssocItem::Const(sema.source(i)?.value), + .copied() + .filter_map(|assoc_item| { + let item = match assoc_item { + hir::AssocItem::Function(it) => sema.source(it)?.map(ast::AssocItem::Fn), + hir::AssocItem::TypeAlias(it) => sema.source(it)?.map(ast::AssocItem::TypeAlias), + hir::AssocItem::Const(it) => sema.source(it)?.map(ast::AssocItem::Const), }; Some(item) }) .filter(has_def_name) - .filter(|it| match it { + .filter(|it| match &it.value { ast::AssocItem::Fn(def) => matches!( (default_methods, def.body()), (DefaultMethods::Only, Some(_)) | (DefaultMethods::No, None) @@ -125,26 +119,55 @@ pub fn filter_assoc_items( ), _ => default_methods == DefaultMethods::No, }) - .collect::<Vec<_>>() + .collect(); + + fn has_def_name(item: &InFile<ast::AssocItem>) -> bool { + match &item.value { + ast::AssocItem::Fn(def) => def.name(), + ast::AssocItem::TypeAlias(def) => def.name(), + ast::AssocItem::Const(def) => def.name(), + ast::AssocItem::MacroCall(_) => None, + } + .is_some() + } } +/// Given `original_items` retrieved from the trait definition (usually by +/// [`filter_assoc_items()`]), clones each item for update and applies path transformation to it, +/// then inserts into `impl_`. Returns the modified `impl_` and the first associated item that got +/// inserted. pub fn add_trait_assoc_items_to_impl( sema: &Semantics<'_, RootDatabase>, - items: Vec<ast::AssocItem>, + original_items: &[InFile<ast::AssocItem>], trait_: hir::Trait, impl_: &ast::Impl, target_scope: hir::SemanticsScope<'_>, ) -> ast::AssocItem { - let source_scope = sema.scope_for_def(trait_); - - let transform = PathTransform::trait_impl(&target_scope, &source_scope, trait_, impl_.clone()); - let new_indent_level = IndentLevel::from_node(impl_.syntax()) + 1; - let items = items.into_iter().map(|assoc_item| { - transform.apply(assoc_item.syntax()); - assoc_item.remove_attrs_and_docs(); - assoc_item.reindent_to(new_indent_level); - assoc_item + let items = original_items.into_iter().map(|InFile { file_id, value: original_item }| { + let cloned_item = { + if file_id.is_macro() { + if let Some(formatted) = + ast::AssocItem::cast(insert_ws_into(original_item.syntax().clone())) + { + return formatted; + } else { + stdx::never!("formatted `AssocItem` could not be cast back to `AssocItem`"); + } + } + original_item.clone_for_update() + }; + + if let Some(source_scope) = sema.scope(original_item.syntax()) { + // FIXME: Paths in nested macros are not handled well. See + // `add_missing_impl_members::paths_in_nested_macro_should_get_transformed` test. + let transform = + PathTransform::trait_impl(&target_scope, &source_scope, trait_, impl_.clone()); + transform.apply(cloned_item.syntax()); + } + cloned_item.remove_attrs_and_docs(); + cloned_item.reindent_to(new_indent_level); + cloned_item }); let assoc_item_list = impl_.get_or_create_assoc_item_list(); diff --git a/crates/ide-completion/src/completions/item_list/trait_impl.rs b/crates/ide-completion/src/completions/item_list/trait_impl.rs index 664aa7b839..269e40e6ef 100644 --- a/crates/ide-completion/src/completions/item_list/trait_impl.rs +++ b/crates/ide-completion/src/completions/item_list/trait_impl.rs @@ -227,9 +227,8 @@ fn get_transformed_assoc_item( assoc_item: ast::AssocItem, impl_def: hir::Impl, ) -> Option<ast::AssocItem> { - let assoc_item = assoc_item.clone_for_update(); let trait_ = impl_def.trait_(ctx.db)?; - let source_scope = &ctx.sema.scope_for_def(trait_); + let source_scope = &ctx.sema.scope(assoc_item.syntax())?; let target_scope = &ctx.sema.scope(ctx.sema.source(impl_def)?.syntax().value)?; let transform = PathTransform::trait_impl( target_scope, @@ -238,6 +237,9 @@ fn get_transformed_assoc_item( ctx.sema.source(impl_def)?.value, ); + let assoc_item = assoc_item.clone_for_update(); + // FIXME: Paths in nested macros are not handled well. See + // `macro_generated_assoc_item2` test. transform.apply(assoc_item.syntax()); assoc_item.remove_attrs_and_docs(); Some(assoc_item) @@ -1221,6 +1223,81 @@ impl Foo for Test { } #[test] + fn macro_generated_assoc_item() { + check_edit( + "fn method", + r#" +macro_rules! ty { () => { i32 } } +trait SomeTrait { type Output; } +impl SomeTrait for i32 { type Output = i64; } +macro_rules! define_method { + () => { + fn method(&mut self, params: <ty!() as SomeTrait>::Output); + }; +} +trait AnotherTrait { define_method!(); } +impl AnotherTrait for () { + $0 +} +"#, + r#" +macro_rules! ty { () => { i32 } } +trait SomeTrait { type Output; } +impl SomeTrait for i32 { type Output = i64; } +macro_rules! define_method { + () => { + fn method(&mut self, params: <ty!() as SomeTrait>::Output); + }; +} +trait AnotherTrait { define_method!(); } +impl AnotherTrait for () { + fn method(&mut self,params: <ty!()as SomeTrait>::Output) { + $0 +} +} +"#, + ); + } + + // FIXME: `T` in `ty!(T)` should be replaced by `PathTransform`. + #[test] + fn macro_generated_assoc_item2() { + check_edit( + "fn method", + r#" +macro_rules! ty { ($me:ty) => { $me } } +trait SomeTrait { type Output; } +impl SomeTrait for i32 { type Output = i64; } +macro_rules! define_method { + ($t:ty) => { + fn method(&mut self, params: <ty!($t) as SomeTrait>::Output); + }; +} +trait AnotherTrait<T: SomeTrait> { define_method!(T); } +impl AnotherTrait<i32> for () { + $0 +} +"#, + r#" +macro_rules! ty { ($me:ty) => { $me } } +trait SomeTrait { type Output; } +impl SomeTrait for i32 { type Output = i64; } +macro_rules! define_method { + ($t:ty) => { + fn method(&mut self, params: <ty!($t) as SomeTrait>::Output); + }; +} +trait AnotherTrait<T: SomeTrait> { define_method!(T); } +impl AnotherTrait<i32> for () { + fn method(&mut self,params: <ty!(T)as SomeTrait>::Output) { + $0 +} +} +"#, + ); + } + + #[test] fn includes_gat_generics() { check_edit( "type Ty", |