improve coverage of PyAnyMethods

Author: davidhewitt

src/instance.rs

@@ -67,54 +67,42 @@ impl<'py> Py2<'py, PyAny> {
 }
 
 impl<'py, T> Py2<'py, T> {
-    /// Helper to cast to Py2<'py, T>
+    /// Helper to cast to Py2<'py, PyAny>
     pub(crate) fn as_any(&self) -> &Py2<'py, PyAny> {
+        // Safety: all Py2<T> have the same memory layout, and all Py2<T> are valid Py2<PyAny>
         unsafe { std::mem::transmute(self) }
     }
 }
 
-impl<'py, T> std::fmt::Debug for Py2<'py, T>
-where
-    Self: Deref<Target = Py2<'py, PyAny>>,
-{
+impl<'py, T> std::fmt::Debug for Py2<'py, T> {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> {
-        let s = self.repr().or(Err(std::fmt::Error))?;
-        f.write_str(&s.as_gil_ref().to_string_lossy())
+        let any = self.as_any();
+        python_format(any, any.repr(), f)
     }
 }
 
-impl<'py, T> std::fmt::Display for Py2<'py, T>
-where
-    Self: Deref<Target = Py2<'py, PyAny>>,
-{
+impl<'py, T> std::fmt::Display for Py2<'py, T> {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> {
-        match self.str() {
-            Result::Ok(s) => return f.write_str(&s.as_gil_ref().to_string_lossy()),
-            Result::Err(err) => {
-                err.write_unraisable(self.py(), std::option::Option::Some(self.as_gil_ref()))
-            }
-        }
-
-        match self.get_type().name() {
-            Result::Ok(name) => std::write!(f, "<unprintable {} object>", name),
-            Result::Err(_err) => f.write_str("<unprintable object>"),
-        }
+        let any = self.as_any();
+        python_format(any, any.str(), f)
     }
 }
 
-impl<'py> std::fmt::Display for Py2<'py, PyAny> {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> {
-        match self.str() {
-            Result::Ok(s) => return f.write_str(&s.as_gil_ref().to_string_lossy()),
-            Result::Err(err) => {
-                err.write_unraisable(self.py(), std::option::Option::Some(self.as_gil_ref()))
-            }
+fn python_format(
+    any: &Py2<'_, PyAny>,
+    format_result: PyResult<Py2<'_, PyString>>,
+    f: &mut std::fmt::Formatter<'_>,
+) -> Result<(), std::fmt::Error> {
+    match format_result {
+        Result::Ok(s) => return f.write_str(&s.as_gil_ref().to_string_lossy()),
+        Result::Err(err) => {
+            err.write_unraisable(any.py(), std::option::Option::Some(any.as_gil_ref()))
         }
+    }
 
-        match self.get_type().name() {
-            Result::Ok(name) => std::write!(f, "<unprintable {} object>", name),
-            Result::Err(_err) => f.write_str("<unprintable object>"),
-        }
+    match any.get_type().name() {
+        Result::Ok(name) => std::write!(f, "<unprintable {} object>", name),
+        Result::Err(_err) => f.write_str("<unprintable object>"),
     }
 }
 
@@ -126,14 +114,16 @@ where
 
     #[inline]
     fn deref(&self) -> &Py2<'py, PyAny> {
-        ::std::convert::AsRef::as_ref(self)
+        self.as_any()
     }
 }
 
-impl<'py, T> AsRef<Py2<'py, PyAny>> for Py2<'py, T> {
+impl<'py, T> AsRef<Py2<'py, PyAny>> for Py2<'py, T>
+where
+    T: AsRef<PyAny>,
+{
     fn as_ref(&self) -> &Py2<'py, PyAny> {
-        // Safety: &Py2<T> has the same layout as &Py2<PyAny>
-        unsafe { std::mem::transmute(self) }
+        self.as_any()
     }
 }
 
@@ -176,9 +166,7 @@ impl<'py, T> Py2<'py, T> {
     /// of the pointer or decrease the reference count (e.g. with [`pyo3::ffi::Py_DecRef`](crate::ffi::Py_DecRef)).
     #[inline]
     pub fn into_ptr(self) -> *mut ffi::PyObject {
-        let ptr = (self.1).0.as_ptr();
-        std::mem::forget(self);
-        ptr
+        self.into_non_null().as_ptr()
     }
 
     /// Internal helper to convert e.g. &'a &'py PyDict to &'a Py2<'py, PyDict> for
@@ -211,10 +199,12 @@ impl<'py, T> Py2<'py, T> {
         unsafe { self.py().from_owned_ptr(self.into_ptr()) }
     }
 
+    // Internal helper to convert `self` into a `NonNull` which owns the
+    // Python reference.
     pub(crate) fn into_non_null(self) -> NonNull<ffi::PyObject> {
-        let ptr = (self.1).0;
-        std::mem::forget(self);
-        ptr
+        // wrap in ManuallyDrop to avoid running Drop for self and decreasing
+        // the reference count
+        ManuallyDrop::new(self).1 .0
     }
 }
 
@@ -1477,6 +1467,22 @@ a = A()
         });
     }
 
+    #[test]
+    fn test_debug_fmt() {
+        Python::with_gil(|py| {
+            let obj = "hello world".to_object(py).attach_into(py);
+            assert_eq!(format!("{:?}", obj), "'hello world'");
+        });
+    }
+
+    #[test]
+    fn test_display_fmt() {
+        Python::with_gil(|py| {
+            let obj = "hello world".to_object(py).attach_into(py);
+            assert_eq!(format!("{}", obj), "hello world");
+        });
+    }
+
     #[cfg(feature = "macros")]
     mod using_macros {
         use super::*;

src/types/any.rs

@@ -702,7 +702,13 @@ impl PyAny {
     /// This is typically a new iterator but if the argument is an iterator,
     /// this returns itself.
     pub fn iter(&self) -> PyResult<&PyIterator> {
-        PyIterator::from_object(self)
+        Py2::<PyAny>::borrowed_from_gil_ref(&self)
+            .iter()
+            .map(|py2| {
+                // Can't use into_gil_ref here because T: PyTypeInfo bound is not satisfied
+                // Safety: into_ptr produces a valid pointer to PyIterator object
+                unsafe { self.py().from_owned_ptr(py2.into_ptr()) }
+            })
     }
 
     /// Returns the Python type object for this object's type.
@@ -1804,42 +1810,48 @@ impl<'py> PyAnyMethods<'py> for Py2<'py, PyAny> {
     where
         O: ToPyObject,
     {
-        self.rich_compare(other, CompareOp::Lt)?.is_true()
+        self.rich_compare(other, CompareOp::Lt)
+            .and_then(|any| any.is_true())
     }
 
     fn le<O>(&self, other: O) -> PyResult<bool>
     where
         O: ToPyObject,
     {
-        self.rich_compare(other, CompareOp::Le)?.is_true()
+        self.rich_compare(other, CompareOp::Le)
+            .and_then(|any| any.is_true())
     }
 
     fn eq<O>(&self, other: O) -> PyResult<bool>
     where
         O: ToPyObject,
     {
-        self.rich_compare(other, CompareOp::Eq)?.is_true()
+        self.rich_compare(other, CompareOp::Eq)
+            .and_then(|any| any.is_true())
     }
 
     fn ne<O>(&self, other: O) -> PyResult<bool>
     where
         O: ToPyObject,
     {
-        self.rich_compare(other, CompareOp::Ne)?.is_true()
+        self.rich_compare(other, CompareOp::Ne)
+            .and_then(|any| any.is_true())
     }
 
     fn gt<O>(&self, other: O) -> PyResult<bool>
     where
         O: ToPyObject,
     {
-        self.rich_compare(other, CompareOp::Gt)?.is_true()
+        self.rich_compare(other, CompareOp::Gt)
+            .and_then(|any| any.is_true())
     }
 
     fn ge<O>(&self, other: O) -> PyResult<bool>
     where
         O: ToPyObject,
     {
-        self.rich_compare(other, CompareOp::Ge)?.is_true()
+        self.rich_compare(other, CompareOp::Ge)
+            .and_then(|any| any.is_true())
     }
 
     fn is_callable(&self) -> bool {
@@ -2169,7 +2181,7 @@ impl<'py> PyAnyMethods<'py> for Py2<'py, PyAny> {
 mod tests {
     use crate::{
         types::{IntoPyDict, PyAny, PyBool, PyList, PyLong, PyModule},
-        Python, ToPyObject,
+        PyTypeInfo, Python, ToPyObject,
     };
 
     #[test]
@@ -2542,4 +2554,28 @@ class SimpleClass:
             assert!(!not_ellipsis.is_ellipsis());
         });
     }
+
+    #[test]
+    fn test_is_callable() {
+        Python::with_gil(|py| {
+            assert!(PyList::type_object(py).is_callable());
+
+            let not_callable = 5.to_object(py).into_ref(py);
+            assert!(!not_callable.is_callable());
+        });
+    }
+
+    #[test]
+    fn test_is_empty() {
+        Python::with_gil(|py| {
+            let empty_list: &PyAny = PyList::empty(py);
+            assert!(empty_list.is_empty().unwrap());
+
+            let list: &PyAny = PyList::new(py, vec![1, 2, 3]);
+            assert!(!list.is_empty().unwrap());
+
+            let not_container = 5.to_object(py).into_ref(py);
+            assert!(not_container.is_empty().is_err());
+        });
+    }
 }

src/types/iterator.rs

@@ -33,10 +33,11 @@ impl PyIterator {
     ///
     /// Equivalent to Python's built-in `iter` function.
     pub fn from_object(obj: &PyAny) -> PyResult<&PyIterator> {
-        unsafe {
-            obj.py()
-                .from_owned_ptr_or_err(ffi::PyObject_GetIter(obj.as_ptr()))
-        }
+        Self::from_object2(Py2::borrowed_from_gil_ref(&obj)).map(|py2| {
+            // Can't use into_gil_ref here because T: PyTypeInfo bound is not satisfied
+            // Safety: into_ptr produces a valid pointer to PyIterator object
+            unsafe { obj.py().from_owned_ptr(py2.into_ptr()) }
+        })
     }
 
     pub(crate) fn from_object2<'py>(obj: &Py2<'py, PyAny>) -> PyResult<Py2<'py, PyIterator>> {

src/types/pysuper.rs

@@ -1,7 +1,7 @@
-use crate::ffi;
 use crate::instance::Py2;
 use crate::types::any::PyAnyMethods;
 use crate::types::PyType;
+use crate::{ffi, PyTypeInfo};
 use crate::{PyAny, PyResult};
 
 /// Represents a Python `super` object.
@@ -57,20 +57,22 @@ impl PySuper {
     /// }
     /// ```
     pub fn new<'py>(ty: &'py PyType, obj: &'py PyAny) -> PyResult<&'py PySuper> {
-        let py = ty.py();
-        let super_ = py.get_type::<PySuper>().call1((ty, obj))?;
-        let super_ = super_.downcast::<PySuper>()?;
-        Ok(super_)
+        Self::new2(
+            Py2::borrowed_from_gil_ref(&ty),
+            Py2::borrowed_from_gil_ref(&obj),
+        )
+        .map(Py2::into_gil_ref)
     }
 
     pub(crate) fn new2<'py>(
         ty: &Py2<'py, PyType>,
         obj: &Py2<'py, PyAny>,
     ) -> PyResult<Py2<'py, PySuper>> {
-        let py = ty.py();
-        let super_ =
-            Py2::<PyType>::borrowed_from_gil_ref(&py.get_type::<PySuper>()).call1((ty, obj))?;
-        let super_ = super_.downcast_into::<PySuper>()?;
-        Ok(super_)
+        Py2::<PyType>::borrowed_from_gil_ref(&PySuper::type_object(ty.py()))
+            .call1((ty, obj))
+            .map(|any| {
+                // Safety: super() always returns instance of super
+                unsafe { any.downcast_into_unchecked() }
+            })
     }
 }