Warn when using to_owned on already owned types

[kangalio]

What it does

By virtue of deferencing to &[T], Vec implements several methods such as .to_owned() or .to_vec(). When called on Vec, these methods do the same thing as .clone(), all the while being more confusing ("why call to_owned or to_vec on something that already is a Vec and is owned?")

Categories

• Kind: style

What is the advantage of the recommended code over the original code

As mentioned above, to_vec and to_owned are potentially misleading and confusing when used on Vec. clone is a clearer alternative.

Drawbacks

None.

Example

let a = vec![1, 2, 3];
let b = a.to_vec();

Could be written as:

let a = vec![1, 2, 3];
let b = a.clone();

Additional comments

Same applies to String with .to_owned() and .to_string(). Or PathBuf with .to_owned() and to_path_buf(). Or I suppose CString with .to_owned(). There's a lot of these cases in the standard library actually. Should those cases be handled using the same lint?

[camsteffen]

Good idea.

There's a lot of these cases in the standard library actually. Should those cases be handled using the same lint?

Yes 👍 We can call the lint owned_to_owned.

[anall]

@rustbot claim

I think I've got something mostly working for this, but I am not sure if I am doing anything the proper way, or if there is a better way to go about this.

I have a few hangups with the "Additional Comments" -- and I still need to figure out how to handle PathBuf or ArbitraryCustomType for that (and how to convert those to snake case path_buf and arbitrary_custom_type)

[camsteffen]

@anall Feel free to ask questions on Zulip or open a work in progress PR.

Converting to snake case is not the right approach. Instead there should be a list of known "to owned" functions that are equivalent to clone. It is okay if the lint only covers one type like Vec at first.

[anall]

Re the "Additional comments", as far as I can tell with a really rough heuristic-filtered grep, these are the types in the standard library that have a "to_type" method (ignoring Owned which trips my heuristic given how the trait is set up):

• OsString
• PathBuf
• String (to_string is already handled by string_to_string, but could be moved to be handled here)
• Vec

[camsteffen]

I like the idea of replacing string_to_string. Seems like the exact same concept. @flip1995 do you agree? Incidentally it would be upgraded to style.

[flip1995]

I would just keep string_to_string, since it is a restriction lint. I wouldn't make this lint warn-by-default, because calling to_owned instead of clone doesn't really make a difference. And if the clone is unnecessary, redundant_clone should pick it up.