Accept surrogates in _json.JsonScanner decode path - #7675
Conversation
The _json decoder had two failure modes when a Python str value would contain a lone surrogate (legal per the Python 3 str model): 1. Boundary UnicodeEncodeError: JsonScanner::Callable::call rejected any input str with surrogates via try_into_utf8 before scanning began. 2. Silent U+FFFD corruption: call_scan_once and parse_object's key path called .to_string() on scanstring's Wtf8Buf output, which routes through Wtf8::Display (lossy). Array values and dict keys decoded from JSON \uXXXX escapes silently became U+FFFD. Switch JsonScanner's five PyUtf8StrRef signatures to PyStrRef, drop the entry-point try_into_utf8 call, and feed Wtf8Buf directly to new_str instead of going through .to_string(). Key memoization now uses HashMap<Wtf8Buf, PyStrRef> so surrogate-bearing keys survive interning. parse_number takes &[u8] since JSON numbers are ASCII. Extends the WTF-8 refactor pattern established in RustPython#7673 to the decoder. machinery::scanstring already returns Wtf8Buf and is unchanged. Unmasks test_single_surrogate_decode. 214 tests in test.test_json pass with no regressions. Decoder output verified byte-identical to CPython 3.13.4 over 10,000 random fuzz cases (JSON docs containing random surrogate escapes at root/list/dict positions, compared via json.dumps(..., ensure_ascii=True, sort_keys=True)).
📝 WalkthroughWalkthroughJSON parsing logic refactored to use WTF-8 representations instead of UTF-8, enabling preservation of lone surrogates. Byte-level operations replace string operations for number/literal parsing, and object memoization now uses Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [ ] lib: cpython/Lib/json dependencies:
dependent tests: (10 tests)
Legend:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/stdlib/src/json.rs (1)
238-238: Memoization onWtf8Bufcorrectly preserves lone surrogates in keys and string values — nice fix.Both the
HashMap<Wtf8Buf, PyStrRef>memo and the directvm.ctx.new_str(wtf8_result)(instead of.to_string()) avoid the lossy U+FFFD collapse path forscanstringoutput. Keying on the WTF-8 bytes also keeps memo hits identical to the source key under surrogate-aware equality.♻️ Optional micro-refactor: avoid one `Wtf8Buf` clone using `Entry`
The cache-miss path clones
key_wtf8once to insert and once (implicitly) to stash the str. You can collapse that to a single clone usingentry:- let key: PyObjectRef = match memo.get(&key_wtf8) { - Some(cached) => cached.clone().into(), - None => { - let py_key = vm.ctx.new_str(key_wtf8.clone()); - memo.insert(key_wtf8, py_key.clone()); - py_key.into() - } - }; + let key: PyObjectRef = memo + .entry(key_wtf8.clone()) + .or_insert_with_key(|k| vm.ctx.new_str(k.clone())) + .clone() + .into();Not a correctness concern — keys are typically short — so feel free to defer.
Also applies to: 289-296, 400-400, 517-517, 544-544
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/stdlib/src/json.rs` at line 238, The memoization currently inserts into memo: &mut HashMap<Wtf8Buf, PyStrRef> by cloning key_wtf8 twice on a cache miss; change the insertion to use HashMap::entry for memo so you can take ownership of key_wtf8 once and either return the existing PyStrRef or insert vm.ctx.new_str(wtf8_result) without a second clone. Locate the cache-miss path where key_wtf8 and vm.ctx.new_str are used (scanstring result handling) and replace the separate contains/insert flow with entry(key_wtf8).or_insert_with(|| vm.ctx.new_str(wtf8_result)) to avoid the extra Wtf8Buf clone while preserving surrogate-aware keys and values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/stdlib/src/json.rs`:
- Line 238: The memoization currently inserts into memo: &mut HashMap<Wtf8Buf,
PyStrRef> by cloning key_wtf8 twice on a cache miss; change the insertion to use
HashMap::entry for memo so you can take ownership of key_wtf8 once and either
return the existing PyStrRef or insert vm.ctx.new_str(wtf8_result) without a
second clone. Locate the cache-miss path where key_wtf8 and vm.ctx.new_str are
used (scanstring result handling) and replace the separate contains/insert flow
with entry(key_wtf8).or_insert_with(|| vm.ctx.new_str(wtf8_result)) to avoid the
extra Wtf8Buf clone while preserving surrogate-aware keys and values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: b72b51c1-2c59-48d1-992e-5fad24ac84e1
⛔ Files ignored due to path filters (1)
Lib/test/test_json/test_unicode.pyis excluded by!Lib/**
📒 Files selected for processing (1)
crates/stdlib/src/json.rs
|
Re-verified against CPython 3.14.4 (RustPython's stated target version): same 10,000-case canonical-form fuzz produces 0 mismatches. Original PR body cited 3.13.4 because that was the system Python at probe time — for completeness, results are identical between the two versions for json decoder behavior (no relevant API changes between 3.13 and 3.14). |
Summary
json.loadshad two distinct failure modes on JSON strings containing lone surrogate code points (U+D800–U+DFFF):UnicodeEncodeErrorwhen the Pythonstrpassed tojson.loadsitself carried a surrogate (e.g.,json.loads('"\ud83d"')) —JsonScanner::Callable::callcalledtry_into_utf8at entry.U+FFFDcorruption when surrogates arrived via JSON\uXXXXescapes inside arrays or dict keys (e.g.,json.loads('["\\uD83D"]')→['�']) —call_scan_onceandparse_objectcollapsed the scanner'sWtf8Bufoutput through.to_string(), which routes throughimpl Display for Wtf8(intentionally lossy withU+FFFD).CPython accepts surrogate-bearing strings in both positions and preserves them.
Root-level strings and mid-string surrogates (e.g.,
"a\uD83Db") already worked becauseJsonScanner::parseputwtf8_resultdirectly into a tuple, routing throughimpl ToPyObject for Wtf8Bufwhich preserves. Surrogate pair combining (😀→😀) also worked and is unchanged (handled inmachinery::scanstring).Switch
JsonScanner's fivePyUtf8StrRefsignatures toPyStrRef, drop thetry_into_utf8atCallable::callentry, and feedWtf8Bufdirectly tonew_strinstead of.to_string(). Memo keys change fromHashMap<String, PyStrRef>toHashMap<Wtf8Buf, PyStrRef>(Wtf8Bufalready implementsHash).parse_numbertakes&[u8]since JSON numbers are ASCII.machinery::scanstringis unchanged.This extends the WTF-8 refactor pattern from #7673 to the decoder side.
Verification
Targeted probes
json.loads('"\ud83d"')'\ud83d''\ud83d'json.loads('"\\uD83D"')'\ud83d''\ud83d''\ud83d'json.loads('"\\uD83D\\uDE00"')'😀''😀''😀'json.loads('["\\uD83D"]')['�']['\ud83d']['\ud83d']json.loads('{"\\uD83D": 1}'){'�': 1}{'\ud83d': 1}{'\ud83d': 1}json.loads('"a\\uD83Db"')'a\ud83db''a\ud83db''a\ud83db'CPython byte-identical fuzz
10,000 random JSON documents containing surrogate escapes at root/list/dict positions. Each decoded in both RustPython and CPython 3.13.4, then re-encoded via
json.dumps(obj, ensure_ascii=True, sort_keys=True)to canonical bytes. All 10,000 byte-identical. Seed 42.Sampled codepoint distribution: ASCII 30%, BMP pre-surrogate 20%, surrogate range 20%, BMP post-surrogate 20%, non-BMP 10%. Surrogate-pair escapes combine per RFC 8259 (handled in
machinery::scanstring).Additional coverage
Six
strict=Falsemode cases mixing surrogates with control characters (\x00,\x01,\t): all byte-identical with CPython.Twelve decode-error position cases with surrogates preceding the error site: eleven byte-identical with CPython. One pre-existing position off-by-one on
"\\uG000"(invalid\uescape inmachinery::scanstring) is outside this PR's scope.Memo cache semantics verified: repeated surrogate-bearing keys in the same document resolve correctly, distinct surrogate keys in one dict remain distinct, duplicate-key "last wins" preserved.
Test suite
test_single_surrogate_decodeskip override removed — now passes directly. No regressions.Pre-push
cargo fmt --all --checkcleancargo clippy -p rustpython-stdlib --all-targets -- -D warningscleanprek run --all-files— all hooks pass (merge conflicts, ruff format, redundant-test-patches, rustfmt, cspell, prettier, ruff check, opcode metadata)Scope
Decoder only. Encoder side was addressed in #7673. Skip marker removed from
Lib/test/test_json/test_unicode.py::TestCUnicode::test_single_surrogate_decode.Summary by CodeRabbit
Bug Fixes
Refactor