From cf74624de9c88e51914c7d2a2dbae5c4d4a6e63d Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Fri, 29 Mar 2024 11:54:33 +0000 Subject: [PATCH] refactor to remove add_to_module functions from generated code (#4009) * refactor to remove add_to_module functions from generated code * mrsv, newsfragment * clippy --- newsfragments/4009.fixed.md | 1 + pyo3-macros-backend/src/module.rs | 14 +++---- pyo3-macros-backend/src/pyclass.rs | 8 ++-- pyo3-macros-backend/src/pyfunction.rs | 10 +---- src/impl_/pymodule.rs | 57 +++++++++++++++++++++++++-- src/macros.rs | 10 ++--- src/types/mod.rs | 13 ++---- tests/test_declarative_module.rs | 2 +- 8 files changed, 75 insertions(+), 40 deletions(-) create mode 100644 newsfragments/4009.fixed.md diff --git a/newsfragments/4009.fixed.md b/newsfragments/4009.fixed.md new file mode 100644 index 00000000..a5d378e1 --- /dev/null +++ b/newsfragments/4009.fixed.md @@ -0,0 +1 @@ +Fix some uncovered code blocks emitted by `#[pymodule]`, `#[pyfunction]` and `#[pyclass]` macros. diff --git a/pyo3-macros-backend/src/module.rs b/pyo3-macros-backend/src/module.rs index dc2b3bfc..080a279a 100644 --- a/pyo3-macros-backend/src/module.rs +++ b/pyo3-macros-backend/src/module.rs @@ -267,7 +267,7 @@ pub fn pymodule_module_impl(mut module: syn::ItemMod) -> Result { use #pyo3_path::impl_::pymodule::PyAddToModule; #( #(#module_items_cfg_attrs)* - #module_items::add_to_module(module)?; + #module_items::_PYO3_DEF.add_to_module(module)?; )* #pymodule_init Ok(()) @@ -358,21 +358,19 @@ fn module_initialization(options: PyModuleOptions, ident: &syn::Ident) -> TokenS let pyinit_symbol = format!("PyInit_{}", name); quote! { + #[doc(hidden)] pub const __PYO3_NAME: &'static str = concat!(stringify!(#name), "\0"); pub(super) struct MakeDef; - pub static DEF: #pyo3_path::impl_::pymodule::ModuleDef = MakeDef::make_def(); - - pub fn add_to_module(module: &#pyo3_path::Bound<'_, #pyo3_path::types::PyModule>) -> #pyo3_path::PyResult<()> { - use #pyo3_path::prelude::PyModuleMethods; - module.add_submodule(DEF.make_module(module.py())?.bind(module.py())) - } + #[doc(hidden)] + pub static _PYO3_DEF: #pyo3_path::impl_::pymodule::ModuleDef = MakeDef::make_def(); /// This autogenerated function is called by the python interpreter when importing /// the module. + #[doc(hidden)] #[export_name = #pyinit_symbol] pub unsafe extern "C" fn __pyo3_init() -> *mut #pyo3_path::ffi::PyObject { - #pyo3_path::impl_::trampoline::module_init(|py| DEF.make_module(py)) + #pyo3_path::impl_::trampoline::module_init(|py| _PYO3_DEF.make_module(py)) } } } diff --git a/pyo3-macros-backend/src/pyclass.rs b/pyo3-macros-backend/src/pyclass.rs index 5122d205..88cf9149 100644 --- a/pyo3-macros-backend/src/pyclass.rs +++ b/pyo3-macros-backend/src/pyclass.rs @@ -1620,11 +1620,9 @@ impl<'a> PyClassImplsBuilder<'a> { let Ctx { pyo3_path } = ctx; let cls = self.cls; quote! { - impl #pyo3_path::impl_::pymodule::PyAddToModule for #cls { - fn add_to_module(module: &#pyo3_path::Bound<'_, #pyo3_path::types::PyModule>) -> #pyo3_path::PyResult<()> { - use #pyo3_path::types::PyModuleMethods; - module.add_class::() - } + impl #cls { + #[doc(hidden)] + const _PYO3_DEF: #pyo3_path::impl_::pymodule::AddClassToModule = #pyo3_path::impl_::pymodule::AddClassToModule::new(); } } } diff --git a/pyo3-macros-backend/src/pyfunction.rs b/pyo3-macros-backend/src/pyfunction.rs index cce9f748..25d2536e 100644 --- a/pyo3-macros-backend/src/pyfunction.rs +++ b/pyo3-macros-backend/src/pyfunction.rs @@ -269,13 +269,7 @@ pub fn impl_wrap_pyfunction( #[doc(hidden)] #vis mod #name { pub(crate) struct MakeDef; - pub const DEF: #pyo3_path::impl_::pymethods::PyMethodDef = MakeDef::DEF; - - pub fn add_to_module(module: &#pyo3_path::Bound<'_, #pyo3_path::types::PyModule>) -> #pyo3_path::PyResult<()> { - use #pyo3_path::prelude::PyModuleMethods; - use ::std::convert::Into; - module.add_function(#pyo3_path::types::PyCFunction::internal_new(module.py(), &DEF, module.into())?) - } + pub const _PYO3_DEF: #pyo3_path::impl_::pymethods::PyMethodDef = MakeDef::_PYO3_DEF; } // Generate the definition inside an anonymous function in the same scope as the original function - @@ -283,7 +277,7 @@ pub fn impl_wrap_pyfunction( // (and `super` doesn't always refer to the outer scope, e.g. if the `#[pyfunction] is // inside a function body) impl #name::MakeDef { - const DEF: #pyo3_path::impl_::pymethods::PyMethodDef = #methoddef; + const _PYO3_DEF: #pyo3_path::impl_::pymethods::PyMethodDef = #methoddef; } #[allow(non_snake_case)] diff --git a/src/impl_/pymodule.rs b/src/impl_/pymodule.rs index ba13f7a9..20560aeb 100644 --- a/src/impl_/pymodule.rs +++ b/src/impl_/pymodule.rs @@ -1,6 +1,6 @@ //! Implementation details of `#[pymodule]` which need to be accessible from proc-macro generated code. -use std::cell::UnsafeCell; +use std::{cell::UnsafeCell, marker::PhantomData}; #[cfg(all( not(any(PyPy, GraalPy)), @@ -11,7 +11,12 @@ use portable_atomic::{AtomicI64, Ordering}; #[cfg(not(any(PyPy, GraalPy)))] use crate::exceptions::PyImportError; -use crate::{ffi, sync::GILOnceCell, types::PyModule, Bound, Py, PyResult, Python}; +use crate::{ + ffi, + sync::GILOnceCell, + types::{PyCFunction, PyModule, PyModuleMethods}, + Bound, Py, PyClass, PyMethodDef, PyResult, PyTypeInfo, Python, +}; /// `Sync` wrapper of `ffi::PyModuleDef`. pub struct ModuleDef { @@ -149,7 +154,53 @@ impl ModuleDef { /// /// Currently only implemented for classes. pub trait PyAddToModule { - fn add_to_module(module: &Bound<'_, PyModule>) -> PyResult<()>; + fn add_to_module(&'static self, module: &Bound<'_, PyModule>) -> PyResult<()>; +} + +/// For adding native types (non-pyclass) to a module. +pub struct AddTypeToModule(PhantomData); + +impl AddTypeToModule { + #[allow(clippy::new_without_default)] + pub const fn new() -> Self { + AddTypeToModule(PhantomData) + } +} + +impl PyAddToModule for AddTypeToModule { + fn add_to_module(&'static self, module: &Bound<'_, PyModule>) -> PyResult<()> { + module.add(T::NAME, T::type_object_bound(module.py())) + } +} + +/// For adding a class to a module. +pub struct AddClassToModule(PhantomData); + +impl AddClassToModule { + #[allow(clippy::new_without_default)] + pub const fn new() -> Self { + AddClassToModule(PhantomData) + } +} + +impl PyAddToModule for AddClassToModule { + fn add_to_module(&'static self, module: &Bound<'_, PyModule>) -> PyResult<()> { + module.add_class::() + } +} + +/// For adding a function to a module. +impl PyAddToModule for PyMethodDef { + fn add_to_module(&'static self, module: &Bound<'_, PyModule>) -> PyResult<()> { + module.add_function(PyCFunction::internal_new(module.py(), self, Some(module))?) + } +} + +/// For adding a module to a module. +impl PyAddToModule for ModuleDef { + fn add_to_module(&'static self, module: &Bound<'_, PyModule>) -> PyResult<()> { + module.add_submodule(self.make_module(module.py())?.bind(module.py())) + } } #[cfg(test)] diff --git a/src/macros.rs b/src/macros.rs index 09e2acfb..0267c266 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -138,7 +138,7 @@ macro_rules! wrap_pyfunction { use $function as wrapped_pyfunction; $crate::impl_::pyfunction::WrapPyFunctionArg::wrap_pyfunction( py_or_module, - &wrapped_pyfunction::DEF, + &wrapped_pyfunction::_PYO3_DEF, ) } }; @@ -150,7 +150,7 @@ macro_rules! wrap_pyfunction { check_gil_refs.is_python(); $crate::impl_::pyfunction::WrapPyFunctionArg::wrap_pyfunction( py_or_module, - &wrapped_pyfunction::DEF, + &wrapped_pyfunction::_PYO3_DEF, ) }}; } @@ -166,7 +166,7 @@ macro_rules! wrap_pyfunction_bound { use $function as wrapped_pyfunction; $crate::impl_::pyfunction::WrapPyFunctionArg::wrap_pyfunction( $crate::impl_::pyfunction::OnlyBound(py_or_module), - &wrapped_pyfunction::DEF, + &wrapped_pyfunction::_PYO3_DEF, ) } }; @@ -174,7 +174,7 @@ macro_rules! wrap_pyfunction_bound { use $function as wrapped_pyfunction; $crate::impl_::pyfunction::WrapPyFunctionArg::wrap_pyfunction( $crate::impl_::pyfunction::OnlyBound($py_or_module), - &wrapped_pyfunction::DEF, + &wrapped_pyfunction::_PYO3_DEF, ) }}; } @@ -189,7 +189,7 @@ macro_rules! wrap_pymodule { ($module:path) => { &|py| { use $module as wrapped_pymodule; - wrapped_pymodule::DEF + wrapped_pymodule::_PYO3_DEF .make_module(py) .expect("failed to wrap pymodule") } diff --git a/src/types/mod.rs b/src/types/mod.rs index 5448999d..a03d01b3 100644 --- a/src/types/mod.rs +++ b/src/types/mod.rs @@ -250,16 +250,9 @@ macro_rules! pyobject_native_type_info( )? } - impl<$($generics,)*> $crate::impl_::pymodule::PyAddToModule for $name { - fn add_to_module( - module: &$crate::Bound<'_, $crate::types::PyModule>, - ) -> $crate::PyResult<()> { - use $crate::types::PyModuleMethods; - module.add( - ::NAME, - ::type_object_bound(module.py()), - ) - } + impl $name { + #[doc(hidden)] + const _PYO3_DEF: $crate::impl_::pymodule::AddTypeToModule = $crate::impl_::pymodule::AddTypeToModule::new(); } }; ); diff --git a/tests/test_declarative_module.rs b/tests/test_declarative_module.rs index 9eea8e2d..5f8e2c9f 100644 --- a/tests/test_declarative_module.rs +++ b/tests/test_declarative_module.rs @@ -145,7 +145,7 @@ mod class_initialization_module { #[cfg(not(Py_LIMITED_API))] fn test_class_initialization_fails() { Python::with_gil(|py| { - let err = class_initialization_module::DEF + let err = class_initialization_module::_PYO3_DEF .make_module(py) .unwrap_err(); assert_eq!(