From 794d7020fef3ea0b7e86f3aacfde03a60450f0c2 Mon Sep 17 00:00:00 2001 From: Xiaofei Wang Date: Wed, 30 Mar 2022 16:20:07 -0700 Subject: [PATCH 01/15] Add return_as_bytes policy --- include/pybind11/cast.h | 9 +++++++-- include/pybind11/detail/common.h | 9 ++++++++- tests/test_builtin_casters.cpp | 16 ++++++++++++++++ tests/test_builtin_casters.py | 10 ++++++++++ tests/test_stl.cpp | 19 +++++++++++++++++++ tests/test_stl.py | 12 ++++++++++++ 6 files changed, 72 insertions(+), 3 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 1eac3cba50..5f27300946 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -448,10 +448,15 @@ struct string_caster { } static handle - cast(const StringType &src, return_value_policy /* policy */, handle /* parent */) { + cast(const StringType &src, return_value_policy policy, handle /* parent */) { const char *buffer = reinterpret_cast(src.data()); auto nbytes = ssize_t(src.size() * sizeof(CharT)); - handle s = decode_utfN(buffer, nbytes); + handle s; + if (policy == return_value_policy::return_as_bytes) { + s = PYBIND11_BYTES_FROM_STRING_AND_SIZE(buffer, nbytes); + } else { + s = decode_utfN(buffer, nbytes); + } if (!s) { throw error_already_set(); } diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index ea09bb3fdf..c4611f12d2 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -459,7 +459,14 @@ enum class return_value_policy : uint8_t { collected while Python is still using the child. More advanced variations of this scheme are also possible using combinations of return_value_policy::reference and the keep_alive call policy */ - reference_internal + reference_internal, + + /** Use this policy to make C++ functions return bytes to Python instead of + str. This is useful when C++ function returns nested type of string, + where we cannot apply `py::bytes` easily. Note that when C++ function + returns string, we don't have to deal ownership for sure, because we + are always copying the C++ string to a new Python str/bytes object.*/ + return_as_bytes }; PYBIND11_NAMESPACE_BEGIN(detail) diff --git a/tests/test_builtin_casters.cpp b/tests/test_builtin_casters.cpp index 7b67515e5e..22a59a432c 100644 --- a/tests/test_builtin_casters.cpp +++ b/tests/test_builtin_casters.cpp @@ -378,4 +378,20 @@ TEST_SUBMODULE(builtin_casters, m) { m.def("takes_const_ref", [](const ConstRefCasted &x) { return x.tag; }); m.def("takes_const_ref_wrap", [](std::reference_wrapper x) { return x.get().tag; }); + + // test return_value_policy::return_as_bytes + m.def("invalid_utf8_string_as_bytes", []() { + return std::string("\xba\xd0\xba\xd0"); + }, py::return_value_policy::return_as_bytes); + m.def("invalid_utf8_string_as_str", []() { + return std::string("\xba\xd0\xba\xd0"); + }); + m.def("invalid_utf8_char_array_as_bytes", []() { + return "\xba\xd0\xba\xd0"; + }, py::return_value_policy::return_as_bytes); +#ifdef PYBIND11_HAS_STRING_VIEW + m.def("invalid_utf8_string_view_as_bytes", []() { + return std::string_view("\xba\xd0\xba\xd0"); + }, py::return_value_policy::return_as_bytes); +#endif } diff --git a/tests/test_builtin_casters.py b/tests/test_builtin_casters.py index d38ae68028..fc3b551e0e 100644 --- a/tests/test_builtin_casters.py +++ b/tests/test_builtin_casters.py @@ -524,3 +524,13 @@ def test_const_ref_caster(): assert m.takes_const_ptr(x) == 5 assert m.takes_const_ref(x) == 4 assert m.takes_const_ref_wrap(x) == 4 + + +def test_return_as_bytes_policy(): + expected_return_value = b"\xba\xd0\xba\xd0" + assert m.invalid_utf8_string_as_bytes() == expected_return_value + with pytest.raises(UnicodeDecodeError): + m.invalid_utf8_string_as_str() + assert m.invalid_utf8_char_array_as_bytes() == expected_return_value + if hasattr(m, "has_string_view"): + assert m.invalid_utf8_string_view_as_bytes() == expected_return_value diff --git a/tests/test_stl.cpp b/tests/test_stl.cpp index b56a91953b..4aac63d62b 100644 --- a/tests/test_stl.cpp +++ b/tests/test_stl.cpp @@ -541,4 +541,23 @@ TEST_SUBMODULE(stl, m) { []() { return new std::vector(4513); }, // Without explicitly specifying `take_ownership`, this function leaks. py::return_value_policy::take_ownership); + + // test return_value_policy::return_as_bytes + m.def("invalid_utf8_string_array_as_bytes", []() { + return std::array{"\xba\xd0\xba\xd0"}; + }, py::return_value_policy::return_as_bytes); + m.def("invalid_utf8_string_array_as_str", []() { + return std::array{std::string("\xba\xd0\xba\xd0")}; + }); +#ifdef PYBIND11_HAS_OPTIONAL + m.def("invalid_utf8_optional_string_as_bytes", []() { + return std::optional{"\xba\xd0\xba\xd0"}; + }, py::return_value_policy::return_as_bytes); +#endif + +#ifdef PYBIND11_TEST_VARIANT + m.def("invalid_utf8_variant_string_as_bytes", []() { + return std::variant{"\xba\xd0\xba\xd0"}; + }, py::return_value_policy::return_as_bytes); +#endif } diff --git a/tests/test_stl.py b/tests/test_stl.py index 3dc55230ab..159a0ccc53 100644 --- a/tests/test_stl.py +++ b/tests/test_stl.py @@ -371,3 +371,15 @@ def test_return_vector_bool_raw_ptr(): v = m.return_vector_bool_raw_ptr() assert isinstance(v, list) assert len(v) == 4513 + + +def test_return_as_bytes_policy(): + expected_return_value = b"\xba\xd0\xba\xd0" + assert m.invalid_utf8_string_array_as_bytes() == [expected_return_value] + with pytest.raises(UnicodeDecodeError): + m.invalid_utf8_string_array_as_str() + if hasattr(m, "has_optional"): + assert m.invalid_utf8_optional_string_as_bytes() == expected_return_value + if hasattr(m, "load_variant"): + assert m.invalid_utf8_variant_string_as_bytes() == expected_return_value + From ef062dcbf79cd1a82c2d7e8be3578f535bbf6774 Mon Sep 17 00:00:00 2001 From: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com> Date: Wed, 30 Mar 2022 16:21:56 -0700 Subject: [PATCH 02/15] Fix format --- include/pybind11/cast.h | 7 +++---- tests/test_builtin_casters.cpp | 25 +++++++++++++------------ tests/test_stl.cpp | 26 ++++++++++++++------------ tests/test_stl.py | 1 - 4 files changed, 30 insertions(+), 29 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 5f27300946..cd3b8eab6c 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -447,15 +447,14 @@ struct string_caster { return true; } - static handle - cast(const StringType &src, return_value_policy policy, handle /* parent */) { + static handle cast(const StringType &src, return_value_policy policy, handle /* parent */) { const char *buffer = reinterpret_cast(src.data()); auto nbytes = ssize_t(src.size() * sizeof(CharT)); handle s; if (policy == return_value_policy::return_as_bytes) { - s = PYBIND11_BYTES_FROM_STRING_AND_SIZE(buffer, nbytes); + s = PYBIND11_BYTES_FROM_STRING_AND_SIZE(buffer, nbytes); } else { - s = decode_utfN(buffer, nbytes); + s = decode_utfN(buffer, nbytes); } if (!s) { throw error_already_set(); diff --git a/tests/test_builtin_casters.cpp b/tests/test_builtin_casters.cpp index 22a59a432c..a4353ac6c6 100644 --- a/tests/test_builtin_casters.cpp +++ b/tests/test_builtin_casters.cpp @@ -380,18 +380,19 @@ TEST_SUBMODULE(builtin_casters, m) { [](std::reference_wrapper x) { return x.get().tag; }); // test return_value_policy::return_as_bytes - m.def("invalid_utf8_string_as_bytes", []() { - return std::string("\xba\xd0\xba\xd0"); - }, py::return_value_policy::return_as_bytes); - m.def("invalid_utf8_string_as_str", []() { - return std::string("\xba\xd0\xba\xd0"); - }); - m.def("invalid_utf8_char_array_as_bytes", []() { - return "\xba\xd0\xba\xd0"; - }, py::return_value_policy::return_as_bytes); + m.def( + "invalid_utf8_string_as_bytes", + []() { return std::string("\xba\xd0\xba\xd0"); }, + py::return_value_policy::return_as_bytes); + m.def("invalid_utf8_string_as_str", []() { return std::string("\xba\xd0\xba\xd0"); }); + m.def( + "invalid_utf8_char_array_as_bytes", + []() { return "\xba\xd0\xba\xd0"; }, + py::return_value_policy::return_as_bytes); #ifdef PYBIND11_HAS_STRING_VIEW - m.def("invalid_utf8_string_view_as_bytes", []() { - return std::string_view("\xba\xd0\xba\xd0"); - }, py::return_value_policy::return_as_bytes); + m.def( + "invalid_utf8_string_view_as_bytes", + []() { return std::string_view("\xba\xd0\xba\xd0"); }, + py::return_value_policy::return_as_bytes); #endif } diff --git a/tests/test_stl.cpp b/tests/test_stl.cpp index 4aac63d62b..a739d217df 100644 --- a/tests/test_stl.cpp +++ b/tests/test_stl.cpp @@ -543,21 +543,23 @@ TEST_SUBMODULE(stl, m) { py::return_value_policy::take_ownership); // test return_value_policy::return_as_bytes - m.def("invalid_utf8_string_array_as_bytes", []() { - return std::array{"\xba\xd0\xba\xd0"}; - }, py::return_value_policy::return_as_bytes); - m.def("invalid_utf8_string_array_as_str", []() { - return std::array{std::string("\xba\xd0\xba\xd0")}; - }); + m.def( + "invalid_utf8_string_array_as_bytes", + []() { return std::array{"\xba\xd0\xba\xd0"}; }, + py::return_value_policy::return_as_bytes); + m.def("invalid_utf8_string_array_as_str", + []() { return std::array{std::string("\xba\xd0\xba\xd0")}; }); #ifdef PYBIND11_HAS_OPTIONAL - m.def("invalid_utf8_optional_string_as_bytes", []() { - return std::optional{"\xba\xd0\xba\xd0"}; - }, py::return_value_policy::return_as_bytes); + m.def( + "invalid_utf8_optional_string_as_bytes", + []() { return std::optional{"\xba\xd0\xba\xd0"}; }, + py::return_value_policy::return_as_bytes); #endif #ifdef PYBIND11_TEST_VARIANT - m.def("invalid_utf8_variant_string_as_bytes", []() { - return std::variant{"\xba\xd0\xba\xd0"}; - }, py::return_value_policy::return_as_bytes); + m.def( + "invalid_utf8_variant_string_as_bytes", + []() { return std::variant{"\xba\xd0\xba\xd0"}; }, + py::return_value_policy::return_as_bytes); #endif } diff --git a/tests/test_stl.py b/tests/test_stl.py index 159a0ccc53..382ab9b403 100644 --- a/tests/test_stl.py +++ b/tests/test_stl.py @@ -382,4 +382,3 @@ def test_return_as_bytes_policy(): assert m.invalid_utf8_optional_string_as_bytes() == expected_return_value if hasattr(m, "load_variant"): assert m.invalid_utf8_variant_string_as_bytes() == expected_return_value - From 6e46b26837f29101648105d0558e9aedff8a8b68 Mon Sep 17 00:00:00 2001 From: Xiaofei Wang Date: Wed, 30 Mar 2022 16:41:08 -0700 Subject: [PATCH 03/15] Fix test failures --- tests/test_stl.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_stl.cpp b/tests/test_stl.cpp index a739d217df..5e7ecd5390 100644 --- a/tests/test_stl.cpp +++ b/tests/test_stl.cpp @@ -545,10 +545,10 @@ TEST_SUBMODULE(stl, m) { // test return_value_policy::return_as_bytes m.def( "invalid_utf8_string_array_as_bytes", - []() { return std::array{"\xba\xd0\xba\xd0"}; }, + []() { return std::array{{"\xba\xd0\xba\xd0"}} }, py::return_value_policy::return_as_bytes); m.def("invalid_utf8_string_array_as_str", - []() { return std::array{std::string("\xba\xd0\xba\xd0")}; }); + []() { return std::array{{"\xba\xd0\xba\xd0"}}; }); #ifdef PYBIND11_HAS_OPTIONAL m.def( "invalid_utf8_optional_string_as_bytes", From 1c1d5957ca678fb3a0e1af2409abe7b306f1ff1e Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 30 Mar 2022 23:41:45 +0000 Subject: [PATCH 04/15] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/test_stl.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/test_stl.cpp b/tests/test_stl.cpp index 5e7ecd5390..2acd5debd4 100644 --- a/tests/test_stl.cpp +++ b/tests/test_stl.cpp @@ -545,7 +545,11 @@ TEST_SUBMODULE(stl, m) { // test return_value_policy::return_as_bytes m.def( "invalid_utf8_string_array_as_bytes", - []() { return std::array{{"\xba\xd0\xba\xd0"}} }, + []() { + return std::array { + { "\xba\xd0\xba\xd0" } + } + }, py::return_value_policy::return_as_bytes); m.def("invalid_utf8_string_array_as_str", []() { return std::array{{"\xba\xd0\xba\xd0"}}; }); From f6ca6f1a48ee253ef6f70f07f3ddaa758eedadd3 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 30 Mar 2022 23:48:47 +0000 Subject: [PATCH 05/15] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/test_stl.cpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tests/test_stl.cpp b/tests/test_stl.cpp index 2acd5debd4..642e476bd1 100644 --- a/tests/test_stl.cpp +++ b/tests/test_stl.cpp @@ -545,11 +545,7 @@ TEST_SUBMODULE(stl, m) { // test return_value_policy::return_as_bytes m.def( "invalid_utf8_string_array_as_bytes", - []() { - return std::array { - { "\xba\xd0\xba\xd0" } - } - }, + []() { return std::array{{"\xba\xd0\xba\xd0"}}; }, py::return_value_policy::return_as_bytes); m.def("invalid_utf8_string_array_as_str", []() { return std::array{{"\xba\xd0\xba\xd0"}}; }); From 3a704039bf2a9196ca8f254c3e813ce14dc767d5 Mon Sep 17 00:00:00 2001 From: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com> Date: Wed, 30 Mar 2022 16:57:09 -0700 Subject: [PATCH 06/15] Fix std::variant --- tests/test_stl.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_stl.cpp b/tests/test_stl.cpp index 642e476bd1..58744d8d95 100644 --- a/tests/test_stl.cpp +++ b/tests/test_stl.cpp @@ -559,7 +559,7 @@ TEST_SUBMODULE(stl, m) { #ifdef PYBIND11_TEST_VARIANT m.def( "invalid_utf8_variant_string_as_bytes", - []() { return std::variant{"\xba\xd0\xba\xd0"}; }, + []() { return variant{"\xba\xd0\xba\xd0"}; }, py::return_value_policy::return_as_bytes); #endif } From 435bc76a0083c29d625f01b658518a6588b5b65a Mon Sep 17 00:00:00 2001 From: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com> Date: Thu, 31 Mar 2022 13:56:31 -0700 Subject: [PATCH 07/15] Resolve comments --- include/pybind11/cast.h | 2 +- include/pybind11/detail/common.h | 12 +++++++----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index cd3b8eab6c..df3cf9d059 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -452,7 +452,7 @@ struct string_caster { auto nbytes = ssize_t(src.size() * sizeof(CharT)); handle s; if (policy == return_value_policy::return_as_bytes) { - s = PYBIND11_BYTES_FROM_STRING_AND_SIZE(buffer, nbytes); + s = PyBytes_FromStringAndSize(buffer, nbytes); } else { s = decode_utfN(buffer, nbytes); } diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index c4611f12d2..60d62ff156 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -461,11 +461,13 @@ enum class return_value_policy : uint8_t { return_value_policy::reference and the keep_alive call policy */ reference_internal, - /** Use this policy to make C++ functions return bytes to Python instead of - str. This is useful when C++ function returns nested type of string, - where we cannot apply `py::bytes` easily. Note that when C++ function - returns string, we don't have to deal ownership for sure, because we - are always copying the C++ string to a new Python str/bytes object.*/ + /** With this policy, C++ string types are converted to Python bytes, + instead of str. This is most useful when a C++ function returns a + container-like type with nested C++ string types, and py::bytes cannot + be applied easily. Note that this return_value_policy is not concerned + with lifetime/ownership semantics, like the other policies, but the + purpose of return_as_bytes is certain to be orthogonal, because C++ + strings are always copied to Python bytes or str. */ return_as_bytes }; From 0ce611e17074cf1de9e4da7540c5a4ca5188e94c Mon Sep 17 00:00:00 2001 From: Xiaofei Wang Date: Thu, 31 Mar 2022 16:59:33 -0700 Subject: [PATCH 08/15] Note this policy experimental --- include/pybind11/detail/common.h | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index 60d62ff156..efa76eb2b5 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -461,13 +461,16 @@ enum class return_value_policy : uint8_t { return_value_policy::reference and the keep_alive call policy */ reference_internal, - /** With this policy, C++ string types are converted to Python bytes, + /** This policy is experimental and likely to change in the future. With + this policy, C++ string types are converted to Python bytes, instead of str. This is most useful when a C++ function returns a container-like type with nested C++ string types, and py::bytes cannot - be applied easily. Note that this return_value_policy is not concerned - with lifetime/ownership semantics, like the other policies, but the - purpose of return_as_bytes is certain to be orthogonal, because C++ - strings are always copied to Python bytes or str. */ + be applied easily. Dictionary like types might not work, for example, + `Dict[str, bytes]`, because this policy forces all string return values + to be converted to bytes. Note that this return_value_policy is not + concerned with lifetime/ownership semantics, like the other policies, + but the purpose of return_as_bytes is certain to be orthogonal, because + C++ strings are always copied to Python bytes or str. */ return_as_bytes }; From 54a640cf372ce43c29d53cc7d2020d19b1e743d9 Mon Sep 17 00:00:00 2001 From: Xiaofei Wang <6218006+wangxf123456@users.noreply.github.com> Date: Mon, 4 Apr 2022 15:29:45 -0700 Subject: [PATCH 09/15] Add tests for `return_as_bytes` with `def_property`. --- tests/test_builtin_casters.cpp | 11 +++++++++++ tests/test_builtin_casters.py | 4 ++++ 2 files changed, 15 insertions(+) diff --git a/tests/test_builtin_casters.cpp b/tests/test_builtin_casters.cpp index a4353ac6c6..5478789579 100644 --- a/tests/test_builtin_casters.cpp +++ b/tests/test_builtin_casters.cpp @@ -15,6 +15,11 @@ struct ConstRefCasted { int tag; }; +struct StringAttr { + StringAttr(std::string v) : value(v) {} + std::string value; +}; + PYBIND11_NAMESPACE_BEGIN(pybind11) PYBIND11_NAMESPACE_BEGIN(detail) template <> @@ -389,6 +394,12 @@ TEST_SUBMODULE(builtin_casters, m) { "invalid_utf8_char_array_as_bytes", []() { return "\xba\xd0\xba\xd0"; }, py::return_value_policy::return_as_bytes); + py::class_(m, "StringAttr") + .def(py::init()) + .def_property("value", + py::cpp_function([](StringAttr &self) { return self.value; }, + py::return_value_policy::return_as_bytes), + py::cpp_function([](StringAttr &self, std::string v) { self.value = v; })); #ifdef PYBIND11_HAS_STRING_VIEW m.def( "invalid_utf8_string_view_as_bytes", diff --git a/tests/test_builtin_casters.py b/tests/test_builtin_casters.py index fc3b551e0e..b2a6ccb6be 100644 --- a/tests/test_builtin_casters.py +++ b/tests/test_builtin_casters.py @@ -532,5 +532,9 @@ def test_return_as_bytes_policy(): with pytest.raises(UnicodeDecodeError): m.invalid_utf8_string_as_str() assert m.invalid_utf8_char_array_as_bytes() == expected_return_value + obj = m.StringAttr(expected_return_value) + assert obj.value == expected_return_value + obj.value = "123" + assert obj.value == b"123" if hasattr(m, "has_string_view"): assert m.invalid_utf8_string_view_as_bytes() == expected_return_value From 4f81655de88ceab5cb86e46287a0eeb373fd1dac Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 15 Apr 2022 07:37:47 -0700 Subject: [PATCH 10/15] Change comment for the new return_as_bytes enum to note that the policy is not available on master. --- include/pybind11/detail/common.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index efa76eb2b5..aa2aa3db46 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -461,8 +461,7 @@ enum class return_value_policy : uint8_t { return_value_policy::reference and the keep_alive call policy */ reference_internal, - /** This policy is experimental and likely to change in the future. With - this policy, C++ string types are converted to Python bytes, + /** With this policy, C++ string types are converted to Python bytes, instead of str. This is most useful when a C++ function returns a container-like type with nested C++ string types, and py::bytes cannot be applied easily. Dictionary like types might not work, for example, @@ -470,7 +469,8 @@ enum class return_value_policy : uint8_t { to be converted to bytes. Note that this return_value_policy is not concerned with lifetime/ownership semantics, like the other policies, but the purpose of return_as_bytes is certain to be orthogonal, because - C++ strings are always copied to Python bytes or str. */ + C++ strings are always copied to Python `bytes` or `str`. + NOTE: This policy is NOT available on master. */ return_as_bytes }; From b06f71b3c97fa0afee3e0e2ab6f11788eded650d Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 15 Apr 2022 07:41:25 -0700 Subject: [PATCH 11/15] Applying pr3838_sh.patch (exactly as used Google-internally since 2022-03-31). --- include/pybind11/detail/smart_holder_type_casters.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 4773abf787..966e6e3ce6 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -790,6 +790,8 @@ struct smart_holder_type_caster> : smart_holder_type_caster_l throw cast_error("Invalid return_value_policy for shared_ptr (reference)."); case return_value_policy::reference_internal: break; + case return_value_policy::return_as_bytes: + break; } if (!src) { return none().release(); From 53663943afd3046ca21eeffefbb954cef6fe35f6 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 15 Apr 2022 07:53:25 -0700 Subject: [PATCH 12/15] Add `case return_as_bytes` to `switch`es in detail/type_caster_base.h and eigen.h Based on systematic review under https://github.com/pybind/pybind11/pull/3838#issuecomment-1094390333 --- include/pybind11/detail/type_caster_base.h | 3 +++ include/pybind11/eigen.h | 2 ++ 2 files changed, 5 insertions(+) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 5b2ec8e1e4..3cac9b2ee3 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -651,6 +651,9 @@ class type_caster_generic { keep_alive_impl(inst, parent); break; + case return_value_policy::return_as_bytes: + pybind11_fail("return_value_policy::return_as_bytes does not apply."); + default: throw cast_error("unhandled return_value_policy: should not happen!"); } diff --git a/include/pybind11/eigen.h b/include/pybind11/eigen.h index f658168ded..d39a6b6cbf 100644 --- a/include/pybind11/eigen.h +++ b/include/pybind11/eigen.h @@ -349,6 +349,8 @@ struct type_caster::value>> { return eigen_ref_array(*src); case return_value_policy::reference_internal: return eigen_ref_array(*src, parent); + case return_value_policy::return_as_bytes: + pybind11_fail("return_value_policy::return_as_bytes does not apply."); default: throw cast_error("unhandled return_value_policy: should not happen!"); }; From 186f477091841ba00d885a50492f40f0b393fa8b Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 15 Apr 2022 08:13:15 -0700 Subject: [PATCH 13/15] Add missing break (clang-tidy). --- include/pybind11/detail/type_caster_base.h | 1 + include/pybind11/eigen.h | 1 + 2 files changed, 2 insertions(+) diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 3cac9b2ee3..ddb9e5cf27 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -653,6 +653,7 @@ class type_caster_generic { case return_value_policy::return_as_bytes: pybind11_fail("return_value_policy::return_as_bytes does not apply."); + break; default: throw cast_error("unhandled return_value_policy: should not happen!"); diff --git a/include/pybind11/eigen.h b/include/pybind11/eigen.h index d39a6b6cbf..3b09b750d4 100644 --- a/include/pybind11/eigen.h +++ b/include/pybind11/eigen.h @@ -351,6 +351,7 @@ struct type_caster::value>> { return eigen_ref_array(*src, parent); case return_value_policy::return_as_bytes: pybind11_fail("return_value_policy::return_as_bytes does not apply."); + break; default: throw cast_error("unhandled return_value_policy: should not happen!"); }; From a21741f3262433ac72c6327fe1855a865c6924f6 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 15 Apr 2022 08:51:41 -0700 Subject: [PATCH 14/15] More clang-tidy fixes (this time around clang-tidy was run interactively to pre-empt repeat trips through the CI). --- include/pybind11/detail/smart_holder_type_casters.h | 3 ++- tests/test_builtin_casters.cpp | 13 ++++++++----- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 966e6e3ce6..2f46f1ac19 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -783,13 +783,14 @@ struct smart_holder_type_caster> : smart_holder_type_caster_l break; case return_value_policy::take_ownership: throw cast_error("Invalid return_value_policy for shared_ptr (take_ownership)."); + break; case return_value_policy::copy: case return_value_policy::move: break; case return_value_policy::reference: throw cast_error("Invalid return_value_policy for shared_ptr (reference)."); - case return_value_policy::reference_internal: break; + case return_value_policy::reference_internal: case return_value_policy::return_as_bytes: break; } diff --git a/tests/test_builtin_casters.cpp b/tests/test_builtin_casters.cpp index 5478789579..79c7bde07f 100644 --- a/tests/test_builtin_casters.cpp +++ b/tests/test_builtin_casters.cpp @@ -11,12 +11,14 @@ #include "pybind11_tests.h" +#include + struct ConstRefCasted { int tag; }; struct StringAttr { - StringAttr(std::string v) : value(v) {} + explicit StringAttr(std::string v) : value(std::move(v)) {} std::string value; }; @@ -396,10 +398,11 @@ TEST_SUBMODULE(builtin_casters, m) { py::return_value_policy::return_as_bytes); py::class_(m, "StringAttr") .def(py::init()) - .def_property("value", - py::cpp_function([](StringAttr &self) { return self.value; }, - py::return_value_policy::return_as_bytes), - py::cpp_function([](StringAttr &self, std::string v) { self.value = v; })); + .def_property( + "value", + py::cpp_function([](StringAttr &self) { return self.value; }, + py::return_value_policy::return_as_bytes), + py::cpp_function([](StringAttr &self, std::string v) { self.value = std::move(v); })); #ifdef PYBIND11_HAS_STRING_VIEW m.def( "invalid_utf8_string_view_as_bytes", From 4397b87b01e46c2b59a00d17d4382fe1ab7c47ec Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Fri, 15 Apr 2022 08:58:56 -0700 Subject: [PATCH 15/15] Underscore prefix: _return_as_bytes --- include/pybind11/cast.h | 2 +- include/pybind11/detail/common.h | 6 +++--- include/pybind11/detail/smart_holder_type_casters.h | 2 +- include/pybind11/detail/type_caster_base.h | 4 ++-- include/pybind11/eigen.h | 4 ++-- tests/test_builtin_casters.cpp | 10 +++++----- tests/test_stl.cpp | 8 ++++---- 7 files changed, 18 insertions(+), 18 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index df3cf9d059..d3e0e4c5bd 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -451,7 +451,7 @@ struct string_caster { const char *buffer = reinterpret_cast(src.data()); auto nbytes = ssize_t(src.size() * sizeof(CharT)); handle s; - if (policy == return_value_policy::return_as_bytes) { + if (policy == return_value_policy::_return_as_bytes) { s = PyBytes_FromStringAndSize(buffer, nbytes); } else { s = decode_utfN(buffer, nbytes); diff --git a/include/pybind11/detail/common.h b/include/pybind11/detail/common.h index aa2aa3db46..67ba322f60 100644 --- a/include/pybind11/detail/common.h +++ b/include/pybind11/detail/common.h @@ -463,15 +463,15 @@ enum class return_value_policy : uint8_t { /** With this policy, C++ string types are converted to Python bytes, instead of str. This is most useful when a C++ function returns a - container-like type with nested C++ string types, and py::bytes cannot + container-like type with nested C++ string types, and `py::bytes` cannot be applied easily. Dictionary like types might not work, for example, `Dict[str, bytes]`, because this policy forces all string return values to be converted to bytes. Note that this return_value_policy is not concerned with lifetime/ownership semantics, like the other policies, - but the purpose of return_as_bytes is certain to be orthogonal, because + but the purpose of _return_as_bytes is certain to be orthogonal, because C++ strings are always copied to Python `bytes` or `str`. NOTE: This policy is NOT available on master. */ - return_as_bytes + _return_as_bytes }; PYBIND11_NAMESPACE_BEGIN(detail) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 2f46f1ac19..67bd237add 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -791,7 +791,7 @@ struct smart_holder_type_caster> : smart_holder_type_caster_l throw cast_error("Invalid return_value_policy for shared_ptr (reference)."); break; case return_value_policy::reference_internal: - case return_value_policy::return_as_bytes: + case return_value_policy::_return_as_bytes: break; } if (!src) { diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index ddb9e5cf27..9d89442182 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -651,8 +651,8 @@ class type_caster_generic { keep_alive_impl(inst, parent); break; - case return_value_policy::return_as_bytes: - pybind11_fail("return_value_policy::return_as_bytes does not apply."); + case return_value_policy::_return_as_bytes: + pybind11_fail("return_value_policy::_return_as_bytes does not apply."); break; default: diff --git a/include/pybind11/eigen.h b/include/pybind11/eigen.h index 3b09b750d4..beb50266e4 100644 --- a/include/pybind11/eigen.h +++ b/include/pybind11/eigen.h @@ -349,8 +349,8 @@ struct type_caster::value>> { return eigen_ref_array(*src); case return_value_policy::reference_internal: return eigen_ref_array(*src, parent); - case return_value_policy::return_as_bytes: - pybind11_fail("return_value_policy::return_as_bytes does not apply."); + case return_value_policy::_return_as_bytes: + pybind11_fail("return_value_policy::_return_as_bytes does not apply."); break; default: throw cast_error("unhandled return_value_policy: should not happen!"); diff --git a/tests/test_builtin_casters.cpp b/tests/test_builtin_casters.cpp index 79c7bde07f..7e629ff8d3 100644 --- a/tests/test_builtin_casters.cpp +++ b/tests/test_builtin_casters.cpp @@ -386,27 +386,27 @@ TEST_SUBMODULE(builtin_casters, m) { m.def("takes_const_ref_wrap", [](std::reference_wrapper x) { return x.get().tag; }); - // test return_value_policy::return_as_bytes + // test return_value_policy::_return_as_bytes m.def( "invalid_utf8_string_as_bytes", []() { return std::string("\xba\xd0\xba\xd0"); }, - py::return_value_policy::return_as_bytes); + py::return_value_policy::_return_as_bytes); m.def("invalid_utf8_string_as_str", []() { return std::string("\xba\xd0\xba\xd0"); }); m.def( "invalid_utf8_char_array_as_bytes", []() { return "\xba\xd0\xba\xd0"; }, - py::return_value_policy::return_as_bytes); + py::return_value_policy::_return_as_bytes); py::class_(m, "StringAttr") .def(py::init()) .def_property( "value", py::cpp_function([](StringAttr &self) { return self.value; }, - py::return_value_policy::return_as_bytes), + py::return_value_policy::_return_as_bytes), py::cpp_function([](StringAttr &self, std::string v) { self.value = std::move(v); })); #ifdef PYBIND11_HAS_STRING_VIEW m.def( "invalid_utf8_string_view_as_bytes", []() { return std::string_view("\xba\xd0\xba\xd0"); }, - py::return_value_policy::return_as_bytes); + py::return_value_policy::_return_as_bytes); #endif } diff --git a/tests/test_stl.cpp b/tests/test_stl.cpp index 58744d8d95..b8bfaaa0a8 100644 --- a/tests/test_stl.cpp +++ b/tests/test_stl.cpp @@ -542,24 +542,24 @@ TEST_SUBMODULE(stl, m) { // Without explicitly specifying `take_ownership`, this function leaks. py::return_value_policy::take_ownership); - // test return_value_policy::return_as_bytes + // test return_value_policy::_return_as_bytes m.def( "invalid_utf8_string_array_as_bytes", []() { return std::array{{"\xba\xd0\xba\xd0"}}; }, - py::return_value_policy::return_as_bytes); + py::return_value_policy::_return_as_bytes); m.def("invalid_utf8_string_array_as_str", []() { return std::array{{"\xba\xd0\xba\xd0"}}; }); #ifdef PYBIND11_HAS_OPTIONAL m.def( "invalid_utf8_optional_string_as_bytes", []() { return std::optional{"\xba\xd0\xba\xd0"}; }, - py::return_value_policy::return_as_bytes); + py::return_value_policy::_return_as_bytes); #endif #ifdef PYBIND11_TEST_VARIANT m.def( "invalid_utf8_variant_string_as_bytes", []() { return variant{"\xba\xd0\xba\xd0"}; }, - py::return_value_policy::return_as_bytes); + py::return_value_policy::_return_as_bytes); #endif }