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

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

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.

Rollup merge of rust-lang#49533 - scottmcm:more-must-use, r=nikomatsakis

As of #49533, this is now on Iterator::collect, Clone::clone, and ToOwned::to_owned.

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

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

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.

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"?

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

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.

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())?

HashMap::new doesn't allocate.

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.

I propose the following guideline:

Add #[must_use] to a function when, for any perceivable usage, not using the return value is a logic error or sub-optimal usage.

I disagree with the following:

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.

"must" does not indicate the severity of the bug; It indicates the level of certainty that not using the value is wrong or sub-optimal.

I believe with this guideline, Iterator::count and Iterator::last can clearly be included.

After thinking some more, I think I can do better. I see three distinct reasons for adding #[must_use] (and all valid reasons IMHO). So here is another attempt at a comprehensive guideline:

Add #[must_use] if at least one of the following conditions is true:

The function has no effect other than producing a return value (a pure function).

There exists a better (more idiomatic, performant, etc.) way to do the same thing if the return value is not used.

The return value can indicate an exceptional outcome.

To say more about those 3 conditions:

Can be determined just by looking at the function itself. Unfortunately this is very common. But at the same time it should not cause any false positives. Examples: checked_add, Vec::new, str::trim, Iterator::map (includes builder-like patterns).
Can be determined by considering other API or language features. Thus it is more nuanced, but with the right justification, it should also be a rather confident decision. Examples: mem::replace, String::split_off, Iterator::last, Iterator::collect.
Can be determined by asking, "Does this function sometimes not do what it normally does?". I believe this is inescapably subjective: you need to define "normal" and "exceptional". For example, we probably don't want to include Vec::pop because None is "not very exceptional". But this is arguably the most important category for helping people avoid logic errors. By adding #[must_use], you are declaring, "If you're going to ignore the return value, you should do so explicitly." (how to idiomatically ignore a #[must_use] value is separate discussion). The good news is, for most instances, this is applied by simply returning Result. However, this category could also include mem::compare_and_swap.

Looking at the 3 conditions from another perspective:

Detects noop or possibly logic errors if the function was expected to modify input.
Detects non-idiomatic or sub-optimal usage.
Detects logic errors.

Condition #2 seems to stand out as "not as severe". But personally I don't think this is something to get hung up on - any unused_must_use warning could easily point to a huge logic error.

Bonus condition #4: The return value is literally designed to determine control flow (looking at you, ops::ControlFlow). Seems like a very strong case for #[must_use], but for a very unique reason that may never be repeated 😆.

It might be nice to have a special annotation for #1 like #[must_use(pure)] to avoid writing a bunch of similar error messages.

@m-ou-se

@m-ou-se No, I don't know of any. It really needs someone to pick it up and find a "hits the majority" subset so that must_use doesn't just become a "put it on everything" annotation.

Unfortunately this is very common

It might be nice to have a special annotation for #‍1 like #[must_use(pure)] to avoid writing a bunch of similar error messages.

Yeah, exactly. But because it's so common -- and the downside to not using something is often so minor -- I think this is a place for a lint to heuristically do it.

We know that unused_results is too strong, but it seems very plausible to start making lints to cut down on the frequency of needing extra annotations about this stuff.

For example, we could start with a lint for any const fns that are only passed &impl Freeze or impl Copy + 'static arguments. (Idea reference above.) Basically piggy-back on existing annotation work that's already happening, but also as a separate lint so people could decide they don't care if there's a few unnecessary calls to u32::wrapping_add, say, since they're very reliably get optimized out, even if they do want to know about the more important must_use warnings.

We could also consider an #[important_side_effect] annotation that would let the heuristics be more aggressive, though logistically transitioning to that might be awkward.

a lint to heuristically do it

Looking at this from the other direction, manually annotating a bunch of functions that fit into these categories (whatever they may be) could be a way to gauge the efficacy of any proposed heuristic. Effectively, this would be like how lifetime elision was established based on feedback from real world usages.

@camsteffen

@camsteffen just brought this discussion to my attention.
cc @joshtriplett

Recently I independently asked around on Zulip and IRLO about adding #[must_use] throughout the standard library. Several Rust team members and many community members at large gave the green light.

With that go ahead, I put together a list of around 800 changes and have been working them off bit by bit. I'm about halfway done at this point. You can see the tracking issue here:

Tracking issue for adding #[must_use] annotations throughout the standard library #89692

I'd appreciate everyone's feedback as I continue working on this.

It looks like your work is focused on the "pure function" case, which is the most straightforward.

Done! The MCP was accepted and the tracking issue is closed. std is now chock full of #[must_use]s.

The MCP adds clear guidelines for some cases that should have #[must_use], but I don't think it is a complete guideline. For example it would not include String::split_off. Namely cases 2 and 3 from my previous post are still open-ended. And some PRs were blocked on this discussion. What would happen to those PRs today if this issue is closed?

I concur. The guidelines in the MCP were for what I was using to scope my changes. I did not intend them to be proscriptive. There are certainly more cases that should be added.

I fudged my own rules a bit, even. To wit, I added a number of functions that take and return &mut when it was clear the mut is a passthrough and the function doesn't itself modify anything. For example:

HashMap::key_mut(&mut self) -> &mut K;
HashMap::get_mut(&mut self) -> &mut V;
HashMap::get_key_value(&mut self) -> (&K, &V);
HashMap::get_key_value_mut(&mut self) -> (&mut K, &mut V);

Navigation Menu

Repository navigation

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

22 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

discussion/tracking issue for #[must_use] functions in the standard library #48926

Description

Activity

scottmcm commented on Mar 14, 2018

scottmcm commented on Apr 5, 2018

jonhoo commented on Apr 20, 2018

shepmaster commented on May 6, 2018

shepmaster commented on May 7, 2018

scottmcm commented on May 7, 2018

shepmaster commented on May 7, 2018

est31 commented on May 15, 2018

jonhoo commented on May 15, 2018

sfackler commented on May 15, 2018

22 remaining items

m-ou-se commented on Oct 22, 2020

camsteffen commented on Jan 27, 2021

camsteffen commented on Jan 28, 2021

scottmcm commented on Jan 29, 2021

shepmaster commented on Feb 1, 2021

jkugelman commented on Oct 11, 2021

camsteffen commented on Oct 11, 2021

jkugelman commented on Nov 1, 2021

camsteffen commented on Nov 2, 2021

jkugelman commented on Nov 2, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

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