Arbitrary self types v2: stabilize

[adetaylor]

This PR stabilizes the arbitrary self types v2 feature, tracked in #44874.

r? @wesleywiser

Stabilization report

I'd like to stabilize Arbitrary Self Types in some upcoming Rust version. A stabilization PR is here. What follows is the list of questions from Niko's new template plus various sections I've seen in other stabilization reports.

Summary

This feature allows custom smart pointer types to be used as method receivers:

#![feature(arbitrary_self_types)]

struct MySmartPtr<T>(T);

impl<T> core::ops::Receiver for MySmartPtr<T> {
  type Target = T;
}

struct Content;

impl Content {
  fn method(self: MySmartPtr<Self>) { // note self type
  }
}

fn main() {
  let p = MySmartPtr(Content);
  p.method();
}

What is the RFC for this feature and what changes have occurred to the user-facing design since the RFC was finalized?

Changes since the RFC:

• The RFC specified additional diagnostics where people forgot to add a T: ?Sized bound - this is in progress here - @cramertj has kindly taken this on, and has determined that this is a pre-existing more general problem which she'll work on. We do not need to block stabilization to wait for this. (The other diagnostics noted in the RFC have been added).

In two cases the RFC wasn't very specific and the implementation has required some choices:

• The RFC specified that we should ban "generic" arbitrary self types, but we failed to define what that actually meant. Subsequent discussion yielded a fairly narrow definition of "generic".
• The RFC specifies that we should attempt to detect methods on smart pointers which might "shadow" pre-existing methods on the pointee. We are going ahead and doing this, but some simplifying assumptions were made, as noted in this comment. The likelihood of shadowing occurring in these cases seems low, relative to the likelihood of false positives in these cases.

What behavior are we committing to that has been controversial? Summarize the major arguments pro/con.

In general, Arbitrary Self Types is the opposite of controversial. It has always been anomalous that some specific stdlib smart pointer types have been hard-coded; this work removes that hard-coding.

The specific points which have involved discussion during the process:

• A prior version of Arbitrary Self Types relied on the Deref trait. This version introduces a new Receiver trait, such that types can be used as self types even if they can't safely devolve to a reference (e.g. because they're a wrapper for a pointer that can't safely comply with Rust reference semantics). This has been largely uncontroversial, but I'd note there's a blanket implementation of Receiver for T: Deref. This ensures all existing smart pointer types continue to work, and reduces complexity in users' brains. I'm 100% sure that this is the right thing to do, but highlighting it here because blanket impls are fairly unusual in the Rust standard library.
• Most of the discussion and complexity has been around the deshadowing algorithm. We can't add new methods to Rust smart pointer types - e.g. Rc, Box - because we may generate errors if they match the name of methods in pointees. It's already good practice to avoid these because of the risk of shadowing pointee methods, so these types tend to have associated functions instead. Now, any such shadowing will generate an error instead of silently shadowing.
• For a while we were considering a much more complex version which would have allowed us to add new methods to Rc, Box etc. and allow smart pointer types such as NonNull and Weak. We decided not to do that.
• There is a theoretical possibility of breaking existing code noted in this comment - which we don't think can happen in practice because any such code would be pointless. (Update: we currently think it's impossible to have written such code, so this concern may not exist at all.)

Are there extensions to this feature that remain unstable? How do we know that we are not accidentally committing to those.

There is an arbitrary_self_types_pointers feature gate which allows raw pointers to be used as self types. Arguments against using this are summarized here.

This was a pre-existing aspect of the former arbitrary_self_types feature gate which has been split out into its own new feature gate because we don't want to stabilize it at this time, but we don't feel a strong need to remove this option from nightly Rust users.

Summarize the major parts of the implementation and provide links into the code (or to PRs)

The three main PRs are:

• Block generic arbitrary self types
• Library changes - adds the Receiver trait
• Compiler changes - switches the compiler to using Receiver instead of Deref; adds the deshadowing algorithm which is responsible for most of the complexity

Summarize existing test coverage of this feature

• The main PR for this feature added 17 tests: 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17. These are a combination of basic functional tests, and tests relating to questions or corner cases which arose in development. These tests should comprehensively cover the basic method dispatch, the deshadowing algorithm (the only complex bit), and additional diagnostics.
• An additional test is in this PR
• Fairly extensive playing around with this has happened in autocxx
• Rust for Linux uses arbitrary self types for its Arc type and has partially adapted to the changes here
• A previous simulated stabilization PR was here and has been through two crater runs. It had perf results which look broadly neutral to me but I'm not an expert at interpreting them. (We expect some small performance penalty by checking for shadowed methods.)

Has a call-for-testing period been conducted? If so, what feedback was received?

No; though an earlier version of arbitrary self types has been in nightly for years. This has been experimented with by multiple communities - Rust/C++ interop, Rust/Python interop, and Rust for Linux. As the RFC notes, v2 was proposed based on experiences with v1.

What outstanding bugs in the issue tracker involve this feature? Are they stabilization-blocking?

• We need to land this fix for it to work well with the unstable DispatchFromDyn and the being-stabilized CoercePointee. This should block stabilization of either this feature, or CoercePointee.

Summarize contributors to the feature by name for recognition and assuredness that people involved in the feature agree with stabilization

• Me: @adetaylor
• Folks mostly involved in motivating the requirements: @davidhewitt @Darksonn @PoignardAzur
• Contributors to the RFC process: @Nadrieril @Manishearth @RalfJung @Veykril @madsmtm @Urhengulas @mikeyhew @joshtriplett @nikomatsakis @scottmcm @programmerjake @nbdd0121 @tmandry (this is not an exhaustive list; thanks to everyone else as well)
• Reviewers of the implementation: @wesleywiser @compiler-errors

Very sorry to those who I've missed; it's been a long road :)

What FIXMEs are still in the code for that feature and why is it ok to leave them there?

None.

What static checks are done that are needed to prevent undefined behavior?

None.

In what way does this feature interact with the reference/specification, and are those edits prepared

• rustc-dev-guide update
• Rust reference updates
• Book stuff (per comments from @chriskrycho we might not even want this)
• Semver rules update

Does this feature introduce new expressions and can they produce temporaries? What are the lifetimes of those temporaries?

No.

What other unstable features may be exposed by this feature?

None, though the separate derive(CoercePointee) becomes more useful when this is also stabilized.

What is tooling support like for this feature, rustdoc/clippy/rust-analzyer/rustfmt/etc

I don't anticipate any work here being needed other than for rust-analyzer, tracked here.

[adetaylor]

@rustbot label -S-waiting-on-review

[nikomatsakis]

Thanks for this! I already see an edit to the template that could be useful, clarifying what static checks are required and linking to tests that demonstrate them, but it kind of duplicates the reference/spec work

[PoignardAzur]

So the report mentions that testing already happened in a few forms.

How interested would you be in other libraries using this feature? We could implement it in Masonry, but since we don't want our crate to be nightly-only, we'd have to either use a feature flag or keep it to a separate branch.

If we do end up writing an implementation, is there any kind of feedback you'd be specifically interested in?

[adetaylor]

How interested would you be in other libraries using this feature? We could implement it in Masonry, but since we don't want our crate to be nightly-only, we'd have to either use a feature flag or keep it to a separate branch.

Good question and thanks for the offer! I personally have run out of runway to work on this feature - so I'm keeping my fingers crossed that no major semantic changes are needed. Or if they are, it will need someone else to pick up the work. So, the most interesting and useful testing to me would be looking for corner cases or oddities in the current implementation which cause ICEs or other oddities. (An example is the interaction with CoercePointee which caused a crash). It's hard for me to guess how likely it is that you'd find any such problem. Obviously I hope you wouldn't, but real world testing always has a habit of finding fun surprises.

Overall: up to you! If you think it would be useful for Masonry long-term then perhaps it's worth having a play in the hopes and expectations that this will be stabilized soon and you can therefore merge it into your main branch before too long.

[obi1kenobi]

I'd like to ask that the cargo SemVer reference be updated to describe any new SemVer hazards that are introduced here. This doesn't have to be a stabilization blocker, but I'd appreciate it if it can be done at least shortly thereafter so I can make sure cargo-semver-checks quickly offers the best possible support for linting this excellent new functionality.

[Nadrieril]

I would personally be in favor of waiting for a bit more testing, particularly for cases where users try to use the new APIs of these crates (as opposed to just the crate authors doing the experiment).

Anecdotally I have a personal usecase where the Deref => Receiver impl is a legitimate hurdle: I have a smart pointer Ptr<Perm, T> that I always want to be Receiver but can only safely Deref for specific values of Perm, which is prevented by the blanket impl (play). Just noting that because the RFC discussion wasn't aware of such a case afaict. I don't know if this can usefully be documented somewhere.

[madsmtm]

usecase where the Deref => Receiver impl is a legitimate hurdle: I have a smart pointer Ptr<Perm, T> that I always want to be Receiver but can only safely Deref for specific values of Perm, which is prevented by the blanket impl

True! If we were designing this from the bottom-up, we'd probably have Deref: Receiver, and require users to implement Receiver themselves, but alas, we can't have that design now due to backwards compatibility.

I don't know if this can usefully be documented somewhere.

I think it would be useful, if not at least to document that you shouldn't do what the standard library is doing here if you're designing a similar API in you own crate ;)

[Kixunil]

@Nadrieril I might have the exact same use case in an experimental crate I started writing recently. I'm not sure what the design will end up like yet.

[programmerjake]

Anecdotally I have a personal usecase where the Deref => Receiver impl is a legitimate hurdle: I have a smart pointer Ptr<Perm, T> that I always want to be Receiver but can only safely Deref for specific values of Perm

that should be solvable in the future if you use a const instead of a type for Perm:

#[derive(Eq, PartialEq, Copy, Clone)]
pub struct PtrPerm {
    pub read: bool,
}

pub struct Ptr<const PERM: PtrPerm, T> {
    ptr: NonNull<T>,
}

impl<T> Receiver for Ptr<{ PtrPerm { read: false } }, T> {
    type Target = T;
}

impl<T> Deref for Ptr<{ PtrPerm { read: true } }, T> {
    type Target = T;
    fn deref(&self) -> &T {
        // Safety: we have at least `Read` permission.
        unsafe { self.ptr.as_ref() }
    }
}

[Kixunil]

@programmerjake is there an issue about exhaustive impls where I can subscribe to? That feature would be super useful for a bunch of other things too.

[programmerjake]

@programmerjake is there an issue about exhaustive impls where I can subscribe to? That feature would be super useful for a bunch of other things too.

rust-lang/project-const-generics#26

the code I wrote also uses adt_const_params

[nikomatsakis]

@rfcbot reviewed

[tmandry]

True! If we were designing this from the bottom-up, we'd probably have Deref: Receiver, and require users to implement Receiver themselves, but alas, we can't have that design now due to backwards compatibility.

Factoring out supertraits like this is something I have long thought the trait system should support. We could enable doing this in a backwards compatible way by allowing impl blocks to implement traits and their supertraits at the same time.

Assuming we do that, stabilizing Receiver now won't do anything to prevent us from making it a supertrait of Deref in the future. Since the blanket impl means users won't be able to write Receiver and Deref impl blocks for the same type, it gives us plenty of freedom to define how it should look when they can.

[BoxyUwU]

@rfcbot concern Self: AutoTrait bounds are unsound

#127323 has not been made a hard error yet, I don't feel great about stabilizing something with a known unsoundness only guarded by a FCW. At the very least the "What static checks are done that are needed to prevent undefined behaviour" needs to be updated to mention this.

[programmerjake]

@rfcbot concern Self: AutoTrait bounds are unsound

imo this should be a blocking concern on #133820
rather than here. arbitrary self types are still quite useful even if you can't do dynamic dispatch.

[RalfJung]

Seems like arbitrary_self_types lets you do some very surprising things; IMO we should not stabilize until we have a good story for why this is sound.

imo this should be a blocking concern on #133820

We have 2 features interacting in surprising ways. It's not clear which feature is "responsible". Stabilizing either risks putting ourselves in a corner we can't get out of again if it turns out the best way to fix the problem is changing the one we stabilized first.

We are lucky that this bad interaction was found before either feature got stabilized, this means we can properly think this through.

[RalfJung]

Filed as an issue: #136702

[LilyFoote]

When experimenting with this in PyO3 (PyO3/pyo3#4885), I kept running into errors like:

error[E0599]: no method named `to_cow` found for reference `&pyo3::Bound<'_, pyo3::types::PyString>` in the current scope
   --> tests/test_enum.rs:341:21
    |
341 |                 rhs.to_cow().map_or(false, |rhs| self.__str__() == rhs)
    |                     ^^^^^^ method not found in `&Bound<'_, PyString>`

Where the implementation of to_cow is:

impl<'py> PyString {
    pub fn to_cow(self: &Bound<'py, Self>) -> PyResult<Cow<'_, str>> {
        self.as_borrowed().to_cow()
    }
}

I could work around these by using path syntax:

                PyString::to_cow(rhs).map_or(false, |rhs| self.__str__() == rhs)

But it feels like this should be unnecessary. I haven't been able to reproduce these errors on a toy project, so I don't have any insight as to what the cause might be.

[nbdd0121]

You might need to enable arbitrary self types feature gate on tests/test_enum.rs to make it work.

[LilyFoote]

Ah! 💡