From 5f9fffa53e8a48d9e1f32b1735542207c42f3ead Mon Sep 17 00:00:00 2001 From: Mingun Date: Tue, 8 Aug 2023 22:07:22 +0500 Subject: [PATCH] Add checks for conflicts for aliases - Check that alias is not the same as name of other field (it still can be the name of owning field/variant) - Check that aliases are unique, i. e. two different fields does not use the same alias --- serde_derive/src/internals/check.rs | 136 +++++++++++++++++- test_suite/tests/ui/conflict/alias-enum.rs | 79 ++++++++++ .../tests/ui/conflict/alias-enum.stderr | 71 +++++++++ test_suite/tests/ui/conflict/alias.rs | 40 ++++++ test_suite/tests/ui/conflict/alias.stderr | 35 +++++ 5 files changed, 360 insertions(+), 1 deletion(-) create mode 100644 test_suite/tests/ui/conflict/alias-enum.rs create mode 100644 test_suite/tests/ui/conflict/alias-enum.stderr create mode 100644 test_suite/tests/ui/conflict/alias.rs create mode 100644 test_suite/tests/ui/conflict/alias.stderr diff --git a/serde_derive/src/internals/check.rs b/serde_derive/src/internals/check.rs index 52b0f379f..df5d63f01 100644 --- a/serde_derive/src/internals/check.rs +++ b/serde_derive/src/internals/check.rs @@ -1,6 +1,8 @@ -use crate::internals::ast::{Container, Data, Field, Style}; +use crate::internals::ast::{Container, Data, Field, Style, Variant}; use crate::internals::attr::{Default, Identifier, TagType}; use crate::internals::{ungroup, Ctxt, Derive}; +use std::collections::btree_map::Entry; +use std::collections::{BTreeMap, BTreeSet}; use syn::{Member, Type}; // Cross-cutting checks that require looking at more than a single attrs object. @@ -16,6 +18,7 @@ pub fn check(cx: &Ctxt, cont: &mut Container, derive: Derive) { check_adjacent_tag_conflict(cx, cont); check_transparent(cx, cont, derive); check_from_and_try_from(cx, cont); + check_name_conflicts(cx, cont, derive); } // If some field of a tuple struct is marked #[serde(default)] then all fields @@ -475,3 +478,134 @@ fn check_from_and_try_from(cx: &Ctxt, cont: &mut Container) { ); } } + +// Checks that aliases does not repeated +fn check_name_conflicts(cx: &Ctxt, cont: &Container, derive: Derive) { + if let Derive::Deserialize = derive { + match &cont.data { + Data::Enum(variants) => check_variant_name_conflicts(cx, &variants), + Data::Struct(Style::Struct, fields) => check_field_name_conflicts(cx, fields), + _ => {} + } + } +} + +// All renames already applied +fn check_variant_name_conflicts(cx: &Ctxt, variants: &[Variant]) { + let names: BTreeSet<_> = variants + .iter() + .filter_map(|variant| { + if variant.attrs.skip_deserializing() { + None + } else { + Some(variant.attrs.name().deserialize_name().to_owned()) + } + }) + .collect(); + let mut alias_owners = BTreeMap::new(); + + for variant in variants { + let name = variant.attrs.name().deserialize_name(); + + for alias in variant.attrs.aliases().intersection(&names) { + // Aliases contains variant names, so filter them out + if alias == name { + continue; + } + + // TODO: report other variant location when this become possible + cx.error_spanned_by( + variant.original, + format!( + "alias `{}` conflicts with deserialization name of other variant", + alias + ), + ); + } + + for alias in variant.attrs.aliases() { + // Aliases contains variant names, so filter them out + if alias == name { + continue; + } + + match alias_owners.entry(alias) { + Entry::Vacant(e) => { + e.insert(variant); + } + Entry::Occupied(e) => { + // TODO: report other variant location when this become possible + cx.error_spanned_by( + variant.original, + format!( + "alias `{}` already used by variant {}", + alias, + e.get().original.ident + ), + ); + } + } + } + + check_field_name_conflicts(cx, &variant.fields); + } +} + +// All renames already applied +fn check_field_name_conflicts(cx: &Ctxt, fields: &[Field]) { + let names: BTreeSet<_> = fields + .iter() + .filter_map(|field| { + if field.attrs.skip_deserializing() { + None + } else { + Some(field.attrs.name().deserialize_name().to_owned()) + } + }) + .collect(); + let mut alias_owners = BTreeMap::new(); + + for field in fields { + let name = field.attrs.name().deserialize_name(); + + for alias in field.attrs.aliases().intersection(&names) { + // Aliases contains field names, so filter them out + if alias == name { + continue; + } + + // TODO: report other field location when this become possible + cx.error_spanned_by( + field.original, + format!( + "alias `{}` conflicts with deserialization name of other field", + alias + ), + ); + } + + for alias in field.attrs.aliases() { + // Aliases contains field names, so filter them out + if alias == name { + continue; + } + + match alias_owners.entry(alias) { + Entry::Vacant(e) => { + e.insert(field); + } + Entry::Occupied(e) => { + // TODO: report other field location when this become possible + cx.error_spanned_by( + field.original, + format!( + "alias `{}` already used by field {}", + alias, + e.get().original.ident.as_ref().unwrap() + ), + ); + } + } + } + } +} diff --git a/test_suite/tests/ui/conflict/alias-enum.rs b/test_suite/tests/ui/conflict/alias-enum.rs new file mode 100644 index 000000000..52af74b97 --- /dev/null +++ b/test_suite/tests/ui/conflict/alias-enum.rs @@ -0,0 +1,79 @@ +#![allow(non_camel_case_types)] + +use serde_derive::Deserialize; + +#[derive(Deserialize)] +enum E { + S1 { + /// Expected error on "alias b", because this is a name of other field + /// Error on "alias a" is not expected because this is a name of this field + /// Error on "alias c" is not expected because field `c` is skipped + #[serde(alias = "a", alias = "b", alias = "c")] + a: (), + + /// Expected error on "alias c", because it is already used as alias of `a` + #[serde(alias = "c")] + b: (), + + #[serde(skip_deserializing)] + c: (), + }, + + S2 { + /// Expected error on "alias c", because this is a name of other field after + /// applying rename rules + #[serde(alias = "b", alias = "c")] + a: (), + + #[serde(rename = "c")] + b: (), + }, + + #[serde(rename_all = "UPPERCASE")] + S3 { + /// Expected error on "alias B", because this is a name of field after + /// applying rename rules + #[serde(alias = "B", alias = "c")] + a: (), + b: (), + }, +} + +#[derive(Deserialize)] +enum E1 { + /// Expected error on "alias b", because this is a name of other variant + /// Error on "alias a" is not expected because this is a name of this variant + /// Error on "alias c" is not expected because variant `c` is skipped + #[serde(alias = "a", alias = "b", alias = "c")] + a, + + /// Expected error on "alias c", because it is already used as alias of `a` + #[serde(alias = "c")] + b, + + #[serde(skip_deserializing)] + c, +} + +#[derive(Deserialize)] +enum E2 { + /// Expected error on "alias c", because this is a name of other variant after + /// applying rename rules + #[serde(alias = "b", alias = "c")] + a, + + #[serde(rename = "c")] + b, +} + +#[derive(Deserialize)] +#[serde(rename_all = "UPPERCASE")] +enum E3 { + /// Expected error on "alias B", because this is a name of variant after + /// applying rename rules + #[serde(alias = "B", alias = "c")] + a, + b, +} + +fn main() {} diff --git a/test_suite/tests/ui/conflict/alias-enum.stderr b/test_suite/tests/ui/conflict/alias-enum.stderr new file mode 100644 index 000000000..36e036f65 --- /dev/null +++ b/test_suite/tests/ui/conflict/alias-enum.stderr @@ -0,0 +1,71 @@ +error: alias `b` conflicts with deserialization name of other field + --> tests/ui/conflict/alias-enum.rs:8:9 + | +8 | / /// Expected error on "alias b", because this is a name of other field +9 | | /// Error on "alias a" is not expected because this is a name of this field +10 | | /// Error on "alias c" is not expected because field `c` is skipped +11 | | #[serde(alias = "a", alias = "b", alias = "c")] +12 | | a: (), + | |_____________^ + +error: alias `c` already used by field a + --> tests/ui/conflict/alias-enum.rs:14:9 + | +14 | / /// Expected error on "alias c", because it is already used as alias of `a` +15 | | #[serde(alias = "c")] +16 | | b: (), + | |_____________^ + +error: alias `c` conflicts with deserialization name of other field + --> tests/ui/conflict/alias-enum.rs:23:9 + | +23 | / /// Expected error on "alias c", because this is a name of other field after +24 | | /// applying rename rules +25 | | #[serde(alias = "b", alias = "c")] +26 | | a: (), + | |_____________^ + +error: alias `B` conflicts with deserialization name of other field + --> tests/ui/conflict/alias-enum.rs:34:9 + | +34 | / /// Expected error on "alias B", because this is a name of field after +35 | | /// applying rename rules +36 | | #[serde(alias = "B", alias = "c")] +37 | | a: (), + | |_____________^ + +error: alias `b` conflicts with deserialization name of other variant + --> tests/ui/conflict/alias-enum.rs:44:5 + | +44 | / /// Expected error on "alias b", because this is a name of other variant +45 | | /// Error on "alias a" is not expected because this is a name of this variant +46 | | /// Error on "alias c" is not expected because variant `c` is skipped +47 | | #[serde(alias = "a", alias = "b", alias = "c")] +48 | | a, + | |_____^ + +error: alias `c` already used by variant a + --> tests/ui/conflict/alias-enum.rs:50:5 + | +50 | / /// Expected error on "alias c", because it is already used as alias of `a` +51 | | #[serde(alias = "c")] +52 | | b, + | |_____^ + +error: alias `c` conflicts with deserialization name of other variant + --> tests/ui/conflict/alias-enum.rs:60:5 + | +60 | / /// Expected error on "alias c", because this is a name of other variant after +61 | | /// applying rename rules +62 | | #[serde(alias = "b", alias = "c")] +63 | | a, + | |_____^ + +error: alias `B` conflicts with deserialization name of other variant + --> tests/ui/conflict/alias-enum.rs:72:5 + | +72 | / /// Expected error on "alias B", because this is a name of variant after +73 | | /// applying rename rules +74 | | #[serde(alias = "B", alias = "c")] +75 | | a, + | |_____^ diff --git a/test_suite/tests/ui/conflict/alias.rs b/test_suite/tests/ui/conflict/alias.rs new file mode 100644 index 000000000..f52a90586 --- /dev/null +++ b/test_suite/tests/ui/conflict/alias.rs @@ -0,0 +1,40 @@ +use serde_derive::Deserialize; + +#[derive(Deserialize)] +struct S1 { + /// Expected error on "alias b", because this is a name of other field + /// Error on "alias a" is not expected because this is a name of this field + /// Error on "alias c" is not expected because field `c` is skipped + #[serde(alias = "a", alias = "b", alias = "c")] + a: (), + + /// Expected error on "alias c", because it is already used as alias of `a` + #[serde(alias = "c")] + b: (), + + #[serde(skip_deserializing)] + c: (), +} + +#[derive(Deserialize)] +struct S2 { + /// Expected error on "alias c", because this is a name of other field after + /// applying rename rules + #[serde(alias = "b", alias = "c")] + a: (), + + #[serde(rename = "c")] + b: (), +} + +#[derive(Deserialize)] +#[serde(rename_all = "UPPERCASE")] +struct S3 { + /// Expected error on "alias B", because this is a name of field after + /// applying rename rules + #[serde(alias = "B", alias = "c")] + a: (), + b: (), +} + +fn main() {} diff --git a/test_suite/tests/ui/conflict/alias.stderr b/test_suite/tests/ui/conflict/alias.stderr new file mode 100644 index 000000000..2115b21b1 --- /dev/null +++ b/test_suite/tests/ui/conflict/alias.stderr @@ -0,0 +1,35 @@ +error: alias `b` conflicts with deserialization name of other field + --> tests/ui/conflict/alias.rs:5:5 + | +5 | / /// Expected error on "alias b", because this is a name of other field +6 | | /// Error on "alias a" is not expected because this is a name of this field +7 | | /// Error on "alias c" is not expected because field `c` is skipped +8 | | #[serde(alias = "a", alias = "b", alias = "c")] +9 | | a: (), + | |_________^ + +error: alias `c` already used by field a + --> tests/ui/conflict/alias.rs:11:5 + | +11 | / /// Expected error on "alias c", because it is already used as alias of `a` +12 | | #[serde(alias = "c")] +13 | | b: (), + | |_________^ + +error: alias `c` conflicts with deserialization name of other field + --> tests/ui/conflict/alias.rs:21:5 + | +21 | / /// Expected error on "alias c", because this is a name of other field after +22 | | /// applying rename rules +23 | | #[serde(alias = "b", alias = "c")] +24 | | a: (), + | |_________^ + +error: alias `B` conflicts with deserialization name of other field + --> tests/ui/conflict/alias.rs:33:5 + | +33 | / /// Expected error on "alias B", because this is a name of field after +34 | | /// applying rename rules +35 | | #[serde(alias = "B", alias = "c")] +36 | | a: (), + | |_________^