iter with slot-wrapper #6488
iter with slot-wrapper #6488
Conversation
📝 WalkthroughWalkthroughThis pull request refactors iteration support in RustPython by moving Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/vm/src/builtins/descriptor.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/class.rs
🧠 Learnings (3)
📓 Common learnings
Learnt from: jackoconnordev
Repo: RustPython/RustPython PR: 6086
File: benches/microbenchmarks/sort.py:1-3
Timestamp: 2025-08-09T22:56:24.527Z
Learning: In RustPython's microbenchmarks (benches/microbenchmarks/*.py), the variable `ITERATIONS` is intentionally used without being defined in the Python files. It is injected by the cargo bench harness at runtime. This pattern should be maintained for consistency across all microbenchmarks, and F821 lint warnings for undefined `ITERATIONS` are expected and acceptable in this context.
📚 Learning: 2025-08-09T22:56:24.527Z
Learnt from: jackoconnordev
Repo: RustPython/RustPython PR: 6086
File: benches/microbenchmarks/sort.py:1-3
Timestamp: 2025-08-09T22:56:24.527Z
Learning: In RustPython's microbenchmarks (benches/microbenchmarks/*.py), the variable `ITERATIONS` is intentionally used without being defined in the Python files. It is injected by the cargo bench harness at runtime. This pattern should be maintained for consistency across all microbenchmarks, and F821 lint warnings for undefined `ITERATIONS` are expected and acceptable in this context.
Applied to files:
crates/vm/src/builtins/descriptor.rs
📚 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/descriptor.rs
🧬 Code graph analysis (2)
crates/vm/src/builtins/descriptor.rs (2)
crates/vm/src/types/slot.rs (1)
obj(1335-1336)crates/vm/src/builtins/type.rs (3)
obj(221-221)obj(639-639)obj(1089-1089)
crates/vm/src/class.rs (1)
crates/vm/src/vm/context.rs (1)
intern_str(353-355)
⏰ 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). (3)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
🔇 Additional comments (10)
crates/vm/src/class.rs (2)
142-159: Macro consolidation looks good.The
add_slot_wrapper!macro effectively consolidates repetitive slot wrapper logic, mirroring CPython'sadd_operators()pattern. The logic correctly checks for slot existence and prevents overwriting existing attributes.
171-187: Special handling for__hash__is correct.The code properly distinguishes between
hash_not_implemented(which sets__hash__ = None) and regular hash functions (which get wrapped). This matches Python semantics for unhashable types.crates/vm/src/builtins/descriptor.rs (4)
7-7: LGTM!The
ToPyResultimport is correctly added for use in line 459'sto_pyresult(vm)call.
10-11: LGTM!The
IterFuncandIterNextFuncimports are necessary for the newSlotFunc::IterandSlotFunc::IterNextvariants.
403-404: LGTM!The new
IterandIterNextvariants correctly extend theSlotFuncenum with appropriate function pointer types, following the established pattern.
445-460: LGTM!The call handling for
IterandIterNextis correctly implemented:
- Both validate that no arguments are passed (beyond self)
IterreturnsPyResultdirectly from the underlying functionIterNextproperly convertsPyIterReturntoPyResultusingto_pyresult(vm)to handleStopIterationThe error messages match CPython's format.
crates/vm/src/types/slot.rs (4)
9-9: LGTM!The
ToPyResultimport has been appropriately moved todescriptor.rswhere it's actually used, keeping imports minimal in this file.
1438-1438: LGTM!The comment correctly documents that
__iter__is now exposed through the slot wrapper mechanism rather than as a trait method.
1458-1458: LGTM!The comment correctly documents that
__next__is now exposed through the slot wrapper mechanism rather than as a trait method.
1473-1473: LGTM!The comment correctly documents that
__iter__is now exposed through the slot wrapper mechanism for theSelfIterimplementation.
| add_slot_wrapper!( | ||
| init, | ||
| __init__, | ||
| Init, | ||
| "Initialize self. See help(type(self)) for accurate signature." | ||
| ); | ||
| add_slot_wrapper!(repr, __repr__, Repr, "Return repr(self)."); | ||
| add_slot_wrapper!(iter, __iter__, Iter, "Implement iter(self)."); | ||
| add_slot_wrapper!(iternext, __next__, IterNext, "Implement next(self)."); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for trait method implementations of __iter__ and __next__
# that might not have been migrated to the slot wrapper system
echo "=== Searching for #[pymethod] __iter__ implementations ==="
rg -n '#\[pymethod\]' -A 1 | grep -A 1 'fn __iter__'
echo ""
echo "=== Searching for #[pymethod] __next__ implementations ==="
rg -n '#\[pymethod\]' -A 1 | grep -A 1 'fn __next__'
echo ""
echo "=== Checking for trait Iterable implementations ==="
rg -n 'impl.*Iterable for' -A 5
echo ""
echo "=== Checking for trait IterNext implementations ==="
rg -n 'impl.*IterNext for' -A 5Repository: RustPython/RustPython
Length of output: 38232
🏁 Script executed:
# Also search for SlotFunc enum to verify Iter and IterNext variants exist
echo "=== Searching for SlotFunc enum definition ==="
rg -n 'enum SlotFunc' -A 30Repository: RustPython/RustPython
Length of output: 2468
🏁 Script executed:
# Check for any __iter__ or __next__ in the entire codebase to get full picture
echo "=== All __iter__ references ==="
rg -n '__iter__' crates/vm/src/
echo ""
echo "=== All __next__ references ==="
rg -n '__next__' crates/vm/src/Repository: RustPython/RustPython
Length of output: 2088
Address the #[pymethod] __iter__ implementation in contextvars.rs that uses a separate approach.
The slot wrappers are correctly added for __iter__ and __next__ in the class machinery. However, a #[pymethod] fn __iter__ exists at crates/stdlib/src/contextvars.rs:211. Consider whether this should also be migrated to the slot wrapper system to maintain consistency with the new approach, or if keeping the trait-based method is intentional.
Summary by CodeRabbit
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.