From d22b71f6daa82dd055673cfb42b20e7e2354c34b Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Sun, 27 Sep 2026 19:45:02 -0400 Subject: [PATCH] fix(const,vendor): return the unregistered member instead of re-entering _missing_ (#866) - `pcapkit/vendor/pcapng/{record_type,secrets_type}.py` rendered a two-line `_missing_` body whose first line called `_unregistered_member` without returning it and whose second was `return cls(value)`. That was harmless while the first line was `extend_enum`, which registers the member so the following lookup found it; #861 replaced it with `_unregistered_member`, which deliberately does not register, so the lookup missed again and re-entered `_missing_` until `RecursionError` - collapse both to the single returning line the other 101 registries already use, and regenerate `pcapkit/const/pcapng/{record_type,secrets_type}.py` from the fixed crawlers - add `tests/vendor/test_vendor_missing_body_unit.py`, a crawler-layer guard: every constant line a crawler emits into `miss` must `return`, none may be `return cls(value)`, and the sweep size is pinned at 35 so a crawler joining or leaving is deliberate. #861 fixed the generated files and not the generators, so the defect was latent until `e58618bdf` regenerated Build: 6 new tests pass. With the four source files reverted, 4 of the 6 fail (counted as methods, not subTest records): the self-recursive-lookup guard, the named-pair guard, and both behavioural lookups. `test_the_sweep_size_is_pinned` and `test_the_hard_coded_body_ends_in_a_return` pass either way -- the latter by design, since #866's body ended on `return cls(value)`, which returns. `tests/const/test_const_enum_lookup.py` also fails on the reverted tree, which is what caught this on main. tests/vendor 92 and tests/project 156 green. --- pcapkit/const/pcapng/record_type.py | 3 +- pcapkit/const/pcapng/secrets_type.py | 3 +- pcapkit/vendor/pcapng/record_type.py | 3 +- pcapkit/vendor/pcapng/secrets_type.py | 3 +- tests/vendor/test_vendor_missing_body_unit.py | 239 ++++++++++++++++++ 5 files changed, 243 insertions(+), 8 deletions(-) create mode 100644 tests/vendor/test_vendor_missing_body_unit.py diff --git a/pcapkit/const/pcapng/record_type.py b/pcapkit/const/pcapng/record_type.py index 25ede0463c..1a21f8f81c 100644 --- a/pcapkit/const/pcapng/record_type.py +++ b/pcapkit/const/pcapng/record_type.py @@ -39,5 +39,4 @@ def _missing_(cls, value: 'int') -> 'RecordType': """ if not (isinstance(value, int) and 0 <= value <= 0xFFFF): raise ValueError('%r is not a valid %s' % (value, cls.__name__)) - cls._unregistered_member(value, 'Unassigned') - return cls(value) + return cls._unregistered_member(value, 'Unassigned') diff --git a/pcapkit/const/pcapng/secrets_type.py b/pcapkit/const/pcapng/secrets_type.py index 95b1328a2d..6a85b0ac28 100644 --- a/pcapkit/const/pcapng/secrets_type.py +++ b/pcapkit/const/pcapng/secrets_type.py @@ -38,5 +38,4 @@ def _missing_(cls, value: 'int') -> 'SecretsType': """ if not (isinstance(value, int) and 0x00000000 <= value <= 0xFFFFFFFF): raise ValueError('%r is not a valid %s' % (value, cls.__name__)) - cls._unregistered_member(value, 'Unassigned') - return cls(value) + return cls._unregistered_member(value, 'Unassigned') diff --git a/pcapkit/vendor/pcapng/record_type.py b/pcapkit/vendor/pcapng/record_type.py index bef958c954..6142f5f23d 100644 --- a/pcapkit/vendor/pcapng/record_type.py +++ b/pcapkit/vendor/pcapng/record_type.py @@ -64,8 +64,7 @@ def process(self, data: 'list[Tag]') -> 'tuple[list[str], list[str]]': """ enum = [] # type: list[str] miss = [ - "cls._unregistered_member(value, 'Unassigned')", - 'return cls(value)' + "return cls._unregistered_member(value, 'Unassigned')", ] for content in data: name = content.select('td')[0].text.strip() diff --git a/pcapkit/vendor/pcapng/secrets_type.py b/pcapkit/vendor/pcapng/secrets_type.py index 8b97fa43f7..f1c4860428 100644 --- a/pcapkit/vendor/pcapng/secrets_type.py +++ b/pcapkit/vendor/pcapng/secrets_type.py @@ -68,8 +68,7 @@ def process(self, data: 'dict[int, str]') -> 'tuple[list[str], list[str]]': # t """ enum = [] # type: list[str] miss = [ - "cls._unregistered_member(value, 'Unassigned')", - 'return cls(value)' + "return cls._unregistered_member(value, 'Unassigned')", ] for code, name in DATA.items(): renm = self.rename(name, code) # type: ignore[arg-type] diff --git a/tests/vendor/test_vendor_missing_body_unit.py b/tests/vendor/test_vendor_missing_body_unit.py new file mode 100644 index 0000000000..cf7f20ea42 --- /dev/null +++ b/tests/vendor/test_vendor_missing_body_unit.py @@ -0,0 +1,239 @@ +# -*- coding: utf-8 -*- +"""A hard-coded ``_missing_`` body must end in a ``return``, and no emitted +line may re-enter ``_missing_`` for the same value. + +GitHub issue #866. A vendor crawler renders the body of the generated +registry's :meth:`~enum.Enum._missing_` as a list of source lines, held in the +local ``miss``. Two crawlers -- +:mod:`pcapkit.vendor.pcapng.record_type` and +:mod:`pcapkit.vendor.pcapng.secrets_type` -- emitted a **two**-line body whose +first line called :meth:`~pcapkit.corekit.enum.EnumRegistry._unregistered_member` +without returning it and whose second line was ``return cls(value)``:: + + cls._unregistered_member(value, 'Unassigned') + return cls(value) + +That shape was harmless while the first line was ``extend_enum(...)``, which +*registers* the member, so the following ``cls(value)`` found it and returned. +#861 replaced the ``extend_enum`` call with ``_unregistered_member``, which +deliberately does **not** register -- so ``cls(value)`` missed again, re-entered +``_missing_``, and recursed until ``RecursionError``. + +The invariant is deliberately not "every emitted line returns": the base +crawler itself emits legitimate non-returning lines for a range-guarded body. +:meth:`pcapkit.vendor.default.Vendor.process` (see +:file:`pcapkit/vendor/default.py:341-343`) renders such a body as three +lines -- an ``if`` header, a ``#:`` comment, then an indented ``return`` -- +and only the third of the three returns:: + + miss.append(f'if {start} <= value <= {stop}:') + miss.append(f' #: {desc}') + miss.append(f" return cls._unregistered_member(value, '{self.safe_name(name)}')") + +Measured: 53 crawlers emit 109 such non-returning lines between them, and +three registries under :mod:`pcapkit.const` carry exactly that shape today. +A crawler hard-coding a range-guarded body as a list literal is writing +something valid, so the guard below checks only that a hard-coded body +*ends* in a ``return`` and that no emitted line re-enters ``_missing_`` for +the same value -- not that every line returns. + +#861 only *half*-corrected the two generated files, and never touched the two +crawlers. It replaced the non-returning ``extend_enum(...)`` call with a +returning ``_unregistered_member(...)`` call, but the ``return cls(value)`` +line that followed was left in place, now unreachable dead code:: + + - extend_enum(cls, 'Unassigned_0x%08x' % value, value) + + return cls._unregistered_member(value, 'Unassigned') + return cls(value) # <- still there, now unreachable + +That is why #861's own CI was green: the first ``return`` short-circuits, so +the dead second line never ran. (A statement following a ``return`` in +generated output is itself worth noticing on its own -- it means the +generator that will overwrite the file disagrees with what the file +currently does.) Because the crawlers were never touched, a later +regeneration -- ``e58618bdf`` ("Bumped version to 1.5.0b5") -- reproduced the +original two-line body from the stale crawlers and reintroduced the +recursion, turning **12** tests red across **three** files: +:file:`tests/const/test_const_enum_lookup.py`, +:file:`tests/const/test_const_enum_no_mint.py` +(``RulingConversionDoesNotMintTests.test_converted_value_does_not_mint`` and +``test_repeated_lookup_does_not_grow_members``, two registries each), and +:file:`tests/protocols/misc/test_pcapng_unit.py` (7 of the 12) -- all twelve +``RecursionError``. + +The guard here is deliberately at the **crawler** layer rather than the +generated one. ``tests/const/test_const_enum_lookup.py`` already exercises every +committed registry and is what caught the regression; what nothing covered was +the source a crawler *emits*, so a fix to a generated file could silently +disagree with the generator that will overwrite it. This module needs no network +and no regeneration: it reads the crawlers' own ASTs. + +""" +from __future__ import annotations + +import ast +import pathlib +import unittest + +#: Root of the vendor crawler package, as a path rather than an import, so this +#: module reads the same tree it is checked out in regardless of which +#: ``pcapkit`` an editable install happens to resolve. +VENDOR_ROOT = pathlib.Path(__file__).resolve().parents[2] / 'pcapkit' / 'vendor' + +#: How many crawlers emit at least one *hard-coded* line into ``miss`` as a +#: string-constant list literal. Pinned so a crawler that leaves this shape -- +#: switching to ``.append()``, or dropping its unassigned range -- is visible +#: here rather than silently shrinking the sweep below. It does **not** catch +#: the opposite drift: a brand-new crawler that starts in a shape this guard +#: never covered joins invisibly, because nothing here shrinks when one is +#: added. +#: +#: Measured on ``main`` at ``946b84e83``: 96 crawler modules define +#: ``process()``. Of those, 35 assign ``miss`` as a list literal of string +#: constants -- the ones this test can see -- 53 build it by ``.append()`` +#: instead, and 8 never return an ``(enum, miss)`` pair at all. That leaves +#: **61 of the 96 invisible to this guard**, not merely the two crawlers used +#: as an example in :func:`_constant_miss_lines`'s docstring below. A crawler +#: that assigns ``miss`` from a comprehension, from a variable, or under a +#: different variable name entirely is skipped by construction too, same as +#: one built by ``.append()``. All 61 are covered behaviourally, not +#: statically, by :file:`tests/const/test_const_enum_lookup.py`. +EXPECTED_CRAWLERS_WITH_CONSTANT_MISS_LINES = 35 + + +def _constant_miss_lines() -> 'dict[str, list[str]]': + """Collect every constant line each crawler assigns to ``miss``. + + Returns a mapping of repository-relative crawler path to the list of + string constants in its ``miss`` list literal. A crawler that builds + ``miss`` by ``.append()``, assigns it from a comprehension or a variable, + or never returns an ``(enum, miss)`` pair at all, is absent from the + returned mapping entirely -- not present with an empty list. That is + **61 of the 96** crawler modules defining ``process()``, not just the two + examples this docstring used to single out + (:mod:`pcapkit.vendor.reg.ethertype`, :mod:`pcapkit.vendor.ipx.socket`); + see :data:`EXPECTED_CRAWLERS_WITH_CONSTANT_MISS_LINES` for the full + breakdown. All 61 are covered behaviourally, not statically, by + :file:`tests/const/test_const_enum_lookup.py`. + + """ + found = {} # type: dict[str, list[str]] + for path in sorted(VENDOR_ROOT.rglob('*.py')): + tree = ast.parse(path.read_text(encoding='utf-8')) + for node in ast.walk(tree): + if not isinstance(node, ast.Assign): + continue + if not any(isinstance(t, ast.Name) and t.id == 'miss' for t in node.targets): + continue + if not isinstance(node.value, ast.List): + continue + lines = [elt.value for elt in node.value.elts + if isinstance(elt, ast.Constant) and isinstance(elt.value, str)] + if lines: + found[str(path.relative_to(VENDOR_ROOT.parents[1]))] = lines + return found + + +class VendorMissingBodyTests(unittest.TestCase): + """Static guards over what the crawlers render into ``_missing_``.""" + + @classmethod + def setUpClass(cls) -> None: + cls.miss_lines = _constant_miss_lines() # type: ignore[attr-defined] + + def test_the_sweep_size_is_pinned(self) -> None: + """A crawler leaving or joining the sweep must be deliberate.""" + self.assertEqual(len(self.miss_lines), # type: ignore[attr-defined] + EXPECTED_CRAWLERS_WITH_CONSTANT_MISS_LINES, + 'the set of crawlers emitting a constant ``_missing_`` body ' + 'changed; update EXPECTED_CRAWLERS_WITH_CONSTANT_MISS_LINES ' + 'once you have checked the new ones still return') + + def test_the_hard_coded_body_ends_in_a_return(self) -> None: + """The invariant #866 is about, restated to the shape that is true. + + Not every emitted line has to ``return`` -- the range-guarded shape + in :meth:`pcapkit.vendor.default.Vendor.process` legitimately emits + an ``if`` header and a ``#:`` comment before its ``return``, and 53 + crawlers do exactly that (see the module docstring). What has to be + true is narrower: a hard-coded body must *end* on a ``return``, so + control never falls off the end of ``miss`` into a fallthrough. + + This test does **not** catch #866 on its own: #866's body ended on + ``return cls(value)``, which does return, so it passes this check + both before and after the fix. It is + :meth:`test_no_crawler_emits_a_self_recursive_lookup` that catches + the cycle -- this test only guards against a body that does not + return at all. + + """ + for crawler, lines in sorted(self.miss_lines.items()): # type: ignore[attr-defined] + last = lines[-1] + with self.subTest(crawler=crawler, line=last): + self.assertTrue(last.lstrip().startswith('return '), + f'{crawler} ends its hard-coded ``_missing_`` body on ' + f'{last!r}, which does not return; a body that falls ' + f'through does nothing observable, see GitHub issue #866') + + def test_no_crawler_emits_a_self_recursive_lookup(self) -> None: + """``return cls(value)`` inside ``_missing_`` is unconditionally wrong. + + Named separately from :meth:`test_the_hard_coded_body_ends_in_a_return` + because this line *does* return and so passes that check, yet re-enters + ``_missing_`` for the same value. It was the second half of #866's + two-line body and is the line that actually recursed. + + """ + for crawler, lines in sorted(self.miss_lines.items()): # type: ignore[attr-defined] + for line in lines: + with self.subTest(crawler=crawler, line=line): + self.assertNotEqual(line.replace(' ', ''), 'returncls(value)', + f'{crawler} emits ``return cls(value)`` inside ' + f'``_missing_``, which re-enters ``_missing_`` ' + f'for the same value; see GitHub issue #866') + + def test_the_two_regressed_crawlers_emit_a_single_returning_line(self) -> None: + """The specific pair from #866, pinned by name. + + Kept alongside the general sweeps because a regression here is the one + that reached ``main``, and a named test says which file to look at + without having to read a subTest label. + + """ + for crawler in ('pcapkit/vendor/pcapng/record_type.py', + 'pcapkit/vendor/pcapng/secrets_type.py'): + with self.subTest(crawler=crawler): + lines = self.miss_lines[crawler] # type: ignore[attr-defined] + self.assertEqual( + lines, ["return cls._unregistered_member(value, 'Unassigned')"], + f'{crawler} must emit exactly one returning line') + + +class RegeneratedRegistryLookupTests(unittest.TestCase): + """The behaviour the broken body produced, pinned on the generated files. + + :file:`tests/const/test_const_enum_lookup.py` sweeps every registry and so + already covers this; these two cases are named for #866 so the reported + symptom is findable from the issue number. + + """ + + def test_record_type_resolves_an_unassigned_value(self) -> None: + from pcapkit.const.pcapng.record_type import RecordType + + member = RecordType(0xFFFF) + self.assertEqual(member.name, 'Unassigned') + self.assertEqual(int(member), 0xFFFF) + self.assertNotIn('Unassigned', RecordType.__members__) + + def test_secrets_type_resolves_an_unassigned_value(self) -> None: + from pcapkit.const.pcapng.secrets_type import SecretsType + + member = SecretsType(0) + self.assertEqual(member.name, 'Unassigned') + self.assertEqual(int(member), 0) + self.assertNotIn('Unassigned', SecretsType.__members__) + + +if __name__ == '__main__': + unittest.main()