error, baseobjspace, function: four more surrogate-bearing surfaces, and the parity coverage for them - #1116
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
WalkthroughThe interpreter now preserves WTF-8 names and lone surrogates in exception, traceback, syntax-error, and bound-method output. Parity tests cover these cases, raising comparisons, and platform-specific timestamp clamping. ChangesWTF-8 preservation
Platform parity assertions
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 228d41f). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)None. 4. Structural adaptations
|
…aces as WTF-8 `wrap_dict_key_hash_error` and `wrap_set_element_hash_error` built the outer message from `message_text()`, whose display-side escape put the six characters spelling `\udcff` into the new exception's own `args[0]`; they take `message_wtf8()` instead. `method_repr` read the bound function's name with `w_str_get_value_opt`, so a `__qualname__` holding a lone surrogate collapsed the whole name to `?`. `write_syntax_error_object` replaced a constructor-supplied `text` that is not valid UTF-8 with `<unprintable>`; the line is kept as WTF-8 and the caret columns are counted in code points. `write_traceback_chain` wrote a frame's `co_filename` as the filesystem bytes, which left the report a mix of those bytes and the WTF-8 around it. The name is decoded for display and the bytes are kept for opening the source, so the buffer is WTF-8 throughout and both sinks spend the same `backslashreplace` encode on it. python3.14 renders all four cases with the escape, byte-identically. Assisted-by: Claude
…ertions `surrogate_name_messages` gains the bound-method repr and the dict-key / set-element hash wrappers, and its `ContextVar` LookupError case now names the variable with the surrogate-bearing string instead of `Repr.__name__`, which held no surrogate and could not have failed. `surrogate_traceback_render` gains the constructor-supplied `SyntaxError` text. Two `try` blocks in `mapdict_devolved_raising_eq` asserted that a raising `__eq__` propagates but had no `else` arm, so a read that returned a value was reported as a pass. Assisted-by: Claude
`os_utime_pathconf_truncate.py` asserted `st_mtime_ns == int(st_mtime) * 1_000_000_000` for a time past what the filesystem can hold. An APFS timestamp is an int64 of nanoseconds and clamps to `2**63 - 1`, whose remainder is 854775807ns, so the assertion failed on macOS under cpython, dynasm and cranelift alike. Assert instead that the count neither wrapped nor names a later instant than the one asked for, and that `st_mtime` is the double the same `(second, nanosecond)` pair derives. Assisted-by: Claude
1 participant
Follow-up to #1089, addressing its review findings. The PR merged before the
review was worked through, so the fixes land separately.
Four more surrogate-bearing surfaces (
4ac2cc9)wrap_dict_key_hash_errorandwrap_set_element_hash_errorbuilt the outermessage from
message_text(), whose display-side escape put the six charactersspelling
\udcffinto the new exception's ownargs[0]. They takemessage_wtf8()instead, so the wrapped value is carried as the value it is andescaping stays with whatever displays it.
method_reprread the bound function's name withw_str_get_value_opt, so a__qualname__holding a lone surrogate collapsed the whole name to?. It nowreads the name as WTF-8 under an
is_strfilter, which is what makes theunchecked read sound.
write_syntax_error_objectreplaced a constructor-suppliedtextthat is notvalid UTF-8 with
<unprintable>. The line is kept as WTF-8 and the caretcolumns are counted in code points, so the caret still lands under the reported
column.
write_traceback_chainwrote a frame'sco_filenameas the filesystem bytes,which left the report a mix of those bytes and the WTF-8 around it. The name is
decoded for display with the same handler that read it off the filesystem, and
the raw bytes are retained separately for opening the source, so the buffer is
WTF-8 throughout and both stderr sinks spend the same
backslashreplaceencodeon it.
python3.14 renders all four cases with the escape, byte-identically.
Parity coverage (
d360a18)surrogate_name_messagesgains the bound-method repr and the dict-key /set-element hash wrappers. Its
ContextVarLookupError case named the variablewith
Repr.__name__, which held no surrogate and so could not have failed; itnow uses the surrogate-bearing string.
surrogate_traceback_rendergains the constructor-suppliedSyntaxErrortext.Two
tryblocks inmapdict_devolved_raising_eqasserted that a raising__eq__propagates but had noelsearm, so a read that returned a value wasreported as a pass.
Review findings not fixed here
Two were checked against python3.14 and the current behaviour is correct, so
nothing changed: the property
__qualname__spelling and theImportErrorcase.
Three are filed as follow-up tasks rather than widened into this diff:
&strprobe indictmultiobjectwants a reentrancy audit underIndexMap::get__name__rather than__qualname__— verified byte-identical onmain, so it predates this workand is a separate divergence from the one fixed above
pyrexshould keep the script path anOsStringthroughparse_argsSummary by CodeRabbit
Bug Fixes
Tests