add pyclass eq option

[Icxolu]

In #4206 (comment) we decided that #[pyclass(hash)] should also require
#[pyclass(eq)], so this implements that option.

Special care needs to be taken of simple enums, since they currently always implement equality using their numeric discriminant. This PR keeps that behavior in the absense of the eq option, but emits a deprecation warning in that case. The new behavior does not allow direct comparisons between an enum variant and an integer anymore, are we fine with that? (The enum tests still need adapting)

[Zyell]

I think it should be straightforward to implement the options into this logic. However, I do have a general design question. In my PR I had allowed the old eq and ne logic to be used in conjunction with the newer comparison arms for all other comparisons for simple enums, since equality is required for ordering. With this eq implementation now available, I'm in favor of not allowing the mixing and requiring the eq be implemented if ord is passed (which aligns with ParialOrd requiring ParialEq). It simplifies the logic too. Does that seem reasonable?

[Icxolu]

I think it should be straightforward to implement the options into this logic.

Yes, there should be no problem with integrating the ordering here, I just left it open to either fill it out if #4202 lands first, or for refactoring of it, if we land this one first 👍

I'm in favor of not allowing the mixing and requiring the eq be implemented if ord is passed (which aligns with ParialOrd requiring ParialEq). It simplifies the logic too. Does that seem reasonable?

I agree, it just makes sense to require that (and planned to add a check if #4202 lands first 😇)

[Zyell]

Sounds good! Hopefully we can land this PR first. I feel like we should have completed eq first before jumping to ord, but hindsight always gives this clarity. 😅

[davidhewitt]

Thanks, this implementation looks nice!

Regarding dropping the integer comparison for simple enums, I'm torn. I agree it doesn't really make sense in the context of a PartialEq implementation, but at the same time it does match Python behaviour for simple enums:

>>> from enum import IntEnum
>>> class Color(IntEnum):
...     Red = 0
...     Blue = 1
...     Green = 2
... 
>>> Color.Red == 0
True
>>> Color.Red == 1
False

I wonder, are there design levers we can explore here? For example, whether an enum should have integer comparison might be affected by whether it actually names the discriminants:

#[pyclass]
enum Foo {
    A = 1,
    B = 2,
}

#[pyclass]
enum Bar {
    A,
    B,
}

One could argue it feels natural for Foo to support integer comparison, but not Bar.

Perhaps rather than doing anything too magical, we should just add the integer comparisons back with another flag? e.g. #[pyclass(eq, int_enum)]

🤔

[Icxolu]

Yeah, I thought so. Dropping the integer comparison without any alternative will be a usability regression. I like the idea of using another option to archive that, but I would not call it int_enum, since that kind of implies interop/subclassing from IntEnum which I believe this does not. Maybe eq_int could be used. An idea could also be to require PartialEq<int_repr> and give a way to autogenerate these.

[Icxolu]

I've added the eq_int option for simple enums. This option is currently treated as always on, but emits a deprecation warning if not user specified. It generates a __richcmp__ implementation that is compatible with the current behavior. If eq is also specified, the comparison using PartialEq is tried first and if the extraction fails, it falls back to integer comparison of the enum discriminants.

[davidhewitt]

Overall this is looking good to me and I agree with the choice of eq_int. Needs a changelog entry, and I'll aim to give a final read through tonight; apart from that, I'm excited to see this merge! Thanks 🙌

[Icxolu]

CI failure seems unrelated, not sure whats causing it...

Edit: The only difference I can see is the macos-14-arm64 runner image changed from 20240514.3 -> 20240526.2. All the actions seem to be exactly the same...

[davidhewitt]

Whatever the cause, it seems to have cleared again now without effort on our part 🚀

[davidhewitt]

Looks great to me, thanks! Just a few docs suggestions.

[davidhewitt]

💯