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

Show file tree
Hide file tree
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
Next Next commit
enter_scope
  • Loading branch information
@youknowone
youknowone committed Jul 12, 2025
commit b8973d6d284938f3ba534014b17bc3b1ebe2a0d2
212 changes: 194 additions & 18 deletions compiler/codegen/src/compile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ pub fn compile_program(
.map_err(|e| e.into_codegen_error(source_code.path.to_owned()))?;
let mut compiler = Compiler::new(opts, source_code, "<module>".to_owned());
compiler.compile_program(ast, symbol_table)?;
let code = compiler.pop_code_object();
let code = compiler.exit_scope();
trace!("Compilation completed: {code:?}");
Ok(code)
}
Expand All @@ -191,7 +191,7 @@ pub fn compile_program_single(
.map_err(|e| e.into_codegen_error(source_code.path.to_owned()))?;
let mut compiler = Compiler::new(opts, source_code, "<module>".to_owned());
compiler.compile_program_single(&ast.body, symbol_table)?;
let code = compiler.pop_code_object();
let code = compiler.exit_scope();
trace!("Compilation completed: {code:?}");
Ok(code)
}
Expand All @@ -205,7 +205,7 @@ pub fn compile_block_expression(
.map_err(|e| e.into_codegen_error(source_code.path.to_owned()))?;
let mut compiler = Compiler::new(opts, source_code, "<module>".to_owned());
compiler.compile_block_expr(&ast.body, symbol_table)?;
let code = compiler.pop_code_object();
let code = compiler.exit_scope();
trace!("Compilation completed: {code:?}");
Ok(code)
}
Expand All @@ -219,7 +219,7 @@ pub fn compile_expression(
.map_err(|e| e.into_codegen_error(source_code.path.to_owned()))?;
let mut compiler = Compiler::new(opts, source_code, "<module>".to_owned());
compiler.compile_eval(ast, symbol_table)?;
let code = compiler.pop_code_object();
let code = compiler.exit_scope();
Ok(code)
}

Expand Down Expand Up @@ -404,6 +404,179 @@ impl Compiler<'_> {
self.symbol_table_stack.pop().expect("compiler bug")
}

/// Enter a new scope
// = compiler_enter_scope
fn enter_scope(
&mut self,
name: &str,
scope_type: SymbolTableType,
key: usize, // In RustPython, we use the index in symbol_table_stack as key
lineno: u32,
) -> CompileResult<()> {
// Create location
let location = ruff_source_file::SourceLocation {
row: OneIndexed::new(lineno as usize).unwrap_or(OneIndexed::MIN),
column: OneIndexed::new(1).unwrap(),
};

// Allocate a new compiler unit

// In Rust, we'll create the structure directly
let source_path = self.source_code.path.to_owned();

// Lookup symbol table entry using key (_PySymtable_Lookup)
let ste = if key < self.symbol_table_stack.len() {
&self.symbol_table_stack[key]
} else {
return Err(self.error(CodegenErrorType::SyntaxError(
"unknown symbol table entry".to_owned(),
)));
};

// Use varnames from symbol table (already collected in definition order)
let varname_cache: IndexSet<String> = ste.varnames.iter().cloned().collect();

// Build cellvars using dictbytype (CELL scope, sorted)
let mut cellvar_cache = IndexSet::default();
let mut cell_names: Vec<_> = ste
.symbols
.iter()
.filter(|(_, s)| s.scope == SymbolScope::Cell)
.map(|(name, _)| name.clone())
.collect();
cell_names.sort();
for name in cell_names {
cellvar_cache.insert(name);
}

// Handle implicit __class__ cell if needed
if ste.needs_class_closure {
// Cook up an implicit __class__ cell
debug_assert_eq!(scope_type, SymbolTableType::Class);
cellvar_cache.insert("__class__".to_string());
}

// Handle implicit __classdict__ cell if needed
if ste.needs_classdict {
// Cook up an implicit __classdict__ cell
debug_assert_eq!(scope_type, SymbolTableType::Class);
cellvar_cache.insert("__classdict__".to_string());
}

// Build freevars using dictbytype (FREE scope, offset by cellvars size)
let mut freevar_cache = IndexSet::default();
let mut free_names: Vec<_> = ste
.symbols
.iter()
.filter(|(_, s)| {
s.scope == SymbolScope::Free || s.flags.contains(SymbolFlags::FREE_CLASS)
})
.map(|(name, _)| name.clone())
.collect();
free_names.sort();
for name in free_names {
freevar_cache.insert(name);
}

// Initialize u_metadata fields
let (flags, posonlyarg_count, arg_count, kwonlyarg_count) = match scope_type {
SymbolTableType::Module => (bytecode::CodeFlags::empty(), 0, 0, 0),
SymbolTableType::Class => (bytecode::CodeFlags::empty(), 0, 0, 0),
SymbolTableType::Function | SymbolTableType::Lambda => (
bytecode::CodeFlags::NEW_LOCALS | bytecode::CodeFlags::IS_OPTIMIZED,
0, // Will be set later in enter_function
0, // Will be set later in enter_function
0, // Will be set later in enter_function
),
SymbolTableType::Comprehension => (
bytecode::CodeFlags::NEW_LOCALS | bytecode::CodeFlags::IS_OPTIMIZED,
0,
1, // comprehensions take one argument (.0)
0,
),
SymbolTableType::TypeParams => (
bytecode::CodeFlags::NEW_LOCALS | bytecode::CodeFlags::IS_OPTIMIZED,
0,
0,
0,
),
};

// Get private name from parent scope
let private = if !self.code_stack.is_empty() {
self.code_stack.last().unwrap().private.clone()
} else {
None
};

// Create the new compilation unit
let code_info = ir::CodeInfo {
flags,
source_path: source_path.clone(),
private: private,
blocks: vec![ir::Block::default()],
current_block: BlockIdx(0),
metadata: ir::CodeUnitMetadata {
name: name.to_owned(),
qualname: None, // Will be set below
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: OneIndexed::new(lineno as usize).unwrap_or(OneIndexed::MIN),
},
static_attributes: if scope_type == SymbolTableType::Class {
Some(IndexSet::default())
} else {
None
},
in_inlined_comp: false,
fblock: Vec::with_capacity(MAXBLOCKS),
};

// Push the old compiler unit on the stack (like PyCapsule)
// This happens before setting qualname
self.code_stack.push(code_info);

// Set qualname after pushing (uses compiler_set_qualname logic)
if scope_type != SymbolTableType::Module {
self.set_qualname();
}

// Emit RESUME instruction
let _resume_loc = if scope_type == SymbolTableType::Module {
// Module scope starts with lineno 0
ruff_source_file::SourceLocation {
row: OneIndexed::MIN,
column: OneIndexed::MIN,
}
} else {
location
};

// Set the source range for the RESUME instruction
// For now, just use an empty range at the beginning
self.current_source_range = TextRange::default();
emit!(
self,
Instruction::Resume {
arg: bytecode::ResumeType::AtFuncStart as u32
}
);

if scope_type == SymbolTableType::Module {
// This would be loc.lineno = -1 in CPython
// We handle this differently in RustPython
}

Ok(())
}

fn push_output(
&mut self,
flags: bytecode::CodeFlags,
Expand Down Expand Up @@ -473,9 +646,18 @@ impl Compiler<'_> {
fblock: Vec::with_capacity(MAXBLOCKS),
};
self.code_stack.push(info);

// Add RESUME instruction just like CPython's compiler_enter_scope
emit!(
self,
Instruction::Resume {
arg: bytecode::ResumeType::AtFuncStart as u32
}
);
}
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.


fn pop_code_object(&mut self) -> CodeObject {
// compiler_exit_scope
fn exit_scope(&mut self) -> CodeObject {
let table = self.pop_symbol_table();
assert!(table.sub_tables.is_empty());
let pop = self.code_stack.pop();
Expand Down Expand Up @@ -755,7 +937,7 @@ impl Compiler<'_> {
}

fn mangle<'a>(&self, name: &'a str) -> Cow<'a, str> {
// Use u_private from current code unit for name mangling
// Use private from current code unit for name mangling
let private = self
.code_stack
.last()
Expand Down Expand Up @@ -1758,13 +1940,7 @@ impl Compiler<'_> {
.consts
.insert_full(ConstantData::None);

// Emit RESUME instruction at function start
emit!(
self,
Instruction::Resume {
arg: bytecode::ResumeType::AtFuncStart as u32
}
);
// RESUME instruction is already emitted in push_output

self.compile_statements(body)?;

Expand All @@ -1778,7 +1954,7 @@ impl Compiler<'_> {
}
}

let code = self.pop_code_object();
let code = self.exit_scope();
self.ctx = prev_ctx;

// Prepare generic type parameters:
Expand Down Expand Up @@ -2030,7 +2206,7 @@ impl Compiler<'_> {

self.emit_return_value();

let code = self.pop_code_object();
let code = self.exit_scope();
self.ctx = prev_ctx;

emit!(self, Instruction::LoadBuildClass);
Expand Down Expand Up @@ -3820,7 +3996,7 @@ impl Compiler<'_> {

self.compile_expression(body)?;
self.emit_return_value();
let code = self.pop_code_object();
let code = self.exit_scope();
if self.build_closure(&code) {
func_flags |= bytecode::MakeFunctionFlags::CLOSURE;
}
Expand Down Expand Up @@ -4369,7 +4545,7 @@ impl Compiler<'_> {
self.emit_return_value();

// Fetch code for listcomp function:
let code = self.pop_code_object();
let code = self.exit_scope();

self.ctx = prev_ctx;

Expand Down Expand Up @@ -5076,7 +5252,7 @@ mod tests {
.unwrap();
let mut compiler = Compiler::new(opts, source_code, "<module>".to_owned());
compiler.compile_program(&ast, symbol_table).unwrap();
compiler.pop_code_object()
compiler.exit_scope()
}

macro_rules! assert_dis_snapshot {
Expand Down
8 changes: 8 additions & 0 deletions compiler/codegen/src/symboltable.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,12 @@ pub struct SymbolTable {

/// Variable names in definition order (parameters first, then locals)
pub varnames: Vec<String>,

/// Whether this class scope needs an implicit __class__ cell
pub needs_class_closure: bool,

/// Whether this class scope needs an implicit __classdict__ cell
pub needs_classdict: bool,
}

impl SymbolTable {
Expand All @@ -60,6 +66,8 @@ impl SymbolTable {
symbols: IndexMap::default(),
sub_tables: vec![],
varnames: Vec::new(),
needs_class_closure: false,
needs_classdict: false,
}
}

Expand Down