utilities: let @prepare read length/packet by keyword, not position - #450
Conversation
…444) `@prepare` wraps `Schema.unpack(cls, data, length=None, packet=None)` but read `length`/`packet` out of `args[2]`/`args[3]` unconditionally, so only a three-positional call worked; a shorter call or either optional argument passed by keyword raised `IndexError` instead of falling back to the documented `None` default. - `pcapkit/utilities/decorators.py`: fall back to `kwargs.pop(...)` when `args` is too short, so all five documented call shapes bind correctly. - `pcapkit/corekit/fields/misc.py:539` (`SchemaField.__init__`) calls `schema.unpack(default)` with one argument -- the one caller this broke in-tree -- so `SchemaField(schema=..., default=b'...')` now works as documented. Its `type: ignore[call-arg,misc]` stays: mypy still cannot see through `prepare`'s `Callable[P, R_prepare]` return type, the same reason every other `Schema.unpack` call site in the tree carries the same ignore; confirmed by removing it and rerunning mypy. - `beholder`, the other decorator with positional lookahead, already guards each subscript with `try/except IndexError`, so it does not share the flaw. Adds regression tests for all five call shapes plus the `SchemaField(default=b'...')` case in `tests/utilities/test_decorators.py` and `tests/corekit/test_fields_misc.py`; each fails on the prior code and passes with the fix. Full suite: 922 passed, 17 skipped, 949 subtests passed (0 failed) vs. 916/17/949 on unmodified `main`. mypy: 128 errors in 41 files, unchanged from baseline.
Review of
|
|
Follow-up: the full suite run completed (it was still in progress when I posted the review above, due to 5+ concurrent full-suite runs from other sessions on this shared host).
Zero failures. Total tests = 932 + 17 = 949, consistent with the collection count and with This actually answers the open question from the main review, rather than leaving it open: in a byte-for-byte identical checkout of Doesn't change the verdict: still GOOD TO MERGE on |
…ation - NestedPacketContext was a hand-written class implementing the mapping protocol from scratch, deriving from nothing -- correct, but it left Schema.pack/unpack's "packet: dict[str, Any]" annotation unsatisfied, which is what every other field class's pack/unpack still declares. Widening those annotations to admit the new type cascades through every field class that forwards packet along (ListField, ConditionalField, FieldBase.__call__, pre_process/post_process, ...), well outside this fix's file list, for seven new mypy errors net. - Fix: NestedPacketContext now subclasses dict directly. dict's own metaclass is plain "type", not ABCMeta, so subclassing or instantiating it never touches the shared _abc_impl cache that #439 is about -- confirmed again on the Python 3.10 venv (this test passes both alone and in the full pcapng module: 37 passed, 139 subtests). Being a real dict also nominally satisfies every existing "dict[str, Any]" annotation, so no other file needs to change. - __missing__ gives the enclosing-schema fallback for free, since dict.__getitem__ calls it automatically when a key is absent locally. - __contains__ and get are overridden because the dict built-ins for both bypass __missing__ entirely. - __iter__/__len__/keys/values/items are overridden for the union-of-both- levels semantics the design promises; dict's own versions would only see this instance's local keys. - copy is overridden because dict.copy() always returns a plain dict, even for a subclass, which would silently drop the fallback. - Plain assignment/deletion need no override: dict's own __setitem__/ __delitem__ already only touch this instance's own storage. - Removed the now-genuinely-unused "type: ignore[has-type]" this rewrite exposed on SchemaField.length -- confirmed present and already-unused on a clean origin/main (da24227) checkout too, so this is a pre-existing, unrelated mypy hygiene gap this rewrite happened to touch, not something introduced by it. Net mypy count: 123 (down from main's own 124), with no new errors from this branch's own code. - Fixed a stale citation: the EOFError-on-zero-length Gap entries pointed at decorators.py:222; the actual "raise EOFError" is at :228 (prepare itself starts at :177, and #450 shifted lines since the citation was written). Verified: mypy pcapkit -> 123 errors/40 files (main: 124; the diff is the one pre-existing unused-ignore above, not a new error). pytest tests -q unaffected on Python 3.14. Local Python 3.10 venv: test_pcapng_remaining_constructor_branches_and_custom_dispatch passes alone and as part of the full misc/test_pcapng_unit.py module.
1 participant
Closes #444
The defect
Schema.unpackis documented and typed asunpack(cls, data, length=None, packet=None), both trailing parameters optional (pcapkit/protocols/schema/schema.py:665-667). The@preparedecorator wrapping it readlength/packetout of positionalargsunconditionally and never looked at**kwargs:so of the five documented call shapes, exactly one worked.
Measured, before and after (two-field schema,
wire = b'\x07\x08')unpack(data)IndexError: tuple index out of rangea=7 b=8unpack(data, length)IndexErrorunpack(data, length, packet)unpack(data, length=2)IndexErrorunpack(data, length=2, packet={})IndexErrorThe live casualty
pcapkit/corekit/fields/misc.py:539, inSchemaField.__init__:One positional argument, so
SchemaField(schema=..., default=b'...')— a documented, typed constructor argument — always raisedIndexError. Confirmed both directions:The fix
pcapkit/utilities/decorators.py, inprepare'sunpackwrapper: fall back tokwargs.pop(...)(with the documentedNonedefault) wheneverargsis too short to holdlength/packetpositionally, instead of subscripting unconditionally:This binds the documented signature the way an ordinary wrapper would, for every mix of positional/keyword/omitted trailing arguments, without changing
Schema.unpack's signature or semantics. Every existing in-tree caller passes three positional arguments (e.g.pcapkit/corekit/fields/collections.py:420) and is unaffected.Sibling decorator check
pcapkit/utilities/decorators.pyhas one other decorator with the same positional-lookahead shape:beholder. It already guards each subscript individually:so a short call already falls back correctly — already covered by
test_beholder_defaults_length_when_argument_is_missingand the "noprotoargument at all" case intest_beholder_forwards_the_protocol_number_as_an_alias.seeksetonly ever subscriptsargs[0](self), which every call supplies. Neither shares the flaw; no fix needed there.The
type: ignoreonmisc.py:539Removed it and reran mypy to check, per the issue's suggestion. It is still required: mypy raises two new errors in its absence —
The cause is unrelated to this runtime bug:
prepare's declared return type isCallable[P, R_prepare], which drops theConcatenate[...]fixed parameters from the exposed signature, so mypy statically seesSchema.unpackas taking no arguments aftercls. Every other real call site ofSchema.unpackin the tree carries the identical# type: ignore[call-arg,misc]for the same reason (frame.py:200,pcapng.py:900,protocol.py:306,collections.py:403,420,misc.py:619). Fixing that decorator-typing quirk is a separate, larger change touching every one of those sites, and out of scope here. Restored the ignore; confirmed mypy is back to the 128-error baseline.Tests
tests/utilities/test_decorators.py: one test per call shape in the table above (test_prepare_accepts_data_only,_and_positional_length,_data_length_and_packet_positionally,_keyword_length,_keyword_length_and_packet).tests/corekit/test_fields_misc.py(new file):test_schema_field_accepts_a_bytes_default, theSchemaField(schema=..., default=b'...')regression.Verified both directions: with the decorator fix stashed out, exactly these 5 new tests fail (
IndexError: tuple index out of range) and the rest of the suite is unaffected; with the fix restored, all pass.Suite results
PYTHONSAFEPATH=1,PYTHONPATHat the worktree, interpreter.venv/bin/python3.14.7,pcapkit.__file__printed and confirmed to resolve inside the worktree for every run below.main@1fffb061e(samples generated viaexamples/generators/make_samples.py, which this worktree needed before any suite run succeeded): 916 passed, 17 skipped, 949 subtests passed, 0 failed.maintip (a4c8d62b1, an unrelated concurrent option round-trip test addition): 929 passed, 20 skipped, 1248 subtests passed, 0 failed.mypy (
mypy --follow-imports=silent --ignore-missing-imports --show-column-numbers --show-error-codes pcapkit): 128 errors in 41 files, unchanged from baseline, both before and after rebasing.#446
Confirmed this fix unblocks the minimal standalone reproduction #446 says is blocked by #444. With the fix,
Demo.unpack(b'\x05\x09', 1)against a two-field schema with oneForwardMatchFieldnow runs (previouslyIndexErroron the two-positional-argument shape), and demonstrates #446 directly:len(schema)reports2even though only 1 byte was actually consumed. Not fixing #446 here — leaving it for whoever picks it up, per the issue's note that it's a separateSchema.__len__design question.