Skip to content

Commit

Permalink
Extract an arguments struct for Builder::then_else_break
Browse files Browse the repository at this point in the history 
Most of this method's arguments are usually or always forwarded as-is to
recursive invocations.

Wrapping them in a dedicated struct allows us to document each struct field,
and lets us use struct-update syntax to indicate which arguments are being
modified when making a recursive call.
  • Loading branch information
@Zalathar
Zalathar committed Mar 4, 2024
1 parent da02fff commit 4146136
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 66 deletions.
8 changes: 5 additions & 3 deletions compiler/rustc_mir_build/src/build/expr/into.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,10 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let then_blk = unpack!(this.then_else_break(
block,
cond,
Some(condition_scope),
Some(condition_scope), // Temp scope
condition_scope,
source_info,
true,
true, // Declare `let` bindings normally
));

this.expr_into_dest(destination, then_blk, then)
Expand Down Expand Up @@ -146,9 +146,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
this.then_else_break(
block,
lhs,
Some(condition_scope),
Some(condition_scope), // Temp scope
condition_scope,
source_info,
// This flag controls how inner `let` expressions are lowered,
// but either way there shouldn't be any of those in here.
true,
)
});
Expand Down
130 changes: 67 additions & 63 deletions compiler/rustc_mir_build/src/build/matches/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,76 +30,92 @@ mod util;
use std::borrow::Borrow;
use std::mem;

/// Arguments to [`Builder::then_else_break_inner`] that are usually forwarded
/// to recursive invocations.
#[derive(Clone, Copy)]
struct ThenElseArgs {
/// Used as the temp scope for lowering `expr`. If absent (for match guards),
/// `self.local_scope()` is used.
temp_scope_override: Option<region::Scope>,
/// Scope to pass to [`Builder::break_for_else`]. Must match the scope used
/// by the enclosing call to [`Builder::in_if_then_scope`].
break_scope: region::Scope,
variable_source_info: SourceInfo,
/// Forwarded to [`Builder::lower_let_expr`] when lowering [`ExprKind::Let`].
/// When false (for match guards), `let` bindings won't be declared.
declare_let_bindings: bool,
}

impl<'a, 'tcx> Builder<'a, 'tcx> {
/// Lowers a condition in a way that ensures that variables bound in any let
/// expressions are definitely initialized in the if body.
///
/// If `declare_bindings` is false then variables created in `let`
/// If `declare_let_bindings` is false then variables created in `let`
/// expressions will not be declared. This is for if let guards on arms with
/// an or pattern, where the guard is lowered multiple times.
pub(crate) fn then_else_break(
&mut self,
mut block: BasicBlock,
block: BasicBlock,
expr_id: ExprId,
temp_scope_override: Option<region::Scope>,
break_scope: region::Scope,
variable_source_info: SourceInfo,
declare_bindings: bool,
declare_let_bindings: bool,
) -> BlockAnd<()> {
self.then_else_break_inner(
block,
expr_id,
ThenElseArgs {
temp_scope_override,
break_scope,
variable_source_info,
declare_let_bindings,
},
)
}

fn then_else_break_inner(
&mut self,
block: BasicBlock, // Block that the condition and branch will be lowered into
expr_id: ExprId, // Condition expression to lower
args: ThenElseArgs,
) -> BlockAnd<()> {
let this = self;
let expr = &this.thir[expr_id];
let expr_span = expr.span;

match expr.kind {
ExprKind::LogicalOp { op: LogicalOp::And, lhs, rhs } => {
let lhs_then_block = unpack!(this.then_else_break(
block,
lhs,
temp_scope_override,
break_scope,
variable_source_info,
declare_bindings,
));

let rhs_then_block = unpack!(this.then_else_break(
lhs_then_block,
rhs,
temp_scope_override,
break_scope,
variable_source_info,
declare_bindings,
));

let lhs_then_block = unpack!(this.then_else_break_inner(block, lhs, args));
let rhs_then_block = unpack!(this.then_else_break_inner(lhs_then_block, rhs, args));
rhs_then_block.unit()
}
ExprKind::LogicalOp { op: LogicalOp::Or, lhs, rhs } => {
let local_scope = this.local_scope();
let (lhs_success_block, failure_block) =
this.in_if_then_scope(local_scope, expr_span, |this| {
this.then_else_break(
this.then_else_break_inner(
block,
lhs,
temp_scope_override,
local_scope,
variable_source_info,
true,
ThenElseArgs {
break_scope: local_scope,
declare_let_bindings: true,
..args
},
)
});
let rhs_success_block = unpack!(this.then_else_break(
let rhs_success_block = unpack!(this.then_else_break_inner(
failure_block,
rhs,
temp_scope_override,
break_scope,
variable_source_info,
true,
ThenElseArgs { declare_let_bindings: true, ..args },
));

// Make the LHS and RHS success arms converge to a common block.
// (We can't just make LHS goto RHS, because `rhs_success_block`
// might contain statements that we don't want on the LHS path.)
let success_block = this.cfg.start_new_block();
this.cfg.goto(lhs_success_block, variable_source_info, success_block);
this.cfg.goto(rhs_success_block, variable_source_info, success_block);
this.cfg.goto(lhs_success_block, args.variable_source_info, success_block);
this.cfg.goto(rhs_success_block, args.variable_source_info, success_block);
success_block.unit()
}
ExprKind::Unary { op: UnOp::Not, arg } => {
Expand All @@ -111,50 +127,38 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
if this.tcx.sess.instrument_coverage() {
this.cfg.push_coverage_span_marker(block, this.source_info(expr_span));
}
this.then_else_break(
this.then_else_break_inner(
block,
arg,
temp_scope_override,
local_scope,
variable_source_info,
true,
ThenElseArgs {
break_scope: local_scope,
declare_let_bindings: true,
..args
},
)
});
this.break_for_else(success_block, break_scope, variable_source_info);
this.break_for_else(success_block, args.break_scope, args.variable_source_info);
failure_block.unit()
}
ExprKind::Scope { region_scope, lint_level, value } => {
let region_scope = (region_scope, this.source_info(expr_span));
this.in_scope(region_scope, lint_level, |this| {
this.then_else_break(
block,
value,
temp_scope_override,
break_scope,
variable_source_info,
declare_bindings,
)
this.then_else_break_inner(block, value, args)
})
}
ExprKind::Use { source } => this.then_else_break(
block,
source,
temp_scope_override,
break_scope,
variable_source_info,
declare_bindings,
),
ExprKind::Use { source } => this.then_else_break_inner(block, source, args),
ExprKind::Let { expr, ref pat } => this.lower_let_expr(
block,
expr,
pat,
break_scope,
Some(variable_source_info.scope),
variable_source_info.span,
declare_bindings,
args.break_scope,
Some(args.variable_source_info.scope),
args.variable_source_info.span,
args.declare_let_bindings,
),
_ => {
let temp_scope = temp_scope_override.unwrap_or_else(|| this.local_scope());
let mut block = block;
let temp_scope = args.temp_scope_override.unwrap_or_else(|| this.local_scope());
let mutability = Mutability::Mut;
let place =
unpack!(block = this.as_temp(block, Some(temp_scope), expr_id, mutability));
Expand All @@ -166,7 +170,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {

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

then_block.unit()
}
Expand Down Expand Up @@ -2105,10 +2109,10 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
this.then_else_break(
block,
guard,
None,
None, // Use `self.local_scope()` as the temp scope
match_scope,
this.source_info(arm.span),
false,
false, // For guards, `let` bindings are declared separately
)
});

Expand Down

0 comments on commit 4146136

Please sign in to comment.