- PyList, PyTuple and PySequence's get_item now accepts only usize indices instead of isize.

Aviram Hassan · davidhewitt · commit c6255e673411 · 2021-08-17T15:01:22.000+01:00
- `PyList` and `PyTuple`'s `get_item` now always uses the safe API. See `get_item_unchecked` for retrieving index without checks.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,11 +10,14 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 ### Added
 
+- Add `PyList::get_item_unchecked()` and `PyTuple::get_item_unchecked()` to get items without bounds checks. [#1733](https://github.com/PyO3/pyo3/pull/1733)
 - Add `PyAny::py()` as a convenience for `PyNativeType::py()`. [#1751](https://github.com/PyO3/pyo3/pull/1751)
 
 ### Changed
 
 - Change `PyErr::fetch()` to return `Option<PyErr>`. [#1717](https://github.com/PyO3/pyo3/pull/1717)
+- `PyList`, `PyTuple` and `PySequence`'s `get_item` now accepts only `usize` indices instead of `isize`. [#1733](https://github.com/PyO3/pyo3/pull/1733)
+- `PyList` and `PyTuple`'s `get_item` now return `PyResult<&PyAny> instead of panicking. [#1733](https://github.com/PyO3/pyo3/pull/1733)
 - Deprecate `PyTuple::split_from`. [#1804](https://github.com/PyO3/pyo3/pull/1804)
 
 ### Fixed
diff --git a/benches/bench_list.rs b/benches/bench_list.rs
@@ -32,7 +32,22 @@ fn list_get_item(b: &mut Bencher) {
     let mut sum = 0;
     b.iter(|| {
         for i in 0..LEN {
-            sum += list.get_item(i as isize).extract::<usize>().unwrap();
+            sum += list.get_item(i).unwrap().extract::<usize>().unwrap();
+        }
+    });
+}
+
+fn list_get_item_unchecked(b: &mut Bencher) {
+    let gil = Python::acquire_gil();
+    let py = gil.python();
+    const LEN: usize = 50_000;
+    let list = PyList::new(py, 0..LEN);
+    let mut sum = 0;
+    b.iter(|| {
+        for i in 0..LEN {
+            unsafe {
+                sum += list.get_item_unchecked(i).extract::<usize>().unwrap();
+            }
         }
     });
 }
@@ -41,6 +56,7 @@ fn criterion_benchmark(c: &mut Criterion) {
     c.bench_function("iter_list", iter_list);
     c.bench_function("list_new", list_new);
     c.bench_function("list_get_item", list_get_item);
+    c.bench_function("list_get_item_unchecked", list_get_item_unchecked);
 }
 
 criterion_group!(benches, criterion_benchmark);
diff --git a/benches/bench_tuple.rs b/benches/bench_tuple.rs
@@ -32,7 +32,22 @@ fn tuple_get_item(b: &mut Bencher) {
     let mut sum = 0;
     b.iter(|| {
         for i in 0..LEN {
-            sum += tuple.get_item(i).extract::<usize>().unwrap();
+            sum += tuple.get_item(i).unwrap().extract::<usize>().unwrap();
+        }
+    });
+}
+
+fn tuple_get_item_unchecked(b: &mut Bencher) {
+    let gil = Python::acquire_gil();
+    let py = gil.python();
+    const LEN: usize = 50_000;
+    let tuple = PyTuple::new(py, 0..LEN);
+    let mut sum = 0;
+    b.iter(|| {
+        for i in 0..LEN {
+            unsafe {
+                sum += tuple.get_item_unchecked(i).extract::<usize>().unwrap();
+            }
         }
     });
 }
@@ -41,6 +56,7 @@ fn criterion_benchmark(c: &mut Criterion) {
     c.bench_function("iter_tuple", iter_tuple);
     c.bench_function("tuple_new", tuple_new);
     c.bench_function("tuple_get_item", tuple_get_item);
+    c.bench_function("tuple_get_item_unchecked", tuple_get_item_unchecked);
 }
 
 criterion_group!(benches, criterion_benchmark);
diff --git a/pyo3-macros-backend/src/from_pyobject.rs b/pyo3-macros-backend/src/from_pyobject.rs
@@ -243,7 +243,7 @@ impl<'a> Container<'a> {
         for i in 0..len {
             let error_msg = format!("failed to extract field {}.{}", quote!(#self_ty), i);
             fields.push(quote!(
-                s.get_item(#i).extract().map_err(|inner| {
+                s.get_item(#i).and_then(PyAny::extract).map_err(|inner| {
                 let py = pyo3::PyNativeType::py(obj);
                 let new_err = pyo3::exceptions::PyTypeError::new_err(#error_msg);
                 new_err.set_cause(py, Some(inner));
diff --git a/src/conversions/array.rs b/src/conversions/array.rs
@@ -60,7 +60,7 @@ mod min_const_generics {
         if seq_len != N {
             return Err(invalid_sequence_length(N, seq_len));
         }
-        array_try_from_fn(|idx| seq.get_item(idx as isize).and_then(PyAny::extract))
+        array_try_from_fn(|idx| seq.get_item(idx).and_then(PyAny::extract))
     }
 
     // TODO use std::array::try_from_fn, if that stabilises:
diff --git a/src/types/dict.rs b/src/types/dict.rs
@@ -639,8 +639,8 @@ mod tests {
             let mut value_sum = 0;
             for el in dict.items().iter() {
                 let tuple = el.cast_as::<PyTuple>().unwrap();
-                key_sum += tuple.get_item(0).extract::<i32>().unwrap();
-                value_sum += tuple.get_item(1).extract::<i32>().unwrap();
+                key_sum += tuple.get_item(0).unwrap().extract::<i32>().unwrap();
+                value_sum += tuple.get_item(1).unwrap().extract::<i32>().unwrap();
             }
             assert_eq!(7 + 8 + 9, key_sum);
             assert_eq!(32 + 42 + 123, value_sum);
diff --git a/src/types/list.rs b/src/types/list.rs
@@ -66,23 +66,39 @@ impl PyList {
         self.len() == 0
     }
 
-    /// Gets the item at the specified index.
-    ///
-    /// Panics if the index is out of range.
-    pub fn get_item(&self, index: isize) -> &PyAny {
-        assert!(index >= 0 && index < self.len() as isize);
+    /// Gets the list item at the specified index.
+    /// # Example
+    /// ```
+    /// use pyo3::{prelude::*, types::PyList};
+    /// Python::with_gil(|py| {
+    ///     let list = PyList::new(py, &[2, 3, 5, 7]);
+    ///     let obj = list.get_item(0);
+    ///     assert_eq!(obj.unwrap().extract::<i32>().unwrap(), 2);
+    /// });
+    /// ```
+    pub fn get_item(&self, index: usize) -> PyResult<&PyAny> {
         unsafe {
-            #[cfg(not(Py_LIMITED_API))]
-            let ptr = ffi::PyList_GET_ITEM(self.as_ptr(), index as Py_ssize_t);
-            #[cfg(Py_LIMITED_API)]
-            let ptr = ffi::PyList_GetItem(self.as_ptr(), index as Py_ssize_t);
-
+            let item = ffi::PyList_GetItem(self.as_ptr(), index as Py_ssize_t);
             // PyList_GetItem return borrowed ptr; must make owned for safety (see #890).
-            ffi::Py_INCREF(ptr);
-            self.py().from_owned_ptr(ptr)
+            ffi::Py_XINCREF(item);
+            self.py().from_owned_ptr_or_err(item)
         }
     }
 
+    /// Gets the list item at the specified index. Undefined behavior on bad index. Use with caution.
+    ///
+    /// # Safety
+    ///
+    /// Caller must verify that the index is within the bounds of the list.
+    #[cfg(not(any(Py_LIMITED_API, PyPy)))]
+    #[cfg_attr(docsrs, doc(cfg(not(any(Py_LIMITED_API, PyPy)))))]
+    pub unsafe fn get_item_unchecked(&self, index: usize) -> &PyAny {
+        let item = ffi::PyList_GET_ITEM(self.as_ptr(), index as Py_ssize_t);
+        // PyList_GET_ITEM return borrowed ptr; must make owned for safety (see #890).
+        ffi::Py_XINCREF(item);
+        self.py().from_owned_ptr(item)
+    }
+
     /// Takes the slice `self[low:high]` and returns it as a new list.
     ///
     /// Indices must be nonnegative, and out-of-range indices are clipped to
@@ -163,16 +179,19 @@ impl PyList {
 /// Used by `PyList::iter()`.
 pub struct PyListIterator<'a> {
     list: &'a PyList,
-    index: isize,
+    index: usize,
 }
 
 impl<'a> Iterator for PyListIterator<'a> {
     type Item = &'a PyAny;
 
     #[inline]
     fn next(&mut self) -> Option<&'a PyAny> {
-        if self.index < self.list.len() as isize {
-            let item = self.list.get_item(self.index);
+        if self.index < self.list.len() {
+            #[cfg(any(Py_LIMITED_API, PyPy))]
+            let item = self.list.get_item(self.index).expect("tuple.get failed");
+            #[cfg(not(any(Py_LIMITED_API, PyPy)))]
+            let item = unsafe { self.list.get_item_unchecked(self.index) };
             self.index += 1;
             Some(item)
         } else {
@@ -185,8 +204,8 @@ impl<'a> Iterator for PyListIterator<'a> {
         let len = self.list.len();
 
         (
-            len.saturating_sub(self.index as usize),
-            Some(len.saturating_sub(self.index as usize)),
+            len.saturating_sub(self.index),
+            Some(len.saturating_sub(self.index)),
         )
     }
 }
@@ -237,10 +256,10 @@ mod tests {
     fn test_new() {
         Python::with_gil(|py| {
             let list = PyList::new(py, &[2, 3, 5, 7]);
-            assert_eq!(2, list.get_item(0).extract::<i32>().unwrap());
-            assert_eq!(3, list.get_item(1).extract::<i32>().unwrap());
-            assert_eq!(5, list.get_item(2).extract::<i32>().unwrap());
-            assert_eq!(7, list.get_item(3).extract::<i32>().unwrap());
+            assert_eq!(2, list.get_item(0).unwrap().extract::<i32>().unwrap());
+            assert_eq!(3, list.get_item(1).unwrap().extract::<i32>().unwrap());
+            assert_eq!(5, list.get_item(2).unwrap().extract::<i32>().unwrap());
+            assert_eq!(7, list.get_item(3).unwrap().extract::<i32>().unwrap());
         });
     }
 
@@ -256,19 +275,10 @@ mod tests {
     fn test_get_item() {
         Python::with_gil(|py| {
             let list = PyList::new(py, &[2, 3, 5, 7]);
-            assert_eq!(2, list.get_item(0).extract::<i32>().unwrap());
-            assert_eq!(3, list.get_item(1).extract::<i32>().unwrap());
-            assert_eq!(5, list.get_item(2).extract::<i32>().unwrap());
-            assert_eq!(7, list.get_item(3).extract::<i32>().unwrap());
-        });
-    }
-
-    #[test]
-    #[should_panic]
-    fn test_get_item_invalid() {
-        Python::with_gil(|py| {
-            let list = PyList::new(py, &[2, 3, 5, 7]);
-            list.get_item(-1);
+            assert_eq!(2, list.get_item(0).unwrap().extract::<i32>().unwrap());
+            assert_eq!(3, list.get_item(1).unwrap().extract::<i32>().unwrap());
+            assert_eq!(5, list.get_item(2).unwrap().extract::<i32>().unwrap());
+            assert_eq!(7, list.get_item(3).unwrap().extract::<i32>().unwrap());
         });
     }
 
@@ -289,9 +299,9 @@ mod tests {
             let list = PyList::new(py, &[2, 3, 5, 7]);
             let val = 42i32.to_object(py);
             let val2 = 42i32.to_object(py);
-            assert_eq!(2, list.get_item(0).extract::<i32>().unwrap());
+            assert_eq!(2, list.get_item(0).unwrap().extract::<i32>().unwrap());
             list.set_item(0, val).unwrap();
-            assert_eq!(42, list.get_item(0).extract::<i32>().unwrap());
+            assert_eq!(42, list.get_item(0).unwrap().extract::<i32>().unwrap());
             assert!(list.set_item(10, val2).is_err());
         });
     }
@@ -321,13 +331,13 @@ mod tests {
             let val = 42i32.to_object(py);
             let val2 = 43i32.to_object(py);
             assert_eq!(4, list.len());
-            assert_eq!(2, list.get_item(0).extract::<i32>().unwrap());
+            assert_eq!(2, list.get_item(0).unwrap().extract::<i32>().unwrap());
             list.insert(0, val).unwrap();
             list.insert(1000, val2).unwrap();
             assert_eq!(6, list.len());
-            assert_eq!(42, list.get_item(0).extract::<i32>().unwrap());
-            assert_eq!(2, list.get_item(1).extract::<i32>().unwrap());
-            assert_eq!(43, list.get_item(5).extract::<i32>().unwrap());
+            assert_eq!(42, list.get_item(0).unwrap().extract::<i32>().unwrap());
+            assert_eq!(2, list.get_item(1).unwrap().extract::<i32>().unwrap());
+            assert_eq!(43, list.get_item(5).unwrap().extract::<i32>().unwrap());
         });
     }
 
@@ -352,8 +362,8 @@ mod tests {
         Python::with_gil(|py| {
             let list = PyList::new(py, &[2]);
             list.append(3).unwrap();
-            assert_eq!(2, list.get_item(0).extract::<i32>().unwrap());
-            assert_eq!(3, list.get_item(1).extract::<i32>().unwrap());
+            assert_eq!(2, list.get_item(0).unwrap().extract::<i32>().unwrap());
+            assert_eq!(3, list.get_item(1).unwrap().extract::<i32>().unwrap());
         });
     }
 
@@ -430,15 +440,15 @@ mod tests {
         Python::with_gil(|py| {
             let v = vec![7, 3, 2, 5];
             let list = PyList::new(py, &v);
-            assert_eq!(7, list.get_item(0).extract::<i32>().unwrap());
-            assert_eq!(3, list.get_item(1).extract::<i32>().unwrap());
-            assert_eq!(2, list.get_item(2).extract::<i32>().unwrap());
-            assert_eq!(5, list.get_item(3).extract::<i32>().unwrap());
+            assert_eq!(7, list.get_item(0).unwrap().extract::<i32>().unwrap());
+            assert_eq!(3, list.get_item(1).unwrap().extract::<i32>().unwrap());
+            assert_eq!(2, list.get_item(2).unwrap().extract::<i32>().unwrap());
+            assert_eq!(5, list.get_item(3).unwrap().extract::<i32>().unwrap());
             list.sort().unwrap();
-            assert_eq!(2, list.get_item(0).extract::<i32>().unwrap());
-            assert_eq!(3, list.get_item(1).extract::<i32>().unwrap());
-            assert_eq!(5, list.get_item(2).extract::<i32>().unwrap());
-            assert_eq!(7, list.get_item(3).extract::<i32>().unwrap());
+            assert_eq!(2, list.get_item(0).unwrap().extract::<i32>().unwrap());
+            assert_eq!(3, list.get_item(1).unwrap().extract::<i32>().unwrap());
+            assert_eq!(5, list.get_item(2).unwrap().extract::<i32>().unwrap());
+            assert_eq!(7, list.get_item(3).unwrap().extract::<i32>().unwrap());
         });
     }
 
@@ -447,15 +457,15 @@ mod tests {
         Python::with_gil(|py| {
             let v = vec![2, 3, 5, 7];
             let list = PyList::new(py, &v);
-            assert_eq!(2, list.get_item(0).extract::<i32>().unwrap());
-            assert_eq!(3, list.get_item(1).extract::<i32>().unwrap());
-            assert_eq!(5, list.get_item(2).extract::<i32>().unwrap());
-            assert_eq!(7, list.get_item(3).extract::<i32>().unwrap());
+            assert_eq!(2, list.get_item(0).unwrap().extract::<i32>().unwrap());
+            assert_eq!(3, list.get_item(1).unwrap().extract::<i32>().unwrap());
+            assert_eq!(5, list.get_item(2).unwrap().extract::<i32>().unwrap());
+            assert_eq!(7, list.get_item(3).unwrap().extract::<i32>().unwrap());
             list.reverse().unwrap();
-            assert_eq!(7, list.get_item(0).extract::<i32>().unwrap());
-            assert_eq!(5, list.get_item(1).extract::<i32>().unwrap());
-            assert_eq!(3, list.get_item(2).extract::<i32>().unwrap());
-            assert_eq!(2, list.get_item(3).extract::<i32>().unwrap());
+            assert_eq!(7, list.get_item(0).unwrap().extract::<i32>().unwrap());
+            assert_eq!(5, list.get_item(1).unwrap().extract::<i32>().unwrap());
+            assert_eq!(3, list.get_item(2).unwrap().extract::<i32>().unwrap());
+            assert_eq!(2, list.get_item(3).unwrap().extract::<i32>().unwrap());
         });
     }
 
@@ -464,8 +474,40 @@ mod tests {
         Python::with_gil(|py| {
             let array: PyObject = [1, 2].into_py(py);
             let list = <PyList as PyTryFrom>::try_from(array.as_ref(py)).unwrap();
-            assert_eq!(1, list.get_item(0).extract::<i32>().unwrap());
-            assert_eq!(2, list.get_item(1).extract::<i32>().unwrap());
+            assert_eq!(1, list.get_item(0).unwrap().extract::<i32>().unwrap());
+            assert_eq!(2, list.get_item(1).unwrap().extract::<i32>().unwrap());
+        });
+    }
+
+    #[test]
+    fn test_list_get_item_invalid_index() {
+        Python::with_gil(|py| {
+            let list = PyList::new(py, &[2, 3, 5, 7]);
+            let obj = list.get_item(5);
+            assert!(obj.is_err());
+            assert_eq!(
+                obj.unwrap_err().to_string(),
+                "IndexError: list index out of range"
+            );
+        });
+    }
+
+    #[test]
+    fn test_list_get_item_sanity() {
+        Python::with_gil(|py| {
+            let list = PyList::new(py, &[2, 3, 5, 7]);
+            let obj = list.get_item(0);
+            assert_eq!(obj.unwrap().extract::<i32>().unwrap(), 2);
+        });
+    }
+
+    #[cfg(not(any(Py_LIMITED_API, PyPy)))]
+    #[test]
+    fn test_list_get_item_unchecked_sanity() {
+        Python::with_gil(|py| {
+            let list = PyList::new(py, &[2, 3, 5, 7]);
+            let obj = unsafe { list.get_item_unchecked(0) };
+            assert_eq!(obj.extract::<i32>().unwrap(), 2);
         });
     }
 }
diff --git a/src/types/sequence.rs b/src/types/sequence.rs
@@ -99,9 +99,9 @@ impl PySequence {
 
     /// Returns the `index`th element of the Sequence.
     ///
-    /// This is equivalent to the Python expression `self[index]`.
+    /// This is equivalent to the Python expression `self[index]` without support of negative indices.
     #[inline]
-    pub fn get_item(&self, index: isize) -> PyResult<&PyAny> {
+    pub fn get_item(&self, index: usize) -> PyResult<&PyAny> {
         unsafe {
             self.py()
                 .from_owned_ptr_or_err(ffi::PySequence_GetItem(self.as_ptr(), index as Py_ssize_t))
@@ -403,11 +403,6 @@ mod tests {
             assert_eq!(3, seq.get_item(3).unwrap().extract::<i32>().unwrap());
             assert_eq!(5, seq.get_item(4).unwrap().extract::<i32>().unwrap());
             assert_eq!(8, seq.get_item(5).unwrap().extract::<i32>().unwrap());
-            assert_eq!(8, seq.get_item(-1).unwrap().extract::<i32>().unwrap());
-            assert_eq!(5, seq.get_item(-2).unwrap().extract::<i32>().unwrap());
-            assert_eq!(3, seq.get_item(-3).unwrap().extract::<i32>().unwrap());
-            assert_eq!(2, seq.get_item(-4).unwrap().extract::<i32>().unwrap());
-            assert_eq!(1, seq.get_item(-5).unwrap().extract::<i32>().unwrap());
             assert!(seq.get_item(10).is_err());
         });
     }
diff --git a/src/types/tuple.rs b/src/types/tuple.rs

Original file line number	Diff line number	Diff line change
`@@ -60,7 +60,7 @@ mod min_const_generics {`
`60`	`60`	`if seq_len != N {`
`61`	`61`	`return Err(invalid_sequence_length(N, seq_len));`
`62`	`62`	`}`
`63`		`- array_try_from_fn(\|idx\| seq.get_item(idx as isize).and_then(PyAny::extract))`
	`63`	`+ array_try_from_fn(\|idx\| seq.get_item(idx).and_then(PyAny::extract))`
`64`	`64`	`}`
`65`	`65`
`66`	`66`	`// TODO use std::array::try_from_fn, if that stabilises:`