type.__type_params__ - #5882
Conversation
Sorry, something went wrong.
WalkthroughThe changes introduce a new property __type_params__ with getter and setter methods to the PyType class, add logic to handle starred expressions in type annotations within the compiler, and update the spell checker configuration to ignore files in target directories. No changes were made to public APIs. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Compiler
participant Bytecode
User->>Compiler: Provide annotation (possibly starred)
Compiler->>Compiler: Check if annotation is starred
alt Starred Expression
Compiler->>Compiler: Compile inner value
Compiler->>Bytecode: Emit UnpackSequence(1)
else Non-starred
Compiler->>Bytecode: Compile annotation directly
end
sequenceDiagram
participant User
participant PyType
User->>PyType: Get __type_params__
PyType->>PyType: Return tuple if exists, else empty tuple
User->>PyType: Set __type_params__ (to tuple)
PyType->>PyType: Store tuple
User->>PyType: Delete __type_params__
PyType->>User: Raise TypeError
Poem
📜 Recent review details Configuration used: .coderabbit.yml Reviewing files that changed from the base of the PR and between ba8730d and be60fe9. ⛔ Files ignored due to path filters (2)
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)vm/src/builtins/type.rs (1)📜 Review details853-864: Consider improving type validation in the getter.
The getter implementation has a potential inconsistency: if __type_params__ is manually set to a non-tuple value, the downcast will fail silently and return an empty tuple. This could be confusing for users.
Consider adding explicit validation or error handling:
#[pygetset] fn __type_params__(&self, vm: &VirtualMachine) -> PyTupleRef { let attrs = self.attributes.read(); let key = vm.ctx.intern_str("__type_params__"); if let Some(params) = attrs.get(&key) { if let Ok(tuple) = params.clone().downcast::<PyTuple>() { return tuple; } + // Log or handle case where __type_params__ exists but isn't a tuple + vm_trace!("__type_params__ exists but is not a tuple, returning empty tuple"); } // Return empty tuple if not found or not a tuple vm.ctx.empty_tuple.clone() }
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Reviewing files that changed from the base of the PR and between 3d951a8 and ba8730d.
⛔ Files ignored due to path filters (1)- Lib/test/test_typing.py is excluded by !Lib/**
- .cspell.json (1 hunks)
- compiler/codegen/src/compile.rs (1 hunks)
- vm/src/builtins/type.rs (1 hunks)
**/*.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
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
- compiler/codegen/src/compile.rs
- vm/src/builtins/type.rs
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run tests under miri
- 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 (ubuntu-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
.cspell.json (1)45-45: LGTM - Appropriate spell checker exclusion.
Adding target/** to ignore paths is a sensible change since target directories contain build artifacts and compiled files that don't require spell checking.
compiler/codegen/src/compile.rs (1)2904-2918: LGTM! Well-implemented PEP 646 support for starred type annotations.
The special handling for starred expressions in type annotations is correctly implemented:
- Proper state management by preserving and restoring in_annotation flag
- Correct unpacking logic using UnpackSequence { size: 1 } to handle *Ts TypeVarTuple expressions
- Clean fallback to normal expression compilation for non-starred annotations
- Consistent error handling with the ? operator
The implementation aligns well with CPython's approach for handling starred type annotations as mentioned in the comment.
Sorry, something went wrong.
| #[pygetset(setter)] | ||
| fn set___type_params__( | ||
| &self, | ||
| value: PySetterValue<PyTupleRef>, | ||
| vm: &VirtualMachine, | ||
| ) -> PyResult<()> { | ||
| match value { | ||
| PySetterValue::Assign(value) => { | ||
| let mut attrs = self.attributes.write(); | ||
| let key = vm.ctx.intern_str("__type_params__"); | ||
| attrs.insert(key, value.into()); | ||
| } | ||
| PySetterValue::Delete => { | ||
| return Err(vm.new_type_error("__type_params__ must be set to a tuple object")); | ||
| } | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding immutability check in the setter.
The setter correctly enforces tuple types and provides appropriate error messages for deletion attempts. However, it doesn't check if the type is immutable before allowing modifications.
Add immutability check consistent with other setters in this file:
#[pygetset(setter)]
fn set___type_params__(
&self,
value: PySetterValue<PyTupleRef>,
vm: &VirtualMachine,
) -> PyResult<()> {
+ // Check if this is an immutable type
+ if self.slots.flags.has_feature(PyTypeFlags::IMMUTABLETYPE) {
+ return Err(vm.new_type_error(format!(
+ "cannot set '__type_params__' attribute of immutable type '{}'",
+ self.name()
+ )));
+ }
+
match value {
PySetterValue::Assign(value) => {
let mut attrs = self.attributes.write();
let key = vm.ctx.intern_str("__type_params__");
attrs.insert(key, value.into());
}
PySetterValue::Delete => {
return Err(vm.new_type_error("__type_params__ must be set to a tuple object"));
}
}
Ok(())
}‼️ 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.
| #[pygetset(setter)] | |
| fn set___type_params__( | |
| &self, | |
| value: PySetterValue<PyTupleRef>, | |
| vm: &VirtualMachine, | |
| ) -> PyResult<()> { | |
| match value { | |
| PySetterValue::Assign(value) => { | |
| let mut attrs = self.attributes.write(); | |
| let key = vm.ctx.intern_str("__type_params__"); | |
| attrs.insert(key, value.into()); | |
| } | |
| PySetterValue::Delete => { | |
| return Err(vm.new_type_error("__type_params__ must be set to a tuple object")); | |
| } | |
| } | |
| Ok(()) | |
| } | |
| #[pygetset(setter)] | |
| fn set___type_params__( | |
| &self, | |
| value: PySetterValue<PyTupleRef>, | |
| vm: &VirtualMachine, | |
| ) -> PyResult<()> { | |
| // Check if this is an immutable type | |
| if self.slots.flags.has_feature(PyTypeFlags::IMMUTABLETYPE) { | |
| return Err(vm.new_type_error(format!( | |
| "cannot set '__type_params__' attribute of immutable type '{}'", | |
| self.name() | |
| ))); | |
| } | |
| match value { | |
| PySetterValue::Assign(value) => { | |
| let mut attrs = self.attributes.write(); | |
| let key = vm.ctx.intern_str("__type_params__"); | |
| attrs.insert(key, value.into()); | |
| } | |
| PySetterValue::Delete => { | |
| return Err(vm.new_type_error("__type_params__ must be set to a tuple object")); | |
| } | |
| } | |
| Ok(()) | |
| } |
In vm/src/builtins/type.rs around lines 866 to 883, the setter for __type_params__ lacks a check for immutability before modifying the attribute. To fix this, add a check at the start of the setter that verifies if the type is immutable, similar to other setters in this file, and return an error if it is immutable to prevent modification. This ensures consistency and enforces immutability constraints properly.
Sorry, something went wrong.
Summary by CodeRabbit
New Features
Improvements
Chores