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.

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

proposal: net/netip: add Unmap() to netip.Is4() #49970

Closed
nleiva opened this issue Dec 3, 2021 · 12 comments
Closed

proposal: net/netip: add Unmap() to netip.Is4() #49970

nleiva opened this issue Dec 3, 2021 · 12 comments
Labels
Milestone

Comments

@nleiva
Copy link

@nleiva nleiva commented Dec 3, 2021

What version of Go are you using (go version)?

$ devel go1.18-a174638a5c Fri Dec 3 14:28:11 2021 +0000

Does this issue reproduce with the latest release?

This is for Go 1.18

What operating system and processor architecture are you using (go env)?

go env Output
$ go env

What did you do?

Currently to check if an IP address ip is an IPv4, you have to do ip.Unmap().Is4(), as IPv4 addresses are mapped into 128 bits, so you first need to Unmap it, to then use the function Is4.

For IPv6 you can simply do ip.Is6(), but ip.Unmap().Is6() will also work.

This is an example: https://go.dev/play/p/slX6Bjf9Ee1?v=gotip

What did you expect to see?

I'm just wondering if there is a chance to have either Unmap called inside the Is4 and Is6 functions, or have a new API that is consistent for v4 and v6, so we can do: ip.IsV4() or ip.IsV6() for example.

What did you see instead?

Not a biggie, just a minor cosmetic thing for very picky people like me.

@mdlayher
Copy link
Member

@mdlayher mdlayher commented Dec 3, 2021

Note that net/netip also has IP address parsing. I've modified your example a bit: https://go.dev/play/p/nGkMK5a0jQR

Using both packages is a bit awkward but I expect most callers should migrate to net/netip exclusively.

@nleiva
Copy link
Author

@nleiva nleiva commented Dec 4, 2021

Thanks Matt, your example is more concise and it better illustrates the point I'm trying to make. You need to add: ?v=gotip to the link for anyone to run it: https://go.dev/play/p/nGkMK5a0jQR?v=gotip

@seankhliao
Copy link
Member

@seankhliao seankhliao commented Dec 6, 2021

Is this still needed then?

@nleiva
Copy link
Author

@nleiva nleiva commented Dec 6, 2021

Yes. In a nutshell, I'm looking for the possibility to change:

ip.Unmap().Is4()
ip.Unmap().Is6()

to

ip.Is4()
ip.Is6()

By doing Unmap inside IsX. Otherwise it feels like we are mixing the nature of the IP address with library implementation details in the API when trying to find out if the address is IPv4 or IPv6. This might be OK, I just wanted to see what other think about this.

@toothrot toothrot changed the title net/netip: add Unmap() to netip.Is4() proposal: net/netip: add Unmap() to netip.Is4() Dec 6, 2021
@toothrot toothrot added this to the Proposal milestone Dec 6, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Dec 8, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Dec 8, 2021

ip.Is4() and ip.Unmap().Is4() are asking different, both completely reasonable questions.
The latter is accepting IPv4-in-IPv6-mapped IPv4 addresses, while the former does not.
It seems like we should keep both. Sometimes people want one, sometimes the other.

Is there anything unclear in the docs? It sounds like you found what you wanted and just wish it was shorter.

@nleiva
Copy link
Author

@nleiva nleiva commented Dec 8, 2021

Thank Russ, I agree both forms answer different questions. In my limited experience, users will use Is4 as the documentation suggests: "to reports whether ip is an IPv4 address", and experience frustration when it returns false even if it's indeed an IPv4 address, but it just happens it ended up as mapped IPv6 because of the library representation of an Addr (128 bits). Yes this is documented: "It returns false for IP4-mapped IPv6 addresses". I think it might be confusing for some users, but of course I'm probably off here.

What if both Is4 and Is6 Unmap by default, considering this would be the most popular use-case (find out the type of the address without caveats), to make it simple and shorter for the user, and add another API IsMapped to find out if it is an IPv4-in-IPv6-mapped address for the rest of the cases? Thanks

@mdlayher
Copy link
Member

@mdlayher mdlayher commented Dec 8, 2021

... and add another API IsMapped to find out if it is an IPv4-in-IPv6-mapped address for the rest of the cases?

This does exist today as https://pkg.go.dev/net/netip#Addr.Is4In6.

@nleiva
Copy link
Author

@nleiva nleiva commented Dec 8, 2021

... and add another API IsMapped to find out if it is an IPv4-in-IPv6-mapped address for the rest of the cases?

This does exist today as https://pkg.go.dev/net/netip#Addr.Is4In6.

Good catch! In fact Unmap calls it, thank you.

Bottom line is I personally believe, so take it with a grain of salt, people won't care as much about whether the address is mapped or not, so why not make it simpler for the potential most popular use case, which is finding out what type of IP address it is: IPv4 or IPv6. Thanks

@rsc
Copy link
Contributor

@rsc rsc commented Dec 8, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals Dec 8, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Dec 15, 2021

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Decline in Proposals Dec 15, 2021
@nleiva
Copy link
Author

@nleiva nleiva commented Dec 16, 2021

Understood. This is just a nice to have. Thanks for taking the time to discuss it.

@rsc rsc moved this from Likely Decline to Declined in Proposals Jan 5, 2022
@rsc
Copy link
Contributor

@rsc rsc commented Jan 5, 2022

No change in consensus, so declined.
— rsc for the proposal review group

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Proposals
Declined
Development

No branches or pull requests

5 participants