From b1124264e913e5d8d83e6f55947c6f359ae191c1 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Sat, 19 Sep 2026 22:18:28 -0400 Subject: [PATCH] fix(tcp): enforce the SACK length rule its docstring already promised (#519) Closes #519. #501 fixed one wrong exception name and forty stale `Args:` labels by hand. #519 showed the class was not exhausted. Re-deriving its census turned up one finding that is not a docstring defect at all, plus a third class the issue never mentions. `TCP._read_mode_sack` documented `ProtocolError: If length is **NOT** multiply of 8 plus 2` and never checked it. The tempting reading -- and the first one taken here -- is that the clause was stale, like the other five phantoms. It is the opposite, and three things settle it: RFC 2018 gives SACK a 2-octet header followed by 8-octet edge pairs; both neighbouring readers validate their own lengths, `_read_mode_sackpmt` on `!= 2` and `_read_mode_echo` on `!= 6`, with the same message; and nothing upstream enforced it either, since the schema's `ListField` consumes as many whole 8-octet items as it finds and ignores the tail. So the docstring was right and the code was wrong. - tcp.py: add the `(length - 2) % 8` check to `_read_mode_sack`, raising `ProtocolError` with the message its two siblings already use. - httpv2.py: drop the `Raises: ProtocolError` clause from the five `_read_http_*` readers that cannot raise it. `_read_http_none` shows the mechanism -- its `raise` is commented out and replaced by a `ProtocolWarning` on the next line. All 11 `_read_http_*` methods carried the clause; the 6 that genuinely raise keep it. - tcp.py, twice: un-indent a `Returns:` header that sat inside the `Args:` block. This is the class the issue missed, and it loses documentation rather than merely misnaming it -- napoleon parses the header as a parameter, so the page rendered `:param Returns:` and carried no `:returns:` at all. Measured through `GoogleDocstring` directly, before and after. - frame.py, schema/application/httpv2.py, schema/misc/pcapng.py: correct four `Args:` labels naming a parameter that does not exist. None of the four takes `**kwargs`, so each is a hard `TypeError`, not a cosmetic slip: `Frame.register(code=..., module=IPv4)` really does raise `TypeError: got an unexpected keyword argument 'module'`, while `protocol=` is accepted. - hip.py: document the parameters `_make_param_reg_response` and `_make_param_route_dst` omit, in the wording their own siblings `_make_param_reg_failed` and `_make_param_route_via` already use. - tests: `test_tcp_sack_length_unit.py` pins the length rule, accepted and rejected cases both. `test_docstring_contract.py` walks every function under `pcapkit/` and checks each documented name against the real signature, each documented exception against the real `raise` statements, and each section header against its indentation. It imports no pcapkit, reading source under its own root, so no editable install can shadow what it measures. Census re-derived at this base rather than taken from the issue, which counted against an older one: phantom Raises: 6 -> 0 remaining Args: naming a missing parameter 13 -> 9 remaining, all allowlisted swallowed section headers 3 -> 1 remaining, allowlisted The `Args:` figure is 13 here and was 14 when #519 was written: #511 retired the IPX socket scrape and renamed that crawler's `soup` parameter to `data`, incidentally fixing one. The nine that remain are in files owned by other in-flight changes and are recorded in `KNOWN_DEFECTS` with a reason each, under a test asserting every one still reproduces -- so the list cannot rot into a description of bugs that are gone. That test has already earned its keep: it failed on exactly the IPX entry during the rebase, which is how the stale entry was found and removed. Be clear about what is provable. A docstring label is not executed, so the corrections cannot fail a test individually -- the contract test is what pins them, by deriving the answer from the code instead of snapshotting today's wrongness. Verified by reintroducing each class into a scratch tree: baseline exit 0, phantom `Raises:` exit 1, bad `Args:` label exit 1, re-indented `Returns:` exit 1. The two `Raises:` assertions are complementary rather than redundant, and `_read_http_none` proves it -- `any(header.flags)` in its body makes the conservative reachability check judge it possibly-raising, so only the commented-out-raise check catches it. Measured: reachability exit 0, commented raise exit 1. The SACK change is the one with a real behavioural fails-before. Lengths 3, 11, 14, 17, 19 and 25 all parsed clean on `c8fd97bcd` with the SACK option present in `tcp.info['options']`, and all six now raise `ProtocolError: TCP: [OptNo 5] invalid format`. Two things deliberately not done. All 12 functions carrying both `**kwargs` and an `Args:` section without documenting `**kwargs` are in hip.py -- 438 of 450 document it elsewhere -- and that set includes the two siblings this change copies its wording from, so fixing 4 of the 12 is what would make hip.py inconsistent. And the SACK check implements exactly the documented rule, not RFC 2018's one-to-four block bound; `length=2` is degenerate and still parses, recorded in the test as a decision. Tests: 5 passed / 10 subtests (contract), 3 passed / 10 subtests (SACK), 25 passed / 19 subtests (tier guard), 81 passed / 73 subtests (transport and schema units, unchanged by the new check). An independent second scanner, written from scratch against the same baseline, reproduced all three counts (6 / 13 / 3) and the residuals (0 / 9 / 1), and turned up three things now recorded in the test module: - `_documented_names` justified its relative-indent measurement by saying `__doc__` is dedented at compile time. True of `__doc__` and irrelevant here, since this module reads `ast.get_docstring(..., clean=False)`, which preserves raw source indentation -- measured [0, 8, 12, 12] against [0, 0, 4, 4] for the dedented forms. The implementation was right for a different reason (nesting depth moves the absolute column); the rationale is corrected rather than left wrong in a checker for wrong rationales. - `_raised_names` resolves only `Name` and `Attribute` raise targets, and `ast.walk` attributes a nested `def`'s raise to the enclosing function. Both can only miss a phantom, never fail a correct docstring, and both are now documented with their measurements: 856 `Name` + 2 `Attribute` + 0 `Subscript` across 858 raise targets, and 11 documented functions with a nested `def`, 3 raising inside it. The second is the correct answer rather than a gap -- `_read_param_locator_set` documents `ProtocolError` and raises none itself, but calls a `_read_locator` helper that does. - One reported defect was a false positive and is recorded as a trap: `Raw.__post_init__` documents `error` and `alias`, has neither in its signature, and assigns a local `alias` -- but forwards `**kwargs` to `read`, which declares both as keyword-only and uses them. Reporting it would ask for correct documentation to be deleted. --- pcapkit/protocols/application/httpv2.py | 15 - pcapkit/protocols/internet/hip.py | 8 + pcapkit/protocols/misc/pcap/frame.py | 2 +- .../protocols/schema/application/httpv2.py | 1 - pcapkit/protocols/schema/misc/pcapng.py | 4 +- pcapkit/protocols/transport/tcp.py | 11 +- .../transport/test_tcp_sack_length_unit.py | 202 +++++++ tests/test_docstring_contract.py | 572 ++++++++++++++++++ 8 files changed, 792 insertions(+), 23 deletions(-) create mode 100644 tests/protocols/transport/test_tcp_sack_length_unit.py create mode 100644 tests/test_docstring_contract.py diff --git a/pcapkit/protocols/application/httpv2.py b/pcapkit/protocols/application/httpv2.py index 5b9afdbf3..aed08d639 100644 --- a/pcapkit/protocols/application/httpv2.py +++ b/pcapkit/protocols/application/httpv2.py @@ -409,9 +409,6 @@ def _read_http_none(self, schema: 'Schema_UnassignedFrame', *, Returns: Parsed packet data. - Raises: - ProtocolError: If the packet is malformed. - """ if any(header.flags): #raise ProtocolError(f'HTTP/2: [Type {frame}] invalid format') @@ -455,9 +452,6 @@ def _read_http_data(self, schema: 'Schema_DataFrame', *, Returns: Parsed packet data. - Raises: - ProtocolError: If the packet is malformed. - """ flag = Data_DataFrameFlags( END_STREAM=bool(header.flags['bit_0']), # bit 0 @@ -510,9 +504,6 @@ def _read_http_headers(self, schema: 'Schema_HeadersFrame', *, Returns: Parsed packet data. - Raises: - ProtocolError: If the packet is malformed. - """ flag = Data_HeadersFrameFlags( END_STREAM=bool(header.flags['bit_0']), # bit 0 @@ -818,9 +809,6 @@ def _read_http_goaway(self, schema: 'Schema_GoawayFrame', *, Returns: Parsed packet data. - Raises: - ProtocolError: If the packet is malformed. - """ data = Data_GoawayFrame( length=header.length, @@ -899,9 +887,6 @@ def _read_http_continuation(self, schema: 'Schema_ContinuationFrame', *, Returns: Parsed packet data. - Raises: - ProtocolError: If the packet is malformed. - """ flag = Data_ContinuationFrameFlags( END_HEADERS=bool(header.flags['bit_2']), # bit 2 diff --git a/pcapkit/protocols/internet/hip.py b/pcapkit/protocols/internet/hip.py index 803be3fa2..de25570e0 100644 --- a/pcapkit/protocols/internet/hip.py +++ b/pcapkit/protocols/internet/hip.py @@ -3916,6 +3916,11 @@ def _make_param_reg_response(self, code: 'Enum_Parameter', param: 'Optional[Data code: parameter code param: parameter data version: HIP protocol version + lifetime: lifetime + reg_response: registration response list + reg_response_default: default registration response + reg_response_namespace: registration response namespace + reg_response_reversed: reverse registration response namespace Returns: HIP parameter schema. @@ -4296,6 +4301,9 @@ def _make_param_route_dst(self, code: 'Enum_Parameter', param: 'Optional[Data_Ro code: parameter code param: parameter data version: HIP protocol version + symmetric: symmetric flag + must_follow: must-follow flag + hit: list of HITs Returns: HIP parameter schema. diff --git a/pcapkit/protocols/misc/pcap/frame.py b/pcapkit/protocols/misc/pcap/frame.py index bdf2f8c7a..7e98692df 100644 --- a/pcapkit/protocols/misc/pcap/frame.py +++ b/pcapkit/protocols/misc/pcap/frame.py @@ -129,7 +129,7 @@ def register(cls, code: 'Enum_LinkType', protocol: 'ModuleDescriptor[Protocol] | Arguments: code: protocol code as in :class:`~pcapkit.const.reg.linktype.LinkType` - module: module descriptor or a + protocol: module descriptor or a :class:`~pcapkit.protocols.protocol.Protocol` subclass """ diff --git a/pcapkit/protocols/schema/application/httpv2.py b/pcapkit/protocols/schema/application/httpv2.py index d02cdb9f6..ff7fdb74f 100644 --- a/pcapkit/protocols/schema/application/httpv2.py +++ b/pcapkit/protocols/schema/application/httpv2.py @@ -130,7 +130,6 @@ def post_process(self, packet: 'dict[str, Any]') -> 'Schema': """Revise ``schema`` data after unpacking process. Args: - schema: parsed schema packet: Unpacked data. Returns: diff --git a/pcapkit/protocols/schema/misc/pcapng.py b/pcapkit/protocols/schema/misc/pcapng.py index 07ba9486c..0a77a9023 100644 --- a/pcapkit/protocols/schema/misc/pcapng.py +++ b/pcapkit/protocols/schema/misc/pcapng.py @@ -211,7 +211,7 @@ def pcapng_block_selector(packet: 'dict[str, Any]') -> 'Field': """Selector function for :attr:`PCAPNG.block` field. Args: - pkt: Packet data. + packet: Packet data. Returns: Returns a :class:`~pcapkit.corekit.fields.misc.SchemaField` @@ -232,7 +232,7 @@ def dsb_secrets_selector(packet: 'dict[str, Any]') -> 'Field': """Selector function for :attr:`DecryptionSecretsBlock.secrets_data` field. Args: - pkt: Packet data. + packet: Packet data. Returns: * If ``secrets_type`` is unknown, returns a diff --git a/pcapkit/protocols/transport/tcp.py b/pcapkit/protocols/transport/tcp.py index fdbbcf623..90017bcdf 100644 --- a/pcapkit/protocols/transport/tcp.py +++ b/pcapkit/protocols/transport/tcp.py @@ -904,6 +904,9 @@ def _read_mode_sack(self, schema: 'Schema_SACK', *, options: 'Option') -> 'Data_ ProtocolError: If length is **NOT** multiply of ``8`` plus ``2``. """ + if (schema.length - 2) % 8 != 0: + raise ProtocolError(f'{self.alias}: [OptNo {schema.kind}] invalid format') + data = Data_SACK( kind=schema.kind, length=schema.length, @@ -2976,8 +2979,8 @@ def _make_mptcp_fail(self, subtype: 'Enum_MPTCPOption', opt: 'Optional[Data_MPTC dsn: data sequence number **kwargs: arbitrary keyword arguments - Returns: - Constructed option schema. + Returns: + Constructed option schema. """ if opt is not None: @@ -3003,8 +3006,8 @@ def _make_mptcp_fastclose(self, subtype: 'Enum_MPTCPOption', opt: 'Optional[Data key: option receiver's key **kwargs: arbitrary keyword arguments - Returns: - Constructed option schema. + Returns: + Constructed option schema. """ if opt is not None: diff --git a/tests/protocols/transport/test_tcp_sack_length_unit.py b/tests/protocols/transport/test_tcp_sack_length_unit.py new file mode 100644 index 000000000..9cf41ae07 --- /dev/null +++ b/tests/protocols/transport/test_tcp_sack_length_unit.py @@ -0,0 +1,202 @@ +# -*- coding: utf-8 -*- +"""The TCP SACK option's length constraint, which was documented but not checked. + +GitHub issue #519 counted :meth:`TCP._read_mode_sack +` among its phantom +``Raises:`` clauses: the docstring promised ``ProtocolError: If length is +**NOT** multiply of 8 plus 2`` and the body contained no such check. The +tempting reading is that the clause was stale and should be deleted, which is +what the other five phantoms in that issue needed. + +It was the opposite. The clause was right and the check was missing, and three +pieces of evidence settle it: + +* :rfc:`2018` gives the SACK option a 2-octet header followed by 8-octet + left/right edge pairs, so a well-formed option's length really is ``8n + 2``. +* The sibling readers validate their own lengths rather than delegating it. + :meth:`TCP._read_mode_sackpmt + ` -- the method + immediately above this one -- raises on ``schema.length != 2``, and + :meth:`TCP._read_mode_echo ` + on ``schema.length != 6``, both with the same message. ``_read_mode_sack`` + was the one option reader documenting a length rule it never enforced. +* Nothing upstream enforced it either. A segment carrying ``kind=5, + length=11`` parsed clean before the fix: the schema field is + ``ListField(length=lambda pkt: pkt['length'] - 2, + item_type=SchemaField(length=8, schema=SACKBlock))``, and a remainder is + simply not noticed. That is what :meth:`SACKLengthTests + .test_invalid_sack_length_is_rejected` pins. + +So this module is the behavioural half of the #519 work. The other half is +:file:`tests/test_docstring_contract.py`, which stops a docstring from drifting +away from the code again; this one stops the code from drifting away from +*this* docstring, which is the direction that actually lets a malformed packet +through. + +One deliberate limit, so a later reader does not think it an oversight: the +check implements exactly the rule the docstring states, ``(length - 2) % 8 == +0``. :rfc:`2018` also wants at least one block and at most four, so ``length=2`` +is degenerate and ``length=42`` is too long, and neither is rejected here. +Tightening past the documented contract would change what the parser accepts on +the strength of a test rather than of the specification it cites, so it is +recorded in :meth:`SACKLengthTests.test_documented_rule_is_the_implemented_rule` +instead of being quietly added. + +Every case builds its own octets in memory and reads no capture under +:file:`examples/captures/`, so this belongs to the unit tier. + +""" +from __future__ import annotations + +import io +import unittest + +from pcapkit.protocols.transport.tcp import TCP +from pcapkit.utilities.exceptions import FieldValueError, ProtocolError + +#: TCP option kind for SACK, per :rfc:`2018`. +SACK = 5 +#: TCP option kind for a single-octet no-op, used to pad the options area out +#: to a 4-octet boundary so the data offset stays legal. +NOP = 1 + + +def sack_option(length: 'int', payload: 'int') -> 'bytes': + """A SACK option declaring ``length`` and carrying ``payload`` data octets. + + ``length`` and ``payload`` are set independently on purpose: the defect + being pinned is a *declared* length that does not match the 8-octet block + structure, so the test has to be able to declare one thing and supply + another. + + """ + return bytes([SACK, length]) + bytes(payload) + + +def segment(option: 'bytes') -> 'bytes': + """A minimal ACK segment whose options area is ``option``, NOP-padded.""" + while len(option) % 4: + option += bytes([NOP]) + offset = (20 + len(option)) // 4 + header = ( + (1234).to_bytes(2, 'big') # source port + + (80).to_bytes(2, 'big') # destination port + + (0).to_bytes(4, 'big') # sequence number + + (0).to_bytes(4, 'big') # acknowledgement number + + bytes([offset << 4, 0x10]) # data offset, and ACK set + + (8192).to_bytes(2, 'big') # window + + (0).to_bytes(2, 'big') # checksum + + (0).to_bytes(2, 'big') # urgent pointer + ) + return header + option + + +def parse(option: 'bytes') -> 'TCP': + """Parse a segment carrying ``option``.""" + raw = segment(option) + return TCP(io.BytesIO(raw), len(raw)) + + +class SACKLengthTests(unittest.TestCase): + """A SACK option's declared length against its block structure.""" + + def test_valid_sack_lengths_are_accepted(self) -> 'None': + """``8n + 2`` parses, for one through four blocks. + + The companion to the rejection test: a length rule enforced too + eagerly would break every real SACK-bearing segment, so the accepted + cases are pinned as tightly as the rejected ones. + + """ + for blocks in range(1, 5): + length = 2 + 8 * blocks + with self.subTest(blocks=blocks, length=length): + tcp = parse(sack_option(length, 8 * blocks)) + self.assertIn(SACK, [int(key) for key in tcp.info['options']]) + + def test_invalid_sack_length_is_rejected(self) -> 'None': + """A length that is not ``8n + 2`` is rejected. + + Every one of these parsed without complaint before the check was + added, which is the whole point of the case. ``11`` and ``14`` are the + interesting ones -- a remainder of 1 and of 4 -- because the block list + happily consumes as many whole 8-octet items as it can find and + silently ignores the tail. + + Two different exceptions satisfy this, and which one arrives depends on + global state rather than on the packet, so the assertion deliberately + accepts either: + + * :exc:`~pcapkit.utilities.exceptions.ProtocolError` from + :meth:`TCP._read_mode_sack + `, the check + this module exists for. This is what a freshly started interpreter + produces for all six lengths. + * :exc:`~pcapkit.utilities.exceptions.FieldValueError` from + :meth:`ListField.unpack + `, whose schema + branch decrements the remaining length by ``len(data)`` per item and + raises once it goes negative. This one was observed in a process that + had already run other parts of the suite, where the option schema is + unpacked before ``_read_mode_sack`` is reached, so the lower layer + notices first and reports instead. + + Be careful how much that second bullet is trusted. The *observation* is + real and is filed as #525, but the trigger is not pinned down: a + deliberate attempt to force it by running + :file:`tests/protocols/schema/` first in the same process did **not** + reproduce it, and ``ProtocolError`` still won for all six lengths. So + the honest statement is that the exception class depends on process + state by some route not yet identified -- not that any particular test + ordering selects it. + + Which is why the assertion is a union rather than a single class: a + caller cannot reliably catch one, and pinning whichever one happens to + arrive here would make this test fail for reasons that have nothing to + do with SACK. Naming both rather than a bare + :exc:`~pcapkit.utilities.exceptions.BaseError` still keeps a third, + unexpected exception a failure. The union records the problem; #525 + owns solving it. + + """ + for length in (3, 11, 14, 17, 19, 25): + with self.subTest(length=length): + with self.assertRaises((ProtocolError, FieldValueError)) as caught: + parse(sack_option(length, length + 8)) + self.assertRegex(str(caught.exception), 'invalid (format|length)') + + def test_documented_rule_is_the_implemented_rule(self) -> 'None': + """The check is exactly ``(length - 2) % 8 == 0``, no more. + + :rfc:`2018` also bounds the block count at one to four, and the + implementation deliberately does not reach past its own docstring to + enforce that. Only one such case is actually reachable, though, and + working out why is what makes the missing bound harmless: + + * ``length=2`` -- no blocks at all. Satisfies the modulo rule, is not + well formed under :rfc:`2018`, and parses. This is the one real gap, + recorded here as a decision rather than left silent. + * five blocks or more cannot be expressed at all, which is why + :rfc:`2018` stops at four. The TCP data offset is four bits, so a + header is at most ``15 * 4 == 60`` octets and the options area at + most 40. A five-block SACK needs ``2 + 40 == 42`` octets of option, + and 44 once padded, which no legal data offset can describe -- + :func:`segment` cannot even build one. The upper bound is therefore + enforced by the header format rather than by a check, and adding one + would be unreachable code. + + So if a lower bound is ever added, this test is what should fail and be + rewritten. + + """ + tcp = parse(sack_option(2, 0)) + self.assertIn(SACK, [int(key) for key in tcp.info['options']]) + + # five blocks: 2 + 8*5 == 42 octets, 44 padded, needing a data offset + # of 16 where the field holds a maximum of 15. + with self.assertRaises(ValueError): + segment(sack_option(42, 40)) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/test_docstring_contract.py b/tests/test_docstring_contract.py new file mode 100644 index 000000000..62145f56a --- /dev/null +++ b/tests/test_docstring_contract.py @@ -0,0 +1,572 @@ +# -*- coding: utf-8 -*- +"""Every ``Args:`` name and ``Raises:`` clause, checked against the code itself. + +GitHub issue #501 fixed one wrong exception name and forty stale ``Args:`` +labels by hand. Issue #519 then showed the class was not exhausted -- more +phantom ``Raises:`` clauses and more ``Args:`` entries naming parameters that do +not exist -- because nothing in the suite *derives* the answer from the code. +A docstring label is not executed, so no existing test could notice that +:meth:`Frame.register ` +documented a ``module`` argument it does not accept. + +That is what this module closes, the same way +:file:`tests/protocols/test_dispatch_registry_unit.py` closed the dispatch-table +gap: the checks walk every function under :file:`pcapkit/` and compare each +docstring against the real signature and the real ``raise`` statements, so a +docstring written tomorrow is checked tomorrow rather than being a snapshot of +what was wrong in 2026. + +Three properties are asserted, and the split between them is deliberate -- +each is sound on its own, and the softer judgements are recorded rather than +asserted: + +:meth:`DocstringParameterTests.test_documented_parameters_exist` + A documented name that is not a parameter *and* has no ``**kwargs`` to + absorb it is a guaranteed :exc:`TypeError` at call time. No intent needs to + be guessed: ``Frame.register(code=..., module=...)`` simply fails. This is + the invariant with no false positives, so it is the one asserted hardest. + +:meth:`DocstringParameterTests.test_section_headers_are_not_swallowed` + A napoleon section header indented *inside* the parameter block is parsed + by napoleon as a parameter entry, so ``Returns:`` over-indented by four + spaces renders as ``:param Returns:`` **and the return documentation + disappears from the rendered page**. Sphinx reports nothing. + +:meth:`DocstringRaisesTests.test_documented_exceptions_are_reachable` + An exception documented by a function that cannot raise it. The check is + deliberately conservative -- see :func:`_reachable` -- because a false + positive here would make the suite fail on a correct docstring, which is + worse than missing one. + +Why a documented name may legitimately be absent from the signature, which is +the trap this module has to avoid: this project documents keyword arguments +consumed from ``**kwargs``, and does so correctly. +:meth:`Protocol.__init__ ` +documents ``_layer`` and ``_protocol`` and really does read them +(``kwargs.pop('_layer', None)``), and :meth:`Frame.unpack +` documents ``\\_seek_set`` as +"forwarded to :meth:`self.read ` through ``**kwargs``", which is exactly +true. Deleting either would destroy real documentation, so the presence of +``**kwargs`` exonerates a name here and those cases are excluded by +construction rather than by being listed. + +:meth:`Raw.__post_init__ ` shows +how sharp that trap is, and it caught a reviewing scanner during this change. +It documents ``error`` and ``alias``, has neither in its signature, and its body +assigns a *local* ``alias`` from ``self._info.protocol.name`` -- which reads +exactly like a docstring naming a parameter that does not exist. It is not: +``__post_init__`` forwards ``**kwargs`` to :meth:`~pcapkit.protocols.misc.raw.Raw.unpack`, +which dispatches to :meth:`Raw.read `, and +``read`` declares both as keyword-only parameters and uses them. The local +variable merely shares a name with the keyword. A checker that reported this +would be asking for correct documentation to be deleted, which is the outcome +the ``**kwargs`` exclusion exists to prevent. + +That exclusion has a known cost, recorded so it is not mistaken for coverage. +:meth:`Reassembly.__init_subclass__ +` and +:meth:`TraceFlow.__init_subclass__ +` both +document ``name`` where the parameter is ``protocol``, and both have +``**kwargs``, so both are invisible here even though their bodies use +``protocol`` and never read ``kwargs['name']``. Catching those needs the intent +behind a keyword, not just its absence from the signature -- ``src_ip`` and +``dst_ip`` on :meth:`IPv6_Route.__post_init__ +` look +identical and are very likely legitimate keywords forwarded to ``super()``. +Guessing between the two would fail correct docstrings, so this module does not +guess. + +Note the RST escape in that last one. ``\\_seek_set`` is written with a +backslash so Sphinx does not read the leading underscore as emphasis, which is +why :func:`_documented_names` normalises ``\\_`` before comparing: a plain +string match drops that entry and then reports a defect that is not there. + +:data:`KNOWN_DEFECTS` carries the findings this change did not fix, each with +the reason. :meth:`DocstringParameterTests.test_known_defects_are_still_defects` +asserts every one of them still reproduces, so the list cannot rot -- when the +owning change lands, that test fails and the entry has to be removed rather +than sitting there forever describing a bug that is gone. + +That is not a hypothetical. The list was written against ``980e52f0c`` and had +an entry for ``pcapkit/vendor/ipx/socket.py``'s ``process``, which documented +``data`` where the parameter was ``soup``. Rebasing this change onto +``c8fd97bcd`` picked up #511, which retired the HTML scrape and renamed that +parameter to ``data`` as a side effect -- so the defect was gone, and this test +failed on exactly that subtest and nothing else. The entry was removed. Worth +knowing that the failure is reported through :meth:`~unittest.TestCase.subTest`, +and ``pytest-subtests`` is not a dependency here, so pytest prints the parent +test as ``PASSED`` while exiting non-zero: read the exit code, not the summary +line. + +Everything here reads :file:`pcapkit/` source and imports nothing but the +standard library, so it reads no capture under :file:`examples/captures/` and +belongs to the unit tier. + +""" +from __future__ import annotations + +import ast +import pathlib +import unittest +from typing import TYPE_CHECKING, NamedTuple + +from tests._tiers import ROOT + +if TYPE_CHECKING: + from typing import Iterator, Optional + +#: Package whose docstrings are checked. +PACKAGE = ROOT / 'pcapkit' + +#: Napoleon headers that open a parameter block. ``Args`` is the house +#: spelling, but ``Arguments`` is used 200-odd times and ``Parameters`` and +#: ``Keyword Args`` a handful more, and a checker that matched only ``Args`` +#: would silently skip every one of them -- which is how a census of this +#: class undercounts by a tenth. +PARAM_SECTIONS = frozenset({ + 'Args', 'Arguments', 'Parameters', 'Keyword Args', 'Keyword Arguments', +}) + +#: Every other napoleon section header. Used twice: to stop a parameter block +#: at the next section, and to notice one that has been indented *into* the +#: parameter block by mistake. +OTHER_SECTIONS = frozenset({ + 'Attributes', 'Example', 'Examples', 'Note', 'Notes', 'Raises', + 'References', 'Return', 'Returns', 'See Also', 'Todo', 'Warning', + 'Warnings', 'Warns', 'Yield', 'Yields', +}) + + +class Finding(NamedTuple): + """One docstring defect, keyed so it survives the lines moving.""" + + #: Path relative to the repository root, e.g. ``pcapkit/vendor/ipx/packet.py``. + module: 'str' + #: The function's own name, not its qualified name -- ``ast`` gives this + #: cheaply and it is unique enough within a module to identify the site. + function: 'str' + #: The documented name, or the swallowed section header, or the exception. + subject: 'str' + + +class Known(NamedTuple): + """A :class:`Finding` left unfixed, and why.""" + + finding: 'Finding' + #: Why it is still here. A bare list of defects rots into a list of + #: things nobody remembers; the reason is what makes an entry reviewable. + reason: 'str' + + +#: Defects this change deliberately left alone, every one because the file +#: belongs to another concurrent change and editing it would silently discard +#: that work. The corrections are recorded in the issue thread; nothing here +#: is a claim that the docstring is right. +KNOWN_DEFECTS = ( + Known(Finding('pcapkit/foundation/registry/foundation.py', + 'register_extractor_engine', 'engine'), + "documents 'engine' where the parameter is 'name'; owned by the " + 'registry docstring change'), + Known(Finding('pcapkit/protocols/internet/ipv4.py', + '_make_ipv4_options', 'option'), + "documents 'option' where the parameter is 'options'; ipv4.py is " + 'owned by another change'), + Known(Finding('pcapkit/vendor/ipx/packet.py', 'process', 'data'), + "documents 'data' where the parameter is 'soup'; pcapkit/vendor is " + 'generated-adjacent and owned elsewhere. Note the sibling ' + 'pcapkit/vendor/ipx/socket.py carried the identical defect and no ' + 'longer does, so that umbrella covers the file rather than the ' + 'directory -- see the module docstring'), + Known(Finding('pcapkit/vendor/mh/binding_ack_flag.py', 'context', 'soup'), + "documents 'soup: Parsed HTML source.' where the parameter is 'data' " + "and holds CSV rows. Nothing was renamed here despite the " + '``# pylint: disable=arguments-renamed`` pragma -- the parameter ' + 'matches the base ``Vendor.context(self, data)`` in ' + 'pcapkit/vendor/default.py:288, which documents it as ' + "'data: CSV data.'. The line was copy-pasted from a sibling whose " + "process()/context() really does take 'soup', so both the name and " + 'the description are wrong'), + Known(Finding('pcapkit/vendor/mh/binding_update_flag.py', 'context', 'soup'), + "documents 'soup' where the parameter is 'data'"), + Known(Finding('pcapkit/vendor/mh/handover_ack_flag.py', 'context', 'soup'), + "documents 'soup' where the parameter is 'data'"), + Known(Finding('pcapkit/vendor/mh/handover_initiate_flag.py', 'context', 'soup'), + "documents 'soup' where the parameter is 'data'"), + Known(Finding('pcapkit/vendor/pcapng/option_type.py', 'context', 'data'), + "documents 'data: CSV data.' where the parameter is 'soup' and holds " + 'parsed HTML -- the name and the description are both wrong'), + Known(Finding('pcapkit/vendor/vlan/priority_level.py', 'process', 'data'), + "documents 'data' where the parameter is 'soup'"), +) + +#: Swallowed section headers left unfixed, for the same ownership reason. +KNOWN_SWALLOWED = ( + Known(Finding('pcapkit/protocols/internet/ipv4.py', '_make_opt_e_sec', 'Returns'), + 'the ``Returns:`` header is indented into the ``Args:`` block, so ' + 'napoleon renders ``:param Returns:`` and drops the return ' + 'documentation; ipv4.py is owned by another change'), +) + + +def _header(line: 'str') -> 'Optional[str]': + """The section name ``line`` opens, or :obj:`None`.""" + stripped = line.strip() + return stripped[:-1].strip() if stripped.endswith(':') else None + + +def _documented_names(doc: 'str') -> 'tuple[list[tuple[str, bool]], list[str]]': + """Parameter entries and swallowed section headers in ``doc``. + + Returns a list of ``(name, was_given_an_explicit_type)`` pairs and a list + of section headers found at entry depth. + + Indentation is measured relative to the section header rather than against + a fixed column, because the absolute column depends on how deeply the + function is nested: a module-level function carries ``Args:`` at four + spaces and its entries at eight, a method carries them at eight and twelve. + A checker keyed on the absolute column matches one and silently skips the + other. + + It is *not* for the reason an earlier draft of this docstring gave, which + said ``__doc__`` is dedented at compile time so the source and runtime + columns differ. That is true of ``__doc__`` and irrelevant here, because + :func:`functions` reads ``ast.get_docstring(node, clean=False)``, which + preserves the raw source indentation. Measured on 3.14.7 for a method whose + ``Args:`` sits at eight spaces:: + + get_docstring(clean=False) -> indents [0, 8, 12, 12] + get_docstring(clean=True) -> indents [0, 0, 4, 4] + compiled __doc__ -> indents [0, 0, 4, 4] + + So this module never sees the dedented form at all. The relative + measurement is right, but for the nesting reason above rather than that + one -- recorded because a wrong rationale in a checker for wrong + rationales is worth correcting explicitly. + + """ + lines = doc.splitlines() + names = [] # type: list[tuple[str, bool]] + swallowed = [] # type: list[str] + index = 0 + while index < len(lines): + if _header(lines[index]) not in PARAM_SECTIONS: + index += 1 + continue + base = len(lines[index]) - len(lines[index].lstrip()) + index += 1 + while index < len(lines): + line = lines[index] + if not line.strip(): + index += 1 + continue + indent = len(line) - len(line.lstrip()) + if indent <= base: + break + if indent == base + 4 and ':' in line: + label = line.split(':', 1)[0].strip() + if label in OTHER_SECTIONS: + swallowed.append(label) + index += 1 + continue + # ``\_seek_set`` is the RST escape for a leading underscore. + name = label.replace('\\_', '_') + typed = name.endswith(')') and '(' in name + if typed: + name = name.split('(')[0].strip() + name = name.lstrip('*') + if name and ' ' not in name: + names.append((name, typed)) + index += 1 + return names, swallowed + + +def _documented_exceptions(doc: 'str') -> 'list[str]': + """Exception names in ``doc``'s ``Raises:`` section.""" + lines = doc.splitlines() + found = [] # type: list[str] + index = 0 + while index < len(lines): + if _header(lines[index]) != 'Raises': + index += 1 + continue + base = len(lines[index]) - len(lines[index].lstrip()) + index += 1 + while index < len(lines): + line = lines[index] + if not line.strip(): + index += 1 + continue + indent = len(line) - len(line.lstrip()) + if indent <= base: + break + if indent == base + 4 and ':' in line: + name = line.split(':', 1)[0].strip() + if name and ' ' not in name: + found.append(name.rsplit('.', 1)[-1]) + index += 1 + return found + + +def functions() -> 'Iterator[tuple[str, ast.FunctionDef | ast.AsyncFunctionDef, str]]': + """Every documented function under :file:`pcapkit/`, with its module path.""" + for path in sorted(PACKAGE.rglob('*.py')): + try: + tree = ast.parse(path.read_text(encoding='utf-8'), str(path)) + except (SyntaxError, UnicodeDecodeError): # pragma: no cover + continue + relative = path.relative_to(ROOT).as_posix() + for node in ast.walk(tree): + if not isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + continue + doc = ast.get_docstring(node, clean=False) + if doc: + yield relative, node, doc + + +def _raised_names(node: 'ast.AST') -> 'set[str]': + """Exception names raised directly in ``node``'s body. + + Two limits, both measured rather than assumed, and both able only to *miss* + a phantom -- neither can fail a correct docstring: + + * Only :class:`ast.Name` and :class:`ast.Attribute` raise targets are + resolved, so a computed ``raise REGISTRY[code]`` would register neither as + a matching raise nor, in :func:`_calls_anything_callable`, as a call. + Currently inert: across ``pcapkit/`` the 858 raise targets break down as + 856 ``Name`` and 2 ``Attribute``, and **no** ``Subscript``. + * :func:`ast.walk` descends into nested ``def``\\ s, so a ``raise`` inside a + local helper is attributed to the function enclosing it. 11 documented + functions contain a nested ``def`` and 3 of those raise inside it. + + That second one reads like a bug and is in fact the right answer here, which + is why it is left alone. In all three cases the nested ``def`` is a local + helper the enclosing body actually calls, so the exception really does + propagate out of the enclosing function: :meth:`HIP._read_param_locator_set + ` documents + ``ProtocolError`` and raises none itself, but its ``_read_locator`` helper + does and it calls that helper once per locator. Attributing the raise + outward is therefore correct, and the clause is not a phantom. It would only + mislead for a nested ``def`` that is *returned* rather than called, which + this package does not currently do. + + """ + raised = set() # type: set[str] + for sub in ast.walk(node): + if not isinstance(sub, ast.Raise) or sub.exc is None: + continue + exc = sub.exc + if isinstance(exc, ast.Call): + exc = exc.func + if isinstance(exc, ast.Name): + raised.add(exc.id) + elif isinstance(exc, ast.Attribute): + raised.add(exc.attr) + return raised + + +def _calls_anything_callable(node: 'ast.AST') -> 'bool': + """Whether the body calls a method or free function that could raise. + + Data-model construction is excluded: this project's ``_read_*`` methods + build :class:`~pcapkit.corekit.infoclass.Info` subclasses whose names begin + ``Data_`` or ``Schema_``, and neither those nor :meth:`Info.__update__` can + raise a protocol error -- ``ProtocolError`` appears nowhere in + :file:`pcapkit/corekit/` outside one comment. Anything else counts as + possibly-raising, which is what keeps :func:`_reachable` conservative. + + """ + for sub in ast.walk(node): + if not isinstance(sub, ast.Call): + continue + func = sub.func + if isinstance(func, ast.Name): + name = func.id + elif isinstance(func, ast.Attribute): + name = func.attr + else: # pragma: no cover + continue + if name.startswith(('Data_', 'Schema_')): + continue + if name in ('bool', 'int', 'str', 'bytes', 'len', 'tuple', 'list', + 'dict', 'set', 'cast', 'warn', '__update__'): + continue + return True + return False + + +def _reachable(node: 'ast.AST', exception: 'str') -> 'bool': + """Whether ``exception`` might be raised from ``node``. + + Answers "might", not "is": :obj:`True` is returned whenever the body calls + anything whose own body is not inspected here, so the only way to get + :obj:`False` is a body that raises nothing and calls nothing but data-model + construction. Every phantom this catches is therefore a function whose + whole body is visible, which is the case that needs no interprocedural + analysis to settle -- and a docstring is never failed on a guess. + + """ + raised = _raised_names(node) + if exception in raised or 'Exception' in raised: + return True + if any(isinstance(sub, ast.Raise) and sub.exc is None for sub in ast.walk(node)): + return True # bare ``raise`` re-raises whatever was caught + return _calls_anything_callable(node) + + +def parameter_defects() -> 'list[Finding]': + """Documented names that are not parameters and have no ``**kwargs``.""" + defects = [] # type: list[Finding] + for module, node, doc in functions(): + args = node.args + real = {arg.arg for arg in (args.posonlyargs + args.args + args.kwonlyargs)} + if args.vararg: + real.add(args.vararg.arg) + if args.kwarg: + # ``**kwargs`` absorbs any keyword, so a name that is missing from + # the signature may still be a keyword this function or one it + # forwards to really consumes. Not decidable here, and wrongly + # failing a correct docstring is the worse error. + continue + seen = set() # type: set[str] + for name, _ in _documented_names(doc)[0]: + if name in real or name in ('self', 'cls') or name in seen: + continue + seen.add(name) + defects.append(Finding(module, node.name, name)) + return defects + + +def swallowed_headers() -> 'list[Finding]': + """Section headers indented inside a parameter block.""" + return [Finding(module, node.name, label) + for module, node, doc in functions() + for label in _documented_names(doc)[1]] + + +def unreachable_exceptions() -> 'list[Finding]': + """Documented exceptions the function demonstrably cannot raise.""" + return [Finding(module, node.name, exception) + for module, node, doc in functions() + for exception in _documented_exceptions(doc) + if not _reachable(node, exception)] + + +class DocstringParameterTests(unittest.TestCase): + """``Args:`` entries against the real signatures.""" + + def test_documented_parameters_exist(self) -> 'None': + """No function documents a parameter it cannot accept. + + Restricted to functions without ``**kwargs``, where passing the + documented name is a guaranteed :exc:`TypeError` and no intent has to + be inferred. + + """ + allowed = {known.finding for known in KNOWN_DEFECTS} + unexpected = [finding for finding in parameter_defects() if finding not in allowed] + self.assertEqual(unexpected, [], '\n'.join( + ['%d docstring(s) name a parameter that does not exist:' % len(unexpected)] + + [' %s %s() documents %r' % finding for finding in unexpected])) + + def test_section_headers_are_not_swallowed(self) -> 'None': + """No napoleon section header sits inside a parameter block. + + An over-indented ``Returns:`` renders as ``:param Returns:`` and the + return documentation is lost from the page without any warning. + + """ + allowed = {known.finding for known in KNOWN_SWALLOWED} + unexpected = [finding for finding in swallowed_headers() if finding not in allowed] + self.assertEqual(unexpected, [], '\n'.join( + ['%d section header(s) indented into a parameter block:' % len(unexpected)] + + [' %s %s() swallowed %r' % finding for finding in unexpected])) + + def test_known_defects_are_still_defects(self) -> 'None': + """Every :data:`KNOWN_DEFECTS` entry still reproduces. + + This is what stops the list rotting. When the change that owns one of + these files corrects the docstring, this test fails and the entry must + be deleted -- rather than staying here describing a bug that is gone + and quietly excusing a new one that is not. + + """ + for group, finder in ((KNOWN_DEFECTS, parameter_defects), + (KNOWN_SWALLOWED, swallowed_headers)): + current = set(finder()) + for known in group: + with self.subTest(entry=known.finding): + self.assertIn(known.finding, current, + 'fixed, so remove this entry -- %s' % known.reason) + + +class DocstringRaisesTests(unittest.TestCase): + """``Raises:`` clauses against the real ``raise`` statements.""" + + def test_documented_exceptions_are_reachable(self) -> 'None': + """No function documents an exception it cannot raise.""" + unexpected = unreachable_exceptions() + self.assertEqual(unexpected, [], '\n'.join( + ['%d phantom Raises: clause(s):' % len(unexpected)] + + [' %s %s() documents %r' % finding for finding in unexpected])) + + def test_commented_out_raise_leaves_no_clause_behind(self) -> 'None': + """A commented-out ``raise`` never keeps its ``Raises:`` clause. + + :meth:`HTTP._read_http_none + ` is where + this went wrong: the ``raise ProtocolError(...)`` was commented out and + replaced by a :class:`~pcapkit.utilities.warnings.ProtocolWarning`, + and the ``Raises:`` clause stayed. Downgrading a raise to a warning is + a deliberate act, so the docstring beside it gets checked. + + This check is not redundant with + :meth:`DocstringRaisesTests.test_documented_exceptions_are_reachable`, + and ``_read_http_none`` is precisely the case that proves it. Of the six + phantom clauses #519 counted, that reachability check finds only five: + ``_read_http_none``'s body opens ``if any(header.flags):``, and ``any`` + is not in :func:`_calls_anything_callable`'s inert-call list, so the + function is judged possibly-raising and is never reported. Widening that + list until it caught this one would be the wrong fix -- every name added + to it is a promise that nothing behind that name raises, which is how a + conservative checker turns into one that fails correct docstrings. The + commented-out ``raise`` is much stronger evidence than the absence of a + live one, so it gets its own assertion. Delete either test and one of + the six stops being covered. + + """ + stale = [] # type: list[str] + for path in sorted(PACKAGE.rglob('*.py')): + lines = path.read_text(encoding='utf-8').splitlines() + commented = {} # type: dict[int, str] + for number, line in enumerate(lines, 1): + stripped = line.strip() + if not stripped.startswith('#'): + continue + body = stripped.lstrip('#').strip() + if body.startswith('raise ') and '(' in body: + commented[number] = body[len('raise '):].split('(')[0].strip() + if not commented: + continue + relative = path.relative_to(ROOT).as_posix() + tree = ast.parse('\n'.join(lines), str(path)) + for node in ast.walk(tree): + if not isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + continue + doc = ast.get_docstring(node, clean=False) + if not doc: + continue + end = getattr(node, 'end_lineno', node.lineno) + documented = set(_documented_exceptions(doc)) + for number, exception in commented.items(): + if not node.lineno <= number <= end: + continue + if exception in documented and exception not in _raised_names(node): + stale.append('%s:%d %s() documents %r but its only ' + 'raise is commented out' + % (relative, number, node.name, exception)) + self.assertEqual(stale, [], '\n'.join(['stale clause(s):'] + stale)) + + +if __name__ == '__main__': + unittest.main()