Add Ipv6Addr::to_ipv4_mapped by nanpuyue · Pull Request #75019 · rust-lang/rust

nanpuyue · 2020-08-01T17:13:30Z

add Ipv6Addr::to_ipv4_mapped
~~deprecate Ipv4Addr::to_ipv6_compatible & Ipv6Addr::to_ipv4~~ reference: Add a note for Ipv4Addr::to_ipv6_compatible #75150

According to IETF RFC 4291, the "IPv4-Compatible IPv6 address" is deprecated.

2.5.5.1. IPv4-Compatible IPv6 Address
The "IPv4-Compatible IPv6 address" was defined to assist in the IPv6
transition. The format of the "IPv4-Compatible IPv6 address" is as
follows:
| 80 bits | 16 | 32 bits |
+--------------------------------------+--------------------------+
|0000..............................0000|0000| IPv4 address |
+--------------------------------------+----+---------------------+
Note: The IPv4 address used in the "IPv4-Compatible IPv6 address"
must be a globally-unique IPv4 unicast address.
The "IPv4-Compatible IPv6 address" is now deprecated because the
current IPv6 transition mechanisms no longer use these addresses.
New or updated implementations are not required to support this
address type.

And the current implementation of Ipv4Addr::to_ipv6_compatibleis incorrect: it does not check whether the IPv4 address is a globally-unique IPv4 unicast address.

Please let me know if there are any issues with this pull request.

rust-highfive · 2020-08-01T17:13:33Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @hanna-kruppe (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.

tesuji · 2020-08-02T04:31:17Z

Suggested change
pubfn to_ipv4_mapped(&self) -> Option<Ipv4Addr>{
#[unstable(feature = "ipv4_mapped", issue = "none")]
pubfn to_ipv4_mapped(&self) -> Option<Ipv4Addr>{

Do I need to add #![feature(ipv4_mapped)] to the lib.rs?

Yes, certainly.

Although there is already an unstable feature named ip, do I still need to add a new unstable feature?
#27709
https://github.com/rust-lang/rust/blob/master/library/std/src/net/ip.rs#L1-L7

I asked on Zulip and it looks like adding stuff to existing unstable features is fine. So just use the ip feature :)

Since you requested another review: the unstable attribute is still missing. Please add #[unstable(feature = "ip", issue = "27709")] right above the line containing pub fn.

Since the following lines have been added at the beginning of this file (ip.rs):
#![unstable( feature = "ip", reason = "extra functionality has not been \ scrutinized to the level that it should \ be to be stable", issue = "27709")]
Adding #[unstable(feature = "ip", issue = "27709")] above this fn is unnecessary.

(It is better to give a permanent link to those code lines in the future).

hanna-kruppe · 2020-08-02T10:06:27Z

@lzutao is right, the deprecation should not happen before the replacement is stable. It's also not clear to me why the deprecation in the IETF RFC ("New or updated implementations are not required to support this address type.") should translate to a deprecation of the Rust API (which remains useful for any particular implementation that intends to support such addresses in some way). Both issues can be side-stepped by only adding the new encoding for now, without deprecating anything. Deprecating could be a separate discussion at a later date (e.g., after stabilization of the new API) and should probably go through @rust-lang/libs in some way.

nanpuyue · 2020-08-02T11:06:07Z

There is no replacement for Ipv4Addr::to_ipv6_compatible, it just should be deprecated for the following reasons:

The current version of IETF RFC 4291 was released on 2006-02-17. It was a long time ago and before the birth of Rust. The "IPv4-Compatible IPv6 address" was deprecated before 2006.
The current implementation of Ipv4Addr::to_ipv6_compatible and Ipv6Addr::to_ipv4 (IPv4-Compatible part) does not check whether the IPv4 address is a globally unique unicast address. This is incorrect (for example, conflicts with the unspecified address and the loopback address), and may mislead the caller.

tesuji · 2020-08-02T11:08:16Z

does not check whether the IPv4 address is a globally unique unicast address

Do you want to fix it in a different PR although the intention is to deprecate it ?

nanpuyue · 2020-08-02T11:11:14Z

@lzutao If we want to fix it, we may need to change the return type of Ipv4Addr::to_ipv6_compatible to Option<Ipv6Addr> or panic when the requirements are not met.

nanpuyue · 2020-08-02T11:20:43Z

Perhaps it should be discussed when to deprecate Ipv6Addr::to_ipv4.

nanpuyue · 2020-08-02T12:23:11Z

@lzutao #75046

hanna-kruppe · 2020-08-03T11:29:04Z

Deprecating a stable method without deprecation sounds like something that needs a @rust-lang/libs FCP, so (rolls dice) r? @LukasKalbertodt

LukasKalbertodt

I don't know a lot about IP6 and that stuff, but since it looks like the RFC defines these mappable addresses, it seems useful to add this method, I think.

@hanna-kruppe @lzutao Do you agree adding this as unstable makes sense?

LukasKalbertodt · 2020-08-08T13:19:00Z

You still need to add the unstable attribute.

hanna-kruppe · 2020-08-08T13:54:35Z

I don't know much about networking either, but FWIW I do agree.

LukasKalbertodt · 2020-08-10T23:47:43Z

This paragraph should still be improved. See my suggestion here

LukasKalbertodt · 2020-08-10T23:49:48Z

Since you requested another review: the unstable attribute is still missing. Please add #[unstable(feature = "ip", issue = "27709")] right above the line containing pub fn.

LukasKalbertodt · 2020-08-11T09:53:43Z

Thanks, the docs look fine now.

But I am somewhat confused: the unstable attribute is still not on the method, right? Or am I completely overlooking something? I am confused as we asked you to add the attribute several times now. Did you overlook those comments by any chance? If it's not clear what we want you to add, just ask :)
But yeah, this PR won't be merged without #[unstable(feature = "ip", issue = "27709")] so you really have to add it. I am actually confused that CI is green with a public method without stable or unstable attribute...

nanpuyue · 2020-08-11T13:24:59Z

@LukasKalbertodt
You may have overlooked this comment from me: #75019 (comment)

LukasKalbertodt · 2020-08-12T14:00:04Z

@nanpuyue Oh I indeed missed that comment, I'm sorry! I have never seen the unstable attribute being applied to a module. Interesting!

@bors r+

bors · 2020-08-12T14:00:06Z

📌 Commit d892a07 has been approved by LukasKalbertodt

bors · 2020-08-12T14:00:07Z

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

bors · 2020-08-12T16:30:52Z

⌛ Testing commit d892a07 with merge 3df25ae...

bors · 2020-08-12T18:50:14Z

☀️ Test successful - checks-actions, checks-azure
Approved by: LukasKalbertodt
Pushing 3df25ae to master...

@joshtriplett

…le, r=LukasKalbertodt Add a note for Ipv4Addr::to_ipv6_compatible Previous discussion: rust-lang#75019 > I think adding a comment saying "This isn't typically the method you want; these addresses don't typically function on modern systems. Use `to_ipv6_mapped` instead." would be a good first step, whether this method gets marked as deprecated or not. _Originally posted by @joshtriplett in rust-lang#75150 (comment)

Move stability attribute for items under the `ip` feature The `#[unstable]` attribute for items under the `ip` feature is currently located on the `std::net::ip` module itself. This is unusual, and less readable. This has sidetracked discussion about these items numerous times (rust-lang#60145 (comment), rust-lang#76098 (comment), rust-lang#76098 (comment), rust-lang#75019 (comment), rust-lang#75019 (comment)) and lead to incorrect assumptions about which items are actually stable (rust-lang#60145 (comment), rust-lang#76098 (comment)). This PR moves the attribute from the module to the items themselves.

rust-highfive assigned hanna-kruppe Aug 1, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 1, 2020

nanpuyue force-pushed the to_ipv4_mapped branch 4 times, most recently from a89e2ed to 5f0be61Compare August 2, 2020 03:13

tesuji reviewed Aug 2, 2020
View reviewed changes

nanpuyue force-pushed the to_ipv4_mapped branch 3 times, most recently from c889fc3 to 288784eCompare August 2, 2020 06:20

tesuji reviewed Aug 2, 2020
View reviewed changes

Comment threadlibrary/std/src/net/ip.rs Outdated

rust-highfive assigned LukasKalbertodt and unassigned hanna-kruppe Aug 3, 2020

hanna-kruppe added needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. T-libs-api [DEPRECATED; DO NOT USE] labels Aug 3, 2020

nanpuyue mentioned this pull request Aug 4, 2020
Fix Ipv6Addr::to_ipv4 #75046
Closed

nanpuyue force-pushed the to_ipv4_mapped branch from 288784e to 4639fe8Compare August 4, 2020 15:34

This was referenced Aug 4, 2020
Add a note for Ipv4Addr::to_ipv6_compatible #75150
Merged
Tracking issue for Ipv{4,6}Addr convenience methods #27709
Open

LukasKalbertodt suggested changes Aug 8, 2020
View reviewed changes

nanpuyue force-pushed the to_ipv4_mapped branch from 4639fe8 to 306860aCompare August 10, 2020 11:57

nanpuyue force-pushed the to_ipv4_mapped branch from 306860a to 124d777Compare August 10, 2020 12:23

nanpuyue requested a review from LukasKalbertodt August 10, 2020 15:39

LukasKalbertodt suggested changes Aug 10, 2020
View reviewed changes

nanpuyue commented Aug 11, 2020
View reviewed changes

Comment threadlibrary/std/src/net/ip.rs Outdated

add Ipv6Addr::to_ipv4_mapped
d892a07

nanpuyue force-pushed the to_ipv4_mapped branch from 124d777 to d892a07Compare August 11, 2020 01:16

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 12, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 12, 2020

bors merged commit 3df25ae into rust-lang:masterAug 12, 2020

bors mentioned this pull request Aug 12, 2020
Avoid recompiling std when tests change in std::net #75108
Closed

This was referenced May 23, 2021
IPv4-in-IPv6 Address Support #85609
Open
Move stability attribute for items under the ip feature #85672
Merged

cuviper added this to the 1.47.0 milestone May 2, 2024

	pubfn to_ipv4_mapped(&self) -> Option<Ipv4Addr>{
	#[unstable(feature = "ipv4_mapped", issue = "none")]
	pubfn to_ipv4_mapped(&self) -> Option<Ipv4Addr>{

Uh oh!

Uh oh!

Conversation

nanpuyue commented Aug 1, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Aug 1, 2020

Uh oh!

tesujiAug 2, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hanna-kruppe commented Aug 2, 2020

Uh oh!

nanpuyue commented Aug 2, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tesuji commented Aug 2, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nanpuyue commented Aug 2, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nanpuyue commented Aug 2, 2020

Uh oh!

nanpuyue commented Aug 2, 2020

Uh oh!

hanna-kruppe commented Aug 3, 2020

Uh oh!

LukasKalbertodt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hanna-kruppe commented Aug 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LukasKalbertodt commented Aug 11, 2020

Uh oh!

nanpuyue commented Aug 11, 2020

Uh oh!

LukasKalbertodt commented Aug 12, 2020

Uh oh!

bors commented Aug 12, 2020

Uh oh!

bors commented Aug 12, 2020

Uh oh!

bors commented Aug 12, 2020

Uh oh!

bors commented Aug 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

nanpuyue commented Aug 1, 2020•
edited
Loading

tesujiAug 2, 2020•
edited
Loading

nanpuyue commented Aug 2, 2020•
edited
Loading

tesuji commented Aug 2, 2020•
edited
Loading

nanpuyue commented Aug 2, 2020•
edited
Loading