Repository navigation
fix: POP_MARK and POP at a mark decode like CPython (#49) - #58
Merged
Merged
Conversation
jensens
force-pushed
the
fix/pop-mark
branch
from
September 30, 2026 15:17
9301222 to
64e36ef
Compare
`POP_MARK` discards everything above the last MARK and the mark itself, flushing the memo bindings of the discarded slots first; `POP` with nothing above the last MARK discards the mark, as CPython's load_pop does. Protocol 0 and 1 picklers write these to unwind a recursive tuple; the pre-scan learns the new no-argument opcode. The recursive tuple decodes to the snapshot the memo holds at the closing GET, the tests pin it. Assisted-by: Claude Fable 5.1
…l 3 (#49) Fresh review finding: CPython writes POP_MARK for a recursive tuple of four or more elements at every binary protocol, protocol 3 included, so a persisted record with such a tuple failed to decode on main; the changelog and the test docstring said protocol 0 or 1 only. Tests now pin the four-element tuple at protocols 1 to 3, the memo flush of the discarded slots (a list bound to a memo index and read after the POP_MARK holds its live value) and a protocol 3 record. Assisted-by: Claude Fable 5.1
jensens
force-pushed
the
fix/pop-mark
branch
from
September 30, 2026 15:46
64e36ef to
530d500
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #49
Two mark opcodes were not handled the way CPython handles them:
POP_MARKraisedunknown pickle opcode, andPOPwith nothing above the lastMARKraisedstack underflowwhere CPython discards the mark.The issue assumed these only occur at protocols 0 and 1. The reviewer's differential run showed otherwise: CPython writes
MARK ... POP_MARK GETfor any recursive tuple of four or more elements at every binary protocol, protocol 3 included, so a persisted ZODB record holding such a tuple (a tuple whose element refers back to it) failed to decode onmain. Protocol 1 writes it for any recursive tuple; protocol 0 writesPOPs instead.What changes
POP_MARK: everything above the lastMARKand the mark itself are discarded; the memo bindings of the discarded slots are flushed with their live values first (a list bound to a memo index and read after thePOP_MARKcomes back with the element that was appended to it).POPat the mark discards the mark, as CPython'sload_popdoes;POPon an empty stack without a mark still raises.A recursive tuple decodes to the value the memo holds at the closing
GET, the same rule the codec already applies to recursive lists and dicts and to short recursive tuples at protocol 2, so the output is identical across protocols (([], 11, 12, 13)for the four-element case).Evidence
main(it contains noPOP_MARK).main, all decode on the branch, deterministic, the 44 thatmaindecodes byte-identical), 19 hand-built streams against both CPython unpicklers, and proved the pre-scan stays active and safe. Its findings (the protocol scope and the test gap) are the last commit.Assisted-by: Claude Fable 5.1