Merge pull request #2562 from Mingun/alias-check

Add checks for conflicts for aliases

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()
+                        ),
+                    );
+                }
+            }
+        }
+    }
+}

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() {}

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,
+   | |_____^

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() {}

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: (),
+   | |_________^