reference_internal does not always imply keep_alive<0, 1> as the documentation claims

[maekrbot]

Issue description

keep_alive<0, 1> is needed even with reference_internal call policy in some cases (e.g., my example below) or things can get SIGSEV'y.

Reproducible example code

I modified test_modules.cpp on master as follows:

diff -Naur <(curl -L https://raw.githubusercontent.com/pybind/pybind11/master/tests/test_modules.cpp) test_modules.cpp

--- /dev/fd/63	2019-04-11 12:50:22.000000000 -0700
+++ test_modules.cpp	2019-04-11 12:47:32.000000000 -0700
@@ -11,6 +11,25 @@
 #include "pybind11_tests.h"
 #include "constructor_stats.h"

+struct X;
+struct Y {
+  X* parent;
+  Y(X* p) : parent(p) {}
+  int f();
+};
+
+struct X {
+  int answer;
+  X() : answer(42) {}
+  int g();
+  Y get_y() {
+    return Y{this};
+  }
+};
+
+int X::g() { return answer; }
+int Y::f() { return parent->g(); }
+
 TEST_SUBMODULE(modules, m) {
     // test_nested_modules
     py::module m_sub = m.def_submodule("subsubmodule");
@@ -43,6 +62,16 @@
         A a1{1};
         A a2{2};
     };
+
+    py::class_<Y>(m_sub, "Y")
+      .def("f", &Y::f);
+
+    py::class_<X>(m_sub, "X")
+        .def(py::init<>())
+        .def("get_y", &X::get_y,
+          /// py::keep_alive<0, 1>(),
+          py::return_value_policy::reference_internal);
+
     py::class_<B>(m_sub, "B")
         .def(py::init<>())
         .def("get_a1", &B::get_a1, "Return the internal A 1", py::return_value_policy::reference_internal)

And modified added the following test to the pytest test_modules.py:

def test_ptr_case():
    y = ms.X().get_y()
    assert y.f() == 42

The result: without keep_alive, the test fails with garbage data in the property because (confirmed with print_created()/print_destroyed() but left out of here for brevity) the X instance is GC'ed after the first statement and before the usage of y. With keep_alive<0, 1> uncommented, the test behaves as expected:

python3 -m pytest -v test_modules.py 
test_modules.py::test_nested_modules PASSED                                                                                     [ 16%]
test_modules.py::test_ptr_case FAILED                                                                                           [ 33%]
test_modules.py::test_reference_internal PASSED                                                                                 [ 50%]
test_modules.py::test_importing PASSED                                                                                          [ 66%]
test_modules.py::test_pydoc PASSED                                                                                              [ 83%]
test_modules.py::test_duplicate_registration PASSED                                                                             [100%]

============================================================== FAILURES ===============================================================
____________________________________________________________ test_ptr_case ____________________________________________________________

    def test_ptr_case():
      y = ms.X().get_y()
>     assert y.f() == 42
E     assert 249231544 == 42
E       -249231544
E       +42

test_modules.py:19: AssertionError
================================================= 1 failed, 5 passed in 0.10 seconds ==================================================

But this shouldn't be necessary because the docs state that using reference_internal is equivalent to having this extra annotation:

Indicates that the lifetime of the return value is tied to the lifetime of a parent object, namely the implicit this, or self argument of the called method or property. Internally, this policy works just like return_value_policy::reference but additionally applies a keep_alive<0, 1> call policy

[eacousineau]

To me, I'd say that reference_internal is kinda working as intended, as it's meant for references, not copies. In your code, get_y is returning an instance of Y by value, which permits pybind11 to copy the object (maybe??? ... see below).

What happens if you change your code to store Y X::y_{this}, and return Y*?

TBH, I'm not sure if it's good or bad that pybind11 isn't puking an error telling you that your return value is inconsistent with your policy; i.e., you are giving it an rvalue T&&, but telling it to treat as T& / T*. I'm wondering if your working case might be relying on UB?