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.

Sign up for GitHub

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

Jump to bottom

stabilize Rc, Arc and Pin as method receivers #55880

Closed
wants to merge 1 commit into from
Closed

stabilize Rc, Arc and Pin as method receivers #55880

wants to merge 1 commit into from

Conversation

mikeyhew
Copy link
Contributor

@mikeyhew mikeyhew commented Nov 11, 2018 •
edited
Loading

This introduces a new Receiver trait, which requires Self: Deref. If the arbitrary_self_types feature is not enabled, then in check_method_receiver we require not only that the receiver type transitively derefs to Self, but also that each type in the deref chain implements Receiver in addition to Deref.

By keeping the Receiver trait unstable, we ensure that only receiver types from the standard library can be used in stable Rust.

This allows the following to be used as the type of self, without a feature flag:

  • Self, &Self, &mut Self, Box<Self> (already allowed)
  • Rc<Self>, Arc<Self>
  • Pin<&Self>, Pin<&mut Self>, etc.
  • any combination of the above, e.g. &Box<Rc<Self>>, Pin<Pin<Pin<&mut Self>>>

An FCP is required here to stabilize Rc and Arc as receiver types. As a more conservative approach, we could remove the impls of Receiver for Rc and Arc for now, and add them back later pending an RFC. We would still allow Pin to be used as a receiver type without a feature flag, but that wouldn't be a stabilization because Pin still requires the pin feature. EDIT: An FCP would still be required because we would be allowing things like &&Self.

cc #44874 #55786

@mikeyhew
stabilize Rc, Arc and Pin as method receivers
29604a3 
This introduces a new trait `Receiver`, which requires `Self: Deref`. If the `arbitrary_self_types` feature is not enabled, then in `check_method_receiver` we require not only that the receiver type transitively derefs to `Self`, but also that each type in the deref chain implements `Receiver` in addition to `Deref`.

By keeping the `Receiver` trait unstable, we ensure that only receiver types from the standard library can be used in stable Rust.

This allows the following to be used as the type of `self`, without a feature flag:

- `Self`, `&Self`, `&mut Self`, `Box<Self>` (already allowed)
- `Rc<Self>`, `Arc<Self>`
- `Pin<&Self>`, `Pin<&mut Self>`, etc.
- any combination of the above, e.g. `Box<Rc<Self>>`, `Pin<Pin<Pin<&mut Self>>>`
@rust-highfive
Copy link
Collaborator

r? @varkor

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 11, 2018
@mikeyhew
Copy link
Contributor Author

mikeyhew commented Nov 11, 2018 •
edited
Loading

I just realized how we can avoid stabilizing self: &&Self, and it's really simple: require that the receiver type directly implements Receiver<Target=Self>.

This would still allow Pin to be used without arbitrary_self_types, because Pin<P> implements Deref<Target=P::Target>. By extension, it would also allow Pin<Pin<&mut Self>>, but it's an improvement.

Closing this in favour of that approach.

@mikeyhew mikeyhew closed this Nov 11, 2018
@mikeyhew
Copy link
Contributor Author

I just saw that the the futures RFC has a method taking self: &Arc<Self>, meaning we would need to stabilize composed receivers. So I'm going to reopen this.

@mikeyhew mikeyhew reopened this Nov 12, 2018
@mikeyhew
Copy link
Contributor Author

cc @withoutboats @cramertj

@varkor varkor added T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Nov 19, 2018
@varkor
Copy link
Member

varkor commented Nov 19, 2018

r? @withoutboats

@rust-highfive rust-highfive assigned withoutboats and unassigned varkor Nov 19, 2018
@Centril Centril added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 20, 2018
@Centril
Copy link
Contributor

Centril commented Nov 20, 2018

Blocked on FCP in #55786.

@withoutboats
Copy link
Contributor

We don't need the composition, because Wake is unlikely to be in the initial stabilization pass. The stabilization proposal is now just Rc, Arc, and Pin.

@Centril
Copy link
Contributor

Centril commented Nov 20, 2018

@withoutboats btw; do we need the extra FCP here? ISTM that the FCP in #55786 should be sufficient?

@withoutboats
Copy link
Contributor

we don't

@Centril Centril added the relnotes Marks issues that should be documented in the release notes of the next release. label Nov 22, 2018
@Centril Centril added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Nov 30, 2018
@Centril
Copy link
Contributor

Centril commented Nov 30, 2018

Triage, @withoutboats: should be good to review now since FCP is ~finished (or hours from it...) :)

@mikeyhew
Copy link
Contributor Author

mikeyhew commented Dec 1, 2018 •
edited
Loading

The only thing is this allows composed receivers, which wasn't included in that FCP. I've been working on another branch that only allows receivers that directly deref to Self, but I've been swamped and haven't finished it. I can get back to working on it next week after Monday.

@Dylan-DPC-zz
Copy link

ping from triage @withoutboats awaiting your review

@withoutboats
Copy link
Contributor

@r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned withoutboats Dec 10, 2018
@eddyb
Copy link
Member

eddyb commented Dec 10, 2018

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned eddyb Dec 10, 2018
@mikeyhew
Copy link
Contributor Author

no need to review this, the replacement PR that doesn't include composed receivers should be up soon

@cramertj
Copy link
Member

cramertj commented Dec 10, 2018 •
edited
Loading

I still don't understand why composed receivers are being removed-- was there any motivation cited for this? (I asked the same in #55786 (comment))

@withoutboats
Copy link
Contributor

@cramertj it was because @mikeyhew had said they presented issues (I think just that they weren't object safe, please clarify @mikeyhew!) The code that used them was being removed from the futures RFC, so I thought it was just simpler to leave them unstable for now.

@mikeyhew
Copy link
Contributor Author

@withoutboats I'm not sure which comment you are referring to, so can't clarify what I meant. But I think my issue with stabilizing composed receivers was that they seemed like something that could be controversial, at least more so than Pin, Rc and Arc. And at the time I didn't know they were going to be used for the Futures API, so it seemed like it was better to wait for an RFC.

@mikeyhew mikeyhew mentioned this pull request Dec 14, 2018
Merged
@mikeyhew
Copy link
Contributor Author

I've opened #56805 as a replacement to this. It allows everything that this PR allows, except for composed receivers. It also includes a bit of a refactor of check_method_receiver, so if composed receivers are going to be stabilized, it should build off that PR instead of merging this one.

@mikeyhew mikeyhew closed this Dec 14, 2018
@Dylan-DPC-zz Dylan-DPC-zz removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. relnotes Marks issues that should be documented in the release notes of the next release. labels Dec 14, 2018
bors added a commit that referenced this pull request Dec 22, 2018
@bors
Auto merge of #56805 - mikeyhew:stabilize-pin-as-receiver, r=nikomats…
abaa934 
…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.
@mikeyhew mikeyhew mentioned this pull request Sep 11, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nikomatsakis nikomatsakis

Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@mikeyhew @rust-highfive @varkor @Centril @withoutboats @Dylan-DPC-zz @eddyb @cramertj @nikomatsakis