feat: Slice allowing None with py::object or std::optional (#1101)

* Adding nullptr slices

Using example from #1095

Some fixes from @wjakob's review

Stop clang-tidy from complaining

New proposal for py::slice constructor

Eric's suggested changes: simplify testing; shift def's

* chore: drop MSVC pragma (hopefully unneeded)

* Apply suggestions from code review

include/pybind11/detail/common.h

@@ -161,7 +161,26 @@
 
 // https://en.cppreference.com/w/c/chrono/localtime
 #if defined(__STDC_LIB_EXT1__) && !defined(__STDC_WANT_LIB_EXT1__)
-#    define __STDC_WANT_LIB_EXT1__
+#  define __STDC_WANT_LIB_EXT1__
+#endif
+
+#ifdef __has_include
+// std::optional (but including it in c++14 mode isn't allowed)
+#  if defined(PYBIND11_CPP17) && __has_include(<optional>)
+#    define PYBIND11_HAS_OPTIONAL 1
+#  endif
+// std::experimental::optional (but not allowed in c++11 mode)
+#  if defined(PYBIND11_CPP14) && (__has_include(<experimental/optional>) && \
+                                 !__has_include(<optional>))
+#    define PYBIND11_HAS_EXP_OPTIONAL 1
+#  endif
+// std::variant
+#  if defined(PYBIND11_CPP17) && __has_include(<variant>)
+#    define PYBIND11_HAS_VARIANT 1
+#  endif
+#elif defined(_MSC_VER) && defined(PYBIND11_CPP17)
+#  define PYBIND11_HAS_OPTIONAL 1
+#  define PYBIND11_HAS_VARIANT 1
 #endif
 
 #include <Python.h>

include/pybind11/pytypes.h

@@ -14,6 +14,10 @@
 #include <utility>
 #include <type_traits>
 
+#if defined(PYBIND11_HAS_OPTIONAL)
+#  include <optional>
+#endif
+
 PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
 
 /* A few forward declarations */
@@ -1345,11 +1349,20 @@ private:
 class slice : public object {
 public:
     PYBIND11_OBJECT_DEFAULT(slice, object, PySlice_Check)
-    slice(ssize_t start_, ssize_t stop_, ssize_t step_) {
-        int_ start(start_), stop(stop_), step(step_);
+    slice(handle start, handle stop, handle step) {
         m_ptr = PySlice_New(start.ptr(), stop.ptr(), step.ptr());
-        if (!m_ptr) pybind11_fail("Could not allocate slice object!");
+        if (!m_ptr)
+            pybind11_fail("Could not allocate slice object!");
     }
+
+#ifdef PYBIND11_HAS_OPTIONAL
+    slice(std::optional<ssize_t> start, std::optional<ssize_t> stop, std::optional<ssize_t> step)
+        : slice(index_to_object(start), index_to_object(stop), index_to_object(step)) {}
+#else
+    slice(ssize_t start_, ssize_t stop_, ssize_t step_)
+        : slice(int_(start_), int_(stop_), int_(step_)) {}
+#endif
+
     bool compute(size_t length, size_t *start, size_t *stop, size_t *step,
                  size_t *slicelength) const {
         return PySlice_GetIndicesEx((PYBIND11_SLICE_OBJECT *) m_ptr,
@@ -1364,6 +1377,12 @@ public:
           stop, step,
           slicelength) == 0;
     }
+
+private:
+    template <typename T>
+    static object index_to_object(T index) {
+        return index ? object(int_(*index)) : object(none());
+    }
 };
 
 class capsule : public object {

include/pybind11/stl.h

@@ -9,6 +9,7 @@
 
 #pragma once
 
+#include "detail/common.h"
 #include "pybind11.h"
 #include <set>
 #include <unordered_set>
@@ -19,28 +20,15 @@
 #include <deque>
 #include <valarray>
 
-#ifdef __has_include
-// std::optional (but including it in c++14 mode isn't allowed)
-#  if defined(PYBIND11_CPP17) && __has_include(<optional>)
-#    include <optional>
-#    define PYBIND11_HAS_OPTIONAL 1
-#  endif
-// std::experimental::optional (but not allowed in c++11 mode)
-#  if defined(PYBIND11_CPP14) && (__has_include(<experimental/optional>) && \
-                                 !__has_include(<optional>))
-#    include <experimental/optional>
-#    define PYBIND11_HAS_EXP_OPTIONAL 1
-#  endif
-// std::variant
-#  if defined(PYBIND11_CPP17) && __has_include(<variant>)
-#    include <variant>
-#    define PYBIND11_HAS_VARIANT 1
-#  endif
-#elif defined(_MSC_VER) && defined(PYBIND11_CPP17)
+// See `detail/common.h` for implementation of these guards.
+#if defined(PYBIND11_HAS_OPTIONAL)
 #  include <optional>
+#elif defined(PYBIND11_HAS_EXP_OPTIONAL)
+#  include <experimental/optional>
+#endif
+
+#if defined(PYBIND11_HAS_VARIANT)
 #  include <variant>
-#  define PYBIND11_HAS_OPTIONAL 1
-#  define PYBIND11_HAS_VARIANT 1
 #endif
 
 PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)

tests/test_sequences_and_iterators.cpp

@@ -17,6 +17,11 @@
 #include <utility>
 #include <vector>
 
+#ifdef PYBIND11_HAS_OPTIONAL
+#include <optional>
+#endif  // PYBIND11_HAS_OPTIONAL
+
+
 template<typename T>
 class NonZeroIterator {
     const T* ptr_;
@@ -117,6 +122,18 @@ TEST_SUBMODULE(sequences_and_iterators, m) {
             return std::make_tuple(istart, istop, istep);
         });
 
+    m.def("make_forward_slice_size_t", []() { return py::slice(0, -1, 1); });
+    m.def("make_reversed_slice_object", []() { return py::slice(py::none(), py::none(), py::int_(-1)); });
+#ifdef PYBIND11_HAS_OPTIONAL
+    m.attr("has_optional") = true;
+    m.def("make_reversed_slice_size_t_optional_verbose", []() { return py::slice(std::nullopt, std::nullopt, -1); });
+    // Warning: The following spelling may still compile if optional<> is not present and give wrong answers.
+    // Please use with caution.
+    m.def("make_reversed_slice_size_t_optional", []() { return py::slice({}, {}, -1); });
+#else
+    m.attr("has_optional") = false;
+#endif
+
     // test_sequence
     class Sequence {
     public:

tests/test_sequences_and_iterators.py

@@ -16,6 +16,17 @@ def allclose(a_list, b_list, rel_tol=1e-05, abs_tol=0.0):
     )
 
 
+def test_slice_constructors():
+    assert m.make_forward_slice_size_t() == slice(0, -1, 1)
+    assert m.make_reversed_slice_object() == slice(None, None, -1)
+
+
+@pytest.mark.skipif(not m.has_optional, reason="no <optional>")
+def test_slice_constructors_explicit_optional():
+    assert m.make_reversed_slice_size_t_optional() == slice(None, None, -1)
+    assert m.make_reversed_slice_size_t_optional_verbose() == slice(None, None, -1)
+
+
 def test_generalized_iterators():
     assert list(m.IntPairs([(1, 2), (3, 4), (0, 5)]).nonzero()) == [(1, 2), (3, 4)]
     assert list(m.IntPairs([(1, 2), (2, 0), (0, 3), (4, 5)]).nonzero()) == [(1, 2)]