False positive for `let_underscore_future`

[agostbiro]

Summary

I just came across a case where I think the new let_underscore_future #9721 produces a false positive:

#[tokio::main]
async fn main() {
    let _ = tokio::spawn(async {
        println!("spawned")
    });
}

Playground

In this case the returned JoinHandle is a future, but it doesn't have to be awaited to make the provided future start execution.

Reproducer

I tried this code:

#[tokio::main]
async fn main() {
    let _ = tokio::spawn(async {
        println!("spawned")
    });
}

I expected to see this happen:

There should be no clippy warnings.

Instead, this happened:

warning: non-binding `let` on a future
 --> src/main.rs:3:5
  |
3 | /     let _ = tokio::spawn(async {
4 | |         println!("spawned")
5 | |     });
  | |_______^

Version

1.67.0-nightly

(2022-11-22 ff8c8dfbe66701531e3e)

Additional Labels

No response

[matthieu-m]

Ran into the same issue today, and found out that ignoring lints in a workspace is much more difficult than expected (unfortunately) when those lints are not "configurable" (work-around).

I do think the lint is valuable by default, but unfortunately it's a little too broad right now.

Maybe it should be made configurable with a hardcoded list of specific types that are allowed (such as tokio::task::JoinHandle), and a configuration option to add more of such types in clippy.toml?

[SquareMan]

Hi, I implemented this lint. At the moment it simply works by only checking the expression being assigned to let _ = ... implements Future.

Part of me definitely wonders if this really is a FP. Is there really a difference between assigning the returned JoinHandle to let _ here rather than not binding it at all? IIRC, both will drop the value immediately.

let _ = ... in my mind is mostly used for supressing #[must_use] warnings, and the original idea behind the lint is that since Futures (mostly) are only useful when polled/awaited/used then it is likely that the original intent was to supress a must_use warning attached to the result of the Future. I could easily see a similar scenario when the spawned task returns a Result and you meant to await the task and ignore the result.

If we decide that my previous scenario isn't really realistic/important, a possible solution here might be to also require that the Future is marked with must_use as well before emitting the lint. A user-provided configuration of allowed types might be a good addition as well.

@matthieu-m Are you able to share more about the scenario you encountered? Or is it the same situation that's being discussed here?

[SquareMan]

Another thing we could consider is to inspect the Future's Output type and emit the lint if either the Future or Output is must_use.

[agostbiro]

Thanks for the detailed reply!

Part of me definitely wonders if this really is a FP. Is there really a difference between assigning the returned JoinHandle to let _ here rather than not binding it at all? IIRC, both will drop the value immediately.

You're right about that. I initially assumed that the result was must use, but actually changing from this:

#[tokio::main]
async fn main() {
    let _ = tokio::spawn(async {
        println!("spawned")
    });
}

to this:

#[tokio::main]
async fn main() {
    tokio::spawn(async {
        println!("spawned")
    });
}

produces no warnings on rust version 1.67.0-nightly (70f8737b2 2022-11-23) and tokio = 1.22.0, so the lint in this case actually improves the code. 🎉

So as far I'm concerned, this is not a false positive, and the bug report can be rejected. @matthieu-m do you have an example where it's a false positive for you?

[matthieu-m]

I've used the pattern in case of TCP acceptors:

loop {
    tokio::select! {
        biased;

        _ = shutdown.recv() => break,

        socket = listener.recv() => {
            let socket = socket?;

            let _ = tokio::spawn(async move { ... });
        },
    }
}

The let _ is indeed not strictly necessary (JoinHandle isn't must_use), however I do find it serves a legitimate documentation goal, it signals to the reader that the return value of spawn is intentionally dropped, whereas with just spawn there's a doubt left of whether the original developer simply forgot, or not.

It can be handled differently: comments, calling mem::drop, ... let _ = is the idiomatic way of signalling "I don't care about the return value", though.

At the same time, I do recognize that JoinHandle is a bit of a special case. Most futures do need to be polled to progress. Maybe the answer would be to annotate JoinHandle itself with #[allow(clippy::let_underscore_future)]?

[SquareMan]

Someone please correct me if I'm wrong here but I don't think that adding an #[allow(...)] to a struct will cause any effect for instances of the struct, just the declaration itself. Certainly in this case it does not (playground).

Are there any examples of lints that do actually do this?

Edit: corrected playground link

[Stargateur]

possibly we could improve the lint to make this work. But I'm no clippy expert so I think we must wait more feedback from clippy folks on how to make such feature.

[matthieu-m]

@SquareMan The #[must_use] attribute can be placed on both functions and structs.

[SquareMan]

@matthieu-m sorry if I wasn't clear, please look at the playground example again now. I forgot to add the #[allow(clippy::let_underscore_future)] attribute. I was talking about your suggestion for annotating JoinHandle and showing that it doesn't actually affect anything.

[matthieu-m]

@SquareMan My turn to apologize for not being clear, then.

I definitely wasn't trying to suggest that it would just work. I was just saying that it could possibly be made to work.

I fully expect that it would require a code-change to Clippy, at best.

[SquareMan]

Understood. I do not know of any existing lints that act like that or if it's even possible, but I'm not all that knowledgeable here so a Clippy maintainer should probably chime in.

I'd like re-raise this idea for a solution though

Another thing we could consider is to inspect the Future's Output type and emit the lint if either the Future or Output is must_use.

Continuing to think about this I do think that it's likely to be the most straightforward solution and fairly robust. In this case, if you have a JoinHandle<()>, no lint would be emitted, as neither JoinHandle or () are marked #[must_use]. In the case of something like:

async fn res() -> Result<(), &'static str> {
    Err("For demonstrative purposes")
}

let _ = tokio::spawn(res());

You would be dealing with a JoinHandle<Result<(), &'static str>> and the lint would then be emitted. I think this is likely correct as usually if the task returns something that is #[must_use] you'll want to use it somewhere. In the event that this isn't the case, I think that allowing the line would be appropriate.

#[allow(clippy:let_underscore_future)]
let _ = tokio::spawn(res());

[matthieu-m]

Another thing we could consider is to inspect the Future's Output type and emit the lint if either the Future or Output is must_use.

I believe the two are rather independent.

In this case, for example, the output of a JoinHandle is a Result<T, ...> where T is the output of the future passed to spawn. I believe this is used, for example, if the evaluation of the future panics at some point. If the output of the future is itself a Result<X, E>, then the output of JoinHandle is Result<Result<X, E>, ...>.

Conversely, a future with an output of () may have side-effects, such as logging, that only occur if the future is polled to completion.

#[let_underscore_future] is a lint about not forgetting to drive futures to completion, and not about not forgetting to handle the result of those futures. Attempting to shove the two usecases in this lint will only result in misery.

Do remember that let _ = <expr>; is the idiomatic way to ignore the result from <expr> if it's unimportant given the context.

[SquareMan]

You are correct about JoinHandle<T>::Output being a Result<T, ...> . That is my bad for getting that wrong and so clearly my "solution" is not a solution at all and would always lint for let _ = tokio::spawn( ... )

It definitely seems to me now that there needs to be an override of some sort. Whether that should be an attribute that can be applied to the JoinHandle struct, a list of exceptions, or something else entirely I don't know.

[jeff-hiner]

There are some scenarios where it's necessary to do an empty let binding in order to specify an async block type. For example, if you want to use the ? operator rustc often isn't quite smart enough to figure out the return type of the block:

let _: tokio::task::JoinHandle<Option<std::convert::Infallible>> =
    tokio::task::spawn(async move {
        loop {
            do_some_stuff();
            something_fallible.await.ok()?;
        }
    });

As noted above, JoinHandle doesn't need to be awaited.

The standard trick from the Async Book doesn't work, because clippy (correctly) informs me that Option::<Infallible>::None outside the loop is unreachable. Thus it appears impossible to write this without manually specifying a lint override.

https://rust-lang.github.io/async-book/07_workarounds/02_err_in_async_blocks.html

[peterjoel]

A workaround here is to use an underscore, but still name the variable:

let _discard = tokio::spawn(res());

This seems like a good compromise; it communicates that you are intentionally discarding the future, without the extra noise of #[allow] or std::mem::drop.

The future will not be dropped immediately, but will typically be dropped soon enough. In the examples above, it is effectively the same as immediately, and I can't think of an obvious case where dropping it later would be a big problem.

---

Bike-shedding possible names for suggesting in a future lint upgrade: _disard, _ignore, _dontwait, _nowait, _dontjoin, _nojoin, _unjoined.

[SquareMan]

@peterjoel I also like this is as a compromise proposal but I'm also in the camp of people who don't bind the return value of tokio::spawn at all. Perhaps there still is a real solution available after all though.

Typically if you have an unused future, rustc emits an unused_must_use lint. This occurs whether the future is the result of an async fn or if you implement Future yourself on a struct. However JoinHandle does not trigger this lint, despite being a Future. What causes the lint to not be emitted in this case and can we perhaps use whatever that may be to naturally exclude JoinHandle from let_underscore_future as well? I'm not able to look into this more deeply at the moment, but wanted to throw this out there anyway if anyone happens to know more.

Example:

async fn foo() {}

fn main() {
    tokio::spawn(async {}); // No lint
    foo(); // emits unused_must_use
}

[Darksonn]

A workaround here is to use an underscore, but still name the variable:

let _discard = tokio::spawn(res());

This can actually hurt performance, because JoinHandle::drop has a fast path for the case where the JoinHandle is dropped immediately. Once the runtime has polled the future once, that fast path will no longer apply and drop will be a bit slower. Naming the variable postpones the destructor until the end of the scope, which can make that happen.