From a799ea171cace988c7ae9856b5b23bd02a2142b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?A=CC=81rpa=CC=81d=20Goretity?= Date: Thu, 8 Mar 2018 00:43:35 +0100 Subject: [PATCH 1/5] Disallow variant field names to conflict with tag of internally-tagged enum --- serde_derive_internals/src/check.rs | 46 ++++++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/serde_derive_internals/src/check.rs b/serde_derive_internals/src/check.rs index 0d6946ff..0f5c17f7 100644 --- a/serde_derive_internals/src/check.rs +++ b/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,7 @@ pub fn check(cx: &Ctxt, cont: &Container) { check_getter(cx, cont); check_identifier(cx, cont); check_variant_skip_attrs(cx, cont); + check_internally_tagged_variant_name_conflict(cx, cont); } /// Getters are only allowed inside structs (not enums) with the `remote` @@ -169,3 +170,46 @@ fn check_variant_skip_attrs(cx: &Ctxt, cont: &Container) { } } } + +fn check_internally_tagged_variant_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; + } + } + }, + _ => {}, + } + } +} From 9fc526b788ee28622d8831dd14748905b1483efd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?A=CC=81rpa=CC=81d=20Goretity?= Date: Thu, 8 Mar 2018 00:53:56 +0100 Subject: [PATCH 2/5] be more explicit in match for future-proofing code via exhaustiveness check --- serde_derive_internals/src/check.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/serde_derive_internals/src/check.rs b/serde_derive_internals/src/check.rs index 0f5c17f7..45d325b4 100644 --- a/serde_derive_internals/src/check.rs +++ b/serde_derive_internals/src/check.rs @@ -209,7 +209,7 @@ fn check_internally_tagged_variant_name_conflict( } } }, - _ => {}, + Style::Unit | Style::Newtype | Style::Tuple => {}, } } } From 0ddebe03174a2bfdcde00e390191b29d78505e55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?A=CC=81rpa=CC=81d=20Goretity?= Date: Thu, 8 Mar 2018 01:05:19 +0100 Subject: [PATCH 3/5] More descriptive function name; add doc comment to function --- serde_derive_internals/src/check.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/serde_derive_internals/src/check.rs b/serde_derive_internals/src/check.rs index 45d325b4..9ab8e45b 100644 --- a/serde_derive_internals/src/check.rs +++ b/serde_derive_internals/src/check.rs @@ -16,7 +16,7 @@ pub fn check(cx: &Ctxt, cont: &Container) { check_getter(cx, cont); check_identifier(cx, cont); check_variant_skip_attrs(cx, cont); - check_internally_tagged_variant_name_conflict(cx, cont); + check_internal_tag_field_name_conflict(cx, cont); } /// Getters are only allowed inside structs (not enums) with the `remote` @@ -171,7 +171,11 @@ fn check_variant_skip_attrs(cx: &Ctxt, cont: &Container) { } } -fn check_internally_tagged_variant_name_conflict( +/// 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, ) { From f4b78e202a12672fbb209536c21996484e238e89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?A=CC=81rpa=CC=81d=20Goretity?= Date: Thu, 8 Mar 2018 09:57:05 +0100 Subject: [PATCH 4/5] add a check for conflicting adjacent tags as well --- serde_derive_internals/src/check.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/serde_derive_internals/src/check.rs b/serde_derive_internals/src/check.rs index 9ab8e45b..229896cd 100644 --- a/serde_derive_internals/src/check.rs +++ b/serde_derive_internals/src/check.rs @@ -17,6 +17,7 @@ pub fn check(cx: &Ctxt, cont: &Container) { 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` @@ -217,3 +218,20 @@ fn check_internal_tag_field_name_conflict( } } } + +/// 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); + } +} From f288a41768f6ecc14b8195a9948a3229ecbf5be1 Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Thu, 8 Mar 2018 09:31:25 -0800 Subject: [PATCH 5/5] Test the new errors on conflicting enum tags --- serde_derive_internals/src/check.rs | 2 +- .../compile-fail/conflict/adjacent-tag.rs | 20 +++++++++++++++++ .../compile-fail/conflict/internal-tag.rs | 22 +++++++++++++++++++ 3 files changed, 43 insertions(+), 1 deletion(-) create mode 100644 test_suite/tests/compile-fail/conflict/adjacent-tag.rs create mode 100644 test_suite/tests/compile-fail/conflict/internal-tag.rs diff --git a/serde_derive_internals/src/check.rs b/serde_derive_internals/src/check.rs index 229896cd..c3df861b 100644 --- a/serde_derive_internals/src/check.rs +++ b/serde_derive_internals/src/check.rs @@ -229,7 +229,7 @@ fn check_adjacent_tag_conflict(cx: &Ctxt, cont: &Container) { if type_tag == content_tag { let message = format!( - "Enum tags `{}` for type and content conflict with each other", + "enum tags `{}` for type and content conflict with each other", type_tag ); cx.error(message); diff --git a/test_suite/tests/compile-fail/conflict/adjacent-tag.rs b/test_suite/tests/compile-fail/conflict/adjacent-tag.rs new file mode 100644 index 00000000..419afa4c --- /dev/null +++ b/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 or the MIT license +// , 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() {} diff --git a/test_suite/tests/compile-fail/conflict/internal-tag.rs b/test_suite/tests/compile-fail/conflict/internal-tag.rs new file mode 100644 index 00000000..777718bc --- /dev/null +++ b/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 or the MIT license +// , 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() {}