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

[Stabilization] Stablize using some arbitrary self types defined in std #55786

Closed
2 tasks
withoutboats opened this issue Nov 8, 2018 · 26 comments · Fixed by #56805
Closed
2 tasks

[Stabilization] Stablize using some arbitrary self types defined in std #55786

withoutboats opened this issue Nov 8, 2018 · 26 comments · Fixed by #56805
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@withoutboats
Copy link
Contributor

withoutboats commented Nov 8, 2018

Feature name: arbitrary_self_types
Stabilization target: 1.32.0
Tracking issue: #44874
Related RFCs: rust-lang/rfcs#2362

This is a proposal to stabilize a subset of the arbitrary_self_types feature,
making it possible on stable to use valid "arbitrary" self types that have been
defined in the standard library, but not to define your own new self types.

Stabilized feature or APIs

Today, only the following types are allowed as self types unless the user uses
the arbitrary_self_types flag:

  • Self
  • &Self
  • &mut Self
  • Box<Self>

We've long desired to extend this set to all pointer types defined in std, and
(ideally) to arbitrary user defined pointer types. For quite a while now, the
ability to create user defined self types has existed on nightly under the
arbitrary_self_types flag; in this stabilization, we propose to stabilize the
extension of self types to all the relevant pointer types defined in std and
their compositions
, while leaving the ability to create your own self types
unstable for now while we iterate on the exact requirements.

The new self types that will be enabled by this stabilization are:

  • *const Self
  • *mut Self
  • Rc<Self>
  • Arc<Self>
  • Pin<P> where P is another type in this set.
  • The composition of any members in this set (e.g. &Box<Self>,
    Pin<&mut Rc<Self>>).

Additionally, all of these receiver types except for Self are object safe, in
the sense that they can be used as the receivers of object-safe trait methods.

Object safety of raw pointer types

By making pointers object-safe, we have introduced an additional requirement on
raw pointers: a wide raw pointer must contain valid metadata, even if the
data pointer is null or dangling. That is, a *const dyn Trait must have
a metadata pointer pointing to a valid vtable for Trait.

As an alternative, we could possibly restrict this to only allowing raw
pointers as the receiver types for unsafe methods, and validity of the metadata
pointer would be an invariant the caller would be expected to uphold.

Non-Deref pointer types

Library defined pointer types can only be receiver types if they implement
Deref currently. This excludes certain std pointer types that don't implement
Deref, because they could be dangling:

  • NonNull<Self>
  • rc::Weak<Self>
  • sync::Weak<Self>

This stabilization is forward compatible with someday supporting these pointers
as receiver types as we continue to iterate on the requirements of defining
arbitrary self types.

Magic traits involved

It's worth noting that currently the implementation of this functionality
involves the intersection of several ops traits:

  • Deref - a self type must be deref, transitively targeting Self
  • CoerceUnsized
  • DispatchFromDyn - this and the previous are necessary for object safe
  • (potentially) Receiver - a private trait in std used to limit stabilization
    only to these std traits

The interaction of this feature with these traits is not made stable as a
part of this proposal. The traits listed that are still unstable remain
unstable. We still have flexibility to iterate on the exact requirements on
arbitrary self types so long as those requirements include all of the types I
have enumerated previously.

Implementation changes prior to stabilization

  • Receiver trait to limit stable self types to those defined in std
    (@mikeyhew is working on this)

  • Adjust documentation and diagnostics to note the additional valid
    receiver types defined in std, instead of only the 4 accepted today

Connected features and larger milestones

A trait making use of this feature is the Future trait, which is still
unstable under the futures_api feature:

trait Future {
    type Output;
    fn poll(self: Pin<&mut Self>, lw: &LocalWaker) -> Poll<Self::Output>;
}

Before we can stabilize this trait, we need to stabilize the ability to
implement traits using Pin<&mut Self> as a receiver type: hence, this
stabilization. Stabilizing the futures_api feature is a high priority because
it is a blocker for stabilizing the async/await syntax, which makes nonblocking
IO much more ergonomic for Rust.

cc @mikeyhew

@withoutboats withoutboats added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Nov 8, 2018
@withoutboats
Copy link
Contributor Author

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Nov 8, 2018

Team member @withoutboats has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Nov 8, 2018
@withoutboats
Copy link
Contributor Author

@rfcbot concern raw-pointers-metadata

I don't have a position, but the lang team should decide: shall we require that raw wide pointers (like *const dyn Trait) always have valid metadata, or shall we require that methods using raw pointers as the receiver type always be marked unsafe?

@withoutboats
Copy link
Contributor Author

We could also delay decisions about raw pointers by not stabilizing them now, they're the only potentially dangling method receivers in the stabilization set.

@kennytm
Copy link
Member

kennytm commented Nov 8, 2018

Additionally, all of these receiver types except for Self are object safe

Do we need this exception given unsized rvalues (#48055)?

Deref - a self type must be deref, transitively targeting Self

The raw pointers don't implement Deref the trait though. Probably further justifying not stabilizing them.

@mikeyhew
Copy link
Contributor

mikeyhew commented Nov 8, 2018

I'm just chiming in since I've been CC'd. I am for everything proposed, but understand that there may be some controversy about raw-pointer methods. I would be fine with excluding them for now and waiting for an RFC.

@joshtriplett
Copy link
Member

I would also like to exclude raw pointer methods for now, and stabilize the rest.

@joshtriplett
Copy link
Member

@rfcbot concern do-not-stabilize-raw-pointers

@Centril
Copy link
Contributor

Centril commented Nov 8, 2018

I agree with @joshtriplett here re. raw pointers; I don't think sufficient justification is given for either approach mentioned so it is hard to determine what the right course of action is.

Tho I wonder a bit about process here... we haven't accepted any RFC to stabilize the rest either yet... is that OK?

@joshtriplett
Copy link
Member

@withoutboats Could you please update the proposal to exclude raw pointers?

@withoutboats
Copy link
Contributor Author

@rfcbot resolve raw-pointers-metadata

Removed raw pointers and compositions from stabilization.

@joshtriplett
Copy link
Member

I don't think compositions would have been a big problem, but this certainly makes it even easier to feel comfortable with.

@joshtriplett
Copy link
Member

@rfcbot resolve do-not-stabilize-raw-pointers
@rfcbot reviewed

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Nov 20, 2018
@rfcbot
Copy link

rfcbot commented Nov 20, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added the finished-final-comment-period The final comment period is finished for this PR / Issue. label Nov 30, 2018
@rfcbot
Copy link

rfcbot commented Nov 30, 2018

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

@rfcbot rfcbot removed the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Nov 30, 2018
@cramertj
Copy link
Member

cramertj commented Dec 3, 2018

Why were compositions removed from this? This means that e.g. &Arc<Self> doesn't work.

mikeyhew added a commit to mikeyhew/rust that referenced this issue Dec 20, 2018
This lets you write methods using `self: Rc<Self>`, `self: Arc<Self>`, `self: Pin<&mut Self>`, `self: Pin<Box<Self>`, and other combinations involving `Pin` and another stdlib receiver type, without needing the `arbitrary_self_types`. Other user-created receiver types can be used, but they still require the feature flag to use.

This is implemented by introducing a new trait, `Receiver`, which the method receiver's type must implement if the `arbitrary_self_types` feature is not enabled. To keep composed receiver types such as `&Arc<Self>` unstable, the receiver type is also required to implement `Deref<Target=Self>` when the feature flag is not enabled.

This lets you use `self: Rc<Self>` and `self: Arc<Self>` in stable Rust, which was not allowed previously. It was agreed that they would be stabilized in rust-lang#55786. `self: Pin<&Self>` and other pinned receiver types do not require the `arbitrary_self_types` feature, but they cannot be used on stable because `Pin` still requires the `pin` feature.
bors added a commit that referenced this issue Dec 22, 2018
…akis

Stabilize `Rc`, `Arc` and `Pin` as method receivers

Replaces #55880
Closes  #55786
r? @nikomatsakis
cc @withoutboats @cramertj

This lets you write methods using `self: Rc<Self>`, `self: Arc<Self>`, `self: Pin<&mut Self>`, `self: Pin<Box<Self>`, and other combinations involving `Pin` and another stdlib receiver type, without needing the `arbitrary_self_types`. Other user-created receiver types can be used, but they still require the feature flag to use.

This is implemented by introducing a new trait, `Receiver`, which the method receiver's type must implement if the `arbitrary_self_types` feature is not enabled. To keep composed receiver types such as `&Arc<Self>` unstable, the receiver type is also required to implement `Deref<Target=Self>` when the feature flag is not enabled.

This lets you use `self: Rc<Self>` and `self: Arc<Self>` in stable Rust, which was not allowed previously. It was agreed that they would be stabilized in #55786. `self: Pin<&Self>` and other pinned receiver types do not require the `arbitrary_self_types` feature, but they cannot be used on stable because `Pin` still requires the `pin` feature.
@cramertj
Copy link
Member

FWIW the removal of compositions means that futures-rs and the pin-utils crates' usage of the feature is still unstable, since they use &mut Pin<&mut T> as a self type in order to have automatic reborrowing (rather than manually calling .as_mut() (aka .reborrow()) everywhere). I'm working on removing these usages so that we can remove the extra feature flag, but it'd really be nice to have since calling .as_mut() everywhere is pretty painful (although what I really want is automatic reborrowing of Pin<&mut ...>).

@mikeyhew
Copy link
Contributor

@cramertj that doesn't seem so bad. I think it's OK that the API isn't as ergonomic as possible right now, because we can always introduce features to make it easier, but it's hard to take things back. And the fact that you're pointing out that the feature you really want is something different — that's a win.

Why is the method called as_mut if reborrow_mut is clearly a better name? That seems unfortunate, especially now that the method name has just been stabilized.

@cramertj
Copy link
Member

the fact that you're pointing out that the feature you really want is something different — that's a win.

Yeah, I agree, I'm just skeptical that anything like that is going to happen on a reasonable timescale (aka <2 years).

Why is the method called as_mut if reborrow_mut is clearly a better name?

@mikeyhew The method turns an &mut Pin<impl DerefMut<Target = T>> into an Pin<&mut T>.

@cramertj
Copy link
Member

There're also a bunch of places that were using self: &Arc<..> in futures that I've had to change to using this: &... which is pretty annoying.

@mikeyhew
Copy link
Contributor

The method turns an &mut Pin<impl DerefMut<Target = T>> into an Pin<&mut T>.

I see. That makes sense, but it is too bad that its name doesn't match its most common use. It might even be worth adding a reborrow method to better convey its purpose.

Also small nit, I think as_pin_mut would have been a better name. Too late now though 🤷‍♂️

There're also a bunch of places that were using self: &Arc<..> in futures that I've had to change to using this: &... which is pretty annoying.

Yeah, I hear you. At least those can be changed back in a backward-compatible way once composed receivers are stabilized.

@mikeyhew
Copy link
Contributor

I'm not opposed to stabilizing composed receivers, by the way. I just think it would be good to put them through the RFC process. I wasn't sure why these stabilizations didn't require an RFC — I just assumed that this issue was created because they were needed for the futures API and an RFC would have taken too long. Am I right about that?

@joshtriplett
Copy link
Member

As mentioned earlier in the process, I'd be comfortable seeing compositions stabilized too, just not raw pointers. Stabilizing references would be fine. @cramertj, would that give you what you need?

@cramertj
Copy link
Member

@joshtriplett Yes, my issues above are all in reference to compositions not being stabilized. I also believe we shouldn't stabilize raw pointers.

@joshtriplett
Copy link
Member

joshtriplett commented Dec 27, 2018 via email

@Centril
Copy link
Contributor

Centril commented Dec 28, 2018

Is there anyone who doesn't want compositions? If not, @withoutboats, could you please add compositions back for stabilization?

For the purposes of rfcbot, please create a new stabilization issue + report for compositions if y'all want to add that back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants