review comments

Added comments and reworded messages

4 files changed, 48 insertions, 20 deletions

diff --git a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
index ae17caf75b6..31bcb54c74b 100644
--- a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
+++ b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
@@ -1006,7 +1006,9 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
         can_suggest_clone
     }
 
-    fn suggest_cloning_on_spread_operator(
+    /// We have `S { foo: val, ..base }`, and we suggest instead writing
+    /// `S { foo: val, bar: base.bar.clone(), .. }` when valid.
+    fn suggest_cloning_on_functional_record_update(
         &self,
         err: &mut Diag<'_>,
         ty: Ty<'tcx>,
@@ -1046,7 +1048,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
         for field in &variant.fields {
             // In practice unless there are more than one field with the same type, we'll be
             // suggesting a single field at a type, because we don't aggregate multiple borrow
-            // checker errors involving the spread operator into a single one.
+            // checker errors involving the functional record update sytnax into a single one.
             let field_ty = field.ty(self.infcx.tcx, args);
             let ident = field.ident(self.infcx.tcx);
             if field_ty == ty && fields.iter().all(|field| field.ident.name != ident.name) {
@@ -1088,7 +1090,8 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
             String::new()
         };
         let msg = format!(
-            "{prefix}clone the value from the field instead of using the spread operator syntax",
+            "{prefix}clone the value from the field instead of using the functional record update \
+             syntax",
         );
         err.span_suggestion_verbose(span, msg, sugg, Applicability::MachineApplicable);
     }
@@ -1105,7 +1108,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
             // `Location` that covers both the `S { ... }` literal, all of its fields and the
             // `base`. If the move happens because of `S { foo: val, bar: base.bar }` the `expr`
             //  will already be correct. Instead, we see if we can suggest writing.
-            self.suggest_cloning_on_spread_operator(err, ty, expr);
+            self.suggest_cloning_on_functional_record_update(err, ty, expr);
             return;
         }
 
@@ -1225,6 +1228,8 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
             .must_apply_modulo_regions()
     }
 
+    /// Given an expression, check if it is a method call `foo.clone()`, where `foo` and
+    /// `foo.clone()` both have the same type, returning the span for `.clone()` if so.
     pub(crate) fn clone_on_reference(&self, expr: &hir::Expr<'_>) -> Option<Span> {
         let typeck_results = self.infcx.tcx.typeck(self.mir_def_id());
         if let hir::ExprKind::MethodCall(segment, rcvr, args, span) = expr.kind
@@ -1276,6 +1281,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
             };
         let mut sugg = Vec::with_capacity(2);
         let mut inner_expr = expr;
+        // Remove uses of `&` and `*` when suggesting `.clone()`.
         while let hir::ExprKind::AddrOf(.., inner) | hir::ExprKind::Unary(hir::UnOp::Deref, inner) =
             &inner_expr.kind
         {
@@ -1841,13 +1847,14 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
         &self,
         err: &mut Diag<'_>,
         ty: Ty<'tcx>,
-        def_id: DefId,
+        trait_def_id: DefId,
         span: Span,
     ) {
-        self.suggest_adding_bounds(err, ty, def_id, span);
+        self.suggest_adding_bounds(err, ty, trait_def_id, span);
         if let ty::Adt(..) = ty.kind() {
-            // The type doesn't implement DefId.
-            let trait_ref = ty::Binder::dummy(ty::TraitRef::new(self.infcx.tcx, def_id, [ty]));
+            // The type doesn't implement the trait.
+            let trait_ref =
+                ty::Binder::dummy(ty::TraitRef::new(self.infcx.tcx, trait_def_id, [ty]));
             let obligation = Obligation::new(
                 self.infcx.tcx,
                 ObligationCause::dummy(),
diff --git a/compiler/rustc_borrowck/src/diagnostics/mod.rs b/compiler/rustc_borrowck/src/diagnostics/mod.rs
index 53697b4b8b0..dbea317e7bb 100644
--- a/compiler/rustc_borrowck/src/diagnostics/mod.rs
+++ b/compiler/rustc_borrowck/src/diagnostics/mod.rs
@@ -1049,6 +1049,27 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                             is_loop_message,
                         },
                     );
+                    // Check if the move occurs on a value because of a call on a closure that comes
+                    // from a type parameter `F: FnOnce()`. If so, we provide a targeted `note`:
+                    // ```
+                    // error[E0382]: use of moved value: `blk`
+                    //   --> $DIR/once-cant-call-twice-on-heap.rs:8:5
+                    //    |
+                    // LL | fn foo<F:FnOnce()>(blk: F) {
+                    //    |                    --- move occurs because `blk` has type `F`, which does not implement the `Copy` trait
+                    // LL | blk();
+                    //    | ----- `blk` moved due to this call
+                    // LL | blk();
+                    //    | ^^^ value used here after move
+                    //    |
+                    // note: `FnOnce` closures can only be called once
+                    //   --> $DIR/once-cant-call-twice-on-heap.rs:6:10
+                    //    |
+                    // LL | fn foo<F:FnOnce()>(blk: F) {
+                    //    |        ^^^^^^^^ `F` is made to be an `FnOnce` closure here
+                    // LL | blk();
+                    //    | ----- this value implements `FnOnce`, which causes it to be moved when called
+                    // ```
                     if let ty::Param(param_ty) = self_ty.kind()
                         && let generics = self.infcx.tcx.generics_of(self.mir_def_id())
                         && let param = generics.type_param(param_ty, self.infcx.tcx)
diff --git a/tests/ui/borrowck/borrowck-struct-update-with-dtor.stderr b/tests/ui/borrowck/borrowck-struct-update-with-dtor.stderr
index d34b07fbcc6..bc11204acf2 100644
--- a/tests/ui/borrowck/borrowck-struct-update-with-dtor.stderr
+++ b/tests/ui/borrowck/borrowck-struct-update-with-dtor.stderr
@@ -12,7 +12,7 @@ note: `B` doesn't implement `Copy` or `Clone`
    |
 LL | struct B;
    | ^^^^^^^^
-help: if `B` implemented `Clone`, you could clone the value from the field instead of using the spread operator syntax
+help: if `B` implemented `Clone`, you could clone the value from the field instead of using the functional record update syntax
    |
 LL |         let _s2 = S { a: 2, b: s0.b.clone(), ..s0 };
    |                           +++++++++++++++++
@@ -31,7 +31,7 @@ note: `B` doesn't implement `Copy` or `Clone`
    |
 LL | struct B;
    | ^^^^^^^^
-help: if `B` implemented `Clone`, you could clone the value from the field instead of using the spread operator syntax
+help: if `B` implemented `Clone`, you could clone the value from the field instead of using the functional record update syntax
    |
 LL |         let _s2 = S { a: 2, b: s0.b.clone(), c: s0.c.clone() };
    |                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
@@ -50,7 +50,7 @@ note: `B` doesn't implement `Copy` or `Clone`
    |
 LL | struct B;
    | ^^^^^^^^
-help: if `B` implemented `Clone`, you could clone the value from the field instead of using the spread operator syntax
+help: if `B` implemented `Clone`, you could clone the value from the field instead of using the functional record update syntax
    |
 LL |         let _s2 = S { a: 2, b: s0.b.clone(), c: s0.c.clone() };
    |                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
@@ -69,7 +69,7 @@ note: `B` doesn't implement `Copy` or `Clone`
    |
 LL | struct B;
    | ^^^^^^^^
-help: if `B` implemented `Clone`, you could clone the value from the field instead of using the spread operator syntax
+help: if `B` implemented `Clone`, you could clone the value from the field instead of using the functional record update syntax
    |
 LL |         let _s2 = S { a: 2, b: s0.b.clone(), ..s0 };
    |                           +++++++++++++++++
@@ -83,7 +83,7 @@ LL |         let _s2 = S { a: 2, ..s0 };
    |                   cannot move out of here
    |                   move occurs because `s0.c` has type `K`, which does not implement the `Copy` trait
    |
-help: clone the value from the field instead of using the spread operator syntax
+help: clone the value from the field instead of using the functional record update syntax
    |
 LL |         let _s2 = S { a: 2, c: s0.c.clone(), ..s0 };
    |                           +++++++++++++++++
@@ -97,7 +97,7 @@ LL |         let _s2 = T { a: 2, ..s0 };
    |                   cannot move out of here
    |                   move occurs because `s0.b` has type `Box<isize>`, which does not implement the `Copy` trait
    |
-help: clone the value from the field instead of using the spread operator syntax
+help: clone the value from the field instead of using the functional record update syntax
    |
 LL |         let _s2 = T { a: 2, b: s0.b.clone() };
    |                           ~~~~~~~~~~~~~~~~~
@@ -111,7 +111,7 @@ LL |         let _s2 = T { ..s0 };
    |                   cannot move out of here
    |                   move occurs because `s0.b` has type `Box<isize>`, which does not implement the `Copy` trait
    |
-help: clone the value from the field instead of using the spread operator syntax
+help: clone the value from the field instead of using the functional record update syntax
    |
 LL |         let _s2 = T { b: s0.b.clone(), ..s0 };
    |                     ~~~~~~~~~~~~~~~~~~~~~~~~~
@@ -139,7 +139,7 @@ LL |         let _s2 = V { a: 2, ..s0 };
    |                   cannot move out of here
    |                   move occurs because `s0.b` has type `Box<isize>`, which does not implement the `Copy` trait
    |
-help: clone the value from the field instead of using the spread operator syntax
+help: clone the value from the field instead of using the functional record update syntax
    |
 LL |         let _s2 = V { a: 2, b: s0.b.clone(), ..s0 };
    |                           +++++++++++++++++
@@ -153,7 +153,7 @@ LL |         let _s2 = V { a: 2, ..s0 };
    |                   cannot move out of here
    |                   move occurs because `s0.c` has type `K`, which does not implement the `Copy` trait
    |
-help: clone the value from the field instead of using the spread operator syntax
+help: clone the value from the field instead of using the functional record update syntax
    |
 LL |         let _s2 = V { a: 2, c: s0.c.clone(), ..s0 };
    |                           +++++++++++++++++
@@ -167,7 +167,7 @@ LL |         let _s2 = V { a: 2, ..s0 };
    |                   cannot move out of here
    |                   move occurs because `s0.b` has type `Box<isize>`, which does not implement the `Copy` trait
    |
-help: clone the value from the field instead of using the spread operator syntax
+help: clone the value from the field instead of using the functional record update syntax
    |
 LL |         let _s2 = V { a: 2, b: s0.b.clone(), ..s0 };
    |                           +++++++++++++++++
@@ -181,7 +181,7 @@ LL |         let _s2 = V { a: 2, ..s0 };
    |                   cannot move out of here
    |                   move occurs because `s0.c` has type `Clonable`, which does not implement the `Copy` trait
    |
-help: clone the value from the field instead of using the spread operator syntax
+help: clone the value from the field instead of using the functional record update syntax
    |
 LL |         let _s2 = V { a: 2, c: s0.c.clone(), ..s0 };
    |                           +++++++++++++++++
diff --git a/tests/ui/functional-struct-update/functional-struct-update-noncopyable.stderr b/tests/ui/functional-struct-update/functional-struct-update-noncopyable.stderr
index 3f07142eeb7..d167a60dad3 100644
--- a/tests/ui/functional-struct-update/functional-struct-update-noncopyable.stderr
+++ b/tests/ui/functional-struct-update/functional-struct-update-noncopyable.stderr
@@ -7,7 +7,7 @@ LL |     let _b = A { y: Arc::new(3), ..a };
    |              cannot move out of here
    |              move occurs because `a.x` has type `Arc<isize>`, which does not implement the `Copy` trait
    |
-help: clone the value from the field instead of using the spread operator syntax
+help: clone the value from the field instead of using the functional record update syntax
    |
 LL |     let _b = A { y: Arc::new(3), x: a.x.clone() };
    |                                ~~~~~~~~~~~~~~~~