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

Clippy generating non-user-code warning when using generator implemented futures. #5360

Closed
VecrsIssues opened this issue Mar 23, 2020 · 10 comments · Fixed by #5535
Closed

Clippy generating non-user-code warning when using generator implemented futures. #5360

VecrsIssues opened this issue Mar 23, 2020 · 10 comments · Fixed by #5535
Labels
C-bug Category: Clippy is not doing the correct thing good-first-issue These issues are a good way to get started with Clippy T-async-await Type: Issues related to async/await

Comments

@VecrsIssues
Copy link

I ran this code under Clippy:

#![warn(clippy::pedantic)]

use std::error::Error;

async fn foo() {
    eprintln!("Done.");
}

async fn bar() {
    foo().await;
}

#[tokio::main]
async fn main() -> Result<(), Box<dyn Error>> {
    bar().await;
    
    Ok(())
}

I expected to see this happen: no lints generated.

Instead, this happened: Clippy produced this lint:

warning: used binding `_task_context` which is prefixed with an underscore. A leading underscore signals that a binding will not be used.
  --> src/main.rs:10:5
   |
10 |     foo().await;
   |     ^^^^^^^^^^^
   |

If you want to reproduce, you can use the Clippy tool on the playground here: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=d343f2d15d5de2e2ac222c73a9680a0b

@VecrsIssues
Copy link
Author

A solution could be to apply #[allow(unused)] directly in front of the offending variable, and remove the leading underscore, like so: ...#[allow(unused)] task_context....

@Mark-Simulacrum Mark-Simulacrum transferred this issue from rust-lang/rust Mar 23, 2020
@flip1995 flip1995 added good-first-issue These issues are a good way to get started with Clippy C-bug Category: Clippy is not doing the correct thing T-async-await Type: Issues related to async/await labels Mar 23, 2020
ChristianBeilschmidt added a commit to geo-engine/geoengine that referenced this issue Apr 11, 2020
@Neitsch
Copy link

Neitsch commented Apr 14, 2020

(Just passing through)

This warning appears reasonable to me. If I understand it correctly, it is caused by foo not actually making use of any async function. Hence there is no point in declaring it async.

@VecrsIssues
Copy link
Author

VecrsIssues commented Apr 14, 2020

(Just passing through)

This warning appears reasonable to me. If I understand it correctly, it is caused by foo not actually making use of any async function. Hence there is no point in declaring it async.

That's not correct, the lint is triggering due to the generator code that is used to implement async/await, that code uses a variable that starts with an underscore. My proposed fix would be to remove the underscore, and allow the warning that would trigger if that variable was never used.

@Arnavion
Copy link
Contributor

My proposed fix would be to remove the underscore, and allow the warning that would trigger if that variable was never used.

Did you raise this on the main rust repo? If not, do that and cc jonas-schievink. Asking for it in the clippy repo will not help.

@VecrsIssues
Copy link
Author

I did do that, it was moved here for some reason though.

@Arnavion
Copy link
Contributor

@jonas-schievink I know you intentionally made it _task_context to suppress rustc's unused lint, but marking it with #[allow(unused)] would do that and also suppress the clippy lint being discussed here. Does it make sense to do that? Or would rather have clippy change to ignore it?

@jonas-schievink
Copy link
Contributor

While I did mainly choose _task_context because it's much easier to create during HIR lowering than a lint attribute, IMO Clippy should never warn on macro- and lowering-generated code the user can't actually fix. Otherwise you'd ask every compiler contributor and macro author to anticipate any possible Clippy lint and to not make the compiler/macro generate code that trips it, which seems like an unrealistic goal.

Doesn't Clippy already ignore macro- and lowering-generated code in some lints? If so, why not ensure and test that all lints do this?

@flip1995
Copy link
Member

Yeah, this is definitely a Clippy bug, not a rustc bug. The fix is pretty easy. Just add a check if this comes from an await expansion.

@Arnavion
Copy link
Contributor

Just add a check if this comes from an await expansion.

As long as it continues to fire for something like ({ let _i = 5; some_future }).await, sure.

@bors bors closed this as completed in 2c4d566 Apr 28, 2020
Erk- referenced this issue in twilight-rs/twilight Jun 5, 2020
When the user uses a retrieval request, like getting a user by ID, the
future output is a `Result<Option<T>>`. The code to handle 404 response
status codes didn't actually exist, so it'd always either be
`Ok(Some(T))` or `Err(E)`.

This patch adds support for handling 404 responses and returning
`Ok(None)` where encountered on select retrieval routes.

Here's an example of output:

```
Getting guild member for user 1
Ok(None)
Getting guild member for user 640434332485287936
Ok(Some(Member { deaf: false, guild_id: None, hoisted_role: Non.....
```

Closes #157.

Approved-by: AEnterprise <aenterprise@aenterprise.info>
Approved-by: Valdemar Erk <valdemar@erk.io>
Merged-by: Valdemar Erk <valdemar@erk.io>
Signed-off-by: Vivian Hellyer <vivian@hellyer.dev>
@Arnavion
Copy link
Contributor

For anyone subscribed to this issue, this fix was released in today's stable 1.45.0 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing good-first-issue These issues are a good way to get started with Clippy T-async-await Type: Issues related to async/await
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants