diff --git a/newsfragments/2914.changed.md b/newsfragments/2914.changed.md new file mode 100644 index 00000000..bea926c4 --- /dev/null +++ b/newsfragments/2914.changed.md @@ -0,0 +1 @@ +FFI definition `PyIter_Check` on CPython 3.7 now does the equivalent for `hasattr(type(obj), "__next__")`, which works correctly on all platforms and adds support for `abi3`. diff --git a/newsfragments/2914.fixed.md b/newsfragments/2914.fixed.md new file mode 100644 index 00000000..9ea78ee8 --- /dev/null +++ b/newsfragments/2914.fixed.md @@ -0,0 +1 @@ +Fix downcast to `PyIterator` succeeding for Python classes which did not implement `__next__`. diff --git a/pyo3-ffi/src/abstract_.rs b/pyo3-ffi/src/abstract_.rs index b8306ca9..24ad4005 100644 --- a/pyo3-ffi/src/abstract_.rs +++ b/pyo3-ffi/src/abstract_.rs @@ -91,21 +91,22 @@ extern "C" { pub fn PyObject_GetIter(arg1: *mut PyObject) -> *mut PyObject; } -// Defined as this macro in Python limited API, but relies on -// non-limited PyTypeObject. Don't expose this since it cannot be used. -#[cfg(not(any(Py_LIMITED_API, PyPy)))] +// Before 3.8 PyIter_Check was defined in CPython as a macro, +// but the implementation of that in PyO3 did not work, see +// https://github.com/PyO3/pyo3/pull/2914 +// +// This is a slow implementation which should function equivalently. +#[cfg(not(any(Py_3_8, PyPy)))] #[inline] pub unsafe fn PyIter_Check(o: *mut PyObject) -> c_int { - (match (*crate::Py_TYPE(o)).tp_iternext { - Some(tp_iternext) => { - tp_iternext as *const std::os::raw::c_void != crate::_PyObject_NextNotImplemented as _ - } - None => false, - }) as c_int + crate::PyObject_HasAttrString( + crate::Py_TYPE(o).cast(), + "__next__\0".as_ptr() as *const c_char, + ) } extern "C" { - #[cfg(any(all(Py_3_8, Py_LIMITED_API), PyPy))] + #[cfg(any(Py_3_8, PyPy))] #[cfg_attr(PyPy, link_name = "PyPyIter_Check")] pub fn PyIter_Check(obj: *mut PyObject) -> c_int; diff --git a/src/types/iterator.rs b/src/types/iterator.rs index 5b51c067..cfbbd31b 100644 --- a/src/types/iterator.rs +++ b/src/types/iterator.rs @@ -3,7 +3,6 @@ // based on Daniel Grunwald's https://github.com/dgrunwald/rust-cpython use crate::{ffi, AsPyPointer, IntoPyPointer, Py, PyAny, PyErr, PyNativeType, PyResult, Python}; -#[cfg(any(not(Py_LIMITED_API), Py_3_8))] use crate::{PyDowncastError, PyTryFrom}; /// A Python iterator object. @@ -29,7 +28,6 @@ use crate::{PyDowncastError, PyTryFrom}; #[repr(transparent)] pub struct PyIterator(PyAny); pyobject_native_type_named!(PyIterator); -#[cfg(any(not(Py_LIMITED_API), Py_3_8))] pyobject_native_type_extract!(PyIterator); impl PyIterator { @@ -64,7 +62,6 @@ impl<'p> Iterator for &'p PyIterator { } // PyIter_Check does not exist in the limited API until 3.8 -#[cfg(any(not(Py_LIMITED_API), Py_3_8))] impl<'v> PyTryFrom<'v> for PyIterator { fn try_from>(value: V) -> Result<&'v PyIterator, PyDowncastError<'v>> { let value = value.into(); @@ -218,7 +215,7 @@ def fibonacci(target): } #[test] - #[cfg(any(not(Py_LIMITED_API), Py_3_8))] + fn iterator_try_from() { Python::with_gil(|py| { let obj: Py = vec![10, 20].to_object(py).as_ref(py).iter().unwrap().into(); @@ -248,4 +245,76 @@ def fibonacci(target): assert_eq!(iter_ref.get_refcnt(), 2); }) } + + #[test] + #[cfg(feature = "macros")] + fn python_class_not_iterator() { + use crate::PyErr; + + #[crate::pyclass(crate = "crate")] + struct Downcaster { + failed: Option, + } + + #[crate::pymethods(crate = "crate")] + impl Downcaster { + fn downcast_iterator(&mut self, obj: &PyAny) { + self.failed = Some(obj.downcast::().unwrap_err().into()); + } + } + + // Regression test for 2913 + Python::with_gil(|py| { + let downcaster = Py::new(py, Downcaster { failed: None }).unwrap(); + crate::py_run!( + py, + downcaster, + r#" + from collections.abc import Sequence + + class MySequence(Sequence): + def __init__(self): + self._data = [1, 2, 3] + + def __getitem__(self, index): + return self._data[index] + + def __len__(self): + return len(self._data) + + downcaster.downcast_iterator(MySequence()) + "# + ); + + assert_eq!( + downcaster.borrow_mut(py).failed.take().unwrap().to_string(), + "TypeError: 'MySequence' object cannot be converted to 'Iterator'" + ); + }); + } + + #[test] + #[cfg(feature = "macros")] + fn python_class_iterator() { + #[crate::pyfunction(crate = "crate")] + fn assert_iterator(obj: &PyAny) { + assert!(obj.downcast::().is_ok()) + } + + // Regression test for 2913 + Python::with_gil(|py| { + let assert_iterator = crate::wrap_pyfunction!(assert_iterator, py).unwrap(); + crate::py_run!( + py, + assert_iterator, + r#" + class MyIter: + def __next__(self): + raise StopIteration + + assert_iterator(MyIter()) + "# + ); + }); + } }