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

must_not_suspend lint regression with -Zdrop-tracking #97333

Closed
Tracked by #97331
eholk opened this issue May 23, 2022 · 5 comments
Closed
Tracked by #97331

must_not_suspend lint regression with -Zdrop-tracking #97333

eholk opened this issue May 23, 2022 · 5 comments

Comments

@eholk
Copy link
Contributor

eholk commented May 23, 2022

The test case src/test/ui/lint/must_not_suspend/ref.rs fails when -Zdrop-tracking is enabled.

We are not recognizing the Umm type as held across a borrow. Here's the revelant code from the test:

async fn uhoh(&mut self) {
let guard = &mut self.u; //~ ERROR `Umm` held across
other().await;
*guard = Umm {
i: 2
}
}

In a way, that's correct. We aren't holding Umm across the borrow, we are holding &mut Umm. The question is whether this should trigger the lint? If not, we need some way to annotate &mut Umm as also not being able to be held across the borrow.

It looks like in check_must_not_suspend_ty, we already recurse through a few types, so maybe we should look through references too?

@eholk
Copy link
Contributor Author

eholk commented May 23, 2022

@guswynn - What do you think is the right behavior here?

@guswynn
Copy link
Contributor

guswynn commented May 23, 2022

@eholk this was always an issue with must_not_suspend: how deep do we recurse into types to check. I think the conclusion was that the cases covered by the existing tests were adequate? I think its just T, `Box, and references?

@eholk
Copy link
Contributor Author

eholk commented Jul 29, 2022

I'm spending some time looking at this one again this afternoon to see if it's still an issue after @jyn514's changes. It seems like yes, although we aren't getting duplicate warnings now, we're just getting now warnings. I think seeing through references is probably the right option, but I'm getting kind of lost between the different PRs I have open so I'm going to rebase some things and figure out where we're at.

@jyn514
Copy link
Member

jyn514 commented Jul 29, 2022

Sounds good :) @eholk let me know if there's any way I can help, I've been repeatedly running into places at work that would be fixed by drop tracking haha

TaKO8Ki added a commit to TaKO8Ki/rust that referenced this issue Aug 17, 2022
…d, r=cjgillot

Make must_not_suspend lint see through references when drop tracking is enabled

See rust-lang#97333.

With drop tracking enabled, sometimes values that were previously linted are now considered dropped and not linted. This change makes must_not_suspend traverse through references to still catch these values.

Unfortunately, this leads to duplicate warnings in some cases (e.g. [dedup.rs](https://cs.github.com/rust-lang/rust/blob/9a74608543d499bcc7dd505e195e8bfab9447315/src/test/ui/lint/must_not_suspend/dedup.rs#L4)), so we only use the new behavior when drop tracking is enabled.

cc `@guswynn`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Aug 18, 2022
…d, r=cjgillot

Make must_not_suspend lint see through references when drop tracking is enabled

See rust-lang#97333.

With drop tracking enabled, sometimes values that were previously linted are now considered dropped and not linted. This change makes must_not_suspend traverse through references to still catch these values.

Unfortunately, this leads to duplicate warnings in some cases (e.g. [dedup.rs](https://cs.github.com/rust-lang/rust/blob/9a74608543d499bcc7dd505e195e8bfab9447315/src/test/ui/lint/must_not_suspend/dedup.rs#L4)), so we only use the new behavior when drop tracking is enabled.

cc ``@guswynn``
@eholk
Copy link
Contributor Author

eholk commented Aug 18, 2022

This should be fixed as of #97962!

@eholk eholk closed this as completed Aug 18, 2022
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