From 2c840ae18dc53d55192269adee39d77a8652b2f7 Mon Sep 17 00:00:00 2001 From: Matthew Jasper Date: Sat, 2 Feb 2019 16:38:12 +0000 Subject: [PATCH] Use normal mutable borrows in MIR match lowering --- src/librustc/ich/impls_mir.rs | 7 +- src/librustc/mir/mod.rs | 13 +- src/librustc_mir/borrow_check/mod.rs | 4 +- .../borrow_check/nll/invalidation.rs | 13 +- src/librustc_mir/build/block.rs | 6 +- src/librustc_mir/build/matches/mod.rs | 217 ++++++++---------- src/librustc_mir/build/mod.rs | 32 +-- src/test/mir-opt/match_false_edges.rs | 124 +++++----- .../ui/nll/match-guards-partially-borrow.rs | 57 +++-- .../nll/match-guards-partially-borrow.stderr | 62 ++--- 10 files changed, 227 insertions(+), 308 deletions(-) diff --git a/src/librustc/ich/impls_mir.rs b/src/librustc/ich/impls_mir.rs index 51fc78ffc866..ddc091b71870 100644 --- a/src/librustc/ich/impls_mir.rs +++ b/src/librustc/ich/impls_mir.rs @@ -196,7 +196,12 @@ impl_stable_hash_for!(impl<'gcx> for enum mir::StatementKind<'gcx> [ mir::Statem }); impl_stable_hash_for!(enum mir::RetagKind { FnEntry, TwoPhase, Raw, Default }); -impl_stable_hash_for!(enum mir::FakeReadCause { ForMatchGuard, ForMatchedPlace, ForLet }); +impl_stable_hash_for!(enum mir::FakeReadCause { + ForMatchGuard, + ForMatchedPlace, + ForGuardBinding, + ForLet +}); impl<'a, 'gcx> HashStable> for mir::Place<'gcx> { fn hash_stable(&self, diff --git a/src/librustc/mir/mod.rs b/src/librustc/mir/mod.rs index 3513d652b534..6c72a7c31591 100644 --- a/src/librustc/mir/mod.rs +++ b/src/librustc/mir/mod.rs @@ -1823,17 +1823,22 @@ pub enum RetagKind { /// The `FakeReadCause` describes the type of pattern why a `FakeRead` statement exists. #[derive(Copy, Clone, RustcEncodable, RustcDecodable, Debug)] pub enum FakeReadCause { - /// Inject a fake read of the borrowed input at the start of each arm's - /// pattern testing code. + /// Inject a fake read of the borrowed input at the end of each guards + /// code. /// - /// This should ensure that you cannot change the variant for an enum - /// while you are in the midst of matching on it. + /// This should ensure that you cannot change the variant for an enum while + /// you are in the midst of matching on it. ForMatchGuard, /// `let x: !; match x {}` doesn't generate any read of x so we need to /// generate a read of x to check that it is initialized and safe. ForMatchedPlace, + /// A fake read of the RefWithinGuard version of a bind-by-value variable + /// in a match guard to ensure that it's value hasn't change by the time + /// we create the OutsideGuard version. + ForGuardBinding, + /// Officially, the semantics of /// /// `let pattern = ;` diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index f7d079c5494e..1e9dab5016f8 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -998,7 +998,9 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } (Read(_), BorrowKind::Shared) | (Reservation(..), BorrowKind::Shared) - | (Read(_), BorrowKind::Shallow) | (Reservation(..), BorrowKind::Shallow) => { + | (Read(_), BorrowKind::Shallow) | (Reservation(..), BorrowKind::Shallow) + | (Read(ReadKind::Borrow(BorrowKind::Shallow)), BorrowKind::Unique) + | (Read(ReadKind::Borrow(BorrowKind::Shallow)), BorrowKind::Mut { .. }) => { Control::Continue } diff --git a/src/librustc_mir/borrow_check/nll/invalidation.rs b/src/librustc_mir/borrow_check/nll/invalidation.rs index 3255899c86cb..9c0676776210 100644 --- a/src/librustc_mir/borrow_check/nll/invalidation.rs +++ b/src/librustc_mir/borrow_check/nll/invalidation.rs @@ -78,13 +78,8 @@ impl<'cx, 'tcx, 'gcx> Visitor<'tcx> for InvalidationGenerator<'cx, 'tcx, 'gcx> { JustWrite ); } - StatementKind::FakeRead(_, ref place) => { - self.access_place( - ContextKind::FakeRead.new(location), - place, - (Deep, Read(ReadKind::Borrow(BorrowKind::Shared))), - LocalMutationIsAllowed::No, - ); + StatementKind::FakeRead(_, _) => { + // Only relavent for initialized/liveness/safety checks. } StatementKind::SetDiscriminant { ref place, @@ -438,7 +433,9 @@ impl<'cg, 'cx, 'tcx, 'gcx> InvalidationGenerator<'cx, 'tcx, 'gcx> { } (Read(_), BorrowKind::Shallow) | (Reservation(..), BorrowKind::Shallow) - | (Read(_), BorrowKind::Shared) | (Reservation(..), BorrowKind::Shared) => { + | (Read(_), BorrowKind::Shared) | (Reservation(..), BorrowKind::Shared) + | (Read(ReadKind::Borrow(BorrowKind::Shallow)), BorrowKind::Unique) + | (Read(ReadKind::Borrow(BorrowKind::Shallow)), BorrowKind::Mut { .. }) => { // Reads/reservations don't invalidate shared or shallow borrows } diff --git a/src/librustc_mir/build/block.rs b/src/librustc_mir/build/block.rs index 7d93e131a6ca..627fd7d2e166 100644 --- a/src/librustc_mir/build/block.rs +++ b/src/librustc_mir/build/block.rs @@ -6,8 +6,6 @@ use rustc::mir::*; use rustc::hir; use syntax_pos::Span; -use std::slice; - impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { pub fn ast_block(&mut self, destination: &Place<'tcx>, @@ -125,7 +123,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { None, remainder_span, lint_level, - slice::from_ref(&pattern), + &pattern, ArmHasGuard(false), Some((None, initializer_span)), ); @@ -138,7 +136,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { })); } else { scope = this.declare_bindings( - None, remainder_span, lint_level, slice::from_ref(&pattern), + None, remainder_span, lint_level, &pattern, ArmHasGuard(false), None); debug!("ast_block_stmts: pattern={:?}", pattern); diff --git a/src/librustc_mir/build/matches/mod.rs b/src/librustc_mir/build/matches/mod.rs index ae1de50dabfd..07c179ded593 100644 --- a/src/librustc_mir/build/matches/mod.rs +++ b/src/librustc_mir/build/matches/mod.rs @@ -6,7 +6,7 @@ //! function parameters. use crate::build::scope::{CachedBlock, DropKind}; -use crate::build::ForGuard::{self, OutsideGuard, RefWithinGuard, ValWithinGuard}; +use crate::build::ForGuard::{self, OutsideGuard, RefWithinGuard}; use crate::build::{BlockAnd, BlockAndExtension, Builder}; use crate::build::{GuardFrame, GuardFrameLocal, LocalsForNode}; use crate::hair::{self, *}; @@ -14,7 +14,7 @@ use rustc::mir::*; use rustc::ty::{self, CanonicalUserTypeAnnotation, Ty}; use rustc::ty::layout::VariantIdx; use rustc_data_structures::bit_set::BitSet; -use rustc_data_structures::fx::FxHashMap; +use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use syntax::ast::{Name, NodeId}; use syntax_pos::Span; @@ -71,7 +71,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { /// /// ## Fake Reads and borrows /// - /// Match exhaustiveness checking is no able to handle the case where the + /// Match exhaustiveness checking is not able to handle the case where the /// place being matched on is mutated in the guards. There is an AST check /// that tries to stop this but it is buggy and overly restrictive. Instead /// we add "fake borrows" to the guards that prevent any mutation of the @@ -84,7 +84,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { /// borrows of `x`, so we only add fake borrows for places which are /// bound or tested by the match. /// 3. We don't want the fake borrows to conflict with `ref mut` bindings, - /// so we lower `ref mut` bindings as two-phase borrows for the guard. + /// so we use a special BorrowKind for them. /// 4. The fake borrows may be of places in inactive variants, so it would /// be UB to generate code for them. They therefore have to be removed /// by a MIR pass run after borrow checking. @@ -145,8 +145,8 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { .map(|_| self.cfg.start_new_block()) .collect(); - // There's one move pre_binding block that there are candidates so that - // every candidate has a next prebinding_block. + // There's one more pre_binding block than there are candidates so that + // every candidate can have a `next_candidate_pre_binding_block`. let outer_source_info = self.source_info(span); self.cfg.terminate( *pre_binding_blocks.last().unwrap(), @@ -197,13 +197,11 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { // Step 3. Create the decision tree and record the places that we bind or test. - // Maps a place to the kind of Fake borrow that we want to perform on - // it: either Shallow or Shared, depending on whether the place is - // bound in the match, or just switched on. - // If there are no match guards then we don't need any fake borrows, - // so don't track them. + // 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 mut fake_borrows = if match_has_guard && tcx.generate_borrow_of_any_match_input() { - Some(FxHashMap::default()) + Some(FxHashSet::default()) } else { None }; @@ -265,19 +263,18 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { None, body.span, LintLevel::Inherited, - &arm.patterns[..], + &arm.patterns[0], ArmHasGuard(arm.guard.is_some()), Some((Some(&scrutinee_place), scrutinee_span)), ); - for (pat_index, candidate) in candidates.into_iter().enumerate() { + for candidate in candidates { self.bind_and_guard_matched_candidate( candidate, arm.guard.clone(), arm_block, &fake_borrow_temps, scrutinee_span, - pat_index, ); } @@ -482,7 +479,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { mut visibility_scope: Option, scope_span: Span, lint_level: LintLevel, - patterns: &[Pattern<'tcx>], + pattern: &Pattern<'tcx>, has_guard: ArmHasGuard, opt_match_place: Option<(Option<&Place<'tcx>>, Span)>, ) -> Option { @@ -491,10 +488,9 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { "can't have both a visibility and a lint scope at the same time" ); let mut scope = self.source_scope; - let num_patterns = patterns.len(); - debug!("declare_bindings: patterns={:?}", patterns); + debug!("declare_bindings: pattern={:?}", pattern); self.visit_bindings( - &patterns[0], + &pattern, UserTypeProjections::none(), &mut |this, mutability, name, mode, var, span, ty, user_ty| { if visibility_scope.is_none() { @@ -515,13 +511,12 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { mutability, name, mode, - num_patterns, var, ty, user_ty, has_guard, opt_match_place.map(|(x, y)| (x.cloned(), y)), - patterns[0].span, + pattern.span, ); }, ); @@ -804,7 +799,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { span: Span, candidates: &mut [&mut Candidate<'pat, 'tcx>], mut block: BasicBlock, - fake_borrows: &mut Option, BorrowKind>>, + fake_borrows: &mut Option>>, ) -> Vec { debug!( "matched_candidate(span={:?}, block={:?}, candidates={:?})", @@ -901,19 +896,41 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { &mut self, matched_candidates: &mut [&mut Candidate<'_, 'tcx>], block: BasicBlock, - fake_borrows: &mut Option, BorrowKind>>, + fake_borrows: &mut Option>>, ) -> Option { debug_assert!( !matched_candidates.is_empty(), "select_matched_candidates called with no candidates", ); - // Insert a *Shared* borrow of any places that are bound. + // Insert a borrows of prefixes of places that are bound and are + // behind a dereference projection. + // + // These borrows are taken to avoid situations like the following: + // + // match x[10] { + // _ if { x = &[0]; false } => (), + // y => (), // Out of bounds array access! + // } + // + // match *x { + // // y is bound by reference in the guard and then by copy in the + // // arm, so y is 2 in the arm! + // y if { y == 1 && (x = &2) == () } => y, + // _ => 3, + // } if let Some(fake_borrows) = fake_borrows { for Binding { source, .. } in matched_candidates.iter().flat_map(|candidate| &candidate.bindings) { - fake_borrows.insert(source.clone(), BorrowKind::Shared); + let mut cursor = source; + while let Place::Projection(box Projection { base, elem }) = cursor { + cursor = base; + if let ProjectionElem::Deref = elem { + fake_borrows.insert(cursor.clone()); + break; + } + } } } @@ -959,8 +976,6 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { } } - - // if None is returned, then debug!("match_candidates: add false edges for unreachable {:?}", unreachable_candidates); for candidate in unreachable_candidates { if let Some(otherwise) = candidate.otherwise_block { @@ -1133,7 +1148,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { span: Span, mut candidates: &'b mut [&'c mut Candidate<'pat, 'tcx>], block: BasicBlock, - fake_borrows: &mut Option, BorrowKind>>, + fake_borrows: &mut Option>>, ) -> (Vec, &'b mut [&'c mut Candidate<'pat, 'tcx>]) { // extract the match-pair from the highest priority candidate let match_pair = &candidates.first().unwrap().match_pairs[0]; @@ -1177,7 +1192,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { // Insert a Shallow borrow of any places that is switched on. fake_borrows.as_mut().map(|fb| { - fb.entry(match_place.clone()).or_insert(BorrowKind::Shallow) + fb.insert(match_place.clone()) }); // perform the test, branching to one of N blocks. For each of @@ -1236,9 +1251,9 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { // will evaluate to the same thing until an arm has been chosen. fn calculate_fake_borrows<'b>( &mut self, - fake_borrows: &'b FxHashMap, BorrowKind>, + fake_borrows: &'b FxHashSet>, temp_span: Span, - ) -> Vec<(&'b Place<'tcx>, BorrowKind, Local)> { + ) -> Vec<(&'b Place<'tcx>, Local)> { let tcx = self.hir.tcx(); debug!("add_fake_borrows fake_borrows = {:?}", fake_borrows); @@ -1246,7 +1261,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { let mut all_fake_borrows = Vec::with_capacity(fake_borrows.len()); // Insert a Shallow borrow of the prefixes of any fake borrows. - for (place, borrow_kind) in fake_borrows + for place in fake_borrows { let mut prefix_cursor = place; while let Place::Projection(box Projection { base, elem }) = prefix_cursor { @@ -1254,12 +1269,12 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { // Insert a shallow borrow after a deref. For other // projections the borrow of prefix_cursor will // conflict with any mutation of base. - all_fake_borrows.push((base, BorrowKind::Shallow)); + all_fake_borrows.push(base); } prefix_cursor = base; } - all_fake_borrows.push((place, *borrow_kind)); + all_fake_borrows.push(place); } // Deduplicate and ensure a deterministic order. @@ -1268,14 +1283,14 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { debug!("add_fake_borrows all_fake_borrows = {:?}", all_fake_borrows); - all_fake_borrows.into_iter().map(|(matched_place, borrow_kind)| { + all_fake_borrows.into_iter().map(|matched_place| { let fake_borrow_deref_ty = matched_place.ty(&self.local_decls, tcx).to_ty(tcx); let fake_borrow_ty = tcx.mk_imm_ref(tcx.types.re_erased, fake_borrow_deref_ty); let fake_borrow_temp = self.local_decls.push( LocalDecl::new_temp(fake_borrow_ty, temp_span) ); - (matched_place, borrow_kind, fake_borrow_temp) + (matched_place, fake_borrow_temp) }).collect() } } @@ -1301,9 +1316,8 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { candidate: Candidate<'pat, 'tcx>, guard: Option>, arm_block: BasicBlock, - fake_borrows: &Vec<(&Place<'tcx>, BorrowKind, Local)>, + fake_borrows: &Vec<(&Place<'tcx>, Local)>, scrutinee_span: Span, - pat_index: usize, ) { debug!("bind_and_guard_matched_candidate(candidate={:?})", candidate); @@ -1399,18 +1413,15 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { // // * Here, the guard expression wants a `&&` or `&&mut` // into the original input. This means we need to borrow - // a reference that we do not immediately have at hand - // (because all we have is the places associated with the - // match input itself; it is up to us to create a place - // holding a `&` or `&mut` that we can then borrow). - + // the reference that we create for the arm. + // * So we eagerly create the reference for the arm and then take a + // reference to that. let tcx = self.hir.tcx(); let autoref = tcx.all_pat_vars_are_implicit_refs_within_guards(); if let Some(guard) = guard { if autoref { self.bind_matched_candidate_for_guard( block, - pat_index, &candidate.bindings, ); let guard_frame = GuardFrame { @@ -1428,10 +1439,10 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { let re_erased = tcx.types.re_erased; let scrutinee_source_info = self.source_info(scrutinee_span); - for &(place, borrow_kind, temp) in fake_borrows { + for &(place, temp) in fake_borrows { let borrow = Rvalue::Ref( re_erased, - borrow_kind, + BorrowKind::Shallow, place.clone(), ); self.cfg.push_assign( @@ -1458,7 +1469,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { ); } - for &(_, _, temp) in fake_borrows { + for &(_, temp) in fake_borrows { self.cfg.push(block, Statement { source_info: guard_end, kind: StatementKind::FakeRead( @@ -1507,7 +1518,26 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { ); if autoref { - self.bind_matched_candidate_for_arm_body(post_guard_block, &candidate.bindings); + let by_value_bindings = candidate.bindings.iter().filter(|binding| { + if let BindingMode::ByValue = binding.binding_mode { true } else { false } + }); + // 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() { + let local_id = self.var_local_id(binding.var_id, RefWithinGuard); + let place = Place::Local(local_id); + self.cfg.push( + block, + Statement { + source_info: guard_end, + kind: StatementKind::FakeRead(FakeReadCause::ForGuardBinding, place), + }, + ); + } + self.bind_matched_candidate_for_arm_body( + post_guard_block, + by_value_bindings, + ); } self.cfg.terminate( @@ -1570,13 +1600,9 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { fn bind_matched_candidate_for_guard( &mut self, block: BasicBlock, - pat_index: usize, bindings: &[Binding<'tcx>], ) { - debug!( - "bind_matched_candidate_for_guard(block={:?}, pat_index={:?}, bindings={:?})", - block, pat_index, bindings - ); + debug!("bind_matched_candidate_for_guard(block={:?}, bindings={:?})", block, bindings); // Assign each of the bindings. Since we are binding for a // guard expression, this will never trigger moves out of the @@ -1603,49 +1629,22 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { .push_assign(block, source_info, &ref_for_guard, rvalue); } BindingMode::ByRef(borrow_kind) => { - // Tricky business: For `ref id` and `ref mut id` - // patterns, we want `id` within the guard to - // correspond to a temp of type `& &T` or `& &mut - // T` (i.e., a "borrow of a borrow") that is - // implicitly dereferenced. - // - // To borrow a borrow, we need that inner borrow - // to point to. So, create a temp for the inner - // borrow, and then take a reference to it. - // - // Note: the temp created here is *not* the one - // used by the arm body itself. This eases - // observing two-phase borrow restrictions. - let val_for_guard = self.storage_live_binding( + let value_for_arm = self.storage_live_binding( block, binding.var_id, binding.span, - ValWithinGuard(pat_index), + OutsideGuard, ); self.schedule_drop_for_binding( binding.var_id, binding.span, - ValWithinGuard(pat_index), + OutsideGuard, ); - // rust-lang/rust#27282: We reuse the two-phase - // borrow infrastructure so that the mutable - // borrow (whose mutabilty is *unusable* within - // the guard) does not conflict with the implicit - // borrow of the whole match input. See additional - // discussion on rust-lang/rust#49870. - let borrow_kind = match borrow_kind { - BorrowKind::Shared - | BorrowKind::Shallow - | BorrowKind::Unique => borrow_kind, - BorrowKind::Mut { .. } => BorrowKind::Mut { - allow_two_phase_borrow: true, - }, - }; let rvalue = Rvalue::Ref(re_erased, borrow_kind, binding.source.clone()); self.cfg - .push_assign(block, source_info, &val_for_guard, rvalue); - let rvalue = Rvalue::Ref(re_erased, BorrowKind::Shared, val_for_guard); + .push_assign(block, source_info, &value_for_arm, rvalue); + let rvalue = Rvalue::Ref(re_erased, BorrowKind::Shared, value_for_arm); self.cfg .push_assign(block, source_info, &ref_for_guard, rvalue); } @@ -1653,16 +1652,12 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { } } - fn bind_matched_candidate_for_arm_body( + fn bind_matched_candidate_for_arm_body<'b>( &mut self, block: BasicBlock, - bindings: &[Binding<'tcx>], - ) { - debug!( - "bind_matched_candidate_for_arm_body(block={:?}, bindings={:?}", - block, bindings - ); - + bindings: impl IntoIterator>, + ) where 'tcx: 'b { + debug!("bind_matched_candidate_for_arm_body(block={:?})", block); let re_erased = self.hir.tcx().types.re_erased; // Assign each of the bindings. This may trigger moves out of the candidate. @@ -1683,21 +1678,11 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { } } - /// Each binding (`ref mut var`/`ref var`/`mut var`/`var`, where - /// the bound `var` has type `T` in the arm body) in a pattern - /// maps to `2+N` locals. The first local is a binding for - /// occurrences of `var` in the guard, which will all have type - /// `&T`. The N locals are bindings for the `T` that is referenced - /// by the first local; they are not used outside of the - /// guard. The last local is a binding for occurrences of `var` in - /// the arm body, which will have type `T`. - /// - /// The reason we have N locals rather than just 1 is to - /// accommodate rust-lang/rust#51348: If the arm has N candidate - /// patterns, then in general they can correspond to distinct - /// parts of the matched data, and we want them to be distinct - /// temps in order to simplify checks performed by our internal - /// leveraging of two-phase borrows). + /// Each binding (`ref mut var`/`ref var`/`mut var`/`var`, where the bound + /// `var` has type `T` in the arm body) in a pattern maps to 2 locals. The + /// first local is a binding for occurrences of `var` in the guard, which + /// will have type `&T`. The second local is a binding for occurrences of + /// `var` in the arm body, which will have type `T`. fn declare_binding( &mut self, source_info: SourceInfo, @@ -1705,7 +1690,6 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { mutability: Mutability, name: Name, mode: BindingMode, - num_patterns: usize, var_id: NodeId, var_ty: Ty<'tcx>, user_ty: UserTypeProjections<'tcx>, @@ -1747,31 +1731,20 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { }; let for_arm_body = self.local_decls.push(local.clone()); let locals = if has_guard.0 && tcx.all_pat_vars_are_implicit_refs_within_guards() { - let mut vals_for_guard = Vec::with_capacity(num_patterns); - for _ in 0..num_patterns { - let val_for_guard_idx = self.local_decls.push(LocalDecl { - // This variable isn't mutated but has a name, so has to be - // immutable to avoid the unused mut lint. - mutability: Mutability::Not, - ..local.clone() - }); - vals_for_guard.push(val_for_guard_idx); - } let ref_for_guard = self.local_decls.push(LocalDecl::<'tcx> { - // See previous comment. + // This variable isn't mutated but has a name, so has to be + // immutable to avoid the unused mut lint. mutability: Mutability::Not, ty: tcx.mk_imm_ref(tcx.types.re_erased, var_ty), user_ty: UserTypeProjections::none(), name: Some(name), source_info, visibility_scope, - // FIXME: should these secretly injected ref_for_guard's be marked as `internal`? internal: false, is_block_tail: None, is_user_variable: Some(ClearCrossCrate::Set(BindingForm::RefForGuard)), }); LocalsForNode::ForGuard { - vals_for_guard, ref_for_guard, for_arm_body, } diff --git a/src/librustc_mir/build/mod.rs b/src/librustc_mir/build/mod.rs index 64ab491cbd5b..903c8f8657f3 100644 --- a/src/librustc_mir/build/mod.rs +++ b/src/librustc_mir/build/mod.rs @@ -459,8 +459,7 @@ enum LocalsForNode { /// The exceptional case is identifiers in a match arm's pattern /// that are referenced in a guard of that match arm. For these, - /// we can have `2 + k` Locals, where `k` is the number of candidate - /// patterns (separated by `|`) in the arm. + /// we have `2` Locals. /// /// * `for_arm_body` is the Local used in the arm body (which is /// just like the `One` case above), @@ -468,16 +467,7 @@ enum LocalsForNode { /// * `ref_for_guard` is the Local used in the arm's guard (which /// is a reference to a temp that is an alias of /// `for_arm_body`). - /// - /// * `vals_for_guard` is the `k` Locals; at most one of them will - /// get initialized by the arm's execution, and after it is - /// initialized, `ref_for_guard` will be assigned a reference to - /// it. - /// - /// There reason we have `k` Locals rather than just 1 is to - /// accommodate some restrictions imposed by two-phase borrows, - /// which apply when we have a `ref mut` pattern. - ForGuard { vals_for_guard: Vec, ref_for_guard: Local, for_arm_body: Local }, + ForGuard { ref_for_guard: Local, for_arm_body: Local }, } #[derive(Debug)] @@ -510,16 +500,11 @@ struct GuardFrame { } /// `ForGuard` indicates whether we are talking about: -/// 1. the temp for a local binding used solely within guard expressions, -/// 2. the temp that holds reference to (1.), which is actually what the -/// guard expressions see, or -/// 3. the temp for use outside of guard expressions. +/// 1. The variable for use outside of guard expressions, or +/// 2. The temp that holds reference to (1.), which is actually what the +/// guard expressions see. #[derive(Copy, Clone, Debug, PartialEq, Eq)] enum ForGuard { - /// The `usize` identifies for which candidate pattern we want the - /// local binding. We keep a temp per-candidate to accommodate - /// two-phase borrows (see `LocalsForNode` documentation). - ValWithinGuard(usize), RefWithinGuard, OutsideGuard, } @@ -532,11 +517,6 @@ impl LocalsForNode { (&LocalsForNode::ForGuard { for_arm_body: local_id, .. }, ForGuard::OutsideGuard) => local_id, - (&LocalsForNode::ForGuard { ref vals_for_guard, .. }, - ForGuard::ValWithinGuard(pat_idx)) => - vals_for_guard[pat_idx], - - (&LocalsForNode::One(_), ForGuard::ValWithinGuard(_)) | (&LocalsForNode::One(_), ForGuard::RefWithinGuard) => bug!("anything with one local should never be within a guard."), } @@ -941,7 +921,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { } _ => { scope = self.declare_bindings(scope, ast_body.span, - LintLevel::Inherited, &[pattern.clone()], + LintLevel::Inherited, &pattern, matches::ArmHasGuard(false), Some((Some(&place), span))); unpack!(block = self.place_into_pattern(block, pattern, &place, false)); diff --git a/src/test/mir-opt/match_false_edges.rs b/src/test/mir-opt/match_false_edges.rs index 9faecf5b7af4..cf650b5a0ba5 100644 --- a/src/test/mir-opt/match_false_edges.rs +++ b/src/test/mir-opt/match_false_edges.rs @@ -73,34 +73,33 @@ fn main() { // return; // } // bb9: { // binding1 and guard -// StorageLive(_8); -// _8 = &(((promoted[2]: std::option::Option) as Some).0: i32); -// _4 = &shallow (promoted[1]: std::option::Option); -// _5 = &(((promoted[0]: std::option::Option) as Some).0: i32); -// StorageLive(_9); -// _9 = const guard() -> [return: bb10, unwind: bb1]; +// StorageLive(_6); +// _6 = &(((promoted[1]: std::option::Option) as Some).0: i32); +// _4 = &shallow (promoted[0]: std::option::Option); +// StorageLive(_7); +// _7 = const guard() -> [return: bb10, unwind: bb1]; // } // bb10: { // FakeRead(ForMatchGuard, _4); -// FakeRead(ForMatchGuard, _5); -// switchInt(move _9) -> [false: bb6, otherwise: bb11]; +// FakeRead(ForGuardBinding, _6); +// switchInt(move _7) -> [false: bb6, otherwise: bb11]; // } // bb11: { -// StorageLive(_6); -// _6 = ((_2 as Some).0: i32); -// StorageLive(_10); -// _10 = _6; -// _1 = (const 1i32, move _10); -// StorageDead(_10); +// StorageLive(_5); +// _5 = ((_2 as Some).0: i32); +// StorageLive(_8); +// _8 = _5; +// _1 = (const 1i32, move _8); +// StorageDead(_8); // goto -> bb8; // } // bb12: { -// StorageLive(_11); -// _11 = ((_2 as Some).0: i32); -// StorageLive(_12); -// _12 = _11; -// _1 = (const 2i32, move _12); -// StorageDead(_12); +// StorageLive(_9); +// _9 = ((_2 as Some).0: i32); +// StorageLive(_10); +// _10 = _9; +// _1 = (const 2i32, move _10); +// StorageDead(_10); // goto -> bb8; // } // bb13: { @@ -143,25 +142,24 @@ fn main() { // return; // } // bb9: { // binding1 and guard -// StorageLive(_8); -// _8 = &((_2 as Some).0: i32); +// StorageLive(_6); +// _6 = &((_2 as Some).0: i32); // _4 = &shallow _2; -// _5 = &((_2 as Some).0: i32); -// StorageLive(_9); -// _9 = const guard() -> [return: bb10, unwind: bb1]; +// StorageLive(_7); +// _7 = const guard() -> [return: bb10, unwind: bb1]; // } // bb10: { // end of guard // FakeRead(ForMatchGuard, _4); -// FakeRead(ForMatchGuard, _5); -// switchInt(move _9) -> [false: bb6, otherwise: bb11]; +// FakeRead(ForGuardBinding, _6); +// switchInt(move _7) -> [false: bb6, otherwise: bb11]; // } // bb11: { // arm1 -// StorageLive(_6); -// _6 = ((_2 as Some).0: i32); -// StorageLive(_10); -// _10 = _6; -// _1 = (const 1i32, move _10); -// StorageDead(_10); +// StorageLive(_5); +// _5 = ((_2 as Some).0: i32); +// StorageLive(_8); +// _8 = _5; +// _1 = (const 1i32, move _8); +// StorageDead(_8); // goto -> bb8; // } // bb12: { // arm2 @@ -169,12 +167,12 @@ fn main() { // goto -> bb8; // } // bb13: { // binding3 and arm3 -// StorageLive(_11); -// _11 = ((_2 as Some).0: i32); -// StorageLive(_12); -// _12 = _11; -// _1 = (const 2i32, move _12); -// StorageDead(_12); +// StorageLive(_9); +// _9 = ((_2 as Some).0: i32); +// StorageLive(_10); +// _10 = _9; +// _1 = (const 2i32, move _10); +// StorageDead(_10); // goto -> bb8; // } // END rustc.full_tested_match2.QualifyAndPromoteConstants.before.mir @@ -216,55 +214,53 @@ fn main() { // return; // } // bb10: { // binding1: Some(w) if guard() -// StorageLive(_9); -// _9 = &((_2 as Some).0: i32); +// StorageLive(_7); +// _7 = &((_2 as Some).0: i32); // _5 = &shallow _2; -// _6 = &((_2 as Some).0: i32); -// StorageLive(_10); -// _10 = const guard() -> [return: bb11, unwind: bb1]; +// StorageLive(_8); +// _8 = const guard() -> [return: bb11, unwind: bb1]; // } // bb11: { //end of guard // FakeRead(ForMatchGuard, _5); -// FakeRead(ForMatchGuard, _6); -// switchInt(move _10) -> [false: bb7, otherwise: bb12]; +// FakeRead(ForGuardBinding, _7); +// switchInt(move _8) -> [false: bb7, otherwise: bb12]; // } // bb12: { // set up bindings for arm1 -// StorageLive(_7); -// _7 = ((_2 as Some).0: i32); +// StorageLive(_6); +// _6 = ((_2 as Some).0: i32); // _1 = const 1i32; // goto -> bb9; // } // bb13: { // binding2 & arm2 -// StorageLive(_11); -// _11 = _2; +// StorageLive(_9); +// _9 = _2; // _1 = const 2i32; // goto -> bb9; // } // bb14: { // binding3: Some(y) if guard2(y) -// StorageLive(_14); -// _14 = &((_2 as Some).0: i32); +// StorageLive(_11); +// _11 = &((_2 as Some).0: i32); // _5 = &shallow _2; -// _6 = &((_2 as Some).0: i32); -// StorageLive(_15); -// StorageLive(_16); -// _16 = (*_14); -// _15 = const guard2(move _16) -> [return: bb15, unwind: bb1]; +// StorageLive(_12); +// StorageLive(_13); +// _13 = (*_11); +// _12 = const guard2(move _13) -> [return: bb15, unwind: bb1]; // } // bb15: { // end of guard2 -// StorageDead(_16); +// StorageDead(_13); // FakeRead(ForMatchGuard, _5); -// FakeRead(ForMatchGuard, _6); -// switchInt(move _15) -> [false: bb8, otherwise: bb16]; +// FakeRead(ForGuardBinding, _11); +// switchInt(move _12) -> [false: bb8, otherwise: bb16]; // } // bb16: { // binding4 & arm4 -// StorageLive(_12); -// _12 = ((_2 as Some).0: i32); +// StorageLive(_10); +// _10 = ((_2 as Some).0: i32); // _1 = const 3i32; // goto -> bb9; // } // bb17: { -// StorageLive(_17); -// _17 = _2; +// StorageLive(_14); +// _14 = _2; // _1 = const 4i32; // goto -> bb9; // } diff --git a/src/test/ui/nll/match-guards-partially-borrow.rs b/src/test/ui/nll/match-guards-partially-borrow.rs index cb8ea2c3d26b..6e158713146f 100644 --- a/src/test/ui/nll/match-guards-partially-borrow.rs +++ b/src/test/ui/nll/match-guards-partially-borrow.rs @@ -17,6 +17,30 @@ fn ok_mutation_in_guard(mut q: i32) { } } +fn ok_mutation_in_guard2(mut u: bool) { + // OK value of u is unused before modification + match u { + _ => (), + _ if { + u = true; + false + } => (), + x => (), + } +} + +fn ok_mutation_in_guard4(mut w: (&mut bool,)) { + // OK value of u is unused before modification + match w { + _ => (), + _ if { + *w.0 = true; + false + } => (), + x => (), + } +} + fn ok_indirect_mutation_in_guard(mut p: &bool) { match *p { // OK, mutation doesn't change which patterns s matches @@ -53,8 +77,8 @@ fn mutation_invalidates_previous_pattern_in_guard(mut r: bool) { fn match_on_borrowed_early_end(mut s: bool) { let h = &mut s; - match s { //~ ERROR - // s changes value between the start of the match and when its value is checked. + // OK value of s is unused before modification. + match s { _ if { *h = !*h; false @@ -75,19 +99,7 @@ fn bad_mutation_in_guard(mut t: bool) { } } -fn bad_mutation_in_guard2(mut u: bool) { - match u { - // Guard changes the value bound in the last pattern. - _ => (), - _ if { - u = true; //~ ERROR - false - } => (), - x => (), - } -} - -pub fn bad_mutation_in_guard3(mut x: Option>) { +fn bad_mutation_in_guard2(mut x: Option>) { // Check that nested patterns are checked. match x { None => (), @@ -103,20 +115,7 @@ pub fn bad_mutation_in_guard3(mut x: Option>) { } } - -fn bad_mutation_in_guard4(mut w: (&mut bool,)) { - match w { - // Guard changes the value bound in the last pattern. - _ => (), - _ if { - *w.0 = true; //~ ERROR - false - } => (), - x => (), - } -} - -fn bad_mutation_in_guard5(mut t: bool) { +fn bad_mutation_in_guard3(mut t: bool) { match t { s if { t = !t; //~ ERROR diff --git a/src/test/ui/nll/match-guards-partially-borrow.stderr b/src/test/ui/nll/match-guards-partially-borrow.stderr index b6302b3a65d3..baff2fda9f5d 100644 --- a/src/test/ui/nll/match-guards-partially-borrow.stderr +++ b/src/test/ui/nll/match-guards-partially-borrow.stderr @@ -1,5 +1,5 @@ error[E0510]: cannot assign `q` in match guard - --> $DIR/match-guards-partially-borrow.rs:35:13 + --> $DIR/match-guards-partially-borrow.rs:59:13 | LL | match q { | - value is immutable in match guard @@ -8,7 +8,7 @@ LL | q = true; //~ ERROR | ^^^^^^^^ cannot assign error[E0510]: cannot assign `r` in match guard - --> $DIR/match-guards-partially-borrow.rs:47:13 + --> $DIR/match-guards-partially-borrow.rs:71:13 | LL | match r { | - value is immutable in match guard @@ -16,19 +16,8 @@ LL | match r { LL | r = true; //~ ERROR | ^^^^^^^^ cannot assign -error[E0503]: cannot use `s` because it was mutably borrowed - --> $DIR/match-guards-partially-borrow.rs:56:11 - | -LL | let h = &mut s; - | ------ borrow of `s` occurs here -LL | match s { //~ ERROR - | ^ use of borrowed `s` -... -LL | *h = !*h; - | -- borrow later used here - error[E0510]: cannot assign `t` in match guard - --> $DIR/match-guards-partially-borrow.rs:71:13 + --> $DIR/match-guards-partially-borrow.rs:95:13 | LL | match t { | - value is immutable in match guard @@ -36,20 +25,8 @@ LL | match t { LL | t = true; //~ ERROR | ^^^^^^^^ cannot assign -error[E0506]: cannot assign to `u` because it is borrowed - --> $DIR/match-guards-partially-borrow.rs:83:13 - | -LL | match u { - | - borrow of `u` occurs here -... -LL | u = true; //~ ERROR - | ^^^^^^^^ assignment to borrowed `u` occurs here -LL | false -LL | } => (), - | - borrow later used here - error[E0510]: cannot mutably borrow `x.0` in match guard - --> $DIR/match-guards-partially-borrow.rs:97:22 + --> $DIR/match-guards-partially-borrow.rs:109:22 | LL | match x { | - value is immutable in match guard @@ -57,24 +34,11 @@ LL | match x { LL | Some(ref mut r) => *r = None, //~ ERROR | ^^^^^^^^^ cannot mutably borrow -error[E0506]: cannot assign to `*w.0` because it is borrowed - --> $DIR/match-guards-partially-borrow.rs:112:13 - | -LL | match w { - | - borrow of `*w.0` occurs here -... -LL | *w.0 = true; //~ ERROR - | ^^^^^^^^^^^ assignment to borrowed `*w.0` occurs here -LL | false -LL | } => (), - | - borrow later used here - error[E0506]: cannot assign to `t` because it is borrowed - --> $DIR/match-guards-partially-borrow.rs:122:13 + --> $DIR/match-guards-partially-borrow.rs:121:13 | -LL | match t { - | - borrow of `t` occurs here LL | s if { + | - borrow of `t` occurs here LL | t = !t; //~ ERROR | ^^^^^^ assignment to borrowed `t` occurs here LL | false @@ -82,7 +46,7 @@ LL | } => (), // What value should `s` have in the arm? | - borrow later used here error[E0510]: cannot assign `y` in match guard - --> $DIR/match-guards-partially-borrow.rs:133:13 + --> $DIR/match-guards-partially-borrow.rs:132:13 | LL | match *y { | -- value is immutable in match guard @@ -91,7 +55,7 @@ LL | y = &true; //~ ERROR | ^^^^^^^^^ cannot assign error[E0510]: cannot assign `z` in match guard - --> $DIR/match-guards-partially-borrow.rs:144:13 + --> $DIR/match-guards-partially-borrow.rs:143:13 | LL | match z { | - value is immutable in match guard @@ -100,7 +64,7 @@ LL | z = &true; //~ ERROR | ^^^^^^^^^ cannot assign error[E0510]: cannot assign `a` in match guard - --> $DIR/match-guards-partially-borrow.rs:156:13 + --> $DIR/match-guards-partially-borrow.rs:155:13 | LL | match a { | - value is immutable in match guard @@ -109,7 +73,7 @@ LL | a = &true; //~ ERROR | ^^^^^^^^^ cannot assign error[E0510]: cannot assign `b` in match guard - --> $DIR/match-guards-partially-borrow.rs:167:13 + --> $DIR/match-guards-partially-borrow.rs:166:13 | LL | match b { | - value is immutable in match guard @@ -117,7 +81,7 @@ LL | match b { LL | b = &true; //~ ERROR | ^^^^^^^^^ cannot assign -error: aborting due to 12 previous errors +error: aborting due to 9 previous errors -Some errors occurred: E0503, E0506, E0510. -For more information about an error, try `rustc --explain E0503`. +Some errors occurred: E0506, E0510. +For more information about an error, try `rustc --explain E0506`.