fix: the types for return_value_policy_override in optional_caster #3376

ryancahoon-zoox · 2021-10-17T21:56:48Z

Description

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. Closes #3376.

Suggested changelog entry:

Fixed the potential for dangling references when using properties with std::optional types

`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 pybind#3330

…onal-return_policy_override

tests/test_stl.cpp

rwgk

Looks good to me. Thanks for the thorough testing!

rwgk · 2021-10-25T20:49:25Z

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 {


Not super important, but this would read much nicer with ValueType (and wouldn't even need extra line breaks).

I was using the same naming convention for template parameters that's used for list_caster and array_caster. I agree ValueType is a better name, though. Would you prefer that I change it or keep it consistent?

Fine as is. Thanks for the explanation!

rwgk · 2021-10-25T20:57:32Z

tests/test_stl.cpp

+    OptionalProperties() : value(EnumType::k1) {}
+    ~OptionalProperties() {
+        // Reset value to detect use-after-destruction.
+        // This is set to a specific value rather than NULL to ensure that


rather than NULL
std::nullopt (to be precise)?

The specific value chosen happens to be 0 in memory. Is that sufficiently reliable for detecting use-after-destruction? (Vs. some random non-zero value.)

Done. I used the generic nullopt instead of std::nullopt because the OptionalImpl type may not be std::optional.

rwgk · 2021-10-25T21:10:39Z

tests/test_stl.cpp

 #endif

+    // test_refsensitive_optional
+    m.attr("has_refsensitive_optional") = true;


Is this actually needed/used? (copy-paste oversight)

henryiii

I think the only test failure is a standard flake. I'm happy once @rwgk's comments are resolved.

rwgk · 2021-10-26T02:04:36Z

The one CI failure is:
Run jwlawson/actions-setup-cmake@v1.11
Error: getaddrinfo ENOTFOUND api.github.com

I've not seen that one before, but it's safe to ignore I think. I'll go ahead merge this now.

rwgk · 2021-10-27T14:04:26Z

Hi @ryancahoon-zoox, when testing this PR with the smart_holder branch and PYBIND11_USE_SMART_HOLDER_AS_DEFAULT I'm seeing a test failure (below). I already spent a couple hours trying to understand, but I'm still in the dark. One quick question: do you know what changed in boost::optional between the older version that unmasks the pybind11 bug you fixed here, and the newer version that doesn't?

========================================================================= FAILURES ==========================================================================
_____________________________________________________________ test_reference_sensitive_optional _____________________________________________________________

    def test_reference_sensitive_optional():
...        props = m.OptionalRefSensitiveProperties()
        assert int(props.access_by_ref) == 42
>       assert int(props.access_by_copy) == 42
E       assert 44934346 == 42
E        +  where 44934346 = int(<EnumType.???: 44934346>)
E        +    where <EnumType.???: 44934346> = <pybind11_tests.stl.OptionalRefSensitiveProperties object at 0x7fb5d6517eb0>.access_by_copy

test_stl.py:228: AssertionError
================================================================== short test summary info ==================================================================

rwgk · 2021-10-27T16:52:27Z

@ryancahoon-zoox never mind, I think I just got it, it needs a tweak on the smart_holder branch:
fc5d70d
A bit amazing that this long-standing oversight wasn't covered by any tests before.

ryancahoon-zoox · 2021-10-27T18:16:31Z

Glad my tests are already being useful :-)

I'm not exactly sure what changed - I didn't get so far as reading the Boost source. I just wrote a binding for a class similar to constructor_stats.h that prints when it is constructed/copied/moved/destroyed and tried returning it wrapped in various optional implementations from a return-by-copy property (like access_by_copy from the test cases I provided). I observed that the lifetimes for the object reported with recent std::optional and boost::optional were the same, but they were different from those with the older version of boost::optional.

My very tenuous intuition is that it's either some optimization involving move semantics, or else in the process of standardizing std::optional the standards committee found some edge case that required the implementation to be changed, which was then back-ported to boost::optional. But I have no idea what the actual change was.

ryancahoon-zoox changed the title ~~fix: Use the right types for return_value_policy_override in optional_caster~~ fix: the types for return_value_policy_override in optional_caster Oct 17, 2021

Skylion007 added 4 commits October 18, 2021 11:50

Merge branch 'master' of https://github.com/pybind/pybind11 into opti…

ba9883c

…onal-return_policy_override

Fix clang-tidy complaints

b486544

Add missing NOLINT

4bdd6fc

Apply a couple more fixes

cab4955

Skylion007 requested review from henryiii and rwgk October 25, 2021 20:28

henryiii reviewed Oct 25, 2021

View reviewed changes

tests/test_stl.cpp Show resolved Hide resolved

rwgk approved these changes Oct 25, 2021

View reviewed changes

henryiii and others added 2 commits October 25, 2021 17:43

fix: support GCC 4.8

3797097

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

2412ad0

henryiii force-pushed the optional-return_policy_override branch from c50c854 to 2412ad0 Compare October 25, 2021 21:45

henryiii approved these changes Oct 25, 2021

View reviewed changes

ryancahoon-zoox added 2 commits October 25, 2021 18:11

Remove unneeded test module attribute

1aed3be

Change test enum to have more unique int values

30a0b29

Skylion007 approved these changes Oct 26, 2021

View reviewed changes

rwgk merged commit c2d3e22 into pybind:master Oct 26, 2021

github-actions bot added the needs changelog Possibly needs a changelog entry label Oct 26, 2021

henryiii mentioned this pull request Oct 26, 2021

add tests for boost::optional casters #3332

Closed

henryiii removed the needs changelog Possibly needs a changelog entry label Dec 21, 2021

rwgk mentioned this pull request Jan 27, 2022

Function pybind11_select_caster uses ADL to select the caster. #3643

Closed

rwgk mentioned this pull request Feb 10, 2023

FWD pybind11 google/pybind11clif#3376

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: the types for return_value_policy_override in optional_caster #3376

fix: the types for return_value_policy_override in optional_caster #3376

ryancahoon-zoox commented Oct 17, 2021 •

edited by henryiii

Loading

rwgk left a comment

rwgk Oct 25, 2021

ryancahoon-zoox Oct 26, 2021

rwgk Oct 26, 2021

rwgk Oct 25, 2021

ryancahoon-zoox Oct 26, 2021 •

edited

Loading

rwgk Oct 25, 2021

ryancahoon-zoox Oct 26, 2021

henryiii left a comment

rwgk commented Oct 26, 2021

rwgk commented Oct 27, 2021

rwgk commented Oct 27, 2021

ryancahoon-zoox commented Oct 27, 2021

fix: the types for return_value_policy_override in optional_caster #3376

fix: the types for return_value_policy_override in optional_caster #3376

Conversation

ryancahoon-zoox commented Oct 17, 2021 • edited by henryiii Loading

Description

Suggested changelog entry:

rwgk left a comment

Choose a reason for hiding this comment

rwgk Oct 25, 2021

Choose a reason for hiding this comment

ryancahoon-zoox Oct 26, 2021

Choose a reason for hiding this comment

rwgk Oct 26, 2021

Choose a reason for hiding this comment

rwgk Oct 25, 2021

Choose a reason for hiding this comment

ryancahoon-zoox Oct 26, 2021 • edited Loading

Choose a reason for hiding this comment

rwgk Oct 25, 2021

Choose a reason for hiding this comment

ryancahoon-zoox Oct 26, 2021

Choose a reason for hiding this comment

henryiii left a comment

Choose a reason for hiding this comment

rwgk commented Oct 26, 2021

rwgk commented Oct 27, 2021

rwgk commented Oct 27, 2021

ryancahoon-zoox commented Oct 27, 2021

ryancahoon-zoox commented Oct 17, 2021 •

edited by henryiii

Loading

ryancahoon-zoox Oct 26, 2021 •

edited

Loading