-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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 | ||
// 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 { | ||
|
@@ -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 => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 :? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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 | ||
|
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() {} |
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 | ||
|
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.