Eigen caster: fix lifetime issue when casting std::optional<Eigen::Ref<const T>> with conversion

The conversion process initially produces an Eigen::Ref that owns its storage, but copying or moving it produces an Eigen::Ref that refers non-owningly to the storage owned by the first Ref. The first Ref is destroyed before the result can be used, leading to a dangling reference. Fix by having the Eigen caster return a type that encapsulates the constructor argument of Ref rather than a Ref itself, so that the owning Ref can be constructed in-place in its final location.

Author: oremanj

docs/changelog.rst

@@ -15,9 +15,16 @@ case, both modules must use the same nanobind ABI version, or they will be
 isolated from each other. Releases that don't explicitly mention an ABI version
 below inherit that of the preceding release.
 
-Version 2.7.0 (Apr 18, 2025)
+Version TBD (unreleased)
 ------------------------
 
+- Casts to ``std::optional<Eigen::Ref<const ..>>`` that require conversion
+  no longer produce a dangling reference. (PR `#1025
+  <https://github.com/wjakob/nanobind/pull/1025>`__).
+
+Version 2.7.0 (Apr 18, 2025)
+----------------------------
+
 - nanobind now provides a zero-copy type caster for
   ``Eigen::Map<Eigen::SparseMatrix>``. (PRs `#1003
   <https://github.com/wjakob/nanobind/pull/1003>`__, `#782

include/nanobind/eigen/dense.h

@@ -381,6 +381,7 @@ struct type_caster<Eigen::Ref<T, Options, StrideType>,
     // Extended version taking arbitrary strides
     using DMap = Eigen::Map<const T, Options, DStride>;
     using DMapCaster = make_caster<DMap>;
+    static constexpr bool DMapIsDifferent = !std::is_same_v<Map, DMap>;
 
     /**
      * The constructor of ``Ref<const T>`` uses one of two strategies
@@ -391,14 +392,66 @@ struct type_caster<Eigen::Ref<T, Options, StrideType>,
      *
      * When the value below is ``true``, then it is guaranteed that
      * ``Ref(<DMap instance>)`` owns the underlying data.
+     * Note that this ownership does *not* propagate to any further ``Ref``
+     * that might be constructed from it; the ``ToRef`` wrapper below
+     * attempts to avoid this hazard by ensuring there is no need to copy
+     * or move the initial ``Ref``.
      */
     static constexpr bool DMapConstructorOwnsData =
         !Eigen::internal::traits<Ref>::template match<DMap>::type::value;
 
+    /**
+     * Result type of our cast operator, which can be used to construct a
+     * ``Ref`` from either ``Map`` or ``DMap``. Unfortunately, ``Ref`` does
+     * not model any sort of shared ownership; if we wind up creating a ``Ref``
+     * that owns the data, then copying it will produce a second ``Ref`` that
+     * refers to the original ``Ref``, which is likely to go out of scope
+     * soon. Eigen also has very minimal support for C++11 features such as
+     * moves, and in particular moving a ``Ref`` is equivalent to copying it
+     * in all released versions of Eigen as of this writing.
+     * This creates problems for nested type casters such as
+     * ``std::optional<Eigen::Ref<..>>``, which can't directly construct the
+     * cast operator's return value inside their data structure.
+     */
+    struct ToRef {
+        // DMap has maximally dynamic strides, so it won't contain
+        // fewer fields than Map (which might have some stride info
+        // known at compile time).
+        static_assert(alignof(DMap) >= alignof(Map) &&
+                      sizeof(DMap) >= sizeof(Map));
+        alignas(DMap) char storage[sizeof(DMap)];
+        bool holds_dmap;
+        static constexpr bool MightHoldDMap = MaybeConvert && DMapIsDifferent;
+
+        ToRef(Map m) : holds_dmap(false) { new(&storage) Map(m); }
+        template <bool Enable = MightHoldDMap, enable_if_t<Enable> = 0>
+        ToRef(DMap m) : holds_dmap(true) { new(&storage) DMap(m); }
+        ToRef(const ToRef&) = delete;
+        ToRef& operator=(const ToRef&) = delete;
+        ~ToRef() {
+            if constexpr (MightHoldDMap) {
+                if (holds_dmap) {
+                    ((DMap *) storage)->~DMap();
+                    return;
+                }
+            }
+            ((Map *) storage)->~Map();
+        }
+
+        operator Ref() const {
+            if constexpr (MightHoldDMap) {
+                if (holds_dmap) {
+                    return Ref(*(DMap *) storage);
+                }
+            }
+            return Ref(*(Map *) storage);
+        }
+    };
+
     static constexpr auto Name =
         const_name<MaybeConvert>(DMapCaster::Name, MapCaster::Name);
 
-    template <typename T_> using Cast = Ref;
+    template <typename T_> using Cast = ToRef;
     template <typename T_> static constexpr bool can_cast() { return true; }
 
     MapCaster caster;
@@ -459,13 +512,13 @@ struct type_caster<Eigen::Ref<T, Options, StrideType>,
             cleanup);
     }
 
-    operator Ref() {
+    operator ToRef() {
         if constexpr (MaybeConvert) {
             if (dcaster.caster.value.is_valid())
-                return Ref(dcaster.operator DMap());
+                return ToRef(dcaster.operator DMap());
         }
 
-        return Ref(caster.operator Map());
+        return ToRef(caster.operator Map());
     }
 };
 

tests/test_eigen.cpp

@@ -1,4 +1,5 @@
 #include <nanobind/stl/complex.h>
+#include <nanobind/stl/optional.h>
 #include <nanobind/eigen/dense.h>
 #include <nanobind/eigen/sparse.h>
 #include <nanobind/trampoline.h>
@@ -144,6 +145,22 @@ NB_MODULE(test_eigen_ext, m) {
     m.def("updateRefVXi", [](Eigen::Ref<Eigen::VectorXi> a) { a[2] = 123; });
     m.def("updateRefVXi_nc", [](Eigen::Ref<Eigen::VectorXi> a) { a[2] = 123; }, nb::arg().noconvert());
 
+    m.def("addOptRefVXi",
+          [](std::optional<Eigen::Ref<const Eigen::VectorXi>> r1,
+             std::optional<Eigen::Ref<const Eigen::VectorXi>> r2)
+                  -> std::optional<Eigen::VectorXi> {
+              if (!r1.has_value() && !r2.has_value()) {
+                  return std::nullopt;
+              }
+              if (!r1.has_value()) {
+                  return *r2;
+              }
+              if (!r2.has_value()) {
+                  return *r1;
+              }
+              return *r1 + *r2;
+          }, "r1"_a.none(), "r2"_a.none());
+
     using SparseMatrixR = Eigen::SparseMatrix<float, Eigen::RowMajor>;
     using SparseMatrixC = Eigen::SparseMatrix<float>;
     Eigen::MatrixXf mat(5, 6);

tests/test_eigen.py

@@ -77,6 +77,12 @@ def test02_vector_dynamic():
     # Try with a big array. This will move the result to avoid a copy
     assert_array_equal(t.addVXi(x, x), 2*x)
 
+    # Regression test for lifetime issues casting std::optional<Ref<VectorXi>>
+    assert_array_equal(t.addOptRefVXi(af, b), c)
+    assert_array_equal(t.addOptRefVXi(af, None), af)
+    assert_array_equal(t.addOptRefVXi(None, b), b)
+    assert t.addOptRefVXi(None, None) is None
+
 
 @needs_numpy_and_eigen
 def test03_update_map():