Skip to content

Commit

Permalink
Auto merge of #88572 - matthewjasper:if-let-scoping-fix, r=oli-obk
Browse files Browse the repository at this point in the history
Fix drop handling for `if let` expressions

MIR lowering for `if let` expressions is now more complicated now that
`if let` exists in HIR. This PR adds a scope for the variables bound in
an `if let` expression and then uses an approach similar to how we
handle loops to ensure that we reliably drop the correct variables.

Closes #88307
cc `@flip1995` `@richkadel` `@c410-f3r`
  • Loading branch information
bors committed Sep 3, 2021
2 parents 4878034 + 4e2fd4f commit b7404c8
Show file tree
Hide file tree
Showing 62 changed files with 596 additions and 562 deletions.
1 change: 1 addition & 0 deletions compiler/rustc_hir/src/hir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1177,6 +1177,7 @@ pub struct Arm<'hir> {
#[derive(Debug, HashStable_Generic)]
pub enum Guard<'hir> {
If(&'hir Expr<'hir>),
// FIXME use ExprKind::Let for this.
IfLet(&'hir Pat<'hir>, &'hir Expr<'hir>),
}

Expand Down
5 changes: 5 additions & 0 deletions compiler/rustc_middle/src/middle/region.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ impl fmt::Debug for Scope {
ScopeData::CallSite => write!(fmt, "CallSite({:?})", self.id),
ScopeData::Arguments => write!(fmt, "Arguments({:?})", self.id),
ScopeData::Destruction => write!(fmt, "Destruction({:?})", self.id),
ScopeData::IfThen => write!(fmt, "IfThen({:?})", self.id),
ScopeData::Remainder(fsi) => write!(
fmt,
"Remainder {{ block: {:?}, first_statement_index: {}}}",
Expand All @@ -120,6 +121,10 @@ pub enum ScopeData {
/// Scope of destructors for temporaries of node-id.
Destruction,

/// Scope of the condition and then block of an if expression
/// Used for variables introduced in an if-let expression.
IfThen,

/// Scope following a `let id = expr;` binding in a block.
Remainder(FirstStatementIndex),
}
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_middle/src/thir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ pub enum ExprKind<'tcx> {
},
/// An `if` expression.
If {
if_then_scope: region::Scope,
cond: ExprId,
then: ExprId,
else_opt: Option<ExprId>,
Expand Down
40 changes: 32 additions & 8 deletions compiler/rustc_mir_build/src/build/expr/into.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,11 +52,33 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
ExprKind::Match { scrutinee, ref arms } => {
this.match_expr(destination, expr_span, block, &this.thir[scrutinee], arms)
}
ExprKind::If { cond, then, else_opt } => {
let local_scope = this.local_scope();
let (mut then_blk, mut else_blk) =
this.then_else_blocks(block, &this.thir[cond], local_scope, source_info);
unpack!(then_blk = this.expr_into_dest(destination, then_blk, &this.thir[then]));
ExprKind::If { cond, then, else_opt, if_then_scope } => {
let then_blk;
let then_expr = &this.thir[then];
let then_source_info = this.source_info(then_expr.span);
let condition_scope = this.local_scope();

let mut else_blk = unpack!(
then_blk = this.in_scope(
(if_then_scope, then_source_info),
LintLevel::Inherited,
|this| {
let (then_block, else_block) =
this.in_if_then_scope(condition_scope, |this| {
let then_blk = unpack!(this.then_else_break(
block,
&this.thir[cond],
condition_scope,
condition_scope,
then_expr.span,
));
this.expr_into_dest(destination, then_blk, then_expr)
});
then_block.and(else_block)
},
)
);

else_blk = if let Some(else_opt) = else_opt {
unpack!(this.expr_into_dest(destination, else_blk, &this.thir[else_opt]))
} else {
Expand All @@ -81,9 +103,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {

join_block.unit()
}
ExprKind::Let { ref pat, expr } => {
let (true_block, false_block) =
this.lower_let(block, &this.thir[expr], pat, expr_span);
ExprKind::Let { expr, ref pat } => {
let scope = this.local_scope();
let (true_block, false_block) = this.in_if_then_scope(scope, |this| {
this.lower_let_expr(block, &this.thir[expr], pat, scope, expr_span)
});

let join_block = this.cfg.start_new_block();

Expand Down
96 changes: 60 additions & 36 deletions compiler/rustc_mir_build/src/build/matches/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,42 +35,48 @@ use std::convert::TryFrom;
use std::mem;

impl<'a, 'tcx> Builder<'a, 'tcx> {
pub(crate) fn then_else_blocks(
pub(crate) fn then_else_break(
&mut self,
mut block: BasicBlock,
expr: &Expr<'tcx>,
scope: region::Scope,
source_info: SourceInfo,
) -> (BasicBlock, BasicBlock) {
temp_scope: region::Scope,
break_scope: region::Scope,
variable_scope_span: Span,
) -> BlockAnd<()> {
let this = self;
let expr_span = expr.span;

match expr.kind {
ExprKind::Scope { region_scope, lint_level, value } => {
let region_scope = (region_scope, source_info);
let then_block;
let else_block = unpack!(
then_block = this.in_scope(region_scope, lint_level, |this| {
let (then_block, else_block) =
this.then_else_blocks(block, &this.thir[value], scope, source_info);
then_block.and(else_block)
})
);
(then_block, else_block)
let region_scope = (region_scope, this.source_info(expr_span));
this.in_scope(region_scope, lint_level, |this| {
this.then_else_break(
block,
&this.thir[value],
temp_scope,
break_scope,
variable_scope_span,
)
})
}
ExprKind::Let { expr, ref pat } => {
// FIXME: Use correct span.
this.lower_let(block, &this.thir[expr], pat, expr_span)
this.lower_let_expr(block, &this.thir[expr], pat, break_scope, variable_scope_span)
}
_ => {
let mutability = Mutability::Mut;
let place = unpack!(block = this.as_temp(block, Some(scope), expr, mutability));
let place =
unpack!(block = this.as_temp(block, Some(temp_scope), expr, mutability));
let operand = Operand::Move(Place::from(place));

let then_block = this.cfg.start_new_block();
let else_block = this.cfg.start_new_block();
let term = TerminatorKind::if_(this.tcx, operand, then_block, else_block);

let source_info = this.source_info(expr_span);
this.cfg.terminate(block, source_info, term);
(then_block, else_block)
this.break_for_else(else_block, break_scope, source_info);

then_block.unit()
}
}
}
Expand Down Expand Up @@ -302,6 +308,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {

let arm_source_info = self.source_info(arm.span);
let arm_scope = (arm.scope, arm_source_info);
let match_scope = self.local_scope();
self.in_scope(arm_scope, arm.lint_level, |this| {
// `try_upvars_resolved` may fail if it is unable to resolve the given
// `PlaceBuilder` inside a closure. In this case, we don't want to include
Expand Down Expand Up @@ -340,6 +347,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
scrutinee_span,
Some(arm.span),
Some(arm.scope),
Some(match_scope),
);

if let Some(source_scope) = scope {
Expand Down Expand Up @@ -384,6 +392,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
scrutinee_span: Span,
arm_span: Option<Span>,
arm_scope: Option<region::Scope>,
match_scope: Option<region::Scope>,
) -> BasicBlock {
if candidate.subcandidates.is_empty() {
// Avoid generating another `BasicBlock` when we only have one
Expand All @@ -395,6 +404,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
fake_borrow_temps,
scrutinee_span,
arm_span,
match_scope,
true,
)
} else {
Expand Down Expand Up @@ -431,6 +441,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
&fake_borrow_temps,
scrutinee_span,
arm_span,
match_scope,
schedule_drops,
);
if arm_scope.is_none() {
Expand Down Expand Up @@ -616,6 +627,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
irrefutable_pat.span,
None,
None,
None,
)
.unit()
}
Expand Down Expand Up @@ -1742,13 +1754,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
// Pat binding - used for `let` and function parameters as well.

impl<'a, 'tcx> Builder<'a, 'tcx> {
pub fn lower_let(
crate fn lower_let_expr(
&mut self,
mut block: BasicBlock,
expr: &Expr<'tcx>,
pat: &Pat<'tcx>,
else_target: region::Scope,
span: Span,
) -> (BasicBlock, BasicBlock) {
) -> BlockAnd<()> {
let expr_span = expr.span;
let expr_place_builder = unpack!(block = self.lower_scrutinee(block, expr, expr_span));
let mut guard_candidate = Candidate::new(expr_place_builder.clone(), &pat, false);
Expand All @@ -1769,6 +1782,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
expr_place = expr_builder.into_place(self.tcx, self.typeck_results);
opt_expr_place = Some((Some(&expr_place), expr_span));
}
let otherwise_post_guard_block = otherwise_candidate.pre_binding_block.unwrap();
self.break_for_else(otherwise_post_guard_block, else_target, self.source_info(expr_span));

self.declare_bindings(None, pat.span.to(span), pat, ArmHasGuard(false), opt_expr_place);
let post_guard_block = self.bind_pattern(
self.source_info(pat.span),
Expand All @@ -1778,9 +1794,10 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
expr.span,
None,
None,
None,
);
let otherwise_post_guard_block = otherwise_candidate.pre_binding_block.unwrap();
(post_guard_block, otherwise_post_guard_block)

post_guard_block.unit()
}

/// Initializes each of the bindings from the candidate by
Expand All @@ -1799,6 +1816,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
fake_borrows: &Vec<(Place<'tcx>, Local)>,
scrutinee_span: Span,
arm_span: Option<Span>,
match_scope: Option<region::Scope>,
schedule_drops: bool,
) -> BasicBlock {
debug!("bind_and_guard_matched_candidate(candidate={:?})", candidate);
Expand Down Expand Up @@ -1929,17 +1947,25 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
self.cfg.push_assign(block, scrutinee_source_info, Place::from(temp), borrow);
}

let (guard_span, (post_guard_block, otherwise_post_guard_block)) = match *guard {
Guard::If(e) => {
let e = &self.thir[e];
let source_info = self.source_info(e.span);
(e.span, self.test_bool(block, e, source_info))
}
Guard::IfLet(ref pat, scrutinee) => {
let s = &self.thir[scrutinee];
(s.span, self.lower_let(block, s, pat, arm_span.unwrap()))
}
};
let arm_span = arm_span.unwrap();
let arm_scope = self.local_scope();
let match_scope = match_scope.unwrap();
let mut guard_span = rustc_span::DUMMY_SP;

let (post_guard_block, otherwise_post_guard_block) =
self.in_if_then_scope(match_scope, |this| match *guard {
Guard::If(e) => {
let e = &this.thir[e];
guard_span = e.span;
this.then_else_break(block, e, arm_scope, match_scope, arm_span)
}
Guard::IfLet(ref pat, scrutinee) => {
let s = &this.thir[scrutinee];
guard_span = s.span;
this.lower_let_expr(block, s, pat, match_scope, arm_span)
}
});

let source_info = self.source_info(guard_span);
let guard_end = self.source_info(tcx.sess.source_map().end_point(guard_span));
let guard_frame = self.guard_context.pop().unwrap();
Expand All @@ -1955,10 +1981,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
self.cfg.terminate(unreachable, source_info, TerminatorKind::Unreachable);
unreachable
});
let outside_scope = self.cfg.start_new_block();
self.exit_top_scope(otherwise_post_guard_block, outside_scope, source_info);
self.false_edges(
outside_scope,
otherwise_post_guard_block,
otherwise_block,
candidate.next_candidate_pre_binding_block,
source_info,
Expand Down
Loading

0 comments on commit b7404c8

Please sign in to comment.