Rust: Warning for unused arithmetic expressions

[jonhoo]

After having spent a day tracking down this bug, and recalling all the past times the same thing has bitten me, I'd like to propose a lint unused results of arithmetic expressions. Consider the following code:

let mut delta = 0isize;
match foo {
    Foo::Add(x) => {
        delta += x;
    }
    Foo::Sub(x) => {
        delta - x;
    }
}

This code currently compiles with no warnings (given a suitable enum Foo). However, bugs like these can be extremely hard to track down. They can often be hard to spot, especially when going through methods like saturating_sub which do not modify self, but instead return the new result (e.g., delta = delta.saturating_sub(x) is necessary).

It would be really neat to have a warning about unused arithmetic expressions (ideally one that also handles saturating_sub). I don't think there are generally cases where not using the result of an arithmetic expression is what the developer intended to do. This also complements the unused_variables and unused_mut lints quite nicely.

[varkor]

We should just be able to add #[must_use] to the operator traits. The comparison operators already have them. See https://github.com/rust-lang/rust/issues/48926 for a discussion of which library functions should be annotated.

[zackmdavis]

just be able to add #[must_use] to the operator traits

As an implementation detail, the operators are hardcoded in the lint (while the corresponding trait methods also take the attribute for when they are called as methods).

[abonander]

It would be trivial to add #[must_use] to .saturating_*(), .checked_*() and .overflowing_*() though.

As an implementation detail, the operators are hardcoded in the lint

Why are we matching on the exact binary operations there anyway? All of them should be triggering the lint. All the op-eq operators are encoded differently, right, so there shouldn't be any false-positives.

This seems like a good beginner bug actually. I have more experience with macro expansion than HIR but I'm willing to mentor if no one else wants to. @kennytm

[varkor]

Why are we matching on the exact binary operations there anyway? All of them should be triggering the lint.

From https://github.com/rust-lang/rust/issues/48926, I think the idea was to be conservative to begin with, but it seems very reasonable that non-assigning operators should be #[must_use].

[jonhoo]

@abonander I'm actually not terribly worried about .checked_*(), since those return Options which I think it's more unlikely that you forget to operate on. Arguably the most important ones are Add::add, Sub::sub, etc.

[abonander]

@jonhoo It's mostly for consistency and Option isn't marked #[must_use] itself so it's conceivable that the user might forget to unwrap it.

But as I mentioned, the lint is actually excluding the arithmetic operators right now; it would be less complex if it linted on all binary operators unconditionally (excluding op-assign which is encoded in the AST/HIR as a separate expression kind anyway).

[abonander]

Mentoring Instructions

This one should be pretty straightforward. Right now the only thing being checked by the unused_must_use lint are the comparison operators. So if you just have a random 1 < 2; somewhere in your code it'll be linted, but a 1 + 2 won't be: https://play.rust-lang.org/?gist=5570a5ad2813967016c43206b0bc32b7&version=nightly

This is caused by this pattern match that only emits the lint error for the comparison operators. We should keep the match but use it to select the lint message.

Additionally, #[must_use] should be added to these methods and to the macros for each ops impl here (ignoring the [op]Assign ones).

When that's done I believe lint warnings are tested with UI tests.

[aaronaaeng]

Hey @abonander,

I've started working on this issue but I just want to clarify a few things. When you say to add #[must_use] to those methods, is that as simple as adding the attribute to each method? For whatever reason, that seems too easy. My understanding of that attribute may be incorrect but that seems as though it will require the use of the operator, not the use of the calculated value.

Thanks!

Edit:
From messing around with a test build, getting the pattern match on the unused arithmetic operation was easy enough. However, I noticed that note: #[warn(unused_must_use)] on by default does not appear for any of the warnings I added. This leads me to believe that I used those attributes incorrectly.

[zackmdavis]

note: #[warn(unused_must_use)] on by default does not appear for any of the warnings I added.

That note intentionally only appears on the first warning for a given lint.

[aaronaaeng]

Oh, in that case I'll just make sure this passes the tests and open a pull request so you all can check it out.