PySequence: use usize everywhere, fix in-place methods

CHANGELOG.md

@@ -16,8 +16,12 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 ### 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)
+- `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)
 - Deprecate `PyTuple::split_from`. [#1804](https://github.com/PyO3/pyo3/pull/1804)
 
 ## [0.14.3] - 2021-08-22

src/types/sequence.rs

@@ -1,7 +1,8 @@
 // Copyright (c) 2017-present PyO3 Project and Contributors
 
 use crate::err::{self, PyDowncastError, PyErr, PyResult};
-use crate::ffi::{self, Py_ssize_t};
+use crate::ffi;
+use crate::internal_tricks::get_ssize_index;
 use crate::types::{PyAny, PyList, PyTuple};
 use crate::AsPyPointer;
 use crate::{FromPyObject, PyTryFrom, ToBorrowedObject};
@@ -17,13 +18,12 @@ impl PySequence {
     ///
     /// This is equivalent to the Python expression `len(self)`.
     #[inline]
-    #[allow(clippy::len_without_is_empty)]
-    pub fn len(&self) -> PyResult<isize> {
+    pub fn len(&self) -> PyResult<usize> {
         let v = unsafe { ffi::PySequence_Size(self.as_ptr()) };
         if v == -1 {
             Err(PyErr::api_call_failed(self.py()))
         } else {
-            Ok(v as isize)
+            Ok(v as usize)
         }
     }
 
@@ -52,48 +52,56 @@ impl PySequence {
     /// Returns the result of repeating a sequence object `count` times.
     ///
     /// This is equivalent to the Python expression `self * count`.
-    /// NB: Python accepts negative counts; it returns an empty Sequence.
     #[inline]
-    pub fn repeat(&self, count: isize) -> PyResult<&PySequence> {
+    pub fn repeat(&self, count: usize) -> PyResult<&PySequence> {
         unsafe {
             let ptr = self
                 .py()
                 .from_owned_ptr_or_err::<PyAny>(ffi::PySequence_Repeat(
                     self.as_ptr(),
-                    count as Py_ssize_t,
+                    get_ssize_index(count),
                 ))?;
             Ok(&*(ptr as *const PyAny as *const PySequence))
         }
     }
 
-    /// Concatenates `self` and `other` in place.
+    /// Concatenates `self` and `other`, in place if possible.
     ///
-    /// This is equivalent to the Python statement `self += other`.
+    /// This is equivalent to the Python expression `self.__iadd__(other)`.
+    ///
+    /// The Python statement `self += other` is syntactic sugar for `self =
+    /// self.__iadd__(other)`.  `__iadd__` should modify and return `self` if
+    /// possible, but create and return a new object if not.
     #[inline]
-    pub fn in_place_concat(&self, other: &PySequence) -> PyResult<()> {
+    pub fn in_place_concat(&self, other: &PySequence) -> PyResult<&PySequence> {
         unsafe {
-            let ptr = ffi::PySequence_InPlaceConcat(self.as_ptr(), other.as_ptr());
-            if ptr.is_null() {
-                Err(PyErr::api_call_failed(self.py()))
-            } else {
-                Ok(())
-            }
+            let ptr = self
+                .py()
+                .from_owned_ptr_or_err::<PyAny>(ffi::PySequence_InPlaceConcat(
+                    self.as_ptr(),
+                    other.as_ptr(),
+                ))?;
+            Ok(&*(ptr as *const PyAny as *const PySequence))
         }
     }
 
-    /// Repeats the sequence object `count` times and updates `self`.
+    /// Repeats the sequence object `count` times and updates `self`, if possible.
+    ///
+    /// This is equivalent to the Python expression `self.__imul__(other)`.
     ///
-    /// This is equivalent to the Python statement `self *= count`.
-    /// NB: Python accepts negative counts; it empties the Sequence.
+    /// The Python statement `self *= other` is syntactic sugar for `self =
+    /// self.__imul__(other)`.  `__imul__` should modify and return `self` if
+    /// possible, but create and return a new object if not.
     #[inline]
-    pub fn in_place_repeat(&self, count: isize) -> PyResult<()> {
+    pub fn in_place_repeat(&self, count: usize) -> PyResult<&PySequence> {
         unsafe {
-            let ptr = ffi::PySequence_InPlaceRepeat(self.as_ptr(), count as Py_ssize_t);
-            if ptr.is_null() {
-                Err(PyErr::api_call_failed(self.py()))
-            } else {
-                Ok(())
-            }
+            let ptr = self
+                .py()
+                .from_owned_ptr_or_err::<PyAny>(ffi::PySequence_InPlaceRepeat(
+                    self.as_ptr(),
+                    get_ssize_index(count),
+                ))?;
+            Ok(&*(ptr as *const PyAny as *const PySequence))
         }
     }
 
@@ -103,21 +111,23 @@ impl PySequence {
     #[inline]
     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))
+            self.py().from_owned_ptr_or_err(ffi::PySequence_GetItem(
+                self.as_ptr(),
+                get_ssize_index(index),
+            ))
         }
     }
 
     /// Returns the slice of sequence object between `begin` and `end`.
     ///
     /// This is equivalent to the Python expression `self[begin:end]`.
     #[inline]
-    pub fn get_slice(&self, begin: isize, end: isize) -> PyResult<&PyAny> {
+    pub fn get_slice(&self, begin: usize, end: usize) -> PyResult<&PyAny> {
         unsafe {
             self.py().from_owned_ptr_or_err(ffi::PySequence_GetSlice(
                 self.as_ptr(),
-                begin as Py_ssize_t,
-                end as Py_ssize_t,
+                get_ssize_index(begin),
+                get_ssize_index(end),
             ))
         }
     }
@@ -126,15 +136,15 @@ impl PySequence {
     ///
     /// This is equivalent to the Python statement `self[i] = v`.
     #[inline]
-    pub fn set_item<I>(&self, i: isize, item: I) -> PyResult<()>
+    pub fn set_item<I>(&self, i: usize, item: I) -> PyResult<()>
     where
         I: ToBorrowedObject,
     {
         unsafe {
             item.with_borrowed_ptr(self.py(), |item| {
                 err::error_on_minusone(
                     self.py(),
-                    ffi::PySequence_SetItem(self.as_ptr(), i as Py_ssize_t, item),
+                    ffi::PySequence_SetItem(self.as_ptr(), get_ssize_index(i), item),
                 )
             })
         }
@@ -144,11 +154,11 @@ impl PySequence {
     ///
     /// This is equivalent to the Python statement `del self[i]`.
     #[inline]
-    pub fn del_item(&self, i: isize) -> PyResult<()> {
+    pub fn del_item(&self, i: usize) -> PyResult<()> {
         unsafe {
             err::error_on_minusone(
                 self.py(),
-                ffi::PySequence_DelItem(self.as_ptr(), i as Py_ssize_t),
+                ffi::PySequence_DelItem(self.as_ptr(), get_ssize_index(i)),
             )
         }
     }
@@ -157,14 +167,14 @@ impl PySequence {
     ///
     /// This is equivalent to the Python statement `self[i1:i2] = v`.
     #[inline]
-    pub fn set_slice(&self, i1: isize, i2: isize, v: &PyAny) -> PyResult<()> {
+    pub fn set_slice(&self, i1: usize, i2: usize, v: &PyAny) -> PyResult<()> {
         unsafe {
             err::error_on_minusone(
                 self.py(),
                 ffi::PySequence_SetSlice(
                     self.as_ptr(),
-                    i1 as Py_ssize_t,
-                    i2 as Py_ssize_t,
+                    get_ssize_index(i1),
+                    get_ssize_index(i2),
                     v.as_ptr(),
                 ),
             )
@@ -175,11 +185,11 @@ impl PySequence {
     ///
     /// This is equivalent to the Python statement `del self[i1:i2]`.
     #[inline]
-    pub fn del_slice(&self, i1: isize, i2: isize) -> PyResult<()> {
+    pub fn del_slice(&self, i1: usize, i2: usize) -> PyResult<()> {
         unsafe {
             err::error_on_minusone(
                 self.py(),
-                ffi::PySequence_DelSlice(self.as_ptr(), i1 as Py_ssize_t, i2 as Py_ssize_t),
+                ffi::PySequence_DelSlice(self.as_ptr(), get_ssize_index(i1), get_ssize_index(i2)),
             )
         }
     }
@@ -330,14 +340,14 @@ impl<'v> PyTryFrom<'v> for PySequence {
 
 #[cfg(test)]
 mod tests {
-    use crate::types::PySequence;
+    use crate::types::{PyList, PySequence};
     use crate::AsPyPointer;
     use crate::Python;
     use crate::{PyObject, PyTryFrom, ToPyObject};
 
     fn get_object() -> PyObject {
         // Convenience function for getting a single unique object
-        Python::with_gil(|py| -> PyObject {
+        Python::with_gil(|py| {
             let obj = py.eval("object()", None, None).unwrap();
 
             obj.to_object(py)
@@ -372,6 +382,19 @@ mod tests {
         });
     }
 
+    #[test]
+    fn test_seq_is_empty() {
+        Python::with_gil(|py| {
+            let list = vec![1].to_object(py);
+            let seq = list.cast_as::<PySequence>(py).unwrap();
+            assert!(!seq.is_empty().unwrap());
+            let vec: Vec<u32> = Vec::new();
+            let empty_list = vec.to_object(py);
+            let empty_seq = empty_list.cast_as::<PySequence>(py).unwrap();
+            assert!(empty_seq.is_empty().unwrap());
+        });
+    }
+
     #[test]
     fn test_seq_contains() {
         Python::with_gil(|py| {
@@ -407,10 +430,6 @@ mod tests {
         });
     }
 
-    // fn test_get_slice() {}
-    // fn test_set_slice() {}
-    // fn test_del_slice() {}
-
     #[test]
     fn test_seq_del_item() {
         Python::with_gil(|py| {
@@ -464,6 +483,54 @@ mod tests {
         });
     }
 
+    #[test]
+    fn test_seq_get_slice() {
+        Python::with_gil(|py| {
+            let v: Vec<i32> = vec![1, 1, 2, 3, 5, 8];
+            let ob = v.to_object(py);
+            let seq = ob.cast_as::<PySequence>(py).unwrap();
+            assert_eq!(
+                [1, 2, 3],
+                seq.get_slice(1, 4).unwrap().extract::<[i32; 3]>().unwrap()
+            );
+            assert_eq!(
+                [3, 5, 8],
+                seq.get_slice(3, 100)
+                    .unwrap()
+                    .extract::<[i32; 3]>()
+                    .unwrap()
+            );
+        });
+    }
+
+    #[test]
+    fn test_set_slice() {
+        Python::with_gil(|py| {
+            let v: Vec<i32> = vec![1, 1, 2, 3, 5, 8];
+            let w: Vec<i32> = vec![7, 4];
+            let ob = v.to_object(py);
+            let seq = ob.cast_as::<PySequence>(py).unwrap();
+            let ins = w.to_object(py);
+            seq.set_slice(1, 4, ins.as_ref(py)).unwrap();
+            assert_eq!([1, 7, 4, 5, 8], seq.extract::<[i32; 5]>().unwrap());
+            seq.set_slice(3, 100, PyList::empty(py)).unwrap();
+            assert_eq!([1, 7, 4], seq.extract::<[i32; 3]>().unwrap());
+        });
+    }
+
+    #[test]
+    fn test_del_slice() {
+        Python::with_gil(|py| {
+            let v: Vec<i32> = vec![1, 1, 2, 3, 5, 8];
+            let ob = v.to_object(py);
+            let seq = ob.cast_as::<PySequence>(py).unwrap();
+            seq.del_slice(1, 4).unwrap();
+            assert_eq!([1, 5, 8], seq.extract::<[i32; 3]>().unwrap());
+            seq.del_slice(1, 100).unwrap();
+            assert_eq!([1], seq.extract::<[i32; 1]>().unwrap());
+        });
+    }
+
     #[test]
     fn test_seq_index() {
         Python::with_gil(|py| {
@@ -570,6 +637,22 @@ mod tests {
         });
     }
 
+    #[test]
+    fn test_seq_inplace() {
+        Python::with_gil(|py| {
+            let v = vec!["foo", "bar"];
+            let ob = v.to_object(py);
+            let seq = ob.cast_as::<PySequence>(py).unwrap();
+            let rep_seq = seq.in_place_repeat(3).unwrap();
+            assert_eq!(6, seq.len().unwrap());
+            assert_eq!(seq, rep_seq);
+
+            let conc_seq = seq.in_place_concat(seq).unwrap();
+            assert_eq!(12, seq.len().unwrap());
+            assert_eq!(seq, conc_seq);
+        });
+    }
+
     #[test]
     fn test_list_coercion() {
         Python::with_gil(|py| {
@@ -653,17 +736,4 @@ mod tests {
             assert!(seq_from.list().is_ok());
         });
     }
-
-    #[test]
-    fn test_is_empty() {
-        Python::with_gil(|py| {
-            let list = vec![1].to_object(py);
-            let seq = list.cast_as::<PySequence>(py).unwrap();
-            assert!(!seq.is_empty().unwrap());
-            let vec: Vec<u32> = Vec::new();
-            let empty_list = vec.to_object(py);
-            let empty_seq = empty_list.cast_as::<PySequence>(py).unwrap();
-            assert!(empty_seq.is_empty().unwrap());
-        });
-    }
 }

tests/test_class_attributes.rs

@@ -40,7 +40,7 @@ impl Foo {
     }
 
     #[classattr]
-    fn foo() -> Foo {
+    fn a_foo() -> Foo {
         Foo { x: 1 }
     }
 }
@@ -54,7 +54,7 @@ fn class_attributes() {
     py_assert!(py, foo_obj, "foo_obj.RENAMED_CONST == 'foobar_2'");
     py_assert!(py, foo_obj, "foo_obj.a == 5");
     py_assert!(py, foo_obj, "foo_obj.B == 'bar'");
-    py_assert!(py, foo_obj, "foo_obj.foo.x == 1");
+    py_assert!(py, foo_obj, "foo_obj.a_foo.x == 1");
 }
 
 // Ignored because heap types are not immutable:
@@ -71,7 +71,7 @@ fn class_attributes_are_immutable() {
 #[pymethods]
 impl Bar {
     #[classattr]
-    fn foo() -> Foo {
+    fn a_foo() -> Foo {
         Foo { x: 3 }
     }
 }
@@ -83,7 +83,7 @@ fn recursive_class_attributes() {
 
     let foo_obj = py.get_type::<Foo>();
     let bar_obj = py.get_type::<Bar>();
-    py_assert!(py, foo_obj, "foo_obj.foo.x == 1");
+    py_assert!(py, foo_obj, "foo_obj.a_foo.x == 1");
     py_assert!(py, foo_obj, "foo_obj.bar.x == 2");
-    py_assert!(py, bar_obj, "bar_obj.foo.x == 3");
+    py_assert!(py, bar_obj, "bar_obj.a_foo.x == 3");
 }