Skip to content

Fix bare TypeError when decoding unhashable items into a set - #1296

Open
simpleqt wants to merge 1 commit into
msgspec:mainfrom
simpleqt:fix-unhashable-set-items
Open

simpleqt wants to merge 1 commit into
msgspec:mainfrom
simpleqt:fix-unhashable-set-items

Conversation

@simpleqt

Copy link
Copy Markdown

Problem

Decoding a value that can't be added to a set/frozenset raises a bare TypeError from all
three entry points, escaping the documented DecodeError contract
(msgspec.DecodeError.__doc__ == "An error occurred while decoding an object",
ValidationError is a subclass), so except msgspec.DecodeError / except msgspec.ValidationError
doesn't catch it:

>>> import msgspec
>>> msgspec.json.decode(b'[{"a": 1}]', type=set)
TypeError: unhashable type: 'dict'
>>> msgspec.json.decode(b"[[1, 2]]", type=set)
TypeError: unhashable type: 'list'
>>> msgspec.msgpack.decode(b"\x91\x81\xa1a\x01", type=set)   # [{"a": 1}]
TypeError: unhashable type: 'dict'
>>> msgspec.convert([{"a": 1}], set)
TypeError: unhashable type: 'dict'

PySet_Add returns an unhashable-value TypeError directly, and the three set-building paths
(mpack_decode_set, json_decode_set, convert_seq_to_set) forwarded it unchanged. This matters
for the common "validate untrusted input" case: a JSON payload whose set/set[Any] field holds an
object 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 TypeError from PySet_Add into a ValidationError (a DecodeError subclass) that
carries the item's path, in the three places that build sets while decoding/converting:

>>> msgspec.json.decode(b'[{"a": 1}]', type=set)
msgspec.ValidationError: Expected a hashable value, got `dict` - at `$[0]`
>>> msgspec.msgpack.decode(b"\x91\xd4\x01\x02", type=set)    # [Ext(1, b"\x02")]
msgspec.ValidationError: Expected a hashable value, got `msgspec.msgpack.Ext` - at `$[0]`
>>> msgspec.convert([[1, 2]], set[list[int]])
msgspec.ValidationError: Expected a hashable value, got `list` - at `$[0]`

The new helper is 7 lines of C and each call site only splits the existing
item == NULL || PySet_Add(...) < 0 condition so the hashability error can be re-raised:

/* `PySet_Add` raises a bare `TypeError` when the value isn't hashable, which
 * escapes the documented `DecodeError` contract. Replace it with a
 * `ValidationError` pointing at the offending value. Always returns -1 so it
 * can be used directly in error handling branches. */
static int
ms_unhashable_error(PyObject *obj, PathNode *path) {
    PyErr_Clear();
    ms_raise_validation_error(
        path, "Expected a hashable value, got `%s`%U", Py_TYPE(obj)->tp_name
    );
    return -1;
}

Behaviour intentionally unchanged

  • msgpack arrays still decode as tuples in hashable-required positions, so arrays keep decoding into
    sets there (TestDecodeArrayTypeUsesTupleIfHashableRequired) — a documented msgpack-only rule
    (docs/supported-types.rst block "MessagePack … array: list or tuple", footnote: "Tuples are only
    used when the array type must be hashable").
  • JSON arrays still decode as lists, so an array item in a JSON set is still an error — it is now a
    ValidationError with a path instead of a bare TypeError.
  • Error types for the already-correct paths are unchanged; no encoded output changes.

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 (proto fixture), set/frozenset item,
    plus a nested $.values[0] path through a struct field.
  • tests/unit/test_json.py::TestSequences — [[1, 2]] into set, frozenset, set[list[int]].
  • tests/unit/test_msgpack.py::TestDecodeArrayTypeUsesTupleIfHashableRequired — map item and Ext
    item.
  • tests/unit/test_convert.py::TestSequences — convert with a dict/list item and a struct field.

RED on unpatched base (only src/msgspec/_core.c reverted, C rebuilt)

$ python -m pytest tests/unit/test_common.py::TestSet \
    tests/unit/test_json.py::TestSequences::test_decode_unhashable_item \
    tests/unit/test_msgpack.py::TestDecodeArrayTypeUsesTupleIfHashableRequired \
    tests/unit/test_convert.py::TestSequences::test_unhashable_item \
    tests/unit/test_convert.py::TestSequences::test_unhashable_item_nested_path -q
...
E           TypeError: unhashable type: 'dict'
tests\unit\test_convert.py:1034: TypeError
=========================== short test summary info ===========================
FAILED tests/unit/test_common.py::TestSet::test_decode_unhashable_item[json-set]
FAILED tests/unit/test_common.py::TestSet::test_decode_unhashable_item[json-frozenset]
FAILED tests/unit/test_common.py::TestSet::test_decode_unhashable_item[msgpack-set]
FAILED tests/unit/test_common.py::TestSet::test_decode_unhashable_item[msgpack-frozenset]
FAILED tests/unit/test_common.py::TestSet::test_decode_unhashable_item_nested_path[json]
FAILED tests/unit/test_common.py::TestSet::test_decode_unhashable_item_nested_path[msgpack]
FAILED tests/unit/test_json.py::TestSequences::test_decode_unhashable_item[set0]
FAILED tests/unit/test_json.py::TestSequences::test_decode_unhashable_item[frozenset]
FAILED tests/unit/test_json.py::TestSequences::test_decode_unhashable_item[set1]
FAILED tests/unit/test_msgpack.py::TestDecodeArrayTypeUsesTupleIfHashableRequired::test_decode_unhashable_item[\x91\x81\xa1a\x01-dict]
FAILED tests/unit/test_msgpack.py::TestDecodeArrayTypeUsesTupleIfHashableRequired::test_decode_unhashable_item[\x91\xd4\x01\x02-msgspec.msgpack.Ext]
FAILED tests/unit/test_convert.py::TestSequences::test_unhashable_item[obj0-set-dict]
FAILED tests/unit/test_convert.py::TestSequences::test_unhashable_item[obj1-frozenset-dict]
FAILED tests/unit/test_convert.py::TestSequences::test_unhashable_item[obj2-set-list]
FAILED tests/unit/test_convert.py::TestSequences::test_unhashable_item_nested_path
15 failed, 10 passed, 2 skipped in 2.12s

GREEN with the fix

$ python -m pytest tests/unit/test_common.py::TestSet \
    tests/unit/test_json.py::TestSequences::test_decode_unhashable_item \
    tests/unit/test_msgpack.py::TestDecodeArrayTypeUsesTupleIfHashableRequired \
    tests/unit/test_convert.py::TestSequences::test_unhashable_item \
    tests/unit/test_convert.py::TestSequences::test_unhashable_item_nested_path -q
25 passed, 2 skipped

Full suites

$ python -X utf8 -m pytest tests/unit -q          # this is what CI runs (`.justfile: test-unit`)
6509 passed, 238 skipped in 16.36s

$ python -X utf8 -m pytest --doctest-modules --pyargs msgspec -q    # `just test-doc`
7 passed

$ python -m pytest tests/unit/test_common.py tests/unit/test_json.py tests/unit/test_msgpack.py tests/unit/test_convert.py -q
4942 passed, 203 skipped in 55.64s

Lint / checks

$ python -m ruff check src/msgspec tests/unit/test_common.py tests/unit/test_convert.py tests/unit/test_json.py tests/unit/test_msgpack.py
All checks passed!

$ python -m ruff format --diff tests/unit/test_common.py tests/unit/test_convert.py tests/unit/test_json.py tests/unit/test_msgpack.py
4 files already formatted

$ python -m codespell_lib src/msgspec/_core.c tests/unit/test_{common,convert,json,msgpack}.py
(no output)

Manual verification of the reproduction script

scratch_repro.py (unpatched → patched), 10 cases across json, msgpack and convert:

TypeError    | json.decode(b'[{"a": 1}]', type=set) | unhashable type: 'dict'
TypeError    | json.decode(b'[[1, 2]]', type=set) | unhashable type: 'list'
...

→

DecodeError  | json.decode(b'[{"a": 1}]', type=set) | Expected a hashable value, got `dict` - at `$[0]`
DecodeError  | json.decode(b'[[1, 2]]', type=set) | Expected a hashable value, got `list` - at `$[0]`
DecodeError  | msgpack.decode(left_square, type=set) | Expected a hashable value, got `msgspec.msgpack.Ext` - at `$[0]`
DecodeError  | struct field `tags: set` | Expected a hashable value, got `dict` - at `$.tags[0]`

(except msgspec.DecodeError catches all of them in the patched build.)

What was not run here (Windows/Python 3.11 host)

  • just test-perf / tests/prof/perf — needs pytest-codspeed, not installed (benchmark-only).
  • just test-typing (mypy/pyright/pyrefly/scripts/stubtest.py) — not installed; the change
    touches no stubs or Python-side code.
  • just hooks / pre-commit run (prek) — not installed; ran the equivalent ruff check,
    ruff format --diff, codespell commands from .pre-commit-config.yaml directly.
  • Sanitizer/valgrind builds, free-threading builds, non-x86 platforms.
  • Note for Windows users running the suite locally: tests/unit/test_cpylint.py reads _core.c
    without 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 at
    offset 741 since before this change).

Risk assessment

  • Low: error-path only; no change to encoded/decoded values, no change to valid-input behaviour, no
    new allocation in the hot path (the extra branch runs only when PySet_Add already failed).
    runtests-style refcount sanity checks over the new error paths show no reference growth.
  • Maintainer-discretion points: (1) whether a bare TypeError was acceptable here (the rationale of
    the merged fix(msgpack): raise DecodeError for unhashable map keys #1209 says it is not); (2) the wording/type — this uses ValidationError with a path
    (Expected a hashable value, got \dict` - at `$[0]`) rather than msgpack's DecodeError: ... map keys must be hashable`, to stay consistent with other element-level
    validation messages.

Copilot AI balanced review requested due to automatic review settings October 20, 2026 14:12

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@cig13zs

cig13zs commented Oct 24, 2026

Copy link
Copy Markdown

Tested f25da90 merged into main 7a16ef5 on Python 3.13 (Linux). On main all of these raise a bare TypeError, and with this branch they raise ValidationError with the right path: json, msgpack and convert into set/frozenset with dict or list items, a non-frozen Struct item (set[P]), an array_like Struct in a frozenset, an unhashable item at index 1 (gives $[1], so moving el_path.index++ is right), a nested $.values[0], and convert from a set or tuple input. Hashable input decodes the same as before, and msgpack arrays still come back as tuples. The 15 new tests fail on main and pass here, and pytest tests/unit passes (6597 passed, 158 skipped).

One place with the same problem that this doesn't cover is dict keys in convert:

msgspec.convert({(1, 2): 0}, dict[list[int], int])
# main and this branch: TypeError: unhashable type: 'list'

msgpack already raises a DecodeError for this since #1209. The same pattern in convert_dict_to_dict fixes it:

        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 Expected a hashable value, got `list` - at `key` in `$` , and the suite still passes with it on top of this branch.

Small thing, probably fine: since the original TypeError is cleared, a frozen Struct holding a list now reports Expected a hashable value, got `FP` where main said unhashable type: 'list', and a custom __hash__ that raises TypeError gets its message replaced. A __hash__ raising anything else still propagates as is.

Siyet commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

Could you merge the latest main into this branch? It includs the pypa/gh-action-pypi-publish update for workflow lint, which is still missing here.

`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]>
@simpleqt
simpleqt force-pushed the fix-unhashable-set-items branch from f25da90 to 2041655 Compare November 8, 2026 13:10
@simpleqt

simpleqt commented Nov 8, 2026

Copy link
Copy Markdown
Author

Done on 2041655. The branch is rebased onto main (44a5048), so it now carries the pypa/gh-action-pypi-publish update (.github/workflows/ci.yml:259, v1.14.3). The rebase was conflict-free and it is still a single commit.

@simpleqt

simpleqt commented Nov 8, 2026

Copy link
Copy Markdown
Author

You were right, and I should have answered this when you posted it rather than let it sit.

Implemented on 2041655. convert_dict_to_dict now maps the TypeError from PyDict_SetItem the same way, so:

>>> msgspec.convert({(1, 2): 0}, dict[list[int], int])
msgspec.ValidationError: Expected a hashable value, got `list` - at `key` in `$`

and - at keyin$.values`` for a nested path. The converted key is still alive when the error is raised, so the message names it.

RED/GREEN on this head: with only the C hunk reverted, the 4 new cases in tests/unit/test_convert.py fail with TypeError: unhashable type: 'list'; with it they pass. tests/unit: 6549 passed, 243 skipped, plus 2 test_cpylint.py errors that are the local cp936 default encoding failing to read _core.c — they fail identically on main.

On your third note: yes, the original TypeError is cleared, so a frozen Struct holding a list now reports Expected a hashable value, got FP`` where main said `unhashable type: 'list'`, and a custom `hash` that raises `TypeError` gets its message replaced. That is the same trade-off #1209 made for map keys, so I kept it rather than special-casing.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants