Initial support for return type notation (RTN)

[compiler-errors]

See: https://smallcultfollowing.com/babysteps/blog/2023/02/13/return-type-notation-send-bounds-part-2/

1. Only supports T: Trait<method(): Send> style bounds, not <T as Trait>::method(): Send. Checking validity and injecting an implicit binder for all of the late-bound method generics is harder to do for the latter.
   • I'd add this in a follow-up.
2. Doesn't support RTN in general type position, i.e. no let x: <T as Trait>::method() = ...
   • I don't think we actually want this.
3. Doesn't add syntax for "eliding" the function args -- i.e. for now, we write method(): Send instead of method(..): Send.
   • May be a hazard if we try to add it in the future. I'll probably add it in a follow-up later, with a structured suggestion to change method() to method(..) once we add it.
4. I'm not in love with the feature gate name 😺
   • I renamed it to return_type_notation ✔️

Follow-up PRs will probably add support for where T::method(): Send bounds. I'm not sure if we ever want to support return-type-notation in arbitrary type positions. I may also make the bounds require .. in the args list later.

r? @ghost

[compiler-errors]

r? compiler

cc @rust-lang/wg-async

I think @nikomatsakis is going to create a lang-team initiative for this (or perhaps expected me to? can't tell from the convo...), but for now, I can put this up for review for the compiler side.

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[eholk]

Looks good to me. Feel free to add the doc comments I mentioned or not (up to you!) and then r=me afterwards.

[compiler-errors]

@bors r=eholk

[bors]

📌 Commit bc95b6ccd222ed4833f9a316d08b55ea5fccbc92 has been approved by eholk

It is now in the queue for this repository.

[lcnr]

I personally also somewhat strongly dislike this approach. Arguing mostly against the feature as presented in https://smallcultfollowing.com/babysteps/blog/2023/02/13/return-type-notation-send-bounds-part-2/.

I agree with the premise of the "send bound" problem and believe this is something we have to fix. I do however see fairly strong negatives of the current approach and believe that most of them can be avoided by using a far smaller extension of the language.

The negatives of RTN

where-bounds are already fairly complex, so adding yet another kind of bound will always has a fairly high cost. The syntax is unusual. It isn't really an expression but it's also not "regular type syntax". I tend to assume that syntax is fairly hard to search for, so when a developer reads existing code using H::check(..): Send it will not be trivial to learn the meaning of that.

I assume that most developers do not have a clear understanding of async functions as something that returns a state machine when called. I think that the intuitive understanding tends to end at "If I use an async function, I have to use .await". So for them, it is far from obvious that H::check(..): Send talks about the anonymous state machine returned by check instead of the actual final value after using .await. This is mentioned in the blogpost.

Because of the above point, the design of RTN will always be suboptimal for ordinary functions. If we allow it and H::sync_check(..): Send talks about the returned value, it is confusing, especially because from the syntax it is not clear whether sync_check is async or not. I don't think we can forbid RTN for regular trait functions as that would be inconsistent and this feature should usable to talk about RPITIT.

I also dislike the complexity necessary to deal with generic functions. The proposal requires yet another syntax extension, inferring the generic parameters for the function from by specifying the expected argument types. This again is unfamiliar and different from how generic parameters of methods are handled in any other context. Considering that the syntax for RTN is something between type and expression syntax, I think that both requiring the turbofish and not requiring it will end up feeling slightly weird.

I also think that the restriction to only use RTN in where-clauses is yet another indicator that this is not ideal.

A not yet mentioned alternative (as far as I know)

As stated above, I think RTN has a very high cost which may however be justified if there is no sensible alternative. While I haven't worked out all the details or the final syntax, here's a general idea I would prefer.

The problem statement is: we need the ability to somehow talk about the future returned by async functions in traits. As stated in Niko's post the reason to use RTN is that we do not want to expose details about this desugaring which we don't want to be stable.

My proposal is that the authors of traits have to explicitly bind the returned future of an async function to an associated type of the trait. This can be done by using an attribute¹. An example would be the following:

trait HealthCheck {
    // idk about `'anon`, need to provide some way to talk about the anonymous lifetimes
    // in the return type of `check`.
    type CheckFut<'anon>;
    // The attribute name is a placeholder.
    #[defined_by_returned_future(CheckFut)]
    async fn check(&mut self, server: Server);
}

Adding bounds on the return future returned by check can now simply use the existing syntax of HealthCheck. Dealing with generic methods therefore does not add new complexity. The main new concept is the defined_by_returned_future attribute which is easier to search for and also only required in the trait definition. You do not have to interact with this when only using the trait.

It does not have to deal with the difference between sync and async functions, as it is explicit opt-in for async. We also do not have to make up some name for the associated type.

Let's end by going through the arguments for RTN in the post.

The most obvious detail is “what is the name of the associated type” — I think the only clear choice is to have it have the same name as the method itself, which is slightly backwards incompatible (since one can have a trait with an associated type and a method that has the same name), but easy enough to do over an edition.

Solved by having the author explicitly define the associated type.

We would also have to expose what generic parameters this associated type has.

Regardless of how we expose the lifetimes, the impl Trait argument also raises interesting questions.

This is a valid advantage of RTN, but I believe the negatives still far outweigh that benefit. I think it's fine to start by not allowing APIT impl Trait together with the defined_by_returned_future attribute.

Footnotes

1. this is similar to the currently discussed approach of #[defines(SomeAlias)] for TAIT. ↩

[marziply]

@lcnr

I suspect you are misunderstanding the problem space here and what RTN has been chalked up to solve. Your criticisms of the syntax are valid in my opinion, even though personally I quite like it, but your points on explicitly binding to the associated type defeat the point of what is trying to be solved here. The issue, as far as I understand it, is the variability of Send bounds at the call site. As a caller of an asynchronous function, perhaps a Send bound, or any bound for that matter, is not relevant to me. In a separate function, perhaps the opposite is true, and it's critical that I need a particular bound. Shifting the burden to the author rather than the caller places us in the same place that foo() and foo_mut() does. With your proposed solution, I would expect authors to provide two functions for callers that need both functionality, something like foo() and foo_send(). This is far from ideal and simply produces more boilerplate that I believe isn't worth focusing on.

Regarding the syntax of H::check(..), I agree that it's not immediately obvious that it's for the generated future. Having said that, I do think we should continue to focus primarily on the call site of the future. Whether the future is constrained by generics or otherwise specified directly on the call site, I'm not sure. I don't want to bikeshed too much here but has anyone considered something like health_check.check(server).await!;? Note the ! here - in this context perhaps this means as the caller, you are enforcing some bound, maybe a bound that's defined in where. I'm not sure what shape that would come in, it's just an idea that I've had just now.

Anyway, as I've mentioned, I do actually quite like the syntax of H::check(..). I suspect there could be improvements to it but as an MVP to test with, I think it's worth merging.

[lcnr]

The issue, as far as I understand it, is the variability of Send bounds at the call site. As a caller of an asynchronous function, perhaps a Send bound, or any bound for that matter, is not relevant to me. In a separate function, perhaps the opposite is true, and it's critical that I need a particular bound.

This seems like my explanation might not have been as clear as I had hoped.

With the attribute provided in the trait definition, all bounds using H::check(..) with RTN can instead be represented using H::CheckFut<'_> instead.

The goal is to replace complexity at use sites - new syntax and imo inconsistent behavior - with complexity at the definition of the trait. By forcing the trait definition to provide a name for the return type of H::check(..) you can add all bounds you would have been able to using RTN using a frequently used and already stable feature: associated types.

[nikomatsakis]

Thanks @lcnr for the feedback. I'll add some notes to the tracking issue and we can carry out further discussion going forward. As I noted above, I don't see these concerns as a reason to block this PR from landing per se, but I definitely think we should dig into them further before authoring any RFCs.

(For now, I've linked to the comment from the tracking issue.)

[rfcbot]

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.

[oli-obk]

@bors r=eholk

[bors]

📌 Commit 8b592db has been approved by eholk

It is now in the queue for this repository.

[bors]

⌛ Testing commit 8b592db with merge 7402519...

[bors]

☀️ Test successful - checks-actions
Approved by: eholk
Pushing 7402519 to master...

[rust-timer]

Finished benchmarking commit (7402519): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.7%	[-1.7%, -1.7%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.2%	[2.2%, 5.2%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.6%	[-2.8%, -2.3%]	2
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.