__type_params__ in __build_class__ - #5883
Conversation
Sorry, something went wrong.
WalkthroughThe changes add support for handling type parameters (as per PEP 695) in both the compiler and runtime. The compiler now compiles and stores type parameters for classes and functions in a dedicated local variable and symbol table scope, while the VM manages these parameters during class body execution and assigns them to the class’s __type_params__ attribute. Additionally, several built-in types have their __class_getitem__ methods temporarily disabled by commenting out their #[pyclassmethod] attributes until upstream Python adds support. Changes
Sequence Diagram(s)sequenceDiagram
participant Compiler
participant VM
participant Namespace
participant ClassBody
participant ClassObject
Compiler->>Compiler: Compile class/function with type_params
Compiler->>Compiler: Push special symbol table scope
Compiler->>Compiler: Compile and store type_params in .type_params local
Compiler->>Compiler: Compile body
Compiler->>Compiler: Load .type_params and store as __type_params__ local
Compiler->>Compiler: Pop special symbol table scope
Compiler->>VM: Emit code referencing .type_params and set TYPE_PARAMS flag
VM->>VM: __build_class__ called
VM->>Namespace: If __type_params__ exists and non-empty, set .type_params in namespace
VM->>ClassBody: Invoke class body function
VM->>Namespace: Remove .type_params from namespace after execution
VM->>ClassObject: After class creation, set __type_params__ attribute from function if present
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Clippy (1.86.0)Updating git repository `https://github.com/youknowone/ferrilab` error: failed to load source for dependency radium Caused by: Caused by: Caused by: 📜 Recent review details Configuration used: .coderabbit.yml Reviewing files that changed from the base of the PR and between 1a1b6f3 and 15263f9. ⛔ Files ignored due to path filters (1)
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/stdlib/builtins.rs (1)📜 Review details935-948: Implementation correctly handles type parameters for PEP 695 classes.
The code properly retrieves __type_params__ from the function object and makes it available in the namespace during class body execution. Good use of type checking with downcast.
Consider whether silently ignoring the get_attr error is intentional. If __type_params__ is expected but missing, you might want to log or handle this case explicitly.
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Reviewing files that changed from the base of the PR and between fa7af0e and 5a33d24.
⛔ Files ignored due to path filters (1)- Lib/test/test_typing.py is excluded by !Lib/**
- compiler/codegen/src/compile.rs (4 hunks)
- compiler/codegen/src/symboltable.rs (1 hunks)
- vm/src/stdlib/builtins.rs (3 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/symboltable.rs
- vm/src/stdlib/builtins.rs
- compiler/codegen/src/compile.rs
Learnt from: CR PR: RustPython/RustPython#0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-06-30T10:08:48.851Z Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
compiler/codegen/src/symboltable.rs (1)1270-1270: LGTM! Good documentation addition.
The comment clearly explains the purpose of the subsequent code block.
vm/src/stdlib/builtins.rs (1)961-965: Proper cleanup of temporary namespace entry.
Good practice to remove the internal .type_params key after use, maintaining CPython compatibility. The use of .ok() is appropriate since the key may not exist in all cases.
compiler/codegen/src/compile.rs (4)1215-1215: LGTM: Clear documentation improvement.
The added comment helpfully clarifies the purpose of the type parameter compilation process.
1668-1674: LGTM: Correct type parameter compilation and storage.
The implementation properly:
- Guards the type parameter handling with a conditional check
- Manages symbol table scope correctly by pushing a new table
- Reuses existing compile_type_params functionality
- Uses the well-chosen .type_params variable name to avoid conflicts
- Employs the appropriate StoreLocal instruction for the scope
1696-1707: LGTM: Correct PEP 695 type_params implementation.
This properly implements the PEP 695 requirement by:
- Making type parameters available as __type_params__ during class body execution
- Using LoadNameAny to correctly access .type_params from the enclosing scope
- Using StoreLocal to properly set the variable in the class body scope
- Including clear comments explaining the PEP 695 compliance purpose
1741-1745: LGTM: Proper type parameter preparation for class creation.
This correctly implements the final phase of type parameter handling by:
- Loading the previously compiled .type_params from the appropriate scope
- Setting the TYPE_PARAMS flag to inform the runtime that type parameters are present
- Using the correct LoadNameAny instruction for cross-scope access
The implementation ensures type parameters are properly passed to the __build_class__ builtin.
Sorry, something went wrong.
| // For PEP 695 classes, set __parameters__ from __type_params__ (CPython compatibility) | ||
| if let Ok(type_params) = class.get_attr(identifier!(vm, __type_params__), vm) { | ||
| if let Ok(type_params_tuple) = type_params.clone().downcast::<PyTuple>() { | ||
| if !type_params_tuple.is_empty() { | ||
| // Extract TypeVar/ParamSpec/TypeVarTuple objects from the tuple | ||
| let mut parameters = Vec::new(); | ||
| for param in type_params_tuple.iter() { | ||
| // Check if this is a TypeVar, ParamSpec, or TypeVarTuple | ||
| if param.get_attr(identifier!(vm, __name__), vm).is_ok() { | ||
| parameters.push(param.clone()); | ||
| } | ||
| } | ||
| if !parameters.is_empty() { | ||
| let parameters_tuple = PyTuple::new_ref(parameters, &vm.ctx); | ||
| class.set_attr(identifier!(vm, __parameters__), parameters_tuple, vm)?; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chainCorrectly implements __parameters__ attribute for PEP 695 compatibility.
The code properly extracts type parameter objects and sets them as the __parameters__ attribute on the class, matching CPython's behavior.
The code uses the presence of __name__ attribute to identify TypeVar/ParamSpec/TypeVarTuple objects. While this should work correctly for objects coming from __type_params__, let's verify this matches CPython's exact behavior:
🌐 Web query:
How does CPython determine which objects from __type_params__ should be included in __parameters__ for PEP 695 generic classes?
💡 Result:
In CPython, when implementing PEP 695 for generic classes, the __type_params__ attribute is used to store the type parameters that parameterize the class. This attribute is a tuple containing instances of TypeVar, TypeVarTuple, and ParamSpec. The __parameters__ attribute, on the other hand, is a tuple that includes the type parameters that are directly used in the class's method signatures.
The determination of which objects from __type_params__ should be included in __parameters__ is based on the usage of these type parameters within the class's methods. Specifically, if a type parameter from __type_params__ is used in the signature of a method (for example, as a type annotation for a method parameter or return type), it is included in __parameters__. This inclusion ensures that the method signatures accurately reflect the types that the class is parameterized with, maintaining consistency and clarity in the class's interface.
This approach aligns with the goals of PEP 695, which aims to simplify the syntax for defining generic classes and to make the relationship between a class and its type parameters more explicit and intuitive. By clearly associating type parameters with method signatures, developers can more easily understand and utilize generic classes in their code.
Implementation of __parameters__ needs to filter by method‐signature usage
The CPython logic for PEP 695 only includes type parameters in __parameters__ if they actually appear in the class’s method signatures (as annotations). Simply checking for a __name__ attribute will pull in all TypeVar/ParamSpec/TypeVarTuple instances, not just those used on methods.
Please update the block in
vm/src/stdlib/builtins.rs (lines 984–1002)
so that, after collecting candidates from __type_params__, you further scan the class’s methods’ __annotations__ and include only those parameters that appear there.
[e.g.]
- // heuristic: has __name__
- if param.get_attr(__name__, vm).is_ok() {
- parameters.push(param.clone());
- }
+ // only include if this TypeVar/ParamSpec/TypeVarTuple
+ // is referenced in any method signature annotation:
+ if class
+ .get_methods()
+ .iter()
+ .any(|func| func.annotations().contains(¶m))
+ {
+ parameters.push(param.clone());
+ }— adjust to match your VM’s API for iterating methods and inspecting annotations.
This will ensure feature parity with CPython’s PEP 695 implementation.
🤖 Prompt for AI AgentsCommittable suggestion skipped: line range outside the PR's diff.
In vm/src/stdlib/builtins.rs around lines 984 to 1002, the current code sets __parameters__ by including all TypeVar/ParamSpec/TypeVarTuple objects from __type_params__ without filtering by actual usage in method signatures. To fix this, after collecting candidate parameters from __type_params__, iterate over the class’s methods and inspect their __annotations__ to identify which parameters are actually used. Then filter the collected parameters to include only those found in these annotations before setting __parameters__. Use your VM’s API to access methods and their annotations for this filtering step.
Sorry, something went wrong.
Summary by CodeRabbit
New Features
Documentation
Chores