Lint for unused return values of "pure" functions

BatmanAoD · 2019-02-13T19:47:30.448Z

I was recently caught by surprise when I tried to use f64::round() to mutate the receiver:

value.round();

…of course, what I should have written was:

value = value.round();

This was admittedly a “dumb” mistake, but I was surprised that round() isn’t marked #[must_use], so I didn’t get a warning.

Even so, this seems like something Clippy could catch, but currently, it doesn’t. For functions where all of the arguments are by-shared-reference or by-copy, and a value is returned, it seems likely to be a bug if the value is ignored.

Does treating such “pure” functions¹ as automatically #[must_use] seem like a good idea for a lint? If so, should I just go ahead and try to implement it in Clippy and submit it as a pull request?

¹ I realize that Rust does not have a formal definition of “pure” functions, and that there’s not really a standard definition that could be easily applied. My suggestion for this lint is to ignore I/O and simply use mutability to determine whether a function is “pure”.

felix.s · 2019-02-13T19:58:08.152Z

Has been proposed before:

github.com/rust-lang/rfcs

RFC for unused const fn results

by est31 on 03:51PM - 29 May 18 UTC

1 commits changed 1 files with 173 additions and 0 deletions.

BatmanAoD · 2019-02-13T20:01:44.275Z

Looks like work was abandoned because there was no one to spearhead it, and the proposal ran into some issues by assuming that const fn is necessarily “pure”. My proposal overlaps with that one, but isn’t identical (and explicitly wouldn’t run into the same issue with Drop, since I specified that it would only apply to functions where the arguments are either & or Copy).

Should I just open a new RFC?

CAD97 · 2019-02-13T20:42:39.117Z

Note that &_ is Copy as well.

I’m pretty sure that clippy would accept a pedantic lint for “immediately discarded return value from function taking only Copy arguments”.

drXor · 2019-02-13T21:01:00.896Z

This is not strong enough to say “this is a pure function”; consider a function that takes no arguments and locks a global, not-RAII mutex in a system library.

dhm · 2019-02-13T21:09:07.554Z

Excluding the (), this sounds like a very decent lint, but some “pure” false positives remain:

aliasing does not always imply immutability. Take, for instance, ::std::sync::atomic::AtomicUsize::fetch_add ;
some form of unfallible global state, like a global mutable var (using unsafe or inner mutability), or printing to stdout through its panicking version (e.g. print[ln]!)

scottmcm · 2019-02-13T21:09:12.853Z

You might be interested in the discussion on this issue:

github.com/rust-lang/rust

Issue: discussion/tracking issue for `#[must_use]` functions in the standard library

opened by zackmdavis on 2018-03-11

RFC 1940 authorized use of the #[must_use] attribute on functions (with the result that invocations of the function behave as if...

C-tracking-issue T-libs

BatmanAoD · 2019-02-13T21:59:57.604Z

drXor:

This is not strong enough to say “this is a pure function”…

That’s why I included the caveat about the term “pure”.

dhm:

…some “pure” false positives remain

I understand; but return values are easy enough to explicitly ignore (let _ =) that I personally don’t really think false positives are a problem. Of course, that’s purely a matter of opinion.

dhm · 2019-02-13T22:33:01.511Z

BatmanAoD:

return values are easy enough to explicitly ignore ( let _ = )

Good point

withoutboats · 2019-02-14T16:04:41.882Z

It seems much more achievable to tag some of these specific functions in std that are liable to be mis-used as methods as #[must_use] now that we have that feature on functions.

BatmanAoD · 2019-02-14T20:28:31.725Z

You’re probably already aware, but that’s the main topic in the GitHub issue thread above. One complaint about that is that the annotations are noisy, which is true.

withoutboats · 2019-02-14T21:03:50.095Z

I think the standard library is doomed to noisy attributes by its place in the ecosystem (we’ve already committed to a stable/unstable attribute on every single item). I don’t think we should put a must_use on every “pure” function; I think we should put them on ones that seem plausible to mistake as mutating methods (like round). And that’s something we can do now, whereas these kinds of abstractions run into problems around fuzziness as we’ve seen before we even get into bothering to implement them.

BatmanAoD · 2019-02-14T22:02:52.405Z

Fair enough. So would the appropriate action be to just start opening PRs that add such annotations?

josh · 2019-02-14T23:47:01.344Z

I’d certainly love to see such PRs.

scottmcm · 2019-02-15T00:01:23.635Z

That’s how I got some added I’d suggest picking a theme to justify why those particular ones.

github.com/rust-lang/rust

Add #[must_use] to a few standard library methods

by scottmcm on 06:09AM - 31 Mar 18 UTC

1 commits changed 4 files with 4 additions and 1 deletions.

Finn · 2019-02-15T12:22:39.506Z

It might have been better if #[must_use] would be the default for other types than (). Now I wonder how often people intentionally allow return values to be implicitly discarded.

Nemo157 · 2019-02-15T12:27:02.561Z

If you want to see how annoying this is go to a crate and run RUSTFLAGS='-W unused-results' cargo build, I have tried having this enabled on projects in the past and found it to be too noisy to be worth doing.