Repository navigation
Conversation
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
|
Tested f25da90 merged into main 7a16ef5 on Python 3.13 (Linux). On main all of these raise a bare One place with the same problem that this doesn't cover is dict keys in msgspec.convert({(1, 2): 0}, dict[list[int], int])
# main and this branch: TypeError: unhashable type: 'list'msgpack already raises a int status = PyDict_SetItem(out, key, val);
if (MS_UNLIKELY(status < 0 && PyErr_ExceptionMatches(PyExc_TypeError))) {
ms_unhashable_error(key, &key_path);
}
Py_DECREF(key);That gives Small thing, probably fine: since the original |
|
Could you merge the latest |
`msgspec.json.decode`, `msgspec.msgpack.decode` and `msgspec.convert` raise a
bare `TypeError` when a value can't be added to a `set`/`frozenset`:
>>> msgspec.json.decode(b'[{"a": 1}]', type=set)
TypeError: unhashable type: 'dict'
`DecodeError` is documented as "An error occurred while decoding an object",
so `except msgspec.DecodeError` doesn't catch this. The same bug was fixed for
msgpack map keys in msgspec#1209; the set item paths still leak it.
`mpack_decode_set`, `json_decode_set` and `convert_seq_to_set` now translate
the `TypeError` raised by `PySet_Add` into a `ValidationError` (a subclass of
`DecodeError`) that points at the offending item:
>>> msgspec.json.decode(b'[{"a": 1}]', type=set)
msgspec.ValidationError: Expected a hashable value, got `dict` - at `$[0]`
`convert_dict_to_dict` leaks it the same way when a key converts to an
unhashable value, so the `PyDict_SetItem` call there gets the same treatment:
>>> msgspec.convert({(1, 2): 0}, dict[list[int], int])
msgspec.ValidationError: Expected a hashable value, got `list` - at `key` in `$`
Note the msgpack decoder decodes arrays as tuples when a hashable value is
required, so arrays still decode into sets there; only maps, `Ext` values and
unhashable `dec_hook` results error out.
Tests: all 19 new cases raise a bare `TypeError` without the fix.
- `tests/unit/test_common.py`: json and msgpack, a `set`/`frozenset` item and a
nested `$.values[0]` path.
- `tests/unit/test_json.py`: `[[1, 2]]` decodes as a list, which can't be added
to a `set`, `frozenset` or `set[list[int]]`.
- `tests/unit/test_msgpack.py`: a map and an `Ext` value as set items.
- `tests/unit/test_convert.py`: `convert` with a dict/list item, a struct field
path, and an unhashable dict key (4 cases, 3 of them parametrized over
`dict`/`dict` subclass/duck mapping).
`pytest tests/unit` (Python 3.11, Windows): 6549 passed, 243 skipped, 2 errors.
Both errors are `test_cpylint.py` reading `_core.c` with the local cp936
default encoding, and they fail identically on `main`.
`ruff check`, `ruff format --diff` and `codespell` pass.
Signed-off-by: simpleqt <[email protected]>
f25da90 to
2041655
Compare
|
Done on |
|
You were right, and I should have answered this when you posted it rather than let it sit. Implemented on and RED/GREEN on this head: with only the C hunk reverted, the 4 new cases in On your third note: yes, the original |
Problem
Decoding a value that can't be added to a
set/frozensetraises a bareTypeErrorfrom allthree entry points, escaping the documented
DecodeErrorcontract(
msgspec.DecodeError.__doc__ == "An error occurred while decoding an object",ValidationErroris a subclass), soexcept msgspec.DecodeError/except msgspec.ValidationErrordoesn't catch it:
PySet_Addreturns an unhashable-valueTypeErrordirectly, and the three set-building paths(
mpack_decode_set,json_decode_set,convert_seq_to_set) forwarded it unchanged. This mattersfor the common "validate untrusted input" case: a JSON payload whose
set/set[Any]field holds anobject or an array aborts with an unexpected exception class, and there is no path information.
Msgpack map keys had exactly this bug and it was fixed in #1209
(
DecodeError: ... map keys must be hashable); the set item paths were missed.Fix
Translate the
TypeErrorfromPySet_Addinto aValidationError(aDecodeErrorsubclass) thatcarries the item's path, in the three places that build sets while decoding/converting:
The new helper is 7 lines of C and each call site only splits the existing
item == NULL || PySet_Add(...) < 0condition so the hashability error can be re-raised:Behaviour intentionally unchanged
sets there (
TestDecodeArrayTypeUsesTupleIfHashableRequired) — a documented msgpack-only rule(
docs/supported-types.rstblock "MessagePack … array: list or tuple", footnote: "Tuples are onlyused when the array type must be hashable").
ValidationErrorwith a path instead of a bareTypeError.Testing
New tests (15 cases) — placed next to the existing set tests of the module that owns each codec path:
tests/unit/test_common.py::TestSet— json + msgpack (protofixture),set/frozensetitem,plus a nested
$.values[0]path through a struct field.tests/unit/test_json.py::TestSequences—[[1, 2]]intoset,frozenset,set[list[int]].tests/unit/test_msgpack.py::TestDecodeArrayTypeUsesTupleIfHashableRequired— map item andExtitem.
tests/unit/test_convert.py::TestSequences—convertwith a dict/list item and a struct field.RED on unpatched base (only
src/msgspec/_core.creverted, C rebuilt)GREEN with the fix
Full suites
Lint / checks
Manual verification of the reproduction script
scratch_repro.py(unpatched → patched), 10 cases acrossjson,msgpackandconvert:→
(
except msgspec.DecodeErrorcatches all of them in the patched build.)What was not run here (Windows/Python 3.11 host)
just test-perf/tests/prof/perf— needspytest-codspeed, not installed (benchmark-only).just test-typing(mypy/pyright/pyrefly/scripts/stubtest.py) — not installed; the changetouches no stubs or Python-side code.
just hooks/pre-commit run(prek) — not installed; ran the equivalentruff check,ruff format --diff,codespellcommands from.pre-commit-config.yamldirectly.tests/unit/test_cpylint.pyreads_core.cwithout an explicit encoding, so it errors under a non-UTF-8 locale codec; run pytest with
-X utf8(base and patched trees behave identically — the file has had non-ASCII bytes atoffset 741 since before this change).
Risk assessment
new allocation in the hot path (the extra branch runs only when
PySet_Addalready failed).runtests-style refcount sanity checks over the new error paths show no reference growth.TypeErrorwas acceptable here (the rationale ofthe merged fix(msgpack): raise DecodeError for unhashable map keys #1209 says it is not); (2) the wording/type — this uses
ValidationErrorwith a path(
Expected a hashable value, got \dict` - at `$[0]`) rather than msgpack'sDecodeError: ... map keys must be hashable`, to stay consistent with other element-levelvalidation messages.