Point at the Fn() or FnMut() bound that coerced a closure, which caused a move error

[estebank]

When encountering a move error involving a closure because the captured value isn't Copy, and the obligation comes from a bound on a type parameter that requires Fn or FnMut, we point at it and explain that an FnOnce wouldn't cause the move error.

error[E0507]: cannot move out of `foo`, a captured variable in an `Fn` closure
  --> f111.rs:15:25
   |
14 | fn do_stuff(foo: Option<Foo>) {
   |             ---  ----------- move occurs because `foo` has type `Option<Foo>`, which does not implement the `Copy` trait
   |             |
   |             captured outer variable
15 |     require_fn_trait(|| async {
   |                      -- ^^^^^ `foo` is moved here
   |                      |
   |                      captured by this `Fn` closure
16 |         if foo.map_or(false, |f| f.foo()) {
   |            --- variable moved due to use in coroutine
   |
help: `Fn` and `FnMut` closures require captured values to be able to be consumed multiple times, but an `FnOnce` consume them only once
  --> f111.rs:12:53
   |
12 | fn require_fn_trait<F: Future<Output = ()>>(_: impl Fn() -> F) {}
   |                                                     ^^^^^^^^^
help: consider cloning the value if the performance cost is acceptable
   |
16 |         if foo.clone().map_or(false, |f| f.foo()) {
   |               ++++++++

Fix #68119, by pointing at Fn and FnMut bounds involved in move errors.

[rustbot]

r? @lcnr

rustbot has assigned @lcnr.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

This PR modifies tests/ui/issues/. If this PR is adding new tests to tests/ui/issues/,
please refrain from doing so, and instead add it to more descriptive subdirectories.

[lcnr]

I personally don't think this change is worth it 🤔

I think that most functions which require an Fn/FnMut do so as they intend to call it multiple times, so suggesting to weaken this bound is likely to be incorrect.

While I think adding a note which explains that FnMut closures may be called multiple times and therefore must not consume their captured values could be useful to some users, I worry that these error messages are already very large: playground

error[E0507]: cannot move out of `foo`, a captured variable in an `Fn` closure
  --> src/lib.rs:11:25
   |
10 | fn do_stuff(foo: Option<Foo>) {
   |             ---  ----------- move occurs because `foo` has type `Option<Foo>`, which does not implement the `Copy` trait
   |             |
   |             captured outer variable
11 |     require_fn_trait(|| async {
   |                      -- ^^^^^ `foo` is moved here
   |                      |
   |                      captured by this `Fn` closure
12 |         if foo.map_or(false, |f| f.foo()) {
   |            --- variable moved due to use in coroutine
   |
note: if `Foo` implemented `Clone`, you could clone the value
  --> src/lib.rs:1:1
   |
1  | struct Foo;
   | ^^^^^^^^^^ consider implementing `Clone` for this type
...
12 |         if foo.map_or(false, |f| f.foo()) {
   |            --- you could clone this value

I think the main issue here is that we're pointing to Foo not being Clone instead of the fact that map_or moves the Option. We should extend our "use .as_ref()" suggestion to extend to this snippet but not add any additional information about the way Fn/FnMut closures work as while potentially useful, I think it's not worth the noise for most users (and stops being useful after seeing it for the first few times)

[estebank]

I think that most functions which require an Fn/FnMut do so as they intend to call it multiple times, so suggesting to weaken this bound is likely to be incorrect.

While I think adding a note which explains that FnMut closures may be called multiple times and therefore must not consume their captured values could be useful to some users, I worry that these error messages are already very large

I agree that the diagnostic for these cases is getting a bit long, my rule of thumb is that the more uncommon a diagnostic is the less problematic it is to be more communicative. For this case in particular there are 3 possibilities that a person would follow: 1) clone the value (what we already suggest), 2) use .as_ref() (which only works when Option or Result are involved) or 3) changing the closure needed. I feel that when encountering these situations, the best thing we can do for our users is give them as much context as possible. For these cases we can't know a priori what the right solution would be. Looking through the affected tests tests/ui/borrowck/unboxed-closures-move-upvar-from-non-once-ref-closure.stderr is the closest one to be aided by 2), and even then it is not a case of calling as_ref() but rather calling .iter() instead of .into_iter().

I think the main issue here is that we're pointing to Foo not being Clone instead of the fact that map_or moves the Option. We should extend our "use .as_ref()" suggestion to extend to this snippet but not add any additional information about the way Fn/FnMut closures work as while potentially useful, I think it's not worth the noise for most users (and stops being useful after seeing it for the first few times)

I think that cloning is the most natural thing someone would try, getting ahead of them and letting them know that the type isn't Clone has value, imo. We should provide the context of why a closure became of a given type. Specially for FnMut. It might be that a closure being required to be FnMut is strong signal that the closure isn't intended to be FnOnce, but I don't think that an Fn closure gives us the same strong signal, it is just the "default" people would lean towards.

I'm just concerned with not showing information that is needed in a subset of cases because it is always adding to the verbosity. Specially for ownership errors, which can be notoriously tricky to understand when not given enough information. For errors that happen more often (E0277, E0308), I'd be more concerned with optimizing for terse output.

[estebank]

CC #47850, which is the same case handled here for function calls but for methods.

[lcnr]

the impl looks fine. I personally feel like large let-chains are hard to read and should ideally be split into multiple statements e.g. using let-else in a sub-fn.

This makes it easier to chunk the match into multiple steps

• get the parent call expr if the closure is a call arg
  • actually, we should also support MethodCall if we want to land this
• check whether the call has a Fn|FnMut where-bound with the closure as a self type

And it's also clearer which of these lets should be fallible, e.g. we should always have a Node::Expr for closures

[lcnr]

We should provide the context of why a closure became of a given type. Specially for FnMut. It might be that a closure being required to be FnMut is strong signal that the closure isn't intended to be FnOnce, but I don't think that an Fn closure gives us the same strong signal, it is just the "default" people would lean towards.

hmm, I prefer just adding the note is fine without the explicit suggestion, but don't strongly care, so I am also fine with keeping it

[estebank]

Moved the logic to its own method, removed the label and added support for methods.

[lcnr]

I would personally prefer returning Option<Span> and doing unwrap_or(DUMMY_SP) in the caller, but that's just a stylistic preference 🤷

I am a bit annoyed as the function was still hard to read for me. I think my main issue here is that similar to text, it's useful to split your code into paragraphs, distinct steps separated by an empty line/a comment. That's my main issue with let-chains I think, that they often result in very long sections where it's unclear what each step is doing and what's actually relevant.

r=me after these nits

[estebank]

@bors r=lcnr

[bors]

📌 Commit 74496bc has been approved by lcnr

It is now in the queue for this repository.