Report when borrow could cause &mut aliasing during Drop

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,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`.