type.__type_params__ #5882
type.__type_params__ #5882
Conversation
WalkthroughThe changes introduce a new property 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 detailsConfiguration used: .coderabbit.yml ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (11)
✨ Finishing Touches
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. 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
|
![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
🧹 Nitpick comments (1)
vm/src/builtins/type.rs (1)
853-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() }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Lib/test/test_typing.pyis excluded by!Lib/**
📒 Files selected for processing (3)
.cspell.json(1 hunks)compiler/codegen/src/compile.rs(1 hunks)vm/src/builtins/type.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.rs`: Follow the default rustfmt code style (`cargo fmt` to format) Always ...
**/*.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
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
compiler/codegen/src/compile.rsvm/src/builtins/type.rs
⏰ Context from checks skipped due to timeout of 90000ms (11)
- 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)
🔇 Additional comments (2)
.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_annotationflag- Correct unpacking logic using
UnpackSequence { size: 1 }to handle*TsTypeVarTuple expressions- Clean fallback to normal expression compilation for non-starred annotations
- Consistent error handling with the
?operatorThe implementation aligns well with CPython's approach for handling starred type annotations as mentioned in the comment.
vm/src/builtins/type.rs
Outdated
| #[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(())
}📝 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.
| #[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(()) | |
| } |
🤖 Prompt for AI Agents
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.
Summary by CodeRabbit
New Features
__type_params__to types, allowing controlled access and assignment of type parameters.Improvements
Chores
targetdirectory.