From 03c50a18392cdb183aa059d65d74dbca9fdc3ec3 Mon Sep 17 00:00:00 2001
From: Jacob Zhong <zyxin@umich.edu>
Date: Thu, 18 Apr 2024 15:33:07 +0800
Subject: [PATCH] Change the types of `PySliceIndices` and `PySlice::indices
 (#3761)

* Change the type of `PySliceIndices::slicelength` and `PySlice::indices()`

* Fix example

* Fix fmt
---
 examples/getitem/src/lib.rs   |  5 ++---
 newsfragments/3761.changed.md |  1 +
 src/types/slice.rs            | 23 +++++++++++++----------
 3 files changed, 16 insertions(+), 13 deletions(-)
 create mode 100644 newsfragments/3761.changed.md

diff --git a/examples/getitem/src/lib.rs b/examples/getitem/src/lib.rs
index c3c662ab..ce162a70 100644
--- a/examples/getitem/src/lib.rs
+++ b/examples/getitem/src/lib.rs
@@ -2,7 +2,6 @@
 use pyo3::exceptions::PyTypeError;
 use pyo3::prelude::*;
 use pyo3::types::PySlice;
-use std::os::raw::c_long;
 
 #[derive(FromPyObject)]
 enum IntOrSlice<'py> {
@@ -29,7 +28,7 @@ impl ExampleContainer {
         } else if let Ok(slice) = key.downcast::<PySlice>() {
             // METHOD 1 - the use PySliceIndices to help with bounds checking and for cases when only start or end are provided
             // in this case the start/stop/step all filled in to give valid values based on the max_length given
-            let index = slice.indices(self.max_length as c_long).unwrap();
+            let index = slice.indices(self.max_length as isize).unwrap();
             let _delta = index.stop - index.start;
 
             // METHOD 2 - Do the getattr manually really only needed if you have some special cases for stop/_step not being present
@@ -62,7 +61,7 @@ impl ExampleContainer {
     fn __setitem__(&self, idx: IntOrSlice, value: u32) -> PyResult<()> {
         match idx {
             IntOrSlice::Slice(slice) => {
-                let index = slice.indices(self.max_length as c_long).unwrap();
+                let index = slice.indices(self.max_length as isize).unwrap();
                 println!(
                     "Got a slice! {}-{}, step: {}, value: {}",
                     index.start, index.stop, index.step, value
diff --git a/newsfragments/3761.changed.md b/newsfragments/3761.changed.md
new file mode 100644
index 00000000..fd084721
--- /dev/null
+++ b/newsfragments/3761.changed.md
@@ -0,0 +1 @@
+Change the type of `PySliceIndices::slicelength` and the `length` parameter of `PySlice::indices()`.
diff --git a/src/types/slice.rs b/src/types/slice.rs
index 8e754520..b6895d09 100644
--- a/src/types/slice.rs
+++ b/src/types/slice.rs
@@ -1,13 +1,12 @@
 use crate::err::{PyErr, PyResult};
-use crate::ffi::{self, Py_ssize_t};
+use crate::ffi;
 use crate::ffi_ptr_ext::FfiPtrExt;
 use crate::types::any::PyAnyMethods;
 use crate::{Bound, PyAny, PyNativeType, PyObject, Python, ToPyObject};
-use std::os::raw::c_long;
 
 /// Represents a Python `slice`.
 ///
-/// Only `c_long` indices supported at the moment by the `PySlice` object.
+/// Only `isize` indices supported at the moment by the `PySlice` object.
 #[repr(transparent)]
 pub struct PySlice(PyAny);
 
@@ -22,13 +21,17 @@ pyobject_native_type!(
 #[derive(Debug, Eq, PartialEq)]
 pub struct PySliceIndices {
     /// Start of the slice
+    ///
+    /// It can be -1 when the step is negative, otherwise it's non-negative.
     pub start: isize,
     /// End of the slice
+    ///
+    /// It can be -1 when the step is negative, otherwise it's non-negative.
     pub stop: isize,
     /// Increment to use when iterating the slice from `start` to `stop`.
     pub step: isize,
     /// The length of the slice calculated from the original input sequence.
-    pub slicelength: isize,
+    pub slicelength: usize,
 }
 
 impl PySliceIndices {
@@ -94,7 +97,7 @@ impl PySlice {
     /// assuming a sequence of length `length`, and stores the length of the
     /// slice in its `slicelength` member.
     #[inline]
-    pub fn indices(&self, length: c_long) -> PyResult<PySliceIndices> {
+    pub fn indices(&self, length: isize) -> PyResult<PySliceIndices> {
         self.as_borrowed().indices(length)
     }
 }
@@ -109,12 +112,11 @@ pub trait PySliceMethods<'py>: crate::sealed::Sealed {
     /// Retrieves the start, stop, and step indices from the slice object,
     /// assuming a sequence of length `length`, and stores the length of the
     /// slice in its `slicelength` member.
-    fn indices(&self, length: c_long) -> PyResult<PySliceIndices>;
+    fn indices(&self, length: isize) -> PyResult<PySliceIndices>;
 }
 
 impl<'py> PySliceMethods<'py> for Bound<'py, PySlice> {
-    fn indices(&self, length: c_long) -> PyResult<PySliceIndices> {
-        // non-negative Py_ssize_t should always fit into Rust usize
+    fn indices(&self, length: isize) -> PyResult<PySliceIndices> {
         unsafe {
             let mut slicelength: isize = 0;
             let mut start: isize = 0;
@@ -122,7 +124,7 @@ impl<'py> PySliceMethods<'py> for Bound<'py, PySlice> {
             let mut step: isize = 0;
             let r = ffi::PySlice_GetIndicesEx(
                 self.as_ptr(),
-                length as Py_ssize_t,
+                length,
                 &mut start,
                 &mut stop,
                 &mut step,
@@ -133,7 +135,8 @@ impl<'py> PySliceMethods<'py> for Bound<'py, PySlice> {
                     start,
                     stop,
                     step,
-                    slicelength,
+                    // non-negative isize should always fit into usize
+                    slicelength: slicelength as _,
                 })
             } else {
                 Err(PyErr::fetch(self.py()))