MIR-borrowck: behavior around Box<T> is different from AST borrowck #45696

arielb1 · 2017-11-01T21:02:16Z

MIR borrowck treats Box<T> differently from AST borrowck (not that AST borrowck is terribly consistent there), for example, in the (simplified) test for #17263:

struct Foo { a: isize, b: isize }

fn main() {
    let mut x: Box<_> = Box::new(Foo { a: 1, b: 2 });
    let (_a, _b) = (&mut x.a, &mut x.b);
    //~^ ERROR cannot borrow `x` (via `x.b`) as mutable more than once at a time
    //~| NOTE first mutable borrow occurs here (via `x.a`)
    //~| NOTE second mutable borrow occurs here (via `x.b`)

    let mut foo: Box<_> = Box::new(Foo { a: 1, b: 2 });
    let (_c, _d) = (&mut foo.a, &foo.b);
    //~^ ERROR cannot borrow `foo` (via `foo.b`) as immutable
    //~| NOTE mutable borrow occurs here (via `foo.a`)
    //~| NOTE immutable borrow occurs here (via `foo.b`)
}
//~^ NOTE first borrow ends here
//~^^ NOTE mutable borrow ends here

AST borrowck reports an error on both lines, but MIR borrowck lets the code pass:

error[E0499]: cannot borrow `x` (via `x.b`) as mutable more than once at a time (Ast)
  --> ../src/test/compile-fail/issue-17263.rs:15:36
   |
15 |     let (_a, _b) = (&mut x.a, &mut x.b);
   |                          ---       ^^^ second mutable borrow occurs here (via `x.b`)
   |                          |
   |                          first mutable borrow occurs here (via `x.a`)
...
25 | }
   | - first borrow ends here

error[E0502]: cannot borrow `foo` (via `foo.b`) as immutable because `foo` is also borrowed as mutable (via `foo.a`) (Ast)
  --> ../src/test/compile-fail/issue-17263.rs:21:34
   |
21 |     let (_c, _d) = (&mut foo.a, &foo.b);
   |                          -----   ^^^^^ immutable borrow occurs here (via `foo.b`)
   |                          |
   |                          mutable borrow occurs here (via `foo.a`)
...
25 | }
   | - mutable borrow ends here

error: aborting due to 2 previous errors

This should be investigated and solved

The text was updated successfully, but these errors were encountered:

nikomatsakis · 2017-11-10T16:14:02Z

I think we have to decide how we are going to handle Box<T> in MIR borrowck. I would prefer not to introduce hacks into borrowck the way we did in the AST -- if we're going to impose some limitations on Box<T>, I would prefer to do it in the MIR lowering phase (e.g., by lowering let x = &mut box.foo more faithfully into something that uses the DerefMut impl).

What do you think @arielb1 ? I imagine this will cause more backwards incompatibility on the various corner cases that AST borrow check gets wrong, is one concern.

nikomatsakis · 2017-11-10T16:14:43Z

An alternative of course is to restore the "full knowledge" to Box<T> -- it's already somewhat special -- with the assumption that we will someday allow user-written types to "opt-in" to such semantics.

arielb1 · 2017-11-16T12:43:25Z

@nikomatsakis

I would prefer to have "full DerefPure" semantics for Box. We already have fairly builtin move semantics, and the AST borrowck semantics are terrible in a few places.

nikomatsakis · 2017-11-16T14:33:50Z

@arielb1 I agree. It seems like then there is not a lot of work to do here, right? Or are there places where MIR borrowck enforces rules that are too strict?

arielb1 · 2017-11-16T15:01:33Z

@nikomatsakis

I think MIR borrowck currently has an even stupider version of the Box borrow rules, which needs to be removed.

nikomatsakis · 2017-11-17T21:18:18Z

I'm going to try writing out some tests here in the comments and their current behavior.

Access field 1 while field 0 is borrowed

fn main() {
    let mut data = Box::new((1, 2));
    let p = &data.0;
    data.1 += 1;
}

This gives an Ast error, but no Mir error, as expected.

Drop box while field 0 is borrowed

fn main() {
    let mut data = Box::new((1, 2));
    let p = &data.0;
    drop(data);
    println!("{}", p);
}

This gives an error in both AST / MIR, as expected.

Access field 1 when field 0 is borrowed

fn main() {
    let data = Box::new((format!("Hello"), format!("World")));
    drop(data.0);
    println!("{}", data.1);
}

This gives an error only in AST, as expected.

Return reborrowed mutable ref in field 0

fn return_borrowed<'a>(x: Box<(&'a mut u32, &mut u32)>) -> &'a mut u32 {
    &mut *x.0
}

fn main() {
}

This gives an error only in MIR. Seems wrong.

(Ironically, both AST and MIR seem wrong here. That is, to be consistent, AST should error, but doesn't. And MIR should not, but does.)

nikomatsakis · 2017-11-17T21:25:11Z

The last error is of course nikomatsakis/nll-rfc#40. I still like the dangly paths answer there, but I do appreciate and understand @arielb1's trepidation. Perhaps we just do some kind of limited variant hard-coded to Box for now. =)

arielb1 · 2017-11-18T19:11:45Z

@nikomatsakis

The last error is of course nikomatsakis/nll-rfc#40

It is. However, luckily for us, Box is DerefPure, which means that we "known" its destructor can't access the interior (after all, the interior could have been moved), so this looks like a clearer place to special-case.

Fixes rust-lang#44831. Fixes rust-lang#44834. Fixes rust-lang#45537. Fixes rust-lang#45696 (by implementing DerefPure semantics, which is what we want going forward).

@nikomatsakis

MIR borrowck: implement union-and-array-compatible semantics Fixes #44831. Fixes #44834. Fixes #45537. Fixes #45696 (by implementing DerefPure semantics, which is what we want going forward). r? @nikomatsakis

Mark-Simulacrum · 2018-07-15T16:56:24Z

Reopening; it seems this has regressed, the following code snippet doesn't compile with MIR but does with AST borrowck; I think it's the same issue as described here (i.e., different semantics for Box).

#![feature(nll)]

fn foo(x: Box<&mut i32>) -> &mut i32 {
    *x
}

fn main() {}

arielb1 · 2018-07-15T17:59:36Z

@Mark-Simulacrum

This is just nikomatsakis/nll-rfc#40, or on the AST side #31567.

pnkfelix · 2018-07-25T12:19:57Z

either way we really need some issue open on the rust-lang/rust repo to track this, and unless I'm missing something, currently both #31567 and #45696 are closed.

So I'm reopening this (which I think @Mark-Simulacrum meant to do but forgot?)

pnkfelix · 2018-07-26T11:28:35Z

I'm going to try to look at this today

pnkfelix · 2018-07-26T20:31:02Z

I think I have a specialized fix specific to Box that I'm reasonably confident in.

It is this commit: pnkfelix@596f363

I hope to put up a PR tomorrow.

pnkfelix · 2018-07-27T11:49:18Z

Okay pnkfelix/rust@596f363 isn't quite right. Or at least, I want to make sure we handle the following three cases correctly: https://play.rust-lang.org/?gist=16b3dc1280f5becafae18e94790c6fc4&version=nightly&mode=debug&edition=2015

pnkfelix · 2018-07-27T13:27:28Z

Well its great that I found that bug; it forced me to reconsider my solution in a number of ways and I found a different technique that I like a lot more than the commit I made yesterday.

New commit: pnkfelix/rust@bcb19fc

pnkfelix · 2018-07-27T14:48:26Z

but oops I really need to get better at testing stuff first. Sigh.

pnkfelix · 2018-07-27T16:04:09Z

(okay now PR #52782 is a change I'm pretty confident in. And as a bonus it also passes the test suite.)

@matthewjasper

…or-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 rust-lang#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 rust-lang#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 rust-lang#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 rust-lang#45696, specifically in [the last example of this comment](rust-lang#45696 (comment)), which I have adapted into the `fn foo` shown above. We did close issue rust-lang#45696 back in December of 2017, but AFAICT that example was not fixed by PR rust-lang#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 rust-lang#45696.

@matthewjasper

…or-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 rust-lang#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 rust-lang#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 rust-lang#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 rust-lang#45696, specifically in [the last example of this comment](rust-lang#45696 (comment)), which I have adapted into the `fn foo` shown above. We did close issue rust-lang#45696 back in December of 2017, but AFAICT that example was not fixed by PR rust-lang#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 rust-lang#45696.

@matthewjasper

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

pnkfelix · 2018-11-06T14:03:10Z

see also some follow-up discussion here #43234 (comment)

arielb1 added A-borrow-checker Area: The borrow checker WG-compiler-nll labels Nov 1, 2017

arielb1 added this to the NLL prototype milestone Nov 16, 2017

arielb1 mentioned this issue Nov 25, 2017

MIR borrowck: implement union-and-array-compatible semantics #46268

Merged

bors closed this as completed in #46268 Dec 6, 2017

Mark-Simulacrum modified the milestones: NLL prototype, Rust 2018 Preview 2 Jul 15, 2018

pnkfelix reopened this Jul 25, 2018

nikomatsakis added the A-NLL Area: Non-lexical lifetimes (NLL) label Jul 25, 2018

pnkfelix added the NLL-complete Working towards the "valid code works" goal label Jul 25, 2018

pnkfelix mentioned this issue Jul 25, 2018

NLL: need a (new) round of review of deltas between .stderr and .nll.stderr output #52663

Closed

pnkfelix self-assigned this Jul 26, 2018

pnkfelix mentioned this issue Jul 27, 2018

[NLL] Dangly paths for box #52782

Merged

Mark-Simulacrum modified the milestones: Rust 2018 Preview 2, Rust 2018 RC Jul 31, 2018

bors closed this as completed in #52782 Aug 2, 2018

Centril added a commit to Centril/rust that referenced this issue Sep 25, 2019

issue-rust-lang#45696-scribble...: remove outdated comment.

4503ad4

Centril added a commit to Centril/rust that referenced this issue Sep 26, 2019

issue-rust-lang#45696-scribble...: remove outdated comment.

ce8c5d2

Centril added a commit to Centril/rust that referenced this issue Sep 26, 2019

issue-rust-lang#45696: remove ignore-compare-mode-nll

5a0e461

c410-f3r mentioned this issue Jan 25, 2021

Move some tests to more reasonable directories - 3 #81387

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MIR-borrowck: behavior around Box<T> is different from AST borrowck #45696

MIR-borrowck: behavior around Box<T> is different from AST borrowck #45696

arielb1 commented Nov 1, 2017

nikomatsakis commented Nov 10, 2017

nikomatsakis commented Nov 10, 2017

arielb1 commented Nov 16, 2017

nikomatsakis commented Nov 16, 2017

arielb1 commented Nov 16, 2017

nikomatsakis commented Nov 17, 2017

nikomatsakis commented Nov 17, 2017

arielb1 commented Nov 18, 2017

Mark-Simulacrum commented Jul 15, 2018

arielb1 commented Jul 15, 2018

pnkfelix commented Jul 25, 2018

pnkfelix commented Jul 26, 2018

pnkfelix commented Jul 26, 2018

pnkfelix commented Jul 27, 2018

pnkfelix commented Jul 27, 2018

pnkfelix commented Jul 27, 2018

pnkfelix commented Jul 27, 2018

pnkfelix commented Nov 6, 2018

MIR-borrowck: behavior around Box<T> is different from AST borrowck #45696

MIR-borrowck: behavior around Box<T> is different from AST borrowck #45696

Comments

arielb1 commented Nov 1, 2017

nikomatsakis commented Nov 10, 2017

nikomatsakis commented Nov 10, 2017

arielb1 commented Nov 16, 2017

nikomatsakis commented Nov 16, 2017

arielb1 commented Nov 16, 2017

nikomatsakis commented Nov 17, 2017

Access field 1 while field 0 is borrowed

Drop box while field 0 is borrowed

Access field 1 when field 0 is borrowed

Return reborrowed mutable ref in field 0

nikomatsakis commented Nov 17, 2017

arielb1 commented Nov 18, 2017

Mark-Simulacrum commented Jul 15, 2018

arielb1 commented Jul 15, 2018

pnkfelix commented Jul 25, 2018

pnkfelix commented Jul 26, 2018

pnkfelix commented Jul 26, 2018

pnkfelix commented Jul 27, 2018

pnkfelix commented Jul 27, 2018

pnkfelix commented Jul 27, 2018

pnkfelix commented Jul 27, 2018

pnkfelix commented Nov 6, 2018