Cache ignores ReactionRemove when the emoji is animated

[laralove143]

Discord sends None as reaction_remove.emoji.animatedwhich is mapped to false on Twilight, the issue happens when the cache's logic compares the removed reaction's emoji with the cached reaction's emoji, since ReactionType derives Eq, == would compare their animated fields too, which do not match so the cache skips over them and ignores the ReactionRemove event, my solution would be to implement Eq on ReactionType manually and have it compare their Ids

[7596ff]

Can reproduce. Fix is probably making animated an Option.

[laralove143]

I don't think that'd work since this code still returns false

let a = ReactionType::Custom {
    animated: Some(true),
    id: 0,
    name: Some("aa".to_owned())
};
let b = ReactionType::Custom {
    animated: None,
    id: 0,
    name: Some("aa".to_owned())
};

dbg!(a == b);

[vilgotf]

Think the fix is manually implementing PartialEq for ReactionType:

impl PartialEq for ReactionType {
    fn eq(&self, other: &Self) -> bool {
        match (self, other) {
            (ReactionType::Custom { id: me, .. }, ReactionType::Custom { id: other, .. }) => {
                me == other
            }
            (ReactionType::Unicode { name: me }, ReactionType::Unicode { name: other }) => {
                me == other
            }
            (ReactionType::Custom { .. }, ReactionType::Unicode { .. })
            | (ReactionType::Unicode { .. }, ReactionType::Custom { .. }) => false,
        }
    }
}

[vilgotf]

The above could be implemented for all resources with an ID as the ID is unique, e.g.

impl PartialEq for Guild {
    fn eq(&self, other: &Self) -> bool {
        self.id == other.id
    }
}

This would compare RESOURCE equality instead of OBJECT equality. I can see user's assuming PartialEq to do either or. It's however, more useful for PartialEq to compare objects, as resources can be compared by ID, but this is definitely something we should document moving forward.

[laralove143]

This would also probably be more efficient, is there a way to use a macro for this? Something like

struct HasId {
  #[eq]
  id: Id<GenericMarker>,
  other: String,
}

[vilgotf]

Think the fix is manually implementing PartialEq for ReactionType:

impl PartialEq for ReactionType {
    fn eq(&self, other: &Self) -> bool {
        match (self, other) {
            (ReactionType::Custom { id: me, .. }, ReactionType::Custom { id: other, .. }) => {
                me == other
            }
            (ReactionType::Unicode { name: me }, ReactionType::Unicode { name: other }) => {
                me == other
            }
            (ReactionType::Custom { .. }, ReactionType::Unicode { .. })
            | (ReactionType::Unicode { .. }, ReactionType::Custom { .. }) => false,
        }
    }
}

Strike that, being able to compare objects is useful, see https://github.com/twilight-rs/twilight/issues/1810#issuecomment-1172873761. We should instead implement that logic directly into the cache's ReactionRemove .find(|r| ...) logic

[laralove143]

Why is comparing objects useful? All the other libraries I've used compare IDs, would this implementation not make every *Update event unique?

[vilgotf]

Why is comparing objects useful?

It can be useful to know that a resource has changed (see #1811).

would this implementation not make every *Update event unique?

I don't see how this would present a problem, update events are matched on by their ID already. See for example, MessageUpdate.

[laralove143]

So we'd be expecting the users to compare IDs themselves? Fair enough, so the solution would be to compare IDs in the cache as well?

[vilgotf]

exactly