NLL (two-phase-borrows) regression: image crate

[SimonSapin]

rustc 1.25.0-nightly (27a046e 2018-02-18), https://github.com/PistonDevelopers/image, output with an unrelated warning removed.

cargo +nightly build: OK

cargo +nightly rustc -- -Znll: OK

cargo +nightly rustc -- -Znll -Ztwo-phase-borrows

   Compiling image v0.18.0 (file:///home/simon/projects/image)
error[E0499]: cannot borrow `_` as mutable more than once at a time
   --> src/tiff/decoder/mod.rs:403:39
    |
402 |             (ColorType::RGBA(8), DecodingBuffer::U8(ref mut buffer)) => {
    |                                                     -------------- first mutable borrow occurs here
403 |                 try!(reader.read(&mut buffer[..bytes]))
    |                                       ^^^^^^ second mutable borrow occurs here

error[E0499]: cannot borrow `_` as mutable more than once at a time
   --> src/tiff/decoder/mod.rs:403:39
    |
401 |             (ColorType:: RGB(8), DecodingBuffer::U8(ref mut buffer)) |
    |                                                     -------------- first mutable borrow occurs here
402 |             (ColorType::RGBA(8), DecodingBuffer::U8(ref mut buffer)) => {
403 |                 try!(reader.read(&mut buffer[..bytes]))
    |                                       ^^^^^^ second mutable borrow occurs here

error[E0499]: cannot borrow `_` as mutable more than once at a time
   --> src/tiff/decoder/mod.rs:407:30
    |
405 |             (ColorType::RGBA(16), DecodingBuffer::U16(ref mut buffer)) |
    |                                                       -------------- first mutable borrow occurs here
406 |             (ColorType:: RGB(16), DecodingBuffer::U16(ref mut buffer)) => {
407 |                 for datum in buffer[..bytes/2].iter_mut() {
    |                              ^^^^^^ second mutable borrow occurs here

error[E0499]: cannot borrow `_` as mutable more than once at a time
   --> src/tiff/decoder/mod.rs:407:30
    |
406 |             (ColorType:: RGB(16), DecodingBuffer::U16(ref mut buffer)) => {
    |                                                       -------------- first mutable borrow occurs here
407 |                 for datum in buffer[..bytes/2].iter_mut() {
    |                              ^^^^^^ second mutable borrow occurs here

error: aborting due to 4 previous errors

error: Could not compile `image`.

cargo +nightly rustc -- -Zborrowck=mir -Ztwo-phase-borrows

   Compiling image v0.18.0 (file:///home/simon/projects/image)
error[E0499]: cannot borrow `_` as mutable more than once at a time
   --> src/tiff/decoder/mod.rs:403:39
    |
402 |             (ColorType::RGBA(8), DecodingBuffer::U8(ref mut buffer)) => {
    |                                                     -------------- first mutable borrow occurs here
403 |                 try!(reader.read(&mut buffer[..bytes]))
    |                 ----------------------^^^^^^-----------
    |                 |                     |
    |                 |                     second mutable borrow occurs here
    |                 first borrow ends here
    |
    = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error[E0499]: cannot borrow `_` as mutable more than once at a time
   --> src/tiff/decoder/mod.rs:407:30
    |
403 |                 try!(reader.read(&mut buffer[..bytes]))
    |                 --------------------------------------- first borrow ends here
404 |             }
405 |             (ColorType::RGBA(16), DecodingBuffer::U16(ref mut buffer)) |
    |                                                       -------------- first mutable borrow occurs here
406 |             (ColorType:: RGB(16), DecodingBuffer::U16(ref mut buffer)) => {
407 |                 for datum in buffer[..bytes/2].iter_mut() {
    |                              ^^^^^^ second mutable borrow occurs here
    |
    = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error: aborting due to 2 previous errors

error: Could not compile `image`.

[pietroalbini]

cc @nikomatsakis

[Aaron1011]

I'd like to work on this.

[nikomatsakis]

I have a suspicion that this is related to two-phase borrows, and hence a duplicate of #48431 / #48070 / #48418.

[Aaron1011]

There actually seems to be two distinct issues at play here. One is #48495 - buffer should be declared mut.

The main cause of the image crate error is a problem with how NLL borrow-checks multiple patterns within a single match arm.

This snippet compiles with ast-borrowck, but not with NLL (playground):

#![feature(nll)]

enum SimpleFoo {
    Ref(u8)
}

fn bar(mut foo: SimpleFoo) {
    match foo {
        SimpleFoo::Ref(ref mut inner) | SimpleFoo::Ref(ref mut inner) => {
            println!("Inner: {:?}", inner);
        },
    }
}

fn main() {
    bar(SimpleFoo::Ref(5));
}

NLL seems to be treating the two 'ref mut's as distinct borrows, even though only one of them will ever be used.

[Aaron1011]

I believe the issue has to do with how match arms are desuraged. A new BasicBlock is created for each of the two patterns in the arm, each of which performs a mutable borrow of Ref.0. Since both of these blocks eventually jump to the println block, region inference infers that both are in scope when the println executes.

Any solution needs to ensure that snippets like this one continue to not compile:

#![feature(nll)]

fn bar(a: u8) {
    let mut first = 5;
    let mut second = 6;
    let temp;
    
    match a {
        10 => {
            temp = &mut first;
        },
        20 => {
            temp = &mut second;
        }
        _ => panic!()
    }
    
    println!("Variables: {:?} {:?}", first, second);
    *temp = 7;
}

fn main() {}

I think the best approach would be to change how the ExprMatch hir is lowered to MIR. If two patterns in the same arm have bindings in common, the genenerated MIR should be placed in its own basic block. The basic blocks for each arm will initialized their non-shared bindings, then jump to the common block to initialize the 'shared' bindings.

[Aaron1011]

A related regression:

#![feature(nll)]

fn bar(a: usize) {
    let mut first = 5;
    let temp;
    
    match a {
        10 => {
            temp = &mut first;
        },
        20 => {
            temp = &mut first;
        }
        _ => panic!()
    }
    
    println!("Temp: {:?}", temp);
}

fn main() {
    bar(10)
}

The general theme seems to be that the depth-first search performed by region inference seems to be interacting badly with multiple borrows of the same place.

In general, region inference is doing the right thing- even though only branch will ever be taken, borrow checking needs to act as though both are taken, so that things like my previous snippet correctly fail to compile. However, we run into problems when the same place is borrowed in multiple blocks that eventually flow to the same target. What we really care about is ensuring that the place remains borrowed - which branch the borrow occurs on isn't important.

[Aaron1011]

One more minimization:

#![feature(nll)]

fn bar(a: bool) {
    let mut first = 5;
    let temp;
    
    if a {
        temp = &mut first;
    } else {
        temp = &mut first;
    }
    
    *temp = 20;
}

fn main() {
    bar(true)
}

[Aaron1011]

I believe @nikomatsakis was correct - this seems to be a duplicate of #48070, as removing -Z two-phase-borrows resolves the issue.

[nikomatsakis]

This should in theory be fixed now -- certainly the minimizations by @Aaron1011 have been fixed.

[SimonSapin]

Confirmed that cargo +nightly rustc -- -Znll -Ztwo-phase-borrows now works in the image crate with rustc 1.26.0-nightly (a04b88d 2018-03-19). Thanks.