Skip to content

Fix custom dictionary-key conversion - #1238

Open
CAOShurong wants to merge 1 commit into
msgspec:mainfrom
CAOShurong:fix-custom-dict-keys
Open

CAOShurong wants to merge 1 commit into
msgspec:mainfrom
CAOShurong:fix-custom-dict-keys

Conversation

@CAOShurong

@CAOShurong CAOShurong commented Oct 8, 2026 •

Copy link
Copy Markdown

Fixes #775.

Exact string dictionary keys go straight to convert_str, bypassing the
custom-type dispatch in convert. Since custom types also carry MS_TYPE_ANY,
the string fast path returns the original key without calling dec_hook or
validating its type.

Handle custom types in that fast path using the same ms_decode_custom
pattern as JSON dictionary-key decoding. Other key conversion rules are unchanged.

Compatibility note: convert(d, dict[Custom, V]) now validates exact string keys
as custom keys. Without a suitable dec_hook, it raises ValidationError at
key instead of silently returning the original string key. This matches
json.decode, but changes behavior for callers that relied on the old pass-through.

Regression coverage includes the existing dict/mapping fixtures, strict and lax
conversion, str_keys, generic custom types, string subclasses, existing custom
keys, missing/invalid hooks, exception propagation and nested key error paths.

Validation on Windows / CPython 3.13.1, using non-editable wheels:

  • New regression cases: 30 failed / 6 passed on main; all 36 pass with the fix.
  • python -m pytest tests/unit: 6,531 passed, 118 skipped, repeated on three
    seeds. All 6,613 existing test outcomes match main in the same dependency setup.
  • python -m pytest tests/unit/test_convert.py: 834 passed, 33 skipped.
  • prek run --all-files --hook-stage manual, mypy/pyright/pyrefly against
    tests/typing, scripts/stubtest.py, and package doctests pass.
  • The sdist-built wheel was installed and checked through convert, JSON,
    MessagePack, YAML and TOML, including rejection/error paths.

MSVC warnings match main. Linux/macOS and sanitizers were not run locally.

@STiFLeR7 STiFLeR7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Summary. convert() skipped dec_hook for custom dict-key types because convert_dict_to_dict sends exact-str keys straight to convert_str, bypassing the MS_TYPE_CUSTOM dispatch at the top of convert(). The PR adds that dispatch inside convert_str's ANY|STR branch.

Verified. CI has not run the test matrix on this PR, so I built the PR merged onto current main (dac6320, clean merge) on Linux, CPython 3.12, gcc:

  • pytest tests/unit: 6505 passed, 146 skipped, 0 failed.
  • Behaviour now matches json.decode for the same type: dict[C, int] with a dec_hook gives {C(a): 1} from json.decode, convert (strict) and convert(strict=False); dict[C | None, int] hooks once; dict[Annotated[C, Meta(...)], int] hooks.
  • With no hook, convert({"a": 1}, dict[C, int]) now raises ValidationError: Expected `C`, got `str` - at `key` in `$` , identical to json.decode. Before, it silently returned the str key.
  • Refcounts: 1000 calls on the same key string leave sys.getrefcount unchanged (4 -> 4), so the Py_INCREF plus ms_decode_custom steal is balanced, the same pattern as convert() (src/msgspec/_core.c:22490 on the PR head).
  • A str-subclass custom key and generic custom keys behave as the new tests expect.

Findings.

  1. Behaviour change worth stating in the description / release notes (src/msgspec/_core.c:21219).
    Code that relied on convert(d, dict[Custom, V]) passing str keys through (no hook, or a hook that was never called) will now raise ValidationError at key. I think that is the right behaviour (it matches json.decode, and #775 is exactly this inconsistency), but it can break existing callers.
  2. Nit: the new branch is reachable from one caller only (_core.c:21219 vs :22490, :22497, :21880).
    convert() already dispatches MS_TYPE_CUSTOM before it calls convert_str for ordinary strings (:22490 before :22497), so the only way to reach the new branch with a custom type is the exact-str key shortcut in convert_dict_to_dict (:21880). Handling the custom case at that call site (or routing custom key types through convert()) would leave convert_str untouched. Not a correctness issue, just a smaller blast radius.

Not covered / unverified. No sanitizer or debug-build refcount run, no Windows/macOS, only CPython 3.12 on Linux. I did not run just check or the type-stub checks (the PR reports them passing).

Verdict. Comment. The fix is correct and the tests are thorough; finding 1 is a heads-up, finding 2 a preference.

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.

msgspec.convert seems to not call dec_hook for custom dict key

2 participants