[System.Net] Introduce CIDR IPNetwork

[mddddb]

PR for a type created based on #79946

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

null

Author:	mddddb
Assignees:	mddddb
Labels:	area-System.Net
Milestone:	-

[antonfirsov]

Some comments regarding validation, more to come.

You also need to generate reference source: https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/updating-ref-source.md#for-most-assemblies-within-libraries

(I can do it for you if you want.)

[mddddb]

Some comments regarding validation, more to come.

You also need to generate reference source: https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/updating-ref-source.md#for-most-assemblies-within-libraries

(I can do it for you if you want.)

@antonfirsov I was actually having a problem with this one, even tests were not able to see IPNetwork😅 Thanks!
Will give it a read and make the changes later, or if you can make the change - I would appreciate that

[antonfirsov]

/azp run runtime-extra-platforms

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[antonfirsov]

@dotnet/ncl this is ready for review now. Since I changed most of the original implementation, it would need an approval from someone else.

Downlevel packaging will be implemented in a separate PR.

[stephentoub]

Is what this renders for default(IPNetwork).ToString() desirable? Seems like here and below we should special-case BaseAddress == null to be the same as BaseAddress == IPAddress.Any.

[antonfirsov]

Not big fan of making default(IPNetwork) equivalent to another arbitrary state. Encountering such instances is a sign of a programming error in most of the cases. Would it be very unconventional if ToString() returned something like <uninitialized> instead?

[antonfirsov]

IPAddress.Any

Note that it could be also IPAddress.IPv6Any.

[stephentoub]

Not big fan of making default(IPNetwork) equivalent to another arbitrary state.

I think we need to make default(IPNetwork) valid. If you were forced to add a new IPNetwork() constructor, what would its behavior be? What would an IPNetwork.Empty do? Etc. Whatever that is, I think we should make default(IPNetwork) behave equivalently. Having the default value of a struct randomly fail is an anti-pattern. We typically only do it on our structs not meant to be used directly, e.g. helper types used by a compiler or other tools.

[antonfirsov]

If you were forced to add a new IPNetwork() constructor, what would its behavior be? What would an IPNetwork.Empty do?

One can argue that default(IPNetwork) is an empty network that doesn't contain any IPAddress, however note that it's not possible to define such an instance with an IP/prefix-length pair.

Having the default value of a struct randomly fail is an anti-pattern.

I have got rid of the exceptions, so the current PR behavior of default(IPNetwork) is what I just described above ("empty network"). The methods don't fail anymore, we just don't pretend that there is a CIDR pair when none was defined.

Personally I find this to be a good compromise, but if you think it is not acceptable, and we should assign an IPAddress to the BaseAddress == null case, I don't think it would be a consistent behavior to special-case it only only in method implementations, we should rather do something like:

public readonly struct IPNetwork
{
    private readonly IPAddress _address;

    public IPAddress BaseAddress => _address ?? IPAddress.Any;
}

but I personally find this worse because IPAddress.Any is an arbitrary choice.

[stephentoub]

default(IPNetwork) is all zeros. Imagine IPAddress were a struct rather than a class. What would its all zero's value be? That's IPAddress.Any. Hence why I was suggesting it behave like IPAddress.Any + PrefixLength==0.

I do think you should make BaseAddress return something if the address is null. Otherwise, the nullable annotations are wrong: it's advertising it will always return something non-nullable, but it might return null. Alternatively, you could make the property be public IPAddress? BaseAddress { get; }.

And then Contains would also need to be tweaked, either making it return true rather than false if the address is null, or if we make it so that BaseAddress is never null (my preference), just using whatever it is and computing the answer based on that without any special-casing.

[antonfirsov]

That's IPAddress.Any

Or IPAddress.IPv6Any, depends on the implementation. I don't think this case is similar to eg. default(Memory<T>). Using default(IPNetwork) is a programming bug in 99% of the cases. As a user, I would expect a sw library to help detecting it and "fail fast" instead of hiding it with some arbitrary "correct" behavior. But based on your feedback, I assume this is not our preference with BCL structs. I wish we had design guidelines covering such details.

[stephentoub]

I wish we had design guidelines covering such details.

We do. They say such values should work.
cc: @bartonjs

[antonfirsov]

/azp run runtime-extra-platforms

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[antonfirsov]

All feedback has been addressed, and all CI failures are unrelated. Will merge this if there are no further suggestions.