From 38a735af3b8def191fb1654ba4ad4c9e83c055fa Mon Sep 17 00:00:00 2001 From: Noa Date: Mon, 19 Dec 2022 18:47:40 -0600 Subject: [PATCH] Tidy up -derive, make use of let-else --- Cargo.lock | 1 - derive/Cargo.toml | 1 - derive/src/compile_bytecode.rs | 96 ++++++++++------- derive/src/error.rs | 2 +- derive/src/from_args.rs | 67 ++++++------ derive/src/lib.rs | 20 +++- derive/src/pyclass.rs | 189 +++++++++++++-------------------- derive/src/pymodule.rs | 34 +++--- derive/src/util.rs | 108 +++++++------------ 9 files changed, 233 insertions(+), 285 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d7a086f27f..837590665a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2003,7 +2003,6 @@ dependencies = [ "once_cell", "proc-macro2", "quote", - "rustpython-codegen", "rustpython-compiler", "rustpython-compiler-core", "rustpython-doc", diff --git a/derive/Cargo.toml b/derive/Cargo.toml index 1b36bacda2..6b565af75a 100644 --- a/derive/Cargo.toml +++ b/derive/Cargo.toml @@ -11,7 +11,6 @@ edition = "2021" proc-macro = true [dependencies] -rustpython-codegen = { path = "../compiler/codegen", version = "0.1.1" } rustpython-compiler = { path = "../compiler", version = "0.1.1" } rustpython-compiler-core = { path = "../compiler/core", version = "0.1.1" } rustpython-doc = { git = "https://github.com/RustPython/__doc__", branch = "main" } diff --git a/derive/src/compile_bytecode.rs b/derive/src/compile_bytecode.rs index 343ffb315e..6f5d3caca1 100644 --- a/derive/src/compile_bytecode.rs +++ b/derive/src/compile_bytecode.rs @@ -17,8 +17,6 @@ use crate::{extract_spans, Diagnostic}; use once_cell::sync::Lazy; use proc_macro2::{Span, TokenStream}; use quote::quote; -use rustpython_codegen as codegen; -use rustpython_compiler::compile; use rustpython_compiler_core::{CodeObject, FrozenModule, Mode}; use std::{ collections::HashMap, @@ -51,15 +49,25 @@ struct CompilationSource { span: (Span, Span), } +pub trait Compiler { + fn compile( + &self, + source: &str, + mode: Mode, + module_name: String, + ) -> Result>; +} + impl CompilationSource { fn compile_string D>( &self, source: &str, mode: Mode, module_name: String, + compiler: &dyn Compiler, origin: F, ) -> Result { - compile(source, mode, module_name, codegen::CompileOpts::default()).map_err(|err| { + compiler.compile(source, mode, module_name).map_err(|err| { Diagnostic::spans_error( self.span, format!("Python compile error from {}: {}", origin(), err), @@ -71,21 +79,30 @@ impl CompilationSource { &self, mode: Mode, module_name: String, + compiler: &dyn Compiler, ) -> Result, Diagnostic> { match &self.kind { - CompilationSourceKind::Dir(rel_path) => { - self.compile_dir(&CARGO_MANIFEST_DIR.join(rel_path), String::new(), mode) - } + CompilationSourceKind::Dir(rel_path) => self.compile_dir( + &CARGO_MANIFEST_DIR.join(rel_path), + String::new(), + mode, + compiler, + ), _ => Ok(hashmap! { module_name.clone() => FrozenModule { - code: self.compile_single(mode, module_name)?, + code: self.compile_single(mode, module_name, compiler)?, package: false, }, }), } } - fn compile_single(&self, mode: Mode, module_name: String) -> Result { + fn compile_single( + &self, + mode: Mode, + module_name: String, + compiler: &dyn Compiler, + ) -> Result { match &self.kind { CompilationSourceKind::File(rel_path) => { let path = CARGO_MANIFEST_DIR.join(rel_path); @@ -95,10 +112,10 @@ impl CompilationSource { format!("Error reading file {:?}: {}", path, err), ) })?; - self.compile_string(&source, mode, module_name, || rel_path.display()) + self.compile_string(&source, mode, module_name, compiler, || rel_path.display()) } CompilationSourceKind::SourceCode(code) => { - self.compile_string(&textwrap::dedent(code), mode, module_name, || { + self.compile_string(&textwrap::dedent(code), mode, module_name, compiler, || { "string literal" }) } @@ -113,6 +130,7 @@ impl CompilationSource { path: &Path, parent: String, mode: Mode, + compiler: &dyn Compiler, ) -> Result, Diagnostic> { let mut code_map = HashMap::new(); let paths = fs::read_dir(path) @@ -144,6 +162,7 @@ impl CompilationSource { format!("{}.{}", parent, file_name) }, mode, + compiler, )?); } else if file_name.ends_with(".py") { let stem = path.file_stem().unwrap().to_str().unwrap(); @@ -163,7 +182,7 @@ impl CompilationSource { format!("Error reading file {:?}: {}", path, err), ) })?; - self.compile_string(&source, mode, module_name.clone(), || { + self.compile_string(&source, mode, module_name.clone(), compiler, || { path.strip_prefix(&*CARGO_MANIFEST_DIR) .ok() .unwrap_or(&path) @@ -239,35 +258,28 @@ impl PyCompileInput { Some(ident) => ident, None => continue, }; + let check_str = || match &name_value.lit { + Lit::Str(s) => Ok(s), + _ => Err(err_span!(name_value.lit, "{ident} must be a string")), + }; if ident == "mode" { - match &name_value.lit { - Lit::Str(s) => match s.value().parse() { - Ok(mode_val) => mode = Some(mode_val), - Err(e) => bail_span!(s, "{}", e), - }, - _ => bail_span!(name_value.lit, "mode must be a string"), + let s = check_str()?; + match s.value().parse() { + Ok(mode_val) => mode = Some(mode_val), + Err(e) => bail_span!(s, "{}", e), } } else if ident == "module_name" { - module_name = Some(match &name_value.lit { - Lit::Str(s) => s.value(), - _ => bail_span!(name_value.lit, "module_name must be string"), - }) + module_name = Some(check_str()?.value()) } else if ident == "source" { assert_source_empty(&source)?; - let code = match &name_value.lit { - Lit::Str(s) => s.value(), - _ => bail_span!(name_value.lit, "source must be a string"), - }; + let code = check_str()?.value(); source = Some(CompilationSource { kind: CompilationSourceKind::SourceCode(code), span: extract_spans(&name_value).unwrap(), }); } else if ident == "file" { assert_source_empty(&source)?; - let path = match &name_value.lit { - Lit::Str(s) => PathBuf::from(s.value()), - _ => bail_span!(name_value.lit, "source must be a string"), - }; + let path = check_str()?.value().into(); source = Some(CompilationSource { kind: CompilationSourceKind::File(path), span: extract_spans(&name_value).unwrap(), @@ -278,19 +290,13 @@ impl PyCompileInput { } assert_source_empty(&source)?; - let path = match &name_value.lit { - Lit::Str(s) => PathBuf::from(s.value()), - _ => bail_span!(name_value.lit, "source must be a string"), - }; + let path = check_str()?.value().into(); source = Some(CompilationSource { kind: CompilationSourceKind::Dir(path), span: extract_spans(&name_value).unwrap(), }); } else if ident == "crate_name" { - let name = match &name_value.lit { - Lit::Str(s) => s.parse()?, - _ => bail_span!(name_value.lit, "source must be a string"), - }; + let name = check_str()?.parse()?; crate_name = Some(name); } } @@ -351,12 +357,17 @@ struct PyCompileArgs { crate_name: syn::Path, } -pub fn impl_py_compile(input: TokenStream) -> Result { +pub fn impl_py_compile( + input: TokenStream, + compiler: &dyn Compiler, +) -> Result { let input: PyCompileInput = parse2(input)?; let args = input.parse(false)?; let crate_name = args.crate_name; - let code = args.source.compile_single(args.mode, args.module_name)?; + let code = args + .source + .compile_single(args.mode, args.module_name, compiler)?; let bytes = code.to_bytes(); let bytes = LitByteStr::new(&bytes, Span::call_site()); @@ -369,12 +380,15 @@ pub fn impl_py_compile(input: TokenStream) -> Result { Ok(output) } -pub fn impl_py_freeze(input: TokenStream) -> Result { +pub fn impl_py_freeze( + input: TokenStream, + compiler: &dyn Compiler, +) -> Result { let input: PyCompileInput = parse2(input)?; let args = input.parse(true)?; let crate_name = args.crate_name; - let code_map = args.source.compile(args.mode, args.module_name)?; + let code_map = args.source.compile(args.mode, args.module_name, compiler)?; let data = rustpython_compiler_core::frozen_lib::encode_lib(code_map.iter().map(|(k, v)| (&**k, v))); diff --git a/derive/src/error.rs b/derive/src/error.rs index 5ab6e5b280..2313e529a9 100644 --- a/derive/src/error.rs +++ b/derive/src/error.rs @@ -33,7 +33,7 @@ use syn::parse::Error; macro_rules! err_span { ($span:expr, $($msg:tt)*) => ( - syn::Error::new_spanned(&$span, format!($($msg)*)) + syn::Error::new_spanned(&$span, format_args!($($msg)*)) ) } diff --git a/derive/src/from_args.rs b/derive/src/from_args.rs index 9cd58859ed..5b3b1e637b 100644 --- a/derive/src/from_args.rs +++ b/derive/src/from_args.rs @@ -1,4 +1,3 @@ -use crate::util::path_eq; use proc_macro2::TokenStream; use quote::{quote, ToTokens}; use syn::{ @@ -39,39 +38,39 @@ impl ArgAttribute { if !attr.path.is_ident("pyarg") { return None; } - let inner = move || match attr.parse_meta()? { - Meta::List(list) => { - let mut iter = list.nested.iter(); - let first_arg = iter.next().ok_or_else(|| { - err_span!(list, "There must be at least one argument to #[pyarg()]") - })?; - let kind = match first_arg { - NestedMeta::Meta(Meta::Path(path)) => { - path.get_ident().and_then(ParameterKind::from_ident) - } - _ => None, - }; - let kind = kind.ok_or_else(|| { - err_span!( - first_arg, - "The first argument to #[pyarg()] must be the parameter type, either \ - 'positional', 'any', 'named', or 'flatten'." - ) - })?; - - let mut attribute = ArgAttribute { - name: None, - kind, - default: None, - }; - - for arg in iter { - attribute.parse_argument(arg)?; + let inner = move || { + let Meta::List(list) = attr.parse_meta()? else { + bail_span!(attr, "pyarg must be a list, like #[pyarg(...)]") + }; + let mut iter = list.nested.iter(); + let first_arg = iter.next().ok_or_else(|| { + err_span!(list, "There must be at least one argument to #[pyarg()]") + })?; + let kind = match first_arg { + NestedMeta::Meta(Meta::Path(path)) => { + path.get_ident().and_then(ParameterKind::from_ident) } + _ => None, + }; + let kind = kind.ok_or_else(|| { + err_span!( + first_arg, + "The first argument to #[pyarg()] must be the parameter type, either \ + 'positional', 'any', 'named', or 'flatten'." + ) + })?; - Ok(attribute) + let mut attribute = ArgAttribute { + name: None, + kind, + default: None, + }; + + for arg in iter { + attribute.parse_argument(arg)?; } - _ => bail_span!(attr, "pyarg must be a list, like #[pyarg(...)]"), + + Ok(attribute) }; Some(inner()) } @@ -82,7 +81,7 @@ impl ArgAttribute { } match arg { NestedMeta::Meta(Meta::Path(path)) => { - if path_eq(path, "default") || path_eq(path, "optional") { + if path.is_ident("default") || path.is_ident("optional") { if self.default.is_none() { self.default = Some(None); } @@ -91,7 +90,7 @@ impl ArgAttribute { } } NestedMeta::Meta(Meta::NameValue(name_value)) => { - if path_eq(&name_value.path, "default") { + if name_value.path.is_ident("default") { if matches!(self.default, Some(Some(_))) { bail_span!(name_value, "Default already set"); } @@ -100,7 +99,7 @@ impl ArgAttribute { Lit::Str(ref val) => self.default = Some(Some(val.parse()?)), _ => bail_span!(name_value, "Expected string value for default argument"), } - } else if path_eq(&name_value.path, "name") { + } else if name_value.path.is_ident("name") { if self.name.is_some() { bail_span!(name_value, "already have a name") } diff --git a/derive/src/lib.rs b/derive/src/lib.rs index aeb34396d9..563095f874 100644 --- a/derive/src/lib.rs +++ b/derive/src/lib.rs @@ -104,14 +104,30 @@ pub fn pystruct_sequence_try_from_object( result_to_tokens(pystructseq::impl_pystruct_sequence_try_from_object(input)) } +// would be cool to move all the macro implementation to a separate rustpython-derive-shared +// that just depends on rustpython-compiler-core, and then rustpython-derive would hook -compiler +// up to it; so that (the bulk of) rustpython-derive and rustpython-codegen could build in parallel +struct Compiler; +impl compile_bytecode::Compiler for Compiler { + fn compile( + &self, + source: &str, + mode: rustpython_compiler_core::Mode, + module_name: String, + ) -> Result> { + use rustpython_compiler::{compile, CompileOpts}; + Ok(compile(source, mode, module_name, CompileOpts::default())?) + } +} + #[proc_macro] pub fn py_compile(input: proc_macro::TokenStream) -> proc_macro::TokenStream { - result_to_tokens(compile_bytecode::impl_py_compile(input.into())) + result_to_tokens(compile_bytecode::impl_py_compile(input.into(), &Compiler)) } #[proc_macro] pub fn py_freeze(input: proc_macro::TokenStream) -> proc_macro::TokenStream { - result_to_tokens(compile_bytecode::impl_py_freeze(input.into())) + result_to_tokens(compile_bytecode::impl_py_freeze(input.into(), &Compiler)) } #[proc_macro_derive(PyPayload)] diff --git a/derive/src/pyclass.rs b/derive/src/pyclass.rs index b386d10579..4f88adeaf4 100644 --- a/derive/src/pyclass.rs +++ b/derive/src/pyclass.rs @@ -1,6 +1,6 @@ use super::Diagnostic; use crate::util::{ - path_eq, pyclass_ident_and_attrs, text_signature, ClassItemMeta, ContentItem, ContentItemInner, + pyclass_ident_and_attrs, text_signature, ClassItemMeta, ContentItem, ContentItemInner, ErrorVec, ItemMeta, ItemMetaInner, ItemNursery, SimpleItemMeta, ALL_ALLOWED_NAMES, }; use proc_macro2::{Span, TokenStream}; @@ -222,7 +222,7 @@ fn generate_class_def( }; let basicsize = quote!(std::mem::size_of::<#ident>()); let is_pystruct = attrs.iter().any(|attr| { - path_eq(&attr.path, "derive") + attr.path.is_ident("derive") && if let Ok(Meta::List(l)) = attr.parse_meta() { l.nested .into_iter() @@ -232,10 +232,7 @@ fn generate_class_def( } }); if base.is_some() && is_pystruct { - return Err(syn::Error::new_spanned( - ident, - "PyStructSequence cannot have `base` class attr", - )); + bail_span!(ident, "PyStructSequence cannot have `base` class attr",); } let base_class = if is_pystruct { Some(quote! { rustpython_vm::builtins::PyTuple }) @@ -331,9 +328,9 @@ pub(crate) fn impl_pyexception(attr: AttributeArgs, item: Item) -> Result `KeyboardInterrupt` - let class_name = class_name.strip_prefix("Py").ok_or_else(|| { - syn::Error::new_spanned(&item, "We require 'class_name' to have 'Py' prefix") - })?; + let class_name = class_name + .strip_prefix("Py") + .ok_or_else(|| err_span!(item, "We require 'class_name' to have 'Py' prefix"))?; // We just "proxy" it into `pyclass` macro, because, exception is a class. let ret = quote! { @@ -737,10 +734,7 @@ impl GetSetNursery { ) -> Result<()> { assert!(!self.validated, "new item is not allowed after validation"); if !matches!(kind, GetSetItemKind::Get) && !cfgs.is_empty() { - return Err(syn::Error::new_spanned( - item_ident, - "Only the getter can have #[cfg]", - )); + bail_span!(item_ident, "Only the getter can have #[cfg]",); } let entry = self.map.entry((name.clone(), cfgs)).or_default(); let func = match kind { @@ -749,10 +743,11 @@ impl GetSetNursery { GetSetItemKind::Delete => &mut entry.2, }; if func.is_some() { - return Err(syn::Error::new_spanned( + bail_span!( item_ident, - format!("Multiple property accessors with name '{}'", name), - )); + "Multiple property accessors with name '{}'", + name + ); } *func = Some(item_ident); Ok(()) @@ -762,9 +757,10 @@ impl GetSetNursery { let mut errors = Vec::new(); for ((name, _cfgs), (getter, setter, deleter)) in &self.map { if getter.is_none() { - errors.push(syn::Error::new_spanned( + errors.push(err_span!( setter.as_ref().or(deleter.as_ref()).unwrap(), - format!("GetSet '{}' is missing a getter", name), + "GetSet '{}' is missing a getter", + name )); }; } @@ -841,10 +837,7 @@ impl MemberNursery { MemberItemKind::Set => &mut entry.1, }; if func.is_some() { - return Err(syn::Error::new_spanned( - item_ident, - format!("Multiple member accessors with name '{}'", name), - )); + bail_span!(item_ident, "Multiple member accessors with name '{}'", name); } *func = Some(item_ident); Ok(()) @@ -854,9 +847,10 @@ impl MemberNursery { let mut errors = Vec::new(); for ((name, _), (getter, setter)) in &self.map { if getter.is_none() { - errors.push(syn::Error::new_spanned( + errors.push(err_span!( setter.as_ref().unwrap(), - format!("Member '{}' is missing a getter", name), + "Member '{}' is missing a getter", + name )); }; } @@ -950,13 +944,11 @@ impl GetSetItemMeta { (true, false) => GetSetItemKind::Set, (false, true) => GetSetItemKind::Delete, (true, true) => { - return Err(syn::Error::new_spanned( + bail_span!( &inner.meta_ident, - format!( - "can't have both setter and deleter on a #[{}] fn", - inner.meta_name() - ), - )) + "can't have both setter and deleter on a #[{}] fn", + inner.meta_name() + ) } }; let name = inner._optional_str("name")?; @@ -967,27 +959,23 @@ impl GetSetItemMeta { let extract_prefix_name = |prefix, item_typ| { if let Some(name) = sig_name.strip_prefix(prefix) { if name.is_empty() { - Err(syn::Error::new_spanned( - &inner.meta_ident, - format!( - "A #[{}({typ})] fn with a {prefix}* name must \ - have something after \"{prefix}\"", - inner.meta_name(), - typ = item_typ, - prefix = prefix - ), + Err(err_span!( + inner.meta_ident, + "A #[{}({typ})] fn with a {prefix}* name must \ + have something after \"{prefix}\"", + inner.meta_name(), + typ = item_typ, + prefix = prefix )) } else { Ok(name.to_owned()) } } else { - Err(syn::Error::new_spanned( - &inner.meta_ident, - format!( - "A #[{}(setter)] fn must either have a `name` \ - parameter or a fn name along the lines of \"set_*\"", - inner.meta_name() - ), + Err(err_span!( + inner.meta_ident, + "A #[{}(setter)] fn must either have a `name` \ + parameter or a fn name along the lines of \"set_*\"", + inner.meta_name() )) } }; @@ -1025,17 +1013,14 @@ impl ItemMeta for SlotItemMeta { } else { Some(HashMap::default()) }; - match (meta_map, nested.next()) { - (Some(meta_map), None) => Ok(Self::from_inner(ItemMetaInner { - item_ident, - meta_ident, - meta_map, - })), - _ => Err(syn::Error::new_spanned( - meta_ident, - "#[pyslot] must be of the form #[pyslot] or #[pyslot(slotname)]", - )), - } + let (Some(meta_map), None) = (meta_map, nested.next()) else { + bail_span!(meta_ident, "#[pyslot] must be of the form #[pyslot] or #[pyslot(slotname)]") + }; + Ok(Self::from_inner(ItemMetaInner { + item_ident, + meta_ident, + meta_map, + })) } fn from_inner(inner: ItemMetaInner) -> Self { @@ -1064,8 +1049,8 @@ impl SlotItemMeta { Some(name) }; slot_name.ok_or_else(|| { - syn::Error::new_spanned( - &inner.meta_ident, + err_span!( + inner.meta_ident, "#[pyslot] must be of the form #[pyslot] or #[pyslot(slotname)]", ) }) @@ -1092,27 +1077,23 @@ impl MemberItemMeta { let extract_prefix_name = |prefix, item_typ| { if let Some(name) = sig_name.strip_prefix(prefix) { if name.is_empty() { - Err(syn::Error::new_spanned( - &inner.meta_ident, - format!( - "A #[{}({typ})] fn with a {prefix}* name must \ - have something after \"{prefix}\"", - inner.meta_name(), - typ = item_typ, - prefix = prefix - ), + Err(err_span!( + inner.meta_ident, + "A #[{}({typ})] fn with a {prefix}* name must \ + have something after \"{prefix}\"", + inner.meta_name(), + typ = item_typ, + prefix = prefix )) } else { Ok(name.to_owned()) } } else { - Err(syn::Error::new_spanned( - &inner.meta_ident, - format!( - "A #[{}(setter)] fn must either have a `name` \ - parameter or a fn name along the lines of \"set_*\"", - inner.meta_name() - ), + Err(err_span!( + inner.meta_ident, + "A #[{}(setter)] fn must either have a `name` \ + parameter or a fn name along the lines of \"set_*\"", + inner.meta_name() )) } }; @@ -1159,15 +1140,12 @@ fn extract_impl_attrs(attr: AttributeArgs, item: &Ident) -> Result { - if path_eq(&path, "with") { + if path.is_ident("with") { for meta in nested { - let path = match meta { - NestedMeta::Meta(Meta::Path(path)) => path, - meta => { - bail_span!(meta, "#[pyclass(with(...))] arguments should be paths") - } + let NestedMeta::Meta(Meta::Path(path)) = meta else { + bail_span!(meta, "#[pyclass(with(...))] arguments should be paths") }; - let (extend_class, extend_slots) = if path_eq(&path, "PyRef") { + let (extend_class, extend_slots) = if path.is_ident("PyRef") { // special handling for PyRef ( quote!(PyRef::::impl_extend_class), @@ -1187,25 +1165,17 @@ fn extract_impl_attrs(attr: AttributeArgs, item: &Ident) -> Result { - if let Some(ident) = path.get_ident() { - flags.push(quote_spanned! { ident.span() => - .union(::rustpython_vm::types::PyTypeFlags::#ident) - }); - } else { - bail_span!( - path, - "#[pyclass(flags(...))] arguments should be ident" - ) - } - } - meta => { - bail_span!(meta, "#[pyclass(flags(...))] arguments should be ident") - } - } + let NestedMeta::Meta(Meta::Path(path)) = meta else { + bail_span!(meta, "#[pyclass(flags(...))] arguments should be ident") + }; + let ident = path.get_ident().ok_or_else(|| { + err_span!(path, "#[pyclass(flags(...))] arguments should be ident") + })?; + flags.push(quote_spanned! { ident.span() => + .union(::rustpython_vm::types::PyTypeFlags::#ident) + }); } } else { bail_span!(path, "Unknown pyimpl attribute") @@ -1295,19 +1265,13 @@ where continue; }; if attr_name == "cfg" { - return Err(syn::Error::new_spanned( - attr, - "#[py*] items must be placed under `cfgs`", - )); + bail_span!(attr, "#[py*] items must be placed under `cfgs`",); } let attr_name = match AttrName::from_str(attr_name.as_str()) { Ok(name) => name, Err(wrong_name) => { if ALL_ALLOWED_NAMES.contains(&attr_name.as_str()) { - return Err(syn::Error::new_spanned( - attr, - format!("#[pyclass] doesn't accept #[{}]", wrong_name), - )); + bail_span!(attr, "#[pyclass] doesn't accept #[{}]", wrong_name) } else { continue; } @@ -1371,14 +1335,13 @@ fn parse_vec_ident( ) -> Result { Ok(attr .get(index) - .ok_or_else(|| { - syn::Error::new_spanned(item, format!("We require {} argument to be set", &message)) - })? + .ok_or_else(|| err_span!(item, "We require {} argument to be set", message))? .get_ident() .ok_or_else(|| { - syn::Error::new_spanned( + err_span!( item, - format!("We require {} argument to be ident or string", &message), + "We require {} argument to be ident or string", + message ) })? .to_string()) diff --git a/derive/src/pymodule.rs b/derive/src/pymodule.rs index 4fdc5c5628..f1d3722cf1 100644 --- a/derive/src/pymodule.rs +++ b/derive/src/pymodule.rs @@ -195,32 +195,28 @@ where continue; }; if attr_name == "cfg" { - return Err(syn::Error::new_spanned( - attr, - "#[py*] items must be placed under `cfgs`", - )); + bail_span!(attr, "#[py*] items must be placed under `cfgs`") } let attr_name = match AttrName::from_str(attr_name.as_str()) { Ok(name) => name, Err(wrong_name) => { - let msg = if !ALL_ALLOWED_NAMES.contains(&wrong_name.as_str()) { + if !ALL_ALLOWED_NAMES.contains(&wrong_name.as_str()) { continue; } else if closed { - "Only one #[pyattr] annotated #[py*] item can exist".to_owned() + bail_span!(attr, "Only one #[pyattr] annotated #[py*] item can exist") } else { - format!("#[pymodule] doesn't accept #[{}]", wrong_name) - }; - return Err(syn::Error::new_spanned(attr, msg)); + bail_span!(attr, "#[pymodule] doesn't accept #[{}]", wrong_name) + } } }; if attr_name == AttrName::Attr { if !result.is_empty() { - return Err(syn::Error::new_spanned( + bail_span!( attr, "#[pyattr] must be placed on top of other #[py*] items", - )); + ) } pyattrs.push(i); continue; @@ -234,10 +230,10 @@ where result.push(item_new(i, attr_name, pyattrs.clone())); } _ => { - return Err(syn::Error::new_spanned( + bail_span!( attr, "#[pyclass] or #[pyfunction] only can follow #[pyattr]", - )); + ) } } pyattrs.clear(); @@ -409,14 +405,12 @@ impl ModuleItem for ClassItem { if self.pyattrs.is_empty() { // check noattr before ClassItemMeta::from_attr if noattr.is_none() { - return Err(syn::Error::new_spanned( + bail_span!( ident, - format!( - "#[{name}] requires #[pyattr] to be a module attribute. \ - To keep it free type, try #[{name}(noattr)]", - name = self.attr_name() - ), - )); + "#[{name}] requires #[pyattr] to be a module attribute. \ + To keep it free type, try #[{name}(noattr)]", + name = self.attr_name() + ) } } let noattr = noattr.is_some(); diff --git a/derive/src/util.rs b/derive/src/util.rs index fff2a403bb..2d9b2f05ce 100644 --- a/derive/src/util.rs +++ b/derive/src/util.rs @@ -3,8 +3,7 @@ use proc_macro2::{Span, TokenStream}; use quote::{quote, ToTokens}; use std::collections::{HashMap, HashSet}; use syn::{ - spanned::Spanned, Attribute, Ident, Meta, MetaList, NestedMeta, Path, Result, Signature, - UseTree, + spanned::Spanned, Attribute, Ident, Meta, MetaList, NestedMeta, Result, Signature, UseTree, }; use syn_ext::{ ext::{AttributeExt as SynAttributeExt, *}, @@ -136,12 +135,10 @@ impl ItemMetaInner { if allowed_names.contains(&name.as_str()) { Ok(Some(name)) } else { - Err(syn::Error::new_spanned( + Err(err_span!( ident, - format!( - "#[{meta_ident}({name})] is not one of allowed attributes [{}]", - allowed_names.join(", ") - ), + "#[{meta_ident}({name})] is not one of allowed attributes [{}]", + allowed_names.iter().format(", ") )) } } else { @@ -149,10 +146,7 @@ impl ItemMetaInner { } })?; if !lits.is_empty() { - return Err(syn::Error::new_spanned( - &meta_ident, - format!("#[{meta_ident}(..)] cannot contain literal"), - )); + bail_span!(meta_ident, "#[{meta_ident}(..)] cannot contain literal") } Ok(Self { @@ -172,22 +166,12 @@ impl ItemMetaInner { pub fn _optional_str(&self, key: &str) -> Result> { let value = if let Some((_, meta)) = self.meta_map.get(key) { - match meta { - Meta::NameValue(syn::MetaNameValue { - lit: syn::Lit::Str(lit), - .. - }) => Some(lit.value()), - other => { - return Err(syn::Error::new_spanned( - other, - format!( - "#[{}({} = ...)] must exist as a string", - self.meta_name(), - key - ), - )); - } - } + let Meta::NameValue(syn::MetaNameValue { + lit: syn::Lit::Str(lit), .. + }) = meta else { + bail_span!(meta, "#[{}({} = ...)] must exist as a string", self.meta_name(), key) + }; + Some(lit.value()) } else { None }; @@ -202,12 +186,7 @@ impl ItemMetaInner { .. }) => lit.value, Meta::Path(_) => true, - other => { - return Err(syn::Error::new_spanned( - other, - format!("#[{}({})] is expected", self.meta_name(), key), - )) - } + _ => bail_span!(meta, "#[{}({})] is expected", self.meta_name(), key), } } else { false @@ -252,10 +231,7 @@ pub(crate) trait ItemMeta: Sized { fn new_meta_error(&self, msg: &str) -> syn::Error { let inner = self.inner(); - syn::Error::new_spanned( - &inner.meta_ident, - format!("#[{}] {}", inner.meta_name(), msg), - ) + err_span!(inner.meta_ident, "#[{}] {}", inner.meta_name(), msg) } } pub(crate) struct SimpleItemMeta(pub ItemMetaInner); @@ -301,25 +277,22 @@ impl ClassItemMeta { pub fn class_name(&self) -> Result { const KEY: &str = "name"; let inner = self.inner(); - let value = if let Some((_, meta)) = inner.meta_map.get(KEY) { + if let Some((_, meta)) = inner.meta_map.get(KEY) { match meta { Meta::NameValue(syn::MetaNameValue { lit: syn::Lit::Str(lit), .. - }) => Some(lit.value()), - Meta::Path(_) => Some(inner.item_name()), - _ => None, + }) => return Ok(lit.value()), + Meta::Path(_) => return Ok(inner.item_name()), + _ => {} } - } else { - None - }.ok_or_else(|| syn::Error::new_spanned( - &inner.meta_ident, - format!( - "#[{attr_name}(name = ...)] must exist as a string. Try #[{attr_name}(name)] to use rust type name.", - attr_name=inner.meta_name() - ), - ))?; - Ok(value) + } + bail_span!( + inner.meta_ident, + "#[{attr_name}(name = ...)] must exist as a string. Try \ + #[{attr_name}(name)] to use rust type name.", + attr_name = inner.meta_name() + ) } pub fn base(&self) -> Result> { @@ -364,21 +337,15 @@ impl ClassItemMeta { // pub fn mandatory_module(&self) -> Result { // let inner = self.inner(); // let value = self.module().ok().flatten(). - // ok_or_else(|| syn::Error::new_spanned( - // &inner.meta_ident, - // format!( - // "#[{attr_name}(module = ...)] must exist as a string. Built-in module is not allowed here.", - // attr_name=inner.meta_name() - // ), + // ok_or_else(|| err_span!( + // inner.meta_ident, + // "#[{attr_name}(module = ...)] must exist as a string. Built-in module is not allowed here.", + // attr_name = inner.meta_name() // ))?; // Ok(value) // } } -pub(crate) fn path_eq(path: &Path, s: &str) -> bool { - path.get_ident().map_or(false, |id| id == s) -} - pub(crate) trait AttributeExt: SynAttributeExt { fn promoted_nested(&self) -> Result; fn ident_and_promoted_nested(&self) -> Result<(&Ident, PunctuatedNestedMeta)>; @@ -392,13 +359,10 @@ impl AttributeExt for Attribute { fn promoted_nested(&self) -> Result { let list = self.promoted_list().map_err(|mut e| { let name = self.get_ident().unwrap().to_string(); - e.combine(syn::Error::new_spanned( + e.combine(err_span!( self, - format!( - "#[{name} = \"...\"] cannot be a name/value, you probably meant \ - #[{name}(name = \"...\")]", - name = name, - ), + "#[{name} = \"...\"] cannot be a name/value, you probably meant \ + #[{name}(name = \"...\")]", )); e })?; @@ -457,7 +421,7 @@ impl AttributeExt for Attribute { let has_name = list .nested .iter() - .any(|nmeta| nmeta.get_path().map_or(false, |p| path_eq(p, name))); + .any(|nmeta| nmeta.get_path().map_or(false, |p| p.is_ident(name))); if !has_name { list.nested.push(new_item()) } @@ -476,7 +440,7 @@ pub(crate) fn pyclass_ident_and_attrs(item: &syn::Item) -> Result<(&Ident, &[Att .into_iter() .exactly_one() .map_err(|_| { - syn::Error::new_spanned( + err_span!( item_use, "#[pyclass] can only be on single name use statement", ) @@ -484,10 +448,10 @@ pub(crate) fn pyclass_ident_and_attrs(item: &syn::Item) -> Result<(&Ident, &[Att &item_use.attrs, ), other => { - return Err(syn::Error::new_spanned( + bail_span!( other, "#[pyclass] can only be on a struct, enum or use declaration", - )) + ) } }) } @@ -570,7 +534,7 @@ where } } UseTree::Glob(glob) => { - return Err(syn::Error::new_spanned(glob, "#[py*] doesn't allow '*'")) + bail_span!(glob, "#[py*] doesn't allow '*'") } } Ok(())