From 457798900a0ff0faae5d9634261d6ad79b76a164 Mon Sep 17 00:00:00 2001 From: Eric Cousineau Date: Mon, 3 Aug 2020 15:57:18 -0400 Subject: [PATCH 1/7] pytypes: Add doc section and tests about interaction with None --- docs/advanced/pycpp/object.rst | 64 ++++++++++++++++++++++++++++++++++ tests/test_pytypes.cpp | 18 ++++++++++ tests/test_pytypes.py | 15 ++++++++ 3 files changed, 97 insertions(+) diff --git a/docs/advanced/pycpp/object.rst b/docs/advanced/pycpp/object.rst index 19a226a876..022d9c18e8 100644 --- a/docs/advanced/pycpp/object.rst +++ b/docs/advanced/pycpp/object.rst @@ -13,6 +13,11 @@ Available types include :class:`handle`, :class:`object`, :class:`bool_`, :class:`iterable`, :class:`iterator`, :class:`function`, :class:`buffer`, :class:`array`, and :class:`array_t`. +.. warning:: + + You should be aware of how these classes interact with :func:`py::none`. + See :ref:`pytypes_interaction_with_none` for more details. + Casting back and forth ====================== @@ -168,3 +173,62 @@ Generalized unpacking according to PEP448_ is also supported: Python functions from C++, including keywords arguments and unpacking. .. _PEP448: https://www.python.org/dev/peps/pep-0448/ + +.. _pytypes_interaction_with_none: + +Interaction with None +===================== + +You may be tempted to use types like ``py::str`` and ``py::dict`` in C++ +signatures (either pure C++, or in bound signatures). However, there are some +"gotchas" for ``py::none()`` and how it interacts with these types. In best +case scenarios, it will fail fast (e.g. with default arguments); in worst +cases, it will silently work but corrupt the types you want to work with. + +At a first glance, you may think after executing the following code, the +expression ``my_value.is(py::none())`` will be true: + +.. code-block:: cpp + + py::str my_value = py::none(); + +However, this is not the case. Instead, the value of ``my_value`` will be equal +to the Python value of ``str(None)``, due to how :c:macro:`PYBIND11_OBJECT_CVT` +is used in :file:`pybind11/pytypes.h`. + +Additionally, calling the following binding with the default argument used will +raise a ``TypeError`` about invalid arguments: + +.. code-block:: cpp + + m.def( + "my_function", + [](py::str my_value) { ... }, + py::arg("my_value") = py::none()); + +In both of these cases where you may want to pass ``None`` through any +signatures where you want to constrain the type, you should either use +:class:`py::object` in conjunction with :func:`py::isinstance`, or use the +corresponding C++ type with `std::optional` (if it is available on your +system). + +An example of working around the above edge case for conversion: + +.. code-block:: cpp + + py::object my_value = /* py::none() or some string */; + ... + if (!my_value.is(py::none()) && !py::isinstance(my_value)) { + /* error behavior */ + } + +An example of working around the above edge case for default arguments: + +.. code-block:: cpp + + m.def( + "my_function", + [](std::optional my_value) { ... }, + py::arg("my_value") = std::nullopt); + +For more details, see the tests for ``pytypes`` mentioned above. diff --git a/tests/test_pytypes.cpp b/tests/test_pytypes.cpp index 9f7bc37dc6..bc7f23fd32 100644 --- a/tests/test_pytypes.cpp +++ b/tests/test_pytypes.cpp @@ -319,6 +319,24 @@ TEST_SUBMODULE(pytypes, m) { return a[py::slice(0, -1, 2)]; }); + // See #2361 + // These four tests should reflect the text in `object.rst`, the + // "Interaction with None" section. + m.def("test_str_with_default_arg_none" ,[](py::str value) { + return value; + }, py::arg("value") = py::none()); + m.def("test_str_assign_none", []() { + py::str is_this_none = py::none(); + return is_this_none; + }); + m.def("test_dict_with_default_arg_none", [](py::dict value) { + return value; + }, py::arg("value") = py::none()); + m.def("test_dict_assign_none", []() { + py::dict is_this_none = py::none(); + return is_this_none; + }); + m.def("test_memoryview_object", [](py::buffer b) { return py::memoryview(b); }); diff --git a/tests/test_pytypes.py b/tests/test_pytypes.py index 4cfc707a32..3ce2b5c0a7 100644 --- a/tests/test_pytypes.py +++ b/tests/test_pytypes.py @@ -281,6 +281,21 @@ def test_list_slicing(): assert li[::2] == m.test_list_slicing(li) +def test_pytypes_with_none(): + # See issue #2361 + assert m.test_str_assign_none() == "None" + with pytest.raises(TypeError) as excinfo: + m.test_str_with_default_arg_none() + assert "incompatible function arguments" in str(excinfo.value) + + with pytest.raises(TypeError) as excinfo: + assert m.test_dict_assign_none() + assert "'NoneType' object is not iterable" in str(excinfo.value) + with pytest.raises(TypeError) as excinfo: + m.test_dict_with_default_arg_none() + assert "incompatible function arguments" in str(excinfo.value) + + @pytest.mark.parametrize('method, args, fmt, expected_view', [ (m.test_memoryview_object, (b'red',), 'B', b'red'), (m.test_memoryview_buffer_info, (b'green',), 'B', b'green'), From 20c06b8983fbcea9612cf8e6c3623d23518b6338 Mon Sep 17 00:00:00 2001 From: Eric Cousineau Date: Mon, 3 Aug 2020 20:04:05 -0400 Subject: [PATCH 2/7] draw (hazy) parallels to other value / reference types --- docs/advanced/pycpp/object.rst | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/advanced/pycpp/object.rst b/docs/advanced/pycpp/object.rst index 022d9c18e8..1c5110baf2 100644 --- a/docs/advanced/pycpp/object.rst +++ b/docs/advanced/pycpp/object.rst @@ -185,6 +185,14 @@ signatures (either pure C++, or in bound signatures). However, there are some case scenarios, it will fail fast (e.g. with default arguments); in worst cases, it will silently work but corrupt the types you want to work with. +In general, the pytypes like ``py::str``, ``py::dict``, etc., are +strict **non-nullable** reference types. They may not store a copy when +assigned to, but they cannot store ``None``. For statically typed languages, +this is in contrast Java's ``String`` or ``List``, or C#'s ``string`` or +``List``, which are strict nullable refernce types, and C++'s +``std::string``, which is simply a value type, or +``std::optional``, which is a nullable value type. + At a first glance, you may think after executing the following code, the expression ``my_value.is(py::none())`` will be true: From 664cc9d8701032ddad93bfcb5901ee3faf8afded Mon Sep 17 00:00:00 2001 From: Eric Cousineau Date: Tue, 4 Aug 2020 00:39:49 -0400 Subject: [PATCH 3/7] hazy explanation of uninitialized py::object_api stuff --- docs/advanced/pycpp/object.rst | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/advanced/pycpp/object.rst b/docs/advanced/pycpp/object.rst index 1c5110baf2..c799ef878b 100644 --- a/docs/advanced/pycpp/object.rst +++ b/docs/advanced/pycpp/object.rst @@ -193,6 +193,13 @@ this is in contrast Java's ``String`` or ``List``, or C#'s ``string`` or ``std::string``, which is simply a value type, or ``std::optional``, which is a nullable value type. +.. note:: + + You actually *can* make ``py::str``, ``py::dict``, etc. nullable by + using their default constructors (which effectively means that + ``.ptr() == nullptr``). However, this nullability cannot be "passed" to + Python without explicit intervention. + At a first glance, you may think after executing the following code, the expression ``my_value.is(py::none())`` will be true: From 146131af5601c98f1f985f246eb4d8518849e1ba Mon Sep 17 00:00:00 2001 From: Eric Cousineau Date: Sun, 9 Aug 2020 14:20:25 -0400 Subject: [PATCH 4/7] trim the text --- docs/advanced/pycpp/object.rst | 86 +++++++--------------------------- 1 file changed, 16 insertions(+), 70 deletions(-) diff --git a/docs/advanced/pycpp/object.rst b/docs/advanced/pycpp/object.rst index c799ef878b..379e5556a9 100644 --- a/docs/advanced/pycpp/object.rst +++ b/docs/advanced/pycpp/object.rst @@ -15,8 +15,8 @@ Available types include :class:`handle`, :class:`object`, :class:`bool_`, .. warning:: - You should be aware of how these classes interact with :func:`py::none`. - See :ref:`pytypes_interaction_with_none` for more details. + Be sure to review the :ref:`gotchas` before using this heavily in your C++ + API. Casting back and forth ====================== @@ -174,76 +174,22 @@ Generalized unpacking according to PEP448_ is also supported: .. _PEP448: https://www.python.org/dev/peps/pep-0448/ -.. _pytypes_interaction_with_none: +.. _pytypes_gotchas: -Interaction with None -===================== +Gotchas +======= -You may be tempted to use types like ``py::str`` and ``py::dict`` in C++ -signatures (either pure C++, or in bound signatures). However, there are some -"gotchas" for ``py::none()`` and how it interacts with these types. In best -case scenarios, it will fail fast (e.g. with default arguments); in worst -cases, it will silently work but corrupt the types you want to work with. - -In general, the pytypes like ``py::str``, ``py::dict``, etc., are -strict **non-nullable** reference types. They may not store a copy when -assigned to, but they cannot store ``None``. For statically typed languages, -this is in contrast Java's ``String`` or ``List``, or C#'s ``string`` or -``List``, which are strict nullable refernce types, and C++'s -``std::string``, which is simply a value type, or -``std::optional``, which is a nullable value type. - -.. note:: - - You actually *can* make ``py::str``, ``py::dict``, etc. nullable by - using their default constructors (which effectively means that - ``.ptr() == nullptr``). However, this nullability cannot be "passed" to - Python without explicit intervention. - -At a first glance, you may think after executing the following code, the -expression ``my_value.is(py::none())`` will be true: - -.. code-block:: cpp - - py::str my_value = py::none(); - -However, this is not the case. Instead, the value of ``my_value`` will be equal -to the Python value of ``str(None)``, due to how :c:macro:`PYBIND11_OBJECT_CVT` -is used in :file:`pybind11/pytypes.h`. - -Additionally, calling the following binding with the default argument used will -raise a ``TypeError`` about invalid arguments: - -.. code-block:: cpp - - m.def( - "my_function", - [](py::str my_value) { ... }, - py::arg("my_value") = py::none()); - -In both of these cases where you may want to pass ``None`` through any -signatures where you want to constrain the type, you should either use -:class:`py::object` in conjunction with :func:`py::isinstance`, or use the -corresponding C++ type with `std::optional` (if it is available on your -system). +Default-Constructed Wrappers +---------------------------- -An example of working around the above edge case for conversion: +When a wrapper type is default-constructed, it is **not** a valid Python object (i.e. it is not ``py::none()``). It is simply the same as +``(PyObject*)nullptr``. To check for this, either use ``(bool)my_wrapper`` or +``my_wrapper.ptr() == nullptr``. -.. code-block:: cpp - - py::object my_value = /* py::none() or some string */; - ... - if (!my_value.is(py::none()) && !py::isinstance(my_value)) { - /* error behavior */ - } +Assigning py::none() to wrappers +-------------------------------- -An example of working around the above edge case for default arguments: - -.. code-block:: cpp - - m.def( - "my_function", - [](std::optional my_value) { ... }, - py::arg("my_value") = std::nullopt); - -For more details, see the tests for ``pytypes`` mentioned above. +You may be tempted to use types like ``py::str`` and ``py::dict`` in C++ +signatures (either pure C++, or in bound signatures), and assign them default values of ``py::none()``. However, in a best case scenario, it will fail fast +because ``None`` is not convertible to that type (e.g. ``py::dict``), or in a worse case scenario, it will silently work but corrupt the types you want to +work with (e.g. ``py::str(py::none())`` will yield ``"None"`` in Python). From dc9e30d397e5d9a5095237d3d30af31767725ce1 Mon Sep 17 00:00:00 2001 From: Eric Cousineau Date: Sun, 9 Aug 2020 14:30:17 -0400 Subject: [PATCH 5/7] simplify tests --- tests/test_pytypes.cpp | 12 ++---------- tests/test_pytypes.py | 13 +++---------- 2 files changed, 5 insertions(+), 20 deletions(-) diff --git a/tests/test_pytypes.cpp b/tests/test_pytypes.cpp index bc7f23fd32..82e653c342 100644 --- a/tests/test_pytypes.cpp +++ b/tests/test_pytypes.cpp @@ -320,19 +320,11 @@ TEST_SUBMODULE(pytypes, m) { }); // See #2361 - // These four tests should reflect the text in `object.rst`, the - // "Interaction with None" section. - m.def("test_str_with_default_arg_none" ,[](py::str value) { - return value; - }, py::arg("value") = py::none()); - m.def("test_str_assign_none", []() { + m.def("issue2361_str_implicit_copy_none", []() { py::str is_this_none = py::none(); return is_this_none; }); - m.def("test_dict_with_default_arg_none", [](py::dict value) { - return value; - }, py::arg("value") = py::none()); - m.def("test_dict_assign_none", []() { + m.def("issue2361_dict_implicit_copy_none", []() { py::dict is_this_none = py::none(); return is_this_none; }); diff --git a/tests/test_pytypes.py b/tests/test_pytypes.py index 3ce2b5c0a7..62865692e4 100644 --- a/tests/test_pytypes.py +++ b/tests/test_pytypes.py @@ -281,19 +281,12 @@ def test_list_slicing(): assert li[::2] == m.test_list_slicing(li) -def test_pytypes_with_none(): +def test_issue2361(): # See issue #2361 - assert m.test_str_assign_none() == "None" + assert m.issue2361_str_implicit_copy_none() == "None" with pytest.raises(TypeError) as excinfo: - m.test_str_with_default_arg_none() - assert "incompatible function arguments" in str(excinfo.value) - - with pytest.raises(TypeError) as excinfo: - assert m.test_dict_assign_none() + assert m.issue2361_dict_implicit_copy_none() assert "'NoneType' object is not iterable" in str(excinfo.value) - with pytest.raises(TypeError) as excinfo: - m.test_dict_with_default_arg_none() - assert "incompatible function arguments" in str(excinfo.value) @pytest.mark.parametrize('method, args, fmt, expected_view', [ From 6d68561d7be810ac1e41da57871022b3fd265bd3 Mon Sep 17 00:00:00 2001 From: Eric Cousineau Date: Sun, 9 Aug 2020 15:04:41 -0400 Subject: [PATCH 6/7] fix misspelled secref --- docs/advanced/pycpp/object.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/advanced/pycpp/object.rst b/docs/advanced/pycpp/object.rst index 379e5556a9..085ae1a236 100644 --- a/docs/advanced/pycpp/object.rst +++ b/docs/advanced/pycpp/object.rst @@ -15,8 +15,8 @@ Available types include :class:`handle`, :class:`object`, :class:`bool_`, .. warning:: - Be sure to review the :ref:`gotchas` before using this heavily in your C++ - API. + Be sure to review the :ref:`pytypes_gotchas` before using this heavily in + your C++ API. Casting back and forth ====================== From e9feb0a3d02e8359e204833bb5867abce0277fb8 Mon Sep 17 00:00:00 2001 From: Eric Cousineau Date: Fri, 4 Sep 2020 18:55:40 -0400 Subject: [PATCH 7/7] address review --- docs/advanced/pycpp/object.rst | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/docs/advanced/pycpp/object.rst b/docs/advanced/pycpp/object.rst index 5ce63178cd..c6c3b1b75b 100644 --- a/docs/advanced/pycpp/object.rst +++ b/docs/advanced/pycpp/object.rst @@ -193,13 +193,15 @@ Default-Constructed Wrappers ---------------------------- When a wrapper type is default-constructed, it is **not** a valid Python object (i.e. it is not ``py::none()``). It is simply the same as -``(PyObject*)nullptr``. To check for this, either use ``(bool)my_wrapper`` or -``my_wrapper.ptr() == nullptr``. +``PyObject*`` null pointer. To check for this, use +``static_cast(my_wrapper)``. Assigning py::none() to wrappers -------------------------------- You may be tempted to use types like ``py::str`` and ``py::dict`` in C++ -signatures (either pure C++, or in bound signatures), and assign them default values of ``py::none()``. However, in a best case scenario, it will fail fast -because ``None`` is not convertible to that type (e.g. ``py::dict``), or in a worse case scenario, it will silently work but corrupt the types you want to +signatures (either pure C++, or in bound signatures), and assign them default +values of ``py::none()``. However, in a best case scenario, it will fail fast +because ``None`` is not convertible to that type (e.g. ``py::dict``), or in a +worse case scenario, it will silently work but corrupt the types you want to work with (e.g. ``py::str(py::none())`` will yield ``"None"`` in Python).