Repository navigation
objspace: pypy-parity sweep batch (attribute lookup, bool/hash/coerce/iter edge cases) - #645
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds numerous executable Python benchmark/regression scripts under pyre/bench/synth and pyre/bench, and modifies the Pyre interpreter's Rust core: exception construction and traceback rendering, attribute/metaclass lookup order, hashing/formatting/length validation, iterator/callable-iterator semantics, collection method arity/enforcement, SimpleNamespace implementation, and aiter/anext builtins. ChangesRuntime semantics and regression benchmarks
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant PythonCode
participant OpenBuiltin
participant PyError
participant OSErrorInstance
PythonCode->>OpenBuiltin: open(missing_path, "r")
OpenBuiltin->>PyError: os_error_syscall(errno, filename)
PyError->>OSErrorInstance: set .errno, .strerror, .filename, .args
PyError->>OSErrorInstance: retag w_class to FileNotFoundError
OSErrorInstance-->>PythonCode: raise FileNotFoundError
sequenceDiagram
participant PythonCode
participant AiterBuiltin
participant HandleCache
participant AppLevelPython
PythonCode->>AiterBuiltin: aiter(iterable)
AiterBuiltin->>HandleCache: handle(AITER)
HandleCache->>AppLevelPython: install/execute ASYNC_OP_SRC (first call only)
AppLevelPython-->>HandleCache: cached function handle
HandleCache-->>AiterBuiltin: aiter function
AiterBuiltin->>AppLevelPython: call_function_impl_result(aiter, iterable)
AppLevelPython-->>PythonCode: async iterator
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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: ba6c6db0ad
ℹ️ 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".
| if is_int(r) || is_bool(r) { | ||
| return Ok((w_int_get_value(r) as f64, 0.0)); |
There was a problem hiding this comment.
When __index__ returns a large Python int, pyre represents it as W_LongObject; space_index already accepts that as an integer, and complex() should then convert it to a float (or raise the normal float overflow). This new fallback only accepts the machine-int/bool layouts and then reports the W_LongObject as a non-int, so class X: __index__ = lambda self: 2**80; complex(X()) raises the wrong TypeError instead of producing the indexed value.
Useful? React with 👍 / 👎.
| if args.len() != 2 { | ||
| return Err(crate::PyError::type_error(format!( | ||
| "{}.{}() takes exactly one argument ({} given)", | ||
| crate::baseobjspace::object_functionstr_type_name(args[0]), |
There was a problem hiding this comment.
For unbound calls made through the class, such as set.add() or set.copy(), the builtin body can be invoked with args.len() == 0 on the slow path because the recorded arity is only a fast-path hint. This error formatter indexes args[0] while handling the wrong-arity case, so those calls panic instead of raising a Python TypeError; the no-argument helper added just below has the same receiver access pattern.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 5ca25e3). 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
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03dbfee502
ℹ️ 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".
| // identity (`default_identity_hash`) — distinct NaN objects hash | ||
| // apart, matching the default-object fall-through below. | ||
| if v.is_nan() { | ||
| return obj as i64; |
There was a problem hiding this comment.
For exact float NaNs this now hashes the current object address, but the same change makes exact floats with identical bit patterns eq_w-identical via floats_is_w; dict/set keys compare cached hashes before calling equality, so two bitwise-identical NaNs can land in different buckets even though container equality treats them as the same value. Because pyre has a moving GC, obj as i64 can also change for the same NaN between insertion and lookup; this needs a bit-pattern/immutable-unique-id hash instead of the raw address.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 677fd3a0a6
ℹ️ 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".
| return Err(crate::PyError::os_error_syscall( | ||
| e.raw_os_error().unwrap_or(2), | ||
| format!("{e}: '{path}'"), | ||
| pyre_object::w_str_new(&path), |
There was a problem hiding this comment.
When open() is called with a bytes path (or a path-like object returning bytes) and the read fails, this new structured OSError stores w_str_new(&path) as .filename. Python/PyPy keep the filename object in the path's filesystem type, so open(b'/missing') should expose a bytes e.filename; this instead returns a decoded/lossy str, which can break callers that reuse e.filename for byte-oriented filesystem handling.
Useful? React with 👍 / 👎.
| let base_hash = | ||
| base.and_then(|b| crate::baseobjspace::lookup_in_type(b, "__hash__")); | ||
| if Some(method) != base_hash { | ||
| return hash_call_normalize(method, obj); |
There was a problem hiding this comment.
For tuple/frozenset subclasses that override __hash__ with a descriptor such as staticmethod, classmethod, or a custom descriptor, this new override path calls the raw type-dict value with the receiver instead of binding it first. PyPy's hash_w uses descriptor binding before invoking __hash__, so class T(tuple): __hash__ = staticmethod(lambda: 5); hash(T()) should return 5; here the lambda gets an unexpected receiver argument (and a classmethod override is treated as not callable).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea2e28b044
ℹ️ 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_descr = unsafe { lookup_in_type_where(w_type, name) }; | ||
| if let Some(descr) = w_descr { | ||
| if unsafe { is_data_descr(descr) } { | ||
| if let Some(result) = unsafe { get(descr, obj, w_type)? } { |
There was a problem hiding this comment.
When a builtin-layout subclass has a descriptor whose __get__ raises AttributeError and the class also defines __getattr__, normal attribute access should fall through to __getattr__ just like the W_ObjectObject path above does. This new hasdict branch exits via ?, so class T(tuple): x = raising_descriptor; def __getattr__(...)... makes T().x propagate the descriptor's AttributeError instead of returning the fallback; the non-data descriptor call below has the same early-exit shape.
Useful? React with 👍 / 👎.
| let w_pkgpath = match crate::baseobjspace::getattr_str(module, "__file__") { | ||
| Ok(v) if unsafe { pyre_object::is_str(v) } => v, | ||
| _ => pyre_object::w_str_new("unknown location"), | ||
| }; |
There was a problem hiding this comment.
For modules without a __file__, such as sys, from sys import missing should format the message with (unknown location) but leave ImportError.path unset. This default path object is passed into import_error_name_path, so callers now see e.path == "unknown location" instead of None, which differs from the structured exception fields the new helper is meant to preserve.
Useful? React with 👍 / 👎.
| let w_name = w_str_new(name); | ||
| return crate::call::call_function_impl_result(da, &[obj, w_name]) | ||
| .map(|_| w_none()); |
There was a problem hiding this comment.
When a metaclass supplies __delattr__ through staticmethod, classmethod, or another descriptor, special-method dispatch should bind that descriptor before invocation. Calling the raw MRO entry with [obj, name] passes an extra class receiver to a staticmethod (or fails to bind a classmethod), so del C.x breaks for metaclasses that CPython/PyPy accept; this should use the same descriptor-binding path as other special-method calls.
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
🤖 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/bench/synth/attr_instance_shadows_class.py`:
- Around line 19-41: Update the TD benchmark setup in main so each td instance
receives a conflicting "d" entry directly in td.__dict__ before the td.d
assertion. Keep the existing DataD descriptor and assertion unchanged, ensuring
the check exercises data-descriptor precedence over the instance dictionary.
In `@pyre/bench/synth/bool_dunder_error_no_leak.py`:
- Line 25: Rename the unused loop variable i to _ in the loops in
pyre/bench/synth/bool_dunder_error_no_leak.py:25,
pyre/bench/synth/reversed_disabled.py:25,
pyre/bench/synth/callable_iterator_type.py:15,
pyre/bench/synth/int_from_bad_dunder.py:13,
pyre/bench/synth/complex_from_index.py:15,
pyre/bench/synth/enumerate_bignum_start.py:10, and
pyre/bench/synth/seq_repeat_overflow.py:13, leaving each loop body unchanged.
In `@pyre/bench/synth/seq_repeat_overflow.py`:
- Line 22: Update the identity check in the condition around t, b, s, and lst to
use the idiomatic `is not` form for the lst comparison, preserving the existing
logic and all other identity checks.
In `@pyre/bench/synth/set_method_arity.py`:
- Around line 5-7: Rename the callable parameter in kind to func or target, and
update its invocation within the function accordingly; preserve the existing
behavior.
In `@pyre/pyre-interpreter/src/error.rs`:
- Around line 626-697: Add GC rooting using the existing
push_roots()/pin_root(...) pattern before intermediate allocations: in
pyre/pyre-interpreter/src/error.rs:626-697, root/pin exc and w_filename in
os_error_syscall before args_list creation and setter calls; in
pyre/pyre-interpreter/src/error.rs:734-767, root/pin exc, w_name, and w_path in
import_error_name_path before w_msg allocation; and in
pyre/pyre-interpreter/src/importing.rs:2228-2241, root/pin w_pkgname in
import_from before getattr_str(module, "__file__").
In `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs`:
- Around line 455-462: Update the error branches in stat_impl and fstat to route
filesystem errors through the existing io_err helper instead of constructing
PyError::os_error_with_errno directly. Pass the relevant path to io_err so
os.stat() and os.fstat() preserve the filename attribute and errno-specific
error behavior used by the other syscall wrappers.
In `@pyre/pyre-interpreter/src/module/sys/vm.rs`:
- Around line 117-156: Update simple_namespace_type so the builtin type
constructor receives a split module and class name, storing “types” as the
module and “SimpleNamespace” as the type name. Ensure type.__name__ and
__qualname__ expose only “SimpleNamespace” while __module__ remains “types”.
🪄 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: 0ef960ed-e818-4e45-b1f6-bcc10b39c034
📒 Files selected for processing (31)
pyre/bench/oserror_errno_fields_regression.pypyre/bench/synth/attr_instance_shadows_class.pypyre/bench/synth/bool_dunder_error_no_leak.pypyre/bench/synth/bytes_split_whitespace_maxsplit.pypyre/bench/synth/callable_iterator_type.pypyre/bench/synth/complex_from_index.pypyre/bench/synth/enumerate_bignum_start.pypyre/bench/synth/hash_subclass_disabled.pypyre/bench/synth/import_from_name_path.pypyre/bench/synth/int_from_bad_dunder.pypyre/bench/synth/list_length_hint_validate.pypyre/bench/synth/list_nan_identity.pypyre/bench/synth/metaclass_getattribute_delattr.pypyre/bench/synth/reversed_disabled.pypyre/bench/synth/seq_repeat_overflow.pypyre/bench/synth/set_method_arity.pypyre/bench/synth/simple_namespace_type.pypyre/bench/synth/slots_class_var_conflict.pypyre/bench/synth/str_encode_text_codec.pypyre/check.pypyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/call.rspyre/pyre-interpreter/src/error.rspyre/pyre-interpreter/src/importing.rspyre/pyre-interpreter/src/module/posix/interp_posix.rspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-interpreter/src/objspace/descroperation.rspyre/pyre-interpreter/src/type_methods.rspyre/pyre-interpreter/src/typedef.rspyre/pyre-object/src/listobject.rs
| def main(): | ||
| b = Boom() | ||
| n = 0 | ||
| for i in range(N): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Rename unused loop control variable.
The loop control variable i is not used within the loop body. Consider renaming it to _ or _i to suppress the Ruff (B007) warning and clearly indicate that the value is intentionally ignored.
pyre/bench/synth/bool_dunder_error_no_leak.py#L25-L25: Renameito_.pyre/bench/synth/reversed_disabled.py#L25-L25: Renameito_.pyre/bench/synth/callable_iterator_type.py#L15-L15: Renameito_.pyre/bench/synth/int_from_bad_dunder.py#L13-L13: Renameito_.pyre/bench/synth/complex_from_index.py#L15-L15: Renameito_.pyre/bench/synth/enumerate_bignum_start.py#L10-L10: Renameito_.pyre/bench/synth/seq_repeat_overflow.py#L13-L13: Renameito_.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 25-25: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
📍 Affects 7 files
pyre/bench/synth/bool_dunder_error_no_leak.py#L25-L25(this comment)pyre/bench/synth/reversed_disabled.py#L25-L25pyre/bench/synth/callable_iterator_type.py#L15-L15pyre/bench/synth/int_from_bad_dunder.py#L13-L13pyre/bench/synth/complex_from_index.py#L15-L15pyre/bench/synth/enumerate_bignum_start.py#L10-L10pyre/bench/synth/seq_repeat_overflow.py#L13-L13
🤖 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/bench/synth/bool_dunder_error_no_leak.py` at line 25, Rename the unused
loop variable i to _ in the loops in
pyre/bench/synth/bool_dunder_error_no_leak.py:25,
pyre/bench/synth/reversed_disabled.py:25,
pyre/bench/synth/callable_iterator_type.py:15,
pyre/bench/synth/int_from_bad_dunder.py:13,
pyre/bench/synth/complex_from_index.py:15,
pyre/bench/synth/enumerate_bignum_start.py:10, and
pyre/bench/synth/seq_repeat_overflow.py:13, leaving each loop body unchanged.
Source: Linters/SAST tools
| def kind(callable, args): | ||
| try: | ||
| callable(*args) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Avoid shadowing the Python built-in callable.
The argument name callable shadows the built-in function. Please rename it to func or target to prevent potential confusion.
♻️ Proposed refactor
-def kind(callable, args):
+def kind(func, args):
try:
- callable(*args)
+ func(*args)📝 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.
| def kind(callable, args): | |
| try: | |
| callable(*args) | |
| def kind(func, args): | |
| try: | |
| func(*args) |
🧰 Tools
🪛 Ruff (0.15.21)
[error] 5-5: Function argument callable is shadowing a Python builtin
(A002)
🤖 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/bench/synth/set_method_arity.py` around lines 5 - 7, Rename the callable
parameter in kind to func or target, and update its invocation within the
function accordingly; preserve the existing behavior.
Source: Linters/SAST tools
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/92c0ac8f349a0a4cc90c1a6335d1656dda3c6449/pyre-interpreter/src/async_operation.rs#L65-L66
After the first aiter() or anext() call, the only persistent references to the lazily-created app-level functions are stored in this TLS OnceCell, but this slot is never exposed to the GC root walker (unlike reduce_protocol::walk_handle_roots, which the comment says this mirrors). When any later collection runs before the next async builtin call, the cached function and its private globals namespace can be swept or moved without updating HANDLES, so subsequent aiter/anext dispatch can dereference stale function/global pointers.
https://github.com/youknowone/pyre/blob/92c0ac8f349a0a4cc90c1a6335d1656dda3c6449/pyre-interpreter/src/error.rs#L653
When a failing syscall passes a non-null filename (for example open('/missing') or the posix helpers), w_filename is only a Rust local while w_exception_new, w_list_new, and the w_str_new calls below allocate before the filename is stored on the exception. If one of those allocations triggers GC, the filename object is not on the shadow stack and can be collected or moved, leaving .filename stale or corrupted; pin the filename across the exception construction before the first allocation here.
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyre/pyre-interpreter/src/module/sys/vm.rs (1)
42-53: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStale type name in
sys_namespace_init's error message.Now that
types.SimpleNamespacehas its own dedicated init (simple_namespace_init, line 91) with its own arity error message,sys_namespace_init(used for the internal"sys.namespace"type backingsys.hash_info,sys.float_info,sys.thread_info,sys.int_info,sys._jit,sys.monitoring, etc.) still raises"types.SimpleNamespace() takes no positional arguments"on extra positional args — the wrong type name for this type. E.g.type(sys.hash_info)(1)would surface a misleadingtypes.SimpleNamespacemessage.🐛 Proposed fix
if positional.len() > 1 { return Err(crate::PyError::type_error( - "types.SimpleNamespace() takes no positional arguments", + "sys.namespace() takes no positional arguments", )); }🤖 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/module/sys/vm.rs` around lines 42 - 53, Update the extra-positional-argument error in sys_namespace_init to identify the internal sys.namespace type instead of types.SimpleNamespace, while leaving simple_namespace_init’s dedicated error message unchanged.
♻️ Duplicate comments (1)
pyre/bench/synth/seq_repeat_overflow.py (1)
22-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
is notfor object identity testing.For improved readability and idiomatic Python style, prefer
is notovernot ... is.♻️ Proposed refactor
- if (t * 1 is t) and (b * 1 is b) and (s * 1 is s) and not (lst * 1 is lst): + if (t * 1 is t) and (b * 1 is b) and (s * 1 is s) and (lst * 1 is not lst):🤖 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/bench/synth/seq_repeat_overflow.py` at line 22, Update the identity check in the sequence condition to use the idiomatic `is not` form for `lst * 1` and `lst`, while preserving the existing identity checks for `t`, `b`, and `s`.Source: Linters/SAST tools
🤖 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/bench/synth/enumerate_bignum_start.py`:
- Line 10: In the loop in enumerate_bignum_start.py, rename the unused iteration
variable i to _ while preserving the existing range(N) iteration behavior.
In `@pyre/bench/synth/int_from_bad_dunder.py`:
- Line 13: Rename the unused loop variable i to _ in the range loop, preserving
the existing iteration count and loop body behavior.
In `@pyre/bench/synth/seq_repeat_overflow.py`:
- Line 13: Rename the unused loop variable in the range loop from i to _ in
seq_repeat_overflow.py, preserving the loop count and body behavior.
In `@pyre/pyre-interpreter/src/async_operation.rs`:
- Around line 65-67: Remove the thread-local HANDLES cache of PyObjectRef values
and store these interpreter-owned objects in the interpreter’s shared execution
context with proper GC rooting. Update the associated access and initialization
paths to use that shared owner, preserving handle reuse without retaining
unrooted GC pointers in TLS.
---
Outside diff comments:
In `@pyre/pyre-interpreter/src/module/sys/vm.rs`:
- Around line 42-53: Update the extra-positional-argument error in
sys_namespace_init to identify the internal sys.namespace type instead of
types.SimpleNamespace, while leaving simple_namespace_init’s dedicated error
message unchanged.
---
Duplicate comments:
In `@pyre/bench/synth/seq_repeat_overflow.py`:
- Line 22: Update the identity check in the sequence condition to use the
idiomatic `is not` form for `lst * 1` and `lst`, while preserving the existing
identity checks for `t`, `b`, and `s`.
🪄 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: 64524496-9eed-444c-b5f6-f204f03d594f
📒 Files selected for processing (47)
pyre/bench/oserror_errno_fields_regression.pypyre/bench/synth/aiter_anext.pypyre/bench/synth/attr_instance_shadows_class.pypyre/bench/synth/bases_reassign_cache.pypyre/bench/synth/bool_dunder_error_no_leak.pypyre/bench/synth/bytes_split_whitespace_maxsplit.pypyre/bench/synth/callable_iterator_type.pypyre/bench/synth/complex_from_index.pypyre/bench/synth/complex_real_imag.pypyre/bench/synth/enumerate_bignum_start.pypyre/bench/synth/exception_dict_slot_reject.pypyre/bench/synth/hash_subclass_disabled.pypyre/bench/synth/import_from_name_path.pypyre/bench/synth/int_base0_error_literal.pypyre/bench/synth/int_from_bad_dunder.pypyre/bench/synth/int_max_str_digits.pypyre/bench/synth/iter_sentinel_stopiteration.pypyre/bench/synth/len_dunder_validation.pypyre/bench/synth/list_length_hint_validate.pypyre/bench/synth/list_nan_identity.pypyre/bench/synth/metaclass_conflict.pypyre/bench/synth/metaclass_getattribute_delattr.pypyre/bench/synth/reversed_disabled.pypyre/bench/synth/seq_repeat_overflow.pypyre/bench/synth/set_method_arity.pypyre/bench/synth/simple_namespace_type.pypyre/bench/synth/slots_class_var_conflict.pypyre/bench/synth/str_encode_text_codec.pypyre/bench/synth/syntaxerror_str.pypyre/bench/synth/tuple_contains_eq_raises.pypyre/bench/synth/type_dotted_name.pypyre/bench/synth/type_error_message_parity.pypyre/check.pypyre/pyre-interpreter/src/async_operation.rspyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/call.rspyre/pyre-interpreter/src/display.rspyre/pyre-interpreter/src/error.rspyre/pyre-interpreter/src/importing.rspyre/pyre-interpreter/src/lib.rspyre/pyre-interpreter/src/module/posix/interp_posix.rspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-interpreter/src/objspace/descroperation.rspyre/pyre-interpreter/src/type_methods.rspyre/pyre-interpreter/src/typedef.rspyre/pyre-object/src/listobject.rs
💤 Files with no reviewable changes (1)
- pyre/pyre-object/src/listobject.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3dc76477b
ℹ️ 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".
| thread_local! { | ||
| static HANDLES: std::cell::OnceCell<[PyObjectRef; 2]> = const { std::cell::OnceCell::new() }; |
There was a problem hiding this comment.
When aiter/anext are first called, these thread-local slots become the only references to the app-level function objects; the public builtins dict only holds the Rust wrappers. Unlike reduce_protocol, there is no extra root walker for these off-GC slots, so a later collection can move or reclaim the functions and their private globals, after which handle() returns stale PyObjectRefs.
Useful? React with 👍 / 👎.
| }; | ||
| Self::new(kind, msg) | ||
| let message = format!("[Errno {errno}] {strerror}"); | ||
| let exc = w_exception_new(exc_kind, &message); |
There was a problem hiding this comment.
For failed syscalls with a filename, both the w_filename argument and the newly-created exc are only raw Rust locals while this helper allocates the exception, args list, ints, and strings. Any GC during those allocations can move or sweep them before the later w_exception_set_* writes, which can corrupt or crash error construction on paths like failed open()/os.mkdir; push roots for the filename and exception before allocating the remaining fields.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyre/pyre-interpreter/src/display.rs (1)
1142-1163: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winMissing null checks before type verification.
is_exact_typedereferences the object to verify its type. Whilew_msgis securely guarded with a null check on line 1129,w_lineno,w_end, andw_filenameare not. Becausesyntax_error_attrreturns a null pointer for missing attributes (such as in a bareSyntaxError()), omitting these null checks will result in a null-pointer dereference crash.🛡️ Proposed fix to add null checks
- if pyre_object::pyobject::is_exact_type(w_lineno, &INT_TYPE) { + if !w_lineno.is_null() && pyre_object::pyobject::is_exact_type(w_lineno, &INT_TYPE) { let lineno = crate::baseobjspace::int_w(w_lineno).unwrap_or(0); let w_end = crate::baseobjspace::syntax_error_attr(obj, "end_lineno"); - let end = if pyre_object::pyobject::is_exact_type(w_end, &INT_TYPE) { + let end = if !w_end.is_null() && pyre_object::pyobject::is_exact_type(w_end, &INT_TYPE) { crate::baseobjspace::int_w(w_end).ok() } else { None }; Some(match end { Some(end) if end > lineno => { Wtf8Buf::from_string(format!("lines {lineno}-{end}")) } _ => Wtf8Buf::from_string(format!("line {lineno}")), }) } else { None }; // `have_filename` → `os.path.basename(self.filename or "???")`. let w_filename = crate::baseobjspace::syntax_error_attr(obj, "filename"); - if pyre_object::pyobject::is_exact_type(w_filename, &STR_TYPE) { + if !w_filename.is_null() && pyre_object::pyobject::is_exact_type(w_filename, &STR_TYPE) {🤖 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/display.rs` around lines 1142 - 1163, Add null-pointer checks for w_lineno, w_end, and w_filename before passing them to is_exact_type in the SyntaxError display logic. Update the surrounding attribute handling so missing attributes from syntax_error_attr are treated as absent and preserve the existing formatting behavior for valid integer and string values.
♻️ Duplicate comments (1)
pyre/pyre-interpreter/src/error.rs (1)
642-679: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winMissing GC rooting for freshly-allocated exception objects/fields.
Both constructors hold freshly-created
PyObjectRefs unrooted while calling functions that can trigger GC allocations (e.g.,w_list_new,w_int_new,w_str_new). This makes them vulnerable to being moved or collected during those allocations.
pyre/pyre-interpreter/src/error.rs#L642-L679: guardexcandw_filenamewithgc_roots::push_roots()andgc_roots::pin_root(...)inos_error_syscallbefore allocatingargs_listand its contents.pyre/pyre-interpreter/src/error.rs#L740-L758: guardexc,w_name, andw_pathinimport_error_name_pathbefore allocatingw_msg.🤖 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 642 - 679, Add GC root protection in os_error_syscall (pyre/pyre-interpreter/src/error.rs:642-679) by creating a gc_roots::push_roots() scope and pinning exc and w_filename before allocating args_list or its w_int_new/w_str_new contents. Apply the same protection in import_error_name_path (pyre/pyre-interpreter/src/error.rs:740-758), pinning exc, w_name, and w_path before allocating w_msg.
🤖 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.
Outside diff comments:
In `@pyre/pyre-interpreter/src/display.rs`:
- Around line 1142-1163: Add null-pointer checks for w_lineno, w_end, and
w_filename before passing them to is_exact_type in the SyntaxError display
logic. Update the surrounding attribute handling so missing attributes from
syntax_error_attr are treated as absent and preserve the existing formatting
behavior for valid integer and string values.
---
Duplicate comments:
In `@pyre/pyre-interpreter/src/error.rs`:
- Around line 642-679: Add GC root protection in os_error_syscall
(pyre/pyre-interpreter/src/error.rs:642-679) by creating a
gc_roots::push_roots() scope and pinning exc and w_filename before allocating
args_list or its w_int_new/w_str_new contents. Apply the same protection in
import_error_name_path (pyre/pyre-interpreter/src/error.rs:740-758), pinning
exc, w_name, and w_path before allocating w_msg.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 947a8b9a-f40b-45e5-8bd7-51b295f789ec
📒 Files selected for processing (7)
pyre/bench/oserror_errno_fields_regression.pypyre/bench/synth/iter_sentinel_stopiteration.pypyre/bench/synth/syntaxerror_str.pypyre/pyre-interpreter/src/display.rspyre/pyre-interpreter/src/error.rspyre/pyre-interpreter/src/lib.rspyre/pyre-interpreter/src/typedef.rs
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/bc7e76d7e32dca20107f49f5876ade03b99494cd/pyre-interpreter/src/error.rs#L762
When a failed from ... import ... reaches this helper, the supplied w_name/w_path can be freshly-created default strings, and the new exc is also only a Rust local. If w_exception_new or the later w_str_new(&message) triggers GC, those unrooted pointers can be moved or swept before the w_exception_set_* calls write them into the exception, so this needs the same shadow-root treatment used for the structured OSError path.
https://github.com/youknowone/pyre/blob/bc7e76d7e32dca20107f49f5876ade03b99494cd/pyre-interpreter/src/module/posix/interp_posix.rs#L456-L457
For path-taking calls that legitimately receive an empty path, such as posix.open('', flags) / os.open('', flags), the failing filename should still be '' on the raised OSError. This helper uses an empty string as the pathless sentinel for all callers, so those path failures now get no .filename at all; pass an explicit Option/sentinel from pathless call sites instead of treating "" as missing.
https://github.com/youknowone/pyre/blob/bc7e76d7e32dca20107f49f5876ade03b99494cd/pyre-interpreter/src/module/sys/vm.rs#L176
If a namespace __dict__ contains non-string/custom keys, sorting this copied Vec calls Python < via baseobjspace::compare; that code can allocate or run user code and trigger GC while the only copies in items are unrooted raw PyObjectRefs. The dict will have its slots forwarded, but the snapshot vector will not, so later comparisons or the final py_str/py_repr loop can use stale pointers; keep the item snapshot rooted or re-read from a rooted container around the sort.
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef7797a278
ℹ️ 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".
| Some(t) => pyre_object::w_str_new(t), | ||
| None => pyre_object::w_none(), | ||
| }; | ||
| let details = pyre_object::w_tuple_new(vec![ |
There was a problem hiding this comment.
For malformed source with a location, w_text and then details are only raw Rust locals while more managed objects are allocated to build the tuple/list. Under the moving GC, a collection during w_tuple_new, w_str_new(&message), or w_list_new can leave the final SyntaxError args populated with stale pointers even though only exc is pinned; pin the intermediate text/details before constructing the final args.
Useful? React with 👍 / 👎.
| w_path: PyObjectRef, | ||
| ) -> Self { | ||
| let message = msg.into(); | ||
| let exc = w_exception_new(ExcKind::ImportError, &message); |
There was a problem hiding this comment.
When a failed from-import reaches this helper, w_name and w_path are raw locals while w_exception_new allocates, and the returned exc is then raw while w_str_new(&message) allocates. Under GC pressure this can move or sweep those objects before the setters stamp .msg, .name, and .path, corrupting structured ImportError construction; mirror the rooting used by the other structured exception helpers before these allocations.
Useful? React with 👍 / 👎.
|
|
||
| /// Raise the structured OSError for `errno` (no filename). The caller's | ||
| /// platform message is superseded by the errno-derived strerror. | ||
| pub fn os_error_with_errno(errno: i32, _msg: impl Into<String>) -> Self { |
There was a problem hiding this comment.
In sandbox builds, host_seam::seam_os_err still passes non-empty path context for operations such as open (builtins.rs:10306) and posix.open (interp_posix.rs:502), but this wrapper now discards _msg and creates an OSError with no filename. Those sandbox failures therefore lose the path in str(e) and leave e.filename unset, unlike the native helpers that were updated to call os_error_syscall with a filename.
Useful? React with 👍 / 👎.
| let code = crate::compile::compile_source(&s, mode) | ||
| .map_err(|e| compile_err_to_syntax_error(e, &s))?; |
There was a problem hiding this comment.
When exec() or eval() compiles a string that has a syntax error, this path now exposes the compiler source_path through the structured SyntaxError, but compile_source hard-codes that path to <pyre> (compile.rs:17). CPython/PyPy report <string> for these APIs, so caught errors now have e.filename == "<pyre>" and top-level rendering points at the wrong file; pass <string> when compiling this helper's string source.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/c76e77f9ace2ceb252e236a046c324fd4164b71c/pyre-interpreter/src/module/posix/interp_posix.rs#L459
When these posix wrappers are called with a bytes path, extract_path has already converted it to a Rust String, and this line wraps that string as a Python str for OSError.filename. CPython/PyPy keep the original filesystem path object, so cases like os.mkdir(b'/existing') or posix.open(b'/missing', flags) should expose a bytes e.filename; this currently returns a decoded/lossy str instead. Preserve the original path object or fspath result when constructing the structured OSError.
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
pyre/bench/synth/set_method_arity.py (1)
5-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid shadowing the Python built-in
callable.The argument name
callableshadows the built-in function. Please rename it tofuncortargetto prevent potential confusion.♻️ Proposed refactor
-def kind(callable, args): +def kind(func, args): try: - callable(*args) + func(*args)🤖 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/bench/synth/set_method_arity.py` around lines 5 - 7, Rename the callable parameter in kind to func or target, and update its invocation inside the try block to use the new name while preserving the existing behavior.Source: Linters/SAST tools
🤖 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/bench/synth/hash_subclass_disabled.py`:
- Line 27: Rename the unused loop control variable in the range loop to `_`,
leaving the loop body and iteration count unchanged.
In `@pyre/bench/synth/list_nan_identity.py`:
- Line 10: Rename the unused loop control variable i to _ in the range(N) loop,
leaving the loop body and iteration behavior unchanged.
In `@pyre/bench/synth/reversed_disabled.py`:
- Line 25: Rename the unused loop variable i to _ in the range(N) loop, leaving
the loop body and iteration behavior unchanged.
In `@pyre/pyre-interpreter/src/importing.rs`:
- Around line 2281-2290: Update the __file__ handling that assigns w_pkgpath so
non-None values are accepted only when is_str(v) is true; default all other
values, including non-string objects, to the existing "unknown location" string
before utf8_w is called. Preserve the current AttributeError and other-error
handling, and keep the subsequent pkgpath conversion unchanged.
---
Duplicate comments:
In `@pyre/bench/synth/set_method_arity.py`:
- Around line 5-7: Rename the callable parameter in kind to func or target, and
update its invocation inside the try block to use the new name while preserving
the existing behavior.
🪄 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: ab50af8f-4da3-429d-980a-64f16fd4c118
📒 Files selected for processing (53)
pyre/bench/oserror_errno_fields_regression.pypyre/bench/synth/aiter_anext.pypyre/bench/synth/attr_instance_shadows_class.pypyre/bench/synth/bases_reassign_cache.pypyre/bench/synth/bool_dunder_error_no_leak.pypyre/bench/synth/bytes_split_whitespace_maxsplit.pypyre/bench/synth/callable_iterator_type.pypyre/bench/synth/complex_from_index.pypyre/bench/synth/complex_real_imag.pypyre/bench/synth/enumerate_bignum_start.pypyre/bench/synth/exception_dict_slot_reject.pypyre/bench/synth/hash_subclass_disabled.pypyre/bench/synth/import_from_name_path.pypyre/bench/synth/int_base0_error_literal.pypyre/bench/synth/int_from_bad_dunder.pypyre/bench/synth/int_max_str_digits.pypyre/bench/synth/iter_sentinel_stopiteration.pypyre/bench/synth/len_dunder_validation.pypyre/bench/synth/list_length_hint_validate.pypyre/bench/synth/list_nan_identity.pypyre/bench/synth/metaclass_conflict.pypyre/bench/synth/metaclass_getattribute_delattr.pypyre/bench/synth/reversed_disabled.pypyre/bench/synth/seq_repeat_overflow.pypyre/bench/synth/set_method_arity.pypyre/bench/synth/simple_namespace_type.pypyre/bench/synth/slots_class_var_conflict.pypyre/bench/synth/str_encode_text_codec.pypyre/bench/synth/syntaxerror_location.pypyre/bench/synth/syntaxerror_str.pypyre/bench/synth/tuple_contains_eq_raises.pypyre/bench/synth/type_dotted_name.pypyre/bench/synth/type_error_message_parity.pypyre/check.pypyre/pyre-interpreter/src/async_operation.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/importing.rspyre/pyre-interpreter/src/lib.rspyre/pyre-interpreter/src/module/_collections/mod.rspyre/pyre-interpreter/src/module/posix/interp_posix.rspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-interpreter/src/objspace/descroperation.rspyre/pyre-interpreter/src/type_methods.rspyre/pyre-interpreter/src/typedef.rspyre/pyre-jit/src/eval.rspyre/pyre-object/src/listobject.rspyre/pyrex/src/lib.rs
💤 Files with no reviewable changes (1)
- pyre/pyre-object/src/listobject.rs
| def main(): | ||
| disabled = 0 | ||
| total = 0 | ||
| for i in range(N): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Rename unused loop variable i to _.
The loop control variable i is not used within the loop body. Renaming it to _ clarifies intent.
♻️ Proposed refactor
- for i in range(N):
+ for _ in range(N):📝 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.
| for i in range(N): | |
| for _ in range(N): |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 25-25: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
🤖 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/bench/synth/reversed_disabled.py` at line 25, Rename the unused loop
variable i to _ in the range(N) loop, leaving the loop body and iteration
behavior unchanged.
Source: Linters/SAST tools
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/c4c34685e3110b701cf30030efc2dc84c1d8c4e8/pyre-interpreter/src/type_methods.rs#L457-L460
For a non-list/tuple iterable whose __iter__ raises or initializes state that __length_hint__ reads, this probes the hint before the iterator exists. Python constructs the iterator first and only then asks for the preallocation hint, so class X: __iter__ raising RuntimeError and __length_hint__ raising ValueError should report the RuntimeError; this path reports ValueError and can run side effects that should not be reached.
https://github.com/youknowone/pyre/blob/c4c34685e3110b701cf30030efc2dc84c1d8c4e8/pyre-interpreter/src/async_operation.rs#L32
When __aiter__ is provided as staticmethod, classmethod, or another descriptor, typ.__aiter__ has already applied class-attribute access semantics, so calling meth(obj) passes an extra instance argument. Special-method dispatch should bind the descriptor for the instance before invoking it; otherwise classes that CPython accepts, such as __aiter__ = staticmethod(lambda: async_iter), fail here, and the __anext__ calls below have the same descriptor issue.
https://github.com/youknowone/pyre/blob/c4c34685e3110b701cf30030efc2dc84c1d8c4e8/pyre-interpreter/src/type_methods.rs#L1803-L1806
After accepting and stripping the z option here, complex formats such as format(complex(-0.0, -0.0), 'z') are accepted but the complex branch never consumes p.no_neg_zero: the default-type fast path still returns str(self), and the component specs omit z. This leaves -0 signs in complex output instead of coercing negative zero components to positive zero.
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7757bffdd
ℹ️ 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".
| // Only the float presentation types e/E/f/F/g/G/n are valid for | ||
| // complex; reject any other code with a `complex`-typed message | ||
| // rather than letting the per-part float formatter report `float`. | ||
| if p.ty != '\0' && !matches!(p.ty, 'e' | 'E' | 'f' | 'F' | 'g' | 'G' | 'n') { |
There was a problem hiding this comment.
For complex values with the new z option, e.g. format(complex(-0.0, -0.0), 'z.1f') or bare 'z', this branch accepts the spec but the component specs built below never include or apply p.no_neg_zero, and bare 'z' even returns str(self). CPython 3.14 formats those as 0.0+0.0j/(0+0j), so pyre will still expose negative-zero components despite accepting the option.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pyre/pyre-interpreter/src/typedef.rs (1)
2617-2627: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPrevent duplicate evaluation of
__len__for non-integer results.If
__len__returns a non-integer (such as an object requiring__index__coercion),is_int(len_result)evaluates to false, causing the function to fall through tois_true(w_obj)at line 2631.is_truewill then look up and invoke__len__a second time to perform the coercion.This causes any side-effects in
__len__to execute twice. Handle the coercion oflen_resultdirectly within this block to avoid re-evaluating it.🤖 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/typedef.rs` around lines 2617 - 2627, Update the __len__ handling around len_result so non-integer results are coerced directly from the already-evaluated value, including __index__-style coercion, rather than falling through to is_true(w_obj). Preserve the existing non-negative validation and boolean conversion, and ensure __len__ is invoked only once.pyre/pyre-interpreter/src/module/sys/vm.rs (1)
57-80: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
__pyre_kw__collision drops a legitimately-named user kwarg.The skip at line 68 filters by key name (
name == "__pyre_kw__") rather than by the sentinel value identity thatsplit_builtin_kwargsuses to distinguish the marker from real data. If a caller doesSimpleNamespace(__pyre_kw__=5), that real kwarg is silently dropped instead of being set on__dict__, since it matches the marker key name regardless of its value.🐛 Proposed fix: check sentinel identity, not key name
if pyre_object::is_str(key) { if let Ok(name) = pyre_object::w_str_get_wtf8(key).as_str() { - if name == "__pyre_kw__" { + if name == "__pyre_kw__" + && pyre_object::kw_marker::is_kw_marker_sentinel(value) + { continue; } crate::baseobjspace::setdictvalue(self_obj, name, value); continue; } }🤖 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/module/sys/vm.rs` around lines 57 - 80, Update namespace_apply_kwargs to skip only the __pyre_kw__ marker identified by the same sentinel-value identity used by split_builtin_kwargs, not by matching the key name. Preserve legitimate user kwargs such as __pyre_kw__=5 by writing them to the namespace dictionary, while continuing to exclude the actual marker entry.pyre/pyre-interpreter/src/baseobjspace.rs (1)
8794-8827: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
del-dispatch shares the same object-vs-type default-baseline risk as the__getattribute__gate, and duplicates theis_defaultcheck.Both branches (instance receiver at Lines 8800-8807 and type receiver at Lines 8814-8823) inline the identical
lookup_in_type(crate::typedef::w_object(), "__delattr__").is_some_and(...)check. For the type-receiver branch, this compares the metatype's__delattr__againstobject's default rather thantype's own default — see the consolidated comment for the shared concern with the__getattribute__gate above. Independently of that, the duplicated inline check is worth extracting into a small helper to avoid the two copies drifting apart.♻️ Proposed refactor to de-duplicate the default-dunder check
+unsafe fn dunder_is_default(w_type: PyObjectRef, dunder: &str, found: PyObjectRef) -> bool { + lookup_in_type(crate::typedef::w_object(), dunder).is_some_and(|d| std::ptr::eq(found, d)) +} + pub fn delattr_str(obj: PyObjectRef, name: &str) -> PyResult { let obj = crate::module::_weakref::interp__weakref::force(obj)?; unsafe { if is_instance(obj) { let w_type = w_instance_get_type(obj); if let Some(da) = lookup_in_type(w_type, "__delattr__") { - let is_default = lookup_in_type(crate::typedef::w_object(), "__delattr__") - .is_some_and(|d| std::ptr::eq(da, d)); - if !is_default { + if !dunder_is_default(w_type, "__delattr__", da) { let w_name = w_str_new(name); return get_and_call_function(da, obj, w_type, &[w_name]).map(|_| w_none()); } } } else if is_type(obj) { if let Some(w_metatype) = crate::typedef::r#type(obj) { if let Some(da) = lookup_in_type(w_metatype, "__delattr__") { - let is_default = lookup_in_type(crate::typedef::w_object(), "__delattr__") - .is_some_and(|d| std::ptr::eq(da, d)); - if !is_default { + if !dunder_is_default(w_metatype, "__delattr__", da) { let w_name = w_str_new(name); return get_and_call_function(da, obj, w_metatype, &[w_name]) .map(|_| w_none()); } } } } } object_delattr(obj, name) }🤖 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/baseobjspace.rs` around lines 8794 - 8827, Update delattr_str to use a shared helper for detecting whether a resolved __delattr__ is the default implementation, passing object’s default for instance receivers and type’s default for type receivers. Remove both duplicated inline is_default checks while preserving custom instance and metaclass dispatch behavior.
♻️ Duplicate comments (1)
pyre/bench/synth/hash_subclass_disabled.py (1)
27-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename unused loop control variable.
The loop variable
iis not used within the loop body. Consider renaming it to_to clarify intent and resolve the static analysis hint.♻️ Proposed refactor
- for i in range(N): + for _ in range(N):🤖 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/bench/synth/hash_subclass_disabled.py` at line 27, Rename the unused loop control variable in the range loop from i to _ while preserving the existing iteration count and loop body.Source: Linters/SAST tools
🤖 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/pyre-interpreter/src/module/sys/vm.rs`:
- Around line 158-215: Update simple_namespace_repr to preserve the insertion
order returned by self.__dict__.items() rather than sorting keys. Remove the
sort_by comparator, sort_error Cell, and related comparison/error plumbing,
while keeping the existing recursive repr guard and key/value formatting
unchanged.
---
Outside diff comments:
In `@pyre/pyre-interpreter/src/baseobjspace.rs`:
- Around line 8794-8827: Update delattr_str to use a shared helper for detecting
whether a resolved __delattr__ is the default implementation, passing object’s
default for instance receivers and type’s default for type receivers. Remove
both duplicated inline is_default checks while preserving custom instance and
metaclass dispatch behavior.
In `@pyre/pyre-interpreter/src/module/sys/vm.rs`:
- Around line 57-80: Update namespace_apply_kwargs to skip only the __pyre_kw__
marker identified by the same sentinel-value identity used by
split_builtin_kwargs, not by matching the key name. Preserve legitimate user
kwargs such as __pyre_kw__=5 by writing them to the namespace dictionary, while
continuing to exclude the actual marker entry.
In `@pyre/pyre-interpreter/src/typedef.rs`:
- Around line 2617-2627: Update the __len__ handling around len_result so
non-integer results are coerced directly from the already-evaluated value,
including __index__-style coercion, rather than falling through to
is_true(w_obj). Preserve the existing non-negative validation and boolean
conversion, and ensure __len__ is invoked only once.
---
Duplicate comments:
In `@pyre/bench/synth/hash_subclass_disabled.py`:
- Line 27: Rename the unused loop control variable in the range loop from i to _
while preserving the existing iteration count and loop body.
🪄 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: 9e976da7-2ae3-41fb-bce0-cce0c8323218
📒 Files selected for processing (52)
pyre/bench/oserror_errno_fields_regression.pypyre/bench/synth/aiter_anext.pypyre/bench/synth/attr_instance_shadows_class.pypyre/bench/synth/bases_reassign_cache.pypyre/bench/synth/bool_dunder_error_no_leak.pypyre/bench/synth/bytes_split_whitespace_maxsplit.pypyre/bench/synth/callable_iterator_type.pypyre/bench/synth/complex_from_index.pypyre/bench/synth/complex_real_imag.pypyre/bench/synth/enumerate_bignum_start.pypyre/bench/synth/exception_dict_slot_reject.pypyre/bench/synth/format_z_negative_zero.pypyre/bench/synth/hash_subclass_disabled.pypyre/bench/synth/import_from_name_path.pypyre/bench/synth/int_base0_error_literal.pypyre/bench/synth/int_from_bad_dunder.pypyre/bench/synth/int_max_str_digits.pypyre/bench/synth/iter_sentinel_stopiteration.pypyre/bench/synth/len_dunder_validation.pypyre/bench/synth/list_length_hint_validate.pypyre/bench/synth/list_nan_identity.pypyre/bench/synth/metaclass_conflict.pypyre/bench/synth/metaclass_getattribute_delattr.pypyre/bench/synth/reversed_disabled.pypyre/bench/synth/seq_repeat_overflow.pypyre/bench/synth/set_method_arity.pypyre/bench/synth/simple_namespace_type.pypyre/bench/synth/slots_class_var_conflict.pypyre/bench/synth/str_encode_text_codec.pypyre/bench/synth/syntaxerror_location.pypyre/bench/synth/syntaxerror_str.pypyre/bench/synth/tuple_contains_eq_raises.pypyre/bench/synth/type_dotted_name.pypyre/bench/synth/type_error_message_parity.pypyre/check.pypyre/pyre-interpreter/src/async_operation.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/importing.rspyre/pyre-interpreter/src/lib.rspyre/pyre-interpreter/src/module/posix/interp_posix.rspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-interpreter/src/objspace/descroperation.rspyre/pyre-interpreter/src/type_methods.rspyre/pyre-interpreter/src/typedef.rspyre/pyre-object/src/listobject.rspyre/pyrex/src/lib.rs
💤 Files with no reviewable changes (1)
- pyre/pyre-object/src/listobject.rs
| /// `_structseq.py:174 SimpleNamespace.__repr__` — `namespace(k=v, ...)` over | ||
| /// the sorted `__dict__` items (`%s=%r`), returning `namespace(...)` when the | ||
| /// instance is already being repr'd on this thread. | ||
| fn simple_namespace_repr(args: &[PyObjectRef]) -> crate::PyResult { | ||
| let Some(&self_obj) = args.first() else { | ||
| return Err(crate::PyError::type_error( | ||
| "__repr__() missing 1 required positional argument: 'self'", | ||
| )); | ||
| }; | ||
| let Some(_guard) = crate::display::ReprGuard::enter(self_obj) else { | ||
| return Ok(w_str_new("namespace(...)")); | ||
| }; | ||
| let dict = crate::baseobjspace::getattr_str(self_obj, "__dict__")?; | ||
| // `sorted(self.__dict__.items())` — order by the key objects with Python | ||
| // `<`, not by their `str()`. Incomparable keys (e.g. `int` mixed with | ||
| // `str`) raise, halting the repr as the sort itself does. Rust's `sort_by` | ||
| // closure cannot return `Result`, so a raising comparison is captured in a | ||
| // `Cell` and surfaced once the sort completes. | ||
| let mut items = unsafe { pyre_object::w_dict_items(dict) }; | ||
| let sort_error: std::cell::Cell<Option<crate::PyError>> = std::cell::Cell::new(None); | ||
| let lt = |x: PyObjectRef, y: PyObjectRef| -> bool { | ||
| if let Some(e) = sort_error.take() { | ||
| sort_error.set(Some(e)); | ||
| return false; | ||
| } | ||
| match crate::baseobjspace::compare(x, y, crate::baseobjspace::CompareOp::Lt) { | ||
| Ok(r) => crate::baseobjspace::is_true(r).unwrap_or_else(|e| { | ||
| sort_error.set(Some(e)); | ||
| false | ||
| }), | ||
| Err(e) => { | ||
| sort_error.set(Some(e)); | ||
| false | ||
| } | ||
| } | ||
| }; | ||
| items.sort_by(|a, b| { | ||
| if lt(a.0, b.0) { | ||
| std::cmp::Ordering::Less | ||
| } else if lt(b.0, a.0) { | ||
| std::cmp::Ordering::Greater | ||
| } else { | ||
| std::cmp::Ordering::Equal | ||
| } | ||
| }); | ||
| if let Some(e) = sort_error.take() { | ||
| return Err(e); | ||
| } | ||
| let mut parts = Vec::with_capacity(items.len()); | ||
| for (k, v) in items { | ||
| parts.push(format!( | ||
| "{}={}", | ||
| unsafe { crate::display::py_str(k)? }, | ||
| unsafe { crate::display::py_repr(v)? } | ||
| )); | ||
| } | ||
| Ok(w_str_new(&format!("namespace({})", parts.join(", ")))) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
git ls-files pyre/pyre-interpreter/src/module/sys/vm.rs pyre/pyre-interpreter/src/module/sys -z | tr '\0' '\n' | sed -n '1,20p'Repository: youknowone/pyre
Length of output: 2014
🌐 Web query:
CPython SimpleNamespace __repr__ insertion order Changed in version 3.9 PyPy 3.11 SimpleNamespace repr order
💡 Result:
In CPython, the repr order for types.SimpleNamespace changed from alphabetical order to insertion order in version 3.9 [1][2][3]. This change was implemented to align SimpleNamespace's behavior with that of the standard dictionary, which preserves insertion order [4][3]. Prior to this change, SimpleNamespace sorted its attributes alphabetically in its representation to ensure consistent, readable output, as it was originally introduced in Python 3.3 before dictionary order preservation became a standard feature [4][3]. Regarding PyPy 3.11, PyPy generally aims for high compatibility with CPython. While specific internal implementation details can vary, PyPy follows the established CPython behavior for standard library types like SimpleNamespace to ensure consistency across the ecosystem [5]. As of Python 3.9+, SimpleNamespace is documented and expected to display attributes in insertion order [1][6][7].
Citations:
- 1: https://docs.python.org/3.9/library/types.html
- 2: https://github.com/python/cpython/blob/3.9/Doc/library/types.rst
- 3: https://bugs.python.org/issue39075
- 4: types.SimpleNamespace should preserve attribute ordering (?) python/cpython#83256
- 5: pypy/pypy@b850bf1
- 6: https://docs.python.org/3/library/types.html
- 7: https://docs.python.org/3.11/library/types.html
SimpleNamespace.__repr__ should preserve insertion order, not sort keys.
types.SimpleNamespace now formats attributes in self.__dict__ order in both CPython 3.14 and PyPy 3.11, so sorted(self.__dict__.items()) produces the wrong repr for any multi-attribute instance. Dropping the sort matches the reference behavior and removes the need for the comparison/error plumbing.
Proposed fix
- let mut items = unsafe { pyre_object::w_dict_items(dict) };
- let sort_error: std::cell::Cell<Option<crate::PyError>> = std::cell::Cell::new(None);
- let lt = |x: PyObjectRef, y: PyObjectRef| -> bool {
- if let Some(e) = sort_error.take() {
- sort_error.set(Some(e));
- return false;
- }
- match crate::baseobjspace::compare(x, y, crate::baseobjspace::CompareOp::Lt) {
- Ok(r) => crate::baseobjspace::is_true(r).unwrap_or_else(|e| {
- sort_error.set(Some(e));
- false
- }),
- Err(e) => {
- sort_error.set(Some(e));
- false
- }
- }
- };
- items.sort_by(|a, b| {
- if lt(a.0, b.0) {
- std::cmp::Ordering::Less
- } else if lt(b.0, a.0) {
- std::cmp::Ordering::Greater
- } else {
- std::cmp::Ordering::Equal
- }
- });
- if let Some(e) = sort_error.take() {
- return Err(e);
- }
+ let items = unsafe { pyre_object::w_dict_items(dict) };📝 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.
| /// `_structseq.py:174 SimpleNamespace.__repr__` — `namespace(k=v, ...)` over | |
| /// the sorted `__dict__` items (`%s=%r`), returning `namespace(...)` when the | |
| /// instance is already being repr'd on this thread. | |
| fn simple_namespace_repr(args: &[PyObjectRef]) -> crate::PyResult { | |
| let Some(&self_obj) = args.first() else { | |
| return Err(crate::PyError::type_error( | |
| "__repr__() missing 1 required positional argument: 'self'", | |
| )); | |
| }; | |
| let Some(_guard) = crate::display::ReprGuard::enter(self_obj) else { | |
| return Ok(w_str_new("namespace(...)")); | |
| }; | |
| let dict = crate::baseobjspace::getattr_str(self_obj, "__dict__")?; | |
| // `sorted(self.__dict__.items())` — order by the key objects with Python | |
| // `<`, not by their `str()`. Incomparable keys (e.g. `int` mixed with | |
| // `str`) raise, halting the repr as the sort itself does. Rust's `sort_by` | |
| // closure cannot return `Result`, so a raising comparison is captured in a | |
| // `Cell` and surfaced once the sort completes. | |
| let mut items = unsafe { pyre_object::w_dict_items(dict) }; | |
| let sort_error: std::cell::Cell<Option<crate::PyError>> = std::cell::Cell::new(None); | |
| let lt = |x: PyObjectRef, y: PyObjectRef| -> bool { | |
| if let Some(e) = sort_error.take() { | |
| sort_error.set(Some(e)); | |
| return false; | |
| } | |
| match crate::baseobjspace::compare(x, y, crate::baseobjspace::CompareOp::Lt) { | |
| Ok(r) => crate::baseobjspace::is_true(r).unwrap_or_else(|e| { | |
| sort_error.set(Some(e)); | |
| false | |
| }), | |
| Err(e) => { | |
| sort_error.set(Some(e)); | |
| false | |
| } | |
| } | |
| }; | |
| items.sort_by(|a, b| { | |
| if lt(a.0, b.0) { | |
| std::cmp::Ordering::Less | |
| } else if lt(b.0, a.0) { | |
| std::cmp::Ordering::Greater | |
| } else { | |
| std::cmp::Ordering::Equal | |
| } | |
| }); | |
| if let Some(e) = sort_error.take() { | |
| return Err(e); | |
| } | |
| let mut parts = Vec::with_capacity(items.len()); | |
| for (k, v) in items { | |
| parts.push(format!( | |
| "{}={}", | |
| unsafe { crate::display::py_str(k)? }, | |
| unsafe { crate::display::py_repr(v)? } | |
| )); | |
| } | |
| Ok(w_str_new(&format!("namespace({})", parts.join(", ")))) | |
| } | |
| /// `_structseq.py:174 SimpleNamespace.__repr__` — `namespace(k=v, ...)` over | |
| /// the sorted `__dict__` items (`%s=%r`), returning `namespace(...)` when the | |
| /// instance is already being repr'd on this thread. | |
| fn simple_namespace_repr(args: &[PyObjectRef]) -> crate::PyResult { | |
| let Some(&self_obj) = args.first() else { | |
| return Err(crate::PyError::type_error( | |
| "__repr__() missing 1 required positional argument: 'self'", | |
| )); | |
| }; | |
| let Some(_guard) = crate::display::ReprGuard::enter(self_obj) else { | |
| return Ok(w_str_new("namespace(...)")); | |
| }; | |
| let dict = crate::baseobjspace::getattr_str(self_obj, "__dict__")?; | |
| let items = unsafe { pyre_object::w_dict_items(dict) }; | |
| let mut parts = Vec::with_capacity(items.len()); | |
| for (k, v) in items { | |
| parts.push(format!( | |
| "{}={}", | |
| unsafe { crate::display::py_str(k)? }, | |
| unsafe { crate::display::py_repr(v)? } | |
| )); | |
| } | |
| Ok(w_str_new(&format!("namespace({})", parts.join(", ")))) | |
| } |
🤖 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/module/sys/vm.rs` around lines 158 - 215, Update
simple_namespace_repr to preserve the insertion order returned by
self.__dict__.items() rather than sorting keys. Remove the sort_by comparator,
sort_error Cell, and related comparison/error plumbing, while keeping the
existing recursive repr guard and key/value formatting unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 195bb2dbc5
ℹ️ 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".
| crate::baseobjspace::length_hint( | ||
| pyre_object::gc_roots::shadow_stack_get(root_base + 1), | ||
| 0, | ||
| )?; |
There was a problem hiding this comment.
For non-list/tuple inputs, this now calls length_hint before iter. list.extend should first obtain the iterator, so inputs whose __iter__ is absent or raises must surface that iterator error even if __len__/__length_hint__ is bad; e.g. a non-iterable object with __len__ raising ValueError should still produce the usual “object is not iterable” TypeError, but this order raises the length error instead. Move the length-hint probe after the successful iter(...) call.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a3916a228
ℹ️ 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".
| if unsafe { crate::is_function(method) } { | ||
| return Ok(pyre_object::w_method_new(method, obj, w_type)); |
There was a problem hiding this comment.
For builtin-layout subclasses with an instance dict, this branch binds every crate::is_function descriptor before asking get() whether it is actually a descriptor. Module-level builtins such as len are BUILTIN_FUNCTION_TYPE and are explicitly left unbound by the normal get() path, so class T(tuple): f = len; T().f([1]) now passes the tuple instance as an extra argument and raises instead of returning 1.
Useful? React with 👍 / 👎.
| // `str`) raise, halting the repr as the sort itself does. Rust's `sort_by` | ||
| // closure cannot return `Result`, so a raising comparison is captured in a | ||
| // `Cell` and surfaced once the sort completes. | ||
| let mut items = unsafe { pyre_object::w_dict_items(dict) }; |
There was a problem hiding this comment.
When a namespace dict contains custom keys, the sort_by comparator below can run Python __lt__ and allocate or collect while items is only a Rust Vec of raw PyObjectRef pairs. A moving collection updates the dict's slots but not this snapshot, so later comparisons or the final formatting can dereference stale key/value pointers; root the snapshot or avoid holding these raw refs across Python calls.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/b6d6af45baadbcd988971aeb274ff796538e869b/pyre-interpreter/src/error.rs#L594-L596
This version pins the location details, but the freshly allocated w_msg is still only a raw local when w_list_new allocates the final args list. For syntax errors under GC pressure, a collection inside that allocation can move or reclaim the message string before it is stored in args, leaving SyntaxError.msg/str(e) with a stale pointer; pin w_msg and reload it the same way the details tuple is handled.
https://github.com/youknowone/pyre/blob/b6d6af45baadbcd988971aeb274ff796538e869b/pyre-interpreter/src/importing.rs#L2270-L2273
When the failed import target lacks __name__ (for example after deleting a module's __name__), this creates a fresh default name string and then continues to allocate/probe the path before using it. Because that default w_pkgname is not rooted until import_error_name_path is called, a moving collection during the later default-path allocation can leave the w_str_get_value and structured ImportError setters using a stale pointer; keep the name/path objects on the shadow stack across this construction.
ℹ️ 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".
The exception constructors pin incoming young caller refs and freshly allocated young values, then use the bare local across further allocations. The precise moving collector may relocate a young object during an intervening allocation, so the raw local can point at the old address even though the pin kept it alive. Reload each pinned ref from its shadow-stack slot immediately before the storing use, capturing the slot via shadow_stack_len() right before the matching pin_root (the function.rs reload idiom): - key_error_with_key: reload key before w_list_new(vec![key]). - os_error_syscall: reload w_filename before set_filename. - import_error_name_path: reload w_name/w_path before the setters (per-object slots; an absent null ref has no slot and stays null). - to_exc_object: reload w_name_context/w_obj_context before set_name/ set_attr_obj, and the args msg string before set_import_msg. - syntax_error_located: pin each details-tuple element and reload before assembling the tuple, then reload details across the message alloc. Assisted-by: Claude
attr_instance_shadows_class asserted `td.d == "data"` but never placed a competing entry in the instance dict, so the check passed regardless of descriptor precedence: the data descriptor's no-op __set__ never writes __dict__, and td.d was only ever read. Inject td.__dict__["d"] = "inst" directly so the data descriptor's __get__ genuinely competes with (and wins over) an instance-dict entry. Output stays 30000 across CPython/PyPy/pyre. Assisted-by: Claude
On Windows `std::io::Error::raw_os_error()` reports a Win32 error code, not a C errno, so a failed syscall carried e.g. ERROR_ALREADY_EXISTS (183) where the errno tables expect EEXIST (17). `os_error_errno_subclass` keys on the libc constants and returned None for those codes, so `os.mkdir` of an existing directory raised a bare OSError instead of FileExistsError. `open()` of a missing path only mapped because ERROR_FILE_NOT_FOUND and ENOENT are both 2. Add `io_error_posix_errno`, translating the common `ErrorKind` variants to their POSIX errno on Windows and keeping the raw code for the rest; it is the existing `raw_os_error()` value unchanged off Windows. Route the posix `io_err` boundary and `builtin_open`'s read error through it. Assisted-by: Claude
Extend the Windows Win32-code translation to the rest of the syscall boundaries that build an OSError from a std::io::Error, so the errno subclass and .errno slot are consistent there too: fd_io_err (fd read / write / seek), close, the buffered-write flush, and posix stat, fstat and getlogin. Each of those sites had its own fallback errno for an error carrying no OS code (5, 2, 9, 0), so give io_error_posix_errno a `default` parameter and pass the value each caller already used, keeping the off-Windows result identical to the previous `raw_os_error().unwrap_or(...)`. The remaining raw_os_error callers sit behind POSIX-only APIs (sem_wait, ftruncate, sigaction, termios, getrlimit, pwd/grp, kqueue, clock_settime, libc sockets) that are cfg'd out on Windows, and the errno comparisons that drive EINTR retries are left alone. Assisted-by: Claude
run_selfcheck recorded only the exit code, so a guard that raised left no trace of why: the script's traceback was captured and discarded, and a failure reproducible on one platform had to be reconstructed from the source instead of read from the log. Print the tail of both captured streams on the two failure paths (nonzero exit / missing marker), bounded to the last 40 lines each with a count of what was omitted so a runaway script cannot flood the log. Assisted-by: Claude
sys.platform is "win32" on the Windows build while the installed filesystem module is posix, so os.name stays "posix": the stdlib's os.name == "nt" branches are dormant there but its sys.platform ones are not, and those import _winapi. Without the module `import shutil` — and so tempfile, and everything downstream of it — raised ModuleNotFoundError. Define the names a shutil call goes on to reach: NeedCurrentDirectoryForExePath, which shutil.which invokes on every executable lookup, and the CopyFile2 flag and error constants. CopyFile2 itself is left out, so shutil.copyfile's hasattr probe fails and its generic read/write copy runs. Assisted-by: Claude
clean_strerror built its message with io::Error::from_raw_os_error, which reads its argument as a Win32 code on Windows. The errnos reaching it there are now the POSIX ones io_error_posix_errno translates to, so the message described an unrelated system error: 17 is EEXIST but also ERROR_NOT_SAME_DEVICE. Go through libc::strerror, whose table is errno-keyed, which is the source os.strerror already reports from. Assisted-by: Claude
The guard reached for tempfile.gettempdir and tempfile.mkdtemp, which import shutil; on Windows that chain ends at _winapi, so the script died before reaching any assertion. Create a pid-named directory in the cwd instead. Drop the os.name == "posix" split too: posix is the module installed on every platform, so the branch guarding the errno values was always the one taken, and those values are now asserted unconditionally. Assisted-by: Claude
The tuple was a second, hand-maintained list and had drifted from the registry `import` resolves against. It named 8 modules that are not importable as builtins -- _datetime, _decimal, _json, _md5, _sha1, _sha2 and _sha3 raise ModuleNotFoundError, _collections_abc loads from disk -- and omitted 20 that are compiled in, among them array, zlib, gc, mmap, unicodedata, pyexpat, _ast, _ctypes, resource, syslog and termios. The per-platform gates were the other half: fcntl is registered only on unix yet was advertised on Windows, and the wasm build advertised posix, time, _signal, pwd and grp, none of which it registers. Read the registry instead, the way get_builtin_module_names does, so the advertised set is the importable set under every cfg combination. Dotted keys are filtered out; they register submodules, not top-level modules. Assisted-by: Claude
Elsewhere they were registered with an empty dict, so `import pwd` and
`import grp` succeeded and handed back a module with no functions. Every
caller wraps the import in `except ImportError` and degrades when the
module is missing, so an empty module defeated the guard and turned the
fallback into an AttributeError on the next attribute access:
posixpath.expanduser('~') with HOME unset, getpass.getuser(), netrc's
uid lookup, http.server's nobody lookup, and subprocess's group=
parameter all raised instead. pathlib and tarfile test `pwd is None`,
which an empty module never satisfies either.
A host without the user/group database has no such module to import,
which is the state those callers are written against.
Assisted-by: Claude
A None entry marks a name as blocked: _find_and_load raises
ModuleNotFoundError('import of X halted; None in sys.modules') with .name
set, rather than searching, which is how code disables an import for
everything downstream of it. check_sys_modules cannot report the entry --
None is not a module it can hand back -- so the lookup fell through and
the module was loaded anyway.
Answer the sentinel before the search, so it also covers a name that was
never importable. Both references emit that exact message, so the new
synthetic fixture compares against them directly.
Assisted-by: Claude
_imp.create_builtin returned its argument, so BuiltinImporter.create_module handed back the spec and nothing could load through the machinery. It now resolves spec.name against sys.modules and falls back to load_builtin_module, raising ImportError for a name that is not a built-in. load_source_module calls _install and _install_external_importers once importlib._bootstrap finishes loading, which is what fills sys.meta_path and sys.path_hooks; before this they stayed empty and importlib.util.find_spec raised NameError. The bootstrap module is read back out of sys.modules rather than reused from the local, and _frozen_importlib is aliased onto it after both installs succeed, matching the alias absolute_import already maps for _frozen_importlib_external. Also drops the collection claim from the sys_modules_blocks ordering note: the order matches check_sys_modules, but the stated reason was not established. Assisted-by: Claude
_bootstrap_external._write_atomic publishes a file by writing a temporary and calling os.replace, so the name has to exist for that path to run. The host layer renames through std::fs::rename, which overwrites an existing destination on both platforms, so replace shares rename's body and the name only selects the text of the argument errors. Registered in the sandbox neutralisation list too: it mutates the host filesystem exactly as rename does. Guarded by bench/posix_replace_regression.py, which covers a fresh destination, an existing one and a missing source, and is skipped on wasm because the guest has no filesystem. Assisted-by: Claude
_bootstrap_external gates _path_join on sys.platform == 'win32', which pyre reports on Windows even though os.name stays posix, and that _path_join maps _os._path_splitroot over its parts. Without the name every path join in the finder raises AttributeError there. split_root returns what ntpath.splitroot returns with the drive and the root joined back together, which is the two-value shape _path_join reads: a root that starts or ends with a separator is absolute, one ending in ':' is drive-relative. Offsets are per character, so a leading multi-byte character followed by ':' still takes the drive branch. The function is compiled on every platform so its tests run everywhere; only the Windows build registers a caller. Assisted-by: Claude
The tree did not compile: three call sites were merged textually against definitions that had changed underneath them, which git cannot detect. typedef.rs carried two definitions of init_callable_iterator_type. The surviving one is the 3.14 shape that also installs __reduce__ and documents why; the older __iter__/__next__ pair is deleted. mro_subclasses called w_type_get_subclasses with one argument. Every other caller passes the flag, and the function documents itself as mirroring baseobjspace::mutated, which passes false. history.rs left extra_virtual_roots off a Snapshot initializer. The single-frame variant directly above it, which this one mirrors, sets it empty because the nested-list append fold resumes through the single-frame collapse. Assisted-by: Claude
The hasdict-instance getattr branch bound every class-attr callable with `w_method_new` before consulting `get`. A `BuiltinFunction` stored as a class attribute (`class T(tuple): f = len`) was therefore bound, so `T().f([1, 2])` passed the instance as an extra positional argument and raised. Route through `get` first, which returns a `BuiltinFunction` unbound, and bind only the plain-function residual the sibling branches already guard with `!is_builtin_code`. Assisted-by: Claude
os_error_with_errno discarded its message and built an OSError with no filename, so os.stat/os.chdir and the sandbox seam_os_err lost the path from both str(e) and .filename. Route the posix helpers and seam_os_err through os_error_syscall with the path as the filename, matching open(). Add os_error_syscall2 for the two-path syscalls and pass the destination through rename/replace so .filename2 and the " -> 'dest'" suffix survive. Assisted-by: Claude
SimpleNamespace construction evaluated self.__dict__ before touching the keyword set, so a receiver without an instance dict now raises AttributeError even for an empty kwargs. __eq__/__ne__ raise the arity TypeError on a missing argument instead of returning NotImplemented. Pin every __dict__ key and value on the shadow stack in __repr__ and address them by slot: a user __lt__, __str__ or __repr__ can collect, and the moving GC would relocate the raw item vector the sort held. Assisted-by: Claude
import_from held w_pkgname as a raw local across getattr_str(module, "__file__"), which can run a descriptor or __getattr__ and collect; pin it and read it back from the slot before building the ImportError. _imp.create_builtin allocated the module but skipped the sys.modules bind and the startup hook that load_part runs. Add create_builtin_module to perform all three steps so the two builtin-import entry points agree. Assisted-by: Claude
compile_source labelled str/bytes source `<pyre>`, so exec/eval report it as co_filename and as the SyntaxError.filename of a failed compile. Use `<string>`. Assisted-by: Claude
complex_component_spec dropped the PEP 682 negative-zero flag, so format(complex(-0.0, -0.0), 'z') left the components signed. Re-emit `z` into each per-component spec alongside the other repeated numeric flags. Assisted-by: Claude
Rename the unused range loop variable to `_` across eight benches, spell `lst * 1 is not lst` in seq_repeat_overflow, and rename the `callable` shadow to `fn` in set_method_arity. Assisted-by: Claude
_imp.create_builtin("builtins") ran load_builtin_module, which reruns
install_default_builtins: a second exception hierarchy whose fresh types
overwrite the name->class registry. A frame still holding the earlier
builtins then caught `except KeyError` against a BaseException the
registry no longer points at, raising "catching classes that do not
inherit from BaseException". Route the name to EC.get_builtin() as
load_part already does, so exactly one builtins hierarchy exists.
Assisted-by: Claude
The rebase re-applied the initializer that assigns the removed `Snapshot.extra_virtual_roots` field, which no longer compiles. Remove the assignment. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ca25e3e16
ℹ️ 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".
| /// hierarchy across each `compute_mro` allocation, so the walk needs no | ||
| /// extra rooting (mirrors `baseobjspace::mutated`). | ||
| unsafe fn mro_subclasses(w_type: PyObjectRef) { | ||
| let mro = crate::baseobjspace::compute_mro(w_type); |
There was a problem hiding this comment.
When the type being reassigned, or any affected subclass, has a metaclass mro() override, this calls the default C3 helper directly instead of the PyPy compute_mro path that dispatches and validates the metaclass hook (pypy/objspace/std/typeobject.py:1090-1096 and 1595-1606). After A.__bases__ = ..., a subclass with a custom mro() can have its __mro__ overwritten with default C3 (and a raising hook cannot abort/rollback the assignment), so subsequent lookup and super() behavior diverge from the class's metaclass semantics; use the existing compute_and_set_mro-style path and propagate failures while walking subclasses.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
A batch of objspace behavior gaps where pyre diverged from CPython 3.14, each
found by a three-way sweep (pyre-dynasm vs CPython 3.14 vs PyPy 3.11) and fixed
to match CPython, with a synth parity bench per fix. Benches compare exception
type / value only where CPython and PyPy message text differs.
pyre/check.pygreen on all three backends.
Attribute lookup
object_getattr_missreturned a class-MRO attribute without consulting theinstance dict, so a heap subclass of
tuple/int/str/… could not shadow asame-named non-data-descriptor class attribute with an instance attribute.
For a
hasdicttype it now runs theobject.__getattribute__protocol (datadescriptor → instance dict → non-data / class var); exact builtins keep the
direct fast path. This also fixes
str.encode()accepting non-textcodecs:
CodecInfois atuplesubclass whose binary-codec instances set_is_text_encoding = False, which the class defaultTruewas masking — so"x".encode("hex"|"base64"|"zlib"|"rot13")now raisesLookupError.bool()
__bool__/__len__was dispatched through the raw call path, whosestashed error was dropped; control fell through to
is_true, which raised asecond copy while the first leaked and resurfaced on a later unrelated opcode.
Both calls now go through
call_and_check, propagating the error immediately.Hashing
hash(float('nan'))returned theHASH_NANconstant (0, all NaNs collide);a NaN now hashes by object identity.
tuple/frozensetsubclass setting__hash__ = Nonereached thestructural hash fast path and hashed by contents; it now consults the
__hash__slot (None → unhashable, override → dispatch, inherited → fastpath), for
hash()and set insertion alike.Numeric coercion
int(x)whose__int__returns a non-int raisedRuntimeError; nowTypeError"int returned non-int (type X)" like the__index__path.complex(x)for a value with only__index__(no__complex__/__float__)raised "must be real number"; it now falls back to
__index__likefloat().Sequences
[2**63, 2**64)raisedMemoryError; a count thatoverflows the signed machine word now raises
OverflowErrorlike the__index__path.tuple * 1/bytes * 1return the receiver for an exactimmutable type (subclass → fresh base;
list/bytearraycopy), matchingstr.Containers
NaNin an all-float list was missed by membership,index/count/remove, and list/tuple==/<=, because the float strategy storeselements unboxed and so loses the shared-object identity
eq_w'sis_wshort-circuit relies on.
eq_wnow applies the floatis_wbit-patterncheck and the
FloatListStrategyfind fast path matches a NaN needle by bitpattern, so
nan in [nan]/[nan] == [nan]hold (matching PyPy) whilescalar
nan == nanstaysFalse.Iteration
enumerate(iterable, start)with astartpast i64 raisedOverflowError;it now activates the bigint index slot and accepts arbitrary precision.
reversed(obj)with__reversed__ = Nonereturned a corrupt null object; itnow raises "not reversible", and a raised
__reversed__propagates.iter(callable, sentinel)produced an object whose type was unregistered(
type(it)broken,isinstance(it, object)false); thecallable_iteratortype is now registered with
__iter__/__next__.set / frozenset method arity
symmetric_difference[_update],issubset,issuperset,isdisjoint,add,discard,remove(exactly one argument) andpop,clear,copy(no arguments) now raise
TypeErroron the wrong positional-argument count.The variadic
union/intersection/difference/updateare unchanged.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
aiter()andanext()support for asynchronous iteration.types.SimpleNamespacebehavior, including structured representation, comparison, and keyword initialization.zformatting support for suppressing negative zero.Bug Fixes
SyntaxError,ImportError,OSError, and traceback details with more accurate locations, fields, messages, and formatting.Tests