Skip to content

Commit

Permalink
Merge pull request #2857 from dtolnay/collide
Browse files Browse the repository at this point in the history 
Revert the colliding aliases hard error (PRs #2562 & #2853)
  • Loading branch information
@dtolnay
dtolnay authored Nov 11, 2024
2 parents b1353a9 + 111ecc5 commit 7d96352
Show file tree
Hide file tree
Showing 5 changed files with 108 additions and 222 deletions.
136 changes: 1 addition & 135 deletions serde_derive/src/internals/check.rs
View file
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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()
),
);
}
}
}
}
}
4 changes: 3 additions & 1 deletion test_suite/tests/ui/conflict/alias-enum.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,4 +76,6 @@ enum E3 {
b,
}

fn main() {}
fn main() {
@//fail
}
120 changes: 64 additions & 56 deletions test_suite/tests/ui/conflict/alias-enum.stderr
View file
Original file line number Diff line number Diff line change
@@ -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
4 changes: 3 additions & 1 deletion test_suite/tests/ui/conflict/alias.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,4 +37,6 @@ struct S3 {
b: (),
}

fn main() {}
fn main() {
@//fail
}
66 changes: 37 additions & 29 deletions test_suite/tests/ui/conflict/alias.stderr
View file
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 7d96352

Please sign in to comment.