From ad575b093bee669258b1d4914bccf56c5b9d1e82 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 26 Jun 2024 11:34:31 +1000 Subject: [PATCH 1/3] Replace a magic boolean with enum `DeclareLetBindings` The new enum `DeclareLetBindings` has three variants: - `Yes`: Declare `let` bindings as normal, for `if` conditions. - `No`: Don't declare bindings, for match guards and let-else. - `LetNotPermitted`: Assert that `let` expressions should not occur. --- compiler/rustc_mir_build/src/build/block.rs | 3 +- .../rustc_mir_build/src/build/expr/into.rs | 5 +- .../rustc_mir_build/src/build/matches/mod.rs | 73 +++++++++++++++---- 3 files changed, 64 insertions(+), 17 deletions(-) diff --git a/compiler/rustc_mir_build/src/build/block.rs b/compiler/rustc_mir_build/src/build/block.rs index 476969a1bd7ad..4616018c3c690 100644 --- a/compiler/rustc_mir_build/src/build/block.rs +++ b/compiler/rustc_mir_build/src/build/block.rs @@ -1,3 +1,4 @@ +use crate::build::matches::DeclareLetBindings; use crate::build::ForGuard::OutsideGuard; use crate::build::{BlockAnd, BlockAndExtension, BlockFrame, Builder}; use rustc_middle::middle::region::Scope; @@ -213,7 +214,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { pattern, None, initializer_span, - false, + DeclareLetBindings::No, true, ) }); diff --git a/compiler/rustc_mir_build/src/build/expr/into.rs b/compiler/rustc_mir_build/src/build/expr/into.rs index 76bdc26a501a6..942c69b5c0a75 100644 --- a/compiler/rustc_mir_build/src/build/expr/into.rs +++ b/compiler/rustc_mir_build/src/build/expr/into.rs @@ -1,6 +1,7 @@ //! See docs in build/expr/mod.rs use crate::build::expr::category::{Category, RvalueFunc}; +use crate::build::matches::DeclareLetBindings; use crate::build::{BlockAnd, BlockAndExtension, BlockFrame, Builder, NeedsTemporary}; use rustc_ast::InlineAsmOptions; use rustc_data_structures::fx::FxHashMap; @@ -86,7 +87,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { cond, Some(condition_scope), // Temp scope source_info, - true, // Declare `let` bindings normally + DeclareLetBindings::Yes, // Declare `let` bindings normally )); // Lower the `then` arm into its block. @@ -163,7 +164,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { source_info, // This flag controls how inner `let` expressions are lowered, // but either way there shouldn't be any of those in here. - true, + DeclareLetBindings::LetNotPermitted, ) }); let (short_circuit, continuation, constant) = match op { diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index 6d52c30823769..71c065b3574c7 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -39,9 +39,27 @@ struct ThenElseArgs { /// `self.local_scope()` is used. temp_scope_override: Option, variable_source_info: SourceInfo, + /// Determines how bindings should be handled when lowering `let` expressions. + /// /// 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, + declare_let_bindings: DeclareLetBindings, +} + +/// 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, } impl<'a, 'tcx> Builder<'a, 'tcx> { @@ -57,7 +75,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { expr_id: ExprId, temp_scope_override: Option, variable_source_info: SourceInfo, - declare_let_bindings: bool, + declare_let_bindings: DeclareLetBindings, ) -> BlockAnd<()> { self.then_else_break_inner( block, @@ -91,13 +109,19 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { this.then_else_break_inner( block, lhs, - ThenElseArgs { declare_let_bindings: true, ..args }, + ThenElseArgs { + declare_let_bindings: DeclareLetBindings::LetNotPermitted, + ..args + }, ) }); let rhs_success_block = unpack!(this.then_else_break_inner( failure_block, 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. @@ -127,7 +151,10 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { this.then_else_break_inner( block, arg, - ThenElseArgs { declare_let_bindings: true, ..args }, + ThenElseArgs { + declare_let_bindings: DeclareLetBindings::LetNotPermitted, + ..args + }, ) }); this.break_for_else(success_block, args.variable_source_info); @@ -1991,8 +2018,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { // Pat binding - used for `let` and function parameters as well. impl<'a, 'tcx> Builder<'a, 'tcx> { - /// If the bindings have already been declared, set `declare_bindings` to - /// `false` to avoid duplicated bindings declaration; used for if-let guards. + /// Lowers a `let` expression that appears in a suitable context + /// (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( &mut self, mut block: BasicBlock, @@ -2000,7 +2033,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { pat: &Pat<'tcx>, source_scope: Option, scope_span: Span, - declare_bindings: bool, + declare_let_bindings: DeclareLetBindings, storages_alive: bool, ) -> BlockAnd<()> { let expr_span = self.thir[expr_id].span; @@ -2017,10 +2050,22 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { self.break_for_else(otherwise_block, self.source_info(expr_span)); - if declare_bindings { - let expr_place = scrutinee.try_to_place(self); - 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); + match declare_let_bindings { + DeclareLetBindings::Yes => { + let expr_place = scrutinee.try_to_place(self); + 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( @@ -2203,7 +2248,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { guard, None, // Use `self.local_scope()` as the temp scope this.source_info(arm.span), - false, // For guards, `let` bindings are declared separately + DeclareLetBindings::No, // For guards, `let` bindings are declared separately ) }); From 3b22589cfa0015535ae08082b2cecf77ac1b5c33 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 26 Jun 2024 12:40:47 +1000 Subject: [PATCH 2/3] Replace a magic boolean with enum `EmitStorageLive` The previous boolean used `true` to indicate that storage-live should _not_ be emitted, so all occurrences of `Yes` and `No` should be the logical opposite of the previous value. --- compiler/rustc_mir_build/src/build/block.rs | 4 +- .../rustc_mir_build/src/build/matches/mod.rs | 47 ++++++++++++------- 2 files changed, 32 insertions(+), 19 deletions(-) diff --git a/compiler/rustc_mir_build/src/build/block.rs b/compiler/rustc_mir_build/src/build/block.rs index 4616018c3c690..80db30f0ae600 100644 --- a/compiler/rustc_mir_build/src/build/block.rs +++ b/compiler/rustc_mir_build/src/build/block.rs @@ -1,4 +1,4 @@ -use crate::build::matches::DeclareLetBindings; +use crate::build::matches::{DeclareLetBindings, EmitStorageLive}; use crate::build::ForGuard::OutsideGuard; use crate::build::{BlockAnd, BlockAndExtension, BlockFrame, Builder}; use rustc_middle::middle::region::Scope; @@ -215,7 +215,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { None, initializer_span, DeclareLetBindings::No, - true, + EmitStorageLive::No, ) }); matching.and(failure) diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index 71c065b3574c7..4827d2dbfa85c 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -62,6 +62,18 @@ pub(crate) enum DeclareLetBindings { 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, +} + 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. @@ -174,7 +186,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { Some(args.variable_source_info.scope), args.variable_source_info.span, args.declare_let_bindings, - false, + EmitStorageLive::Yes, ), _ => { let mut block = block; @@ -467,7 +479,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { &fake_borrow_temps, scrutinee_span, Some((arm, match_scope)), - false, + EmitStorageLive::Yes, ); this.fixed_temps_scope = old_dedup_scope; @@ -512,7 +524,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { fake_borrow_temps: &[(Place<'tcx>, Local, FakeBorrowKind)], scrutinee_span: Span, arm_match_scope: Option<(&Arm<'tcx>, region::Scope)>, - storages_alive: bool, + emit_storage_live: EmitStorageLive, ) -> BasicBlock { if candidate.subcandidates.is_empty() { // Avoid generating another `BasicBlock` when we only have one @@ -524,7 +536,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { scrutinee_span, arm_match_scope, true, - storages_alive, + emit_storage_live, ) } else { // It's helpful to avoid scheduling drops multiple times to save @@ -561,7 +573,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { scrutinee_span, arm_match_scope, schedule_drops, - storages_alive, + emit_storage_live, ); if arm.is_none() { schedule_drops = false; @@ -731,7 +743,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { &[], irrefutable_pat.span, None, - false, + EmitStorageLive::Yes, ) .unit() } @@ -807,6 +819,8 @@ 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( &mut self, block: BasicBlock, @@ -2034,7 +2048,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { source_scope: Option, scope_span: Span, declare_let_bindings: DeclareLetBindings, - storages_alive: bool, + emit_storage_live: EmitStorageLive, ) -> BlockAnd<()> { let expr_span = self.thir[expr_id].span; let scrutinee = unpack!(block = self.lower_scrutinee(block, expr_id, expr_span)); @@ -2074,7 +2088,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { &[], expr_span, None, - storages_alive, + emit_storage_live, ); // If branch coverage is enabled, record this branch. @@ -2099,7 +2113,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { scrutinee_span: Span, arm_match_scope: Option<(&Arm<'tcx>, region::Scope)>, schedule_drops: bool, - storages_alive: bool, + emit_storage_live: EmitStorageLive, ) -> BasicBlock { debug!("bind_and_guard_matched_candidate(candidate={:?})", candidate); @@ -2314,7 +2328,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { post_guard_block, true, by_value_bindings, - storages_alive, + emit_storage_live, ); post_guard_block @@ -2326,7 +2340,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { block, schedule_drops, bindings, - storages_alive, + emit_storage_live, ); block } @@ -2417,7 +2431,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { block: BasicBlock, schedule_drops: bool, bindings: impl IntoIterator>, - storages_alive: bool, + emit_storage_live: EmitStorageLive, ) where 'tcx: 'b, { @@ -2427,19 +2441,18 @@ 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 = if storages_alive { + let local = match emit_storage_live { // 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( + EmitStorageLive::No => self.var_local_id(binding.var_id, OutsideGuard).into(), + EmitStorageLive::Yes => 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); From ed07712e96506ca827ced41d748550b99318d47f Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 26 Jun 2024 13:44:25 +1000 Subject: [PATCH 3/3] Replace a magic boolean with enum `ScheduleDrops` --- compiler/rustc_mir_build/src/build/block.rs | 18 +++++- .../rustc_mir_build/src/build/matches/mod.rs | 56 ++++++++++++++----- 2 files changed, 56 insertions(+), 18 deletions(-) diff --git a/compiler/rustc_mir_build/src/build/block.rs b/compiler/rustc_mir_build/src/build/block.rs index 80db30f0ae600..5ccbd7c59cfba 100644 --- a/compiler/rustc_mir_build/src/build/block.rs +++ b/compiler/rustc_mir_build/src/build/block.rs @@ -1,4 +1,4 @@ -use crate::build::matches::{DeclareLetBindings, EmitStorageLive}; +use crate::build::matches::{DeclareLetBindings, EmitStorageLive, ScheduleDrops}; use crate::build::ForGuard::OutsideGuard; use crate::build::{BlockAnd, BlockAndExtension, BlockFrame, Builder}; use rustc_middle::middle::region::Scope; @@ -202,7 +202,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { pattern, UserTypeProjections::none(), &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; @@ -292,7 +298,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { pattern, UserTypeProjections::none(), &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); }, ) diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index 4827d2dbfa85c..efed52231e3fa 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -28,6 +28,7 @@ mod simplify; mod test; mod util; +use std::assert_matches::assert_matches; use std::borrow::Borrow; use std::mem; @@ -74,6 +75,17 @@ pub(crate) enum EmitStorageLive { 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> { /// Lowers a condition in a way that ensures that variables bound in any let /// expressions are definitely initialized in the if body. @@ -535,7 +547,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { fake_borrow_temps, scrutinee_span, arm_match_scope, - true, + ScheduleDrops::Yes, emit_storage_live, ) } else { @@ -554,7 +566,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { // To handle this we instead unschedule it's drop after each time // we lower the guard. 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; // We keep a stack of all of the bindings and type ascriptions // from the parent candidates that we visit, that also need to @@ -576,7 +588,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { emit_storage_live, ); if arm.is_none() { - schedule_drops = false; + schedule_drops = ScheduleDrops::No; } self.cfg.goto(binding_end, outer_source_info, target_block); }, @@ -602,8 +614,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { match irrefutable_pat.kind { // Optimize the case of `let x = ...` to write directly into `x` PatKind::Binding { mode: BindingMode(ByRef::No, _), var, subpattern: None, .. } => { - let place = - self.storage_live_binding(block, var, irrefutable_pat.span, OutsideGuard, true); + let place = self.storage_live_binding( + block, + var, + irrefutable_pat.span, + OutsideGuard, + ScheduleDrops::Yes, + ); unpack!(block = self.expr_into_dest(place, block, initializer_id)); // Inject a fake read, see comments on `FakeReadCause::ForLet`. @@ -636,8 +653,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { }, ascription: thir::Ascription { ref annotation, variance: _ }, } => { - let place = - self.storage_live_binding(block, var, irrefutable_pat.span, OutsideGuard, true); + let place = self.storage_live_binding( + block, + var, + irrefutable_pat.span, + OutsideGuard, + ScheduleDrops::Yes, + ); unpack!(block = self.expr_into_dest(place, block, initializer_id)); // Inject a fake read, see comments on `FakeReadCause::ForLet`. @@ -827,7 +849,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { var: LocalVarId, span: Span, for_guard: ForGuard, - schedule_drop: bool, + schedule_drop: ScheduleDrops, ) -> Place<'tcx> { let local_id = self.var_local_id(var, for_guard); let source_info = self.source_info(span); @@ -835,7 +857,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { // Although there is almost always scope for given variable in corner cases // like #92893 we might get variable with no scope. 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); } @@ -2112,7 +2134,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { fake_borrows: &[(Place<'tcx>, Local, FakeBorrowKind)], scrutinee_span: Span, arm_match_scope: Option<(&Arm<'tcx>, region::Scope)>, - schedule_drops: bool, + schedule_drops: ScheduleDrops, emit_storage_live: EmitStorageLive, ) -> BasicBlock { debug!("bind_and_guard_matched_candidate(candidate={:?})", candidate); @@ -2323,10 +2345,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let cause = FakeReadCause::ForGuardBinding; 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( post_guard_block, - true, + ScheduleDrops::Yes, by_value_bindings, emit_storage_live, ); @@ -2376,7 +2402,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { fn bind_matched_candidate_for_guard<'b>( &mut self, block: BasicBlock, - schedule_drops: bool, + schedule_drops: ScheduleDrops, bindings: impl IntoIterator>, ) where 'tcx: 'b, @@ -2429,7 +2455,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { fn bind_matched_candidate_for_arm_body<'b>( &mut self, block: BasicBlock, - schedule_drops: bool, + schedule_drops: ScheduleDrops, bindings: impl IntoIterator>, emit_storage_live: EmitStorageLive, ) where @@ -2454,7 +2480,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { schedule_drops, ), }; - if schedule_drops { + if matches!(schedule_drops, ScheduleDrops::Yes) { self.schedule_drop_for_binding(binding.var_id, binding.span, OutsideGuard); } let rvalue = match binding.binding_mode.0 {