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 unsafe_cell_get_mut #76943

Closed
2 tasks done
danielhenrymantilla opened this issue Sep 19, 2020 · 6 comments
Closed
2 tasks done

Tracking Issue for unsafe_cell_get_mut #76943

danielhenrymantilla opened this issue Sep 19, 2020 · 6 comments
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@danielhenrymantilla
Copy link
Contributor

danielhenrymantilla commented Sep 19, 2020

This is a tracking issue for adding the fn get_mut (self: &mut UnsafeCell<T>) -> &mut T method.
The feature gate for the issue is #![feature(unsafe_cell_get_mut)].

History

Unresolved Questions

None

@danielhenrymantilla danielhenrymantilla added the C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. label Sep 19, 2020
@danielhenrymantilla
Copy link
Contributor Author

danielhenrymantilla commented Sep 19, 2020

This is small enough not to need an RFC, it seems, so it can go directly to the FCP: cc @RalfJung & the rest of T-Lang T-libs

@RalfJung
Copy link
Member

RalfJung commented Sep 20, 2020

FCP will happen when we move towards stabilization. We usually wait a few release cycles before doing that. Also this is T-libs territory, I don't think any language rules are being changed here (just clarified).


@rust-lang/libs this is about adding UnsafeCell::get_mut, a method to safely go from &mut UnsafeCell<T> to &mut T. This is a sound operation on its own, and all standard library interior mutable types have an equivalent function in their API surface.

However, there is a theoretical scenario where this can cause unsoundness in existing libraries: if a user-defined library, for some reason, exposes an &mut UnsafeCell<T> to a client and relied on the fact that there was no way for safe clients to do anything with that reference. I do not know why a library would do this, but I seem to recall this concern being brought up before when changes to UnsafeCell are proposed. In this sense, the change is not backwards compatible, but only if a library relied on UnsafeCell guarantees that I do not think we ever made.

@RalfJung RalfJung added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 20, 2020
@danielhenrymantilla
Copy link
Contributor Author

danielhenrymantilla commented Sep 20, 2020

if a user-defined library, for some reason, exposes an &mut UnsafeCell to a client and relied on the fact that there was no way for safe clients to do anything with that reference

Good point, although the only way they could rely on "not being to do anything with that reference" is if they (ab)used the fact that there was no other instance of type T around (singleton pattern, or through a highly generic API relying on "type-level generativity").

Otherwise it is possible to:

fn mutate<T> (p: &mut UnsafeCell<T>, value: T)
{
    *p = value.into();
}

and more generally, we can currently write:

fn with_mut<T, R> (
    p: &'_ mut UnsafeCell<T>,
    f: impl FnOnce(&'_ mut T) -> R,
) -> R
where
    T : Default,
{
    let mut prev = mem::take(p).into_inner();
    // defer putting the value back
    let mut prev = ::scopeguard::guard(prev, |it| *p = it.into());
    f(&mut *prev)
}
  • which performs a read-write of the payload when entering, and a write (+ drop in place) when leaving

@RalfJung
Copy link
Member

Ah right, you can just overwrite what is in an &mut UnsafeCell<T>. Yeah that makes it even less likely that this would be a problem.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 21, 2020
…mut, r=RalfJung

Add non-`unsafe` `.get_mut()` for `Unsafecell`

  - Tracking issue: rust-lang#76943

As discussed in: https://internals.rust-lang.org/t/add-non-unsafe-get-mut-for-unsafecell/12407

  - ### [Rendered documentation](https://modest-dubinsky-1f9f47.netlify.app/core/cell/struct.unsafecell)

This PR tries to move the sound `&mut UnsafeCell<T> -> &mut T` projection that all the "downstream" constructions were already relying on, up to the root abstraction, where it rightfully belongs, and officially blessing it.

  - this **helps reduce the amount of `unsafe` snippets out there** (_c.f._, the second commit of this PR: rust-lang@09503fd)

The fact that this getter is now expose for `UnsafeCell<T>` itself, will also help convey the idea that **`UnsafeCell` is not magical _w.r.t._ `&mut` accesses**, contrary to what some people incorrectly think.

  - Even the standard library itself at some point had such a confusion, _c.f._ this comment where there is a mention of multi-threaded (and thus _shared_) access despite dealing with exclusive references over unique ownership: https://github.com/rust-lang/rust/blob/59fb88d061544a035f3043b47594b34789204cee/library/core/src/cell.rs#L498-L499

r? @RalfJung
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 21, 2020
…mut, r=RalfJung

Add non-`unsafe` `.get_mut()` for `Unsafecell`

  - Tracking issue: rust-lang#76943

As discussed in: https://internals.rust-lang.org/t/add-non-unsafe-get-mut-for-unsafecell/12407

  - ### [Rendered documentation](https://modest-dubinsky-1f9f47.netlify.app/core/cell/struct.unsafecell)

This PR tries to move the sound `&mut UnsafeCell<T> -> &mut T` projection that all the "downstream" constructions were already relying on, up to the root abstraction, where it rightfully belongs, and officially blessing it.

  - this **helps reduce the amount of `unsafe` snippets out there** (_c.f._, the second commit of this PR: rust-lang@09503fd)

The fact that this getter is now expose for `UnsafeCell<T>` itself, will also help convey the idea that **`UnsafeCell` is not magical _w.r.t._ `&mut` accesses**, contrary to what some people incorrectly think.

  - Even the standard library itself at some point had such a confusion, _c.f._ this comment where there is a mention of multi-threaded (and thus _shared_) access despite dealing with exclusive references over unique ownership: https://github.com/rust-lang/rust/blob/59fb88d061544a035f3043b47594b34789204cee/library/core/src/cell.rs#L498-L499

r? @RalfJung
RalfJung added a commit to RalfJung/rust that referenced this issue Sep 21, 2020
…mut, r=RalfJung

Add non-`unsafe` `.get_mut()` for `Unsafecell`

  - Tracking issue: rust-lang#76943

As discussed in: https://internals.rust-lang.org/t/add-non-unsafe-get-mut-for-unsafecell/12407

  - ### [Rendered documentation](https://modest-dubinsky-1f9f47.netlify.app/core/cell/struct.unsafecell)

This PR tries to move the sound `&mut UnsafeCell<T> -> &mut T` projection that all the "downstream" constructions were already relying on, up to the root abstraction, where it rightfully belongs, and officially blessing it.

  - this **helps reduce the amount of `unsafe` snippets out there** (_c.f._, the second commit of this PR: rust-lang@09503fd)

The fact that this getter is now expose for `UnsafeCell<T>` itself, will also help convey the idea that **`UnsafeCell` is not magical _w.r.t._ `&mut` accesses**, contrary to what some people incorrectly think.

  - Even the standard library itself at some point had such a confusion, _c.f._ this comment where there is a mention of multi-threaded (and thus _shared_) access despite dealing with exclusive references over unique ownership: https://github.com/rust-lang/rust/blob/59fb88d061544a035f3043b47594b34789204cee/library/core/src/cell.rs#L498-L499

r? @RalfJung
RalfJung added a commit to RalfJung/rust that referenced this issue Sep 21, 2020
…mut, r=RalfJung

Add non-`unsafe` `.get_mut()` for `Unsafecell`

  - Tracking issue: rust-lang#76943

As discussed in: https://internals.rust-lang.org/t/add-non-unsafe-get-mut-for-unsafecell/12407

  - ### [Rendered documentation](https://modest-dubinsky-1f9f47.netlify.app/core/cell/struct.unsafecell)

This PR tries to move the sound `&mut UnsafeCell<T> -> &mut T` projection that all the "downstream" constructions were already relying on, up to the root abstraction, where it rightfully belongs, and officially blessing it.

  - this **helps reduce the amount of `unsafe` snippets out there** (_c.f._, the second commit of this PR: rust-lang@09503fd)

The fact that this getter is now expose for `UnsafeCell<T>` itself, will also help convey the idea that **`UnsafeCell` is not magical _w.r.t._ `&mut` accesses**, contrary to what some people incorrectly think.

  - Even the standard library itself at some point had such a confusion, _c.f._ this comment where there is a mention of multi-threaded (and thus _shared_) access despite dealing with exclusive references over unique ownership: https://github.com/rust-lang/rust/blob/59fb88d061544a035f3043b47594b34789204cee/library/core/src/cell.rs#L498-L499

r? @RalfJung
@KodrAus KodrAus added the Libs-Tracked Libs issues that are tracked on the team's project board. label Nov 6, 2020
@m-ou-se
Copy link
Member

m-ou-se commented Nov 28, 2020

Stabilization PR: #79485

bors added a commit to rust-lang-ci/rust that referenced this issue Dec 18, 2020
…ut, r=m-ou-se

Stabilize `unsafe_cell_get_mut`

Tracking issue: rust-lang#76943

r? `@m-ou-se`
@jonas-schievink
Copy link
Contributor

#79485 was merged 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants