Clean up the MIR borrowck code #46558
Conversation
kennytm
added
the
S-waiting-on-review
There was a problem hiding this comment.
This all seems pretty good to me, modulo the final commit, but I'm willing to run with it for now. The one thing I was concerned about was the "cannot assign twice" error message, which struck me as misleading. I also think it'd be nice to move those tests to ui tests so that we can see the full output.
|
|
||
| fn f(y: Box<isize>) { | ||
| *y = 5; //~ ERROR cannot assign | ||
| *y = 5; //[ast]~ ERROR cannot assign | ||
| //[mir]~^ ERROR cannot assign twice |
There was a problem hiding this comment.
Why are we reporting "cannot assign twice" here? That is, I don't see a second assignment? I guess it is referring to whatever initialize *y in the caller?
There was a problem hiding this comment.
Because the value was potentially initialized by the caller. i.e. this is the same error as:
fn foo(_x: u32) {
_x = 4;
}
fn main() {}And the error message is (should be the same between AST/MIR):
error[E0384]: cannot assign twice to immutable variable `_x`
--> src/main.rs:2:5
|
1 | fn foo(_x: u32) {
| -- first assignment to `_x`
2 | _x = 4;
| ^^^^^^ cannot assign twice to immutable variable
error: aborting due to previous error
There was a problem hiding this comment.
So, I think this is a pretty confusing error message, but I see that this is the current behavior. I think we should special-case arguments, but it seems like a separate PR.
| @@ -1242,7 +1242,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { | |||
| let mut error_reported = false; | |||
| match kind { | |||
| Write(WriteKind::MutableBorrow(BorrowKind::Unique)) => { | |||
| if let Err(_place_err) = self.is_unique(place) { | |||
| if let Err(_place_err) = self.is_mutable(place, LocalMutationIsAllowed::Yes) { | |||
There was a problem hiding this comment.
Hmm. I'm not sure if I like this particular commit. In part this is because I don't like the "is local mutation" allowed flag -- I'm trying to put clearly into words why. I guess because it feels like there is context to the is_mutable check that is dependent on the path being assigned (and in particular whether it's been assigned before), so is_mutable in some sense is not able to answer the question it is being asked.
There was a problem hiding this comment.
Maybe we could instead refactor is_mutable so that it doesn't return a bool but some kind of conditional result.
There was a problem hiding this comment.
That said, particularly since I can't quite clearly enunciate my concerns, I'm happy enough to land these commits and then take a stab at refactoring things myself -- I might well find I don't like the result.
|
☔ The latest upstream changes (presumably #46572) made this pull request unmergeable. Please resolve the merge conflicts. |
The borrow_check module is too big for its own good
We can now use it in e.g. drop elaboration if we want to.
If the gen/kill bits are set there, the effects of `start_block_effects` will not be seen when using `FlowAtLocation` etc. to go over the MIR. EverInitializedLvals is the only pass that got this wrong, but this fixes the footgun for everyone.
This fixes the handling of reassignment of struct fields.
arielb1
force-pushed
the
union-borrow-refactor
branch
from
0fe5b69 to
e798cb0
Compare
|
@bors r+ |
|
📌 Commit e798cb0 has been approved by |
kennytm
added
S-waiting-on-bors
Clean up the MIR borrowck code I want to avoid the new code to start with so much technical debt. r? @nikomatsakis
|
☀️ Test successful - status-appveyor, status-travis |
I want to avoid the new code to start with so much technical debt.
r? @nikomatsakis