Closure tries to borrow &mut to entire struct, when only a field is required

[mihneadb]

Offending code:

struct Foo {
    b: i32,
    v: Vec<i32>
}

fn incr(x: &mut i32) {
    *x += 1;
}

fn main() {
    let mut f = Foo { b: 0, v: vec![1, 2, 3] };

    f.v.iter().map(|s| {
        incr(&mut f.b);
        println!("{}: {}", f.b, s);
    }).last();
}

Error: error: cannot borrow fas mutable becausef.v is also borrowed as immutable

On playpen: http://is.gd/PTXhJq

Version info:

rustc 0.13.0-nightly (d91a015ab 2014-11-14 23:37:27 +0000)
binary: rustc
commit-hash: d91a015ab4c538f02fbbc03dfb08193a381c2e3b
commit-date: 2014-11-14 23:37:27 +0000
host: x86_64-unknown-linux-gnu
release: 0.13.0-nightly

Thanks @bjz for helping me submit this.

[brendanzab]

I'm pretty sure that it is fine to reference f.v as immutable and f.b as mutable because they don't touch each other. You can fix this by doing:

fn main() {
    let mut f = Foo { b: 0, v: vec![1, 2, 3] };
    let b = &mut f.b;

    f.v.iter().map(|s| {
        incr(b);
        println!("{}: {}", b, s);
    }).last();
}

It would be nice if Rust was smarter about this though.

[joshtriplett]

I just ran into this one today, with 'f' being 'self'; that seems like a rather common case, which will come up any time you write self.somefield.method(|| { ... self.otherfield.method(...) ... }) (and at least one of the methods wants a &mut).

Based on some discussion in the #rust IRC channel, this seems potentially feasible to fix. The closure desugaring just needs to recognize references to fields and capture the field in that case, rather than capturing the entire top-level label.

[Diggsey]

Capturing the individual fields rather than "self" would significantly increase the size of the closure. It should be possible to internally capture just the "self" pointer, but make it appear for all intents and purposes that the fields were captured individually.

[steveklabnik]

Triage: no updates in almost two years.

Personal note: this code still compiles with the original error today. Neat! Not too much pre-1.0 stuff left that does that...

[oli-obk]

I don't think we should change this.

1. There's an easy workaround when you actually want to borrow only a part of the struct.
2. I actually like this, because it makes me think about what I'm doing.

What should be done imo is to add a suggestion to the error that gives the solution of borrowing the field into a new binding and then using that binding inside the closure.

[Diggsey]

@oli-obk That's fine if you're only borrowing one field, but it's a fair amount of boilerplate when you're borrowing say, all but one of the fields in the struct. It also produces a larger closure than necessary, which could be the difference between having to allocate the closure on the heap, vs being able to store it inline, if anyone writes a std::function equivalent for rust.

[oli-obk]

@Diggsey ah, so you are suggesting to store the pointer to the struct, but the borrowchecker will still forbid accessing already borrowed fields?

[Diggsey]

Yep :)

[joshtriplett]

That's what I would like to see as well: this would entirely be a change to the borrow checker, with the implementation still just passing the struct pointer (commonly a self pointer) around.

[samsartor]

This is fine:

let _a = &mut foo.a;
&mut foo.b; // ok!

So is this:

let _a = &mut foo.a;
loop { &mut foo.b; } // ok!

Why should closures be treated differently? It seems inconsistent with the rest of the language:

let _a = &mut foo.a;
|| &mut foo.b; // cannot borrow `foo` as mutable more than once at a time

Q: Does this require an RFC, or is it minimal and non-breaking enough to be fixed outright?

[Diggsey]

this would entirely be a change to the borrow checker

Might have to be careful not to hint to LLVM that this is an exclusive reference?

[arielb1]

@samsartor

This is enough of a feature to require an RFC.

[samsartor]

Working on an RFC now...