From 9e0555d05b8f88b5b424d05ea72967a97bcb9af8 Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Fri, 19 Mar 2021 22:29:29 +0100 Subject: [PATCH 1/6] Test return by reference Returning a non-const reference from C++ to Python, shouldn't create a new copy. However, by design, the user explicitly needs to opt-in for reference-passing by providing return_value_policy::reference. --- tests/test_class_sh_basic.cpp | 2 +- tests/test_class_sh_basic.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_class_sh_basic.cpp b/tests/test_class_sh_basic.cpp index ff9f97607d..252c401f86 100644 --- a/tests/test_class_sh_basic.cpp +++ b/tests/test_class_sh_basic.cpp @@ -102,7 +102,7 @@ TEST_SUBMODULE(class_sh_basic, m) { m.def("rtrn_valu", rtrn_valu); m.def("rtrn_rref", rtrn_rref); m.def("rtrn_cref", rtrn_cref); - m.def("rtrn_mref", rtrn_mref); + m.def("rtrn_mref", rtrn_mref, py::return_value_policy::reference); m.def("rtrn_cptr", rtrn_cptr); m.def("rtrn_mptr", rtrn_mptr); diff --git a/tests/test_class_sh_basic.py b/tests/test_class_sh_basic.py index 3727dcdb59..f46688dcc1 100644 --- a/tests/test_class_sh_basic.py +++ b/tests/test_class_sh_basic.py @@ -22,7 +22,7 @@ def test_atyp_constructors(): (m.rtrn_valu, "rtrn_valu(_MvCtor)*_MvCtor"), (m.rtrn_rref, "rtrn_rref(_MvCtor)*_MvCtor"), (m.rtrn_cref, "rtrn_cref(_MvCtor)*_CpCtor"), - (m.rtrn_mref, "rtrn_mref(_MvCtor)*_CpCtor"), + (m.rtrn_mref, "rtrn_mref"), (m.rtrn_cptr, "rtrn_cptr"), (m.rtrn_mptr, "rtrn_mptr"), (m.rtrn_shmp, "rtrn_shmp"), From 4121474d1b0a3b11de2d8dfdc8217ff464255b34 Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Sat, 20 Mar 2021 03:20:05 +0100 Subject: [PATCH 2/6] More roundtrip tests: What happens to objects passed to C++ and returned back? This reveals that passing a unique_ptr const-reference from python to C++ doesn't work. unique_ptrs can only be moved from Python to C++ (requiring that Python actually owns the object). The tests also reveal that passing a unique_ptr non-const reference from C++ to Python is possible. However, this is bad programming style (it's not clear whether ownership is transferred or not). Shouldn't we suppress this? Removed redundant test_unique_ptr_cref_roundtrip() Renamed uconsumer -> consumer --- .../detail/smart_holder_type_casters.h | 5 + tests/test_class_sh_basic.cpp | 54 +++++--- tests/test_class_sh_basic.py | 117 +++++++++++++----- 3 files changed, 124 insertions(+), 52 deletions(-) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 14b3ea9979..4ecd15e6f0 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -732,6 +732,11 @@ struct smart_holder_type_caster> : smart_holder_type_caste using cast_op_type = std::unique_ptr; operator std::unique_ptr() { return this->template loaded_as_unique_ptr(); } + // TODO: To allow passing unique_ptr const-references from Python to C++, + // we need to add another cast operator for const std::unique_ptr&(). + // The above cast operator always moves the held raw pointer, even if the argument only + // asked for a (const!) reference. + // See test_class_sh_basic.py::test_unique_ptr_cref_store_roundtrip }; template diff --git a/tests/test_class_sh_basic.cpp b/tests/test_class_sh_basic.cpp index 252c401f86..5b80dd755d 100644 --- a/tests/test_class_sh_basic.cpp +++ b/tests/test_class_sh_basic.cpp @@ -2,6 +2,7 @@ #include +#include #include #include #include @@ -17,18 +18,27 @@ struct atyp { // Short for "any type". atyp(atyp &&other) { mtxt = other.mtxt + "_MvCtor"; } }; -struct uconsumer { // unique_ptr consumer +// clang-format off +struct store { // unique_ptr store: std::unique_ptr held; bool valid() const { return static_cast(held); } - void pass_valu(std::unique_ptr obj) { held = std::move(obj); } - void pass_rref(std::unique_ptr &&obj) { held = std::move(obj); } - std::unique_ptr rtrn_valu() { return std::move(held); } - std::unique_ptr &rtrn_lref() { return held; } - const std::unique_ptr &rtrn_cref() { return held; } + std::string pass_uq_valu(std::unique_ptr obj) { held = std::move(obj); return held->mtxt; } + std::string pass_uq_rref(std::unique_ptr &&obj) { held = std::move(obj); return held->mtxt; } + std::string pass_uq_cref(const std::unique_ptr &obj) { return obj->mtxt; } + std::string pass_cptr(const atyp *obj) { return obj->mtxt; } + std::string pass_cref(const atyp &obj) { return obj.mtxt; } + + std::unique_ptr rtrn_uq_valu() { return std::move(held); } + std::unique_ptr&& rtrn_uq_rref() { return std::move(held); } + std::unique_ptr& rtrn_uq_mref() { return held; } + const std::unique_ptr& rtrn_uq_cref() { return held; } + const atyp* rtrn_cptr() const { return held.get(); } + const atyp& rtrn_cref() const { return *held; } + atyp *rtrn_mptr() { return held.get(); } + atyp &rtrn_mref() { return *held; } }; -// clang-format off atyp rtrn_valu() { atyp obj{"rtrn_valu"}; return obj; } atyp&& rtrn_rref() { static atyp obj; obj.mtxt = "rtrn_rref"; return std::move(obj); } @@ -68,12 +78,9 @@ std::string pass_udcp(std::unique_ptr obj) { return "pass_udcp // Helpers for testing. std::string get_mtxt(atyp const &obj) { return obj.mtxt; } -std::ptrdiff_t get_ptr(atyp const &obj) { return reinterpret_cast(&obj); } +std::uintptr_t get_ptr(atyp const &obj) { return reinterpret_cast(&obj); } std::unique_ptr unique_ptr_roundtrip(std::unique_ptr obj) { return obj; } -const std::unique_ptr &unique_ptr_cref_roundtrip(const std::unique_ptr &obj) { - return obj; -} struct SharedPtrStash { std::vector> stash; @@ -84,7 +91,7 @@ struct SharedPtrStash { } // namespace pybind11_tests PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_basic::atyp) -PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_basic::uconsumer) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_basic::store) PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_basic::SharedPtrStash) namespace pybind11_tests { @@ -130,14 +137,22 @@ TEST_SUBMODULE(class_sh_basic, m) { m.def("pass_udmp", pass_udmp); m.def("pass_udcp", pass_udcp); - py::classh(m, "uconsumer") + py::classh(m, "store") .def(py::init<>()) - .def("valid", &uconsumer::valid) - .def("pass_valu", &uconsumer::pass_valu) - .def("pass_rref", &uconsumer::pass_rref) - .def("rtrn_valu", &uconsumer::rtrn_valu) - .def("rtrn_lref", &uconsumer::rtrn_lref) - .def("rtrn_cref", &uconsumer::rtrn_cref); + .def("valid", &store::valid) + .def("pass_uq_valu", &store::pass_uq_valu) + .def("pass_uq_rref", &store::pass_uq_rref) + .def("pass_uq_cref", &store::pass_uq_cref) + .def("pass_cptr", &store::pass_cptr) + .def("pass_cref", &store::pass_cref) + .def("rtrn_uq_valu", &store::rtrn_uq_valu) + .def("rtrn_uq_rref", &store::rtrn_uq_rref) + .def("rtrn_uq_mref", &store::rtrn_uq_mref) + .def("rtrn_uq_cref", &store::rtrn_uq_cref) + .def("rtrn_mptr", &store::rtrn_mptr, py::return_value_policy::reference_internal) + .def("rtrn_mref", &store::rtrn_mref, py::return_value_policy::reference_internal) + .def("rtrn_cptr", &store::rtrn_cptr, py::return_value_policy::reference_internal) + .def("rtrn_cref", &store::rtrn_cref, py::return_value_policy::reference_internal); // Helpers for testing. // These require selected functions above to work first, as indicated: @@ -145,7 +160,6 @@ TEST_SUBMODULE(class_sh_basic, m) { m.def("get_ptr", get_ptr); // pass_cref m.def("unique_ptr_roundtrip", unique_ptr_roundtrip); // pass_uqmp, rtrn_uqmp - m.def("unique_ptr_cref_roundtrip", unique_ptr_cref_roundtrip); py::classh(m, "SharedPtrStash") .def(py::init<>()) diff --git a/tests/test_class_sh_basic.py b/tests/test_class_sh_basic.py index f46688dcc1..a282e3f8af 100644 --- a/tests/test_class_sh_basic.py +++ b/tests/test_class_sh_basic.py @@ -118,46 +118,99 @@ def test_unique_ptr_roundtrip(num_round_trips=1000): id_orig = id_rtrn -# This currently fails, because a unique_ptr is always loaded by value -# due to pybind11/detail/smart_holder_type_casters.h:689 -# I think, we need to provide more cast operators. -@pytest.mark.skip -def test_unique_ptr_cref_roundtrip(num_round_trips=1000): - orig = m.atyp("passenger") - id_orig = id(orig) +# Validate moving an object from Python into a C++ object store +@pytest.mark.parametrize("pass_f", [m.store.pass_uq_valu, m.store.pass_uq_rref]) +def test_unique_ptr_moved(pass_f): + store = m.store() + orig = m.atyp("O") mtxt_orig = m.get_mtxt(orig) + ptr_orig = m.get_ptr(orig) + assert re.match("O(_MvCtor){1,2}", mtxt_orig) + + pass_f(store, orig) # pass object to C++ store c + with pytest.raises(ValueError) as excinfo: + m.get_mtxt(orig) + assert "Python instance was disowned" in str(excinfo.value) - recycled = m.unique_ptr_cref_roundtrip(orig) - assert m.get_mtxt(orig) == mtxt_orig - assert m.get_mtxt(recycled) == mtxt_orig - assert id(recycled) == id_orig + del orig + recycled = store.rtrn_uq_cref() + assert m.get_ptr(recycled) == ptr_orig # underlying C++ object doesn't change + assert m.get_mtxt(recycled) == mtxt_orig # object was not moved or copied +# This series of roundtrip tests checks how an object instance moved from +# Python to C++ (into store) can be later returned back to Python. @pytest.mark.parametrize( - "pass_f, rtrn_f, moved_out, moved_in", + "rtrn_f, moved_in", [ - (m.uconsumer.pass_valu, m.uconsumer.rtrn_valu, True, True), - (m.uconsumer.pass_rref, m.uconsumer.rtrn_valu, True, True), - (m.uconsumer.pass_valu, m.uconsumer.rtrn_lref, True, False), - (m.uconsumer.pass_valu, m.uconsumer.rtrn_cref, True, False), + (m.store.rtrn_uq_valu, True), # moved back in + (m.store.rtrn_uq_rref, True), # moved back in + (m.store.rtrn_uq_mref, None), # forbidden + (m.store.rtrn_uq_cref, False), # fetched by reference + (m.store.rtrn_mref, None), # forbidden + (m.store.rtrn_cref, False), # fetched by reference + (m.store.rtrn_mptr, None), # forbidden + (m.store.rtrn_cptr, False), # fetched by reference ], ) -def test_unique_ptr_consumer_roundtrip(pass_f, rtrn_f, moved_out, moved_in): - c = m.uconsumer() - assert not c.valid() - recycled = m.atyp("passenger") - mtxt_orig = m.get_mtxt(recycled) - assert re.match("passenger_(MvCtor){1,2}", mtxt_orig) - - pass_f(c, recycled) - if moved_out: - with pytest.raises(ValueError) as excinfo: - m.get_mtxt(recycled) - assert "Python instance was disowned" in str(excinfo.value) - - recycled = rtrn_f(c) - assert c.valid() != moved_in - assert m.get_mtxt(recycled) == mtxt_orig +def test_unique_ptr_store_roundtrip(rtrn_f, moved_in): + c = m.store() + orig = m.atyp("passenger") + ptr_orig = m.get_ptr(orig) + + c.pass_uq_valu(orig) # pass object to C++ store c + try: + recycled = rtrn_f(c) # retrieve object back from C++ + except RuntimeError: # expect failure for rtrn_uq_lref + assert moved_in is None + return + + assert m.get_ptr(recycled) == ptr_orig # do we yield the same object? + if moved_in: # store should have given up ownership? + assert c.valid() is False + else: # store still helds the object + assert c.valid() is True + del recycled + assert c.valid() is True + + +# Additionally to the above test_unique_ptr_store_roundtrip, this test +# validates that an object initially moved from Python to C++ can be returned +# to Python as a *const* reference/raw pointer/unique_ptr *and*, subsequently, +# passed from Python to C++ again. There shouldn't be any copy or move operation +# involved (We want the object to be passed by reference!) +@pytest.mark.parametrize( + "rtrn_f", + [m.store.rtrn_uq_cref, m.store.rtrn_cref, m.store.rtrn_cptr], +) +@pytest.mark.parametrize( + "pass_f", + [ + # This fails with: ValueError: Cannot disown non-owning holder (loaded_as_unique_ptr). + # This could, at most, work for the combination rtrn_uq_cref() + pass_uq_cref(), + # i.e. fetching a unique_ptr const-ref from C++ and passing the very same reference back. + # Currently, it is forbidden - by design - to pass a unique_ptr const-ref to C++. + # unique_ptrs are always moved (if possible). + # To allow this use case, smart_holder would need to store the unique_ptr reference, + # originally received from C++, e.g. using a union of unique_ptr + shared_ptr. + pytest.param(m.store.pass_uq_cref, marks=pytest.mark.xfail), + m.store.pass_cptr, + m.store.pass_cref, + ], +) +def test_unique_ptr_cref_store_roundtrip(rtrn_f, pass_f): + c = m.store() + passenger = m.atyp("passenger") + mtxt_orig = m.get_mtxt(passenger) + ptr_orig = m.get_ptr(passenger) + + # moves passenger to C++ (checked in test_unique_ptr_store_roundtrip) + c.pass_uq_valu(passenger) + + for _ in range(10): + cref = rtrn_f(c) # fetches const reference, should keep-alive parent c + assert pass_f(c, cref) == mtxt_orig # no copy/move happened? + assert m.get_ptr(cref) == ptr_orig # it's still the same raw pointer def test_py_type_handle_of_atyp(): From dd956c98419ff229c960912d9a8de9e7d55af7de Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Mon, 22 Mar 2021 22:26:14 +0100 Subject: [PATCH 3/6] More roundtrip tests: object references passed to trampoline methods Add tests to ensure that reference arguments passed to trampoline methods (from C++ -> Python -> C++) are actually passed by reference (and not copied), which is the default return_value_policy to pass from C++ to Python. Being able to pass by reference is essential to allow object modifications performed in Python code to become visible in C++. --- tests/test_class_sh_with_alias.cpp | 104 ++++++++++++++++++++ tests/test_class_sh_with_alias.py | 147 +++++++++++++++++++++++++++++ 2 files changed, 251 insertions(+) diff --git a/tests/test_class_sh_with_alias.cpp b/tests/test_class_sh_with_alias.cpp index 649eb61f0a..8154404446 100644 --- a/tests/test_class_sh_with_alias.cpp +++ b/tests/test_class_sh_with_alias.cpp @@ -2,6 +2,7 @@ #include +#include #include namespace pybind11_tests { @@ -73,14 +74,117 @@ void wrap(py::module_ m, const char *py_class_name) { m.def("AddInCppUniquePtr", AddInCppUniquePtr, py::arg("obj"), py::arg("other_val")); } +/* Tests passing objects by reference to Python-derived class methods */ +struct Passenger { // object class passed around, recording its copy and move constructions + std::string mtxt; + Passenger() = default; + // on copy or move: keep old mtxt and augment operation as well as new pointer id + Passenger(const Passenger &other) { mtxt = other.mtxt + "Copy->" + std::to_string(id()); } + Passenger(Passenger &&other) { mtxt = other.mtxt + "Move->" + std::to_string(id()); } + uintptr_t id() const { return reinterpret_cast(this); } +}; +struct ReferencePassingTest { // virtual base class used to test reference passing + ReferencePassingTest() = default; + ReferencePassingTest(const ReferencePassingTest &) = default; + ReferencePassingTest(ReferencePassingTest &&) = default; + virtual ~ReferencePassingTest() = default; + virtual uintptr_t pass_uq_cref(const std::unique_ptr &obj) { return modify(*obj); }; + // NOLINTNEXTLINE(clang-analyzer-core.StackAddrEscapeBase) + virtual uintptr_t pass_valu(Passenger obj) { return modify(obj); }; + virtual uintptr_t pass_mref(Passenger &obj) { return modify(obj); }; + virtual uintptr_t pass_mptr(Passenger *obj) { return modify(*obj); }; + virtual uintptr_t pass_cref(const Passenger &obj) { return modify(obj); }; + virtual uintptr_t pass_cptr(const Passenger *obj) { return modify(*obj); }; + uintptr_t modify(const Passenger &obj) { return obj.id(); } + uintptr_t modify(Passenger &obj) { + obj.mtxt.append("_MODIFIED"); + return obj.id(); + } +}; +struct PyReferencePassingTest : ReferencePassingTest { + using ReferencePassingTest::ReferencePassingTest; + uintptr_t pass_uq_cref(const std::unique_ptr &obj) override { + PYBIND11_OVERRIDE(uintptr_t, ReferencePassingTest, pass_uq_cref, obj); + } + uintptr_t pass_valu(Passenger obj) override { + PYBIND11_OVERRIDE(uintptr_t, ReferencePassingTest, pass_valu, obj); + } + uintptr_t pass_mref(Passenger &obj) override { + PYBIND11_OVERRIDE(uintptr_t, ReferencePassingTest, pass_mref, obj); + } + uintptr_t pass_cref(const Passenger &obj) override { + PYBIND11_OVERRIDE(uintptr_t, ReferencePassingTest, pass_cref, obj); + } + uintptr_t pass_mptr(Passenger *obj) override { + PYBIND11_OVERRIDE(uintptr_t, ReferencePassingTest, pass_mptr, obj); + } + uintptr_t pass_cptr(const Passenger *obj) override { + PYBIND11_OVERRIDE(uintptr_t, ReferencePassingTest, pass_cptr, obj); + } +}; + +std::string evaluate(const Passenger &orig, uintptr_t cycled) { + return orig.mtxt + (orig.id() == cycled ? "_REF" : "_COPY"); +} +// Functions triggering virtual-method calls from python-derived class (caller) +// Goal: modifications to Passenger happening in Python-code methods +// overriding the C++ virtual methods, should remain visible in C++. +std::string check_roundtrip_uq_cref(ReferencePassingTest &caller) { + std::unique_ptr obj(new Passenger()); + return evaluate(*obj, caller.pass_uq_cref(obj)); +} +// TODO: Find template magic to avoid this code duplication +std::string check_roundtrip_valu(ReferencePassingTest &caller) { + Passenger obj; + return evaluate(obj, caller.pass_valu(obj)); +} +std::string check_roundtrip_mref(ReferencePassingTest &caller) { + Passenger obj; + return evaluate(obj, caller.pass_mref(obj)); +} +std::string check_roundtrip_cref(ReferencePassingTest &caller) { + Passenger obj; + return evaluate(obj, caller.pass_cref(obj)); +} +std::string check_roundtrip_mptr(ReferencePassingTest &caller) { + Passenger obj; + return evaluate(obj, caller.pass_mptr(&obj)); +} +std::string check_roundtrip_cptr(ReferencePassingTest &caller) { + Passenger obj; + return evaluate(obj, caller.pass_cptr(&obj)); +} + } // namespace class_sh_with_alias } // namespace pybind11_tests PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_with_alias::Abase<0>) PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_with_alias::Abase<1>) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_with_alias::Passenger) +PYBIND11_SMART_HOLDER_TYPE_CASTERS(pybind11_tests::class_sh_with_alias::ReferencePassingTest) TEST_SUBMODULE(class_sh_with_alias, m) { using namespace pybind11_tests::class_sh_with_alias; wrap<0>(m, "Abase0"); wrap<1>(m, "Abase1"); + + py::classh(m, "Passenger") + .def_property_readonly("id", &Passenger::id) + .def_readwrite("mtxt", &Passenger::mtxt); + + py::classh(m, "ReferencePassingTest") + .def(py::init<>()) + .def("pass_uq_cref", &ReferencePassingTest::pass_uq_cref) + .def("pass_valu", &ReferencePassingTest::pass_valu) + .def("pass_mref", &ReferencePassingTest::pass_mref) + .def("pass_cref", &ReferencePassingTest::pass_cref) + .def("pass_mptr", &ReferencePassingTest::pass_mptr) + .def("pass_cptr", &ReferencePassingTest::pass_cptr); + + m.def("check_roundtrip_uq_cref", check_roundtrip_uq_cref); + m.def("check_roundtrip_valu", check_roundtrip_valu); + m.def("check_roundtrip_mref", check_roundtrip_mref); + m.def("check_roundtrip_cref", check_roundtrip_cref); + m.def("check_roundtrip_mptr", check_roundtrip_mptr); + m.def("check_roundtrip_cptr", check_roundtrip_cptr); } diff --git a/tests/test_class_sh_with_alias.py b/tests/test_class_sh_with_alias.py index 4650eaa5e1..ece0feafb1 100644 --- a/tests/test_class_sh_with_alias.py +++ b/tests/test_class_sh_with_alias.py @@ -1,5 +1,6 @@ # -*- coding: utf-8 -*- import pytest +import env # noqa: F401 from pybind11_tests import class_sh_with_alias as m @@ -56,3 +57,149 @@ def test_drvd1_add_in_cpp_unique_ptr(): drvd = PyDrvd1(25) assert m.AddInCppUniquePtr(drvd, 83) == ((25 * 10 + 3) * 200 + 83) * 100 + 13 return # Comment out for manual leak checking (use `top` command). + + +# Python class inheriting from C++ class ReferencePassingTest +# virtual methods modify the obj's mtxt, which should become visible in C++ +# To ensure that the original object instance was passed through, +# the pointer id of the received obj is returned by all pass_*() functions +# (and compared by the C++ caller with the originally passed obj id). +class PyReferencePassingTest1(m.ReferencePassingTest): + def __init__(self): + m.ReferencePassingTest.__init__(self) + + def pass_uq_cref(self, obj): + obj.mtxt = obj.mtxt + "pass_uq_cref" + return obj.id + + def pass_valu(self, obj): + obj.mtxt = obj.mtxt + "pass_valu" + return obj.id + + def pass_mref(self, obj): + obj.mtxt = obj.mtxt + "pass_mref" + return obj.id + + def pass_mptr(self, obj): + obj.mtxt = obj.mtxt + "pass_mptr" + return obj.id + + def pass_cref(self, obj): + with pytest.raises(Exception): # should be forbidden + obj.mtxt = obj.mtxt + "pass_cref" + return obj.id + + def pass_cptr(self, obj): + with pytest.raises(Exception): # should be forbidden + obj.mtxt = obj.mtxt + "pass_cptr" + return obj.id + + +# This class, in contrast to PyReferencePassingTest1, calls the base class methods as well, +# which will augment mtxt with a _MODIFIED stamp. +# These calls to the base class methods actually result in a 2nd call to the +# trampoline override dispatcher, requiring argument loading, which should pass +# references through as well, to make these tests succeed. +# argument is passed like this: C++ -> Python (call #1) -> C++ (call #2). +class PyReferencePassingTest2(m.ReferencePassingTest): + def __init__(self): + m.ReferencePassingTest.__init__(self) + + def pass_uq_cref(self, obj): + obj.mtxt = obj.mtxt + "pass_uq_cref" + return m.ReferencePassingTest.pass_uq_cref(self, obj) + + def pass_valu(self, obj): + obj.mtxt = obj.mtxt + "pass_valu" + return m.ReferencePassingTest.pass_valu(self, obj) + + def pass_mref(self, obj): + obj.mtxt = obj.mtxt + "pass_mref" + return m.ReferencePassingTest.pass_mref(self, obj) + + def pass_mptr(self, obj): + obj.mtxt = obj.mtxt + "pass_mptr" + return m.ReferencePassingTest.pass_mptr(self, obj) + + def pass_cref(self, obj): + with pytest.raises(Exception): # should be forbidden + obj.mtxt = obj.mtxt + "pass_cref" + return m.ReferencePassingTest.pass_cref(self, obj) + + def pass_cptr(self, obj): + with pytest.raises(Exception): # should be forbidden + obj.mtxt = obj.mtxt + "pass_cptr" + return m.ReferencePassingTest.pass_cptr(self, obj) + + +# roundtrip tests, creating a Passenger object in C++ that is passed by reference +# to a virtual method of a class derived in Python (PyReferencePassingTest1). +# If the object is correctly passed by reference, modifications should be visible +# by the C++ caller. The final obj's mtxt is returned by the check_* functions +# and validated here. Expected scheme: _[REF|COPY] +@pytest.mark.parametrize( + "f, expected", + [ + (m.check_roundtrip_uq_cref, "pass_uq_cref_REF"), + (m.check_roundtrip_valu, "_COPY"), # modification not passed back to C++ + (m.check_roundtrip_mref, "pass_mref_REF"), + (m.check_roundtrip_mptr, "pass_mptr_REF"), + ], +) +def test_refpassing1_roundtrip_modifyable(f, expected): + c = PyReferencePassingTest1() + assert f(c) == expected + + +@pytest.mark.parametrize( + "f, expected", + [ + # object passed as reference, but not modified + (m.check_roundtrip_cref, "_REF"), + (m.check_roundtrip_cptr, "_REF"), + ], +) +# PYPY always copies the argument (to ensure constness?) +@pytest.mark.skipif("env.PYPY") +@pytest.mark.xfail # maintaining constness isn't implemented yet +def test_refpassing1_roundtrip_const(f, expected): + c = PyReferencePassingTest1() + assert f(c) == expected + + +# Similar test as above, but now using PyReferencePassingTest2, calling +# to the C++ base class methods as well. +# Expected mtxt scheme: _MODIFIED_[REF|COPY] +@pytest.mark.parametrize( + "f, expected", + [ + pytest.param( # cannot (yet) load not owned const unique_ptr& (for 2nd call) + m.check_roundtrip_uq_cref, + "pass_uq_cref", + marks=pytest.mark.xfail, + ), + # object copied, modification not passed back to C++ + (m.check_roundtrip_valu, "_COPY"), + (m.check_roundtrip_mref, "pass_mref_MODIFIED_REF"), + (m.check_roundtrip_mptr, "pass_mptr_MODIFIED_REF"), + ], +) +def test_refpassing2_roundtrip_modifyable(f, expected): + c = PyReferencePassingTest2() + assert f(c) == expected + + +@pytest.mark.parametrize( + "f, expected", + [ + # object passed as reference, but not modified + (m.check_roundtrip_cref, "_REF"), + (m.check_roundtrip_cptr, "_REF"), + ], +) +# PYPY always copies the argument (to ensure constness?) +@pytest.mark.skipif("env.PYPY") +@pytest.mark.xfail # maintaining constness isn't implemented yet +def test_refpassing2_roundtrip_const(f, expected): + c = PyReferencePassingTest2() + assert f(c) == expected From a76c7258fb993c779d71e16babfa145882698931 Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Sun, 21 Mar 2021 03:43:59 +0100 Subject: [PATCH 4/6] Improve accuaracy of unit tests: Count num of copies/moves --- tests/test_class_sh_basic.py | 36 +++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/tests/test_class_sh_basic.py b/tests/test_class_sh_basic.py index a282e3f8af..02f814df36 100644 --- a/tests/test_class_sh_basic.py +++ b/tests/test_class_sh_basic.py @@ -16,12 +16,18 @@ def test_atyp_constructors(): assert obj.__class__.__name__ == "atyp" +def check_regex(expected, actual): + result = re.match(expected + "$", actual) + if result is None: + pytest.fail("expected: '{}' != actual: '{}'".format(expected, actual)) + + @pytest.mark.parametrize( "rtrn_f, expected", [ - (m.rtrn_valu, "rtrn_valu(_MvCtor)*_MvCtor"), - (m.rtrn_rref, "rtrn_rref(_MvCtor)*_MvCtor"), - (m.rtrn_cref, "rtrn_cref(_MvCtor)*_CpCtor"), + (m.rtrn_valu, "rtrn_valu(_MvCtor){1,3}"), + (m.rtrn_rref, "rtrn_rref(_MvCtor){1}"), + (m.rtrn_cref, "rtrn_cref_CpCtor"), (m.rtrn_mref, "rtrn_mref"), (m.rtrn_cptr, "rtrn_cptr"), (m.rtrn_mptr, "rtrn_mptr"), @@ -34,25 +40,25 @@ def test_atyp_constructors(): ], ) def test_cast(rtrn_f, expected): - assert re.match(expected, m.get_mtxt(rtrn_f())) + check_regex(expected, m.get_mtxt(rtrn_f())) @pytest.mark.parametrize( "pass_f, mtxt, expected", [ - (m.pass_valu, "Valu", "pass_valu:Valu(_MvCtor)*_CpCtor"), - (m.pass_cref, "Cref", "pass_cref:Cref(_MvCtor)*_MvCtor"), - (m.pass_mref, "Mref", "pass_mref:Mref(_MvCtor)*_MvCtor"), - (m.pass_cptr, "Cptr", "pass_cptr:Cptr(_MvCtor)*_MvCtor"), - (m.pass_mptr, "Mptr", "pass_mptr:Mptr(_MvCtor)*_MvCtor"), - (m.pass_shmp, "Shmp", "pass_shmp:Shmp(_MvCtor)*_MvCtor"), - (m.pass_shcp, "Shcp", "pass_shcp:Shcp(_MvCtor)*_MvCtor"), - (m.pass_uqmp, "Uqmp", "pass_uqmp:Uqmp(_MvCtor)*_MvCtor"), - (m.pass_uqcp, "Uqcp", "pass_uqcp:Uqcp(_MvCtor)*_MvCtor"), + (m.pass_valu, "Valu", "pass_valu:Valu(_MvCtor){1,2}_CpCtor"), + (m.pass_cref, "Cref", "pass_cref:Cref(_MvCtor){1,2}"), + (m.pass_mref, "Mref", "pass_mref:Mref(_MvCtor){1,2}"), + (m.pass_cptr, "Cptr", "pass_cptr:Cptr(_MvCtor){1,2}"), + (m.pass_mptr, "Mptr", "pass_mptr:Mptr(_MvCtor){1,2}"), + (m.pass_shmp, "Shmp", "pass_shmp:Shmp(_MvCtor){1,2}"), + (m.pass_shcp, "Shcp", "pass_shcp:Shcp(_MvCtor){1,2}"), + (m.pass_uqmp, "Uqmp", "pass_uqmp:Uqmp(_MvCtor){1,2}"), + (m.pass_uqcp, "Uqcp", "pass_uqcp:Uqcp(_MvCtor){1,2}"), ], ) def test_load_with_mtxt(pass_f, mtxt, expected): - assert re.match(expected, pass_f(m.atyp(mtxt))) + check_regex(expected, pass_f(m.atyp(mtxt))) @pytest.mark.parametrize( @@ -111,7 +117,7 @@ def test_unique_ptr_roundtrip(num_round_trips=1000): for _ in range(num_round_trips): id_orig = id(recycled) recycled = m.unique_ptr_roundtrip(recycled) - assert re.match("passenger(_MvCtor)*_MvCtor", m.get_mtxt(recycled)) + check_regex("passenger(_MvCtor){1,2}", m.get_mtxt(recycled)) id_rtrn = id(recycled) # Ensure the returned object is a different Python instance. assert id_rtrn != id_orig From c0dab649d6b264e534d4bfe7dc36db58180e35f2 Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Tue, 23 Mar 2021 01:03:49 +0100 Subject: [PATCH 5/6] Fixup cast() functions of smart_holder_type_caster> - forbid to pass a non-const unique_ptr reference - forbid return_value_policy::reference_internal for unique_ptr&&: It's always moved! --- .../detail/smart_holder_type_casters.h | 20 ++++++++++--------- tests/test_class_sh_basic.py | 7 +++++-- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 4ecd15e6f0..803cc8b448 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -685,12 +685,10 @@ struct smart_holder_type_caster> : smart_holder_type_caste smart_holder_type_caster_class_hooks { static constexpr auto name = _>(); - static handle cast(std::unique_ptr &&src, return_value_policy policy, handle parent) { - if (policy != return_value_policy::automatic - && policy != return_value_policy::reference_internal - && policy != return_value_policy::move) { + static handle cast(std::unique_ptr &&src, return_value_policy policy, handle) { + if (policy != return_value_policy::automatic && policy != return_value_policy::move) { // SMART_HOLDER_WIP: IMPROVABLE: Error message. - throw cast_error("Invalid return_value_policy for unique_ptr."); + throw cast_error("Invalid return_value_policy: unique_ptr&& can only move"); } auto src_raw_ptr = src.get(); @@ -712,11 +710,14 @@ struct smart_holder_type_caster> : smart_holder_type_caste auto smhldr = pybindit::memory::smart_holder::from_unique_ptr(std::move(src)); tinfo->init_instance(inst_raw_ptr, static_cast(&smhldr)); - if (policy == return_value_policy::reference_internal) - keep_alive_impl(inst, parent); - return inst.release(); } + static handle cast(std::unique_ptr &, return_value_policy, handle) { + throw cast_error("Passing non-const unique_ptr& is not supported. " + "If you want to transfer ownership, use unique_ptr&&. " + "If you want to return a reference, use unique_ptr const&."); + } + static handle cast(const std::unique_ptr &src, return_value_policy policy, handle parent) { if (!src) @@ -724,7 +725,8 @@ struct smart_holder_type_caster> : smart_holder_type_caste if (policy == return_value_policy::automatic) policy = return_value_policy::reference_internal; if (policy != return_value_policy::reference_internal) - throw cast_error("Invalid return_value_policy for unique_ptr&"); + throw cast_error( + "Invalid return_value_policy: unique_ptr const& expects reference_internal"); return smart_holder_type_caster::cast(src.get(), policy, parent); } diff --git a/tests/test_class_sh_basic.py b/tests/test_class_sh_basic.py index 02f814df36..039b27df7b 100644 --- a/tests/test_class_sh_basic.py +++ b/tests/test_class_sh_basic.py @@ -167,8 +167,11 @@ def test_unique_ptr_store_roundtrip(rtrn_f, moved_in): c.pass_uq_valu(orig) # pass object to C++ store c try: recycled = rtrn_f(c) # retrieve object back from C++ - except RuntimeError: # expect failure for rtrn_uq_lref - assert moved_in is None + except RuntimeError as excinfo: # expect failure for rtrn_uq_lref + assert ( + moved_in is None + and "Passing non-const unique_ptr& is not supported" in str(excinfo) + ) return assert m.get_ptr(recycled) == ptr_orig # do we yield the same object? From 1691a844d74b168d4ca59a1d2dc6877087f6ebd0 Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Tue, 23 Mar 2021 01:18:16 +0100 Subject: [PATCH 6/6] Pass arguments from trampoline methods via return_value_policy::reference Otherwise, modifications applied by Python-coded method overrides would be applied to copies, even though the parameters were passed by pointer or reference. --- include/pybind11/detail/smart_holder_type_casters.h | 4 +++- include/pybind11/pybind11.h | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/include/pybind11/detail/smart_holder_type_casters.h b/include/pybind11/detail/smart_holder_type_casters.h index 803cc8b448..c799d255c4 100644 --- a/include/pybind11/detail/smart_holder_type_casters.h +++ b/include/pybind11/detail/smart_holder_type_casters.h @@ -724,7 +724,9 @@ struct smart_holder_type_caster> : smart_holder_type_caste return none().release(); if (policy == return_value_policy::automatic) policy = return_value_policy::reference_internal; - if (policy != return_value_policy::reference_internal) + else if (policy == return_value_policy::reference && !parent) + ; // passing from trampoline dispatcher: no parent available + else if (policy != return_value_policy::reference_internal) throw cast_error( "Invalid return_value_policy: unique_ptr const& expects reference_internal"); return smart_holder_type_caster::cast(src.get(), policy, parent); diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 7ffadcc51b..5c4c301ccd 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -2243,7 +2243,7 @@ template function get_override(const T *this_ptr, const char *name) { pybind11::gil_scoped_acquire gil; \ pybind11::function override = pybind11::get_override(static_cast(this), name); \ if (override) { \ - auto o = override(__VA_ARGS__); \ + auto o = override.operator()(__VA_ARGS__); \ if (pybind11::detail::cast_is_temporary_value_reference::value) { \ static pybind11::detail::override_caster_t caster; \ return pybind11::detail::cast_ref(std::move(o), caster); \