Cannot borrow *self as mutable more than once at a time

[In-line]

I tried this code:

struct Test;
impl Test {
    fn test(&mut self) -> Option<&str> {
        match self.test() {
            None => None,
            Some(txt) if txt.len() == 1 => Some(txt),
            Some(_) => self.test()
        }
    }
}

I expected to it to compile without erorrs.

Instead, this happened:

error[E0499]: cannot borrow `*self` as mutable more than once at a time
  --> src/lib.rs:17:24
   |
13 |     fn test(&mut self) -> Option<&str> {
   |             - let's call the lifetime of this reference `'1`
14 |         match self.test() {
   |               ---- first mutable borrow occurs here
15 |             None => None,
16 |             Some(txt) if txt.len() == 1 => Some(txt),
   |                                            --------- returning this value requires that `*self` is borrowed for `'1`
17 |             Some(_) => self.test()
   |                        ^^^^ second mutable borrow occurs here

Playground link
Playground (version with if)

[Andy-Python-Programmer]

This is supposed to happen. The lifetime of &str in the code you mentioned above is tied to the lifetime to &mut self and as the compile error says cannot borrow *self as mutable more than once at a time therefore in this situation using String instead of &str will be more suitable.

For more in detail information check out the rust book chapter 4: https://doc.rust-lang.org/book/ch04-00-understanding-ownership.html. Also just to be aware your code will cause an infinite loop as you are matching the value of self.test() in self.test() without any conditions (the compiler will warn you about this).

[In-line]

This is supposed to happen. The lifetime of &str in the code you mentioned above is tied to the lifetime to &mut self and as the compile error says cannot borrow *self as mutable more than once at a time therefore in this situation using String instead of &str will be more suitable.

For more in detail information check out the rust book chapter 4: https://doc.rust-lang.org/book/ch04-00-understanding-ownership.html. Also just to be aware your code will cause an infinite loop as you are matching the value of self.test() in self.test() without any conditions (the compiler will warn you about this).

It doesn't explain, why different match arms produce such result. It's seems to be this issue fixed by NLL.

[In-line]

Polonius borrow checker seems to compile code just fine, but I'm still not 100% sure if this code is correct.

[pchampin]

@In-line submitted this bug based on my question on StackOverflow -- thanks to him.

Below is a slightly less artificial example, which maybe makes the problem clearer:

pub struct Test(String);
impl Test {
    pub fn next(&mut self) -> Option<&str> {
        self.0.pop();
        if self.0.is_empty() {
            None
        } else {
            Some(&self.0)
        }
    }
    pub fn next_multiple(&mut self, n: usize) -> Option<&str> {
        match self.next() {
            None => None,
            Some(txt) if txt.len() % n == 0 => Some(txt),
            Some(_) => self.next()
        }
    }
}

Playground link

The method next_multiple causes the borrow checker to complain, while it seems to me that this code should be fine.

[iamthetoaster]

I'm a little fuzzy on lifetimes, so take this with a grain of salt, but it looks like the issue is that the lifetime of the first self.next() persists until the end of the match block, and you're expecting it to end before the last match arm's expression.

It seems to me like it definitely needs the lifetime to persist at least to the => so it can be sure that the value matches that case, but it could end at the arrow as the value is not used any more. That, however, looks like more complicated behavior that would be harder to reason about, rather than just assuming the lifetime of the thing matched on goes until the end of the match.

Like I said, I'm fuzzy on lifetimes, so I might be missing something, but this seems like expected and proper behavior to me.

[nagisa]

The problem here is that the reference created by the first self.match() is only made dead in this function right before a return. In slightly abridged-MIR:

    bb0: {
        StorageLive(_2);                 // this is the variable we'll be matching on, the function result
        _3 = &mut (*_1);
        _2 = Test::test(move _3) -> bb1;
    }

    bb1: {
        _4 = discriminant(_2);
        switchInt(move _4) -> [0_isize: bb3, 1_isize: bb4, otherwise: unreachable];
    }

    bb3: {
        // The None branch
        discriminant(_0) = 0;
        goto -> bb8;
    }

    bb4: {
        _6 = &((_2 as Some).0: &str);
        _8 = (*_6);
        _7 = core::str::<impl str>::len(move _8) -> bb5;
    }

    bb5: {
        switchInt(move _7) -> [1_usize: bb6, otherwise: bb7];
    }

    bb6: {
        // Some(_) if ... branch
        _5 = ((_2 as Some).0: &str);
        _9 = _5;
        ((_0 as Some).0: &str) = move _9;
        discriminant(_0) = 1;
        goto -> bb8;
    }

    bb7: {
        // this is the last Some(_) branch...
        goto -> bb8;
    }

    bb8: {
        StorageDead(_2);                 // matched variable finally dead here
        return;
    }

there isn't any reason why the StorageDead(_2) couldn't be sunk into the bb6 and bb7, making them look more like...

    bb6: {
        // Some(_) if ... branch
        _5 = ((_2 as Some).0: &str);
        StorageDead(_2);                         // no longer used...
        _9 = _5;
        ((_0 as Some).0: &str) = move _9;
        discriminant(_0) = 1;
        goto -> bb8;
    }

    bb7: {
        StorageDead(_2);
        // this is the last Some(_) branch...
        // no borrows to `self` or the return value of `Test::test` are live at this point, free to call whatever user wishes to.
        goto -> bb8;
    }

    bb8: {
        return;
    }

However, such a sinking transformation is only valid in absence of side-effectful drops (which Rust does have). In that situation user could observe that the drop for the return value of the first self.test() is called before the second self.test() is invoked.

Additionally, while we do have situations where the behaviour of borrowck changes based on certain drop-specific relationships, we still maintain the property that in most (all?) cases adding a Drop implementation will not make borrowck explode. Making the given code example build would, however, make it a problem (probably a much more prominent one).

[In-line]

@nagisa Can we theoretically change drop order for next Rust edition?

[pchampin]

@nagisa thanks for this explanations. I tried to explicitly change the drop order using std::mem::drop, but still no luck.

    pub fn next_multiple(&mut self, n: usize) -> Option<&str> {
        let opt = self.next();
        match opt {
            None => return None,
            Some(txt) if txt.len() % n == 0 => return Some(txt),
            Some(_) => (),
        }
        std::mem::drop(opt);
        self.next()
    }

link to playground

[JakkuSakura]

I have a similar issue like this. I have client_id and server_id. The logic is to get a &mut Order by either client_id or server_id, dynamically. I have to use unsafe to work around borrow checker.

    pub fn get_live_order_mut(&mut self, client_id: &str, server_id: &str) -> Option<&mut Order> {
        let this = self as *mut Self;
        unsafe {
            if let Some(x) = (*this).live_orders.get_by_cid_mut(client_id) {
                Some(x)
            } else {
                (*this).live_orders.get_by_sid_mut(server_id)
            }
        }
    }