Auto merge of #50783 - pnkfelix:issue-27282-match-borrows-its-input-take-three, r=nikomatsakis

every match arm reads the match's borrowed input

This PR changes the `match` codegen under NLL (and just NLL, at least for now) to make the following adjustments:
 * It adds a `-Z disable-ast-check-for-mutation-in-guard` which, as described, turns off the naive (conservative but also not 100% sound) check for mutation in guards of match arms.
 * We now borrow the match input at the outset and emit a special `ReadForMatch` statement (that, according to the *static* semantics, reads that borrowed match input) at the start of each match arm. The intent here is to catch cases where the match guard mutates the match input, either via an independent borrow or via `ref mut` borrows in that arm's pattern.
 * In order to ensure that `ref mut` borrows do not actually conflict with the emitted `ReadForMatch` statements, I expanded the two-phase-borrow system slightly, and also changed the MIR code gen so that under NLL, when there is a guard on a match arm, then each pattern variable ends up having *three* temporaries associated with it:
   1. The first temporary will hold the substructure being matched; this is what we will move the (substructural) value into *if* the guard succeeds.
   2. The second temporary also corresponds to the same value as the first, but we are just constructing this temporarily for use during the scope of the guard; it is unaliased and its sole referrer is the third temporary.
   3. The third temporary is a reference to the second temporary.
   * (This sounds complicated, I know, but its actually *simpler* than what I was doing before and had checked into the repo, which was to sometimes construct the final value and then take a reference to it before evaluating the guard. See also PR #49870.)

Fix #27282

This also provides a path towards resolving #24535 aka rust-lang/rfcs#1006, at least once the `-Z disable-ast-check-for-mutation-in-guard` is just turned on by default (under NLL, that is. It is not sound under AST-borrowck).
 * But I did not want to make `#![feature(nll)]` imply that as part of this PR; that seemed like too drastic a change to me.

Author: bors

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);

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),

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 {

src/librustc/session/config.rs

@@ -1290,16 +1290,22 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options,
         useful for profiling / PGO."),
     relro_level: Option<RelroLevel> = (None, parse_relro_level, [TRACKED],
         "choose which RELRO level to use"),
+    disable_ast_check_for_mutation_in_guard: bool = (false, parse_bool, [UNTRACKED],
+        "skip AST-based mutation-in-guard check (mir-borrowck provides more precise check)"),
     nll_subminimal_causes: bool = (false, parse_bool, [UNTRACKED],
         "when tracking region error causes, accept subminimal results for faster execution."),
     nll_facts: bool = (false, parse_bool, [UNTRACKED],
                        "dump facts from NLL analysis into side files"),
     disable_nll_user_type_assert: bool = (false, parse_bool, [UNTRACKED],
         "disable user provided type assertion in NLL"),
+    nll_dont_emit_read_for_match: bool = (false, parse_bool, [UNTRACKED],
+        "in match codegen, do not include ReadForMatch statements (used by mir-borrowck)"),
     polonius: bool = (false, parse_bool, [UNTRACKED],
         "enable polonius-based borrow-checker"),
     codegen_time_graph: bool = (false, parse_bool, [UNTRACKED],
         "generate a graphical HTML report of time spent in codegen and LLVM"),
+    trans_time_graph: bool = (false, parse_bool, [UNTRACKED],
+        "generate a graphical HTML report of time spent in trans and LLVM"),
     thinlto: Option<bool> = (None, parse_opt_bool, [TRACKED],
         "enable ThinLTO when possible"),
     inline_in_all_cgus: Option<bool> = (None, parse_opt_bool, [TRACKED],

src/librustc/ty/context.rs

@@ -1344,12 +1344,31 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
         self.on_disk_query_result_cache.serialize(self.global_tcx(), encoder)
     }
 
+    /// If true, we should use a naive AST walk to determine if match
+    /// guard could perform bad mutations (or mutable-borrows).
+    pub fn check_for_mutation_in_guard_via_ast_walk(self) -> bool {
+        !self.sess.opts.debugging_opts.disable_ast_check_for_mutation_in_guard
+    }
+
     /// If true, we should use the MIR-based borrowck (we may *also* use
     /// the AST-based borrowck).
     pub fn use_mir_borrowck(self) -> bool {
         self.borrowck_mode().use_mir()
     }
 
+    /// If true, make MIR codegen for `match` emit a temp that holds a
+    /// borrow of the input to the match expression.
+    pub fn generate_borrow_of_any_match_input(&self) -> bool {
+        self.emit_read_for_match()
+    }
+
+    /// If true, make MIR codegen for `match` emit ReadForMatch
+    /// statements (which simulate the maximal effect of executing the
+    /// patterns in a match arm).
+    pub fn emit_read_for_match(&self) -> bool {
+        self.use_mir_borrowck() && !self.sess.opts.debugging_opts.nll_dont_emit_read_for_match
+    }
+
     /// If true, pattern variables for use in guards on match arms
     /// will be bound as references to the data, and occurrences of
     /// those variables in the guard expression will implicitly

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(..) |

src/librustc_mir/borrow_check/borrow_set.rs

@@ -53,14 +53,25 @@ 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.
     /// In many cases, this will be equal to the activation location but not always.
     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,17 +226,16 @@ 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;
                     }
 
                     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 => {}

src/librustc_mir/borrow_check/mod.rs

@@ -423,6 +423,14 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx
                     flow_state,
                 );
             }
+            StatementKind::ReadForMatch(ref place) => {
+                self.access_place(ContextKind::ReadForMatch.new(location),
+                                  (place, span),
+                                  (Deep, Read(ReadKind::Borrow(BorrowKind::Shared))),
+                                  LocalMutationIsAllowed::No,
+                                  flow_state,
+                                  );
+            }
             StatementKind::SetDiscriminant {
                 ref place,
                 variant_index: _,
@@ -1689,14 +1697,16 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
         );
         let mut error_reported = false;
         match kind {
-            Reservation(WriteKind::MutableBorrow(BorrowKind::Unique))
-            | Write(WriteKind::MutableBorrow(BorrowKind::Unique)) => {
-                if let Err(_place_err) = self.is_mutable(place, LocalMutationIsAllowed::Yes) {
-                    span_bug!(span, "&unique borrow for {:?} should not fail", place);
-                }
-            }
-            Reservation(WriteKind::MutableBorrow(BorrowKind::Mut { .. }))
-            | Write(WriteKind::MutableBorrow(BorrowKind::Mut { .. })) => {
+            Reservation(WriteKind::MutableBorrow(borrow_kind @ BorrowKind::Unique))
+            | Reservation(WriteKind::MutableBorrow(borrow_kind @ BorrowKind::Mut { .. }))
+            | Write(WriteKind::MutableBorrow(borrow_kind @ BorrowKind::Unique))
+            | Write(WriteKind::MutableBorrow(borrow_kind @ BorrowKind::Mut { .. })) =>
+            {
+                let is_local_mutation_allowed = match borrow_kind {
+                    BorrowKind::Unique => LocalMutationIsAllowed::Yes,
+                    BorrowKind::Mut { .. } => is_local_mutation_allowed,
+                    BorrowKind::Shared => unreachable!(),
+                };
                 match self.is_mutable(place, is_local_mutation_allowed) {
                     Ok(root_place) => self.add_used_mut(root_place, flow_state),
                     Err(place_err) => {
@@ -2090,6 +2100,7 @@ enum ContextKind {
     CallDest,
     Assert,
     Yield,
+    ReadForMatch,
     StorageDead,
 }
 

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: _,

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(_)

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

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 {