add descriptive error msg for `__traverse__` receivers other than `&self` (#4045)

* add descriptive error msg for `__traverse__` receivers other than `self`

* add newsfragment

* improve error message

newsfragments/4045.fixed.md

@@ -0,0 +1 @@
+Add better error message on wrong receiver extraction in `__traverse__`.

pyo3-macros-backend/src/pymethod.rs

@@ -434,9 +434,24 @@ fn impl_traverse_slot(
     let Ctx { pyo3_path } = ctx;
     if let (Some(py_arg), _) = split_off_python_arg(&spec.signature.arguments) {
         return Err(syn::Error::new_spanned(py_arg.ty, "__traverse__ may not take `Python`. \
-            Usually, an implementation of `__traverse__` should do nothing but calls to `visit.call`. \
+            Usually, an implementation of `__traverse__(&self, visit: PyVisit<'_>) -> Result<(), PyTraverseError>` \
-            Most importantly, safe access to the GIL is prohibited inside implementations of `__traverse__`, \
+            should do nothing but calls to `visit.call`. Most importantly, safe access to the GIL is prohibited \
-            i.e. `Python::with_gil` will panic."));
+            inside implementations of `__traverse__`, i.e. `Python::with_gil` will panic."));
+    }
+
+    // check that the receiver does not try to smuggle an (implicit) `Python` token into here
+    if let FnType::Fn(SelfType::TryFromBoundRef(span))
+    | FnType::Fn(SelfType::Receiver {
+        mutable: true,
+        span,
+    }) = spec.tp
+    {
+        bail_spanned! { span =>
+            "__traverse__ may not take a receiver other than `&self`. Usually, an implementation of \
+            `__traverse__(&self, visit: PyVisit<'_>) -> Result<(), PyTraverseError>` \
+            should do nothing but calls to `visit.call`. Most importantly, safe access to the GIL is prohibited \
+            inside implementations of `__traverse__`, i.e. `Python::with_gil` will panic."
+        }
     }
 
     let rust_fn_ident = spec.name;

tests/ui/traverse.rs

@@ -1,13 +1,55 @@
 use pyo3::prelude::*;
-use pyo3::PyVisit;
 use pyo3::PyTraverseError;
+use pyo3::PyVisit;
 
 #[pyclass]
 struct TraverseTriesToTakePyRef {}
 
 #[pymethods]
 impl TraverseTriesToTakePyRef {
-    fn __traverse__(slf: PyRef<Self>, visit: PyVisit) {}
+    fn __traverse__(slf: PyRef<Self>, visit: PyVisit) -> Result<(), PyTraverseError> {
+        Ok(())
+    }
+}
+
+#[pyclass]
+struct TraverseTriesToTakePyRefMut {}
+
+#[pymethods]
+impl TraverseTriesToTakePyRefMut {
+    fn __traverse__(slf: PyRefMut<Self>, visit: PyVisit) -> Result<(), PyTraverseError> {
+        Ok(())
+    }
+}
+
+#[pyclass]
+struct TraverseTriesToTakeBound {}
+
+#[pymethods]
+impl TraverseTriesToTakeBound {
+    fn __traverse__(slf: Bound<'_, Self>, visit: PyVisit) -> Result<(), PyTraverseError> {
+        Ok(())
+    }
+}
+
+#[pyclass]
+struct TraverseTriesToTakeMutSelf {}
+
+#[pymethods]
+impl TraverseTriesToTakeMutSelf {
+    fn __traverse__(&mut self, visit: PyVisit) -> Result<(), PyTraverseError> {
+        Ok(())
+    }
+}
+
+#[pyclass]
+struct TraverseTriesToTakeSelf {}
+
+#[pymethods]
+impl TraverseTriesToTakeSelf {
+    fn __traverse__(&self, visit: PyVisit) -> Result<(), PyTraverseError> {
+        Ok(())
+    }
 }
 
 #[pyclass]
@@ -19,9 +61,7 @@ impl Class {
         Ok(())
     }
 
-    fn __clear__(&mut self) {
+    fn __clear__(&mut self) {}
-    }
 }
 
-
 fn main() {}

tests/ui/traverse.stderr

@@ -1,23 +1,29 @@
-error: __traverse__ may not take `Python`. Usually, an implementation of `__traverse__` should do nothing but calls to `visit.call`. Most importantly, safe access to the GIL is prohibited inside implementations of `__traverse__`, i.e. `Python::with_gil` will panic.
+error: __traverse__ may not take a receiver other than `&self`. Usually, an implementation of `__traverse__(&self, visit: PyVisit<'_>) -> Result<(), PyTraverseError>` should do nothing but calls to `visit.call`. Most importantly, safe access to the GIL is prohibited inside implementations of `__traverse__`, i.e. `Python::with_gil` will panic.
-  --> tests/ui/traverse.rs:18:32
+  --> tests/ui/traverse.rs:10:26
    |
-18 |     fn __traverse__(&self, py: Python<'_>, visit: PyVisit<'_>) -> Result<(), PyTraverseError> {
+10 |     fn __traverse__(slf: PyRef<Self>, visit: PyVisit) -> Result<(), PyTraverseError> {
-   |                                ^^^^^^^^^^
+   |                          ^^^^^
 
-error[E0308]: mismatched types
+error: __traverse__ may not take a receiver other than `&self`. Usually, an implementation of `__traverse__(&self, visit: PyVisit<'_>) -> Result<(), PyTraverseError>` should do nothing but calls to `visit.call`. Most importantly, safe access to the GIL is prohibited inside implementations of `__traverse__`, i.e. `Python::with_gil` will panic.
-  --> tests/ui/traverse.rs:9:6
+  --> tests/ui/traverse.rs:20:26
    |
-8  |   #[pymethods]
+20 |     fn __traverse__(slf: PyRefMut<Self>, visit: PyVisit) -> Result<(), PyTraverseError> {
-   |   ------------ arguments to this function are incorrect
+   |                          ^^^^^^^^
-9  |   impl TraverseTriesToTakePyRef {
+
-   |  ______^
+error: __traverse__ may not take a receiver other than `&self`. Usually, an implementation of `__traverse__(&self, visit: PyVisit<'_>) -> Result<(), PyTraverseError>` should do nothing but calls to `visit.call`. Most importantly, safe access to the GIL is prohibited inside implementations of `__traverse__`, i.e. `Python::with_gil` will panic.
-10 | |     fn __traverse__(slf: PyRef<Self>, visit: PyVisit) {}
+  --> tests/ui/traverse.rs:30:26
-   | |___________________^ expected fn pointer, found fn item
    |
-   = note: expected fn pointer `for<'a, 'b> fn(&'a TraverseTriesToTakePyRef, PyVisit<'b>) -> Result<(), PyTraverseError>`
+30 |     fn __traverse__(slf: Bound<'_, Self>, visit: PyVisit) -> 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
+error: __traverse__ may not take a receiver other than `&self`. Usually, an implementation of `__traverse__(&self, visit: PyVisit<'_>) -> Result<(), PyTraverseError>` should do nothing but calls to `visit.call`. Most importantly, safe access to the GIL is prohibited inside implementations of `__traverse__`, i.e. `Python::with_gil` will panic.
+  --> tests/ui/traverse.rs:40:21
    |
-   | pub unsafe fn _call_traverse<T>(
+40 |     fn __traverse__(&mut self, visit: PyVisit) -> Result<(), PyTraverseError> {
-   |               ^^^^^^^^^^^^^^
+   |                     ^
+
+error: __traverse__ may not take `Python`. Usually, an implementation of `__traverse__(&self, visit: PyVisit<'_>) -> Result<(), PyTraverseError>` should do nothing but calls to `visit.call`. Most importantly, safe access to the GIL is prohibited inside implementations of `__traverse__`, i.e. `Python::with_gil` will panic.
+  --> tests/ui/traverse.rs:60:32
+   |
+60 |     fn __traverse__(&self, py: Python<'_>, visit: PyVisit<'_>) -> Result<(), PyTraverseError> {
+   |                                ^^^^^^^^^^