two-phase borrows are causing #[feature(nll)] to reject a mutable borrow incorrectly
#48070
Labels
A-NLL
Area: Non-lexical lifetimes (NLL)
NLL-complete
Working towards the "valid code works" goal
T-compiler
Relevant to the compiler team, which will review and decide on the PR/issue.
Milestone
Comments
nikomatsakis
added
T-compiler
|
cc @rust-lang/wg-compiler-nll |
|
This is a case where our debug flags are very helpful for isolating the cause of a bug like this.
I was scratching my head at this point... and then it hit me: |
pnkfelix
changed the title
pnkfelix
changed the title
#[feature(nll)] to reject a mutable borrow incorrectly
|
@pnkfelix ah, good catch! I guess this is because those mutable borrows are "activating" at this later point? Fascinating. |
|
I'm going to take a look at this bug. |
|
Here's the MIR dump I managed to extract: And its corresponding MIR dump looks like the following: It's probably not that important to look at the MIR dump, because the second line of the RUST_LOG output contained this: Where the last parameter is the value of Note that _4 is a temporary created by MIR that will be used to call .emit() upon. |
|
I think what is happening is not that surprising. I don't think it's related to the Let's simplify to two arms. I think you'll find this has the same error: fn main() {
let mut foo = Foo { x: 0 };
match 22 {
22 => &mut foo, // borrow B1
_ => foo.twaddle(), // borrow B2
}.emit();
}I've also labeled the two As you can see from the diagram, borrow B2 is already active when borrow B1 is activated. This is in part why the initial RFC for two-phase borrows was limited to the temporaries created for method calls: I wanted to ensure that the temporary was (a) assigned once and (b) that assigned was postdominated by a use which was dominated by the one assignment. |
|
I'm a bit surprised though that this doesn't work now. Maybe @pnkfelix's PR that limits two-phase borrows hasn't yet made it into nightly? Would be good to verify whether this still activates with the latest master. |
|
So I just rebased against master, and I still run into this exact same bug. @nikomatsakis which PR were you referring to? |
|
Hmm I wonder if #47489 is still too naive? I’ll try to investigate tomorrow |
|
(I've started looking at the dataflow state, via borrowck_graphviz_postflow. The initial information I'm getting from there is confusing me at best, so I'm not sure yet what conclusion to draw from it. Will report more later once I've delved into it a little more deeply.) |
nikomatsakis
added a commit
to nikomatsakis/rust
that referenced
this issue
Feb 20, 2018
|
So, looking at the dataflow graph (after the dataflow has been run, so the entry sets contain the fixed point results), generated via
The dataflow is concluding, on entry to bb7, that both of the borrow Thus we get an error that it is not valid to have two simultaneous multiple borrows of One logical step to take next would be to compare against the dataflow that is derived when we do not have two-phase-borrows turned on. I am going to do that in a bit. |
|
The bug here is probably that we are erroring on having two mutable-borrows of the same path reach the use of I often get the details wrong; I suspect that the issue is that we should solely be detecting conflicts between mutable borrows of a given path at the point where the borrow is taken (i.e. the points were we do Here, we are checking for it at the point where the borrow is activated (the use of And so that explains why we do not see the problem without two-phase borrows turned on. When two-phase borrows are turned off, the check in question is performed at the point of reservation (since activation points are irrelevant when two-phase borrows are turned off). |
|
@pnkfelix I believe that is roughly the same as my diagnosis here, right? |
|
Well, let me read your comments more closely, because I'm confused which borrow is not activating. |
|
@nikomatsakis there's some differences compared to your diagnosis, but I don't know if they matter. For example, your hypothesized control flow graph has two activations that reach the same control flow point. The actual graph has a reservation on one path (but no activation), and the other path has a reservation and an activation. |
|
So, the thing that is confusing me from the output is that the (Note: this might also need my fix from #47689) |
|
An update regarding the previous comment: We discussed further on gitter; to summarize, I think that the intention is that two-phase borrows have no effect on the dataflow -- instead, their effect is meant to be encoded solely in the borrow_check code itself. Specific to this case, the dataflow only shows a reservation, not an activation, at the statement |
|
After even further discussion on gitter and also in video chat, @nikomatsakis and @pnkfelix came to the conclusion that the answer here is probably not a surgical fix covering a small number of lines, but rather a slightly larger-scale (but mentorable) effort to redo two-phase borrows so that it more closely matches the original intention of the RFC. See #48431 |
sapphire-arches
added a commit
to sapphire-arches/rust
that referenced
this issue
Mar 13, 2018
Resolves rust-lang#48070. The bug itself was fixed by rust-lang#48770, but that PR didn't add a test for it.
Merged
kennytm
added a commit
to kennytm/rust
that referenced
this issue
Mar 14, 2018
…atsakis Add a test for rust-lang#48070 Resolves rust-lang#48070. The bug itself was fixed by rust-lang#48770, but that PR didn't add a test for it. r? @nikomatsakis
nikomatsakis
added a commit
to nikomatsakis/rust
that referenced
this issue
Mar 19, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
In this example, the NLL checker incorrectly judges
footo be borrowed more than once:I get this error:
This works without NLL. This was found while trying to bootstrap rustc with NLL enabled, where it manifests as:
The text was updated successfully, but these errors were encountered: