diff options
37 files changed, 790 insertions, 235 deletions
diff --git a/src/librustc/ich/impls_mir.rs b/src/librustc/ich/impls_mir.rs index c71b10ce142..e77d38de582 100644 --- a/src/librustc/ich/impls_mir.rs +++ b/src/librustc/ich/impls_mir.rs @@ -241,6 +241,9 @@ for mir::StatementKind<'gcx> { place.hash_stable(hcx, hasher); rvalue.hash_stable(hcx, hasher); } + mir::StatementKind::ReadForMatch(ref place) => { + place.hash_stable(hcx, hasher); + } mir::StatementKind::SetDiscriminant { ref place, variant_index } => { place.hash_stable(hcx, hasher); variant_index.hash_stable(hcx, hasher); diff --git a/src/librustc/mir/mod.rs b/src/librustc/mir/mod.rs index d35884ec78a..a94e5e793b4 100644 --- a/src/librustc/mir/mod.rs +++ b/src/librustc/mir/mod.rs @@ -1225,6 +1225,10 @@ pub enum StatementKind<'tcx> { /// Write the RHS Rvalue to the LHS Place. Assign(Place<'tcx>, Rvalue<'tcx>), + /// This represents all the reading that a pattern match may do + /// (e.g. inspecting constants and discriminant values). + ReadForMatch(Place<'tcx>), + /// Write the discriminant for a variant to the enum Place. SetDiscriminant { place: Place<'tcx>, variant_index: usize }, @@ -1327,6 +1331,7 @@ impl<'tcx> Debug for Statement<'tcx> { use self::StatementKind::*; match self.kind { Assign(ref place, ref rv) => write!(fmt, "{:?} = {:?}", place, rv), + ReadForMatch(ref place) => write!(fmt, "ReadForMatch({:?})", place), // (reuse lifetime rendering policy from ppaux.) EndRegion(ref ce) => write!(fmt, "EndRegion({})", ty::ReScope(*ce)), Validate(ref op, ref places) => write!(fmt, "Validate({:?}, {:?})", op, places), @@ -2212,6 +2217,7 @@ BraceStructTypeFoldableImpl! { EnumTypeFoldableImpl! { impl<'tcx> TypeFoldable<'tcx> for StatementKind<'tcx> { (StatementKind::Assign)(a, b), + (StatementKind::ReadForMatch)(place), (StatementKind::SetDiscriminant) { place, variant_index }, (StatementKind::StorageLive)(a), (StatementKind::StorageDead)(a), diff --git a/src/librustc/mir/visit.rs b/src/librustc/mir/visit.rs index b647ba553dd..9dd1432167a 100644 --- a/src/librustc/mir/visit.rs +++ b/src/librustc/mir/visit.rs @@ -355,6 +355,11 @@ macro_rules! make_mir_visitor { ref $($mutability)* rvalue) => { self.visit_assign(block, place, rvalue, location); } + StatementKind::ReadForMatch(ref $($mutability)* place) => { + self.visit_place(place, + PlaceContext::Inspect, + location); + } StatementKind::EndRegion(_) => {} StatementKind::Validate(_, ref $($mutability)* places) => { for operand in places { diff --git a/src/librustc/session/config.rs b/src/librustc/session/config.rs index 755b4af1a3a..35538e5d02a 100644 --- a/src/librustc/session/config.rs +++ b/src/librustc/session/config.rs @@ -1290,16 +1290,22 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options, useful for profiling / PGO."), relro_level: Option<RelroLevel> = (None, parse_relro_level, [TRACKED], "choose which RELRO level to use"), + disable_ast_check_for_mutation_in_guard: bool = (false, parse_bool, [UNTRACKED], + "skip AST-based mutation-in-guard check (mir-borrowck provides more precise check)"), nll_subminimal_causes: bool = (false, parse_bool, [UNTRACKED], "when tracking region error causes, accept subminimal results for faster execution."), nll_facts: bool = (false, parse_bool, [UNTRACKED], "dump facts from NLL analysis into side files"), disable_nll_user_type_assert: bool = (false, parse_bool, [UNTRACKED], "disable user provided type assertion in NLL"), + nll_dont_emit_read_for_match: bool = (false, parse_bool, [UNTRACKED], + "in match codegen, do not include ReadForMatch statements (used by mir-borrowck)"), polonius: bool = (false, parse_bool, [UNTRACKED], "enable polonius-based borrow-checker"), codegen_time_graph: bool = (false, parse_bool, [UNTRACKED], "generate a graphical HTML report of time spent in codegen and LLVM"), + trans_time_graph: bool = (false, parse_bool, [UNTRACKED], + "generate a graphical HTML report of time spent in trans and LLVM"), thinlto: Option<bool> = (None, parse_opt_bool, [TRACKED], "enable ThinLTO when possible"), inline_in_all_cgus: Option<bool> = (None, parse_opt_bool, [TRACKED], diff --git a/src/librustc/ty/context.rs b/src/librustc/ty/context.rs index a533e1a5b9b..68f55b49933 100644 --- a/src/librustc/ty/context.rs +++ b/src/librustc/ty/context.rs @@ -1344,12 +1344,31 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { self.on_disk_query_result_cache.serialize(self.global_tcx(), encoder) } + /// If true, we should use a naive AST walk to determine if match + /// guard could perform bad mutations (or mutable-borrows). + pub fn check_for_mutation_in_guard_via_ast_walk(self) -> bool { + !self.sess.opts.debugging_opts.disable_ast_check_for_mutation_in_guard + } + /// If true, we should use the MIR-based borrowck (we may *also* use /// the AST-based borrowck). pub fn use_mir_borrowck(self) -> bool { self.borrowck_mode().use_mir() } + /// If true, make MIR codegen for `match` emit a temp that holds a + /// borrow of the input to the match expression. + pub fn generate_borrow_of_any_match_input(&self) -> bool { + self.emit_read_for_match() + } + + /// If true, make MIR codegen for `match` emit ReadForMatch + /// statements (which simulate the maximal effect of executing the + /// patterns in a match arm). + pub fn emit_read_for_match(&self) -> bool { + self.use_mir_borrowck() && !self.sess.opts.debugging_opts.nll_dont_emit_read_for_match + } + /// If true, pattern variables for use in guards on match arms /// will be bound as references to the data, and occurrences of /// those variables in the guard expression will implicitly diff --git a/src/librustc_codegen_llvm/mir/statement.rs b/src/librustc_codegen_llvm/mir/statement.rs index 578481df157..c0cce297ef6 100644 --- a/src/librustc_codegen_llvm/mir/statement.rs +++ b/src/librustc_codegen_llvm/mir/statement.rs @@ -82,6 +82,7 @@ impl<'a, 'tcx> FunctionCx<'a, 'tcx> { asm::codegen_inline_asm(&bx, asm, outputs, input_vals); bx } + mir::StatementKind::ReadForMatch(_) | mir::StatementKind::EndRegion(_) | mir::StatementKind::Validate(..) | mir::StatementKind::UserAssertTy(..) | diff --git a/src/librustc_mir/borrow_check/borrow_set.rs b/src/librustc_mir/borrow_check/borrow_set.rs index ccfb44a8b58..3d6f49c3772 100644 --- a/src/librustc_mir/borrow_check/borrow_set.rs +++ b/src/librustc_mir/borrow_check/borrow_set.rs @@ -53,6 +53,17 @@ impl<'tcx> Index<BorrowIndex> for BorrowSet<'tcx> { } } +/// Every two-phase borrow has *exactly one* use (or else it is not a +/// proper two-phase borrow under our current definition). However, not +/// all uses are actually ones that activate the reservation.. In +/// particular, a shared borrow of a `&mut` does not activate the +/// reservation. +#[derive(Copy, Clone, PartialEq, Eq, Debug)] +crate enum TwoPhaseUse { + MutActivate, + SharedUse, +} + #[derive(Debug)] crate struct BorrowData<'tcx> { /// Location where the borrow reservation starts. @@ -60,7 +71,7 @@ crate struct BorrowData<'tcx> { crate reserve_location: Location, /// Location where the borrow is activated. None if this is not a /// 2-phase borrow. - crate activation_location: Option<Location>, + crate activation_location: Option<(TwoPhaseUse, Location)>, /// What kind of borrow this is crate kind: mir::BorrowKind, /// The region for which this borrow is live @@ -215,9 +226,8 @@ impl<'a, 'gcx, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'gcx, 'tcx> { Some(&borrow_index) => { let borrow_data = &mut self.idx_vec[borrow_index]; - // Watch out: the use of TMP in the borrow - // itself doesn't count as an - // activation. =) + // Watch out: the use of TMP in the borrow itself + // doesn't count as an activation. =) if borrow_data.reserve_location == location && context == PlaceContext::Store { return; } @@ -225,7 +235,7 @@ impl<'a, 'gcx, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'gcx, 'tcx> { if let Some(other_activation) = borrow_data.activation_location { span_bug!( self.mir.source_info(location).span, - "found two activations for 2-phase borrow temporary {:?}: \ + "found two uses for 2-phase borrow temporary {:?}: \ {:?} and {:?}", temp, location, @@ -235,11 +245,25 @@ impl<'a, 'gcx, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'gcx, 'tcx> { // Otherwise, this is the unique later use // that we expect. - borrow_data.activation_location = Some(location); - self.activation_map - .entry(location) - .or_insert(Vec::new()) - .push(borrow_index); + + let two_phase_use; + + match context { + // The use of TMP in a shared borrow does not + // count as an actual activation. + PlaceContext::Borrow { kind: mir::BorrowKind::Shared, .. } => { + two_phase_use = TwoPhaseUse::SharedUse; + } + _ => { + two_phase_use = TwoPhaseUse::MutActivate; + self.activation_map + .entry(location) + .or_insert(Vec::new()) + .push(borrow_index); + } + } + + borrow_data.activation_location = Some((two_phase_use, location)); } None => {} diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 9bfba219ccd..20eb084e1a1 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -423,6 +423,14 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx flow_state, ); } + StatementKind::ReadForMatch(ref place) => { + self.access_place(ContextKind::ReadForMatch.new(location), + (place, span), + (Deep, Read(ReadKind::Borrow(BorrowKind::Shared))), + LocalMutationIsAllowed::No, + flow_state, + ); + } StatementKind::SetDiscriminant { ref place, variant_index: _, @@ -1689,14 +1697,16 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { ); let mut error_reported = false; match kind { - Reservation(WriteKind::MutableBorrow(BorrowKind::Unique)) - | Write(WriteKind::MutableBorrow(BorrowKind::Unique)) => { - if let Err(_place_err) = self.is_mutable(place, LocalMutationIsAllowed::Yes) { - span_bug!(span, "&unique borrow for {:?} should not fail", place); - } - } - Reservation(WriteKind::MutableBorrow(BorrowKind::Mut { .. })) - | Write(WriteKind::MutableBorrow(BorrowKind::Mut { .. })) => { + Reservation(WriteKind::MutableBorrow(borrow_kind @ BorrowKind::Unique)) + | Reservation(WriteKind::MutableBorrow(borrow_kind @ BorrowKind::Mut { .. })) + | Write(WriteKind::MutableBorrow(borrow_kind @ BorrowKind::Unique)) + | Write(WriteKind::MutableBorrow(borrow_kind @ BorrowKind::Mut { .. })) => + { + let is_local_mutation_allowed = match borrow_kind { + BorrowKind::Unique => LocalMutationIsAllowed::Yes, + BorrowKind::Mut { .. } => is_local_mutation_allowed, + BorrowKind::Shared => unreachable!(), + }; match self.is_mutable(place, is_local_mutation_allowed) { Ok(root_place) => self.add_used_mut(root_place, flow_state), Err(place_err) => { @@ -2090,6 +2100,7 @@ enum ContextKind { CallDest, Assert, Yield, + ReadForMatch, StorageDead, } diff --git a/src/librustc_mir/borrow_check/nll/invalidation.rs b/src/librustc_mir/borrow_check/nll/invalidation.rs index 50aa1550fb7..46026cdc941 100644 --- a/src/librustc_mir/borrow_check/nll/invalidation.rs +++ b/src/librustc_mir/borrow_check/nll/invalidation.rs @@ -93,6 +93,14 @@ impl<'cg, 'cx, 'tcx, 'gcx> Visitor<'tcx> for InvalidationGenerator<'cg, 'cx, 'tc JustWrite ); } + StatementKind::ReadForMatch(ref place) => { + self.access_place( + ContextKind::ReadForMatch.new(location), + place, + (Deep, Read(ReadKind::Borrow(BorrowKind::Shared))), + LocalMutationIsAllowed::No, + ); + } StatementKind::SetDiscriminant { ref place, variant_index: _, diff --git a/src/librustc_mir/borrow_check/nll/type_check/mod.rs b/src/librustc_mir/borrow_check/nll/type_check/mod.rs index 456aa1aa66f..04f5024b769 100644 --- a/src/librustc_mir/borrow_check/nll/type_check/mod.rs +++ b/src/librustc_mir/borrow_check/nll/type_check/mod.rs @@ -836,7 +836,8 @@ impl<'a, 'gcx, 'tcx> TypeChecker<'a, 'gcx, 'tcx> { ); } } - StatementKind::StorageLive(_) + StatementKind::ReadForMatch(_) + | StatementKind::StorageLive(_) | StatementKind::StorageDead(_) | StatementKind::InlineAsm { .. } | StatementKind::EndRegion(_) diff --git a/src/librustc_mir/borrow_check/path_utils.rs b/src/librustc_mir/borrow_check/path_utils.rs index d8d160b73e6..4871d427d07 100644 --- a/src/librustc_mir/borrow_check/path_utils.rs +++ b/src/librustc_mir/borrow_check/path_utils.rs @@ -12,7 +12,7 @@ /// allowed to be split into separate Reservation and /// Activation phases. use borrow_check::ArtificialField; -use borrow_check::borrow_set::{BorrowSet, BorrowData}; +use borrow_check::borrow_set::{BorrowSet, BorrowData, TwoPhaseUse}; use borrow_check::{Context, Overlap}; use borrow_check::{ShallowOrDeep, Deep, Shallow}; use dataflow::indexes::BorrowIndex; @@ -431,10 +431,13 @@ pub(super) fn is_active<'tcx>( ) -> bool { debug!("is_active(borrow_data={:?}, location={:?})", borrow_data, location); - // If this is not a 2-phase borrow, it is always active. let activation_location = match borrow_data.activation_location { - Some(v) => v, + // If this is not a 2-phase borrow, it is always active. None => return true, + // And if the unique 2-phase use is not an activation, then it is *never* active. + Some((TwoPhaseUse::SharedUse, _)) => return false, + // Otherwise, we derive info from the activation point `v`: + Some((TwoPhaseUse::MutActivate, v)) => v, }; // Otherwise, it is active for every location *except* in between diff --git a/src/librustc_mir/build/expr/as_place.rs b/src/librustc_mir/build/expr/as_place.rs index 365b9babd08..964841e7a9e 100644 --- a/src/librustc_mir/build/expr/as_place.rs +++ b/src/librustc_mir/build/expr/as_place.rs @@ -11,7 +11,7 @@ //! See docs in build/expr/mod.rs use build::{BlockAnd, BlockAndExtension, Builder}; -use build::ForGuard::{OutsideGuard, WithinGuard}; +use build::ForGuard::{OutsideGuard, RefWithinGuard, ValWithinGuard}; use build::expr::category::Category; use hair::*; use rustc::mir::*; @@ -88,10 +88,11 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { } ExprKind::VarRef { id } => { let place = if this.is_bound_var_in_guard(id) { - let index = this.var_local_id(id, WithinGuard); if this.hir.tcx().all_pat_vars_are_implicit_refs_within_guards() { + let index = this.var_local_id(id, RefWithinGuard); Place::Local(index).deref() } else { + let index = this.var_local_id(id, ValWithinGuard); Place::Local(index) } } else { diff --git a/src/librustc_mir/build/matches/mod.rs b/src/librustc_mir/build/matches/mod.rs index f3953d0877c..a3c7bcfbd0b 100644 --- a/src/librustc_mir/build/matches/mod.rs +++ b/src/librustc_mir/build/matches/mod.rs @@ -15,7 +15,7 @@ use build::{BlockAnd, BlockAndExtension, Builder}; use build::{GuardFrame, GuardFrameLocal, LocalsForNode}; -use build::ForGuard::{self, OutsideGuard, WithinGuard}; +use build::ForGuard::{self, OutsideGuard, RefWithinGuard, ValWithinGuard}; use rustc_data_structures::fx::FxHashMap; use rustc_data_structures::bitvec::BitVector; use rustc::ty::{self, Ty}; @@ -43,6 +43,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { discriminant: ExprRef<'tcx>, arms: Vec<Arm<'tcx>>) -> BlockAnd<()> { + let tcx = self.hir.tcx(); let discriminant_place = unpack!(block = self.as_place(block, discriminant)); // Matching on a `discriminant_place` with an uninhabited type doesn't @@ -55,12 +56,33 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { // HACK(eddyb) Work around the above issue by adding a dummy inspection // of `discriminant_place`, specifically by applying `Rvalue::Discriminant` // (which will work regardless of type) and storing the result in a temp. + // + // NOTE: Under NLL, the above issue should no longer occur because it + // injects a borrow of the matched input, which should have the same effect + // as eddyb's hack. Once NLL is the default, we can remove the hack. + let dummy_source_info = self.source_info(span); let dummy_access = Rvalue::Discriminant(discriminant_place.clone()); - let dummy_ty = dummy_access.ty(&self.local_decls, self.hir.tcx()); + let dummy_ty = dummy_access.ty(&self.local_decls, tcx); let dummy_temp = self.temp(dummy_ty, dummy_source_info.span); self.cfg.push_assign(block, dummy_source_info, &dummy_temp, dummy_access); + let source_info = self.source_info(span); + let borrowed_input_temp = if tcx.generate_borrow_of_any_match_input() { + // The region is unknown at this point; we rely on NLL + // inference to find an appropriate one. Therefore you can + // only use this when NLL is turned on. + assert!(tcx.use_mir_borrowck()); + let borrowed_input = + Rvalue::Ref(tcx.types.re_empty, BorrowKind::Shared, discriminant_place.clone()); + let borrowed_input_ty = borrowed_input.ty(&self.local_decls, tcx); + let borrowed_input_temp = self.temp(borrowed_input_ty, span); + self.cfg.push_assign(block, source_info, &borrowed_input_temp, borrowed_input); + Some(borrowed_input_temp) + } else { + None + }; + let mut arm_blocks = ArmBlocks { blocks: arms.iter() .map(|_| self.cfg.start_new_block()) @@ -99,6 +121,44 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { .zip(pre_binding_blocks.iter().zip(pre_binding_blocks.iter().skip(1))) .map(|((arm_index, pattern, guard), (pre_binding_block, next_candidate_pre_binding_block))| { + + if let (true, Some(borrow_temp)) = (tcx.emit_read_for_match(), + borrowed_input_temp.clone()) { + // inject a fake read of the borrowed input at + // the start of each arm's pattern testing + // code. + // + // This should ensure that you cannot change + // the variant for an enum while you are in + // the midst of matching on it. + + self.cfg.push(*pre_binding_block, Statement { + source_info, + kind: StatementKind::ReadForMatch(borrow_temp.clone()), + }); + } + + // One might ask: why not build up the match pair such that it + // matches via `borrowed_input_temp.deref()` instead of + // using the `discriminant_place` directly, as it is doing here? + // + // The basic answer is that if you do that, then you end up with + // accceses to a shared borrow of the input and that conflicts with + // any arms that look like e.g. + // + // match Some(&4) { + // ref mut foo => { + // ... /* mutate `foo` in arm body */ ... + // } + // } + // + // (Perhaps we could further revise the MIR + // construction here so that it only does a + // shared borrow at the outset and delays doing + // the mutable borrow until after the pattern is + // matched *and* the guard (if any) for the arm + // has been run.) + Candidate { span: pattern.span, match_pairs: vec![MatchPair::new(discriminant_place.clone(), pattern)], @@ -229,7 +289,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { } // now apply the bindings, which will also declare the variables - self.bind_matched_candidate_for_arm_body(block, &candidate.bindings, false); + self.bind_matched_candidate_for_arm_body(block, &candidate.bindings); block.unit() } @@ -870,22 +930,6 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { // (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). - // - // * Therefore, when the binding is by-reference, we - // *eagerly* introduce the binding for the arm body - // (`tmp2`) and then borrow it (`tmp1`). - // - // * This is documented with "NOTE tricky business" below. - // - // FIXME The distinction in how `tmp2` is initialized is - // currently encoded in a pretty awkward fashion; namely, by - // passing a boolean to bind_matched_candidate_for_arm_body - // indicating whether all of the by-ref bindings were already - // initialized. - // - // * Also: pnkfelix thinks "laziness" is natural; but since - // MIR-borrowck did not complain with earlier (universally - // eager) MIR codegen, laziness might not be *necessary*. let autoref = self.hir.tcx().all_pat_vars_are_implicit_refs_within_guards(); if let Some(guard) = candidate.guard { @@ -899,7 +943,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { debug!("Entering guard building context: {:?}", guard_frame); self.guard_context.push(guard_frame); } else { - self.bind_matched_candidate_for_arm_body(block, &candidate.bindings, false); + self.bind_matched_candidate_for_arm_body(block, &candidate.bindings); } // the block to branch to if the guard fails; if there is no @@ -913,14 +957,47 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { } let false_edge_block = self.cfg.start_new_block(); + + // We want to ensure that the matched candidates are bound + // after we have confirmed this candidate *and* any + // associated guard; Binding them on `block` is too soon, + // because that would be before we've checked the result + // from the guard. + // + // But binding them on `arm_block` is *too late*, because + // then all of the candidates for a single arm would be + // bound in the same place, that would cause a case like: + // + // ```rust + // match (30, 2) { + // (mut x, 1) | (2, mut x) if { true } => { ... } + // ... // ^^^^^^^ (this is `arm_block`) + // } + // ``` + // + // would yield a `arm_block` something like: + // + // ``` + // StorageLive(_4); // _4 is `x` + // _4 = &mut (_1.0: i32); // this is handling `(mut x, 1)` case + // _4 = &mut (_1.1: i32); // this is handling `(2, mut x)` case + // ``` + // + // and that is clearly not correct. + let post_guard_block = self.cfg.start_new_block(); self.cfg.terminate(block, source_info, - TerminatorKind::if_(self.hir.tcx(), cond, arm_block, - false_edge_block)); + TerminatorKind::if_(self.hir.tcx(), cond, post_guard_block, + false_edge_block)); - let otherwise = self.cfg.start_new_block(); if autoref { - self.bind_matched_candidate_for_arm_body(block, &candidate.bindings, true); + self.bind_matched_candidate_for_arm_body(post_guard_block, &candidate.bindings); } + + self.cfg.terminate(post_guard_block, source_info, + TerminatorKind::Goto { target: arm_block }); + + let otherwise = self.cfg.start_new_block(); + self.cfg.terminate(false_edge_block, source_info, TerminatorKind::FalseEdges { real_target: otherwise, @@ -929,13 +1006,18 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { }); Some(otherwise) } else { - self.bind_matched_candidate_for_arm_body(block, &candidate.bindings, false); + // (Here, it is not too early to bind the matched + // candidate on `block`, because there is no guard result + // that we have to inspect before we bind them.) + self.bind_matched_candidate_for_arm_body(block, &candidate.bindings); self.cfg.terminate(block, candidate_source_info, TerminatorKind::Goto { target: arm_block }); None } } + // Only called when all_pat_vars_are_implicit_refs_within_guards, + // and thus all code/comments assume we are in that context. fn bind_matched_candidate_for_guard(&mut self, block: BasicBlock, bindings: &[Binding<'tcx>]) { @@ -948,61 +1030,54 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { let re_empty = self.hir.tcx().types.re_empty; for binding in bindings { let source_info = self.source_info(binding.span); - let local_for_guard = self.storage_live_binding( - block, binding.var_id, binding.span, WithinGuard); + + // For each pattern ident P of type T, `ref_for_guard` is + // a reference R: &T pointing to the location matched by + // the pattern, and every occurrence of P within a guard + // denotes *R. + let ref_for_guard = self.storage_live_binding( + block, binding.var_id, binding.span, RefWithinGuard); // Question: Why schedule drops if bindings are all // shared-&'s? Answer: Because schedule_drop_for_binding // also emits StorageDead's for those locals. - self.schedule_drop_for_binding(binding.var_id, binding.span, WithinGuard); + self.schedule_drop_for_binding(binding.var_id, binding.span, RefWithinGuard); match binding.binding_mode { BindingMode::ByValue => { let rvalue = Rvalue::Ref(re_empty, BorrowKind::Shared, binding.source.clone()); - self.cfg.push_assign(block, source_info, &local_for_guard, rvalue); + self.cfg.push_assign(block, source_info, &ref_for_guard, rvalue); } BindingMode::ByRef(region, borrow_kind) => { - // NOTE tricky business: For `ref id` and `ref mut - // id` patterns, we want `id` within the guard to + // 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`, while within the arm body it will - // correspond to a temp of type `&T` or `&mut T`, - // as usual. - // - // But to inject the level of indirection, we need - // something to point to. - // - // So: - // - // 1. First set up the local for the arm body - // (even though we have not yet evaluated the - // guard itself), + // T` (i.e. a "borrow of a borrow") that is + // implicitly dereferenced. // - // 2. Then setup the local for the guard, which is - // just a reference to the local from step 1. + // 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 that since we are setting up the local for - // the arm body a bit eagerly here (and likewise - // scheduling its drop code), we should *not* do - // it redundantly later on. - // - // While we could have kept track of this with a - // flag or collection of such bindings, the - // treatment of all of these cases is uniform, so - // we should be safe just avoiding the code - // without maintaining such state.) - let local_for_arm_body = self.storage_live_binding( - block, binding.var_id, binding.span, OutsideGuard); - self.schedule_drop_for_binding(binding.var_id, binding.span, OutsideGuard); - - // rust-lang/rust#27282: this potentially mutable - // borrow may require a cast in the future to - // avoid conflicting with an implicit borrow of - // the whole match input; or maybe it just - // requires an extension of our two-phase borrows - // system. See discussion on rust-lang/rust#49870. + // 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( + block, binding.var_id, binding.span, ValWithinGuard); + self.schedule_drop_for_binding(binding.var_id, binding.span, ValWithinGuard); + + // 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::Unique => borrow_kind, + BorrowKind::Mut { .. } => BorrowKind::Mut { allow_two_phase_borrow: true }, + }; let rvalue = Rvalue::Ref(region, borrow_kind, binding.source.clone()); - self.cfg.push_assign(block, source_info, &local_for_arm_body, rvalue); - let rvalue = Rvalue::Ref(region, BorrowKind::Shared, local_for_arm_body); - self.cfg.push_assign(block, source_info, &local_for_guard, rvalue); + self.cfg.push_assign(block, source_info, &val_for_guard, rvalue); + let rvalue = Rvalue::Ref(region, BorrowKind::Shared, val_for_guard); + self.cfg.push_assign(block, source_info, &ref_for_guard, rvalue); } } } @@ -1010,19 +1085,11 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { fn bind_matched_candidate_for_arm_body(&mut self, block: BasicBlock, - bindings: &[Binding<'tcx>], - already_initialized_state_for_refs: bool) { - debug!("bind_matched_candidate_for_arm_body(block={:?}, bindings={:?}, \ - already_initialized_state_for_refs={:?})", - block, bindings, already_initialized_state_for_refs); + bindings: &[Binding<'tcx>]) { + debug!("bind_matched_candidate_for_arm_body(block={:?}, bindings={:?}", block, bindings); // Assign each of the bindings. This may trigger moves out of the candidate. for binding in bindings { - if let BindingMode::ByRef(..) = binding.binding_mode { - // See "NOTE tricky business" above - if already_initialized_state_for_refs { continue; } - } - let source_info = self.source_info(binding.span); let local = self.storage_live_binding(block, binding.var_id, binding.span, OutsideGuard); @@ -1059,7 +1126,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { var_id, name, var_ty, source_info, syntactic_scope); let tcx = self.hir.tcx(); - let for_arm_body = self.local_decls.push(LocalDecl::<'tcx> { + let local = LocalDecl::<'tcx> { mutability, ty: var_ty.clone(), name: Some(name), @@ -1067,9 +1134,11 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { syntactic_scope, internal: false, is_user_variable: true, - }); + }; + 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 for_guard = self.local_decls.push(LocalDecl::<'tcx> { + let val_for_guard = self.local_decls.push(local); + let ref_for_guard = self.local_decls.push(LocalDecl::<'tcx> { mutability, ty: tcx.mk_imm_ref(tcx.types.re_empty, var_ty), name: Some(name), @@ -1078,7 +1147,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { internal: false, is_user_variable: true, }); - LocalsForNode::Two { for_guard, for_arm_body } + LocalsForNode::Three { val_for_guard, ref_for_guard, for_arm_body } } else { LocalsForNode::One(for_arm_body) }; diff --git a/src/librustc_mir/build/mod.rs b/src/librustc_mir/build/mod.rs index 2da2d3f697f..4822b9e4dfd 100644 --- a/src/librustc_mir/build/mod.rs +++ b/src/librustc_mir/build/mod.rs @@ -293,7 +293,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { #[derive(Debug)] enum LocalsForNode { One(Local), - Two { for_guard: Local, for_arm_body: Local }, + Three { val_for_guard: Local, ref_for_guard: Local, for_arm_body: Local }, } #[derive(Debug)] @@ -325,12 +325,15 @@ struct GuardFrame { locals: Vec<GuardFrameLocal>, } -/// ForGuard is isomorphic to a boolean flag. It indicates whether we are -/// talking about the temp for a local binding for a use within a guard expression, -/// or a temp for use outside of a guard expressions. +/// 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. #[derive(Copy, Clone, Debug, PartialEq, Eq)] enum ForGuard { - WithinGuard, + ValWithinGuard, + RefWithinGuard, OutsideGuard, } @@ -338,11 +341,13 @@ impl LocalsForNode { fn local_id(&self, for_guard: ForGuard) -> Local { match (self, for_guard) { (&LocalsForNode::One(local_id), ForGuard::OutsideGuard) | - (&LocalsForNode::Two { for_guard: local_id, .. }, ForGuard::WithinGuard) | - (&LocalsForNode::Two { for_arm_body: local_id, .. }, ForGuard::OutsideGuard) => + (&LocalsForNode::Three { val_for_guard: local_id, .. }, ForGuard::ValWithinGuard) | + (&LocalsForNode::Three { ref_for_guard: local_id, .. }, ForGuard::RefWithinGuard) | + (&LocalsForNode::Three { for_arm_body: local_id, .. }, ForGuard::OutsideGuard) => local_id, - (&LocalsForNode::One(_), ForGuard::WithinGuard) => + (&LocalsForNode::One(_), ForGuard::ValWithinGuard) | + (&LocalsForNode::One(_), ForGuard::RefWithinGuard) => bug!("anything with one local should never be within a guard."), } } diff --git a/src/librustc_mir/dataflow/impls/borrows.rs b/src/librustc_mir/dataflow/impls/borrows.rs index 04c62854c5c..78886baf514 100644 --- a/src/librustc_mir/dataflow/impls/borrows.rs +++ b/src/librustc_mir/dataflow/impls/borrows.rs @@ -227,6 +227,7 @@ impl<'a, 'gcx, 'tcx> BitDenotation for Borrows<'a, 'gcx, 'tcx> { } } + mir::StatementKind::ReadForMatch(..) | mir::StatementKind::SetDiscriminant { .. } | mir::StatementKind::StorageLive(..) | mir::StatementKind::Validate(..) | diff --git a/src/librustc_mir/dataflow/move_paths/builder.rs b/src/librustc_mir/dataflow/move_paths/builder.rs index cbf4c822769..2ff22842141 100644 --- a/src/librustc_mir/dataflow/move_paths/builder.rs +++ b/src/librustc_mir/dataflow/move_paths/builder.rs @@ -278,6 +278,9 @@ impl<'b, 'a, 'gcx, 'tcx> Gatherer<'b, 'a, 'gcx, 'tcx> { } self.gather_rvalue(rval); } + StatementKind::ReadForMatch(ref place) => { + self.create_move_path(place); + } StatementKind::InlineAsm { ref outputs, ref inputs, ref asm } => { for (output, kind) in outputs.iter().zip(&asm.outputs) { if !kind.is_indirect { diff --git a/src/librustc_mir/hair/pattern/check_match.rs b/src/librustc_mir/hair/pattern/check_match.rs index 7c44a8d4d5f..0a113970098 100644 --- a/src/librustc_mir/hair/pattern/check_match.rs +++ b/src/librustc_mir/hair/pattern/check_match.rs @@ -181,7 +181,9 @@ impl<'a, 'tcx> MatchVisitor<'a, 'tcx> { // Second, if there is a guard on each arm, make sure it isn't // assigning or borrowing anything mutably. if let Some(ref guard) = arm.guard { - check_for_mutation_in_guard(self, &guard); + if self.tcx.check_for_mutation_in_guard_via_ast_walk() { + check_for_mutation_in_guard(self, &guard); + } } // Third, perform some lints. diff --git a/src/librustc_mir/interpret/step.rs b/src/librustc_mir/interpret/step.rs index 554d87a04e2..ab15278219f 100644 --- a/src/librustc_mir/interpret/step.rs +++ b/src/librustc_mir/interpret/step.rs @@ -79,6 +79,10 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> { self.deallocate_local(old_val)?; } + // No dynamic semantics attached to `ReadForMatch`; MIR + // interpreter is solely intended for borrowck'ed code. + ReadForMatch(..) => {} + // Validity checks. Validate(op, ref places) => { for operand in places { diff --git a/src/librustc_mir/transform/check_unsafety.rs b/src/librustc_mir/transform/check_unsafety.rs index fc3764e4f49..4081f827d4b 100644 --- a/src/librustc_mir/transform/check_unsafety.rs +++ b/src/librustc_mir/transform/check_unsafety.rs @@ -100,6 +100,7 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> { self.source_info = statement.source_info; match statement.kind { StatementKind::Assign(..) | + StatementKind::ReadForMatch(..) | StatementKind::SetDiscriminant { .. } | StatementKind::StorageLive(..) | StatementKind::StorageDead(..) | diff --git a/src/librustc_mir/transform/qualify_consts.rs b/src/librustc_mir/transform/qualify_consts.rs index c249dc312f2..71963012944 100644 --- a/src/librustc_mir/transform/qualify_consts.rs +++ b/src/librustc_mir/transform/qualify_consts.rs @@ -1135,6 +1135,7 @@ This does not pose a problem by itself because they can't be accessed directly." StatementKind::Assign(ref place, ref rvalue) => { this.visit_assign(bb, place, rvalue, location); } + StatementKind::ReadForMatch(..) | StatementKind::SetDiscriminant { .. } | StatementKind::StorageLive(_) | StatementKind::StorageDead(_) | diff --git a/src/librustc_mir/transform/remove_noop_landing_pads.rs b/src/librustc_mir/transform/remove_noop_landing_pads.rs index bcc8fef18f0..680b60b9728 100644 --- a/src/librustc_mir/transform/remove_noop_landing_pads.rs +++ b/src/librustc_mir/transform/remove_noop_landing_pads.rs @@ -47,6 +47,7 @@ impl RemoveNoopLandingPads { { for stmt in &mir[bb].statements { match stmt.kind { + StatementKind::ReadForMatch(_) | StatementKind::StorageLive(_) | StatementKind::StorageDead(_) | StatementKind::EndRegion(_) | diff --git a/src/librustc_mir/transform/rustc_peek.rs b/src/librustc_mir/transform/rustc_peek.rs index 8f67b9e7c3d..b23f0568012 100644 --- a/src/librustc_mir/transform/rustc_peek.rs +++ b/src/librustc_mir/transform/rustc_peek.rs @@ -158,6 +158,7 @@ fn each_block<'a, 'tcx, O>(tcx: TyCtxt<'a, 'tcx, 'tcx>, mir::StatementKind::Assign(ref place, ref rvalue) => { (place, rvalue) } + mir::StatementKind::ReadForMatch(_) | mir::StatementKind::StorageLive(_) | mir::StatementKind::StorageDead(_) | mir::StatementKind::InlineAsm { .. } | diff --git a/src/librustc_passes/mir_stats.rs b/src/librustc_passes/mir_stats.rs index 45c6e89321d..f7c8f8f43f1 100644 --- a/src/librustc_passes/mir_stats.rs +++ b/src/librustc_passes/mir_stats.rs @@ -85,6 +85,7 @@ impl<'a, 'tcx> mir_visit::Visitor<'tcx> for StatCollector<'a, 'tcx> { self.record("Statement", statement); self.record(match statement.kind { StatementKind::Assign(..) => "StatementKind::Assign", + StatementKind::ReadForMatch(..) => "StatementKind::ReadForMatch", StatementKind::EndRegion(..) => "StatementKind::EndRegion", StatementKind::Validate(..) => "StatementKind::Validate", StatementKind::SetDiscriminant { .. } => "StatementKind::SetDiscriminant", diff --git a/src/test/mir-opt/match_false_edges.rs b/src/test/mir-opt/match_false_edges.rs index c2a40399efe..739cbc0a996 100644 --- a/src/test/mir-opt/match_false_edges.rs +++ b/src/test/mir-opt/match_false_edges.rs @@ -54,60 +54,65 @@ fn main() { // ... // _2 = std::option::Option<i32>::Some(const 42i32,); // _3 = discriminant(_2); -// _7 = discriminant(_2); -// switchInt(move _7) -> [0isize: bb6, 1isize: bb4, otherwise: bb8]; +// _14 = promoted[1]; +// _4 = &(*_14); +// _9 = discriminant(_2); +// switchInt(move _9) -> [0isize: bb5, 1isize: bb3, otherwise: bb7]; // } // bb1: { // resume; // } // bb2: { // arm1 -// StorageLive(_9); -// _9 = _4; -// _1 = (const 1i32, move _9); -// StorageDead(_9); +// _1 = (const 3i32, const 3i32); // goto -> bb13; // } // bb3: { // binding3(empty) and arm3 -// _1 = (const 3i32, const 3i32); -// goto -> bb13; +// ReadForMatch(_4); +// falseEdges -> [real: bb8, imaginary: bb4]; //pre_binding1 // } // bb4: { -// falseEdges -> [real: bb9, imaginary: bb5]; //pre_binding1 +// ReadForMatch(_4); +// falseEdges -> [real: bb12, imaginary: bb5]; //pre_binding2 // } // bb5: { -// falseEdges -> [real: bb12, imaginary: bb6]; //pre_binding2 +// ReadForMatch(_4); +// falseEdges -> [real: bb2, imaginary: bb6]; //pre_binding3 // } // bb6: { -// falseEdges -> [real: bb3, imaginary: bb7]; //pre_binding3 +// unreachable; // } // bb7: { // unreachable; // } -// bb8: { -// unreachable; +// bb8: { // binding1 and guard +// StorageLive(_7); +// _13 = promoted[0]; +// _7 = &(((*_13) as Some).0: i32); +// StorageLive(_10); +// _10 = const guard() -> [return: bb9, unwind: bb1]; // } -// bb9: { // binding1 and guard -// StorageLive(_5); -// _11 = promoted[0]; -// _5 = &(((*_11) as Some).0: i32); -// StorageLive(_8); -// _8 = const guard() -> [return: bb10, unwind: bb1]; +// bb9: { +// switchInt(move _10) -> [false: bb10, otherwise: bb11]; // } -// bb10: { // end of guard -// StorageLive(_4); -// _4 = ((_2 as Some).0: i32); -// switchInt(move _8) -> [false: bb11, otherwise: bb2]; +// bb10: { // to pre_binding2 +// falseEdges -> [real: bb4, imaginary: bb4]; // } -// bb11: { // to pre_binding2 -// falseEdges -> [real: bb5, imaginary: bb5]; +// bb11: { // bindingNoLandingPads.before.mir2 and arm2 +// StorageLive(_5); +// _5 = ((_2 as Some).0: i32); +// StorageLive(_11); +// _11 = _5; +// _1 = (const 1i32, move _11); +// StorageDead(_11); +// goto -> bb13; // } -// bb12: { // bindingNoLandingPads.before.mir2 and arm2 -// StorageLive(_6); -// _6 = ((_2 as Some).0: i32); -// StorageLive(_10); -// _10 = _6; -// _1 = (const 2i32, move _10); -// StorageDead(_10); +// bb12: { +// StorageLive(_8); +// _8 = ((_2 as Some).0: i32); +// StorageLive(_12); +// _12 = _8; +// _1 = (const 2i32, move_12); +// StorageDead(_12); // goto -> bb13; // } // bb13: { @@ -121,59 +126,63 @@ fn main() { // ... // _2 = std::option::Option<i32>::Some(const 42i32,); // _3 = discriminant(_2); -// _7 = discriminant(_2); -// switchInt(move _7) -> [0isize: bb5, 1isize: bb4, otherwise: bb8]; +// _4 = &_2; +// _9 = discriminant(_2); +// switchInt(move _9) -> [0isize: bb4, 1isize: bb3, otherwise: bb7]; // } // bb1: { // resume; // } -// bb2: { // arm1 -// StorageLive(_9); -// _9 = _4; -// _1 = (const 1i32, move _9); -// StorageDead(_9); -// goto -> bb13; -// } -// bb3: { // binding3(empty) and arm3 +// bb2: { // arm2 // _1 = (const 3i32, const 3i32); // goto -> bb13; // } +// bb3: { +// ReadForMatch(_4); +// falseEdges -> [real: bb8, imaginary: bb4]; //pre_binding1 +// } // bb4: { -// falseEdges -> [real: bb9, imaginary: bb5]; //pre_binding1 +// ReadForMatch(_4); +// falseEdges -> [real: bb2, imaginary: bb5]; //pre_binding2 // } // bb5: { -// falseEdges -> [real: bb3, imaginary: bb6]; //pre_binding2 +// ReadForMatch(_4); +// falseEdges -> [real: bb12, imaginary: bb6]; //pre_binding3 // } // bb6: { -// falseEdges -> [real: bb12, imaginary: bb7]; //pre_binding3 +// unreachable; // } // bb7: { // unreachable; // } -// bb8: { -// unreachable; +// bb8: { // binding1 and guard +// StorageLive(_7); +// _7 = &((_2 as Some).0: i32); +// StorageLive(_10); +// _10 = const guard() -> [return: bb9, unwind: bb1]; // } -// bb9: { // binding1 and guard -// StorageLive(_5); -// _5 = &((_2 as Some).0: i32); -// StorageLive(_8); -// _8 = const guard() -> [return: bb10, unwind: bb1]; +// bb9: { // end of guard +// switchInt(move _10) -> [false: bb10, otherwise: bb11]; // } -// bb10: { // end of guard -// StorageLive(_4); -// _4 = ((_2 as Some).0: i32); -// switchInt(move _8) -> [false: bb11, otherwise: bb2]; +// bb10: { // to pre_binding3 (can skip 2 since this is `Some`) +// falseEdges -> [real: bb5, imaginary: bb4]; // } -// bb11: { // to pre_binding2 -// falseEdges -> [real: bb6, imaginary: bb5]; +// bb11: { // arm1 +// StorageLive(_5); +// _5 = ((_2 as Some).0: i32); +// StorageLive(_11); +// _11 = _5; +// _1 = (const 1i32, move _11); +// StorageDead(_11); +// goto -> bb13; // } -// bb12: { // binding2 and arm2 -// StorageLive(_6); -// _6 = ((_2 as Some).0: i32); -// StorageLive(_10); -// _10 = _6; -// _1 = (const 2i32, move _10); -// StorageDead(_10); +// bb12: { // binding3 and arm3 +// StorageLive(_8); +// _8 = ((_2 as Some).0: i32); +// StorageLive(_12); +// _12 = _8; +// _1 = (const 2i32, move _12); +// StorageDead(_12); // goto -> bb13; // } // bb13: { @@ -187,76 +196,80 @@ fn main() { // ... // _2 = std::option::Option<i32>::Some(const 1i32,); // _3 = discriminant(_2); -// _10 = discriminant(_2); -// switchInt(move _10) -> [1isize: bb4, otherwise: bb5]; +// _4 = &_2; +// _13 = discriminant(_2); +// switchInt(move _13) -> [1isize: bb2, otherwise: bb3]; // } // bb1: { // resume; // } -// bb2: { // arm1 -// _1 = const 1i32; -// goto -> bb17; +// bb2: { +// ReadForMatch(_4); +// falseEdges -> [real: bb7, imaginary: bb3]; //pre_binding1 // } -// bb3: { // arm3 -// _1 = const 3i32; -// goto -> bb17; +// bb3: { +// ReadForMatch(_4); +// falseEdges -> [real: bb11, imaginary: bb4]; //pre_binding2 // } -// // bb4: { -// falseEdges -> [real: bb9, imaginary: bb5]; //pre_binding1 +// ReadForMatch(_4); +// falseEdges -> [real: bb12, imaginary: bb5]; //pre_binding3 // } // bb5: { -// falseEdges -> [real: bb12, imaginary: bb6]; //pre_binding2 +// ReadForMatch(_4); +// falseEdges -> [real: bb16, imaginary: bb6]; //pre_binding4 // } // bb6: { -// falseEdges -> [real: bb13, imaginary: bb7]; //pre_binding3 -// } -// bb7: { -// falseEdges -> [real: bb16, imaginary: bb8]; //pre_binding4 -// } -// bb8: { // unreachable; // } -// bb9: { // binding1: Some(w) if guard() -// StorageLive(_5); -// _5 = &((_2 as Some).0: i32); -// StorageLive(_11); -// _11 = const guard() -> [return: bb10, unwind: bb1]; +// bb7: { // binding1: Some(w) if guard() +// StorageLive(_7); +// _7 = &((_2 as Some).0: i32); +// StorageLive(_14); +// _14 = const guard() -> [return: bb8, unwind: bb1]; // } -// bb10: { //end of guard -// StorageLive(_4); -// _4 = ((_2 as Some).0: i32); -// switchInt(move _11) -> [false: bb11, otherwise: bb2]; +// bb8: { //end of guard +// switchInt(move _14) -> [false: bb9, otherwise: bb10]; // } -// bb11: { // to pre_binding2 -// falseEdges -> [real: bb5, imaginary: bb5]; +// bb9: { // to pre_binding2 +// falseEdges -> [real: bb3, imaginary: bb3]; // } -// bb12: { // binding2 & arm2 -// StorageLive(_6); -// _6 = _2; -// _1 = const 2i32; +// bb10: { // set up bindings for arm1 +// StorageLive(_5); +// _5 = ((_2 as Some).0: i32); +// _1 = const 1i32; // goto -> bb17; // } -// bb13: { // binding3: Some(y) if guard2(y) +// bb11: { // binding2 & arm2 // StorageLive(_8); -// _8 = &((_2 as Some).0: i32); -// StorageLive(_13); -// StorageLive(_14); -// _14 = (*_8); -// _13 = const guard2(move _14) -> [return: bb14, unwind: bb1]; +// _8 = _2; +// _1 = const 2i32; +// goto -> bb17; // } -// bb14: { // end of guard2 -// StorageDead(_14); -// StorageLive(_7); -// _7 = ((_2 as Some).0: i32); -// switchInt(move _13) -> [false: bb15, otherwise: bb3]; +// bb12: { // binding3: Some(y) if guard2(y) +// StorageLive(_11); +// _11 = &((_2 as Some).0: i32); +// StorageLive(_16); +// StorageLive(_17); +// _17 = (*_11); +// _16 = const guard2(move _17) -> [return: bb13, unwind: bb1]; // } -// bb15: { // to pre_binding4 -// falseEdges -> [real: bb7, imaginary: bb7]; +// bb13: { // end of guard2 +// StorageDead(_17); +// switchInt(move _16) -> [false: bb14, otherwise: bb15]; // } -// bb16: { // binding4 & arm4 +// bb14: { // to pre_binding4 +// falseEdges -> [real: bb5, imaginary: bb5]; +// } +// bb15: { // set up bindings for arm3 // StorageLive(_9); -// _9 = _2; +// _9 = ((_2 as Some).0: i32); +// _1 = const 3i32; +// goto -> bb17; +// } +// bb16: { // binding4 & arm4 +// StorageLive(_12); +// _12 = _2; // _1 = const 4i32; // goto -> bb17; // } diff --git a/src/test/run-pass/issue-24535-allow-mutable-borrow-in-match-guard.rs b/src/test/run-pass/issue-24535-allow-mutable-borrow-in-match-guard.rs new file mode 100644 index 00000000000..ac415e31f2b --- /dev/null +++ b/src/test/run-pass/issue-24535-allow-mutable-borrow-in-match-guard.rs @@ -0,0 +1,68 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or +// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license +// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// This test illustrates that under NLL, we can remove our overly +// conservative approach for disallowing mutations of match inputs. + +// See further discussion on rust-lang/rust#24535 and +// rust-lang/rfcs#1006. + +// compile-flags: -Z disable-ast-check-for-mutation-in-guard + +#![feature(nll)] + +fn main() { + rust_issue_24535(); + rfcs_issue_1006_1(); + rfcs_issue_1006_2(); +} + +fn rust_issue_24535() { + fn compare(a: &u8, b: &mut u8) -> bool { + a == b + } + + let a = 3u8; + + match a { + 0 => panic!("nope"), + 3 if compare(&a, &mut 3) => (), + _ => panic!("nope"), + } +} + +fn rfcs_issue_1006_1() { + let v = vec!["1".to_string(), "2".to_string(), "3".to_string()]; + match Some(&v) { + Some(iv) if iv.iter().any(|x| &x[..]=="2") => true, + _ => panic!("nope"), + }; +} + +fn rfcs_issue_1006_2() { + #[inline(always)] + fn check<'a, I: Iterator<Item=&'a i32>>(mut i: I) -> bool { + i.any(|&x| x == 2) + } + + let slice = [1, 2, 3]; + + match 42 { + _ if slice.iter().any(|&x| x == 2) => { true }, + _ => { panic!("nope"); } + }; + + // (This match is just illustrating how easy it was to circumvent + // the checking performed for the previous `match`.) + match 42 { + _ if check(slice.iter()) => { true }, + _ => { panic!("nope"); } + }; +} diff --git a/src/test/ui/borrowck/issue-41962.rs b/src/test/ui/borrowck/issue-41962.rs index f7c33691ad0..29481dbe522 100644 --- a/src/test/ui/borrowck/issue-41962.rs +++ b/src/test/ui/borrowck/issue-41962.rs @@ -15,11 +15,12 @@ pub fn main(){ loop { if let Some(thing) = maybe { - //~^ ERROR use of partially moved value: `maybe` (Ast) [E0382] + } + //~^^ ERROR use of partially moved value: `maybe` (Ast) [E0382] //~| ERROR use of moved value: `(maybe as std::prelude::v1::Some).0` (Ast) [E0382] //~| ERROR use of moved value: `maybe` (Mir) [E0382] //~| ERROR use of moved value: `maybe` (Mir) [E0382] //~| ERROR use of moved value: `maybe.0` (Mir) [E0382] - } + //~| ERROR borrow of moved value: `maybe` (Mir) [E0382] } } diff --git a/src/test/ui/borrowck/issue-41962.stderr b/src/test/ui/borrowck/issue-41962.stderr index 39525d787b1..e6eb3739d8c 100644 --- a/src/test/ui/borrowck/issue-41962.stderr +++ b/src/test/ui/borrowck/issue-41962.stderr @@ -23,16 +23,23 @@ LL | if let Some(thing) = maybe { | ^ ----- value moved here | _________| | | -LL | | //~^ ERROR use of partially moved value: `maybe` (Ast) [E0382] -LL | | //~| ERROR use of moved value: `(maybe as std::prelude::v1::Some).0` (Ast) [E0382] -LL | | //~| ERROR use of moved value: `maybe` (Mir) [E0382] -LL | | //~| ERROR use of moved value: `maybe` (Mir) [E0382] -LL | | //~| ERROR use of moved value: `maybe.0` (Mir) [E0382] LL | | } | |_________^ value used here after move | = note: move occurs because `maybe` has type `std::option::Option<std::vec::Vec<bool>>`, which does not implement the `Copy` trait +error[E0382]: borrow of moved value: `maybe` (Mir) + --> $DIR/issue-41962.rs:17:9 + | +LL | if let Some(thing) = maybe { + | ^ ----- value moved here + | _________| + | | +LL | | } + | |_________^ value borrowed here after move + | + = note: move occurs because `maybe` has type `std::option::Option<std::vec::Vec<bool>>`, which does not implement the `Copy` trait + error[E0382]: use of moved value: `maybe` (Mir) --> $DIR/issue-41962.rs:17:16 | @@ -52,6 +59,6 @@ LL | if let Some(thing) = maybe { | = note: move occurs because `maybe.0` has type `std::vec::Vec<bool>`, which does not implement the `Copy` trait -error: aborting due to 5 previous errors +error: aborting due to 6 previous errors For more information about this error, try `rustc --explain E0382`. diff --git a/src/test/ui/issue-27282-move-match-input-into-guard.rs b/src/test/ui/issue-27282-move-match-input-into-guard.rs new file mode 100644 index 00000000000..b3be36e41e6 --- /dev/null +++ b/src/test/ui/issue-27282-move-match-input-into-guard.rs @@ -0,0 +1,33 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or +// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license +// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Issue 27282: Example 2: This sidesteps the AST checks disallowing +// mutable borrows in match guards by hiding the mutable borrow in a +// guard behind a move (of the mutably borrowed match input) within a +// closure. +// +// This example is not rejected by AST borrowck (and then reliably +// reaches the panic code when executed, despite the compiler warning +// about that match arm being unreachable. + +#![feature(nll)] + +fn main() { + let b = &mut true; + match b { + &mut false => {}, + _ if { (|| { let bar = b; *bar = false; })(); + //~^ ERROR cannot move out of `b` because it is borrowed [E0505] + false } => { }, + &mut true => { println!("You might think we should get here"); }, + //~^ ERROR use of moved value: `*b` [E0382] + _ => panic!("surely we could never get here, since rustc warns it is unreachable."), + } +} diff --git a/src/test/ui/issue-27282-move-match-input-into-guard.stderr b/src/test/ui/issue-27282-move-match-input-into-guard.stderr new file mode 100644 index 00000000000..f89388f1738 --- /dev/null +++ b/src/test/ui/issue-27282-move-match-input-into-guard.stderr @@ -0,0 +1,32 @@ +error[E0505]: cannot move out of `b` because it is borrowed + --> $DIR/issue-27282-move-match-input-into-guard.rs:26:16 + | +LL | match b { + | _____- + | |_____| + | || +LL | || &mut false => {}, +LL | || _ if { (|| { let bar = b; *bar = false; })(); + | || ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ move out of `b` occurs here +LL | || //~^ ERROR cannot move out of `b` because it is borrowed [E0505] +... || +LL | || _ => panic!("surely we could never get here, since rustc warns it is unreachable."), +LL | || } + | || - + | ||_____| + | |______borrow of `b` occurs here + | borrow later used here + +error[E0382]: use of moved value: `*b` + --> $DIR/issue-27282-move-match-input-into-guard.rs:29:14 + | +LL | _ if { (|| { let bar = b; *bar = false; })(); + | ----------------------------------- value moved here +... +LL | &mut true => { println!("You might think we should get here"); }, + | ^^^^ value used here after move + +error: aborting due to 2 previous errors + +Some errors occurred: E0382, E0505. +For more information about an error, try `rustc --explain E0382`. diff --git a/src/test/ui/issue-27282-move-ref-mut-into-guard.rs b/src/test/ui/issue-27282-move-ref-mut-into-guard.rs new file mode 100644 index 00000000000..5b4c746a1b6 --- /dev/null +++ b/src/test/ui/issue-27282-move-ref-mut-into-guard.rs @@ -0,0 +1,28 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or +// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license +// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Issue 27282: Example 1: This sidesteps the AST checks disallowing +// mutable borrows in match guards by hiding the mutable borrow in a +// guard behind a move (of the ref mut pattern id) within a closure. +// +// This example is not rejected by AST borrowck (and then reliably +// segfaults when executed). + +#![feature(nll)] + +fn main() { + match Some(&4) { + None => {}, + ref mut foo + if { (|| { let bar = foo; bar.take() })(); false } => {}, + //~^ ERROR cannot move out of borrowed content [E0507] + Some(s) => std::process::exit(*s), + } +} diff --git a/src/test/ui/issue-27282-move-ref-mut-into-guard.stderr b/src/test/ui/issue-27282-move-ref-mut-into-guard.stderr new file mode 100644 index 00000000000..f6ffa90069c --- /dev/null +++ b/src/test/ui/issue-27282-move-ref-mut-into-guard.stderr @@ -0,0 +1,9 @@ +error[E0507]: cannot move out of borrowed content + --> $DIR/issue-27282-move-ref-mut-into-guard.rs:24:18 + | +LL | if { (|| { let bar = foo; bar.take() })(); false } => {}, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot move out of borrowed content + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0507`. diff --git a/src/test/ui/issue-27282-mutate-before-diverging-arm-1.rs b/src/test/ui/issue-27282-mutate-before-diverging-arm-1.rs new file mode 100644 index 00000000000..b575f4ebce6 --- /dev/null +++ b/src/test/ui/issue-27282-mutate-before-diverging-arm-1.rs @@ -0,0 +1,43 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or +// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license +// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// This is testing an attempt to corrupt the discriminant of the match +// arm in a guard, followed by an attempt to continue matching on that +// corrupted discriminant in the remaining match arms. +// +// Basically this is testing that our new NLL feature of emitting a +// fake read on each match arm is catching cases like this. +// +// This case is interesting because it includes a guard that +// diverges, and therefore a single final fake-read at the very end +// after the final match arm would not suffice. + +#![feature(nll)] + +struct ForceFnOnce; + +fn main() { + let mut x = &mut Some(&2); + let force_fn_once = ForceFnOnce; + match x { + &mut None => panic!("unreachable"), + &mut Some(&_) if { + // ForceFnOnce needed to exploit #27282 + (|| { *x = None; drop(force_fn_once); })(); + //~^ ERROR closure requires unique access to `x` but it is already borrowed [E0500] + false + } => {} + &mut Some(&a) if { // this binds to garbage if we've corrupted discriminant + println!("{}", a); + panic!() + } => {} + _ => panic!("unreachable"), + } +} diff --git a/src/test/ui/issue-27282-mutate-before-diverging-arm-1.stderr b/src/test/ui/issue-27282-mutate-before-diverging-arm-1.stderr new file mode 100644 index 00000000000..8f7fe9d33fe --- /dev/null +++ b/src/test/ui/issue-27282-mutate-before-diverging-arm-1.stderr @@ -0,0 +1,25 @@ +error[E0500]: closure requires unique access to `x` but it is already borrowed + --> $DIR/issue-27282-mutate-before-diverging-arm-1.rs:33:14 + | +LL | match x { + | _____- + | |_____| + | || +LL | || &mut None => panic!("unreachable"), +LL | || &mut Some(&_) if { +LL | || // ForceFnOnce needed to exploit #27282 +LL | || (|| { *x = None; drop(force_fn_once); })(); + | || ^^ - borrow occurs due to use of `x` in closure + | || | + | || closure construction occurs here +... || +LL | || _ => panic!("unreachable"), +LL | || } + | || - + | ||_____| + | |______borrow occurs here + | borrow later used here + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0500`. diff --git a/src/test/ui/issue-27282-mutate-before-diverging-arm-2.rs b/src/test/ui/issue-27282-mutate-before-diverging-arm-2.rs new file mode 100644 index 00000000000..866fed13685 --- /dev/null +++ b/src/test/ui/issue-27282-mutate-before-diverging-arm-2.rs @@ -0,0 +1,52 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or +// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license +// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// This is testing an attempt to corrupt the discriminant of the match +// arm in a guard, followed by an attempt to continue matching on that +// corrupted discriminant in the remaining match arms. +// +// Basically this is testing that our new NLL feature of emitting a +// fake read on each match arm is catching cases like this. +// +// This case is interesting because it includes a guard that +// diverges, and therefore a single final fake-read at the very end +// after the final match arm would not suffice. +// +// It is also interesting because the access to the corrupted data +// occurs in the pattern-match itself, and not in the guard +// expression. + +#![feature(nll)] + +struct ForceFnOnce; + +fn main() { + let mut x = &mut Some(&2); + let force_fn_once = ForceFnOnce; + match x { + &mut None => panic!("unreachable"), + &mut Some(&_) + if { + // ForceFnOnce needed to exploit #27282 + (|| { *x = None; drop(force_fn_once); })(); + //~^ ERROR closure requires unique access to `x` but it is already borrowed [E0500] + false + } => {} + + // this segfaults if we corrupted the discriminant, because + // the compiler gets to *assume* that it cannot be the `None` + // case, even though that was the effect of the guard. + &mut Some(&2) + if { + panic!() + } => {} + _ => panic!("unreachable"), + } +} diff --git a/src/test/ui/issue-27282-mutate-before-diverging-arm-2.stderr b/src/test/ui/issue-27282-mutate-before-diverging-arm-2.stderr new file mode 100644 index 00000000000..df5e4300cec --- /dev/null +++ b/src/test/ui/issue-27282-mutate-before-diverging-arm-2.stderr @@ -0,0 +1,26 @@ +error[E0500]: closure requires unique access to `x` but it is already borrowed + --> $DIR/issue-27282-mutate-before-diverging-arm-2.rs:38:18 + | +LL | match x { + | _____- + | |_____| + | || +LL | || &mut None => panic!("unreachable"), +LL | || &mut Some(&_) +LL | || if { +LL | || // ForceFnOnce needed to exploit #27282 +LL | || (|| { *x = None; drop(force_fn_once); })(); + | || ^^ - borrow occurs due to use of `x` in closure + | || | + | || closure construction occurs here +... || +LL | || _ => panic!("unreachable"), +LL | || } + | || - + | ||_____| + | |______borrow occurs here + | borrow later used here + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0500`. diff --git a/src/test/ui/issue-27282-reborrow-ref-mut-in-guard.rs b/src/test/ui/issue-27282-reborrow-ref-mut-in-guard.rs new file mode 100644 index 00000000000..5d445c63ef4 --- /dev/null +++ b/src/test/ui/issue-27282-reborrow-ref-mut-in-guard.rs @@ -0,0 +1,32 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or +// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license +// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// Issue 27282: This is a variation on issue-27282-move-ref-mut-into-guard.rs +// +// It reborrows instead of moving the `ref mut` pattern borrow. This +// means that our conservative check for mutation in guards will +// reject it. But I want to make sure that we continue to reject it +// (under NLL) even when that conservaive check goes away. + +// compile-flags: -Z disable-ast-check-for-mutation-in-guard + +#![feature(nll)] + +fn main() { + let mut b = &mut true; + match b { + &mut false => {}, + ref mut r if { (|| { let bar = &mut *r; **bar = false; })(); + //~^ ERROR cannot borrow immutable item `*r` as mutable + false } => { &mut *r; }, + &mut true => { println!("You might think we should get here"); }, + _ => panic!("surely we could never get here, since rustc warns it is unreachable."), + } +} diff --git a/src/test/ui/issue-27282-reborrow-ref-mut-in-guard.stderr b/src/test/ui/issue-27282-reborrow-ref-mut-in-guard.stderr new file mode 100644 index 00000000000..d767fdde9f2 --- /dev/null +++ b/src/test/ui/issue-27282-reborrow-ref-mut-in-guard.stderr @@ -0,0 +1,9 @@ +error[E0596]: cannot borrow immutable item `*r` as mutable + --> $DIR/issue-27282-reborrow-ref-mut-in-guard.rs:26:24 + | +LL | ref mut r if { (|| { let bar = &mut *r; **bar = false; })(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot borrow as mutable + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0596`. |