Fold a virtual's never-stored field read, check its slot descr, and carry surrogate-bearing text as WTF-8 - #1089
Conversation
|
Warning Review limit reached
Next review available in: 41 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (17)
WalkthroughThe PR migrates interpreter representations, errors, filesystem paths, imports, and module APIs to WTF-8-aware handling. It also strengthens virtual-field validation, preserves dictionary comparison errors, refines JIT compile-outcome handling, and adds parity tests for lone surrogates. ChangesWTF-8 interpreter migration
JIT compilation and virtual fields
Checked dictionary lookup propagation
Estimated code review effort: 5 (Critical) | ~120 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ec0c55eb5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let w_key = crate::w_str_from_wtf8(key.to_wtf8_buf()); | ||
| w_dict_lookup_checked(obj, w_key) | ||
| match key.as_str() { | ||
| Ok(s) => w_dict_getitem_str_checked(obj, s), |
There was a problem hiding this comment.
When a devolved object dict contains a hash-colliding non-string key whose __eq__ mutates that dict, this arm now reaches dict_entries_probe_str, which holds an IndexMap::get borrow while executing the callback. Reentrant mutation can invalidate that borrow and return a removed entry or access reallocated storage. The previous w_dict_lookup_checked path avoids this via callback_free_dict_op! followed by scan_dict_key_reentrant, including the table-capacity/key-position restart checks. Keep that checked reentrant behavior while optimizing away only the temporary string allocation.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
youknowone
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 324dd40681
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let field_val = match &info { | ||
| _ if !slot_resolvable => None, | ||
| PtrInfo::Virtual(vinfo) => get_field(&vinfo.fields, field_idx), |
There was a problem hiding this comment.
In release builds, field_slot_disagreement is disabled, so optimize_setfield_gc can still store a value at field_idx for a mismatched descriptor; this new arm then deliberately ignores that tracked value, causing a subsequent GETFIELD_GC with the same descriptor to return zero rather than the value just stored. The concrete vable_token descriptor currently has the placeholder index 0, so a virtual frame that encounters both its token store and read can hit this inconsistency. Assign the appended field its correct generated descriptor/index and retain the normal upstream getfield path rather than masking descriptor-generation defects with a general zero-fold shortcut.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 96c86bc). Files in the reviewed diffCodex did not produce a report (exit 1). Last log lines: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5f1a235c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "cannot use '{}' as a dict key ({})", | ||
| object_functionstr_type_name(key), | ||
| err.message, | ||
| err.message_text(), |
There was a problem hiding this comment.
When an exact TypeError raised by __hash__ contains a lone surrogate, message_text() applies the display-side backslashreplace conversion before constructing the outer dict-key error. Consequently, e.args[0] contains the six literal characters \ud800 instead of the original surrogate; Python 3.14 preserves the surrogate in both the dict-key and set-element wrappers. Assemble these messages from err.message_wtf8() instead; the stream should perform escaping only when the final exception is displayed.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
| } | ||
| wtf8_of(w).map(|wtf8| match wtf8.as_str() { | ||
| Ok(s) => s.to_owned(), | ||
| Err(_) => "<unprintable>".to_string(), |
There was a problem hiding this comment.
For a user-constructed SyntaxError whose text field contains a lone surrogate, this branch replaces the entire offending source line with <unprintable>. Such values are valid Python strings—for example, SyntaxError('bad', ('x.py', 1, 1, '\ud800\n', 1, 2))—and Python 3.14 displays the line using backslashreplace (\ud800) while retaining the caret. Preserve the WTF-8 text through rendering, or escape it without discarding the line.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b302f1a56
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // still names the file to whatever reads the stream, and re-reading it | ||
| // as UTF-8 would substitute U+FFFD for the one byte that carries the | ||
| // name. | ||
| None => crate::host_seam::emit_stderr(buf), |
There was a problem hiding this comment.
When an imported module lives at a Unix path containing an undecodable byte, load_source_module now stores that raw filesystem spelling in co_filename, and write_traceback_chain copies it into this buffer. Wtf8::from_bytes then returns None, so this arm sends the entire mixed report directly to stderr and bypasses backslashreplace: the path byte is emitted as invalid UTF-8 instead of the expected \udcff escape, and any surrogate elsewhere in the exception report is also emitted as raw WTF-8. Decode frame filenames to WTF-8 while assembling the report, retaining the raw bytes separately only for source-file lookup.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pyre/pyre-interpreter/src/_pypy_generic_alias.rs (2)
1322-1334: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winA surrogate-bearing
__qualname__silently changes rendering.
text_wreturns a&str, so a__qualname__holding a lone surrogate fails theOk(...)arm at line 1323 and falls through topy_repr_wtf8at line 1334. The item then renders as a quotedreprinstead of the dotted qualname, which is the shape this branch exists to produce. The rest of this function was converted to WTF-8 for exactly this case.Read the qualname and module through the WTF-8 accessors and assemble the result with
wtf8_format!.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyre/pyre-interpreter/src/_pypy_generic_alias.rs` around lines 1322 - 1334, Update the qualname-rendering branch around current_item() to use WTF-8 accessors for both __qualname__ and __module__ instead of text_w, preserving surrogate-containing names. Assemble the optional module prefix and qualname with rustpython_wtf8::wtf8_format!, retaining the builtins omission and py_repr_wtf8 fallback.
528-568: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBuild the argument-count errors with
wtf8_format!.
py_repr_wtf8(self)returns aWtf8Buf, but the twoformat!calls interpolate it throughDisplay. That substitutes U+FFFD for lone surrogates. Usecrate::display::wtf8_format!for the literal prefix/suffix chunks and thesfragment so the repr bytes survive.🐛 Proposed fix for the two argument-count messages
if nitems < required { - return Err(crate::PyError::type_error(format!( - "Too few arguments for {s}; actual {nitems}, expected at least {required}" - ))); + return Err(crate::PyError::type_error(crate::display::wtf8_format!( + "Too few arguments for ", + s, + format!("; actual {nitems}, expected at least {required}") + ))); } } - return Err(crate::PyError::type_error(format!( - "Too {direction} arguments for {s}; actual {nitems}, expected {nparams}" - ))); + return Err(crate::PyError::type_error(crate::display::wtf8_format!( + format!("Too {direction} arguments for "), + s, + format!("; actual {nitems}, expected {nparams}") + )));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyre/pyre-interpreter/src/_pypy_generic_alias.rs` around lines 528 - 568, Update both argument-count error messages in the shown validation flow to use crate::display::wtf8_format! instead of format!, including the literal prefix/suffix chunks and the s Wtf8Buf fragment, so lone-surrogate repr bytes are preserved while retaining the existing message text and values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/extra_tests/parity_tests/mapdict_devolved_raising_eq.py`:
- Around line 65-70: In
pyre/extra_tests/parity_tests/mapdict_devolved_raising_eq.py at lines 65-70, add
an else branch after the KeyError handler that raises AssertionError("raising
__eq__ swallowed on the dict subscript"). At lines 130-135, add an else branch
after the AttributeError handler that raises AssertionError("raising __eq__
swallowed on a surrogate name"), matching the established must-raise blocks.
In `@pyre/extra_tests/parity_tests/surrogate_name_messages.py`:
- Around line 103-109: Update the LookupError fixture around unset so the
ContextVar name includes a lone surrogate rather than using Repr.__name__
("Repr"). Keep the assertion comparing str(e) with repr(unset), ensuring the
test exercises surrogate preservation through the ContextVar name and error
message.
In `@pyre/pyre-interpreter/src/baseobjspace.rs`:
- Around line 16624-16632: Update the property error-message construction around
the `qualname` value to obtain the type’s `__qualname__` as WTF-8 instead of
using `py_str_display`, then splice it into both message branches with
`wtf8_format!` so lone surrogates remain unescaped. Remove the now-stale comment
explaining the escaped representation.
In `@pyre/pyre-interpreter/src/error.rs`:
- Around line 1493-1499: Update the non-WTF-8 fallback in the buffer conversion
near w_text to use the repository’s filesystem decoder helper, such as
fsdecode_filename_bytes, instead of String::from_utf8_lossy. Preserve
undecodable filename bytes via surrogateescape so os.fsencode can round-trip
them, while leaving the successful Wtf8 conversion unchanged.
In `@pyre/pyre-interpreter/src/function.rs`:
- Around line 2817-2821: Update the bound-method representation code around the
method-name lookup to read the name with w_str_get_wtf8(...).to_wtf8_buf()
instead of w_str_get_value_opt, preserving surrogate-bearing __qualname__ or
__name__ values when formatting the output with wtf8_format!.
In `@pyre/pyre-interpreter/src/importing.rs`:
- Around line 4016-4052: Keep the ImportError message construction in WTF-8
throughout. Update the branches in the surrounding import-error logic that
include pkgpath to use text_wtf8_w() for the path and wtf8_format! (or
equivalent Wtf8Buf assembly), avoiding utf8_w()/format! so lone surrogates in
__file__ do not raise before the error is returned.
In `@pyre/pyrex/src/lib.rs`:
- Around line 607-611: Preserve the script argument as an OsString or PathBuf
through parse_args and RunMode::Script instead of converting it with
Value(script).string()?. At filesystem call sites, including the path
construction, absolute-path resolution, and decode_source_bytes input near
pyre_interpreter::decode_source_bytes, use Path::new(&path),
std::path::absolute(&path), and Wtf8::new(&path) so non-UTF-8 Unix bytes remain
intact.
---
Outside diff comments:
In `@pyre/pyre-interpreter/src/_pypy_generic_alias.rs`:
- Around line 1322-1334: Update the qualname-rendering branch around
current_item() to use WTF-8 accessors for both __qualname__ and __module__
instead of text_w, preserving surrogate-containing names. Assemble the optional
module prefix and qualname with rustpython_wtf8::wtf8_format!, retaining the
builtins omission and py_repr_wtf8 fallback.
- Around line 528-568: Update both argument-count error messages in the shown
validation flow to use crate::display::wtf8_format! instead of format!,
including the literal prefix/suffix chunks and the s Wtf8Buf fragment, so
lone-surrogate repr bytes are preserved while retaining the existing message
text and values.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 64d95db3-0075-4f9d-b822-eb2efe3a6567
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (67)
majit/majit-metainterp/src/lib.rsmajit/majit-metainterp/src/optimizeopt/virtualize.rsmajit/majit-metainterp/src/pyjitpl.rspyre/extra_tests/parity_tests/mapdict_devolved_raising_eq.pypyre/extra_tests/parity_tests/surrogate_name_messages.pypyre/extra_tests/parity_tests/surrogate_str_roundtrip.pypyre/extra_tests/parity_tests/surrogate_traceback_render.pypyre/pyre-interpreter/src/_pypy_generic_alias.rspyre/pyre-interpreter/src/argument.rspyre/pyre-interpreter/src/astcompiler/validate.rspyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/call.rspyre/pyre-interpreter/src/compile.rspyre/pyre-interpreter/src/display.rspyre/pyre-interpreter/src/error.rspyre/pyre-interpreter/src/eval.rspyre/pyre-interpreter/src/executioncontext.rspyre/pyre-interpreter/src/function.rspyre/pyre-interpreter/src/gateway.rspyre/pyre-interpreter/src/importing.rspyre/pyre-interpreter/src/launch_env.rspyre/pyre-interpreter/src/lib.rspyre/pyre-interpreter/src/module/__pypy__/mod.rspyre/pyre-interpreter/src/module/_ast/convert.rspyre/pyre-interpreter/src/module/_collections/mod.rspyre/pyre-interpreter/src/module/_contextvars/mod.rspyre/pyre-interpreter/src/module/_csv/mod.rspyre/pyre-interpreter/src/module/_ctypes/cdata.rspyre/pyre-interpreter/src/module/_ctypes/funcptr.rspyre/pyre-interpreter/src/module/_ctypes/interp_ctypes.rspyre/pyre-interpreter/src/module/_io/buffered.rspyre/pyre-interpreter/src/module/_io/buffered_random.rspyre/pyre-interpreter/src/module/_io/buffered_writer.rspyre/pyre-interpreter/src/module/_io/stringio.rspyre/pyre-interpreter/src/module/_io/textio.rspyre/pyre-interpreter/src/module/_json/mod.rspyre/pyre-interpreter/src/module/_pickle/mod.rspyre/pyre-interpreter/src/module/_pickle/pickler.rspyre/pyre-interpreter/src/module/_socket/interp_socket.rspyre/pyre-interpreter/src/module/_sre/interp_sre.rspyre/pyre-interpreter/src/module/_symtable/mod.rspyre/pyre-interpreter/src/module/_tokenize/mod.rspyre/pyre-interpreter/src/module/_weakref/interp__weakref.rspyre/pyre-interpreter/src/module/binascii/mod.rspyre/pyre-interpreter/src/module/importlib/interp_importlib.rspyre/pyre-interpreter/src/module/posix/interp_posix.rspyre/pyre-interpreter/src/module/signal/interp_signal.rspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-interpreter/src/module/thread/mod.rspyre/pyre-interpreter/src/module/time/interp_time.rspyre/pyre-interpreter/src/module/unicodedata/mod.rspyre/pyre-interpreter/src/objspace/descroperation.rspyre/pyre-interpreter/src/opcode_ops.rspyre/pyre-interpreter/src/pyframe.rspyre/pyre-interpreter/src/pyopcode.rspyre/pyre-interpreter/src/runtime_ops.rspyre/pyre-interpreter/src/type_methods.rspyre/pyre-interpreter/src/typedef.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit/tests/gc_stress.rspyre/pyre-macros/src/lib.rspyre/pyre-object/src/dictmultiobject.rspyre/pyrex/Cargo.tomlpyre/pyrex/src/lib.rspyre/pyrex/src/repl.rs
| try: | ||
| r.__dict__["zz"] | ||
| except ValueError: | ||
| pass | ||
| except KeyError: | ||
| raise AssertionError("raising __eq__ reported as a missing key") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Two "must raise" blocks accept a successful read. Both blocks assert that a raising __eq__ in the probe propagates, but neither has an else arm. If the read returns a value instead of raising, the block falls through and the test reports success. The blocks at lines 48-53 and 96-101 in the same file show the intended shape.
pyre/extra_tests/parity_tests/mapdict_devolved_raising_eq.py#L65-L70: addelse: raise AssertionError("raising __eq__ swallowed on the dict subscript")after theexcept KeyErrorarm.pyre/extra_tests/parity_tests/mapdict_devolved_raising_eq.py#L130-L135: addelse: raise AssertionError("raising __eq__ swallowed on a surrogate name")after theexcept AttributeErrorarm.
💚 Proposed fix for both blocks
try:
r.__dict__["zz"]
except ValueError:
pass
except KeyError:
raise AssertionError("raising __eq__ reported as a missing key")
+else:
+ raise AssertionError("raising __eq__ swallowed on the dict subscript") try:
getattr(s2, SURROGATE)
except ValueError:
pass
except AttributeError:
raise AssertionError("raising __eq__ reported as a missing attribute")
+else:
+ raise AssertionError("raising __eq__ swallowed on a surrogate name")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| r.__dict__["zz"] | |
| except ValueError: | |
| pass | |
| except KeyError: | |
| raise AssertionError("raising __eq__ reported as a missing key") | |
| try: | |
| r.__dict__["zz"] | |
| except ValueError: | |
| pass | |
| except KeyError: | |
| raise AssertionError("raising __eq__ reported as a missing key") | |
| else: | |
| raise AssertionError("raising __eq__ swallowed on the dict subscript") |
| try: | |
| r.__dict__["zz"] | |
| except ValueError: | |
| pass | |
| except KeyError: | |
| raise AssertionError("raising __eq__ reported as a missing key") | |
| try: | |
| getattr(s2, SURROGATE) | |
| except ValueError: | |
| pass | |
| except AttributeError: | |
| raise AssertionError("raising __eq__ reported as a missing attribute") | |
| else: | |
| raise AssertionError("raising __eq__ swallowed on a surrogate name") |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 70-70: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[warning] 70-70: Avoid specifying long messages outside the exception class
(TRY003)
📍 Affects 1 file
pyre/extra_tests/parity_tests/mapdict_devolved_raising_eq.py#L65-L70(this comment)pyre/extra_tests/parity_tests/mapdict_devolved_raising_eq.py#L130-L135
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/extra_tests/parity_tests/mapdict_devolved_raising_eq.py` around lines 65
- 70, In pyre/extra_tests/parity_tests/mapdict_devolved_raising_eq.py at lines
65-70, add an else branch after the KeyError handler that raises
AssertionError("raising __eq__ swallowed on the dict subscript"). At lines
130-135, add an else branch after the AttributeError handler that raises
AssertionError("raising __eq__ swallowed on a surrogate name"), matching the
established must-raise blocks.
| # A variable with no default names itself by repr in the LookupError, so | ||
| # the message carries whatever its own repr does. | ||
| unset = contextvars.ContextVar(Repr.__name__) | ||
| try: | ||
| unset.get() | ||
| except LookupError as e: | ||
| assert str(e) == repr(unset), ascii(str(e)) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use the surrogate-bearing name in the LookupError fixture.
Repr.__name__ is "Repr", so this path contains no lone surrogate. The assertion can pass even if ContextVar names are converted through a lossy String path.
Proposed test fix
- unset = contextvars.ContextVar(Repr.__name__)
+ unset = contextvars.ContextVar(S)
try:
unset.get()
except LookupError as e:
assert str(e) == repr(unset), ascii(str(e))
+ assert FFFD not in str(e), ascii(str(e))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # A variable with no default names itself by repr in the LookupError, so | |
| # the message carries whatever its own repr does. | |
| unset = contextvars.ContextVar(Repr.__name__) | |
| try: | |
| unset.get() | |
| except LookupError as e: | |
| assert str(e) == repr(unset), ascii(str(e)) | |
| # A variable with no default names itself by repr in the LookupError, so | |
| # the message carries whatever its own repr does. | |
| unset = contextvars.ContextVar(S) | |
| try: | |
| unset.get() | |
| except LookupError as e: | |
| assert str(e) == repr(unset), ascii(str(e)) | |
| assert FFFD not in str(e), ascii(str(e)) |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 109-109: Found assertion on exception e in except block, use pytest.raises() instead
(PT017)
[warning] 109-109: Found assertion on exception e in except block, use pytest.raises() instead
(PT017)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/extra_tests/parity_tests/surrogate_name_messages.py` around lines 103 -
109, Update the LookupError fixture around unset so the ContextVar name includes
a lone surrogate rather than using Repr.__name__ ("Repr"). Keep the assertion
comparing str(e) with repr(unset), ensuring the test exercises surrogate
preservation through the ContextVar name and error message.
| // The buffer is assembled from text the printer already escaped, so | ||
| // it is well-formed; a decode failure would mean a byte no writer put | ||
| // there, and the lossy read is the last resort for it. | ||
| let w_text = match rustpython_wtf8::Wtf8::from_bytes(buf) { | ||
| Some(text) => pyre_object::w_str_from_wtf8(text.to_wtf8_buf()), | ||
| None => pyre_object::w_str_new(&String::from_utf8_lossy(buf)), | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The sys.stderr path still folds filesystem bytes to U+FFFD.
The comment at lines 1493-1495 states that a decode failure would mean a byte no writer put in the buffer. That premise does not hold. write_exception reaches write_traceback_chain_from_tb, which writes a frame's co_filename as raw filesystem bytes (lines 2869-2871), and emit_report_to_host_stderr documents the same case at lines 3145-3152. A path byte with no UTF-8 spelling therefore reaches this buffer.
The result is that the same report loses the filename byte when sys.stderr exists and keeps it when the host fd is used. Decode the buffer with the filesystem decoder (surrogateescape) instead of String::from_utf8_lossy, so the byte survives as its escape and os.fsencode still round-trips it.
🐛 Proposed fix for the non-WTF-8 fallback
let w_text = match rustpython_wtf8::Wtf8::from_bytes(buf) {
Some(text) => pyre_object::w_str_from_wtf8(text.to_wtf8_buf()),
- None => pyre_object::w_str_new(&String::from_utf8_lossy(buf)),
+ // A frame's `co_filename` rides the report as filesystem bytes, so
+ // the buffer is not always WTF-8. Decode it the way every other
+ // filesystem name is decoded, keeping the byte as its escape.
+ None => crate::gateway::fsdecode_filename_bytes(buf),
};Run the following script to confirm the helper name and return type:
#!/bin/bash
# Description: Inspect the filesystem decoding helpers available to error.rs.
set -euo pipefail
rg -n -C 6 'pub fn fsdecode_filename_bytes|pub fn fsdecode_filename_wtf8|pub fn fsdecode_os_str' --type=rust🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-interpreter/src/error.rs` around lines 1493 - 1499, Update the
non-WTF-8 fallback in the buffer conversion near w_text to use the repository’s
filesystem decoder helper, such as fsdecode_filename_bytes, instead of
String::from_utf8_lossy. Preserve undecodable filename bytes via surrogateescape
so os.fsencode can round-trip them, while leaving the successful Wtf8 conversion
unchanged.
| let instance_repr = unsafe { crate::display::py_repr_wtf8(instance)? }; | ||
| Ok(pyre_object::w_str_from_wtf8(crate::display::wtf8_format!( | ||
| format!("<bound method {name} of "), | ||
| instance_repr, | ||
| ">" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant implementation and symbol definitions.
printf '--- function.rs context ---\n'
sed -n '2780,2845p' pyre/pyre-interpreter/src/function.rs | cat -n
printf '\n--- definitions/usages of w_str_get_value_opt and w_str_get_wtf8 ---\n'
rg -n "w_str_get_value_opt|w_str_get_wtf8|fn w_str_get_value_opt|pub fn w_str_get_wtf8|pub unsafe fn w_str_get_value_opt" pyre -S
printf '\n--- object/string implementation files ---\n'
rg -n "w_str_get_value_opt|w_str_get_wtf8|value_opt|wtf8" pyre/pyre-interpreter/src -S --glob '*.rs' | head -200Repository: youknowone/pyre
Length of output: 244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Programmatic semantic probe of Rust's str::from_utf8 validation for surrogates,
# if the needed functions cannot be inspected statically, and identify the exact helper body.
python3 - <<'PY'
from pathlib import Path
p=Path('pyre/pyre-interpreter/src')
for text in sorted(p.glob('*')):
if text.is_file():
s=text.read_text(errors='replace')
if 'w_str_get_value_opt' in s or 'w_str_get_wtf8' in s:
print(f'\n### {text} contains target symbols')
lines=s.splitlines()
for i,l in enumerate(lines,1):
if 'w_str_get_value_opt' in l or 'w_str_get_wtf8' in l:
lo=max(1,i-6); hi=min(len(lines),i+40)
print('\n'.join(f'{n:5}\t{lines[n-1]}' for n in range(lo,hi+1)))
PY
printf '\n--- Rust str::from_utf8 surrogate behavior in the local toolchain ---\n'
cat > /tmp/rust_str_surrogate_probe.rs <<'RS'
fn main() {
let b = [0xC3, 0xA9, 0xED, 0xA0, 0x80, 0xFF];
println!("{:?}", std::str::from_utf8(&b).is_ok());
}
RS
rustc /tmp/rust_str_surrogate_probe.rs -o /tmp/rust_str_surrogate_probe && /tmp/rust_str_surrogate_probeRepository: youknowone/pyre
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- function.rs context ---'
sed -n '2780,2845p' pyre/pyre-interpreter/src/function.rs | cat -n
printf '%s\n' '--- targeted definitions/usages of w_str_get_value_opt and w_str_get_wtf8 ---'
rg -n -C 4 'fn w_str_get_value_opt|unsafe fn w_str_get_value_opt|pub unsafe fn w_str_get_value_opt|w_str_get_value_opt|fn w_str_get_wtf8|unsafe fn w_str_get_wtf8|pub unsafe fn w_str_get_wtf8|w_str_get_wtf8' \
pyre/pyre-interpreter/src/pyre_object.rs pyre/pyre-interpreter/src -S --glob '*.rs' | sed -n '1,240p'
printf '%s\n' '--- local symbol references with surrounding line numbers ---'
rg -n -C 8 'w_str_get_value_opt|fmt!|format!|wtf8_format!\(|py_repr_wtf8|__qualname__' pyre/pyre-interpreter/src/function.rs | sed -n '1,220p'Repository: youknowone/pyre
Length of output: 23258
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- pyre_object string api definitions ---'
rg -n -C 5 'pub unsafe fn w_str_get_value_opt|unsafe fn w_str_get_value_opt|w_str_get_value_opt|pub unsafe fn w_str_get_wtf8|unsafe fn w_str_get_wtf8|w_str_get_wtf8' pyre/pyre-interpreter/src --glob '*.rs' | sed -n '1,260p'
printf '%s\n' '--- candidate source files containing w_str implementations ---'
rg -n 'w_str_get_value_opt|w_str_get_wtf8|fn wstr|struct WString|pub struct WStr|impl.*str' pyre/pyre-interpreter/src --glob '*.rs' | sed -n '1,220p'Repository: youknowone/pyre
Length of output: 46750
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- object module files ---'
git ls-files pyre/pyre-interpreter/src | rg '(^|/)object(_.*|)\.rs$|pyre_object|unicodeobject' | sed -n '1,160p'
printf '%s\n' '--- exact definitions with file names ---'
python3 - <<'PY'
from pathlib import Path
for p in Path('pyre/pyre-interpreter/src').rglob('*.rs'):
s=p.read_text(errors='replace')
if 'pub unsafe fn w_str_get_value_opt' in s or 'pub unsafe fn w_str_get_wtf8' in s or 'pub fn w_str_get_wtf8' in s or 'pub fn w_str_get_value_opt' in s:
lines=s.splitlines()
for i,l in enumerate(lines,1):
if 'w_str_get_value_opt' in l or 'w_str_get_wtf8' in l:
print(f'## {p}:{i}: {l.strip()}')
for n in range(max(1,i-12), min(len(lines), i+25)+1):
print(f'{n:5}\t{lines[n-1]}')
PY
printf '%s\n' '--- all def sites for string getters ---'
rg -n 'w_str_get_value_opt|w_str_get_value|to_str|to_wtf8|to_string|as_str|Wtf8' pyre/pyre-interpreter/src --glob '*.rs' | rg 'w_str_get_value_opt|w_str_get_wtf8|to_str\(|to_wtf8\(|to_string\(|as_str\(|fn .*wtf8|struct Wtf8' || trueRepository: youknowone/pyre
Length of output: 181
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- object-related files ---'
git ls-files pyre/pyre-interpreter/src | rg '(^|/)object(_.*|)\.rs$|pyre_object' || true
printf '%s\n' '--- exact definitions with file names ---'
python3 - <<'PY'
from pathlib import Path
for p in Path('pyre/pyre-interpreter/src').rglob('*.rs'):
s=p.read_text(errors='replace')
if 'pub unsafe fn w_str_get_value_opt' in s or 'pub unsafe fn w_str_get_wtf8' in s or 'pub fn w_str_get_wtf8' in s or 'pub fn w_str_get_value_opt' in s:
lines=s.splitlines()
for i,l in enumerate(lines,1):
if 'w_str_get_value_opt' in l or 'w_str_get_wtf8' in l:
print(f'## {p}:{i}: {l.strip()}')
for n in range(max(1,i-12), min(len(lines), i+25)+1):
print(f'{n:5}\t{lines[n-1]}')
PY
printf '%s\n' '--- all def sites for string getters ---'
rg -n 'w_str_get_value_opt|to_str\(|to_wtf8\(|to_string\(|as_str\(|fn .*wtf8|struct Wtf8|impl Wtf8' pyre/pyre-interpreter/src --glob '*.rs' | sed -n '1,240p' || trueRepository: youknowone/pyre
Length of output: 26684
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- module imports of pyre_object ---'
rg -n 'use .*pyre_object|pub use .*pyre_object' pyre/pyre-interpreter/src --glob '*.rs' | sed -n '1,220p'
printf '%s\n' '--- exact source with definitions via python parse extraction ---'
python3 - <<'PY'
from pathlib import Path
terms=['w_str_get_value_opt', 'w_str_get_value', 'w_str_get_wtf8']
for p in Path('pyre/pyre-interpreter/src').rglob('*.rs'):
lines=p.read_text(errors='replace').splitlines()
hits=[]
for i,l in enumerate(lines,1):
if any(t in l for t in terms):
hits.append((i,l))
if hits:
print(f'## {p}')
for i,l in hits[:120]:
lo=max(1,i-8)
hi=min(len(lines),i+22)
for n in range(lo,hi+1):
print(f'{n:5}\t{lines[n-1]}')
print()
PY
printf '%s\n' '--- read-only semantic probe of UTF-8 validation model ---'
python3 - <<'PY'
samples = [
('\u00e9', 'U+00E9 byte sequence'),
('\ud800', 'Lone high surrogate'),
('\ud800\uDC00', 'Pair surrogate'),
('\ud800\u00e9', 'Lone high + valid char'),
]
for s,n in samples:
print(n, 'bytes=', s.encode('utf-8','surrogatepass').hex(), 'valid utf8=', s.encode('utf-8','surrogatepass').decode('utf-8', 'strict')==s)
PYRepository: youknowone/pyre
Length of output: 50372
🌐 Web query:
Rust std string from_utf8 accepts lone surrogates surrogatepass
💡 Result:
Rust’s String::from_utf8 does NOT accept UTF-8 byte sequences that decode to lone surrogate code points (U+D800..=U+DFFF). It returns Err(FromUtf8Error) when the input is not valid UTF-8 (which includes rejecting surrogate code points) [1][2][3][4]. Supporting details: 1) Rust’s from_utf8 checks that the bytes are valid UTF-8 and returns an error otherwise [1][2]. 2) Rust’s UTF-8 validation explicitly excludes surrogate code points: the core validation comments describe “excluding surrogates codepoints u{d800} to u{dfff}” (e.g., ED A0 80..= ED BF BF) [4]. 3) From the API standpoint, from_utf8 returns a Result and documents that it returns Err if the slice is not UTF-8 [1][2].
Citations:
- 1: https://doc.rust-lang.org/std/string/struct.String.html
- 2: https://doc.rust-lang.org/std/str/fn.from_utf8.html
- 3: https://doc.rust-lang.org/stable/src/core/str/converts.rs.html
- 4: https://github.com/rust-lang/rust/blob/0f145634/library/core/src/str/validations.rs
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
terms = {
'w_str_get_value_opt': False,
'w_str_get_wtf8': False,
'w_str_get_value': False,
}
for p in sorted(Path('pyre/pyre-interpreter/src').rglob('*.rs')):
text = p.read_text(errors='ignore')
for term in terms:
if term in text:
terms[term] = True
lines = text.splitlines()
for i, line in enumerate(lines, 1):
if term in line:
lo, hi = max(1, i - 15), min(len(lines), i + 22)
print(f'## {p}:{i}')
for n in range(lo, hi + 1):
print(f'{n:5}\t{lines[n-1]}')
break
break
print('terms_found', terms)
# Probe Rust's public UTF-8 validation rule by running only the std/libcore parser behavior,
# not repository code. This is a read-only behavioral check.
import subprocess
print('\n--- rust str::from_utf8 surrogate probe ---')
src = r'''
fn main() {
for (bytes, label) in [
(vec![0xC3, 0xA9, b'.'], "e acute dot"),
(vec![b'u', 0xED, 0xA0, 0x80, b'.'], "u plus lone surrogate dot"),
(vec![0xC3, 0x82, 0xED, 0xA0, 0x80], "valid char plus lone surrogate"),
] {
match std::str::from_utf8(&bytes) {
Ok(s) => println!("ok {} = {:?}", label, s),
Err(e) => println!("err {} {}", label, e),
}
}
}
'''
subprocess.check_call(["rustc", "-o", "/tmp/utf8_surrogate_probe", "-"] , input=src.encode())
subprocess.check_call(["/tmp/utf8_surrogate_probe"])
PYRepository: youknowone/pyre
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat > /tmp/check_surrogate_utf8.rs <<'RS'
use std::str;
fn main() {
for (bytes, label) in [
(vec![0xC3, 0xA9, b'.'], "e acute dot"),
(vec![b'u', 0xED, 0xA0, 0x80, b'.'], "u plus lone surrogate dot"),
(vec![0xC3, 0x82, 0xED, 0xA0, 0x80], "valid char plus lone surrogate"),
] {
match str::from_utf8(&bytes) {
Ok(s) => println!("ok {} = {:?}", label, s),
Err(e) => println!("err {} {}", label, e),
}
}
}
RS
rustc /tmp/check_surrogate_utf8.rs -o /tmp/check_surrogate_utf8
/tmp/check_surrogate_utf8Repository: youknowone/pyre
Length of output: 400
🌐 Web query:
Rust core str from_utf8 docs excludes surrogate code points U+D800 U+DFFF
💡 Result:
Rust's str::from_utf8 function correctly excludes surrogate code points (U+D800 through U+DFFF) because it adheres to the UTF-8 standard defined in RFC 3629 [1][2]. In the Unicode standard, surrogate code points are not Unicode scalar values; they are reserved exclusively for UTF-16 encoding [3][4]. Because str::from_utf8 is designed to validate that a byte slice contains well-formed UTF-8, it treats any byte sequence that would map to these surrogate code points as an invalid byte sequence [5][3]. This behavior is consistent with the definition of UTF-8, which prohibits the encoding of code points in the U+D800–U+DFFF range [1][6]. Consequently, if str::from_utf8 encounters bytes corresponding to these values, it returns a Result::Err containing a Utf8Error, as they do not constitute valid UTF-8 [5][7][8].
Citations:
- 1: https://datatracker.ietf.org/doc/html/rfc3629/
- 2: https://www.rfc-editor.org/info/rfc3629/
- 3: Problem with String::from_utf8 rust-lang/rust#54845
- 4: https://learn.microsoft.com/en-us/windows/win32/intl/surrogates-and-supplementary-characters
- 5: https://doc.rust-lang.org/stable/std/str/fn.from_utf8.html
- 6: https://en.wikipedia.org/wiki/UTF8
- 7: https://doc.rust-lang.org/stable/std/str/struct.Utf8Error.html
- 8: https://doc.rust-lang.org/src/core/str/error.rs.html
Read the method name as WTF-8 before formatting the bound-method repr.
w_str_get_value_opt returns None for strings with lone surrogates because Rust UTF-8 validation rejects surrogate code points. Use w_str_get_wtf8(...).to_wtf8_buf() here so a surrogate-bearing __qualname__/__name__ keeps the name in the output.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-interpreter/src/function.rs` around lines 2817 - 2821, Update the
bound-method representation code around the method-name lookup to read the name
with w_str_get_wtf8(...).to_wtf8_buf() instead of w_str_get_value_opt,
preserving surrogate-bearing __qualname__ or __name__ values when formatting the
output with wtf8_format!.
41bac54 routed every name through `w_dict_lookup_checked`, so a valid-UTF-8 attribute name wrapped a throwaway `W_UnicodeObject` per lookup on the hot mapdict read path (`mapdict.rs:3143`). `w_dict_getitem_str_object_strategy` exists to avoid exactly that (dictmultiobject.rs:3387-3389 "A borrowed `&str` probe avoids the per-lookup throwaway `W_UnicodeObject` (`getitem_str` parity)"). The flag-swallowing that made the pre-41bac542c18 post-hoc drain inert is in the object-lookup leaf -- `w_dict_lookup_object_strategy` (:2668) is `..._checked(..).unwrap_or(None)` -- not in `w_dict_getitem_str`, whose leaf `dict_entries_get_str` (:451) clears the flag before the probe and leaves whatever the probe sets. So the `&str` arm can use `w_dict_getitem_str_checked` and lose no error; a raising comparison is reachable only from a bucket holding a non-string key, i.e. only under the object strategy, whose `getitem_str` (:6219) is the borrowed probe. The devolved-terminator reproducer from b62b1be still raises `ValueError: boom` where it printed `CLASSVALUE` before that commit. Assisted-by: Claude
`exception_suggestion` read the failing name with `w_str_get_value`, which
panics on a lone surrogate, and `dict_string_keys` did the same for every
candidate. An uncaught AttributeError whose attribute name carried an
unpaired surrogate therefore killed the interpreter while rendering the
traceback:
S = "z\udcffz"
class C: pass
getattr(C(), S)
thread '<unnamed>' panicked at pyre/pyre-object/src/unicodeobject.rs:579:
w_str_get_value: backing Wtf8Buf is not valid UTF-8 (lone surrogate)
python3.14 prints `AttributeError: 'C' object has no attribute 'z\udcffz'`
with no suggestion suffix. Take `w_str_get_value_opt` at the three name reads
and answer `None` when there is no `&str` view -- `suggestion_distance` is
computed over `char`s, so such a name has nothing to compare against -- and
skip non-UTF-8 candidates instead of pushing them.
The `__module__` read in `exc_object_class_name` is left alone: it is not on
this path and rendering it needs the WTF-8 spelling, not a bail.
Assisted-by: Claude
…e getattr path `object_getattribute_surrogate` probed the module dict and the instance dict with `w_dict_lookup`, whose object-strategy leaf is `..._checked(..).unwrap_or(None)`. A stored non-string key whose hash collides with the name can run a user `__eq__`; the raising comparison was swallowed and the attribute read back as absent. b62b1be closed that for names with a `&str` view; the lone-surrogate arm kept the swallowing spelling. S = "z\udcffz" class R: def __hash__(self): return hash(S) def __eq__(self, o): raise ValueError("boom") o = C(); o.__dict__[R()] = 1 getattr(o, S) AttributeError before ValueError: boom after, matching python3.14 Take `finditem` -- `space.finditem`, the same helper `finditem_str` and the devolved terminator already use -- so the pending key error becomes the raised exception. Both the devolved and the small instance-dict shapes were affected. Add `pyre/extra_tests/parity_tests/mapdict_devolved_raising_eq.py`, which covers the devolved probe for both name kinds, the non-raising colliding-key control, and the plain lone-surrogate attribute round trip. b62b1be and 41bac54 landed without a fixture; this is the one that would have caught both of the above. check.py: dynasm 391/391, cranelift 391/391, wasm 387/387. `cargo test --all --no-default-features --features dynasm`: 0 failed. parity_tests: all pass, new file cpython=OK dynasm=OK cranelift=OK. Assisted-by: Claude
optimize_getfield_gc answered GETFIELD_GC on a virtual only when the trace had
already stored that field; an unset field fell through to PassOn and the load
was emitted, surviving to the arg-forcing pass, which materialized the very
virtual it read. virtualize.py:184-193 substitutes
`optimizer.new_const(fielddescr)` when `opinfo.getfield` returns None, with
optimizer.py:528-534 choosing CONST_NULL / CONST_ZERO_FLOAT / CONST_0 by field
kind. Port that fallback.
typeptr, w_class and the GETFIELD_RAW_* opcodes stay out of it: the first two
are header fields the same function already resolves from class identity, and
upstream defines this handler for GETFIELD_GC_{I,R,F} only.
The fold does not add an assumption the rest of the optimizer lacks --
virtualstate.py:171-174 tolerates a None fieldstate and info.py:216-226
`_force_elements` emits no SETFIELD for a None field, so upstream already
depends on the allocation being zeroed.
pytraceback.rs reads an exception's traceback slot before writing it, so every
raise emitted that load and dragged the exception, its args list and the
traceback node out of virtual state. Measured on
pyre/bench/synth/type_immutable_reject.py (MAJIT_LOG=1, same tree, rebuilt both
ways, `git checkout HEAD -- virtualize.rs` for the control and restored by
sha256):
compiled loop 108 -> 128 ops ==> 108 -> 42 ops
forced virtuals 12 ==> 0
NewWithVtable/NewArrayClear 16 ==> 0
CallMallocNursery 9 ==> 0
SetfieldGc 66 ==> 4
CallR 4 ==> 0
Both print 400000. check.py exec time on the exception family, dynasm /
cranelift / wasm, from the two full runs:
type_immutable_reject 0.08/0.08/0.11 -> 0.05/0.06/0.07
exception_value_op_caught 0.11/0.13/0.14 -> 0.06/0.06/0.07
exception_escape_hot_callee_tb_node_once 0.34/0.42/0.47 -> 0.25/0.29/0.34
exception_const_operand_resume 0.12/0.13/0.15 -> 0.09/0.10/0.12
Single runs of the same harness on the same machine, not min-of-rounds; the
load-independent signal is the allocation count above.
No .jitstats baseline moved: check.py dynasm 391/391, cranelift 391/391,
wasm 387/387. `cargo test --all --no-default-features --features dynasm`:
0 failed.
Assisted-by: Claude
`optimize_getfield_gc` addressed a virtual's field list with `field_descr.index_in_parent()` and never checked that the slot it landed on describes the field being read. `optimize_setfield_gc` has checked the same pairing since #1072, but only on the write side, so a descr spelling that appears solely on reads never reached it. `PYFRAME_VABLE_TOKEN_FIELD_DESCR` is such a spelling: it describes `PyFrame.vable_token` at offset 80 with a placeholder `index_in_parent: 0`, because the positional census that assigns the indices does not list the field -- upstream carries it as `rvirtualizable.py:29`'s appended `('vable_token', llmemory.GCREF)` and pyre registers it only as an extra GC edge. Slot 0 of that layout is `PyFrame.locals_cells_stack_w`, so the read returned the locals array pointer as the frame's token. Add `field_slot_identifies`, the release-live half of the existing `field_slot_disagreement` (both now share `slot_holds_field`). A read whose slot does not hold the field takes the `virtualize.py:188` zeroed-allocation fold instead of the slot's value; the write side keeps its panic. Logged as `[jit][getfield-slot-unlisted]` under `MAJIT_LOG=1`. Also port `info.py:212-213`: upstream's `getfield` opens with the same `init_fields(fielddescr.get_parent_descr(), fielddescr.get_index())` that `setfield` does, which is what upgrades `vinfo.descr` to a more precise subclass descr. pyre's read side omitted the call. A release build with `-C debug-assertions=on` over the check.py corpus reported 66 hits across 33 benches, all of them this one descr, all from `compile_loop_body`. `MAJIT_LOG=1` confirmed the aliased slot was populated. Assisted-by: Claude
…outcome The walker's loop-header close read its `CompileOutcome` as `Compiled` / not-`Compiled` and recorded every non-`Compiled` result as a declined close, which is latched in `TraceCtx::declined_cross_loop_closes` and never retried for that header. `MetaInterp::classify_compile_outcome` keeps three states apart there, and the sibling close in `jitdriver.rs:2947` already goes through it. Route this close through it too: `RetraceNeeded` no longer latches, since the attempt armed `partial_trace` and the next visit of the header takes the `has_partial` arm regardless. Track whether an attempt was made: the give-up arm that returns `Cancelled` without calling `compile_trace` costs no optimizer pass and is excluded from the latch. `classify_compile_outcome` becomes `pub` and `BridgeCompileResult` is re-exported from `majit_metainterp`. Assisted-by: Claude
Two sites turned an unpaired surrogate into U+FFFD.
`error.rs` `write_plain_exception_object` and `write_exception_group` wrote
`render_rooted_exc_object_wtf8`'s buffer straight into their byte buffers.
`sys.stderr` carries `errors='backslashreplace'`, so the surrogate owes the
six-character `\udcXX` escape on the way out; the raw WTF-8 bytes behind it
are not valid UTF-8. `render_exc_object` already spent that encode. Extract
it as `display::wtf8_display_string` and add
`error.rs::render_rooted_exc_object_display` for the two writers.
`builtins.rs` `exception_group_str` built its result as
`format!("{} ({count} sub-exception{suffix})", message.to_string_lossy())`.
`app_group.py:88-90` interpolates `self.message` with no encode, so
`str(group)` disagreed with `group.message` -- a loss visible from Python,
not only on stderr. Build the result as a `Wtf8Buf` instead.
Add `surrogate_traceback_render.py`. The parity harness checks only exit 0
and a final `OK`, so the two stderr-shape checks self-spawn through
`subprocess.run(capture_output=True)` and assert on bytes; the other two
assert the exception values keep the code point.
Assisted-by: Claude
…String
`display::py_str` and `py_repr` are their `_wtf8` twins plus a lossy UTF-8
encode. Five entry points took that spelling and immediately minted a Python
`str` from the result, so a lone surrogate in the value became U+FFFD:
- `object.__format__` (typedef.rs) and `builtin_value_format`
(type_methods.rs) — the empty-spec arm falls through to `str(self)`, which
is the `f"{obj}"` / `format(obj)` path for any instance. `builtin_value_format`
already had a WTF-8 arm for a `str` receiver; the two branches differed only
in the encode, so they collapse into one.
- `weakref.proxy.__str__` (interp__weakref.rs)
- `mappingproxy.__str__` (typedef.rs)
- the `BaseExceptionGroup` constructor's recorded sequence repr (builtins.rs)
Widening the last one made `exception_group_repr` panic in
`w_str_get_value`, which asserts UTF-8; that function assembled its whole
result through `String`, so it moves to WTF-8 with it.
`app_group.py:92-93` interpolates the two `!r` results verbatim.
`os.getcwd()` returned `String::from_utf8_lossy` / `Path::to_string_lossy`
of the directory bytes on both the sandbox and `host_env` arms.
`interp_posix.py:906-908` is `space.fsdecode(getcwdb(space))`; use the
`gateway` fsdecode helpers, which are what the sibling path entry points
already use.
Measured against CPython 3.14 with a `__str__` returning `"s\udcffz"`:
`f"{obj}"`, `format(obj)` and `str(weakref.proxy(obj))` answered `s�z`
before and `s\udcffz` after. `"%s" % obj` and `str(obj)` were already
lossless and are kept as controls in the fixture.
Add `surrogate_str_roundtrip.py`. A mappingproxy check was written and
dropped: a dict's repr escapes surrogates to ASCII, so it cannot fail.
Assisted-by: Claude
A Python `str` may hold an unpaired surrogate -- `surrogateescape` puts one there for every undecodable filesystem byte -- and `format!` renders a `Wtf8Buf` through `Display`, which substitutes U+FFFD. Every surface below rebuilt a Python-visible value that way. Message channel: - `PyError.message` becomes a `Wtf8Buf` and all 27 constructors take `impl Into<Wtf8Buf>`; `message_text()` keeps the display-side reading (backslashreplace) and `message_wtf8()` answers the value. `syntax_error_located` takes a `&Wtf8` filename and pins it verbatim. - `display::wtf8_format!` assembles a message from `str` / `String` / `Wtf8` / `Wtf8Buf` pieces through a `Wtf8Piece` trait. - `py_module!` gains a `Wtf8Buf` return arm (`w_str_from_wtf8`). - `display::py_repr` and `display::py_str`, the `String` wrappers over `py_repr_wtf8` / `py_str_wtf8`, are removed; their callers read the WTF-8 (typedef, _pickle, _csv, _ast, _sre, generic_alias, _contextvars, argument, call, opcode_ops, runtime_ops, type_methods and the rest). OS strings and the import machinery: - `gateway::fsencode_os_str` / `os_string_from_fs_bytes` are the two directions between a host `OsStr` and filesystem bytes. - posix `getcwd` / `getlogin` / `ttyname` / sandbox `getenv` / `strerror`, `create_environ`, `win_nt::arg_path` / `wrap_path` and `_path_splitroot` decode with fsdecode instead of a lossy UTF-8 pass; `split_root` scans the `Wtf8` by code point. - `load_source_module` spells the path once as bytes, so `__file__`, `co_filename` (through `PyCode.filename_bytes`) and the package `__path__` keep the name; `sys.path[0]`, the shadowing hint and `create_sys_path_list` follow. - `decode_source_bytes` takes a `&Wtf8` filename, and a declared codec that yields a surrogate now raises `UnicodeEncodeError` (`typedef::utf8_strict_w`) rather than rewriting the source. - `host_seam::getenv` (non-unix) and `launch_env` keep environment values in the filesystem-bytes spelling. Rendered reports: - A report is assembled as WTF-8 throughout and the sink decides the spelling: `sys.stderr.write` takes the text and its own `errors='backslashreplace'` applies, while `error::emit_report_to_host_stderr` spends that encode for a raw fd. `sys.excepthook` into a `StringIO` now reads the surrogate itself. - `BaseException.__str__`, the SyntaxError writer's filename and msg, `__notes__`, the thread excepthook and `SystemExit`'s printed code follow the same split. Adds `surrogate_name_messages.py`, covering `__qualname__` in `repr(f)` and the binder's three TypeErrors, the ContextVar / Token reprs and the already-used RuntimeError, `sys.excepthook`, and `BaseException.__str__`. Assisted-by: Claude
`arg_path` returns an `OsString`; `encode_utf16` is inherent to `str`, so `_add_dll_directory` did not compile for windows-msvc (E0599). `encode_wide` re-emits the code units `arg_path` decoded the path into. Assisted-by: Claude
A frame's `co_filename` is written as the filesystem bytes it was read as, so a path byte with no UTF-8 spelling leaves the report a mix of those bytes and the WTF-8 around them. Reading that mix back through `String::from_utf8_lossy` substituted U+FFFD for the byte that carries the name; pass the buffer through instead. The valid-WTF-8 arm still spends the backslashreplace encode. Assisted-by: Claude
`__pypy__.write_unraisable`'s first argument and the value `_module_repr` returns were read with `w_str_get_wtf8`, which casts without a tag test, so a non-str was dereferenced as a `W_UnicodeObject` instead of raising TypeError. `text_wtf8_w` runs `expect_str` first and performs the same WTF-8 read. Assisted-by: Claude
Four keyword-binding TypeErrors and `fileio.__repr__` interpolated a `Wtf8Buf` through `format!`, which renders it with `Display` and substitutes U+FFFD, so the same call family spelled a `__qualname__` two different ways depending on which arm raised. The parity fixture gains the two binder arms a defaulted parameter reaches. Assisted-by: Claude
`read_prompt` took `py_str_display`, which answers `"<unprintable>"` rather than failing, so a raising `__str__` became the prompt itself instead of leaving `load_prompt`'s default to stand in. `py_str_display_result` renders the same text and reports the failure. Assisted-by: Claude
The note names a dict key, which may hold a lone surrogate. It was built from `display::py_repr`, which this branch removed along with the `to_string_lossy` behind it, so the call site did not compile; `add_json_note` now takes the text as WTF-8 and the key is read with `py_repr_wtf8`. Assisted-by: Claude
`fileio_method_repr`'s `wtf8_format!` call and the multiple-values `push_str` in `call_with_kwargs_in_ctx` were left in a shape `cargo fmt --check` rejects. Assisted-by: Claude
1 participant
§1–§4 are the four fixes this PR opened with: one missing upstream optimizer pass, one parity
regression of my own from #1083, and the two defects the regression fixture turned up on the way.
§5–§8 are the follow-ups those sections filed, plus the surrogate audit they opened, landed on the
same branch. 15 commits, 68 files, +2381/−1022.
1.
optimize_getfield_gcnever folded a virtual's unset fieldvirtualize.py:184-193substitutesoptimizer.new_const(fielddescr)whenopinfo.getfieldreturns None — the field was never stored, so it reads the zeroed allocation. pyre had no such arm: the read fell through toPassOn, the load was emitted, and it survived to the arg-forcing pass, which materialized the very virtual it read.pytraceback.rsreads an exception's traceback slot before writing it, so everyraisehit this and dragged the exception, its args list and the traceback node out of virtual state.Measured on
pyre/bench/synth/type_immutable_reject.pywithMAJIT_LOG=1, same tree rebuilt both ways (git checkout HEAD -- virtualize.rsfor the control, restored by sha256):NewWithVtable+NewArrayClearCallMallocNurserySetfieldGcCallRBoth print
400000.typeptr,w_classand theGETFIELD_RAW_*opcodes stay out of it: the first two are header fields the same function already resolves from class identity and are never zero on a live object, and upstream defines this handler forGETFIELD_GC_{I,R,F}only.The fold adds no assumption the rest of the optimizer lacks —
virtualstate.py:171-174tolerates aNonefieldstate andinfo.py:216-226 _force_elementsemits no SETFIELD for aNonefield, so upstream already depends on the allocation being zeroed.2. Revert my own parity regression from #1083
41bac542c18routed every name throughw_dict_lookup_checked, wrapping a throwawayW_UnicodeObjectper lookup on the hot mapdict read path (mapdict.rs:3143) — which is exactly whatw_dict_getitem_str_object_strategyexists to avoid, as that file's own comment says.The flag-swallowing that commit diagnosed lives in the object leaf (
w_dict_lookup_object_strategyis..._checked(..).unwrap_or(None)), not in the&strleaf (dict_entries_get_strclears the flag pre-probe and leaves whatever the probe sets). So the&strarm keepsw_dict_getitem_str_checkedand loses no error: a raising comparison is reachable only from a bucket holding a non-string key, i.e. only under the object strategy, whosegetitem_stris the borrowed probe.3. Interpreter panic rendering a lone-surrogate attribute name
The suggestion machinery read the failing name — and every candidate — with
w_str_get_value. python3.14 printsAttributeError: 'C' object has no attribute 'z\udcffz'with no suggestion suffix;suggestion_distanceis computed overchars, so such a name has nothing to compare against. Takew_str_get_value_optand answerNone.4. The surrogate
getattrpath still swallowed a raising__eq__object_getattribute_surrogateprobed both the module dict and the instance dict with the uncheckedw_dict_lookup.b62b1be9dbaclosed this for names with a&strview; the lone-surrogate arm kept the swallowing spelling.Fixture
b62b1be9dbaand41bac542c18both landed without one.pyre/extra_tests/parity_tests/mapdict_devolved_raising_eq.pycovers the devolved probe for both name kinds, thegetattr/__getattribute__/__dict__[...]read paths, a non-raisingNotImplementedcolliding-key control, instance-attr-wins-after-devolve, a builtin subclass, and the plain lone-surrogate round trip. It catches all four.Verification of §1–§4 (as measured when they were written; the whole-branch run is at the bottom)
.jitstatsbaseline moved.cargo test --all --no-default-features --features dynasm: 0 failed.cpython=OK dynasm=OK cranelift=OK.Exec times on the exception family moved with the allocation counts (dynasm/cranelift/wasm, single runs of the same harness, not min-of-rounds):
Follow-ups, now landed on the same branch
The two follow-ups the sections above filed are fixed here, plus the surrogate audit they opened.
5.
optimize_getfield_gcmatched a virtual's slot by index aloneThe fold in §1 turns a conservative
PassOninto a substitution, so the slot it reads has to be the slot the descr names.resolve_virtual_slotmatcheddescr.index_in_parent()against thePtrInfo's field list without ever comparing the descr itself, so two descrs sharing an index resolved to the same slot. The write side already ranfield_slot_disagreement; the read side now resolves against thePtrInfodescr and declines the fold when they disagree, instead of folding to the wrong slot.PYFRAME_VABLE_TOKEN_FIELD_DESCRcarries a placeholderindex_in_parentof 0, which aliases slot 0 of everyPyFramevirtual — that placeholder is filed separately and is untouched here.6. The walker's cross-loop close cached
Cancelledas permanentclose_cross_loopstored its result in the declined-close cache without going throughclassify_compile_outcome, so aCompileOutcome::Cancelled— a this-attempt refusal — was recorded exactly like a permanent decline and the pair was never retried. Routed throughclassify_compile_outcome, which is the function that already knows which outcomes are terminal.7.
stderrwrote a lone surrogate as U+FFFDThe follow-up above.
PyError.messagewas aString, so every constructor that named a user string ran it through a lossy UTF-8 conversion and substituted U+FFFD before the message ever reached a sink.messageis now aWtf8Bufand all 27 constructors takeimpl Into<Wtf8Buf>.A rendered report is assembled as raw WTF-8 end to end, and the sink decides the spelling:
sys.stderr.writegets the text and the stream's ownerrors='backslashreplace'applies, which is what_PyErr_Displaydoes. pyre'ssys.stderr.errorsisbackslashreplace, same as python3.14.emit_report_to_host_stderrspends the encode itself.Terminal bytes are byte-identical to python3.14 under
od -c.8. Every other surface that quotes a name
repr(), the argument binder'sTypeErrors,__qualname__, the import machinery's__file__/co_filename/__path__/sys.pathseeds,_ast/_sre/_contextvarsreprs, and the posix/socket OS-string decoders all rebuilt astrthroughStringorto_string_lossy. They now carry WTF-8:w_str_get_valuepanics on a lone surrogate andw_str_get_value_optreturnsNone, so anelsearm raising "not a unicode object" was wrong for a surrogate-bearing string. Readers moved tow_str_get_wtf8.format!("{}", wtf8buf)goes throughDisplay for Wtf8Buf, which is lossy. Awtf8_format!macro assembles the same text without the round trip.fsencode_os_str/os_string_from_fs_byteswithFS_ERRORS(surrogatepasson Windows,surrogateescapeelsewhere), notto_string_lossy.os_string_from_fs_bytesis written with three#[cfg]arms —windows,unix, and a fallback — becausenot(windows)includeswasm32-unknown-unknown, wherestd::os::unixdoes not exist.interp_posix.rs:1336-1343is the in-repo precedent.New fixture
pyre/extra_tests/parity_tests/surrogate_name_messages.py, oracle-verified against python3.14, covers__qualname__inrepr()and in three binderTypeErrors, generator__qualname__, the threecontextvarsreprs plus itsLookupErrorand reset-RuntimeError,sys.excepthookinto aStringIO, andBaseException.__str__. It reads the surrogate out of a__repr__rather than a plainstr, becausestr.__repr__backslash-escapes a surrogate and would hide the difference.Verification (whole branch, rebased onto
d936eb4be42)main's own; see below. No.jitstatsbaseline moved — the branch touches none.cargo test --all --no-default-features --features dynasm: 0 failed, across 102 test binaries.pyre/extra_tests/parity_tests/run.py: 0 failed. CI runs this as its own step after check.py, so a green check.py alone does not cover it.cargo build -p pyre-wasm --target wasm32-unknown-unknown --features wasm-host): clean.cargo check -p pyre-interpreter --target x86_64-pc-windows-msvc --features dynasm: clean.The one failing gate row is inherited from
mainsynth/pypy_type_surfacereportsbridges_compiled 5 -> 102, guard_failures 1011 -> 20497on all three backends. It is not this branch's movement, and it has not been re-recorded. Four independent controls:.rsfiles toorigin/maincontent, re-extracting LLBC and re-running the bench reproduces102 / 20497 / loops_compiled=11byte-identically..jitstatsfile toorigin/mainreproduces it as well, so it is not an artifact of baseline content.main's own macOS check.py job atd936eb4be42— this branch's base — carries the identical rows and the identical totals:dynasm 1 failed, 404 passed,cranelift 1 failed, 404 passed.dad2a722907(stdlib: stabilize collections JIT and heapq fallback #999); no commit here touches either.The branch's FAIL set equals
main's FAIL set exactly and adds no row of its own.A blind spot named in the first revision, now closed
The
#[cfg(windows)]arms are compile-verified.cargo check --target x86_64-pc-windows-msvcpreviously died instacker's build script for want ofwindows.h; a one-typedef stub header, a shell shim translating cc-rs's-out:toar rcS, and a cargo build-script override forlibffi-sys(which declareslinks = "ffi") make it check clean in about a minute, with no repo changes. It caught a real defect here:OsStringhas noencode_utf16, so_add_dll_directoryencodes withencode_wide— going back through astrhas no spelling for a lone surrogate and would address a different directory.macOS APFS still rejects an undecodable filename with
EILSEQ, so the unix OS-string findings cannot be reproduced locally against a real file; the fixtures drive them through__qualname__and friends instead.Summary by CodeRabbit
New Features
Bug Fixes
Tests