From da431d1310cb50af99c68a2dc9b226dcc0d24603 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Sun, 4 Oct 2026 22:23:20 -0400 Subject: [PATCH] fix(foundation): declare the engine-typed slots as EngineBase, not Engine Three declarations in `pcapkit/foundation/extraction.py` named a class the attribute never holds. Closes #1022. * `_exeng` (`:183`), `Extractor.engine` (`:354`) and `record_header` (`:738`) said `Engine`, and the latter two said it unparameterised. Both built-in engines subclass `EngineBase` directly -- `PCAP` is `EngineBase[Frame]` and `PCAPNG` is `EngineBase[PCAPNG]` -- so neither has ever been an `Engine` at run time. All three now say `EngineBase[_P]`. * The two `cast` calls at `:609`/`:612` are **retargeted**, not deleted. They were laundering the class *and* the frame type parameter; they now launder only the parameter, which is a narrower and honest claim. * Corrected `record_header`'s docstring, which named a class the attributes do not live on, had lost its verb, and said "also ... as well". Keeping the parameter is what makes this safe rather than merely more accurate. Declaring the slots bare `EngineBase` type-checks identically -- 305 errors either way -- but degrades `_exeng.read_frame()` from `Frame` to `Any`, and that expression is the whole body of `Extractor.__next__` and `__call__`, both declared `-> '_P'`. Bare would have silently dropped the only type-level guarantee that the two public iteration entry points return the frame type they advertise, on a `py.typed` package, with no error-count change to show it. `Engine` adds no instance API over `EngineBase` -- measured, its only added member is `__init_subclass__`, the `dir()` difference is empty in both directions, the metaclass object is the same, and no `__slots__`, `__getattr__` or `__dir__` override appears anywhere in the MRO. A sweep of every read of `_exeng`, `.engine` and `.record_header` across `pcapkit/`, `tests/`, `examples/` and `docs/` found none touching an `Engine`-only member. Breaking, in two ways a type checker sees. Code passing `Extractor.engine` to an `Engine`-typed parameter must now accept `EngineBase`; and since the old declarations were unparameterised, `engine.read_frame()` and `record_header().read_frame()` were `Any` and are now the extractor's frame type, so an assignment relying on `Any` there stops type-checking. New test pins both halves -- that each declaration names a class both built-in engines satisfy, and that it stays parameterised. Eight subtests fail on main, six for the class and two because `engine` and `record_header` are bare there; dropping `[_P]` from any one declaration fails that declaration's own subtest. tests/foundation + tests/interface + tests/project: 555 passed, 13 skipped, 1291 subtests passed. mypy on extraction.py: 305 errors, byte-identical to main's set, none in the file. --- CHANGELOG.md | 1 + docs/source/changelog/1.5.0.rst | 12 +++++ .../contributing/conventions/process.rst | 2 +- pcapkit/foundation/extraction.py | 15 +++--- tests/foundation/test_extraction.py | 49 +++++++++++++++++++ 5 files changed, 70 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e8bc02add..23c29b1e9e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -86,6 +86,7 @@ Preceded by `1.5.0a1` (2026-09-15), `1.5.0b1` and `1.5.0b2` (both 2026-09-18) an - renames with no compatibility alias left behind: `HoleDiscriptor` is spelled `HoleDescriptor` and its package alias `TCP_HoleDiscriptor` is `TCP_HoleDescriptor` ([#350](https://github.com/JarryShaw/PyPCAPKit/pull/350)). - subclass registration is **opt-in** for `Engine`, `Reassembly`, `TraceFlow` and `dumpkit`'s `Dumper` ([#514](https://github.com/JarryShaw/PyPCAPKit/issues/514)). Each registers if and only if its registry keyword is given -- `engine=` for `Engine`, `protocol=` for `Reassembly` and `TraceFlow`, `fmt=` for `Dumper`. Previously an absent keyword fell back to the class' own name, so *every* subclass of the public class was registered, and declining meant subclassing the parallel `*Base` class under an alias, which every built-in does (hence the public classes had **0** subclasses against the `*Base` classes' 9, 5, 2 and 3). **This breaks out-of-tree code that subclasses one of the four and relies on the derived key**; pass the keyword, or call the matching `register_*` function. Nothing the library ships is affected, and the `*Base` classes remain importable. Two silent failures are now loud: an unrecognised class keyword raises `UnsupportedCall` instead of being swallowed by `**kwargs` (passing `name=` to a `Reassembly` subclass silently ignored the key, since `protocol=` is the real one), and so does `Dumper`'s `ext=` without `fmt=`. A class attribute is not an opt-in: `__engine_name__` and `__protocol_name__` still set the name a class reports, registered or not. Each metaclass gained a class-level `registry` property mirroring `EnumSchema.registry`, and a `Dumper` subclass no longer touches the filesystem while its `class` statement runs (inferring `fmt` from `kind` meant instantiating it against a `NamedTemporaryFile`). `Engine`'s keyword is `engine=` rather than the `name=` first shipped, because `name` cannot be a class keyword on Python 3.10: `mcls`, `name`, `bases` and `namespace` collide with `abc.ABCMeta.__new__`'s parameters (positional-or-keyword before 3.11, positional-only from 3.11), raising `TypeError` before the hook runs. Those four are the whole collision surface (measured on 3.10.21, 3.11.15 and 3.14.7), and `engine=`, `protocol=` and `fmt=` are outside it. There is no `name=` alias: a keyword that works on some interpreters and not others is the trap being removed. - extraction is around 46% faster on a 1,117-frame HTTP capture, with byte-identical output ([#420](https://github.com/JarryShaw/PyPCAPKit/pull/420)). A reassembled datagram's payload is analysed on first read rather than eagerly, cutting IP reassembly's own cost by 90.7% and TCP's by 23.7% (IP reassembly submits a datagram for every frame, fragmented or not) ([#424](https://github.com/JarryShaw/PyPCAPKit/pull/424)). Flow tracing over the same capture went from 1416.6 ms to 744.0 ms, because the flow dumper handed each record to a `Frame` constructor that re-dissected the whole stack to return the bytes it had just been given; options are no longer parsed twice either ([#427](https://github.com/JarryShaw/PyPCAPKit/pull/427)). +- **a breaking change to** `Extractor.engine` and `Extractor.record_header`: both are now declared to return `EngineBase[_P]` rather than `Engine`, and the private `_exeng` attribute follows. Both built-in engines subclass `EngineBase` directly, so neither was ever an `Engine`; the declaration now names the class the object has, and the `cast` calls that hid the gap now launder only the frame type parameter. Nothing is lost: `Engine` adds only the `__init_subclass__` registration hook, and the frame type parameter is preserved. Two things are visible to a type checker. Code passing the result to an `Engine`-typed parameter must now accept `EngineBase`; and because the old declarations were *unparameterised*, `engine.read_frame()` and `record_header().read_frame()` used to be `Any` and are now the extractor's own frame type, so an assignment that relied on `Any` there no longer type-checks ([#1022](https://github.com/JarryShaw/PyPCAPKit/issues/1022)). #### Fixed diff --git a/docs/source/changelog/1.5.0.rst b/docs/source/changelog/1.5.0.rst index 27d13b53ac..baf2f12d49 100644 --- a/docs/source/changelog/1.5.0.rst +++ b/docs/source/changelog/1.5.0.rst @@ -835,6 +835,18 @@ Changed dumper handed each record to a ``Frame`` constructor that re-dissected the whole stack to return the bytes it had just been given; options are no longer parsed twice either (:pr:`427`). +* **a breaking change to** ``Extractor.engine`` and ``Extractor.record_header``: both + are now declared to return ``EngineBase[_P]`` rather than ``Engine``, and the private + ``_exeng`` attribute follows. Both built-in engines subclass ``EngineBase`` directly, + so neither was ever an ``Engine``; the declaration now names the class the object + has, and the ``cast`` calls that hid the gap now launder only the frame type + parameter. Nothing is lost: ``Engine`` adds only the ``__init_subclass__`` + registration hook, and the frame type parameter is preserved. Two things are + visible to a type checker. Code passing the result to an ``Engine``-typed + parameter must now accept ``EngineBase``; and because the old declarations were + *unparameterised*, ``engine.read_frame()`` and ``record_header().read_frame()`` + used to be ``Any`` and are now the extractor's own frame type, so an assignment + that relied on ``Any`` there no longer type-checks (:issue:`1022`). Fixed ~~~~~ diff --git a/docs/source/contributing/conventions/process.rst b/docs/source/contributing/conventions/process.rst index f0ccb9a2e3..e1b5f0b90c 100644 --- a/docs/source/contributing/conventions/process.rst +++ b/docs/source/contributing/conventions/process.rst @@ -101,7 +101,7 @@ commands down instead of a figure that will be stale by the next merge: The grouping scheme was settled on :issue:`918`: **a section per top-level module, with** ``Added``/``Changed``/``Fixed`` **nested inside each** -- module granularity, not per-file and not per-subpackage. The file carries **9** module-level sections holding -155 entries, and no entry carries an inline kind label:: +156 entries, and no entry carries an inline kind label:: $ grep -cE '^\* \*\*(Added|Changed|Fixed)\*\*' docs/source/changelog/1.5.0.rst 0 diff --git a/pcapkit/foundation/extraction.py b/pcapkit/foundation/extraction.py index 45850be05e..58ea481e40 100644 --- a/pcapkit/foundation/extraction.py +++ b/pcapkit/foundation/extraction.py @@ -180,7 +180,7 @@ class Extractor(Generic[_P]): #: Extraction engine name. _exnam: 'Engines' #: Extraction engine instance. - _exeng: 'Engine[_P]' + _exeng: 'EngineBase[_P]' #: Input file object. _ifile: 'BufferedReader' @@ -351,7 +351,7 @@ def trace(self) -> 'TraceFlowData': raise UnsupportedCall("'Extractor(trace=False)' object has no attribute 'trace'") @property - def engine(self) -> 'Engine': + def engine(self) -> 'EngineBase[_P]': """PCAP extraction engine.""" return self._exeng @@ -606,10 +606,10 @@ def run(self) -> 'None': # pylint: disable=inconsistent-return-statements if self._magic in PCAP_Engine.MAGIC_NUMBER: logger.debug('magic number %r identifies a PCAP file', self._magic) - self._exeng = cast('Engine[_P]', PCAP_Engine(self)) + self._exeng = cast('EngineBase[_P]', PCAP_Engine(self)) elif self._magic in PCAPNG_Engine.MAGIC_NUMBER: logger.debug('magic number %r identifies a PCAP-NG file', self._magic) - self._exeng = cast('Engine[_P]', PCAPNG_Engine(self)) + self._exeng = cast('EngineBase[_P]', PCAPNG_Engine(self)) else: raise FormatError(f'unknown file format: {self._magic!r}') @@ -735,14 +735,13 @@ def make_name(cls, fin: 'str | IO[bytes]' = 'in.pcap', fout: 'str' = 'out', return ifnm, ofnm, fmt, ext, files - def record_header(self) -> 'Engine': + def record_header(self) -> 'EngineBase[_P]': """Read global header. The method will parse the PCAP global header and save the parsed result to its extraction context. Information such as PCAP version, data link - layer protocol type, nanosecond flag and byteorder will also be save - the current :class:`~pcapkit.foundation.engines.engine.Engine` instance - as well. + layer protocol type, nanosecond flag and byteorder are also saved on the + returned engine instance. If TCP flow tracing is enabled, the nanosecond flag and byteorder will be used for the output PCAP file of the traced TCP flows. diff --git a/tests/foundation/test_extraction.py b/tests/foundation/test_extraction.py index 97acf93933..1eb788a1c4 100644 --- a/tests/foundation/test_extraction.py +++ b/tests/foundation/test_extraction.py @@ -1,11 +1,14 @@ from __future__ import annotations +import ast import importlib.util +import inspect import io import pathlib import sys import tempfile import types +import typing import unittest import warnings from unittest import mock @@ -158,6 +161,52 @@ def test_properties_success_and_unsupported_paths(self) -> None: with self.assertRaises(UnsupportedCall): _ = unsupported.trace + def test_declared_engine_type_is_satisfied_by_the_built_in_engines(self) -> None: + """``_exeng``, ``engine`` and ``record_header`` must name a type the engine holds. + + Both built-in engines subclass :class:`~pcapkit.foundation.engines.engine.EngineBase` + directly (so that they are not auto-registered), hence neither is an + :class:`~pcapkit.foundation.engines.engine.Engine`. The three declarations + used to say ``Engine`` and two ``cast`` calls hid the gap (#1022). + """ + import pcapkit.foundation.extraction as module + from pcapkit.foundation.engines.pcap import PCAP + from pcapkit.foundation.engines.pcapng import PCAPNG + from pcapkit.foundation.extraction import Extractor + + # ``_exeng`` is annotated under ``TYPE_CHECKING`` only, so it is not visible + # to ``get_type_hints``: read the annotation out of the module source. + tree = ast.parse(inspect.getsource(module)) + klass = next(node for node in tree.body + if isinstance(node, ast.ClassDef) and node.name == 'Extractor') + found = [node for node in ast.walk(klass) + if isinstance(node, ast.AnnAssign) + and isinstance(node.target, ast.Name) and node.target.id == '_exeng'] + self.assertEqual(len(found), 1) + node = found[0].annotation + # quoted (``'EngineBase[_P]'``) or bare (``EngineBase[_P]``) spelling + source = node.value if isinstance(node, ast.Constant) else ast.unparse(node) + attribute = eval(source, vars(module)) # pylint: disable=eval-used + + declared = { + '_exeng': attribute, + 'engine': typing.get_type_hints(Extractor.engine.fget, vars(module))['return'], + 'record_header': typing.get_type_hints(Extractor.record_header, vars(module))['return'], + } + # The frame type parameter is what lets mypy check ``read_frame()`` against + # ``Extractor.__next__``'s ``_P``; a bare class would silently make it ``Any``. + for name, hint in declared.items(): + with self.subTest(declaration=name, check='parameterised'): + self.assertTrue(typing.get_args(hint), + f'{name} is declared {hint!r}, which drops the frame type parameter') + for name, hint in declared.items(): + origin = typing.get_origin(hint) or hint + for engine in (PCAP, PCAPNG): + with self.subTest(declaration=name, engine=engine.__name__): + self.assertTrue(issubclass(engine, origin), + f'{name} is declared {hint!r}, which {engine.__name__} ' + 'does not subclass') + def test_register_helpers_validate_descriptors_and_overwrite_warnings(self) -> None: from pcapkit.corekit.module import ModuleDescriptor from pcapkit.dumpkit.null import NotImplementedIO