PyTypeFlags::{SEQUENCE,MAPPING}#6109
Conversation
Sorry, something went wrong.
WalkthroughAdds SEQUENCE and MAPPING type flags, applies them to builtin classes, and updates type creation to inherit/compute these flags (including via abc_tpflags). Also adds memoryview.class_getitem to support subscription-based generic aliasing. No runtime behavior changes beyond flag metadata and new classmethod. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant VM as VM Type Creation
participant Base as Base Type
participant Helpers as Helpers
note over VM,Helpers: New type initialization
Dev->>VM: create type (heap/static)
VM->>Base: read existing flags
VM->>Helpers: inherit_patma_flags(SEQUENCE, MAPPING)
Helpers-->>VM: apply missing flags from base
VM->>Helpers: check_abc_tpflags(mro)
Helpers-->>VM: parse int bits, set flags (non-overriding)
VM-->>Dev: new type with updated PyTypeFlags
sequenceDiagram
autonumber
actor User as User Code
participant Type as memoryview (type)
participant VM as VM
participant GA as PyGenericAlias
User->>Type: memoryview[...]
Type->>Type: __class_getitem__(cls, args, vm)
Type->>GA: PyGenericAlias::from_args(cls, args, vm)
GA-->>Type: generic alias
Type-->>User: return alias
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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)
vm/src/builtins/memory.rs (1)
558-560: Bug: class_getitem isn’t exposed without #[pyclassmethod]The method compiles but won’t be visible to Python without the macro attribute. The current comment suggests intent to expose it; please add #[pyclassmethod] so memoryview[...] works.
Apply:
- // TODO: Uncomment when Python adds __class_getitem__ to memoryview - // #[pyclassmethod] - fn __class_getitem__(cls: PyTypeRef, args: PyObjectRef, vm: &VirtualMachine) -> PyGenericAlias { + #[pyclassmethod] + fn __class_getitem__(cls: PyTypeRef, args: PyObjectRef, vm: &VirtualMachine) -> PyGenericAlias { PyGenericAlias::from_args(cls, args, vm) }Optionally, add a small test covering memoryview[int] creation.
I can draft a unit/integration test ensuring memoryview.class_getitem returns a PyGenericAlias and that memoryview[int].origin is memoryview.
🧹 Nitpick comments (1)
vm/src/builtins/type.rs (1)
235-244: Nit: simplify flag set constructionYou can construct COLLECTION_FLAGS directly with bitflags OR. It’s clearer and const-evaluable without from_bits_truncate.
- const COLLECTION_FLAGS: PyTypeFlags = PyTypeFlags::from_bits_truncate( - PyTypeFlags::SEQUENCE.bits() | PyTypeFlags::MAPPING.bits(), - ); + const COLLECTION_FLAGS: PyTypeFlags = PyTypeFlags::SEQUENCE | PyTypeFlags::MAPPING;
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
vm/src/builtins/dict.rs(1 hunks)vm/src/builtins/list.rs(1 hunks)vm/src/builtins/memory.rs(1 hunks)vm/src/builtins/range.rs(1 hunks)vm/src/builtins/tuple.rs(1 hunks)vm/src/builtins/type.rs(4 hunks)vm/src/types/slot.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style (cargo fmtto format)
Always run clippy to lint code (cargo clippy) before completing tasks. Fix any warnings or lints that are introduced by your 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:
vm/src/builtins/list.rsvm/src/types/slot.rsvm/src/builtins/dict.rsvm/src/builtins/tuple.rsvm/src/builtins/range.rsvm/src/builtins/type.rsvm/src/builtins/memory.rs
🧬 Code graph analysis (1)
vm/src/builtins/range.rs (2)
derive/src/lib.rs (1)
pyclass(124-128)derive-impl/src/util.rs (1)
with(305-317)
⏰ 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). (11)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run tests under miri
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Check the WASM package and demo
🔇 Additional comments (9)
vm/src/types/slot.rs (1)
126-127: Add documentation for SEQUENCE and MAPPING flagsThe new flags in
vm/src/types/slot.rsare correctly placed at bits1<<5and1<<6and already propagate as expected:
- SEQUENCE is applied to
list,tuple,range, andmemoryviewin
vm/src/builtins/{list,tuple,range,memory}.rs.- MAPPING is applied to
dictinvm/src/builtins/dict.rs.- Both flags are used in the ABC/type-creation logic (
inherit_patma_flagsand__abc_tpflags__) invm/src/builtins/type.rs.Please add these inline docs at the definition site:
--- a/vm/src/types/slot.rs +++ b/vm/src/types/slot.rs @@ lines 126-127 - const SEQUENCE = 1 << 5; - const MAPPING = 1 << 6; + /// Semantically sequence-like types (e.g., list, tuple, range, memoryview). + /// This is a nominal flag used by type/ABC logic, not a raw “has sq_* slots” check. + const SEQUENCE = 1 << 5; + /// Semantically mapping-like types (e.g., dict). + /// Nominal flag; presence of mp_* slots alone shouldn’t set this. + const MAPPING = 1 << 6;With these comments in place, the implementation and propagation checks are complete.
vm/src/builtins/list.rs (1)
112-113: Marking list with SEQUENCE is correct and consistent with its behavior.List also exposes mapping slots for index-based get/set, but not setting the MAPPING flag is appropriate; the nominal “mapping” concept shouldn’t apply to list.
vm/src/builtins/tuple.rs (1)
247-247: Tuple flagged as SEQUENCE — good alignment with protocol and slots.No further action; consistent with AsSequence implementation.
vm/src/builtins/dict.rs (1)
179-180: Dict flagged as MAPPING — matches semantics and ABC expectations.Dict keeps AsSequence for contains/len, but not marking SEQUENCE is correct.
vm/src/builtins/range.rs (1)
177-188: Range flagged as SEQUENCE — matches Python’s semantics.Length/getitem paths already go through protocol_length/downcasts appropriately; no issues spotted.
vm/src/builtins/type.rs (3)
303-308: LGTM: flag propagation order matches CPython intentInheriting from the best base and only consulting abc_tpflags if neither SEQUENCE nor MAPPING is set aligns with CPython’s “don’t override explicit/base-provided flags” behavior.
361-363: LGTM: static type creation also inherits collection flagsPropagating SEQUENCE/MAPPING for static types ensures builtin subclasses correctly advertise their pattern-matching traits.
34-34: No action needed:num-traitsis already declared and aligned
The workspace rootCargo.tomlpinsnum-traits = "0.2", and thevmcrate (along with all other workspace members) references it viaworkspace = true. Theuse num_traits::ToPrimitiveimport is satisfied—no further dependency changes are required.vm/src/builtins/memory.rs (1)
541-554: LGTM: advertise memoryview as a SEQUENCEGiven memoryview implements AsSequence (length, item) and is used as a sequence in pattern matching contexts, the flags(SEQUENCE) addition is appropriate. Keeping MAPPING out is correct despite AsMapping support, since memoryview is not semantically a mapping.
Sorry, something went wrong.
youknowone
merged commit 711b1a6
into
RustPython:main
Aug 26, 2025
Summary by CodeRabbit