fix drop scope for super let bindings within if let
#145342
Open
+155
−41
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
They now use the enclosing temporary scope as their scope, regardless of which `ScopeData` was used to mark it.
rustbot
added
S-waiting-on-review
dianne
force-pushed
the
if-let-super-let
branch
from
f7b3d29 to
8fc3938
Compare
traviscross
added
P-lang-drag-1
|
@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.
rust-bors bot
added a commit
that referenced
this pull request
Aug 13, 2025
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 |
craterbot
added
S-waiting-on-crater
rustbot
added
the
stable-nominated
compiler-errors
added
the
beta-nominated
|
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 |
traviscross
added
T-lang
|
We talked about this in the lang call today. For our notes, here's how I'd demonstrate the issue: fn main() {
assert_drop_order(1..=4, |e| {
(
if let _x = e.log(3)
&& let _x = &e.log(2)
{
drop(e.log(1))
},
{ drop(e.log(4)) },
);
});
assert_drop_order(1..=4, |e| {
(
if let _x = e.log(2)
&& let _x = { super let x = &e.log(4); x }
{
drop(e.log(1))
},
{ drop(e.log(3)) },
);
});
}In the call, we discussed what we'd expect the behavior to be for Everyone on the call agreed with that analysis, so let's agree via FCP 1) to accept this and 2) to waive our 10-day FCP period so we can hopefully get this into the point release that we need to cut anyway due to #145262. @rfcbot fcp merge Note that the crater results aren't back yet. We can file a concern if anything surprises us enough to change the plan here. That however seems unlikely. |
|
Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. |
rfcbot
added
proposed-final-comment-period
traviscross
added
the
T-compiler
|
This proposed stable backport was discussed in the T-compiler steering meeting today and there was a fair amount of concern about backporting this to stable for a few reasons:
This is not a definitive "backport declined" but we thought it could be useful to T-lang to know our current position on backporting this. |
|
@rfcbot reviewed I would defer to T-compiler on the stable backport risk. I would encourage a beta backport however.
However, the consequences of any impact that does exist can be large (especially if the change doesn't cause any compilation errors and leads to runtime bugs), and changing this kind of subtle behavior is ungreat, so it would be good to minimize the window this bug exists in. Many thanks to @theemathas for finding this bug and to @dianne for the fix. |
|
That's only on nightly though1, since it requires #143376; landing this before the beta cutoff for 1.91.0 (or beta-backporting it after) should be sufficient to fix that. The regression visible in stable Rust is only with Footnotes
|
@tmandry |
|
Hmm, I had missed the point about I can't tell if |
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
I am unable to find a stable-to-stable regression that involves Sorry for the confusion. |
|
fn foo(ty: &Ty, f: &mut fmt::Formatter<'_>) {
if let Ty { pointee } = ty
&& let _ = write!(
f,
"{}, {}",
pointee.behind_pointer(),
ItemIdentifier::path(&ItemIdentifier::nserror()),
)
{}
} |
|
Edit: This is a different bug. Minimization of the 1.88-to-1.89 regression (all editions): #[derive(Debug)]
struct Thing;
#[derive(Debug)]
struct Ref<'a>(&'a Thing);
impl Drop for Ref<'_> {
fn drop(&mut self) {}
}
fn new_thing() -> Thing {
Thing
}
fn new_ref(x: &Thing) -> Ref<'_> {
Ref(x)
}
pub fn foo() {
let _x = format_args!("{:?}, {:?}", 1, new_ref(&new_thing()));
}The above code compiles in 1.88.0. It produces a compile error in 1.89.0. Notably, this does not use if-let. I don't understand what's happening here. |
|
Oh nice, I think that's a different bug! Lifetime extension is happening there. Here's the pretty-printed HIR: fn foo() {
let _x =
{
super let args = (&1, &new_ref(&new_thing()));
super let args =
[format_argument::new_debug(args.0),
format_argument::new_debug(args.1)];
format_arguments::new_v1(&["", ", "], &args)
};
}Unlike with This PR only changes non-lifetime-extended |
|
Here's the minimization of the 1.88-to-1.89 regression which is fixed by this PR: #[derive(Debug)]
struct Thing;
#[derive(Debug)]
struct Ref<'a>(&'a Thing);
impl Drop for Ref<'_> {
fn drop(&mut self) {}
}
fn new_thing() -> Thing {
Thing
}
fn new_ref(x: &Thing) -> Ref<'_> {
Ref(x)
}
pub fn foo(f: &mut std::fmt::Formatter<'_>) {
if let _x = write!(f, "{:?}, {:?}", 1, new_ref(&new_thing())) {}
}I am filing the other bug as a separate issue. |
|
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: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #145328 by making non-lifetime-extended
super letreuse 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