Expand two-phase-borrows so that a case like this still compiles:

```rust
fn main() {
    fn reuse<X>(_: &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.)

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 ccfb44a8b5..5d0913e8eb 100644
--- a/src/librustc_mir/borrow_check/borrow_set.rs
+++ b/src/librustc_mir/borrow_check/borrow_set.rs
@@ -53,6 +53,17 @@ impl<'tcx> Index<BorrowIndex> for BorrowSet<'tcx> {
     }
 }
 
+/// Every two-phase borrow has *exactly one* use (or else it is not a
+/// proper two-phase borrow under our current definition. However, not
+/// all uses are actually ones that activate the reservation.. In
+/// particular, a shared borrow of a `&mut` does not activate the
+/// reservation.
+#[derive(Copy, Clone, PartialEq, Eq, Debug)]
+crate enum TwoPhaseUse {
+    MutActivate,
+    SharedUse,
+}
+
 #[derive(Debug)]
 crate struct BorrowData<'tcx> {
     /// Location where the borrow reservation starts.
@@ -60,7 +71,7 @@ crate struct BorrowData<'tcx> {
     crate reserve_location: Location,
     /// Location where the borrow is activated. None if this is not a
     /// 2-phase borrow.
-    crate activation_location: Option<Location>,
+    crate activation_location: Option<(TwoPhaseUse, Location)>,
     /// What kind of borrow this is
     crate kind: mir::BorrowKind,
     /// The region for which this borrow is live
@@ -215,9 +226,8 @@ impl<'a, 'gcx, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'gcx, 'tcx> {
                 Some(&borrow_index) => {
                     let borrow_data = &mut self.idx_vec[borrow_index];
 
-                    // Watch out: the use of TMP in the borrow
-                    // itself doesn't count as an
-                    // activation. =)
+                    // Watch out: the use of TMP in the borrow itself
+                    // doesn't count as an activation. =)
                     if borrow_data.reserve_location == location && context == PlaceContext::Store {
                         return;
                     }
@@ -225,7 +235,7 @@ impl<'a, 'gcx, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'gcx, 'tcx> {
                     if let Some(other_activation) = borrow_data.activation_location {
                         span_bug!(
                             self.mir.source_info(location).span,
-                            "found two activations for 2-phase borrow temporary {:?}: \
+                            "found two uses for 2-phase borrow temporary {:?}: \
                              {:?} and {:?}",
                             temp,
                             location,
@@ -235,11 +245,25 @@ impl<'a, 'gcx, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'gcx, 'tcx> {
 
                     // Otherwise, this is the unique later use
                     // that we expect.
-                    borrow_data.activation_location = Some(location);
-                    self.activation_map
-                        .entry(location)
-                        .or_insert(Vec::new())
-                        .push(borrow_index);
+
+                    let two_phase_use;
+
+                    match context {
+                        // The use of TMP in a shared borrow does not
+                        // count as an actual activation.
+                        PlaceContext::Borrow { kind: mir::BorrowKind::Shared, .. } => {
+                            two_phase_use = TwoPhaseUse::SharedUse;
+                        }
+                        _ => {
+                            two_phase_use = TwoPhaseUse::MutActivate;
+                            self.activation_map
+                                .entry(location)
+                                .or_insert(Vec::new())
+                                .push(borrow_index);
+                        }
+                    }
+
+                    borrow_data.activation_location = Some((two_phase_use, location));
                 }
 
                 None => {}
diff --git a/src/librustc_mir/borrow_check/path_utils.rs b/src/librustc_mir/borrow_check/path_utils.rs
index d8d160b73e..4871d427d0 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 365b9babd0..964841e7a9 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 67c9db1bf9..552066e679 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 2da2d3f697..4822b9e4df 100644
--- a/src/librustc_mir/build/mod.rs
+++ b/src/librustc_mir/build/mod.rs
@@ -293,7 +293,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
 #[derive(Debug)]
 enum LocalsForNode {
     One(Local),
-    Two { for_guard: Local, for_arm_body: Local },
+    Three { val_for_guard: Local, ref_for_guard: Local, for_arm_body: Local },
 }
 
 #[derive(Debug)]
@@ -325,12 +325,15 @@ struct GuardFrame {
     locals: Vec<GuardFrameLocal>,
 }
 
-/// ForGuard is isomorphic to a boolean flag. It indicates whether we are
-/// talking about the temp for a local binding for a use within a guard expression,
-/// or a temp for use outside of a guard expressions.
+/// ForGuard indicates whether we are talking about:
+///   1. the temp for a local binding used solely within guard expressions,
+///   2. the temp that holds reference to (1.), which is actually what the
+///      guard expressions see, or
+///   3. the temp for use outside of guard expressions.
 #[derive(Copy, Clone, Debug, PartialEq, Eq)]
 enum ForGuard {
-    WithinGuard,
+    ValWithinGuard,
+    RefWithinGuard,
     OutsideGuard,
 }
 
@@ -338,11 +341,13 @@ impl LocalsForNode {
     fn local_id(&self, for_guard: ForGuard) -> Local {
         match (self, for_guard) {
             (&LocalsForNode::One(local_id), ForGuard::OutsideGuard) |
-            (&LocalsForNode::Two { for_guard: local_id, .. }, ForGuard::WithinGuard) |
-            (&LocalsForNode::Two { for_arm_body: local_id, .. }, ForGuard::OutsideGuard) =>
+            (&LocalsForNode::Three { val_for_guard: local_id, .. }, ForGuard::ValWithinGuard) |
+            (&LocalsForNode::Three { ref_for_guard: local_id, .. }, ForGuard::RefWithinGuard) |
+            (&LocalsForNode::Three { for_arm_body: local_id, .. }, ForGuard::OutsideGuard) =>
                 local_id,
 
-            (&LocalsForNode::One(_), ForGuard::WithinGuard) =>
+            (&LocalsForNode::One(_), ForGuard::ValWithinGuard) |
+            (&LocalsForNode::One(_), ForGuard::RefWithinGuard) =>
                 bug!("anything with one local should never be within a guard."),
         }
     }