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

note_obligation_cause reporting for non-async/await not optimal #70818

Closed
rkuhn opened this issue Apr 5, 2020 · 3 comments · Fixed by #71923
Closed

note_obligation_cause reporting for non-async/await not optimal #70818

rkuhn opened this issue Apr 5, 2020 · 3 comments · Fixed by #71923
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-enhancement Category: An issue proposing an enhancement or a PR with one. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@rkuhn
Copy link
Contributor

rkuhn commented Apr 5, 2020

Consider the following code:

fn d<T: Sized>(t: T) -> impl std::future::Future<Output = T> + Send {
    async { t }
}

This produces the following error message in cargo build:

error[E0277]: `T` cannot be sent between threads safely
  --> src/main.rs:27:25
   |
27 | fn d<T: Sized>(t: T) -> impl std::future::Future<Output = T> + Send {
   |      --                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `T` cannot be sent between threads safely
   |      |
   |      help: consider further restricting this bound: `T: std::marker::Send +`
28 |     async { t }
   |     ----------- this returned value is of type `impl std::future::Future`
   |
   = help: within `impl std::future::Future`, the trait `std::marker::Send` is not implemented for `T`
   = note: required because it appears within the type `[static generator@src/main.rs:28:11: 28:16 t:T _]`
   = note: required because it appears within the type `std::future::GenFuture<[static generator@src/main.rs:28:11: 28:16 t:T _]>`
   = 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

and the following tooltip in VS Code:

`T` cannot be sent between threads safely

`T` cannot be sent between threads safely

help: within `impl std::future::Future`, the trait `std::marker::Send` is not implemented for `T`
note: required because it appears within the type `[static generator@src/main.rs:28:11: 28:16 t:T _]`
note: required because it appears within the type `std::future::GenFuture<[static generator@src/main.rs:28:11: 28:16 t:T _]>`
note: required because it appears within the type `impl std::future::Future`
note: the return type of a function must have a statically known sizerustc(E0277)
future.rs(21, 58): within this `impl std::future::Future`
main.rs(27, 18): `T` cannot be sent between threads safely
main.rs(28, 5): this returned value is of type `impl std::future::Future`

@eddyb asked me to open this issue because the notes are mostly confusing (at least they won’t help mere mortals) and said that the message should not mention GenFuture nor [static generator ...]. He used the following code for a larger error message (playground):

async fn foo<T>(x: T) -> T {
    x
}

fn assert_send(_: impl Send) {}

fn test<T>(x: T) {
    assert_send(foo(x));
}

/cc @davidtwco @nikomatsakis

@jonas-schievink jonas-schievink added A-async-await Area: Async & Await A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 5, 2020
@nikomatsakis
Copy link
Contributor

Hmm, so the error message for the second example is the following:

Standard Error

   Compiling playground v0.0.1 (/playground)
error[E0277]: `T` cannot be sent between threads safely
 --> src/lib.rs:8:17
  |
1 | async fn foo<T>(x: T) -> T {
  |                          - within this `impl std::future::Future`
...
5 | fn assert_send(_: impl Send) {}
  |    -----------         ---- required by this bound in `assert_send`
...
8 |     assert_send(foo(x));
  |                 ^^^^^^ `T` cannot be sent between threads safely
  |
  = help: within `impl std::future::Future`, the trait `std::marker::Send` is not implemented for `T`
  = note: required because it appears within the type `[static generator@src/lib.rs:1:28: 3:2 x:T {}]`
  = note: required because it appears within the type `std::future::from_generator::GenFuture<[static generator@src/lib.rs:1:28: 3:2 x:T {}]>`
  = note: required because it appears within the type `impl std::future::Future`
  = note: required because it appears within the type `impl std::future::Future`
help: consider restricting type parameter `T`
  |
7 | fn test<T: std::marker::Send>(x: T) {
  |          ^^^^^^^^^^^^^^^^^^^

I think it would be helpful to try and enumerate what the "ideal" error in these two cases would look like. I see a few problems:

  • The notes talking about the details of the types involved (e.g., GenFuture and static generators) are really leaking implementation details of rustc. Maybe we should just suppress those particular types.
  • We are underlining the return type and talking about the "impl Trait" declaration, but the problem is actually the use of x in both cases, we should probably underline that?
  • In the second example, with an async fn, we talk about an impl Future that the user didn't even write, which is not particularly great.

@eddyb
Copy link
Member

eddyb commented Apr 6, 2020

@nikomatsakis Also, I think what happened in the past is that #64895 + #65345 improved the errors when a value is kept across an .await but not when something is just captured... maybe the suboptimal output is only shown if there is no .await at all in the body of the async fn?

So this might be a strawman example we've stumbled over?

@tmandry tmandry added P-medium Medium priority AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. labels Apr 7, 2020
@tmandry tmandry self-assigned this Apr 7, 2020
@tmandry
Copy link
Member

tmandry commented Apr 8, 2020

We could fix this by outputting the nicer error messages, which does seem to happen anytime there exists an await in the async fn.

The reason we don't output the nice error messages is that this code doesn't create a GeneratorInteriorTypeCause for types which are captured from the generator's environment (as in the case of async fn args). As soon as we have an await anywhere in the function, a cause is added for T and we get the nice error message (from this code):

async fn noop() {}
async fn foo<T>(x: T) -> T {
    noop().await;
    x
}
fn assert_send(_: impl Send) {}
fn test<T>(x: T) {
    assert_send(foo(x));
}
error: future cannot be sent between threads safely
 --> issue-70818.rs:8:17
  |
6 | fn assert_send(_: impl Send) {}
  |    -----------         ---- required by this bound in `assert_send`
7 | fn test<T>(x: T) {
8 |     assert_send(foo(x));
  |                 ^^^^^^ future returned by `foo` is not `Send`
  |
  = help: within `impl std::future::Future`, the trait `std::marker::Send` is not implemented for `T`
note: future is not `Send` as this value is used across an await
 --> issue-70818.rs:3:5
  |
2 | async fn foo<T>(x: T) -> T {
  |                 - has type `T` which is not `Send`
3 |     noop().await;
  |     ^^^^^^^^^^^^ await occurs here, with `x` maybe used later
4 |     x
5 | }
  | - `x` is later dropped here
help: consider restricting type parameter `T`
  |
7 | fn test<T: std::marker::Send>(x: T) {
  |          ^^^^^^^^^^^^^^^^^^^

However, this logic can still lead to bad diagnostics for captured vars. For example, when we drop x before awaiting, the diagnostics claim x may still be used after the await:

async fn noop() {}
async fn foo<T>(x: T) {
    std::mem::drop(x);
    noop().await;
}
fn assert_send(_: impl Send) {}
fn test<T>(x: T) {
    assert_send(foo(x));
}
error: future cannot be sent between threads safely
...
  = help: within `impl std::future::Future`, the trait `std::marker::Send` is not implemented for `T`
note: future is not `Send` as this value is used across an await
 --> issue-70818.rs:4:5
  |
2 | async fn foo<T>(x: T) {
  |                 - has type `T` which is not `Send`
3 |     std::mem::drop(x);
4 |     noop().await;
  |     ^^^^^^^^^^^^ await occurs here, with `x` maybe used later
5 | }
  | - `x` is later dropped here
...

So what we need is a way to express a cause for captured vars which doesn't include an await/yield point, and for the diagnostics code to handle these causes.

@tmandry tmandry added E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Apr 8, 2020
@tmandry tmandry removed their assignment Apr 8, 2020
@csmoe csmoe self-assigned this Apr 17, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 20, 2020
Check non-Send/Sync upvars captured by generator

Closes rust-lang#70818
r? @tmandry
@bors bors closed this as completed in f182c4a May 20, 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-enhancement Category: An issue proposing an enhancement or a PR with one. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. P-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants