Repository navigation
stdlib: complete copy, enum, and weakref compatibility - #721
youknowone wants to merge 1 commit into
Conversation
WalkthroughChangesRuntime semantics and compatibility
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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: 258a21607f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }; | ||
| } | ||
| } | ||
| crate::builtins::type_new_set_doc(class_ns)?; |
There was a problem hiding this comment.
Because this runs before the if let Some(w_metaclass) branch, a class whose custom metaclass consumes the namespace itself now fails during __build_class__ if __slots__ contains non-string/invalid entries: type_new_set_doc propagates collect_slot_names(...)? instead of leaving slot validation to type.__new__. In that scenario CPython/PyPy pass the namespace to the metaclass unchanged, so gate this validation to the default/type construction path or avoid propagating slot errors before custom metaclass dispatch.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
| { | ||
| return Ok(None); | ||
| } | ||
| let result = crate::builtins::call_and_check(method, &[obj])?; |
There was a problem hiding this comment.
When a metaclass supplies __repr__ through a descriptor such as @staticmethod, @classmethod, or a custom __get__, this raw call passes the class object as an explicit argument instead of first binding the descriptor and then calling it with no arguments. CPython/PyPy special-method lookup applies the descriptor protocol here, so a no-arg staticmethod metaclass __repr__ should work but this path raises; use the existing get_and_call_function(method, obj, metaclass, &[]) shape.
AGENTS.md reference: AGENTS.md:L194-L195
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: 5
🤖 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/display.rs`:
- Around line 521-527: Update the new metaclass __repr__ branch in the display
function to avoid w_str_get_value(result), which panics on lone surrogates.
Reuse the WTF-8-safe conversion pattern from py_repr_wtf8 and the
backslashreplace-style fallback used by builtin_subclass_dunder, preserving the
existing successful __repr__ return behavior.
In `@pyre/pyre-interpreter/src/module/_weakref/interp__weakref.rs`:
- Around line 872-874: Replace the separate finditem and delitem_slot calls in
the weakref deletion flow with the atomic delitem_if_value_is or
nondescr_delitem_if_value_is helper, using backing, key, and stored so the value
check and deletion cannot be separated by re-entrant code.
In `@pyre/pyre-interpreter/src/objspace/std/mapdict.rs`:
- Around line 2014-2034: Update is_unboxable_int to explicitly reject bool
values before the bootstrap fallback, using the existing pyre_object::is_bool
check. Preserve the current exact-int and uninitialized-type handling for
non-bool integer objects.
In `@pyre/pyre-object/src/typeobject.rs`:
- Around line 807-820: Update w_type_get_name_obj and w_type_get_qualname_obj so
each lazy assignment to the GC-tracked w_name or w_qualname field is immediately
followed by crate::gc_hook::try_gc_write_barrier(obj as *mut u8). Leave the
existing caching and return behavior unchanged.
- Around line 848-853: Prevent lone-surrogate __qualname__ values from reaching
w_type_set_qualname by adding check_surrogate validation in
type_new_take_qualname at pyre/pyre-interpreter/src/builtins.rs:3655-3670 and
the __qualname__ branch of object_setattr at
pyre/pyre-interpreter/src/baseobjspace.rs:8611-8624, before calling the setter.
Update w_type_set_qualname in pyre/pyre-object/src/typeobject.rs:848-853 only as
needed to preserve this contract, ensuring valid strings are stored unchanged
and invalid values return an error instead of panicking.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ab840aed-549c-45db-a2f9-0a4d6b41fc7a
📒 Files selected for processing (15)
pyre/extra_tests/snippets/stdlib_copy.pypyre/extra_tests/snippets/stdlib_enum.pypyre/extra_tests/snippets/stdlib_weakref.pypyre/pyre-interpreter/src/baseobjspace.rspyre/pyre-interpreter/src/builtins.rspyre/pyre-interpreter/src/call.rspyre/pyre-interpreter/src/display.rspyre/pyre-interpreter/src/eval.rspyre/pyre-interpreter/src/module/_weakref/interp__weakref.rspyre/pyre-interpreter/src/module/_weakref/mod.rspyre/pyre-interpreter/src/objspace/descroperation.rspyre/pyre-interpreter/src/objspace/std/mapdict.rspyre/pyre-interpreter/src/typedef.rspyre/pyre-jit/src/eval.rspyre/pyre-object/src/typeobject.rs
| // A class object is an instance of its metaclass. PyPy's | ||
| // `space.repr` therefore resolves `__repr__` on that metaclass before | ||
| // `type`'s native `<class ...>` representation. This is observable | ||
| // for EnumType and any user metaclass defining `__repr__`. | ||
| if let Some(result) = type_metaclass_dunder_obj(obj, "__repr__")? { | ||
| return Ok(pyre_object::w_str_get_value(result).to_string()); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
New metaclass __repr__ branch panics on a lone-surrogate result.
pyre_object::w_str_get_value(result) panics if the metaclass's __repr__ returns a str containing a lone surrogate. py_repr_wtf8 right above handles the identical situation safely via w_str_get_wtf8(...).to_wtf8_buf(). This mirrors two other pre-existing (unchanged) __repr__ dispatch sites later in this same function that have the same limitation, so it's a narrow addition to an existing, broader gap rather than a novel regression — but it's a real, easily-triggered crash (define a metaclass whose __repr__ returns a lone-surrogate string) worth closing here since the fix is a one-line swap to the WTF-8-safe path plus a backslashreplace-style fallback (as already done for builtin_subclass_dunder).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-interpreter/src/display.rs` around lines 521 - 527, Update the new
metaclass __repr__ branch in the display function to avoid
w_str_get_value(result), which panics on lone surrogates. Reuse the WTF-8-safe
conversion pattern from py_repr_wtf8 and the backslashreplace-style fallback
used by builtin_subclass_dunder, preserving the existing successful __repr__
return behavior.
| if crate::baseobjspace::finditem(backing, key)? == Some(stored) { | ||
| crate::baseobjspace::delitem_slot(backing, key)?; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate existing conditional dictionary-deletion support before porting one.
ast-grep outline pyre/pyre-interpreter/src --items all --type function --match 'delitem'
rg -n -C 3 \
'delitem_if_value_is|nondescr_delitem_if_value_is|dict_delitem|delitem_slot' \
pyre/pyre-interpreter/srcRepository: youknowone/pyre
Length of output: 19676
🏁 Script executed:
rg -n -C 4 'nondescr_delitem_if_value_is|delitem_if_value_is|w_dict_delitem_if_value_is|delitem_slot\(backing, key\)' pyre/pyre-interpreter/srcRepository: youknowone/pyre
Length of output: 2682
🏁 Script executed:
ast-grep outline pyre/pyre-interpreter/src/baseobjspace.rs --match 'finditem' --view expanded
rg -n -C 3 'nondescr_delitem_if_value_is|delitem_if_value_is|value_is|finditem\(|delitem_slot\(' pyre/pyre-interpreter/src/baseobjspace.rs pyre/pyre-interpreter/src/objspace pyre/pyre-interpreter/src/module/_weakref/interp__weakref.rsRepository: youknowone/pyre
Length of output: 5079
Use the atomic weakref-delete helper here. finditem(backing, key) followed by delitem_slot(backing, key) can re-enter user code and let backing change between the check and delete; route this through the delitem_if_value_is / nondescr_delitem_if_value_is path instead.
🤖 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/_weakref/interp__weakref.rs` around lines
872 - 874, Replace the separate finditem and delitem_slot calls in the weakref
deletion flow with the atomic delitem_if_value_is or
nondescr_delitem_if_value_is helper, using backing, key, and stored so the value
check and deletion cannot be separated by re-entrant code.
Source: Coding guidelines
| unsafe fn is_unboxable_int(w_value: PyObjectRef) -> bool { | ||
| // Early-return control flow rather than `if c { false } else { x }`, which | ||
| // can become a `!c && x` (a `bool_not`) that majit-translate cannot lower. | ||
| if unsafe { pyre_object::is_bool(w_value) } { | ||
| if !unsafe { pyre_object::is_int(w_value) } { | ||
| return false; | ||
| } | ||
| let exact = crate::typedef::gettypeobject(&pyre_object::INT_TYPE); | ||
| if exact.is_null() { | ||
| // Interpreter-only unit tests may exercise mapdict before the app-level | ||
| // type registry is initialized. A native int with no class stamp is | ||
| // the bootstrap representation of that same exact type. | ||
| if pyre_object::tagged_int::CAN_BE_TAGGED | ||
| && unsafe { pyre_object::tagged_int::is_tagged_int(w_value) } | ||
| { | ||
| return true; | ||
| } | ||
| return unsafe { (*w_value).w_class.is_null() }; | ||
| } | ||
| let Some(actual) = crate::typedef::r#type(w_value) else { | ||
| return false; | ||
| }; | ||
| std::ptr::eq(actual, exact) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm pyre_object::is_bool exists and its signature matches is_int/is_float.
rg -n 'pub unsafe fn is_bool' pyre/pyre-object/srcRepository: youknowone/pyre
Length of output: 241
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- mapdict.rs around is_unboxable_int ---'
sed -n '2004,2038p' pyre/pyre-interpreter/src/objspace/std/mapdict.rs
echo
echo '--- pyobject.rs around is_int/is_bool/is_float ---'
sed -n '1118,1160p' pyre/pyre-object/src/pyobject.rs
echo
echo '--- typedef.rs around r#type docs ---'
rg -n -A8 -B4 'pub unsafe fn r#type|RODATA statics|init_typeobjects' pyre/pyre-object/src/typedef.rsRepository: youknowone/pyre
Length of output: 3336
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- typedef.rs search for bootstrap statics and null w_class ---'
rg -n -A12 -B6 'True|False|RODATA|null w_class|init_typeobjects|before init_typeobjects|SIGBUS' pyre/pyre-object/src/typedef.rs pyre/pyre-object/src/pyobject.rs
echo
echo '--- bool/int constructors and singleton layout ---'
rg -n -A10 -B4 'BOOL_TYPE|INT_TYPE|w_class|ob_type' pyre/pyre-object/src/pyobject.rs pyre/pyre-object/src/typedef.rs
echo
echo '--- mapdict pick_unbox_type and value_has_unbox_type ---'
sed -n '2084,2100p' pyre/pyre-interpreter/src/objspace/std/mapdict.rs
sed -n '2624,2642p' pyre/pyre-interpreter/src/objspace/std/mapdict.rsRepository: youknowone/pyre
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l pyre/pyre-object/src/typedef.rs
rg -n 'True|False|Ellipsis|NotImplemented|RODATA|w_class' pyre/pyre-object/src/typedef.rsRepository: youknowone/pyre
Length of output: 282
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- value_has_unbox_type and pick_unbox_type ---'
sed -n '2088,2100p' pyre/pyre-interpreter/src/objspace/std/mapdict.rs
sed -n '2624,2640p' pyre/pyre-interpreter/src/objspace/std/mapdict.rs
echo
echo '--- find callers of is_unboxable_int ---'
rg -n -A4 -B4 'is_unboxable_int\(' pyre/pyre-interpreter/src/objspace/std/mapdict.rs pyre/pyre-interpreter/src/objspace/std
echo
echo '--- search for explicit bool handling in mapdict unbox logic ---'
rg -n -A3 -B3 'is_bool|BOOL_TYPE|bool' pyre/pyre-interpreter/src/objspace/std/mapdict.rsRepository: youknowone/pyre
Length of output: 14401
Bootstrap bool values are still misclassified as unboxable int. At pyre/pyre-interpreter/src/objspace/std/mapdict.rs:2014-2034, is_int() also matches bool, and the exact.is_null() fallback only checks w_class.is_null(). True/False are bootstrap singletons with null w_class, so this branch accepts them as ints before init_typeobjects(). Keep an explicit is_bool() exclusion here as well.
🤖 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/objspace/std/mapdict.rs` around lines 2014 - 2034,
Update is_unboxable_int to explicitly reject bool values before the bootstrap
fallback, using the existing pyre_object::is_bool check. Preserve the current
exact-int and uninitialized-type handling for non-bool integer objects.
Source: Coding guidelines
| /// Return the stable app-level `type.__name__` object. | ||
| pub unsafe fn w_type_get_name_obj(obj: PyObjectRef) -> PyObjectRef { | ||
| let t = &mut *(obj as *mut W_TypeObject); | ||
| if t.w_name.is_null() { | ||
| let full = &*t.name; | ||
| let bare = if t.flag_heaptype { | ||
| full.as_str() | ||
| } else { | ||
| full.rsplit('.').next().unwrap_or(full) | ||
| }; | ||
| t.w_name = crate::w_str_new(bare); | ||
| } | ||
| t.w_name | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Missing GC write barrier when lazily caching w_name/w_qualname.
Both w_type_get_name_obj and w_type_get_qualname_obj mutate a PyObjectRef field on W_TypeObject (t.w_name = crate::w_str_new(bare); / t.w_qualname = crate::w_str_new(&*t.qualname);) but never call crate::gc_hook::try_gc_write_barrier(obj as *mut u8) afterward. Every other place in this file that stores into these GC-tracked fields post-construction (w_type_set_name, w_type_set_qualname, and w_type_new's own construction path) explicitly triggers the barrier. type_object_custom_trace (pyre-jit/src/eval.rs) now forwards w_name/w_qualname as real GC edges, which confirms these are genuine GC references requiring barrier protection on old-gen objects.
Since a heap type is typically old-gen (w_type_new's gc_managed path) while w_str_new(bare) likely allocates a fresh (young) string, storing it into t.w_name without a barrier means a minor collection may never re-scan this type object, so the freshly cached string can be collected while t.w_name still points at it — a latent use-after-free that would only surface under GC pressure. Note that stdlib_enum.py's Custom.__name__ is Custom.__name__ identity assertion exercises exactly this cached path but wouldn't reliably catch this bug since it needs a GC cycle between the two reads to manifest.
[critical_issue_confidence: high]
🛡️ Proposed fix
pub unsafe fn w_type_get_name_obj(obj: PyObjectRef) -> PyObjectRef {
let t = &mut *(obj as *mut W_TypeObject);
if t.w_name.is_null() {
let full = &*t.name;
let bare = if t.flag_heaptype {
full.as_str()
} else {
full.rsplit('.').next().unwrap_or(full)
};
t.w_name = crate::w_str_new(bare);
+ crate::gc_hook::try_gc_write_barrier(obj as *mut u8);
}
t.w_name
} pub unsafe fn w_type_get_qualname_obj(obj: PyObjectRef) -> PyObjectRef {
let t = &mut *(obj as *mut W_TypeObject);
if t.w_qualname.is_null() {
t.w_qualname = crate::w_str_new(&*t.qualname);
+ crate::gc_hook::try_gc_write_barrier(obj as *mut u8);
}
t.w_qualname
}Also applies to: 839-846
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-object/src/typeobject.rs` around lines 807 - 820, Update
w_type_get_name_obj and w_type_get_qualname_obj so each lazy assignment to the
GC-tracked w_name or w_qualname field is immediately followed by
crate::gc_hook::try_gc_write_barrier(obj as *mut u8). Leave the existing caching
and return behavior unchanged.
| pub unsafe fn w_type_set_qualname(obj: PyObjectRef, w_qualname: PyObjectRef) { | ||
| let t = &mut *(obj as *mut W_TypeObject); | ||
| *t.qualname = crate::w_str_get_value(w_qualname).to_string(); | ||
| t.w_qualname = w_qualname; | ||
| crate::gc_hook::try_gc_write_barrier(obj as *mut u8); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
__qualname__ can crash the interpreter on a lone-surrogate string; __name__'s surrogate guard was never mirrored for __qualname__. w_type_set_qualname stores the value's native form via w_str_get_value, which panics (.expect(...)) when the backing WTF-8 buffer holds an unpaired surrogate. Both places that call it validate only is_str(value), never surrogate-safety — unlike the __name__ setter, which explicitly runs check_surrogate first. CPython never crashes on this input (a __qualname__ str is stored as-is); this is a genuine interpreter-parity/crash defect, user-triggerable via class X: __qualname__ = "\udc80" or X.__qualname__ = "\udc80".
pyre/pyre-object/src/typeobject.rs#L848-L853: root-cause implementation — either add a surrogate guard here (mirroringcheck_surrogate) or document/enforce that all callers must pre-validate, then update both call sites below to match.pyre/pyre-interpreter/src/builtins.rs#L3655-L3670(type_new_take_qualname): callcrate::builtins::check_surrogate(value)?beforepyre_object::w_type_set_qualname(w_type, value).pyre/pyre-interpreter/src/baseobjspace.rs#L8611-L8624(object_setattr's__qualname__branch): callcheck_surrogate(value)?beforepyre_object::w_type_set_qualname(obj, value).
📍 Affects 3 files
pyre/pyre-object/src/typeobject.rs#L848-L853(this comment)pyre/pyre-interpreter/src/builtins.rs#L3655-L3670pyre/pyre-interpreter/src/baseobjspace.rs#L8611-L8624
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-object/src/typeobject.rs` around lines 848 - 853, Prevent
lone-surrogate __qualname__ values from reaching w_type_set_qualname by adding
check_surrogate validation in type_new_take_qualname at
pyre/pyre-interpreter/src/builtins.rs:3655-3670 and the __qualname__ branch of
object_setattr at pyre/pyre-interpreter/src/baseobjspace.rs:8611-8624, before
calling the setter. Update w_type_set_qualname in
pyre/pyre-object/src/typeobject.rs:848-853 only as needed to preserve this
contract, ensuring valid strings are stored unchanged and invalid values return
an error instead of panicking.
|
Superseded by #726 after moving the complete stdlib work onto a fresh branch based on latest main in the primary /Users/youknowone/Projects/pyre worktree. |
1 participant
Summary
_weakref._remove_dead_weakrefand public weakref type metadatacopyandenum__name__/__qualname__objectsLOAD_LOCALSretain custom metaclass__prepare__mappings, matching PyPyValidation
PYRE_JIT=0 target/debug/pyre-dynasm -m test test_enum— 1,081 tests successfultest_copy.py— 81 passedcargo check --features dynasmcargo test --features dynasmpython pyre/check.py: cranelift 248/248, wasm 247/247; dynasm 247/248 with the existingnested_loopperformance gate exceeding 2x PyPy (all correctness runs passed)Summary by CodeRabbit
Bug Fixes
__doc__,__name__, and__qualname__.repr()andstr()behavior.Tests
copy,enum, andweakrefbehavior.