Some fixes to mir-borrowck #44736
Some fixes to mir-borrowck #44736
Conversation
|
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
|
@bors r+ |
|
📌 Commit 6438c7f has been approved by |
pnkfelix
added
the
S-waiting-on-author
pnkfelix
force-pushed
the
mir-borrowck4
branch
2 times, most recently
from
54068fd to
bf1c691
Compare
|
r? @arielb1 |
|
(one further refactoring that I am considering is to fold the calls to |
|
☔ The latest upstream changes (presumably #44784) made this pull request unmergeable. Please resolve the merge conflicts. |
src/librustc_mir/borrow_check.rs
Outdated
| @@ -174,13 +174,17 @@ impl<'c, 'b, 'a: 'b+'c, 'gcx, 'tcx: 'a> DataflowResultsConsumer<'b, 'gcx> | |||
| match stmt.kind { | |||
| StatementKind::Assign(ref lhs, ref rhs) => { | |||
| self.mutate_lvalue(ContextKind::AssignLhs.new(location), | |||
| (lhs, span), JustWrite, flow_state); | |||
| (lhs, span), Shallow(None), JustWrite, flow_state); | |||
There was a problem hiding this comment.
AST borrowck basically does a Deep write here and we might want to stay in sync with it. Also, if you leave this as Shallow without erasing borrows in dataflow::impls::borrows you'll have some very confusing diagnostics.
src/librustc_mir/borrow_check.rs
Outdated
| this.report_conflicting_borrow(context, lvalue_span, | ||
| (lvalue_span.0, bk), | ||
| (&borrow.lvalue, borrow.kind)), | ||
| WriteKind::StorageDead | // separate diagnostic for StorageDead case? |
There was a problem hiding this comment.
In AST borrowck, this is "borrow does not live long enough" for both Drop and StorageDead. ofc. you can match on the context instead if you want.
| impl<'c, 'b, 'a: 'b+'c, 'gcx, 'tcx: 'a> MirBorrowckCtxt<'c, 'b, 'a, 'gcx, 'tcx> { | ||
| fn access_lvalue(&mut self, |
There was a problem hiding this comment.
You should check somewhere that you have sufficient permissions to do the access - aka no mutable borrows of immutable references or moves through a non-Box reference.
There was a problem hiding this comment.
You can leave that to a different PR through.
There was a problem hiding this comment.
(added a fixme-comment noting that we need to have this.)
|
Basically r=me after a rebase. There are a few comments, but you can leave them to me or to a follow-up PR. |
src/librustc_mir/borrow_check.rs
Outdated
|
|
||
| // Is `lvalue` (or a prefix of it) already borrowed? If | ||
| // so, that's relevant. | ||
| for accessed_prefix in self.prefixes(lvalue, PrefixSet::All) { |
There was a problem hiding this comment.
This includes a "drive-by" fix to #38899. I think we might have a "legacy compatibility mode" that does not include this drive-by fix, so we can get a good summary of real issues in a crater.
At least add a comment // DIFFERENCE FROM BORROWCK.
|
Thanks for taking time |
|
I think my approach to the AST-/MIR-borrrowck discrepancies that @arielb1 has pointed out will be to just put in comments for now. Haven't decided 100% about that yet, but since one has to opt in to using MIR-borrowck in the first place via a Update: I decided in some cases to switch immediately to the AST-compatible mode. In either case I added comments in each instance pointing out that this particular piece of code is responsible for some planned discrepancy between AST- and (eventual) MIR-borrowck. |
In particular: * introduce the shallow/deep distinction for read/write accesses * use the notions of prefixes, shallow prefixes, and supporting prefixes rather than trying to recreate the restricted sets from ast-borrowck. * Add shallow reads of Discriminant and ArrayLength, and treat them as artificial fields when doing prefix traversals.
pnkfelix
force-pushed
the
mir-borrowck4
branch
from
bf1c691 to
e319f40
Compare
|
@bors r+ |
|
📌 Commit e319f40 has been approved by |
pnkfelix
added
S-waiting-on-bors
Some fixes to mir-borrowck Make the code more closely match the NLL RFC (updated description). (The biggest visible fix the addition of the Shallow/Deep distinction, which means mir-borrowck stops falsely thinking that StorageDeads need deep access to their input L-value.)
|
☀️ Test successful - status-appveyor, status-travis |
Make the code more closely match the NLL RFC (updated description).
(The biggest visible fix the addition of the Shallow/Deep distinction, which means mir-borrowck stops falsely thinking that StorageDeads need deep access to their input L-value.)