compile_type_param_bound_or_default - #6005
Conversation
Sorry, something went wrong.
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 details Configuration used: .coderabbit.yml Reviewing files that changed from the base of the PR and between 545f915 and c3b56bf. 📒 Files selected for processing (2)
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: 0
🧹 Nitpick comments (1)compiler/codegen/src/symboltable.rs (1)📜 Review details1406-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);
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Reviewing files that changed from the base of the PR and between ff35dcd and 545f915.
📒 Files selected for processing (2)- compiler/codegen/src/compile.rs (5 hunks)
- compiler/codegen/src/symboltable.rs (8 hunks)
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .github/copilot-instructions.md
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 TypeParams scope
- Handles starred expressions for TypeVarTuple defaults
- 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_expression calls with the new compile_type_param_bound_or_default method. 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: false which 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: false is 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 for TypeVarTuple defaults 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_scope field 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_scope flag 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 to enter_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
Sorry, something went wrong.
Summary by CodeRabbit
New Features
Refactor