Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Revert the colliding aliases hard error (PRs #2562 & #2853) #2857

Merged
merged 3 commits into from
Nov 11, 2024
Merged

Revert the colliding aliases hard error (PRs #2562 & #2853) #2857

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a20e924
Revert "pacify clippy"
dtolnay Nov 11, 2024
edd6fe9
Revert "Add checks for conflicts for aliases"
dtolnay Nov 11, 2024
111ecc5
Update ui tests for warning on colliding aliases
dtolnay Nov 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
136 changes: 1 addition & 135 deletions serde_derive/src/internals/check.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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