WIP: Handle dangling objects and rv_policy changes robustly

This is a potential answer to wjakob#888, also inspired by the discussion in wjakob#589. Seeking feedback on the approach before I work on docs and tests.

When returning unique or shared ownership to Python when some Python instances already hold a non-owning reference to the same C++ object, we have a few choices:
* Create a new owning instance in addition to the existing non-owning one. (This PR's approach.)
* Upgrade the existing non-owning instance to owning. (More appealing in some ways, but hard to implement with shared ownership since nanobind doesn't have holders.)
* Just return the non-owning instance and forget about the ownership. (nanobind's current approach, which sometimes causes problems.)

This PR also handles the fact that a non-owning instance can dangle. That is, the referenced C++ object could be destroyed while the Python instance is still alive -- especially on PyPy, where it's hard to control when a Python instance dies. Dangling instances are always non-owning, so they are mostly handled by the same logic that handles rv_policy changes. The remaining piece of support for dangling instances is to acknowledge that, if the C++ referent is freed, a new instance could be allocated with its same address, even one with internal storage. (I have observed this in production.) So, there is some new logic in `inst_new_int` to remove the previous must-be-dangling instances from `inst_c2p`, rather than crashing because they exist. This also implies a new state that a nanobind instance can be in: if inst->offset == 0, the instance refers to no C++ object and is not stored in the inst_c2p map.

Author: oremanj

docs/api_core.rst

@@ -2887,7 +2887,8 @@ The documentation below refers to two per-instance flags with the following mean
    set to ``true`` and ``false``.
 
    This is analogous to casting a C++ object with return value policy
-   :cpp:enumerator:`rv_policy::reference`.
+   :cpp:enumerator:`rv_policy::reference`, except that it will create a
+   new Python instance even if one wrapping this object already exists.
 
    If a `parent` object is specified, the instance keeps this parent alive
    while the newly created object exists. This is analogous to casting a C++
@@ -2904,7 +2905,8 @@ The documentation below refers to two per-instance flags with the following mean
    ``true``.
 
    This is analogous to casting a C++ object with return value policy
-   :cpp:enumerator:`rv_policy::take_ownership`.
+   :cpp:enumerator:`rv_policy::take_ownership`, except that it will create a
+   new Python instance even if one wrapping this object already exists.
 
 .. cpp:function:: void inst_zero(handle h)
 

docs/changelog.rst

@@ -65,6 +65,13 @@ Version TBD (not yet released)
   not be an instance of the alias/trampoline type.
   (PR `#859 <https://github.com/wjakob/nanobind/pull/859>`__)
 
+* Nanobind now avoids returning an existing non-owning Python instance when
+  an owning Python instance was requested. Instead, it will create a new
+  Python instance that holds (unique or shared) ownership of the C++ object,
+  and allow the existing instance to continue to hold a reference to it.
+  This also helps in situations where <TODO...>
+  (PR `#889 <https://github.com/wjakob/nanobind/pull/889>`__)
+
 Version 2.4.0 (Dec 6, 2024)
 ---------------------------
 

include/nanobind/intrusive/ref.h

@@ -139,12 +139,23 @@ template <typename T> struct type_caster<nanobind::ref<T>> {
 
     static handle from_cpp(const ref<T> &value, rv_policy policy,
                            cleanup_list *cleanup) noexcept {
+        bool force_new = policy == rv_policy::copy || policy == rv_policy::move;
+
         if constexpr (std::is_base_of_v<intrusive_base, T>)
-            if (policy != rv_policy::copy && policy != rv_policy::move && value.get())
+            if (!force_new && value.get())
                 if (PyObject* obj = value->self_py())
                     return handle(obj).inc_ref();
 
-        return Caster::from_cpp(value.get(), policy, cleanup);
+        /* The combination of rv_policy::_shared_ownership + not asking
+           for an is_new response, is treated as requiring intrusive ownership.
+           We could pass any policy here except copy/move, but using this one
+           allows nanobind to check that the intrusive_ptr() annotation was
+           specified correctly. */
+        if (!force_new)
+            policy = rv_policy::_shared_ownership;
+
+        return Caster::from_cpp_raw((std::decay_t<T> *) value.get(), policy,
+                                    cleanup, nullptr);
     }
 };
 NAMESPACE_END(detail)

include/nanobind/nb_cast.h

@@ -481,7 +481,14 @@ template <typename Type_> struct type_caster_base : type_caster_base_tag {
         else
             ptr = (Type *) &value;
 
-        policy = infer_policy<T>(policy);
+        return from_cpp_raw(ptr, infer_policy<T>(policy), cleanup, nullptr);
+    }
+
+    /* Version of from_cpp that uses the rv_policy you give it as-is,
+       without resolving through infer_policy */
+    NB_INLINE static handle from_cpp_raw(Type *ptr, rv_policy policy,
+                                         cleanup_list *cleanup,
+                                         bool *is_new) noexcept {
         const std::type_info *type = &typeid(Type);
 
         constexpr bool has_type_hook =
@@ -490,11 +497,11 @@ template <typename Type_> struct type_caster_base : type_caster_base_tag {
             type = type_hook<Type>::get(ptr);
 
         if constexpr (!std::is_polymorphic_v<Type>) {
-            return nb_type_put(type, ptr, policy, cleanup);
+            return nb_type_put(type, ptr, policy, cleanup, is_new);
         } else {
             const std::type_info *type_p =
                 (!has_type_hook && ptr) ? &typeid(*ptr) : nullptr;
-            return nb_type_put_p(type, type_p, ptr, policy, cleanup);
+            return nb_type_put_p(type, type_p, ptr, policy, cleanup, is_new);
         }
     }
 

include/nanobind/nb_enums.h

@@ -18,9 +18,24 @@ enum class rv_policy {
     move,
     reference,
     reference_internal,
-    none
+    none,
     /* Note to self: nb_func.h assumes that this value fits into 3 bits,
        hence no further policies can be added. */
+
+    /* Special internal-use policy that can only be passed to `nb_type_put()`
+       or `type_caster_base<T>::from_cpp_raw()`. (To keep rv_policy fitting
+       in 3 bits, it aliases `automatic_reference`, which would always have
+       been converted into `copy` or `move` or `reference` before reaching
+       those functions.) It means we are creating a Python instance that holds
+       shared ownership of the C++ object we're casting, so we shouldn't reuse
+       a Python instance that only holds a reference to that object. The actual
+       enforcement of the shared ownership must be done separately after the
+       cast completes, via a keep_alive callback or similar, to ensure that
+       the C++ object lives at least as long as the new Python instance does.
+
+       This is an internal tool that is not part of nanobind's public API,
+       and may be changed without notice. */
+    _shared_ownership = automatic_reference
 };
 
 NAMESPACE_END(NB_NAMESPACE)

include/nanobind/stl/shared_ptr.h

@@ -102,23 +102,8 @@ template <typename T> struct type_caster<std::shared_ptr<T>> {
         handle result;
 
         Td *ptr = (Td *) value.get();
-        const std::type_info *type = &typeid(Td);
-
-        constexpr bool has_type_hook =
-            !std::is_base_of_v<std::false_type, type_hook<Td>>;
-        if constexpr (has_type_hook)
-            type = type_hook<Td>::get(ptr);
-
-        if constexpr (!std::is_polymorphic_v<Td>) {
-            result = nb_type_put(type, ptr, rv_policy::reference,
-                                 cleanup, &is_new);
-        } else {
-            const std::type_info *type_p =
-                (!has_type_hook && ptr) ? &typeid(*ptr) : nullptr;
-
-            result = nb_type_put_p(type, type_p, ptr, rv_policy::reference,
-                                   cleanup, &is_new);
-        }
+        result = Caster::from_cpp_raw(ptr, rv_policy::_shared_ownership,
+                                      cleanup, &is_new);
 
         if (is_new) {
             std::shared_ptr<void> pp;

src/nb_internals.h

@@ -51,7 +51,10 @@ struct func_data : func_data_prelim<0> {
 struct nb_inst { // usually: 24 bytes
     PyObject_HEAD
 
-    /// Offset to the actual instance data
+    /// Offset to the actual C++ object. A value of zero here is special;
+    /// it means this instance was "detached" (assumed dangling) because its
+    /// C++ object address collided with a newly allocated internal instance.
+    /// Detached instances are not stored in the inst_c2p map.
     int32_t offset;
 
     /// State of the C++ object this instance points to: is it constructed?
@@ -87,8 +90,11 @@ struct nb_inst { // usually: 24 bytes
     /// Does this instance use intrusive reference counting?
     uint32_t intrusive : 1;
 
+    /// Does this instance hold partial/shared ownership of its C++ object?
+    uint32_t shared_ownership : 1;
+
     // That's a lot of unused space. I wonder if there is a good use for it..
-    uint32_t unused : 24;
+    uint32_t unused : 23;
 };
 
 static_assert(sizeof(nb_inst) == sizeof(PyObject) + sizeof(uint32_t) * 2);