Merge pull request #1170 from H2CO3/disallow_internal_tag_conflict

Disallow variant field names to conflict with tag of internally-tagged enum

serde_derive_internals/src/check.rs

@@ -7,7 +7,7 @@
 // except according to those terms.
 
 use ast::{Data, Container, Style};
-use attr::Identifier;
+use attr::{Identifier, EnumTag};
 use Ctxt;
 
 /// Cross-cutting checks that require looking at more than a single attrs
@@ -16,6 +16,8 @@ pub fn check(cx: &Ctxt, cont: &Container) {
     check_getter(cx, cont);
     check_identifier(cx, cont);
     check_variant_skip_attrs(cx, cont);
+    check_internal_tag_field_name_conflict(cx, cont);
+    check_adjacent_tag_conflict(cx, cont);
 }
 
 /// Getters are only allowed inside structs (not enums) with the `remote`
@@ -169,3 +171,67 @@ fn check_variant_skip_attrs(cx: &Ctxt, cont: &Container) {
         }
     }
 }
+
+/// The tag of an internally-tagged struct variant must not be
+/// the same as either one of its fields, as this would result in
+/// duplicate keys in the serialized output and/or ambiguity in
+/// the to-be-deserialized input.
+fn check_internal_tag_field_name_conflict(
+    cx: &Ctxt,
+    cont: &Container,
+) {
+    let variants = match cont.data {
+        Data::Enum(ref variants) => variants,
+        Data::Struct(_, _) => return,
+    };
+
+    let tag = match *cont.attrs.tag() {
+        EnumTag::Internal { ref tag } => tag.as_str(),
+        EnumTag::External | EnumTag::Adjacent { .. } | EnumTag::None => return,
+    };
+
+    let diagnose_conflict = || {
+        let message = format!(
+            "variant field name `{}` conflicts with internal tag",
+            tag
+        );
+        cx.error(message);
+    };
+
+    for variant in variants {
+        match variant.style {
+            Style::Struct => {
+                for field in &variant.fields {
+                    let check_ser = !field.attrs.skip_serializing();
+                    let check_de = !field.attrs.skip_deserializing();
+                    let name = field.attrs.name();
+                    let ser_name = name.serialize_name();
+                    let de_name = name.deserialize_name();
+
+                    if check_ser && ser_name == tag || check_de && de_name == tag {
+                        diagnose_conflict();
+                        return;
+                    }
+                }
+            },
+            Style::Unit | Style::Newtype | Style::Tuple => {},
+        }
+    }
+}
+
+/// In the case of adjacently-tagged enums, the type and the
+/// contents tag must differ, for the same reason.
+fn check_adjacent_tag_conflict(cx: &Ctxt, cont: &Container) {
+    let (type_tag, content_tag) = match *cont.attrs.tag() {
+        EnumTag::Adjacent { ref tag, ref content } => (tag, content),
+        EnumTag::Internal { .. } | EnumTag::External | EnumTag::None => return,
+    };
+
+    if type_tag == content_tag {
+        let message = format!(
+            "enum tags `{}` for type and content conflict with each other",
+            type_tag
+        );
+        cx.error(message);
+    }
+}

test_suite/tests/compile-fail/conflict/adjacent-tag.rs

@@ -0,0 +1,20 @@
+// Copyright 2018 Serde Developers
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+#[macro_use]
+extern crate serde_derive;
+
+#[derive(Serialize)] //~ ERROR: proc-macro derive panicked
+#[serde(tag = "conflict", content = "conflict")]
+//~^^ HELP: enum tags `conflict` for type and content conflict with each other
+enum E {
+    A,
+    B,
+}
+
+fn main() {}

test_suite/tests/compile-fail/conflict/internal-tag.rs

@@ -0,0 +1,22 @@
+// Copyright 2018 Serde Developers
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+#[macro_use]
+extern crate serde_derive;
+
+#[derive(Serialize)] //~ ERROR: proc-macro derive panicked
+#[serde(tag = "conflict")]
+//~^^ HELP: variant field name `conflict` conflicts with internal tag
+enum E {
+    A {
+        #[serde(rename = "conflict")]
+        x: (),
+    },
+}
+
+fn main() {}