Skip to content

Commit

Permalink
Auto merge of #101894 - dingxiangfei2009:let-else-avoid-duplicate-sto…
Browse files Browse the repository at this point in the history
…rage-live, r=oli-obk

Avoid duplicating StorageLive in let-else

cc `@est31`

Fix #101867
Fix #101932

#101410 introduced directives to activate storages of bindings in let-else earlier. However, since it is using the machinery of `match` and friends for pattern matching and binding, those storages are activated for the second time. This PR adjusts this behavior and avoid the duplicated activation for let-else statements.
  • Loading branch information
bors committed Sep 19, 2022
2 parents 11bb80a + eb36f5e commit 2019147
Show file tree
Hide file tree
Showing 5 changed files with 136 additions and 9 deletions.
3 changes: 2 additions & 1 deletion compiler/rustc_mir_build/src/build/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
pattern,
UserTypeProjections::none(),
&mut |this, _, _, _, node, span, _, _| {
this.storage_live_binding(block, node, span, OutsideGuard, false);
this.storage_live_binding(block, node, span, OutsideGuard, true);
this.schedule_drop_for_binding(node, span, OutsideGuard);
},
);
let failure = unpack!(
Expand Down
39 changes: 31 additions & 8 deletions compiler/rustc_mir_build/src/build/matches/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
Some(arm.span),
Some(arm.scope),
Some(match_scope),
false,
);

if let Some(source_scope) = scope {
Expand Down Expand Up @@ -416,6 +417,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
arm_span: Option<Span>,
arm_scope: Option<region::Scope>,
match_scope: Option<region::Scope>,
storages_alive: bool,
) -> BasicBlock {
if candidate.subcandidates.is_empty() {
// Avoid generating another `BasicBlock` when we only have one
Expand All @@ -429,6 +431,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
arm_span,
match_scope,
true,
storages_alive,
)
} else {
// It's helpful to avoid scheduling drops multiple times to save
Expand Down Expand Up @@ -466,6 +469,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
arm_span,
match_scope,
schedule_drops,
storages_alive,
);
if arm_scope.is_none() {
schedule_drops = false;
Expand Down Expand Up @@ -641,6 +645,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
None,
None,
None,
false,
)
.unit()
}
Expand Down Expand Up @@ -1813,6 +1818,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
None,
None,
None,
false,
);

post_guard_block.unit()
Expand All @@ -1836,6 +1842,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
arm_span: Option<Span>,
match_scope: Option<region::Scope>,
schedule_drops: bool,
storages_alive: bool,
) -> BasicBlock {
debug!("bind_and_guard_matched_candidate(candidate={:?})", candidate);

Expand Down Expand Up @@ -2051,7 +2058,12 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
self.cfg.push_fake_read(post_guard_block, guard_end, cause, Place::from(local_id));
}
assert!(schedule_drops, "patterns with guards must schedule drops");
self.bind_matched_candidate_for_arm_body(post_guard_block, true, by_value_bindings);
self.bind_matched_candidate_for_arm_body(
post_guard_block,
true,
by_value_bindings,
storages_alive,
);

post_guard_block
} else {
Expand All @@ -2065,6 +2077,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
.iter()
.flat_map(|(bindings, _)| bindings)
.chain(&candidate.bindings),
storages_alive,
);
block
}
Expand Down Expand Up @@ -2154,6 +2167,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
block: BasicBlock,
schedule_drops: bool,
bindings: impl IntoIterator<Item = &'b Binding<'tcx>>,
storages_alive: bool,
) where
'tcx: 'b,
{
Expand All @@ -2163,13 +2177,20 @@ 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 = self.storage_live_binding(
block,
binding.var_id,
binding.span,
OutsideGuard,
schedule_drops,
);
let local = if storages_alive {
// Here storages are already alive, probably because this is a binding
// from let-else.
// We just need to schedule drop for the value.
self.var_local_id(binding.var_id, OutsideGuard).into()
} else {
self.storage_live_binding(
block,
binding.var_id,
binding.span,
OutsideGuard,
schedule_drops,
)
};
if schedule_drops {
self.schedule_drop_for_binding(binding.var_id, binding.span, OutsideGuard);
}
Expand Down Expand Up @@ -2300,6 +2321,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
None,
None,
None,
true,
);
// This block is for the failure case
let failure = this.bind_pattern(
Expand All @@ -2311,6 +2333,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
None,
None,
None,
true,
);
this.break_for_else(failure, *let_else_scope, this.source_info(initializer_span));
matching.unit()
Expand Down
9 changes: 9 additions & 0 deletions src/test/mir-opt/issue-101867.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#![cfg_attr(bootstrap, feature(let_else))]

// EMIT_MIR issue_101867.main.mir_map.0.mir
fn main() {
let x: Option<u8> = Some(1);
let Some(y) = x else {
panic!();
};
}
75 changes: 75 additions & 0 deletions src/test/mir-opt/issue_101867.main.mir_map.0.mir
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
// MIR for `main` 0 mir_map

| User Type Annotations
| 0: user_ty: Canonical { max_universe: U0, variables: [], value: Ty(std::option::Option<u8>) }, span: $DIR/issue-101867.rs:5:12: 5:22, inferred_ty: std::option::Option<u8>
| 1: user_ty: Canonical { max_universe: U0, variables: [], value: Ty(std::option::Option<u8>) }, span: $DIR/issue-101867.rs:5:12: 5:22, inferred_ty: std::option::Option<u8>
|
fn main() -> () {
let mut _0: (); // return place in scope 0 at $DIR/issue-101867.rs:+0:11: +0:11
let _1: std::option::Option<u8> as UserTypeProjection { base: UserType(0), projs: [] }; // in scope 0 at $DIR/issue-101867.rs:+1:9: +1:10
let mut _2: !; // in scope 0 at $DIR/issue-101867.rs:+2:26: +4:6
let _3: (); // in scope 0 at $SRC_DIR/std/src/panic.rs:LL:COL
let mut _4: !; // in scope 0 at $SRC_DIR/std/src/panic.rs:LL:COL
let mut _6: isize; // in scope 0 at $DIR/issue-101867.rs:+2:9: +2:16
scope 1 {
debug x => _1; // in scope 1 at $DIR/issue-101867.rs:+1:9: +1:10
let _5: u8; // in scope 1 at $DIR/issue-101867.rs:+2:14: +2:15
scope 2 {
debug y => _5; // in scope 2 at $DIR/issue-101867.rs:+2:14: +2:15
}
}

bb0: {
StorageLive(_1); // scope 0 at $DIR/issue-101867.rs:+1:9: +1:10
_1 = Option::<u8>::Some(const 1_u8); // scope 0 at $DIR/issue-101867.rs:+1:25: +1:32
FakeRead(ForLet(None), _1); // scope 0 at $DIR/issue-101867.rs:+1:9: +1:10
AscribeUserType(_1, o, UserTypeProjection { base: UserType(1), projs: [] }); // scope 0 at $DIR/issue-101867.rs:+1:12: +1:22
StorageLive(_5); // scope 1 at $DIR/issue-101867.rs:+2:14: +2:15
FakeRead(ForMatchedPlace(None), _1); // scope 1 at $DIR/issue-101867.rs:+2:19: +2:20
_6 = discriminant(_1); // scope 1 at $DIR/issue-101867.rs:+2:19: +2:20
switchInt(move _6) -> [1_isize: bb4, otherwise: bb3]; // scope 1 at $DIR/issue-101867.rs:+2:9: +2:16
}

bb1: {
StorageLive(_3); // scope 1 at $SRC_DIR/std/src/panic.rs:LL:COL
StorageLive(_4); // scope 1 at $SRC_DIR/std/src/panic.rs:LL:COL
_4 = begin_panic::<&str>(const "explicit panic") -> bb7; // scope 1 at $SRC_DIR/std/src/panic.rs:LL:COL
// mir::Constant
// + span: $SRC_DIR/std/src/panic.rs:LL:COL
// + literal: Const { ty: fn(&str) -> ! {begin_panic::<&str>}, val: Value(<ZST>) }
// mir::Constant
// + span: $SRC_DIR/std/src/panic.rs:LL:COL
// + literal: Const { ty: &str, val: Value(Slice(..)) }
}

bb2: {
StorageDead(_4); // scope 1 at $SRC_DIR/std/src/panic.rs:LL:COL
StorageDead(_3); // scope 1 at $DIR/issue-101867.rs:+3:16: +3:17
unreachable; // scope 1 at $DIR/issue-101867.rs:+2:26: +4:6
}

bb3: {
goto -> bb6; // scope 1 at $DIR/issue-101867.rs:+2:19: +2:20
}

bb4: {
falseEdge -> [real: bb5, imaginary: bb3]; // scope 1 at $DIR/issue-101867.rs:+2:9: +2:16
}

bb5: {
_5 = ((_1 as Some).0: u8); // scope 1 at $DIR/issue-101867.rs:+2:14: +2:15
_0 = const (); // scope 0 at $DIR/issue-101867.rs:+0:11: +5:2
StorageDead(_5); // scope 1 at $DIR/issue-101867.rs:+5:1: +5:2
StorageDead(_1); // scope 0 at $DIR/issue-101867.rs:+5:1: +5:2
return; // scope 0 at $DIR/issue-101867.rs:+5:2: +5:2
}

bb6: {
StorageDead(_5); // scope 1 at $DIR/issue-101867.rs:+5:1: +5:2
goto -> bb1; // scope 0 at $DIR/issue-101867.rs:+0:11: +5:2
}

bb7 (cleanup): {
resume; // scope 0 at $DIR/issue-101867.rs:+0:1: +5:2
}
}
19 changes: 19 additions & 0 deletions src/test/ui/let-else/const-fn.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// run-pass
// issue #101932

#![cfg_attr(bootstrap, feature(let_else))]

const fn foo(a: Option<i32>) -> i32 {
let Some(a) = a else {
return 42
};

a + 1
}

fn main() {
const A: i32 = foo(None);
const B: i32 = foo(Some(1));

println!("{} {}", A, B);
}

0 comments on commit 2019147

Please sign in to comment.