Skip to content
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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

adetaylor
Copy link
Contributor

@adetaylor adetaylor commented Jan 30, 2025

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:

  • 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 #1699. If we go ahead with this approach, #1699 becomes irrelevant. There was also discussion at rust-lang/cargo#15117 (comment)

Tracking:

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 rustbot added the S-waiting-on-review Status: The marked PR is awaiting review from a maintainer label Jan 30, 2025
@nikomatsakis nikomatsakis self-assigned this Jan 30, 2025
@adetaylor adetaylor marked this pull request as ready for review January 30, 2025 18:58
Copy link
Contributor

@madsmtm madsmtm left a 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!

Comment on lines 70 to 71
* For other tyings (e.g. bools, chars) there's a search for inherent candidates
for the incoherent type.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: is "tyings" a typo or jargon?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And what does incoherent mean here? Is it in terms of Trait Implementation Coherence?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, are we talking the fake "Integer" type the literal 42 has before it turns into i32?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"tyings" was a typo.
I don't know what "incoherent" means here - I was rather hoping that this was obvious to all possible readers except me :) Sadly not, I'll look it up and expand this bit!

src/expressions/method-call-expr.md Show resolved Hide resolved
src/expressions/method-call-expr.md Show resolved Hide resolved
src/expressions/method-call-expr.md Show resolved Hide resolved
Comment on lines 108 to 111
* 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
Copy link
Contributor

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.

@traviscross
Copy link
Contributor

Overall, this is fantastic. Very much an improvement. Thanks @adetaylor.

@traviscross traviscross added the S-waiting-on-stabilization Waiting for a stabilization PR to be merged in the main Rust repository label Jan 31, 2025
Comment on lines +142 to +144
* 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, this is another thing I'll need to go and figure out

@rustbot
Copy link
Collaborator

rustbot commented Jan 31, 2025

☔ The latest upstream changes (possibly 4249fb4) made this pull request unmergeable. Please resolve the merge conflicts.

src/expressions/method-call-expr.md Outdated Show resolved Hide resolved
src/expressions/method-call-expr.md Outdated Show resolved Hide resolved
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
the object, then inherent impl candidates for the type.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should one of these say "trait"? I'm a little confused what is meant by "object" vs "type". I know one of them is the dyn Trait type, but not sure which it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair, I'll try to figure it out myself and amend this!

src/expressions/method-call-expr.md Outdated Show resolved Hide resolved
src/expressions/method-call-expr.md Outdated Show resolved Hide resolved
Comment on lines +80 to +81
(that is, applicable candidates from traits). Only [visible] candidates
are included.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're of course the expert here, but is this parenthetical, and are the parentheticals added below, correct?

To me, it seems that what probe.rs considers to be an "inherent candidate" is (something in the vicinity of) one that can be deduced exclusively from the signature, including WCs, whereas an "extension candidate" is one that considers the traits in scope. So, e.g., in...

impl<F: FnPtr> hash::Hash for F {
    fn hash<HH: hash::Hasher>(&self, state: &mut HH) {
        state.write_usize(self.addr() as _)
    }
}

(From library/core/src/ptr/mod.rs.)

...the hash::Hasher::write_usize method is an "inherent candidate" even though it is from a trait, and it is not an "extension candidate" because Hasher is not in scope.

This is, I surmise, what answers @tmandry's question in the other thread about what it means to find inherent candidates on type parameters.

Anyway, if you would, have a look and see what's correct here. We should probably somewhere just clearly define what we mean by "inherent candidate" and "extension candidate" and lean into the model used by probe.rs.

Also cc @nikomatsakis who seems to have written much of this way back when.

@mattheww
Copy link
Contributor

mattheww commented Feb 3, 2025

Note there's another description of the method lookup process in the
rustc-dev-guide.

Comment on lines +72 to +73
* After any of these, there's a further search for extension candidates for
traits in scope.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand it, the "further search for extension candidates for traits in scope." happens once, not once for each candidate type, so I suggest putting this after the bulleted list rather than as part of it.

Perhaps mention the special case described in #630 (comment) here?

@mattheww
Copy link
Contributor

mattheww commented Feb 3, 2025

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:

  • At this point the receiver type might be something like Vec<?>, which I think explains how the "If there are multiple candidates from traits, they may in fact be identical" case can happen.

  • If the type isn't constrained at all by the code coming before the method call, method resolution will just give up (even if there's only one possible type for which the method call could work).

@mattheww
Copy link
Contributor

mattheww commented Feb 3, 2025

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 NoParameter is found earlier in the picking process even though it's "obviously" the wrong one.

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);
}

@obi1kenobi
Copy link
Member

Wow, this example is very surprising to me!

With my "cargo-semver-checks maintainer" hat on, it's extremely useful to have someplace where these details are written up with examples like this. The better I can understand these nuances, the better our rules for discovering breakage will be.

I will leave to you all the question of whether the reference or some other location is the best place to document these behaviors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: The marked PR is awaiting review from a maintainer S-waiting-on-stabilization Waiting for a stabilization PR to be merged in the main Rust repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants