Skip to content

Adjusting type_caster<std::reference_wrapper<T>> to support const/non-const propagation in cast_op. #2705

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 31 commits into from
Dec 16, 2020
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
65abb62
Allow type_caster of std::reference_wrapper<T> to be the same as a na…
laramiel Nov 30, 2020
c603907
Add tests/examples for std::reference_wrapper<const T>
laramiel Nov 30, 2020
5cdc0ca
Add tests which use mutable/immutable variants
laramiel Dec 2, 2020
35e7fa5
Add/finish tests that distinguish const& from &
laramiel Dec 2, 2020
4cd06d3
Add passing a const to non-const method.
laramiel Dec 2, 2020
5abd823
Demonstrate non-const conversion of reference_wrapper in tests.
laramiel Dec 2, 2020
c5bd104
Fix build errors from presubmit checks.
laramiel Dec 2, 2020
f0e41f5
Try and fix a few more CI errors
laramiel Dec 2, 2020
b56a568
More CI fixes.
laramiel Dec 2, 2020
d71cd7a
More CI fixups.
laramiel Dec 2, 2020
3c69635
Try and get PyPy to work.
laramiel Dec 2, 2020
8f32ba3
Additional minor fixups. Getting close to CI green.
laramiel Dec 2, 2020
d144e59
More ci fixes?
laramiel Dec 2, 2020
a7ec72e
fix clang-tidy warnings from presubmit
laramiel Dec 2, 2020
e1aca4b
fix more clang-tidy warnings
laramiel Dec 2, 2020
005d3fe
minor comment and consistency cleanups
laramiel Dec 2, 2020
4ac23ca
PyDECREF -> Py_DECREF
laramiel Dec 2, 2020
a3d9b18
copy/move constructors
laramiel Dec 2, 2020
5b4fb98
Resolve codereview comments
laramiel Dec 3, 2020
8dcbc42
more review comment fixes
laramiel Dec 3, 2020
9f77862
review comments: remove spurious &
laramiel Dec 7, 2020
7672e92
Make the test fail even when the static_assert is commented out.
laramiel Dec 8, 2020
c747ae2
apply presubmit formatting
laramiel Dec 8, 2020
33f57cc
Revert inclusion of test_freezable_type_caster
laramiel Dec 12, 2020
d869976
Add a test that validates const references propagation.
laramiel Dec 14, 2020
f109edd
mend
laramiel Dec 14, 2020
775a7c9
Review comments based changes.
laramiel Dec 14, 2020
6f6dac7
formatted files again.
laramiel Dec 14, 2020
fd6dc5b
Move const_ref_caster test to builtin_casters
laramiel Dec 14, 2020
b3ecc53
Review comments: use cast_op and adjust some comments.
laramiel Dec 15, 2020
c22772b
Simplify ConstRefCasted test
laramiel Dec 15, 2020
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
14 changes: 10 additions & 4 deletions include/pybind11/cast.h
Original file line number Diff line number Diff line change
Expand Up @@ -960,9 +960,15 @@ template <typename type> class type_caster<std::reference_wrapper<type>> {
private:
using caster_t = make_caster<type>;
caster_t subcaster;
using subcaster_cast_op_type = typename caster_t::template cast_op_type<type>;
static_assert(std::is_same<typename std::remove_const<type>::type &, subcaster_cast_op_type>::value,
"std::reference_wrapper<T> caster requires T to have a caster with an `T &` operator");
using reference_t = typename std::add_lvalue_reference<type>::type;
Copy link
Collaborator

@YannickJadoul YannickJadoul Dec 14, 2020

Choose a reason for hiding this comment

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

Why not just type &? Apparently the only difference is void, but I don't think std::reference_wrapper<void> is supported, or is it?

Copy link
Collaborator

@YannickJadoul YannickJadoul Dec 14, 2020

Choose a reason for hiding this comment

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

To be more specific, type_caster<std::reference_wrapper<void>> would already result in a compilation error a few lines further, to evaluate std::is_same<typename std::remove_const<type>::type &, subcaster_cast_op_type>::value: typename std::remove_const<void>::type is still void, and would also result in void &, and thus a compilation error.

According to cppreference.com, this is where add_lvalue_reference<T> differs from T&: https://en.cppreference.com/w/cpp/types/add_reference

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.

using subcaster_cast_op_type =
typename caster_t::template cast_op_type<reference_t>;

static_assert((std::is_same<typename std::remove_const<type>::type &,
subcaster_cast_op_type>::value ||
std::is_same<reference_t, subcaster_cast_op_type>::value),
"std::reference_wrapper<T> caster requires T to have a caster "
"with an operator `T &` or `const T&`");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this line isn't as long as this one below:

static handle cast(const std::reference_wrapper<type> &src, return_value_policy policy, handle parent) {

It's probably more clear if we put it like this? (also with correct indentation and with const T & instead of const T&, like the rest of pybind11's codebase)

Suggested change
static_assert((std::is_same<typename std::remove_const<type>::type &,
subcaster_cast_op_type>::value ||
std::is_same<reference_t, subcaster_cast_op_type>::value),
"std::reference_wrapper<T> caster requires T to have a caster "
"with an operator `T &` or `const T&`");
static_assert(std::is_same<typename std::remove_const<type>::type &, subcaster_cast_op_type>::value ||
std::is_same<reference_t, subcaster_cast_op_type>::value,
"std::reference_wrapper<T> caster requires T to have a caster with an "
"`operator T &()` or `operator const T &()`");

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.

public:
bool load(handle src, bool convert) { return subcaster.load(src, convert); }
static constexpr auto name = caster_t::name;
Expand All @@ -973,7 +979,7 @@ template <typename type> class type_caster<std::reference_wrapper<type>> {
return caster_t::cast(&src.get(), policy, parent);
}
template <typename T> using cast_op_type = std::reference_wrapper<type>;
operator std::reference_wrapper<type>() { return subcaster.operator subcaster_cast_op_type&(); }
operator std::reference_wrapper<type>() { return subcaster.operator subcaster_cast_op_type(); }
Copy link
Collaborator

@YannickJadoul YannickJadoul Dec 14, 2020

Choose a reason for hiding this comment

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

Suggested change
operator std::reference_wrapper<type>() { return subcaster.operator subcaster_cast_op_type(); }
operator std::reference_wrapper<type>() { return cast_op<reference_t>(subcaster); }

or even (depending on the other discussion)

Suggested change
operator std::reference_wrapper<type>() { return subcaster.operator subcaster_cast_op_type(); }
operator std::reference_wrapper<type>() { return cast_op<type &>(subcaster); }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you already listed point 3/ where I removed a & based on review feedback as a "separate change"; how is this not the same?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you mean? I don't mean this should be taken out, but if you go and change std::add_lvalue_reference<type> to type &, this part can also be simplified?

Copy link
Collaborator

@YannickJadoul YannickJadoul Dec 14, 2020

Choose a reason for hiding this comment

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

If you don't agree with the suggestion in #2705 (comment), that's also fine with me; then just argue why not and do not do it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean operator std::reference_wrapper<type>() { return cast_op<type &>(subcaster); } is clearly a more substantive change than removing & from operator std::reference_wrapper<type>() { return subcaster.operator subcaster_cast_op_type(); }

Copy link
Collaborator

Choose a reason for hiding this comment

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

True, yes. I just saw this being used in other casters.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm also fine with not applying this change; it's supposed to do exactly the same, though.

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.

};

#define PYBIND11_TYPE_CASTER(type, py_name) \
Expand Down
1 change: 1 addition & 0 deletions tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ set(PYBIND11_TEST_FILES
test_callbacks.cpp
test_chrono.cpp
test_class.cpp
test_const_ref_caster.cpp
test_constants_and_functions.cpp
test_copy_move.cpp
test_custom_type_casters.cpp
Expand Down
11 changes: 11 additions & 0 deletions tests/test_builtin_casters.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,17 @@ TEST_SUBMODULE(builtin_casters, m) {
// test_reference_wrapper
m.def("refwrap_builtin", [](std::reference_wrapper<int> p) { return 10 * p.get(); });
m.def("refwrap_usertype", [](std::reference_wrapper<UserType> p) { return p.get().value(); });
m.def("refwrap_usertype_const", [](std::reference_wrapper<const UserType> p) { return p.get().value(); });

m.def("refwrap_lvalue", []() -> std::reference_wrapper<UserType> {
static UserType x(1);
return std::ref(x);
});
m.def("refwrap_lvalue_const", []() -> std::reference_wrapper<const UserType> {
static UserType x(1);
return std::cref(x);
});

// Not currently supported (std::pair caster has return-by-value cast operator);
// triggers static_assert failure.
//m.def("refwrap_pair", [](std::reference_wrapper<std::pair<int, int>>) { });
Expand Down
4 changes: 4 additions & 0 deletions tests/test_builtin_casters.py
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,7 @@ def test_reference_wrapper():
"""std::reference_wrapper for builtin and user types"""
assert m.refwrap_builtin(42) == 420
assert m.refwrap_usertype(UserType(42)) == 42
assert m.refwrap_usertype_const(UserType(42)) == 42

with pytest.raises(TypeError) as excinfo:
m.refwrap_builtin(None)
Expand All @@ -324,6 +325,9 @@ def test_reference_wrapper():
m.refwrap_usertype(None)
assert "incompatible function arguments" in str(excinfo.value)

assert m.refwrap_lvalue().value == 1
assert m.refwrap_lvalue_const().value == 1

a1 = m.refwrap_list(copy=True)
a2 = m.refwrap_list(copy=True)
assert [x.value for x in a1] == [2, 3]
Expand Down
88 changes: 88 additions & 0 deletions tests/test_const_ref_caster.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/*
tests/test_const_ref_caster.cpp
Copy link
Collaborator

Choose a reason for hiding this comment

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

These tests seem more appropriate in test_builtin_casters.cpp/test_builtin_casters.py.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps, but I think that since they are intended to exercise different aspects, putting it into a separate file makes the intent a bit more clear.

Copy link
Collaborator

@YannickJadoul YannickJadoul Dec 14, 2020

Choose a reason for hiding this comment

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

Yes, pybind11's tests are a mess. But I think the idea it to keep a minimal amount of files, because they all take quite some time to compile. I also don't think this one test is enough to create a separate file for.

Meanwhile, type_caster<std::reference_wrapper<T>> is already tested in test_builtin_casters.cpp and other test files already have custom casters as well (test_custom_type_casters.cpp, test_copy_move.cpp, and even one in pybind11_tests.h). So if we can keep the scope of the caster & test focused enough, I think it's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other counter point is that this requires a custom type caster; so maybe move them to custom_type_casters.cc? I think that the current test is about as minimal as it can be while exercising the various paths we want to test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just logging my view, not a request: it makes me sad to see this test lumped in with other things. I know the existing tests do that a lot, but I think it really isn't a good approach, because it convolutes unrelated things, makes them harder to understand/follow, and obviously is counter the the idea of testing small self-contained units. Sticking with the established pattern only has so much value. Nudging things in a better direction has value, too. My preference is definitely to keep this test separate, if Boris or Yannick can agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anyway, Done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with @rwgk however I will change it to the reviewers consensus.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The other counter point is that this requires a custom type caster; so maybe move them to custom_type_casters.cc? I think that the current test is about as minimal as it can be while exercising the various paths we want to test.

Well, coming from this PR, it's testing the built-in caster for std::reference_wrapper, so I thought it would be best to keep this as close to the other tests for reference_wrapper, and to not create yet another file is "something with casters" (which takes time to compile, each time!).

move them to custom_type_casters.cc

It's not really about the custom caster, though. It's about testing reference_wrapper, currently, I though? If this maybe gets expanded further in the future, the caster could be shared in a header, though, when it's necessary.

it makes me sad to see this test lumped in with other things.

Why exactly? Because I had explicitly tried to design/shrink this to be as minimal, unit-testy as possible, such that it could fit in with the rest of the std::reference_wrapper tests (that were even extended) in builtin_casters.cpp.

And to me it's more confusing to break convention than to add a few lines to the existing tests.

I do agree the tests are messy, btw, but I don't see the urgency of splitting things up. Build times are (partially) to blame, I think. So maybe once #2445 gets merged, it will be more feasible to split up tests, in a separate PR that brings order to the tests in a less ad-hoc fashion?

I agree with @rwgk however I will change it to the reviewers consensus.

Not sure there's consensus, though. If you want, we can give @henryiii a decisive vote, but ... well, yeah, I've explained my reasoning :-)


This test verifies that const-ref is propagated through type_caster cast_op.

The returned ConstRefCasted type is a mimimal type that is constructed to
reference the casting mode used.
*/

#include <pybind11/complex.h>

#include "pybind11_tests.h"


struct ConstRefCasted {
bool is_const;
bool is_ref;
};

PYBIND11_NAMESPACE_BEGIN(pybind11)
PYBIND11_NAMESPACE_BEGIN(detail)
template <>
class type_caster<ConstRefCasted> {
public:
static constexpr auto name = _<ConstRefCasted>();

bool load(handle, bool) { return true; }

operator ConstRefCasted&&() { value = {false, false}; return std::move(value); }
operator ConstRefCasted&() { value = {false, true}; return value; }
operator ConstRefCasted*() { value = {false, false}; return &value; }

operator const ConstRefCasted&() { value = {true, true}; return value; }
operator const ConstRefCasted*() { value = {true, false}; return &value; }

//
template <typename T_>
using cast_op_type =
/// const
conditional_t<
std::is_same<remove_reference_t<T_>, const ConstRefCasted*>::value, const ConstRefCasted*,
conditional_t<
std::is_same<T_, const ConstRefCasted&>::value, const ConstRefCasted&,
// non-const
conditional_t<
std::is_same<remove_reference_t<T_>, ConstRefCasted*>::value, ConstRefCasted*,
conditional_t<
std::is_same<T_, ConstRefCasted&>::value, ConstRefCasted&,
/* else */ConstRefCasted&&>>>>;

private:
ConstRefCasted value = {false, false};
};
PYBIND11_NAMESPACE_END(detail)
PYBIND11_NAMESPACE_END(pybind11)

TEST_SUBMODULE(const_ref_caster, m) {
py::class_<ConstRefCasted>(m, "ConstRefCasted",
"A `py::class_` type for testing")
.def(py::init<>())
.def_readonly("is_const", &ConstRefCasted::is_const)
.def_readonly("is_ref", &ConstRefCasted::is_ref);


m.def("takes", [](ConstRefCasted x) {
return !x.is_const && !x.is_ref;
});
m.def("takes_ptr", [](ConstRefCasted* x) {
return !x->is_const && !x->is_ref;
});
m.def("takes_ref", [](ConstRefCasted& x) {
return !x.is_const && x.is_ref;
});
m.def("takes_ref_wrap", [](std::reference_wrapper<ConstRefCasted> x) {
return !x.get().is_const && x.get().is_ref;
});

m.def("takes_const_ptr", [](const ConstRefCasted* x) {
return x->is_const && !x->is_ref;
});
m.def("takes_const_ref", [](const ConstRefCasted& x) {
return x.is_const && x.is_ref;
});
m.def("takes_const_ref_wrap",
[](std::reference_wrapper<const ConstRefCasted> x) {
return x.get().is_const && x.get().is_ref;
});
}
18 changes: 18 additions & 0 deletions tests/test_const_ref_caster.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# -*- coding: utf-8 -*-
import pytest

import env # noqa: F401

from pybind11_tests import const_ref_caster as m


def test_takes():
assert m.takes(m.ConstRefCasted())

assert m.takes_ptr(m.ConstRefCasted())
assert m.takes_ref(m.ConstRefCasted())
assert m.takes_ref_wrap(m.ConstRefCasted())

assert m.takes_const_ptr(m.ConstRefCasted())
assert m.takes_const_ref(m.ConstRefCasted())
assert m.takes_const_ref_wrap(m.ConstRefCasted())