builtins: type-creation and product validation, generic-alias and struct rooting, SimpleNamespace and UnionType parity - #923
Conversation
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughThe interpreter now roots objects across allocating calls, updates class construction and C3 validation, expands ChangesInterpreter runtime safety and semantics
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit a231d3a). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
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
https://github.com/youknowone/pyre/blob/103c2b30396e8f2d53efe9422d68b8efab2f767c/pyre-interpreter/src/module/sys/vm.rs#L91-L94
When a SimpleNamespace subclass shadows __dict__ with a property or custom __getattribute__, this lookup executes that hook and treats its return value as the update destination. Consequently, Subclass(a=1) can update a temporary dictionary and return without an a attribute, while a non-dict return can reach dict-layout operations; the target CPython 3.14 implementation updates the namespace's internal dict directly and does not consult these hooks.
https://github.com/youknowone/pyre/blob/103c2b30396e8f2d53efe9422d68b8efab2f767c/pyre-interpreter/src/module/sys/vm.rs#L484-L487
When copying the source dictionary invokes Python—for example, through a str subclass key with an allocating __hash__—namespace_update_dict can trigger the moving collector. The outer shadow-stack slot is updated, but the raw result local passed here and returned below is not, so the keyword overlay or final return can use an evacuated pointer; reload the result slot after each potentially collecting call.
https://github.com/youknowone/pyre/blob/103c2b30396e8f2d53efe9422d68b8efab2f767c/pyre-interpreter/src/module/sys/vm.rs#L322-L324
When the public namespace dictionary contains a str subclass whose __hash__ allocates, getitem runs that Python method and may move the key. Its rooted slot is updated, but the local key used by py_str remains the pre-collection pointer, causing an invalid dereference while rendering an otherwise valid namespace; reread keys_sp + i after the lookup.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6bc7c0d420
ℹ️ 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 r_format = pyre_object::gc_roots::shadow_stack_get(sp); | ||
| let r_buffer = pyre_object::gc_roots::shadow_stack_get(sp + 1); | ||
| let buf = unsafe { readbuf(r_buffer)? }; |
There was a problem hiding this comment.
When readbuf invokes a Python-level __buffer__ hook that triggers a moving collection, r_format and r_buffer retain the addresses read before that call even though their shadow-stack slots are updated. The stale r_buffer is subsequently passed to buffer_export_incref, and the stale r_format is stored in the iterator, potentially causing invalid memory access or a corrupt iterator; reload both values from their slots after the hook and when constructing the payload.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
| let destination = crate::baseobjspace::getattr_str( | ||
| pyre_object::gc_roots::shadow_stack_get(sp), | ||
| "__dict__", | ||
| )?; |
There was a problem hiding this comment.
__dict__
For a SimpleNamespace subclass that overrides __dict__, this lookup returns the override rather than the namespace's backing instance dict. Construction and __replace__ then update that temporary mapping instead of the object's state; if the override returns a non-dict, the unchecked dict operations can also interpret the wrong object layout. CPython accesses ns_dict directly, and the previous implementation wrote through setdictvalue_native, so this helper must obtain the backing dict without attribute dispatch.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
| "__mro_entries__", | ||
| make_builtin_function("__mro_entries__", union_mro_entries_method), |
There was a problem hiding this comment.
__mro_entries__ argument count
Because this is registered with the arity-less wrapper and the implementation only reads args[0], both (int | str).__mro_entries__() and calls with extra arguments reach the Cannot subclass error instead of raising the required missing/extra-argument TypeError. Register it with fixed arity 2 (bound self plus orig_bases); the newly added __repr__ and __hash__ registrations have the analogous extra-argument problem.
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: 3
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/struct/mod.rs (1)
1363-1383: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winReread both slots after
readbuf;r_formatandr_bufferare captured before the collecting call.Lines 1363-1364 read both slots into plain Rust locals. Line 1365 then calls
readbuf, which the new comment states may dispatch a Python-level__buffer__hook. That call can collect and move objects. The collector rewrites the shadow-stack slots, but it does not rewriter_formatandr_buffer, because those are Rust locals outside the root set.Two later sites use those stale-capable copies:
- Line 1376 passes
r_buffertobuffer_export_incref. That function performs type checks and increments an exporter count through the pointer, so a moved buffer gets its lease recorded on the old header.- Line 1382 stores
r_formatinto the stable allocation.__next__later callsw_str_get_value(self.format)on that field.Line 1383 already rereads
shadow_stack_get(sp + 1)for thebufferfield. That asymmetry indicates the reread was intended at every post-readbufuse. Read both slots afterreadbufinstead of before it.🐛 Proposed fix to reread both roots after the collecting call
let _roots = pyre_object::gc_roots::push_roots(); let sp = pyre_object::gc_roots::shadow_stack_len(); pyre_object::gc_roots::pin_root(format); pyre_object::gc_roots::pin_root(buffer); - let r_format = pyre_object::gc_roots::shadow_stack_get(sp); - let r_buffer = pyre_object::gc_roots::shadow_stack_get(sp + 1); - let buf = unsafe { readbuf(r_buffer)? }; + let buf = unsafe { readbuf(pyre_object::gc_roots::shadow_stack_get(sp + 1))? }; if size <= 0 { return Err(struct_error(format!( "cannot iteratively unpack with a struct of length {size}" ))); } if buf.len() as i64 % size != 0 { return Err(struct_error(format!( "iterative unpacking requires a buffer of a multiple of {size} bytes" ))); } - let export_active = unsafe { crate::builtins::buffer_export_incref(r_buffer) }; + let export_active = unsafe { + crate::builtins::buffer_export_incref(pyre_object::gc_roots::shadow_stack_get(sp + 1)) + }; let w_iter = W_UnpackIter::allocate_stable(W_UnpackIter { ob: pyre_object::PyObject { ob_type: std::ptr::null(), w_class: std::ptr::null_mut(), }, - format: r_format, + format: pyre_object::gc_roots::shadow_stack_get(sp), buffer: pyre_object::gc_roots::shadow_stack_get(sp + 1),🤖 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/struct/mod.rs` around lines 1363 - 1383, Reread both shadow-stack roots after the collecting `readbuf(r_buffer)?` call and before any subsequent use. Update the `buffer_export_incref` call to use the reread buffer root and store the reread format root in `W_UnpackIter.format`; keep the existing `shadow_stack_get(sp + 1)` buffer assignment consistent with these refreshed values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyre/pyre-interpreter/src/module/sys/vm.rs`:
- Around line 306-331: Reload the key from its shadow-stack slot after
crate::baseobjspace::getitem returns and before formatting it with
crate::display::py_str. Replace the stale local key use in the parts.push block,
while preserving the existing value reload and error-handling behavior.
- Around line 472-491: Reload the GC-rooted result object after every collecting
call in the surrounding function: after getattr_str and each
namespace_update_dict invocation, fetch the current result pointer from its
shadow-stack slot before the next use or return. Update the result argument
passed to both namespace_update_dict calls and return the reloaded value,
preserving the existing update order and kwargs behavior.
- Around line 375-386: Update the namespace rich-comparison implementation
around the existing other_type check to also validate self_obj is a
SimpleNamespace before accessing either __dict__. Return NotImplemented when
either operand is not a namespace instance, while preserving the current
six-operation dict comparison behavior for valid operands.
---
Outside diff comments:
In `@pyre/pyre-interpreter/src/module/struct/mod.rs`:
- Around line 1363-1383: Reread both shadow-stack roots after the collecting
`readbuf(r_buffer)?` call and before any subsequent use. Update the
`buffer_export_incref` call to use the reread buffer root and store the reread
format root in `W_UnpackIter.format`; keep the existing `shadow_stack_get(sp +
1)` buffer assignment consistent with these refreshed values.
🪄 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: 9f1442e0-83bf-4b1d-a647-8b5da4774328
📒 Files selected for processing (5)
pyre/pyre-interpreter/src/_pypy_generic_alias.rspyre/pyre-interpreter/src/call.rspyre/pyre-interpreter/src/module/struct/mod.rspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-interpreter/src/typedef.rs
| let other_type = crate::typedef::r#type(other) | ||
| .map(|tp| tp.as_ptr()) | ||
| .unwrap_or(PY_NULL); | ||
| if !unsafe { crate::baseobjspace::issubtype_w(other_type, simple_namespace_type()) } { | ||
| return Ok(w_not_implemented()); | ||
| } | ||
| // `self.__dict__ == other.__dict__` — read through the descriptor so a | ||
| // subclass `__dict__` override is honoured, as PyPy's attribute access is. | ||
| // CPython 3.14 forwards all six operations to the two namespace dicts. | ||
| // In particular, ordering reaches dict's TypeError instead of returning | ||
| // NotImplemented from the namespace type itself. | ||
| let self_dict = crate::baseobjspace::getattr_str(self_obj, "__dict__")?; | ||
| let other_dict = crate::baseobjspace::getattr_str(other, "__dict__")?; | ||
| let equal = crate::baseobjspace::eq_w(self_dict, other_dict)?; | ||
| Ok(w_bool_from(equal ^ negate)) | ||
| crate::baseobjspace::compare(self_dict, other_dict, op) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Check the type of self as well as other.
CPython namespace_richcompare requires both operands to be namespace instances and returns NotImplemented otherwise. This port validates only other. An unbound call such as types.SimpleNamespace.__eq__(1, ns) therefore reaches getattr_str(self_obj, "__dict__") and raises AttributeError, where CPython returns NotImplemented.
The ordering comment is correct: CPython forwards all six operations to the two namespace dicts, and the reported error names dict for ordering (a < b raises TypeError: '<' not supported between instances of 'dict' and 'dict').
🐛 Proposed fix to validate both operands
+ let self_type = crate::typedef::r#type(self_obj)
+ .map(|tp| tp.as_ptr())
+ .unwrap_or(PY_NULL);
let other_type = crate::typedef::r#type(other)
.map(|tp| tp.as_ptr())
.unwrap_or(PY_NULL);
- if !unsafe { crate::baseobjspace::issubtype_w(other_type, simple_namespace_type()) } {
+ if !unsafe { crate::baseobjspace::issubtype_w(self_type, simple_namespace_type()) }
+ || !unsafe { crate::baseobjspace::issubtype_w(other_type, simple_namespace_type()) }
+ {
return Ok(w_not_implemented());
}📝 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.
| let other_type = crate::typedef::r#type(other) | |
| .map(|tp| tp.as_ptr()) | |
| .unwrap_or(PY_NULL); | |
| if !unsafe { crate::baseobjspace::issubtype_w(other_type, simple_namespace_type()) } { | |
| return Ok(w_not_implemented()); | |
| } | |
| // `self.__dict__ == other.__dict__` — read through the descriptor so a | |
| // subclass `__dict__` override is honoured, as PyPy's attribute access is. | |
| // CPython 3.14 forwards all six operations to the two namespace dicts. | |
| // In particular, ordering reaches dict's TypeError instead of returning | |
| // NotImplemented from the namespace type itself. | |
| let self_dict = crate::baseobjspace::getattr_str(self_obj, "__dict__")?; | |
| let other_dict = crate::baseobjspace::getattr_str(other, "__dict__")?; | |
| let equal = crate::baseobjspace::eq_w(self_dict, other_dict)?; | |
| Ok(w_bool_from(equal ^ negate)) | |
| crate::baseobjspace::compare(self_dict, other_dict, op) | |
| let self_type = crate::typedef::r#type(self_obj) | |
| .map(|tp| tp.as_ptr()) | |
| .unwrap_or(PY_NULL); | |
| let other_type = crate::typedef::r#type(other) | |
| .map(|tp| tp.as_ptr()) | |
| .unwrap_or(PY_NULL); | |
| if !unsafe { crate::baseobjspace::issubtype_w(self_type, simple_namespace_type()) } | |
| || !unsafe { crate::baseobjspace::issubtype_w(other_type, simple_namespace_type()) } | |
| { | |
| return Ok(w_not_implemented()); | |
| } | |
| // CPython 3.14 forwards all six operations to the two namespace dicts. | |
| // In particular, ordering reaches dict's TypeError instead of returning | |
| // NotImplemented from the namespace type itself. | |
| let self_dict = crate::baseobjspace::getattr_str(self_obj, "__dict__")?; | |
| let other_dict = crate::baseobjspace::getattr_str(other, "__dict__")?; | |
| crate::baseobjspace::compare(self_dict, other_dict, op) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-interpreter/src/module/sys/vm.rs` around lines 375 - 386, Update
the namespace rich-comparison implementation around the existing other_type
check to also validate self_obj is a SimpleNamespace before accessing either
__dict__. Return NotImplemented when either operand is not a namespace instance,
while preserving the current six-operation dict comparison behavior for valid
operands.
Source: Coding guidelines
The loop skipped them, so `type('C', (object, object()), {})` built a
class whose MRO contained a plain instance; `compute_mro` appends every
supplied base and attribute lookup then reads that entry as a type
layout. `best_base` raises `bases must be types` in the same loop.
`__bases__` assignment screens its tuple earlier and keeps its own
message.
Assisted-by: Claude
`product_descr_new` reached `check_user_subclass` only through `itertools_alloc_for_class`, after the keyword census, the `repeat` conversion and a full pass over the input iterables. An unbound call such as `itertools.product.__new__(int, gen())` therefore consumed the iterable and could report an unrelated error first; `tp_new_wrapper` screens the subtype before `product_new` runs. Assisted-by: Claude
…ution `newargs`, `old_arg` and the owner arguments were raw `PyObjectRef`s in Rust locals across `__typing_prepare_subst__`, `__typing_subst__`, the recursive descent into nested lists and tuples, and each `w_tuple_new` / `w_list_new` — all of which allocate or run Python. The collector moves objects and does not scan Rust locals. Keeps them on the shadow stack and rereads them after anything that can collect; produced arguments are held as slots so an entry survives the later iterations' allocations. Assisted-by: Claude
`W_Product__new__` extracts `repeat` and raises for a leftover keyword before `allocate_instance` screens the subtype, so `product.__new__(int, gen(), bogus=1)` reports the keyword, not the class. Placing the check first inverted that pair. The check still precedes `W_Product.__init__`, so the `repeat` conversion and the pass over the input iterables stay behind it and `product.__new__(int, gen())` leaves `gen()` unconsumed. Assisted-by: Claude
`find_best_base` skipped a non-type base and nothing downstream looked at
it again: `compute_mro` appends every supplied base, so
`type('C', (object, object()), {})` built a class whose MRO held a plain
instance and attribute lookup then read that entry as a type layout.
`find_best_base` (typeobject.py:1341-1342) is not where such a base is
rejected — it belongs to `get_mro` (typeobject.py:1680-1684), whose
non-`W_TypeObject` branch walks the base with `abstract_mro`
(typeobject.py:1665-1678) and reads `__bases__` off it. Port that branch
into `validate_c3_mro`, the fallible front door pyre already routes the
C3 merge through, and restore the `continue`.
`setup_user_defined_type` reaches `compute_mro` only after
`check_and_find_best_base` accepted the tuple, so the classic walk is
gated on some base being a type; an all-classic tuple keeps
`check_and_find_best_base`'s own message.
`abstract_mro` reads `__bases__`, which can run `__getattr__` and
allocate, so the C3 list build now accumulates shadow-stack slots and
materializes them only for the merge, which allocates nothing.
Assisted-by: Claude
`union_getitem` reduced `subs_parameters`' result with `curr |= newargs[i]` while both the accumulator and the remaining members lived in a plain Rust `Vec`: `subs_parameters` releases its root scope on the way out, and every `|` dispatches `__or__`/`__ror__` and allocates a fresh union, so from the first step on the loop held pre-relocation addresses. _pypy_generic_alias.py:315-323 folds an app-level `newargs` list the collector traces. Pin the members and reread the accumulator and the next member from their slots after each step. Assisted-by: Claude
`unpack_iter` pinned `format` and `buffer` and then kept reading the values it had captured before `readbuf` — which may dispatch a Python-level `__buffer__` hook — so `buffer_export_incref` and the iterator's `format` field both named pre-relocation addresses. Read both slots back after the hooks instead, as the `buffer` field already did. Assisted-by: Claude
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
pyre/pyre-interpreter/src/module/sys/vm.rs (3)
375-386: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCheck the type of
selfas well asother.CPython
namespace_richcomparerequires both operands to be namespace instances and returnsNotImplementedotherwise. This port validates onlyother. An unbound call such astypes.SimpleNamespace.__eq__(1, ns)reachesgetattr_str(self_obj, "__dict__")at line 384 and raisesAttributeError, where CPython returnsNotImplemented.🐛 Proposed fix to validate both operands
+ let self_type = crate::typedef::r#type(self_obj) + .map(|tp| tp.as_ptr()) + .unwrap_or(PY_NULL); let other_type = crate::typedef::r#type(other) .map(|tp| tp.as_ptr()) .unwrap_or(PY_NULL); - if !unsafe { crate::baseobjspace::issubtype_w(other_type, simple_namespace_type()) } { + if !unsafe { crate::baseobjspace::issubtype_w(self_type, simple_namespace_type()) } + || !unsafe { crate::baseobjspace::issubtype_w(other_type, simple_namespace_type()) } + { return Ok(w_not_implemented()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyre/pyre-interpreter/src/module/sys/vm.rs` around lines 375 - 386, Update the rich-comparison implementation around the existing other_type check to validate self_obj is also a SimpleNamespace before accessing either operand’s __dict__. Return w_not_implemented() when either operand is not a namespace instance, while preserving the existing namespace-dictionary comparison for valid operands.Source: Coding guidelines
306-331: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winRe-read
keyaftergetitem.Line 307 reads
keyfrom the shadow stack. Line 313 callscrate::baseobjspace::getitem, which can hash the key, run Python code, and allocate. A moving collection during that call invalidates the localkeypointer. Line 324 passes the stale pointer topy_str. The value is already reloaded at lines 326-328, so only the key lacks the reload.🐛 Proposed fix to reload the key slot
pyre_object::gc_roots::pin_root(value); + // `getitem` above is a collection point; reload the key from its + // root slot instead of using the pre-call pointer. + let key = pyre_object::gc_roots::shadow_stack_get(keys_sp + i); parts.push(format!( "{}={}", unsafe { crate::display::py_str(key)? },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyre/pyre-interpreter/src/module/sys/vm.rs` around lines 306 - 331, Reload key from the shadow stack after crate::baseobjspace::getitem returns and before using it in crate::display::py_str, because the call may move the object and invalidate the local pointer. Keep the existing value reload and error handling unchanged.
472-491: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winRe-read
resultafter each collecting call.Line 449 reads
resultonce.getattr_strat line 472 can run a subclass__getattribute__.namespace_update_dictat lines 477 and 485 allocates and can run Python code. Each of those is a collection point, so the localresultpointer can become stale. Lines 478, 486, and 491 use that stale pointer, and line 491 can return a moved-from address.🐛 Proposed fix to reload the result slot
let source_dict = crate::baseobjspace::getattr_str( pyre_object::gc_roots::shadow_stack_get(sp), "__dict__", )?; pyre_object::gc_roots::pin_root(source_dict); + let result_slot = sp + 2 + usize::from(kwargs.is_some()); namespace_update_dict( - result, + pyre_object::gc_roots::shadow_stack_get(result_slot), pyre_object::gc_roots::shadow_stack_get( sp + 3 + usize::from(kwargs.is_some()), ), false, )?; if kwargs.is_some() { namespace_update_dict( - result, + pyre_object::gc_roots::shadow_stack_get(result_slot), pyre_object::gc_roots::shadow_stack_get(sp + 1), true, )?; } - Ok(result) + Ok(pyre_object::gc_roots::shadow_stack_get(result_slot))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyre/pyre-interpreter/src/module/sys/vm.rs` around lines 472 - 491, In the surrounding function, reload `result` from its shadow-stack slot after every collecting call—`getattr_str`, each `namespace_update_dict`, and immediately before returning—so no stale pointer is used after GC. Update the calls and final `Ok(result)` to use the reloaded slot while preserving the existing namespace-update order and arguments.
🤖 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 9478-9487: Update the MRO merge around bases_slot and list_slots
so it does not retain PyObjectRef values across allocations that may trigger
collection. Keep the merge inputs slot-valued until lists are fully
materialized, then reread bases_slot immediately before use, preserving the
existing C3 and is_type_like_w behavior.
In `@pyre/pyre-interpreter/src/module/sys/vm.rs`:
- Around line 384-386: Update the surrounding function to root or pin self_obj
and other before the first getattr_str call, then reload both operands after
each potentially allocating getattr_str invocation before using them in the
second lookup and compare call. Ensure the self_dict and other_dict references
are also rooted or safely reloaded across allocation points so no stale pointers
are passed to compare.
In `@pyre/pyre-interpreter/src/typedef.rs`:
- Around line 8813-8852: Update union_repr_method, union_hash_method, and
union_mro_entries_method to enforce their expected positional arity, and change
their UnionType registrations to use make_builtin_function_with_arity. Validate
the corresponding user-argument count inside each handler with
type_methods::arity_exact, or explicitly reject incorrect args.len(), while
preserving the existing self-type checks and return behavior.
---
Duplicate comments:
In `@pyre/pyre-interpreter/src/module/sys/vm.rs`:
- Around line 375-386: Update the rich-comparison implementation around the
existing other_type check to validate self_obj is also a SimpleNamespace before
accessing either operand’s __dict__. Return w_not_implemented() when either
operand is not a namespace instance, while preserving the existing
namespace-dictionary comparison for valid operands.
- Around line 306-331: Reload key from the shadow stack after
crate::baseobjspace::getitem returns and before using it in
crate::display::py_str, because the call may move the object and invalidate the
local pointer. Keep the existing value reload and error handling unchanged.
- Around line 472-491: In the surrounding function, reload `result` from its
shadow-stack slot after every collecting call—`getattr_str`, each
`namespace_update_dict`, and immediately before returning—so no stale pointer is
used after GC. Update the calls and final `Ok(result)` to use the reloaded slot
while preserving the existing namespace-update order and arguments.
🪄 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: 899f9b2e-b6fa-4dc9-9d0b-9df0c5b306c3
📒 Files selected for processing (6)
pyre/pyre-interpreter/src/_pypy_generic_alias.rspyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/call.rspyre/pyre-interpreter/src/module/struct/mod.rspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-interpreter/src/typedef.rs
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/57b8f38bcf89112673ec432973dd8f3551a4e120/pyre-interpreter/src/baseobjspace.rs#L9365-L9367
When a mixed real/classic base graph contains two distinct nodes that compare equal, PyPy's klass not in mro invokes list-containment equality and suppresses the second node, whereas this pointer-identity check traverses it independently. That can perform extra __bases__ reads or produce a different C3 validation result; use Python equality semantics as the ported typeobject.py:1672 does.
AGENTS.md reference: AGENTS.md:L194-L195
https://github.com/youknowone/pyre/blob/57b8f38bcf89112673ec432973dd8f3551a4e120/pyre-interpreter/src/baseobjspace.rs#L9372
For a classic base whose __bases__ is provided dynamically, PyPy's abstract_mro evaluates it once for the tuple check and again for the reversed traversal (typeobject.py:1674-1676). Caching one result here changes observable getter side effects and can accept or traverse a hierarchy that PyPy would reject or build differently when the second access changes or raises; preserve the two reads required by the line-by-line port.
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".
`setup_user_defined_type` runs `check_and_find_best_base`
(typeobject.py:1519) before `compute_mro` (typeobject.py:1560), so an
unacceptable base type or a layout conflict is reported without any
classic base's `__bases__` ever executing. pyre's `validate_c3_mro`
pre-flight runs ahead of the best-base check, and putting `get_mro`'s
classic branch there inverted that pair:
`type('C', (bool, object()), {})` reported the classic base's
`AttributeError` instead of "type 'bool' is not an acceptable base
type".
Gate the classic walk on a `walk_classic_bases` argument. The pre-flight
passes `false` and stays a pure C3 check; the two calls standing in for
`compute_mro` — `compute_and_set_mro` and the default `type()` path's
`compute_default_mro`, which cannot raise on its own — pass `true`.
Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/465bcf49751843894d18b598373daa046ba31e82/pyre-interpreter/src/baseobjspace.rs#L9365-L9367
When a non-W_TypeObject base graph contains distinct nodes that compare equal, or an __eq__ implementation that raises, PyPy's app-level abstract_mro executes klass not in mro, so it either suppresses the second node or propagates the comparison error. This pointer-identity test instead walks the second node and reads its __bases__, producing different validation or an unrelated exception; preserve the app-level membership semantics.
AGENTS.md reference: AGENTS.md:L195-L195
https://github.com/youknowone/pyre/blob/465bcf49751843894d18b598373daa046ba31e82/pyre-interpreter/src/module/sys/vm.rs#L118-L122
When the positional mapping contains a str-subclass key with a side-effecting __hash__, this per-item insertion hashes the key again after dict_update1 has already built the temporary exact dict. CPython's PyDict_Update(ns->ns_dict, source) reuses hashes cached in the source dict, so a key whose hash raises on its second invocation constructs successfully there but fails here; use the dict-to-dict update/copy path that preserves cached hashes instead of reinserting each item.
ℹ️ 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".
`simple_namespace_replace` pinned the constructed namespace and then kept the value it had captured, but the `__dict__` lookup and each `namespace_update_dict` in between can run Python and collect: the second update and the return both named the pre-relocation address. The function's other operands already reread their slots; do the same for the result. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8289fb217
ℹ️ 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".
| // `check_and_find_best_base` inside `create_all_slots` above accepted | ||
| // the tuple. `compute_default_mro` cannot raise, so `get_mro`'s | ||
| // classic branch runs through the fallible validation here. | ||
| unsafe { crate::baseobjspace::validate_c3_mro(w_effective_bases, true)? }; |
There was a problem hiding this comment.
When this default class-construction path has an injected non-type base whose __bases__ getter forces a major collection, w is only an untraced Rust local while validate_c3_mro(..., true) executes Python. For a class with __slots__ = () and no class cell or descriptor back-reference, the fresh stable type is therefore unreachable and can be swept before lines 4040-4041 dereference it. Pin and reload the nascent type across this validation; compute_and_set_mro now needs the same protection for direct type.__new__ calls.
AGENTS.md reference: AGENTS.md:L195-L195
Useful? React with 👍 / 👎.
| if mro_slots | ||
| .iter() | ||
| .any(|&seen| std::ptr::eq(pyre_object::gc_roots::shadow_stack_get(seen), w_cls)) |
There was a problem hiding this comment.
For a non-type base graph containing distinct nodes that compare equal, this identity test diverges from PyPy's literal klass not in mro, which invokes Python equality. For example, if a child compares equal to an already visited pseudo-class but lacks __bases__, PyPy skips the child while this implementation visits it and raises AttributeError; custom equality side effects are also lost. Use the app-level membership semantics of the upstream walk rather than pointer identity.
AGENTS.md reference: AGENTS.md:L195-L195
Useful? React with 👍 / 👎.
build_class_inner left the freshly created W_TypeObject in an untraced Rust local across create_all_slots, the classic-base C3 validation and every __set_name__ call, and held the class-dict snapshot those calls iterate in a plain Vec. compute_and_set_mro did the same for w_self and kept the default MRO plus a metaclass mro() result in Vecs across the Python calls that follow. Pin each of them and reread from the slot after every step that can collect. simple_namespace_repr reused a key read from before the getitem that fetches its value; simple_namespace_richcompare had no root scope around its two __dict__ lookups. Assisted-by: Claude
…arity
They were registered through make_builtin_function, which accepts any
argument count, so `(int | str).__repr__('x')` returned the repr instead
of raising TypeError.
Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f323839e4
ℹ️ 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".
| // `check_and_find_best_base` inside `create_all_slots` above accepted | ||
| // the tuple. `compute_default_mro` cannot raise, so `get_mro`'s | ||
| // classic branch runs through the fallible validation here. | ||
| unsafe { crate::baseobjspace::validate_c3_mro(w_effective_bases, true)? }; |
There was a problem hiding this comment.
When create_all_slots executes Python through a custom iterable supplied as __slots__, a moving collection can relocate the bases tuple. The rooted type retains the updated bases pointer, but w_effective_bases is still the address captured before that call, so passing it to validate_c3_mro can dereference evacuated memory. Reload the bases from the rooted w (or root and reload the tuple itself) before validation.
Useful? React with 👍 / 👎.
| let self_type = crate::typedef::r#type(self_obj) | ||
| .map(|tp| tp.as_ptr()) | ||
| .unwrap_or(PY_NULL); | ||
| pyre_object::gc_roots::pin_root(self_type); |
There was a problem hiding this comment.
On an unbound call such as types.SimpleNamespace.__replace__(foreign), this accepts the foreign object as self and invokes type(foreign)() before eventually checking the result. This can run arbitrary constructor side effects or raise an unrelated constructor error, whereas the SimpleNamespace descriptor must reject a non-namespace receiver immediately; validate self_obj as a SimpleNamespace subtype before calling its type.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyre/pyre-interpreter/src/module/sys/vm.rs (1)
377-382: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCheck
self_obj's type, not onlyother's.CPython's
namespace_richcomparerequires both operands to be namespace instances:PyObject_IsInstance(self, ...) && PyObject_IsInstance(other, ...). This code checks onlyother_type. An unbound call such astypes.SimpleNamespace.__eq__(1, ns)reachesgetattr_str(self_obj, "__dict__")on a non-namespaceself_objand raisesAttributeError, where CPython returnsNotImplemented.This is the same gap flagged in a prior review on this line range, and it lacks the "✅ Addressed" confirmation the other three findings in this file received. The code at lines 377-382 still shows only the
other_typecheck, so the issue remains open.🐛 Proposed fix to validate both operands
+ let self_type = crate::typedef::r#type(self_obj) + .map(|tp| tp.as_ptr()) + .unwrap_or(PY_NULL); let other_type = crate::typedef::r#type(other) .map(|tp| tp.as_ptr()) .unwrap_or(PY_NULL); - if !unsafe { crate::baseobjspace::issubtype_w(other_type, simple_namespace_type()) } { + if !unsafe { crate::baseobjspace::issubtype_w(self_type, simple_namespace_type()) } + || !unsafe { crate::baseobjspace::issubtype_w(other_type, simple_namespace_type()) } + { return Ok(w_not_implemented()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyre/pyre-interpreter/src/module/sys/vm.rs` around lines 377 - 382, Update the rich-comparison validation near the other_type check to verify self_obj is also a SimpleNamespace before accessing its __dict__. Return w_not_implemented() when either operand is not a namespace instance, preserving normal comparison behavior for two valid namespace operands.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pyre/pyre-interpreter/src/module/sys/vm.rs`:
- Around line 377-382: Update the rich-comparison validation near the other_type
check to verify self_obj is also a SimpleNamespace before accessing its
__dict__. Return w_not_implemented() when either operand is not a namespace
instance, preserving normal comparison behavior for two valid namespace
operands.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4b9707b3-7a3d-456c-9cc2-67377f8ac5b3
📒 Files selected for processing (5)
pyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/call.rspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-interpreter/src/typedef.rs
build_class_inner captured the effective bases tuple once and reused that address after create_all_slots, which unpacks __slots__ and can therefore run Python. Pin the tuple and reread it at w_type_new, create_all_slots, the classic-base validation and __init_subclass__ dispatch. simple_namespace_replace called type(self)() before checking anything, so an unbound call such as types.SimpleNamespace.__replace__(Foreign()) ran a foreign constructor. Reject a non-namespace receiver up front with namespace___replace__'s descriptor message. Assisted-by: Claude
1 participant
Six commits on top of
main, one concept each, all inpyre-interpreter.Type creation
call: reject non-type entries in find_best_base— the loop skipped anon-type base instead of rejecting it, so
type('C', (object, object()), {})built a class whose MRO contained a plain instance.
compute_mroappendsevery supplied base, and attribute lookup then reads that entry as a type
layout.
best_baseraisesbases must be typesin the same loop.__bases__assignment screens its tuple earlier and keeps its own message.itertools: validate the product subtype before reading any argument—product_descr_newreachedcheck_user_subclassonly throughitertools_alloc_for_class, after the keyword census, therepeatconversion and a full pass over the input iterables. An unbound call such as
itertools.product.__new__(int, gen())therefore consumed the iterable andcould report an unrelated error first.
tp_new_wrapperscreens the subtypebefore
product_newruns.GC rooting
generic alias: root subs_parameters owners and results across substitution—newargs,old_argand the owner arguments were rawPyObjectRefs in Rust locals across__typing_prepare_subst__,__typing_subst__, the recursive descent into nested lists and tuples, andeach
w_tuple_new/w_list_new— all of which allocate or run Python. Thecollector moves objects and does not scan Rust locals. They now live on the
shadow stack and are reread after anything that can collect; produced
arguments are held as slots so an entry survives later iterations'
allocations.
struct: root iter_unpack buffer lease—readbufmay dispatch aPython-level
__buffer__hook, so the format and buffer operands are pinnedacross it and across the stable allocation, matching
W_UnpackIterholdingits live
self.viewlease on the iterator rather than an unrootedcaller-local pointer.
3.14 type parity
types: complete SimpleNamespace 3.14 parity— adds__replace__,__reduce__, the full rich-comparison set (__lt__/__le__/__gt__/__ge__alongside
__eq__/__ne__), and__dict__update handling.types: expose UnionType special methods— adds__hash__,__repr__and
__mro_entries__.Summary by CodeRabbit
New Features
SimpleNamespacewith flexible initialization, copying, pickling, rich comparisons, and improved representations.Bug Fixes