-
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
Stabilize Ipv6Addr::is_unique_local
and Ipv6Addr::is_unicast_link_local
#129238
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
r? dtolnay Since @dtolnay reviewed the last stabilisation PR in this domain. |
Failed to set assignee to
|
This one isn't entirely true. (I didn't try any of the other languages.) #![feature(ip)]
fn main() {
let ip = std::net::Ipv6Addr::from([0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 255, 255, 169, 254, 0, 0]);
println!("{} {}", ip, ip.is_unicast_link_local());
} ::ffff:169.254.0.0 false Go: https://go.dev/play/p/b4E92eet5Yg package main
import (
"fmt"
"net/netip"
)
func main() {
ip := netip.AddrFrom16([16]byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 255, 255, 169, 254, 0, 0})
fmt.Println(ip, ip.IsLinkLocalUnicast())
} ::ffff:169.254.0.0 true The difference comes down to this "not assigned any special meaning", which is not the case in Go: rust/library/core/src/net/ip_addr.rs Lines 98 to 104 in 804be74
|
@rust-lang/libs-api: |
Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. 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. |
Is These methods, along with similar unstabilized ones, have unfortunately had a pretty rough time getting finalized. I added unresolved questions to the tracking issue at #27709 to list some of these concerns. Most relevant here:
There is a tool designed to check our IP methods against those from other languages, could you try to run it to see where we stand? https://github.com/rust-lang/libs-team/tree/main/tools/ipcheck?
It predates rust 1 :) #22015 also typo unqiue/unique if you were searching for that exact string. |
I ran the ipcheck tool, some of my changes I applied can be seen in my PR: rust-lang/libs-team#454 Complete output of a runFound Rust implementation.
Operations with only a Rust implementation:
SummaryAs far as I can see the difference in output in However since Rust went a different way anyway in handling these cases, I would recon that we can assume Rust is compatible with other languages, however there exists a pitfall. |
I'm open to discussion here. If you want to see how it looks, I could include it in this. |
I agree with @tgross35 that adding this would make it more consistent with |
What I get from https://www.rfc-editor.org/rfc/rfc4291#section-2.5: there are only two scope types for unicast addresses: global unicast and link-local unicast. However, I think we could also add |
☔ The latest upstream changes (presumably #131672) made this pull request unmergeable. Please resolve the merge conflicts. |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
@joshtriplett (or others who checked for FCP) could you ack that the |
33d251e
to
9cca296
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
I think stabilizing these as-is is good. I am open to a separate Ipv6UnicastScope API in addition if someone can express use cases for it beyond just replacing if ip.is_unicast_link_local()
with if let Some(Ipv6UnicastScope::LinkLocal) = ip.unicast_scope()
.
For me the API of the IP types is governed by exposing the behaviors that people want in real-world usage, that are well-specified, and using naming that aligns with term-of-art in a specification where possible. This is a different set of factors than the API for something like Result, where the possible behaviors people want is boundless, and the naming space is cramped, and the design needs to be driven by using a small number of naming schemes/fragments to describe orthogonal and composable behaviors.
@bors r+ |
@bors ping |
@bors ping |
😪 I'm awake I'm awake |
@bors r+ |
…ocal, r=dtolnay Stabilize `Ipv6Addr::is_unique_local` and `Ipv6Addr::is_unicast_link_local` Make `Ipv6Addr::is_unique_local` and `Ipv6Addr::is_unicast_link_local` stable (+const). Newly stable API: ```rust impl Ipv6Addr { // Newly stable under `ipv6_is_unique_local` const fn is_unique_local(&self) -> bool; // Newly stable under `ipv6_is_unique_local` const fn is_unicast_link_local(&self) -> bool; } ``` These stabilise a subset of the following tracking issue: - rust-lang#27709 I have looked and could not find any issues with `is_unique_local` and `is_unicast_link_local`. There is a well received comment calling for stabilisation of the latter function. Both functions are well defined and consistent with implementations in other languages: - [Go](https://cs.opensource.google/go/go/+/refs/tags/go1.23.0:src/net/netip/netip.go;l=518) - [Python](https://github.com/python/cpython/blob/e9d1bf353c3ccafc0d9b61b1b3688051bc976604/Lib/ipaddress.py#L2319-L2321) - [Ruby (unique local)](https://ruby-doc.org/stdlib-2.5.1/libdoc/ipaddr/rdoc/IPAddr.html#private-3F-source) - [Ruby (unicast link local)](https://ruby-doc.org/stdlib-2.5.1/libdoc/ipaddr/rdoc/IPAddr.html#link_local-3F-source) cc implementor `@little-dude` (I can't find the original PR for `is_unqiue_local`) r? libs-api `@rustbot` label +T-libs-api +needs-fcp
…mpiler-errors Rollup of 9 pull requests Successful merges: - rust-lang#129238 (Stabilize `Ipv6Addr::is_unique_local` and `Ipv6Addr::is_unicast_link_local`) - rust-lang#130867 (distinguish overflow and unimplemented in Step::steps_between) - rust-lang#131505 (use `confstr(_CS_DARWIN_USER_TEMP_DIR, ...)` as a `TMPDIR` fallback on Darwin) - rust-lang#132090 (Stop being so bail-y in candidate assembly) - rust-lang#133159 (Don't allow `-Zunstable-options` to take a value ) - rust-lang#133215 (Fix missing submodule in `./x vendor`) - rust-lang#133286 (Re-delay a resolve `bug` related to `Self`-ctor in patterns) - rust-lang#133301 (Add code example for `wrapping_neg` method for signed integers) - rust-lang#133313 (Use arc4random of libc for RTEMS target) r? `@ghost` `@rustbot` modify labels: rollup
…al, r=dtolnay Stabilize `Ipv6Addr::is_unique_local` and `Ipv6Addr::is_unicast_link_local` Make `Ipv6Addr::is_unique_local` and `Ipv6Addr::is_unicast_link_local` stable (+const). Newly stable API: ```rust impl Ipv6Addr { // Newly stable under `ipv6_is_unique_local` const fn is_unique_local(&self) -> bool; // Newly stable under `ipv6_is_unique_local` const fn is_unicast_link_local(&self) -> bool; } ``` These stabilise a subset of the following tracking issue: - rust-lang#27709 I have looked and could not find any issues with `is_unique_local` and `is_unicast_link_local`. There is a well received comment calling for stabilisation of the latter function. Both functions are well defined and consistent with implementations in other languages: - [Go](https://cs.opensource.google/go/go/+/refs/tags/go1.23.0:src/net/netip/netip.go;l=518) - [Python](https://github.com/python/cpython/blob/e9d1bf353c3ccafc0d9b61b1b3688051bc976604/Lib/ipaddress.py#L2319-L2321) - [Ruby (unique local)](https://ruby-doc.org/stdlib-2.5.1/libdoc/ipaddr/rdoc/IPAddr.html#private-3F-source) - [Ruby (unicast link local)](https://ruby-doc.org/stdlib-2.5.1/libdoc/ipaddr/rdoc/IPAddr.html#link_local-3F-source) cc implementor `@little-dude` (I can't find the original PR for `is_unqiue_local`) r? libs-api `@rustbot` label +T-libs-api +needs-fcp
⌛ Testing commit 9cca296 with merge 6b5b52663ee637f1ea29efdaa7348871c9241101... |
Yield to rollup @bors retry |
…mpiler-errors Rollup of 9 pull requests Successful merges: - rust-lang#129238 (Stabilize `Ipv6Addr::is_unique_local` and `Ipv6Addr::is_unicast_link_local`) - rust-lang#130867 (distinguish overflow and unimplemented in Step::steps_between) - rust-lang#131505 (use `confstr(_CS_DARWIN_USER_TEMP_DIR, ...)` as a `TMPDIR` fallback on Darwin) - rust-lang#132090 (Stop being so bail-y in candidate assembly) - rust-lang#133159 (Don't allow `-Zunstable-options` to take a value ) - rust-lang#133215 (Fix missing submodule in `./x vendor`) - rust-lang#133286 (Re-delay a resolve `bug` related to `Self`-ctor in patterns) - rust-lang#133301 (Add code example for `wrapping_neg` method for signed integers) - rust-lang#133313 (Use arc4random of libc for RTEMS target) r? `@ghost` `@rustbot` modify labels: rollup
☀️ Test successful - checks-actions |
Finished benchmarking commit (2cf7908): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary -1.3%, secondary 3.2%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary -2.2%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 796.222s -> 796.948s (0.09%) |
Make
Ipv6Addr::is_unique_local
andIpv6Addr::is_unicast_link_local
stable (+const).Newly stable API:
These stabilise a subset of the following tracking issue:
I have looked and could not find any issues with
is_unique_local
andis_unicast_link_local
. There is a well received comment calling for stabilisation of the latter function.Both functions are well defined and consistent with implementations in other languages:
cc implementor @little-dude
(I can't find the original PR for
is_unqiue_local
)r? libs-api
@rustbot label +T-libs-api +needs-fcp