Skip to content

compiler enter_scope #5950

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
youknowone merged 3 commits into from
Jul 12, 2025
Merged

compiler enter_scope #5950

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b8973d6
enter_scope
youknowone Jul 11, 2025
8ade52a
drop_class_free
youknowone Jul 11, 2025
4db33c5
push_output based on enter_scope
youknowone Jul 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
push_output based on enter_scope
  • Loading branch information
@youknowone
youknowone committed Jul 12, 2025
commit 4db33c56008f705aaa698aa76af59f128bc9062e
107 changes: 21 additions & 86 deletions compiler/codegen/src/compile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,7 +513,7 @@ impl Compiler<'_> {
let code_info = ir::CodeInfo {
flags,
source_path: source_path.clone(),
private: private,
private,
blocks: vec![ir::Block::default()],
current_block: BlockIdx(0),
metadata: ir::CodeUnitMetadata {
Expand Down Expand Up @@ -585,95 +585,32 @@ impl Compiler<'_> {
kwonlyarg_count: u32,
obj_name: String,
) {
let source_path = self.source_code.path.to_owned();
let first_line_number = self.get_source_line_number();

// Get the private name from current scope if exists
let private = self.code_stack.last().and_then(|info| info.private.clone());

// First push the symbol table
let table = self.push_symbol_table();
let scope_type = table.typ;

// Build cellvars in sorted order (like CPython's dictbytype)
let mut cell_names: Vec<_> = table
.symbols
.iter()
.filter(|(_, s)| s.scope == SymbolScope::Cell)
.map(|(name, _)| name.clone())
.collect();
cell_names.sort(); // Sort for deterministic order
let mut cellvar_cache: IndexSet<String> = cell_names.into_iter().collect();
// The key is the current position in the symbol table stack
let key = self.symbol_table_stack.len() - 1;

// Handle implicit __class__ cell if needed (like CPython)
if table.needs_class_closure {
cellvar_cache.insert("__class__".to_string());
}
// Get the line number
let lineno = self.get_source_line_number().get();

// Handle implicit __classdict__ cell if needed (like CPython)
if table.needs_classdict {
cellvar_cache.insert("__classdict__".to_string());
// Call enter_scope which does most of the work
if let Err(e) = self.enter_scope(&obj_name, scope_type, key, lineno.to_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:?}");
}

// Build freevars in sorted order (like CPython's dictbytype)
let mut free_names: Vec<_> = table
.symbols
.iter()
.filter(|(_, s)| {
s.scope == SymbolScope::Free || s.flags.contains(SymbolFlags::FREE_CLASS)
})
.map(|(name, _)| name.clone())
.collect();
free_names.sort(); // Sort for deterministic order
let freevar_cache: IndexSet<String> = free_names.into_iter().collect();

// Initialize varname_cache from SymbolTable::varnames
let varname_cache: IndexSet<String> = table.varnames.iter().cloned().collect();

// Qualname will be set later by set_qualname
let qualname = None;

// Check if this is a class scope
let is_class_scope = table.typ == SymbolTableType::Class;

let info = ir::CodeInfo {
flags,
source_path,
private,
blocks: vec![ir::Block::default()],
current_block: ir::BlockIdx(0),
metadata: ir::CodeUnitMetadata {
name: obj_name,
qualname,
consts: IndexSet::default(),
names: IndexSet::default(),
varnames: varname_cache,
cellvars: cellvar_cache,
freevars: freevar_cache,
fast_hidden: IndexMap::default(),
argcount: arg_count,
posonlyargcount: posonlyarg_count,
kwonlyargcount: kwonlyarg_count,
firstlineno: first_line_number,
},
static_attributes: if is_class_scope {
Some(IndexSet::default())
} else {
None
},
in_inlined_comp: false,
fblock: Vec::with_capacity(MAXBLOCKS),
};
self.code_stack.push(info);

// We just pushed a code object, and need the qualname
self.set_qualname();

// Add RESUME instruction just like CPython's compiler_enter_scope
emit!(
self,
Instruction::Resume {
arg: bytecode::ResumeType::AtFuncStart 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;
}
}
Comment on lines +588 to +614
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Jul 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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:

  1. Change push_output to return CompileResult<()> and propagate the error to all callers
  2. Use unwrap() with a more descriptive error message using expect()
  3. 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.

Suggested change
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.


// compiler_exit_scope
Expand Down Expand Up @@ -1960,8 +1897,6 @@ impl Compiler<'_> {
.consts
.insert_full(ConstantData::None);

// RESUME instruction is already emitted in push_output

self.compile_statements(body)?;

// Emit None at end:
Expand Down
Loading