Merge pull request #37 from BetsyMcPhail/inactive_overload_cache-key

Inactive overload cache key

include/pybind11/detail/internals.h

@@ -83,9 +83,9 @@ template <typename value_type>
 using type_map = std::unordered_map<std::type_index, value_type, type_hash, type_equal_to>;
 
 struct overload_hash {
-    inline size_t operator()(const std::pair<const PyObject *, const char *>& v) const {
+    inline size_t operator()(const std::pair<const PyObject *, std::string>& v) const {
         size_t value = std::hash<const void *>()(v.first);
-        value ^= std::hash<const void *>()(v.second)  + 0x9e3779b9 + (value<<6) + (value>>2);
+        value ^= std::hash<std::string>()(v.second);
         return value;
     }
 };
@@ -97,7 +97,7 @@ struct internals {
     type_map<type_info *> registered_types_cpp; // std::type_index -> pybind11's type information
     std::unordered_map<PyTypeObject *, std::vector<type_info *>> registered_types_py; // PyTypeObject* -> base type_info(s)
     std::unordered_multimap<const void *, instance*> registered_instances; // void * -> instance*
-    std::unordered_set<std::pair<const PyObject *, const char *>, overload_hash> inactive_overload_cache;
+    std::unordered_set<std::pair<const PyObject *, std::string>, overload_hash> inactive_overload_cache;
     type_map<std::vector<bool (*)(PyObject *, void *&)>> direct_conversions;
     std::unordered_map<const PyObject *, std::vector<PyObject *>> patients;
     std::forward_list<void (*) (std::exception_ptr)> registered_exception_translators;

include/pybind11/pybind11.h

@@ -2426,7 +2426,10 @@ inline function get_type_overload(const void *this_ptr, const detail::type_info
     if (!self)
         return function();
     handle type = self.get_type();
-    auto key = std::make_pair(type.ptr(), name);
+    /* N.B. This uses `__qualname__.name` instead of `name`
+       to resolve pybind11#1922. */
+    std::string full_name = type.attr("__qualname__").cast<std::string>() + "." + name;
+    auto key = std::make_pair(type.ptr(), full_name);
 
     /* Cache functions that aren't overloaded in Python to avoid
        many costly Python dictionary lookups below */

tests/test_class.cpp

@@ -367,6 +367,24 @@ TEST_SUBMODULE(class_, m) {
             .def(py::init<>())
             .def("ptr", &Aligned::ptr);
     #endif
+
+    // Test #1922 (drake#11424).
+    class ExampleVirt2 {
+        public:
+            virtual ~ExampleVirt2() {}
+            virtual std::string get_name() const { return "ExampleVirt2"; }
+        };
+    class PyExampleVirt2 : public ExampleVirt2 {
+    public:
+        std::string get_name() const override {
+            PYBIND11_OVERLOAD(std::string, ExampleVirt2, get_name, );
+        }
+    };
+    py::class_<ExampleVirt2, PyExampleVirt2>(m, "ExampleVirt2")
+        .def(py::init())
+        .def("get_name", &ExampleVirt2::get_name);
+    m.def("example_virt2_get_name",
+        [](const ExampleVirt2& obj) { return obj.get_name(); });
 }
 
 template <int N> class BreaksBase { public: virtual ~BreaksBase() = default; };

tests/test_class.py

@@ -1,4 +1,5 @@
 import pytest
+import weakref
 
 from pybind11_tests import class_ as m
 from pybind11_tests import UserType, ConstructorStats
@@ -289,3 +290,39 @@ def test_aligned():
     if hasattr(m, "Aligned"):
         p = m.Aligned().ptr()
         assert p % 1024 == 0
+
+
+@pytest.mark.skip(
+    reason="Generally reproducible in CPython, Python 3, non-debug, on Linux. "
+           "However, hard to pin this down for CI.")
+def test_1922():
+    # Test #1922 (drake#11424).
+    # Define a derived class which *does not* overload the method.
+    # WARNING: The reproduction of this failure may be platform-specific, and
+    # seems to depend on the order of definition and/or the name of the classes
+    # defined. For example, trying to place this and the C++ code in
+    # `test_virtual_functions` makes `assert id_1 == id_2` below fail.
+    class Child1(m.ExampleVirt2):
+        pass
+
+    id_1 = id(Child1)
+    assert m.example_virt2_get_name(m.ExampleVirt2()) == "ExampleVirt2"
+    assert m.example_virt2_get_name(Child1()) == "ExampleVirt2"
+
+    # Now delete everything (and ensure it's deleted).
+    wref = weakref.ref(Child1)
+    del Child1
+    pytest.gc_collect()
+    assert wref() is None
+
+    # Define a derived class which *does* define an overload.
+    class Child2(m.ExampleVirt2):
+        def get_name(self):
+            return "Child2"
+
+    id_2 = id(Child2)
+    assert id_1 == id_2  # This happens in CPython; not sure about PyPy.
+    assert m.example_virt2_get_name(m.ExampleVirt2()) == "ExampleVirt2"
+    # THIS WILL FAIL: This is using the cached `ExampleVirt2.get_name`, rather
+    # than re-inspect the Python dictionary.
+    assert m.example_virt2_get_name(Child2()) == "Child2"