From 2a7ec0b0ad0b9d7aea147694c23b5f52b6dc6fa9 Mon Sep 17 00:00:00 2001 From: Chayim Refael Friedman Date: Wed, 28 Aug 2024 23:55:31 +0300 Subject: [PATCH 1/3] Consider all expressions that autoderef in "Extract variable", not just method and field accesses. --- .../src/handlers/extract_variable.rs | 51 +++++++++++++++++-- 1 file changed, 48 insertions(+), 3 deletions(-) diff --git a/src/tools/rust-analyzer/crates/ide-assists/src/handlers/extract_variable.rs b/src/tools/rust-analyzer/crates/ide-assists/src/handlers/extract_variable.rs index 0ef71a38661..33cd5252813 100644 --- a/src/tools/rust-analyzer/crates/ide-assists/src/handlers/extract_variable.rs +++ b/src/tools/rust-analyzer/crates/ide-assists/src/handlers/extract_variable.rs @@ -58,9 +58,15 @@ pub(crate) fn extract_variable(acc: &mut Assists, ctx: &AssistContext<'_>) -> Op } let parent = to_extract.syntax().parent().and_then(ast::Expr::cast); - let needs_adjust = parent - .as_ref() - .map_or(false, |it| matches!(it, ast::Expr::FieldExpr(_) | ast::Expr::MethodCallExpr(_))); + // Any expression that autoderefs may need adjustment. + let needs_adjust = parent.as_ref().map_or(false, |it| match it { + ast::Expr::FieldExpr(_) + | ast::Expr::MethodCallExpr(_) + | ast::Expr::CallExpr(_) + | ast::Expr::AwaitExpr(_) => true, + ast::Expr::IndexExpr(index) if index.base().as_ref() == Some(&to_extract) => true, + _ => false, + }); let anchor = Anchor::from(&to_extract)?; let target = to_extract.syntax().text_range(); @@ -1220,6 +1226,45 @@ fn foo(s: &S) { ); } + #[test] + fn test_extract_var_index_deref() { + check_assist( + extract_variable, + r#" +//- minicore: index +struct X; + +impl std::ops::Index for X { + type Output = i32; + fn index(&self) -> &Self::Output { 0 } +} + +struct S { + sub: X +} + +fn foo(s: &S) { + $0s.sub$0[0]; +}"#, + r#" +struct X; + +impl std::ops::Index for X { + type Output = i32; + fn index(&self) -> &Self::Output { 0 } +} + +struct S { + sub: X +} + +fn foo(s: &S) { + let $0sub = &s.sub; + sub[0]; +}"#, + ); + } + #[test] fn test_extract_var_reference_parameter_deep_nesting() { check_assist( From fe5f91ed8e54eeac1cc81930982d01483d745a65 Mon Sep 17 00:00:00 2001 From: Chayim Refael Friedman Date: Thu, 29 Aug 2024 00:10:26 +0300 Subject: [PATCH 2/3] Don't add reference when it isn't needed for the "Extract variable" assist I.e. don't generate `let var_name = &foo()`. Anything that creates a new value don't need a reference. That excludes mostly field accesses and indexing. I had a thought that we can also not generate a reference for fields and indexing as long as the type is `Copy`, but sometimes people impl `Copy` even when they don't want to copy the values (e.g. a large type), so I didn't do that. --- .../src/handlers/extract_variable.rs | 43 ++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/src/tools/rust-analyzer/crates/ide-assists/src/handlers/extract_variable.rs b/src/tools/rust-analyzer/crates/ide-assists/src/handlers/extract_variable.rs index 33cd5252813..e6d1662e687 100644 --- a/src/tools/rust-analyzer/crates/ide-assists/src/handlers/extract_variable.rs +++ b/src/tools/rust-analyzer/crates/ide-assists/src/handlers/extract_variable.rs @@ -67,6 +67,15 @@ pub(crate) fn extract_variable(acc: &mut Assists, ctx: &AssistContext<'_>) -> Op ast::Expr::IndexExpr(index) if index.base().as_ref() == Some(&to_extract) => true, _ => false, }); + let needs_ref = needs_adjust + && matches!( + to_extract, + ast::Expr::FieldExpr(_) + | ast::Expr::IndexExpr(_) + | ast::Expr::MacroExpr(_) + | ast::Expr::ParenExpr(_) + | ast::Expr::PathExpr(_) + ); let anchor = Anchor::from(&to_extract)?; let target = to_extract.syntax().text_range(); @@ -93,10 +102,16 @@ pub(crate) fn extract_variable(acc: &mut Assists, ctx: &AssistContext<'_>) -> Op Some(ast::Expr::RefExpr(expr)) if expr.mut_token().is_some() => { make::ident_pat(false, true, make::name(&var_name)) } + _ if needs_adjust + && !needs_ref + && ty.as_ref().is_some_and(|ty| ty.is_mutable_reference()) => + { + make::ident_pat(false, true, make::name(&var_name)) + } _ => make::ident_pat(false, false, make::name(&var_name)), }; - let to_extract = match ty.as_ref().filter(|_| needs_adjust) { + let to_extract = match ty.as_ref().filter(|_| needs_ref) { Some(receiver_type) if receiver_type.is_mutable_reference() => { make::expr_ref(to_extract, true) } @@ -1503,6 +1518,32 @@ fn foo() { fn foo() { let mut $0var_name = 0; let v = &mut var_name; +}"#, + ); + } + + #[test] + fn generates_no_ref_on_calls() { + check_assist( + extract_variable, + r#" +struct S; +impl S { + fn do_work(&mut self) {} +} +fn bar() -> S { S } +fn foo() { + $0bar()$0.do_work(); +}"#, + r#" +struct S; +impl S { + fn do_work(&mut self) {} +} +fn bar() -> S { S } +fn foo() { + let mut $0bar = bar(); + bar.do_work(); }"#, ); } From 3ff3d39b96584fc263facc002c723e7c8d8c5a85 Mon Sep 17 00:00:00 2001 From: Chayim Refael Friedman Date: Thu, 29 Aug 2024 00:35:45 +0300 Subject: [PATCH 3/3] Also handle deref expressions in "Extract variable" And BTW, remove the parentheses of the extracted expression if there are. --- .../src/handlers/extract_variable.rs | 72 +++++++++++++++---- .../crates/ide-assists/src/tests/generated.rs | 2 +- 2 files changed, 59 insertions(+), 15 deletions(-) diff --git a/src/tools/rust-analyzer/crates/ide-assists/src/handlers/extract_variable.rs b/src/tools/rust-analyzer/crates/ide-assists/src/handlers/extract_variable.rs index e6d1662e687..5ae75bb1ff8 100644 --- a/src/tools/rust-analyzer/crates/ide-assists/src/handlers/extract_variable.rs +++ b/src/tools/rust-analyzer/crates/ide-assists/src/handlers/extract_variable.rs @@ -20,7 +20,7 @@ // -> // ``` // fn main() { -// let $0var_name = (1 + 2); +// let $0var_name = 1 + 2; // var_name * 4; // } // ``` @@ -59,7 +59,7 @@ pub(crate) fn extract_variable(acc: &mut Assists, ctx: &AssistContext<'_>) -> Op let parent = to_extract.syntax().parent().and_then(ast::Expr::cast); // Any expression that autoderefs may need adjustment. - let needs_adjust = parent.as_ref().map_or(false, |it| match it { + let mut needs_adjust = parent.as_ref().map_or(false, |it| match it { ast::Expr::FieldExpr(_) | ast::Expr::MethodCallExpr(_) | ast::Expr::CallExpr(_) @@ -67,15 +67,21 @@ pub(crate) fn extract_variable(acc: &mut Assists, ctx: &AssistContext<'_>) -> Op ast::Expr::IndexExpr(index) if index.base().as_ref() == Some(&to_extract) => true, _ => false, }); + let mut to_extract_no_ref = peel_parens(to_extract.clone()); let needs_ref = needs_adjust - && matches!( - to_extract, + && match &to_extract_no_ref { ast::Expr::FieldExpr(_) - | ast::Expr::IndexExpr(_) - | ast::Expr::MacroExpr(_) - | ast::Expr::ParenExpr(_) - | ast::Expr::PathExpr(_) - ); + | ast::Expr::IndexExpr(_) + | ast::Expr::MacroExpr(_) + | ast::Expr::ParenExpr(_) + | ast::Expr::PathExpr(_) => true, + ast::Expr::PrefixExpr(prefix) if prefix.op_kind() == Some(ast::UnaryOp::Deref) => { + to_extract_no_ref = prefix.expr()?; + needs_adjust = false; + false + } + _ => false, + }; let anchor = Anchor::from(&to_extract)?; let target = to_extract.syntax().text_range(); @@ -111,19 +117,19 @@ pub(crate) fn extract_variable(acc: &mut Assists, ctx: &AssistContext<'_>) -> Op _ => make::ident_pat(false, false, make::name(&var_name)), }; - let to_extract = match ty.as_ref().filter(|_| needs_ref) { + let to_extract_no_ref = match ty.as_ref().filter(|_| needs_ref) { Some(receiver_type) if receiver_type.is_mutable_reference() => { - make::expr_ref(to_extract, true) + make::expr_ref(to_extract_no_ref, true) } Some(receiver_type) if receiver_type.is_reference() => { - make::expr_ref(to_extract, false) + make::expr_ref(to_extract_no_ref, false) } - _ => to_extract, + _ => to_extract_no_ref, }; let expr_replace = edit.make_syntax_mut(expr_replace); let let_stmt = - make::let_stmt(ident_pat.into(), None, Some(to_extract)).clone_for_update(); + make::let_stmt(ident_pat.into(), None, Some(to_extract_no_ref)).clone_for_update(); let name_expr = make::expr_path(make::ext::ident_path(&var_name)).clone_for_update(); match anchor { @@ -223,6 +229,14 @@ pub(crate) fn extract_variable(acc: &mut Assists, ctx: &AssistContext<'_>) -> Op ) } +fn peel_parens(mut expr: ast::Expr) -> ast::Expr { + while let ast::Expr::ParenExpr(parens) = &expr { + let Some(expr_inside) = parens.expr() else { break }; + expr = expr_inside; + } + expr +} + /// Check whether the node is a valid expression which can be extracted to a variable. /// In general that's true for any expression, but in some cases that would produce invalid code. fn valid_target_expr(node: SyntaxNode) -> Option { @@ -1547,4 +1561,34 @@ fn foo() { }"#, ); } + + #[test] + fn generates_no_ref_for_deref() { + check_assist( + extract_variable, + r#" +struct S; +impl S { + fn do_work(&mut self) {} +} +fn bar() -> S { S } +fn foo() { + let v = &mut &mut bar(); + $0(**v)$0.do_work(); +} +"#, + r#" +struct S; +impl S { + fn do_work(&mut self) {} +} +fn bar() -> S { S } +fn foo() { + let v = &mut &mut bar(); + let $0s = *v; + s.do_work(); +} +"#, + ); + } } diff --git a/src/tools/rust-analyzer/crates/ide-assists/src/tests/generated.rs b/src/tools/rust-analyzer/crates/ide-assists/src/tests/generated.rs index 595ce1affb0..0a3242c7384 100644 --- a/src/tools/rust-analyzer/crates/ide-assists/src/tests/generated.rs +++ b/src/tools/rust-analyzer/crates/ide-assists/src/tests/generated.rs @@ -994,7 +994,7 @@ fn main() { "#####, r#####" fn main() { - let $0var_name = (1 + 2); + let $0var_name = 1 + 2; var_name * 4; } "#####,