Inconvenient NetworkSize

vorner commented 4 years ago

Hello

I wonder what the motivation behind the NetworkSize is. As I understand, the purpose of the IpNetwork enum is to generalize handling of both IPv4 and IPv6 networks. If it is so, why doesn't IpNetwork::size simply return u128? The NetworkSize isn't smaller, doesn't have many traits (eg. can't be summed together, doesn't have a Display implementation, doesn't support comparing to literals well…

Anyway, thank you for working on this, it is something I've stumbled upon multiple times that I'd like to have.

achanda commented 4 years ago

Hi @vorner thanks for bringing this up. It seemed natural that IPv4 size should be 32 bits and IPv6 should be u128. Given that, the only way to have an API around both the types was to have a generic NetworkSize and specialize in both cases. I do see your point about NetworkSize not being ergonomic. But do you think implementing a bunch of traits for it should be enough?

vorner commented 4 years ago

Implementing bunch of traits would help somewhat. Nevertheless, I still believe it would be less convenient that it would have to be and would be a lot of work and more code.

I do agree that size of IPv4 network range should be u32 and u128 for IPv6 network range. But I don't think the only way to handle the case of network range, I don't care if IPv4 or IPv6 one, has to be the enum. If I have a range whose type I don't particularly care about, I'll gladly accept any type holding any potential size it could have, which is u128. On the other hand, if I do care about if it is u32 or u128, I can do the match on the IpNetwork itself instead postponing it until the NetworkSize.

So, my proposals are (in order of how I personally like them):

Ipv4Network::size() -> u32, Ipv6Network::size() -> u128, IpNetwork::size() -> u128 (casting inside it to bigger type as needed).
NetworkSize having Into<u128> implementation, From<u128>, From<u32>, PartialEq<u128|u32>, PartialOrd<...>`, etc + bunch of other traits.

achanda commented 4 years ago

I see your point. I am a bit hesitant to change the external API since this has been out there for a while now. A quick Github search did not show any usage of NetworkSize though. I think I will open a PR to deprecate NetworkSize pointing to this issue, merge and cut a release. And later remove it and implement your first suggestion.

ctrlcctrlv commented 1 year ago

I think this should have been closed via #175.

Alextopher commented 6 months ago

I have a new idea for this type that I find would solve the problem elegantly.

// status quo
pub enum NetworkSize {
    V4(u32),
    V6(u128),
}

In this crate it's impossible (without unsafe) to create an IpNetwork (V4 or V6) of size 0. ::/128 and 0.0.0.0/24 both contain 1 address, there are no ::/129 or 0.0.0.0/25 subnets. This means that NetworkSize currently has 2 variants with bit patterns that are impossible to construct in normal use NetworkSize::V4(0) and NetworkSize::V6(0). Those 2 spare variants would be much better used to represent these edge cases of MAX_V4 and MAX_V6, or perhaps just MAX_V6 as a u128 / u64 can represent MAX_V4 perfectly fine and u128::MAX + 1 is the real problem here.

Rust has a great way of representing non-zero integers like this https://doc.rust-lang.org/stable/core/num/struct.NonZero.html.

If we're too keep the existing structure, but would like to improve the status quo on #130 #178, I'd recommend returning Option<NonZeroU128> or Option<NonZeroU32> in the places that would otherwise go out-of-bounds. This could be an internal-only implementation with well defined convenience methods for end users.

Due to niche optimizations by the rust compiler size_of(Option<NonZero<T>> == size_of(T) making this solution optimal in that sense. It also passes along more information to the compiler, which never hurts!

// proposal 1
pub enum NetworkSize {
    V4(NonZeroU32),
    V6(NonZeroU128),
    MAX_V6,
}

In addition to this change I would also make the implementation of NetworkSize private to this crate. I think there are only 3 user stories for this type:

What is the size of this network as a u32? What is the size as a u128? I know that ::/0 is an edge case, give me an Option<u128> instead.

None of these really require users know the inner workings of NetworkSize, as long as it has both highly convenient methods with safe alternatives. Think how slices have the both the index operators that panic and get(idx: usize) -> Option<T>.

It may be true that there are better implementations no-one has considered yet, and we can open the door to this change again in the future without breaking existing code.

// proposal 2
enum NetworkSizeInner {
    V4(NonZeroU32),
    V6(NonZeroU128),
    MAX_V6,
}

pub struct NetworkSize(NetworkSizeInner);

I would love to help resolve the long-term stabilization issue in this project, I'm happy to write up a Draft PR showing what the a stabilized API here could look like.

achanda / ipnetwork

Inconvenient NetworkSize #102