Mark Id as thread-safe

[ElusiveMori]

(sorry for the repost)

Suggestion:
Change the implementation of Id<T> to hold a marker of type PhantomData<*const T> instead of PhantomData<T>, and to manually implement Send + Sync for Id<T>.

Rationale:
Ultimately, Id<T> semantically behaves just like an index into some storage to retrieve an object of type T. In this regard, passing around an Id<T> should be no different than passing around a usize.

However, because Id<T> uses PhantomData<T>, the Rust compiler thinks that Id<T> actually owns an object of type T, thus imposing the same thread-safety restrictions on Id<T>, as if it actually held T.

I believe, this is semantically not correct, and in this case, Id could be marked as Send + Sync regardless of the type of data it references, in addition to changing the definition inside Id<T> from PhantomData<T> to PhantomData<*const T> to relieve Id<T> from having to obey by the lifetime restrictions of T.

Here is the relevant excerpt from the Rust docs on this practice:

If your struct does not in fact own the data of type T, it is better to use a reference type, like PhantomData<&'a T> (ideally) or PhantomData<*const T> (if no lifetime applies), so as not to indicate ownership.

Use cases:
This is especially useful if you want an ID to data that is stored in some system somewhere that isn't Send or Sync, or has certain lifetime restrictions considerably complicating the API.

I believe it is appropriate to do so, because you cannot acquire an object of type T using just the Id<T>, you also need the relevant IdMap or IdSlab, which would in turn impose the correct lifetime and thread-safety restrictions upon trying to retrieve the actual object.

Ultimately, this would allow other systems (and threads!) to hold IDs to data of the relevant system, without being burdened by the lifetimes and thread-safety restrictions of the original type.

Please, correct me if my reasoning is wrong and if it could potentially introduce UB into the code. Otherwise, this should be a very simple change and should not break any backwards compatibility (I have used rust-doom as an example to verify that).

I can open a PR with the change if you approve of the concept.

[cristicbz]

Thanks a lot for the thorough post! I think this makes sense; the only issue is that I do not want to introduce any unsafe (which this would do for the manual impls for Send and Sync).

We could use &'static T, but that would require the T: 'static bound, which isn't great. What about doing PhantomData<fn(T) -> T>: this makes Id<T>: Send + Sync and invariant in T which I think is a good idea.

[ElusiveMori]

Thanks for the swift reply!

I think PhantomData<fn(T) -> T> may look like a bit of a hack, but I definitely understand trying to avoid unsafe, and I personally can't think of any more alternatives. It would probably be worth leaving a comment about this in the code, too.

I will test this approach in my code (which requires Id<T> to be thread safe and without lifetime bounds), verify that it compiles, and report back.

[ElusiveMori]

Another variant, I think, could be PhantomData<Arc<Mutex<*const T>>>, but it is hardly less hacky than just PhantomData<fn(T) -> T>, although it could be argued that it conveys the intent better:

• Arc + Mutex provide Send + Sync traits
• *const T ignores lifetimes.

[cristicbz]

I don't think fn(T) -> T is that hacky; it's what https://doc.rust-lang.org/nomicon/phantom-data.html recommends for non-lifetime bound invariance.

You could argue fn() -> T is actually how an Id behaves: it's a function that given an IdSlab it returns a T.