Fixes issue #3788 (#3796)

* Reproducer for #3788 Expected to build & run as-is. Uncommenting reproduces the infinite recursion. * Moving try_as_void_ptr_capsule() to the end of load_impl() * Moving new test into the existing test_class_sh_void_ptr_capsule * Experiment * Remove comments and simplify the test cases. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Co-authored-by: Ralf W. Grosse-Kunstleve <rwgk@google.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
pybind · Mar 11, 2022 · 9d4b4df · 9d4b4df
1 parent bcc241f
commit 9d4b4df
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 29 deletions.
diff --git 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);
         }
@@ -295,7 +292,9 @@ class modified_type_caster_generic_load_impl {
             loaded_v_h = value_and_holder();
             return true;
         }
-
+        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
@@ -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<Valid> &c) { return c->get(); }
@@ -83,6 +79,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;
@@ -94,10 +91,8 @@ TEST_SUBMODULE(class_sh_void_ptr_capsule, m) {
 
     py::classh<Valid, HelperBase>(m, "Valid")
         .def(py::init<>())
-        .def("as_pybind11_tests_class_sh_void_ptr_capsule_Valid", [](HelperBase *self) {
-            auto *obj = dynamic_cast<Valid *>(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<py::capsule>(capsule);
         });
 
@@ -106,20 +101,16 @@ TEST_SUBMODULE(class_sh_void_ptr_capsule, m) {
     py::classh<NoCapsuleReturned, HelperBase>(m, "NoCapsuleReturned")
         .def(py::init<>())
         .def("as_pybind11_tests_class_sh_void_ptr_capsule_NoCapsuleReturned",
-             [](HelperBase *self) {
-                 auto *obj = dynamic_cast<NoCapsuleReturned *>(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<py::capsule>(capsule);
              });
 
     py::classh<AsAnotherObject, HelperBase>(m, "AsAnotherObject")
         .def(py::init<>())
-        .def("as_pybind11_tests_class_sh_void_ptr_capsule_Valid", [](HelperBase *self) {
-            auto *obj = dynamic_cast<AsAnotherObject *>(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<py::capsule>(capsule);
         });
 
@@ -128,4 +119,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<TypeWithGetattr>(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
@@ -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),
     ],
 )
@@ -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"