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

jonhoo · 2018-06-04T17:15:33Z

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 · 2018-06-04T18:15:22Z

Was this working with a previous nightly?

jonhoo · 2018-06-04T18:24:20Z

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 · 2018-06-06T16:17:43Z

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

jonhoo · 2018-06-06T16:22:38Z

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

jonhoo · 2018-06-26T17:30:40Z

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 · 2018-06-27T17:15:34Z

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 · 2018-06-27T17:23:28Z

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 · 2018-06-29T20:11:24Z

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 =)

nikomatsakis · 2018-06-29T20:11:39Z

Certainly not an Edition Preview blocker though.

nikomatsakis · 2018-06-29T20:22:18Z

OK, so, I still think this is not an EP blocker, but for a different reason:

Basically, the situation is obscure enough. But I would consider it an RC blocker.

nikomatsakis · 2018-07-23T18:43:31Z

Nominating for reconsideration ... should we try to tackle this for EP2? (cc @rust-lang/wg-compiler-nll)

pnkfelix · 2018-07-25T11:23:03Z

I'll take a shot at this today.

pnkfelix · 2018-07-25T13:55:16Z

Interestingly, I was vaguely worried about making sure we had a test case that covered this scenario:

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

but it turns out (of course?) that doing that is illegal: One of the patterns is non-moving and the other moves, and you cannot do that within a single match arm.

And if you split it into two separate arms, you avoid the ICE in the first place because we don't attempt to leverage the two-phase borrows system in that case.

(it is also illegal to bind-by-move into a pattern guard, but that is a limitation that we seek to remove long term, so i don't intend to depend on that constraint.)

pnkfelix · 2018-07-26T11:05:46Z

I have something plausible. Not sure if we want to actually land it for EP2, but we'll see.

… *for each* candidate pat. This required a bit of plumbing to keep track of candidates. But I took advantage of the hack session to try to improve the docs for the relevant structs here. (I also tried to simplify some of the related code in passing.)

…ate-in-arm, r=nikomatsakis [NLL] make temp for each candidate in `match` arm In NLL, `ref mut` patterns leverage the two-phase borrow infrastructure to allow the shared borrows within a guard before the "activation" of the mutable borrow when we begin execution of the match arm's body. (There is further discussion of this on PR #50783.) To accommodate the restrictions we impose on two-phase borrows (namely that there is a one-to-one mapping between each activation and the original initialization), this PR is making separate temps for each candidate pattern. So in an arm like this: ```rust PatA(_, ref mut ident) | PatB(ref mut ident) | PatC(_, _, ref mut ident) | PatD(ref mut ident) if guard_stuff(ident) => ... ``` instead of 3 temps (two for the guard and one for the arm body), we now have 4 + 2 temps associated with `ident`: one for each candidate plus the actual temp that the guard uses directly, and then the sixth is the temp used in the arm body. Fix #51348

jonhoo changed the title ~~ICE on nightly 2018-06-04~~ ICE on nightly 2018-06-03 Jun 4, 2018

estebank added the I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ label Jun 4, 2018

jonhoo changed the title ~~ICE on nightly 2018-06-03~~ assertion failed: old_value.is_none() on nightly 2018-06-03 Jun 6, 2018

sfackler added the A-NLL Area: Non-lexical lifetimes (NLL) label Jun 26, 2018

nikomatsakis added WG-compiler-nll NLL-complete Working towards the "valid code works" goal labels Jun 26, 2018

jonhoo added a commit to mit-pdos/noria that referenced this issue Jun 27, 2018

Work around rust-lang/rust#51348 ICE

6abe455

nikomatsakis added this to the Rust 2018 Release Candidate milestone Jun 29, 2018

jonhoo mentioned this issue Jul 5, 2018

Bad codegen partitioning with non-incremental compile in release mode #52084

Open

pnkfelix changed the title ~~assertion failed: old_value.is_none() on nightly 2018-06-03~~ two ref mut patterns with same name used in arm guard causes ICE on NLL Jul 6, 2018

lqd mentioned this issue Jul 10, 2018

NLL ICEs from nll-3 crater #52217

Closed

nikomatsakis added the I-nominated label Jul 23, 2018

pnkfelix self-assigned this Jul 25, 2018

pnkfelix mentioned this issue Jul 26, 2018

[NLL] make temp for each candidate in match arm #52733

Merged

bors closed this as completed in #52733 Jul 27, 2018

pnkfelix removed the I-nominated label Aug 28, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jonhoo commented Jun 4, 2018 •

edited by pnkfelix

Loading

pietroalbini commented Jun 4, 2018

jonhoo commented Jun 4, 2018

jonhoo commented Jun 6, 2018 •

edited

Loading

jonhoo commented Jun 6, 2018

jonhoo commented Jun 26, 2018

nikomatsakis commented Jun 27, 2018 •

edited

Loading

nikomatsakis commented Jun 27, 2018

nikomatsakis commented Jun 29, 2018 •

edited

Loading

nikomatsakis commented Jun 29, 2018

nikomatsakis commented Jun 29, 2018

nikomatsakis commented Jul 23, 2018

pnkfelix commented Jul 25, 2018

pnkfelix commented Jul 25, 2018 •

edited

Loading

pnkfelix commented Jul 26, 2018

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

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

Comments

jonhoo commented Jun 4, 2018 • edited by pnkfelix Loading

pietroalbini commented Jun 4, 2018

jonhoo commented Jun 4, 2018

jonhoo commented Jun 6, 2018 • edited Loading

jonhoo commented Jun 6, 2018

jonhoo commented Jun 26, 2018

nikomatsakis commented Jun 27, 2018 • edited Loading

nikomatsakis commented Jun 27, 2018

nikomatsakis commented Jun 29, 2018 • edited Loading

nikomatsakis commented Jun 29, 2018

nikomatsakis commented Jun 29, 2018

nikomatsakis commented Jul 23, 2018

pnkfelix commented Jul 25, 2018

pnkfelix commented Jul 25, 2018 • edited Loading

pnkfelix commented Jul 26, 2018

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

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

jonhoo commented Jun 4, 2018 •

edited by pnkfelix

Loading

jonhoo commented Jun 6, 2018 •

edited

Loading

nikomatsakis commented Jun 27, 2018 •

edited

Loading

nikomatsakis commented Jun 29, 2018 •

edited

Loading

pnkfelix commented Jul 25, 2018 •

edited

Loading