Skip to content

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
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
8 changes: 2 additions & 6 deletions compiler/rustc_hir_analysis/src/check/region.rs
Original file line number Diff line number Diff line change
Expand Up @@ -490,12 +490,8 @@ fn resolve_local<'tcx>(
//
// Iterate up to the enclosing destruction scope to find the same scope that will also
// be used for the result of the block itself.
while let Some(s) = visitor.cx.var_parent {
let parent = visitor.scope_tree.parent_map.get(&s).cloned();
if let Some(Scope { data: ScopeData::Destruction, .. }) = parent {
break;
}
visitor.cx.var_parent = parent;
if let Some(inner_scope) = visitor.cx.var_parent {
(visitor.cx.var_parent, _) = visitor.scope_tree.default_temporary_scope(inner_scope)
}
}
}
Expand Down
39 changes: 39 additions & 0 deletions compiler/rustc_middle/src/middle/region.rs
Original file line number Diff line number Diff line change
Expand Up @@ -299,4 +299,43 @@ impl ScopeTree {

true
}

/// Returns the scope of non-lifetime-extended temporaries within a given scope, as well as
/// whether we've recorded a potential backwards-incompatible change to lint on.
/// Returns `None` when no enclosing temporary scope is found, such as for static items.
pub fn default_temporary_scope(&self, inner: Scope) -> (Option<Scope>, Option<Scope>) {
let mut id = inner;
let mut backwards_incompatible = None;

while let Some(&p) = self.parent_map.get(&id) {
match p.data {
ScopeData::Destruction => {
debug!("temporary_scope({inner:?}) = {id:?} [enclosing]");
return (Some(id), backwards_incompatible);
}
ScopeData::IfThenRescope | ScopeData::MatchGuard => {
debug!("temporary_scope({inner:?}) = {p:?} [enclosing]");
return (Some(p), backwards_incompatible);
}
ScopeData::Node
| ScopeData::CallSite
| ScopeData::Arguments
| ScopeData::IfThen
| ScopeData::Remainder(_) => {
// If we haven't already passed through a backwards-incompatible node,
// then check if we are passing through one now and record it if so.
// This is for now only working for cases where a temporary lifetime is
// *shortened*.
if backwards_incompatible.is_none() {
backwards_incompatible =
self.backwards_incompatible_scope.get(&p.local_id).copied();
}
id = p
}
}
}

debug!("temporary_scope({inner:?}) = None");
(None, backwards_incompatible)
}
}
37 changes: 2 additions & 35 deletions compiler/rustc_middle/src/ty/rvalue_scopes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,41 +35,8 @@ impl RvalueScopes {
// if there's one. Static items, for instance, won't
// have an enclosing scope, hence no scope will be
// returned.
let mut id = Scope { local_id: expr_id, data: ScopeData::Node };
let mut backwards_incompatible = None;

while let Some(&p) = region_scope_tree.parent_map.get(&id) {
match p.data {
ScopeData::Destruction => {
debug!("temporary_scope({expr_id:?}) = {id:?} [enclosing]");
return (Some(id), backwards_incompatible);
}
ScopeData::IfThenRescope | ScopeData::MatchGuard => {
debug!("temporary_scope({expr_id:?}) = {p:?} [enclosing]");
return (Some(p), backwards_incompatible);
}
ScopeData::Node
| ScopeData::CallSite
| ScopeData::Arguments
| ScopeData::IfThen
| ScopeData::Remainder(_) => {
// If we haven't already passed through a backwards-incompatible node,
// then check if we are passing through one now and record it if so.
// This is for now only working for cases where a temporary lifetime is
// *shortened*.
if backwards_incompatible.is_none() {
backwards_incompatible = region_scope_tree
.backwards_incompatible_scope
.get(&p.local_id)
.copied();
}
id = p
}
}
}

debug!("temporary_scope({expr_id:?}) = None");
(None, backwards_incompatible)
region_scope_tree
.default_temporary_scope(Scope { local_id: expr_id, data: ScopeData::Node })
}

/// Make an association between a sub-expression and an extended lifetime
Expand Down
112 changes: 112 additions & 0 deletions tests/ui/drop/if-let-super-let.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
//! Test for #145328: ensure the lifetime of a `super let` binding within an `if let` scrutinee is
//! at most the scope of the `if` condition's temporaries. Additionally, test `pin!` since it's
//! implemented in terms of `super let` and exposes this behavior.
//@ run-pass
//@ revisions: e2021 e2024
//@ [e2021] edition: 2021
//@ [e2024] edition: 2024

#![feature(if_let_guard)]
#![feature(super_let)]
#![expect(irrefutable_let_patterns)]

use std::cell::RefCell;
use std::pin::pin;

fn main() {
// The `super let` bindings here should have the same scope as `if let` temporaries.
// In Rust 2021, this means it lives past the end of the `if` expression.
// In Rust 2024, this means it lives to the end of the `if`'s success block.
assert_drop_order(0..=2, |o| {
#[cfg(e2021)]
(
if let _ = { super let _x = o.log(2); } { o.push(0) },
o.push(1),
);
#[cfg(e2024)]
(
if let _ = { super let _x = o.log(1); } { o.push(0) },
o.push(2),
);
});
assert_drop_order(0..=2, |o| {
#[cfg(e2021)]
(
if let true = { super let _x = o.log(2); false } {} else { o.push(0) },
o.push(1),
);
#[cfg(e2024)]
(
if let true = { super let _x = o.log(0); false } {} else { o.push(1) },
o.push(2),
);
});

// `pin!` should behave likewise.
assert_drop_order(0..=2, |o| {
#[cfg(e2021)] (if let _ = pin!(o.log(2)) { o.push(0) }, o.push(1));
#[cfg(e2024)] (if let _ = pin!(o.log(1)) { o.push(0) }, o.push(2));
});
assert_drop_order(0..=2, |o| {
#[cfg(e2021)]
(
if let None = Some(pin!(o.log(2))) {} else { o.push(0) },
o.push(1),
);
#[cfg(e2024)]
(
if let None = Some(pin!(o.log(0))) {} else { o.push(1) },
o.push(2),
);
});

// `super let` bindings' scope should also be consistent with `if let` temporaries in guards.
// Here, that means the `super let` binding in the second guard condition operand should be
// dropped before the first operand's temporary. This is consistent across Editions.
assert_drop_order(0..=1, |o| {
match () {
_ if let _ = o.log(1)
&& let _ = { super let _x = o.log(0); } => {}
_ => unreachable!(),
}
});
assert_drop_order(0..=1, |o| {
match () {
_ if let _ = o.log(1)
&& let _ = pin!(o.log(0)) => {}
_ => unreachable!(),
}
});
}

// # Test scaffolding...

struct DropOrder(RefCell<Vec<u64>>);
struct LogDrop<'o>(&'o DropOrder, u64);

impl DropOrder {
fn log(&self, n: u64) -> LogDrop<'_> {
LogDrop(self, n)
}
fn push(&self, n: u64) {
self.0.borrow_mut().push(n);
}
}

impl<'o> Drop for LogDrop<'o> {
fn drop(&mut self) {
self.0.push(self.1);
}
}

#[track_caller]
fn assert_drop_order(
ex: impl IntoIterator<Item = u64>,
f: impl Fn(&DropOrder),
) {
let order = DropOrder(RefCell::new(Vec::new()));
f(&order);
let order = order.0.into_inner();
let expected: Vec<u64> = ex.into_iter().collect();
assert_eq!(order, expected);
}
Loading