From a5a3f3f7f28b6a542b29fa552059b8e7bee496fa 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 | 137 ++++++++---------- pyo3-macros/src/lib.rs | 44 +++--- tests/test_declarative_module.rs | 6 +- tests/test_module.rs | 3 +- tests/ui/duplicate_pymodule_submodule.stderr | 14 +- 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 +- 17 files changed, 125 insertions(+), 120 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 ce162a70..ba850a06 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 ee5485e0..78616946 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 00000000..d465ec99 --- /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 78e89994..1e764176 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, @@ -16,7 +16,7 @@ use std::ffi::CString; use syn::{ ext::IdentExt, parse::{Parse, ParseStream}, - parse_quote, + parse_quote, parse_quote_spanned, punctuated::Punctuated, spanned::Spanned, token::Comma, @@ -26,105 +26,89 @@ 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(()) +impl PyModuleOptions { + fn take_pyo3_options(&mut self, attrs: &mut Vec) -> Result<()> { + self.add_attributes(take_pyo3_options(attrs)?) } - 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(()) - } - - fn set_submodule(&mut self, submod: SubmoduleAttribute) -> Result<()> { - ensure_spanned!( - !self.is_submodule, - submod.span() => "`submodule` may only be specified once (it is implicitly always specified for nested modules)" - ); - - self.is_submodule = true; + fn add_attributes( + &mut self, + attrs: impl IntoIterator, + ) -> Result<()> { + macro_rules! set_option { + ($key:ident $(, $extra:literal)?) => { + { + ensure_spanned!( + self.$key.is_none(), + $key.span() => concat!("`", stringify!($key), "` may only be specified once" $(, $extra)?) + ); + 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, + " (it is implicitly always specified for nested modules)" + ), + } + } 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 = match (is_submodule, options.is_submodule) { - (true, true) => { - bail_spanned!(module.span() => "`submodule` may only be specified once (it is implicitly always specified for nested modules)") - } - (false, false) => false, - (true, false) | (false, true) => true, - }; - let mut module_items = Vec::new(); let mut module_items_cfg_attrs = Vec::new(); @@ -280,7 +264,9 @@ pub fn pymodule_module_impl( )? { set_module_attribute(&mut item_mod.attrs, &full_name); } - item_mod.attrs.push(parse_quote!(#[pyo3(submodule)])); + item_mod + .attrs + .push(parse_quote_spanned!(item_mod.mod_token.span()=> #[pyo3(submodule)])); } } Item::ForeignMod(item) => { @@ -358,10 +344,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 @@ -381,13 +368,18 @@ 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 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); @@ -402,7 +394,6 @@ pub fn pymodule_function_impl(mut function: syn::ItemFn) -> Result .push(quote!(::std::convert::Into::into(#pyo3_path::impl_::pymethods::BoundRef(module)))); Ok(quote! { - #function #[doc(hidden)] #vis mod #ident { #initialization diff --git a/pyo3-macros/src/lib.rs b/pyo3-macros/src/lib.rs index d9e22a94..c4263a51 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 f0952438..060da188 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 b2487cfd..eba1bcce 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/duplicate_pymodule_submodule.stderr b/tests/ui/duplicate_pymodule_submodule.stderr index a16e9ac7..a2141b0d 100644 --- a/tests/ui/duplicate_pymodule_submodule.stderr +++ b/tests/ui/duplicate_pymodule_submodule.stderr @@ -4,8 +4,14 @@ error: `submodule` may only be specified once (it is implicitly always specified 4 | mod submod {} | ^^^ -error[E0433]: failed to resolve: use of undeclared crate or module `submod` - --> tests/ui/duplicate_pymodule_submodule.rs:4:6 +error[E0425]: cannot find value `_PYO3_DEF` in module `submod` + --> tests/ui/duplicate_pymodule_submodule.rs:1:1 + | +1 | #[pyo3::pymodule] + | ^^^^^^^^^^^^^^^^^ not found in `submod` + | + = note: this error originates in the attribute macro `pyo3::pymodule` (in Nightly builds, run with -Z macro-backtrace for more info) +help: consider importing this static + | +3 + use crate::mymodule::_PYO3_DEF; | -4 | mod submod {} - | ^^^^^^ use of undeclared crate or module `submod` diff --git a/tests/ui/empty.rs b/tests/ui/empty.rs new file mode 100644 index 00000000..be89c636 --- /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 933b6d60..261d8115 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 107cdf93..853493b5 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 237e0203..1c033083 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 47af4205..76ced6c3 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 91783be0..06152161 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 4b02f14a..0b0d46da 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 d676b0fa..ed72cbb7 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 c117ebd5..8fbd12f2 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<()> { | ^^