Skip to content

[WIP] More precise conflict modelling between borrows and drops #54324

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

[WIP] More precise conflict modelling between borrows and drops #54324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
df93825
Added `is_drop` boolean flag to `ShallowOrDeep::Deep` variant.
pnkfelix Sep 18, 2018
11719d5
Expand notion of `places_conflict` to more precisely model destructor…
pnkfelix Sep 18, 2018
3115e2f
Regression test.
pnkfelix Sep 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 21 additions & 14 deletions src/librustc_mir/borrow_check/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -482,7 +482,7 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx
self.access_place(
ContextKind::ReadForMatch.new(location),
(place, span),
(Deep, Read(ReadKind::Borrow(BorrowKind::Shared))),
(Deep { is_drop: false }, Read(ReadKind::Borrow(BorrowKind::Shared))),
LocalMutationIsAllowed::No,
flow_state,
);
Expand Down Expand Up @@ -512,7 +512,7 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx
self.access_place(
context,
(output, span),
(Deep, Read(ReadKind::Copy)),
(Deep { is_drop: false }, Read(ReadKind::Copy)),
LocalMutationIsAllowed::No,
flow_state,
);
Expand All @@ -526,7 +526,7 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx
self.mutate_place(
context,
(output, span),
if o.is_rw { Deep } else { Shallow(None) },
if o.is_rw { Deep { is_drop: false } } else { Shallow(None) },
if o.is_rw { WriteAndRead } else { JustWrite },
flow_state,
);
Expand Down Expand Up @@ -617,7 +617,7 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx
self.mutate_place(
ContextKind::DropAndReplace.new(loc),
(drop_place, span),
Deep,
Deep { is_drop: true },
JustWrite,
flow_state,
);
Expand Down Expand Up @@ -645,7 +645,7 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx
self.mutate_place(
ContextKind::CallDest.new(loc),
(dest, span),
Deep,
Deep { is_drop: true },
JustWrite,
flow_state,
);
Expand Down Expand Up @@ -743,7 +743,12 @@ enum ShallowOrDeep {
/// From the RFC: "A *deep* access means that all data reachable
/// through the given place may be invalidated or accesses by
/// this action."
Deep,
///
/// If `is_drop` is true, then this access is due to a drop, which
/// follows a recursive traversal until it hits types that
/// implement the `Drop` trait (and then runs the code associated
/// with those impls of `Drop`).
Deep { is_drop: bool },
}

/// Kind of access to a value: read or write
Expand Down Expand Up @@ -1012,7 +1017,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
self.access_place(
ContextKind::Drop.new(loc),
(drop_place, span),
(Deep, Write(WriteKind::StorageDeadOrDrop)),
(Deep { is_drop: true },
Write(WriteKind::StorageDeadOrDrop)),
LocalMutationIsAllowed::Yes,
flow_state,
);
Expand Down Expand Up @@ -1300,13 +1306,14 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
match *rvalue {
Rvalue::Ref(_ /*rgn*/, bk, ref place) => {
let access_kind = match bk {
BorrowKind::Shared => (Deep, Read(ReadKind::Borrow(bk))),
BorrowKind::Shared => (Deep { is_drop: false },
Read(ReadKind::Borrow(bk))),
BorrowKind::Unique | BorrowKind::Mut { .. } => {
let wk = WriteKind::MutableBorrow(bk);
if allow_two_phase_borrow(&self.tcx, bk) {
(Deep, Reservation(wk))
(Deep { is_drop: false }, Reservation(wk))
} else {
(Deep, Write(wk))
(Deep { is_drop: false }, Write(wk))
}
}
};
Expand Down Expand Up @@ -1429,7 +1436,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
self.access_place(
context,
(place, span),
(Deep, Read(ReadKind::Copy)),
(Deep { is_drop: false }, Read(ReadKind::Copy)),
LocalMutationIsAllowed::No,
flow_state,
);
Expand All @@ -1447,7 +1454,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
self.access_place(
context,
(place, span),
(Deep, Write(WriteKind::Move)),
(Deep { is_drop: false }, Write(WriteKind::Move)),
LocalMutationIsAllowed::Yes,
flow_state,
);
Expand Down Expand Up @@ -1509,7 +1516,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {

// FIXME: replace this with a proper borrow_conflicts_with_place when
// that is merged.
let sd = if might_be_alive { Deep } else { Shallow(None) };
let sd = if might_be_alive { Deep { is_drop: true } } else { Shallow(None) };

if places_conflict::places_conflict(self.tcx, self.mir, place, root_place, sd) {
debug!("check_for_invalidation_at_exit({:?}): INVALID", place);
Expand Down Expand Up @@ -1569,7 +1576,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
ContextKind::Activation.new(location),
(&borrow.borrowed_place, span),
(
Deep,
Deep { is_drop: false },
Activation(WriteKind::MutableBorrow(borrow.kind), borrow_index),
),
LocalMutationIsAllowed::No,
Expand Down
23 changes: 12 additions & 11 deletions src/librustc_mir/borrow_check/nll/invalidation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ impl<'cg, 'cx, 'tcx, 'gcx> Visitor<'tcx> for InvalidationGenerator<'cg, 'cx, 'tc
self.access_place(
ContextKind::ReadForMatch.new(location),
place,
(Deep, Read(ReadKind::Borrow(BorrowKind::Shared))),
(Deep { is_drop: false }, Read(ReadKind::Borrow(BorrowKind::Shared))),
LocalMutationIsAllowed::No,
);
}
Expand Down Expand Up @@ -125,14 +125,14 @@ impl<'cg, 'cx, 'tcx, 'gcx> Visitor<'tcx> for InvalidationGenerator<'cg, 'cx, 'tc
self.access_place(
context,
output,
(Deep, Read(ReadKind::Copy)),
(Deep { is_drop: false }, Read(ReadKind::Copy)),
LocalMutationIsAllowed::No,
);
} else {
self.mutate_place(
context,
output,
if o.is_rw { Deep } else { Shallow(None) },
if o.is_rw { Deep { is_drop: false } } else { Shallow(None) },
if o.is_rw { WriteAndRead } else { JustWrite },
);
}
Expand Down Expand Up @@ -199,7 +199,7 @@ impl<'cg, 'cx, 'tcx, 'gcx> Visitor<'tcx> for InvalidationGenerator<'cg, 'cx, 'tc
self.mutate_place(
ContextKind::DropAndReplace.new(location),
drop_place,
Deep,
Deep { is_drop: true },
JustWrite,
);
self.consume_operand(
Expand All @@ -221,7 +221,7 @@ impl<'cg, 'cx, 'tcx, 'gcx> Visitor<'tcx> for InvalidationGenerator<'cg, 'cx, 'tc
self.mutate_place(
ContextKind::CallDest.new(location),
dest,
Deep,
Deep { is_drop: true },
JustWrite,
);
}
Expand Down Expand Up @@ -347,7 +347,7 @@ impl<'cg, 'cx, 'tcx, 'gcx> InvalidationGenerator<'cg, 'cx, 'tcx, 'gcx> {
self.access_place(
ContextKind::Drop.new(loc),
drop_place,
(Deep, Write(WriteKind::StorageDeadOrDrop)),
(Deep { is_drop: true }, Write(WriteKind::StorageDeadOrDrop)),
LocalMutationIsAllowed::Yes,
);
}
Expand Down Expand Up @@ -382,15 +382,15 @@ impl<'cg, 'cx, 'tcx, 'gcx> InvalidationGenerator<'cg, 'cx, 'tcx, 'gcx> {
self.access_place(
context,
place,
(Deep, Read(ReadKind::Copy)),
(Deep { is_drop: false }, Read(ReadKind::Copy)),
LocalMutationIsAllowed::No,
);
}
Operand::Move(ref place) => {
self.access_place(
context,
place,
(Deep, Write(WriteKind::Move)),
(Deep { is_drop: false }, Write(WriteKind::Move)),
LocalMutationIsAllowed::Yes,
);
}
Expand All @@ -407,13 +407,14 @@ impl<'cg, 'cx, 'tcx, 'gcx> InvalidationGenerator<'cg, 'cx, 'tcx, 'gcx> {
match *rvalue {
Rvalue::Ref(_ /*rgn*/, bk, ref place) => {
let access_kind = match bk {
BorrowKind::Shared => (Deep, Read(ReadKind::Borrow(bk))),
BorrowKind::Shared => (Deep { is_drop: false },
Read(ReadKind::Borrow(bk))),
BorrowKind::Unique | BorrowKind::Mut { .. } => {
let wk = WriteKind::MutableBorrow(bk);
if allow_two_phase_borrow(&self.infcx.tcx, bk) {
(Deep, Reservation(wk))
(Deep { is_drop: false }, Reservation(wk))
} else {
(Deep, Write(wk))
(Deep { is_drop: false }, Write(wk))
}
}
};
Expand Down
22 changes: 21 additions & 1 deletion src/librustc_mir/borrow_check/places_conflict.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,8 @@ fn place_components_conflict<'gcx, 'tcx>(
// - If we didn't run out of access to match, our borrow and access are comparable
// and either equal or disjoint.
// - If we did run out of access, the borrow can access a part of it.

let mut saw_drop_impl = false;
loop {
// loop invariant: borrow_c is always either equal to access_c or disjoint from it.
if let Some(borrow_c) = borrow_components.next() {
Expand Down Expand Up @@ -155,6 +157,15 @@ fn place_components_conflict<'gcx, 'tcx>(
};
let base_ty = base.ty(mir, tcx).to_ty(tcx);

// record for future reference if we see destructor
// applicable to `x.y.z` while traversing borrow of
// path like `x.y.z.w`.
if let ty::Adt(def, _substs) = &base_ty.sty {
if def.is_struct() && def.has_dtor(tcx) {
saw_drop_impl = true;
}
}

match (elem, &base_ty.sty, access) {
(_, _, Shallow(Some(ArtificialField::Discriminant)))
| (_, _, Shallow(Some(ArtificialField::ArrayLength))) => {
Expand All @@ -178,6 +189,15 @@ fn place_components_conflict<'gcx, 'tcx>(
debug!("places_conflict: shallow access behind ptr");
return false;
}
(ProjectionElem::Deref, _, Deep { is_drop: true }) if !saw_drop_impl => {
// e.g. a borrow of `*(x.0 as Ok).0` during
// access `drop(x)` where `x` is
// `(Result<&u32, D1>, D2)`. Then dropping `x`
// cannot observe borrowed state.
debug!("places_conflict: access behind ptr of non-drop path");
return false;
}

(ProjectionElem::Deref, ty::Ref(_, _, hir::MutImmutable), _) => {
// the borrow goes through a dereference of a shared reference.
//
Expand All @@ -188,7 +208,7 @@ fn place_components_conflict<'gcx, 'tcx>(
return false;
}

(ProjectionElem::Deref, _, Deep)
(ProjectionElem::Deref, _, Deep { .. })
| (ProjectionElem::Field { .. }, _, _)
| (ProjectionElem::Index { .. }, _, _)
| (ProjectionElem::ConstantIndex { .. }, _, _)
Expand Down
32 changes: 32 additions & 0 deletions src/test/ui/nll/issue-53569-precision-drop-effects.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
#![feature(nll)]
#![allow(dead_code)]

// compile-pass

// rust-lang/rust#53569: a drop imposed solely by one enum variant
// should not conflict with a reborrow from another variant of that
// enum.

struct S<'a> {
value: &'a Value,
}

struct Value {
data: u32,
}

impl<'a> S<'a> {
fn get(&self) -> Result<&'a mut Value, String> {
unimplemented!();
}

fn at(&self) {
let v = self.get();
if let Ok(Value { ref mut data }) = v {
let _res : &'a u32 = data;
}
}
}

fn main() {
}