Skip to content

[NLL] Dangly paths for box #52782

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.

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

Merged
merged 6 commits into from
Aug 2, 2018
Merged
Changes from all commits
Commits
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
151 changes: 146 additions & 5 deletions src/librustc_mir/borrow_check/mod.rs
Original file line number Diff line number Diff line change
@@ -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`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @nikomatsakis this is starting to quickly become the rustc team's favorite hack!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, in the commit message I even wrote:

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/`.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#[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<C>, ... }`
// * `struct D { field: Box<(D, D)>, ... }`:
// *accepted*, though impossible to construct
//
// Here is *NOT* an example:
// * `struct Z { field: Option<Box<Z>>, ... }`:
// 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, can we not fall-back-to-error instead of fall-back-to-accept? Seems like a disaster waiting to happen.^^

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot fallback to error, at least not to hard error. That could break hypothetical code.

We could issue a future compatibility warning.

Or we could try to emit drop code that error dynamically ... maybe that is what you are asking for?

(When I wrote "skip them during drop", I meant during this static analysis. I believe the actual dynamic behavior here will be to ... infinite loop ... i think... )

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This is the test case showing the kind of case that concerns me. It is currently marked as run-pass; we could make a variant of it that actually conjures up one of this infinitely regressed things, but I'm pretty sure that the unsafe code guidelines group would tag such a test as having undefined behavior, no?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure that the unsafe code guidelines group would tag such a test as having undefined behavior,

Yeah, I agree.

When I wrote "skip them during drop", I meant during this static analysis. I believe the actual dynamic behavior here will be to ... infinite loop ... i think...

Probably it'll fill up the stack. ;)
What does it mean for the static analysis to skip this? Does it not consider this a use, or so? That seems problematic.^^

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It considers the first one it sees a use, but not the second one encountered via the recursive descent.

But to even get to that code during dynamic execution you’d have to already built the invalid thing and be witnessing UB.

Maybe I should make a more complete illustrative test case for us to discuss, with the aforementioned construction...?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It considers the first one it sees a use, but not the second one encountered via the recursive descent.

I see. Yes, without knowing any of the details, this does make some sense.

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,20 +976,84 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
.enumerate()
.for_each(|field| drop_field(self, field));
}

// #45696: special-case Box<T> 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<Box<ScribbleWhenDropped<&mut T>>>`, 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),
(Deep, Write(WriteKind::StorageDeadOrDrop)),
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,
);
}
}
}
2 changes: 0 additions & 2 deletions src/test/ui/generator/dropck.nll.stderr
Original file line number Diff line number Diff line change
@@ -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
133 changes: 133 additions & 0 deletions src/test/ui/issue-45696-long-live-borrows-in-boxes.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
// 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 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, 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

// 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 }

// 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_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_1(Box::new(&mut x)) += 10;
assert_eq!((x, y, z), (12, 3, 4));
*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
// 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<Box<&'a mut Scribble>>) -> &'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();
}
66 changes: 66 additions & 0 deletions src/test/ui/issue-45696-no-variant-box-recur.rs
Original file line number Diff line number Diff line change
@@ -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 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, 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<A>, ...);`
// (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<C> }

// 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<E> }

// indirect non-regular recursion with indirect ownership via box.
struct G { field: (H, H) }
struct H { field: Box<G> }

// 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<I>) }
enum J { One(Box<J>), Two(Box<J>) }

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() {

}
14 changes: 14 additions & 0 deletions src/test/ui/issue-45696-scribble-on-boxed-borrow.ast.stderr
Original file line number Diff line number Diff line change
@@ -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

69 changes: 69 additions & 0 deletions src/test/ui/issue-45696-scribble-on-boxed-borrow.migrate.stderr
Original file line number Diff line number Diff line change
@@ -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<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: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<Box<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.

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`.
48 changes: 48 additions & 0 deletions src/test/ui/issue-45696-scribble-on-boxed-borrow.nll.stderr
Original file line number Diff line number Diff line change
@@ -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<Scribble<'a>>) -> &'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<Box<Scribble<'a>>>) -> &'a mut u32 {
| ^^

error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0597`.
110 changes: 110 additions & 0 deletions src/test/ui/issue-45696-scribble-on-boxed-borrow.rs
Original file line number Diff line number Diff line change
@@ -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 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, 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<Box<&'a mut Scribble>>) -> &'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<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_boxed_scribbled<'a>(s: Box<Box<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
}

#[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;
}
2 changes: 0 additions & 2 deletions src/test/ui/span/dropck-object-cycle.nll.stderr
Original file line number Diff line number Diff line change
@@ -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