GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||
GeezerVison is showing github.com — __slots__ xor __dict__ , name mangling by youknowone · Pull Request #6392 · RustPython/RustPython Conversation
|
WalkthroughThe pull request modifies object and type construction in RustPython. It replaces a hardcoded object-type check with a HAS_DICT flag check for dict allocation, inherits HAS_DICT from all bases in the MRO during type creation, introduces Python name mangling for private slot attributes, and adds new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/vm/src/builtins/type.rs (1)
1798-1808: Handle edge case when class name consists entirely of underscores.When
class_nameconsists only of underscores (e.g.,_,__,___),trim_start_matches('_')returns an empty string. This causes the code to produce"___x"for a name like"__x"in such a class, which diverges from CPython's behavior—CPython does not mangle names in classes whose names consist only of underscores and returns the original name unchanged.Add a check after stripping the leading underscores:
fn mangle_name(class_name: &str, name: &str) -> String { // Only mangle names starting with __ and not ending with __ if !name.starts_with("__") || name.ends_with("__") || name.contains('.') { return name.to_string(); } // Strip leading underscores from class name let class_name = class_name.trim_start_matches('_'); + if class_name.is_empty() { + return name.to_string(); + } format!("_{}{}", class_name, name) }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
Lib/test/datetimetester.pyis excluded by!Lib/**Lib/test/test_contextlib.pyis excluded by!Lib/**Lib/test/test_contextlib_async.pyis excluded by!Lib/**Lib/test/test_dataclasses.pyis excluded by!Lib/**Lib/test/test_decimal.pyis excluded by!Lib/**Lib/test/test_fractions.pyis excluded by!Lib/**Lib/test/test_functools.pyis excluded by!Lib/**Lib/test/test_os.pyis excluded by!Lib/**Lib/test/test_statistics.pyis excluded by!Lib/**Lib/test/test_typing.pyis excluded by!Lib/**
📒 Files selected for processing (3)
crates/vm/src/builtins/object.rs(1 hunks)crates/vm/src/builtins/type.rs(4 hunks)crates/vm/src/stdlib/typevar.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/vm/src/builtins/object.rscrates/vm/src/stdlib/typevar.rscrates/vm/src/builtins/type.rs
🧠 Learnings (1)
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/vm/src/builtins/type.rs
⏰ 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). (1)
- GitHub Check: Run snippets and cpython tests (windows-latest)
🔇 Additional comments (7)
crates/vm/src/builtins/type.rs (3)
310-317: LGTM! Correct HAS_DICT inheritance from MRO.The change from checking only the first base to checking any base in the MRO aligns with CPython's behavior. If any base class in the MRO has
HAS_DICT, the subclass should inherit it too since it needs to support instance dictionaries from any base.
1188-1213: LGTM! Name mangling correctly applied to slot-backed member descriptors.The implementation properly:
- Extracts the class name before processing slots
- Applies mangling to each slot attribute name
- Uses the mangled name consistently for both the
PyMemberDefand the attribute key on the typeThis matches CPython's behavior where
__xin__slots__becomes_ClassName__xas the actual attribute.
1233-1246: LGTM! Correct conditional addition of__dict__descriptor.The updated condition ensures the
__dict__descriptor is only added when:
- The base is not
type(type subclasses inherit__dict__from type)- The class has the
HAS_DICTflag (meaning__slots__was not defined, or__dict__was explicitly in__slots__)This correctly implements the
__slots__ xor __dict__semantics.crates/vm/src/builtins/object.rs (1)
69-79: LGTM! Dict creation now driven byHAS_DICTflag.The change correctly ties instance dict creation to the
HAS_DICTflag rather than a hardcoded type check. This ensures:
- Classes with
__slots__(no__dict__in slots) → no instance dict- Classes without
__slots__or with__dict__in__slots__→ instance dict createdThis aligns with the type initialization changes and properly implements Python's
__slots__semantics.crates/vm/src/stdlib/typevar.rs (3)
3-3: LGTM!The
Contextimport is required for the new__slots__method signature.
975-978: LGTM! Empty__slots__on Generic matches CPython.Defining
__slots__ = ()onGenericprevents instances from having an instance dictionary, which is the expected behavior for this typing construct. Using#[pyattr]correctly defines this as a class attribute.
986-988: LGTM!__init_subclass__delegation to typing module.The implementation correctly delegates to
_generic_init_subclassin the typing module, consistent with the existing__class_getitem__pattern and CPython's behavior for handling Generic subclass initialization.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
1 participant
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.