nb::[try_]cast: improved handling of implicit conversions

The ``nb::cast`` and ``nb::try_cast`` function may perform implicit
conversions of their input argument when called with ``convert=true``.
However, more complex chains of conversions that requiring lifetime from
the ``detail::cleanup_list`` were rejected. This commit fixes this
confusing corner case.

Author: wjakob

docs/porting.rst

@@ -284,9 +284,9 @@ changes are needed:
 
 Note that the cleanup list is only available when ``from_python()`` or
 ``from_cpp()`` are called as part of function dispatch, while usage by
-:cpp:func:`nb::cast() <cast>` sets ``cleanup`` to ``nullptr``. This case should
-be handled gracefully by refusing the conversion if the cleanup list is
-absolutely required.
+:cpp:func:`nb::cast() <cast>` may set ``cleanup`` to ``nullptr`` if implicit
+conversions are not enabled. This case should be handled gracefully by refusing
+the conversion if the cleanup list is absolutely required.
 
 Type casters may not raise C++ exceptions. Both ``from_python()`` and
 ``from_cpp()`` must be annotated with ``noexcept``. Exceptions or failure

include/nanobind/eigen/dense.h

@@ -198,7 +198,7 @@ struct type_caster<T, enable_if_t<is_eigen_plain_v<T> &&
             owner = capsule(temp, [](void *p) noexcept { delete (T *) p; });
             ptr = temp->data();
             policy = rv_policy::reference;
-        } else if (policy == rv_policy::reference_internal) {
+        } else if (policy == rv_policy::reference_internal && cleanup->self()) {
             owner = borrow(cleanup->self());
             policy = rv_policy::reference;
         }
@@ -417,15 +417,17 @@ struct type_caster<Eigen::Ref<T, Options, StrideType>,
         if constexpr (MaybeConvert) {
             /* Generating an implicit copy requires some object to assume
                ownership. During a function call, ``dcaster`` can serve that
-               role (this case is detected by checking whether ``cleanup`` is
-               defined). When used in other situatons (e.g. ``nb::cast()``),
-               the created ``Eigen::Ref<..>`` must take ownership of the copy.
-               This is only guranteed to work if DMapConstructorOwnsData.
+               role (this case is detected by checking whether ``flags`` has
+               the ``manual`` flag set). When used in other situations (e.g.
+               ``nb::cast()``), the created ``Eigen::Ref<..>`` must take
+               ownership of the copy. This is only guranteed to work if
+               DMapConstructorOwnsData.
 
                If neither of these is possible, we disable implicit
                conversions. */
 
-            if (!cleanup && !DMapConstructorOwnsData)
+            if ((flags & (uint8_t) cast_flags::manual) &&
+                !DMapConstructorOwnsData)
                 flags &= ~(uint8_t) cast_flags::convert;
 
             if (dcaster.from_python_(src, flags, cleanup))

include/nanobind/nb_cast.h

@@ -40,6 +40,9 @@ enum cast_flags : uint8_t {
 
     // Don't accept 'None' Python objects in the base class caster
     none_disallowed = (1 << 2),
+
+    // Indicates that this cast is performed by nb::cast or nb::try_cast
+    manual = (1 << 3)
 };
 
 /**
@@ -411,53 +414,86 @@ template <typename Type_> struct type_caster_base : type_caster_base_tag {
 template <typename Type, typename SFINAE>
 struct type_caster : type_caster_base<Type> { };
 
-NAMESPACE_END(detail)
+template <bool Convert, typename T>
+T cast_impl(handle h) {
+    using Caster = detail::make_caster<T>;
 
-template <typename T, typename Derived>
-bool try_cast(const detail::api<Derived> &value, T &out, bool convert = true) noexcept {
+    static_assert(
+        !(std::is_reference_v<T> || std::is_pointer_v<T>) ||
+            detail::is_base_caster_v<Caster> ||
+            std::is_same_v<const char *, T>,
+        "nanobind::cast(): cannot return a reference to a temporary.");
+
+    Caster caster;
+    bool rv;
+    if constexpr (Convert) {
+        cleanup_list cleanup(nullptr);
+        rv = caster.from_python(h.ptr(),
+                                ((uint8_t) cast_flags::convert) |
+                                ((uint8_t) cast_flags::manual),
+                                &cleanup);
+        cleanup.release(); // 'from_python' is 'noexcept', so this always runs
+    } else {
+        rv = caster.from_python(h.ptr(), (uint8_t) cast_flags::manual, nullptr);
+    }
+
+    if (!rv)
+        detail::raise_cast_error();
+    return caster.operator detail::cast_t<T>();
+}
+
+template <bool Convert, typename T>
+bool try_cast_impl(handle h, T &out) noexcept {
     using Caster = detail::make_caster<T>;
 
     static_assert(!std::is_same_v<const char *, T>,
                   "nanobind::try_cast(): cannot return a reference to a temporary.");
 
     Caster caster;
-    if (caster.from_python(value.derived().ptr(),
-                           convert ? (uint8_t) detail::cast_flags::convert
-                                   : (uint8_t) 0, nullptr)) {
+    bool rv;
+    if constexpr (Convert) {
+        cleanup_list cleanup(nullptr);
+        rv = caster.from_python(h.ptr(),
+                                ((uint8_t) cast_flags::convert) |
+                                ((uint8_t) cast_flags::manual),
+                                &cleanup);
+        cleanup.release(); // 'from_python' is 'noexcept', so this always runs
+    } else {
+        rv = caster.from_python(h.ptr(), (uint8_t) cast_flags::manual, nullptr);
+    }
+
+    if (rv) {
         try {
             out = caster.operator detail::cast_t<T>();
             return true;
-        } catch (const builtin_exception&) {
-            return false;
-        }
+        } catch (const builtin_exception&) { }
     }
 
     return false;
 }
 
+NAMESPACE_END(detail)
+
 template <typename T, typename Derived>
-T cast(const detail::api<Derived> &value, bool convert = true) {
+NB_INLINE T cast(const detail::api<Derived> &value, bool convert = true) {
     if constexpr (std::is_same_v<T, void>) {
         return;
     } else {
-        using Caster = detail::make_caster<T>;
-
-        static_assert(
-            !(std::is_reference_v<T> || std::is_pointer_v<T>) ||
-                detail::is_base_caster_v<Caster> ||
-                std::is_same_v<const char *, T>,
-            "nanobind::cast(): cannot return a reference to a temporary.");
-
-        Caster caster;
-        if (!caster.from_python(value.derived().ptr(),
-                                convert ? (uint8_t) detail::cast_flags::convert
-                                        : (uint8_t) 0, nullptr))
-            detail::raise_cast_error();
-
-        return caster.operator detail::cast_t<T>();
+        if (convert)
+            return detail::cast_impl<true, T>(value);
+        else
+            return detail::cast_impl<false, T>(value);
     }
 }
 
+template <typename T, typename Derived>
+NB_INLINE bool try_cast(const detail::api<Derived> &value, T &out, bool convert = true) noexcept {
+    if (convert)
+        return detail::try_cast_impl<true, T>(value, out);
+    else
+        return detail::try_cast_impl<false, T>(value, out);
+}
+
 template <typename T>
 object cast(T &&value, rv_policy policy = rv_policy::automatic_reference) {
     handle h = detail::make_caster<T>::from_cpp((detail::forward_t<T>) value,