[BUG]: Regression from 2.5.0 to 2.6.0: Existance of operator breaks binding of some collections

[marlamb]

Required prerequisites

• Make sure you've read the documentation. Your issue may be addressed there.
• Search the issue tracker and Discussions to verify that this hasn't already been reported. +1 or comment there if it has.
• Consider asking first in the Gitter chat room or in a Discussion.

Problem description

In the following example the Struct has a Python binding and is convertible on its own. Also std::unordered_set works in general for build in types. However for the combination std::unordered_set<Struct> the conversion fails at runtime with the error by calling test_func()

Traceback (most recent call last):
  File "<string>", line 1, in <module>
TypeError: Unable to convert function return value to a Python type! The signature was
	() -> Set[test_module.Struct]

• The removal of py::is_operator() in the binding fixes the problem.
• The example does work for version 2.5.0, but fails for 2.6.0, 2.7.1, 2.8.1 and 2.9.0.
• It does also not work for an std::set, but for an std::vector.
• The code was compiled with clang13.0.0 on x86_64 (Ubuntu20.04)

Reproducible example code

#include <pybind11/pybind11.h>
#include <pybind11/stl.h>

namespace py = ::pybind11;

struct Struct
{
  std::string member;
};

bool operator==(const Struct& lhs, const Struct& rhs)
{
  return lhs.member == rhs.member;
}

namespace std
{
template <>
struct hash<Struct>
{
  std::size_t operator()(const Struct& selector) const;
};
}  // namespace std

std::size_t std::hash<Struct>::operator()(const Struct& selector) const
{
  return std::hash<std::string>()(selector.member);
}

PYBIND11_MODULE(test_module, module)
{
  py::class_<Struct>(module, "Struct")
      // Removing the binding makes the example work.
      .def(
          "__eq__",
          [](const Struct& lhs, const Struct& rhs) {
            return lhs == rhs;
          },
          py::is_operator());

  module.def("test_func", []() {
    return std::unordered_set<Struct>{{"one"}, {"two"}};
  });
}

[Skylion007]

The error message could definitely be improved. The real issue is that it should be an error for where the is_operator is specified or not.

When you set __eq__, you also need to set __hash__ (otherwise the hash function will be None). This feature was added in 2.6.0 to prevent bug and unexpected behavior, but it seems like that behavior can be bypassed somehow. #2291

Ping @YannickJadoul and @rwgk since you were contributors on that PR.

Ultimately seems that the issue is that we can return properly descriptive errors due to our type casters:

pybind11/include/pybind11/pytypes.h

Line 1611 in 39fbc79

return PySet_Add(m_ptr, detail::object_or_cast(std::forward<T>(val)).ptr()) == 0;

The real issue that this demonstrates is that the setting of the hash function to None can be bypassed.

[Skylion007]

Okay, yeah this is entirely user error as only binary operators should has is_operator applied. __eq__ can return not implemented, but hash should not be an operator. This is user error. https://docs.python.org/3/library/constants.html#NotImplemented . We may want to guard this with an assert or something as it's really incorrect behavior.

[bluenote10]

But why does pybind11 then allow this

  module.def("test_func", []() {
    return std::vector<Struct>{{"one"}, {"two"}};
  });

or this

  module.def("test_func", []() {
    return Struct{"one"};
  });

Both seem to return instances towards Python just fine. If it is a general user error, should it not fail consistently?

[Skylion007]

unordered_sets get converted to python sets which require the object to be hashable.
unordered_maps get converted to python dicts which requires the keys (and only the keys) to be hashable.
std::vectors get converted to lists which do no have any requirements for hashability of the object.
returning the object Struct itself doesn't require struct to be hashable in Python.

This is a quirk in Python that if you define __eq__ then the object is no longer hashable unless you also define a __hash__ function.

[bluenote10]

I see, thanks for the clarification!

If I understand it correctly a by-product of #1853 would be that the error raised in this case would become

TypeError: unhashable type: 'Struct'

instead of the default Unable to convert function return value to a Python type! .... That would indeed have simplified solving this problem a lot. Alternatively it would be great to include that underlying Python exception into the default error message.

[Skylion007]

@bluenote10 It's now included as a nested exception.