tweaks to PR #549 and miscellaneous cleanups

This commit splits up a large testcase into smaller ones, which fixes
a weird GC issue that seems to be specific to Python on Windows
(instances being collected too late, which makes a test flaky).

Author: wjakob

include/nanobind/stl/unique_ptr.h

@@ -155,16 +155,18 @@ struct type_caster<std::unique_ptr<T, Deleter>> {
     }
 
     explicit operator Value() {
-        if (inflight)
-            inflight = false;
-        else if (!nb_type_relinquish_ownership(src.ptr(), IsDefaultDeleter))
+        if (!inflight && !nb_type_relinquish_ownership(src.ptr(), IsDefaultDeleter))
             throw next_overload();
 
-        T *value = caster.operator T *();
+        T *p = caster.operator T *();
+
+        Value value;
         if constexpr (IsNanobindDeleter)
-            return Value(value, deleter<T>(src.inc_ref()));
+            value = Value(p, deleter<T>(src.inc_ref()));
         else
-            return Value(value);
+            value = Value(p);
+        inflight = false;
+        return value;
     }
 };
 

src/nb_enum.cpp

@@ -166,8 +166,8 @@ bool enum_from_python(const std::type_info *tp, PyObject *o, int64_t *out, uint8
                 PyErr_Clear();
                 return false;
             }
-            enum_map::iterator it = fwd->find((int64_t) value);
-            if (it != fwd->end()) {
+            enum_map::iterator it2 = fwd->find((int64_t) value);
+            if (it2 != fwd->end()) {
                 *out = (int64_t) value;
                 return true;
             }
@@ -177,8 +177,8 @@ bool enum_from_python(const std::type_info *tp, PyObject *o, int64_t *out, uint8
                 PyErr_Clear();
                 return false;
             }
-            enum_map::iterator it = fwd->find((int64_t) value);
-            if (it != fwd->end()) {
+            enum_map::iterator it2 = fwd->find((int64_t) value);
+            if (it2 != fwd->end()) {
                 *out = (int64_t) value;
                 return true;
             }

src/nb_type.cpp

@@ -1677,18 +1677,17 @@ static void warn_relinquish_failed(const char *why, PyObject *o) noexcept {
 bool nb_type_relinquish_ownership(PyObject *o, bool cpp_delete) noexcept {
     nb_inst *inst = (nb_inst *) o;
 
-    /* This function is called after nb_type_get() succeeds, so the
-       instance should be ready; but the !ready case is possible if
-       an attempt is made to transfer ownership of the same object
-       to C++ multiple times as part of the same data structure.
-       For example, converting Python (foo, foo) to C++
+    /* This function is called after nb_type_get() succeeds, so the instance
+       should be ready; but the !ready case is possible if an attempt is made to
+       transfer ownership of the same object to C++ multiple times as part of
+       the same data structure. For example, converting Python (foo, foo) to C++
        std::pair<std::unique_ptr<T>, std::unique_ptr<T>>. */
 
     if (!inst->ready) {
         warn_relinquish_failed(
-            "The resulting data structure would have had multiple "
-            "std::unique_ptrs that each thought they owned the same "
-            "Python instance's data, which is not allowed.", o);
+            "The resulting data structure would have multiple "
+            "std::unique_ptrs, each thinking that they own the same instance, "
+            "which is not allowed.", o);
         return false;
     }
 
@@ -1698,8 +1697,8 @@ bool nb_type_relinquish_ownership(PyObject *o, bool cpp_delete) noexcept {
                 "This is only possible when the instance was previously "
                 "constructed on the C++ side and is now owned by Python, which "
                 "was not the case here. You could change the unique pointer "
-                "signature to std::unique_ptr<T, nb::deleter<T>> to work around "
-                "this issue.", o);
+                "signature to std::unique_ptr<T, nb::deleter<T>> to work "
+                "around this issue.", o);
             return false;
         }
 

tests/test_holders.cpp

@@ -177,9 +177,8 @@ NB_MODULE(test_holders_ext, m) {
           [](std::vector<std::pair<std::unique_ptr<Example>,
                                    std::unique_ptr<Example>>> v,
              bool clear) {
-              if (clear) {
+              if (clear)
                   v.clear();
-              }
               return v;
           }, nb::arg("v"), nb::arg("clear") = false);
 

tests/test_holders.py

@@ -85,7 +85,7 @@ def test04_uniqueptr_from_cpp(clean):
     assert t.stats() == (2, 2)
 
 
-def test05_uniqueptr_from_cpp(clean):
+def test05a_uniqueptr_from_cpp(clean):
     # Test ownership exchange when the object has been created on the C++ side
     a = t.unique_from_cpp()
     b = t.unique_from_cpp_2()
@@ -121,6 +121,8 @@ def test05_uniqueptr_from_cpp(clean):
     collect()
     assert t.stats() == (2, 2)
 
+def test05b_uniqueptr_list(clean):
+    t.reset()
     # Test ownership exchange as part of a larger data structure
     k = t.unique_from_cpp(1)
     v = t.unique_from_cpp(2)
@@ -134,20 +136,23 @@ def test05_uniqueptr_from_cpp(clean):
         with pytest.warns(RuntimeWarning, match='nanobind: attempted to access an uninitialized instance of type \'test_holders_ext.Example\'!'):
             with pytest.raises(TypeError) as excinfo:
                 obj.value
+    collect()
+    assert t.stats() == (2, 2)
 
+def test05c_uniqueptr_structure_duplicate(clean):
+    t.reset()
     # Test ownership exchange that fails partway through
-    # (can't take ownershp from k twice)
+    # (can't take ownership from k twice)
     k = t.unique_from_cpp(3)
     with pytest.warns(RuntimeWarning, match=r'nanobind::detail::nb_relinquish_ownership()'):
         with pytest.raises(TypeError):
             t.passthrough_unique_pairs([(k, k)])
     # Ownership passes back to Python
     assert k.value == 3
 
-    del k, v, res
-    collect()
+    del k
     collect()
-    assert t.stats() == (5, 5)
+    assert t.stats() == (1, 1)
 
 
 def test06_uniqueptr_from_py(clean):