Implemented suspicious_to_owned lint to check if to_owned is called on a Cow

[xanathar]

changelog: Add lint [`suspicious_to_owned`]

---

Hi,
posting this unsolicited PR as I've been burned by this issue :)
Being unsolicited, feel free to reject it or reassign a different lint level etc.

This lint checks whether to_owned is called on Cow<'_, _>. This is done because to_owned is very similarly named to into_owned, but the effect of calling those two methods is completely different (one makes the Cow::Borrowed into a Cow::Owned, the other just clones the Cow). If the cow is then passed to code for which the type is not checked (e.g. generics, closures, etc.) it might slip through and if the cow data is coming from an unsafe context there is the potential for accidentally cause undefined behavior.
Even if not falling into this painful case, there's really no reason to call to_owned on a Cow other than confusing people reading the code: either into_owned or clone should be called.

Note that this overlaps perfectly with implicit_clone as a warning, but implicit_clone is classified pedantic (while the consequences for Cow might be of a wider blast radius than just pedantry); given the overlap, I set-up the lint so that if suspicious_to_owned triggers implicit_clone will not trigger. I'm not 100% sure this is done in the correct way (I tried to copy what other lints were doing) so please provide feedback on it if it isn't.

Checklist

• Followed [lint naming conventions][lint_naming]
• Added passing UI tests (including committed .stderr file)
• cargo test passes locally
• Executed cargo dev update_lints
• Added lint documentation
• Run cargo dev fmt

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @llogiq (or someone else) soon.

Please see the contribution instructions for more information.

[xanathar]

Converted to draft to fix the test issue (that for some reason doesn't manifest locally :( )

[xanathar]

CI is happy now, marking ready for review.

[ghost]

This looks OK but shouldn't redundant_clone warn about this? See also #2387.

[ghost]

Here are the warnings from running this on something like the 1000 top crates.

---> quick-xml-0.23.0/src/events/mod.rs:126:21                        
warning: this call to `to_owned` does not cause the std::borrow::Cow<[u8]> contents to become owned
   --> src/events/mod.rs:126:21
    |
126 |         Self::owned(self.buf.to_owned(), self.name_len)
    |                     ^^^^^^^^^^^^^^^^^^^ help: consider using: `self.buf.into_owned()`
    |
    = note: requested on the command line with `-W clippy::suspicious-to-owned`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_to_owned
                                                                      
---> sentry-0.26.0/src/transports/reqwest.rs:58:26                    
warning: this call to `to_owned` does not cause the std::borrow::Cow<str> contents to become owned
  --> src/transports/reqwest.rs:58:26
   |
58 |         let user_agent = options.user_agent.to_owned();
   |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `options.user_agent.into_owned()`
   |
   = note: requested on the command line with `-W clippy::suspicious-to-owned`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_to_owned

---> toml-0.5.9/src/de.rs:457:34                                      
warning: this call to `to_owned` does not cause the std::borrow::Cow<str> contents to become owned
   --> src/de.rs:457:34
    |
457 |                         .map(|k| k.1.to_owned())
    |                                  ^^^^^^^^^^^^^^ help: consider using: `k.1.into_owned()`
    |
    = note: requested on the command line with `-W clippy::suspicious-to-owned`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_to_owned
                                                                      
---> toml-0.5.9/src/de.rs:689:20                                      
warning: this call to `to_owned` does not cause the std::borrow::Cow<str> contents to become owned
   --> src/de.rs:689:20
    |
689 |         let name = table.header[table.header.len() - 1].1.to_owned();
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `table.header[table.header.len() - 1].1.into_owned()`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_to_owned
                                                                      
---> vcpkg-0.2.15/src/lib.rs:314:21                                   
warning: this call to `to_owned` does not cause the std::borrow::Cow<str> contents to become owned
   --> src/lib.rs:314:21
    |
314 |                     vcpkg_user_targets_path.to_string_lossy().to_owned()
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `vcpkg_user_targets_path.to_string_lossy().into_owned()`
    |
    = note: requested on the command line with `-W clippy::suspicious-to-owned`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_to_owned

It looks like that for most of them the advice of using to_owned() won't work and the intention is to clone. I haven't looked at them too closely.

[xanathar]

Thanks, running this on the top 1000 crates makes sense.

Quick-xml is certainly conscious of the caveats and in some way perpetrates the same to_owned vs into_owned argument (see comment in code ).

Vpckg use of to_owned is totally redundant (code compiles even after removing it) and notably does not seem to raise neither redundant_clone nor implicit_clone (I should really double check though, I admit I checked in a hurry -- anyway it's not the point).

That said, I can see how these usages (especially the choice made in quick-xml, even if I honestly disagree with it) make a strong argument against this lint.

The reason against relying on implicit_clone is because it's only at the pedantic level, plus it really is aesthetic only.

The issue I found was when using Cows coming from unsafe contexts like:

#[no_mangle]
pub extern "C" fn delayed_print(cstr: *const c_char) {
    let mystr = unsafe { CStr::from_ptr(cstr) }.to_string_lossy().to_owned();

    tokio::task::spawn(async move {
        println!("{}", mystr);
    });
}

The above is UB and very prone to all kinds of crash with to_owned, while it's safe and correct with into_owned. Now, the core of the issue of the above code could be the programmer not jumping through hoops to get a proper lifetime over that CStr, but unless one has been burned before the above code seems sound (it's not!) and can easily slip through a code review.

This the reason why I was suggesting it for a more specific lint with a higher warning level, but I can see the reasons against this argument so feel free to close the PR in that case.

[bors]

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

[Jarcho]

Should the suggestion not be to suggest using clone in that case? I agree that to_owned on Cow is very misleading while clone would be much clearer in intent. A second suggestion to use into_owned could also be made using Applicability::MaybeIncorrect

redundant_clone doesn't work with nested lifetimes.

[xanathar]

You're probably right that suggesting to use clone is better as it would not suggest to have a different semantic than the one written but suggest to use a more readable alternative, and anybody who wrote to_owned by mistake would catch the difference. I'll do the change.

(I'll also rebase the changes on top of the latest master head -- sorry for the delay, but was away)

[llogiq]

Sorry, I was absent for a while. I think the lint docs could use some minor rewording, otherwise I think we can merge this after a rebase.

[xanathar]

I rebased and changed the wording so that it's less opinionated between clone() and into_owned, and now it suggests both replacements, "depending on intent".

Let me know if it's fine or other changes to the wording are required.

[bors]

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

[llogiq]

Thank you! r=me with another rebase.

[xanathar]

Rebased on the latest master but forgot to update_lints 🤷‍♂️

Fixed the mistake and exercising CI again.

[llogiq]

Thank you!

@bors r+

[bors]

📌 Commit de028e2 has been approved by llogiq

It is now in the queue for this repository.

[bors]

⌛ Testing commit de028e2 with merge 63af75b...

[bors]

💔 Test failed - checks-action_test

[xanathar]

Not sure about the merge failure here.
I tried to do (*) a rebase on the latest master and it went without conflicts; cargo tests also not failing after it, update_lints did not change any files, and neither did cargo dev fmt.

I'd be happy to help this landed but... I need a clue :)

(*) I did the rebase locally and didn't push so not to break the review validity, but can push it if it helps of course.

[Jarcho]

The changelog: part needs to be on one line.

@bors retry

[bors]

⌛ Testing commit de028e2 with merge 2d4d8e1...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 2d4d8e1 to master...

[morrisonlevi]

Hello, I don't understand the premise. Can you help me understand? If I am currently borrowing a Cow<str>, for instance, and I need to copy this to pass it to something else, why wouldn't .to_owned() be called? A clone is insufficient if lifetimes don't match like when the string comes from an FFI source, but since I don't own the Cow, I can't .into_owned() it either, right?

[Jarcho]

to_owned and clone on &Cow<'a, T> do the same thing. They both create a new Cow<'a, T> with the same state as the old one. This is unlike to_owned on &'a str which would allocate a new String, and clone which would create a new &'a str.

[ghost]

The Cow to_owned implementation just calls clone. You can find this in the Cow documentation.