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

Spurious new Send requirement in async block #64477

Closed
dtolnay opened this issue Sep 15, 2019 · 18 comments

Comments

@dtolnay
Copy link
Member

commented Sep 15, 2019

This is minimized from code that was working with nightly-2019-09-10 but broken on nightly-2019-09-11.
0b36e9d...34e82a7

use std::future::Future;
use std::pin::Pin;

fn f<T>(_: &T) -> Pin<Box<dyn Future<Output = ()> + Send>> {
    unimplemented!()
}

pub fn g<T: Sync>(x: &'static T) -> impl Future<Output = ()> + Send {
    async move { f(x).await }
}
error[E0277]: `T` cannot be sent between threads safely
 --> src/main.rs:8:37
  |
8 | pub fn g<T: Sync>(x: &'static T) -> impl Future<Output = ()> + Send {
  |                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `T` cannot be sent between threads safely
  |
  = help: within `impl std::future::Future`, the trait `std::marker::Send` is not implemented for `T`
  = help: consider adding a `where T: std::marker::Send` bound
  = note: required because it appears within the type `for<'r, 's, 't0, 't1> {for<'t2> fn(&'t2 T) -> std::pin::Pin<std::boxed::Box<(dyn std::future::Future<Output = ()> + std::marker::Send + 'static)>> {f::<T>}, &'r T, T, &'s T, std::pin::Pin<std::boxed::Box<(dyn std::future::Future<Output = ()> + std::marker::Send + 't0)>>, std::pin::Pin<std::boxed::Box<(dyn std::future::Future<Output = ()> + std::marker::Send + 't1)>>, ()}`
  = note: required because it appears within the type `[static generator@src/main.rs:9:16: 9:30 x:&T for<'r, 's, 't0, 't1> {for<'t2> fn(&'t2 T) -> std::pin::Pin<std::boxed::Box<(dyn std::future::Future<Output = ()> + std::marker::Send + 'static)>> {f::<T>}, &'r T, T, &'s T, std::pin::Pin<std::boxed::Box<(dyn std::future::Future<Output = ()> + std::marker::Send + 't0)>>, std::pin::Pin<std::boxed::Box<(dyn std::future::Future<Output = ()> + std::marker::Send + 't1)>>, ()}]`
  = note: required because it appears within the type `std::future::GenFuture<[static generator@src/main.rs:9:16: 9:30 x:&T for<'r, 's, 't0, 't1> {for<'t2> fn(&'t2 T) -> std::pin::Pin<std::boxed::Box<(dyn std::future::Future<Output = ()> + std::marker::Send + 'static)>> {f::<T>}, &'r T, T, &'s T, std::pin::Pin<std::boxed::Box<(dyn std::future::Future<Output = ()> + std::marker::Send + 't0)>>, std::pin::Pin<std::boxed::Box<(dyn std::future::Future<Output = ()> + std::marker::Send + 't1)>>, ()}]>`
  = 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

I think the old behavior was correct. No T-by-value exists in this code so T: Send shouldn't come up as a requirement.

Mentioning @davidtwco because #64292 looks relevant in the right commit range.

I am filing this separately from #64391 because this manifests as a type error and not a borrow checker error, though maybe they are the same thing.

dtolnay added a commit to dtolnay/async-trait that referenced this issue Sep 15, 2019
Tests are broken on newer nightlies by rust-lang/rust#64477.
@Nemo157

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

There's a similar issue hitting any usage of format! within an .awaited expression, it creates !Send temporaries that are now live across the await point (playground):

async fn foo(_: String) {
}

fn bar() -> impl Send {
    async move {
        foo(format!("{}:{}", 1, 2)).await;
    }
}
error[E0277]: `*mut (dyn std::ops::Fn() + 'static)` cannot be shared between threads safely
 --> src/lib.rs:4:13
  |
4 | fn bar() -> impl Send {
  |             ^^^^^^^^^ `*mut (dyn std::ops::Fn() + 'static)` cannot be shared between threads safely
  |
  = help: within `core::fmt::Void`, the trait `std::marker::Sync` is not implemented for `*mut (dyn std::ops::Fn() + 'static)`
  = note: required because it appears within the type `std::marker::PhantomData<*mut (dyn std::ops::Fn() + 'static)>`
  = note: required because it appears within the type `core::fmt::Void`
  = note: required because of the requirements on the impl of `std::marker::Send` for `&core::fmt::Void`
  = note: required because it appears within the type `std::fmt::ArgumentV1<'_>`
  = note: required because it appears within the type `[std::fmt::ArgumentV1<'_>; 2]`
  = note: required because it appears within the type `for<'r, 's, 't0, 't1, 't2, 't3, 't4, 't5, 't6, 't7, 't8, 't9, 't10, 't11, 't12, 't13, 't14, 't15, 't16, 't17, 't18, 't19> {fn(std::string::String) -> impl std::future::Future {foo}, for<'t20> fn(std::fmt::Arguments<'t20>) -> 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; 2], &'t2 [&'t3 str; 2], &'t4 [&'t5 str; 2], &'t6 [&'t7 str], i32, &'t8 i32, &'t9 i32, (&'t10 i32, &'t11 i32), [std::fmt::ArgumentV1<'t12>; 2], &'t13 [std::fmt::ArgumentV1<'t14>; 2], &'t15 [std::fmt::ArgumentV1<'t16>; 2], &'t17 [std::fmt::ArgumentV1<'t18>], std::fmt::Arguments<'t19>, std::string::String, impl std::future::Future, ()}`
  = note: required because it appears within the type `[static generator@src/lib.rs:5:16: 7:6 for<'r, 's, 't0, 't1, 't2, 't3, 't4, 't5, 't6, 't7, 't8, 't9, 't10, 't11, 't12, 't13, 't14, 't15, 't16, 't17, 't18, 't19> {fn(std::string::String) -> impl std::future::Future {foo}, for<'t20> fn(std::fmt::Arguments<'t20>) -> 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; 2], &'t2 [&'t3 str; 2], &'t4 [&'t5 str; 2], &'t6 [&'t7 str], i32, &'t8 i32, &'t9 i32, (&'t10 i32, &'t11 i32), [std::fmt::ArgumentV1<'t12>; 2], &'t13 [std::fmt::ArgumentV1<'t14>; 2], &'t15 [std::fmt::ArgumentV1<'t16>; 2], &'t17 [std::fmt::ArgumentV1<'t18>], std::fmt::Arguments<'t19>, std::string::String, impl std::future::Future, ()}]`
  = note: required because it appears within the type `std::future::GenFuture<[static generator@src/lib.rs:5:16: 7:6 for<'r, 's, 't0, 't1, 't2, 't3, 't4, 't5, 't6, 't7, 't8, 't9, 't10, 't11, 't12, 't13, 't14, 't15, 't16, 't17, 't18, 't19> {fn(std::string::String) -> impl std::future::Future {foo}, for<'t20> fn(std::fmt::Arguments<'t20>) -> 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; 2], &'t2 [&'t3 str; 2], &'t4 [&'t5 str; 2], &'t6 [&'t7 str], i32, &'t8 i32, &'t9 i32, (&'t10 i32, &'t11 i32), [std::fmt::ArgumentV1<'t12>; 2], &'t13 [std::fmt::ArgumentV1<'t14>; 2], &'t15 [std::fmt::ArgumentV1<'t16>; 2], &'t17 [std::fmt::ArgumentV1<'t18>], std::fmt::Arguments<'t19>, std::string::String, impl std::future::Future, ()}]>`
  = note: required because it appears within the type `impl std::future::Future`
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

Yeah, this is the same issue. I've written up a long report on the change that caused this and its motivations. The report also discussed whether we should consider revering that change, and what that would mean.

@jonhoo

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2019

Another example of this (if we want more test-cases) is:

use std::cell::RefCell;

fn foo() -> impl Send {
    async {
        let x = RefCell::new(String::new());
        match x {
            ref z => {
                drop(z);
                async move {
                    x
                }.await
            }
        }
    }
}
@dtolnay

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2019

@nikomatsakis -- your report has this under "Longer lifetime for temporaries results in things \"living\" across await that did not used to". In my code at the top, I can't figure out what temporary might be at play. It seems like the only types that exist in the async block before the await are f::<T>, &T, and Pin<Box<dyn Future<Output = ()> + Send>> which all three implement Send. Are we able to tell where the spurious T: Send bound is coming from? That could be a bug that is orthogonal to the await desugaring.

The required-because chain mentions this type:

for<'r, 's, 't0, 't1> {for<'t2> fn(&'t2 T) -> std::pin::Pin<std::boxed::Box<(
dyn std::future::Future<Output = ()> + std::marker::Send + 'static)>> {f::<T>
}, &'r T, T, &'s T, std::pin::Pin<std::boxed::Box<(dyn std::future::Future<Ou
tput = ()> + std::marker::Send + 't0)>>, std::pin::Pin<std::boxed::Box<(dyn s
td::future::Future<Output = ()> + std::marker::Send + 't1)>>, ()}

What's the T temporary in here that is giving rise to a T: Send bound?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2019

(Marking as blocking as we must reach a decision one way or the other.)

@Nemo157

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2019

It could be the temporary from the reborrowing of x, f(x) expands to f(&*x). Although that making it all the way in to the generator type seems odd.

EDIT: This playground shows that it very likely is (since foo which doesn't reborrow is fine, but bar which does fails), it seems like it could be a generator bug that that temporary is involved at all, which exacerbates the change in await desugaring.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2019

@Nemo157 hmm, that is actually not a temporary by the definition I am using here.

@dtolnay good point, perhaps I misdiagnosed.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2019

Let me dig a bit more deeply.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

OK, I think @Nemo157 was correct -- the code which computes which types may be live across yields is over-approximating the result here to include a number of intermediaries, far more than is needed. This includes the value of type *x, despite it not really being a temporary.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

Specifically, this clause seems to be the problematic one:

// Also include the adjusted types, since these can result in MIR locals
for adjustment in self.fcx.tables.borrow().expr_adjustments(expr) {
self.record(adjustment.target, scope, Some(expr), expr.span);
}

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2019

I opened #64584 which addresses @dtolnay's original example. In general, we are known to overapproximate the set of captures, though, so I wouldn't be surprised if we find more cases that are worth looking at.

bors added a commit that referenced this issue Sep 20, 2019
…es, r=eddyb

record fewer adjustment types in generator witnesses, avoid spurious drops in MIR construction

Don't record all intermediate adjustment types -- That's way more than is needed, and winds up recording types that will never appear in MIR.

Note: I'm like 90% sure that this logic is correct, but this stuff is subtle and can be hard to keep straight.  However, the risk of this PR is fairly low -- if we miss types here, I believe the most common outcome is an ICE.

This fixes the original issue cited by #64477, but I'm leaving the issue open for now since there may be other cases we can detect and improve in a targeted way.

r? @Zoxc
@jebrosen

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2019

It looks like @Nemo157's case in #64477 (comment) (with format!()) was not fixed by #64584 after all, with the same error occurring in the 2019-09-20 nightly.

@bstrie bstrie referenced this issue Sep 23, 2019
10 of 11 tasks complete
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

The format! errors are not, afaict, spurious. I created the expanded version of the example, and I see this:

fn bar() -> impl Send {
    async move
        {
            foo(

                ::alloc::fmt::format(::core::fmt::Arguments::new_v1(&["",
                                                                      ":"],
                                                                    &match (&1,
                                                                            &2)
                                                                         {
                                                                         (arg0,
                                                                          arg1)
                                                                         =>
                                                                         [::core::fmt::ArgumentV1::new(arg0,
                                                                                                       ::core::fmt::Display::fmt),
                                                                          ::core::fmt::ArgumentV1::new(arg1,
                                                                                                       ::core::fmt::Display::fmt)],
                                                                     }))).await;
        }
}

To simplify:

foo(Arguments::new(&[...], &[...])).await;

we are making temporaries to store those [...] arrays. Those temporaries live until the end of the innermost enclosing statement, which occurs after the await -- hence they are live across the await. Unfortunately, the format! temporaries include dyn Trait objects that are not Send. So the error is correct. The shorter-term fix would I think be to alter the desugaring of format! so that its intermediate values are Send, I'm afraid.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

I'm going to close this issue for now, as a result.

@jebrosen

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

A bit sad, but understandable. And a glance through #45198 suggests that fixing the format! machinery to preserve Sendness would not be so easy either.

@Nemo157

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2019

A simpler change might be to introduce an extra block inside the format! expansion to reduce the temporaries lifetimes, since they're guaranteed unused after the String is created (playground):

macro_rules! format {
    ($($arg:tt)*) => ({
        let res = $crate::fmt::format($crate::__export::format_args!($($arg)*));
        res
    })
}
@jonhoo

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2019

@Nemo157 did you ever end up filing a PR for that change? This keeps biting me, and fixing it at the root would save a bunch of work. Do you see any drawback with making your proposed change?

jonhoo added a commit to mit-pdos/noria that referenced this issue Sep 27, 2019
Currently semi-blocked on rust-lang/rust#64477.

Or rather, it would take a bunch of work to fix the last error in our
code. Instead, there's a small change to std that would also fix it, so
waiting on that:

rust-lang/rust#64477 (comment)
jonhoo added a commit to jonhoo/rust that referenced this issue Sep 27, 2019
This places the temporaries that `format!` generates to refer to its
arguments (through `&dyn Trait`) in a short-lived scope surrounding just
the invocation of `format!`. This enables `format!` to be used in
generators without the temporaries preventing the generator from being
`Send` (due to `dyn Trait` not being `Sync`).

See rust-lang#64477 for details.
@jonhoo

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2019

@Nemo157 filed it in #64856.

jonhoo added a commit to jonhoo/rust that referenced this issue Sep 27, 2019
This places the temporaries that `format!` generates to refer to its
arguments (through `&dyn Trait`) in a short-lived scope surrounding just
the invocation of `format!`. This enables `format!` to be used in
generators without the temporaries preventing the generator from being
`Send` (due to `dyn Trait` not being `Sync`).

See rust-lang#64477 for details.
bors added a commit that referenced this issue Sep 28, 2019
Scope format! temporaries

This places the temporaries that `format!` generates to refer to its arguments (through `&dyn Trait`) in a short-lived scope surrounding just the invocation of `format!`. This enables `format!` to be used in generators without the temporaries preventing the generator from being `Send` (due to `dyn Trait` not being `Sync`).

See #64477 for details.
jonhoo added a commit to tokio-rs/tokio that referenced this issue Sep 30, 2019
jonhoo added a commit to tokio-rs/tokio that referenced this issue Sep 30, 2019
`foo(format!(...)).await` no longer compiles. There's a fix in
rust-lang/rust#64856, but this works around the problem.
jonhoo added a commit to tokio-rs/tokio that referenced this issue Sep 30, 2019
`foo(format!(...)).await` no longer compiles. There's a fix in
rust-lang/rust#64856, but this works around the problem.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.