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

[core] add Exclusive to sync #97629

Merged
merged 2 commits into from
Jul 1, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
173 changes: 173 additions & 0 deletions library/core/src/sync/exclusive.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,173 @@
//! Defines [`Exclusive`].

use core::fmt;
use core::future::Future;
use core::pin::Pin;
use core::task::{Context, Poll};

/// `Exclusive` provides only _mutable_ access, also referred to as _exclusive_
/// access to the underlying value. It provides no _immutable_, or _shared_
/// access to the underlying value.
///
/// While this may seem not very useful, it allows `Exclusive` to _unconditionally_
/// implement [`Sync`]. Indeed, the safety requirements of `Sync` state that for `Exclusive`
/// to be `Sync`, it must be sound to _share_ across threads, that is, it must be sound
/// for `&Exclusive` to cross thread boundaries. By design, a `&Exclusive` has no API
/// whatsoever, making it useless, thus harmless, thus memory safe.
///
/// Certain constructs like [`Future`]s can only be used with _exclusive_ access,
/// and are often `Send` but not `Sync`, so `Exclusive` can be used as hint to the
/// rust compiler that something is `Sync` in practice.
///
/// ## Examples
/// Using a non-`Sync` future prevents the wrapping struct from being `Sync`
/// ```compile_fail
/// use core::cell::Cell;
///
/// async fn other() {}
/// fn assert_sync<T: Sync>(t: T) {}
/// struct State<F> {
/// future: F
/// }
///
/// assert_sync(State {
/// future: async {
/// let cell = Cell::new(1);
/// let cell_ref = &cell;
/// other().await;
/// let value = cell_ref.get();
/// }
/// });
/// ```
///
/// `Exclusive` ensures the struct is `Sync` without stripping the future of its
/// functionality.
/// ```
/// #![feature(exclusive_wrapper)]
/// use core::cell::Cell;
/// use core::sync::Exclusive;
///
/// async fn other() {}
/// fn assert_sync<T: Sync>(t: T) {}
/// struct State<F> {
/// future: Exclusive<F>
/// }
///
/// assert_sync(State {
/// future: Exclusive::new(async {
/// let cell = Cell::new(1);
/// let cell_ref = &cell;
/// other().await;
/// let value = cell_ref.get();
/// })
/// });
/// ```
///
/// ## Parallels with a mutex
/// In some sense, `Exclusive` can be thought of as a _compile-time_ version of
/// a mutex, as the borrow-checker guarantees that only one `&mut` can exist
/// for any value. This is a parallel with the fact that
/// `&` and `&mut` references together can be thought of as a _compile-time_
/// version of a read-write lock.
///
///
/// [`Sync`]: core::marker::Sync
#[unstable(feature = "exclusive_wrapper", issue = "98407")]
#[doc(alias = "SyncWrapper")]
#[doc(alias = "SyncCell")]
#[doc(alias = "Unique")]
// `Exclusive` can't have `PartialOrd`, `Clone`, etc. impls as they would
// use `&` access to the inner value, violating the `Sync` impl's safety
// requirements.
#[derive(Default)]
guswynn marked this conversation as resolved.
Show resolved Hide resolved
#[repr(transparent)]
guswynn marked this conversation as resolved.
Show resolved Hide resolved
pub struct Exclusive<T: ?Sized> {
inner: T,
}

// See `Exclusive`'s docs for justification.
#[unstable(feature = "exclusive_wrapper", issue = "98407")]
unsafe impl<T: ?Sized> Sync for Exclusive<T> {}

#[unstable(feature = "exclusive_wrapper", issue = "98407")]
impl<T: ?Sized> fmt::Debug for Exclusive<T> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> Result<(), fmt::Error> {
f.debug_struct("Exclusive").finish_non_exhaustive()
}
}

impl<T: Sized> Exclusive<T> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason for the explicit : Sized bound?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to make it clear how its different than the other impl block

/// Wrap a value in an `Exclusive`
#[unstable(feature = "exclusive_wrapper", issue = "98407")]
#[must_use]
pub const fn new(t: T) -> Self {
Self { inner: t }
}

/// Unwrap the value contained in the `Exclusive`
#[unstable(feature = "exclusive_wrapper", issue = "98407")]
#[must_use]
pub const fn into_inner(self) -> T {
self.inner
}
}

impl<T: ?Sized> Exclusive<T> {
/// Get exclusive access to the underlying value.
#[unstable(feature = "exclusive_wrapper", issue = "98407")]
#[must_use]
pub const fn get_mut(&mut self) -> &mut T {
&mut self.inner
}

/// Get pinned exclusive access to the underlying value.
///
/// `Exclusive` is considered to _structurally pin_ the underlying
/// value, which means _unpinned_ `Exclusive`s can produce _unpinned_
/// access to the underlying value, but _pinned_ `Exclusive`s only
/// produce _pinned_ access to the underlying value.
#[unstable(feature = "exclusive_wrapper", issue = "98407")]
#[must_use]
pub const fn get_pin_mut(self: Pin<&mut Self>) -> Pin<&mut T> {
// SAFETY: `Exclusive` can only produce `&mut T` if itself is unpinned
// `Pin::map_unchecked_mut` is not const, so we do this conversion manually
unsafe { Pin::new_unchecked(&mut self.get_unchecked_mut().inner) }
}

/// Build a _mutable_ references to an `Exclusive<T>` from
/// a _mutable_ reference to a `T`. This allows you to skip
/// building an `Exclusive` with [`Exclusive::new`].
#[unstable(feature = "exclusive_wrapper", issue = "98407")]
#[must_use]
pub const fn from_mut(r: &'_ mut T) -> &'_ mut Exclusive<T> {
// SAFETY: repr is ≥ C, so refs have the same layout; and `Exclusive` properties are `&mut`-agnostic
unsafe { &mut *(r as *mut T as *mut Exclusive<T>) }
}

/// Build a _pinned mutable_ references to an `Exclusive<T>` from
/// a _pinned mutable_ reference to a `T`. This allows you to skip
/// building an `Exclusive` with [`Exclusive::new`].
#[unstable(feature = "exclusive_wrapper", issue = "98407")]
#[must_use]
pub const fn from_pin_mut(r: Pin<&'_ mut T>) -> Pin<&'_ mut Exclusive<T>> {
// SAFETY: `Exclusive` can only produce `&mut T` if itself is unpinned
// `Pin::map_unchecked_mut` is not const, so we do this conversion manually
unsafe { Pin::new_unchecked(Self::from_mut(r.get_unchecked_mut())) }
guswynn marked this conversation as resolved.
Show resolved Hide resolved
}
}

#[unstable(feature = "exclusive_wrapper", issue = "98407")]
impl<T> From<T> for Exclusive<T> {
fn from(t: T) -> Self {
Self::new(t)
}
}

#[unstable(feature = "exclusive_wrapper", issue = "98407")]
impl<T: Future + ?Sized> Future for Exclusive<T> {
type Output = T::Output;

fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
self.get_pin_mut().poll(cx)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we have impls of FnMut and FnOnce as well?

Ditto for Read, Write, and basically any stdlib trait with &mut self methods exclusively

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that Read::is_read_vectored and Write::is_write_vectored take &self, so those would both have to remain their default false rather than forwarding inward.

We could generalize that principle to implement traits where all required methods are owned or mutable, leaving any &self methods to their trait default. That could include Iterator, for instance, implementing next but not size_hint. I'm not sure how far we should go with that, given that you can use get_mut or into_inner for direct access anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, we could for starters limit ourselves to Fn{Mut,Once}, then, before considering the others which can always have a manual impl through a wrapper struct: not only are these the only traits stable users can't implement themselves, it's also sufficiently simple traits for them being implemented to be non-controversial.

  • A follow-up PR for the remaining traits would be easy to make 🙂

And even beyond manual impls on wrapper structs, we can also use ad-hoc closures and from_fn-like constructors (currently only for Iterator):

let mut sync_fn = move || exclusive_fn(); // wouldn't be needed with the fn impls
let mut sync_fut = async move { exclusive_fut.await }; // isn't needed thanks to the future impl
let mut sync_iterator = iter::from_fn(move || exclusive_iterator.next());

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, its a bit weird to me to only be able to forward some methods. The guidance we decide on would be: "if all the required methods only require &mut, then its okay to implement", as @cuviper mentioned, but i am tempted to only implement the ones that offer full functionality (Future, FnOnce, FnMut), or none of them

Copy link
Member

@cuviper cuviper Jun 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that FnOnce and FnMut are compelling for their simplicity and that they are unstable for users. It also relates to this forum thread, although I think they probably don't need the direct trait, per se.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't impl's like these easy to add instantly-stable in the future?

3 changes: 3 additions & 0 deletions library/core/src/sync/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,6 @@
#![stable(feature = "rust1", since = "1.0.0")]

pub mod atomic;
mod exclusive;
#[unstable(feature = "exclusive_wrapper", issue = "98407")]
pub use exclusive::Exclusive;
1 change: 1 addition & 0 deletions library/std/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,7 @@
#![feature(duration_checked_float)]
#![feature(duration_constants)]
#![feature(exact_size_is_empty)]
#![feature(exclusive_wrapper)]
#![feature(extend_one)]
#![feature(float_minimum_maximum)]
#![feature(hasher_prefixfree_extras)]
Expand Down
2 changes: 2 additions & 0 deletions library/std/src/sync/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@
pub use alloc_crate::sync::{Arc, Weak};
#[stable(feature = "rust1", since = "1.0.0")]
pub use core::sync::atomic;
#[unstable(feature = "exclusive_wrapper", issue = "98407")]
pub use core::sync::Exclusive;

#[stable(feature = "rust1", since = "1.0.0")]
pub use self::barrier::{Barrier, BarrierWaitResult};
Expand Down