two `ref mut` patterns with same name used in arm guard causes ICE on NLL

[jonhoo]

To reproduce:

#![feature(nll)]

fn foo(x: &mut Result<u32, u32>) {
    match x {
        Ok(v) | Err(v) if *v > 0 => { }
        _ => { }
    }
}

fn main() { }

---

Original bug report:

To reproduce:

$ git clone https://github.com/mit-pdos/distributary.git
$ cd distributary
$ rustc --version
rustc 1.28.0-nightly (29f48ccf3 2018-06-03)
$ git checkout cargo-update
$ cargo check
...
    Checking distributary v0.1.0 (file:///home/jon/dev/projects/distributary)
thread 'main' panicked at 'assertion failed: old_value.is_none()', librustc_mir/borrow_check/borrow_set.rs:350:9
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::print
             at libstd/sys_common/backtrace.rs:71
             at libstd/sys_common/backtrace.rs:59
   2: std::panicking::default_hook::{{closure}}
             at libstd/panicking.rs:211
   3: std::panicking::default_hook
             at libstd/panicking.rs:227
   4: rustc::util::common::panic_hook
   5: std::panicking::rust_panic_with_hook
             at libstd/panicking.rs:515
   6: std::panicking::begin_panic
   7: <rustc_mir::borrow_check::borrow_set::GatherBorrows<'a, 'gcx, 'tcx> as rustc::mir::visit::Visitor<'tcx>>::visit_assign
   8: rustc_mir::borrow_check::borrow_set::BorrowSet::build
   9: rustc_mir::borrow_check::do_mir_borrowck
  10: rustc::ty::context::tls::with_related_context
  11: rustc::infer::InferCtxtBuilder::enter
  12: rustc_mir::borrow_check::mir_borrowck
  13: rustc::ty::maps::__query_compute::mir_borrowck
  14: rustc::ty::maps::<impl rustc::ty::maps::config::QueryConfig<'tcx> for rustc::ty::maps::queries::mir_borrowck<'tcx>>::compute
  15: rustc::ty::context::tls::with_context
  16: rustc::dep_graph::graph::DepGraph::with_task_impl
  17: rustc::ty::context::tls::with_related_context
  18: rustc::ty::maps::plumbing::<impl rustc::ty::context::TyCtxt<'a, 'gcx, 'tcx>>::force_query_with_job
  19: rustc::ty::maps::plumbing::<impl rustc::ty::context::TyCtxt<'a, 'gcx, 'tcx>>::get_query
  20: rustc::ty::maps::<impl rustc::ty::context::TyCtxt<'a, 'tcx, 'lcx>>::mir_borrowck
  21: rustc_mir::borrow_check::nll::type_check::type_check_internal
  22: rustc_mir::borrow_check::nll::type_check::type_check
  23: rustc_mir::borrow_check::nll::compute_regions
  24: rustc_mir::borrow_check::do_mir_borrowck
  25: rustc::ty::context::tls::with_related_context
  26: rustc::infer::InferCtxtBuilder::enter
  27: rustc_mir::borrow_check::mir_borrowck
  28: rustc::ty::maps::__query_compute::mir_borrowck
  29: rustc::ty::maps::<impl rustc::ty::maps::config::QueryConfig<'tcx> for rustc::ty::maps::queries::mir_borrowck<'tcx>>::compute
  30: rustc::ty::context::tls::with_context
  31: rustc::dep_graph::graph::DepGraph::with_task_impl
  32: rustc::ty::context::tls::with_related_context
  33: rustc::ty::maps::plumbing::<impl rustc::ty::context::TyCtxt<'a, 'gcx, 'tcx>>::force_query_with_job
  34: rustc::ty::maps::plumbing::<impl rustc::ty::context::TyCtxt<'a, 'gcx, 'tcx>>::get_query
  35: rustc::ty::maps::<impl rustc::ty::context::TyCtxt<'a, 'tcx, 'lcx>>::mir_borrowck
  36: rustc_driver::driver::phase_3_run_analysis_passes::{{closure}}::{{closure}}
  37: rustc::util::common::time
  38: rustc::ty::context::tls::enter_context
  39: <std::thread::local::LocalKey<T>>::with
  40: rustc::ty::context::TyCtxt::create_and_enter
  41: rustc_driver::driver::compile_input
  42: rustc_driver::run_compiler_with_pool
  43: <scoped_tls::ScopedKey<T>>::set
  44: syntax::with_globals
  45: <std::panic::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
  46: __rust_maybe_catch_panic
             at libpanic_unwind/lib.rs:105
  47: rustc_driver::run
  48: rustc_driver::main
  49: std::rt::lang_start::{{closure}}
  50: std::panicking::try::do_call
             at libstd/rt.rs:59
             at libstd/panicking.rs:310
  51: __rust_maybe_catch_panic
             at libpanic_unwind/lib.rs:105
  52: std::rt::lang_start_internal
             at libstd/panicking.rs:289
             at libstd/panic.rs:374
             at libstd/rt.rs:58
  53: main
  54: __libc_start_main
  55: <unknown>
query stack during panic:
#0 [mir_borrowck] processing `controller::sql::passes::implied_tables::rewrite_selection::{{closure}}`
#1 [mir_borrowck] processing `controller::sql::passes::implied_tables::rewrite_selection`
end of query stack

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.28.0-nightly (29f48ccf3 2018-06-03) running on x86_64-unknown-linux-gnu

note: compiler flags: -C debuginfo=2 -C incremental -C target-cpu=native --crate-type lib

note: some of the compiler flags provided by cargo are hidden

error: Could not compile `distributary`.

[pietroalbini]

Was this working with a previous nightly?

[jonhoo]

It's a little hard to evaluate as we're using a fork of HashMap that recently had to be updated to match nightly. The same code compiled just fine with an older nightly (2018-05-13) before a cargo update.

[jonhoo]

This also happens with CARGO_INCREMENTAL=0 and on 2018-06-05.

[jonhoo]

It also seems to be related to two-phased borrows?

[jonhoo]

Okay, here's a reproducing failure case. Happens only with NLL as far as I can tell.

#![feature(nll)]

pub struct Foo {
    pub bar: Option<String>,
    pub baz: Option<Box<Baz>>,
}

pub enum Baz {
    A(Foo),
    B(Foo, String),
}

fn sadness() {
    use Baz::*;

    let _fail = |mut f: Foo| -> Foo {
        f.bar = match f.bar {
            None => match f.baz {
                Some(ref mut f) => {
                    match (*f).as_mut() {
                        | &mut A(ref mut foo) | &mut B(ref mut foo, _) if foo.bar.is_none() => {
                            foo.bar = None;
                        }
                        _ => {}
                    }
                    None
                }
                None => None,
            },
            Some(x) => Some(x),
        };
        f
    };
}

$ cargo check
    Checking sadness v0.1.0 (file:///home/jon/d)
thread 'main' panicked at 'assertion failed: old_value.is_none()', librustc_mir/borrow_check/borrow_set.rs:350:9
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::print
             at libstd/sys_common/backtrace.rs:71
             at libstd/sys_common/backtrace.rs:59
   2: std::panicking::default_hook::{{closure}}
             at libstd/panicking.rs:211
   3: std::panicking::default_hook
             at libstd/panicking.rs:227
   4: rustc::util::common::panic_hook
   5: std::panicking::rust_panic_with_hook
             at libstd/panicking.rs:515
   6: std::panicking::begin_panic
   7: <rustc_mir::borrow_check::borrow_set::GatherBorrows<'a, 'gcx, 'tcx> as rustc::mir::visit::Visitor<'tcx>>::visit_assign
   8: rustc_mir::borrow_check::borrow_set::BorrowSet::build
   9: rustc_mir::borrow_check::do_mir_borrowck
  10: rustc::ty::context::tls::with_related_context
  11: rustc::infer::InferCtxtBuilder::enter
  12: rustc_mir::borrow_check::mir_borrowck
  13: rustc::ty::query::__query_compute::mir_borrowck
  14: rustc::ty::query::<impl rustc::ty::query::config::QueryAccessors<'tcx> for rustc::ty::query::queries::mir_borrowck<'tcx>>::compute
  15: rustc::ty::context::tls::with_context
  16: rustc::dep_graph::graph::DepGraph::with_task_impl
  17: rustc::ty::context::tls::with_related_context
  18: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt<'a, 'gcx, 'tcx>>::force_query_with_job
  19: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt<'a, 'gcx, 'tcx>>::get_query
  20: rustc::ty::query::<impl rustc::ty::context::TyCtxt<'a, 'tcx, 'lcx>>::mir_borrowck
  21: rustc_mir::borrow_check::nll::type_check::TypeChecker::typeck_mir
  22: rustc_mir::borrow_check::nll::type_check::type_check_internal
  23: rustc_mir::borrow_check::nll::compute_regions
  24: rustc_mir::borrow_check::do_mir_borrowck
  25: rustc::ty::context::tls::with_related_context
  26: rustc::infer::InferCtxtBuilder::enter
  27: rustc_mir::borrow_check::mir_borrowck
  28: rustc::ty::query::__query_compute::mir_borrowck
  29: rustc::ty::query::<impl rustc::ty::query::config::QueryAccessors<'tcx> for rustc::ty::query::queries::mir_borrowck<'tcx>>::compute
  30: rustc::ty::context::tls::with_context
  31: rustc::dep_graph::graph::DepGraph::with_task_impl
  32: rustc::ty::context::tls::with_related_context
  33: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt<'a, 'gcx, 'tcx>>::force_query_with_job
  34: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt<'a, 'gcx, 'tcx>>::get_query
  35: rustc::ty::query::<impl rustc::ty::context::TyCtxt<'a, 'tcx, 'lcx>>::mir_borrowck
  36: rustc::ty::<impl rustc::ty::context::TyCtxt<'a, 'gcx, 'tcx>>::par_body_owners
  37: rustc::util::common::time
  38: rustc::ty::context::tls::enter_context
  39: <std::thread::local::LocalKey<T>>::with
  40: rustc::ty::context::TyCtxt::create_and_enter
  41: rustc_driver::driver::compile_input
  42: rustc_driver::run_compiler_with_pool
  43: <scoped_tls::ScopedKey<T>>::set
  44: syntax::with_globals
  45: <std::panic::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
  46: __rust_maybe_catch_panic
             at libpanic_unwind/lib.rs:105
  47: rustc_driver::run
  48: rustc_driver::main
  49: std::rt::lang_start::{{closure}}
  50: std::panicking::try::do_call
             at libstd/rt.rs:59
             at libstd/panicking.rs:310
  51: __rust_maybe_catch_panic
             at libpanic_unwind/lib.rs:105
  52: std::rt::lang_start_internal
             at libstd/panicking.rs:289
             at libstd/panic.rs:397
             at libstd/rt.rs:58
  53: main
  54: __libc_start_main
  55: <unknown>
query stack during panic:
#0 [mir_borrowck] processing `sadness::{{closure}}`
#1 [mir_borrowck] processing `sadness`
end of query stack

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.28.0-nightly (2a1c4eec4 2018-06-25) running on x86_64-unknown-linux-gnu

note: compiler flags: -C debuginfo=2 -C incremental -C target-cpu=native --crate-type lib

note: some of the compiler flags provided by cargo are hidden

[nikomatsakis]

OK so the error is arising because of the combination of

• two patterns that match the same name in one arm
  • and are ref mut patterns
• and a guard

For example this is a (more) minimal reproduction:

#![feature(nll)]

fn foo(x: &mut Result<u32, u32>) {
    match x {
        Ok(v) | Err(v) if *v > 0 => { }
        _ => { }
    }
}

fn main() { }

[nikomatsakis]

The problem seems to be that, during the match desugaring, we re-use the temporaries for the Ok and Err variants. that is, we create MIR like this:

    bb9: {
        _5 = &'_#4r mut (((*_1) as Ok).0: u32); // bb9[2]: scope 0 at /home/nmatsakis/tmp/issue-51348.rs:5:12: 5:13
        _6 = &'_#5r _5;                  // bb9[3]: scope 0 at /home/nmatsakis/tmp/issue-51348.rs:5:12: 5:13
        ...
    }

    ...

    bb12: {
        _5 = &'_#7r mut (((*_1) as Err).0: u32); // bb12[2]: scope 0 at /home/nmatsakis/tmp/issue-51348.rs:5:21: 5:22
        _6 = &'_#8r _5;                  // bb12[3]: scope 0 at /home/nmatsakis/tmp/issue-51348.rs:5:21: 5:22
        ...
    }

but we also mark the _5 borrow as two-phase. We're not supposed to mark any borrows as two-phase that write into temporaries that are also used other places... but in this case we sort of want to... this may be easy to fix, but I'm not sure yet.

Probably we could make a more complex def'n of "two-phaseness" -- that is, the two borrows here use the same temporary, but they have disjoint activation periods, and they cannot reach one another, so we could probably engineer something that permits this MIR as is to work. Alternatively, maybe we can lower via distinct temporaries and that would be fine.

It is crucial though that the first use of the temporary _5 be &_5, since that is what makes the borrowing work out (this particular temporary will only be used in the guard).

[nikomatsakis]

So this ICE doesn't occur on master, therefore I'm going to defer it, but the code is still kinda wrong. I may try to fix it in "spare time". I haven't yet finishd my essay series on the best fix yet, so I could start there. :) wrong issue =)