Update types.py + more PEP695 #5917
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
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
GenericSubscripting: Thesubscript_genericintrinsic function in the VM has been updated to createGenerictype aliases by callingtyping._GenericAliasdirectly, mirroring CPython's internal mechanism for better compatibility. LoadClassDerefInstruction Correction: A bug in theLoadClassDerefbytecode 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.
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 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.
| 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
defaultvalue 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.
No description provided.