From 6231d2ad4911c6856a67ae4077b8288d8565f087 Mon Sep 17 00:00:00 2001 From: Cheukting Date: Thu, 13 Jun 2024 00:03:55 +0100 Subject: [PATCH 01/11] collecting multiple errors --- pyo3-macros-backend/src/attributes.rs | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/pyo3-macros-backend/src/attributes.rs b/pyo3-macros-backend/src/attributes.rs index 02af17b618b..8f7d32abce9 100644 --- a/pyo3-macros-backend/src/attributes.rs +++ b/pyo3-macros-backend/src/attributes.rs @@ -233,13 +233,28 @@ pub fn take_attributes( pub fn take_pyo3_options(attrs: &mut Vec) -> Result> { let mut out = Vec::new(); - take_attributes(attrs, |attr| { - if let Some(options) = get_pyo3_options(attr)? { - out.extend(options); + let mut allerr = Vec::new(); + take_attributes(attrs, |attr| match get_pyo3_options(attr) { + Ok(result) => { + if let Some(options) = result { + out.extend(options); + Ok(true) + } else { + Ok(false) + } + } + Err(err) => { + allerr.extend(err); Ok(true) - } else { - Ok(false) } })?; - Ok(out) + if !allerr.is_empty() { + let mut error = allerr[0].clone(); + for err in &allerr[1..] { + error.combine(err.clone()); + } + Err(error) + } else { + Ok(out) + } } From b9da07752e22937379a36a556211d8e18cd3ea05 Mon Sep 17 00:00:00 2001 From: Cheukting Date: Thu, 13 Jun 2024 10:45:46 +0100 Subject: [PATCH 02/11] collecting errors from different fileds --- pyo3-macros-backend/src/pyclass.rs | 78 +++++++++++++++++++----------- 1 file changed, 51 insertions(+), 27 deletions(-) diff --git a/pyo3-macros-backend/src/pyclass.rs b/pyo3-macros-backend/src/pyclass.rs index 403e5e8e9dc..7a10edbe58c 100644 --- a/pyo3-macros-backend/src/pyclass.rs +++ b/pyo3-macros-backend/src/pyclass.rs @@ -230,34 +230,58 @@ pub fn build_py_class( For an explanation, see https://pyo3.rs/latest/class.html#no-generic-parameters" ); - let mut field_options: Vec<(&syn::Field, FieldPyO3Options)> = match &mut class.fields { - syn::Fields::Named(fields) => fields - .named - .iter_mut() - .map(|field| { - FieldPyO3Options::take_pyo3_options(&mut field.attrs) - .map(move |options| (&*field, options)) - }) - .collect::>()?, - syn::Fields::Unnamed(fields) => fields - .unnamed - .iter_mut() - .map(|field| { - FieldPyO3Options::take_pyo3_options(&mut field.attrs) - .map(move |options| (&*field, options)) - }) - .collect::>()?, - syn::Fields::Unit => { - if let Some(attr) = args.options.set_all { - return Err(syn::Error::new_spanned(attr, UNIT_SET)); - }; - if let Some(attr) = args.options.get_all { - return Err(syn::Error::new_spanned(attr, UNIT_GET)); - }; - // No fields for unit struct - Vec::new() + let mut field_options_res: Vec> = + match &mut class.fields { + syn::Fields::Named(fields) => fields + .named + .iter_mut() + .map(|field| { + FieldPyO3Options::take_pyo3_options(&mut field.attrs) + .map(move |options| (&*field, options)) + }) + .collect::>(), + syn::Fields::Unnamed(fields) => fields + .unnamed + .iter_mut() + .map(|field| { + FieldPyO3Options::take_pyo3_options(&mut field.attrs) + .map(move |options| (&*field, options)) + }) + .collect::>(), + syn::Fields::Unit => { + if let Some(attr) = args.options.set_all { + return Err(syn::Error::new_spanned(attr, UNIT_SET)); + }; + if let Some(attr) = args.options.get_all { + return Err(syn::Error::new_spanned(attr, UNIT_GET)); + }; + // No fields for unit struct + Vec::new() + } + }; + + // handle error here + + let mut allerr = Vec::new(); + + let mut field_options: Vec<(&syn::Field, FieldPyO3Options)> = field_options_res + .drain(..) + .filter_map(|result| match result { + Err(err) => { + allerr.push(err.clone()); + None + } + Ok(options) => Some(options), + }) + .collect::>(); + + if !allerr.is_empty() { + let mut error = allerr[0].clone(); + for err in &allerr[1..] { + error.combine(err.clone()); } - }; + return Err(error); + } if let Some(attr) = args.options.get_all { for (_, FieldPyO3Options { get, .. }) in &mut field_options { From 978c2f1a649c92fdea030e00392e79ac38ebafdb Mon Sep 17 00:00:00 2001 From: Cheukting Date: Thu, 13 Jun 2024 10:57:01 +0100 Subject: [PATCH 03/11] adding changelog --- newsfragments/4243.changed.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 newsfragments/4243.changed.md diff --git a/newsfragments/4243.changed.md b/newsfragments/4243.changed.md new file mode 100644 index 00000000000..36110492d28 --- /dev/null +++ b/newsfragments/4243.changed.md @@ -0,0 +1 @@ +Reporting multiple errors from proc-macros attributes. From 5ccd8a4bc30f15e27a1ee4414043a643dd5e33a5 Mon Sep 17 00:00:00 2001 From: Cheukting Date: Fri, 14 Jun 2024 14:36:50 +0100 Subject: [PATCH 04/11] Adding UI test --- tests/ui/invalid_pyclass_args.rs | 9 +++++++++ tests/ui/invalid_pyclass_args.stderr | 18 ++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/tests/ui/invalid_pyclass_args.rs b/tests/ui/invalid_pyclass_args.rs index f74fa49d8de..cff4ab6e6a3 100644 --- a/tests/ui/invalid_pyclass_args.rs +++ b/tests/ui/invalid_pyclass_args.rs @@ -76,4 +76,13 @@ struct InvalidOrderedStruct { inner: i32 } +#[pyclass] +struct Example { + #[pyo3(foo)] + #[pyo3(blah)] + x: i32, + #[pyo3(pop)] + y: i32, +} + fn main() {} diff --git a/tests/ui/invalid_pyclass_args.stderr b/tests/ui/invalid_pyclass_args.stderr index 23d3c3bbc64..8263991fd22 100644 --- a/tests/ui/invalid_pyclass_args.stderr +++ b/tests/ui/invalid_pyclass_args.stderr @@ -82,6 +82,24 @@ error: The `ord` option requires the `eq` option. 74 | #[pyclass(ord)] | ^^^ +error: expected one of: `get`, `set`, `name` + --> tests/ui/invalid_pyclass_args.rs:81:12 + | +81 | #[pyo3(foo)] + | ^^^ + +error: expected one of: `get`, `set`, `name` + --> tests/ui/invalid_pyclass_args.rs:82:12 + | +82 | #[pyo3(blah)] + | ^^^^ + +error: expected one of: `get`, `set`, `name` + --> tests/ui/invalid_pyclass_args.rs:84:12 + | +84 | #[pyo3(pop)] + | ^^^ + error[E0592]: duplicate definitions with name `__pymethod___richcmp____` --> tests/ui/invalid_pyclass_args.rs:36:1 | From 1289c9be6ca61dc8c05c0675cde98e9f1bc7911a Mon Sep 17 00:00:00 2001 From: Cheukting Date: Fri, 14 Jun 2024 17:18:53 +0100 Subject: [PATCH 05/11] refactoring --- pyo3-macros-backend/src/attributes.rs | 31 +++++++++++++++++++-------- pyo3-macros-backend/src/pyclass.rs | 16 +++++--------- 2 files changed, 27 insertions(+), 20 deletions(-) diff --git a/pyo3-macros-backend/src/attributes.rs b/pyo3-macros-backend/src/attributes.rs index 8f7d32abce9..c2dfb76293a 100644 --- a/pyo3-macros-backend/src/attributes.rs +++ b/pyo3-macros-backend/src/attributes.rs @@ -233,7 +233,7 @@ pub fn take_attributes( pub fn take_pyo3_options(attrs: &mut Vec) -> Result> { let mut out = Vec::new(); - let mut allerr = Vec::new(); + let mut all_error = ErrorCombiner(None); take_attributes(attrs, |attr| match get_pyo3_options(attr) { Ok(result) => { if let Some(options) = result { @@ -244,17 +244,30 @@ pub fn take_pyo3_options(attrs: &mut Vec) -> Result { - allerr.extend(err); + all_error.combine(err); Ok(true) } })?; - if !allerr.is_empty() { - let mut error = allerr[0].clone(); - for err in &allerr[1..] { - error.combine(err.clone()); + all_error.ensure_empty()?; + Ok(out) +} + +pub struct ErrorCombiner(pub Option); + +impl ErrorCombiner { + pub fn combine(&mut self, error: syn::Error) { + if let Some(existing) = &mut self.0 { + existing.combine(error); + } else { + self.0 = Some(error); + } + } + + pub fn ensure_empty(self) -> Result<()> { + if let Some(error) = self.0 { + Err(error) + } else { + Ok(()) } - Err(error) - } else { - Ok(out) } } diff --git a/pyo3-macros-backend/src/pyclass.rs b/pyo3-macros-backend/src/pyclass.rs index 7a10edbe58c..249d14bd221 100644 --- a/pyo3-macros-backend/src/pyclass.rs +++ b/pyo3-macros-backend/src/pyclass.rs @@ -2,8 +2,8 @@ use std::borrow::Cow; use crate::attributes::kw::frozen; use crate::attributes::{ - self, kw, take_pyo3_options, CrateAttribute, ExtendsAttribute, FreelistAttribute, - ModuleAttribute, NameAttribute, NameLitStr, RenameAllAttribute, + self, kw, take_pyo3_options, CrateAttribute, ErrorCombiner, ExtendsAttribute, + FreelistAttribute, ModuleAttribute, NameAttribute, NameLitStr, RenameAllAttribute, }; use crate::deprecations::Deprecations; use crate::konst::{ConstAttributes, ConstSpec}; @@ -262,26 +262,20 @@ pub fn build_py_class( // handle error here - let mut allerr = Vec::new(); + let mut all_error = ErrorCombiner(None); let mut field_options: Vec<(&syn::Field, FieldPyO3Options)> = field_options_res .drain(..) .filter_map(|result| match result { Err(err) => { - allerr.push(err.clone()); + all_error.combine(err); None } Ok(options) => Some(options), }) .collect::>(); - if !allerr.is_empty() { - let mut error = allerr[0].clone(); - for err in &allerr[1..] { - error.combine(err.clone()); - } - return Err(error); - } + all_error.ensure_empty()?; if let Some(attr) = args.options.get_all { for (_, FieldPyO3Options { get, .. }) in &mut field_options { From 04b237afc53124091251b34e41126e992196d98e Mon Sep 17 00:00:00 2001 From: Cheuk Ting Ho Date: Sat, 13 Jul 2024 10:41:05 +0300 Subject: [PATCH 06/11] Update pyo3-macros-backend/src/attributes.rs Co-authored-by: David Hewitt --- pyo3-macros-backend/src/attributes.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyo3-macros-backend/src/attributes.rs b/pyo3-macros-backend/src/attributes.rs index c2dfb76293a..ccdef56a36c 100644 --- a/pyo3-macros-backend/src/attributes.rs +++ b/pyo3-macros-backend/src/attributes.rs @@ -233,7 +233,7 @@ pub fn take_attributes( pub fn take_pyo3_options(attrs: &mut Vec) -> Result> { let mut out = Vec::new(); - let mut all_error = ErrorCombiner(None); + let mut all_errors = ErrorCombiner(None); take_attributes(attrs, |attr| match get_pyo3_options(attr) { Ok(result) => { if let Some(options) = result { From 020e28a263b028f6ab7cafb436927b6e856c9da7 Mon Sep 17 00:00:00 2001 From: Cheuk Ting Ho Date: Sat, 13 Jul 2024 10:41:12 +0300 Subject: [PATCH 07/11] Update newsfragments/4243.changed.md Co-authored-by: David Hewitt --- newsfragments/4243.changed.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/newsfragments/4243.changed.md b/newsfragments/4243.changed.md index 36110492d28..d9464ab37aa 100644 --- a/newsfragments/4243.changed.md +++ b/newsfragments/4243.changed.md @@ -1 +1 @@ -Reporting multiple errors from proc-macros attributes. +Report multiple errors from `#[pyclass]` and `#[pyo3(..)]` attributes. From bce031d4900863f55c4c48f205e4377e2d6ea65d Mon Sep 17 00:00:00 2001 From: Cheuk Ting Ho Date: Sat, 13 Jul 2024 10:41:30 +0300 Subject: [PATCH 08/11] Update tests/ui/invalid_pyclass_args.rs Co-authored-by: David Hewitt --- tests/ui/invalid_pyclass_args.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ui/invalid_pyclass_args.rs b/tests/ui/invalid_pyclass_args.rs index cff4ab6e6a3..6c2e3cc0b51 100644 --- a/tests/ui/invalid_pyclass_args.rs +++ b/tests/ui/invalid_pyclass_args.rs @@ -77,7 +77,7 @@ struct InvalidOrderedStruct { } #[pyclass] -struct Example { +struct MultipleErrors { #[pyo3(foo)] #[pyo3(blah)] x: i32, From 50ec829e8a64d6024a8d98c2e85753f003650e81 Mon Sep 17 00:00:00 2001 From: Cheukting Date: Sat, 13 Jul 2024 10:04:29 +0200 Subject: [PATCH 09/11] using pural for names --- pyo3-macros-backend/src/attributes.rs | 4 ++-- pyo3-macros-backend/src/pyclass.rs | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pyo3-macros-backend/src/attributes.rs b/pyo3-macros-backend/src/attributes.rs index ccdef56a36c..0f7129a68ff 100644 --- a/pyo3-macros-backend/src/attributes.rs +++ b/pyo3-macros-backend/src/attributes.rs @@ -244,11 +244,11 @@ pub fn take_pyo3_options(attrs: &mut Vec) -> Result { - all_error.combine(err); + all_errors.combine(err); Ok(true) } })?; - all_error.ensure_empty()?; + all_errors.ensure_empty()?; Ok(out) } diff --git a/pyo3-macros-backend/src/pyclass.rs b/pyo3-macros-backend/src/pyclass.rs index 249d14bd221..2287fe9d601 100644 --- a/pyo3-macros-backend/src/pyclass.rs +++ b/pyo3-macros-backend/src/pyclass.rs @@ -262,20 +262,20 @@ pub fn build_py_class( // handle error here - let mut all_error = ErrorCombiner(None); + let mut all_errors = ErrorCombiner(None); let mut field_options: Vec<(&syn::Field, FieldPyO3Options)> = field_options_res .drain(..) .filter_map(|result| match result { Err(err) => { - all_error.combine(err); + all_errors.combine(err); None } Ok(options) => Some(options), }) .collect::>(); - all_error.ensure_empty()?; + all_errors.ensure_empty()?; if let Some(attr) = args.options.get_all { for (_, FieldPyO3Options { get, .. }) in &mut field_options { From 08e372b3a57fd3e536104634dcc7d07b9221d2f0 Mon Sep 17 00:00:00 2001 From: Cheukting Date: Sat, 13 Jul 2024 10:13:40 +0200 Subject: [PATCH 10/11] get rid of internidiate field_options_res --- pyo3-macros-backend/src/pyclass.rs | 80 ++++++++++++++---------------- 1 file changed, 38 insertions(+), 42 deletions(-) diff --git a/pyo3-macros-backend/src/pyclass.rs b/pyo3-macros-backend/src/pyclass.rs index 2287fe9d601..be46440c719 100644 --- a/pyo3-macros-backend/src/pyclass.rs +++ b/pyo3-macros-backend/src/pyclass.rs @@ -230,50 +230,46 @@ pub fn build_py_class( For an explanation, see https://pyo3.rs/latest/class.html#no-generic-parameters" ); - let mut field_options_res: Vec> = - match &mut class.fields { - syn::Fields::Named(fields) => fields - .named - .iter_mut() - .map(|field| { - FieldPyO3Options::take_pyo3_options(&mut field.attrs) - .map(move |options| (&*field, options)) - }) - .collect::>(), - syn::Fields::Unnamed(fields) => fields - .unnamed - .iter_mut() - .map(|field| { - FieldPyO3Options::take_pyo3_options(&mut field.attrs) - .map(move |options| (&*field, options)) - }) - .collect::>(), - syn::Fields::Unit => { - if let Some(attr) = args.options.set_all { - return Err(syn::Error::new_spanned(attr, UNIT_SET)); - }; - if let Some(attr) = args.options.get_all { - return Err(syn::Error::new_spanned(attr, UNIT_GET)); - }; - // No fields for unit struct - Vec::new() - } - }; - - // handle error here - let mut all_errors = ErrorCombiner(None); - let mut field_options: Vec<(&syn::Field, FieldPyO3Options)> = field_options_res - .drain(..) - .filter_map(|result| match result { - Err(err) => { - all_errors.combine(err); - None - } - Ok(options) => Some(options), - }) - .collect::>(); + let mut field_options: Vec<(&syn::Field, FieldPyO3Options)> = match &mut class.fields { + syn::Fields::Unnamed(fields) => fields + .unnamed + .iter_mut() + .filter_map( + |field| match FieldPyO3Options::take_pyo3_options(&mut field.attrs) { + Ok(options) => Some((&*field, options)), + Err(e) => { + all_errors.combine(e); + None + } + }, + ) + .collect::>(), + syn::Fields::Named(fields) => fields + .named + .iter_mut() + .filter_map( + |field| match FieldPyO3Options::take_pyo3_options(&mut field.attrs) { + Ok(options) => Some((&*field, options)), + Err(e) => { + all_errors.combine(e); + None + } + }, + ) + .collect::>(), + syn::Fields::Unit => { + if let Some(attr) = args.options.set_all { + return Err(syn::Error::new_spanned(attr, UNIT_SET)); + }; + if let Some(attr) = args.options.get_all { + return Err(syn::Error::new_spanned(attr, UNIT_GET)); + }; + // No fields for unit struct + Vec::new() + } + }; all_errors.ensure_empty()?; From d5b185933d62dc7f7c9f0fd8dfb6f8181e0a0669 Mon Sep 17 00:00:00 2001 From: David Hewitt Date: Sat, 20 Jul 2024 07:02:20 +0100 Subject: [PATCH 11/11] reset ordering --- pyo3-macros-backend/src/pyclass.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pyo3-macros-backend/src/pyclass.rs b/pyo3-macros-backend/src/pyclass.rs index be46440c719..ae33f93493d 100644 --- a/pyo3-macros-backend/src/pyclass.rs +++ b/pyo3-macros-backend/src/pyclass.rs @@ -233,8 +233,8 @@ pub fn build_py_class( let mut all_errors = ErrorCombiner(None); let mut field_options: Vec<(&syn::Field, FieldPyO3Options)> = match &mut class.fields { - syn::Fields::Unnamed(fields) => fields - .unnamed + syn::Fields::Named(fields) => fields + .named .iter_mut() .filter_map( |field| match FieldPyO3Options::take_pyo3_options(&mut field.attrs) { @@ -246,8 +246,8 @@ pub fn build_py_class( }, ) .collect::>(), - syn::Fields::Named(fields) => fields - .named + syn::Fields::Unnamed(fields) => fields + .unnamed .iter_mut() .filter_map( |field| match FieldPyO3Options::take_pyo3_options(&mut field.attrs) {