require #[pyclass] to be Sync (#4566)

* require `#[pyclass]` to be `Sync`

* silence deprecation warning in pyo3 internals

* make 'cargo test' pass (modulo test_getter_setter)

* fix nox -s test-py

* silence new deprecation warning

* use `unsendable` to test cell get / set

* add WIP changelog entry

* fix wasm clippy

* add a test for assert_pyclass_sync to fix coverage error

* use a better name

* fix cargo doc --lib

* fix building with no default features

* apply Bruno's wording suggestions

* simplify compile-time checking for PyClass being Sync

* update discussion around the decorator example in the guide

---------

Co-authored-by: Nathan Goldbaum <[email protected]>

examples/decorator/src/lib.rs

@@ -1,6 +1,6 @@
 use pyo3::prelude::*;
 use pyo3::types::{PyDict, PyTuple};
-use std::cell::Cell;
+use std::sync::atomic::{AtomicU64, Ordering};
 
 /// A function decorator that keeps track how often it is called.
 ///
@@ -9,8 +9,8 @@ use std::cell::Cell;
 pub struct PyCounter {
     // Keeps track of how many calls have gone through.
     //
-    // See the discussion at the end for why `Cell` is used.
-    count: Cell<u64>,
+    // See the discussion at the end for why `AtomicU64` is used.
+    count: AtomicU64,
 
     // This is the actual function being wrapped.
     wraps: Py<PyAny>,
@@ -26,14 +26,14 @@ impl PyCounter {
     #[new]
     fn __new__(wraps: Py<PyAny>) -> Self {
         PyCounter {
-            count: Cell::new(0),
+            count: AtomicU64::new(0),
             wraps,
         }
     }
 
     #[getter]
     fn count(&self) -> u64 {
-        self.count.get()
+        self.count.load(Ordering::Relaxed)
     }
 
     #[pyo3(signature = (*args, **kwargs))]
@@ -43,9 +43,7 @@ impl PyCounter {
         args: &Bound<'_, PyTuple>,
         kwargs: Option<&Bound<'_, PyDict>>,
     ) -> PyResult<Py<PyAny>> {
-        let old_count = self.count.get();
-        let new_count = old_count + 1;
-        self.count.set(new_count);
+        let new_count = self.count.fetch_add(1, Ordering::Relaxed);
         let name = self.wraps.getattr(py, "__name__")?;
 
         println!("{} has been called {} time(s).", name, new_count);

guide/src/class/call.md

@@ -66,7 +66,7 @@ def Counter(wraps):
     return call
 ```
 
-### What is the `Cell` for?
+### What is the `AtomicU64` for?
 
 A [previous implementation] used a normal `u64`, which meant it required a `&mut self` receiver to update the count:
 
@@ -108,14 +108,15 @@ say_hello()
 # RuntimeError: Already borrowed
 ```
 
-The implementation in this chapter fixes that by never borrowing exclusively; all the methods take `&self` as receivers, of which multiple may exist simultaneously. This requires a shared counter and the easiest way to do that is to use [`Cell`], so that's what is used here.
+The implementation in this chapter fixes that by never borrowing exclusively; all the methods take `&self` as receivers, of which multiple may exist simultaneously. This requires a shared counter and the most straightforward way to implement thread-safe interior mutability (e.g. the type does not need to accept `&mut self` to modify the "interior" state) for a `u64` is to use [`AtomicU64`], so that's what is used here.
 
 This shows the dangers of running arbitrary Python code - note that "running arbitrary Python code" can be far more subtle than the example above:
 - Python's asynchronous executor may park the current thread in the middle of Python code, even in Python code that *you* control, and let other Python code run.
 - Dropping arbitrary Python objects may invoke destructors defined in Python (`__del__` methods).
 - Calling Python's C-api (most PyO3 apis call C-api functions internally) may raise exceptions, which may allow Python code in signal handlers to run.
+- On the free-threaded build, users might use Python's `threading` module to work with your types simultaneously from multiple OS threads.
 
 This is especially important if you are writing unsafe code; Python code must never be able to cause undefined behavior. You must ensure that your Rust code is in a consistent state before doing any of the above things.
 
 [previous implementation]: https://github.com/PyO3/pyo3/discussions/2598 "Thread Safe Decorator <Help Wanted> · Discussion #2598 · PyO3/pyo3"
-[`Cell`]: https://doc.rust-lang.org/std/cell/struct.Cell.html "Cell in std::cell - Rust"
+[`AtomicU64`]: https://doc.rust-lang.org/std/sync/atomic/struct.AtomicU64.html "AtomicU64 in std::sync::atomic - Rust"

guide/src/class/protocols.md

@@ -158,18 +158,20 @@ Example:
 ```rust
 use pyo3::prelude::*;
 
+use std::sync::Mutex;
+
 #[pyclass]
 struct MyIterator {
-    iter: Box<dyn Iterator<Item = PyObject> + Send>,
+    iter: Mutex<Box<dyn Iterator<Item = PyObject> + Send>>,
 }
 
 #[pymethods]
 impl MyIterator {
     fn __iter__(slf: PyRef<'_, Self>) -> PyRef<'_, Self> {
         slf
     }
-    fn __next__(mut slf: PyRefMut<'_, Self>) -> Option<PyObject> {
-        slf.iter.next()
+    fn __next__(slf: PyRefMut<'_, Self>) -> Option<PyObject> {
+        slf.iter.lock().unwrap().next()
     }
 }
 ```

guide/src/migration.md

@@ -1805,7 +1805,7 @@ There can be two fixes:
    ```
 
    After:
-   ```rust
+   ```rust,ignore
    # #![allow(dead_code)]
    use pyo3::prelude::*;
    use std::sync::{Arc, Mutex};

newsfragments/4566.changed.md

@@ -0,0 +1,5 @@
+* The `pyclass` macro now creates a rust type that is `Sync` by default.  If you
+  would like to opt out of this, annotate your class with
+  `pyclass(unsendable)`. See the migraiton guide entry (INSERT GUIDE LINK HERE)
+  for more information on updating to accommadate this change.
+

pyo3-macros-backend/src/pyclass.rs

@@ -2310,7 +2310,21 @@ impl<'a> PyClassImplsBuilder<'a> {
             }
         });
 
+        let assertions = if attr.options.unsendable.is_some() {
+            TokenStream::new()
+        } else {
+            quote_spanned! {
+                cls.span() =>
+                const _: () = {
+                    use #pyo3_path::impl_::pyclass::*;
+                    assert_pyclass_sync::<#cls>();
+                };
+            }
+        };
+
         Ok(quote! {
+            #assertions
+
             #pyclass_base_type_impl
 
             impl #pyo3_path::impl_::pyclass::PyClassImpl for #cls {

src/coroutine.rs

@@ -35,6 +35,10 @@ pub struct Coroutine {
     waker: Option<Arc<AsyncioWaker>>,
 }
 
+// Safety: `Coroutine` is allowed to be `Sync` even though the future is not,
+// because the future is polled with `&mut self` receiver
+unsafe impl Sync for Coroutine {}
+
 impl Coroutine {
     ///  Wrap a future into a Python coroutine.
     ///

src/impl_/pyclass.rs

@@ -23,8 +23,13 @@ use std::{
     thread,
 };
 
+mod assertions;
 mod lazy_type_object;
+mod probes;
+
+pub use assertions::*;
 pub use lazy_type_object::LazyTypeObject;
+pub use probes::*;
 
 /// Gets the offset of the dictionary from the start of the object in bytes.
 #[inline]
@@ -1418,67 +1423,6 @@ impl<ClassT: PyClass, FieldT, Offset: OffsetCalculator<ClassT, FieldT>>
     }
 }
 
-/// Trait used to combine with zero-sized types to calculate at compile time
-/// some property of a type.
-///
-/// The trick uses the fact that an associated constant has higher priority
-/// than a trait constant, so we can use the trait to define the false case.
-///
-/// The true case is defined in the zero-sized type's impl block, which is
-/// gated on some property like trait bound or only being implemented
-/// for fixed concrete types.
-pub trait Probe {
-    const VALUE: bool = false;
-}
-
-macro_rules! probe {
-    ($name:ident) => {
-        pub struct $name<T>(PhantomData<T>);
-        impl<T> Probe for $name<T> {}
-    };
-}
-
-probe!(IsPyT);
-
-impl<T> IsPyT<Py<T>> {
-    pub const VALUE: bool = true;
-}
-
-probe!(IsToPyObject);
-
-#[allow(deprecated)]
-impl<T: ToPyObject> IsToPyObject<T> {
-    pub const VALUE: bool = true;
-}
-
-probe!(IsIntoPy);
-
-#[allow(deprecated)]
-impl<T: IntoPy<crate::PyObject>> IsIntoPy<T> {
-    pub const VALUE: bool = true;
-}
-
-probe!(IsIntoPyObjectRef);
-
-// Possible clippy beta regression,
-// see https://github.com/rust-lang/rust-clippy/issues/13578
-#[allow(clippy::extra_unused_lifetimes)]
-impl<'a, 'py, T: 'a> IsIntoPyObjectRef<T>
-where
-    &'a T: IntoPyObject<'py>,
-{
-    pub const VALUE: bool = true;
-}
-
-probe!(IsIntoPyObject);
-
-impl<'py, T> IsIntoPyObject<T>
-where
-    T: IntoPyObject<'py>,
-{
-    pub const VALUE: bool = true;
-}
-
 /// ensures `obj` is not mutably aliased
 #[inline]
 unsafe fn ensure_no_mutable_alias<'py, ClassT: PyClass>(

src/impl_/pyclass/assertions.rs

@@ -0,0 +1,40 @@
+/// Helper function that can be used at compile time to emit a diagnostic if
+/// the type does not implement `Sync` when it should.
+///
+/// The mere act of invoking this function will cause the diagnostic to be
+/// emitted if `T` does not implement `Sync` when it should.
+///
+/// The additional `const IS_SYNC: bool` parameter is used to allow the custom
+/// diagnostic to be emitted; if `PyClassSync`
+#[allow(unused)]
+pub const fn assert_pyclass_sync<T>()
+where
+    T: PyClassSync + Sync,
+{
+}
+
+#[cfg_attr(
+    diagnostic_namespace,
+    diagnostic::on_unimplemented(
+        message = "the trait `Sync` is not implemented for `{Self}`",
+        label = "required by `#[pyclass]`",
+        note = "replace thread-unsafe fields with thread-safe alternatives",
+        note = "see <TODO INSERT PYO3 GUIDE> for more information",
+    )
+)]
+pub trait PyClassSync<T: Sync = Self> {}
+
+impl<T> PyClassSync for T where T: Sync {}
+
+mod tests {
+    #[cfg(feature = "macros")]
+    #[test]
+    fn test_assert_pyclass_sync() {
+        use super::assert_pyclass_sync;
+
+        #[crate::pyclass(crate = "crate")]
+        struct MyClass {}
+
+        assert_pyclass_sync::<MyClass>();
+    }
+}

src/impl_/pyclass/probes.rs

@@ -0,0 +1,72 @@
+use std::marker::PhantomData;
+
+use crate::{conversion::IntoPyObject, Py};
+#[allow(deprecated)]
+use crate::{IntoPy, ToPyObject};
+
+/// Trait used to combine with zero-sized types to calculate at compile time
+/// some property of a type.
+///
+/// The trick uses the fact that an associated constant has higher priority
+/// than a trait constant, so we can use the trait to define the false case.
+///
+/// The true case is defined in the zero-sized type's impl block, which is
+/// gated on some property like trait bound or only being implemented
+/// for fixed concrete types.
+pub trait Probe {
+    const VALUE: bool = false;
+}
+
+macro_rules! probe {
+    ($name:ident) => {
+        pub struct $name<T>(PhantomData<T>);
+        impl<T> Probe for $name<T> {}
+    };
+}
+
+probe!(IsPyT);
+
+impl<T> IsPyT<Py<T>> {
+    pub const VALUE: bool = true;
+}
+
+probe!(IsToPyObject);
+
+#[allow(deprecated)]
+impl<T: ToPyObject> IsToPyObject<T> {
+    pub const VALUE: bool = true;
+}
+
+probe!(IsIntoPy);
+
+#[allow(deprecated)]
+impl<T: IntoPy<crate::PyObject>> IsIntoPy<T> {
+    pub const VALUE: bool = true;
+}
+
+probe!(IsIntoPyObjectRef);
+
+// Possible clippy beta regression,
+// see https://github.com/rust-lang/rust-clippy/issues/13578
+#[allow(clippy::extra_unused_lifetimes)]
+impl<'a, 'py, T: 'a> IsIntoPyObjectRef<T>
+where
+    &'a T: IntoPyObject<'py>,
+{
+    pub const VALUE: bool = true;
+}
+
+probe!(IsIntoPyObject);
+
+impl<'py, T> IsIntoPyObject<T>
+where
+    T: IntoPyObject<'py>,
+{
+    pub const VALUE: bool = true;
+}
+
+probe!(IsSync);
+
+impl<T: Sync> IsSync<T> {
+    pub const VALUE: bool = true;
+}