Update reference & deshadowing.

This changes the RFC in two significant ways:

* As requested widely, it now proposes that we implement Receiver for
  NonNull<T> and Weak<T>. This requires us, for the first time, to add explicit
  code to spot potentially shadowed methods and avoid such shadowing. This is
  described.
* It expands the Reference section to describe changes to the probing algorithm,
  which are now a little more extensive than the previous version of the RFC
  described, because we now search two different chains - one for types into
  which the receiver can be converted, and another chain for locations to search
  for possible methods.

Author: adetaylor

text/3519-arbitrary-self-types-v2.md

@@ -228,15 +228,46 @@ where
 
 (See [alternatives](#no-blanket-implementation) for discussion of the tradeoffs here.)
 
-It is also implemented for `&T`, `&mut T`, `*const T` and `*mut T`.
+It is also implemented for `&T`, `&mut T`, `Weak<T>`, `NonNull<T>`, `*const T` and `*mut T`.
 
-## Compiler changes
+## Compiler changes: method probing
 
-The existing Rust [reference section for method calls describes the algorithm for assembling method call candidates](https://doc.rust-lang.org/reference/expressions/method-call-expr.html). This algorithm changes in one simple way: instead of dereferencing types (using the `Deref<Target=T>`) trait, we use the new `Receiver<Target=T>` trait to determine the next step.
+The existing Rust [reference section for method calls describes the algorithm for assembling method call candidates](https://doc.rust-lang.org/reference/expressions/method-call-expr.html), and there's more detail in the [rustc dev guide](https://rustc-dev-guide.rust-lang.org/method-lookup.html).
 
-(Note that the existing algorithm isn't quite as simple as following the chain of `Deref<Target=T>`. In particular, `&T` and `&mut T` are considered as candidates too at each step; this RFC does not change that.)
+The key part of the first page is this:
 
-Because a blanket implementation is provided for users of the `Deref` trait and for `&T`/`&mut T`, the net behavior is similar. But this provides the opportunity for types which can't implement `Deref` to act as method receivers.
+> Then, for each candidate type `T`, search for a visible method with a receiver of that type in the following places:
+> - `T`'s inherent methods (methods implemented directly on `T`).
+> Any of the methods provided by a visible trait implemented by `T`.
+
+This changes.
+
+The list of candidate types is assembled in exactly the same way, but we now search for a visible method with a receiver of that type in _more_ places.
+
+Specifically, instead of using the list of candidate types assembled using the `Deref` trait, we search a list assembled using the `Receiver` trait. As `Receiver` is implemented for all types that implement `Deref`, this is a longer list.
+
+It's particularly important to emphasize that the list of candidate receiver types _does not change_ - that's still assembled using the `Deref` trait just as now. But, a wider set of locations is searched for methods with those receiver types.
+
+For instance, `Weak<T>` implements `Receiver` but not `Deref`. Imagine you have `let t: Weak<SomeStruct> = /* obtain */; t.some_method();`. We will now search `impl SomeStruct {}` blocks for an implementation of `fn some_method(self: Weak<SomeStruct>)`, `fn some_method(self: &Weak<SomeStruct>)`, etc. The possible self types are unchanged - they're still obtained by searching the `Deref` chain for `t` - but we'll look in more places for methods with those valid `self` types.
+
+## Compiler changes: deshadowing
+[compiler-changes-deshadowing]: #compiler-changes-deshadowing
+
+The major functional change to the compiler is described above, but a couple of extra adjustments are necessary to avoid future compatibility breaks by method shadowing.
+
+Specifically, that page also states:
+
+> If this results in multiple possible candidates, then it is an error, and the receiver must be converted to an appropriate receiver type to make the method call.
+
+This changes. For smart pointer types which implement `Receiver` (such as `NonNull<T>`) the future addition of any method would become an incompatible change, because it would run the risk of this ambiguity if there were a method of the same name within `T`. So, if there are multiple candidates, and if one of those candidates is in a more "nested" level of receiver than the others (that is, further along the chain of `Receiver`), we will choose that candidate and warn instead of producing a fatal error.
+
+Similarly,
+
+> Note: the lookup is done for each type in order, which can occasionally lead to surprising results.
+
+This changes too, for the same reason. We check for matching candidates for `T`, `&T` and `&mut T`, and again, if there's a candidate on an "inner" type (that, is, further along the chain of `Receiver`) we will choose that type in preference to less nested types and emit a warning.
+
+(The current reference doesn't describe it, but the current algorithm also searches for method receivers of type `*const Self` and handles them explicitly in case the receiver type was `*mut Self`. We do not check for cases where a new `self: *mut Self` method on an outer type might shadow an existing `self: *const SomePtr<Self>` method on an inner type. Although this is a theoretical risk, such compatibility breaks should be easy to avoid because `self: *mut Self` are rare. It's not readily possible to apply the same de-shadowing approach to these, because we already intentionally shadow `*const::cast` with `*mut::cast`.)
 
 ## Object safety
 
@@ -276,6 +307,11 @@ The existing branches in the compiler for "arbitrary self types" already emit ex
   }
   ```
   We don't know a use-case for this. There are several cases where this can result in misleading diagnostics. (For instance, if such a method is called with an incorrect type (for example `smart_ptr.a::<&Foo>()` instead of `smart_ptr.a::<Foo>()`). We could attempt to find and fix all those cases. However, we feel that generic receiver types might risk subtle interactions with method resolutions and other parts of the language. We think it is a safer choice to generate an error on any declaration of a generic `self` type.
+- As noted in [#compiler-changes-deshadowing](the section about compiler changes for deshadowing) we will downgrade an existing error to a warning if there are multiple
+  method candidates found, if one of those candidates is further along the chain of `Receiver`s than the others.
+- As also noted in [#compiler-changes-deshadowing](the section about compiler changes for deshadowing), we will produce a new warning if a method in an inner type is chosen
+  in preference to a method in an outer type ("inner" = further along the `Receiver` chain) and the inner type is either `self: &T` or `self: &mut T` and we're choosing it
+  in preference to `self: T` or `self: &T` in the outer type.
 
 # Drawbacks
 [drawbacks]: #drawbacks
@@ -297,7 +333,44 @@ Rust standard library smart pointers are designed with this shadowing behavior i
 
 Furthermore, the `Deref` trait itself [documents this possible compatibility hazard](https://doc.rust-lang.org/nightly/std/ops/trait.Deref.html#when-to-implement-deref-or-derefmut), and the Rust API Guidelines has [a guideline about avoiding inherent methods on smart pointers](https://rust-lang.github.io/api-guidelines/predictability.html#smart-pointers-do-not-add-inherent-methods-c-smart-ptr).
 
-These method shadowing risks also apply to `Receiver`. This RFC does not make things worse for types that implement `Deref`, it only adds additional flexibility to the `self` parameter type for `T::m`.
+This RFC does not make things worse for types that implement `Deref`.
+
+_However_, this RFC allow types to implement `Receiver`, and in fact does so for `NonNull<T>` and `Weak<T>`. `NonNull<T>` and `Weak<T>` were not designed with method shadowing concerns in mind. This would run the risk of breakage:
+
+```rust
+struct Concrete;
+
+impl Concrete {
+    fn wardrobe(self: Weak<Self>) { }
+}
+
+fn main() {
+    let concrete: Weak<Concrete> = /* obtain */;
+    concrete.wardrobe()
+}
+```
+
+If Rust now adds `Weak::wardrobe(self)`, the above valid code would start to error.
+
+The same would apply in this slightly different circumstance:
+
+```rust
+struct Concrete;
+
+impl Concrete {
+    fn wardrobe(self: &Weak<Self>) { } // this is now a reference
+}
+
+fn main() {
+    let concrete: Weak<Concrete> = /* obtain */;
+    concrete.wardrobe()
+}
+```
+
+If Rust added `Weak::wardrobe(&self)` we would start to produce an error here. If Rust added `Weak::wardrobe(self)` then it would be
+even worse - code would start to call `Weak::wardrobe` where it had previously called `Concrete::wardrobe`.
+
+The [#compiler-changes-deshadowing](deshadowing section of the compiler changes, above), describes how we avoid this. The compiler will take pains to identify any such ambiguities. If it finds them, it will warn of the situation and then choose the innermost method (in the example above, always `Concrete::wardrobe`).
 
 # Rationale and alternatives
 [rationale-and-alternatives]: #rationale-and-alternatives
@@ -367,10 +440,6 @@ This RFC proposes to implement `Receiver` for `*mut T` and `*const T` within the
 
 We prefer the option of specifying behavior in the library using the normal trait, though it's a compatibility break for users of Rust who don't adopt the `core` crate (including compiler tests).
 
-## Implement for `Weak` and `NonNull`
-
-`Weak<T>` and `NonNull<T>` were not supported by the prior unstable arbitrary self types support, but they share the property that it may be desirable to implement method calls to `T` using them as self types. Unfortunately they also share the property that these types have many Rust methods using `self`, `&self` or `&mut self`. If we added to the set of Rust methods in future, we'd [shadow any such method calls](#method-shadowing). We can't implement `Receiver` for these types unless we come up with a policy that all subsequent additions to these types would instead be associated functions. That would make the future APIs for these types a confusing mismash of methods and associated functions, and the extra user complexity doesn't seem merited.
-
 ## Not do it
 [not-do-it]: #not-do-it