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

async/await regression regarding borrows across yields #65436

Closed
valff opened this issue Oct 15, 2019 · 9 comments · Fixed by #68269
Closed

async/await regression regarding borrows across yields #65436

valff opened this issue Oct 15, 2019 · 9 comments · Fixed by #68269
Assignees
Labels
A-async-await Area: Async & Await A-diagnostics Area: Messages for errors, warnings, and lints AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@valff
Copy link
Contributor

valff commented Oct 15, 2019

The following code was working in nightly-2019-09-05, but no longer works in nightly-2019-10-15. The workaround is not quite intuitive.

struct Foo(*const u8);

unsafe impl Send for Foo {}

async fn bar(_: Foo) {}

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

fn main() {
    assert_send(async {
        // This works in nightly-2019-09-05, but no longer works in nightly-2019-10-15:
        bar(Foo(std::ptr::null())).await;
        // This way it does work in nightly-2019-10-15:
        //let foo = Foo(std::ptr::null());
        //bar(foo).await;
    })
}

(Playground)

Errors:

   Compiling playground v0.0.1 (/playground)
error[E0277]: `*const u8` cannot be sent between threads safely
  --> src/main.rs:10:5
   |
7  | fn assert_send<T: Send>(_: T) {}
   |    -----------    ---- required by this bound in `assert_send`
...
10 |     assert_send(async {
   |     ^^^^^^^^^^^ `*const u8` cannot be sent between threads safely
   |
   = help: within `impl std::future::Future`, the trait `std::marker::Send` is not implemented for `*const u8`
   = note: required because it appears within the type `{fn(Foo) -> impl std::future::Future {bar}, fn(*const u8) -> Foo {Foo}, fn() -> *const u8 {std::ptr::null::<u8>}, *const u8, Foo, impl std::future::Future, impl std::future::Future, ()}`
   = note: required because it appears within the type `[static generator@src/main.rs:10:23: 16:6 {fn(Foo) -> impl std::future::Future {bar}, fn(*const u8) -> Foo {Foo}, fn() -> *const u8 {std::ptr::null::<u8>}, *const u8, Foo, impl std::future::Future, impl std::future::Future, ()}]`
   = note: required because it appears within the type `std::future::GenFuture<[static generator@src/main.rs:10:23: 16:6 {fn(Foo) -> impl std::future::Future {bar}, fn(*const u8) -> Foo {Foo}, fn() -> *const u8 {std::ptr::null::<u8>}, *const u8, Foo, impl std::future::Future, impl std::future::Future, ()}]>`
   = note: required because it appears within the type `impl std::future::Future`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.
error: could not compile `playground`.

To learn more, run the command again with --verbose.

@Alexendoo Alexendoo added A-async-await Area: Async & Await A-diagnostics Area: Messages for errors, warnings, and lints C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 16, 2019
@callym
Copy link

callym commented Oct 21, 2019

I'm also running into this error using Tide and a DB Pool from Diesel

I've managed to get a slightly simpler/different version of the above code - I'd expect this to work because I'd expect the Foo.v() to be finished before the .async starts (I guess it's similar to how pre-NLL we'd expect function calls in arguments to work but sometimes they wouldn't because existing borrows?)

#![feature(optin_builtin_traits)]

struct Foo;

impl Foo {
  fn v(&self) -> () {}
}

impl !Send for Foo {}

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

async fn take<T>(_: T) {}

fn main() {
  // This compiles fine
  assert_send(async {
    let v = Foo.v();

    take(v).await;
  });

  // This used to work but now doesn't
  assert_send(async {
    take(Foo.v()).await;
  });
}

I've managed to track it down to happening somewhere between these - but I'm not sure how to go about figuring this out any further (I've never built the compiler, let alone actually debugged it)

rustc 1.39.0-nightly (0b36e9dea 2019-09-09) - This successfully builds

rustc 1.39.0-nightly (34e82a7b7 2019-09-10) - This fails

https://github.com/rust-lang/rust/compare/0b36e9dea..34e82a7b7

@sfackler
Copy link
Member

This was an intentional change to match the drop order in synchronous code: #64512

@nikomatsakis
Copy link
Contributor

This was an intentional change but these errors also look like they are caused by "overapproximation" of the set of captured values. We are working to improve that; separately, we are also working on improving the diagnostics here.

@nikomatsakis nikomatsakis added the AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. label Oct 22, 2019
@davidtwco
Copy link
Member

Assigning to myself as this is a case I'd hope to tackle in #65345. It isn't currently handled by that PR, as the current diagnostic improvements only work when the error originates from an async fn, not async { }.

@rustbot claim

@yshui
Copy link
Contributor

yshui commented Jan 14, 2020

@davidtwco
Copy link
Member

Unassigning myself since #65345 has landed.

@yshui
Copy link
Contributor

yshui commented Jan 17, 2020

#68269 adds a compiler message to suggest a workaround for this problem. Is it not possible to make the compiler smarter and not borrow unnecessary variables across .await?

@csmoe

@csmoe
Copy link
Member

csmoe commented Jan 17, 2020

@yshui rustc is not that smarter yet, we already have an issue tracking this #68112 , #68269 along with #68212 were trying to lint users with "shorten" workaround(might be lifted once the analysis is improved) in these two "common" cases.

@tmandry
Copy link
Member

tmandry commented Jan 18, 2020

#68269 adds a compiler message to suggest a workaround for this problem. Is it not possible to make the compiler smarter and not borrow unnecessary variables across .await?

It's a matter of having consistent language semantics for async and non-async code. The lang team discussed this and decided the drop order should be consistent (i.e. we shouldn't drop temporaries until the end of the statement, even if the statement contains .await).

@bors bors closed this as completed in 8bf1758 Jan 25, 2020
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-diagnostics Area: Messages for errors, warnings, and lints AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-bug Category: This is a bug. 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.

10 participants