From 913aa8befe5a6a81a7e8d403881b677db95965f2 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 10 Mar 2022 12:58:25 -0800 Subject: [PATCH 1/6] Reproducer for https://github.com/pybind/pybind11/issues/3788 Expected to build & run as-is. Uncommenting reproduces the infinite recursion. --- tests/CMakeLists.txt | 1 + tests/test_class_sh_getattr_issue.cpp | 25 +++++++++++++++++++++++++ tests/test_class_sh_getattr_issue.py | 8 ++++++++ 3 files changed, 34 insertions(+) create mode 100644 tests/test_class_sh_getattr_issue.cpp create mode 100644 tests/test_class_sh_getattr_issue.py diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index d9086f39b1..2574eeff5d 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -126,6 +126,7 @@ set(PYBIND11_TEST_FILES test_class_sh_disowning test_class_sh_disowning_mi test_class_sh_factory_constructors + test_class_sh_getattr_issue test_class_sh_inheritance test_class_sh_module_local.py test_class_sh_shared_ptr_copy_move diff --git a/tests/test_class_sh_getattr_issue.cpp b/tests/test_class_sh_getattr_issue.cpp new file mode 100644 index 0000000000..f699e54ea5 --- /dev/null +++ b/tests/test_class_sh_getattr_issue.cpp @@ -0,0 +1,25 @@ +#include + +#include "pybind11_tests.h" + +namespace pybind11_tests { +namespace class_sh_getattr_issue { + +// https://github.com/pybind/pybind11/issues/3788 +struct Foo { + Foo() = default; + int bar() const { return 42; } +}; + +} // namespace class_sh_getattr_issue +} // namespace pybind11_tests + +PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_getattr_issue::Foo) + +TEST_SUBMODULE(class_sh_getattr_issue, m) { + using namespace pybind11_tests::class_sh_getattr_issue; + py::classh(m, "Foo") + .def(py::init<>()) + .def("bar", &Foo::bar) + .def("__getattr__", [](Foo &, std::string key) { return "GetAttr: " + key; }); +} diff --git a/tests/test_class_sh_getattr_issue.py b/tests/test_class_sh_getattr_issue.py new file mode 100644 index 0000000000..8a2351b5ab --- /dev/null +++ b/tests/test_class_sh_getattr_issue.py @@ -0,0 +1,8 @@ +from pybind11_tests import class_sh_getattr_issue as m + + +def test_drvd1_add_in_cpp_unique_ptr(): + f = m.Foo() + # assert f.bar() == 42 + # assert f.something == "GetAttr: something" + del f From 0af3532fddde2e2b288a7541b5f8d74d3c882c81 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 10 Mar 2022 13:30:46 -0800 Subject: [PATCH 2/6] Moving try_as_void_ptr_capsule() to the end of load_impl() --- include/pybind11/detail/smart_holder_type_casters.h | 7 ++++--- tests/test_class_sh_getattr_issue.py | 5 ++--- tests/test_class_sh_void_ptr_capsule.py | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index afec27bd8a..0c5179e512 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -200,9 +200,6 @@ class modified_type_caster_generic_load_impl { if (!src) { return false; } - if (cpptype && try_as_void_ptr_capsule(src)) { - return true; - } if (!typeinfo) { return try_load_foreign_module_local(src); } @@ -296,6 +293,10 @@ class modified_type_caster_generic_load_impl { return true; } + if (cpptype && try_as_void_ptr_capsule(src)) { + return true; + } + return false; } diff --git a/tests/test_class_sh_getattr_issue.py b/tests/test_class_sh_getattr_issue.py index 8a2351b5ab..85789c1d2d 100644 --- a/tests/test_class_sh_getattr_issue.py +++ b/tests/test_class_sh_getattr_issue.py @@ -3,6 +3,5 @@ def test_drvd1_add_in_cpp_unique_ptr(): f = m.Foo() - # assert f.bar() == 42 - # assert f.something == "GetAttr: something" - del f + assert f.bar() == 42 + assert f.something == "GetAttr: something" diff --git a/tests/test_class_sh_void_ptr_capsule.py b/tests/test_class_sh_void_ptr_capsule.py index d5bf449614..982b3ea40f 100644 --- a/tests/test_class_sh_void_ptr_capsule.py +++ b/tests/test_class_sh_void_ptr_capsule.py @@ -6,9 +6,9 @@ @pytest.mark.parametrize( "ctor, caller, expected, capsule_generated", [ - (m.Valid, m.get_from_valid_capsule, 101, True), + (m.Valid, m.get_from_valid_capsule, 101, False), (m.NoConversion, m.get_from_no_conversion_capsule, 102, False), - (m.NoCapsuleReturned, m.get_from_no_capsule_returned, 103, True), + (m.NoCapsuleReturned, m.get_from_no_capsule_returned, 103, False), (m.AsAnotherObject, m.get_from_valid_capsule, 104, True), ], ) From bc442f0cd8246ff596afc64140a5e928dc2d81b9 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 10 Mar 2022 13:50:44 -0800 Subject: [PATCH 3/6] Moving new test into the existing test_class_sh_void_ptr_capsule --- tests/CMakeLists.txt | 1 - tests/test_class_sh_getattr_issue.cpp | 25 ------------------------ tests/test_class_sh_getattr_issue.py | 7 ------- tests/test_class_sh_void_ptr_capsule.cpp | 13 ++++++++++++ tests/test_class_sh_void_ptr_capsule.py | 6 ++++++ 5 files changed, 19 insertions(+), 33 deletions(-) delete mode 100644 tests/test_class_sh_getattr_issue.cpp delete mode 100644 tests/test_class_sh_getattr_issue.py diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 2574eeff5d..d9086f39b1 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -126,7 +126,6 @@ set(PYBIND11_TEST_FILES test_class_sh_disowning test_class_sh_disowning_mi test_class_sh_factory_constructors - test_class_sh_getattr_issue test_class_sh_inheritance test_class_sh_module_local.py test_class_sh_shared_ptr_copy_move diff --git a/tests/test_class_sh_getattr_issue.cpp b/tests/test_class_sh_getattr_issue.cpp deleted file mode 100644 index f699e54ea5..0000000000 --- a/tests/test_class_sh_getattr_issue.cpp +++ /dev/null @@ -1,25 +0,0 @@ -#include - -#include "pybind11_tests.h" - -namespace pybind11_tests { -namespace class_sh_getattr_issue { - -// https://github.com/pybind/pybind11/issues/3788 -struct Foo { - Foo() = default; - int bar() const { return 42; } -}; - -} // namespace class_sh_getattr_issue -} // namespace pybind11_tests - -PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_getattr_issue::Foo) - -TEST_SUBMODULE(class_sh_getattr_issue, m) { - using namespace pybind11_tests::class_sh_getattr_issue; - py::classh(m, "Foo") - .def(py::init<>()) - .def("bar", &Foo::bar) - .def("__getattr__", [](Foo &, std::string key) { return "GetAttr: " + key; }); -} diff --git a/tests/test_class_sh_getattr_issue.py b/tests/test_class_sh_getattr_issue.py deleted file mode 100644 index 85789c1d2d..0000000000 --- a/tests/test_class_sh_getattr_issue.py +++ /dev/null @@ -1,7 +0,0 @@ -from pybind11_tests import class_sh_getattr_issue as m - - -def test_drvd1_add_in_cpp_unique_ptr(): - f = m.Foo() - assert f.bar() == 42 - assert f.something == "GetAttr: something" diff --git a/tests/test_class_sh_void_ptr_capsule.cpp b/tests/test_class_sh_void_ptr_capsule.cpp index f6597d568f..a13b3c4fa4 100644 --- a/tests/test_class_sh_void_ptr_capsule.cpp +++ b/tests/test_class_sh_void_ptr_capsule.cpp @@ -75,6 +75,12 @@ int get_from_no_conversion_capsule(const NoConversion *c) { return c->get(); } int get_from_no_capsule_returned(const NoCapsuleReturned *c) { return c->get(); } +// https://github.com/pybind/pybind11/issues/3788 +struct TypeWithGetattr { + TypeWithGetattr() = default; + int get_42() const { return 42; } +}; + } // namespace class_sh_void_ptr_capsule } // namespace pybind11_tests @@ -83,6 +89,7 @@ PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_void_ptr_capsule::Va PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_void_ptr_capsule::NoConversion) PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_void_ptr_capsule::NoCapsuleReturned) PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_void_ptr_capsule::AsAnotherObject) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_void_ptr_capsule::TypeWithGetattr) TEST_SUBMODULE(class_sh_void_ptr_capsule, m) { using namespace pybind11_tests::class_sh_void_ptr_capsule; @@ -128,4 +135,10 @@ TEST_SUBMODULE(class_sh_void_ptr_capsule, m) { m.def("get_from_unique_ptr_valid_capsule", &get_from_unique_ptr_valid_capsule); m.def("get_from_no_conversion_capsule", &get_from_no_conversion_capsule); m.def("get_from_no_capsule_returned", &get_from_no_capsule_returned); + + py::classh(m, "TypeWithGetattr") + .def(py::init<>()) + .def("get_42", &TypeWithGetattr::get_42) + .def("__getattr__", + [](TypeWithGetattr &, const std::string &key) { return "GetAttr: " + key; }); } diff --git a/tests/test_class_sh_void_ptr_capsule.py b/tests/test_class_sh_void_ptr_capsule.py index 982b3ea40f..20193b4871 100644 --- a/tests/test_class_sh_void_ptr_capsule.py +++ b/tests/test_class_sh_void_ptr_capsule.py @@ -31,3 +31,9 @@ def test_as_void_ptr_capsule_unsupported(ctor, caller, pointer_type, capsule_gen caller(obj) assert pointer_type in str(excinfo.value) assert obj.capsule_generated == capsule_generated + + +def test_type_with_getattr(): + obj = m.TypeWithGetattr() + assert obj.get_42() == 42 + assert obj.something == "GetAttr: something" From ce6bfba0351d87f7274d98f09da08ae78940b80d Mon Sep 17 00:00:00 2001 From: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com> Date: Thu, 10 Mar 2022 14:09:18 -0800 Subject: [PATCH 4/6] Experiment --- README.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.rst b/README.rst index f9e1ebe329..75875ef130 100644 --- a/README.rst +++ b/README.rst @@ -1,7 +1,7 @@ .. figure:: https://github.com/pybind/pybind11/raw/master/docs/pybind11-logo.png :alt: pybind11 logo -**pybind11 — Seamless operability between C++11 and Python** +**pybind — Seamless operability between C++11 and Python** |Latest Documentation Status| |Stable Documentation Status| |Gitter chat| |GitHub Discussions| |CI| |Build status| From 7d4fa2ff1130955e627ddf335c11aafe48967b4f Mon Sep 17 00:00:00 2001 From: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com> Date: Thu, 10 Mar 2022 15:41:42 -0800 Subject: [PATCH 5/6] Remove comments and simplify the test cases. --- README.rst | 2 +- .../detail/smart_holder_type_casters.h | 4 +- tests/test_class_sh_void_ptr_capsule.cpp | 43 ++++++------------- 3 files changed, 16 insertions(+), 33 deletions(-) diff --git a/README.rst b/README.rst index 75875ef130..f9e1ebe329 100644 --- a/README.rst +++ b/README.rst @@ -1,7 +1,7 @@ .. figure:: https://github.com/pybind/pybind11/raw/master/docs/pybind11-logo.png :alt: pybind11 logo -**pybind — Seamless operability between C++11 and Python** +**pybind11 — Seamless operability between C++11 and Python** |Latest Documentation Status| |Stable Documentation Status| |Gitter chat| |GitHub Discussions| |CI| |Build status| diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 0c5179e512..d4819793cc 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -292,11 +292,9 @@ class modified_type_caster_generic_load_impl { loaded_v_h = value_and_holder(); return true; } - - if (cpptype && try_as_void_ptr_capsule(src)) { + if (convert && cpptype && try_as_void_ptr_capsule(src)) { return true; } - return false; } diff --git a/tests/test_class_sh_void_ptr_capsule.cpp b/tests/test_class_sh_void_ptr_capsule.cpp index a13b3c4fa4..3ebc3de874 100644 --- a/tests/test_class_sh_void_ptr_capsule.cpp +++ b/tests/test_class_sh_void_ptr_capsule.cpp @@ -7,17 +7,7 @@ namespace pybind11_tests { namespace class_sh_void_ptr_capsule { -// Without the helper we will run into a type_caster::load recursion. -// This is because whenever the type_caster::load is called, it checks -// whether the object defines an `as_` method that returns the void pointer -// capsule. If yes, it calls the method. But in the following testcases, those -// `as_` methods are defined with pybind11, which implicitly takes the object -// itself as the first parameter. Therefore calling those methods causes loading -// the object again, which causes infinite recursion. -// This test is unusual in the sense that the void pointer capsules are meant to -// be provided by objects wrapped with systems other than pybind11 -// (i.e. having to avoid the recursion is an artificial problem, not the norm). -// Conveniently, the helper also serves to keep track of `capsule_generated`. +// Conveniently, the helper serves to keep track of `capsule_generated`. struct HelperBase { HelperBase() = default; HelperBase(const HelperBase &) = delete; @@ -65,6 +55,12 @@ struct AsAnotherObject : public HelperBase { } }; +// https://github.com/pybind/pybind11/issues/3788 +struct TypeWithGetattr { + TypeWithGetattr() = default; + int get_42() const { return 42; } +}; + int get_from_valid_capsule(const Valid *c) { return c->get(); } int get_from_shared_ptr_valid_capsule(const std::shared_ptr &c) { return c->get(); } @@ -75,12 +71,6 @@ int get_from_no_conversion_capsule(const NoConversion *c) { return c->get(); } int get_from_no_capsule_returned(const NoCapsuleReturned *c) { return c->get(); } -// https://github.com/pybind/pybind11/issues/3788 -struct TypeWithGetattr { - TypeWithGetattr() = default; - int get_42() const { return 42; } -}; - } // namespace class_sh_void_ptr_capsule } // namespace pybind11_tests @@ -101,10 +91,8 @@ TEST_SUBMODULE(class_sh_void_ptr_capsule, m) { py::classh(m, "Valid") .def(py::init<>()) - .def("as_pybind11_tests_class_sh_void_ptr_capsule_Valid", [](HelperBase *self) { - auto *obj = dynamic_cast(self); - assert(obj != nullptr); - PyObject *capsule = obj->as_pybind11_tests_class_sh_void_ptr_capsule_Valid(); + .def("as_pybind11_tests_class_sh_void_ptr_capsule_Valid", [](Valid &self) { + PyObject *capsule = self.as_pybind11_tests_class_sh_void_ptr_capsule_Valid(); return pybind11::reinterpret_steal(capsule); }); @@ -113,20 +101,17 @@ TEST_SUBMODULE(class_sh_void_ptr_capsule, m) { py::classh(m, "NoCapsuleReturned") .def(py::init<>()) .def("as_pybind11_tests_class_sh_void_ptr_capsule_NoCapsuleReturned", - [](HelperBase *self) { - auto *obj = dynamic_cast(self); - assert(obj != nullptr); + [](NoCapsuleReturned& self) { PyObject *capsule - = obj->as_pybind11_tests_class_sh_void_ptr_capsule_NoCapsuleReturned(); + = self.as_pybind11_tests_class_sh_void_ptr_capsule_NoCapsuleReturned(); return pybind11::reinterpret_steal(capsule); }); py::classh(m, "AsAnotherObject") .def(py::init<>()) - .def("as_pybind11_tests_class_sh_void_ptr_capsule_Valid", [](HelperBase *self) { - auto *obj = dynamic_cast(self); - assert(obj != nullptr); - PyObject *capsule = obj->as_pybind11_tests_class_sh_void_ptr_capsule_Valid(); + .def("as_pybind11_tests_class_sh_void_ptr_capsule_Valid", + [](AsAnotherObject& self) { + PyObject *capsule = self.as_pybind11_tests_class_sh_void_ptr_capsule_Valid(); return pybind11::reinterpret_steal(capsule); }); From 5fbf89455f0ed3abc2f825fdac4f7d50a38bd755 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 10 Mar 2022 23:42:49 +0000 Subject: [PATCH 6/6] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/test_class_sh_void_ptr_capsule.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/test_class_sh_void_ptr_capsule.cpp b/tests/test_class_sh_void_ptr_capsule.cpp index 3ebc3de874..8083a74412 100644 --- a/tests/test_class_sh_void_ptr_capsule.cpp +++ b/tests/test_class_sh_void_ptr_capsule.cpp @@ -101,7 +101,7 @@ TEST_SUBMODULE(class_sh_void_ptr_capsule, m) { py::classh(m, "NoCapsuleReturned") .def(py::init<>()) .def("as_pybind11_tests_class_sh_void_ptr_capsule_NoCapsuleReturned", - [](NoCapsuleReturned& self) { + [](NoCapsuleReturned &self) { PyObject *capsule = self.as_pybind11_tests_class_sh_void_ptr_capsule_NoCapsuleReturned(); return pybind11::reinterpret_steal(capsule); @@ -109,8 +109,7 @@ TEST_SUBMODULE(class_sh_void_ptr_capsule, m) { py::classh(m, "AsAnotherObject") .def(py::init<>()) - .def("as_pybind11_tests_class_sh_void_ptr_capsule_Valid", - [](AsAnotherObject& self) { + .def("as_pybind11_tests_class_sh_void_ptr_capsule_Valid", [](AsAnotherObject &self) { PyObject *capsule = self.as_pybind11_tests_class_sh_void_ptr_capsule_Valid(); return pybind11::reinterpret_steal(capsule); });