From 3c061f216899485399fbb1e6b7732fa0051733f1 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Thu, 27 Aug 2020 17:49:52 -0700 Subject: [PATCH] Fixing `pybind11::bytes()` ambiguous conversion issue. Adding missing `bytes` type to `test_constructors()`, to exercise the code change. The changes in the PR were cherry-picked from PR #2409 (with a very minor modification in test_pytypes.py related to flake8). Via PR #2409, these changes were extensively tested in the Google environment, as summarized here: https://docs.google.com/document/d/1TPL-J__mph_yHa1quDvsO12E_F5OZnvBaZlW9IIrz8M/ The changes in this PR did not cause an issues at all. Note that `test_constructors()` before this PR passes for Python 2 only because `pybind11::str` can hold `PyUnicodeObject` or `PyBytesObject`. As a side-effect of this PR, `test_constructors()` no longer relies on this permissive `pybind11::str` behavior. However, the permissive behavior is still exercised/exposed via the existing `test_pybind11_str_raw_str()`. The test code change is designed to enable easy removal later, when Python 2 support is dropped. For completeness: confusingly, the non-test code changes travelled through PR Example `ambiguous conversion` error fixed by this PR: ``` pybind11/tests/test_pytypes.cpp:214:23: error: ambiguous conversion for functional-style cast from 'pybind11::detail::item_accessor' (aka 'accessor') to 'py::bytes' "bytes"_a=py::bytes(d["bytes"]), ^~~~~~~~~~~~~~~~~~~~ pybind11/include/pybind11/detail/../pytypes.h:957:21: note: candidate constructor PYBIND11_OBJECT(bytes, object, PYBIND11_BYTES_CHECK) ^ pybind11/include/pybind11/detail/../pytypes.h:957:21: note: candidate constructor pybind11/include/pybind11/detail/../pytypes.h:987:15: note: candidate constructor inline bytes::bytes(const pybind11::str &s) { ^ 1 error generated. ``` --- include/pybind11/pytypes.h | 8 ++++---- tests/test_pytypes.cpp | 3 +++ tests/test_pytypes.py | 13 ++++++++++++- 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index e2b63757d..bea34cd93 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -796,7 +796,9 @@ PYBIND11_NAMESPACE_END(detail) Name(handle h, stolen_t) : Parent(h, stolen_t{}) { } \ PYBIND11_DEPRECATED("Use py::isinstance(obj) instead") \ bool check() const { return m_ptr != nullptr && (bool) CheckFun(m_ptr); } \ - static bool check_(handle h) { return h.ptr() != nullptr && CheckFun(h.ptr()); } + static bool check_(handle h) { return h.ptr() != nullptr && CheckFun(h.ptr()); } \ + template \ + Name(const ::pybind11::detail::accessor &a) : Name(object(a)) { } #define PYBIND11_OBJECT_CVT(Name, Parent, CheckFun, ConvertFun) \ PYBIND11_OBJECT_COMMON(Name, Parent, CheckFun) \ @@ -806,9 +808,7 @@ PYBIND11_NAMESPACE_END(detail) { if (!m_ptr) throw error_already_set(); } \ Name(object &&o) \ : Parent(check_(o) ? o.release().ptr() : ConvertFun(o.ptr()), stolen_t{}) \ - { if (!m_ptr) throw error_already_set(); } \ - template \ - Name(const ::pybind11::detail::accessor &a) : Name(object(a)) { } + { if (!m_ptr) throw error_already_set(); } #define PYBIND11_OBJECT(Name, Parent, CheckFun) \ PYBIND11_OBJECT_COMMON(Name, Parent, CheckFun) \ diff --git a/tests/test_pytypes.cpp b/tests/test_pytypes.cpp index 9dae6e7d6..0f8d56410 100644 --- a/tests/test_pytypes.cpp +++ b/tests/test_pytypes.cpp @@ -197,6 +197,7 @@ TEST_SUBMODULE(pytypes, m) { // test_constructors m.def("default_constructors", []() { return py::dict( + "bytes"_a=py::bytes(), "str"_a=py::str(), "bool"_a=py::bool_(), "int"_a=py::int_(), @@ -210,6 +211,7 @@ TEST_SUBMODULE(pytypes, m) { m.def("converting_constructors", [](py::dict d) { return py::dict( + "bytes"_a=py::bytes(d["bytes"]), "str"_a=py::str(d["str"]), "bool"_a=py::bool_(d["bool"]), "int"_a=py::int_(d["int"]), @@ -225,6 +227,7 @@ TEST_SUBMODULE(pytypes, m) { m.def("cast_functions", [](py::dict d) { // When converting between Python types, obj.cast() should be the same as T(obj) return py::dict( + "bytes"_a=d["bytes"].cast(), "str"_a=d["str"].cast(), "bool"_a=d["bool"].cast(), "int"_a=d["int"].cast(), diff --git a/tests/test_pytypes.py b/tests/test_pytypes.py index c21ad6114..95cc94af8 100644 --- a/tests/test_pytypes.py +++ b/tests/test_pytypes.py @@ -190,11 +190,17 @@ def test_accessors(): def test_constructors(): """C++ default and converting constructors are equivalent to type calls in Python""" - types = [str, bool, int, float, tuple, list, dict, set] + types = [bytes, str, bool, int, float, tuple, list, dict, set] expected = {t.__name__: t() for t in types} + if env.PY2: + # Note that bytes.__name__ == 'str' in Python 2. + # pybind11::str is unicode even under Python 2. + expected["bytes"] = bytes() + expected["str"] = unicode() # noqa: F821 assert m.default_constructors() == expected data = { + bytes: b'41', # Currently no supported or working conversions. str: 42, bool: "Not empty", int: "42", @@ -207,6 +213,11 @@ def test_constructors(): } inputs = {k.__name__: v for k, v in data.items()} expected = {k.__name__: k(v) for k, v in data.items()} + if env.PY2: # Similar to the above. See comments above. + inputs["bytes"] = b'41' + inputs["str"] = 42 + expected["bytes"] = b'41' + expected["str"] = u"42" assert m.converting_constructors(inputs) == expected assert m.cast_functions(inputs) == expected