Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Collecting multiple attribute error #4243

Merged
merged 12 commits into from
Jul 20, 2024
Merged

Collecting multiple attribute error #4243

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
6231d2a
collecting multiple errors
Cheukting Jun 12, 2024
b9da077
collecting errors from different fileds
Cheukting Jun 13, 2024
978c2f1
adding changelog
Cheukting Jun 13, 2024
5ccd8a4
Adding UI test
Cheukting Jun 14, 2024
1289c9b
refactoring
Cheukting Jun 14, 2024
04b237a
Update pyo3-macros-backend/src/attributes.rs
Cheukting Jul 13, 2024
020e28a
Update newsfragments/4243.changed.md
Cheukting Jul 13, 2024
bce031d
Update tests/ui/invalid_pyclass_args.rs
Cheukting Jul 13, 2024
50ec829
using pural for names
Cheukting Jul 13, 2024
08e372b
get rid of internidiate field_options_res
Cheukting Jul 13, 2024
d5b1859
reset ordering
davidhewitt Jul 20, 2024
d9a3501
Merge branch 'main' into mul_error
davidhewitt Jul 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions newsfragments/4243.changed.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Reporting multiple errors from proc-macros attributes.
Cheukting marked this conversation as resolved.
Show resolved Hide resolved
27 changes: 21 additions & 6 deletions pyo3-macros-backend/src/attributes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,13 +233,28 @@ pub fn take_attributes(

pub fn take_pyo3_options<T: Parse>(attrs: &mut Vec<syn::Attribute>) -> Result<Vec<T>> {
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)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
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!

}
78 changes: 51 additions & 27 deletions pyo3-macros-backend/src/pyclass.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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::<Result<_>>()?,
syn::Fields::Unnamed(fields) => fields
.unnamed
.iter_mut()
.map(|field| {
FieldPyO3Options::take_pyo3_options(&mut field.attrs)
.map(move |options| (&*field, options))
})
.collect::<Result<_>>()?,
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<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 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::<Vec<_>>();

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 {
Expand Down
Loading