Skip to content

Commit 1f0fbdd

Browse files
pnkfelixpnkfelix
committed
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.
1 parent 2224a42 commit 1f0fbdd

File tree

6 files changed

+187
-21
lines changed

6 files changed

+187
-21
lines changed

‎src/librustc_mir/borrow_check/error_reporting.rs‎

Lines changed: 92 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,8 @@
88
// option. This file may not be copied, modified, or distributed
99
// except according to those terms.
1010

11-
use borrow_check::WriteKind;
11+
use borrow_check::{WriteKind, StorageDeadOrDrop};
12+
use borrow_check::prefixes::IsPrefixOf;
1213
use rustc::middle::region::ScopeTree;
1314
use rustc::mir::VarBindingForm;
1415
use rustc::mir::{BindingForm, BorrowKind, ClearCrossCrate, Field, Local};
@@ -374,13 +375,27 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
374375
err.buffer(&mut self.errors_buffer);
375376
}
376377

378+
/// Reports StorageDeadOrDrop of `place` conflicts with `borrow`.
379+
///
380+
/// This means that some data referenced by `borrow` needs to live
381+
/// past the point where the StorageDeadOrDrop of `place` occurs.
382+
/// This is usually interpreted as meaning that `place` has too
383+
/// short a lifetime. (But sometimes it is more useful to report
384+
/// it as a more direct conflict between the execution of a
385+
/// `Drop::drop` with an aliasing borrow.)
377386
pub(super) fn report_borrowed_value_does_not_live_long_enough(
378387
&mut self,
379388
context: Context,
380389
borrow: &BorrowData<'tcx>,
381390
place_span: (&Place<'tcx>, Span),
382391
kind: Option<WriteKind>,
383392
) {
393+
debug!("report_borrowed_value_does_not_live_long_enough(\
394+
{:?}, {:?}, {:?}, {:?}\
395+
)",
396+
context, borrow, place_span, kind
397+
);
398+
384399
let drop_span = place_span.1;
385400
let scope_tree = self.tcx.region_scope_tree(self.mir_def_id);
386401
let root_place = self
@@ -412,6 +427,19 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
412427

413428
let borrow_reason = self.find_why_borrow_contains_point(context, borrow);
414429

430+
if let Some(WriteKind::StorageDeadOrDrop(StorageDeadOrDrop::Destructor)) = kind
431+
{
432+
// If a borrow of path `B` conflicts with drop of `D` (and
433+
// we're not in the uninteresting case where `B` is a
434+
// prefix of `D`), then report this as a more interesting
435+
// destructor conflict.
436+
if !borrow.borrowed_place.is_prefix_of(place_span.0) {
437+
self.report_borrow_conflicts_with_destructor(
438+
context, borrow, borrow_reason, place_span, kind);
439+
return;
440+
}
441+
}
442+
415443
let mut err = match &self.describe_place(&borrow.borrowed_place) {
416444
Some(_) if self.is_place_thread_local(root_place) => {
417445
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> {
475503
err
476504
}
477505

506+
pub(super) fn report_borrow_conflicts_with_destructor(
507+
&mut self,
508+
context: Context,
509+
borrow: &BorrowData<'tcx>,
510+
borrow_reason: BorrowContainsPointReason<'tcx>,
511+
place_span: (&Place<'tcx>, Span),
512+
kind: Option<WriteKind>,
513+
) {
514+
debug!(
515+
"report_borrow_conflicts_with_destructor(\
516+
{:?}, {:?}, {:?}, {:?} {:?}\
517+
)",
518+
context, borrow, borrow_reason, place_span, kind,
519+
);
520+
521+
let borrow_spans = self.retrieve_borrow_spans(borrow);
522+
let borrow_span = borrow_spans.var_or_use();
523+
524+
let mut err = self.tcx.cannot_borrow_across_destructor(borrow_span, Origin::Mir);
525+
526+
let drop_span = place_span.1;
527+
528+
let (what_was_dropped, dropped_ty) = {
529+
let place = place_span.0;
530+
let desc = match self.describe_place(place) {
531+
Some(name) => format!("`{}`", name.as_str()),
532+
None => format!("temporary value"),
533+
};
534+
let ty = place.ty(self.mir, self.tcx).to_ty(self.tcx);
535+
(desc, ty)
536+
};
537+
538+
let label = match dropped_ty.sty {
539+
ty::Adt(adt, _) if adt.has_dtor(self.tcx) && !adt.is_box() => {
540+
match self.describe_place(&borrow.borrowed_place) {
541+
Some(borrowed) =>
542+
format!("here, drop of {D} needs exclusive access to `{B}`, \
543+
because the type `{T}` implements the `Drop` trait",
544+
D=what_was_dropped, T=dropped_ty, B=borrowed),
545+
None =>
546+
format!("here is drop of {D}; whose type `{T}` implements the `Drop` trait",
547+
D=what_was_dropped, T=dropped_ty),
548+
}
549+
}
550+
_ => format!("drop of {D} occurs here", D=what_was_dropped),
551+
};
552+
err.span_label(drop_span, label);
553+
554+
// Only give this note and suggestion if they could be relevant
555+
match borrow_reason {
556+
BorrowContainsPointReason::Liveness {..}
557+
| BorrowContainsPointReason::DropLiveness {..} => {
558+
err.note("consider using a `let` binding to create a longer lived value");
559+
}
560+
BorrowContainsPointReason::OutlivesFreeRegion {..} => (),
561+
}
562+
563+
self.report_why_borrow_contains_point(
564+
&mut err, borrow_reason, kind.map(|k| (k, place_span.0)));
565+
566+
err.buffer(&mut self.errors_buffer);
567+
}
568+
478569
fn report_thread_local_value_does_not_live_long_enough(
479570
&mut self,
480571
drop_span: Span,

‎src/librustc_mir/borrow_check/mod.rs‎

Lines changed: 28 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -551,7 +551,8 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx
551551
self.access_place(
552552
ContextKind::StorageDead.new(location),
553553
(&Place::Local(local), span),
554-
(Shallow(None), Write(WriteKind::StorageDeadOrDrop)),
554+
(Shallow(None), Write(WriteKind::StorageDeadOrDrop(
555+
StorageDeadOrDrop::LocalStorageDead))),
555556
LocalMutationIsAllowed::Yes,
556557
flow_state,
557558
);
@@ -778,12 +779,21 @@ enum ReadKind {
778779
/// (For informational purposes only)
779780
#[derive(Copy, Clone, PartialEq, Eq, Debug)]
780781
enum WriteKind {
781-
StorageDeadOrDrop,
782+
StorageDeadOrDrop(StorageDeadOrDrop),
782783
MutableBorrow(BorrowKind),
783784
Mutate,
784785
Move,
785786
}
786787

788+
/// Specify whether which case a StorageDeadOrDrop is in.
789+
/// (For informational purposes only)
790+
#[derive(Copy, Clone, PartialEq, Eq, Debug)]
791+
enum StorageDeadOrDrop {
792+
LocalStorageDead,
793+
BoxedStorageDead,
794+
Destructor,
795+
}
796+
787797
/// When checking permissions for a place access, this flag is used to indicate that an immutable
788798
/// local place can be mutated.
789799
///
@@ -1012,7 +1022,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
10121022
self.access_place(
10131023
ContextKind::Drop.new(loc),
10141024
(drop_place, span),
1015-
(Deep, Write(WriteKind::StorageDeadOrDrop)),
1025+
(Deep, Write(WriteKind::StorageDeadOrDrop(
1026+
StorageDeadOrDrop::Destructor))),
10161027
LocalMutationIsAllowed::Yes,
10171028
flow_state,
10181029
);
@@ -1039,15 +1050,17 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
10391050
// Why? Because we do not schedule/emit
10401051
// `Drop(x)` in the MIR unless `x` needs drop in
10411052
// the first place.
1042-
//
1043-
// FIXME: Its possible this logic actually should
1044-
// be attached to the `StorageDead` statement
1045-
// rather than the `Drop`. See discussion on PR
1046-
// #52782.
10471053
self.access_place(
10481054
ContextKind::Drop.new(loc),
10491055
(drop_place, span),
1050-
(Shallow(None), Write(WriteKind::StorageDeadOrDrop)),
1056+
(Shallow(None), Write(WriteKind::StorageDeadOrDrop(
1057+
// rust-lang/rust#52059: distinguish
1058+
// between invaliding the backing storage
1059+
// vs running a destructor.
1060+
//
1061+
// See also: rust-lang/rust#52782,
1062+
// specifically #discussion_r206658948
1063+
StorageDeadOrDrop::BoxedStorageDead))),
10511064
LocalMutationIsAllowed::Yes,
10521065
flow_state,
10531066
);
@@ -1215,14 +1228,13 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
12151228
error_reported = true;
12161229
this.report_conflicting_borrow(context, place_span, bk, &borrow)
12171230
}
1218-
WriteKind::StorageDeadOrDrop => {
1231+
WriteKind::StorageDeadOrDrop(_) => {
12191232
error_reported = true;
12201233
this.report_borrowed_value_does_not_live_long_enough(
12211234
context,
12221235
borrow,
12231236
place_span,
1224-
Some(kind),
1225-
);
1237+
Some(kind))
12261238
}
12271239
WriteKind::Mutate => {
12281240
error_reported = true;
@@ -1464,7 +1476,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
14641476
}
14651477
}
14661478

1467-
/// Returns whether a borrow of this place is invalidated when the function
1479+
/// Checks whether a borrow of this place is invalidated when the function
14681480
/// exits
14691481
fn check_for_invalidation_at_exit(
14701482
&mut self,
@@ -1889,9 +1901,9 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
18891901

18901902
Reservation(wk @ WriteKind::Move)
18911903
| Write(wk @ WriteKind::Move)
1892-
| Reservation(wk @ WriteKind::StorageDeadOrDrop)
1904+
| Reservation(wk @ WriteKind::StorageDeadOrDrop(_))
18931905
| Reservation(wk @ WriteKind::MutableBorrow(BorrowKind::Shared))
1894-
| Write(wk @ WriteKind::StorageDeadOrDrop)
1906+
| Write(wk @ WriteKind::StorageDeadOrDrop(_))
18951907
| Write(wk @ WriteKind::MutableBorrow(BorrowKind::Shared)) => {
18961908
if let Err(_place_err) = self.is_mutable(place, is_local_mutation_allowed) {
18971909
if self.tcx.migrate_borrowck() {
@@ -1906,7 +1918,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
19061918
error_access = match wk {
19071919
WriteKind::MutableBorrow(_) => AccessKind::MutableBorrow,
19081920
WriteKind::Move => AccessKind::Move,
1909-
WriteKind::StorageDeadOrDrop |
1921+
WriteKind::StorageDeadOrDrop(_) |
19101922
WriteKind::Mutate => AccessKind::Mutate,
19111923
};
19121924
self.report_mutability_error(

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
154154
format!("borrow later used here, when `{}` is dropped", local_name),
155155
);
156156

157-
if let Some((WriteKind::StorageDeadOrDrop, place)) = kind_place {
157+
if let Some((WriteKind::StorageDeadOrDrop(_), place)) = kind_place {
158158
if let Place::Local(borrowed_local) = place {
159159
let dropped_local_scope = mir.local_decls[local].visibility_scope;
160160
let borrowed_local_scope =

‎src/librustc_mir/borrow_check/nll/invalidation.rs‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ use borrow_check::{ReadOrWrite, Activation, Read, Reservation, Write};
1616
use borrow_check::{Context, ContextKind};
1717
use borrow_check::{LocalMutationIsAllowed, MutateMode};
1818
use borrow_check::ArtificialField;
19-
use borrow_check::{ReadKind, WriteKind};
19+
use borrow_check::{ReadKind, WriteKind, StorageDeadOrDrop};
2020
use borrow_check::nll::facts::AllFacts;
2121
use borrow_check::path_utils::*;
2222
use dataflow::move_paths::indexes::BorrowIndex;
@@ -154,7 +154,8 @@ impl<'cg, 'cx, 'tcx, 'gcx> Visitor<'tcx> for InvalidationGenerator<'cg, 'cx, 'tc
154154
self.access_place(
155155
ContextKind::StorageDead.new(location),
156156
&Place::Local(local),
157-
(Shallow(None), Write(WriteKind::StorageDeadOrDrop)),
157+
(Shallow(None), Write(WriteKind::StorageDeadOrDrop(
158+
StorageDeadOrDrop::LocalStorageDead))),
158159
LocalMutationIsAllowed::Yes,
159160
);
160161
}
@@ -347,7 +348,8 @@ impl<'cg, 'cx, 'tcx, 'gcx> InvalidationGenerator<'cg, 'cx, 'tcx, 'gcx> {
347348
self.access_place(
348349
ContextKind::Drop.new(loc),
349350
drop_place,
350-
(Deep, Write(WriteKind::StorageDeadOrDrop)),
351+
(Deep, Write(WriteKind::StorageDeadOrDrop(
352+
StorageDeadOrDrop::Destructor))),
351353
LocalMutationIsAllowed::Yes,
352354
);
353355
}

‎src/librustc_mir/diagnostics.rs‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2187,6 +2187,51 @@ fn main() {
21872187
```
21882188
"##,
21892189

2190+
E0713: r##"
2191+
This error occurs when an attempt is made to borrow state past the end of the
2192+
lifetime of a type that implements the `Drop` trait.
2193+
2194+
Example of erroneous code:
2195+
2196+
```compile_fail,E0713
2197+
pub struct S<'a> { data: &'a mut String }
2198+
2199+
impl<'a> Drop for S<'a> {
2200+
fn drop(&mut self) { self.data.push_str("being dropped"); }
2201+
}
2202+
2203+
fn demo(s: S) -> &mut String { let p = &mut *s.data; p }
2204+
```
2205+
2206+
Here, `demo` tries to borrow the string data held within its
2207+
argument `s` and then return that borrow. However, `S` is
2208+
declared as implementing `Drop`.
2209+
2210+
Structs implementing the `Drop` trait have an implicit destructor that
2211+
gets called when they go out of scope. This destructor gets exclusive
2212+
access to the fields of the struct when it runs.
2213+
2214+
This means that when `s` reaches the end of `demo`, its destructor
2215+
gets exclusive access to its `&mut`-borrowed string data. allowing
2216+
another borrow of that string data (`p`), to exist across the drop of
2217+
`s` would be a violation of the principle that `&mut`-borrows have
2218+
exclusive, unaliased access to their referenced data.
2219+
2220+
This error can be fixed by changing `demo` so that the destructor does
2221+
not run while the string-data is borrowed; for example by taking `S`
2222+
by reference:
2223+
2224+
```
2225+
pub struct S<'a> { data: &'a mut String }
2226+
2227+
impl<'a> Drop for S<'a> {
2228+
fn drop(&mut self) { self.data.push_str("being dropped"); }
2229+
}
2230+
2231+
fn demo(s: &mut S) -> &mut String { let p = &mut *(*s).data; p }
2232+
```
2233+
"##,
2234+
21902235
}
21912236

21922237
register_diagnostics! {

‎src/librustc_mir/util/borrowck_errors.rs‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -573,6 +573,22 @@ pub trait BorrowckErrors<'cx>: Sized + Copy {
573573
self.cancel_if_wrong_origin(err, o)
574574
}
575575

576+
fn cannot_borrow_across_destructor(
577+
self,
578+
borrow_span: Span,
579+
o: Origin,
580+
) -> DiagnosticBuilder<'cx> {
581+
let err = struct_span_err!(
582+
self,
583+
borrow_span,
584+
E0713,
585+
"borrow may still be in use when destructor runs{OGN}",
586+
OGN = o
587+
);
588+
589+
self.cancel_if_wrong_origin(err, o)
590+
}
591+
576592
fn path_does_not_live_long_enough(
577593
self,
578594
span: Span,

0 commit comments

Comments
 (0)