compile_type_param_bound_or_default #6005
Conversation
WalkthroughThis update refactors how type parameter bounds and defaults are compiled and scanned. It introduces dedicated methods and nested scopes for these expressions, ensuring type parameter bounds/defaults are processed in their own closure and symbol table context. Additionally, symbol tables now track whether type parameter scopes can access their parent class scope. Changes
Sequence Diagram(s)sequenceDiagram
participant Compiler
participant SymbolTable
participant Closure
Compiler->>SymbolTable: enter_type_param_block(name)
Note right of SymbolTable: Set can_see_class_scope, register __classdict__ if in class
Compiler->>Compiler: compile_type_param_bound_or_default(expr, name, allow_starred)
Compiler->>Closure: create closure from compiled code
Compiler->>Closure: call closure immediately
SymbolTable->>SymbolTable: leave type_param_block
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (10)
✨ 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
compiler/codegen/src/symboltable.rs (1)
1406-1420: Good refactoring to use nested scopes for type parameter expressions.The implementation correctly handles TypeVar bounds and defaults in separate scopes. The distinction between constraints (tuple expressions) and bounds is properly maintained.
Consider extracting the scope name generation into a helper method to reduce duplication:
+ fn type_param_scope_name(param_name: &str, scope_type: &str) -> String { + format!("<{} of {}>", scope_type, param_name) + }Then use it as:
- let scope_name = if binding.is_tuple_expr() { - format!("<TypeVar constraint of {}>", name.as_str()) - } else { - format!("<TypeVar bound of {}>", name.as_str()) - }; + let scope_type = if binding.is_tuple_expr() { + "TypeVar constraint" + } else { + "TypeVar bound" + }; + let scope_name = Self::type_param_scope_name(name.as_str(), scope_type);
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
compiler/codegen/src/compile.rs(5 hunks)compiler/codegen/src/symboltable.rs(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .github/copilot-instructions.md
🔇 Additional comments (11)
compiler/codegen/src/compile.rs (5)
1655-1696: LGTM! Well-structured implementation for type parameter scoping.The new method correctly implements the compilation of type parameter bounds and defaults in isolated scopes, which aligns with PEP 695 requirements. The implementation properly:
- Manages symbol table stack with
push_symbol_table()- Creates a nested
TypeParamsscope- Handles starred expressions for
TypeVarTupledefaults- Creates and immediately calls a closure to evaluate the expression
- Properly handles scope cleanup via
exit_scope()The comment on line 1687 correctly notes that
exit_scope()handles the symbol table cleanup, which is good documentation.
1710-1729: Correct application of scoped compilation for TypeVar bounds.The refactoring properly replaces direct
compile_expressioncalls with the newcompile_type_param_bound_or_defaultmethod. The logic correctly:
- Distinguishes between tuple expressions (constraints) and single bounds
- Uses appropriate scope names for debugging
- Maintains the existing intrinsic function selection logic
- Passes
allow_starred: falsewhich is correct for TypeVar bounds
1740-1742: Proper scoping for TypeVar defaults.Correctly applies the new compilation method for TypeVar default values with appropriate parameters (
allow_starred: falseis correct for TypeVar defaults).
1767-1769: Consistent application for ParamSpec defaults.Follows the same pattern as TypeVar defaults with correct parameters.
1794-1797: Correct handling of TypeVarTuple defaults with starred expressions.This usage correctly passes
allow_starred: true, which is the key distinction forTypeVarTupledefaults that can contain starred expressions (e.g.,*Ts). This is the only type parameter variant that allows this syntax.compiler/codegen/src/symboltable.rs (6)
58-59: LGTM! Well-documented field addition.The new
can_see_class_scopefield is properly documented and follows the existing struct field pattern.
74-74: LGTM! Appropriate default initialization.The field is correctly initialized to
false, which is the appropriate default for most scopes.
672-695: LGTM! Well-implemented type parameter scope handling.The method correctly:
- Detects class scope context before entering the new scope
- Sets
can_see_class_scopeflag appropriately- Registers necessary symbols (
__classdict__and.type_params) with proper usage types
1371-1389: LGTM! Clean implementation of nested scope for type parameter expressions.The method properly encapsulates the pattern of creating a nested scope for type parameter bounds and defaults, ensuring proper symbol resolution.
1428-1433: LGTM! Consistent handling of defaults for ParamSpec and TypeVarTuple.The implementation follows the same pattern as TypeVar, ensuring uniform behavior across all type parameter variants.
Also applies to: 1441-1446
778-781: LGTM! Consistent migration toenter_type_param_block.All three call sites (FunctionDef, ClassDef, and TypeAlias) have been correctly updated to use the new method while maintaining the same parameter structure.
Also applies to: 804-807, 994-997
Summary by CodeRabbit
New Features
Refactor