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.

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

Add checks for conflicts for aliases #2562

Merged
merged 1 commit into from
Nov 1, 2024
Merged

Conversation

Mingun
Copy link
Contributor

@Mingun Mingun commented Aug 8, 2023

  • Check that alias is not the same as name of other field / variant (it still can be the name of owning field / variant)
  • Check that aliases are unique, i. e. two different fields / variants does not use the same alias

It would be better, if error points to corresponding alias, but currently alias spans are not stored anywhere, so I left this to another PR.

Fixes #2308, fixes #2551

@Mingun
Copy link
Contributor Author

Mingun commented Mar 3, 2024

@dtolnay, @oli-obk, can you give some feedback?

- Check that alias is not the same as name of other field (it still can be the name of owning field/variant)
- Check that aliases are unique, i. e. two different fields does not use the same alias
@oli-obk
Copy link
Member

oli-obk commented Nov 1, 2024

Annoying that we can't crater it first, but this also catches really subtly broken code, so I guess we can Yolo it.

@oli-obk oli-obk merged commit adf05a5 into serde-rs:master Nov 1, 2024
15 checks passed
@oli-obk oli-obk mentioned this pull request Nov 1, 2024
@Mingun Mingun deleted the alias-check branch November 1, 2024 14:39
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think this can be released as is. This is a pretty blatant breaking change in existing working code.

I would recommend emitting something like a unreachable_patterns warning on duplicated names.

@oli-obk
Copy link
Member

oli-obk commented Nov 11, 2024

The code would never have worked correctly. I don't think anyone would ship this for long without users complaining.

If we were shipping such a breaking change in rustc, this would be the equivalent of adding an error for unsound code that we know has almost no real world usage

@dtolnay
Copy link
Member

dtolnay commented Nov 11, 2024

I do not agree that this PR is analogous to rustc emitting a new error on an unsound code pattern. It is analogous to rustc deciding that unreachable patterns are a hard error now, because their previous behavior cannot possibly be what the person who wrote the unreachable pattern intended. For code in which deserializing to the first variant with the same alias was the desired behavior (hence users not complaining) I don't understand why it's better for a new version of serde to break them as opposed to a warning that says to remove the irrelevant alias.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants