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.

Sign up for GitHub

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

fix: Reject keyword argument None with .none(false) #2611

Merged
merged 5 commits into from
Oct 20, 2020
Merged

fix: Reject keyword argument None with .none(false) #2611

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
77f31fc
demo kwarg with none(false)
potpath Oct 20, 2020
ae32040
Reorder and extend tests for arg::none(false) in test_methods_and_att…
YannickJadoul Oct 20, 2020
6f057f3
Fix arg::none() for keyword arguments
YannickJadoul Oct 20, 2020
7e3cf90
Add changelog note
YannickJadoul Oct 20, 2020
e437463
Fix names of no_none_kw test functions
YannickJadoul Oct 20, 2020
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
26 changes: 15 additions & 11 deletions include/pybind11/pybind11.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -606,15 +606,15 @@ class cpp_function : public function {
// 1.5. Fill in any missing pos_only args from defaults if they exist
if (args_copied < func.nargs_pos_only) {
for (; args_copied < func.nargs_pos_only; ++args_copied) {
const auto &arg = func.args[args_copied];
const auto &arg_rec = func.args[args_copied];
handle value;

if (arg.value) {
value = arg.value;
if (arg_rec.value) {
value = arg_rec.value;
}
if (value) {
call.args.push_back(value);
call.args_convert.push_back(arg.convert);
call.args_convert.push_back(arg_rec.convert);
} else
break;
}
Expand All @@ -628,26 +628,30 @@ class cpp_function : public function {
bool copied_kwargs = false;

for (; args_copied < num_args; ++args_copied) {
const auto &arg = func.args[args_copied];
const auto &arg_rec = func.args[args_copied];
Copy link
Collaborator

Choose a reason for hiding this comment

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

This rename is, strictly speaking, unrelated, but to increase conformity with the naming in step // 1. above, and to more clearly indicate the type, I renamed it.


handle value;
if (kwargs_in && arg.name)
value = PyDict_GetItemString(kwargs.ptr(), arg.name);
if (kwargs_in && arg_rec.name)
value = PyDict_GetItemString(kwargs.ptr(), arg_rec.name);

if (value) {
// Consume a kwargs value
if (!copied_kwargs) {
kwargs = reinterpret_steal<dict>(PyDict_Copy(kwargs.ptr()));
copied_kwargs = true;
}
PyDict_DelItemString(kwargs.ptr(), arg.name);
} else if (arg.value) {
value = arg.value;
PyDict_DelItemString(kwargs.ptr(), arg_rec.name);
} else if (arg_rec.value) {
value = arg_rec.value;
}

if (!arg_rec.none && value.is_none()) {
break;
}

if (value) {
call.args.push_back(value);
call.args_convert.push_back(arg.convert);
call.args_convert.push_back(arg_rec.convert);
}
else
break;
Expand Down
3 changes: 3 additions & 0 deletions tests/test_methods_and_attributes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,9 @@ TEST_SUBMODULE(methods_and_attributes, m) {
m.def("ok_none4", &none4, py::arg().none(true));
m.def("ok_none5", &none5);

m.def("no_none_kw", &none5, py::arg("a").none(false));
m.def("no_none_kw_only", &none5, py::kw_only(), py::arg("a").none(false));

// test_str_issue
// Issue #283: __str__ called on uninitialized instance when constructor arguments invalid
py::class_<StrIssue>(m, "StrIssue")
Expand Down
13 changes: 13 additions & 0 deletions tests/test_methods_and_attributes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,19 @@ def test_accepts_none(msg):
assert m.ok_none4(None) == -1
assert m.ok_none5(None) == -1

with pytest.raises(TypeError) as excinfo:
m.no_none_kw(None)
Copy link
Collaborator

Choose a reason for hiding this comment

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

no_none_kw seems to suggest that one keyword arguments with None as the value are forbidden. At least that's how I'm reading it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also after the others, no_none{1..5} and ok_none{1..5}?
Any suggestions? no_none_as_kw?

Copy link
Collaborator

Choose a reason for hiding this comment

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

no_none sounds good.

assert "incompatible function arguments" in str(excinfo.value)
with pytest.raises(TypeError) as excinfo:
m.no_none_kw(a=None)
assert "incompatible function arguments" in str(excinfo.value)
with pytest.raises(TypeError) as excinfo:
m.no_none_kw_only(None)
assert "incompatible function arguments" in str(excinfo.value)
with pytest.raises(TypeError) as excinfo:
m.no_none_kw_only(a=None)
assert "incompatible function arguments" in str(excinfo.value)


def test_str_issue(msg):
"""#283: __str__ called on uninitialized instance when constructor arguments invalid"""
Expand Down