Stream: wg-async-foundations

Topic: future-not-send diagnostic #64130


nikomatsakis (Sep 10 2019 at 17:21, on Zulip):

I'm going to dump some thoughts here to start. @Esteban Küber I'd love your feedback, too

nikomatsakis (Sep 10 2019 at 17:21, on Zulip):

First off, I think the ideal would be if we could figure out, when looking at a causal chain that derives from a generator or closure in the current crate, that we want to somehow cite the local variable that gave rise to that field.

nikomatsakis (Sep 10 2019 at 17:24, on Zulip):

I'm imagining

error[E0277]: `std::sync::MutexGuard<'_, u32>` cannot be sent between threads safely
  --> src/main.rs:23:5
   |
23 |     is_send(foo());
   |     ^^^^^^^ `std::sync::MutexGuard<'_, u32>` cannot be sent between threads safely
   |
note: the `MutexGuard` is captured by the future representing the function `foo`:
   | async fn foo() {
xx |     bar(&Mutex::new(22)).await;
   |            ------------ the result of this expression is potentially live across an await
   | }
nikomatsakis (Sep 10 2019 at 17:24, on Zulip):

this seems like something we could pretty readily do by examining the causal chain; when we see that one of the types is a field of a generator, we can find the def-id of that generator

nikomatsakis (Sep 10 2019 at 17:24, on Zulip):

we can figure out what expression the field represents (I have to check on how to do that)

nikomatsakis (Sep 10 2019 at 17:25, on Zulip):

and that kind of gives us everything we want to know

nikomatsakis (Sep 10 2019 at 17:25, on Zulip):

it won't work for futures defined in other crates, but let's define our scope apropriately

nikomatsakis (Sep 10 2019 at 17:25, on Zulip):

(in that case, we could write something like "captured by the future respresenting other_crate::foo::bar, at least)

nikomatsakis (Sep 10 2019 at 17:25, on Zulip):

one question would be whether it's important to show how the future type is reachable from the actual type, but I think that's less vital

nikomatsakis (Sep 16 2019 at 19:33, on Zulip):

Hey @Esteban Küber -- you around by any chance?

Esteban Küber (Sep 16 2019 at 19:40, on Zulip):

What's up

nikomatsakis (Sep 16 2019 at 19:46, on Zulip):

I was looking into what this message should say and wanted to bounce it off somebody

nikomatsakis (Sep 16 2019 at 19:48, on Zulip):

I feel like it should .. walk you through the call stack?

nikomatsakis (Sep 16 2019 at 19:50, on Zulip):

what I had in mind so far

nikomatsakis (Sep 16 2019 at 19:50, on Zulip):
error[E0277]: `std::sync::MutexGuard<'_, u32>` cannot be sent between threads safely
  --> src/main.rs:23:5
   |
23 |     is_send(foo());
   |     ^^^^^^^ `std::sync::MutexGuard<'_, u32>` cannot be sent between threads safely
   |
note: the `MutexGuard` arises from the suspended state from calling `foo`
note: in the function `foo`:
   | async fn foo() {
   |          --- the future that results from calling foo...
NN |     bar(&Mutex::new(22)).await;
   |     --- may suspend while calling bar...
note: in the function `bar`
   | async fn bar() {
   |          --- the future that results from calling bar...
NN |     let g = x.lock().unwrap();
   |         - may suspend with `g`, a `MutexGuard` on the stack
nikomatsakis (Sep 16 2019 at 19:50, on Zulip):

something like that?

nikomatsakis (Sep 16 2019 at 19:50, on Zulip):

kind of busy

nikomatsakis (Sep 16 2019 at 19:51, on Zulip):

also, it doesn't say where the original Send requirement comes from

Esteban Küber (Sep 16 2019 at 21:45, on Zulip):

@nikomatsakis as is the output you're mocking can't be done

Esteban Küber (Sep 16 2019 at 21:45, on Zulip):

we can't have multiple spans in a note :-/

Esteban Küber (Sep 16 2019 at 21:45, on Zulip):

I'm thinking using secondary spans directly would be better for communication:

nikomatsakis (Sep 16 2019 at 21:45, on Zulip):

we can't have multiple spans in a note :-/

we should fix that

nikomatsakis (Sep 16 2019 at 21:46, on Zulip):

but I don't know that the "primary span" is important here anyway

nikomatsakis (Sep 16 2019 at 21:46, on Zulip):

er, the underlining of foo

Esteban Küber (Sep 16 2019 at 21:46, on Zulip):

I believe we have a ticket for it, but we haven't worked on it due to the annotate-snippets epic

Esteban Küber (Sep 16 2019 at 21:46, on Zulip):

can you paste the original code?

nikomatsakis (Sep 16 2019 at 21:46, on Zulip):

yeah I mean we should fix it by adopting a more general model from which it just falls out

Esteban Küber (Sep 16 2019 at 21:46, on Zulip):

I wan to mock my preferred output with the right code

nikomatsakis (Sep 16 2019 at 21:47, on Zulip):

code is in #64130

nikomatsakis (Sep 16 2019 at 21:47, on Zulip):
use std::sync::Mutex;

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

}

async fn foo() {
  bar(&Mutex::new(22)).await;
}

async fn bar(x: &Mutex<u32>) {
  let g = x.lock().unwrap();
  baz().await;
}

async fn baz() {

}

fn main() {
    is_send(foo());
}
nikomatsakis (Sep 16 2019 at 21:47, on Zulip):

gotta run right now

nikomatsakis (Sep 16 2019 at 21:47, on Zulip):

but I'll check back in later

nikomatsakis (Sep 16 2019 at 21:47, on Zulip):

separately, I was chatting about it with wycats, who thought that I was overemphasizing the concept of "suspended state", but we didnt' get a chance to dig into what could replace that

Esteban Küber (Sep 16 2019 at 21:50, on Zulip):

I'm thinking something like

error[E0277]: `std::sync::MutexGuard<'_, u32>` cannot be sent between threads safely
  --> src/main.rs:23:5
   |
LL | async fn foo() {
   |          --- when calling this fn resolving in a future...
LL |     bar(&Mutex::new(22)).await;
   |     --- ...this might suspend
...
   | async fn bar() {
   |          --- when calling this fn resolving in a future...
LL |     let g = x.lock().unwrap();
   |         - may suspend with `g`, a `MutexGuard` on the stack (improve wording)
...
LL |     is_send(foo());
   |     ^^^^^^^ `std::sync::MutexGuard<'_, u32>` cannot be sent between threads safely
Esteban Küber (Sep 16 2019 at 21:51, on Zulip):

but even it is not ideal...

Giles Cope (Sep 17 2019 at 17:15, on Zulip):

As a nubie reading this, the point is coming across, though I spent a lot of time parsing " when calling this fn resolving in a future..."
How about:

let g = x.lock().unwrap();
- may suspend with g on the stack as it's in an async context, but alas MutexGuard is not Send.

and ditch the preamble of "--- when calling this fn resolving in a future..."? (Sorry for unasked bikeshedding)

nikomatsakis (Sep 18 2019 at 21:05, on Zulip):

I'm dubious @Esteban Küber about avoiding notes, because I think the order in which things show up may be confusing.

nikomatsakis (Sep 18 2019 at 21:05, on Zulip):

This reminds me of my ideas to extend the diagnostic API with some way to say "this really has to be shown first", which could take advantage of source order when it fits...but otherwise change how we show things.

nikomatsakis (Sep 18 2019 at 21:06, on Zulip):

Anyway I'm going to leave some notes about what it would take to do anything at all. And we can hammer out the best appearance after

nikomatsakis (Sep 18 2019 at 21:30, on Zulip):

OK, @davidtwco, I left some high-level notes that sketch out the approach I had in mind. It's a non-trivial task; seems unlikely to get done before beta branches, but could maybe be backported. (I think it would well be worth it.) Even if we don't backport, just having it landed on nightly (and available in next release) seems worth it.

Do you think you're up to try and tackle it? Feel free to say no, I could also take a stab, though I don't know if I'll have time.

davidtwco (Sep 18 2019 at 21:34, on Zulip):

Thanks. I should have time. I’ll give it a go over the next few days.

nikomatsakis (Sep 18 2019 at 21:46, on Zulip):

Awesome! Obviously send any questions my way.

Esteban Küber (Sep 18 2019 at 21:52, on Zulip):

This reminds me of my ideas to extend the diagnostic API with some way to say "this really has to be shown first", which could take advantage of source order when it fits...but otherwise change how we show things.

I'll try to tackle that once we've moved to annotate-snippets across the board.

davidtwco (Sep 23 2019 at 21:03, on Zulip):

@nikomatsakis can you clarify something?

I think what I would like to do is to also remember the hir_id that caused this type to be added. This would probably be stored in the typeck-tables for the generator. Then, when we are printing the stack trace above, and we see an error was reported for some type in the interior, we can find its index and use that to grab the hir_id that caused the type to be added.

By this, do you mean adding a new field to TypeckTables and storing the HirIds in that?

nikomatsakis (Sep 23 2019 at 21:14, on Zulip):

I think that's what I meant, yes.

davidtwco (Sep 23 2019 at 21:43, on Zulip):

I'm struggling to get the hir_id for the exprs.. I added a field to TypeckTables, and in the record function in generator_interior, I also kept track of the expr.map(|e| e.hir_id). In resolve_interior, I stored that into visitor.fcx.inh.tables.borrow_mut().<my_field>. But in my error reporting function, if I do tcx.typeck_tables_of(generator_did), then there's nothing in my field. I suspect this isn't quite what you intended me to be doing.

davidtwco (Sep 23 2019 at 21:45, on Zulip):

I'm also unclear on this part:

and we see an error was reported for some type in the interior

Not sure how to work out which type in the interior had the error and thus, which index I should use to get the HirId (when I have access to them).

nikomatsakis (Sep 23 2019 at 21:51, on Zulip):

I suspect this isn't quite what you intended me to be doing.

Hmm, it sounds right, but I suspect some of ... "off by one" thing going on.

nikomatsakis (Sep 23 2019 at 21:55, on Zulip):

Ah, @davidtwco -- perhaps the problem is that you haven't modified the write-back code?

davidtwco (Sep 23 2019 at 21:55, on Zulip):

I definitely haven't modified the writeback code, so if that's something I need to be doing, that'll be the issue.

nikomatsakis (Sep 23 2019 at 21:56, on Zulip):

Yeah, so, dear god I don't know if this in the rustc-guide but it should be

nikomatsakis (Sep 23 2019 at 21:56, on Zulip):

anyway so we build up a "temporary" copy of the typeck-tables, which is the one you are modifying

nikomatsakis (Sep 23 2019 at 21:56, on Zulip):

then around this line we copy from that temporary one to the final one,

nikomatsakis (Sep 23 2019 at 21:56, on Zulip):

resolving type inference variables along the way and other artifacts of type inference

nikomatsakis (Sep 23 2019 at 21:56, on Zulip):

so you'll want to add a line like that one for your field

nikomatsakis (Sep 23 2019 at 21:57, on Zulip):

(man, I need to start making a little list of like "random things that have to be explained semi-regularly", so we can point to them...)

davidtwco (Sep 23 2019 at 21:57, on Zulip):

Thanks, will try that.

nikomatsakis (Sep 23 2019 at 21:57, on Zulip):

great! I gotta run, but bbl

nikomatsakis (Sep 23 2019 at 21:57, on Zulip):

also btw I posted an error message that @Yoshua Wuyts and I came up with --

nikomatsakis (Sep 23 2019 at 21:58, on Zulip):

it's worth mentioning only because it doesn't require gathering the whole 'stack trace' for the diagnostics, just the 'top frame'

nikomatsakis (Sep 23 2019 at 21:58, on Zulip):

but I guess you didn't get that far yet :)

davidtwco (Sep 23 2019 at 21:58, on Zulip):

I've got the frames (not with actual messages, just spans for the functions) like in your original example, and none of the details inside of the functions yet.

davidtwco (Sep 23 2019 at 23:06, on Zulip):

Having not seen your newest sketch of what the error message should be until having been going on this for a day or two, this is what I've got tonight before packing it in for the day:

error[E0277]: `std::sync::MutexGuard<'_, u32>` cannot be sent between threads safely
  --> src/test/ui/async-await/issue-64130-non-send-future-diags.rs:23:5
   |
23 |     is_send(foo());
   |     ^^^^^^^ `std::sync::MutexGuard<'_, u32>` cannot be sent between threads safely
   |
   = help: within `impl std::future::Future`, the trait `std::marker::Send` is not implemented for `std::sync::MutexGuard<'_, u32>`
note: in the suspended state from calling `XXX`
  --> src/test/ui/async-await/issue-64130-non-send-future-diags.rs:9:16
   |
9  |   async fn foo() {
   |  ________________^
10 | |     bar(&Mutex::new(22)).await;
   | |     --- may suspend when calling
11 | | }
   | |_^
note: in the suspended state from calling `XXX`
  --> src/test/ui/async-await/issue-64130-non-send-future-diags.rs:13:30
   |
13 |   async fn bar(x: &Mutex<u32>) {
   |  ______________________________^
14 | |     let g = x.lock().unwrap();
15 | |     baz().await;
   | |     --- may suspend when calling
16 | | }
   | |_^
nikomatsakis (Sep 24 2019 at 00:28, on Zulip):

nice!

davidtwco (Sep 24 2019 at 21:12, on Zulip):

my error:

error[E0277]: `std::sync::MutexGuard<'_, u32>` cannot be sent between threads safely
  --> src/test/ui/async-await/issue-64130-non-send-future-diags.rs:23:5
   |
23 |     is_send(foo());
   |     ^^^^^^^ `std::sync::MutexGuard<'_, u32>` cannot be sent between threads safely
   |
   = help: within `impl std::future::Future`, the trait `std::marker::Send` is not implemented for `std::sync::MutexGuard<'_, u32>`
note: in the suspended state from calling `bar`
  --> src/test/ui/async-await/issue-64130-non-send-future-diags.rs:13:1
   |
13 | / async fn bar(x: &Mutex<u32>) {
14 | |     let g = x.lock().unwrap();
15 | |     baz().await;
   | |     --- may suspend when calling
16 | | }
   | |_^

I'm not sure how to get a smaller span for the function name, or how to get the other parts to point at, like g, or the await.

The generator interior types expression spans are just bar().await and then bar() and await and effectively all of the smaller spans you could get out of that line.

Any ideas @nikomatsakis?

davidtwco (Sep 24 2019 at 21:12, on Zulip):

Oh, or the span for the T: Send bound on is_send.

davidtwco (Sep 24 2019 at 21:13, on Zulip):

target error:

error[E0277]: future cannot be sent between threads safely
  --> src/main.rs:23:5
   |
23 |     is_send(foo());
   |     ^^^^^^^ future returned by `foo()` is not `Send`
   |
note: future is not send because this value is used across an await
NN |     let g = x.lock().unwrap();
   |         - has type `std::sync::MutexGuard<'_, u32>`
NN |     baz().await;
   |           ^^^^^ await occurs here, with `g` maybe used later
NN |  }
   |  - `g` is later dropped here
note: `Send` is required because of this where clause
  --> src/main.rs:5:1
   |
5  | fn is_send<T: Send>(t: T) {
   |            -------
nikomatsakis (Sep 25 2019 at 12:42, on Zulip):

@davidtwco is your branch pushed somewhere I can take a look?

nikomatsakis (Sep 25 2019 at 12:42, on Zulip):

Might help me to give an intelligent answer

davidtwco (Sep 25 2019 at 12:58, on Zulip):

Not yet, can do shortly.

davidtwco (Sep 25 2019 at 12:59, on Zulip):

https://git.office.codeplay.com/Integrations/build-inf-control-repo/blob/production/data/common/jenkins.hocon

nikomatsakis (Sep 25 2019 at 13:05, on Zulip):

woah what is that :)

davidtwco (Sep 25 2019 at 13:09, on Zulip):

Oops.

davidtwco (Sep 25 2019 at 13:10, on Zulip):

https://github.com/davidtwco/rust/tree/issue-64130-async-error-definition - copying and pasting is hard.

nikomatsakis (Sep 25 2019 at 13:20, on Zulip):

thanks :)

nikomatsakis (Sep 26 2019 at 15:17, on Zulip):

@davidtwco so I'm looking at your branch now -- I think the problem is that you are not getting the HirId of the correct type. I see this:

nikomatsakis (Sep 26 2019 at 15:18, on Zulip):
 let expr_hir_id = tables.generator_interior_exprs.iter().filter_map(|i| *i).last();
nikomatsakis (Sep 26 2019 at 15:18, on Zulip):

but I think what you want to do is to look at the causation chain

davidtwco (Sep 26 2019 at 15:18, on Zulip):

Ah, I look at those types for other generator interiors in the chain..?

nikomatsakis (Sep 26 2019 at 15:18, on Zulip):

the actual error should be for some type T which is a part of the generator

nikomatsakis (Sep 26 2019 at 15:18, on Zulip):

not the other generators

nikomatsakis (Sep 26 2019 at 15:18, on Zulip):

the chain should look something like this:

nikomatsakis (Sep 26 2019 at 15:19, on Zulip):

really the interior should not be part of that, actually

nikomatsakis (Sep 26 2019 at 15:19, on Zulip):

(I guess i'm going to have to teach myself to use * for lists, dang it)

nikomatsakis (Sep 26 2019 at 15:19, on Zulip):

really the interior should not be part of that, actually

but I think it is today

nikomatsakis (Sep 26 2019 at 15:19, on Zulip):

so you want to find that type T, and then find its index

davidtwco (Sep 26 2019 at 15:19, on Zulip):

but I think it is today

It is on the PR.

davidtwco (Sep 26 2019 at 15:19, on Zulip):

Where do I get T?

nikomatsakis (Sep 26 2019 at 15:19, on Zulip):

(yeah, that's a bug, but a separate one)

nikomatsakis (Sep 26 2019 at 15:20, on Zulip):

@davidtwco let me look at the debug output a bit, it would be the self_ty from the obligation or something

davidtwco (Sep 26 2019 at 15:21, on Zulip):

Sure. On the last generator in the chain (which is the only one I'm looking at now), it's the future, IIRC.

nikomatsakis (Sep 26 2019 at 15:23, on Zulip):

to make life easier, maybe we should add the DefId of the generator to the witness type

nikomatsakis (Sep 26 2019 at 15:23, on Zulip):

ah well never mind that hardly matters

nikomatsakis (Sep 26 2019 at 15:24, on Zulip):

ok so the data structures here are sort of annoying

nikomatsakis (Sep 26 2019 at 15:25, on Zulip):

in the example example we have, the type we want is the MutexGuard type, which is part of the initial Obligation

nikomatsakis (Sep 26 2019 at 15:25, on Zulip):

unfortunately your function is generic over T so it can't readily extract that :)

nikomatsakis (Sep 26 2019 at 15:25, on Zulip):

though we could add a trait bound for it

nikomatsakis (Sep 26 2019 at 15:25, on Zulip):

or we could make some variant of note_obligation_cause that applies when we have a Obligation<TraitRef> or whatever

nikomatsakis (Sep 26 2019 at 15:26, on Zulip):

if you have to climb the stack, however, the self-type would come from the parent_trait_ref at each step

nikomatsakis (Sep 26 2019 at 15:27, on Zulip):

so right now you have something like

Obligation(
    MutexGuard: Send, // <-- what you have to prove, and failed
    DerivedObligation {
        parent_trait_ref: GeneratorWitness(...), // annoying
        parent_cause: DerivedObligation {
            parent_trait_ref: Generator(def_id), // <-- what we see that tells us we are in a generator
            parent_cause: ... // irrelevant
nikomatsakis (Sep 26 2019 at 15:27, on Zulip):

actually adding a DefId to the generator witness might help a little

nikomatsakis (Sep 26 2019 at 15:28, on Zulip):

the idea would be "when we see a "cause" that is a DerivedObligation from a generator witness, we get its def-id, and we take the self type of the current thing we are trying to prove"

nikomatsakis (Sep 26 2019 at 15:28, on Zulip):

if we iterate to the next step, we get the self-type from the parent_trait_ref and repeat

nikomatsakis (Sep 26 2019 at 15:28, on Zulip):

but even w/o modifying the GeneratorWitness would be possible, just .. hairier

nikomatsakis (Sep 26 2019 at 15:29, on Zulip):

does that make sense, @davidtwco? (I'm figuring you'd prefer to make the changes? I probably don't have time anyway, though I'm tempted to tinker with it :P)

nikomatsakis (Sep 26 2019 at 15:29, on Zulip):

(but actually today is super full for me)

davidtwco (Sep 26 2019 at 15:30, on Zulip):

I think so, with that extra context I'll take another look later today.

davidtwco (Sep 28 2019 at 23:36, on Zulip):

@nikomatsakis Sorry for the delay here, updated the branch. I'm now getting an error that looks like this:

error[E0277]: `std::sync::MutexGuard<'_, u32>` cannot be sent between threads safely
  --> $DIR/issue-64130-non-send-future-diags.rs:23:5
   |
LL | fn is_send<T: Send>(t: T) {
   | ------------------------- required by `is_send`
...
LL |     is_send(foo());
   |     ^^^^^^^ `std::sync::MutexGuard<'_, u32>` cannot be sent between threads safely
   |
   = help: within `impl std::future::Future`, the trait `std::marker::Send` is not implemented for `std::sync::MutexGuard<'_, u32>`
note: future is not send because this value is used across an await
  --> $DIR/issue-64130-non-send-future-diags.rs:13:10
   |
LL | async fn bar(x: &Mutex<u32>) {
   |          ^^^
LL |     let g = x.lock().unwrap();
   |         - has type `std::sync::MutexGuard<'_, u32>`
LL |     baz().await;
   |     ----------- await occurs here, with `g` maybe used later

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.

Almost there, just missing the "is dropped here" part of your sketch. It is regressing one other test (see the blessed changes on the branch, changes an error to a cycle error) which I've not looked into fixing yet.

centril (Sep 28 2019 at 23:44, on Zulip):

@davidtwco a quick look: a diagnostic item should be used instead of a lang item

davidtwco (Sep 28 2019 at 23:45, on Zulip):

Will look into changing that tomorrow.

davidtwco (Sep 29 2019 at 13:40, on Zulip):

Opened #64895

nikomatsakis (Sep 30 2019 at 12:38, on Zulip):

@davidtwco that looks pretty awesome! :tada:

nikomatsakis (Sep 30 2019 at 21:33, on Zulip):

@davidtwco I left a few nits here -- I see @oli also r+'d it

nikomatsakis (Sep 30 2019 at 21:34, on Zulip):

I edited the description to cc the issue, vs "fixing" it

davidtwco (Sep 30 2019 at 22:17, on Zulip):

@nikomatsakis I've updated the PR to resolve your comments.

nikomatsakis (Sep 30 2019 at 22:23, on Zulip):

@davidtwco wdyt?

davidtwco (Sep 30 2019 at 22:24, on Zulip):

Looks good, will update PR.

davidtwco (Sep 30 2019 at 22:41, on Zulip):

@nikomatsakis updated again!

oli (Oct 01 2019 at 06:49, on Zulip):

oh oops, I thought it was waiting for my review

davidtwco (Oct 02 2019 at 08:30, on Zulip):

Looks like the PR caused an ICE

nikomatsakis (Oct 02 2019 at 13:04, on Zulip):

d'oh :)

nikomatsakis (Oct 02 2019 at 13:06, on Zulip):

ah, I see there's a fix

nikomatsakis (Oct 02 2019 at 13:06, on Zulip):

and it looks quite reasonable, vg

nikomatsakis (Oct 02 2019 at 13:07, on Zulip):

@davidtwco you had mentioned being up for doing the remaining work, do you have a clear picture of what that is and how to go about it?

davidtwco (Oct 02 2019 at 13:10, on Zulip):

Was it just intercepting the error earlier (or cancelling it in that function and issuing a new one) with a more specific message?

davidtwco (Oct 02 2019 at 13:11, on Zulip):

i.e. this comment

nikomatsakis (Oct 02 2019 at 13:21, on Zulip):

yes

nikomatsakis (Oct 02 2019 at 13:22, on Zulip):

ideally we'd produce this message --

nikomatsakis (Oct 02 2019 at 13:22, on Zulip):

I think there are a few nits, e.g. we'd want to extract the top-most generator frame so we can say things like "future returned by foo()"

nikomatsakis (Oct 02 2019 at 13:23, on Zulip):

we may also want to special case "future cannot be sent between threads safely", though I think right now some of that is done via the more generic #[rustc_error] attributes or whatever, maybe we want to extend those with an optional "future" attribute or something?

nikomatsakis (Oct 02 2019 at 13:28, on Zulip):

but I'm happy to take incremental steps toward it

davidtwco (Oct 02 2019 at 13:28, on Zulip):

I think there are a few nits, e.g. we'd want to extract the top-most generator frame so we can say things like "future returned by foo()"

I think this and the more-specific error shouldn't be too hard.

davidtwco (Oct 02 2019 at 13:29, on Zulip):

Right now, the error is pretty general in that it isn't just send that it cares about.

nikomatsakis (Oct 02 2019 at 13:51, on Zulip):

@davidtwco yep -- and we probably need to consider Sync too --

nikomatsakis (Oct 02 2019 at 13:51, on Zulip):

but basically this whole thing is tailored to auto traits

nikomatsakis (Oct 02 2019 at 13:52, on Zulip):

(hmm, maybe Clone?)

nikomatsakis (Oct 02 2019 at 13:53, on Zulip):

ah, no, of course not, only auto traits, "because impl Trait leakage"

nikomatsakis (Oct 02 2019 at 13:53, on Zulip):

it so happens of course that send/sync are a very common case

davidtwco (Oct 02 2019 at 14:04, on Zulip):

I can make it specialised for those pretty easily I think.

nikomatsakis (Oct 02 2019 at 16:02, on Zulip):

(I'm seeing that ICE just on a -i bootstrap, actually)

davidtwco (Oct 02 2019 at 16:02, on Zulip):

I did too.

davidtwco (Oct 02 2019 at 16:02, on Zulip):

(this morning, not before it landed)

Paul Faria (Oct 04 2019 at 13:51, on Zulip):

Was the PR meant to fix all instances of <Some type> cannot be sent between threads safely regarding futures? I have one that does not end in a note: required by <X>. I can find out if it's possible for me to share this code (it would be an amazing example of an extremely long type being converted to a nice error message)

nikomatsakis (Oct 04 2019 at 15:10, on Zulip):

not necessarily -- but it'd be good to see your example @Paul Faria

Paul Faria (Oct 04 2019 at 15:54, on Zulip):

Would just the error suffice or access to the code would help too? I can see if I'm allowed to publish it opensource, it's just a utility tool.

nikomatsakis (Oct 04 2019 at 17:32, on Zulip):

@Paul Faria the error might suffice

Paul Faria (Oct 04 2019 at 18:46, on Zulip):

https://gist.github.com/Nashenas88/f39c143daf2147d3d599ec78c5f6c961 hopefully this helps

nikomatsakis (Oct 04 2019 at 19:35, on Zulip):

If I remember from reading @davidtwco's gist, I thnk we might have been somewhat conservative when the generator appears inside of a struct, does that sound right @davidtwco?

nikomatsakis (Oct 04 2019 at 19:35, on Zulip):

If so, I wonder if we should be more general than that

nikomatsakis (Oct 04 2019 at 19:35, on Zulip):

(I sort of think so)

davidtwco (Oct 04 2019 at 19:49, on Zulip):

It only considers cases where the obligation stack is generators, generator interiors, opaque types and std::future::GenFuture.

davidtwco (Oct 04 2019 at 19:51, on Zulip):

And the last obligation is a item/binding obligation.

nikomatsakis (Oct 04 2019 at 20:04, on Zulip):

It only considers cases where the obligation stack is generators, generator interiors, opaque types and std::future::GenFuture.

notably, anyone using combinators will be unable to benefit here :(

nikomatsakis (Oct 04 2019 at 20:04, on Zulip):

I think we should consider extending that to "random structs" that live "atop" the generators

nikomatsakis (Oct 04 2019 at 20:05, on Zulip):

And the last obligation is a item/binding obligation.

I'm not sure I understand that

davidtwco (Oct 04 2019 at 20:08, on Zulip):

And the last obligation is a item/binding obligation.

I'm not sure I understand that

https://github.com/rust-lang/rust/blob/master/src/librustc/traits/error_reporting.rs#L1708-L1709

davidtwco (Oct 06 2019 at 22:04, on Zulip):

Update: I've got the error changed as shown in the diff below:

diff --git a/src/test/ui/async-await/issue-64130-non-send-future-diags.stderr b/src/test/ui/async-await/issue-64130-non-send-future-diags.stderr
index 9e9fc52e30b..a393669a3ab 100644
--- a/src/test/ui/async-await/issue-64130-non-send-future-diags.stderr
+++ b/src/test/ui/async-await/issue-64130-non-send-future-diags.stderr
@@ -1,14 +1,14 @@
-error[E0277]: `std::sync::MutexGuard<'_, u32>` cannot be sent between threads safely
+error[E0277]: future cannot be sent between threads safely
   --> $DIR/issue-64130-non-send-future-diags.rs:23:5
    |
 LL | fn is_send<T: Send>(t: T) {
    |    -------    ---- required by this bound in `is_send`
 ...
 LL |     is_send(foo());
-   |     ^^^^^^^ `std::sync::MutexGuard<'_, u32>` cannot be sent between threads safely
+   |     ^^^^^^^ future returned by `foo` is not send
    |
    = help: within `impl std::future::Future`, the trait `std::marker::Send` is not implemented for `std::sync::MutexGuard<'_, u32>`
-note: future does not implement `std::marker::Send` as this value is used across an await
+note: future does not implement send as this value is used across an await
   --> $DIR/issue-64130-non-send-future-diags.rs:15:5
    |
 LL |     let g = x.lock().unwrap();

I've generalized it a bit so that combinators should get changed too and that there's a specialized version for sync - not written test cases for those yet though. I'm going to polish it off a bit locally before throwing a PR up.

nagisa (Oct 06 2019 at 22:16, on Zulip):

"is not send" reads weird.

davidtwco (Oct 06 2019 at 22:17, on Zulip):

I was just working to the draft in this comment, I think it reads fine though? Any suggestions for alternate wording?

nagisa (Oct 06 2019 at 22:18, on Zulip):

I was just working to the draft in this comment, I think it reads fine though? Any suggestions for alternate wording?

is not Send? That capital letter does a lot, and so does the inline code markup

davidtwco (Oct 06 2019 at 22:19, on Zulip):

I can change it to that.

nikomatsakis (Oct 07 2019 at 12:58, on Zulip):

@davidtwco looks good to me!

nikomatsakis (Oct 07 2019 at 12:58, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/librustc/traits/error_reporting.rs#L1708-L1709

ok ok I see -- i'm not sure if that's relevant to @Paul Faria's example. It's probably the case that we could lift this restriction if needed.

davidtwco (Oct 07 2019 at 13:00, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/librustc/traits/error_reporting.rs#L1708-L1709

ok ok I see -- i'm not sure if that's relevant to Paul Faria's example. It's probably the case that we could lift this restriction if needed.

I have in my newest changes.

nikomatsakis (Oct 09 2019 at 13:57, on Zulip):

@davidtwco by any chance, could you try this example on your current branch?

nikomatsakis (Oct 09 2019 at 13:57, on Zulip):

I'm curious if the diagnostics at least are improved

davidtwco (Oct 09 2019 at 13:59, on Zulip):

Still the old error, I'll add that as a test case to get working.

davidtwco (Oct 12 2019 at 17:12, on Zulip):

Just got around to looking at that case now, the diagnostic doesn't trigger for that because wat isn't an async fn, just a fn with an async move { } and for a reason I've yet to understand, this diagnostic will cause a cycle-error for async move { }.

davidtwco (Oct 12 2019 at 17:21, on Zulip):

I've opened #65345 with what I have so far.

davidtwco (Oct 12 2019 at 17:21, on Zulip):

Struggling to make it apply to more cases, it still feels very tied to a handful of specific conditions.

nikomatsakis (Oct 17 2019 at 22:33, on Zulip):

Hey @davidtwco -- so https://github.com/rust-lang/rust/pull/65345 is causing add'l cycle errors?

davidtwco (Oct 17 2019 at 22:35, on Zulip):

@nikomatsakis No. But, if I remove the check that there must be an async fn, so that it works with async move { } blocks, then that causes cycle errors (for an example, see the case you sent on Oct 9th).

davidtwco (Oct 17 2019 at 22:36, on Zulip):

I should clarify, this happens without that patch, the original patch did the same, which is why that check for async fn specifically existed in the first place.

davidtwco (Oct 17 2019 at 22:37, on Zulip):

These lines specifically

Mark Drobnak (Oct 21 2019 at 14:41, on Zulip):

I read the "Future is not Send" blog post and have a question about the error and diagnostic. If this isn't the right place to ask, please redirect me to the right place.

https://blog.rust-lang.org/inside-rust/2019/10/11/AsyncAwait-Not-Send-Error-Improvements.html

async fn bar(x: &Mutex<u32>) {
    let g = x.lock().unwrap();
    baz().await
}

As NLL is now in use, why does g live until the end of the function? Shouldn't it be dropped once it is no longer used?

simulacrum (Oct 21 2019 at 15:03, on Zulip):

NLL has little to do with this -- you'd want the lock to still be taken for the duration of the scope

simulacrum (Oct 21 2019 at 15:03, on Zulip):

i.e. it's not really a lifetimes issue

simulacrum (Oct 21 2019 at 15:03, on Zulip):

in the 'a sense :)

Mark Drobnak (Oct 21 2019 at 16:28, on Zulip):

@simulacrum do you know why this happens? It's probably not specific to locks, so what is the underlying issue? (If you don't know feel free to redirect me)

simulacrum (Oct 21 2019 at 17:03, on Zulip):

@Mark Drobnak So the code example can be rewritten as the following, which illustrates why it would change behavior for g's lifetime to end before the await

async fn bar(x: &Mutex<u32>) {
    let g = x.lock().unwrap();
    baz().await
    mem::drop(g);
}
simulacrum (Oct 21 2019 at 17:03, on Zulip):

(since the code in the destructor would then run earlier)

Mark Drobnak (Oct 21 2019 at 17:11, on Zulip):

Right, but I thought NLL would have changed it to be:

async fn bar(x: &Mutex<u32>) {
    let g = x.lock().unwrap();
    mem::drop(g);
    baz().await
}
Mark Drobnak (Oct 21 2019 at 17:12, on Zulip):

What stops this from happening?

simulacrum (Oct 21 2019 at 17:49, on Zulip):

NLL does not change destructor ordering

simulacrum (Oct 21 2019 at 17:49, on Zulip):

that would be a massive breaking change

simulacrum (Oct 21 2019 at 17:49, on Zulip):

like, that drop releases the lock

simulacrum (Oct 21 2019 at 17:49, on Zulip):

if we previously had that for the entire block and now don't that could have a number of unexpected effects that people don't want

Mark Drobnak (Oct 21 2019 at 18:02, on Zulip):

Ok, I see. It's a difference between reference and data lifetimes. The data lifetimes didn't change as part of NLL, only references. Thanks for sticking with me to figure this out!

Mark Drobnak (Oct 21 2019 at 18:03, on Zulip):

This distinction is talked about in the NLL RFC:

Note that Rust uses the term lifetime in a very particular way. In everyday speech, the word lifetime can be used in two distinct -- but similar -- ways:

  1. The lifetime of a reference, corresponding to the span of time in which that reference is used.
  2. The lifetime of a value, corresponding to the span of time before that value gets freed (or, put another way, before the destructor for the value runs).

https://github.com/rust-lang/rfcs/blob/master/text/2094-nll.md

davidtwco (Oct 24 2019 at 12:44, on Zulip):

@nikomatsakis do you have any thoughts on why the diagnostic improvement causes cycle errors when I stop limiting it to only apply to errors originating from async fn and not async { }?

nikomatsakis (Oct 24 2019 at 12:51, on Zulip):

@davidtwco I .. do not :) can I see the diff maybe?

simulacrum (Oct 24 2019 at 12:51, on Zulip):

eddyb might've fixed this in a recent PR

simulacrum (Oct 24 2019 at 12:51, on Zulip):

not sure though, I recall something similar

davidtwco (Oct 24 2019 at 13:15, on Zulip):

The diff would be removing these lines?

davidtwco (Oct 24 2019 at 13:15, on Zulip):

eddyb might've fixed this in a recent PR

I'll rebase and check.

simulacrum (Oct 24 2019 at 13:29, on Zulip):

oh I'm not sure it landed... let me look

simulacrum (Oct 24 2019 at 13:29, on Zulip):

@davidtwco I was referring to https://github.com/rust-lang/rust/pull/65743

simulacrum (Oct 24 2019 at 13:30, on Zulip):

not sure if that actually relates or not though

davidtwco (Oct 24 2019 at 13:30, on Zulip):

It wouldn't take much to rebase atop and check, thanks!

davidtwco (Oct 24 2019 at 14:01, on Zulip):

Unfortunately, that didn't do the trick.

davidtwco (Oct 24 2019 at 14:01, on Zulip):

Still cycle errors.

davidtwco (Oct 24 2019 at 14:17, on Zulip):

This is an example of one of the cycle errors:

error[E0391]: cycle detected when processing `main`
  --> /home/david/projects/rust/rust4/src/test/ui/generator/not-send-sync.rs:5:1
   |
LL | fn main() {
   | ^^^^^^^^^
   |
note: ...which requires processing `main::{{closure}}#1`...
  --> /home/david/projects/rust/rust4/src/test/ui/generator/not-send-sync.rs:16:17
   |
LL |     assert_send(|| {
   |                 ^^
   = note: ...which again requires processing `main`, completing the cycle
note: cycle used when processing `main::{{closure}}#0`
  --> /home/david/projects/rust/rust4/src/test/ui/generator/not-send-sync.rs:9:17
   |
LL |     assert_sync(|| {
   |                 ^^

error: aborting due to previous error
davidtwco (Oct 24 2019 at 14:24, on Zulip):

Unrelated, but I've also noticed that a case like this:

async fn bar() {
    let x = Foo;
    baz().await;
    std::mem::drop(x);
}

Will produce this:

note: future does not implement `Qux` as this value is used across an await
  --> /home/david/projects/rust/rust4/src/test/ui/async-await/issue-64130-4-manual-drop.rs:17:5
   |
LL |     let x = Foo;
   |         - has type `Foo`
LL |     baz().await;
   |     ^^^^^^^^^^^ await occurs here, with `x` maybe used later
LL |     std::mem::drop(x);
LL | }
   | - `x` is later dropped here

I'll also need to work out how to fix that.

nikomatsakis (Oct 25 2019 at 18:37, on Zulip):

@davidtwco well I guess the problem is somehow related to invoking typeck_tables_of on the generator

nikomatsakis (Oct 25 2019 at 18:38, on Zulip):

what's the example?

nikomatsakis (Oct 25 2019 at 18:38, on Zulip):

I am guessing that there is some error using the generator

nikomatsakis (Oct 25 2019 at 18:38, on Zulip):

triggered by the context that also created the generator

nikomatsakis (Oct 25 2019 at 18:38, on Zulip):

and hence we are reporting the error while type-checking that context

nikomatsakis (Oct 25 2019 at 18:38, on Zulip):

and so there is a sort of cycle

nikomatsakis (Oct 25 2019 at 18:39, on Zulip):

that said, I would expect it may be possible to trigger the error even without removing those lines?

nikomatsakis (Oct 25 2019 at 18:39, on Zulip):

but maybe there's some reason you can't

davidtwco (Oct 25 2019 at 19:14, on Zulip):

That example was from the not-send-sync test.

davidtwco (Oct 25 2019 at 19:14, on Zulip):

davidtwco by any chance, could you try this example on your current branch?

This case also triggers it.

davidtwco (Oct 25 2019 at 19:27, on Zulip):

I think because I rely on the information I put in the typeck table to generate the error, as soon as I get rid of that check, and it gets to that point, it cycle errors. I just don’t know how to get around that.

nikomatsakis (Oct 30 2019 at 12:58, on Zulip):

@davidtwco so I have to apply a diff to see the cycle errors?

davidtwco (Oct 30 2019 at 12:58, on Zulip):

Yeah.

davidtwco (Oct 30 2019 at 12:59, on Zulip):

The PR currently passes everything by prohibiting the diagnostic from applying to non-async fn cases.

davidtwco (Oct 30 2019 at 12:59, on Zulip):

Same as the original PR.

nikomatsakis (Oct 30 2019 at 12:59, on Zulip):

ok I see

nikomatsakis (Oct 30 2019 at 12:59, on Zulip):

I commented out the lines you mentioned earlier

nikomatsakis (Oct 30 2019 at 12:59, on Zulip):

I'm doing a build

nikomatsakis (Oct 30 2019 at 13:00, on Zulip):

that said, I think I know what the problem is

nikomatsakis (Oct 30 2019 at 13:04, on Zulip):

what I'm not sure of is the best way to fix it

nikomatsakis (Oct 30 2019 at 13:05, on Zulip):

ah, hmm

nikomatsakis (Oct 30 2019 at 13:17, on Zulip):

So I'm trying something @davidtwco -- which is to get the typeck-tables optionally from the current InferCtxt

nikomatsakis (Oct 30 2019 at 13:18, on Zulip):

but hmm that doesn't quite help

nikomatsakis (Oct 30 2019 at 13:18, on Zulip):

oh, I bet I know why

nikomatsakis (Oct 30 2019 at 13:30, on Zulip):

@davidtwco ok I pushed a commit

nikomatsakis (Oct 30 2019 at 13:30, on Zulip):

it's marked as [wip]

nikomatsakis (Oct 30 2019 at 13:30, on Zulip):

here's the good news: no more cycle errors

nikomatsakis (Oct 30 2019 at 13:30, on Zulip):

the bad news is: the nice diagnostic doesn't trigger

nikomatsakis (Oct 30 2019 at 13:30, on Zulip):

I have to run for a bit but my guess here is that the "in-progress tables" haven't had the generator information added to them yet?

nikomatsakis (Oct 30 2019 at 13:31, on Zulip):

otoh, I thought that we added that information at the same time as we resolved the inference variables, so that could be just wrong

nikomatsakis (Oct 30 2019 at 13:32, on Zulip):

here's the good news: no more cycle errors

I'm like 90% sure this fix is sufficient -- I was a bit afraid you might have

so that the in-progress tables are not the same as the "current function" being tested, but I think that you can't actually wind up here in that case, because you can't have a generator type with the def-id

nikomatsakis (Oct 30 2019 at 13:32, on Zulip):

/me -> out

davidtwco (Oct 30 2019 at 13:36, on Zulip):

Thanks! Will take a look why it doesn't trigger now.

davidtwco (Oct 30 2019 at 13:54, on Zulip):

For not-send-sync, I get

error[E0277]: `std::cell::Cell<i32>` cannot be shared between threads safely
  --> src/test/ui/generator/not-send-sync.rs:16:5
   |
7  |     fn assert_send<T: Send>(_: T) {}
   |        -----------    ---- required by this bound in `main::assert_send`
...
16 |     assert_send(|| {
   |     ^^^^^^^^^^^ `std::cell::Cell<i32>` cannot be shared between threads safely
   |
   = help: the trait `std::marker::Sync` is not implemented for `std::cell::Cell<i32>`
   = note: required because of the requirements on the impl of `std::marker::Send` for `&std::cell::Cell<i32>`
   = note: required because it appears within the type `[generator@src/test/ui/generator/not-send-sync.rs:16:17: 20:6 a:&std::cell::Cell<i32> _]`

error: future cannot be shared between threads safely
  --> src/test/ui/generator/not-send-sync.rs:9:5
   |
6  |     fn assert_sync<T: Sync>(_: T) {}
   |        -----------    ---- required by this bound in `main::assert_sync`
...
9  |     assert_sync(|| {
   |     ^^^^^^^^^^^ future returned by `main` is not `Sync`
   |
   = help: within `[generator@src/test/ui/generator/not-send-sync.rs:9:17: 13:6 {std::cell::Cell<i32>, ()}]`, the trait `std::marker::Sync` is not implemented for `std::cell::Cell<i32>`
note: future is not `Sync` as this value is used across an await
  --> src/test/ui/generator/not-send-sync.rs:12:9
   |
11 |         let a = Cell::new(2);
   |             - has type `std::cell::Cell<i32>`
12 |         yield;
   |         ^^^^^ await occurs here, with `a` maybe used later
13 |     });
   |     - `a` is later dropped here

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0277`.

I think that's correct, assert_send doesn't have any variables inside the generator that aren't Send, it's only the captured var from outside the generator.

davidtwco (Oct 30 2019 at 13:54, on Zulip):

I'll need to adjust the wording so if it isn't async then we say "yield" instead of "await".

nikomatsakis (Oct 30 2019 at 14:03, on Zulip):

by any chance, could you try this example on your current branch?

this is the example I was saying doesn't trigger, I think

davidtwco (Oct 30 2019 at 14:05, on Zulip):

Oh, I'll check that in a moment then.

davidtwco (Oct 30 2019 at 14:28, on Zulip):

The issue with that case is that the type in the obligation is dyn Any + Send + 'static but the generator interior list doesn't contain that, it does contain a Client (whose only field is the type we're looking for).

davidtwco (Oct 30 2019 at 14:29, on Zulip):

So I suppose I need to check if the generator interior type "contains" the target type.

nikomatsakis (Oct 30 2019 at 14:47, on Zulip):

@davidtwco what I expected to do was to take the type from the frame "just below" the generator in the "backtrace"

davidtwco (Oct 30 2019 at 16:18, on Zulip):

@nikomatsakis Current issue: checking two &Client types (from that test case) for equality returns false, I assume that the region is different in the type, what function do I use to normalize for that?

davidtwco (Oct 30 2019 at 16:19, on Zulip):

Was previously just checking TyS::same_type.

davidtwco (Oct 30 2019 at 16:40, on Zulip):

(I've worked around it with a builtin_deref)

davidtwco (Oct 30 2019 at 17:12, on Zulip):

Updated the PR with my changes, left some comments where the diagnostic could get better or where I did a hack.

Last update: Nov 18 2019 at 02:05UTC