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

Tracking issue for UnsafeCell::raw_get #66358

Closed
1 task
RalfJung opened this issue Nov 13, 2019 · 14 comments · Fixed by #88551
Closed
1 task

Tracking issue for UnsafeCell::raw_get #66358

RalfJung opened this issue Nov 13, 2019 · 14 comments · Fixed by #88551
Assignees
Labels
A-raw-pointers Area: raw pointers, MaybeUninit, NonNull B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. 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. Libs-Tracked Libs issues that are tracked on the team's project board. requires-nightly This issue requires a nightly compiler in some way. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Nov 13, 2019

#66248 added a raw pointer variant of UnsafeCell::get.

Unresolved issues:

@RalfJung RalfJung added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Nov 13, 2019
@Centril Centril added requires-nightly This issue requires a nightly compiler in some way. B-unstable Blocker: Implemented in the nightly compiler and unstable. labels Nov 13, 2019
@programmerjake
Copy link
Member

Would it work to have it be a method by adding additional implementations on * mut/const UnsafeCell<T>?

impl<T: ?Sized> *mut UnsafeCell<T> {
    pub const fn raw_get(self) -> *mut T {
        UnsafeCell::raw_get(self)
    }
}

impl<T: ?Sized> *const UnsafeCell<T> {
    pub const fn raw_get(self) -> *mut T {
        UnsafeCell::raw_get(self)
    }
}

@KodrAus KodrAus added Libs-Tracked Libs issues that are tracked on the team's project board. A-raw-pointers Area: raw pointers, MaybeUninit, NonNull labels Jul 30, 2020
@DavidVonDerau
Copy link

@RalfJung Is there an option to stabilize the associated function first and open a separate tracking issue for the method impl? I read the discussion at #66248 (comment) and it sounded like the associated function would be forward compatible with any future implementation as a method, unless I misunderstood.

@RalfJung
Copy link
Member Author

RalfJung commented May 8, 2021

@DavidVonDerau that's up to @rust-lang/libs, really -- I don't know if this is definitely forward compatible or not (though it seems plausible).

Also, don't have have stable functions with *const Self receiver type these days? A lot of time has passed since #66248.

@RalfJung
Copy link
Member Author

RalfJung commented May 8, 2021

Cc @rust-lang/libs (I had a typo in the ping above, don't know if GH sends notifications for pings in edits)

@m-ou-se m-ou-se added the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Jul 5, 2021
@dtolnay
Copy link
Member

dtolnay commented Aug 4, 2021

@rust-lang/libs-api:
@rfcbot fcp merge

I am proposing to stabilize the UnsafeCell::raw_get associated function as currently implemented in nightly:

impl<T: ?Sized> UnsafeCell<T> {
     pub const fn raw_get(this: *const Self) -> *mut T;
}

At the call site, use of this associated function looks like:

let m = MaybeUninit::<UnsafeCell<i32>>::uninit();
unsafe { UnsafeCell::raw_get(m.as_ptr()).write(5); }
let uc = unsafe { m.assume_init() };

In response to the unresolved issue that method syntax would be cleaner at the call site, apparently this use case is already covered by #44874. We can backward compatibly replace this: *const Self with self: *const Self in a future release.

Demo:

#![feature(arbitrary_self_types)]

use std::mem::MaybeUninit;

struct UnsafeCell;
struct T;

impl UnsafeCell {
    fn raw_get(self: *const Self) -> *mut T { unimplemented!() }
}

fn main() {
    let m = MaybeUninit::<UnsafeCell>::uninit();
    unsafe { m.as_ptr().raw_get().write(T) }
    let _uc = unsafe { m.assume_init() };
}

@rfcbot
Copy link

rfcbot commented Aug 4, 2021

Team member @dtolnay 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 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. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Aug 4, 2021
@rfcbot
Copy link

rfcbot commented Aug 5, 2021

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

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Aug 5, 2021
@matklad
Copy link
Member

matklad commented Aug 11, 2021

To me, it feels surprising that the name is raw_get. I would expect get_raw, similarly to get_mut|from_raw|’into_raw’. Am I missing a reason why this needs to be inconsistent?

@m-ou-se
Copy link
Member

m-ou-se commented Aug 11, 2021

I don't have a strong opinion on it. But I suppose the 'raw' applies to the UnsafeCell, not to the return type. So the 'raw' applies to the input rather than the return type: "get from a raw thing" instead of "get a raw thing". So raw_get seems fine to me.

@rust-lang/libs-api Any opinions on raw_get vs get_raw?

@BurntSushi
Copy link
Member

Spitballing other names:

  • get_mut
  • mut_from_raw
  • raw_to_mut
  • raw_get_mut

Not a huge fan of any of them (including raw_get to be honest). Do we have other instances of self (or self-like) types being raw pointers? Maybe the problem is that we just don't have an established convention for them yet.

@matklad
Copy link
Member

matklad commented Aug 11, 2021

One more thing: we have get_ptr in NonNull: https://doc.rust-lang.org/stable/std/ptr/struct.NonNull.html#method.as_ptr. Seems better to use one of ptr or raw, not both.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Aug 15, 2021
@rfcbot
Copy link

rfcbot commented Aug 15, 2021

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.

The RFC will be merged soon.

@m-ou-se
Copy link
Member

m-ou-se commented Aug 16, 2021

Spitballing other names

Note that the existing UnsafeCell::get also gives a *mut T and does not have mut in the name. (The only difference is that raw_get takes a *const Self instead of a &Self.)

Do we have other instances of self (or self-like) types being raw pointers?

Doesn't look like it.

Maybe the problem is that we just don't have an established convention for them yet.

As @dtolnay pointed out during the libs api meeting last week: raw_ seems like a good convention.

@matklad
Copy link
Member

matklad commented Aug 16, 2021

Aha, now that clicked for me! What we are emphasizing here is that self is a raw pointer: &Self vs *const Self. Which admittedly is exactly what the docs say, and what m-ou-se pointed out in the comment five days ago, seems like I just needed to read this n+1 time :-)

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Aug 19, 2021
@inquisitivecrystal inquisitivecrystal self-assigned this Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-raw-pointers Area: raw pointers, MaybeUninit, NonNull B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. 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. Libs-Tracked Libs issues that are tracked on the team's project board. requires-nightly This issue requires a nightly compiler in some way. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.