Skip to content

Handle DropKind::ForLint in coroutines correctly #134575

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

Merged
merged 1 commit into from
Dec 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions compiler/rustc_mir_build/src/builder/scope.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1481,14 +1481,6 @@ fn build_scope_drops<'tcx>(
block = next;
}
DropKind::ForLint => {
// If the operand has been moved, and we are not on an unwind
// path, then don't generate the drop. (We only take this into
// account for non-unwind paths so as not to disturb the
// caching mechanism.)
if scope.moved_locals.iter().any(|&o| o == local) {
continue;
}

// As in the `DropKind::Storage` case below:
// normally lint-related drops are not emitted for unwind,
// so we can just leave `unwind_to` unmodified, but in some
Expand All @@ -1500,6 +1492,14 @@ fn build_scope_drops<'tcx>(
unwind_to = unwind_drops.drops[unwind_to].next;
}

// If the operand has been moved, and we are not on an unwind
Copy link
Member Author

Choose a reason for hiding this comment

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

We want to do this early skip after we've adjusted the unwind_to block.

Copy link
Member Author

Choose a reason for hiding this comment

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

At least I think we do -- this isn't realted, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think you're right, that seems to match what happens above. It makes sense that we would do it after.

// path, then don't generate the drop. (We only take this into
// account for non-unwind paths so as not to disturb the
// caching mechanism.)
if scope.moved_locals.iter().any(|&o| o == local) {
continue;
}

cfg.push(block, Statement {
source_info,
kind: StatementKind::BackwardIncompatibleDropHint {
Expand Down Expand Up @@ -1552,7 +1552,7 @@ impl<'a, 'tcx: 'a> Builder<'a, 'tcx> {
let mut unwind_indices = IndexVec::from_elem_n(unwind_target, 1);
for (drop_idx, drop_node) in drops.drops.iter_enumerated().skip(1) {
match drop_node.data.kind {
DropKind::Storage => {
DropKind::Storage | DropKind::ForLint => {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the fix for the ICE. Not totally sure why it fixes it :?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. I think it fixes it because we generally copied the "storage" logic -- i.e., we only adjust unwind_to when storage_dead_on_unwind is true. I'm not 100% sure what the "add entry point" business is about though.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I did a bit of looking, I think this is correct, and it has to do with the fact that we are inserting a statement into a block and not a terminator.

if is_coroutine {
let unwind_drop = self
.scopes
Expand All @@ -1563,7 +1563,7 @@ impl<'a, 'tcx: 'a> Builder<'a, 'tcx> {
unwind_indices.push(unwind_indices[drop_node.next]);
}
}
DropKind::Value | DropKind::ForLint => {
DropKind::Value => {
let unwind_drop = self
.scopes
.unwind_drops
Expand Down
29 changes: 29 additions & 0 deletions tests/ui/drop/tail_expr_drop_order-on-coroutine-unwind.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
//@ edition: 2021
//@ build-fail

// Make sure we don't ICE when emitting the "lint" drop statement
// used for tail_expr_drop_order.

#![deny(tail_expr_drop_order)]

struct Drop;
impl std::ops::Drop for Drop {
fn drop(&mut self) {}
}

async fn func() -> Result<(), Drop> {
todo!()
}

async fn retry_db() -> Result<(), Drop> {
loop {
match func().await {
//~^ ERROR relative drop order changing in Rust 2024
//~| WARNING this changes meaning in Rust 2024
Ok(()) => return Ok(()),
Err(e) => {}
}
}
}

fn main() {}
52 changes: 52 additions & 0 deletions tests/ui/drop/tail_expr_drop_order-on-coroutine-unwind.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
error: relative drop order changing in Rust 2024
--> $DIR/tail_expr_drop_order-on-coroutine-unwind.rs:20:15
|
LL | match func().await {
| ^^^^^^^-----
| | |
| | this value will be stored in a temporary; let us call it `#1`
| | `#1` will be dropped later as of Edition 2024
| this value will be stored in a temporary; let us call it `#2`
| up until Edition 2021 `#2` is dropped last but will be dropped earlier in Edition 2024
...
LL | Err(e) => {}
| -
| |
| `e` calls a custom destructor
| `e` will be dropped later as of Edition 2024
LL | }
LL | }
| - now the temporary value is dropped here, before the local variables in the block or statement
|
= warning: this changes meaning in Rust 2024
= note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2024/temporary-tail-expr-scope.html>
note: `#2` invokes this custom destructor
--> $DIR/tail_expr_drop_order-on-coroutine-unwind.rs:10:1
|
LL | / impl std::ops::Drop for Drop {
LL | | fn drop(&mut self) {}
LL | | }
| |_^
note: `#1` invokes this custom destructor
--> $DIR/tail_expr_drop_order-on-coroutine-unwind.rs:10:1
|
LL | / impl std::ops::Drop for Drop {
LL | | fn drop(&mut self) {}
LL | | }
| |_^
note: `e` invokes this custom destructor
--> $DIR/tail_expr_drop_order-on-coroutine-unwind.rs:10:1
|
LL | / impl std::ops::Drop for Drop {
LL | | fn drop(&mut self) {}
LL | | }
| |_^
= note: most of the time, changing drop order is harmless; inspect the `impl Drop`s for side effects like releasing locks or sending messages
note: the lint level is defined here
--> $DIR/tail_expr_drop_order-on-coroutine-unwind.rs:7:9
|
LL | #![deny(tail_expr_drop_order)]
| ^^^^^^^^^^^^^^^^^^^^

error: aborting due to 1 previous error

Loading