From ec79285fe454f14415eae7c30b2fb221e667e5dd Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Wed, 8 Jan 2020 00:05:03 +0000 Subject: [PATCH 1/6] Implement IntoIterator for PySet and PyFrozenSet --- CHANGELOG.md | 4 ++++ src/types/set.rs | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index bd8c315ba0a..e0b3814e6ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. * The blanket implementations for `FromPyObject` for `&T` and `&mut T` are no longer specializable. Implement `PyTryFrom` for your type to control the behavior of `FromPyObject::extract()` for your types. * The implementation for `IntoPy for T` where `U: FromPy` is no longer specializable. Control the behavior of this via the implementation of `FromPy`. +### Added + +* Implemented `IntoIterator` for `PySet` and `PyFrozenSet`. [#716](https://github.com/PyO3/pyo3/pull/716) + ## [0.8.5] * Support for `#[name = "foo"]` attribute for `#[pyfunction]` and in `#[pymethods]`. [#692](https://github.com/PyO3/pyo3/pull/692) diff --git a/src/types/set.rs b/src/types/set.rs index c62ef734e32..e340af9a74e 100644 --- a/src/types/set.rs +++ b/src/types/set.rs @@ -6,6 +6,8 @@ use crate::ffi; use crate::instance::PyNativeType; use crate::internal_tricks::Unsendable; use crate::object::PyObject; +use crate::objectprotocol::ObjectProtocol; +use crate::types::{PyAny, PyIterator}; use crate::AsPyPointer; use crate::Python; use crate::{ToBorrowedObject, ToPyObject}; @@ -96,6 +98,15 @@ impl PySet { } } +impl<'a> std::iter::IntoIterator for &'a PySet { + type Item = PyResult<&'a PyAny>; + type IntoIter = PyIterator<'a>; + + fn into_iter(self) -> Self::IntoIter { + self.iter().expect("Failed to get set iterator") + } +} + impl ToPyObject for collections::HashSet where T: hash::Hash + Eq + ToPyObject, @@ -168,6 +179,15 @@ impl PyFrozenSet { } } +impl<'a> std::iter::IntoIterator for &'a PyFrozenSet { + type Item = PyResult<&'a PyAny>; + type IntoIter = PyIterator<'a>; + + fn into_iter(self) -> Self::IntoIter { + self.iter().expect("Failed to get frozen set iterator") + } +} + #[cfg(test)] mod test { use super::{PyFrozenSet, PySet}; @@ -267,9 +287,15 @@ mod test { let py = gil.python(); let set = PySet::new(py, &[1]).unwrap(); + // objectprotocol iteration for el in set.iter().unwrap() { assert_eq!(1i32, el.unwrap().extract::().unwrap()); } + + // intoiterator iteration + for el in set { + assert_eq!(1i32, el.unwrap().extract().unwrap()); + } } #[test] @@ -306,8 +332,15 @@ mod test { let py = gil.python(); let set = PyFrozenSet::new(py, &[1]).unwrap(); + + // objectprotocol iteration for el in set.iter().unwrap() { assert_eq!(1i32, el.unwrap().extract::().unwrap()); } + + // intoiterator iteration + for el in set { + assert_eq!(1i32, el.unwrap().extract::().unwrap()); + } } } From f7a4fbaa3838fa1170a5c8556ed8657ddad3afbb Mon Sep 17 00:00:00 2001 From: David Hewitt <1939362+davidhewitt@users.noreply.github.com> Date: Wed, 8 Jan 2020 09:43:03 +0000 Subject: [PATCH 2/6] Use _PySet_NextEntry --- src/ffi/setobject.rs | 9 ++++++++ src/types/set.rs | 52 ++++++++++++++++++++++++++++++++++---------- 2 files changed, 50 insertions(+), 11 deletions(-) diff --git a/src/ffi/setobject.rs b/src/ffi/setobject.rs index d1e4dbd703e..605e1a9d68d 100644 --- a/src/ffi/setobject.rs +++ b/src/ffi/setobject.rs @@ -60,4 +60,13 @@ extern "C" { pub fn PySet_Add(set: *mut PyObject, key: *mut PyObject) -> c_int; #[cfg_attr(PyPy, link_name = "PyPySet_Pop")] pub fn PySet_Pop(set: *mut PyObject) -> *mut PyObject; + + #[cfg(not(Py_LIMITED_API))] + #[cfg_attr(PyPy, link_name = "_PySet_NextEntry")] + pub fn _PySet_NextEntry( + set: *mut PyObject, + pos: *mut Py_ssize_t, + key: *mut *mut PyObject, + hash: *mut super::Py_hash_t, + ) -> c_int; } diff --git a/src/types/set.rs b/src/types/set.rs index e340af9a74e..bb75b9e557f 100644 --- a/src/types/set.rs +++ b/src/types/set.rs @@ -6,8 +6,7 @@ use crate::ffi; use crate::instance::PyNativeType; use crate::internal_tricks::Unsendable; use crate::object::PyObject; -use crate::objectprotocol::ObjectProtocol; -use crate::types::{PyAny, PyIterator}; +use crate::types::PyAny; use crate::AsPyPointer; use crate::Python; use crate::{ToBorrowedObject, ToPyObject}; @@ -98,12 +97,40 @@ impl PySet { } } +#[cfg(not(Py_LIMITED_API))] +pub struct PySetIterator<'py> { + set: &'py super::PyAny, + pos: isize, +} + +#[cfg(not(Py_LIMITED_API))] +impl<'py> Iterator for PySetIterator<'py> { + type Item = &'py super::PyAny; + + #[inline] + fn next(&mut self) -> Option { + unsafe { + let mut key: *mut ffi::PyObject = std::ptr::null_mut(); + let mut hash: ffi::Py_hash_t = 0; + if ffi::_PySet_NextEntry(self.set.as_ptr(), &mut self.pos, &mut key, &mut hash) != 0 { + Some(self.set.py().from_borrowed_ptr(key)) + } else { + None + } + } + } +} + +#[cfg(not(Py_LIMITED_API))] impl<'a> std::iter::IntoIterator for &'a PySet { - type Item = PyResult<&'a PyAny>; - type IntoIter = PyIterator<'a>; + type Item = &'a PyAny; + type IntoIter = PySetIterator<'a>; fn into_iter(self) -> Self::IntoIter { - self.iter().expect("Failed to get set iterator") + PySetIterator { + set: self.as_ref(), + pos: 0, + } } } @@ -178,13 +205,16 @@ impl PyFrozenSet { }) } } - +#[cfg(not(Py_LIMITED_API))] impl<'a> std::iter::IntoIterator for &'a PyFrozenSet { - type Item = PyResult<&'a PyAny>; - type IntoIter = PyIterator<'a>; + type Item = &'a PyAny; + type IntoIter = PySetIterator<'a>; fn into_iter(self) -> Self::IntoIter { - self.iter().expect("Failed to get frozen set iterator") + PySetIterator { + set: self.as_ref(), + pos: 0, + } } } @@ -294,7 +324,7 @@ mod test { // intoiterator iteration for el in set { - assert_eq!(1i32, el.unwrap().extract().unwrap()); + assert_eq!(1i32, el.extract().unwrap()); } } @@ -340,7 +370,7 @@ mod test { // intoiterator iteration for el in set { - assert_eq!(1i32, el.unwrap().extract::().unwrap()); + assert_eq!(1i32, el.extract::().unwrap()); } } } From 026caeda684148191e65a4f0d8017193e9eb7a98 Mon Sep 17 00:00:00 2001 From: kngwyu Date: Wed, 8 Jan 2020 23:15:53 +0900 Subject: [PATCH 3/6] Implement iter for PySet and PyFrozenSet --- src/types/dict.rs | 6 +++--- src/types/set.rs | 34 +++++++++++++++++++++++++--------- 2 files changed, 28 insertions(+), 12 deletions(-) diff --git a/src/types/dict.rs b/src/types/dict.rs index 287334cfa42..418819d203d 100644 --- a/src/types/dict.rs +++ b/src/types/dict.rs @@ -150,9 +150,9 @@ impl PyDict { } } - /// Returns a iterator of (key, value) pairs in this dictionary - /// Note that it's unsafe to use when the dictionary might be changed - /// by other python code. + /// Returns a iterator of (key, value) pairs in this dictionary. + /// + /// Note that it's unsafe to use when the dictionary might be changed by other code. pub fn iter(&self) -> PyDictIterator { let py = self.py(); PyDictIterator { diff --git a/src/types/set.rs b/src/types/set.rs index bb75b9e557f..e1257b8b806 100644 --- a/src/types/set.rs +++ b/src/types/set.rs @@ -95,6 +95,14 @@ impl PySet { pub fn pop(&self) -> Option { unsafe { PyObject::from_owned_ptr_or_opt(self.py(), ffi::PySet_Pop(self.as_ptr())) } } + + /// Returns an iterator of values in this set. + /// + /// Note that it can be unsafe to use when the set might be changed by other code. + #[cfg(not(Py_LIMITED_API))] + pub fn iter(&self) -> PySetIterator { + self.into_iter() + } } #[cfg(not(Py_LIMITED_API))] @@ -204,7 +212,16 @@ impl PyFrozenSet { } }) } + + /// Returns an iterator of values in this frozen set. + /// + /// Note that it can be unsafe to use when the set might be changed by other code. + #[cfg(not(Py_LIMITED_API))] + pub fn iter(&self) -> PySetIterator { + self.into_iter() + } } + #[cfg(not(Py_LIMITED_API))] impl<'a> std::iter::IntoIterator for &'a PyFrozenSet { type Item = &'a PyAny; @@ -222,9 +239,7 @@ impl<'a> std::iter::IntoIterator for &'a PyFrozenSet { mod test { use super::{PyFrozenSet, PySet}; use crate::instance::AsPyRef; - use crate::objectprotocol::ObjectProtocol; - use crate::Python; - use crate::{PyTryFrom, ToPyObject}; + use crate::{ObjectProtocol, PyTryFrom, Python, ToPyObject}; use std::collections::HashSet; #[test] @@ -317,9 +332,10 @@ mod test { let py = gil.python(); let set = PySet::new(py, &[1]).unwrap(); - // objectprotocol iteration - for el in set.iter().unwrap() { - assert_eq!(1i32, el.unwrap().extract::().unwrap()); + + // iter method + for el in set.iter() { + assert_eq!(1i32, el.extract().unwrap()); } // intoiterator iteration @@ -363,9 +379,9 @@ mod test { let set = PyFrozenSet::new(py, &[1]).unwrap(); - // objectprotocol iteration - for el in set.iter().unwrap() { - assert_eq!(1i32, el.unwrap().extract::().unwrap()); + // iter method + for el in set.iter() { + assert_eq!(1i32, el.extract::().unwrap()); } // intoiterator iteration From 7b502821ce5f58b471781de09a8a1ae1a0635e52 Mon Sep 17 00:00:00 2001 From: kngwyu Date: Wed, 8 Jan 2020 23:43:26 +0900 Subject: [PATCH 4/6] Use &PyAny instead of PyObject in PyDictIterator --- src/types/dict.rs | 9 +++------ src/types/set.rs | 10 +++++----- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/src/types/dict.rs b/src/types/dict.rs index 418819d203d..74bbd0c412f 100644 --- a/src/types/dict.rs +++ b/src/types/dict.rs @@ -154,19 +154,16 @@ impl PyDict { /// /// Note that it's unsafe to use when the dictionary might be changed by other code. pub fn iter(&self) -> PyDictIterator { - let py = self.py(); PyDictIterator { - dict: self.to_object(py), + dict: self.as_ref(), pos: 0, - py, } } } pub struct PyDictIterator<'py> { - dict: PyObject, + dict: &'py PyAny, pos: isize, - py: Python<'py>, } impl<'py> Iterator for PyDictIterator<'py> { @@ -178,7 +175,7 @@ impl<'py> Iterator for PyDictIterator<'py> { let mut key: *mut ffi::PyObject = std::ptr::null_mut(); let mut value: *mut ffi::PyObject = std::ptr::null_mut(); if ffi::PyDict_Next(self.dict.as_ptr(), &mut self.pos, &mut key, &mut value) != 0 { - let py = self.py; + let py = self.dict.py(); Some((py.from_borrowed_ptr(key), py.from_borrowed_ptr(value))) } else { None diff --git a/src/types/set.rs b/src/types/set.rs index e1257b8b806..23c43f0be57 100644 --- a/src/types/set.rs +++ b/src/types/set.rs @@ -101,7 +101,10 @@ impl PySet { /// Note that it can be unsafe to use when the set might be changed by other code. #[cfg(not(Py_LIMITED_API))] pub fn iter(&self) -> PySetIterator { - self.into_iter() + PySetIterator { + set: self.as_ref(), + pos: 0, + } } } @@ -135,10 +138,7 @@ impl<'a> std::iter::IntoIterator for &'a PySet { type IntoIter = PySetIterator<'a>; fn into_iter(self) -> Self::IntoIter { - PySetIterator { - set: self.as_ref(), - pos: 0, - } + self.iter() } } From ea9824a982de1fa709b20b789f0a13e0b72a0674 Mon Sep 17 00:00:00 2001 From: kngwyu Date: Wed, 8 Jan 2020 23:44:16 +0900 Subject: [PATCH 5/6] Fix document for PyList::iter --- src/types/list.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/types/list.rs b/src/types/list.rs index 9392482d2cd..484ba5d0ae4 100644 --- a/src/types/list.rs +++ b/src/types/list.rs @@ -113,7 +113,7 @@ impl PyList { }) } - /// Returns an iterator over the tuple items. + /// Returns an iterator over this list items. pub fn iter(&self) -> PyListIterator { PyListIterator { list: self, From 1f675dcaa75d3457b86f0a83a1981ec12c3b3e42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Niederb=C3=BChl?= Date: Fri, 10 Jan 2020 23:27:10 +0100 Subject: [PATCH 6/6] Clear error indicator when the exception is handled on the Rust side Leaving Python's global exception state is misleading, e.g. subsequent calls of `py.eval` will fail. --- CHANGELOG.md | 4 ++++ src/types/iterator.rs | 1 + src/types/set.rs | 10 +++++++++- tests/test_various.rs | 3 +++ 4 files changed, 17 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e0b3814e6ba..4ef6eff35de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. * Implemented `IntoIterator` for `PySet` and `PyFrozenSet`. [#716](https://github.com/PyO3/pyo3/pull/716) +### Fixed + +* Clear error indicator when the exception is handled on the Rust side. [#719](https://github.com/PyO3/pyo3/pull/719) + ## [0.8.5] * Support for `#[name = "foo"]` attribute for `#[pyfunction]` and in `#[pymethods]`. [#692](https://github.com/PyO3/pyo3/pull/692) diff --git a/src/types/iterator.rs b/src/types/iterator.rs index 0d7ab4aba75..52378caa4b4 100644 --- a/src/types/iterator.rs +++ b/src/types/iterator.rs @@ -42,6 +42,7 @@ impl<'p> PyIterator<'p> { let ptr = ffi::PyObject_GetIter(obj.as_ptr()); // Returns NULL if an object cannot be iterated. if ptr.is_null() { + PyErr::fetch(py); return Err(PyDowncastError); } diff --git a/src/types/set.rs b/src/types/set.rs index 23c43f0be57..f565af6ffa4 100644 --- a/src/types/set.rs +++ b/src/types/set.rs @@ -93,7 +93,12 @@ impl PySet { /// Remove and return an arbitrary element from the set pub fn pop(&self) -> Option { - unsafe { PyObject::from_owned_ptr_or_opt(self.py(), ffi::PySet_Pop(self.as_ptr())) } + let element = + unsafe { PyObject::from_owned_ptr_or_err(self.py(), ffi::PySet_Pop(self.as_ptr())) }; + match element { + Ok(e) => Some(e), + Err(_) => None, + } } /// Returns an iterator of values in this set. @@ -324,6 +329,9 @@ mod test { assert!(val.is_some()); let val2 = set.pop(); assert!(val2.is_none()); + assert!(py + .eval("print('Exception state should not be set.')", None, None) + .is_ok()); } #[test] diff --git a/tests/test_various.rs b/tests/test_various.rs index 655101db2ad..563166bd7cd 100644 --- a/tests/test_various.rs +++ b/tests/test_various.rs @@ -179,4 +179,7 @@ fn incorrect_iter() { let int_ref = int.as_ref(py); // Should not segfault. assert!(int_ref.iter().is_err()); + assert!(py + .eval("print('Exception state should not be set.')", None, None) + .is_ok()); }