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

Delay stabilizing async closures to consider if they should return impl IntoFuture instead of impl Future #135664

Closed
eholk opened this issue Jan 18, 2025 · 15 comments
Labels
C-discussion Category: Discussion or questions that doesn't represent real issues. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@eholk
Copy link
Contributor

eholk commented Jan 18, 2025

Async closures are currently in beta for stabilization in 1.85. I'm very excited to see this feature stabilized!

That said, I would like the Lang team to consider delaying their stabilization to consider a new design detail. Delaying stabilization would amount to putting a PR in nightly that undoes the stabilization, and then doing a beta backport.

Note that this doesn't necessarily mean the lang team would be committing to making this change, but if the lang team feels there's a non-trivial chance they would prefer the IntoFuture semantics, then we should delay stabilization so there is time to consider this.

I talked with @tmandry and @traviscross about this and they did not remember discussing this particular detail during the stabilization process. If we are mistaken and this was discussed, then we should let the decision stand and not change anything.

Why should T-lang consider this change?

@yoshuawuyts recently published a blog post about generators and auto trait leakage, which argues that gen {} should return impl IntoIterator instead of impl Iterator. The key example in the post is (adapted slightly):

let iter = gen {
    let rc = Rc::new(...);
    yield 12u32;
    rc.do_something();
};

spawn(|| {
    for num in iter {
        println!("{num}");
    }
}).unwrap();

If gen {} returns impl Iterator, as it does on nightly today, this example fails to compile because the Rc is not Send. However, this is overly restrictive because the Rc is not created until the spawned thread calls next() the first time.

Instead, having gen {} return impl IntoIterator allows this to work, because the IntoIterator part could be Send, and once it becomes an Iterator it would no longer be Send.

This has a similar shape to annoyances we've seen with async Rust, where it's not uncommon for people to want to write futures that are Send until they are polled for the first time. If we had made async {} blocks return impl IntoFuture instead, this would be straightforward to do for the same reasons.

So then the next logical question is what should generator closures or async closures do? For much the same reason, it would be nice for async closures to return futures that are Send until first polled.

At this point, changing the return type for async {} would require an edition change. However, we have a brief window where we can still make this change for async closures.

Why should T-lang keep the status quo?

A lot of care has gone into the current design by @compiler-errors and others to come up with something that balances a lot of competing goals. For example, with the current design, the following example works, but it would break with this proposed change:

fn foo<F, U>(_f: F)
where
    F: Fn() -> U,
    U: Future
{}

// can be invoked with legacy, fake async closures or new, real async closures
foo(|| async {});
foo(async || {});

This is a desirable property, since a lot of existing ecosystem code is written using Fn() -> impl Future as an approximation of an async closure. If async closures returned IntoFuture then they would not work until existing code is updated to support async closures natively.

I bring this up mainly to show that it's not obvious that we actually should make this change. There are many factors in tension and there's a good chance we already have the optimal balance. We'd want to thoroughly consider any changes, so this issue is mostly about whether we want to take the time to consider those changes.

Mitigating Factors

This would still be changeable over an edition if we were to decide it's worthwhile but don't want to churn things now. If we were to make this change, we'd likely want to change the behavior of async {}. Doing both over an edition at the same time would keep the overall language more consistent.

@eholk eholk added I-lang-nominated Nominated for discussion during a lang team meeting. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jan 18, 2025
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jan 18, 2025
@traviscross traviscross added C-discussion Category: Discussion or questions that doesn't represent real issues. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jan 18, 2025
@compiler-errors
Copy link
Member

compiler-errors commented Jan 18, 2025

(edited to be a bit more coherent and also express my feelings more clearly)

I think it's frankly ridiculous that this is being brought up so late in the process, and it's incredibly disappointing and stressful to go through stabilization whiplash like this.

I hope T-lang is aware of the burden this type of shit has on contributors who spend significant time on a feature to just have it be churned after stabilization like this as soon as someone comes up with a possible blocker, regardless of how remotely relevant or even feasible it is to change at that point.

It's also pretty disappointing to read from the issue that apparently two T-lang members were consulted about this, but I had to hear about this at 7:55 PM on a Friday with no prior knowledge; I'd like to believe I'm pretty accessible, and on one hand I know there's no obligation to ever keep any one person in the loop1, but on the other hand, this really erodes my confidence in Rust's stabilization process and makes me really second guess whether my insight is valued at all here.

I'm probably not going to participate in any of this discussion out of consideration for my own mental health. I'd encourage anyone who does participate to be acutely aware of the technical limitations and burden on users that changing this design has, and the implications that last-minute blocking this feature has on delivering async closures to Rust users.

Footnotes

  1. That is to say, there's no blatant process violation here, but it sure hurts like one.

@cramertj
Copy link
Member

cramertj commented Jan 18, 2025

This is a really neat idea, and we should definitely explore it in the future!

In the meantime, I don't think it makes sense to block the stabilization of async closures. As @compiler-errors pointed out, an absolutely massive amount of work has been done in order to prepare this feature for stabilization and to exercise the details of the interface and (as mentioned above) ensure that it is compatible with existing APIs that accept Fn() -> impl Future.

The current behavior is also consistent with the current semantics of async fn and async blocks. I believe it would be more confusing to create a state where only one of these (async closures) behaves differently. I believe that the same motivations for -> impl IntoFuture apply for all of async fn, async blocks, and async closures, and that this change should be considered alongside potential changes to the behavior of those APIs.

P.S.: we'll need to investigate some ergonomic way to spell the bounds for these types-- impl Fn() -> (impl IntoFuture<Output=u32>: Send) is pretty wacky, and I don't know how to spell it with the AsyncFn syntax.

@compiler-errors
Copy link
Member

compiler-errors commented Jan 18, 2025

From a technical perspective, unlike what was discussed in rust-lang/rfcs#3668 (comment), in order to mitigate this "Rc problem", the thing returned from even concrete async closures would need to not implement Future and only implement IntoFuture, and so code as simple as...

fn main() {
    let callback = async |x| { /* ... */ };
    for item in items {
        tokio::spawn(callback(item));
    }
}

...would be breaking, as the type of callback(item) would not be a Future but would be an IntoFuture type that would need to be turned into a future immediately.

Async closures would also no longer be compatible with any existing APIs that take a "old-style"/async-closure-like Fn bound, like <F: Fn() -> Fut, Fut: Future<Output = T>>, because async closures would no longer be returning Future when called:

// My older upstream API
async fn for_each_element<Fut: Future<Output = ()>>(cb: impl Fn(Element) -> Fut) {
    let mut futs = vec![];    
    for e in /* */ {
        futs.push(cb(e));
    }
    join_all(futs).await
}

// My downstream, Rust 1.85 crate:
async fn main() {
    for_each_element(async |e| { do_something(e).await });
}

Using async closures with existing future combinators like futures-rs's Stream, and existing user-written callback code would now be impossible, and there would be a new significant difference in meaning between || async {} and || async {} on top of the existing differences w.r.t. self-borrowing (which is irreducible complexity and fine to accept IMO).

I've taken a lot of care to ensure that async closures are compatible with the existing ecosystem which has had to rely on Fn* trait bounds, and to encourage users to never have to think about whether they write async || {} and || async {}, and to allow users to migrate their APIs slowly over to async closures after the feature lands. So I don't think that this is acceptable from a user ergonomics perspective for both of the two reasons above, because we would now need to massively recontextualize how async closures fit into existing async ecosystem code which may or may not be as easy to integrate if we changed their design.

I think that this churn will be far more prevalent compared to the relatively niche case of wanting Send before but not after polling has begun. If T-lang thinks this is a problem that needs to be solved (because I do not), then care and dedicated design work should be put into a general strategy to migrate this for all async blocks/fn/closure, etc, separately from the stabilization of async closures which is consistent with existing futures today.

@workingjubilee
Copy link
Member

workingjubilee commented Jan 18, 2025

as I mentioned:

Iinterfaces based on AsRef, From, or IntoWhatever have the complication that you have erased whatever you started with and must now only have whatever you arrived at. This may make the surface API flexible but in my experience this can make it subtly harder to improve things just below the boundary of that interface later.

It seems to me that this was in fact effectively discussed, as it is isomorphic to the concern raised by programmerjake, then resolved. Then the FCP commenced (i.e. all T-lang members decided and resolved their concerns) after. Then the RFC was accepted. So we already looked at something almost identical to this problem and said "nah, not worth" for fundamentally the same reasons to make the same decision again. I think it would be surprising for us to about-face now, and I would like to see the explanation that materially distinguishes these two cases in any significant way.

In other words, why does @tmandry's answer not still hold?

Interesting question. I replied in the thread but so far I do not see a compelling reason to do this. I think the compatibility headaches mentioned above and the general inconvenience of another layer of indirection are good reasons to avoid it.

@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 18, 2025

This was explicitly brought up as a concern as part of the FCP no? a concern was registered here rust-lang/rfcs#3668 (comment) and then resolved here rust-lang/rfcs#3668 (comment). I can see atleast @tmandry @traviscross and @eholk were aware of this at time of RFC and the nebuluous set of people "in the triage meeting" talked about this. I don't see any reason why lang should be re-litigating something this late in process that has already been taken into account during the RFC process

@tmandry
Copy link
Member

tmandry commented Jan 18, 2025

Hey everyone. I think the points raised on this thread have been good ones. While I think the motivation provided on the issue is more compelling than any we considered for IntoFuture before, personally I feel convinced that we should stabilize the feature as-is, and only consider this as an edition change together with async blocks and async fn.

@eholk
Copy link
Contributor Author

eholk commented Jan 18, 2025

I would like to apologize for the way in which I raised this issue because it caused unnecessary stress, particularly on @compiler-errors. It was sloppy, and as a result, hurtful.

This is an exceptional request, and I did not do the due diligence required in making a request like this.

As a bit of background, here's how this got posted. I say this not as a defense, but so I can use that to show concrete ways I could have done better.

I was having a discussion with some folks about a design for generators, which have a lot in common with async Rust due to both being built around the coroutine transform. The question of whether gen {} should be IntoX or X came up, and this led to the parallel discussion about whether we'd do async {} differently today. We thought we likely would have, and that we probably should for closures as well. We realized we still had a small window to consider this change for async closures, which meant there was some urgency to raise the issue.

We believed this amounted to new information that could have bearing on the decision. Part of why we have a beta period is to allow this to happen, so I think raising issues like this with actual new information is appropriate. But it should be done with care and diligence that I did not show.

Given the urgency, this afternoon I drafted up an issue and chose to post it so that it could be considered at the next Lang Team meeting.

The result was that I posted the issue at around 5pm on a Friday evening. This meant @compiler-errors got to go into his weekend with me trying to roll back work that he has put a ton energy into getting across the finish line.

With that background, what could and should I have done differently?

A simple thing that I wish I had done is to hold off and post the issue after the weekend. That would have avoided the timing and also given me more time to think through a proposal of this magnitude. Had I done so, I would have either decided not to post it, or would have posted something that was stronger. While I felt there was some urgency, posting on Monday or Tuesday would have still given enough time to discuss it in the next Lang Team meeting and make a decision before the beta is promoted to stable.

Second, I did not consult with @compiler-errors before posting this issue. This was inconsiderate. Had I asked @compiler-errors, it would have meant this issue didn't come as a public surprise and would have given him the chance to respond without feeling the stress and urgency of rebutting something on GitHub. After asking him, had I gone forward with proposing the revert, I would have likely done so with his support (and if I didn't have his support, he probably would have also convinced me this change was a bad idea).

There's a principle I've heard discussed somewhere of respecting what came before. The idea is that carefully thought-out decisions that have stood the test of time and/or faced a lot of scrutiny are likely correct. Obviously, that's not always the case, but changing these decisions needs to be done with care and understanding. I could have and should have done better to live up to this principle.

I intend to learn from this and show more care in the future. I apologize to @compiler-errors for the stress and harm I have caused.

@traviscross
Copy link
Contributor

traviscross commented Jan 18, 2025

In case it wasn't clear, lang hadn't and hasn't made any decision here -- not about a beta revert and not about changing the behavior -- neither had any lang members, and as best as I understand @eholk's position at the time, nor had he either really.

The most any of us thought for sure is that it was worth discussing with the team, and that we were curious what others might make of it. As everyone here knows, I think, nothing is or was going to happen until Wednesday at the earliest.

As above, @eholk has started in on beating himself up a bit, so I want to be clear that I don't think he's at any fault. In light of @yoshuawuyts' piece, and some reasonable extrapolation, he put forward a reasonable write-up of something we on lang should reasonably consider, in the spirit of double checking things, as we try to do. He clearly put it forward in good faith and in trying to do the best thing for Rust. He CCed the likely interested parties as a necessary and appropriate courtesy, as we do. He's certainly not responsible for what we on lang might or might not do with this.

We check, double check, and yes -- sometimes second-guess ourselves. That's all this is. It normally amounts to us still doing what we planned to do.

At the same time, having certainly been on all sides of that myself, I understand how even the prospect of that can be concerning and frustrating. I get it. Certainly concern and frustration isn't what we want. That's a hard problem, and minimizing that is of course something to which we will continue to give thought and attention.

@workingjubilee
Copy link
Member

As everyone here knows, I think, nothing is or was going to happen until Wednesday at the earliest.

That doesn't make it better.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jan 21, 2025

Focusing purely on the technical issues for a moment---my recollection is indeed that we discussed the idea of returning IntoFuture (as others have cited) though I don't know that we talked through the specific example given here. (I think @workingjubilee and others cited some parts of that discussion here.)

As others have suggested, I too think it's better that we stay the course and keep the design compatible with Rust's overall shape (and especially with the behavior of async blocks). If we did want to revisit the decision, I'd prefer to do it holistically with async and other similar shapes.

That said, I am not sure the is treating "first poll" specially as the fact that we have no way to confine values to an "async task". In other words, an async task feels fundamentally different than a thread but I don't think that's the right way to think about it.

EDIT: This was phrased really poorly. What I was going to say is that the interaction of Send with async tasks is already "ungreat", and I'm not sure that the "send until first poll" problem is the root one versus Send not meaning what it ought to mean for async tasks. I also think users would like some way to have a "synchronous" prefix to an "async" task -- the current way to do that is not especially convenient -- and I wonder if that would address this use case as well.

@nikomatsakis
Copy link
Contributor

As for the procedural questions, I definitely appreciate how stressful it can be to have last minute questions raised up. I also take pride in the lang team's willingness to hear people out, even when we are close to stabilization (though the bar for changing course goes ever higher).

I don't think that @eholk did anything wrong in raising this question, but clearly something in the process of moving big features forward isn't working right (*). @compiler-errors and @workingjubilee I'd love to talk with you two specifically to get your take on where we are going awry, but also anybody else on this thread.

(*) As a starting point, I observe that it's not clear how Eric would raise concerns like this apart from opening an issue nor who he ought to reach out. It'd be nice to have a central "point of contact", perhaps. I also note that the RFC doesn't, as far as I can tell, have any discussion on the question of returning IntoFuture that lays out the rationale, even though the topic was discussed at some point. I'd like us to have a system where we make sure that those kind of decisions get recorded somewhere so that when people are looking for the rationable, they can readily find it.

@traviscross
Copy link
Contributor

traviscross commented Jan 21, 2025

even though the topic was discussed at some point

What we raised in particular was the meaning of T: AsyncFn() in a bound with respect to Future or IntoFuture. That's the concern I resolved in rust-lang/rfcs#3668 (comment). Inlined:

We discussed the question in the triage meeting today of what an F: async Fn() bound should mean. Does it mean that the type, when called, returns a type that implements Future? Or should it mean that, when called, it returns a type that implements IntoFuture?

We did feel generally that people should take IntoFuture (and IntoIterator, etc.) bounds on input arguments to functions.

But at the same time, we wanted to preserve a kind of equivalence of what async Fn* means, that it's what async closures return, and that's also what people can expect in a generic context.

That is, we decided this largely on the basis of consistency with what the literal syntax async || .. returns. The issue here is distinct, in that it asks whether we may want to resolve this by changing that too, which would likely imply wanting to eventually change async { .. } and async fn over an edition.

Of course, if we wanted, we could still do that after this stabilization, and just handle changing async closures at the same time. But on the other hand, fixing the meaning of the bound to match, even over an edition, seems like it might be hard. So anyway, it's something to discuss.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jan 23, 2025 via email

@traviscross
Copy link
Contributor

traviscross commented Jan 23, 2025

Here's how I think I'd frame the strongest version of the argument for action:

Whatever we call it, IntoFuture should have been the main trait. Fixing that today is hard but possible. Over an edition, we'd change the meaning of async fn and async { .. } to return IntoFuture with semantics-preserving edition migrations (i.e., fn .. -> impl Future { async { ... } } and async { .. }.into_future()). It'd take some time for the ecosystem to relax bounds, where those were too tight, but that could happen gradually.

Conversely, once we stabilize async closures, it's going to be a lot harder to ever fix that, for these reasons:

  • The bound, part 1: Changing the meaning of a T: AsyncFn(..) -> .. bound over an edition seems hard. There's no general semantics-preserving way currently to desugar this bound.

  • Prevalence: The || async { .. } / F: for<..> Fn(..) -> Fut, Fut: Future<..> trick never really worked -- that's why we're doing async closures -- and so the prevalence of this hack in current APIs is limited. Once we stabilize async closures, these are actually going to work, and there's going to be an explosion of use. However much is out there now, there's going to be N times more of it after it actually works, and N may be large. This will complicate any later changes.

  • The bound, part 2: This explosion of use will result in an explosion of too-tight bounds due to the meaning of T: AsyncFn() as a callable that returns Future rather than IntoFuture. There's no way at all for users to express the usually-correct looser bound. This will narrow our options further.

  • The bound, part 3: If we can't fix the bound, then we'll never change the meaning of the literals, because that would be inconsistent.

Whatever we do for async, we're going to be under pressure to do that for gen as well, for consistency. So this choice, whatever it is, is likely to permeate the language.

Is that a strong enough reason to act? I don't know. Delaying async closures would break my heart for the same reason that stabilizing them is going to make later changes hard -- they're going to enable a big new attractive capability that a lot of APIs will build around. I find myself having to consciously suppress my own feelings about how much I too want that soon so as to try to weigh this evenly.

The strongest version of the argument that we shouldn't act, I think, boils down to the idea that we're already locked in -- that the presence of || async { .. } and existing bounds and whatnot is something that we must be maximally compatible with to do this at all. I'm still trying to decide the degree to which that might or might not be true. I weigh this in particular against the fact that what's out there today may be only a small fraction of what will be out there once this actually works.

Anyway, whatever we decide, it seems a likely worthwhile discussion. Increasingly, I think that in doing this, we may be rather firmly closing a door to a degree greater than we may have realized. That may still be OK. But then it'll be worth taking a final moment to mourn Into* (under any name) as the right approach that we may just never be able to achieve.1

cc @rust-lang/lang

Footnotes

  1. Paradoxically, if we do close that door, I think the door that opens is for async (and gen) thunk closures to be the solution. That is, T: AsyncFn() becomes the new T: IntoFuture, and people get used to writing async || { .. } where they might have instead written async { .. } in an IntoFuture world. We could even consider implementing IntoFuture for thunk async closures.

@traviscross
Copy link
Contributor

traviscross commented Feb 5, 2025

@rustbot labels -I-lang-nominated

We discussed this in triage last week and then again today. We'll be going ahead with the stabilization of async closures as planned, and of course we're all rather excited to see that feature stabilize and to see the many things that people will surely build on it. We're of course all grateful to @compiler-errors and others who have put in the substantial effort to push this forward.

We had a good discussion about IntoFuture, the result of which is that probably we think that AsyncFnOnce is in fact a better IntoFuture, so we're not really losing anything. We might later think about implementing IntoFuture for thunk async closures.

One interesting thread we explored is the question of how spawn should be ideally implemented for a thread-per-core executor. In that model, we want to give it something that cannot necessarily be sent across threads once it has been polled but can be before that, and it'd be good if the implementation doesn't need to box the thing twice. After analysis, we decided that this can be done just fine with AsyncFnOnce.

@traviscross traviscross closed this as not planned Won't fix, can't repro, duplicate, stale Feb 5, 2025
@rustbot rustbot removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-discussion Category: Discussion or questions that doesn't represent real issues. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants