implicit_clone shouldn't lint when called on a reference

[scottmcm]

Summary

I agree that for something like https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=bba81815705c82d64c18911576b5244a

#[deny(clippy::implicit_clone)]
pub fn dup(x: String) -> (String, String) {
    (x.to_owned(), x)
}

This is a good lint, because x is already owned, so clone better describes what's happening.

However, today the lint also complains if I'm starting with a &String, which definitely isn't owned, and I'd very much like to use .to_owned() on such things to clarify that what I want is something owned (as I describe in further detail in https://users.rust-lang.org/t/current-guidance-on-str-to-string-to-owned-vs-to-string/75754/7?u=scottmcm or https://users.rust-lang.org/t/what-is-the-difference-between-clone-and-to-owned/54705/3?u=scottmcm).

As such, I think this lint is over-aggressive and should be slightly tuned back. I would say that if the call to to_owned ends up needing auto-ref, like it does in the dup example above, it should lint, but if it doesn't need autoref (because the LHS is already a reference) then it should stop linting.

(Opened this issue because it came up on discord.)

Lint Name

implicit_clone

Reproducer

I tried this code:

#[deny(clippy::implicit_clone)]
pub fn foo(x: &String) -> String {
    x.to_owned()
}

I saw this happen:

error: implicitly cloning a `String` by calling `to_owned` on its dereferenced type
 --> src/lib.rs:3:5
  |
3 |     x.to_owned()
  |     ^^^^^^^^^^^^ help: consider using: `x.clone()`
  |

I expected to see this happen:

It compiles fine.

Version

(2023-07-31 db7ff98a72f3e742b641)

Additional Labels

No response

[J-ZhengLi]

However, today the lint also complains if I'm starting with a &String, which definitely isn't owned, and I'd very much like to use .to_owned() on such things to clarify that what I want is something owned

I agree, using to_owned would make more sense to me.

basically when I got a reference and I want to return an owned value, the first thing I'd think about was to_owned.

On the other hand, if I convert a borrow data "to owned", does that mean I now took the ownership of the one I borrowed? but in reality, it has became a cloned data of the borrowed one. So yes, both way would work, it's highly up to people's preference of which one should be used, just I'm a little bit leans toward to_owned which helps in terms of readability though~

[J-ZhengLi]

Also, from what I see in that first discussion link: Current guidance on &str to String (to_owned vs to_string), maybe this lint could also be branching out to one that lints against to_string calls where a simple to_owned/clone/into would do (if that sort of lint does not already exists).

@rustbot claim
for now

[scottmcm]

I don't know that there's agreement that str -> String should never use to_string, so linting on it is more uncertain, and thus I think it'd be better as a separate lint to give people more control over it separately from what they want with this lint.

[Centri3]

While to_owned's name makes more sense, I'm a bit unsure if it should always be used. ToOwned's purpose is borrowed -> owned, not just &T -> T, so if I see it, I know I'm going from a type I must borrow to an owned version of it; like &str -> String, or Path -> PathBuf. The name makes more sense, but enforcing it for all references may be a bit harsh, imo.

[scottmcm]

@Centri3 At least for what I was thinking in this issue was just that this link shouldn't complain if you use to_owned for &T -> T, not that it should insist on it.

I fully agree that reasonable people might be fine to use clone for that too.

[Centri3]

I see. I misunderstood then, Makes sense