Merge pull request #1051 from davidhewitt/pyiterator-from-object

Change PyIterator::from_object` to return underlying TypeError

CHANGELOG.md

@@ -8,13 +8,15 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 ### Added
 - Add FFI definitions `Py_FinalizeEx`, `PyOS_getsig`, `PyOS_setsig`. [#1021](https://github.com/PyO3/pyo3/pull/1021)
 - Add `Python::with_gil` for executing a closure with the Python GIL. [#1037](https://github.com/PyO3/pyo3/pull/1037)
+- Implement `Debug` for `PyIterator`. [#1051](https://github.com/PyO3/pyo3/pull/1051)
 
 ### Changed
 - Exception types have been renamed from e.g. `RuntimeError` to `PyRuntimeError`, and are now only accessible by `&T` or `Py<T>` similar to other Python-native types. The old names continue to exist but are deprecated. [#1024](https://github.com/PyO3/pyo3/pull/1024)
 - Correct FFI definitions `Py_SetProgramName` and `Py_SetPythonHome` to take `*const` argument instead of `*mut`. [#1021](https://github.com/PyO3/pyo3/pull/1021)
 - Rename `PyString::to_string` to `to_str`, change return type `Cow<str>` to `&str`. [#1023](https://github.com/PyO3/pyo3/pull/1023)
 - Correct FFI definition `_PyLong_AsByteArray` `*mut c_uchar` argument instead of `*const c_uchar`. [#1029](https://github.com/PyO3/pyo3/pull/1029)
 - `PyType::as_type_ptr` is no longer `unsafe`. [#1047](https://github.com/PyO3/pyo3/pull/1047)
+- Change `PyIterator::from_object` to return `PyResult<PyIterator>` instead of `Result<PyIterator, PyDowncastError>`. [#1051](https://github.com/PyO3/pyo3/pull/1051)
 
 ### Removed
 - Remove `PyString::as_bytes`. [#1023](https://github.com/PyO3/pyo3/pull/1023)

src/types/any.rs

@@ -363,7 +363,7 @@ impl PyAny {
     /// This is typically a new iterator but if the argument is an iterator,
     /// this returns itself.
     pub fn iter(&self) -> PyResult<PyIterator> {
-        Ok(PyIterator::from_object(self.py(), self)?)
+        PyIterator::from_object(self.py(), self)
     }
 
     /// Returns the Python type object for this object's type.

src/types/iterator.rs

@@ -2,7 +2,7 @@
 //
 // based on Daniel Grunwald's https://github.com/dgrunwald/rust-cpython
 
-use crate::{ffi, AsPyPointer, PyAny, PyDowncastError, PyErr, PyNativeType, PyResult, Python};
+use crate::{ffi, AsPyPointer, PyAny, PyErr, PyNativeType, PyResult, Python};
 
 /// A Python iterator object.
 ///
@@ -25,34 +25,26 @@ use crate::{ffi, AsPyPointer, PyAny, PyDowncastError, PyErr, PyNativeType, PyRes
 /// # Ok(())
 /// # }
 /// ```
+#[derive(Debug)]
 pub struct PyIterator<'p>(&'p PyAny);
 
 impl<'p> PyIterator<'p> {
     /// Constructs a `PyIterator` from a Python iterator object.
-    pub fn from_object<T>(py: Python<'p>, obj: &T) -> Result<PyIterator<'p>, PyDowncastError>
+    pub fn from_object<T>(py: Python<'p>, obj: &T) -> PyResult<PyIterator<'p>>
     where
         T: AsPyPointer,
     {
-        unsafe {
-            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);
-            }
-
-            if ffi::PyIter_Check(ptr) != 0 {
-                // This looks suspicious, but is actually correct. Even though ptr is an owned
-                // reference, PyIterator takes ownership of the reference and decreases the count
-                // in its Drop implementation.
-                //
-                // Therefore we must use from_borrowed_ptr instead of from_owned_ptr so that the
-                // GILPool does not take ownership of the reference.
-                Ok(PyIterator(py.from_borrowed_ptr(ptr)))
-            } else {
-                Err(PyDowncastError)
-            }
-        }
+        let iter = unsafe {
+            // This looks suspicious, but is actually correct. Even though ptr is an owned
+            // reference, PyIterator takes ownership of the reference and decreases the count
+            // in its Drop implementation.
+            //
+            // Therefore we must use from_borrowed_ptr_or_err instead of from_owned_ptr_or_err so
+            // that the GILPool does not take ownership of the reference.
+            py.from_borrowed_ptr_or_err(ffi::PyObject_GetIter(obj.as_ptr()))?
+        };
+
+        Ok(PyIterator(iter))
     }
 }
 
@@ -90,6 +82,8 @@ impl<'p> Drop for PyIterator<'p> {
 
 #[cfg(test)]
 mod tests {
+    use super::PyIterator;
+    use crate::exceptions::PyTypeError;
     use crate::gil::GILPool;
     use crate::instance::AsPyRef;
     use crate::types::{PyDict, PyList};
@@ -186,4 +180,15 @@ mod tests {
             assert_eq!(actual, *expected)
         }
     }
+
+    #[test]
+    fn int_not_iterable() {
+        let gil = GILGuard::acquire();
+        let py = gil.python();
+
+        let x = 5.to_object(py);
+        let err = PyIterator::from_object(py, &x).unwrap_err();
+
+        assert!(err.is_instance::<PyTypeError>(py))
+    }
 }