From: bors Date: Fri, 27 Jul 2018 11:02:27 +0000 (+0000) Subject: Auto merge of #52733 - pnkfelix:issue-51348-make-temp-for-each-candidate-in-arm,... X-Git-Url: https://git.lizzy.rs/?a=commitdiff_plain;h=6998b369faf3daf849f538d636bac9eda659fa5a;hp=b18b9edf006c10f4e08794d31425001401e27a09;p=rust.git Auto merge of #52733 - pnkfelix:issue-51348-make-temp-for-each-candidate-in-arm, r=nikomatsakis [NLL] make temp for each candidate in `match` arm In NLL, `ref mut` patterns leverage the two-phase borrow infrastructure to allow the shared borrows within a guard before the "activation" of the mutable borrow when we begin execution of the match arm's body. (There is further discussion of this on PR #50783.) To accommodate the restrictions we impose on two-phase borrows (namely that there is a one-to-one mapping between each activation and the original initialization), this PR is making separate temps for each candidate pattern. So in an arm like this: ```rust PatA(_, ref mut ident) | PatB(ref mut ident) | PatC(_, _, ref mut ident) | PatD(ref mut ident) if guard_stuff(ident) => ... ``` instead of 3 temps (two for the guard and one for the arm body), we now have 4 + 2 temps associated with `ident`: one for each candidate plus the actual temp that the guard uses directly, and then the sixth is the temp used in the arm body. Fix #51348 --- diff --git a/src/librustc_mir/borrow_check/borrow_set.rs b/src/librustc_mir/borrow_check/borrow_set.rs index e9e0c5c3613..347bf61480e 100644 --- a/src/librustc_mir/borrow_check/borrow_set.rs +++ b/src/librustc_mir/borrow_check/borrow_set.rs @@ -333,14 +333,21 @@ fn insert_as_pending_if_two_phase( // Consider the borrow not activated to start. When we find an activation, we'll update // this field. - let borrow_data = &mut self.idx_vec[borrow_index]; - borrow_data.activation_location = TwoPhaseActivation::NotActivated; + { + let borrow_data = &mut self.idx_vec[borrow_index]; + borrow_data.activation_location = TwoPhaseActivation::NotActivated; + } // Insert `temp` into the list of pending activations. From // now on, we'll be on the lookout for a use of it. Note that // we are guaranteed that this use will come after the // assignment. let old_value = self.pending_activations.insert(temp, borrow_index); - assert!(old_value.is_none()); + if let Some(old_index) = old_value { + span_bug!(self.mir.source_info(start_location).span, + "found already pending activation for temp: {:?} \ + at borrow_index: {:?} with associated data {:?}", + temp, old_index, self.idx_vec[old_index]); + } } } diff --git a/src/librustc_mir/build/block.rs b/src/librustc_mir/build/block.rs index bbbe757e96e..c3637a5abeb 100644 --- a/src/librustc_mir/build/block.rs +++ b/src/librustc_mir/build/block.rs @@ -16,6 +16,8 @@ use rustc::hir; use syntax_pos::Span; +use std::slice; + impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { pub fn ast_block(&mut self, destination: &Place<'tcx>, @@ -126,7 +128,7 @@ fn ast_block_stmts(&mut self, None, remainder_span, lint_level, - &pattern, + slice::from_ref(&pattern), ArmHasGuard(false), Some((None, initializer_span)), ); @@ -138,8 +140,9 @@ fn ast_block_stmts(&mut self, }) })); } else { - scope = this.declare_bindings(None, remainder_span, lint_level, &pattern, - ArmHasGuard(false), None); + scope = this.declare_bindings( + None, remainder_span, lint_level, slice::from_ref(&pattern), + ArmHasGuard(false), None); // FIXME(#47184): We currently only insert `UserAssertTy` statements for // patterns that are bindings, this is as we do not want to deconstruct diff --git a/src/librustc_mir/build/expr/as_place.rs b/src/librustc_mir/build/expr/as_place.rs index 964841e7a9e..a38ca7ae5bf 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, RefWithinGuard, ValWithinGuard}; +use build::ForGuard::{OutsideGuard, RefWithinGuard}; use build::expr::category::Category; use hair::*; use rustc::mir::*; @@ -87,14 +87,11 @@ fn expr_as_place(&mut self, block.and(Place::Local(Local::new(1))) } ExprKind::VarRef { id } => { - let place = if this.is_bound_var_in_guard(id) { - 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) - } + let place = if this.is_bound_var_in_guard(id) && + 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, OutsideGuard); Place::Local(index) diff --git a/src/librustc_mir/build/matches/mod.rs b/src/librustc_mir/build/matches/mod.rs index 5de316a6640..7c949669028 100644 --- a/src/librustc_mir/build/matches/mod.rs +++ b/src/librustc_mir/build/matches/mod.rs @@ -97,7 +97,7 @@ pub fn match_expr(&mut self, let body = self.hir.mirror(arm.body.clone()); let scope = self.declare_bindings(None, body.span, LintLevel::Inherited, - &arm.patterns[0], + &arm.patterns[..], ArmHasGuard(arm.guard.is_some()), Some((Some(&discriminant_place), discriminant_span))); (body, scope.unwrap_or(self.source_scope)) @@ -118,11 +118,13 @@ pub fn match_expr(&mut self, arms.iter() .enumerate() .flat_map(|(arm_index, arm)| { - arm.patterns.iter() - .map(move |pat| (arm_index, pat, arm.guard.clone())) + arm.patterns.iter().enumerate() + .map(move |(pat_index, pat)| { + (arm_index, pat_index, pat, arm.guard.clone()) + }) }) .zip(pre_binding_blocks.iter().zip(pre_binding_blocks.iter().skip(1))) - .map(|((arm_index, pattern, guard), + .map(|((arm_index, pat_index, pattern, guard), (pre_binding_block, next_candidate_pre_binding_block))| { if let (true, Some(borrow_temp)) = (tcx.emit_read_for_match(), @@ -168,6 +170,7 @@ pub fn match_expr(&mut self, bindings: vec![], guard, arm_index, + pat_index, pre_binding_block: *pre_binding_block, next_candidate_pre_binding_block: *next_candidate_pre_binding_block, } @@ -277,6 +280,7 @@ pub fn place_into_pattern(&mut self, // since we don't call `match_candidates`, next fields is unused arm_index: 0, + pat_index: 0, pre_binding_block: block, next_candidate_pre_binding_block: block }; @@ -324,14 +328,15 @@ pub fn declare_bindings(&mut self, mut visibility_scope: Option, scope_span: Span, lint_level: LintLevel, - pattern: &Pattern<'tcx>, + patterns: &[Pattern<'tcx>], has_guard: ArmHasGuard, opt_match_place: Option<(Option<&Place<'tcx>>, Span)>) -> Option { assert!(!(visibility_scope.is_some() && lint_level.is_explicit()), "can't have both a visibility and a lint scope at the same time"); let mut scope = self.source_scope; - self.visit_bindings(pattern, &mut |this, mutability, name, mode, var, span, ty| { + let num_patterns = patterns.len(); + self.visit_bindings(&patterns[0], &mut |this, mutability, name, mode, var, span, ty| { if visibility_scope.is_none() { visibility_scope = Some(this.new_source_scope(scope_span, LintLevel::Inherited, @@ -349,8 +354,9 @@ pub fn declare_bindings(&mut self, scope, }; let visibility_scope = visibility_scope.unwrap(); - this.declare_binding(source_info, visibility_scope, mutability, name, mode, var, - ty, has_guard, opt_match_place.map(|(x, y)| (x.cloned(), y))); + this.declare_binding(source_info, visibility_scope, mutability, name, mode, + num_patterns, var, ty, has_guard, + opt_match_place.map(|(x, y)| (x.cloned(), y))); }); visibility_scope } @@ -453,6 +459,9 @@ pub struct Candidate<'pat, 'tcx:'pat> { // ...and the blocks for add false edges between candidates pre_binding_block: BasicBlock, next_candidate_pre_binding_block: BasicBlock, + + // This uniquely identifies this candidate *within* the arm. + pat_index: usize, } #[derive(Clone, Debug)] @@ -972,7 +981,8 @@ fn bind_and_guard_matched_candidate<'pat>(&mut self, let autoref = self.hir.tcx().all_pat_vars_are_implicit_refs_within_guards(); if let Some(guard) = candidate.guard { if autoref { - self.bind_matched_candidate_for_guard(block, &candidate.bindings); + self.bind_matched_candidate_for_guard( + block, candidate.pat_index, &candidate.bindings); let guard_frame = GuardFrame { locals: candidate.bindings.iter() .map(|b| GuardFrameLocal::new(b.var_id, b.binding_mode)) @@ -1058,9 +1068,10 @@ fn bind_and_guard_matched_candidate<'pat>(&mut self, // and thus all code/comments assume we are in that context. fn bind_matched_candidate_for_guard(&mut self, block: BasicBlock, + pat_index: usize, bindings: &[Binding<'tcx>]) { - debug!("bind_matched_candidate_for_guard(block={:?}, bindings={:?})", - block, bindings); + debug!("bind_matched_candidate_for_guard(block={:?}, pat_index={:?}, bindings={:?})", + block, pat_index, bindings); // Assign each of the bindings. Since we are binding for a // guard expression, this will never trigger moves out of the @@ -1099,8 +1110,9 @@ fn bind_matched_candidate_for_guard(&mut self, // 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); + block, binding.var_id, binding.span, ValWithinGuard(pat_index)); + self.schedule_drop_for_binding( + binding.var_id, binding.span, ValWithinGuard(pat_index)); // rust-lang/rust#27282: We reuse the two-phase // borrow infrastructure so that the mutable @@ -1146,16 +1158,26 @@ fn bind_matched_candidate_for_arm_body(&mut self, /// Each binding (`ref mut var`/`ref var`/`mut var`/`var`, where /// the bound `var` has type `T` in the arm body) in a pattern - /// maps to *two* locals. The first local is a binding for + /// maps to `2+N` locals. The first local is a binding for /// occurrences of `var` in the guard, which will all have type - /// `&T`. The second local is a binding for occurrences of `var` - /// in the arm body, which will have type `T`. + /// `&T`. The N locals are bindings for the `T` that is referenced + /// by the first local; they are not used outside of the + /// guard. The last local is a binding for occurrences of `var` in + /// the arm body, which will have type `T`. + /// + /// The reason we have N locals rather than just 1 is to + /// accommodate rust-lang/rust#51348: If the arm has N candidate + /// patterns, then in general they can correspond to distinct + /// parts of the matched data, and we want them to be distinct + /// temps in order to simplify checks performed by our internal + /// leveraging of two-phase borrows). fn declare_binding(&mut self, source_info: SourceInfo, visibility_scope: SourceScope, mutability: Mutability, name: Name, mode: BindingMode, + num_patterns: usize, var_id: NodeId, var_ty: Ty<'tcx>, has_guard: ArmHasGuard, @@ -1189,7 +1211,11 @@ fn declare_binding(&mut self, }; 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 val_for_guard = self.local_decls.push(local); + let mut vals_for_guard = Vec::with_capacity(num_patterns); + for _ in 0..num_patterns { + let val_for_guard_idx = self.local_decls.push(local.clone()); + vals_for_guard.push(val_for_guard_idx); + } let ref_for_guard = self.local_decls.push(LocalDecl::<'tcx> { mutability, ty: tcx.mk_imm_ref(tcx.types.re_empty, var_ty), @@ -1200,7 +1226,7 @@ fn declare_binding(&mut self, internal: false, is_user_variable: Some(ClearCrossCrate::Set(BindingForm::RefForGuard)), }); - LocalsForNode::Three { val_for_guard, ref_for_guard, for_arm_body } + LocalsForNode::ForGuard { vals_for_guard, ref_for_guard, for_arm_body } } else { LocalsForNode::One(for_arm_body) }; diff --git a/src/librustc_mir/build/matches/test.rs b/src/librustc_mir/build/matches/test.rs index 3ff209c872f..afa0d28dd77 100644 --- a/src/librustc_mir/build/matches/test.rs +++ b/src/librustc_mir/build/matches/test.rs @@ -631,6 +631,7 @@ fn candidate_without_match_pair<'pat>(&mut self, bindings: candidate.bindings.clone(), guard: candidate.guard.clone(), arm_index: candidate.arm_index, + pat_index: candidate.pat_index, pre_binding_block: candidate.pre_binding_block, next_candidate_pre_binding_block: candidate.next_candidate_pre_binding_block, } @@ -694,6 +695,7 @@ fn candidate_after_variant_switch<'pat>(&mut self, bindings: candidate.bindings.clone(), guard: candidate.guard.clone(), arm_index: candidate.arm_index, + pat_index: candidate.pat_index, pre_binding_block: candidate.pre_binding_block, next_candidate_pre_binding_block: candidate.next_candidate_pre_binding_block, } diff --git a/src/librustc_mir/build/mod.rs b/src/librustc_mir/build/mod.rs index 24228389fbf..054bd69c361 100644 --- a/src/librustc_mir/build/mod.rs +++ b/src/librustc_mir/build/mod.rs @@ -310,8 +310,31 @@ fn var_local_id(&self, id: ast::NodeId, for_guard: ForGuard) -> Local { #[derive(Debug)] enum LocalsForNode { + /// In the usual case, a node-id for an identifier maps to at most + /// one Local declaration. One(Local), - Three { val_for_guard: Local, ref_for_guard: Local, for_arm_body: Local }, + + /// The exceptional case is identifiers in a match arm's pattern + /// that are referenced in a guard of that match arm. For these, + /// we can have `2+k` Locals, where `k` is the number of candidate + /// patterns (separated by `|`) in the arm. + /// + /// * `for_arm_body` is the Local used in the arm body (which is + /// just like the `One` case above), + /// + /// * `ref_for_guard` is the Local used in the arm's guard (which + /// is a reference to a temp that is an alias of + /// `for_arm_body`). + /// + /// * `vals_for_guard` is the `k` Locals; at most one of them will + /// get initialized by the arm's execution, and after it is + /// initialized, `ref_for_guard` will be assigned a reference to + /// it. + /// + /// There reason we have `k` Locals rather than just 1 is to + /// accommodate some restrictions imposed by two-phase borrows, + /// which apply when we have a `ref mut` pattern. + ForGuard { vals_for_guard: Vec, ref_for_guard: Local, for_arm_body: Local }, } #[derive(Debug)] @@ -350,7 +373,10 @@ struct GuardFrame { /// 3. the temp for use outside of guard expressions. #[derive(Copy, Clone, Debug, PartialEq, Eq)] enum ForGuard { - ValWithinGuard, + /// The `usize` identifies for which candidate pattern we want the + /// local binding. We keep a temp per-candidate to accommodate + /// two-phase borrows (see `LocalsForNode` documentation). + ValWithinGuard(usize), RefWithinGuard, OutsideGuard, } @@ -359,12 +385,15 @@ impl LocalsForNode { fn local_id(&self, for_guard: ForGuard) -> Local { match (self, for_guard) { (&LocalsForNode::One(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) => + (&LocalsForNode::ForGuard { ref_for_guard: local_id, .. }, ForGuard::RefWithinGuard) | + (&LocalsForNode::ForGuard { for_arm_body: local_id, .. }, ForGuard::OutsideGuard) => local_id, - (&LocalsForNode::One(_), ForGuard::ValWithinGuard) | + (&LocalsForNode::ForGuard { ref vals_for_guard, .. }, + ForGuard::ValWithinGuard(pat_idx)) => + vals_for_guard[pat_idx], + + (&LocalsForNode::One(_), ForGuard::ValWithinGuard(_)) | (&LocalsForNode::One(_), ForGuard::RefWithinGuard) => bug!("anything with one local should never be within a guard."), } @@ -740,7 +769,7 @@ fn args_and_body(&mut self, } _ => { scope = self.declare_bindings(scope, ast_body.span, - LintLevel::Inherited, &pattern, + LintLevel::Inherited, &[pattern.clone()], matches::ArmHasGuard(false), Some((Some(&place), span))); unpack!(block = self.place_into_pattern(block, pattern, &place, false)); diff --git a/src/test/ui/borrowck/issue-51348-multi-ref-mut-in-guard.rs b/src/test/ui/borrowck/issue-51348-multi-ref-mut-in-guard.rs new file mode 100644 index 00000000000..c9d39fa360e --- /dev/null +++ b/src/test/ui/borrowck/issue-51348-multi-ref-mut-in-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. + +// We used to ICE if you had a single match arm with multiple +// candidate patterns with `ref mut` identifiers used in the arm's +// guard. +// +// Also, this test expands on the original bug's example by actually +// trying to double check that we are matching against the right part +// of the input data based on which candidate pattern actually fired. + +// run-pass + +#![feature(nll)] + +fn foo(x: &mut Result<(u32, u32), (u32, u32)>) -> u32 { + match *x { + Ok((ref mut v, _)) | Err((_, ref mut v)) if *v > 0 => { *v } + _ => { 0 } + } +} + +fn main() { + assert_eq!(foo(&mut Ok((3, 4))), 3); + assert_eq!(foo(&mut Err((3, 4))), 4); +}