Merge #2975 #3022 #3023

2975: RFC: Add GILProtected synchronization primitive and use it for LazyTypeObjectInner. r=davidhewitt a=adamreichold

I would also like to use that type in rust-numpy and it seems we can avoid ~~both a manual unsafe impl and~~ a full blown mutex if we apply it to `LazyTypeObjectInner`.

One downside might be that it ties us closer to the GIL when we want to enable nogil experimentation, but on the other hand, it may also help by reifying the GIL usage. (This is currently limited to comments in unsafe code in rust-numpy for example.)

3022: Fix function name shadowing r=davidhewitt a=mejrs

Fixes #3017

3023: Emit a better error for bad argument names r=davidhewitt a=mejrs

This will emit a better error for code like 
```rust
#[pyfunction]
fn output([a,b,c]: [u8;3]) {}
```



Co-authored-by: Adam Reichold <adam.reichold@t-online.de>
Co-authored-by: mejrs <59372212+mejrs@users.noreply.github.com>

guide/src/faq.md

@@ -13,7 +13,7 @@
 
 PyO3 provides a struct [`GILOnceCell`] which works equivalently to `OnceCell` but relies solely on the Python GIL for thread safety. This means it can be used in place of `lazy_static` or `once_cell` where you are experiencing the deadlock described above. See the documentation for [`GILOnceCell`] for an example how to use it.
 
-[`GILOnceCell`]: {{#PYO3_DOCS_URL}}/pyo3/once_cell/struct.GILOnceCell.html
+[`GILOnceCell`]: {{#PYO3_DOCS_URL}}/pyo3/sync/struct.GILOnceCell.html
 
 ## I can't run `cargo test`; or I can't build in a Cargo workspace: I'm having linker issues like "Symbol not found" or "Undefined reference to _PyExc_SystemError"!
 

newsfragments/2975.added.md

@@ -0,0 +1 @@
+Add `GILProtected<T>` to mediate concurrent access to a value using Python's global interpreter lock (GIL).

newsfragments/3022.fixed.md

@@ -0,0 +1 @@
+Fix compile error for `#[pymethods]` and `#[pyfunction]` called "output".

pyo3-macros-backend/src/method.rs

@@ -40,14 +40,14 @@ impl<'a> FnArg<'a> {
                 }
 
                 let arg_attrs = PyFunctionArgPyO3Attributes::from_attrs(&mut cap.attrs)?;
-                let (ident, by_ref, mutability) = match *cap.pat {
+                let (ident, by_ref, mutability) = match &*cap.pat {
                     syn::Pat::Ident(syn::PatIdent {
-                        ref ident,
-                        ref by_ref,
-                        ref mutability,
+                        ident,
+                        by_ref,
+                        mutability,
                         ..
                     }) => (ident, by_ref, mutability),
-                    _ => bail_spanned!(cap.pat.span() => "unsupported argument"),
+                    other => return Err(handle_argument_error(other)),
                 };
 
                 Ok(FnArg {
@@ -67,6 +67,19 @@ impl<'a> FnArg<'a> {
     }
 }
 
+fn handle_argument_error(pat: &syn::Pat) -> syn::Error {
+    let span = pat.span();
+    let msg = match pat {
+        syn::Pat::Wild(_) => "wildcard argument names are not supported",
+        syn::Pat::Struct(_)
+        | syn::Pat::Tuple(_)
+        | syn::Pat::TupleStruct(_)
+        | syn::Pat::Slice(_) => "destructuring in arguments is not supported",
+        _ => "unsupported argument",
+    };
+    syn::Error::new(span, msg)
+}
+
 #[derive(Clone, PartialEq, Debug, Copy, Eq)]
 pub enum MethodTypeAttribute {
     /// `#[new]`
@@ -409,21 +422,22 @@ impl<'a> FnSpec<'a> {
         let self_arg = self.tp.self_arg();
         let py = syn::Ident::new("_py", Span::call_site());
         let func_name = &self.name;
-        let rust_name = if let Some(cls) = cls {
-            quote!(#cls::#func_name)
-        } else {
-            quote!(#func_name)
-        };
 
         let rust_call = |args: Vec<TokenStream>| {
             quote! {
-                let mut ret = #rust_name(#self_arg #(#args),*);
+                let mut ret = function(#self_arg #(#args),*);
                 let owned = _pyo3::impl_::pymethods::OkWrap::wrap(ret, #py);
                 owned.map(|obj| _pyo3::conversion::IntoPyPointer::into_ptr(obj))
                     .map_err(::core::convert::Into::into)
             }
         };
 
+        let rust_name = if let Some(cls) = cls {
+            quote!(#cls::#func_name)
+        } else {
+            quote!(#func_name)
+        };
+
         Ok(match self.convention {
             CallingConvention::Noargs => {
                 let call = if !self.signature.arguments.is_empty() {
@@ -437,6 +451,7 @@ impl<'a> FnSpec<'a> {
                         #py: _pyo3::Python<'py>,
                         _slf: *mut _pyo3::ffi::PyObject,
                     ) -> _pyo3::PyResult<*mut _pyo3::ffi::PyObject> {
+                        let function = #rust_name; // Shadow the function name to avoid #3017
                         #deprecations
                         #self_conversion
                         #call
@@ -454,6 +469,7 @@ impl<'a> FnSpec<'a> {
                         _nargs: _pyo3::ffi::Py_ssize_t,
                         _kwnames: *mut _pyo3::ffi::PyObject
                     ) -> _pyo3::PyResult<*mut _pyo3::ffi::PyObject> {
+                        let function = #rust_name; // Shadow the function name to avoid #3017
                         #deprecations
                         #self_conversion
                         #arg_convert
@@ -471,6 +487,7 @@ impl<'a> FnSpec<'a> {
                         _args: *mut _pyo3::ffi::PyObject,
                         _kwargs: *mut _pyo3::ffi::PyObject
                     ) -> _pyo3::PyResult<*mut _pyo3::ffi::PyObject> {
+                        let function = #rust_name; // Shadow the function name to avoid #3017
                         #deprecations
                         #self_conversion
                         #arg_convert
@@ -489,6 +506,7 @@ impl<'a> FnSpec<'a> {
                         _kwargs: *mut _pyo3::ffi::PyObject
                     ) -> _pyo3::PyResult<*mut _pyo3::ffi::PyObject> {
                         use _pyo3::callback::IntoPyCallbackOutput;
+                        let function = #rust_name; // Shadow the function name to avoid #3017
                         #deprecations
                         #arg_convert
                         let result = #call;

pyo3-macros-backend/src/pymethod.rs

@@ -441,9 +441,9 @@ fn impl_py_class_attribute(cls: &syn::Type, spec: &FnSpec<'_>) -> syn::Result<Me
 
     let name = &spec.name;
     let fncall = if py_arg.is_some() {
-        quote!(#cls::#name(py))
+        quote!(function(py))
     } else {
-        quote!(#cls::#name())
+        quote!(function())
     };
 
     let wrapper_ident = format_ident!("__pymethod_{}__", name);
@@ -452,6 +452,7 @@ fn impl_py_class_attribute(cls: &syn::Type, spec: &FnSpec<'_>) -> syn::Result<Me
 
     let associated_method = quote! {
         fn #wrapper_ident(py: _pyo3::Python<'_>) -> _pyo3::PyResult<_pyo3::PyObject> {
+            let function = #cls::#name; // Shadow the method name to avoid #3017
             #deprecations
             let mut ret = #fncall;
             let owned = _pyo3::impl_::pymethods::OkWrap::wrap(ret, py);
@@ -1151,12 +1152,14 @@ impl SlotDef {
             *extract_error_mode,
             return_mode.as_ref(),
         )?;
+        let name = spec.name;
         let associated_method = quote! {
             unsafe fn #wrapper_ident(
                 #py: _pyo3::Python<'_>,
                 _raw_slf: *mut _pyo3::ffi::PyObject,
                 #(#arg_idents: #arg_types),*
             ) -> _pyo3::PyResult<#ret_ty> {
+                let function = #cls::#name; // Shadow the method name to avoid #3017
                 let _slf = _raw_slf;
                 #body
             }

src/exceptions.rs

@@ -102,7 +102,7 @@ macro_rules! import_exception {
 
         impl $name {
             fn type_object_raw(py: $crate::Python<'_>) -> *mut $crate::ffi::PyTypeObject {
-                use $crate::once_cell::GILOnceCell;
+                use $crate::sync::GILOnceCell;
                 use $crate::AsPyPointer;
                 static TYPE_OBJECT: GILOnceCell<$crate::Py<$crate::types::PyType>> =
                     GILOnceCell::new();
@@ -241,7 +241,7 @@ macro_rules! create_exception_type_object {
 
         impl $name {
             fn type_object_raw(py: $crate::Python<'_>) -> *mut $crate::ffi::PyTypeObject {
-                use $crate::once_cell::GILOnceCell;
+                use $crate::sync::GILOnceCell;
                 use $crate::AsPyPointer;
                 static TYPE_OBJECT: GILOnceCell<$crate::Py<$crate::types::PyType>> =
                     GILOnceCell::new();

src/impl_/pyclass.rs

@@ -802,7 +802,7 @@ unsafe fn bpo_35810_workaround(_py: Python<'_>, ty: *mut ffi::PyTypeObject) {
     {
         // Must check version at runtime for abi3 wheels - they could run against a higher version
         // than the build config suggests.
-        use crate::once_cell::GILOnceCell;
+        use crate::sync::GILOnceCell;
         static IS_PYTHON_3_8: GILOnceCell<bool> = GILOnceCell::new();
 
         if *IS_PYTHON_3_8.get_or_init(_py, || _py.version_info() >= (3, 8)) {

src/impl_/pyclass/lazy_type_object.rs

@@ -1,16 +1,18 @@
 use std::{
     borrow::Cow,
+    cell::RefCell,
     ffi::CStr,
     marker::PhantomData,
     thread::{self, ThreadId},
 };
 
-use parking_lot::{const_mutex, Mutex};
-
 use crate::{
-    exceptions::PyRuntimeError, ffi, once_cell::GILOnceCell, pyclass::create_type_object,
-    types::PyType, AsPyPointer, IntoPyPointer, Py, PyClass, PyErr, PyMethodDefType, PyObject,
-    PyResult, Python,
+    exceptions::PyRuntimeError,
+    ffi,
+    pyclass::create_type_object,
+    sync::{GILOnceCell, GILProtected},
+    types::PyType,
+    AsPyPointer, IntoPyPointer, Py, PyClass, PyErr, PyMethodDefType, PyObject, PyResult, Python,
 };
 
 use super::PyClassItemsIter;
@@ -24,7 +26,7 @@ struct LazyTypeObjectInner {
     value: GILOnceCell<Py<PyType>>,
     // Threads which have begun initialization of the `tp_dict`. Used for
     // reentrant initialization detection.
-    initializing_threads: Mutex<Vec<ThreadId>>,
+    initializing_threads: GILProtected<RefCell<Vec<ThreadId>>>,
     tp_dict_filled: GILOnceCell<()>,
 }
 
@@ -34,7 +36,7 @@ impl<T> LazyTypeObject<T> {
         LazyTypeObject(
             LazyTypeObjectInner {
                 value: GILOnceCell::new(),
-                initializing_threads: const_mutex(Vec::new()),
+                initializing_threads: GILProtected::new(RefCell::new(Vec::new())),
                 tp_dict_filled: GILOnceCell::new(),
             },
             PhantomData,
@@ -109,7 +111,7 @@ impl LazyTypeObjectInner {
 
         let thread_id = thread::current().id();
         {
-            let mut threads = self.initializing_threads.lock();
+            let mut threads = self.initializing_threads.get(py).borrow_mut();
             if threads.contains(&thread_id) {
                 // Reentrant call: just return the type object, even if the
                 // `tp_dict` is not filled yet.
@@ -119,18 +121,20 @@ impl LazyTypeObjectInner {
         }
 
         struct InitializationGuard<'a> {
-            initializing_threads: &'a Mutex<Vec<ThreadId>>,
+            initializing_threads: &'a GILProtected<RefCell<Vec<ThreadId>>>,
+            py: Python<'a>,
             thread_id: ThreadId,
         }
         impl Drop for InitializationGuard<'_> {
             fn drop(&mut self) {
-                let mut threads = self.initializing_threads.lock();
+                let mut threads = self.initializing_threads.get(self.py).borrow_mut();
                 threads.retain(|id| *id != self.thread_id);
             }
         }
 
         let guard = InitializationGuard {
             initializing_threads: &self.initializing_threads,
+            py,
             thread_id,
         };
 
@@ -170,7 +174,7 @@ impl LazyTypeObjectInner {
             // Initialization successfully complete, can clear the thread list.
             // (No further calls to get_or_init() will try to init, on any thread.)
             std::mem::forget(guard);
-            *self.initializing_threads.lock() = Vec::new();
+            self.initializing_threads.get(py).replace(Vec::new());
             result
         });
 

src/lib.rs

@@ -409,7 +409,7 @@ mod instance;
 pub mod marker;
 pub mod marshal;
 #[macro_use]
-pub mod once_cell;
+pub mod sync;
 pub mod panic;
 pub mod prelude;
 pub mod pycell;
@@ -420,6 +420,15 @@ pub mod type_object;
 pub mod types;
 mod version;
 
+#[doc(hidden)]
+#[deprecated(since = "0.19.0", note = "Please use the `sync` module instead.")]
+pub mod once_cell {
+    // FIXME: We want to deprecate these,
+    // but that does not yet work for re-exports,
+    // c.f. https://github.com/rust-lang/rust/issues/30827
+    pub use crate::sync::{GILOnceCell, Interned};
+}
+
 pub use crate::conversions::*;
 
 #[cfg(feature = "macros")]

src/once_cell.rs → src/sync.rs

@@ -1,7 +1,46 @@
-//! A write-once cell mediated by the Python GIL.
+//! Synchronization mechanisms based on the Python GIL.
 use crate::{types::PyString, Py, Python};
 use std::cell::UnsafeCell;
 
+/// Value with concurrent access protected by the GIL.
+///
+/// This is a synchronization primitive based on Python's global interpreter lock (GIL).
+/// It ensures that only one thread at a time can access the inner value via shared references.
+/// It can be combined with interior mutability to obtain mutable references.
+///
+/// # Example
+///
+/// Combining `GILProtected` with `RefCell` enables mutable access to static data:
+///
+/// ```
+/// # use pyo3::prelude::*;
+/// use pyo3::sync::GILProtected;
+/// use std::cell::RefCell;
+///
+/// static NUMBERS: GILProtected<RefCell<Vec<i32>>> = GILProtected::new(RefCell::new(Vec::new()));
+///
+/// Python::with_gil(|py| {
+///     NUMBERS.get(py).borrow_mut().push(42);
+/// });
+/// ```
+pub struct GILProtected<T> {
+    value: T,
+}
+
+impl<T> GILProtected<T> {
+    /// Place the given value under the protection of the GIL.
+    pub const fn new(value: T) -> Self {
+        Self { value }
+    }
+
+    /// Gain access to the inner value by giving proof of having acquired the GIL.
+    pub fn get<'py>(&'py self, _py: Python<'py>) -> &'py T {
+        &self.value
+    }
+}
+
+unsafe impl<T> Sync for GILProtected<T> where T: Send {}
+
 /// A write-once cell similar to [`once_cell::OnceCell`](https://docs.rs/once_cell/latest/once_cell/).
 ///
 /// Unlike `once_cell::sync` which blocks threads to achieve thread safety, this implementation
@@ -25,7 +64,7 @@ use std::cell::UnsafeCell;
 /// between threads:
 ///
 /// ```
-/// use pyo3::once_cell::GILOnceCell;
+/// use pyo3::sync::GILOnceCell;
 /// use pyo3::prelude::*;
 /// use pyo3::types::PyList;
 ///
@@ -170,7 +209,7 @@ impl<T> GILOnceCell<T> {
 #[macro_export]
 macro_rules! intern {
     ($py: expr, $text: expr) => {{
-        static INTERNED: $crate::once_cell::Interned = $crate::once_cell::Interned::new($text);
+        static INTERNED: $crate::sync::Interned = $crate::sync::Interned::new($text);
         INTERNED.get($py)
     }};
 }

src/types/mapping.rs

@@ -1,7 +1,7 @@
 // Copyright (c) 2017-present PyO3 Project and Contributors
 
 use crate::err::{PyDowncastError, PyErr, PyResult};
-use crate::once_cell::GILOnceCell;
+use crate::sync::GILOnceCell;
 use crate::type_object::PyTypeInfo;
 use crate::types::{PyAny, PyDict, PySequence, PyType};
 use crate::{ffi, AsPyPointer, IntoPyPointer, Py, PyNativeType, PyTryFrom, Python, ToPyObject};

src/types/sequence.rs

@@ -4,7 +4,7 @@ use crate::exceptions::PyTypeError;
 #[cfg(feature = "experimental-inspect")]
 use crate::inspect::types::TypeInfo;
 use crate::internal_tricks::get_ssize_index;
-use crate::once_cell::GILOnceCell;
+use crate::sync::GILOnceCell;
 use crate::type_object::PyTypeInfo;
 use crate::types::{PyAny, PyList, PyString, PyTuple, PyType};
 use crate::{ffi, PyNativeType, ToPyObject};

tests/ui/invalid_pyfunctions.rs

@@ -9,4 +9,10 @@ fn impl_trait_function(impl_trait: impl AsRef<PyAny>) {}
 #[pyfunction]
 async fn async_function() {}
 
+#[pyfunction]
+fn wildcard_argument(_: i32) {}
+
+#[pyfunction]
+fn destructured_argument((a, b): (i32, i32)) {}
+
 fn main() {}

tests/ui/invalid_pyfunctions.stderr

@@ -17,3 +17,15 @@ error: `async fn` is not yet supported for Python functions.
    |
 10 | async fn async_function() {}
    | ^^^^^
+
+error: wildcard argument names are not supported
+  --> tests/ui/invalid_pyfunctions.rs:13:22
+   |
+13 | fn wildcard_argument(_: i32) {}
+   |                      ^
+
+error: destructuring in arguments is not supported
+  --> tests/ui/invalid_pyfunctions.rs:16:26
+   |
+16 | fn destructured_argument((a, b): (i32, i32)) {}
+   |                          ^^^^^^