cristicbz / idcontain.rs Goto Github PK

(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.