Endpoints with lists of Ids don't accept multiple of them

[marioortizmanero]

Describe the bug

I just realized that Ids don't really work in lists. We'd have to use Box, I think, to support for example items of both episodes and tracks. Otherwise, with a snippet like this:

use rspotify::client::SpotifyBuilder;
use rspotify::model::{EpisodeId, TrackId};
use rspotify::oauth2::CredentialsBuilder;

#[tokio::main]
async fn main() {
    env_logger::init();

    let creds = CredentialsBuilder::from_env().build().unwrap();
    let mut spotify = SpotifyBuilder::default()
        .credentials(creds)
        .build()
        .unwrap();

    spotify.request_client_token().await.unwrap();

    let uris = &[
        EpisodeId::from_uri("spotify:episode:7zC6yTmY67f32WOShs0qbb").unwrap(),
        EpisodeId::from_uri("spotify:episode:3hCrwz19QlXY0myYh0XgmL").unwrap(),
        TrackId::from_uri("spotify:track:0hAMkY2kwdXPPDfQ1e3BmJ").unwrap()
    ];
    let resp = spotify.start_uris_playback(uris, None, None, None).await;

    println!("Response: {:#?}", resp);
}

You get:

error[E0308]: mismatched types
  --> examples/broken.rs:20:9
   |
20 |         TrackId::from_uri("spotify:track:0hAMkY2kwdXPPDfQ1e3BmJ").unwrap()
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected enum `rspotify::model::idtypes::Episode`, found enum `rspotify::model::idtypes::
Track`
   |
   = note: expected reference `&Id<rspotify::model::idtypes::Episode>`
              found reference `&Id<rspotify::model::idtypes::Track>`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0308`.
error: could not compile `rspotify`

To learn more, run the command again with --verbose.

This is also relatively dangerous because if the user tries to initialize the Ids with Id::from_uri instead of the specific EpisodeId:.from_uri or TrackId::from_uri, they will all be converted to the type of the first one in the list and fail at the unwrap.

[marioortizmanero]

The design for IDs is a bit wrong now that I've tried to actually use it... The thing is that you don't always know at compile-time what kind of ID is being used. So for example, this is impossible to do right now:

let id = match x {
    Track(t) => TrackId::from_uri(&t.uri).unwrap(),
    Episode(t) => EpisodeId::from_uri(&t.uri).unwrap(),
};

client.start_uris_playback(id, /* ... */).await.unwrap();

Because the arms of the match statement have different types.

Any opinions, @kstep ? Sorry for bringing this up so late :(

[marioortizmanero]

Here's my reasoning behind all of this, step by step so that anyone else can understand it as well. If I'm wrong at any point please let me know!

So we currently have an Id struct that is generic over its type, Id<T>. And for functions like start_uris_playback, we need T to take different values in a Vec/equivalent (as a simplification over the impl IntoIterator<...> thing). We also want to be able to have T be checked at runtime.

• The first idea that comes to my mind is dynamic dispatching; Id<dyn T> might work.

  • In order for Ìd<dyn T> to work, where T: IdType:
    1. IdType must be object safe, so that dyn IdType is possible
    2. Id should be defined with pub struct Id<T: IdType + ?Sized>, so that it may take a dyn IdType directly (just like how Box works). Otherwise we'd have to use Id<Box<dyn IdType>>, which wouldn't work either because Box<dyn IdType> doesn't implement IdType.
  • Problems:
    1. Those requirements are basically what a Box<T> is. But Box<T> performs a memory allocation, so Id<T> wouldn't be a zero-cost abstraction at all.
    2. The trait IdType simply contains const TYPE: Type; in its definition. Which means IdType is not object-safe at all. We would have to make IdType a trait that initializes T and then calls get_type(&self) -> Type, which would be type-safe.
  • In summary, I think we're walking in circles with Id<T>.

• Another solution would be what I've already implemented in Fix IDs #241. Simply add an Unknown variant that doesn't check types at all. You can then have Vec<Id<Unknown>>, which would allow any kind of ID to be passed as its generic parameter. Though this sounds like a cool solution for being able to have IDs defined at runtime (Id<Unknown>), with it you can't enforce the type safety Id<T> is supposedly useful for over a &str.

  While this is my favorite solution so far, I think there must be something else that's better.

• Based on the last solution, we could turn traits like PlayableIdType into types (say Playable) just like Artist or Track. One could then convert Id<Track> into Id<Playable> with id.into() in order to pass it as a parameter. That way no generics are involved at all over T; it's always going to be a known type.

  The main problem of this solution is that there is some data loss as well. For example the Display implementation of Id<T> uses T::TYPE to print its URI. What exactly would happen with Id<Playable>? It would print the URI spotify:playable:xxxx, which is not a valid URI.

• A different approach would be to scratch Id<T> and just use ArtistId and similars directly. What I mean is that we wouldn't have a single type for IDs called Id<T> that implements its logic. We could make ArtistId and similars, and implement the trait the same way clients work. The main component would be a trait that implements all the logic, say Id, and a struct for each type (ArtistId, etc) that implements the trait. With this one could be generic over Id directly, have Box<dyn Id> for runtime usage, and Vec<Box>` for multiple values, which is exactly what we need.

  The main problems are:

  1. We can't use type inference to our benefit; Id::from_uri can't work with this solution; ArtistId::from_uri would have to be used explicitly.
  2. We need the trait Id always available when using IDs; we'd have to add it to the prelude. Considering the user needs to import the prelude to use the clients anyway I don't think it's a big deal

  So this solution is basically the same as how clients work. We have a big trait that does everything, and we let Rust "clone" the code for each of our implementations. It's also basically the newtype pattern, but with a common trait for all the newtypes.

Sorry for the long text lol. Just brainstorming. I'll try to implement my favorite solutions in PRs and showcase how they work.

[marioortizmanero]

Ugh. Unfortunately literally none of these ideas work. This is what happens when trying to implement From to convert any Id<T> into Id<Unknown>:

error[E0119]: conflicting implementations of trait `std::convert::From<&idtypes::Id<idtypes::Unknown>>` for type `&idtypes::Id<idtypes::Unknown>`
   --> rspotify-model/src/idtypes.rs:152:1
    |
152 | impl<T> From<&Id<Unknown>> for &Id<T> {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: conflicting implementation in crate `core`:
            - impl<T> From<T> for T;

For more information about this error, try `rustc --explain E0119`.
error: could not compile `rspotify-model` due to previous error

So in that case the user would have to do:

let id1 = ArtistId::from_uri("...").unwrap();
let id2 = UnknownId::from_id(id1.id()).unwrap();

Which is way uglier and has more overhead than:

let id1 = ArtistId::from_uri("...").unwrap();
let id2: UnknownId = id1.into();

[ramsayleung]

It turns out that the zero cost abstractions isn't that easy to reach out, it comes with unexpected obstacles :)

[marioortizmanero]

Yeah, it's very hard to mix both compile time and runtime. I'm trying a new approach that consists of mixing two of the ideas I explained here. I'll try to have multiple types for Id instead of a single Id<T> so that there's no generics instead, AND I'll add UnknownId/similars as IDs as well. The difference is that since the Id types are different, it is possible to implement conversions from them (instead of using dyn, which is impossible given the Id trait).

I'll explain more if I get it right, though it still has a couple drawbacks.