fix incorrect __richcmp__ for eq_int only simple enums

[Icxolu]

This fixes a regression that I accidentally introduced in #4210. @Zyell noticed it in #4202 (comment) 🙏

The match arms for the eq_int comparison would not be generated as intended if eq was not also specified. This should now generate the correct arms.

[davidhewitt]

Hmm interesting case. I sort of wonder if we don't need this and it's ok if adding eq_int will disable the automated value equality?

Justification: users who've added eq_int are already customising the equality of their enum beyone the 0.21 behaviour and while it seems unlikely that users would intentionally want eq_int without eq, if we merge this patch then they might accidentally have implicit eq which goes away in the future when we remove the deprecated behaviour?

[Zyell]

@davidhewitt currently adding eq_int causes invalid equality checks and it deviates from the prior behavoir. Isn't eq_int supposed to maintain the current equality behavior? I am somewhat confused about the use cases. I had thought it was like the following:

• eq only - provides the new equality check behavior (requires PartialEq implementation)
• eq and eq_int - provides the new equality check with fallback to the older integer comparison that was default provided by default on simple enums
• eq_int only- provides only the original integer comparison behavior
• no equality parameter - maintains old behavior with deprecation warning. (this also currently gives incorrect comparisons)

Please correct me if I mistook the structure.

[davidhewitt]

I'll recheck (and I might be remembering wrong) - I thought the 0.21 behaviour matches (functionally, but not in implementation) what we now call #[pyclass(eq, eq_int)]... 🤔

[Icxolu]

Nice list, this is essentially what I tried to build 😅 . No equality is equivalent to eq_int only, so they are both currently broken. I think the question is if we want Enum.VariantA == Enum.VariantA to work even if only eq_int (and not eq) is provided.

I think we probably also don't want to emit a deprecation warning if eq_int is missing, but eq was added. I think I will add that fix here as well.

[Zyell]

@davidhewitt If you check this test from the 0.21 release, it will not work under the same conditions on main.

@Icxolu I agree with the deprecation warning. Are you also thinking eq_int should require eq to be passed then?

[Icxolu]

I was thinking to only emit the deprecation warning if neither eq nor eq_int is present. If one or both of them were added, the user made a choice, which behavior is desired, so there is no need for the warning anymore. We could require eq for eq_int, but it's not technically necessary.

[davidhewitt]

@davidhewitt If you check this test from the 0.21 release, it will not work under the same conditions on main.

Right I see, so we already broke it. Can we perhaps add that test of the 0.21 behaviour back under an #[allow(deprecated)] guard? That would help me be sure that this patch is doing what we want.

I was thinking to only emit the deprecation warning if neither eq nor eq_int is present. If one or both of them were added, the user made a choice, which behavior is desired, so there is no need for the warning anymore. We could require eq for eq_int, but it's not technically necessary.

Fully agree with this 👍

[Icxolu]

Can we perhaps add that test of the 0.21 behaviour back under an #[allow(deprecated)] guard?

That's a good idea. I added a deprecated module with the relevant tests.

[davidhewitt]

Thanks, the new test cases really help! I think we're nearly there...

[davidhewitt]

Does this imply that by having eq_int that normal equality "works" anyway (but via integer conversion presumably)?

If so, maybe it makes sense to have eq_int require eq to make it not a surprise that value equality works? We can always relax that requirement later I guess.

[Icxolu]

Does this imply that by having eq_int that normal equality "works" anyway (but via integer conversion presumably)?

Yes, this is correct.

If so, maybe it makes sense to have eq_int require eq to make it not a surprise that value equality works? We can always relax that requirement later I guess.

Sure, I would love to have value comparisons only work with eq, but I did not see a good way to archive that.

[davidhewitt]

LGTM, thanks! 💯