diff --git a/newsfragments/6096.fixed.md b/newsfragments/6096.fixed.md new file mode 100644 index 00000000000..84326d5edc2 --- /dev/null +++ b/newsfragments/6096.fixed.md @@ -0,0 +1 @@ +Fix missing `Sync` bound on closure type in `PyCFunction::new_closure`. diff --git a/src/types/function.rs b/src/types/function.rs index ec17e9c2e62..f97183b7cdb 100644 --- a/src/types/function.rs +++ b/src/types/function.rs @@ -103,7 +103,7 @@ impl PyCFunction { closure: F, ) -> PyResult> where - F: Fn(&Bound<'_, PyTuple>, Option<&Bound<'_, PyDict>>) -> R + Send + 'static, + F: Fn(&Bound<'_, PyTuple>, Option<&Bound<'_, PyDict>>) -> R + Send + Sync + 'static, for<'p> R: crate::impl_::callback::IntoPyCallbackOutput<'p, *mut ffi::PyObject>, { let name = name.unwrap_or(c"pyo3-closure"); diff --git a/tests/test_pyfunction.rs b/tests/test_pyfunction.rs index 046b6044c86..34f707fcb4e 100644 --- a/tests/test_pyfunction.rs +++ b/tests/test_pyfunction.rs @@ -2,6 +2,7 @@ #![warn(unsafe_op_in_unsafe_fn)] use std::collections::HashMap; +use std::sync::atomic::{AtomicI32, Ordering}; #[cfg(not(Py_LIMITED_API))] use pyo3::buffer::PyBuffer; @@ -565,13 +566,12 @@ fn test_closure() { #[test] fn test_closure_counter() { Python::attach(|py| { - let counter = std::cell::RefCell::new(0); + let counter = AtomicI32::new(0); let counter_fn = move |_args: &Bound<'_, types::PyTuple>, _kwargs: Option<&Bound<'_, types::PyDict>>| -> PyResult { - let mut counter = counter.borrow_mut(); - *counter += 1; - Ok(*counter) + let prev_count = counter.fetch_add(1, Ordering::SeqCst); + Ok(prev_count + 1) }; let counter_py = PyCFunction::new_closure(py, None, None, counter_fn).unwrap(); diff --git a/tests/ui/invalid_closure.rs b/tests/ui/invalid_closure.rs index feb64c561b4..9ae8da2386b 100644 --- a/tests/ui/invalid_closure.rs +++ b/tests/ui/invalid_closure.rs @@ -2,22 +2,17 @@ use pyo3::prelude::*; use pyo3::types::{PyCFunction, PyDict, PyTuple}; fn main() { - let fun: Py = Python::attach(|py| { + // Closure must be `'static` + Python::attach(|py| { let local_data = vec![0, 1, 2, 3, 4]; let ref_: &[u8] = &local_data; -//~^ ERROR: `local_data` does not live long enough + //~^ ERROR: `local_data` does not live long enough let closure_fn = |_args: &Bound<'_, PyTuple>, _kwargs: Option<&Bound<'_, PyDict>>| -> PyResult<()> { println!("This is five: {:?}", ref_.len()); Ok(()) }; - PyCFunction::new_closure(py, None, None, closure_fn) - .unwrap() - .into() - }); - - Python::attach(|py| { - fun.call0(py).unwrap(); + PyCFunction::new_closure(py, None, None, closure_fn).unwrap(); }); } diff --git a/tests/ui/invalid_closure.stderr b/tests/ui/invalid_closure.stderr index e50dd61beef..b20cde4aa32 100644 --- a/tests/ui/invalid_closure.stderr +++ b/tests/ui/invalid_closure.stderr @@ -1,22 +1,21 @@ error[E0597]: `local_data` does not live long enough - --> tests/ui/invalid_closure.rs:7:27 + --> tests/ui/invalid_closure.rs:8:27 | - 6 | let local_data = vec![0, 1, 2, 3, 4]; + 7 | let local_data = vec![0, 1, 2, 3, 4]; | ---------- binding `local_data` declared here - 7 | let ref_: &[u8] = &local_data; + 8 | let ref_: &[u8] = &local_data; | ^^^^^^^^^^^ borrowed value does not live long enough ... - 15 | PyCFunction::new_closure(py, None, None, closure_fn) + 16 | PyCFunction::new_closure(py, None, None, closure_fn).unwrap(); | ---------------------------------------------------- argument requires that `local_data` is borrowed for `'static` -... - 18 | }); + 17 | }); | - `local_data` dropped here while still borrowed | note: requirement that the value outlives `'static` introduced here --> src/types/function.rs | - | F: Fn(&Bound<'_, PyTuple>, Option<&Bound<'_, PyDict>>) -> R + Send + 'static, - | ^^^^^^^ + | F: Fn(&Bound<'_, PyTuple>, Option<&Bound<'_, PyDict>>) -> R + Send + Sync + 'static, + | ^^^^^^^ error: aborting due to 1 previous error diff --git a/tests/ui/invalid_closure_send.rs b/tests/ui/invalid_closure_send.rs new file mode 100644 index 00000000000..21fac3aeda7 --- /dev/null +++ b/tests/ui/invalid_closure_send.rs @@ -0,0 +1,22 @@ +use pyo3::prelude::*; +use pyo3::types::{PyCFunction, PyDict, PyTuple}; + +#[derive(Clone)] +struct NotSend(*mut std::ffi::c_void); +unsafe impl Sync for NotSend {} + +fn main() { + // Closure must be `Send` + Python::attach(|py| { + let value = NotSend(std::ptr::null_mut()); + let closure_fn = move |_args: &Bound<'_, PyTuple>, + _kwargs: Option<&Bound<'_, PyDict>>| + -> PyResult<()> { + let _ = value.clone(); + Ok(()) + }; + + PyCFunction::new_closure(py, None, None, closure_fn).unwrap(); + //~^ ERROR: `*mut c_void` cannot be sent between threads safely + }); +} diff --git a/tests/ui/invalid_closure_send.stderr b/tests/ui/invalid_closure_send.stderr new file mode 100644 index 00000000000..5c6ea2999b1 --- /dev/null +++ b/tests/ui/invalid_closure_send.stderr @@ -0,0 +1,40 @@ +error[E0277]: `*mut c_void` cannot be sent between threads safely + --> tests/ui/invalid_closure_send.rs:19:50 + | + 12 | let closure_fn = move |_args: &Bound<'_, PyTuple>, + | __________________________- + 13 | | _kwargs: Option<&Bound<'_, PyDict>>| + 14 | | -> PyResult<()> { + | |_____________________________- within this `{closure@tests/ui/invalid_closure_send.rs:12:26: 14:30}` +... + 19 | PyCFunction::new_closure(py, None, None, closure_fn).unwrap(); + | ------------------------ ^^^^^^^^^^ `*mut c_void` cannot be sent between threads safely + | | + | required by a bound introduced by this call + | + = help: within `{closure@tests/ui/invalid_closure_send.rs:12:26: 14:30}`, the trait `Send` is not implemented for `*mut c_void` +note: required because it appears within the type `NotSend` + --> tests/ui/invalid_closure_send.rs:5:8 + | + 5 | struct NotSend(*mut std::ffi::c_void); + | ^^^^^^^ +note: required because it's used within this closure + --> tests/ui/invalid_closure_send.rs:12:26 + | + 12 | let closure_fn = move |_args: &Bound<'_, PyTuple>, + | __________________________^ + 13 | | _kwargs: Option<&Bound<'_, PyDict>>| + 14 | | -> PyResult<()> { + | |_____________________________^ +note: required by a bound in `PyCFunction::new_closure` + --> src/types/function.rs + | + | pub fn new_closure<'py, F, R>( + | ----------- required by a bound in this associated function +... + | F: Fn(&Bound<'_, PyTuple>, Option<&Bound<'_, PyDict>>) -> R + Send + Sync + 'static, + | ^^^^ required by this bound in `PyCFunction::new_closure` + +error: aborting due to 1 previous error + +For more information about this error, try `rustc --explain E0277`. diff --git a/tests/ui/invalid_closure_sync.rs b/tests/ui/invalid_closure_sync.rs new file mode 100644 index 00000000000..1fc7a25825e --- /dev/null +++ b/tests/ui/invalid_closure_sync.rs @@ -0,0 +1,18 @@ +use pyo3::prelude::*; +use pyo3::types::{PyCFunction, PyDict, PyTuple}; + +fn main() { + // Closure must be `Sync` + Python::attach(|py| { + let data = std::cell::Cell::new(0); + let closure_fn = move |_args: &Bound<'_, PyTuple>, + _kwargs: Option<&Bound<'_, PyDict>>| + -> PyResult<()> { + let _ = data.clone(); + Ok(()) + }; + + PyCFunction::new_closure(py, None, None, closure_fn).unwrap(); + //~^ ERROR: `Cell` cannot be shared between threads safely + }); +} diff --git a/tests/ui/invalid_closure_sync.stderr b/tests/ui/invalid_closure_sync.stderr new file mode 100644 index 00000000000..a68f45a8ac3 --- /dev/null +++ b/tests/ui/invalid_closure_sync.stderr @@ -0,0 +1,36 @@ +error[E0277]: `Cell` cannot be shared between threads safely + --> tests/ui/invalid_closure_sync.rs:15:50 + | + 8 | let closure_fn = move |_args: &Bound<'_, PyTuple>, + | __________________________- + 9 | | _kwargs: Option<&Bound<'_, PyDict>>| + 10 | | -> PyResult<()> { + | |_____________________________- within this `{closure@tests/ui/invalid_closure_sync.rs:8:26: 10:30}` +... + 15 | PyCFunction::new_closure(py, None, None, closure_fn).unwrap(); + | ------------------------ ^^^^^^^^^^ `Cell` cannot be shared between threads safely + | | + | required by a bound introduced by this call + | + = help: within `{closure@tests/ui/invalid_closure_sync.rs:8:26: 10:30}`, the trait `Sync` is not implemented for `Cell` + = note: if you want to do aliasing and mutation between multiple threads, use `std::sync::RwLock` or `std::sync::atomic::AtomicI32` instead +note: required because it's used within this closure + --> tests/ui/invalid_closure_sync.rs:8:26 + | + 8 | let closure_fn = move |_args: &Bound<'_, PyTuple>, + | __________________________^ + 9 | | _kwargs: Option<&Bound<'_, PyDict>>| + 10 | | -> PyResult<()> { + | |_____________________________^ +note: required by a bound in `PyCFunction::new_closure` + --> src/types/function.rs + | + | pub fn new_closure<'py, F, R>( + | ----------- required by a bound in this associated function +... + | F: Fn(&Bound<'_, PyTuple>, Option<&Bound<'_, PyDict>>) -> R + Send + Sync + 'static, + | ^^^^ required by this bound in `PyCFunction::new_closure` + +error: aborting due to 1 previous error + +For more information about this error, try `rustc --explain E0277`.