Repository navigation
Fix custom dictionary-key conversion - #1238
CAOShurong wants to merge 1 commit into
Conversation
STiFLeR7
left a comment
There was a problem hiding this comment.
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.decodefor the same type:dict[C, int]with adec_hookgives{C(a): 1}fromjson.decode,convert(strict) andconvert(strict=False);dict[C | None, int]hooks once;dict[Annotated[C, Meta(...)], int]hooks. - With no hook,
convert({"a": 1}, dict[C, int])now raisesValidationError: Expected `C`, got `str` - at `key` in `$`, identical tojson.decode. Before, it silently returned thestrkey. - Refcounts: 1000 calls on the same key string leave
sys.getrefcountunchanged (4 -> 4), so thePy_INCREFplusms_decode_customsteal is balanced, the same pattern asconvert()(src/msgspec/_core.c:22490on the PR head). - A
str-subclass custom key and generic custom keys behave as the new tests expect.
Findings.
- Behaviour change worth stating in the description / release notes (
src/msgspec/_core.c:21219).
Code that relied onconvert(d, dict[Custom, V])passingstrkeys through (no hook, or a hook that was never called) will now raiseValidationErroratkey. I think that is the right behaviour (it matchesjson.decode, and #775 is exactly this inconsistency), but it can break existing callers. - Nit: the new branch is reachable from one caller only (
_core.c:21219vs:22490,:22497,:21880).
convert()already dispatchesMS_TYPE_CUSTOMbefore it callsconvert_strfor ordinary strings (:22490before:22497), so the only way to reach the new branch with a custom type is the exact-strkey shortcut inconvert_dict_to_dict(:21880). Handling the custom case at that call site (or routing custom key types throughconvert()) would leaveconvert_struntouched. 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.
Fixes #775.
Exact string dictionary keys go straight to
convert_str, bypassing thecustom-type dispatch in
convert. Since custom types also carryMS_TYPE_ANY,the string fast path returns the original key without calling
dec_hookorvalidating its type.
Handle custom types in that fast path using the same
ms_decode_custompattern as JSON dictionary-key decoding. Other key conversion rules are unchanged.
Compatibility note:
convert(d, dict[Custom, V])now validates exact string keysas custom keys. Without a suitable
dec_hook, it raisesValidationErroratkeyinstead of silently returning the original string key. This matchesjson.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 customkeys, missing/invalid hooks, exception propagation and nested key error paths.
Validation on Windows / CPython 3.13.1, using non-editable wheels:
python -m pytest tests/unit: 6,531 passed, 118 skipped, repeated on threeseeds. 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 againsttests/typing,scripts/stubtest.py, and package doctests pass.MessagePack, YAML and TOML, including rejection/error paths.
MSVC warnings match main. Linux/macOS and sanitizers were not run locally.