bjoernager/rust
bjoernager/rust
1
Fork 0
Code Activity

Rollup merge of #126981 - Zalathar:enums, r=Nadrieril

Browse source 
Replace some magic booleans in match-lowering with enums

This PR takes some boolean arguments used by the match-lowering code, and replaces them with dedicated enums that more clearly express their effect, while also making it much easier to find how each value is ultimately used.
This commit is contained in:
Matthias Krüger 2024-06-30 18:25:32 +02:00 • committed by GitHub
commit d404ce7015
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 150 additions and 52 deletions
Download patch file Download diff file Expand all files Collapse all files

21
compiler/rustc_mir_build/src/build/block.rs
View file

@ -1,3 +1,4 @@
use crate::build::matches::{DeclareLetBindings, EmitStorageLive, ScheduleDrops};
use crate::build::ForGuard::OutsideGuard; use crate::build::ForGuard::OutsideGuard;
use crate::build::{BlockAnd, BlockAndExtension, BlockFrame, Builder}; use crate::build::{BlockAnd, BlockAndExtension, BlockFrame, Builder};
use rustc_middle::middle::region::Scope; use rustc_middle::middle::region::Scope;
@ -201,7 +202,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
pattern, pattern,
UserTypeProjections::none(), UserTypeProjections::none(),
&mut |this, _, _, node, span, _, _| { &mut |this, _, _, node, span, _, _| {
this.storage_live_binding(block, node, span, OutsideGuard, true); this.storage_live_binding(
block,
node,
span,
OutsideGuard,
ScheduleDrops::Yes,
);
}, },
); );
let else_block_span = this.thir[*else_block].span; let else_block_span = this.thir[*else_block].span;
@ -213,8 +220,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
pattern, pattern,
None, None,
initializer_span, initializer_span,
false, DeclareLetBindings::No,
true, EmitStorageLive::No,
) )
}); });
matching.and(failure) matching.and(failure)
@ -291,7 +298,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
pattern, pattern,
UserTypeProjections::none(), UserTypeProjections::none(),
&mut |this, _, _, node, span, _, _| { &mut |this, _, _, node, span, _, _| {
this.storage_live_binding(block, node, span, OutsideGuard, true); this.storage_live_binding(
block,
node,
span,
OutsideGuard,
ScheduleDrops::Yes,
);
this.schedule_drop_for_binding(node, span, OutsideGuard); this.schedule_drop_for_binding(node, span, OutsideGuard);
}, },
) )

5
compiler/rustc_mir_build/src/build/expr/into.rs
View file

@ -1,6 +1,7 @@
//! See docs in build/expr/mod.rs //! See docs in build/expr/mod.rs
use crate::build::expr::category::{Category, RvalueFunc}; use crate::build::expr::category::{Category, RvalueFunc};
use crate::build::matches::DeclareLetBindings;
use crate::build::{BlockAnd, BlockAndExtension, BlockFrame, Builder, NeedsTemporary}; use crate::build::{BlockAnd, BlockAndExtension, BlockFrame, Builder, NeedsTemporary};
use rustc_ast::InlineAsmOptions; use rustc_ast::InlineAsmOptions;
use rustc_data_structures::fx::FxHashMap; use rustc_data_structures::fx::FxHashMap;
@ -86,7 +87,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
cond, cond,
Some(condition_scope), // Temp scope Some(condition_scope), // Temp scope
source_info, source_info,
true, // Declare `let` bindings normally DeclareLetBindings::Yes, // Declare `let` bindings normally
)); ));
// Lower the `then` arm into its block. // Lower the `then` arm into its block.
@ -163,7 +164,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
source_info, source_info,
// This flag controls how inner `let` expressions are lowered, // This flag controls how inner `let` expressions are lowered,
// but either way there shouldn't be any of those in here. // but either way there shouldn't be any of those in here.
true, DeclareLetBindings::LetNotPermitted,
) )
}); });
let (short_circuit, continuation, constant) = match op { let (short_circuit, continuation, constant) = match op {

176
compiler/rustc_mir_build/src/build/matches/mod.rs
View file

@ -28,6 +28,7 @@ mod simplify;
mod test; mod test;
mod util; mod util;
use std::assert_matches::assert_matches;
use std::borrow::Borrow; use std::borrow::Borrow;
use std::mem; use std::mem;
@ -39,9 +40,50 @@ struct ThenElseArgs {
/// `self.local_scope()` is used. /// `self.local_scope()` is used.
temp_scope_override: Option<region::Scope>, temp_scope_override: Option<region::Scope>,
variable_source_info: SourceInfo, variable_source_info: SourceInfo,
/// Determines how bindings should be handled when lowering `let` expressions.
///
/// Forwarded to [`Builder::lower_let_expr`] when lowering [`ExprKind::Let`]. /// Forwarded to [`Builder::lower_let_expr`] when lowering [`ExprKind::Let`].
/// When false (for match guards), `let` bindings won't be declared. declare_let_bindings: DeclareLetBindings,
declare_let_bindings: bool, }
/// Should lowering a `let` expression also declare its bindings?
///
/// Used by [`Builder::lower_let_expr`] when lowering [`ExprKind::Let`].
#[derive(Clone, Copy)]
pub(crate) enum DeclareLetBindings {
/// Yes, declare `let` bindings as normal for `if` conditions.
Yes,
/// No, don't declare `let` bindings, because the caller declares them
/// separately due to special requirements.
///
/// Used for match guards and let-else.
No,
/// Let expressions are not permitted in this context, so it is a bug to
/// try to lower one (e.g inside lazy-boolean-or or boolean-not).
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)]
pub(crate) enum ScheduleDrops {
/// Yes, the relevant functions should also schedule drops as appropriate.
Yes,
/// No, don't schedule drops. The caller has taken responsibility for any
/// appropriate drops.
No,
} }
impl<'a, 'tcx> Builder<'a, 'tcx> { impl<'a, 'tcx> Builder<'a, 'tcx> {
@ -57,7 +99,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
expr_id: ExprId, expr_id: ExprId,
temp_scope_override: Option<region::Scope>, temp_scope_override: Option<region::Scope>,
variable_source_info: SourceInfo, variable_source_info: SourceInfo,
declare_let_bindings: bool, declare_let_bindings: DeclareLetBindings,
) -> BlockAnd<()> { ) -> BlockAnd<()> {
self.then_else_break_inner( self.then_else_break_inner(
block, block,
@ -91,13 +133,19 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
this.then_else_break_inner( this.then_else_break_inner(
block, block,
lhs, lhs,
ThenElseArgs { declare_let_bindings: true, ..args }, ThenElseArgs {
declare_let_bindings: DeclareLetBindings::LetNotPermitted,
..args
},
) )
}); });
let rhs_success_block = unpack!(this.then_else_break_inner( let rhs_success_block = unpack!(this.then_else_break_inner(
failure_block, failure_block,
rhs, rhs,
ThenElseArgs { declare_let_bindings: true, ..args }, ThenElseArgs {
declare_let_bindings: DeclareLetBindings::LetNotPermitted,
..args
},
)); ));
// Make the LHS and RHS success arms converge to a common block. // Make the LHS and RHS success arms converge to a common block.
@ -127,7 +175,10 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
this.then_else_break_inner( this.then_else_break_inner(
block, block,
arg, arg,
ThenElseArgs { declare_let_bindings: true, ..args }, ThenElseArgs {
declare_let_bindings: DeclareLetBindings::LetNotPermitted,
..args
},
) )
}); });
this.break_for_else(success_block, args.variable_source_info); this.break_for_else(success_block, args.variable_source_info);
@ -147,7 +198,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
Some(args.variable_source_info.scope), Some(args.variable_source_info.scope),
args.variable_source_info.span, args.variable_source_info.span,
args.declare_let_bindings, args.declare_let_bindings,
false, EmitStorageLive::Yes,
), ),
_ => { _ => {
let mut block = block; let mut block = block;
@ -440,7 +491,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
&fake_borrow_temps, &fake_borrow_temps,
scrutinee_span, scrutinee_span,
Some((arm, match_scope)), Some((arm, match_scope)),
false, EmitStorageLive::Yes,
); );
this.fixed_temps_scope = old_dedup_scope; this.fixed_temps_scope = old_dedup_scope;
@ -485,7 +536,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
fake_borrow_temps: &[(Place<'tcx>, Local, FakeBorrowKind)], fake_borrow_temps: &[(Place<'tcx>, Local, FakeBorrowKind)],
scrutinee_span: Span, scrutinee_span: Span,
arm_match_scope: Option<(&Arm<'tcx>, region::Scope)>, arm_match_scope: Option<(&Arm<'tcx>, region::Scope)>,
storages_alive: bool, emit_storage_live: EmitStorageLive,
) -> BasicBlock { ) -> BasicBlock {
if candidate.subcandidates.is_empty() { if candidate.subcandidates.is_empty() {
// Avoid generating another `BasicBlock` when we only have one // Avoid generating another `BasicBlock` when we only have one
@ -496,8 +547,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
fake_borrow_temps, fake_borrow_temps,
scrutinee_span, scrutinee_span,
arm_match_scope, arm_match_scope,
true, ScheduleDrops::Yes,
storages_alive, emit_storage_live,
) )
} else { } else {
// It's helpful to avoid scheduling drops multiple times to save // It's helpful to avoid scheduling drops multiple times to save
@ -515,7 +566,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
// To handle this we instead unschedule it's drop after each time // To handle this we instead unschedule it's drop after each time
// we lower the guard. // we lower the guard.
let target_block = self.cfg.start_new_block(); let target_block = self.cfg.start_new_block();
let mut schedule_drops = true; let mut schedule_drops = ScheduleDrops::Yes;
let arm = arm_match_scope.unzip().0; let arm = arm_match_scope.unzip().0;
// We keep a stack of all of the bindings and type ascriptions // We keep a stack of all of the bindings and type ascriptions
// from the parent candidates that we visit, that also need to // from the parent candidates that we visit, that also need to
@ -534,10 +585,10 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
scrutinee_span, scrutinee_span,
arm_match_scope, arm_match_scope,
schedule_drops, schedule_drops,
storages_alive, emit_storage_live,
); );
if arm.is_none() { if arm.is_none() {
schedule_drops = false; schedule_drops = ScheduleDrops::No;
} }
self.cfg.goto(binding_end, outer_source_info, target_block); self.cfg.goto(binding_end, outer_source_info, target_block);
}, },
@ -563,8 +614,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
match irrefutable_pat.kind { match irrefutable_pat.kind {
// Optimize the case of `let x = ...` to write directly into `x` // Optimize the case of `let x = ...` to write directly into `x`
PatKind::Binding { mode: BindingMode(ByRef::No, _), var, subpattern: None, .. } => { PatKind::Binding { mode: BindingMode(ByRef::No, _), var, subpattern: None, .. } => {
let place = let place = self.storage_live_binding(
self.storage_live_binding(block, var, irrefutable_pat.span, OutsideGuard, true); block,
var,
irrefutable_pat.span,
OutsideGuard,
ScheduleDrops::Yes,
);
unpack!(block = self.expr_into_dest(place, block, initializer_id)); unpack!(block = self.expr_into_dest(place, block, initializer_id));
// Inject a fake read, see comments on `FakeReadCause::ForLet`. // Inject a fake read, see comments on `FakeReadCause::ForLet`.
@ -597,8 +653,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
}, },
ascription: thir::Ascription { ref annotation, variance: _ }, ascription: thir::Ascription { ref annotation, variance: _ },
} => { } => {
let place = let place = self.storage_live_binding(
self.storage_live_binding(block, var, irrefutable_pat.span, OutsideGuard, true); block,
var,
irrefutable_pat.span,
OutsideGuard,
ScheduleDrops::Yes,
);
unpack!(block = self.expr_into_dest(place, block, initializer_id)); unpack!(block = self.expr_into_dest(place, block, initializer_id));
// Inject a fake read, see comments on `FakeReadCause::ForLet`. // Inject a fake read, see comments on `FakeReadCause::ForLet`.
@ -704,7 +765,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
&[], &[],
irrefutable_pat.span, irrefutable_pat.span,
None, None,
false, EmitStorageLive::Yes,
) )
.unit() .unit()
} }
@ -780,13 +841,15 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
} }
} }
/// Emits a [`StatementKind::StorageLive`] for the given var, and also
/// schedules a drop if requested (and possible).
pub(crate) fn storage_live_binding( pub(crate) fn storage_live_binding(
&mut self, &mut self,
block: BasicBlock, block: BasicBlock,
var: LocalVarId, var: LocalVarId,
span: Span, span: Span,
for_guard: ForGuard, for_guard: ForGuard,
schedule_drop: bool, schedule_drop: ScheduleDrops,
) -> Place<'tcx> { ) -> Place<'tcx> {
let local_id = self.var_local_id(var, for_guard); let local_id = self.var_local_id(var, for_guard);
let source_info = self.source_info(span); let source_info = self.source_info(span);
@ -794,7 +857,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
// Although there is almost always scope for given variable in corner cases // Although there is almost always scope for given variable in corner cases
// like #92893 we might get variable with no scope. // like #92893 we might get variable with no scope.
if let Some(region_scope) = self.region_scope_tree.var_scope(var.0.local_id) if let Some(region_scope) = self.region_scope_tree.var_scope(var.0.local_id)
&& schedule_drop && matches!(schedule_drop, ScheduleDrops::Yes)
{ {
self.schedule_drop(span, region_scope, local_id, DropKind::Storage); self.schedule_drop(span, region_scope, local_id, DropKind::Storage);
} }
@ -1991,8 +2054,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
// Pat binding - used for `let` and function parameters as well. // Pat binding - used for `let` and function parameters as well.
impl<'a, 'tcx> Builder<'a, 'tcx> { impl<'a, 'tcx> Builder<'a, 'tcx> {
/// If the bindings have already been declared, set `declare_bindings` to /// Lowers a `let` expression that appears in a suitable context
/// `false` to avoid duplicated bindings declaration; used for if-let guards. /// (e.g. an `if` condition or match guard).
///
/// Also used for lowering let-else statements, since they have similar
/// needs despite not actually using `let` expressions.
///
/// Use [`DeclareLetBindings`] to control whether the `let` bindings are
/// declared or not.
pub(crate) fn lower_let_expr( pub(crate) fn lower_let_expr(
&mut self, &mut self,
mut block: BasicBlock, mut block: BasicBlock,
@ -2000,8 +2069,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
pat: &Pat<'tcx>, pat: &Pat<'tcx>,
source_scope: Option<SourceScope>, source_scope: Option<SourceScope>,
scope_span: Span, scope_span: Span,
declare_bindings: bool, declare_let_bindings: DeclareLetBindings,
storages_alive: bool, emit_storage_live: EmitStorageLive,
) -> BlockAnd<()> { ) -> BlockAnd<()> {
let expr_span = self.thir[expr_id].span; let expr_span = self.thir[expr_id].span;
let scrutinee = unpack!(block = self.lower_scrutinee(block, expr_id, expr_span)); let scrutinee = unpack!(block = self.lower_scrutinee(block, expr_id, expr_span));
@ -2017,10 +2086,22 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
self.break_for_else(otherwise_block, self.source_info(expr_span)); self.break_for_else(otherwise_block, self.source_info(expr_span));
if declare_bindings { match declare_let_bindings {
let expr_place = scrutinee.try_to_place(self); DeclareLetBindings::Yes => {
let opt_expr_place = expr_place.as_ref().map(|place| (Some(place), expr_span)); let expr_place = scrutinee.try_to_place(self);
self.declare_bindings(source_scope, pat.span.to(scope_span), pat, None, opt_expr_place); let opt_expr_place = expr_place.as_ref().map(|place| (Some(place), expr_span));
self.declare_bindings(
source_scope,
pat.span.to(scope_span),
pat,
None,
opt_expr_place,
);
}
DeclareLetBindings::No => {} // Caller is responsible for bindings.
DeclareLetBindings::LetNotPermitted => {
self.tcx.dcx().span_bug(expr_span, "let expression not expected in this context")
}
} }
let success = self.bind_pattern( let success = self.bind_pattern(
@ -2029,7 +2110,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
&[], &[],
expr_span, expr_span,
None, None,
storages_alive, emit_storage_live,
); );
// If branch coverage is enabled, record this branch. // If branch coverage is enabled, record this branch.
@ -2053,8 +2134,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
fake_borrows: &[(Place<'tcx>, Local, FakeBorrowKind)], fake_borrows: &[(Place<'tcx>, Local, FakeBorrowKind)],
scrutinee_span: Span, scrutinee_span: Span,
arm_match_scope: Option<(&Arm<'tcx>, region::Scope)>, arm_match_scope: Option<(&Arm<'tcx>, region::Scope)>,
schedule_drops: bool, schedule_drops: ScheduleDrops,
storages_alive: bool, emit_storage_live: EmitStorageLive,
) -> BasicBlock { ) -> BasicBlock {
debug!("bind_and_guard_matched_candidate(candidate={:?})", candidate); debug!("bind_and_guard_matched_candidate(candidate={:?})", candidate);
@ -2203,7 +2284,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
guard, guard,
None, // Use `self.local_scope()` as the temp scope None, // Use `self.local_scope()` as the temp scope
this.source_info(arm.span), this.source_info(arm.span),
false, // For guards, `let` bindings are declared separately DeclareLetBindings::No, // For guards, `let` bindings are declared separately
) )
}); });
@ -2264,12 +2345,16 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let cause = FakeReadCause::ForGuardBinding; let cause = FakeReadCause::ForGuardBinding;
self.cfg.push_fake_read(post_guard_block, guard_end, cause, Place::from(local_id)); self.cfg.push_fake_read(post_guard_block, guard_end, cause, Place::from(local_id));
} }
assert!(schedule_drops, "patterns with guards must schedule drops"); assert_matches!(
schedule_drops,
ScheduleDrops::Yes,
"patterns with guards must schedule drops"
);
self.bind_matched_candidate_for_arm_body( self.bind_matched_candidate_for_arm_body(
post_guard_block, post_guard_block,
true, ScheduleDrops::Yes,
by_value_bindings, by_value_bindings,
storages_alive, emit_storage_live,
); );
post_guard_block post_guard_block
@ -2281,7 +2366,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
block, block,
schedule_drops, schedule_drops,
bindings, bindings,
storages_alive, emit_storage_live,
); );
block block
} }
@ -2317,7 +2402,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
fn bind_matched_candidate_for_guard<'b>( fn bind_matched_candidate_for_guard<'b>(
&mut self, &mut self,
block: BasicBlock, block: BasicBlock,
schedule_drops: bool, schedule_drops: ScheduleDrops,
bindings: impl IntoIterator<Item = &'b Binding<'tcx>>, bindings: impl IntoIterator<Item = &'b Binding<'tcx>>,
) where ) where
'tcx: 'b, 'tcx: 'b,
@ -2370,9 +2455,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
fn bind_matched_candidate_for_arm_body<'b>( fn bind_matched_candidate_for_arm_body<'b>(
&mut self, &mut self,
block: BasicBlock, block: BasicBlock,
schedule_drops: bool, schedule_drops: ScheduleDrops,
bindings: impl IntoIterator<Item = &'b Binding<'tcx>>, bindings: impl IntoIterator<Item = &'b Binding<'tcx>>,
storages_alive: bool, emit_storage_live: EmitStorageLive,
) where ) where
'tcx: 'b, 'tcx: 'b,
{ {
@ -2382,21 +2467,20 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
// Assign each of the bindings. This may trigger moves out of the candidate. // Assign each of the bindings. This may trigger moves out of the candidate.
for binding in bindings { for binding in bindings {
let source_info = self.source_info(binding.span); let source_info = self.source_info(binding.span);
let local = if storages_alive { let local = match emit_storage_live {
// Here storages are already alive, probably because this is a binding // Here storages are already alive, probably because this is a binding
// from let-else. // from let-else.
// We just need to schedule drop for the value. // We just need to schedule drop for the value.
self.var_local_id(binding.var_id, OutsideGuard).into() EmitStorageLive::No => self.var_local_id(binding.var_id, OutsideGuard).into(),
} else { EmitStorageLive::Yes => self.storage_live_binding(
self.storage_live_binding(
block, block,
binding.var_id, binding.var_id,
binding.span, binding.span,
OutsideGuard, OutsideGuard,
schedule_drops, schedule_drops,
) ),
}; };
if schedule_drops { if matches!(schedule_drops, ScheduleDrops::Yes) {
self.schedule_drop_for_binding(binding.var_id, binding.span, OutsideGuard); self.schedule_drop_for_binding(binding.var_id, binding.span, OutsideGuard);
} }
let rvalue = match binding.binding_mode.0 { let rvalue = match binding.binding_mode.0 {