more PEP695 - #5917
Conversation
Sorry, something went wrong.
WalkthroughThe changes refactor class compilation by separating class body code generation into a new compile_class_body method and restructuring compile_class_def for clearer orchestration, especially around generic classes and type parameter handling. The generic alias creation logic in subscript_generic now dynamically calls Python's _GenericAlias instead of constructing the alias directly. Changes
Sequence Diagram(s)sequenceDiagram
participant Compiler
participant VM
participant TypingModule
Compiler->>Compiler: compile_class_def()
alt Generic class
Compiler->>Compiler: Push type param scope
Compiler->>Compiler: compile type params
Compiler->>Compiler: compile_class_body()
Compiler->>VM: subscript_generic()
VM->>TypingModule: import typing, get Generic, _GenericAlias
VM->>VM: Call _GenericAlias(Generic, type_params)
Compiler->>Compiler: Prepare and call __build_class__ with generic base
Compiler->>Compiler: Exit type param scope
else Non-generic class
Compiler->>Compiler: compile_class_body()
Compiler->>Compiler: Prepare and call __build_class__
end
Compiler->>Compiler: Apply decorators and store class name
Possibly related PRs
Poem
📜 Recent review details Configuration used: .coderabbit.yml Reviewing files that changed from the base of the PR and between 3ef0cfc and 9403919. ⛔ Files ignored due to path filters (2)
Instructions used from: Sources:
vm/src/builtins/genericalias.rs (2)✨ 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. ❤️ 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.
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 refactors the compilation process for Python's PEP 695 generic classes, introducing a new wrapper-based approach that better manages type parameters and their scope. It also includes improvements to symbol table handling for type parameters and aligns the Generic type subscripting with CPython's internal implementation, enhancing overall compatibility and correctness.
Highlights
- Enhanced PEP 695 Support: I've significantly advanced the implementation of PEP 695 (Type Parameter Syntax) for generic classes. This includes a new compilation strategy that aligns more closely with CPython's behavior.
- Wrapper Function for Generic Classes: For generic classes, a new compilation approach is introduced where the class definition is wrapped in an outer function. This wrapper handles the creation and management of type parameters, then compiles and calls the actual class body, ensuring proper scope and attribute setting (__type_params__).
- Refined Symbol Table Management: The compiler's symbol table logic has been updated to correctly identify and handle type parameters as cell variables. This ensures they are properly captured and accessible in nested scopes, which is crucial for the new wrapper-based compilation.
- Improved Generic Subscripting: The subscript_generic intrinsic function in the VM has been updated to create Generic type aliases by calling typing._GenericAlias directly, mirroring CPython's internal mechanism for better compatibility.
- LoadClassDeref Instruction Correction: A bug in the LoadClassDeref bytecode instruction has been fixed. It now correctly resolves variables by first checking cell variables and then free variables, which is essential for correctly loading __class__ and other captured variables in class scopes.
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. ↩
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review
This pull request implements support for PEP 695 generic classes. The changes involve significant additions to the compiler to handle the new syntax, including creating wrapper functions for generic classes and managing symbol tables accordingly. The implementation seems to correctly follow the logic required by PEP 695.
I've identified an area in the compiler with significant code duplication that could be refactored to improve maintainability. Other than that, the changes look good and the bug fix in frame.rs is a great catch.
Sorry, something went wrong.
| for type_param in &type_params.type_params { | ||
| match type_param { | ||
| TypeParam::TypeVar(TypeParamTypeVar { | ||
| name, | ||
| bound, | ||
| default, | ||
| .. | ||
| }) => { | ||
| if let Some(expr) = bound { | ||
| self.compile_expression(expr)?; | ||
| self.emit_load_const(ConstantData::Str { | ||
| value: name.as_str().into(), | ||
| }); | ||
| emit!(self, Instruction::TypeVarWithBound); | ||
| } else { | ||
| self.emit_load_const(ConstantData::Str { | ||
| value: name.as_str().into(), | ||
| }); | ||
| emit!(self, Instruction::TypeVar); | ||
| } | ||
|
|
||
| if let Some(default_expr) = default { | ||
| self.compile_expression(default_expr)?; | ||
| emit!( | ||
| self, | ||
| Instruction::CallIntrinsic2 { | ||
| func: bytecode::IntrinsicFunction2::SetTypeparamDefault | ||
| } | ||
| ); | ||
| } | ||
|
|
||
| // Store as cell variable | ||
| self.store_name(name.as_str())?; | ||
| } | ||
| TypeParam::ParamSpec(TypeParamParamSpec { name, default, .. }) => { | ||
| self.emit_load_const(ConstantData::Str { | ||
| value: name.as_str().into(), | ||
| }); | ||
| emit!(self, Instruction::ParamSpec); | ||
|
|
||
| if let Some(default_expr) = default { | ||
| self.compile_expression(default_expr)?; | ||
| emit!( | ||
| self, | ||
| Instruction::CallIntrinsic2 { | ||
| func: bytecode::IntrinsicFunction2::SetTypeparamDefault | ||
| } | ||
| ); | ||
| } | ||
|
|
||
| // Store as cell variable | ||
| self.store_name(name.as_str())?; | ||
| } | ||
| TypeParam::TypeVarTuple(TypeParamTypeVarTuple { name, default, .. }) => { | ||
| self.emit_load_const(ConstantData::Str { | ||
| value: name.as_str().into(), | ||
| }); | ||
| emit!(self, Instruction::TypeVarTuple); | ||
|
|
||
| if let Some(default_expr) = default { | ||
| self.compile_expression(default_expr)?; | ||
| emit!( | ||
| self, | ||
| Instruction::CallIntrinsic2 { | ||
| func: bytecode::IntrinsicFunction2::SetTypeparamDefault | ||
| } | ||
| ); | ||
| } | ||
|
|
||
| // Store as cell variable | ||
| self.store_name(name.as_str())?; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
There's significant code duplication in the for loop that compiles type parameters. The match arms for TypeParam::TypeVar, TypeParam::ParamSpec, and TypeParam::TypeVarTuple are very similar. Each arm:
- Loads the name as a string constant.
- Emits a specific instruction (TypeVar, ParamSpec, etc.).
- Handles an optional default value by compiling an expression and calling an intrinsic.
- Stores the created type parameter as a cell variable.
This repetition makes the code harder to maintain. Consider refactoring this logic to reduce duplication. For example, you could extract the common parts, like handling the default value and storing the name, into a helper function or restructure the loop to handle the differences more concisely.
A possible refactoring could look something like this:
for type_param in &type_params.type_params {
let (name, default) = match type_param {
TypeParam::TypeVar(tp) => {
if let Some(expr) = &tp.bound {
self.compile_expression(expr)?;
self.emit_load_const(ConstantData::Str { value: tp.name.as_str().into() });
emit!(self, Instruction::TypeVarWithBound);
} else {
self.emit_load_const(ConstantData::Str { value: tp.name.as_str().into() });
emit!(self, Instruction::TypeVar);
}
(tp.name.as_str(), &tp.default)
}
TypeParam::ParamSpec(tp) => {
self.emit_load_const(ConstantData::Str { value: tp.name.as_str().into() });
emit!(self, Instruction::ParamSpec);
(tp.name.as_str(), &tp.default)
}
TypeParam::TypeVarTuple(tp) => {
self.emit_load_const(ConstantData::Str { value: tp.name.as_str().into() });
emit!(self, Instruction::TypeVarTuple);
(tp.name.as_str(), &tp.default)
}
};
if let Some(default_expr) = default {
self.compile_expression(default_expr)?;
emit!(
self,
Instruction::CallIntrinsic2 {
func: bytecode::IntrinsicFunction2::SetTypeparamDefault
}
);
}
// Store as cell variable
self.store_name(name)?;
}This is just an idea, and there might be even cleaner ways to structure this.
Sorry, something went wrong.
|
✅ Actions performed
Review triggered.
|
Sorry, something went wrong.
Summary by CodeRabbit
Refactor
New Features