[PR #243] [CLOSED] Fix IDs v3 #337

New issue

Closed

opened 2026-02-27 20:24:18 +03:00 by kerem · 0 comments

kerem commented

2026-02-27 20:24:18 +03:00

Owner

📋 Pull Request Information

Original PR: https://github.com/ramsayleung/rspotify/pull/243
Author: @marioortizmanero
Created: 8/25/2021
Status: ❌ Closed

Base: master ← Head: fix-ids-3

📝 Commits (10+)

c2046c7 Add UnknownId and some tests
d5d2812 Proper docs thanks to reworked macro
5a52393 Module-level documentation
1ab8ad1 Now with an example!
3b66b9a Stuck fighting compiler error about Deserialize
5d459f4 First draft with newtype
ea435db The compiler hates me
ce0a860 Mixing Attempt #1 and Attempt #2
bbf0cc2 Almost working now
5b1562f Finally fix Offset impl

📊 Changes

21 files changed (+564 additions, -434 deletions)

View changed files

📝 rspotify-model/src/album.rs (+6 -10)
📝 rspotify-model/src/artist.rs (+4 -7)
📝 rspotify-model/src/audio.rs (+4 -3)
📝 rspotify-model/src/category.rs (+2 -2)
📝 rspotify-model/src/context.rs (+11 -9)
📝 rspotify-model/src/enums/types.rs (+1 -0)
📝 rspotify-model/src/idtypes.rs (+426 -189)
📝 rspotify-model/src/lib.rs (+21 -18)
📝 rspotify-model/src/offset.rs (+4 -4)
📝 rspotify-model/src/playing.rs (+2 -3)
📝 rspotify-model/src/playlist.rs (+4 -8)
📝 rspotify-model/src/recommend.rs (+3 -3)
📝 rspotify-model/src/search.rs (+5 -6)
📝 rspotify-model/src/show.rs (+7 -11)
📝 rspotify-model/src/track.rs (+6 -19)
📝 rspotify-model/src/user.rs (+3 -6)
📝 src/clients/mod.rs (+3 -6)
📝 src/clients/oauth.rs (+12 -10)
📝 src/lib.rs (+1 -0)
📝 tests/test_models.rs (+2 -78)

...and 1 more files

📄 Description

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 a const field in one way or another that links its type to the variant in Type. So attempting to use dyn for runtime usage (Box<dyn Id> or Vec<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 a TrackId or an EpisodeId, 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 a Vec<PlayableId>. In that case, you'd simply pass your tracks with tracks.into_iter().map(|t| t.as_ref()).collect(). Note that we use AsRef for conversions instead of From/Into because it's a cheap conversion; it only requires a pointer cast (that doesn't even use unsafe).

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 all Id<Playable> and Id<Track> are the same type. It's just the generics that change. So this impl block would conflict with impl 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 a struct for each Id which implements the Id trait with all of its functionality. Thus, for each kind of ID we need to:

Declare a struct
Implement all required traits for the struct (Id, AsRef...)
Implement all required conversions into group IDs (e.g. TrackId -> PlayableId)

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:

One macro to just declare the struct and implement the Id trait, and same for IdBuf: define_idtypes!
One macro to implement the conversions of IDs: define_one_way_conversions!
Blanket implementations for the rest of traits (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 because T may be from a foreign crate, just like AsRef. And as we all know that is not allowed by Rust. So in the end we have to make these impl 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:

The use of a large macro, although not that complex
It's impossible to use type inference to create IDs now. Id::from_uri(...) won't work.
Losing the original type of an Id when converting it to something more generic. For example, when converting TrackId into PlayableId, it is impossible to turn it back into TrackId.

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.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/ramsayleung/rspotify/pull/243 **Author:** [@marioortizmanero](https://github.com/marioortizmanero) **Created:** 8/25/2021 **Status:** ❌ Closed **Base:** `master` ← **Head:** `fix-ids-3` --- ### 📝 Commits (10+) - [`c2046c7`](https://github.com/ramsayleung/rspotify/commit/c2046c71a0408660ed6b49ea2a2914264865f388) Add UnknownId and some tests - [`d5d2812`](https://github.com/ramsayleung/rspotify/commit/d5d281249c73227870a4b487317ecbe89b4c9993) Proper docs thanks to reworked macro - [`5a52393`](https://github.com/ramsayleung/rspotify/commit/5a52393c384239a5b525505ccf75dd91e8f08806) Module-level documentation - [`1ab8ad1`](https://github.com/ramsayleung/rspotify/commit/1ab8ad1fcb82464a647728b8ed3b4d4b36b45f43) Now with an example! - [`3b66b9a`](https://github.com/ramsayleung/rspotify/commit/3b66b9a7c8a46c2cdb620ae68d7b88f15c3be6b2) Stuck fighting compiler error about Deserialize - [`5d459f4`](https://github.com/ramsayleung/rspotify/commit/5d459f47b543e354d1fdb0911b7654749db94fc6) First draft with newtype - [`ea435db`](https://github.com/ramsayleung/rspotify/commit/ea435db5e9d13bb1c5497842b3b56c9a9ce719d6) The compiler hates me - [`ce0a860`](https://github.com/ramsayleung/rspotify/commit/ce0a860c0e9292d9c752889bc943bee75fc86dea) Mixing Attempt #1 and Attempt #2 - [`bbf0cc2`](https://github.com/ramsayleung/rspotify/commit/bbf0cc22138230749a6ec1d04bea98155891a16e) Almost working now - [`5b1562f`](https://github.com/ramsayleung/rspotify/commit/5b1562f84f59c67e8c8746265e202c816e861b9a) Finally fix Offset impl ### 📊 Changes **21 files changed** (+564 additions, -434 deletions) <details> <summary>View changed files</summary> 📝 `rspotify-model/src/album.rs` (+6 -10) 📝 `rspotify-model/src/artist.rs` (+4 -7) 📝 `rspotify-model/src/audio.rs` (+4 -3) 📝 `rspotify-model/src/category.rs` (+2 -2) 📝 `rspotify-model/src/context.rs` (+11 -9) 📝 `rspotify-model/src/enums/types.rs` (+1 -0) 📝 `rspotify-model/src/idtypes.rs` (+426 -189) 📝 `rspotify-model/src/lib.rs` (+21 -18) 📝 `rspotify-model/src/offset.rs` (+4 -4) 📝 `rspotify-model/src/playing.rs` (+2 -3) 📝 `rspotify-model/src/playlist.rs` (+4 -8) 📝 `rspotify-model/src/recommend.rs` (+3 -3) 📝 `rspotify-model/src/search.rs` (+5 -6) 📝 `rspotify-model/src/show.rs` (+7 -11) 📝 `rspotify-model/src/track.rs` (+6 -19) 📝 `rspotify-model/src/user.rs` (+3 -6) 📝 `src/clients/mod.rs` (+3 -6) 📝 `src/clients/oauth.rs` (+12 -10) 📝 `src/lib.rs` (+1 -0) 📝 `tests/test_models.rs` (+2 -78) _...and 1 more files_ </details> ### 📄 Description ## 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 a `const` field in one way or another that links its type to the variant in `Type`. So attempting to use `dyn` for runtime usage (`Box<dyn Id>` or `Vec<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 a `TrackId` or an `EpisodeId`, 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 a `Vec<PlayableId>`. In that case, you'd simply pass your tracks with `tracks.into_iter().map(|t| t.as_ref()).collect()`. Note that we use `AsRef` for conversions instead of `From/Into` because it's a cheap conversion; it only requires a pointer cast (that doesn't even use `unsafe`). 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 all `Id<Playable>` and `Id<Track>` are the *same* type. It's just the generics that change. So this `impl` block would conflict with `impl 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 a `struct` for each Id which implements the `Id` trait with all of its functionality. Thus, for each kind of ID we need to: 1. Declare a struct 2. Implement all required traits for the struct (`Id`, `AsRef`...) 3. Implement all required conversions into group IDs (e.g. TrackId -> PlayableId) 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: * One macro to just declare the struct and implement the `Id` trait, and same for `IdBuf`: `define_idtypes!` * One macro to implement the conversions of IDs: `define_one_way_conversions!` * Blanket implementations for the rest of traits (`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` because `T` may be from a foreign crate, just like `AsRef`. And as we all know that is not allowed by Rust. So in the end we have to make these `impl` 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: * The use of a large macro, although not that complex * It's impossible to use type inference to create IDs now. `Id::from_uri(...)` won't work. * Losing the original type of an Id when converting it to something more generic. For example, when converting `TrackId` into `PlayableId`, it is impossible to turn it back into `TrackId`. 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. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>