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

Jump to bottom

Emit an error if a variant of an untagged enum is annoted with #[serde(rename)] or #[serde(alias)] #2797

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Emit an error if a variant of an untagged enum is annoted with #[serde(rename)] or #[serde(alias)] #2797

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f9dddaa
Emit an error if a variant of an untagged enum is annotated with #[se…
devnetsec Aug 12, 2024
876f5b9
Made errors point to attributes instead of variants and added a compi…
devnetsec Aug 14, 2024
6d72564
Made errors point to attribute names only instead of the whole line.
devnetsec Aug 14, 2024
453d1dc
Add support for #[serde(rename_all)] as well and fix indentation.
devnetsec Aug 17, 2024
5e629d9
Move checks to check.rs
devnetsec Aug 22, 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
9 changes: 8 additions & 1 deletion serde_derive/src/internals/ast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,10 @@ impl<'a> Container<'a> {
) -> Option<Container<'a>> {
let mut attrs = attr::Container::from_ast(cx, item);

let untagged = attrs.tag() == &attr::TagType::None;

let mut data = match &item.data {
syn::Data::Enum(data) => Data::Enum(enum_from_ast(cx, &data.variants, attrs.default())),
syn::Data::Enum(data) => Data::Enum(enum_from_ast(cx, &data.variants, attrs.default(), untagged)),
syn::Data::Struct(data) => {
let (style, fields) = struct_from_ast(cx, &data.fields, None, attrs.default());
Data::Struct(style, fields)
Expand Down Expand Up @@ -141,11 +143,16 @@ fn enum_from_ast<'a>(
cx: &Ctxt,
variants: &'a Punctuated<syn::Variant, Token![,]>,
container_default: &attr::Default,
container_is_untagged: bool,
) -> Vec<Variant<'a>> {
let variants: Vec<Variant> = variants
.iter()
.map(|variant| {
let attrs = attr::Variant::from_ast(cx, variant);

if container_is_untagged && attrs.name().renamed() {
cx.error_spanned_by(&variant.ident, "renaming or adding a alias to a variant of an untagged enum does nothing");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, error should point to the each problematic attribute instead of a variant name. Also, compile tests should be added to test_suite/tests/ui

Copy link
Author

Choose a reason for hiding this comment

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

I fixed those two things. Are there any other attributes that need to have error messages like this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, much better, but it is better to point to the alias = "..." and rename = "..." parts only, so when you have multiple attributes on a single line, errors pointed to the corresponding parts of that line.

Don't known about other attributes. You may check other if they need improvements.

Copy link
Author

Choose a reason for hiding this comment

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

I made it point towards the alias and rename parts to be even more clear. I think #[serde(rename_all)] may be another attribute that has this issue, but I couldn't get that error to work in 6d72564.

let (style, fields) =
struct_from_ast(cx, &variant.fields, Some(&attrs), container_default);
Variant {
Expand Down
6 changes: 6 additions & 0 deletions serde_derive/src/internals/attr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,11 @@ impl Name {
pub fn deserialize_name(&self) -> &str {
&self.deserialize
}

/// Return whether the container name was changed for serialization or deserialization.
pub fn renamed(&self) -> bool {
self.serialize_renamed || self.deserialize_renamed || !self.deserialize_aliases.is_empty()
}

fn deserialize_aliases(&self) -> &BTreeSet<String> {
&self.deserialize_aliases
Expand Down Expand Up @@ -241,6 +246,7 @@ pub struct Container {
}

/// Styles of representing an enum.
#[derive(PartialEq)]
pub enum TagType {
/// The default.
///
Expand Down