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

Explanation for why future is not Send is wrong #68112

Closed
tmandry opened this issue Jan 11, 2020 · 4 comments · Fixed by #70679
Closed

Explanation for why future is not Send is wrong #68112

tmandry opened this issue Jan 11, 2020 · 4 comments · Fixed by #70679
Assignees
Labels
A-async-await A-diagnostics AsyncAwait-Triaged C-bug C-enhancement D-incorrect P-high T-compiler

Comments

@tmandry
Copy link
Contributor

@tmandry tmandry commented Jan 11, 2020

The following code (playground):

fn main() {
    let send_fut = async {
        let non_send_fut = make_non_send_future();
        let _ = non_send_fut.await;
        ready(0).await;
    };
    require_send(send_fut);
}

Gives an error message stating that non_send_fut can live until the end of the scope, and that's why send_fut is not Send:

error: future cannot be sent between threads safely
  --> src/main.rs:18:5
   |
6  | fn require_send(_: impl Send) {}
   |    ------------         ---- required by this bound in `require_send`
...
18 |     require_send(send_fut);
   |     ^^^^^^^^^^^^ future returned by `main` is not `Send`
   |
   = help: the trait `std::marker::Sync` is not implemented for `std::cell::RefCell<i32>`
note: future is not `Send` as this value is used across an await
  --> src/main.rs:16:9
   |
14 |         let non_send_fut = make_non_send_future();
   |             ------------ has type `impl core::future::future::Future`
15 |         let _ = non_send_fut.await;
16 |         ready(0).await;
   |         ^^^^^^^^^^^^^^ await occurs here, with `non_send_fut` maybe used later
17 |     };
   |     - `non_send_fut` is later dropped here

but it doesn't; it's consumed by value when it gets awaited. The problem is we're awaiting a non-Send future inside of send_fut, which has to be Send.

Also, the text

future returned by `main` is not `Send`

is wrong; main doesn't return anything.

Thanks to @kellerb for the reproducer and @JakeEhrlich for originally reporting this.

@varkor varkor added A-async-await A-diagnostics D-incorrect labels Jan 11, 2020
@tmandry
Copy link
Contributor Author

@tmandry tmandry commented Jan 11, 2020

The problem is we're awaiting a non-Send future inside of send_fut, which has to be Send.

Some version of this is roughly the error message I'd want.

@JohnTitor JohnTitor added C-enhancement T-compiler labels Jan 12, 2020
@tmandry
Copy link
Contributor Author

@tmandry tmandry commented Jan 14, 2020

@rustbot modify labels to +AsyncAwait-OnDeck +AsyncAwait-Triaged

@rustbot rustbot added AsyncAwait-OnDeck AsyncAwait-Triaged labels Jan 14, 2020
@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jan 14, 2020

A few thoughts:

  • I suspect that the code might be incorrectly identifying the await that should be highlighted

If you remove the second await, you get this error (playground):

error: future cannot be sent between threads safely
  --> src/main.rs:17:5
   |
6  | fn require_send(_: impl Send) {}
   |    ------------         ---- required by this bound in `require_send`
...
17 |     require_send(send_fut);
   |     ^^^^^^^^^^^^ future returned by `main` is not `Send`
   |
   = help: the trait `std::marker::Sync` is not implemented for `std::cell::RefCell<i32>`
note: future is not `Send` as this value is used across an await
  --> src/main.rs:15:17
   |
14 |         let non_send_fut = make_non_send_future();
   |             ------------ has type `impl core::future::future::Future`
15 |         let _ = non_send_fut.await;
   |                 ^^^^^^^^^^^^^^^^^^ await occurs here, with `non_send_fut` maybe used later
16 |         //ready(0).await;
17 |     };
   |     - `non_send_fut` is later dropped here

Granted, also wrong, though it might be "correct" in the sense that the compiler thinks that non_send_fut may be used later (because our analysis is not precise).

@csmoe
Copy link
Member

@csmoe csmoe commented Jan 15, 2020

yes, live-across-yield analysis inside generator should be more precise, a test case from src/test/ui:

#![feature(optin_builtin_traits)]
// edition:2018
struct Foo;

impl !Send for Foo {}

fn is_send<T: Send>(t: T) { }

async fn bar() {
    let x = Foo;
    baz().await;
}

async fn baz() { }

fn main() {
    is_send(bar());
}

this snippet will compile if the non-Send Foo's scope is shorten within a block

@tmandry tmandry added this to To do in wg-async work Feb 11, 2020
@tmandry tmandry added C-bug P-high labels Mar 3, 2020
@tmandry tmandry self-assigned this Mar 24, 2020
@tmandry tmandry moved this from To do to In progress in wg-async work Mar 28, 2020
tmandry added a commit to tmandry/rust that referenced this issue Apr 2, 2020
tmandry added a commit to tmandry/rust that referenced this issue Apr 2, 2020
Centril added a commit to Centril/rust that referenced this issue Apr 11, 2020
Improve async-await/generator obligation errors in some cases

Fixes rust-lang#68112.

This change is best read one commit at a time (I add a test at the beginning and update it in each change after).

The `test2` function is a case I found while writing the test that we don't handle with this code yet. I don't attempt to fix it in this PR, but it's a good candidate for future work.

r? @davidtwco, @nikomatsakis
tmandry added a commit to tmandry/rust that referenced this issue Apr 14, 2020
wg-async work automation moved this from In progress to Done Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await A-diagnostics AsyncAwait-Triaged C-bug C-enhancement D-incorrect P-high T-compiler
Projects
Development

Successfully merging a pull request may close this issue.

6 participants