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

x11rb-async async functions should have #[must_use] attributes #903

Open
Antikyth opened this issue Nov 19, 2023 · 3 comments
Open

x11rb-async async functions should have #[must_use] attributes #903

Antikyth opened this issue Nov 19, 2023 · 3 comments

Comments

@Antikyth
Copy link

It is easy to forget to .await an async fn, and without a #[must_use] attribute, you won't get a warning for forgetting to do so.

@Antikyth
Copy link
Author

This should probably go for cookies too actually.

@psychon
Copy link
Owner

psychon commented Nov 19, 2023

My first thought is "this is not x11rb-async specific, but should rather be a rust ecosystem thing", but I cannot find much compiler or clippy lints that would warn about this. The closest I found are https://rust-lang.github.io/rust-clippy/master/index.html#/let_underscore_future and https://rust-lang.github.io/rust-clippy/master/index.html#/unused_async but neither would exactly hit this case.

Actually... the Future trait is marked must_use, so shouldn't that already be enough for a warning? I'm not sure.

But I just tried this and I couldn't get a warning. conn.query_tree(window); doesn't do anything. At least conn.query_tree(window).await; generates a warning due to the Result. conn.query_tree(window).await?; then again doesn't warn. With conn.query_tree(window).await?.reply(); I finally get warning: unused implementer of Future that must be used. conn.query_tree(window).await?.reply().await; again warns due to the unused Result.

Ahh, query_tree does not return a Future, but a Pin<Box<dyn Future<....>>>. That's why there is no warning here. x11rb_async::protocol::xproto::query_tree(conn, window); also causes a warning due to the naked future.

This should probably go for cookies too actually.

I'm not quite sure. The cookie by itself doesn't do much. The request was already sent and there are valid cases for ignoring a cookie. I guess must VoidCookies are immediately dropped and I bet I could even come up with a case where dropping a Cookie would be the right thing to do.

Edit: Now that I know that this is about boxed futures, I found rust-lang/futures-rs#2017 which leads to rust-lang/rust#67387 and rust-lang/rust#73417

@notgull
Copy link
Collaborator

notgull commented Feb 5, 2024

Actually... the Future trait is marked must_use, so shouldn't that already be enough for a warning? I'm not sure.

At least for async it should be. Like you mentioned it's a footgun that should be fixed on the Rust level.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants