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

[zackmdavis]

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

This has been implemented for a while under a fn_must_use feature gate (tracking issue), and PR #48925 proposes stabilization.

#[must_use] has been added to the comparison methods in the standard library (.eq, .lt, _&c._), but we never got a consensus as to what other standard library methods, if any, should get it. _In principle_, any function or with no side effects could be must-use (_e.g._, .len() on collections), but adding dozens or hundreds of annotations to the standard library feels kind of dramatic: perhaps must-use should instead be reserved for functions with "unusually result-like" semantics—after all, users who want to be _really_ sure they're not uselessly throwing away a return value can always opt in to the (allow-by-default) unused-results lint.

If we wanted to be very conservative, we could refuse to stabilize until we've made a decision on this: if we were to stabilize first, any new #[must_use] annotations in the standard library would be insta-stable. But, maybe this isn't a big deal (cargo passes cap-lints allow to dependencies, so tinkering with lints shouldn't break people's builds).

[scottmcm]

I'm not sure "result-like" is broad enough. Maybe something like "things that may have side effects, but which ought never be used for their side effects, especially if often expensive". As a canonical example of such a thing, Iterator::collect. Perhaps more controversially, Clone::clone.

I definitely don't think this needs to block stabilization, because any deny(warnings) impact will be the same whether with stabilization or after it.

[scottmcm]

As of https://github.com/rust-lang/rust/pull/49533, this is now on Iterator::collect, Clone::clone, and ToOwned::to_owned.

[jonhoo]

I think the non-assigning arithmetic functions are good candidates for this (see #50124).

[shepmaster]

Another metric could be: "things that (are highly likely to) allocate".

[shepmaster]

There's a concrete question around String::from_raw_parts, if people are interested in chiming in. My position is that such a function is mostly used for the purpose of converting back from FFI to be dropped, and shouldn't use the attribute.

[scottmcm]

A thought: I agree we don't want to clutter code with #[must_use] everywhere. But would we be open to the warnings if they came free? Is there some cheap conservative heuristic that would give a bunch of the warnings without the annotation noise? Perhaps "any &self method on a : Freeze type"?

[shepmaster]

Perhaps "any &self method

I was starting to think down these lines:

Any function which:

• returns non-() (and maybe some others, like !?)
• has no &mut arguments of any kind (including and beyond self)

And then we opt-out of things with internal mutability?

We may still end up with a lot of attributes if we follow @Manishearth's suggestion to have explanatory text on every function. (Not sure if I agree with that or not).

[est31]

In principle, any function or with no side effects could be must-use (e.g., .len() on collections), but adding dozens or hundreds of annotations to the standard library feels kind of dramatic

The Rust language already has a concept of side-effect freedom which is const fn. So what about adding a warn-by-default lint to the compiler that complains about any unused result of a const fn invocation? This would remove the need to add #[must_use] everywhere, and wouldn't water it down.

MUST is a very strong word. When I see #[must_use], I think that if the result isn't used, this is most likely a dangerous bug. Using the attribute to lint for dead code seems wrong to me because it would water down the strong meaning of MUST. Of course, dead code might hide a bug as well so this distinction is a bit muddy.

[jonhoo]

Hmm, I'm not sure that warning on unused const fn results is sufficient. I could be mistaken, but isn't it currently impossible to write a const fn that, say, does allocation (e.g., one that returns HashMap::new())?

[sfackler]

HashMap::new doesn't allocate.

[jonhoo]

Sorry, you're right. I guess String::from would be a better example.

[est31]

I've made an implementation of my lint idea here: #50805

@jonhoo allocating functions can be const fn. String::from is not const fn for a different reason: it needs a const impl Trait for Struct like feature as e.g. proposed in this RFC. But there is no good reason why the language can't be extended to make String::from const as well.

Edit: fixed link

[est31]

I've opened an RFC now as it seems that it requires one: https://github.com/rust-lang/rfcs/pull/2450

[jonhoo]

As discussed in #52201, I think another set of candidates for this is methods like compare_and_swap on atomics; methods that do not return Result, but nonetheless need to have their return value checked to determine the outcome of the operation.

[LukasKalbertodt]

Just to give another example: I just noticed a bug in code that I wrote which could have been caught early with a proper #[must_use] lint. I assumed that a Vec has at leastone element and I wanted to remove the last element. So I wrote this:

vec.pop();

I didn't think about what would happen if the vec is empty. This empty case is a special case I need to cover elsewhere in my code. If the statement above would have caused a warning, I would have thought about it. So I guess the Option<T> returned by pop() is kind of "result like"?

I think marking pop with #[must_use] is a good idea. I think it's extremely rare that the programmer doesn't care about the returned value and also doesn't care about whether or not the method actually modified the vector.

[shepmaster]

I recently watched a newcomer to Rust write this shape of code:

let mut s = String::new();
stdin.read_line(&mut s);
s.trim();
if s == "hello" { /* ... */ }

They either believed that trim operates in-place or simply forgot to use the result, but a must_use on that would have helped them.

[rkjnsn]

I think marking pop with #[must_use] is a good idea. I think it's extremely rare that the programmer doesn't care about the returned value and also doesn't care about whether or not the method actually modified the vector.

I think I would lean more toward #[must_use] being reserved for situations where ignoring the return value is almost certainly a programmer error. It does not seem unreasonable that code might inspect the last element and, if a certain condition is met, discard it using pop. (Indeed, I have written similar code in other languages.)

In contrast, something like calling trim without checking the return value is surely an error, as there is no possible use for doing so.

[cramertj]

cc @rust-lang/libs

[BatmanAoD]

In an internals post, I suggested an alternate heuristic:

For functions where all of the arguments are by-shared-reference or by-copy, and a value is returned...

CAD97 pointed out that shared-reference is Copy, so "all parameters are Copy" is sufficient.

By "a value is returned", I of course meant something other than () or !.

My opinion is that we should err on the side of more "must-use warnings", because they're trivial to explicitly silence with let _ =. (Though that's a stronger argument when adding such warnings to Clippy rather than rustc.)

[shepmaster]

@alex said:

I just ran into a project where Result::is_ok was accidentally being called without using the result -- I believe it had been confused with an assertion method like expect(). Had is_ok been marked #[must_use] this mistake would have been prevented.

[varkor]

Until we have umbrella guidelines for when a function should be marked #[must_use], I propose the following heuristic.

If a function is entirely effectless and there is evidence that a user has misinterpreted it as being effectful, leading to an incorrect program (e.g. if a user opens an issue specifically, such as in https://github.com/rust-lang/rust/issues/59610 or https://github.com/rust-lang/rust/issues/59787), then it is sensible to mark the function as #[must_use].

This still errs on the side of being conservative, but responsively addresses issues that have arisen in practice surrounded possibly ambiguous function names. (See also https://github.com/rust-lang/rust/issues/59610#issuecomment-478899738.)

That is, we shouldn't be blocking adding #[must_use] to functions simply because we don't have overarching guidelines. (I imagine this isn't a controversial viewpoint, but it should be made explicitly somewhere.)

[Jasperav]

Coming from a Swift background it really bothers me that I don't get warnings when not using a functions return value by default.

Copied from my comments at https://stackoverflow.com/a/50437107/7715250:

Do you know any reason why this isn't the default behavior for functions to be annotated with the must_use annotation? Swift's behavior is reversed: we must explicitly annotate the function with @discardableResult when not wanting to do anything with the return type of the function (else: warning). This looks more error-free than Rust's approach. When creating a function with a return type, I think it is likely that the creator of the function wants the caller of the function to do something with the results. Adding the must_use annotation everything is a lot of noise and pain to maintain.

and

When nothing is returned, nothing is expected. When returning a value and it isn't assigned to anything, I think it's weird that Rust things: 'O, nothing to see' and a annotation must be added to produce any sort of warning. I can not imagine any case in which the Rust way of dealing with not-assigned values is more error-free than Swift's style (which is: always a warning when not assigning a value when calling a function but you can explicitly opt out by adding @discardableResult (this is pretty rare from what I know))

When I write a function in in my main languages (Swift/Java) and I add a return type, it is really 99% likely I want to caller to DO something with the result. I really dislike the Rust way, no warnings when not using the return type by default on all types. There are I think a few functions in which it is not needed to do anything with the return type, they are annotated with @discardableResult in swift (e.g. https://developer.apple.com/documentation/swift/array/1641390-remove). These kind of methods, which return something and the caller does not have to assign the variable to anything to not get a warning, are much more fewer compared to return values which expect you to do something with the value.

IMO: when writing a function with a return value, it is so much more likely it is expected to do something with the variable instead of not. So that's why I think the #[must_use] should be on by default.

[BatmanAoD]

@Jasperav That might be too big a change to make except at an edition boundary, but Rust 2021 is right around the corner 😃

[BatmanAoD]

It would also be appropriate to create an off-by-default clippy lint implementing that behavior, I think.

[scottmcm]

No need for a clippy lint; it already exists in rustc: https://doc.rust-lang.org/rustc/lints/listing/allowed-by-default.html#unused-results

[cuviper]

FYI, I've added a must_use on mem::replace in #71256. It doesn't quite meet the rough guidelines here about being _useless_ otherwise, but I made the suggestion to use direct assignment if you don't need that return value. This idea came from noticing such unused values in some of my own code.

[scottmcm]

I like that change. I'd support a guideline for that kind of thing, perhaps

Add #[must_use] if there's a simpler construct that you can suggest in the message when the result is unused

That also covers things like the attribute that was added on split_off to suggest truncate.

[m-ou-se]

Is there any progress towards clearer guidelines for #[must_use]? #78225 adds #[must_use] for Iterator::count and Iterator::last, which seem to fall outside of the current rough guidelines. It would be nice to have some documentation to link and check PRs like that against.