[BUG]: return_value_policy_override in optional_caster does not use the right types

[ryancahoon-zoox]

Required prerequisites

• Make sure you've read the documentation. Your issue may be addressed there.
• Search the issue tracker and Discussions to verify that this hasn't already been reported. +1 or comment there if it has.
• Consider asking first in the Gitter chat room or in a Discussion.

Problem description

return_value_policy_override is not being applied correctly in optional_caster in two ways.

• The is_lvalue_reference condition references T, which is the optional<T> type parameter from the class, when it should use T_, which is the parameter to the cast function. T_ can potentially be a reference type, but T will never be.
• The type parameter passed to return_value_policy_override should be T::value_type, not T. This matches the way that the other STL container type casters work.

The result of these issues is that a method/property definition which uses a reference or reference_internal return value policy will create a Python value that's bound by reference to a temporary C++ object, resulting in undefined behavior. For reasons that I was not able to figure out, it seems like this causes problems when using boost::optional, but not with the libstdc++ implementation of std::optional. The issue (that the override to return_value_policy::move is never being applied) is present for all implementations, it just seems like that somehow doesn't result in problems for the libstdc++ implementation.

I believe the following patch is the correct resolution. I would open a PR directly, except I'm not sure how to write a regression test for this case.

diff --git a/include/pybind11/stl.h b/include/pybind11/stl.h
index 2c017b4f..5c0592f8 100644
--- a/include/pybind11/stl.h
+++ b/include/pybind11/stl.h
@@ -252,8 +252,8 @@ template<typename T> struct optional_caster {
     static handle cast(T_ &&src, return_value_policy policy, handle parent) {
         if (!src)
             return none().inc_ref();
-        if (!std::is_lvalue_reference<T>::value) {
-            policy = return_value_policy_override<T>::policy(policy);
+        if (!std::is_lvalue_reference<T_>::value) {
+            policy = return_value_policy_override<typename T::value_type>::policy(policy);
         }
         return value_conv::cast(*std::forward<T_>(src), policy, parent);
     }

Reproducible example code

#include <boost/optional.hpp>
#include <pybind11/pybind11.h>
#include <pybind11/stl.h>

enum class E {
  k0 = 0,
  k1 = 1,
};

struct A {
  boost::optional<E> value = E::k1;
};

namespace pybind11 {
namespace detail {

template <typename T>
struct type_caster<boost::optional<T>> : optional_caster<boost::optional<T>> {};

} // namespace detail
} // namespace pybind11

PYBIND11_MODULE(test_lib, m) {
  pybind11::enum_<E>(m, "E")
      .value("k0", E::k0)
      .value("k1", E::k1);

  pybind11::class_<A>(m, "A")
      .def(pybind11::init<>())
      .def_property_readonly("by_ref",
                             [](A& a) -> boost::optional<E>& { return a.value; })
      .def_property_readonly("by_copy",
                             [](A& a) -> boost::optional<E> { return a.value; });
}




import test_lib


print(int(test_lib.A().by_ref))
print(int(test_lib.A().by_copy))

[Skylion007]

@ryancahoon-zoox For future reference, feel free to just open a PR, we can work on adding regressions tests there if necessary.

[rwgk]

Hi @ryancahoon-zoox, is there a chance that you could help by moving your code to a PR?

We already have the ability to conditionally test with boost:

pybind11/tests/test_stl.cpp

Line 25 in ba9f919

#elif defined(PYBIND11_TEST_BOOST) && (!defined(_MSC_VER) || _MSC_VER >= 1910)

Following that pattern would be a practical way to leverage your existing reproducer.

Please mark the PR as modifiable by maintainers (I think it's the default, a green checkbox when you create the PR). That way we can jump in fixing issues surfaced by the CI.

[henryiii]

@ryancahoon-zoox are you planning to open a PR with your boost example as a test?

[ryancahoon-zoox]

I've been trying to work on it as I have time, but I'm having trouble reproducing my original issue in the test environment. There's a number of factors that I need to check still, like whether there are some versions of boost that aren't affected by this issue.

[henryiii]

We have had no problem in reproducing it in #3332, AFAICT, the problem has been compiling the new test on Windows; MSVC doesn't like it at all.

[Skylion007]

@henryiii I've haven't been able to replicate the segfault behavior in the PR, but I was just going ahead and added the boost::optional tests since they are useful regardless.

[ryancahoon-zoox]

I've reproduced the issue after switching to Boost 1.54 (which is what is used in the software environment where I originally observed the bug), whereas the issue does not seem to be reproduced with Boost 1.71. I've not bothered to check intermediate versions, so I'm not sure exactly when boost::optional might have changed.

For the purposes of testing, I'm going to write a simple optional-like type that reproduces the necessary parts of the old boost::optional behavior.

[rwgk]

Awesome, thanks a lot for working on the tailored test!

…

On Sun, Oct 17, 2021 at 14:00 Ryan Cahoon ***@***.***> wrote: I've reproduced the issue after switching to Boost 1.54 (which is what is used in the software environment where I originally observed the bug), whereas the issue does not seem to be reproduced with Boost 1.71. I've not bothered to check intermediate versions, so I'm not sure exactly when boost::optional might have changed. For the purposes of testing, I'm going to write a simple optional-like type that reproduces the necessary parts of the old boost::optional behavior. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3330 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFUZAAGDB2AUHJYFCGYMODUHM2QJANCNFSM5FL4M3CA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.