diff --git a/src/librustc_mir/borrow_check/error_reporting.rs b/src/librustc_mir/borrow_check/error_reporting.rs index 4671332f2824..73db525eb569 100644 --- a/src/librustc_mir/borrow_check/error_reporting.rs +++ b/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,6 +375,14 @@ 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, @@ -381,6 +390,12 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { place_span: (&Place<'tcx>, Span), kind: Option, ) { + 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, + ) { + 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, diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 3c694fe7b4e5..22cdc5d6f40e 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/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( diff --git a/src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs b/src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs index 414cb1d6f05c..8a44895a97c6 100644 --- a/src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs +++ b/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 = diff --git a/src/librustc_mir/borrow_check/nll/invalidation.rs b/src/librustc_mir/borrow_check/nll/invalidation.rs index 71345f22e443..81f7fa89e994 100644 --- a/src/librustc_mir/borrow_check/nll/invalidation.rs +++ b/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, ); } diff --git a/src/librustc_mir/diagnostics.rs b/src/librustc_mir/diagnostics.rs index ae4713a65de6..a09eece02661 100644 --- a/src/librustc_mir/diagnostics.rs +++ b/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! { diff --git a/src/librustc_mir/util/borrowck_errors.rs b/src/librustc_mir/util/borrowck_errors.rs index b67780ccdbc1..82617ee10747 100644 --- a/src/librustc_mir/util/borrowck_errors.rs +++ b/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,