Stream: wg-async-foundations

Topic: temporary lifetimes for awaited future #64292 #64391


nikomatsakis (Sep 16 2019 at 13:11, on Zulip):

So

cc @WG-async-foundations but also
cc lang-team: @centril, @Josh Triplett, @pnkfelix, @boats, @Taylor Cramer but also
especially cc @Florian Gilcher, as I think this presents a "schedule risk" to stabilizing async-await, and I'm sure you'd like to know =)

It seems pretty important that we decide whether this breakage is acceptable and if we're going to change anything about it. Unfortunately, this is a pretty thorny area overall (one we've long hoped to try and tweak, though I don't hold out a ton of hope).

The TL;DR is that we have to decide how foo.await interacts with the temporary rules. Unfortunately, this is the kind of issue that is quite hard to assess in the abstract. We thought we'd made the right decision before in fixing #63832, but it seems clear that it's had a lot of impact, and I'm not convinced we're making the right call in a practical sense (although it still feels the most "consistent" to me).

I was thinking I will try to spend some day today getting to the bottom of exactly what's happening and trying to do a write-up, then maybe we can try to reach a decision on what to do.

nikomatsakis (Sep 16 2019 at 13:13, on Zulip):

At the moment, async-await is stable on nightly. The next beta date is September 26th 2019, though typically I think beta branches a few days earlier than that. The good news is that we still have this week to deliberate. The bad news is that this doesn't give us a lot of time to gain experience with any potential tweaks before the beta branches (though we could backport a decision).

nikomatsakis (Sep 16 2019 at 13:13, on Zulip):

And yes, I lost quite a bit of sleep about this last night when I remembered it :sleepy:

centril (Sep 16 2019 at 13:15, on Zulip):

It seems pretty important that we decide whether this breakage is acceptable and if we're going to change anything about it.

You mean in the sense that we assess what is overall most ergonomic using the breakage as "how is code going to look like in 3 years"? (I think it's clearly allowed within the stability policy to break this code)

nikomatsakis (Sep 16 2019 at 13:22, on Zulip):

Yes, sorry.

nikomatsakis (Sep 16 2019 at 13:22, on Zulip):

I guess you mean "we can break code using async-await today as it is not yet stable" and I definitely agree

nikomatsakis (Sep 16 2019 at 13:22, on Zulip):

but the question is "is it good that this code broke" :)

nikomatsakis (Sep 16 2019 at 13:35, on Zulip):

@Yoshua Wuyts I'm trying to document exactly what was affected and why, and then I'll better be able to answer that question. I would say however that it is not obvious that this would be something we can fix later

nikomatsakis (Sep 16 2019 at 13:36, on Zulip):

I'm almost done with my first survey, but I would be interested to know if you have a lot of examples of broken code, so I can be sure to understand the patterns (and document the extent of the effect)

nikomatsakis (Sep 16 2019 at 13:54, on Zulip):

Specifically, @Yoshua Wuyts / @stjepang, is there more impact than the changes I see here?

nikomatsakis (Sep 16 2019 at 14:06, on Zulip):

In any case, I created a gist describing the situation and starting to catalog the effects. I plan to keep updating it, but those who'd like to get up to date should check it out.

Yoshua Wuyts (Sep 16 2019 at 14:07, on Zulip):

Yeah, that's all that we needed to change on our end. That said I didn't
have the greatest time debugging it
<https://twitter.com/yoshuawuyts/status/1171771795613126657> (Stjepan
bailed me out here), but otherwise the changes have been fairly limited for
our work.
— Yosh

nikomatsakis (Sep 16 2019 at 14:22, on Zulip):

woah

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

that reminds me that I have to finish improving the diagnostics

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

@tmandry seemed to report substantially more impact on Fuschia

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

In any case, reviewing the cases it didn't seem as bad as I initially remembered, so it may be that there is no action needed here.

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

But in any case i'd like to decide that now

nikomatsakis (Sep 16 2019 at 14:24, on Zulip):

and not the night before we are branching beta

Florian Gilcher (Sep 16 2019 at 14:33, on Zulip):

Is the main problem here a semantic problem or an implementation problem?

Josh Triplett (Sep 16 2019 at 15:55, on Zulip):

Seems like the former to me, first and foremost: figuring out the semantics we want

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

Thinking about this more, I realize there may be a "fwd-compatible" path involving the perenially delayed (but still relevant) rfc#66

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

Also, I'm realizing there may be 2 bugs at play here

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

Question : what is the easiest way to "drive" a future from playground .. I guess maybe there isn't one?

nikomatsakis (Sep 16 2019 at 16:17, on Zulip):

I guess I should look how the existing testcases for drop order that @davidtwco added work

nikomatsakis (Sep 16 2019 at 16:17, on Zulip):

I think there is a subtle flaw in the drop order for our desugaring around the "tail expression" of an async fn

centril (Sep 16 2019 at 16:19, on Zulip):

Question : what is the easiest way to "drive" a future from playground .. I guess maybe there isn't one?

(Feels like an omission from the standard library possibly)

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

I mean it's kind of hard to do "independently" of a runtime, in any non-trivial case

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

anyway, neither here nor there

nikomatsakis (Sep 16 2019 at 17:28, on Zulip):

Yeah, that's all that we needed to change on our end.

@Yoshua Wuyts to be clear, this error was about some type from format! not being Send, I guess? i.e., this is ultimately an instance of #64477

tmandry (Sep 16 2019 at 18:10, on Zulip):

tmandry seemed to report substantially more impact on Fuschia

I think I counted about 60 errors in our tree, each of which had to be manually deciphered and fixed

tmandry (Sep 16 2019 at 18:10, on Zulip):

maybe more in targets that depend on other targets which broke

tmandry (Sep 16 2019 at 18:11, on Zulip):

the fix is always the same, but half the time the diagnostics are very unhelpful (the other half they pretty much point you right to the code you need to change)

nikomatsakis (Sep 16 2019 at 18:12, on Zulip):

Sorry for the spam, but I've finished my gist. It's kind of ridiculously long. The TL;DR at the front tries to summarize the situation.

The good news:

The bad news:

One unknown:

Overall, I think we have to choose between:

On net, I'm not really sure what I think though.

cc @WG-async-foundations @centril, @Josh Triplett, @pnkfelix, @boats, @Taylor Cramer @Florian Gilcher

nikomatsakis (Sep 16 2019 at 18:13, on Zulip):

cc @WG-async-foundations @centril, @Josh Triplett, @pnkfelix, @boats, @Taylor Cramer @Florian Gilcher :point_up: -- I edited the message which seems to trigger some Zulip bug that clears the notification, sorry

nikomatsakis (Sep 16 2019 at 18:14, on Zulip):

the fix is always the same, but half the time the diagnostics are very unhelpful (the other half they pretty much point you right to the code you need to change)

thanks! It sounds to me like you are hitting both #64512 (which has reasonably targeted diagnostics) and #64477 (which doesn't)

nikomatsakis (Sep 16 2019 at 18:14, on Zulip):

If we had a fix for #64512, do you think you could try reverting those fixes and seeing the effect?

tmandry (Sep 16 2019 at 18:16, on Zulip):

thanks! It sounds to me like you are hitting both #64512 (which has reasonably targeted diagnostics) and #64477 (which doesn't)

that sounds right

tmandry (Sep 16 2019 at 18:16, on Zulip):

and yes I can, I only fixed a few before I decided to wait and see if this was going to stick or not ;)

tmandry (Sep 16 2019 at 18:18, on Zulip):

those never even made it into the tree, so we'll have a fresh slate to look at

nikomatsakis (Sep 16 2019 at 18:18, on Zulip):

and yes I can, I only fixed a few before I decided to wait and see if this was going to stick or not ;)

smart :)

RalfJ (Sep 16 2019 at 19:00, on Zulip):

the temporaries in $body wind up being dropped after the let-bound variables of the block. Proposed fix in a comment below.

wait what? I thought in let x = expr; body, x is dropped after stuff in body (inverse declaration order)?

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

So, for better or worse -- I mostly think worse, these days -- the temporaries in a tail expression of a block are promoted to the surrounding "temporary lifetime context".

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

(The original motivation was that let x = expr; and let x = {expr}; should work equivalently, which I now suspect was wrong-headed.)

Josh Triplett (Sep 16 2019 at 20:46, on Zulip):

That's probably going to end up being Rust's equivalent of Python's and C99's loop variable scoping, in terms of "quirk to explain to new users".

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

I was wondering if we can change it, maybe in an edition

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

non-trivial to do but maybe worthwhile

Josh Triplett (Sep 17 2019 at 01:23, on Zulip):

I suspect we had use cases for this in mind more complex than just "adding braces around an expression shouldn't change scoping".

Matthias247 (Sep 17 2019 at 05:16, on Zulip):

Question : what is the easiest way to "drive" a future from playground .. I guess maybe there isn't one?

(Feels like an omission from the standard library possibly)

I think adding just block_on(future) to the std library would probably not too controversial.

Matthias247 (Sep 17 2019 at 05:31, on Zulip):

@nikomatsakis I am not sure whether it helps for the discussion, but sync and async functions are already consistent in other ways. E.g. that async functions are cancellable and may not resume after a .await point, which users might need to design for.
But being as consistent as possible has still the advantage of reducing the cognitive overhead of dealing with 2 worlds, so I understand the reasoning behind the change to align the lifetime of temporaries.

On the other hand the new issue is just another consistency issue: Normal functions never experience the symptom of parts of the function being called on a different thread, whereas it is normal for futures on multithreaded executors.

So it seems like we need to trade of which of those 2 seems more reasonable?

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

I wonder if later on we might be able to drop the + Send part of impl Future + Send for a same thread executor,
maybe by have a syntax like: async fn foo() -> () + !Send - then the local variables would not need Send over suspention points?

centril (Sep 18 2019 at 18:05, on Zulip):

@nikomatsakis some notes:

centril (Sep 18 2019 at 18:06, on Zulip):

possibly relevant: https://github.com/rust-rfcs/const-eval/pull/27

also cc @oli @RalfJ @eddyb @ecstatic-morse

nikomatsakis (Sep 18 2019 at 18:43, on Zulip):

context for those following along is that I sent this e-mail to the lang team

Hey lang team,

So I just want to raise this issue of temporary scopes a bit more broadly, since we're going to be stabilizing async-await soon and that won't leave us room to make changes. I've written up a gist that contains the details:

https://gist.github.com/nikomatsakis/fee0e47e14c09c4202316d8ea51e50a0

For those who've seen the gist, I made a few minor edits. In particular, I noted that (a) some of the regression reports can be fixed by making the compiler more precise about what is being captured and (b) there is a forwards compatible path to improving all cases, and it would benefit sync code to boot, but it would be a major bit of design work. (In particular, we'd need some mechanism for "pure drops" that lets us move drops earlier.)

After having thought about this for a while, I think that the current order on master is correct, but I wanted to raise it to give people time to object.

nikomatsakis (Sep 18 2019 at 18:43, on Zulip):

Also, I just opened https://github.com/rust-lang/rust/pull/64584, which addresses some "overzealous" cases of #64477

boats (Sep 18 2019 at 19:53, on Zulip):

I've read the gist and I agree that the current rules on stable are fundamentally correct. :+1:

boats (Sep 18 2019 at 19:53, on Zulip):

With the really unfortunate &format!() example, the Arguments temporary created shouldn't actually need to outlive the future, because it will be moved into the std::fmt::format call which returns a String, is this an example of the overzealousness you're talking about with 64584?

nikomatsakis (Sep 18 2019 at 20:49, on Zulip):

@boats yes -- that is an example. I've heard a lot of people complain, similarly, about cases like this:

let x = ...;
mem::drop(x);
foo().await; // error: x is considered maybe live
nikomatsakis (Sep 18 2019 at 20:49, on Zulip):

the fundamental problem here is that we do an "initial approximation" based on the HIR, which is used for trait checking; then we build the MIR, which gives us access to much more detailed information

nikomatsakis (Sep 18 2019 at 20:49, on Zulip):

but it's too late

nikomatsakis (Sep 18 2019 at 20:51, on Zulip):

I'm not sure what I think is the best fix. It may be possible to build the MIR first, but I don't entirely see how. Maybe it works because we're sufficiently restrictive of cycles or something.

I've been wondering though if there isn't room for us to refactoring the relationship of HIR to MIR, basically trying to create some kind of view onto HIR that gives us the detailed information we want and suffices for borrow checking and other stuff, and which would then be the basis for building MIR. Anyway, that's going a bit astray.

lqd (Sep 18 2019 at 23:08, on Zulip):

It seems #64391 was reopened :/

lqd (Sep 18 2019 at 23:13, on Zulip):

it took a while but here's a minimization it seems the fix might have only worked for Copy types ?

nikomatsakis (Sep 19 2019 at 12:53, on Zulip):

Hmm, so I was able to build tokio-postgres; regardless, those errors are correct

nikomatsakis (Sep 19 2019 at 12:56, on Zulip):

well, I'll have to dig a bit more

nikomatsakis (Sep 19 2019 at 12:56, on Zulip):

there may be a tweaked desugaring we could use...

nikomatsakis (Sep 19 2019 at 13:29, on Zulip):

OK, did a detailed examination. The desugaring is not the problem. It seems like our MIR lowering is created a lot more drops than we need to -- and those drops sometime make us more conservative than we should be, because they introduce the possibility of unwind edges.

lqd (Sep 19 2019 at 15:43, on Zulip):

hopefully I didn't reduce this so much that it isn't representative of the initial problem anymore; there were a lot more generic types in tokio-postgres (instead of a String argument per se) but that should be the same, moving a T argument between two async fns, wrt the drops

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

Pushed a fix for both #64391 and #64433, I believe.

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

@lqd I'll try to check the tokio-postgres exampe again.

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

It's weird because I thought I checked that the crate itself was fixed before

lqd (Sep 19 2019 at 15:51, on Zulip):

I was confused by this as well

lqd (Sep 19 2019 at 15:51, on Zulip):

the latest rev builds

lqd (Sep 19 2019 at 15:51, on Zulip):

because they added the workaround you mentioned earlier

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

I thought I reverted the workaround

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

but maybe I confused myself

lqd (Sep 19 2019 at 15:52, on Zulip):

maybe :)

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

OK, double checked

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

it appears I was confused before, but it should be good now

lqd (Sep 19 2019 at 15:57, on Zulip):

awesome :tada:

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

not mega happy with my patch though :)

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

I mean, it's ok

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

but the scope tracking mechanism feels like it could be simplified...

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

I wonder if we could insert drops instead of removing them

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

(ugh, unwinds)

centril (Sep 19 2019 at 16:01, on Zulip):

@nikomatsakis maybe a mir-opt test could be useful here?

nikomatsakis (Sep 19 2019 at 20:20, on Zulip):

maybe a mir-opt test could be useful here?

done

nikomatsakis (Sep 20 2019 at 20:38, on Zulip):

@tmandry once https://github.com/rust-lang/rust/pull/64584 hits a nightly, you may want to take another stab at fuschia

tmandry (Sep 21 2019 at 00:59, on Zulip):

@nikomatsakis interestingly, the number of instances of that error has gone up from the last time I built

tmandry (Sep 21 2019 at 00:59, on Zulip):

but I'm guessing that's probably because some crates now compile successfully, which unblocks other crates which depend on them

tmandry (Sep 21 2019 at 01:00, on Zulip):

now I'm seeing 47 instances of that error ("Foo cannot be sent between threads safely"), up from 29

tmandry (Sep 21 2019 at 01:04, on Zulip):

and zero instances of "Foo does not live long enough", down from 25

tmandry (Sep 21 2019 at 01:13, on Zulip):

I am also now seeing instances of #64636, which I think should be marked blocking

centril (Sep 21 2019 at 12:13, on Zulip):

@tmandry I think you'll have to investigate #64636 if you want it fixed before master promotion since it's so close :slight_smile:

tmandry (Sep 22 2019 at 01:47, on Zulip):

Looks like @Esteban Küber already got to it <3

Taylor Cramer (Sep 23 2019 at 20:27, on Zulip):

hey y'all

Taylor Cramer (Sep 23 2019 at 20:28, on Zulip):

sorry for being late to weigh in-- I was out last week

Taylor Cramer (Sep 23 2019 at 20:28, on Zulip):

https://github.com/rust-lang/rust/pull/64584 is the most recent update?

Taylor Cramer (Sep 23 2019 at 20:31, on Zulip):

@nikomatsakis should https://github.com/rust-lang/rust/issues/64391 be closed?

nikomatsakis (Sep 23 2019 at 20:34, on Zulip):

Probably

nikomatsakis (Sep 23 2019 at 20:34, on Zulip):

I'm not aware of any new reports or bugs

nikomatsakis (Sep 23 2019 at 20:35, on Zulip):

https://github.com/rust-lang/rust/pull/64584 is the most recent update?

correct

nikomatsakis (Sep 23 2019 at 20:35, on Zulip):

there may still be more bugs -- and of course we also know that the Send rules in particular are overly conservative

nikomatsakis (Sep 23 2019 at 20:35, on Zulip):

(we could backport further fixes if needed)

Last update: Nov 18 2019 at 01:35UTC