Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Broken MIR in futures generator (async/await) #61442

Closed
Bunogi opened this issue Jun 1, 2019 · 8 comments · Fixed by #61572
Closed

Broken MIR in futures generator (async/await) #61442

Bunogi opened this issue Jun 1, 2019 · 8 comments · Fixed by #61572
Labels
A-async-await Area: Async & Await A-coroutines Area: Coroutines A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html AsyncAwait-Polish Async-await issues that are part of the "polish" area C-bug Category: This is a bug. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Bunogi
Copy link

Bunogi commented Jun 1, 2019

Issue

The code below causes an internal compiler error. I've provided some more details as comments.

error: internal compiler error: src/librustc_mir/transform/generator.rs:532: Broken MIR: generator contains type &mut std::string::String in MIR, but typeck only knows about for<'r, 's, 't0, 't1, 't2, 't3, 't4, 't5, 't6, 't7> {Update, u64, std::string::String, for<'t8> fn(std::fmt::Arguments<'t8>) -> std::string::String {std::fmt::format}, fn(&'r [&'r str], &'r [std::fmt::ArgumentV1<'r>]) -> std::fmt::Arguments<'r> {std::fmt::Arguments::<'r>::new_v1}, &'s str, str, &'t0 str, [&'t1 str; 1], &'t2 [&'t3 str; 1], &'t4 [&'t5 str; 1], &'t6 [&'t7 str], impl core::future::future::Future, ()}
  --> src/main.rs:32:41
   |
32 |   async fn perform_update(update: Update) {
   |  _________________________________________^
33 | |     if let Update::User(id) = update {
34 | |         let mut reply = String::new();
35 | |         reply += &format!("{}", get_user(id).await);
36 | |     }
37 | | }
   | |_^

Code

#![feature(async_await)]

use std::fmt;

#[derive(Default)]
struct User {
    first_name: String,
    last_name: Option<String>,
    username: Option<String>,
}

impl fmt::Display for User {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        if let Some(u) = &self.username {
            write!(f, "{}", u)
        } else if let Some(last) = &self.last_name {
            write!(f, "{} {}", self.first_name, last)
        } else {
            write!(f, "{}", self.first_name)
        }
    }
}

async fn get_user(id: u64) -> User {
    unimplemented!()
}

enum Update {
    User(u64),
}

async fn perform_update(update: Update) {
    if let Update::User(id) = update {
        let mut reply = String::new();
        //If `reply` is assigned directly to this value, the code compiles.
        reply += &format!("{}", get_user(id).await);
    }
}

fn main() {
    //Compiles when this line is omitted, using futures-preview 0.3.0-alpha.16
    futures::executor::block_on(perform_update(Update::User(Default::default())));
}

Meta

rustc 1.37.0-nightly (7840a0b 2019-05-31)
binary: rustc
commit-hash: 7840a0b
commit-date: 2019-05-31
host: x86_64-unknown-linux-gnu
release: 1.37.0-nightly
LLVM version: 8.0

Backtrace:

thread 'rustc' panicked at 'Box<Any>', src/librustc_errors/lib.rs:572:9
stack backtrace:
   0: backtrace::backtrace::libunwind::trace
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.25/src/backtrace/libunwind.rs:97
   1: backtrace::backtrace::trace_unsynchronized
             at /cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-0.3.25/src/backtrace/mod.rs:66
   2: std::sys_common::backtrace::_print
             at src/libstd/sys_common/backtrace.rs:47
   3: std::sys_common::backtrace::print
             at src/libstd/sys_common/backtrace.rs:36
   4: std::panicking::default_hook::{{closure}}
             at src/libstd/panicking.rs:197
   5: std::panicking::default_hook
             at src/libstd/panicking.rs:211
   6: rustc::util::common::panic_hook
   7: std::panicking::rust_panic_with_hook
             at src/libstd/panicking.rs:478
   8: std::panicking::begin_panic
   9: rustc_errors::Handler::span_bug
  10: rustc::util::bug::opt_span_bug_fmt::{{closure}}
  11: rustc::ty::context::tls::with_opt::{{closure}}
  12: rustc::ty::context::tls::with_context_opt
  13: rustc::ty::context::tls::with_opt
  14: rustc::util::bug::opt_span_bug_fmt
  15: rustc::util::bug::span_bug_fmt
  16: <rustc_mir::transform::generator::StateTransform as rustc_mir::transform::MirPass>::run_pass
  17: rustc_mir::transform::run_passes::{{closure}}
  18: rustc_mir::transform::run_passes
  19: rustc_mir::transform::optimized_mir
  20: rustc::ty::query::<impl rustc::ty::query::config::QueryAccessors for rustc::ty::query::queries::optimized_mir>::compute
  21: rustc::dep_graph::graph::DepGraph::with_task_impl
  22: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt>::get_query
  23: rustc::ty::layout::LayoutCx<rustc::ty::context::TyCtxt>::layout_raw_uncached
  24: rustc::ty::layout::layout_raw
  25: rustc::ty::query::__query_compute::layout_raw
  26: rustc::ty::query::<impl rustc::ty::query::config::QueryAccessors for rustc::ty::query::queries::layout_raw>::compute
  27: rustc::dep_graph::graph::DepGraph::with_task_impl
  28: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt>::get_query
  29: <rustc::ty::layout::LayoutCx<rustc::ty::context::TyCtxt> as rustc_target::abi::LayoutOf>::layout_of
  30: <rustc_mir::transform::const_prop::ConstPropagator as rustc::mir::visit::MutVisitor>::visit_statement
  31: <rustc_mir::transform::const_prop::ConstProp as rustc_mir::transform::MirPass>::run_pass
  32: rustc_mir::transform::run_passes::{{closure}}
  33: rustc_mir::transform::run_passes
  34: rustc_mir::transform::optimized_mir
  35: rustc::ty::query::<impl rustc::ty::query::config::QueryAccessors for rustc::ty::query::queries::optimized_mir>::compute
  36: rustc::dep_graph::graph::DepGraph::with_task_impl
  37: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt>::get_query
  38: rustc_mir::monomorphize::collector::collect_items_rec
  39: rustc_mir::monomorphize::collector::collect_items_rec
  40: rustc_mir::monomorphize::collector::collect_crate_mono_items::{{closure}}
  41: rustc::util::common::time
  42: rustc_mir::monomorphize::collector::collect_crate_mono_items
  43: rustc::util::common::time
  44: rustc_mir::monomorphize::partitioning::collect_and_partition_mono_items
  45: rustc::ty::query::__query_compute::collect_and_partition_mono_items
  46: rustc::ty::query::<impl rustc::ty::query::config::QueryAccessors for rustc::ty::query::queries::collect_and_partition_mono_items>::compute
  47: rustc::dep_graph::graph::DepGraph::with_task_impl
  48: rustc::ty::query::plumbing::<impl rustc::ty::context::TyCtxt>::get_query
  49: rustc_codegen_ssa::base::codegen_crate
  50: <rustc_codegen_llvm::LlvmCodegenBackend as rustc_codegen_utils::codegen_backend::CodegenBackend>::codegen_crate
  51: rustc::util::common::time
  52: rustc_interface::passes::start_codegen
  53: rustc::ty::context::tls::enter_global
  54: rustc_interface::passes::BoxedGlobalCtxt::access::{{closure}}
  55: rustc_interface::passes::create_global_ctxt::{{closure}}
  56: rustc_interface::passes::BoxedGlobalCtxt::enter
  57: rustc_interface::queries::<impl rustc_interface::interface::Compiler>::ongoing_codegen
  58: rustc_interface::interface::run_compiler_in_existing_thread_pool
  59: std::thread::local::LocalKey<T>::with
  60: scoped_tls::ScopedKey<T>::set
  61: syntax::with_globals
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
query stack during panic:
#0 [optimized_mir] processing `perform_update::{{closure}}#0`
#1 [layout_raw] computing layout of `[static generator@src/main.rs:32:41: 37:2 update:Update for<'r, 's, 't0, 't1, 't2, 't3, 't4, 't5, 't6, 't7> {Update, u64, std::string::String, for<'t8> fn(std::fmt::Arguments<'t8>) -> std::string::String {std::fmt::format}, fn(&'r [&'r str], &'r [std::fmt::ArgumentV1<'r>]) -> std::fmt::Arguments<'r> {std::fmt::Arguments::<'r>::new_v1}, &'s str, str, &'t0 str, [&'t1 str; 1], &'t2 [&'t3 str; 1], &'t4 [&'t5 str; 1], &'t6 [&'t7 str], std::future::GenFuture<[static generator@src/main.rs:24:36: 26:2 id:u64 {}]>, ()}]`
#2 [optimized_mir] processing `perform_update`
#3 [collect_and_partition_mono_items] collect_and_partition_mono_items
end of query stack
@jonas-schievink jonas-schievink added A-async-await Area: Async & Await A-coroutines Area: Coroutines A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-bug Category: This is a bug. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 1, 2019
@JohnTitor
Copy link
Member

Does this issue still occur? I couldn't reproduce it from given code on playground.

@Bunogi
Copy link
Author

Bunogi commented Jun 3, 2019

I just checked with nightly 2019-06-03, and it's still there. It requires the block_on line to occur.

@matthewjasper
Copy link
Contributor

minimized:

#![feature(generators)]

fn foo() {
    let x = static || {
        let mut s = String::new();
        s += { yield; "" };
    };
}

fn main() {
    foo()
}

@nikomatsakis nikomatsakis added the AsyncAwait-Polish Async-await issues that are part of the "polish" area label Jun 4, 2019
@nikomatsakis
Copy link
Contributor

Marking as blocking -- not entirely clear how general this is but it looks like something people might run across fairly easily.

@nikomatsakis
Copy link
Contributor

cc @Zoxc -- maybe you have some idea what the problem is here?

@Zoxc
Copy link
Contributor

Zoxc commented Jun 4, 2019

My guess is that s += ... implicitly borrows s during the type checking phase, so we don't record &mut String as being part of the generator.

@Aaron1011
Copy link
Member

This seems to be related to the expr_count field calculated here. When s += { yield (); "" }, a computed count of 9 gets compared against a yield expr_count of 5, resulting in live_across_yield being set to None.

However,I'm seeing this line earlier in the logs:

rustc::middle::region: resolve_expr post-increment 9, expr = expr(HirId { owner: DefIndex(12), local_id: 20 }: s += { yield (); "" })

which has a count of 9, as expected. This would seem to suggest that the count is somehow getting overwritten later on.

@Aaron1011
Copy link
Member

On further inspection, the count doesn't appear to be getting overwritten. InteriorVisitor and ExprLocatorVisitor are both computing it in the same way, but it seems inconsistent with the MIR that ultimately gets generated.

I think that due to the way the HIR post-order traversal works, the overall assignment expression s += { yield (); "" } is computed to be 'after' the inner yield ();, which is interpreted to mean that it isn't live over the yield point.

However, consider the MIR for a similar function:

fn main() {
    let mut a = String::new();
    a += { let b = "Hello"; b };
}

The relevant MIR is;

   bb2: {
        StorageLive(_3);                 // bb2[0]: scope 1 at src/main.rs:16:5: 16:6
        _3 = &mut _1;                    // bb2[1]: scope 1 at src/main.rs:16:5: 16:6
        StorageLive(_4);                 // bb2[2]: scope 1 at src/main.rs:16:10: 16:32
        StorageLive(_5);                 // bb2[3]: scope 1 at src/main.rs:16:16: 16:17
        _5 = const "Hello";              // bb2[4]: scope 1 at src/main.rs:16:20: 16:27
                                         // ty::Const
                                         // + ty: &str
                                         // + val: Slice(Ptr(Pointer { alloc_id: AllocId(0), offset: Size { raw: 0 }, tag: () }), 5)
                                         // mir::Constant
                                         // + span: src/main.rs:16:20: 16:27
                                         // + ty: &str
                                         // + literal: Const { ty: &str, val: Slice(Ptr(Pointer { alloc_id: AllocId(0), offset: Size { raw: 0 }, tag: () }), 5) }
        _4 = _5;                         // bb2[5]: scope 3 at src/main.rs:16:29: 16:30
        StorageDead(_5);                 // bb2[6]: scope 1 at src/main.rs:16:31: 16:32
        _2 = const std::ops::AddAssign::add_assign(move _3, move _4) -> [return: bb3, unwind: bb4]; // bb2[7]: scope 1 at src/main.rs:16:5: 16:32
                                         // ty::Const
                                         // + ty: for<'r> fn(&'r mut std::string::String, &str) {<std::string::String as std::ops::AddAssign<&str>>::add_assign}
                                         // + val: Scalar(Bits { size: 0, bits: 0 })
                                         // mir::Constant
                                         // + span: src/main.rs:16:5: 16:32
                                         // + ty: for<'r> fn(&'r mut std::string::String, &str) {<std::string::String as std::ops::AddAssign<&str>>::add_assign}
                                         // + literal: Const { ty: for<'r> fn(&'r mut std::string::String, &str) {<std::string::String as std::ops::AddAssign<&str>>::add_assign}, val: Scalar(Bits { size: 0, bits: 0 }) }
    }

Here, the mutable borrow adjustment _3 = &mut _1 occurs before the body of the block. When the block includes a yield, the &mut String is clearly live across the yield point.

Aaron1011 added a commit to Aaron1011/rust that referenced this issue Jun 22, 2019
Fixes rust-lang#61442

When rustc::middle::region::ScopeTree ccomputes its yield_in_scope
field, it relies on the HIR visitor order to properly compute which
types must be live across yield points. In order for the computed scopes
to agree with the generated MIR, we must ensure that expressions
evaluated before a yield point are visited before the 'yield'
expression.

However, the visitor order for ExprKind::AssignOp
was incorrect. The left-hand side of a compund assignment expression is
evaluated before the right-hand side, but the right-hand expression was
being visited before the left-hand expression. If the left-hand
expression caused a new type to be introduced (e.g. through a
deref-coercion), the new type would be incorrectly seen as occuring
*after* the yield point, instead of before. This leads to a mismatch
between the computed generator types and the MIR, since the MIR will
correctly see the type as being live across the yield point.

To fix this, we correct the visitor order for ExprKind::AssignOp
to reflect the actual evaulation order.
bors added a commit that referenced this issue Jun 25, 2019
Fix HIR visit order

Fixes #61442

When rustc::middle::region::ScopeTree computes its yield_in_scope
field, it relies on the HIR visitor order to properly compute which
types must be live across yield points. In order for the computed scopes
to agree with the generated MIR, we must ensure that expressions
evaluated before a yield point are visited before the 'yield'
expression.

However, the visitor order for ExprKind::AssignOp
was incorrect. The left-hand side of a compund assignment expression is
evaluated before the right-hand side, but the right-hand expression was
being visited before the left-hand expression. If the left-hand
expression caused a new type to be introduced (e.g. through a
deref-coercion), the new type would be incorrectly seen as occuring
*after* the yield point, instead of before. This leads to a mismatch
between the computed generator types and the MIR, since the MIR will
correctly see the type as being live across the yield point.

To fix this, we correct the visitor order for ExprKind::AssignOp
to reflect the actual evaulation order.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await Area: Async & Await A-coroutines Area: Coroutines A-mir Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html AsyncAwait-Polish Async-await issues that are part of the "polish" area C-bug Category: This is a bug. I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants