Auto merge of #53438 - matthewjasper:permissive-match-access, r=pnkfelix

[NLL] Be more permissive when checking access due to Match

Partially addresses #53114. notably, we should now have parity with AST borrowck. Matching on uninitialized values is still forbidden.

* ~~Give fake borrows for match their own `BorrowKind`~~
* ~~Allow borrows with this kind to happen on values that are already mutably borrowed.~~
* ~~Track borrows with this type even behind shared reference dereferences and consider all accesses to be deep when checking for conflicts with this borrow type. See [src/test/ui/issues/issue-27282-mutate-before-diverging-arm-3.rs](cb5c989#diff-a2126cd3263a1f5342e2ecd5e699fbc6) for an example soundness issue this fixes (a case of #27282 that wasn't handled correctly).~~
* Create a new `BorrowKind`: `Shallow` (name can be bike-shed)
* `Shallow` borrows differ from shared borrows in that
  * When we check for access we treat them as a `Shallow(Some(_))` read
  * When we check for conflicts with them, if the borrow place is a strict prefix of the access place then we don't consider that a conflict.
    * For example, a `Shallow` borrow of `x` does not conflict with any access or borrow of `x.0` or `*x`
* Remove the current fake borrow in matches.
* When building matches, we take a `Shallow` borrow of any `Place` that we switch on or bind in a match, and any prefix of those places. (There are some optimizations where we do fewer borrows, but this shouldn't change semantics)
  * `match x { &Some(1) => (),  _ => (), }` would `Shallow` borrow `x`, `*x` and `(*x as Some).0` (the `*x` borrow is unnecessary, but I'm not sure how easy it would be to remove.)
* Replace the fake discriminant read with a `ReadForMatch`.
* Change ReadForMatch to only check for initializedness (to prevent `let x: !; match x {}`), but not conflicting borrows. It is still considered a use for liveness and `unsafe` checking.
* Give special cased error messages for this kind of borrow.

Table from the above issue after this PR

| Thing | AST | MIR | Want | Example |
| --- | --- | --- | --- |---|
| `let _ = <unsafe-field>` | 💚  | 💚  | ❌ |  [playground](https://play.rust-lang.org/?gist=bb7843e42fa5318c1043d04bd72abfe4&version=nightly&mode=debug&edition=2015) |
| `match <unsafe_field> { _ => () }` | ❌  | ❌ | ❌ | [playground](https://play.rust-lang.org/?gist=3e3af05fbf1fae28fab2aaf9412fb2ea&version=nightly&mode=debug&edition=2015) |
| `let _ = <moved>` | 💚  | 💚 | 💚 | [playground](https://play.rust-lang.org/?gist=91a6efde8288558e584aaeee0a50558b&version=nightly&mode=debug&edition=2015) |
| `match <moved> { _ => () }` | ❌ | ❌  | 💚 | [playground](https://play.rust-lang.org/?gist=804f8185040b2fe131f2c4a64b3048ca&version=nightly&mode=debug&edition=2015) |
| `let _ = <borrowed>` | 💚  | 💚 | 💚 | [playground](https://play.rust-lang.org/?gist=0e487c2893b89cb772ec2f2b7c5da876&version=nightly&mode=debug&edition=2015) |
| `match <borrowed> { _ => () }` | 💚  | 💚 | 💚 | [playground](https://play.rust-lang.org/?gist=0e487c2893b89cb772ec2f2b7c5da876&version=nightly&mode=debug&edition=2015) |

r? @nikomatsakis

Author: bors

src/librustc/ich/impls_mir.rs

@@ -46,6 +46,7 @@ for mir::BorrowKind {
 
         match *self {
             mir::BorrowKind::Shared |
+            mir::BorrowKind::Shallow |
             mir::BorrowKind::Unique => {}
             mir::BorrowKind::Mut { allow_two_phase_borrow } => {
                 allow_two_phase_borrow.hash_stable(hcx, hasher);
@@ -272,7 +273,7 @@ for mir::StatementKind<'gcx> {
     }
 }
 
-impl_stable_hash_for!(enum mir::FakeReadCause { ForMatch, ForLet });
+impl_stable_hash_for!(enum mir::FakeReadCause { ForMatchGuard, ForMatchedPlace, ForLet });
 
 impl<'a, 'gcx, T> HashStable<StableHashingContext<'a>>
     for mir::ValidationOperand<'gcx, T>

src/librustc/mir/mod.rs

@@ -451,11 +451,32 @@ impl From<Mutability> for hir::Mutability {
     }
 }
 
-#[derive(Copy, Clone, Debug, PartialEq, Eq, RustcEncodable, RustcDecodable)]
+#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord, RustcEncodable, RustcDecodable)]
 pub enum BorrowKind {
     /// Data must be immutable and is aliasable.
     Shared,
 
+    /// The immediately borrowed place must be immutable, but projections from
+    /// it don't need to be. For example, a shallow borrow of `a.b` doesn't
+    /// conflict with a mutable borrow of `a.b.c`.
+    ///
+    /// This is used when lowering matches: when matching on a place we want to
+    /// ensure that place have the same value from the start of the match until
+    /// an arm is selected. This prevents this code from compiling:
+    ///
+    ///     let mut x = &Some(0);
+    ///     match *x {
+    ///         None => (),
+    ///         Some(_) if { x = &None; false } => (),
+    ///         Some(_) => (),
+    ///     }
+    ///
+    /// This can't be a shared borrow because mutably borrowing (*x as Some).0
+    /// should not prevent `if let None = x { ... }`, for example, becase the
+    /// mutating `(*x as Some).0` can't affect the discriminant of `x`.
+    /// We can also report errors with this kind of borrow differently.
+    Shallow,
+
     /// Data must be immutable but not aliasable.  This kind of borrow
     /// cannot currently be expressed by the user and is used only in
     /// implicit closure bindings. It is needed when the closure is
@@ -504,7 +525,7 @@ pub enum BorrowKind {
 impl BorrowKind {
     pub fn allows_two_phase_borrow(&self) -> bool {
         match *self {
-            BorrowKind::Shared | BorrowKind::Unique => false,
+            BorrowKind::Shared | BorrowKind::Shallow | BorrowKind::Unique => false,
             BorrowKind::Mut {
                 allow_two_phase_borrow,
             } => allow_two_phase_borrow,
@@ -1672,7 +1693,11 @@ pub enum FakeReadCause {
     ///
     /// This should ensure that you cannot change the variant for an enum
     /// while you are in the midst of matching on it.
-    ForMatch,
+    ForMatchGuard,
+
+    /// `let x: !; match x {}` doesn't generate any read of x so we need to
+    /// generate a read of x to check that it is initialized and safe.
+    ForMatchedPlace,
 
     /// Officially, the semantics of
     ///
@@ -1773,7 +1798,7 @@ impl<'tcx> Debug for Statement<'tcx> {
 
 /// A path to a value; something that can be evaluated without
 /// changing or disturbing program state.
-#[derive(Clone, PartialEq, Eq, Hash, RustcEncodable, RustcDecodable)]
+#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash, RustcEncodable, RustcDecodable)]
 pub enum Place<'tcx> {
     /// local variable
     Local(Local),
@@ -1790,7 +1815,7 @@ pub enum Place<'tcx> {
 
 /// The def-id of a static, along with its normalized type (which is
 /// stored to avoid requiring normalization when reading MIR).
-#[derive(Clone, PartialEq, Eq, Hash, RustcEncodable, RustcDecodable)]
+#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash, RustcEncodable, RustcDecodable)]
 pub struct Static<'tcx> {
     pub def_id: DefId,
     pub ty: Ty<'tcx>,
@@ -1805,13 +1830,13 @@ impl_stable_hash_for!(struct Static<'tcx> {
 /// or `*B` or `B[index]`. Note that it is parameterized because it is
 /// shared between `Constant` and `Place`. See the aliases
 /// `PlaceProjection` etc below.
-#[derive(Clone, Debug, PartialEq, Eq, Hash, RustcEncodable, RustcDecodable)]
+#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, RustcEncodable, RustcDecodable)]
 pub struct Projection<'tcx, B, V, T> {
     pub base: B,
     pub elem: ProjectionElem<'tcx, V, T>,
 }
 
-#[derive(Clone, Debug, PartialEq, Eq, Hash, RustcEncodable, RustcDecodable)]
+#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, RustcEncodable, RustcDecodable)]
 pub enum ProjectionElem<'tcx, V, T> {
     Deref,
     Field(Field, T),
@@ -2198,6 +2223,7 @@ impl<'tcx> Debug for Rvalue<'tcx> {
             Ref(region, borrow_kind, ref place) => {
                 let kind_str = match borrow_kind {
                     BorrowKind::Shared => "",
+                    BorrowKind::Shallow => "shallow ",
                     BorrowKind::Mut { .. } | BorrowKind::Unique => "mut ",
                 };
 

src/librustc/mir/tcx.rs

@@ -287,6 +287,10 @@ impl BorrowKind {
             // use `&mut`. It gives all the capabilities of an `&uniq`
             // and hence is a safe "over approximation".
             BorrowKind::Unique => hir::MutMutable,
+
+            // We have no type corresponding to a shallow borrow, so use
+            // `&` as an approximation.
+            BorrowKind::Shallow => hir::MutImmutable,
         }
     }
 }

src/librustc/mir/visit.rs

@@ -963,6 +963,7 @@ impl<'tcx> PlaceContext<'tcx> {
 
             PlaceContext::Inspect |
             PlaceContext::Borrow { kind: BorrowKind::Shared, .. } |
+            PlaceContext::Borrow { kind: BorrowKind::Shallow, .. } |
             PlaceContext::Borrow { kind: BorrowKind::Unique, .. } |
             PlaceContext::Projection(Mutability::Not) |
             PlaceContext::Copy | PlaceContext::Move |
@@ -974,7 +975,9 @@ impl<'tcx> PlaceContext<'tcx> {
     /// Returns true if this place context represents a use that does not change the value.
     pub fn is_nonmutating_use(&self) -> bool {
         match *self {
-            PlaceContext::Inspect | PlaceContext::Borrow { kind: BorrowKind::Shared, .. } |
+            PlaceContext::Inspect |
+            PlaceContext::Borrow { kind: BorrowKind::Shared, .. } |
+            PlaceContext::Borrow { kind: BorrowKind::Shallow, .. } |
             PlaceContext::Borrow { kind: BorrowKind::Unique, .. } |
             PlaceContext::Projection(Mutability::Not) |
             PlaceContext::Copy | PlaceContext::Move => true,

src/librustc_mir/borrow_check/borrow_set.rs

@@ -87,6 +87,7 @@ impl<'tcx> fmt::Display for BorrowData<'tcx> {
     fn fmt(&self, w: &mut fmt::Formatter) -> fmt::Result {
         let kind = match self.kind {
             mir::BorrowKind::Shared => "",
+            mir::BorrowKind::Shallow => "shallow ",
             mir::BorrowKind::Unique => "uniq ",
             mir::BorrowKind::Mut { .. } => "mut ",
         };
@@ -287,7 +288,8 @@ impl<'a, 'gcx, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'gcx, 'tcx> {
                     borrow_data.activation_location = match context {
                         // The use of TMP in a shared borrow does not
                         // count as an actual activation.
-                        PlaceContext::Borrow { kind: mir::BorrowKind::Shared, .. } => {
+                        PlaceContext::Borrow { kind: mir::BorrowKind::Shared, .. }
+                        | PlaceContext::Borrow { kind: mir::BorrowKind::Shallow, .. } => {
                             TwoPhaseActivation::NotActivated
                         }
                         _ => {

src/librustc_mir/borrow_check/error_reporting.rs

@@ -333,6 +333,27 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                     Origin::Mir,
                 ),
 
+            (BorrowKind::Mut { .. }, _, _, BorrowKind::Shallow, _, _)
+            | (BorrowKind::Unique, _, _, BorrowKind::Shallow, _, _) => {
+                let mut err = tcx.cannot_mutate_in_match_guard(
+                    span,
+                    issued_span,
+                    &desc_place,
+                    "mutably borrow",
+                    Origin::Mir,
+                );
+                borrow_spans.var_span_label(
+                    &mut err,
+                    format!(
+                        "borrow occurs due to use of `{}` in closure",
+                        desc_place
+                    ),
+                );
+                err.buffer(&mut self.errors_buffer);
+
+                return;
+            }
+
             (BorrowKind::Unique, _, _, _, _, _) => tcx.cannot_uniquely_borrow_by_one_closure(
                 span,
                 &desc_place,
@@ -368,7 +389,16 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                     Origin::Mir,
                 ),
 
-            (BorrowKind::Shared, _, _, BorrowKind::Shared, _, _) => unreachable!(),
+            (BorrowKind::Shallow, _, _, BorrowKind::Unique, _, _)
+            | (BorrowKind::Shallow, _, _, BorrowKind::Mut { .. }, _, _) => {
+                // Shallow borrows are uses from the user's point of view.
+                self.report_use_while_mutably_borrowed(context, (place, span), issued_borrow);
+                return
+            }
+            (BorrowKind::Shared, _, _, BorrowKind::Shared, _, _)
+            | (BorrowKind::Shared, _, _, BorrowKind::Shallow, _, _)
+            | (BorrowKind::Shallow, _, _, BorrowKind::Shared, _, _)
+            | (BorrowKind::Shallow, _, _, BorrowKind::Shallow, _, _) => unreachable!(),
         };
 
         if issued_spans == borrow_spans {
@@ -780,12 +810,22 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
         let loan_span = loan_spans.args_or_use();
 
         let tcx = self.infcx.tcx;
-        let mut err = tcx.cannot_assign_to_borrowed(
+        let mut err = if loan.kind == BorrowKind::Shallow {
+            tcx.cannot_mutate_in_match_guard(
                 span,
                 loan_span,
                 &self.describe_place(place).unwrap_or("_".to_owned()),
+                "assign",
                 Origin::Mir,
-        );
+            )
+        } else {
+            tcx.cannot_assign_to_borrowed(
+                span,
+                loan_span,
+                &self.describe_place(place).unwrap_or("_".to_owned()),
+                Origin::Mir,
+            )
+        };
 
         loan_spans.var_span_label(&mut err, "borrow occurs due to use in closure");
 

src/librustc_mir/borrow_check/mod.rs

@@ -499,11 +499,20 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx
                 );
             }
             StatementKind::FakeRead(_, ref place) => {
-                self.access_place(
+                // Read for match doesn't access any memory and is used to
+                // assert that a place is safe and live. So we don't have to
+                // do any checks here.
+                //
+                // FIXME: Remove check that the place is initialized. This is
+                // needed for now because matches don't have never patterns yet.
+                // So this is the only place we prevent
+                //      let x: !;
+                //      match x {};
+                // from compiling.
+                self.check_if_path_or_subpath_is_moved(
                     ContextKind::FakeRead.new(location),
+                    InitializationRequiringAction::Use,
                     (place, span),
-                    (Deep, Read(ReadKind::Borrow(BorrowKind::Shared))),
-                    LocalMutationIsAllowed::No,
                     flow_state,
                 );
             }
@@ -755,6 +764,7 @@ use self::AccessDepth::{Deep, Shallow};
 enum ArtificialField {
     Discriminant,
     ArrayLength,
+    ShallowBorrow,
 }
 
 #[derive(Copy, Clone, PartialEq, Eq, Debug)]
@@ -835,6 +845,7 @@ enum LocalMutationIsAllowed {
 enum InitializationRequiringAction {
     Update,
     Borrow,
+    MatchOn,
     Use,
     Assignment,
 }
@@ -849,6 +860,7 @@ impl InitializationRequiringAction {
         match self {
             InitializationRequiringAction::Update => "update",
             InitializationRequiringAction::Borrow => "borrow",
+            InitializationRequiringAction::MatchOn => "use", // no good noun
             InitializationRequiringAction::Use => "use",
             InitializationRequiringAction::Assignment => "assign",
         }
@@ -858,6 +870,7 @@ impl InitializationRequiringAction {
         match self {
             InitializationRequiringAction::Update => "updated",
             InitializationRequiringAction::Borrow => "borrowed",
+            InitializationRequiringAction::MatchOn => "matched on",
             InitializationRequiringAction::Use => "used",
             InitializationRequiringAction::Assignment => "assigned",
         }
@@ -972,7 +985,13 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                     Control::Continue
                 }
 
-                (Read(_), BorrowKind::Shared) | (Reservation(..), BorrowKind::Shared) => {
+                (Read(_), BorrowKind::Shared) | (Reservation(..), BorrowKind::Shared)
+                | (Read(_), BorrowKind::Shallow) | (Reservation(..), BorrowKind::Shallow) => {
+                    Control::Continue
+                }
+
+                (Write(WriteKind::Move), BorrowKind::Shallow) => {
+                    // Handled by initialization checks.
                     Control::Continue
                 }
 
@@ -1108,6 +1127,9 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
         match *rvalue {
             Rvalue::Ref(_ /*rgn*/, bk, ref place) => {
                 let access_kind = match bk {
+                    BorrowKind::Shallow => {
+                        (Shallow(Some(ArtificialField::ShallowBorrow)), Read(ReadKind::Borrow(bk)))
+                    },
                     BorrowKind::Shared => (Deep, Read(ReadKind::Borrow(bk))),
                     BorrowKind::Unique | BorrowKind::Mut { .. } => {
                         let wk = WriteKind::MutableBorrow(bk);
@@ -1127,9 +1149,15 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                     flow_state,
                 );
 
+                let action = if bk == BorrowKind::Shallow {
+                    InitializationRequiringAction::MatchOn
+                } else {
+                    InitializationRequiringAction::Borrow
+                };
+
                 self.check_if_path_or_subpath_is_moved(
                     context,
-                    InitializationRequiringAction::Borrow,
+                    action,
                     (place, span),
                     flow_state,
                 );
@@ -1315,11 +1343,16 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
             return;
         }
 
-        // FIXME: replace this with a proper borrow_conflicts_with_place when
-        // that is merged.
         let sd = if might_be_alive { Deep } else { Shallow(None) };
 
-        if places_conflict::places_conflict(self.infcx.tcx, self.mir, place, root_place, sd) {
+        if places_conflict::borrow_conflicts_with_place(
+            self.infcx.tcx,
+            self.mir,
+            place,
+            borrow.kind,
+            root_place,
+            sd
+        ) {
             debug!("check_for_invalidation_at_exit({:?}): INVALID", place);
             // FIXME: should be talking about the region lifetime instead
             // of just a span here.
@@ -1369,7 +1402,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
 
             // only mutable borrows should be 2-phase
             assert!(match borrow.kind {
-                BorrowKind::Shared => false,
+                BorrowKind::Shared | BorrowKind::Shallow => false,
                 BorrowKind::Unique | BorrowKind::Mut { .. } => true,
             });
 
@@ -1669,7 +1702,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                 let is_local_mutation_allowed = match borrow_kind {
                     BorrowKind::Unique => LocalMutationIsAllowed::Yes,
                     BorrowKind::Mut { .. } => is_local_mutation_allowed,
-                    BorrowKind::Shared => unreachable!(),
+                    BorrowKind::Shared | BorrowKind::Shallow => unreachable!(),
                 };
                 match self.is_mutable(place, is_local_mutation_allowed) {
                     Ok(root_place) => {
@@ -1699,8 +1732,10 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
             | Write(wk @ WriteKind::Move)
             | Reservation(wk @ WriteKind::StorageDeadOrDrop)
             | Reservation(wk @ WriteKind::MutableBorrow(BorrowKind::Shared))
+            | Reservation(wk @ WriteKind::MutableBorrow(BorrowKind::Shallow))
             | Write(wk @ WriteKind::StorageDeadOrDrop)
-            | Write(wk @ WriteKind::MutableBorrow(BorrowKind::Shared)) => {
+            | Write(wk @ WriteKind::MutableBorrow(BorrowKind::Shared))
+            | Write(wk @ WriteKind::MutableBorrow(BorrowKind::Shallow)) => {
                 if let Err(_place_err) = self.is_mutable(place, is_local_mutation_allowed) {
                     if self.infcx.tcx.migrate_borrowck() {
                         // rust-lang/rust#46908: In pure NLL mode this
@@ -1743,6 +1778,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
             Read(ReadKind::Borrow(BorrowKind::Unique))
             | Read(ReadKind::Borrow(BorrowKind::Mut { .. }))
             | Read(ReadKind::Borrow(BorrowKind::Shared))
+            | Read(ReadKind::Borrow(BorrowKind::Shallow))
             | Read(ReadKind::Copy) => {
                 // Access authorized
                 return false;