Skip to content
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

&mut self borrow conflicting with itself. #21906

Open
leejunseok opened this issue Feb 3, 2015 · 23 comments
Open

&mut self borrow conflicting with itself. #21906

leejunseok opened this issue Feb 3, 2015 · 23 comments
Labels
A-borrow-checker Area: The borrow checker A-NLL Area: Non-lexical lifetimes (NLL) A-typesystem Area: The type system C-bug Category: This is a bug. fixed-by-polonius Compiling with `-Zpolonius` fixes this issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@leejunseok
Copy link
Contributor

leejunseok commented Feb 3, 2015

I don't know why this happens but rovar and XMPPwocky on IRC believed it to be a compiler bug.

struct A {
    a: i32
}

impl A {
    fn one(&mut self) -> &i32{
        self.a = 10;
        &self.a
    }
    fn two(&mut self) -> &i32 {
        loop {
            let k = self.one();
            if *k > 10i32 {
                return k;
            }
        }
    }
}

...gives the following error...

<anon>:12:21: 12:25 error: cannot borrow `*self` as mutable more than once at a time
<anon>:12             let k = self.one();
                              ^~~~
<anon>:12:21: 12:25 note: previous borrow of `*self` occurs here; the mutable borrow prevents subsequent moves, borrows, or modification of `*self` until the borrow ends
<anon>:12             let k = self.one();
                              ^~~~
<anon>:17:6: 17:6 note: previous borrow ends here
<anon>:10     fn two(&mut self) -> &i32 {
...
<anon>:17     }
              ^
error: aborting due to previous error
playpen: application terminated with error code 101

Interestingly, if the second method is changed to...

    fn two(&mut self) -> &i32 {
        loop {
            let k = self.one();
            return k;
        }
    }

This will compile fine.

playpen: http://is.gd/mTkfw5

@kmcallister kmcallister added the A-typesystem Area: The type system label Feb 4, 2015
@gereeter
Copy link
Contributor

gereeter commented Feb 6, 2015

I believe this is correct behavior. To see why, consider the code with explicit lifetime annotations:

struct A {
    a: i32
}

impl A {
    fn one<'a>(&'a mut self) -> &'a i32{
        self.a = 10;
        &self.a
    }
    fn two<'a>(&'a mut self) -> &'a i32 {
        loop {
            let k = self.one();
            if *k > 10i32 {
                return k;
            }
        }
    }
}

two needs to return a reference with the same lifetime as the mutable borrow given to it. However, since it gets its. This means that k must have lifetime 'a. However, for the returned value of one to have lifetime 'a, the input to one must also have lifetime 'a. This means that Rust is forced to not "reborrow" self and move it in to one. Since mutable references are linear, they can only be moved once, but the loop means that one may need to be called with the same mutable reference repeatedly.

The second example only works because Rust can see that the loop will only run once, so self will only be moved once.

@Gankra
Copy link
Contributor

Gankra commented Feb 6, 2015

This looks like a non-lexical borrows issue. You can get similar behaviour without any loops at all:

struct Foo { data: Option<i32> }

fn main() {
    let mut x = Foo{data: Some(1)};

    foo(&mut x);
}

fn foo(x: &mut Foo) -> Option<&mut i32> {
    if let Some(y) = x.data.as_mut() {
        return Some(y);
    }

    println!("{:?}", x.data); 
    None
}
<anon>:14:22: 14:28 error: cannot borrow `x.data` as immutable because it is also borrowed as mutable
<anon>:14     println!("{:?}", x.data); 
                               ^~~~~~
note: in expansion of format_args!
<std macros>:2:43: 2:76 note: expansion site
<std macros>:1:1: 2:78 note: in expansion of println!
<anon>:14:5: 14:30 note: expansion site
<anon>:10:22: 10:28 note: previous borrow of `x.data` occurs here; the mutable borrow prevents subsequent moves, borrows, or modification of `x.data` until the borrow ends
<anon>:10     if let Some(y) = x.data.as_mut() {
                               ^~~~~~
<anon>:16:2: 16:2 note: previous borrow ends here
<anon>:9 fn foo(x: &mut Foo) -> Option<&mut i32> {
...
<anon>:16 }
          ^
error: aborting due to previous error

Rustc can't "deal" with conditional borrowing returns.

@Gankra
Copy link
Contributor

Gankra commented Feb 6, 2015

CC @pcwalton

@oli-obk
Copy link
Contributor

oli-obk commented Apr 13, 2015

I think the issue is actually that not the return value's mutability is used but the function argument's mutability.

The following code calls a mutating function that returns an immutable reference. After that I cannot take immutable references.

struct Foo (u8);

impl Foo {
    fn bar(&mut self) -> &u8 {
        self.0 += 1;
        &self.0
    }
}

fn main() {
    let mut x = Foo(42);
    let a = x.bar(); // note: borrow of `x` occurs here
    let b = x.0; // error: cannot use `x.0` because it was mutably borrowed
}

Playpen

@ghost
Copy link

ghost commented Dec 19, 2015

I'm still running into this issue on the latest nightly. It appears that it treats returns as having a lifetime until the end of the function, even if the function without the return passes the borrow checker. This seems kind of odd, considering how the point of a return is to end the function early.

If you take a look at @gankro's example, it passes the borrow checker if you remove the return keyword, even though the resulting function won't work properly.

@Gankra
Copy link
Contributor

Gankra commented Dec 22, 2015

This is blocked on non-lexical lifetimes, which is in turn blocked on MIR. Ideally this will be fixed by the end of 2016, but a fix won't land soon.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 29, 2016

@aidanhs
Copy link
Member

aidanhs commented Jul 20, 2016

Non lexical borrows feature discussion: rust-lang/rfcs#811

@Mark-Simulacrum
Copy link
Member

So reading the internals thread makes me feel like this isn't fixable (even with NLL). Can someone confirm? (@eddyb?)

@eddyb
Copy link
Member

eddyb commented May 7, 2017

cc @rust-lang/lang

@nikomatsakis
Copy link
Contributor

This is indeed NLL and, if I'm not mistaken, it would be fixed by the various proposals that I have made. This roughly corresponds to "problem case #3" from my first blog post. The basic idea (in terms of the formulation from the latest blog post) of why it would work is that the subtyping the borrow would only be required to extend until the end of 'a in the branch of the if that causes a return.

@krdln
Copy link
Contributor

krdln commented May 17, 2017

@nikomatsakis Could you please elaborate more on how NLL interacts with this issue? In my mental model of &mut references, they can be passed either by move or reborrow, and the problem in this issue is that the return requires the move mode, and the re-use of self afterwards requires reborrow. In my understanding, the new reborrowed &mut reference's lifetime is limited by the lifetime of the variable holding the &mut-reference – in this case, self variable, thus it's limited by the function's body, thus it cannot extend outside the function call. Are the NLL going to change that limitation of reborrows (or maybe there is no such limitation)?

Also I am wondering if fixing this issue is something inherently tied to NLL or is it maybe orthogonal? If it's the latter, maybe it's worth landing a fix before NLL?

Also, if NLL are going to fix this issue, does that mean that under NLL you'll never have to manually choose between a move and reborrow?

@osa1
Copy link
Contributor

osa1 commented May 22, 2017

Is this going to be fixed any time soon?

@nikomatsakis
Copy link
Contributor

@krdln

Could you please elaborate more on how NLL interacts with this issue?

The summary is that, today, if a value gets returned out of the function on any path, the loan must be valid for the rest of the function on all paths. Under the NLL proposal I wrote up, that restriction is lifted.

In more detail, if you take the elaborated signature here, you can see that we must return a reference with lifetime 'a:

fn two<'a>(&'a mut self) -> &'a i32 { .. }

now observe that we call let k = self.one() and then return this k. This means that the type of k must be &'a i32. This in turn means that when we call self.one(), we must supply it a borrow of self with type &'a mut Self. Therefore, at the let k = self.one() we issue a loan of self with lifetime 'a. This lifetime is bigger than the loop (and, indeed, the entire function), so it persists as we go around the loop, leading to the error report. Under the NLL rules that I proposed, the lifetime of k is only required to extend to 'a if the if is taken. Therefore, if the if is not taken, the loan can end earlier.

Does that help?


@osa1

Is this going to be fixed any time soon?

There are still a few steps to go before we can do this, but there is active work on doing the refactorings needed.

@krdln
Copy link
Contributor

krdln commented May 23, 2017

@nikomatsakis
Thanks, it helped somewhat. But there's one little thing that I don't understand – what exactly happens when you write self in your code – how the reborrow of self works. I've read your exaplanation of the "Problem case 3" (get_default), where you inline the code inside the caller, but there, you've changed every use of self into borrowing the map variable directly, so that desugaring didn't clear it up for me.

Here's where I'm stuck: When we call let k = self.one(), the self can't be moved (because it's needed later), so it's consider borrowed. Later, we conditionally return that k, so that borrow has to have a lifetime 'a, which outlives the function call. But! We've borrowed from self, which lives only till the end of the function. That limitation seems to shorten 'a, so in my mental model, even under NLL, we should get "does not live long enough" error.

@nikomatsakis
Copy link
Contributor

@krdln we've actually borrowed from *self -- that is, we've reborrowed what self refers to. We permit you to borrow *self for a lifetime outliving self itself because the type of self implies a 'lock' for the entire lifetime 'a. Hence, as long as we can guarantee that during 'a, self is no longer used, the result should be an exclusive reference -- in this case, after the fn returns, self has been popped, and hence it could not be used, so just guaranteeing that self is not used until the end of the fn suffices. (At least I hope that's true. =)

@Mark-Simulacrum Mark-Simulacrum added A-borrow-checker Area: The borrow checker C-bug Category: This is a bug. A-NLL Area: Non-lexical lifetimes (NLL) labels Jul 22, 2017
@starwed
Copy link

starwed commented May 16, 2018

This is blocked on non-lexical lifetimes, which is in turn blocked on MIR. Ideally this will be fixed by the end of 2016, but a fix won't land soon.

Not very surprising, but I confirmed that the initial example does indeed compile on nightly if you turn the non-lexical lifetimes feature on.

@shepmaster shepmaster added NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. labels May 16, 2018
@SkylerLipthay
Copy link
Contributor

I think I ran into a similar problem in production where an immutable borrow seems to live too long (workaround).

I should also mention that starwed's code no longer compiles on the latest nightly. @oberien suggested a "regression-from-nightly-to-nightly" tag, and it was also suggested I should tag @nikomatsakis in case this was an urgent problem :)

@matthewjasper matthewjasper added NLL-polonius Issues related for using Polonius in the borrow checker and removed NLL-fixed-by-NLL Bugs fixed, but only when NLL is enabled. labels Dec 26, 2018
@kurnevsky
Copy link

kurnevsky commented Feb 6, 2019

Stumbled upon this issue trying to do something very similar (which is not allowed by current nll):

fn f(vec: &mut Vec<u8>) -> &u8 {
    if let Some(n) = vec.iter_mut().find(|n| **n == 1) {
        *n = 10;
        n
    } else {
        vec.push(10);
        vec.last().unwrap()
    }
}

fn main() {
    let mut vec = vec![1, 2, 3];
    f(&mut vec);
}
error[E0499]: cannot borrow `*vec` as mutable more than once at a time
 --> src/main.rs:6:9
  |
1 | fn f(vec: &mut Vec<u8>) -> &u8 {
  |           - let's call the lifetime of this reference `'1`
2 |     if let Some(n) = vec.iter_mut().find(|n| **n == 1) {
  |                      --- first mutable borrow occurs here
3 |         *n = 10;
4 |         n
  |         - returning this value requires that `*vec` is borrowed for `'1`
5 |     } else {
6 |         vec.push(10);
  |         ^^^ second mutable borrow occurs here

error[E0502]: cannot borrow `*vec` as immutable because it is also borrowed as mutable
 --> src/main.rs:7:9
  |
1 | fn f(vec: &mut Vec<u8>) -> &u8 {
  |           - let's call the lifetime of this reference `'1`
2 |     if let Some(n) = vec.iter_mut().find(|n| **n == 1) {
  |                      --- mutable borrow occurs here
3 |         *n = 10;
4 |         n
  |         - returning this value requires that `*vec` is borrowed for `'1`
...
7 |         vec.last().unwrap()
  |         ^^^ immutable borrow occurs here

@pnkfelix
Copy link
Member

(we should have removed E-needstest when we removed NLL-fixed-by-NLL)

@pnkfelix pnkfelix removed the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Jun 27, 2019
@crlf0710 crlf0710 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 11, 2020
@camsteffen
Copy link
Contributor

I just ran into this and it really confused me for a while. I think a tip along the lines of "try changing the first borrow to an immutable borrow" would have helped me find a solution faster, at least in my case. Even though in retrospect that seems obvious...

Example
fn last_big(vec: &mut Vec<i32>) -> &mut i32 {
    match **vec {
        [.., ref mut last] if *last > 99 => last,
        _ => {
            vec.push(100); // ERROR: can't borrow mutably again
            vec.last_mut().unwrap()
        }
    }
}

@tamird
Copy link
Contributor

tamird commented Oct 13, 2023

This bug has a lot of duplicates, and is problem case #3 in the NLL RFC. Should we close some of these duplicates?

#51545
#54663
#58910
#70255
#97901
#112087

This was intentionally cut from NLL, which @shepmaster described in #51545 (comment).

Should some of these duplicates be closed so that there's on canonical bug with all the relevant breadcrumbs (and perhaps an update on polonius 🤞)? cc @nikomatsakis.

@fmease fmease added fixed-by-polonius Compiling with `-Zpolonius` fixes this issue. and removed NLL-polonius Issues related for using Polonius in the borrow checker labels Jan 29, 2024
@JakkuSakura
Copy link

This is blocked on non-lexical lifetimes, which is in turn blocked on MIR. Ideally this will be fixed by the end of 2016, but a fix won't land soon.

Not very surprising, but I confirmed that the initial example does indeed compile on nightly if you turn the non-lexical lifetimes feature on.

Sadly, latest nightly doesn't compile it.

I wrote an Entry API for Vec instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-borrow-checker Area: The borrow checker A-NLL Area: Non-lexical lifetimes (NLL) A-typesystem Area: The type system C-bug Category: This is a bug. fixed-by-polonius Compiling with `-Zpolonius` fixes this issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests