-
Notifications
You must be signed in to change notification settings - Fork 123
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
Fix IDs v3 #243
Fix IDs v3 #243
Conversation
This is WIP because I still have to fix the tests and docs, but it should work perfectly and is otherwise ready for review. I'll try to fix the docs and tests soon though. |
This PR also makes |
Oh wow. Incredible. I guess I'll keep doing research but I honestly can't believe this has to be so difficult. Maybe I'm asking for too much or for something impossible. Sorry for making such a mess of comments/PRs/etc... If only I found out about things like these before finishing the implementation... |
Closing over #244, I finally got it :) |
Description
Closes #203
This final approach consists of mixing two of the ideas I explained in #203.
Explanation
It is fundamentally impossible to have an object-safe
Id
trait because it requires aconst
field in one way or another that links its type to the variant inType
. So attempting to usedyn
for runtime usage (Box<dyn Id>
orVec<Box<dyn Id>>
) has to be discarded. Thus, I've created IDs that may hold multiple types instead of a specific one;AnyId
serves for any kind of Id,PlayableId
can be created with aTrackId
or anEpisodeId
, etc. Note that these are types and not traits. So when a function wants to take different kinds of IDs, it's not done via generics; it's done via ID conversions.Say for example you have a
tracks: Vec<TrackId>
and the function wants aVec<PlayableId>
. In that case, you'd simply pass your tracks withtracks.into_iter().map(|t| t.as_ref()).collect()
. Note that we useAsRef
for conversions instead ofFrom/Into
because it's a cheap conversion; it only requires a pointer cast (that doesn't even useunsafe
).In order to make the previous conversion possible, one has to implement
impl AsRef<Id<Playable>> for Id<Track>
. The problem here is that after allId<Playable>
andId<Track>
are the same type. It's just the generics that change. So thisimpl
block would conflict withimpl AsRef<T> for T
, which already exists in the standard library, and makes it impossible.Given the previous reasoning, we know that each Id must be a different type, discarding
Id<T>
. The only remaining option here is to have astruct
for each Id which implements theId
trait with all of its functionality. Thus, for each kind of ID we need to:Id
,AsRef
...)Which is a lot of boilerplate when we have 2+ kinds of IDs. To make it a bit simpler, we can use both macros and blanket implementations. Ideally, we just need:
Id
trait, and same forIdBuf
:define_idtypes!
define_one_way_conversions!
AsRef
,Deref
...)The only issue here is that the blanket implementations don't work; you can't just
impl<T: Id> AsRef<AnyId> for T
becauseT
may be from a foreign crate, just likeAsRef
. And as we all know that is not allowed by Rust. So in the end we have to make theseimpl
in the first macro instead of with blanket implementations. Which is not really a problem, but large macros are scary.Even though this explanation is quite complex and may be hard to follow, the
idtypes
module isn't really that hard to understand. Here I just tried to explain in detail why this is the only way to do it, and how it works. But the user only needs to know that there are different types to represent IDs, even groups of types, and that they can freely convert from single types into groups of types.Disadvantages
The main disadvantages are:
Id::from_uri(...)
won't work.TrackId
intoPlayableId
, it is impossible to turn it back intoTrackId
.But this is literally the only way I've found to fix IDs after lots of different approaches and time invested. Please, if there's anything unclear just ask, and if you find anything with room improvement let me know.