Allow Copy Construction or "Casting" to Different Sizes? about python-macaddress HOT 18 CLOSED

Here is the kind of stuff that this enables:

OUI(EUI48('01-23-45-67-89-ab'))  # OUI('01-23-45')
EUI48(OUI('01-23-45'))  # EUI48('01-23-45-00-00-00')

from python-macaddress.

The prefix-getter generalization problem:

Version 1.* of this library has a special case of an attribute called .oui on the provided address classes which gets the first three bytes of that address as an OUI object.

This was a good "quick, clean, simple" solution, something that seemed useful enough to have that I didn't want to just leave it out, but the exact way of doing it is something that I chose so I didn't block myself indefinitely just to figure out the best solution for it.

But that interface shape is more rigid, and also less elegant, in the logical elegance sense: it costs more logical "stuff" to achieve the same result.

With this interface, in order to get a prefix out of an address class, we need

1. a class for the prefix (f.e. OUI),
2. a function (decorated @property, but that's just sugar, not substance) that takes that size of prefix from the front of the address (f.e. _StartsWithOUI.oui), and
3. a way to put that function on each address class that starts with that type of prefix (f.e. inherit from _StartsWithOUI).

But if a shorter address class' constructor can take a longer address class instance as an argument, then in order to get a prefix out of an address class, the above requirements are reduced to just:

1. a class for the prefix (f.e. OUI).

Of course, it's trivial to just cast addresses to byte strings, and then slice them up as needed: OUI(bytes(address)[0:3]). Arguably it's so trivial that maybe the better move is to just delete the OUI-specific .oui helper entirely. So this by itself isn't really a huge win or need or improvement. It's only minimally nice, a little bit of helpfulness for interactive use on the fly, and it saves you from writing redundant information like the size of your prefix class in different places (in bits vs bytes, so some text searches you might think to do in a code base won't even find the relevant information for you).

It's also trivial to define your own subclasses for stuff like this as-needed. I'm guessing that most real-world code that has a use-case for their own prefix sizes/classes only needs one, so it would take less time to just write some helper functions or subclasses with new methods/properties as needed than it's taking me to think and write about it here.

But the way this elegantly and generally solves all get-prefix-from-address cases is notable.

Consider how much simpler, cleaner, and easier it becomes to add support for MA-S/OUI-36 prefixes, or IAB prefixes, or for any other prefix, both in this library and especially as a user from the outside.

from python-macaddress.

If it turns out that this enables or conceptually fits with a very elegant solution to #7, I think that would be strong evidence that this is a good idea, because we'd have consilience of at least two different problems.

But so far it seems like it just gives us the option to write

PrefixClass(address) == prefix

instead of

bytes(address).startwith(bytes(prefix))

and that doesn't really feel like much of a win.

(Related concept: perhaps a startwith method, which takes the prefix HWAddress instance as the other argument besides "self"?)

from python-macaddress.

I just realized that copy-construction isn't really documented in the README/description, but it has been covered by the tests for a while.

I have vague memories of originally not supporting copy construction, including consciously deciding against it after some thought, only to run into a use case for it.

But in version 1.* it was limited by isinstance(address, type(self)), so copy construction could only happen rootwards on the class hierarchy. (Incidentally, I think this opened up a way to instantiate HWAddress instances - ideally that should not be allowed, because HWAddress doesn't provide a size and most of its methods break if the class doesn't have a size.) This allows "casting" from child class to parent class.

And in the upcoming 2.*, copy construction is currently already planned/implemented to work for any address of the same size. This makes it less "copy constructor" functionality and more of a general "cast" functionality. I've fairly thoroughly convinced myself that this is useful, especially building on the decision in #3.

Now it's just a matter of whether or not it's worth generalizing across sizes.

from python-macaddress.

class IAB(HWAddress):
    size = 36

    def __init__(self, address):
        super().__init__(address)
        if OUI(self) not in (OUI('40-D8-55'), OUI('00:50:C2')):
            raise ValueError('invalid OUI for IAB')

Now imagine how achieving all of the same behavior would have to look if we don't support copy/cast construction from larger to smaller address sizes.... Near as I can tell:

1. You have to manually implement the OUI extraction, or get it by inheriting from _StartsWithOUI.

2. You have to do your own logic before calling into super().__init__ to check if you got a hardware address instance and if so then you have to extract and use just the correct initial bits from it.

   (If you don't, then any time someone wants to check if their address starts with an IAB or to extract that IAB, they have to manually implement it the way we would manually implement OUI extraction, by casting to bytes and replicating the size information to know what to slice.)

from python-macaddress.

Verdict (1/2):

1. Definitely yes on copy/cast construction downward in size - when the address parameter type has smaller size than the self parameter.

(Edit 2022-10-28: decided to change course on this, at least for v2)

Still need a verdict on:

2. Should we allow copy construction upward in size? This seems less obviously useful, although it becomes more obviously usable, just perhaps not very usefully usable, once we also layer in things like arithmetic and bitwise operator overloads. It's also in some ways obviously the symmetrical analog to downward casting - if the former is supported then it seems almost self-evident and natural to support the latter.

from python-macaddress.

I think this is the inflection point into overthinking. Worrying about the possible badness of allowing upwards-in-size casts and considering alternatives like errors in those cases, despite deciding on having downwards-in-size casts, and not seeing any serious unacceptable issues.

Generally it is better, if there is a natural/sensible way for something to work, for that thing to work instead of erroring out.

What's the worst possible unintended result of letting upwards-in-size casting work?

from python-macaddress.

Verdict (2/2):

Yes. Casting both up and down in address sizes will be implemented in version 2.

(Edit 2022-10-28: decided to change course on this)

from python-macaddress.

Perhaps instead of allowing any "cast" construction, this library should have a distinction between "prefix" classes and normal hardware address classes, and only allow "cast" construction when at least one of the two address classes involved is a prefix.

I had forgotten this consideration, but my example here reminded me of it:

Therefore, people could justifiably have written code such as macaddress.EUI48(some_address) with the idea that if some_address was, say, an macaddress.EUI64 for some reason, it would get rejected with a TypeError rather than truncating.

So, if an EUI64 instance is passed into an EUI48 construction, is that more likely to be an error or a deliberate usage? If it's an error but it silently works, is that fine, because that error is best prevented elsewhere in the stack? Conversely, if it is deliberate but errors, what then?

from python-macaddress.

I remember briefly thinking about this before opening this issue, and concluding that the generality/universality was better than proactively adding another conceptual wrinkle to the model.

And of course I imagine that this issue doesn't happen unless you are intentionally writing code that takes advantage of the casting/copy construction feature.

However, the one annoying thing is that If the constructor handles this always or even just by default, then you don't get a great way to separate cases...

Let's say we have subclasses of EUI48 addresses for whatever reason, and we want to be able to pass them into code that always homogenizes the input into an EUI48 instance by passing it into an EUI48 construction call. Then due to some bug, an OUI or an EUI64 or a CDI40 or whatever gets passed in there.

Now obviously in some sense the Correct(tm) thing is for us as users to have read the documentation and played with the behavior of this library and thoroughly grokked the ramifications, and then when writing code that calls EUI(foo) we Of Course(tm) ask ourselves exactly what foo could be and is allowed to be. And then we decide either

1. the odds of it being a wrong size address class (by the time it gets to here) are too low, and even if it happens it will lead to (an obvious enough failure|a reasonable enough result), so no need to do anything;
2. the odds of it being wrong are high or if it happens the behavior is unacceptable, but
   • this is more appropriately handled elsewhere in the code, earlier in the code and data path; or
   • okay so I should slap down a type(foo).size == EUI48.size check here (if necessary also guard that check with an instanceof(foo, macaddress.HWAddress) check).

And honestly that seems pretty reasonable to me.

If this comes up enough, maybe we could add some secondary constructor class methods, or perhaps add flags to the main default __init__ constructor to allow people to turn off specific cases of what's allowed.

But realistically I also don't think that should come up very often, maybe even ever in ideally factored code. Because most code can justifiably (and arguably should) assume that other code has validated its inputs or ensured that they are the right type.

Basically, I'm not seeing a reason why this would be bad to allow universally, even for types where it doesn't quite make sense, because it achieves greater generality and less friction in the cases where it is desirable, and the cases where it is undesirable and actually can happen seem unlikely and just contrived by my "paranoid" edge-case thoroughness.

from python-macaddress.

Just jotting down this idea for the record in case it appeals to anyone else:

We could add a class variable, something like cross_size_castable or cross_size_convertible. If it is true on either of the two hardware address classes participating in the "cast" usage of __init__, then truncation or right-zero-padding can happen, otherwise no.

This would allow classes like EUI48 and and EUI64 to set this variable to false, so they could not be cast between each other, but OUI could still have it set to true and thus be castable to or from either of them.

But I think overall this just adds more complexity and variation in the possibilities than is justified by how little it wins in protecting against unintentional misbehavior in the edge cases.

from python-macaddress.

So I was having another perverse edge case thought:

I create:

class Bottom24Bits(HWAddress):
    size = 24
    formats = OUI.formats

And of course it turns out that if I proceed with #8 as planned, then OUI(Bottom24Bits) works, which seems like an obviously totally wrong thing.

I'm kinda tempted to say that this is an acceptably broken/bad edge case, because this seems like a mess that never really has a good reason to happen.

Why would you even want to parse or handle split up suffixes of addresses like this? When would it come up?

It seems super unlikely and unreasonable, basically. But I wanted to jot it down.

from python-macaddress.

All conflicts between this issue's idea and the last comment can probably be resolved by having those suffix classes (if they ever come up for anyone) overload __init__ a bit:

class Bottom24Bits(HWAddress):
    def __init__(self, value):
        if isinstance(value, HWAddress):
            value = int(value) & ((1 << type(self).size) - 1)
         super().__init__(value)

I haven't thought through the edge-cases much, because for now this whole address suffix idea is still approximately useless, although my mind is seeing a way to combine such classes (just not very usefully) to #19 and #7.

from python-macaddress.

Coming back to this half a year later, I'm strongly feeling the appeal of having some way of limiting casting across sizes to just classes that are declared as prefixes. So

• these would work: MAC(OUI('12-34-56')) and OUI(MAC('12-34-56-78-90-AB')), but
• these would not work: MAC(EUI64('12-34-56-78-90-ab-cd-ef')) and EUI64(MAC('12-34-56-78-90-AB')).

I shut myself down on this last time because it felt like I was letting myself overthink, and maybe I still am, but in some ways it still seems very right to limit cross-size casts to just classes that are intended/known to be pieces of full addresses rather than full addresses in their own right.

*sigh*

Ultimately it comes down to whether or not it makes more sense for a library like this to provide

• raw address-like objects which are basically fancy MSB-aligned integers, or
• opinionated taxonomy into addresses and pieces of addresses.

from python-macaddress.

Another thought that I apparently never wrote: could provide casting methods instead of cast-construction.

For example,

• MAC('12-34-56-78-90-ab').get_prefix(OUI) could return OUI('12-34-56').
• OUI('12-34-56').expand_to(MAC) could return MAC('12-34-56-78-90-ab').

This seems like worse ergonomics than OUI(MAC('12-34-56-78-90-ab')) and MAC(OUI('12-34-56')), but maybe there's some merit to it. Especially the explicitness of doing a size-changing operation instead of just any construction/cast, and having to explicitly couple your code to the intended direction of size change (smaller by get_prefix, bigger by expand_to), which is a tradeoff that maybe helps with correctness more than it gets in the way of flexibility....

from python-macaddress.

Actually it's probably better to reverse the order: AddressClass.prefix_of(address_instance) instead of address_instance.get_prefix(AddressClass) (especially considering the state of the Python type hinting ecosystem, what fits better with thinking about types/classes, and what makes implementation easier/cleaner).

from python-macaddress.

The last thought provides another solution to the IAB example problem:

Instead of

class IAB(HWAddress):
    size = 36

    def __init__(self, address):
        super().__init__(address)
        if OUI(self) not in (OUI('40-D8-55'), OUI('00:50:C2')):
            raise ValueError('invalid OUI for IAB')

could just do

class IAB(HWAddress):
    size = 36

    def __init__(self, address):
        super().__init__(address)
        if OUI.prefix_of(self) not in (OUI('40-D8-55'), OUI('00:50:C2')):
            raise ValueError('invalid OUI for IAB')

And then of course IAB can inherit .prefix_of from HWAddress (or this could be moved into a HWAddressPrefix subclass, which then OUI and this hypothetical IAB could inherit from - might be good for other uses of taxonimizing address classes that can be prefixes...).

from python-macaddress.

New verdict!

1. No cast construction in v2.
2. No cast construction between arbitrary classes - maybe if explicitly prefix classes are involved, but definitely not arbitrary classes.
3. Maybe even separate dedicated methods for casting across sizes.

Rationale (summarizing and expanding the recent comments):

1. cast construction can hide errors, and right now that's seeming more important to me, possibly because
2. cast construction no longer seems as necessary, at least not the indiscriminate kind without explicit prefix subclasses, because we can add dedicated class methods with reasonably clean and concise names for it, and because the situations I imagined using it (bitwise operator, address ranges) don't really seem to end up being able to use it all that much in their implementations.

from python-macaddress.