From 6b5505fb2bc34e4e97c76a9a0c1d4f18271f272e Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Fri, 18 Sep 2026 00:56:48 -0400 Subject: [PATCH] schema: give every Schema subclass its own ABCMeta cache, fixing isinstance 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. --- pcapkit/protocols/schema/misc/pcapng.py | 39 +- pcapkit/protocols/schema/schema.py | 83 ++++- tests/protocols/misc/test_pcapng_unit.py | 2 +- .../test_schema_metaclass_abc_cache_unit.py | 352 ++++++++++++++++++ tests/protocols/schema/test_schema_unit.py | 20 +- tests/protocols/test_option_roundtrip_unit.py | 185 +-------- 6 files changed, 468 insertions(+), 213 deletions(-) create mode 100644 tests/protocols/schema/test_schema_metaclass_abc_cache_unit.py diff --git a/pcapkit/protocols/schema/misc/pcapng.py b/pcapkit/protocols/schema/misc/pcapng.py index e3a0bedd64..07ba9486c5 100644 --- a/pcapkit/protocols/schema/misc/pcapng.py +++ b/pcapkit/protocols/schema/misc/pcapng.py @@ -399,14 +399,19 @@ class Option(EnumSchema[Enum_OptionType]): ) def __init_subclass__(cls, /, code: 'Optional[Enum_OptionType | Iterable[Enum_OptionType]]' = None, - namespace: 'Optional[str]' = None, *args: 'Any', **kwargs: 'Any') -> 'None': + ns: 'Optional[str]' = None, *args: 'Any', **kwargs: 'Any') -> 'None': """Register option type to :attr:`__enum__` mapping. Args: code: Option type code. It can be either a single option type enumeration or a list of option type enumerations. - namespace: Namespace of option type enumeration. If not given, the value - will be inferred from the option type code. + ns: Namespace of option type enumeration. If not given, the value + will be inferred from the option type code. Spelled ``ns`` rather + than ``namespace`` because :meth:`abc.ABCMeta.__new__` names its + own fourth parameter ``namespace``, and before Python 3.11 that + parameter is positional-or-keyword rather than positional-only -- + so a class keyword literally called ``namespace`` bound it twice. + See GitHub issue #439. *args: Arbitrary positional arguments. **kwargs: Arbitrary keyword arguments. @@ -421,25 +426,25 @@ def __init_subclass__(cls, /, code: 'Optional[Enum_OptionType | Iterable[Enum_Op from pcapkit.const.pcapng.option_type import OptionType as Enum_OptionType from pcapkit.protocols.schema.misc.pcapng improt Option - class NewOption(Option, namespace='opt', code=Enum_OptionType.opt_new): + class NewOption(Option, ns='opt', code=Enum_OptionType.opt_new): ... See Also: - :class:`pcapkit.const.pcapng.option_type.OptionType` """ - if namespace is not None: - cls.__namespace__ = namespace + if ns is not None: + cls.__namespace__ = ns if code is not None: - if namespace is None: - namespace = cast('Optional[str]', cls.__namespace__) + if ns is None: + ns = cast('Optional[str]', cls.__namespace__) if not isinstance(code, Enum_OptionType): for _code in code: - Option.register(_code, cls, namespace) + Option.register(_code, cls, ns) else: - Option.register(code, cls, namespace) + Option.register(code, cls, ns) super().__init_subclass__() @staticmethod @@ -472,7 +477,7 @@ def register(code: 'Enum_OptionType', cls: 'Type[Option]', ns: 'Optional[str]' = length: 'int' -class _OPT_Option(Option, namespace='opt'): +class _OPT_Option(Option, ns='opt'): """Header schema for ``opt_*`` options.""" #: Option type. @@ -623,7 +628,7 @@ def __init__(self, length: 'int', magic: 'Literal[0x1A2B3C4D]', major: 'int', length2: 'int') -> 'None': ... -class _IF_Option(Option, namespace='if'): +class _IF_Option(Option, ns='if'): """Header schema for ``if_*`` options.""" #: Option type. @@ -892,7 +897,7 @@ def __init__(self, length: 'int', linktype: 'int', snaplen: 'int', options: 'list[Option | bytes] | bytes', length2: 'int') -> 'None': ... -class _EPB_Option(Option, namespace='epb'): +class _EPB_Option(Option, ns='epb'): """Header schema for ``epb_*`` options.""" #: Option type. @@ -1159,7 +1164,7 @@ def post_process(self, packet: 'dict[str, Any]') -> 'Schema': def __init__(self, type: 'Enum_RecordType', length: 'int', ip: 'IPv6Address | str | bytes | int', resol: 'str') -> 'None': ... -class _NS_Option(Option, namespace='ns'): +class _NS_Option(Option, ns='ns'): """Header schema for ``ns_*`` options.""" #: Option type. @@ -1266,7 +1271,7 @@ def __init__(self, length: 'int', options: 'list[Option | bytes] | bytes', length2: 'int') -> 'None': ... -class _ISB_Option(Option, namespace='isb'): +class _ISB_Option(Option, ns='isb'): """Header schema for ``isb_*`` options.""" #: Option type. @@ -1570,7 +1575,7 @@ class ZigBeeAPSKey(DSBSecrets, code=Enum_SecretsType.ZigBee_APS_Key): def __init__(self, key: 'bytes', panid: 'int', addr_low: 'int', addr_high: 'int') -> 'None': ... -class _DSB_Option(Option, namespace='dsb'): +class _DSB_Option(Option, ns='dsb'): """Header schema for ``dsb_*`` options.""" #: Option type. @@ -1643,7 +1648,7 @@ class CustomBlock(BlockType, code=[Enum_BlockType.Custom_Block_that_rewriters_ca def __init__(self, length: 'int', pen: 'int', data: 'bytes', length2: 'int') -> 'None': ... -class _PACK_Option(Option, namespace='pack'): +class _PACK_Option(Option, ns='pack'): """Header schema for ``pack_*`` options.""" #: Option type. diff --git a/pcapkit/protocols/schema/schema.py b/pcapkit/protocols/schema/schema.py index 9c0546c67f..1cab73401e 100644 --- a/pcapkit/protocols/schema/schema.py +++ b/pcapkit/protocols/schema/schema.py @@ -6,7 +6,6 @@ import collections.abc import io import itertools -import sys from typing import TYPE_CHECKING, Any, Generic, TypeVar, cast, final from pcapkit.corekit.fields.collections import ListField, OptionField @@ -16,7 +15,7 @@ from pcapkit.corekit.infoclass import FinalisedState from pcapkit.utilities.compat import Mapping from pcapkit.utilities.decorators import prepare -from pcapkit.utilities.exceptions import NoDefaultValue, ProtocolUnbound, stacklevel +from pcapkit.utilities.exceptions import NoDefaultValue, ProtocolUnbound, SchemaError, stacklevel from pcapkit.utilities.warnings import SchemaWarning, UnknownFieldWarning, warn if TYPE_CHECKING: @@ -210,7 +209,55 @@ def __prepare__(cls, name: 'str', bases: 'tuple[type, ...]', /, **kwds: 'Any') - fields.update(base.__fields__) return collections.OrderedDict(__fields__=fields) - def __new__(cls, name: 'str', bases: 'tuple[type, ...]', attrs: 'dict[str, Any]', **kwargs: 'Any') -> 'Type[Schema]': + #: Class keywords that collide with a parameter this class does not + #: control, so a schema class declared with one fails with an opaque + #: ``TypeError`` from several frames away instead of a clear message here. + #: Two different collisions, both reserved: + #: + #: ``mcls``, ``name``, ``bases``, ``namespace`` collide with + #: :meth:`abc.ABCMeta.__new__` -- a *different* function from this one, one + #: level up the ``super().__new__(...)`` call below. Before Python 3.11 + #: those four are positional-or-keyword there (from 3.11 they are + #: positional-only, ``def __new__(mcls, name, bases, namespace, /, + #: **kwargs)``), so a class keyword spelled the same as any of them binds + #: that parameter twice: ``TypeError: ABCMeta.__new__() got multiple + #: values for argument '...'``. That is GitHub issue #439's root cause -- + #: ``namespace`` collided this way, which is why + #: :mod:`pcapkit.protocols.schema.misc.pcapng`'s ``Option`` subclasses + #: spell it ``ns=`` instead. + #: + #: ``cls`` collides one level *later*: every ``__init_subclass__`` is an + #: implicit classmethod, so ``cls`` is always bound as its first argument, + #: and a class keyword also spelled ``cls`` binds it twice there -- + #: ``TypeError: Generic.__init_subclass__() got multiple values for + #: argument 'cls'`` for a :class:`Schema` subclass (:class:`Schema` + #: inherits :class:`typing.Generic`), or the equivalent from whichever + #: class in the MRO defines ``__init_subclass__`` first. This one is not + #: specific to ``abc.ABCMeta`` or to this metaclass at all -- it holds for + #: *any* Python class with any ``__init_subclass__`` in its MRO -- but it + #: is reserved here anyway since it is exactly the class of mistake this + #: guard exists to catch early and legibly. + #: + #: Listed explicitly rather than derived from + #: :func:`inspect.signature(abc.ABCMeta.__new__) ` at + #: import time: the signature's positional-only marker differs across + #: Python versions, introspecting a CPython internal's exact shape to + #: guard against a CPython internal's exact shape is circular, and five + #: names that will not change are cheaper to read than the machinery to + #: recompute them. + #: + #: ``name``, ``bases`` and ``attrs`` -- *this* method's own parameters -- + #: collide the same way one level earlier than ``ABCMeta.__new__``, on + #: every Python version, and are not in this set for that reason alone: + #: they are made positional-only below instead (the same fix CPython gave + #: ``ABCMeta.__new__`` in 3.11), which removes the collision rather than + #: merely naming it. ``name`` and ``bases`` stay in this set regardless, + #: because they still collide with ``ABCMeta.__new__``'s parameters of the + #: same name one level up; ``attrs`` does not appear anywhere downstream + #: and so needs no entry once it is positional-only here. + _RESERVED_CLASS_KWARGS = frozenset({'mcls', 'name', 'bases', 'namespace', 'cls'}) + + def __new__(cls, name: 'str', bases: 'tuple[type, ...]', attrs: 'dict[str, Any]', /, **kwargs: 'Any') -> 'Type[Schema]': """Create the schema class. Args: @@ -223,7 +270,22 @@ def __new__(cls, name: 'str', bases: 'tuple[type, ...]', attrs: 'dict[str, Any]' :attr:`~Schema.__excluded__` fields from the base classes, as well as populating both fields from the subclass attributes. + Raises: + SchemaError: If a class keyword in ``**kwargs`` collides with a + parameter of :meth:`abc.ABCMeta.__new__` or of + ``__init_subclass__`` -- see :attr:`_RESERVED_CLASS_KWARGS`. + """ + if clash := cls._RESERVED_CLASS_KWARGS.intersection(kwargs): + raise SchemaError( + f'{name}: class keyword(s) {sorted(clash)!r} are reserved -- ' + f'each collides with a same-named parameter of either ' + f'abc.ABCMeta.__new__ or the implicit __init_subclass__ ' + f'classmethod binding, and cannot be used as a class keyword ' + f'on a Schema subclass; rename to a different spelling (see ' + f'GitHub issue #439)' + ) + if '__additional__' not in attrs: attrs['__additional__'] = [] if '__excluded__' not in attrs: @@ -235,10 +297,17 @@ def __new__(cls, name: 'str', bases: 'tuple[type, ...]', attrs: 'dict[str, Any]' if hasattr(base, '__excluded__'): attrs['__excluded__'].extend(name for name in base.__excluded__ if name not in attrs['__excluded__']) - # NOTE: for unknown reason, the following code will cause an error - # for duplicated keyword arguments in class definition. - if sys.version_info < (3, 11): - return type.__new__(cls, name, bases, attrs, **kwargs) + # See #439: this used to branch on ``sys.version_info < (3, 11)`` and + # call ``type.__new__`` directly below that, to dodge the ``namespace`` + # collision described above. That branch skipped ``ABCMeta.__new__``'s + # call to ``abc._abc_init(cls)``, so no :class:`Schema` subclass ever + # got its own ``_abc_impl``, and every one of them fell through the + # MRO to :class:`collections.abc.Mapping`'s -- corrupting + # ``isinstance`` against *any* of them for as long as the process ran. + # The actual fix was renaming the one colliding class keyword that was + # actually in use, which means this can now call ``ABCMeta.__new__`` + # unconditionally, on every supported version, like any other + # metaclass would. return super().__new__(cls, name, bases, attrs, **kwargs) # type: ignore[return-value] diff --git a/tests/protocols/misc/test_pcapng_unit.py b/tests/protocols/misc/test_pcapng_unit.py index 67352b8d71..d43a3f476a 100644 --- a/tests/protocols/misc/test_pcapng_unit.py +++ b/tests/protocols/misc/test_pcapng_unit.py @@ -1261,7 +1261,7 @@ class LocalDefaultOption(schema_pcapng.Option, code=OptionType.opt_comment): class LocalIterableOption(schema_pcapng.Option, code=[OptionType.if_name], - namespace='localtest'): + ns='localtest'): pass self.assertIs(registry['opt'][OptionType.opt_comment], LocalDefaultOption) diff --git a/tests/protocols/schema/test_schema_metaclass_abc_cache_unit.py b/tests/protocols/schema/test_schema_metaclass_abc_cache_unit.py new file mode 100644 index 0000000000..f7ae3c15ca --- /dev/null +++ b/tests/protocols/schema/test_schema_metaclass_abc_cache_unit.py @@ -0,0 +1,352 @@ +from __future__ import annotations + +import importlib.util +import unittest +from collections import ChainMap +from collections.abc import Mapping + +from tests._support import purge_modules + +RUNTIME_DEPS = ('tbtrim', 'aenum', 'chardet', 'dictdumper') +HAS_RUNTIME = all(importlib.util.find_spec(name) is not None for name in RUNTIME_DEPS) + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class SchemaMetaAbcCacheTests(unittest.TestCase): + """Regression tests for GitHub issue #439. + + ``SchemaMeta.__new__`` (:mod:`pcapkit.protocols.schema.schema`) used to bypass + :meth:`abc.ABCMeta.__new__` on Python < 3.11, to dodge a ``namespace=`` class + keyword collision described in that module. Bypassing it also skipped + :meth:`abc.ABCMeta.__new__`'s call to ``abc._abc_init(cls)``, so no + :class:`~pcapkit.protocols.schema.schema.Schema` subclass ever got its own + ``_abc_impl``. Attribute lookup then fell through the MRO to + :class:`collections.abc.Mapping`'s ``_abc_impl`` -- the one ABC in the chain + still built through an untouched ``ABCMeta.__new__`` -- so *every* schema + class was reading and writing the same ``isinstance`` cache that + ``collections.abc.Mapping`` itself uses. + + :class:`dict` is a registered virtual subclass of ``Mapping``, so asking + ``isinstance(x, Mapping)`` for a plain :class:`dict` caches a positive answer + keyed on ``dict`` -- and with the cache shared, ``isinstance(x, Schema)`` for + that same ``dict`` then reads the *same* cache entry straight back, without + ever running :class:`Schema`'s real subclass check. + + The corruption runs **both ways**, which is why two tests below ask the two + questions in opposite orders rather than one test asking them in the + "obvious" order. Asking ``Schema`` *first* computes a fresh (correct) + ``False`` for ``dict`` -- but caches it in the shared object's *negative* + cache, and since that object is ``Mapping``'s own, a plain ``dict`` then + stops answering ``isinstance(x, Mapping)`` at all, anywhere in the process, + until the cache token advances. Measured directly against the pre-fix code + (not merely inferred): asking ``isinstance({}, Schema)`` and then + ``isinstance({}, Mapping)`` returns ``False`` for the second question too. + This is the more dangerous direction, since it breaks code with no + connection to schemas at all -- ordinary library code such as + :meth:`Info.from_dict ` asking + ``isinstance(dict_, (dict, collections.abc.Mapping))`` is enough to trigger + either direction, which is why the defect only ever showed up in the full + suite and not in a single test file run alone. + + """ + + def setUp(self) -> None: + # Fresh :mod:`pcapkit` classes *and* a freshly reset + # :mod:`collections.abc` cache -- :func:`purge_modules` does both, and + # both matter: a stale positive cache entry left over from an earlier + # test in the same process would make the very first assertion below + # true for the wrong reason. + purge_modules(['pcapkit']) + + def tearDown(self) -> None: + # A couple of tests below deliberately poison collections.abc's shared + # caches (that is the point of them) -- reset them again on the way + # out, so nothing outside this module ever reads what was left behind. + purge_modules(['pcapkit']) + + def test_a_plain_dict_is_not_a_schema_after_mapping_is_asked_first(self) -> None: + """The order-sensitive reproduction from GitHub issue #439 itself. + + Ask ``isinstance({}, Mapping)`` *before* ``isinstance({}, Schema)`` -- + reversing that order is the single easiest way to make this test pass on + the broken code, so the ordering here is deliberate and load-bearing, + not incidental. + + """ + # Ask Mapping FIRST. On the broken code this is what poisons the shared + # cache; on the fixed code it is a completely ordinary, unrelated + # question that leaves Schema's own cache alone. + self.assertTrue(isinstance({}, Mapping), 'sanity: dict really is a Mapping') + + from pcapkit.protocols.schema.misc.pcapng import EndRecord, IPv4Record + from pcapkit.protocols.schema.schema import Schema + + # THEN ask Schema. A plain dict must not test True as a Schema, nor as + # any concrete Schema subclass, no matter what was asked about Mapping + # a moment ago. + self.assertFalse(isinstance({}, Schema)) + self.assertFalse(isinstance({}, EndRecord)) + self.assertFalse(isinstance({}, IPv4Record)) + + def test_asking_schema_first_does_not_break_mapping_for_a_plain_dict(self) -> None: + """The reverse direction: ``Schema`` first must not corrupt ``Mapping``. + + Before this class had its own ``_abc_impl``, asking ``isinstance({}, + Schema)`` computed a fresh (correct) ``False`` for ``dict`` and cached + it in the *negative* cache of the object it shared with + :class:`collections.abc.Mapping` -- after which a plain :class:`dict` + stopped answering ``isinstance(x, Mapping)`` at all. This is the + direction most likely to be missed: it has nothing to do with + ``Schema`` being asked about a dict that should not match it, and + everything to do with an *unrelated* stdlib question breaking as a + side effect. + + """ + from pcapkit.protocols.schema.schema import Schema + + # Ask Schema FIRST this time -- the opposite order from the test above. + self.assertFalse(isinstance({}, Schema)) + + # THEN ask Mapping. A plain dict must still be reported as one. + self.assertTrue(isinstance({}, Mapping)) + + def test_schema_subclasses_have_their_own_abc_impl(self) -> None: + """Every schema class gets its own ABCMeta registry/cache triple. + + This is the mechanism behind the test above, pinned directly: a + :class:`Schema` subclass's ``_abc_impl`` must not be the same object as + another subclass's, or as :class:`Schema`'s own -- and, since the + defect's actual shape was falling through to :class:`collections.abc. + Mapping`'s, not that one either. + + """ + from pcapkit.protocols.schema.misc.pcapng import EndRecord, IPv4Record + from pcapkit.protocols.schema.schema import Schema + + self.assertIsNotNone(Schema._abc_impl) + self.assertIsNot(EndRecord._abc_impl, Schema._abc_impl) + self.assertIsNot(IPv4Record._abc_impl, Schema._abc_impl) + self.assertIsNot(EndRecord._abc_impl, IPv4Record._abc_impl) + self.assertIsNot(Schema._abc_impl, Mapping._abc_impl) + + def test_a_real_instance_still_isinstance_of_its_own_class(self) -> None: + """The fix must not overcorrect into every schema object testing False. + + Giving each class its own ``_abc_impl`` must not disturb the ordinary + case: a real instance of a schema class is still that class, even right + after the same ``Mapping``-first probe used above. + + """ + self.assertTrue(isinstance({}, Mapping)) + + from pcapkit.protocols.schema.misc.pcapng import EndRecord + + # ``__new__`` rather than the normal constructor, matching how this was + # verified in the issue itself -- EndRecord's ``__init__`` wants field + # values this test has no reason to know. + instance = EndRecord.__new__(EndRecord) + self.assertIsInstance(instance, EndRecord) + self.assertIsInstance(instance, Mapping) + self.assertNotIsInstance({}, EndRecord) + + @staticmethod + def _make_shared_hierarchy() -> 'tuple[type, type]': + """A throwaway ABC pair reproducing #439's exact shape, without pcapkit. + + Neither class is given its own ``_abc_impl``, so attribute lookup + falls through the MRO to :class:`collections.abc.Mapping`'s -- exactly + the shape ``SchemaMeta.__new__`` used to produce on Python < 3.11, + confirmed below rather than assumed. Building this on plain + ``abc.ABCMeta`` rather than ``SchemaMeta`` keeps the two tests below + about ``abc.ABCMeta`` in general, not about this library's fix. + + """ + class _Shared(Mapping): # pylint: disable=abstract-method + def __getitem__(self, key: object) -> object: + raise KeyError(key) + + def __iter__(self): + return iter(()) + + def __len__(self) -> int: + return 0 + + class _SharedChild(_Shared): + pass + + del _Shared._abc_impl + del _SharedChild._abc_impl + return _Shared, _SharedChild + + def test_probing_the_same_exact_type_reproduces_439s_shape(self) -> None: + """The general mechanism, generalised beyond :class:`dict` and ``Schema``. + + ``collections.ChainMap`` is a *separately* registered + :class:`~collections.abc.Mapping` implementation (registered + directly, not via real inheritance from :class:`dict`), so probing + with it and then checking the *same* exact type exercises the same + registered-virtual-subclass path :class:`dict` does in the real bug -- + demonstrating that #439's shape was never specific to ``dict``, only + to *asking about the same exact type twice*. + + """ + _Shared, _SharedChild = self._make_shared_hierarchy() + self.assertIs(_Shared._abc_impl, Mapping._abc_impl) + self.assertIs(_SharedChild._abc_impl, Mapping._abc_impl) + + # Probe and check the SAME exact type (ChainMap, both times). + self.assertTrue(isinstance(ChainMap(), Mapping)) + self.assertTrue(isinstance(ChainMap(), _SharedChild)) + + def test_probing_a_different_exact_type_gives_a_false_negative(self) -> None: + """A pitfall in reproducing #439: the cache is keyed by *exact* type. + + ``abc.ABCMeta.__subclasscheck__`` (see ``Lib/_py_abc.py``, which + mirrors the C ``_abc`` accelerator) caches its answer under + ``subclass`` -- the *exact* type asked about -- whatever rule actually + matched. So probing with one type and then checking a *different* + type is not a weaker version of the reproduction above, it is not a + reproduction at all: the second question is a cache miss regardless of + whether #439 is fixed, and comes back looking clean either way. + + A plain ``dict`` *subclass* is the probe type here, deliberately, to + pin exactly the trap described in this issue: it has a genuine + ``Mapping`` tie (``isinstance(x, Mapping)`` is ``True`` for it) but + checking a *different* exact type (``ChainMap``) afterwards is + unaffected by that -- "has a Mapping tie" is not "populates the cache + entry the next question will read". A regression test built by + probing with a dict subclass and asserting about anything else would + not have caught #439; :meth:`test_probing_the_same_exact_type_reproduces_439s_shape` + above is what actually exercises the shared cache, and it does so by + keeping the exact type identical across both questions. + + """ + _Shared, _SharedChild = self._make_shared_hierarchy() + self.assertIs(_SharedChild._abc_impl, Mapping._abc_impl) + + class _DictSubclass(dict): + pass + + # Probe with one exact type (a dict subclass)... + self.assertTrue(isinstance(_DictSubclass(), Mapping)) + # ...and check a *different* exact type (ChainMap, never asked about + # in this process before). Clean -- not because the hierarchy above + # is not broken (it is, by construction), but because the two + # questions never touch the same cache entry. + self.assertFalse(isinstance(ChainMap(), _SharedChild)) + self.assertFalse(isinstance(ChainMap(), _Shared)) + + def test_the_pcapng_1248_symptom_no_longer_reproduces(self) -> None: + """The user-facing payoff: a real ``isinstance`` call site, not a probe. + + :meth:`NameResolutionBlock.post_process + ` + asks ``isinstance(record, (IPv4Record, IPv6Record))`` at + :file:`pcapkit/protocols/schema/misc/pcapng.py:1248` and then reads + ``record.names``. Before #439 was fixed, every schema class shared one + ``_abc_impl``, so that question answered ``True`` for the block's + *terminating* ``EndRecord`` -- which has no ``names`` -- provided + *anything at all* had already asked whether that same ``EndRecord`` + instance was a :class:`~pcapkit.protocols.schema.schema.Schema`. + + That is a lower bar than it sounds: ``isinstance(record, Schema)`` is + genuinely, unconditionally true for any real schema instance, via + ordinary inheritance rather than any cache -- and every schema class + asks it routinely (:meth:`Schema.pack`, elsewhere in this module, does + exactly that). Measured directly (not merely inferred): on the broken + code, asking ``isinstance(record, Schema)`` first -- correctly true, + cached against the object every schema class shared -- made the next, + unrelated question, ``isinstance(record, (IPv4Record, IPv6Record))``, + answer ``True`` too, for an ``EndRecord`` that is neither. This is a + *more* direct reproduction of the real call site than probing with a + plain ``dict`` first (as the other tests in this class do): it needs + no ``Mapping``/``dict`` involvement at all, only two ordinary + ``isinstance`` questions about the very code under test, in the order + real code already asks them. + + """ + from pcapkit.protocols.schema.misc.pcapng import EndRecord, IPv4Record, IPv6Record + from pcapkit.protocols.schema.schema import Schema + + record = EndRecord.__new__(EndRecord) + self.assertIs(type(record), EndRecord) + + # Ask the question every schema class asks routinely -- correctly + # True, and on the broken code this is what poisoned the shared cache + # for this exact object. + self.assertTrue(isinstance(record, Schema)) + + self.assertFalse( + isinstance(record, (IPv4Record, IPv6Record)), + "a terminating EndRecord must not test as an IPv4Record/IPv6Record -- " + "if it does, NameResolutionBlock.post_process reads .names off a " + "record that has none, which is the AttributeError #439 caused" + ) + + +@unittest.skipUnless(HAS_RUNTIME, 'runtime dependencies not installed') +class SchemaMetaReservedClassKwargsTests(unittest.TestCase): + """Regression tests for the reserved-class-keyword guard added with #439. + + Renaming ``namespace=`` to ``ns=`` (see :mod:`pcapkit.protocols.schema.misc. + pcapng`) fixed the one collision that was actually in use, but the + collision class is not specific to ``namespace``: any class keyword + spelled ``mcls``, ``name``, ``bases`` or ``namespace`` collides with a + same-named parameter of :meth:`abc.ABCMeta.__new__` (on Python < 3.11 for + the first three, on every version for the fourth -- see + :attr:`~pcapkit.protocols.schema.schema.SchemaMeta._RESERVED_CLASS_KWARGS` + for the precise reasoning), and ``cls`` collides with the implicit + ``__init_subclass__`` classmethod binding on every version. Without a + guard, the *next* accidental use of one of these names resurfaces as an + opaque ``TypeError: ...__new__() got multiple values for argument '...'`` + several frames away, exactly the class of confusion that produced the + "for unknown reason" comment #439 started from. These tests pin that the + guard actually fires, names the offending keyword, and that ordinary class + keywords are unaffected. + + """ + + def setUp(self) -> None: + purge_modules(['pcapkit']) + + def test_each_reserved_keyword_raises_a_named_schema_error(self) -> None: + """Every reserved keyword raises :exc:`SchemaError`, not a bare TypeError. + + A bare ``TypeError`` here would violate the house rule that in-library + exceptions come from :mod:`pcapkit.utilities.exceptions` -- and would + also be indistinguishable from the very opaque failure this guard + exists to replace. + + """ + from pcapkit.protocols.schema.schema import Schema, SchemaMeta + from pcapkit.utilities.exceptions import SchemaError + + for keyword in sorted(SchemaMeta._RESERVED_CLASS_KWARGS): + with self.subTest(keyword=keyword): + with self.assertRaises(SchemaError) as caught: + type(f'Reserved_{keyword}', (Schema,), {}, **{keyword: 'x'}) + self.assertIn(keyword, str(caught.exception)) + + def test_an_ordinary_class_keyword_is_unaffected(self) -> None: + """The guard must not reject a class keyword that does not collide. + + ``code=`` is the other class keyword :class:`Schema` subclasses use + (see :meth:`EnumSchema.__init_subclass__ + `), and + it does not appear anywhere in + :attr:`~pcapkit.protocols.schema.schema.SchemaMeta._RESERVED_CLASS_KWARGS`, + so it must keep working exactly as before -- as must the renamed + ``ns=`` keyword itself. + + """ + from pcapkit.const.pcapng.option_type import OptionType as Enum_OptionType + from pcapkit.protocols.schema.misc.pcapng import Option + + class _RenamedOption(Option, ns='opt', code=Enum_OptionType.opt_comment): + pass + + self.assertEqual(_RenamedOption.__namespace__, 'opt') + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/protocols/schema/test_schema_unit.py b/tests/protocols/schema/test_schema_unit.py index 70a45bd7e3..4cda19844f 100644 --- a/tests/protocols/schema/test_schema_unit.py +++ b/tests/protocols/schema/test_schema_unit.py @@ -227,7 +227,19 @@ class ManySchema(BaseEnumSchema, code=[Code.two, Code.three]): self.assertIs(BaseEnumSchema.registry[Code.two], OneSchema) self.assertIs(BaseEnumSchema.from_dict().registry, BaseEnumSchema.registry) - def test_schema_final_generated_init_and_legacy_version_branch(self) -> None: + def test_schema_final_generated_init(self) -> None: + """``schema_final`` generates a typed ``__init__`` from the field table. + + This used to also cover a ``sys.version_info``-mocked "legacy version" + branch in ``SchemaMeta.__new__``, which no longer exists: that branch + was GitHub issue #439's workaround for a class-keyword collision with + ``abc.ABCMeta.__new__`` on Python < 3.11, and the fix removed it (and + the ``sys`` import along with it) in favour of renaming the one + keyword that actually collided. ``SchemaMeta.__new__`` no longer + branches on the interpreter version at all, so there is nothing left + for a mocked ``sys.version_info`` to exercise here. + + """ from pcapkit.corekit.fields.numbers import UInt8Field from pcapkit.protocols.schema.schema import Schema, schema_final @@ -243,12 +255,6 @@ class EmptyGeneratedSchema(Schema): self.assertEqual(bytes(GeneratedInitSchema()), b'\x01') self.assertEqual(bytes(EmptyGeneratedSchema()), b'') - with mock.patch('pcapkit.protocols.schema.schema.sys.version_info', (3, 10)): - class LegacyVersionSchema(Schema): - value: int = UInt8Field(default=3) - - self.assertIn('value', LegacyVersionSchema.__fields__) - def test_generated_init_is_installed_and_runs_post_init(self) -> None: from pcapkit.corekit.fields.collections import ListField from pcapkit.corekit.fields.numbers import UInt8Field, UInt16Field diff --git a/tests/protocols/test_option_roundtrip_unit.py b/tests/protocols/test_option_roundtrip_unit.py index 9eda19c983..1b31bef19a 100644 --- a/tests/protocols/test_option_roundtrip_unit.py +++ b/tests/protocols/test_option_roundtrip_unit.py @@ -23,25 +23,6 @@ by case, which cycles do not close today and which defect stops each one. A case absent from that table has to come back ``'OK'``. -When the outcome depends on the interpreter, not the code ---------------------------------------------------------- - -One recorded expectation per case assumes the outcome is a property of the -library. Seven PCAP-NG name-resolution cases show it can be a property of the -*interpreter*: they round-trip from Python 3.11 on and fail to construct on 3.10 -and earlier, because :class:`~pcapkit.protocols.schema.schema.Schema` subclasses -share one ``_abc_impl`` there and an ``isinstance`` against a schema class -therefore answers by cache order rather than by type (GitHub issue #439). - -Those seven live in :data:`INTERPRETER_GAPS` rather than in -:data:`EXPECTED_FAILURES`, and the separation is deliberate. Behaving differently -on two supported Pythons is a defect and not a variant, so the mechanism is -asserted directly by -:meth:`OptionRoundTripTests.test_schema_isinstance_is_interpreter_dependent`: -the record is evidence about a named library bug, not an exemption keyed on a -version number. On a modern interpreter the seven are held to ``'OK'`` like -anything else, and fixing #439 turns 3.10 red. - Why the table is asserted in both directions -------------------------------------------- @@ -507,93 +488,6 @@ class Gap(NamedTuple): } -#: Whether this interpreter gives every :class:`Schema` subclass its own -#: ``_abc_impl``, and therefore answers ``isinstance`` against schema classes -#: correctly. -#: -#: On CPython 3.10 and earlier they all share :class:`Schema`'s, which is GitHub -#: issue #439. Measured on 3.10.20 and 3.14.7: -#: ``EndRecord._abc_impl is IPv4Record._abc_impl`` is :data:`True` on 3.10 and -#: :data:`False` on 3.14, for the schema classes in -#: :mod:`pcapkit.protocols.schema.misc.pcapng`. Note that the *data* models -#: (:class:`~pcapkit.corekit.infoclass.Info` subclasses) are **not** affected on -#: either: ``Data_EndRecord._abc_impl is Info._abc_impl`` is :data:`False` on -#: both, so it is specifically the schema hierarchy. -SCHEMA_ABC_IS_PER_CLASS = sys.version_info >= (3, 11) - -#: Cases whose outcome is a property of the *interpreter* rather than of the -#: option, applying only where :data:`SCHEMA_ABC_IS_PER_CLASS` is false. -#: -#: This table exists reluctantly, and it is worth being explicit about why it is -#: not the same thing as :data:`EXPECTED_FAILURES`. -#: -#: A case that behaves differently on two supported Pythons is a defect, not a -#: variant, and the honest thing is to surface it rather than to encode it as a -#: shrug. So it is surfaced twice over: the entries below name the library defect -#: and the ``file:line`` that causes it, exactly as the main table does, and -#: :meth:`OptionRoundTripTests.test_schema_isinstance_is_interpreter_dependent` -#: pins the *mechanism* directly so the record is evidence and not an assertion -#: about a version number. What the table does not do is let the cases pass -#: quietly: on a modern interpreter they are held to ``'OK'`` by the main table's -#: absence of an entry, and on an old one they are held to failing in exactly the -#: recorded way. Fixing #439 turns this red on 3.10, which is the point. -#: -#: An entry here **overrides** :data:`EXPECTED_FAILURES` for the same label -#: rather than being disjoint from it, and three of the seven need that. The -#: ``ns_dns*`` options fail on every interpreter, but for different reasons: from -#: 3.11 on it is the shared ``self._opt`` counter rejecting the option the same -#: instance just built, and on 3.10 the block never gets that far because -#: ``post_process`` dies first. Same status, different cause, so both are recorded -#: and the interpreter decides which one is checked. The remaining four have no -#: entry in the main table at all, because they round-trip cleanly from 3.11. -#: -#: All seven are PCAP-NG name resolution, and they share one cause on 3.10. -#: ``NameResolutionBlock.post_process`` asks -#: ``isinstance(record, (IPv4Record, IPv6Record))`` at -#: :file:`pcapkit/protocols/schema/misc/pcapng.py:1248` and then reads -#: ``record.names``. Because every schema class shares one ``_abc_impl`` on 3.10, -#: that question returns :data:`True` for the block's terminating ``EndRecord`` -#: -- measured in the real path, with ``type(record) is EndRecord`` :data:`True` -#: and ``isinstance(record, (IPv4Record, IPv6Record))`` also :data:`True` in the -#: same breath -- so ``post_process`` reads ``names`` off a record that has none. -#: Every NRB carries a terminating ``EndRecord``, which is why the block, its -#: three ``ns_dns*`` options and its three records all go together. -INTERPRETER_GAPS = { - label: Gap( - 'CONSTRUCT', "'EndRecord' object has no attribute 'names'", - 'pcapkit/protocols/schema/misc/pcapng.py:1248 -- isinstance against ' - 'schema classes, which share one _abc_impl on CPython <= 3.10 (#439), so ' - 'the terminating EndRecord tests True as an IPv4Record/IPv6Record') - for label in ( - 'pcapng-block/Name_Resolution_Block', - 'pcapng-option/ns_dnsname_2', - 'pcapng-option/ns_dnsIP4addr_3', - 'pcapng-option/ns_dnsIP6addr_4', - 'pcapng-record/nrb_record_end', - 'pcapng-record/nrb_record_ipv4', - 'pcapng-record/nrb_record_ipv6', - ) -} - - -def _abc_impl(cls: 'type') -> 'Any': - """The ABC implementation object a class dispatches ``isinstance`` through. - - ``_abc_impl`` is a CPython internal and is not in the type stubs, so it is - read by name. Present on both the C ``_abc`` and the pure-python ``_py_abc`` - backends; :func:`tests._support._reset_abc_caches` relies on the same - attribute. - - Args: - cls: The class to inspect. - - Returns: - The class's ``_abc_impl``, or :data:`None` if it has none. - - """ - return getattr(cls, '_abc_impl', None) - - def _gap_for(label: 'str') -> 'Optional[Gap]': """The recorded expectation for ``label``, or :data:`None` if it should pass. @@ -605,13 +499,6 @@ def _gap_for(label: 'str') -> 'Optional[Gap]': round-trip on this interpreter. """ - # The interpreter-conditional record wins where there is one, because on an - # affected interpreter the case fails there before it can reach whatever the - # main table describes. - if not SCHEMA_ABC_IS_PER_CLASS: - gap = INTERPRETER_GAPS.get(label) - if gap is not None: - return gap return EXPECTED_FAILURES.get(label) @@ -750,73 +637,11 @@ def test_expected_failures_name_real_cases(self) -> None: """ labels = {case.label for case in self.options.cases()} - for name, table in (('EXPECTED_FAILURES', EXPECTED_FAILURES), - ('INTERPRETER_GAPS', INTERPRETER_GAPS)): - with self.subTest(table=name): - stale = sorted(set(table) - labels) - self.assertEqual( - stale, [], - f'these entries of {name} name cases that no longer exist; ' - f'delete them, or fix the label' - ) - - # An override that records the same thing as the entry it overrides is - # noise, and would quietly outlive the reason it was added. - for label in sorted(set(EXPECTED_FAILURES) & set(INTERPRETER_GAPS)): - with self.subTest(overridden=label): - general, specific = EXPECTED_FAILURES[label], INTERPRETER_GAPS[label] - self.assertNotEqual( - (general.status, general.fragment), - (specific.status, specific.fragment), - f'{label} records the same status and fragment in both tables, ' - f'so the interpreter-conditional entry adds nothing; delete it' - ) - - def test_schema_isinstance_is_interpreter_dependent(self) -> None: - """Pin the mechanism behind :data:`INTERPRETER_GAPS`, so it is evidence. - - The seven cases in that table are excused on old interpreters, and an - excuse resting on a version comparison would be indistinguishable from - giving up. So the *cause* is asserted here directly: every - :class:`~pcapkit.protocols.schema.schema.Schema` subclass shares one - ``_abc_impl`` on CPython 3.10 and earlier, and has its own from 3.11 -- - which is what makes ``isinstance`` against a schema class answer by cache - order rather than by type. - - This is GitHub issue #439 and belongs to ``SchemaMeta``, not here. When it - is fixed this test fails on 3.10, which is the prompt to delete both it - and :data:`INTERPRETER_GAPS`. - - The data models are checked too, and asserted *unaffected* on both - interpreters -- that is the boundary of the defect, and getting it wrong - in either direction would send the next reader down the wrong path. - - """ - from pcapkit.corekit.infoclass import Info - from pcapkit.protocols.data.misc.pcapng import EndRecord as Data_EndRecord - from pcapkit.protocols.schema.misc.pcapng import EndRecord, IPv4Record - from pcapkit.protocols.schema.schema import Schema - - shared = _abc_impl(EndRecord) is _abc_impl(IPv4Record) + stale = sorted(set(EXPECTED_FAILURES) - labels) self.assertEqual( - shared, not SCHEMA_ABC_IS_PER_CLASS, - f'on Python {sys.version_info[0]}.{sys.version_info[1]}, schema ' - f'classes sharing one _abc_impl is {shared}; INTERPRETER_GAPS assumes ' - f'{not SCHEMA_ABC_IS_PER_CLASS}. If #439 is fixed, delete that table ' - f'and this test.' - ) - if shared: - self.assertIs( - _abc_impl(EndRecord), _abc_impl(Schema), - "the shared _abc_impl should be Schema's own" - ) - - # The boundary: Info subclasses are unaffected on every interpreter, so - # nothing that dispatches on a *data* model is implicated in #439. - self.assertIsNot( - _abc_impl(Data_EndRecord), _abc_impl(Info), - 'the data models were unaffected by #439 on both 3.10 and 3.14; if ' - 'that has changed, the scope of the defect is wider than recorded' + stale, [], + f'these entries of EXPECTED_FAILURES name cases that no longer ' + f'exist; delete them, or fix the label' ) def test_round_trip_is_identity_or_a_recorded_gap(self) -> None: @@ -912,8 +737,6 @@ def test_recorded_gaps_are_a_minority(self) -> None: """ total = len(self.options.cases()) recorded = len(EXPECTED_FAILURES) - if not SCHEMA_ABC_IS_PER_CLASS: - recorded += len(INTERPRETER_GAPS) self.assertLess( recorded, total // 2, f'{recorded} of {total} cases are recorded as failing; that is more '