feat(corekit,schema,utilities)!: enforce @final at runtime on Info and Schema (#778)

docs/source/pcapkit/corekit/infoclass.rst

@@ -15,6 +15,7 @@ in :pep:`557`.
    :param \*args: Arbitrary positional arguments.
    :param \*\*kwargs: Arbitrary keyword arguments.
 
+   .. automethod:: __init_subclass__
    .. automethod:: __post_init__
 
    .. autoattribute:: __additional__

docs/source/pcapkit/protocols/index.rst

@@ -177,6 +177,7 @@ Header Schema
    .. autoattribute:: __excluded__
       :no-value:
 
+   .. automethod:: __init_subclass__
    .. automethod:: __new__
 
    .. automethod:: pack

pcapkit/corekit/infoclass.py

@@ -14,10 +14,10 @@
 import collections.abc
 import enum
 import itertools
-from typing import TYPE_CHECKING, Generic, TypeVar, final
+from typing import TYPE_CHECKING, Generic, TypeVar
 
-from pcapkit.utilities.compat import Mapping
-from pcapkit.utilities.exceptions import UnsupportedCall, stacklevel
+from pcapkit.utilities.compat import Mapping, final
+from pcapkit.utilities.exceptions import InfoError, UnsupportedCall, stacklevel
 from pcapkit.utilities.warnings import InfoWarning, warn
 
 if TYPE_CHECKING:
@@ -38,7 +38,17 @@ class FinalisedState(enum.IntEnum):
     NONE = enum.auto()
     #: Base class.
     BASE = enum.auto()
-    #: Finalised.
+    #: Finalised. A class reaching this state is also handed to
+    #: :func:`~pcapkit.utilities.compat.final`, so a correctly finalised class
+    #: carries *both* markers -- and they answer two different questions, which
+    #: is why both are read rather than one standing in for the other.
+    #: ``__final__`` answers "may this be subclassed?", for
+    #: :meth:`Info.__init_subclass__`; this state answers "has
+    #: :func:`info_final` already generated the attributes?", for
+    #: :func:`info_final`'s own re-entry check. Carrying ``__final__``
+    #: *without* this state is the mismatch :meth:`Info.__new__` refuses:
+    #: marked final by hand, never finalised, and so missing the generated
+    #: ``__init__`` that makes the class usable at all.
     FINAL = enum.auto()
 
 
@@ -51,9 +61,22 @@ def info_final(cls: 'ST', *, _finalised: 'bool' = True) -> 'ST':
     time as well as caching already generated attributes.
 
     Notes:
-        The decorator should only be used on the *final*
-        class, otherwise, any subclasses derived from a
-        finalised info class will not be re-finalised.
+        The decorator should only be used on the *final* class. Applying it
+        with ``_finalised=True`` seals the class against *subclassing*, which
+        :meth:`Info.__init_subclass__` enforces, and marks it with
+        :func:`~pcapkit.utilities.compat.final` -- so ``@info_final`` implies
+        ``@final`` and there is never a reason to write both. Writing both is
+        harmless in either order, though; what is not is ``@final`` *without*
+        this decorator, which :meth:`Info.__new__` refuses -- provided ``@final``
+        is :func:`~pcapkit.utilities.compat.final` (or, on Python 3.11 and up,
+        :func:`typing.final` directly). Below 3.11, plain :func:`typing.final`
+        does not set ``__final__`` at all (gh-90500), so a class marked with a
+        user's own ``from typing import final`` on 3.10 is not refused: there is
+        nothing on the class for :meth:`Info.__new__` to read.
+
+        Applying this decorator to the same class a second time only warns: the
+        first application already did the work, so the duplicate is redundant
+        rather than wrong, and the class comes back finalised and usable.
 
     Args:
         cls: Info class.
@@ -62,13 +85,47 @@ def info_final(cls: 'ST', *, _finalised: 'bool' = True) -> 'ST':
     Returns:
         Finalised info class.
 
+    Warns:
+        pcapkit.utilities.warnings.InfoWarning: If ``cls`` has already been
+            finalised *by this function*, i.e. it carries
+            :attr:`FinalisedState.FINAL` in its own ``__dict__``. The class is
+            returned untouched.
+
     :meta decorator:
     """
-    if cls.__finalised__ == FinalisedState.FINAL:
+    # NOTE: keyed on ``__finalised__`` rather than on
+    # :func:`~pcapkit.utilities.compat.final`'s ``__final__``, because the two
+    # record different facts and only this one records *this function having
+    # run*. Decorators apply bottom-up, so ``@info_final`` over ``@final``
+    # reaches here with ``__final__`` already set by a decorator that generated
+    # nothing: a ``__final__`` test would read that as "already finalised", skip
+    # the generation, and hand back precisely the ``__init__``-less class
+    # :meth:`Info.__new__` now refuses -- while the opposite order worked. Keying
+    # on ``__finalised__`` is what makes the two orders agree.
+    #
+    # ``cls.__dict__`` rather than ``getattr``: ``__finalised__`` is an ordinary
+    # class attribute and so inherited, and a subclass declared *before* its
+    # parent was finalised reads FINAL through ``getattr`` while never having
+    # been finalised itself. Skipping it would skip the one operation it still
+    # needs -- silently, since this path only warns.
+    if cls.__dict__.get('__finalised__') == FinalisedState.FINAL:
         warn(f'{cls.__name__}: info class has been finalised; now skipping',
              InfoWarning, stacklevel=stacklevel())
         return cls
 
+    # NOTE: ``Info`` itself never reaches ``FinalisedState.BASE`` (see the
+    # ``cls is not Info`` guard below), so a bare ``Info()`` re-enters this
+    # function on *every* call rather than once -- and every line below this
+    # point, not just the ``__excluded__`` write, would otherwise redo the same
+    # ``dir()``-over-the-MRO scan each time. ``__base_ready__`` is a marker of
+    # its own, separate from ``__finalised__``, that records only "this base
+    # class's one-time setup already ran" -- checked and set in ``Info``'s own
+    # ``__dict__`` alone, so it is invisible to every check keyed on
+    # ``__finalised__`` staying ``NONE``. A subclass is never affected: it is
+    # promoted to ``BASE`` on its own first call and never reaches here again.
+    if cls is Info and cls.__dict__.get('__base_ready__'):
+        return cls
+
     temp = ['__map__', '__map_reverse__', '__builtin__', '__finalised__']
     temp.extend(cls.__additional__)
     for obj in cls.mro():
@@ -134,7 +191,26 @@ def info_final(cls: 'ST', *, _finalised: 'bool' = True) -> 'ST':
         cls.__init__.__qualname__ = f'{cls.__name__}.__init__'  # type: ignore[misc]
 
     if not _finalised:
-        cls.__finalised__ = FinalisedState.BASE
+        # NOTE: ``Info`` itself must never receive this promotion. ``__finalised__``
+        # is an ordinary class attribute, and this branch is only ever reached with
+        # ``cls`` bound to ``Info`` when something bare-constructs ``Info()``
+        # directly -- every real subclass reaches :meth:`Info.__new__` with ``cls``
+        # bound to itself. Writing ``BASE`` onto ``Info.__dict__`` would therefore
+        # make *every* subclass declared afterwards inherit ``BASE`` from ``Info``,
+        # so its own ``cls.__finalised__ == FinalisedState.NONE`` test in
+        # :meth:`Info.__new__` would never see ``NONE`` again -- silently defeating
+        # both that one-shot auto-finalisation and the bare-``@final`` guard nested
+        # inside it, for every class declared after the first bare ``Info()``
+        # anywhere in the process. See GitHub issue #778's cross-review, which
+        # demonstrated the bypass with a class declared after such a call.
+        #
+        # ``__base_ready__`` is set instead, for the short-circuit at the top of
+        # this function -- own-``__dict__`` only, so it neither inherits nor
+        # touches ``__finalised__``.
+        if cls is not Info:
+            cls.__finalised__ = FinalisedState.BASE
+        else:
+            cls.__base_ready__ = True
         return cls
 
     cls.__finalised__ = FinalisedState.FINAL
@@ -229,6 +305,49 @@ class Info(Mapping[str, VT], Generic[VT], metaclass=InfoMeta):
     #: List of names to be excluded from :obj:`dict` conversion.
     __excluded__: 'list[str]' = []
 
+    def __init_subclass__(cls, /, *args: 'Any', **kwargs: 'Any') -> 'None':
+        """Refuse to derive from a finalised info class.
+
+        :func:`~pcapkit.utilities.compat.final` is a promise to the type
+        checker and nothing more: it records ``__final__`` on the class and
+        leaves the interpreter free to subclass it anyway. Every class
+        :func:`info_final` finalises carries a generated ``__init__`` built
+        from the annotations that were visible at that moment, so a subclass
+        added afterwards inherits a constructor that does not know about its
+        own fields -- silently, and with the ``__builtin__`` and
+        ``__excluded__`` sets of its parent. This turns that promise into a
+        rule the interpreter keeps.
+
+        Args:
+            *args: Arbitrary positional arguments.
+            **kwargs: Arbitrary keyword arguments in class definition.
+
+        Raises:
+            InfoError: If any class in ``cls``'s ancestry carries the
+                ``__final__`` marker in its own ``__dict__``.
+
+        """
+        # NOTE: the whole ancestry rather than ``cls.__bases__``, because the
+        # direct bases alone leave a one-line way round the rule: declare the
+        # subclass *before* applying the decorator to its parent, and every
+        # further descendant of that subclass is then unguarded, since the
+        # marker sits two levels up rather than one. Walking the MRO costs one
+        # dict lookup per ancestor, once per class declaration.
+        #
+        # ``base.__dict__`` rather than ``getattr`` for the reason given in
+        # :func:`info_final`: ``__final__`` is inherited, so a ``getattr`` here
+        # would read it off every descendant and reject declarations that
+        # predate the marker rather than the ones the rule is about.
+        for base in cls.__mro__[1:]:
+            if base.__dict__.get('__final__'):
+                raise InfoError(f'{cls.__name__}: cannot subclass {base.__name__}, '
+                                'which is final')
+
+        # NOTE: ``*args`` and ``**kwargs`` are forwarded rather than swallowed, so
+        # that a class keyword nobody accepts still reaches ``object`` and still
+        # fails there, as it did before this hook existed.
+        super().__init_subclass__(*args, **kwargs)
+
     def __new__(cls, *args: 'VT', **kwargs: 'VT') -> 'Self':  # pylint: disable=unused-argument
         """Create a new instance.
 
@@ -240,8 +359,66 @@ def __new__(cls, *args: 'VT', **kwargs: 'VT') -> 'Self':  # pylint: disable=unus
             *args: Arbitrary positional arguments.
             **kwargs: Arbitrary keyword arguments.
 
+        Raises:
+            InfoError: If ``cls`` was marked with
+                :func:`~pcapkit.utilities.compat.final` but never finalised by
+                :func:`info_final`, i.e. it carries ``__final__`` in its own
+                ``__dict__`` without :attr:`FinalisedState.FINAL`.
+
+        Warning:
+            Out of scope, deliberately: a ``@final`` class descending from a
+            :attr:`FinalisedState.BASE` ancestor is not caught. ``__finalised__``
+            inherits, and a ``BASE`` ancestor means some earlier bare
+            construction already walked this branch for the chain -- so the
+            subclass's own ``cls.__finalised__`` reads ``BASE`` too, the branch
+            below is skipped entirely, and the marker on ``cls`` itself is never
+            inspected. Closing this means re-finalising every descendant of a
+            ``BASE`` class on each subclassing, which is the auto-finalisation
+            behaviour ``test_an_unfinalised_descendant_is_not_mistaken_for_a_
+            mismarked_class`` deliberately leaves alone. In the tree today no
+            ``BASE``-state class is ever marked ``@final`` by hand, so the escape
+            is theoretical rather than live.
+
         """
+        # NOTE: ``final`` is applied *after* the class object exists, so
+        # :meth:`__init_subclass__` has already run and returned by the time a
+        # bare ``@final`` lands -- it cannot see the mistake, and no other
+        # class-creation hook fires later. First instantiation is the next event
+        # in the class's life this library controls, and it is also where the
+        # damage surfaced: an unfinalised class has no generated ``__init__``, so
+        # construction fell through to :meth:`__update__` and failed with
+        # ``TypeError: 'int' object is not iterable``, naming neither the class
+        # nor the mistake.
+        #
+        # It is nested inside the NONE branch rather than run ahead of it, and
+        # that placement is what makes it free on the hot path: a finalised class
+        # never enters the branch at all, so it never evaluates the nested
+        # ``cls.__dict__.get('__final__')`` either -- ``dis.dis(Info.__new__)``
+        # shows the ``FinalisedState.NONE`` comparison compiles to a single
+        # ``POP_JUMP_IF_FALSE`` that, for a ``FINAL`` class, jumps straight past
+        # this whole block to ``super().__new__``: zero of the added bytecode
+        # executes, which is a property of the compiled branch rather than of
+        # any particular timing run, and is the reason to trust here rather than
+        # a timer. The raise also leaves ``__finalised__`` at NONE, so a second
+        # attempt re-enters and fails again rather than the error firing once.
+        #
+        # ``cls.__dict__`` rather than ``getattr``: not because a class that
+        # inherits ``__final__`` from a *finalised* ancestor would be misread --
+        # such a class also inherits ``__finalised__ == FINAL``, so the outer
+        # ``if`` above already excludes it before this line is ever reached,
+        # regardless of which lookup is used here. The case ``getattr`` gets
+        # wrong is an ancestor hand-marked bare ``@final`` and *never finalised*:
+        # its own ``__finalised__`` stays ``NONE`` and so, inherited, does every
+        # subclass declared before the marker landed -- so such a subclass still
+        # reaches this line, and ``getattr`` would read the marker it merely
+        # inherits and blame it for a mismarking that is its ancestor's, not its
+        # own. ``OwnDictRuleTests.test_a_subclass_that_predates_an_unfinalised_
+        # ancestors_bare_final_is_not_blamed_for_it`` pins exactly this shape --
+        # mutating this line to ``getattr`` fails it.
         if cls.__finalised__ == FinalisedState.NONE:
+            if cls.__dict__.get('__final__'):
+                raise InfoError(f'{cls.__name__}: marked final but never finalised, so it has no generated '
+                                '__init__; apply info_final, which applies final itself, not final alone')
             cls = info_final(cls, _finalised=False)
         self = super().__new__(cls)
 

pcapkit/protocols/schema/misc/pcapng.py

@@ -728,6 +728,16 @@ class NewOption(Option, ns='opt', code=Enum_OptionType.opt_new):
             - :class:`pcapkit.const.pcapng.option_type.OptionType`
 
         """
+        # NOTE: the base hook goes first, before any of the registration work
+        # below -- see :meth:`EnumSchema.__init_subclass__`, which makes the same
+        # move for the same reason. :meth:`Schema.__init_subclass__` is what
+        # refuses to derive from a finalised schema, and a refusal has to land
+        # before :meth:`Option.register` writes ``cls`` into ``__enum__``: calling
+        # this last, as this method used to, let a raise discard the class object
+        # while leaving the registry pointing at it, so a rejected declaration
+        # still displaced a built-in option schema for the rest of the process.
+        super().__init_subclass__()
+
         if ns is not None:
             cls.__namespace__ = ns
 
@@ -740,7 +750,6 @@ class NewOption(Option, ns='opt', code=Enum_OptionType.opt_new):
                     Option.register(_code, cls, ns)
             else:
                 Option.register(code, cls, ns)
-        super().__init_subclass__()
 
     @staticmethod
     def register(code: 'Enum_OptionType', cls: 'Type[Option]', ns: 'Optional[str]' = None) -> 'None':