diff --git a/newsfragments/5525.added.md b/newsfragments/5525.added.md new file mode 100644 index 00000000000..3262953febc --- /dev/null +++ b/newsfragments/5525.added.md @@ -0,0 +1 @@ +added PEP-489 multi-phase initialization for pymodules diff --git a/pyo3-macros-backend/src/module.rs b/pyo3-macros-backend/src/module.rs index 831bd677b92..8eef9d57876 100644 --- a/pyo3-macros-backend/src/module.rs +++ b/pyo3-macros-backend/src/module.rs @@ -15,7 +15,7 @@ use crate::{ get_doc, pyclass::PyClassPyO3Option, pyfunction::{impl_wrap_pyfunction, PyFunctionOptions}, - utils::{has_attribute, has_attribute_with_namespace, Ctx, IdentOrStr}, + utils::{has_attribute, has_attribute_with_namespace, Ctx, IdentOrStr, PythonDoc}, }; use proc_macro2::{Span, TokenStream}; use quote::quote; @@ -389,23 +389,15 @@ pub fn pymodule_module_impl( #[cfg(not(feature = "experimental-inspect"))] let introspection_id = quote! {}; - let module_def = quote! {{ - use #pyo3_path::impl_::pymodule as impl_; - const INITIALIZER: impl_::ModuleInitializer = impl_::ModuleInitializer(__pyo3_pymodule); - unsafe { - impl_::ModuleDef::new( - __PYO3_NAME, - #doc, - INITIALIZER - ) - } - }}; + let gil_used = options.gil_used.is_some_and(|op| op.value.value); + let initialization = module_initialization( &name, ctx, - module_def, + quote! { __pyo3_pymodule }, options.submodule.is_some(), - options.gil_used.is_none_or(|op| op.value.value), + gil_used, + doc, ); let module_consts_names = module_consts.iter().map(|i| i.unraw().to_string()); @@ -456,12 +448,15 @@ pub fn pymodule_function_impl( let vis = &function.vis; let doc = get_doc(&function.attrs, None, ctx)?; + let gil_used = options.gil_used.is_some_and(|op| op.value.value); + let initialization = module_initialization( &name, ctx, - quote! { MakeDef::make_def() }, + quote! { ModuleExec::__pyo3_module_exec }, false, - options.gil_used.is_none_or(|op| op.value.value), + gil_used, + doc, ); #[cfg(feature = "experimental-inspect")] @@ -495,20 +490,9 @@ pub fn pymodule_function_impl( // (and `super` doesn't always refer to the outer scope, e.g. if the `#[pymodule] is // inside a function body) #[allow(unknown_lints, non_local_definitions)] - impl #ident::MakeDef { - const fn make_def() -> #pyo3_path::impl_::pymodule::ModuleDef { - fn __pyo3_pymodule(module: &#pyo3_path::Bound<'_, #pyo3_path::types::PyModule>) -> #pyo3_path::PyResult<()> { - #ident(#(#module_args),*) - } - - const INITIALIZER: #pyo3_path::impl_::pymodule::ModuleInitializer = #pyo3_path::impl_::pymodule::ModuleInitializer(__pyo3_pymodule); - unsafe { - #pyo3_path::impl_::pymodule::ModuleDef::new( - #ident::__PYO3_NAME, - #doc, - INITIALIZER - ) - } + impl #ident::ModuleExec { + fn __pyo3_module_exec(module: &#pyo3_path::Bound<'_, #pyo3_path::types::PyModule>) -> #pyo3_path::PyResult<()> { + #ident(#(#module_args),*) } } }) @@ -517,9 +501,10 @@ pub fn pymodule_function_impl( fn module_initialization( name: &syn::Ident, ctx: &Ctx, - module_def: TokenStream, + module_exec: TokenStream, is_submodule: bool, gil_used: bool, + doc: PythonDoc, ) -> TokenStream { let Ctx { pyo3_path, .. } = ctx; let pyinit_symbol = format!("PyInit_{name}"); @@ -530,9 +515,27 @@ fn module_initialization( #[doc(hidden)] pub const __PYO3_NAME: &'static ::std::ffi::CStr = #pyo3_name; - pub(super) struct MakeDef; + // This structure exists for `fn` modules declared within `fn` bodies, where due to the hidden + // module (used for importing) the `fn` to initialize the module cannot be seen from the #module_def + // declaration just below. #[doc(hidden)] - pub static _PYO3_DEF: #pyo3_path::impl_::pymodule::ModuleDef = #module_def; + pub(super) struct ModuleExec; + + #[doc(hidden)] + pub static _PYO3_DEF: #pyo3_path::impl_::pymodule::ModuleDef = { + use #pyo3_path::impl_::pymodule as impl_; + + unsafe extern "C" fn __pyo3_module_exec(module: *mut #pyo3_path::ffi::PyObject) -> ::std::os::raw::c_int { + #pyo3_path::impl_::trampoline::module_exec(module, #module_exec) + } + + static SLOTS: impl_::PyModuleSlots<4> = impl_::PyModuleSlotsBuilder::new() + .with_mod_exec(__pyo3_module_exec) + .with_gil_used(#gil_used) + .build(); + + impl_::ModuleDef::new(__PYO3_NAME, #doc, &SLOTS) + }; #[doc(hidden)] // so wrapped submodules can see what gil_used is pub static __PYO3_GIL_USED: bool = #gil_used; @@ -544,7 +547,10 @@ fn module_initialization( #[doc(hidden)] #[export_name = #pyinit_symbol] pub unsafe extern "C" fn __pyo3_init() -> *mut #pyo3_path::ffi::PyObject { - unsafe { #pyo3_path::impl_::trampoline::module_init(|py| _PYO3_DEF.make_module(py, #gil_used)) } + _PYO3_DEF.init_multi_phase( + unsafe { #pyo3_path::Python::assume_attached() }, + #gil_used + ) } }); } diff --git a/pytests/tests/test_misc.py b/pytests/tests/test_misc.py index dd7f8007e81..117af8bf419 100644 --- a/pytests/tests/test_misc.py +++ b/pytests/tests/test_misc.py @@ -24,6 +24,7 @@ def test_multiple_imports_same_interpreter_ok(): spec = importlib.util.find_spec("pyo3_pytests.pyo3_pytests") module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) assert dir(module) == dir(pyo3_pytests.pyo3_pytests) diff --git a/src/impl_/pymodule.rs b/src/impl_/pymodule.rs index 7151ad3bad3..1ad5a4f499f 100644 --- a/src/impl_/pymodule.rs +++ b/src/impl_/pymodule.rs @@ -1,6 +1,11 @@ //! Implementation details of `#[pymodule]` which need to be accessible from proc-macro generated code. -use std::{cell::UnsafeCell, ffi::CStr, marker::PhantomData}; +use std::{ + cell::UnsafeCell, + ffi::CStr, + marker::PhantomData, + os::raw::{c_int, c_void}, +}; #[cfg(all( not(any(PyPy, GraalPy)), @@ -15,27 +20,24 @@ use portable_atomic::AtomicI64; not(all(windows, Py_LIMITED_API, not(Py_3_10))), target_has_atomic = "64", ))] -use std::sync::atomic::AtomicI64; -use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::atomic::{AtomicI64, Ordering}; #[cfg(not(any(PyPy, GraalPy)))] use crate::exceptions::PyImportError; use crate::prelude::PyTypeMethods; -#[cfg(all(not(Py_LIMITED_API), Py_GIL_DISABLED))] -use crate::PyErr; use crate::{ ffi, impl_::pyfunction::PyFunctionDef, sync::PyOnceLock, - types::{PyModule, PyModuleMethods}, - Bound, Py, PyClass, PyResult, PyTypeInfo, Python, + types::{any::PyAnyMethods, dict::PyDictMethods, PyDict, PyModule, PyModuleMethods}, + Bound, Py, PyAny, PyClass, PyResult, PyTypeInfo, Python, }; +use crate::{ffi_ptr_ext::FfiPtrExt, PyErr}; /// `Sync` wrapper of `ffi::PyModuleDef`. pub struct ModuleDef { // wrapped in UnsafeCell so that Rust compiler treats this as interior mutability ffi_def: UnsafeCell, - initializer: ModuleInitializer, /// Interpreter ID where module was initialized (not applicable on PyPy). #[cfg(all( not(any(PyPy, GraalPy)), @@ -46,20 +48,20 @@ pub struct ModuleDef { /// Initialized module object, cached to avoid reinitialization. module: PyOnceLock>, /// Whether or not the module supports running without the GIL - gil_used: AtomicBool, + gil_used: bool, } -/// Wrapper to enable initializer to be used in const fns. -pub struct ModuleInitializer(pub for<'py> fn(&Bound<'py, PyModule>) -> PyResult<()>); - unsafe impl Sync for ModuleDef {} impl ModuleDef { /// Make new module definition with given module name. - pub const unsafe fn new( + pub const fn new( name: &'static CStr, doc: &'static CStr, - initializer: ModuleInitializer, + // TODO: it might be nice to make this unsized and not need the + // const N generic parameter, however that might need unsized return values + // or other messy hacks. + slots: &'static PyModuleSlots, ) -> Self { #[allow(clippy::declare_interior_mutable_const)] const INIT: ffi::PyModuleDef = ffi::PyModuleDef { @@ -77,12 +79,14 @@ impl ModuleDef { let ffi_def = UnsafeCell::new(ffi::PyModuleDef { m_name: name.as_ptr(), m_doc: doc.as_ptr(), + // TODO: would be slightly nicer to use `[T]::as_mut_ptr()` here, + // but that requires mut ptr deref on MSRV. + m_slots: slots.0.get() as _, ..INIT }); ModuleDef { ffi_def, - initializer, // -1 is never expected to be a valid interpreter ID #[cfg(all( not(any(PyPy, GraalPy)), @@ -91,16 +95,24 @@ impl ModuleDef { ))] interpreter: AtomicI64::new(-1), module: PyOnceLock::new(), - gil_used: AtomicBool::new(true), + gil_used: false, } } - /// Builds a module using user given initializer. Used for [`#[pymodule]`][crate::pymodule]. + + pub fn init_multi_phase(&'static self, _py: Python<'_>, _gil_used: bool) -> *mut ffi::PyObject { + unsafe { ffi::PyModuleDef_Init(self.ffi_def.get()) } + } + + /// Builds a module object directly. Used for [`#[pymodule]`][crate::pymodule] submodules. #[cfg_attr(any(Py_LIMITED_API, not(Py_GIL_DISABLED)), allow(unused_variables))] - pub fn make_module(&'static self, py: Python<'_>, gil_used: bool) -> PyResult> { + pub fn make_module(&'static self, py: Python<'_>, _gil_used: bool) -> PyResult> { // Check the interpreter ID has not changed, since we currently have no way to guarantee // that static data is not reused across interpreters. // // PyPy does not have subinterpreters, so no need to check interpreter ID. + // + // TODO: it should be possible to use the Py_mod_multiple_interpreters slot on sufficiently + // new Python versions to remove the need for this custom logic #[cfg(not(any(PyPy, GraalPy)))] { // PyInterpreterState_Get is only available on 3.9 and later, but is missing @@ -134,34 +146,116 @@ impl ModuleDef { } } } + + // Make a dummy spec, needs a `name` attribute and that seems to be sufficient + // for the loader system + + static SIMPLE_NAMESPACE: PyOnceLock> = PyOnceLock::new(); + let simple_ns = SIMPLE_NAMESPACE.import(py, "types", "SimpleNamespace")?; + + let ffi_def = self.ffi_def.get(); + + let name = unsafe { CStr::from_ptr((*ffi_def).m_name).to_str()? }.to_string(); + let kwargs = PyDict::new(py); + kwargs.set_item("name", name)?; + let spec = simple_ns.call((), Some(&kwargs))?; + self.module .get_or_try_init(py, || { + let def = self.ffi_def.get(); let module = unsafe { - Py::::from_owned_ptr_or_err( - py, - ffi::PyModule_Create(self.ffi_def.get()), - )? - }; - #[cfg(all(not(Py_LIMITED_API), Py_GIL_DISABLED))] - { - let gil_used_ptr = { - if gil_used { - ffi::Py_MOD_GIL_USED - } else { - ffi::Py_MOD_GIL_NOT_USED - } - }; - if unsafe { ffi::PyUnstable_Module_SetGIL(module.as_ptr(), gil_used_ptr) } < 0 { - return Err(PyErr::fetch(py)); - } + ffi::PyModule_FromDefAndSpec(def, spec.as_ptr()).assume_owned_or_err(py)? } - self.initializer.0(module.bind(py))?; - Ok(module) + .cast_into()?; + if unsafe { ffi::PyModule_ExecDef(module.as_ptr(), def) } != 0 { + return Err(PyErr::fetch(py)); + } + Ok(module.unbind()) }) .map(|py_module| py_module.clone_ref(py)) } } +/// Type of the exec slot used to initialise module contents +pub type ModuleExecSlot = unsafe extern "C" fn(*mut ffi::PyObject) -> c_int; + +/// Builder to create `PyModuleSlots`. The size of the number of slots desired must +/// be known up front, and N needs to be at least one greater than the number of +/// actual slots pushed due to the need to have a zeroed element on the end. +pub struct PyModuleSlotsBuilder { + // values (initially all zeroed) + values: [ffi::PyModuleDef_Slot; N], + // current length + len: usize, +} + +impl PyModuleSlotsBuilder { + #[allow(clippy::new_without_default)] + pub const fn new() -> Self { + Self { + values: [unsafe { std::mem::zeroed() }; N], + len: 0, + } + } + + pub const fn with_mod_exec(self, exec: ModuleExecSlot) -> Self { + self.push(ffi::Py_mod_exec, exec as *mut c_void) + } + + pub const fn with_gil_used(self, gil_used: bool) -> Self { + #[cfg(Py_3_13)] + { + self.push( + ffi::Py_mod_gil, + if gil_used { + ffi::Py_MOD_GIL_USED + } else { + ffi::Py_MOD_GIL_NOT_USED + }, + ) + } + + #[cfg(not(Py_3_13))] + { + // Silence unused variable warning + let _ = gil_used; + + // Py_mod_gil didn't exist before 3.13, can just make + // this function a noop. + // + // By handling it here we can avoid conditional + // compilation within the macros; they can always emit + // a `.with_gil_used()` call. + self + } + } + + pub const fn build(self) -> PyModuleSlots { + // Required to guarantee there's still a zeroed element + // at the end + assert!( + self.len < N, + "N must be greater than the number of slots pushed" + ); + PyModuleSlots(UnsafeCell::new(self.values)) + } + + const fn push(mut self, slot: c_int, value: *mut c_void) -> Self { + self.values[self.len] = ffi::PyModuleDef_Slot { slot, value }; + self.len += 1; + self + } +} + +/// Wrapper to safely store module slots, to be used in a `ModuleDef`. +pub struct PyModuleSlots(UnsafeCell<[ffi::PyModuleDef_Slot; N]>); + +// It might be possible to avoid this with SyncUnsafeCell in the future +// +// SAFETY: the inner values are only accessed within a `ModuleDef`, +// which only uses them to build the `ffi::ModuleDef`. +unsafe impl Sync for PyModuleSlots {} + /// Trait to add an element (class, function...) to a module. /// /// Currently only implemented for classes. @@ -214,7 +308,7 @@ impl PyAddToModule for PyFunctionDef { impl PyAddToModule for ModuleDef { fn add_to_module(&'static self, module: &Bound<'_, PyModule>) -> PyResult<()> { module.add_submodule( - self.make_module(module.py(), self.gil_used.load(Ordering::Relaxed))? + self.make_module(module.py(), self.gil_used)? .bind(module.py()), ) } @@ -222,31 +316,36 @@ impl PyAddToModule for ModuleDef { #[cfg(test)] mod tests { - use std::{ - borrow::Cow, - ffi::CStr, - sync::atomic::{AtomicBool, Ordering}, - }; + use std::{borrow::Cow, ffi::CStr, os::raw::c_int}; use crate::{ - types::{any::PyAnyMethods, module::PyModuleMethods, PyModule}, - Bound, PyResult, Python, + ffi, + impl_::{ + pymodule::{PyModuleSlots, PyModuleSlotsBuilder}, + trampoline, + }, + types::{any::PyAnyMethods, module::PyModuleMethods}, + Python, }; - use super::{ModuleDef, ModuleInitializer}; + use super::ModuleDef; #[test] fn module_init() { - static MODULE_DEF: ModuleDef = unsafe { - ModuleDef::new( - c"test_module", - c"some doc", - ModuleInitializer(|m| { + unsafe extern "C" fn module_exec(module: *mut ffi::PyObject) -> c_int { + unsafe { + trampoline::module_exec(module, |m| { m.add("SOME_CONSTANT", 42)?; Ok(()) - }), - ) - }; + }) + } + } + + static SLOTS: PyModuleSlots<2> = PyModuleSlotsBuilder::new() + .with_mod_exec(module_exec) + .build(); + static MODULE_DEF: ModuleDef = ModuleDef::new(c"test_module", c"some doc", &SLOTS); + Python::attach(|py| { let module = MODULE_DEF.make_module(py, false).unwrap().into_bound(py); assert_eq!( @@ -283,23 +382,36 @@ mod tests { static NAME: &CStr = c"test_module"; static DOC: &CStr = c"some doc"; - static INIT_CALLED: AtomicBool = AtomicBool::new(false); - - #[allow(clippy::unnecessary_wraps)] - fn init(_: &Bound<'_, PyModule>) -> PyResult<()> { - INIT_CALLED.store(true, Ordering::SeqCst); - Ok(()) - } + static SLOTS: PyModuleSlots<2> = PyModuleSlotsBuilder::new().build(); unsafe { - let module_def: ModuleDef = ModuleDef::new(NAME, DOC, ModuleInitializer(init)); + let module_def: ModuleDef = ModuleDef::new(NAME, DOC, &SLOTS); assert_eq!((*module_def.ffi_def.get()).m_name, NAME.as_ptr() as _); assert_eq!((*module_def.ffi_def.get()).m_doc, DOC.as_ptr() as _); + assert_eq!((*module_def.ffi_def.get()).m_slots, SLOTS.0.get().cast()); + } + } - Python::attach(|py| { - module_def.initializer.0(&py.import("builtins").unwrap()).unwrap(); - assert!(INIT_CALLED.load(Ordering::SeqCst)); - }) + #[test] + #[should_panic] + fn test_module_slots_builder_overflow() { + unsafe extern "C" fn module_exec(_module: *mut ffi::PyObject) -> c_int { + 0 } + + PyModuleSlotsBuilder::<0>::new().with_mod_exec(module_exec); + } + + #[test] + #[should_panic] + fn test_module_slots_builder_overflow_2() { + unsafe extern "C" fn module_exec(_module: *mut ffi::PyObject) -> c_int { + 0 + } + + PyModuleSlotsBuilder::<2>::new() + .with_mod_exec(module_exec) + .with_mod_exec(module_exec) + .build(); } } diff --git a/src/impl_/trampoline.rs b/src/impl_/trampoline.rs index 6bcf71c6ca7..91ef17eac12 100644 --- a/src/impl_/trampoline.rs +++ b/src/impl_/trampoline.rs @@ -12,14 +12,21 @@ use std::{ use crate::internal::state::AttachGuard; use crate::{ ffi, ffi_ptr_ext::FfiPtrExt, impl_::callback::PyCallbackOutput, impl_::panic::PanicTrap, - impl_::pymethods::IPowModulo, panic::PanicException, types::PyModule, Py, PyResult, Python, + impl_::pymethods::IPowModulo, panic::PanicException, types::PyModule, Bound, PyResult, Python, }; #[inline] -pub unsafe fn module_init( - f: for<'py> unsafe fn(Python<'py>) -> PyResult>, -) -> *mut ffi::PyObject { - unsafe { trampoline(|py| f(py).map(|module| module.into_ptr())) } +pub unsafe fn module_exec( + module: *mut ffi::PyObject, + f: for<'a, 'py> fn(&'a Bound<'py, PyModule>) -> PyResult<()>, +) -> c_int { + unsafe { + trampoline(|py| { + let module = module.assume_borrowed_or_err(py)?.cast::()?; + f(&module)?; + Ok(0) + }) + } } #[inline] diff --git a/tests/ui/invalid_property_args.stderr b/tests/ui/invalid_property_args.stderr index 047e6709c21..dc52fc31159 100644 --- a/tests/ui/invalid_property_args.stderr +++ b/tests/ui/invalid_property_args.stderr @@ -65,11 +65,11 @@ error[E0277]: `PhantomData` cannot be converted to a Python object &'a (T0, T1, T2, T3, T4) and $N others = note: required for `PhantomData` to implement `for<'py> PyO3GetField<'py>` -note: required by a bound in `PyClassGetterGenerator::::generate` +note: required by a bound in `PyClassGetterGenerator::::generate` --> src/impl_/pyclass.rs | | pub const fn generate(&self, name: &'static CStr, doc: &'static CStr) -> PyMethodDefType | -------- required by a bound in this associated function ... | for<'py> FieldT: PyO3GetField<'py>, - | ^^^^^^^^^^^^^^^^^ required by this bound in `PyClassGetterGenerator::::generate` + | ^^^^^^^^^^^^^^^^^ required by this bound in `PyClassGetterGenerator::::generate` diff --git a/tests/ui/invalid_result_conversion.stderr b/tests/ui/invalid_result_conversion.stderr index f782e00828f..ef31f073689 100644 --- a/tests/ui/invalid_result_conversion.stderr +++ b/tests/ui/invalid_result_conversion.stderr @@ -6,12 +6,12 @@ error[E0277]: the trait bound `PyErr: From` is not satisfied | = help: the following other types implement trait `From`: `PyErr` implements `From` + `PyErr` implements `From>` + `PyErr` implements `From>` `PyErr` implements `From` `PyErr` implements `From>` `PyErr` implements `From>` `PyErr` implements `From` `PyErr` implements `From` - `PyErr` implements `From` - `PyErr` implements `From>` and $N others = note: required for `MyError` to implement `Into`