From 3a90ff77f4bfb06fd7f760910e78fc954c25561a Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Tue, 24 Aug 2021 23:06:56 +0100 Subject: [PATCH] list,tuple,sequence: add slice indexing --- CHANGELOG.md | 11 ++-- build.rs | 5 ++ guide/src/migration.md | 12 ++++ src/ffi/abstract_.rs | 6 +- src/internal_tricks.rs | 143 +++++++++++++++++++++++++++++++++++++++++ src/types/list.rs | 67 +++++++++++++++---- src/types/sequence.rs | 86 +++++++++++++++++++++---- src/types/tuple.rs | 75 +++++++++++++++++---- 8 files changed, 358 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c6355bb..a90b465e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - 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) -- Add implementation of `std::ops::Index` for `PyList`, `PyTuple` and `PySequence`. [#1825](https://github.com/PyO3/pyo3/pull/1825) +- Add implementation of `std::ops::Index` for `PyList`, `PyTuple` and `PySequence`. [#1825](https://github.com/PyO3/pyo3/pull/1825) +- Add range indexing implementations of `std::ops::Index` for `PyList`, `PyTuple` and `PySequence`. [#1829](https://github.com/PyO3/pyo3/pull/1829) ### Changed @@ -20,11 +21,11 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - `PyList`, `PyTuple` and `PySequence`'s APIs now accepts only `usize` indices instead of `isize`. [#1733](https://github.com/PyO3/pyo3/pull/1733), [#1802](https://github.com/PyO3/pyo3/pull/1802), [#1803](https://github.com/PyO3/pyo3/pull/1803) -- `PyList` and `PyTuple`'s `get_item` now return `PyResult<&PyAny>` instead of panicking. [#1733](https://github.com/PyO3/pyo3/pull/1733) -- `PySequence`'s `in_place_repeat` and `in_place_concat` now return the result instead of `()`, which is needed - in case of immutable sequences. [#1803](https://github.com/PyO3/pyo3/pull/1803) +- `PyList::get_item` and `PyTuple::get_item` now return `PyResult<&PyAny>` instead of panicking. [#1733](https://github.com/PyO3/pyo3/pull/1733) +- `PySequence::in_place_repeat` and `PySequence::in_place_concat` now return `PyResult<&PySequence>` instead of `PyResult<()>`, which is needed in case of immutable sequences such as tuples. [#1803](https://github.com/PyO3/pyo3/pull/1803) +- `PySequence::get_slice` now returns `PyResult<&PySequence>` instead of `PyResult<&PyAny>`. [#1829](https://github.com/PyO3/pyo3/pull/1829) - Deprecate `PyTuple::split_from`. [#1804](https://github.com/PyO3/pyo3/pull/1804) -- Deprecate `PyTuple::slice`, new method `get_slice` added with proper index types. [#1828](https://github.com/PyO3/pyo3/pull/1828) +- Deprecate `PyTuple::slice`, new method `PyTuple::get_slice` added with `usize` indices. [#1828](https://github.com/PyO3/pyo3/pull/1828) ## [0.14.3] - 2021-08-22 diff --git a/build.rs b/build.rs index 4b510128..4a9f8e25 100644 --- a/build.rs +++ b/build.rs @@ -137,6 +137,11 @@ fn configure_pyo3() -> Result<()> { let rustc_minor_version = rustc_minor_version().unwrap_or(0); + // Enable use of #[track_caller] on Rust 1.46 and greater + if rustc_minor_version >= 46 { + println!("cargo:rustc-cfg=track_caller"); + } + // Enable use of const generics on Rust 1.51 and greater if rustc_minor_version >= 51 { println!("cargo:rustc-cfg=min_const_generics"); diff --git a/guide/src/migration.md b/guide/src/migration.md index de424e4b..0745a0b0 100644 --- a/guide/src/migration.md +++ b/guide/src/migration.md @@ -21,6 +21,18 @@ Note that *slice* indices (accepted by `PySequence::get_slice` and other) still inherit the Python behavior of clamping the indices to the actual length, and not panicking/returning an error on out of range indices. +An additional advantage of using Rust's indexing conventions for these types is +that these types can now also support Rust's indexing operators as part of a +consistent API: + +```rust +use pyo3::{Python, types::PyList}; + +Python::with_gil(|py| { + let list = PyList::new(py, &[1, 2, 3]); + assert_eq!(list[0..2].to_string(), "[1, 2]"); +}); +``` ## from 0.13.* to 0.14 diff --git a/src/ffi/abstract_.rs b/src/ffi/abstract_.rs index e4ee4ead..bb38f8ae 100644 --- a/src/ffi/abstract_.rs +++ b/src/ffi/abstract_.rs @@ -251,9 +251,9 @@ extern "C" { pub fn PySequence_List(o: *mut PyObject) -> *mut PyObject; #[cfg_attr(PyPy, link_name = "PyPySequence_Fast")] pub fn PySequence_Fast(o: *mut PyObject, m: *const c_char) -> *mut PyObject; - // skipped PySequenc_Fast_GET_SIZE - // skipped PySequenc_Fast_GET_ITEM - // skipped PySequenc_Fast_GET_ITEMS + // skipped PySequence_Fast_GET_SIZE + // skipped PySequence_Fast_GET_ITEM + // skipped PySequence_Fast_GET_ITEMS pub fn PySequence_Count(o: *mut PyObject, value: *mut PyObject) -> Py_ssize_t; #[cfg_attr(PyPy, link_name = "PyPySequence_Contains")] pub fn PySequence_Contains(seq: *mut PyObject, ob: *mut PyObject) -> c_int; diff --git a/src/internal_tricks.rs b/src/internal_tricks.rs index 5e72a6dd..4d443ed0 100644 --- a/src/internal_tricks.rs +++ b/src/internal_tricks.rs @@ -58,3 +58,146 @@ pub(crate) fn extract_cstr_or_leak_cstring( pub(crate) fn get_ssize_index(index: usize) -> Py_ssize_t { index.min(PY_SSIZE_T_MAX as usize) as Py_ssize_t } + +/// Implementations used for slice indexing PySequence, PyTuple, and PyList +macro_rules! index_impls { + ( + $ty:ty, + $ty_name:literal, + $len:expr, + $get_slice:expr $(,)? + ) => { + impl std::ops::Index for $ty { + // Always PyAny output (even if the slice operation returns something else) + type Output = PyAny; + + #[cfg_attr(track_caller, track_caller)] + fn index(&self, index: usize) -> &Self::Output { + self.get_item(index).unwrap_or_else(|_| { + crate::internal_tricks::index_len_fail(index, $ty_name, $len(self)) + }) + } + } + + impl std::ops::Index> for $ty { + type Output = $ty; + + #[cfg_attr(track_caller, track_caller)] + fn index( + &self, + std::ops::Range { start, end }: std::ops::Range, + ) -> &Self::Output { + let len = $len(self); + if start > len { + crate::internal_tricks::slice_start_index_len_fail(start, $ty_name, len) + } else if end > len { + crate::internal_tricks::slice_end_index_len_fail(end, $ty_name, len) + } else if start > end { + crate::internal_tricks::slice_index_order_fail(start, end) + } else { + $get_slice(self, start, end) + } + } + } + + impl std::ops::Index> for $ty { + type Output = $ty; + + #[cfg_attr(track_caller, track_caller)] + fn index( + &self, + std::ops::RangeFrom { start }: std::ops::RangeFrom, + ) -> &Self::Output { + let len = $len(self); + if start > len { + crate::internal_tricks::slice_start_index_len_fail(start, $ty_name, len) + } else { + $get_slice(self, start, len) + } + } + } + + impl std::ops::Index for $ty { + type Output = $ty; + + #[cfg_attr(track_caller, track_caller)] + fn index(&self, _: std::ops::RangeFull) -> &Self::Output { + let len = $len(self); + $get_slice(self, 0, len) + } + } + + impl std::ops::Index> for $ty { + type Output = $ty; + + #[cfg_attr(track_caller, track_caller)] + fn index(&self, range: std::ops::RangeInclusive) -> &Self::Output { + let exclusive_end = range + .end() + .checked_add(1) + .expect("range end exceeds Python limit"); + &self[*range.start()..exclusive_end] + } + } + + impl std::ops::Index> for $ty { + type Output = $ty; + + #[cfg_attr(track_caller, track_caller)] + fn index(&self, std::ops::RangeTo { end }: std::ops::RangeTo) -> &Self::Output { + &self[0..end] + } + } + + impl std::ops::Index> for $ty { + type Output = $ty; + + #[cfg_attr(track_caller, track_caller)] + fn index( + &self, + std::ops::RangeToInclusive { end }: std::ops::RangeToInclusive, + ) -> &Self::Output { + &self[0..=end] + } + } + }; +} + +// these error messages are shamelessly "borrowed" from std. + +#[inline(never)] +#[cold] +#[cfg_attr(track_caller, track_caller)] +pub(crate) fn index_len_fail(index: usize, ty_name: &str, len: usize) -> ! { + panic!( + "index {} out of range for {} of length {}", + index, ty_name, len + ); +} + +#[inline(never)] +#[cold] +#[cfg_attr(track_caller, track_caller)] +pub(crate) fn slice_start_index_len_fail(index: usize, ty_name: &str, len: usize) -> ! { + panic!( + "range start index {} out of range for {} of length {}", + index, ty_name, len + ); +} + +#[inline(never)] +#[cold] +#[cfg_attr(track_caller, track_caller)] +pub(crate) fn slice_end_index_len_fail(index: usize, ty_name: &str, len: usize) -> ! { + panic!( + "range end index {} out of range for {} of length {}", + index, ty_name, len + ); +} + +#[inline(never)] +#[cold] +#[cfg_attr(track_caller, track_caller)] +pub(crate) fn slice_index_order_fail(index: usize, end: usize) -> ! { + panic!("slice index starts at {} but ends at {}", index, end); +} diff --git a/src/types/list.rs b/src/types/list.rs index fe9ba300..abcdb63a 100644 --- a/src/types/list.rs +++ b/src/types/list.rs @@ -8,7 +8,6 @@ use crate::internal_tricks::get_ssize_index; use crate::{ AsPyPointer, IntoPy, IntoPyPointer, PyAny, PyObject, Python, ToBorrowedObject, ToPyObject, }; -use std::ops::Index; /// Represents a Python `list`. #[repr(transparent)] @@ -177,19 +176,7 @@ impl PyList { } } -impl Index for PyList { - type Output = PyAny; - - fn index(&self, index: usize) -> &Self::Output { - self.get_item(index).unwrap_or_else(|_| { - panic!( - "index {} out of range for list of length {}", - index, - self.len() - ); - }) - } -} +index_impls!(PyList, "list", PyList::len, PyList::get_slice); /// Used by `PyList::iter()`. pub struct PyListIterator<'a> { @@ -544,4 +531,56 @@ mod tests { let _ = &list[7]; }); } + + #[test] + fn test_list_index_trait_ranges() { + Python::with_gil(|py| { + let list = PyList::new(py, &[2, 3, 5]); + assert_eq!(vec![3, 5], list[1..3].extract::>().unwrap()); + assert_eq!(Vec::::new(), list[3..3].extract::>().unwrap()); + assert_eq!(vec![3, 5], list[1..].extract::>().unwrap()); + assert_eq!(Vec::::new(), list[3..].extract::>().unwrap()); + assert_eq!(vec![2, 3, 5], list[..].extract::>().unwrap()); + assert_eq!(vec![3, 5], list[1..=2].extract::>().unwrap()); + assert_eq!(vec![2, 3], list[..2].extract::>().unwrap()); + assert_eq!(vec![2, 3], list[..=1].extract::>().unwrap()); + }) + } + + #[test] + #[should_panic = "range start index 5 out of range for list of length 3"] + fn test_list_index_trait_range_panic_start() { + Python::with_gil(|py| { + let list = PyList::new(py, &[2, 3, 5]); + list[5..10].extract::>().unwrap(); + }) + } + + #[test] + #[should_panic = "range end index 10 out of range for list of length 3"] + fn test_list_index_trait_range_panic_end() { + Python::with_gil(|py| { + let list = PyList::new(py, &[2, 3, 5]); + list[1..10].extract::>().unwrap(); + }) + } + + #[test] + #[should_panic = "slice index starts at 2 but ends at 1"] + fn test_list_index_trait_range_panic_wrong_order() { + Python::with_gil(|py| { + let list = PyList::new(py, &[2, 3, 5]); + #[allow(clippy::reversed_empty_ranges)] + list[2..1].extract::>().unwrap(); + }) + } + + #[test] + #[should_panic = "range start index 8 out of range for list of length 3"] + fn test_list_index_trait_range_from_panic() { + Python::with_gil(|py| { + let list = PyList::new(py, &[2, 3, 5]); + list[8..].extract::>().unwrap(); + }) + } } diff --git a/src/types/sequence.rs b/src/types/sequence.rs index 4506ed8d..c41ec3c8 100644 --- a/src/types/sequence.rs +++ b/src/types/sequence.rs @@ -6,7 +6,6 @@ use crate::internal_tricks::get_ssize_index; use crate::types::{PyAny, PyList, PyTuple}; use crate::AsPyPointer; use crate::{FromPyObject, PyTryFrom, ToBorrowedObject}; -use std::ops::Index; /// Represents a reference to a Python object supporting the sequence protocol. #[repr(transparent)] @@ -108,7 +107,7 @@ impl PySequence { /// /// This is equivalent to the Python expression `self[begin:end]`. #[inline] - pub fn get_slice(&self, begin: usize, end: usize) -> PyResult<&PyAny> { + pub fn get_slice(&self, begin: usize, end: usize) -> PyResult<&PySequence> { unsafe { self.py().from_owned_ptr_or_err(ffi::PySequence_GetSlice( self.as_ptr(), @@ -254,16 +253,19 @@ impl PySequence { } } -impl Index for PySequence { - type Output = PyAny; - - fn index(&self, index: usize) -> &Self::Output { - self.get_item(index).unwrap_or_else(|_| { - panic!("index {} out of range for sequence", index); - }) - } +#[inline] +fn sequence_len(seq: &PySequence) -> usize { + seq.len().expect("failed to get sequence length") } +#[inline] +fn sequence_slice(seq: &PySequence, start: usize, end: usize) -> &PySequence { + seq.get_slice(start, end) + .expect("sequence slice operation failed") +} + +index_impls!(PySequence, "sequence", sequence_len, sequence_slice); + impl<'a, T> FromPyObject<'a> for Vec where T: FromPyObject<'a>, @@ -439,7 +441,7 @@ mod tests { } #[test] - #[should_panic] + #[should_panic = "index 7 out of range for sequence"] fn test_seq_index_trait_panic() { Python::with_gil(|py| { let v: Vec = vec![1, 1, 2]; @@ -449,6 +451,68 @@ mod tests { }); } + #[test] + fn test_seq_index_trait_ranges() { + Python::with_gil(|py| { + let v: Vec = vec![1, 1, 2]; + let ob = v.to_object(py); + let seq = ob.cast_as::(py).unwrap(); + assert_eq!(vec![1, 2], seq[1..3].extract::>().unwrap()); + assert_eq!(Vec::::new(), seq[3..3].extract::>().unwrap()); + assert_eq!(vec![1, 2], seq[1..].extract::>().unwrap()); + assert_eq!(Vec::::new(), seq[3..].extract::>().unwrap()); + assert_eq!(vec![1, 1, 2], seq[..].extract::>().unwrap()); + assert_eq!(vec![1, 2], seq[1..=2].extract::>().unwrap()); + assert_eq!(vec![1, 1], seq[..2].extract::>().unwrap()); + assert_eq!(vec![1, 1], seq[..=1].extract::>().unwrap()); + }) + } + + #[test] + #[should_panic = "range start index 5 out of range for sequence of length 3"] + fn test_seq_index_trait_range_panic_start() { + Python::with_gil(|py| { + let v: Vec = vec![1, 1, 2]; + let ob = v.to_object(py); + let seq = ob.cast_as::(py).unwrap(); + seq[5..10].extract::>().unwrap(); + }) + } + + #[test] + #[should_panic = "range end index 10 out of range for sequence of length 3"] + fn test_seq_index_trait_range_panic_end() { + Python::with_gil(|py| { + let v: Vec = vec![1, 1, 2]; + let ob = v.to_object(py); + let seq = ob.cast_as::(py).unwrap(); + seq[1..10].extract::>().unwrap(); + }) + } + + #[test] + #[should_panic = "slice index starts at 2 but ends at 1"] + fn test_seq_index_trait_range_panic_wrong_order() { + Python::with_gil(|py| { + let v: Vec = vec![1, 1, 2]; + let ob = v.to_object(py); + let seq = ob.cast_as::(py).unwrap(); + #[allow(clippy::reversed_empty_ranges)] + seq[2..1].extract::>().unwrap(); + }) + } + + #[test] + #[should_panic = "range start index 8 out of range for sequence of length 3"] + fn test_seq_index_trait_range_from_panic() { + Python::with_gil(|py| { + let v: Vec = vec![1, 1, 2]; + let ob = v.to_object(py); + let seq = ob.cast_as::(py).unwrap(); + seq[8..].extract::>().unwrap(); + }) + } + #[test] fn test_seq_del_item() { Python::with_gil(|py| { diff --git a/src/types/tuple.rs b/src/types/tuple.rs index e1ec3777..c844382c 100644 --- a/src/types/tuple.rs +++ b/src/types/tuple.rs @@ -6,7 +6,6 @@ use crate::{ exceptions, AsPyPointer, FromPyObject, IntoPy, IntoPyPointer, Py, PyAny, PyErr, PyObject, PyResult, PyTryFrom, Python, ToPyObject, }; -use std::ops::Index; /// Represents a Python `tuple` object. /// @@ -155,19 +154,7 @@ impl PyTuple { } } -impl Index for PyTuple { - type Output = PyAny; - - fn index(&self, index: usize) -> &Self::Output { - self.get_item(index).unwrap_or_else(|_| { - panic!( - "index {} out of range for tuple of length {}", - index, - self.len() - ); - }) - } -} +index_impls!(PyTuple, "tuple", PyTuple::len, PyTuple::get_slice); /// Used by `PyTuple::iter()`. pub struct PyTupleIterator<'a> { @@ -588,4 +575,64 @@ mod tests { let _ = &tuple[7]; }); } + + #[test] + fn test_tuple_index_trait_ranges() { + Python::with_gil(|py| { + let ob = (1, 2, 3).to_object(py); + let tuple = ::try_from(ob.as_ref(py)).unwrap(); + assert_eq!(vec![2, 3], tuple[1..3].extract::>().unwrap()); + assert_eq!( + Vec::::new(), + tuple[3..3].extract::>().unwrap() + ); + assert_eq!(vec![2, 3], tuple[1..].extract::>().unwrap()); + assert_eq!(Vec::::new(), tuple[3..].extract::>().unwrap()); + assert_eq!(vec![1, 2, 3], tuple[..].extract::>().unwrap()); + assert_eq!(vec![2, 3], tuple[1..=2].extract::>().unwrap()); + assert_eq!(vec![1, 2], tuple[..2].extract::>().unwrap()); + assert_eq!(vec![1, 2], tuple[..=1].extract::>().unwrap()); + }) + } + + #[test] + #[should_panic = "range start index 5 out of range for tuple of length 3"] + fn test_tuple_index_trait_range_panic_start() { + Python::with_gil(|py| { + let ob = (1, 2, 3).to_object(py); + let tuple = ::try_from(ob.as_ref(py)).unwrap(); + tuple[5..10].extract::>().unwrap(); + }) + } + + #[test] + #[should_panic = "range end index 10 out of range for tuple of length 3"] + fn test_tuple_index_trait_range_panic_end() { + Python::with_gil(|py| { + let ob = (1, 2, 3).to_object(py); + let tuple = ::try_from(ob.as_ref(py)).unwrap(); + tuple[1..10].extract::>().unwrap(); + }) + } + + #[test] + #[should_panic = "slice index starts at 2 but ends at 1"] + fn test_tuple_index_trait_range_panic_wrong_order() { + Python::with_gil(|py| { + let ob = (1, 2, 3).to_object(py); + let tuple = ::try_from(ob.as_ref(py)).unwrap(); + #[allow(clippy::reversed_empty_ranges)] + tuple[2..1].extract::>().unwrap(); + }) + } + + #[test] + #[should_panic = "range start index 8 out of range for tuple of length 3"] + fn test_tuple_index_trait_range_from_panic() { + Python::with_gil(|py| { + let ob = (1, 2, 3).to_object(py); + let tuple = ::try_from(ob.as_ref(py)).unwrap(); + tuple[8..].extract::>().unwrap(); + }) + } }