diff --git a/docs/advanced/cast/stl.rst b/docs/advanced/cast/stl.rst index 1e17bc389c..6abe8b467d 100644 --- a/docs/advanced/cast/stl.rst +++ b/docs/advanced/cast/stl.rst @@ -17,6 +17,10 @@ converted (i.e. copied) on every Python->C++ and C++->Python transition, which can have implications on the program semantics and performance. Please read the next sections for more details and alternative approaches that avoid this. +Copying the container does not make non-owning element types own their data. +In particular, containers of C++ string views have additional +:ref:`string_view_lifetime` requirements. + .. note:: Arbitrary nesting of any of these types is possible. diff --git a/docs/advanced/cast/strings.rst b/docs/advanced/cast/strings.rst index 271716b4b3..2b24d7cb99 100644 --- a/docs/advanced/cast/strings.rst +++ b/docs/advanced/cast/strings.rst @@ -280,6 +280,8 @@ expressed as a single Unicode code point no way to capture them in a C++ character type. +.. _string_view_lifetime: + C++17 string views ================== @@ -289,6 +291,30 @@ string type (for example, a ``std::u16string_view`` argument will be passed UTF-16-encoded data, and a returned ``std::string_view`` will be decoded as UTF-8). +A string view does not own its character data. When a view is loaded as an +argument to a pybind11-bound function, pybind11 keeps the Python object that +provides the data alive until the function returns. This also applies to views +nested in automatically converted STL containers. The C++ function must not +retain any such view after it returns unless it separately guarantees that the +backing storage remains alive and valid. + +Lifetime support keeps the Python object alive, but does not prevent its storage +from being invalidated. For example, if C++ releases the GIL or calls back into +Python, resizing a backing ``bytearray`` while the view is in use can invalidate +the view. + +A direct Python-to-C++ :func:`py::cast` made when no bound-function call is +active has no such lifetime support. When a cast to a non-owning view succeeds, +the caller must keep the backing Python object alive, with its storage +unchanged, for as long as the view is used. For a container of views, this +requirement applies to every element: retain the elements directly or through +an unmodified owning container, and do not cast an iterable that creates +temporary elements. + +Some view conversions require temporary backing storage, for example to encode +text. Outside a bound-function call, such conversions raise +:class:`cast_error` instead of returning a dangling view. + References ========== diff --git a/docs/advanced/pycpp/object.rst b/docs/advanced/pycpp/object.rst index 93e1a94d8f..d8661c2ce7 100644 --- a/docs/advanced/pycpp/object.rst +++ b/docs/advanced/pycpp/object.rst @@ -76,6 +76,10 @@ The reverse direction uses the following syntax: When conversion fails, both directions throw the exception :class:`cast_error`. +When casting to a non-owning type such as ``std::string_view``, the Python +source may need to remain alive after a successful cast. See +:ref:`string_view_lifetime` for details. + .. _python_libs: Accessing Python libraries from C++ diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 62ca45a09a..9ab6e332d1 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -525,6 +525,11 @@ struct string_caster { return false; } value = StringType(buffer, static_cast(size)); + if (IsView) { + // `src` owns the buffer; keep it alive if inside a bound function, + // otherwise the caller is responsible for its lifetime. + loader_life_support::try_add_patient(src); + } return true; } @@ -602,6 +607,9 @@ struct string_caster { pybind11_fail("Unexpected PYBIND11_BYTES_AS_STRING() failure."); } value = StringType(bytes, (size_t) PYBIND11_BYTES_SIZE(src.ptr())); + if (IsView) { + loader_life_support::try_add_patient(src); + } return true; } if (PyByteArray_Check(src.ptr())) { @@ -612,6 +620,9 @@ struct string_caster { pybind11_fail("Unexpected PyByteArray_AsString() failure."); } value = StringType(bytearray, (size_t) PyByteArray_Size(src.ptr())); + if (IsView) { + loader_life_support::try_add_patient(src); + } return true; } diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 8fbf700e12..b6d03ca903 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -81,11 +81,26 @@ class loader_life_support { } } + /// Keep `h` alive until the current patient frame is destroyed, if there is one. + /// Returns false when called outside a bound function (no frame). Use this, rather + /// than `add_patient`, when failing to register is acceptable because the caller + /// owns the source's lifetime outside the call framework (e.g. a view that points + /// into an existing Python object, as opposed to a freshly created temporary). + PYBIND11_NOINLINE static bool try_add_patient(handle h) { + loader_life_support *frame = tls_current_frame(); + if (!frame) { + return false; + } + if (frame->keep_alive.insert(h.ptr()).second) { + Py_INCREF(h.ptr()); + } + return true; + } + /// This can only be used inside a pybind11-bound function, either by `argument_loader` /// at argument preparation time or by `py::cast()` at execution time. PYBIND11_NOINLINE static void add_patient(handle h) { - loader_life_support *frame = tls_current_frame(); - if (!frame) { + if (!try_add_patient(h)) { // NOTE: It would be nice to include the stack frames here, as this indicates // use of pybind11::cast<> outside the normal call framework, finding such // a location is challenging. Developers could consider printing out @@ -94,10 +109,6 @@ class loader_life_support { "do Python -> C++ conversions which require the creation " "of temporary values"); } - - if (frame->keep_alive.insert(h.ptr()).second) { - Py_INCREF(h.ptr()); - } } }; diff --git a/tests/test_stl.cpp b/tests/test_stl.cpp index 8bddbb1f38..b5d1979d04 100644 --- a/tests/test_stl.cpp +++ b/tests/test_stl.cpp @@ -582,6 +582,24 @@ TEST_SUBMODULE(stl, m) { [](const std::list &) { return 2; }); m.def("func_with_string_or_vector_string_arg_overload", [](const std::string &) { return 3; }); +#ifdef PYBIND11_HAS_STRING_VIEW + m.def("func_with_string_views", [](const std::vector &svs) { + py::list l; + for (std::string_view sv : svs) { + l.append(sv); + } + return l; + }); + m.def("string_view_life_support_check", + [](const std::vector &, int, const py::list &destroyed) { + return destroyed.size(); + }); + m.def("nested_string_view_life_support_check", + [](const std::vector> &, int, const py::list &destroyed) { + return destroyed.size(); + }); +#endif + class Placeholder { public: Placeholder() { print_created(this); } diff --git a/tests/test_stl.py b/tests/test_stl.py index b04f55c9f8..13e422314c 100644 --- a/tests/test_stl.py +++ b/tests/test_stl.py @@ -1,5 +1,7 @@ from __future__ import annotations +import weakref + import pytest import env # noqa: F401 @@ -28,6 +30,116 @@ def test_vector(doc): # Test regression caused by 936: pointers to stl containers weren't castable assert m.cast_ptr_vector() == ["lvalue", "lvalue"] + if hasattr(m, "func_with_string_views"): + + def gen(): + return ("a" + str(x) for x in range(10000, 10010)) + + expected = list(gen()) + assert m.func_with_string_views(gen()) == expected + assert m.func_with_string_views(x.encode() for x in gen()) == expected + assert ( + m.func_with_string_views(bytearray(x.encode()) for x in gen()) == expected + ) + + +@pytest.mark.skipif( + not hasattr(m, "string_view_life_support_check"), reason="no " +) +@pytest.mark.skipif("env.GRAALPY", reason="Cannot reliably trigger GC") +def test_string_view_life_support_during_argument_conversion(): + destroyed = [] + + class TrackedString(str): + pass + + source = [TrackedString("first"), TrackedString("second")] + weakrefs = [weakref.ref(item, lambda _: destroyed.append(None)) for item in source] + + # Clear the only Python owners after the views have loaded, but before the + # bound function is called. + class ClearSourceOnIndex: + def __index__(self): + source.clear() + pytest.gc_collect() + return 0 + + assert ( + m.string_view_life_support_check(source, ClearSourceOnIndex(), destroyed) == 0 + ) + assert source == [] + pytest.gc_collect() + assert len(destroyed) == 2 + assert all(ref() is None for ref in weakrefs) + + +@pytest.mark.skipif( + not hasattr(m, "string_view_life_support_check"), reason="no " +) +@pytest.mark.skipif("env.GRAALPY", reason="Cannot reliably trigger GC") +@pytest.mark.parametrize( + ("element_type", "values"), + [ + pytest.param(str, ("first", "second"), id="str"), + pytest.param(bytes, (b"first", b"second"), id="bytes"), + pytest.param(bytearray, (b"first", b"second"), id="bytearray"), + ], +) +def test_string_view_life_support_for_generator(element_type, values): + destroyed = [] + + class Tracked(element_type): + def __del__(self): + destroyed.append(None) + + def source(): + for value in values: + yield Tracked(value) + + class CollectGarbageOnIndex: + def __index__(self): + pytest.gc_collect() + return 0 + + assert ( + m.string_view_life_support_check(source(), CollectGarbageOnIndex(), destroyed) + == 0 + ) + pytest.gc_collect() + assert len(destroyed) == len(values) + + +@pytest.mark.skipif( + not hasattr(m, "nested_string_view_life_support_check"), + reason="no ", +) +@pytest.mark.skipif("env.GRAALPY", reason="Cannot reliably trigger GC") +def test_string_view_life_support_for_nested_containers(): + destroyed = [] + + class TrackedString(str): + def __del__(self): + destroyed.append(None) + + source = [ + [TrackedString("first"), TrackedString("second")], + [TrackedString("third"), TrackedString("fourth")], + ] + + class ClearSourceOnIndex: + def __index__(self): + source.clear() + pytest.gc_collect() + return 0 + + assert ( + m.nested_string_view_life_support_check(source, ClearSourceOnIndex(), destroyed) + == 0 + ) + assert source == [] + pytest.gc_collect() + assert len(destroyed) == 4 + def test_deque(): """std::deque <-> list""" diff --git a/tests/test_with_catch/test_interpreter.cpp b/tests/test_with_catch/test_interpreter.cpp index 4103c0f5ff..daa1041bf5 100644 --- a/tests/test_with_catch/test_interpreter.cpp +++ b/tests/test_with_catch/test_interpreter.cpp @@ -509,3 +509,32 @@ TEST_CASE("make_iterator can be called before then after finalizing an interpret py::initialize_interpreter(); } + +#ifdef PYBIND11_HAS_STRING_VIEW +TEST_CASE("Casting to a string_view outside a bound function") { + // Regression for PR #6092: view casters add the source to loader_life_support, but + // outside a bound function there is no frame. The caller owns the source's lifetime + // here, so the cast must succeed rather than throw. + py::str unicode("hello"); + py::bytes bytes_obj("world", 5); + auto bytearray_obj + = py::reinterpret_steal(PyByteArray_FromStringAndSize("bytes", 5)); + + REQUIRE(py::cast(unicode) == "hello"); + REQUIRE(py::cast(bytes_obj) == "world"); + REQUIRE(py::cast(bytearray_obj) == "bytes"); + + // Wide string views require an encoded temporary. With no loader life-support + // frame, returning a view into that temporary must fail. + REQUIRE_THROWS_AS(py::cast(unicode), py::cast_error); + REQUIRE_THROWS_AS(py::cast(unicode), py::cast_error); + + // Bound-function dispatch provides a frame that keeps both temporaries alive. + auto accepts_wide_views + = py::cpp_function([](std::u16string_view value16, std::u32string_view value32) { + return value16 == std::u16string_view(u"hello") + && value32 == std::u32string_view(U"hello"); + }); + REQUIRE(accepts_wide_views(unicode, unicode).cast()); +} +#endif