require PyCFunction::new_closure closures to be Sync

newsfragments/6096.fixed.md

@@ -0,0 +1 @@
+Fix missing `Sync` bound on closure type in `PyCFunction::new_closure`.

src/types/function.rs

@@ -103,7 +103,7 @@ impl PyCFunction {
         closure: F,
     ) -> PyResult<Bound<'py, Self>>
     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");

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<i32> {
-            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();
 

tests/ui/invalid_closure.rs

@@ -2,22 +2,17 @@ use pyo3::prelude::*;
 use pyo3::types::{PyCFunction, PyDict, PyTuple};
 
 fn main() {
-    let fun: Py<PyCFunction> = 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();
     });
 }

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
 

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
+    });
+}

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`.

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<i32>` cannot be shared between threads safely
+    });
+}

tests/ui/invalid_closure_sync.stderr

@@ -0,0 +1,36 @@
+error[E0277]: `Cell<i32>` 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<i32>` 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<i32>`
+    = 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`.