-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Introduce terminating scope for tail expressions of breakable scopes #106493
Conversation
This comment has been minimized.
This comment has been minimized.
d238916
to
3bf7e84
Compare
The job Click to see the possible cause of the failure (guessed by this bot)
|
@bors try let's crater it at minimum |
⌛ Trying commit 3bf7e84 with merge ebbfb508237d160e7a5e663a930916c87c4a8a45... |
☀️ Try build successful - checks-actions |
@craterbot check |
🚨 Error: missing start toolchain 🆘 If you have any trouble with Crater please ping |
@craterbot check start=2e677c0645862d17a12c6d04b3019203c8e23fcc end=ebbfb508237d160e7a5e663a930916c87c4a8a45 |
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
@craterbot abort |
🗑️ Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
@craterbot check start=master#2e677c0645862d17a12c6d04b3019203c8e23fcc end=try#ebbfb508237d160e7a5e663a930916c87c4a8a45 |
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🚨 Report generation of 🆘 Can someone from the infra team check in on this? @rust-lang/infra |
@craterbot retry-report |
🛠️ Generation of the report for ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🚨 Report generation of 🆘 Can someone from the infra team check in on this? @rust-lang/infra |
@craterbot retry-report |
🛠️ Generation of the report for ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🎉 Experiment
|
Nominating for T-lang FCP. This PR was created to fix an ICE due to a local being accessed outside of its "live region" (between StorageLive and StorageDead). The main post explains where this is a breaking change (none found on crater). The short version is that we accidentally made borrowck think some temporaries lived longer than they did. This was not unsound because we had assertions catching it. |
Sorry I haven't been on top of this issue, day job has been kicking my butt. This is not the fix to this issue that I was expecting, but it doesn't seem unreasonable. @b-naber can you clarify how the reference would have to be updated to reflect the changed semantics here? I'm specifically looking at this and surrounding sections. Are we just adding labeled blocks to the bulleted list? |
Yes, though this change more specifically only includes the tail expression of labeled blocks. We could add a temporary scope for labeled blocks in general by wrapping them in in We could in principle also fix this in mir building, e.g. by tracking all temps that are created in a labeled block and then dropping all temps that aren't moved into the destination place of the |
This feels somewhat awkward to me? I wouldn't expect that adding an unused label to a block would ever make something change behaviour. |
That's fair, we could easily check for whether a label is unused or not. Is it reasonable to assume different behavior (in terms of the tail expression scope) if the label is used? |
We discussed in lang team meeting today. We have a design meeting to cover temporary lifetimes and we'll include some discussion, but a number of us have reservations about the specific approach in this PR because having the label to a block determined when destructors execute seems surprising. Not inclined to fcp at this time, hence removing nomination. @rustbot labels -I-lang-nominated |
Closing this as it needs to be reworked and there has been no activity since so it is better to do it in a new pr with a link to this one if needed. |
Currently we schedule drops for tail expressions of blocks in the surrounding scope of the block. This does, however, cause problems with drops of temporaries and unwind paths in breakable scopes (#104736). More specifically we schedule drops for temporaries only after the exit tree was already built for a breakable scope, which allows for paths in the CFG that include drops for which no
StorageLive
was ever created. This also causes problems with the caching of unwind drops.This change is not backwards-compatible though, as we currently allow this to compile:
The following version also fails to compile though:
I think it makes sense to treat the tail expression of a breakable scope similar to the else clause.
Fixes #104736
r? @oli-obk maybe?