Repository navigation
Port time, datetime, and descriptor semantics - #654
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:
WalkthroughThe pull request updates Pyre’s object model, descriptor and class-construction behavior, special-method dispatch, native float/complex layouts, time APIs, CPython test execution, and compatibility baselines. ChangesNative object model
Time APIs and compatibility tests
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1ac6608 to
f989b1e
Compare
youknowone
changed the title
f989b1e to
96090ed
Compare
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit d94e6c3). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
![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: 96090ed9cc
ℹ️ 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".
| #[cfg(unix)] | ||
| { | ||
| // PyPy `Module.startup` calls `_init_timezone`, and exposes | ||
| // `tzset` on every POSIX build. | ||
| t::init_timezone(ns); |
There was a problem hiding this comment.
In Unix builds with --features sandbox, this unconditional block runs init_timezone while importing time, before the timezone-dependent functions are replaced with stubs. That function calls the host_env localtime implementation twice and exposes the host's timezone attributes directly, bypassing the controller even though the following sandbox block explicitly treats local timezone state as unavailable; guard this initialization from sandbox builds or obtain the values through a mediated operation.
Useful? React with 👍 / 👎.
| let result = rustpython_wtf8::Wtf8Buf::from_bytes(rendered.clone()).map_err(|_| { | ||
| crate::typedef::unicode_decode_error( | ||
| "utf-8", | ||
| &rendered, | ||
| 0, | ||
| rendered.len(), | ||
| "invalid UTF-8 returned by strftime", |
There was a problem hiding this comment.
When LC_TIME uses a non-UTF-8 encoding and a directive emits non-ASCII text (for example a localized month name in an ISO-8859 locale), libc returns bytes in that locale, so parsing them directly as WTF-8 raises UnicodeDecodeError. The corresponding PyPy path uses str_decode_locale_surrogateescape, and this implementation should likewise decode according to the active locale rather than require UTF-8.
AGENTS.md reference: AGENTS.md:L194-L196
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: 2
🤖 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/time/interp_time.rs`:
- Around line 1092-1107: Update the `strftime` result handling in the loop
around `render_segment` and `Wtf8Buf::from_bytes` to decode libc output with
surrogateescape semantics. Preserve valid UTF-8 while converting each invalid
byte into its corresponding lone surrogate, and return the resulting Unicode
value directly instead of raising `unicode_decode_error` when `from_bytes`
rejects the data.
- Around line 849-881: Update _get_seconds to floor fractional seconds before
converting them to time_t, matching CPython’s _PyTime_ROUND_FLOOR behavior so
negative fractions such as -0.5 become -1. Apply the existing loss-of-precision
validation against the floored value while preserving current NaN, range, and
absent/None handling.
🪄 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 Plus
Run ID: 3d7340f2-334d-413a-a262-3e3b49654d4b
📒 Files selected for processing (5)
pyre/cpython_tests/baseline.jsonpyre/extra_tests/snippets/stdlib_time.pypyre/pyre-interpreter/src/module/_sre/interp_sre.rspyre/pyre-interpreter/src/module/time/interp_time.rspyre/pyre-interpreter/src/module/time/mod.rs
96090ed to
ba41f84
Compare
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/ba41f849a29532cac0e067512030006a82258a15/pyre-interpreter/src/module/time/mod.rs#L115-L118
On Unix builds with --features sandbox, this registers the real t::tzset, while the sandbox replacement loop below only stubs localtime, mktime, ctime, and strftime. Calling time.tzset() therefore executes the raw extern "C" tzset and then init_timezone, bypassing the controller and potentially exposing host timezone state or triggering a seccomp-denied timezone-file access; guard this registration or replace tzset with the sandbox stub as well.
ℹ️ 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".
ba41f84 to
72b6316
Compare
youknowone
changed the title
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/72b63164110e14c6b165b8892527bb7e821bbebc/pyre-interpreter/src/eval.rs#L4104-L4108
When a with/async with operation becomes JIT-compiled and the instance shadows __enter__/__exit__ or overrides attribute access, this new interpreter path correctly looks only on the type, but the inspected JIT lowering still calls bh_load_special_fn in pyre-jit/src/call_jit.rs:5246-5271, which falls back to getattr_str and method-style binding. The same bytecode can therefore select a different callable after it gets hot; update the LOAD_SPECIAL JIT helpers/lowering to preserve this type-only descriptor lookup.
AGENTS.md reference: AGENTS.md:L14-L19
https://github.com/youknowone/pyre/blob/72b63164110e14c6b165b8892527bb7e821bbebc/pyre-interpreter/src/module/time/mod.rs#L115-L118
In Unix sandbox builds, this registration leaves time.tzset() callable even though the surrounding sandbox block stubs all other local-time operations. Separate from the already-noted import-time initialization, the callable's implementation at interp_time.rs:730-738 declares and invokes libc tzset directly, bypassing host_seam to reread host timezone state and mutate process-global libc state; omit it under feature = "sandbox" or route it through a mediated controller operation.
ℹ️ 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: 15
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/type_methods.rs (2)
1024-1039: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRoot
args[0]around the strip cut.
strip_charsreturns a slice borrowed from the receiver’sWtf8Buf, butw_str_cutthen copies/allocates from it. Add one root forargs[0]before this path instead of relying onw_str_cutargument evaluation order; this also applies to the split paths at 868-888 and 942-962.🤖 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/type_methods.rs` around lines 1024 - 1039, Root or otherwise retain args[0] before invoking w_str_cut, ensuring the Wtf8 slice returned by strip_chars remains valid while it is copied or allocated. Apply the same rooting pattern to the split paths around the referenced ranges, rather than relying on argument evaluation order.
782-787: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
with_capacitytakes bytes, not code points.
cps.len()is a code-point count, so any non-ASCII run under-reserves and reallocates during the push loop. Reservingcps.len() * 4, or summingcp.len_wtf8(), avoids that.🤖 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/type_methods.rs` around lines 782 - 787, Update cps_to_str_cut so Wtf8Buf::with_capacity reserves UTF-8 byte capacity rather than the code-point count; compute the required capacity by summing each CodePoint’s len_wtf8() (or conservatively reserving cps.len() * 4) before the push loop, while preserving the existing w_str_cut call.pyre/pyre-interpreter/src/baseobjspace.rs (1)
7745-7768: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
memoryview_check_releasedearly-returns without releasing the acquired view.Line 7753 propagates with
?, but the exporter's__buffer__has already run andw_viewis pinned — the very next branch (Lines 7754-7768) is careful to construct aSimpleBufferBytesandrelease()it before erroring. Take the same path here, otherwise an exporter that returns an already-released memoryview leaves its export count permanently elevated (abytearrayin that state can never be resized again). The non-memoryviewTypeErrorat Line 7746 has the same shape —w_viewis not yet pinned there, so the exporter's release hook is never invoked either.🛡️ Proposed fix
pyre_object::gc_roots::pin_root(w_view); let view_slot = pyre_object::gc_roots::shadow_stack_len() - 1; let r_view = pyre_object::gc_roots::shadow_stack_get(view_slot); - crate::builtins::memoryview_check_released(r_view)?; - if !crate::builtins::memoryview_contiguity(r_view).0 { - let buffer = SimpleBufferBytes { - _roots: roots, - data: Vec::new(), - native_owner_slot: None, - native_export_active: false, - release_exporter_slot: Some(obj_slot), - release_view_slot: Some(view_slot), - }; - buffer.release(); - return Err(PyError::new( - PyErrorKind::BufferError, - "memoryview: underlying buffer is not C-contiguous", - )); - } + let mut fail = |roots| { + SimpleBufferBytes { + _roots: roots, + data: Vec::new(), + native_owner_slot: None, + native_export_active: false, + release_exporter_slot: Some(obj_slot), + release_view_slot: Some(view_slot), + } + .release() + }; + if let Err(error) = crate::builtins::memoryview_check_released(r_view) { + fail(roots); + return Err(error); + } + if !crate::builtins::memoryview_contiguity(r_view).0 { + fail(roots); + return Err(PyError::new( + PyErrorKind::BufferError, + "memoryview: underlying buffer is not C-contiguous", + )); + }🤖 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 7745 - 7768, Update the buffer-export handling around memoryview_check_released to release the acquired exporter/view state before propagating a released-memoryview error, using the same SimpleBufferBytes construction and release path as the non-contiguous branch. Also ensure the non-memoryview TypeError path invokes the exporter release hook even though w_view is not yet pinned, while preserving the existing error types and messages.
🤖 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/baseobjspace.rs`:
- Around line 4197-4224: Prevent the float and complex dictionary paths in
baseobjspace.rs around lines 4197-4224 from dereferencing obj after allocation
without rooting or reloading it; reuse the neighboring bytes-arm
push_roots/pin_root/shadow_stack_get pattern, or explicitly document the proven
non-moving guarantee. Also document that invariant at the w_slots stores in
pyre/pyre-object/src/complexobject.rs lines 99-111 and
pyre/pyre-object/src/floatobject.rs lines 116-128, covering both stable
allocation and malloc_typed paths.
- Around line 4695-4719: Move the __dict__ narrowing block in the default
attribute lookup path below the module_getattribute_if_not_from_default and
getattribute_if_not_from_object gates, or execute it only when those gates
confirm the default __getattribute__ slot. Preserve raw module-dict handling for
default lookup while ensuring user-defined __getattribute__ overrides receive
m.__dict__ first.
- Around line 6174-6185: Update the "__dict__" handling in the relevant
attribute lookup flow so the metaclass data-descriptor walk runs first,
preserving overrides such as a Meta.__dict__ property; during that walk, ignore
only owners whose layout typedef is INSTANCE_TYPE, matching getattr_str_impl.
For a null result from w_type_get_dict_ptr, replace the fresh throwaway dict
proxy with the established debug assertion or error path rather than returning a
writable discarded mapping.
- Around line 9646-9649: Remove the early "__dict__" exception special case from
object_setattr so exception assignments proceed through the existing
lookup_in_type_where/set descriptor walk and reach the later "__dict__" handling
arm. Preserve the descriptor setter behavior for exception subclasses that
define their own "__dict__" data descriptor; if interception still fails,
investigate that walk rather than adding another early return.
- Around line 4411-4432: Extract the repeated DICT_TYPE resolution, isinstance_w
check, and error construction from setdict into a single
require_dict_for_setdict helper. Update the float and complex branches, along
with the existing staticmethod, classmethod, bytes, and bytearray branches, to
call this helper while preserving subclass acceptance and the current error
message.
In `@pyre/pyre-interpreter/src/eval.rs`:
- Around line 4100-4121: Update PyFrame::load_special to restore the [exit_func,
exit_self] split expected by with_except_start: extract the function and
receiver from bound Method results, while preserving the receiver information
for non-function descriptors whose __get__ returns a callable. Ensure
WITH_EXCEPT_START can invoke property or custom __exit__ descriptors for both
with and async with without treating them as unbound.
In `@pyre/pyre-interpreter/src/module/_sre/interp_sre.rs`:
- Around line 821-841: Update replacement accumulation around finish_output to
build the Subject::Str result with Wtf8Buf push/builder APIs rather than
concatenating raw Vec<u8> fragments. Ensure matched slices, literal
replacements, and callable string results are appended as WTF-8 fragments so
adjacent surrogate halves merge correctly, while preserving the existing
byte-output path and returning the validated Wtf8Buf without panic-based
fallback.
In `@pyre/pyre-interpreter/src/type_methods.rs`:
- Around line 5418-5449: Extract the shared `__dict_data__` slot traversal into
an unsafe helper such as `dict_data_slot_index`, including the layout walk,
base-slot calculation, and `Option<u32>` result. Update both `set_dict_backing`
and `resolve_dict_backing` to call this helper, preserving their existing
write/read behavior while removing duplicated slot-index logic.
In `@pyre/pyre-interpreter/src/typedef.rs`:
- Around line 11909-11937: Remove the later unconditional "__doc__" registration
in init_member_descriptor_type that uses the doc_getter returning None. Preserve
the earlier getset descriptor backed by w_member_get_doc so native member
descriptors expose their stored documentation while Python-level slots retain
their existing behavior.
- Around line 9479-9528: Revert the custom `doc_getter`, `doc_setter`,
`doc_deleter`, and `make_getset_property` registration for `type.__doc__` in the
surrounding type initialization. Preserve the existing `baseobjspace::get`
handling, which should apply the `TYPE_DOC` bypass consistently with exact
builtin types such as `function`, `method`, and `property`.
- Around line 2948-2952: Replace the debug-only check of the installed result
from set_dict_backing in the dict-subclass construction path with an
unconditional failure when the backing cannot be installed. Preserve the
existing invariant message and ensure production builds surface the layout
violation immediately rather than returning an instance without dict backing.
- Around line 11205-11225: Remove the redundant
patch_fixed_code_descriptor_getsets function and its invocation, leaving
stamp_new_descr_self() as the sole owner of stamping getset descriptor receiver
types for METHOD_DESCRIPTOR_TYPE and SLOT_WRAPPER_TYPE. Do not alter the
existing type-construction stamping behavior.
In `@pyre/pyre-jit/src/eval.rs`:
- Around line 1227-1235: Update the preceding comment for the
W_IntObject/W_FloatObject registrations to accurately describe W_FloatObject as
a three-edge traced type, including w_class, w_dict, and w_slots. Add the
rationale used by the W_UnicodeObject registration, specifically that tracing
w_slots delegates slot-value ownership to the list’s custom tracer; leave the
registration code unchanged.
- Around line 2373-2381: The comments documenting both GC registrations are
stale: in pyre/pyre-jit/src/eval.rs lines 2373-2381, update the comment near the
W_ComplexObject registration to describe its three traced edges and explain that
the w_slots list edge delegates slot-value tracing to the list custom tracer; in
lines 1227-1235, revise the W_FloatObject comment so it is no longer grouped
with W_IntObject as a bare NewWithVtable leaf, mirroring the rationale used for
W_UnicodeObject. No registration code changes are needed.
In `@pyre/pyre-object/src/floatobject.rs`:
- Around line 107-141: Create shared slot helpers for list-backed get, set, and
delete behavior, including null initialization, growth/padding, item updates,
and the GC write barrier. Move the duplicated logic from
w_float_slot_get/set/del, w_complex_slot_get/set/del, and w_str_slot_* into the
shared module, then make each type-specific function a thin forwarder passing
its w_slots field and owner object.
---
Outside diff comments:
In `@pyre/pyre-interpreter/src/baseobjspace.rs`:
- Around line 7745-7768: Update the buffer-export handling around
memoryview_check_released to release the acquired exporter/view state before
propagating a released-memoryview error, using the same SimpleBufferBytes
construction and release path as the non-contiguous branch. Also ensure the
non-memoryview TypeError path invokes the exporter release hook even though
w_view is not yet pinned, while preserving the existing error types and
messages.
In `@pyre/pyre-interpreter/src/type_methods.rs`:
- Around line 1024-1039: Root or otherwise retain args[0] before invoking
w_str_cut, ensuring the Wtf8 slice returned by strip_chars remains valid while
it is copied or allocated. Apply the same rooting pattern to the split paths
around the referenced ranges, rather than relying on argument evaluation order.
- Around line 782-787: Update cps_to_str_cut so Wtf8Buf::with_capacity reserves
UTF-8 byte capacity rather than the code-point count; compute the required
capacity by summing each CodePoint’s len_wtf8() (or conservatively reserving
cps.len() * 4) before the push loop, while preserving the existing w_str_cut
call.
🪄 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 Plus
Run ID: 1ac04931-d8f0-437d-8e15-96daa158f01c
📒 Files selected for processing (26)
pyre/cpython_tests/baseline.jsonpyre/extra_tests/snippets/stdlib_time.pypyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/call.rspyre/pyre-interpreter/src/display.rspyre/pyre-interpreter/src/eval.rspyre/pyre-interpreter/src/function.rspyre/pyre-interpreter/src/lib.rspyre/pyre-interpreter/src/module/_sre/interp_sre.rspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-interpreter/src/module/time/interp_time.rspyre/pyre-interpreter/src/module/time/mod.rspyre/pyre-interpreter/src/objspace/descroperation.rspyre/pyre-interpreter/src/objspace/std/formatting.rspyre/pyre-interpreter/src/objspace/std/mapdict.rspyre/pyre-interpreter/src/opcode_ops.rspyre/pyre-interpreter/src/pyopcode.rspyre/pyre-interpreter/src/reduce_protocol.rspyre/pyre-interpreter/src/type_methods.rspyre/pyre-interpreter/src/typedef.rspyre/pyre-jit/src/eval.rspyre/pyre-object/src/complexobject.rspyre/pyre-object/src/floatobject.rspyre/pyre-object/src/typedef.rspyre/pyre-object/src/typeobject.rs
| /// `method_descriptor` and `wrapper_descriptor` share PyPy's | ||
| /// FunctionWithFixedCode getsets, but each public descriptor type is its own | ||
| /// required receiver under CPython's descriptor surface. | ||
| fn patch_fixed_code_descriptor_getsets() { | ||
| for pytype in [ | ||
| &crate::METHOD_DESCRIPTOR_TYPE as *const PyType, | ||
| &crate::SLOT_WRAPPER_TYPE as *const PyType, | ||
| ] { | ||
| let w_type = gettypefor(pytype).map_or(pyre_object::PY_NULL, |p| p.as_ptr()); | ||
| if w_type.is_null() || !crate::type_dict_has_storage(w_type) { | ||
| continue; | ||
| } | ||
| let ns = unsafe { pyre_object::w_type_get_dict_ptr(w_type) } as PyObjectRef; | ||
| for (_, descr) in unsafe { pyre_object::w_dict_items(ns) } { | ||
| if unsafe { pyre_object::typedef::is_getset_property(descr) } { | ||
| unsafe { pyre_object::typedef::w_getset_set_reqcls(descr, w_type) }; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether anything inserts additional getset entries into
# method_descriptor_type / slot_wrapper_type's namespace after their initial
# construction (which would make patch_fixed_code_descriptor_getsets non-redundant).
rg -n 'METHOD_DESCRIPTOR_TYPE|SLOT_WRAPPER_TYPE' pyre/pyre-interpreter/src -g '*.rs' -A2 -B2Repository: youknowone/pyre
Length of output: 6564
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant typedef implementations around stamp_new_descr_self,
# new_typeobject_with_base_and_layout, and the added patch, plus usages in lib.
sed -n '1720,1795p' pyre/pyre-interpreter/src/typedef.rs
printf '\n--- typedef around patch_fixed_code_descriptor_getsets ---\n'
sed -n '11175,11240p' pyre/pyre-interpreter/src/typedef.rs
printf '\n--- calls to patch_fixed_code_descriptor_getsets and related functions in typedef/lib ---\n'
rg -n 'stamp_new_descr_self|new_typeobject_with_base_and_layout|patch_fixed_code_descriptor_getsets|copy_for_type' pyre/pyre-interpreter/src/typedef.rs pyre/pyre-interpreter/src/lib.rs -A4 -B4
printf '\n--- precise METHOD/SLOT usage ---\n'
rg -n 'METHOD_DESCRIPTOR_TYPE|SLOT_WRAPPER_TYPE' pyre/pyre-interpreter/src/function.rs pyre/pyre-interpreter/src/typedef.rs pyre/pyre-interpreter/src/lib.rs -A5 -B5Repository: youknowone/pyre
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- PATCH_TYPEOBJECTS call path and timing ---'
rg -n 'PATCH_TYPEOBJECTS|run.*patch|init_pyre|interpreter|start' pyre/pyre-interpreter/src -g '*.rs' -A4 -B4
printf '%s\n' '--- init function definitions mentioning method_descriptor / wrapper_descriptor ---'
rg -n 'init_.*function|function::SLOT_WRAPPER_TYPE|function::METHOD_DESCRIPTOR_TYPE|wrapper_descriptor|method_descriptor|w_typeobject' pyre/pyre-interpreter/src/typedef.rs -A3 -B3
printf '%s\n' '--- PyPy source references for typematerialized descr handling (if vendored/local) ---'
rg -n 'function_set_objclass|Function.set_objclass|FunctionWithFixedCode' pyre/pyre-interpreter/src -g '*.rs' -A2 -B2Repository: youknowone/pyre
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- exact PATCH_TYPEOBJECTS definition and call sites ---'
rg -n 'PATCH_TYPEOBJECTS|\.with_init\(|after.*init|init|interpreter' pyre/pyre-interpreter/src -g '*.rs' | rg 'PATCH_TYPEOBJECTS|init_pyre|run_main|main|interpreter' -A2 -B2
printf '%s\n' '--- focused file context for patched methods ---'
sed -n '1270,1315p' pyre/pyre-interpreter/src/typedef.rs
rg -n 'fn init_.*function|pub fn init_.*function|function::SLOT_WRAPPER_TYPE|function::METHOD_DESCRIPTOR_TYPE' pyre/pyre-interpreter/src/typedef.rs -A20 -B20Repository: youknowone/pyre
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- exact PATCH_TYPEOBJECTS definition and nearest call sites ---'
rg -n 'PATCH_TYPEOBJECTS|patch_typeobjects' pyre/pyre-interpreter/src -g '*.rs' --no-heading
printf '%s\n' '--- lines 1260-1325 typedef.rs ---'
sed -n '1260,1325p' pyre/pyre-interpreter/src/typedef.rs
printf '%s\n' '--- init_function_type implementations/declarations ---'
rg -n 'init_function_type|FUNCTION_TYPE|SLOT_WRAPPER_TYPE|METHOD_DESCRIPTOR_TYPE' pyre/pyre-interpreter/src -g '*.rs' -A20 -B20 | sed -n '1,260p'
printf '%s\n' '--- focused patch_fixed_code_descriptor_getsets vs stamp_new_descr_self call ordering ---'
python3 - <<'PY'
from pathlib import Path
p = Path("pyre/pyre-interpreter/src/typedef.rs")
s = p.read_text()
def idx(line): return s.count("\n", 0, s.find(line)) + 1
print("patch_fixed_code_descriptor_getsets:", idx("fn patch_fixed_code_descriptor_getsets()"))
print("PATCH_TYPEOBJECTS call:", idx("patch_fixed_code_descriptor_getsets();"))
print("init_function_type lines:", [idx(l) for l in ["fn init_function_type(", "pub fn init_function_type("]])
print("new_typeobject_with_base_and_layout lines:", [idx(l) for l in ["fn new_typeobject_with_base_and_layout(", "unsafe { stamp_new_descr_self(ns, type_obj) }"]])
PYRepository: youknowone/pyre
Length of output: 26337
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- function.rs init_function_type full body ---'
sed -n '10725,10795p' pyre/pyre-interpreter/src/function.rs
printf '%s\n' '--- locate registrations that actually run init_function_type ---'
rg -n 'function::init_function_type|init_function_type\(' pyre/pyre-interpreter/src -g '*.rs' -A30 -B30 | sed -n '1,260p'Repository: youknowone/pyre
Length of output: 5256
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- init_function_type_common body ---'
rg -n 'unsafe fn init_function_type_common|pub.*unsafe fn init_function_type_common|fn init_function_type_common|pub fn init_function_type_common' pyre/pyre-interpreter/src/function.rs -A220 -B10
printf '%s\n' '--- all init_function_type_common entries with line context ---'
rg -n 'init_function_type_common\(' pyre/pyre-interpreter/src -g '*.rs' -A40 -B40Repository: youknowone/pyre
Length of output: 192
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- all init_function_type_common occurrences ---'
rg -n 'init_function_type_common\(' pyre/pyre-interpreter/src -g '*.rs' -A50 -B50 | sed -n '1,260p'
printf '%s\n' '--- lines around function common def occurrences ---'
rg -n 'init_function_type_common' pyre/pyre-interpreter/src/function.rs -A140 -B20 | sed -n '1,300p'Repository: youknowone/pyre
Length of output: 22138
Skip the redundant method_descriptor/wrapper_descriptor getset patch.
stamp_new_descr_self() already stamps these dict entries during type construction, and there are no later type-dict inserts for these two types before patch_fixed_code_descriptor_getsets() runs. Remove the post-hoc pass to keep one canonical owner path.
🤖 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 11205 - 11225, Remove the
redundant patch_fixed_code_descriptor_getsets function and its invocation,
leaving stamp_new_descr_self() as the sole owner of stamping getset descriptor
receiver types for METHOD_DESCRIPTOR_TYPE and SLOT_WRAPPER_TYPE. Do not alter
the existing type-construction stamping behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/cpython_tests/baseline.json`:
- Line 282: Keep the `test.test_datetime` entry in the baseline as `TIMEOUT`
rather than marking it `PASS`, so the known timeout remains excluded from the
default passing gate until the test is verified to pass.
In `@pyre/cpython_tests/run.py`:
- Around line 200-205: Update the script-mode package branch in run_module() to
use _PKG_DRIVER instead of _DOTTED_DRIVER, so package execution invokes its
__main__.py. Keep _DOTTED_DRIVER only for dotted-identity file modules such as
test.test_descr.
🪄 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 Plus
Run ID: c0ac2929-0616-40a8-8087-6a4a252fbc15
📒 Files selected for processing (2)
pyre/cpython_tests/baseline.jsonpyre/cpython_tests/run.py
| }, | ||
| "test.test_datetime": { | ||
| "dynasm": "CRASH" | ||
| "dynasm": "PASS" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not mark test.test_datetime as PASS yet.
The PR objective states that test.test_datetime remains a timeout while the timezone sweep is optimized separately. Because PASS modules are executed by the default gate, this change turns the known timeout into a gating failure. Keep the baseline at TIMEOUT until the test actually passes.
🤖 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/cpython_tests/baseline.json` at line 282, Keep the `test.test_datetime`
entry in the baseline as `TIMEOUT` rather than marking it `PASS`, so the known
timeout remains excluded from the default passing gate until the test is
verified to pass.
| _DOTTED_DRIVER = ( | ||
| "import sys, unittest\n" | ||
| "mod = {module!r}\n" | ||
| "__import__(mod)\n" | ||
| "unittest.main(module=sys.modules[mod], argv=['pyre'], verbosity=2)\n" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Locate file"
git ls-files | rg '(^|/)pyre/cpython_tests/run\.py$|run\.py$' || true
echo "## File outline/size"
wc -l pyre/cpython_tests/run.py
sed -n '1,280p' pyre/cpython_tests/run.py | cat -n
echo "## Search for _DOTTED_DRIVER and _PACKAGE_DRIVER"
rg -n "_DOTTED_DRIVER|_PACKAGE_DRIVER|module_path|__main__|runpy|test\." pyre/cpython_tests/run.pyRepository: youknowone/pyre
Length of output: 18142
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Test package __main__ files (excluding hidden/internal)"
python3 - <<'PY'
from pathlib import Path
TESTDIR = Path("lib-python/3/test")
packages = sorted(d for d in TESTDIR.glob("test_*") if d.is_dir() and (d / "__init__.py").exists())
print(f"packages={len(packages)}")
for d in packages:
mm = d / "__main__.py"
print(f"{d.name}: __main__.py={'exists' if mm.exists() else 'MISSING'}; content=")
if mm.exists():
lines = mm.read_text(errors="replace").splitlines()
for i, line in enumerate(lines[:20], 1):
print(f" {i:02d}: {line}")
PY
echo "## Run an empty package-driver probe with the system interpreter"
python3 - <<'PY'
import subprocess, tempfile, pathlib, sys, os
driver = (
("import sys, unittest\n"
"mod = {module!r}\n"
"__import__(mod)\n"
"unittest.main(module=sys.modules[mod], argv=['pyre'], verbosity=2)\n").format(module=sys.modules[__name__].__package__ or __name__)
)
print("driver:")
lines=driver.splitlines()
print(f" lines={len(lines)}")
for i,line in enumerate(lines,1):
print(f" {i:02d}: {line}")
print("classify expected if rc=0 and no Ran/Failed: PASS")
PY
echo "## Search git history for previous package driver (last 50 commits touched file)"
git -C pyre/cpython_tests/log --oneline -n 80 -- run.py || true
git -C pyre/cpython_tests diff HEAD~20..HEAD -- run.py | sed -n '1,260p' || trueRepository: youknowone/pyre
Length of output: 7495
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Package __main__.py entrypoint behavior vs simple import + unittest.main"
python3 - <<'PY'
import subprocess, tempfile, pathlib, sys, os
packages = []
TESTDIR = pathlib.Path("lib-python/3/test")
for d in TESTDIR.glob("test_*"):
if d.is_dir() and (d / "__init__.py").exists():
packages.append(f"test.{d.name}")
# We can run these from a temporary sandbox cwd, using only the vendored stdlib test packages.
for mod in packages[:35]:
driver = (
f"import sys, unittest\n"
f"mod = {mod!r}\n"
f"__import__(mod)\n"
f"unittest.main(module=sys.modules[mod], argv=['pyre'], verbosity=2)\n"
)
with tempfile.TemporaryDirectory(prefix="pyre-cpytest-sandbox-") as wd:
driver_path = pathlib.Path(wd) / "_driver_pkg.py"
driver_path.write_text(driver)
env = os.environ.copy()
env["PYTHONPATH" if "PYTHONPATH" not in env else "PYTHONPATH"] = "."
proc = subprocess.run(
[sys.executable, str(driver_path)],
cwd=wd,
env=env,
capture_output=True,
text=True,
)
out = proc.stdout + proc.stderr
status = "PASS" if proc.returncode == 0 and ("Ran" in out or "OK" in out) else f"NOT PASS rc={proc.returncode}"
if status != "PASS":
print(f"\ud83d\udfe0 {mod}: {status}")
for line in out.splitlines()[:40]:
print(f" {line}")
else:
if ("SKIP" in out or "ok" not in out) and proc.returncode == 0:
print(f"\ud83d\udd35 {mod}: PASS but likely skipped; " + str(proc.returncode))
PYRepository: youknowone/pyre
Length of output: 401
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Package __main__.py entrypoint behavior vs simple import + unittest.main"
python3 - <<'PY'
import subprocess, tempfile, pathlib, sys, os, io, codecs
packages = []
TESTDIR = pathlib.Path("lib-python/3/test")
for d in TESTDIR.glob("test_*"):
if d.is_dir() and (d / "__init__.py").exists():
packages.append(f"test.{d.name}")
sys.stdout = io.TextIOWrapper(sys.stdout.buffer, encoding=sys.getfilesystemencoding(), errors="replace", line_buffering=True)
sys.stderr = io.TextIOWrapper(sys.stderr.buffer, encoding=sys.getfilesystemencoding(), errors="replace", line_buffering=True)
for mod in packages[:35]:
driver = (
f"import sys, unittest\n"
f"mod = {mod!r}\n"
f"__import__(mod)\n"
f"unittest.main(module=sys.modules[mod], argv=['pyre'], verbosity=2)\n"
)
with tempfile.TemporaryDirectory(prefix="pyre-cpytest-sandbox-") as wd:
driver_path = pathlib.Path(wd) / "_driver_pkg.py"
driver_path.write_text(driver)
env = os.environ.copy()
env["PYTHONPATH" if "PYTHONPATH" not in env else "PYTHONPATH"] = "."
proc = subprocess.run(
[sys.executable, str(driver_path)],
cwd=wd,
env=env,
capture_output=True,
text=True,
)
out = proc.stdout or "" + (proc.stderr or "")
status = "PASS" if proc.returncode == 0 and ("Ran" in out or "OK" in out) else f"NOT PASS rc={proc.returncode}"
if status != "PASS":
print(f"RECHECK {mod}: {status}")
for line in out.splitlines()[:60]:
print(f" {line}")
else:
if ("SKIP" in out or "ok" not in out) and proc.returncode == 0:
print(f"NOT OK PASS {mod}: passed without tests/skips rc={proc.returncode}")
PYRepository: youknowone/pyre
Length of output: 8264
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Search tests directory for __main__.py imports and entrypoints"
python3 - <<'PY'
from pathlib import Path
import ast
TESTDIR = Path("lib-python/3/test")
for p in sorted(TestDir.glob("**/__main__.py")):
pass
PY
echo "## Read existing package __init__/main patterns"
python3 - <<'PY'
from pathlib import Path
import subprocess
paths = []
for d in sorted(Path("lib-python/3/test").glob("test_*")):
if not d.is_dir() or not (d / "__init__.py").exists():
continue
mm = d / "__main__.py"
init = d / "__init__.py"
if mm.exists() or init.exists():
paths.extend([init, mm])
for p in paths:
if not p.exists():
continue
print(f"\n# {p.parts[-3]}/{p.parts[-2]}/{p.parts[-1]}")
lines = p.read_text(errors="replace").splitlines()
for i, line in enumerate(lines[:80], 1):
if any(marker in line for marker in ("load_tests", "unittest.main(", "if __name__")):
print(f"{i:03d}: {line}")
PYRepository: youknowone/pyre
Length of output: 333
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Focus on existing package drivers from current run.py"
sed -n '195,210p' pyre/cpython_tests/run.py | cat -n
echo "## Read relevant package __main__.py files"
for name in asyncio cext ctipec ctypes email import importlib interpreters json peg_generator pydoc pyrepl sqlite3 tools tomllib unittest zipfile zoneinfo asyncio dataclasses tz; do
p="lib-python/3/test/test_${name}"
if [ -f "${p}/__main__.py" ]; then
echo "===== ${p}/__main__.py ====="
cat -n "${p}/__main__.py" | sed -n '1,40p'
fi
done
echo "## Compare with __init__.py load_tests patterns"
python3 - <<'PY'
from pathlib import Path
for d in sorted(Path("lib-python/3/test").glob("test_*")):
if not d.is_dir() or not (d / "__init__.py").exists():
continue
mm = list(d.rglob("test_*.py")) + list(d.rglob("*.py"))
print(f"{d.name}: init_has_load_tests={'load_tests' in (d/'__init__.py').read_text(errors='replace')}")
PYRepository: youknowone/pyre
Length of output: 4840
Keep packages on the package-specific driver.
The branch for script-mode packages now uses _DOTTED_DRIVER at run_module() line 245, but that driver still only imports test.<name> and calls unittest.main(module=sys.modules[mod], ...); it does not run the package’s __main__.py. Use the previous package driver (_PKG_DRIVER) here, reserving _DOTTED_DRIVER for dotted-identity file modules such as test.test_descr.
🤖 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/cpython_tests/run.py` around lines 200 - 205, Update the script-mode
package branch in run_module() to use _PKG_DRIVER instead of _DOTTED_DRIVER, so
package execution invokes its __main__.py. Keep _DOTTED_DRIVER only for
dotted-identity file modules such as test.test_descr.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f25aac3e88
ℹ️ 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 let Some(w_sizeof) = unsafe { | ||
| crate::baseobjspace::lookup_in_type(w_type.as_ptr(), "__sizeof__") | ||
| } { | ||
| return unsafe { |
There was a problem hiding this comment.
When a user-defined __sizeof__ returns None, a float, or a negative integer, this branch returns that object verbatim. sys.getsizeof() must instead reject non-integer results with TypeError (or return the supplied default) and reject negative results with ValueError; otherwise callers can receive an invalid size of an arbitrary type. Validate and normalize the method result before returning it.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
| for index in 0..items.len() { | ||
| let key = pyre_object::gc_roots::shadow_stack_get(root_base + index * 2); | ||
| let value = pyre_object::gc_roots::shadow_stack_get(root_base + index * 2 + 1); | ||
| pyre_object::w_dict_store(dst, key, value); |
There was a problem hiding this comment.
When a type namespace contains a non-string key whose __hash__ has side effects or raises on a subsequent call, this store re-hashes the key while copying the dictionary, and the earlier scratch copy re-hashes it once more. A real dictionary copy preserves its cached hashes, whereas this unchecked store also swallows hash failures, so type('C', (), d) unexpectedly executes user code multiple additional times and silently continues after its exception. Copy the existing entries together with their cached hashes rather than reinserting each key.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
- time module: gate init_timezone/tzset registration behind not(sandbox) so a sandbox build keeps the UTC interplevel defaults. - time.strftime: decode libc output with surrogateescape (charp2uni) instead of rejecting non-UTF-8 locale bytes. - sys.getsizeof: reject a non-int __sizeof__ result with TypeError and a negative one with ValueError. - type.__new__: copy the assembled namespace with w_dict_copy so keys are not re-hashed while building the type dict. - float/complex __dict__ lazy-init: root and reload the receiver across w_dict_new, matching the bytes/bytearray arms. - member_descriptor.__doc__: drop the dead shadowing getter that returned None over the real member_get_doc. - dict subclass __new__: return a RuntimeError when the layout has no mapping payload slot instead of relying only on debug_assert. - jit: correct the W_FloatObject/W_ComplexObject GC-registration comments that still described them as pointer-free leaves. Assisted-by: Claude
- time.strftime: decode libc output as canonical WTF-8 first (Wtf8Buf::from_bytes) and fall back to charp2uni surrogateescape only for non-WTF-8 bytes, so a lone-surrogate directive round-trips instead of being re-split into three surrogate escapes. - object.__getattr__ __dict__ miss: walk the metatype MRO and honor a genuine metatype __dict__ override (a data descriptor whose owner is a proper subclass of type); every owner chains its layout typedef to object's INSTANCE_TYPE, so type's own canonical getset is told apart by owner identity, not layout. Fall through to the canonical dict proxy otherwise, and surface a null canonical dict as an error. - object.__setattr__: document why the exception-instance __dict__ assignment is routed straight to setdict ahead of the descriptor walk (a plain base mixed into an exception contributes a layout-incompatible instance __dict__ getset the walk must not reach). - float/complex __slots__ storage: extract shared slot_get/set/del helpers (pyre-object/src/slots.rs) that root and reload the receiver across the list-growth safepoints, and delegate both types to them; w_float_subclass_new / w_complex_subclass_new initialize the w_dict and w_slots fields the shared struct carries. - setdict: fold the six duplicated dict-type validation blocks into require_dict_for_setdict. - type_methods: extract dict_data_slot for the shared dict-backing layout walk used by set_dict_backing and resolve_dict_backing. - method/wrapper descriptor types: install __get__ (descr_carrier_get) via init_descriptor_type_common. - _weakref proxy pow test: assert three-arg pow reaches __rpow__. Assisted-by: Claude
f25aac3 to
16e75d1
Compare
|
Second review round applied. Summary of how each finding was resolved. Fixed
Deliberately not changed, with reasons
Validation: — commented by Claude |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16e75d18a0
ℹ️ 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 descr = if w_type.is_null() { | ||
| None | ||
| } else { | ||
| unsafe { crate::baseobjspace::lookup_in_type(w_type, name) } |
There was a problem hiding this comment.
When a hot with or async with receiver overrides __getattribute__ for __enter__/__exit__, the interpreter now correctly bypasses that override through this type lookup, but the JIT frontend still lowers LOAD_SPECIAL to bh_load_special_fn (pyre/pyre-jit/src/call_jit.rs:5245-5253), which calls getattr_str and therefore invokes the instance override. Consequently, the same context manager can call different methods after tracing; update the JIT residual helpers and their binding shape to preserve this lookup.
AGENTS.md reference: AGENTS.md:L14-L19
Useful? React with 👍 / 👎.
| for (cls, old_mro) in temp.into_iter().rev() { | ||
| match old_mro { | ||
| Some(mro) => pyre_object::w_type_set_mro(cls, mro), | ||
| None => pyre_object::w_type_clear_mro(cls), | ||
| } |
There was a problem hiding this comment.
When a custom mro() performs a nested __bases__ assignment on a subclass and a later subclass recomputation raises, this loop unconditionally restores every visited class's old MRO because temp does not retain the newly installed MRO identity. PyPy's typeobject.py:1146-1148 restores each entry only while cls.mro_w is new_mro; checking only the root type's bases does not detect a nested change to another class, so this rollback can leave that subclass's MRO inconsistent with its newer bases. Record the installed MRO and condition each restoration on it still being current.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
| None => pyre_object::w_type_clear_mro(cls), | ||
| } | ||
| } | ||
| pyre_object::typeobject::w_type_set_bases(w_type, saved_bases); |
There was a problem hiding this comment.
When __bases__ reassignment enters a custom metaclass mro() that triggers GC and then raises, replacing the type's bases removes the old tuple's last traced owner while saved_bases remains only an untraced raw Rust pointer. The rollback then stores and iterates that potentially collected or forwarded tuple, risking memory corruption instead of restoring the hierarchy; pin the old bases before the swap and reload the rooted value during rollback.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyre/pyre-interpreter/src/module/time/interp_time.rs (1)
1234-1246: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
os_errorfor Windowsctimeconversion failures.Windows
_ctime64returningNULLrepresents the same platform-level conversion failure as the Unix_c_localtimeerror, so this path should returnPyError::os_error("unconvertible time")to match the current CPython behavior for invalid timestamps.🤖 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/time/interp_time.rs` around lines 1234 - 1246, The Windows `_ctime64` failure branch should return an OS error rather than a value error. Update the `p.is_null()` handling in the Windows `ctime` conversion path to use `PyError::os_error("unconvertible time")`, preserving the existing message and successful conversion behavior.
🤖 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/baseobjspace.rs`:
- Around line 4197-4240: Introduce a shared typed-getdict helper near the
existing dict-access logic to centralize type validation, has-dict checking,
existing-dict reuse, GC rooting/reload, lazy allocation, and setter invocation.
Replace the duplicated `bytes`, `bytearray`, `float`, and `complex` arms with
calls to this helper, passing each type’s predicate and getter/setter pair while
preserving their current behavior.
In `@pyre/pyre-interpreter/src/type_methods.rs`:
- Around line 5413-5423: Move the object-resident dict payload description from
the doc block before dict_data_slot to the set_dict_backing function it
describes, keeping the layout-slot operation and subclass behavior details
attached to set_dict_backing. Leave dict_data_slot documented only with its
slot-index purpose, None behavior, and safety requirements, and apply the same
correction to the related documentation at the indicated later lines.
In `@pyre/pyre-interpreter/src/typedef.rs`:
- Around line 1758-1765: Update the descriptor check in the getset-property
handling to compare type identity with std::ptr::eq against
gettypeobject(&crate::METHOD_DESCRIPTOR_TYPE) and
gettypeobject(&crate::SLOT_WRAPPER_TYPE), replacing the w_type_get_name string
comparisons. Preserve the existing w_getset_set_reqcls call and behavior for
only these concrete descriptor types.
- Around line 9927-9947: Update mro_subclasses and the surrounding rollback path
to root every w_type, saved MRO element, and saved_bases on the shadow stack
before compute_and_set_mro or other user-code execution. On rollback, reload all
rooted objects from the shadow stack before passing them to w_type_set_mro and
w_type_set_bases, including the saved_bases captured in the later error-handling
flow.
- Around line 9422-9442: Add an args.len() < 3 guard at the start of the
__setattr__ wrapper created by make_slot_wrapper_with_arity, before accessing
args[1] or args[2], and return the appropriate PyError::type_error for missing
arguments. Preserve the existing type, string-name validation, surrogate
handling, and object_setattr behavior for valid calls.
- Around line 9516-9526: Update the __doc__ deleter created by
make_builtin_function_with_arity to check w_type_is_heaptype(cls): delete the
__doc__ entry for heap types, and retain the immutable-type TypeError only for
static types. Match the setter’s heap-type behavior and preserve the existing
error wording for non-heap types.
In `@pyre/pyre-object/src/typedef.rs`:
- Around line 329-343: Update w_member_get_doc to remove the hardcoded
"real"/"imag" name matching and rely solely on the stored doc field for direct
member descriptors. Preserve the existing null check and return behavior so
descriptors with a stored document return it, while descriptors without one
return None.
---
Outside diff comments:
In `@pyre/pyre-interpreter/src/module/time/interp_time.rs`:
- Around line 1234-1246: The Windows `_ctime64` failure branch should return an
OS error rather than a value error. Update the `p.is_null()` handling in the
Windows `ctime` conversion path to use `PyError::os_error("unconvertible
time")`, preserving the existing message and successful conversion 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 Plus
Run ID: 03e184b0-7f6c-44b3-a086-24c41f1a874c
📒 Files selected for processing (30)
pyre/cpython_tests/baseline.jsonpyre/cpython_tests/run.pypyre/extra_tests/snippets/stdlib_time.pypyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/call.rspyre/pyre-interpreter/src/display.rspyre/pyre-interpreter/src/eval.rspyre/pyre-interpreter/src/function.rspyre/pyre-interpreter/src/lib.rspyre/pyre-interpreter/src/module/_sre/interp_sre.rspyre/pyre-interpreter/src/module/_weakref/interp__weakref.rspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-interpreter/src/module/time/interp_time.rspyre/pyre-interpreter/src/module/time/mod.rspyre/pyre-interpreter/src/objspace/descroperation.rspyre/pyre-interpreter/src/objspace/std/formatting.rspyre/pyre-interpreter/src/objspace/std/mapdict.rspyre/pyre-interpreter/src/opcode_ops.rspyre/pyre-interpreter/src/pyopcode.rspyre/pyre-interpreter/src/reduce_protocol.rspyre/pyre-interpreter/src/type_methods.rspyre/pyre-interpreter/src/typedef.rspyre/pyre-jit/src/eval.rspyre/pyre-object/src/complexobject.rspyre/pyre-object/src/floatobject.rspyre/pyre-object/src/lib.rspyre/pyre-object/src/slots.rspyre/pyre-object/src/typedef.rspyre/pyre-object/src/typeobject.rs
| /// Write pyre's object-resident stand-in for a dict subclass's intrinsic | ||
| /// `W_DictMultiObject` payload. This is a layout-slot operation, not Python | ||
| /// attribute assignment: a subclass may override `__setattr__` with a dict | ||
| /// method before its mapping payload exists. | ||
| /// Absolute layout-slot index of the reserved `__dict_data__` payload for | ||
| /// `obj`, walking the layout chain from the object's type toward its base. | ||
| /// `None` when no layout in the chain reserves the slot. | ||
| /// | ||
| /// # Safety | ||
| /// `obj` must be a valid, non-null pointer to a `PyObject`. | ||
| unsafe fn dict_data_slot(obj: PyObjectRef) -> Option<u32> { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Misplaced doc comment: the set_dict_backing description is attached to dict_data_slot.
Lines 5413-5416 document the layout-slot write performed by set_dict_backing, but they are the opening of the doc block on dict_data_slot (5417-5423), leaving set_dict_backing undocumented and rustdoc rendering both descriptions on the wrong item.
♻️ Proposed fix
-/// Write pyre's object-resident stand-in for a dict subclass's intrinsic
-/// `W_DictMultiObject` payload. This is a layout-slot operation, not Python
-/// attribute assignment: a subclass may override `__setattr__` with a dict
-/// method before its mapping payload exists.
/// Absolute layout-slot index of the reserved `__dict_data__` payload for
/// `obj`, walking the layout chain from the object's type toward its base.
/// `None` when no layout in the chain reserves the slot.
@@
+/// Write pyre's object-resident stand-in for a dict subclass's intrinsic
+/// `W_DictMultiObject` payload. This is a layout-slot operation, not Python
+/// attribute assignment: a subclass may override `__setattr__` with a dict
+/// method before its mapping payload exists.
pub fn set_dict_backing(obj: PyObjectRef, backing: PyObjectRef) -> bool {Also applies to: 5448-5449
🤖 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/type_methods.rs` around lines 5413 - 5423, Move the
object-resident dict payload description from the doc block before
dict_data_slot to the set_dict_backing function it describes, keeping the
layout-slot operation and subclass behavior details attached to
set_dict_backing. Leave dict_data_slot documented only with its slot-index
purpose, None behavior, and safety requirements, and apply the same correction
to the related documentation at the indicated later lines.
| let doc_deleter = make_builtin_function_with_arity( | ||
| "__doc__", | ||
| |args| { | ||
| let cls = args[1]; | ||
| Err(crate::PyError::type_error(format!( | ||
| "cannot delete '__doc__' attribute of immutable type '{}'", | ||
| unsafe { pyre_object::w_type_get_name(cls) }, | ||
| ))) | ||
| }, | ||
| 2, | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
__doc__ deleter rejects heap types with an "immutable type" message.
The setter correctly gates on w_type_is_heaptype, but the deleter raises unconditionally, so del C.__doc__ on a user class reports cannot delete '__doc__' attribute of immutable type 'C' — wrong on both counts (CPython 3.14 deletes the entry on a heap type and only refuses for static types).
🐛 Proposed fix
let doc_deleter = make_builtin_function_with_arity(
"__doc__",
|args| {
let cls = args[1];
- Err(crate::PyError::type_error(format!(
- "cannot delete '__doc__' attribute of immutable type '{}'",
- unsafe { pyre_object::w_type_get_name(cls) },
- )))
+ if !unsafe { pyre_object::w_type_is_heaptype(cls) } {
+ return Err(crate::PyError::type_error(format!(
+ "cannot delete '__doc__' attribute of immutable type '{}'",
+ unsafe { pyre_object::w_type_get_name(cls) },
+ )));
+ }
+ crate::type_dict_store(cls, "__doc__", pyre_object::w_none());
+ Ok(pyre_object::w_none())
},
2,
);🤖 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 9516 - 9526, Update the
__doc__ deleter created by make_builtin_function_with_arity to check
w_type_is_heaptype(cls): delete the __doc__ entry for heap types, and retain the
immutable-type TypeError only for static types. Match the setter’s heap-type
behavior and preserve the existing error wording for non-heap types.
| unsafe fn mro_subclasses( | ||
| w_type: PyObjectRef, | ||
| temp: &mut Vec<(PyObjectRef, Option<Vec<PyObjectRef>>)>, | ||
| ) -> crate::PyResult { | ||
| let bases = pyre_object::w_type_get_bases(w_type); | ||
| crate::baseobjspace::validate_c3_mro(bases)?; | ||
| let old_mro = pyre_object::w_type_get_mro(w_type); | ||
| temp.push(( | ||
| w_type, | ||
| if old_mro.is_null() { | ||
| None | ||
| } else { | ||
| Some((*old_mro).to_vec()) | ||
| }, | ||
| )); | ||
| crate::baseobjspace::compute_and_set_mro(w_type)?; | ||
| let subclasses = pyre_object::typeobject::w_type_get_subclasses(w_type, false); | ||
| for w_sc in subclasses { | ||
| mro_subclasses(w_sc, temp)?; | ||
| } | ||
| Ok(pyre_object::w_none()) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Rollback state (temp, saved_bases) is untraced across GC-safepointing user code.
mro_subclasses accumulates (w_type, old_mro) pairs into a plain Rust Vec and then calls compute_and_set_mro, which may invoke a metaclass mro() — arbitrary Python that allocates and can trigger a moving collection. compute_and_set_mro pins its own materialised vector for exactly this reason ("Pyre materializes an untraced Rust Vec, so pin every yielded class"), but the temp entries here, and saved_bases captured at Line 10084 and reused at Line 10112, are not rooted. On the error path those possibly-stale pointers are written straight back via w_type_set_mro / w_type_set_bases, corrupting the hierarchy that the rollback is supposed to restore.
Pin each w_type and each saved MRO element (plus saved_bases) on the shadow stack and reload them before the restore writes.
Also applies to: 10099-10132
🤖 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 9927 - 9947, Update
mro_subclasses and the surrounding rollback path to root every w_type, saved MRO
element, and saved_bases on the shadow stack before compute_and_set_mro or other
user-code execution. On rollback, reload all rooted objects from the shadow
stack before passing them to w_type_set_mro and w_type_set_bases, including the
saved_bases captured in the later error-handling flow.
| pub unsafe fn w_member_get_doc(obj: PyObjectRef) -> Option<&'static str> { | ||
| if unsafe { w_member_is_direct(obj) } { | ||
| match unsafe { w_member_get_name(obj) } { | ||
| "real" => return Some("the real part of a complex number"), | ||
| "imag" => return Some("the imaginary part of a complex number"), | ||
| _ => {} | ||
| } | ||
| } | ||
| let doc = unsafe { (*(obj as *const W_MemberDescr)).doc }; | ||
| if !doc.is_null() { | ||
| Some(unsafe { &*doc }) | ||
| } else { | ||
| None | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Hardcoded real/imag docs shadow the newly stored doc field — dead value, duplicate source of truth.
For a direct member named "real"/"imag", this always returns the hardcoded string and never reaches the doc field below it. But the interpreter's patch_complex_realimag_descriptors (pyre-interpreter typedef.rs) creates these same descriptors via w_member_new_with_doc(kind, name, complex_type, Some(doc.to_owned())), storing the identical doc string into that now-unreachable doc field. The stored value is allocated (via malloc_raw, leaked) but never read, and the two doc strings can silently diverge if one side is edited without the other.
Prefer a single source of truth: either drop the hardcoded name match here and rely solely on the doc field (already populated correctly for real/imag), or drop the Some(doc.to_owned()) argument on the interpreter side and keep this hardcoded fallback as the sole source.
♻️ Proposed fix: rely on the stored `doc` field only
pub unsafe fn w_member_get_doc(obj: PyObjectRef) -> Option<&'static str> {
- if unsafe { w_member_is_direct(obj) } {
- match unsafe { w_member_get_name(obj) } {
- "real" => return Some("the real part of a complex number"),
- "imag" => return Some("the imaginary part of a complex number"),
- _ => {}
- }
- }
let doc = unsafe { (*(obj as *const W_MemberDescr)).doc };
if !doc.is_null() {
Some(unsafe { &*doc })
} else {
None
}
}📝 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.
| pub unsafe fn w_member_get_doc(obj: PyObjectRef) -> Option<&'static str> { | |
| if unsafe { w_member_is_direct(obj) } { | |
| match unsafe { w_member_get_name(obj) } { | |
| "real" => return Some("the real part of a complex number"), | |
| "imag" => return Some("the imaginary part of a complex number"), | |
| _ => {} | |
| } | |
| } | |
| let doc = unsafe { (*(obj as *const W_MemberDescr)).doc }; | |
| if !doc.is_null() { | |
| Some(unsafe { &*doc }) | |
| } else { | |
| None | |
| } | |
| } | |
| pub unsafe fn w_member_get_doc(obj: PyObjectRef) -> Option<&'static str> { | |
| let doc = unsafe { (*(obj as *const W_MemberDescr)).doc }; | |
| if !doc.is_null() { | |
| Some(unsafe { &*doc }) | |
| } else { | |
| None | |
| } | |
| } |
🤖 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-object/src/typedef.rs` around lines 329 - 343, Update
w_member_get_doc to remove the hardcoded "real"/"imag" name matching and rely
solely on the stored doc field for direct member descriptors. Preserve the
existing null check and return behavior so descriptors with a stored document
return it, while descriptors without one return None.
- time module: gate init_timezone/tzset registration behind not(sandbox) so a sandbox build keeps the UTC interplevel defaults. - time.strftime: decode libc output with surrogateescape (charp2uni) instead of rejecting non-UTF-8 locale bytes. - sys.getsizeof: reject a non-int __sizeof__ result with TypeError and a negative one with ValueError. - type.__new__: copy the assembled namespace with w_dict_copy so keys are not re-hashed while building the type dict. - float/complex __dict__ lazy-init: root and reload the receiver across w_dict_new, matching the bytes/bytearray arms. - member_descriptor.__doc__: drop the dead shadowing getter that returned None over the real member_get_doc. - dict subclass __new__: return a RuntimeError when the layout has no mapping payload slot instead of relying only on debug_assert. - jit: correct the W_FloatObject/W_ComplexObject GC-registration comments that still described them as pointer-free leaves. Assisted-by: Claude
- time.strftime: decode libc output as canonical WTF-8 first (Wtf8Buf::from_bytes) and fall back to charp2uni surrogateescape only for non-WTF-8 bytes, so a lone-surrogate directive round-trips instead of being re-split into three surrogate escapes. - object.__getattr__ __dict__ miss: walk the metatype MRO and honor a genuine metatype __dict__ override (a data descriptor whose owner is a proper subclass of type); every owner chains its layout typedef to object's INSTANCE_TYPE, so type's own canonical getset is told apart by owner identity, not layout. Fall through to the canonical dict proxy otherwise, and surface a null canonical dict as an error. - object.__setattr__: document why the exception-instance __dict__ assignment is routed straight to setdict ahead of the descriptor walk (a plain base mixed into an exception contributes a layout-incompatible instance __dict__ getset the walk must not reach). - float/complex __slots__ storage: extract shared slot_get/set/del helpers (pyre-object/src/slots.rs) that root and reload the receiver across the list-growth safepoints, and delegate both types to them; w_float_subclass_new / w_complex_subclass_new initialize the w_dict and w_slots fields the shared struct carries. - setdict: fold the six duplicated dict-type validation blocks into require_dict_for_setdict. - type_methods: extract dict_data_slot for the shared dict-backing layout walk used by set_dict_backing and resolve_dict_backing. - method/wrapper descriptor types: install __get__ (descr_carrier_get) via init_descriptor_type_common. - _weakref proxy pow test: assert three-arg pow reaches __rpow__. Assisted-by: Claude
16e75d1 to
8c1f784
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c1f7841fb
ℹ️ 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 negative = crate::baseobjspace::is_true( | ||
| crate::objspace::descroperation::compare( | ||
| w_size, | ||
| pyre_object::w_int_new(0), |
There was a problem hiding this comment.
__sizeof__ result before validation
When a user-defined __sizeof__ returns a managed integer, constructing the zero operand can trigger a moving GC while w_size exists only as an untraced raw pointer; an overridden comparison can allocate again, and the stale pointer is then also returned on line 1471. Pin w_size before any allocation or Python dispatch and reload the rooted value for both comparison and return to avoid use-after-move or memory corruption.
Useful? React with 👍 / 👎.
| let owner = if cls_is_none { | ||
| r#type(w_obj).map_or(pyre_object::PY_NULL, |p| p.as_ptr()) | ||
| } else { | ||
| w_cls | ||
| }; | ||
| Ok(pyre_object::w_method_new(w_function, w_obj, owner)) |
There was a problem hiding this comment.
For a fixed-code method descriptor such as list.append, list.append.__get__(42, int) must raise TypeError immediately because 42 is not an instance of the descriptor's __objclass__. This branch creates a bound method for every non-None receiver without checking the stamped owner, so the invalid binding succeeds and only a later invocation may fail; type-check method and wrapper descriptor receivers before constructing the method.
Useful? React with 👍 / 👎.
| Err(err) | ||
| if err.kind == crate::PyErrorKind::TypeError | ||
| && err | ||
| .message | ||
| .starts_with("unsupported operand type(s) for ** or pow():") => |
There was a problem hiding this comment.
__pow__
When a user-defined __pow__ raises TypeError whose message happens to start with this text, x **= y rewrites that user exception into pyre's generic **= error. Operator fallback must distinguish the dispatcher's own unsupported-operands outcome structurally rather than classify arbitrary Python exceptions by their message.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
| fn init_descriptor_type_common(ns: PyObjectRef) { | ||
| init_function_type_common(ns); |
There was a problem hiding this comment.
Calling init_function_type_common installs user-function-only entries such as __code__, __globals__, __defaults__, __kwdefaults__, __annotations__, and __dict__ on both method_descriptor and wrapper_descriptor. Consequently objects such as list.append expose or attempt to service attributes that CPython's descriptor types do not have, which also misleads introspection; build a descriptor-specific namespace or remove the user-function entries before adding __get__.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 6
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)
15025-15048: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winStamp
float.real/float.imagwithW_FloatObject.
make_getset_descriptor(...)installs these as getsets withoutreqcls, sodict.__getitem__("real").__get__(non_float)calls the getter and reads arbitrary receiver memory throughw_float_get_value. Build the descriptor withW_FloatObjector add an explicitis_float(args[1])guard before the cast.🤖 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 15025 - 15048, Update the float.real and float.imag descriptor setup in the surrounding namespace initialization to stamp each getset with W_FloatObject, or otherwise validate args[1] with is_float before w_float_get_value is called. Ensure non-float receivers are rejected rather than allowing the real getter to read arbitrary memory, while preserving normal float attribute access.pyre/pyre-interpreter/src/builtins.rs (2)
11785-11789: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
pathand itsclone()are dead on unix builds.Every unix branch (sandbox and non-sandbox) now passes
&path_bytestohost_seam::ops::open;pathis only consumed by the#[cfg(all(not(feature = "sandbox"), not(unix)))]in-memory branch. As written, unix builds allocate anOsStringplus a fullVecclone of the path on everyopen()and never read it.Narrow the cfg to the consuming branch and drop the clone.
♻️ Proposed narrowing
- // `host_env::fs` takes a `Path`; only the seam consumes the raw bytes. - #[cfg(unix)] - let path = std::ffi::OsString::from_vec(path_bytes.clone()); - #[cfg(not(unix))] - let path = String::from_utf8_lossy(&path_bytes).into_owned(); + // Only the non-unix in-memory branch needs a `Path`-shaped value; the + // unix branches hand the raw bytes to the seam verbatim. + #[cfg(all(not(feature = "sandbox"), not(unix)))] + let path = String::from_utf8_lossy(&path_bytes).into_owned();🤖 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/builtins.rs` around lines 11785 - 11789, Remove the unconditional platform-specific `path` construction near the open flow, and define `path` only inside the `#[cfg(all(not(feature = "sandbox"), not(unix)))]` in-memory branch that consumes it. Eliminate the `path_bytes.clone()` allocation by constructing the non-Unix path directly from `path_bytes` there, while keeping Unix branches passing `&path_bytes` to `host_seam::ops::open`.
12669-12675: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve the execution context in the surrogate-name import fallback.
dunder_importrouteslevel > 0resolution into native helpers that read fromexecution_context, whiledunder_import_name_objonly passes the five Python imports. Addexec_ctxtodunder_import_name_objand forward it through any native fallback path it might reach so non-UTF-8 names have the same context-sensitive import semantics as UTF-8 names.🤖 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/builtins.rs` around lines 12669 - 12675, The surrogate-name fallback in dunder_import currently drops execution context. Update dunder_import_name_obj to accept exec_ctx, pass it from this fallback, and propagate it through every native fallback path that dunder_import_name_obj can reach so non-UTF-8 names preserve the same level-sensitive import behavior as UTF-8 names.
♻️ Duplicate comments (2)
pyre/pyre-interpreter/src/module/time/interp_time.rs (1)
849-880: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
_get_secondsstill truncates toward zero instead of flooring — same bug flagged previously, marked "addressed" but still present.
seconds as time_ttruncates toward zero, so-0.5becomes0, not-1. CPython'sparse_time_t_argsconverts via_PyTime_ObjectToTime_t(..., _PyTime_ROUND_FLOOR), which floors toward negative infinity. The subsequentdiffcheck (!(-1.0..=1.0).contains(&diff) || diff == -1.0 || diff == 1.0) is effectively dead code: truncation toward zero always yields|diff| < 1.0for any float within the already-checkedi64bounds, so this branch can never actually detect a precision-losing cast — it doesn't implement the "reject casts that would lose whole-second precision" behavior the comment above it claims.This is the exact code pattern from the earlier review round (that round's fix proposal floored
secondsbefore casting); the fix does not appear to have landed in this revision.🐛 Proposed fix: floor before truncating
- let result = seconds as time_t; - let diff = seconds - result as f64; - if !(-1.0..=1.0).contains(&diff) || diff == -1.0 || diff == 1.0 { + let floored = seconds.floor(); + let result = floored as time_t; + let diff = seconds - floored; + if !(0.0..1.0).contains(&diff) { return Err(crate::PyError::overflow_error( "timestamp out of range for platform time_t", )); }🤖 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/time/interp_time.rs` around lines 849 - 880, Update _get_seconds to floor fractional epoch values before converting them to time_t, so negative fractions such as -0.5 become -1 rather than truncating to zero. Replace the current truncation-based diff validation with logic that preserves the intended precision-loss/range checks after flooring, while keeping the existing None, NaN, and overflow handling.pyre/cpython_tests/baseline.json (1)
282-283: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
test.test_datetimedynasm baseline still set toPASS, contradicting the PR's own stated objective.Both the "Comments summary" and "Updated objectives summary" explicitly state the test remains a DynASM timeout baseline (e.g., "
test_datetimeis baselined as a DynASM timeout"), yet this entry marks itPASS. SincePASSentries are enforced by the default gate, this turns a known timeout into a gating requirement — the same concern raised on a previous revision of this file at this exact line.🐛 Proposed fix
"test.test_datetime": { - "dynasm": "PASS" + "dynasm": "TIMEOUT" },🤖 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/cpython_tests/baseline.json` around lines 282 - 283, Update the test.test_datetime entry in the dynasm baseline within baseline.json from PASS to the timeout status specified by the PR objectives, so the known DynASM timeout is not enforced as a passing-test gate.
🤖 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/call.rs`:
- Around line 2687-2703: Replace the inline unsafe w_class/type selection in the
_calculate_metaclass flow with the existing crate::typedef::r#type(base) helper,
preserving the existing None handling and subtype checks. Remove the duplicated
raw-field logic here and apply the same reuse in real_build_class.
- Around line 2687-2703: Extract the duplicated base-metaclass derivation into a
shared base_metaclass(base: PyObjectRef) -> Option<PyObjectRef> helper in
call.rs, preferring a non-null w_class for types and otherwise using
typedef::r#type(base).map(NonNull::as_ptr). At pyre/pyre-interpreter/src/call.rs
lines 2687-2703, replace the inline w_base_type block with this helper; at lines
3327-3337, replace the and_then closure body with base_metaclass(base) while
preserving the unwrap_or_else(crate::typedef::w_type) tail.
- Around line 4690-4699: Update the native_layout_conflict calculation in the
layout-selection logic so differing typedefs are considered conflicting only
when the layouts are incompatible sublayouts in both directions; preserve
compatibility whenever either layout’s issublayout relationship succeeds,
including derived native layouts, while retaining the INSTANCE_TYPE exclusions.
In `@pyre/pyre-interpreter/src/function.rs`:
- Around line 538-547: Update function_retag_method_descriptor to invoke the
repository’s established GC write-barrier helper immediately before assigning
the GC-managed w_class field, using obj as the owner and the method-descriptor
class value as the stored edge; keep the existing type retagging and class
assignment behavior unchanged.
In `@pyre/pyre-interpreter/src/opcode_ops.rs`:
- Around line 65-79: Define and reuse a shared constant for the
unsupported-operand error prefix emitted by descroperation.rs::pow and checked
in the BinaryOperator::InplacePower match. Replace the hardcoded starts_with
string with that constant so wording changes remain synchronized and the **=
rewrite continues to trigger correctly.
In `@pyre/pyre-object/src/typeobject.rs`:
- Around line 975-981: Update w_type_clear_mro to reset the type’s version_tag
to 0 whenever it clears mro_w, before the existing GC write barrier. Preserve
the null-MRO restoration behavior while ensuring subsequent cached MRO lookups
cannot reuse the previous tag.
---
Outside diff comments:
In `@pyre/pyre-interpreter/src/builtins.rs`:
- Around line 11785-11789: Remove the unconditional platform-specific `path`
construction near the open flow, and define `path` only inside the
`#[cfg(all(not(feature = "sandbox"), not(unix)))]` in-memory branch that
consumes it. Eliminate the `path_bytes.clone()` allocation by constructing the
non-Unix path directly from `path_bytes` there, while keeping Unix branches
passing `&path_bytes` to `host_seam::ops::open`.
- Around line 12669-12675: The surrogate-name fallback in dunder_import
currently drops execution context. Update dunder_import_name_obj to accept
exec_ctx, pass it from this fallback, and propagate it through every native
fallback path that dunder_import_name_obj can reach so non-UTF-8 names preserve
the same level-sensitive import behavior as UTF-8 names.
In `@pyre/pyre-interpreter/src/typedef.rs`:
- Around line 15025-15048: Update the float.real and float.imag descriptor setup
in the surrounding namespace initialization to stamp each getset with
W_FloatObject, or otherwise validate args[1] with is_float before
w_float_get_value is called. Ensure non-float receivers are rejected rather than
allowing the real getter to read arbitrary memory, while preserving normal float
attribute access.
---
Duplicate comments:
In `@pyre/cpython_tests/baseline.json`:
- Around line 282-283: Update the test.test_datetime entry in the dynasm
baseline within baseline.json from PASS to the timeout status specified by the
PR objectives, so the known DynASM timeout is not enforced as a passing-test
gate.
In `@pyre/pyre-interpreter/src/module/time/interp_time.rs`:
- Around line 849-880: Update _get_seconds to floor fractional epoch values
before converting them to time_t, so negative fractions such as -0.5 become -1
rather than truncating to zero. Replace the current truncation-based diff
validation with logic that preserves the intended precision-loss/range checks
after flooring, while keeping the existing None, NaN, and overflow handling.
🪄 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 Plus
Run ID: a3e923ec-29f5-481a-a565-0c6be56ce160
📒 Files selected for processing (30)
pyre/cpython_tests/baseline.jsonpyre/cpython_tests/run.pypyre/extra_tests/snippets/stdlib_time.pypyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/call.rspyre/pyre-interpreter/src/display.rspyre/pyre-interpreter/src/eval.rspyre/pyre-interpreter/src/function.rspyre/pyre-interpreter/src/lib.rspyre/pyre-interpreter/src/module/_weakref/interp__weakref.rspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-interpreter/src/module/time/interp_time.rspyre/pyre-interpreter/src/module/time/mod.rspyre/pyre-interpreter/src/objspace/descroperation.rspyre/pyre-interpreter/src/objspace/std/formatting.rspyre/pyre-interpreter/src/objspace/std/mapdict.rspyre/pyre-interpreter/src/opcode_ops.rspyre/pyre-interpreter/src/pyopcode.rspyre/pyre-interpreter/src/reduce_protocol.rspyre/pyre-interpreter/src/type_methods.rspyre/pyre-interpreter/src/typedef.rspyre/pyre-jit/src/call_jit.rspyre/pyre-jit/src/eval.rspyre/pyre-object/src/complexobject.rspyre/pyre-object/src/floatobject.rspyre/pyre-object/src/lib.rspyre/pyre-object/src/slots.rspyre/pyre-object/src/typedef.rspyre/pyre-object/src/typeobject.rs
| let native_layout_conflict = !layout.is_null() | ||
| && !std::ptr::eq((*best_layout).typedef, (*layout).typedef) | ||
| && !std::ptr::eq( | ||
| (*best_layout).typedef, | ||
| &pyre_object::pyobject::INSTANCE_TYPE, | ||
| ) | ||
| && !std::ptr::eq((*layout).typedef, &pyre_object::pyobject::INSTANCE_TYPE); | ||
| if !layout.is_null() | ||
| && (!(*best_layout).issublayout(layout) || native_layout_conflict) | ||
| { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Does any native type declare a typedef distinct from its base's while still
# forming a sublayout chain? If so the new check produces false positives.
ast-grep outline pyre/pyre-object/src/typeobject.rs --match 'Layout|issublayout' --view expanded
rg -nP --type=rust -C8 'fn issublayout' pyre/pyre-object/src
# Where are distinct typedefs assigned to layouts, and do native subclasses
# inherit the base typedef or get their own?
rg -nP --type=rust -C4 '\.typedef\s*[:=]' pyre/pyre-object/src | head -60
# Exception hierarchy typedefs specifically.
rg -nP --type=rust -C3 'INSTANCE_TYPE|BASE_EXCEPTION|EXCEPTION_TYPE' pyre/pyre-object/src/pyobject.rs | head -40Repository: youknowone/pyre
Length of output: 4812
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- call.rs relevant lines ---\n'
sed -n '4650,4715p' pyre/pyre-interpreter/src/call.rs
printf '\n--- typeobject.rs Layout definitions and conflicts ---\n'
sed -n '1,95p' pyre/pyre-object/src/typeobject.rs
sed -n '600,660p' pyre/pyre-object/src/typeobject.rs
printf '\n--- all native typedef declarations with bases ---\n'
rg -n --type=rust -C5 'PyType = new_pytype|static .*TYPE|typedef\s*[:=]\s*PyType' pyre/pyre-object/src/typeobject.rs pyre/pyre-object/src/pyobject.rs pyre/pyre-object/src/typedef.rs | head -240Repository: youknowone/pyre
Length of output: 244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- call.rs relevant lines ---'
sed -n '4650,4715p' pyre/pyre-interpreter/src/call.rs
echo
echo '--- typeobject.rs Layout definitions and methods ---'
sed -n '1,90p' pyre/pyre-object/src/typeobject.rs
sed -n '600,660p' pyre/pyre-object/src/typeobject.rs
echo
echo '--- native typedef declarations with bases ---'
rg -n --type=rust -C5 'PyType = new_pytype|static .*TYPE|typedef\s*[:=]\s*PyType' pyre/pyre-object/src/typeobject.rs pyre/pyre-object/src/pyobject.rs pyre/pyre-object/src/typedef.rs | head -240Repository: youknowone/pyre
Length of output: 15856
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- typeobject.rs layout construction / conflict implementation ---'
rg -n --type=rust -C8 'Layout|base_layout|issublayout|native_layout_conflict|layout conflicts|multiple inheritance|w_type_new' pyre/pyre-object/src/pyobject.rs pyre/pyre-object/src/typeobject.rs
echo
echo '--- all w_type_new_builtin / layout usages ---'
rg -n --type=rust -C5 'w_type_new_builtin|w_type_set_layout|w_type_get_layout_ptr|w_tuple_getitem|native_layout_conflict' pyre/pyre-interpreter/src/call.rs pyre/pyre-object/src pyre/jit/src | head -260Repository: youknowone/pyre
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- call.rs type creation layout/best base conflict slice ---'
sed -n '3340,3470p' pyre/pyre-interpreter/src/call.rs
echo
echo '--- pyobject.rs builtin type initialization and layout setup slice---'
sed -n '450,560p' pyre/pyre-object/src/pyobject.rs
sed -n '720,780p' pyre/pyre-object/src/pyobject.rs
echo
echo '--- typeobject.rs _setup_builtin_type and builtin layout slice ---'
sed -n '400,465p' pyre/pyre-object/src/typeobject.rs
sed -n '1360,1440p' pyre/pyre-object/src/typeobject.rs
echo
echo '--- search for native exception typedefs/base_layout layout setup ---'
rg -n --type=rust -C4 'OSError|BaseException|Exception|BASE_EXCEPTION|EXCEPTION_TYPE|instance layout|w_type_new_builtin|leak_layout.*Layout|struct Layout { |typedef = .*Exception|Exception.*Exception.*Exception' pyre/pyre-*.rs pyre/pyre-object/src/typeobject.rs pyre/pyre-object/src/pyobject.rs pyre/pyre-object/src/typedef.rsRepository: youknowone/pyre
Length of output: 14889
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- call.rs build_class_inner best base/layout conflict slice ---'
sed -n '3470,3545p' pyre/pyre-interpreter/src/call.rs
echo
echo '--- pyobject.rs exception pytype decl and pytypes entries slice ---'
rg -n --type=rust -C4 'EXCEPTION_TYPE|EXC_EXCEPTION_TYPE|BASE_EXCEPTION_TYPE|EXC_RANOUT_TYPE|Exception|BaseException' pyre/pyre-object/src/pyobject.rs pyre/python/pyre-jit/src pyre/jit/src 2>/dev/null | head -220
echo
echo '--- typeobject.rs builtin type setup and any native layout references ---'
rg -n --type=rust -C5 'all_foreign_pytypes|foreign|setup.*builtin|builtin_type|w_type_new_builtin|leak_layout|Layout \\{|typedef = .*INSTANCE_TYPE|typedef = &.*(EXCEPTION|Base|OSError)' pyre/pyre-object/src/typeobject.rs pyre/pyre-object/src/pyobject.rs | head -240Repository: youknowone/pyre
Length of output: 18481
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- pyobject.rs EXC_* static definitions and PyType constructors ---'
rg -n --type=rust -C3 '#\[pyre_class\(\"EXC|pub static EXC|EXC.*TYPE.*PyType|_new_exception|PyType = new_pytype' pyre/pyre-object/src/interp_exceptions.rs pyre/pyre-object/src/exceptions.rs pyre/pyre-object/src/*.rs pyre/pyre-*.rs | head -320
echo
echo '--- call.rs exact conflict block ---'
sed -n '3530,3548p' pyre/pyre-interpreter/src/call.rs
echo
echo '--- pyobject.rs object struct and pytype definition constructors ---'
rg -n --type=rust -C3 'pub struct PyType|pub struct PyObject|fn new_pytype|new_pytype\(' pyre/pyre-object/src/pyobject.rs | head -180Repository: youknowone/pyre
Length of output: 27036
Preserve sublayout compatibility in the native layout conflict check.
This currently rejects any pair of native layouts with different typedefs unless one is INSTANCE_TYPE, so a derived native layout is rejected even when issublayout(layout) succeeds. Gate the typedef mismatch on the sublayout relation failing in both directions, or keep the layout-check semantics tightly focused on incompatible sibling layouts.
🤖 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/call.rs` around lines 4690 - 4699, Update the
native_layout_conflict calculation in the layout-selection logic so differing
typedefs are considered conflicting only when the layouts are incompatible
sublayouts in both directions; preserve compatibility whenever either layout’s
issublayout relationship succeeds, including derived native layouts, while
retaining the INSTANCE_TYPE exclusions.
| /// Retag a freshly materialised `FunctionWithFixedCode` as the public | ||
| /// method-descriptor carrier. TypeDef builds its namespace before the owner | ||
| /// type exists, so this is deliberately performed during materialisation. | ||
| pub unsafe fn function_retag_method_descriptor(obj: PyObjectRef) { | ||
| unsafe { | ||
| (*obj).ob_type = &METHOD_DESCRIPTOR_TYPE as *const PyType; | ||
| (*obj).w_class = get_instantiate(&METHOD_DESCRIPTOR_TYPE); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find where function_retag_method_descriptor is called and whether
# the target object could already be published/reachable at that point.
rg -n -B3 -A10 'function_retag_method_descriptor' --type=rustRepository: youknowone/pyre
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -e
echo "== locate function.rs =="
fd -a 'function\.rs$' . | sed 's#^\./##'
echo "== git status/stat =="
git status --short
git diff --stat || true
echo "== search symbol text across tracked files =="
rg -n -B2 -A12 'function_retag_method_descriptor|METHOD_DESCRIPTOR_TYPE|walk_raw_function_roots|function_write_barrier' . --glob '!target' --glob '!node_modules' || true
echo "== list function.rs candidates with wc =="
fd 'function\.rs$' . -x sh -c 'echo "--- $1"; wc -l "$1"; sed -n "510,560p;620,680p;710,735p" "$1"' sh {}Repository: youknowone/pyre
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -e
echo "== typedef.rs around retag call =="
sed -n '1720,1770p' pyre/pyre-interpreter/src/typedef.rs
echo "== related descriptors/types context =="
sed -n '1660,1725p' pyre/pyre-interpreter/src/typedef.rs
sed -n '1770,1785p' pyre/pyre-interpreter/src/typedef.rs
echo "== Function struct and get_instantiate definitions =="
sed -n '90,140p' pyre/pyre-interpreter/src/function.rs
rg -n -B3 -A8 'fn get_instantiate|macro_rules!.*gc|pub unsafe fn .*get_instantiate|type.*PyObjectRef|struct Function' pyre/pyre-interpreter/src pyre/pyre-object/src --glob '*.rs' | head -n 220
echo "== inspect gc_roots prebuilt API =="
fd -a 'gc_roots.rs$|gc_hook.rs$|pyobject.rs$' pyre/pyre-interpreter/src pyre/pyre-object/src | sort | xargs -r sed -n '1,120p'Repository: youknowone/pyre
Length of output: 33405
Add the write barrier before stamping w_class.
function_retag_method_descriptor is called during typedef materialisation with descr pinned on the shadow stack, so it can run while the Function object is reachable and promoted. The w_class field is GC-managed, and missing this write barrier can let a clean minor collection skip the newly stored edge, leaving a dangling/reference-wrong child when that edge is later collected or the target moves.
Proposed defensive fix
pub unsafe fn function_retag_method_descriptor(obj: PyObjectRef) {
unsafe {
+ function_write_barrier(obj);
(*obj).ob_type = &METHOD_DESCRIPTOR_TYPE as *const PyType;
(*obj).w_class = get_instantiate(&METHOD_DESCRIPTOR_TYPE);
}
}📝 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.
| /// Retag a freshly materialised `FunctionWithFixedCode` as the public | |
| /// method-descriptor carrier. TypeDef builds its namespace before the owner | |
| /// type exists, so this is deliberately performed during materialisation. | |
| pub unsafe fn function_retag_method_descriptor(obj: PyObjectRef) { | |
| unsafe { | |
| (*obj).ob_type = &METHOD_DESCRIPTOR_TYPE as *const PyType; | |
| (*obj).w_class = get_instantiate(&METHOD_DESCRIPTOR_TYPE); | |
| } | |
| } | |
| /// Retag a freshly materialised `FunctionWithFixedCode` as the public | |
| /// method-descriptor carrier. TypeDef builds its namespace before the owner | |
| /// type exists, so this is deliberately performed during materialisation. | |
| pub unsafe fn function_retag_method_descriptor(obj: PyObjectRef) { | |
| unsafe { | |
| function_write_barrier(obj); | |
| (*obj).ob_type = &METHOD_DESCRIPTOR_TYPE as *const PyType; | |
| (*obj).w_class = get_instantiate(&METHOD_DESCRIPTOR_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/function.rs` around lines 538 - 547, Update
function_retag_method_descriptor to invoke the repository’s established GC
write-barrier helper immediately before assigning the GC-managed w_class field,
using obj as the owner and the method-descriptor class value as the stored edge;
keep the existing type retagging and class assignment behavior unchanged.
| BinaryOperator::InplacePower => match pow(a, b) { | ||
| Err(err) | ||
| if err.kind == crate::PyErrorKind::TypeError | ||
| && err | ||
| .message | ||
| .starts_with("unsupported operand type(s) for ** or pow():") => | ||
| { | ||
| Err(crate::PyError::type_error(format!( | ||
| "unsupported operand type(s) for **=: '{}' and '{}'", | ||
| crate::baseobjspace::object_functionstr_type_name(a), | ||
| crate::baseobjspace::object_functionstr_type_name(b), | ||
| ))) | ||
| } | ||
| result => result, | ||
| }, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Brittle string-prefix coupling with pow()'s error message.
The **= rewrite triggers only if the message text matches descroperation.rs::pow's exact wording. A future edit to that message (typo fix, wording tweak) would silently disable this rewrite instead of failing loudly.
♻️ Suggested fix: share one prefix constant
+// in descroperation.rs, exposed for opcode_ops.rs to match against:
+pub(crate) const POW_TYPE_ERROR_PREFIX: &str = "unsupported operand type(s) for ** or pow():";- && err
- .message
- .starts_with("unsupported operand type(s) for ** or pow():") =>
+ && err
+ .message
+ .starts_with(crate::objspace::descroperation::POW_TYPE_ERROR_PREFIX) =>🤖 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/opcode_ops.rs` around lines 65 - 79, Define and
reuse a shared constant for the unsupported-operand error prefix emitted by
descroperation.rs::pow and checked in the BinaryOperator::InplacePower match.
Replace the hardcoded starts_with string with that constant so wording changes
remain synchronized and the **= rewrite continues to trigger correctly.
| /// Restore the uninitialised `tp_mro == NULL` state while a metaclass MRO | ||
| /// calculation is in progress or when a transactional base update rolls | ||
| /// back a nascent type. | ||
| pub unsafe fn w_type_clear_mro(obj: PyObjectRef) { | ||
| (*(obj as *mut W_TypeObject)).mro_w = std::ptr::null_mut(); | ||
| crate::gc_hook::try_gc_write_barrier(obj as *mut u8); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# What does the version tag gate, and is a null mro_w reachable by a cached lookup?
rg -nP --type=rust -C6 'fn w_type_set_version_tag|version_tag' pyre/pyre-object/src/typeobject.rs | head -60
# Does any lookup path deref mro_w without a null check?
rg -nP --type=rust -C4 'w_type_get_mro\s*\(' pyre/pyre-interpreter/src pyre/pyre-object/src | head -60Repository: youknowone/pyre
Length of output: 8134
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant typeobject.rs snippets =="
sed -n '850,1005p' pyre/pyre-object/src/typeobject.rs
echo
echo "== lookup/cache paths with version tags =="
rg -nP --type=rust -C5 'version_tag|MethodCache|method_cache|MethodCache|InlineCache|CACHE|cache' pyre/pyre-interpreter/src pyre/rustlib/src | head -240
echo
echo "== type object definition around mro/version methods =="
rg -nP --type=rust -C8 'pub.*w_type_set_mro|pub fn w_type_set_version_tag|pub fn mutated|pub fn mutated' pyre/pyre-object/src/typeobject.rs | head -220
echo
echo "== pyre-interpreter type_set_bases rollback context =="
sed -n '10080,10145p' pyre/pyre-interpreter/src/typedef.rsRepository: youknowone/pyre
Length of output: 27540
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== mutable API for version_tag and mutated =="
rg -nP --type=rust -C8 'pub .*mutated|fn mutated|w_type_set_version_tag|version_tag\.store|version_tag\.load' pyre/pyre-object/src pyre/pyre-interpreter/src | head -240
echo
echo "== explicit version_tag access =="
rg -nP --type=rust -C4 'version_tag\.(store|load|swap|fetch_add|fetch_update)' pyre/pyre-interpreter/src pyre/pyre-object/src | head -240
echo
echo "== typeobject.rs around mutability methods =="
sed -n '920,990p' pyre/pyre-object/src/typeobject.rs
echo
echo "== files containing mutated call sites =="
rg -nP --type=rust -C4 'mutated\s*\(' pyre/pyre-interpreter/src pyre/pyre-object/src | head -260Repository: youknowone/pyre
Length of output: 12125
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== baseobjspace mutated implementation =="
sed -n '8075,8105p' pyre/pyre-interpreter/src/baseobjspace.rs
echo
echo "== type_set_bases around transactional base update =="
sed -n '10030,10135p' pyre/pyre-interpreter/src/typedef.rs
echo
echo "== broader mutated call sites =="
rg -nP --type=rust -C4 'baseobjspace::mutated|crate::baseobjspace::mutated|mutated\s*\(' pyre/pyre-interpreter/src | head -320Repository: youknowone/pyre
Length of output: 31817
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== method-cache / invalidation implementation =="
sed -n '8380,8540p' pyre/pyre-interpreter/src/baseobjspace.rs
echo
echo "== method-cache lookup implementation candidates =="
rg -nP --type=rust -C6 'method_cache|METHOD_CACHE|version_tag |version_tag\(|load|store|lookup_where|lookup_in_type|method.*cache|store_method_cache|get_method_cache' pyre/pyre-interpreter/src/baseobjspace.rs | head -260
echo
echo "== upstream/typeobject.py references if present =="
git ls-files | rg '(^|/)typeobject\.py$|typeobject\.rs$|baseobjspace\.rs$'Repository: youknowone/pyre
Length of output: 21605
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== raw MRO lookup paths =="
rg -nP --type=rust -C6 'lookup_where_pair|lookup_where_all_typeobjects|w_type_get_mro\(|mro_w|as_slice\(\)' pyre/pyre-interpreter/src/baseobjspace.rs pyre/pyre-object/src/typeobject.rs | head -260
echo
echo "== static check of version_tag==0 gate before method cache entries =="
python3 - <<'PY'
from pathlib import Path
src = Path('pyre/pyre-interpreter/src/baseobjspace.rs').read_text()
hits = src.splitlines()
for i,line in enumerate(hits,1):
if 'version_tag == 0' in line or 'version_tag == 0' in line:
print(f"line {i}: {line.strip()}")
for j in range(max(1,i-3), min(len(hits),i+3)+1):
print(f" {j}: {hits[j-1].strip()}")
print()
PYRepository: youknowone/pyre
Length of output: 22879
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== mro_subclasses implementation =="
sed -n '9930,10030p' pyre/pyre-interpreter/src/typedef.rs
echo
echo "== compute_mro / custom metaclass computation relevant snippets =="
rg -nP --type=rust -C6 'compute_mro|type_in_mro|type_in_bases|mro_subclasses' pyre/pyre-interpreter/src/typedef.rs | head -220
echo
echo "== broader references to w_type_clear_mro =="
rg -nP --type=rust -C4 'w_type_clear_mro|clear_mro|mro_clear' pyre/pyre-interpreter/src pyre/pyre-object/srcRepository: youknowone/pyre
Length of output: 11385
Clearing a cached MRO should restore a non-uncacheable version tag to 0.
w_type_set_mro sets version_tag = 0 when the new MRO cannot be cached, but w_type_clear_mro leaves the old tag unchanged. Since cached MRO lookups dispatch on a non-zero version tag, a rolled-back subclass whose previous MRO was cached can stay cached and produce stale descriptors/values from its old linearization.
🛡️ Proposed fix
pub unsafe fn w_type_clear_mro(obj: PyObjectRef) {
(*(obj as *mut W_TypeObject)).mro_w = std::ptr::null_mut();
crate::gc_hook::try_gc_write_barrier(obj as *mut u8);
+ // A type with no MRO cannot validate any cached lookup.
+ w_type_set_version_tag(obj, 0);
}[low,effort_and_high_reward]
📝 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.
| /// Restore the uninitialised `tp_mro == NULL` state while a metaclass MRO | |
| /// calculation is in progress or when a transactional base update rolls | |
| /// back a nascent type. | |
| pub unsafe fn w_type_clear_mro(obj: PyObjectRef) { | |
| (*(obj as *mut W_TypeObject)).mro_w = std::ptr::null_mut(); | |
| crate::gc_hook::try_gc_write_barrier(obj as *mut u8); | |
| } | |
| /// Restore the uninitialised `tp_mro == NULL` state while a metaclass MRO | |
| /// calculation is in progress or when a transactional base update rolls | |
| /// back a nascent type. | |
| pub unsafe fn w_type_clear_mro(obj: PyObjectRef) { | |
| (*(obj as *mut W_TypeObject)).mro_w = std::ptr::null_mut(); | |
| crate::gc_hook::try_gc_write_barrier(obj as *mut u8); | |
| // A type with no MRO cannot validate any cached lookup. | |
| w_type_set_version_tag(obj, 0); | |
| } |
🤖 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-object/src/typeobject.rs` around lines 975 - 981, Update
w_type_clear_mro to reset the type’s version_tag to 0 whenever it clears mro_w,
before the existing GC write barrier. Preserve the null-MRO restoration behavior
while ensuring subsequent cached MRO lookups cannot reuse the previous tag.
- time module: gate init_timezone/tzset registration behind not(sandbox) so a sandbox build keeps the UTC interplevel defaults. - time.strftime: decode libc output with surrogateescape (charp2uni) instead of rejecting non-UTF-8 locale bytes. - sys.getsizeof: reject a non-int __sizeof__ result with TypeError and a negative one with ValueError. - type.__new__: copy the assembled namespace with w_dict_copy so keys are not re-hashed while building the type dict. - float/complex __dict__ lazy-init: root and reload the receiver across w_dict_new, matching the bytes/bytearray arms. - member_descriptor.__doc__: drop the dead shadowing getter that returned None over the real member_get_doc. - dict subclass __new__: return a RuntimeError when the layout has no mapping payload slot instead of relying only on debug_assert. - jit: correct the W_FloatObject/W_ComplexObject GC-registration comments that still described them as pointer-free leaves. Assisted-by: Claude
- time.strftime: decode libc output as canonical WTF-8 first (Wtf8Buf::from_bytes) and fall back to charp2uni surrogateescape only for non-WTF-8 bytes, so a lone-surrogate directive round-trips instead of being re-split into three surrogate escapes. - object.__getattr__ __dict__ miss: walk the metatype MRO and honor a genuine metatype __dict__ override (a data descriptor whose owner is a proper subclass of type); every owner chains its layout typedef to object's INSTANCE_TYPE, so type's own canonical getset is told apart by owner identity, not layout. Fall through to the canonical dict proxy otherwise, and surface a null canonical dict as an error. - object.__setattr__: document why the exception-instance __dict__ assignment is routed straight to setdict ahead of the descriptor walk (a plain base mixed into an exception contributes a layout-incompatible instance __dict__ getset the walk must not reach). - float/complex __slots__ storage: extract shared slot_get/set/del helpers (pyre-object/src/slots.rs) that root and reload the receiver across the list-growth safepoints, and delegate both types to them; w_float_subclass_new / w_complex_subclass_new initialize the w_dict and w_slots fields the shared struct carries. - setdict: fold the six duplicated dict-type validation blocks into require_dict_for_setdict. - type_methods: extract dict_data_slot for the shared dict-backing layout walk used by set_dict_backing and resolve_dict_backing. - method/wrapper descriptor types: install __get__ (descr_carrier_get) via init_descriptor_type_common. - _weakref proxy pow test: assert three-arg pow reaches __rpow__. Assisted-by: Claude
2aa9aae to
e83d646
Compare
Exercise time.get_clock_info over the time, monotonic, perf_counter and process_time clocks and assert the namespace field types, and assert an unknown clock name raises ValueError.
The full test.test_datetime suite (datetimetester, including the per-timezone ZoneInfoTest matrix) does not finish within the 120s gate budget, so gating it as PASS regressed the CPython suite gate to TIMEOUT. Record it as TIMEOUT, matching the existing convention for modules that exceed the budget; the gate runs only the baseline-PASS subset, so it is no longer executed there. test_time stays PASS. Assisted-by: Claude
- time module: gate init_timezone/tzset registration behind not(sandbox) so a sandbox build keeps the UTC interplevel defaults. - time.strftime: decode libc output with surrogateescape (charp2uni) instead of rejecting non-UTF-8 locale bytes. - sys.getsizeof: reject a non-int __sizeof__ result with TypeError and a negative one with ValueError. - type.__new__: copy the assembled namespace with w_dict_copy so keys are not re-hashed while building the type dict. - float/complex __dict__ lazy-init: root and reload the receiver across w_dict_new, matching the bytes/bytearray arms. - member_descriptor.__doc__: drop the dead shadowing getter that returned None over the real member_get_doc. - dict subclass __new__: return a RuntimeError when the layout has no mapping payload slot instead of relying only on debug_assert. - jit: correct the W_FloatObject/W_ComplexObject GC-registration comments that still described them as pointer-free leaves. Assisted-by: Claude
- time.strftime: decode libc output as canonical WTF-8 first (Wtf8Buf::from_bytes) and fall back to charp2uni surrogateescape only for non-WTF-8 bytes, so a lone-surrogate directive round-trips instead of being re-split into three surrogate escapes. - object.__getattr__ __dict__ miss: walk the metatype MRO and honor a genuine metatype __dict__ override (a data descriptor whose owner is a proper subclass of type); every owner chains its layout typedef to object's INSTANCE_TYPE, so type's own canonical getset is told apart by owner identity, not layout. Fall through to the canonical dict proxy otherwise, and surface a null canonical dict as an error. - object.__setattr__: document why the exception-instance __dict__ assignment is routed straight to setdict ahead of the descriptor walk (a plain base mixed into an exception contributes a layout-incompatible instance __dict__ getset the walk must not reach). - float/complex __slots__ storage: extract shared slot_get/set/del helpers (pyre-object/src/slots.rs) that root and reload the receiver across the list-growth safepoints, and delegate both types to them; w_float_subclass_new / w_complex_subclass_new initialize the w_dict and w_slots fields the shared struct carries. - setdict: fold the six duplicated dict-type validation blocks into require_dict_for_setdict. - type_methods: extract dict_data_slot for the shared dict-backing layout walk used by set_dict_backing and resolve_dict_backing. - method/wrapper descriptor types: install __get__ (descr_carrier_get) via init_descriptor_type_common. - _weakref proxy pow test: assert three-arg pow reaches __rpow__. Assisted-by: Claude
…esolution - type.__bases__ setter: pin `saved_bases` across `mro_subclasses` so a custom `mro()` that triggers a collection cannot free the old bases tuple before the rollback reads it, and reload the pinned value in the rollback. - type.__bases__ rollback: on a recomputation failure restore every visited class's MRO and the old bases unconditionally (typeobject.py:961-963), dropping the `bases_now == w_value` guard that upstream does not have. - LOAD_SPECIAL: extract `load_special_resolve` — a pure type lookup plus the descriptor's `__get__`, bypassing the instance dict and `__getattribute__` — and route both the interpreter (`load_special`) and the JIT residual (`bh_load_special_fn`) through it, so a traced `with`/`async with` on a receiver overriding `__getattribute__` calls the same method as the interpreter; `bh_load_special_self_fn` reports a NULL self accordingly. - Wrap an over-length line in slots.rs. Assisted-by: Claude
… guard `Arguments.parse_obj` fills every parameter slot before an inline or method builtin body runs, so a keyword-only parameter widens the received `args` past the positional count. The generated surplus-positional guard compared that widened count against the positional-only total, so every call to a non-varargs builtin carrying a keyword-only parameter — `warnings.warn` among them — raised a spurious "expected at most N arguments" TypeError. Compare against the full parameter-slot count, the bound `bind_builtin_kwargs` already enforces, and keep the positional count in the message. Assisted-by: Claude
- super(): when `type(obj)` is not a subtype of the start type, resolve the bound object's type through `obj.__class__` honouring `__getattribute__`, so a proxy that forwards `__class__` to a wrapped instance is a valid super() target (descriptor.py:73-96 `_super_check`). - type.__bases__ rollback: record a class for MRO restoration only after its MRO recomputes, and roll the bases back only while they are still the ones this call installed. A reentrant `__bases__` assignment reached through a custom `mro()` that then raises keeps the bases and MRO it committed (typeobject.c `mro_hierarchy` / `type_set_bases`). Assisted-by: Claude
…receiver The type.__doc__ getset bound the instance __doc__ descriptor that property/function/method keep under the __doc__ key, so property.__doc__ and types.MethodType.__doc__ surfaced the getset_descriptor instead of the type's own doc. A shared type_builtin_own_doc helper now serves those constants for descr__doc's non-heap branch, consulted by both the getset getter and the type-attribute cascade. function.__get__ and function.__repr__ ran checks that accept any function-layout carrier (is_function_carrier), so builtin functions reached them. builtin_function deletes __get__ and overrides __repr__, so these function-only slots now require a genuine function receiver via function_type_receiver; the shared descr_carrier_get bind is factored into carrier_descr_bind for method_descriptor/wrapper_descriptor. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e83d6464cb
ℹ️ 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 rhs_first = match (base_type, exp_type) { | ||
| (Some(lhs), Some(rhs)) if !std::ptr::eq(lhs, rhs) => unsafe { | ||
| crate::baseobjspace::issubtype_w(rhs, lhs) |
There was a problem hiding this comment.
When the exponent is a strict subclass that merely inherits its base's __rpow__, rhs_first becomes true solely because of the subtype relationship. For example, if A.__pow__ returns "forward", A.__rpow__ returns "reverse", and class B(A): pass, pow(A(), B(), 3) now calls the inherited A.__rpow__ first and returns "reverse", whereas Python calls A.__pow__ and returns "forward". Reflected precedence applies only when the subclass provides a distinct reflected implementation, so this needs the defining-source/implementation comparison used by the binary _pow_find_impls dispatch rather than a subtype-only shortcut.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
e83d646 to
ac859fc
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac859fc4ee
ℹ️ 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 value = unsafe { pyre_object::w_float_get_value(args[1]) }; | ||
| Ok(pyre_object::w_float_new(value)) |
There was a problem hiding this comment.
float.real
When the receiver is an exact float, this always allocates a new object, so x.real is x becomes false. Both CPython and PyPy preserve identity here: PyPy's descr_get_real returns space.float(self), which returns the same exact float while converting only strict subclasses to a base float. Keep the fresh allocation for subclasses, but return args[1] for an exact float.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
W_FloatObject carries w_dict/w_slots gc-pointer slots that the runtime TypeInfo traces, but W_FLOAT_DESCR_GROUP declared only floatval. The backend clear_gc_fields zeroes exactly the group's Type::Ref fields, so a NewWithVtable'd exact float left w_dict/w_slots as uninitialized nursery bytes and the collector traced poison (nbody: invalid type_id at minor_fixed_field_target / oldgen LayoutError). Register both as mutable gc-pointer fields after floatval so its stable field index stays 0. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/d94e6c3e61e0fbe1a3d3de63bed6e001677a2b7b/pyre-macros/src/lib.rs#L1925
When a #[pyre_methods] method declares a keyword-only parameter, such as f(self, x, #[kwonly] y), comparing __pyre_positional_count with the full slot count allows obj.f(1, 2) to pass and unwraps 2 as y. These wrappers do not carry a gateway Signature that could reject the call first, so the limit must remain receiver_slots + visible_max; bound keyword-only slots do not increase the number of positionals accepted.
AGENTS.md reference: AGENTS.md:L194-L195
ℹ️ 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".
Summary
Port PyPy-compatible
time/datetimebehavior and the descriptor/typesemantics the CPython
test_time,test_datetime, andtest_descrsuitesdepend on, and mark those three modules PASS in the dynasm runner baseline.
What changed
time/datetimetzset, timestamp validation, clock metadata,and the supporting string/regex behavior for
timeand pure-Pythondatetimetime.strftimedecodes libc output as canonical WTF-8 first and only fallsback to
charp2unisurrogateescape for genuinely non-WTF-8 locale bytes, soa lone-surrogate directive round-trips instead of being re-split into three
surrogate escapes
descriptors / type object
staticmethods, classmethods, properties, members, getsets, and slot wrappers;
method_descriptor/wrapper_descriptorexpose__get____bases__mutation/reentrancy, native subclass layout, and special-method lookup with
PyPy/CPython
object.__getattr__resolves a__dict__miss on a class by walking themetatype MRO and honoring a genuine metatype
__dict__override (a datadescriptor whose owner is a proper subclass of
type);type's owncanonical getset is told apart by owner identity, since every class chains
its layout typedef to
object'sINSTANCE_TYPEfloat/complex__slots__storage intoslot_get/slot_set/slot_delhelpers that root and reload the receiveracross the list-growth safepoints, and fold the six duplicated
setdictdict-type validations into one gate
runner
test_datetime, avoiding theopt-in exhaustive system-tzdata sweep
test_descrtest.test_time,test.test_datetime, andtest.test_descrPASSReview fixes
Two rounds of review were applied on top of the feature commits:
timeinit registration gated behindnot(sandbox)so a sandbox build keepsthe UTC interplevel defaults
sys.getsizeofrejects a non-int__sizeof__(TypeError) and a negative one(ValueError);
sys.getsizeofprefers the type's__sizeof__type.__new__copies the assembled namespace withw_dict_copyso keys arenot re-hashed while the type dict is built
float/complex__dict__lazy-init roots and reloads the receiver acrossw_dict_new, matching the bytes/bytearray armsmember_descriptor.__doc__no longer shadows the realmember_get_docdictsubclass__new__raisesRuntimeErrorwhen the layout carries nomapping payload slot
pow(base, exp, mod)reaches__rpow__in the reflected-power order (3.14)__dict__assignment is routed tosetdictahead of thedescriptor walk, so a plain base mixed into an exception cannot expose a
layout-incompatible instance
__dict__getsetW_FloatObject/W_ComplexObjectGC-registration comments thatstill described them as pointer-free leaves
Rebase revalidation fixes
Rebasing onto
mainpulled in #791'scount_preambleand a newertest_descr, surfacing three gate regressions, all fixed here:pyre-macros):Arguments.parse_objfills everyparameter slot before a builtin body runs, so a keyword-only parameter
widened the received
argspast the positional count. The generatedsurplus-positional guard compared that widened count against the
positional-only total, so every non-varargs builtin carrying a keyword-only
parameter —
warnings.warnincluded — raised a spurious "expected at most Narguments"
TypeError, breakingtest_timeandtest_datetime. The guardnow compares against the full parameter-slot count, the bound
bind_builtin_kwargsalready enforces, and keeps the positional count in themessage.
baseobjspace): whentype(obj)is not a subtype of thestart type,
super()resolves the bound object's type throughobj.__class__honouring__getattribute__, so a proxy forwarding__class__to a wrapped instance is a validsuper()target(
_super_check); fixestest_proxy_super.typedef): a class is recorded for MRO restorationonly after its MRO recomputes, and the bases roll back only while they are
still the ones this call installed — a reentrant
__bases__assignmentreached through a custom
mro()that then raises keeps the bases and MRO itcommitted (
mro_hierarchy/type_set_bases); fixestest_tp_subclasses_cycle_error_return_path.Validation
test.test_time,test.test_datetime, andtest.test_descrPASSPASS -> FAIL/TIMEOUT/CRASH/IMPORTERRORregressions
cargo teston the touched cratesthe prepass rebuilt
mainSummary by CodeRabbit
New Features
time.tzset().time.strftime()support, including embedded null characters and broader Unicode handling.floatandcomplexsubclasses.pow(),round(),reversed(), andcomplex().Bug Fixes
sys.getsizeof()handling and reflected three-argument power operations.Tests
time.get_clock_info()and updated CPython compatibility results.