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

[unused_async]: don't lint if paths reference async fn without immediate call #11200

Merged
merged 2 commits into from
Jul 22, 2023

Conversation

y21
Copy link
Member

@y21 y21 commented Jul 20, 2023

Fixes #9695
Fixes #9359

Clippy shouldn't lint unused async if there are paths referencing them if that path isn't the receiver of a function call, because that means that the function might be passed to some other function:

async fn f() {} // No await statements, so unused at this point

fn requires_fn_future<F: Future<Output = ()>>(_: fn() -> F) {}
requires_fn_future(f); // `f`'s asyncness is actually not unused.

(This isn't limited to just passing the function as a parameter to another function, it could also first be stored in a variable and later passed to another function as an argument)

This requires delaying the linting until post-crate and collecting path references to local async functions along the way.

changelog: [unused_async]: don't lint if paths reference async fn that require asyncness

@rustbot
Copy link
Collaborator

rustbot commented Jul 20, 2023

r? @Jarcho

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 20, 2023
clippy_lints/src/unused_async.rs Outdated Show resolved Hide resolved
clippy_lints/src/unused_async.rs Outdated Show resolved Hide resolved
Comment on lines +149 to +150
// Depending on how `x` is used, f's asyncness might be required despite not having any `await`
// statements, so don't lint at all if there are any such paths.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this can use #11166 once that's merged, that way some cases are still linted

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unlikely to be the case. The signatures of the two functions are incompatible.

@Jarcho
Copy link
Contributor

Jarcho commented Jul 22, 2023

Thank you. @bors r+

@bors
Copy link
Contributor

bors commented Jul 22, 2023

📌 Commit 482d5fa has been approved by Jarcho

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 22, 2023

⌛ Testing commit 482d5fa with merge fa026ee...

bors added a commit that referenced this pull request Jul 22, 2023
[`unused_async`]: don't lint if paths reference async fn without immediate call

Fixes #9695
Fixes #9359

Clippy shouldn't lint unused `async` if there are paths referencing them if that path isn't the receiver of a function call, because that means that the function might be passed to some other function:
```rs
async fn f() {} // No await statements, so unused at this point

fn requires_fn_future<F: Future<Output = ()>>(_: fn() -> F) {}
requires_fn_future(f); // `f`'s asyncness is actually not unused.
```
(This isn't limited to just passing the function as a parameter to another function, it could also first be stored in a variable and later passed to another function as an argument)

This requires delaying the linting until post-crate and collecting path references to local async functions along the way.

[`unused_async`]: don't lint if paths reference async fn that require asyncness
@bors
Copy link
Contributor

bors commented Jul 22, 2023

💔 Test failed - checks-action_test

@y21
Copy link
Member Author

y21 commented Jul 22, 2023

oops, I must have deleted the changelog word from the pr description again somehow. It's fixed now

@Jarcho
Copy link
Contributor

Jarcho commented Jul 22, 2023

@bors retry

@bors
Copy link
Contributor

bors commented Jul 22, 2023

⌛ Testing commit 482d5fa with merge e8403a8...

@bors
Copy link
Contributor

bors commented Jul 22, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Jarcho
Pushing e8403a8 to master...

@bors bors merged commit e8403a8 into rust-lang:master Jul 22, 2023
bors added a commit that referenced this pull request Sep 28, 2024
Remove method call receiver special casing in `unused_async` lint

Fixes the false positive mentioned in #13466 (comment).

The false negative in the OP would be nice to fix too, but I'd rather do that in a separate PR because it's much more involved

Before this change, the `unused_async` lint would check if the async fn is also used anywhere and avoid linting if so. The exception is if the async function is immediately called, because the returned future handling can be easily removed (and also if we don't have some exceptions then the lint wouldn't trigger anywhere) *or* if it's a method call receiver.

I'm not exactly sure why I implemented that special casing for method call receivers in #11200, but it doesn't make much sense in hindsight imo. Especially given that method calls are essentially equivalent to function calls with the receiver as the first argument, which was the primary motivation for not linting in the first place (async fn passed to another function, like `axum::get(handler)` where handler has to be an async fn).

changelog: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
5 participants