Mention type that could be Clone but isn't in more cases

[estebank]

When encountering a moved value of a type that isn't Clone because of unmet obligations, but where all the unmet predicates reference crate-local types, mention them and suggest cloning, as we do in other cases already:

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

CC #68119.

[rustbot]

r? @SparrowLii

rustbot has assigned @SparrowLii.
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

[SparrowLii]

The impl looks fine, but I'm not sure suggesting using clone for all errors about value used after move is a best way. After all, it often reduces the performance and increases the redundancy. We should guide programmers to adopt better designs rather than simply workarounds IMO.

[SparrowLii]

@compiler-errors How do you think?

[estebank]

Do notice that for the case of a bare Foo in the description, we already emit the note. This just expands the same logic to also work for cases like Option<Foo>, where there are additional bounds that would be met if Foo was Clone. We should absolutely add more heuristics for when cloning would make sense or not (like checking how the value is consumed in context). Also, if #[derive(Foo)], we already suggest .clone() in the same situation.

After all, it often reduces the performance and increases the redundancy. We should guide programmers to adopt better designs rather than simply workarounds IMO.

I do think that we should provide better suggestions for architectural changes people should make, and we need to be very careful not to send people down the wrong path of writing brittle code. Specially because some people might assume that all suggestions are always correct (regardless of how much weasel wording like "consider" or "you might have meant" we use). But I disagree that cloning is inherently a problematic step for people to take, specially in code being worked on. The .clone() call is a beacon for people to come back to at a later time after they've gotten their code to work and (some?) unnecessary clones are caught by clippy.

There's a tension between the information that experienced rustaceans need and what newcomers need. But even as an experienced rustacean, faced with a situation like the one in the description:

#![allow(dead_code)]
use std::future::Future;

struct Foo;
impl Foo {
    fn foo(&self) -> bool {
        true
    }
}

fn require_fn_trait<F: Future<Output = ()>>(_: impl Fn() -> F) {}

fn do_stuff(foo: Option<Foo>) {
    require_fn_trait(|| async {
        if foo.map_or(false, |f| f.foo()) {
            panic!("foo");
        }
    })
}

fn main() {}

I would absolutely #[derive(Clone)], .clone() and move on for a bit, even if the more appropriate suggestion would have been making the parameter be impl FnOnce() -> F (in this case).

I think that's an argument to improve the logic before suggest_cloning is called, not to keep it's checks less complete than they could be.

[SparrowLii]

Thanks for the explanation, it makes sense

[SparrowLii]

@bors r+

[bors]

📌 Commit 168cead has been approved by SparrowLii

It is now in the queue for this repository.

[bors]

☔ The latest upstream changes (presumably #144440) made this pull request unmergeable. Please resolve the merge conflicts.

[estebank]

@bors r=SparrowLii

[bors]

📌 Commit 1106183 has been approved by SparrowLii

It is now in the queue for this repository.