From c2d3e220bd1bfcf664e29d95ac2ed774fd6723f4 Mon Sep 17 00:00:00 2001
From: Ryan Cahoon <ryan.cahoon@zoox.com>
Date: Mon, 25 Oct 2021 19:04:45 -0700
Subject: [PATCH] fix: the types for return_value_policy_override in
 optional_caster (#3376)

* fix: the types for return_value_policy_override in optional_caster

`return_value_policy_override` was not being applied correctly in
`optional_caster` in two ways:
- The `is_lvalue_reference` condition referenced `T`, which was the
`optional<T>` type parameter from the class, when it should have used `T_`,
which was 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 was that a method/property definition which used a
`reference` or `reference_internal` return value policy would 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 fully, it
seems like this causes problems when using old versions of `boost::optional`,
but not with recent versions of `boost::optional` or 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 some implementation of `optional`. This change includes a regression type
with a custom optional-like type which was able to reproduce the issue.

Part of the issue with using the wrong types may have stemmed from the type
variables `T` and `T_` having very similar names. This also changes the type
variables in `optional_caster` to use slightly more descriptive names, which
also more closely follow the naming convention used by the other STL casters.

Fixes #3330

* Fix clang-tidy complaints

* Add missing NOLINT

* Apply a couple more fixes

* fix: support GCC 4.8

* tests: avoid warning about unknown compiler for compilers missing C++17

* Remove unneeded test module attribute

* Change test enum to have more unique int values

Co-authored-by: Aaron Gokaslan <skylion.aaron@gmail.com>
Co-authored-by: Henry Schreiner <HenrySchreinerIII@gmail.com>
---
 include/pybind11/stl.h |  16 ++--
 tests/CMakeLists.txt   |   6 +-
 tests/test_stl.cpp     | 189 ++++++++++++++++++++++++++++++++++++++++-
 tests/test_stl.py      |  67 +++++++++++++++
 4 files changed, 265 insertions(+), 13 deletions(-)

diff --git a/include/pybind11/stl.h b/include/pybind11/stl.h
index 2c017b4f..3608d298 100644
--- a/include/pybind11/stl.h
+++ b/include/pybind11/stl.h
@@ -245,17 +245,17 @@ template <typename Key, typename Value, typename Hash, typename Equal, typename
   : map_caster<std::unordered_map<Key, Value, Hash, Equal, Alloc>, Key, Value> { };
 
 // This type caster is intended to be used for std::optional and std::experimental::optional
-template<typename T> struct optional_caster {
-    using value_conv = make_caster<typename T::value_type>;
+template<typename Type, typename Value = typename Type::value_type> struct optional_caster {
+    using value_conv = make_caster<Value>;
 
-    template <typename T_>
-    static handle cast(T_ &&src, return_value_policy policy, handle parent) {
+    template <typename T>
+    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);
+            policy = return_value_policy_override<Value>::policy(policy);
         }
-        return value_conv::cast(*std::forward<T_>(src), policy, parent);
+        return value_conv::cast(*std::forward<T>(src), policy, parent);
     }
 
     bool load(handle src, bool convert) {
@@ -269,11 +269,11 @@ template<typename T> struct optional_caster {
         if (!inner_caster.load(src, convert))
             return false;
 
-        value.emplace(cast_op<typename T::value_type &&>(std::move(inner_caster)));
+        value.emplace(cast_op<Value &&>(std::move(inner_caster)));
         return true;
     }
 
-    PYBIND11_TYPE_CASTER(T, _("Optional[") + value_conv::name + _("]"));
+    PYBIND11_TYPE_CASTER(Type, _("Optional[") + value_conv::name + _("]"));
 };
 
 #if defined(PYBIND11_HAS_OPTIONAL)
diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
index d4e7b71e..e1b18dd7 100644
--- a/tests/CMakeLists.txt
+++ b/tests/CMakeLists.txt
@@ -256,7 +256,9 @@ if(Boost_FOUND)
 endif()
 
 # Check if we need to add -lstdc++fs or -lc++fs or nothing
-if(MSVC)
+if(DEFINED CMAKE_CXX_STANDARD AND CMAKE_CXX_STANDARD LESS 17)
+  set(STD_FS_NO_LIB_NEEDED TRUE)
+elseif(MSVC)
   set(STD_FS_NO_LIB_NEEDED TRUE)
 else()
   file(
@@ -286,7 +288,7 @@ elseif(${STD_FS_NEEDS_CXXFS})
 elseif(${STD_FS_NO_LIB_NEEDED})
   set(STD_FS_LIB "")
 else()
-  message(WARNING "Unknown compiler - not passing -lstdc++fs")
+  message(WARNING "Unknown C++17 compiler - not passing -lstdc++fs")
   set(STD_FS_LIB "")
 endif()
 
diff --git a/tests/test_stl.cpp b/tests/test_stl.cpp
index 7e3363c5..bc5c6553 100644
--- a/tests/test_stl.cpp
+++ b/tests/test_stl.cpp
@@ -19,6 +19,18 @@
 #include <vector>
 #include <string>
 
+#if defined(PYBIND11_TEST_BOOST)
+#include <boost/optional.hpp>
+
+namespace pybind11 { namespace detail {
+template <typename T>
+struct type_caster<boost::optional<T>> : optional_caster<boost::optional<T>> {};
+
+template <>
+struct type_caster<boost::none_t> : void_caster<boost::none_t> {};
+}} // namespace pybind11::detail
+#endif
+
 // Test with `std::variant` in C++17 mode, or with `boost::variant` in C++11/14
 #if defined(PYBIND11_HAS_VARIANT)
 using std::variant;
@@ -59,7 +71,8 @@ namespace std {
 template <template <typename> class OptionalImpl, typename T>
 struct OptionalHolder
 {
-    OptionalHolder() = default;
+    // NOLINTNEXTLINE(modernize-use-equals-default): breaks GCC 4.8
+    OptionalHolder() {};
     bool member_initialized() const {
         return member && member->initialized;
     }
@@ -67,6 +80,95 @@ struct OptionalHolder
 };
 
 
+enum class EnumType {
+  kSet = 42,
+  kUnset = 85,
+};
+
+// This is used to test that return-by-ref and return-by-copy policies are
+// handled properly for optional types. This is a regression test for a dangling
+// reference issue. The issue seemed to require the enum value type to
+// reproduce - it didn't seem to happen if the value type is just an integer.
+template <template <typename> class OptionalImpl>
+class OptionalProperties {
+public:
+    using OptionalEnumValue = OptionalImpl<EnumType>;
+
+    OptionalProperties() : value(EnumType::kSet) {}
+    ~OptionalProperties() {
+        // Reset value to detect use-after-destruction.
+        // This is set to a specific value rather than nullopt to ensure that
+        // the memory that contains the value gets re-written.
+        value = EnumType::kUnset;
+    }
+
+    OptionalEnumValue& access_by_ref() { return value; }
+    OptionalEnumValue access_by_copy() { return value; }
+
+private:
+    OptionalEnumValue value;
+};
+
+// This type mimics aspects of boost::optional from old versions of Boost,
+// which exposed a dangling reference bug in Pybind11. Recent versions of
+// boost::optional, as well as libstdc++'s std::optional, don't seem to be
+// affected by the same issue. This is meant to be a minimal implementation
+// required to reproduce the issue, not fully standard-compliant.
+// See issue #3330 for more details.
+template <typename T>
+class ReferenceSensitiveOptional {
+public:
+    using value_type = T;
+
+    ReferenceSensitiveOptional() = default;
+    // NOLINTNEXTLINE(google-explicit-constructor)
+    ReferenceSensitiveOptional(const T& value) : storage{value} {}
+    // NOLINTNEXTLINE(google-explicit-constructor)
+    ReferenceSensitiveOptional(T&& value) : storage{std::move(value)} {}
+    ReferenceSensitiveOptional& operator=(const T& value) {
+        storage = {value};
+        return *this;
+    }
+    ReferenceSensitiveOptional& operator=(T&& value) {
+        storage = {std::move(value)};
+        return *this;
+    }
+
+    template <typename... Args>
+    T& emplace(Args&&... args) {
+        storage.clear();
+        storage.emplace_back(std::forward<Args>(args)...);
+        return storage.back();
+    }
+
+    const T& value() const noexcept {
+        assert(!storage.empty());
+        return storage[0];
+    }
+
+    const T& operator*() const noexcept {
+        return value();
+    }
+
+    const T* operator->() const noexcept {
+        return &value();
+    }
+
+    explicit operator bool() const noexcept {
+        return !storage.empty();
+    }
+
+private:
+    std::vector<T> storage;
+};
+
+namespace pybind11 { namespace detail {
+template <typename T>
+struct type_caster<ReferenceSensitiveOptional<T>> : optional_caster<ReferenceSensitiveOptional<T>> {};
+} // namespace detail
+} // namespace pybind11
+
+
 TEST_SUBMODULE(stl, m) {
     // test_vector
     m.def("cast_vector", []() { return std::vector<int>{1}; });
@@ -145,6 +247,10 @@ TEST_SUBMODULE(stl, m) {
         return v;
     });
 
+    pybind11::enum_<EnumType>(m, "EnumType")
+        .value("kSet", EnumType::kSet)
+        .value("kUnset", EnumType::kUnset);
+
     // test_move_out_container
     struct MoveOutContainer {
         struct Value { int value; };
@@ -213,6 +319,12 @@ TEST_SUBMODULE(stl, m) {
         .def(py::init<>())
         .def_readonly("member", &opt_holder::member)
         .def("member_initialized", &opt_holder::member_initialized);
+
+    using opt_props = OptionalProperties<std::optional>;
+    pybind11::class_<opt_props>(m, "OptionalProperties")
+        .def(pybind11::init<>())
+        .def_property_readonly("access_by_ref", &opt_props::access_by_ref)
+        .def_property_readonly("access_by_copy", &opt_props::access_by_copy);
 #endif
 
 #ifdef PYBIND11_HAS_EXP_OPTIONAL
@@ -239,8 +351,75 @@ TEST_SUBMODULE(stl, m) {
         .def(py::init<>())
         .def_readonly("member", &opt_exp_holder::member)
         .def("member_initialized", &opt_exp_holder::member_initialized);
+
+    using opt_exp_props = OptionalProperties<std::experimental::optional>;
+    pybind11::class_<opt_exp_props>(m, "OptionalExpProperties")
+        .def(pybind11::init<>())
+        .def_property_readonly("access_by_ref", &opt_exp_props::access_by_ref)
+        .def_property_readonly("access_by_copy", &opt_exp_props::access_by_copy);
 #endif
 
+#if defined(PYBIND11_TEST_BOOST)
+    // test_boost_optional
+    m.attr("has_boost_optional") = true;
+
+    using boost_opt_int = boost::optional<int>;
+    using boost_opt_no_assign = boost::optional<NoAssign>;
+    m.def("double_or_zero_boost", [](const boost_opt_int& x) -> int {
+        return x.value_or(0) * 2;
+    });
+    m.def("half_or_none_boost", [](int x) -> boost_opt_int {
+        return x != 0 ? boost_opt_int(x / 2) : boost_opt_int();
+    });
+    m.def("test_nullopt_boost", [](boost_opt_int x) {
+        return x.value_or(42);
+    }, py::arg_v("x", boost::none, "None"));
+    m.def("test_no_assign_boost", [](const boost_opt_no_assign &x) {
+        return x ? x->value : 42;
+    }, py::arg_v("x", boost::none, "None"));
+
+    using opt_boost_holder = OptionalHolder<boost::optional, MoveOutDetector>;
+    py::class_<opt_boost_holder>(m, "OptionalBoostHolder", "Class with optional member")
+        .def(py::init<>())
+        .def_readonly("member", &opt_boost_holder::member)
+        .def("member_initialized", &opt_boost_holder::member_initialized);
+
+    using opt_boost_props = OptionalProperties<boost::optional>;
+    pybind11::class_<opt_boost_props>(m, "OptionalBoostProperties")
+        .def(pybind11::init<>())
+        .def_property_readonly("access_by_ref", &opt_boost_props::access_by_ref)
+        .def_property_readonly("access_by_copy", &opt_boost_props::access_by_copy);
+#endif
+
+    // test_refsensitive_optional
+    using refsensitive_opt_int = ReferenceSensitiveOptional<int>;
+    using refsensitive_opt_no_assign = ReferenceSensitiveOptional<NoAssign>;
+    m.def("double_or_zero_refsensitive", [](const refsensitive_opt_int& x) -> int {
+        return (x ? x.value() : 0) * 2;
+    });
+    m.def("half_or_none_refsensitive", [](int x) -> refsensitive_opt_int {
+        return x != 0 ? refsensitive_opt_int(x / 2) : refsensitive_opt_int();
+    });
+    // NOLINTNEXTLINE(performance-unnecessary-value-param)
+    m.def("test_nullopt_refsensitive", [](refsensitive_opt_int x) {
+        return x ? x.value() : 42;
+    }, py::arg_v("x", refsensitive_opt_int(), "None"));
+    m.def("test_no_assign_refsensitive", [](const refsensitive_opt_no_assign &x) {
+        return x ? x->value : 42;
+    }, py::arg_v("x", refsensitive_opt_no_assign(), "None"));
+
+    using opt_refsensitive_holder = OptionalHolder<ReferenceSensitiveOptional, MoveOutDetector>;
+    py::class_<opt_refsensitive_holder>(m, "OptionalRefSensitiveHolder", "Class with optional member")
+        .def(py::init<>())
+        .def_readonly("member", &opt_refsensitive_holder::member)
+        .def("member_initialized", &opt_refsensitive_holder::member_initialized);
+
+    using opt_refsensitive_props = OptionalProperties<ReferenceSensitiveOptional>;
+    pybind11::class_<opt_refsensitive_props>(m, "OptionalRefSensitiveProperties")
+        .def(pybind11::init<>())
+        .def_property_readonly("access_by_ref", &opt_refsensitive_props::access_by_ref)
+        .def_property_readonly("access_by_copy", &opt_refsensitive_props::access_by_copy);
+
 #ifdef PYBIND11_HAS_FILESYSTEM
     // test_fs_path
     m.attr("has_filesystem") = true;
@@ -280,8 +459,12 @@ TEST_SUBMODULE(stl, m) {
     m.def("tpl_ctor_set", [](std::unordered_set<TplCtorClass> &) {});
 #if defined(PYBIND11_HAS_OPTIONAL)
     m.def("tpl_constr_optional", [](std::optional<TplCtorClass> &) {});
-#elif defined(PYBIND11_HAS_EXP_OPTIONAL)
-    m.def("tpl_constr_optional", [](std::experimental::optional<TplCtorClass> &) {});
+#endif
+#if defined(PYBIND11_HAS_EXP_OPTIONAL)
+    m.def("tpl_constr_optional_exp", [](std::experimental::optional<TplCtorClass> &) {});
+#endif
+#if defined(PYBIND11_TEST_BOOST)
+    m.def("tpl_constr_optional_boost", [](boost::optional<TplCtorClass> &) {});
 #endif
 
     // test_vec_of_reference_wrapper
diff --git a/tests/test_stl.py b/tests/test_stl.py
index 3f63d6c3..e2179759 100644
--- a/tests/test_stl.py
+++ b/tests/test_stl.py
@@ -132,6 +132,10 @@ def test_optional():
     assert mvalue.initialized
     assert holder.member_initialized()
 
+    props = m.OptionalProperties()
+    assert int(props.access_by_ref) == 42
+    assert int(props.access_by_copy) == 42
+
 
 @pytest.mark.skipif(
     not hasattr(m, "has_exp_optional"), reason="no <experimental/optional>"
@@ -160,6 +164,69 @@ def test_exp_optional():
     assert mvalue.initialized
     assert holder.member_initialized()
 
+    props = m.OptionalExpProperties()
+    assert int(props.access_by_ref) == 42
+    assert int(props.access_by_copy) == 42
+
+
+@pytest.mark.skipif(not hasattr(m, "has_boost_optional"), reason="no <boost/optional>")
+def test_boost_optional():
+    assert m.double_or_zero_boost(None) == 0
+    assert m.double_or_zero_boost(42) == 84
+    pytest.raises(TypeError, m.double_or_zero_boost, "foo")
+
+    assert m.half_or_none_boost(0) is None
+    assert m.half_or_none_boost(42) == 21
+    pytest.raises(TypeError, m.half_or_none_boost, "foo")
+
+    assert m.test_nullopt_boost() == 42
+    assert m.test_nullopt_boost(None) == 42
+    assert m.test_nullopt_boost(42) == 42
+    assert m.test_nullopt_boost(43) == 43
+
+    assert m.test_no_assign_boost() == 42
+    assert m.test_no_assign_boost(None) == 42
+    assert m.test_no_assign_boost(m.NoAssign(43)) == 43
+    pytest.raises(TypeError, m.test_no_assign_boost, 43)
+
+    holder = m.OptionalBoostHolder()
+    mvalue = holder.member
+    assert mvalue.initialized
+    assert holder.member_initialized()
+
+    props = m.OptionalBoostProperties()
+    assert int(props.access_by_ref) == 42
+    assert int(props.access_by_copy) == 42
+
+
+def test_reference_sensitive_optional():
+    assert m.double_or_zero_refsensitive(None) == 0
+    assert m.double_or_zero_refsensitive(42) == 84
+    pytest.raises(TypeError, m.double_or_zero_refsensitive, "foo")
+
+    assert m.half_or_none_refsensitive(0) is None
+    assert m.half_or_none_refsensitive(42) == 21
+    pytest.raises(TypeError, m.half_or_none_refsensitive, "foo")
+
+    assert m.test_nullopt_refsensitive() == 42
+    assert m.test_nullopt_refsensitive(None) == 42
+    assert m.test_nullopt_refsensitive(42) == 42
+    assert m.test_nullopt_refsensitive(43) == 43
+
+    assert m.test_no_assign_refsensitive() == 42
+    assert m.test_no_assign_refsensitive(None) == 42
+    assert m.test_no_assign_refsensitive(m.NoAssign(43)) == 43
+    pytest.raises(TypeError, m.test_no_assign_refsensitive, 43)
+
+    holder = m.OptionalRefSensitiveHolder()
+    mvalue = holder.member
+    assert mvalue.initialized
+    assert holder.member_initialized()
+
+    props = m.OptionalRefSensitiveProperties()
+    assert int(props.access_by_ref) == 42
+    assert int(props.access_by_copy) == 42
+
 
 @pytest.mark.skipif(not hasattr(m, "has_filesystem"), reason="no <filesystem>")
 def test_fs_path():