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.

Sign up for GitHub

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

Jump to bottom

Suggest using a lock for *Cell: Sync bounds #106944

Merged
merged 2 commits into from
Jan 26, 2023
Merged

Suggest using a lock for *Cell: Sync bounds #106944

merged 2 commits into from
Jan 26, 2023

Conversation

Noratrieb
Copy link
Member

I mostly did this for OnceCell<T> at first because users will be confused to see that the OnceCell<T> in std isn't Sync but then extended it to Cell<T> and RefCell<T> as well.

@rustbot
Copy link
Collaborator

rustbot commented Jan 16, 2023

r? @WaffleLapkin

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 16, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jan 16, 2023

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 @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. 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

mejrs
mejrs reviewed Jan 16, 2023
View reviewed changes
Copy link
Contributor

@mejrs mejrs left a comment

Choose a reason for hiding this comment

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

Like the std::rc and std::cell module docs, I think it would be better if we add some qualification:

if you want to do aliasing and mutation between multiple threads, use X instead

because I'm not sure this suggestion is always the right thing to do - I've seen two scenarios when users hit this error:

  • They're designing something multithreaded and really should be using Mutex/RwLock, not RefCell
  • They're designing something singlethreaded and accidentally use the type in a context requiring Sync, in which case they shouldn't do that.

library/core/src/marker.rs Outdated
on(_Self = "std::cell::OnceCell<T>", note = "use `std::sync::OnceLock` instead"),
on(
_Self = "std::cell::Cell<T>",
note = "use `std::sync::RwLock` or an atomic variable instead",
Copy link
Contributor

Choose a reason for hiding this comment

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

or an atomic variable instead

Do you mean an atomic integer? That'll only map with Cell<{integer}> where integer != i128/u128

Copy link
Member

Choose a reason for hiding this comment

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

Can we do

on(_Self = "std::cell::Cell<bool>", note = "...AtomicBool..."),
on(_Self = "std::cell::Cell<u8>", note = "...AtomicU8..."),
...

?

@WaffleLapkin WaffleLapkin added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 17, 2023
WaffleLapkin
WaffleLapkin reviewed Jan 20, 2023
View reviewed changes
library/core/src/cell/once.rs
Comment on lines +302 to +304
// Just like for `Cell<T>` this isn't needed, but results in nicer error messages.
#[unstable(feature = "once_cell", issue = "74465")]
impl<T> !Sync for OnceCell<T> {}
Copy link
Member

Choose a reason for hiding this comment

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

Actually... Removing this would be a breaking change: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=1f4ad2c6d839ce8a21237c4faa7b2e9e (tl;dr: you'll be able to impl trait for <T: Sync> and OnceCell).

Should we consult t-libs?

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not believe so (I'm not totally sure) - coherence only lets you do this in the crate where S is defined, for now there is no (stable) way to rely on a negative impl.

Copy link
Member

Choose a reason for hiding this comment

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

Right, this is easy to prove: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=1ff2fdaa8ed22c14e94aaddb348ab850.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, it is said to be a breaking change by the docs: https://doc.rust-lang.org/beta/unstable-book/language-features/negative-impls.html, so I'm again not sure about this.

@WaffleLapkin
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jan 23, 2023

📌 Commit 6d0c91f has been approved by WaffleLapkin

It is now in the queue for this repository.

@bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 23, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 23, 2023
@matthiaskrgr
Rollup merge of rust-lang#106944 - Nilstrieb:there-once-was-a-diagnos…
f843609 
…tic, r=WaffleLapkin

Suggest using a lock for `*Cell: Sync` bounds

I mostly did this for `OnceCell<T>` at first because users will be confused to see that the `OnceCell<T>` in `std` isn't `Sync` but then extended it to `Cell<T>` and `RefCell<T>` as well.
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 23, 2023
Closed
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jan 24, 2023
@Dylan-DPC
Rollup merge of rust-lang#106944 - Nilstrieb:there-once-was-a-diagnos…
b5255d2 
…tic, r=WaffleLapkin

Suggest using a lock for `*Cell: Sync` bounds

I mostly did this for `OnceCell<T>` at first because users will be confused to see that the `OnceCell<T>` in `std` isn't `Sync` but then extended it to `Cell<T>` and `RefCell<T>` as well.
@Dylan-DPC Dylan-DPC mentioned this pull request Jan 24, 2023
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 25, 2023
@matthiaskrgr
Rollup merge of rust-lang#106944 - Nilstrieb:there-once-was-a-diagnos…
11b227a 
…tic, r=WaffleLapkin

Suggest using a lock for `*Cell: Sync` bounds

I mostly did this for `OnceCell<T>` at first because users will be confused to see that the `OnceCell<T>` in `std` isn't `Sync` but then extended it to `Cell<T>` and `RefCell<T>` as well.
This was referenced Jan 25, 2023
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 25, 2023
@bors
Auto merge of rust-lang#107309 - matthiaskrgr:rollup-0wgq6be, r=matth…
2a17174 
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#105345 (Add hint for missing lifetime bound on trait object when type alias is used)
 - rust-lang#106897 (Tweak E0597)
 - rust-lang#106944 (Suggest using a lock for `*Cell: Sync` bounds)
 - rust-lang#107239 (Bring tests back into rustc source tarball)
 - rust-lang#107244 (rustdoc: rearrange HTML in primitive reference links)
 - rust-lang#107255 (add test where we ignore hr implied bounds)
 - rust-lang#107256 (Delete `SimplifyArmIdentity` and `SimplifyBranchSame` mir opts)
 - rust-lang#107266 (rustdoc: prohibit scroll bar on source viewer in Safari)
 - rust-lang#107282 (erica solver: implement builtin `Pointee` trait impl candidates)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 22e62a4 into rust-lang:master Jan 26, 2023
@rustbot rustbot added this to the 1.69.0 milestone Jan 26, 2023
@Noratrieb Noratrieb deleted the there-once-was-a-diagnostic branch January 26, 2023 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mejrs mejrs mejrs left review comments

@WaffleLapkin WaffleLapkin WaffleLapkin left review comments

Assignees

@WaffleLapkin WaffleLapkin

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.69.0
Development

Successfully merging this pull request may close these issues.
5 participants
@Noratrieb @rustbot @WaffleLapkin @bors @mejrs