following needless_borrows_for_generic_args leads to unnecessary moves and makes code less refactoring-friendly

[RalfJung]

Description

Consider this code:

use std::path::*;

fn item(_x: impl AsRef<Path>) {}

fn work(x: PathBuf) {
    item(&x);
    //item(&x);
}

Clippy shows a warning here:

warning: the borrowed expression implements the required traits
 --> src/lib.rs:6:10
  |
6 |     item(&x);
  |          ^^ help: change this to: `x`
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrows_for_generic_args
  = note: `#[warn(clippy::needless_borrows_for_generic_args)]` on by default

I do not think this is good advice. For functions that take AsRef, I generally think one should give them borrowed arguments, for the following reasons:

• They will end up borrowing anyway, so there's no possible performance benefit from moving.
• If the code gets refactored later to use x more than once, I will get an error saying x has been moved already. Now I have to go up and change the use of x to &x. If I don't realize that I can change x to &x, then I might resort to x.clone(), which would be even worse. I generally prefer to write my code in a future-proof way that doesn't require further changes at the beginning of the function just because I want to change what I do at the end of the function.

You can simulate point 2 by uncommenting the second call to item. In the original code, that just works. If the code gets changed the way clippy wants, now this requires changing the first call.

In fact I think clippy should do the opposite of what it does today, and lint against this code:

use std::path::*;

fn item(_x: impl AsRef<Path>) {}

fn work(x: PathBuf) {
    item(x); // unnecesary move into `AsRef` function
}

That would steer people towards making it visible that we have borrowing here. But as a first step it'd be great if Clippy could stop linting against the pattern that avoids unnecessary moves and keeps code more refactoring-friendly.

Of course, the following should still lint, here the & is truly unnecessary since x is already Copy so passing it by-ref has no advantage (assuming that it is not prohibitively big and hence expensive to copy):

use std::path::*;

fn item(_x: impl AsRef<Path>) {}

pub fn work(x: &Path) {
    item(&x);
}

(That's also why just allowing needless_borrows_for_generic_args is not a great solution, the lint groups together two very different situations: borrows of already borrowed things that are just entirely redundant, and borrows of owned things that could be avoided in principle but it's less clear-cut whether that's a win.)

Version

Current stable (on playground)

Additional Labels

No response