Auto merge of #52782 - pnkfelix:issue-45696-dangly-paths-for-box, r=eddyb

[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.

Author: bors

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<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.
+            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,
+                    );
                 }
             }
         }

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

src/test/ui/issue-45696-long-live-borrows-in-boxes.rs

@@ -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();
+}