fix(corekit): make EnumRegistry.get's no-default marker an identity sentinel (#857)

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 ``<object object at 0x...>`` in a
+    signature, in :func:`help`, and in a traceback, where ``NoDefaultType()``
+    -- via :meth:`__repr__` below -- prints as ``<NO_DEFAULT>``.
+
+    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 ``<Name>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() <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)
+        (<function __newobj__ at 0x...>, (<class '...NoDefaultType'>,), 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)
+        (<function _reconstructor at 0x...>, (<class '...NoDefaultType'>, <class 'object'>, 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: <NO_DEFAULT> 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 '<NO_DEFAULT>'
+
 
-#: 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]
 

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``."""