NLL / MIR-borrowck regression in getopts

[Aaron1011]

Compiling getopts version 0.2.15 with cargo build succeeds. However, compiling the same version with RUSTFLAGS='-Z borrowck=mir -Z nll -Z two-phase-borrows' cargo build fails with the following errors:

error[E0503]: cannot use `state` because it was mutably borrowed
    --> src/lib.rs:1033:19
     |
978  |       let mut machine = |cont: &mut bool, (i, c): (usize, char)| {
     |  _______________________-
979  | |         let whitespace = if c.is_whitespace() { Ws }       else { Cr };
980  | |         let limit      = if (i - slice_start + 1) <= lim  { UnderLim } else { OverLim };
981  | |
...    |
1027 | |         *cont
1028 | |     };
     | |_____- borrow of `state` occurs here
...
1033 |       while cont && match state { B | C => true, A => false } {
     |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ use of borrowed `state`

error[E0503]: cannot use `state` because it was mutably borrowed
    --> src/lib.rs:1033:33
     |
978  |       let mut machine = |cont: &mut bool, (i, c): (usize, char)| {
     |  _______________________-
979  | |         let whitespace = if c.is_whitespace() { Ws }       else { Cr };
980  | |         let limit      = if (i - slice_start + 1) <= lim  { UnderLim } else { OverLim };
981  | |
...    |
1027 | |         *cont
1028 | |     };
     | |_____- borrow of `state` occurs here
...
1033 |       while cont && match state { B | C => true, A => false } {
     |                                   ^ use of borrowed `state`

error: aborting due to 2 previous errors

[Aaron1011]

I'll need to investigate further, but I suspect that this may have the same root cause as #47680.

In particular, this line:

state = match (state, whitespace, limit) {

seems to fit the pattern of 'borrow variable, manipulate borrow, assign manipulated borrow back to original' that appears in the many examples in the above issue.

[Aaron1011]

This minimized example seems to confirm it: (playground):

#![feature(nll)]

enum State {
    A
}

fn bar() {
    let mut state = State::A;

    let mut closure = || {
        state = match state {
            State::A => State::A,
        };
    };

    while match state {  State::A => false } {
        closure();
    }
}

fn main() {}

Removing #![feature(nll)] allows this snippet to compile.

[nikomatsakis]

Hmm, I'm tagging this as NLL-deferred because #47680 is out of scope for the present borrow checker (tackling that is Polonius's job). However, it's interesting that this is a regression. My guess is that the old borrow checker was unsound here, but I'm not entirely sure.

[pnkfelix]

The minimized example from the playground works now?

@Aaron1011 do you know off hand whether the problem in getopts is still arising today? And also, is it only exposed by #![feature(nll)? (I.e. how does just the migration mode enabled via the 2018 edition behave)

---

With respect to #56754, marking as NLL-complete for now, as well as E-needs-test and P-medium.

[pnkfelix]

(also assigning to self to make the test and followup with @Aaron1011 )

[Aaron1011]

@pnkfelix: Running RUSTFLAGS='-Z borrowck=mir' cargo +nightly buil still reproduces the original error.

[pnkfelix]

Okay yes I can reproduce (atop getopts version 0.2.15, that is)

[pnkfelix]

Migrate mode saves us here w.r.t. the 2018 edition

i.e. I see this output under migrate mode:

warning[E0503]: cannot use `state` because it was mutably borrowed
    --> src/lib.rs:1033:33
     |
978  |     let mut machine = |cont: &mut bool, (i, c): (usize, char)| {
     |                       ---------------------------------------- borrow of `state` occurs here
...
982  |         state = match (state, whitespace, limit) {
     |         ----- borrow occurs due to use of `state` in closure
...
1033 |     while cont && match state { B | C => true, A => false } {
     |                                 ^ use of borrowed `state`
1034 |         machine(&mut cont, (fake_i, ' '));
     |         ------- borrow used here, in later iteration of loop
     |
     = warning: This error has been downgraded to a warning for backwards compatibility with previous releases.
             It represents potential unsoundness in your code.
             This warning will become a hard error in the future.

so this is not super super high priority to resolve.

Just ... sort of high priority.

Re-tagging as P-high.

[pnkfelix]

Its also worth revisiting @nikomatsakis 's note that the fact this compiled under AST-borrowck may represent unsoundness there. Its certainly suspicious that the closure bound to machine is (clearly?) doing a mutable borrow of state, and there are calls to machine before and after a use of state.

• (but then why does the reduced example compile... curiouser and curiouser...)

[pnkfelix]

ah I think the reduced test case compiles because it reduces the intervening "read" of state to match state { State::A => false } and it looks like MIR borrowck manages to ... treat that as a no-op?

If I put two variants into the State enum and actually use them to drive the loop condition, then we get the error I expect here (play):

#![feature(nll)]

enum State {
    A,
    B
}

fn bar() {
    let mut state = State::A;
    let state_ref = &mut state;

    let mut closure = || {
        *state_ref = match *state_ref {
            _ => State::A,
        };
    };

    while match state { State::A => true, State::B => false } {
        closure();
    }
}

fn main() { 
    bar();
}

(and the compiler continues to flag an error even if you get rid of the state_ref i introduced above while debugging: play)

[pnkfelix]

@nikomatsakis do you actually think this is a duplicate of #47680 ?

#47680 is a case where you have a factoring of an expression into a separate method. But there's no implicit captures from closures (I think)

This (#48225) is, I think, describing a factoring into a closure that makes use of free variables rather than passing them anew. But I don't know if its reasonable to expect that to work...?

In other words, this fails to compile under NLL (play):

#![feature(nll)]
fn main() {
    enum State { A, B }
    let mut state = State::A;
    let closure = || {
        state = match state {
            State::A => State::B,
            State::B => State::A,
        };
    };
    while match state { State::A => false, State::B => true } {
        closure();
    }
}

but this variant below will compile and run fine, because instead of having a single mut borrow captured by the closure in a free variable, I now create free mutable borrows at each call (play):

#![feature(nll)]
fn main() {
    enum State { A, B }
    let mut state = State::A;
    let closure = |s: &mut _| {
        *s = match *s {
            State::A => State::B,
            State::B => State::A,
        };
    };
    while match state { State::A => false, State::B => true } {
        closure(&mut state);
    }
}

[pnkfelix]

nominating for discussion at next NLL meeting, in case @nikomatsakis and/or @rust-lang/wg-compiler-nll doesn't get around to resolving the question I posed in the previous comment before then.

[nikomatsakis]

@pnkfelix Indeed, I do not see the "hallmark" of #47680, which I consider to be:

• one branch that creates an outlives relationship, but kills the loan,
• and one branch that does not create an outlives relationship.

That said, the examples I see here look to me like they ought not to compile. Moreover, they don't compile, even in 2015 Rust.

But this example does compile in 2015 but not 2018: (you also posted it)

enum State {
    A,
    B
}

fn bar() {
    let mut state = State::A;
    let state_ref = &mut state;

    let mut closure = || {
        *state_ref = match *state_ref {
            _ => State::A,
        };
    };

    while match state { State::A => true, State::B => false } {
        closure();
    }
}

fn main() { 
    bar();
}

This seems like a bug in 2015 Rust, though.

[nikomatsakis]

I'm taking over this issue, but I think at this point it looks like it should just be closed as "no bug here".

[nikomatsakis]

Nominating for discussion in the next NLL meeting. I think we likely should just close this.

[nikomatsakis]

We discussed this in the NLL meeting today and decided everything was working as expected. Closing.