From a20e9249c5849b6855ca2d2aa1d0ce563855c3bd Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Sun, 10 Nov 2024 22:32:55 -0800 Subject: [PATCH 1/3] Revert "pacify clippy" This reverts commit 951ca5ace0ca0abf9b44e0d6c76005fbd3b77c5c. --- serde_derive/src/internals/check.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/serde_derive/src/internals/check.rs b/serde_derive/src/internals/check.rs index c9c0fa46d..6892d4403 100644 --- a/serde_derive/src/internals/check.rs +++ b/serde_derive/src/internals/check.rs @@ -483,7 +483,7 @@ fn check_from_and_try_from(cx: &Ctxt, cont: &mut Container) { 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::Enum(variants) => check_variant_name_conflicts(cx, &variants), Data::Struct(Style::Struct, fields) => check_field_name_conflicts(cx, fields), _ => {} } From edd6fe954bc35bbafb454835c6529d0e30148624 Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Sun, 10 Nov 2024 22:33:03 -0800 Subject: [PATCH 2/3] Revert "Add checks for conflicts for aliases" This reverts commit 5f9fffa53e8a48d9e1f32b1735542207c42f3ead. --- serde_derive/src/internals/check.rs | 136 +--------------------------- 1 file changed, 1 insertion(+), 135 deletions(-) diff --git a/serde_derive/src/internals/check.rs b/serde_derive/src/internals/check.rs index 6892d4403..da2a0fbdc 100644 --- a/serde_derive/src/internals/check.rs +++ b/serde_derive/src/internals/check.rs @@ -1,8 +1,6 @@ -use crate::internals::ast::{Container, Data, Field, Style, Variant}; +use crate::internals::ast::{Container, Data, Field, Style}; 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. @@ -18,7 +16,6 @@ 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 @@ -478,134 +475,3 @@ 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() - ), - ); - } - } - } - } -} From 111ecc5d8c01cf7795059f3bc436bfd0e57d1d64 Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Sun, 10 Nov 2024 22:38:59 -0800 Subject: [PATCH 3/3] Update ui tests for warning on colliding aliases --- test_suite/tests/ui/conflict/alias-enum.rs | 4 +- .../tests/ui/conflict/alias-enum.stderr | 120 ++++++++++-------- test_suite/tests/ui/conflict/alias.rs | 4 +- test_suite/tests/ui/conflict/alias.stderr | 66 +++++----- 4 files changed, 107 insertions(+), 87 deletions(-) diff --git a/test_suite/tests/ui/conflict/alias-enum.rs b/test_suite/tests/ui/conflict/alias-enum.rs index 52af74b97..faceae3e7 100644 --- a/test_suite/tests/ui/conflict/alias-enum.rs +++ b/test_suite/tests/ui/conflict/alias-enum.rs @@ -76,4 +76,6 @@ enum E3 { b, } -fn main() {} +fn main() { + @//fail +} diff --git a/test_suite/tests/ui/conflict/alias-enum.stderr b/test_suite/tests/ui/conflict/alias-enum.stderr index 36e036f65..aa0ca687c 100644 --- a/test_suite/tests/ui/conflict/alias-enum.stderr +++ b/test_suite/tests/ui/conflict/alias-enum.stderr @@ -1,71 +1,79 @@ -error: alias `b` conflicts with deserialization name of other field - --> tests/ui/conflict/alias-enum.rs:8:9 +error: expected expression, found `@` + --> tests/ui/conflict/alias-enum.rs:80:5 | -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: (), - | |_____________^ +80 | @//fail + | ^ expected expression -error: alias `c` already used by field a - --> tests/ui/conflict/alias-enum.rs:14:9 +warning: unreachable pattern + --> tests/ui/conflict/alias-enum.rs:16:9 | -14 | / /// Expected error on "alias c", because it is already used as alias of `a` -15 | | #[serde(alias = "c")] -16 | | b: (), - | |_____________^ +11 | #[serde(alias = "a", alias = "b", alias = "c")] + | --- matches all the relevant values +... +16 | b: (), + | ^ no value can reach this + | + = note: `#[warn(unreachable_patterns)]` on by default + +warning: unreachable pattern + --> tests/ui/conflict/alias-enum.rs:15:25 + | +11 | #[serde(alias = "a", alias = "b", alias = "c")] + | --- matches all the relevant values +... +15 | #[serde(alias = "c")] + | ^^^ no value can reach this -error: alias `c` conflicts with deserialization name of other field - --> tests/ui/conflict/alias-enum.rs:23:9 +warning: unreachable pattern + --> tests/ui/conflict/alias-enum.rs:28:26 | -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: (), - | |_____________^ +25 | #[serde(alias = "b", alias = "c")] + | --- matches all the relevant values +... +28 | #[serde(rename = "c")] + | ^^^ no value can reach this -error: alias `B` conflicts with deserialization name of other field - --> tests/ui/conflict/alias-enum.rs:34:9 +warning: unreachable pattern + --> tests/ui/conflict/alias-enum.rs:38: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: (), - | |_____________^ +36 | #[serde(alias = "B", alias = "c")] + | --- matches all the relevant values +37 | a: (), +38 | b: (), + | ^ no value can reach this -error: alias `b` conflicts with deserialization name of other variant - --> tests/ui/conflict/alias-enum.rs:44:5 +warning: unreachable pattern + --> tests/ui/conflict/alias-enum.rs:52: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, - | |_____^ +47 | #[serde(alias = "a", alias = "b", alias = "c")] + | --- matches all the relevant values +... +52 | b, + | ^ no value can reach this -error: alias `c` already used by variant a - --> tests/ui/conflict/alias-enum.rs:50:5 +warning: unreachable pattern + --> tests/ui/conflict/alias-enum.rs:51:21 | -50 | / /// Expected error on "alias c", because it is already used as alias of `a` -51 | | #[serde(alias = "c")] -52 | | b, - | |_____^ +47 | #[serde(alias = "a", alias = "b", alias = "c")] + | --- matches all the relevant values +... +51 | #[serde(alias = "c")] + | ^^^ no value can reach this -error: alias `c` conflicts with deserialization name of other variant - --> tests/ui/conflict/alias-enum.rs:60:5 +warning: unreachable pattern + --> tests/ui/conflict/alias-enum.rs:65:22 | -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, - | |_____^ +62 | #[serde(alias = "b", alias = "c")] + | --- matches all the relevant values +... +65 | #[serde(rename = "c")] + | ^^^ no value can reach this -error: alias `B` conflicts with deserialization name of other variant - --> tests/ui/conflict/alias-enum.rs:72:5 +warning: unreachable pattern + --> tests/ui/conflict/alias-enum.rs:76: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, - | |_____^ +74 | #[serde(alias = "B", alias = "c")] + | --- matches all the relevant values +75 | a, +76 | b, + | ^ no value can reach this diff --git a/test_suite/tests/ui/conflict/alias.rs b/test_suite/tests/ui/conflict/alias.rs index f52a90586..3f6cee295 100644 --- a/test_suite/tests/ui/conflict/alias.rs +++ b/test_suite/tests/ui/conflict/alias.rs @@ -37,4 +37,6 @@ struct S3 { b: (), } -fn main() {} +fn main() { + @//fail +} diff --git a/test_suite/tests/ui/conflict/alias.stderr b/test_suite/tests/ui/conflict/alias.stderr index 2115b21b1..7e665a630 100644 --- a/test_suite/tests/ui/conflict/alias.stderr +++ b/test_suite/tests/ui/conflict/alias.stderr @@ -1,35 +1,43 @@ -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: expected expression, found `@` + --> tests/ui/conflict/alias.rs:41:5 + | +41 | @//fail + | ^ expected expression + +warning: unreachable pattern + --> tests/ui/conflict/alias.rs:13:5 + | +8 | #[serde(alias = "a", alias = "b", alias = "c")] + | --- matches all the relevant values +... +13 | b: (), + | ^ no value can reach this + | + = note: `#[warn(unreachable_patterns)]` on by default -error: alias `c` already used by field a - --> tests/ui/conflict/alias.rs:11:5 +warning: unreachable pattern + --> tests/ui/conflict/alias.rs:12:21 | -11 | / /// Expected error on "alias c", because it is already used as alias of `a` -12 | | #[serde(alias = "c")] -13 | | b: (), - | |_________^ +8 | #[serde(alias = "a", alias = "b", alias = "c")] + | --- matches all the relevant values +... +12 | #[serde(alias = "c")] + | ^^^ no value can reach this -error: alias `c` conflicts with deserialization name of other field - --> tests/ui/conflict/alias.rs:21:5 +warning: unreachable pattern + --> tests/ui/conflict/alias.rs:26:22 | -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: (), - | |_________^ +23 | #[serde(alias = "b", alias = "c")] + | --- matches all the relevant values +... +26 | #[serde(rename = "c")] + | ^^^ no value can reach this -error: alias `B` conflicts with deserialization name of other field - --> tests/ui/conflict/alias.rs:33:5 +warning: unreachable pattern + --> tests/ui/conflict/alias.rs:37: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: (), - | |_________^ +35 | #[serde(alias = "B", alias = "c")] + | --- matches all the relevant values +36 | a: (), +37 | b: (), + | ^ no value can reach this