Collecting multiple attribute error #4243
Conversation
Cheukting
changed the title
There was a problem hiding this comment.
Thanks, this looks like a great discovery!
To test this, we should update the UI tests to make use of these attributes. For example, see tests/ui/invalid_pyclass_args.rs.
At the moment it has a separate class for each invalid entry. We could instead write pieces which can be combined together, for example:
#[pyclass(
extend = pyo3::types::PyDict, // typo in the key
extends = "PyDict", // Needs to be an ident, not a string
)]
struct InvalidExtends;... and so on.
See also invalid_pymethods.rs - we have loads of #[pymethods] impl MyClass blocks, and we should be able to combine them all into one with this functionality!
| if !allerr.is_empty() { | ||
| let mut error = allerr[0].clone(); | ||
| for err in &allerr[1..] { | ||
| error.combine(err.clone()); | ||
| } | ||
| Err(error) | ||
| } else { | ||
| Ok(out) | ||
| } |
There was a problem hiding this comment.
Ah, this error.combine() function is extremely handy!
I think now that we know about this, to avoid the allerr vec we can probably have something like this:
struct ErrorCombiner(Option<syn::Error>);
impl ErrorCombiner {
fn combine(&mut self, error: syn::Error) {
if let Some(existing) = &mut self.0 {
existing.combine(error);
} else {
self.0 = Some(error);
}
}
fn ensure_empty(self) -> Result<()> {
if let Some(error) = self.0 {
Err(error)
} else {
None
}
}Above, instead of extending the Vec we would combine the error immediately, and then at the end of the function we could just check the combiner is empty:
| if !allerr.is_empty() { | |
| let mut error = allerr[0].clone(); | |
| for err in &allerr[1..] { | |
| error.combine(err.clone()); | |
| } | |
| Err(error) | |
| } else { | |
| Ok(out) | |
| } | |
| error_combiner.ensure_empty()?; | |
| Ok(out) |
I think this pattern would be useful in a lot of places in the macro code!
|
Thanks for the suggestions @davidhewitt i have added the test and have refactored as suggested. |
There was a problem hiding this comment.
Thanks, I love these kinds of UX improvements! Just a couple of thoughts related to the implementation still.
(And sorry it took me a few days here, was focussing on #4266 for a while.)
pyo3-macros-backend/src/pyclass.rs
Outdated
| let mut field_options_res: Vec<Result<(&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::<Vec<_>>(), | ||
| syn::Fields::Unnamed(fields) => fields | ||
| .unnamed | ||
| .iter_mut() | ||
| .map(|field| { | ||
| FieldPyO3Options::take_pyo3_options(&mut field.attrs) | ||
| .map(move |options| (&*field, options)) | ||
| }) | ||
| .collect::<Vec<_>>(), | ||
| 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_error = 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); | ||
| None | ||
| } | ||
| Ok(options) => Some(options), | ||
| }) | ||
| .collect::<Vec<_>>(); |
There was a problem hiding this comment.
I think we should now be able to simplify here and go back to something more like the original implementation and drop the need for the intermediate field_options_res.
I think the idea would be to set up all_errors before the match &mut class.fields. And then instead of collecting into vec-of-results I think it would be possible to swap the .map for .filter_map.
Something like
.filter_map(|field| {
match FieldPyO3Options::take_pyo3_options(&mut field.attrs) {
Ok(options) => Some((&*field, options)),
Err(e) => {
all_errors.combine(e);
None
}
}
})
|
Thank you for your patience. I was busy with EuroPython. I plan to finish this PR this weekend or next week. |
Co-authored-by: David Hewitt <mail@davidhewitt.dev>
Co-authored-by: David Hewitt <mail@davidhewitt.dev>
Co-authored-by: David Hewitt <mail@davidhewitt.dev>
|
(I took the liberty to fix the new merge conflicts.) |
|
Thank you @davidhewitt |
Attempt to clos e #2892(Edit by @davidhewitt - I think there's more we can do with#[pymethods]probably, though might be a fair bit more work.)I am not sure how to add the test in CI but I have tested manually with the following:
Originally it gave:
Now it gives: