enumflags2 icon indicating copy to clipboard operation
enumflags2 copied to clipboard

BitFlags should implement PartialEq with #[derive] to allow for matching on constants of BitFlags type

Open pamburus opened this issue 1 year ago • 12 comments

Constants of type BitFlags cannot be used as a pattern in match expressions because it has manually implemented PartialEq trait. To be able to use constants of a type in match expressions, the type must derive PartialEq trait.

Here is the exact error message:

to use a constant of type `BitFlags<MyFlag, u16>` in a pattern, `BitFlags<MyFlag, u16>` must be annotated with `#[derive(PartialEq)]`
the traits must be derived, manual `impl`s are not sufficient
see https://doc.rust-lang.org/stable/std/marker/trait.StructuralPartialEq.html for details

But it looks like there is no need in manual implementation of PartialEq trait. It anyway just compares the inner values

impl<T, N: PartialEq> PartialEq for BitFlags<T, N> {
    #[inline(always)]
    fn eq(&self, other: &Self) -> bool {
        self.val == other.val
    }
}

Please consider deriving PartialEq.

Thank you.

pamburus avatar Jun 08 '24 10:06 pamburus

I don't remember all the design constraints at this point – I'd have to go digging in the git history a bit more.

One issue is that the current implementation doesn't require T: PartialEq, and #[derive(PartialEq)] will add that useless bound – and I don't see how to avoid that.

This creates compatibility issues – examples currently in the test suite will stop compiling.

meithecatte avatar Jun 08 '24 10:06 meithecatte

How can it perform self.val == other.val without requiring T: PartialEq?

pamburus avatar Jun 08 '24 10:06 pamburus

It has N: PartialEq, but not T: PartialEq.

meithecatte avatar Jun 08 '24 10:06 meithecatte

Ah.. So, the problem can go out of marker: PhantomData<T>?

pamburus avatar Jun 08 '24 10:06 pamburus

Hmm, however according to this https://doc.rust-lang.org/src/core/marker.rs.html#749 it should not.

pamburus avatar Jun 08 '24 10:06 pamburus

See https://github.com/rust-lang/rust/issues/26925

meithecatte avatar Jun 08 '24 10:06 meithecatte

Oh, that is a pretty serious limitation, and given the low priority, it looks like it's unlikely to ever be fixed.

pamburus avatar Jun 08 '24 11:06 pamburus

But is it so bad to require PartialEq for T?

pamburus avatar Jun 08 '24 11:06 pamburus

Well, it would require bumping the version to 0.8.0

meithecatte avatar Jun 08 '24 11:06 meithecatte

Actually, I am not sure this will help. Probably PhantomData should also derive PartialEq, but it doesn't. It looks like an experiment should be done first.

pamburus avatar Jun 08 '24 12:06 pamburus

Yes, I'd suggest you clone the repo and try getting it to work in your project with enumflags2 = { path = "..." }

meithecatte avatar Jun 08 '24 12:06 meithecatte

Tried. Yes, it requires T to implement PartialEq, as expected due to the issue you mentioned earlier. But everything else works fine. #[derive(PartialEq)] works without issues. And I was able to use constants as patterns in match expressions with this fix.

pamburus avatar Jun 08 '24 12:06 pamburus