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

Async closures #3668

Merged
merged 25 commits into from
Aug 3, 2024
Merged

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Jul 1, 2024

This RFC adds an async bound modifier to the Fn family of trait bounds. The combination desugars to a set of perma-unstable AsyncFn{,Mut,Once} traits that parallel the current Fn{,Mut,Once} traits.

These traits give users the ability to express bounds for async callable types that are higher-ranked, and allow async closures to return futures which borrow from the closure's captures.

This RFC also connects these traits to the async || {} closure syntax, as originally laid out in RFC 2394, and confirms the necessity of a first-class async closure syntax.

Rendered

For additional background, see this blog post (which is referenced in the RFC):

Tracking:

@traviscross traviscross added the T-lang Relevant to the language team, which will review and decide on the RFC. label Jul 1, 2024
@compiler-errors compiler-errors added A-sync Synchronization related proposals & ideas A-closures Proposals relating to closures / lambdas. WG-async Relevant to the async working group. I-lang-nominated Indicates that an issue has been nominated for prioritizing at the next lang team meeting. A-async-await Proposals re. async / await and removed A-sync Synchronization related proposals & ideas labels Jul 1, 2024
@traviscross traviscross changed the title Add async_closure RFC Async closures Jul 1, 2024
@traviscross traviscross removed the WG-async Relevant to the async working group. label Jul 1, 2024
@traviscross
Copy link
Contributor

traviscross commented Jul 1, 2024

@rfcbot fcp merge

This important work has been a long time coming. The RFC is well motivated, well written, and well specified. This solves a real problem, and by solving it, we'll be supporting libraries to innovate in new and interesting ways across the async Rust ecosystem.

By carefully limiting what is exposed as part of the stable interface, the RFC leaves us future room to maneuver.

Thanks to @compiler-errors for putting together this RFC and the currently-experimental implementation.

@rfcbot
Copy link
Collaborator

rfcbot commented Jul 1, 2024

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. labels Jul 1, 2024
@traviscross traviscross added the WG-async Relevant to the async working group. label Jul 1, 2024
@programmerjake
Copy link
Member

I think you should use IntoFuture instead of Future here, since it will allow passing more functions as impl async Fn(..) -> .. and will also allow the idea for replacing Pin with !Move as outlined here

@traviscross
Copy link
Contributor

@programmerjake: That's a great point to raise.

@rfcbot concern should-it-be-into-future

@compiler-errors
Copy link
Member Author

I think you should use IntoFuture instead of Future here, since it will allow passing more functions as impl async Fn(..) -> ..

Yeah, sure, that's definitely possible from a technical perspective. For example, I've implemented the change here: rust-lang/rust#127225. I'll wait a bit to see what T-lang thinks, though, before changing the RFC.

There's a bit of friction introduced by making these traits harder to use with the ecosystem when calling functions that take Futures. For example, tokio::spawn currently takes F: Future and not F: IntoFuture, so you'll need to call my_closure().into_future() in generic code, like:

async fn foo(closure: impl async Fn()) {
  spawn(closure().into_future()).await;
//                ^^^^^^^^^^^
}

And similarly, all of the FutureExt and StreamExt combinators currently have Future and not IntoFuture bounds. Ideally we change this to use async FnOnce() -> T eventually, but there will be some growing pains until that becomes possible.

The only other question is whether the concrete future returned by calling a concrete async closure should also only implement just IntoFuture and not Future. This honestly would made using async closures a bit annoying compared to regular async functions (which when they're called, they return Futures), so I would hestitate to make that specific change until we've decided to do it for async fn as well. Changing all of them will likely have to happen over an edition.

@eholk
Copy link
Contributor

eholk commented Jul 1, 2024

In my experience lately, it's generally more flexible to use the Into* variants of various traits, so I've started encouraging people to do that. But the * version is often a lot more natural so I often forget to use it. In the ideal world, it seems like having async closures and async fn return IntoFuture would be better.

I think @compiler-errors raises some good points about the migration. Lots of code is already written to take a Future, so we'd be adding a little friction there.

I also agree that having asymmetry between async fn and async || would not be great.

So given the state of things, it's probably better to treat the return IntoFuture question as a separate one and handle all the migration concerns at once. Since we've already shipped async fn, we aren't really making things harder for ourselves if we ship async || now.

Is there a way we could signal to the ecosystem that we'd like them to migrate towards taking IntoFuture instead of Future where possible, so that it's more feasible we could make this change later?

@tmandry
Copy link
Member

tmandry commented Jul 2, 2024

I think this RFC is great as-is. Thanks for writing this RFC and the implementation, @compiler-errors!

@rfcbot reviewed

I think you should use IntoFuture instead of Future here, since it will allow passing more functions as impl async Fn(..) -> .. and will also allow the idea for replacing Pin with !Move as outlined here

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.

@workingjubilee
Copy link
Member

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.

@tgross35
Copy link
Contributor

tgross35 commented Jul 2, 2024

const Fn could be listed under future possibilities if there is anything from the type syntax interaction that could be used there. Also there will be some interaction with async trait and other effects-related concepts (cc @fee1-dead).

@compiler-errors
Copy link
Member Author

const Fn could be listed under future possibilities if there is anything from the type syntax interaction that could be used there.

I don't really think mention const as a trait bound modifier is really in the scope of this PR, or at least I don't see anything that's important enough to warrant mentioning. The way that async Fn*() desugars into AsyncFn* operates really differently than how const/~const trait bound modifiers work.

Also there will be some interaction with async trait and other effects-related concepts (cc fee1-dead).

This RFC specifically chooses not to generalize the async bound modifier to any traits other than Fn*, and even the specific desugaring remains an implementation detail, so I don't believe there are any interactions with other effects at the moment (constness is the only thing that could arguably be called an effect in the compiler, but I don't see any interactions with it either, at least past how it operates on any other trait).

@Jules-Bertholet
Copy link
Contributor

Jules-Bertholet commented Jul 2, 2024

Why not F: AsyncFn() -> T, naming AsyncFn* directly?

Reusing the async keyword allows users to understand what an async Fn() -> T trait bound does by analogy, since they already should know that adding async to some fn foo() -> T makes it return an impl Future<Output = T> instead of the type T.

I don’t think this async Fn() sugar pulls its weight versus AsyncFn().

  • It’s an ad-hoc special case, doesn’t fit with some wider language feature, and might even conflict with a hypothetical future effects/keyword generics feature
    • Users might be misled into believing some arcane keyword generics ritual is being invoked, even though nothing of the sort is happening
  • It mentions the wrong trait, misleading anyone who tries to look for it in rustdoc
  • It carries no real semantic distinction, as opposed to the Fn() -> parens/arrow sugar which signals different lifetime elision behavior
  • It's 1 char longer than the desugaring
  • Users aren’t stupid, they’ll understand that AsyncFn is like Fn but async
  • It’s backward-compatible to add later

@compiler-errors
Copy link
Member Author

compiler-errors commented Jul 2, 2024

It's an ad-hoc special case, doesn't fit with some wider language feature, and might even conflict with a hypothetical future effects/keyword generics feature

If we did introduce an async bound modifier with a different set of semantics than how users observe async Fn(), that would probably suggest there's a problem with the keyword generics feature, not async Fn().

We're reserving quite a lot of space for async Fn()'s desugaring to change in the future specifically by reserving the trait definition, and while we should be mindful of future extensions, I don't think stabilizing AsyncFn as the name now to save space for a potentially different async Fn() (with possibly different semantics in the future?!) is the right move.

It mentions the wrong trait, misleading anyone who tries to look for it in rustdoc

I'm not super convinced that users (except for the curious ones) really care about looking at the Fn* trait definitions themselves for the purposes of learning. When they do look up Fn* in rustdoc, I assume they care more about the specific docs that discuss, e.g., the relationship between the traits and their restrictions for calling, rather than the exact trait definition, since none of the trait's methods (or its extern "rust-call" calling convention) are even usable directly.

If we find out that users are confusingly going to the FnOnce docs for async FnOnce, we can mention in the rustdocs for FnOnce that async is a trait bound modifier for it and link to AsyncFnOnce. Something like "FnOnce takes an optional async keyword that currently desugars to [[link to the AsyncFnOnce trait]]".

It carries no real semantic distinction, as opposed to the Fn() parens sugar which signals different lifetime elision behavior

The paren sugar does not exist just to signal different lifetime elision behavior. It's pretty clear to me that it exists to give people an intuitive way to write Fn-like bounds with Fn-like syntax. Like, it's the only syntax that allows ->.

The lifetime elision behavior is a useful side effect that mirrors how fn pointers and closures' lifetime resolution work, but that's obviously not like the only reason to have them.

It's 1 char longer than the desugaring

And I could make an equally useful argument that Async is slighly harder to write than async because you have to press the shift key to capitalize the A character hahaha

Users aren't stupid, they'll understand that AsyncFn is like Fn but async

I think it's uncharitable to suggest that we're only doing this because we think that users are stupid -- that feels kinda inflammatory. Of course users can read and parse AsyncFn into "async fn". That doesn't mean that there isn't fundamental usefulness in drawing analogy between the async keyword in various positions, and I think that the analogy between async Fn, async fn, and async || here is very favorable.

It's backward-compatible to add later

It is not backwards-compatible to stabilize AsyncFn* without possibly putting us into the situation of needing adding new features to the language.

If we stabilize AsyncFn* and we later want to, e.g., unify the traits into LendingFn/LendingFnMut/FnOnce, we'd need to design some system of trait aliases with associated types to emulate the AsyncFn*trait users are moving off of. That's something I'm particularly keen to avoid unless it's necessary.

@traviscross
Copy link
Contributor

traviscross commented Jul 2, 2024

Regarding IntoFuture, what seems at least on the table is for T: async Fn() bounds to mean that T is a type that, when called, returns a type that implements IntoFuture (rather than Future).

Concrete async closures themselves, e.g. async || (), would still, when called, return some type that implements both Future and IntoFuture.

This is what @compiler-errors put up in the PR rust-lang/rust#127225 for discussion.

The rationale for this would be that, in the case of concrete async closures, we know that they do implement both Future and IntoFuture, and so it makes sense to "offer callers the most we can offer." But in bounds, it's reasonable to want to "ask of callers the minimum that we can ask." That is, there's a kind of contravariance / subtyping argument to be made here.

And unlike with the concrete async closures, where there's a comparable for us to want to be consistent with (async {} blocks), the bounds are something new, so we have some greater flexibility to do the right thing, whatever that turns out to be.

text/3668-async-closure.md Outdated Show resolved Hide resolved
text/3668-async-closure.md Outdated Show resolved Hide resolved
The feature gate for async closures is unfortunately in the singular,
`async_closure`, and while that's not a stable commitment, it's
probably too much of a pain to change that.  But that doesn't mean
that we can't name the file in the plural, so let's do that.
As it turns out, `AsyncFn()` probably commits our direction on
generalized `async` trait bound modifiers to the same degree as `async
Fn()`.  Let's describe this.

CE brought up this excellent point.

Some have argued that using `F: AsyncFn() -> T` rather than `F: async Fn() -> T` would better save space for whatever we might want the semantics to be if we were to later adopt generalized `async` trait bound modifiers.

We don't think this is correct in a practical sense. If we were give meaning to and stabilize `AsyncFn()` trait bounds, and then later, due to work on generalized `async` trait bound modifiers, we were to give a different meaning to `async Fn()`, that would seem so undesirable that it's hard to imagine we would ever actually do it. People would too strongly expect `AsyncFn()` and `async Fn()` to work in the same way.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about a future where async Fn() still has no meaning, but async SomeOtherTrait does, in a way that is not analogous to AsyncFn()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then those meanings can coexist since by definition they don't overlap.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also feel like at this point it's not productive to debate against a "what if" that's quantified over a space of possibilities that's not really enumerable. I'm happy to say that any async Trait design that conflicts with at least the broad, user-facing observables of the definition of async Fn is not worth considering.

add the note in one other place

Co-authored-by: Josh Triplett <[email protected]>
@joshtriplett
Copy link
Member

The concern about the proposed async Fn syntax has been captured in the RFC as an unresolved question, together with a note ahead of the first use of that syntax making it clear it's unresolved. Furthermore, @compiler-errors stated that it'd be straightforward to have both syntaxes work on nightly behind the same feature gate, and that any blog posts calling for testing that occur before this question is resolved will not favor one syntax over the other, to avoid creating inertia behind one syntax. On that basis, I'm deferring that question as something that can be settled before stabilization, and resolving the concern:

@rfcbot resolved use-AsyncFn-rather-than-async-Fn

@rfcbot rfcbot added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Jul 23, 2024
@rfcbot
Copy link
Collaborator

rfcbot commented Jul 23, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. label Jul 23, 2024
@joshtriplett
Copy link
Member

joshtriplett commented Jul 23, 2024

For reference, to make sure details discussed in a lang design meeting get reflected here: we also discussed several alternative syntactic sugars (including async fn(Args) -> T, async(Args) -> T, and async |Args| -> T, with mut or once in appropriate places), in addition to the async Fn*(Args) -> T and AsyncFn(Args) -> T syntaxes mentioned in the RFC. None of these syntaxes had full consensus, and in the meeting we did not have much hope that any other different syntactic sugar other than those in the RFC would be likely to reach a consensus.

@traviscross traviscross removed the I-lang-nominated Indicates that an issue has been nominated for prioritizing at the next lang team meeting. label Jul 24, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 24, 2024
…li-obk

Gate `AsyncFn*` under `async_closure` feature

T-lang has not come to a consensus on the naming of async closure callable bounds, and as part of allowing the async closures RFC merge, we agreed to place `AsyncFn` under the same gate as `async Fn` so that these syntaxes can be evaluated in parallel.

See rust-lang/rfcs#3668 (comment)

r? oli-obk
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 24, 2024
Rollup merge of rust-lang#128120 - compiler-errors:async-fn-name, r=oli-obk

Gate `AsyncFn*` under `async_closure` feature

T-lang has not come to a consensus on the naming of async closure callable bounds, and as part of allowing the async closures RFC merge, we agreed to place `AsyncFn` under the same gate as `async Fn` so that these syntaxes can be evaluated in parallel.

See rust-lang/rfcs#3668 (comment)

r? oli-obk
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Jul 25, 2024
Gate `AsyncFn*` under `async_closure` feature

T-lang has not come to a consensus on the naming of async closure callable bounds, and as part of allowing the async closures RFC merge, we agreed to place `AsyncFn` under the same gate as `async Fn` so that these syntaxes can be evaluated in parallel.

See rust-lang/rfcs#3668 (comment)

r? oli-obk
@yoshuawuyts
Copy link
Member

There have been some questions raised both in this thread and other places about the forward-compatibility of this design with a possible Effect Generics design. The Effects Initiative has discussed this and as far as we can tell, this RFC does not pose any forward-compatibility concerns. From the Future Possibilities section of this RFC:

async bound modifier on arbitrary traits

There has been previous discussion of allowing async trait bounds on arbitrary traits, possibly based off a ?async maybe-async genericity system.

This RFC neither requires this more general extension to the language to be implemented, nor does it necessarily preclude this being an eventual possibility, since AsyncFn* remains unstable to implement.

We agree with this assessment. This RFC only expands the use of the async keyword as a modifier to existing language items. It crucially does not expose details about any underlying traits. As written this design neither closes any doors to-, nor requires a further generalization of async in trait bounds or declarations. From an Effect Generics perspective, we're happy with this RFC.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this RFC. to-announce and removed final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. labels Aug 2, 2024
@rfcbot
Copy link
Collaborator

rfcbot commented Aug 2, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@traviscross traviscross merged commit 9523834 into rust-lang:master Aug 3, 2024
@traviscross
Copy link
Contributor

The lang team has accepted this RFC, and we've now merged it.

Thanks to @compiler-errors for pushing forward this important work, and thanks to all those who reviewed the RFC and provided useful feedback.

For further updates, please follow the tracking issue:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await Proposals re. async / await A-closures Proposals relating to closures / lambdas. disposition-merge This RFC is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this RFC. T-lang Relevant to the language team, which will review and decide on the RFC. to-announce WG-async Relevant to the async working group.
Projects
None yet
Development

Successfully merging this pull request may close these issues.