Add type_caster<PyObject> (#4601)

* Add `type_caster<PyObject>` (tests are still incomplete).

* Fix oversight (`const PyObject *`).

* Ensure `type_caster<PyObject>` only works for `PyObject *`

* Move `is_same_ignoring_cvref` into `detail` namespace.

* Add test_cast_nullptr

* Change is_same_ignoring_cvref from variable template to using.

```
test_type_caster_pyobject_ptr.cpp:8:23: error: variable templates only available with ‘-std=c++14’ or ‘-std=gnu++14’ [-Werror]
    8 | static constexpr bool is_same_ignoring_cvref = std::is_same<detail::remove_cvref_t<T>, U>::value;
      |                       ^~~~~~~~~~~~~~~~~~~~~~
```

* Remove `return_value_policy::reference_internal` `keep_alive` feature (because of doubts about it actually being useful).

* Add missing test, fix bug (missing `throw error_already_set();`), various cosmetic changes.

* Move `type_caster<PyObject>` from test to new include (pybind11/type_caster_pyobject_ptr.h)

* Add new header file to CMakeLists.txt and tests/extra_python_package/test_files.py

* Backport changes from https://github.com/google/pywrapcc/pull/30021 to https://github.com/pybind/pybind11/pull/4601

* Fix oversight in test (to resolve a valgrind leak detection error) and add a related comment in cast.h.

No production code changes.

Make tests more sensitive by using `ValueHolder` instead of empty tuples and dicts.

Manual leak checks with `while True:` & top command repeated for all tests.

* Add tests for interop with stl.h `list_caster`

(No production code changes.)

* Bug fix in test. Minor comment enhancements.

* Change `type_caster<PyObject>::name` to `object`, as suggested by @Skylion007

* Expand comment for the new `T cast(const handle &handle)` [`T` = `PyObject *`]

* Add `T cast(object &&obj)` overload as suggested by @Skylion007

The original suggestion leads to `error: call to 'cast' is ambiguous` (full error message below), therefore SFINAE guarding is needed.

```
clang++ -o pybind11/tests/test_type_caster_pyobject_ptr.os -c -std=c++17 -fPIC -fvisibility=hidden -O0 -g -Wall -Wextra -Wconversion -Wcast-qual -Wdeprecated -Wundef -Wnon-virtual-dtor -Wunused-result -Werror -isystem /usr/include/python3.10 -isystem /usr/include/eigen3 -DPYBIND11_STRICT_ASSERTS_CLASS_HOLDER_VS_TYPE_CASTER_MIX -DPYBIND11_ENABLE_TYPE_CASTER_ODR_GUARD_IF_AVAILABLE -DPYBIND11_TEST_BOOST -Ipybind11/include -I/usr/local/google/home/rwgk/forked/pybind11/include -I/usr/local/google/home/rwgk/clone/pybind11/include /usr/local/google/home/rwgk/forked/pybind11/tests/test_type_caster_pyobject_ptr.cpp
In file included from /usr/local/google/home/rwgk/forked/pybind11/tests/test_type_caster_pyobject_ptr.cpp:1:
In file included from /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/functional.h:12:
In file included from /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/pybind11.h:13:
In file included from /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/detail/class.h:12:
In file included from /usr/local/google/home/rwgk/forked/pybind11/include/pybind11/attr.h:14:
/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/cast.h:1165:12: error: call to 'cast' is ambiguous
    return pybind11::cast<T>(std::move(*this));
           ^~~~~~~~~~~~~~~~~
/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/functional.h:109:70: note: in instantiation of function template specialization 'pybind11::object::cast<_object *>' requested here
                return hfunc.f(std::forward<Args>(args)...).template cast<Return>();
                                                                     ^
/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/functional.h:103:16: note: in instantiation of member function 'pybind11::detail::type_caster<std::function<_object *(int)>>::load(pybind11::handle, bool)::func_wrapper::operator()' requested here
        struct func_wrapper {
               ^
/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/cast.h:1456:47: note: in instantiation of member function 'pybind11::detail::type_caster<std::function<_object *(int)>>::load' requested here
        if ((... || !std::get<Is>(argcasters).load(call.args[Is], call.args_convert[Is]))) {
                                              ^
/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/cast.h:1434:50: note: in instantiation of function template specialization 'pybind11::detail::argument_loader<const std::function<_object *(int)> &, int>::load_impl_sequence<0UL, 1UL>' requested here
    bool load_args(function_call &call) { return load_impl_sequence(call, indices{}); }
                                                 ^
/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/pybind11.h:227:33: note: in instantiation of member function 'pybind11::detail::argument_loader<const std::function<_object *(int)> &, int>::load_args' requested here
            if (!args_converter.load_args(call)) {
                                ^
/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/pybind11.h:101:9: note: in instantiation of function template specialization 'pybind11::cpp_function::initialize<(lambda at /usr/local/google/home/rwgk/forked/pybind11/tests/test_type_caster_pyobject_ptr.cpp:50:9), _object *, const std::function<_object *(int)> &, int, pybind11::name, pybind11::scope, pybind11::sibling, pybind11::return_value_policy>' requested here
        initialize(
        ^
/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/pybind11.h:1163:22: note: in instantiation of function template specialization 'pybind11::cpp_function::cpp_function<(lambda at /usr/local/google/home/rwgk/forked/pybind11/tests/test_type_caster_pyobject_ptr.cpp:50:9), pybind11::name, pybind11::scope, pybind11::sibling, pybind11::return_value_policy, void>' requested here
        cpp_function func(std::forward<Func>(f),
                     ^
/usr/local/google/home/rwgk/forked/pybind11/tests/test_type_caster_pyobject_ptr.cpp:48:7: note: in instantiation of function template specialization 'pybind11::module_::def<(lambda at /usr/local/google/home/rwgk/forked/pybind11/tests/test_type_caster_pyobject_ptr.cpp:50:9), pybind11::return_value_policy>' requested here
    m.def(
      ^
/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/cast.h:1077:3: note: candidate function [with T = _object *, $1 = 0]
T cast(object &&obj) {
  ^
/usr/local/google/home/rwgk/forked/pybind11/include/pybind11/cast.h:1149:1: note: candidate function [with T = _object *]
cast(object &&object) {
^
1 error generated.
```
This commit is contained in:
Ralf W. Grosse-Kunstleve 2023-05-07 10:15:53 -07:00 committed by GitHub
parent f701654633
commit 90312a6ee8
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 336 additions and 2 deletions

View File

@ -139,7 +139,8 @@ set(PYBIND11_HEADERS
include/pybind11/pytypes.h
include/pybind11/stl.h
include/pybind11/stl_bind.h
include/pybind11/stl/filesystem.h)
include/pybind11/stl/filesystem.h
include/pybind11/type_caster_pyobject_ptr.h)
# Compare with grep and warn if mismatched
if(PYBIND11_MASTER_PROJECT AND NOT CMAKE_VERSION VERSION_LESS 3.12)

View File

@ -1041,7 +1041,11 @@ make_caster<T> load_type(const handle &handle) {
PYBIND11_NAMESPACE_END(detail)
// pytype -> C++ type
template <typename T, detail::enable_if_t<!detail::is_pyobject<T>::value, int> = 0>
template <typename T,
detail::enable_if_t<!detail::is_pyobject<T>::value
&& !detail::is_same_ignoring_cvref<T, PyObject *>::value,
int>
= 0>
T cast(const handle &handle) {
using namespace detail;
static_assert(!cast_is_temporary_value_reference<T>::value,
@ -1055,6 +1059,34 @@ T cast(const handle &handle) {
return T(reinterpret_borrow<object>(handle));
}
// Note that `cast<PyObject *>(obj)` increments the reference count of `obj`.
// This is necessary for the case that `obj` is a temporary, and could
// not possibly be different, given
// 1. the established convention that the passed `handle` is borrowed, and
// 2. we don't want to force all generic code using `cast<T>()` to special-case
// handling of `T` = `PyObject *` (to increment the reference count there).
// It is the responsibility of the caller to ensure that the reference count
// is decremented.
template <typename T,
typename Handle,
detail::enable_if_t<detail::is_same_ignoring_cvref<T, PyObject *>::value
&& detail::is_same_ignoring_cvref<Handle, handle>::value,
int>
= 0>
T cast(Handle &&handle) {
return handle.inc_ref().ptr();
}
// To optimize way an inc_ref/dec_ref cycle:
template <typename T,
typename Object,
detail::enable_if_t<detail::is_same_ignoring_cvref<T, PyObject *>::value
&& detail::is_same_ignoring_cvref<Object, object>::value,
int>
= 0>
T cast(Object &&obj) {
return obj.release().ptr();
}
// C++ type -> py::object
template <typename T, detail::enable_if_t<!detail::is_pyobject<T>::value, int> = 0>
object cast(T &&value,

View File

@ -661,6 +661,10 @@ template <class T>
using remove_cvref_t = typename remove_cvref<T>::type;
#endif
/// Example usage: is_same_ignoring_cvref<T, PyObject *>::value
template <typename T, typename U>
using is_same_ignoring_cvref = std::is_same<detail::remove_cvref_t<T>, U>;
/// Index sequences
#if defined(PYBIND11_CPP14)
using std::index_sequence;

View File

@ -0,0 +1,61 @@
// Copyright (c) 2023 The pybind Community.
#pragma once
#include "detail/common.h"
#include "detail/descr.h"
#include "cast.h"
#include "pytypes.h"
PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
PYBIND11_NAMESPACE_BEGIN(detail)
template <>
class type_caster<PyObject> {
public:
static constexpr auto name = const_name("object"); // See discussion under PR #4601.
// This overload is purely to guard against accidents.
template <typename T,
detail::enable_if_t<!is_same_ignoring_cvref<T, PyObject *>::value, int> = 0>
static handle cast(T &&, return_value_policy, handle /*parent*/) {
static_assert(is_same_ignoring_cvref<T, PyObject *>::value,
"Invalid C++ type T for to-Python conversion (type_caster<PyObject>).");
return nullptr; // Unreachable.
}
static handle cast(PyObject *src, return_value_policy policy, handle /*parent*/) {
if (src == nullptr) {
throw error_already_set();
}
if (PyErr_Occurred()) {
raise_from(PyExc_SystemError, "src != nullptr but PyErr_Occurred()");
throw error_already_set();
}
if (policy == return_value_policy::take_ownership) {
return src;
}
if (policy == return_value_policy::reference
|| policy == return_value_policy::automatic_reference) {
return handle(src).inc_ref();
}
pybind11_fail("type_caster<PyObject>::cast(): unsupported return_value_policy: "
+ std::to_string(static_cast<int>(policy)));
}
bool load(handle src, bool) {
value = reinterpret_borrow<object>(src);
return true;
}
template <typename T>
using cast_op_type = PyObject *;
explicit operator PyObject *() { return value.ptr(); }
private:
object value;
};
PYBIND11_NAMESPACE_END(detail)
PYBIND11_NAMESPACE_END(PYBIND11_NAMESPACE)

View File

@ -154,6 +154,7 @@ set(PYBIND11_TEST_FILES
test_stl_binders
test_tagbased_polymorphic
test_thread
test_type_caster_pyobject_ptr
test_union
test_unnamed_namespace_a
test_unnamed_namespace_b

View File

@ -43,6 +43,7 @@ main_headers = {
"include/pybind11/pytypes.h",
"include/pybind11/stl.h",
"include/pybind11/stl_bind.h",
"include/pybind11/type_caster_pyobject_ptr.h",
}
detail_headers = {

View File

@ -0,0 +1,130 @@
#include <pybind11/functional.h>
#include <pybind11/stl.h>
#include <pybind11/type_caster_pyobject_ptr.h>
#include "pybind11_tests.h"
#include <cstddef>
#include <vector>
namespace {
std::vector<PyObject *> make_vector_pyobject_ptr(const py::object &ValueHolder) {
std::vector<PyObject *> vec_obj;
for (int i = 1; i < 3; i++) {
vec_obj.push_back(ValueHolder(i * 93).release().ptr());
}
// This vector now owns the refcounts.
return vec_obj;
}
} // namespace
TEST_SUBMODULE(type_caster_pyobject_ptr, m) {
m.def("cast_from_pyobject_ptr", []() {
PyObject *ptr = PyLong_FromLongLong(6758L);
return py::cast(ptr, py::return_value_policy::take_ownership);
});
m.def("cast_handle_to_pyobject_ptr", [](py::handle obj) {
auto rc1 = obj.ref_count();
auto *ptr = py::cast<PyObject *>(obj);
auto rc2 = obj.ref_count();
if (rc2 != rc1 + 1) {
return -1;
}
return 100 - py::reinterpret_steal<py::object>(ptr).attr("value").cast<int>();
});
m.def("cast_object_to_pyobject_ptr", [](py::object obj) {
py::handle hdl = obj;
auto rc1 = hdl.ref_count();
auto *ptr = py::cast<PyObject *>(std::move(obj));
auto rc2 = hdl.ref_count();
if (rc2 != rc1) {
return -1;
}
return 300 - py::reinterpret_steal<py::object>(ptr).attr("value").cast<int>();
});
m.def("cast_list_to_pyobject_ptr", [](py::list lst) {
// This is to cover types implicitly convertible to object.
py::handle hdl = lst;
auto rc1 = hdl.ref_count();
auto *ptr = py::cast<PyObject *>(std::move(lst));
auto rc2 = hdl.ref_count();
if (rc2 != rc1) {
return -1;
}
return 400 - static_cast<int>(py::len(py::reinterpret_steal<py::list>(ptr)));
});
m.def(
"return_pyobject_ptr",
[]() { return PyLong_FromLongLong(2314L); },
py::return_value_policy::take_ownership);
m.def("pass_pyobject_ptr", [](PyObject *ptr) {
return 200 - py::reinterpret_borrow<py::object>(ptr).attr("value").cast<int>();
});
m.def("call_callback_with_object_return",
[](const std::function<py::object(int)> &cb, int value) { return cb(value); });
m.def(
"call_callback_with_pyobject_ptr_return",
[](const std::function<PyObject *(int)> &cb, int value) { return cb(value); },
py::return_value_policy::take_ownership);
m.def(
"call_callback_with_pyobject_ptr_arg",
[](const std::function<int(PyObject *)> &cb, py::handle obj) { return cb(obj.ptr()); },
py::arg("cb"), // This triggers return_value_policy::automatic_reference
py::arg("obj"));
m.def("cast_to_pyobject_ptr_nullptr", [](bool set_error) {
if (set_error) {
PyErr_SetString(PyExc_RuntimeError, "Reflective of healthy error handling.");
}
PyObject *ptr = nullptr;
py::cast(ptr);
});
m.def("cast_to_pyobject_ptr_non_nullptr_with_error_set", []() {
PyErr_SetString(PyExc_RuntimeError, "Reflective of unhealthy error handling.");
py::cast(Py_None);
});
m.def("pass_list_pyobject_ptr", [](const std::vector<PyObject *> &vec_obj) {
int acc = 0;
for (const auto &ptr : vec_obj) {
acc = acc * 1000 + py::reinterpret_borrow<py::object>(ptr).attr("value").cast<int>();
}
return acc;
});
m.def("return_list_pyobject_ptr_take_ownership",
make_vector_pyobject_ptr,
// Ownership is transferred one-by-one when the vector is converted to a Python list.
py::return_value_policy::take_ownership);
m.def("return_list_pyobject_ptr_reference",
make_vector_pyobject_ptr,
// Ownership is not transferred.
py::return_value_policy::reference);
m.def("dec_ref_each_pyobject_ptr", [](const std::vector<PyObject *> &vec_obj) {
std::size_t i = 0;
for (; i < vec_obj.size(); i++) {
py::handle h(vec_obj[i]);
if (static_cast<std::size_t>(h.ref_count()) < 2) {
break; // Something is badly wrong.
}
h.dec_ref();
}
return i;
});
m.def("pass_pyobject_ptr_and_int", [](PyObject *, int) {});
#ifdef PYBIND11_NO_COMPILE_SECTION // Change to ifndef for manual testing.
{
PyObject *ptr = nullptr;
(void) py::cast(*ptr);
}
#endif
}

View File

@ -0,0 +1,104 @@
import pytest
from pybind11_tests import type_caster_pyobject_ptr as m
# For use as a temporary user-defined object, to maximize sensitivity of the tests below.
class ValueHolder:
def __init__(self, value):
self.value = value
def test_cast_from_pyobject_ptr():
assert m.cast_from_pyobject_ptr() == 6758
def test_cast_handle_to_pyobject_ptr():
assert m.cast_handle_to_pyobject_ptr(ValueHolder(24)) == 76
def test_cast_object_to_pyobject_ptr():
assert m.cast_object_to_pyobject_ptr(ValueHolder(43)) == 257
def test_cast_list_to_pyobject_ptr():
assert m.cast_list_to_pyobject_ptr([1, 2, 3, 4, 5]) == 395
def test_return_pyobject_ptr():
assert m.return_pyobject_ptr() == 2314
def test_pass_pyobject_ptr():
assert m.pass_pyobject_ptr(ValueHolder(82)) == 118
@pytest.mark.parametrize(
"call_callback",
[
m.call_callback_with_object_return,
m.call_callback_with_pyobject_ptr_return,
],
)
def test_call_callback_with_object_return(call_callback):
def cb(value):
if value < 0:
raise ValueError("Raised from cb")
return ValueHolder(1000 - value)
assert call_callback(cb, 287).value == 713
with pytest.raises(ValueError, match="^Raised from cb$"):
call_callback(cb, -1)
def test_call_callback_with_pyobject_ptr_arg():
def cb(obj):
return 300 - obj.value
assert m.call_callback_with_pyobject_ptr_arg(cb, ValueHolder(39)) == 261
@pytest.mark.parametrize("set_error", [True, False])
def test_cast_to_python_nullptr(set_error):
expected = {
True: r"^Reflective of healthy error handling\.$",
False: (
r"^Internal error: pybind11::error_already_set called "
r"while Python error indicator not set\.$"
),
}[set_error]
with pytest.raises(RuntimeError, match=expected):
m.cast_to_pyobject_ptr_nullptr(set_error)
def test_cast_to_python_non_nullptr_with_error_set():
with pytest.raises(SystemError) as excinfo:
m.cast_to_pyobject_ptr_non_nullptr_with_error_set()
assert str(excinfo.value) == "src != nullptr but PyErr_Occurred()"
assert str(excinfo.value.__cause__) == "Reflective of unhealthy error handling."
def test_pass_list_pyobject_ptr():
acc = m.pass_list_pyobject_ptr([ValueHolder(842), ValueHolder(452)])
assert acc == 842452
def test_return_list_pyobject_ptr_take_ownership():
vec_obj = m.return_list_pyobject_ptr_take_ownership(ValueHolder)
assert [e.value for e in vec_obj] == [93, 186]
def test_return_list_pyobject_ptr_reference():
vec_obj = m.return_list_pyobject_ptr_reference(ValueHolder)
assert [e.value for e in vec_obj] == [93, 186]
# Commenting out the next `assert` will leak the Python references.
# An easy way to see evidence of the leaks:
# Insert `while True:` as the first line of this function and monitor the
# process RES (Resident Memory Size) with the Unix top command.
assert m.dec_ref_each_pyobject_ptr(vec_obj) == 2
def test_type_caster_name_via_incompatible_function_arguments_type_error():
with pytest.raises(TypeError, match=r"1\. \(arg0: object, arg1: int\) -> None"):
m.pass_pyobject_ptr_and_int(ValueHolder(101), ValueHolder(202))