From 08c8b845aab760007f4d0185a827bab3eae4d02c Mon Sep 17 00:00:00 2001 From: mejrs <> Date: Tue, 18 Oct 2022 21:14:22 +0200 Subject: [PATCH] Create better error spans/messages --- pyo3-macros-backend/src/pyclass.rs | 126 +++++++++++++++----------- tests/ui/get_set_all.stderr | 16 ++-- tests/ui/invalid_property_args.stderr | 2 +- 3 files changed, 80 insertions(+), 64 deletions(-) diff --git a/pyo3-macros-backend/src/pyclass.rs b/pyo3-macros-backend/src/pyclass.rs index 52c80b2c..e15900b1 100644 --- a/pyo3-macros-backend/src/pyclass.rs +++ b/pyo3-macros-backend/src/pyclass.rs @@ -1,7 +1,6 @@ // Copyright (c) 2017-present PyO3 Project and Contributors use std::borrow::Cow; -use std::mem; use crate::attributes::{ self, kw, take_pyo3_options, CrateAttribute, ExtendsAttribute, FreelistAttribute, @@ -242,10 +241,10 @@ pub fn build_py_class( .collect::>()?, syn::Fields::Unit => { if let Some(attr) = args.options.set_all { - return Err(syn::Error::new(attr.span(), UNIT_SET)); + return Err(syn::Error::new_spanned(attr, UNIT_SET)); }; if let Some(attr) = args.options.get_all { - return Err(syn::Error::new(attr.span(), UNIT_GET)); + return Err(syn::Error::new_spanned(attr, UNIT_GET)); }; // No fields for unit struct Vec::new() @@ -254,16 +253,16 @@ pub fn build_py_class( if let Some(attr) = args.options.get_all { for (_, FieldPyO3Options { get, .. }) in &mut field_options { - if mem::replace(get, true) { - return Err(syn::Error::new(attr.span(), DUPE_GET)); + if let Some(old_get) = get.replace(Annotated::Struct(attr)) { + return Err(syn::Error::new(old_get.span(), DUPE_GET)); } } } if let Some(attr) = args.options.set_all { for (_, FieldPyO3Options { set, .. }) in &mut field_options { - if mem::replace(set, true) { - return Err(syn::Error::new(attr.span(), DUPE_SET)); + if let Some(old_set) = set.replace(Annotated::Struct(attr)) { + return Err(syn::Error::new(old_set.span(), DUPE_SET)); } } } @@ -271,10 +270,24 @@ pub fn build_py_class( impl_class(&class.ident, &args, doc, field_options, methods_type, krate) } +enum Annotated { + Field(X), + Struct(Y), +} + +impl Spanned for Annotated { + fn span(&self) -> Span { + match self { + Self::Field(x) => x.span(), + Self::Struct(y) => y.span(), + } + } +} + /// `#[pyo3()]` options for pyclass fields struct FieldPyO3Options { - get: bool, - set: bool, + get: Option>, + set: Option>, name: Option, } @@ -302,33 +315,27 @@ impl Parse for FieldPyO3Option { impl FieldPyO3Options { fn take_pyo3_options(attrs: &mut Vec) -> Result { let mut options = FieldPyO3Options { - get: false, - set: false, + get: None, + set: None, name: None, }; for option in take_pyo3_options(attrs)? { match option { FieldPyO3Option::Get(kw) => { - ensure_spanned!( - !options.get, - kw.span() => "`get` may only be specified once" - ); - options.get = true; + if options.get.replace(Annotated::Field(kw)).is_some() { + return Err(syn::Error::new(kw.span(), UNIQUE_GET)); + } } FieldPyO3Option::Set(kw) => { - ensure_spanned!( - !options.set, - kw.span() => "`set` may only be specified once" - ); - options.set = true; + if options.set.replace(Annotated::Field(kw)).is_some() { + return Err(syn::Error::new(kw.span(), UNIQUE_SET)); + } } FieldPyO3Option::Name(name) => { - ensure_spanned!( - options.name.is_none(), - name.span() => "`name` may only be specified once" - ); - options.name = Some(name); + if options.name.replace(name).is_some() { + return Err(syn::Error::new(options.name.span(), UNIQUE_NAME)); + } } } } @@ -697,39 +704,42 @@ fn descriptors_to_items( field_options: Vec<(&syn::Field, FieldPyO3Options)>, ) -> syn::Result> { let ty = syn::parse_quote!(#cls); - field_options - .into_iter() - .enumerate() - .flat_map(|(field_index, (field, options))| { - let name_err = if options.name.is_some() && !options.get && !options.set { - Some(Err(err_spanned!(options.name.as_ref().unwrap().span() => "`name` is useless without `get` or `set`"))) - } else { - None - }; + let mut items = Vec::new(); + for (field_index, (field, options)) in field_options.into_iter().enumerate() { + if let FieldPyO3Options { + name: Some(name), + get: None, + set: None, + } = options + { + return Err(syn::Error::new_spanned(name, USELESS_NAME)); + } - let getter = if options.get { - Some(impl_py_getter_def(&ty, PropertyType::Descriptor { + if options.get.is_some() { + let getter = impl_py_getter_def( + &ty, + PropertyType::Descriptor { field_index, field, - python_name: options.name.as_ref() - })) - } else { - None - }; + python_name: options.name.as_ref(), + }, + )?; + items.push(getter); + } - let setter = if options.set { - Some(impl_py_setter_def(&ty, PropertyType::Descriptor { + if options.set.is_some() { + let setter = impl_py_setter_def( + &ty, + PropertyType::Descriptor { field_index, field, - python_name: options.name.as_ref() - })) - } else { - None - }; - - name_err.into_iter().chain(getter).chain(setter) - }) - .collect::>() + python_name: options.name.as_ref(), + }, + )?; + items.push(setter); + }; + } + Ok(items) } fn impl_pytypeinfo( @@ -1119,9 +1129,15 @@ fn define_inventory_class(inventory_class_name: &syn::Ident) -> TokenStream { } } -const DUPE_SET: &str = "duplicate `set` - the struct is already annotated with `set_all`"; -const DUPE_GET: &str = "duplicate `get` - the struct is already annotated with `get_all`"; +const UNIQUE_GET: &str = "`get` may only be specified once"; +const UNIQUE_SET: &str = "`set` may only be specified once"; +const UNIQUE_NAME: &str = "`name` may only be specified once"; + +const DUPE_SET: &str = "useless `set` - the struct is already annotated with `set_all`"; +const DUPE_GET: &str = "useless `get` - the struct is already annotated with `get_all`"; const UNIT_GET: &str = "`get_all` on an unit struct does nothing, because unit structs have no fields"; const UNIT_SET: &str = "`set_all` on an unit struct does nothing, because unit structs have no fields"; + +const USELESS_NAME: &str = "`name` is useless without `get` or `set`"; diff --git a/tests/ui/get_set_all.stderr b/tests/ui/get_set_all.stderr index 4d52f817..607d5f96 100644 --- a/tests/ui/get_set_all.stderr +++ b/tests/ui/get_set_all.stderr @@ -4,11 +4,11 @@ error: `set_all` on an unit struct does nothing, because unit structs have no fi 3 | #[pyclass(set_all)] | ^^^^^^^ -error: duplicate `set` - the struct is already annotated with `set_all` - --> tests/ui/get_set_all.rs:6:11 +error: useless `set` - the struct is already annotated with `set_all` + --> tests/ui/get_set_all.rs:8:12 | -6 | #[pyclass(set_all)] - | ^^^^^^^ +8 | #[pyo3(set)] + | ^^^ error: `get_all` on an unit struct does nothing, because unit structs have no fields --> tests/ui/get_set_all.rs:12:11 @@ -16,8 +16,8 @@ error: `get_all` on an unit struct does nothing, because unit structs have no fi 12 | #[pyclass(get_all)] | ^^^^^^^ -error: duplicate `get` - the struct is already annotated with `get_all` - --> tests/ui/get_set_all.rs:15:11 +error: useless `get` - the struct is already annotated with `get_all` + --> tests/ui/get_set_all.rs:17:12 | -15 | #[pyclass(get_all)] - | ^^^^^^^ +17 | #[pyo3(get)] + | ^^^ diff --git a/tests/ui/invalid_property_args.stderr b/tests/ui/invalid_property_args.stderr index bf8976a6..a41b6c79 100644 --- a/tests/ui/invalid_property_args.stderr +++ b/tests/ui/invalid_property_args.stderr @@ -44,4 +44,4 @@ error: `name` is useless without `get` or `set` --> tests/ui/invalid_property_args.rs:40:33 | 40 | struct NameWithoutGetSet(#[pyo3(name = "value")] i32); - | ^^^^ + | ^^^^^^^^^^^^^^