From 7b211fae2ac4728d95a725c8faaba42fd3a8e302 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Sun, 20 Sep 2026 02:24:39 -0400 Subject: [PATCH] fix(exports): export the 23 public attributes their package __all__ omitted (#533) * `pcapkit/__init__.py` mirrors `pcapkit.protocols.__all__` again, restoring the 12 protocol names it drifted behind: `C_Tag`, `S_Tag`, `DRARP`, `InARP`, `L2TPv2`, `HTTPv1`, `HTTPv2`, `PCAPNG`, `Header`, `Frame`, `Data`, `Schema`. #436 updated the child list and left the parent alone. * `pcapkit/foundation/__init__.py` lists the six registry functions it withheld while listing the other 27, plus the two manager classes. #527 fixed the same six one level down, in `registry/__init__.py`. * `pcapkit/foundation/reassembly/__init__.py` and `traceflow/__init__.py` export `ReassemblyManager` and `TraceFlowManager`, defined below the `__all__` literal that forgot them and imported by name from six modules. * `pcapkit/utilities/__init__.py` exports `beholder`, `prepare` and `seekset`, which its own module docstring advertises and which pylint reported as unused imports precisely because no `__all__` named them. * `tests/project/test_public_api.py` gains the converse assertion over all 52 public packages against `DELIBERATE_NON_EXPORTS`, plus a test that stops that allowlist rotting into a silencer. Nothing outside an export list changed, and nothing reads the five lists as data. Unit tier 1036 passed / 8 skipped / 2536 subtests, with one pre-existing docstring-registry failure that reproduces identically on 122d32795. pylint drops three messages and adds none. --- pcapkit/__init__.py | 11 +- pcapkit/foundation/__init__.py | 5 + pcapkit/foundation/reassembly/__init__.py | 1 + pcapkit/foundation/traceflow/__init__.py | 1 + pcapkit/utilities/__init__.py | 3 +- tests/project/test_public_api.py | 198 ++++++++++++++++++---- 6 files changed, 186 insertions(+), 33 deletions(-) diff --git a/pcapkit/__init__.py b/pcapkit/__init__.py index 41c7873cf2..1d12bdeea3 100644 --- a/pcapkit/__init__.py +++ b/pcapkit/__init__.py @@ -106,10 +106,14 @@ 'LINKTYPE', 'ETHERTYPE', 'TRANSTYPE', 'APPTYPE', # Protocol Numbers + 'Header', 'Frame', # PCAP Headers + 'PCAPNG', # PCAPNG Format + 'NoPayload', # No Payload 'Raw', # Raw Packet - 'ARP', 'Ethernet', 'L2TP', 'OSPF', 'RARP', 'VLAN', # Link Layer + 'ARP', 'C_Tag', 'DRARP', 'Ethernet', 'InARP', 'L2TP', # Link Layer + 'L2TPv2', 'OSPF', 'RARP', 'S_Tag', 'VLAN', 'AH', 'ESP', 'IP', 'IPsec', 'IPv4', 'IPv6', 'IPX', # Internet Layer 'HIP', 'HOPOPT', 'IPv6_Frag', 'IPv6_Opts', 'IPv6_Route', 'MH', @@ -118,7 +122,10 @@ 'TCP', 'UDP', 'SCTP', # Transport Layer 'FTP', 'FTP_DATA', # Application Layer - 'HTTP', 'NGAP', + 'HTTP', 'HTTPv1', 'HTTPv2', 'NGAP', + + 'Data', # Protocol Data + 'Schema', # Protocol Schema ] #: version number diff --git a/pcapkit/foundation/__init__.py b/pcapkit/foundation/__init__.py index 443f4b3603..3c34336817 100644 --- a/pcapkit/foundation/__init__.py +++ b/pcapkit/foundation/__init__.py @@ -21,8 +21,10 @@ 'Extractor', 'IPv4_Reassembly', 'IPv6_Reassembly', 'TCP_Reassembly', + 'ReassemblyManager', 'TCP_TraceFlow', + 'TraceFlowManager', 'register_protocol', 'register_linktype', @@ -42,4 +44,7 @@ 'register_extractor_engine', 'register_dumper', 'register_extractor_dumper', 'register_traceflow_dumper', + 'register_extractor_reassembly', 'register_extractor_traceflow', + 'register_reassembly_ipv4_callback', 'register_reassembly_ipv6_callback', + 'register_reassembly_tcp_callback', 'register_traceflow_tcp_callback', ] diff --git a/pcapkit/foundation/reassembly/__init__.py b/pcapkit/foundation/reassembly/__init__.py index 2dd00ad1c9..302edf2c91 100644 --- a/pcapkit/foundation/reassembly/__init__.py +++ b/pcapkit/foundation/reassembly/__init__.py @@ -24,6 +24,7 @@ __all__ = [ 'IPv4_Reassembly', 'IPv6_Reassembly', # IP Reassembly 'TCP_Reassembly', # TCP Reassembly + 'ReassemblyManager', # Reassembly Manager, defined below ] from typing import TYPE_CHECKING diff --git a/pcapkit/foundation/traceflow/__init__.py b/pcapkit/foundation/traceflow/__init__.py index 9e0d0ffdf5..5cc34294ab 100644 --- a/pcapkit/foundation/traceflow/__init__.py +++ b/pcapkit/foundation/traceflow/__init__.py @@ -22,6 +22,7 @@ __all__ = [ 'TCP_TraceFlow', + 'TraceFlowManager', # TraceFlow Manager, defined below ] diff --git a/pcapkit/utilities/__init__.py b/pcapkit/utilities/__init__.py index 8e86915243..d67b5ce779 100644 --- a/pcapkit/utilities/__init__.py +++ b/pcapkit/utilities/__init__.py @@ -19,4 +19,5 @@ from pcapkit.utilities.warnings import warn __all__ = ['logger', 'get_logger', 'configure', 'reset', 'ensure_output', - 'warn', 'stacklevel', 'detect'] + 'warn', 'stacklevel', 'detect', + 'beholder', 'prepare', 'seekset'] diff --git a/tests/project/test_public_api.py b/tests/project/test_public_api.py index add735aeb8..1e1cc04b2a 100644 --- a/tests/project/test_public_api.py +++ b/tests/project/test_public_api.py @@ -1,12 +1,15 @@ # -*- coding: utf-8 -*- """The public API surface holds together. -Three invariants about :attr:`__all__`, none of which anything else in the suite +Four invariants about :attr:`__all__`, none of which anything else in the suite checked before: #. every public subpackage survives ``from import *``; #. every name in every public module's :attr:`__all__` actually resolves; -#. the three re-export aggregators list *everything* public they hold. +#. every public package lists everything public it holds, bar an allowlist of + exclusions that had to be written down one at a time; +#. the three re-export aggregators list *everything* public they hold, with no + allowlist at all. The first two exist because of GitHub issue #515. ``pcapkit/protocols/data`` listed ``'HOPOPT_QuickStartOption'`` in its :attr:`__all__` and never bound it, @@ -54,22 +57,41 @@ defect and is allowed to fail the test, rather than being softened into a skip that would let the next :attr:`__all__` bug through. -**The converse -- every public attribute appears in :attr:`__all__` -- is -asserted only for the three aggregators.** It is not viable in general, and the -numbers are the reason. Applied to every module it flags 2848 names across 286 -modules; applied to the 52 public packages it still flags 45 across 13, and most -of those are not omissions at all but names a wildcard import dragged in: -``TYPE_CHECKING`` in :mod:`pcapkit.foundation.reassembly`, ``Info`` and -``info_final`` from :mod:`pcapkit.corekit`, a stray ``name`` in -:mod:`pcapkit.protocols`. Exporting those would be wrong, so a blanket assertion -would be a wall of false positives -- and an invitation to silence it by adding -junk to :attr:`__all__`, which is worse than not having it. - -:data:`AGGREGATORS` is where the assertion does hold, exactly, at zero. Those -three packages are pure re-export surfaces: each one wildcard-imports its -children and re-lists their names, so every public attribute it has came from a -child that already chose to export it, and any name missing from its -:attr:`__all__` is an oversight by construction. That is precisely how #515 +**The converse -- every public attribute appears in :attr:`__all__` -- holds for +every public *package*, against a written-down allowlist of exclusions.** That is +issue #533, and scope is what makes it a test rather than noise. + +Applied to every *module* the converse flags 2848 names across 286. That is not a +failing test, it is a wall: a module's namespace holds whatever it imported in +order to do its job, and none of that is its public surface. Applied to the 52 +public *packages* it flags 45 across 12 -- small enough to read, triage, and write +down one at a time. A package's namespace is a composed thing: it holds what its +:file:`__init__.py` chose to pull in, so a public attribute it has and does not +export is a decision in one direction or the other, and the assertion exists to +make that decision explicit instead of leaving it to be guessed at. + +:data:`DELIBERATE_NON_EXPORTS` is where the decisions are recorded, and it is +load-bearing rather than an escape hatch: without it the assertion is +unsatisfiable, and with it every exclusion is something somebody wrote down and a +reviewer can argue with. Of the 45, 23 were genuine omissions and are exported as +of the commit that added this assertion -- among them the six +``register_*_callback`` and ``register_extractor_*`` functions that +:mod:`pcapkit.foundation` withheld while listing the other 27 registry functions, +``ReassemblyManager`` and ``TraceFlowManager``, which are defined *below* the +:attr:`__all__` literal that forgot them and imported by name from six modules, and +the three decorators :mod:`pcapkit.utilities` advertises in its own module +docstring. The 22 that remain are listed with their reasons, and +:meth:`~PublicAPISurfaceTests.test_the_non_export_allowlist_is_tight` stops the +list rotting into a silencer: every entry has to name a package that exists, an +attribute that package really holds, and a name its :attr:`__all__` really omits, +so an entry stops being true the moment the tree moves under it. + +:data:`AGGREGATORS` keeps an assertion of its own even though the package-wide one +now covers those three, because the guarantee is a stronger one: it admits no +allowlist at all. Those three packages are pure re-export surfaces -- each +wildcard-imports its children and re-lists their names, so every public attribute +it has came from a child that already chose to export it, and any name missing from +its :attr:`__all__` is an oversight by construction. That is precisely how #515 happened, and this is the assertion that makes the next one fail immediately instead of in three years. @@ -129,14 +151,80 @@ class exists at ``pcapkit/const/mh/handover_initiate_status.py`` and is used by EXCLUDED_ROOTS = frozenset({'vendor'}) #: The packages whose whole job is to re-export their children, and which -#: therefore have to list every public name they hold. See the module docstring -#: for why the converse assertion is confined to these three. +#: therefore have to list every public name they hold -- with no allowlist, unlike +#: every other package. See the module docstring for why these three are held to +#: the stricter form. AGGREGATORS = ( 'pcapkit.protocols.data', 'pcapkit.protocols.schema', 'pcapkit.foundation.registry', ) +#: Public attributes a public package holds and deliberately does not export, +#: keyed by the package. Adding a name here is an API decision to be argued for in +#: review -- it is not the way to quiet a failure of +#: :meth:`~PublicAPISurfaceTests.test_every_public_package_exports_every_public_attribute`. +#: Three kinds of thing are in here, and nothing else should be. +#: +#: **The abstract bases** -- ``Field``, ``Engine``, ``Reassembly``, +#: ``IP_Reassembly``, ``TraceFlow``, ``Protocol``, ``Application``, ``Internet``, +#: ``Link``, ``Transport``. Each is imported by its package explicitly, under a +#: "Base Class" comment, and left out of that package's :attr:`__all__`; nine +#: packages do the same thing the same way. Neither :mod:`pcapkit.all` -- the +#: library index, whose stated job is to hold "all things from :mod:`pcapkit`" -- +#: nor :mod:`pcapkit.corekit` exports any of them either. Three independent export +#: surfaces agreeing is a convention rather than nine separate oversights, so they +#: stay reachable by their documented dotted path and out of ``import *``. +#: +#: **Names another module's import dragged in** -- ``Info`` and ``info_final`` +#: belong to :mod:`pcapkit.corekit.infoclass`, which is where they are documented +#: and exported from; the reassembly and traceflow packages import them only to +#: build their manager classes. ``TYPE_CHECKING`` is :mod:`typing`'s. Re-exporting +#: any of them would claim ownership of a name this package does not own. +#: +#: **Import-time machinery, and one leaked loop variable.** +#: :mod:`pcapkit` imports ``BaseError``, ``DEVMODE`` and ``DevModeWarning`` to wire +#: up the ``tbtrim`` excepthook, not to publish them -- the top-level surface is +#: interface functions, macros and protocols, and those three are documented under +#: :mod:`pcapkit.utilities`. ``ROOT`` is bound only on the non-``DEVMODE`` branch, +#: so exporting it would make ``from pcapkit import *`` raise +#: :exc:`AttributeError` under ``PCAPKIT_DEVMODE=1`` -- a fresh #515 rather than a +#: fix for one. ``pcapkit.protocols.name`` is the loop variable of the +#: ``for name in __all__`` that builds ``__proto__``, left behind in the namespace; +#: renaming it ``_name`` would express that better than this entry does, but that +#: is a change to a module body rather than to an export list. +DELIBERATE_NON_EXPORTS = { + 'pcapkit': ('BaseError', 'DEVMODE', 'DevModeWarning', 'ROOT'), + 'pcapkit.corekit.fields': ('Field',), + 'pcapkit.foundation.engines': ('Engine',), + 'pcapkit.foundation.reassembly': ('IP_Reassembly', 'Info', 'Reassembly', + 'TYPE_CHECKING', 'info_final'), + 'pcapkit.foundation.traceflow': ('Info', 'TYPE_CHECKING', 'TraceFlow', + 'info_final'), + 'pcapkit.protocols': ('Protocol', 'TYPE_CHECKING', 'name'), + 'pcapkit.protocols.application': ('Application',), + 'pcapkit.protocols.internet': ('Internet',), + 'pcapkit.protocols.link': ('Link',), + 'pcapkit.protocols.transport': ('Transport',), +} + + +def _unlisted_public_attributes(module: 'types.ModuleType') -> 'list[str]': + """Public attributes of ``module`` that its :attr:`__all__` does not name. + + Submodules are skipped. A submodule becomes an attribute of its parent package + as a side effect of being imported, whether or not the parent means to + re-export it, so counting them would flag every package that has children. + + """ + declared = getattr(module, '__all__', ()) + return sorted( + attribute for attribute in dir(module) + if not attribute.startswith('_') + and attribute not in declared + and not isinstance(getattr(module, attribute), types.ModuleType) + ) + def _is_excluded(name: 'str') -> 'bool': """Whether ``name`` -- a dotted module path under ``pcapkit.`` -- is skipped. @@ -325,16 +413,7 @@ def test_aggregators_export_every_public_attribute(self) -> None: # packages re-export their children wholesale, so a public attribute they # hold but do not list is an oversight rather than a judgement call. for name in AGGREGATORS: - module = importlib.import_module(name) - declared = getattr(module, '__all__', ()) - # Submodules are attributes of their parent package as a side effect - # of being imported, and are not part of the re-exported surface. - unlisted = sorted( - attribute for attribute in dir(module) - if not attribute.startswith('_') - and attribute not in declared - and not isinstance(getattr(module, attribute), types.ModuleType) - ) + unlisted = _unlisted_public_attributes(importlib.import_module(name)) with self.subTest(package=name): self.assertEqual( unlisted, [], @@ -342,6 +421,65 @@ def test_aggregators_export_every_public_attribute(self) -> None: f'so `from {name} import *` does not provide them: {unlisted}' ) + def test_every_public_package_exports_every_public_attribute(self) -> None: + # The same converse as above, over all 52 public packages rather than the + # three aggregators, against DELIBERATE_NON_EXPORTS: issue #533. This is the + # only assertion in the class that reads __all__ as the *expectation* -- + # every other one reads it as the input, and an attribute that exists while + # nothing names it satisfies all of them. + for name, is_package in public_modules(): + if not is_package: + continue + allowed = DELIBERATE_NON_EXPORTS.get(name, ()) + unlisted = [attribute + for attribute in _unlisted_public_attributes(importlib.import_module(name)) + if attribute not in allowed] + with self.subTest(package=name): + self.assertEqual( + unlisted, [], + f'{name} holds these public attributes but leaves them out of __all__, ' + f'so `from {name} import *` does not provide them and Sphinx does not ' + f'see them as exported: {unlisted}. Add them if they are meant to be ' + f'public; if the omission is deliberate, record them in ' + f'DELIBERATE_NON_EXPORTS with the reason, which is what makes the ' + f'intent reviewable instead of indistinguishable from an oversight.' + ) + + def test_the_non_export_allowlist_is_tight(self) -> None: + # DELIBERATE_NON_EXPORTS is the whole reason the assertion above can be + # satisfied at all, so it gets an assertion of its own. Any of three kinds of + # staleness turns an entry into an exclusion nobody decided, silently + # excusing the next real omission of that name: the package is gone, the + # attribute is gone, or the name has since been added to __all__ and the + # entry now excuses nothing while looking as though it does. + packages = {name for name, is_package in public_modules() if is_package} + + self.assertEqual(sorted(set(DELIBERATE_NON_EXPORTS) - packages), [], + 'DELIBERATE_NON_EXPORTS names packages that are not in the public ' + 'walk, so those entries exclude nothing') + + absent = {} # type: dict[str, list[str]] + redundant = {} # type: dict[str, list[str]] + for name, allowed in DELIBERATE_NON_EXPORTS.items(): + if name not in packages: + continue + module = importlib.import_module(name) + declared = getattr(module, '__all__', ()) + missing = [symbol for symbol in allowed if not hasattr(module, symbol)] + listed = [symbol for symbol in allowed if symbol in declared] + if missing: + absent[name] = missing + if listed: + redundant[name] = listed + + self.assertEqual(absent, {}, + 'DELIBERATE_NON_EXPORTS names attributes these packages do not have; ' + 'the name was renamed or removed and the entry was left behind') + self.assertEqual(redundant, {}, + 'DELIBERATE_NON_EXPORTS names attributes these packages now export ' + 'anyway, so the entry is dead and should be deleted -- leaving it ' + 'would excuse the name if it were ever dropped from __all__ again') + if __name__ == '__main__': unittest.main()