Could unchecked views prevent garbage collection of the underlying arrays?

[anntzer]

Issue description

Unchecked views appear to not prevent garbage collection of the underlying array (object). Unless I am mistaken, it should be possible for them to incref the underlying object upon construction of the unchecked view and decref it upon destruction, for a minimal cost; I do not see a case where the opposite behavior is desirable.

Reproducible example code

#include <pybind11/pybind11.h>
#include <pybind11/numpy.h>

namespace py = pybind11;

int foo() {
    auto a =
        py::module::import("numpy").attr("zeros")(1)
        .cast<py::array_t<double>>().mutable_unchecked<1>();
    auto b =
        py::module::import("numpy").attr("zeros")(1)
        .cast<py::array_t<double>>().mutable_unchecked<1>();
    a(0) = 1;
    // Rely on the fact that after a and b are (usually) allocated at the same position as the
    // first one has been immediately garbage collected.
    py::print(a(0), b(0));
}

PYBIND11_PLUGIN(python_example) {
    py::module m("python_example");
    m.def("foo", &foo);
    return m.ptr();
}

$ python -c 'from python_example import *; foo()'
1.0 1.0  # Should be 1.0 0.0 if a did not get GC before b gets allocated.

[jagerman]

The unchecked reference class itself is meant to be extremely light: it is really nothing more than a pointer and shape/stride calculator. Adding a reference to the underlying array would make it non-trivially-destructible, which while not a huge deal, is no longer quite as light-weight.

The example is indeed a problem; I'm tempted to qualify the unchecked methods with an lvalue context (i.e. ... mutable_unchecked() & {) to turn the above into a compilation failure to catch this sort of thing.

[anntzer]

I would prefer referencing but erroring on lost references would certainly be already an improvement.

[anntzer]

Upon further reflection, I am not certain this is the right approach: the function returning an array_t<> could well be returning a reference, say, to an attribute that is held somewhere on the Python side, in which case there is no issue with the above idiom.

I guess if you want to make this safe without actually holding a reference, you should check the refcount on the array when creating the view?

[jagerman]

unchecked is meant to be a bit lower level than that: think of it basically pointer access with stride and shape math (if you're familiar with Eigen, it's basically doing the same thing as an Eigen::Map, which similarly doesn't keep its referenced matrix alive). While it could be safe to hold those pointers (not just the data: also the strides and shape) whether or not your unchecked-using code would be safe or not would depend on how your method was called from Python, which seems pretty undesirable. lvalue-qualifying the method is a cheap way to better ensure that you hold an instance that isn't a temporary; it's not 100%, but ought to prevent unintentional misuse.

Adding a reference for the duration of the proxy object is another option. I'm not dead set against it, but I don't really see a huge advantage here, either.

(Also see #599, #617, and #746 for the (rather lengthy) discussion that led up to this; there were multiple proposals for unchecked access considered before we finally settled on the current approach.)