Repository navigation
schema: give every Schema subclass its own ABCMeta cache, and delete the version guard at its cause - #471
Conversation
…stance corruption (#439) SchemaMeta.__new__ used to branch on sys.version_info < (3, 11) and call type.__new__ directly on old Pythons, bypassing abc.ABCMeta.__new__ (and its call to abc._abc_init) to dodge a class-keyword collision: several Option subclasses in pcapkit/protocols/schema/misc/pcapng.py declared a namespace= class keyword, which bound abc.ABCMeta.__new__'s own namespace parameter twice on Python < 3.11 (that parameter became positional-only only in 3.11). Bypassing ABCMeta.__new__ meant no Schema subclass ever got its own _abc_impl; every one of them fell through the MRO to collections.abc. Mapping's, so any isinstance(x, Mapping) or isinstance(x, Schema) question about a plain dict poisoned the other's answer for the rest of the process, in both directions. NameResolutionBlock.post_process (pcapng.py:1248) is the real-world casualty: a terminating EndRecord tested True as an IPv4Record/ IPv6Record and AttributeError'd on record.names. - Rename the colliding keyword: Option subclasses now spell it ns= instead of namespace= (pcapkit/protocols/schema/misc/pcapng.py). The stored attribute __namespace__ is unchanged. This is a public API change to Option's subclassing convention; nothing outside pcapkit/ was found using the old spelling. - Delete the version guard: SchemaMeta.__new__ now calls super().__new__(...) unconditionally on every supported version. - Add a guard that raises SchemaError, naming the offending keyword, when a Schema subclass is declared with a class keyword that collides with abc.ABCMeta.__new__ (mcls/name/bases/namespace) or with the implicit __init_subclass__ classmethod binding (cls) -- so the next accidental collision fails loudly here instead of several frames away. cls/name/bases are also now positional-only in SchemaMeta.__new__'s own signature, matching the fix CPython gave ABCMeta.__new__ in 3.11. - tests/protocols/test_option_roundtrip_unit.py: delete INTERPRETER_GAPS, SCHEMA_ABC_IS_PER_CLASS and test_schema_isinstance_is_interpreter_dependent, which existed to record and pin this exact defect on Python 3.10; fixing #439 is what makes them obsolete. - New regression tests in tests/protocols/schema/test_schema_metaclass_abc_cache_unit.py, covering both corruption directions, the exact-type cache keying that makes probing order matter, the real pcapng.py:1248 symptom, and the new reserved-keyword guard. Build: brazil-build equivalent (pip install -e '.[test,Scapy]') green on 3.10.21 and 3.14.7; mypy clean of new errors on 3.10.21.
What I read
1. The reserved-keyword setVerified the two collision mechanisms independently, not by re-deriving the author's table. Mechanism A — This exactly reproduces the PR's claimed table ( Mechanism B — Confirmed on every supported interpreter, independent of the ABCMeta fix — the PR's claim that Completeness / over-breadth. Grepped the whole tree for
2. Both directions of the cache corruptionExtracted Against the fixed PR code (checked out Against pre-fix The 2 that still pass on broken code are The 7 that fail all fail for exactly the reason their docstrings claim:
Both directions are genuinely pinned, and every reproducer that claims to need exact-type dict actually uses 3. Test cleanup in
|
The owner asked for the dict subclass gone in favour of ChainMap, which is where this design started before #439 was (wrongly) suspected of being disturbed by it. Two premises settled first: - The fallback container is still required: #471 (fixing #439 directly) does not touch pcapkit/corekit/fields/misc.py at all, and #445's own KeyError symptom persists on #471's tree without this fix. Different defects. - ChainMap never poisoned anything. The shared ABCMeta cache keys on the exact type queried: asking about a ChainMap instance caches ChainMap, not dict, and isinstance({}, Schema) stayed False afterwards. The actual poisoner was ordinary code asking isinstance about a plain dict (Info.__update__), unrelated to what this function returns either way. #471 has also now given every Schema subclass its own _abc_impl, removing the mechanism regardless. nested_packet_context() now returns a bare collections.ChainMap({'__packet__': packet}, packet) -- no custom class. This gets, with no code: setdefault honouring the fallback (the dict subclass's own setdefault bypassed __missing__ and would insert a name locally instead of returning the parent's value -- caught directly, see the test below), Mapping's __eq__ instead of dict's, and dict(**pkt)/.copy() working correctly out of the box. Only one thing needed a local fix rather than a class: ChainMap is not nominally a dict, so `value.pack(nested_packet_context(packet))` no longer satisfies Schema.pack's `dict[str, Any]` annotation. Widening that annotation cascades into every other field class's pack/unpack, which forward the same packet argument with their own dict[str, Any] annotations (measured: +7 new mypy errors). Used an explicit cast('dict[str, Any]', ...) at the one call site instead -- asserting that ChainMap satisfies every operation the annotation promises (subscript, in, .get(), iteration), which it does, rather than widening the annotation or reintroducing a fresh type: ignore. tests/corekit/test_fields_misc_packet_context.py's docstring already named collections.ChainMap (stale by coincidence during the dict detour, accurate again now) -- expanded it to say so deliberately, and added explicit setdefault/.copy() assertions to test_nested_schema_reads_enclosing_field_by_name_and_does_not_leak_writes, confirmed to fail under the retired dict subclass (setdefault returned 999, shadowing the parent's 3, instead of honouring it). Re-verified all ten EXPECTED_FAILURES entries deleted across the dict detour -- six httpv2-frame, four mh-extension -- individually via options.roundtrip(), not inferred from a green suite: all ten still return 'OK' against the ChainMap version. Merged origin/main (5182ad0, #471) first -- clean, no conflicts. Verified: mypy pcapkit -> 124 errors/40 files (a fresh main at 5182ad0: 125, one more from an unrelated pre-existing gap this branch already cleans up). Round-trip harness: 6 passed, 358 subtests, 0 failed. tests/protocols/misc/test_pcapng_unit.py + tests/protocols/internet/test_mh_unit.py + tests/protocols/schema/: 97 passed, 416 subtests, 0 failed.
JarryShaw
added
fix
1 participant
Closes #439.
SchemaMeta.__new__branched onsys.version_info < (3, 11)and returnedtype.__new__(...), bypassingabc.ABCMeta.__new__and so never calling_abc_init. The consequence was that noSchemasubclass ever got its own_abc_impl— they sharedcollections.abc.Mapping's, sinceSchemainheritsMapping.isinstance/issubclassagainst any schema class then answered froma cache keyed on whatever had been asked first.
The code comment admitted it never understood why:
Why the guard existed
abc.ABCMeta.__new__names its fourth parameternamespace, and that parameteris positional-or-keyword — not positional-only — before Python 3.11:
Seven
Optionsubclasses inpcapkit/protocols/schema/misc/pcapng.pyweredeclared with a class keyword spelled
namespace=, which bound that parametertwice on 3.10 and earlier:
That name collision — not anything about
ABCMeta— is what the version guarddodged, and the guard's
< (3, 11)boundary is exactly where CPython added the/.The fix
SchemaMeta.__new__is now an unconditionalsuper().__new__(cls, name, bases, attrs, **kwargs), with the guard and the NOTE both deleted, and its ownname/bases/attrsmade positional-only — the same fix CPython applied toABCMeta.__new__in 3.11, which removes the collision structurally rather thanguarding it.
API change.
Optionsubclasses takens=instead ofnamespace=:class Foo(Option, ns='opt')rather thanclass Foo(Option, namespace='opt').The stored attribute
Option.__namespace__is unchanged, andcode=— the otherclass keyword this hierarchy uses — is unaffected. A repository-wide,
multi-line-aware search found nothing outside this repo's own seven subclasses
plus one test fixture using the old spelling; both are updated here.
And a guard so it cannot silently return
Because the collision class is not specific to
namespace,SchemaMeta.__new__now raises
SchemaErrorif a class keyword collides, naming the offender._RESERVED_CLASS_KWARGSis{'mcls', 'name', 'bases', 'namespace', 'cls'}, andthe five are not all there for the same reason — measured with a metaclass whose
own parameters are positional-only, as
SchemaMeta's now are:So
mcls/name/bases/namespacecollide only whereABCMeta's parameters arenot positional-only, while
clscollides on every version through theimplicit
__init_subclass__classmethod binding — a different mechanism, andindependent of
ABCMetaentirely. Covering all five is what makes thislong-standing rather than merely correct today.
SchemaErrorhad no caller beforethis; it now has its first.
The guard earned its place immediately
A single-line grep sweep for the rename missed
tests/protocols/misc/test_pcapng_unit.py, where the declaration is split acrossthree lines (
[^)]*cannot span newlines). It surfaced as the one failure in anotherwise-clean 3.10 suite run — as
SchemaError, exactly as designed — ratherthan as a silently lost namespace. Fixed, and re-swept with a
re.DOTALLpatternacross
pcapkit/,tests/andexamples/afterwards.Required test cleanup
tests/protocols/test_option_roundtrip_unit.pyinstructed this fix to clean upafter itself —
:43"fixing #439 turns 3.10 red",:539"which is the point",and the guard test's own message "If #439 is fixed, delete that table and this
test." So
INTERPRETER_GAPS,SCHEMA_ABC_IS_PER_CLASS,test_schema_isinstance_is_interpreter_dependent, its_abc_implhelper, theinterpreter-dependence docstring section and the table's consistency-check
references are all gone. The seven PCAP-NG name-resolution cases it held are now
held to
'OK'by the main table's absence of an entry. Themh-extension,httpv2-frameandhip-parameterentries were not touched.One existing test in
tests/protocols/schema/test_schema_unit.pymockedsys.version_infoto exercise the deleted branch; it is narrowed and renamed,with a docstring saying why.
The real-world symptom
NameResolutionBlock.post_processasksisinstance(record, (IPv4Record, IPv6Record))atpcapkit/protocols/schema/misc/pcapng.py:1248and then readsrecord.names. On the broken code that returnedTruefor the block'sterminating
EndRecord, so it readnamesoff a record that has none — and everyNRB carries a terminating
EndRecord. Reproduced with the twoisinstancequestions real code actually asks, in order, with no
dict/Mappinginvolvementneeded:
Truebefore,Falseafter. Pinned as a regression test.Verification
tests/protocols/schema/test_schema_metaclass_abc_cache_unit.py, 9 tests,checked against the pre-fix source so each fails there in the way its docstring
predicts. The cache tests pin both directions of the corruption: asking
Mappingfirst madeisinstance({}, Schema)returnTrue; askingSchemafirst made
isinstance({}, Mapping)returnFalse.git merge-treeclean against both contested branches —fix-445-nested-packet-contextand
fix-463-hip-list-length-underflow— with noCONFLICTlines.subtests; 3.14.7 at 939 passed with one failure traced to a missing
typing_extensionsin the ad-hoc venv, confirmed by installing it and rerunningthat test alone.
are the return-value mismatch on the old
type.__new__branch and theunreachable/unused-ignore pair the version guard produced.
Findings not folded in
mypy 2.3.1 raises an internal error on this codebase when invoked under Python
3.14, reproducible on pre-fix code too — a pre-existing tooling gap, unrelated,
and there is no mypy job in
.github/workflows/that it affects.Correction to this description
The API-change paragraph says the search "found nothing outside this repo's own
seven subclasses plus one test fixture" — that undercounts by one. The review
ran its own paren-depth-tracking multi-line sweep and found nine sites
carrying the old spelling: the seven
Optionsubclass declarations, thedocstring example in
pcapkit/protocols/schema/misc/pcapng.py, and thethree-line class declaration in
tests/protocols/misc/test_pcapng_unit.py. Thedocstring example was omitted from the count above.
The code is unaffected — all nine were updated, the post-fix sweep finds zero
remaining, and the ~35 unrelated
EnumField/OptionEnumField/BitFieldnamespace=constructor parameters were left untouched. Only the prose count waswrong.