-
Notifications
You must be signed in to change notification settings - Fork 13.7k
fix drop scope for super let
bindings within if let
#145342
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
base: master
Are you sure you want to change the base?
Conversation
They now use the enclosing temporary scope as their scope, regardless of which `ScopeData` was used to mark it.
f7b3d29
to
8fc3938
Compare
@dianne does this need crater? |
@bors2 try |
This comment was marked as resolved.
This comment was marked as resolved.
This comment has been minimized.
This comment has been minimized.
fix drop scope for `super let` bindings within `if let`
@craterbot check |
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
How does #145374 relate to this? The code there doesn't use |
Format arguments, e.g. as used by
The interaction between that, old editions not treating block tail expressions as temporary drop scopes, my guard scoping PR, and the existing |
This code causes this PR to ICE in the pub fn foo() {
drop(format_args!("a"));
} Error output
|
Thanks for testing this! That's also a separate bug; it ICEs with debug asserts on before my commits. Here's what it looks like on stable/nightly with debug asserts off:
Edit: To make sure there's no interaction, I've recompiled this PR with debug asserts off and am getting the |
🎉 Experiment
|
The crater run is clean. No relevant regressions or fixes. |
Summary of everything so farI am naming each of the regressions: A, B, C, D, so it's easier to refer to them in the future. For each of them, I give a link to a reproducer of the bug, and I describe when the regression occurred, the prerequisites for code to be affected by the bug, the likelihood (in my opinion) of real user code being affected, and the impact of the bug on such code.
Additionally, there's:
|
With regard to regression D, I imagine it affects edition 2024 as well for match arms where the arm body isn't wrapped in a block expression. It's caused by #143376 extending the surface area of the bug underlying regressions A/B to all match arms with guards where the |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
We discussed this in the @rust-lang/libs-api meeting and we're happy to defer to lang for this issue. |
Stable backport declined as per compiler team on Zulip. We reviewed again the stable backport and our concerns still stand. @rustbot label -stable-nominated |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
@craterbot run mode=check-only crates=https://crater-reports.s3.amazonaws.com/pr-145342/retry-regressed-list.txt |
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
Something to consider for the FCP: this PR does not make the identity in #145342 (comment) hold. See #145784 (comment) for how it fails to hold in non-extending expressions, due to My personal judgment is that that issue is orthogonal to the bug this PR fixes: this PR fixes |
Fixes #145328 by making non-lifetime-extended
super let
reuse the logic used to compute drop scopes for non-lifetime-extended temporaries.Also fixes #145374, which regressed due to #143376 introducing
if let
-like scopes for match arms with guards.Tracking issue for
super let
: #139076This is a regression fix / breaking change for macros stably exposing
super let
, includingpin!
andformat_args!
.Nominating to be discussed alongside #145328: @rustbot label +I-lang-nominated +I-libs-api-nominated