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

* Allow type_caster of std::reference_wrapper<T> to be the same as a native reference.

Before, both std::reference_wrapper<T> and std::reference_wrapper<const T> would
invoke cast_op<type>. This doesn't allow the type_caster<> specialization for T
to distinguish reference_wrapper types from value types.

After, the type_caster<> specialization invokes cast_op<type&>, which allows
reference_wrapper to behave in the same way as a native reference type.

* Add tests/examples for std::reference_wrapper<const T>

* Add tests which use mutable/immutable variants

This test is a chimera; it blends the pybind11 casters with a custom
pytype implementation that supports immutable and mutable calls.

In order to detect the immutable/mutable state, the cast_op needs
to propagate it, even through e.g. std::reference<const T>

Note: This is still a work in progress; some things are crashing,
which likely means that I have a refcounting bug or something else
missing.

* Add/finish tests that distinguish const& from &

Fixes the bugs in my custom python type implementation,
demonstrate test that requires const& and reference_wrapper<const T>
being treated differently from Non-const.

* Add passing a const to non-const method.

* Demonstrate non-const conversion of reference_wrapper in tests.

Apply formatting presubmit check.

* Fix build errors from presubmit checks.

* Try and fix a few more CI errors

* More CI fixes.

* More CI fixups.

* Try and get PyPy to work.

* Additional minor fixups. Getting close to CI green.

* More ci fixes?

* fix clang-tidy warnings from presubmit

* fix more clang-tidy warnings

* minor comment and consistency cleanups

* PyDECREF -> Py_DECREF

* copy/move constructors

* Resolve codereview comments

* more review comment fixes

* review comments: remove spurious &

* Make the test fail even when the static_assert is commented out.

This expands the test_freezable_type_caster a bit by:
1/ adding accessors .is_immutable and .addr to compare identity
from python.
2/ Changing the default cast_op of the type_caster<> specialization
to return a non-const value. In normal codepaths this is a reasonable
default.
3/ adding roundtrip variants to exercise the by reference, by pointer
and by reference_wrapper in all call paths.  In conjunction with 2/, this
demonstrates the failure case of the existing std::reference_wrpper conversion,
which now loses const in a similar way that happens when using the default cast_op_type<>.

* apply presubmit formatting

* Revert inclusion of test_freezable_type_caster

There's some concern that this test is a bit unwieldly because of the use
of the raw <Python.h> functions. Removing for now.

* Add a test that validates const references propagation.

This test verifies that cast_op may be used to correctly detect
const reference types when used with std::reference_wrapper.

* mend

* Review comments based changes.

1. std::add_lvalue_reference<type> -> type&
2. Simplify the test a little more; we're never returning the ConstRefCaster
type so the class_ definition can be removed.

* formatted files again.

* Move const_ref_caster test to builtin_casters

* Review comments: use cast_op and adjust some comments.

* Simplify ConstRefCasted test

I like this version better as it moves the assertion that matters
back into python.

Author: laramiel

include/pybind11/cast.h

@@ -960,9 +960,14 @@ 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 = type&;
+    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 `operator const T &()`");
 public:
     bool load(handle src, bool convert) { return subcaster.load(src, convert); }
     static constexpr auto name = caster_t::name;
@@ -973,7 +978,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 cast_op<type &>(subcaster); }
 };
 
 #define PYBIND11_TYPE_CASTER(type, py_name) \

tests/test_builtin_casters.cpp

@@ -15,6 +15,49 @@
 #  pragma warning(disable: 4127) // warning C4127: Conditional expression is constant
 #endif
 
+struct ConstRefCasted {
+  int tag;
+};
+
+PYBIND11_NAMESPACE_BEGIN(pybind11)
+PYBIND11_NAMESPACE_BEGIN(detail)
+template <>
+class type_caster<ConstRefCasted> {
+ public:
+  static constexpr auto name = _<ConstRefCasted>();
+
+  // Input is unimportant, a new value will always be constructed based on the
+  // cast operator.
+  bool load(handle, bool) { return true; }
+
+  operator ConstRefCasted&&() { value = {1}; return std::move(value); }
+  operator ConstRefCasted&() { value = {2}; return value; }
+  operator ConstRefCasted*() { value = {3}; return &value; }
+
+  operator const ConstRefCasted&() { value = {4}; return value; }
+  operator const ConstRefCasted*() { value = {5}; return &value; }
+
+  // custom cast_op to explicitly propagate types to the conversion operators.
+  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 = {0};
+};
+PYBIND11_NAMESPACE_END(detail)
+PYBIND11_NAMESPACE_END(pybind11)
+
 TEST_SUBMODULE(builtin_casters, m) {
     // test_simple_string
     m.def("string_roundtrip", [](const char *s) { return s; });
@@ -147,6 +190,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>>) { });
@@ -189,4 +243,14 @@ TEST_SUBMODULE(builtin_casters, m) {
         py::object o = py::cast(v);
         return py::cast<void *>(o) == v;
     });
+
+    // Tests const/non-const propagation in cast_op.
+    m.def("takes", [](ConstRefCasted x) { return x.tag; });
+    m.def("takes_move", [](ConstRefCasted&& x) { return x.tag; });
+    m.def("takes_ptr", [](ConstRefCasted* x) { return x->tag; });
+    m.def("takes_ref", [](ConstRefCasted& x) { return x.tag; });
+    m.def("takes_ref_wrap", [](std::reference_wrapper<ConstRefCasted> x) { return x.get().tag; });
+    m.def("takes_const_ptr", [](const ConstRefCasted* x) { return x->tag; });
+    m.def("takes_const_ref", [](const ConstRefCasted& x) { return x.tag; });
+    m.def("takes_const_ref_wrap", [](std::reference_wrapper<const ConstRefCasted> x) { return x.get().tag; });
 }

tests/test_builtin_casters.py

@@ -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)
@@ -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]
@@ -421,3 +425,21 @@ def test_int_long():
 
 def test_void_caster_2():
     assert m.test_void_caster()
+
+
+def test_const_ref_caster():
+    """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.
+    """
+    x = False
+    assert m.takes(x) == 1
+    assert m.takes_move(x) == 1
+
+    assert m.takes_ptr(x) == 3
+    assert m.takes_ref(x) == 2
+    assert m.takes_ref_wrap(x) == 2
+
+    assert m.takes_const_ptr(x) == 5
+    assert m.takes_const_ref(x) == 4
+    assert m.takes_const_ref_wrap(x) == 4