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.
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. ❤️ Share 🪧 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 @coderabbitai help to get the list of available commands. Other keywords and placeholders
Status, Documentation and Community
|
Sorry, something went wrong.
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.
vm/src/builtins/type.rs (1)📜 Review details235-244: Nit: simplify flag set construction
You 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;
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.
📥 CommitsReviewing files that changed from the base of the PR and between dae9584 and 96364ac.
📒 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)
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style (cargo fmt to 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.rs
- vm/src/types/slot.rs
- vm/src/builtins/dict.rs
- vm/src/builtins/tuple.rs
- vm/src/builtins/range.rs
- vm/src/builtins/type.rs
- vm/src/builtins/memory.rs
derive/src/lib.rs (1)⏰ 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)derive-impl/src/util.rs (1)
- pyclass (124-128)
- with (305-317)
- 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
vm/src/types/slot.rs (1)vm/src/builtins/list.rs (1)126-127: Add documentation for SEQUENCE and MAPPING flags
The new flags in vm/src/types/slot.rs are correctly placed at bits 1<<5 and 1<<6 and already propagate as expected:
- SEQUENCE is applied to list, tuple, range, and memoryview in
vm/src/builtins/{list,tuple,range,memory}.rs.- MAPPING is applied to dict in vm/src/builtins/dict.rs.
- Both flags are used in the ABC/type-creation logic (inherit_patma_flags and __abc_tpflags__) in vm/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/tuple.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/dict.rs (1)247-247: Tuple flagged as SEQUENCE — good alignment with protocol and slots.
No further action; consistent with AsSequence implementation.
vm/src/builtins/range.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/type.rs (3)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/memory.rs (1)303-308: LGTM: flag propagation order matches CPython intent
Inheriting 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 flags
Propagating SEQUENCE/MAPPING for static types ensures builtin subclasses correctly advertise their pattern-matching traits.
34-34: No action needed: num-traits is already declared and aligned
The workspace root Cargo.toml pins num-traits = "0.2", and the vm crate (along with all other workspace members) references it via workspace = true. The use num_traits::ToPrimitive import is satisfied—no further dependency changes are required.541-554: LGTM: advertise memoryview as a SEQUENCE
Given 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.
| /// Check for __abc_tpflags__ and set the appropriate flags | ||
| /// This checks in attrs and all base classes for __abc_tpflags__ | ||
| fn check_abc_tpflags( | ||
| slots: &mut PyTypeSlots, | ||
| attrs: &PyAttributes, | ||
| bases: &[PyRef<Self>], | ||
| ctx: &Context, | ||
| ) { | ||
| const COLLECTION_FLAGS: PyTypeFlags = PyTypeFlags::from_bits_truncate( | ||
| PyTypeFlags::SEQUENCE.bits() | PyTypeFlags::MAPPING.bits(), | ||
| ); | ||
|
|
||
| // Don't override if flags are already set | ||
| if slots.flags.intersects(COLLECTION_FLAGS) { | ||
| return; | ||
| } | ||
|
|
||
| // First check in our own attributes | ||
| let abc_tpflags_name = ctx.intern_str("__abc_tpflags__"); | ||
| if let Some(abc_tpflags_obj) = attrs.get(abc_tpflags_name) { | ||
| if let Some(int_obj) = abc_tpflags_obj.downcast_ref::<crate::builtins::int::PyInt>() { | ||
| let flags_val = int_obj.as_bigint().to_i64().unwrap_or(0); | ||
| let abc_flags = PyTypeFlags::from_bits_truncate(flags_val as u64); | ||
| slots.flags |= abc_flags & COLLECTION_FLAGS; | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| // Then check in base classes | ||
| for base in bases { | ||
| if let Some(abc_tpflags_obj) = base.find_name_in_mro(abc_tpflags_name) { | ||
| if let Some(int_obj) = abc_tpflags_obj.downcast_ref::<crate::builtins::int::PyInt>() | ||
| { | ||
| let flags_val = int_obj.as_bigint().to_i64().unwrap_or(0); | ||
| let abc_flags = PyTypeFlags::from_bits_truncate(flags_val as u64); | ||
| slots.flags |= abc_flags & COLLECTION_FLAGS; | ||
| return; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix: avoid silently zeroing or truncating abc_tpflags when converting from PyInt
Using to_i64().unwrap_or(0) will:
- turn any large positive flag value (> i64::MAX) into 0,
- turn any negative value into 0,
- silently mask real issues.
At minimum, prefer an unsigned conversion and ignore non-convertible values; this avoids introducing incorrect zero flags. If you want CPython-compatible behavior (PyLong_AsUnsignedLongMask), you can additionally mask to the low 64 bits, but even the minimal fix below prevents accidental zeroing.
Apply this diff (both occurrences inside check_abc_tpflags):
- if let Some(int_obj) = abc_tpflags_obj.downcast_ref::<crate::builtins::int::PyInt>() {
- let flags_val = int_obj.as_bigint().to_i64().unwrap_or(0);
- let abc_flags = PyTypeFlags::from_bits_truncate(flags_val as u64);
- slots.flags |= abc_flags & COLLECTION_FLAGS;
- return;
- }
+ if let Some(int_obj) = abc_tpflags_obj.downcast_ref::<crate::builtins::int::PyInt>() {
+ if let Some(flags_val) = int_obj.as_bigint().to_u64() {
+ let abc_flags = PyTypeFlags::from_bits_truncate(flags_val);
+ slots.flags |= abc_flags & COLLECTION_FLAGS;
+ return;
+ }
+ }And:
- if let Some(int_obj) = abc_tpflags_obj.downcast_ref::<crate::builtins::int::PyInt>()
- {
- let flags_val = int_obj.as_bigint().to_i64().unwrap_or(0);
- let abc_flags = PyTypeFlags::from_bits_truncate(flags_val as u64);
- slots.flags |= abc_flags & COLLECTION_FLAGS;
- return;
- }
+ if let Some(int_obj) = abc_tpflags_obj.downcast_ref::<crate::builtins::int::PyInt>()
+ {
+ if let Some(flags_val) = int_obj.as_bigint().to_u64() {
+ let abc_flags = PyTypeFlags::from_bits_truncate(flags_val);
+ slots.flags |= abc_flags & COLLECTION_FLAGS;
+ return;
+ }
+ }If you’d like parity with PyLong_AsUnsignedLongMask, I can follow up with a small helper that extracts the low 64 bits (two’s complement) from BigInt.
📝 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.
| /// Check for __abc_tpflags__ and set the appropriate flags | |
| /// This checks in attrs and all base classes for __abc_tpflags__ | |
| fn check_abc_tpflags( | |
| slots: &mut PyTypeSlots, | |
| attrs: &PyAttributes, | |
| bases: &[PyRef<Self>], | |
| ctx: &Context, | |
| ) { | |
| const COLLECTION_FLAGS: PyTypeFlags = PyTypeFlags::from_bits_truncate( | |
| PyTypeFlags::SEQUENCE.bits() | PyTypeFlags::MAPPING.bits(), | |
| ); | |
| // Don't override if flags are already set | |
| if slots.flags.intersects(COLLECTION_FLAGS) { | |
| return; | |
| } | |
| // First check in our own attributes | |
| let abc_tpflags_name = ctx.intern_str("__abc_tpflags__"); | |
| if let Some(abc_tpflags_obj) = attrs.get(abc_tpflags_name) { | |
| if let Some(int_obj) = abc_tpflags_obj.downcast_ref::<crate::builtins::int::PyInt>() { | |
| let flags_val = int_obj.as_bigint().to_i64().unwrap_or(0); | |
| let abc_flags = PyTypeFlags::from_bits_truncate(flags_val as u64); | |
| slots.flags |= abc_flags & COLLECTION_FLAGS; | |
| return; | |
| } | |
| } | |
| // Then check in base classes | |
| for base in bases { | |
| if let Some(abc_tpflags_obj) = base.find_name_in_mro(abc_tpflags_name) { | |
| if let Some(int_obj) = abc_tpflags_obj.downcast_ref::<crate::builtins::int::PyInt>() | |
| { | |
| let flags_val = int_obj.as_bigint().to_i64().unwrap_or(0); | |
| let abc_flags = PyTypeFlags::from_bits_truncate(flags_val as u64); | |
| slots.flags |= abc_flags & COLLECTION_FLAGS; | |
| return; | |
| } | |
| } | |
| } | |
| } | |
| /// Check for __abc_tpflags__ and set the appropriate flags | |
| /// This checks in attrs and all base classes for __abc_tpflags__ | |
| fn check_abc_tpflags( | |
| slots: &mut PyTypeSlots, | |
| attrs: &PyAttributes, | |
| bases: &[PyRef<Self>], | |
| ctx: &Context, | |
| ) { | |
| const COLLECTION_FLAGS: PyTypeFlags = PyTypeFlags::from_bits_truncate( | |
| PyTypeFlags::SEQUENCE.bits() | PyTypeFlags::MAPPING.bits(), | |
| ); | |
| // Don't override if flags are already set | |
| if slots.flags.intersects(COLLECTION_FLAGS) { | |
| return; | |
| } | |
| // First check in our own attributes | |
| let abc_tpflags_name = ctx.intern_str("__abc_tpflags__"); | |
| if let Some(abc_tpflags_obj) = attrs.get(abc_tpflags_name) { | |
| if let Some(int_obj) = abc_tpflags_obj.downcast_ref::<crate::builtins::int::PyInt>() { | |
| if let Some(flags_val) = int_obj.as_bigint().to_u64() { | |
| let abc_flags = PyTypeFlags::from_bits_truncate(flags_val); | |
| slots.flags |= abc_flags & COLLECTION_FLAGS; | |
| return; | |
| } | |
| } | |
| } | |
| // Then check in base classes | |
| for base in bases { | |
| if let Some(abc_tpflags_obj) = base.find_name_in_mro(abc_tpflags_name) { | |
| if let Some(int_obj) = abc_tpflags_obj.downcast_ref::<crate::builtins::int::PyInt>() { | |
| if let Some(flags_val) = int_obj.as_bigint().to_u64() { | |
| let abc_flags = PyTypeFlags::from_bits_truncate(flags_val); | |
| slots.flags |= abc_flags & COLLECTION_FLAGS; | |
| return; | |
| } | |
| } | |
| } | |
| } | |
| } |
In vm/src/builtins/type.rs around lines 245–286, the code uses to_i64().unwrap_or(0) when reading __abc_tpflags__, which can silently turn large/negative BigInt values into 0; replace that conversion with an unsigned-aware approach: call the BigInt -> u64 conversion (e.g. ToPrimitive::to_u64 or an equivalent helper) and if it returns None, treat the value as non-convertible and skip applying flags (do not default to 0); if you need CPython parity instead, extract the low 64 bits (unsigned mask) from the BigInt (two’s complement) and use that u64. Apply this change in both places inside check_abc_tpflags so flags are not silently zeroed or truncated.
Sorry, something went wrong.
Summary by CodeRabbit