More strict data type for member roles

[taufik-rama]

Hello 👋

I've been debugging an issue, it seems to be related to how roles are handled.

$ cargo run
bot is connected!

# ...

Caused by:
    Invalid Form Body (roles.1: The set already contains this value)

# ...

Based on the message, I'd assume that there's at least a requirement that when we add a role to a server member, the list of roles shouldn't be duplicated. If this is the case, we could change the roles field data type into a HashSet

/// Information about a member of a guild.
#[derive(Clone, Debug, Deserialize, Serialize)]
#[non_exhaustive]
pub struct Member {
    /// ...

    /// Vector of Ids of [`Role`]s given to the member.
    pub roles: Vec<RoleId>, // Change into `HashSet<RoleId>`
    
    /// ...

This ensures that the roles can never be duplicated. Also to note that this would be an API break since it's a public item

I've taken a peek at other (internal implementation) location, and I think changing the type shouldn't be very difficult, since the operation on Vec should be able to applied also to HashSet

As a context: the way I handle the roles assignment also uses HashSet, and currently I just convert it to Vec specifically for this field

[kangalio]

There are a lot of Vec<Id>. HashSet has higher overhead (2x stack size, 3x slower in ad-hoc benchmark), which makes me reluctant to change all those to HashSet. Do you think the higher overhead is worth it? Or is it ok to be inconsistent and change just Member::roles to HashSet?

[taufik-rama]

Ah, I didn't realize there are more fields with the same pattern

To be clear, the root cause for what I'm trying to raise is that a duplicated values will result in an error when the Discord's API gets called. If the API can tolerate duplicates, it might be fine to just keep it with Vecs.

I obviously don't have the data on how people uses the library, but for my usecase (web devs) generally the performance hit is negligible compared to time spent waiting on I/O. Not to mention that this fields usually only gets processed once in a while. If I can raise a point, I'd say that Vec is fast, rather than HashSet being the one that is slow :p

tl;dr: it might be more proper to change the fields that better correspond to what Discord APIs expects

Having said that, I understand if this changes might not be considered, since it will break existing Serenity APIs since it's a publicly accessible fields

[kangalio]

Any others' opinions? Use HashSet for all sets of non-duplicating IDs, which prevents duplicates before reaching Discord's servers but increases struct sizes?

[taufik-rama]

Just that from me 👍