diff --git a/docs/changelog.rst b/docs/changelog.rst index cc3a25d6..5a86e57d 100644 --- a/docs/changelog.rst +++ b/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>`` that require conversion + no longer produce a dangling reference. (PR `#1025 + `__). + +Version 2.7.0 (Apr 18, 2025) +---------------------------- + - nanobind now provides a zero-copy type caster for ``Eigen::Map``. (PRs `#1003 `__, `#782 diff --git a/include/nanobind/eigen/dense.h b/include/nanobind/eigen/dense.h index b8224cb5..9709251e 100644 --- a/include/nanobind/eigen/dense.h +++ b/include/nanobind/eigen/dense.h @@ -381,6 +381,7 @@ struct type_caster, // Extended version taking arbitrary strides using DMap = Eigen::Map; using DMapCaster = make_caster; + static constexpr bool DMapIsDifferent = !std::is_same_v; /** * The constructor of ``Ref`` uses one of two strategies @@ -391,14 +392,66 @@ struct type_caster, * * When the value below is ``true``, then it is guaranteed that * ``Ref()`` 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::template match::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>``, 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 = 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(DMapCaster::Name, MapCaster::Name); - template using Cast = Ref; + template using Cast = ToRef; template static constexpr bool can_cast() { return true; } MapCaster caster; @@ -459,13 +512,13 @@ struct type_caster, 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()); } }; diff --git a/tests/test_eigen.cpp b/tests/test_eigen.cpp index 904e1159..1c2f1b21 100644 --- a/tests/test_eigen.cpp +++ b/tests/test_eigen.cpp @@ -1,4 +1,5 @@ #include +#include #include #include #include @@ -144,6 +145,22 @@ NB_MODULE(test_eigen_ext, m) { m.def("updateRefVXi", [](Eigen::Ref a) { a[2] = 123; }); m.def("updateRefVXi_nc", [](Eigen::Ref a) { a[2] = 123; }, nb::arg().noconvert()); + m.def("addOptRefVXi", + [](std::optional> r1, + std::optional> r2) + -> std::optional { + 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; using SparseMatrixC = Eigen::SparseMatrix; Eigen::MatrixXf mat(5, 6); diff --git a/tests/test_eigen.py b/tests/test_eigen.py index 304437e3..d874335e 100644 --- a/tests/test_eigen.py +++ b/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> + 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():