Closed Icxolu closed 1 month ago
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?
@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 enumseq_int
only- provides only the original integer comparison behaviorPlease correct me if I mistook the structure.
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)]
... 🤔
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.
@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?
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 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
noreq_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 requireeq
foreq_int
, but it's not technically necessary.
Fully agree with this 👍
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.
This fixes a regression that I accidentally introduced in #4210. @Zyell noticed it in https://github.com/PyO3/pyo3/pull/4202#issuecomment-2142991466 🙏
The match arms for the
eq_int
comparison would not be generated as intended ifeq
was not also specified. This should now generate the correct arms.