Repository navigation
fix(fields): fall through to the bounded pseudo-member instead of minting (#575) #771
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we actually raise if it's an illegal value since the
self._namespaceenum rejected it? instead of creating a pseudo value.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, and it splits into two cases that this PR currently conflates — you are right about one of them.
For an in-range but unassigned value, the pseudo-member is correct and raising would be a regression. The namespace did not reject
54321as illegal; it has no name for it. Port 54321 is perfectly legal wire data — IANA simply assigns nothing individually in 49152-65535.EnumField.post_process's own docstring (written for #701) spells out what raising cost: the bareValueErroris aenum's, not one ofpcapkit.utilities.exceptions, so a caller cannot tell it from a bug of its own, and it is not anEOFError, soExtractor.record_framesdoes not catch it — one unassigned code cost the whole extraction. It also made theunknownreaders the formats require unreachable: PCAP-NG'sUnknownBlockand theunassignedoption readers of IPv4, TCP, HOPOPT, MH and HIP, because the lookup failed frames before the dispatch that would have selected them. PCAP-NG repeats a block's length at both ends precisely so an unrecognised block can be skipped; that skip is what the fallback restores.But for a genuinely illegal value it should raise, and right now it does not. Measured on this branch:
#764 made
AppType.getraise for anything outside 0-65535 — abreakingchange, merged as57b2c1761. This PR silently undoes it for these four fields. The type-based rule ("absorb any bareValueError") is too coarse: it cannot tell aenum's "no member has this value" from #764's deliberate range rejection, because both are bareValueErrors.The PR's own reasoning was that the range guard is unreachable from parsing since the fields are
PortEnumField(length=2, …), so 0-65535 is guaranteed. That is true of the parse path, butpost_processis reachable directly, and more to the point it means the absorption buys nothing while quietly reverting a fix that landed deliberately.So: keep the pseudo-member for unassigned-but-legal, and let an out-of-range rejection propagate. Routing that as a required change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for in-range unnamed values, we should still return the
self._namespaceenum tho, rather than creating a new one. so that means we shouldextend_enumon theself._namepaceon that case, which I was expecting the enum itself to handle it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Measured this before answering, and there is a third option that gives you what you want without the
extend_enum— which matters, becauseextend_enumis precisely #575's cost.str.__new__on the namespace class produces a genuine member of it that is never registered:So it is a
self._namespaceenum —isinstance(x, TCP)holds, and it reprs identically to a declared member — while the registry does not grow. That satisfies your requirement and #575's at the same time, whereextend_enumcan only satisfy yours: minting is what costs 111 calls and ~13.5% of extraction self time onhttp.pcap, which is the whole of #575.Note
TCPis aStrEnum, not int-based — MRO isTCP → AppType → StrEnum → str → ReprEnum— so it isstr.__new__, notint.__new__; the latter raisesTypeError: TCP is not a subtype of int.Three caveats I have not resolved, and they decide whether this is actually better than the
<unknown>pseudo-enum now in the PR:TCP(54321)still raises — the instance is absent from_value2member_map_, so it is not recoverable by value lookup. Same as the current pseudo-member, but worth being explicit that "returns aTCP" does not mean "findable as aTCP".AppTypedefines__eq__/__hash__on.port, so two unregistered instances for the same port would compare equal without being identical. Whether anything depends onisneeds checking.copymay not survive an unregistered member. If any dump path pickles these, that breaks.Also, on "I was expecting the enum itself to handle it" — that would put it in
AppType._missing_, which is whereaenumwould naturally hook. I steered the worker away from_missing_on your earlier ruling thatget()is the public API and should keep minting for direct callers. If you would rather the enum own this, say so and I will move it there instead of into the field classes.Which do you want: this unregistered namespace member, or the
<unknown>pseudo-enum the PR currently returns? The out-of-range fix is unaffected either way and is already routed.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is actually quite a nice workaround and i think we should even apply to all other Enum's legit but unbounded values - so that we dont create registered enums out of unrecognised/unregistered values, unless user/caller explicitly created them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ruling recorded, in your words: "this is actually quite a nice workaround and i think we should even apply to all other Enum's legit but unbounded values - so that we dont create registered enums out of unrecognised/unregistered values, unless user/caller explicitly created them."
Taking the unregistered namespace member for this PR. But measuring the general rule's reach turned up something that corrects a claim I made earlier in this thread: the declared-range
_missing_branches mint per value as well.I had described those as "the bounded declared-range mechanism — the registries doing their job", against the unbounded
get()fall-through. Wrong: they are bounded in which ranges they cover, not in how many members they create. A capture touching 200 distinct ports inside one reserved span grows the registry by 200.So your principle reaches both paths, and the scope is:
That is 1,169 sites across 113 registries — far beyond this PR. So I am splitting it: #771 keeps its four
EnumFieldsubclasses, which is where the measured 111 calls and ~13.5% of extraction self time live and is what closes #575, and the general rule gets its own issue with this measurement in it. Say if you would rather #771 carry the whole thing; I would not, because a 113-registry change with abreakinglabel and no incremental verification is not reviewable.Two things your wording settles that were open:
AppType.get(54321)may still register, since that is an explicit caller action. That matches your earlier ruling thatget()is the public API. The parse path never asked for a name, so it gets the unregistered member.AppType._missing_should own this rather than the field classes. For fix(fields): fall through to the bounded pseudo-member instead of minting (#575) #771 I am keeping it in the field classes, per your earlier ruling. The general issue is where the_missing_question belongs, since that is the path with 1,056 sites.The three caveats I raised still need answering there rather than here:
TCP(54321)stays unfindable by value lookup,__eq__/__hash__key on.portso two such instances compare equal without being identical, and pickling an unregistered member may not survive.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for
.get(), it's not an explicit call. I say we can probably add something like a.register(...)call that takes code, name, and any other required values to generate a proper enum as an explicit call.for
_missing_, it is still scoped to those ranged bounded but not expanded branches, like for ports 100-200, they are allreserverd_nnnbranch.this ruling must apply to all enums under
pcapkit.const, not just the AppType classes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recorded, in your words: "for
.get(), it's not an explicit call. I say we can probably add something like a.register(...)call that takes code, name, and any other required values to generate a proper enum as an explicit call. for_missing_, it is still scoped to those ranged bounded but not expanded branches, like for ports 100-200, they are allreserverd_nnnbranch. this ruling must apply to all enums underpcapkit.const, not just the AppType classes."That corrects me — I said a direct
AppType.get(54321)counted as an explicit caller action and could keep registering. It does not, and the consequence is that registration becomes opt-in through a new API rather than a side effect of lookup. Carried into #775 with the two measurements below.#771 is unaffected and stays as it is. It only touches the four
EnumFieldsubclasses'post_process, neverget()or_missing_, so its scope does not move. CI is green at57c82794eand the switch to the unregistered namespace member is with the worker.Two things measured that shape the
.register()design, neither resolved:.register()cannot have one signature. Member shapes genuinely differ —AppType.__new__(cls, value, name='<null>', proto=TransportProtocol.undefined)carries port/svc/proto, whileLinkType(IntEnum)is a plainIntEnumwith no custom__new__at all. So "code, name, and any other required values" is per-registry. Either.register()lives on each class with its own signature, or it takes**kwargsand each registry validates — the second keeps one call site but moves the error from signature to runtime. Worth deciding before anyone writes it. Nothing namedregisterexists underpcapkit/const/today.Three existing tests depend on minting through lookup and will need rewriting to
.register():Your
_missing_scoping is exactly what I measured — those branches mint one member per distinct value inside a declared range, so a capture touching 200 ports in areserved_nnnspan grows the registry by 200. Full scope stands at 1,169 sites across 113 registries (1,056 in_missing_, 113 inget()).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.register(...)lives per register with its own signature.