typing ParamSpec #5837
typing ParamSpec #5837
Conversation
WalkthroughThis update refines the typing module implementation and its tests. It enhances Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PythonVM
participant TypingModule
User->>PythonVM: Create TypeVar/ParamSpec/TypeVarTuple
PythonVM->>TypingModule: Call constructor with args/kwargs
TypingModule->>TypingModule: Parse and validate arguments
TypingModule->>TypingModule: Set default/module attributes via set_module_from_caller
TypingModule-->>PythonVM: Return constructed object
PythonVM-->>User: Provide typing object
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (11)
🔇 Additional comments (18)
✨ 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
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.cspell.dict/python-more.txt(1 hunks)Lib/test/test_typing.py(0 hunks)vm/src/frame.rs(1 hunks)vm/src/stdlib/typing.rs(11 hunks)
💤 Files with no reviewable changes (1)
- Lib/test/test_typing.py
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
🔇 Additional comments (6)
.cspell.dict/python-more.txt (1)
20-20: LGTM!The addition of "bivariant" to the spell checking dictionary is appropriate, as this term is used in the typing module's error messages.
vm/src/frame.rs (1)
1285-1285: LGTM!The addition of the
vmparameter tomake_typevartupleis correct and necessary for proper initialization of the TypeVarTuple's default value.vm/src/stdlib/typing.rs (4)
177-187: Well-implemented operator support for TypeVar!The
AsNumbertrait implementation correctly delegates the|operator to the typing module's_make_unionfunction, enabling proper union type creation.
601-710: Excellent TypeVarTuple implementation!The implementation properly handles default values with mutex protection, includes comprehensive argument parsing and validation, and correctly integrates with the VM context for NoDefault singleton usage.
855-889: Well-implemented module attribution helpers!The
callerandset_module_from_callerfunctions correctly handle module attribution with appropriate special cases for builtins and special modules. The detailed comments explaining the differences from CPython's implementation are particularly helpful.
713-800: Clean implementation of ParamSpec helper classes!ParamSpecArgs and ParamSpecKwargs are well-implemented with proper origin tracking, equality comparisons, and string representations that follow Python's typing conventions.
| if let Some(ref default_value) = self.default_value { | ||
| // Check if default_value is NoDefault (not just None) | ||
| if !default_value.is(&vm.ctx.typing_no_default) { | ||
| return Ok(default_value.clone()); | ||
| } | ||
| } | ||
| // handle evaluate_default | ||
| if let Some(evaluate_default) = self.evaluate_default.clone() { | ||
| let default_value = vm.call_method(evaluate_default.as_ref(), "__call__", ())?; | ||
| let default_value = evaluate_default.call((), vm)?; | ||
| return Ok(default_value); | ||
| } | ||
| // TODO: this isn't up to spec | ||
| Ok(vm.ctx.none()) | ||
| // Return NoDefault singleton | ||
| Ok(vm.ctx.typing_no_default.clone().into()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using Mutex for default_value in ParamSpec
The default_value field in ParamSpec is defined as Option<PyObjectRef> while TypeVar and TypeVarTuple use parking_lot::Mutex<PyObjectRef>. This inconsistency might cause issues if the default value needs to be computed lazily and cached. Consider using the same Mutex pattern for consistency and thread safety.
pub(crate) struct ParamSpec {
name: PyObjectRef,
bound: Option<PyObjectRef>,
- default_value: Option<PyObjectRef>,
+ default_value: parking_lot::Mutex<PyObjectRef>,
evaluate_default: Option<PyObjectRef>,
covariant: bool,
contravariant: bool,
infer_variance: bool,
}And update the getter accordingly:
#[pygetset(magic)]
fn default(&self, vm: &VirtualMachine) -> PyResult {
- if let Some(ref default_value) = self.default_value {
- // Check if default_value is NoDefault (not just None)
- if !default_value.is(&vm.ctx.typing_no_default) {
- return Ok(default_value.clone());
- }
+ let mut default_value = self.default_value.lock();
+ // Check if default_value is NoDefault (not just None)
+ if !default_value.is(&vm.ctx.typing_no_default) {
+ return Ok(default_value.clone());
}
// handle evaluate_default
if let Some(evaluate_default) = self.evaluate_default.clone() {
- let default_value = evaluate_default.call((), vm)?;
- return Ok(default_value);
+ *default_value = evaluate_default.call((), vm)?;
+ return Ok(default_value.clone());
}
// Return NoDefault singleton
Ok(vm.ctx.typing_no_default.clone().into())
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In vm/src/stdlib/typing.rs around lines 394 to 407, the default_value field in
ParamSpec is currently an Option<PyObjectRef> but should be changed to use
parking_lot::Mutex<PyObjectRef> like in TypeVar and TypeVarTuple for thread
safety and lazy computation. Update the field definition to wrap the
default_value in a Mutex and modify the getter method to lock the mutex when
accessing or updating the default_value, ensuring consistent and safe concurrent
access.
There was a problem hiding this comment.
@youknowone Understood! I'll leave that as-is for now. Thanks for the clarification.
🧠 Learnings used
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-24T23:38:13.314Z
Learning: Follow Rust best practices for error handling and memory management when contributing to RustPython.
Summary by CodeRabbit