[core] add `Exclusive` to sync #97629

guswynn · 2022-06-01T19:23:08Z

(discussed here: https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Adding.20.60SyncWrapper.60.20to.20std)

Exclusive is a wrapper that exclusively allows mutable access to the inner value if you have exclusive access to the wrapper. It acts like a compile time mutex, and hold an unconditional Sync implementation.

Justification for inclusion into std

This wrapper unblocks actual problems:
- The example that I hit was a vector of futures::future::BoxFuture's causing a central struct in a script to be non-Sync. To work around it, you either write really difficult code, or wrap the futures in a needless mutex.
Easy to maintain: this struct is as simple as a wrapper can get, and its Sync implementation has very clear reasoning
Fills a gap: &/&mut are to RwLock as Exclusive is to Mutex

Public Api

// core::sync
#[derive(Default)]
struct Exclusive<T: ?Sized> { ... }

impl<T: ?Sized> Sync for Exclusive {}

impl<T> Exclusive<T> {
    pub const fn new(t: T) -> Self;
    pub const fn into_inner(self) -> T;
}

impl<T: ?Sized> Exclusive<T> {
    pub const fn get_mut(&mut self) -> &mut T;
    pub const fn get_pin_mut(Pin<&mut self>) -> Pin<&mut T>;
    pub const fn from_mut(&mut T) -> &mut Exclusive<T>;
    pub const fn from_pin_mut(Pin<&mut T>) -> Pin<&mut Exclusive<T>>;
}

impl<T: Future> Future for Exclusive { ... }

impl<T> From<T> for Exclusive<T> { ... }
impl<T: ?Sized> Debug for Exclusive { ... }

Naming

This is a big bikeshed, but I felt that Exclusive captured its general purpose quite well.

Stability and location

As this is so simple, it can be in core. I feel that it can be stabilized quite soon after it is merged, if the libs teams feels its reasonable to add. Also, I don't really know how unstable feature work in std/core's codebases, so I might need help fixing them

Tips for review

The docs probably are the thing that needs to be reviewed! I tried my best, but I'm sure people have more experience than me writing docs for Core

Implementation:

The API is mostly pulled from https://docs.rs/sync_wrapper/latest/sync_wrapper/struct.SyncWrapper.html (which is apache 2.0 licenesed), and the implementation is trivial:

its an unsafe justification for pinning
its an unsafe justification for the Sync impl (mostly reasoned about by @danielhenrymantilla here: Remove the T : Send bound for SyncWrapper to be Sync Actyx/sync_wrapper#2)
and forwarding impls, starting with derivable ones and Future

rust-highfive · 2022-06-01T19:23:11Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

rust-highfive · 2022-06-01T19:23:12Z

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

guswynn · 2022-06-01T19:23:30Z

r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs

library/core/src/sync/exclusive.rs

SabrinaJewson · 2022-06-02T06:36:01Z

library/core/src/sync/exclusive.rs

+impl<T> Exclusive<T> {
+    /// Wrap a value in an `Exclusive`
+    #[unstable(feature = "exclusive_struct", issue = "none")]
+    pub fn new(t: T) -> Self {


Suggested change

pub fn new(t: T) -> Self {

pub const fn new(t: T) -> Self {

good point! actually on unstable pretty much everything can be made const! I am not sure if I need to add attributes to make stabilization easier though...

library/core/src/sync/exclusive.rs

danielhenrymantilla

I like this proposal very much 🙂

library/core/src/sync/exclusive.rs

m-ou-se · 2022-06-02T12:13:01Z

@guswynn Can you update the top comment to include a complete API overview? So just the types, methods, and trait impl blocks, without implementation or doc comments. (Just like we try to do on tracking issues.) That makes it a bit easier for reviewers to quickly get an idea of what API this PR is adding.

guswynn · 2022-06-02T16:01:48Z

@m-ou-se let me know if thats the format you wanted!

guswynn · 2022-06-02T20:01:44Z

@danielhenrymantilla the future impl doesnt work with ?Sized because of

error[E0599]: the method `get_pin_mut` exists for struct `Pin<&mut Exclusive<T>>`, but its trait bounds were not satisfied
   --> library/core/src/sync/exclusive.rs:163:14
    |
163 |         self.get_pin_mut().poll(cx)
    |              ^^^^^^^^^^^
    |
   ::: library/core/src/pin.rs:408:1
    |
408 | pub struct Pin<P> {
    | ----------------- method `get_pin_mut` not found for this
    |
    = note: the following trait bounds were not satisfied:
            `T: Sized`

which is unclear to me, should I add a &mut Self: ?Sized bound on the get_pin_mut method?

danielhenrymantilla · 2022-06-02T20:04:32Z

Yes, anything that operates through indirection can have a ?Sized unbound on T; I suspect that only the fn new(), the fn into_inner(), and the From impl will be the ones requiring the (implicit) T : Sized bound

library/core/src/sync/exclusive.rs

danielhenrymantilla · 2022-06-07T15:40:48Z

library/core/src/sync/exclusive.rs

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


Could we have impls of FnMut and FnOnce as well?

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

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.

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());

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

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.

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

m-ou-se · 2022-06-07T16:44:26Z

Last week in the libs team meeting, we discussed a new experimental process for adding unstable features. Unlike an RFC or stabilization, we don't need a sign-off of the full team for adding an unstable API. But unlike a regular PR, we do want some time to allow for people to signal any concerns with a new API before merging it. I believe we haven't yet settled on a name for this 10 day not-yet-final comment period, so for now I'll call it the Unnamed Comment Period (UCP). I'd like to try it out on this PR.

I think this API is worth trying out as an unstable feature, and the api overview looks good to me.

So, since we don't have a bot for this yet...

@m-ou-se ucp merge

guswynn · 2022-06-21T18:12:45Z

@m-ou-se its been 10 days, is this merge-able?

m-ou-se · 2022-06-22T11:31:21Z

@m-ou-se its been 10 days, is this merge-able?

Yes! Only five days late. We should probably replace me doing this manually by a bot that doesn't have adhd. :D

m-ou-se · 2022-06-22T11:31:33Z

🤖 📣 The <unnamed> comment period, with a disposition to merge, as per the review above, is now complete.

As the ~~automated~~ human representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

m-ou-se · 2022-06-22T11:31:58Z

library/core/src/sync/exclusive.rs

+///
+///
+/// [`Sync`]: core::marker::Sync
+#[unstable(feature = "exclusive_wrapper", issue = "none")]


Can you open a tracking issue? Thanks!

guswynn · 2022-06-23T15:53:59Z

@m-ou-se #98407 created and linked to the unstable attributes in this pr!

m-ou-se · 2022-06-30T08:46:00Z

@bors r+

bors · 2022-06-30T08:46:04Z

📌 Commit 029f9aa has been approved by m-ou-se

[core] add `Exclusive` to sync (discussed here: https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Adding.20.60SyncWrapper.60.20to.20std) `Exclusive` is a wrapper that exclusively allows mutable access to the inner value if you have exclusive access to the wrapper. It acts like a compile time mutex, and hold an unconditional `Sync` implementation. ## Justification for inclusion into std - This wrapper unblocks actual problems: - The example that I hit was a vector of `futures::future::BoxFuture`'s causing a central struct in a script to be non-`Sync`. To work around it, you either write really difficult code, or wrap the futures in a needless mutex. - Easy to maintain: this struct is as simple as a wrapper can get, and its `Sync` implementation has very clear reasoning - Fills a gap: `&/&mut` are to `RwLock` as `Exclusive` is to `Mutex` ## Public Api ```rust // core::sync #[derive(Default)] struct Exclusive<T: ?Sized> { ... } impl<T: ?Sized> Sync for Exclusive {} impl<T> Exclusive<T> { pub const fn new(t: T) -> Self; pub const fn into_inner(self) -> T; } impl<T: ?Sized> Exclusive<T> { pub const fn get_mut(&mut self) -> &mut T; pub const fn get_pin_mut(Pin<&mut self>) -> Pin<&mut T>; pub const fn from_mut(&mut T) -> &mut Exclusive<T>; pub const fn from_pin_mut(Pin<&mut T>) -> Pin<&mut Exclusive<T>>; } impl<T: Future> Future for Exclusive { ... } impl<T> From<T> for Exclusive<T> { ... } impl<T: ?Sized> Debug for Exclusive { ... } ``` ## Naming This is a big bikeshed, but I felt that `Exclusive` captured its general purpose quite well. ## Stability and location As this is so simple, it can be in `core`. I feel that it can be stabilized quite soon after it is merged, if the libs teams feels its reasonable to add. Also, I don't really know how unstable feature work in std/core's codebases, so I might need help fixing them ## Tips for review The docs probably are the thing that needs to be reviewed! I tried my best, but I'm sure people have more experience than me writing docs for `Core` ### Implementation: The API is mostly pulled from https://docs.rs/sync_wrapper/latest/sync_wrapper/struct.SyncWrapper.html (which is apache 2.0 licenesed), and the implementation is trivial: - its an unsafe justification for pinning - its an unsafe justification for the `Sync` impl (mostly reasoned about by `@danielhenrymantilla` here: Actyx/sync_wrapper#2) - and forwarding impls, starting with derivable ones and `Future`

[core] add `Exclusive` to sync (discussed here: https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Adding.20.60SyncWrapper.60.20to.20std) `Exclusive` is a wrapper that exclusively allows mutable access to the inner value if you have exclusive access to the wrapper. It acts like a compile time mutex, and hold an unconditional `Sync` implementation. ## Justification for inclusion into std - This wrapper unblocks actual problems: - The example that I hit was a vector of `futures::future::BoxFuture`'s causing a central struct in a script to be non-`Sync`. To work around it, you either write really difficult code, or wrap the futures in a needless mutex. - Easy to maintain: this struct is as simple as a wrapper can get, and its `Sync` implementation has very clear reasoning - Fills a gap: `&/&mut` are to `RwLock` as `Exclusive` is to `Mutex` ## Public Api ```rust // core::sync #[derive(Default)] struct Exclusive<T: ?Sized> { ... } impl<T: ?Sized> Sync for Exclusive {} impl<T> Exclusive<T> { pub const fn new(t: T) -> Self; pub const fn into_inner(self) -> T; } impl<T: ?Sized> Exclusive<T> { pub const fn get_mut(&mut self) -> &mut T; pub const fn get_pin_mut(Pin<&mut self>) -> Pin<&mut T>; pub const fn from_mut(&mut T) -> &mut Exclusive<T>; pub const fn from_pin_mut(Pin<&mut T>) -> Pin<&mut Exclusive<T>>; } impl<T: Future> Future for Exclusive { ... } impl<T> From<T> for Exclusive<T> { ... } impl<T: ?Sized> Debug for Exclusive { ... } ``` ## Naming This is a big bikeshed, but I felt that `Exclusive` captured its general purpose quite well. ## Stability and location As this is so simple, it can be in `core`. I feel that it can be stabilized quite soon after it is merged, if the libs teams feels its reasonable to add. Also, I don't really know how unstable feature work in std/core's codebases, so I might need help fixing them ## Tips for review The docs probably are the thing that needs to be reviewed! I tried my best, but I'm sure people have more experience than me writing docs for `Core` ### Implementation: The API is mostly pulled from https://docs.rs/sync_wrapper/latest/sync_wrapper/struct.SyncWrapper.html (which is apache 2.0 licenesed), and the implementation is trivial: - its an unsafe justification for pinning - its an unsafe justification for the `Sync` impl (mostly reasoned about by ``@danielhenrymantilla`` here: Actyx/sync_wrapper#2) - and forwarding impls, starting with derivable ones and `Future`

[core] add `Exclusive` to sync (discussed here: https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Adding.20.60SyncWrapper.60.20to.20std) `Exclusive` is a wrapper that exclusively allows mutable access to the inner value if you have exclusive access to the wrapper. It acts like a compile time mutex, and hold an unconditional `Sync` implementation. ## Justification for inclusion into std - This wrapper unblocks actual problems: - The example that I hit was a vector of `futures::future::BoxFuture`'s causing a central struct in a script to be non-`Sync`. To work around it, you either write really difficult code, or wrap the futures in a needless mutex. - Easy to maintain: this struct is as simple as a wrapper can get, and its `Sync` implementation has very clear reasoning - Fills a gap: `&/&mut` are to `RwLock` as `Exclusive` is to `Mutex` ## Public Api ```rust // core::sync #[derive(Default)] struct Exclusive<T: ?Sized> { ... } impl<T: ?Sized> Sync for Exclusive {} impl<T> Exclusive<T> { pub const fn new(t: T) -> Self; pub const fn into_inner(self) -> T; } impl<T: ?Sized> Exclusive<T> { pub const fn get_mut(&mut self) -> &mut T; pub const fn get_pin_mut(Pin<&mut self>) -> Pin<&mut T>; pub const fn from_mut(&mut T) -> &mut Exclusive<T>; pub const fn from_pin_mut(Pin<&mut T>) -> Pin<&mut Exclusive<T>>; } impl<T: Future> Future for Exclusive { ... } impl<T> From<T> for Exclusive<T> { ... } impl<T: ?Sized> Debug for Exclusive { ... } ``` ## Naming This is a big bikeshed, but I felt that `Exclusive` captured its general purpose quite well. ## Stability and location As this is so simple, it can be in `core`. I feel that it can be stabilized quite soon after it is merged, if the libs teams feels its reasonable to add. Also, I don't really know how unstable feature work in std/core's codebases, so I might need help fixing them ## Tips for review The docs probably are the thing that needs to be reviewed! I tried my best, but I'm sure people have more experience than me writing docs for `Core` ### Implementation: The API is mostly pulled from https://docs.rs/sync_wrapper/latest/sync_wrapper/struct.SyncWrapper.html (which is apache 2.0 licenesed), and the implementation is trivial: - its an unsafe justification for pinning - its an unsafe justification for the `Sync` impl (mostly reasoned about by ```@danielhenrymantilla``` here: Actyx/sync_wrapper#2) - and forwarding impls, starting with derivable ones and `Future`

…askrgr Rollup of 10 pull requests Successful merges: - rust-lang#97629 ([core] add `Exclusive` to sync) - rust-lang#98503 (fix data race in thread::scope) - rust-lang#98670 (llvm-wrapper: adapt for LLVMConstExtractValue removal) - rust-lang#98671 (Fix source sidebar bugs) - rust-lang#98677 (For diagnostic information of Boolean, remind it as use the type: 'bool') - rust-lang#98684 (add test for 72793) - rust-lang#98688 (interpret: add From<&MplaceTy> for PlaceTy) - rust-lang#98695 (use "or pattern") - rust-lang#98709 (Remove unneeded methods declaration for old web browsers) - rust-lang#98717 (get rid of tidy 'unnecessarily ignored' warnings) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jun 1, 2022

rust-highfive assigned m-ou-se Jun 1, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 1, 2022

rustbot added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 1, 2022

fbstj reviewed Jun 1, 2022

View reviewed changes

library/core/src/sync/exclusive.rs Outdated Show resolved Hide resolved

cuviper requested changes Jun 1, 2022

View reviewed changes

library/core/src/sync/exclusive.rs Outdated Show resolved Hide resolved

ChayimFriedman2 reviewed Jun 2, 2022

View reviewed changes

library/core/src/sync/exclusive.rs Outdated Show resolved Hide resolved

ChayimFriedman2 reviewed Jun 2, 2022

View reviewed changes

library/core/src/sync/exclusive.rs Outdated Show resolved Hide resolved

ChayimFriedman2 reviewed Jun 2, 2022

View reviewed changes

library/core/src/sync/exclusive.rs Show resolved Hide resolved

SabrinaJewson reviewed Jun 2, 2022

View reviewed changes

danielhenrymantilla reviewed Jun 2, 2022

View reviewed changes

library/core/src/sync/exclusive.rs Outdated Show resolved Hide resolved

guswynn force-pushed the exclusive_struct branch from 53dd8c1 to 1277caa Compare June 2, 2022 15:55

guswynn force-pushed the exclusive_struct branch from 1277caa to eeb2d6a Compare June 2, 2022 20:52

ChayimFriedman2 reviewed Jun 2, 2022

View reviewed changes

library/core/src/sync/exclusive.rs Show resolved Hide resolved

ChayimFriedman2 reviewed Jun 2, 2022

View reviewed changes

library/core/src/sync/exclusive.rs Outdated Show resolved Hide resolved

guswynn force-pushed the exclusive_struct branch from eeb2d6a to 4a6aa10 Compare June 2, 2022 22:14

guswynn changed the title ~~[core] add Exclusive to sync~~ [core] add Exclusive to sync Jun 2, 2022

ChayimFriedman2 reviewed Jun 2, 2022

View reviewed changes

library/core/src/sync/exclusive.rs Show resolved Hide resolved

guswynn force-pushed the exclusive_struct branch from 4a6aa10 to bdba027 Compare June 6, 2022 23:30

danielhenrymantilla reviewed Jun 7, 2022

View reviewed changes

infmagic2047 mentioned this pull request Jun 15, 2022

Make Resource trait opt-in, requiring #[derive(Resource)] bevyengine/bevy#5007

Closed

m-ou-se reviewed Jun 22, 2022

View reviewed changes

guswynn mentioned this pull request Jun 22, 2022

Tracking Issue for Exclusive #98407

Open

5 tasks

add tracking issue for exclusive

029f9aa

danielhenrymantilla mentioned this pull request Jun 26, 2022

[�CP] Option: add by-{ref,mut} variants to the most pervasive adapters #98536

Closed

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 30, 2022

Dylan-DPC mentioned this pull request Jun 30, 2022

Rollup of 5 pull requests #98703

Closed

matthiaskrgr mentioned this pull request Jun 30, 2022

Rollup of 13 pull requests #98723

Closed

matthiaskrgr mentioned this pull request Jun 30, 2022

Rollup of 12 pull requests #98724

Closed

matthiaskrgr mentioned this pull request Jun 30, 2022

Rollup of 10 pull requests #98730

Merged

bors merged commit 0e71d1f into rust-lang:master Jul 1, 2022

rustbot added this to the 1.64.0 milestone Jul 1, 2022

noot mentioned this pull request Jun 20, 2023

conductor: remove GossipNetwork from Driver astriaorg/astria#111

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] add `Exclusive` to sync #97629

[core] add `Exclusive` to sync #97629

guswynn commented Jun 1, 2022 •

edited by dtolnay

Loading

rust-highfive commented Jun 1, 2022

rust-highfive commented Jun 1, 2022

guswynn commented Jun 1, 2022

SabrinaJewson Jun 2, 2022

guswynn Jun 2, 2022

danielhenrymantilla left a comment

m-ou-se commented Jun 2, 2022

guswynn commented Jun 2, 2022

guswynn commented Jun 2, 2022

danielhenrymantilla commented Jun 2, 2022

danielhenrymantilla Jun 7, 2022

cuviper Jun 7, 2022

danielhenrymantilla Jun 7, 2022

guswynn Jun 7, 2022

cuviper Jun 7, 2022 •

edited

Loading

guswynn Jun 7, 2022

m-ou-se commented Jun 7, 2022

guswynn commented Jun 21, 2022

m-ou-se commented Jun 22, 2022

m-ou-se commented Jun 22, 2022

m-ou-se Jun 22, 2022

guswynn Jun 22, 2022

guswynn commented Jun 23, 2022

m-ou-se commented Jun 30, 2022

bors commented Jun 30, 2022

[core] add Exclusive to sync #97629

[core] add Exclusive to sync #97629

Conversation

guswynn commented Jun 1, 2022 • edited by dtolnay Loading

Justification for inclusion into std

Public Api

Naming

Stability and location

Tips for review

Implementation:

rust-highfive commented Jun 1, 2022

rust-highfive commented Jun 1, 2022

guswynn commented Jun 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielhenrymantilla left a comment

Choose a reason for hiding this comment

m-ou-se commented Jun 2, 2022

guswynn commented Jun 2, 2022

guswynn commented Jun 2, 2022

danielhenrymantilla commented Jun 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cuviper Jun 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-ou-se commented Jun 7, 2022

guswynn commented Jun 21, 2022

m-ou-se commented Jun 22, 2022

m-ou-se commented Jun 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guswynn commented Jun 23, 2022

m-ou-se commented Jun 30, 2022

bors commented Jun 30, 2022

[core] add `Exclusive` to sync #97629

[core] add `Exclusive` to sync #97629

guswynn commented Jun 1, 2022 •

edited by dtolnay

Loading

cuviper Jun 7, 2022 •

edited

Loading