Repository navigation
Seven register/issubclass guards in the protocol classes still leak a bare TypeError for a non-class #1026
Description
All seven register classmethods in pcapkit/protocols/ carry a bare if not issubclass(x, ProtocolBase): with no isinstance(x, type) ahead of it, so a non-class argument is rejected by issubclass before the guard's own raise is reached and the caller gets TypeError: issubclass() arg 1 must be a class leaked out of abc instead of the library's own exception.
Measured on main, raise site read off traceback.extract_tb(...)[-1] rather than off the message:
| Site | |
|---|---|
pcapkit/protocols/protocol.py:768 |
ProtocolBase.register |
pcapkit/protocols/misc/pcap/frame.py:151 |
Frame.register |
pcapkit/protocols/misc/pcapng.py:919 |
PCAPNG.register |
pcapkit/protocols/transport/sctp.py:628 |
SCTP.register |
pcapkit/protocols/link/link.py:143 |
Link.register |
pcapkit/protocols/internet/internet.py:163 |
Internet.register |
pcapkit/protocols/transport/transport.py:114 |
Transport.register |
Transport.register is included, and an earlier version of this body wrongly excluded it. Its UnsupportedCall at transport.py:110 is gated on if cls is Transport:, so it fires only for a literal call on the abstract class — which is never how it is reached. The guard below it leaks through the concrete subclasses:
Transport.register -> UnsupportedCall | transport.py:110
TCP.register -> TypeError | <frozen abc>:123
UDP.register -> TypeError | <frozen abc>:123
grep -rn "if not issubclass" pcapkit/ finds 13 sites: six are fixed in #1025, and these seven remain.
The per-site question
The maintainer's ruling on #1021 was "code follows prose": add an explicit class check so callers get the registry's own exception. That ruling was scoped to the register_* module functions, and these are a different surface — register classmethods on protocol classes, keyed by code.
Measured: all seven Raises: clauses already promise RegistryError for "not a … subclass", and none says "class" explicitly. So the fix is a correction rather than a contract addition, on the reading that an int is not a subclass of anything — but the wording is ambiguous enough that each clause wants rewording to "is not a class, or not a … subclass" alongside the code change.
Two inconsistencies worth settling in the same pass, both pre-existing: the Raises: prose alternates between promising a Protocol subclass and a ProtocolBase subclass across these seven sites, while every runtime message says "Protocol subclass" and every guard checks ProtocolBase. That is the same conflation #514 is about, so the guard targets should not move here — only the prose should stop contradicting itself.
Not breaking when fixed: RegistryError subclasses TypeError through BaseError, so except TypeError keeps working and only code matching the exact type or the old message text sees a difference.
Worth doing as one change rather than seven, since the fix is mechanical and the tests are parallel. Related: #1021, #1025, #514.
Metadata
Metadata
Assignees
Labels
Projects
- StatusShow more project fieldsDone
Activity
fix(foundation): raise RegistryError, not a leaked TypeError, for a n…
fix(foundation): raise RegistryError, not a leaked TypeError, for a n…
JarryShaw commentedon Oct 5, 2026
Correcting this issue's own premise:
Transport.registeris not exempt, so this is seven sites and not six.I wrote that
Transport.registerraisesUnsupportedCallbefore its guard is reachable. That is only true of the abstract class. The guard atpcapkit/protocols/transport/transport.py:109readsif cls is Transport:, so theUnsupportedCallfires for a literalTransport.registercall and for nothing else — andTransport.registeris never called that way. Measured on the current tree:So the bare
issubclassattransport.py:114is reachable through every concrete transport subclass, which is the only way it is ever reached in practice, and it leaks exactly like the other six. The list isProtocolBase.register,Frame.register,PCAPNG.register,SCTP.register,Link.register,Internet.registerandTransport.register— seven of the thirteenif not issubclasssites in the package, with six fixed in #1025.This matters beyond the count: a reader of the old wording would conclude
TCP.registerandUDP.registerwere already safe. The same wrong claim was in #1025's commit message and changelog entry and has been corrected there too.Worth noting for whoever takes this: the per-site judgement in the issue body still stands, and
Transport.registeradds a wrinkle to it. ItsRaises:prose needs reading alongside thecls is Transportgate, because the honest documentation has to say which exception a caller gets for which class.fix(foundation): raise RegistryError, not a leaked TypeError, for a n…
fix(protocols): raise RegistryError for a non-class in seven register…
fix(protocols): raise RegistryError for a non-class in seven register…