typing __parameters__ __type_params__ #5909
Conversation
WalkthroughThis update extends the compiler and runtime to support default values for type parameters and automatic insertion of generic base classes in line with PEP 695. It introduces new bytecode instructions, intrinsic function handling, synchronization for type parameter defaults, and updates class construction to manage type parameter attributes and defaults. Changes
Sequence Diagram(s)sequenceDiagram
participant Compiler
participant VM
participant Typing
participant TypeParam
Compiler->>VM: Emit instructions for type parameters
alt Default value present
Compiler->>VM: CallIntrinsic2(SetTypeparamDefault) with TypeParam, default
VM->>Typing: set_typeparam_default(TypeParam, default)
Typing-->>VM: Default set on TypeParam
end
Compiler->>VM: Emit class construction instructions
alt TypeParams exist and no explicit bases
Compiler->>VM: CallIntrinsic1(SubscriptGeneric) with type_params
VM->>Typing: subscript_generic(type_params)
Typing-->>VM: GenericAlias returned
Compiler->>VM: Call __build_class__ with GenericAlias as base
else Explicit bases present
Compiler->>VM: Call __build_class__ with explicit bases
else No type params
Compiler->>VM: Call __build_class__ normally
end
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yml ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
🧰 Additional context used📓 Path-based instructions (1)`**/*.rs`: Follow the default rustfmt code style (`cargo fmt` to format) Always ...
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md) List of files the instruction was applied to:
🧠 Learnings (1)vm/src/stdlib/typevar.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)
🔇 Additional comments (5)
✨ 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
|
There was a problem hiding this comment.
Summary of Changes
Hello @youknowone, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly advances the implementation of PEP 695 (Type Parameter Syntax) by introducing support for default values in type parameters and handling the implicit Generic base class for generic types. These changes involve extending the bytecode instruction set, adding new runtime intrinsic functions, and refactoring the internal representation of type parameters for improved consistency and functionality.
Highlights
- PEP 695 Type Parameter Defaults: Added compiler and runtime support for specifying default values for
TypeVar,ParamSpec, andTypeVarTupleinstances, aligning with PEP 695. This includes compiling default expressions and setting them via a new intrinsic. - Automatic
GenericBase for PEP 695 Classes: Implemented logic to automatically injecttyping.Generic[*type_params]as a base class for generic classes declared with PEP 695 syntax when no explicit base classes are provided, mirroring CPython's behavior. - New Bytecode Instructions for Type Operations: Introduced
CALL_INTRINSIC_1andCALL_INTRINSIC_2bytecode instructions, specifically forSubscriptGeneric(to handleGeneric[*type_params]creation) andSetTypeparamDefault(to set type parameter defaults at runtime). - Refactored Type Parameter Storage: Updated the internal
TypeVar,ParamSpec, andTypeVarTuplestructures to usePyMutex<PyObjectRef>forevaluate_defaultfields, ensuring thread-safe access and consistent handling of default values.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces PEP 695 features, specifically handling default values in type parameters and generic class creation. The changes are well-structured, introducing new bytecode intrinsics and updating the VM to support them. However, there's a critical stack manipulation bug in the compiler when handling default values for type parameters, which will lead to runtime errors.
4f9c355 to
2ffdc88
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
compiler/codegen/src/compile.rs (3)
1237-1250: Remove the unnecessaryDuplicateinstruction to prevent stale object on stack.The first
Duplicateinstruction will leave a stale TypeVar object on the stack. TheSetTypeparamDefaultintrinsic consumes the type parameter and default value, then pushes back the modified type parameter.Apply this diff to fix the issue:
// Handle default value if present (PEP 695) if let Some(default_expr) = default { - emit!(self, Instruction::Duplicate); - // Compile the default expression self.compile_expression(default_expr)?; emit!( self, Instruction::CallIntrinsic2 { func: bytecode::IntrinsicFunction2::SetTypeparamDefault } ); }
1261-1274: Remove the unnecessaryDuplicateinstruction to prevent stale object on stack.The first
Duplicateinstruction will leave a stale ParamSpec object on the stack. TheSetTypeparamDefaultintrinsic consumes the type parameter and default value, then pushes back the modified type parameter.Apply this diff to fix the issue:
// Handle default value if present (PEP 695) if let Some(default_expr) = default { - emit!(self, Instruction::Duplicate); - // Compile the default expression self.compile_expression(default_expr)?; emit!( self, Instruction::CallIntrinsic2 { func: bytecode::IntrinsicFunction2::SetTypeparamDefault } ); }
1285-1302: Remove the unnecessaryDuplicateinstruction to prevent stale object on stack.The first
Duplicateinstruction will leave a stale TypeVarTuple object on the stack. TheSetTypeparamDefaultintrinsic consumes the type parameter and default value, then pushes back the modified type parameter.Apply this diff to fix the issue:
// Handle default value if present (PEP 695) if let Some(default_expr) = default { - emit!(self, Instruction::Duplicate); - // Compile the default expression self.compile_expression(default_expr)?; // Handle starred expression (*default) // CPython handles the unpacking during runtime evaluation of the default value // We don't need special intrinsic for this - just compile the expression normally emit!( self, Instruction::CallIntrinsic2 { func: bytecode::IntrinsicFunction2::SetTypeparamDefault } ); }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (12)
Lib/test/_typed_dict_helper.pyis excluded by!Lib/**Lib/test/ann_module.pyis excluded by!Lib/**Lib/test/ann_module2.pyis excluded by!Lib/**Lib/test/ann_module3.pyis excluded by!Lib/**Lib/test/ann_module4.pyis excluded by!Lib/**Lib/test/ann_module5.pyis excluded by!Lib/**Lib/test/ann_module6.pyis excluded by!Lib/**Lib/test/ann_module7.pyis excluded by!Lib/**Lib/test/mod_generics_cache.pyis excluded by!Lib/**Lib/test/test_grammar.pyis excluded by!Lib/**Lib/test/test_typing.pyis excluded by!Lib/**Lib/test/typinganndata/mod_generics_cache.pyis excluded by!Lib/**
📒 Files selected for processing (7)
compiler/codegen/src/compile.rs(2 hunks)compiler/core/src/bytecode.rs(4 hunks)vm/src/builtins/genericalias.rs(1 hunks)vm/src/frame.rs(3 hunks)vm/src/stdlib/builtins.rs(1 hunks)vm/src/stdlib/typevar.rs(19 hunks)vm/src/stdlib/typing.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:
vm/src/stdlib/builtins.rsvm/src/stdlib/typing.rsvm/src/builtins/genericalias.rsvm/src/frame.rscompiler/core/src/bytecode.rscompiler/codegen/src/compile.rsvm/src/stdlib/typevar.rs
🧠 Learnings (1)
vm/src/stdlib/typevar.rs (1)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T10:08:48.858Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
🧬 Code Graph Analysis (2)
vm/src/stdlib/typing.rs (1)
vm/src/stdlib/typevar.rs (1)
set_typeparam_default(1019-1055)
vm/src/frame.rs (2)
vm/src/builtins/genericalias.rs (1)
subscript_generic(619-633)vm/src/stdlib/typevar.rs (1)
set_typeparam_default(1019-1055)
⏰ 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 snippets and cpython tests on wasm-wasi
- GitHub Check: Check the WASM package and demo
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-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: Ensure compilation on various targets
🔇 Additional comments (17)
vm/src/stdlib/typing.rs (1)
6-6: LGTM! Clean re-export for PEP 695 support.The addition of
set_typeparam_defaultto the public exports enables external access to the type parameter default functionality, which aligns with the broader PEP 695 implementation for default type parameters.vm/src/builtins/genericalias.rs (1)
616-633: Excellent implementation of CPython's_Py_subscript_genericequivalent.The function correctly handles both tuple and single parameter cases, has proper error handling, and includes clear documentation about its purpose for PEP 695 generic class support. The implementation follows Rust best practices and mirrors the expected CPython behavior.
vm/src/stdlib/builtins.rs (1)
1012-1014: Good addition for typing module compatibility.The implementation correctly sets both
__type_params__and__parameters__attributes for PEP 695 classes. The comment clearly explains the rationale for the dual attribute setup, and the error handling is appropriate.vm/src/frame.rs (5)
1053-1058: LGTM! Clean implementation of CallIntrinsic1 instruction.The instruction correctly manages the stack (pop input, push result) and follows established patterns for bytecode execution with proper error handling.
1059-1065: LGTM! Correct implementation of CallIntrinsic2 instruction.The stack operations are properly ordered (value2 popped first, then value1) and the implementation follows established bytecode execution patterns.
2260-2272: LGTM! Proper intrinsic function dispatch implementation.The method correctly matches on the intrinsic function enum and delegates to the appropriate implementation. The function signature matches the called
subscript_genericfunction.
2274-2286: LGTM! Correct implementation of two-argument intrinsic dispatch.The method properly handles the two-argument case and delegates to
set_typeparam_defaultwith the correct parameter order and types.
1304-1304: LGTM! Bug fix for missing vm parameter.The addition of the
vmparameter toParamSpec::newcorrectly aligns with the expected function signature.compiler/core/src/bytecode.rs (5)
378-405: LGTM! Well-structured intrinsic function enum.The
IntrinsicFunction1enum follows established patterns with appropriate derives and#[repr(u8)]for bytecode compatibility. The commented variants suggest a planned but gradual implementation approach for PEP 695 features.
407-419: LGTM! Complementary enum for two-argument intrinsics.The
IntrinsicFunction2enum mirrors the structure ofIntrinsicFunction1and properly implements the OpArgType trait via the macro. TheSetTypeparamDefault = 5variant aligns with the PEP 695 type parameter default functionality.
500-505: LGTM! Proper integration into instruction enum.The new
CallIntrinsic1andCallIntrinsic2instruction variants are correctly integrated into theInstructionenum with appropriate argument types. The placement maintains the existing instruction organization.
1287-1288: LGTM! Correct stack effect calculations.The stack effects are properly calculated:
CallIntrinsic1: 0 (consumes 1, pushes 1)CallIntrinsic2: -1 (consumes 2, pushes 1)These align with the expected behavior for single and binary intrinsic operations.
1498-1499: LGTM! Consistent display formatting.The display formatting uses the established
w!macro pattern with debug formatting (?func) for the intrinsic function arguments, maintaining consistency with other instruction display implementations.vm/src/stdlib/typevar.rs (4)
5-5: LGTM!The import of
PyMutexis correctly added to support the synchronization primitive change throughout the file.
80-80: Consistent synchronization primitive migration.The migration from
parking_lot::MutextoPyMutexforevaluate_defaultfields is implemented consistently across all type parameter structs (TypeVar, ParamSpec, TypeVarTuple). All access points properly acquire locks before use, ensuring thread-safe access.Also applies to: 149-151, 176-177, 364-364, 390-391, 406-406, 474-476, 485-487, 499-500, 614-614, 640-641, 655-655, 671-673, 683-684, 776-776, 799-799
1017-1055: Well-implemented type parameter default setter.The
set_typeparam_defaultfunction is elegantly implemented with proper error handling and code reuse through the generictry_set_defaultinner function. The function correctly handles all three type parameter types and provides clear error messages.
636-636: Document the breaking API change for ParamSpec::new
- Verified all internal calls to
ParamSpec::new(e.g. invm/src/frame.rs) now include the newvmparameter.- No code changes required in the repository itself.
- Please update any external consumers, extension modules, and release notes to reflect that
ParamSpec::newnow requires avm: &VirtualMachineargument.
a57d350 to
f5d036e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
compiler/codegen/src/compile.rs (3)
1237-1251: Remove the Duplicate instruction - it creates a stale stack object.The intrinsic
SetTypeparamDefaultconsumes both the type parameter and the default value, then pushes back the modified type parameter. Adding aDuplicateinstruction here will leave a stale object on the stack.This matches the previous review feedback about removing the
Duplicateinstruction to prevent stack issues.- emit!( - self, - Instruction::CallIntrinsic2 { - func: bytecode::IntrinsicFunction2::SetTypeparamDefault - } - ); - - emit!(self, Instruction::Duplicate); + emit!( + self, + Instruction::CallIntrinsic2 { + func: bytecode::IntrinsicFunction2::SetTypeparamDefault + } + );
1259-1273: Remove the Duplicate instruction - it creates a stale stack object.The intrinsic
SetTypeparamDefaultconsumes both the type parameter and the default value, then pushes back the modified type parameter. Adding aDuplicateinstruction here will leave a stale object on the stack.This matches the previous review feedback about removing the
Duplicateinstruction to prevent stack issues.- emit!( - self, - Instruction::CallIntrinsic2 { - func: bytecode::IntrinsicFunction2::SetTypeparamDefault - } - ); - - emit!(self, Instruction::Duplicate); + emit!( + self, + Instruction::CallIntrinsic2 { + func: bytecode::IntrinsicFunction2::SetTypeparamDefault + } + );
1281-1299: Remove the Duplicate instruction - it creates a stale stack object.The intrinsic
SetTypeparamDefaultconsumes both the type parameter and the default value, then pushes back the modified type parameter. Adding aDuplicateinstruction here will leave a stale object on the stack.This matches the previous review feedback about removing the
Duplicateinstruction to prevent stack issues.- emit!( - self, - Instruction::CallIntrinsic2 { - func: bytecode::IntrinsicFunction2::SetTypeparamDefault - } - ); - - emit!(self, Instruction::Duplicate); + emit!( + self, + Instruction::CallIntrinsic2 { + func: bytecode::IntrinsicFunction2::SetTypeparamDefault + } + );
🧹 Nitpick comments (1)
compiler/codegen/src/compile.rs (1)
1286-1288: Clarify the comment about starred expressions in TypeVarTuple defaults.The comment states "CPython handles the unpacking during runtime evaluation of the default value" and "We don't need special intrinsic for this". This could be clearer about why no special handling is needed and how the starred expression is properly handled.
- // Handle starred expression (*default) - // CPython handles the unpacking during runtime evaluation of the default value - // We don't need special intrinsic for this - just compile the expression normally + // Handle starred expression (*default) + // The starred expression is compiled normally as CPython handles + // the unpacking during runtime evaluation of the default value
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Lib/test/test_module/__init__.pyis excluded by!Lib/**Lib/test/test_typing.pyis excluded by!Lib/**
📒 Files selected for processing (6)
compiler/codegen/src/compile.rs(2 hunks)compiler/core/src/bytecode.rs(4 hunks)vm/src/builtins/genericalias.rs(1 hunks)vm/src/frame.rs(3 hunks)vm/src/stdlib/typevar.rs(18 hunks)vm/src/stdlib/typing.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- vm/src/stdlib/typing.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- vm/src/builtins/genericalias.rs
- vm/src/frame.rs
- compiler/core/src/bytecode.rs
- vm/src/stdlib/typevar.rs
🧰 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 8000 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
⏰ 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 snippets and cpython tests (macos-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust 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 (1)
compiler/codegen/src/compile.rs (1)
1810-1844: Verify PEP 695 Generic base insertion logic matches CPythonI didn’t find any existing tests in RustPython or CPython’s test suite that exercise this—please cross-check CPython’s implementation in Python/compile.c (the call to build_class) to ensure that:
- Generic[*type_params] is only injected when no explicit bases are provided
- No Generic is added if the class has any user-specified bases
Locations to review:
- compiler/codegen/src/compile.rs, lines 1810–1844
Consider adding tests for both scenarios to lock in the correct behavior:
class C[T]: …→ implicitly inherits fromGeneric[T]class C[T](Base): …→ no automaticGenericinsertion
f5d036e to
bd54e53
Compare
Summary by CodeRabbit
New Features
Bug Fixes
Chores