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

Stabilize nested self receivers in 1.40.0 #64325

Open
wants to merge 1 commit into
base: master
from

Conversation

@cramertj
Copy link
Member

commented Sep 9, 2019

Previously, only Self, &Self, &mut Self, Arc<Self>, Rc<Self>,
and Box<Self> were available as stable method receivers.

This commit stabilizes nested uses of all the above types.
However, nested receivers remain non-object-safe.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Sep 9, 2019

r? @zackmdavis

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

src/librustc_typeck/check/wfcheck.rs Outdated Show resolved Hide resolved
src/librustc_typeck/check/wfcheck.rs Show resolved Hide resolved
src/librustc_typeck/check/wfcheck.rs Outdated Show resolved Hide resolved
@cramertj cramertj force-pushed the cramertj:nested-self-types branch from 0e077b0 to 4355661 Sep 9, 2019
@Centril

This comment was marked as resolved.

Copy link
Member

commented Sep 11, 2019

Thought about this some more... I think it would be good to extend the lifetime elisions tests as well to account for what we're adding here to be on the safe side (although that is controlled by a different part of the compiler, that is the current state of affairs and having an insurance policy is nice).

@Centril

This comment was marked as resolved.

Copy link
Member

commented Sep 11, 2019

Also... some more thinking: It might be premature to do the impl + stabilization in one go. I think I would be more comfortable with feature gating the impl + letting it sit for a few weeks in nightly before stabilizing so we are sure there aren't any problems...

@cramertj

This comment was marked as resolved.

Copy link
Member Author

commented Sep 12, 2019

I think it would be good to extend the lifetime elisions tests as well to account for what we're adding here to be on the safe side (although that is controlled by a different part of the compiler, that is the current state of affairs and having an insurance policy is nice).

Sure, I can add some more elision-specific tests.

It might be premature to do the impl + stabilization in one go. I think I would be more comfortable with feature gating the impl + letting it sit for a few weeks in nightly before stabilizing so we are sure there aren't any problems...

This has already been available on nightly under #![feature(arbitrary_self_types)] for nearly a year now. I don't think that it'd benefit from being separately feature-gated for more time.

@Centril

This comment was marked as resolved.

Copy link
Member

commented Sep 12, 2019

This has already been available on nightly under #![feature(arbitrary_self_types)] for nearly a year now. I don't think that it'd benefit from being separately feature-gated for more time.

I mostly wanted a few weeks to test the new code paths but I guess the gating itself might be as much code as the PR itself so it might not actually test the specifics much more.

@zackmdavis

This comment has been minimized.

Copy link
Member

commented Sep 14, 2019

(going through my review debt now; since Centril has already stepped up to comment in depth, I hope it's OK if I abdicate reviewer status on this one)

r? @Centril

@rust-highfive rust-highfive assigned Centril and unassigned zackmdavis Sep 14, 2019
@Centril

This comment has been minimized.

Copy link
Member

commented Sep 14, 2019

I'm gonna pass the torch to @mikeyhew since this is the first time I've read this code. ;)

@Centril

This comment has been minimized.

Copy link
Member

commented Sep 14, 2019

Forgot to actually do that... 😅

r? @mikeyhew

@Centril

This comment has been minimized.

Copy link
Member

commented Sep 14, 2019

I guess they cannot be assigned... assigning to @nikomatsakis who reviewed #56805 and also cc @arielb1 who reviewed #45870.

r? @nikomatsakis

Copy link
Contributor

left a comment

I thought I'd take a look anyway. LGTM

src/librustc_typeck/check/wfcheck.rs Show resolved Hide resolved
@jonas-schievink jonas-schievink added this to the 1.39 milestone Sep 15, 2019
@Centril

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

I'm gonna move this over to r? @arielb1 to reduce Niko's reviewer load.

@Centril Centril modified the milestones: 1.39, 1.40 Sep 26, 2019
@JohnCSimon

This comment was marked as resolved.

Copy link

commented Oct 5, 2019

Ping from triage

This PR has sat idle for the last 17 days
@cramertj can you address @mikeyhew 's comment?
thanks!

CC: @arielb1 @Centril

Previously, only Self, &Self, &mut Self, Arc<Self>, Rc<Self>,
and Box<Self> were available as stable method receivers.

This commit stabilizes nested uses of all the above types.
However, nested receivers remain non-object-safe.
@cramertj cramertj force-pushed the cramertj:nested-self-types branch from 4355661 to 2802f51 Oct 8, 2019
@cramertj

This comment has been minimized.

Copy link
Member Author

commented Oct 8, 2019

@Centril

Thought about this some more... I think it would be good to extend the lifetime elisions tests as well to account for what we're adding here to be on the safe side (although that is controlled by a different part of the compiler, that is the current state of affairs and having an insurance policy is nice).

I've updated the elision tests here-- there were already quite a few. It was mostly a matter of sorting out which no longer required the arbitrary_self_types feature gate.

@cramertj

This comment has been minimized.

Copy link
Member Author

commented Oct 8, 2019

@rfcbot fcp merge

I'd like to propose stabilizing the nesting portion of the arbitrary_self_types feature. A number of arbitrary_self_types were stabilized in #55786. However, it was decided to omit composed receivers like &Rc<Self>, &&Self, etc. because they weren't needed for the futures and Pin MVP. However, these composed Self types are quite useful and don't represent significant additional feature surface area (see #62282 for one place in the standard library that wants them). Note that these composed Self types are not object-safe. Tests for the feature can be found here, with elision-specific tests in this subdirectory.

@rfcbot

This comment has been minimized.

Copy link

commented Oct 8, 2019

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

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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

This comment has been minimized.

Copy link

commented Oct 17, 2019

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

@Centril Centril removed the I-nominated label Oct 19, 2019
@Centril Centril changed the title Stabilize nested self receivers Stabilize nested self receivers in 1.40.0 Oct 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.