Auto merge of #95257 - compiler-errors:fn-borrow, r=lcnr

Add suggestion to borrow `Fn` and `FnMut` params/opaque/closures instead of move

I think that Closure/ParamTy/Opaque are all "opaque" enough that it's meaningful to suggest borrowing them instead of moving them at their usage sites when we see a move error. See the attached issue for example.

Is this suggestion too general? I could perhaps use the move site information to limit this to places like fn calls, but I don't know enough about mir borrowck to know if that's an easy change.

Fixes #90828

compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs

@@ -12,7 +12,7 @@ use rustc_middle::mir::{
     FakeReadCause, LocalDecl, LocalInfo, LocalKind, Location, Operand, Place, PlaceRef,
     ProjectionElem, Rvalue, Statement, StatementKind, Terminator, TerminatorKind, VarBindingForm,
 };
-use rustc_middle::ty::{self, suggest_constraining_type_params, PredicateKind, Ty};
+use rustc_middle::ty::{self, subst::Subst, suggest_constraining_type_params, PredicateKind, Ty};
 use rustc_mir_dataflow::move_paths::{InitKind, MoveOutIndex, MovePathIndex};
 use rustc_span::symbol::sym;
 use rustc_span::{BytePos, MultiSpan, Span};
@@ -151,6 +151,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                         .args_or_use()
                 })
                 .collect::<Vec<Span>>();
+
             let reinits = maybe_reinitialized_locations.len();
             if reinits == 1 {
                 err.span_label(reinit_spans[0], "this reinitialization might get skipped");
@@ -276,76 +277,23 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                 }
             }
 
+            let opt_name =
+                self.describe_place_with_options(place.as_ref(), IncludingDowncast(true));
+            let note_msg = match opt_name {
+                Some(ref name) => format!("`{}`", name),
+                None => "value".to_owned(),
+            };
+            if self.suggest_borrow_fn_like(&mut err, ty, &move_site_vec, &note_msg) {
+                // Suppress the next suggestion since we don't want to put more bounds onto
+                // something that already has `Fn`-like bounds (or is a closure), so we can't
+                // restrict anyways.
+            } else {
+                self.suggest_adding_copy_bounds(&mut err, ty, span);
+            }
+
             if needs_note {
-                let opt_name =
-                    self.describe_place_with_options(place.as_ref(), IncludingDowncast(true));
-                let note_msg = match opt_name {
-                    Some(ref name) => format!("`{}`", name),
-                    None => "value".to_owned(),
-                };
-
-                // Try to find predicates on *generic params* that would allow copying `ty`
-                let tcx = self.infcx.tcx;
-                let generics = tcx.generics_of(self.mir_def_id());
-                if let Some(hir_generics) = tcx
-                    .typeck_root_def_id(self.mir_def_id().to_def_id())
-                    .as_local()
-                    .and_then(|def_id| tcx.hir().get_generics(def_id))
-                {
-                    let predicates: Result<Vec<_>, _> = tcx.infer_ctxt().enter(|infcx| {
-                        let mut fulfill_cx =
-                            <dyn rustc_infer::traits::TraitEngine<'_>>::new(infcx.tcx);
-
-                        let copy_did = infcx.tcx.lang_items().copy_trait().unwrap();
-                        let cause = ObligationCause::new(
-                            span,
-                            self.mir_hir_id(),
-                            rustc_infer::traits::ObligationCauseCode::MiscObligation,
-                        );
-                        fulfill_cx.register_bound(
-                            &infcx,
-                            self.param_env,
-                            // Erase any region vids from the type, which may not be resolved
-                            infcx.tcx.erase_regions(ty),
-                            copy_did,
-                            cause,
-                        );
-                        // Select all, including ambiguous predicates
-                        let errors = fulfill_cx.select_all_or_error(&infcx);
-
-                        // Only emit suggestion if all required predicates are on generic
-                        errors
-                            .into_iter()
-                            .map(|err| match err.obligation.predicate.kind().skip_binder() {
-                                PredicateKind::Trait(predicate) => {
-                                    match predicate.self_ty().kind() {
-                                        ty::Param(param_ty) => Ok((
-                                            generics.type_param(param_ty, tcx),
-                                            predicate.trait_ref.print_only_trait_path().to_string(),
-                                        )),
-                                        _ => Err(()),
-                                    }
-                                }
-                                _ => Err(()),
-                            })
-                            .collect()
-                    });
-
-                    if let Ok(predicates) = predicates {
-                        suggest_constraining_type_params(
-                            tcx,
-                            hir_generics,
-                            &mut err,
-                            predicates.iter().map(|(param, constraint)| {
-                                (param.name.as_str(), &**constraint, None)
-                            }),
-                        );
-                    }
-                }
-
                 let span = if let Some(local) = place.as_local() {
-                    let decl = &self.body.local_decls[local];
+                    Some(self.body.local_decls[local].source_info.span)
-                    Some(decl.source_info.span)
                 } else {
                     None
                 };
@@ -373,6 +321,144 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
         }
     }
 
+    fn suggest_borrow_fn_like(
+        &self,
+        err: &mut DiagnosticBuilder<'tcx, ErrorGuaranteed>,
+        ty: Ty<'tcx>,
+        move_sites: &[MoveSite],
+        value_name: &str,
+    ) -> bool {
+        let tcx = self.infcx.tcx;
+
+        // Find out if the predicates show that the type is a Fn or FnMut
+        let find_fn_kind_from_did = |predicates: &[(ty::Predicate<'tcx>, Span)], substs| {
+            predicates.iter().find_map(|(pred, _)| {
+                let pred = if let Some(substs) = substs {
+                    pred.subst(tcx, substs).kind().skip_binder()
+                } else {
+                    pred.kind().skip_binder()
+                };
+                if let ty::PredicateKind::Trait(pred) = pred && pred.self_ty() == ty {
+                    if Some(pred.def_id()) == tcx.lang_items().fn_trait() {
+                        return Some(hir::Mutability::Not);
+                    } else if Some(pred.def_id()) == tcx.lang_items().fn_mut_trait() {
+                        return Some(hir::Mutability::Mut);
+                    }
+                }
+                None
+            })
+        };
+
+        // If the type is opaque/param/closure, and it is Fn or FnMut, let's suggest (mutably)
+        // borrowing the type, since `&mut F: FnMut` iff `F: FnMut` and similarly for `Fn`.
+        // These types seem reasonably opaque enough that they could be substituted with their
+        // borrowed variants in a function body when we see a move error.
+        let borrow_level = match ty.kind() {
+            ty::Param(_) => find_fn_kind_from_did(
+                tcx.explicit_predicates_of(self.mir_def_id().to_def_id()).predicates,
+                None,
+            ),
+            ty::Opaque(did, substs) => {
+                find_fn_kind_from_did(tcx.explicit_item_bounds(*did), Some(*substs))
+            }
+            ty::Closure(_, substs) => match substs.as_closure().kind() {
+                ty::ClosureKind::Fn => Some(hir::Mutability::Not),
+                ty::ClosureKind::FnMut => Some(hir::Mutability::Mut),
+                _ => None,
+            },
+            _ => None,
+        };
+
+        let Some(borrow_level) = borrow_level else { return false; };
+        let sugg = move_sites
+            .iter()
+            .map(|move_site| {
+                let move_out = self.move_data.moves[(*move_site).moi];
+                let moved_place = &self.move_data.move_paths[move_out.path].place;
+                let move_spans = self.move_spans(moved_place.as_ref(), move_out.source);
+                let move_span = move_spans.args_or_use();
+                let suggestion = if borrow_level == hir::Mutability::Mut {
+                    "&mut ".to_string()
+                } else {
+                    "&".to_string()
+                };
+                (move_span.shrink_to_lo(), suggestion)
+            })
+            .collect();
+        err.multipart_suggestion_verbose(
+            &format!(
+                "consider {}borrowing {value_name}",
+                if borrow_level == hir::Mutability::Mut { "mutably " } else { "" }
+            ),
+            sugg,
+            Applicability::MaybeIncorrect,
+        );
+        true
+    }
+
+    fn suggest_adding_copy_bounds(
+        &self,
+        err: &mut DiagnosticBuilder<'tcx, ErrorGuaranteed>,
+        ty: Ty<'tcx>,
+        span: Span,
+    ) {
+        let tcx = self.infcx.tcx;
+        let generics = tcx.generics_of(self.mir_def_id());
+
+        let Some(hir_generics) = tcx
+            .typeck_root_def_id(self.mir_def_id().to_def_id())
+            .as_local()
+            .and_then(|def_id| tcx.hir().get_generics(def_id))
+        else { return; };
+        // Try to find predicates on *generic params* that would allow copying `ty`
+        let predicates: Result<Vec<_>, _> = tcx.infer_ctxt().enter(|infcx| {
+            let mut fulfill_cx = <dyn rustc_infer::traits::TraitEngine<'_>>::new(infcx.tcx);
+
+            let copy_did = infcx.tcx.lang_items().copy_trait().unwrap();
+            let cause = ObligationCause::new(
+                span,
+                self.mir_hir_id(),
+                rustc_infer::traits::ObligationCauseCode::MiscObligation,
+            );
+            fulfill_cx.register_bound(
+                &infcx,
+                self.param_env,
+                // Erase any region vids from the type, which may not be resolved
+                infcx.tcx.erase_regions(ty),
+                copy_did,
+                cause,
+            );
+            // Select all, including ambiguous predicates
+            let errors = fulfill_cx.select_all_or_error(&infcx);
+
+            // Only emit suggestion if all required predicates are on generic
+            errors
+                .into_iter()
+                .map(|err| match err.obligation.predicate.kind().skip_binder() {
+                    PredicateKind::Trait(predicate) => match predicate.self_ty().kind() {
+                        ty::Param(param_ty) => Ok((
+                            generics.type_param(param_ty, tcx),
+                            predicate.trait_ref.print_only_trait_path().to_string(),
+                        )),
+                        _ => Err(()),
+                    },
+                    _ => Err(()),
+                })
+                .collect()
+        });
+
+        if let Ok(predicates) = predicates {
+            suggest_constraining_type_params(
+                tcx,
+                hir_generics,
+                err,
+                predicates
+                    .iter()
+                    .map(|(param, constraint)| (param.name.as_str(), &**constraint, None)),
+            );
+        }
+    }
+
     pub(crate) fn report_move_out_while_borrowed(
         &mut self,
         location: Location,

src/test/ui/chalkify/closure.stderr

@@ -12,6 +12,10 @@ note: closure cannot be moved more than once as it is not `Copy` due to moving t
    |
 LL |         a = 1;
    |         ^
+help: consider mutably borrowing `b`
+   |
+LL |     let mut c = &mut b;
+   |                 ++++
 
 error: aborting due to previous error
 

src/test/ui/closures/2229_closure_analysis/diagnostics/closure-origin-multi-variant-diagnostics.stderr

@@ -11,6 +11,10 @@ note: closure cannot be moved more than once as it is not `Copy` due to moving t
    |
 LL |         if let MultiVariant::Point(ref mut x, _) = point {
    |                                                    ^^^^^
+help: consider mutably borrowing `c`
+   |
+LL |     let a = &mut c;
+   |             ++++
 
 error: aborting due to previous error
 

src/test/ui/closures/2229_closure_analysis/diagnostics/closure-origin-single-variant-diagnostics.stderr

@@ -11,6 +11,10 @@ note: closure cannot be moved more than once as it is not `Copy` due to moving t
    |
 LL |         let SingleVariant::Point(ref mut x, _) = point;
    |                                                  ^^^^^
+help: consider mutably borrowing `c`
+   |
+LL |     let b = &mut c;
+   |             ++++
 
 error: aborting due to previous error
 

src/test/ui/closures/2229_closure_analysis/diagnostics/closure-origin-struct-diagnostics.stderr

@@ -11,6 +11,10 @@ note: closure cannot be moved more than once as it is not `Copy` due to moving t
    |
 LL |         x.y.a += 1;
    |         ^^^^^
+help: consider mutably borrowing `hello`
+   |
+LL |     let b = &mut hello;
+   |             ++++
 
 error: aborting due to previous error
 

src/test/ui/closures/2229_closure_analysis/diagnostics/closure-origin-tuple-diagnostics-1.stderr

@@ -11,6 +11,10 @@ note: closure cannot be moved more than once as it is not `Copy` due to moving t
    |
 LL |         x.0 += 1;
    |         ^^^
+help: consider mutably borrowing `hello`
+   |
+LL |     let b = &mut hello;
+   |             ++++
 
 error: aborting due to previous error
 

src/test/ui/moves/borrow-closures-instead-of-move.rs

@@ -0,0 +1,36 @@
+fn takes_fn(f: impl Fn()) {
+    loop {
+        takes_fnonce(f);
+        //~^ ERROR use of moved value
+        //~| HELP consider borrowing
+    }
+}
+
+fn takes_fn_mut(m: impl FnMut()) {
+    if maybe() {
+        takes_fnonce(m);
+        //~^ HELP consider mutably borrowing
+    }
+    takes_fnonce(m);
+    //~^ ERROR use of moved value
+}
+
+fn has_closure() {
+    let mut x = 0;
+    let mut closure = || {
+        x += 1;
+    };
+    takes_fnonce(closure);
+    //~^ HELP consider mutably borrowing
+    closure();
+    //~^ ERROR borrow of moved value
+}
+
+fn maybe() -> bool {
+    false
+}
+
+// Could also be Fn[Mut], here it doesn't matter
+fn takes_fnonce(_: impl FnOnce()) {}
+
+fn main() {}

src/test/ui/moves/borrow-closures-instead-of-move.stderr

@@ -0,0 +1,53 @@
+error[E0382]: use of moved value: `f`
+  --> $DIR/borrow-closures-instead-of-move.rs:3:22
+   |
+LL | fn takes_fn(f: impl Fn()) {
+   |             - move occurs because `f` has type `impl Fn()`, which does not implement the `Copy` trait
+LL |     loop {
+LL |         takes_fnonce(f);
+   |                      ^ value moved here, in previous iteration of loop
+   |
+help: consider borrowing `f`
+   |
+LL |         takes_fnonce(&f);
+   |                      +
+
+error[E0382]: use of moved value: `m`
+  --> $DIR/borrow-closures-instead-of-move.rs:14:18
+   |
+LL | fn takes_fn_mut(m: impl FnMut()) {
+   |                 - move occurs because `m` has type `impl FnMut()`, which does not implement the `Copy` trait
+LL |     if maybe() {
+LL |         takes_fnonce(m);
+   |                      - value moved here
+...
+LL |     takes_fnonce(m);
+   |                  ^ value used here after move
+   |
+help: consider mutably borrowing `m`
+   |
+LL |         takes_fnonce(&mut m);
+   |                      ++++
+
+error[E0382]: borrow of moved value: `closure`
+  --> $DIR/borrow-closures-instead-of-move.rs:25:5
+   |
+LL |     takes_fnonce(closure);
+   |                  ------- value moved here
+LL |
+LL |     closure();
+   |     ^^^^^^^ value borrowed here after move
+   |
+note: closure cannot be moved more than once as it is not `Copy` due to moving the variable `x` out of its environment
+  --> $DIR/borrow-closures-instead-of-move.rs:21:9
+   |
+LL |         x += 1;
+   |         ^
+help: consider mutably borrowing `closure`
+   |
+LL |     takes_fnonce(&mut closure);
+   |                  ++++
+
+error: aborting due to 3 previous errors
+
+For more information about this error, try `rustc --explain E0382`.

src/test/ui/moves/moves-based-on-type-no-recursive-stack-closure.stderr

@@ -17,10 +17,10 @@ LL |     let mut r = R {c: Box::new(f)};
 LL |     f(&mut r, false)
    |     ^ value borrowed here after move
    |
-help: consider further restricting this bound
+help: consider mutably borrowing `f`
    |
-LL | fn conspirator<F>(mut f: F) where F: FnMut(&mut R, bool) + Copy {
+LL |     let mut r = R {c: Box::new(&mut f)};
-   |                                                          ++++++
+   |                                ++++
 
 error: aborting due to 2 previous errors
 

src/test/ui/not-copy-closure.stderr

@@ -11,6 +11,10 @@ note: closure cannot be moved more than once as it is not `Copy` due to moving t
    |
 LL |         a += 1;
    |         ^
+help: consider mutably borrowing `hello`
+   |
+LL |     let b = &mut hello;
+   |             ++++
 
 error: aborting due to previous error