Move locking the GIL and handling the resulting panics into a library function.

pyo3-macros-backend/src/pymethod.rs

@@ -401,35 +401,13 @@ fn impl_call_slot(cls: &syn::Type, mut spec: FnSpec<'_>) -> Result<MethodAndSlot
 }
 
 fn impl_traverse_slot(cls: &syn::Type, rust_fn_ident: &syn::Ident) -> MethodAndSlotDef {
-    // It is important the implementation of `__traverse__` cannot safely access the GIL,
-    // c.f. https://github.com/PyO3/pyo3/issues/3165, and hence we do not expose our GIL
-    // token to the user code and lock safe methods for acquiring the GIL.
-    // Since we do not create a `GILPool` at all, it is important that our usage of the GIL
-    // token does not produce any owned objects thereby calling into `register_owned`.
     let associated_method = quote! {
         pub unsafe extern "C" fn __pymethod_traverse__(
             slf: *mut _pyo3::ffi::PyObject,
             visit: _pyo3::ffi::visitproc,
             arg: *mut ::std::os::raw::c_void,
-        ) -> ::std::os::raw::c_int
-        {
-            let trap = _pyo3::impl_::panic::PanicTrap::new("uncaught panic inside __traverse__ handler");
-            let py = _pyo3::Python::assume_gil_acquired();
-            let slf = py.from_borrowed_ptr::<_pyo3::PyCell<#cls>>(slf);
-            let borrow = slf.try_borrow();
-            let visit = _pyo3::class::gc::PyVisit::from_raw(visit, arg, py);
-
-            let retval = if let ::std::result::Result::Ok(borrow) = borrow {
-                let _lock = _pyo3::impl_::LockGIL::during_traverse();
-                match ::std::panic::catch_unwind(::std::panic::AssertUnwindSafe(move || borrow.#rust_fn_ident(visit))) {
-                    ::std::result::Result::Ok(res) => _pyo3::impl_::pymethods::unwrap_traverse_result(res),
-                    ::std::result::Result::Err(_err) => -1,
-                }
-            } else {
-                0
-            };
-            trap.disarm();
-            retval
+        ) -> ::std::os::raw::c_int {
+            _pyo3::impl_::pymethods::call_traverse_impl::<#cls>(slf, #cls::#rust_fn_ident, visit, arg)
         }
     };
     let slot_def = quote! {

src/gil.rs

@@ -317,7 +317,7 @@ impl Drop for SuspendGIL {
 }
 
 /// Used to lock safe access to the GIL
-pub struct LockGIL {
+pub(crate) struct LockGIL {
     count: isize,
 }
 

src/impl_.rs

@@ -19,5 +19,3 @@ pub mod pymethods;
 pub mod pymodule;
 #[doc(hidden)]
 pub mod trampoline;
-
-pub use crate::gil::LockGIL;

src/impl_/pymethods.rs

@@ -1,9 +1,15 @@
+use crate::gil::LockGIL;
+use crate::impl_::panic::PanicTrap;
 use crate::internal_tricks::extract_c_string;
-use crate::{ffi, IntoPy, Py, PyAny, PyErr, PyObject, PyResult, PyTraverseError, Python};
+use crate::{
+    ffi, IntoPy, Py, PyAny, PyCell, PyClass, PyErr, PyObject, PyResult, PyTraverseError, PyVisit,
+    Python,
+};
 use std::borrow::Cow;
 use std::ffi::CStr;
 use std::fmt;
-use std::os::raw::c_int;
+use std::os::raw::{c_int, c_void};
+use std::panic::{catch_unwind, AssertUnwindSafe};
 
 /// Python 3.8 and up - __ipow__ has modulo argument correctly populated.
 #[cfg(Py_3_8)]
@@ -239,14 +245,46 @@ impl PySetterDef {
     }
 }
 
-/// Unwraps the result of __traverse__ for tp_traverse
+/// Calls an implementation of __traverse__ for tp_traverse
 #[doc(hidden)]
-#[inline]
-pub fn unwrap_traverse_result(result: Result<(), PyTraverseError>) -> c_int {
-    match result {
-        Ok(()) => 0,
-        Err(PyTraverseError(value)) => value,
-    }
+pub unsafe fn call_traverse_impl<T>(
+    slf: *mut ffi::PyObject,
+    impl_: fn(&T, PyVisit<'_>) -> Result<(), PyTraverseError>,
+    visit: ffi::visitproc,
+    arg: *mut c_void,
+) -> c_int
+where
+    T: PyClass,
+{
+    // It is important the implementation of `__traverse__` cannot safely access the GIL,
+    // c.f. https://github.com/PyO3/pyo3/issues/3165, and hence we do not expose our GIL
+    // token to the user code and lock safe methods for acquiring the GIL.
+    // (This includes enforcing the `&self` method receiver as e.g. `PyRef<Self>` could
+    // reconstruct a GIL token via `PyRef::py`.)
+    // Since we do not create a `GILPool` at all, it is important that our usage of the GIL
+    // token does not produce any owned objects thereby calling into `register_owned`.
+    let trap = PanicTrap::new("uncaught panic inside __traverse__ handler");
+
+    let py = Python::assume_gil_acquired();
+    let slf = py.from_borrowed_ptr::<PyCell<T>>(slf);
+    let borrow = slf.try_borrow();
+    let visit = PyVisit::from_raw(visit, arg, py);
+
+    let retval = if let Ok(borrow) = borrow {
+        let _lock = LockGIL::during_traverse();
+
+        match catch_unwind(AssertUnwindSafe(move || impl_(&*borrow, visit))) {
+            Ok(res) => match res {
+                Ok(()) => 0,
+                Err(PyTraverseError(value)) => value,
+            },
+            Err(_err) => -1,
+        }
+    } else {
+        0
+    };
+    trap.disarm();
+    retval
 }
 
 pub(crate) struct PyMethodDefDestructor {

tests/test_compile_error.rs

@@ -45,6 +45,7 @@ fn _test_compile_errors() {
     t.compile_fail("tests/ui/invalid_pymethod_names.rs");
     t.compile_fail("tests/ui/invalid_pymodule_args.rs");
     t.compile_fail("tests/ui/reject_generics.rs");
+    t.compile_fail("tests/ui/traverse_bare_self.rs");
 
     tests_rust_1_49(&t);
     tests_rust_1_56(&t);

tests/test_gc.rs

@@ -339,8 +339,7 @@ struct TriesGILInTraverse {}
 #[pymethods]
 impl TriesGILInTraverse {
     fn __traverse__(&self, _visit: PyVisit<'_>) -> Result<(), PyTraverseError> {
-        Python::with_gil(|_py| {});
-        Ok(())
+        Python::with_gil(|_py| Ok(()))
     }
 }
 

tests/ui/traverse_bare_self.rs

@@ -0,0 +1,12 @@
+use pyo3::prelude::*;
+use pyo3::PyVisit;
+
+#[pyclass]
+struct TraverseTriesToTakePyRef {}
+
+#[pymethods]
+impl TraverseTriesToTakePyRef {
+    fn __traverse__(slf: PyRef<Self>, visit: PyVisit) {}
+}
+
+fn main() {}

tests/ui/traverse_bare_self.stderr

@@ -0,0 +1,17 @@
+error[E0308]: mismatched types
+ --> tests/ui/traverse_bare_self.rs:8:6
+  |
+7 |   #[pymethods]
+  |   ------------ arguments to this function are incorrect
+8 |   impl TraverseTriesToTakePyRef {
+  |  ______^
+9 | |     fn __traverse__(slf: PyRef<Self>, visit: PyVisit) {}
+  | |___________________^ expected fn pointer, found fn item
+  |
+  = note: expected fn pointer `for<'a, 'b> fn(&'a TraverseTriesToTakePyRef, PyVisit<'b>) -> Result<(), PyTraverseError>`
+                found fn item `for<'a, 'b> fn(pyo3::PyRef<'a, TraverseTriesToTakePyRef>, PyVisit<'b>) {TraverseTriesToTakePyRef::__traverse__}`
+note: function defined here
+ --> src/impl_/pymethods.rs
+  |
+  | pub unsafe fn call_traverse_impl<T>(
+  |               ^^^^^^^^^^^^^^^^^^