Avoid redundant clone suggestions in borrowck diagnostics

[yuk1ty]

Fixes #153886

Removed redundant .clone() suggestions.

I found that there are two patterns to handle this issue while I was implementing:

• Should suggest only UFCS
• Should suggest only simple .clone()

For the target issue, we can just remove the UFCS (<Option<String> as Clone>::clone(&selection.1)) side.

However, for the BorrowedContentSource::OverloadedDeref pattern like Rc<Vec<i32>>, for instance the borrowck-move-out-of-overloaded-auto-deref.rs test case, I think we need to employ the UFCS way. The actual test case is:

//@ run-rustfix
use std::rc::Rc;

pub fn main() {
    let _x = Rc::new(vec![1, 2]).into_iter();
    //~^ ERROR [E0507]
}

And another error will be shown if we simply use the simple .clone() pattern. Like:

use std::rc::Rc;

pub fn main() {
    let _x = Rc::new(vec![1, 2]).clone().into_iter();
}

then we will get

error[E0507]: cannot move out of an `Rc`
   --> src/main.rs:5:14
    |
  5 |     let _x = Rc::new(vec![1, 2]).clone().into_iter();
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^ ----------- value moved due to this method call
    |              |
    |              move occurs because value has type `Vec<i32>`, which does not implement the `Copy` trait
    |
note: `into_iter` takes ownership of the receiver `self`, which moves value
   --> /playground/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/iter/traits/collect.rs:310:18
    |
310 |     fn into_iter(self) -> Self::IntoIter;
    |                  ^^^^
help: you can `clone` the value and consume it, but this might not be your desired behavior
    |
  5 -     let _x = Rc::new(vec![1, 2]).clone().into_iter();
  5 +     let _x = <Vec<i32> as Clone>::clone(&Rc::new(vec![1, 2])).into_iter();
    |

For more information about this error, try `rustc --explain E0507`.

Rust Playground

In this case, Rc::clone only increments the reference count and returns a new Rc<Vec<i32>>; it does not grant ownership of the inner Vec<i32>. As a result, calling into_iter() attempts to move the Vec<i32>, leading to the same E0507 error again.

On the other hand, in UFCS form:

<Vec<i32> as Clone>::clone(&Rc::new(vec![1, 2])).into_iter()

This explicitly calls <Vec<i32> as Clone>::clone, and the argument &Rc<Vec<i32>> is treated as &Vec<i32> via Rc’s Deref implementation. As a result, the Vec<i32> itself is cloned, yielding an owned Vec<i32>, which allows into_iter() to succeed, if my understanding is correct.

I addressed the issue as far as I could find the edge cases but please advice me if I'm overlooking something.

[rustbot]

r? @adwinwhite

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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: borrowck, compiler
• borrowck, compiler expanded to 69 candidates
• Random selection from 13 candidates

[yuk1ty]

The failed tests tell me another case of the issue. Let me mark PR as draft.

[Kivooeo]

it's fine to use bool here instead of enum

[yuk1ty]

Thank you, fixed this https://github.com/rust-lang/rust/pull/154197/changes#diff-3c0de6462469af483c9ecdf2c4b00cb26192218ef2d5c62a0fde75107a74caaeR1007-R1009

[Kivooeo]

I'll possibly take a look today or tomorrow. I'm rather busy at the moment, so I wouldn't expect this to be that quick, to be honest.

[adwinwhite]

It seems that @Kivooeo is quite busy. I'll have a look.

[adwinwhite]

Some nits, otherwise it's a very nice improvement. Thank you for working on this!

[adwinwhite]

This PR also improves the diag for the case below, I think.
Could you add this as a test if it's confirmed to be fixed?

use std::ops::Deref;

struct MyBox<T>(T);

impl<T> Deref for MyBox<T> {
    type Target = T;
    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

pub fn main() {
    let _x = MyBox(vec![1, 2]).into_iter();
    // currently we have a simple clone help which is wrong. it's better to have UFCS.
}

[adwinwhite]

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[yuk1ty]

Thank you! Let me try 👍

[adwinwhite]

Sorry, forgot to submit inline reviews.

View changes since this review

[adwinwhite]

This isn't a real result. I think the wildcard binding is unnecessary.

[adwinwhite]

Would it be better to use a simple enum as return type? For readability and less indirect wrapper.

[adwinwhite]

We don't really get what the bool means from the fn signature.