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

Mini-fix double_must_use for async functions #10589

Merged
merged 3 commits into from
Apr 4, 2023

Conversation

blyxyas
Copy link
Member

@blyxyas blyxyas commented Apr 2, 2023

From Rust 1.67 onwards, the #[must_use] attribute also applies to the Future::Output (rust-lang/rust#100633). So the lint double_must_use was linting all async functions. This PR changes the double_must_use lint so it ignores async functions.


Closes #10486
changelog: [double_must_use]: Fix false positive in async function

@rustbot
Copy link
Collaborator

rustbot commented Apr 2, 2023

r? @giraffate

(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 Apr 2, 2023
Copy link
Contributor

@giraffate giraffate left a comment

Choose a reason for hiding this comment

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

Can this catch the case #[must_use] async fn async_result() -> Result<(), ()> { Ok(()) }?

@@ -43,7 +44,7 @@ pub(super) fn check_item<'tcx>(cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>
}

pub(super) fn check_impl_item<'tcx>(cx: &LateContext<'tcx>, item: &'tcx hir::ImplItem<'_>) {
if let hir::ImplItemKind::Fn(ref sig, ref body_id) = item.kind {
if let hir::ImplItemKind::Fn(ref sig, ref body_id) = item.kind && !sig.header.is_async() /* (#10486) */ {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this affect must_use_unit? If so, it would seem to me that it should be limited to double_must_use only.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in the last commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! I made one comment.

@blyxyas blyxyas force-pushed the fix-double_must_use branch from 2781fc9 to d602743 Compare April 3, 2023 13:16
}

#[must_use]
async fn async_must_use_result() -> Result<(), ()> {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like that this case is false negative. It would be better to check that the return type in async function already has #[must_use]. I think we can get the type from Future::Output like below:

let infcx = cx.tcx.infer_ctxt().build();
let Some(ret_ty) = infcx.get_impl_future_output_ty(return_ty(cx, item_id));

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@blyxyas blyxyas force-pushed the fix-double_must_use branch from 4b27377 to f4b701e Compare April 3, 2023 14:02
@blyxyas blyxyas force-pushed the fix-double_must_use branch from f4b701e to a37eb4d Compare April 3, 2023 14:07
@giraffate
Copy link
Contributor

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented Apr 4, 2023

📌 Commit a37eb4d has been approved by giraffate

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Apr 4, 2023

⌛ Testing commit a37eb4d with merge 85d9f17...

@bors
Copy link
Contributor

bors commented Apr 4, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: giraffate
Pushing 85d9f17 to master...

@bors bors merged commit 85d9f17 into rust-lang:master Apr 4, 2023
@blyxyas blyxyas deleted the fix-double_must_use branch October 5, 2023 09:05
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
Development

Successfully merging this pull request may close these issues.

clippy::double-must-use emitted for #[must_use] on async fn
4 participants