Skip to content

Commit c2d3e22

Browse files
ryancahoon-zooxSkylion007henryiii
authored
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 <[email protected]> Co-authored-by: Henry Schreiner <[email protected]>
1 parent d45a881 commit c2d3e22

File tree

4 files changed

+265
-13
lines changed

4 files changed

+265
-13
lines changed

include/pybind11/stl.h

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -245,17 +245,17 @@ template <typename Key, typename Value, typename Hash, typename Equal, typename
245245
: map_caster<std::unordered_map<Key, Value, Hash, Equal, Alloc>, Key, Value> { };
246246

247247
// This type caster is intended to be used for std::optional and std::experimental::optional
248-
template<typename T> struct optional_caster {
249-
using value_conv = make_caster<typename T::value_type>;
248+
template<typename Type, typename Value = typename Type::value_type> struct optional_caster {
249+
using value_conv = make_caster<Value>;
250250

251-
template <typename T_>
252-
static handle cast(T_ &&src, return_value_policy policy, handle parent) {
251+
template <typename T>
252+
static handle cast(T &&src, return_value_policy policy, handle parent) {
253253
if (!src)
254254
return none().inc_ref();
255255
if (!std::is_lvalue_reference<T>::value) {
256-
policy = return_value_policy_override<T>::policy(policy);
256+
policy = return_value_policy_override<Value>::policy(policy);
257257
}
258-
return value_conv::cast(*std::forward<T_>(src), policy, parent);
258+
return value_conv::cast(*std::forward<T>(src), policy, parent);
259259
}
260260

261261
bool load(handle src, bool convert) {
@@ -269,11 +269,11 @@ template<typename T> struct optional_caster {
269269
if (!inner_caster.load(src, convert))
270270
return false;
271271

272-
value.emplace(cast_op<typename T::value_type &&>(std::move(inner_caster)));
272+
value.emplace(cast_op<Value &&>(std::move(inner_caster)));
273273
return true;
274274
}
275275

276-
PYBIND11_TYPE_CASTER(T, _("Optional[") + value_conv::name + _("]"));
276+
PYBIND11_TYPE_CASTER(Type, _("Optional[") + value_conv::name + _("]"));
277277
};
278278

279279
#if defined(PYBIND11_HAS_OPTIONAL)

tests/CMakeLists.txt

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -256,7 +256,9 @@ if(Boost_FOUND)
256256
endif()
257257

258258
# Check if we need to add -lstdc++fs or -lc++fs or nothing
259-
if(MSVC)
259+
if(DEFINED CMAKE_CXX_STANDARD AND CMAKE_CXX_STANDARD LESS 17)
260+
set(STD_FS_NO_LIB_NEEDED TRUE)
261+
elseif(MSVC)
260262
set(STD_FS_NO_LIB_NEEDED TRUE)
261263
else()
262264
file(
@@ -286,7 +288,7 @@ elseif(${STD_FS_NEEDS_CXXFS})
286288
elseif(${STD_FS_NO_LIB_NEEDED})
287289
set(STD_FS_LIB "")
288290
else()
289-
message(WARNING "Unknown compiler - not passing -lstdc++fs")
291+
message(WARNING "Unknown C++17 compiler - not passing -lstdc++fs")
290292
set(STD_FS_LIB "")
291293
endif()
292294

tests/test_stl.cpp

Lines changed: 186 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,18 @@
1919
#include <vector>
2020
#include <string>
2121

22+
#if defined(PYBIND11_TEST_BOOST)
23+
#include <boost/optional.hpp>
24+
25+
namespace pybind11 { namespace detail {
26+
template <typename T>
27+
struct type_caster<boost::optional<T>> : optional_caster<boost::optional<T>> {};
28+
29+
template <>
30+
struct type_caster<boost::none_t> : void_caster<boost::none_t> {};
31+
}} // namespace pybind11::detail
32+
#endif
33+
2234
// Test with `std::variant` in C++17 mode, or with `boost::variant` in C++11/14
2335
#if defined(PYBIND11_HAS_VARIANT)
2436
using std::variant;
@@ -59,14 +71,104 @@ namespace std {
5971
template <template <typename> class OptionalImpl, typename T>
6072
struct OptionalHolder
6173
{
62-
OptionalHolder() = default;
74+
// NOLINTNEXTLINE(modernize-use-equals-default): breaks GCC 4.8
75+
OptionalHolder() {};
6376
bool member_initialized() const {
6477
return member && member->initialized;
6578
}
6679
OptionalImpl<T> member = T{};
6780
};
6881

6982

83+
enum class EnumType {
84+
kSet = 42,
85+
kUnset = 85,
86+
};
87+
88+
// This is used to test that return-by-ref and return-by-copy policies are
89+
// handled properly for optional types. This is a regression test for a dangling
90+
// reference issue. The issue seemed to require the enum value type to
91+
// reproduce - it didn't seem to happen if the value type is just an integer.
92+
template <template <typename> class OptionalImpl>
93+
class OptionalProperties {
94+
public:
95+
using OptionalEnumValue = OptionalImpl<EnumType>;
96+
97+
OptionalProperties() : value(EnumType::kSet) {}
98+
~OptionalProperties() {
99+
// Reset value to detect use-after-destruction.
100+
// This is set to a specific value rather than nullopt to ensure that
101+
// the memory that contains the value gets re-written.
102+
value = EnumType::kUnset;
103+
}
104+
105+
OptionalEnumValue& access_by_ref() { return value; }
106+
OptionalEnumValue access_by_copy() { return value; }
107+
108+
private:
109+
OptionalEnumValue value;
110+
};
111+
112+
// This type mimics aspects of boost::optional from old versions of Boost,
113+
// which exposed a dangling reference bug in Pybind11. Recent versions of
114+
// boost::optional, as well as libstdc++'s std::optional, don't seem to be
115+
// affected by the same issue. This is meant to be a minimal implementation
116+
// required to reproduce the issue, not fully standard-compliant.
117+
// See issue #3330 for more details.
118+
template <typename T>
119+
class ReferenceSensitiveOptional {
120+
public:
121+
using value_type = T;
122+
123+
ReferenceSensitiveOptional() = default;
124+
// NOLINTNEXTLINE(google-explicit-constructor)
125+
ReferenceSensitiveOptional(const T& value) : storage{value} {}
126+
// NOLINTNEXTLINE(google-explicit-constructor)
127+
ReferenceSensitiveOptional(T&& value) : storage{std::move(value)} {}
128+
ReferenceSensitiveOptional& operator=(const T& value) {
129+
storage = {value};
130+
return *this;
131+
}
132+
ReferenceSensitiveOptional& operator=(T&& value) {
133+
storage = {std::move(value)};
134+
return *this;
135+
}
136+
137+
template <typename... Args>
138+
T& emplace(Args&&... args) {
139+
storage.clear();
140+
storage.emplace_back(std::forward<Args>(args)...);
141+
return storage.back();
142+
}
143+
144+
const T& value() const noexcept {
145+
assert(!storage.empty());
146+
return storage[0];
147+
}
148+
149+
const T& operator*() const noexcept {
150+
return value();
151+
}
152+
153+
const T* operator->() const noexcept {
154+
return &value();
155+
}
156+
157+
explicit operator bool() const noexcept {
158+
return !storage.empty();
159+
}
160+
161+
private:
162+
std::vector<T> storage;
163+
};
164+
165+
namespace pybind11 { namespace detail {
166+
template <typename T>
167+
struct type_caster<ReferenceSensitiveOptional<T>> : optional_caster<ReferenceSensitiveOptional<T>> {};
168+
} // namespace detail
169+
} // namespace pybind11
170+
171+
70172
TEST_SUBMODULE(stl, m) {
71173
// test_vector
72174
m.def("cast_vector", []() { return std::vector<int>{1}; });
@@ -145,6 +247,10 @@ TEST_SUBMODULE(stl, m) {
145247
return v;
146248
});
147249

250+
pybind11::enum_<EnumType>(m, "EnumType")
251+
.value("kSet", EnumType::kSet)
252+
.value("kUnset", EnumType::kUnset);
253+
148254
// test_move_out_container
149255
struct MoveOutContainer {
150256
struct Value { int value; };
@@ -213,6 +319,12 @@ TEST_SUBMODULE(stl, m) {
213319
.def(py::init<>())
214320
.def_readonly("member", &opt_holder::member)
215321
.def("member_initialized", &opt_holder::member_initialized);
322+
323+
using opt_props = OptionalProperties<std::optional>;
324+
pybind11::class_<opt_props>(m, "OptionalProperties")
325+
.def(pybind11::init<>())
326+
.def_property_readonly("access_by_ref", &opt_props::access_by_ref)
327+
.def_property_readonly("access_by_copy", &opt_props::access_by_copy);
216328
#endif
217329

218330
#ifdef PYBIND11_HAS_EXP_OPTIONAL
@@ -239,8 +351,75 @@ TEST_SUBMODULE(stl, m) {
239351
.def(py::init<>())
240352
.def_readonly("member", &opt_exp_holder::member)
241353
.def("member_initialized", &opt_exp_holder::member_initialized);
354+
355+
using opt_exp_props = OptionalProperties<std::experimental::optional>;
356+
pybind11::class_<opt_exp_props>(m, "OptionalExpProperties")
357+
.def(pybind11::init<>())
358+
.def_property_readonly("access_by_ref", &opt_exp_props::access_by_ref)
359+
.def_property_readonly("access_by_copy", &opt_exp_props::access_by_copy);
242360
#endif
243361

362+
#if defined(PYBIND11_TEST_BOOST)
363+
// test_boost_optional
364+
m.attr("has_boost_optional") = true;
365+
366+
using boost_opt_int = boost::optional<int>;
367+
using boost_opt_no_assign = boost::optional<NoAssign>;
368+
m.def("double_or_zero_boost", [](const boost_opt_int& x) -> int {
369+
return x.value_or(0) * 2;
370+
});
371+
m.def("half_or_none_boost", [](int x) -> boost_opt_int {
372+
return x != 0 ? boost_opt_int(x / 2) : boost_opt_int();
373+
});
374+
m.def("test_nullopt_boost", [](boost_opt_int x) {
375+
return x.value_or(42);
376+
}, py::arg_v("x", boost::none, "None"));
377+
m.def("test_no_assign_boost", [](const boost_opt_no_assign &x) {
378+
return x ? x->value : 42;
379+
}, py::arg_v("x", boost::none, "None"));
380+
381+
using opt_boost_holder = OptionalHolder<boost::optional, MoveOutDetector>;
382+
py::class_<opt_boost_holder>(m, "OptionalBoostHolder", "Class with optional member")
383+
.def(py::init<>())
384+
.def_readonly("member", &opt_boost_holder::member)
385+
.def("member_initialized", &opt_boost_holder::member_initialized);
386+
387+
using opt_boost_props = OptionalProperties<boost::optional>;
388+
pybind11::class_<opt_boost_props>(m, "OptionalBoostProperties")
389+
.def(pybind11::init<>())
390+
.def_property_readonly("access_by_ref", &opt_boost_props::access_by_ref)
391+
.def_property_readonly("access_by_copy", &opt_boost_props::access_by_copy);
392+
#endif
393+
394+
// test_refsensitive_optional
395+
using refsensitive_opt_int = ReferenceSensitiveOptional<int>;
396+
using refsensitive_opt_no_assign = ReferenceSensitiveOptional<NoAssign>;
397+
m.def("double_or_zero_refsensitive", [](const refsensitive_opt_int& x) -> int {
398+
return (x ? x.value() : 0) * 2;
399+
});
400+
m.def("half_or_none_refsensitive", [](int x) -> refsensitive_opt_int {
401+
return x != 0 ? refsensitive_opt_int(x / 2) : refsensitive_opt_int();
402+
});
403+
// NOLINTNEXTLINE(performance-unnecessary-value-param)
404+
m.def("test_nullopt_refsensitive", [](refsensitive_opt_int x) {
405+
return x ? x.value() : 42;
406+
}, py::arg_v("x", refsensitive_opt_int(), "None"));
407+
m.def("test_no_assign_refsensitive", [](const refsensitive_opt_no_assign &x) {
408+
return x ? x->value : 42;
409+
}, py::arg_v("x", refsensitive_opt_no_assign(), "None"));
410+
411+
using opt_refsensitive_holder = OptionalHolder<ReferenceSensitiveOptional, MoveOutDetector>;
412+
py::class_<opt_refsensitive_holder>(m, "OptionalRefSensitiveHolder", "Class with optional member")
413+
.def(py::init<>())
414+
.def_readonly("member", &opt_refsensitive_holder::member)
415+
.def("member_initialized", &opt_refsensitive_holder::member_initialized);
416+
417+
using opt_refsensitive_props = OptionalProperties<ReferenceSensitiveOptional>;
418+
pybind11::class_<opt_refsensitive_props>(m, "OptionalRefSensitiveProperties")
419+
.def(pybind11::init<>())
420+
.def_property_readonly("access_by_ref", &opt_refsensitive_props::access_by_ref)
421+
.def_property_readonly("access_by_copy", &opt_refsensitive_props::access_by_copy);
422+
244423
#ifdef PYBIND11_HAS_FILESYSTEM
245424
// test_fs_path
246425
m.attr("has_filesystem") = true;
@@ -280,8 +459,12 @@ TEST_SUBMODULE(stl, m) {
280459
m.def("tpl_ctor_set", [](std::unordered_set<TplCtorClass> &) {});
281460
#if defined(PYBIND11_HAS_OPTIONAL)
282461
m.def("tpl_constr_optional", [](std::optional<TplCtorClass> &) {});
283-
#elif defined(PYBIND11_HAS_EXP_OPTIONAL)
284-
m.def("tpl_constr_optional", [](std::experimental::optional<TplCtorClass> &) {});
462+
#endif
463+
#if defined(PYBIND11_HAS_EXP_OPTIONAL)
464+
m.def("tpl_constr_optional_exp", [](std::experimental::optional<TplCtorClass> &) {});
465+
#endif
466+
#if defined(PYBIND11_TEST_BOOST)
467+
m.def("tpl_constr_optional_boost", [](boost::optional<TplCtorClass> &) {});
285468
#endif
286469

287470
// test_vec_of_reference_wrapper

tests/test_stl.py

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,10 @@ def test_optional():
132132
assert mvalue.initialized
133133
assert holder.member_initialized()
134134

135+
props = m.OptionalProperties()
136+
assert int(props.access_by_ref) == 42
137+
assert int(props.access_by_copy) == 42
138+
135139

136140
@pytest.mark.skipif(
137141
not hasattr(m, "has_exp_optional"), reason="no <experimental/optional>"
@@ -160,6 +164,69 @@ def test_exp_optional():
160164
assert mvalue.initialized
161165
assert holder.member_initialized()
162166

167+
props = m.OptionalExpProperties()
168+
assert int(props.access_by_ref) == 42
169+
assert int(props.access_by_copy) == 42
170+
171+
172+
@pytest.mark.skipif(not hasattr(m, "has_boost_optional"), reason="no <boost/optional>")
173+
def test_boost_optional():
174+
assert m.double_or_zero_boost(None) == 0
175+
assert m.double_or_zero_boost(42) == 84
176+
pytest.raises(TypeError, m.double_or_zero_boost, "foo")
177+
178+
assert m.half_or_none_boost(0) is None
179+
assert m.half_or_none_boost(42) == 21
180+
pytest.raises(TypeError, m.half_or_none_boost, "foo")
181+
182+
assert m.test_nullopt_boost() == 42
183+
assert m.test_nullopt_boost(None) == 42
184+
assert m.test_nullopt_boost(42) == 42
185+
assert m.test_nullopt_boost(43) == 43
186+
187+
assert m.test_no_assign_boost() == 42
188+
assert m.test_no_assign_boost(None) == 42
189+
assert m.test_no_assign_boost(m.NoAssign(43)) == 43
190+
pytest.raises(TypeError, m.test_no_assign_boost, 43)
191+
192+
holder = m.OptionalBoostHolder()
193+
mvalue = holder.member
194+
assert mvalue.initialized
195+
assert holder.member_initialized()
196+
197+
props = m.OptionalBoostProperties()
198+
assert int(props.access_by_ref) == 42
199+
assert int(props.access_by_copy) == 42
200+
201+
202+
def test_reference_sensitive_optional():
203+
assert m.double_or_zero_refsensitive(None) == 0
204+
assert m.double_or_zero_refsensitive(42) == 84
205+
pytest.raises(TypeError, m.double_or_zero_refsensitive, "foo")
206+
207+
assert m.half_or_none_refsensitive(0) is None
208+
assert m.half_or_none_refsensitive(42) == 21
209+
pytest.raises(TypeError, m.half_or_none_refsensitive, "foo")
210+
211+
assert m.test_nullopt_refsensitive() == 42
212+
assert m.test_nullopt_refsensitive(None) == 42
213+
assert m.test_nullopt_refsensitive(42) == 42
214+
assert m.test_nullopt_refsensitive(43) == 43
215+
216+
assert m.test_no_assign_refsensitive() == 42
217+
assert m.test_no_assign_refsensitive(None) == 42
218+
assert m.test_no_assign_refsensitive(m.NoAssign(43)) == 43
219+
pytest.raises(TypeError, m.test_no_assign_refsensitive, 43)
220+
221+
holder = m.OptionalRefSensitiveHolder()
222+
mvalue = holder.member
223+
assert mvalue.initialized
224+
assert holder.member_initialized()
225+
226+
props = m.OptionalRefSensitiveProperties()
227+
assert int(props.access_by_ref) == 42
228+
assert int(props.access_by_copy) == 42
229+
163230

164231
@pytest.mark.skipif(not hasattr(m, "has_filesystem"), reason="no <filesystem>")
165232
def test_fs_path():

0 commit comments

Comments
 (0)