-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
RFC: Return Type Notation #3654
base: master
Are you sure you want to change the base?
Conversation
There are multiple ways we could write this where-clause, varying in their specificity... | ||
|
||
* `where C::capture(..): Send` -- this indicates that `C::capture()` will return a `Send` value for any possible set of parameters | ||
* `where C::capture(&mut C, i32): Send` -- this indicates that `C::capture()` will return a `Send` value when invoked specifically on a `&mut C` (for the `self` parameter) and an `i32` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't this bring back the ambiguity with Fn()
? for example when we have fn capture() -> impl Future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, it would. fn f() where C::capture(&mut C, i32): Send {}
is already syntactically legal today. The parentheses denote parenthesized type arguments which may be attached to arbitary type paths (and it also comes with turbofish for expression and pattern paths: C::capture::(&mut C, i32)
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is syntactically legal, yes, but not meaningful semantically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Semantically as well if C
is a module but yeah, I don't think it can actually meaningfully clash if C
is a type parameter since types and modules live in the same namespace.
//@ edition: 2015
//@ check-pass
mod C { pub use std::ops::Fn as capture; }
fn f() where C::capture(): 'static {}
|
||
We encountered a number of problems with this design. | ||
|
||
### If the name is implicit, what name should we use? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could also use one of those #
namespace such as fn#widgets
or return#widgets
or result_of#widgets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I'll add that as a possibility
I would like to propose a tweak to the desguaring of RTN, which simplifies the limitation to "only in the The RFC describes the By comparison, while normal associated types do support a projection syntax, they also crucially support a syntax for naming and unification. For example, given the bound This is sort of the type level analog to So the tweak I propose is this: This opens the door for |
Thanks kennytm! Co-authored-by: kennytm <[email protected]>
I like the suggestion of It seems to me to be more of a change in how we think about things, is that correct? |
I believe that is correct, because of the limitation to It does also change how we might relax that limitation in the future, though- if trait DataFactory {
async fn load(&self) -> Data;
}
fn load_data<D: DataFactory<load(..) -> L>, L>(data_factory: D) {
let load_future: L = data_factory.load();
await_future(load_future);
}
fn await_future<D: DataFactory<load(..) -> L>, L>(argument: L) -> Data {
argument.await
}
struct Wrap<'a, D: DataFactory<load(&'a D) -> L>, L> {
load_future: L, // the future returned by `D::load`.
} This does look like it would subtly change some quantifier scopes, but maybe they would wind up being closer to what you would get if you wrote this out by hand with generic associated types? |
The RFC is written with RTNs acting as a kind of "pseudo-associated type"1. The idea is that you can think of traits as having an internal I agree with your suggestion that should also mean being able to do But I don't quite follow why we would want to rewrite the I guess my point of confusion is this paragraph that you wrote:
Again here I think that making the Footnotes
|
Ah, I could have spelled that out more clearly- the issue I see is that if It's probably a good idea to have a projection syntax as well, but since the RFC limits the |
The RFC doesn't limit it to bounds, it limits it to "self position" -- i.e., the RFC allows for I see your point about consistency, but there are other kinds of consistency to consider, e.g., I think I will add this as an "unresolved question". I expect we will stabilize the bound form first anyway for implementation reasons. |
Right, this is all I meant by "limited to bounds." I was thinking of I guess if we consider that form to be rare enough (i.e. we expect everyone to use |
The fn start_health_check<H>(health_check: H, server: Server)
where
H: HealthCheck + Send + 'static,
for<'a> typeof for (health_check: &'a mut H, server: Server) H::check(health_check, server): Send, or even fn start_health_check<H>(health_check: H, server: Server)
where
H: HealthCheck + Send + 'static,
for<'a> typeof for (health_check: &'a mut H, server: Server) health_check.check(send): Send, This avoids the need for a |
Co-authored-by: zachs18 <[email protected]>
github markdown makes newlines significant
@bluebear94 I pushed a variant of your proposal using |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as off-topic.
This comment was marked as off-topic.
@rfcbot fcp merge This proposal has been under discussion for a long time and, in this first week, all the feedback has been incorporated. I'm going to go ahead and kick off the checkboxes towards final-comment-period. |
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! cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. |
I'd love to summaries here discussion around these couple of posts:
While I don't think this summary should affect the specific decision on this RFC, I think it helps The main motivation for RTN is to be able to say that a particular future is Send. We care about Peeling Rust specific abstractions, the picture is this: We have an asynchronous computation. The computation is suspended, meaning that it's not actually
It is important to realize that this claim is actually wrong. It is totally fine to migrate async
Non-the-less, it would be wrong to just remove async fn sneaky() {
thread_local! { static TL: Rc<()> = Rc::new(()); }
let rc = TL.with(|it| it.clone());
async {}.await;
rc.clone();
} One can imagine alternative world, where:
In that world, both work-stealing and thread-per-core executors would work fine with non- In other words, this is an instance of a classic Rust problem of composing unsafe abstractions. The two unsafe abstractions here are:
Each can be made safe in isolation, but the combination of the two end up being unsound. From where I stand, I am 0.85 sure that we've picked the wrong one of the two in this case: I'll That being said, I don't see a reasonable way we can rectify the mistake while maintaining backwards So it seems reasonable to accept this part of Rust as is, and instead try to address the |
I'm unsure how one continues to write safe abstractions with such a proposed change to |
Hi @matklad,
I agree that `Send` is not a perfect fit for async tasks as is. Ideally we would (for example) distinguish `Rc` values that were given to the task initially from `Rc` created within the task itself. I'd like to explore how to do that. However, I don't think that really changes the need for this RFC. It is possible to create tasks that are "tightly tied' to the current thread in other ways. For example, a task might introduce something into thread-local data and then retain a raw pointer to that information. This is a legitimate optimization that I've seen a lot of systems use to enable caching without the need for expensive synchronization.
…On Fri, Jun 14, 2024, at 1:46 PM, Alex Kladov wrote:
I'd love to summaries here discussion around these couple of posts:
• https://blaz.is/blog/post/lets-pretend-that-task-equals-thread/
• https://matklad.github.io/2023/12/10/nsfw.html
While I don't think this summary should affect the specific decision on this RFC, I think it helps
to illuminate the surrounding context.
The main motivation for RTN is to be able to say that a particular future is Send. We care about
`Send`ness of futures because, roughly, only `Send` futures work with work-stealing executors.
Peeling Rust specific abstractions, the picture is this:
We have an asynchronous computation. The computation is suspended, meaning that it's not actually
executing, and instead is just a bunch of memory waiting for a wake up. Specifically, this memory is
roughly "local variables live across a yield point". It *seems* that the safety claim here is:
> To safely migrate this computation to a different OS-thread, all suspended state must be `Send`.
>
It is important to realize that this claim is actually wrong. It is totally fine to migrate async
tasks across threads even if they hold onto non-`Send` data across the yield point. Two proofs by
example here are:
• OS-threads, which can hold onto all kinds of non-`Send` data before a blocking syscall, and end up
being woken up on a different CPU core after syscall returns.
• Goroutines, which similarly can use local non-thread safe data and migrate between OS threads.
Non-the-less, it would be wrong to just remove `Send` bounds from work-stealing executors --- while
they are not needed to protect us from general data races, they end up outlawing one very specific
scenario where unsoundly-bad things can happen --- smuggling non-thread-safe data between threads
using thread locals. The following example breaks if we just remove `Send` bounds:
async fn sneaky() {
thread_local! { static TL: Rc<()> = Rc::new(()); }
let rc = TL.with(|it| it.clone());
async {}.await;
rc.clone();
}
One can imagine alternative world, where:
• `Send` and `Sync` traits are not definitionaly dependent on OS threads.
• There's only unsafe API for accessing thread locals, with one part of the contract being "make
sure that the data you get from a thread local doesn't end up in a different OS thread".
In that world, both work-stealing and thread-per-core executors would work fine with non-`Send`
futures. So, there wouldn't be any need to specify or constrain the sendness of async functions, and
moreover there wouldn't be a need to be generic over sendness. As such, that world would have much
less motivation for either RTN or trait transformers.
In other words, this is an instance of a classic Rust problem of composing unsafe abstractions <https://smallcultfollowing.com/babysteps/blog/2016/10/02/observational-equivalence-and-unsafe-code/>.
The two unsafe abstractions here are:
• Thread locals
• Work-stealing async tasks
Each can be made safe in isolation, but the combination of the two end up being unsound.
From where I stand, I am 0.85 sure that we've picked the wrong one of the two in this case: I'll
gladly use a context object in lieu of a thread local, or write some `unsafe` inside my global
allocator, if that means that crates like `may` <https://docs.rs/may/latest/may/> are sound, and if I don't
need to *further* paint async part of the ecosystem into `Send`, `!Send` and `~Send` colors.
That being said, I don't see a reasonable way we can rectify the mistake while maintaining backwards
compatibility. Given that the issues were somewhat widely discussed and no-one else seemed to
suggested a workable approach either, I am skeptical that this is at all possible.
So it seems reasonable to accept this part of Rust as is, and instead try to address the
repercussions, where something like RTN does seem necessary.
—
Reply to this email directly, view it on GitHub <#3654 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABF4ZQV4FUZVGFM5Y2DEKTZHMT7FAVCNFSM6AAAAABI2KAHLSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRYGQ4TEMZWGY>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
This comment was marked as off-topic.
This comment was marked as off-topic.
|
||
When using a trait `MyTrait` that defines a sendable alias `SendMyTrait`... | ||
|
||
* Implement `MyTrait` directly. Your type will implement `SendMyTrait` automatically if appropriate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting.. I had to think about whether I agree with this and started to argue against it, but I think it's probably the right call.
Implementing SendMyTrait
directly can be nice for concrete types, because makes it your contracts more explicit. Of course this is a special case of auto trait leakage, but here we have the opportunity to mitigate its downsides with best practices. But a big problem with this take is that the converse is not true; implementing MyTrait
does not remove any semver hazards from your code, it only hides them. I would not want to lead users into a false sense of security here.
The other problem is that the practice of implementing SendMyTrait
doesn't extend to generic types. You could argue that the resulting need to add a where T: Send
or where T: SendMyTrait
to impl<T> SendMyTrait for Struct<T>
might cause an implementer to think twice about whether their impl needs to be so constrained. But then the key question becomes whether they know they can write impl<T> MyTrait for Struct<T>
and still have SendMyTrait
implemented when possible. In the case where you always implement MyTrait
directly, this quickly becomes obvious from experience.
With all that said, I might personally choose to implement SendMyTrait
directly in cases where it's important.. but that would be going against the general best practices laid out here (which I agree with), and is probably best left for the cases where I would otherwise bother to write a static assertion.
Note that this is connected to the convention of whether to use MyTrait
/SendMyTrait
or LocalMyTrait
/MyTrait
. In the latter case it would seem we are encouraging impls to use the send variant by default and to "opt in" to more genericity / fewer guarantees.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points. I think when I wrote this, I was also thinking about the fact that you cannot implement trait aliases today. I think we should expedite work on changing that, in which case I suspect that the best advice would be to implement the Send variant if that's what you always want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not so concerned about people learning that they can implement the 'non-send' variant and still get the benefits. They'll get that in time, I think, I don't think they have to learn it right away. Using the send variant will get you faster and better error messages.
I put a lot of input into this RFC in the early design stages and while it was a draft, and I'm happy with how it turned out! Thanks @nikomatsakis for being the one to drive the RFC forward, and to everyone who participated in the many design meetings we had on this feature. @rfcbot reviewed |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
Co-authored-by: Tyler Mandry <[email protected]>
Re-reading your comment in full, I see I responded too hastily, and you raised the very point I was making. Apologies for that. In any case, I generally agree with your conclusion. That said, I have noticed thread-locals cropping up more and more often as an annoying soundness hole (most recently in duchess and salsa), and I am wondering whether we should consider a way to make using them more 'opt-in' (or at least 'opt-out'). This is connected to the context and capabilities idea that @tmandry blogged about some time past. Put another way, I am not quite as pessimistic as you are about eventually changing how this works. That said, I don't want to block async progress while we figure that out (and there's a chance that it doesn't get us all the way there, as @CraftSpider pointed out). And as you said, there is backwards compatibility to consider. I think ultimately it'll bottom out in a lot more futures being Plus (as the RFC points out) this feature is more general regardless. |
Nit: This doc takes a bit too long, as written, to establish firmly that the ".." in the syntax At least for me (and this is after I had participated in design meetings on this subject), when I first picked up this RFC and read it, my gut upon encountering the "method(..)" syntax was to interpret the ".." as placeholder for some sort of concrete arguments (I know that it doesn't say whether those arguments would be type-expressions, value-expressions, or something else; my brain was happy to to let the doc fill that in later). The main hints that served to correct the above misimpression, at least for me, are:
|
@rfcbot reviewed |
|
||
Return type notation (RTN) gives a way to reference or bound the type returned by a trait method. The new bounds look like `T: Trait<method(..): Send>` or `T::method(..): Send`. The primary use case is to add bounds such as `Send` to the futures returned by `async fn`s in traits and `-> impl Future` functions, but they work for any trait function defined with return-position impl trait (e.g., `where T: Factory<widgets(..): DoubleEndedIterator>` would also be valid). | ||
|
||
This RFC proposes a new kind of type written `<T as Trait>::method(..)` (or `T::method(..)` for short). RTN refers to "the type returned by invoking `method` on `T`". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, in the general case the two forms here must represent a set of types, not just a single type, right?
That is, if the assigned return type is derived from the argument types, then it is not correct to say "the type returned", right? (I'm referring to the kind of issue described at the end of the RFC in the section entitled "Specifying the values for argument types")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I'd be fine if this were handled as a footnote.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct.
Side note: I thought about this some around the time of @matklad's blog post on the topic and came to a similar conclusion. In fact, I have draft about this that I need to actually post.. |
Good feedback. I'll consider some minor edits to clarify. |
@rfcbot reviewed |
@rfcbot fcp reviewed |
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. |
Return type notation (RTN) gives a way to reference or bound the type returned by a trait method. The new bounds look like
T: Trait<method(..): Send>
orT::method(..): Send
. The primary use case is to add bounds such asSend
to the futures returned byasync fn
s in traits and-> impl Future
functions, but they work for any trait function defined with return-position impl trait (e.g.,where T: Factory<widgets(..): DoubleEndedIterator>
would also be valid).This RFC proposes a new kind of type written
<T as Trait>::method(..)
(orT::method(..)
for short). RTN refers to "the type returned by invokingmethod
onT
".To keep this RFC focused, it only covers usage of RTN as the
Self
type of a bound or where-clause. The expectation is that, after accepting this RFC, we will gradually expand RTN usage to other places as covered under Future Possibilities. As a notable example, supporting RTN in struct field types would allow constructing types that store the results of a call to a trait-> impl Trait
method, making them more suitable for use in public APIs.Examples of RTN usage allowed by this RFC include:
where <T as Trait>::method(..): Send
where T: Trait<method(..): Send>
where T::method(..): Send
Trait
is inferred from the compiler)dyn Trait<method(..): Send>
dyn
types take lists of bounds)impl Trait<method(..): Send>
impl
types)Rendered