must_use_candidate is going mad (false positives?)

[tyranron]

With the latest Rust beta release:

❯ cargo +beta clippy -- --version
clippy 0.0.212 (c8e3cfbd 2019-10-28)

must_use_candidate lint complains about every single function in my code.

Is it an intended behavior or just a false positives bug? It has worked OK on previous version.

[flip1995]

must_use_candidate is a new lint #4560. And as you can see, it also caused pretty much fallout in the Clippy codebase. This is why it is a pedantic lint. Do you have an example, where you think that the lint should not trigger?

[tyranron]

@flip1995 yup, I understand that. However, I live with -D clippy::pedantic -D warnings in all my projects almost a year with a half already, and it hadn't been painful like that. Actually, just a few allow attrs here and there resolved any issues with Clippy's false positive or undesired pedantism.

But this new must_use_candidate lint feels like not just pedantic, but overly pedantic 🙃

It triggers almost any From implementation (and other conversions):

error: this method could have a `#[must_use]` attribute
  --> services/chat/src/storage/mod.rs:32:5
   |
32 |     fn from(db: cockroachdb::Db) -> Self {
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: add the attribute: `#[must_use] fn from(db: cockroachdb::Db) -> Self`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#must_use_candidate

even borrowing of trivial stuff:

error: this method could have a `#[must_use]` attribute
  --> services/chat/src/storage/mod.rs:39:5
   |
39 |     fn borrow(&self) -> &() {
   |     ^^^^^^^^^^^^^^^^^^^^^^^ help: add the attribute: `#[must_use] fn borrow(&self) -> &()`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#must_use_candidate

error: aborting due to 65 previous errors

Almost any getter methods:

error: this method could have a `#[must_use]` attribute
   --> crates/panicker/src/lib.rs:290:5
    |
290 |     pub fn column(&self) -> u32 {
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: add the attribute: `#[must_use] pub fn column(&self) -> u32`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#must_use_candidate

It seems like with such behavior, the #[must_use] attribute should be better declared on a From trait itself, rather than demanding it on every its implementation.

I don't know at which level Clippy operates and whether it has access to such information, but maybe we can skip trait implementations and issue warning for a trait itself?

[flip1995]

But this new must_use_candidate lint feels like not just pedantic, but overly pedantic

Yeah I agree with that. But we don't really have a category for that. restriction would be possible, which are overly pedantic lints, but also restrict the language (this lint doesn't really restrict the language though). But we can move it there. (Or maybe to nursery)

but maybe we can skip trait implementations and issue warning for a trait itself?

Yes this should be done.

cc @llogiq what do you think about this?

[ghost]

Does it makes sense to make the suggestion for functions? You might mistake a method call as mutating due to autorefs but with a function you typically have to pass the arguments as references so it's harder to make that mistake. As far as I know, in rustc they have only added #[must_use] to methods so far.

[jhpratt]

I also believe this is getting triggered by having an impl method that takes ownership. While this would normally be a good thing, having it not do so on Copy types would be nice. Right now, I'm disabling this on my entire codebase.

[gnzlbg]

This is why it is a pedantic lint. Do you have an example, where you think that the lint should not trigger?

It should probably not trigger on non-exported methods. I'm getting hundreds of #[must_use] errors on many private methods that get used once or twice. It is an anti-pattern to use #[must_use] on everything.

[llogiq]

Ok, I can add a visibility check.

[gnzlbg]

Maybe it would also make sense to keep the version that warns on every candidate function as a different lint, that must be opt-in. I’m not sure how useful that would be, but might be worth considering.

[MalteT]

Since it's not explicitly mentioned here yet: According to the documentation:

When used on a function in a trait implementation, the attribute does nothing.

So must_use_candidate should never be triggered for them.

[llogiq]

Do you have an example where the lint is triggered in a trait impl? I ask.because the UI test has an example where it isn't.

[MalteT]

@llogiq, I don't, I just saw that tyranron mentioned this:

It triggers almost any From implementation (and other conversions):

And there was no mention about this (or I missed it) in the conversation. Happy to hear that it seems to work!

[tyranron]

@llogiq it seems that this issue can be closed now. I don't feel a big tension with this lint in latest releases. Thank you for taking care!