Is needless_pass_by_value behaving properly?

[gnzlbg]

This (playground):

pub enum Algorithm {
    Scalar,
    Simd,
}
pub trait O {}
fn foo<O>(_o: O) {}
fn bar<O>(_o: O) {}

pub fn run<T: O>(o: T, alg: Algorithm) {
    match alg {
        Algorithm::Scalar => foo(o),
        Algorithm::Simd => bar(o),
    }
}

fn main() {}

errors with

warning: this argument is passed by value, but not consumed in the function body
 --> src/main.rs:6:15
  |
6 | fn foo<O>(_o: O) {}
  |               ^ help: consider taking a reference instead: `&O`
  |
  = note: #[warn(needless_pass_by_value)] on by default
  = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#needless_pass_by_value

warning: this argument is passed by value, but not consumed in the function body
 --> src/main.rs:7:15
  |
7 | fn bar<O>(_o: O) {}
  |               ^ help: consider taking a reference instead: `&O`
  |
  = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#needless_pass_by_value

warning: this argument is passed by value, but not consumed in the function body
 --> src/main.rs:9:29
  |
9 | pub fn run<T: O>(o: T, alg: Algorithm) {
  |                             ^^^^^^^^^
  |
help: consider marking this type as Copy
 --> src/main.rs:1:1
  |
1 | / pub enum Algorithm {
2 | |     Scalar,
3 | |     Simd,
4 | | }
  | |_^
  = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#needless_pass_by_value
help: consider taking a reference instead
  |
9 | pub fn run<T: O>(o: T, alg: &Algorithm) {
10|     match *alg {
  |

but it appears to me that both errors are incorrect? o is consumed in the body, in the calls to foo and bar, and alg is consumed by the match ? Or what am I missing?

Or is the lint telling me that the intent of foo, bar, and run might not be to drop o and alg ? None of these types is Copy, so from the API signature, that's pretty much what the intent appears to be to me.

Making this change requires you to also change all callers of these functions to pass a reference &, or to make the types implement Copy which would break the use case where the intent is to ensure that they are used once, so these are not very clear cut suggestions either.

So what's the intent of this lint ? It doesn't appear to be to catch any bugs, because all possible issues would be compiler errors, and the code has to compile before clippy can be called (typically).

So maybe it is to improve APIs, but then this lint should only apply to pub exported items or something, and even then, I am skeptical.

[sinkuu]

Note that warnings for o is mentioning not at all run, but foo and bar, whose bodies actually does not consume o.

This match expression is not consuming alg:

match alg {
    Algorithm::Scalar => foo(o),
    Algorithm::Simd => bar(o),
}
let x = alg; // you can still use `alg`

Try adding non-Copy data field to the enum and using a consuming pattern (neither ref nor the "_") for it (playground). This lets match consume values and should correctly silent the lint.

[gnzlbg]

That might be a philosophical question, but this is how drop can be implemented:

fn drop<T>(_: T) {}

Does drop consume its argument? I'd say so: the whole raison d'être of drop is to consume its argument.

It does not use it, but that's why it uses an underscore for the argument name. Changing drop to take a & or make T: Copy breaks the objective of the function.

[sinkuu]

Please just add #[allow(needless_pass_by_value)] if you are on purpose. This is a lint for detecting functions that are accidentally consuming values, which could be troublesome for callers (requiring .clone()s for example). Imagine a function taking a String even though it can be changed to take a &str.

[gnzlbg]

@sinkuu that's what I meant with "this lint is not trying to catch ''bugs'' but to make APIs better" but if that's its objective, then it might make sense to make it apply only to public items at least by default (and make the private item version a restriction lint).

If this happens in code that you control, and you need to call clone, you... can just change it. But if this is provided by some external crate that you cannot modify, then this is very annoying and arguably the API of that crate is broken.

[sinkuu]

Yeah, that's a nice idea. Sorry for out-of-focus replay, OP I read was at the edition ended with "... Or what am I missing?`" and haven't scroll up since then...

[gnzlbg]

Don't worries, I was out-of-focus as well. I think the actionable things we could do here are:

• document the lint a bit better, maybe using your string.clone() vs &string example to show which problem this could cause for users on APIs - make it clear that this lint improves APIs, but that this is not a "bug" in the code.

• think whether this lint should be enabled by default for all items, or only public items

• if we choose to enable this only for public items by default, think how to allow people to opt-in to enabling this for private items as well since that is a reasonable thing that many people will want to do. Maybe just keep this lint "as is" but make it a restriction, and add a new one needles_pass_by_val_pub that is enabled by default instead?

No idea of what would be best here, and no idea yet of whether splitting this for pub / private items is worth doing. I'm unconvinced.

[TrolledWoods]

I had a similar problem however slightly different. I have some api types that are just wrappers around a reference, so they are both Copy and 8 bytes. Which means that taking a reference has no other effect than just adding another layer of indirection. This can of course be fixed by adding #[allow(clippy::needless_pass_by_value)] above the functions affected, but that would mean that these functions would not have the benefit of other arguments being checked by the lint.

Could it be possible to make the lint only apply to non-copy types? Or split it into several lints, one for non copy types, and maybe another one for arguments which would be smaller if you took them by reference instead of copying them?

(Turns out the type wasn't Copy, nevermind)

[camsteffen]

The lint documentation says "Taking arguments by reference is more flexible...". I think this is sufficiently clear and Clippy is right to lint the case in the OP.