Skip to content

Missing cast from const unique_ptr& #2904

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 17, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions include/pybind11/detail/smart_holder_type_casters.h
Original file line number Diff line number Diff line change
Expand Up @@ -716,6 +716,15 @@ struct smart_holder_type_caster<std::unique_ptr<T, D>> : smart_holder_type_caste

return inst.release();
}
static handle cast(const std::unique_ptr<T, D> &src, return_value_policy policy, handle parent) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha :-)
That makes everything clear immediately, thanks!
Initial thought: This is very similar to static handle cast(const T *src, ...). We could mimic that implementation, or maybe even reuse? The easy case is that an instance exists already and we just incref as you suggest. Or there needs to be return_value_policy::reference_internal or an explicit keep_alive.
Is there a chance you could help with that implementation? It's unlikely that I can prioritize working on this myself anytime soon, unless I hit on a use case in PyCLIF (difficult to predict). But I will review & merge very quickly if you take the lead.
My revised suggestion for a separate new unit test name iff we cannot reuse static handle cast(const T *src, ...): test_class_sh_const_unique_return
Keeping the tests separate helps minimizing merge issues (between master, drake, smart_holder) and is best practice IMO regardless.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks for the hint to cast(const T*). Regarding the unit test: I think we should have a unit test in any case.
Do you suggest to move the test into a new file, named test_class_sh_const_unique_return.cpp or to include a test named test_class_sh_const_unique_return into some existing file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. I don't understand why CI fails, while locally it works fine. Need to investigate later...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry to be ambiguous: I was thinking of test_class_sh_const_unique_return.cpp and .py.
I think I also want to add a few lines to test_class_sh_basic.cpp and .py, but that's a small enough effort I could squeeze that in myself.

if (!src)
return none().release();
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&");
return smart_holder_type_caster<T>::cast(src.get(), policy, parent);
}

template <typename>
using cast_op_type = std::unique_ptr<T, D>;
Expand Down
29 changes: 29 additions & 0 deletions tests/test_class_sh_basic.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,17 @@ struct atyp { // Short for "any type".
atyp(atyp &&other) { mtxt = other.mtxt + "_MvCtor"; }
};

struct uconsumer { // unique_ptr consumer
std::unique_ptr<atyp> held;
bool valid() const { return static_cast<bool>(held); }

void pass_valu(std::unique_ptr<atyp> obj) { held = std::move(obj); }
void pass_rref(std::unique_ptr<atyp> &&obj) { held = std::move(obj); }
std::unique_ptr<atyp> rtrn_valu() { return std::move(held); }
std::unique_ptr<atyp>& rtrn_lref() { return held; }
const std::unique_ptr<atyp> &rtrn_cref() { return held; }
};

// clang-format off

atyp rtrn_valu() { atyp obj{"rtrn_valu"}; return obj; }
Expand Down Expand Up @@ -57,7 +68,11 @@ std::string pass_udcp(std::unique_ptr<atyp const, sddc> 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<std::ptrdiff_t>(&obj); }

std::unique_ptr<atyp> unique_ptr_roundtrip(std::unique_ptr<atyp> obj) { return obj; }
const std::unique_ptr<atyp>& unique_ptr_cref_roundtrip(const std::unique_ptr<atyp>& obj) { return obj; }

struct SharedPtrStash {
std::vector<std::shared_ptr<const atyp>> stash;
void Add(std::shared_ptr<const atyp> obj) { stash.push_back(obj); }
Expand All @@ -67,6 +82,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::SharedPtrStash)

namespace pybind11_tests {
Expand Down Expand Up @@ -112,10 +128,23 @@ TEST_SUBMODULE(class_sh_basic, m) {
m.def("pass_udmp", pass_udmp);
m.def("pass_udcp", pass_udcp);

py::classh<uconsumer>(m, "uconsumer")
.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);

// Helpers for testing.
// These require selected functions above to work first, as indicated:
m.def("get_mtxt", get_mtxt); // pass_cref
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<SharedPtrStash>(m, "SharedPtrStash")
.def(py::init<>())
.def("Add", &SharedPtrStash::Add, py::arg("obj"));
Expand Down
42 changes: 42 additions & 0 deletions tests/test_class_sh_basic.py
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,48 @@ 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)
mtxt_orig = m.get_mtxt(orig)

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


@pytest.mark.parametrize(
"pass_f, rtrn_f, moved_out, 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),
],
)
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_py_type_handle_of_atyp():
obj = m.py_type_handle_of_atyp()
assert obj.__class__.__name__ == "pybind11_type"