From 947b372dcc776f717e5a9862c87c85609a9e8d0d Mon Sep 17 00:00:00 2001
From: David Hewitt <mail@davidhewitt.dev>
Date: Fri, 19 Apr 2024 20:35:52 +0100
Subject: [PATCH] change `PyAnyMethods::dir` to be fallible (#4100)

---
 newsfragments/4100.changed.md |  1 +
 src/types/any.rs              | 34 +++++++++++++++++++++++++++++-----
 2 files changed, 30 insertions(+), 5 deletions(-)
 create mode 100644 newsfragments/4100.changed.md

diff --git a/newsfragments/4100.changed.md b/newsfragments/4100.changed.md
new file mode 100644
index 00000000..13fd2e02
--- /dev/null
+++ b/newsfragments/4100.changed.md
@@ -0,0 +1 @@
+Change `PyAnyMethods::dir` to be fallible and return `PyResult<Bound<'py, PyList>>` (and similar for `PyAny::dir`).
diff --git a/src/types/any.rs b/src/types/any.rs
index a5ea4c80..6ba34c86 100644
--- a/src/types/any.rs
+++ b/src/types/any.rs
@@ -845,8 +845,8 @@ impl PyAny {
     /// Returns the list of attributes of this object.
     ///
     /// This is equivalent to the Python expression `dir(self)`.
-    pub fn dir(&self) -> &PyList {
-        self.as_borrowed().dir().into_gil_ref()
+    pub fn dir(&self) -> PyResult<&PyList> {
+        self.as_borrowed().dir().map(Bound::into_gil_ref)
     }
 
     /// Checks whether this object is an instance of type `ty`.
@@ -1674,7 +1674,7 @@ pub trait PyAnyMethods<'py>: crate::sealed::Sealed {
     /// Returns the list of attributes of this object.
     ///
     /// This is equivalent to the Python expression `dir(self)`.
-    fn dir(&self) -> Bound<'py, PyList>;
+    fn dir(&self) -> PyResult<Bound<'py, PyList>>;
 
     /// Checks whether this object is an instance of type `ty`.
     ///
@@ -2220,10 +2220,10 @@ impl<'py> PyAnyMethods<'py> for Bound<'py, PyAny> {
         Ok(v as usize)
     }
 
-    fn dir(&self) -> Bound<'py, PyList> {
+    fn dir(&self) -> PyResult<Bound<'py, PyList>> {
         unsafe {
             ffi::PyObject_Dir(self.as_ptr())
-                .assume_owned(self.py())
+                .assume_owned_or_err(self.py())
                 .downcast_into_unchecked()
         }
     }
@@ -2471,6 +2471,7 @@ class SimpleClass:
                 .unwrap();
             let a = obj
                 .dir()
+                .unwrap()
                 .into_iter()
                 .map(|x| x.extract::<String>().unwrap());
             let b = dir.into_iter().map(|x| x.extract::<String>().unwrap());
@@ -2745,4 +2746,27 @@ class SimpleClass:
             assert!(not_container.is_empty().is_err());
         });
     }
+
+    #[cfg(feature = "macros")]
+    #[test]
+    #[allow(unknown_lints, non_local_definitions)]
+    fn test_fallible_dir() {
+        use crate::exceptions::PyValueError;
+        use crate::prelude::*;
+
+        #[pyclass(crate = "crate")]
+        struct DirFail;
+
+        #[pymethods(crate = "crate")]
+        impl DirFail {
+            fn __dir__(&self) -> PyResult<PyObject> {
+                Err(PyValueError::new_err("uh-oh!"))
+            }
+        }
+
+        Python::with_gil(|py| {
+            let obj = Bound::new(py, DirFail).unwrap();
+            assert!(obj.dir().unwrap_err().is_instance_of::<PyValueError>(py));
+        })
+    }
 }