Skip to content

Commit

Permalink
Rework checking for borrows conflicting with drops
Browse files Browse the repository at this point in the history
Previously, we would split the drop access into multiple checks for each
field of a struct/tuple/closure and through `Box` dereferences. This
changes this to check if the borrow is accessed by the drop in
places_conflict.

This also allows us to handle enums in a simpler way, since we don't
have to construct any new places.
  • Loading branch information
matthewjasper committed Sep 23, 2018
1 parent d3f9af8 commit 4603fb8
Show file tree
Hide file tree
Showing 8 changed files with 252 additions and 397 deletions.
108 changes: 83 additions & 25 deletions src/librustc_mir/borrow_check/error_reporting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use borrow_check::{WriteKind, StorageDeadOrDrop};
use borrow_check::WriteKind;
use borrow_check::prefixes::IsPrefixOf;
use borrow_check::nll::explain_borrow::BorrowExplanation;
use rustc::middle::region::ScopeTree;
use rustc::mir::{
self, AggregateKind, BindingForm, BorrowKind, ClearCrossCrate, FakeReadCause, Field, Local,
LocalDecl, LocalKind, Location, Operand, Place, ProjectionElem, Rvalue, Statement,
StatementKind, TerminatorKind, VarBindingForm,
LocalDecl, LocalKind, Location, Operand, Place, PlaceProjection, ProjectionElem, Rvalue,
Statement, StatementKind, TerminatorKind, VarBindingForm,
};
use rustc::hir;
use rustc::hir::def_id::DefId;
Expand Down Expand Up @@ -452,13 +452,21 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
self.access_place_error_reported
.insert((root_place.clone(), borrow_span));

if let Some(WriteKind::StorageDeadOrDrop(StorageDeadOrDrop::Destructor)) = kind {
if let StorageDeadOrDrop::Destructor(dropped_ty)
= self.classify_drop_access_kind(&borrow.borrowed_place)
{
// 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, place_span, kind);
self.report_borrow_conflicts_with_destructor(
context,
borrow,
place_span,
kind,
dropped_ty,
);
return;
}
}
Expand Down Expand Up @@ -566,6 +574,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
borrow: &BorrowData<'tcx>,
(place, drop_span): (&Place<'tcx>, Span),
kind: Option<WriteKind>,
dropped_ty: ty::Ty<'tcx>,
) {
debug!(
"report_borrow_conflicts_with_destructor(\
Expand All @@ -579,28 +588,19 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {

let mut err = self.infcx.tcx.cannot_borrow_across_destructor(borrow_span, Origin::Mir);

let (what_was_dropped, dropped_ty) = {
let desc = match self.describe_place(place) {
Some(name) => format!("`{}`", name.as_str()),
None => format!("temporary value"),
};
let ty = place.ty(self.mir, self.infcx.tcx).to_ty(self.infcx.tcx);
(desc, ty)
let what_was_dropped = match self.describe_place(place) {
Some(name) => format!("`{}`", name.as_str()),
None => format!("temporary value"),
};

let label = match dropped_ty.sty {
ty::Adt(adt, _) if adt.has_dtor(self.infcx.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),
let label = 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),
};
err.span_label(drop_span, label);

Expand Down Expand Up @@ -880,6 +880,14 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {

pub(super) struct IncludingDowncast(bool);

/// Which case a StorageDeadOrDrop is for.
#[derive(Copy, Clone, PartialEq, Eq, Debug)]
enum StorageDeadOrDrop<'tcx> {
LocalStorageDead,
BoxedStorageDead,
Destructor(ty::Ty<'tcx>),
}

impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
// End-user visible description of `place` if one can be found. If the
// place is a temporary for instance, None will be returned.
Expand Down Expand Up @@ -1167,6 +1175,56 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
}
}

fn classify_drop_access_kind(&self, place: &Place<'tcx>) -> StorageDeadOrDrop<'tcx> {
let tcx = self.infcx.tcx;
match place {
Place::Local(_)
| Place::Static(_)
| Place::Promoted(_) => StorageDeadOrDrop::LocalStorageDead,
Place::Projection(box PlaceProjection { base, elem }) => {
let base_access = self.classify_drop_access_kind(base);
match elem {
ProjectionElem::Deref => {
match base_access {
StorageDeadOrDrop::LocalStorageDead
| StorageDeadOrDrop::BoxedStorageDead => {
assert!(base.ty(self.mir, tcx).to_ty(tcx).is_box(),
"Drop of value behind a reference or raw pointer");
StorageDeadOrDrop::BoxedStorageDead
}
StorageDeadOrDrop::Destructor(_) => {
base_access
}
}
}
ProjectionElem::Field(..)
| ProjectionElem::Downcast(..) => {
let base_ty = base.ty(self.mir, tcx).to_ty(tcx);
match base_ty.sty {
ty::Adt(def, _) if def.has_dtor(tcx) => {
// Report the outermost adt with a destructor
match base_access {
StorageDeadOrDrop::Destructor(_) => {
base_access
}
StorageDeadOrDrop::LocalStorageDead
| StorageDeadOrDrop::BoxedStorageDead => {
StorageDeadOrDrop::Destructor(base_ty)
}
}
}
_ => base_access,
}
}

ProjectionElem::ConstantIndex { .. }
| ProjectionElem::Subslice { .. }
| ProjectionElem::Index(_) => base_access,
}
}
}
}

/// Annotate argument and return type of function and closure with (synthesized) lifetime for
/// borrow of local value that does not live long enough.
fn annotate_argument_and_return_for_borrow(
Expand Down
Loading

0 comments on commit 4603fb8

Please sign in to comment.