diff --git a/docs/source/contributing/conventions.rst b/docs/source/contributing/conventions.rst index 5a9736ed4..0f7634696 100644 --- a/docs/source/contributing/conventions.rst +++ b/docs/source/contributing/conventions.rst @@ -148,16 +148,25 @@ four in the tree follow it: - Defined in * - ``NULL`` - ``NullType`` - - :mod:`pcapkit.corekit.module` + - :mod:`pcapkit.corekit.sentinels` * - ``NoValue`` - ``NoValueType`` - - :mod:`pcapkit.corekit.fields.field` + - :mod:`pcapkit.corekit.sentinels` * - ``NO_DEFAULT`` - ``NoDefaultType`` - - :mod:`pcapkit.corekit.enum` + - :mod:`pcapkit.corekit.sentinels` * - ``_Absent`` - ``_AbsentType`` - - :mod:`pcapkit.protocols.protocol` + - :mod:`pcapkit.corekit.sentinels` + +All four used to live beside the one class that used them -- +:mod:`pcapkit.corekit.module`, :mod:`pcapkit.corekit.fields.field`, +:mod:`pcapkit.corekit.enum` and :mod:`pcapkit.protocols.protocol` respectively. +GitHub issue #911's housing ruling, verbatim -- *"Okay one module for all four it +is."* -- moved the four definitions into the single shared module the table now +names; each original module keeps a re-export so every existing +``from import `` keeps working, including the +``if TYPE_CHECKING:``-only imports of the types. Note what the rule does **not** fix: the **instance** name's casing is deliberately free, which is why ``NULL`` and ``NoValue`` disagree and both are correct. Pick @@ -166,16 +175,16 @@ renaming a published sentinel costs every caller for no gain. Nor does it fix the **leading underscore**. ``_Absent`` is private -- it is read in ``_declared_keywords`` and discarded there, never leaving -:mod:`pcapkit.protocols.protocol` -- and it is still held to the convention, which is -why its type is ``_AbsentType`` and not ``_Absent_t`` or ``Absent``. It is a -deliberate fourth rather than an accident, and its own docstring -(:file:`pcapkit/protocols/protocol.py`, line 95) says so: +:mod:`pcapkit.protocols.protocol` even though its *definition* now does -- and it is +still held to the convention, which is why its type is ``_AbsentType`` and not +``_Absent_t`` or ``Absent``. It is a deliberate fourth rather than an accident, and +its own docstring (:file:`pcapkit/corekit/sentinels.py`, line 430) says so: A distinct class rather than a bare :obj:`object` so that the sentinel has a name of its own in a traceback or a debugger, and so that a type checker has something to name where ``object()`` would give it nothing. It follows - :class:`~pcapkit.corekit.fields.field.NoValueType`, which does the same job for an - unset field default; this is a sibling of it rather than a reuse [...] + :class:`NoValueType`, which does the same job for an unset field default; + this is a sibling of it rather than a reuse [...] The private name is also why this table listed three for as long as it did: a sweep filtered on capitalised names does not see it. When adding a sentinel, add it here @@ -190,7 +199,7 @@ neither, which is what private means here. .. note:: - Of the four, only :class:`~pcapkit.corekit.module.NullType` is a full worked + Of the four, only :class:`~pcapkit.corekit.sentinels.NullType` is a full worked example. ``NoValueType`` follows the naming rule but is **not** a singleton (``NoValueType() is NoValue`` is :obj:`False`) and has no ``__repr__`` of its own, so it demonstrates the name and nothing else; ``_AbsentType`` has a ``__repr__`` @@ -239,9 +248,12 @@ inconsistencies**: fails every ``is`` check. Worth having wherever the type is reachable by a caller at all -- which, since the type is kept out of ``__all__``, means wherever it is importable by its dotted path rather than wherever it is star-exported. - :class:`~pcapkit.corekit.module.NullType` documents the limit honestly: a module + :class:`~pcapkit.corekit.sentinels.NullType` documents the limit honestly: a module **reload** re-executes the class statement, so the guard does not survive one, and - code holding the pre-reload instance will fail ``is``. + code holding the pre-reload instance will fail ``is``. Since GitHub issue #911, + that means reloading :mod:`pcapkit.corekit.sentinels` itself -- reloading + :mod:`pcapkit.corekit.module`, which now only re-exports the sentinel, no longer + has any effect on it. ``__bool__`` returning :obj:`False` ``NULL``, ``NoValue`` and ``_Absent`` have it, because each stands for an *absent @@ -253,7 +265,7 @@ inconsistencies**: prevent. ``__copy__`` / ``__deepcopy__`` / ``__reduce__`` - :class:`~pcapkit.corekit.module.NullType` has them because ``NULL`` is stored in a + :class:`~pcapkit.corekit.sentinels.NullType` has them because ``NULL`` is stored in a :class:`~pcapkit.corekit.module.ModuleDescriptor` field, so a caller's :func:`copy.deepcopy` or :mod:`pickle` can walk into it and would otherwise reconstruct a second instance. ``NO_DEFAULT`` and ``_Absent`` have none, because diff --git a/pcapkit/corekit/enum.py b/pcapkit/corekit/enum.py index 857654ca3..d7e513ce6 100644 --- a/pcapkit/corekit/enum.py +++ b/pcapkit/corekit/enum.py @@ -90,7 +90,7 @@ from aenum import extend_enum -from pcapkit.utilities.compat import final +from pcapkit.corekit.sentinels import NO_DEFAULT, NoDefaultType # pylint: disable=unused-import if TYPE_CHECKING: from typing import Any @@ -100,171 +100,6 @@ __all__ = ['NO_DEFAULT', 'EnumLookup', 'EnumRegistry'] -@final -class NoDefaultType: - """Type of :data:`NO_DEFAULT`, the omitted-``default`` sentinel for :meth:`EnumLookup.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:`EnumLookup.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:`EnumLookup.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:`EnumLookup.get`'s own bound - parameter default (``EnumLookup.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:`EnumLookup.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:`EnumLookup.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:`EnumLookup.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 '' - - -#: 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 EnumLookup: """Bare lookup protocol, shared by open registries and closed sets alike. diff --git a/pcapkit/corekit/fields/field.py b/pcapkit/corekit/fields/field.py index 4726535e7..d765e4956 100644 --- a/pcapkit/corekit/fields/field.py +++ b/pcapkit/corekit/fields/field.py @@ -8,7 +8,7 @@ import struct from typing import TYPE_CHECKING, Generic, TypeVar, cast -from pcapkit.utilities.compat import final +from pcapkit.corekit.sentinels import NoValue, NoValueType # pylint: disable=unused-import from pcapkit.utilities.exceptions import FieldValueError, NoDefaultValue, ProtocolError __all__ = ['NoValue', 'Field'] @@ -16,25 +16,12 @@ if TYPE_CHECKING: from typing import IO, Any, Callable, Iterator, Optional - from typing_extensions import Literal, Self + from typing_extensions import Self from pcapkit.protocols.schema.schema import Schema _T = TypeVar('_T') - -@final -class NoValueType: - """Default value for fields.""" - - def __bool__(self) -> 'Literal[False]': - """Return :obj:`False`.""" - return False - - -#: NoValueType: Default value for :attr:`FieldBase.default`. -NoValue = NoValueType() - #: int: Ceiling on the zero-padding :meth:`FieldBase.unpack` will still perform #: for a field whose declared length outruns its buffer. #: diff --git a/pcapkit/corekit/module.py b/pcapkit/corekit/module.py index 773fa1a13..24e9ed71d 100644 --- a/pcapkit/corekit/module.py +++ b/pcapkit/corekit/module.py @@ -14,157 +14,25 @@ import sys from typing import TYPE_CHECKING, Generic, TypeVar, cast -from pcapkit.utilities.compat import final +# NOTE: ``_get_null`` is re-exported alongside the two public names for +# backward compatibility, not for use here. Every pickle written before GitHub +# issue #911 moved the definitions names ``pcapkit.corekit.module _get_null`` +# in its payload -- that is what :meth:`NullType.__reduce__` emitted -- so +# dropping the name from this module would make those payloads unloadable with +# ``AttributeError: module 'pcapkit.corekit.module' has no attribute +# '_get_null'``. Newly written pickles name the new home; both resolve to the +# same function and yield the same singleton. +from pcapkit.corekit.sentinels import NULL, NullType, _get_null # pylint: disable=unused-import from pcapkit.utilities.exceptions import ProtocolError __all__ = ['NULL', 'ModuleDescriptor'] if TYPE_CHECKING: - from typing import Any, Callable, Type - - from typing_extensions import Literal + from typing import Type _T = TypeVar('_T') -@final -class NullType: - """Type of :data:`NULL`, the omitted-``class_``/``module`` sentinel. - - A distinct class rather than a plain :class:`str` -- which is what the - registry helpers in :mod:`pcapkit.foundation.registry.protocols` and - :mod:`pcapkit.foundation.registry.foundation` used to define, - independently of each other -- so that ``is`` comparisons against it mean - what they say: no :class:`str` a caller passes, including one that - happens to spell ``'(null)'`` itself, can compare equal to this sentinel - by identity. See GitHub issue #833. - - 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, so no caller -- - direct, or :mod:`copy`/:mod:`pickle` reconstructing an instance behind - the scenes -- can end up holding a second object that fails an ``is - NULL`` check downstream. :meth:`ModuleDescriptor.klass` makes exactly - that check, and a stricter guard that raises on a second call would be - truer to "singleton" in the abstract, but it would also mean the - module's own ``NULL = NullType()`` below is the only call that is ever - allowed to succeed -- fragile for no real benefit, since nothing here - needs *rejecting* a second construction, only preventing it from - producing a distinct object. - - That still leaves :func:`copy.deepcopy`, :func:`copy.copy` and - :mod:`pickle` unhandled: none of them constructs a new instance by - calling ``NullType()`` themselves, so the guard above never runs for - them. Each is therefore given its own override below, rather than left - to fall back to the default behaviour for a plain object: - - * :func:`copy.copy` and :func:`copy.deepcopy` check for - :meth:`__copy__`/:meth:`__deepcopy__` before ever falling back to - reduction, so :meth:`__deepcopy__` in particular has to be defined -- - its absence is the actual defect this class used to have: deepcopying - a :class:`ModuleDescriptor` recursed into this sentinel, reduced it, - and rebuilt a second, non-identical :class:`NullType` that then read as - an ordinary attribute name to :func:`getattr`, downgrading a clean - :exc:`~pcapkit.utilities.exceptions.ProtocolError` into a bare - :exc:`TypeError` (``attribute name must be string, not 'NullType'``). - * :mod:`pickle` protocols 2 and up reconstruct through - ``cls.__new__(cls)``, which the guarded :meth:`__new__` already keeps - to one instance -- but protocols 0 and 1 reconstruct through - :func:`copyreg._reconstructor`, which calls :func:`object.__new__` - *directly*, bypassing :meth:`__new__` entirely. :meth:`__reduce__` is - defined so that every protocol, not only the ones that happen to go - through this class's own :meth:`__new__`, is routed through the same - module-level getter instead of through reconstruction at all. - - A caveat rather than a defect: :func:`importlib.reload` on this module - re-executes ``NULL = NullType()`` below, producing a *second* singleton - that the reloaded code compares against correctly but that every module - which already imported the pre-reload :data:`NULL` still holds -- so a - comparison spanning the reload sees two "singletons" that are not each - other. :meth:`ModuleDescriptor.klass` faces exactly this class of - problem for the *class* it resolves, which is why it re-reads - :data:`sys.modules` on every call rather than memoising; nothing - equivalent is possible here, because unlike a resolved class there is no - live registry this sentinel could be re-read from. The pre-#833 ``str`` - sentinel had the same fragility for the same reason -- it is a property - of sharing one module-level binding across a reload, not something this - class's singleton guarantees claim to solve -- and nothing in this - package reloads :mod:`pcapkit.corekit.module` after import. - - """ - - #: 'NullType | None': The one instance :meth:`__new__` ever returns, - #: including for the module-level ``NULL = NullType()`` 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: 'NullType | None' = None - - def __new__(cls) -> 'NullType': - """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 __bool__(self) -> 'Literal[False]': - """Return :obj:`False`.""" - return False - - def __repr__(self) -> 'str': - """Return :obj:`str` representation of the sentinel.""" - return '' - - def __copy__(self) -> 'NullType': - """Return ``self`` -- there is, and only ever will be, one of these.""" - return self - - def __deepcopy__(self, memo: 'dict[int, Any]') -> 'NullType': - """Return ``self``, for the same reason as :meth:`__copy__`. - - Args: - memo: The :func:`copy.deepcopy` memo table. Unused: returning - ``self`` needs no entry, since nothing about this object is - ever copied. - - """ - return self - - def __reduce__(self) -> 'tuple[Callable[[], NullType], tuple[()]]': - """Reduce to the module-level singleton getter, for every :mod:`pickle` protocol. - - A class that defines :meth:`__reduce__` has it honoured by - :meth:`object.__reduce_ex__` for every protocol uniformly, rather - than only for the ones that would otherwise call - :func:`copyreg._reconstructor` -- so naming :func:`_get_null` here - sidesteps reconstruction, and therefore :meth:`__new__`, altogether. - That makes this correct independent of whatever :meth:`__new__` does, - which is what actually covers protocols 0 and 1; see the class - docstring. - - """ - return (_get_null, ()) - - -#: NullType: Sentinel for an omitted ``class_`` argument to the ``register_*`` -#: helpers in :mod:`pcapkit.foundation.registry.protocols` and -#: :mod:`pcapkit.foundation.registry.foundation`. Defined once, here, rather -#: than once per module: both already import :class:`ModuleDescriptor` from -#: this module, so it is the shared home that needs no new module and creates -#: no import cycle. -NULL = NullType() - - -def _get_null() -> 'NullType': - """Return :data:`NULL`, for :meth:`NullType.__reduce__`. - - A module-level function rather than a lambda or a bound method, so every - :mod:`pickle` protocol -- including 0 and 1, which cannot reference - anything nested inside a class -- can name it. - - """ - return NULL - - class ModuleDescriptor(collections.namedtuple('ModuleDescriptor', ['module', 'name']), Generic[_T]): """Module descriptor contains module name and class name, the actual class can be imported by ``from module import name``.""" diff --git a/pcapkit/corekit/sentinels.py b/pcapkit/corekit/sentinels.py new file mode 100644 index 000000000..aa94c16b9 --- /dev/null +++ b/pcapkit/corekit/sentinels.py @@ -0,0 +1,469 @@ +# -*- coding: utf-8 -*- +"""Sentinel Objects +===================== + +.. module:: pcapkit.corekit.sentinels + +:mod:`pcapkit.corekit.sentinels` is the single, shared home for every +module-level singleton sentinel this package defines for itself -- a value +whose only job is to be recognised by identity (``value is SENTINEL``), so +that it can never be confused with a value a caller might legitimately pass. +See the "Naming a sentinel" section of +:file:`docs/source/contributing/conventions.rst` for the house rule the four +below follow. + +Before this module existed, each of the four lived beside the one class that +used it: :class:`NullType` in :mod:`pcapkit.corekit.module`, +:class:`NoValueType` in :mod:`pcapkit.corekit.fields.field`, +:class:`NoDefaultType` in :mod:`pcapkit.corekit.enum` and +:class:`_AbsentType` in :mod:`pcapkit.protocols.protocol`. The owner's ruling +on GitHub issue #911, verbatim -- *"Okay one module for all four it is."* -- +moves the four *definitions* here; each original module keeps a three-line +re-export so that no existing ``from import `` breaks, +including the ``if TYPE_CHECKING:``-only imports of the *types* that +:mod:`pcapkit.foundation.registry.foundation`, +:mod:`pcapkit.foundation.registry.protocols` and +:mod:`pcapkit.corekit.fields.ipaddress`, :mod:`~pcapkit.corekit.fields.misc`, +:mod:`~pcapkit.corekit.fields.numbers` and :mod:`~pcapkit.corekit.fields.strings` +already carry. + +``NoValue`` and ``_Absent`` differ in how far the ruling reaches. ``NoValue`` +is documented as the value of +:attr:`FieldBase.default `, +so it is a published contract and the re-export at +:mod:`pcapkit.corekit.fields.field` is load-bearing for callers outside this +package. ``_Absent`` is private to :mod:`pcapkit.protocols.protocol` -- +nothing outside that module ever imports it, from here or from there -- so +its re-export exists only so that module's own code keeps reading +``_Absent`` rather than a fully-qualified name; see :class:`_AbsentType`'s +own docstring below for why it stays private after the move. + +""" +from typing import TYPE_CHECKING + +from pcapkit.utilities.compat import final + +__all__ = ['NULL', 'NoValue', 'NO_DEFAULT'] + +if TYPE_CHECKING: + from typing import Any, Callable + + from typing_extensions import Literal + + +@final +class NullType: + """Type of :data:`NULL`, the omitted-``class_``/``module`` sentinel. + + A distinct class rather than a plain :class:`str` -- which is what the + registry helpers in :mod:`pcapkit.foundation.registry.protocols` and + :mod:`pcapkit.foundation.registry.foundation` used to define, + independently of each other -- so that ``is`` comparisons against it mean + what they say: no :class:`str` a caller passes, including one that + happens to spell ``'(null)'`` itself, can compare equal to this sentinel + by identity. See GitHub issue #833. + + 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, so no caller -- + direct, or :mod:`copy`/:mod:`pickle` reconstructing an instance behind + the scenes -- can end up holding a second object that fails an ``is + NULL`` check downstream. :meth:`ModuleDescriptor.klass + ` makes exactly that + check, and a stricter guard that raises on a second call would be truer + to "singleton" in the abstract, but it would also mean the module's own + ``NULL = NullType()`` below is the only call that is ever allowed to + succeed -- fragile for no real benefit, since nothing here needs + *rejecting* a second construction, only preventing it from producing a + distinct object. + + That still leaves :func:`copy.deepcopy`, :func:`copy.copy` and + :mod:`pickle` unhandled: none of them constructs a new instance by + calling ``NullType()`` themselves, so the guard above never runs for + them. Each is therefore given its own override below, rather than left + to fall back to the default behaviour for a plain object: + + * :func:`copy.copy` and :func:`copy.deepcopy` check for + :meth:`__copy__`/:meth:`__deepcopy__` before ever falling back to + reduction, so :meth:`__deepcopy__` in particular has to be defined -- + its absence is the actual defect this class used to have: deepcopying + a :class:`~pcapkit.corekit.module.ModuleDescriptor` recursed into this + sentinel, reduced it, and rebuilt a second, non-identical + :class:`NullType` that then read as an ordinary attribute name to + :func:`getattr`, downgrading a clean + :exc:`~pcapkit.utilities.exceptions.ProtocolError` into a bare + :exc:`TypeError` (``attribute name must be string, not 'NullType'``). + * :mod:`pickle` protocols 2 and up reconstruct through + ``cls.__new__(cls)``, which the guarded :meth:`__new__` already keeps + to one instance -- but protocols 0 and 1 reconstruct through + :func:`copyreg._reconstructor`, which calls :func:`object.__new__` + *directly*, bypassing :meth:`__new__` entirely. :meth:`__reduce__` is + defined so that every protocol, not only the ones that happen to go + through this class's own :meth:`__new__`, is routed through the same + module-level getter instead of through reconstruction at all. + + A caveat rather than a defect: :func:`importlib.reload` on this module + re-executes ``NULL = NullType()`` below, producing a *second* singleton + that the reloaded code compares against correctly but that every module + which already imported the pre-reload :data:`NULL` still holds -- so a + comparison spanning the reload sees two "singletons" that are not each + other. :meth:`ModuleDescriptor.klass + ` faces exactly this class + of problem for the *class* it resolves, which is why it re-reads + :data:`sys.modules` on every call rather than memoising; nothing + equivalent is possible here, because unlike a resolved class there is no + live registry this sentinel could be re-read from. The pre-#833 ``str`` + sentinel had the same fragility for the same reason -- it is a property + of sharing one module-level binding across a reload, not something this + class's singleton guarantees claim to solve -- and nothing in this + package reloads :mod:`pcapkit.corekit.sentinels` after import. + + A second caveat, specific to this class now living apart from its one + caller-visible re-export: reloading :mod:`pcapkit.corekit.module` + itself -- rather than this module -- no longer has any effect on the + singleton at all. That module now only re-imports :data:`NULL` and + :class:`NullType` from here, and re-running an already-satisfied + ``from ... import`` reads the current binding in :mod:`sys.modules` + rather than re-executing anything, so it neither mints a new instance + nor loses the old one. The reload hazard this docstring describes moved + with the class definition; it did not double. + + """ + + #: 'NullType | None': The one instance :meth:`__new__` ever returns, + #: including for the module-level ``NULL = NullType()`` 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: 'NullType | None' = None + + def __new__(cls) -> 'NullType': + """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 __bool__(self) -> 'Literal[False]': + """Return :obj:`False`.""" + return False + + def __repr__(self) -> 'str': + """Return :obj:`str` representation of the sentinel.""" + return '' + + def __copy__(self) -> 'NullType': + """Return ``self`` -- there is, and only ever will be, one of these.""" + return self + + def __deepcopy__(self, memo: 'dict[int, Any]') -> 'NullType': + """Return ``self``, for the same reason as :meth:`__copy__`. + + Args: + memo: The :func:`copy.deepcopy` memo table. Unused: returning + ``self`` needs no entry, since nothing about this object is + ever copied. + + """ + return self + + def __reduce__(self) -> 'tuple[Callable[[], NullType], tuple[()]]': + """Reduce to the module-level singleton getter, for every :mod:`pickle` protocol. + + A class that defines :meth:`__reduce__` has it honoured by + :meth:`object.__reduce_ex__` for every protocol uniformly, rather + than only for the ones that would otherwise call + :func:`copyreg._reconstructor` -- so naming :func:`_get_null` here + sidesteps reconstruction, and therefore :meth:`__new__`, altogether. + That makes this correct independent of whatever :meth:`__new__` does, + which is what actually covers protocols 0 and 1; see the class + docstring. + + """ + return (_get_null, ()) + + +#: NullType: Sentinel for an omitted ``class_`` argument to the ``register_*`` +#: helpers in :mod:`pcapkit.foundation.registry.protocols` and +#: :mod:`pcapkit.foundation.registry.foundation`. Housed here, alongside the +#: package's other sentinels, rather than in :mod:`pcapkit.corekit.module` +#: where it used to live -- per the owner's ruling on GitHub issue #911, see +#: the module docstring above. :mod:`pcapkit.corekit.module` keeps a +#: re-export so every existing ``from pcapkit.corekit.module import NULL`` +#: keeps working. +NULL = NullType() + + +def _get_null() -> 'NullType': + """Return :data:`NULL`, for :meth:`NullType.__reduce__`. + + A module-level function rather than a lambda or a bound method, so every + :mod:`pickle` protocol -- including 0 and 1, which cannot reference + anything nested inside a class -- can name it. + + """ + return NULL + + +@final +class NoValueType: + """Type of :data:`NoValue`, the default value for :mod:`pcapkit.corekit.fields`. + + Housed here per GitHub issue #911 rather than in + :mod:`pcapkit.corekit.fields.field`, where it used to be defined and where + :attr:`FieldBase.default ` + still documents it as the field-default sentinel. + + """ + + def __bool__(self) -> 'Literal[False]': + """Return :obj:`False`.""" + return False + + +#: NoValueType: Default value for +#: :attr:`FieldBase.default `. +#: :mod:`pcapkit.corekit.fields.field` keeps a re-export, since that +#: attribute's own documentation is a published contract naming this object. +NoValue = NoValueType() + + +@final +class NoDefaultType: + """Type of :data:`NO_DEFAULT`, the omitted-``default`` sentinel for + :meth:`EnumLookup.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:`NullType` and :class:`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:`EnumLookup.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:`EnumLookup.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:`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:`NullType`'s own + :meth:`~NullType.__reduce__` exists to close, because :data:`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:`EnumLookup.get + `'s own bound parameter default + (``EnumLookup.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 that turns out to be dormant rather than live, on the current + tree -- worth stating precisely rather than either repeating the older, + inaccurate claim or dropping the topic. :func:`importlib.reload` on this + module re-executes both ``class NoDefaultType:`` and ``NO_DEFAULT = + NoDefaultType()`` below, producing a fresh, distinct object; any consumer + that had already captured the pre-reload one -- as a bound parameter + default, say -- goes on holding the stale one, and a bare + ``held_default is NO_DEFAULT`` comparison against the post-reload global + then reads :data:`False` where it once read :data:`True`. + :meth:`EnumLookup.get ` looked + exactly like such a consumer before GitHub issue #864: an unrecognised, + non-``NO_DEFAULT`` value used to fall through to ``cls(default)``, so a + stale sentinel handed to that call could raise a :exc:`ValueError` a + caller had no reason to expect from an *omitted* argument. #864 closed a + different hole -- ``default`` could mint a new member -- by replacing + that call with a ``default not in cls._value2member_map_`` guard, and the + guard happens to close this one too: a :class:`NoDefaultType` instance, + stale or fresh, is never a registered enum value, so the guard's ``not + in`` half reads :data:`True` for it either way and ``get`` re-raises the + original lookup error correctly regardless of which :data:`NO_DEFAULT` + a caller's stale default is stale *against*. Measured on the current + tree, guard included:: + + >>> Hardware.get('Definitely-Not-A-Member') # before reload + KeyError: 'Definitely-Not-A-Member' + >>> importlib.reload(pcapkit.corekit.sentinels) + >>> Hardware.get('Definitely-Not-A-Member') # after reload + KeyError: 'Definitely-Not-A-Member' + + So the docstring this class carried before GitHub issue #911's move -- + which claimed the second call above raises :exc:`ValueError` -- was + already wrong on ``main`` at ``d31c0aaf6``, independently of the move: + it described the pre-#864 ``cls(default)`` call, and nobody had + re-verified it against the guard #864 added afterwards. Fixed here as a + drive-by correction, not a consequence of the housing change itself. + + None of that makes the underlying hazard theoretical elsewhere in this + package: ``-1`` never had this failure mode at all, since ``-1 == -1`` + compares by value rather than identity, and reload staleness is a + *tracked* defect class here for other constructs -- 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. A + *future* comparison site written the vulnerable way -- a bare ``is + NO_DEFAULT`` with no independent guard behind it, the way #864's fix + itself was not -- would still reproduce it. "Nothing in this package + reloads :mod:`pcapkit.corekit.sentinels` after import" remains true + today, but it is a caveat to keep honest rather than a guarantee this + class enforces. + + .. note:: + + GitHub issue #911 also changes *which* reload is the one that + matters, independently of the #864 finding above. + :mod:`pcapkit.corekit.enum` no longer defines ``NoDefaultType`` + itself; it only reads :data:`NO_DEFAULT` off this module once, at its + own import time, into its own module global. Reloading + :mod:`pcapkit.corekit.enum` alone therefore now just re-runs that + read, which -- so long as this module has not *also* been reloaded -- + fetches back the identical object and changes nothing. Producing a + fresh :data:`NO_DEFAULT` at all now takes reloading *this* module, + where the class statement lives; reloading only + :mod:`pcapkit.corekit.enum` is not enough on its own, because ``from + ... import`` binds a copy rather than a live alias, and that module's + own global stays pointed at whatever it read until something re-runs + that import. + + Also unlike both :class:`NullType` and :class:`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:`EnumLookup.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 '' + + +#: 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 +#: :mod:`pcapkit.corekit` siblings do. :mod:`pcapkit.corekit.enum` keeps a +#: re-export, since :meth:`EnumLookup.get ` +#: names this object in its signature and docstring. +NO_DEFAULT = NoDefaultType() + + +@final +class _AbsentType: + """Type of :data:`_Absent`, the absent-key sentinel. + + A distinct class rather than a bare :obj:`object` so that the sentinel has a + name of its own in a traceback or a debugger, and so that a type checker has + something to name where ``object()`` would give it nothing. It + follows :class:`NoValueType`, which does the same job for an unset field + default; this is a sibling of it rather than a reuse, since that one is + documented as the default value of + :attr:`FieldBase.default ` + and means "no value was given", not "this key is not here". + + Defined here, alongside the package's other sentinels, per the owner's + ruling on GitHub issue #911 -- but it stays exactly as private as it was + in :mod:`pcapkit.protocols.protocol`: nothing outside that module reads + :data:`_Absent`, from here or from there, and this module's own + :attr:`__all__` does not name it. The move relocates the *definition*, + not the visibility. + + """ + + def __bool__(self) -> 'Literal[False]': + """Return :obj:`False`.""" + return False + + def __repr__(self) -> 'str': + """Return :obj:`str` representation of the sentinel.""" + return '' + + +#: _AbsentType: Absent-versus-:obj:`None` sentinel for +#: :func:`pcapkit.protocols.protocol._declared_keywords` to read a class's +#: own ``__keywords__`` out of its :attr:`~object.__dict__`, where +#: :obj:`None` is itself a meaningful value -- the opt-out that says the +#: class cannot enumerate its keywords, c.f. +#: :attr:`ProtocolBase.__keywords__ +#: `. Never leaves +#: :mod:`pcapkit.protocols.protocol`, which keeps a private re-export of it +#: for exactly that one read. +_Absent = _AbsentType() diff --git a/pcapkit/protocols/protocol.py b/pcapkit/protocols/protocol.py index 5e694c9cf..a3ca55536 100644 --- a/pcapkit/protocols/protocol.py +++ b/pcapkit/protocols/protocol.py @@ -32,6 +32,7 @@ from pcapkit.corekit.context import ContextRegistry from pcapkit.corekit.module import ModuleDescriptor from pcapkit.corekit.protochain import ProtoChain +from pcapkit.corekit.sentinels import _Absent, _AbsentType # pylint: disable=unused-import from pcapkit.protocols import data as data_module from pcapkit.protocols import schema as schema_module from pcapkit.protocols.data.data import Data @@ -40,7 +41,7 @@ from pcapkit.protocols.schema.misc.raw import Raw as Schema_Raw from pcapkit.protocols.schema.schema import Schema from pcapkit.utilities.chardet import detect -from pcapkit.utilities.compat import cached_property, final +from pcapkit.utilities.compat import cached_property from pcapkit.utilities.decorators import beholder, seekset from pcapkit.utilities.exceptions import (ProtocolNotFound, ProtocolNotImplemented, RegistryError, StructError, UnsupportedCall) @@ -91,38 +92,6 @@ '__packet__', 'packet'}) -@final -class _AbsentType: - """Type of :data:`_Absent`, the absent-key sentinel. - - A distinct class rather than a bare :obj:`object` so that the sentinel has a - name of its own in a traceback or a debugger, and so that a type checker has - something to name where ``object()`` would give it nothing. It - follows :class:`~pcapkit.corekit.fields.field.NoValueType`, which does the - same job for an unset field default; this is a sibling of it rather than a - reuse, since that one is documented as the default value of - :attr:`FieldBase.default ` - and means "no value was given", not "this key is not here". - - """ - - def __bool__(self) -> 'Literal[False]': - """Return :obj:`False`.""" - return False - - def __repr__(self) -> 'str': - """Return :obj:`str` representation of the sentinel.""" - return '' - - -#: _AbsentType: Absent-versus-:obj:`None` sentinel for reading ``__keywords__`` -#: out of a class :attr:`~object.__dict__`, where :obj:`None` is a meaningful -#: value -- it is the opt-out that says the class cannot enumerate its keywords, -#: c.f. :attr:`ProtocolBase.__keywords__ -#: `. Never leaves this -#: module: it is read in :func:`_declared_keywords` and discarded there. -_Absent = _AbsentType() - #: Cache for :func:`_declared_keywords`, keyed by protocol class. A protocol's #: signatures do not change after the class is created, and the walk below is #: :math:`O(\\text{MRO} \\times \\text{methods})`, so it is done once per class diff --git a/tests/corekit/test_sentinel_exports_unit.py b/tests/corekit/test_sentinel_exports_unit.py index f9865bc1c..3e97be184 100644 --- a/tests/corekit/test_sentinel_exports_unit.py +++ b/tests/corekit/test_sentinel_exports_unit.py @@ -32,6 +32,15 @@ out of :attr:`__all__` in both directions, which is what the ruling means by "to users". +A follow-up to this same issue moved all four *definitions* into +:mod:`pcapkit.corekit.sentinels`, per the owner's later ruling -- *"Okay one module +for all four it is."* Every assertion above still holds unchanged, since it is about +each original module's ``__all__``, which the re-export shims left untouched; what +changed is only :attr:`type.__module__` for the four types, which +:meth:`SentinelExportTests.test_every_sentinel_type_is_still_importable_by_name` now +checks against :data:`CANONICAL_MODULE` rather than against a different module per +sentinel. + One sentinel is deliberately **not** held to any of this: ``_NOT_FOUND = object()`` at :file:`pcapkit/utilities/compat.py`, line 73, inside the ``cached_property`` backport for interpreters below 3.8. It is ported code and exempt, and @@ -74,14 +83,24 @@ #: Repository root, for the two tests that read a file rather than import it. ROOT = pathlib.Path(__file__).resolve().parents[2] +#: Where all four sentinels are now *defined*, since GitHub issue #911's housing +#: move -- *"Okay one module for all four it is."* Each entry in :data:`SENTINELS` +#: below used to name a different module here (``pcapkit.corekit.module``, +#: ``pcapkit.corekit.fields.field``, ``pcapkit.corekit.enum`` and +#: ``pcapkit.protocols.protocol`` respectively); all four now report this one. +CANONICAL_MODULE = 'pcapkit.corekit.sentinels' + #: Every sentinel in the tree that follows the house ``Type`` convention, -#: as ``(instance name, instance, type, module)``. Four, not the three +#: as ``(instance name, instance, type)``. Four, not the three #: :file:`docs/source/contributing/conventions.rst` used to document -- see the module docstring. +#: All four now share :data:`CANONICAL_MODULE` as their defining module, which is +#: why a per-entry module column is no longer part of this tuple -- see +#: :data:`PUBLIC_SENTINELS` below for the (still distinct) *shim* locations. SENTINELS = ( - ('NULL', NULL, NullType, 'pcapkit.corekit.module'), - ('NoValue', NoValue, NoValueType, 'pcapkit.corekit.fields.field'), - ('NO_DEFAULT', NO_DEFAULT, NoDefaultType, 'pcapkit.corekit.enum'), - ('_Absent', _Absent, _AbsentType, 'pcapkit.protocols.protocol'), + ('NULL', NULL, NullType), + ('NoValue', NoValue, NoValueType), + ('NO_DEFAULT', NO_DEFAULT, NoDefaultType), + ('_Absent', _Absent, _AbsentType), ) #: The public three of :data:`SENTINELS`, as ``(module, object name, type name)``. @@ -210,8 +229,11 @@ def test_star_import_hands_back_the_canonical_object(self) -> 'None': for module, obj, _ in PUBLIC_SENTINELS: namespace = _star_import(module) with self.subTest(module=module): - expected = next(instance for name, instance, _, where in SENTINELS - if name == obj and where == module) + # Matched on the instance name alone: every entry in ``SENTINELS`` + # now shares :data:`CANONICAL_MODULE`, so a ``where == module`` + # filter against the *shim* location would no longer distinguish + # them -- the instance names themselves already do. + expected = next(instance for name, instance, _ in SENTINELS if name == obj) self.assertIs(namespace[obj], expected) def test_every_sentinel_type_is_still_importable_by_name(self) -> 'None': @@ -222,10 +244,10 @@ def test_every_sentinel_type_is_still_importable_by_name(self) -> 'None': relies on that -- this module's own imports are the demonstration. """ - for name, instance, type_, module in SENTINELS: + for name, instance, type_ in SENTINELS: with self.subTest(sentinel=name): self.assertIs(type(instance), type_) - self.assertEqual(type_.__module__, module) + self.assertEqual(type_.__module__, CANONICAL_MODULE) def test_the_private_sentinel_is_exported_neither_way(self) -> 'None': """``_Absent`` is private, so the export rule does not reach it. @@ -248,7 +270,7 @@ class SentinelPopulationTests(unittest.TestCase): def test_every_sentinel_follows_the_naming_convention(self) -> 'None': """*"Keep the sentinel object's type class naming as* ``Type``*."*""" - for name, _, type_, _ in SENTINELS: + for name, _, type_ in SENTINELS: with self.subTest(sentinel=name): self.assertEqual(type_.__name__, _expected_type_name(name)) @@ -257,16 +279,19 @@ def test_conventions_doc_lists_every_sentinel_in_the_tree(self) -> 'None': Asserting the names rather than only the count, because a count corrected without the row -- or a row added without the count -- is the same defect - in a different place. + in a different place. The "Defined in" column now names + :data:`CANONICAL_MODULE` for every row, since GitHub issue #911's housing + move gave all four the same defining module -- checked once, outside the + loop, rather than once per row against a value that no longer varies. """ section = _sentinel_section() - for name, _, type_, module in SENTINELS: + for name, _, type_ in SENTINELS: with self.subTest(sentinel=name): self.assertIn(f'``{name}``', section) self.assertIn(f'``{type_.__name__}``', section) - self.assertIn(module, section) + self.assertIn(CANONICAL_MODULE, section) self.assertIn('four in the tree follow it', section) self.assertNotIn('three in the tree follow it', section) @@ -305,7 +330,7 @@ def test_the_sentinel_table_has_a_row_per_sentinel_and_no_more(self) -> 'None': table = table[:table.index('\n\n', table.index('- Defined in'))] rows = re.findall(r'^ \* - (\S+)$', table, re.MULTILINE) - self.assertEqual(rows, ['Instance'] + [f'``{name}``' for name, _, _, _ in SENTINELS]) + self.assertEqual(rows, ['Instance'] + [f'``{name}``' for name, _, _ in SENTINELS]) class SentinelBehaviourTests(unittest.TestCase): diff --git a/tests/corekit/test_sentinels_housing_unit.py b/tests/corekit/test_sentinels_housing_unit.py new file mode 100644 index 000000000..7d5a22ee9 --- /dev/null +++ b/tests/corekit/test_sentinels_housing_unit.py @@ -0,0 +1,360 @@ +# -*- coding: utf-8 -*- +"""GitHub issue #911's housing half: one module for all four sentinels. + +The `__all__` half of #911 landed as #916 and is pinned by +:mod:`tests.corekit.test_sentinel_exports_unit`. The owner left the second +question -- where the four sentinels should be *defined* -- open, and settled it +with a one-line ruling once offered the choice: *"Okay one module for all four it +is."* + +So :mod:`pcapkit.corekit.sentinels` is now the single defining module for all +four -- :class:`~pcapkit.corekit.sentinels.NullType`, +:class:`~pcapkit.corekit.sentinels.NoValueType`, +:class:`~pcapkit.corekit.sentinels.NoDefaultType` and +:class:`~pcapkit.corekit.sentinels._AbsentType`, and their four instances -- and +each of the four original modules (:mod:`pcapkit.corekit.module`, +:mod:`pcapkit.corekit.fields.field`, :mod:`pcapkit.corekit.enum` and +:mod:`pcapkit.protocols.protocol`) keeps a re-export so that no existing +``from import `` breaks. + +None of this module exists before that move, which is exactly what makes it a +regression test rather than a description: on ``origin/main`` at ``d31c0aaf6`` +(the tip this branch was cut from), ``import pcapkit.corekit.sentinels`` itself +fails -- + +.. code-block:: text + + ModuleNotFoundError: No module named 'pcapkit.corekit.sentinels' + +-- so every test below, including the module-level import at the top of this +file, fails before its body ever runs. Quoted from an actual run against that +commit, not inferred: + +.. code-block:: text + + $ PYTHONPATH= python -m pytest tests/corekit/test_sentinels_housing_unit.py + ERRORS + ImportError while importing test module '.../test_sentinels_housing_unit.py' + ModuleNotFoundError: No module named 'pcapkit.corekit.sentinels' + +:class:`IdentityAcrossShimsTests` is the load-bearing class here. A sentinel's +entire reason for existing is identity comparison (``value is SENTINEL``), so the +one thing worth pinning about a re-export shim is not merely that it resolves -- +an equal-but-distinct object would make every existing ``import`` line "work" +while quietly breaking every ``is NULL`` (etc.) check downstream -- but that +``from pcapkit.corekit.module import NULL`` and +``from pcapkit.corekit.sentinels import NULL`` hand back the *same* object, for +all four pairs. :meth:`IdentityAcrossShimsTests.test_shim_and_canonical_import_are_the_same_object` +asserts that with ``is``, never ``==``. + +:class:`NoImportCycleTests` pins the other thing the issue asked to be checked +"on purpose" rather than left to the 39 pre-existing ``cyclic-import`` findings +:command:`pylint` already reports for this package (which would swallow a 40th +without comment): that :mod:`pcapkit.corekit.sentinels` itself imports nothing +from any of the four modules it is now depended on by, so the dependency graph +between them is a star with :mod:`pcapkit.corekit.sentinels` at the centre and no +edge pointing back into it. + +""" +from __future__ import annotations + +import importlib +import pickle +import unittest + +import pcapkit.corekit.enum as enum_module +import pcapkit.corekit.fields.field as field_module +import pcapkit.corekit.module as module_module +import pcapkit.corekit.sentinels as sentinels +import pcapkit.protocols.protocol as protocol_module +from pcapkit.corekit.enum import NO_DEFAULT, NoDefaultType +from pcapkit.corekit.fields.field import NoValue, NoValueType +from pcapkit.corekit.module import NULL, NullType +from pcapkit.protocols.protocol import _Absent, _AbsentType +from tests._support import purge_modules + +#: Every sentinel, as ``(instance name, shim module, shim instance, shim type)``. +#: The shim module is the *original* location -- the one whose re-export this +#: pins -- and is deliberately not :mod:`pcapkit.corekit.sentinels` itself, which +#: is what :data:`pcapkit.corekit.sentinels`'s own attributes are compared against +#: in each test below rather than being folded into this tuple a fifth time. +SHIMMED_SENTINELS = ( + ('NULL', module_module, NULL, NullType), + ('NoValue', field_module, NoValue, NoValueType), + ('NO_DEFAULT', enum_module, NO_DEFAULT, NoDefaultType), + ('_Absent', protocol_module, _Absent, _AbsentType), +) + + +class IdentityAcrossShimsTests(unittest.TestCase): + """A re-export must hand back the *same* object, not an equal one.""" + + def test_shim_and_canonical_import_are_the_same_object(self) -> 'None': + """``is``, not ``==`` -- the whole point of a sentinel. + + Fails on an implementation that re-*constructs* the sentinel at each + shim location (e.g. a shim that says ``NULL = NullType()`` instead of + ``from pcapkit.corekit.sentinels import NULL``) even though such a shim + would satisfy every ``isinstance`` check and every existing import + statement. + + """ + for name, shim_instance, canonical_name in ( + ('NULL', NULL, 'NULL'), + ('NoValue', NoValue, 'NoValue'), + ('NO_DEFAULT', NO_DEFAULT, 'NO_DEFAULT'), + ('_Absent', _Absent, '_Absent'), + ): + with self.subTest(sentinel=name): + self.assertIs(shim_instance, getattr(sentinels, canonical_name)) + + def test_shim_and_canonical_type_are_the_same_class_object(self) -> 'None': + """The *type*, not only the instance, is one object shared everywhere. + + A shim that rebuilt the type (``class NullType(sentinels.NullType): + pass``) would let ``isinstance`` checks against either name keep working + while making ``NullType is sentinels.NullType`` false -- exactly the + gap an ``is``-only check on the instance above would miss. + + """ + for name, shim_type, canonical_name in ( + ('NullType', NullType, 'NullType'), + ('NoValueType', NoValueType, 'NoValueType'), + ('NoDefaultType', NoDefaultType, 'NoDefaultType'), + ('_AbsentType', _AbsentType, '_AbsentType'), + ): + with self.subTest(sentinel=name): + self.assertIs(shim_type, getattr(sentinels, canonical_name)) + + def test_shim_module_attribute_is_the_same_object_too(self) -> 'None': + """Reached through the *module*, the way every real caller does it. + + The two tests above import through this module's own ``from ... import`` + statements at the top of the file, which python's import machinery + could in principle special-case. Re-reading the attribute off each + shim module object directly rules that out. + + """ + for name, shim_module, shim_instance, shim_type in SHIMMED_SENTINELS: + with self.subTest(sentinel=name): + self.assertIs(getattr(shim_module, name), shim_instance) + self.assertIs(shim_instance, getattr(sentinels, name)) + # The type is read off the *instance* (``type(shim_instance)``) + # rather than reconstructed from the instance's name, since the + # house ``Type`` name is not a straight capitalisation + # of every instance name (``NULL`` -> ``NullType``, not + # ``NULLType``) -- exactly what a caller comparing + # ``type(value) is NullType`` would do instead of guessing. + self.assertIs(type(shim_instance), shim_type) + + def test_defining_module_is_the_canonical_one_for_every_type(self) -> 'None': + """:attr:`type.__module__` names where the class statement executed. + + Distinct from the identity checks above: two different classes could in + principle report the same ``__module__`` by coincidence, but the same + *class object* reporting the wrong ``__module__`` would mean something + rebuilt it under a different name, which is exactly the failure mode + :meth:`test_shim_and_canonical_type_are_the_same_class_object` also + guards against from a different angle. + + """ + for name, _, _, shim_type in SHIMMED_SENTINELS: + with self.subTest(sentinel=name): + self.assertEqual(shim_type.__module__, 'pcapkit.corekit.sentinels') + + +class NoImportCycleTests(unittest.TestCase): + """:mod:`pcapkit.corekit.sentinels` must not import back into a consumer. + + The issue's own instruction: check for a cycle *deliberately*, because + :command:`pylint` already reports 39 ``cyclic-import`` findings in this + package, so a fortieth would be invisible in the noise rather than caught. + This does not run :command:`pylint` -- that check is done separately and + reported alongside this PR -- it instead pins the one fact that actually + rules a cycle out for this specific set of modules: that + :mod:`pcapkit.corekit.sentinels` imports none of them. + + """ + + def test_sentinels_module_imports_none_of_its_four_consumers(self) -> 'None': + """Parse the source's *import statements*, rather than trust that nothing + changes them later. + + A cycle here would need :mod:`pcapkit.corekit.sentinels` to import from + :mod:`pcapkit.corekit.module`, :mod:`pcapkit.corekit.fields.field`, + :mod:`pcapkit.corekit.enum` or :mod:`pcapkit.protocols.protocol` -- each + of which imports *it*. Walked with :mod:`ast` rather than grepped for the + dotted name as plain text, because the module's own docstring above + names all four *in prose*, explaining where each sentinel used to live -- + a text search would flag that explanation as if it were the cycle it is + warning readers there is no longer any risk of. + + """ + import ast + import pathlib + + source = pathlib.Path(sentinels.__file__).read_text(encoding='utf-8') + tree = ast.parse(source, filename=sentinels.__file__) + + imported_modules = [] # type: list[str] + for node in ast.walk(tree): + if isinstance(node, ast.Import): + imported_modules.extend(alias.name for alias in node.names) + elif isinstance(node, ast.ImportFrom) and node.module is not None: + imported_modules.append(node.module) + + forbidden = ('pcapkit.corekit.module', 'pcapkit.corekit.fields.field', + 'pcapkit.corekit.enum', 'pcapkit.protocols.protocol') + for name in forbidden: + with self.subTest(forbidden=name): + self.assertFalse( + any(imported == name or imported.startswith(name + '.') + for imported in imported_modules), + f'{sentinels.__name__} imports {name!r}: {imported_modules!r}') + + def test_sentinels_module_imports_cleanly_on_its_own(self) -> 'None': + """A module with no cyclic dependency imports standalone, purged first. + + Purging every ``pcapkit`` entry from :data:`sys.modules` first, so this + is a cold import rather than a cache hit that would pass regardless of + whether a cycle exists -- a real cycle raises :exc:`ImportError` + (*"cannot import name ... from partially initialized module"*) on + exactly this kind of cold, direct import. + + """ + purge_modules(['pcapkit']) + try: + fresh = importlib.import_module('pcapkit.corekit.sentinels') + finally: + purge_modules(['pcapkit']) + self.assertTrue(hasattr(fresh, 'NULL')) + self.assertTrue(hasattr(fresh, 'NoValue')) + self.assertTrue(hasattr(fresh, 'NO_DEFAULT')) + + def test_each_consumer_module_still_imports_cleanly_on_its_own(self) -> 'None': + """The other direction: importing a consumer first must not deadlock either. + + Each of the four is tried as the *first* thing imported after a purge, + one at a time -- the order a cycle would actually be sensitive to, + since a cycle through ``pcapkit.corekit.sentinels`` would surface as + whichever of the two modules is entered second finding the other only + partially initialised. + + """ + for name in ('pcapkit.corekit.module', 'pcapkit.corekit.fields.field', + 'pcapkit.corekit.enum', 'pcapkit.protocols.protocol'): + with self.subTest(first_import=name): + purge_modules(['pcapkit']) + try: + fresh = importlib.import_module(name) + finally: + purge_modules(['pcapkit']) + self.assertIsNotNone(fresh) + + +class SentinelsModuleExportRuleTests(unittest.TestCase): + """The canonical module follows its own house rule: objects only, in ``__all__``. + + Nothing forces :mod:`pcapkit.corekit.sentinels` to honour the *"only export the + objects"* ruling for itself -- the ruling was stated about the shim locations, + which #916 already fixed -- but shipping a brand new module that violates the + rule its own docstring cites would be a strange way to land it, so this pins + that it does not. + + """ + + def test_all_names_the_three_public_objects_and_no_types(self) -> 'None': + """``_Absent`` stays out too: it is private regardless of which module + defines it.""" + self.assertIn('NULL', sentinels.__all__) + self.assertIn('NoValue', sentinels.__all__) + self.assertIn('NO_DEFAULT', sentinels.__all__) + for name in ('NullType', 'NoValueType', 'NoDefaultType', + '_Absent', '_AbsentType'): + with self.subTest(name=name): + self.assertNotIn(name, sentinels.__all__) + + +class PreMovePickleStillLoadsTests(unittest.TestCase): + """A pickle written before the move still unpickles. + + :meth:`NullType.__reduce__` names its factory by module path, so every + payload written before GitHub issue #911 relocated the definitions carries + the literal string ``pcapkit.corekit.module _get_null``. Moving the + function without leaving the name behind makes those payloads unloadable -- + measured as ``AttributeError: module 'pcapkit.corekit.module' has no + attribute '_get_null'`` -- which is a break in on-disk data rather than in + the API, and so invisible to every other test here. :data:`NULL` is public + and is what :attr:`ModuleDescriptor.name` holds, so the payloads are real. + + The fix is one re-exported private name in :mod:`pcapkit.corekit.module`; + these tests are what stop it being tidied away later as an unused import. + + """ + + def setUp(self) -> 'None': + """Resolve the modules afresh rather than using this file's own imports. + + :class:`NoImportCycleTests` purges ``pcapkit*`` from :data:`sys.modules` + and does not put it back, and it sorts ahead of this class. The + module-level :data:`NULL` above is therefore a stale object by the time + these tests run, whose ``_get_null`` is no longer the one a fresh import + produces -- which pickle rejects outright with ``Can't pickle …: it's not + the same object as pcapkit.corekit.sentinels._get_null``. Re-importing + here keeps these tests measuring the re-export rather than that + pre-existing ordering hazard. + + """ + self.sentinels = importlib.import_module('pcapkit.corekit.sentinels') + self.module = importlib.import_module('pcapkit.corekit.module') + self.null = self.module.NULL + + #: A ``NULL`` payload as the pre-#911 code emitted it. + #: + #: Built by pointing the factory's ``__module__`` at its old home for the + #: duration of the dump, because that attribute is exactly what pickle + #: consults to name it. Substituting the module string in the finished bytes + #: does not work: protocols 4 and 5 length-prefix it, so shortening + #: ``pcapkit.corekit.sentinels`` to ``pcapkit.corekit.module`` leaves a + #: stale prefix and the payload fails to load as truncated rather than for + #: the reason under test. + def _pre_move_payload(self, protocol: 'int') -> 'bytes': + factory = self.sentinels._get_null + original = factory.__module__ + factory.__module__ = 'pcapkit.corekit.module' + try: + blob = pickle.dumps(self.null, protocol=protocol) + finally: + factory.__module__ = original + self.assertIn(b'pcapkit.corekit.module', blob) + self.assertNotIn(b'pcapkit.corekit.sentinels', blob) + return blob + + def test_the_old_factory_name_is_still_reachable(self) -> 'None': + """The re-export, stated as the property rather than as an import line.""" + self.assertIs(self.module._get_null, self.sentinels._get_null) + + def test_a_pre_move_payload_round_trips_to_the_singleton(self) -> 'None': + """Not merely "does not raise": it has to hand back *the* ``NULL``. + + From protocol 0, not 2. :meth:`NullType.__reduce__` says it covers + *"every :mod:`pickle` protocol"* and singles out 0 and 1 as the ones + that would otherwise reach :func:`copyreg._reconstructor`, so those are + the protocols the mechanism most exists for and the last ones to leave + unasserted. Measured: all six name the old path and load back to the + singleton. + + """ + for protocol in range(0, pickle.HIGHEST_PROTOCOL + 1): + with self.subTest(protocol=protocol): + self.assertIs(pickle.loads(self._pre_move_payload(protocol)), self.null) + + def test_a_freshly_written_payload_names_the_new_home(self) -> 'None': + """The re-export is for reading old data, not for writing new.""" + blob = pickle.dumps(self.null, protocol=2) + self.assertIn(b'pcapkit.corekit.sentinels', blob) + self.assertNotIn(b'pcapkit.corekit.module', blob) + + +if __name__ == '__main__': + unittest.main()