Fix __dict__ getset type #6010
Fix __dict__ getset type #6010
Conversation
WalkthroughThe change refactors the process of adding the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant PyTypeConstructor
participant TypeObject
participant BaseTypeWithDict
participant DictDescriptor
Caller->>PyTypeConstructor: Create new type
PyTypeConstructor->>TypeObject: Initialize type object
alt If base is not type
PyTypeConstructor->>TypeObject: Insert __dict__ descriptor (post-creation)
end
Caller->>TypeObject: Access __dict__
TypeObject->>BaseTypeWithDict: Find builtin base with dict
BaseTypeWithDict->>DictDescriptor: Lookup __dict__ descriptor
DictDescriptor->>TypeObject: Get or set __dict__ value
Estimated code review effort2 (60 minutes) Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
✨ 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
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
vm/src/builtins/type.rs (1)
1476-1529: Excellent refactoring that improves CPython alignment and code organization.The refactored
subtype_get_dictandsubtype_set_dictfunctions are much cleaner:✅ Proper use of the new helper functions for separation of concerns
✅ Better alignment with CPython's implementation approach
✅ Consistent error handling and messaging
✅ Appropriate fallback to generic object dict operations
✅ Clear comments indicating CPython equivalenceConsider extracting the repeated error message pattern into a helper function to reduce duplication:
fn dict_descriptor_error(obj: &PyObject, vm: &VirtualMachine) -> PyBaseExceptionRef { vm.new_type_error(format!( "this __dict__ descriptor does not support '{}' objects", obj.class().name() )) }This would eliminate the duplicated error message on lines 1484-1487, 1491-1493, 1513-1515, and 1520-1522.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
vm/src/builtins/type.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .github/copilot-instructions.md
🧠 Learnings (1)
vm/src/builtins/type.rs (1)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
🔇 Additional comments (2)
vm/src/builtins/type.rs (2)
1125-1142: Good improvement to dict descriptor handling, but clarify unsafe usage.The move from pre-creation to post-creation insertion of the
__dict__descriptor is a solid improvement that ensures the descriptor has the correct__objclass__reference. The logic and conditions look correct.However, the
unsafeblock needs justification:
- Why is
unsafenecessary here when other attribute insertions don't require it?- The comment explains the "what" but not the "why" of the unsafe usage.
Could you clarify why the
unsafeblock is required for this specific descriptor creation when other attribute insertions in the same function don't useunsafe?
1449-1474: Well-implemented helper functions that align with CPython.Both
get_builtin_base_with_dictandget_dict_descriptorare cleanly implemented:✅ Clear CPython equivalence documentation
✅ Proper flag checking (HAS_DICT, HEAPTYPE)
✅ Correct handling of thetypespecial case
✅ Appropriate use ofOptionreturn types
✅ Good function naming that matches CPython conventionsThe base chain iteration logic is correct and the descriptor lookup using
lookup_refproperly mimics CPython's_PyType_Lookup.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
vm/src/builtins/type.rs (1)
1144-1162: Document the unsafe block and verify its safety.The post-creation
__dict__descriptor insertion logic is sound and addresses the timing issue mentioned in the AI summary. However, theunsafeblock needs proper documentation.Add a safety comment explaining why the unsafe block is necessary:
+ // SAFETY: We're creating a getset descriptor with static function pointers + // (subtype_get_dict, subtype_set_dict) and the type reference is valid + // since we just created it above. unsafe { let descriptor = vm.ctx .new_getset("__dict__", &typ, subtype_get_dict, subtype_set_dict); typ.attributes.write().insert(__dict__, descriptor.into()); }The logic correctly prevents type subclasses from getting duplicate
__dict__descriptors and ensures the descriptor is only added when needed.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
vm/src/builtins/type.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .github/copilot-instructions.md
🧠 Learnings (1)
vm/src/builtins/type.rs (1)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.rs : Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust
🧬 Code Graph Analysis (1)
vm/src/builtins/type.rs (2)
vm/src/builtins/tuple.rs (1)
new_ref_typed(234-242)vm/src/builtins/object.rs (2)
object_get_dict(496-499)object_set_dict(500-503)
🔇 Additional comments (3)
vm/src/builtins/type.rs (3)
1022-1076: LGTM: Correct slots and dict interaction logic.The enhanced slots processing correctly implements Python's semantics:
- Detects
__dict__in__slots__and sets theadd_dictflag- Filters out
__dict__from the actual slots list (since it's handled specially)- Sets
HAS_DICTandMANAGED_DICTflags when no slots are defined OR when__dict__is explicitly in slotsThis aligns well with CPython's behavior where
__dict__in__slots__is treated as a special case.
1468-1548: LGTM: Well-structured helper functions that improve CPython alignment.The new helper functions are well-designed and improve the codebase:
get_builtin_base_with_dict: Correctly traverses the base chain looking for builtin types with dictionary support, with proper handling of the special case fortypeitself.
get_dict_descriptor: Clean abstraction that useslookup_refto match CPython's_PyType_LookupRefbehavior.
subtype_get_dict/subtype_set_dict: The rewrite improves error handling and follows CPython's logic more closely. The fallback toobject::object_get_dict/object::object_set_dictis appropriate.The separation of concerns makes the code more maintainable and easier to understand.
1022-1162: Verify__dict__descriptor tests continue to passI ran searches for existing
__dict__–related tests and didn’t find any Rust-side checks or direct Python tests exercising post-creation descriptor timing. To ensure this refactoring hasn’t regressed__dict__behavior, please:
- Run the Python test suite, focusing on files that reference
__dict__in extra_tests/snippets:
• extra_tests/snippets/builtin_object.py
• extra_tests/snippets/builtin_mappingproxy.py
• extra_tests/snippets/builtin_type.py
• extra_tests/snippets/builtins_module.py
• extra_tests/snippets/builtin_exceptions.py- Add or update tests that explicitly access an instance’s
__dict__before and after defining__slots__to confirm__objclass__is correctly set.- Search for any Rust tests or code paths that inspect
__objclass__on descriptors and add coverage if missing.Without explicit coverage, timing changes may slip through. Please verify these tests still pass (or add new ones) to confirm no regressions around
__dict__descriptor behavior.
Summary by CodeRabbit
__dict__attribute on types to better align with standard Python behavior, enhancing internal consistency without affecting user-facing features.