Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add py::nullptr_default_arg (intended for PyObject * arguments) #30081

Merged
merged 6 commits into from
Dec 8, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions include/pybind11/attr.h
Original file line number Diff line number Diff line change
Expand Up @@ -179,13 +179,20 @@ struct argument_record {
const char *name; ///< Argument name
const char *descr; ///< Human-readable version of the argument value
handle value; ///< Associated Python object
// The explicit value_is_nullptr variable is for safety, to unambiguously
// distinguish these two cases in all situations:
// * value is nullptr on purpose.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which of the case does True correspond to?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, thanks. I made the comment more precise.

// * value is nullptr because of an error condition.
bool value_is_nullptr;
from_python_policies policies;

argument_record(const char *name,
const char *descr,
handle value,
bool value_is_nullptr,
const from_python_policies &policies)
: name(name), descr(descr), value(value), policies(policies) {}
: name(name), descr(descr), value(value), value_is_nullptr(value_is_nullptr),
policies(policies) {}
};

/// Internal data structure which holds metadata about a bound function (signature, overloads,
Expand Down Expand Up @@ -474,8 +481,11 @@ inline void check_kw_only_arg(const arg &a, function_record *r) {

inline void append_self_arg_if_needed(function_record *r) {
if (r->is_method && r->args.empty()) {
r->args.emplace_back(
"self", nullptr, handle(), from_python_policies(/*convert=*/true, /*none=*/false));
r->args.emplace_back("self",
nullptr,
handle(),
/*value_is_nullptr=*/false,
from_python_policies(/*convert=*/true, /*none=*/false));
}
}

Expand All @@ -488,6 +498,7 @@ struct process_attribute<arg> : process_attribute_default<arg> {
a.name,
nullptr,
handle(),
/*value_is_nullptr=*/false,
from_python_policies(a.m_policies.rvpp, !a.flag_noconvert, a.flag_none));

check_kw_only_arg(a, r);
Expand All @@ -504,10 +515,11 @@ struct process_attribute<arg_v> : process_attribute_default<arg_v> {
r->args.emplace_back("self",
/*descr=*/nullptr,
/*parent=*/handle(),
/*value_is_nullptr=*/false,
from_python_policies(/*convert=*/true, /*none=*/false));
}

if (!a.value) {
if (!a.value && !a.value_is_nullptr) {
#if defined(PYBIND11_DETAILED_ERROR_MESSAGES)
std::string descr("'");
if (a.name) {
Expand Down Expand Up @@ -537,6 +549,7 @@ struct process_attribute<arg_v> : process_attribute_default<arg_v> {
a.name,
a.descr,
a.value.inc_ref(),
a.value_is_nullptr,
from_python_policies(a.m_policies.rvpp, !a.flag_noconvert, a.flag_none));

check_kw_only_arg(a, r);
Expand Down
13 changes: 13 additions & 0 deletions include/pybind11/cast.h
Original file line number Diff line number Diff line change
Expand Up @@ -1535,6 +1535,8 @@ struct arg : detail::arg_base {
from_python_policies m_policies;
};

struct nullptr_default_arg {};

/// \ingroup annotations
/// Annotation for arguments with values
struct arg_v : arg {
Expand All @@ -1544,6 +1546,16 @@ struct arg_v : arg {
private:
#endif
friend struct arg_base;

arg_v(arg &&base, nullptr_default_arg, const char *descr = nullptr)
: arg(base), value(), value_is_nullptr(true), descr(descr)
#if defined(PYBIND11_DETAILED_ERROR_MESSAGES)
,
type("pybind11::nullptr_default_arg")
#endif
{
}

template <typename T>
arg_v(arg &&base, T &&x, const char *descr = nullptr)
: arg(base), value(reinterpret_steal<object>(detail::make_caster<T>::cast(
Expand Down Expand Up @@ -1587,6 +1599,7 @@ struct arg_v : arg {

/// The default value
object value;
bool value_is_nullptr = false;
/// The (optional) description of the default value
const char *descr;
#if defined(PYBIND11_DETAILED_ERROR_MESSAGES)
Expand Down
2 changes: 1 addition & 1 deletion include/pybind11/pybind11.h
Original file line number Diff line number Diff line change
Expand Up @@ -910,7 +910,7 @@ class cpp_function : public function {
break;
}

if (value) {
if (value || arg_rec.value_is_nullptr) {
// If we're at the py::args index then first insert a stub for it to be
// replaced later
if (func.has_args && call.args.size() == func.nargs_pos) {
Expand Down
24 changes: 24 additions & 0 deletions tests/test_type_caster_pyobject_ptr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -127,4 +127,28 @@ TEST_SUBMODULE(type_caster_pyobject_ptr, m) {
(void) py::cast(*ptr);
}
#endif

// This test exercises functionality (`py::cast<PyObject *>(handle_nullptr)`)
// that is needed indirectly below in py_arg_handle_nullptr.
m.def("pyobject_ptr_from_handle_nullptr", []() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not very clear at first glance how this handle_nullptr logic is used with the rest of the change. Could you elaborate a bit more on this? Also surprising to me is that if a py::handle can already hold a nullptr, then why do we need to add another value_is_nullptr member to the arg struct, which already hold a py::handle member?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes, the information you're looking for was only in a commit message before (pybind11 master PR used to start this work, but closed already):

pybind/pybind11@a3fd84d

I added a comment to explain.

The core idea is to cleanly exercise a critical sub-feature in isolation. It did not need production code changes, but I don't think it is explicitly exercised anywhere else already.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

py::handle handle_nullptr;
if (handle_nullptr.ptr() != nullptr) {
return "UNEXPECTED: handle_nullptr.ptr() != nullptr";
}
auto *pyobject_ptr_from_handle = py::cast<PyObject *>(handle_nullptr);
if (pyobject_ptr_from_handle != nullptr) {
return "UNEXPECTED: pyobject_ptr_from_handle != nullptr";
}
return "SUCCESS";
});

m.def(
"py_arg_handle_nullptr",
[](PyObject *ptr) {
if (ptr == nullptr) {
return "ptr == nullptr";
}
return py::detail::obj_class_name(ptr);
},
py::arg("ptr") = py::nullptr_default_arg());
}
10 changes: 10 additions & 0 deletions tests/test_type_caster_pyobject_ptr.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,3 +102,13 @@ def test_return_list_pyobject_ptr_reference():
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))


def test_pyobject_ptr_from_handle_nullptr():
assert m.pyobject_ptr_from_handle_nullptr() == "SUCCESS"


def test_py_arg_handle_nullptr():
assert m.py_arg_handle_nullptr(None) == "NoneType"
assert m.py_arg_handle_nullptr([]) == "list"
assert m.py_arg_handle_nullptr() == "ptr == nullptr"