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

SocketAddrV6 Ord and Eq disagree with each other #116711

Closed
ds84182 opened this issue Oct 13, 2023 · 1 comment · Fixed by #116714
Closed

SocketAddrV6 Ord and Eq disagree with each other #116711

ds84182 opened this issue Oct 13, 2023 · 1 comment · Fixed by #116714
Assignees
@WaffleLapkin
Labels
C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@ds84182
Copy link

ds84182 commented Oct 13, 2023

I tried this code:

https://play.rust-lang.org/?version=stable&mode=release&edition=2021&gist=5c3bc19334a7cf61d10517e874f8cf0b

I expected to see this happen: Equals, true

Instead, this happened: Equals, false

Meta

yeah so SocketAddr behaves differently in a HashMap vs BTreeMap. Not only is this surprising but it also conflicts with the documentation for Ord & PartialOrd.
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Oct 13, 2023
@whentze
Copy link
Contributor

whentze commented Oct 13, 2023 •
edited
Loading

This is caused by Eq being derived but Ord being manually implemented for SocketAddrV6, with the Ord impl only taking into account address and port, ignoring flowinfo and scope_id.

Edit:

rust/library/core/src/net/socket_addr.rs

Line 99 in 4603f0b

#[derive(Copy, Clone, Eq, PartialEq)]

vs.

rust/library/core/src/net/socket_addr.rs

Line 676 in 4603f0b

self.ip().cmp(other.ip()).then(self.port().cmp(&other.port()))

@WaffleLapkin WaffleLapkin added C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Oct 14, 2023
@WaffleLapkin WaffleLapkin self-assigned this Oct 14, 2023
@WaffleLapkin WaffleLapkin mentioned this issue Oct 14, 2023
Merged
@bors bors closed this as completed in 845c414 Oct 24, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Oct 24, 2023
@rust-timer
Unrolled build for rust-lang#116714
fdcf0fa 
Rollup merge of rust-lang#116714 - WaffleLapkin:order-the-order, r=joshtriplett

Derive `Ord`, `PartialOrd` and `Hash` for `SocketAddr*`

Fixes rust-lang#116711

The main pain of this PR is to fix the buggy impl of `Ord` for `SocketAddrV6`, which ignored half of the fields (while `PartialEq` is derived):
https://github.com/rust-lang/rust/blob/4603f0b8afb495ae56cd4c8f70d5d478d906ac54/library/core/src/net/socket_addr.rs#L99-L106

https://github.com/rust-lang/rust/blob/4603f0b8afb495ae56cd4c8f70d5d478d906ac54/library/core/src/net/socket_addr.rs#L676

For me it looks like a simple copy-paste error made in rust-lang#72239 (copy from v4 impl) (cc `@hch12907),` as I don't see this behavior being mentioned anywhere on the PR and it also does not respect `cmp` trait "rules". I also do not see any reasons for those impls to _not_ be derived.

It's a shame we did not notice this for 28 versions/3 years. I guess this is a bug fix, but I'm not sure what the process here should be.

r? libs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@WaffleLapkin WaffleLapkin

Labels
C-bug Category: This is a bug. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Derive Ord, PartialOrd and Hash for SocketAddr* WaffleLapkin/rust
4 participants
@ds84182 @whentze @WaffleLapkin @rustbot