diff --git a/crates/base_db/src/fixture.rs b/crates/base_db/src/fixture.rs index 27be3746b71..cc1de54dfb3 100644 --- a/crates/base_db/src/fixture.rs +++ b/crates/base_db/src/fixture.rs @@ -161,7 +161,8 @@ impl ChangeFixture { } if crates.is_empty() { - let crate_root = default_crate_root.unwrap(); + let crate_root = default_crate_root + .expect("missing default crate root, specify a main.rs or lib.rs"); crate_graph.add_crate_root( crate_root, Edition::CURRENT, diff --git a/crates/ide_assists/src/handlers/inline_call.rs b/crates/ide_assists/src/handlers/inline_call.rs index 33e029c236f..9927ceb74ec 100644 --- a/crates/ide_assists/src/handlers/inline_call.rs +++ b/crates/ide_assists/src/handlers/inline_call.rs @@ -1,10 +1,18 @@ use ast::make; -use hir::{HasSource, PathResolution, TypeInfo}; -use ide_db::{defs::Definition, path_transform::PathTransform, search::FileReference}; -use itertools::izip; +use either::Either; +use hir::{db::HirDatabase, HasSource, PathResolution, Semantics, TypeInfo}; +use ide_db::{ + base_db::{FileId, FileRange}, + defs::Definition, + helpers::insert_use::remove_path_if_in_use_stmt, + path_transform::PathTransform, + search::{FileReference, SearchScope}, + RootDatabase, +}; +use itertools::{izip, Itertools}; use syntax::{ ast::{self, edit_in_place::Indent, ArgListOwner}, - ted, AstNode, + ted, AstNode, SyntaxNode, }; use crate::{ @@ -12,6 +20,139 @@ use crate::{ AssistId, AssistKind, }; +// Assist: inline_into_callers +// +// Inline a function or method body into all of its callers where possible, creating a `let` statement per parameter +// unless the parameter can be inlined. The parameter will be inlined either if it the supplied argument is a simple local +// or if the parameter is only accessed inside the function body once. +// If all calls can be inlined the function will be removed. +// +// ``` +// fn print(_: &str) {} +// fn foo$0(word: &str) { +// if !word.is_empty() { +// print(word); +// } +// } +// fn bar() { +// foo("안녕하세요"); +// foo("여러분"); +// } +// ``` +// -> +// ``` +// fn print(_: &str) {} +// +// fn bar() { +// { +// let word = "안녕하세요"; +// if !word.is_empty() { +// print(word); +// } +// }; +// { +// let word = "여러분"; +// if !word.is_empty() { +// print(word); +// } +// }; +// } +// ``` +pub(crate) fn inline_into_callers(acc: &mut Assists, ctx: &AssistContext) -> Option<()> { + let def_file = ctx.frange.file_id; + let name = ctx.find_node_at_offset::()?; + let ast_func = name.syntax().parent().and_then(ast::Fn::cast)?; + let func_body = ast_func.body()?; + let param_list = ast_func.param_list()?; + + let function = ctx.sema.to_def(&ast_func)?; + + let params = get_fn_params(ctx.sema.db, function, ¶m_list)?; + + let usages = Definition::ModuleDef(hir::ModuleDef::Function(function)).usages(&ctx.sema); + if !usages.at_least_one() { + return None; + } + + let is_recursive_fn = usages + .clone() + .in_scope(SearchScope::file_range(FileRange { + file_id: def_file, + range: func_body.syntax().text_range(), + })) + .at_least_one(); + if is_recursive_fn { + cov_mark::hit!(inline_into_callers_recursive); + return None; + } + + acc.add( + AssistId("inline_into_callers", AssistKind::RefactorInline), + "Inline into all callers", + name.syntax().text_range(), + |builder| { + let mut usages = usages.all(); + let current_file_usage = usages.references.remove(&def_file); + + let mut remove_def = true; + let mut inline_refs_for_file = |file_id, refs: Vec| { + builder.edit_file(file_id); + let count = refs.len(); + // The collects are required as we are otherwise iterating while mutating 🙅‍♀️🙅‍♂️ + let (name_refs, name_refs_use): (Vec<_>, Vec<_>) = refs + .into_iter() + .filter_map(|file_ref| match file_ref.name { + ast::NameLike::NameRef(name_ref) => Some(name_ref), + _ => None, + }) + .partition_map(|name_ref| { + match name_ref.syntax().ancestors().find_map(ast::UseTree::cast) { + Some(use_tree) => Either::Right(builder.make_mut(use_tree)), + None => Either::Left(name_ref), + } + }); + let call_infos: Vec<_> = name_refs + .into_iter() + .filter_map(CallInfo::from_name_ref) + .map(|call_info| { + let mut_node = builder.make_syntax_mut(call_info.node.syntax().clone()); + (call_info, mut_node) + }) + .collect(); + let replaced = call_infos + .into_iter() + .map(|(call_info, mut_node)| { + let replacement = + inline(&ctx.sema, def_file, function, &func_body, ¶ms, &call_info); + ted::replace(mut_node, replacement.syntax()); + }) + .count(); + if replaced + name_refs_use.len() == count { + // we replaced all usages in this file, so we can remove the imports + name_refs_use.into_iter().for_each(|use_tree| { + if let Some(path) = use_tree.path() { + remove_path_if_in_use_stmt(&path); + } + }) + } else { + remove_def = false; + } + }; + for (file_id, refs) in usages.into_iter() { + inline_refs_for_file(file_id, refs); + } + if let Some(refs) = current_file_usage { + inline_refs_for_file(def_file, refs); + } else { + builder.edit_file(def_file); + } + if remove_def { + builder.delete(ast_func.syntax().text_range()); + } + }, + ) +} + // Assist: inline_call // // Inlines a function or method body creating a `let` statement per parameter unless the parameter @@ -34,54 +175,117 @@ use crate::{ // } // ``` pub(crate) fn inline_call(acc: &mut Assists, ctx: &AssistContext) -> Option<()> { - let (label, function, arguments, generic_arg_list, expr) = - if let Some(path_expr) = ctx.find_node_at_offset::() { - let call = path_expr.syntax().parent().and_then(ast::CallExpr::cast)?; - let path = path_expr.path()?; - + let name_ref: ast::NameRef = ctx.find_node_at_offset()?; + let call_info = CallInfo::from_name_ref(name_ref.clone())?; + let (function, label) = match &call_info.node { + CallExprNode::Call(call) => { + let path = match call.expr()? { + ast::Expr::PathExpr(path) => path.path(), + _ => None, + }?; let function = match ctx.sema.resolve_path(&path)? { - PathResolution::Def(hir::ModuleDef::Function(f)) - | PathResolution::AssocItem(hir::AssocItem::Function(f)) => f, + PathResolution::Def(hir::ModuleDef::Function(f)) => f, + PathResolution::AssocItem(hir::AssocItem::Function(f)) => f, _ => return None, }; - ( - format!("Inline `{}`", path), - function, - call.arg_list()?.args().collect(), - path.segment().and_then(|it| it.generic_arg_list()), - ast::Expr::CallExpr(call), - ) - } else { - let name_ref: ast::NameRef = ctx.find_node_at_offset()?; - let call = name_ref.syntax().parent().and_then(ast::MethodCallExpr::cast)?; - let receiver = call.receiver()?; - let function = ctx.sema.resolve_method_call(&call)?; - let mut arguments = vec![receiver]; - arguments.extend(call.arg_list()?.args()); - ( - format!("Inline `{}`", name_ref), - function, - arguments, - call.generic_arg_list(), - ast::Expr::MethodCallExpr(call), - ) - }; + (function, format!("Inline `{}`", path)) + } + CallExprNode::MethodCallExpr(call) => { + (ctx.sema.resolve_method_call(call)?, format!("Inline `{}`", name_ref)) + } + }; - inline_(acc, ctx, label, function, arguments, expr, generic_arg_list) + let fn_source = function.source(ctx.db())?; + let fn_body = fn_source.value.body()?; + let param_list = fn_source.value.param_list()?; + + let FileRange { file_id, range } = fn_source.syntax().original_file_range(ctx.sema.db); + if file_id == ctx.frange.file_id && range.contains(ctx.frange.range.start()) { + cov_mark::hit!(inline_call_recursive); + return None; + } + let params = get_fn_params(ctx.sema.db, function, ¶m_list)?; + + if call_info.arguments.len() != params.len() { + // Can't inline the function because they've passed the wrong number of + // arguments to this function + cov_mark::hit!(inline_call_incorrect_number_of_arguments); + return None; + } + + let syntax = call_info.node.syntax().clone(); + acc.add( + AssistId("inline_call", AssistKind::RefactorInline), + label, + syntax.text_range(), + |builder| { + let replacement = inline(&ctx.sema, file_id, function, &fn_body, ¶ms, &call_info); + + builder.replace_ast( + match call_info.node { + CallExprNode::Call(it) => ast::Expr::CallExpr(it), + CallExprNode::MethodCallExpr(it) => ast::Expr::MethodCallExpr(it), + }, + replacement, + ); + }, + ) } -pub(crate) fn inline_( - acc: &mut Assists, - ctx: &AssistContext, - label: String, - function: hir::Function, - arg_list: Vec, - expr: ast::Expr, +enum CallExprNode { + Call(ast::CallExpr), + MethodCallExpr(ast::MethodCallExpr), +} + +impl CallExprNode { + fn syntax(&self) -> &SyntaxNode { + match self { + CallExprNode::Call(it) => it.syntax(), + CallExprNode::MethodCallExpr(it) => it.syntax(), + } + } +} + +struct CallInfo { + node: CallExprNode, + arguments: Vec, generic_arg_list: Option, -) -> Option<()> { - let hir::InFile { value: function_source, file_id } = function.source(ctx.db())?; - let param_list = function_source.param_list()?; - let mut assoc_fn_params = function.assoc_fn_params(ctx.sema.db).into_iter(); +} + +impl CallInfo { + fn from_name_ref(name_ref: ast::NameRef) -> Option { + let parent = name_ref.syntax().parent()?; + if let Some(call) = ast::MethodCallExpr::cast(parent.clone()) { + let receiver = call.receiver()?; + let mut arguments = vec![receiver]; + arguments.extend(call.arg_list()?.args()); + Some(CallInfo { + generic_arg_list: call.generic_arg_list(), + node: CallExprNode::MethodCallExpr(call), + arguments, + }) + } else if let Some(segment) = ast::PathSegment::cast(parent) { + let path = segment.syntax().parent().and_then(ast::Path::cast)?; + let path = path.syntax().parent().and_then(ast::PathExpr::cast)?; + let call = path.syntax().parent().and_then(ast::CallExpr::cast)?; + + Some(CallInfo { + arguments: call.arg_list()?.args().collect(), + node: CallExprNode::Call(call), + generic_arg_list: segment.generic_arg_list(), + }) + } else { + None + } + } +} + +fn get_fn_params( + db: &dyn HirDatabase, + function: hir::Function, + param_list: &ast::ParamList, +) -> Option, hir::Param)>> { + let mut assoc_fn_params = function.assoc_fn_params(db).into_iter(); let mut params = Vec::new(); if let Some(self_param) = param_list.self_param() { @@ -101,131 +305,116 @@ pub(crate) fn inline_( params.push((param.pat()?, param.ty(), assoc_fn_params.next()?)); } - if arg_list.len() != params.len() { - // Can't inline the function because they've passed the wrong number of - // arguments to this function - cov_mark::hit!(inline_call_incorrect_number_of_arguments); - return None; + Some(params) +} + +fn inline( + sema: &Semantics, + function_def_file_id: FileId, + function: hir::Function, + fn_body: &ast::BlockExpr, + params: &[(ast::Pat, Option, hir::Param)], + CallInfo { node, arguments, generic_arg_list }: &CallInfo, +) -> ast::Expr { + let body = fn_body.clone_for_update(); + let usages_for_locals = |local| { + Definition::Local(local) + .usages(&sema) + .all() + .references + .remove(&function_def_file_id) + .unwrap_or_default() + .into_iter() + }; + let param_use_nodes: Vec> = params + .iter() + .map(|(pat, _, param)| { + if !matches!(pat, ast::Pat::IdentPat(pat) if pat.is_simple_ident()) { + return Vec::new(); + } + usages_for_locals(param.as_local(sema.db)) + .map(|FileReference { name, range, .. }| match name { + ast::NameLike::NameRef(_) => body + .syntax() + .covering_element(range) + .ancestors() + .nth(3) + .and_then(ast::PathExpr::cast), + _ => None, + }) + .collect::>>() + .unwrap_or_default() + }) + .collect(); + if function.self_param(sema.db).is_some() { + let this = || make::name_ref("this").syntax().clone_for_update(); + usages_for_locals(params[0].2.as_local(sema.db)) + .flat_map(|FileReference { name, range, .. }| match name { + ast::NameLike::NameRef(_) => Some(body.syntax().covering_element(range)), + _ => None, + }) + .for_each(|it| { + ted::replace(it, &this()); + }) + } + // Inline parameter expressions or generate `let` statements depending on whether inlining works or not. + for ((pat, param_ty, _), usages, expr) in izip!(params, param_use_nodes, arguments).rev() { + let expr_is_name_ref = matches!(&expr, + ast::Expr::PathExpr(expr) + if expr.path().and_then(|path| path.as_single_name_ref()).is_some() + ); + match &*usages { + // inline single use closure arguments + [usage] + if matches!(expr, ast::Expr::ClosureExpr(_)) + && usage.syntax().parent().and_then(ast::Expr::cast).is_some() => + { + cov_mark::hit!(inline_call_inline_closure); + let expr = make::expr_paren(expr.clone()); + ted::replace(usage.syntax(), expr.syntax().clone_for_update()); + } + // inline single use literals + [usage] if matches!(expr, ast::Expr::Literal(_)) => { + cov_mark::hit!(inline_call_inline_literal); + ted::replace(usage.syntax(), expr.syntax().clone_for_update()); + } + // inline direct local arguments + [_, ..] if expr_is_name_ref => { + cov_mark::hit!(inline_call_inline_locals); + usages.into_iter().for_each(|usage| { + ted::replace(usage.syntax(), &expr.syntax().clone_for_update()); + }); + } + // cant inline, emit a let statement + _ => { + let ty = + sema.type_of_expr(expr).filter(TypeInfo::has_adjustment).and(param_ty.clone()); + body.push_front( + make::let_stmt(pat.clone(), ty, Some(expr.clone())).clone_for_update().into(), + ) + } + } + } + if let Some(generic_arg_list) = generic_arg_list.clone() { + PathTransform::function_call( + &sema.scope(node.syntax()), + &sema.scope(fn_body.syntax()), + function, + generic_arg_list, + ) + .apply(body.syntax()); } - let fn_body = function_source.body()?; + let original_indentation = match node { + CallExprNode::Call(it) => it.indent_level(), + CallExprNode::MethodCallExpr(it) => it.indent_level(), + }; + body.reindent_to(original_indentation); - acc.add( - AssistId("inline_call", AssistKind::RefactorInline), - label, - expr.syntax().text_range(), - |builder| { - let body = fn_body.clone_for_update(); - - let file_id = file_id.original_file(ctx.sema.db); - let usages_for_locals = |local| { - Definition::Local(local) - .usages(&ctx.sema) - .all() - .references - .remove(&file_id) - .unwrap_or_default() - .into_iter() - }; - // Contains the nodes of usages of parameters. - // If the inner Vec for a parameter is empty it either means there are no usages or that the parameter - // has a pattern that does not allow inlining - let param_use_nodes: Vec> = params - .iter() - .map(|(pat, _, param)| { - if !matches!(pat, ast::Pat::IdentPat(pat) if pat.is_simple_ident()) { - return Vec::new(); - } - usages_for_locals(param.as_local(ctx.sema.db)) - .map(|FileReference { name, range, .. }| match name { - ast::NameLike::NameRef(_) => body - .syntax() - .covering_element(range) - .ancestors() - .nth(3) - .and_then(ast::PathExpr::cast), - _ => None, - }) - .collect::>>() - .unwrap_or_default() - }) - .collect(); - - // Rewrite `self` to `this` - if param_list.self_param().is_some() { - let this = || make::name_ref("this").syntax().clone_for_update(); - usages_for_locals(params[0].2.as_local(ctx.sema.db)) - .flat_map(|FileReference { name, range, .. }| match name { - ast::NameLike::NameRef(_) => Some(body.syntax().covering_element(range)), - _ => None, - }) - .for_each(|it| { - ted::replace(it, &this()); - }) - } - - // Inline parameter expressions or generate `let` statements depending on whether inlining works or not. - for ((pat, param_ty, _), usages, expr) in izip!(params, param_use_nodes, arg_list).rev() - { - let expr_is_name_ref = matches!(&expr, - ast::Expr::PathExpr(expr) - if expr.path().and_then(|path| path.as_single_name_ref()).is_some() - ); - match &*usages { - // inline single use closure arguments - [usage] - if matches!(expr, ast::Expr::ClosureExpr(_)) - && usage.syntax().parent().and_then(ast::Expr::cast).is_some() => - { - cov_mark::hit!(inline_call_inline_closure); - let expr = make::expr_paren(expr); - ted::replace(usage.syntax(), expr.syntax().clone_for_update()); - } - // inline single use literals - [usage] if matches!(expr, ast::Expr::Literal(_)) => { - cov_mark::hit!(inline_call_inline_literal); - ted::replace(usage.syntax(), expr.syntax().clone_for_update()); - } - // inline direct local arguments - [_, ..] if expr_is_name_ref => { - cov_mark::hit!(inline_call_inline_locals); - usages.into_iter().for_each(|usage| { - ted::replace(usage.syntax(), &expr.syntax().clone_for_update()); - }); - } - // cant inline, emit a let statement - _ => { - let ty = ctx - .sema - .type_of_expr(&expr) - .filter(TypeInfo::has_adjustment) - .and(param_ty); - body.push_front( - make::let_stmt(pat, ty, Some(expr)).clone_for_update().into(), - ) - } - } - } - if let Some(generic_arg_list) = generic_arg_list { - PathTransform::function_call( - &ctx.sema.scope(expr.syntax()), - &ctx.sema.scope(fn_body.syntax()), - function, - generic_arg_list, - ) - .apply(body.syntax()); - } - - let original_indentation = expr.indent_level(); - body.reindent_to(original_indentation); - - let replacement = match body.tail_expr() { - Some(expr) if body.statements().next().is_none() => expr, - _ => ast::Expr::BlockExpr(body), - }; - builder.replace_ast(expr, replacement); - }, - ) + match body.tail_expr() { + Some(expr) if body.statements().next().is_none() => expr, + _ => ast::Expr::BlockExpr(body), + } } #[cfg(test)] @@ -678,7 +867,7 @@ fn foo() { fn bar() {} fn main() { - foo::$0(); + foo$0::(); } "#, r#" @@ -691,6 +880,144 @@ fn bar() {} fn main() { bar::(); } +"#, + ); + } + + #[test] + fn inline_callers() { + check_assist( + inline_into_callers, + r#" +fn do_the_math$0(b: u32) -> u32 { + let foo = 10; + foo * b + foo +} +fn foo() { + do_the_math(0); + let bar = 10; + do_the_math(bar); +} +"#, + r#" + +fn foo() { + { + let foo = 10; + foo * 0 + foo + }; + let bar = 10; + { + let foo = 10; + foo * bar + foo + }; +} +"#, + ); + } + + #[test] + fn inline_callers_across_files() { + check_assist( + inline_into_callers, + r#" +//- /lib.rs +mod foo; +fn do_the_math$0(b: u32) -> u32 { + let foo = 10; + foo * b + foo +} +//- /foo.rs +use super::do_the_math; +fn foo() { + do_the_math(0); + let bar = 10; + do_the_math(bar); +} +"#, + r#" +//- /lib.rs +mod foo; + +//- /foo.rs +fn foo() { + { + let foo = 10; + foo * 0 + foo + }; + let bar = 10; + { + let foo = 10; + foo * bar + foo + }; +} +"#, + ); + } + + #[test] + fn inline_callers_across_files_with_def_file() { + check_assist( + inline_into_callers, + r#" +//- /lib.rs +mod foo; +fn do_the_math$0(b: u32) -> u32 { + let foo = 10; + foo * b + foo +} +fn bar(a: u32, b: u32) -> u32 { + do_the_math(0); +} +//- /foo.rs +use super::do_the_math; +fn foo() { + do_the_math(0); +} +"#, + r#" +//- /lib.rs +mod foo; + +fn bar(a: u32, b: u32) -> u32 { + { + let foo = 10; + foo * 0 + foo + }; +} +//- /foo.rs +fn foo() { + { + let foo = 10; + foo * 0 + foo + }; +} +"#, + ); + } + + #[test] + fn inline_callers_recursive() { + cov_mark::check!(inline_into_callers_recursive); + check_assist_not_applicable( + inline_into_callers, + r#" +fn foo$0() { + foo(); +} +"#, + ); + } + + #[test] + fn inline_call_recursive() { + cov_mark::check!(inline_call_recursive); + check_assist_not_applicable( + inline_call, + r#" +fn foo() { + foo$0(); +} "#, ); } diff --git a/crates/ide_assists/src/lib.rs b/crates/ide_assists/src/lib.rs index 53348c26d6f..255ff47b801 100644 --- a/crates/ide_assists/src/lib.rs +++ b/crates/ide_assists/src/lib.rs @@ -217,6 +217,7 @@ mod handlers { generate_is_empty_from_len::generate_is_empty_from_len, generate_new::generate_new, inline_call::inline_call, + inline_call::inline_into_callers, inline_local_variable::inline_local_variable, introduce_named_generic::introduce_named_generic, introduce_named_lifetime::introduce_named_lifetime, diff --git a/crates/ide_assists/src/tests/generated.rs b/crates/ide_assists/src/tests/generated.rs index 831f63d4762..87b3be34984 100644 --- a/crates/ide_assists/src/tests/generated.rs +++ b/crates/ide_assists/src/tests/generated.rs @@ -1051,6 +1051,43 @@ fn foo(name: Option<&str>) { ) } +#[test] +fn doctest_inline_into_callers() { + check_doc_test( + "inline_into_callers", + r#####" +fn print(_: &str) {} +fn foo$0(word: &str) { + if !word.is_empty() { + print(word); + } +} +fn bar() { + foo("안녕하세요"); + foo("여러분"); +} +"#####, + r#####" +fn print(_: &str) {} + +fn bar() { + { + let word = "안녕하세요"; + if !word.is_empty() { + print(word); + } + }; + { + let word = "여러분"; + if !word.is_empty() { + print(word); + } + }; +} +"#####, + ) +} + #[test] fn doctest_inline_local_variable() { check_doc_test( diff --git a/crates/ide_db/src/helpers/insert_use.rs b/crates/ide_db/src/helpers/insert_use.rs index 80cc2ca815e..8acad6a8ca4 100644 --- a/crates/ide_db/src/helpers/insert_use.rs +++ b/crates/ide_db/src/helpers/insert_use.rs @@ -220,6 +220,23 @@ pub fn insert_use(scope: &ImportScope, path: ast::Path, cfg: &InsertUseConfig) { insert_use_(scope, &path, cfg.group, use_item); } +pub fn remove_path_if_in_use_stmt(path: &ast::Path) { + // FIXME: improve this + if path.parent_path().is_some() { + return; + } + if let Some(use_tree) = path.syntax().parent().and_then(ast::UseTree::cast) { + if use_tree.use_tree_list().is_some() || use_tree.star_token().is_some() { + return; + } + if let Some(use_) = use_tree.syntax().parent().and_then(ast::Use::cast) { + use_.remove(); + return; + } + use_tree.remove(); + } +} + #[derive(Eq, PartialEq, PartialOrd, Ord)] enum ImportGroup { // the order here defines the order of new group inserts diff --git a/crates/ide_db/src/search.rs b/crates/ide_db/src/search.rs index a84e6b3ba40..00acfde243e 100644 --- a/crates/ide_db/src/search.rs +++ b/crates/ide_db/src/search.rs @@ -315,6 +315,7 @@ impl Definition { } } +#[derive(Clone)] pub struct FindUsages<'a> { def: Definition, sema: &'a Semantics<'a, RootDatabase>, @@ -341,7 +342,7 @@ impl<'a> FindUsages<'a> { self } - pub fn at_least_one(self) -> bool { + pub fn at_least_one(&self) -> bool { let mut found = false; self.search(&mut |_, _| { found = true; @@ -359,7 +360,7 @@ impl<'a> FindUsages<'a> { res } - fn search(self, sink: &mut dyn FnMut(FileId, FileReference) -> bool) { + fn search(&self, sink: &mut dyn FnMut(FileId, FileReference) -> bool) { let _p = profile::span("FindUsages:search"); let sema = self.sema;