jcrist / msgspec

A fast serialization and validation library, with builtin support for JSON, MessagePack, YAML, and TOML
https://jcristharif.com/msgspec/
BSD 3-Clause "New" or "Revised" License
2.09k stars 61 forks source link

`__ne__` does not defer to custom `__eq__` #585

Closed Solumin closed 7 months ago

Solumin commented 7 months ago

Description

I've encountered unexpected behavior with custom-defined __eq__ methods:

class A(msgspec.Struct, eq=False):
  x: int
  y: int
  def __eq__(self, other):
    return self.__class__ == other.__class__ and self.x == other.x

x = A(1, 2)
y = A(1, 3)
print(x == y)  # True
print(x != y)  # True  <-- unexpected!

The default implementation of __ne__ just defers to __eq__, so x != y should be False. It seems that Struct has its own __ne__ implementation that does not do this.

Admittedly, this is a really weird case that would probably be solved by making y a private field (#199). I'd also be happy with just updating the docs to state that if you provide a custom __eq__ that you should also define __ne__.

jcrist commented 7 months ago

Thanks for the excellent reproducible issue, this has been fixed in #593.

Admittedly, this is a really weird case that would probably be solved by making y a private field (https://github.com/jcrist/msgspec/issues/199).

As part of that work I also intend to support field level configuration for eq/repr/init, matching the behavior of dataclasses and attrs. When that's done, you should be able to do something like this to get what you want:

class A(msgspec.Struct):
  x: int
  y: int = msgspec.field(eq=False)
Solumin commented 7 months ago

Thank you for fixing this so quickly!

As part of that work I also intend to support field level configuration for eq/repr/init, matching the behavior of dataclasses and attrs.

Excellent, I think that will be a very helpful feature.