Return value policy gray zone

[wjakob]

cc @hawkinsp @vfdev-5 @oremanj

Nanobind exposes various return value policies that generally do an OK job. But there is a gray zone where their current behavior is IMO confusing. In particular, rv_policy::move, rv_policy::take_ownership, and rv_policy::reference_internal all check if an existing Python object is already associated with the pointer/reference. In that case, they directly return that and the return value policy is ignored.

This can lead to leaks. For example, suppose that code returns an object twice -- once using rv_policy::reference, and later using rv_policy::take_ownership. The second ownership transfer will never occur.

It can also lead to weird/unexpected behavior. For example,

.def("move_field", [](Struct &s) -> Field& { return s.field; }, rv_policy::move)

will not actually move the field when somebody else has previously created a reference to s.field.

Confusion aside, these lookups to check for the existence of an instance also have a non-negligible cost (hash table traversal) that would be nice to avoid.

But it is not so simple to change this behavior.

For example, one low hanging fruit that I was looking at just now was to disable the search for existing instances when the user passes rv_policy::move (which is used for pass-by value, so this would be great optimization that hits many usecases).

However, this breaks overloaded assignment operators (specifically nb::self() += nb::self() used in test14_operators). This is related to a PR by @oremanj (#803). The operator is overloaded with rv_policy::move and enforcing that now breaks preservation of the same object for an in-place update.

Taking a step back, the ideal behavior for in-place operators is to return the same Python object if the operator returns *this, and copy or move otherwise (perhaps move in the case of pass-by value, and copy in remaining cases?) We always move for pass-by value, so it seems that we need an return_existing_or_copy return value policy...

Next, I looked at disabling the instance search for rv_policy::take_ownership. This causes many unique pointer-related tests to fail in test_holders.py, and the test suite eventually segfaults in tests/test_thread.py.

The segfault is instructive: it happens in a function binding that essentially does [](Value *value) -> Value * { return value; } (pointer pass-through). The default value policy automatic turns into take_ownership, and that's the wrong one to use here. So making any change here will probably break quite a lot of user code.

I thought that it would be useful to have a discussion about whether others think this is a problem, and how to improve it.

One potential solution that I was thinking about is to separate the return value policy into two independent aspects:

1. what happens when an existing instance is found, and
2. what to do if not?

So we could, e.g., have

• always_copy
• always_move
• always_take_ownership
• return_existing_or_copy
• return_existing_or_move
• return_existing_or_take_ownership
• ...

(with the current policy names mapping to the return_existing_* variants).

By numbering them suitably, the use of these (many) policies could be dealt with using bit arithmetic in the implementation. To take the negative position, this change makes something that users already find confusing even more overwhelming.

Your feedback would be greatly appreaciated!

[oremanj]

As it happens, I attempted to solve this problem some months ago, following the discussion in #589. I gave up on it because I was running into a few too many fiddly corner cases, but with fresh eyes all of them were resolved pretty easily and I've uploaded the result as #889. That PR is focused on the correctness issues here, not the performance ones.

I don't think straight-up doubling the number of rv_policies is a very user-friendly solution (correctly choosing between always_take_ownership and return_existing_or_take_ownership is equivalent to choosing whether the already-exposed instances are non-owning or owning, which seems very difficult for the binding author to determine), but if we're willing to break the 3-bit barrier, I could at least see a use case for adding new copy/move policies:

• return_existing_or_copy (possible shorter names copy_or_none, copy_if_new) would be the appropriate policy for augmented assignment operators, freeing up move to not look for an existing object
• return_existing_or_move (move_or_none, move_if_new) might be the appropriate policy for functions that return an rvalue reference, while unconditional move is appropriate for functions that return a value

[wjakob]

Thank you @oremanj. I think you meant #889, I posted feedback there.

[oremanj]

Sorry yes, #888 for #889 was a typo. I'll respond there.

[hpkfft]

A quick thought (that might appear simpler to developers) would be rvp::lookup | rvp::move instead of return_existing_or_move.

[hpkfft]

Is it useful to have both copy and move? I haven't had the occasion to use this aspect of nanobind much, so I may be missing something obvious. However, I'm wondering if the existing policies are unnecessarily mixing purely C++ policy with the relationship between Python objects and C++ objects.
C++ has copy elision, and if that is not used, selects move constructors or copy constructors as appropriate for the return statement.
Can nanobind have something like rvp::new_object to replace both copy and move?

rvp	description
take_ownership	Python will own and will eventually delete/free the C++ resources
new_object	Python will own a new object that is distinct from the C++ one
lookup	Python already has a corresponding object, which is looked up and returned
reference	Python refers to a C++ object and C++ has lifetime responsibilities

And, reference_internal as before

[oremanj]

Is it useful to have both copy and move?

They're at least needed internally in order to communicate the value category to nb_type_put, since a function's return value has been type-erased to void* by the time nanobind actually copies or moves it. We need to be able to tell that, by default, we should move out of a returned rvalue (it's guaranteed safe since we're the only ones with access to it) but not move out of a returned const T&.

It would be theoretically possible to distinguish between "user-facing" RVPs (which get mentioned in .def() or nb::cast() arguments, and ultimately passed to type_caster::from_cpp() which knows the static type of the thing that's being converted) versus "internal" RVPs (which get passed to nb_type_put and need to provide enough detail to operate on a void*). If we did that, then automatic/automatic_reference/new_object would be external-only, copy/move would be internal-only, and reference/lookup/take_ownership would be both. I'm not sure it's worth the trouble and mental overhead though.

For reference, the converter between "external" and "internal" RVPs in current nanobind is infer_policy. It's hard to tell because both use the same enum type.