Skip to content

Conversation

samueltardieu
Copy link
Member

Fixes #10482

changelog: FP [redundant_async_block] Do not propose to remove async move if variables are captured by ref

@rustbot
Copy link
Collaborator

rustbot commented Mar 12, 2023

r? @xFrednet

(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 Mar 12, 2023
@samueltardieu
Copy link
Member Author

samueltardieu commented Mar 12, 2023

The captured_by_ref() function can have false positives, but this errs on the safe side.

@OliverNChalk
Copy link

Is there an estimated release date for this, our codebase is running into this quite a bit when we launch tasks to drive components

!future.span.from_expansion() &&
!await_in_expr(future)
{
if matches!(capture, CaptureBy::Value) && captured_by_ref(last) {
Copy link

Choose a reason for hiding this comment

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

This fix is rather incomplete because CaptureBy::Ref can also move a value into the closure:

use std::future::Future;

async fn work(_: &str) {}

fn spawn(_: impl Future<Output = ()> + 'static) {}

fn main() {
    let val = "Hello World".to_owned();
    spawn(async { work(&{ val }).await }); // false-positive
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, fixed.

@samueltardieu samueltardieu force-pushed the issue-10482 branch 2 times, most recently from 66f3ebf to d84b246 Compare March 15, 2023 22:30
@samueltardieu
Copy link
Member Author

I also propose to put the lint in nursery for the time being, notably because of #10509.

@xFrednet
Copy link
Contributor

Hey, thank you for the PR and comments left by others. I'll sadly be busy until the end of the week, but you'll get a review by the end of next week :)

Copy link
Contributor

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

Generally looks good to me, I have a few questions, regarding names and documentation, but I'd say this is ready, once they've been cleared up.

Thank you for your patience, while waiting for the review :)

@xFrednet xFrednet added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 21, 2023
@xFrednet
Copy link
Contributor

Looks good to me, thank you!

@bors r+

I've added the beta-nominated label. It looks like the lint has a few FP and changing the lint group should be able to land before the next release :)

@bors
Copy link
Contributor

bors commented Mar 22, 2023

📌 Commit 3497086 has been approved by xFrednet

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Mar 22, 2023

⌛ Testing commit 3497086 with merge 5839621...

@bors
Copy link
Contributor

bors commented Mar 22, 2023

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

@bors bors merged commit 5839621 into rust-lang:master Mar 22, 2023
nim65s added a commit to nim65s/thermostazv2 that referenced this pull request Mar 22, 2023
@samueltardieu samueltardieu deleted the issue-10482 branch March 24, 2023 23:47
@flip1995
Copy link
Member

This will make it into the same release as the commit that added the lint. No action required.

@flip1995 flip1995 removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 13, 2023
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.

redundant_async_block results in broken code for async fn f(&self)
7 participants