TimelyDataflow / abomonation

A mortifying serialization library for Rust
MIT License
317 stars 30 forks source link

Is it a good idea to implement Abomonation for non-abomonable PhantomData? #29

Open HadrienG2 opened 4 years ago

HadrienG2 commented 4 years ago

So, while resolving the memory safety issue of #28 that you pointed out in #27, I had a pause while reaching the implementation of Abomonation for PhantomData.

Currently, abomonation provides an impl of Abomonation for PhantomData<T> even if T is not abomonable. This is by design, as there is a test checking that this impl is available. And it is certainly technically correct to the first order of approximation: since PhantomData contains no data, it is trivially serializable.

Where I get uneasy, though, is when I consider how PhantomData<T> is typically used. By and large, the main use for this marker type in the wild is in container classes like Box and Vec, where you get types which only hold a *mut T, NonNull<T>, or index into some kind of arena of T, but need to tell rustc that they "logically own" one or more Ts, so that Send, Sync, Drop and other stuff that gets automatically implemented works as expected.

From this perspective, if a type contains a PhantomData<T>, it should almost certainly be regarded as containing a T by abomonation too. In which case we should require that this T be abomonable.

What do you think about this train of thought?