From 899bbdb3b2946c3675fe466d29e3cc8779cb6649 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Sun, 27 Sep 2026 14:59:20 -0400 Subject: [PATCH] fix(corekit): make EnumRegistry.get's no-default marker an identity sentinel (#857) - Replace `NO_DEFAULT = -1` in `pcapkit/corekit/enum.py` with a dedicated `NoDefaultType` singleton, per the owner's ruling on #859 ("use dedicated class rather than bare object. Follow the house convention"). Change both `get()` comparisons from `==` to `is`. - `-1` compared with `==` let a caller's genuine `-1` default, and worse, a `-1.0` default (`-1.0 == -1` is `True`), collide with the "no default" marker silently. - `NoDefaultType` follows the settled half of the house convention (`Type` for the class, as `NullType`/`NoValueType` do) but keeps the existing instance name `NO_DEFAULT` -- the instance-naming half is unsettled per the owner, and continuity with `main` decided it here. `__new__` returns a cached singleton, guarding against a caller minting a second, non-identical sentinel; `__bool__` is deliberately omitted, since `NO_DEFAULT` is checked only by `is` and a falsy sentinel would invite the exact truthy/falsy conflation this fix removes. `NO_DEFAULT` and `NoDefaultType` are now both exported in `__all__`. - Copy/pickle hooks (`__copy__`/`__deepcopy__`/`__reduce__`) are omitted, correctly reasoned per round-2 cross-review: `__reduce_ex__` at protocol >= 2 already routes through the guarded `__new__` (a `copyreg.__newobj__` reduction), so `copy.copy`/`copy.deepcopy`/ pickle >= 2 already preserve identity with no extra code; only pickle protocol 0/1 bypasses it (`copyreg._reconstructor` -> `object.__new__` directly), and that path is unreachable because the sentinel is never stored in anything this package pickles. The reload caveat is stated precisely too: unlike `-1 == -1` (immune, since it compares by value), a reload leaves `get`'s already-bound parameter default holding the pre-reload singleton while the module global holds the fresh one, so an *omitted* `default` stops raising the original lookup error after a reload -- worse than the marker it replaces, though nothing in this package reloads this module. - No call site changes: `default` keeps its own default value, so no new import is needed anywhere. - Pin the new behaviour in `tests/const/test_const_registry_protocol.py`: `-1` and `-1.0` are genuine defaults now (raising for the attempted fallback rather than being read as "no default"), an omitted default is unaffected on both an IntEnum and an IntFlag registry, `NO_DEFAULT` compares unequal to `-1`, `-1.0`, `0`, `''`, `None` and `False`, its `repr` is readable rather than a bare address, and its identity is stable across the singleton guard and an ordinary re-import. - Correct `tests/const/test_const_enum_get.py`'s `test_the_placeholder_still_raises`, which asserted "explicitly passing [-1] is the same as omitting it" -- no longer true once -1 is an ordinary default, so it now pins the corrected, distinguishable behaviour instead of the stale claim. Scope: one file, `pcapkit/corekit/enum.py`, the sole consumer of `NO_DEFAULT`; 111 of 121 const registries inherit `get()` from it. No correctness bug today since no `pcapkit.const` registry's domain reaches -1; this is a clarity fix. Three of the ten bespoke registries that don't inherit this base (`ftp/return_code`, `http/status_code`, `pcapng/option_type`) carry their own, separate `-1`-as-default convention in a hand-written `get()` -- left alone, out of scope here. Build: `tests/const` (168), `tests/vendor` (86) and `tests/test_tier_guard.py` (107) all pass via plain `unittest`. mypy and pylint clean on `pcapkit/corekit/enum.py` (one pre-existing, unchanged mypy finding); isort clean. --- pcapkit/corekit/enum.py | 183 ++++++++++++++++++-- tests/const/test_const_enum_get.py | 28 ++- tests/const/test_const_registry_protocol.py | 145 ++++++++++++++++ 3 files changed, 336 insertions(+), 20 deletions(-) diff --git a/pcapkit/corekit/enum.py b/pcapkit/corekit/enum.py index b1c68ebacf..ad3e3fa37d 100644 --- a/pcapkit/corekit/enum.py +++ b/pcapkit/corekit/enum.py @@ -44,24 +44,179 @@ class will do its necessary overrides and dispatching logic; AppType subclasses from aenum import extend_enum +from pcapkit.utilities.compat import final + if TYPE_CHECKING: from typing import Any from typing_extensions import Self -__all__ = ['EnumRegistry'] +__all__ = ['NO_DEFAULT', 'NoDefaultType', 'EnumRegistry'] + + +@final +class NoDefaultType: + """Type of :data:`NO_DEFAULT`, the omitted-``default`` sentinel for :meth:`EnumRegistry.get`. + + A dedicated class rather than a bare :class:`object`, per the owner's ruling + on #859: *"use dedicated class rather than bare object. Follow the house + convention."* A bare :class:`object` compares under ``is`` exactly as + safely as a dedicated class with no ``__eq__`` of its own does -- identity + comparison was never the problem an earlier revision's docstring here + overstated it to be. What a bare :class:`object` actually lacks is a + readable representation: it prints as ```` in a + signature, in :func:`help`, and in a traceback, where ``NoDefaultType()`` + -- via :meth:`__repr__` below -- prints as ````. + + Named ``NoDefaultType`` for the *class* because that half of the house + convention is settled: both :class:`~pcapkit.corekit.module.NullType` and + :class:`~pcapkit.corekit.fields.field.NoValueType` use ``Type``. The + *instance*'s own name is not similarly settled -- the owner's follow-up + on #859 is explicit that ``NULL`` (``SCREAMING_CASE``) and ``NoValue`` + (``CapWords``) disagree, and "mainly depends on how we need it." The need + here is continuity: ``NO_DEFAULT`` is already the name on ``main`` -- + referenced in :meth:`EnumRegistry.get`'s signature, its docstring, and + both comparison sites -- and this change is to *what the sentinel is*, + not to *what it is called*, so it keeps that name rather than being + renamed to match either precedent's instance casing for its own sake. + ``NULL``'s ``SCREAMING_CASE`` is the closer match regardless, since + :data:`NO_DEFAULT` was already spelled that way. + + Genuinely a singleton, not merely a class this module happens to + instantiate once: :meth:`__new__` always hands back the one instance that + already exists, rather than building a new one. That guards against a + caller writing ``registry.get(key, default=NoDefaultType())`` -- perhaps + not realising :data:`NO_DEFAULT` already exists -- and getting back a + *second*, non-identical sentinel that silently fails ``is NO_DEFAULT`` + inside :meth:`EnumRegistry.get`, so their call is treated as supplying a + real (if useless) default instead of the *no default* they meant. With the + guard, :class:`NoDefaultType() ` always returns the one + canonical :data:`NO_DEFAULT`, so that mistake self-corrects. + + Unlike :class:`~pcapkit.corekit.module.NullType`, this does *not* also + define ``__copy__``, ``__deepcopy__`` or ``__reduce__`` -- but not because + :func:`copy.deepcopy` or :mod:`pickle` "bypass" :meth:`__new__`; they do + not, for protocol 2 and above. :meth:`object.__reduce_ex__` at protocol 2 + reduces through :func:`copyreg.__newobj__`, which reconstructs by calling + ``cls.__new__(cls)`` -- exactly the guarded path above -- so + ``copy.copy``, ``copy.deepcopy`` and every pickle protocol from 2 on + already come back as the one canonical instance with no extra code. + Measured:: + + >>> NO_DEFAULT.__reduce_ex__(2) + (, (,), None, None, None) + >>> copy.deepcopy(NO_DEFAULT) is NO_DEFAULT + True + + The one path :meth:`__new__` cannot see is pickle protocol 0 (and 1), + which reduces through :func:`copyreg._reconstructor` instead, and *that* + calls :func:`object.__new__` directly:: + + >>> NO_DEFAULT.__reduce_ex__(0) + (, (, , None)) + >>> pickle.loads(pickle.dumps(NO_DEFAULT, protocol=0)) is NO_DEFAULT + False + + That gap is exactly what :class:`~pcapkit.corekit.module.NullType`'s own + :meth:`~pcapkit.corekit.module.NullType.__reduce__` exists to close, because + :data:`~pcapkit.corekit.module.NULL` is stored as a + :class:`~pcapkit.corekit.module.ModuleDescriptor` field that a caller's own + :func:`copy.deepcopy` or :mod:`pickle` call can walk into and reconstruct. + :data:`NO_DEFAULT` is left unhandled here not because the gap cannot occur + in principle, but because nothing in this package ever pickles it at + protocol 0: it is reachable -- from :meth:`EnumRegistry.get`'s own bound + parameter default (``EnumRegistry.get.__defaults__[0]``, or any + subclass's, e.g. ``Hardware.get.__func__.__defaults__[0]``) and from + ``inspect.signature(Hardware.get).parameters['default'].default`` -- but + neither is a field any object here gets pickled *as*, and both still + survive :func:`copy.deepcopy` with identity intact regardless, because + deep-copying either still reduces the sentinel itself through the same, + guarded protocol-2 path measured above. + + A caveat, not a defect, but a real and *worse* one than the marker it + replaces: :func:`importlib.reload` on this module does not merely leave + the class stale, it breaks :meth:`EnumRegistry.get`'s own no-default + contract for every subclass whose ``get`` was already resolved before the + reload. Reload re-executes both ``class NoDefaultType:`` and + ``NO_DEFAULT = NoDefaultType()`` below, so the *module global* + :meth:`EnumRegistry.get`'s body compares against becomes a fresh, distinct + object. But each subclass's own ``get`` inherited its **bound parameter + default** -- ``default: 'Any' = NO_DEFAULT`` -- at function-definition + time, before the reload, and a bound default is frozen then, not looked + up again per call. So after the reload, calling ``get`` with ``default`` + *omitted* no longer compares the pre-reload default against ``NO_DEFAULT`` + truthfully: it is comparing the stale pre-reload sentinel against the + fresh post-reload one, ``is`` reads *False*, and ``get`` falls through to + attempting ``cls(default)`` on the stale sentinel instead of re-raising + the original lookup error. Measured:: + + >>> Hardware.get('Definitely-Not-A-Member') # before reload + KeyError: 'Definitely-Not-A-Member' + >>> importlib.reload(pcapkit.corekit.enum) + >>> Hardware.get('Definitely-Not-A-Member') # after reload + ValueError: is not a valid Hardware + + ``-1`` never had this failure mode: ``-1 == -1`` holds no matter which + module execution produced either side, since it compares by value, not by + identity, so a reload could not disturb it. Trading that immunity away is + the real cost of moving to an identity-compared sentinel, and it is not + hypothetical to this tree specifically: reload staleness is a *tracked* + defect class here, not a theoretical one -- see + :meth:`pcapkit.protocols.protocol.ProtocolBase._lookup_next_layer`'s own + docstring note citing GitHub issues #425, #428 and #560, and + :mod:`tests.protocols.test_dispatch_default_resolution_unit`'s own + ``test_no_stale_class_survives_a_module_reload``, which reloads a module + deliberately to pin the fix for exactly that class of bug elsewhere. So + "nothing in this package reloads :mod:`pcapkit.corekit.enum` after + import" is the only thing standing between this sentinel and that same + defect class -- true today, but a caveat to keep honest rather than a + guarantee this class enforces. + + Also unlike both :class:`~pcapkit.corekit.module.NullType` and + :class:`~pcapkit.corekit.fields.field.NoValueType`, this deliberately does + *not* define ``__bool__``. Both of those model an *absent* value, so + reading falsy in a boolean context is the point. :data:`NO_DEFAULT` models + something different: a marker meaning *no default was supplied*, checked + exclusively by ``is`` at :meth:`EnumRegistry.get`'s two comparison sites -- + nothing here ever evaluates it for truthiness. Giving it ``__bool__ -> + False`` for symmetry with the other two would invite exactly the + conflation this sentinel exists to rule out: code that writes ``if not + default:`` instead of ``if default is NO_DEFAULT:`` would then read + :data:`NO_DEFAULT` the same way it reads a caller's genuine falsy default + -- ``0``, ``''``, ``None`` or ``False`` -- which is the exact collision + ``-1`` used to cause under ``==`` and the reason #857 exists. Leaving + ``__bool__`` undefined makes ``NoDefaultType()`` truthy (the default for + any object defining neither ``__bool__`` nor ``__len__``), which at least + does not *look* like one of the falsy values it must never be mistaken + for. + + """ + + #: 'NoDefaultType | None': The one instance :meth:`__new__` ever returns, + #: including for the module-level ``NO_DEFAULT = NoDefaultType()`` below + #: that creates it in the first place. Kept on the class rather than as a + #: module global so :meth:`__new__` can read and write it without a + #: ``global`` statement. + _instance: 'NoDefaultType | None' = None + + def __new__(cls) -> 'NoDefaultType': + """Return the one instance of this class there will ever be.""" + if cls._instance is None: + cls._instance = super().__new__(cls) + return cls._instance + + def __repr__(self) -> 'str': + """Return :obj:`str` representation of the sentinel.""" + return '' + -#: The ``default`` argument value that means *no default*, i.e. let an -#: unresolvable key propagate its lookup error rather than falling back. -#: -#: ``-1`` rather than a sentinel object because that is what the 121 generated -#: registries already document and what their callers already pass, so the -#: migration onto this base class is not also a signature change. It is a safe -#: sentinel for both member types in play: no registry generated from an -#: upstream assignment carries a negative code, and ``-1`` is not a -#: :class:`str`, so a :class:`~aenum.StrEnum` registry can never mistake it for -#: one of its own values either. -NO_DEFAULT = -1 +#: NoDefaultType: The ``default`` argument value that means *no default*, i.e. +#: let an unresolvable key propagate its lookup error rather than falling +#: back. See :class:`NoDefaultType` for why this is a dedicated class rather +#: than a bare :class:`object`, why it is a guarded singleton, and why it +#: defines neither the copy/pickle hooks nor the ``__bool__`` that its two +#: :mod:`pcapkit.corekit` precedents do. +NO_DEFAULT = NoDefaultType() class EnumRegistry: @@ -137,13 +292,13 @@ def get(cls, key: 'Any', default: 'Any' = NO_DEFAULT) -> 'Self': try: return cls._member_map_[key] except KeyError: - if default == NO_DEFAULT: + if default is NO_DEFAULT: raise return cls(default) # type: ignore[call-arg] try: return cls(key) # type: ignore[call-arg] except ValueError: - if default == NO_DEFAULT: + if default is NO_DEFAULT: raise return cls(default) # type: ignore[call-arg] diff --git a/tests/const/test_const_enum_get.py b/tests/const/test_const_enum_get.py index 99db232b20..92a98a0462 100644 --- a/tests/const/test_const_enum_get.py +++ b/tests/const/test_const_enum_get.py @@ -186,19 +186,35 @@ def test_the_reported_case_returns_the_default(self) -> None: self.assertEqual(Hardware.get(40), Hardware(40)) self.assertIs(Hardware.get('Ethernet'), Hardware.Ethernet) - def test_the_placeholder_still_raises(self) -> None: - """``-1`` means *no default*, so the lookup error must still propagate.""" + def test_omitting_the_default_still_raises_for_the_original_key(self) -> None: + """GitHub issue #857: ``-1`` used to be compared with ``==`` against + :data:`~pcapkit.corekit.enum.NO_DEFAULT`, so a caller-supplied ``-1`` + was silently read as *no default was supplied* -- "explicitly passing + the placeholder is the same as omitting it", as this test used to + assert. :data:`~pcapkit.corekit.enum.NO_DEFAULT` is now an exported + instance of the dedicated :class:`~pcapkit.corekit.enum.NoDefaultType` + compared with ``is``, so ``-1`` is a genuine default like any other: + omitting ``default`` entirely still raises for the *original* key + (unchanged, pinned below), while explicitly passing ``-1`` now raises + for the *attempted fallback* ``cls(-1)`` instead -- no longer the same + error, since every registry's domain here starts at ``0`` and ``-1`` + is never a legitimate value. + """ from pcapkit.const.arp.hardware import Hardware from pcapkit.const.arp.operation import Operation for enum in (Hardware, Operation): with self.subTest(enum=_qualname(enum)): - with self.assertRaises(ValueError) as caught: + with self.assertRaises(ValueError) as omitted: enum.get(99999) - self.assertIn('99999', str(caught.exception)) - # Explicitly passing the placeholder is the same as omitting it. - with self.assertRaises(ValueError): + self.assertIn('99999', str(omitted.exception)) + + # No longer "the same as omitting it": this now names the + # failed fallback (``-1``), not the original key (``99999``). + with self.assertRaises(ValueError) as supplied: enum.get(99999, -1) + self.assertIn('-1', str(supplied.exception)) + self.assertNotIn('99999', str(supplied.exception)) def test_the_two_unverified_enums_from_the_issue(self) -> None: """#584 named ``Operation`` and ``LinkType`` but verified only ``Hardware``.""" diff --git a/tests/const/test_const_registry_protocol.py b/tests/const/test_const_registry_protocol.py index f02090d22b..e438a0e316 100644 --- a/tests/const/test_const_registry_protocol.py +++ b/tests/const/test_const_registry_protocol.py @@ -1051,5 +1051,150 @@ class _Str(EnumRegistry, StrEnum): _Str.get('known-value') +class NoDefaultSentinelTests(unittest.TestCase): + """GitHub issue #857: :data:`~pcapkit.corekit.enum.NO_DEFAULT` is now an + instance of the dedicated :class:`~pcapkit.corekit.enum.NoDefaultType` + compared with ``is``, rather than the magic value ``-1`` compared with + ``==``. Owner ruling on #859: a bare :class:`object` -- this batch's first + attempt -- is no less safe under ``is``, but a dedicated class matches the + house convention :class:`~pcapkit.corekit.module.NullType` and + :class:`~pcapkit.corekit.fields.field.NoValueType` already set, and gives + a readable :func:`repr` in a signature, in :func:`help`, and in a + traceback. + + Before #857, ``default == NO_DEFAULT`` read a caller-supplied ``-1`` -- + or, worse, ``-1.0``, since ``-1.0 == -1`` -- as if no default had been + supplied at all, so the caller's own fallback silently never took effect. + There is no live defect in :mod:`pcapkit.const` today (no registry's + domain reaches ``-1``), so this batch pins the *clarity* fix: ``-1`` and + ``-1.0`` are ordinary defaults now, indistinguishable from any other + value a caller might pass. + """ + + def setUp(self) -> None: + snapshot = snapshot_modules(ISOLATED_PREFIXES) + purge_modules(['pcapkit']) + self.addCleanup(restore_modules, snapshot, ISOLATED_PREFIXES) + + def test_no_default_is_not_equal_to_any_plausible_caller_value(self) -> None: + """The sentinel's whole point: unlike ``-1``, a + :class:`~pcapkit.corekit.enum.NoDefaultType` instance -- which defines + no ``__eq__`` of its own, so it inherits identity comparison from + :class:`object` -- cannot compare equal to anything a caller might + legitimately pass as ``default``, not even ``-1.0``, which the old + ``-1`` marker could not tell apart from itself.""" + from pcapkit.corekit.enum import NO_DEFAULT + + for value in (-1, -1.0, 0, '', None, False): + with self.subTest(value=value): + self.assertNotEqual(NO_DEFAULT, value) + self.assertIsNot(NO_DEFAULT, value) + + def test_repr_is_the_readable_form_not_a_bare_object_address(self) -> None: + """The concrete reason #859 asked for a dedicated class over a bare + ``object()``: the latter prints as ```` + wherever it turns up -- a signature, :func:`help`, a traceback -- + and this prints as ```` instead.""" + from pcapkit.corekit.enum import NO_DEFAULT + + rendered = repr(NO_DEFAULT) + self.assertEqual(rendered, '') + self.assertNotIn('0x', rendered) + + def test_type_is_the_dedicated_sentinel_class_and_both_are_exported(self) -> None: + import pcapkit.corekit.enum as enum_module + + self.assertIs(type(enum_module.NO_DEFAULT), enum_module.NoDefaultType) + self.assertIn('NO_DEFAULT', enum_module.__all__) + self.assertIn('NoDefaultType', enum_module.__all__) + + def test_constructing_the_type_again_returns_the_same_instance(self) -> None: + """The ``__new__`` singleton guard: a caller who does not realise + :data:`~pcapkit.corekit.enum.NO_DEFAULT` already exists and writes + ``NoDefaultType()`` themselves still gets back the one canonical + sentinel, rather than a second, non-identical object that would + silently fail ``is NO_DEFAULT`` inside + :meth:`~pcapkit.corekit.enum.EnumRegistry.get` and be treated as a + real (if useless) default instead of *no default*.""" + from pcapkit.corekit.enum import NO_DEFAULT, NoDefaultType + + self.assertIs(NoDefaultType(), NO_DEFAULT) + # And repeatedly -- not merely once by coincidence. + self.assertIs(NoDefaultType(), NoDefaultType()) + + def test_identity_survives_an_ordinary_second_import(self) -> None: + """An already-loaded module is cached in :data:`sys.modules`, so + importing it again -- by any of the usual spellings -- must not + construct a second sentinel. Only a genuine :func:`importlib.reload` + would do that (see :class:`~pcapkit.corekit.enum.NoDefaultType`'s own + docstring caveat, and :class:`~pcapkit.corekit.module.NullType`'s + before it), and nothing in this package reloads + :mod:`pcapkit.corekit.enum` after import.""" + import importlib + + from pcapkit.corekit.enum import NO_DEFAULT as first_import + + module_again = importlib.import_module('pcapkit.corekit.enum') + + self.assertIs(module_again.NO_DEFAULT, first_import) + + def test_missing_name_with_default_negative_one_is_now_a_real_default(self) -> None: + """Fails on the pre-#857 tree: ``ExtensionHeader.get(, -1)`` + raised :exc:`KeyError` there, because ``-1 == NO_DEFAULT`` read the + caller's ``-1`` as *no default* and re-raised the name lookup's own + error. On this head it raises :exc:`ValueError` instead, for the + *attempted fallback* ``ExtensionHeader(-1)`` -- ``-1`` is now a + genuine default, and every registry's domain starts at ``0``, so it + is itself unresolvable. + """ + from pcapkit.const.ipv6.extension_header import ExtensionHeader + + before = len(ExtensionHeader.__members__) + with self.assertRaises(ValueError) as caught: + ExtensionHeader.get('Definitely-Not-A-Member', -1) + self.assertIn('-1', str(caught.exception)) + self.assertNotIn('Definitely-Not-A-Member', str(caught.exception)) + self.assertEqual(before, len(ExtensionHeader.__members__)) + + def test_missing_name_with_default_negative_one_float_is_now_a_real_default(self) -> None: + """The float case that actually motivates #857: ``-1.0 == -1`` is + ``True``, so the old ``==`` comparison could not tell a caller's + ``-1.0`` apart from the ``-1`` marker either. Fails on the pre-#857 + tree with :exc:`KeyError` for the same reason as the ``-1`` case + above; raises :exc:`ValueError` here, for ``ExtensionHeader(-1.0)``. + """ + from pcapkit.const.ipv6.extension_header import ExtensionHeader + + before = len(ExtensionHeader.__members__) + with self.assertRaises(ValueError) as caught: + ExtensionHeader.get('Definitely-Not-A-Member', -1.0) + self.assertIn('-1.0', str(caught.exception)) + self.assertEqual(before, len(ExtensionHeader.__members__)) + + def test_missing_name_without_default_still_raises_on_an_int_enum(self) -> None: + """Omitting ``default`` entirely is unaffected by this change -- it + was, and remains, ``NO_DEFAULT`` by the parameter's own default + value, so identity trivially holds either way.""" + from pcapkit.const.ipv6.extension_header import ExtensionHeader + + before = len(ExtensionHeader.__members__) + with self.assertRaises(KeyError) as caught: + ExtensionHeader.get('Definitely-Not-A-Member') + self.assertIn('Definitely-Not-A-Member', str(caught.exception)) + self.assertEqual(before, len(ExtensionHeader.__members__)) + + def test_missing_name_without_default_still_raises_on_an_int_flag(self) -> None: + """As above, on an :class:`~aenum.IntFlag` registry -- the base + ``get()`` does not branch on member type before consulting + ``NO_DEFAULT``.""" + from pcapkit.const.mh.binding_update_flag import BindingUpdateFlag + + before = len(BindingUpdateFlag.__members__) + with self.assertRaises(KeyError) as caught: + BindingUpdateFlag.get('Definitely-Not-A-Member') + self.assertIn('Definitely-Not-A-Member', str(caught.exception)) + self.assertEqual(before, len(BindingUpdateFlag.__members__)) + + if __name__ == '__main__': unittest.main()