-
Notifications
You must be signed in to change notification settings - Fork 13.4k
consider fixing common regression with expansion of 2-phase borrows #51915
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Comments
Adding to the Preview 2 milestone since this affected bootstrap. We have to decide how to proceed here though. |
Notably, the original source that gave rise to this error was:
and in this case you can see that the reborrow of |
(The fact that the borrow is implicit is important because we need for 2-phase borrows to have a clear activation point.) |
We decided to make a final decision about this based on the feedback from EP2. Moving to RC. |
Slightly surprisingly, the following version of the code does compile: struct S {
a: &'static str,
}
fn f(_: &mut S, _: &str) {
}
fn main() {
let mut s = S {
a: "a",
};
let r = &mut s;
f(r, r.a);
} |
Example from the #![feature(nll)]
use std::mem;
struct Node<T, U> {
links_len: usize,
entry: Entry<T, U>,
links: [*mut Node<T, U>; 0],
}
impl<T, U> Node<T, U> {
fn get_pointer(&self, index: usize) -> &*mut Node<T, U> {
loop { }
}
}
pub struct Entry<T, U> {
pub key: T,
pub value: U,
}
pub struct SkipMapIter<'a, T, U>
where
T: 'a,
U: 'a,
{
current: &'a *mut Node<T, U>,
}
impl<'a, T, U> Iterator for SkipMapIter<'a, T, U>
where
T: 'a + Ord,
U: 'a,
{
type Item = (&'a T, &'a U);
fn next(&mut self) -> Option<Self::Item> {
if self.current.is_null() {
None
} else {
unsafe {
let Entry { ref key, ref value } = (**self.current).entry;
mem::replace(&mut self.current, &*(**self.current).get_pointer(0));
Some((key, value))
}
}
}
}
fn main() { } yields
|
That said, this code would probably not be fixed by the proposed change, because the |
I think it would be helpful to have some kind of list for how often this sort of regression occurs in real life. I am currently leaning towards making no change here and instead improving the diagnostics (as described in #47349) |
I'm nominating this for discussion — I'm not sure what action to take here. Leaning towards nothing, as most of the 2PB regressions we've seen in the wild would not be readily addressed by this. |
In the discussion, we decided not to expand 2PB for the time being, but to consider that as future work. Link to our discussion |
Accordingly I am removing this from the milestone |
The following code works without NLL but not with NLL enabled:
https://play.rust-lang.org/?gist=9b797f941b3aa419991e15fd5a2d07a0&version=nightly&mode=debug
NLL is not wrong to reject this code. This is #38899. However, it could plausibly be accepted if we expanded two-phase borrows ever so slightly... actually, I'm not sure about this exact source, or it least it wouldn't quite fit into what I had in mind.
I had in mind an expansion for arguments of type
&mut
, basically, so thatfoo(bar)
, ifbar
is an&mut
that is being reborrowed, would reborrow using a 2-phase borrow.The text was updated successfully, but these errors were encountered: