Make must_not_suspend lint see through references when drop tracking is enabled #97962
Conversation
rustbot
added
the
T-compiler
|
r? @cjgillot (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
| @@ -558,6 +560,12 @@ pub fn check_must_not_suspend_ty<'tcx>( | |||
| }, | |||
| ) | |||
| } | |||
| // If drop tracking is enabled, we want to look through references, since the referrent | |||
| // may not be considered live across the await point. | |||
There was a problem hiding this comment.
Why don't we want to look through references when we are not in drop_tracking mode?
There was a problem hiding this comment.
the PR description says
Unfortunately, this leads to duplicate warnings in some cases (e.g. dedup.rs), so we only use the new behavior when drop tracking is enabled.
There was a problem hiding this comment.
I did not see that this whole function could only emit a lint.
Anyway, why does looking through references cause duplicate warnings?
There was a problem hiding this comment.
Sorry for the delay, I finally have time to look at this again.
Without drop tracking, we use a scope-based analysis to decide what should be considered live across an await point, while the drop tracking version is more precise. With the scope based analysis, I think what's happening is we are seeing both a &mut Umm live across the await point and an Umm live across the await point. The first one doesn't trigger the lint because we currently don't see through the reference, but we do warn on the second.
With drop tracking, we don't see the Umm but instead only see &mut Umm. In order to warn, we need to look through the reference. The reason we don't see the Umm is because drop tracking handles temporary values differently, which leads to use not considering the field in self, but only the reference to it.
cjgillot
added
S-waiting-on-author
jyn514
added
S-waiting-on-review
eholk
force-pushed
the
drop-tracking-must-not-suspend
branch
from
5d61b6d
to
0da8199
Compare
|
@eholk, what's the status of this PR? Is this still a draft? |
|
@cjgillot I left it as a draft because I was hoping I'd come up with a better way to do it, where we could keep the behavior consistent with and without drop tracking but also not get duplicate errors. I think it's probably better to go ahead and land as-is. We can adjust the warnings easy enough in the future if needed. @bors r=cjgillot |
|
@eholk: 🔑 Insufficient privileges: Not in reviewers |
|
@bors r=cjgillot |
bors
added
S-waiting-on-bors
…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`
…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#97962 (Make must_not_suspend lint see through references when drop tracking is enabled) - rust-lang#99966 (avoid assertion failures in try_to_scalar_int) - rust-lang#100637 (Improving Fuchsia rustc support documentation) - rust-lang#100643 (Point at a type parameter shadowing another type) - rust-lang#100651 (Migrations for rustc_expand transcribe.rs) - rust-lang#100669 (Attribute cleanups) - rust-lang#100670 (Fix documentation of rustc_parse::parser::Parser::parse_stmt_without_recovery) - rust-lang#100674 (Migrate lint reports in typeck::check_unused to LintDiagnostic) - rust-lang#100688 (`ty::Error` does not match other types for region constraints) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
See #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), so we only use the new behavior when drop tracking is enabled.
cc @guswynn