Arbitrary self types v2

[adetaylor]

This PR suggests small changes to the existing unstable "aribtrary self types" feature to make it more flexible. In particular, it suggests driving this feature from a new (ish) Receiver trait instead of from Deref, but to maintain compatibility by having a blanket implementation for all Deref types.

This is a squashed commit of many edits by various folks including @Urhengulas, @Veykril , @madsmtm and myself. Thanks also to @davidhewitt, @Manishearth and many folks over on Zulip for feedback.

Rendered

Tracking issue:

• Tracking issue for RFC 3519: arbitrary_self_types rust#44874

[adetaylor]

Here's the zulip thread where this has mostly been discussed

[Nadrieril]

Doesn't that mean that adding methods on *const T, like add(), is a breaking change? How has this been dealt with so far?

[adetaylor]

Good point. There are various Rust standard library types - Box, Arc etc. - where by policy no new methods are added because it would be a breaking change. Instead, associated functions are added.

If arbitrary self types is enabled (either the existing nightly unstable version, or our slightly tweaked version here) then that policy would need to be extended to the raw pointer types as well.

The Rust community therefore needs to decide which is higher priority:

• being able to add new methods to raw pointer types; or
• being able to receive method calls by raw pointer.

Unless someone can see some workaround or compromise?

@Manishearth hello, I think you were especially keen that arbitrary self types continues to support raw pointers. Do you have any views?

(Personally I think this might be a good argument to enable arbitrary self types without raw pointers, and therefore to require people to write their own newtype wrappers if they want method dispatch).

[adetaylor]

(there's a little bit more discussion of this in this comment thread below which I'm going to close to avoid duplication)

[Nadrieril]

I wanted to reply to your comment about changing priority: I think this new problem is a much better problem. The original issue is "the pointer type *const T can't add any new methods ever", the new issue is "crate A can add foo(*const Self) methods, except that if they're from the set of known *const T methods, this is a major semver change". This is a bit surprising but highly manageable.

What worries me more is the high confusion cost of making method resolution more complicated.

[adetaylor]

I'm still thinking this through but I wanted to note that we Rust doesn't current shadow things invisibly; instead it gives an error message if there's ambiguity.

• For arbitrary self types which are not raw pointers: we show the ambiguity as an error.
• For arbitrary self types which are raw pointers, we show similar errors if there are ambiguities, and we even warn if there are known upcoming future ambiguities:

   = warning: once this associated item is added to the standard library, the ambiguity may cause an error or change in behavior!
   = note: for more information, see issue #48919 <https://github.com/rust-lang/rust/issues/48919>
   = help: call with fully qualified syntax `MyType::addr(...)` to keep using the current method
   = help: add `#![feature(strict_provenance)]` to the crate attributes to enable `ptr::const_ptr::<impl *const T>::addr`
   = note: `#[warn(unstable_name_collisions)]` on by default

This RFC should be more explicit somewhere about the existence of these errors and warnings, especially in the Method Shadowing section - I'll adjust.

Of course, this doesn't solve the issue here: if Rust adds a new <raw pointer>::repaint() method, that will cause downstream crates to cease building if they had:

struct Bedroom;
impl Bedroom {
  fn repaint(self: *const Bedroom) {}
}

@Nadrieril 's proposal, as I understand it, is that if method resolution does result in ambiguities in such a situation, where raw pointers are involved, we don't show an error for the ambiguity but instead automatically pick Bedroom::repaint and show a warning.

I do agree that seems to be manageable and I don't think it makes method resolution cognitively more complex, because any such situations will result in a nice clear warning explaining the situation. It might make the code significantly more complex - I'll look into it.

[Nadrieril]

We mustn't change what type would be picked by Deref indeed. My guess is that the rule we want would apply only in case of an ambiguity that involves a Receiver type, and would favor the self: P<Self> method over P's inherent methods. This shouldn't be a breaking change if there isn't a Receiver involved, or if the Receiver has no inherent methods (like Box or Rc). I'd need to write it down clearly to be fully convinced that works though

[Nadrieril]

I had an example the other day: say x: &Arc<Box<T>> and I call x.foo(). Here are the methods that could be called, in my proposed order of priority ("most specific first"):

1. foo(&Arc<Box<Self>>) on T
2. foo(&Arc<Self>) on Box<T>
3. foo(&Box<Self>) on T
4. foo(&self) on Arc<T>
5. foo(&self) on Box<T>
6. foo(&self) on T
7. any trait methods

If we want "adding inherent methods to a Receiver type is not a breaking change", we need 4, 5 and 6 to be the lowest priority before trait methods. Question is: does this order break anything compared to what's allowed on stable today?

The current implementation of arbitrary_self_types sometimes errors on some of these combinations. The interesting cases for us is when 3 and 5 are available, or 1 and 4. This currently errors as ambiguous; we would instead pick 3 and 1 respectively.

[Nadrieril]

Ok so I did a thorough experiment (try out my madness here). It appears the algorithm picks 4 over 3 in my list above, which could appear to break my proposal. The rest of the order is ok, so today's method resolution algorithm is compatible with this ordering:

a. foo(&Arc<Box<Self>>) on T
b. foo(&Arc<Self>) on Box<T>
c. foo(&self) on Arc<T>
d. foo(&Box<Self>) on T
e. foo(&self) on Box<T>
f. foo(&self) on T

However! All is still fine. Indeed, if Arc was not Deref, we would only consider the following cases:

a. foo(&Arc<Box<Self>>) on T
b. foo(&Arc<Self>) on Box<T>
c. foo(&self) on Arc<T>

Here adding an inherent method to Arc would not be a breaking change. The reason adding an inherent method to Arc is breaking is because Arc: Deref, and we already know that adding methods to Deref types is a breaking change.

In short: if we take the current implementation of arbitrary_self_types and simply accept more cases that are currently errors, we can get the order above and the property that "adding inherent methods to a Receiver type is not a breaking change".

Moreover! Given that the only stable Receiver types don't have inherent methods (I think, right?), I believe we can even change this order a bit without breakage. It will only break users of the unstable arbitrary_self_types feature in that one corner case. So we can pick my initial "most specific first" order too if we prefer.

I think that proves that we're good? Did I miss anything?

[Nadrieril]

I grepped through std: the stable Receivers are exactly Box, Rc, Arc, Pin, &T, and &mut T. None of these have inherent methods, except Pin, which has e.g. Pin::as_ref(). So we can't change the order. But we can still do the first thing

[Manishearth]

@Manishearth hello, I think you were especially keen that arbitrary self types continues to support raw pointers. Do you have any views?

Missed this. Yeah, I think it's important for the ergonomics of unsafe code. Wrapper types are an okay workaround, as are free functions.

Raw pointers are basically the native rust version of "I want to be able to hold a CppPointer<T> without breaking it", so it would be ideal for

I think #[fundamental]'s behavior may be useful for dealing with raw pointer receivers: I don't think there's anything new here that doesn't apply to &T as well. I think it's fine for Rust to need new editions for new methods on *const T; and there are some ways of making that work even without editions.

[Veykril]

There is actually a problem here that I didn't realize back when we talked about this. Implementing Receiver is effectively a promise to not add any new methods to the implementing type (given the reasons already outlined here, as adding methods could break downstream users for these kinds of types). This impl means, this promise now propagates to Deref implementations as well which seems like a very much unwanted side effect, especially given all the current Deref impls out there.

[davidhewitt]

Isn't this already true of Deref implementations? Adding a new method to a type A implementing Deref<Target = B> will shadow a method of the same name on B, so I think there is no change in semantics by adding Receiver to the mix?

[davidhewitt]

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=ba3385bf2e83d371bc8e603a641646a5

EDIT: original link mistakenly missed closing parenthesis.

[Veykril]

No because you can't add inherent impls to a foreign type, so you can't add new inherent methods to them (ignoring fundamental types). With the Receiver trait you can kind of add new inherent methods to foreign types though.

//- crate: foo
pub struct Foo<T>(pub T);
impl<T> Receiver for Foo<T> {
    type Target = T;
}
//- crate: bar (depends on foo)
use foo::Foo;
struct Bar;
impl Bar {
    fn foobar(self: Foo<Bar>) {}
}
fn main() {
    Foo(Bar).foobar();
}

Adding a fn foobar(self) method to Foo would break the crate bar in this case, so adding a method to a Receiver implementing type is a breaking change. And since Deref here implies implementing Receiver, the same issue arises from just Deref implementations, that is the same issue would occur if instead of the Receiver impl in that example we had

impl<T> Deref for Foo<T> {
    type Target = T;
    fn deref(&self) -> &Self::Target { &self.0 }
}

[davidhewitt]

Discarding Receiver from this RFC for the moment, I'm still missing what makes you take the stance that Deref implementors on today's Rust don't already carry the same promise not to add new methods. As far as I understand, this problem already exists with Deref, so adding Receiver to all Deref types changes nothing.

[obi1kenobi]

I also agree that it's a breaking but non-major change. Adding a blanket impl for an existing trait, implementing a new method on an existing type, and newly implementing a trait for an existing type are all breaking changes (and ones deemed non-major) regardless of arbitrary self types.

[Nadrieril]

Oh I see. Doesn't Deref already have this exact problem though? That's exactly why Box/Rc/etc have no normal methods, since adding a fn foo(&self) to Box would break x.foo() on x: Box<Foo> if Foo has fn foo(&self)

[Veykril]

I don't see how Deref already has this problem (unless you are referring to the current Deref version of this feature prior to this RFC). Box, Rc, Pin etc have htis problem because they are already allowed as arbitrary receivers as they have been special cased. Deref has no say in that. With the RFC as written, all Deref implementations will have this problem. Anyways, even if this is considered a minor breakage only (which I'd argue against personally, it very much feels like a major breakage to me), the RFC should definitely talk about this, both in that implementing Receiver has this semver problem as well as that the blanket impl will extend it to Deref impls.

[Nadrieril]

No I mean plain Deref. Let me spell it out:

//- crate: foo (depends on nothing)
pub struct Foo<T>(pub T);
impl<T> Deref for Foo<T> {
    type Target = T;
    fn deref(...) {...}
}

//- crate: bar (depends on nothing)
pub struct Bar;
impl Bar {
    pub fn foobar(&self) {}
}

//- crate: qux (depends on bar and foo)
fn main() {
    Foo(Bar).foobar(); // resolves to `Bar::foobar`
}

Now, if crate foo adds the following:

impl<T> Foo<T> {
    pub fn foobar(&self) {}
}

then Foo(Bar).foobar() in crate qux now resolves to Foo<T>::foobar. I imagine this is a major breaking change since now qux could still compile yet be using a completely different function.

This is the same problem; Deref pointers already have to deal with it today. Hence I conclude: adding Receiver into the mix should not change anything, all is good.

[madsmtm]

I agree with @Nadrieril, this is a non-issue for this RFC, since the problem is already present on Deref types.

The RFC actually has a section that talks about this, I've updated that in #3519 (review) to include a few more references to external resources that further explain the problem.

I'll also link to rust-lang/rust-clippy#11820, which proposes a new clippy lint to catch inherent methods on smart pointers, to hopefully help implementers of such types (including in the future, implementers of Receiver) to avoid this pitfall.

[programmerjake]

another good use case for arbitrary self types that may be worth mentioning in the rfc:

#[derive(Default)]
pub struct GCArena {
    objects: Vec<Rc<dyn Any>>,
}

impl GCArena {
    pub fn alloc<T: 'static>(&mut self, value: T) -> Gc<T> {
        let rc: Rc<T> = Rc::new(value);
        let weak = Rc::downgrade(&rc);
        self.objects.push(rc);
        Gc(weak)
    }
}

#[derive(Clone)]
pub struct Gc<T: 'static + ?Sized>(Weak<T>);

impl PartialEq for Gc<T: 'static + ?Sized> {
    fn eq(&self, other: &Gc<T>) -> bool {
        self.0.ptr_eq(&other.0)
    }
}

impl Eq for Gc<T: 'static + ?Sized> {}

impl Hash for Gc<T: 'static + ?Sized> {
    fn hash<H: Hasher>(&self, state: &mut H) {
        (self.0.as_ptr() as *const ()).hash(state);
    }
}

impl<T: 'static + ?Sized> Receiver for Gc<T> {
    type Target = T;
}

impl<T: 'static + ?Sized> Receiver for Gc<T> {
    pub fn get(&self) -> Rc<T> {
        self.0.upgrade().expect("GCArena has been dropped")
    }
}

// demo of using Gc:

pub struct Node<T> {
    pub edges: Vec<Gc<Node<T>>>,
    pub data: T,
}

impl<T> Node<T> {
    pub fn walk(self: &Gc<Self>, seen: &mut HashSet<Gc<Self>>, f: &mut impl FnMut(&Gc<Self>)) {
        if seen.insert(self.clone()) {
            f(self);
            for i in &self.get().edges {
                i.walk(seen, f);
            }
        }
    }
}

[clarfonthey]

@programmerjake I think this is now covered by the paragraph I wrote that is now part of the RFC?

For example, taking &Arc allows me to both clone the smart pointer (noting that the underlying T might not implement Clone) in addition to access the data inside the type, which is useful for some methods. Also, being able to change a method from accepting &self to self: &Arc can be done in a mostly frictionless way, whereas changing from &self to a static method accepting &Arc will always require some amount of refactoring.

This feels like the same case you present: you want the ability to both access the inner type and clone the smart pointer, and simply accessing the inner type isn't enough.

[programmerjake]

This feels like the same case you present: you want the ability to both access the inner type and clone the smart pointer, and simply accessing the inner type isn't enough.

well, i had intended to demonstrate a type where you can't just impl Deref, since the inner value may have already been dropped -- I didn't do a very good job of that.

another use case: a type where the inner value doesn't exist locally, e.g. for RPC with promise pipelining:

pub struct Id(u32);
pub struct Promise<T>(Id, PhantomData<T>);

impl<T> Receiver for Promise<T> {
    type Target = T;
}

pub struct MyRemoteData;
pub struct SomeIntermediate;

impl MyRemoteData {
    pub fn get() -> Promise<Self> {
        ...
    }
    pub fn foo(self: Promise<Self>, a: i32) -> Promise<SomeIntermediate> {
        ...
    }
}

impl SomeIntermediate {
    pub fn bar(self: Promise<Self>) -> Promise<()> {
        ...
    }
}

pub async fn demo() {
    MyRemoteData::get().foo(5).bar().await;
}

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[traviscross]

The lang team has accepted this RFC, and we've now merged it.

Thanks to @adetaylor for pushing forward this important work, and thanks to all of the reviewers who contributed helpful feedback.

For further updates on this work, follow the tracking issue:

• Tracking issue for RFC 3519: arbitrary_self_types rust#44874