Auto merge of rust-lang#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 rust-lang#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 rust-lang#51348

Author: bors

src/librustc_mir/borrow_check/borrow_set.rs

@@ -333,14 +333,21 @@ impl<'a, 'gcx, 'tcx> GatherBorrows<'a, 'gcx, 'tcx> {
 
         // 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]);
+        }
     }
 }

src/librustc_mir/build/block.rs

@@ -16,6 +16,8 @@ use rustc::mir::*;
 use rustc::hir;
 use syntax_pos::Span;
 
+use std::slice;
+
 impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
     pub fn ast_block(&mut self,
                      destination: &Place<'tcx>,
@@ -126,7 +128,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                             None,
                             remainder_span,
                             lint_level,
-                            &pattern,
+                            slice::from_ref(&pattern),
                             ArmHasGuard(false),
                             Some((None, initializer_span)),
                         );
@@ -138,8 +140,9 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                                 })
                             }));
                     } 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

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 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                 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)

src/librustc_mir/build/matches/mod.rs

@@ -97,7 +97,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
             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 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
             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 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                         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 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
 
             // 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 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                             mut visibility_scope: Option<SourceScope>,
                             scope_span: Span,
                             lint_level: LintLevel,
-                            pattern: &Pattern<'tcx>,
+                            patterns: &[Pattern<'tcx>],
                             has_guard: ArmHasGuard,
                             opt_match_place: Option<(Option<&Place<'tcx>>, Span)>)
                             -> Option<SourceScope> {
         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 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                 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 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
         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 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
     // 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 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                     // 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 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
 
     /// Each binding (`ref mut var`/`ref var`/`mut var`/`var`, where
     /// the bound `var` has type `T` in the arm body) in a pattern
-    /// maps to *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 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
         };
         let for_arm_body = self.local_decls.push(local.clone());
         let locals = if has_guard.0 && tcx.all_pat_vars_are_implicit_refs_within_guards() {
-            let 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 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                 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)
         };

src/librustc_mir/build/matches/test.rs

@@ -631,6 +631,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
             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 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
             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,
         }

src/librustc_mir/build/mod.rs

@@ -310,8 +310,31 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
 
 #[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<Local>, 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 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                     }
                     _ => {
                         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));

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 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// 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);
+}