Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix drop handling for if let expressions #88572

Merged
merged 4 commits into from
Sep 3, 2021
Merged

Fix drop handling for if let expressions #88572

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ff8c0ef
Fix drop handling for `if let` expressions
matthewjasper Sep 1, 2021
fd5b40f
Remove TODO
matthewjasper Sep 2, 2021
9366dfd
Bless 32bit MIR opt tests
matthewjasper Sep 2, 2021
4e2fd4f
Update coverage tests
matthewjasper Sep 3, 2021
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
1 change: 1 addition & 0 deletions compiler/rustc_hir/src/hir.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1187,6 +1187,7 @@ pub struct Arm<'hir> {
#[derive(Debug, HashStable_Generic)]
pub enum Guard<'hir> {
If(&'hir Expr<'hir>),
// FIXME use ExprKind::Let for this.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I can take care of this in a following PR

IfLet(&'hir Pat<'hir>, &'hir Expr<'hir>),
}

Expand Down
5 changes: 5 additions & 0 deletions compiler/rustc_middle/src/middle/region.rs
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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