From df938252d8a4c006c1c9c3f2314ec6f5889979d8 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 18 Sep 2018 15:13:17 +0200 Subject: [PATCH 1/3] Added `is_drop` boolean flag to `ShallowOrDeep::Deep` variant. No change in functional behavior here. --- src/librustc_mir/borrow_check/mod.rs | 35 +++++++++++-------- .../borrow_check/nll/invalidation.rs | 23 ++++++------ .../borrow_check/places_conflict.rs | 2 +- 3 files changed, 34 insertions(+), 26 deletions(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 3c694fe7b4e58..e2008d9a5f03b 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -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, ); @@ -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, ); @@ -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, ); @@ -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, ); @@ -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, ); @@ -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 @@ -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, ); @@ -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)) } } }; @@ -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, ); @@ -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, ); @@ -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); @@ -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, diff --git a/src/librustc_mir/borrow_check/nll/invalidation.rs b/src/librustc_mir/borrow_check/nll/invalidation.rs index 71345f22e443b..3b77284014b52 100644 --- a/src/librustc_mir/borrow_check/nll/invalidation.rs +++ b/src/librustc_mir/borrow_check/nll/invalidation.rs @@ -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, ); } @@ -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 }, ); } @@ -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( @@ -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, ); } @@ -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, ); } @@ -382,7 +382,7 @@ 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, ); } @@ -390,7 +390,7 @@ impl<'cg, 'cx, 'tcx, 'gcx> InvalidationGenerator<'cg, 'cx, 'tcx, 'gcx> { self.access_place( context, place, - (Deep, Write(WriteKind::Move)), + (Deep { is_drop: false }, Write(WriteKind::Move)), LocalMutationIsAllowed::Yes, ); } @@ -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)) } } }; diff --git a/src/librustc_mir/borrow_check/places_conflict.rs b/src/librustc_mir/borrow_check/places_conflict.rs index 3f055283e0c3f..629f679b6bcd3 100644 --- a/src/librustc_mir/borrow_check/places_conflict.rs +++ b/src/librustc_mir/borrow_check/places_conflict.rs @@ -188,7 +188,7 @@ fn place_components_conflict<'gcx, 'tcx>( return false; } - (ProjectionElem::Deref, _, Deep) + (ProjectionElem::Deref, _, Deep { .. }) | (ProjectionElem::Field { .. }, _, _) | (ProjectionElem::Index { .. }, _, _) | (ProjectionElem::ConstantIndex { .. }, _, _) From 11719d5b26de0012fe195af8bc5f7d28ec73dbab Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 18 Sep 2018 15:13:40 +0200 Subject: [PATCH 2/3] Expand notion of `places_conflict` to more precisely model destructor effects. --- .../borrow_check/places_conflict.rs | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/librustc_mir/borrow_check/places_conflict.rs b/src/librustc_mir/borrow_check/places_conflict.rs index 629f679b6bcd3..503a13ac437df 100644 --- a/src/librustc_mir/borrow_check/places_conflict.rs +++ b/src/librustc_mir/borrow_check/places_conflict.rs @@ -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() { @@ -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))) => { @@ -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. // From 3115e2f62db906ff564cd546c568e7aefb6d21a2 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 18 Sep 2018 16:36:42 +0200 Subject: [PATCH 3/3] Regression test. --- .../nll/issue-53569-precision-drop-effects.rs | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) create mode 100644 src/test/ui/nll/issue-53569-precision-drop-effects.rs diff --git a/src/test/ui/nll/issue-53569-precision-drop-effects.rs b/src/test/ui/nll/issue-53569-precision-drop-effects.rs new file mode 100644 index 0000000000000..635d7a46cf143 --- /dev/null +++ b/src/test/ui/nll/issue-53569-precision-drop-effects.rs @@ -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() { +}