-
Notifications
You must be signed in to change notification settings - Fork 12.2k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Auto merge of #52782 - pnkfelix:issue-45696-dangly-paths-for-box, r=e…
…ddyb
[NLL] Dangly paths for box
Special-case `Box` in `rustc_mir::borrow_check`.
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.
----
There are three main things going on here:
1. The first main thing, this PR is fixing a bug in NLL where `rustc` previously would issue a diagnostic error in a case like this:
```rust
fn foo(x: Box<&mut i32>) -> &mut i32 { &mut **x }
```
such code was accepted by the AST-borrowck in the past, but NLL was rejecting it with the following message ([playground](https://play.rust-lang.org/?gist=13c5560f73bfb16d6dab3ceaad44c0f8&version=nightly&mode=release&edition=2015))
```
error[E0597]: `**x` does not live long enough
--> src/main.rs:3:40
|
3 | fn foo(x: Box<&mut i32>) -> &mut i32 { &mut **x }
| ^^^^^^^^ - `**x` dropped here while still borrowed
| |
| borrowed value does not live long enough
|
note: borrowed value must be valid for the anonymous lifetime #1 defined on the function body at 3:1...
--> src/main.rs:3:1
|
3 | fn foo(x: Box<&mut i32>) -> &mut i32 { &mut **x }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
error: aborting due to previous error
```
2. The second main thing: The reason such code was previously rejected was because NLL (MIR-borrowck) incorporates a fix for issue #31567, where it models a destructor's execution as potentially accessing any borrows held by the thing being destructed. The tests with `Scribble` model this, showing that the compiler now catches such unsoundness.
However, that fix for issue #31567 is too strong, in that NLL (MIR-borrowck) includes `Box` as one of the types with a destructor that potentially accesses any borrows held by the box. This thus was the cause of the main remaining discrepancy between AST-borrowck and MIR-borrowck, as documented in issue #45696, specifically in [the last example of this comment](#45696 (comment)), which I have adapted into the `fn foo` shown above.
We did close issue #45696 back in December of 2017, but AFAICT that example was not fixed by PR #46268. (And we did not include a test, etc etc.)
This PR fixes that case, by trying to model the so-called `DerefPure` semantics of `Box<T>` when we traverse the type of the input to `visit_terminator_drop`.
3. The third main thing is that during a review of the first draft of this PR, @matthewjasper pointed out that the new traversal of `Box<T>` could cause the compiler to infinite loop. I have adjusted the PR to avoid this (by tracking what types we have previously seen), and added a much needed test of this somewhat odd scenario. (Its an odd scenario because the particular case only arises for things like `struct A(Box<A>);`, something which cannot be constructed in practice.)
Fix #45696.- Loading branch information
Showing
9 changed files
with
586 additions
and
9 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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(); | ||
| } |
Oops, something went wrong.