From 2979fb8268080dffe1396480a9d858a68863946a Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Tue, 17 Aug 2021 08:51:08 +0200 Subject: [PATCH 1/5] tests: fix new clippy warning (does not like Foo::foo) --- tests/test_class_attributes.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/test_class_attributes.rs b/tests/test_class_attributes.rs index 91212754736..ca6b77d9772 100644 --- a/tests/test_class_attributes.rs +++ b/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::(); let bar_obj = py.get_type::(); - 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"); } From b55d62dd3e57929afe9f979698e08e67404c6950 Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Tue, 17 Aug 2021 08:54:31 +0200 Subject: [PATCH 2/5] PySequence: use usize everywhere See #1667 --- src/types/sequence.rs | 48 +++++++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/src/types/sequence.rs b/src/types/sequence.rs index 0b70da0ee40..402d8801cfa 100644 --- a/src/types/sequence.rs +++ b/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 { + pub fn len(&self) -> PyResult { 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,15 +52,14 @@ 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::(ffi::PySequence_Repeat( self.as_ptr(), - count as Py_ssize_t, + get_ssize_index(count), ))?; Ok(&*(ptr as *const PyAny as *const PySequence)) } @@ -84,11 +83,10 @@ impl PySequence { /// Repeats the sequence object `count` times and updates `self`. /// /// This is equivalent to the Python statement `self *= count`. - /// NB: Python accepts negative counts; it empties the Sequence. #[inline] - pub fn in_place_repeat(&self, count: isize) -> PyResult<()> { + pub fn in_place_repeat(&self, count: usize) -> PyResult<()> { unsafe { - let ptr = ffi::PySequence_InPlaceRepeat(self.as_ptr(), count as Py_ssize_t); + let ptr = ffi::PySequence_InPlaceRepeat(self.as_ptr(), get_ssize_index(count)); if ptr.is_null() { Err(PyErr::api_call_failed(self.py())) } else { @@ -103,8 +101,10 @@ 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), + )) } } @@ -112,12 +112,12 @@ impl PySequence { /// /// 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,7 +126,7 @@ impl PySequence { /// /// This is equivalent to the Python statement `self[i] = v`. #[inline] - pub fn set_item(&self, i: isize, item: I) -> PyResult<()> + pub fn set_item(&self, i: usize, item: I) -> PyResult<()> where I: ToBorrowedObject, { @@ -134,7 +134,7 @@ impl PySequence { 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 +144,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 +157,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 +175,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)), ) } } From 925fef1ecea4a20440735b1eb36f3107ab5eea08 Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Wed, 18 Aug 2021 08:40:10 +0200 Subject: [PATCH 3/5] PySequence: fix the in_place methods to a) not leak a reference b) correctly return the result, since for immutable types `self` is not actually mutated in place --- src/types/sequence.rs | 46 ++++++++++++++++++++++++++----------------- 1 file changed, 28 insertions(+), 18 deletions(-) diff --git a/src/types/sequence.rs b/src/types/sequence.rs index 402d8801cfa..80d744f75ad 100644 --- a/src/types/sequence.rs +++ b/src/types/sequence.rs @@ -65,33 +65,43 @@ impl 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::(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 statement `self *= count`. + /// This is equivalent to the Python expression `self.__imul__(other)`. + /// + /// 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: usize) -> PyResult<()> { + pub fn in_place_repeat(&self, count: usize) -> PyResult<&PySequence> { unsafe { - let ptr = ffi::PySequence_InPlaceRepeat(self.as_ptr(), get_ssize_index(count)); - if ptr.is_null() { - Err(PyErr::api_call_failed(self.py())) - } else { - Ok(()) - } + let ptr = self + .py() + .from_owned_ptr_or_err::(ffi::PySequence_InPlaceRepeat( + self.as_ptr(), + get_ssize_index(count), + ))?; + Ok(&*(ptr as *const PyAny as *const PySequence)) } } From 8083c1c310765c3c1a40e18ad5f1c7c1e666799a Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Wed, 18 Aug 2021 08:43:32 +0200 Subject: [PATCH 4/5] Update changelog for #1802 and #1803. --- CHANGELOG.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b3dc9d25519..8eaf1b841e2 100644 --- a/CHANGELOG.md +++ b/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`. [#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 From ead86bd666bc576187a8d6526d24c6f608abcb0a Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Mon, 23 Aug 2021 16:03:35 +0200 Subject: [PATCH 5/5] PySequence: add missing tests --- src/types/sequence.rs | 98 ++++++++++++++++++++++++++++++++++--------- 1 file changed, 79 insertions(+), 19 deletions(-) diff --git a/src/types/sequence.rs b/src/types/sequence.rs index 80d744f75ad..58ffbe854b2 100644 --- a/src/types/sequence.rs +++ b/src/types/sequence.rs @@ -340,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) @@ -382,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::(py).unwrap(); + assert!(!seq.is_empty().unwrap()); + let vec: Vec = Vec::new(); + let empty_list = vec.to_object(py); + let empty_seq = empty_list.cast_as::(py).unwrap(); + assert!(empty_seq.is_empty().unwrap()); + }); + } + #[test] fn test_seq_contains() { Python::with_gil(|py| { @@ -417,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| { @@ -474,6 +483,54 @@ mod tests { }); } + #[test] + fn test_seq_get_slice() { + Python::with_gil(|py| { + let v: Vec = vec![1, 1, 2, 3, 5, 8]; + let ob = v.to_object(py); + let seq = ob.cast_as::(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 = vec![1, 1, 2, 3, 5, 8]; + let w: Vec = vec![7, 4]; + let ob = v.to_object(py); + let seq = ob.cast_as::(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 = vec![1, 1, 2, 3, 5, 8]; + let ob = v.to_object(py); + let seq = ob.cast_as::(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| { @@ -580,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::(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| { @@ -663,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::(py).unwrap(); - assert!(!seq.is_empty().unwrap()); - let vec: Vec = Vec::new(); - let empty_list = vec.to_object(py); - let empty_seq = empty_list.cast_as::(py).unwrap(); - assert!(empty_seq.is_empty().unwrap()); - }); - } }