Skip to content

Commit

Permalink
nb::[try_]cast: improved handling of implicit conversions
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
wjakob committed Mar 11, 2024
1 parent 5259872 commit 56d7e93
Show file tree
Hide file tree
Showing 2 changed files with 69 additions and 31 deletions.
14 changes: 8 additions & 6 deletions include/nanobind/eigen/dense.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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))
Expand Down
86 changes: 61 additions & 25 deletions include/nanobind/nb_cast.h
Original file line number Diff line number Diff line change
Expand Up @@ -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)
};

/**
Expand Down Expand Up @@ -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,
Expand Down

0 comments on commit 56d7e93

Please sign in to comment.