Reducing from two to only one macro: PYBIND11_STR_LEGACY_PERMISSIVE

Author: rwgk

docs/changelog.rst

@@ -6,6 +6,15 @@ Changelog
 Starting with version 1.8.0, pybind11 releases use a `semantic versioning
 <http://semver.org>`_ policy.
 
+v2.7.0 (WIP)
+------------------------------
+
+* ``py::str`` changed to exclusively hold `PyUnicodeObject`. Previously
+  ``py::str`` could also hold `bytes`, which is probably surprising, was
+  never documented, and can mask bugs (e.g. accidental use of ``py::str``
+  instead of ``py::bytes``).
+  `#2409 <https://github.com/pybind/pybind11/pull/2409>`_
+
 
 v2.6.2 (TBA, not yet released)
 ------------------------------

docs/upgrade.rst

@@ -10,6 +10,31 @@ modernization and other useful information.
 
 .. _upgrade-guide-2.6:
 
+v2.7
+====
+
+*Before* v2.7, ``py::str`` can hold ``PyUnicodeObject`` or ``PyBytesObject``,
+and ``py::isinstance<str>()`` is ``true`` for both ``py::str`` and
+``py::bytes``. Starting with v2.7, ``py::str`` exclusively holds
+``PyUnicodeObject`` (`#2409 <https://github.com/pybind/pybind11/pull/2409>`_),
+and ``py::isinstance<str>()`` is ``true`` only for ``py::str``. To help in
+the transition of client code, the ``PYBIND11_STR_LEGACY_PERMISSIVE`` macro
+is provided as an escape hatch to go back to the legacy behavior. This macro
+will be removed in future releases. Two types of required client-code fixes
+are expected to be common:
+
+* Accidental use of ``py::str`` instead of ``py::bytes``, masked by the legacy
+  behavior. These are probably very easy to fix, by changing from
+  ``py::str`` to ``py::bytes``.
+
+* Reliance on py::isinstance<str>(obj) being ``true`` for
+  ``py::bytes``. This is likely to be easy to fix in most cases by adding
+  ``|| py::isinstance<bytes>(obj)``, but a fix may be more involved, e.g. if
+  ``py::isinstance<T>`` appears in a template. Such situations will require
+  careful review and custom fixes.
+
+
+
 v2.6
 ====
 

include/pybind11/cast.h

@@ -1628,7 +1628,10 @@ struct pyobject_caster {
 
     template <typename T = type, enable_if_t<std::is_base_of<object, T>::value, int> = 0>
     bool load(handle src, bool /* convert */) {
-#if defined(PYBIND11_STR_NON_PERMISSIVE) && !defined(PYBIND11_STR_CASTER_NO_IMPLICIT_DECODE)
+#if PY_MAJOR_VERSION < 3 && !defined(PYBIND11_STR_LEGACY_PERMISSIVE)
+        // For Python 2, without this implicit conversion, Python code would
+        // need to be cluttered with six.ensure_text() or similar, only to be
+        // un-cluttered later after Python 2 support is dropped.
         if (std::is_same<T, str>::value && isinstance<bytes>(src)) {
             PyObject *str_from_bytes = PyUnicode_FromEncodedObject(src.ptr(), "utf-8", nullptr);
             if (!str_from_bytes) throw error_already_set();

include/pybind11/detail/common.h

@@ -163,19 +163,18 @@
 #include <typeindex>
 #include <type_traits>
 
-#define PYBIND11_STR_NON_PERMISSIVE
-// If UNDEFINED, pybind11::str can hold PyUnicodeObject or PyBytesObject
-// (probably surprising, but this is the legacy behavior). As a side-effect,
-// pybind11::isinstance<str>() is true for both pybind11::str and pybind11::bytes.
-// If DEFINED, pybind11::str can only hold PyUnicodeObject, and
-// pybind11::isinstance<str>() is true only for pybind11::str.
-
-#if PY_MAJOR_VERSION >= 3
-#define PYBIND11_STR_CASTER_NO_IMPLICIT_DECODE
-#endif
-// This macro has an effect only if PYBIND11_STR_NON_PERMISSIVE is defined.
-// If UNDEFINED, the pybind11::str caster will implicitly decode bytes to PyUnicodeObject.
-// If DEFINED, the pybind11::str caster will only accept PyUnicodeObject.
+// #define PYBIND11_STR_LEGACY_PERMISSIVE
+// If DEFINED, pybind11::str can hold PyUnicodeObject or PyBytesObject
+//             (probably surprising and never documented, but this was the
+//             legacy behavior until and including v2.6.x). As a side-effect,
+//             pybind11::isinstance<str>() is true for both pybind11::str and
+//             pybind11::bytes.
+// If UNDEFINED, pybind11::str can only hold PyUnicodeObject, and
+//               pybind11::isinstance<str>() is true only for pybind11::str.
+//               However, for Python 2 only (!), the pybind11::str caster
+//               implicitly decodes bytes to PyUnicodeObject. This is to ease
+//               the transition from the legacy behavior to the non-permissive
+//               behavior.
 
 #if PY_MAJOR_VERSION >= 3 /// Compatibility macros for various Python versions
 #define PYBIND11_INSTANCE_METHOD_NEW(ptr, class_) PyInstanceMethod_New(ptr)

include/pybind11/pytypes.h

@@ -754,11 +754,11 @@ inline bool PyIterable_Check(PyObject *obj) {
 inline bool PyNone_Check(PyObject *o) { return o == Py_None; }
 inline bool PyEllipsis_Check(PyObject *o) { return o == Py_Ellipsis; }
 
-#ifdef PYBIND11_STR_NON_PERMISSIVE
-#define PYBIND11_STR_CHECK_FUN PyUnicode_Check
-#else
+#ifdef PYBIND11_STR_LEGACY_PERMISSIVE
 inline bool PyUnicode_Check_Permissive(PyObject *o) { return PyUnicode_Check(o) || PYBIND11_BYTES_CHECK(o); }
 #define PYBIND11_STR_CHECK_FUN detail::PyUnicode_Check_Permissive
+#else
+#define PYBIND11_STR_CHECK_FUN PyUnicode_Check
 #endif
 
 inline bool PyStaticMethod_Check(PyObject *o) { return o->ob_type == &PyStaticMethod_Type; }

tests/test_eval.py

@@ -22,8 +22,6 @@ def test_evals(capture):
 @pytest.mark.xfail("env.PYPY and not env.PY2", raises=RuntimeError)
 def test_eval_file():
     filename = os.path.join(os.path.dirname(__file__), "test_eval_call.py")
-    if env.PY2:
-        filename = filename.decode("utf-8")
     assert m.test_eval_file(filename)
 
     assert m.test_eval_file_failure()

tests/test_exceptions.py

@@ -79,7 +79,7 @@ def hook(unraisable_hook_args):
         # Use monkeypatch so pytest can apply and remove the patch as appropriate
         monkeypatch.setattr(sys, "unraisablehook", hook)
 
-    assert m.python_alreadyset_in_destructor(u"already_set demo") is True
+    assert m.python_alreadyset_in_destructor("already_set demo") is True
     if hooked:
         assert triggered[0] is True
 

tests/test_pytypes.cpp

@@ -414,11 +414,8 @@ TEST_SUBMODULE(pytypes, m) {
     // test_builtin_functions
     m.def("get_len", [](py::handle h) { return py::len(h); });
 
-#ifdef PYBIND11_STR_NON_PERMISSIVE
-    m.attr("has_str_non_permissive") = true;
-#endif
-#ifdef PYBIND11_STR_CASTER_NO_IMPLICIT_DECODE
-    m.attr("has_str_caster_no_implicit_decode") = true;
+#ifdef PYBIND11_STR_LEGACY_PERMISSIVE
+    m.attr("PYBIND11_STR_LEGACY_PERMISSIVE") = true;
 #endif
 
     m.def("isinstance_pybind11_bytes", [](py::object o) { return py::isinstance<py::bytes>(o); });

tests/test_pytypes.py

@@ -120,23 +120,17 @@ def __repr__(self):
     assert s1 == s2
 
     malformed_utf8 = b"\x80"
+    if hasattr(m, "PYBIND11_STR_LEGACY_PERMISSIVE"):
+        assert m.str_from_object(malformed_utf8) is malformed_utf8
+    elif env.PY2:
+        with pytest.raises(UnicodeDecodeError):
+            m.str_from_object(malformed_utf8)
+    else:
+        assert m.str_from_object(malformed_utf8) == "b'\\x80'"
     if env.PY2:
-        if hasattr(m, "has_str_non_permissive"):
-            with pytest.raises(UnicodeDecodeError):
-                m.str_from_object(malformed_utf8)
-        else:
-            m.str_from_object(
-                malformed_utf8
-            ) is malformed_utf8  # To be fixed; see #2380
         with pytest.raises(UnicodeDecodeError):
             m.str_from_handle(malformed_utf8)
     else:
-        if hasattr(m, "has_str_non_permissive"):
-            assert m.str_from_object(malformed_utf8) == "b'\\x80'"
-        else:
-            assert (
-                m.str_from_object(malformed_utf8) is malformed_utf8
-            )  # To be fixed; see #2380
         assert m.str_from_handle(malformed_utf8) == "b'\\x80'"
 
 
@@ -312,26 +306,26 @@ def test_pybind11_str_raw_str():
     valid_orig = u"DZ"
     valid_utf8 = valid_orig.encode("utf-8")
     valid_cvt = cvt(valid_utf8)
-    if hasattr(m, "has_str_non_permissive"):
+    if hasattr(m, "PYBIND11_STR_LEGACY_PERMISSIVE"):
+        assert valid_cvt is valid_utf8
+    else:
         assert type(valid_cvt) is unicode if env.PY2 else str  # noqa: F821
         if env.PY2:
             assert valid_cvt == valid_orig
         else:
-            assert valid_cvt == u"b'\\xc7\\xb1'"
-    else:
-        assert valid_cvt is valid_utf8
+            assert valid_cvt == "b'\\xc7\\xb1'"
 
     malformed_utf8 = b"\x80"
-    if hasattr(m, "has_str_non_permissive"):
+    if hasattr(m, "PYBIND11_STR_LEGACY_PERMISSIVE"):
+        assert cvt(malformed_utf8) is malformed_utf8
+    else:
         if env.PY2:
             with pytest.raises(UnicodeDecodeError):
                 cvt(malformed_utf8)
         else:
             malformed_cvt = cvt(malformed_utf8)
-            assert type(malformed_cvt) is unicode if env.PY2 else str  # noqa: F821
-            assert malformed_cvt == u"b'\\x80'"
-    else:
-        assert cvt(malformed_utf8) is malformed_utf8
+            assert type(malformed_cvt) is str
+            assert malformed_cvt == "b'\\x80'"
 
 
 def test_implicit_casting():
@@ -517,34 +511,33 @@ def test_isinstance_string_types():
     assert not m.isinstance_pybind11_bytes(u"")
 
     assert m.isinstance_pybind11_str(u"")
-    if hasattr(m, "has_str_non_permissive"):
-        assert not m.isinstance_pybind11_str(b"")
-    else:
+    if hasattr(m, "PYBIND11_STR_LEGACY_PERMISSIVE"):
         assert m.isinstance_pybind11_str(b"")
+    else:
+        assert not m.isinstance_pybind11_str(b"")
 
 
 def test_pass_bytes_or_unicode_to_string_types():
     assert m.pass_to_pybind11_bytes(b"Bytes") == 5
     with pytest.raises(TypeError):
         m.pass_to_pybind11_bytes(u"Str")
 
-    if hasattr(m, "has_str_caster_no_implicit_decode"):
+    if hasattr(m, "PYBIND11_STR_LEGACY_PERMISSIVE") or env.PY2:
+        assert m.pass_to_pybind11_str(b"Bytes") == 5
+    else:
         with pytest.raises(TypeError):
             m.pass_to_pybind11_str(b"Bytes")
-    else:
-        assert m.pass_to_pybind11_str(b"Bytes") == 5
     assert m.pass_to_pybind11_str(u"Str") == 3
 
     assert m.pass_to_std_string(b"Bytes") == 5
     assert m.pass_to_std_string(u"Str") == 3
 
     malformed_utf8 = b"\x80"
-    if hasattr(m, "has_str_non_permissive"):
-        if hasattr(m, "has_str_caster_no_implicit_decode"):
-            with pytest.raises(TypeError):
-                m.pass_to_pybind11_str(malformed_utf8)
-        else:
-            with pytest.raises(UnicodeDecodeError):
-                m.pass_to_pybind11_str(malformed_utf8)
-    else:
+    if hasattr(m, "PYBIND11_STR_LEGACY_PERMISSIVE"):
         assert m.pass_to_pybind11_str(malformed_utf8) == 1
+    elif env.PY2:
+        with pytest.raises(UnicodeDecodeError):
+            m.pass_to_pybind11_str(malformed_utf8)
+    else:
+        with pytest.raises(TypeError):
+            m.pass_to_pybind11_str(malformed_utf8)