Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
zackmdavis opened this Issue Mar 11, 2018 · 19 comments

Comments

Projects
None yet
@zackmdavis
Copy link
Member

zackmdavis commented Mar 11, 2018

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

This comment has been minimized.

Copy link
Member

scottmcm commented Mar 14, 2018

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.

kennytm added a commit to kennytm/rust that referenced this issue Apr 3, 2018

Rollup merge of rust-lang#49533 - scottmcm:more-must-use, r=nikomatsakis
Add #[must_use] to a few standard library methods

Chosen to start a precedent of using it on ones that are potentially-expensive and where using it for side effects is particularly discouraged.

Discuss :)

```rust
warning: unused return value of `std::iter::Iterator::collect` which must be used: if you really need to exhaust the iterator, consider `.for_each(drop)` instead
  --> $DIR/fn_must_use_stdlib.rs:19:5
   |
LL |     "1 2 3".split_whitespace().collect::<Vec<_>>();
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: unused return value of `std::borrow::ToOwned::to_owned` which must be used: cloning is often expensive and is not expected to have side effects
  --> $DIR/fn_must_use_stdlib.rs:21:5
   |
LL |     "hello".to_owned();
   |     ^^^^^^^^^^^^^^^^^^^

warning: unused return value of `std::clone::Clone::clone` which must be used: cloning is often expensive and is not expected to have side effects
  --> $DIR/fn_must_use_stdlib.rs:23:5
   |
LL |     String::from("world").clone();
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
```

cc rust-lang#48926

kennytm added a commit to kennytm/rust that referenced this issue Apr 3, 2018

Rollup merge of rust-lang#49533 - scottmcm:more-must-use, r=nikomatsakis
Add #[must_use] to a few standard library methods

Chosen to start a precedent of using it on ones that are potentially-expensive and where using it for side effects is particularly discouraged.

Discuss :)

```rust
warning: unused return value of `std::iter::Iterator::collect` which must be used: if you really need to exhaust the iterator, consider `.for_each(drop)` instead
  --> $DIR/fn_must_use_stdlib.rs:19:5
   |
LL |     "1 2 3".split_whitespace().collect::<Vec<_>>();
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: unused return value of `std::borrow::ToOwned::to_owned` which must be used: cloning is often expensive and is not expected to have side effects
  --> $DIR/fn_must_use_stdlib.rs:21:5
   |
LL |     "hello".to_owned();
   |     ^^^^^^^^^^^^^^^^^^^

warning: unused return value of `std::clone::Clone::clone` which must be used: cloning is often expensive and is not expected to have side effects
  --> $DIR/fn_must_use_stdlib.rs:23:5
   |
LL |     String::from("world").clone();
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
```

cc rust-lang#48926

kennytm added a commit to kennytm/rust that referenced this issue Apr 4, 2018

Rollup merge of rust-lang#49533 - scottmcm:more-must-use, r=nikomatsakis
Add #[must_use] to a few standard library methods

Chosen to start a precedent of using it on ones that are potentially-expensive and where using it for side effects is particularly discouraged.

Discuss :)

```rust
warning: unused return value of `std::iter::Iterator::collect` which must be used: if you really need to exhaust the iterator, consider `.for_each(drop)` instead
  --> $DIR/fn_must_use_stdlib.rs:19:5
   |
LL |     "1 2 3".split_whitespace().collect::<Vec<_>>();
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: unused return value of `std::borrow::ToOwned::to_owned` which must be used: cloning is often expensive and is not expected to have side effects
  --> $DIR/fn_must_use_stdlib.rs:21:5
   |
LL |     "hello".to_owned();
   |     ^^^^^^^^^^^^^^^^^^^

warning: unused return value of `std::clone::Clone::clone` which must be used: cloning is often expensive and is not expected to have side effects
  --> $DIR/fn_must_use_stdlib.rs:23:5
   |
LL |     String::from("world").clone();
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
```

cc rust-lang#48926
@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Apr 5, 2018

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

@jonhoo

This comment has been minimized.

Copy link
Contributor

jonhoo commented Apr 20, 2018

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

@shepmaster

This comment has been minimized.

Copy link
Member

shepmaster commented May 6, 2018

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

@shepmaster

This comment has been minimized.

Copy link
Member

shepmaster commented May 7, 2018

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

This comment has been minimized.

Copy link
Member

scottmcm commented May 7, 2018

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

This comment has been minimized.

Copy link
Member

shepmaster commented May 7, 2018

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

This comment has been minimized.

Copy link
Contributor

est31 commented May 15, 2018

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

This comment has been minimized.

Copy link
Contributor

jonhoo commented May 15, 2018

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

This comment has been minimized.

Copy link
Member

sfackler commented May 15, 2018

HashMap::new doesn't allocate.

@jonhoo

This comment has been minimized.

Copy link
Contributor

jonhoo commented May 15, 2018

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

@est31

This comment has been minimized.

Copy link
Contributor

est31 commented May 16, 2018

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

This comment has been minimized.

Copy link
Contributor

est31 commented May 29, 2018

I've opened an RFC now as it seems that it requires one: rust-lang/rfcs#2450

@jonhoo

This comment has been minimized.

Copy link
Contributor

jonhoo commented Jul 11, 2018

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

This comment has been minimized.

Copy link
Contributor

LukasKalbertodt commented Oct 23, 2018

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

This comment has been minimized.

Copy link
Member

shepmaster commented Oct 23, 2018

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

This comment has been minimized.

Copy link
Contributor

rkjnsn commented Nov 27, 2018

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

This comment has been minimized.

Copy link
Member

cramertj commented Dec 26, 2018

@BatmanAoD

This comment has been minimized.

Copy link

BatmanAoD commented Feb 13, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.