str: len(str) JIT specializer, 3.14 UnboundLocalError phrasing, utf8_mode locale fix#566
str: len(str) JIT specializer, 3.14 UnboundLocalError phrasing, utf8_mode locale fix#566youknowone wants to merge 7 commits into
Conversation
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 Run ID: 📒 Files selected for processing (9)
WalkthroughThe change standardizes unbound-local error messages across interpreter and JIT paths, extends ChangesRuntime error message updates
Builtin length specialization
Locale detection behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 2b52869). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/pyrex/src/lib.rs`:
- Around line 121-124: Update the locale reader closure used to initialize
locale selection so it calls std::env::var_os instead of std::env::var,
preserving non-Unicode environment values while skipping only empty entries.
Keep the existing LC_ALL, LC_CTYPE, then LANG fallback order unchanged.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: db495870-8b38-4878-b8a2-b38b8e906c90
📒 Files selected for processing (6)
pyre/pyre-interpreter/src/eval.rspyre/pyre-interpreter/src/pyframe.rspyre/pyre-jit-trace/src/jitcode_dispatch.rspyre/pyre-jit-trace/src/trace_opcode.rspyre/pyre-jit/src/call_jit.rspyre/pyrex/src/lib.rs
| let read = |name: &str| std::env::var(name).ok().filter(|v| !v.is_empty()); | ||
| let locale = read("LC_ALL") | ||
| .or_else(|| read("LC_CTYPE")) | ||
| .or_else(|| read("LANG")); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant function and nearby tests.
file="pyre/pyrex/src/lib.rs"
sed -n '100,170p' "$file"
echo
echo "---- search ----"
rg -n "locale_implies_utf8_mode|UTF-8 mode|LC_ALL|LC_CTYPE|LANG|var_os|NotUnicode" pyre/pyrex -SRepository: youknowone/pyre
Length of output: 3472
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact lines around the changed code.
file="pyre/pyrex/src/lib.rs"
nl -ba "$file" | sed -n '110,140p'
echo
echo "---- tests in the same crate ----"
fd -a -t f . pyre/pyrex | sed 's#^\./##'Repository: youknowone/pyre
Length of output: 193
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate any locale-related tests or docs that constrain the intended behavior.
rg -n "locale.*utf8|utf8.*locale|LC_ALL|LC_CTYPE|LANG|NotUnicode|var_os" pyre -SRepository: youknowone/pyre
Length of output: 13495
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print the function body with line numbers and any nearby tests.
nl -ba pyre/pyrex/src/lib.rs | sed -n '110,145p'
echo
echo "---- test mentions ----"
rg -n "locale_implies_utf8_mode|LC_ALL|LC_CTYPE|LANG|UTF-8 mode" pyre/pyrex -SRepository: youknowone/pyre
Length of output: 193
Preserve non-Unicode locale values in pyre/pyrex/src/lib.rs:121-124.
std::env::var(name).ok() turns a present non-UTF-8 LC_ALL/LC_CTYPE/LANG into None, so the fallback chain can incorrectly enable UTF-8 mode. Read the raw value with var_os and skip only empty entries.
🤖 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/pyrex/src/lib.rs` around lines 121 - 124, Update the locale reader
closure used to initialize locale selection so it calls std::env::var_os instead
of std::env::var, preserving non-Unicode environment values while skipping only
empty entries. Keep the existing LC_ALL, LC_CTYPE, then LANG fallback order
unchanged.
…d locals The unbound-local and unbound-free-variable errors used the pre-3.11 text "local variable 'x' referenced before assignment" / "free variable 'x' referenced before assignment in enclosing scope". Python 3.14 emits "cannot access local variable 'x' where it is not associated with a value" and "cannot access free variable 'x' where it is not associated with a value in enclosing scope". Update the runtime message text at the seven local-variable sites (eval.rs load_local_checked_value / delete_fast, call_jit.rs, trace_opcode.rs ×3, pyframe.rs deref_unbound_error) and the adjacent free-variable site in deref_unbound_error, plus the deref_unbound_error doc comment. The .args / isinstance / base wiring already matched. The flowspace unbound-local diagnostics in majit-translate are left as-is: they mirror rpython/flowspace, not the runtime error. Assisted-by: Claude
`try_walker_specialize_builtin_len` lowered `len(x)` on an exact canonical list to an inline strategy-guarded length read; extend it to an exact canonical `str`. A str carries no storage strategy (a single Wtf8Buf), so the str path skips the strategy guard and reads the codepoint-count field directly via `str_len_descr()` (`W_UnicodeObject.len`, immutable), guarded by `guard_class STR` + the exact `w_class` guard (the subclass `__len__` hazard, same as list). Removes the `len(s)` residual call from str hot loops (e.g. bench/synth/str_fstring.py fstring_*/string_ops); a str subclass overriding `__len__` side-exits to the generic residual via the exact w_class guard. Assisted-by: Claude
locale_implies_utf8_mode selected the first present LC_ALL/LC_CTYPE/LANG, so a present-but-empty variable (e.g. `LC_ALL=`) matched the empty arm and coerced utf8_mode to 1 without consulting the next variable. setlocale treats an empty value as unset, so `LC_ALL= LC_CTYPE=en_US.UTF-8` resolves to en_US.UTF-8 (utf8_mode 0). Filter empty values before selecting and drop the empty match arm; a fully unset chain still resolves to C (utf8_mode 1). Assisted-by: Claude
… token A JC_DONT_TRACE_HERE cell whose once-seen procedure token had since been invalidated hit the unconditional early return before the dead-token cleanup gate at four tick-state sites (counter_tick_checked, maybe_compile, maybe_compile_with_key, should_trace_function_entry), so cleanup_chain was skipped and the stale chain entry lingered while the counter re-armed slowly. Gate each early return on `dead_token = has_seen_a_procedure_token && get_procedure_token().is_none()`: a live TEMPORARY token still declines, but an invalidated token falls through to the shared cleanup_chain, mirroring warmstate.py:483-491 where a procedure_token that is None reaches cleanup_chain(hash) for the (DONT_TRACE_HERE && has_seen) case. Assisted-by: Claude
A custom decode error handler registered via codecs.register_error may reassign exc.object; the decoder must reread it after the handler returns and resume decoding from the new bytes at the returned position, folding newpos against the new object's length (interp_codecs.py:94-104). Previously the loops always resumed into the original byte slice, so a replacing handler produced wrong output or an out-of-bounds newpos error. call_registered_decode_error_handler now rereads exc.object, rejects a non-bytes object with TypeError, folds newpos against its length, and returns Some(new_bytes) when it differs from the input. The utf-8, utf-16/32, raw-unicode-escape and generic (ascii/latin-1) decode loops carry a Cow<[u8]> working buffer and rebind it to the replacement, staying zero-copy when the object is left unchanged. charmap and unicode-escape decode do not route custom handlers through this helper and are left unchanged. Assisted-by: Claude
…at saw a token" This reverts commit 8df3ff1.
![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: cff2998595
ℹ️ 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 resume = if new_bytes == data { | ||
| None | ||
| } else { | ||
| Some(new_bytes.to_vec()) |
There was a problem hiding this comment.
When a custom decode error handler assigns exc.object to replacement bytes and that replacement produces another decode error, this stores only a copied Vec<u8>. The next UnicodeDecodeError is then built from a fresh bytes object, so handlers that branch on exc.object is replacement or on a bytes subclass see different state from CPython and can produce different output. Carry the replacement object along with the bytes when rebinding.
Useful? React with 👍 / 👎.
…d handler charmap_decode_impl and unicode_escape_decode_impl handled only the built-in strict/ignore/replace modes inline and raised on any other error mode, so a custom handler registered via codecs.register_error was never invoked and a handler-mutated exc.object was never reread. - unicode_escape_error routes strict through unicode_decode_error, xmlcharrefreplace/namereplace through decode_error_encode_only_handler, and every other mode through call_registered_decode_error_handler. - unicode_escape_decode_impl is rewritten over a Cow<[u8]> working buffer that rebinds when the handler replaces exc.object. Error positions follow str_decode_unicode_escape/hexescape: truncated vs illegal spans, the span running over the actual hex digits present, the backslash-anchored start, and pos_delta keeping the consumed count relative to the input length. - charmap_decode_impl walks by index over a Cow<[u8]> buffer, routes an <undefined> byte through strict/ignore/replace/backslashreplace/ encode-only/custom, rebinds on replacement, and reports the original input length as consumed. - decode_error_encode_only_handler is now pub(crate). Assisted-by: Claude
1 participant
Continues the
strbranch (prior work landed via #533) with three CPython 3.14 parity / JIT changes on top ofmain.Commits
cannot access local variable 'x' where it is not associated with a value; the free-var NameError analog) at the 7 local + 1 free runtime sites. The majit-translate flowspace diagnostics stay on the RPython phrasing intentionally.try_walker_specialize_builtin_lenloweredlen(x)on an exact canonical list to an inline strategy-guarded length read; extend it to canonicalstr, reading the codepoint field directly viastr_len_descr()(str has no storage strategy). Guardsguard_class STR+ exactw_class(the subclass__len__hazard). Removes thelen(s)residual call from str hot loops (~6.6x on an isolatedlen(str)loop; modest at whole-benchmark level).locale_implies_utf8_modematched a present-but-emptyLC_ALL=/LC_CTYPE=as the C locale and coerced utf8_mode to 1 without consulting the next variable;setlocaletreats empty as unset. Filter empty values soLC_ALL= LC_CTYPE=en_US.UTF-8resolves to utf8_mode 0.Verification
check.pydynasm synthetic suite: 176/177 (sole failure =list_insert_pop_index, a pre-existing cpython≠pypy oracle disagreement / BASEFAIL, not a regression).__len__deopt, bytes residual, concat-then-len; IR confirms thelenresidual is replaced by an inline getfield.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Performance
len()handling for exact lists and strings, enabling faster length retrieval in supported cases.