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 — Add tp_str by youknowone · Pull Request #6495 · RustPython/RustPython Conversation
|
📝 WalkthroughWalkthroughThis PR adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/vm/src/types/slot.rs (1)
488-873: Critical: Missingupdate_slotcase for__str__.The
update_slotfunction handles dynamic slot updates when attributes are added/removed from types, but there's no case for__str__. This means when a heap type dynamically defines or removes__str__, thestrslot won't be updated, potentially breaking the slot mechanism for dynamically defined__str__methods.Add a case similar to
__repr__(lines 570-572):🔎 Proposed fix
_ if name == identifier!(ctx, __repr__) => { update_slot!(repr, repr_wrapper); } + _ if name == identifier!(ctx, __str__) => { + toggle_slot!(str, str_wrapper); + } _ if name == identifier!(ctx, __hash__) => {Note: You'll also need to define
str_wrapperfunction similar torepr_wrapper(line 351-359) if it doesn't already exist:fn str_wrapper(zelf: &PyObject, vm: &VirtualMachine) -> PyResult<PyRef<PyStr>> { let ret = vm.call_special_method(zelf, identifier!(vm, __str__), ())?; ret.downcast::<PyStr>().map_err(|obj| { vm.new_type_error(format!( "__str__ returned non-string (type {})", obj.class() )) }) }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Lib/test/test_inspect/test_inspect.pyis excluded by!Lib/**
📒 Files selected for processing (4)
crates/vm/src/builtins/descriptor.rscrates/vm/src/builtins/object.rscrates/vm/src/class.rscrates/vm/src/types/slot.rs
🧰 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/descriptor.rscrates/vm/src/types/slot.rscrates/vm/src/builtins/object.rscrates/vm/src/class.rs
🧬 Code graph analysis (1)
crates/vm/src/class.rs (2)
crates/vm/src/types/slot.rs (4)
iter(1440-1440)iter(1476-1478)hash_not_implemented(374-376)hash(1103-1103)crates/vm/src/builtins/set.rs (2)
hash(438-461)hash(1155-1173)
⏰ 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). (2)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
🔇 Additional comments (5)
crates/vm/src/types/slot.rs (1)
143-143: LGTM:strslot field added correctly.The new
strfield follows the same pattern as the existingreprslot on line 144.crates/vm/src/builtins/descriptor.rs (2)
402-402: LGTM:Strvariant added correctly.The new
Str(StringifyFunc)variant properly extends theSlotFuncenum to support the__str__slot.
438-449: LGTM: Unified handling ofReprandStrimproves maintainability.The refactored approach consolidates the argument validation and dispatch logic for both
__repr__and__str__, reducing code duplication while correctly differentiating error messages.crates/vm/src/class.rs (2)
168-168: LGTM:__str__slot wrapper added correctly.The slot wrapper follows the same pattern as
__repr__on line 167 and properly integrates the newStrslot with the class implementation machinery.
173-177: LGTM: Cleaner__hash__handling.The refactored approach using
map_orand pointer comparison is more concise and readable than the previous implementation, while maintaining the same behavior.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| #[pyslot] | ||
| fn slot_str(zelf: &PyObject, vm: &VirtualMachine) -> PyResult<PyStrRef> { | ||
| // FIXME: try tp_repr first and fallback to object.__repr__ | ||
| zelf.repr(vm) | ||
| } |
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.
Minor: Clarify the FIXME comment.
The conversion to a slot-based implementation is correct, but the FIXME comment is confusing. It mentions "try tp_repr first" but this is the __str__ slot implementation. While the fallback to repr is correct per CPython behavior (where object.__str__ calls object.__repr__), the comment should clarify this intent.
🔎 Suggested comment improvement
- // FIXME: try tp_repr first and fallback to object.__repr__ + // Default __str__ implementation falls back to __repr__ + // CPython behavior: object.__str__() calls object.__repr__() fn slot_str(zelf: &PyObject, vm: &VirtualMachine) -> PyResult<PyStrRef> {
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[pyslot] | |
| fn slot_str(zelf: &PyObject, vm: &VirtualMachine) -> PyResult<PyStrRef> { | |
| // FIXME: try tp_repr first and fallback to object.__repr__ | |
| zelf.repr(vm) | |
| } | |
| #[pyslot] | |
| fn slot_str(zelf: &PyObject, vm: &VirtualMachine) -> PyResult<PyStrRef> { | |
| // Default __str__ implementation falls back to __repr__ | |
| // CPython behavior: object.__str__() calls object.__repr__() | |
| zelf.repr(vm) | |
| } |
🤖 Prompt for AI Agents
In crates/vm/src/builtins/object.rs around lines 428 to 432, the FIXME comment
is misleading because this is the __str__ slot implementation and should clarify
that we call the object's str (tp_str) and, per CPython semantics for
object.__str__, fall back to object.__repr__ via zelf.repr(vm); update the
comment to explicitly state "This is the __str__ slot: use tp_str, and fall back
to object.__repr__ (implemented here by calling zelf.repr(vm))" so future
readers understand the intent.
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
Labels
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.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.