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

Static async fn in traits #3185

Merged
merged 8 commits into from
Dec 7, 2021
Merged

Static async fn in traits #3185

merged 8 commits into from
Dec 7, 2021

Conversation

tmandry
Copy link
Member

@tmandry tmandry commented Oct 26, 2021

This is an RFC to support async fn in traits that can be called via static dispatch. These will desugar to an anonymous associated type.

Rendered

This RFC was authored as part of the async fundamentals initiative.

@ehuss ehuss added the T-lang Relevant to the language team, which will review and decide on the RFC. label Oct 26, 2021
@ibraheemdev
Copy link
Member

I don't see anything mentioned about Sendness. Is requiring the future to be send a potential future extension?

@kennytm
Copy link
Member

kennytm commented Oct 27, 2021

i suppose that requires ability to name the type of the returned future, stated in the last section of the "Future possibilities".

trait Service2 {
    async fn request(&self, key: i32) -> Response
    where
        Self::request::Output: Send;
}

the RFC does need to state whether the default is Send or ?Send though. "letting the compiler automatically" is not an answer because the generic code don't know the impl.

async fn foo<S: Service>(s: &S) {
    let future = s.request(1);
    // can we assume `typeof(future): Send` or not?? 
    future.await;
}

@tmandry
Copy link
Member Author

tmandry commented Oct 28, 2021

i suppose that requires ability to name the type of the returned future, stated in the last section of the "Future possibilities".

That's right. There's a separate RFC being worked on to add this functionality and make it possible to write those bounds.

the RFC does need to state whether the default is Send or ?Send though. "letting the compiler automatically" is not an answer because the generic code don't know the impl.

There is no Send default. In cases like the example you gave this is rarely a problem in practice. The reason is that we leak auto traits from async fn (and any function with return position impl Trait). If you call foo with a type S where S::request::Output: Send, the future returned by foo will be Send and callers are allowed to rely on this.

The story with dynamic dispatch is more complicated; there you need explicit Send bounds. But we aren't tackling that in this RFC.

@nikomatsakis
Copy link
Contributor

That's right. There's a separate RFC being worked on to add this functionality and make it possible to write those bounds.

https://rust-lang.github.io/impl-trait-initiative/RFCs/named-function-types.html


Currently, if you use an `async fn` in a trait, that trait is not `dyn` safe. If you need to use dynamic dispatch combined with async functions, you can use the [`async-trait`] crate. We expect to extend the language to support this use case in the future.

Note that if a function in a trait is written as an `async fn`, it must also be written as an `async fn` in your implementation of that trait. With the above trait, you could not write this:
Copy link

Choose a reason for hiding this comment

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

Is there a possibility that this restriction could be later lifted in a backwards-compatible way?

This is important, because with the explicit impl Future you're able to run some preliminary logic before constructing the final future, which makes it possible for the future not to capture some of the input arguments, potentially making the future Send where it wouldn't be otherwise.

In regular async functions, it's sort of an implementation detail of the function whether it's implemented as an actual async fn or as an fn -> impl Future — while this fact "leaks" to rustdoc and elsewhere, it can be more or less flipped back and forth without breaking the public interface of the function. Whereas this design would "bake in", at the trait level, the fact that implementation functions must use the async fn syntax, even though the users of the trait don't actually care since they invoke a method and get a future from it either way.

So my concern is, would anyone actually use the async fn syntax when defining a trait, knowing that this forces an unnecessary requirement on all implementations?

Copy link

@rushmorem rushmorem Oct 31, 2021

Choose a reason for hiding this comment

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

@bugaevc IMHO, as you have already alluded to, these function signatures are not interchangeable and I don't think they should be treated as such. At a glance async fn tells me that it is fully async and not awaiting has no side effects at all. fn -> impl Future, on the other hand, may or may not have side effects. Not awaiting it may also block the thread or not. To cut the long story short, I think these function signatures provide different guarantees. A fact we might want to make explicit.

Copy link
Member

@scottmcm scottmcm Nov 2, 2021

Choose a reason for hiding this comment

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

One way that this could happen would be for impls to infer the associated type's values.

That would be nice for things other than async fn too -- one could imagine

impl Iterator for Foo {
    fn next(&mut self) -> Option<i32> { ... }
}

just working without needing to say type Output = i32;, treating the associated type kinda like it was a TAIT.

(With TAIT it could almost be written something like

type BlahBlah = impl Any;
impl Iterator for Foo {
    type Output = BlahBlah;
    fn next(&mut self) -> Option<i32> { ... }
}

which is noisier without really adding anything particularly useful to the reader.)

Copy link
Member

@ibraheemdev ibraheemdev Nov 2, 2021

Choose a reason for hiding this comment

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

Yes, I had the same idea. Inferring associated types would be very nice outside of async traits as well. The downside is that when reading a trait impl, you might have to do a bit more searching to identify a given associated type.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can allow this later by adding return position impl Trait in traits and making this feature interoperable with it (i.e., redefine the async fn in traits desugaring in terms of return position impl Trait). I think we should do so, because as you and @cramertj point out, library authors will want to know they aren't backing themselves into a corner by using this feature.

@nikomatsakis
Copy link
Contributor

@rfcbot fcp merge

Given that this has been open for 6 days without much discussion it seems like we've reached a "fixed point", so I'm going to go ahead and move to FCP. There hasn't been a lot of conversation amongst @rust-lang/lang, so this doesn't represent lang team consensus in particular. Hopefully it will spur a bit more feedback! (That said, this was calibrated to be "obviously correct next steps", so I'm hoping we can move quickly to approval.)

@rfcbot
Copy link
Collaborator

rfcbot commented Nov 1, 2021

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

No concerns currently listed.

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!

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 Nov 1, 2021
@cramertj
Copy link
Member

cramertj commented Nov 1, 2021

Thanks for writing this up! I'm excited to make progress on async fn-in-traits. I have a couple of initial thoughts below, but I don't consider either of them blocking. Overall, I'd be surprised if there weren't consensus on the basic "async fn-in-traits via static dispatch desugars to an associated type" setup.

Forward compatibility with CamelCase-d associated types

Should we consider banning associated items matching the CamelCase-d version of the trait name so that we can be future-compatible with a version of this proposal which allows the future type returned by these methods to be named this way? e.g. disallow this:

trait Foo {
    type Bar; // and maybe `BarFuture`, etc.
    async fn bar();
}

I don't know if I'd personally be in favor of actually using this approach, but it has been brought up a number of times, and I'd hate if it were written off because of some silly backwards-compatibility issue.

What users are we targeting with this early version of async fn-in-traits?

One larger concern I have: where would we advise / suggest that people use this version of async fn-in-traits? Would we suggest that fols should limit uses to, for example, mocking or other uses in "binary" or application-layer code? I think I'd personally advise against using this in widely-used libraries given that it significantly restricts the usage of traits (since you can't name the resulting type). AFAIK we don't have a plan in place for how to name the resulting future types, and it isn't obvious whether we would have a backwards-compatible path to move a trait from "unnameable future return type" to "nameable future return type", so choosing to use async fn in a library trait at this point seems both limiting and a backwards-compatibility hazard. Additionally, for higher-level application code I think i'd suggest most users opt for @dtolnay 's async_trait crate for the time being instead since it supports dynamic dispatch.

I think it'd be really unfortunate to ship async fn-in-traits and then wind up in a situation where we're advising folks not to use it. Personally, I think I'd be confident and excited and I think we'd get a positive community reaction if we could "land async fn in traits" in one big "fully usable" step, rather than piecemeal introducing features that folks have to avoid for a while due to limitations.

@rfcbot rfcbot added final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. and removed proposed-final-comment-period Currently awaiting signoff of all team members in order to enter the final comment period. labels Nov 2, 2021
@rfcbot
Copy link
Collaborator

rfcbot commented Nov 2, 2021

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

@ibraheemdev
Copy link
Member

ibraheemdev commented Nov 4, 2021

Should we consider banning associated items matching the CamelCase-d version of the trait name so that we can be future-compatible with a version of this proposal which allows the future type returned by these methods to be named this way

If we do this, we should ban the actual (camel_case) function name as well, not that anyone should be using it:

trait Foo {
    type foo;
    fn foo();
}

Personally, I think I'd be confident and excited and I think we'd get a positive community reaction if we could "land async fn in traits" in one big "fully usable" step, rather than piecemeal introducing features that folks have to avoid for a while due to limitations.

Smaller rfcs like this can get the implementation/experimentation going on nightly, and the actual stabilization can happen in one big step once everything fits together cohesively. I agree that people would be very disappointed to see such a limited version of this stabilized. Probably one of the most common use cases, awaiting an async trait method in a spawned task (e.g http request handler) is not possible because of Send bounds (assuming a work-stealing executor).

@scottmcm
Copy link
Member

scottmcm commented Nov 4, 2021

consider banning associated items matching the CamelCase-d version of the trait name

I'm skeptical of this kind of thing -- it feels wrong to have the naming conventions be part of semantics (rather than just lints) especially when we support unicode identifiers from languages without casing differences.

@tmandry
Copy link
Member Author

tmandry commented Nov 4, 2021

Forward compatibility with CamelCase-d associated types

Should we consider banning associated items matching the CamelCase-d version of the trait name so that we can be future-compatible with a version of this proposal which allows the future type returned by these methods to be named this way?

Niko has been working on the named function types RFC in another initiative which proposes a different solution to this. It too has to deal with the possibility of naming conflicts, though it would be far less common due to using the (usually lowercase) name of the function as part of the path of the type; someone would have to go against standard Rust naming conventions to get a conflict.

Assuming we go with that proposal I wouldn't be in favor of adding a rule like this, both because the potential for conflicts is reduced and because it's not specific to async fn (this type of conflict could already exist for non-async methods).

What users are we targeting with this early version of async fn-in-traits?

AFAIK we don't have a plan in place for how to name the resulting future types, and it isn't obvious whether we would have a backwards-compatible path to move a trait from "unnameable future return type" to "nameable future return type", so choosing to use async fn in a library trait at this point seems both limiting and a backwards-compatibility hazard.

You bring up some good points here. As I wrote above we are working on a plan to make the return types nameable.

Even with that proposal though, I think we should at least consider whether someone might want to migrate from the anonymous associated type to a named one. @scottmcm's comment above provides one possible mechanism for allowing traits to do so. With that said, this is really a more general issue with return position impl Trait in traits that I think would be better worked out on an RFC dedicated to that feature.

That leads to my answer for the backwards compatibility hazards you correctly point out: I think we should define this feature in terms of return position impl Trait when it is available, which should give maximum flexibility to move between the sugared async fn and desugared form (in both trait and impl code).

Additionally, for higher-level application code I think i'd suggest most users opt for @dtolnay 's async_trait crate for the time being instead since it supports dynamic dispatch.

It's true that we won't have an answer for dynamic dispatch when this RFC lands. We are actively working on design here, though there are a number of significant aspects to be worked out. In any case our intent is to enable dynamic dispatch for async fn in traits in the future, but for the RFC process at least it seemed best to start with the least controversial part of the design that we already have worked out.

Roll-out strategy

I think it'd be really unfortunate to ship async fn-in-traits and then wind up in a situation where we're advising folks not to use it. Personally, I think I'd be confident and excited and I think we'd get a positive community reaction if we could "land async fn in traits" in one big "fully usable" step, rather than piecemeal introducing features that folks have to avoid for a while due to limitations.

This is a reasonable perspective and one I don't necessarily disagree with, though I can say I know of at least one performance-sensitive user who needs async fn in traits and are eager to get away from dynamic dispatch. I think the strategy we take should come down to an empirical question around how often users need dynamic dispatch. We plan to speak to a number of production users about this as well as ask them to try out the feature before stabilizing (see stakeholders in the initiative repo).

(To be clear though, I agree with you that the conversation around how to ship the feature shouldn't block this RFC.)

@pickfire
Copy link
Contributor

pickfire commented Nov 11, 2021

Is it possible to have async fn foo and async foo in the same impl? It not, won't people that want to have both name one async fn foo_async?

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

rfcbot commented Nov 12, 2021

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.

@tmandry
Copy link
Member Author

tmandry commented Nov 20, 2021

Is it possible to have async fn foo and async foo in the same impl? It not, won't people that want to have both name one async fn foo_async?

It's not possible in this proposal. There's an extension we've considered called async overloading that would make something like this possible, but this proposal wouldn't do anything to preclude it. Also see this blog post.

@pickfire
Copy link
Contributor

It's not possible in this proposal. There's an extension we've considered called async overloading that would make something like this possible, but this proposal wouldn't do anything to preclude it. Also see this blog post.

Ah, that was the article I saw. But if we don't have it early on, then later when we introduce it, won't it cause a breakage since people need to migrate to it? Especially with crates involved that is probably gonna make stuff harder.

@tmandry
Copy link
Member Author

tmandry commented Nov 20, 2021

@pickfire One answer is that if there's a problem for migration, it's one that we already have to solve because async overloading applies equally well to free functions and inherent methods.

The details of any async overloading proposal are still hazy at this point. Migration is certainly a question that any proposal would need to consider, but it's not in scope for this RFC.

@danieleades
Copy link

incredible. when can i play with it?

@nikomatsakis nikomatsakis merged commit 69833de into rust-lang:master Dec 7, 2021
@nikomatsakis
Copy link
Contributor

Huzzah! The @rust-lang/lang team has decided to accept this RFC.

Tracking issue: rust-lang/rust#91611

If you'd like to keep following the development of this feature, please subscribe to that issue, thanks! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.