-
Notifications
You must be signed in to change notification settings - Fork 13k
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
MIR borrowck: implement union-and-array-compatible semantics #46268
Conversation
☔ The latest upstream changes (presumably #46106) made this pull request unmergeable. Please resolve the merge conflicts. |
44950cd
to
8e9b536
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me, I left some requests for comments, but this seems quite elegant. =)
src/librustc_mir/borrow_check.rs
Outdated
debug!("borrow_conflicts_with_lvalue({:?},{:?},{:?})", borrow, lvalue, access); | ||
|
||
// Return the elements pf | ||
fn lvalue_elements<'a, 'tcx>(lvalue: &'a Lvalue<'tcx>) -> Vec<&'a Lvalue<'tcx>> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this just self.prefixes(lvalue, PrefixSet::All).collect()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I guess it's reversed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure enough, I'll switch to use reversed prefixes instead.
src/librustc_mir/borrow_check.rs
Outdated
@@ -1115,13 +1116,238 @@ enum NoMovePathFound { | |||
ReachedStatic, | |||
} | |||
|
|||
enum Overlap { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to have a comment here explaining the semantic meaning of these terms. I guess they are hard to explain without reference to lvalue_element_conflict
and the overall algorithm here. Maybe just write up a good explanation elsewhere and refer to it from here.
In any case, my understanding is that this type represents the "effect" on disjointness from a single element, under assumption that all preceding elements were equal.
- Disjoint. No data in common.
- AllOrNothing. Overlaps unless subsequent yield disjoint.
- Arbitrary. May overlap, regardless of subsequent elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well for me it represented the "current" effect for a prefix of the lvalue (i.e. it's a monoid, and lvalue_conflicts_with_borrow
is its product over all pairs of path elements).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. These seem like similar ways of saying the same thing. In any case, the point is that this represents the "contribution" of a single part of a path.
src/librustc_mir/borrow_check.rs
Outdated
impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { | ||
fn lvalue_element_conflict(&self, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment please. =) My attempt to explain based on my understanding:
Given that all base paths (if any) of `elem1` and `elem2` are disjoint,
determines whether `elem1` and `elem2` represent overlapping paths.
Some examples:
- `a` vs `a`
- Yes, overlapping (`AllOrNothing`).
- `a` vs `b`
- No, distinct locals (`Disjoint`).
- `a.b` vs `a.c`
- `Disjoint` if `b` and `c` are distinct fields of the same struct
- `Arbitrary` if `b` and `c` are fields in the same union.
- `a.b` vs `x.y`
- This represents an illegal call, since the base path `a` is disjoint from the base path `x`.
src/librustc_mir/borrow_check.rs
Outdated
// (and therefore, to be borrowed) at the same time. | ||
// | ||
// Note that this is different from unions, but both | ||
// behaviors are grandfathered. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This "grandfathered" reference confused me for a bit. I guess you mean that "Note that this behavior is different from unions, where distinct 'variants' (fields) are considered to overlap.", basically, right? And the grandfathered just means "this is how Rust behaves today".
In any case, we could alter unions to behave similarly to enums if we wanted, I imagine, but not the opposite, right? Or is there a reason we can't? (Not that I think we necessarily should.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case, we could alter unions to behave similarly to enums if we wanted, I imagine, but not the opposite, right? Or is there a reason we can't?
I think we could make unions behave similarly to enums, it would just be "super-unsafe".
src/librustc_mir/borrow_check.rs
Outdated
.map(Some).chain(iter::repeat(None)); | ||
let lvalue_components = lvalue_components.into_iter() | ||
.map(Some).chain(iter::repeat(None)); | ||
for (borrow_c, lvalue_c) in borrow_components.zip(lvalue_components) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be a good place to include a comment walk through some examples. Let me take a stab at it.
Given a borrow path and an access path, this loop walks forward through the paths in lockstep, trying to find a point where they become disjoint. The invariant is that all preceding components have been found to be equal. At each point, we use lvalue_element_conflict
to test if that element implies disjointness (or overlap). Note that we include a terminating None
value for each path so that we know when one path ends.
src/librustc_mir/borrow_check.rs
Outdated
debug!("borrow_conflict_with_lvalue: full borrow, CONFLICT"); | ||
return true; | ||
} | ||
(Some(borrow_c), None) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example comment:
The borrow path "extends" the access path. For example, the borrow path might be a.b.c
with an access path of a.b
. The borrow may or may not be considered to cover the access here, depending on the precise kind of access. Note that in some cases we will loop around here, so we might hit this case more than once if we had e.g. a borrow path of *a.b.c.d
and an access path of a.b
.
src/librustc_mir/borrow_check.rs
Outdated
// overlap any existing data there. Furthermore, | ||
// they cannot actually be a prefix of any | ||
// borrowed lvalue (at least in MIR as it is | ||
// currently.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good comment. Might be good to add an example:
So, for example, if we have borrowed a.b.as<Some>.0
, and we access the discriminant of a.b
, that is not considered to overlap.
src/librustc_mir/borrow_check.rs
Outdated
return false; | ||
} | ||
|
||
(ProjectionElem::Deref, _, Shallow(None)) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment:
Borrowed *a.b
but accessed a.b
or some prefix thereof. This is ok for shallow accesses, since they don't deref pointers.
src/librustc_mir/borrow_check.rs
Outdated
Overlap::Arbitrary => { | ||
// the borrow partially overlaps. Give up - we | ||
// could try to be smarter here (e.g. counting | ||
// dereferences) but I think we should not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment: This primarily occurs with unions. For example, a borrow of some_union.x.y
and some_union.a
would trigger this case (when accessing the x
and a
fields).
src/librustc_mir/borrow_check.rs
Outdated
// | ||
// I'm not sure why we are tracking these borrows - shared | ||
// references can *always* be aliased, which means the | ||
// permission check already account for this borrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we should probably not bother to track them.
80bba9a
to
f4a6729
Compare
☔ The latest upstream changes (presumably #46425) made this pull request unmergeable. Please resolve the merge conflicts. |
1e12245
to
1e6ddb9
Compare
☔ The latest upstream changes (presumably #46319) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me. I left some nits, but they are just nits.
src/librustc_mir/borrow_check.rs
Outdated
@@ -158,6 +166,9 @@ pub struct MirBorrowckCtxt<'cx, 'gcx: 'tcx, 'tcx: 'cx> { | |||
node_id: ast::NodeId, | |||
move_data: &'cx MoveData<'tcx>, | |||
param_env: ParamEnv<'gcx>, | |||
/// This keeps track of whether local variables are free-ed when the function | |||
/// exits even without a `StorageDead`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: It'd be nice to say something about why it might be true or false
src/librustc_mir/borrow_check.rs
Outdated
debug!("place_is_invalidated_at_exit({:?})", place); | ||
let root_place = self.prefixes(place, PrefixSet::All).last().unwrap(); | ||
|
||
// FIXME(nll-rfc#40): do more precise destructor tracking here. For now | ||
// we just know that all locals are dropped at function exit (otherwise | ||
// we'll have a memory leak) and assume that all statics have a destructor. | ||
let (might_be_alive, will_be_dropped) = match root_place { | ||
// | ||
// FIXME: allow thread-locals to borrow other thread locals?x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you say a bit more about this? It seems like you have disabled these checks in statics/constants, right? Is that what you meant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, never mind, that flag was for whether locals are considered dropped, right...
src/librustc_mir/borrow_check.rs
Outdated
/// the place. | ||
EqualOrDisjoint, | ||
/// The places are disjoint, so we know all extensions of them | ||
/// will also be disjoint. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
src/librustc_mir/borrow_check.rs
Outdated
impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { | ||
// Given that the bases of `elem1` and `elem2` are always either equal | ||
// or disjoint (and have the same type!), return the overlap situation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does "equal or disjoint" mean "not known to be disjoint", basically? I guess it is referencing the EqualOrDisjoint
variant? The phrasing as is sounds a bit like something that can't be false (i.e., sort of like "given that X is either true or false"), though I understand what you mean by it. Maybe just "given that the basis of elem1
and elem2
thus far overlap with Overlap::EqualOrDisjoint
" or something.
src/librustc_mir/borrow_check.rs
Outdated
(ProjectionElem::Subslice { .. }, ProjectionElem::Subslice { .. }) => { | ||
// Array indexes (`a[0]` vs. `a[i]`). These can either be disjoint | ||
// (if the indexes differ) or equal (if they are the same), so this | ||
// is the recursive case that gives "equal *or* disjoint" its meaning. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, nice comment here.
One question, but perhaps this is the FIXME. IIRC the intention with ConstantIndex
and friends was to be able to handle things like:
let mut x = [1, 2];
match foo {
[ref mut a, ref mut b] => { ... }
}
without an error. This current code doesn't handle that sort of thing yet, does it? (These will in MIR be sequentialized, after all.)
src/librustc_mir/borrow_check.rs
Outdated
Place::Projection(interior) => { | ||
place = &interior.base; | ||
} | ||
_ => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I'd personally be happier with Place::Local(..) | Place::Static(..) => { ... }
. When there are so few cases, I prefer to see them written out, so that I don't have to try and remember what they are. That said, I think Projection
is clearly the only case that would want to be handled here (that's the whole point of separating projections from base case...), so if you prefer to keep it as is, maybe use if let
instead just to be more compact?
src/librustc_mir/borrow_check.rs
Outdated
// Our invariant is, that at each step of the iteration: | ||
// - If we didn't run out of access to match, our borrow and access are comparable | ||
// and either equal or disjoint. | ||
// - If we did run out of accesss, the borrow can access a part of it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a wonderful comment.
Also: @bors p=1 This is super critical to making MIR borrowck functional. |
rustc_mir: don't move temporaries that are still used later. This should unbreak using the MIR borrow-checker on `libcore` (assuming #46268 is merged).
@arielb1 seems like it needs moar rebase. =( |
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).
This improves performance on large functions.
I'm not sure how correct it this, but it gets whatever needs to compile to compile.
This fixes the tests for issue rust-lang#29793
@nikomatsakis This should be good to go? |
@bors r+ p=1 |
📌 Commit 9d35587 has been approved by |
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
☀️ Test successful - status-appveyor, status-travis |
…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.
…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.
…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.
Fixes #44831.
Fixes #44834.
Fixes #45537.
Fixes #45696 (by implementing DerefPure semantics, which is what we want going forward).
r? @nikomatsakis