diff --git a/newsfragments/4045.fixed.md b/newsfragments/4045.fixed.md new file mode 100644 index 00000000..6b2bbcfa --- /dev/null +++ b/newsfragments/4045.fixed.md @@ -0,0 +1 @@ +Add better error message on wrong receiver extraction in `__traverse__`. diff --git a/pyo3-macros-backend/src/pymethod.rs b/pyo3-macros-backend/src/pymethod.rs index ee7d3d7a..abe8c7ac 100644 --- a/pyo3-macros-backend/src/pymethod.rs +++ b/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`. \ - Most importantly, safe access to the GIL is prohibited inside implementations of `__traverse__`, \ - i.e. `Python::with_gil` will panic.")); + 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.")); + } + + // 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; diff --git a/tests/ui/traverse.rs b/tests/ui/traverse.rs index 03422495..0cf7170d 100644 --- a/tests/ui/traverse.rs +++ b/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, visit: PyVisit) {} + fn __traverse__(slf: PyRef, visit: PyVisit) -> Result<(), PyTraverseError> { + Ok(()) + } +} + +#[pyclass] +struct TraverseTriesToTakePyRefMut {} + +#[pymethods] +impl TraverseTriesToTakePyRefMut { + fn __traverse__(slf: PyRefMut, 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() {} diff --git a/tests/ui/traverse.stderr b/tests/ui/traverse.stderr index 4448e67e..5b1d1b6b 100644 --- a/tests/ui/traverse.stderr +++ b/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. - --> tests/ui/traverse.rs:18:32 +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:10:26 | -18 | fn __traverse__(&self, py: Python<'_>, visit: PyVisit<'_>) -> Result<(), PyTraverseError> { - | ^^^^^^^^^^ +10 | fn __traverse__(slf: PyRef, visit: PyVisit) -> Result<(), PyTraverseError> { + | ^^^^^ -error[E0308]: mismatched types - --> tests/ui/traverse.rs:9:6 +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:20:26 | -8 | #[pymethods] - | ------------ arguments to this function are incorrect -9 | impl TraverseTriesToTakePyRef { - | ______^ -10 | | fn __traverse__(slf: PyRef, visit: PyVisit) {} - | |___________________^ expected fn pointer, found fn item +20 | fn __traverse__(slf: PyRefMut, visit: PyVisit) -> Result<(), PyTraverseError> { + | ^^^^^^^^ + +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:30:26 | - = 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 +30 | fn __traverse__(slf: Bound<'_, Self>, visit: PyVisit) -> Result<(), PyTraverseError> { + | ^^^^^ + +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( - | ^^^^^^^^^^^^^^ +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> { + | ^^^^^^^^^^