stabilize the "ip" feature #66584
stabilize the "ip" feature #66584
Conversation
|
r? @Kimundi (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
jonas-schievink
added
relnotes
jonas-schievink
added
the
needs-fcp
|
@the8472, what do you think of the "stability guarantees" disclaimer? |
|
r? @kennytm |
|
@Dylan-DPC This stabilization PR requires FCP from the libs team. Could you |
|
Thanks for reviewing. r? @KodrAus |
|
Hi, is there anything I can do to push this forward? |
|
@little-dude nothing. we will do a team consensus voting after the holiday break. |
|
☔ The latest upstream changes (presumably #67540) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@Dylan-DPC @KodrAus can we start an FCP? I seems that it did not make it to 1.41 so I'd like to make sure it's not forgotten for 1.42. |
There was a problem hiding this comment.
Thanks for your patience @little-dude!
I’ve just left a few comments around docs, but I don’t think that needs to hold up the FCP.
@rfcbot fcp merge
src/libstd/net/ip.rs
Outdated
| @@ -124,13 +116,21 @@ pub struct Ipv6Addr { | |||
|
|
|||
| #[allow(missing_docs)] | |||
There was a problem hiding this comment.
Let’s make sure we have proper docs for this before stabilizing, which I think would include linking to the appropriate section of the RFC.
There was a problem hiding this comment.
I just noticed that the Reserved and Unassigned variants are part of the table but are missing from the enum. Instead, Ipv6Address::multicast_scope() returns None for such scopes. I don't think this is a problem but I think it's worth mentioning. Maybe @therealbstern or @the8472 have an opinion on that API?
There was a problem hiding this comment.
FWIW, I think Reserved and Unassigned are good things to put in the enum, but my concern is that there's a non-zero chance the Reserved/Unassigned space will get assigned to something in the future.
On the other hand, None is just as wrong as Reserved or Unassigned if the IETF assigns them in the future. In addition, IPv6 is so big that who knows when/if they'll get assigned.
My measured opinion is that we should add Reserved and Unassigned right now, since it's correct right now and might remain correct indefinitely.
There was a problem hiding this comment.
I think it’s something to consider now before stabilization.
Let’s at least drop a #[non_exhaustive] attribute on the enum so we can add those variants later if need be.
There was a problem hiding this comment.
Yes, nonexhaustive should cover future RFC changes nicely.
There was a problem hiding this comment.
sounds good, I'll make the changes.
edit: pushed a commit with the changes.
|
@KodrAus I think rfcbot only responds to normal comments , not review comments |
|
Thanks for the review @KodrAus. I addressed your comments and squashed the commits. |
|
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 |
|
#69772 raises the question how these helper methods should behave if the ipv6 address is actually an ipv4-compatible or ipv4-mapped address. Maybe that should be discussed before stabilization. |
|
@rfcbot concern ipv4 in ipv6 Not identifying |
|
Re
Given point 3, I don't see any situation where I would want to check anything but the /10 prefix. |
|
@little-dude do you have a plan for resolving the existing concerns? Could you use a hand? |
|
@little-dude Ping from triage, any updates here? |
|
@little-dude Closing due to inactivity. Free free to reopen or create another PR to address the mentioned concerns above. You might also create a topic on Zulip for discussions. Thanks! |
rfcbot
removed
proposed-final-comment-period
crlf0710
added
S-inactive
|
Hello! I'd really love to use
If we already have capability to check IPv4 addresses, I feel like it's almost trivial to implement these rules for IPv6 as well if we just need to pad it with some extra bytes? Famous last words, I know...
From the commit that introduced the change:
From section 2.4:
Section section 2.5.3 seems...unrelated? It has to do with the loopback address. Section 2.5.6 however, is where the problem lies:
So the difference in the spec is this:
This does seem like an issue, so I did some more digging to figure out what's going on. I found this draft, which appears to have died. It does however point us in the right direction:
I'm not an expert in reading IETF material -- far from it -- but I am vaguely aware of the passage of time. Every document that has been linked here that specifies /64 is more recent than those specifying /10, unless I've missed one. Additionally, there are countless secondhand sources (blogs, stackoverflow, etc) online all talking about how /64 is the spec. Again, not an expert. I posted all this info here so someone more versed than me can evaluate and decide what the best course of action is. Without further input, I'd personally go ahead with just the one |
Stabilize `{IpAddr, Ipv6Addr}::to_canonical`
Make `IpAddr::to_canonical` and `IpV6Addr::to_canonical` stable (+const), as well as const stabilize `Ipv6Addr::to_ipv4_mapped`.
Newly stable API:
```rust
impl IpAddr {
// Newly stable under `ip_to_canonical`
const fn to_canonical(&self) -> IpAddr;
}
impl Ipv6Addr {
// Newly stable under `ip_to_canonical`
const fn to_canonical(&self) -> IpAddr;
// Already stable, this makes it const stable under
// `const_ipv6_to_ipv4_mapped`
const fn to_ipv4_mapped(&self) -> Option<Ipv4Addr>
}
```
These stabilize a subset of the following tracking issues:
- rust-lang#27709
- rust-lang#76205
Stabilization of all methods under the `ip` gate was attempted once at rust-lang#66584 then again at rust-lang#76098. These were not successful because there are still unknowns about `is_documentation` `is_benchmarking` and similar; `to_canonical` is much more straightforward.
I have looked and could not find any known issues with `to_canonical`. These were added in 2021 in rust-lang#87708
cc implementor `@the8472`
r? libs-api
`@rustbot` label +T-libs-api +needs-fcp
Stabilize `{IpAddr, Ipv6Addr}::to_canonical`
Make `IpAddr::to_canonical` and `IpV6Addr::to_canonical` stable (+const), as well as const stabilize `Ipv6Addr::to_ipv4_mapped`.
Newly stable API:
```rust
impl IpAddr {
// Newly stable under `ip_to_canonical`
const fn to_canonical(&self) -> IpAddr;
}
impl Ipv6Addr {
// Newly stable under `ip_to_canonical`
const fn to_canonical(&self) -> IpAddr;
// Already stable, this makes it const stable under
// `const_ipv6_to_ipv4_mapped`
const fn to_ipv4_mapped(&self) -> Option<Ipv4Addr>
}
```
These stabilize a subset of the following tracking issues:
- rust-lang#27709
- rust-lang#76205
Stabilization of all methods under the `ip` gate was attempted once at rust-lang#66584 then again at rust-lang#76098. These were not successful because there are still unknowns about `is_documentation` `is_benchmarking` and similar; `to_canonical` is much more straightforward.
I have looked and could not find any known issues with `to_canonical`. These were added in 2021 in rust-lang#87708
cc implementor ``@the8472``
r? libs-api
``@rustbot`` label +T-libs-api +needs-fcp
Rollup merge of rust-lang#115955 - tgross35:ip-to-canonical, r=dtolnay Stabilize `{IpAddr, Ipv6Addr}::to_canonical` Make `IpAddr::to_canonical` and `IpV6Addr::to_canonical` stable (+const), as well as const stabilize `Ipv6Addr::to_ipv4_mapped`. Newly stable API: ```rust impl IpAddr { // Newly stable under `ip_to_canonical` const fn to_canonical(&self) -> IpAddr; } impl Ipv6Addr { // Newly stable under `ip_to_canonical` const fn to_canonical(&self) -> IpAddr; // Already stable, this makes it const stable under // `const_ipv6_to_ipv4_mapped` const fn to_ipv4_mapped(&self) -> Option<Ipv4Addr> } ``` These stabilize a subset of the following tracking issues: - rust-lang#27709 - rust-lang#76205 Stabilization of all methods under the `ip` gate was attempted once at rust-lang#66584 then again at rust-lang#76098. These were not successful because there are still unknowns about `is_documentation` `is_benchmarking` and similar; `to_canonical` is much more straightforward. I have looked and could not find any known issues with `to_canonical`. These were added in 2021 in rust-lang#87708 cc implementor ``@the8472`` r? libs-api ``@rustbot`` label +T-libs-api +needs-fcp
This my first time doing that, so I hope I didn't mess this up.
Stabilize the "ip" feature (fixes #27709), and add a disclaimer about the IP helpers stability (fixes #60239)
Stabilize the following methods:
IpAddr::is_globalIpAddr::is_documentationIpv4Addr::is_globalIpv4addr::is_sharedIpv4Addr::is_ietf_protocol_assignmentIpv4addr::is_benchmarkingIpv4Addr::is_reservedIpv6Addr::is_globalIpv6Addr::is_unique_localIpv6Addr::is_unicast_link_local_strictIpv6Addr::is_unicast_link_localIpv6Addr::is_unicast_site_localIpv6Addr::is_documentationIpv6Addr::is_unicast_globalIpv6Addr::multicast_scopeStabilize the following enum:
Ipv6MulticastScope