From af6b9428e7b6899e9e315811e897b6ad769ff681 Mon Sep 17 00:00:00 2001 From: Shivam Kunwar Date: Fri, 20 Jun 2025 13:42:00 +0530 Subject: [PATCH 1/2] Handle Self replacement contextually in inline assists --- .../ide-assists/src/handlers/inline_call.rs | 184 +++++++++++++++++- 1 file changed, 175 insertions(+), 9 deletions(-) diff --git a/crates/ide-assists/src/handlers/inline_call.rs b/crates/ide-assists/src/handlers/inline_call.rs index 6f028e58d0cd..f407af049bcc 100644 --- a/crates/ide-assists/src/handlers/inline_call.rs +++ b/crates/ide-assists/src/handlers/inline_call.rs @@ -19,7 +19,7 @@ use ide_db::{ }; use itertools::{Itertools, izip}; use syntax::{ - AstNode, NodeOrToken, SyntaxKind, + AstNode, NodeOrToken, SyntaxKind, SyntaxToken, ast::{ self, HasArgList, HasGenericArgs, Pat, PathExpr, edit::IndentLevel, edit_in_place::Indent, }, @@ -311,6 +311,80 @@ fn get_fn_params( Some(params) } +fn is_self_in_expression_context(self_token: &SyntaxToken) -> bool { + let mut current = self_token.parent(); + while let Some(node) = current { + // Check for function call expressions: Self::method() or Self(...) + if let Some(call_expr) = ast::CallExpr::cast(node.clone()) { + if let Some(expr) = call_expr.expr() { + if expr.syntax().text_range().contains_range(self_token.text_range()) { + return true; + } + } + } + + if let Some(method_call) = ast::MethodCallExpr::cast(node.clone()) { + if let Some(receiver) = method_call.receiver() { + if receiver.syntax().text_range().contains_range(self_token.text_range()) { + return true; + } + } + } + + if let Some(_path_expr) = ast::PathExpr::cast(node.clone()) { + return true; + } + + // Check for record expressions (struct construction) + if let Some(record_expr) = ast::RecordExpr::cast(node.clone()) { + if let Some(path) = record_expr.path() { + if path.syntax().text_range().contains_range(self_token.text_range()) { + return true; + } + } + } + + // Stop at certain boundaries (type/pattern contexts) + if ast::Type::cast(node.clone()).is_some() + || ast::Pat::cast(node.clone()).is_some() + || ast::RetType::cast(node.clone()).is_some() + { + return false; + } + + current = node.parent(); + } + false +} + +fn get_qualified_type_for_turbofish(ty: &ast::Type) -> String { + match ty { + ast::Type::PathType(path_type) => { + if let Some(path) = path_type.path() { + // For turbofish, we need the full path but potentially without generic args + // depending on context. For now, use the bare name. + if let Some(segment) = path.segments().last() { + if let Some(name) = segment.name_ref() { + return name.text().to_string(); + } + } + } + } + _ => {} + } + ty.syntax().text().to_string() +} + +fn have_same_self_type(source_impl: &ast::Impl, target_impl: &ast::Impl) -> bool { + match (source_impl.self_ty(), target_impl.self_ty()) { + (Some(source_ty), Some(target_ty)) => { + // Compare the textual representation of the types + source_ty.syntax().text() == target_ty.syntax().text() + } + _ => false, + } +} + fn inline( sema: &Semantics<'_, RootDatabase>, function_def_file_id: EditionedFileId, @@ -391,22 +465,46 @@ fn inline( // We should place the following code after last usage of `usages_for_locals` // because `ted::replace` will change the offset in syntax tree, which makes // `FileReference` incorrect - if let Some(imp) = + if let Some(source_impl) = sema.ancestors_with_macros(fn_body.syntax().clone()).find_map(ast::Impl::cast) { - if !node.syntax().ancestors().any(|anc| &anc == imp.syntax()) { - if let Some(t) = imp.self_ty() { - while let Some(self_tok) = body + // Check if the target (call site) is also in an impl block + let target_impl = node.syntax().ancestors().find_map(ast::Impl::cast); + + let should_replace_self = match target_impl { + Some(target_impl) => { + // Both source and target are in impl blocks + // Only replace Self if they have different Self types + !have_same_self_type(&source_impl, &target_impl) + } + None => { + // Target is not in an impl block, so we must replace Self + true + } + }; + + if should_replace_self { + if let Some(self_ty) = source_impl.self_ty() { + let self_tokens: Vec<_> = body .syntax() .descendants_with_tokens() .filter_map(NodeOrToken::into_token) - .find(|tok| tok.kind() == SyntaxKind::SELF_TYPE_KW) - { - let replace_with = t.clone_subtree().syntax().clone_for_update(); - ted::replace(self_tok, replace_with); + .filter(|tok| tok.kind() == SyntaxKind::SELF_TYPE_KW) + .collect(); + + // Replace each Self token based on its context + for self_tok in self_tokens { + let replacement = if is_self_in_expression_context(&self_tok) { + let qualified_name = get_qualified_type_for_turbofish(&self_ty); + make::name_ref(&qualified_name).syntax().clone_for_update() + } else { + self_ty.clone_subtree().syntax().clone_for_update() + }; + ted::replace(self_tok, replacement); } } } + // If same Self type context, leave Self as-is (it remains valid) } let mut func_let_vars: BTreeSet = BTreeSet::new(); @@ -1832,4 +1930,72 @@ fn f() { "#, ); } + + #[test] + fn inline_call_generic_self_constructor() { + check_assist( + inline_call, + r#" +struct Generic(T); + +impl Generic { + fn new(value: T) -> Self { + Self(value) + } +} + +fn main() { + let x = Generic::::new$0(42); +} +"#, + r#" +struct Generic(T); + +impl Generic { + fn new(value: T) -> Self { + Self(value) + } +} + +fn main() { + let x = Generic(42); +} +"#, + ) + } + + #[test] + fn inline_call_generic_self_type_position() { + check_assist( + inline_call, + r#" +struct Generic(T); + +impl Generic { + fn identity(self) -> Self { + self + } +} + +fn main() { + let x = Generic(42); + let y = x.identity$0(); +} +"#, + r#" +struct Generic(T); + +impl Generic { + fn identity(self) -> Self { + self + } +} + +fn main() { + let x = Generic(42); + let y = x; +} +"#, + ) + } } From 781ee7a77f5e1625ca31b14f5929d1d6a9203ca2 Mon Sep 17 00:00:00 2001 From: Shivam Kunwar Date: Fri, 20 Jun 2025 15:42:21 +0530 Subject: [PATCH 2/2] Use PathTransform for context-aware Self replacement in inline assists --- .../ide-assists/src/handlers/inline_call.rs | 137 ++---------------- crates/ide-db/src/path_transform.rs | 54 ++++++- 2 files changed, 63 insertions(+), 128 deletions(-) diff --git a/crates/ide-assists/src/handlers/inline_call.rs b/crates/ide-assists/src/handlers/inline_call.rs index f407af049bcc..be244a619eb5 100644 --- a/crates/ide-assists/src/handlers/inline_call.rs +++ b/crates/ide-assists/src/handlers/inline_call.rs @@ -19,7 +19,7 @@ use ide_db::{ }; use itertools::{Itertools, izip}; use syntax::{ - AstNode, NodeOrToken, SyntaxKind, SyntaxToken, + AstNode, ast::{ self, HasArgList, HasGenericArgs, Pat, PathExpr, edit::IndentLevel, edit_in_place::Indent, }, @@ -311,80 +311,6 @@ fn get_fn_params( Some(params) } -fn is_self_in_expression_context(self_token: &SyntaxToken) -> bool { - let mut current = self_token.parent(); - while let Some(node) = current { - // Check for function call expressions: Self::method() or Self(...) - if let Some(call_expr) = ast::CallExpr::cast(node.clone()) { - if let Some(expr) = call_expr.expr() { - if expr.syntax().text_range().contains_range(self_token.text_range()) { - return true; - } - } - } - - if let Some(method_call) = ast::MethodCallExpr::cast(node.clone()) { - if let Some(receiver) = method_call.receiver() { - if receiver.syntax().text_range().contains_range(self_token.text_range()) { - return true; - } - } - } - - if let Some(_path_expr) = ast::PathExpr::cast(node.clone()) { - return true; - } - - // Check for record expressions (struct construction) - if let Some(record_expr) = ast::RecordExpr::cast(node.clone()) { - if let Some(path) = record_expr.path() { - if path.syntax().text_range().contains_range(self_token.text_range()) { - return true; - } - } - } - - // Stop at certain boundaries (type/pattern contexts) - if ast::Type::cast(node.clone()).is_some() - || ast::Pat::cast(node.clone()).is_some() - || ast::RetType::cast(node.clone()).is_some() - { - return false; - } - - current = node.parent(); - } - false -} - -fn get_qualified_type_for_turbofish(ty: &ast::Type) -> String { - match ty { - ast::Type::PathType(path_type) => { - if let Some(path) = path_type.path() { - // For turbofish, we need the full path but potentially without generic args - // depending on context. For now, use the bare name. - if let Some(segment) = path.segments().last() { - if let Some(name) = segment.name_ref() { - return name.text().to_string(); - } - } - } - } - _ => {} - } - ty.syntax().text().to_string() -} - -fn have_same_self_type(source_impl: &ast::Impl, target_impl: &ast::Impl) -> bool { - match (source_impl.self_ty(), target_impl.self_ty()) { - (Some(source_ty), Some(target_ty)) => { - // Compare the textual representation of the types - source_ty.syntax().text() == target_ty.syntax().text() - } - _ => false, - } -} - fn inline( sema: &Semantics<'_, RootDatabase>, function_def_file_id: EditionedFileId, @@ -462,51 +388,6 @@ fn inline( } } - // We should place the following code after last usage of `usages_for_locals` - // because `ted::replace` will change the offset in syntax tree, which makes - // `FileReference` incorrect - if let Some(source_impl) = - sema.ancestors_with_macros(fn_body.syntax().clone()).find_map(ast::Impl::cast) - { - // Check if the target (call site) is also in an impl block - let target_impl = node.syntax().ancestors().find_map(ast::Impl::cast); - - let should_replace_self = match target_impl { - Some(target_impl) => { - // Both source and target are in impl blocks - // Only replace Self if they have different Self types - !have_same_self_type(&source_impl, &target_impl) - } - None => { - // Target is not in an impl block, so we must replace Self - true - } - }; - - if should_replace_self { - if let Some(self_ty) = source_impl.self_ty() { - let self_tokens: Vec<_> = body - .syntax() - .descendants_with_tokens() - .filter_map(NodeOrToken::into_token) - .filter(|tok| tok.kind() == SyntaxKind::SELF_TYPE_KW) - .collect(); - - // Replace each Self token based on its context - for self_tok in self_tokens { - let replacement = if is_self_in_expression_context(&self_tok) { - let qualified_name = get_qualified_type_for_turbofish(&self_ty); - make::name_ref(&qualified_name).syntax().clone_for_update() - } else { - self_ty.clone_subtree().syntax().clone_for_update() - }; - ted::replace(self_tok, replacement); - } - } - } - // If same Self type context, leave Self as-is (it remains valid) - } - let mut func_let_vars: BTreeSet = BTreeSet::new(); // grab all of the local variable declarations in the function @@ -632,11 +513,17 @@ fn inline( } } - if let Some(generic_arg_list) = generic_arg_list.clone() { - if let Some((target, source)) = &sema.scope(node.syntax()).zip(sema.scope(fn_body.syntax())) - { - PathTransform::function_call(target, source, function, generic_arg_list) - .apply(body.syntax()); + // Apply PathTransform for path transformations when needed + if let Some((target, source)) = &sema.scope(node.syntax()).zip(sema.scope(fn_body.syntax())) { + let needs_transformation = generic_arg_list.is_some() || !target.has_same_self_type(source); + + if needs_transformation { + let path_transform = if let Some(generic_arg_list) = generic_arg_list.clone() { + PathTransform::function_call(target, source, function, generic_arg_list) + } else { + PathTransform::generic_transformation(target, source) + }; + path_transform.apply(body.syntax()); } } diff --git a/crates/ide-db/src/path_transform.rs b/crates/ide-db/src/path_transform.rs index 232648af661f..f0e0840db182 100644 --- a/crates/ide-db/src/path_transform.rs +++ b/crates/ide-db/src/path_transform.rs @@ -248,6 +248,47 @@ fn preorder_rev(item: &SyntaxNode) -> impl Iterator { x.into_iter().rev() } +fn is_path_in_expression_context(path: &ast::Path) -> bool { + let mut current = path.syntax().parent(); + while let Some(node) = current { + // Expression contexts where we need bare type names + if let Some(call_expr) = ast::CallExpr::cast(node.clone()) { + if let Some(expr) = call_expr.expr() { + if expr.syntax().text_range().contains_range(path.syntax().text_range()) { + return true; + } + } + } + if ast::PathExpr::cast(node.clone()).is_some() { + return true; + } + if let Some(record_expr) = ast::RecordExpr::cast(node.clone()) { + if let Some(record_path) = record_expr.path() { + if record_path.syntax().text_range().contains_range(path.syntax().text_range()) { + return true; + } + } + } + // Stop at type/pattern boundaries + if ast::Type::cast(node.clone()).is_some() + || ast::Pat::cast(node.clone()).is_some() + || ast::RetType::cast(node.clone()).is_some() + { + return false; + } + current = node.parent(); + } + false +} + +fn get_bare_type_name(ty_str: &str) -> String { + if let Some(angle_pos) = ty_str.find('<') { + ty_str[..angle_pos].to_owned() + } else { + ty_str.to_owned() + } +} + impl Ctx<'_> { fn apply(&self, item: &SyntaxNode) { // `transform_path` may update a node's parent and that would break the @@ -413,10 +454,17 @@ impl Ctx<'_> { true, ) .ok()?; - let ast_ty = make::ty(ty_str).clone_for_update(); + + // Context-aware replacement + let replacement = if is_path_in_expression_context(&path) { + let bare_name = get_bare_type_name(ty_str); + make::ty(&bare_name).clone_for_update() + } else { + make::ty(ty_str).clone_for_update() + }; if let Some(adt) = ty.as_adt() { - if let ast::Type::PathType(path_ty) = &ast_ty { + if let ast::Type::PathType(path_ty) = &replacement { let cfg = ImportPathConfig { prefer_no_std: false, prefer_prelude: true, @@ -439,7 +487,7 @@ impl Ctx<'_> { } } - ted::replace(path.syntax(), ast_ty.syntax()); + ted::replace(path.syntax(), replacement.syntax()); } hir::PathResolution::Local(_) | hir::PathResolution::Def(_)