-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Implement PartialOrd and Ord for SocketAddr* #72239
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @LukasKalbertodt (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Aren't trait impls insta-stable? |
To be honest, I wasn't really sure.. which is why I played it safe by making them unstable. Should I just change all the instances of Edit: Done. |
Uh, I'll rebase it properly in a moment. |
r? @Amanieu |
I'm not really sure if ordering addresses makes sense, but since we already have the precedent in @rfcbot fcp merge |
@rfcbot fcp merge |
Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members: Concerns:
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
This FCP covers: // heterogeneous PartialEq
impl PartialEq<SocketAddr> for SocketAddrV4 {...}
impl PartialEq<SocketAddr> for SocketAddrV6 {...}
impl PartialEq<SocketAddrV4> for SocketAddr {...}
impl PartialEq<SocketAddrV6> for SocketAddr {...}
// Ord
impl Ord for SocketAddr {...}
impl Ord for SocketAddrV4 {...}
impl Ord for SocketAddrV6 {...}
// homogeneous PartialOrd
impl PartialOrd<SocketAddr> for SocketAddr {...}
impl PartialOrd<SocketAddrV4> for SocketAddrV4 {...}
impl PartialOrd<SocketAddrV6> for SocketAddrV6 {...}
// heterogeneous PartialOrd
impl PartialOrd<SocketAddr> for SocketAddrV4 {...}
impl PartialOrd<SocketAddr> for SocketAddrV6 {...}
impl PartialOrd<SocketAddrV4> for SocketAddr {...}
impl PartialOrd<SocketAddrV6> for SocketAddr {...} The first 3 groups looks fine to me. Is there any use case for the last group? What kind of code would reasonably want to use those? |
Just gonna register my skepticism in #72239 (comment). @rfcbot concern heterogeneous PartialOrd I think those impls could cause more harm than good, where a refactor turns something that used to be a homogeneous |
I implemented it because Though I agree that its use cases are very niche and generally not useful, it may be surprising for users to find that their code fails to compile, when they find-and-replace |
Removed heterogeneous ordering as per @dtolnay's suggestion and updated the tests a little bit. The PR should be ready for further (and hopefully final!) review now. |
This makes it possible to search a MultiNodeCut by SocketAddr without doing a linear scan. Unfortunately, SocketAddr doesn't implement Ord, so we have to base the sort on tuples of (ip, port). related: rust-lang/rust#72239
🔔 This is now entering its final comment period, as per the review above. 🔔 |
@bors r+ |
📌 Commit d1bc8ad has been approved by |
Rollup of 12 pull requests Successful merges: - rust-lang#72239 (Implement PartialOrd and Ord for SocketAddr*) - rust-lang#72466 (Stabilize str_strip feature) - rust-lang#72605 (Add working example for E0617 explanation) - rust-lang#72636 (Cleanup `Resolver::<clone|into>_outputs` methods) - rust-lang#72645 (Add myself to .mailmap) - rust-lang#72667 (expand unaligned_references test) - rust-lang#72670 (Fix incorrect comment in generator test) - rust-lang#72674 (Clippy should always build) - rust-lang#72682 (Add test for rust-lang#66930) - rust-lang#72695 (update data layout for illumos x86) - rust-lang#72697 (Remove rustc-ux-guidelines) - rust-lang#72702 (rustc_lint: Remove `unused_crate_dependencies` from the `unused` group) Failed merges: r? @ghost
Revert heterogeneous SocketAddr PartialEq impls Originally added in rust-lang#72239. These lead to inference regressions (mostly in tests) in code that looks like: ```rust let socket = SocketAddrV4::new(Ipv4Addr::new(127, 0, 0, 1), 8080); assert_eq!(socket, "127.0.0.1:8080".parse().unwrap()); ``` That compiles as of stable 1.44.0 but fails in beta with: ```console error[E0284]: type annotations needed --> src/main.rs:3:41 | 3 | assert_eq!(socket, "127.0.0.1:8080".parse().unwrap()); | ^^^^^ cannot infer type for type parameter `F` declared on the associated function `parse` | = note: cannot satisfy `<_ as std::str::FromStr>::Err == _` help: consider specifying the type argument in the method call | 3 | assert_eq!(socket, "127.0.0.1:8080".parse::<F>().unwrap()); | ``` Closes rust-lang#73242.
Revert heterogeneous SocketAddr PartialEq impls Originally added in rust-lang#72239. These lead to inference regressions (mostly in tests) in code that looks like: ```rust let socket = SocketAddrV4::new(Ipv4Addr::new(127, 0, 0, 1), 8080); assert_eq!(socket, "127.0.0.1:8080".parse().unwrap()); ``` That compiles as of stable 1.44.0 but fails in beta with: ```console error[E0284]: type annotations needed --> src/main.rs:3:41 | 3 | assert_eq!(socket, "127.0.0.1:8080".parse().unwrap()); | ^^^^^ cannot infer type for type parameter `F` declared on the associated function `parse` | = note: cannot satisfy `<_ as std::str::FromStr>::Err == _` help: consider specifying the type argument in the method call | 3 | assert_eq!(socket, "127.0.0.1:8080".parse::<F>().unwrap()); | ``` Closes rust-lang#73242.
ACL checking may requires lots of calculation and network I/O (DNS resolution)
Revert heterogeneous SocketAddr PartialEq impls Originally added in rust-lang#72239. These lead to inference regressions (mostly in tests) in code that looks like: ```rust let socket = SocketAddrV4::new(Ipv4Addr::new(127, 0, 0, 1), 8080); assert_eq!(socket, "127.0.0.1:8080".parse().unwrap()); ``` That compiles as of stable 1.44.0 but fails in beta with: ```console error[E0284]: type annotations needed --> src/main.rs:3:41 | 3 | assert_eq!(socket, "127.0.0.1:8080".parse().unwrap()); | ^^^^^ cannot infer type for type parameter `F` declared on the associated function `parse` | = note: cannot satisfy `<_ as std::str::FromStr>::Err == _` help: consider specifying the type argument in the method call | 3 | assert_eq!(socket, "127.0.0.1:8080".parse::<F>().unwrap()); | ``` Closes rust-lang#73242.
Revert heterogeneous SocketAddr PartialEq impls Originally added in rust-lang#72239. These lead to inference regressions (mostly in tests) in code that looks like: ```rust let socket = SocketAddrV4::new(Ipv4Addr::new(127, 0, 0, 1), 8080); assert_eq!(socket, "127.0.0.1:8080".parse().unwrap()); ``` That compiles as of stable 1.44.0 but fails in beta with: ```console error[E0284]: type annotations needed --> src/main.rs:3:41 | 3 | assert_eq!(socket, "127.0.0.1:8080".parse().unwrap()); | ^^^^^ cannot infer type for type parameter `F` declared on the associated function `parse` | = note: cannot satisfy `<_ as std::str::FromStr>::Err == _` help: consider specifying the type argument in the method call | 3 | assert_eq!(socket, "127.0.0.1:8080".parse::<F>().unwrap()); | ``` Closes rust-lang#73242.
While here clean up all pkglint warnings. Changes since 1.44.1: Version 1.45.2 (2020-08-03) ========================== * [Fix bindings in tuple struct patterns][74954] * [Fix track_caller integration with trait objects][74784] [74954]: rust-lang/rust#74954 [74784]: rust-lang/rust#74784 Version 1.45.1 (2020-07-30) ========================== * [Fix const propagation with references.][73613] * [rustfmt accepts rustfmt_skip in cfg_attr again.][73078] * [Avoid spurious implicit region bound.][74509] * [Install clippy on x.py install][74457] [73613]: rust-lang/rust#73613 [73078]: rust-lang/rust#73078 [74509]: rust-lang/rust#74509 [74457]: rust-lang/rust#74457 Version 1.45.0 (2020-07-16) ========================== Language -------- - [Out of range float to int conversions using `as` has been defined as a saturating conversion.][71269] This was previously undefined behaviour, but you can use the `{f64, f32}::to_int_unchecked` methods to continue using the current behaviour, which may be desirable in rare performance sensitive situations. - [`mem::Discriminant<T>` now uses `T`'s discriminant type instead of always using `u64`.][70705] - [Function like procedural macros can now be used in expression, pattern, and statement positions.][68717] This means you can now use a function-like procedural macro anywhere you can use a declarative (`macro_rules!`) macro. Compiler -------- - [You can now override individual target features through the `target-feature` flag.][72094] E.g. `-C target-feature=+avx2 -C target-feature=+fma` is now equivalent to `-C target-feature=+avx2,+fma`. - [Added the `force-unwind-tables` flag.][69984] This option allows rustc to always generate unwind tables regardless of panic strategy. - [Added the `embed-bitcode` flag.][71716] This codegen flag allows rustc to include LLVM bitcode into generated `rlib`s (this is on by default). - [Added the `tiny` value to the `code-model` codegen flag.][72397] - [Added tier 3 support\* for the `mipsel-sony-psp` target.][72062] - [Added tier 3 support for the `thumbv7a-uwp-windows-msvc` target.][72133] \* Refer to Rust's [platform support page][forge-platform-support] for more information on Rust's tiered platform support. Libraries --------- - [`net::{SocketAddr, SocketAddrV4, SocketAddrV6}` now implements `PartialOrd` and `Ord`.][72239] - [`proc_macro::TokenStream` now implements `Default`.][72234] - [You can now use `char` with `ops::{Range, RangeFrom, RangeFull, RangeInclusive, RangeTo}` to iterate over a range of codepoints.][72413] E.g. you can now write the following; ```rust for ch in 'a'..='z' { print!("{}", ch); } println!(); // Prints "abcdefghijklmnopqrstuvwxyz" ``` - [`OsString` now implements `FromStr`.][71662] - [The `saturating_neg` method as been added to all signed integer primitive types, and the `saturating_abs` method has been added for all integer primitive types.][71886] - [`Arc<T>`, `Rc<T>` now implement `From<Cow<'_, T>>`, and `Box` now implements `From<Cow>` when `T` is `[T: Copy]`, `str`, `CStr`, `OsStr`, or `Path`.][71447] - [`Box<[T]>` now implements `From<[T; N]>`.][71095] - [`BitOr` and `BitOrAssign` are implemented for all `NonZero` integer types.][69813] - [The `fetch_min`, and `fetch_max` methods have been added to all atomic integer types.][72324] - [The `fetch_update` method has been added to all atomic integer types.][71843] Stabilized APIs --------------- - [`Arc::as_ptr`] - [`BTreeMap::remove_entry`] - [`Rc::as_ptr`] - [`rc::Weak::as_ptr`] - [`rc::Weak::from_raw`] - [`rc::Weak::into_raw`] - [`str::strip_prefix`] - [`str::strip_suffix`] - [`sync::Weak::as_ptr`] - [`sync::Weak::from_raw`] - [`sync::Weak::into_raw`] - [`char::UNICODE_VERSION`] - [`Span::resolved_at`] - [`Span::located_at`] - [`Span::mixed_site`] - [`unix::process::CommandExt::arg0`] Cargo ----- Misc ---- - [Rustdoc now supports strikethrough text in Markdown.][71928] E.g. `~~outdated information~~` becomes "~~outdated information~~". - [Added an emoji to Rustdoc's deprecated API message.][72014] Compatibility Notes ------------------- - [Trying to self initialize a static value (that is creating a value using itself) is unsound and now causes a compile error.][71140] - [`{f32, f64}::powi` now returns a slightly different value on Windows.][73420] This is due to changes in LLVM's intrinsics which `{f32, f64}::powi` uses. - [Rustdoc's CLI's extra error exit codes have been removed.][71900] These were previously undocumented and not intended for public use. Rustdoc still provides a non-zero exit code on errors. Internals Only -------------- - [Make clippy a git subtree instead of a git submodule][70655] - [Unify the undo log of all snapshot types][69464] [73420]: rust-lang/rust#73420 [72324]: rust-lang/rust#72324 [71843]: rust-lang/rust#71843 [71886]: rust-lang/rust#71886 [72234]: rust-lang/rust#72234 [72239]: rust-lang/rust#72239 [72397]: rust-lang/rust#72397 [72413]: rust-lang/rust#72413 [72014]: rust-lang/rust#72014 [72062]: rust-lang/rust#72062 [72094]: rust-lang/rust#72094 [72133]: rust-lang/rust#72133 [71900]: rust-lang/rust#71900 [71928]: rust-lang/rust#71928 [71662]: rust-lang/rust#71662 [71716]: rust-lang/rust#71716 [71447]: rust-lang/rust#71447 [71269]: rust-lang/rust#71269 [71095]: rust-lang/rust#71095 [71140]: rust-lang/rust#71140 [70655]: rust-lang/rust#70655 [70705]: rust-lang/rust#70705 [69984]: rust-lang/rust#69984 [69813]: rust-lang/rust#69813 [69464]: rust-lang/rust#69464 [68717]: rust-lang/rust#68717 [`Arc::as_ptr`]: https://doc.rust-lang.org/stable/std/sync/struct.Arc.html#method.as_ptr [`BTreeMap::remove_entry`]: https://doc.rust-lang.org/stable/std/collections/struct.BTreeMap.html#method.remove_entry [`Rc::as_ptr`]: https://doc.rust-lang.org/stable/std/rc/struct.Rc.html#method.as_ptr [`rc::Weak::as_ptr`]: https://doc.rust-lang.org/stable/std/rc/struct.Weak.html#method.as_ptr [`rc::Weak::from_raw`]: https://doc.rust-lang.org/stable/std/rc/struct.Weak.html#method.from_raw [`rc::Weak::into_raw`]: https://doc.rust-lang.org/stable/std/rc/struct.Weak.html#method.into_raw [`sync::Weak::as_ptr`]: https://doc.rust-lang.org/stable/std/sync/struct.Weak.html#method.as_ptr [`sync::Weak::from_raw`]: https://doc.rust-lang.org/stable/std/sync/struct.Weak.html#method.from_raw [`sync::Weak::into_raw`]: https://doc.rust-lang.org/stable/std/sync/struct.Weak.html#method.into_raw [`str::strip_prefix`]: https://doc.rust-lang.org/stable/std/primitive.str.html#method.strip_prefix [`str::strip_suffix`]: https://doc.rust-lang.org/stable/std/primitive.str.html#method.strip_suffix [`char::UNICODE_VERSION`]: https://doc.rust-lang.org/stable/std/char/constant.UNICODE_VERSION.html [`Span::resolved_at`]: https://doc.rust-lang.org/stable/proc_macro/struct.Span.html#method.resolved_at [`Span::located_at`]: https://doc.rust-lang.org/stable/proc_macro/struct.Span.html#method.located_at [`Span::mixed_site`]: https://doc.rust-lang.org/stable/proc_macro/struct.Span.html#method.mixed_site [`unix::process::CommandExt::arg0`]: https://doc.rust-lang.org/std/os/unix/process/trait.CommandExt.html#tymethod.arg0
…shtriplett 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
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
The implementation is mostly the same as the one found in
IpAddr
(other than adding comparison for ports, of course).Continues #53788 and #53863
Fixes #53710