Rollup merge of #128244 - compiler-errors:move-clone-sugg, r=estebank

Peel off explicit (or implicit) deref before suggesting clone on move error in borrowck, remove some hacks

Also remove a heck of a lot of weird hacks in `suggest_cloning` that I don't think we should have around.

I know this regresses tests, but I don't believe most of these suggestions were accurate, b/c:
1. They either produced type errors (e.g. turning `&x` into `x.clone()`)
2. They don't fix the issue
3. They fix the issue ostensibly, but introduce logic errors (e.g. cloning a `&mut Option<T>` to then `Option::take` out...)

Most of the suggestions are still wrong, but they're not particularly *less* wrong IMO.

Stacked on top of #128241, which is an "obviously worth landing" subset of this PR.

r? estebank

Author: matthiaskrgr

compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs

@@ -563,11 +563,11 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> {
                 } = move_spans
                     && can_suggest_clone
                 {
-                    self.suggest_cloning(err, ty, expr, None, Some(move_spans));
+                    self.suggest_cloning(err, ty, expr, Some(move_spans));
                 } else if self.suggest_hoisting_call_outside_loop(err, expr) && can_suggest_clone {
                     // The place where the type moves would be misleading to suggest clone.
                     // #121466
-                    self.suggest_cloning(err, ty, expr, None, Some(move_spans));
+                    self.suggest_cloning(err, ty, expr, Some(move_spans));
                 }
             }
 
@@ -1229,8 +1229,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> {
         &self,
         err: &mut Diag<'_>,
         ty: Ty<'tcx>,
-        mut expr: &'tcx hir::Expr<'tcx>,
-        mut other_expr: Option<&'tcx hir::Expr<'tcx>>,
+        expr: &'tcx hir::Expr<'tcx>,
         use_spans: Option<UseSpans<'tcx>>,
     ) {
         if let hir::ExprKind::Struct(_, _, Some(_)) = expr.kind {
@@ -1242,66 +1241,6 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> {
             return;
         }
 
-        if let Some(some_other_expr) = other_expr
-            && let Some(parent_binop) =
-                self.infcx.tcx.hir().parent_iter(expr.hir_id).find_map(|n| {
-                    if let (hir_id, hir::Node::Expr(e)) = n
-                        && let hir::ExprKind::AssignOp(_binop, target, _arg) = e.kind
-                        && target.hir_id == expr.hir_id
-                    {
-                        Some(hir_id)
-                    } else {
-                        None
-                    }
-                })
-            && let Some(other_parent_binop) =
-                self.infcx.tcx.hir().parent_iter(some_other_expr.hir_id).find_map(|n| {
-                    if let (hir_id, hir::Node::Expr(expr)) = n
-                        && let hir::ExprKind::AssignOp(..) = expr.kind
-                    {
-                        Some(hir_id)
-                    } else {
-                        None
-                    }
-                })
-            && parent_binop == other_parent_binop
-        {
-            // Explicitly look for `expr += other_expr;` and avoid suggesting
-            // `expr.clone() += other_expr;`, instead suggesting `expr += other_expr.clone();`.
-            other_expr = Some(expr);
-            expr = some_other_expr;
-        }
-        'outer: {
-            if let ty::Ref(..) = ty.kind() {
-                // We check for either `let binding = foo(expr, other_expr);` or
-                // `foo(expr, other_expr);` and if so we don't suggest an incorrect
-                // `foo(expr, other_expr).clone()`
-                if let Some(other_expr) = other_expr
-                    && let Some(parent_let) =
-                        self.infcx.tcx.hir().parent_iter(expr.hir_id).find_map(|n| {
-                            if let (hir_id, hir::Node::LetStmt(_) | hir::Node::Stmt(_)) = n {
-                                Some(hir_id)
-                            } else {
-                                None
-                            }
-                        })
-                    && let Some(other_parent_let) =
-                        self.infcx.tcx.hir().parent_iter(other_expr.hir_id).find_map(|n| {
-                            if let (hir_id, hir::Node::LetStmt(_) | hir::Node::Stmt(_)) = n {
-                                Some(hir_id)
-                            } else {
-                                None
-                            }
-                        })
-                    && parent_let == other_parent_let
-                {
-                    // Explicitly check that we don't have `foo(&*expr, other_expr)`, as cloning the
-                    // result of `foo(...)` won't help.
-                    break 'outer;
-                }
-            }
-        }
-        let ty = ty.peel_refs();
         if self.implements_clone(ty) {
             self.suggest_cloning_inner(err, ty, expr);
         } else if let ty::Adt(def, args) = ty.kind()
@@ -1573,10 +1512,27 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> {
             );
         self.suggest_copy_for_type_in_cloned_ref(&mut err, place);
         let typeck_results = self.infcx.tcx.typeck(self.mir_def_id());
-        if let Some(expr) = self.find_expr(borrow_span)
-            && let Some(ty) = typeck_results.node_type_opt(expr.hir_id)
-        {
-            self.suggest_cloning(&mut err, ty, expr, self.find_expr(span), Some(move_spans));
+        if let Some(expr) = self.find_expr(borrow_span) {
+            // This is a borrow span, so we want to suggest cloning the referent.
+            if let hir::ExprKind::AddrOf(_, _, borrowed_expr) = expr.kind
+                && let Some(ty) = typeck_results.expr_ty_opt(borrowed_expr)
+            {
+                self.suggest_cloning(&mut err, ty, borrowed_expr, Some(move_spans));
+            } else if typeck_results.expr_adjustments(expr).first().is_some_and(|adj| {
+                matches!(
+                    adj.kind,
+                    ty::adjustment::Adjust::Borrow(ty::adjustment::AutoBorrow::Ref(
+                        _,
+                        ty::adjustment::AutoBorrowMutability::Not
+                            | ty::adjustment::AutoBorrowMutability::Mut {
+                                allow_two_phase_borrow: ty::adjustment::AllowTwoPhase::No
+                            }
+                    ))
+                )
+            }) && let Some(ty) = typeck_results.expr_ty_opt(expr)
+            {
+                self.suggest_cloning(&mut err, ty, expr, Some(move_spans));
+            }
         }
         self.buffer_error(err);
     }

compiler/rustc_borrowck/src/diagnostics/move_errors.rs

@@ -564,9 +564,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> {
 
     fn add_move_hints(&self, error: GroupedMoveError<'tcx>, err: &mut Diag<'_>, span: Span) {
         match error {
-            GroupedMoveError::MovesFromPlace {
-                mut binds_to, move_from, span: other_span, ..
-            } => {
+            GroupedMoveError::MovesFromPlace { mut binds_to, move_from, .. } => {
                 self.add_borrow_suggestions(err, span);
                 if binds_to.is_empty() {
                     let place_ty = move_from.ty(self.body, self.infcx.tcx).ty;
@@ -576,7 +574,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> {
                     };
 
                     if let Some(expr) = self.find_expr(span) {
-                        self.suggest_cloning(err, place_ty, expr, self.find_expr(other_span), None);
+                        self.suggest_cloning(err, place_ty, expr, None);
                     }
 
                     err.subdiagnostic(crate::session_diagnostics::TypeNoCopy::Label {
@@ -608,13 +606,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> {
                 };
 
                 if let Some(expr) = self.find_expr(use_span) {
-                    self.suggest_cloning(
-                        err,
-                        place_ty,
-                        expr,
-                        self.find_expr(span),
-                        Some(use_spans),
-                    );
+                    self.suggest_cloning(err, place_ty, expr, Some(use_spans));
                 }
 
                 err.subdiagnostic(crate::session_diagnostics::TypeNoCopy::Label {
@@ -739,7 +731,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> {
                 let place_desc = &format!("`{}`", self.local_names[*local].unwrap());
 
                 if let Some(expr) = self.find_expr(binding_span) {
-                    self.suggest_cloning(err, bind_to.ty, expr, None, None);
+                    self.suggest_cloning(err, bind_to.ty, expr, None);
                 }
 
                 err.subdiagnostic(crate::session_diagnostics::TypeNoCopy::Label {

tests/ui/associated-types/associated-types-outlives.stderr

@@ -18,7 +18,7 @@ LL | pub fn free_and_use<T: for<'a> Foo<'a>,
    |                     ^ consider constraining this type parameter with `Clone`
 ...
 LL |         's: loop { y = denormalise(&x); break }
-   |                                    -- you could clone this value
+   |                                     - you could clone this value
 
 error: aborting due to 1 previous error
 

tests/ui/augmented-assignments.rs

@@ -17,7 +17,6 @@ fn main() {
     x;
     //~^ ERROR cannot move out of `x` because it is borrowed
     //~| move out of `x` occurs here
-    //~| HELP consider cloning
 
     let y = Int(2);
     //~^ HELP consider changing this to be mutable

tests/ui/augmented-assignments.stderr

@@ -8,14 +8,9 @@ LL |     x
 ...
 LL |     x;
    |     ^ move out of `x` occurs here
-   |
-help: consider cloning the value if the performance cost is acceptable
-   |
-LL |     x.clone();
-   |      ++++++++
 
 error[E0596]: cannot borrow `y` as mutable, as it is not declared as mutable
-  --> $DIR/augmented-assignments.rs:25:5
+  --> $DIR/augmented-assignments.rs:24:5
    |
 LL |     y
    |     ^ cannot borrow as mutable

tests/ui/binop/binop-move-semantics.stderr

@@ -65,7 +65,7 @@ help: if `T` implemented `Clone`, you could clone the value
 LL | fn move_borrowed<T: Add<Output=()>>(x: T, mut y: T) {
    |                  ^ consider constraining this type parameter with `Clone`
 LL |     let m = &x;
-   |             -- you could clone this value
+   |              - you could clone this value
 
 error[E0505]: cannot move out of `y` because it is borrowed
   --> $DIR/binop-move-semantics.rs:23:5
@@ -88,7 +88,7 @@ LL | fn move_borrowed<T: Add<Output=()>>(x: T, mut y: T) {
    |                  ^ consider constraining this type parameter with `Clone`
 LL |     let m = &x;
 LL |     let n = &mut y;
-   |             ------ you could clone this value
+   |                  - you could clone this value
 
 error[E0507]: cannot move out of `*m` which is behind a mutable reference
   --> $DIR/binop-move-semantics.rs:30:5

tests/ui/borrowck/borrow-tuple-fields.stderr

@@ -13,9 +13,8 @@ LL |     r.use_ref();
    |
 help: consider cloning the value if the performance cost is acceptable
    |
-LL -     let r = &x.0;
-LL +     let r = x.0.clone();
-   |
+LL |     let r = &x.0.clone();
+   |                 ++++++++
 
 error[E0502]: cannot borrow `x.0` as mutable because it is also borrowed as immutable
   --> $DIR/borrow-tuple-fields.rs:18:13
@@ -51,9 +50,8 @@ LL |     r.use_ref();
    |
 help: consider cloning the value if the performance cost is acceptable
    |
-LL -     let r = &x.0;
-LL +     let r = x.0.clone();
-   |
+LL |     let r = &x.0.clone();
+   |                 ++++++++
 
 error[E0502]: cannot borrow `x.0` as mutable because it is also borrowed as immutable
   --> $DIR/borrow-tuple-fields.rs:33:13

tests/ui/borrowck/borrowck-bad-nested-calls-move.stderr

@@ -14,7 +14,7 @@ LL |         a);
 help: consider cloning the value if the performance cost is acceptable
    |
 LL -         &*a,
-LL +         a.clone(),
+LL +         &a.clone(),
    |
 
 error[E0505]: cannot move out of `a` because it is borrowed
@@ -32,7 +32,7 @@ LL |         a);
 help: consider cloning the value if the performance cost is acceptable
    |
 LL -         &*a,
-LL +         a.clone(),
+LL +         &a.clone(),
    |
 
 error: aborting due to 2 previous errors

tests/ui/borrowck/borrowck-field-sensitivity.stderr

@@ -52,9 +52,8 @@ LL |     drop(**p);
    |
 help: consider cloning the value if the performance cost is acceptable
    |
-LL -     let p = &x.b;
-LL +     let p = x.b.clone();
-   |
+LL |     let p = &x.b.clone();
+   |                 ++++++++
 
 error[E0505]: cannot move out of `x.b` because it is borrowed
   --> $DIR/borrowck-field-sensitivity.rs:41:14
@@ -70,9 +69,8 @@ LL |     drop(**p);
    |
 help: consider cloning the value if the performance cost is acceptable
    |
-LL -     let p = &x.b;
-LL +     let p = x.b.clone();
-   |
+LL |     let p = &x.b.clone();
+   |                 ++++++++
 
 error[E0499]: cannot borrow `x.a` as mutable more than once at a time
   --> $DIR/borrowck-field-sensitivity.rs:48:13

tests/ui/borrowck/borrowck-issue-48962.stderr

@@ -17,11 +17,6 @@ LL |     {src};
    |      --- value moved here
 LL |     src.0 = 66;
    |     ^^^^^^^^^^ value used here after move
-   |
-help: consider cloning the value if the performance cost is acceptable
-   |
-LL |     {src.clone()};
-   |         ++++++++
 
 error: aborting due to 2 previous errors