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

RalfJung · 2024-03-10T07:31:24Z

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

The text was updated successfully, but these errors were encountered:

Current implementation looks for significant drops, that can change the behavior, but that's not enough - value might not have a Drop itself but one of its children might have it. A good example is passing a reference to `PathBuf` to `std::fs::File::open`. There's no benefits to pass `PathBuf` by value, but since clippy can't see `Drop` on `Vec` several layers down it complains forcing pass by value and making it impossible to use the same name later. New implementation only looks at variables and only those that are copy, so existing variable will never be moved but things that take a string reference created and value is created inplace `&"".to_owned()` will make it to suggest to use `"".to_owned()` still. Fixes rust-lang/rust-clippy#12454

Current implementation looks for significant drops, that can change the behavior, but that's not enough - value might not have a Drop itself but one of its children might have it. A good example is passing a reference to `PathBuf` to `std::fs::File::open`. There's no benefits to pass `PathBuf` by value, but since clippy can't see `Drop` on `Vec` several layers down it complains forcing pass by value and making it impossible to use the same name later. New implementation only looks at variables and only those that are copy, so existing variable will never be moved but things that take a string reference created and value is created inplace `&"".to_owned()` will make it to suggest to use `"".to_owned()` still. Fixes rust-lang#12454

Current implementation looks for significant drops, that can change the behavior, but that's not enough - value might not have a Drop itself but one of its children might have it. A good example is passing a reference to `PathBuf` to `std::fs::File::open`. There's no benefits to pass `PathBuf` by value, but since clippy can't see `Drop` on `Vec` several layers down it complains forcing pass by value and making it impossible to use the same name later. New implementation only looks at copy values or values created inplace so existing variable will never be moved but things that take a string reference created and value is created inplace `&"".to_owned()` will make it to suggest to use `"".to_owned()` still. Fixes rust-lang#12454

less aggressive needless_borrows_for_generic_args Current implementation looks for significant drops, that can change the behavior, but that's not enough - value might not have a `Drop` itself but one of its children might have it. A good example is passing a reference to `PathBuf` to `std::fs::File::open`. There's no benefits to pass `PathBuf` by value, but since `clippy` can't see `Drop` on `Vec` several layers down it complains forcing pass by value and making it impossible to use the same name later. New implementation only looks at copy values or values created in place so existing variable will never be moved but things that take a string reference created and value is created inplace `&"".to_owned()` will make it to suggest to use `"".to_owned()` still. Fixes #12454 changelog: [`needless_borrows_for_generic_args`]: avoid moving variables

apoelstra mentioned this issue Mar 14, 2024

Add check to max difficulty transition threshold rust-bitcoin/rust-bitcoin#2337

Merged

pacak mentioned this issue Apr 23, 2024

clippy: less aggressive needless_borrows_for_generic_args rust-lang/rust#124292

Closed

pacak mentioned this issue Apr 23, 2024

less aggressive needless_borrows_for_generic_args #12706

Merged

bors closed this as completed in 79a14de May 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

RalfJung commented Mar 10, 2024 •

edited

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

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

Comments

RalfJung commented Mar 10, 2024 • edited

Description

Version

Additional Labels

RalfJung commented Mar 10, 2024 •

edited