Holistic Reform of #[must_use] Needed

[dekellum]

Description

There are at least 13 cases found, where #[must_use] attributes are not applied where they should be. The lack of a unused_must_use warnings lead users to potentially hard to find bugs. For example, currently this compiles without warning and results in a race condition:

use tokio::prelude::*;
use tokio::fs::File;

#[tokio::main]
async fn main() -> io::Result<()> {
    let data = b"some bytes";

    let mut pos = 0;
    let mut buffer = File::create("foo.txt").await?;

    while pos < data.len() {
        let bytes_written = buffer.write(&data[pos..]).await?;
        pos += bytes_written;
    }

    buffer.flush(); // BUG, and without any must-use warning:
    // `AsyncWriteExt::flush` returns `Flush` which implements `Future`, and
    // should be awaited. Worse, this results in a race condition where
    // either:
    //
    // * 10 bytes are written to the file
    // *  0 bytes are written, with no error
    //
    // Adding the `await` ensures that all bytes are written.
    Ok(())
}

There are other cases where #[must_use] is redundantly applied, or with the included message being inconsistent. I propose to clean these all up across the tree in one pass, but I need agreement on the guidelines first, see below.

Background: I encountered uncertainty regarding the proper application of the #[must_use] attribute in #2129 (comment) and later attempted #2140, which made it clear that neither Tokio nor I had a consistent approach or clear guidelines. Since this will be a bunch of small changes everywhere, its better to arrive at a clear plan here first, followed by a hopefully short PR time.

Proposed Solution

Based on research including:

• Rust Reference: the must_use attribute

• Similar github issue: Add #[must_use] to Stream and Sink rust-lang/futures-rs#1415 Add #[must_use] to more types rust-lang/futures-rs#1600 BoxFuture / LocalBoxFuture should be marked as must_use  rust-lang/futures-rs#2017 improved error message for unused futures rustasync/runtime#32

Guidelines

As best I can tell from above research:

1. Apply #[must_use = "..."] to types that implement Future, Stream or Sink.

2. Do not apply on fn() -> impl Future<..> (or impl Stream, or impl Sink) or async fn of any return type, since rustc is able to provide must-use warnings on these already. Also do not apply to functions returning the types of (1).

3. But, we may apply on fn() -> Box<dyn Future>, pin or other wrapped types, as rustc is not currently able to provide must-use warning for these.

4. Apply consistently to priv or pub(crate) types, as well as pub.

One point of prior confusion around (2) for me, is that rustdoc does show must_use attributes, but unfortunately does not fully emulate the must_use handling of rustc. In other words, (not/)applying must_use to functions has an effect on the rustdoc, see example in #2140. However, in the scheme of things this is a minor issue with these guidelines that can be tolerated.

Recommended syntax

This is clearly subjective, but having looked at the current code base (see below) and compared with what others have done, I suggest the following unless there is a good reason to deviate:

#[must_use = "futures do nothing unless awaited or polled"]
#[must_use = "streams do nothing unless polled"]
#[must_use = "sinks do nothing unless polled"]
// This is _not_ must_use. Using the future is intentionally optional.

The last comment is intended for JoinHandle or any other cases like it that are found.

Note, per guidelines, these are almost always applied to types. I did not find a current example where we would want to apply it to a function.

See also: Attribute ordering, below.

I intend to edit the above guidelines/syntax based on further suggestions here.

In the current code base

I went hunting for missing cases via the following and checking for must_use on the nearby type:

git grep -n  -E "impl.*\s+(Future|Sink<.*>|Stream(<.*>)?|)\s+for\s+([A-Z][A-z]+)"

Based on the above rules, I temporarily added #[must_use = "missing"] or similar based on visibility of the types. I skipped cases where the type was only defined in tests. With this in place, searching and grouping occurrences:

git ls-files | grep -E '.rs$' | xargs cat | grep -o -E "#\\[must_use.*\\]" | sort | uniq -c
      1 #[must_use]
      3 #[must_use = "futures do nothing unless polled"]
     19 #[must_use = "futures do nothing unless you `.await` or poll them"]
     13 #[must_use = "missing"]
      4 #[must_use = "missing (crate)"]
      7 #[must_use = "missing (priv)"]
      1 #[must_use = "sinks do nothing unless polled"]
     21 #[must_use = "streams do nothing unless polled"]
      1 #[must_use = "streams do nothing unless you `.await` or poll them"]
      1 #[must_use = "yield_now does nothing unless polled/`await`-ed"]

Attribute ordering

A related syntax question arises because the line order of these attributes is not well controlled by rust fmt. Usually in the current tree, #[must_use] is placed after #[derive]. But we also have:

#[derive(Debug)]
#[must_use = "futures do nothing unless you `.await` or poll them"]
#[cfg_attr(docsrs, doc(cfg(feature = "io-util")))]
pub struct ReadToEnd<'a, R: ?Sized> {
    \\...
}

I suggest alphabetic order, which conveniently puts cfg(_attr) at the top (e.g. if not a configured item, then the remaining attributes are irrelevant):

#[cfg_attr(docsrs, doc(cfg(feature = "io-util")))]
#[derive(Debug)]
#[must_use = "futures do nothing unless you `.await` or poll them"]
pub struct ReadToEnd<'a, R: ?Sized> {
    \\...
}

And I would like to change those as part of updating must_use.

[dekellum]

Your busy of course, but I'm surprised this has gone without comment, @carllerche ? If you read it prevously, perhaps I originally missed the mark on describing the problem and motivating a solution? I've now added a concrete example in the top description section.

[LucioFranco]

This looks mostly good, I think @carllerche is out on vacation right now so I'd like to wait for him. We've just been quite busy :) Thanks for writing this up!

[dekellum]

Thanks for that @LucioFranco , a little update hint goes a long way. 👍

[dekellum]

No more mental bandwidth for this, but AFAIK the issue still remains.

[barafael]

This looks like it can be closed. On current tokio 1.34, the code above triggers a warning.