From 451158a6a6a10b02a94478623e22d6c92e7f3bf1 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 21 Oct 2022 18:12:36 -0700 Subject: [PATCH 1/3] Adding #4117 reproducer to test_virtual_functions.cpp --- tests/test_virtual_functions.cpp | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/tests/test_virtual_functions.cpp b/tests/test_virtual_functions.cpp index 323aa0d22d..4d71669a47 100644 --- a/tests/test_virtual_functions.cpp +++ b/tests/test_virtual_functions.cpp @@ -589,3 +589,32 @@ void initialize_inherited_virtuals(py::module_ &m) { m.def("test_gil", &test_gil); m.def("test_gil_from_thread", &test_gil_from_thread); }; + +namespace issue4117 { // Broken by PR #3861 + +class Animal { +public: + virtual ~Animal() {} + virtual void *go() = 0; +}; + +class PyAnimal : public Animal { +public: + /* Inherit the constructors */ + using Animal::Animal; + /* Trampoline (need one for each virtual function) */ + void *go() override { + PYBIND11_OVERRIDE_PURE(void *, /* Return type */ + Animal, /* Parent class */ + go, /* Name of function in C++ (must match Python name) */ + ); + } +}; + +void bindings(py::module_ m) { + py::class_(m, "Animal") + .def(py::init<>()) + .def("go", &Animal::go); +} + +} // namespace issue4117 From f6ba6a043ad693265a866d90ad27fa6f7d9161b6 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 21 Oct 2022 19:51:39 -0700 Subject: [PATCH 2/3] rollback + two fixes. this commit is just a snapshot to archive the current state --- include/pybind11/cast.h | 34 ++++++++++++++++-- tests/test_virtual_functions.cpp | 60 +++++++++++++++++--------------- tests/test_virtual_functions.py | 9 +++++ 3 files changed, 72 insertions(+), 31 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 5699d4fb00..cac61a9eb1 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1178,16 +1178,46 @@ template enable_if_t::value, T> cast_safe(object &&) { pybind11_fail("Internal error: cast_safe fallback invoked"); } + +#ifdef PR3861_ROLLBACK + +template +enable_if_t::value, T> cast_safe(object &&o) { + return pybind11::cast(std::move(o)); +} + +template <> +inline void cast_safe(object &&) {} + +#elif defined(FIRST_STAB) + +template +enable_if_t>::value && !std::is_pointer::value, void> +cast_safe(object &&) {} + template -enable_if_t>::value, void> cast_safe(object &&) {} +enable_if_t::value + && !(std::is_same>::value && !std::is_pointer::value), + T> +cast_safe(object &&o) { + return pybind11::cast(std::move(o)); +} + +#else + +template +enable_if_t>::value, void> cast_safe(object &&) {} + template enable_if_t, - std::is_same>>::value, + std::is_same>>::value, T> cast_safe(object &&o) { return pybind11::cast(std::move(o)); } +#endif + PYBIND11_NAMESPACE_END(detail) // The overloads could coexist, i.e. the #if is not strictly speaking needed, diff --git a/tests/test_virtual_functions.cpp b/tests/test_virtual_functions.cpp index 4d71669a47..db05374fc1 100644 --- a/tests/test_virtual_functions.cpp +++ b/tests/test_virtual_functions.cpp @@ -230,6 +230,35 @@ inline int test_override_cache(std::shared_ptr const // are rather long). void initialize_inherited_virtuals(py::module_ &m); +namespace issue_4117 { // Broken by PR #3861 + +class Animal { +public: + virtual ~Animal() {} + virtual void *go() = 0; +}; + +class PyAnimal : public Animal { +public: + /* Inherit the constructors */ + using Animal::Animal; + /* Trampoline (need one for each virtual function) */ + void *go() override { + PYBIND11_OVERRIDE_PURE(void *, /* Return type */ + Animal, /* Parent class */ + go, /* Name of function in C++ (must match Python name) */ + ); + } +}; + +void bindings(py::module_ m) { + py::class_(m, "Animal") + .def(py::init<>()) + .def("go", &Animal::go); +} + +} // namespace issue_4117 + TEST_SUBMODULE(virtual_functions, m) { // test_override py::class_(m, "ExampleVirt") @@ -410,6 +439,8 @@ TEST_SUBMODULE(virtual_functions, m) { .def("func", &test_override_cache_helper::func); m.def("test_override_cache", test_override_cache); + + issue_4117::bindings(m); } // Inheriting virtual methods. We do two versions here: the repeat-everything version and the @@ -589,32 +620,3 @@ void initialize_inherited_virtuals(py::module_ &m) { m.def("test_gil", &test_gil); m.def("test_gil_from_thread", &test_gil_from_thread); }; - -namespace issue4117 { // Broken by PR #3861 - -class Animal { -public: - virtual ~Animal() {} - virtual void *go() = 0; -}; - -class PyAnimal : public Animal { -public: - /* Inherit the constructors */ - using Animal::Animal; - /* Trampoline (need one for each virtual function) */ - void *go() override { - PYBIND11_OVERRIDE_PURE(void *, /* Return type */ - Animal, /* Parent class */ - go, /* Name of function in C++ (must match Python name) */ - ); - } -}; - -void bindings(py::module_ m) { - py::class_(m, "Animal") - .def(py::init<>()) - .def("go", &Animal::go); -} - -} // namespace issue4117 diff --git a/tests/test_virtual_functions.py b/tests/test_virtual_functions.py index 4d00d3690d..e8a5d45966 100644 --- a/tests/test_virtual_functions.py +++ b/tests/test_virtual_functions.py @@ -457,3 +457,12 @@ class Test(m.test_override_cache_helper): for _ in range(1500): assert m.test_override_cache(func()) == 42 assert m.test_override_cache(func2()) == 0 + + +def test_issue_4117(): + class Lynx(m.Animal): + def go(self): + return self + + obj = Lynx() + assert obj.go() is obj From 554c03012ebfc1be4c665c5944d0e801ad4691d2 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 21 Oct 2022 19:56:54 -0700 Subject: [PATCH 3/3] Fix and test for issue #4117 --- include/pybind11/cast.h | 28 ---------------------------- tests/test_virtual_functions.cpp | 2 +- tests/test_virtual_functions.py | 2 +- 3 files changed, 2 insertions(+), 30 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index cac61a9eb1..b36383f8ac 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -1179,32 +1179,6 @@ enable_if_t::value, T> cast_safe(object &&) pybind11_fail("Internal error: cast_safe fallback invoked"); } -#ifdef PR3861_ROLLBACK - -template -enable_if_t::value, T> cast_safe(object &&o) { - return pybind11::cast(std::move(o)); -} - -template <> -inline void cast_safe(object &&) {} - -#elif defined(FIRST_STAB) - -template -enable_if_t>::value && !std::is_pointer::value, void> -cast_safe(object &&) {} - -template -enable_if_t::value - && !(std::is_same>::value && !std::is_pointer::value), - T> -cast_safe(object &&o) { - return pybind11::cast(std::move(o)); -} - -#else - template enable_if_t>::value, void> cast_safe(object &&) {} @@ -1216,8 +1190,6 @@ cast_safe(object &&o) { return pybind11::cast(std::move(o)); } -#endif - PYBIND11_NAMESPACE_END(detail) // The overloads could coexist, i.e. the #if is not strictly speaking needed, diff --git a/tests/test_virtual_functions.cpp b/tests/test_virtual_functions.cpp index db05374fc1..462f7adfec 100644 --- a/tests/test_virtual_functions.cpp +++ b/tests/test_virtual_functions.cpp @@ -252,7 +252,7 @@ class PyAnimal : public Animal { }; void bindings(py::module_ m) { - py::class_(m, "Animal") + py::class_(m, "Issue4117Animal") .def(py::init<>()) .def("go", &Animal::go); } diff --git a/tests/test_virtual_functions.py b/tests/test_virtual_functions.py index e8a5d45966..aa9b8e0230 100644 --- a/tests/test_virtual_functions.py +++ b/tests/test_virtual_functions.py @@ -460,7 +460,7 @@ class Test(m.test_override_cache_helper): def test_issue_4117(): - class Lynx(m.Animal): + class Lynx(m.Issue4117Animal): def go(self): return self