Open steffahn opened 9 months ago
One possible alternative / addition would be to introduce separate AssertSend
and AssertSync
wrappers that are always Send
and Sync
, with the wrapped field being private and construction either requiring those new marker traits (new
), or being unsafe
(new_unchecked
).
Just some thoughts on the problem you're addressing, not on the specific proposal to solve it:
I very much agree that the !Sync/!Send-ness of pointers and UnsafeCell should be 'weaker' than that of other types (like some MutexGuards, etc.).
It's annoying how this does not compile:
fn main() {
let a: *const i32 = &10;
std::thread::scope(|s| {
s.spawn(|| {
println!("{a:?}");
});
})
}
But this does:
fn main() {
let a: *const i32 = &10;
+ let a = a as usize;
std::thread::scope(|s| {
s.spawn(|| {
+ let a = a as *const i32;
println!("{a:?}");
});
})
}
One one hand it's good that this forces you to properly encapsulate the pointer (and the related unsafety) in a type, although in this trivial example (just printing an address), there is no unsafety at all to be encapsulated.
(I suppose that in any real world situation, you'd only use a pointer when doing something unsafe with it, so it's good to force encapsulation. But I've noticed that this is can cause confusion and annoyance when learning or explaining Rust with small snippets.)
This "not Send/Sync but could safely be Send/Sync" property only really applies to four types (*const T
, *mut T
, NonNull<T>
, UnsafeCell<T>
), and I wonder how things would have looked liked if we were to design Rust today. Perhaps they would all be 'thread safe' by default, with a NotThreadSafe<…>
wrapper? Or would we still want their default to be the same as today? Or would we have a language syntax like *const+Sync T
or something? Or would there be another way to override Send/Sync for pointers without using unsafe
?
I've often thought that this problem should be solved at the language level, but this ACP shows that we can also provide a reasonable solution/workaround in the library. Whether this is the best solution we can provide is a good question.
Making pointers Send and Sync today would make existing code unsound, as many structs today rely on a pointer field making them !Send and !Sync. We could make pointers Send and Sync in a new edition, however, if we allow for another way to make your type !Send/!Sync (negative impls or std::marker::NotSync or something). But that would cause any code that is not carefully fixed/updated (including sample snippets from books etc.) to become wildly unsound when copied into a newer edition.
The Iter
example in the ACP is one of many many examples where a pointer (or UnsafeCell) forces us to write a manual (unsafe) Send and Sync implementation which is easy to get wrong, even though it would have worked perfectly fine had pointers been Send+Sync. When working on low level code, I run into these situations daily.
But I'm not sure if this solution makes it much better:
pub struct Iter<'a, T: 'a> { ptr: AssertThreadSafe<NonNull<T>>, end_or_len: AssertThreadSafe<*const T>, _marker: PhantomData<&'a T>, }
Perhaps it's just a naming problem, but this reads to me as if we're a) making an unsafe assumption, b) making an assumption about the Send/Sync-ness of T
itself.
I'm trying to imagine how I'd explain AssertThreadSafe<T>
to someone. (I imagine I'd be using this type in my book on atomics, so I'd have to be able to explain it clearly.) But I don't think I could explain this in a paragraph or two in a way that would just "click" for most people. (It says "assert" but it's not an assert, it says "safe" but it's not removing "unsafe" from anything, but rather adding Send/Sync, etc.)
More importantly, even with this AssertThreadSafe
workaround, this Iter
still requires a PhantomData
to make it sound. If the marker is forgotten, things will be unsound. In a way, the AssertThreadSafe<NonNull<T>>
and AssertThreadSafe<*const T>
types here assert a bit too much and need to be restricted by the PhantomData. :(
That Iter
struct should perhaps use something like a SyncNonNull<T>
that still requires T: Sync
to be Send
; it doesn't need the overpowered "AssertThreadSafe" tool to completely ignore thread safety (just to add it back with the PhantomData).
Which makes me wonder how often one really wants AssertThreadSafe vs something weaker ("safer"?).
I suppose that's the exact same question as whether SyncUnsafeCell<T>
should be Sync
for T: !Sync
:
type | Send | Sync |
---|---|---|
*const T | never | never |
SyncConstPtr\ |
if T: Sync | if T: Sync |
AssertThreadSafe<*const T> | always | always |
UnsafeCell\ |
if T: Send | never |
SyncUnsafeCell\ |
if T: Send | if T: Sync |
AssertThreadSafe<UnsafeCell\ |
if T: Send | always |
You mention that SyncUnsafeCell
is not a full static mut replacement, but that example is about an SyncUnsafeCell<*const ()>
. I don't agree that the problem lies with SyncUnsafeCell
, but rather with *const ()
: SyncUnsafeCell<AssertThreadSafe<*const ()>>
would have worked fine.
More importantly, SyncUnsafeCell<SyncConstPtr<()>>
would also have worked fine, without using the (perhaps overpowered?) AssertThreadSafe
.
I can think of a few use cases where one might use an AssertThreadSafe<UnsafeCell<T>>
that is still Sync
when T: !Sync
, such as a Mutex<T>
implementation.
But.. in those cases, are we really asserting something about the UnsafeCell, or about the T? That is, should we be looking at Something<UnsafeCell<T>>
or UnsafeCell<Something<T>>
?
For the latter, we already have that exact type (unstably): Exclusive.
UnsafeCell<Exclusive<T>>
seems like it could be the right type for the value
field of a Mutex<T>
.
type | Send | Sync |
---|---|---|
*const T | never | never |
SyncConstPtr\ |
if T: Sync | if T: Sync |
SyncConstPtr\<Exclusive\ |
always | always |
UnsafeCell\ |
if T: Send | never |
SyncUnsafeCell\ |
if T: Send | if T: Sync |
SyncUnsafeCell\<Exclusive\ |
if T: Send | always |
This idea basically splits up the functionality of AssertThreadSafe
into two: 1. adding Sync to a pointer/cell, and 2. ignoring !Sync on T, with both steps having their own type, allowing you to pick one, the other, or both.
Would that be better, or just be harder to explain and get right? It feels "safer" to me.
Perhaps it's just a naming problem, but this reads to me as if we're a) making an unsafe assumption, b) making an assumption about the Send/Sync-ness of
T
itself.More importantly, even with this
AssertThreadSafe
workaround, thisIter
still requires aPhantomData
to make it sound. If the marker is forgotten, things will be unsound. In a way, theAssertThreadSafe<NonNull<T>>
andAssertThreadSafe<*const T>
types here assert a bit too much and need to be restricted by the PhantomData. :(
Thanks for the feedback on the naming. I think it’s safe to say that including the word “assert” has its problems, and it now seems like using it for thread-safety traits (Send
/Sync
) can have even worse interpretations than its usage with unwind safety (AssertUnwindSafe
). On the other hand, AssertUnwindSafe
is probably the worse offender in incorrectly conveying it had something to do with “unsafe assumptions”.
More than asserting anything about the contained type, instead “AssertThreadSafe
” asserts that the context the value is used in uses it in a thread-safe manner. It means “in this case of using this type that has to do with raw pointers, I will[^1] not only make sure that all pointer accesses are valid, but also that they are safe (or safely encapsulated) for multi-threaded contexts”. Not that that is something particularly new, because coercions to and from usize
could already me used to send pointers to other threads without typing out the words “unsafe
” at the very place where that conversion or sending happens.
[^1]: or maybe “I assert I will …”
That
Iter
struct should perhaps use something like aSyncNonNull<T>
that still requiresT: Sync
to beSend
Let’s not forget about IterMut
, which wouldn’t be able to profit from a SyncNonNull
that requires T: Sync
for SyncNonNull<T>: Send
.
pub struct IterMut<'a, T: 'a> {
ptr: NonNull<T>,
end_or_len: *mut T,
_marker: PhantomData<&'a mut T>,
}
You mention that
SyncUnsafeCell
is not a full static mut replacement, but that example is about anSyncUnsafeCell<*const ()>
. I don't agree that the problem lies withSyncUnsafeCell
, but rather with*const ()
:SyncUnsafeCell<AssertThreadSafe<*const ()>>
would have worked fine.
Fair point. My example was using *const ()
just as an example for “any non-Sync
type” which is not the best example if followed up by an argument about making types like *const ()
less strictly non-Sync
. The relevant question here was just the question whether or not all instances of static mut
could – for newer editions – be eliminated, e.g. with a cargo fix
kind of approach or by turning static mut
into a syntactic sugar for something using an UnsafeCell
-like type.
I figured it was important to point out (and demonstrate with a code example) that static mut
has no Sync
requirements on the contained value at all.
I don't even know what the exact reasoning was behind the design decision that static mut X: T;
not require T: Sync
, but it might be one of “you need to include manual synchronization anyways”, and depending on the nature of that synchronization, the “correct” bound may be any of T: Sync
, T: Send
, T: Send + Sync
, or no bound at all. Obviously, this design decision should/would/could have been a fairly similar discussion as this one on how to handle SyncUnsafeCell
and “sync” versions of raw pointers.
I can think of a few use cases where one might use an
AssertThreadSafe<UnsafeCell<T>>
that is stillSync
whenT: !Sync
, such as aMutex<T>
implementation.But.. in those cases, are we really asserting something about the UnsafeCell, or about the T? That is, should we be looking at
Something<UnsafeCell<T>>
orUnsafeCell<Something<T>>
?
I think the opposite case is just as important. Many use cases will require more than T: Sync
. The whole point of UnsafeCell
is to enable mutations, so T: Send + Sync
is a very common requirement, too. In fact, maybe the discussion of whether or not AssertThreadSafe<UnsafeCell<T>> ≅ SyncUnsafeCell<T>
should have a bound on T
for Self: Sync
should shift entirely to discussing the candidate T: Send + Sync
for SyncUnsafeCell<T>: Sync
, as in this moment, I can hardly think of a use-case where T: Sync
is (necessary and) sufficient.
Would that be better, or just be harder to explain and get right? It feels "safer" to me.
How useful Exclusive
could be here is an interesting question. I think I’ll look at that a few more times before concluding how I’d feel about that. However, I think you’re also right in questioning whether that’s not just making it harder to get right. I think there’s some value in the consistency that you’ll always need an additional PhantomData
marker to properly convey the access pattern to the T
. And unlike this new SyncConstPtr/Exclusive/…
calculus being introduced, a PhantomData
at least features common familiar types, so there’s no need to think too hardly (or ask Rustdoc) to be sure you have the right bounds after all.
One would teach that raw pointers (and UnsafeCell) are !Sync + !Send
for a good reason, and while one can lift this bound without requiring unsafe
directly, that typically weakens the restrictions beyond the soundness requirements for use-cases, and it should always be accompanied with alternative means of ensuring thread safety: In structs that’s easy, just add PhantomData
. I’m not sure I have a good enough idea of what proper use-cases usually look like that want to use an AssertThreadSafe<unsafe cell / pointer / …>
type directly, like the thread::spawn
one you showed, instead of in a struct field.
I think the requirements between “thread-safe versions of pointers”[^1] and “thread-safe versions of UnsafeCell
”[^1] might be sufficiently different that the two things could be discussed separately, and perhaps only unified if (or as far as) the same (or a compatible) design is reached. I will consider perhaps opening an IRLO thread about “thread-safe versions of pointers” alone.
[^1]: This sounds not quite right, and also relates to the general difficulty of naming these types. Anything that just puts “sync” in its name has the same kind of issue, including SyncUnsafeCell
: A “thread-safe version of pointers” or say a “SyncConstPtr<T>
” or in fact SyncUnsafeCell<T>
might be misunderstood as being something to ordinary raw pointers as Arc<T>
is to Rc<T>
, i.e. the implementation somehow checks thread-safety and makes this pointer safer, even though the opposite is the case, and the “Sync” version of the pointer is less safe to use, and does nothing in terms of additional checks.
It could even be the case that SyncUnsafeCell<T>
(that requires T: Send + Sync
for Self: Sync
) is deemed useful in addition to a version that doesn’t place such a restriction, and then the version that more closely mirrors the approach taken for pointers could possibly get a unified wrapper API type.
For example… if in the end of such a discussion, “thread-safe versions of pointers” doesn’t get any T: Sync
or T: Send
requirements, then AssertThreadSafe<*const T>
and AssertThreadSafe<UnsafeCell<T>>
both do so consistently, and SyncUnsafeCell<T>
could coexist as the alternative that does put bounds on T
.
It's annoying how this does not compile:
fn main() { let a: *const i32 = &10; std::thread::scope(|s| { s.spawn(|| { println!("{a:?}"); }); }) }
But this does:
fn main() { let a: *const i32 = &10; + let a = a as usize; std::thread::scope(|s| { s.spawn(|| { + let a = a as *const i32; println!("{a:?}"); }); }) }
One one hand it's good that this forces you to properly encapsulate the pointer (and the related unsafety) in a type, although in this trivial example (just printing an address), there is no unsafety at all to be encapsulated.
Under provenance rules (which ralf is going to RFC) this might become a dubious justification. Do we have any other way to smuggle pointers between scopes that would remain valid?
Do we have any other way to smuggle pointers between scopes that would remain valid?
Probably conversion to and from AtomicPtr
does the same.
AtomicPtr<T>
is an interesting thing to compare against, anyways, as it does not place any constraint on T
either. Thus any SyncConstPtr<T>
-like type would also be more consistent with the precedent of AtomicPtr
if it didn’t restrict T: Sync
for Self: Sync
.
Here’s the relevant code example
fn main() {
+ use core::sync::atomic::AtomicPtr;
let a: *const i32 = &10;
+ let a = AtomicPtr::new(a.cast_mut());
std::thread::scope(|s| {
s.spawn(|| {
+ let a = a.into_inner() as *const i32;
println!("{a:?}");
});
})
}
A thought to consider before stabilization of an API like this: Given some better trait coherence handling, we could instead write this such that it can't be instantiated at all on certain types (e.g. MutexGuard), rather than such that it can be instantiated but isn't actually Send:
This is not a blocker for the ACP. It's something I'd like added to unresolved questions in the corresponding tracking issue for the unstable feature if this is added. That way, if coherence has improved by the time we go to stabilize this, we can use it, and if it hasn't, we'll probably go ahead with this as-is.
@joshtriplett Can you give some more context to this? This sounds a bit like a thought I’d had, too, that supporting types T
inside of the AssertThreadSafe<T>
that don’t implement the [Send|Sync]IsNotUnsound
traits at all can be undesired. But I’m curious what your thinking is here, too. Also I’m not sure what “coherence handling” you have in mind that helps here. For comparison, these are my thoughts on this so far.
Here’s my reasoning why that could be undesired:
There’s the point that “there’s little use in supporting it”, because the indended use-case of AssertThreadSafe
is to use it always with concrete types like AssertThreadSafe<*const T>
or AssertThreadSafe<UnsafeCell<T>>
, not a generically as AssertThreadSafe<T>
with a generic parameter T
or T: SomeTrait
from somewhere. With concrete types, the AssertThreadSafe
is either useless and does nothing, it you put a type into it that is supported in which case that’s exactly the usage that would still be allowed.
Besides the point that there’s no reason against such a restriction, I could see the downside that something like AssertThreadSafe<OtherType<T>>
, where the “AssertThreadSafe
” does essentially nothing, can be harmful:
User’s structs can include a !Send
or !Sync
type as field and rely on it to mark the whole type non threadsafe, in turn relying on that automatic !Send
and/or !Sync
for soundness. Even though that’s only actually proper when using a type that somehow promises never to implement Send
/Sync
in the future, still there’s no need to create more opportunity for issues.[^1]
[^1]: E.g. assume for now Option<NonNull<T>>
doesn’t implement [Send|Sync]IsNotUnsound
. Then someone for some reason uses AssertThreadSafe<Option<NonNull<T>>
nonetheless, is perhaps confused that it doesn’t work as expected in lifting any bounds, but ends up not removing the useless AssertThreadSafe
wrapper anyways, and then modifies their type further that it turns out it shouldn’t ever be considered thread-safe to begin with, but the type is only !Sync
because of the AssertThreadSafe<Option<NonNull<T>>
.
Finally, at some point in the future `Option<NonNull<T>>` gets the `SyncIsNotUnsound` implementation after all, and that breaks the user’s type’s soundness. Not the most realistic scenario… sure… but maybe it’s even enough of an argument to say we don’t want the confusing “try `AssertThreadSafe<Option<NonNull<T>>` and be confused why it didn’t change anything” experience to be able to happen in the first place.
And now how it could be achieved: I don’t seem to be able to spot any blockers or language features that could help make this any more easily achieved, but I’m curious to learn more. I though, it could be a simple trait bound on the struct.
trait CanAssertThreadSafe
for all types that can be put into AssertThreadSafe
. (So it’s struct AssertThreadSafe<T: CanAssertThreadSafe>(pub T);
then.)SendIsNotUnsound
& SyncIsNotUnsound
like before. Maybe these could be renamed to something relating to AssertThreadSafe
, too. Yeah, I still have no good naming ideas here.[Send|Sync]IsNotUnsound: CanAssertThreadSafe
to make sure a sensible CanAssertThreadSafe
implementation isn’t forgotten.The third trait is sometimes more generally implemented and thus necessary. E.g. UnsafeCell<T>: Send
absolutely needs T: Send
for soundness. However we don’t want to limit a struct using AssertUnwindSafe<UnsafeCell<T>>
to only T: Send
types. So one would have
impl<T> CanAssertThreadSafe for UnsafeCell<T> {}
fully general, but
impl<T: Send> SendIsNotUnsound for UnsafeCell<T> {}
with the T: Send
restriction.
Also I’m right now realizing that I’m missing a lot of ?Sized
bounds everywhere in my code examples. Shouldn’t forget that in any actual implementation either :-)
How about naming the types UncheckedSend
and UncheckedSync
? This would, in my opinion, very clearly communicate the intent of what the types should do.
As for the API:
Send
and Sync
, respectively. This makes the common case of static
s convenient. As for the case of struct fields, users can use PhantomData
to make the Send
and Sync
conditional on a generic type, since a PhantomData
is probably needed anyway.new()
function would be unsafe
. All other operations would be safe. This avoids littering the codebase with unsafe
everywhere, making usage more convenient.Deref
and DerefMut
. This minimizes the burden of having to navigate yet another wrapper type. This is sound because new()
is unsafe.
Proposal
Problem statement
This proposes a generalization of the existing unstable API
SyncUnsafeCell
. TheSyncUnsafeCell
type – as far as I understand from the PR that adds it – serves (at least) two purposes:static mut
Send
orSync
implementations could be skipped.The motivation for skipping explicit
Send
orSync
implementations isn’t fully explained. However my take on this is that it allows to reduce verbosity, and maybe even to increase accuracy, lowering the chance to mess up manualSend
orSync
implementations: For instance, if you are implementing a synchronization primitive that closely mirrors another existing one in terms of its API – say, a customMutex
re-implementation – then you could simply useSyncUnsafeCell<T>
, together with aPhantomData<Mutex<T>>
and then be confident that you got the rightSend
andSync
implementation to inherit fromstd
’sMutex
. (This example also doesn’t fully work withSyncUnsafeCell<T>
because unlike aMutex
,SyncUnsafeCell<T>: Sync
requiresT: Sync
, something that makes it not a fullstatic mut
replacement, anyways, but that’s a discussion separate from this proposal which can support either of keeping or dropping thisT: Sync
restriction.)My observation now is: There is another type in the standard library that comes with a conservative, but non necessary for soundness, implementation of
!Send
and!Sync
: Raw pointers. And there, too, it’s very common one has to re-writeSend
andSync
implementations manually, even when the type very closely resembles another existing one.Just to give one example: The type
std::slice::Iter
which even contains a
PhantomData
marker already, where it could simply inheritSend
andSync
implementations from&'a T
, needs to verbosely (and with a chance of typo / mistake), feature some manualunsafe impl Send
andunsafe impl Sync
implementations mirroring the ones from&'a T
.There likely countless other use cases of pointers used in ways where the pointer is “almost like a reference” in most ways including thread safety, and coming with an accompanying
PhantomData
(taking care of the lifetime). For example if one uses a pointer only because the data is not aligned properly, or to opt out of strict aliasing requirements as inaliasable::boxed::AliasableBox
.Instead of offering a full copy of
UnsafeCell
’s API calledSyncUnsafeCell
without the conservative!Sync
restriction, and then addressing the above analogous problems for pointers, too, by mirroring their full API in hypothetical things likeSyncConstPtr<T>
,SyncMutPtr<T>
,SyncNonNull<T>
, we can introduce a single unified way (API) for lifting instance of types with a “lint-like!Sync
and!Send
implementation”, quite similar to the “lint-like” nature of!UnwindSafe
being lifted byAssertUnwindSafe
, I’m using the nameAssertThreadSafe
here.Motivating examples or use cases
To take the above example of
slice::Iter
, that could then look likeand needs no additional
Send
orSync
implementation anymore.Existing
SyncUnsafeCell<T>
is replaced byAssertThreadSafe<UnsafeCell<T>>
.Solution sketch
A minimal / starting point of such an API looks as follows
This particular approach also gives some
cross-crate traits with a default impl, like `Send`, should not be specialized
warnings, so I’m not sure whether or not this needs some additional (!Send
+!Sync
) marker field to reduce theSend
andSync
implementations to just the listed ones.Alternatively, there’s the option to make this (eventually) extensible, usable even for other crates that may feature some conservatively-
!Sync
types, too. The trait names are placeholders, but the idea would look like this:One could also support additional types from
std
. For exampleOption<NonNull<T>>
comes to mind, becauseAssertThreadSafe<Option<NonNull<T>>>
would be easier to work with perhaps thanOption<AssertThreadSafe<NonNull<T>>>
.To that end, this kind if
impl
could even be further generalized, at least in principle.though it’s hard to say where to stop, and also this all can be happening subsequently, anyways.
Beyond the above
#[derive(Debug, Copy, Clone)]
,AssertThreadSafe
could implement other traits, too. In particularDefault
. Furthermore,Deref
/DerefMut
(pointing to the wrapped type, similar to howManuallyDrop
does it) would also be an option. The field ofAssertThreadSafe
could also be private instead of public, and/or named.Alternatives
Removing or keeping
SyncUnsafeCell
as-is is the alternatives regardingUnsafeCell
. For pointer types: As already mentioned in the “Problem Statement” section, one could also offer non-!Sync
/!Send
pointer types via a dedicated API, such as individual typesSyncConstPtr<T>
,SyncMutPtr<T>
,SyncNonNull<T>
, or a wrapper only for the pointer types. But if those would be introduced in the future, we definitely need to compare the option of multiple separate types, or a smaller-scope wrapper type, to the unified API from this ACP.Or we can decide non-
!Sync
/!Send
pointer types will never be wanted enough, suggest relying on the crate ecosystem for anyone that may find use in it, in which case, forUnsafeCell
alone, a generalized wrapper like this may be overkill.Links and related work
Existing discussions on
SyncUnsafeCell
should be relevant, I don’t have a full overview of the most important ones at hand; I can update this section if anything relevant/related gets suggested.I’ve read https://github.com/rust-lang/rust/issues/53639#issuecomment-634936393 and the answer https://github.com/rust-lang/rust/issues/53639#issuecomment-635009243 where a hypothetical wrapper type came up being called
UnsafeSync<UnsafeCell<T>>
, and my proposal is partially inspired by that, even though I’m not sure what API (and most likely something different from this) was envisioned for “UnsafeSync
” in that comment. Also the observation in the answer thatdoesn’t apply here, because
AssertThreadSafe<UnsafeCell<T>>
can support requiringT: Sync
forSelf: Sync
, just as it does (and has to do to stay sound) forSend
. As mentioned above, I’m questioning whether thatT: Sync
is even wanted, but it works either way. (Just not both ways, in case we wanted both versions for some reason.[^1]) (Here the idea to distinguishSyncUnsafeCell
from a differentAlwaysSyncCell
is mentioned, for instance.)[^1]: Or maybe it can even work both ways, dropping the
T: Sync
requirement only onceAssertThreadSafe<AssertThreadSafe<UnsafeCell<T>>>
is used to really emphasize all thread-safety that can safely be dropped will be dropped.What happens now?
This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.
Possible responses
The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):
Second, if there's a concrete solution: