From 934358e95cd2c8190396ba4d7119e5c3dc3b2ead Mon Sep 17 00:00:00 2001 From: Ryan Mehri Date: Mon, 2 Oct 2023 22:04:59 -0700 Subject: [PATCH 1/3] fix: resolve Self type references in delegate method assist --- .../src/handlers/generate_delegate_methods.rs | 168 +++++++++++++++++- crates/ide-db/src/path_transform.rs | 32 +++- 2 files changed, 197 insertions(+), 3 deletions(-) diff --git a/crates/ide-assists/src/handlers/generate_delegate_methods.rs b/crates/ide-assists/src/handlers/generate_delegate_methods.rs index bbac0a26ea4..92fbdf53f6b 100644 --- a/crates/ide-assists/src/handlers/generate_delegate_methods.rs +++ b/crates/ide-assists/src/handlers/generate_delegate_methods.rs @@ -1,6 +1,7 @@ use std::collections::HashSet; use hir::{self, HasCrate, HasSource, HasVisibility}; +use ide_db::path_transform::PathTransform; use syntax::{ ast::{ self, edit_in_place::Indent, make, AstNode, HasGenericParams, HasName, HasVisibility as _, @@ -106,7 +107,10 @@ pub(crate) fn generate_delegate_methods(acc: &mut Assists, ctx: &AssistContext<' |edit| { // Create the function let method_source = match method.source(ctx.db()) { - Some(source) => source.value, + Some(source) => { + ctx.sema.parse_or_expand(source.file_id); + source.value + } None => return, }; let vis = method_source.visibility(); @@ -183,6 +187,12 @@ pub(crate) fn generate_delegate_methods(acc: &mut Assists, ctx: &AssistContext<' let assoc_items = impl_def.get_or_create_assoc_item_list(); assoc_items.add_item(f.clone().into()); + PathTransform::generic_transformation( + &ctx.sema.scope(strukt.syntax()).unwrap(), + &ctx.sema.scope(method_source.syntax()).unwrap(), + ) + .apply(f.syntax()); + if let Some(cap) = ctx.config.snippet_cap { edit.add_tabstop_before(cap, f) } @@ -454,6 +464,162 @@ impl Person { ); } + #[test] + fn test_fixes_basic_self_references() { + check_assist( + generate_delegate_methods, + r#" +struct Foo { + field: $0Bar, +} + +struct Bar; + +impl Bar { + fn bar(&self, other: Self) -> Self { + other + } +} +"#, + r#" +struct Foo { + field: Bar, +} + +impl Foo { + $0fn bar(&self, other: Bar) -> Bar { + self.field.bar(other) + } +} + +struct Bar; + +impl Bar { + fn bar(&self, other: Self) -> Self { + other + } +} +"#, + ); + } + + #[test] + fn test_fixes_nested_self_references() { + check_assist( + generate_delegate_methods, + r#" +struct Foo { + field: $0Bar, +} + +struct Bar; + +impl Bar { + fn bar(&mut self, a: (Self, [Self; 4]), b: Vec) {} +} +"#, + r#" +struct Foo { + field: Bar, +} + +impl Foo { + $0fn bar(&mut self, a: (Bar, [Bar; 4]), b: Vec) { + self.field.bar(a, b) + } +} + +struct Bar; + +impl Bar { + fn bar(&mut self, a: (Self, [Self; 4]), b: Vec) {} +} +"#, + ); + } + + #[test] + fn test_fixes_self_references_with_lifetimes_and_generics() { + check_assist( + generate_delegate_methods, + r#" +struct Foo<'a, T> { + $0field: Bar<'a, T>, +} + +struct Bar<'a, T>(&'a T); + +impl<'a, T> Bar<'a, T> { + fn bar(self, mut b: Vec<&'a Self>) -> &'a Self { + b.pop().unwrap() + } +} +"#, + r#" +struct Foo<'a, T> { + field: Bar<'a, T>, +} + +impl<'a, T> Foo<'a, T> { + $0fn bar(self, mut b: Vec<&'a Bar<'_, T>>) -> &'a Bar<'_, T> { + self.field.bar(b) + } +} + +struct Bar<'a, T>(&'a T); + +impl<'a, T> Bar<'a, T> { + fn bar(self, mut b: Vec<&'a Self>) -> &'a Self { + b.pop().unwrap() + } +} +"#, + ); + } + + #[test] + fn test_fixes_self_references_across_macros() { + check_assist( + generate_delegate_methods, + r#" +//- /bar.rs +macro_rules! test_method { + () => { + pub fn test(self, b: Bar) -> Self { + self + } + }; +} + +pub struct Bar; + +impl Bar { + test_method!(); +} + +//- /main.rs +mod bar; + +struct Foo { + $0bar: bar::Bar, +} +"#, + r#" +mod bar; + +struct Foo { + bar: bar::Bar, +} + +impl Foo { + $0pub fn test(self,b:bar::Bar) ->bar::Bar { + self.bar.test(b) + } +} +"#, + ); + } + #[test] fn test_generate_delegate_visibility() { check_assist_not_applicable( diff --git a/crates/ide-db/src/path_transform.rs b/crates/ide-db/src/path_transform.rs index fa9339f30f2..49b990172a5 100644 --- a/crates/ide-db/src/path_transform.rs +++ b/crates/ide-db/src/path_transform.rs @@ -2,7 +2,7 @@ use crate::helpers::mod_path_to_ast; use either::Either; -use hir::{AsAssocItem, HirDisplay, SemanticsScope}; +use hir::{AsAssocItem, HirDisplay, ModuleDef, SemanticsScope}; use rustc_hash::FxHashMap; use syntax::{ ast::{self, make, AstNode}, @@ -332,8 +332,36 @@ fn transform_path(&self, path: ast::Path) -> Option<()> { ted::replace(path.syntax(), subst.clone_subtree().clone_for_update()); } } + hir::PathResolution::SelfType(imp) => { + let ty = imp.self_ty(self.source_scope.db); + let ty_str = &ty + .display_source_code( + self.source_scope.db, + self.source_scope.module().into(), + true, + ) + .ok()?; + let ast_ty = make::ty(&ty_str).clone_for_update(); + + if let Some(adt) = ty.as_adt() { + if let ast::Type::PathType(path_ty) = &ast_ty { + let found_path = self.target_module.find_use_path( + self.source_scope.db.upcast(), + ModuleDef::from(adt), + false, + )?; + + if let Some(qual) = mod_path_to_ast(&found_path).qualifier() { + let res = make::path_concat(qual, path_ty.path()?).clone_for_update(); + ted::replace(path.syntax(), res.syntax()); + return Some(()); + } + } + } + + ted::replace(path.syntax(), ast_ty.syntax()); + } hir::PathResolution::Local(_) - | hir::PathResolution::SelfType(_) | hir::PathResolution::Def(_) | hir::PathResolution::BuiltinAttr(_) | hir::PathResolution::ToolModule(_) From f4349ff26ef0736ed6e37a7e491f01ee4c6a76a7 Mon Sep 17 00:00:00 2001 From: Ryan Mehri Date: Thu, 5 Oct 2023 12:45:26 -0700 Subject: [PATCH 2/3] fix: preserve where clause in delegate method --- .../src/handlers/generate_delegate_methods.rs | 49 ++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-) diff --git a/crates/ide-assists/src/handlers/generate_delegate_methods.rs b/crates/ide-assists/src/handlers/generate_delegate_methods.rs index 92fbdf53f6b..7a5d3d0859c 100644 --- a/crates/ide-assists/src/handlers/generate_delegate_methods.rs +++ b/crates/ide-assists/src/handlers/generate_delegate_methods.rs @@ -134,7 +134,7 @@ pub(crate) fn generate_delegate_methods(acc: &mut Assists, ctx: &AssistContext<' vis, fn_name, type_params, - None, + method_source.where_clause(), params, body, ret_type, @@ -464,6 +464,53 @@ impl Person { ); } + #[test] + fn test_preserve_where_clause() { + check_assist( + generate_delegate_methods, + r#" +struct Inner(T); +impl Inner { + fn get(&self) -> T + where + T: Copy, + T: PartialEq, + { + self.0 + } +} + +struct Struct { + $0field: Inner, +} +"#, + r#" +struct Inner(T); +impl Inner { + fn get(&self) -> T + where + T: Copy, + T: PartialEq, + { + self.0 + } +} + +struct Struct { + field: Inner, +} + +impl Struct { + $0fn get(&self) -> T where + T: Copy, + T: PartialEq, { + self.field.get() + } +} +"#, + ); + } + #[test] fn test_fixes_basic_self_references() { check_assist( From 7e768cbe703188bb8c126d1be6960637795e4943 Mon Sep 17 00:00:00 2001 From: Ryan Mehri Date: Mon, 9 Oct 2023 08:30:34 -0700 Subject: [PATCH 3/3] fix: prefer keeping Self if it is in the same impl def --- crates/hir-def/src/resolver.rs | 8 ++++++++ crates/hir/src/semantics.rs | 4 ++++ .../src/handlers/generate_delegate_methods.rs | 19 ++++++++----------- crates/ide-db/src/path_transform.rs | 8 ++++++++ 4 files changed, 28 insertions(+), 11 deletions(-) diff --git a/crates/hir-def/src/resolver.rs b/crates/hir-def/src/resolver.rs index 50da9ed06a0..ba0a2c0224a 100644 --- a/crates/hir-def/src/resolver.rs +++ b/crates/hir-def/src/resolver.rs @@ -588,6 +588,14 @@ pub fn body_owner(&self) -> Option { _ => None, }) } + + pub fn impl_def(&self) -> Option { + self.scopes().find_map(|scope| match scope { + Scope::ImplDefScope(def) => Some(*def), + _ => None, + }) + } + /// `expr_id` is required to be an expression id that comes after the top level expression scope in the given resolver #[must_use] pub fn update_to_inner_scope( diff --git a/crates/hir/src/semantics.rs b/crates/hir/src/semantics.rs index 46835ec04e0..55c14312071 100644 --- a/crates/hir/src/semantics.rs +++ b/crates/hir/src/semantics.rs @@ -1556,6 +1556,10 @@ pub fn extern_crates(&self) -> impl Iterator + '_ { pub fn extern_crate_decls(&self) -> impl Iterator + '_ { self.resolver.extern_crate_decls_in_scope(self.db.upcast()) } + + pub fn has_same_self_type(&self, other: &SemanticsScope<'_>) -> bool { + self.resolver.impl_def() == other.resolver.impl_def() + } } #[derive(Debug)] diff --git a/crates/ide-assists/src/handlers/generate_delegate_methods.rs b/crates/ide-assists/src/handlers/generate_delegate_methods.rs index 7a5d3d0859c..db1e0ceaec1 100644 --- a/crates/ide-assists/src/handlers/generate_delegate_methods.rs +++ b/crates/ide-assists/src/handlers/generate_delegate_methods.rs @@ -1,6 +1,6 @@ use std::collections::HashSet; -use hir::{self, HasCrate, HasSource, HasVisibility}; +use hir::{self, HasCrate, HasVisibility}; use ide_db::path_transform::PathTransform; use syntax::{ ast::{ @@ -106,11 +106,8 @@ pub(crate) fn generate_delegate_methods(acc: &mut Assists, ctx: &AssistContext<' target, |edit| { // Create the function - let method_source = match method.source(ctx.db()) { - Some(source) => { - ctx.sema.parse_or_expand(source.file_id); - source.value - } + let method_source = match ctx.sema.source(method) { + Some(source) => source.value, None => return, }; let vis = method_source.visibility(); @@ -187,11 +184,11 @@ pub(crate) fn generate_delegate_methods(acc: &mut Assists, ctx: &AssistContext<' let assoc_items = impl_def.get_or_create_assoc_item_list(); assoc_items.add_item(f.clone().into()); - PathTransform::generic_transformation( - &ctx.sema.scope(strukt.syntax()).unwrap(), - &ctx.sema.scope(method_source.syntax()).unwrap(), - ) - .apply(f.syntax()); + if let Some((target, source)) = + ctx.sema.scope(strukt.syntax()).zip(ctx.sema.scope(method_source.syntax())) + { + PathTransform::generic_transformation(&target, &source).apply(f.syntax()); + } if let Some(cap) = ctx.config.snippet_cap { edit.add_tabstop_before(cap, f) diff --git a/crates/ide-db/src/path_transform.rs b/crates/ide-db/src/path_transform.rs index 49b990172a5..fb4c0c12691 100644 --- a/crates/ide-db/src/path_transform.rs +++ b/crates/ide-db/src/path_transform.rs @@ -183,6 +183,7 @@ fn build_ctx(&self) -> Ctx<'a> { lifetime_substs, target_module, source_scope: self.source_scope, + same_self_type: self.target_scope.has_same_self_type(self.source_scope), }; ctx.transform_default_values(defaulted_params); ctx @@ -195,6 +196,7 @@ struct Ctx<'a> { lifetime_substs: FxHashMap, target_module: hir::Module, source_scope: &'a SemanticsScope<'a>, + same_self_type: bool, } fn postorder(item: &SyntaxNode) -> impl Iterator { @@ -333,6 +335,11 @@ fn transform_path(&self, path: ast::Path) -> Option<()> { } } hir::PathResolution::SelfType(imp) => { + // keep Self type if it does not need to be replaced + if self.same_self_type { + return None; + } + let ty = imp.self_ty(self.source_scope.db); let ty_str = &ty .display_source_code( @@ -349,6 +356,7 @@ fn transform_path(&self, path: ast::Path) -> Option<()> { self.source_scope.db.upcast(), ModuleDef::from(adt), false, + true, )?; if let Some(qual) = mod_path_to_ast(&found_path).qualifier() {