From ea29d6ad0bfb2b1544566369746aeaa85b5f9de5 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Mon, 17 Jun 2024 19:15:03 +0200 Subject: [PATCH 1/8] We can traverse bindings before `lower_match_tree` now --- .../rustc_mir_build/src/build/matches/mod.rs | 71 ++++++++--------- .../rustc_mir_build/src/build/matches/util.rs | 77 ++++++++++++++++--- 2 files changed, 98 insertions(+), 50 deletions(-) diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index 68244136d1adf..f182e1c57f416 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -21,6 +21,7 @@ use rustc_span::symbol::Symbol; use rustc_span::{BytePos, Pos, Span}; use rustc_target::abi::VariantIdx; use tracing::{debug, instrument}; +use util::visit_bindings; // helper functions, broken out by category: mod simplify; @@ -725,55 +726,49 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { set_match_place: bool, ) -> BlockAnd<()> { let mut candidate = Candidate::new(initializer.clone(), irrefutable_pat, false, self); - let fake_borrow_temps = self.lower_match_tree( - block, - irrefutable_pat.span, - &initializer, - irrefutable_pat.span, - false, - &mut [&mut candidate], - ); // For matches and function arguments, the place that is being matched // can be set when creating the variables. But the place for // let PATTERN = ... might not even exist until we do the assignment. // so we set it here instead. if set_match_place { - let mut next = Some(&candidate); - while let Some(candidate_ref) = next.take() { - for binding in &candidate_ref.extra_data.bindings { + // `try_to_place` may fail if it is unable to resolve the given `PlaceBuilder` inside a + // closure. In this case, we don't want to include a scrutinee place. + // `scrutinee_place_builder` will fail for destructured assignments. This is because a + // closure only captures the precise places that it will read and as a result a closure + // may not capture the entire tuple/struct and rather have individual places that will + // be read in the final MIR. + // Example: + // ``` + // let foo = (0, 1); + // let c = || { + // let (v1, v2) = foo; + // }; + // ``` + if let Some(place) = initializer.try_to_place(self) { + visit_bindings(&[&mut candidate], |binding: &Binding<'_>| { let local = self.var_local_id(binding.var_id, OutsideGuard); - // `try_to_place` may fail if it is unable to resolve the given - // `PlaceBuilder` inside a closure. In this case, we don't want to include - // a scrutinee place. `scrutinee_place_builder` will fail for destructured - // assignments. This is because a closure only captures the precise places - // that it will read and as a result a closure may not capture the entire - // tuple/struct and rather have individual places that will be read in the - // final MIR. - // Example: - // ``` - // let foo = (0, 1); - // let c = || { - // let (v1, v2) = foo; - // }; - // ``` - if let Some(place) = initializer.try_to_place(self) { - let LocalInfo::User(BindingForm::Var(VarBindingForm { - opt_match_place: Some((ref mut match_place, _)), - .. - })) = **self.local_decls[local].local_info.as_mut().assert_crate_local() - else { - bug!("Let binding to non-user variable.") - }; + if let LocalInfo::User(BindingForm::Var(VarBindingForm { + opt_match_place: Some((ref mut match_place, _)), + .. + })) = **self.local_decls[local].local_info.as_mut().assert_crate_local() + { *match_place = Some(place); - } - } - // All of the subcandidates should bind the same locals, so we - // only visit the first one. - next = candidate_ref.subcandidates.get(0) + } else { + bug!("Let binding to non-user variable.") + }; + }); } } + let fake_borrow_temps = self.lower_match_tree( + block, + irrefutable_pat.span, + &initializer, + irrefutable_pat.span, + false, + &mut [&mut candidate], + ); self.bind_pattern( self.source_info(irrefutable_pat.span), candidate, diff --git a/compiler/rustc_mir_build/src/build/matches/util.rs b/compiler/rustc_mir_build/src/build/matches/util.rs index 50f4ca2d819d3..e0ba736f1989c 100644 --- a/compiler/rustc_mir_build/src/build/matches/util.rs +++ b/compiler/rustc_mir_build/src/build/matches/util.rs @@ -1,3 +1,5 @@ +use std::marker::PhantomData; + use crate::build::expr::as_place::{PlaceBase, PlaceBuilder}; use crate::build::matches::{Binding, Candidate, FlatPat, MatchPair, TestCase}; use crate::build::Builder; @@ -267,18 +269,6 @@ impl<'pat, 'tcx> MatchPair<'pat, 'tcx> { } } -pub(super) struct FakeBorrowCollector<'a, 'b, 'tcx> { - cx: &'a mut Builder<'b, 'tcx>, - /// Base of the scrutinee place. Used to distinguish bindings inside the scrutinee place from - /// bindings inside deref patterns. - scrutinee_base: PlaceBase, - /// Store for each place the kind of borrow to take. In case of conflicts, we take the strongest - /// borrow (i.e. Deep > Shallow). - /// Invariant: for any place in `fake_borrows`, all the prefixes of this place that are - /// dereferences are also borrowed with the same of stronger borrow kind. - fake_borrows: FxIndexMap, FakeBorrowKind>, -} - /// Determine the set of places that have to be stable across match guards. /// /// Returns a list of places that need a fake borrow along with a local to store it. @@ -342,6 +332,18 @@ pub(super) fn collect_fake_borrows<'tcx>( .collect() } +pub(super) struct FakeBorrowCollector<'a, 'b, 'tcx> { + cx: &'a mut Builder<'b, 'tcx>, + /// Base of the scrutinee place. Used to distinguish bindings inside the scrutinee place from + /// bindings inside deref patterns. + scrutinee_base: PlaceBase, + /// Store for each place the kind of borrow to take. In case of conflicts, we take the strongest + /// borrow (i.e. Deep > Shallow). + /// Invariant: for any place in `fake_borrows`, all the prefixes of this place that are + /// dereferences are also borrowed with the same of stronger borrow kind. + fake_borrows: FxIndexMap, FakeBorrowKind>, +} + impl<'a, 'b, 'tcx> FakeBorrowCollector<'a, 'b, 'tcx> { // Fake borrow this place and its dereference prefixes. fn fake_borrow(&mut self, place: Place<'tcx>, kind: FakeBorrowKind) { @@ -455,6 +457,57 @@ impl<'a, 'b, 'tcx> FakeBorrowCollector<'a, 'b, 'tcx> { } } +/// Visit all the bindings of these candidates. Because or-alternatives bind the same variables, we +/// only explore the first one of each or-pattern. +pub(super) fn visit_bindings<'tcx>( + candidates: &[&mut Candidate<'_, 'tcx>], + f: impl FnMut(&Binding<'tcx>), +) { + let mut visitor = BindingsVisitor { f, phantom: PhantomData }; + for candidate in candidates.iter() { + visitor.visit_candidate(candidate); + } +} + +pub(super) struct BindingsVisitor<'tcx, F> { + f: F, + phantom: PhantomData<&'tcx ()>, +} + +impl<'tcx, F> BindingsVisitor<'tcx, F> +where + F: FnMut(&Binding<'tcx>), +{ + fn visit_candidate(&mut self, candidate: &Candidate<'_, 'tcx>) { + for binding in &candidate.extra_data.bindings { + (self.f)(binding) + } + for match_pair in &candidate.match_pairs { + self.visit_match_pair(match_pair); + } + } + + fn visit_flat_pat(&mut self, flat_pat: &FlatPat<'_, 'tcx>) { + for binding in &flat_pat.extra_data.bindings { + (self.f)(binding) + } + for match_pair in &flat_pat.match_pairs { + self.visit_match_pair(match_pair); + } + } + + fn visit_match_pair(&mut self, match_pair: &MatchPair<'_, 'tcx>) { + if let TestCase::Or { pats, .. } = &match_pair.test_case { + // All the or-alternatives should bind the same locals, so we only visit the first one. + self.visit_flat_pat(&pats[0]) + } else { + for subpair in &match_pair.subpairs { + self.visit_match_pair(subpair); + } + } + } +} + #[must_use] pub(crate) fn ref_pat_borrow_kind(ref_mutability: Mutability) -> BorrowKind { match ref_mutability { From 012626b32bee88181cca63ef592cad83b19c8fda Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Mon, 17 Jun 2024 19:29:14 +0200 Subject: [PATCH 2/8] Only one caller of `lower_match_tree` was using the fake borrows --- .../rustc_mir_build/src/build/matches/mod.rs | 48 +++++++------------ 1 file changed, 17 insertions(+), 31 deletions(-) diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index f182e1c57f416..e9fc9361a719e 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -315,12 +315,19 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let match_start_span = span.shrink_to_lo().to(scrutinee_span); - let fake_borrow_temps = self.lower_match_tree( + // The set of places that we are creating fake borrows of. If there are no match guards then + // we don't need any fake borrows, so don't track them. + let fake_borrow_temps: Vec<(Place<'tcx>, Local, FakeBorrowKind)> = if match_has_guard { + util::collect_fake_borrows(self, &candidates, scrutinee_span, scrutinee_place.base()) + } else { + Vec::new() + }; + + self.lower_match_tree( block, scrutinee_span, &scrutinee_place, match_start_span, - match_has_guard, &mut candidates, ); @@ -377,30 +384,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { /// /// Modifies `candidates` to store the bindings and type ascriptions for /// that candidate. - /// - /// Returns the places that need fake borrows because we bind or test them. fn lower_match_tree<'pat>( &mut self, block: BasicBlock, scrutinee_span: Span, scrutinee_place_builder: &PlaceBuilder<'tcx>, match_start_span: Span, - match_has_guard: bool, candidates: &mut [&mut Candidate<'pat, 'tcx>], - ) -> Vec<(Place<'tcx>, Local, FakeBorrowKind)> { - // The set of places that we are creating fake borrows of. If there are no match guards then - // we don't need any fake borrows, so don't track them. - let fake_borrows: Vec<(Place<'tcx>, Local, FakeBorrowKind)> = if match_has_guard { - util::collect_fake_borrows( - self, - candidates, - scrutinee_span, - scrutinee_place_builder.base(), - ) - } else { - Vec::new() - }; - + ) { // See the doc comment on `match_candidates` for why we have an // otherwise block. Match checking will ensure this is actually // unreachable. @@ -452,8 +443,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { previous_candidate = Some(leaf_candidate); }); } - - fake_borrows } /// Lower the bindings, guards and arm bodies of a `match` expression. @@ -761,18 +750,17 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { } } - let fake_borrow_temps = self.lower_match_tree( + self.lower_match_tree( block, irrefutable_pat.span, &initializer, irrefutable_pat.span, - false, &mut [&mut candidate], ); self.bind_pattern( self.source_info(irrefutable_pat.span), candidate, - fake_borrow_temps.as_slice(), + &[], irrefutable_pat.span, None, false, @@ -1995,12 +1983,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let mut guard_candidate = Candidate::new(expr_place_builder.clone(), pat, false, self); let mut otherwise_candidate = Candidate::new(expr_place_builder.clone(), &wildcard, false, self); - let fake_borrow_temps = self.lower_match_tree( + self.lower_match_tree( block, pat.span, &expr_place_builder, pat.span, - false, &mut [&mut guard_candidate, &mut otherwise_candidate], ); let expr_place = expr_place_builder.try_to_place(self); @@ -2015,7 +2002,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let post_guard_block = self.bind_pattern( self.source_info(pat.span), guard_candidate, - fake_borrow_temps.as_slice(), + &[], expr_span, None, false, @@ -2490,19 +2477,18 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let pat = Pat { ty: pattern.ty, span: else_block_span, kind: PatKind::Wild }; let mut wildcard = Candidate::new(scrutinee.clone(), &pat, false, this); let mut candidate = Candidate::new(scrutinee.clone(), pattern, false, this); - let fake_borrow_temps = this.lower_match_tree( + this.lower_match_tree( block, initializer_span, &scrutinee, pattern.span, - false, &mut [&mut candidate, &mut wildcard], ); // This block is for the matching case let matching = this.bind_pattern( this.source_info(pattern.span), candidate, - fake_borrow_temps.as_slice(), + &[], initializer_span, None, true, @@ -2511,7 +2497,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let failure = this.bind_pattern( this.source_info(else_block_span), wildcard, - fake_borrow_temps.as_slice(), + &[], initializer_span, None, true, From cef49f73e76a5f133452b589caa179e4bea8fad2 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Mon, 17 Jun 2024 19:39:09 +0200 Subject: [PATCH 3/8] Small dedup --- .../rustc_mir_build/src/build/matches/mod.rs | 33 +++++++++---------- 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index e9fc9361a719e..3826f58a6c918 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -2059,14 +2059,15 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { return self.cfg.start_new_block(); } - self.ascribe_types( - block, - parent_data - .iter() - .flat_map(|d| &d.ascriptions) - .cloned() - .chain(candidate.extra_data.ascriptions), - ); + let ascriptions = parent_data + .iter() + .flat_map(|d| &d.ascriptions) + .cloned() + .chain(candidate.extra_data.ascriptions); + let bindings = + parent_data.iter().flat_map(|d| &d.bindings).chain(&candidate.extra_data.bindings); + + self.ascribe_types(block, ascriptions); // rust-lang/rust#27282: The `autoref` business deserves some // explanation here. @@ -2153,12 +2154,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { && let Some(guard) = arm.guard { let tcx = self.tcx; - let bindings = - parent_data.iter().flat_map(|d| &d.bindings).chain(&candidate.extra_data.bindings); self.bind_matched_candidate_for_guard(block, schedule_drops, bindings.clone()); - let guard_frame = - GuardFrame { locals: bindings.map(|b| GuardFrameLocal::new(b.var_id)).collect() }; + let guard_frame = GuardFrame { + locals: bindings.clone().map(|b| GuardFrameLocal::new(b.var_id)).collect(), + }; debug!("entering guard building context: {:?}", guard_frame); self.guard_context.push(guard_frame); @@ -2231,11 +2231,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { // ``` // // and that is clearly not correct. - let by_value_bindings = parent_data - .iter() - .flat_map(|d| &d.bindings) - .chain(&candidate.extra_data.bindings) - .filter(|binding| matches!(binding.binding_mode.0, ByRef::No)); + let by_value_bindings = + bindings.filter(|binding| matches!(binding.binding_mode.0, ByRef::No)); // Read all of the by reference bindings to ensure that the // place they refer to can't be modified by the guard. for binding in by_value_bindings.clone() { @@ -2259,7 +2256,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { self.bind_matched_candidate_for_arm_body( block, schedule_drops, - parent_data.iter().flat_map(|d| &d.bindings).chain(&candidate.extra_data.bindings), + bindings, storages_alive, ); block From 878ccd22fa6562afd6658f6558632ba8953ae22a Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Mon, 17 Jun 2024 19:46:07 +0200 Subject: [PATCH 4/8] There's nothing to bind for a wildcard This commit was obtained by repeatedly inlining and simplifying. --- compiler/rustc_mir_build/src/build/matches/mod.rs | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index 3826f58a6c918..17b5e1b495578 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -2491,14 +2491,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { true, ); // This block is for the failure case - let failure = this.bind_pattern( - this.source_info(else_block_span), - wildcard, - &[], - initializer_span, - None, - true, - ); + let failure = wildcard.pre_binding_block.unwrap(); // If branch coverage is enabled, record this branch. this.visit_coverage_conditional_let(pattern, matching, failure); From c0c6c32a45e9ef05d99459a77e39072524ed1dc4 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Mon, 17 Jun 2024 20:08:05 +0200 Subject: [PATCH 5/8] Move `lower_match_tree` --- .../rustc_mir_build/src/build/matches/mod.rs | 129 +++++++++--------- 1 file changed, 64 insertions(+), 65 deletions(-) diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index 17b5e1b495578..8531ea891d2e3 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -380,71 +380,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { .collect() } - /// Create the decision tree for the match expression, starting from `block`. - /// - /// Modifies `candidates` to store the bindings and type ascriptions for - /// that candidate. - fn lower_match_tree<'pat>( - &mut self, - block: BasicBlock, - scrutinee_span: Span, - scrutinee_place_builder: &PlaceBuilder<'tcx>, - match_start_span: Span, - candidates: &mut [&mut Candidate<'pat, 'tcx>], - ) { - // See the doc comment on `match_candidates` for why we have an - // otherwise block. Match checking will ensure this is actually - // unreachable. - let otherwise_block = self.cfg.start_new_block(); - - // This will generate code to test scrutinee_place and - // branch to the appropriate arm block - self.match_candidates(match_start_span, scrutinee_span, block, otherwise_block, candidates); - - let source_info = self.source_info(scrutinee_span); - - // Matching on a `scrutinee_place` with an uninhabited type doesn't - // generate any memory reads by itself, and so if the place "expression" - // contains unsafe operations like raw pointer dereferences or union - // field projections, we wouldn't know to require an `unsafe` block - // around a `match` equivalent to `std::intrinsics::unreachable()`. - // See issue #47412 for this hole being discovered in the wild. - // - // HACK(eddyb) Work around the above issue by adding a dummy inspection - // of `scrutinee_place`, specifically by applying `ReadForMatch`. - // - // NOTE: ReadForMatch also checks that the scrutinee is initialized. - // This is currently needed to not allow matching on an uninitialized, - // uninhabited value. If we get never patterns, those will check that - // the place is initialized, and so this read would only be used to - // check safety. - let cause_matched_place = FakeReadCause::ForMatchedPlace(None); - - if let Some(scrutinee_place) = scrutinee_place_builder.try_to_place(self) { - self.cfg.push_fake_read( - otherwise_block, - source_info, - cause_matched_place, - scrutinee_place, - ); - } - - self.cfg.terminate(otherwise_block, source_info, TerminatorKind::Unreachable); - - // Link each leaf candidate to the `pre_binding_block` of the next one. - let mut previous_candidate: Option<&mut Candidate<'_, '_>> = None; - - for candidate in candidates { - candidate.visit_leaves(|leaf_candidate| { - if let Some(ref mut prev) = previous_candidate { - assert!(leaf_candidate.false_edge_start_block.is_some()); - prev.next_candidate_start_block = leaf_candidate.false_edge_start_block; - } - previous_candidate = Some(leaf_candidate); - }); - } - } - /// Lower the bindings, guards and arm bodies of a `match` expression. /// /// The decision tree should have already been created @@ -1275,6 +1210,70 @@ pub(crate) struct ArmHasGuard(pub(crate) bool); // Main matching algorithm impl<'a, 'tcx> Builder<'a, 'tcx> { + /// The entrypoint of the matching algorithm. Create the decision tree for the match expression, + /// starting from `block`. + /// + /// Modifies `candidates` to store the bindings and type ascriptions for + /// that candidate. + fn lower_match_tree<'pat>( + &mut self, + block: BasicBlock, + scrutinee_span: Span, + scrutinee_place_builder: &PlaceBuilder<'tcx>, + match_start_span: Span, + candidates: &mut [&mut Candidate<'pat, 'tcx>], + ) { + // See the doc comment on `match_candidates` for why we have an + // otherwise block. Match checking will ensure this is actually + // unreachable. + let otherwise_block = self.cfg.start_new_block(); + + // This will generate code to test scrutinee_place and branch to the appropriate arm block + self.match_candidates(match_start_span, scrutinee_span, block, otherwise_block, candidates); + + let source_info = self.source_info(scrutinee_span); + + // Matching on a `scrutinee_place` with an uninhabited type doesn't + // generate any memory reads by itself, and so if the place "expression" + // contains unsafe operations like raw pointer dereferences or union + // field projections, we wouldn't know to require an `unsafe` block + // around a `match` equivalent to `std::intrinsics::unreachable()`. + // See issue #47412 for this hole being discovered in the wild. + // + // HACK(eddyb) Work around the above issue by adding a dummy inspection + // of `scrutinee_place`, specifically by applying `ReadForMatch`. + // + // NOTE: ReadForMatch also checks that the scrutinee is initialized. + // This is currently needed to not allow matching on an uninitialized, + // uninhabited value. If we get never patterns, those will check that + // the place is initialized, and so this read would only be used to + // check safety. + let cause_matched_place = FakeReadCause::ForMatchedPlace(None); + + if let Some(scrutinee_place) = scrutinee_place_builder.try_to_place(self) { + self.cfg.push_fake_read( + otherwise_block, + source_info, + cause_matched_place, + scrutinee_place, + ); + } + + self.cfg.terminate(otherwise_block, source_info, TerminatorKind::Unreachable); + + // Link each leaf candidate to the `false_edge_start_block` of the next one. + let mut previous_candidate: Option<&mut Candidate<'_, '_>> = None; + for candidate in candidates { + candidate.visit_leaves(|leaf_candidate| { + if let Some(ref mut prev) = previous_candidate { + assert!(leaf_candidate.false_edge_start_block.is_some()); + prev.next_candidate_start_block = leaf_candidate.false_edge_start_block; + } + previous_candidate = Some(leaf_candidate); + }); + } + } + /// The main match algorithm. It begins with a set of candidates /// `candidates` and has the job of generating code to determine /// which of these candidates, if any, is the correct one. The From 7b150a161e7c90cc067c1a709a63785b5dc2a5e9 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Mon, 17 Jun 2024 20:16:53 +0200 Subject: [PATCH 6/8] Don't use fake wildcards when we can get the failure block directly This commit too was obtained by repeatedly inlining and simplifying. --- .../rustc_mir_build/src/build/matches/mod.rs | 109 +++++++------- .../issue_101867.main.built.after.mir | 21 +-- ...n_conditional.test_complex.built.after.mir | 138 ++++++++---------- 3 files changed, 125 insertions(+), 143 deletions(-) diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index 8531ea891d2e3..3eacd598f66c2 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -329,6 +329,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { &scrutinee_place, match_start_span, &mut candidates, + false, ); self.lower_match_arms( @@ -691,6 +692,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { &initializer, irrefutable_pat.span, &mut [&mut candidate], + false, ); self.bind_pattern( self.source_info(irrefutable_pat.span), @@ -1215,6 +1217,10 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { /// /// Modifies `candidates` to store the bindings and type ascriptions for /// that candidate. + /// + /// `refutable` indicates whether the candidate list is refutable (for `if let` and `let else`) + /// or not (for `let` and `match`). In the refutable case we return the block to which we branch + /// on failure. fn lower_match_tree<'pat>( &mut self, block: BasicBlock, @@ -1222,45 +1228,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { scrutinee_place_builder: &PlaceBuilder<'tcx>, match_start_span: Span, candidates: &mut [&mut Candidate<'pat, 'tcx>], - ) { - // See the doc comment on `match_candidates` for why we have an - // otherwise block. Match checking will ensure this is actually - // unreachable. + refutable: bool, + ) -> BasicBlock { + // See the doc comment on `match_candidates` for why we have an otherwise block. let otherwise_block = self.cfg.start_new_block(); // This will generate code to test scrutinee_place and branch to the appropriate arm block self.match_candidates(match_start_span, scrutinee_span, block, otherwise_block, candidates); - let source_info = self.source_info(scrutinee_span); - - // Matching on a `scrutinee_place` with an uninhabited type doesn't - // generate any memory reads by itself, and so if the place "expression" - // contains unsafe operations like raw pointer dereferences or union - // field projections, we wouldn't know to require an `unsafe` block - // around a `match` equivalent to `std::intrinsics::unreachable()`. - // See issue #47412 for this hole being discovered in the wild. - // - // HACK(eddyb) Work around the above issue by adding a dummy inspection - // of `scrutinee_place`, specifically by applying `ReadForMatch`. - // - // NOTE: ReadForMatch also checks that the scrutinee is initialized. - // This is currently needed to not allow matching on an uninitialized, - // uninhabited value. If we get never patterns, those will check that - // the place is initialized, and so this read would only be used to - // check safety. - let cause_matched_place = FakeReadCause::ForMatchedPlace(None); - - if let Some(scrutinee_place) = scrutinee_place_builder.try_to_place(self) { - self.cfg.push_fake_read( - otherwise_block, - source_info, - cause_matched_place, - scrutinee_place, - ); - } - - self.cfg.terminate(otherwise_block, source_info, TerminatorKind::Unreachable); - // Link each leaf candidate to the `false_edge_start_block` of the next one. let mut previous_candidate: Option<&mut Candidate<'_, '_>> = None; for candidate in candidates { @@ -1272,6 +1247,46 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { previous_candidate = Some(leaf_candidate); }); } + + if refutable { + // In refutable cases there's always at least one candidate, and we want a false edge to + // the failure block. + previous_candidate.as_mut().unwrap().next_candidate_start_block = Some(otherwise_block) + } else { + // Match checking ensures `otherwise_block` is actually unreachable in irrefutable + // cases. + let source_info = self.source_info(scrutinee_span); + + // Matching on a `scrutinee_place` with an uninhabited type doesn't + // generate any memory reads by itself, and so if the place "expression" + // contains unsafe operations like raw pointer dereferences or union + // field projections, we wouldn't know to require an `unsafe` block + // around a `match` equivalent to `std::intrinsics::unreachable()`. + // See issue #47412 for this hole being discovered in the wild. + // + // HACK(eddyb) Work around the above issue by adding a dummy inspection + // of `scrutinee_place`, specifically by applying `ReadForMatch`. + // + // NOTE: ReadForMatch also checks that the scrutinee is initialized. + // This is currently needed to not allow matching on an uninitialized, + // uninhabited value. If we get never patterns, those will check that + // the place is initialized, and so this read would only be used to + // check safety. + let cause_matched_place = FakeReadCause::ForMatchedPlace(None); + + if let Some(scrutinee_place) = scrutinee_place_builder.try_to_place(self) { + self.cfg.push_fake_read( + otherwise_block, + source_info, + cause_matched_place, + scrutinee_place, + ); + } + + self.cfg.terminate(otherwise_block, source_info, TerminatorKind::Unreachable); + } + + otherwise_block } /// The main match algorithm. It begins with a set of candidates @@ -1978,21 +1993,18 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { ) -> BlockAnd<()> { let expr_span = self.thir[expr_id].span; let expr_place_builder = unpack!(block = self.lower_scrutinee(block, expr_id, expr_span)); - let wildcard = Pat::wildcard_from_ty(pat.ty); let mut guard_candidate = Candidate::new(expr_place_builder.clone(), pat, false, self); - let mut otherwise_candidate = - Candidate::new(expr_place_builder.clone(), &wildcard, false, self); - self.lower_match_tree( + let otherwise_block = self.lower_match_tree( block, pat.span, &expr_place_builder, pat.span, - &mut [&mut guard_candidate, &mut otherwise_candidate], + &mut [&mut guard_candidate], + true, ); let expr_place = expr_place_builder.try_to_place(self); let opt_expr_place = expr_place.as_ref().map(|place| (Some(place), expr_span)); - let otherwise_post_guard_block = otherwise_candidate.pre_binding_block.unwrap(); - self.break_for_else(otherwise_post_guard_block, self.source_info(expr_span)); + self.break_for_else(otherwise_block, self.source_info(expr_span)); if declare_bindings { self.declare_bindings(source_scope, pat.span.to(span), pat, None, opt_expr_place); @@ -2008,7 +2020,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { ); // If branch coverage is enabled, record this branch. - self.visit_coverage_conditional_let(pat, post_guard_block, otherwise_post_guard_block); + self.visit_coverage_conditional_let(pat, post_guard_block, otherwise_block); post_guard_block.unit() } @@ -2470,15 +2482,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let else_block_span = self.thir[else_block].span; let (matching, failure) = self.in_if_then_scope(*let_else_scope, else_block_span, |this| { let scrutinee = unpack!(block = this.lower_scrutinee(block, init_id, initializer_span)); - let pat = Pat { ty: pattern.ty, span: else_block_span, kind: PatKind::Wild }; - let mut wildcard = Candidate::new(scrutinee.clone(), &pat, false, this); let mut candidate = Candidate::new(scrutinee.clone(), pattern, false, this); - this.lower_match_tree( + let failure_block = this.lower_match_tree( block, initializer_span, &scrutinee, pattern.span, - &mut [&mut candidate, &mut wildcard], + &mut [&mut candidate], + true, ); // This block is for the matching case let matching = this.bind_pattern( @@ -2489,13 +2500,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { None, true, ); - // This block is for the failure case - let failure = wildcard.pre_binding_block.unwrap(); // If branch coverage is enabled, record this branch. - this.visit_coverage_conditional_let(pattern, matching, failure); + this.visit_coverage_conditional_let(pattern, matching, failure_block); - this.break_for_else(failure, this.source_info(initializer_span)); + this.break_for_else(failure_block, this.source_info(initializer_span)); matching.unit() }); matching.and(failure) diff --git a/tests/mir-opt/building/issue_101867.main.built.after.mir b/tests/mir-opt/building/issue_101867.main.built.after.mir index 0f7917dbb5cbf..5c50b3db5cad6 100644 --- a/tests/mir-opt/building/issue_101867.main.built.after.mir +++ b/tests/mir-opt/building/issue_101867.main.built.after.mir @@ -27,13 +27,13 @@ fn main() -> () { StorageLive(_5); PlaceMention(_1); _6 = discriminant(_1); - switchInt(move _6) -> [1: bb6, otherwise: bb4]; + switchInt(move _6) -> [1: bb4, otherwise: bb3]; } bb1: { StorageLive(_3); StorageLive(_4); - _4 = begin_panic::<&str>(const "explicit panic") -> bb10; + _4 = begin_panic::<&str>(const "explicit panic") -> bb8; } bb2: { @@ -43,12 +43,11 @@ fn main() -> () { } bb3: { - FakeRead(ForMatchedPlace(None), _1); - unreachable; + goto -> bb7; } bb4: { - goto -> bb9; + falseEdge -> [real: bb6, imaginary: bb3]; } bb5: { @@ -56,14 +55,6 @@ fn main() -> () { } bb6: { - falseEdge -> [real: bb8, imaginary: bb4]; - } - - bb7: { - goto -> bb4; - } - - bb8: { _5 = ((_1 as Some).0: u8); _0 = const (); StorageDead(_5); @@ -71,12 +62,12 @@ fn main() -> () { return; } - bb9: { + bb7: { StorageDead(_5); goto -> bb1; } - bb10 (cleanup): { + bb8 (cleanup): { resume; } } diff --git a/tests/mir-opt/building/logical_or_in_conditional.test_complex.built.after.mir b/tests/mir-opt/building/logical_or_in_conditional.test_complex.built.after.mir index fd8eb370ca958..3e16efe6980d9 100644 --- a/tests/mir-opt/building/logical_or_in_conditional.test_complex.built.after.mir +++ b/tests/mir-opt/building/logical_or_in_conditional.test_complex.built.after.mir @@ -19,22 +19,21 @@ fn test_complex() -> () { bb0: { StorageLive(_1); StorageLive(_2); - _2 = E::f() -> [return: bb1, unwind: bb38]; + _2 = E::f() -> [return: bb1, unwind: bb34]; } bb1: { PlaceMention(_2); _3 = discriminant(_2); - switchInt(move _3) -> [0: bb5, otherwise: bb3]; + switchInt(move _3) -> [0: bb3, otherwise: bb2]; } bb2: { - FakeRead(ForMatchedPlace(None), _2); - unreachable; + goto -> bb21; } bb3: { - goto -> bb23; + falseEdge -> [real: bb5, imaginary: bb2]; } bb4: { @@ -42,175 +41,158 @@ fn test_complex() -> () { } bb5: { - falseEdge -> [real: bb7, imaginary: bb3]; + StorageLive(_4); + _4 = always_true() -> [return: bb6, unwind: bb34]; } bb6: { - goto -> bb3; + switchInt(move _4) -> [0: bb8, otherwise: bb7]; } bb7: { - StorageLive(_4); - _4 = always_true() -> [return: bb8, unwind: bb38]; - } - - bb8: { - switchInt(move _4) -> [0: bb10, otherwise: bb9]; - } - - bb9: { StorageLive(_5); StorageLive(_6); StorageLive(_7); _7 = Droppy(const 0_u8); _6 = (_7.0: u8); _5 = Gt(move _6, const 0_u8); - switchInt(move _5) -> [0: bb12, otherwise: bb11]; + switchInt(move _5) -> [0: bb10, otherwise: bb9]; } - bb10: { - goto -> bb16; + bb8: { + goto -> bb14; } - bb11: { - drop(_7) -> [return: bb13, unwind: bb38]; + bb9: { + drop(_7) -> [return: bb11, unwind: bb34]; } - bb12: { - goto -> bb14; + bb10: { + goto -> bb12; } - bb13: { + bb11: { StorageDead(_7); StorageDead(_6); - goto -> bb20; + goto -> bb18; } - bb14: { - drop(_7) -> [return: bb15, unwind: bb38]; + bb12: { + drop(_7) -> [return: bb13, unwind: bb34]; } - bb15: { + bb13: { StorageDead(_7); StorageDead(_6); - goto -> bb16; + goto -> bb14; } - bb16: { + bb14: { StorageLive(_8); StorageLive(_9); StorageLive(_10); _10 = Droppy(const 1_u8); _9 = (_10.0: u8); _8 = Gt(move _9, const 1_u8); - switchInt(move _8) -> [0: bb18, otherwise: bb17]; + switchInt(move _8) -> [0: bb16, otherwise: bb15]; } - bb17: { - drop(_10) -> [return: bb19, unwind: bb38]; + bb15: { + drop(_10) -> [return: bb17, unwind: bb34]; } - bb18: { - goto -> bb21; + bb16: { + goto -> bb19; } - bb19: { + bb17: { StorageDead(_10); StorageDead(_9); - goto -> bb20; + goto -> bb18; } - bb20: { + bb18: { _1 = const (); - goto -> bb24; + goto -> bb22; } - bb21: { - drop(_10) -> [return: bb22, unwind: bb38]; + bb19: { + drop(_10) -> [return: bb20, unwind: bb34]; } - bb22: { + bb20: { StorageDead(_10); StorageDead(_9); - goto -> bb23; + goto -> bb21; } - bb23: { + bb21: { _1 = const (); - goto -> bb24; + goto -> bb22; } - bb24: { + bb22: { StorageDead(_8); StorageDead(_5); StorageDead(_4); StorageDead(_2); StorageDead(_1); StorageLive(_11); - _11 = always_true() -> [return: bb25, unwind: bb38]; + _11 = always_true() -> [return: bb23, unwind: bb34]; } - bb25: { - switchInt(move _11) -> [0: bb27, otherwise: bb26]; + bb23: { + switchInt(move _11) -> [0: bb25, otherwise: bb24]; } - bb26: { - goto -> bb36; + bb24: { + goto -> bb32; } - bb27: { - goto -> bb28; + bb25: { + goto -> bb26; } - bb28: { + bb26: { StorageLive(_12); - _12 = E::f() -> [return: bb29, unwind: bb38]; + _12 = E::f() -> [return: bb27, unwind: bb34]; } - bb29: { + bb27: { PlaceMention(_12); _13 = discriminant(_12); - switchInt(move _13) -> [1: bb33, otherwise: bb31]; - } - - bb30: { - FakeRead(ForMatchedPlace(None), _12); - unreachable; + switchInt(move _13) -> [1: bb29, otherwise: bb28]; } - bb31: { - goto -> bb36; - } - - bb32: { - goto -> bb30; + bb28: { + goto -> bb32; } - bb33: { - falseEdge -> [real: bb35, imaginary: bb31]; + bb29: { + falseEdge -> [real: bb31, imaginary: bb28]; } - bb34: { - goto -> bb31; + bb30: { + goto -> bb28; } - bb35: { + bb31: { _0 = const (); - goto -> bb37; + goto -> bb33; } - bb36: { + bb32: { _0 = const (); - goto -> bb37; + goto -> bb33; } - bb37: { + bb33: { StorageDead(_11); StorageDead(_12); return; } - bb38 (cleanup): { + bb34 (cleanup): { resume; } } From ff49c3769b0ec5b1abc628e55c7547e765086e6a Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Mon, 17 Jun 2024 20:51:35 +0200 Subject: [PATCH 7/8] Reuse `lower_let_expr` for `let .. else` lowering --- compiler/rustc_mir_build/src/build/block.rs | 63 ++++++++-------- .../rustc_mir_build/src/build/matches/mod.rs | 73 +++++-------------- 2 files changed, 49 insertions(+), 87 deletions(-) diff --git a/compiler/rustc_mir_build/src/build/block.rs b/compiler/rustc_mir_build/src/build/block.rs index c1d645aa42cb8..476969a1bd7ad 100644 --- a/compiler/rustc_mir_build/src/build/block.rs +++ b/compiler/rustc_mir_build/src/build/block.rs @@ -189,38 +189,37 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let initializer_span = this.thir[*initializer].span; let scope = (*init_scope, source_info); - let failure = unpack!( - block = this.in_scope(scope, *lint_level, |this| { - this.declare_bindings( - visibility_scope, - remainder_span, - pattern, - None, - Some((Some(&destination), initializer_span)), - ); - this.visit_primary_bindings( - pattern, - UserTypeProjections::none(), - &mut |this, _, _, node, span, _, _| { - this.storage_live_binding( - block, - node, - span, - OutsideGuard, - true, - ); - }, - ); - this.ast_let_else( - block, - *initializer, - initializer_span, - *else_block, - &last_remainder_scope, - pattern, - ) - }) - ); + let failure_and_block = this.in_scope(scope, *lint_level, |this| { + this.declare_bindings( + visibility_scope, + remainder_span, + pattern, + None, + Some((Some(&destination), initializer_span)), + ); + this.visit_primary_bindings( + pattern, + UserTypeProjections::none(), + &mut |this, _, _, node, span, _, _| { + this.storage_live_binding(block, node, span, OutsideGuard, true); + }, + ); + let else_block_span = this.thir[*else_block].span; + let (matching, failure) = + this.in_if_then_scope(last_remainder_scope, else_block_span, |this| { + this.lower_let_expr( + block, + *initializer, + pattern, + None, + initializer_span, + false, + true, + ) + }); + matching.and(failure) + }); + let failure = unpack!(block = failure_and_block); this.cfg.goto(failure, source_info, failure_entry); if let Some(source_scope) = visibility_scope { diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index 3eacd598f66c2..f9333a165cef1 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -147,6 +147,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { Some(args.variable_source_info.scope), args.variable_source_info.span, args.declare_let_bindings, + false, ), _ => { let mut block = block; @@ -1981,48 +1982,50 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { 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. + /// `false` to avoid duplicated bindings declaration; used for if-let guards. pub(crate) fn lower_let_expr( &mut self, mut block: BasicBlock, expr_id: ExprId, pat: &Pat<'tcx>, source_scope: Option, - span: Span, + scope_span: Span, declare_bindings: bool, + storages_alive: bool, ) -> BlockAnd<()> { let expr_span = self.thir[expr_id].span; - let expr_place_builder = unpack!(block = self.lower_scrutinee(block, expr_id, expr_span)); - let mut guard_candidate = Candidate::new(expr_place_builder.clone(), pat, false, self); + let scrutinee = unpack!(block = self.lower_scrutinee(block, expr_id, expr_span)); + let mut candidate = Candidate::new(scrutinee.clone(), pat, false, self); let otherwise_block = self.lower_match_tree( block, + expr_span, + &scrutinee, pat.span, - &expr_place_builder, - pat.span, - &mut [&mut guard_candidate], + &mut [&mut candidate], true, ); - let expr_place = expr_place_builder.try_to_place(self); - let opt_expr_place = expr_place.as_ref().map(|place| (Some(place), expr_span)); + self.break_for_else(otherwise_block, self.source_info(expr_span)); if declare_bindings { - self.declare_bindings(source_scope, pat.span.to(span), pat, None, opt_expr_place); + 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); } - let post_guard_block = self.bind_pattern( + let success = self.bind_pattern( self.source_info(pat.span), - guard_candidate, + candidate, &[], expr_span, None, - false, + storages_alive, ); // If branch coverage is enabled, record this branch. - self.visit_coverage_conditional_let(pat, post_guard_block, otherwise_block); + self.visit_coverage_conditional_let(pat, success, otherwise_block); - post_guard_block.unit() + success.unit() } /// Initializes each of the bindings from the candidate by @@ -2469,44 +2472,4 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { debug!(?locals); self.var_indices.insert(var_id, locals); } - - pub(crate) fn ast_let_else( - &mut self, - mut block: BasicBlock, - init_id: ExprId, - initializer_span: Span, - else_block: BlockId, - let_else_scope: ®ion::Scope, - pattern: &Pat<'tcx>, - ) -> BlockAnd { - let else_block_span = self.thir[else_block].span; - let (matching, failure) = self.in_if_then_scope(*let_else_scope, else_block_span, |this| { - let scrutinee = unpack!(block = this.lower_scrutinee(block, init_id, initializer_span)); - let mut candidate = Candidate::new(scrutinee.clone(), pattern, false, this); - let failure_block = this.lower_match_tree( - block, - initializer_span, - &scrutinee, - pattern.span, - &mut [&mut candidate], - true, - ); - // This block is for the matching case - let matching = this.bind_pattern( - this.source_info(pattern.span), - candidate, - &[], - initializer_span, - None, - true, - ); - - // If branch coverage is enabled, record this branch. - this.visit_coverage_conditional_let(pattern, matching, failure_block); - - this.break_for_else(failure_block, this.source_info(initializer_span)); - matching.unit() - }); - matching.and(failure) - } } From beb1d35d7d63d331089620c60d591bab7e6495a5 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Sat, 22 Jun 2024 18:21:03 +0200 Subject: [PATCH 8/8] Change comment to reflect switch to THIR unsafeck --- .../rustc_mir_build/src/build/matches/mod.rs | 24 ++++++++----------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index f9333a165cef1..a92272c9809a2 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -1258,21 +1258,17 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { // cases. let source_info = self.source_info(scrutinee_span); - // Matching on a `scrutinee_place` with an uninhabited type doesn't - // generate any memory reads by itself, and so if the place "expression" - // contains unsafe operations like raw pointer dereferences or union - // field projections, we wouldn't know to require an `unsafe` block - // around a `match` equivalent to `std::intrinsics::unreachable()`. - // See issue #47412 for this hole being discovered in the wild. - // - // HACK(eddyb) Work around the above issue by adding a dummy inspection - // of `scrutinee_place`, specifically by applying `ReadForMatch`. + // Matching on a scrutinee place of an uninhabited type doesn't generate any memory + // reads by itself, and so if the place is uninitialized we wouldn't know. In order to + // disallow the following: + // ```rust + // let x: !; + // match x {} + // ``` + // we add a dummy read on the place. // - // NOTE: ReadForMatch also checks that the scrutinee is initialized. - // This is currently needed to not allow matching on an uninitialized, - // uninhabited value. If we get never patterns, those will check that - // the place is initialized, and so this read would only be used to - // check safety. + // NOTE: If we require never patterns for empty matches, those will check that the place + // is initialized, and so this read would no longer be needed. let cause_matched_place = FakeReadCause::ForMatchedPlace(None); if let Some(scrutinee_place) = scrutinee_place_builder.try_to_place(self) {