Skip to content

SymbolTable::varnames, fblock #5948

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 5 commits into from
Jul 11, 2025
Merged

SymbolTable::varnames, fblock #5948

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1abe636
SymbolTable::varnames
youknowone Jul 11, 2025
db374ae
varname_cache copies it
youknowone Jul 11, 2025
2725d42
fasthidden & static attributes
youknowone Jul 11, 2025
3503b76
metadata
youknowone Jul 11, 2025
ab14160
fblock
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
fblock
  • Loading branch information
@youknowone
youknowone committed Jul 11, 2025
commit ab14160761b662c664b00f539ac3af8f9f852465
153 changes: 135 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 @@ -14,6 +14,33 @@ use crate::{
symboltable::{self, SymbolFlags, SymbolScope, SymbolTable, SymbolTableType},
unparse::unparse_expr,
};

const MAXBLOCKS: usize = 20;

#[derive(Debug, Clone, Copy)]
pub enum FBlockType {
WhileLoop,
ForLoop,
TryExcept,
FinallyTry,
FinallyEnd,
With,
AsyncWith,
HandlerCleanup,
PopValue,
ExceptionHandler,
ExceptionGroupHandler,
AsyncComprehensionGenerator,
StopIteration,
}

#[derive(Debug, Clone)]
pub struct FBlockInfo {
pub fb_type: FBlockType,
pub fb_block: BlockIdx,
pub fb_exit: BlockIdx,
// fb_datum is not needed in RustPython
}
use itertools::Itertools;
use malachite_bigint::BigInt;
use num_complex::Complex;
Expand Down Expand Up @@ -323,6 +350,7 @@ impl<'src> Compiler<'src> {
},
static_attributes: None,
in_inlined_comp: false,
fblock: Vec::with_capacity(MAXBLOCKS),
};
Compiler {
code_stack: vec![module_code],
Expand Down Expand Up @@ -442,6 +470,7 @@ impl Compiler<'_> {
None
},
in_inlined_comp: false,
fblock: Vec::with_capacity(MAXBLOCKS),
};
self.code_stack.push(info);
}
Expand All @@ -454,6 +483,37 @@ impl Compiler<'_> {
unwrap_internal(self, stack_top.finalize_code(self.opts.optimize))
}

/// Push a new fblock
// = compiler_push_fblock
fn push_fblock(
&mut self,
fb_type: FBlockType,
fb_block: BlockIdx,
fb_exit: BlockIdx,
) -> CompileResult<()> {
let code = self.current_code_info();
if code.fblock.len() >= MAXBLOCKS {
return Err(self.error(CodegenErrorType::SyntaxError(
"too many statically nested blocks".to_owned(),
)));
}
code.fblock.push(FBlockInfo {
fb_type,
fb_block,
fb_exit,
});
Ok(())
}

/// Pop an fblock
// = compiler_pop_fblock
fn pop_fblock(&mut self, _expected_type: FBlockType) -> FBlockInfo {
let code = self.current_code_info();
// TODO: Add assertion to check expected type matches
// assert!(matches!(fblock.fb_type, expected_type));
code.fblock.pop().expect("fblock stack underflow")
}
Comment on lines +509 to +515
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Jul 11, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

Complete the fblock type validation.

The pop_fblock method accepts an _expected_type parameter but doesn't validate it. This could lead to subtle bugs if blocks are popped in the wrong order.

Consider implementing the type check as indicated by the TODO:

 fn pop_fblock(&mut self, _expected_type: FBlockType) -> FBlockInfo {
     let code = self.current_code_info();
     let fblock = code.fblock.pop().expect("fblock stack underflow");
-    // TODO: Add assertion to check expected type matches
-    // assert!(matches!(fblock.fb_type, expected_type));
+    assert!(
+        matches!(fblock.fb_type, _expected_type),
+        "Expected {:?} but got {:?}",
+        _expected_type,
+        fblock.fb_type
+    );
     fblock
 }

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents 
In compiler/codegen/src/compile.rs around lines 509 to 516, the pop_fblock
method takes an _expected_type parameter but does not validate that the popped
fblock's type matches it. To fix this, implement an assertion that checks if
fblock.fb_type matches the expected_type parameter before returning fblock. This
ensures the block types are consistent and prevents subtle bugs from popping
blocks in the wrong order.


// could take impl Into<Cow<str>>, but everything is borrowed from ast structs; we never
// actually have a `String` to pass
fn name(&mut self, name: &str) -> bytecode::NameIdx {
Expand Down Expand Up @@ -1086,26 +1146,62 @@ impl Compiler<'_> {
self.switch_to_block(after_block);
}
}
Stmt::Break(_) => match self.ctx.loop_data {
Some((_, end)) => {
emit!(self, Instruction::Break { target: end });
}
None => {
return Err(
self.error_ranged(CodegenErrorType::InvalidBreak, statement.range())
);
}
},
Stmt::Continue(_) => match self.ctx.loop_data {
Some((start, _)) => {
emit!(self, Instruction::Continue { target: start });
Stmt::Break(_) => {
// Find the innermost loop in fblock stack
let found_loop = {
let code = self.current_code_info();
let mut result = None;
for i in (0..code.fblock.len()).rev() {
match code.fblock[i].fb_type {
FBlockType::WhileLoop | FBlockType::ForLoop => {
result = Some(code.fblock[i].fb_exit);
break;
}
_ => continue,
}
}
result
};

match found_loop {
Some(exit_block) => {
emit!(self, Instruction::Break { target: exit_block });
}
None => {
return Err(
self.error_ranged(CodegenErrorType::InvalidBreak, statement.range())
);
}
}
None => {
return Err(
self.error_ranged(CodegenErrorType::InvalidContinue, statement.range())
);
}
Stmt::Continue(_) => {
// Find the innermost loop in fblock stack
let found_loop = {
let code = self.current_code_info();
let mut result = None;
for i in (0..code.fblock.len()).rev() {
match code.fblock[i].fb_type {
FBlockType::WhileLoop | FBlockType::ForLoop => {
result = Some(code.fblock[i].fb_block);
break;
}
_ => continue,
}
}
result
};

match found_loop {
Some(loop_block) => {
emit!(self, Instruction::Continue { target: loop_block });
}
None => {
return Err(
self.error_ranged(CodegenErrorType::InvalidContinue, statement.range())
);
}
}
},
}
Stmt::Return(StmtReturn { value, .. }) => {
if !self.ctx.in_func() {
return Err(
Expand Down Expand Up @@ -2028,6 +2124,9 @@ impl Compiler<'_> {
emit!(self, Instruction::SetupLoop);
self.switch_to_block(while_block);

// Push fblock for while loop
self.push_fblock(FBlockType::WhileLoop, while_block, after_block)?;

self.compile_jump_if(test, false, else_block)?;

let was_in_loop = self.ctx.loop_data.replace((while_block, after_block));
Expand All @@ -2040,6 +2139,9 @@ impl Compiler<'_> {
}
);
self.switch_to_block(else_block);

// Pop fblock
self.pop_fblock(FBlockType::WhileLoop);
emit!(self, Instruction::PopBlock);
self.compile_statements(orelse)?;
self.switch_to_block(after_block);
Expand Down Expand Up @@ -2150,6 +2252,10 @@ impl Compiler<'_> {
emit!(self, Instruction::GetAIter);

self.switch_to_block(for_block);

// Push fblock for async for loop
self.push_fblock(FBlockType::ForLoop, for_block, after_block)?;

emit!(
self,
Instruction::SetupExcept {
Expand All @@ -2172,6 +2278,10 @@ impl Compiler<'_> {
emit!(self, Instruction::GetIter);

self.switch_to_block(for_block);

// Push fblock for for loop
self.push_fblock(FBlockType::ForLoop, for_block, after_block)?;

emit!(self, Instruction::ForIter { target: else_block });

// Start of loop iteration, set targets:
Expand All @@ -2184,6 +2294,10 @@ impl Compiler<'_> {
emit!(self, Instruction::Jump { target: for_block });

self.switch_to_block(else_block);

// Pop fblock
self.pop_fblock(FBlockType::ForLoop);

if is_async {
emit!(self, Instruction::EndAsyncFor);
}
Expand Down Expand Up @@ -4162,6 +4276,9 @@ impl Compiler<'_> {
// Create magnificent function <listcomp>:
self.push_output(flags, 1, 1, 0, name.to_owned());

// Mark that we're in an inlined comprehension
self.current_code_info().in_inlined_comp = true;

// Set qualname for comprehension
self.set_qualname();

Expand Down
4 changes: 4 additions & 0 deletions compiler/codegen/src/ir.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,9 @@ pub struct CodeInfo {

// True if compiling an inlined comprehension
pub in_inlined_comp: bool,

// Block stack for tracking nested control structures
pub fblock: Vec<crate::compile::FBlockInfo>,
}
impl CodeInfo {
pub fn finalize_code(mut self, optimize: u8) -> crate::InternalResult<CodeObject> {
Expand All @@ -118,6 +121,7 @@ impl CodeInfo {
metadata,
static_attributes: _,
in_inlined_comp: _,
fblock: _,
} = self;

let CodeUnitMetadata {
Expand Down
Loading