Add new useless_allocation lint

[GuillaumeGomez]

Fixes #8088.

I'm not too happy with this implementation as it currently limits the detection to String and Vec. If someone has an idea on how to check that the "argument type" (the one that was being converted) can be used as argument directly without all the checks I added, it'd allow this lint to work on all types implementing Borrow<T>.

changelog: Add new useless_allocation lint

[rustbot]

r? @Manishearth

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

[GuillaumeGomez]

Ah funny, it already allowed to find a case in clippy itself. :)

[Manishearth]

r? clippy

rather busy at the moment

[Manishearth]

issue: I think this lint name is quite general for how specific the actual lint is

[matthiaskrgr]

its a bit as if needless_clone and unnecessary_to_owned had a child together.

[y21]

That makes me wonder if it would make sense to just extend unnecessary_to_owned with these additional cases.

[y21]

For example, unnecessary_to_owned already lints on:

fn f(x: &str) {}
f(&"".to_owned());

[GuillaumeGomez]

its a bit as if needless_clone and unnecessary_to_owned had a child together.

Pretty much yes. 😆

@y21 I wondered about that as well but since it's covering than just to_owned call, I felt like it should not be there. Same goes for needless_clone since it's not related to clone.

[GuillaumeGomez]

For example, unnecessary_to_owned already lints on:

fn f(x: &str) {}
f(&"".to_owned());

But it doesn't work when the argument is generic (yet?) unfortunately.

[GuillaumeGomez]

So what do you want me to do here? Rename it or merge it with another existing lint?

[y21]

In my opinion, merging it with unnecessary_to_owned makes the most sense.

I wondered about that as well but since it's covering than just to_owned call

But the lint description for it also says "[...] and other to_owned-like functions", and also covers the same to_owned, to_vec and to_string functions.

But it doesn't work when the argument is generic (yet?) unfortunately.

I mentioned this in my comment, but it does seem to understand some generic cases:

fn f<K: Deref<Target=str>>(x: K) {}
f("".to_owned());

This gets linted

[GuillaumeGomez]

So, in the case of the maps, it gets a bit more complicated because the argument is a generic Q and Q doesn't have the Borrow trait as predicate (as you can see here). So I can merge both lints, but it might require a bit more work than just adding a check for Borrow in this case...

[GuillaumeGomez]

I also tested to add the Borrow trait as check and it doesn't detect this code:

fn borrow_str<Q: Borrow<str>>(s: &Q) {}
fn borrow_slice<Q: Borrow<[u8]>>(s: &Q) {}
fn check_borrow() {
    borrow_str(&"a".to_owned());
    borrow_str(&"a".to_string());
    borrow_slice(&[0].to_vec());
}

So I'll merge this code with the lint for now.

[samueltardieu]

s/is/in/ ?

[y21]

If someone has an idea on how to check that the "argument type" (the one that was being converted) can be used as argument directly without all the checks I added, it'd allow this lint to work on all types implementing Borrow.

This should be possible by using expr_use_ctxt on the addrof expression. The ExprUseNode can tell us the DefId of the function or method being called, and also the argument position.
We can use tcx.fn_sig with that DefId to get the signature of the fn being called (the argument position from expr_use_ctxt should allow us to find the "required" type from the signature) and we can use tcx.predicates_of to find the bounds on that function.

We should then be able to substitute the generic parameter with the borrowed expression's type (i.e., the unadjusted ty of x in &x.to_owned()) in the list of predicates and see if they hold.

We're already doing pretty much exactly that in the useless_conversion lint: https://github.com/rust-lang/rust-clippy/blob/master/clippy_lints/src/useless_conversion.rs#L66 , this might help here.
I think having this in clippy_utils would be useful.

---

That said, I'd also be fine with just landing a simpler version which hardcodes a bunch of cases like this, and it could be improved in followups, if that's easier

[y21]

... Actually, now looking closer into the unnecessary_to_owned lint, much of this "try and see if all predicates also hold for the borrowed type" logic already seems to exist, but it limits itself to K: Deref<str> and AsRef bounds specifically. Maybe we can just add Borrow there?

[y21]

@GuillaumeGomez with #12324 opened as an alternative, do we want to close this one?

[GuillaumeGomez]

Absolutely!