unused_async doesn't trigger if the function is passed into another function

[junbl]

Summary

If an async function (with no await statements) is passed into another function as an argument, it no longer triggers the unused_async lint.

Believe this is a relatively recent issue - came up now because I replaced our allows with expects, but we've had a good amount of these unused_asyncs for a while.

Side note - at first I thought this was because the function it was passed into required its argument to be an async function and unused_async was smart enough to detect that, but unfortunately not. That'd be a nice feature though!

Lint Name

unused_async

Reproducer

I tried this code:

#![deny(clippy::unused_async)]

pub async fn unused_async() {}

fn baz<F>(_: F) {}

fn main() {
    
    // suppresses unused_async
    baz(unused_async);
    
}

I expected to see this happen: unused_async triggers as there are no await statements.

Instead, this happened: unused_async does not trigger, and expect produces "unfulfilled lint".

Interestingly, this only happens when my async fn is passed into another function, not if you call a method on the async fn (even if it's the same function):

#![deny(clippy::unused_async)]

pub async fn unused_async() {}

trait Foo {
    fn bar(&self) {
        
    }
}
impl<F> Foo for F { }

fn main() {

    // does not suppress unused_async
    unused_async.bar();
    
    // suppresses unused_async
    Foo::bar(&unused_async);
}

playground

Version

rustc 1.81.0 (eeb90cda1 2024-09-04)
binary: rustc
commit-hash: eeb90cda1969383f56a2637cbd3037bdf598841c
commit-date: 2024-09-04
host: x86_64-unknown-linux-gnu
release: 1.81.0
LLVM version: 18.1.7

[y21]

Side note - at first I thought this was because the function it was passed into required its argument to be an async function and unused_async was smart enough to detect that, but unfortunately not. That'd be a nice feature though!

This is the reason for the false negative (or the intention behind not linting). It doesn't emit a warning here because it thinks that the asyncness might be needed to prevent false positives, but the check is rather conservative and doesn't actually check much (it checks if the async function is used anywhere except as a callee).

But I think we can special case async fns passed as an argument if the function really doesn't require it
@rustbot claim

[junbl]

Oh neat, thanks for the clarification!

Unfortunately, my actual code is more affected by the inconsistency with the traits. It's something like this:

/// Trait from an external library
trait FutureFn {}
impl<F: FnOnce() -> Fut, Fut: Future<Output = ()>> FutureFn for F {}

fn takes_future_wrapper<F>(fut_wrapped: FutureWrapper<F>)
where
    F: FutureFn,
{
}

struct FutureWrapper<F>(F);
impl From<F> for FutureWrapper<F> { ... }

async fn unused_async() {}

fn main() {
    takes_future_wrapper(unused_async.into());
}

...which seems difficult to detect through all those layers of indirection.

For my particular use case, the current conservative behavior would work if it also applied to method calls on the async function.

[y21]

What's the false negative in this example code? It is emitting a warning here, and if anything this looks like a false positive, seeing the F: FutureFn bound that wouldn't be satisfied when removing async from the function?

To be clear, the current conservative behavior is, "does the function have no await statements and is never referenced anywhere except as a callee or a method call receiver" (phrased that wrongly in my last comment)

[junbl]

You're exactly right, it's a false positive in that case - sorry I was unclear.

To clarify - in my codebase I have some instances where unused_async fns are callees, and some where they are method call receivers. I saw that the method call receivers still produced the lint, and the callees didn't, and I had assumed that the callees were false negatives rather than the method receivers being false positives.

Here's a playground link with my example cleaned up to show the false positive: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=1e26f5e86e19e8cd4c97b68dc15d718f

[samueltardieu]

claim

Btw @y21, I also toyed a bit with this lint, and it looks like the match against MethodCall in is_node_func_call() can be removed without failing any existing test. You might want to look if this code has any use, and add a test if it does.

[y21]

The false positive above was fixed in #13471: the only case where the lint now warns is if you always directly call the async function (not if it appears in a method call receiver like before).

I also wrote up a fix for both false negatives in the original issue description where it will emit a warning if you pass the async function to a function if the where clauses on the function allow it, and propagating it up the expression chain so it can validate multiple layers of calls, e.g. takes_future_wrapper(FutureWrapper::from(unused_async)) from the last code block would still emit a warning iff you had fn takes_future_wrapper<F: Fn() -> R, R>(_: FutureWrapper<F>) which can take both async and non-async functions

However, that is a lot of code and complicates the lint a fair bit, and I'm questioning whether that's actually useful to special case and would really be hit by anyone in real code. I can't think of a practical use case for a function that can take an async function as well as non-async ones. Or rather, I don't know if you can usefully be generic over "might or might not be async" today

Do you think that emitting a warning for your original reproducer would be useful/reach real code, or is this just a very constructed case? What can you actually do with the F in the implementation of baz?

[junbl]

I don't think so. The closest thing to a use case for functions that can take a sync or async function that I can think of is StreamExt::map -- this kind of pattern is relatively common in our code:

let stream: impl Stream<Output = T> = ...;

let collection = stream.map(do_something_sync).collect::<Vec<_>>().await;
// OR
let collection = stream.map(do_something_async).collect::<FuturesUnordered<_>>().collect::<Vec<_>>().await;

So unused_async could be helpful if you wrote the following and you were trying to figure out why collection wasn't the type you expected:

let collection = stream.map(do_something_async).collect::<Vec<_>>().await;

But if you did anything with collection the compiler would probably tell you pretty quick that impl Future<Output = T> != T, so I think it's unlikely that unused_async would catch stuff on its own in that context.

Anything I can think of for functions that receive both sync and async has to do with passing the result on somewhere else, so you'd either 1. fail to compile, so the compiler'd already tell you something's wrong or 2. detect that it was needed further up the call chain with your complex logic, so it wouldn't lint regardless.

Just grasping at straws, but it would be useful if it would lint on the following, but it doesn't seem possible to detect something like this generically so I think out of scope for this lint. This:

async fn do_something(t: T) -> U {}
let collection = stream.then(do_something).collect::<Vec<_>>().await;

would be replaced with this:

fn do_something(t: T) -> U {}
let collection = stream.map(do_something).collect::<Vec<_>>().await;

So overall no it doesn't seem all that useful, except that it's a little confusing the way it works currently. Would putting a note on this page about that behavior make sense? That's where I went to try to figure out what it was doing.