◐ Shell
clean mode source ↗

tp_itemsize by youknowone · Pull Request #6544 · RustPython/RustPython

youknowone

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9b2ad34 and 8a89e3c.

⛔ Files ignored due to path filters (1)
  • Lib/test/test_builtin.py is excluded by !Lib/**
📒 Files selected for processing (7)
  • crates/derive-impl/src/pyclass.rs
  • crates/vm/src/builtins/bytes.rs
  • crates/vm/src/builtins/int.rs
  • crates/vm/src/builtins/tuple.rs
  • crates/vm/src/builtins/type.rs
  • crates/vm/src/class.rs
  • crates/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 running cargo fmt to format Rust code
Always run clippy to lint 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/tuple.rs
  • crates/vm/src/builtins/int.rs
  • crates/vm/src/types/slot.rs
  • crates/vm/src/builtins/bytes.rs
  • crates/vm/src/class.rs
  • crates/derive-impl/src/pyclass.rs
  • crates/vm/src/builtins/type.rs
🧠 Learnings (4)
📓 Common learnings 
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.011Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
📚 Learning: 2025-08-26T05:20:54.540Z 
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6110
File: vm/src/frame.rs:1311-1316
Timestamp: 2025-08-26T05:20:54.540Z
Learning: In RustPython's pattern matching implementation, only certain builtin types should have the SEQUENCE flag: list and tuple are confirmed sequences. The user youknowone indicated that bytes, bytearray are not considered sequences in this context, even though they implement sequence-like protocols.

Applied to files:

  • crates/vm/src/builtins/int.rs
📚 Learning: 2025-08-26T05:20:54.540Z 
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6110
File: vm/src/frame.rs:1311-1316
Timestamp: 2025-08-26T05:20:54.540Z
Learning: In the RustPython codebase, only certain builtin types should be marked with the SEQUENCE flag for pattern matching. List and tuple are sequences, but bytes, bytearray, and range are not considered sequences in this context, even though they may implement sequence-like protocols.

Applied to files:

  • crates/vm/src/builtins/int.rs
📚 Learning: 2025-12-27T14:03:49.011Z 
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.011Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust

Applied to files:

  • crates/derive-impl/src/pyclass.rs
🧬 Code graph analysis (1)
crates/vm/src/builtins/bytes.rs (2)
crates/vm/src/builtins/memory.rs (1)
  • itemsize (584-586)
crates/stdlib/src/array.rs (1)
  • itemsize (738-740)
🔇 Additional comments (4)
crates/vm/src/builtins/int.rs (1)

322-326: itemsize = 4 for int looks correct but ties you to BigInt’s current layout

Using itemsize = 4 makes int a variable-sized type for layout checks and __itemsize__ introspection, matching a u32‑digit representation. If malachite’s internal digit size ever changes, this constant will need to be revisited (and tests around int.__itemsize__/type(int).slots.itemsize would catch that).

crates/vm/src/builtins/bytes.rs (1)

189-205: itemsize = 1 on bytes is consistent with element size and flag usage

Marking bytes with itemsize = 1 aligns tp_itemsize with the per‑byte element size and lets the type/slots machinery treat it as variable‑sized, without changing SEQUENCE flags (bytes still only has BASETYPE and _MATCH_SELF, which is consistent with earlier decisions about sequence flags).

crates/vm/src/types/slot.rs (1)

120-187: New itemsize field cleanly exposes tp_itemsize in PyTypeSlots

Adding pub itemsize: usize with a Default of 0 and wiring it through PyTypeSlots::new/heap_default is sound; types that care about variable-size layout now have an explicit slot to populate, while existing code that doesn’t touch itemsize continues to see a fixed-size (0) default.

crates/vm/src/builtins/tuple.rs (1)

259-263: Tuple itemsize correctly reflects per-element pointer size

Setting itemsize = std::mem::size_of::<crate::PyObjectRef>() makes tuple.__itemsize__ line up with the pointer-sized element layout and lets the type system treat tuples as variable-sized for layout/slots checks. Flags still include SEQUENCE, which matches the earlier guidance that only list and tuple should carry the SEQUENCE flag for pattern matching.