From 6edd0e6d9038aa339e20b1293b4a2b555183c0d1 Mon Sep 17 00:00:00 2001 From: Mana Borwornpadungkitti Date: Tue, 20 Oct 2020 17:57:22 -0400 Subject: [PATCH] fix: Reject keyword argument `None` with `.none(false)` (#2611) * demo kwarg with none(false) * Reorder and extend tests for arg::none(false) in test_methods_and_attributes.py::test_accepts_none * Fix arg::none() for keyword arguments * Add changelog note * Fix names of no_none_kw test functions Co-authored-by: Yannick Jadoul --- docs/changelog.rst | 3 +++ include/pybind11/pybind11.h | 26 +++++++++++++++----------- tests/test_methods_and_attributes.cpp | 3 +++ tests/test_methods_and_attributes.py | 13 +++++++++++++ 4 files changed, 34 insertions(+), 11 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index c0f4273e8..952550500 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -204,6 +204,9 @@ Smaller or developer focused features and fixes: * Avoid a segfault on some compilers when types are removed in Python. `#2564 `_ +* ``py::arg::none()`` is now also respected when passing keyword arguments. + `#2611 `_ + * PyPy fixes, PyPy 7.3.x now supported, including PyPy3. (Known issue with PyPy2 and Windows `#2596 `_). `#2146 `_ diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 51fb74a57..c63e7db73 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -606,15 +606,15 @@ protected: // 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; } @@ -628,11 +628,11 @@ protected: 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]; 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 @@ -640,14 +640,18 @@ protected: kwargs = reinterpret_steal(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; diff --git a/tests/test_methods_and_attributes.cpp b/tests/test_methods_and_attributes.cpp index 8febefcdb..6a2cfb6f7 100644 --- a/tests/test_methods_and_attributes.cpp +++ b/tests/test_methods_and_attributes.cpp @@ -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_kwarg", &none2, py::arg("a").none(false)); + m.def("no_none_kwarg_kw_only", &none2, py::kw_only(), py::arg("a").none(false)); + // test_str_issue // Issue #283: __str__ called on uninitialized instance when constructor arguments invalid py::class_(m, "StrIssue") diff --git a/tests/test_methods_and_attributes.py b/tests/test_methods_and_attributes.py index 3e0e6cf05..db135ccec 100644 --- a/tests/test_methods_and_attributes.py +++ b/tests/test_methods_and_attributes.py @@ -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_kwarg(None) + assert "incompatible function arguments" in str(excinfo.value) + with pytest.raises(TypeError) as excinfo: + m.no_none_kwarg(a=None) + assert "incompatible function arguments" in str(excinfo.value) + with pytest.raises(TypeError) as excinfo: + m.no_none_kwarg_kw_only(None) + assert "incompatible function arguments" in str(excinfo.value) + with pytest.raises(TypeError) as excinfo: + m.no_none_kwarg_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"""