Method call reference: major rewrite #1725
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)
rustbot
added
the
S-waiting-on-review
| * For other tyings (e.g. bools, chars) there's a search for inherent candidates | ||
| for the incoherent type. |
There was a problem hiding this comment.
Nit: is "tyings" a typo or jargon?
There was a problem hiding this comment.
And what does incoherent mean here? Is it in terms of Trait Implementation Coherence?
There was a problem hiding this comment.
Ah, are we talking the fake "Integer" type the literal 42 has before it turns into i32?
| * After any of these, there's a further search for extension candidates for | ||
| traits in scope. |
There was a problem hiding this comment.
Nit: The use of "extension" here and in the next section is a bit underspecified. Not sure how to resolve it, maybe "trait method"? Or "non-inherent"?
| starts. | ||
|
|
||
| Once again, the candidate types are iterated. This time, only those types | ||
| are iterated which can be reached via the `Deref` trait or built-in derefs; |
There was a problem hiding this comment.
Nit: Does it make sense to differentiate between the Deref trait and built-in derefs here? What is the difference / why don't we just say Deref?
(Also relevant for "only consider the types reachable via Deref or built-in dereferencing" above).
| * Extra lists are maintained for diagnostic purposes: | ||
| unstable candidates, unsatisfied predicates, and static candidates. |
There was a problem hiding this comment.
Nit: Maybe leave notes about diagnostics out of the overview if saying under "extra details" that they're left out?
| This list of candidate types is then converted to a list of candidate methods. | ||
| For each step, the `self` type is used to determine what searches to perform: | ||
|
|
||
| * For a trait object, there is first a search for for inherent candidates for |
There was a problem hiding this comment.
| * For a trait object, there is first a search for for inherent candidates for | |
| * For a trait object, there is first a search for inherent candidates for |
| * 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.
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.
| then it is an error, and the receiver must be [converted][disambiguate call] | ||
| to an appropriate receiver type to make the method call. |
There was a problem hiding this comment.
| then it is an error, and the receiver must be [converted][disambiguate call] | |
| to an appropriate receiver type to make the method call. | |
| then it is an error, and the user must [disambiguate][disambiguate call] | |
| the call and convert the receiver to an appropriate receiver type. |
This is about clarifying that it's the user and not the compiler that must do something here.
|
Overall, this is fantastic. Very much an improvement. Thanks @adetaylor. |
traviscross
added
the
S-waiting-on-stabilization
| * The search for candidate methods will also consider searches for | ||
| incoherent types if `rustc_has_incoherent_inherent_impls` is active for | ||
| a `dyn`, struct, enum, or foreign type. |
There was a problem hiding this comment.
Is there any way for stable Rust code to be affected by this, e.g. by how this is applied to the Error trait or the CStr type?
|
☔ The latest upstream changes (possibly 4249fb4) made this pull request unmergeable. Please resolve the merge conflicts. |
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_typesrust#44874