From 47bb3fd50559c31030c5f89400864f368694c02f Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Mon, 7 May 2018 15:54:41 +0200 Subject: [PATCH 01/14] Debug flag to bypass restriction of mutation in match guards. Now, if you pass `-Z disable-ast-check-for-mutation-in-guard`, then we will just allow you to mutably-borrow and assign in guards of `match` arms. This is wildly unsound with AST-borrowck. It is also unsound with MIR-borrowck without further adjustments, which come in later in the commit series on this Pull Request. See also rust-lang/rust#24535 and rust-lang/rfcs#1006. --- src/librustc/session/config.rs | 2 ++ src/librustc/ty/context.rs | 6 ++++++ src/librustc_mir/hair/pattern/check_match.rs | 4 +++- 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/librustc/session/config.rs b/src/librustc/session/config.rs index 755b4af1a3aed..45af66815fc2d 100644 --- a/src/librustc/session/config.rs +++ b/src/librustc/session/config.rs @@ -1290,6 +1290,8 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options, useful for profiling / PGO."), relro_level: Option = (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], diff --git a/src/librustc/ty/context.rs b/src/librustc/ty/context.rs index a533e1a5b9b89..1ee28a65424d5 100644 --- a/src/librustc/ty/context.rs +++ b/src/librustc/ty/context.rs @@ -1344,6 +1344,12 @@ 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 { diff --git a/src/librustc_mir/hair/pattern/check_match.rs b/src/librustc_mir/hair/pattern/check_match.rs index 7c44a8d4d5f3b..0a1139700984d 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. From 24abe6f363cd47d444e4cff1123da93817b980f8 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 4 May 2018 12:04:33 +0200 Subject: [PATCH 02/14] rust-lang/rust#27282: Add `StatementKind::ReadForMatch` to MIR. (This is just the data structure changes and some boilerplate match code that followed from it; the actual emission of these statements comes in a follow-up commit.) --- src/librustc/ich/impls_mir.rs | 3 +++ src/librustc/mir/mod.rs | 6 ++++++ src/librustc/mir/visit.rs | 5 +++++ src/librustc_codegen_llvm/mir/statement.rs | 1 + src/librustc_mir/borrow_check/mod.rs | 9 +++++++++ src/librustc_mir/borrow_check/nll/invalidation.rs | 8 ++++++++ src/librustc_mir/borrow_check/nll/type_check/mod.rs | 3 ++- src/librustc_mir/dataflow/impls/borrows.rs | 1 + src/librustc_mir/dataflow/move_paths/builder.rs | 3 +++ src/librustc_mir/interpret/step.rs | 5 +++++ src/librustc_mir/transform/check_unsafety.rs | 1 + src/librustc_mir/transform/qualify_consts.rs | 1 + src/librustc_mir/transform/remove_noop_landing_pads.rs | 1 + src/librustc_mir/transform/rustc_peek.rs | 1 + src/librustc_passes/mir_stats.rs | 1 + 15 files changed, 48 insertions(+), 1 deletion(-) diff --git a/src/librustc/ich/impls_mir.rs b/src/librustc/ich/impls_mir.rs index c71b10ce142c5..e77d38de58264 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 d35884ec78a82..a94e5e793b470 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 b647ba553dd6c..9dd1432167a90 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_codegen_llvm/mir/statement.rs b/src/librustc_codegen_llvm/mir/statement.rs index 578481df157e8..c0cce297ef6a9 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/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 9bfba219ccd71..233974435f3f8 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: _, @@ -2090,6 +2098,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 50aa1550fb768..46026cdc94121 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 456aa1aa66f11..04f5024b76946 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/dataflow/impls/borrows.rs b/src/librustc_mir/dataflow/impls/borrows.rs index 04c62854c5cbd..78886baf51476 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 cbf4c822769c6..2ff22842141d9 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/interpret/step.rs b/src/librustc_mir/interpret/step.rs index 554d87a04e2f8..b9edd2c07f381 100644 --- a/src/librustc_mir/interpret/step.rs +++ b/src/librustc_mir/interpret/step.rs @@ -79,6 +79,11 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> { self.deallocate_local(old_val)?; } + // FIXME: is there some dynamic semantics we should attach to + // these? Or am I correct in thinking that the inerpreter + // 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 fc3764e4f49a5..4081f827d4b3d 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 c249dc312f2f7..719630129440a 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 bcc8fef18f013..680b60b972841 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 8f67b9e7c3d9d..b23f056801210 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 45c6e89321d04..f7c8f8f43f178 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", From 173315e353a9c3c3b8ba2301eaac14fada41f84f Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 4 May 2018 12:05:10 +0200 Subject: [PATCH 03/14] rust-lang/rust#27282: emit `ReadForMatch` on each match arm. Also, turn on ReadForMatch emission by default (when using NLL). --- src/librustc/session/config.rs | 4 ++ src/librustc/ty/context.rs | 13 ++++ src/librustc_mir/build/matches/mod.rs | 94 +++++++++++++++++++++++++-- 3 files changed, 107 insertions(+), 4 deletions(-) diff --git a/src/librustc/session/config.rs b/src/librustc/session/config.rs index 45af66815fc2d..35538e5d02a23 100644 --- a/src/librustc/session/config.rs +++ b/src/librustc/session/config.rs @@ -1298,10 +1298,14 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options, "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 = (None, parse_opt_bool, [TRACKED], "enable ThinLTO when possible"), inline_in_all_cgus: Option = (None, parse_opt_bool, [TRACKED], diff --git a/src/librustc/ty/context.rs b/src/librustc/ty/context.rs index 1ee28a65424d5..68f55b4993301 100644 --- a/src/librustc/ty/context.rs +++ b/src/librustc/ty/context.rs @@ -1356,6 +1356,19 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> { 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_mir/build/matches/mod.rs b/src/librustc_mir/build/matches/mod.rs index f3953d0877c35..67c9db1bf9ee2 100644 --- a/src/librustc_mir/build/matches/mod.rs +++ b/src/librustc_mir/build/matches/mod.rs @@ -30,6 +30,16 @@ mod simplify; mod test; mod util; +/// Injects a borrow of `place`. The region is unknown at this point; we rely on NLL +/// inference to find an appropriate one. Therefore you can only call this when NLL +/// is turned on. +fn inject_borrow<'a, 'gcx, 'tcx>(tcx: ty::TyCtxt<'a, 'gcx, 'tcx>, + place: Place<'tcx>) + -> Rvalue<'tcx> { + assert!(tcx.use_mir_borrowck()); + Rvalue::Ref(tcx.types.re_empty, BorrowKind::Shared, place) +} + /// ArmHasGuard is isomorphic to a boolean flag. It indicates whether /// a match arm has a guard expression attached to it. #[derive(Copy, Clone, Debug)] @@ -43,6 +53,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { discriminant: ExprRef<'tcx>, arms: Vec>) -> 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,16 +66,34 @@ 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. + // + // FIXME: would just the borrow into `borrowed_input_temp` + // also achieve the desired effect here? TBD. 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() { + let borrowed_input = inject_borrow(tcx, 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()) - .collect(), + .map(|_| { + let arm_block = self.cfg.start_new_block(); + arm_block + }) + .collect(), + }; // Get the arm bodies and their scopes, while declaring bindings. @@ -81,7 +110,9 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { // create binding start block for link them by false edges let candidate_count = arms.iter().fold(0, |ac, c| ac + c.patterns.len()); let pre_binding_blocks: Vec<_> = (0..candidate_count + 1) - .map(|_| self.cfg.start_new_block()).collect(); + .map(|_| self.cfg.start_new_block()) + + .collect(); // assemble a list of candidates: there is one candidate per // pattern, which means there may be more than one candidate @@ -99,6 +130,61 @@ 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. + // + // FIXME: I originally had put this at the + // start of each elem of arm_blocks (see + // ArmBlocks construction above). But this was + // broken; for example, when you had a trivial + // match like `match "foo".to_string() { _s => + // {} }`, it would inject a ReadForMatch + // *after* the move of the input into `_s`, + // and that then does not pass the borrow + // check. + // + // * So: I need to fine tune exactly *where* + // the ReadForMatch belongs. Should it come + // at the beginning of each pattern match, + // or the end? And, should there be one + // ReadForMatch per arm, or one per + // candidate (aka pattern)? + + 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)], From 5c30dc85c23d23482148f5eb0485320bc1abc2a3 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Wed, 9 May 2018 17:17:58 +0200 Subject: [PATCH 04/14] rust-lang/rust#41962 has a new error with my new code. Incorporate that into my code. In particular, I am adding an implicit injected borrow on the pattern matches, and when we go around the loop, the compiler is reporting that this injected borrow is conflicting with the move of the original value when the match succeeds. --- src/test/ui/borrowck/issue-41962.rs | 5 +++-- src/test/ui/borrowck/issue-41962.stderr | 19 +++++++++++++------ 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/src/test/ui/borrowck/issue-41962.rs b/src/test/ui/borrowck/issue-41962.rs index f7c33691ad072..29481dbe52211 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 39525d787b1f9..e6eb3739d8c6f 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>`, 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>`, 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`, 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`. From 638acd300fa42a2d0128378a37bffae2c11315ad Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Mon, 7 May 2018 15:58:09 +0200 Subject: [PATCH 05/14] Fallout from allowing some mutation in guards. For some reason, allowing restricted mutation in match arms exposed an obvious case where a unique borrow can indeed fail, namely something like: ```rust match b { ... ref mut r if { (|| { let bar = &mut *r; **bar = false; })(); false } => { &mut *r } // ~~~~~~~ // | // This ends up holding a `&unique` borrow of `r`, but there ends up being an // implicit shared borrow in the guard thanks to rust-lang/rust#49870 ... } ``` --- src/librustc_mir/borrow_check/mod.rs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 233974435f3f8..ec267104518e4 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -1697,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) => { From 3bc5073dbb9e284a64ae8815bdb54d3b1d6c484a Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 11 May 2018 23:32:13 +0200 Subject: [PATCH 06/14] Expand two-phase-borrows so that a case like this still compiles: ```rust fn main() { fn reuse(_: &mut X) {} let mut t = 2f64; match t { ref mut _b if { false } => { reuse(_b); } _ => {} } } ``` Note: The way this is currently written is confusing; when `autoref` is off, then the arm body bindings (introduced by `bind_matched_candidate_for_arm_body`) are *also* used for the guard. (Any attempt to fix this needs to still ensure that the bindings used by the guard are introduced before the guard is evaluated.) (Once we turn NLL on by default, we can presumably simplify all of that.) --- src/librustc_mir/borrow_check/borrow_set.rs | 44 +++-- src/librustc_mir/borrow_check/path_utils.rs | 9 +- src/librustc_mir/build/expr/as_place.rs | 5 +- src/librustc_mir/build/matches/mod.rs | 183 ++++++++++---------- src/librustc_mir/build/mod.rs | 21 ++- 5 files changed, 149 insertions(+), 113 deletions(-) diff --git a/src/librustc_mir/borrow_check/borrow_set.rs b/src/librustc_mir/borrow_check/borrow_set.rs index ccfb44a8b58fb..5d0913e8eb479 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 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, + 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/path_utils.rs b/src/librustc_mir/borrow_check/path_utils.rs index d8d160b73e614..4871d427d0767 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 365b9babd0869..964841e7a9ed4 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 67c9db1bf9ee2..552066e679721 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}; @@ -88,12 +88,8 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { let mut arm_blocks = ArmBlocks { blocks: arms.iter() - .map(|_| { - let arm_block = self.cfg.start_new_block(); - arm_block - }) - .collect(), - + .map(|_| self.cfg.start_new_block()) + .collect(), }; // Get the arm bodies and their scopes, while declaring bindings. @@ -110,9 +106,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { // create binding start block for link them by false edges let candidate_count = arms.iter().fold(0, |ac, c| ac + c.patterns.len()); let pre_binding_blocks: Vec<_> = (0..candidate_count + 1) - .map(|_| self.cfg.start_new_block()) - - .collect(); + .map(|_| self.cfg.start_new_block()).collect(); // assemble a list of candidates: there is one candidate per // pattern, which means there may be more than one candidate @@ -315,7 +309,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() } @@ -956,22 +950,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 { @@ -985,7 +963,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 @@ -999,14 +977,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, @@ -1015,13 +1026,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>]) { @@ -1034,61 +1050,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. + // T` (i.e. a "borrow of a borrow") that is + // implicitly dereferenced. // - // So: + // 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. // - // 1. First set up the local for the arm body - // (even though we have not yet evaluated the - // guard itself), - // - // 2. Then setup the local for the guard, which is - // just a reference to the local from step 1. - // - // 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); } } } @@ -1096,19 +1105,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); @@ -1145,7 +1146,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), @@ -1153,9 +1154,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), @@ -1164,7 +1167,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 2da2d3f697f2e..4822b9e4dfd81 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, } -/// 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."), } } From 5eebd36c93de007f10ede2458431f25fa80bb98f Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Wed, 9 May 2018 13:16:43 +0200 Subject: [PATCH 07/14] Test update: Fallout from ReadForMatch statements + changes to codegen under NLL. --- src/test/mir-opt/match_false_edges.rs | 237 ++++++++++++++------------ 1 file changed, 125 insertions(+), 112 deletions(-) diff --git a/src/test/mir-opt/match_false_edges.rs b/src/test/mir-opt/match_false_edges.rs index c2a40399efe3d..739cbc0a99678 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::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::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::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; // } From 98d5e134f9eb38d8807466a674774987554c860a Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 15 May 2018 23:23:00 +0200 Subject: [PATCH 08/14] Tests illustrating the bug fixes for #27282 and #24535. --- ...535-allow-mutable-borrow-in-match-guard.rs | 68 +++++++++++++++++++ ...issue-27282-move-match-input-into-guard.rs | 33 +++++++++ ...e-27282-move-match-input-into-guard.stderr | 32 +++++++++ .../ui/issue-27282-move-ref-mut-into-guard.rs | 28 ++++++++ ...issue-27282-move-ref-mut-into-guard.stderr | 9 +++ .../issue-27282-reborrow-ref-mut-in-guard.rs | 32 +++++++++ ...sue-27282-reborrow-ref-mut-in-guard.stderr | 9 +++ 7 files changed, 211 insertions(+) create mode 100644 src/test/run-pass/issue-24535-allow-mutable-borrow-in-match-guard.rs create mode 100644 src/test/ui/issue-27282-move-match-input-into-guard.rs create mode 100644 src/test/ui/issue-27282-move-match-input-into-guard.stderr create mode 100644 src/test/ui/issue-27282-move-ref-mut-into-guard.rs create mode 100644 src/test/ui/issue-27282-move-ref-mut-into-guard.stderr create mode 100644 src/test/ui/issue-27282-reborrow-ref-mut-in-guard.rs create mode 100644 src/test/ui/issue-27282-reborrow-ref-mut-in-guard.stderr 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 0000000000000..ac415e31f2b64 --- /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 or the MIT license +// , 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>(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/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 0000000000000..b3be36e41e657 --- /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 or the MIT license +// , 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 0000000000000..f89388f1738ea --- /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 0000000000000..5b4c746a1b611 --- /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 or the MIT license +// , 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 0000000000000..f6ffa90069cc4 --- /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-reborrow-ref-mut-in-guard.rs b/src/test/ui/issue-27282-reborrow-ref-mut-in-guard.rs new file mode 100644 index 0000000000000..5d445c63ef492 --- /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 or the MIT license +// , 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 0000000000000..d767fdde9f217 --- /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`. From 2b5aa905003e2c7b44a9e79d41d86f4ac248b5ce Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 22 May 2018 15:03:40 +0200 Subject: [PATCH 09/14] review feedback: fix indentation of pattern candidates to match code elsewhere in file. --- src/librustc_mir/borrow_check/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index ec267104518e4..20eb084e1a17b 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -1698,9 +1698,9 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { let mut error_reported = false; match kind { 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 { .. })) => + | 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, From a4a5fa293453c4ff9339f11c47eb7262925f7c12 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 22 May 2018 15:05:02 +0200 Subject: [PATCH 10/14] Review feedback: update fixme comment to reflect reality. --- src/librustc_mir/interpret/step.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/librustc_mir/interpret/step.rs b/src/librustc_mir/interpret/step.rs index b9edd2c07f381..ab15278219f40 100644 --- a/src/librustc_mir/interpret/step.rs +++ b/src/librustc_mir/interpret/step.rs @@ -79,9 +79,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> { self.deallocate_local(old_val)?; } - // FIXME: is there some dynamic semantics we should attach to - // these? Or am I correct in thinking that the inerpreter - // is solely intended for borrowck'ed code? + // No dynamic semantics attached to `ReadForMatch`; MIR + // interpreter is solely intended for borrowck'ed code. ReadForMatch(..) => {} // Validity checks. From 7f0c8f6638cba650d5a18aaa91d3fdd4ce3c01fa Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 22 May 2018 16:45:21 +0200 Subject: [PATCH 11/14] Review feedback: Remove a fixme/tbd note and just add a note for the post-NLL future. Driveby: just inline the two-line `fn inject_borrow` into its one call site and remove its definition. --- src/librustc_mir/build/matches/mod.rs | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/src/librustc_mir/build/matches/mod.rs b/src/librustc_mir/build/matches/mod.rs index 552066e679721..7555827ff01ec 100644 --- a/src/librustc_mir/build/matches/mod.rs +++ b/src/librustc_mir/build/matches/mod.rs @@ -30,16 +30,6 @@ mod simplify; mod test; mod util; -/// Injects a borrow of `place`. The region is unknown at this point; we rely on NLL -/// inference to find an appropriate one. Therefore you can only call this when NLL -/// is turned on. -fn inject_borrow<'a, 'gcx, 'tcx>(tcx: ty::TyCtxt<'a, 'gcx, 'tcx>, - place: Place<'tcx>) - -> Rvalue<'tcx> { - assert!(tcx.use_mir_borrowck()); - Rvalue::Ref(tcx.types.re_empty, BorrowKind::Shared, place) -} - /// ArmHasGuard is isomorphic to a boolean flag. It indicates whether /// a match arm has a guard expression attached to it. #[derive(Copy, Clone, Debug)] @@ -67,8 +57,10 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { // of `discriminant_place`, specifically by applying `Rvalue::Discriminant` // (which will work regardless of type) and storing the result in a temp. // - // FIXME: would just the borrow into `borrowed_input_temp` - // also achieve the desired effect here? TBD. + // 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, tcx); @@ -77,7 +69,12 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { let source_info = self.source_info(span); let borrowed_input_temp = if tcx.generate_borrow_of_any_match_input() { - let borrowed_input = inject_borrow(tcx, discriminant_place.clone()); + // 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); From 2f7f7ac14ef6537e266983449fbd63b163b8560e Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 22 May 2018 16:48:43 +0200 Subject: [PATCH 12/14] Review feedback, remove fixme comment. --- src/librustc_mir/build/matches/mod.rs | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/src/librustc_mir/build/matches/mod.rs b/src/librustc_mir/build/matches/mod.rs index 7555827ff01ec..a3c7bcfbd0bef 100644 --- a/src/librustc_mir/build/matches/mod.rs +++ b/src/librustc_mir/build/matches/mod.rs @@ -131,23 +131,6 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { // This should ensure that you cannot change // the variant for an enum while you are in // the midst of matching on it. - // - // FIXME: I originally had put this at the - // start of each elem of arm_blocks (see - // ArmBlocks construction above). But this was - // broken; for example, when you had a trivial - // match like `match "foo".to_string() { _s => - // {} }`, it would inject a ReadForMatch - // *after* the move of the input into `_s`, - // and that then does not pass the borrow - // check. - // - // * So: I need to fine tune exactly *where* - // the ReadForMatch belongs. Should it come - // at the beginning of each pattern match, - // or the end? And, should there be one - // ReadForMatch per arm, or one per - // candidate (aka pattern)? self.cfg.push(*pre_binding_block, Statement { source_info, From 324ced89e89a4e050ab2d0ef9e30e4ec8968f51a Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 22 May 2018 16:50:03 +0200 Subject: [PATCH 13/14] Review feedback: Fix typo. --- src/librustc_mir/borrow_check/borrow_set.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_mir/borrow_check/borrow_set.rs b/src/librustc_mir/borrow_check/borrow_set.rs index 5d0913e8eb479..3d6f49c377226 100644 --- a/src/librustc_mir/borrow_check/borrow_set.rs +++ b/src/librustc_mir/borrow_check/borrow_set.rs @@ -54,7 +54,7 @@ impl<'tcx> Index 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 +/// 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. From 9d5cdc958db3ee832dbdb781358442fddd51d6ee Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Wed, 23 May 2018 11:21:01 +0200 Subject: [PATCH 14/14] Review feedback: Adding test cases suggested by arielb1. --- ...sue-27282-mutate-before-diverging-arm-1.rs | 43 +++++++++++++++ ...27282-mutate-before-diverging-arm-1.stderr | 25 +++++++++ ...sue-27282-mutate-before-diverging-arm-2.rs | 52 +++++++++++++++++++ ...27282-mutate-before-diverging-arm-2.stderr | 26 ++++++++++ 4 files changed, 146 insertions(+) create mode 100644 src/test/ui/issue-27282-mutate-before-diverging-arm-1.rs create mode 100644 src/test/ui/issue-27282-mutate-before-diverging-arm-1.stderr create mode 100644 src/test/ui/issue-27282-mutate-before-diverging-arm-2.rs create mode 100644 src/test/ui/issue-27282-mutate-before-diverging-arm-2.stderr 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 0000000000000..b575f4ebce6c0 --- /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 or the MIT license +// , 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 0000000000000..8f7fe9d33fe41 --- /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 0000000000000..866fed1368504 --- /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 or the MIT license +// , 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 0000000000000..df5e4300ceca2 --- /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`.