Be more careful with move semantics in type casters

This commit addresses a serious issue involving combinations of bound
types (e.g., ``T``) and type casters (e.g., ``std::vector<T>``), where
nanobind was too aggressive in its use of move semantics.

Calling a bound function from Python taking such a list (e.g., ``f([t1,
t2, ..])``) would lead to the destruction of the Python objects ``t1,
t2, ..`` if the type ``T`` exposed a move constructor, which is highly
non-intuitive.

This commit fixes, simplifies, and documents the rules of this process.
In particular, ``type_caster::Cast`` continues to serve its role to
infer what types a type caster can and wants to produce. It aggressively
tries to move, but now only does so when this is legal (i.e., when the
type caster created a temporary copy that is safe to move without
affecting program state elsewhere).

This involves two new default casting rules: ``movable_cast_t`` (adapted
to be more aggressive with moves) and ``precise_cast_t`` for bound types
that does not involve a temporary and consequently should only move when
this was explicitly requested by the user.

The fix also revealed inefficiencies in the previous implementation
where moves were actually possible but not done (e.g. for functions
taking an STL list by value). Some binding projects may see speedups as
a consequence of this change.

Fixes issue wjakob#307.

Author: wjakob

include/nanobind/eigen/dense.h

@@ -218,7 +218,6 @@ struct type_caster<T, enable_if_t<is_eigen_xpr_v<T> &&
     using Array = Eigen::Array<typename T::Scalar, T::RowsAtCompileTime,
                                T::ColsAtCompileTime>;
     using Caster = make_caster<Array>;
-    static constexpr bool IsClass = false;
     static constexpr auto Name = Caster::Name;
     template <typename T_> using Cast = T;
 
@@ -249,7 +248,6 @@ struct type_caster<Eigen::Map<T, Options, StrideType>,
                                                   const typename Map::Scalar,
                                                   typename Map::Scalar>>;
     using NDArrayCaster = type_caster<NDArray>;
-    static constexpr bool IsClass = false;
     static constexpr auto Name = NDArrayCaster::Name;
     template <typename T_> using Cast = Map;
 
@@ -395,7 +393,6 @@ struct type_caster<Eigen::Ref<T, Options, StrideType>,
     static constexpr bool DMapConstructorOwnsData =
         !Eigen::internal::traits<Ref>::template match<DMap>::type::value;
 
-    static constexpr bool IsClass = false;
     static constexpr auto Name =
         const_name<MaybeConvert>(DMapCaster::Name, MapCaster::Name);
 

include/nanobind/nb_cast.h

@@ -9,17 +9,16 @@
 
 #define NB_TYPE_CASTER(Value_, descr)                                          \
     using Value = Value_;                                                      \
-    static constexpr bool IsClass = false;                                     \
     static constexpr auto Name = descr;                                        \
     template <typename T_> using Cast = movable_cast_t<T_>;                    \
     static handle from_cpp(Value *p, rv_policy policy, cleanup_list *list) {   \
         if (!p)                                                                \
             return none().release();                                           \
         return from_cpp(*p, policy, list);                                     \
     }                                                                          \
-    explicit operator Value *() { return &value; }                             \
-    explicit operator Value &() { return value; }                              \
-    explicit operator Value &&() && { return (Value &&) value; }               \
+    explicit operator Value*() { return &value; }                              \
+    explicit operator Value&() { return (Value &) value; }                     \
+    explicit operator Value&&() { return (Value &&) value; }                   \
     Value value;
 
 #define NB_MAKE_OPAQUE(...)                                                    \
@@ -38,14 +37,45 @@ enum cast_flags : uint8_t {
     construct = (1 << 1)
 };
 
+/**
+ * Type casters expose a member 'Cast<T>' which users of a type caster must
+ * query to determine what the caster actually can (and prefers) to produce.
+ * The convenience alias ``cast_t<T>`` defined below performs this query for a
+ * given type ``T``.
+ *
+ * Often ``cast_t<T>`` is simply equal to ``T`` or ``T&``. More significant
+ * deviations are also possible, which could be due to one of the following
+ * two reasons:
+ *
+ * 1. Efficiency: most STL type casters create a local copy (``value`` member)
+ *    of the value being cast. The caller should move this value to its
+ *    intended destination instead of making further copies along the way.
+ *    Consequently, ``cast_t<std::vector<T>>`` yields ``cast_t<std::vector<T>>
+ *    &&`` to enable such behavior.
+ *
+ * 2. STL pairs may contain references, and such pairs aren't
+ *    default-constructible. The STL pair caster therefore cannot create a local
+ *    copy and must construct the pair on the fly, which in turns means that it
+ *    cannot return references. Therefore, ``cast_t<const std::pair<T1, T2>&>``
+ *    yields ``std::pair<T1, T2>``.
+ */
+
+/// Ask a type caster what flavors of a type it can actually produce -- may be different from 'T'
 template <typename T> using cast_t = typename make_caster<T>::template Cast<T>;
 
+/// This is a default choice for the 'Cast' type alias described above. It
+/// prefers to return rvalue references to allow the caller to move the object.
 template <typename T>
-using simple_cast_t =
-    std::conditional_t<is_pointer_v<T>, intrinsic_t<T> *, intrinsic_t<T> &>;
+using movable_cast_t =
+    std::conditional_t<is_pointer_v<T>, intrinsic_t<T> *,
+                       std::conditional_t<std::is_lvalue_reference_v<T>,
+                                          intrinsic_t<T> &, intrinsic_t<T> &&>>;
 
+/// This version is more careful about what the caller actually requested and
+/// only moves when this was explicitly requested. It is the default for the
+/// base type caster (i.e., types bound via ``nanobind::class_<..>``)
 template <typename T>
-using movable_cast_t =
+using precise_cast_t =
     std::conditional_t<is_pointer_v<T>, intrinsic_t<T> *,
                        std::conditional_t<std::is_rvalue_reference_v<T>,
                                           intrinsic_t<T> &&, intrinsic_t<T> &>>;
@@ -105,13 +135,11 @@ struct type_caster<T, enable_if_t<std::is_arithmetic_v<T> && !is_std_char_v<T>>>
 
 template <> struct type_caster<void_type> {
     static constexpr auto Name = const_name("None");
-    static constexpr bool IsClass = false;
 };
 
 template <> struct type_caster<void> {
     template <typename T_> using Cast = void *;
     using Value = void*;
-    static constexpr bool IsClass = false;
     static constexpr auto Name = const_name("capsule");
     explicit operator void *() { return value; }
     Value value;
@@ -175,7 +203,6 @@ template <> struct type_caster<bool> {
 template <> struct type_caster<char> {
     using Value = const char *;
     Value value;
-    static constexpr bool IsClass = false;
     static constexpr auto Name = const_name("str");
     template <typename T_>
     using Cast = std::conditional_t<is_pointer_v<T_>, const char *, char>;
@@ -289,12 +316,10 @@ template <typename T> NB_INLINE rv_policy infer_policy(rv_policy policy) {
 
 template <typename T, typename SFINAE = int> struct type_hook : std::false_type { };
 
-template <typename Type_> struct type_caster_base {
+template <typename Type_> struct type_caster_base : type_caster_base_tag {
     using Type = Type_;
     static constexpr auto Name = const_name<Type>();
-    static constexpr bool IsClass = true;
-
-    template <typename T> using Cast = movable_cast_t<T>;
+    template <typename T> using Cast = precise_cast_t<T>;
 
     NB_INLINE bool from_python(handle src, uint8_t flags,
                                cleanup_list *cleanup) noexcept {
@@ -335,7 +360,7 @@ template <typename Type_> struct type_caster_base {
         return *value;
     }
 
-    operator Type&&() && {
+    operator Type&&() {
         raise_next_overload_if_null(value);
         return (Type &&) *value;
     }
@@ -352,7 +377,6 @@ NAMESPACE_END(detail)
 template <typename T, typename Derived>
 bool try_cast(const detail::api<Derived> &value, T &out, bool convert = true) noexcept {
     using Caster = detail::make_caster<T>;
-    using Output = typename Caster::template Cast<T>;
 
     static_assert(!std::is_same_v<const char *, T>,
                   "nanobind::try_cast(): cannot return a reference to a temporary.");
@@ -361,11 +385,7 @@ bool try_cast(const detail::api<Derived> &value, T &out, bool convert = true) no
     if (caster.from_python(value.derived().ptr(),
                            convert ? (uint8_t) detail::cast_flags::convert
                                    : (uint8_t) 0, nullptr)) {
-        if constexpr (Caster::IsClass)
-            out = caster.operator Output();
-        else
-            out = std::move(caster.operator Output&&());
-
+        out = caster.operator detail::cast_t<T>();
         return true;
     }
 
@@ -378,11 +398,11 @@ T cast(const detail::api<Derived> &value, bool convert = true) {
         return;
     } else {
         using Caster = detail::make_caster<T>;
-        using Output = typename Caster::template Cast<T>;
 
         static_assert(
-            !(std::is_reference_v<T> || std::is_pointer_v<T>) || Caster::IsClass ||
-            std::is_same_v<const char *, T>,
+            !(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;
@@ -397,7 +417,7 @@ T cast(const detail::api<Derived> &value, bool convert = true) {
           #pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
         #endif
 
-        return caster.operator Output();
+        return caster.operator detail::cast_t<T>();
 
         #if defined(__GNUC__) && !defined(__clang__)
           #pragma GCC diagnostic pop
@@ -411,6 +431,7 @@ object cast(T &&value, rv_policy policy = rv_policy::automatic_reference) {
                                                 policy, nullptr);
     if (!h.is_valid())
         detail::raise_cast_error();
+
     return steal(h);
 }
 

include/nanobind/nb_class.h

@@ -293,12 +293,6 @@ template <typename... Args> struct init {
     NB_INLINE static void execute(Class &cl, const Extra&... extra) {
         using Type = typename Class::Type;
         using Alias = typename Class::Alias;
-        static_assert(
-            detail::make_caster<Type>::IsClass,
-            "Attempted to create a constructor for a type that won't be "
-            "handled by the nanobind's class type caster. Is it possible that "
-            "you forgot to add NB_MAKE_OPAQUE() somewhere?");
-
         cl.def(
             "__init__",
             [](pointer_and_handle<Type> v, Args... args) {
@@ -324,12 +318,6 @@ template <typename Arg> struct init_implicit {
     NB_INLINE static void execute(Class &cl, const Extra&... extra) {
         using Type = typename Class::Type;
         using Alias = typename Class::Alias;
-        using Caster = detail::make_caster<Arg>;
-        static_assert(
-            detail::make_caster<Type>::IsClass,
-            "Attempted to create a constructor for a type that won't be "
-            "handled by the nanobind's class type caster. Is it possible that "
-            "you forgot to add NB_MAKE_OPAQUE() somewhere?");
 
         cl.def(
             "__init__",
@@ -344,7 +332,9 @@ template <typename Arg> struct init_implicit {
                 new ((Alias *) v.p) Alias{ (detail::forward_t<Arg>) arg };
             }, is_implicit(), extra...);
 
-        if constexpr (!Caster::IsClass) {
+        using Caster = detail::make_caster<Arg>;
+
+        if constexpr (!detail::is_class_caster_v<Caster>) {
             detail::implicitly_convertible(
                 [](PyTypeObject *, PyObject *src,
                    detail::cleanup_list *cleanup) noexcept -> bool {
@@ -364,12 +354,22 @@ class class_ : public object {
     using Base  = typename detail::extract<T, detail::is_base,  Ts...>::type;
     using Alias = typename detail::extract<T, detail::is_alias, Ts...>::type;
 
-    static_assert(sizeof(Alias) < (1 << 24), "instance size is too big!");
-    static_assert(alignof(Alias) < (1 << 8), "instance alignment is too big!");
+    static_assert(sizeof(Alias) < (1 << 24), "Instance size is too big!");
+    static_assert(alignof(Alias) < (1 << 8), "Instance alignment is too big!");
     static_assert(
         sizeof...(Ts) == !std::is_same_v<Base, T> + !std::is_same_v<Alias, T>,
         "nanobind::class_<> was invoked with extra arguments that could not be handled");
 
+    static_assert(
+        detail::is_base_caster_v<detail::make_caster<Type>>,
+        "You attempted to bind a type that is already intercepted by a type "
+        "caster. Having both at the same time is not allowed. Are you perhaps "
+        "binding an STL type, while at the same time including a matching "
+        "type caster from <nanobind/stl/*>? Or did you perhaps forget to "
+        "declare NB_MAKE_OPAQUE(..) to specifically disable the type caster "
+        "catch-all for a specific type? Please review the documentation "
+        "to learn about the difference between bindings and type casters.");
+
     template <typename... Extra>
     NB_INLINE class_(handle scope, const char *name, const Extra &... extra) {
         detail::type_init_data d;
@@ -521,7 +521,9 @@ class class_ : public object {
         static_assert(std::is_base_of_v<C, T>,
                       "def_rw() requires a (base) class member!");
 
-        using Q = std::conditional_t<detail::make_caster<D>::IsClass, const D &, D &&>;
+        using Q =
+            std::conditional_t<detail::is_base_caster_v<detail::make_caster<D>>,
+                               const D &, D &&>;
 
         def_prop_rw(name,
             [p](const T &c) -> const D & { return c.*p; },
@@ -534,7 +536,9 @@ class class_ : public object {
     template <typename D, typename... Extra>
     NB_INLINE class_ &def_rw_static(const char *name, D *p,
                                     const Extra &...extra) {
-        using Q = std::conditional_t<detail::make_caster<D>::IsClass, const D &, D &&>;
+        using Q =
+            std::conditional_t<detail::is_base_caster_v<detail::make_caster<D>>,
+                               const D &, D &&>;
 
         def_prop_rw_static(name,
             [p](handle) -> const D & { return *p; },
@@ -624,7 +628,7 @@ template <typename T> class enum_ : public class_<T> {
 template <typename Source, typename Target> void implicitly_convertible() {
     using Caster = detail::make_caster<Source>;
 
-    if constexpr (Caster::IsClass) {
+    if constexpr (detail::is_base_caster_v<Caster>) {
         detail::implicitly_convertible(&typeid(Source), &typeid(Target));
     } else {
         detail::implicitly_convertible(

include/nanobind/nb_func.h

@@ -121,12 +121,12 @@ NB_INLINE PyObject *func_create(Func &&func, Return (*)(Args...),
 
         PyObject *result;
         if constexpr (std::is_void_v<Return>) {
-            cap->func(((make_caster<Args>&&) in.template get<Is>()).operator cast_t<Args>()...);
+            cap->func(in.template get<Is>().operator cast_t<Args>()...);
             result = Py_None;
             Py_INCREF(result);
         } else {
             result = cast_out::from_cpp(
-                       cap->func(((make_caster<Args> &&) in.template get<Is>())
+                       cap->func((in.template get<Is>())
                                      .operator cast_t<Args>()...),
                        policy, cleanup).ptr();
         }

include/nanobind/nb_traits.h

@@ -142,6 +142,21 @@ template <typename T>
 constexpr bool has_shared_from_this_v =
     decltype(has_shared_from_this_impl((T *) nullptr))::value;
 
+/// Base of all type casters for traditional bindings created via nanobind::class_<>
+struct type_caster_base_tag {
+    static constexpr bool IsClass = true;
+};
+
+/// Check if a type caster represents traditional bindings created via nanobind::class_<>
+template <typename Caster>
+constexpr bool is_base_caster_v = std::is_base_of_v<type_caster_base_tag, Caster>;
+
+template <typename T> using is_class_caster_test = std::enable_if_t<T::IsClass>;
+
+/// Generalized version of the is_base_caster_v test that also accepts unique_ptr/shared_ptr
+template <typename Caster>
+constexpr bool is_class_caster_v = detail::detector<void, is_class_caster_test, Caster>::value;
+
 NAMESPACE_END(detail)
 
 template <typename... Args>

include/nanobind/nb_types.h

@@ -551,7 +551,7 @@ template <typename T>
 NB_INLINE bool isinstance(handle h) noexcept {
     if constexpr (std::is_base_of_v<handle, T>)
         return T::check_(h);
-    else if constexpr (detail::make_caster<T>::IsClass)
+    else if constexpr (detail::is_base_caster_v<detail::make_caster<T>>)
         return detail::nb_type_isinstance(h.ptr(), &typeid(detail::intrinsic_t<T>));
     else
         return detail::make_caster<T>().from_python(h, 0, nullptr);

include/nanobind/stl/detail/nb_array.h

@@ -27,7 +27,7 @@ template <typename Value_, typename Entry, size_t Size> struct array_caster {
                     break;
                 }
 
-                value[i] = ((Caster &&) caster).operator cast_t<Entry &&>();
+                value[i] = caster.operator cast_t<Entry>();
             }
 
             Py_XDECREF(temp);

include/nanobind/stl/detail/nb_dict.h

@@ -51,8 +51,8 @@ template <typename Value_, typename Key, typename Element> struct dict_caster {
                 break;
             }
 
-            value.emplace(((KeyCaster &&) key_caster).operator cast_t<Key &&>(),
-                          ((ElementCaster &&) element_caster).operator cast_t<Element &&>());
+            value.emplace(key_caster.operator cast_t<Key>(),
+                          element_caster.operator cast_t<Element>());
         }
 
         Py_DECREF(items);

include/nanobind/stl/detail/nb_list.h

@@ -44,7 +44,7 @@ template <typename Value_, typename Entry> struct list_caster {
                 break;
             }
 
-            value.push_back(((Caster &&) caster).operator cast_t<Entry &&>());
+            value.push_back(caster.operator cast_t<Entry>());
         }
 
         Py_XDECREF(temp);

include/nanobind/stl/detail/nb_set.h

@@ -39,7 +39,7 @@ template <typename Value_, typename Key> struct set_caster {
             if (!success)
                 break;
 
-            value.emplace(((KeyCaster &&) key_caster).operator cast_t<Key&&>());
+            value.emplace(key_caster.operator cast_t<Key>());
         }
 
         if (PyErr_Occurred()) {

include/nanobind/stl/detail/traits.h

@@ -62,7 +62,7 @@ template <typename T>
 constexpr bool is_copy_assignable_v = is_copy_assignable<T>::value;
 
 // Analogous template for checking comparability
-template<typename T> using comparable_test = decltype(std::declval<T>() == std::declval<T>());
+template <typename T> using comparable_test = decltype(std::declval<T>() == std::declval<T>());
 
 template <typename T, typename SFINAE = int>
 struct is_equality_comparable {