Refactor compile_function #6001
Conversation
WalkthroughThis update refactors and modularizes the compiler's symbol table and scope management, adding helper methods for variable indexing, enhancing qualified name handling, and restructuring function and class compilation to better support type parameter scopes. It also introduces stricter checks for invalid expressions in type parameter contexts and adjusts stack operations and symbol table optimization logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Compiler
participant SymbolTable
participant CodeInfo
Compiler->>SymbolTable: push_symbol_table()
Note right of SymbolTable: Panic if no sub-tables
Compiler->>CodeInfo: enter_scope(name, scope_type, key, lineno)
Note right of CodeInfo: Set symbol_table_index
Compiler->>SymbolTable: get_var_index(name, var_type)
Compiler->>SymbolTable: pop_symbol_table()
Compiler->>CodeInfo: exit_scope()
sequenceDiagram
participant Compiler
participant SymbolTable
Compiler->>SymbolTable: scan_expression(expr)
alt Scope is TypeParams
SymbolTable-->>Compiler: Error if expr is yield/await/named
else
SymbolTable-->>Compiler: Continue scanning
end
Possibly related PRs
Poem
✨ 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)
1015-1036: LGTM! Well-implemented type parameter expression validation.The new validation logic correctly enforces Python's semantic rules for type parameter scopes. The implementation properly:
- Checks the current scope type before applying restrictions
- Uses comprehensive pattern matching for prohibited expressions
- Provides clear error messages with precise location information
- Follows Rust best practices for error handling
Consider using "assignment expression" instead of "named expression" in the error message for consistency with Python terminology:
- Expr::Named(_) => Some("named"), + Expr::Named(_) => Some("assignment"),And update the error message format string accordingly:
- "{keyword} expression cannot be used within a type parameter" + "{keyword} expression cannot be used within a type parameter"This would make the error message "assignment expression cannot be used within a type parameter" which aligns better with standard Python terminology.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
compiler/codegen/src/compile.rs(17 hunks)compiler/codegen/src/ir.rs(3 hunks)compiler/codegen/src/symboltable.rs(1 hunks)vm/src/frame.rs(1 hunks)vm/src/stdlib/symtable.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .github/copilot-instructions.md
🧠 Learnings (1)
vm/src/stdlib/symtable.rs (1)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
⏰ 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). (9)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (ubuntu-latest)
🔇 Additional comments (14)
vm/src/stdlib/symtable.rs (1)
73-76: LGTM! Proper extension of optimization to async functions.The change correctly extends optimization recognition to async functions using the idiomatic
matches!macro. This aligns well with the broader compiler enhancements for async function scope handling.compiler/codegen/src/ir.rs (2)
104-106: Good addition for symbol table tracking.The new
symbol_table_indexfield properly links code objects to their symbol tables, supporting the enhanced symbol table management mentioned in the refactoring. The field is correctly integrated into the struct and destructuring pattern.
325-328: Solid defensive programming improvement.The conditional check prevents pushing null block indices onto the stack during stack depth calculation, which could otherwise cause issues. This is a good defensive programming practice that improves robustness.
vm/src/frame.rs (1)
698-707: Add unit tests to verify theSwapinstruction index calculationWe couldn’t find any existing tests or documentation for
Instruction::Swap. The current code invm/src/frame.rs(≈ lines 698–707) computes:
- i = len – 1 (TOS index)
- index_val = index.get(arg) as usize
- j = len – index_val
But swapping TOS with the element i positions down usually maps to index
(len – 1) – i. Please:
- Add targeted unit tests for
Swap(i)(e.g., small stacks, variousivalues)- Confirm whether
j = len – index_valmatches the intended semantics or needs adjustmentcompiler/codegen/src/compile.rs (10)
388-439: LGTM! Well-structured helper methods for symbol table management.These helper methods effectively centralize variable index management and improve code maintainability.
441-462: Good defensive programming with the sub_tables check.The added validation prevents attempting to remove from an empty vector and provides a helpful error message for debugging.
602-602: Correct initialization of symbol_table_index field.The field is properly set to link the code object with its symbol table.
680-693: Appropriate removal of the sub_tables assertion.The comment clearly documents why various scope types can have remaining sub_tables, making this change correct for the enhanced type parameter support.
754-844: Excellent refactoring of qualified name generation.The enhanced logic properly handles type parameter scopes, global symbols, and function locals according to Python semantics. The implementation is well-documented and covers all necessary cases.
987-1113: Excellent unification of name operation handling.The refactored
compile_namemethod provides clear separation of concerns with theNameOpenum and comprehensive handling of all name operation scenarios, including proper type parameter scope support.
1925-2089: Well-designed modularization of function compilation logic.The three helper methods effectively separate concerns for default arguments, function body compilation, and annotation processing. This improves code organization and maintainability.
2093-2230: Comprehensive implementation of PEP 695 generic function support.The refactored method effectively orchestrates the compilation of both regular and generic functions, with proper handling of type parameters, defaults passing, and stack management.
2437-2666: Robust implementation of generic class compilation.The changes properly handle both regular and generic classes, with correct setup of class namespaces, type parameters, and the generic base mechanism required by PEP 695.
5413-5414: Better documentation reference.The updated comment more accurately describes the function's relationship to
inspect.cleandoc.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation