From d308f17a211068971858551a1bd9149abdf09d35 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Sat, 3 Jul 2021 20:05:00 +0200 Subject: [PATCH 1/3] Inline parameters in `inline_call` if possible --- .../ide_assists/src/handlers/inline_call.rs | 152 +++++++++++------- crates/syntax/src/ast/edit_in_place.rs | 6 + crates/syntax/src/ast/node_ext.rs | 9 ++ 3 files changed, 111 insertions(+), 56 deletions(-) diff --git a/crates/ide_assists/src/handlers/inline_call.rs b/crates/ide_assists/src/handlers/inline_call.rs index 93f8edb1a82..9b46b8f2fee 100644 --- a/crates/ide_assists/src/handlers/inline_call.rs +++ b/crates/ide_assists/src/handlers/inline_call.rs @@ -1,5 +1,7 @@ use ast::make; use hir::{HasSource, PathResolution}; +use ide_db::{defs::Definition, search::FileReference}; +use itertools::izip; use syntax::{ ast::{self, edit::AstNodeEdit, ArgListOwner}, ted, AstNode, @@ -69,23 +71,25 @@ pub(crate) fn inline_( arg_list: Vec, expr: ast::Expr, ) -> Option<()> { - let hir::InFile { value: function_source, .. } = function.source(ctx.db())?; + 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(); let mut params = Vec::new(); if let Some(self_param) = param_list.self_param() { // FIXME this should depend on the receiver as well as the self_param - params.push( + params.push(( make::ident_pat( self_param.amp_token().is_some(), self_param.mut_token().is_some(), make::name("this"), ) .into(), - ); + assoc_fn_params.next()?, + )); } for param in param_list.params() { - params.push(param.pat()?); + params.push((param.pat()?, assoc_fn_params.next()?)); } if arg_list.len() != params.len() { @@ -95,8 +99,6 @@ pub(crate) fn inline_( return None; } - let new_bindings = params.into_iter().zip(arg_list); - let body = function_source.body()?; acc.add( @@ -104,32 +106,88 @@ pub(crate) fn inline_( label, expr.syntax().text_range(), |builder| { - // FIXME: emit type ascriptions when a coercion happens? - // FIXME: dont create locals when its not required - let statements = new_bindings - .map(|(pattern, value)| make::let_stmt(pattern, Some(value)).into()) - .chain(body.statements()); + let body = 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) + .filter(|it| ast::PathExpr::can_cast(it.kind())), + _ => 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].1.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, _), usages, expr) in izip!(params, param_use_nodes, arg_list).rev() { + match &*usages { + // inline single use parameters + [usage] => { + ted::replace(usage, expr.syntax().clone_for_update()); + } + // inline parameters whose expression is a simple local reference + [_, ..] + if matches!(&expr, + ast::Expr::PathExpr(expr) + if expr.path().and_then(|path| path.as_single_name_ref()).is_some() + ) => + { + let expr = expr.syntax().clone_for_update(); + usages.into_iter().for_each(|usage| { + ted::replace(usage, &expr); + }); + } + // cant inline, emit a let statement + // FIXME: emit type ascriptions when a coercion happens? + _ => body.push_front(make::let_stmt(pat, Some(expr)).clone_for_update().into()), + } + } let original_indentation = expr.indent_level(); - let mut replacement = make::block_expr(statements, body.tail_expr()) - .reset_indent() - .indent(original_indentation); + let replacement = body.reset_indent().indent(original_indentation); - if param_list.self_param().is_some() { - replacement = replacement.clone_for_update(); - let this = make::name_ref("this").syntax().clone_for_update(); - // FIXME dont look into descendant methods - replacement - .syntax() - .descendants() - .filter_map(ast::NameRef::cast) - .filter(|n| n.self_token().is_some()) - .collect::>() - .into_iter() - .rev() - .for_each(|self_ref| ted::replace(self_ref.syntax(), &this)); - } - builder.replace_ast(expr, ast::Expr::BlockExpr(replacement)); + let replacement = match replacement.tail_expr() { + Some(expr) if replacement.statements().next().is_none() => expr, + _ => ast::Expr::BlockExpr(replacement), + }; + builder.replace_ast(expr, replacement); }, ) } @@ -153,9 +211,7 @@ fn main() { r#" fn foo() { println!("Hello, World!"); } fn main() { - { - println!("Hello, World!"); - }; + { println!("Hello, World!"); }; } "#, ); @@ -174,10 +230,7 @@ fn main() { r#" fn foo(name: String) { println!("Hello, {}!", name); } fn main() { - { - let name = String::from("Michael"); - println!("Hello, {}!", name); - }; + { let name = String::from("Michael"); println!("Hello, {}!", name); }; } "#, ); @@ -218,10 +271,8 @@ fn foo(a: u32, b: u32) -> u32 { } fn main() { - let x = { - let a = 1; - let b = 2; - let x = a + b; + let x = { let b = 2; + let x = 1 + b; let y = x - b; x * y }; @@ -257,11 +308,7 @@ impl Foo { } fn main() { - let x = { - let this = Foo(3); - let a = 2; - Foo(this.0 + a) - }; + let x = Foo(Foo(3).0 + 2); } "#, ); @@ -294,11 +341,7 @@ impl Foo { } fn main() { - let x = { - let this = Foo(3); - let a = 2; - Foo(this.0 + a) - }; + let x = Foo(Foo(3).0 + 2); } "#, ); @@ -331,10 +374,8 @@ impl Foo { } fn main() { - let x = { - let ref this = Foo(3); - let a = 2; - Foo(this.0 + a) + let x = { let ref this = Foo(3); + Foo(this.0 + 2) }; } "#, @@ -370,8 +411,7 @@ impl Foo { fn main() { let mut foo = Foo(3); - { - let ref mut this = foo; + { let ref mut this = foo; this.0 = 0; }; } diff --git a/crates/syntax/src/ast/edit_in_place.rs b/crates/syntax/src/ast/edit_in_place.rs index f7ee29d14cc..30af469bc7e 100644 --- a/crates/syntax/src/ast/edit_in_place.rs +++ b/crates/syntax/src/ast/edit_in_place.rs @@ -431,6 +431,12 @@ impl ast::RecordExprFieldList { } } +impl ast::BlockExpr { + pub fn push_front(&self, statement: ast::Stmt) { + ted::insert(Position::after(self.l_curly_token().unwrap()), statement.syntax()); + } +} + fn normalize_ws_between_braces(node: &SyntaxNode) -> Option<()> { let l = node .children_with_tokens() diff --git a/crates/syntax/src/ast/node_ext.rs b/crates/syntax/src/ast/node_ext.rs index d8c5c4e76fb..e54c3f430c3 100644 --- a/crates/syntax/src/ast/node_ext.rs +++ b/crates/syntax/src/ast/node_ext.rs @@ -641,6 +641,15 @@ impl ast::SlicePat { } } +impl ast::IdentPat { + pub fn is_simple_ident(&self) -> bool { + self.at_token().is_none() + && self.mut_token().is_none() + && self.ref_token().is_none() + && self.pat().is_none() + } +} + #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] pub enum SelfParamKind { /// self From 6181154d50720d7474ade413211e2e1fcb644714 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Sat, 3 Jul 2021 21:42:59 +0200 Subject: [PATCH 2/3] Add some more `inline_call` tests --- .../ide_assists/src/handlers/inline_call.rs | 92 +++++++++++++++++-- crates/ide_assists/src/tests/generated.rs | 12 +-- 2 files changed, 86 insertions(+), 18 deletions(-) diff --git a/crates/ide_assists/src/handlers/inline_call.rs b/crates/ide_assists/src/handlers/inline_call.rs index 9b46b8f2fee..c3251b06880 100644 --- a/crates/ide_assists/src/handlers/inline_call.rs +++ b/crates/ide_assists/src/handlers/inline_call.rs @@ -17,20 +17,16 @@ use crate::{ // Inlines a function or method body. // // ``` -// fn add(a: u32, b: u32) -> u32 { a + b } +// fn align(a: u32, b: u32) -> u32 { (a + b - 1) & !(b - 1) } // fn main() { -// let x = add$0(1, 2); +// let x = align$0(1, 2); // } // ``` // -> // ``` -// fn add(a: u32, b: u32) -> u32 { a + b } +// fn align(a: u32, b: u32) -> u32 { (a + b - 1) & !(b - 1) } // fn main() { -// let x = { -// let a = 1; -// let b = 2; -// a + b -// }; +// let x = { let b = 2; (1 + b - 1) & !(b - 1) }; // } // ``` pub(crate) fn inline_call(acc: &mut Assists, ctx: &AssistContext) -> Option<()> { @@ -169,9 +165,8 @@ pub(crate) fn inline_( if expr.path().and_then(|path| path.as_single_name_ref()).is_some() ) => { - let expr = expr.syntax().clone_for_update(); usages.into_iter().for_each(|usage| { - ted::replace(usage, &expr); + ted::replace(usage, &expr.syntax().clone_for_update()); }); } // cant inline, emit a let statement @@ -415,6 +410,83 @@ fn main() { this.0 = 0; }; } +"#, + ); + } + + #[test] + fn function_single_use_expr_in_param() { + check_assist( + inline_call, + r#" +fn double(x: u32) -> u32 { + 2 * x +} +fn main() { + let x = 51; + let x = double$0(10 + x); +} +"#, + r#" +fn double(x: u32) -> u32 { + 2 * x +} +fn main() { + let x = 51; + let x = 2 * 10 + x; +} +"#, + ); + } + + #[test] + fn function_multi_use_expr_in_param() { + check_assist( + inline_call, + r#" +fn square(x: u32) -> u32 { + x * x +} +fn main() { + let x = 51; + let y = square$0(10 + x); +} +"#, + r#" +fn square(x: u32) -> u32 { + x * x +} +fn main() { + let x = 51; + let y = { let x = 10 + x; + x * x + }; +} +"#, + ); + } + + #[test] + fn function_multi_use_local_in_param() { + check_assist( + inline_call, + r#" +fn square(x: u32) -> u32 { + x * x +} +fn main() { + let local = 51; + let y = square$0(local); +} +"#, + r#" +fn square(x: u32) -> u32 { + x * x +} +fn main() { + let local = 51; + let y = local * local; +} "#, ); } diff --git a/crates/ide_assists/src/tests/generated.rs b/crates/ide_assists/src/tests/generated.rs index cae2ad57eb5..f6ecc2d5d4b 100644 --- a/crates/ide_assists/src/tests/generated.rs +++ b/crates/ide_assists/src/tests/generated.rs @@ -923,19 +923,15 @@ fn doctest_inline_call() { check_doc_test( "inline_call", r#####" -fn add(a: u32, b: u32) -> u32 { a + b } +fn align(a: u32, b: u32) -> u32 { (a + b - 1) & !(b - 1) } fn main() { - let x = add$0(1, 2); + let x = align$0(1, 2); } "#####, r#####" -fn add(a: u32, b: u32) -> u32 { a + b } +fn align(a: u32, b: u32) -> u32 { (a + b - 1) & !(b - 1) } fn main() { - let x = { - let a = 1; - let b = 2; - a + b - }; + let x = { let b = 2; (1 + b - 1) & !(b - 1) }; } "#####, ) From ea02d27a1e5632466cb437bad7432783bebba70f Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Mon, 5 Jul 2021 13:37:40 +0200 Subject: [PATCH 3/3] Fixup emitted whitespace in most cases --- .../ide_assists/src/handlers/inline_call.rs | 109 +++++++++++++----- crates/ide_assists/src/tests/generated.rs | 13 ++- crates/syntax/src/ted.rs | 6 + 3 files changed, 97 insertions(+), 31 deletions(-) diff --git a/crates/ide_assists/src/handlers/inline_call.rs b/crates/ide_assists/src/handlers/inline_call.rs index c3251b06880..5b6992b6409 100644 --- a/crates/ide_assists/src/handlers/inline_call.rs +++ b/crates/ide_assists/src/handlers/inline_call.rs @@ -17,16 +17,23 @@ use crate::{ // Inlines a function or method body. // // ``` -// fn align(a: u32, b: u32) -> u32 { (a + b - 1) & !(b - 1) } +// fn align(a: u32, b: u32) -> u32 { +// (a + b - 1) & !(b - 1) +// } // fn main() { // let x = align$0(1, 2); // } // ``` // -> // ``` -// fn align(a: u32, b: u32) -> u32 { (a + b - 1) & !(b - 1) } +// fn align(a: u32, b: u32) -> u32 { +// (a + b - 1) & !(b - 1) +// } // fn main() { -// let x = { let b = 2; (1 + b - 1) & !(b - 1) }; +// let x = { +// let b = 2; +// (1 + b - 1) & !(b - 1) +// }; // } // ``` pub(crate) fn inline_call(acc: &mut Assists, ctx: &AssistContext) -> Option<()> { @@ -140,14 +147,14 @@ pub(crate) fn inline_( // Rewrite `self` to `this` if param_list.self_param().is_some() { - let this = make::name_ref("this").syntax().clone_for_update(); + let this = || make::name_ref("this").syntax().clone_for_update(); usages_for_locals(params[0].1.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); + ted::replace(it, &this()); }) } @@ -212,25 +219,6 @@ fn main() { ); } - #[test] - fn args_with_side_effects() { - check_assist( - inline_call, - r#" -fn foo(name: String) { println!("Hello, {}!", name); } -fn main() { - foo$0(String::from("Michael")); -} -"#, - r#" -fn foo(name: String) { println!("Hello, {}!", name); } -fn main() { - { let name = String::from("Michael"); println!("Hello, {}!", name); }; -} -"#, - ); - } - #[test] fn not_applicable_when_incorrect_number_of_parameters_are_provided() { cov_mark::check!(inline_call_incorrect_number_of_arguments); @@ -243,6 +231,32 @@ fn main() { let x = add$0(42); } ); } + #[test] + fn args_with_side_effects() { + check_assist( + inline_call, + r#" +fn foo(name: String) { + println!("Hello, {}!", name); +} +fn main() { + foo$0(String::from("Michael")); +} +"#, + r#" +fn foo(name: String) { + println!("Hello, {}!", name); +} +fn main() { + { + let name = String::from("Michael"); + println!("Hello, {}!", name); + }; +} +"#, + ); + } + #[test] fn function_with_multiple_statements() { check_assist( @@ -266,7 +280,8 @@ fn foo(a: u32, b: u32) -> u32 { } fn main() { - let x = { let b = 2; + let x = { + let b = 2; let x = 1 + b; let y = x - b; x * y @@ -369,7 +384,8 @@ impl Foo { } fn main() { - let x = { let ref this = Foo(3); + let x = { + let ref this = Foo(3); Foo(this.0 + 2) }; } @@ -406,7 +422,8 @@ impl Foo { fn main() { let mut foo = Foo(3); - { let ref mut this = foo; + { + let ref mut this = foo; this.0 = 0; }; } @@ -458,7 +475,8 @@ fn square(x: u32) -> u32 { } fn main() { let x = 51; - let y = { let x = 10 + x; + let y = { + let x = 10 + x; x * x }; } @@ -487,6 +505,41 @@ fn main() { let local = 51; let y = local * local; } +"#, + ); + } + + #[test] + fn method_in_impl() { + check_assist( + inline_call, + r#" +struct Foo; +impl Foo { + fn foo(&self) { + self; + self; + } + fn bar(&self) { + self.foo$0(); + } +} +"#, + r#" +struct Foo; +impl Foo { + fn foo(&self) { + self; + self; + } + fn bar(&self) { + { + let ref this = self; + this; + this; + }; + } +} "#, ); } diff --git a/crates/ide_assists/src/tests/generated.rs b/crates/ide_assists/src/tests/generated.rs index f6ecc2d5d4b..c0714c38052 100644 --- a/crates/ide_assists/src/tests/generated.rs +++ b/crates/ide_assists/src/tests/generated.rs @@ -923,15 +923,22 @@ fn doctest_inline_call() { check_doc_test( "inline_call", r#####" -fn align(a: u32, b: u32) -> u32 { (a + b - 1) & !(b - 1) } +fn align(a: u32, b: u32) -> u32 { + (a + b - 1) & !(b - 1) +} fn main() { let x = align$0(1, 2); } "#####, r#####" -fn align(a: u32, b: u32) -> u32 { (a + b - 1) & !(b - 1) } +fn align(a: u32, b: u32) -> u32 { + (a + b - 1) & !(b - 1) +} fn main() { - let x = { let b = 2; (1 + b - 1) & !(b - 1) }; + let x = { + let b = 2; + (1 + b - 1) & !(b - 1) + }; } "#####, ) diff --git a/crates/syntax/src/ted.rs b/crates/syntax/src/ted.rs index ae970f44f16..f04feb883c7 100644 --- a/crates/syntax/src/ted.rs +++ b/crates/syntax/src/ted.rs @@ -188,6 +188,12 @@ fn ws_between(left: &SyntaxElement, right: &SyntaxElement) -> Option