override: Fix wrong caching of the overrides

There was a problem when the python type, which was stored in override
cache for C++ functions, was destroyed and  the record wasn't removed from the
override cache. Therefor, dangling pointer was stored there. Then when the
memory was reused and new type was allocated at the given address and the
method with the same name (as previously stored in the cache) was actually
overridden in python, it would wrongly find it in the override cache for C++
functions and therefor override from python wouldn't be called.
The fix is to erase the type from the override cache when the type is destroyed.

Author: Trigve

include/pybind11/pybind11.h

@@ -2093,6 +2093,16 @@ inline std::pair<decltype(internals::registered_types_py)::iterator, bool> all_t
         // gets destroyed:
         weakref((PyObject *) type, cpp_function([type](handle wr) {
             get_internals().registered_types_py.erase(type);
+
+            // Actually just `std::erase_if`, but that's only available in C++20
+            auto &cache = get_internals().inactive_override_cache;
+            for (auto it = cache.begin(), last = cache.end(); it != last; ) {
+                if (it->first == reinterpret_cast<PyObject *>(type))
+                    it = cache.erase(it);
+                else
+                    ++it;
+            }
+
             wr.dec_ref();
         })).release();
     }

tests/test_class_sh_inheritance.cpp

@@ -49,6 +49,24 @@ struct drvd2 : base1, base2 {
     int id() const override { return 3 * base1::base_id + 4 * base2::base_id; }
 };
 
+class TestDerived {
+
+public:
+    virtual int func() { return 0; }
+
+    TestDerived() = default;
+    ~TestDerived() = default;
+    // Non-copyable
+    TestDerived &operator=(TestDerived const &Right) = delete;
+    TestDerived(TestDerived const &Copy) = delete;
+};
+
+class PyTestDerived : public TestDerived {
+    virtual int func() override { PYBIND11_OVERRIDE(int, TestDerived, func); }
+};
+
+inline int test_override_cache(std::shared_ptr < TestDerived> instance) { return instance->func(); }
+
 // clang-format off
 inline drvd2 *rtrn_mptr_drvd2()          { return new drvd2; }
 inline base1 *rtrn_mptr_drvd2_up_cast1() { return new drvd2; }
@@ -69,6 +87,8 @@ PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_inheritance::base1)
 PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_inheritance::base2)
 PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_inheritance::drvd2)
 
+PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_inheritance::TestDerived)
+
 namespace pybind11_tests {
 namespace class_sh_inheritance {
 
@@ -99,6 +119,12 @@ TEST_SUBMODULE(class_sh_inheritance, m) {
     m.def("pass_cptr_base1", pass_cptr_base1);
     m.def("pass_cptr_base2", pass_cptr_base2);
     m.def("pass_cptr_drvd2", pass_cptr_drvd2);
+
+    py::classh<TestDerived, PyTestDerived>(m, "TestDerived")
+        .def(py::init_alias<>())
+        .def("func", &TestDerived::func);
+
+    m.def("test_override_cache", test_override_cache);
 }
 
 } // namespace class_sh_inheritance

tests/test_class_sh_inheritance.py

@@ -61,3 +61,22 @@ def __init__(self):
     assert i1 == 110 + 21
     i2 = m.pass_cptr_base2(d)
     assert i2 == 120 + 22
+
+
+def test_python_override():
+    def func():
+        class Test(m.TestDerived):
+            def func(self):
+                return 42
+
+        return Test()
+
+    def func2():
+        class Test(m.TestDerived):
+            pass
+
+        return Test()
+
+    for i in range(1500):
+        assert m.test_override_cache(func()) == 42
+        assert m.test_override_cache(func2()) == 0

tests/test_embed/CMakeLists.txt

@@ -25,7 +25,7 @@ pybind11_enable_warnings(test_embed)
 target_link_libraries(test_embed PRIVATE pybind11::embed Catch2::Catch2 Threads::Threads)
 
 if(NOT CMAKE_CURRENT_SOURCE_DIR STREQUAL CMAKE_CURRENT_BINARY_DIR)
-  file(COPY test_interpreter.py DESTINATION "${CMAKE_CURRENT_BINARY_DIR}")
+  file(COPY test_interpreter.py test_derived.py DESTINATION "${CMAKE_CURRENT_BINARY_DIR}")
 endif()
 
 add_custom_target(

tests/test_embed/test_derived.py

@@ -0,0 +1,19 @@
+# -*- coding: utf-8 -*-
+import sys
+
+import derived_module
+
+
+def func():
+    class Test(derived_module.TestDerived):
+        def func(self):
+            return 42
+
+    return Test()
+
+
+def func2():
+    class Test(derived_module.TestDerived):
+        pass
+
+    return Test()

tests/test_embed/test_interpreter.cpp

@@ -1,4 +1,5 @@
 #include <pybind11/embed.h>
+#include <pybind11/smart_holder.h>
 
 #ifdef _MSC_VER
 // Silence MSVC C++17 deprecation warning from Catch regarding std::uncaught_exceptions (up to catch
@@ -37,6 +38,24 @@ class PyWidget final : public Widget {
     std::string argv0() const override { PYBIND11_OVERRIDE_PURE(std::string, Widget, argv0); }
 };
 
+class TestDerived {
+
+public:
+    virtual int func() { return 0; }
+
+    TestDerived() = default;
+    virtual ~TestDerived() = default;
+    // Non-copyable
+    TestDerived &operator=(TestDerived const &Right) = delete;
+    TestDerived(TestDerived const &Copy) = delete;
+};
+
+class PyTestDerived : public TestDerived {
+    virtual int func() override { PYBIND11_OVERRIDE(int, TestDerived, func); }
+};
+
+PYBIND11_SMART_HOLDER_TYPE_CASTERS(TestDerived)
+
 PYBIND11_EMBEDDED_MODULE(widget_module, m) {
     py::class_<Widget, PyWidget>(m, "Widget")
         .def(py::init<std::string>())
@@ -45,6 +64,12 @@ PYBIND11_EMBEDDED_MODULE(widget_module, m) {
     m.def("add", [](int i, int j) { return i + j; });
 }
 
+PYBIND11_EMBEDDED_MODULE(derived_module, m) {
+    py::classh<TestDerived, PyTestDerived>(m, "TestDerived")
+        .def(py::init_alias<>())
+        .def("func", &TestDerived::func);
+}
+
 PYBIND11_EMBEDDED_MODULE(throw_exception, ) {
     throw std::runtime_error("C++ Error");
 }
@@ -73,6 +98,30 @@ TEST_CASE("Pass classes and data between modules defined in C++ and Python") {
     REQUIRE(cpp_widget.the_answer() == 42);
 }
 
+TEST_CASE("Override cache") {
+    auto module_ = py::module_::import("test_derived");
+    REQUIRE(py::hasattr(module_, "func"));
+    REQUIRE(py::hasattr(module_, "func2"));
+
+    auto locals = py::dict(**module_.attr("__dict__"));
+
+    int i = 0;
+    for (; i < 1500; ++i) {
+        std::shared_ptr<TestDerived> p_obj;
+        std::shared_ptr<TestDerived> p_obj2;
+
+        p_obj = pybind11::cast<std::shared_ptr<TestDerived>>(locals["func"]());
+
+        int ret = p_obj->func();
+
+        REQUIRE(ret == 42);
+
+        p_obj2 = pybind11::cast<std::shared_ptr<TestDerived>>(locals["func2"]());
+
+        p_obj2->func();
+    }
+}
+
 TEST_CASE("Import error handling") {
     REQUIRE_NOTHROW(py::module_::import("widget_module"));
     REQUIRE_THROWS_WITH(py::module_::import("throw_exception"),