Rollup merge of #68816 - estebank:fn-mut-closure, r=varkor

Tweak borrow error on `FnMut` when `Fn` is expected

Fix #31701, fix #66097.

src/librustc_mir/borrow_check/diagnostics/mutability_errors.rs

@@ -10,7 +10,7 @@ use rustc_span::Span;
 use crate::borrow_check::diagnostics::BorrowedContentSource;
 use crate::borrow_check::MirBorrowckCtxt;
 use crate::util::collect_writes::FindAssignments;
-use rustc_errors::Applicability;
+use rustc_errors::{Applicability, DiagnosticBuilder};
 
 #[derive(Copy, Clone, Debug, Eq, PartialEq)]
 pub(crate) enum AccessKind {
@@ -412,11 +412,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
                 projection: [ProjectionElem::Deref],
                 // FIXME document what is this 1 magic number about
             } if local == Local::new(1) && !self.upvars.is_empty() => {
-                err.span_label(span, format!("cannot {ACT}", ACT = act));
-                err.span_help(
-                    self.body.span,
-                    "consider changing this to accept closures that implement `FnMut`",
-                );
+                self.expected_fn_found_fn_mut_call(&mut err, span, act);
             }
 
             PlaceRef { local: _, projection: [.., ProjectionElem::Deref] } => {
@@ -448,6 +444,101 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
 
         err.buffer(&mut self.errors_buffer);
     }
+
+    /// Targetted error when encountering an `FnMut` closure where an `Fn` closure was expected.
+    fn expected_fn_found_fn_mut_call(&self, err: &mut DiagnosticBuilder<'_>, sp: Span, act: &str) {
+        err.span_label(sp, format!("cannot {}", act));
+
+        let hir = self.infcx.tcx.hir();
+        let closure_id = hir.as_local_hir_id(self.mir_def_id).unwrap();
+        let fn_call_id = hir.get_parent_node(closure_id);
+        let node = hir.get(fn_call_id);
+        let item_id = hir.get_parent_item(fn_call_id);
+        let mut look_at_return = true;
+        // If we can detect the expression to be an `fn` call where the closure was an argument,
+        // we point at the `fn` definition argument...
+        match node {
+            hir::Node::Expr(hir::Expr { kind: hir::ExprKind::Call(func, args), .. }) => {
+                let arg_pos = args
+                    .iter()
+                    .enumerate()
+                    .filter(|(_, arg)| arg.span == self.body.span)
+                    .map(|(pos, _)| pos)
+                    .next();
+                let def_id = hir.local_def_id(item_id);
+                let tables = self.infcx.tcx.typeck_tables_of(def_id);
+                if let Some(ty::FnDef(def_id, _)) =
+                    tables.node_type_opt(func.hir_id).as_ref().map(|ty| &ty.kind)
+                {
+                    let arg = match hir.get_if_local(*def_id) {
+                        Some(hir::Node::Item(hir::Item {
+                            ident,
+                            kind: hir::ItemKind::Fn(sig, ..),
+                            ..
+                        }))
+                        | Some(hir::Node::TraitItem(hir::TraitItem {
+                            ident,
+                            kind: hir::TraitItemKind::Method(sig, _),
+                            ..
+                        }))
+                        | Some(hir::Node::ImplItem(hir::ImplItem {
+                            ident,
+                            kind: hir::ImplItemKind::Method(sig, _),
+                            ..
+                        })) => Some(
+                            arg_pos
+                                .and_then(|pos| {
+                                    sig.decl.inputs.get(
+                                        pos + if sig.decl.implicit_self.has_implicit_self() {
+                                            1
+                                        } else {
+                                            0
+                                        },
+                                    )
+                                })
+                                .map(|arg| arg.span)
+                                .unwrap_or(ident.span),
+                        ),
+                        _ => None,
+                    };
+                    if let Some(span) = arg {
+                        err.span_label(span, "change this to accept `FnMut` instead of `Fn`");
+                        err.span_label(func.span, "expects `Fn` instead of `FnMut`");
+                        if self.infcx.tcx.sess.source_map().is_multiline(self.body.span) {
+                            err.span_label(self.body.span, "in this closure");
+                        }
+                        look_at_return = false;
+                    }
+                }
+            }
+            _ => {}
+        }
+        if look_at_return && hir.get_return_block(closure_id).is_some() {
+            // ...otherwise we are probably in the tail expression of the function, point at the
+            // return type.
+            match hir.get(hir.get_parent_item(fn_call_id)) {
+                hir::Node::Item(hir::Item { ident, kind: hir::ItemKind::Fn(sig, ..), .. })
+                | hir::Node::TraitItem(hir::TraitItem {
+                    ident,
+                    kind: hir::TraitItemKind::Method(sig, _),
+                    ..
+                })
+                | hir::Node::ImplItem(hir::ImplItem {
+                    ident,
+                    kind: hir::ImplItemKind::Method(sig, _),
+                    ..
+                }) => {
+                    err.span_label(ident.span, "");
+                    err.span_label(
+                        sig.decl.output.span(),
+                        "change this to return `FnMut` instead of `Fn`",
+                    );
+                    err.span_label(self.body.span, "in this closure");
+                }
+                _ => {}
+            }
+        }
+    }
 }
 
 fn suggest_ampmut_self<'tcx>(

src/test/ui/borrowck/borrow-immutable-upvar-mutation.rs

@@ -18,7 +18,10 @@ fn main() {
         let _g = to_fn(|| set(&mut y)); //~ ERROR cannot borrow
 
         let mut z = 0;
-        let _h = to_fn_mut(|| { set(&mut z); to_fn(|| z = 42); }); //~ ERROR cannot assign
+        let _h = to_fn_mut(|| {
+            set(&mut z);
+            to_fn(|| z = 42); //~ ERROR cannot assign
+        });
     }
 
     // By-value captures
@@ -33,3 +36,19 @@ fn main() {
         let _h = to_fn_mut(move || { set(&mut z); to_fn(move || z = 42); }); //~ ERROR cannot assign
     }
 }
+
+fn foo() -> Box<dyn Fn() -> usize> {
+    let mut x = 0;
+    Box::new(move || {
+        x += 1; //~ ERROR cannot assign
+        x
+    })
+}
+
+fn bar() -> impl Fn() -> usize {
+    let mut x = 0;
+    move || {
+        x += 1; //~ ERROR cannot assign
+        x
+    }
+}

src/test/ui/borrowck/borrow-immutable-upvar-mutation.stderr

@@ -1,76 +1,97 @@
 error[E0594]: cannot assign to `x`, as it is a captured variable in a `Fn` closure
   --> $DIR/borrow-immutable-upvar-mutation.rs:15:27
    |
+LL | fn to_fn<A,F:Fn<A>>(f: F) -> F { f }
+   |                        - change this to accept `FnMut` instead of `Fn`
+...
 LL |         let _f = to_fn(|| x = 42);
-   |                           ^^^^^^ cannot assign
-   |
-help: consider changing this to accept closures that implement `FnMut`
-  --> $DIR/borrow-immutable-upvar-mutation.rs:15:24
-   |
-LL |         let _f = to_fn(|| x = 42);
-   |                        ^^^^^^^^^
+   |                  -----    ^^^^^^ cannot assign
+   |                  |
+   |                  expects `Fn` instead of `FnMut`
 
 error[E0596]: cannot borrow `y` as mutable, as it is a captured variable in a `Fn` closure
   --> $DIR/borrow-immutable-upvar-mutation.rs:18:31
    |
+LL | fn to_fn<A,F:Fn<A>>(f: F) -> F { f }
+   |                        - change this to accept `FnMut` instead of `Fn`
+...
 LL |         let _g = to_fn(|| set(&mut y));
-   |                               ^^^^^^ cannot borrow as mutable
-   |
-help: consider changing this to accept closures that implement `FnMut`
-  --> $DIR/borrow-immutable-upvar-mutation.rs:18:24
-   |
-LL |         let _g = to_fn(|| set(&mut y));
-   |                        ^^^^^^^^^^^^^^
+   |                  -----        ^^^^^^ cannot borrow as mutable
+   |                  |
+   |                  expects `Fn` instead of `FnMut`
 
 error[E0594]: cannot assign to `z`, as it is a captured variable in a `Fn` closure
-  --> $DIR/borrow-immutable-upvar-mutation.rs:21:55
-   |
-LL |         let _h = to_fn_mut(|| { set(&mut z); to_fn(|| z = 42); });
-   |                                                       ^^^^^^ cannot assign
-   |
-help: consider changing this to accept closures that implement `FnMut`
-  --> $DIR/borrow-immutable-upvar-mutation.rs:21:52
-   |
-LL |         let _h = to_fn_mut(|| { set(&mut z); to_fn(|| z = 42); });
-   |                                                    ^^^^^^^^^
+  --> $DIR/borrow-immutable-upvar-mutation.rs:23:22
+   |
+LL | fn to_fn<A,F:Fn<A>>(f: F) -> F { f }
+   |                        - change this to accept `FnMut` instead of `Fn`
+...
+LL |             to_fn(|| z = 42);
+   |             -----    ^^^^^^ cannot assign
+   |             |
+   |             expects `Fn` instead of `FnMut`
 
 error[E0594]: cannot assign to `x`, as it is a captured variable in a `Fn` closure
-  --> $DIR/borrow-immutable-upvar-mutation.rs:27:32
-   |
-LL |         let _f = to_fn(move || x = 42);
-   |                                ^^^^^^ cannot assign
-   |
-help: consider changing this to accept closures that implement `FnMut`
-  --> $DIR/borrow-immutable-upvar-mutation.rs:27:24
+  --> $DIR/borrow-immutable-upvar-mutation.rs:30:32
    |
+LL | fn to_fn<A,F:Fn<A>>(f: F) -> F { f }
+   |                        - change this to accept `FnMut` instead of `Fn`
+...
 LL |         let _f = to_fn(move || x = 42);
-   |                        ^^^^^^^^^^^^^^
+   |                  -----         ^^^^^^ cannot assign
+   |                  |
+   |                  expects `Fn` instead of `FnMut`
 
 error[E0596]: cannot borrow `y` as mutable, as it is a captured variable in a `Fn` closure
-  --> $DIR/borrow-immutable-upvar-mutation.rs:30:36
-   |
-LL |         let _g = to_fn(move || set(&mut y));
-   |                                    ^^^^^^ cannot borrow as mutable
-   |
-help: consider changing this to accept closures that implement `FnMut`
-  --> $DIR/borrow-immutable-upvar-mutation.rs:30:24
+  --> $DIR/borrow-immutable-upvar-mutation.rs:33:36
    |
+LL | fn to_fn<A,F:Fn<A>>(f: F) -> F { f }
+   |                        - change this to accept `FnMut` instead of `Fn`
+...
 LL |         let _g = to_fn(move || set(&mut y));
-   |                        ^^^^^^^^^^^^^^^^^^^
+   |                  -----             ^^^^^^ cannot borrow as mutable
+   |                  |
+   |                  expects `Fn` instead of `FnMut`
 
 error[E0594]: cannot assign to `z`, as it is a captured variable in a `Fn` closure
-  --> $DIR/borrow-immutable-upvar-mutation.rs:33:65
+  --> $DIR/borrow-immutable-upvar-mutation.rs:36:65
    |
+LL | fn to_fn<A,F:Fn<A>>(f: F) -> F { f }
+   |                        - change this to accept `FnMut` instead of `Fn`
+...
 LL |         let _h = to_fn_mut(move || { set(&mut z); to_fn(move || z = 42); });
-   |                                                                 ^^^^^^ cannot assign
-   |
-help: consider changing this to accept closures that implement `FnMut`
-  --> $DIR/borrow-immutable-upvar-mutation.rs:33:57
-   |
-LL |         let _h = to_fn_mut(move || { set(&mut z); to_fn(move || z = 42); });
-   |                                                         ^^^^^^^^^^^^^^
+   |                                                   -----         ^^^^^^ cannot assign
+   |                                                   |
+   |                                                   expects `Fn` instead of `FnMut`
+
+error[E0594]: cannot assign to `x`, as it is a captured variable in a `Fn` closure
+  --> $DIR/borrow-immutable-upvar-mutation.rs:43:9
+   |
+LL |   fn foo() -> Box<dyn Fn() -> usize> {
+   |      ---      ---------------------- change this to return `FnMut` instead of `Fn`
+LL |       let mut x = 0;
+LL |       Box::new(move || {
+   |  ______________-
+LL | |         x += 1;
+   | |         ^^^^^^ cannot assign
+LL | |         x
+LL | |     })
+   | |_____- in this closure
+
+error[E0594]: cannot assign to `x`, as it is a captured variable in a `Fn` closure
+  --> $DIR/borrow-immutable-upvar-mutation.rs:51:9
+   |
+LL |   fn bar() -> impl Fn() -> usize {
+   |      ---      ------------------ change this to return `FnMut` instead of `Fn`
+LL |       let mut x = 0;
+LL | /     move || {
+LL | |         x += 1;
+   | |         ^^^^^^ cannot assign
+LL | |         x
+LL | |     }
+   | |_____- in this closure
 
-error: aborting due to 6 previous errors
+error: aborting due to 8 previous errors
 
 Some errors have detailed explanations: E0594, E0596.
 For more information about an error, try `rustc --explain E0594`.

src/test/ui/borrowck/borrow-raw-address-of-mutability.stderr

@@ -27,32 +27,32 @@ LL |     f();
 error[E0596]: cannot borrow `x` as mutable, as it is a captured variable in a `Fn` closure
   --> $DIR/borrow-raw-address-of-mutability.rs:29:17
    |
-LL |         let y = &raw mut x;
-   |                 ^^^^^^^^^^ cannot borrow as mutable
-   |
-help: consider changing this to accept closures that implement `FnMut`
-  --> $DIR/borrow-raw-address-of-mutability.rs:28:21
-   |
+LL |   fn make_fn<F: Fn()>(f: F) -> F { f }
+   |                          - change this to accept `FnMut` instead of `Fn`
+...
 LL |       let f = make_fn(|| {
-   |  _____________________^
+   |  _____________-------_-
+   | |             |
+   | |             expects `Fn` instead of `FnMut`
 LL | |         let y = &raw mut x;
+   | |                 ^^^^^^^^^^ cannot borrow as mutable
 LL | |     });
-   | |_____^
+   | |_____- in this closure
 
 error[E0596]: cannot borrow `x` as mutable, as it is a captured variable in a `Fn` closure
   --> $DIR/borrow-raw-address-of-mutability.rs:37:17
    |
-LL |         let y = &raw mut x;
-   |                 ^^^^^^^^^^ cannot borrow as mutable
-   |
-help: consider changing this to accept closures that implement `FnMut`
-  --> $DIR/borrow-raw-address-of-mutability.rs:36:21
-   |
+LL |   fn make_fn<F: Fn()>(f: F) -> F { f }
+   |                          - change this to accept `FnMut` instead of `Fn`
+...
 LL |       let f = make_fn(move || {
-   |  _____________________^
+   |  _____________-------_-
+   | |             |
+   | |             expects `Fn` instead of `FnMut`
 LL | |         let y = &raw mut x;
+   | |                 ^^^^^^^^^^ cannot borrow as mutable
 LL | |     });
-   | |_____^
+   | |_____- in this closure
 
 error: aborting due to 5 previous errors