Auto merge of #17991 - ChayimFriedman2:extract-variable-ref, r=Veykril

fix: Don't add reference when it isn't needed for the "Extract variable" assist

I.e. don't generate `let var_name = &foo()`. Because it always irritates me when I need to fix that.

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/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;
 // }
 // ```
@@ -58,9 +58,30 @@ 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
+    // Any expression that autoderefs may need adjustment.
-        .as_ref()
+    let mut needs_adjust = parent.as_ref().map_or(false, |it| match it {
-        .map_or(false, |it| matches!(it, ast::Expr::FieldExpr(_) | ast::Expr::MethodCallExpr(_)));
+        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 mut to_extract_no_ref = peel_parens(to_extract.clone());
+    let needs_ref = needs_adjust
+        && match &to_extract_no_ref {
+            ast::Expr::FieldExpr(_)
+            | 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();
@@ -87,22 +108,28 @@ 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_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 {
@@ -202,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<ast::Expr> {
@@ -1220,6 +1255,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<usize> 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<usize> 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(
@@ -1461,4 +1535,60 @@ fn foo() {
 }"#,
         );
     }
+
+    #[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();
+}"#,
+        );
+    }
+
+    #[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();
+}
+"#,
+        );
+    }
 }

src/tools/rust-analyzer/crates/ide-assists/src/tests/generated.rs

@@ -1028,7 +1028,7 @@ fn main() {
 "#####,
         r#####"
 fn main() {
-    let $0var_name = (1 + 2);
+    let $0var_name = 1 + 2;
     var_name * 4;
 }
 "#####,