Conversation
…ve error reporting for invalid states
📝 WalkthroughWalkthroughThe PR changes unpacker stack storage and failure reporting in the C templates, removes a dead inline callback, and updates the Cython wrapper to stop re-raising pending Python exceptions in two unpack paths. Changesmsgpack Unpacker Error Handling and Stack Cleanup
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
msgpack/unpack_template.h (1)
301-303: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueGood: invalid
csno longer fails silently. Setting aRuntimeErrorbeforegoto _failedis exactly what makes the downstream Cythonexcept -1change safe (every-1now carries an exception).Minor:
csisunsigned int, so the conversion specifier should be%urather than%d.♻️ Optional
- PyErr_Format(PyExc_RuntimeError, "Invalid state: %d", cs); + PyErr_Format(PyExc_RuntimeError, "Invalid state: %u", cs);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@msgpack/unpack_template.h` around lines 301 - 303, The invalid-state error in the unpack path should use the correct unsigned format specifier for `cs`; update the `PyErr_Format` call in `unpack_template.h` so the `Invalid state` message matches `unsigned int` rather than `%d`. Keep the existing `RuntimeError` and `goto _failed` flow in the same state-check block so the Cython `except -1` behavior remains intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@msgpack/unpack_template.h`:
- Around line 301-303: The invalid-state error in the unpack path should use the
correct unsigned format specifier for `cs`; update the `PyErr_Format` call in
`unpack_template.h` so the `Invalid state` message matches `unsigned int` rather
than `%d`. Keep the existing `RuntimeError` and `goto _failed` flow in the same
state-check block so the Cython `except -1` behavior remains intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5817ca8b-6ec7-40cd-8d87-fa46d3f036a3
📒 Files selected for processing (3)
msgpack/_unpacker.pyxmsgpack/unpack.hmsgpack/unpack_template.h
💤 Files with no reviewable changes (1)
- msgpack/unpack.h
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
msgpack/unpack_template.h (1)
301-302: 🎯 Functional Correctness | 🟡 MinorUse
%uforcs.csis anunsigned int, so%dis the wrong format specifier in this error path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@msgpack/unpack_template.h` around lines 301 - 302, The error formatting in the unpack error path uses the wrong specifier for cs, which is an unsigned int. Update the PyErr_Format call in the unpack logic to use the correct unsigned format for cs, and keep the change localized to the invalid-state handling in the unpack template so the error message matches the variable type.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@msgpack/unpack_template.h`:
- Around line 301-302: The error formatting in the unpack error path uses the
wrong specifier for cs, which is an unsigned int. Update the PyErr_Format call
in the unpack logic to use the correct unsigned format for cs, and keep the
change localized to the invalid-state handling in the unpack template so the
error message matches the variable type.
Note
Medium Risk
Changes core msgpack unpack control flow and exception propagation for all
unpackband streaming unpack paths; behavior should match for normal errors but edge cases around C/Python exception interaction deserve regression testing.Overview
Unpack error handling is simplified so Python exceptions raised inside the C unpack path propagate through Cython instead of being double-handled in Python.
The
execute_fnpointer is declared withexcept -1(replacingexcept? -1), and the redundantPyErr_Occurred()/ bareraisebranches are removed fromunpackbandUnpacker._unpack. Callback failures and explicitPyErr_*calls in the unpack template still surface as normal Python exceptions.Internal unpack setup no longer uses a no-op
unpack_callback_root; the root stack slot is initialized withNULL, and dead commented code around an alternate dynamic stack is removed.Invalid internal parser state (unexpected
csor container type) now setsRuntimeErrorviaPyErr_Formatbefore failing, instead of falling through without a clear exception.Reviewed by Cursor Bugbot for commit 38927d1. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit