Allow heap getset creation #5854
Conversation
WalkthroughThe changes primarily refactor how property and getset descriptors are created and managed, switching from dynamic to static getset descriptors across multiple modules. Internal safety and pointer management are improved for class references in getset descriptors. In Python's Changes
Sequence Diagram(s)sequenceDiagram
participant Context
participant PyType
participant PyGetSet
Context->>PyType: Provide static reference for class
Context->>PyGetSet: Create static getset descriptor (new_static_getset)
PyGetSet-->>PyType: Store class reference via PointerSlot
Note right of PyGetSet: Access class via PointerSlot safety methods
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
⏰ Context from checks skipped due to timeout of 90000ms (11)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
stdlib/src/pyexpat.rs(1 hunks)vm/src/builtins/getset.rs(4 hunks)vm/src/builtins/property.rs(1 hunks)vm/src/builtins/type.rs(4 hunks)vm/src/class.rs(1 hunks)vm/src/vm/context.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
vm/src/builtins/type.rs (4)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.109Z
Learning: Minimize modifications to CPython standard library files in the Lib/ directory; bug fixes should be made through Rust code modifications whenever possible.
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.109Z
Learning: When implementing a Python module in Rust for RustPython, use the #[pymodule] attribute and expose functions and classes using #[pyfunction], #[pyclass], and #[pymethod] as appropriate.
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.109Z
Learning: Use the macro system (such as 'pyclass', 'pymodule', 'pyfunction') when implementing Python functionality in Rust for RustPython.
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.109Z
Learning: Always use 'cargo fmt' to format Rust code and follow the default rustfmt code style when contributing to RustPython.
vm/src/builtins/getset.rs (2)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.109Z
Learning: Use the macro system (such as 'pyclass', 'pymodule', 'pyfunction') when implementing Python functionality in Rust for RustPython.
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.109Z
Learning: When implementing a Python module in Rust for RustPython, use the #[pymodule] attribute and expose functions and classes using #[pyfunction], #[pyclass], and #[pymethod] as appropriate.
🧬 Code Graph Analysis (1)
vm/src/builtins/getset.rs (1)
vm/src/builtins/type.rs (5)
from(90-92)class(146-148)borrow_static(76-78)slot_name(426-431)name(433-438)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
🔇 Additional comments (11)
vm/src/vm/context.rs (2)
573-587: LGTM!The renaming to
new_static_getsetclearly indicates that this method requires a static lifetime for the class parameter, which improves API clarity.
589-607: Clarify the safety invariants for the unsafe cast.The safety comment mentions this can be broken "if used unconventionally" but doesn't specify what constitutes unconventional use. Consider documenting:
- The specific invariants that must be upheld (e.g., the class must outlive all its getset descriptors)
- Examples of what would constitute unsafe usage
- Why this unsafe approach is preferred over alternative safe designs
vm/src/builtins/property.rs (1)
389-394: LGTM!Correct usage of
new_static_getsetsincecontext.types.property_typeis a static type reference.vm/src/class.rs (1)
88-93: LGTM!Appropriate use of
new_static_getsetas theclassparameter has a'staticlifetime in theextend_classmethod signature.stdlib/src/pyexpat.rs (1)
21-21: LGTM!The macro correctly uses
new_static_getsetas it's called fromextend_class_with_fieldswhere the class has a'staticlifetime.vm/src/builtins/getset.rs (3)
16-16: Good use of PointerSlot for lifetime management.The change from
&'static Py<PyType>toPointerSlot<Py<PyType>>provides a safer abstraction for managing the class reference lifetime, supporting both static and heap-allocated types.
142-146: LGTM!Correct usage of
unsafe { self.class.borrow_static() }to access the class reference through PointerSlot.
152-152: LGTM!Consistent and correct usage of the unsafe borrow pattern for PointerSlot.
vm/src/builtins/type.rs (3)
34-34: LGTM: Import cleanup is consistent with trait implementation changes.Removing the unused
fmtimport aligns with the switch to fully qualified paths in the Display/Debug implementations below.
132-142: LGTM: Good practice using fully qualified paths.The switch from imported
fmtto fully qualifiedstd::fmtpaths improves code clarity and is consistent with the import cleanup above.
999-1020: Verify the safety of unsafe lifetime casting in getset creation.This restructuring introduces unsafe lifetime manipulation by passing a non-
'staticreference tonew_getset. According to the AI summary,new_getsetnow uses unsafe casting to convert non-'staticreferences to'static.Key concerns:
- The safety contract of this pattern isn't documented in the code
- Risk of use-after-free if the type object is dropped while getsets still reference it
- The unsafe block obscures the specific safety invariants being relied upon
Verify that the lifetime safety guarantees are properly maintained:
#!/bin/bash # Verify the new_getset implementation and its safety contract ast-grep --pattern 'fn new_getset($$$) { $$$ }' # Check for documentation of safety invariants rg -A 10 -B 5 "new_getset.*unsafe|unsafe.*new_getset" # Look for related unsafe casting patterns rg -A 5 "unsafe.*'static|'static.*unsafe"Please document the safety invariants that ensure the type object outlives any getset descriptors created from it.
| unsafe impl<T> Sync for PointerSlot<T> {} | ||
| unsafe impl<T> Send for PointerSlot<T> {} |
There was a problem hiding this comment.
Critical: Overly broad unsafe trait implementations pose safety risks.
These blanket unsafe impl Sync/Send for PointerSlot<T> implementations are concerning because:
- They apply to any type
Twithout safety constraints NonNull<T>alone doesn't guarantee the pointed-to data is thread-safe- This could enable unsound code if
PointerSlotis used with non-thread-safe data
Consider constraining these implementations:
-unsafe impl<T> Sync for PointerSlot<T> {}
-unsafe impl<T> Send for PointerSlot<T> {}
+unsafe impl<T: Sync> Sync for PointerSlot<T> {}
+unsafe impl<T: Send> Send for PointerSlot<T> {}Or if the use case is specifically for static/immutable type data:
-unsafe impl<T> Sync for PointerSlot<T> {}
-unsafe impl<T> Send for PointerSlot<T> {}
+unsafe impl<T: Sync + Send + 'static> Sync for PointerSlot<T> {}
+unsafe impl<T: Sync + Send + 'static> Send for PointerSlot<T> {}🤖 Prompt for AI Agents
In vm/src/builtins/type.rs at lines 72-73, the unsafe impls of Sync and Send for
PointerSlot<T> are too broad and unsafe because they apply to all T without
constraints. To fix this, add appropriate trait bounds to ensure T is Sync and
Send before implementing these traits for PointerSlot<T>. This will restrict the
implementations to only thread-safe types and prevent unsound behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
Lib/_threading_local.py (1)
68-79: Critical: Context manager may not properly restore__dict__state.Based on the AI summary, the
_patchcontext manager no longer saves and restores the old__dict__attribute. This is a significant behavioral change that could cause issues:
- State persistence: The thread-local dictionary remains set on the object after the context exits, which may not be the intended behavior.
- Memory leaks: Without proper cleanup, references to thread-local dictionaries may persist longer than expected.
- Concurrency issues: Other threads accessing the same object may see unexpected state.
If this change was intentional to work around RustPython limitations, it should be clearly documented. Otherwise, consider restoring the original behavior:
@contextmanager def _patch(self): impl = object.__getattribute__(self, '_local__impl') try: dct = impl.get_dict() except KeyError: dct = impl.create_dict() args, kw = impl.localargs self.__init__(*args, **kw) + old_dict = getattr(self, '__dict__', None) with impl.locallock: object.__setattr__(self, '__dict__', dct) - yield + try: + yield + finally: + if old_dict is not None: + object.__setattr__(self, '__dict__', old_dict)
🧹 Nitpick comments (2)
extra_tests/snippets/builtin_type.py (2)
602-602: Fix class definition formatting.The class definition violates PEP 8 formatting guidelines as flagged by static analysis.
Apply this diff to fix the formatting:
-class A(type): pass + + +class A(type): + pass
604-604: LGTM! Test validates metaclass behavior.This assertion correctly validates that subclasses of
typedon't have__dict__in their__dict__, which aligns with the conditional insertion logic mentioned in the PR summary.Consider adding a blank line after this assertion to improve readability and comply with PEP 8:
assert '__dict__' not in A.__dict__ +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Lib/_threading_local.py(1 hunks)extra_tests/snippets/builtin_type.py(1 hunks)stdlib/src/pyexpat.rs(1 hunks)vm/src/builtins/getset.rs(4 hunks)vm/src/builtins/property.rs(1 hunks)vm/src/builtins/type.rs(6 hunks)vm/src/class.rs(1 hunks)vm/src/stdlib/thread.rs(1 hunks)vm/src/vm/context.rs(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- vm/src/stdlib/thread.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- vm/src/class.rs
- vm/src/builtins/getset.rs
- stdlib/src/pyexpat.rs
- vm/src/builtins/property.rs
- vm/src/builtins/type.rs
- vm/src/vm/context.rs
🧰 Additional context used
📓 Path-based instructions (1)
`Lib/**/*`: Files in the Lib/ directory (Python standard library copied from CPy...
Lib/**/*: Files in the Lib/ directory (Python standard library copied from CPython) should be edited very conservatively; modifications should be minimal and only to work around RustPython limitations.
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
Lib/_threading_local.py
🪛 Flake8 (7.2.0)
extra_tests/snippets/builtin_type.py
[error] 602-602: expected 2 blank lines, found 1
(E302)
[error] 602-602: multiple statements on one line (colon)
(E701)
[error] 604-604: expected 2 blank lines after class or function definition, found 1
(E305)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
🔇 Additional comments (3)
extra_tests/snippets/builtin_type.py (1)
599-600: LGTM! Test assertions validate internal descriptor behavior.These assertions correctly test the
__objclass__attribute of the__dict__descriptor and verify its type information, which aligns with the refactoring of getset descriptors mentioned in the PR objectives.Lib/_threading_local.py (2)
1-7: Inconsistency: AI summary claims docstring was removed but it's still present.The AI summary states that "module-level docstring and extensive inline documentation...have been completely removed," but the docstring is clearly present in the provided code. This indicates either:
- The summary is inaccurate, or
- The changes were partially reverted
Please clarify whether the docstring removal was intentional and if so, why it appears to have been reverted.
Likely an incorrect or invalid review comment.
85-85: Verify that positional-only parameter change is necessary for RustPython compatibility.The addition of
/to enforce positional-only parameters forclsis a behavior change from the original CPython implementation. Since this is a standard library file that should be edited conservatively, ensure this change is necessary to work around a specific RustPython limitation.#!/bin/bash # Description: Search for related changes or comments about positional-only parameters in RustPython codebase # Expected: Evidence of RustPython-specific requirements for this change # Search for related changes in threading or local implementations rg -A 5 -B 5 "positional.*only|__new__.*/" --type rust rg -A 5 -B 5 "threading.*local" --type rust
11dd424 to
d8f64fb
Compare
Summary by CodeRabbit