Auto merge of rust-lang#124136 - estebank:clone-o-rama-2, r=nnethercote

Provide more context and suggestions in borrowck errors involving closures

Start pointing to where bindings where declared when they are captured in closures:

```
error[E0597]: `x` does not live long enough
  --> $DIR/suggest-return-closure.rs:23:9
   |
LL |     let x = String::new();
   |         - binding `x` declared here
...
LL |     |c| {
   |     --- value captured here
LL |         x.push(c);
   |         ^ borrowed value does not live long enough
...
LL | }
   | -- borrow later used here
   | |
   | `x` dropped here while still borrowed
```

Suggest cloning in more cases involving closures:

```
error[E0507]: cannot move out of `foo` in pattern guard
  --> $DIR/issue-27282-move-ref-mut-into-guard.rs:11:19
   |
LL |             if { (|| { let mut bar = foo; bar.take() })(); false } => {},
   |                   ^^                 --- move occurs because `foo` has type `&mut Option<&i32>`, which does not implement the `Copy` trait
   |                   |
   |                   `foo` is moved here
   |
   = note: variables bound in patterns cannot be moved from until after the end of the pattern guard
help: consider cloning the value if the performance cost is acceptable
   |
LL |             if { (|| { let mut bar = foo.clone(); bar.take() })(); false } => {},
   |                                         ++++++++
```

Mention when type parameter could be Clone

```
error[E0382]: use of moved value: `t`
  --> $DIR/use_of_moved_value_copy_suggestions.rs:7:9
   |
LL | fn duplicate_t<T>(t: T) -> (T, T) {
   |                   - move occurs because `t` has type `T`, which does not implement the `Copy` trait
...
LL |     (t, t)
   |      -  ^ value used here after move
   |      |
   |      value moved here
   |
help: if `T` implemented `Clone`, you could clone the value
  --> $DIR/use_of_moved_value_copy_suggestions.rs:4:16
   |
LL | fn duplicate_t<T>(t: T) -> (T, T) {
   |                ^ consider constraining this type parameter with `Clone`
...
LL |     (t, t)
   |      - you could clone this value
help: consider restricting type parameter `T`
   |
LL | fn duplicate_t<T: Copy>(t: T) -> (T, T) {
   |                 ++++++
```

The `help` is new. On ADTs, we also extend the output with span labels:

```
error[E0507]: cannot move out of static item `FOO`
  --> $DIR/issue-17718-static-move.rs:6:14
   |
LL |     let _a = FOO;
   |              ^^^ move occurs because `FOO` has type `Foo`, which does not implement the `Copy` trait
   |
note: if `Foo` implemented `Clone`, you could clone the value
  --> $DIR/issue-17718-static-move.rs:1:1
   |
LL | struct Foo;
   | ^^^^^^^^^^ consider implementing `Clone` for this type
...
LL |     let _a = FOO;
   |              --- you could clone this value
help: consider borrowing here
   |
LL |     let _a = &FOO;
   |              +
```

Suggest cloning captured binding in move closure

```
error[E0507]: cannot move out of `bar`, a captured variable in an `FnMut` closure
  --> $DIR/borrowck-move-by-capture.rs:9:29
   |
LL |     let bar: Box<_> = Box::new(3);
   |         --- captured outer variable
LL |     let _g = to_fn_mut(|| {
   |                        -- captured by this `FnMut` closure
LL |         let _h = to_fn_once(move || -> isize { *bar });
   |                             ^^^^^^^^^^^^^^^^   ----
   |                             |                  |
   |                             |                  variable moved due to use in closure
   |                             |                  move occurs because `bar` has type `Box<isize>`, which does not implement the `Copy` trait
   |                             `bar` is moved here
   |
help: clone the value before moving it into the closure 1
   |
LL ~         let value = bar.clone();
LL ~         let _h = to_fn_once(move || -> isize { value });
   |
```

Author: bors

compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs

@@ -347,7 +347,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
         mpi: MovePathIndex,
         err: &mut Diag<'tcx>,
         in_pattern: &mut bool,
-        move_spans: UseSpans<'_>,
+        move_spans: UseSpans<'tcx>,
     ) {
         let move_span = match move_spans {
             UseSpans::ClosureUse { capture_kind_span, .. } => capture_kind_span,
@@ -491,11 +491,11 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                     ..
                 } = move_spans
                 {
-                    self.suggest_cloning(err, ty, expr, None);
+                    self.suggest_cloning(err, ty, expr, None, Some(move_spans));
                 } else if self.suggest_hoisting_call_outside_loop(err, expr) {
                     // The place where the the type moves would be misleading to suggest clone.
                     // #121466
-                    self.suggest_cloning(err, ty, expr, None);
+                    self.suggest_cloning(err, ty, expr, None, Some(move_spans));
                 }
             }
             if let Some(pat) = finder.pat {
@@ -1085,6 +1085,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
         ty: Ty<'tcx>,
         mut expr: &'cx hir::Expr<'cx>,
         mut other_expr: Option<&'cx hir::Expr<'cx>>,
+        use_spans: Option<UseSpans<'tcx>>,
     ) {
         if let hir::ExprKind::Struct(_, _, Some(_)) = expr.kind {
             // We have `S { foo: val, ..base }`. In `check_aggregate_rvalue` we have a single
@@ -1197,14 +1198,50 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                     .all(|field| self.implements_clone(field.ty(self.infcx.tcx, args)))
             })
         {
+            let ty_span = self.infcx.tcx.def_span(def.did());
+            let mut span: MultiSpan = ty_span.into();
+            span.push_span_label(ty_span, "consider implementing `Clone` for this type");
+            span.push_span_label(expr.span, "you could clone this value");
             err.span_note(
-                self.infcx.tcx.def_span(def.did()),
+                span,
+                format!("if `{ty}` implemented `Clone`, you could clone the value"),
+            );
+        } else if let ty::Param(param) = ty.kind()
+            && let Some(_clone_trait_def) = self.infcx.tcx.lang_items().clone_trait()
+            && let generics = self.infcx.tcx.generics_of(self.mir_def_id())
+            && let generic_param = generics.type_param(*param, self.infcx.tcx)
+            && let param_span = self.infcx.tcx.def_span(generic_param.def_id)
+            && if let Some(UseSpans::FnSelfUse { kind, .. }) = use_spans
+                && let CallKind::FnCall { fn_trait_id, self_ty } = kind
+                && let ty::Param(_) = self_ty.kind()
+                && ty == self_ty
+                && [
+                    self.infcx.tcx.lang_items().fn_once_trait(),
+                    self.infcx.tcx.lang_items().fn_mut_trait(),
+                    self.infcx.tcx.lang_items().fn_trait(),
+                ]
+                .contains(&Some(fn_trait_id))
+            {
+                // Do not suggest `F: FnOnce() + Clone`.
+                false
+            } else {
+                true
+            }
+        {
+            let mut span: MultiSpan = param_span.into();
+            span.push_span_label(
+                param_span,
+                "consider constraining this type parameter with `Clone`",
+            );
+            span.push_span_label(expr.span, "you could clone this value");
+            err.span_help(
+                span,
                 format!("if `{ty}` implemented `Clone`, you could clone the value"),
             );
         }
     }
 
-    fn implements_clone(&self, ty: Ty<'tcx>) -> bool {
+    pub(crate) fn implements_clone(&self, ty: Ty<'tcx>) -> bool {
         let Some(clone_trait_def) = self.infcx.tcx.lang_items().clone_trait() else { return false };
         self.infcx
             .type_implements_trait(clone_trait_def, [ty], self.param_env)
@@ -1403,7 +1440,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
         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));
+            self.suggest_cloning(&mut err, ty, expr, self.find_expr(span), Some(move_spans));
         }
         self.buffer_error(err);
     }
@@ -1964,7 +2001,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
     pub(crate) fn find_expr(&self, span: Span) -> Option<&hir::Expr<'_>> {
         let tcx = self.infcx.tcx;
         let body_id = tcx.hir_node(self.mir_hir_id()).body_id()?;
-        let mut expr_finder = FindExprBySpan::new(span);
+        let mut expr_finder = FindExprBySpan::new(span, tcx);
         expr_finder.visit_expr(tcx.hir().body(body_id).value);
         expr_finder.result
     }
@@ -1998,14 +2035,14 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
             };
 
             let mut expr_finder =
-                FindExprBySpan::new(self.body.local_decls[*index1].source_info.span);
+                FindExprBySpan::new(self.body.local_decls[*index1].source_info.span, tcx);
             expr_finder.visit_expr(hir.body(body_id).value);
             let Some(index1) = expr_finder.result else {
                 note_default_suggestion();
                 return;
             };
 
-            expr_finder = FindExprBySpan::new(self.body.local_decls[*index2].source_info.span);
+            expr_finder = FindExprBySpan::new(self.body.local_decls[*index2].source_info.span, tcx);
             expr_finder.visit_expr(hir.body(body_id).value);
             let Some(index2) = expr_finder.result else {
                 note_default_suggestion();

compiler/rustc_borrowck/src/diagnostics/explain_borrow.rs

@@ -76,7 +76,7 @@ impl<'tcx> BorrowExplanation<'tcx> {
                 && let Some(body_id) = node.body_id()
             {
                 let body = tcx.hir().body(body_id);
-                let mut expr_finder = FindExprBySpan::new(span);
+                let mut expr_finder = FindExprBySpan::new(span, tcx);
                 expr_finder.visit_expr(body.value);
                 if let Some(mut expr) = expr_finder.result {
                     while let hir::ExprKind::AddrOf(_, _, inner)

compiler/rustc_borrowck/src/diagnostics/move_errors.rs

@@ -2,10 +2,13 @@
 #![allow(rustc::untranslatable_diagnostic)]
 
 use rustc_errors::{Applicability, Diag};
+use rustc_hir::intravisit::Visitor;
+use rustc_hir::{CaptureBy, ExprKind, HirId, Node};
 use rustc_middle::mir::*;
 use rustc_middle::ty::{self, Ty};
 use rustc_mir_dataflow::move_paths::{LookupResult, MovePathIndex};
 use rustc_span::{BytePos, ExpnKind, MacroKind, Span};
+use rustc_trait_selection::traits::error_reporting::FindExprBySpan;
 
 use crate::diagnostics::CapturedMessageOpt;
 use crate::diagnostics::{DescribePlaceOpt, UseSpans};
@@ -303,17 +306,133 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
         self.cannot_move_out_of(span, &description)
     }
 
+    fn suggest_clone_of_captured_var_in_move_closure(
+        &self,
+        err: &mut Diag<'_>,
+        upvar_hir_id: HirId,
+        upvar_name: &str,
+        use_spans: Option<UseSpans<'tcx>>,
+    ) {
+        let tcx = self.infcx.tcx;
+        let typeck_results = tcx.typeck(self.mir_def_id());
+        let Some(use_spans) = use_spans else { return };
+        // We only care about the case where a closure captured a binding.
+        let UseSpans::ClosureUse { args_span, .. } = use_spans else { return };
+        let Some(body_id) = tcx.hir_node(self.mir_hir_id()).body_id() else { return };
+        // Fetch the type of the expression corresponding to the closure-captured binding.
+        let Some(captured_ty) = typeck_results.node_type_opt(upvar_hir_id) else { return };
+        if !self.implements_clone(captured_ty) {
+            // We only suggest cloning the captured binding if the type can actually be cloned.
+            return;
+        };
+        // Find the closure that captured the binding.
+        let mut expr_finder = FindExprBySpan::new(args_span, tcx);
+        expr_finder.include_closures = true;
+        expr_finder.visit_expr(tcx.hir().body(body_id).value);
+        let Some(closure_expr) = expr_finder.result else { return };
+        let ExprKind::Closure(closure) = closure_expr.kind else { return };
+        // We'll only suggest cloning the binding if it's a `move` closure.
+        let CaptureBy::Value { .. } = closure.capture_clause else { return };
+        // Find the expression within the closure where the binding is consumed.
+        let mut suggested = false;
+        let use_span = use_spans.var_or_use();
+        let mut expr_finder = FindExprBySpan::new(use_span, tcx);
+        expr_finder.include_closures = true;
+        expr_finder.visit_expr(tcx.hir().body(body_id).value);
+        let Some(use_expr) = expr_finder.result else { return };
+        let parent = tcx.parent_hir_node(use_expr.hir_id);
+        if let Node::Expr(expr) = parent
+            && let ExprKind::Assign(lhs, ..) = expr.kind
+            && lhs.hir_id == use_expr.hir_id
+        {
+            // Cloning the value being assigned makes no sense:
+            //
+            // error[E0507]: cannot move out of `var`, a captured variable in an `FnMut` closure
+            //   --> $DIR/option-content-move2.rs:11:9
+            //    |
+            // LL |     let mut var = None;
+            //    |         ------- captured outer variable
+            // LL |     func(|| {
+            //    |          -- captured by this `FnMut` closure
+            // LL |         // Shouldn't suggest `move ||.as_ref()` here
+            // LL |         move || {
+            //    |         ^^^^^^^ `var` is moved here
+            // LL |
+            // LL |             var = Some(NotCopyable);
+            //    |             ---
+            //    |             |
+            //    |             variable moved due to use in closure
+            //    |             move occurs because `var` has type `Option<NotCopyable>`, which does not implement the `Copy` trait
+            //    |
+            return;
+        }
+
+        // Search for an appropriate place for the structured `.clone()` suggestion to be applied.
+        // If we encounter a statement before the borrow error, we insert a statement there.
+        for (_, node) in tcx.hir().parent_iter(closure_expr.hir_id) {
+            if let Node::Stmt(stmt) = node {
+                let padding = tcx
+                    .sess
+                    .source_map()
+                    .indentation_before(stmt.span)
+                    .unwrap_or_else(|| "    ".to_string());
+                err.multipart_suggestion_verbose(
+                    "clone the value before moving it into the closure",
+                    vec![
+                        (
+                            stmt.span.shrink_to_lo(),
+                            format!("let value = {upvar_name}.clone();\n{padding}"),
+                        ),
+                        (use_span, "value".to_string()),
+                    ],
+                    Applicability::MachineApplicable,
+                );
+                suggested = true;
+                break;
+            } else if let Node::Expr(expr) = node
+                && let ExprKind::Closure(_) = expr.kind
+            {
+                // We want to suggest cloning only on the first closure, not
+                // subsequent ones (like `ui/suggestions/option-content-move2.rs`).
+                break;
+            }
+        }
+        if !suggested {
+            // If we couldn't find a statement for us to insert a new `.clone()` statement before,
+            // we have a bare expression, so we suggest the creation of a new block inline to go
+            // from `move || val` to `{ let value = val.clone(); move || value }`.
+            let padding = tcx
+                .sess
+                .source_map()
+                .indentation_before(closure_expr.span)
+                .unwrap_or_else(|| "    ".to_string());
+            err.multipart_suggestion_verbose(
+                "clone the value before moving it into the closure",
+                vec![
+                    (
+                        closure_expr.span.shrink_to_lo(),
+                        format!("{{\n{padding}let value = {upvar_name}.clone();\n{padding}"),
+                    ),
+                    (use_spans.var_or_use(), "value".to_string()),
+                    (closure_expr.span.shrink_to_hi(), format!("\n{padding}}}")),
+                ],
+                Applicability::MachineApplicable,
+            );
+        }
+    }
+
     fn report_cannot_move_from_borrowed_content(
         &mut self,
         move_place: Place<'tcx>,
         deref_target_place: Place<'tcx>,
         span: Span,
         use_spans: Option<UseSpans<'tcx>>,
     ) -> Diag<'tcx> {
+        let tcx = self.infcx.tcx;
         // Inspect the type of the content behind the
         // borrow to provide feedback about why this
         // was a move rather than a copy.
-        let ty = deref_target_place.ty(self.body, self.infcx.tcx).ty;
+        let ty = deref_target_place.ty(self.body, tcx).ty;
         let upvar_field = self
             .prefixes(move_place.as_ref(), PrefixSet::All)
             .find_map(|p| self.is_upvar_field_projection(p));
@@ -363,8 +482,8 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
 
                 let upvar = &self.upvars[upvar_field.unwrap().index()];
                 let upvar_hir_id = upvar.get_root_variable();
-                let upvar_name = upvar.to_string(self.infcx.tcx);
-                let upvar_span = self.infcx.tcx.hir().span(upvar_hir_id);
+                let upvar_name = upvar.to_string(tcx);
+                let upvar_span = tcx.hir().span(upvar_hir_id);
 
                 let place_name = self.describe_any_place(move_place.as_ref());
 
@@ -380,12 +499,21 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
                     closure_kind_ty, closure_kind, place_description,
                 );
 
-                self.cannot_move_out_of(span, &place_description)
+                let closure_span = tcx.def_span(def_id);
+                let mut err = self
+                    .cannot_move_out_of(span, &place_description)
                     .with_span_label(upvar_span, "captured outer variable")
                     .with_span_label(
-                        self.infcx.tcx.def_span(def_id),
+                        closure_span,
                         format!("captured by this `{closure_kind}` closure"),
-                    )
+                    );
+                self.suggest_clone_of_captured_var_in_move_closure(
+                    &mut err,
+                    upvar_hir_id,
+                    &upvar_name,
+                    use_spans,
+                );
+                err
             }
             _ => {
                 let source = self.borrowed_content_source(deref_base);
@@ -415,7 +543,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
                     ),
                     (_, _, _) => self.cannot_move_out_of(
                         span,
-                        &source.describe_for_unnamed_place(self.infcx.tcx),
+                        &source.describe_for_unnamed_place(tcx),
                     ),
                 }
             }
@@ -447,7 +575,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
                     };
 
                     if let Some(expr) = self.find_expr(span) {
-                        self.suggest_cloning(err, place_ty, expr, self.find_expr(other_span));
+                        self.suggest_cloning(err, place_ty, expr, self.find_expr(other_span), None);
                     }
 
                     err.subdiagnostic(
@@ -482,7 +610,13 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
                 };
 
                 if let Some(expr) = self.find_expr(use_span) {
-                    self.suggest_cloning(err, place_ty, expr, self.find_expr(span));
+                    self.suggest_cloning(
+                        err,
+                        place_ty,
+                        expr,
+                        self.find_expr(span),
+                        Some(use_spans),
+                    );
                 }
 
                 err.subdiagnostic(
@@ -595,7 +729,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, '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);
+                    self.suggest_cloning(err, bind_to.ty, expr, None, None);
                 }
 
                 err.subdiagnostic(

compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs

@@ -50,22 +50,38 @@ pub struct FindExprBySpan<'hir> {
     pub span: Span,
     pub result: Option<&'hir hir::Expr<'hir>>,
     pub ty_result: Option<&'hir hir::Ty<'hir>>,
+    pub include_closures: bool,
+    pub tcx: TyCtxt<'hir>,
 }
 
 impl<'hir> FindExprBySpan<'hir> {
-    pub fn new(span: Span) -> Self {
-        Self { span, result: None, ty_result: None }
+    pub fn new(span: Span, tcx: TyCtxt<'hir>) -> Self {
+        Self { span, result: None, ty_result: None, tcx, include_closures: false }
     }
 }
 
 impl<'v> Visitor<'v> for FindExprBySpan<'v> {
+    type NestedFilter = rustc_middle::hir::nested_filter::OnlyBodies;
+
+    fn nested_visit_map(&mut self) -> Self::Map {
+        self.tcx.hir()
+    }
+
     fn visit_expr(&mut self, ex: &'v hir::Expr<'v>) {
         if self.span == ex.span {
             self.result = Some(ex);
         } else {
+            if let hir::ExprKind::Closure(..) = ex.kind
+                && self.include_closures
+                && let closure_header_sp = self.span.with_hi(ex.span.hi())
+                && closure_header_sp == ex.span
+            {
+                self.result = Some(ex);
+            }
             hir::intravisit::walk_expr(self, ex);
         }
     }
+
     fn visit_ty(&mut self, ty: &'v hir::Ty<'v>) {
         if self.span == ty.span {
             self.ty_result = Some(ty);