-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
[NLL] Be more permissive when checking access due to Match #53438
Conversation
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
c10b7ec
to
7ab8ac1
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
7ab8ac1
to
867fc1e
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Ugh, good catch! |
☔ The latest upstream changes (presumably #53581) made this pull request unmergeable. Please resolve the merge conflicts. |
f278d08
to
3972d81
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.
I'm pondering this. Adding a new kind of borrow doesn't feel great to me, since it is sort of a "fundamental extension", but maybe it's the right thing.
src/librustc/mir/mod.rs
Outdated
@@ -427,6 +427,26 @@ pub enum BorrowKind { | |||
/// Data must be immutable and is aliasable. | |||
Shared, | |||
|
|||
/// When matching on a place we cannot further mutation as this can break |
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: missing verb. "we cannot permit future mutation"?
src/librustc/mir/mod.rs
Outdated
/// | ||
/// This can't be a shared borrow because: | ||
/// * Existing mutable references are fine, since they prevent matching | ||
/// with a pattern that isn't exhaustive. |
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 don't understand what "existing mutable references" means here
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 meant that we can match on a mutably borrowed place.
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.
e.g., match &mut foo { ... }
?
src/librustc/mir/mod.rs
Outdated
/// This can't be a shared borrow because: | ||
/// * Existing mutable references are fine, since they prevent matching | ||
/// with a pattern that isn't exhaustive. | ||
/// * All accesses should be considered deep for conflicts with this kind |
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: some missing puncutation, I think. "All accesses should be considered deep for conflicts: with this kind of borrow, the above example..."
src/librustc_mir/borrow_check/mod.rs
Outdated
@@ -533,6 +524,10 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx | |||
self.consume_operand(context, (input, span), flow_state); | |||
} | |||
} | |||
StatementKind::ReadForMatch(_) => { | |||
// If a temporary is ReadForMatch, then there are no other | |||
// accesses of it, so there's no need to check anything here. |
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 don't understand this comment, can you elaborate?
// value of a memory location. | ||
ShallowOrDeep::Deep | ||
} else { | ||
access |
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 is weird to me that we examine the borrow kind in order to change the access. I see why it sort of does the right thing, but something seems off to me. I wonder if what we "really want" here is instead to have multiple shared borrows, rather than adding a new kind of borrow -- that is, we originally thought that a single borrow of the "place being matched" would suffice, but it seems like that is not true, we want to borrow all prefixes of the place being matched perhaps? Or..something like that.
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 probably more sensible here. But there still needs to be some way to not error when matching on a borrowed place.
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.
But there still needs to be some way to not error when matching on a borrowed place.
I'm not entirely sure what you mean by that, actually, what's the example you have in mind?
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.
// permission check already account for this borrow. | ||
debug!("places_conflict: behind a shared ref"); | ||
return false; | ||
} |
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.
As I wrote in a comment somewhere else:
(a) I think that the second element of the tuples here (the type) is now unused
(b) Why is it ok to remove this logic -- is it because we aren't tracking the borrows anymore? If so, maybe we can add an assertion of some kind...
cc @pnkfelix -- you've been doing a lot of the work in this area |
Having thought about this, I am still tempted by the idea of pushing this into the MIR desugaring in the form of multiple borrows. I'm not sure how best to code things but I think what I would ideally want would be:
This would presumably be the most precise thing, though it might introduce a fair number of borrows and hence have a perf impact on "match heavy" code. It's also kind of a pain since we accumulate the list of But maybe that's too complex? Still, it feels not that complex to me, and we need the artifical reads and so forth for other reasons... |
I'll give this a try to see how it ends up. But it does sound like a plausible way forwards. |
☔ The latest upstream changes (presumably #53909) made this pull request unmergeable. Please resolve the merge conflicts. |
3972d81
to
670c5de
Compare
Updated to the new approach |
This comment has been minimized.
This comment has been minimized.
670c5de
to
a8e7d6f
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Actual error:
Looks like stage1 is miscompiling things. |
I think I have a solution once #54188 is merged |
@matthewjasper does the description reflect the current state of the PR? Eg is this still adding a new BorrowKind for match? |
The description has been updated |
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.
Awesome work.
src/librustc/mir/mod.rs
Outdated
@@ -456,6 +456,23 @@ pub enum BorrowKind { | |||
/// Data must be immutable and is aliasable. | |||
Shared, | |||
|
|||
/// When matching on a place we want to ensure that places have the same |
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 but I'd prefer to start with a description of the "semantics" of a "shallow borrow". I think this is accurate:
The immediately borrowed Place
must be immutable
but projections from that place are not considered borrowed.
So a shallow borrow of a.b
still permits writes to a.b.c
.
This effectively borrows the discriminant of enums as well as
atomic integer values.
This is used when desugaring matches to ensure that
the places involved in the match have the same value
from the start of the match until an arm is selected. ...
src/librustc_mir/borrow_check/mod.rs
Outdated
@@ -1294,6 +1300,9 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { | |||
match *rvalue { | |||
Rvalue::Ref(_ /*rgn*/, bk, ref place) => { | |||
let access_kind = match bk { | |||
BorrowKind::Shallow => { | |||
(Shallow(Some(ArtificialField::Discriminant)), Read(ReadKind::Borrow(bk))) |
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.
Are we sure we want discriminant here? Isn't part of the purpose of this to freeze integer values and the like? Maybe a comment might be nice here too.
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.
From looking over the code, it seems like the effect of this is that if you have a borrow of (say) a.b.c
and we shallow borrow a.b
, this won't be considered a conflict. I think .. this is correct, but I'm wondering if maybe we should add another ArtificialField
variant, like ShallowBorrow
. Re-using Discriminant
seems to give bad "intuitions" to me, though I guess I can't think where it would go wrong.
Maybe we should just rename ArtificialField::Discriminant
to ShallowBorrow
, actually.
src/librustc_mir/borrow_check/mod.rs
Outdated
@@ -1505,7 +1514,14 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { | |||
// that is merged. | |||
let sd = if might_be_alive { Deep } else { Shallow(None) }; | |||
|
|||
if places_conflict::places_conflict(self.tcx, self.mir, place, root_place, sd) { | |||
if places_conflict::places_conflict( |
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.
we should maybe rename this borrow_conflicts_with_place
. That would make me feel better about passing in the borrow.kind
😛
src/librustc_mir/borrow_check/mod.rs
Outdated
@@ -1505,7 +1514,14 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { | |||
// that is merged. |
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 think this FIXME is outdated
// so many false edges in the future, so we read for all Candidates for | ||
// now. | ||
// Another option would be to make our own block and add our own false | ||
// edges to 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.
Hmm, there can sometimes be a lot of arms. I like the idea of branching to a block via fake-edges, but I'm trying to figure out how that would work... would said block "branch back" to every arm? I'm worried that might cause excessive problems of its own, but perhaps not, perhaps we already have equivalent fake edges?
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 guess we can see if this becomes a perf problem. I just know we've encountered e.g. matches with huge numbers of arms being generated from constants. But typically in that case the match is matching on a single value, so I guess it would just be linear growth, which is probably fine.
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.
Indeed, I think that this can stay as is until someone shows performance problems. I think the most likely way this could end up being slow is from a custom derive on an enum with a lot of variants with fields (which somehow adds a guard somewhere).
@@ -473,11 +473,20 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx | |||
); | |||
} | |||
StatementKind::ReadForMatch(ref place) => { | |||
self.access_place( | |||
// Read for match doesn't access any memory and is used to |
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.
Which borrows was this conflicting with? It feels like we should be able to treat this as an access. Maybe the ReadForMatch
statements are coming at the wrong place?
Oh, I guess I see, this is not really about the shallow borrows we are making, but rather about the "read for match" that we add to the "main path" being matched?
(Which may or may not have a shallow borrow associated with 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 for the discriminant => ReadForMatch change (although I might change this when ReadForMatch is split)
--> $DIR/borrowck-describe-lvalue.rs:201:15 | ||
| | ||
LL | let x = &mut e; | ||
| ------ borrow of `e` occurs here |
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.
did you inspect all these caes? are these duplicate errors?
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 just inspected them myself. They all appear to be cases where we were previously flagging both the initial match v { ... }
as conflicting with a mutable borrow of v
, as well as flagging the binding in the pattern. In every case I think the github presentation of the diff here shows the flagging of the binding in the pattern as an error.
However, it does lead me to wonder: do we have a test somewhere of how we are handling let x = &mut v; match v { _ => ... }; use(x);
...? Because a PR like this might affect that. I haven't tried to look for that yet.
(And I'm not really certainly I know how it should affect it. I believe i understand that under this PR's model, let x = &mut v.0; match v { _ => ... }; use(x);
would be accepted by the compiler. But would let x = &mut v; match v { _ => ... }; use(x);
be rejected ?)
// switchInt(move _9) -> [0isize: bb5, 1isize: bb3, otherwise: bb7]; | ||
// ReadForMatch(_2); | ||
// _7 = discriminant(_2); | ||
// _9 = &(promoted[2]: std::option::Option<i32>); |
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.
are these shallow borrows? can we have that show up somehow in the MIR?
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, I'll change how they are printed
This comment has been minimized.
This comment has been minimized.
8dc27ae
to
67c9653
Compare
☔ The latest upstream changes (presumably #54509) made this pull request unmergeable. Please resolve the merge conflicts. |
8916946
to
9208578
Compare
rebased and fixed test and an out of date comment |
@bors r=pnkfelix |
📌 Commit 92085784b5f66045ee388cdd777670ee041a6615 has been approved by |
@matthewjasper in general you can feel free to |
This allows treating the "fake" match borrows differently from shared borrows.
As we are now creating borrows of places that may not be valid for borrow checking matches, these have to be removed to avoid generating broken code.
This name better reflects the asymmetry of this function.
9208578
to
a830732
Compare
@bors r=pnkfelix |
📌 Commit a830732 has been approved by |
[NLL] Be more permissive when checking access due to Match Partially addresses #53114. notably, we should now have parity with AST borrowck. Matching on uninitialized values is still forbidden. * ~~Give fake borrows for match their own `BorrowKind`~~ * ~~Allow borrows with this kind to happen on values that are already mutably borrowed.~~ * ~~Track borrows with this type even behind shared reference dereferences and consider all accesses to be deep when checking for conflicts with this borrow type. See [src/test/ui/issues/issue-27282-mutate-before-diverging-arm-3.rs](cb5c989#diff-a2126cd3263a1f5342e2ecd5e699fbc6) for an example soundness issue this fixes (a case of #27282 that wasn't handled correctly).~~ * Create a new `BorrowKind`: `Shallow` (name can be bike-shed) * `Shallow` borrows differ from shared borrows in that * When we check for access we treat them as a `Shallow(Some(_))` read * When we check for conflicts with them, if the borrow place is a strict prefix of the access place then we don't consider that a conflict. * For example, a `Shallow` borrow of `x` does not conflict with any access or borrow of `x.0` or `*x` * Remove the current fake borrow in matches. * When building matches, we take a `Shallow` borrow of any `Place` that we switch on or bind in a match, and any prefix of those places. (There are some optimizations where we do fewer borrows, but this shouldn't change semantics) * `match x { &Some(1) => (), _ => (), }` would `Shallow` borrow `x`, `*x` and `(*x as Some).0` (the `*x` borrow is unnecessary, but I'm not sure how easy it would be to remove.) * Replace the fake discriminant read with a `ReadForMatch`. * Change ReadForMatch to only check for initializedness (to prevent `let x: !; match x {}`), but not conflicting borrows. It is still considered a use for liveness and `unsafe` checking. * Give special cased error messages for this kind of borrow. Table from the above issue after this PR | Thing | AST | MIR | Want | Example | | --- | --- | --- | --- |---| | `let _ = <unsafe-field>` | 💚 | 💚 | ❌ | [playground](https://play.rust-lang.org/?gist=bb7843e42fa5318c1043d04bd72abfe4&version=nightly&mode=debug&edition=2015) | | `match <unsafe_field> { _ => () }` | ❌ | ❌ | ❌ | [playground](https://play.rust-lang.org/?gist=3e3af05fbf1fae28fab2aaf9412fb2ea&version=nightly&mode=debug&edition=2015) | | `let _ = <moved>` | 💚 | 💚 | 💚 | [playground](https://play.rust-lang.org/?gist=91a6efde8288558e584aaeee0a50558b&version=nightly&mode=debug&edition=2015) | | `match <moved> { _ => () }` | ❌ | ❌ | 💚 | [playground](https://play.rust-lang.org/?gist=804f8185040b2fe131f2c4a64b3048ca&version=nightly&mode=debug&edition=2015) | | `let _ = <borrowed>` | 💚 | 💚 | 💚 | [playground](https://play.rust-lang.org/?gist=0e487c2893b89cb772ec2f2b7c5da876&version=nightly&mode=debug&edition=2015) | | `match <borrowed> { _ => () }` | 💚 | 💚 | 💚 | [playground](https://play.rust-lang.org/?gist=0e487c2893b89cb772ec2f2b7c5da876&version=nightly&mode=debug&edition=2015) | r? @nikomatsakis
☀️ Test successful - status-appveyor, status-travis |
Partially addresses #53114. notably, we should now have parity with AST borrowck. Matching on uninitialized values is still forbidden.
Give fake borrows for match their ownBorrowKind
Allow borrows with this kind to happen on values that are already mutably borrowed.Track borrows with this type even behind shared reference dereferences and consider all accesses to be deep when checking for conflicts with this borrow type. See src/test/ui/issues/issue-27282-mutate-before-diverging-arm-3.rs for an example soundness issue this fixes (a case of Can mutate in match-arm using a closure #27282 that wasn't handled correctly).BorrowKind
:Shallow
(name can be bike-shed)Shallow
borrows differ from shared borrows in thatShallow(Some(_))
readShallow
borrow ofx
does not conflict with any access or borrow ofx.0
or*x
Shallow
borrow of anyPlace
that we switch on or bind in a match, and any prefix of those places. (There are some optimizations where we do fewer borrows, but this shouldn't change semantics)match x { &Some(1) => (), _ => (), }
wouldShallow
borrowx
,*x
and(*x as Some).0
(the*x
borrow is unnecessary, but I'm not sure how easy it would be to remove.)ReadForMatch
.let x: !; match x {}
), but not conflicting borrows. It is still considered a use for liveness andunsafe
checking.Table from the above issue after this PR
let _ = <unsafe-field>
match <unsafe_field> { _ => () }
let _ = <moved>
match <moved> { _ => () }
let _ = <borrowed>
match <borrowed> { _ => () }
r? @nikomatsakis