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>
This commit is contained in:
Ryan Cahoon 2021-10-25 19:04:45 -07:00 committed by GitHub
parent d45a88105c
commit c2d3e220bd
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 265 additions and 13 deletions

View File

@ -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> { }; : 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 // This type caster is intended to be used for std::optional and std::experimental::optional
template<typename T> struct optional_caster { template<typename Type, typename Value = typename Type::value_type> struct optional_caster {
using value_conv = make_caster<typename T::value_type>; using value_conv = make_caster<Value>;
template <typename T_> template <typename T>
static handle cast(T_ &&src, return_value_policy policy, handle parent) { static handle cast(T &&src, return_value_policy policy, handle parent) {
if (!src) if (!src)
return none().inc_ref(); return none().inc_ref();
if (!std::is_lvalue_reference<T>::value) { 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) { bool load(handle src, bool convert) {
@ -269,11 +269,11 @@ template<typename T> struct optional_caster {
if (!inner_caster.load(src, convert)) if (!inner_caster.load(src, convert))
return false; 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; return true;
} }
PYBIND11_TYPE_CASTER(T, _("Optional[") + value_conv::name + _("]")); PYBIND11_TYPE_CASTER(Type, _("Optional[") + value_conv::name + _("]"));
}; };
#if defined(PYBIND11_HAS_OPTIONAL) #if defined(PYBIND11_HAS_OPTIONAL)

View File

@ -256,7 +256,9 @@ if(Boost_FOUND)
endif() endif()
# Check if we need to add -lstdc++fs or -lc++fs or nothing # 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) set(STD_FS_NO_LIB_NEEDED TRUE)
else() else()
file( file(
@ -286,7 +288,7 @@ elseif(${STD_FS_NEEDS_CXXFS})
elseif(${STD_FS_NO_LIB_NEEDED}) elseif(${STD_FS_NO_LIB_NEEDED})
set(STD_FS_LIB "") set(STD_FS_LIB "")
else() else()
message(WARNING "Unknown compiler - not passing -lstdc++fs") message(WARNING "Unknown C++17 compiler - not passing -lstdc++fs")
set(STD_FS_LIB "") set(STD_FS_LIB "")
endif() endif()

View File

@ -19,6 +19,18 @@
#include <vector> #include <vector>
#include <string> #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 // Test with `std::variant` in C++17 mode, or with `boost::variant` in C++11/14
#if defined(PYBIND11_HAS_VARIANT) #if defined(PYBIND11_HAS_VARIANT)
using std::variant; using std::variant;
@ -59,7 +71,8 @@ namespace std {
template <template <typename> class OptionalImpl, typename T> template <template <typename> class OptionalImpl, typename T>
struct OptionalHolder struct OptionalHolder
{ {
OptionalHolder() = default; // NOLINTNEXTLINE(modernize-use-equals-default): breaks GCC 4.8
OptionalHolder() {};
bool member_initialized() const { bool member_initialized() const {
return member && member->initialized; 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_SUBMODULE(stl, m) {
// test_vector // test_vector
m.def("cast_vector", []() { return std::vector<int>{1}; }); m.def("cast_vector", []() { return std::vector<int>{1}; });
@ -145,6 +247,10 @@ TEST_SUBMODULE(stl, m) {
return v; return v;
}); });
pybind11::enum_<EnumType>(m, "EnumType")
.value("kSet", EnumType::kSet)
.value("kUnset", EnumType::kUnset);
// test_move_out_container // test_move_out_container
struct MoveOutContainer { struct MoveOutContainer {
struct Value { int value; }; struct Value { int value; };
@ -213,6 +319,12 @@ TEST_SUBMODULE(stl, m) {
.def(py::init<>()) .def(py::init<>())
.def_readonly("member", &opt_holder::member) .def_readonly("member", &opt_holder::member)
.def("member_initialized", &opt_holder::member_initialized); .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 #endif
#ifdef PYBIND11_HAS_EXP_OPTIONAL #ifdef PYBIND11_HAS_EXP_OPTIONAL
@ -239,8 +351,75 @@ TEST_SUBMODULE(stl, m) {
.def(py::init<>()) .def(py::init<>())
.def_readonly("member", &opt_exp_holder::member) .def_readonly("member", &opt_exp_holder::member)
.def("member_initialized", &opt_exp_holder::member_initialized); .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 #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 #ifdef PYBIND11_HAS_FILESYSTEM
// test_fs_path // test_fs_path
m.attr("has_filesystem") = true; m.attr("has_filesystem") = true;
@ -280,8 +459,12 @@ TEST_SUBMODULE(stl, m) {
m.def("tpl_ctor_set", [](std::unordered_set<TplCtorClass> &) {}); m.def("tpl_ctor_set", [](std::unordered_set<TplCtorClass> &) {});
#if defined(PYBIND11_HAS_OPTIONAL) #if defined(PYBIND11_HAS_OPTIONAL)
m.def("tpl_constr_optional", [](std::optional<TplCtorClass> &) {}); m.def("tpl_constr_optional", [](std::optional<TplCtorClass> &) {});
#elif defined(PYBIND11_HAS_EXP_OPTIONAL) #endif
m.def("tpl_constr_optional", [](std::experimental::optional<TplCtorClass> &) {}); #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 #endif
// test_vec_of_reference_wrapper // test_vec_of_reference_wrapper

View File

@ -132,6 +132,10 @@ def test_optional():
assert mvalue.initialized assert mvalue.initialized
assert holder.member_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( @pytest.mark.skipif(
not hasattr(m, "has_exp_optional"), reason="no <experimental/optional>" not hasattr(m, "has_exp_optional"), reason="no <experimental/optional>"
@ -160,6 +164,69 @@ def test_exp_optional():
assert mvalue.initialized assert mvalue.initialized
assert holder.member_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>") @pytest.mark.skipif(not hasattr(m, "has_filesystem"), reason="no <filesystem>")
def test_fs_path(): def test_fs_path():