Closed
Description
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
Activity
nikomatsakis commentedon Nov 10, 2017
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 onBox<T>
, I would prefer to do it in the MIR lowering phase (e.g., by loweringlet x = &mut box.foo
more faithfully into something that uses theDerefMut
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 commentedon Nov 10, 2017
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 commentedon Nov 16, 2017
@nikomatsakis
I would prefer to have "full
DerefPure
" semantics forBox
. We already have fairly builtin move semantics, and the AST borrowck semantics are terrible in a few places.nikomatsakis commentedon Nov 16, 2017
@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 commentedon Nov 16, 2017
@nikomatsakis
I think MIR borrowck currently has an even stupider version of the Box borrow rules, which needs to be removed.
nikomatsakis commentedon Nov 17, 2017
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
This gives an Ast error, but no Mir error, as expected.
Drop box while field 0 is borrowed
This gives an error in both AST / MIR, as expected.
Access field 1 when field 0 is borrowed
This gives an error only in AST, as expected.
Return reborrowed mutable ref in field 0
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 commentedon Nov 17, 2017
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 commentedon Nov 18, 2017
@nikomatsakis
It is. However, luckily for us,
Box
isDerefPure
, 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.39 remaining items