compiler enter_scope #5950
compiler enter_scope #5950
Conversation
|
""" WalkthroughThe changes refactor how compiler scopes are managed by introducing explicit Changes
Sequence Diagram(s)sequenceDiagram
participant Compiler
participant SymbolTable
Compiler->>SymbolTable: push next symbol table
Compiler->>Compiler: enter_scope(name, type, key, lineno)
Compiler->>Compiler: ...compile code...
Compiler->>Compiler: exit_scope()
Compiler->>SymbolTable: pop symbol table
Compiler->>Compiler: finalize CodeObject
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: 1
🧹 Nitpick comments (1)
compiler/codegen/src/compile.rs (1)
407-586: Consider improving error message and refactoring for readability.The
enter_scopemethod implementation is correct and comprehensive. However:
- The error message at line 432 could be more descriptive:
- "unknown symbol table entry".to_owned(), + format!("symbol table entry not found at index {}", key),
- The method is quite long (180 lines) and handles multiple responsibilities. Consider extracting helper methods for better readability.
Would you like me to help refactor this method into smaller, more focused helper methods?
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
compiler/codegen/src/compile.rs(12 hunks)compiler/codegen/src/symboltable.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .github/copilot-instructions.md
🧠 Learnings (1)
compiler/codegen/src/symboltable.rs (1)
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T10:08:48.858Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
🧬 Code Graph Analysis (1)
compiler/codegen/src/compile.rs (3)
compiler/codegen/src/symboltable.rs (5)
enter_scope(659-669)new(60-72)new(171-178)new(636-646)default(272-274)compiler/core/src/bytecode.rs (2)
new(299-301)new(710-712)compiler/codegen/src/ir.rs (1)
default(78-83)
⏰ 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 the WASM package and demo
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Ensure compilation on various targets
🔇 Additional comments (8)
compiler/codegen/src/symboltable.rs (4)
52-56: LGTM! Well-designed fields for class closure tracking.The new boolean fields clearly indicate their purpose and follow Rust naming conventions. The documentation comments are helpful and explain what each field represents.
69-70: LGTM! Proper initialization of new fields.The fields are correctly initialized to
falsein the constructor, which is the expected default state.
239-261: LGTM! Well-implemented CPython-compatible logic.The
drop_class_freefunction correctly implements the detection of implicit class closure cells. The logic properly checks for__class__and__classdict__symbols withSymbolScope::Freeand sets the appropriate flags. The comments clearly explain the difference from CPython's approach, which is handled during code generation rather than by removing symbols from the free set.The implementation is clean and follows the documented behavior from the CPython reference.
350-353: LGTM! Proper integration into symbol analysis.The call to
drop_class_freeis correctly placed after symbol analysis and only for class scopes. This ensures that all symbols have been properly analyzed before checking for implicit class closure requirements.compiler/codegen/src/compile.rs (4)
179-179: LGTM!The renaming from
pop_code_objecttoexit_scopeprovides clearer semantics about the operation being performed.
625-631: LGTM!The rename from
pop_code_objecttoexit_scopecreates a clear and consistent enter/exit pattern with the newenter_scopemethod.
905-910: LGTM!The comment clarification accurately describes that the method uses the current code unit's private name for mangling.
194-194: LGTM!All replacements of
pop_code_object()withexit_scope()are consistent throughout the file.Also applies to: 208-208, 222-222, 1920-1920, 1962-1962, 2172-2172, 4511-4511, 5218-5218
compiler/codegen/src/compile.rs
Outdated
| fn push_output( | ||
| &mut self, | ||
| flags: bytecode::CodeFlags, | ||
| posonlyarg_count: u32, | ||
| arg_count: u32, | ||
| kwonlyarg_count: u32, | ||
| obj_name: String, | ||
| ) { | ||
| // First push the symbol table | ||
| let table = self.push_symbol_table(); | ||
| let scope_type = table.typ; | ||
|
|
||
| // The key is the current position in the symbol table stack | ||
| let key = self.symbol_table_stack.len() - 1; | ||
|
|
||
| // Get the line number | ||
| let lineno = self.get_source_line_number().get(); | ||
|
|
||
| // Call enter_scope which does most of the work | ||
| if let Err(e) = self.enter_scope(&obj_name, scope_type, key, lineno as u32) { | ||
| // In the current implementation, push_output doesn't return an error, | ||
| // so we panic here. This maintains the same behavior. | ||
| panic!("enter_scope failed: {:?}", e); | ||
| } | ||
|
|
||
| // Override the values that push_output sets explicitly | ||
| // enter_scope sets default values based on scope_type, but push_output | ||
| // allows callers to specify exact values | ||
| if let Some(info) = self.code_stack.last_mut() { | ||
| info.flags = flags; | ||
| info.metadata.argcount = arg_count; | ||
| info.metadata.posonlyargcount = posonlyarg_count; | ||
| info.metadata.kwonlyargcount = kwonlyarg_count; | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Replace panic with proper error handling.
The panic at line 610 when enter_scope fails is problematic. This will produce a poor user experience with an unhelpful error message.
Since the original push_output signature doesn't return a Result, consider one of these approaches:
- Change
push_outputto returnCompileResult<()>and propagate the error to all callers - Use
unwrap()with a more descriptive error message usingexpect() - Convert the error to a compile error and store it for later handling
The first option would be the most robust:
- fn push_output(
+ fn push_output(
&mut self,
flags: bytecode::CodeFlags,
posonlyarg_count: u32,
arg_count: u32,
kwonlyarg_count: u32,
obj_name: String,
- ) {
+ ) -> CompileResult<()> {
// First push the symbol table
let table = self.push_symbol_table();
let scope_type = table.typ;
// The key is the current position in the symbol table stack
let key = self.symbol_table_stack.len() - 1;
// Get the line number
let lineno = self.get_source_line_number().get();
// Call enter_scope which does most of the work
- if let Err(e) = self.enter_scope(&obj_name, scope_type, key, lineno as u32) {
- // In the current implementation, push_output doesn't return an error,
- // so we panic here. This maintains the same behavior.
- panic!("enter_scope failed: {:?}", e);
- }
+ self.enter_scope(&obj_name, scope_type, key, lineno as u32)?;
// Override the values that push_output sets explicitly
// enter_scope sets default values based on scope_type, but push_output
// allows callers to specify exact values
if let Some(info) = self.code_stack.last_mut() {
info.flags = flags;
info.metadata.argcount = arg_count;
info.metadata.posonlyargcount = posonlyarg_count;
info.metadata.kwonlyarg_count = kwonlyarg_count;
}
+ Ok(())
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn push_output( | |
| &mut self, | |
| flags: bytecode::CodeFlags, | |
| posonlyarg_count: u32, | |
| arg_count: u32, | |
| kwonlyarg_count: u32, | |
| obj_name: String, | |
| ) { | |
| // First push the symbol table | |
| let table = self.push_symbol_table(); | |
| let scope_type = table.typ; | |
| // The key is the current position in the symbol table stack | |
| let key = self.symbol_table_stack.len() - 1; | |
| // Get the line number | |
| let lineno = self.get_source_line_number().get(); | |
| // Call enter_scope which does most of the work | |
| if let Err(e) = self.enter_scope(&obj_name, scope_type, key, lineno as u32) { | |
| // In the current implementation, push_output doesn't return an error, | |
| // so we panic here. This maintains the same behavior. | |
| panic!("enter_scope failed: {:?}", e); | |
| } | |
| // Override the values that push_output sets explicitly | |
| // enter_scope sets default values based on scope_type, but push_output | |
| // allows callers to specify exact values | |
| if let Some(info) = self.code_stack.last_mut() { | |
| info.flags = flags; | |
| info.metadata.argcount = arg_count; | |
| info.metadata.posonlyargcount = posonlyarg_count; | |
| info.metadata.kwonlyargcount = kwonlyarg_count; | |
| } | |
| } | |
| fn push_output( | |
| &mut self, | |
| flags: bytecode::CodeFlags, | |
| posonlyarg_count: u32, | |
| arg_count: u32, | |
| kwonlyarg_count: u32, | |
| obj_name: String, | |
| ) -> CompileResult<()> { | |
| // First push the symbol table | |
| let table = self.push_symbol_table(); | |
| let scope_type = table.typ; | |
| // The key is the current position in the symbol table stack | |
| let key = self.symbol_table_stack.len() - 1; | |
| // Get the line number | |
| let lineno = self.get_source_line_number().get(); | |
| // Call enter_scope which does most of the work | |
| self.enter_scope(&obj_name, scope_type, key, lineno as u32)?; | |
| // Override the values that push_output sets explicitly | |
| // enter_scope sets default values based on scope_type, but push_output | |
| // allows callers to specify exact values | |
| if let Some(info) = self.code_stack.last_mut() { | |
| info.flags = flags; | |
| info.metadata.argcount = arg_count; | |
| info.metadata.posonlyargcount = posonlyarg_count; | |
| info.metadata.kwonlyargcount = kwonlyarg_count; | |
| } | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
In compiler/codegen/src/compile.rs around lines 588 to 622, the current panic on
enter_scope failure leads to poor error handling. Modify the push_output
function to return a CompileResult<()> instead of (). Change the call to
enter_scope to propagate the error using the ? operator, allowing the error to
bubble up to callers for proper handling. Update all callers of push_output
accordingly to handle the Result return type. This approach replaces the panic
with robust error propagation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
compiler/codegen/src/compile.rs (1)
599-603: Critical: The panic on enter_scope failure still needs to be addressed.This panic was previously identified as problematic and remains unfixed. The panic will produce a poor user experience with an unhelpful error message when
enter_scopefails.As suggested in previous reviews,
push_outputshould returnCompileResult<()>and propagate the error using the?operator instead of panicking.
🧹 Nitpick comments (1)
compiler/codegen/src/compile.rs (1)
407-578: Consider breaking down this large method into smaller, focused functions.The
enter_scopemethod is quite long (170+ lines) and handles multiple responsibilities:
- Symbol table lookup and validation
- Variable name collection and sorting
- Cell/free variable setup
- Code info initialization
- RESUME instruction emission
This makes it harder to test, understand, and maintain. Consider extracting helper methods like:
setup_variable_caches(&self, ste: &SymbolTable) -> (IndexSet<String>, IndexSet<String>, IndexSet<String>)initialize_code_flags(scope_type: SymbolTableType) -> (CodeFlags, u32, u32, u32)create_code_info(...) -> CodeInfo
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
compiler/codegen/src/compile.rs(12 hunks)compiler/codegen/src/symboltable.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- compiler/codegen/src/symboltable.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- .github/copilot-instructions.md
🧠 Learnings (1)
compiler/codegen/src/compile.rs (1)
undefined
<retrieved_learning>
Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-30T10:08:48.858Z
Learning: Applies to **/*.rs : Follow Rust best practices for error handling and memory management
</retrieved_learning>
🧬 Code Graph Analysis (1)
compiler/codegen/src/compile.rs (3)
compiler/codegen/src/symboltable.rs (5)
enter_scope(659-669)new(60-72)new(171-178)new(636-646)default(272-274)compiler/core/src/bytecode.rs (2)
new(299-301)new(710-712)compiler/codegen/src/ir.rs (1)
default(78-83)
⏰ 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). (11)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run tests under miri
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- 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)
🔇 Additional comments (4)
compiler/codegen/src/compile.rs (4)
616-623: LGTM! Clean refactoring of scope exit logic.The
exit_scopemethod is a clean replacement forpop_code_objectthat:
- Centralizes scope exit logic in one place
- Maintains consistency with existing error handling patterns
- Has a clear, focused responsibility
896-902: Good documentation improvement.The clarified comment makes it clear that name mangling uses the private name from the current code unit, improving code readability.
179-179: Consistent API migration to exit_scope.All call sites have been consistently updated from
pop_code_object()toexit_scope(), maintaining the same behavior while using the new scope management API.Also applies to: 194-194, 208-208, 222-222, 912-912, 954-954, 2164-2164, 5210-5210
407-623: Overall solid refactoring that improves scope management.This refactoring successfully:
- Centralizes scope lifecycle management in dedicated methods
- Provides clearer separation between scope entry and exit logic
- Integrates well with enhanced symbol table analysis for class scopes
- Improves error handling (except for the panic issue in
push_output)The new
enter_scope/exit_scopepattern is more explicit and maintainable than the previouspush_output/pop_code_objectapproach.
Summary by CodeRabbit
Refactor
New Features
__class__and__classdict__cells are required.Bug Fixes