-
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
Method call reference: major rewrite #1725
base: master
Are you sure you want to change the base?
Conversation
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)
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.
Fully agree that it's better to list this out in detail than trying to condense it down to unreadable, and what you've written here is phenomenal!
* And finally, a method with a `Pin` that's reborrowed, if the `pin_ergonomics` | ||
feature is enabled. | ||
* First for inherent methods | ||
* Then for extension methods |
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.
We don't generally document unstable features, but this is the kind of case that makes me pretty sad about taking it out, as the comprehensiveness here is nice. Wish we had a way to conditionalize this somehow.
cc @nikomatsakis as an example for us to think about.
Overall, this is fantastic. Very much an improvement. Thanks @adetaylor. |
☔ The latest upstream changes (possibly 4249fb4) made this pull request unmergeable. Please resolve the merge conflicts. |
Co-authored-by: Travis Cross <[email protected]>
Co-authored-by: Travis Cross <[email protected]>
Co-authored-by: Tyler Mandry <[email protected]>
Co-authored-by: Tyler Mandry <[email protected]>
Co-authored-by: Tyler Mandry <[email protected]>
Co-authored-by: Tyler Mandry <[email protected]>
Note there's another description of the method lookup process in the |
One thing that makes this business difficult to document is that method lookup happens when type inference is not yet complete (and the results of method lookup participate in type inference). All the writeups I've seen gloss over this by writing "the receiver type" and then proceeding to describe what happens as if that type was fully determined. I'm not sure how much detail it makes sense to go into on this page as long as the Reference has very little to say in general about type inference. But I think it might be worth mentioning that:
|
It might also be worth noting explicitly that the types of parameters in the method call expression aren't taken into account in method resolution. Even the number of parameters is ignored. So the following won't compile, because the method from trait NoParameter {
fn method(self);
}
trait OneParameter {
fn method(&self, jj: i32);
}
impl NoParameter for char {
fn method(self) {}
}
impl OneParameter for char {
fn method(&self, jj: i32) {}
}
fn f() {
'x'.method(123);
} |
Wow, this example is very surprising to me! With my " I will leave to you all the question of whether the reference or some other location is the best place to document these behaviors. |
b828a41
to
f1a7dd7
Compare
Agreed - I added your example in 687a52b. |
@mattheww regarding
I agree that it would be good to explain that here, but I'd prefer it be done in a separate PR - this one is already ambitious enough, and I don't think it makes that situation worse. |
Yes, it would be better to get this merged and think about improving it later. I think with the Reference in its current state there's an argument that this part should just be omitted (if it can't happen in a model where the full type of the receiver is known):
But I'd certainly have no objection to leaving that question for another time. |
I'm not sure whether that's the only case where the candidates may turn out to be identical - I'll try to work it out. |
This section of the reference has been oversimplistic for some time (#1018 and #1534) and various rewrites have been attempted (e.g. #1394, #1432). Here's another attempt!
My approach here is:
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 #1699. If we go ahead with this approach, #1699 becomes irrelevant. There was also discussion at rust-lang/cargo#15117 (comment)
Tracking:
arbitrary_self_types
rust#44874