diff --git a/pcapkit/const/reg/apptype/apptype.py b/pcapkit/const/reg/apptype/apptype.py index 8abbee09fe..453f8347a4 100644 --- a/pcapkit/const/reg/apptype/apptype.py +++ b/pcapkit/const/reg/apptype/apptype.py @@ -11,10 +11,7 @@ """ from typing import TYPE_CHECKING, cast -from aenum import IntFlag, StrEnum, auto, extend_enum - -from pcapkit.utilities.compat import show_flag_values -from pcapkit.utilities.exceptions import ProtocolError +from aenum import IntEnum, StrEnum, extend_enum __all__ = ['TransportProtocol', 'AppType'] @@ -24,31 +21,42 @@ from pcapkit.corekit.multidict import MultiDict -class TransportProtocol(IntFlag): +class TransportProtocol(IntEnum): """Transport layer protocol.""" - # mypy has no aenum plugin, so this class is a plain class to it: a bare - # ``0`` here infers as int while the auto()-valued members below infer as - # Any, and only this member then disagrees with the TransportProtocol - # annotations that use it. cast is the identity function at run time, so - # this changes nothing that runs -- see GitHub issue #770. mypy.ini sets - # warn_redundant_casts, so if aenum ever ships type stubs letting it infer - # TransportProtocol on its own, this cast starts erroring instead of - # lingering as dead scaffolding. + # mypy has no aenum plugin, so this class is a plain class to it: every + # member below is a literal int rather than an auto()-valued one -- GitHub + # issue #808 dropped the IntFlag base, so auto() would number sequentially + # instead of by the power-of-two spacing the values must keep -- and a + # literal infers as int while the TransportProtocol annotations that use + # each member (__transport__, and the proto default on __new__, get and + # get_all) expect TransportProtocol. cast is the identity function at run + # time, so this changes nothing that runs -- see GitHub issue #770, which + # cast only this member while the rest still inferred Any from auto(). + # mypy.ini sets warn_redundant_casts, so if aenum ever ships type stubs + # letting it infer TransportProtocol on its own, these casts start + # erroring instead of lingering as dead scaffolding. #: No transport protocol. ``TransportProtocol(0) is undefined`` and #: ``bool(undefined)`` is ``False``; it is the ``proto`` sentinel default #: for ``__transport__``, ``__new__``, ``get`` and ``get_all``, and what #: the base registry's ``_missing_`` extends unassigned/reserved rows from. undefined = cast('TransportProtocol', 0) - #: Transmission Control Protocol. - tcp = auto() - #: User Datagram Protocol. - udp = auto() - #: Stream Control Transmission Protocol. - sctp = auto() - #: Datagram Congestion Control Protocol. - dccp = auto() + #: Transmission Control Protocol. Value fixed at ``1`` rather than + #: renumbered sequentially -- GitHub issue #808 dropped the ``IntFlag`` + #: base once nothing built a composite, but did not revisit the four + #: values themselves, which predate this class and are not its call to + #: renumber. + tcp = cast('TransportProtocol', 1) + #: User Datagram Protocol. See ``tcp`` above for why the value stays ``2`` + #: rather than becoming sequential. + udp = cast('TransportProtocol', 2) + #: Stream Control Transmission Protocol. See ``tcp`` above for why the + #: value stays ``4`` rather than becoming sequential. + sctp = cast('TransportProtocol', 4) + #: Datagram Congestion Control Protocol. See ``tcp`` above for why the + #: value stays ``8`` rather than becoming sequential. + dccp = cast('TransportProtocol', 8) @staticmethod def get(key: 'int | str') -> 'TransportProtocol': @@ -63,35 +71,35 @@ def get(key: 'int | str') -> 'TransportProtocol': return TransportProtocol(key) if key.lower() in TransportProtocol.__members__: return TransportProtocol[key.lower()] # type: ignore[misc] - max_val = max(TransportProtocol.__members__.values()) - return extend_enum(TransportProtocol, key.lower(), max_val * 2) - - @classmethod - def _missing_(cls, value: 'int') -> 'TransportProtocol': - """Lookup function used when value is not found. + # NOTE: maintainer ruling on this PR (#836): "Do not allow extension + # of TransportProtocol at all." A name that is not a declared member + # used to mint a brand-new one here, at ``max_val + 1`` (before that, + # ``max_val * 2``) -- an unbounded, ever-growing set of transport + # protocols nothing ever asked for. There is nothing left to walk + # now: it is simply refused, exactly like any other unrecognised + # name -- including one spelling a composite, e.g. ``'tcp|udp'``. + # ``'|'`` used to be intercepted here on its own, so a composite in + # disguise never got minted into a member whose own name lied about + # being a single transport; the owner's further ruling on this PR + # retired that special case along with the rest of the composite + # handling once TransportProtocol stopped being a Flag at all: + # "since it's no longer a Flag, `|` joined values are no longer + # parsed and accepted, we will treat it as a whole, instead of + # splitting." A ``'|'``-joined name is therefore not special any + # more -- it is simply not the name of a declared member, and gets + # the same message as any other one that is not. + raise ValueError(f'{key!r} is not a valid {TransportProtocol.__name__}') - Args: - value: Value to get enum item. - - Raises: - ValueError: If ``value`` sets a bit no member declares. - - Note: - This is what makes an unrecognised transport protocol *rejected* - rather than accepted -- GitHub issue #647's fix for this registry. - :mod:`aenum` on its own is permissive here and composes whatever bits - it is handed: measured on aenum 3.1.17 with this method removed, - ``TransportProtocol(-1)`` returns ``tcp|udp|sctp|dccp``, which is - #647's recorded defect for this class verbatim, and - ``TransportProtocol(16)`` returns a member whose ``name`` is - :obj:`None`. Declared bits still compose, since a service assigned to - several transport protocols is the ordinary case rather than the - exception. - - """ - if not (isinstance(value, int) and 0 <= value <= max(cls.__members__.values()) * 2 - 1): - raise ValueError(f'{value!r} is not a valid {cls.__name__}') - return super()._missing_(value) + # NOTE: ``_missing_`` used to range-check ``value`` and then defer to + # :mod:`aenum`'s own ``Flag._missing_``, which is what composed an + # unrecognised bit combination into a pseudo-member -- ``TransportProtocol(3)`` + # returning ``tcp|udp`` -- GitHub issue #647's guard against that composing + # *anything*, including values no member declares. GitHub issue #808 removed + # the ``IntFlag`` base once nothing built a composite, and with it the only + # reason this method existed: a plain :class:`~aenum.IntEnum` already raises + # ``ValueError`` for a value no member declares, with no ``_missing_`` of + # its own needed to get there, so declaring one here would only be + # reproducing what the base class already does. class AppType(StrEnum): @@ -2344,26 +2352,30 @@ def __hash__(self) -> 'int': return hash(self.port) @classmethod - def _dispatch(cls, key: 'int', proto: 'TransportProtocol | str') -> 'Type[AppType]': + def _dispatch(cls, key: 'int', proto: 'TransportProtocol | str | int') -> 'Type[AppType]': """The registry that owns ``proto``, or ``cls`` where it is one already. Args: key: Port number the caller is looking up, validated here so that every entry point rejects a non-port identically. - proto: Transport protocol, as a flag or its name. Exactly one, on - the delegating path -- see :exc:`~pcapkit.utilities.exceptions.ProtocolError` - below. + proto: Transport protocol, as a member, its name, or a bare + :class:`int`. That last shape is not merely defensive: GitHub + issue #808 dropped ``TransportProtocol``'s ``IntFlag`` base, so + ``TransportProtocol.a | TransportProtocol.b`` -- built by hand, + the same as any caller passing a literal port-transport bitmask + -- falls through to ``int.__or__`` and returns a bare + :class:`int` rather than a member. Never split back into the + transports its bits would each name -- owner ruling on this PR + (#836) -- so it is refused as a whole exactly like any other + value naming no registry. Returns: The registry class to search. Raises: - ValueError: If ``key`` is not a port number, or if ``cls`` holds no - members and ``proto`` names no registry to delegate to. - ProtocolError: If ``cls`` holds no members and ``proto`` names more - than one transport protocol, which names more than one registry - and so no single answer. Itself a :exc:`ValueError`, so a caller - catching that keeps catching this. + ValueError: If ``key`` is not a port number, or if ``proto`` -- + member or bare int, composite or not -- names no registry to + delegate to. """ # NOTE: this registry resolves ports, not service names. The old string @@ -2379,52 +2391,52 @@ def _dispatch(cls, key: 'int', proto: 'TransportProtocol | str') -> 'Type[AppTyp if isinstance(proto, str): proto = TransportProtocol.get(proto.lower()) - # NOTE: ``TransportProtocol`` is an :class:`~aenum.IntFlag`, so ``tcp | - # udp`` stays constructible by hand even though no member carries one any - # more -- every member's ``proto`` is the single transport of the registry - # it lives in, GitHub issue #806. A composite names two registries, - # though, holding two different services for the same port -- and a lookup - # answers with one member, so there is no answer to which of them the - # composite meant. Resolving it picked the lowest set bit: - # :func:`~enum.show_flag_values` iterates LSB-first and ``tcp`` is the - # lowest, so every composite containing it dispatched into the TCP - # registry whatever else it named. Measured on fe80b8525, when members - # still carried the whole set, that answered 46 of the 10,625 multi-transport - # members' own ``get(m.port, proto=m.proto)`` with a service other than the - # member's own, of which 23 -- the UDP-declared half -- came back as a - # member of the *TCP* registry, carrying the wrong type and a narrower - # ``proto``. Two of those named a service UDP does not answer for the port - # at all, and they are the whole blast radius: ``AppType.get(888, proto=tcp - # | udp)`` gave ``cddbp`` for ````, and - # 999 gave ``garcon`` against UDP's ``applix``. The other 44 differ from - # ``m.svc`` only in that the member is not its port's canonical, which a - # single-bit lookup does too and which ``get`` documents. All of it silent, - # and undetectable to a caller checking equality, since ``__eq__`` compares - # on ``port`` alone. GitHub issue #759, whose body found 888 and - # generalised from it. - # - # Refusing the composite is the honest answer and costs - # nothing: a caller resolving a parsed port knows which transport carried - # it and passes that one bit, which is what every call site in - # :mod:`pcapkit` does, and one that wants every service on a port asks each - # registry in turn. This is the delegating path only -- a registry subclass - # returned above already knows its own transport and documents ``proto`` as - # ignored, so nothing there is ambiguous to begin with. - namespaces = show_flag_values(proto) - if len(namespaces) > 1: - raise ProtocolError(f'{proto!r} names {len(namespaces)} transport protocols, and so ' - f'{len(namespaces)} registries of {cls.__name__}; look the port up ' - 'under one transport protocol at a time') - if namespaces: - subclass = cls.__registries__.get(TransportProtocol(namespaces[0])) - if subclass is not None: - return subclass + + # NOTE: a direct dict lookup covers every genuine single transport -- + # one of the four real members, or a bare int equal to one of their + # values -- since ``__registries__`` keys compare by the same + # int-valued hash/eq every member and bare int alike already use. The + # cast is for mypy alone: ``dict.get`` accepts any hashable key at run + # time regardless of its declared key type, but mypy holds ``.get`` to + # the dict's own ``TransportProtocol`` keys, and does not know ``proto`` + # can genuinely be a bare :class:`int` here now that GitHub issue #808 + # dropped the ``IntFlag`` base -- see the annotation on ``proto`` above. + subclass = cls.__registries__.get(cast('TransportProtocol', proto)) + if subclass is not None: + return subclass + + # NOTE: everything that reaches here names no registry, and nothing + # below decodes ``proto``'s bits looking for a partial answer. A + # genuine member reaching this point is ``undefined`` -- the four + # real transports would already have resolved above, and + # :meth:`TransportProtocol.get` cannot mint anything else, per this + # PR's own maintainer ruling against extending TransportProtocol at + # all -- and a bare :class:`int` is refused exactly the same way + # whether it is a single stray bit, e.g. ``17``, or a composite of + # several real transports, e.g. ``3`` (``tcp | udp``). That composite + # case used to get its own + # :exc:`~pcapkit.utilities.exceptions.ProtocolError`, decoded through + # :func:`~pcapkit.utilities.compat.show_flag_values` and naming every + # transport whose bit was set -- the fix for GitHub issue #759, where + # resolving a composite by picking its lowest set bit dispatched + # every one containing ``tcp`` into the TCP registry regardless of + # what else it named. The owner's further ruling on this PR (#836) + # retired that decoding along with the rest of the composite + # handling: "since it's no longer a Flag, `|` joined values are no + # longer parsed and accepted, we will treat it as a whole, instead of + # splitting." So ``AppType.get(80, proto=3)`` now says "3 names no + # transport protocol registry" rather than naming ``tcp`` and ``udp`` + # individually -- the same answer a caller resolving a parsed port + # already gets right, since it knows which single transport carried + # it and passes that one bit, and the same answer a caller wanting + # every service on a port already has to ask each registry for in + # turn regardless. raise ValueError(f'{proto!r} names no transport protocol registry of ' f'{cls.__name__}') @classmethod def get(cls, key: 'int', *, - proto: 'TransportProtocol | str' = TransportProtocol.undefined) -> 'AppType': + proto: 'TransportProtocol | str | int' = TransportProtocol.undefined) -> 'AppType': """Backport support for original codes. Args: @@ -2452,10 +2464,11 @@ def get(cls, key: 'int', *, this registry resolves ports and not service names -- including one outside ``0..65535``, whose rejection by :meth:`_missing_` this method propagates rather than minting over, so that ``get`` is - never more permissive than ``AppType(...)``. - ProtocolError: If ``proto`` names more than one transport protocol -- - see :meth:`_dispatch`, which refuses it rather than answering from - whichever registry the lowest set bit happens to name. + never more permissive than ``AppType(...)``. Also covers a + ``proto`` naming more than one transport protocol -- a + composite built by hand is refused as a whole rather than + answered from any one of the registries it names -- see + :meth:`_dispatch`. :meta private: """ @@ -2489,7 +2502,7 @@ def get(cls, key: 'int', *, @classmethod def get_all(cls, key: 'int', *, - proto: 'TransportProtocol | str' = TransportProtocol.undefined) -> 'tuple[AppType, ...]': + proto: 'TransportProtocol | str | int' = TransportProtocol.undefined) -> 'tuple[AppType, ...]': """Every service IANA assigns to a port, canonical first. :meth:`get` answers with one member because that is what a port lookup @@ -2509,7 +2522,6 @@ def get_all(cls, key: 'int', *, Raises: ValueError: As :meth:`get`. - ProtocolError: As :meth:`get`. """ owner = cls._dispatch(key, proto) diff --git a/pcapkit/foundation/registry/protocols.py b/pcapkit/foundation/registry/protocols.py index 28c2474552..862582b04b 100644 --- a/pcapkit/foundation/registry/protocols.py +++ b/pcapkit/foundation/registry/protocols.py @@ -888,14 +888,21 @@ def register_apptype(code: 'int | Enum_AppType', module: 'str | ModuleDescriptor # ``code.proto`` default and the registries lookup below -- sees members # only. Resolution is by the member's own ``name``, case-insensitively -- # maintainer ruling on #815 -- via ``__members__`` directly rather than - # ``TransportProtocol[name]``: aenum's ``Flag.__getitem__`` parses a - # ``'|'``-joined name into a composite value on its own - # (``TransportProtocol['tcp|udp']`` silently returns the value ``3``), - # which is exactly the composite this function has to refuse, and - # lowercasing does not change that: ``'tcp|udp'`` is not a member name - # either. Anything that is neither a ``str`` nor a ``TransportProtocol`` - # member is rejected here too, rather than falling through to the - # registries lookup below: ``TransportProtocol`` is an ``IntFlag``, so + # ``TransportProtocol[name]``: this function's contract is + # :exc:`~pcapkit.utilities.exceptions.RegistryError` for anything + # unrecognised, composite-spelled or not, and ``__getitem__`` raises a + # bare :exc:`KeyError` on a miss instead of that -- both + # ``TransportProtocol['tcp|udp']`` and ``TransportProtocol['bogus']`` do, + # now that GitHub issue #808 dropped the ``IntFlag`` base that used to + # make the first of those two silently compose into the value ``3`` + # rather than miss at all. ``__members__.get(...)`` lets this function + # raise its own exception on a miss instead of letting ``__getitem__``'s + # propagate, and lowercasing does not turn ``'tcp|udp'`` into a member + # name either way. Anything that is neither a ``str`` nor a + # ``TransportProtocol`` member is rejected here too, rather than falling + # through to the registries lookup below: a bare ``int`` still hashes + # and compares equal to its matching member -- ``TransportProtocol`` + # being an ``IntEnum`` rather than an ``IntFlag`` changes neither -- so # ``registries.get(1)`` resolves to ``TCP`` just as # ``registries.get(TransportProtocol.tcp)`` does, and would otherwise # register the port before the ``proto.name`` access two lines below it diff --git a/pcapkit/vendor/reg/apptype/apptype.py b/pcapkit/vendor/reg/apptype/apptype.py index bba5e6fb20..63ec0b4206 100644 --- a/pcapkit/vendor/reg/apptype/apptype.py +++ b/pcapkit/vendor/reg/apptype/apptype.py @@ -104,10 +104,7 @@ """ from typing import TYPE_CHECKING, cast -from aenum import IntFlag, StrEnum, auto, extend_enum - -from pcapkit.utilities.compat import show_flag_values -from pcapkit.utilities.exceptions import ProtocolError +from aenum import IntEnum, StrEnum, extend_enum __all__ = ['TransportProtocol', '{NAME}'] @@ -117,31 +114,42 @@ from pcapkit.corekit.multidict import MultiDict -class TransportProtocol(IntFlag): +class TransportProtocol(IntEnum): """Transport layer protocol.""" - # mypy has no aenum plugin, so this class is a plain class to it: a bare - # ``0`` here infers as int while the auto()-valued members below infer as - # Any, and only this member then disagrees with the TransportProtocol - # annotations that use it. cast is the identity function at run time, so - # this changes nothing that runs -- see GitHub issue #770. mypy.ini sets - # warn_redundant_casts, so if aenum ever ships type stubs letting it infer - # TransportProtocol on its own, this cast starts erroring instead of - # lingering as dead scaffolding. + # mypy has no aenum plugin, so this class is a plain class to it: every + # member below is a literal int rather than an auto()-valued one -- GitHub + # issue #808 dropped the IntFlag base, so auto() would number sequentially + # instead of by the power-of-two spacing the values must keep -- and a + # literal infers as int while the TransportProtocol annotations that use + # each member (__transport__, and the proto default on __new__, get and + # get_all) expect TransportProtocol. cast is the identity function at run + # time, so this changes nothing that runs -- see GitHub issue #770, which + # cast only this member while the rest still inferred Any from auto(). + # mypy.ini sets warn_redundant_casts, so if aenum ever ships type stubs + # letting it infer TransportProtocol on its own, these casts start + # erroring instead of lingering as dead scaffolding. #: No transport protocol. ``TransportProtocol(0) is undefined`` and #: ``bool(undefined)`` is ``False``; it is the ``proto`` sentinel default #: for ``__transport__``, ``__new__``, ``get`` and ``get_all``, and what #: the base registry's ``_missing_`` extends unassigned/reserved rows from. undefined = cast('TransportProtocol', 0) - #: Transmission Control Protocol. - tcp = auto() - #: User Datagram Protocol. - udp = auto() - #: Stream Control Transmission Protocol. - sctp = auto() - #: Datagram Congestion Control Protocol. - dccp = auto() + #: Transmission Control Protocol. Value fixed at ``1`` rather than + #: renumbered sequentially -- GitHub issue #808 dropped the ``IntFlag`` + #: base once nothing built a composite, but did not revisit the four + #: values themselves, which predate this class and are not its call to + #: renumber. + tcp = cast('TransportProtocol', 1) + #: User Datagram Protocol. See ``tcp`` above for why the value stays ``2`` + #: rather than becoming sequential. + udp = cast('TransportProtocol', 2) + #: Stream Control Transmission Protocol. See ``tcp`` above for why the + #: value stays ``4`` rather than becoming sequential. + sctp = cast('TransportProtocol', 4) + #: Datagram Congestion Control Protocol. See ``tcp`` above for why the + #: value stays ``8`` rather than becoming sequential. + dccp = cast('TransportProtocol', 8) @staticmethod def get(key: 'int | str') -> 'TransportProtocol': @@ -156,35 +164,35 @@ def get(key: 'int | str') -> 'TransportProtocol': return TransportProtocol(key) if key.lower() in TransportProtocol.__members__: return TransportProtocol[key.lower()] # type: ignore[misc] - max_val = max(TransportProtocol.__members__.values()) - return extend_enum(TransportProtocol, key.lower(), max_val * 2) - - @classmethod - def _missing_(cls, value: 'int') -> 'TransportProtocol': - """Lookup function used when value is not found. - - Args: - value: Value to get enum item. - - Raises: - ValueError: If ``value`` sets a bit no member declares. - - Note: - This is what makes an unrecognised transport protocol *rejected* - rather than accepted -- GitHub issue #647's fix for this registry. - :mod:`aenum` on its own is permissive here and composes whatever bits - it is handed: measured on aenum 3.1.17 with this method removed, - ``TransportProtocol(-1)`` returns ``tcp|udp|sctp|dccp``, which is - #647's recorded defect for this class verbatim, and - ``TransportProtocol(16)`` returns a member whose ``name`` is - :obj:`None`. Declared bits still compose, since a service assigned to - several transport protocols is the ordinary case rather than the - exception. - - """ - if not (isinstance(value, int) and 0 <= value <= max(cls.__members__.values()) * 2 - 1): - raise ValueError(f'{{value!r}} is not a valid {{cls.__name__}}') - return super()._missing_(value) + # NOTE: maintainer ruling on this PR (#836): "Do not allow extension + # of TransportProtocol at all." A name that is not a declared member + # used to mint a brand-new one here, at ``max_val + 1`` (before that, + # ``max_val * 2``) -- an unbounded, ever-growing set of transport + # protocols nothing ever asked for. There is nothing left to walk + # now: it is simply refused, exactly like any other unrecognised + # name -- including one spelling a composite, e.g. ``'tcp|udp'``. + # ``'|'`` used to be intercepted here on its own, so a composite in + # disguise never got minted into a member whose own name lied about + # being a single transport; the owner's further ruling on this PR + # retired that special case along with the rest of the composite + # handling once TransportProtocol stopped being a Flag at all: + # "since it's no longer a Flag, `|` joined values are no longer + # parsed and accepted, we will treat it as a whole, instead of + # splitting." A ``'|'``-joined name is therefore not special any + # more -- it is simply not the name of a declared member, and gets + # the same message as any other one that is not. + raise ValueError(f'{{key!r}} is not a valid {{TransportProtocol.__name__}}') + + # NOTE: ``_missing_`` used to range-check ``value`` and then defer to + # :mod:`aenum`'s own ``Flag._missing_``, which is what composed an + # unrecognised bit combination into a pseudo-member -- ``TransportProtocol(3)`` + # returning ``tcp|udp`` -- GitHub issue #647's guard against that composing + # *anything*, including values no member declares. GitHub issue #808 removed + # the ``IntFlag`` base once nothing built a composite, and with it the only + # reason this method existed: a plain :class:`~aenum.IntEnum` already raises + # ``ValueError`` for a value no member declares, with no ``_missing_`` of + # its own needed to get there, so declaring one here would only be + # reproducing what the base class already does. class {NAME}(StrEnum): @@ -299,26 +307,30 @@ def __hash__(self) -> 'int': return hash(self.port) @classmethod - def _dispatch(cls, key: 'int', proto: 'TransportProtocol | str') -> 'Type[{NAME}]': + def _dispatch(cls, key: 'int', proto: 'TransportProtocol | str | int') -> 'Type[{NAME}]': """The registry that owns ``proto``, or ``cls`` where it is one already. Args: key: Port number the caller is looking up, validated here so that every entry point rejects a non-port identically. - proto: Transport protocol, as a flag or its name. Exactly one, on - the delegating path -- see :exc:`~pcapkit.utilities.exceptions.ProtocolError` - below. + proto: Transport protocol, as a member, its name, or a bare + :class:`int`. That last shape is not merely defensive: GitHub + issue #808 dropped ``TransportProtocol``'s ``IntFlag`` base, so + ``TransportProtocol.a | TransportProtocol.b`` -- built by hand, + the same as any caller passing a literal port-transport bitmask + -- falls through to ``int.__or__`` and returns a bare + :class:`int` rather than a member. Never split back into the + transports its bits would each name -- owner ruling on this PR + (#836) -- so it is refused as a whole exactly like any other + value naming no registry. Returns: The registry class to search. Raises: - ValueError: If ``key`` is not a port number, or if ``cls`` holds no - members and ``proto`` names no registry to delegate to. - ProtocolError: If ``cls`` holds no members and ``proto`` names more - than one transport protocol, which names more than one registry - and so no single answer. Itself a :exc:`ValueError`, so a caller - catching that keeps catching this. + ValueError: If ``key`` is not a port number, or if ``proto`` -- + member or bare int, composite or not -- names no registry to + delegate to. """ # NOTE: this registry resolves ports, not service names. The old string @@ -334,52 +346,52 @@ def _dispatch(cls, key: 'int', proto: 'TransportProtocol | str') -> 'Type[{NAME} if isinstance(proto, str): proto = TransportProtocol.get(proto.lower()) - # NOTE: ``TransportProtocol`` is an :class:`~aenum.IntFlag`, so ``tcp | - # udp`` stays constructible by hand even though no member carries one any - # more -- every member's ``proto`` is the single transport of the registry - # it lives in, GitHub issue #806. A composite names two registries, - # though, holding two different services for the same port -- and a lookup - # answers with one member, so there is no answer to which of them the - # composite meant. Resolving it picked the lowest set bit: - # :func:`~enum.show_flag_values` iterates LSB-first and ``tcp`` is the - # lowest, so every composite containing it dispatched into the TCP - # registry whatever else it named. Measured on fe80b8525, when members - # still carried the whole set, that answered 46 of the 10,625 multi-transport - # members' own ``get(m.port, proto=m.proto)`` with a service other than the - # member's own, of which 23 -- the UDP-declared half -- came back as a - # member of the *TCP* registry, carrying the wrong type and a narrower - # ``proto``. Two of those named a service UDP does not answer for the port - # at all, and they are the whole blast radius: ``AppType.get(888, proto=tcp - # | udp)`` gave ``cddbp`` for ````, and - # 999 gave ``garcon`` against UDP's ``applix``. The other 44 differ from - # ``m.svc`` only in that the member is not its port's canonical, which a - # single-bit lookup does too and which ``get`` documents. All of it silent, - # and undetectable to a caller checking equality, since ``__eq__`` compares - # on ``port`` alone. GitHub issue #759, whose body found 888 and - # generalised from it. - # - # Refusing the composite is the honest answer and costs - # nothing: a caller resolving a parsed port knows which transport carried - # it and passes that one bit, which is what every call site in - # :mod:`pcapkit` does, and one that wants every service on a port asks each - # registry in turn. This is the delegating path only -- a registry subclass - # returned above already knows its own transport and documents ``proto`` as - # ignored, so nothing there is ambiguous to begin with. - namespaces = show_flag_values(proto) - if len(namespaces) > 1: - raise ProtocolError(f'{{proto!r}} names {{len(namespaces)}} transport protocols, and so ' - f'{{len(namespaces)}} registries of {{cls.__name__}}; look the port up ' - 'under one transport protocol at a time') - if namespaces: - subclass = cls.__registries__.get(TransportProtocol(namespaces[0])) - if subclass is not None: - return subclass + + # NOTE: a direct dict lookup covers every genuine single transport -- + # one of the four real members, or a bare int equal to one of their + # values -- since ``__registries__`` keys compare by the same + # int-valued hash/eq every member and bare int alike already use. The + # cast is for mypy alone: ``dict.get`` accepts any hashable key at run + # time regardless of its declared key type, but mypy holds ``.get`` to + # the dict's own ``TransportProtocol`` keys, and does not know ``proto`` + # can genuinely be a bare :class:`int` here now that GitHub issue #808 + # dropped the ``IntFlag`` base -- see the annotation on ``proto`` above. + subclass = cls.__registries__.get(cast('TransportProtocol', proto)) + if subclass is not None: + return subclass + + # NOTE: everything that reaches here names no registry, and nothing + # below decodes ``proto``'s bits looking for a partial answer. A + # genuine member reaching this point is ``undefined`` -- the four + # real transports would already have resolved above, and + # :meth:`TransportProtocol.get` cannot mint anything else, per this + # PR's own maintainer ruling against extending TransportProtocol at + # all -- and a bare :class:`int` is refused exactly the same way + # whether it is a single stray bit, e.g. ``17``, or a composite of + # several real transports, e.g. ``3`` (``tcp | udp``). That composite + # case used to get its own + # :exc:`~pcapkit.utilities.exceptions.ProtocolError`, decoded through + # :func:`~pcapkit.utilities.compat.show_flag_values` and naming every + # transport whose bit was set -- the fix for GitHub issue #759, where + # resolving a composite by picking its lowest set bit dispatched + # every one containing ``tcp`` into the TCP registry regardless of + # what else it named. The owner's further ruling on this PR (#836) + # retired that decoding along with the rest of the composite + # handling: "since it's no longer a Flag, `|` joined values are no + # longer parsed and accepted, we will treat it as a whole, instead of + # splitting." So ``AppType.get(80, proto=3)`` now says "3 names no + # transport protocol registry" rather than naming ``tcp`` and ``udp`` + # individually -- the same answer a caller resolving a parsed port + # already gets right, since it knows which single transport carried + # it and passes that one bit, and the same answer a caller wanting + # every service on a port already has to ask each registry for in + # turn regardless. raise ValueError(f'{{proto!r}} names no transport protocol registry of ' f'{{cls.__name__}}') @classmethod def get(cls, key: 'int', *, - proto: 'TransportProtocol | str' = TransportProtocol.undefined) -> '{NAME}': + proto: 'TransportProtocol | str | int' = TransportProtocol.undefined) -> '{NAME}': """Backport support for original codes. Args: @@ -407,10 +419,11 @@ def get(cls, key: 'int', *, this registry resolves ports and not service names -- including one outside ``0..65535``, whose rejection by :meth:`_missing_` this method propagates rather than minting over, so that ``get`` is - never more permissive than ``{NAME}(...)``. - ProtocolError: If ``proto`` names more than one transport protocol -- - see :meth:`_dispatch`, which refuses it rather than answering from - whichever registry the lowest set bit happens to name. + never more permissive than ``{NAME}(...)``. Also covers a + ``proto`` naming more than one transport protocol -- a + composite built by hand is refused as a whole rather than + answered from any one of the registries it names -- see + :meth:`_dispatch`. :meta private: """ @@ -444,7 +457,7 @@ def get(cls, key: 'int', *, @classmethod def get_all(cls, key: 'int', *, - proto: 'TransportProtocol | str' = TransportProtocol.undefined) -> 'tuple[{NAME}, ...]': + proto: 'TransportProtocol | str | int' = TransportProtocol.undefined) -> 'tuple[{NAME}, ...]': """Every service IANA assigns to a port, canonical first. :meth:`get` answers with one member because that is what a port lookup @@ -464,7 +477,6 @@ def get_all(cls, key: 'int', *, Raises: ValueError: As :meth:`get`. - ProtocolError: As :meth:`get`. """ owner = cls._dispatch(key, proto) diff --git a/tests/const/test_const_apptype_split_unit.py b/tests/const/test_const_apptype_split_unit.py index 9c11076f2c..8808c4fa28 100644 --- a/tests/const/test_const_apptype_split_unit.py +++ b/tests/const/test_const_apptype_split_unit.py @@ -390,76 +390,50 @@ def test_every_transport_named_span_is_claimed_by_one_registry(self) -> None: # on that being true to stay correct. self.assertTrue(condition.endswith(claim), condition) - def test_a_proto_naming_two_transports_is_refused_rather_than_resolved(self) -> None: - """GitHub issue #759: the lowest set bit decided, silently. - - ``TransportProtocol`` is an :class:`~aenum.IntFlag`, so ``tcp | udp`` stays - constructible by hand even though no member carries one since GitHub issue - #806 -- which is why this guard is still needed and is tested with - composites built here rather than read off a member. It names two - registries holding two different services for the same port, and a lookup - answers with one member -- so there is no answer to which of them the - composite meant. ``_dispatch`` resolved it through - :func:`~pcapkit.utilities.compat.show_flag_values`, which iterates - **LSB-first**, so every composite containing ``tcp`` -- the lowest declared - bit -- dispatched into the TCP registry whatever else it named. - - The refusal is :exc:`~pcapkit.utilities.exceptions.ProtocolError` rather - than a bare :exc:`ValueError` because in-library errors come from - :mod:`pcapkit.utilities.exceptions`, and it is safe here in a way it is - **not** in ``_missing_``: - ``ConstEnumBuiltinParityTests.test_the_exception_is_not_an_in_library_one``, - in :mod:`tests.const.test_const_enum_builtin_parity`, requires the - constructor's guard to stay a non-:class:`BaseError` precisely because - ``get``'s ``except ValueError`` fallback catches and discards it, and a - ``BaseError`` would log at CRITICAL once per discarded default. Nothing - catches this one -- it propagates out of ``get``/``get_all`` to the caller - -- so a loud error is what it should be. It subclasses :exc:`ValueError` all - the same, so the documented contract of both entry points still holds. + def test_a_bare_int_composite_is_refused_as_a_whole(self) -> None: + """GitHub issue #759's old fix, superseded by the owner's #836 ruling. + + ``TransportProtocol`` dropped its :class:`~aenum.IntFlag` base in GitHub + issue #808, so ``tcp | udp`` no longer builds a member at all -- it falls + through to ``int.__or__`` and returns a bare :class:`int`. That bare int + stays constructible by hand even though no member carries one since + GitHub issue #806, which is why this guard is still needed and is tested + with composites built here rather than read off a member. + + ``_dispatch`` used to resolve a composite through + :func:`~pcapkit.utilities.compat.show_flag_values`, decoding its bits and + answering :exc:`~pcapkit.utilities.exceptions.ProtocolError` naming every + transport whose bit was set -- the fix for #759, where resolving one by + picking the lowest set bit (``tcp``, iterating **LSB-first**) dispatched + every composite containing it into the TCP registry whatever else it + named. The owner's ruling on this PR (#836) retires that decoding + instead of refining it: "since it's no longer a Flag, `|` joined values + are no longer parsed and accepted, we will treat it as a whole, instead + of splitting." A bare-int composite is therefore refused exactly like + any other value that names no registry -- a stray bit, ``undefined``, or + a number with nothing to do with any transport -- through the one plain + :exc:`ValueError` :meth:`AppType._dispatch` already gives those. There is + no longer a "too many transports" refusal distinct from a "no + transport" one. """ - import sys - from pcapkit.const.reg.apptype import DCCP, SCTP, TCP, UDP, AppType, TransportProtocol - from pcapkit.utilities.compat import show_flag_values - from pcapkit.utilities.exceptions import BaseError, ProtocolError - - # NOTE: a loud BaseError sets ``sys.tracebacklimit`` to 0 process-wide - # outside development mode, which would truncate the traceback of every - # later failure in this process. Restored the way - # ``QuietExceptionTests.setUp`` does it. - saved = getattr(sys, 'tracebacklimit', None) - - def restore() -> None: - if saved is None: - if hasattr(sys, 'tracebacklimit'): - del sys.tracebacklimit - else: - sys.tracebacklimit = saved - - self.addCleanup(restore) - - # The mechanism, pinned rather than described: ``tcp`` is the lowest bit - # and LSB-first iteration is why it used to win. - self.assertEqual(show_flag_values(TransportProtocol.tcp | TransportProtocol.udp), - [TransportProtocol.tcp, TransportProtocol.udp]) + from pcapkit.utilities.exceptions import ProtocolError # #759's own reproduction. 888 carries ``cddbp`` on TCP and - # ``accessbuilder`` on UDP, so the composite answered ``cddbp`` for a UDP - # member -- and ``==`` read ``True``, since it compares on ``port`` alone. - # The member's own ``proto`` was that composite until #806; it is ``udp`` - # now, so the composite is built here instead and the member's own value is - # asserted to be the single bit that no longer reaches this guard. + # ``accessbuilder`` on UDP. The member's own ``proto`` was that + # composite until #806; it is ``udp`` now, so the composite is built + # here instead and the member's own value is asserted to be the + # single bit that resolves cleanly. both = TransportProtocol.tcp | TransportProtocol.udp member = next(each for each in UDP.__registry__.getlist(888) if each.svc == 'accessbuilder') self.assertIs(member.proto, TransportProtocol.udp) self.assertIs(AppType.get(888, proto=member.proto), member) - with self.assertRaises(ProtocolError) as caught: + with self.assertRaises(ValueError) as caught: AppType.get(888, proto=both) - self.assertIsInstance(caught.exception, ValueError) - self.assertIsInstance(caught.exception, BaseError) - self.assertIn('tcp|udp', str(caught.exception)) - self.assertIn('2 transport protocols', str(caught.exception)) + self.assertNotIsInstance(caught.exception, ProtocolError) + self.assertIn(str(int(both)), str(caught.exception)) + self.assertIn('names no transport protocol registry', str(caught.exception)) # Every composite over the four declared bits, through all three entry # points -- ``get_all`` and ``_dispatch`` dispatch exactly as ``get`` does, @@ -476,22 +450,28 @@ def restore() -> None: before = {cls: len(cls) for cls in (TCP, UDP, SCTP, DCCP)} for proto in composites: - with self.subTest(proto=proto.name): - with self.assertRaises(ProtocolError): + # NOTE: not ``proto.name`` -- ``proto`` is a bare ``int`` since + # GitHub issue #808 dropped the ``IntFlag`` base composites used to + # be built from, and a bare int has no ``.name``. ``hex`` still + # tells the subTests apart on failure. + with self.subTest(proto=hex(proto)): + with self.assertRaises(ValueError) as via_get: AppType.get(80, proto=proto) - with self.assertRaises(ProtocolError): + self.assertNotIsInstance(via_get.exception, ProtocolError) + with self.assertRaises(ValueError) as via_get_all: AppType.get_all(80, proto=proto) - with self.assertRaises(ProtocolError): + self.assertNotIsInstance(via_get_all.exception, ProtocolError) + with self.assertRaises(ValueError) as via_dispatch: AppType._dispatch(80, proto) + self.assertNotIsInstance(via_dispatch.exception, ProtocolError) # And nothing was minted on the way out: #758's shape of defect was a # rejection that grew the registry anyway. self.assertEqual({cls: len(cls) for cls in (TCP, UDP, SCTP, DCCP)}, before) - # A single bit is untouched, and so is ``undefined``: zero bits names no - # registry rather than too many, which is a different refusal and stays the - # plain ``ValueError`` ``test_a_port_lookup_on_the_base_dispatches_by_transport`` - # asserts. + # A single bit resolves, and ``undefined`` is refused the identical way + # a composite now is -- there is no longer a distinct refusal for + # "too many transports" versus "no transport". self.assertIs(AppType.get(80, proto=TransportProtocol.tcp), TCP.get(80)) with self.assertRaises(ValueError) as plain: AppType.get(80, proto=TransportProtocol.undefined) @@ -1007,6 +987,187 @@ def test_every_member_renders_its_own_registrys_transport_protocol(self) -> None self.assertEqual(len(wrong), 0) self.assertEqual(total, 12391) + def test_transport_protocol_dropped_its_flag_base(self) -> None: + """GitHub issue #808: the base itself, once #806 removed every composite. + + Pins three things directly, so a regression that reintroduces the + ``IntFlag`` base or renumbers the four transports fails here rather + than only in the ``_dispatch`` refusal tests above: + + * ``TransportProtocol`` is a plain :class:`~aenum.IntEnum`, not a + :class:`~enum.Flag` of any kind -- ``&``, ``^`` and ``~`` are gone + along with ``|``'s member-composing behaviour. + * ``tcp | udp`` no longer builds a named member: it falls through to + ``int.__or__`` and returns a bare :class:`int`, so ``.name`` on the + result raises :class:`AttributeError` rather than answering + ``'tcp|udp'``. + * The five members keep the exact integer values they had as Flag + bits -- ``undefined`` 0, ``tcp`` 1, ``udp`` 2, ``sctp`` 4, ``dccp`` + 8 -- since GitHub issue #808 is about the base class, not about + renumbering members that predate it. + + Fails on stock ``ad4805f5f``: ``TransportProtocol`` there is still an + ``IntFlag``, so ``issubclass(TransportProtocol, enum.Flag)`` is + ``True``, and ``TransportProtocol.tcp | TransportProtocol.udp`` is a + genuine composite member whose ``.name`` answers ``'tcp|udp'`` rather + than raising :class:`AttributeError`. + + Also pins a change to plain iteration that is easy to miss because + every *member's* own ``repr``/``str``/``.name``/``.value`` stay + byte-identical: :class:`~enum.Flag` hides a zero-valued canonical + member from ``list(cls)``/``for m in cls``, so stock iterates + ``undefined`` out and yields the four real transports alone. A plain + :class:`~aenum.IntEnum` has no such convention and yields all five. + Nothing in :mod:`pcapkit` iterates ``TransportProtocol`` bare -- + ``apptype.py``'s own ``__init__.py`` populates ``__registries__`` + through ``__members__`` (5 either way, unaffected), never through + iteration -- so this is a correctness fact about the change worth + pinning, not a defect to fix. + """ + import enum + + from pcapkit.const.reg.apptype import TransportProtocol + + self.assertTrue(issubclass(TransportProtocol, int)) + self.assertFalse(issubclass(TransportProtocol, enum.Flag)) + + composite = TransportProtocol.tcp | TransportProtocol.udp + self.assertNotIsInstance(composite, TransportProtocol) + self.assertIsInstance(composite, int) + self.assertEqual(int(composite), 3) + with self.assertRaises(AttributeError): + composite.name # type: ignore[union-attr] + + self.assertEqual( + {member.name: int(member) for member in + (TransportProtocol.undefined, TransportProtocol.tcp, TransportProtocol.udp, + TransportProtocol.sctp, TransportProtocol.dccp)}, + {'undefined': 0, 'tcp': 1, 'udp': 2, 'sctp': 4, 'dccp': 8}) + + # 5 rather than 4: undefined is no longer hidden from iteration. Stock + # ad4805f5f gives 4 here (tcp, udp, sctp, dccp only), so this half of + # the test would also fail there, on a different assertion than the + # ones above. + self.assertEqual(len(list(TransportProtocol)), 5) + self.assertEqual(len(TransportProtocol.__members__), 5) + + def test_get_refuses_an_unrecognised_name_rather_than_minting_it(self) -> None: + """Maintainer ruling on this PR (#836), the ``TransportProtocol.get`` inline comment. + + ``TransportProtocol.get`` used to mint a brand-new member for any + name it did not recognise. This PR's own prior revision minted at + ``max_val + 1`` -- right after ``dccp``'s 8, so ``.get('bogus')`` + minted 9 -- rather than stock ``ad4805f5f``'s ``max_val * 2`` + doubling, which mints 16 for that same call; the difference between + the two schemes is explained below. The maintainer's ruling refuses + minting outright either way: "Do not allow extension of + TransportProtocol at all." There is no bound left to walk and + nothing left to mint, so the refusal is the one plain + :class:`ValueError` every unrecognised name gets, whether or not it + happens to spell a composite like ``'tcp|udp'``. A later round of + this PR briefly gave the composite case its own, more specific + message; the owner's ruling retired that split too -- "since it's + no longer a Flag, `|` joined values are no longer parsed and + accepted, we will treat it as a whole, instead of splitting" -- so + ``'|'`` is not treated specially any more, here or in + :meth:`AppType._dispatch` (see + ``test_a_bare_int_composite_is_refused_as_a_whole``). + + This also retires a sharper defect the old minting scheme created, + which is what this test used to be named for: 9 is exactly ``1 | 8``, + the bits ``tcp`` and ``dccp`` declare, so an intermediate round of + this same PR -- after minting had already moved to ``max_val + 1`` + but before this ruling -- decoded every ``proto`` reaching + :meth:`AppType._dispatch` through + :func:`~pcapkit.utilities.compat.show_flag_values` regardless of + whether it was a genuine member, and answered "tcp|dccp names 2 + transport protocols" for a name that named neither. That shape never + shipped in stock ``ad4805f5f``, whose ``max_val * 2`` doubling kept + every minted value a fresh single bit, and it cannot happen now + either, from the other direction: refusing the mint outright leaves + :meth:`AppType._dispatch` nothing minted to mis-decode in the first + place. + + Regression check: this fails against this PR's own prior head, + ``3567359e2``, which still mints ``9`` and returns it rather than + raising -- see the session report for the quoted failure. + """ + from pcapkit.utilities.exceptions import ProtocolError + + from pcapkit.const.reg.apptype import TransportProtocol + + self.assertNotIn('unit_test_836_bogus', TransportProtocol.__members__) + before = len(TransportProtocol.__members__) + + with self.assertRaises(ValueError) as caught: + TransportProtocol.get('unit_test_836_bogus') + # Plain ValueError -- not the ProtocolError the old + # show_flag_values-based decoding briefly answered with for a + # minted 9, back before this ruling retired minting entirely. + self.assertNotIsInstance(caught.exception, ProtocolError) + self.assertIn('unit_test_836_bogus', str(caught.exception)) + self.assertIn('is not a valid', str(caught.exception)) + + # Refused, not minted: the registry is exactly as it was, and a + # second distinct unrecognised name is refused the same way rather + # than taking the next integer after a member that was never created. + self.assertEqual(len(TransportProtocol.__members__), before) + self.assertNotIn('unit_test_836_bogus', TransportProtocol.__members__) + with self.assertRaises(ValueError): + TransportProtocol.get('unit_test_836_bogus_two') + self.assertEqual(len(TransportProtocol.__members__), before) + + def test_get_refuses_a_composite_spelled_string(self) -> None: + """A ``'|'``-joined name is just another unrecognised name. + + :meth:`TransportProtocol.get` used to mint whatever string it did not + recognise, including one spelling a composite -- ``'tcp|udp'`` -- as a + brand-new, single-bit member whose own name lies about being one + transport. That member's value would then satisfy the bare-int + composite branch :meth:`AppType._dispatch` used to have just as + readily as a genuinely OR-ed value, the mirror image of the + minted-member defect above -- and that branch is gone now too (see + ``test_a_bare_int_composite_is_refused_as_a_whole``). + :func:`~pcapkit.foundation.registry.protocols.register_apptype` + refuses the identical string the same generic way it refuses any + other unrecognised one, and the owner's ruling on this PR -- "since + it's no longer a Flag, `|` joined values are no longer parsed and + accepted, we will treat it as a whole, instead of splitting" -- + settles :meth:`TransportProtocol.get` onto that same answer: ``'|'`` + is not special, it is simply not the name of a declared member. A + review round of this PR briefly carved the composite case out with + its own diagnostic message; the owner's ruling retired that too. + """ + from pcapkit.const.reg.apptype import TransportProtocol + + self.assertNotIn('tcp|udp', TransportProtocol.__members__) + with self.assertRaises(ValueError) as caught: + TransportProtocol.get('tcp|udp') + self.assertIn('tcp|udp', str(caught.exception)) + self.assertIn('is not a valid', str(caught.exception)) + self.assertNotIn('tcp|udp', TransportProtocol.__members__) + + def test_a_bare_int_with_a_stray_bit_names_no_registry_either(self) -> None: + """A stray bit gets the exact same refusal a clean composite does. + + ``17`` is ``1 | 16`` -- ``tcp`` plus a bit no registry declares -- so + it never named a clean composite of real transports the way ``3`` + (``tcp | udp``) once did either, back when ``_dispatch`` still + decoded a bare int's bits at all. The owner's ruling on this PR + (#836) retired that decoding entirely (see + ``test_a_bare_int_composite_is_refused_as_a_whole``), so ``17`` and + ``3`` are no longer two different cases -- both are simply an + ``int`` that names no registry, and both get the identical plain + ``ValueError`` any other unmatched value gets. + """ + from pcapkit.const.reg.apptype import AppType + from pcapkit.utilities.exceptions import ProtocolError + + with self.assertRaises(ValueError) as caught: + AppType.get(80, proto=17) + self.assertNotIsInstance(caught.exception, ProtocolError) + self.assertIn('names no transport protocol registry', str(caught.exception)) + @staticmethod def _purge_member(cls: type, name: str, port: int) -> None: """Undo an :func:`~aenum.extend_enum` so the registry is left as found. diff --git a/tests/const/test_const_enum_builtin_parity.py b/tests/const/test_const_enum_builtin_parity.py index c245c061e2..74edb145d4 100644 --- a/tests/const/test_const_enum_builtin_parity.py +++ b/tests/const/test_const_enum_builtin_parity.py @@ -348,17 +348,20 @@ def test_the_sweep_size_is_pinned(self) -> None: if issubclass(obj, int) and not issubclass(obj, aenum.Flag)] # The decomposition is asserted, not just the total, so this sweep stays - # in step with the three narrower ones it overlaps: 111 non-flag IntEnum - # and 7 IntFlag in tests.const.test_const_enum_lookup, and 118 -- their - # sum -- in tests.const.test_const_enum_get. + # in step with the three narrower ones it overlaps: 112 non-flag IntEnum + # and 6 IntFlag in tests.const.test_const_enum_lookup, and 118 -- their + # sum, unchanged -- in tests.const.test_const_enum_get. GitHub issue #808 + # moved ``TransportProtocol`` from the flag count to the int count by + # dropping its ``IntFlag`` base, one for one, so the sum each of those + # counts on stays the same even though the two addends moved. # # Five of the nine string registries are the application layer one, which # GitHub issue #732 split into a package: the memberless # pcapkit.const.reg.apptype.apptype.AppType base plus one registry per # transport protocol. It is discovered exactly like a member-bearing # registry, since this sweep is structural and never looks at members. - self.assertEqual(len(ints), 111) - self.assertEqual(len(flags), 7) + self.assertEqual(len(ints), 112) + self.assertEqual(len(flags), 6) self.assertEqual(len(strs), 9) self.assertEqual(len(self.enums), 127) self.assertEqual(len({obj.__module__ for obj in self.enums}), 121) @@ -474,7 +477,18 @@ def test_every_new_guard_is_a_classmethod(self) -> None: # The six of issue #647 specifically, since the sweep above would still # pass if they had no ``_missing_`` at all -- which was the defect. + # + # ``TransportProtocol`` is the one deliberate exception: GitHub issue + # #808 dropped its ``IntFlag`` base once nothing built a composite, and + # with it the custom ``_missing_`` this guard checks for -- a plain + # ``IntEnum``'s own default ``_missing_`` already rejects everything + # undeclared, so declaring one here would only reproduce the base + # class. Its ``-1`` rejection is still checked, just not through this + # guard shape; see ``test_the_registries_named_in_issue_647`` above, + # which does not skip it. for module_name, class_name, _ in ISSUE_647_OUTLIERS: + if (module_name, class_name) == ('pcapkit.const.reg.apptype.apptype', 'TransportProtocol'): + continue obj = getattr(importlib.import_module(module_name), class_name) with self.subTest(enum=f'{module_name}.{class_name}'): self.assertIn('_missing_', vars(obj), @@ -541,19 +555,41 @@ def test_tcp_flag_composites_resolve(self) -> None: # being rejected: bits 0-3 of that field are the data offset. self.assertEqual(int(Flags(1)), 1) - def test_the_other_two_flag_registries_compose(self) -> None: + def test_the_other_flag_registry_composes(self) -> None: + """GitHub issue #808 dropped ``TransportProtocol`` out of this group. + + This used to test ``TransportProtocol`` here too, as the third + registry in the codebase sharing Flag semantics alongside ``Flags`` + (:class:`ConstFlagCompositeTests` above) and ``CommandType``. #808 + dropped ``TransportProtocol``'s ``IntFlag`` base once nothing built a + composite, so it no longer belongs to this group; its own -- now + negative -- assertions live in + :meth:`test_transport_protocol_no_longer_composes` below instead. + """ from pcapkit.const.ftp.command import CommandType - from pcapkit.const.reg.apptype import TransportProtocol self.assertEqual(int(CommandType(0)), 0) self.assertEqual(CommandType(0x07), CommandType.A | CommandType.P | CommandType.S) with self.assertRaises(ValueError): CommandType(0x08) + def test_transport_protocol_no_longer_composes(self) -> None: + """GitHub issue #808: dropping the ``IntFlag`` base removed composing. + + ``TransportProtocol(0x0F)`` used to equal the union of all four + declared bits, and ``TransportProtocol(0x10)`` -- one past the widest + legitimate combination -- was rejected by the range check + ``_missing_`` used to carry. Both mechanisms are gone now rather than + dormant: a plain :class:`~aenum.IntEnum` recognises only the five + declared values, so any other integer -- in range for the old bound + or not -- is rejected by the base class's own default ``_missing_``, + with no custom one declared here to widen it. + """ + from pcapkit.const.reg.apptype import TransportProtocol + self.assertEqual(int(TransportProtocol(0)), 0) - self.assertEqual(TransportProtocol(0x0F), - TransportProtocol.tcp | TransportProtocol.udp - | TransportProtocol.sctp | TransportProtocol.dccp) + with self.assertRaises(ValueError): + TransportProtocol(0x0F) with self.assertRaises(ValueError): TransportProtocol(0x10) @@ -674,32 +710,54 @@ def test_apptype_still_registers_an_unassigned_port(self) -> None: self.assertEqual(int(registered), 65000) self.assertIs(AppType.get(65000, proto=TransportProtocol.tcp), registered) - def test_transport_protocol_can_still_be_extended_at_runtime(self) -> None: - """Why this registry's bound is derived rather than written down. - - ``TransportProtocol.get`` registers an unknown protocol name at - ``max * 2``, so a literal upper bound -- the shape the Mobility Header - flag guards use -- would reject the very member the registry had just - grown, and every composite containing it. The guard reads the bound off - the current members instead, and this test is what pins that: it fails - against a hard-coded ``0x0F``. + def test_transport_protocol_can_no_longer_be_extended_at_runtime(self) -> None: + """Maintainer ruling on PR #836: extension refused, not renumbered. + + This used to pin the *shape* of ``TransportProtocol.get``'s + registration. GitHub issue #808 dropped the ``IntFlag`` base -- see + :meth:`ConstFlagCompositeTests.test_transport_protocol_no_longer_composes` + for why composing stopped mattering -- but left the doubling itself + alone: stock ``ad4805f5f`` still mints at ``max_val * 2``, so + ``.get('quic')`` there returns ``16``, right after ``dccp``'s ``8``. + This PR's own intermediate revision, not #808, is what switched an + unrecognised name to ``max_val + 1`` instead, minting ``9``. PR + #836's own inline comment on ``TransportProtocol.get`` removes the + registration entirely regardless of which scheme numbered it: "Do + not allow extension of TransportProtocol at all." Unlike + :class:`~pcapkit.const.ipv4.protection_authority.ProtectionAuthority` + and :class:`~pcapkit.const.mh.cga_type.CGAType` below, + ``TransportProtocol`` was never one of :data:`EXPECTED_TO_REGISTER` + -- it reached the "look up, miss, then register" shape through its + own hand-written ``get`` rather than through a generated + ``_missing_`` -- so pulling it back out of that shape is this + module's ruling changing which registries are mutable, not a + regression the sweep above would otherwise have to catch. + + Regression check: this fails against this PR's own prior head, + ``3567359e2``, which still registers ``'quic'`` at value 9 rather + than raising -- see the session report for the quoted failure. """ from pcapkit.const.reg.apptype import TransportProtocol self.assertNotIn('quic', TransportProtocol.__members__) with self.assertRaises(ValueError): - TransportProtocol(0x10) + TransportProtocol(9) - grown = TransportProtocol.get('quic') - self.assertEqual(int(grown), 0x10) - self.assertIn('quic', TransportProtocol.__members__) + before = len(TransportProtocol.__members__) + with self.assertRaises(ValueError): + TransportProtocol.get('quic') + self.assertNotIn('quic', TransportProtocol.__members__) + self.assertEqual(len(TransportProtocol.__members__), before) - # The new member composes with the old ones, which is the assertion a - # literal bound fails. - self.assertEqual(int(TransportProtocol(0x11)), 0x11) - self.assertEqual(TransportProtocol(0x11), grown | TransportProtocol.tcp) + # A second unrecognised name is refused identically -- there is no + # ``max + 1`` left to walk to, since nothing registers in the first + # place. + with self.assertRaises(ValueError): + TransportProtocol.get('quic2') + self.assertEqual(len(TransportProtocol.__members__), before) - # And the bound moved with it rather than disappearing. + # An int naming no declared member is still rejected exactly as + # before -- this half of the guard is untouched by the ruling. with self.assertRaises(ValueError): TransportProtocol(0x20) diff --git a/tests/const/test_const_enum_lookup.py b/tests/const/test_const_enum_lookup.py index e9ccd1954f..7c4c0ce09b 100644 --- a/tests/const/test_const_enum_lookup.py +++ b/tests/const/test_const_enum_lookup.py @@ -193,9 +193,11 @@ def test_every_registry_enum_was_discovered(self) -> None: # Pins the size of the sweep itself: if this drifts, a const enum was # added, removed, or renamed, and EXPECTED_TO_REJECT_ZERO (and the # analysis in GitHub issue #492) needs a fresh look rather than a - # silent pass. + # silent pass. 112 rather than 111 since GitHub issue #808: dropping + # ``TransportProtocol``'s ``IntFlag`` base moved it from + # ``_iter_const_int_flags`` into this sweep. names = {f'{obj.__module__}.{obj.__qualname__}' for obj in self.enums} - self.assertEqual(len(self.enums), 111) + self.assertEqual(len(self.enums), 112) self.assertTrue(EXPECTED_TO_REJECT_ZERO.issubset(names), f'expected reject-list entries missing from the sweep: ' f'{EXPECTED_TO_REJECT_ZERO - names}') @@ -304,9 +306,14 @@ def setUpClass(cls) -> None: cls.addClassCleanup(restore_modules, snapshot, ISOLATED_PREFIXES) def test_the_sweep_size_is_pinned(self) -> None: - """A flag enum added or removed needs a fresh look, not a silent pass.""" + """A flag enum added or removed needs a fresh look, not a silent pass. + + 6 rather than 7 since GitHub issue #808: dropping + ``TransportProtocol``'s ``IntFlag`` base moved it out of this sweep and + into ``_iter_const_int_enums`` instead. + """ names = {f'{obj.__module__}.{obj.__qualname__}' for obj in self.flags} - self.assertEqual(len(self.flags), 7) + self.assertEqual(len(self.flags), 6) expected = {f'pcapkit.const.mh.{stem}.{name}' for stem, name, _, _ in MH_FLAG_ENUMS} diff --git a/tests/dumpkit/test_nameless_enum_rendering_unit.py b/tests/dumpkit/test_nameless_enum_rendering_unit.py index b63bcbc73f..5c347edc40 100644 --- a/tests/dumpkit/test_nameless_enum_rendering_unit.py +++ b/tests/dumpkit/test_nameless_enum_rendering_unit.py @@ -104,9 +104,12 @@ class NamelessEnumRenderingTests(unittest.TestCase): sixteen-bit field :class:`~pcapkit.const.tcp.flags.Flags` bounds its :meth:`~enum.Enum._missing_` to. That guard is correct, so the probe was what had to move: a value the registry is right to refuse cannot also be a - value the dumper renders. Every one of the library's seven flag registries - carries such a guard, at four distinct widths, so exempting the guarded ones - instead would have left this half of the sweep with nothing in it at all. + value the dumper renders. Every one of the library's six flag registries + carries such a guard, at three distinct widths, so exempting the guarded + ones instead would have left this half of the sweep with nothing in it at + all. (GitHub issue #808 retyped the seventh, ``TransportProtocol``, out of + this sweep entirely -- see ``test_no_flag_registry_renders_the_literal_none``'s + docstring below.) """ @@ -241,10 +244,20 @@ def test_no_flag_registry_renders_the_literal_none(self) -> None: """Every flag enumeration in the library, swept rather than sampled. The sweep is the point: #648 was first reported against - :class:`pcapkit.const.tcp.flags.Flags` alone, and five of the seven flag + :class:`pcapkit.const.tcp.flags.Flags` alone, and five of the six flag registries turn out to be nameless at zero -- ``Flags`` plus the four Mobility Header flag registries. Naming them here would rot the moment - an eighth is added, so they are discovered. + a seventh is added, so they are discovered. + + GitHub issue #808 dropped this sweep from seven registries to six: + :class:`~pcapkit.const.reg.apptype.TransportProtocol` was a bounded + :class:`~aenum.IntFlag` before that issue and is a plain + :class:`~aenum.IntEnum` after, so it no longer matches the + ``issubclass(attribute, (enum.Flag, aenum.Flag))`` test + :func:`_flag_registries` sweeps on. It contributed nothing to + ``nameless`` either before or after -- it declared ``0`` as + ``undefined`` and had no undeclared bits left in its field, the same + shape as ``CommandType`` -- so only the registry *count* below moved. Since #702 the sweep is over every nameless value each registry admits rather than over ``registry(0)`` alone. Before that, the four Mobility @@ -260,7 +273,7 @@ def test_no_flag_registry_renders_the_literal_none(self) -> None: # A guard on the sweep itself: an empty mapping would make every # assertion below vacuous, and that is how this test would rot silently. - self.assertGreaterEqual(len(registries), 7, registries) + self.assertGreaterEqual(len(registries), 6, registries) nameless = [] for label, registry in sorted(registries.items()): @@ -268,11 +281,11 @@ def test_no_flag_registry_renders_the_literal_none(self) -> None: if values: nameless.append(label) - # ``0`` is rendered whether or not it is nameless. For the two - # registries that declare it -- ``CommandType`` and - # ``TransportProtocol``, the two with no undeclared bits left in - # their field -- it is the only thing there is to render, and it is - # the control showing the fix keys on the name and not on the value. + # ``0`` is rendered whether or not it is nameless. For the one + # registry that declares it -- ``CommandType``, the one with no + # undeclared bits left in its field -- it is the only thing there + # is to render, and it is the control showing the fix keys on the + # name and not on the value. for value in dict.fromkeys((0, *values)): with self.subTest(registry=label, value=value): member = registry(value) @@ -290,16 +303,18 @@ def test_no_flag_registry_renders_the_literal_none(self) -> None: # Not an incidental detail: if this ever drops to zero the test above # stops exercising the fix at all and would pass on unfixed code. + # Unchanged at 5 by GitHub issue #808 dropping the sweep from seven + # registries to six -- see the class docstring above: the registry it + # removed, TransportProtocol, was never one of the nameless five. self.assertGreaterEqual(len(nameless), 5, nameless) def test_a_value_past_the_field_is_refused_rather_than_rendered(self) -> None: """#702 -- the probe that was wrong, asserted the right way round. - All seven flag registries bound their :meth:`~enum.Enum._missing_` to + All six flag registries bound their :meth:`~enum.Enum._missing_` to the width of their own field, and the widths differ: three bits for - :class:`~pcapkit.const.ftp.command.CommandType`, four for - :class:`~pcapkit.const.reg.apptype.TransportProtocol`, eight for three - of the Mobility Header flags, sixteen for ``BindingUpdateFlag`` and + :class:`~pcapkit.const.ftp.command.CommandType`, eight for three of the + Mobility Header flags, sixteen for ``BindingUpdateFlag`` and :class:`~pcapkit.const.tcp.flags.Flags`. One literal therefore cannot mean "past the field" for all of them, which is the whole reason :func:`_field_mask` derives it per registry. @@ -309,15 +324,24 @@ def test_a_value_past_the_field_is_refused_rather_than_rendered(self) -> None: holds only if the mask :func:`_field_mask` computes is exactly the bound each registry wrote down for itself. + Until GitHub issue #808 there were seven registries and four distinct + widths here, the fourth being + :class:`~pcapkit.const.reg.apptype.TransportProtocol`'s four bits -- + derived as ``max(cls.__members__.values()) * 2 - 1`` since it extended + itself at runtime and could not hard-code a bound. #808 retyped it to a + plain :class:`~aenum.IntEnum`, dropping it out of :func:`_flag_registries`' + sweep entirely (see the class docstring above), which is why three + widths remain rather than four. + ``65536`` survives here, as the bound of the two sixteen-bit registries -- asserted as the rejection it always was, rather than as a value the dumper was expected to render. Which also keeps the ``raise`` in those - guards covered: six of the seven were never reached by any test, and the - seventh was reached only by this file failing on it. + guards covered: five of the six were never reached by any test, and the + sixth was reached only by this file failing on it. """ registries = _flag_registries() - self.assertGreaterEqual(len(registries), 7, registries) + self.assertGreaterEqual(len(registries), 6, registries) widths = set() for label, registry in sorted(registries.items()): @@ -340,8 +364,9 @@ def test_a_value_past_the_field_is_refused_rather_than_rendered(self) -> None: # The literal could not have been right for all of them, and this is the # measurement that says so: several distinct widths, and ``65536`` is - # outside every single one of them. - self.assertGreaterEqual(len(widths), 4, widths) + # outside every single one of them. 3 rather than 4 since GitHub issue + # #808 -- see the docstring above. + self.assertGreaterEqual(len(widths), 3, widths) def _const_path() -> 'list[str]': @@ -416,14 +441,21 @@ def _field_mask(registry: 'FlagRegistry') -> int: """The width of *registry*'s field, as an all-ones mask. Every flag registry in the library bounds its :meth:`~enum.Enum._missing_` - to the field its declared bits live in, and for all seven of them that bound + to the field its declared bits live in, and for all six of them that bound is exactly the smallest all-ones mask covering every declared bit -- ``0xFFFF`` for ``Flags``' ``1 << 4 .. 1 << 15``, ``0xFF`` for the eight-bit Mobility Header flags, ``0x07`` for the three-bit :class:`~pcapkit.const.ftp.command.CommandType`. - :class:`~pcapkit.const.reg.apptype.TransportProtocol` already spells it that - way in its own source, as ``max(cls.__members__.values()) * 2 - 1``, because - it extends itself at runtime and cannot hard-code a bound. + + A seventh registry used to belong here on the same terms: + :class:`~pcapkit.const.reg.apptype.TransportProtocol` spelled its own bound + as ``max(cls.__members__.values()) * 2 - 1`` because it extends itself at + runtime and could not hard-code one. GitHub issue #808 retyped it from + :class:`~aenum.IntFlag` to a plain :class:`~aenum.IntEnum` once nothing + built a composite, dropping it out of :func:`_flag_registries`'s sweep + entirely rather than merely changing its bound -- it no longer matches + ``issubclass(attribute, (enum.Flag, aenum.Flag))``, so this function never + sees it at all. Derived rather than read off the source, so it cannot drift from the guard and needs no table to maintain. That it does not drift is itself asserted, by @@ -461,9 +493,12 @@ def _nameless_values(registry: 'FlagRegistry') -> 'tuple[int, ...]': A registry whose declared bits fill its field has no nameless value at all and yields an empty tuple. :class:`~pcapkit.const.ftp.command.CommandType` - and :class:`~pcapkit.const.reg.apptype.TransportProtocol` are both of that - shape, each declaring ``0`` as ``undefined``, which is why five of the seven - registries are nameless at zero rather than all seven. + is of that shape, declaring ``0`` as ``undefined``, which is why five of + the six registries :func:`_flag_registries` discovers are nameless at zero + rather than all six. + :class:`~pcapkit.const.reg.apptype.TransportProtocol` used to be a second + example of the same shape before GitHub issue #808 retyped it out of the + sweep entirely -- see :func:`_field_mask`'s docstring. Args: registry: The flag enumeration to inspect. diff --git a/tests/vendor/test_vendor_reg_apptype_generator_unit.py b/tests/vendor/test_vendor_reg_apptype_generator_unit.py index e1b2d26378..90dc4ef857 100644 --- a/tests/vendor/test_vendor_reg_apptype_generator_unit.py +++ b/tests/vendor/test_vendor_reg_apptype_generator_unit.py @@ -262,23 +262,30 @@ def test_undefined_member_infers_as_transport_protocol_under_mypy(self) -> None: self.assertEqual(status, 0, msg=stdout + stderr) self.assertNotIn('[assignment]', stdout) - def test_transport_protocol_undefined_still_zero_and_composes(self) -> None: + def test_transport_protocol_undefined_still_zero(self) -> None: """GitHub issue #770: the ``cast`` changes nothing at run time. :func:`~typing.cast` is the identity function at run time, so ``TransportProtocol.undefined`` has to stay the same genuine ``TransportProtocol`` member with value ``0`` -- ``TransportProtocol(0) is undefined`` and ``bool(undefined)`` is ``False`` -- rather than - merely being *typed* as one. There is no flag composition naming - ``undefined`` in this module or its siblings to preserve; what the - value has to keep is its role as the ``proto`` sentinel default and - its neutrality under ``|``, checked directly below. + merely being *typed* as one. + + Before GitHub issue #808 dropped the ``IntFlag`` base, this also + asserted ``TransportProtocol.tcp | TransportProtocol.undefined is + TransportProtocol.tcp`` -- ``|``'s neutral element composing back to the + same singleton. That assertion is gone rather than adapted: ``|`` on a + plain :class:`~aenum.IntEnum` falls through to ``int.__or__`` and + returns a bare :class:`int`, never a ``TransportProtocol`` -- not even + one identical to an existing member -- so there is no longer a + singleton for ``is`` to find on *either* side of ``|``, and pinning + ``int(tcp | undefined) == int(tcp)`` would only be pinning integer + arithmetic. """ from pcapkit.const.reg.apptype.apptype import TransportProtocol self.assertIsInstance(TransportProtocol.undefined, TransportProtocol) self.assertEqual(int(TransportProtocol.undefined), 0) - self.assertIs(TransportProtocol.tcp | TransportProtocol.undefined, TransportProtocol.tcp) if __name__ == '__main__':