Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
Method call reference: major rewrite #1725
Changes from 15 commits
b73a724
d86363f
c52cfca
875073f
ba02319
f545a85
8e682a6
8c16081
a8c7502
3d73f8f
4f2125b
fb49894
f1a7dd7
e17b9b6
687a52b
41098a6
9d00834
923094b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 currently confusing to me, it's also confusing in the source :3
what's happening here is:
assemble_inherent_candidates
for params looks for where-bounds with that param as the self type. Giving us a list of traits methodsassemble_extension_candidates_for_all_traits
However, we manually look for methods from
T: Trait
bounds to give them precedence over methods from other traits:The same in the "trait object" section.
I personally would prefer to only talk about 'inherent' and 'trait' candidates and then mention at the end that some trait candidates are given the same precedence as inherent ones. But that feels like a fairly big change (and diverges from the current terminology used in rustc)
there's already a convo with @traviscross about this, i think it's fine and idk how to meaningfully change this to be clearer for me 🤔
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.
Here's a fun set of differential examples about what constitutes an "inherent candidate" or not:
Playground link
Seemingly, it only counts methods from trait impls in bounds as inherent candidates if the type has unified with a generic parameter. The bound doesn't need to be directly on that generic parameter though. This is an interesting way that adding a seemingly-redundant generic parameter can actually affect behavior.
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.
These are indeed interesting and quite surprising.
If it helps, this is the code which is presumably having this effect - I'm afraid most of the stuff around relating, normalizing, and transforming types is beyond me, but perhaps it helps you figure out what's up.
Might I propose that I don't alter this PR to account for this complexity, and that as our understanding here is currently low and hopefully will increase, it might be appopriate for a follow-up change?
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 referenced
probe.rs
in crafting the examples. I think it's pretty clear at this point what it's doing. We just need to tweak the wording a bit to capture it. Methods from inherent impls are always inherent candidates. Fordyn Trait
types, this includes the inherent impls on thedyn Trait
and the "built-in impl" for vtable dispatch. If the receiver type unifies with a generic parameter, and there's a trait bound on that type, then the methods from that trait are inherent candidates.Methods from in-scope traits are extension candidates, including when the trait is in scope due to being the trait being implemented.
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.
OK. I'd like to take your wording as-is but first I need to work out whether the call to
self.n()
here is inherent or extension. If it's inherent (I think so) then it's a sufficiently common case we should probably explicitly mention it, even though I suspect that behind the scenes it's handled by "if the receiver type unifies with a generic parameter".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 example. Yes, it's inherent. The proof is this:
Playground link
It is probably worth mentioning this one too.
Here's that example desugared with specialization:
Playground link
As annotated there, we can see there must be a default
Self: A
bound on the specialization impl, which agreeing with your theory, would explain why this is treated as an inherent candidate.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.
OK, here's a question. Above, we create a list of candidate types. We then look for candidate methods in the last step. But notably, we collect candidate methods from extension candidates without regard to the candidate types:
Then we get to here, in the picking process. But this text now says that we're only considering those candidate types that can be reached via the
Deref
chain.So as written, this would seem to imply that the
Receiver
chain extending beyond theDeref
chain isn't considered at all with regard to selecting or picking methods from in scope traits, but that doesn't seem right.What's the right answer here?
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's a perspective I hadn't thought of before: the
Receiver
trait makes no difference to method resolution for extension methods. I think that's the logically correct conclusion.However, the
Receiver
trait is considered also inwfcheck.rs
which rejects examples like this if you remove theimpl Receiver
before we even get as far as resolving any 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.
Wild. OK, now this is snapping into focus.
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.
OK, that then gives us something to explain in the text. We say:
There must, I'm guessing, be some interaction with the well-formedness checking at this point. I.e., maybe it's that if exactly one candidate is identified that is well-formed (where the well-formedness checking considers the
Receiver
chain), then it is picked. Because we don't report ambiguity errors just because two in-scope traits have a method of the same name that could apply. I'm thinking of examples like this:Playground link
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.