feat(foundation): make subclass registration opt-in for Engine, Reassembly, TraceFlow and Dumper (#514) - #547
Conversation
|
✅ GOOD TO MERGE -- confirmed by AST inspection that all four public classes ( |
Detailed review (independent verification, falsify-not-bless)Head sha reviewed: "No behaviour change beyond registration" -- confirmed exhaustively via AST, not by reading the diffWrote a small AST script rather than trusting the docstrings: for each of Zero in-tree descendants of the four public classes -- re-derived independentlyWalked Opt-in behaviour, constructed both directions for all four familiesFor each family, defined a subclass with the registration keyword and one without it in the same session, then diffed the registry before/after:
Unrecognised-keyword and
|
…embly, TraceFlow and Dumper (#514) * invert the fallback into a guard in all four `__init_subclass__` hooks: each registers if and only if its registry keyword is given (`name=` for Engine, `protocol=` for Reassembly and TraceFlow, `fmt=` for Dumper). Previously an absent keyword fell back to `cls.name`, so every subclass of the public class was registered and declining meant inheriting the `*Base` class under an alias. * reject unrecognised class keywords with `UnsupportedCall` instead of letting `**kwargs` swallow them, and reject Dumper's `ext=` without `fmt=` likewise. Both previously registered the class under a key nobody chose, or now would silently skip registration. * add a class-level `registry` property to `EngineMeta`, `ReassemblyMeta` and `TraceFlowMeta`, mirroring `EnumMeta.registry`. Dumper gets none: it writes two registries, so one `registry` name would be ambiguous. * a Dumper subclass no longer touches the filesystem while its `class` statement runs -- inferring `fmt` from `kind` meant instantiating the class against a `NamedTemporaryFile` mid-definition. * fix the two `ext.rst` examples that relied on the fallback and would otherwise have produced unregistered classes, the `name:`/`protocol:` docstring mismatches on Reassembly and TraceFlow, and Reassembly's class docstring, which read "Base flow tracing class". Unit tier 3.14: 1083 passed, 8 skipped, 2605 subtests, exit 0 read from a file; 3.10 foundation+dumpkit: 220 passed, 12 skipped, exit 0. pylint 25 messages to 20 with no new message ID; mypy unchanged at 2 pre-existing errors. Python 3.10 follow-up: `mcls`/`name`/`bases`/`namespace` collide with ABCMeta.__new__'s positional-or-keyword parameters before 3.11, so the two tests that used `name=` as their unrecognised keyword asserted against a metaclass TypeError. They now use a non-colliding keyword and run on every version, with the colliding case pinned per version rather than skipped; the engine test using `protocol=` loses its skip entirely. Engine's own keyword is `name`, so on 3.10 an engine must be registered via Extractor.register_engine -- documented in the docstring and the changelog rather than worked around.
0cf8c4f to
aea52cd
Compare
|
✅ GOOD TO MERGE (re-review of head |
Detailed re-review (independent verification, falsify-not-bless)Head sha reviewed: Separating the rebase pickup from the actual fixThe full Within 1. Is the documented workaround (
|
…works on Python 3.10 (#514) * `Engine.__init_subclass__`'s keyword is `engine` rather than `name`. `name` is one of four class keyword names -- `mcls`, `name`, `bases`, `namespace` -- that collide with `abc.ABCMeta.__new__`'s own parameters, which are positional-or-keyword before Python 3.11 and positional-only from 3.11. So on 3.10 `class MyEngine(Engine, name='x')` raised `TypeError` from the metaclass before the hook ran, making the documented registration path unusable there. Those four are the whole collision surface, measured; `engine`, `protocol` and `fmt` are all outside it. * no `name=` alias. A keyword that works on some interpreters and not others is the trap being removed, not a compatibility measure. * the `skipIf(sys.version_info < (3, 11))` on the inheritance test and the inline version branch in the opt-in test both come out, since neither needed a guard for any reason other than requiring `name=` at class-creation time. The engine tests now run in full on every supported version. * `name=` is an unrecognised keyword from here on, and its exception type is version-dependent -- `UnsupportedCall` from 3.11, `TypeError` from the metaclass on 3.10 -- so it is pinned per version rather than skipped. * corrects the #547 changelog entry, which named the old keyword, and `docs/source/ext.rst`'s engine example, which showed the crashing form. Unit tier 3.14: 1104 passed, 8 skipped, 2660 subtests, exit 0 read from a file. 3.10.21: engine + extraction + changelog tests 142 passed, 21 skipped, exit 0. Reverting the rename fails 3 tests on 3.14 and 2 on 3.10, exit 1 on both. mypy clean; pylint unchanged at its 3 pre-existing messages for this file.
…works on Python 3.10 (#514) (#557) * `Engine.__init_subclass__`'s keyword is `engine` rather than `name`. `name` is one of four class keyword names -- `mcls`, `name`, `bases`, `namespace` -- that collide with `abc.ABCMeta.__new__`'s own parameters, which are positional-or-keyword before Python 3.11 and positional-only from 3.11. So on 3.10 `class MyEngine(Engine, name='x')` raised `TypeError` from the metaclass before the hook ran, making the documented registration path unusable there. Those four are the whole collision surface, measured; `engine`, `protocol` and `fmt` are all outside it. * no `name=` alias. A keyword that works on some interpreters and not others is the trap being removed, not a compatibility measure. * the `skipIf(sys.version_info < (3, 11))` on the inheritance test and the inline version branch in the opt-in test both come out, since neither needed a guard for any reason other than requiring `name=` at class-creation time. The engine tests now run in full on every supported version. * `name=` is an unrecognised keyword from here on, and its exception type is version-dependent -- `UnsupportedCall` from 3.11, `TypeError` from the metaclass on 3.10 -- so it is pinned per version rather than skipped. * corrects the #547 changelog entry, which named the old keyword, and `docs/source/ext.rst`'s engine example, which showed the crashing form. Unit tier 3.14: 1104 passed, 8 skipped, 2660 subtests, exit 0 read from a file. 3.10.21: engine + extraction + changelog tests 142 passed, 21 skipped, exit 0. Reverting the rename fails 3 tests on 3.14 and 2 on 3.10, exit 1 on both. mypy clean; pylint unchanged at its 3 pre-existing messages for this file.
…or Protocol/ProtocolBase (#514) (#570) * add a `code=` class keyword to `ProtocolBase`/`Protocol.__init_subclass__` that registers a subclass into the next-layer dispatch registry (or registries) it names -- `Link.__proto__`, `Internet.__proto__`, `TCP.__proto__`, `UDP.__proto__`, `SCTP.__proto__`, `Frame.__proto__` and `PCAPNG.__proto__`. Omitting `code` leaves the class unregistered, exactly as before this keyword existed: the built-in tables are still populated by literal assignment in each layer module, not by this hook, so nothing the library ships moves. * add `register_protocol_code` in `foundation/registry/protocols.py`, backed by a small enum-type -> destination table: `EtherType` infers `Link`, `TransType` infers `Internet`, `PayloadProtocolIdentifier` infers `SCTP`, and `LinkType` infers *both* `Frame` and `PCAPNG` -- mirroring the fan-out `register_linktype` already does by hand. A raw `int` (e.g. a TCP/UDP port) has no inferable type and must name its destination explicitly via a `{destination: key}` mapping; either form may appear in an iterable to register one class into several registries from a single declaration. The explicit form is accepted even for a key whose type could be inferred. Inference refuses rather than guesses: an enum type absent from the table raises `RegistryError` instead of silently doing nothing or picking an arbitrary registry. * reject unrecognised class keywords with `UnsupportedCall`, matching the guard #547 added to `Engine`/`Reassembly`/`TraceFlow`/`Dumper`. * mirror the new name into the `__all__` lists of `pcapkit.foundation.registry`, `pcapkit.foundation` and `pcapkit.all`, which each re-declare every registry function by hand. * document the keyword in `docs/source/ext.rst`'s "New Protocol" example and in `docs/source/changelog/1.5.0.rst` (regenerating `CHANGELOG.md`). Verified backward compatible by measurement: `Protocol`/`ProtocolBase` descendant counts (0/43) and the byte contents of all seven `__proto__` tables are unchanged before and after, since no built-in class passes `code=`. New unit tests in `tests/protocols/test_protocol_code_registration_unit.py` (19 cases) proven to fail without this change (18 of 19 fail on the pre-fix tree, the remaining one being a before/after invariant that must hold both ways) and pass with it. Full unit tier: 1279 passed, 17 skipped, 2850 subtests, exit 0. mypy unchanged at its 4 pre-existing errors.
…nd register_extractor_engine's keyword (#577) Two gaps the #514 keyword audit turned up while checking whether an abandoned local change was still needed. Neither had a test before, and both turned out to already be correct on main -- just unguarded. - StreamEOFError's docstring did not say that @prepare always raises it with quiet=True (the same end-of-stream convention StructError follows via its own eof=True), and nothing pinned that silence. Document it, and add a test proving @prepare's StreamEOFError logs nothing, with a loud control proving the silence comes from quiet=True and not from StreamEOFError having stopped logging altogether. - register_extractor_engine's real keyword is `name`, not `engine`; an earlier audit (noted on #557) found it documented the other way around. The docstring was already fixed, but the keyword itself was never under test, so a future rename could put docstring and signature back out of step exactly as quietly as before. Pin both directions: `name=` registers, `engine=` raises TypeError. The rest of the inherited local change -- docstring edits to Engine/Reassembly/TraceFlow's __init_subclass__ and two tests asserting that an unrecognised `name=` class keyword is silently swallowed -- is superseded by #547/#557, which made registration opt-in and rejects an unrecognised class keyword with UnsupportedCall instead of swallowing it. Confirmed by rebase conflict (the Args: text they edited no longer exists) and by running the swallowed-keyword tests against current main, where they fail because the keyword is now rejected loudly rather than ignored. Build: full unit tier (pytest -q --ignore=tests/integration --ignore-glob='*_runtime.py' --ignore-glob='*_regression.py'): 1123 passed, 5 skipped, 2704 subtests passed, exit 0. Both new tests proven to fail without their fix.
First of the separate PRs agreed on #514. Design and the owner's answers to nine questions are in that thread: design, answers 1-7, answers 6/8/9.
Scope, and what is deliberately not here
Per Q7 — "separate ones so that we can review individually and the size of PRs can be reasonable" — this is the four families whose
__init_subclass__already takes a registry-key keyword:Engine(name=),Reassembly(protocol=),TraceFlow(protocol=) andDumper(fmt=). Q1 asked for all five includingDumper;Dumperis here.Protocolis held back deliberately. It has no registry-key keyword today — its key is the class name, derived insideregister_protocol— so making it opt-in means inventing a keyword, and what that keyword should mean is the subject of Q3's enum-type inference. It belongs in the registry PR rather than doubling this one.Also not here, and each its own follow-up: unwinding the ~76
*Base as <Public>alias imports (Q6 — and note the ordering is load-bearing, the guard has to land first, or a built-in inheritingProtocolunder today's fallback auto-registers under its own class name); the per-class registry ownership fix; andTransType.L2TPbeing registered nowhere.The change
Four
__init_subclass__hooks had the shapeand now guard instead, which is what
EnumSchema.__init_subclass__has done all along atschema.py:1044. Measured consequence, unchanged built-ins:In-tree blast radius is zero by construction: the public classes have 0 descendants each (against
EngineBase9,ReassemblyBase5,TraceFlowBase2,DumperBase3, re-measured on this branch), because every built-in inherits the*Baseclass to decline registration. Nothing the library ships went through the changed code path.Two silent failures made loud
Both are
UnsupportedCall, the in-library exception already used for API misuse of this kind (Transport.registerraises it forcls is Transport). No new exception class, which would have meant touchingexceptions.__all__— a public surface, and out of scope here.Enginespells the keyname=whileReassemblyandTraceFlowspell the same ideaprotocol=, so guessing wrong is the expected mistake. Verified onmain:class WrongKeyword(Reassembly, name='deliberately_wrong')registered under'wrongkeyword'— the wrong keyword swallowed by**kwargs, dropped by the baresuper().__init_subclass__(), and the class-name fallback firing. No exception, no warning, no log. With registration now opt-in the same typo would instead skip registration silently, which is quieter still. On Python 3.10 this holds for every keyword except the four that collide withABCMeta.__new__— see the 3.10 section below; those are loud either way, just asTypeErrorrather thanUnsupportedCall.Dumper'sext=withoutfmt=. With registration keyed onfmt, anextalone has nothing to attach to and would discard the caller's intent.registryon three metaclassesMirrors
EnumMeta.registry(schema.py:948-951). It has to live on the metaclass — apropertyin the class body would be an instance property, soReassembly.registrywould return the property object. Cheap, becauseEngineMetaandReassemblyMetaalready carry class-level properties (name,module,protocol).Two honest asymmetries, both documented in the docstrings and the
.rst: it is not per-class (every registration lands in the oneExtractortable, so any subclass returns the same object), and it is class-only, whereEnumSchemacarries tworegistryproperties and so also answers on an instance.Dumpergets none — it writes two registries (Extractor.__output__andTraceFlow.__output__), so a singleregistryname would be ambiguous, andDumperBasehas no pcapkit metaclass to hang it on.A class statement no longer touches the filesystem
Dumper.__init_subclass__used to inferfmtfrom the subclass'kindproperty.kindis an instance property, so it built one against atempfile.NamedTemporaryFilewhile theclassstatement was still executing. Guarding onfmtremoves that path;tempfileis no longer imported. The test assertsNamedTemporaryFileis not called, so the path is gone rather than merely unused.The
finalsubstitute the*Basehierarchy was standing in forWorth calling out because it answers the concern raised in Q6. The owner's reason for the
*Baseinheritance was to stop a built-in's own subclasses being registered again — a substitute for afinalmarker. That works, but it is a property of which base you inherited, so it cannot vary per subclass: a class is either registering-and-closed or non-registering-and-open, never both.Keying on the keyword gives both at once. Measured on this branch:
Under the old fallback the middle line registered itself as
'derived'. Pinned astest_registration_is_not_inherited_by_a_subclass.Separately measured, and the reason nothing needs revisiting urgently: subclassing
HTTP,L2TP,IPor any foundation built-in registers nothing today, because all of them descend from a*Baseclass and have no registering hook in their MRO.Documentation
docs/source/ext.rsthad two worked examples that relied on the fallback and would silently have produced unregistered classes:class MyScapy(Engine['Packet'])with__engine_name__ = 'Scapy'and noname=, andclass MyReassembly(Reassembly[...])with__protocol_name__and noprotocol=. Both now pass the keyword, with a note that the class attribute is not an opt-in — it sets the name a class reports about itself, registered or not.Also fixed: the
name:/protocol:Args:mismatch onReassembly.__init_subclass__andTraceFlow.__init_subclass__(the real parameter isprotocol),Reassembly's class docstring which read "Base flow tracing class" — copied fromTraceFlow— andDumper.__init_subclass__'s reference topcapkit.foundation.extraction.Extraction, a class that does not exist. None of these is inKNOWN_DEFECTSintests/test_docstring_contract.py, so no known-defect entry is invalidated.Fails without the fix
Reverting only the four
pcapkit/files (git apply -Rof the production half) and keeping the tests:Every one of the 13 new-or-changed tests fails, across all four families: the opt-in assertion, the unrecognised-keyword rejection, the
registryproperty, the non-inherited registration, andext-without-fmt.Read from a file, not a pipe.
echo $? > …thencat, because pytest 9.1.1's native subtests print a failing subtest's parent asPASSEDand a wrapper's exit code is not pytest's. That is not hypothetical here: the background runner reported "exit code 0" for the run whose own exit file said 1.Suite, lint, types
CI's unit-tier selection, verbatim from
unit-tests.yml:origin/main→ 20 here, and the difference is exactly the fiveC0415 import-outside-toplevelnow carrying the inline# pylint: disablethe codebase already uses for deferred imports. Every other message ID has an identical count —E10031,R04011,R08012,W022310,W04041,W06221,W11134 — so no new message of any kind, and no new cyclic import fromengine.py's new runtime import ofpcapkit.utilities.exceptions.registryagainst the*Baseclasses and produced threereturn-valueerrors, becauseExtractor.__engine__and siblings are declared against the public class. The annotations now match whatExtractordeclares. Worth noting that those declarations are the narrower-than-reality kind — the tables really hold*Basesubclasses — which is the same imprecision as the elevenRegistryErrorstrings named in the design comment; correcting it belongs with the alias work.Python 3.10, and a correction to an earlier claim in this description
The first revision of this PR failed the
Python 3.10andIntegration Python 3.10jobs, and the explanation I gave for the version guards was wrong. Corrected, and now measured on three interpreters rather than assumed:So on 3.10 exactly four class keyword names —
mcls,name,bases,namespace— collide withABCMeta.__new__'s own parameters and raiseTypeErrorbefore__init_subclass__is reached. From 3.11 they are positional-only and arrive at the hook normally. It has nothing to do withEngineMetabeingGeneric: measured, aGenericmetaclass takes a non-colliding class keyword perfectly well on 3.10. That earlier claim was mine and it was unfounded.Which reading: both, and the split matters
Test problem, fixed here. Using
name=as the "unrecognised keyword" in theReassemblyandTraceFlowtests was a pathological pick — it is one of the four colliding names, so those tests were assertingUnsupportedCallagainst aTypeErrorthe metaclass raised first. They now use a non-colliding keyword and run on every supported version instead of being skipped. The engine test that usesprotocol=has had itsskipIfremoved for the same reason — verified reaching the guard on 3.10 against this tree.Feature limitation, documented not fixed.
Engine's registry keyword isname. So on Python 3.10,class MyEngine(Engine, name='my_engine')— the documented registration path — raisesTypeError. Verified against this tree on 3.10.21, not inferred:This predates the PR, but the PR makes it load-bearing: before, the
cls.namefallback registered the engine anyway, so 3.10 worked by accident. Now the keyword is the only class-definition path, so on 3.10 an engine must be registered withExtractor.register_engine, which works on every version. Recorded in theEngine.__init_subclass__docstring as aWarning:, in the changelog entry, and in the one remainingskipIf's reason.Not fixable from
__init_subclass__, because the collision happens in the metaclass before the hook runs. Fixing it means either renamingEngine's keyword or intercepting in a metaclass__new__— both API decisions rather than test hygiene, so I have raised it on Design: adopt the EnumMeta/EnumSchema opt-in registration pattern for Protocol, Engine, Reassembly and TraceFlow #514 rather than deciding it here.Verified on 3.10 locally this time
I said in the first revision that I could not execute a pre-3.11 interpreter. That was wrong — I had not looked hard enough;
python3.10is present via mise, and a throwaway venv with the four runtime deps runs the suite. Results, exit codes read from files:tests/foundation+tests/dumpkit, CI's ignore globsorigin/mainSkips went from three to one; the one that remains is the test that must pass
name=to make its point.One process note worth recording, because it nearly produced a false green: my first attempt at the fails-without proof reverted
git diff -- pcapkit, which after the rebase captured only the uncommitted docstring edits — the guard being already in the commit. It reported "0 failed", which for a proof that things should fail is the obvious tell. Redone againstgit diff origin/main -- pcapkit, which is the real delta.