Auto merge of #54310 - pnkfelix:issue-52059-report-borrow-drop-conflict, r=nikomatsakis

Report when borrow could cause `&mut` aliasing during Drop

We were already issuing an error for the cases where this cropped up, so this is not fixing any soundness holes. The previous diagnostic just wasn't accurately describing the problem in the user's code.

Fix #52059

Author: bors

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};
@@ -394,13 +395,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
@@ -432,6 +447,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)
@@ -495,6 +523,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

@@ -574,7 +574,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,
                 );
@@ -801,12 +802,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.
 ///
@@ -1035,7 +1045,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,
                     );
@@ -1062,15 +1073,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,
                     );
@@ -1238,14 +1251,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;
@@ -1487,7 +1499,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,
@@ -1912,9 +1924,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() {
@@ -1929,7 +1941,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

@@ -160,7 +160,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,63 @@ 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
+#![feature(nll)]
+
+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<'a>(s: S<'a>) -> &'a 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:
+
+```
+#![feature(nll)]
+
+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<'a>(s: &'a mut S<'a>) -> &'a mut String { let p = &mut *(*s).data; p }
+```
+
+Note that this approach needs a reference to S with lifetime `'a`.
+Nothing shorter than `'a` will suffice: a shorter lifetime would imply
+that after `demo` finishes excuting, something else (such as the
+destructor!) could access `s.data` after the end of that shorter
+lifetime, which would again violate the `&mut`-borrow's exclusive
+access.
+
+"##,
+
 }
 
 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,

src/test/ui/borrowck/borrowck-fn-in-const-c.nll.stderr

@@ -1,13 +1,13 @@
-error[E0597]: `local.inner` does not live long enough
+error[E0713]: borrow may still be in use when destructor runs
   --> $DIR/borrowck-fn-in-const-c.rs:27:16
    |
 LL |         return &local.inner; //~ ERROR does not live long enough
-   |                ^^^^^^^^^^^^ borrowed value does not live long enough
+   |                ^^^^^^^^^^^^
 LL |     }
-   |     - `local.inner` dropped here while still borrowed
+   |     - here, drop of `local` needs exclusive access to `local.inner`, because the type `DropString` implements the `Drop` trait
    |
    = note: borrowed value must be valid for the static lifetime...
 
 error: aborting due to previous error
 
-For more information about this error, try `rustc --explain E0597`.
+For more information about this error, try `rustc --explain E0713`.