coroutine, generator, io, exceptions and CPython allocation/layout metadata; five jit fixes - #1126
Conversation
WalkthroughThe change set adds CPython-compatible behavior for object sizing, exceptions, generators, files, functions, layouts, lists, and bytearrays. It also updates JIT tracing, rollback, snapshot, bridge, and exception-resume handling with extensive regression coverage. ChangesInterpreter runtime and JIT correctness
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ad1d471a3
ℹ️ 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 op == self.op_catch_exception { | ||
| return crossed_trailing_live.then_some(q); |
There was a problem hiding this comment.
When a GUARD_NO_EXCEPTION for an operation outside a try fails and resumes at the first opcode of a following try, this forward scan ignores every operation until the next live marker and then accepts that following operation's catch. The blackhole consequently routes the earlier exception into a handler that does not cover the raising operation, so compiled execution can swallow an exception that the interpreter propagates; restrict this path to the known successor-sync operations or encode the originating catch explicitly.
AGENTS.md reference: AGENTS.md:L14-L19
Useful? React with 👍 / 👎.
| let pre_header = crate::typedef::r#type(current()) | ||
| .filter(|tp| unsafe { pyre_object::w_type_is_heaptype(tp.as_ptr()) }) | ||
| .map_or(0u64, |_| (4 * std::mem::size_of::<usize>()) as u64); |
There was a problem hiding this comment.
For common inputs this equates heap types with the entire CPython preheader, but _PyType_PreHeaderSize independently adds the GC header and the managed dict/weakref prefix. Thus an exact GC-tracked builtin such as list is not a heap type and gets no header (sys.getsizeof([]) becomes 40 rather than 56 on 64-bit CPython 3.14), while a heap class with __slots__ = () gets 32 bytes despite having only the 16-byte GC header. Use the type's GC and managed-prefix properties rather than flag_heaptype alone.
Useful? React with 👍 / 👎.
| let (basicsize, itemsize) = cpython_type_layout(w_type) | ||
| .unwrap_or((2 * std::mem::size_of::<usize>() as i64, 0)); | ||
| let nitems = if itemsize == 0 { | ||
| 0 | ||
| } else { | ||
| int_cpython_digit_count(args[0])? | ||
| }; | ||
| Ok(w_int_new(basicsize + itemsize * nitems)) |
There was a problem hiding this comment.
When the receiver is itself a class object, special lookup currently falls through to this new object.__sizeof__; cpython_type_layout(type) then makes every class report the metatype basicsize, 936 bytes on 64-bit. CPython 3.14 defines a separate type.__sizeof__ and reports 416 for examples such as sys.getsizeof(int), so the newly advertised type sizing is substantially wrong until that descriptor and its allocation calculation are installed on type.
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 8650fc5). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c722103830
ℹ️ 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 nitems = if itemsize == 0 { | ||
| 0 | ||
| } else { | ||
| int_cpython_digit_count(args[0])? |
There was a problem hiding this comment.
For every variable-sized type that inherits object.__sizeof__, this calls the int-specific digit counter, which returns one for non-integers. Consequently, on 64-bit builds b"abc".__sizeof__() reports 34 instead of 36 and (1, 2).__sizeof__() reports 40 instead of 48; longer bytes, tuples, and memoryviews remain similarly wrong. Obtain the equivalent of each layout's Py_SIZE rather than treating every variable-sized object as a one-digit integer.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
| let written = |f| { | ||
| exc_is_blocking_io_error(exc) | ||
| .then(|| crate::baseobjspace::int_w(f).ok()) | ||
| .flatten() | ||
| }; | ||
| if let Some(value) = w_filename.and_then(written) { |
There was a problem hiding this comment.
When the third argument is a non-int object accepted by int_w (for example, an object implementing __index__), this stores the converted value in written but keeps the original object in the three-element args tuple. The later filename fallback only recognizes an exact int in args[2], so BlockingIOError(1, "x", indexable).filename incorrectly returns that object instead of None, even though characters_written was set. Track the successful conversion when deciding whether the argument is a filename.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
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/type_methods.rs (1)
587-598: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse the refreshed
listpointer beforew_list_resize_for_extendruns.
pin_root(list)protectslistacrossw_set_items(other), but that path can hash set keys and run a user__hash__; GC may then movelist. Reload it from the shadow stack beforew_list_resize_for_extend(list, items.len()), the same way the append loop reloadslistand the set items.🛡️ Proposed fix
- pyre_object::listobject::w_list_resize_for_extend(list, items.len()); + pyre_object::listobject::w_list_resize_for_extend( + pyre_object::gc_roots::shadow_stack_get(root_base), + items.len(), + );🤖 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 587 - 598, Refresh the protected list pointer after w_set_items(other) and before w_list_resize_for_extend by loading it from the shadow stack, as the append loop already does. Use this refreshed pointer for resizing while preserving the existing root indexing and append 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/extra_tests/parity_tests/blockingioerror_characters_written.py`:
- Around line 24-27: Add a negative case to the test using a non-OSError
exception, such as a generic Exception: verify characters_written is initially
absent, assign a value and verify it is stored in the instance dictionary rather
than the special slot, then confirm the attribute can be removed without
affecting OSError behavior.
In `@pyre/pyre-interpreter/src/call.rs`:
- Around line 5148-5159: Update base_has_variable_items to derive whether the
base layout has non-zero item size from the shared cpython_type_layout
predicate/table, rather than comparing typedefs against INT_TYPE, TUPLE_TYPE,
and BYTES_TYPE. Preserve the existing null-layout handling and use the shared
layout metadata so TYPE_TYPE and all other variable-sized bases are covered.
In `@pyre/pyre-interpreter/src/executioncontext.rs`:
- Around line 833-841: Update finalize_discarded_coroutine_after_frame_get and
the associated UserDelAction dispatch so repeated coroutine_get_frame accesses
do not trigger a full old-generation collection on every request. Coalesce or
rate-limit pending requests, while preserving the required CPython
refcount-boundary behavior and ensuring at most one major collection occurs per
configured batch or dispatch.
In `@pyre/pyre-interpreter/src/function.rs`:
- Around line 522-523: Guard the result of resolve_dict_backing in the globals
lookup before calling w_dict_getitem_str: when it is pyre_object::PY_NULL, treat
selected as absent and continue to the execution-context fallback; only invoke
w_dict_getitem_str for a valid backing pointer.
In `@pyre/pyre-interpreter/src/pyframe.rs`:
- Around line 3294-3306: Update the generator cleanup path around
generator_finalize to root w_gen on the generator shadow stack before
finalization, then reload the frame pointer afterward via
shadow_stack_get(gen_slot). Pass the reloaded frame to
generator_frame_is_finished and subsequent finalizer/clear operations, retaining
FrameAnchor::new(self) across the Python call.
In `@pyre/pyre-interpreter/src/typedef.rs`:
- Around line 19086-19091: Update the object.__sizeof__ size calculation around
nitems so non-integer receivers do not use int_cpython_digit_count(args[0]).
Dispatch the item count by receiver kind: use the receiver’s logical length for
tuple and bytes, and implement the CPython-compatible counts for type and
memoryview; retain the existing integer path and size formula for integer
receivers.
- Around line 10398-10424: Update the four getters—type_basicsize_getter,
type_itemsize_getter, type_dictoffset_getter, and type_weakrefoffset_getter—to
use the generic-object fallback established by object.__sizeof__ when
cpython_type_layout or cpython_type_offsets returns None. Preserve the existing
specialized values for listed layouts, but return the corresponding generic
fallback values for all unlisted types instead of raising AttributeError.
In `@pyre/pyre-object/src/int_array.rs`:
- Line 28: Restore private visibility for the len fields in IntArray
(pyre/pyre-object/src/int_array.rs:28-28) and FloatArray
(pyre/pyre-object/src/float_array.rs:24-24). Provide only a read accessor or a
crate-visible setter that delegates to set_len, preserving its capacity
assertion so all length updates remain validated.
---
Outside diff comments:
In `@pyre/pyre-interpreter/src/type_methods.rs`:
- Around line 587-598: Refresh the protected list pointer after
w_set_items(other) and before w_list_resize_for_extend by loading it from the
shadow stack, as the append loop already does. Use this refreshed pointer for
resizing while preserving the existing root indexing and append behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 322eaaa4-47a8-4c3a-82ce-f437d89e8078
📒 Files selected for processing (38)
extra_tests/test_itertools.pymajit/majit-metainterp/src/blackhole.rspyre/extra_tests/parity_tests/blockingioerror_characters_written.pypyre/extra_tests/parity_tests/function_builtins_inheritance.pypyre/extra_tests/parity_tests/open_non_inheritable.pypyre/extra_tests/parity_tests/socket_timeout_identity.pypyre/extra_tests/parity_tests/unraisable_del_message.pypyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/call.rspyre/pyre-interpreter/src/eval.rspyre/pyre-interpreter/src/executioncontext.rspyre/pyre-interpreter/src/function.rspyre/pyre-interpreter/src/module/_ctypes/interp_ctypes.rspyre/pyre-interpreter/src/module/_io/bytesio.rspyre/pyre-interpreter/src/module/_socket/interp_socket.rspyre/pyre-interpreter/src/module/_weakref/interp__weakref.rspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-interpreter/src/objspace/descroperation.rspyre/pyre-interpreter/src/pyframe.rspyre/pyre-interpreter/src/reduce_protocol.rspyre/pyre-interpreter/src/type_methods.rspyre/pyre-interpreter/src/typedef.rspyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit-trace/src/jitcode_dispatch/tests.rspyre/pyre-jit-trace/src/jitcode_dispatch/vstack_mirror.rspyre/pyre-jit/src/eval.rspyre/pyre-object/src/bytearrayobject.rspyre/pyre-object/src/float_array.rspyre/pyre-object/src/int_array.rspyre/pyre-object/src/interp_exceptions.rspyre/pyre-object/src/listobject.rspyre/pyre-object/src/weakref.rs
| /// Request the CPython refcount boundary associated with exposing a | ||
| /// coroutine's frame. The action runs before the next opcode, after the | ||
| /// attribute receiver has left the value stack. | ||
| pub fn finalize_discarded_coroutine_after_frame_get(&mut self) { | ||
| if !self.user_del_action.is_null() { | ||
| unsafe { (*self.user_del_action).collect_oldgen_and_fire() }; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
A full old-generation collection now runs on every cr_frame read.
coroutine_get_frame calls this helper on each attribute access, and UserDelAction::perform then runs try_gc_collect_oldgen() (Line 2427), a non-moving major mark-sweep. A loop such as for c in coros: inspect(c.cr_frame) therefore performs one major collection per iteration. The CPython behavior being modeled is a single refcount decrement.
Bound the cost. Options: fire only when the receiver is not reachable from the caller's value stack, coalesce repeated requests within one action dispatch, or gate the request behind a counter so that at most one collection runs per N requests.
🤖 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/executioncontext.rs` around lines 833 - 841, Update
finalize_discarded_coroutine_after_frame_get and the associated UserDelAction
dispatch so repeated coroutine_get_frame accesses do not trigger a full
old-generation collection on every request. Coalesce or rate-limit pending
requests, while preserving the required CPython refcount-boundary behavior and
ensuring at most one major collection occurs per configured batch or dispatch.
| pub block: *mut TypedItemsBlock, | ||
| /// Live length (rlist.py:116 `("length", Signed)`). | ||
| len: usize, | ||
| pub(crate) len: usize, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
len widened to pub(crate) on both typed arrays, bypassing the set_len capacity assertion. Both IntArray and FloatArray guard len through set_len, which asserts new_len <= capacity because as_slice / as_mut_slice build a slice of exactly len elements from the block base. Opening the field to the whole crate lets any pyre-object module write len directly and produce an out-of-bounds slice.
pyre/pyre-object/src/int_array.rs#L28-L28: restore the field to private and add apub(crate) fn len_mut-free accessor, or apub(crate)setter that reuses theset_lenassertion.pyre/pyre-object/src/float_array.rs#L24-L24: apply the same change so both sibling types keep one access rule.
📍 Affects 2 files
pyre/pyre-object/src/int_array.rs#L28-L28(this comment)pyre/pyre-object/src/float_array.rs#L24-L24
🤖 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/int_array.rs` at line 28, Restore private visibility for
the len fields in IntArray (pyre/pyre-object/src/int_array.rs:28-28) and
FloatArray (pyre/pyre-object/src/float_array.rs:24-24). Provide only a read
accessor or a crate-visible setter that delegates to set_len, preserving its
capacity assertion so all length updates remain validated.
…ed sites
`cargo fmt --all -- --check` reported four hunks in `baseobjspace.rs` and one
each in `executioncontext.rs` and `function.rs`: `unsafe { ... }` blocks and
call argument lists that rustfmt joins onto fewer lines. Whitespace only.
Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/cf40e9e46c0a0d4aaa508a15a54d575b7ca89c75/pyre-jit-trace/src/descr.rs#L1519-L1522
When BUILD_LIST is specialized, both emit_typed_list_inline and emit_object_list_inline allocate a W_ListObject but never emit a store for this newly registered integer field. Nursery allocations are not zero-filled for scalar fields—the nearby helpers explicitly store length for that reason—so an escaped compiled list can materialize with an arbitrary allocated; list.__sizeof__() and subsequent resize decisions then diverge from w_list_new, which initializes it to the element count. Emit the matching SetfieldGc in every inline list-construction path.
AGENTS.md reference: AGENTS.md:L14-L19
https://github.com/youknowone/pyre/blob/cf40e9e46c0a0d4aaa508a15a54d575b7ca89c75/pyre-interpreter/src/module/sys/vm.rs#L63-L66
Fresh evidence in the updated code is that the GC-header decision now uses try_gc_owns_object, which describes pyre's physical allocator rather than CPython's Py_TPFLAGS_HAVE_GC semantics. Consequently the same exact type can change size depending on how it was produced: for example a JIT-materialized exact int or str owned by majit's GC receives an extra two words even though CPython does not GC-track those types, while a fallback allocation of a GC-tracked type can omit them when no ownership hook is active. Base this component on the logical type/layout tracking flag so sys.getsizeof() is independent of execution backend and allocation mode.
https://github.com/youknowone/pyre/blob/cf40e9e46c0a0d4aaa508a15a54d575b7ca89c75/pyre-interpreter/src/typedef.rs#L25858-L25861
This schedules an old-generation collection after every successful cr_frame lookup, even when the receiver is strongly retained, such as c = coro(); frame = c.cr_frame. CPython merely decrements the temporary attribute receiver in this operation; it does not collect unrelated cyclic garbage, whereas pyre's next opcode can now run unrelated finalizers and pays for a full old-gen pass on ordinary introspection. The collection request needs to be limited to an actually discarded temporary coroutine rather than using a global GC pass as a refcount-timing substitute.
AGENTS.md reference: AGENTS.md:L252-L254
ℹ️ 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs (1)
972-975: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale append rollback comment and keep the restored allocation aligned with the live strategy.
The “Append entries record only spare-capacity growth, so no allocation needs undoing” note contradicts the unconditional
w_list_set_allocated(list, allocated_before)after the length rewind. Update the comment so it no longer says rollbacks skip allocation restore. If an append entry’s capturedallocated_beforecomes from the original backed capacity while rollout writes that value onto anIntOrFloatblock, ensure that value is valid for the current block’sTypedItemsBlock.capacity; otherwise align the saved/restore semantics so the stored value is strategy-compatible.🤖 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-jit-trace/src/jitcode_dispatch/fbw_state.rs` around lines 972 - 975, Update the rollback comment near the eager-list undo logic to accurately describe allocation restoration. In the append-entry rollback path, ensure allocated_before is compatible with the live strategy’s TypedItemsBlock.capacity before w_list_set_allocated restores it, or change capture/restore semantics so the saved allocation value is strategy-specific.majit/majit-metainterp/src/pyjitpl.rs (1)
5968-5986: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd the missing
cancel_countbump before returningCancelledon a declined cross-loop cut.When
cut_trace_from_with_constsdeclines (None), the code returnsCompileOutcome::Cancelleddirectly without incrementingself.cancel_count:let Some(cut) = trace.cut_trace_from_with_consts( start, original_boxes, &ctx.initial_inputarg_consts, ) else { return CompileOutcome::Cancelled; };The comment above this code states the cancellation "lands on the outcome the
except InvalidLooparm just below it already defines" (Line 5976-5978), but that arm (around Line 6187-6199) doesself.cancel_count += 1;before anyCancelledreturn. This return happens afterself.tracing.take(), socompile_loop's wrapper tears the session down viaclear_trace_session()on the way out — the trace is fully discarded either way. Without the increment, a cross-loop-cut trace that deterministically declines at this exact point (same trace shape reached again on retrace) can repeat forever without ever reachingcancelled_too_many_times(), unlike the analogous decline incompile_retrace(Lines 7606-7616), whose barefalsereturn is escalated by its caller with the samecancel_count += 1; if self.cancelled_too_many_times() { ... }bookkeeping.🐛 Proposed fix
let Some(cut) = trace.cut_trace_from_with_consts( start, original_boxes, &ctx.initial_inputarg_consts, ) else { + // Mirror the `except InvalidLoop` arm's bookkeeping so a + // trace that deterministically declines at this exact cut + // eventually reaches `cancelled_too_many_times()` instead of + // retracing forever. + self.cancel_count += 1; + if crate::closedbg_enabled() { + eprintln!("@@@CANCEL-SITE line={}", line!()); + } return CompileOutcome::Cancelled; }; cut🤖 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 `@majit/majit-metainterp/src/pyjitpl.rs` around lines 5968 - 5986, Increment self.cancel_count before returning CompileOutcome::Cancelled when cut_trace_from_with_consts returns None, matching the bookkeeping in the nearby InvalidLoop cancellation path and ensuring repeated declined cross-loop cuts can reach cancelled_too_many_times().
🤖 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 `@majit/majit-backend-dynasm/src/aarch64/assembler.rs`:
- Around line 2218-2220: Update the input-argument mapping around
setup_input_state and the enumerate loop so bridge input arguments retain their
bridge-specific frame slots. Do not unconditionally assign JITFRAME_FIXED_SIZE +
position for every iarg; preserve existing opref_to_slot entries and only apply
sequential slots to arguments without a bridge mapping, while still allowing
current_frame_loc updates for regalloc-assigned inputs.
In `@majit/majit-metainterp/src/blackhole.rs`:
- Around line 1379-1418: Before merging the change in
find_catch_after_resume_live, run cargo check and cargo test with the dynasm
feature enabled, then execute all eight required benchmarks and verify they
complete successfully.
In `@majit/majit-metainterp/src/pyjitpl.rs`:
- Around line 7254-7284: Update the shared orig_vable_ptr_from_trace_ctx helper
to ignore null OpRef::ConstPtr values before trying
standard_virtualizable_concrete() and virtualizable_heap_ptr() fallbacks.
Replace the duplicated entry_orig_vable_ptr resolution with a call to this
helper, preserving the null result when no valid pointer exists and applying the
behavior consistently across all callers.
In `@pyre/pyre-interpreter/src/typedef.rs`:
- Around line 10444-10460: Validate args[0] as a type object at the start of the
__sizeof__ closure before calling w_type_is_heaptype or w_type_get_hasdict,
reusing the existing cpython_type_layout validation used by
type_basicsize_getter and related members. Ensure non-type receivers such as
type.__sizeof__(1) raise the expected TypeError while valid type receivers
retain the current size calculation.
In `@pyre/pyre-jit-trace/src/helpers.rs`:
- Around line 1232-1241: Remove the tautological debug_assert! from the
ListStrategy::Empty | ListStrategy::IntOrFloat match arm in the surrounding
helper, since it only rechecks the pattern that selected the arm. Keep the
existing explanatory comment and arm behavior unchanged; do not replace it
unless there is a concrete unreachable-state assertion for an unexpected caller.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/tests.rs`:
- Around line 6883-6902: Update the folded destination initialization in
cast_float_to_int_folds_a_const_float_without_recording to use the valid
OpRef::NONE constant instead of OpRef::None, while preserving the existing
folding assertions.
In `@pyre/pyre-object/src/interp_exceptions.rs`:
- Around line 1399-1407: Add Rust documentation comments with a # Safety section
to the public unsafe accessors w_exception_get_blocking_written_arg and
w_exception_set_blocking_written_arg, documenting the required valid
W_BaseException pointer precondition consistently with the existing
w_exception_get_written and w_exception_set_written functions.
In `@pyre/pyre-object/src/listobject.rs`:
- Around line 1591-1663: Update w_list_reserve_for_extend,
w_list_resize_for_extend, w_list_finish_extend, w_list_finish_batch_resize, and
w_list_set_allocated to pin obj with gc_roots::push_roots()/pin_root and reload
it after w_list_lock(obj), matching w_list_len, w_list_append, and w_list_pop.
Use the reloaded post-lock object for all subsequent dereferences.
- Around line 2080-2104: Update w_list_sort_int_or_float’s comparator to impose
a deterministic total order for NaN values instead of mapping partial_cmp
failures to Equal. Preserve the existing numeric ordering for non-NaN int/float
values and ensure equal values retain stable-sort behavior, including through
the existing reverse handling.
---
Outside diff comments:
In `@majit/majit-metainterp/src/pyjitpl.rs`:
- Around line 5968-5986: Increment self.cancel_count before returning
CompileOutcome::Cancelled when cut_trace_from_with_consts returns None, matching
the bookkeeping in the nearby InvalidLoop cancellation path and ensuring
repeated declined cross-loop cuts can reach cancelled_too_many_times().
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs`:
- Around line 972-975: Update the rollback comment near the eager-list undo
logic to accurately describe allocation restoration. In the append-entry
rollback path, ensure allocated_before is compatible with the live strategy’s
TypedItemsBlock.capacity before w_list_set_allocated restores it, or change
capture/restore semantics so the saved allocation value is strategy-specific.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 877b5d72-7756-45ad-ba23-0e02b10b19d4
📒 Files selected for processing (36)
majit/majit-backend-dynasm/src/aarch64/assembler.rsmajit/majit-backend-dynasm/src/x86/assembler.rsmajit/majit-metainterp/src/blackhole.rsmajit/majit-metainterp/src/pyjitpl.rspyre/bench/synth/closure_per_call.wasm.jitstatspyre/bench/synth/inline_freevar_after_mayforce.cranelift.jitstatspyre/bench/synth/inline_freevar_after_mayforce.dynasm.jitstatspyre/bench/synth/inline_freevar_after_mayforce.wasm.jitstatspyre/bench/synth/list_pop_append.wasm.jitstatspyre/bench/synth/pypy_type_surface.cranelift.jitstatspyre/bench/synth/pypy_type_surface.dynasm.jitstatspyre/bench/synth/pypy_type_surface.pypyre/bench/synth/pypy_type_surface.wasm.jitstatspyre/extra_tests/parity_tests/blockingioerror_characters_written.pypyre/extra_tests/parity_tests/pickletools_jit_carrier_abort.pypyre/extra_tests/parity_tests/re_jit_call_resume.pypyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/call.rspyre/pyre-interpreter/src/function.rspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-interpreter/src/pyframe.rspyre/pyre-interpreter/src/type_methods.rspyre/pyre-interpreter/src/typedef.rspyre/pyre-jit-trace/src/descr.rspyre/pyre-jit-trace/src/helpers.rspyre/pyre-jit-trace/src/jitcode_dispatch/arith.rspyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit-trace/src/jitcode_dispatch/resume_snapshot.rspyre/pyre-jit-trace/src/jitcode_dispatch/specialize.rspyre/pyre-jit-trace/src/jitcode_dispatch/tests.rspyre/pyre-jit-trace/src/trace.rspyre/pyre-object/src/interp_exceptions.rspyre/pyre-object/src/listobject.rs
| for (position, iarg) in inputargs.iter().enumerate() { | ||
| self.opref_to_slot | ||
| .insert(iarg.opref(), JITFRAME_FIXED_SIZE + iarg.index as usize); | ||
| .insert(iarg.opref(), JITFRAME_FIXED_SIZE + position); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== InputArg and FrameLoc definitions =="
rg -n 'struct InputArg' -A 15 --type=rust
rg -n 'struct FrameLoc' -A 12 --type=rust
echo "== RegAlloc::prepare_bridge: how are bridge input-arg frame positions assigned? =="
rg -n 'fn prepare_bridge' -A 50 --type=rust
echo "== Does ra.longevity cover every input arg with a current_frame_loc? =="
rg -n 'current_frame_loc' -B3 -A10 --type=rustRepository: youknowone/pyre
Length of output: 193
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files matching assembler.rs =="
fd -a 'assembler\.rs$' . | sed 's#^\./##'
echo "== git status/stat =="
git status --short
git diff --stat
echo "== search symbols broadly =="
rg -n 'struct InputArg|struct FrameLoc|fn prepare_bridge|current_frame_loc|setup_input_state|bridge_input_locs|rebuild_faillocs_from_descr|JITFRAME_FIXED_SIZE|opref_to_slot|lifetimes_iter' -S .Repository: youknowone/pyre
Length of output: 33064
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== majit aarch64 setup_input_state & _assemble =="
sed -n '1330,1490p' majit/majit-backend-dynasm/src/aarch64/assembler.rs | cat -n
sed -n '2155,2240p' majit/majit-backend-dynasm/src/aarch64/assembler.rs | cat -n
sed -n '2010,2060p' majit/majit-backend-dynasm/src/aarch64/assembler.rs | cat -n
echo "== majit aarch64 regalloc prepare_bridge, current_frame_loc, longevity =="
sed -n '80,95p' majit/majit-backend-dynasm/src/regalloc.rs | cat -n
sed -n '1876,1930p' majit/majit-backend-dynasm/src/regalloc.rs | cat -n
sed -n '286,305p' majit/majit-backend-dynasm/src/regalloc.rs | cat -n
sed -n '620,655p' majit/majit-backend-dynasm/src/regalloc.rs | cat -n
echo "== x86 parity for same path =="
sed -n '2840,2865p' majit/majit-backend-dynasm/src/x86/assembler.rs | cat -n
sed -n '1648,1670p' majit/majit-backend-dynasm/src/x86/assembler.rs | cat -n
echo "== rust code parser probe =="
python3 - <<'PY'
from pathlib import Path
for p in ['majit/majit-backend-dynasm/src/aarch64/assembler.rs','majit/majit-backend-dynasm/src/x86/assembler.rs']:
text=Path(p).read_text()
for needle in ['fn setup_input_state(&mut self, inputargs: &[InputArg])', 'fn _assemble(&mut self', 'if self.bridge_input_locs.is_some()']:
idx=text.find(needle)
print(p, needle, 'FOUND' if idx!=-1 else 'MISSING', idx)
PYRepository: youknowone/pyre
Length of output: 25397
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== regalloc binding methods =="
sed -n '1,220p' majit/majit-backend-dynasm/src/regalloc.rs | cat -n
echo "== rpython aarch64 comparable path =="
python3 - <<'PY'
from pathlib import Path
for p in [
'rpython/jit/backend/aarch64/assembler.py',
'rpython/jit/backend/aarch64/regalloc.py',
'majit/majit-backend-dynasm/src/aarch64/assembler.rs',
'majit/majit-backend-dynasm/src/regalloc.rs'
]:
s=Path(p).read_text()
print(p, 'setup_input_state', s.find('def setup_input_state')),
print(p, '_assemble bridge sequential loop', s.find('for position iarg in enumerate(inputargs)')),
print(p, 'lifetimes_iter', s.find('lifetimes_iter')),
PYRepository: youknowone/pyre
Length of output: 10045
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== rpython dynasm backends containing setup_input_state / assemble_bridge =="
rg -n 'def setup_input_state|def assemble_bridge|bridge_input_locs|rebuild_faillocs_from_descr|for position, iarg in enumerate\(inputargs\)' rpython/jit/backend -S
echo "== rpython x86 assembler relevant sections =="
rg -n 'def setup_input_state|def assemble_bridge|bridge_input_locs|for position, iarg in enumerate\(inputargs\)|def prepare_bridge' rpython/jit/backend/x86/assembler.py -A 20 -B 5
echo "== rpython x86 regalloc prepare_binding update bindings =="
rg -n 'def prepare_bridge|def .*(binding|bindings)|current_frame_loc|hint_frame_pos|_update_bindings' rpython/jit/backend/x86/regalloc.py -A 25 -B 10 | sed -n '1,220p'Repository: youknowone/pyre
Length of output: 7439
Keep bridge input frame slots from bridge input arg mapping.
The unconditional enumerate() loop writes JITFRAME_FIXED_SIZE + position for every bridge input arg, overwriting the bridge-specific positions set by setup_input_state. The subsequent current_frame_loc loop only updates locations that the regalloc FrameManager actually assigned, so inputs that came from a bridge frame arg can be left mapped by sequential index. Restore or carry forward the original bridge input arg slots before this loop, or apply regalloc frame updates without overwriting inputs that never received a current_frame_loc binding.
🤖 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 `@majit/majit-backend-dynasm/src/aarch64/assembler.rs` around lines 2218 -
2220, Update the input-argument mapping around setup_input_state and the
enumerate loop so bridge input arguments retain their bridge-specific frame
slots. Do not unconditionally assign JITFRAME_FIXED_SIZE + position for every
iarg; preserve existing opref_to_slot entries and only apply sequential slots to
arguments without a bridge mapping, while still allowing current_frame_loc
updates for regalloc-assigned inputs.
| // compile.py:1006-1017 `ResumeFromInterpDescr.compile_and_attach` | ||
| // passes `orig_inputargs` through to `send_loop_to_backend`, which | ||
| // reads the concrete virtualizable before patching the expanded loop | ||
| // entry. Capture both pieces while the originating TraceCtx is still | ||
| // live; after this method returns to `compile_entry_bridge`, an | ||
| // ambient `active_jitdriver_sd` / `vable_ptr` can belong to an inlined | ||
| // callee and is not an admissible substitute for this frame. | ||
| let entry_driver_descriptor = ctx.driver_descriptor().cloned(); | ||
| let entry_orig_vable_ptr = if entry_bridge.is_some() { | ||
| let from_initial_args = entry_driver_descriptor | ||
| .as_ref() | ||
| .and_then(|driver| driver.virtualizable_arg_index()) | ||
| .and_then(|idx| ctx.initial_inputarg_consts.get(idx)) | ||
| .and_then(|value| match value { | ||
| OpRef::ConstPtr(reference) if !reference.is_null() => { | ||
| Some(reference.0 as *const u8) | ||
| } | ||
| _ => None, | ||
| }); | ||
| from_initial_args | ||
| .or_else(|| match ctx.standard_virtualizable_concrete() { | ||
| Some(Value::Ref(reference)) if !reference.is_null() => { | ||
| Some(reference.as_usize() as *const u8) | ||
| } | ||
| _ => None, | ||
| }) | ||
| .or_else(|| ctx.virtualizable_heap_ptr()) | ||
| .unwrap_or(std::ptr::null()) | ||
| } else { | ||
| std::ptr::null() | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reconcile the duplicated vable-pointer resolution with orig_vable_ptr_from_trace_ctx.
This new entry_orig_vable_ptr computation duplicates the existing orig_vable_ptr_from_trace_ctx helper, but adds a null check that the existing helper lacks:
.and_then(|value| match value {
OpRef::ConstPtr(reference) if !reference.is_null() => {
Some(reference.0 as *const u8)
}
_ => None,
})The unmodified orig_vable_ptr_from_trace_ctx (used by compile_loop_body, compile_retrace, finish_and_compile, and compile_simple_loop) has:
.and_then(|const_ref| match const_ref {
OpRef::ConstPtr(gcref) => Some(gcref.0 as *const u8),
_ => None,
})with no null check, and returns immediately on the first Some. If initial_inputarg_consts[idx] happens to hold a null ConstPtr while the trace's virtualizable actually has a valid pointer reachable through standard_virtualizable_concrete() or virtualizable_heap_ptr(), the shared helper returns null before trying those working fallbacks. patch_new_loop_to_load_virtualizable_fields then hits:
assert!(!orig_vable_ptr.is_null(), "patch_new_loop_to_load_virtualizable_fields requires ...");which panics if the trace's inputargs were expanded for virtualizable fields. Fix the shared helper's null check (and have this new code call it, instead of duplicating the logic) so the fix applies to all five call sites consistently.
🐛 Proposed fix for the shared helper
fn orig_vable_ptr_from_trace_ctx(
&self,
ctx: &TraceCtx,
driver_descriptor: Option<&crate::jitdriver::JitDriverStaticData>,
) -> *const u8 {
let from_consts = driver_descriptor
.and_then(|driver| driver.virtualizable_arg_index())
.and_then(|idx| ctx.initial_inputarg_consts.get(idx))
.and_then(|const_ref| match const_ref {
- OpRef::ConstPtr(gcref) => Some(gcref.0 as *const u8),
+ OpRef::ConstPtr(gcref) if !gcref.is_null() => Some(gcref.0 as *const u8),
_ => None,
});
if let Some(ptr) = from_consts {
return ptr;
}🤖 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 `@majit/majit-metainterp/src/pyjitpl.rs` around lines 7254 - 7284, Update the
shared orig_vable_ptr_from_trace_ctx helper to ignore null OpRef::ConstPtr
values before trying standard_virtualizable_concrete() and
virtualizable_heap_ptr() fallbacks. Replace the duplicated entry_orig_vable_ptr
resolution with a call to this helper, preserving the null result when no valid
pointer exists and applying the behavior consistently across all callers.
| |args| { | ||
| crate::type_methods::arity_no_args(args, "__sizeof__")?; | ||
| let word = std::mem::size_of::<usize>() as i64; | ||
| let size = if pyre_object::w_type_is_heaptype(args[0]) { | ||
| // CPython 3.14 typeobject.c:type___sizeof___impl: | ||
| // PyHeapTypeObject plus the cached-keys table carried | ||
| // by a managed instance dictionary. | ||
| 117 * word | ||
| + if pyre_object::w_type_get_hasdict(args[0]) { | ||
| 96 * word | ||
| } else { | ||
| 0 | ||
| } | ||
| } else { | ||
| 52 * word | ||
| }; | ||
| Ok(w_int_new(size)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate the receiver before reading type flags.
The closure reads pyre_object::w_type_is_heaptype(args[0]) and w_type_get_hasdict(args[0]) without checking that args[0] is a type object. type.__sizeof__ is reachable unbound, so type.__sizeof__(1) reaches this body with an int receiver and dereferences it as a W_TypeObject. The sibling members added in this change (type_basicsize_getter and friends) guard through cpython_type_layout, which rejects a non-type. CPython raises TypeError: descriptor '__sizeof__' requires a 'type' object.
🛡️ Proposed guard
|args| {
crate::type_methods::arity_no_args(args, "__sizeof__")?;
+ if args[0].is_null() || !pyre_object::is_type(args[0]) {
+ return Err(crate::PyError::type_error(
+ "descriptor '__sizeof__' requires a 'type' object",
+ ));
+ }
let word = std::mem::size_of::<usize>() as i64;📝 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.
| |args| { | |
| crate::type_methods::arity_no_args(args, "__sizeof__")?; | |
| let word = std::mem::size_of::<usize>() as i64; | |
| let size = if pyre_object::w_type_is_heaptype(args[0]) { | |
| // CPython 3.14 typeobject.c:type___sizeof___impl: | |
| // PyHeapTypeObject plus the cached-keys table carried | |
| // by a managed instance dictionary. | |
| 117 * word | |
| + if pyre_object::w_type_get_hasdict(args[0]) { | |
| 96 * word | |
| } else { | |
| 0 | |
| } | |
| } else { | |
| 52 * word | |
| }; | |
| Ok(w_int_new(size)) | |
| |args| { | |
| crate::type_methods::arity_no_args(args, "__sizeof__")?; | |
| if args[0].is_null() || !pyre_object::is_type(args[0]) { | |
| return Err(crate::PyError::type_error( | |
| "descriptor '__sizeof__' requires a 'type' object", | |
| )); | |
| } | |
| let word = std::mem::size_of::<usize>() as i64; | |
| let size = if pyre_object::w_type_is_heaptype(args[0]) { | |
| // CPython 3.14 typeobject.c:type___sizeof___impl: | |
| // PyHeapTypeObject plus the cached-keys table carried | |
| // by a managed instance dictionary. | |
| 117 * word | |
| if pyre_object::w_type_get_hasdict(args[0]) { | |
| 96 * word | |
| } else { | |
| 0 | |
| } | |
| } else { | |
| 52 * word | |
| }; | |
| Ok(w_int_new(size)) |
🤖 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 10444 - 10460, Validate
args[0] as a type object at the start of the __sizeof__ closure before calling
w_type_is_heaptype or w_type_get_hasdict, reusing the existing
cpython_type_layout validation used by type_basicsize_getter and related
members. Ensure non-type receivers such as type.__sizeof__(1) raise the expected
TypeError while valid type receivers retain the current size calculation.
| pyre_object::listobject::ListStrategy::Empty | ||
| | pyre_object::listobject::ListStrategy::IntOrFloat => { | ||
| // First append can only select Integer, Float, or Object; | ||
| // IntOrFloat is reached later by a numeric strategy transition. | ||
| debug_assert!(matches!( | ||
| strategy, | ||
| pyre_object::listobject::ListStrategy::Empty | ||
| | pyre_object::listobject::ListStrategy::IntOrFloat | ||
| )); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Remove the tautological debug_assert!.
The assertion re-tests the pattern that selected this arm, so it can never fail and compiles to nothing useful. The comment already states the invariant. If the intent is to catch an unexpected caller, assert that this arm is unreachable instead; otherwise drop the assertion.
♻️ Proposed simplification
pyre_object::listobject::ListStrategy::Empty
| pyre_object::listobject::ListStrategy::IntOrFloat => {
// First append can only select Integer, Float, or Object;
// IntOrFloat is reached later by a numeric strategy transition.
- debug_assert!(matches!(
- strategy,
- pyre_object::listobject::ListStrategy::Empty
- | pyre_object::listobject::ListStrategy::IntOrFloat
- ));
+ // No storage transition is emitted for either state.
}📝 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.
| pyre_object::listobject::ListStrategy::Empty | |
| | pyre_object::listobject::ListStrategy::IntOrFloat => { | |
| // First append can only select Integer, Float, or Object; | |
| // IntOrFloat is reached later by a numeric strategy transition. | |
| debug_assert!(matches!( | |
| strategy, | |
| pyre_object::listobject::ListStrategy::Empty | |
| | pyre_object::listobject::ListStrategy::IntOrFloat | |
| )); | |
| } | |
| pyre_object::listobject::ListStrategy::Empty | |
| | pyre_object::listobject::ListStrategy::IntOrFloat => { | |
| // First append can only select Integer, Float, or Object; | |
| // IntOrFloat is reached later by a numeric strategy transition. | |
| // No storage transition is emitted for either state. | |
| } |
🤖 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-jit-trace/src/helpers.rs` around lines 1232 - 1241, Remove the
tautological debug_assert! from the ListStrategy::Empty |
ListStrategy::IntOrFloat match arm in the surrounding helper, since it only
rechecks the pattern that selected the arm. Keep the existing explanatory
comment and arm behavior unchanged; do not replace it unless there is a concrete
unreachable-state assertion for an unexpected caller.
| #[test] | ||
| fn cast_float_to_int_folds_a_const_float_without_recording() { | ||
| let byte = *insns_opname_to_byte() | ||
| .get("cast_float_to_int/f>i") | ||
| .expect("`cast_float_to_int/f>i` must be in insns table"); | ||
| let code = [byte, 0x00, 0x00]; // `f>i`: f-src=0, i-dst=0 | ||
| let mut tc = fresh_trace_ctx(); | ||
| let operand = tc.const_float((42.75f64).to_bits() as i64); | ||
| let mut regs_f = [operand]; | ||
| let mut regs_i = [OpRef::None]; | ||
| let (_, next_pc) = run_float_step(&code, &mut tc, &mut regs_f, &mut regs_i) | ||
| .expect("cast_float_to_int on a const float must fold"); | ||
| assert_eq!(next_pc, 3); | ||
| assert_eq!(tc.num_ops(), 0, "a const operand folds without recording"); | ||
| assert_eq!( | ||
| regs_i[0].inline_const_to_value(), | ||
| Some(majit_ir::Value::Int(42)), | ||
| "dst must hold the folded ConstInt", | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Confirm whether OpRef::None exists as a distinct item =="
rg -n '\bNone\b' --type=rust -g '*majit-ir*' -g '*OpRef*' | rg -i opref || true
rg -n 'enum OpRef|const NONE\s*:\s*OpRef|impl OpRef' -A 10 --type=rust
echo "== All OpRef::None / OpRef::NONE occurrences in the repo =="
rg -n 'OpRef::None\b' --type=rust
rg -n 'OpRef::NONE\b' --type=rust | wc -lRepository: youknowone/pyre
Length of output: 318
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate OpRef definitions/usages =="
rg -n 'enum OpRef|struct OpRef|type OpRef|const NONE\b|fn new|OpRef' --type=rust | head -200
echo
echo "== Search for OpRef::None exact =="
rg -n '\bOpRef::None\b' --type=rust || true
echo
echo "== Search for all None under OpRef-related files if any =="
rg -n '\bNone\b' --type=rust | rg -i 'opref|OpRef::' || trueRepository: youknowone/pyre
Length of output: 412
Use OpRef::NONE for the folded destination operand
OpRef::None is not a valid OpRef item; use OpRef::NONE so this test compiles.
🤖 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-jit-trace/src/jitcode_dispatch/tests.rs` around lines 6883 - 6902,
Update the folded destination initialization in
cast_float_to_int_folds_a_const_float_without_recording to use the valid
OpRef::NONE constant instead of OpRef::None, while preserving the existing
folding assertions.
| #[inline] | ||
| pub unsafe fn w_exception_get_blocking_written_arg(obj: PyObjectRef) -> bool { | ||
| unsafe { (*(obj as *const W_BaseException)).blocking_written_arg } | ||
| } | ||
|
|
||
| #[inline] | ||
| pub unsafe fn w_exception_set_blocking_written_arg(obj: PyObjectRef) { | ||
| unsafe { (*(obj as *mut W_BaseException)).blocking_written_arg = true }; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Add # Safety documentation to the two new unsafe accessors.
w_exception_get_written and w_exception_set_written document their pointer precondition. w_exception_get_blocking_written_arg and w_exception_set_blocking_written_arg have no doc comment. clippy::missing_safety_doc fires on public unsafe functions without a # Safety section.
♻️ Proposed doc addition
+/// Read the constructor-shape flag for an exact `BlockingIOError`.
+///
+/// # Safety
+/// `obj` must point to a valid `W_BaseException`.
#[inline]
pub unsafe fn w_exception_get_blocking_written_arg(obj: PyObjectRef) -> bool {
unsafe { (*(obj as *const W_BaseException)).blocking_written_arg }
}
+/// Mark that the constructor interpreted its third argument as
+/// `characters_written`. The flag is never cleared.
+///
+/// # Safety
+/// `obj` must point to a valid `W_BaseException`.
#[inline]
pub unsafe fn w_exception_set_blocking_written_arg(obj: PyObjectRef) {
unsafe { (*(obj as *mut W_BaseException)).blocking_written_arg = true };
}📝 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.
| #[inline] | |
| pub unsafe fn w_exception_get_blocking_written_arg(obj: PyObjectRef) -> bool { | |
| unsafe { (*(obj as *const W_BaseException)).blocking_written_arg } | |
| } | |
| #[inline] | |
| pub unsafe fn w_exception_set_blocking_written_arg(obj: PyObjectRef) { | |
| unsafe { (*(obj as *mut W_BaseException)).blocking_written_arg = true }; | |
| } | |
| /// Read the constructor-shape flag for an exact `BlockingIOError`. | |
| /// | |
| /// # Safety | |
| /// `obj` must point to a valid `W_BaseException`. | |
| #[inline] | |
| pub unsafe fn w_exception_get_blocking_written_arg(obj: PyObjectRef) -> bool { | |
| unsafe { (*(obj as *const W_BaseException)).blocking_written_arg } | |
| } | |
| /// Mark that the constructor interpreted its third argument as | |
| /// `characters_written`. The flag is never cleared. | |
| /// | |
| /// # Safety | |
| /// `obj` must point to a valid `W_BaseException`. | |
| #[inline] | |
| pub unsafe fn w_exception_set_blocking_written_arg(obj: PyObjectRef) { | |
| unsafe { (*(obj as *mut W_BaseException)).blocking_written_arg = true }; | |
| } |
🤖 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/interp_exceptions.rs` around lines 1399 - 1407, Add Rust
documentation comments with a # Safety section to the public unsafe accessors
w_exception_get_blocking_written_arg and w_exception_set_blocking_written_arg,
documenting the required valid W_BaseException pointer precondition consistently
with the existing w_exception_get_written and w_exception_set_written functions.
| ListStrategy::IntOrFloat => list.int_items.len(), | ||
| ListStrategy::Float => list.float_items.len(), | ||
| } | ||
| } | ||
|
|
||
| /// CPython-visible `PyListObject.allocated` under the list's mutation lock. | ||
| pub unsafe fn w_list_allocated(obj: PyObjectRef) -> isize { | ||
| let _roots = crate::gc_roots::push_roots(); | ||
| let root_base = crate::gc_roots::shadow_stack_len(); | ||
| crate::gc_roots::pin_root(obj); | ||
| let obj = crate::gc_roots::shadow_stack_get(root_base); | ||
| let _list_guard = w_list_lock(obj); | ||
| let obj = crate::gc_roots::shadow_stack_get(root_base); | ||
| (*(obj as *const W_ListObject)).allocated | ||
| } | ||
|
|
||
| /// Reserve CPython's logical slots before `list.extend` consumes its source. | ||
| pub unsafe fn w_list_reserve_for_extend(obj: PyObjectRef, extra: usize) { | ||
| if extra == 0 { | ||
| return; | ||
| } | ||
| let _list_guard = w_list_lock(obj); | ||
| let list = &mut *(obj as *mut W_ListObject); | ||
| let old_size = list.live_len(); | ||
| if list.allocated == 0 { | ||
| list.allocated = ((extra + 1) & !1) as isize; | ||
| } else if let Some(new_size) = old_size.checked_add(extra) { | ||
| list.allocated = list.resized_allocation(old_size, new_size) as isize; | ||
| } | ||
| } | ||
|
|
||
| /// `list_extend_set` / `list_extend_dict` use ordinary `list_resize` even | ||
| /// when the destination has no backing array; unlike sequence-fast extension | ||
| /// they do not call `list_preallocate_exact`. | ||
| pub unsafe fn w_list_resize_for_extend(obj: PyObjectRef, extra: usize) { | ||
| if extra == 0 { | ||
| return; | ||
| } | ||
| let _list_guard = w_list_lock(obj); | ||
| let list = &mut *(obj as *mut W_ListObject); | ||
| let old_size = list.live_len(); | ||
| if let Some(new_size) = old_size.checked_add(extra) { | ||
| list.allocated = list.resized_allocation(old_size, new_size) as isize; | ||
| } | ||
| } | ||
|
|
||
| /// `list_extend_iter_lock_held` trims an overestimated length hint after the | ||
| /// iterator ends, using ordinary `list_resize` shrink rules. | ||
| pub unsafe fn w_list_finish_extend(obj: PyObjectRef) { | ||
| let _list_guard = w_list_lock(obj); | ||
| let list = &mut *(obj as *mut W_ListObject); | ||
| let size = list.live_len(); | ||
| if list.allocated > size as isize { | ||
| list.allocated = list.resized_allocation(size, size) as isize; | ||
| } | ||
| } | ||
|
|
||
| /// Recompute one CPython `list_resize` after a pyre implementation performed | ||
| /// a batch mutation as several primitive removals. | ||
| pub unsafe fn w_list_finish_batch_resize(obj: PyObjectRef, old_size: usize, old_allocated: isize) { | ||
| let _list_guard = w_list_lock(obj); | ||
| let list = &mut *(obj as *mut W_ListObject); | ||
| list.allocated = old_allocated; | ||
| list.sync_allocated(old_size); | ||
| } | ||
|
|
||
| /// Set CPython's raw `PyListObject.allocated` field. `list.sort` uses `-1` | ||
| /// while the saved item array is detached, then restores the previous value. | ||
| pub unsafe fn w_list_set_allocated(obj: PyObjectRef, allocated: isize) { | ||
| let _list_guard = w_list_lock(obj); | ||
| (*(obj as *mut W_ListObject)).allocated = allocated; | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Take the root/reload bracket for the new lock-taking allocation APIs.
w_list_reserve_for_extend, w_list_resize_for_extend, w_list_finish_extend, w_list_finish_batch_resize, and w_list_set_allocated call w_list_lock(obj) and then dereference the pre-lock obj. Every other public locking entry point in this file (w_list_len, w_list_append, w_list_pop) first pins obj with gc_roots::push_roots() / pin_root and reloads it after the lock, because a contended acquire passes through before_external_block, which is a GC safepoint. Use the same bracket here so the five new APIs cannot diverge if the list header ever stops being allocated from the non-moving old generation.
🤖 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/listobject.rs` around lines 1591 - 1663, Update
w_list_reserve_for_extend, w_list_resize_for_extend, w_list_finish_extend,
w_list_finish_batch_resize, and w_list_set_allocated to pin obj with
gc_roots::push_roots()/pin_root and reload it after w_list_lock(obj), matching
w_list_len, w_list_append, and w_list_pop. Use the reloaded post-lock object for
all subsequent dereferences.
| /// listobject.py:2234 IntOrFloatListStrategy.sort and | ||
| /// listobject.py:2449 IntOrFloatSort.lt. Unlike the homogeneous raw-array | ||
| /// accessors above, the encoded `i64` values must be ordered after decoding. | ||
| /// Reverse follows PyPy's reverse/stable-sort/reverse sequence so equal | ||
| /// int/float values retain reverse-sort stability. | ||
| pub unsafe fn w_list_sort_int_or_float(obj: PyObjectRef, reverse: bool) -> bool { | ||
| let list = &mut *(obj as *mut W_ListObject); | ||
| if list.strategy != ListStrategy::IntOrFloat { | ||
| return false; | ||
| } | ||
| let items = list.int_items.as_mut_slice(); | ||
| if reverse { | ||
| items.reverse(); | ||
| } | ||
| items.sort_by(|a, b| { | ||
| int_or_float_as_float(*a) | ||
| .partial_cmp(&int_or_float_as_float(*b)) | ||
| .unwrap_or(std::cmp::Ordering::Equal) | ||
| }); | ||
| if reverse { | ||
| items.reverse(); | ||
| } | ||
| true | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Rust slice sort_by panic if the comparator is not a total order?
💡 Result:
Yes. Rust’s slice::sort_by may panic if the provided comparator does not implement a total order; even if it doesn’t panic, the resulting order is unspecified. The standard library documentation for slice::sort_by states: “Panics: May panic if compare does not implement a total order …; even if the function exits normally, the resulting order of elements in the slice is unspecified.” [1][2] Additionally, Rust’s library issue notes that newer sort implementations were updated specifically to detect Ord/PartialOrd violations and panic, with new “Panics” documentation sections added to reflect this behavior. [3] So, comparator not being a total order is not only a correctness issue—it is explicitly documented as potentially triggering a panic.
Citations:
- 1: https://doc.rust-lang.org/std/primitive.slice.html
- 2: https://doc.rust-lang.org/stable/src/alloc/slice.rs.html
- 3: New panics from sort detecting Ord/PartialOrd violations rust-lang/rust#129561
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
fd -a 'listobject\.rs$' . | sed 's#^\./##'
echo "== relevant function context =="
file="$(fd 'listobject\.rs$' . | head -n 1)"
if [ -n "${file:-}" ]; then
echo "FILE=$file"
wc -l "$file"
sed -n '2040,2125p' "$file" | cat -n
fi
echo "== find encoding/strategy-related definitions =="
rg -n "int_or_float_encode_float|IntOrFloat|ListStrategy|sort_int_or_float|partial_cmp|0xfffffffe|0x7ff8" -S .Repository: youknowone/pyre
Length of output: 50374
🌐 Web query:
Rust slice::sort_by panic total order documentation
💡 Result:
In Rust, the slice sorting methods—including slice::sort_by, slice::sort, and others—require that the provided comparator (or the type's Ord implementation) defines a strict total order [1][2][3]. If the comparison function fails to implement a total order, the standard library sorting implementations may panic [1][4][5]. Even if the function does not panic and exits normally, the resulting order of elements in the slice is considered unspecified [1][2][5]. Key details regarding this behavior include: 1. Requirement for Total Order: A valid comparator must satisfy the properties of a total order: totality, antisymmetry, and transitivity [3]. Violating these requirements is a logic error [3]. 2. Panic Behavior: Beginning with Rust 1.81, the standard library's sort implementations include checks that can detect violations of the total order requirement [4][6]. If such a violation is detected during the sorting process, the function may panic [1][4]. 3. Unspecified Output: Historically and currently, if a sort algorithm encounters an invalid ordering and does not panic, the output remains unspecified [4][5]. Users should not rely on any particular output when the total order requirement is violated [4]. 4. Floating-Point Types: Because types like f32 and f64 do not implement Ord (due to NaN values violating reflexivity), they do not provide a total order by default [2][3]. When sorting such types, it is standard practice to use functions that explicitly define a total order, such as f32::total_cmp or f64::total_cmp [1][2][3]. The introduction of these panics in newer versions of Rust is intended to assist developers in identifying logic bugs in their comparison functions rather than allowing sorting to proceed with inconsistent results [4][6].
Citations:
- 1: https://doc.rust-lang.org/stable/src/alloc/slice.rs.html
- 2: https://github.com/rust-lang/rust/blob/master/library/alloc/src/slice.rs
- 3: https://doc.rust-lang.org/stable/std/cmp/trait.Ord.html
- 4: New panics from sort detecting Ord/PartialOrd violations rust-lang/rust#129561
- 5: unstable_sort_by vs panics rust-lang/rust#136665
- 6: Improve
Ordviolation help rust-lang/rust#128273
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd 'listobject\.rs$' . | head -n 1)"
echo "FILE=$file"
if [ -z "${file:-}" ]; then
echo "listobject.rs not found"
exit 1
fi
echo "== function context =="
sed -n '2052,2112p' "$file" | cat -n
echo "== relevant definitions and encoders =="
rg -n "enum ListStrategy|struct IntOrFloat|int_or_float_encode_float|int_or_float_as_float|0xfffffffe|int_or_float_compare|sort_int_or_float" "$file" -C 3
echo "== py/relevant source references if present =="
rg -n "IntOrFloat|Sort|sort" pyre/pyre-object | head -n 200Repository: youknowone/pyre
Length of output: 263
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tracked listobject files =="
git ls-files | rg '(^|/)listobject\.(rs|py)$|listobject' | head -n 100
echo "== current branch diff stat =="
git diff --stat || true
echo "== git status files =="
git status --short | head -n 200Repository: youknowone/pyre
Length of output: 503
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== pyre-object listobject outline/matches =="
fd -e rs 'listobject\.rs' . | while read -r f; do
echo "--- $f"
rg -n "struct IntOrFloat|int_or_float_encode_float|int_or_float_as_float|0xfffffffe|0x7ff8|sort_int_or_float|int_or_float_compare" "$f" -C 4 || true
done
echo "== pyre-interpreter listobject.rs around lines =="
wc -l pyre/pyre-interpreter/src/listobject.rs
sed -n '1,260p' pyre/pyre-interpreter/src/listobject.rs | cat -nRepository: youknowone/pyre
Length of output: 8903
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== listobject.py IntOrFloatSort definitions =="
rg -n "class IntOrFloatSort|def lt|def __lt__|IntOrFloatListStrategy" pypy/objspace/std/listobject.py -C 8
echo "== pyre-object constants and encoder/decorator context =="
sed -n '70,105p' pyre/pyre-object/src/listobject.rs | cat -n
sed -n '558,592p' pyre/pyre-object/src/listobject.rs | cat -n
sed -n '2080,2106p' pyre/pyre-object/src/listobject.rs | cat -n
echo "== Rust sort_by docs check via rustc docs tool, if available =="
rustc --print sysroot >/dev/null 2>&1 || true
RUSTDOC_FLAGS= rustc -Z unstable-options --print sysroot >/dev/null 2>&1 || true
if command -v rustdoc >/dev/null 2>&1; then
rustdoc --print sysroot >/dev/null || true
rustdoc --version >/dev/null || true
rustdoc --crate-name alloc /path 2>&1 | sed -n '1,20p' || true
fiRepository: youknowone/pyre
Length of output: 13016
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Python Float.lt behavior probe =="
python3 - <<'PY'
for a in (1.0, float("nan"), 2.0):
for b in (1.0, float("nan"), 2.0):
print(f"{a!r} < {b!r} -> {a < b}")
PY
echo "== Python IntOrFloatSort simulation probe =="
python3 - <<'PY'
from functools import cmp_to_key
def cmp(a, b):
fa = a if isinstance(a, float) else float(a)
fb = b if isinstance(b, float) else float(b)
if fa == fb:
return 0
return -1 if fa < fb else 1
for seq in [[float("nan"), 1.0, 2.0], [3.0, float("nan"), 1.0], [1.0, float("nan"), 2.0, float("nan"), 3.0]]:
print(seq, "=>", sorted(seq, key=cmp_to_key(cmp)))
PY
echo "== Rust sort_by does not require Ord == total order; check for existing unsafe/panic docs =="
rg -n "sort_by|Panics|total order|partial_cmp|Ordering::Equal|std::cmp::Ordering::Equal" pyre -S | head -n 120Repository: youknowone/pyre
Length of output: 3845
🌐 Web query:
Rust current std::slice::sort_by comparator must be total order panic documentation
💡 Result:
Beginning with Rust 1.81, the standard library sorting functions, including slice::sort_by, were updated to include enhanced safety checks that may result in panics when a total order is not maintained [1][2]. Key points regarding these panics: - Documentation: The official documentation for slice::sort_by now explicitly states that the function may panic if the provided comparator does not implement a total order [3][4]. - Behavior: Previously, using an invalid comparator that failed to define a total order resulted in an unspecified output order but did not panic [1][2]. In current versions, if the implementation detects an Ord or comparator violation, it may panic, or alternatively, complete execution with an unspecified element order [3][4]. - Reason: These checks were introduced as part of new sorting implementations to catch violations of the total order requirement, which is a fundamental precondition for the correctness of the sorting algorithms used by the standard library [1][5]. - Total Order Requirement: For any comparison function (or Ord implementation) used in sorting, the implementation must provide a consistent total ordering [6][7]. If the logic violates this (e.g., inconsistencies like a > b and b > a, or failing to remain consistent with PartialOrd where applicable), the function is now liable to panic at runtime [3][7]. If you encounter this panic, it indicates that the comparator logic is inconsistent [7]. Developers are encouraged to ensure that their comparison functions strictly adhere to the properties of a total order (reflexivity, antisymmetry, and transitivity) [6][5]. For types that do not naturally support a total order, such as floating-point numbers (f32, f64), developers should use methods like f32::total_cmp, which are designed to provide a valid total ordering [3][7].
Citations:
- 1: New panics from sort detecting Ord/PartialOrd violations rust-lang/rust#129561
- 2: New panics from sort detecting Ord/PartialOrd violations rust-lang/rust#129561
- 3: https://doc.rust-lang.org/stable/src/alloc/slice.rs.html
- 4: petrochenkov/rust@8a8c0e2
- 5: Improve
Ordviolation help rust-lang/rust#128273 - 6: https://users.rust-lang.org/t/too-severe-precondition-for-slice-sort-by/131953
- 7: https://users.rust-lang.org/t/panic-on-sort-of-f32-s-with-f32-total-cmp-in-1-81-0/117675
Make the IntOrFloat sort comparator a total order.
int_or_float_encode_float accepts quiet NaN, so items.sort_by can compare NaN as equal to every other element. That violates Rust’s total-order pre-condition for sort_by and can abort instead of matching PyPy’s arbitrary-but-stable IntOrFloatSort.lt output. Order NaN deterministically, while preserving the existing lt ordering for non-NaN values.
🤖 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/listobject.rs` around lines 2080 - 2104, Update
w_list_sort_int_or_float’s comparator to impose a deterministic total order for
NaN values instead of mapping partial_cmp failures to Equal. Preserve the
existing numeric ordering for non-NaN int/float values and ensure equal values
retain stable-sort behavior, including through the existing reverse handling.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/bfa00bc8a8d5717198f13c5ecf526e5e121109a0/pyre-interpreter/src/eval.rs#L1038-L1040
When user code retains f = g().gi_frame for an ordinary generator whose body does not yield inside a try, that generator is not registered with UserDelAction; after a major collection sweeps the owner, omitting f_generator_nowref from the walkers neither clears nor weakens the raw pointer. frame.f_generator then returns a dangling object pointer instead of None (CPython 3.14 clears it), potentially causing a use-after-free on subsequent access. Store this backlink through the existing weak-reference machinery or invalidate it whenever any generator is swept.
AGENTS.md reference: AGENTS.md:L231-L232
https://github.com/youknowone/pyre/blob/bfa00bc8a8d5717198f13c5ecf526e5e121109a0/pyre-interpreter/src/module/sys/vm.rs#L63-L66
For untracked CPython types that pyre can allocate through either storage path, this makes sys.getsizeof depend on collector ownership rather than type semantics. Fresh evidence in the updated code is the replacement of the earlier heap-type test with try_gc_owns_object: an exact string produced by w_str_new_managed under gc_interp receives a 16-byte header while the same exact str produced by off-GC w_str_new does not, even though CPython strings are untracked and must have identical sizing. Use logical Py_TPFLAGS_HAVE_GC-equivalent metadata instead of physical pyre allocation ownership.
https://github.com/youknowone/pyre/blob/bfa00bc8a8d5717198f13c5ecf526e5e121109a0/pyre-jit-trace/src/jitcode_dispatch/arith.rs#L793-L798
When generated jitcode casts a NaN, infinity, or out-of-range finite float, execute_cast_const deliberately declines folding, so this new path records CastFloatToInt while stamping the concrete result with Rust's saturating f as i64 semantics. The native x86 backend still emits bare cvttsd2si, which returns i64::MIN for invalid conversions rather than Rust's 0 for NaN and i64::MAX for positive overflow; compiled execution can therefore diverge from the blackhole interpreter and from downstream decisions made using the stamped concrete value. Emit the required saturation sequence in native backends or decline this trace shape.
AGENTS.md reference: AGENTS.md:L14-L18
https://github.com/youknowone/pyre/blob/bfa00bc8a8d5717198f13c5ecf526e5e121109a0/pyre-interpreter/src/typedef.rs#L10343-L10347
For every builtin payload not explicitly listed above, this fallback advertises the two-word object layout even when the type has a distinct native representation. On 64-bit CPython 3.14, for example, function.__basicsize__, module.__basicsize__, property.__basicsize__, and BaseException.__basicsize__ are 152, 56, 64, and 72 respectively, while this path reports 16; their dict/weakref offsets also fall through to zero, and inherited object.__sizeof__ uses the same bad base. Reserve this fallback for genuine ordinary instance layouts and add metadata for each custom builtin layout before exposing the descriptors.
ℹ️ 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/extra_tests/parity_tests/type_new_metatype_guard.py`:
- Around line 174-188: Update the two expect_value calls named
ctypes_metatype_is_a_type and ctypes_metatype_subtypes_type to pass the boolean
True using the value keyword argument, preserving their existing assertions and
clearing Ruff FBT003.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e03f93c2-366a-4dad-a010-59b441b4b2a3
📒 Files selected for processing (3)
pyre/extra_tests/parity_tests/open_non_inheritable.pypyre/extra_tests/parity_tests/socket_timeout_identity.pypyre/extra_tests/parity_tests/type_new_metatype_guard.py
| # type objects. Pyre's real ctypes types are currently the POSIX + host_env | ||
| # implementation; non-POSIX builds deliberately expose only an import stub. | ||
| if os.name == "posix": | ||
| import ctypes # noqa: E402 | ||
|
|
||
| CTYPES_META = type(ctypes.py_object) | ||
|
|
||
| expect_value("ctypes_metatype_is_a_type", lambda: isinstance(CTYPES_META, type), True) | ||
| expect_value("ctypes_metatype_subtypes_type", lambda: issubclass(CTYPES_META, type), True) | ||
| expect_value("ctypes_subclass_name", lambda: type("P", (ctypes.py_object,), {}).__name__, "P") | ||
| expect_value( | ||
| "ctypes_subclass_metatype", | ||
| lambda: type(type("P", (ctypes.py_object,), {})), | ||
| CTYPES_META, | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use a keyword for the boolean expected values.
Ruff reports FBT003 on Line 181 and Line 182. Pass True as value=True to preserve the assertion and clear the warning.
Proposed fix
- expect_value("ctypes_metatype_is_a_type", lambda: isinstance(CTYPES_META, type), True)
- expect_value("ctypes_metatype_subtypes_type", lambda: issubclass(CTYPES_META, type), True)
+ expect_value("ctypes_metatype_is_a_type", lambda: isinstance(CTYPES_META, type), value=True)
+ expect_value("ctypes_metatype_subtypes_type", lambda: issubclass(CTYPES_META, type), value=True)📝 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.
| # type objects. Pyre's real ctypes types are currently the POSIX + host_env | |
| # implementation; non-POSIX builds deliberately expose only an import stub. | |
| if os.name == "posix": | |
| import ctypes # noqa: E402 | |
| CTYPES_META = type(ctypes.py_object) | |
| expect_value("ctypes_metatype_is_a_type", lambda: isinstance(CTYPES_META, type), True) | |
| expect_value("ctypes_metatype_subtypes_type", lambda: issubclass(CTYPES_META, type), True) | |
| expect_value("ctypes_subclass_name", lambda: type("P", (ctypes.py_object,), {}).__name__, "P") | |
| expect_value( | |
| "ctypes_subclass_metatype", | |
| lambda: type(type("P", (ctypes.py_object,), {})), | |
| CTYPES_META, | |
| ) | |
| # type objects. Pyre's real ctypes types are currently the POSIX + host_env | |
| # implementation; non-POSIX builds deliberately expose only an import stub. | |
| if os.name == "posix": | |
| import ctypes # noqa: E402 | |
| CTYPES_META = type(ctypes.py_object) | |
| expect_value("ctypes_metatype_is_a_type", lambda: isinstance(CTYPES_META, type), value=True) | |
| expect_value("ctypes_metatype_subtypes_type", lambda: issubclass(CTYPES_META, type), value=True) | |
| expect_value("ctypes_subclass_name", lambda: type("P", (ctypes.py_object,), {}).__name__, "P") | |
| expect_value( | |
| "ctypes_subclass_metatype", | |
| lambda: type(type("P", (ctypes.py_object,), {})), | |
| CTYPES_META, | |
| ) |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 181-181: Boolean positional value in function call
(FBT003)
[warning] 182-182: Boolean positional value in function call
(FBT003)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/extra_tests/parity_tests/type_new_metatype_guard.py` around lines 174 -
188, Update the two expect_value calls named ctypes_metatype_is_a_type and
ctypes_metatype_subtypes_type to pass the boolean True using the value keyword
argument, preserving their existing assertions and clearing Ruff FBT003.
Source: Linters/SAST tools
1 participant
21 commits rebased onto
1a63e36e37a, plus one formatting commit.Sizeof / layout metadata
str,int,type,bytearray,listandweakrefgain the CPython 3.14size and allocation metadata they were missing.
listandbytearraycarryPyListObject.allocatedandlist_resize's over-allocation calculation, sosys.getsizeofreports the allocated capacity rather than the logical length.coroutine, generator, async-for, copy, io, socket, exceptions
coroutine: yield-from and exhausted state survive; an unawaited finalizer'serror is reported.
generator: a fresh frame is cleared throughgi_frame; a consumed closeexception is released.
async for: an invalid__anext__awaitable chains its cause.copy: a type without a constructor is rejected.io: opened descriptors are non-inheritable.socket.timeoutaliases the builtinTimeoutError.BlockingIOError.characters_writtenis backed by a real slot.__builtins__from the caller frame.jit
LOAD_GLOBAL's null is preserved in guard snapshots.Rebase note
list: port CPython allocation metadataconflicted with #1119, whichgeneralized the FBW undo log from
Vec<(list, len, allocated)>toVec<FbwListEffect>and dropped theallocatedelement in the process. Theresolution keeps #1119's
FbwListEffectand restoresallocated_beforeas afield on its
Appendvariant.sync_allocatedsits in the outerw_list_pop_end, not inw_list_pop_end_inner, so #1119's foldable guard-freeinner body is untouched — the same split
w_list_appendalready uses.Known gap, not addressed here: the jit descends
w_list_pop_end_innerdirectly, so a folded
list.pop()does not runsync_allocated. When a listshrinks past half its allocation the interpreted and compiled paths can report
different
sys.getsizeof.— opened by Claude
Summary by CodeRabbit
New Features
sys.getsizeof()behavior and object layout support.__slots__validation.Bug Fixes
Tests