async/await: awaiting inside a match block captures borrow too eagerly

[seanmonstar]

If you use await!(some_fut) inside an arm of a match X, the generated future eagerly borrows the value of X, if it not needed.

This may not usually be noticeable, but the issue compounds when the type X contains a trait object, and the future you wish to return is impl Future + Send. This causes a misleading error message that "dyn Trait + Send cannot be shared between threads", which is required to for &X: Send.

Example

Here's a simple struct with a trait object:

struct Client(Box<Any + Send>);

Consider a function like this:

impl Client {
    fn status(&self) -> u16 {
        200
    }
}

You could consider using a match to determine what kind of future to await (or what arguments to pass):

async fn get() {
}

pub fn wat() -> impl Future + Send {
    let client = Client(Box::new(true));
    async move {
        match client.status() {
            200 => {
                let _x = await!(get());
            },
            _ => (),
        }
    }
}

If the await is moved out of the match block, all is well:

pub fn ok() -> impl Future + Send {
    let client = Client(Box::new(true));
    async move {
        if client.status() == 200 {
            let _x = await!(get());
        }
    }
}

The wat function causes this compilation error:

error[E0277]: `(dyn std::any::Any + std::marker::Send + 'static)` cannot be shared between threads safely
  --> src/main.rs:21:17
   |
21 | pub fn wat() -> impl Future + Send {
   |                 ^^^^^^^^^^^^^^^^^^ `(dyn std::any::Any + std::marker::Send + 'static)` cannot be shared between threads safely
   |
   = help: the trait `std::marker::Sync` is not implemented for `(dyn std::any::Any + std::marker::Send + 'static)`
   = note: required because of the requirements on the impl of `std::marker::Sync` for `std::ptr::Unique<(dyn std::any::Any + std::marker::Send + 'static)>`
   = note: required because it appears within the type `std::boxed::Box<(dyn std::any::Any + std::marker::Send + 'static)>`
   = note: required because it appears within the type `Client`
   = note: required because of the requirements on the impl of `for<'r> std::marker::Send` for `&Client`
   = note: required because it appears within the type `for<'r> {Client, &'r Client, u16, impl std::future::Future, ()}`
   = note: required because it appears within the type `[static generator@src/main.rs:23:16: 30:6 client:Client for<'r> {Client, &'r Client, u16, impl std::future::Future, ()}]`
   = note: required because it appears within the type `std::future::GenFuture<[static generator@src/main.rs:23:16: 30:6 client:Client for<'r> {Client, &'r Client, u16, impl std::future::Future, ()}]>`
   = note: required because it appears within the type `impl std::future::Future`
   = note: the return type of a function must have a statically known size

Playground: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=2a9dbea32d31457d50d40b99c52ee214 (updated to latest syntax -Niko)

[cramertj]

This is just an artifact of the compiler holding the borrow of client in your match expression for the duration of the match condition-- there's nothing async/await!-specific going on here. There's a potential fix to be made to eliminate the borrow of client sooner, but that's a general MIR representation issue.

[cramertj]

("liveness" is really the tag I want, but we don't have one for that ;) )

[jonas-schievink]

This might be a duplicate of https://github.com/rust-lang/rust/issues/46525?

[seanmonstar]

I'm not certain it's only that; it does seem related to await!. The compiler seems to be fine with give up the borrow if I call a self or &mut self method inside the match.

[nikomatsakis]

Marking this as deferred -- the problem here (discussed here on Zulip) has to do with how we use an "overapproximation" to decide what data may be inside a generator, at least initially. This is based on the HIR. Only later do we get more precise results when generating MIR.

I think we could resolve this by making that over-appoximation more precise, but @Zoxc expressed a desire to do away with it altogether. This would obviously be better, if we can make it work, though I'm a bit skeptical. (See the Zulip topic for more details.)

[nikomatsakis]

I'm adding the AsyncAwait-Unclear label, because I'd like to revisit this question and try to make some progress here before we stabilize, but I'm not willing to call it blocking yet =)

[sdroege]

Another instance of this from https://github.com/rust-lang/rust/issues/61211. Doesn't only affect what is borrowed there but also the trait bounds of the generated future.

    let foo = Mutex::new("123");

    let foo: Box<dyn futures::future::Future<Output = ()> + Send> = Box::new(async move {
        // In a separate block works
        // {
        let bar = foo.lock().unwrap();
        drop(bar);
        // }
        futures::future::lazy(|_| ()).await;
    });

Fails with

error[E0277]: `std::sync::MutexGuard<'_, &str>` cannot be sent between threads safely

[Mark-Simulacrum]

FWIW it took me ~30 minutes or so to decide that this was likely a compiler "bug" with async/await, after spending 10-15 minutes reducing the amount of code I had added to just one match statement. The errors are long -- and largely unintelligible. I'm not sure if I'd call this a blocker, but I would say that if it's not fixed, we should definitely include it in a FAQ section of "common errors" or something along those lines.

Here's a gist of the error. There is no clear mention of the actual source of the error, which is in a different crate (this snippet points at main.rs, which is a thin shim over the larger crate rooted at main.rs) that compiled fine(!) -- that means that this might be introduced accidentally in a library and not discovered until a client attempts to use said function in a manner requiring Sync/Send, for example.

[inv2004]

@Mark-Simulacrum Is it the same problem?

https://gist.github.com/inv2004/9593c35216ee7a94268eb39f47ecaa52

[Mark-Simulacrum]

My understanding is yes; but I can't be certain.

[nikomatsakis]

@rustbot claim

I'm assigning this to myself not to necessarily fix but to organize a call where we can dig a bit into what it would take to fix this, especially if we can move the MIR construction earlier.

[nikomatsakis]

I tried an experiment today: in the trait select code, I inserted a call to the mir_built query at the point where we expand, for an auto-trait, generator types to get at their contents. Basically right before the let witness = ... line here:

https://github.com/rust-lang/rust/blob/e59dab52d48f628ae169033da80b169e5b6f39d6/src/librustc/traits/select.rs#L2763-L2770

This "mostly works" but did cause some errors. One example is the regression test for #64477:

https://github.com/rust-lang/rust/blob/e59dab52d48f628ae169033da80b169e5b6f39d6/src/test/ui/async-await/issues/issue-64477.rs#L9-L20

which now errors out with a cycle error:

query stack during panic:
#0 [mir_built] processing `bar::{{closure}}#0`
#1 [typeck_tables_of] processing `bar`
#2 [typeck_tables_of] processing `bar::{{closure}}#0`
#3 [type_of] processing `bar::{{closure}}#0`
#4 [collect_mod_item_types] collecting item types in top-level module
#5 [analysis] running analysis passes on this crate

I am now sort of investigating this cycle in a bit more detail. For example, the type_of query for generators requests the full "typeck tables" for the generator:

https://github.com/rust-lang/rust/blob/e59dab52d48f628ae169033da80b169e5b6f39d6/src/librustc_typeck/collect.rs#L1357-L1367

However, in contrast, the handling for closures is much simpler (and I think more appropriate). It seems likely that we could refactor this to be more similar to closures -- basically creating a generator type like Generator<DefId, W> where W is a formal type parameter representing the "witness". This would certainly allow the type_of query to terminate.

However, we are still going to have to construct the typeck_tables_of at some point. So the real problem here is that typeck_tables_of(bar::generator) for the generator invokes typeck_tables_of(bar) for the base type which then, in turn, attempts to solve some trait and triggers the MIR construction query. What's going on there?

Presumably the reason that typeck_tables_of(bar) winds up requesting the generators types is that it is trying to prove that its hidden return type (which involves the generator) implements Send (i.e., that it meets its impl Send bound).

So why is typeck_tables_of(bar::generator) invoking typeck_tables_of? Well, this is because the typeck-tables for a closure and its container are created together:

https://github.com/rust-lang/rust/blob/e59dab52d48f628ae169033da80b169e5b6f39d6/src/librustc_typeck/check/mod.rs#L859-L865

This happens because of the limitations around inference; the two are inferred in a coherent unit. So the full type of something in the generator can be inferred as a result of the surrounding function. You can see that in this example (playground):

use std::fmt::Debug;

fn is_debug<T: Debug>(_: T) { }

fn main() {
    let mut v: Option<_> = None;

    let generator = async move {
        is_debug(v);
    };

    if false {
        // Commenting out this line will yield an error, because the type
        // of `v` is not specified.
        v = Some(String::new());
    }

    drop(generator);
}

So my conclusion from this is that we can't "just" construct the MIR from the generator and use it to inform its constituent types. It's going to take a bit more cleverness.

Somewhat annoyingly, we probably also can't remove the checking of bounds from type-checking the function, although we might be able to do something. Right now, when we start type-checking bar, we register some obligations for the hidden type $H (a type variable) like $H: Send. After all, we want to check that the hidden-type is Send. It is these obligations that are causing the cycle problem. You'd almost think we could defer them until later (i.e., finish generating the types and then check them), except that in some cases the search for impls could influence the type checker.

Consider this example (which compiles today):

struct Foo<T> { t: Option<T> }

unsafe impl Send for Foo<u32> { }

fn foo() -> impl Send {
    Foo { t: None }
}

fn main() { }

Here, the hidden type $H is Foo<$T> where $T is basically unconstrained. Forcing $H: Send results in $T = u32.

Anyway, not saying we can't construct the MIR and therefore get precise results, but it's going to take a bit of careful thought to untangle these dependencies.

[Aaron1011]

@nikomatsakis: That last example is extremely surprising to me - the compiler is essentially pulling a type out of thin air. I think this can only occur when there is exactly one possible impl - if I add unsafe impl Send for Foo<bool>, I get the expected inference error.

I think it would be possible to divide up the generated bounds into two categories:

1. 'Normal' bounds (those not involving a generator type).
2. 'Generator' bounds (e.g. <generator_type>: Send)

'Generator' bounds will have deferred checking as you described, while 'normal' bounds will still be checked during type checking (to preserve this odd inference behavior).

I believe the fact that generator types are unnameable prevents this from having an effect on type inference. If I understand correctly, this kind of inference occurs when we unify a type variable (i.e $T) with a type from an applicable impl block when checking SomeType<$T>: Send. However, generators don't have (normal) generics, so it's impossible for checking such a predicate to ever cause a type variable to be unified.

In general, I think we could safely defer checking any predicate of the form SomeType: SomeTrait, where SomeType and SomeTrait have no inference variables. However, I don't think there's any advantage to doing this for types other than generators, since the entire point of doing this is to generate generator MIR early on.

[Aaron1011]

I'm working on this, and will have a PR open soon.

[Aaron1011]

@rustbot claim

[Aaron1011]

@nikomatsakis: Oops - didn't mean to unassign you.

[eholk]

I'm looking into this a little now. I noticed there was a difference between the match and if versions of the wat and ok functions and decided to try if let:

pub fn wat() -> impl Future + Send {
    let client = Client(Box::new(true));
    async move {
        if let 200 = client.status() {
            get().await;
        }
    }
}

This also fails. Maybe this isn't too informative though, since if let desugars into a match.

[eholk]

@rustbot claim

[eholk]

Here's a shorter test case that illustrates the issue. This one is based around yield, so it avoids some of the extra syntax introduced by desugaring await. It makes the logs a lot easier to follow.

#![feature(generators, generator_trait, negative_impls)]

struct Client;

impl !Sync for Client {}

fn status(_client_status: &Client) -> i16 {
    200
}

fn assert_send<T: Send>(_thing: T) {}

fn main() {
    let g = move || match status(&Client) {
        _status => yield,
    };
    assert_send(g);
}

[estebank]

Current output:

error: future cannot be sent between threads safely
  --> src/main.rs:19:17
   |
19 | pub fn wat() -> impl Future + Send {
   |                 ^^^^^^^^^^^^^^^^^^ future created by async block is not `Send`
   |
   = help: the trait `Sync` is not implemented for `(dyn Any + Send + 'static)`
note: future is not `Send` as this value is used across an await
  --> src/main.rs:24:26
   |
22 |         match client.status() {
   |               ------ has type `&Client` which is not `Send`
23 |             200 => {
24 |                 let _x = get().await;
   |                          ^^^^^^^^^^^ await occurs here, with `client` maybe used later
...
28 |     }
   |     - `client` is later dropped here
help: consider moving this into a `let` binding to create a shorter lived borrow
  --> src/main.rs:22:15
   |
22 |         match client.status() {
   |               ^^^^^^^^^^^^^^^

Following the suggestion solves the problem with the code, but the match block borrowing should still support the original code.

[nikomatsakis]

So excited this is closed! Nice work @eholk

[eholk]

Ugh, so it looks like this isn't actually fixed yet. Playground

use std::any::Any;
use std::future::Future;

struct Client(Box<dyn Any + Send>);

impl Client {
    fn status(&self) -> u16 {
        200
    }
}

async fn get() {
}

pub fn wat() -> impl Future + Send {
    let client = Client(Box::new(true));
    async move {
        match client.status() {
            200 => {
                let _x = get().await;
            },
            _ => (),
        }
    }
}

Output:

   Compiling playground v0.0.1 (/playground)
error: future cannot be sent between threads safely
  --> src/lib.rs:15:17
   |
15 | pub fn wat() -> impl Future + Send {
   |                 ^^^^^^^^^^^^^^^^^^ future created by async block is not `Send`
   |
   = help: the trait `Sync` is not implemented for `(dyn Any + Send + 'static)`
note: future is not `Send` as this value is used across an await
  --> src/lib.rs:20:31
   |
18 |         match client.status() {
   |               ------ has type `&Client` which is not `Send`
19 |             200 => {
20 |                 let _x = get().await;
   |                               ^^^^^^ await occurs here, with `client` maybe used later
...
24 |     }
   |     - `client` is later dropped here
help: consider moving this into a `let` binding to create a shorter lived borrow
  --> src/lib.rs:18:15
   |
18 |         match client.status() {
   |               ^^^^^^^^^^^^^^^

error: could not compile `playground` due to previous error

[danielhenrymantilla]

I'm a bit confused by what is the part considered problematic and deemed fix-worty.

match / if let semantics state that temporaries be dropped / "go out of scope" at the end of the match / if let block.

So match client.status() {… becomes:

{ let temporary = &client;
    match temporary.status() { 200 => {
        …
    }}
} // <- temporary dropped here

which can be further simplified down to:

let temporary = &client;
if temporary.status() == 200 {
    … /* stuff, such as a yield or await point */
}

So the problem in this issue becomes that of temporary above crossing a yield/.await. This, thus, by definition, requires that they be captured by the generator, but since this is just an "automatic out-of-scope" operation, it is semantically meaningless, according to NLL:

• Incidentally, this NLL observation is what allows, in both this simplification, and the original match code, for other borrows to occur, or even for the borrowee to be moved / dropped. The temporary binding, technically StorageLive-allocated in the "stack", ends up "stack"-deallocated / StorageDead, and yet this does not require the borrow to still be usable at that point.

So I guess the whole issue is about "making generator captures NLL-aware", of sorts?

[eholk]

So I guess the whole issue is about "making generator captures NLL-aware", of sorts?

I think that's a good way of looking at it.

As you point out, @danielhenrymantilla, technically temporarily does live across the await point. But users find this surprising and it's not observable (other than in the generator type) if we end the borrow earlier, so we'd like to have rules that allow this program to work.

[jyn514]

This will be fixed by -Zdrop-tracking, I think. https://github.com/rust-lang/rust/issues/97331