Adjust receiver lookup to match the actual impl #190

alercah · 2018-01-02T22:34:43Z

As a part of this, remove the reference to coerce_inner and replace it
with a proper description of unsized coercions. That description will
have to be updated later when more of the unsized work stabilizes.

Please review this for correctness; I was mostly working off of the description in #62, the DST RFC, and some personal test examples.

And of course, if there's anything that can be done to include clarity (such as more examples), I can do that too.

As a part of this, remove the reference to coerce_inner and replace it with a proper description of unsized coercions. That description will have to be updated later when more of the unsized work stabilizes. Closes rust-lang#62.

Havvy

Mostly nits. Otherwise looking good.

Havvy · 2018-01-06T03:20:07Z

src/expressions/method-call-expr.md

-    fn method(self) {}
+When resolving method calls on an expression of type `A`, Rust looks up methods
+both on the type itself and the traits in implements. Additionally, unlike with
+non-method function calls, the `self` parameter is special and may be


I don't like calling them "non-method function calls", but I don't know of a better name. "standard" and "regular" both make it sounds like method calls are non-standard or irregular. Any ideas here?

Might also want to link to that expression kind from here.

How about just "other"?

Havvy · 2018-01-06T03:26:39Z

src/expressions/method-call-expr.md

+When resolving method calls on an expression of type `A`, Rust looks up methods
+both on the type itself and the traits in implements. Additionally, unlike with
+non-method function calls, the `self` parameter is special and may be
+automatically dereferenced in order to resolve it. Rust uses the following


Everything in this document is about Rust. Mentioning it should be avoided, I think. Better wording would be "The following is the process/procedure to resolve which method to call."

Havvy · 2018-01-06T03:27:47Z

src/expressions/method-call-expr.md

+automatically dereferenced in order to resolve it. Rust uses the following
+process to resolve method calls.
+
+First, Rust will attempt to build a list of candidate receiver types. It obtains


"First, attempt to" & "Obtain these"

Havvy · 2018-01-06T03:29:34Z

src/expressions/method-call-expr.md

+these by repeatedly [dereferencing][dereference] the 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`, Rust adds `&T` and `&mut T` to the list immediately afterward.


More "Rust" removal. Also, clarify after what to add after. Currently it's ambiguous as to whether it's the end of the list or after the T. Though the example clarifies it.

"for each candidate T, add &T and &mut T to the list immediately after T."

Havvy · 2018-01-06T03:30:51Z

src/expressions/method-call-expr.md

+end, and adding the result type if that is successful. Then, for each candidate
+`T`, Rust adds `&T` and `&mut T` to the list immediately afterward.
+
+So, for instance, if `A` is `Box<[i32;2]>`, then the candidate types will be


The So, for instance is two leading clauses. I would drop the "So". Also, we want to use "example" for examples, not "instance". (Later I want to actually put examples into their own <div>.

Havvy · 2018-01-06T04:01:20Z

src/expressions/method-call-expr.md

+a receiver of that type in the following places:
+
+1. `T`'s inherent methods (methods implemented directly on `T`).
+1. Any of the methods provided by a trait implemented by `T`. If `T` is


Should we say "visible trait" here? Do we define that term anywhere?

We do, the [visible] is a link to the visibility page. You're right that it should say visible trait, I think, so I can add that in.

Havvy · 2018-01-06T04:03:12Z

src/expressions/method-call-expr.md

+   methods from the trait constraints on `T` are available for lookup. If `T` is
+   not, then methods from any in-scope trait are available.
+
+Note that the lookup is done for each type in order, which can occasionally lead


Colon after "Note". Entire note and example should be in blockquotes. Could raise to a "Warning:" even.

Havvy · 2018-01-06T04:04:07Z

src/expressions/method-call-expr.md

+receiver must be [converted][disambiguate call] to an appropriate receiver type
+to make the method call.
+
+The lookup process does not take into account the mutability or lifetime of the


"This process"

Havvy · 2018-01-06T04:06:17Z

src/expressions/method-call-expr.md

+to make the method call.
+
+The lookup process does not take into account the mutability or lifetime of the
+receiver, or whether a method is `unsafe`. Once a method is looked up.


The ending sentence here is a sentence fragment. We've lost the part where it's a compiler error if the listed constraints fail.

Oops, that was what the fragment was supposed to be. Thanks.

matthewjasper · 2018-01-03T16:38:48Z

src/expressions/method-call-expr.md

+
+First, Rust will attempt to build a list of candidate receiver types. It obtains
+these by repeatedly [dereferencing][dereference] the type, adding each type
+encountered to the list, then finally attempting an [unsized coercion] at the


It's only array to slice unsizing coercions.

I'll verify this and then update if needed.

Looks like even a CoerceUnsized coercion can work:

https://play.rust-lang.org/?gist=a08eb003e02c3f9abc0771cd7017b794&version=stable

You're going from &[i32; 3] to &[i32], which is exactly the case that I'm saying works.

Err, right. Niko said it could be any unsized conversion, but I guess CoerceUnsized won't ever happen here, because it'll just get dereferenced and the underlying conversion applied. Likewise I think supertrait object conversion is impossible since it'll just find the method on the subtrait. The last option is unsized struct conversion, but that seems like it can't be applied here: https://play.rust-lang.org/?gist=1aa827380405e1a287b275fe0f83b69a&version=stable

Is that example correct to demonstrate that the unsized struct conversion doesn't work here? @nikomatsakis, is this the expected behaviour?

matthewjasper · 2018-01-03T16:39:57Z

src/type-coercions.md

+* `[T; n]` to `[T]`.
+
+* `T` to `U`, when `U` is a trait object type and either `T` implements `U` or
+  `T` is a trait object for a subtrait of `U`.


Also the third case in the Unsize docs

I left that out since it isn't yet stabilized. Should I put it in anyway?

The coercion is stable, it's just the trait that isn't.

matthewjasper · 2018-01-05T21:16:14Z

src/expressions/method-call-expr.md

+1. `T`'s inherent methods (methods implemented directly on `T`).
+1. Any of the methods provided by a trait implemented by `T`. If `T` is
+   a type parameter (including the `Self` parameter of a trait), then only
+   methods from the trait constraints on `T` are available for lookup. If `T` is


In scope traits are still checked, methods from traits in bounds are treated as inherent methods so get looked at first.

Ah, ok. How can an in scope trait apply to a bounded type parameter? Does it have to be declared locally?

trait A { fn foo(&self) {} } impl<U> A for U {} fn bar<T>(t: T) { t.foo(); // OK, calls A::foo }

Ahh, right. Thanks!

alercah · 2018-01-07T01:03:41Z

Do you prefer to have changes rebased into a single commit?

Havvy · 2018-01-07T03:46:46Z

It's useful to have multiple commits while editing, especially for large sections of new prose, but it's always nice to just have a single commit for it after editing. Ultimately, there's no official policy on this for the reference, as far as I know.

Havvy

I'm a bit more awake on this review than the last one. These are all things I can do myself, so if you ever feel like we're going in circles too much, and just want me to merge, I can do so.

With method call receiver not defined anywhere, I sort of want to do another PR on this section anyways.

Havvy · 2018-01-07T04:34:51Z

src/expressions/method-call-expr.md

-process to resolve method calls.
-
-First, Rust will attempt to build a list of candidate receiver types. It obtains
+When resolving method calls on an expression of type `A`, that expression will


We only use A twice now, and it's sort of useless to name it. It'd be nice to remove it. Especially since I think the other usage would read better without it.

Havvy · 2018-01-07T04:39:27Z

src/expressions/method-call-expr.md

-
-First, Rust will attempt to build a list of candidate receiver types. It obtains
+When resolving method calls on an expression of type `A`, that expression will
+be the `self` parameter. This  parameter is special and, unlike with other


"be the self parameter on the called method."

A second reading of this, I would also get rid of everything from "is special" to "or other functions", unless you feel like it's worth emphasizing that this behavior only applies to the method receiver. Which I noticed isn't really defined either.

Havvy · 2018-01-07T04:43:13Z

src/expressions/method-call-expr.md


-So, for instance, if `A` is `Box<[i32;2]>`, then the candidate types will be
+For instance, if `A` is `Box<[i32;2]>`, then the candidate types will be


Here is the other usage of A. I think it would read better without referring to A and just said "if the method receiver is of type Box<[i32; 2]>".

Havvy · 2018-01-07T04:45:08Z

src/expressions/method-call-expr.md

-to surprising results. The below code will print "In trait impl!", because
-`&self` methods are looked up first, the trait method is found before the
-struct's `&mut self` method is found.
+    Note: the lookup is done for each type in order, which can occasionally lead


This Note and example should still all be in blockquotes. You can do so by starting each line with a >.

I'm used to four spaces for blockquote. I'll change to > though. And sorry about the example.

Havvy · 2018-01-07T04:57:07Z

src/type-coercions.md

+cases where other coercions are not, as described above. They can still happen
+anywhere else a coercion can occur.
+
+Two traits, `Unsized` and `CoerceUnsized`, are used to assist in this process


Make sure these are actual links. It'd be ../std/marker/trait.Unsize.html and ../std/ops/trait.CoerceUnsized.html respectively.

The "expose it for library use" part is definitely unstable and should be removed for now.

Havvy · 2018-01-11T19:00:59Z

LGTM. Though I don't know if @matthewjasper 's concerns are addressed. @matthewjasper, are you good with this PR?

matthewjasper · 2018-01-11T21:16:35Z

Comment about which unsizing coercions are actually performed is still relevant, but this is generally an improvement. I'm happy to have this merged with an issue created (either here or on the main repo) if we can't reach a decision now.

Havvy · 2018-01-12T04:10:15Z

Alright, merging it in.

@matthewjasper I'm not entirely sure what the details are about unsizing concerns, so could you file the issue?

@alercah 💟

Adjust receiver lookup to match the actual impl

8cc56ae

As a part of this, remove the reference to coerce_inner and replace it with a proper description of unsized coercions. That description will have to be updated later when more of the unsized work stabilizes. Closes rust-lang#62.

Havvy requested changes Jan 6, 2018

View reviewed changes

matthewjasper reviewed Jan 6, 2018

View reviewed changes

Review feedback.

87bea5d

Havvy requested changes Jan 7, 2018

View reviewed changes

Further review feedback.

3122881

Havvy approved these changes Jan 11, 2018

View reviewed changes

Havvy merged commit b6acf26 into rust-lang:master Jan 12, 2018

alercah deleted the receiver-lookups branch May 3, 2018 20:05

QuineDot mentioned this pull request Nov 10, 2022

Incorrect statement about unsized coercion in method lookup #819

Open


		So, for instance, if `A` is `Box<[i32;2]>`, then the candidate types will be
		For instance, if `A` is `Box<[i32;2]>`, then the candidate types will be

Adjust receiver lookup to match the actual impl #190

Adjust receiver lookup to match the actual impl #190

Uh oh!

Conversation

alercah commented Jan 2, 2018

Uh oh!

Havvy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alercah commented Jan 7, 2018

Uh oh!

Havvy commented Jan 7, 2018

Uh oh!

Havvy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Havvy commented Jan 11, 2018

Uh oh!