-
Notifications
You must be signed in to change notification settings - Fork 508
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
Arbitrary self types v2: update reference. #1699
Conversation
@rustbot label +S-waiting-on-author -S-waiting-on-review |
Wouldn't the point
for |
I agree, I'll remove that bit. |
This comment has been minimized.
This comment has been minimized.
c860df3
to
e138cf7
Compare
I've pushed a couple of extra commits here to respond to review feedback - I suspect I should probably squash them before this is merged (?) but for now it's easier for folks to see what I changed. |
d641699
to
84f896f
Compare
The first step is to build a list of types where we might find methods. | ||
Obtain these by repeatedly [dereferencing][dereference] the receiver expression's type, adding each type encountered to the list, then finally attempting an [unsized coercion] at the end, and adding the result type if that is successful. | ||
Then, for each candidate `T`, add `&T` and `&mut T` to the list immediately after `T`. | ||
Then, for each candidate `T`, add `&T` and `&mut T` to the list immediately after `T`. While dereferencing, we don't use the normal `Deref` trait, but instead the `Receiver` trait: there is a blanket implementation of `Receiver` for all `T: Deref` so in practice this is often the same. | ||
|
||
For instance, if the receiver has type `Box<[i32;2]>`, then the candidate types will be `Box<[i32;2]>`, `&Box<[i32;2]>`, `&mut Box<[i32;2]>`, `[i32; 2]` (by dereferencing), `&[i32; 2]`, `&mut [i32; 2]`, `[i32]` (by unsized coercion), `&[i32]`, and finally `&mut [i32]`. | ||
For instance, if the receiver has type `Box<[i32;2]>`, then the contributing types will be `Box<[i32;2]>`, `&Box<[i32;2]>`, `&mut Box<[i32;2]>`, `[i32; 2]` (by dereferencing), `&[i32; 2]`, `&mut [i32; 2]`, `[i32]` (by unsized coercion), `&[i32]`, and finally `&mut [i32]`. | ||
|
||
Then, for each candidate type `T`, search for a [visible] method with a receiver of that type in the following places: | ||
Some custom smart pointers may implement `Receiver` but not `Deref`. Imagine `MySmartPtr<T>: Receiver<Target=T>`: if the receiver type has `Box<MySmartPtr<SomeStruct>>` the contributing types will be `Box<MySmartPtr<SomeStruct>>`, `MySmartPtr<SomeStruct>` and `SomeStruct`. | ||
|
||
Even though we assemble this list by following the chain of `Receiver` implementations, we keep a note of which steps can be reached by following the regular `Deref` chain. In the above example, that would be all but the last step. The items in the list reachable by the `Deref` chain are termed the "candidate types"; the full list reachable by the `Receiver` chain is called the "contributing types". | ||
|
||
Then, for each _contributing_ type `T`, search for a [visible] method with a receiver of any _candidate_ type in the following places: |
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.
This is better, but still some things we can improve:
- We're inconsistently using the word "candidate". Toward the top, we talk about assembling the list of candidates (i.e. "for each candidate
T
, add ... to the list"), but we don't mean the list of "candidate types" as defined later, we mean the list of "contributing types". The word "candidate" is also used further below in a seemingly-inconsistent way. - We separately define this notion of "candidate types" from "contributing types", but we don't say why or use the list of candidate types for anything as far as I can tell. It's a bit odd to define this separately if it has no semantic relevance to the language. If there is some semantic relevance, we should describe it. If it's only of implementation relevance, then it's probably a better fit for the dev guide.
- We're no longer stating our terms upfront. That's what the bit you removed about "The first step is to build a list of candidate receiver types" was doing -- it introduced that term, then described how we built that list. The text now says "the contributing types will be..." without first introducing that this is a special term and we're building a list of these things.
- Talking about repeated dereferencing and then saying, "well, really it's not dereferencing but this other thing" is a bit odd. It's probably better to just directly describe that we're following the chain of receiver targets.
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.
Agreed with all your points other than this one:
We separately define this notion of "candidate types" from "contributing types", but we don't say why or use the list of candidate types for anything as far as I can tell. It's a bit odd to define this separately if it has no semantic relevance to the language. If there is some semantic relevance, we should describe it. If it's only of implementation relevance, then it's probably a better fit for the dev guide.
It is of semantic relevance - it says this, which you may have overlooked:
Then, for each contributing type
T
, search for a visible method with a receiver of any candidate type in the following places:
I'll have another crack at it.
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.
Great, thanks. Did miss that bit.
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 attempted a major rewrite of this bit of the reference in #1725. It's an alternative to this PR.
so it's rare to implement `Receiver` directly: you'd only normally do this | ||
for smart pointer types which for some reason can't implement `Deref`. |
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 probably remove this bit about it being rare and instead describe affirmatively why Receiver
can be implemented but Deref
cannot in some cases.
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.
The same comment probably applies to #1725 but let's see which PR seems most likely to succeed before I tackle this (relative) detail.
This section of the reference has been oversimplistic for some time (rust-lang#1018 and rust-lang#1534) and various rewrites have been attempted (e.g. rust-lang#1394, rust-lang#1432). Here's another attempt! My approach here is: * Stop trying to keep it short and concise * Document what actually happens in the code, step by step This does result in a long explanation, because we're trying to document nearly 2400 lines of code in `probe.rs`, but doing otherwise feels as though we'll continue to run into criticisms of oversimplification. This rewrite documents the post-arbitrary-self-types v2 situation, i.e. it assumes rust-lang/rust#135881 has landed. We should not merge this until or unless that lands. This PR was inspired by discussion in rust-lang#1699. If we go ahead with this approach, rust-lang#1699 becomes irrelevant. There was also discussion at rust-lang/cargo#15117 (comment)
r[lang-types.pin] | ||
## `Pin<P>` | ||
|
||
r[lang-types.pin.receiver] | ||
[Methods] can take [`Pin<P>`] as a receiver. |
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.
Nit: Is Pin
not still a special type? A quick search in the reference reveals that it's at least fundamental, but I'd have thought it was more? Something something soundness something unpin something?
Closing this in favor of: |
Part of rust-lang/rust#44874