fix(foundation): declare the engine-typed slots as EngineBase, not Engine by JarryShaw · Pull Request #1023 · JarryShaw/PyPCAPKit

JarryShaw · 2026-10-05T01:51:53Z

Please follow the guide below

Searched for similar pull requests
Followed the coding style (make pylint, make mypy, make isort)
make test passes, and a test case covers the change
Added a changelog entry under docs/source/changelog/ and regenerated CHANGELOG.md, if the change is user-visible

What is the purpose of your pull request?

fix — corrects a defect
feat — adds a feature
perf — changes performance, not behaviour
refactor — changes neither behaviour nor performance
test — tests only
docs — documentation only
ci — workflows or build tooling
chore — anything else

Description of your pull request and other information

Closes #1022. Three declarations in pcapkit/foundation/extraction.py named a class the attribute never holds:

PCAP     bases=['EngineBase']  __orig_bases__=(EngineBase[Frame],)   issubclass(PCAP, Engine)=False
PCAPNG   bases=['EngineBase']  __orig_bases__=(EngineBase[PCAPNG],)  issubclass(PCAPNG, Engine)=False

_exeng (:183), Extractor.engine (:354) and record_header (:738) all 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, and now launder only the parameter.

Keeping the type parameter is the point, and it is why the first revision of this PR was wrong. Declaring the slots bare EngineBase type-checks identically — 305 errors either way, none in the file — but degrades _exeng.read_frame() from Frame to Any. That expression is the whole body of Extractor.__next__ (:1121) and __call__ (:1147), both declared -> '_P', so bare would silently drop 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. Measured with reveal_type:

	`main`	this head
`e._exeng`	`Engine[Frame]`	`EngineBase[Frame]`
`e._exeng.read_frame()`	`Frame`	`Frame`
`e.engine`	`Engine[Any]`	`EngineBase[Frame]`
`e.engine.read_frame()`	`Any`	`Frame`

Why it is safe. Engine adds no instance API over EngineBase — its only added member is __init_subclass__, the dir() difference is empty in both directions, the metaclass object is literally the same so EngineMeta's properties are reachable identically, and there is no __slots__, __getattr__ or __dir__ override 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, hence the label and the 1.5.0 entry. Code passing Extractor.engine to an Engine-typed parameter must now accept EngineBase; and because the old declarations on engine/record_header were unparameterised, read_frame() through them was Any and is now the extractor's frame type, so an assignment relying on Any there stops type-checking. Run-time behaviour is unchanged — the only non-annotation edits are two typing.cast calls, which return their argument untouched.

The 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, two because engine and record_header are bare there — and dropping [_P] from any one declaration fails that declaration's own subtest by name.

tests/foundation + tests/interface + tests/project: 555 passed, 13 skipped, 1291 subtests passed. mypy on extraction.py: 305 errors, the sorted set byte-identical to main's, none in the file. util/changelog_md.py --check exits 0.

Deliberately not done, though measured as viable. Casting at record_header's two local assignments instead of keeping the two # type: ignore[return-value] at :759/:766 also gives 305 errors and would retire three suppressions including the [assignment] one. Declined here because those two ignores already exist on main, so retaining them keeps record_header's body byte-identical to main and this PR to declarations only; the alternative edits the function body for a suppression-style preference. Worth revisiting separately.

Out of scope and untouched: the registry # type: comments at :242/:251/:259, the #: attribute docs at :227/:244/:253, and the whole register_* region — PR #1020 owns that. #1024 tracks the consequence that EngineBase is excluded from __all__ and documented :no-members:, so this public return type currently has no documented API.

JarryShaw · 2026-10-05T02:09:51Z

Cross-review verdict: NEEDS CHANGES (ran on Opus; the change was authored on Sonnet). One substantive finding, which I reproduced myself before acting on it.

Bare EngineBase silently removes type checking that main had. reveal_type on an Extractor[Frame], same probe against both trees:

expression	`main` `3d8a0d476`	this head `c19463965`
`e._exeng`	`Engine[Frame]`	`EngineBase[Any]`
`e._exeng.read_frame()`	`Frame`	`Any`
returning it where `int` is declared	error `[return-value]`	no error

_exeng.read_frame() is the sole expression behind Extractor.__next__ (:1121) and Extractor.__call__ (:1147), both declared -> '_P'. So this head drops the only type-level guarantee that the two public iteration entry points return the frame type they advertise, on a py.typed package. The unchanged 305 error count is consistent with that loss rather than evidence against it — Any never errors.

It is avoidable at zero cost. Keeping the parameter and retargeting the casts instead of deleting them measures at 305 errors in 33 files, 0 in extraction.py — identical to this head — while read_frame() still reveals Frame and the wrong-type probe still errors:

_exeng: 'EngineBase[_P]'
self._exeng = cast('EngineBase[_P]', PCAP_Engine(self))

That is strictly better than both this head and main: the cast now names the class the object actually has and launders only the parameter, rather than lying about the class. #1022 asked for the casts to go, but they turn out to serve a second purpose the issue did not know about. Reworking on that basis; a revision follows.

Everything else in the review reproduced, several claims more strongly than the description states them — notably that dir() alone would not have settled "Engine adds no instance API" (same metaclass object, so EngineMeta's properties are reachable identically from both; no __slots__, no __getattr__/__dir__ override anywhere in the MRO; and a live EngineBase vs Engine subclass pair differs in neither class nor instance __dict__).

…gine 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.

JarryShaw · 2026-10-05T03:10:16Z

Merge this before #1020. They turn out to be complementary, and I have measured it rather than inferred it.

#1020 implements the ruled design from #1016: the public register_* door narrows to the public class, and pcapkit's own base-only built-ins go through a new internal path. That internal path writes base-only classes into Extractor.__engine__, which is declared Type[Engine] — so #1020 also has to widen that store to EngineBase, along with the three metaclass registry properties that return these dicts, since dict is invariant in its value type.

Widening the store then collides with _exeng, which this PR fixes. On #1020 alone, self._exeng = eng(self) at extraction.py:678 draws:

error: Incompatible types in assignment (expression has type "Any | EngineBase[Any]", variable has type "Engine[_P]")  [assignment]

Measured on a scratch tree with this PR's commit cherry-picked onto main and #1020's diff applied on top: 305 errors in 33 files, zero attributed to extraction.py — identical to main's figure. So the error exists only in #1020-without-this-PR, and lands at zero once both are in.

Nothing changes here as a result; this is a note about ordering, not a request. Worth recording because #1020's own CI will carry that one error until this merges, and it should not be mistaken for a defect in #1020.

Implements the maintainer's ruling on #1016: the public `register_*` door names the public class, and the base check moves to an internal path, so neither that ruling nor #513 has to concede. * `register_engine`, `register_reassembly` and `register_traceflow` now guard `issubclass(x, Engine / Reassembly / TraceFlow)` instead of the `*Base` classes. Their messages are unchanged -- `main` already named the public class, which is what made the mismatch #1016 reported. * Three private classmethods take the base check: `Extractor._register_internal_engine` and its two siblings. They unwrap a `ModuleDescriptor`, check the base, keep the overwrite warning, and write the registry. None is in any `__all__`, none is autodocumented, and no `register_extractor_*` wrapper reaches them. * The three registry stores and the three metaclass `registry` properties widen from `Type[Engine]` to `Type[EngineBase]` and friends. The internal path writes base-only classes by design, so the narrow declaration was false by construction rather than by accident; `dict` is invariant in its value type, so the stores could not widen without the properties that return them. * The `#:` prose above each store said the values were "a tuple representing the module name and class name" and an `Engine` subclass. They are `ModuleDescriptor`s and base-only classes -- wrong on both counts before this change, not because of it. The built-ins are not affected by the narrowing and do not use the internal path at runtime: they are `ModuleDescriptor` literals in `Extractor`'s class body and never pass through a registrar. The internal path exists for programmatic internal registration, and is what #513's regression test now exercises. `register_protocol` and `register_dumper` are untouched. The former cannot narrow: `ProtocolBase` has 44 descendants including `Protocol` itself, while the public `Protocol` has none, so narrowing would reject every in-house protocol class. The latter already guards the public `Dumper`. Breaking: a third party subclassing `EngineBase` directly and calling `register_engine` is now rejected where it was accepted. #513's regression test is retargeted, not weakened. It keeps its real subject classes, its unmocked gate, its registry read-back and its anti-no-op comment, and now exercises the internal path -- with a comment citing #1016 so a reader arriving from #513 sees why the assertion moved, plus `mock.patch.dict` rollback it did not have before. What it guaranteed still holds: pcapkit's own base-only classes are registrable through a real gate. Two new tests pin the other half, that the public door rejects a base-only class both as a class and as a `ModuleDescriptor`. mypy, single-file entry point `mypy pcapkit/foundation/extraction.py`: 305 errors, 0 attributed to the file, sorted error set identical to main. The whole-package `mypy pcapkit` form reads 321 on both trees. The store widening draws a `_exeng` mismatch on its own, which #1023 resolved. tests/foundation + tests/interface + tests/project: 557 passed, 13 skipped, 1300 subtests passed.

…ne (#1024) `Extractor.engine` is declared `EngineBase[_P]` since #1023, but `EngineBase` was documented `:no-members:` under Internal Definitions while `Engine` carried every member stub, so the type a reader followed exposed no API. * `engine.rst`: the member stubs move to `EngineBase`, where they are defined; `Engine` documents only `__init_subclass__` and keeps the customisation `seealso`. `EngineBase` is not added to `__all__` -- that is left to #1016. * Cross-references to `Engine.run`, `read_frame`, `close` and `unsupported_reason` (`pep.rst`, `engines/index.rst`, four `_exeng` docstrings in `extraction.py`) now point at `EngineBase`, where they resolve. * `Engine.__init_subclass__`: qualify the bare `__engine_name__` reference, which stopped resolving once the attribute moved to `EngineBase`. * `PCAP.run`: repair the verbless "will also be save the current `PCAP` engine instance", as #1023 did in `extraction.py`. * Changelog entry added; the entry-count pin in `process.rst` is 157. Also corrected `docs/source/contributing/pep.rst`, which said subclassing an engine registers it automatically. Registration has been opt-in since #514: `__init_subclass__` registers only when the `engine` class keyword is given, and defaults to skipping. Measured -- a subclass without the keyword adds nothing to `Extractor.__engine__`; with it, the name appears. Nitpicky Sphinx build: 1239 unresolved references on fa3e861, 1237 here. The two `EngineBase.unsupported_reason` references in `docs/source/index.rst` newly resolve; no new miss. tests/project passes.

…cks (#1020) Implements the maintainer's ruling on #1016: the public `register_*` door names the public class, and the base check moves to an internal path, so neither that ruling nor #513 has to concede. * `register_engine`, `register_reassembly` and `register_traceflow` now guard `issubclass(x, Engine / Reassembly / TraceFlow)` instead of the `*Base` classes. Their messages are unchanged -- `main` already named the public class, which is what made the mismatch #1016 reported. * Three private classmethods take the base check: `Extractor._register_internal_engine` and its two siblings. They unwrap a `ModuleDescriptor`, check the base, keep the overwrite warning, and write the registry. None is in any `__all__`, none is autodocumented, and no `register_extractor_*` wrapper reaches them. * The three registry stores and the three metaclass `registry` properties widen from `Type[Engine]` to `Type[EngineBase]` and friends. The internal path writes base-only classes by design, so the narrow declaration was false by construction rather than by accident; `dict` is invariant in its value type, so the stores could not widen without the properties that return them. * The `#:` prose above each store said the values were "a tuple representing the module name and class name" and an `Engine` subclass. They are `ModuleDescriptor`s and base-only classes -- wrong on both counts before this change, not because of it. The built-ins are not affected by the narrowing and do not use the internal path at runtime: they are `ModuleDescriptor` literals in `Extractor`'s class body and never pass through a registrar. The internal path exists for programmatic internal registration, and is what #513's regression test now exercises. `register_protocol` and `register_dumper` are untouched. The former cannot narrow: `ProtocolBase` has 44 descendants including `Protocol` itself, while the public `Protocol` has none, so narrowing would reject every in-house protocol class. The latter already guards the public `Dumper`. Breaking: a third party subclassing `EngineBase` directly and calling `register_engine` is now rejected where it was accepted. #513's regression test is retargeted, not weakened. It keeps its real subject classes, its unmocked gate, its registry read-back and its anti-no-op comment, and now exercises the internal path -- with a comment citing #1016 so a reader arriving from #513 sees why the assertion moved, plus `mock.patch.dict` rollback it did not have before. What it guaranteed still holds: pcapkit's own base-only classes are registrable through a real gate. Two new tests pin the other half, that the public door rejects a base-only class both as a class and as a `ModuleDescriptor`. mypy, single-file entry point `mypy pcapkit/foundation/extraction.py`: 305 errors, 0 attributed to the file, sorted error set identical to main. The whole-package `mypy pcapkit` form reads 321 on both trees. The store widening draws a `_exeng` mismatch on its own, which #1023 resolved. tests/foundation + tests/interface + tests/project: 557 passed, 13 skipped, 1300 subtests passed.

…ne (#1024) (#1027) `Extractor.engine` is declared `EngineBase[_P]` since #1023, but `EngineBase` was documented `:no-members:` under Internal Definitions while `Engine` carried every member stub, so the type a reader followed exposed no API. * `engine.rst`: the member stubs move to `EngineBase`, where they are defined; `Engine` documents only `__init_subclass__` and keeps the customisation `seealso`. `EngineBase` is not added to `__all__` -- that is left to #1016. * Cross-references to `Engine.run`, `read_frame`, `close` and `unsupported_reason` (`pep.rst`, `engines/index.rst`, four `_exeng` docstrings in `extraction.py`) now point at `EngineBase`, where they resolve. * `Engine.__init_subclass__`: qualify the bare `__engine_name__` reference, which stopped resolving once the attribute moved to `EngineBase`. * `PCAP.run`: repair the verbless "will also be save the current `PCAP` engine instance", as #1023 did in `extraction.py`. * Changelog entry added; the entry-count pin in `process.rst` is 157. Also corrected `docs/source/contributing/pep.rst`, which said subclassing an engine registers it automatically. Registration has been opt-in since #514: `__init_subclass__` registers only when the `engine` class keyword is given, and defaults to skipping. Measured -- a subclass without the keyword adds nothing to `Extractor.__engine__`; with it, the name appears. Nitpicky Sphinx build: 1239 unresolved references on fa3e861, 1237 here. The two `EngineBase.unsupported_reason` references in `docs/source/index.rst` newly resolve; no new miss. tests/project passes.

JarryShaw added fix Pull requests that fix a defect (fix: subject prefix) breaking Breaks public-facing behaviour or API (apply alongside the type label) review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 5, 2026

JarryShaw mentioned this pull request Oct 5, 2026

EngineBase is excluded from __all__ and documented :no-members:, so a public return type will expose no API #1024

Closed

JarryShaw force-pushed the fix/1022-engine-typed-slots branch from c194639 to 11cde1a Compare October 5, 2026 02:23

JarryShaw force-pushed the fix/1022-engine-typed-slots branch from 11cde1a to da431d1 Compare October 5, 2026 02:42

This was referenced Oct 5, 2026

fix(foundation): registrar error messages and type hints name the public class, not the Base the guard checks #1016

Closed

fix(foundation): raise RegistryError, not a leaked TypeError, for a non-class #1025

Merged

JarryShaw mentioned this pull request Oct 5, 2026

fix(foundation): name the base class the registrar guard actually checks #1020

Merged

5 of 12 tasks

JarryShaw added review: good-to-go Cross-review at the current head says ready; CI state is separate and removed review: pending No verdict for the current head - never reviewed, or the head moved since the last one labels Oct 5, 2026

JarryShaw merged commit 5fd0a1b into main Oct 5, 2026
132 of 134 checks passed

JarryShaw deleted the fix/1022-engine-typed-slots branch October 5, 2026 03:44

JarryShaw removed the review: good-to-go Cross-review at the current head says ready; CI state is separate label Oct 5, 2026

JarryShaw mentioned this pull request Oct 5, 2026

docs(foundation): document the engine members on EngineBase, not Engine (#1024) #1027

Merged

4 of 12 tasks

JarryShaw mentioned this pull request Oct 5, 2026

fix(foundation): warn once when an extraction engine is unavailable (#1045) #1046

Merged

4 of 12 tasks

JarryShaw added this to the 1.5 milestone Oct 6, 2026

Navigation Menu

Uh oh!

fix(foundation): declare the engine-typed slots as EngineBase, not Engine - #1023

fix(foundation): declare the engine-typed slots as EngineBase, not Engine#1023
JarryShaw merged 1 commit into
mainfrom
fix/1022-engine-typed-slots

JarryShaw commented Oct 5, 2026 •

edited

Loading

Uh oh!

JarryShaw commented Oct 5, 2026

Uh oh!

JarryShaw commented Oct 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

JarryShaw commented Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Please follow the guide below

What is the purpose of your pull request?

Description of your pull request and other information

Uh oh!

JarryShaw commented Oct 5, 2026

Uh oh!

JarryShaw commented Oct 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

JarryShaw commented Oct 5, 2026 •

edited

Loading