Revert "Remove deregistered objects from the inactive overload cache" (#35)

Reverts #32

This change cause segfaults on Mac CI: RobotLocomotion/drake#12105

This was PR'd against this pybind11 fork and merged, then incorporated into a Drake PR as a commit update.
The Drake PR was then reverted in #12105, but the commit was not reverted in pybind11 itself, hence this PR.

Author: BetsyMcPhail

include/pybind11/detail/class.h

@@ -225,19 +225,6 @@ inline bool deregister_instance_impl(void *ptr, instance *self) {
     for (auto it = range.first; it != range.second; ++it) {
         if (self == it->second && Py_TYPE(self) == Py_TYPE(it->second)) {
             registered_instances.erase(it);
-            // TODO(eric.cousineau): This section can be removed if we can use
-            // a different key, as mentioned in `get_type_overload` for
-            // pybind11#1922.
-            PyObject *to_remove = (PyObject *)it->second;
-            auto &cache = detail::get_internals().inactive_overload_cache;
-            for (auto key = cache.begin(); key != cache.end();) {
-               if (key->first == to_remove) {
-                  key = cache.erase(key);
-               }
-               else {
-                  ++key;
-               }
-            }
             return true;
         }
     }

include/pybind11/pybind11.h

@@ -2410,15 +2410,8 @@ inline function get_type_overload(const void *this_ptr, const detail::type_info
     handle self = detail::get_object_handle(this_ptr, this_type);
     if (!self)
         return function();
-    // N.B. This uses `self.ptr()` instead of `type.ptr()` to resolve
-    // pybind11#1922.
-    // TODO(eric.cousineau): Consider using something like the tuple
-    // (cls, f"{cls.__module__}.{cls.__qualname__}", name). However, since
-    // internals::inactive_overload_cache only supports
-    // `(PyObject*, const char*)` (and thus avoids memory management), it may
-    // require some hand-crafting and really bumping the internals version. For
-    // now, leave as-is.
-    auto key = std::make_pair(self.ptr(), name);
+    handle type = self.get_type();
+    auto key = std::make_pair(type.ptr(), name);
 
     /* Cache functions that aren't overloaded in Python to avoid
        many costly Python dictionary lookups below */

tests/test_class.cpp

@@ -367,24 +367,6 @@ 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,5 +1,4 @@
 import pytest
-import weakref
 
 from pybind11_tests import class_ as m
 from pybind11_tests import UserType, ConstructorStats
@@ -290,39 +289,3 @@ 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"