Lint for unused return values of "pure" functions

tools and infrastructure
1

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” functions1 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?

1 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”.

4 Likes
2

Has been proposed before:

1 Like
3

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?

1 Like
4

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”.

2 Likes
5

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.

1 Like
6

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]!)
7

You might be interested in the discussion on this issue:

1 Like
8

That's why I included the caveat about the term "pure".

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.

9

Good point

10

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.

5 Likes
11

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.

12

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.

2 Likes
13

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

14

I’d certainly love to see such PRs.

15

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

1 Like
16

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.

17

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.

4 Likes
18

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.