fixes for cidr equality, validity and consistency

[martijnhoekstra]

the root of the problem: 172.168.0.1/30 isn't a valid cidr range, because 172.168.0.1 isn't a valid prefix for a /30, but is accepted by both Cidr.fromString and Cidr.apply, which now cause cidr#toString to output invalid cidr ranges.

This MR changes CIDR equality and printing:

equality:

• before: ip"172.168.0.1" / 30 != ip"172.168.0.0" / 30, even though it matches the same addressess
• after: ip"172.168.0.1" / 30 == ip"172.168.0.0" / 30, even though the base address is different

printing:

• before: (ip"172.168.0.1" / 30).toString == "172.168.0.1/30" even though that isn't a valid CIDR range
• after: (ip"172.168.0.1" / 30).toString == "172.168.0.0/30" even though that doesn't contain the base address

misc:

• (adress0 / 0).address == adress0 still holds
• cidr1 == cidr2 no longer implies cidr1.address == cidr2.address
• cidr1 == cidr2 no longer implies cidr1.productIterator.toList == cidr2.productIterator.toList

This is not the only way to fix the given problems. The other obvious option is at construction (Cidr.apply) time, set address to prefix, and deprecate address for prefix. This is a bigger breaking change: it will break users that assume (addr / 0).address == addr. It also kills the (somewhat?) useful ability to have this datatype carry at the same time an arbitrary IP address and some cidr range the address is part of. passing around some machines ip / 24 (or something) does come up a fair bit in practice, and comes "for free" in the current implementation.

A third option is to provide separate between strict and lenient constructors

[martijnhoekstra]

see also: #534

[mpilquist]

What if we left equality as it was but then added a method def normalized: Cidr[[A] = Cidr(prefix, prefixBits)? Or we could add a type like Cidr.Strict[A]?

[martijnhoekstra]

One of the concrete problems I'm running in to, is that Cidr#toString returns something that's not a CIDR range. So maybe that's the most reasonable place for a fix, either just toString itself (but if you do that, you lose Cidr.fromString(cider.toString).get == cider), or by making something like a Cidr#toSpec: String = s"$prefix/$prefixBits" (but then you don't have the spec format in interpolations)

a problem with cidr#normalize: Cidr[A] is that you'd never know before you if the cidr you have is already normalized or still needs to be, leading to potentially calling .normalize over and over. In that sense, I like the idea of having Cidr.Strict[A] with the semantics in this PR, but then that raises the question about what returns a Strict vs a normal Cidr: maybe most importently, which would you want the ip / bits return?

would you like some drafts of various options?

[mpilquist]

Okay, why don't we pursue the Cidr.Strict option and see what it looks like?

I'm concerned about overriding toString as it will likely confuse folks if only the prefix is printed. And I'm concerned that overriding equals/hashCode will lead to surprising bugs -- e.g., consider a hash map Map[Cidr[IpAddress], X] when the keys are representing both an ip and a netmask.

[martijnhoekstra]

main...martijnhoekstra:strict-cidr is a potential minimal Strict version. It could have its own fromString too.

I'd need to kick the tires on whether this would work for me in practice.

[mpilquist]

Looks good to me!

[armanbilge]

Nit: can't these be passed along as ctor args?

Suggested change

	final class Strict[+A <: IpAddress] private (override val prefix: A, override val prefixBits: Int)
	final class Strict[+A <: IpAddress] private (prefix: A, prefixBits: Int)

[martijnhoekstra]

for the prefix bits, yes, that's just plain better.

For prefix, if it's an override val, it overrides def prefix: A = address.transform(_.masked(Ipv4Address.mask(prefixBits)), _.masked(Ipv6Address.mask(prefixBits))) with the val, which adds a field but potentially saves a bit of useless work. I guess I'd rather take the extra field? What do you think?

[martijnhoekstra]

@armanbilge what do you think?

[armanbilge]

@martijnhoekstra sorry, I missed this.

Instead of adding an extra redundant field, can't we just do override def prefix = address?

[armanbilge]

• Micro-optimization + bump base version #545