Skip to content

emit StorageLive and schedule StorageDead for let-else's bindings after matching #143028

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 1 commit 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
12 changes: 1 addition & 11 deletions compiler/rustc_mir_build/src/builder/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use rustc_span::Span;
use tracing::debug;

use crate::builder::ForGuard::OutsideGuard;
use crate::builder::matches::{DeclareLetBindings, EmitStorageLive, ScheduleDrops};
use crate::builder::matches::{DeclareLetBindings, ScheduleDrops};
use crate::builder::{BlockAnd, BlockAndExtension, BlockFrame, Builder};

impl<'a, 'tcx> Builder<'a, 'tcx> {
Expand Down Expand Up @@ -199,15 +199,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
None,
Some((Some(&destination), initializer_span)),
);
this.visit_primary_bindings(pattern, &mut |this, node, span| {
this.storage_live_binding(
block,
node,
span,
OutsideGuard,
ScheduleDrops::Yes,
);
});
let else_block_span = this.thir[*else_block].span;
let (matching, failure) =
this.in_if_then_scope(last_remainder_scope, else_block_span, |this| {
Expand All @@ -218,7 +209,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
None,
initializer_span,
DeclareLetBindings::No,
EmitStorageLive::No,
)
});
matching.and(failure)
Expand Down
52 changes: 8 additions & 44 deletions compiler/rustc_mir_build/src/builder/matches/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,18 +69,6 @@ pub(crate) enum DeclareLetBindings {
LetNotPermitted,
}

/// Used by [`Builder::bind_matched_candidate_for_arm_body`] to determine
/// whether or not to call [`Builder::storage_live_binding`] to emit
/// [`StatementKind::StorageLive`].
#[derive(Clone, Copy)]
pub(crate) enum EmitStorageLive {
/// Yes, emit `StorageLive` as normal.
Yes,
/// No, don't emit `StorageLive`. The caller has taken responsibility for
/// emitting `StorageLive` as appropriate.
No,
}

/// Used by [`Builder::storage_live_binding`] and [`Builder::bind_matched_candidate_for_arm_body`]
/// to decide whether to schedule drops.
#[derive(Clone, Copy, Debug)]
Expand Down Expand Up @@ -207,7 +195,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
Some(args.variable_source_info.scope),
args.variable_source_info.span,
args.declare_let_bindings,
EmitStorageLive::Yes,
),
_ => {
let mut block = block;
Expand Down Expand Up @@ -479,7 +466,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
&built_match_tree.fake_borrow_temps,
scrutinee_span,
Some((arm, match_scope)),
EmitStorageLive::Yes,
);

this.fixed_temps_scope = old_dedup_scope;
Expand Down Expand Up @@ -533,7 +519,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
fake_borrow_temps: &[(Place<'tcx>, Local, FakeBorrowKind)],
scrutinee_span: Span,
arm_match_scope: Option<(&Arm<'tcx>, region::Scope)>,
emit_storage_live: EmitStorageLive,
) -> BasicBlock {
if branch.sub_branches.len() == 1 {
let [sub_branch] = branch.sub_branches.try_into().unwrap();
Expand All @@ -544,7 +529,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
scrutinee_span,
arm_match_scope,
ScheduleDrops::Yes,
emit_storage_live,
)
} else {
// It's helpful to avoid scheduling drops multiple times to save
Expand Down Expand Up @@ -577,7 +561,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
scrutinee_span,
arm_match_scope,
schedule_drops,
emit_storage_live,
);
if arm.is_none() {
schedule_drops = ScheduleDrops::No;
Expand Down Expand Up @@ -741,7 +724,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
&[],
irrefutable_pat.span,
None,
EmitStorageLive::Yes,
)
.unit()
}
Expand Down Expand Up @@ -2364,7 +2346,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
source_scope: Option<SourceScope>,
scope_span: Span,
declare_let_bindings: DeclareLetBindings,
emit_storage_live: EmitStorageLive,
) -> BlockAnd<()> {
let expr_span = self.thir[expr_id].span;
let scrutinee = unpack!(block = self.lower_scrutinee(block, expr_id, expr_span));
Expand Down Expand Up @@ -2398,14 +2379,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
}
}

let success = self.bind_pattern(
self.source_info(pat.span),
branch,
&[],
expr_span,
None,
emit_storage_live,
);
let success = self.bind_pattern(self.source_info(pat.span), branch, &[], expr_span, None);

// If branch coverage is enabled, record this branch.
self.visit_coverage_conditional_let(pat, success, built_tree.otherwise_block);
Expand All @@ -2428,7 +2402,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
scrutinee_span: Span,
arm_match_scope: Option<(&Arm<'tcx>, region::Scope)>,
schedule_drops: ScheduleDrops,
emit_storage_live: EmitStorageLive,
) -> BasicBlock {
debug!("bind_and_guard_matched_candidate(subbranch={:?})", sub_branch);

Expand Down Expand Up @@ -2547,7 +2520,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
post_guard_block,
ScheduleDrops::Yes,
by_value_bindings,
emit_storage_live,
);

post_guard_block
Expand All @@ -2559,7 +2531,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
block,
schedule_drops,
sub_branch.bindings.iter(),
emit_storage_live,
);
block
}
Expand Down Expand Up @@ -2730,7 +2701,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
block: BasicBlock,
schedule_drops: ScheduleDrops,
bindings: impl IntoIterator<Item = &'b Binding<'tcx>>,
emit_storage_live: EmitStorageLive,
) where
'tcx: 'b,
{
Expand All @@ -2740,19 +2710,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
// Assign each of the bindings. This may trigger moves out of the candidate.
for binding in bindings {
let source_info = self.source_info(binding.span);
let local = match emit_storage_live {
// Here storages are already alive, probably because this is a binding
// from let-else.
// We just need to schedule drop for the value.
EmitStorageLive::No => self.var_local_id(binding.var_id, OutsideGuard).into(),
EmitStorageLive::Yes => self.storage_live_binding(
block,
binding.var_id,
binding.span,
OutsideGuard,
schedule_drops,
),
};
let local = self.storage_live_binding(
block,
binding.var_id,
binding.span,
OutsideGuard,
schedule_drops,
);
if matches!(schedule_drops, ScheduleDrops::Yes) {
self.schedule_drop_for_binding(binding.var_id, binding.span, OutsideGuard);
}
Expand Down
3 changes: 1 addition & 2 deletions tests/mir-opt/building/issue_101867.main.built.after.mir
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ fn main() -> () {
_1 = Option::<u8>::Some(const 1_u8);
FakeRead(ForLet(None), _1);
AscribeUserType(_1, o, UserTypeProjection { base: UserType(1), projs: [] });
StorageLive(_5);
PlaceMention(_1);
_6 = discriminant(_1);
switchInt(move _6) -> [1: bb4, otherwise: bb3];
Expand Down Expand Up @@ -55,6 +54,7 @@ fn main() -> () {
}

bb6: {
StorageLive(_5);
_5 = copy ((_1 as Some).0: u8);
_0 = const ();
StorageDead(_5);
Expand All @@ -63,7 +63,6 @@ fn main() -> () {
}

bb7: {
StorageDead(_5);
goto -> bb1;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,6 @@ fn let_else() -> () {
}

bb0: {
StorageLive(_2);
StorageLive(_3);
StorageLive(_4);
StorageLive(_5);
StorageLive(_6);
StorageLive(_7);
Expand Down Expand Up @@ -51,26 +48,26 @@ fn let_else() -> () {

bb4: {
AscribeUserType(_5, +, UserTypeProjection { base: UserType(1), projs: [] });
StorageLive(_2);
_2 = copy (_5.0: u32);
StorageLive(_3);
_3 = copy (_5.1: u64);
StorageLive(_4);
_4 = copy (_5.2: &char);
StorageDead(_7);
StorageDead(_5);
_0 = const ();
StorageDead(_8);
StorageDead(_4);
StorageDead(_3);
StorageDead(_2);
StorageDead(_8);
return;
}

bb5: {
StorageDead(_7);
StorageDead(_5);
StorageDead(_8);
StorageDead(_4);
StorageDead(_3);
StorageDead(_2);
goto -> bb1;
}

Expand Down
7 changes: 4 additions & 3 deletions tests/ui/dropck/let-else-more-permissive.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
//! The drop check is currently more permissive when `let` statements have an `else` block, due to
//! scheduling drops for bindings' storage before pattern-matching (#142056).
//! Regression test for #142056. The drop check used to be more permissive for `let` statements with
//! `else` blocks, due to scheduling drops for bindings' storage before pattern-matching.

struct Struct<T>(T);
impl<T> Drop for Struct<T> {
Expand All @@ -14,10 +14,11 @@ fn main() {
//~^ ERROR `short1` does not live long enough
}
{
// This is OK: `short2`'s storage is live until after `long2`'s drop runs.
// This was OK: `short2`'s storage was live until after `long2`'s drop ran.
#[expect(irrefutable_let_patterns)]
let (mut long2, short2) = (Struct(&0), 1) else { unreachable!() };
long2.0 = &short2;
//~^ ERROR `short2` does not live long enough
}
{
// Sanity check: `short3`'s drop is significant; it's dropped before `long3`:
Expand Down
20 changes: 18 additions & 2 deletions tests/ui/dropck/let-else-more-permissive.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,24 @@ LL | }
|
= note: values in a scope are dropped in the opposite order they are defined

error[E0597]: `short2` does not live long enough
--> $DIR/let-else-more-permissive.rs:20:19
|
LL | let (mut long2, short2) = (Struct(&0), 1) else { unreachable!() };
| ------ binding `short2` declared here
LL | long2.0 = &short2;
| ^^^^^^^ borrowed value does not live long enough
LL |
LL | }
| -
| |
| `short2` dropped here while still borrowed
| borrow might be used here, when `long2` is dropped and runs the `Drop` code for type `Struct`
|
= note: values in a scope are dropped in the opposite order they are defined

error[E0597]: `short3` does not live long enough
--> $DIR/let-else-more-permissive.rs:27:19
--> $DIR/let-else-more-permissive.rs:28:19
|
LL | let (mut long3, short3) = (Struct(&tmp), Box::new(1)) else { unreachable!() };
| ------ binding `short3` declared here
Expand All @@ -30,6 +46,6 @@ LL | }
|
= note: values in a scope are dropped in the opposite order they are defined

error: aborting due to 2 previous errors
error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0597`.
Loading