Overriding __eq__ does not set __hash__ to None

[ixje]

Issue description

The Python documentation states

A class that overrides __eq__() and does not define __hash__() will have its __hash__() implicitly set to None. When the __hash__() method of a class is None, instances of the class will raise an appropriate TypeError when a program attempts to retrieve their hash value, and will also be correctly identified as unhashable when checking isinstance(obj, collections.abc.Hashable).

Thus if we override __eq__ but not __hash__ we should get an unhashable type (e.g. cannot be used in a set container). Pybind however seems to always have a __hash__ implemented, regardless if we override __eq__. The expected behaviour is that __hash__ is set to None

Reproducible example code

In Python it exhibits the following behaviour

from collections import abc

class MyClass1:
    pass

class MyClass2:
    def __eq__(self, other):
        return True

def main():
    c1 = MyClass1()
    c2 = MyClass2()
    print(isinstance(c1, abc.Hashable)) # True 
    print(isinstance(c2, abc.Hashable)) # False <- OK

if __name__ == "__main__":
    main()

Using pybind

#include <pybind11/pybind11.h>
namespace py = pybind11;

class DummyClass {};
class DummyClass2 {};

PYBIND11_MODULE(DummyClass, m) {
    py::class_<DummyClass>(m, "MyClass1")
            .def(py::init());
    py::class_<DummyClass2>(m, "MyClass2")
            .def(py::init())
            .def("__eq__", [](py::object&) { return true; });
}

from collections import abc
from lib.DummyClass import MyClass1, MyClass2

def main():
    c1 = MyClass1()
    c2 = MyClass2()
    print(isinstance(c1, abc.Hashable)) # True 
    print(isinstance(c2, abc.Hashable)) # True <- NOT OK

if __name__ == "__main__":
    main()

The workaround

A workaround is to manually add .attr("__hash__") = py::none()

I'm assuming it should be possible to detect if there is a __eq__ override without any __hash__ override and then set the attribute. This would resolve the deviating/unexpected behaviour.

[bstaletic]

Interesting. Pybind can check that the argument to .def is __eq__ and then set attr("__hash__") to py::none().

[ixje]

Interesting. Pybind can check that the argument to .def is __eq__ and then set attr("__hash__") to py::none().

Note that it should only do that if there is no .def('__hash__' elsewhere for the object. The following should not overwrite __hash__ again to py::none()

.def("__hash__", ..)
.def("__eq__", ...)

[YannickJadoul]

I agree in principle, but it seems quite horrible to do a string comparison with "__eq__" on every call to .def?

[ixje]

Is that really a concern? I mean it is all compile time and how big are bindings really going to be that this will increase will have a noticeable impact?

[YannickJadoul]

Is that really a concern? I mean it is all compile time and how big are bindings really going to be that this will increase will have a noticeable impact?

Is it? Most of the time, it will be, indeed, but in principle, you could dynamically generate strings. So this can't be expected to have no effect on runtime, I think. I'm not against, but it's something to consider.

There are talks about getting benchmarks (both for size and time), so that would make such a discussion a lot easier :-)

[sizmailov]

Can this check & fixup be in py::class_ desctructor?

[YannickJadoul]

Can this check & fixup be in py::class_ desctructor?

Interesting idea, but I don't think there's anything stopping you from keeping the py::class_ stored somewhere?
In a typical case, it will be destructed, most likely, but wouldn't it be a horrible, hard-to-discover/trace bug if just by storing an existing variable somewhere, this behaviour would suddenly change?

[sizmailov]

I agree, it might lead to nasty debug session. I've choose destructor as most closest approximation to "class bindings are complete" event. Would it be better to have dedicated py::class_::done method instead? Probably there other things that needs to be checked/fixed after python class construction completion.

[YannickJadoul]

Would it be better to have dedicated py::class_::done method instead? Probably there other things that needs to be checked/fixed after python class construction completion.

I'm not entirely convinced of that either. Lots of existing project might depend on that not being there, currently?
For now, we've been able to avoid this, btw.

Actually, maybe the check with 7 chars in __eq__\0 could still be OK (if you take into account the amount of lookups and compares Python does on every call, anyway). But adding it to def seems quite ad-hoc in design.

[sizmailov]

I meant explicit call to "finalize" method (if it was not clear from previous comment), smth like

    py::class_<DummyClass2>(m, "MyClass2")
            .def(py::init())
            .def("__eq__", [](py::object&) { return true; })
            .done(); // checks, fix-ups, etc 

I don't think it would break any code (it might, but chances are very low).

[YannickJadoul]

Yes. So once we start counting on that (for maybe more crucial things than __eq__ and __hash__), all the existing code that doesn't call done would be wrong?

[bstaletic]

Here's a potentially bad idea. Instead of strcmp("__eq__", name), compare with ==, as pointers. Yes, this wouldn't be as robust as lexicographic comparison, but string literals are stored in .data section and all pointers to the same string literal are equal.

My idea breaks if the user copies the string literal onto the stack and passes that pointer, or creates a std::string and passes c_str().

 

I don't like the idea of done() as it's easy to forget and all the examples and docs need to be updated.

[sizmailov]

del

No, existing code would be as good as it is right now, not worse nor better.

Such call will be required for some cases, but not every class. The check that required call was not made can go to module destructor (py::class_ instances can be created multiple times for same c++ class)

@bstaletic I agree, for this issue done() seems to be unnecessary complication compared to patching def.

[YannickJadoul]

Or, best of both worlds: if (name == "__eq__" || !std::strcmp(name, "__eq__"))?

[bstaletic]

That might be a good idea. I'd just be explicit regarding that ! and actually compare strcmp to 0.