Fine tune dianostics for when a borrow conflicts with a destructor that needs exclusive access.

In particular:

 1. Extend `WriteKind::StorageDeadOrDrop` with state to track whether
    we are running a destructor or just freeing backing storage.  (As
    part of this, when we drop a Box<..<Box<T>..> where `T` does not
    need drop, we now signal that the drop of `T` is a kind of storage
    dead rather than a drop.)

 2. When reporting that a value does not live long enough, check if
    we're doing an "interesting" drop, i.e. we aren't just trivally
    freeing the borrowed state, but rather a user-defined dtor will
    run and potentially require exclusive aces to the borrowed state.

 3. Added a new diagnosic to describe the scenario here.

Author: pnkfelix

src/librustc_mir/borrow_check/error_reporting.rs

@@ -8,7 +8,8 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-use borrow_check::WriteKind;
+use borrow_check::{WriteKind, StorageDeadOrDrop};
+use borrow_check::prefixes::IsPrefixOf;
 use rustc::middle::region::ScopeTree;
 use rustc::mir::VarBindingForm;
 use rustc::mir::{BindingForm, BorrowKind, ClearCrossCrate, Field, Local};
@@ -374,13 +375,27 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
         err.buffer(&mut self.errors_buffer);
     }
 
+    /// Reports StorageDeadOrDrop of `place` conflicts with `borrow`.
+    ///
+    /// This means that some data referenced by `borrow` needs to live
+    /// past the point where the StorageDeadOrDrop of `place` occurs.
+    /// This is usually interpreted as meaning that `place` has too
+    /// short a lifetime. (But sometimes it is more useful to report
+    /// it as a more direct conflict between the execution of a
+    /// `Drop::drop` with an aliasing borrow.)
     pub(super) fn report_borrowed_value_does_not_live_long_enough(
         &mut self,
         context: Context,
         borrow: &BorrowData<'tcx>,
         place_span: (&Place<'tcx>, Span),
         kind: Option<WriteKind>,
     ) {
+        debug!("report_borrowed_value_does_not_live_long_enough(\
+                {:?}, {:?}, {:?}, {:?}\
+                )",
+               context, borrow, place_span, kind
+        );
+
         let drop_span = place_span.1;
         let scope_tree = self.tcx.region_scope_tree(self.mir_def_id);
         let root_place = self
@@ -412,6 +427,19 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
 
         let borrow_reason = self.find_why_borrow_contains_point(context, borrow);
 
+        if let Some(WriteKind::StorageDeadOrDrop(StorageDeadOrDrop::Destructor)) = kind
+        {
+            // If a borrow of path `B` conflicts with drop of `D` (and
+            // we're not in the uninteresting case where `B` is a
+            // prefix of `D`), then report this as a more interesting
+            // destructor conflict.
+            if !borrow.borrowed_place.is_prefix_of(place_span.0) {
+                self.report_borrow_conflicts_with_destructor(
+                    context, borrow, borrow_reason, place_span, kind);
+                return;
+            }
+        }
+
         let mut err = match &self.describe_place(&borrow.borrowed_place) {
             Some(_) if self.is_place_thread_local(root_place) => {
                 self.report_thread_local_value_does_not_live_long_enough(drop_span, borrow_span)
@@ -475,6 +503,69 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
         err
     }
 
+    pub(super) fn report_borrow_conflicts_with_destructor(
+        &mut self,
+        context: Context,
+        borrow: &BorrowData<'tcx>,
+        borrow_reason: BorrowContainsPointReason<'tcx>,
+        place_span: (&Place<'tcx>, Span),
+        kind: Option<WriteKind>,
+    ) {
+        debug!(
+            "report_borrow_conflicts_with_destructor(\
+             {:?}, {:?}, {:?}, {:?} {:?}\
+             )",
+            context, borrow, borrow_reason, place_span, kind,
+        );
+
+        let borrow_spans = self.retrieve_borrow_spans(borrow);
+        let borrow_span = borrow_spans.var_or_use();
+
+        let mut err = self.tcx.cannot_borrow_across_destructor(borrow_span, Origin::Mir);
+
+        let drop_span = place_span.1;
+
+        let (what_was_dropped, dropped_ty) = {
+            let place = place_span.0;
+            let desc = match self.describe_place(place) {
+                Some(name) => format!("`{}`", name.as_str()),
+                None => format!("temporary value"),
+            };
+            let ty = place.ty(self.mir, self.tcx).to_ty(self.tcx);
+            (desc, ty)
+        };
+
+        let label = match dropped_ty.sty {
+            ty::Adt(adt, _) if adt.has_dtor(self.tcx) && !adt.is_box() => {
+                match self.describe_place(&borrow.borrowed_place) {
+                    Some(borrowed) =>
+                        format!("here, drop of {D} needs exclusive access to `{B}`, \
+                                 because the type `{T}` implements the `Drop` trait",
+                                D=what_was_dropped, T=dropped_ty, B=borrowed),
+                    None =>
+                        format!("here is drop of {D}; whose type `{T}` implements the `Drop` trait",
+                                D=what_was_dropped, T=dropped_ty),
+                }
+            }
+            _ => format!("drop of {D} occurs here", D=what_was_dropped),
+        };
+        err.span_label(drop_span, label);
+
+        // Only give this note and suggestion if they could be relevant
+        match borrow_reason {
+            BorrowContainsPointReason::Liveness {..}
+            | BorrowContainsPointReason::DropLiveness {..} => {
+                err.note("consider using a `let` binding to create a longer lived value");
+            }
+            BorrowContainsPointReason::OutlivesFreeRegion {..} => (),
+        }
+
+        self.report_why_borrow_contains_point(
+            &mut err, borrow_reason, kind.map(|k| (k, place_span.0)));
+
+        err.buffer(&mut self.errors_buffer);
+    }
+
     fn report_thread_local_value_does_not_live_long_enough(
         &mut self,
         drop_span: Span,

src/librustc_mir/borrow_check/mod.rs

@@ -551,7 +551,8 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx
                 self.access_place(
                     ContextKind::StorageDead.new(location),
                     (&Place::Local(local), span),
-                    (Shallow(None), Write(WriteKind::StorageDeadOrDrop)),
+                    (Shallow(None), Write(WriteKind::StorageDeadOrDrop(
+                        StorageDeadOrDrop::LocalStorageDead))),
                     LocalMutationIsAllowed::Yes,
                     flow_state,
                 );
@@ -778,12 +779,21 @@ enum ReadKind {
 /// (For informational purposes only)
 #[derive(Copy, Clone, PartialEq, Eq, Debug)]
 enum WriteKind {
-    StorageDeadOrDrop,
+    StorageDeadOrDrop(StorageDeadOrDrop),
     MutableBorrow(BorrowKind),
     Mutate,
     Move,
 }
 
+/// Specify whether which case a StorageDeadOrDrop is in.
+/// (For informational purposes only)
+#[derive(Copy, Clone, PartialEq, Eq, Debug)]
+enum StorageDeadOrDrop {
+    LocalStorageDead,
+    BoxedStorageDead,
+    Destructor,
+}
+
 /// When checking permissions for a place access, this flag is used to indicate that an immutable
 /// local place can be mutated.
 ///
@@ -1012,7 +1022,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                     self.access_place(
                         ContextKind::Drop.new(loc),
                         (drop_place, span),
-                        (Deep, Write(WriteKind::StorageDeadOrDrop)),
+                        (Deep, Write(WriteKind::StorageDeadOrDrop(
+                            StorageDeadOrDrop::Destructor))),
                         LocalMutationIsAllowed::Yes,
                         flow_state,
                     );
@@ -1039,15 +1050,17 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                     //   Why? Because we do not schedule/emit
                     //   `Drop(x)` in the MIR unless `x` needs drop in
                     //   the first place.
-                    //
-                    // FIXME: Its possible this logic actually should
-                    // be attached to the `StorageDead` statement
-                    // rather than the `Drop`. See discussion on PR
-                    // #52782.
                     self.access_place(
                         ContextKind::Drop.new(loc),
                         (drop_place, span),
-                        (Shallow(None), Write(WriteKind::StorageDeadOrDrop)),
+                        (Shallow(None), Write(WriteKind::StorageDeadOrDrop(
+                            // rust-lang/rust#52059: distinguish
+                            // between invaliding the backing storage
+                            // vs running a destructor.
+                            //
+                            // See also: rust-lang/rust#52782,
+                            // specifically #discussion_r206658948
+                            StorageDeadOrDrop::BoxedStorageDead))),
                         LocalMutationIsAllowed::Yes,
                         flow_state,
                     );
@@ -1215,14 +1228,13 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                             error_reported = true;
                             this.report_conflicting_borrow(context, place_span, bk, &borrow)
                         }
-                        WriteKind::StorageDeadOrDrop => {
+                        WriteKind::StorageDeadOrDrop(_) => {
                             error_reported = true;
                             this.report_borrowed_value_does_not_live_long_enough(
                                 context,
                                 borrow,
                                 place_span,
-                                Some(kind),
-                            );
+                                Some(kind))
                         }
                         WriteKind::Mutate => {
                             error_reported = true;
@@ -1464,7 +1476,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
         }
     }
 
-    /// Returns whether a borrow of this place is invalidated when the function
+    /// Checks whether a borrow of this place is invalidated when the function
     /// exits
     fn check_for_invalidation_at_exit(
         &mut self,
@@ -1889,9 +1901,9 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
 
             Reservation(wk @ WriteKind::Move)
             | Write(wk @ WriteKind::Move)
-            | Reservation(wk @ WriteKind::StorageDeadOrDrop)
+            | Reservation(wk @ WriteKind::StorageDeadOrDrop(_))
             | Reservation(wk @ WriteKind::MutableBorrow(BorrowKind::Shared))
-            | Write(wk @ WriteKind::StorageDeadOrDrop)
+            | Write(wk @ WriteKind::StorageDeadOrDrop(_))
             | Write(wk @ WriteKind::MutableBorrow(BorrowKind::Shared)) => {
                 if let Err(_place_err) = self.is_mutable(place, is_local_mutation_allowed) {
                     if self.tcx.migrate_borrowck() {
@@ -1906,7 +1918,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                         error_access = match wk {
                             WriteKind::MutableBorrow(_) => AccessKind::MutableBorrow,
                             WriteKind::Move => AccessKind::Move,
-                            WriteKind::StorageDeadOrDrop |
+                            WriteKind::StorageDeadOrDrop(_) |
                             WriteKind::Mutate => AccessKind::Mutate,
                         };
                         self.report_mutability_error(

src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs

@@ -154,7 +154,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                         format!("borrow later used here, when `{}` is dropped", local_name),
                     );
 
-                    if let Some((WriteKind::StorageDeadOrDrop, place)) = kind_place {
+                    if let Some((WriteKind::StorageDeadOrDrop(_), place)) = kind_place {
                         if let Place::Local(borrowed_local) = place {
                             let dropped_local_scope = mir.local_decls[local].visibility_scope;
                             let borrowed_local_scope =

src/librustc_mir/borrow_check/nll/invalidation.rs

@@ -16,7 +16,7 @@ use borrow_check::{ReadOrWrite, Activation, Read, Reservation, Write};
 use borrow_check::{Context, ContextKind};
 use borrow_check::{LocalMutationIsAllowed, MutateMode};
 use borrow_check::ArtificialField;
-use borrow_check::{ReadKind, WriteKind};
+use borrow_check::{ReadKind, WriteKind, StorageDeadOrDrop};
 use borrow_check::nll::facts::AllFacts;
 use borrow_check::path_utils::*;
 use dataflow::move_paths::indexes::BorrowIndex;
@@ -154,7 +154,8 @@ impl<'cg, 'cx, 'tcx, 'gcx> Visitor<'tcx> for InvalidationGenerator<'cg, 'cx, 'tc
                 self.access_place(
                     ContextKind::StorageDead.new(location),
                     &Place::Local(local),
-                    (Shallow(None), Write(WriteKind::StorageDeadOrDrop)),
+                    (Shallow(None), Write(WriteKind::StorageDeadOrDrop(
+                        StorageDeadOrDrop::LocalStorageDead))),
                     LocalMutationIsAllowed::Yes,
                 );
             }
@@ -347,7 +348,8 @@ impl<'cg, 'cx, 'tcx, 'gcx> InvalidationGenerator<'cg, 'cx, 'tcx, 'gcx> {
                     self.access_place(
                         ContextKind::Drop.new(loc),
                         drop_place,
-                        (Deep, Write(WriteKind::StorageDeadOrDrop)),
+                        (Deep, Write(WriteKind::StorageDeadOrDrop(
+                            StorageDeadOrDrop::Destructor))),
                         LocalMutationIsAllowed::Yes,
                     );
                 }

src/librustc_mir/diagnostics.rs

@@ -2187,6 +2187,51 @@ fn main() {
 ```
 "##,
 
+E0713: r##"
+This error occurs when an attempt is made to borrow state past the end of the
+lifetime of a type that implements the `Drop` trait.
+
+Example of erroneous code:
+
+```compile_fail,E0713
+pub struct S<'a> { data: &'a mut String }
+
+impl<'a> Drop for S<'a> {
+    fn drop(&mut self) { self.data.push_str("being dropped"); }
+}
+
+fn demo(s: S) -> &mut String { let p = &mut *s.data; p }
+```
+
+Here, `demo` tries to borrow the string data held within its
+argument `s` and then return that borrow. However, `S` is
+declared as implementing `Drop`.
+
+Structs implementing the `Drop` trait have an implicit destructor that
+gets called when they go out of scope. This destructor gets exclusive
+access to the fields of the struct when it runs.
+
+This means that when `s` reaches the end of `demo`, its destructor
+gets exclusive access to its `&mut`-borrowed string data.  allowing
+another borrow of that string data (`p`), to exist across the drop of
+`s` would be a violation of the principle that `&mut`-borrows have
+exclusive, unaliased access to their referenced data.
+
+This error can be fixed by changing `demo` so that the destructor does
+not run while the string-data is borrowed; for example by taking `S`
+by reference:
+
+```
+pub struct S<'a> { data: &'a mut String }
+
+impl<'a> Drop for S<'a> {
+    fn drop(&mut self) { self.data.push_str("being dropped"); }
+}
+
+fn demo(s: &mut S) -> &mut String { let p = &mut *(*s).data; p }
+```
+"##,
+
 }
 
 register_diagnostics! {

src/librustc_mir/util/borrowck_errors.rs

@@ -573,6 +573,22 @@ pub trait BorrowckErrors<'cx>: Sized + Copy {
         self.cancel_if_wrong_origin(err, o)
     }
 
+    fn cannot_borrow_across_destructor(
+        self,
+        borrow_span: Span,
+        o: Origin,
+    ) -> DiagnosticBuilder<'cx> {
+        let err = struct_span_err!(
+            self,
+            borrow_span,
+            E0713,
+            "borrow may still be in use when destructor runs{OGN}",
+            OGN = o
+        );
+
+        self.cancel_if_wrong_origin(err, o)
+    }
+
     fn path_does_not_live_long_enough(
         self,
         span: Span,