Report invalid \uXXXX escape position at the u character by changjoon-park · Pull Request #7676 · RustPython/RustPython

changjoon-park · 2026-04-24T19:39:10Z

Summary

json.loads of a string with an invalid \uXXXX escape reported a decode error position one character earlier than CPython: at the preceding \ instead of at the u specifier. For surrogate-pair paths (\uXXXX\uYYYY where the second escape is invalid), the position was off by more — it landed on the first hex digit of the first escape rather than on the second u.

Examples (before / after):

Input	CPython pos	Before	After
`json.loads('"\\uG000"')`	2	1	2
`json.loads('"abc\\uG000"')`	5	4	5
`json.loads('"\\u1234\\uG000"')`	8	7	8
`json.loads('"\\ud83d\\uG000"')`	8	3	8
`json.loads('"\\ud83d\\u"')`	8	3	8
`json.loads('"\\ud83d\\uDE0G"')`	8	3	8

Fix

In crates/stdlib/src/json/machinery.rs::scanstring:

Primary \uXXXX call: pass char_offset + next_char_i (position of the u) instead of char_offset + char_i (position of the \).
Surrogate-pair second-escape call: capture the second u's char index from the next_tuple() peek and pass char_offset + u2_char_i. The previous char_offset + next_char_i + 1 referred to the first escape's position — unrelated to where the second escape fails.

No behavioural change on the success path, on non-\u \escape errors, on unterminated strings, or on lone-surrogate preservation (pair filter unchanged). Decoded output bit-for-bit identical.

Verification

Targeted probes (13 cases)

Invalid hex digit at position 1/2/3/4, short escapes (\u, \u1, \u12, \u123), escape preceded by ASCII, valid-then-invalid, and surrogate-pair error paths. All positions match CPython 3.13.4 after the fix.

Test suite

$ ./target/release/rustpython -m unittest test.test_json
Ran 214 tests in 111s
OK (skipped=4, expected failures=12)

No regressions.

Pre-push

cargo fmt --all --check clean
cargo clippy -p rustpython-stdlib --all-targets -- -D warnings clean
prek run --all-files — all hooks pass

Scope

Single file, +7/-4 lines (3 substantive + 4 comment lines). Independent from #7675 (decoder WTF-8 refactor) — this PR touches machinery::scanstring error-reporting path only; #7675 touches json.rs scanner frontend. Either order of merge is fine.

Summary by CodeRabbit

Bug Fixes
- Improved JSON parsing error reporting for malformed Unicode escape sequences so error positions point precisely at the invalid escape, matching expected behavior and making debugging clearer.
Tests
- Added tests that validate invalid Unicode-escape handling and confirm reported error positions match the expected locations.

CPython's json decoder reports the position of the `u` specifier when a \uXXXX escape fails to parse, but RustPython was reporting the preceding `\`. For surrogate-pair cases (\uXXXX\uYYYY) the second call was passing char_offset + next_char_i + 1, which lands on the first hex digit of the first escape -- unrelated to the actual failure site. Pass next_char_i (position of the primary `u`) to the primary decode_unicode call, and capture the second `u`'s char index from the next_tuple peek to pass to the surrogate-pair decode_unicode call. Verified: 13 targeted probes across invalid-hex, short, and pair cases now all match CPython positions. test.test_json 214 tests pass with no regressions.

coderabbitai · 2026-04-24T19:39:23Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: d2be6e77-22ee-46e3-a838-12c388cf3090

📥 Commits

Reviewing files that changed from the base of the PR and between cf9c6b6 and 3ce5dda.

📒 Files selected for processing (1)

extra_tests/snippets/stdlib_json.py

📝 Walkthrough

Walkthrough

scanstring now passes the index of the u character into decode_unicode for both single \u decodes and surrogate-pair second \u, so DecodeError.pos for invalid \uXXXX sequences points at the u (matching CPython). Tests were added to assert positions.

Changes

Cohort / File(s)	Summary
JSON Unicode Escape Error Reporting `crates/stdlib/src/json/machinery.rs`	Adjust `scanstring` to pass the `u` character index into `decode_unicode`; update iterator destructuring to capture the second `u` index for surrogate-pair handling so error positions point at `'u'`.
Tests: JSON malformed Unicode escapes `extra_tests/snippets/stdlib_json.py`	Add negative tests asserting `json.loads` raises `json.JSONDecodeError` for malformed `\uXXXX` sequences and that `e.pos` equals the `u` positions (matching CPython).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

youknowone
ShaharNaveh

Poem

🐰 I hop through strings where backslashes play,
I find the \u and point the way.
Now errors land on the tiny u,
A carrot fix—precise and true! 🥕

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly and clearly describes the primary change: fixing the error position reporting for invalid \uXXXX escapes to point at the 'u' character rather than the backslash, matching CPython behavior.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

youknowone

was there any test affected by this change?

changjoon-park · 2026-04-25T04:30:38Z

Re-verified against CPython 3.14.4 (RustPython's stated target version): all 13 escape-position probe cases match. Original PR body referenced 3.13.4 because that was the system Python at probe time — for completeness, the error position convention is identical between 3.13 and 3.14 (the validator code in Python/ast.c has been stable for this since at least 3.10).

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

extra_tests/snippets/stdlib_json.py (1)
244-257: Optional: add surrogate-pair second-\u coverage.

The PR description highlights the surrogate-pair path (\uXXXX\uYYYY) where the second u's position was previously off by one (landing on the first hex digit of the first escape). The two added tests only exercise the primary \u path. Consider adding a case like '"\\uD834\\uXYZW"' and asserting e.pos == 8 (the second u) so the regression coverage matches the full scope of the fix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@extra_tests/snippets/stdlib_json.py` around lines 244 - 257, Add a test that
exercises the surrogate-pair second-`\u` path: call json.loads with the string
'"\\uD834\\uXYZW"' and catch json.JSONDecodeError, asserting that e.pos == 8
(the position of the second 'u'); place this alongside the existing
invalid-escape tests so the surrogate-pair case (referencing json.loads and
json.JSONDecodeError) verifies the regression fix for the second `\u` position.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@extra_tests/snippets/stdlib_json.py`:
- Around line 249-250: Replace the insecure "else: assert False, 'expected
JSONDecodeError'" pattern with an explicit raise so the check isn't removed
under -O; change those else branches to "raise AssertionError('expected
JSONDecodeError')" (apply to both occurrences around the JSONDecodeError test
blocks).

---

Nitpick comments:
In `@extra_tests/snippets/stdlib_json.py`:
- Around line 244-257: Add a test that exercises the surrogate-pair second-`\u`
path: call json.loads with the string '"\\uD834\\uXYZW"' and catch
json.JSONDecodeError, asserting that e.pos == 8 (the position of the second
'u'); place this alongside the existing invalid-escape tests so the
surrogate-pair case (referencing json.loads and json.JSONDecodeError) verifies
the regression fix for the second `\u` position.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 56065126-ff7a-4d9e-8ff8-faf24914efee

📥 Commits

Reviewing files that changed from the base of the PR and between 30c0bfc and cf9c6b6.

📒 Files selected for processing (1)

extra_tests/snippets/stdlib_json.py

changjoon-park · 2026-04-25T15:21:43Z

there was no existing cpython test in Lib/test/test_json/ checks the exact "pos" value for invalid \uXXXX escapes — they only regex-match the message text, so this fix doesn't unmask any test

so i added a focused regression test in extra_tests/snippets/stdlib_json.py covering both the leading-position case ("\uXYZW" → pos=2) and the offset case ("abc\uZZZZ" → pos=5). Verified byte-identical against CPython 3.14.4

thanks for catching !

youknowone

👍

youknowone reviewed Apr 24, 2026

View reviewed changes

youknowone enabled auto-merge (squash) April 24, 2026 20:18

Add regression test for invalid \uXXXX escape position

cf9c6b6

auto-merge was automatically disabled April 25, 2026 15:17
Head branch was pushed to by a user without write access

coderabbitai Bot reviewed Apr 25, 2026

View reviewed changes

Comment thread extra_tests/snippets/stdlib_json.py Outdated

Use raise AssertionError instead of assert False (B011)

3ce5dda

youknowone approved these changes Apr 26, 2026

View reviewed changes

youknowone merged commit 625e5bf into RustPython:main Apr 26, 2026
20 checks passed

changjoon-park deleted the fix-json-uxxxx-error-position branch April 27, 2026 13:24

Navigation Menu

Uh oh!

Report invalid \uXXXX escape position at the u character - #7676

Report invalid \uXXXX escape position at the u character#7676
youknowone merged 3 commits into
RustPython:mainfrom
changjoon-park:fix-json-uxxxx-error-position

changjoon-park commented Apr 24, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Apr 24, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Suggested reviewers

Poem

Uh oh!

youknowone left a comment

Uh oh!

changjoon-park commented Apr 25, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

changjoon-park commented Apr 25, 2026

Uh oh!

youknowone left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

changjoon-park commented Apr 24, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Fix

Verification

Targeted probes (13 cases)

Test suite

Pre-push

Scope

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Suggested reviewers

Poem

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

changjoon-park commented Apr 25, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

changjoon-park commented Apr 25, 2026

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

changjoon-park commented Apr 24, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Apr 24, 2026 •

edited

Loading