[smart_holder] Wrong caching of overrides

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 test_derived {
+
+public:
+    virtual int func() { return 0; }
+
+    test_derived() = default;
+    ~test_derived() = default;
+    // Non-copyable
+    test_derived &operator=(test_derived const &Right) = delete;
+    test_derived(test_derived const &Copy) = delete;
+};
+
+class py_test_derived : public test_derived {
+    virtual int func() override { PYBIND11_OVERRIDE(int, test_derived, func); }
+};
+
+inline int test_override_cache(std::shared_ptr < test_derived> 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::test_derived)
+
 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<test_derived, py_test_derived>(m, "test_derived")
+        .def(py::init_alias<>())
+        .def("func", &test_derived::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.test_derived):
+            def func(self):
+                return 42
+
+        return Test()
+
+    def func2():
+        class Test(m.test_derived):
+            pass
+
+        return Test()
+
+    for _ 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,18 @@
+# -*- coding: utf-8 -*-
+
+import derived_module
+
+
+def func():
+    class Test(derived_module.test_derived):
+        def func(self):
+            return 42
+
+    return Test()
+
+
+def func2():
+    class Test(derived_module.test_derived):
+        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 test_derived {
+
+public:
+    virtual int func() { return 0; }
+
+    test_derived() = default;
+    virtual ~test_derived() = default;
+    // Non-copyable
+    test_derived &operator=(test_derived const &Right) = delete;
+    test_derived(test_derived const &Copy) = delete;
+};
+
+class py_test_derived : public test_derived {
+    virtual int func() override { PYBIND11_OVERRIDE(int, test_derived, func); }
+};
+
+PYBIND11_SMART_HOLDER_TYPE_CASTERS(test_derived)
+
 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<test_derived, py_test_derived>(m, "test_derived")
+        .def(py::init_alias<>())
+        .def("func", &test_derived::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<test_derived> p_obj;
+        std::shared_ptr<test_derived> p_obj2;
+
+        p_obj = pybind11::cast<std::shared_ptr<test_derived>>(locals["func"]());
+
+        int ret = p_obj->func();
+
+        REQUIRE(ret == 42);
+
+        p_obj2 = pybind11::cast<std::shared_ptr<test_derived>>(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"),