borrow checker does not work correctly with explicit return statements inside for-in loops

[jchlapinski]

When I try this:

#[derive(Debug)]
struct User {
    id: u64,
    email: String,
}

fn add_user<'a>(id: u64, users: &'a mut Vec<User>) -> &'a mut User {
    let mut index = ::std::usize::MAX;
    for (i, u) in users.iter_mut().enumerate() {
        if u.id == id {
            index = i;
            //break; // this would work
            return &mut users[index]; // this gives compilation error
        }
    };
    if index == ::std::usize::MAX {
        index = users.len();
        users.push(User { id: id, email: String::new() });
    }
    &mut users[index]
}

fn main() {
    let mut users = Vec::new();
    add_user(1, &mut users).email = "one@example.com".to_string();
    add_user(2, &mut users).email = "two@example.com".to_string();
    add_user(1, &mut users).email = "one1@example.com".to_string();
    println!("{:#?}", users);
}

it gives me a compilation error:

error[E0499]: cannot borrow `*users` as mutable more than once at a time
  --> <anon>:12:25
   |
9  |     for (i, u) in users.iter_mut().enumerate() {
   |                   ----- first mutable borrow occurs here
...
12 |             return &mut users[index];
   |                         ^^^^^ second mutable borrow occurs here
13 |         }
14 |     };
   |     - first borrow ends here

error: aborting due to previous error

link to Rust Playground: https://play.rust-lang.org/?gist=4d192eaabfc22904c9fb20e14ea39acc&version=nightly&backtrace=0

Now, when I simply change it to this:

#[derive(Debug)]
struct User {
    id: u64,
    email: String,
}

fn add_user<'a>(id: u64, users: &'a mut Vec<User>) -> &'a mut User {
    let mut index = ::std::usize::MAX;
    for (i, u) in users.iter_mut().enumerate() {
        if u.id == id {
            index = i;
            break; // this would work
            //return &mut users[index]; // this gives compilation error
        }
    };
    if index == ::std::usize::MAX {
        index = users.len();
        users.push(User { id: id, email: String::new() });
    }
    &mut users[index]
}

fn main() {
    let mut users = Vec::new();
    add_user(1, &mut users).email = "one@example.com".to_string();
    add_user(2, &mut users).email = "two@example.com".to_string();
    add_user(1, &mut users).email = "one1@example.com".to_string();
    println!("{:#?}", users);
}

(break instead of explicit return) everything works OK.
Link to Rust Playground: https://play.rust-lang.org/?gist=196c27cc90694c7b6daefe7ceeb517d3&version=nightly&backtrace=0

Clearly the borrow checker is working incorrectly when there is an explicit return inside a loop, as those programs are equivalent.

[keeperofdakeys]

The for loop is using a mutable iterator which has the borrow &mut users, so taking a second immutable or mutable borrow on users is invalid, even if you are returning the reference.

In this case, you might consider using a range query, and indexing users manually. This way there are never two mutable borrows of users active at the same time. You might also be able to simply use return u.

[jchlapinski]

@keeperofdakeys whether the iterator is mutable or not (iter() or iter_mut()) does not matter in this case. There still is a compilation error with return, albeit different.

I do realize that this program can be rewritten to work, that is not the issue here and this code is just an example to illustrate the problem. Lets not try to correct it.

The issue is that in my understanding explicit return statement should effectively end the for-loop scope (end borrowing done by the iterator), since this is the last expression that will be executed in the scope, and all other variables not referenced in the return expression must be safely discarded (dropped) before returning anyway.

Please note that when return in replaced by 'break' this code works, borrow checker is happy. But whether the loop is ended by break or return should not matter here. I think that the problem lays in that borrow checker is assuming that return expression must be executed within the current for-loop scope, but in fact the scope can be ended before executing return statement in this case.

I do realize that there are more such problems with the borrow checker and I've learnt to work around them, however I filled this issue in hope that this fix/optimization might not be too difficult to implement?

[Stebalien]

whether the iterator is mutable or not (iter() or iter_mut()) does not matter in this case.

The problem is that you're mutably borrowing users (return &mut users[index];) while it's already borrowed by the iterator. Whether you're using iter_mut or iter doesn't actually matter (you can't mutably borrow something while it is borrowed, mutably or otherwise).

The issue is that in my understanding explicit return statement should effectively end the for-loop scope (end borrowing done by the iterator), since this is the last expression that will be executed in the scope, and all other variables not referenced in the return expression must be safely discarded (dropped) before returning anyway.

Any code inside a block will be executed before the end of that block. If return statements were excluded, you wouldn't be able to, e.g., write:

for x in vec![1u32, 2, 3], iter() {
    // x: &u32;
    if x < 3 {
        return x + 1; // Under your proposal, `x` would be out of scope in this statement.
    }
}

However, there has been some thought put into something called non-lexical lifetimes. This feature might (depending on how (and if) it's implemented) allow the compiler to "unborrow" variables early in cases like this. However, there's a non-trivial amount of design work needed to even understand how this feature would work (let alone how it would be implemented) to avoid surprising the programmer.

See rust-lang/rfcs#811 for more.

[jchlapinski]

@Stebalien Thanks very much for the link, "non-lexical lifetimes" is precisely the optimization/fix I had in mind!. Basically finalization of scope's lifetime should be done as early as possible.

I hope this feature will be implemented, as fighting with the borrow checker in a logically correct code is rather annoying.

Any code inside a block will be executed before the end of that block. If return statements were excluded, you wouldn't be able to, e.g., write:

Of course not, that is why I used the phrase "in this case" at the end of the sentence. The case being that in my particular example the return statement does not use any binding from the for-in loop scope.

I am closing this, and I will follow the discussion in the RFC.
Cheers