From c3618c8b2e87d58fc5e8f18f5a2f8801e29c01e7 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Thu, 26 Jul 2018 22:29:50 +0200 Subject: [PATCH 1/6] Special-case `Box` in `rustc_mir::borrow_check`. This should address issue 45696. Since we know dropping a box will not access any `&mut` or `&` references, it is safe to model its destructor as only touching the contents *owned* by the box. Note: At some point we may want to generalize this machinery to other reference and collection types that are "pure" in the same sense as box. If we add a `&move` reference type, it would probably also fall into this branch of code. But for the short term, we will be conservative and restrict this change to `Box` alone. The code works by recursively descending a deref of the `Box`. We prevent `visit_terminator_drop` infinite-loop (which can arise in a very obscure scenario) via a linked-list of seen types. Note: A similar style stack-only linked-list definition can be found in `rustc_mir::borrow_check::places_conflict`. It might be good at some point in the future to unify the two types and put the resulting definition into `librustc_data_structures/`. ---- One final note: Review feedback led to significant simplification of logic here. During review, eddyb RalfJung and I uncovered the heart of why I needed a so-called "step 2" aka the Shallow Write to the Deref of the box. It was because the `visit_terminator_drop`, in its base case, will not emit any write at all (shallow or deep) to a place unless that place has a need_drop. So I was encoding a Shallow Write by hand for a `Box`, as a separate step from recursively descending through `*a_box` (which was at the time known as "step 1"; it is now the *only* step, apart from the change to the base case for `visit_terminator_drop` that this commit now has encoded). eddyb aruged that *something* should be emitting some sort of write in the base case here (even a shallow one), of the dropped place, since by analogy we also emit a write when you *move* a place. That led to the revision here in this commit. * (Its possible that this desired write should be attached in some manner to StorageDead instead of Drop. But in this PR, I tried to leave the StorageDead logic alone and focus my attention solely on how Drop(x) is modelled in MIR-borrowck.) --- src/librustc_mir/borrow_check/mod.rs | 151 ++++++++++++++++++++++++++- 1 file changed, 146 insertions(+), 5 deletions(-) diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 27221296ff31f..4596c7be1c557 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -22,7 +22,7 @@ use rustc::mir::{ClearCrossCrate, Local, Location, Mir, Mutability, Operand, Pla use rustc::mir::{Field, Projection, ProjectionElem, Rvalue, Statement, StatementKind}; use rustc::mir::{Terminator, TerminatorKind}; use rustc::ty::query::Providers; -use rustc::ty::{self, ParamEnv, TyCtxt}; +use rustc::ty::{self, ParamEnv, TyCtxt, Ty}; use rustc_errors::{Diagnostic, DiagnosticBuilder, Level}; use rustc_data_structures::graph::dominators::Dominators; @@ -598,7 +598,12 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx // that is useful later. let drop_place_ty = gcx.lift(&drop_place_ty).unwrap(); - self.visit_terminator_drop(loc, term, flow_state, drop_place, drop_place_ty, span); + debug!("visit_terminator_drop \ + loc: {:?} term: {:?} drop_place: {:?} drop_place_ty: {:?} span: {:?}", + loc, term, drop_place, drop_place_ty, span); + + self.visit_terminator_drop( + loc, term, flow_state, drop_place, drop_place_ty, span, SeenTy(None)); } TerminatorKind::DropAndReplace { location: ref drop_place, @@ -832,6 +837,35 @@ impl InitializationRequiringAction { } } +/// A simple linked-list threaded up the stack of recursive calls in `visit_terminator_drop`. +#[derive(Copy, Clone, Debug)] +struct SeenTy<'a, 'gcx: 'a>(Option<(Ty<'gcx>, &'a SeenTy<'a, 'gcx>)>); + +impl<'a, 'gcx> SeenTy<'a, 'gcx> { + /// Return a new list with `ty` prepended to the front of `self`. + fn cons(&'a self, ty: Ty<'gcx>) -> Self { + SeenTy(Some((ty, self))) + } + + /// True if and only if `ty` occurs on the linked list `self`. + fn have_seen(self, ty: Ty) -> bool { + let mut this = self.0; + loop { + match this { + None => return false, + Some((seen_ty, recur)) => { + if seen_ty == ty { + return true; + } else { + this = recur.0; + continue; + } + } + } + } + } +} + impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { /// Invokes `access_place` as appropriate for dropping the value /// at `drop_place`. Note that the *actual* `Drop` in the MIR is @@ -847,14 +881,57 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { drop_place: &Place<'tcx>, erased_drop_place_ty: ty::Ty<'gcx>, span: Span, + prev_seen: SeenTy<'_, 'gcx>, ) { + if prev_seen.have_seen(erased_drop_place_ty) { + // if we have directly seen the input ty `T`, then we must + // have had some *direct* ownership loop between `T` and + // some directly-owned (as in, actually traversed by + // recursive calls below) part that is also of type `T`. + // + // Note: in *all* such cases, the data in question cannot + // be constructed (nor destructed) in finite time/space. + // + // Proper examples, some of which are statically rejected: + // + // * `struct A { field: A, ... }`: + // statically rejected as infinite size + // + // * `type B = (B, ...);`: + // statically rejected as cyclic + // + // * `struct C { field: Box, ... }` + // * `struct D { field: Box<(D, D)>, ... }`: + // *accepted*, though impossible to construct + // + // Here is *NOT* an example: + // * `struct Z { field: Option>, ... }`: + // Here, the type is both representable in finite space (due to the boxed indirection) + // and constructable in finite time (since the recursion can bottom out with `None`). + // This is an obvious instance of something the compiler must accept. + // + // Since some of the above impossible cases like `C` and + // `D` are accepted by the compiler, we must take care not + // to infinite-loop while processing them. But since such + // cases cannot actually arise, it is sound for us to just + // skip them during drop. If the developer uses unsafe + // code to construct them, they should not be surprised by + // weird drop behavior in their resulting code. + debug!("visit_terminator_drop previously seen \ + erased_drop_place_ty: {:?} on prev_seen: {:?}; returning early.", + erased_drop_place_ty, prev_seen); + return; + } + let gcx = self.tcx.global_tcx(); let drop_field = |mir: &mut MirBorrowckCtxt<'cx, 'gcx, 'tcx>, (index, field): (usize, ty::Ty<'gcx>)| { let field_ty = gcx.normalize_erasing_regions(mir.param_env, field); let place = drop_place.clone().field(Field::new(index), field_ty); - mir.visit_terminator_drop(loc, term, flow_state, &place, field_ty, span); + debug!("visit_terminator_drop drop_field place: {:?} field_ty: {:?}", place, field_ty); + let seen = prev_seen.cons(erased_drop_place_ty); + mir.visit_terminator_drop(loc, term, flow_state, &place, field_ty, span, seen); }; match erased_drop_place_ty.sty { @@ -899,13 +976,42 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { .enumerate() .for_each(|field| drop_field(self, field)); } + + // #45696: special-case Box by treating its dtor as + // only deep *across owned content*. Namely, we know + // dropping a box does not touch data behind any + // references it holds; if we were to instead fall into + // the base case below, we would have a Deep Write due to + // the box being `needs_drop`, and that Deep Write would + // touch `&mut` data in the box. + ty::TyAdt(def, _) if def.is_box() => { + // When/if we add a `&own T` type, this action would + // be like running the destructor of the `&own T`. + // (And the owner of backing storage referenced by the + // `&own T` would be responsible for deallocating that + // backing storage.) + + // we model dropping any content owned by the box by + // recurring on box contents. This catches cases like + // `Box>>`, while + // still restricting Write to *owned* content. + let ty = erased_drop_place_ty.boxed_ty(); + let deref_place = drop_place.clone().deref(); + debug!("visit_terminator_drop drop-box-content deref_place: {:?} ty: {:?}", + deref_place, ty); + let seen = prev_seen.cons(erased_drop_place_ty); + self.visit_terminator_drop( + loc, term, flow_state, &deref_place, ty, span, seen); + } + _ => { // We have now refined the type of the value being // dropped (potentially) to just the type of a // subfield; so check whether that field's type still - // "needs drop". If so, we assume that the destructor - // may access any data it likes (i.e., a Deep Write). + // "needs drop". if erased_drop_place_ty.needs_drop(gcx, self.param_env) { + // If so, we assume that the destructor may access + // any data it likes (i.e., a Deep Write). self.access_place( ContextKind::Drop.new(loc), (drop_place, span), @@ -913,6 +1019,41 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { LocalMutationIsAllowed::Yes, flow_state, ); + } else { + // If there is no destructor, we still include a + // *shallow* write. This essentially ensures that + // borrows of the memory directly at `drop_place` + // cannot continue to be borrowed across the drop. + // + // If we were to use a Deep Write here, then any + // `&mut T` that is reachable from `drop_place` + // would get invalidated; fixing that is the + // essence of resolving issue #45696. + // + // * Note: In the compiler today, doing a Deep + // Write here would not actually break + // anything beyond #45696; for example it does not + // break this example: + // + // ```rust + // fn reborrow(x: &mut i32) -> &mut i32 { &mut *x } + // ``` + // + // 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)), + LocalMutationIsAllowed::Yes, + flow_state, + ); } } } From 88284baa0e3a8e8f9274b4c5a76803d84c9533a9 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 27 Jul 2018 17:58:17 +0200 Subject: [PATCH 2/6] minor fallout from the change. (Presumably the place that borrow_check ends up reporting for the error about is no longer the root `Local` itself, and thus the note diagnostic here stops firing.) --- src/test/ui/generator/dropck.nll.stderr | 2 -- src/test/ui/span/dropck-object-cycle.nll.stderr | 2 -- 2 files changed, 4 deletions(-) diff --git a/src/test/ui/generator/dropck.nll.stderr b/src/test/ui/generator/dropck.nll.stderr index ef7e64ffd97ae..b49bf81715079 100644 --- a/src/test/ui/generator/dropck.nll.stderr +++ b/src/test/ui/generator/dropck.nll.stderr @@ -9,8 +9,6 @@ LL | } | | | `*cell` dropped here while still borrowed | borrow later used here, when `gen` is dropped - | - = note: values in a scope are dropped in the opposite order they are defined error[E0597]: `ref_` does not live long enough --> $DIR/dropck.rs:22:11 diff --git a/src/test/ui/span/dropck-object-cycle.nll.stderr b/src/test/ui/span/dropck-object-cycle.nll.stderr index 225ed0f9cc832..08e4b9ec9faa2 100644 --- a/src/test/ui/span/dropck-object-cycle.nll.stderr +++ b/src/test/ui/span/dropck-object-cycle.nll.stderr @@ -9,8 +9,6 @@ LL | } | | | `*m` dropped here while still borrowed | borrow later used here, when `m` is dropped - | - = note: values in a scope are dropped in the opposite order they are defined error: aborting due to previous error From 08b3a8e4294484adcb8b7ed36e76f49e76c3a5f8 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 27 Jul 2018 15:52:17 +0200 Subject: [PATCH 3/6] Regression tests. --- .../issue-45696-long-live-borrows-in-boxes.rs | 103 ++++++++++++++++ ...-45696-scribble-on-boxed-borrow.ast.stderr | 14 +++ ...96-scribble-on-boxed-borrow.migrate.stderr | 69 +++++++++++ ...-45696-scribble-on-boxed-borrow.nll.stderr | 48 ++++++++ .../issue-45696-scribble-on-boxed-borrow.rs | 110 ++++++++++++++++++ 5 files changed, 344 insertions(+) create mode 100644 src/test/ui/issue-45696-long-live-borrows-in-boxes.rs create mode 100644 src/test/ui/issue-45696-scribble-on-boxed-borrow.ast.stderr create mode 100644 src/test/ui/issue-45696-scribble-on-boxed-borrow.migrate.stderr create mode 100644 src/test/ui/issue-45696-scribble-on-boxed-borrow.nll.stderr create mode 100644 src/test/ui/issue-45696-scribble-on-boxed-borrow.rs diff --git a/src/test/ui/issue-45696-long-live-borrows-in-boxes.rs b/src/test/ui/issue-45696-long-live-borrows-in-boxes.rs new file mode 100644 index 0000000000000..e5326bb315ea3 --- /dev/null +++ b/src/test/ui/issue-45696-long-live-borrows-in-boxes.rs @@ -0,0 +1,103 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// rust-lang/rust#45696: This test is checking that we can return +// mutable borrows owned by boxes even when the boxes are dropped. +// +// We will explicitly test AST-borrowck, NLL, and migration modes; +// thus we will also skip the automated compare-mode=nll. + +// revisions: ast nll migrate +// ignore-compare-mode-nll + +#![cfg_attr(nll, feature(nll))] +//[migrate]compile-flags: -Z borrowck=migrate -Z two-phase-borrows + +// run-pass + +type Boxed<'a, 'b> = Box<(&'a mut u32, &'b mut u32)>; + +fn return_borrow_from_dropped_box<'a>(x: Boxed<'a, '_>) -> &'a mut u32 { + &mut *x.0 +} + +fn return_borrow_from_dropped_tupled_box<'a>(x: (Boxed<'a, '_>, &mut u32)) -> &'a mut u32 { + &mut *(x.0).0 +} + +fn basic_tests() { + let mut x = 2; + let mut y = 3; + let mut z = 4; + *return_borrow_from_dropped_box(Box::new((&mut x, &mut y))) += 10; + assert_eq!((x, y, z), (12, 3, 4)); + *return_borrow_from_dropped_tupled_box((Box::new((&mut x, &mut y)), &mut z)) += 10; + assert_eq!((x, y, z), (22, 3, 4)); +} + +// These scribbling tests have been transcribed from +// issue-45696-scribble-on-boxed-borrow.rs +// +// In the context of that file, these tests are meant to show cases +// that should be *accepted* by the compiler, so here we are actually +// checking that the code we get when they are compiled matches our +// expectations. + +struct Scribble<'a>(&'a mut u32); + +impl<'a> Drop for Scribble<'a> { fn drop(&mut self) { *self.0 = 42; } } + +// this is okay, in both AST-borrowck and NLL: The `Scribble` here *has* +// to strictly outlive `'a` +fn borrowed_scribble<'a>(s: &'a mut Scribble) -> &'a mut u32 { + &mut *s.0 +} + +// this, by analogy to previous case, is also okay. +fn boxed_borrowed_scribble<'a>(s: Box<&'a mut Scribble>) -> &'a mut u32 { + &mut *(*s).0 +} + +// this, by analogy to previous case, is also okay. +fn boxed_boxed_borrowed_scribble<'a>(s: Box>) -> &'a mut u32 { + &mut *(**s).0 +} + +fn scribbling_tests() { + let mut x = 1; + { + let mut long_lived = Scribble(&mut x); + *borrowed_scribble(&mut long_lived) += 10; + assert_eq!(*long_lived.0, 11); + // (Scribble dtor runs here, after `&mut`-borrow above ends) + } + assert_eq!(x, 42); + x = 1; + { + let mut long_lived = Scribble(&mut x); + *boxed_borrowed_scribble(Box::new(&mut long_lived)) += 10; + assert_eq!(*long_lived.0, 11); + // (Scribble dtor runs here, after `&mut`-borrow above ends) + } + assert_eq!(x, 42); + x = 1; + { + let mut long_lived = Scribble(&mut x); + *boxed_boxed_borrowed_scribble(Box::new(Box::new(&mut long_lived))) += 10; + assert_eq!(*long_lived.0, 11); + // (Scribble dtor runs here, after `&mut`-borrow above ends) + } + assert_eq!(x, 42); +} + +fn main() { + basic_tests(); + scribbling_tests(); +} diff --git a/src/test/ui/issue-45696-scribble-on-boxed-borrow.ast.stderr b/src/test/ui/issue-45696-scribble-on-boxed-borrow.ast.stderr new file mode 100644 index 0000000000000..6172a5e35a8d9 --- /dev/null +++ b/src/test/ui/issue-45696-scribble-on-boxed-borrow.ast.stderr @@ -0,0 +1,14 @@ +error: compilation successful + --> $DIR/issue-45696-scribble-on-boxed-borrow.rs:89:1 + | +LL | / fn main() { //[ast]~ ERROR compilation successful +LL | | //[migrate]~^ ERROR compilation successful +LL | | let mut x = 1; +LL | | { +... | +LL | | *boxed_boxed_scribbled(Box::new(Box::new(Scribble(&mut x)))) += 10; +LL | | } + | |_^ + +error: aborting due to previous error + diff --git a/src/test/ui/issue-45696-scribble-on-boxed-borrow.migrate.stderr b/src/test/ui/issue-45696-scribble-on-boxed-borrow.migrate.stderr new file mode 100644 index 0000000000000..da0dfac2d18b1 --- /dev/null +++ b/src/test/ui/issue-45696-scribble-on-boxed-borrow.migrate.stderr @@ -0,0 +1,69 @@ +warning[E0597]: `*s.0` does not live long enough + --> $DIR/issue-45696-scribble-on-boxed-borrow.rs:63:5 + | +LL | &mut *s.0 //[nll]~ ERROR `*s.0` does not live long enough [E0597] + | ^^^^^^^^^ borrowed value does not live long enough +... +LL | } + | - `*s.0` dropped here while still borrowed + | +note: borrowed value must be valid for the lifetime 'a as defined on the function body at 62:14... + --> $DIR/issue-45696-scribble-on-boxed-borrow.rs:62:14 + | +LL | fn scribbled<'a>(s: Scribble<'a>) -> &'a mut u32 { + | ^^ + = warning: This error has been downgraded to a warning for backwards compatibility with previous releases. + It represents potential unsoundness in your code. + This warning will become a hard error in the future. + +warning[E0597]: `*s.0` does not live long enough + --> $DIR/issue-45696-scribble-on-boxed-borrow.rs:73:5 + | +LL | &mut *(*s).0 //[nll]~ ERROR `*s.0` does not live long enough [E0597] + | ^^^^^^^^^^^^ borrowed value does not live long enough +... +LL | } + | - `*s.0` dropped here while still borrowed + | +note: borrowed value must be valid for the lifetime 'a as defined on the function body at 72:20... + --> $DIR/issue-45696-scribble-on-boxed-borrow.rs:72:20 + | +LL | fn boxed_scribbled<'a>(s: Box>) -> &'a mut u32 { + | ^^ + = warning: This error has been downgraded to a warning for backwards compatibility with previous releases. + It represents potential unsoundness in your code. + This warning will become a hard error in the future. + +warning[E0597]: `*s.0` does not live long enough + --> $DIR/issue-45696-scribble-on-boxed-borrow.rs:83:5 + | +LL | &mut *(**s).0 //[nll]~ ERROR `*s.0` does not live long enough [E0597] + | ^^^^^^^^^^^^^ borrowed value does not live long enough +... +LL | } + | - `*s.0` dropped here while still borrowed + | +note: borrowed value must be valid for the lifetime 'a as defined on the function body at 82:26... + --> $DIR/issue-45696-scribble-on-boxed-borrow.rs:82:26 + | +LL | fn boxed_boxed_scribbled<'a>(s: Box>>) -> &'a mut u32 { + | ^^ + = warning: This error has been downgraded to a warning for backwards compatibility with previous releases. + It represents potential unsoundness in your code. + This warning will become a hard error in the future. + +error: compilation successful + --> $DIR/issue-45696-scribble-on-boxed-borrow.rs:89:1 + | +LL | / fn main() { //[ast]~ ERROR compilation successful +LL | | //[migrate]~^ ERROR compilation successful +LL | | let mut x = 1; +LL | | { +... | +LL | | *boxed_boxed_scribbled(Box::new(Box::new(Scribble(&mut x)))) += 10; +LL | | } + | |_^ + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0597`. diff --git a/src/test/ui/issue-45696-scribble-on-boxed-borrow.nll.stderr b/src/test/ui/issue-45696-scribble-on-boxed-borrow.nll.stderr new file mode 100644 index 0000000000000..09cbc2f945129 --- /dev/null +++ b/src/test/ui/issue-45696-scribble-on-boxed-borrow.nll.stderr @@ -0,0 +1,48 @@ +error[E0597]: `*s.0` does not live long enough + --> $DIR/issue-45696-scribble-on-boxed-borrow.rs:63:5 + | +LL | &mut *s.0 //[nll]~ ERROR `*s.0` does not live long enough [E0597] + | ^^^^^^^^^ borrowed value does not live long enough +... +LL | } + | - `*s.0` dropped here while still borrowed + | +note: borrowed value must be valid for the lifetime 'a as defined on the function body at 62:14... + --> $DIR/issue-45696-scribble-on-boxed-borrow.rs:62:14 + | +LL | fn scribbled<'a>(s: Scribble<'a>) -> &'a mut u32 { + | ^^ + +error[E0597]: `*s.0` does not live long enough + --> $DIR/issue-45696-scribble-on-boxed-borrow.rs:73:5 + | +LL | &mut *(*s).0 //[nll]~ ERROR `*s.0` does not live long enough [E0597] + | ^^^^^^^^^^^^ borrowed value does not live long enough +... +LL | } + | - `*s.0` dropped here while still borrowed + | +note: borrowed value must be valid for the lifetime 'a as defined on the function body at 72:20... + --> $DIR/issue-45696-scribble-on-boxed-borrow.rs:72:20 + | +LL | fn boxed_scribbled<'a>(s: Box>) -> &'a mut u32 { + | ^^ + +error[E0597]: `*s.0` does not live long enough + --> $DIR/issue-45696-scribble-on-boxed-borrow.rs:83:5 + | +LL | &mut *(**s).0 //[nll]~ ERROR `*s.0` does not live long enough [E0597] + | ^^^^^^^^^^^^^ borrowed value does not live long enough +... +LL | } + | - `*s.0` dropped here while still borrowed + | +note: borrowed value must be valid for the lifetime 'a as defined on the function body at 82:26... + --> $DIR/issue-45696-scribble-on-boxed-borrow.rs:82:26 + | +LL | fn boxed_boxed_scribbled<'a>(s: Box>>) -> &'a mut u32 { + | ^^ + +error: aborting due to 3 previous errors + +For more information about this error, try `rustc --explain E0597`. diff --git a/src/test/ui/issue-45696-scribble-on-boxed-borrow.rs b/src/test/ui/issue-45696-scribble-on-boxed-borrow.rs new file mode 100644 index 0000000000000..5a4874249e2f4 --- /dev/null +++ b/src/test/ui/issue-45696-scribble-on-boxed-borrow.rs @@ -0,0 +1,110 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// rust-lang/rust#45696: This test is checking that we *cannot* return +// mutable borrows that would be scribbled over by destructors before +// the return occurs. +// +// We will explicitly test AST-borrowck, NLL, and migration modes; +// thus we will also skip the automated compare-mode=nll. + +// revisions: ast nll migrate +// ignore-compare-mode-nll + +// This test is going to pass in the ast and migrate revisions, +// because the AST-borrowck accepted this code in the past (see notes +// below). So we use `#[rustc_error]` to keep the outcome as an error +// in all scenarios, and rely on the stderr files to show what the +// actual behavior is. (See rust-lang/rust#49855.) +#![feature(rustc_attrs)] + +#![cfg_attr(nll, feature(nll))] +//[migrate]compile-flags: -Z borrowck=migrate -Z two-phase-borrows + +struct Scribble<'a>(&'a mut u32); + +impl<'a> Drop for Scribble<'a> { fn drop(&mut self) { *self.0 = 42; } } + +// this is okay, in both AST-borrowck and NLL: The `Scribble` here *has* +// to strictly outlive `'a` +fn borrowed_scribble<'a>(s: &'a mut Scribble) -> &'a mut u32 { + &mut *s.0 +} + +// this, by analogy to previous case, is also okay. +fn boxed_borrowed_scribble<'a>(s: Box<&'a mut Scribble>) -> &'a mut u32 { + &mut *(*s).0 +} + +// this, by analogy to previous case, is also okay. +fn boxed_boxed_borrowed_scribble<'a>(s: Box>) -> &'a mut u32 { + &mut *(**s).0 +} + +// this is not okay: in between the time that we take the mutable +// borrow and the caller receives it as a return value, the drop of +// `s` will scribble on it, violating our aliasing guarantees. +// +// * (Maybe in the future the two-phase borrows system will be +// extended to support this case. But for now, it is an error in +// NLL, even with two-phase borrows.) +// +// In any case, the AST-borrowck was not smart enough to know that +// this should be an error. (Which is perhaps the essence of why +// rust-lang/rust#45696 arose in the first place.) +fn scribbled<'a>(s: Scribble<'a>) -> &'a mut u32 { + &mut *s.0 //[nll]~ ERROR `*s.0` does not live long enough [E0597] + //[migrate]~^ WARNING `*s.0` does not live long enough [E0597] + //[migrate]~| WARNING This error has been downgraded to a warning for backwards compatibility +} + +// This, by analogy to previous case, is *also* not okay. +// +// (But again, AST-borrowck was not smart enogh to know that this +// should be an error.) +fn boxed_scribbled<'a>(s: Box>) -> &'a mut u32 { + &mut *(*s).0 //[nll]~ ERROR `*s.0` does not live long enough [E0597] + //[migrate]~^ WARNING `*s.0` does not live long enough [E0597] + //[migrate]~| WARNING This error has been downgraded to a warning for backwards compatibility +} + +// This, by analogy to previous case, is *also* not okay. +// +// (But again, AST-borrowck was not smart enogh to know that this +// should be an error.) +fn boxed_boxed_scribbled<'a>(s: Box>>) -> &'a mut u32 { + &mut *(**s).0 //[nll]~ ERROR `*s.0` does not live long enough [E0597] + //[migrate]~^ WARNING `*s.0` does not live long enough [E0597] + //[migrate]~| WARNING This error has been downgraded to a warning for backwards compatibility +} + +#[rustc_error] +fn main() { //[ast]~ ERROR compilation successful + //[migrate]~^ ERROR compilation successful + let mut x = 1; + { + let mut long_lived = Scribble(&mut x); + *borrowed_scribble(&mut long_lived) += 10; + // (Scribble dtor runs here, after `&mut`-borrow above ends) + } + { + let mut long_lived = Scribble(&mut x); + *boxed_borrowed_scribble(Box::new(&mut long_lived)) += 10; + // (Scribble dtor runs here, after `&mut`-borrow above ends) + } + { + let mut long_lived = Scribble(&mut x); + *boxed_boxed_borrowed_scribble(Box::new(Box::new(&mut long_lived))) += 10; + // (Scribble dtor runs here, after `&mut`-borrow above ends) + } + *scribbled(Scribble(&mut x)) += 10; + *boxed_scribbled(Box::new(Scribble(&mut x))) += 10; + *boxed_boxed_scribbled(Box::new(Box::new(Scribble(&mut x)))) += 10; +} From 469d6a819d3d7348fdd54d8c7ec1e694f2f43051 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Mon, 30 Jul 2018 15:38:18 +0200 Subject: [PATCH 4/6] Test for (previously uncaught) infinite loop identified by matthewjasper. --- .../ui/issue-45696-no-variant-box-recur.rs | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 src/test/ui/issue-45696-no-variant-box-recur.rs diff --git a/src/test/ui/issue-45696-no-variant-box-recur.rs b/src/test/ui/issue-45696-no-variant-box-recur.rs new file mode 100644 index 0000000000000..740e99c82a556 --- /dev/null +++ b/src/test/ui/issue-45696-no-variant-box-recur.rs @@ -0,0 +1,66 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// rust-lang/rust#45696: This test checks the compiler won't infinite +// loop when you declare a variable of type `struct A(Box, ...);` +// (which is impossible to construct but *is* possible to declare; see +// also issues #4287, #44933, and #52852). +// +// We will explicitly test AST-borrowck, NLL, and migration modes; +// thus we will also skip the automated compare-mode=nll. + +// revisions: ast nll migrate +// ignore-compare-mode-nll + +#![cfg_attr(nll, feature(nll))] +//[migrate]compile-flags: -Z borrowck=migrate -Z two-phase-borrows + +// run-pass + +// This test has structs and functions that are by definiton unusable +// all over the place, so just go ahead and allow dead_code +#![allow(dead_code)] + +// direct regular recursion with indirect ownership via box +struct C { field: Box } + +// direct non-regular recursion with indirect ownership via box +struct D { field: Box<(D, D)> } + +// indirect regular recursion with indirect ownership via box. +struct E { field: F } +struct F { field: Box } + +// indirect non-regular recursion with indirect ownership via box. +struct G { field: (F, F) } +struct H { field: Box } + +// These enums are cases that are not currently hit by the +// `visit_terminator_drop` recursion down a type's structural +// definition. +// +// But it seems prudent to include them in this test as variants on +// the above, in that they are similarly non-constructable data types +// with destructors that would diverge. +enum I { One(Box) } +enum J { One(Box), Two(Box) } + +fn impossible_to_call_c(_c: C) { } +fn impossible_to_call_d(_d: D) { } +fn impossible_to_call_e(_e: E) { } +fn impossible_to_call_f(_f: F) { } +fn impossible_to_call_g(_g: G) { } +fn impossible_to_call_h(_h: H) { } +fn impossible_to_call_i(_i: I) { } +fn impossible_to_call_j(_j: J) { } + +fn main() { + +} From a1b8a93f81f4da89eaaa75f27df395f46c17c470 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Mon, 30 Jul 2018 16:17:04 +0200 Subject: [PATCH 5/6] Expand long-live-borrows-in-boxes test to include simplier illustrative cases. After talking about the PR with eddyb, I decided it was best to try to have some test cases that simplify the problem down to its core, so that people trying to understand what the issue is here will see those core examples first. --- .../issue-45696-long-live-borrows-in-boxes.rs | 40 ++++++++++++++++--- 1 file changed, 35 insertions(+), 5 deletions(-) diff --git a/src/test/ui/issue-45696-long-live-borrows-in-boxes.rs b/src/test/ui/issue-45696-long-live-borrows-in-boxes.rs index e5326bb315ea3..881f37c2e0b0e 100644 --- a/src/test/ui/issue-45696-long-live-borrows-in-boxes.rs +++ b/src/test/ui/issue-45696-long-live-borrows-in-boxes.rs @@ -22,24 +22,54 @@ // run-pass -type Boxed<'a, 'b> = Box<(&'a mut u32, &'b mut u32)>; +// This function shows quite directly what is going on: We have a +// reborrow of contents within the box. +fn return_borrow_from_dropped_box_1(x: Box<&mut u32>) -> &mut u32 { &mut **x } -fn return_borrow_from_dropped_box<'a>(x: Boxed<'a, '_>) -> &'a mut u32 { +// This function is the way you'll probably see this in practice (the +// reborrow is now implicit). +fn return_borrow_from_dropped_box_2(x: Box<&mut u32>) -> &mut u32 { *x } + +// For the remaining tests we just add some fields or other +// indirection to ensure that the compiler isn't just special-casing +// the above `Box<&mut T>` as the only type that would work. + +// Here we add a tuple of indirection between the box and the +// reference. +type BoxedTup<'a, 'b> = Box<(&'a mut u32, &'b mut u32)>; + +fn return_borrow_of_field_from_dropped_box_1<'a>(x: BoxedTup<'a, '_>) -> &'a mut u32 { &mut *x.0 } -fn return_borrow_from_dropped_tupled_box<'a>(x: (Boxed<'a, '_>, &mut u32)) -> &'a mut u32 { +fn return_borrow_of_field_from_dropped_box_2<'a>(x: BoxedTup<'a, '_>) -> &'a mut u32 { + x.0 +} + +fn return_borrow_from_dropped_tupled_box_1<'a>(x: (BoxedTup<'a, '_>, &mut u32)) -> &'a mut u32 { &mut *(x.0).0 } +fn return_borrow_from_dropped_tupled_box_2<'a>(x: (BoxedTup<'a, '_>, &mut u32)) -> &'a mut u32 { + (x.0).0 +} + fn basic_tests() { let mut x = 2; let mut y = 3; let mut z = 4; - *return_borrow_from_dropped_box(Box::new((&mut x, &mut y))) += 10; + *return_borrow_from_dropped_box_1(Box::new(&mut x)) += 10; assert_eq!((x, y, z), (12, 3, 4)); - *return_borrow_from_dropped_tupled_box((Box::new((&mut x, &mut y)), &mut z)) += 10; + *return_borrow_from_dropped_box_2(Box::new(&mut x)) += 10; assert_eq!((x, y, z), (22, 3, 4)); + *return_borrow_of_field_from_dropped_box_1(Box::new((&mut x, &mut y))) += 10; + assert_eq!((x, y, z), (32, 3, 4)); + *return_borrow_of_field_from_dropped_box_2(Box::new((&mut x, &mut y))) += 10; + assert_eq!((x, y, z), (42, 3, 4)); + *return_borrow_from_dropped_tupled_box_1((Box::new((&mut x, &mut y)), &mut z)) += 10; + assert_eq!((x, y, z), (52, 3, 4)); + *return_borrow_from_dropped_tupled_box_2((Box::new((&mut x, &mut y)), &mut z)) += 10; + assert_eq!((x, y, z), (62, 3, 4)); } // These scribbling tests have been transcribed from From c02c00b8455d6ec6eff50ae94bebb4a424c95e02 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 31 Jul 2018 01:10:06 +0200 Subject: [PATCH 6/6] Fix bug in test pointed out during review. --- src/test/ui/issue-45696-no-variant-box-recur.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/ui/issue-45696-no-variant-box-recur.rs b/src/test/ui/issue-45696-no-variant-box-recur.rs index 740e99c82a556..da42e171fcc50 100644 --- a/src/test/ui/issue-45696-no-variant-box-recur.rs +++ b/src/test/ui/issue-45696-no-variant-box-recur.rs @@ -39,8 +39,8 @@ struct E { field: F } struct F { field: Box } // indirect non-regular recursion with indirect ownership via box. -struct G { field: (F, F) } -struct H { field: Box } +struct G { field: (H, H) } +struct H { field: Box } // These enums are cases that are not currently hit by the // `visit_terminator_drop` recursion down a type's structural