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

Atomics need to take &self rather than &mut sef #11583

Closed
bill-myers opened this issue Jan 15, 2014 · 11 comments · Fixed by #13036
Closed

Atomics need to take &self rather than &mut sef #11583

bill-myers opened this issue Jan 15, 2014 · 11 comments · Fixed by #13036
Milestone

Comments

@bill-myers
Copy link
Contributor

In Rust, safe code cannot create a situation where an &mut aliases anything else that is accessible.

Unsafe code can do so, but it makes sense to ban it and make that undefined behavior at least if the &muts are then used, since in addition to preserving the desirable safe semantics, it means that the information can be used in optimizations like alias analysis without fear of introducing bugs due to unsafe code creating and using aliased &muts.

However, current Atomic* and mutex implementations require an &mut self.

This is wrong, because the whole point of such classes is to support multiple concurrent users, so having them take an &mut self means that they are either pointless or that clients are breaking the rules about &mut detailed above.

So, I think atomics need to take &self, and AtomicT should simply be viewed as a variant of Cell that uses atomic operations instead of non-atomic ones.

This also applies to operations on mutexes and other lockless data structures.

Note that this means that safe code will be able use atomics (for example, by putting them in an Arc-like structure without a Freeze bound), which does introduce potential data races but not memory corruption; however, data races that can happen due to multiple atomics can be equivalently produced using multiple RwArcs, so I think this is not an issue.

There has already been quite some discussion of this in pull request #11520, and also a partial implementation (it doesn't change the compiler intrinsics themselves, only the high-level Atomic* APIs).

@brson
Copy link
Contributor

brson commented Jan 17, 2014

@nikomatsakis curious what you think of this argument.

@brson
Copy link
Contributor

brson commented Jan 17, 2014

Nominating.

@nikomatsakis
Copy link
Contributor

I think this argument makes sense -- basically atomics should declare themselves as "inherently mutable" types, as Cell does. Or non-freezable, whatever we call that.

@nikomatsakis
Copy link
Contributor

That said, I think we should revisit these categorizations before 1.0, with an eye towards possible future data parallelism. Atomic is quite different from Cell in that, while it permits mutation and hence is not freezable, it's also threadsafe.

@brson
Copy link
Contributor

brson commented Jan 18, 2014

Perhaps they should also be #[non_freeze] then as well. There are probably a lot of synchronization types that need to be reexamined in this light.

@brson
Copy link
Contributor

brson commented Jan 18, 2014

Yeah, it seems pretty clear that we will eventually need some notion of a type that is threadsafe.

@pnkfelix
Copy link
Member

Accepted for 1.0, P-backcompat-libs.

@brson
Copy link
Contributor

brson commented Feb 17, 2014

I've started looking into changing atomics to use immutable references. There's one snag though in that drop creates aliased &mut pointers, as in

#[unsafe_destructor]
impl<T> Drop for AtomicOption<T> {
    fn drop(&mut self) {
        let _ = self.take(SeqCst);
    }
}

@alexcrichton
Copy link
Member

One use-case I realized the other day is that

static COUNT: AtomicUint = INIT_ATOMIC_UINT;

is placed into read-only data, so any usage of the atomic will result in a segfault :(

@nikomatsakis
Copy link
Contributor

@alexcrichton good point. It seems like we may want a rule saying that static values must be freezable.

cc @flaper87 and issue #10577

@flaper87
Copy link
Contributor

@nikomatsakis @alexcrichton just noticed this (I've to improve my email filters a bit).

Makes sense to me, I'll update the pull-request.

brson added a commit to brson/rust that referenced this issue Feb 28, 2014
In Rust, the strongest guarantee that `&mut` provides is that the memory
pointed to is *not aliased*, whereas `&`'s guarantees are much weaker:
that the value can be aliased, and may be mutated under proper precautions
(interior mutability).

Our atomics though use `&mut` for mutation even while creating multiple
aliases, so this changes them to use 'interior mutability', mutating
through immutable references.
alexcrichton pushed a commit to alexcrichton/rust that referenced this issue Mar 20, 2014
In Rust, the strongest guarantee that `&mut` provides is that the memory
pointed to is *not aliased*, whereas `&`'s guarantees are much weaker:
that the value can be aliased, and may be mutated under proper precautions
(interior mutability).

Our atomics though use `&mut` for mutation even while creating multiple
aliases, so this changes them to use 'interior mutability', mutating
through immutable references.
@bors bors closed this as completed in 069cede Mar 22, 2014
flip1995 pushed a commit to flip1995/rust that referenced this issue Oct 6, 2023
…n_exhaustive`

There are cases where users create a unit variant for the purposes
of tracking the number of variants for an nonexhaustive enum.
We should check if an enum is explicitly marked as nonexhaustive
before reporting `manual_non_exhaustive` in these cases. Fixes rust-lang#11583
flip1995 pushed a commit to flip1995/rust that referenced this issue Oct 6, 2023
…xendoo

Don't lint `manual_non_exhaustive` when enum is `#[non_exhaustive]`

Fixes rust-lang#11583

changelog: Fix [`manual_non_exhaustive`] false positive for unit enum variants when enum is explicitly `non_exhaustive`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants