Borrow checker bug?


#1

I’m trying to compile this code:

pub fn idle_loop(&mut self) -> TUIRet {
    loop {
        self.draw();

        match self.keypressed() {
            ret @ TUIRet::Abort => { return ret; },
            ret @ TUIRet::Input(_, _) => { return ret; },
            _ => {}
        }
    }
}

and it fails with “cannot borrow *self as immutable …” in self.draw(), showing that it’s already borrowed in the code that comes after that line, in self.keypressed().

This makes no sense at all. First, like I said, second borrow comes after this line. I believe the compiler is just confused about loop, thinking that borrows will live across iterations of the loop.

Any ideas?

EDIT: I forgot to add that TUIRet::Input has a reference to self.


#2

&mut self has a longer lifetime than the loop itself. It’s not a local variable to the loop, when one iteration of the loop finishes, the variable doesn’t get destroyed. One way to look at it: to the loop, self is like a global variable.
Could we see the signature of keypressed()? My guess would be that it looks something like that: keypressed(&mut self) -> TUIRet. keypressed could theoretically work with self in a separate thread(in your case it’s probably serial code, but it could be the other case), then the loop makes a circle and tries again to borrow self, this would be risky, so Rust basically saved you from a potential data-race mistake.

Could you provide a playground link with the code that doesn’t compile? May be we can help with suggestions how to change it.


#3

Here’s a minimal example: http://is.gd/RbYRws

I still don’t understand how does this have to do with threading or anything. I think this is just a clear bug. Can anyone try the example and explain why is that an error?


#4

It’s a known problem with overly conservative scoping. The borrow scope of &mut self is extended to the rest of the function, to make sure it lives long enough to be returned, which clashed with how the loop scope is defined. The borrow checker thinks that self may still be borrowed during the next iteration. It’s treated as if you had written this: http://is.gd/xTDLJL and the compiler doesn’t realize that ret_val can only be set once.


#5

I actually think this is a good situation where Rust protects you from doing a mistake.
Ok, we see right now what the code does, but lets imagine that we didn’t know what keypressed(&mut self) does, only that it takes a mutable reference to self.
Are you, as the user of the function, absolutely sure that when keypressed returns, that self is not being modified?
In release 1.0 of the library, keypressed is serial and when it returns, all is ok with our data and we can read it in draw().
Now, 1.1 comes along and decides that keypressed actually spawns a thread(and that TUIRet returns if the thread, event loop, … was created) that will mutate for a while self and then finish and close(of course keypressed doesn’t know that it will be called in a loop {}). The function returns, the loop goes on to the next cicle, but that thread is still mutating &mut self, now draw() comes along and tries to read some data from self, boom, data-race(potential), world gone, rockets flying into babies, lawn mowers killing cats and “Wow, I received 476328742 euros from my bank instead of the asked 50 euros”.


#6

The signature of keypress is pub fn keypressed<'a>(&'a mut self) -> TUIRet<'a> when lifetime annotations are added in. A thread would either be scoped and have the same lifetime as the function call, or have a longer lifetime. Both of these situations should be totally safe because the compiler would obviously make sure that the scoped thread blocks the main thread until it’s finished, and the non-scoped thread wouldn’t be able to take control of self, since it’s limited to the lifetime 'a.

The return value of keypress isn’t used past the match, and the return could be limited to that scope, so this could be safe. The compiler could come to the conclusion that the loop will stop there, and that kind of analysis is already in there. Try removing the breaks from my example, and the compiler will notice that ret_val will be assigned more than once.


#8

Does anyone know any workarounds?


#9

It depends. Is the value of Input a slice of something within TUI? If yes, does it have to keep it or can it be moved instead of sliced?


#10

Right, sorry for not giving more context. Indeed I can just move things from self to TUIRet and that solves everything, but that programming style encourages lots of redundant copying. I’m already doing a lot of redundant copying just to avoid lifetime parameters and errors like this, I really want to avoid copying in this case, if possible.


#11

Is it a buffer of text input? You don’t necessarily have to copy it if it’s not supposed to be read more than once. You could instead replace it with an empty string:

let old_buffer = ::std::mem::replace(&mut self.buffer, String::new());

You are in a deeper pickle if you have to keep buffer intact for later, but you could move the borrowing into the idle_loop function, using a simpler private function: http://is.gd/d8PaFy