weakref: reuse proxy objects#8307
Merged
Merged
Conversation
Make PyWeakProxy a #[repr(transparent)] view sharing PyWeak's payload (manual PyPayload with shared PAYLOAD_TYPE_ID + class check), so a proxy is the weakref-list node itself and add()'s reuse applies to it directly. Port CPython's insert_weakref/get_basic_refs slot discipline: genericref at head, generic proxy right after it, with callback/class re-verificationbefore reuse. Constructor moves to a slot_new override since reuse returns an existing object. Drops the hidden __weakproxy marker subclass and the proxy branch in PyWeakref_GetRef. Assisted-by: Claude Code:claude-opus-4-8
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughWeak proxy payloads now use a transparent ChangesWeakref rework
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PyObject
participant WeakRefList
participant PyWeak
participant PyWeakref_GetRef
PyObject->>WeakRefList: request weakref or weakproxy with classification
WeakRefList->>PyWeak: reuse existing payload or allocate and insert
WeakRefList-->>PyObject: return weakref payload
PyWeakref_GetRef->>PyWeak: upgrade direct PyWeak
PyWeakref_GetRef-->>PyObject: return referent or TypeError
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/weakref.py dependencies:
dependent tests: (222 tests)
Legend:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@crates/vm/src/object/core.rs`:
- Around line 668-675: Update the basic-proxy lookup in the after-slot logic and
the corresponding path around the referenced later code to call
find_generic_proxy_ptr with the canonical weakproxy type instead of None. Ensure
the candidate’s class is validated so callback-less weakref subclasses are not
treated as the basic proxy, while preserving the existing fallback to
self.generic.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: c8aa6465-869b-489b-b025-143443b1a9a7
⛔ Files ignored due to path filters (1)
Lib/test/test_weakref.pyis excluded by!Lib/**
📒 Files selected for processing (3)
crates/capi/src/weakrefobject.rscrates/vm/src/builtins/weakproxy.rscrates/vm/src/object/core.rs
💤 Files with no reviewable changes (1)
- crates/capi/src/weakrefobject.rs
rlaisqls
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Make callback-less
weakref.proxyobjects reusable, like CPython. Whenweakref.proxy(o)is called twice on the same object, CPython returns the same proxy object, but RustPython has always created a new proxy each time.With this change, the
@unittest.expectedFailuremarkers are removed from two tests intest_weakref.py:test_proxy_reuseandtest_shared_proxy_without_callback.Background
PyWeakProxywas a separate wrapper struct ({ weak: PyRef<PyWeak> }) pointing at an innerPyWeaknode. With this structure, even if the innerPyWeaknode was reused, the outer wrapper was freshly allocated on every call, soisidentity between twoweakref.proxy(o)calls could never hold in the first place. This is exactly the problem the// TODO: PyWeakProxy should use the same payload as PyWeakcomment was pointing at.Moreover,
WeakRefListwhich holds the reuse candidates is a linked list whose nodes arePyWeakstruct pointers, soPyWeakProxy, being a separate struct, could not be placed in that list and was never even eligible for the reuse cache.However, since
PyWeakProxyis an empty shell with no fields of its own besides the singlePyRef<PyWeak>, I judged that this gap could be closed by merging its payload withPyWeak, and implemented it that way.Changes
PyWeakProxyto a#[repr(transparent)]view sharingPyWeak's payload. It uses a manualPyPayloadimplementation that sharesPAYLOAD_TYPE_IDwithPyWeakand distinguishes downcasts via a class check.WeakRefList::add()applies to proxies as-is.(subclasses are excluded from reuse)
slot_newoverride. Since reuse must return an existing object rather than a new payload, this is handled inslot_newinstead ofpy_new.__weakproxymarker subclass, and since proxies now downcast toPyWeak, drop the proxy branch in the capiPyWeakref_GetRefas well.Summary by CodeRabbit
TypeError.