From 554478eb53a526a62c0138502f3963f9be22da7b Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Wed, 10 Jul 2024 23:38:38 +0100 Subject: [PATCH] allow `#[pymodule(...)]` to accept all relevant `#[pyo3(...)]` options (#4330) --- examples/getitem/src/lib.rs | 3 +- guide/src/module.md | 3 +- newsfragments/4330.changed.md | 1 + pyo3-macros-backend/src/module.rs | 121 +++++++++--------- pyo3-macros/src/lib.rs | 44 +++---- tests/test_declarative_module.rs | 6 +- tests/test_module.rs | 3 +- tests/ui/empty.rs | 1 + tests/ui/invalid_pymodule_args.stderr | 2 +- tests/ui/invalid_pymodule_glob.rs | 2 + tests/ui/invalid_pymodule_glob.stderr | 4 +- tests/ui/invalid_pymodule_in_root.rs | 1 + tests/ui/invalid_pymodule_in_root.stderr | 8 +- tests/ui/invalid_pymodule_trait.stderr | 6 + .../ui/invalid_pymodule_two_pymodule_init.rs | 6 +- .../invalid_pymodule_two_pymodule_init.stderr | 4 +- 16 files changed, 108 insertions(+), 107 deletions(-) create mode 100644 newsfragments/4330.changed.md create mode 100644 tests/ui/empty.rs diff --git a/examples/getitem/src/lib.rs b/examples/getitem/src/lib.rs index ce162a70bf9..ba850a06b8d 100644 --- a/examples/getitem/src/lib.rs +++ b/examples/getitem/src/lib.rs @@ -75,8 +75,7 @@ impl ExampleContainer { } } -#[pymodule] -#[pyo3(name = "getitem")] +#[pymodule(name = "getitem")] fn example(m: &Bound<'_, PyModule>) -> PyResult<()> { // ? -https://github.com/PyO3/maturin/issues/475 m.add_class::()?; diff --git a/guide/src/module.md b/guide/src/module.md index a2cb8b37a05..af6da0c916f 100644 --- a/guide/src/module.md +++ b/guide/src/module.md @@ -31,8 +31,7 @@ fn double(x: usize) -> usize { x * 2 } -#[pymodule] -#[pyo3(name = "custom_name")] +#[pymodule(name = "custom_name")] fn my_extension(m: &Bound<'_, PyModule>) -> PyResult<()> { m.add_function(wrap_pyfunction!(double, m)?) } diff --git a/newsfragments/4330.changed.md b/newsfragments/4330.changed.md new file mode 100644 index 00000000000..d465ec99cec --- /dev/null +++ b/newsfragments/4330.changed.md @@ -0,0 +1 @@ +`#[pymodule(...)]` now directly accepts all relevant `#[pyo3(...)]` options. diff --git a/pyo3-macros-backend/src/module.rs b/pyo3-macros-backend/src/module.rs index 2ca084a6a4b..cd6340eef88 100644 --- a/pyo3-macros-backend/src/module.rs +++ b/pyo3-macros-backend/src/module.rs @@ -2,8 +2,8 @@ use crate::{ attributes::{ - self, take_attributes, take_pyo3_options, CrateAttribute, ModuleAttribute, NameAttribute, - SubmoduleAttribute, + self, kw, take_attributes, take_pyo3_options, CrateAttribute, ModuleAttribute, + NameAttribute, SubmoduleAttribute, }, get_doc, pyclass::PyClassPyO3Option, @@ -26,97 +26,85 @@ use syn::{ #[derive(Default)] pub struct PyModuleOptions { krate: Option, - name: Option, + name: Option, module: Option, - is_submodule: bool, + submodule: Option, } -impl PyModuleOptions { - pub fn from_attrs(attrs: &mut Vec) -> Result { +impl Parse for PyModuleOptions { + fn parse(input: ParseStream<'_>) -> syn::Result { let mut options: PyModuleOptions = Default::default(); - for option in take_pyo3_options(attrs)? { - match option { - PyModulePyO3Option::Name(name) => options.set_name(name.value.0)?, - PyModulePyO3Option::Crate(path) => options.set_crate(path)?, - PyModulePyO3Option::Module(module) => options.set_module(module)?, - PyModulePyO3Option::Submodule(submod) => options.set_submodule(submod)?, - } - } + options.add_attributes( + Punctuated::::parse_terminated(input)?, + )?; Ok(options) } +} - fn set_name(&mut self, name: syn::Ident) -> Result<()> { - ensure_spanned!( - self.name.is_none(), - name.span() => "`name` may only be specified once" - ); - - self.name = Some(name); - Ok(()) - } - - fn set_crate(&mut self, path: CrateAttribute) -> Result<()> { - ensure_spanned!( - self.krate.is_none(), - path.span() => "`crate` may only be specified once" - ); - - self.krate = Some(path); - Ok(()) - } - - fn set_module(&mut self, name: ModuleAttribute) -> Result<()> { - ensure_spanned!( - self.module.is_none(), - name.span() => "`module` may only be specified once" - ); - - self.module = Some(name); - Ok(()) +impl PyModuleOptions { + fn take_pyo3_options(&mut self, attrs: &mut Vec) -> Result<()> { + self.add_attributes(take_pyo3_options(attrs)?) } - fn set_submodule(&mut self, submod: SubmoduleAttribute) -> Result<()> { - ensure_spanned!( - !self.is_submodule, - submod.span() => "`submodule` may only be specified once" - ); - - self.is_submodule = true; + fn add_attributes( + &mut self, + attrs: impl IntoIterator, + ) -> Result<()> { + macro_rules! set_option { + ($key:ident) => { + { + ensure_spanned!( + self.$key.is_none(), + $key.span() => concat!("`", stringify!($key), "` may only be specified once") + ); + self.$key = Some($key); + } + }; + } + for attr in attrs { + match attr { + PyModulePyO3Option::Crate(krate) => set_option!(krate), + PyModulePyO3Option::Name(name) => set_option!(name), + PyModulePyO3Option::Module(module) => set_option!(module), + PyModulePyO3Option::Submodule(submodule) => set_option!(submodule), + } + } Ok(()) } } pub fn pymodule_module_impl( - mut module: syn::ItemMod, - mut is_submodule: bool, + module: &mut syn::ItemMod, + mut options: PyModuleOptions, ) -> Result { let syn::ItemMod { attrs, vis, unsafety: _, ident, - mod_token: _, + mod_token, content, semi: _, - } = &mut module; + } = module; let items = if let Some((_, items)) = content { items } else { - bail_spanned!(module.span() => "`#[pymodule]` can only be used on inline modules") + bail_spanned!(mod_token.span() => "`#[pymodule]` can only be used on inline modules") }; - let options = PyModuleOptions::from_attrs(attrs)?; + options.take_pyo3_options(attrs)?; let ctx = &Ctx::new(&options.krate, None); let Ctx { pyo3_path, .. } = ctx; let doc = get_doc(attrs, None, ctx); - let name = options.name.unwrap_or_else(|| ident.unraw()); + let name = options + .name + .map_or_else(|| ident.unraw(), |name| name.value.0); let full_name = if let Some(module) = &options.module { format!("{}.{}", module.value.value(), name) } else { name.to_string() }; - is_submodule = is_submodule || options.is_submodule; let mut module_items = Vec::new(); let mut module_items_cfg_attrs = Vec::new(); @@ -350,10 +338,11 @@ pub fn pymodule_module_impl( ) } }}; - let initialization = module_initialization(&name, ctx, module_def, is_submodule); + let initialization = module_initialization(&name, ctx, module_def, options.submodule.is_some()); + Ok(quote!( #(#attrs)* - #vis mod #ident { + #vis #mod_token #ident { #(#items)* #initialization @@ -373,14 +362,19 @@ pub fn pymodule_module_impl( /// Generates the function that is called by the python interpreter to initialize the native /// module -pub fn pymodule_function_impl(mut function: syn::ItemFn) -> Result { - let options = PyModuleOptions::from_attrs(&mut function.attrs)?; - process_functions_in_module(&options, &mut function)?; +pub fn pymodule_function_impl( + function: &mut syn::ItemFn, + mut options: PyModuleOptions, +) -> Result { + options.take_pyo3_options(&mut function.attrs)?; + process_functions_in_module(&options, function)?; let ctx = &Ctx::new(&options.krate, None); let stmts = std::mem::take(&mut function.block.stmts); let Ctx { pyo3_path, .. } = ctx; let ident = &function.sig.ident; - let name = options.name.unwrap_or_else(|| ident.unraw()); + let name = options + .name + .map_or_else(|| ident.unraw(), |name| name.value.0); let vis = &function.vis; let doc = get_doc(&function.attrs, None, ctx); @@ -419,7 +413,6 @@ pub fn pymodule_function_impl(mut function: syn::ItemFn) -> Result .push(parse_quote!(#[allow(clippy::used_underscore_binding)])); Ok(quote! { - #function #[doc(hidden)] #vis mod #ident { #initialization diff --git a/pyo3-macros/src/lib.rs b/pyo3-macros/src/lib.rs index d9e22a94ede..c4263a512d3 100644 --- a/pyo3-macros/src/lib.rs +++ b/pyo3-macros/src/lib.rs @@ -3,14 +3,14 @@ #![cfg_attr(docsrs, feature(doc_cfg, doc_auto_cfg))] use proc_macro::TokenStream; -use proc_macro2::{Span, TokenStream as TokenStream2}; +use proc_macro2::TokenStream as TokenStream2; use pyo3_macros_backend::{ build_derive_from_pyobject, build_py_class, build_py_enum, build_py_function, build_py_methods, pymodule_function_impl, pymodule_module_impl, PyClassArgs, PyClassMethodsType, - PyFunctionOptions, + PyFunctionOptions, PyModuleOptions, }; use quote::quote; -use syn::{parse::Nothing, parse_macro_input, Item}; +use syn::{parse_macro_input, Item}; /// A proc macro used to implement Python modules. /// @@ -24,6 +24,9 @@ use syn::{parse::Nothing, parse_macro_input, Item}; /// | Annotation | Description | /// | :- | :- | /// | `#[pyo3(name = "...")]` | Defines the name of the module in Python. | +/// | `#[pyo3(submodule)]` | Skips adding a `PyInit_` FFI symbol to the compiled binary. | +/// | `#[pyo3(module = "...")]` | Defines the Python `dotted.path` to the parent module for use in introspection. | +/// | `#[pyo3(crate = "pyo3")]` | Defines the path to PyO3 to use code generated by the macro. | /// /// For more on creating Python modules see the [module section of the guide][1]. /// @@ -35,32 +38,29 @@ use syn::{parse::Nothing, parse_macro_input, Item}; #[doc = concat!("[1]: https://pyo3.rs/v", env!("CARGO_PKG_VERSION"), "/module.html")] #[proc_macro_attribute] pub fn pymodule(args: TokenStream, input: TokenStream) -> TokenStream { - match parse_macro_input!(input as Item) { + let options = parse_macro_input!(args as PyModuleOptions); + + let mut ast = parse_macro_input!(input as Item); + let expanded = match &mut ast { Item::Mod(module) => { - let is_submodule = match parse_macro_input!(args as Option) { - Some(i) if i == "submodule" => true, - Some(_) => { - return syn::Error::new( - Span::call_site(), - "#[pymodule] only accepts submodule as an argument", - ) - .into_compile_error() - .into(); - } - None => false, - }; - pymodule_module_impl(module, is_submodule) - } - Item::Fn(function) => { - parse_macro_input!(args as Nothing); - pymodule_function_impl(function) + match pymodule_module_impl(module, options) { + // #[pymodule] on a module will rebuild the original ast, so we don't emit it here + Ok(expanded) => return expanded.into(), + Err(e) => Err(e), + } } + Item::Fn(function) => pymodule_function_impl(function, options), unsupported => Err(syn::Error::new_spanned( unsupported, "#[pymodule] only supports modules and functions.", )), } - .unwrap_or_compile_error() + .unwrap_or_compile_error(); + + quote!( + #ast + #expanded + ) .into() } diff --git a/tests/test_declarative_module.rs b/tests/test_declarative_module.rs index f62d51822ee..4f227a73462 100644 --- a/tests/test_declarative_module.rs +++ b/tests/test_declarative_module.rs @@ -49,8 +49,7 @@ create_exception!( "Some description." ); -#[pymodule] -#[pyo3(submodule)] +#[pymodule(submodule)] mod external_submodule {} /// A module written using declarative syntax. @@ -144,8 +143,7 @@ mod declarative_submodule { use super::{double, double_value}; } -#[pymodule] -#[pyo3(name = "declarative_module_renamed")] +#[pymodule(name = "declarative_module_renamed")] mod declarative_module2 { #[pymodule_export] use super::double; diff --git a/tests/test_module.rs b/tests/test_module.rs index b2487cfd8b3..eba1bcce6d2 100644 --- a/tests/test_module.rs +++ b/tests/test_module.rs @@ -138,8 +138,7 @@ fn test_module_with_explicit_py_arg() { }); } -#[pymodule] -#[pyo3(name = "other_name")] +#[pymodule(name = "other_name")] fn some_name(m: &Bound<'_, PyModule>) -> PyResult<()> { m.add("other_name", "other_name")?; Ok(()) diff --git a/tests/ui/empty.rs b/tests/ui/empty.rs new file mode 100644 index 00000000000..be89c636426 --- /dev/null +++ b/tests/ui/empty.rs @@ -0,0 +1 @@ +// see invalid_pymodule_in_root.rs diff --git a/tests/ui/invalid_pymodule_args.stderr b/tests/ui/invalid_pymodule_args.stderr index 933b6d6081c..261d8115e15 100644 --- a/tests/ui/invalid_pymodule_args.stderr +++ b/tests/ui/invalid_pymodule_args.stderr @@ -1,4 +1,4 @@ -error: unexpected token +error: expected one of: `name`, `crate`, `module`, `submodule` --> tests/ui/invalid_pymodule_args.rs:3:12 | 3 | #[pymodule(some_arg)] diff --git a/tests/ui/invalid_pymodule_glob.rs b/tests/ui/invalid_pymodule_glob.rs index 107cdf9382a..853493b535e 100644 --- a/tests/ui/invalid_pymodule_glob.rs +++ b/tests/ui/invalid_pymodule_glob.rs @@ -1,3 +1,5 @@ +#![allow(unused_imports)] + use pyo3::prelude::*; #[pyfunction] diff --git a/tests/ui/invalid_pymodule_glob.stderr b/tests/ui/invalid_pymodule_glob.stderr index 237e02037aa..1c033083e0c 100644 --- a/tests/ui/invalid_pymodule_glob.stderr +++ b/tests/ui/invalid_pymodule_glob.stderr @@ -1,5 +1,5 @@ error: #[pymodule] cannot import glob statements - --> tests/ui/invalid_pymodule_glob.rs:11:16 + --> tests/ui/invalid_pymodule_glob.rs:13:16 | -11 | use super::*; +13 | use super::*; | ^ diff --git a/tests/ui/invalid_pymodule_in_root.rs b/tests/ui/invalid_pymodule_in_root.rs index 47af4205f71..76ced6c3fb6 100644 --- a/tests/ui/invalid_pymodule_in_root.rs +++ b/tests/ui/invalid_pymodule_in_root.rs @@ -1,6 +1,7 @@ use pyo3::prelude::*; #[pymodule] +#[path = "empty.rs"] // to silence error related to missing file mod invalid_pymodule_in_root_module; fn main() {} diff --git a/tests/ui/invalid_pymodule_in_root.stderr b/tests/ui/invalid_pymodule_in_root.stderr index 91783be0e97..06152161e5d 100644 --- a/tests/ui/invalid_pymodule_in_root.stderr +++ b/tests/ui/invalid_pymodule_in_root.stderr @@ -1,13 +1,13 @@ error[E0658]: non-inline modules in proc macro input are unstable - --> tests/ui/invalid_pymodule_in_root.rs:4:1 + --> tests/ui/invalid_pymodule_in_root.rs:5:1 | -4 | mod invalid_pymodule_in_root_module; +5 | mod invalid_pymodule_in_root_module; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: see issue #54727 for more information error: `#[pymodule]` can only be used on inline modules - --> tests/ui/invalid_pymodule_in_root.rs:4:1 + --> tests/ui/invalid_pymodule_in_root.rs:5:1 | -4 | mod invalid_pymodule_in_root_module; +5 | mod invalid_pymodule_in_root_module; | ^^^ diff --git a/tests/ui/invalid_pymodule_trait.stderr b/tests/ui/invalid_pymodule_trait.stderr index 4b02f14a540..0b0d46da93d 100644 --- a/tests/ui/invalid_pymodule_trait.stderr +++ b/tests/ui/invalid_pymodule_trait.stderr @@ -3,3 +3,9 @@ error: `#[pymodule_export]` may only be used on `use` statements | 5 | #[pymodule_export] | ^ + +error: cannot find attribute `pymodule_export` in this scope + --> tests/ui/invalid_pymodule_trait.rs:5:7 + | +5 | #[pymodule_export] + | ^^^^^^^^^^^^^^^ diff --git a/tests/ui/invalid_pymodule_two_pymodule_init.rs b/tests/ui/invalid_pymodule_two_pymodule_init.rs index d676b0fa277..ed72cbb7237 100644 --- a/tests/ui/invalid_pymodule_two_pymodule_init.rs +++ b/tests/ui/invalid_pymodule_two_pymodule_init.rs @@ -2,13 +2,15 @@ use pyo3::prelude::*; #[pymodule] mod module { + use pyo3::prelude::*; + #[pymodule_init] - fn init(m: &PyModule) -> PyResult<()> { + fn init(_m: &Bound<'_, PyModule>) -> PyResult<()> { Ok(()) } #[pymodule_init] - fn init2(m: &PyModule) -> PyResult<()> { + fn init2(_m: &Bound<'_, PyModule>) -> PyResult<()> { Ok(()) } } diff --git a/tests/ui/invalid_pymodule_two_pymodule_init.stderr b/tests/ui/invalid_pymodule_two_pymodule_init.stderr index c117ebd573f..8fbd12f2e45 100644 --- a/tests/ui/invalid_pymodule_two_pymodule_init.stderr +++ b/tests/ui/invalid_pymodule_two_pymodule_init.stderr @@ -1,5 +1,5 @@ error: only one `#[pymodule_init]` may be specified - --> tests/ui/invalid_pymodule_two_pymodule_init.rs:11:5 + --> tests/ui/invalid_pymodule_two_pymodule_init.rs:13:5 | -11 | fn init2(m: &PyModule) -> PyResult<()> { +13 | fn init2(_m: &Bound<'_, PyModule>) -> PyResult<()> { | ^^