deprecate `&PyModule` as `#[pymodule]` argument type (#3936)

* deprecate `&PyModule` as `#[pymodule]` argument type

* cleanup

* add ui tests

* fix deprecations in tests

* fix maturin and setuptools-rust starters

* run `deprecated` ui test only when `gil-refs` as disabled

examples/maturin-starter/src/lib.rs

@@ -20,7 +20,7 @@ impl ExampleClass {
 
 /// An example module implemented in Rust using PyO3.
 #[pymodule]
-fn maturin_starter(py: Python<'_>, m: &PyModule) -> PyResult<()> {
+fn maturin_starter(py: Python<'_>, m: &Bound<'_, PyModule>) -> PyResult<()> {
     m.add_class::<ExampleClass>()?;
     m.add_wrapped(wrap_pymodule!(submodule::submodule))?;
 

examples/setuptools-rust-starter/src/lib.rs

@@ -20,7 +20,7 @@ impl ExampleClass {
 
 /// An example module implemented in Rust using PyO3.
 #[pymodule]
-fn _setuptools_rust_starter(py: Python<'_>, m: &PyModule) -> PyResult<()> {
+fn _setuptools_rust_starter(py: Python<'_>, m: &Bound<'_, PyModule>) -> PyResult<()> {
     m.add_class::<ExampleClass>()?;
     m.add_wrapped(wrap_pymodule!(submodule::submodule))?;
 

guide/src/exception.md

@@ -44,7 +44,7 @@ use pyo3::exceptions::PyException;
 pyo3::create_exception!(mymodule, CustomError, PyException);
 
 #[pymodule]
-fn mymodule(py: Python<'_>, m: &PyModule) -> PyResult<()> {
+fn mymodule(py: Python<'_>, m: &Bound<'_, PyModule>) -> PyResult<()> {
     // ... other elements added to module ...
     m.add("CustomError", py.get_type_bound::<CustomError>())?;
 

guide/src/migration.md

@@ -847,7 +847,7 @@ fn my_module(_py: Python<'_>, m: &PyModule) -> PyResult<()> {
 
 To fix it, make the private submodule visible, e.g. with `pub` or `pub(crate)`.
 
-```rust
+```rust,ignore
 mod foo {
     use pyo3::prelude::*;
 

pyo3-macros-backend/src/module.rs

@@ -11,6 +11,7 @@ use quote::quote;
 use syn::{
     ext::IdentExt,
     parse::{Parse, ParseStream},
+    parse_quote, parse_quote_spanned,
     spanned::Spanned,
     token::Comma,
     Item, Path, Result,
@@ -281,6 +282,7 @@ pub fn pymodule_function_impl(mut function: syn::ItemFn) -> Result<TokenStream>
     let options = PyModuleOptions::from_attrs(&mut function.attrs)?;
     process_functions_in_module(&options, &mut function)?;
     let ctx = &Ctx::new(&options.krate);
+    let stmts = std::mem::take(&mut function.block.stmts);
     let Ctx { pyo3_path } = ctx;
     let ident = &function.sig.ident;
     let vis = &function.vis;
@@ -295,6 +297,29 @@ pub fn pymodule_function_impl(mut function: syn::ItemFn) -> Result<TokenStream>
     }
     module_args.push(quote!(::std::convert::Into::into(BoundRef(module))));
 
+    let extractors = function
+        .sig
+        .inputs
+        .iter()
+        .filter_map(|param| {
+            if let syn::FnArg::Typed(pat_type) = param {
+                if let syn::Pat::Ident(pat_ident) = &*pat_type.pat {
+                    let ident = &pat_ident.ident;
+                    return Some([
+                        parse_quote! { let (#ident, e) = #pyo3_path::impl_::pymethods::inspect_type(#ident); },
+                        parse_quote_spanned! { pat_type.span() => e.extract_gil_ref(); },
+                    ]);
+                }
+            }
+            None
+        })
+        .flatten();
+
+    function.block.stmts = extractors.chain(stmts).collect();
+    function
+        .attrs
+        .push(parse_quote!(#[allow(clippy::used_underscore_binding)]));
+
     Ok(quote! {
         #function
         #vis mod #ident {

pytests/src/lib.rs

@@ -18,7 +18,7 @@ pub mod sequence;
 pub mod subclassing;
 
 #[pymodule]
-fn pyo3_pytests(py: Python<'_>, m: &PyModule) -> PyResult<()> {
+fn pyo3_pytests(py: Python<'_>, m: &Bound<'_, PyModule>) -> PyResult<()> {
     m.add_wrapped(wrap_pymodule!(awaitable::awaitable))?;
     #[cfg(not(Py_LIMITED_API))]
     m.add_wrapped(wrap_pymodule!(buf_and_str::buf_and_str))?;

src/impl_/pymethods.rs

@@ -580,3 +580,43 @@ pub unsafe fn tp_new_impl<T: PyClass>(
         .create_class_object_of_type(py, target_type)
         .map(Bound::into_ptr)
 }
+
+pub fn inspect_type<T>(t: T) -> (T, Extractor<T>) {
+    (t, Extractor::new())
+}
+
+pub struct Extractor<T>(NotAGilRef<T>);
+pub struct NotAGilRef<T>(std::marker::PhantomData<T>);
+
+pub trait IsGilRef {}
+
+impl<T: crate::PyNativeType> IsGilRef for &'_ T {}
+
+impl<T> Extractor<T> {
+    #[allow(clippy::new_without_default)]
+    pub fn new() -> Self {
+        Extractor(NotAGilRef(std::marker::PhantomData))
+    }
+}
+
+impl<T: IsGilRef> Extractor<T> {
+    #[cfg_attr(
+        not(feature = "gil-refs"),
+        deprecated(
+            since = "0.21.0",
+            note = "use `&Bound<'_, T>` instead for this function argument"
+        )
+    )]
+    pub fn extract_gil_ref(&self) {}
+}
+
+impl<T> NotAGilRef<T> {
+    pub fn extract_gil_ref(&self) {}
+}
+
+impl<T> std::ops::Deref for Extractor<T> {
+    type Target = NotAGilRef<T>;
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}

src/tests/hygiene/pymodule.rs

@@ -7,12 +7,14 @@ fn do_something(x: i32) -> crate::PyResult<i32> {
     ::std::result::Result::Ok(x)
 }
 
+#[allow(deprecated)]
 #[crate::pymodule]
 #[pyo3(crate = "crate")]
 fn foo(_py: crate::Python<'_>, _m: &crate::types::PyModule) -> crate::PyResult<()> {
     ::std::result::Result::Ok(())
 }
 
+#[allow(deprecated)]
 #[crate::pymodule]
 #[pyo3(crate = "crate")]
 fn my_module(_py: crate::Python<'_>, m: &crate::types::PyModule) -> crate::PyResult<()> {

src/types/module.rs

@@ -335,11 +335,11 @@ impl PyModule {
     /// use pyo3::prelude::*;
     ///
     /// #[pymodule]
-    /// fn my_module(py: Python<'_>, module: &PyModule) -> PyResult<()> {
+    /// fn my_module(py: Python<'_>, module: &Bound<'_, PyModule>) -> PyResult<()> {
     ///     let submodule = PyModule::new_bound(py, "submodule")?;
     ///     submodule.add("super_useful_constant", "important")?;
     ///
-    ///     module.add_submodule(submodule.as_gil_ref())?;
+    ///     module.add_submodule(&submodule)?;
     ///     Ok(())
     /// }
     /// ```
@@ -530,11 +530,11 @@ pub trait PyModuleMethods<'py>: crate::sealed::Sealed {
     /// use pyo3::prelude::*;
     ///
     /// #[pymodule]
-    /// fn my_module(py: Python<'_>, module: &PyModule) -> PyResult<()> {
+    /// fn my_module(py: Python<'_>, module: &Bound<'_, PyModule>) -> PyResult<()> {
     ///     let submodule = PyModule::new_bound(py, "submodule")?;
     ///     submodule.add("super_useful_constant", "important")?;
     ///
-    ///     module.add_submodule(submodule.as_gil_ref())?;
+    ///     module.add_submodule(&submodule)?;
     ///     Ok(())
     /// }
     /// ```

tests/test_compile_error.rs

@@ -18,6 +18,7 @@ fn test_compile_errors() {
     t.compile_fail("tests/ui/invalid_pymethod_names.rs");
     t.compile_fail("tests/ui/invalid_pymodule_args.rs");
     t.compile_fail("tests/ui/reject_generics.rs");
+    #[cfg(not(feature = "gil-refs"))]
     t.compile_fail("tests/ui/deprecations.rs");
     t.compile_fail("tests/ui/invalid_closure.rs");
     t.compile_fail("tests/ui/pyclass_send.rs");

tests/test_module.rs

@@ -118,7 +118,7 @@ fn test_module_with_functions() {
 
 /// This module uses a legacy two-argument module function.
 #[pymodule]
-fn module_with_explicit_py_arg(_py: Python<'_>, m: &PyModule) -> PyResult<()> {
+fn module_with_explicit_py_arg(_py: Python<'_>, m: &Bound<'_, PyModule>) -> PyResult<()> {
     m.add_function(wrap_pyfunction!(double, m)?)?;
     Ok(())
 }

tests/test_no_imports.rs

@@ -10,6 +10,7 @@ fn basic_function(py: pyo3::Python<'_>, x: Option<pyo3::PyObject>) -> pyo3::PyOb
     x.unwrap_or_else(|| py.None())
 }
 
+#[allow(deprecated)]
 #[pyo3::pymodule]
 fn basic_module(_py: pyo3::Python<'_>, m: &pyo3::types::PyModule) -> pyo3::PyResult<()> {
     #[pyfn(m)]

tests/ui/deprecations.rs

@@ -14,3 +14,38 @@ impl MyClass {
 }
 
 fn main() {}
+
+#[pyfunction]
+fn double(x: usize) -> usize {
+    x * 2
+}
+
+#[pymodule]
+fn module_gil_ref(m: &PyModule) -> PyResult<()> {
+    m.add_function(wrap_pyfunction!(double, m)?)?;
+    Ok(())
+}
+
+#[pymodule]
+fn module_gil_ref_with_explicit_py_arg(_py: Python<'_>, m: &PyModule) -> PyResult<()> {
+    m.add_function(wrap_pyfunction!(double, m)?)?;
+    Ok(())
+}
+
+#[pymodule]
+fn module_bound(m: &Bound<'_, PyModule>) -> PyResult<()> {
+    m.add_function(wrap_pyfunction!(double, m)?)?;
+    Ok(())
+}
+
+#[pymodule]
+fn module_bound_with_explicit_py_arg(_py: Python<'_>, m: &Bound<'_, PyModule>) -> PyResult<()> {
+    m.add_function(wrap_pyfunction!(double, m)?)?;
+    Ok(())
+}
+
+#[pymodule]
+fn module_bound_by_value(m: Bound<'_, PyModule>) -> PyResult<()> {
+    m.add_function(wrap_pyfunction!(double, &m)?)?;
+    Ok(())
+}

tests/ui/deprecations.stderr

@@ -9,3 +9,15 @@ note: the lint level is defined here
    |
 1  | #![deny(deprecated)]
    |         ^^^^^^^^^^
+
+error: use of deprecated method `pyo3::methods::Extractor::<T>::extract_gil_ref`: use `&Bound<'_, T>` instead for this function argument
+  --> tests/ui/deprecations.rs:24:19
+   |
+24 | fn module_gil_ref(m: &PyModule) -> PyResult<()> {
+   |                   ^
+
+error: use of deprecated method `pyo3::methods::Extractor::<T>::extract_gil_ref`: use `&Bound<'_, T>` instead for this function argument
+  --> tests/ui/deprecations.rs:30:57
+   |
+30 | fn module_gil_ref_with_explicit_py_arg(_py: Python<'_>, m: &PyModule) -> PyResult<()> {
+   |                                                         ^