Skip to content

Clean up compiler parity leftovers #8174

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 1 commit into from
Jun 26, 2026
Merged

Clean up compiler parity leftovers #8174

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 0 additions & 4 deletions crates/codegen/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,6 @@ pub enum CodegenErrorType {
ConflictingNameBindPattern,
/// break/continue/return inside except* block
BreakContinueReturnInExceptStar,
NotImplementedYet, // RustPython marker for unimplemented features
}

impl core::error::Error for CodegenErrorType {}
Expand Down Expand Up @@ -173,9 +172,6 @@ impl fmt::Display for CodegenErrorType {
"'break', 'continue' and 'return' cannot appear in an except* block"
)
}
Self::NotImplementedYet => {
write!(f, "RustPython does not implement this feature yet")
}
}
}
}
295 changes: 91 additions & 204 deletions crates/codegen/src/symboltable.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,7 @@ pub struct Symbol {
pub name: String,
pub scope: SymbolScope,
pub flags: SymbolFlags,
pub location: Option<SourceLocation>,
}

impl Symbol {
Expand All @@ -334,6 +335,7 @@ impl Symbol {
// table,
scope: SymbolScope::Unknown,
flags: SymbolFlags::empty(),
location: None,
}
}

Expand Down Expand Up @@ -777,117 +779,98 @@ impl SymbolTableAnalyzer {
sub_tables: &[SymbolTable],
class_entry: Option<&SymbolMap>,
) -> SymbolTableResult {
if symbol
.flags
.contains(SymbolFlags::ASSIGNED_IN_COMPREHENSION)
&& st_typ == CompilerScope::Comprehension
{
// propagate symbol to next higher level that can hold it,
// i.e., function or module. Comprehension is skipped and
// Class is not allowed and detected as error.
self.analyze_symbol_comprehension(symbol, 0)?
} else {
match symbol.scope {
SymbolScope::Free => {
if !self.tables.as_ref().is_empty() {
let scope_depth = self.tables.as_ref().len();
// check if the name is already defined in any outer scope
if scope_depth < 2
|| self.found_in_outer_scope(
&symbol.name,
st_typ,
skip_enclosing_function_scope,
) != Some(SymbolScope::Free)
{
return Err(SymbolTableError {
error: format!("no binding for nonlocal '{}' found", symbol.name),
// TODO: accurate location info, somehow
location: None,
});
}
// Check if the nonlocal binding refers to a type parameter
if symbol.flags.contains(SymbolFlags::NONLOCAL) {
for (symbols, _typ, _skip) in self.tables.iter().rev() {
if let Some(sym) = symbols.get(&symbol.name) {
if sym.flags.contains(SymbolFlags::TYPE_PARAM) {
return Err(SymbolTableError {
error: format!(
"nonlocal binding not allowed for type parameter '{}'",
symbol.name
),
location: None,
});
}
if sym.is_bound() {
break;
}
match symbol.scope {
SymbolScope::Free => {
if !self.tables.as_ref().is_empty() {
let scope_depth = self.tables.as_ref().len();
// check if the name is already defined in any outer scope
if scope_depth < 2
|| self.found_in_outer_scope(
&symbol.name,
st_typ,
skip_enclosing_function_scope,
) != Some(SymbolScope::Free)
{
return Err(SymbolTableError {
error: format!("no binding for nonlocal '{}' found", symbol.name),
location: symbol.location,
});
}
// Check if the nonlocal binding refers to a type parameter
if symbol.flags.contains(SymbolFlags::NONLOCAL) {
for (symbols, _typ, _skip) in self.tables.iter().rev() {
if let Some(sym) = symbols.get(&symbol.name) {
if sym.flags.contains(SymbolFlags::TYPE_PARAM) {
return Err(SymbolTableError {
error: format!(
"nonlocal binding not allowed for type parameter '{}'",
symbol.name
),
location: symbol.location,
});
}
if sym.is_bound() {
break;
}
}
}
} else {
return Err(SymbolTableError {
error: format!(
"nonlocal {} defined at place without an enclosing scope",
symbol.name
),
// TODO: accurate location info, somehow
location: None,
});
}
} else {
return Err(SymbolTableError {
error: format!(
"nonlocal {} defined at place without an enclosing scope",
symbol.name
),
location: symbol.location,
});
}
SymbolScope::GlobalExplicit | SymbolScope::GlobalImplicit => {
// TODO: add more checks for globals?
}
SymbolScope::Local | SymbolScope::Cell => {
// all is well
}
SymbolScope::Unknown => {
// Try hard to figure out what the scope of this symbol is.
let scope = if symbol.is_bound() {
if symbol.flags.contains(SymbolFlags::COMP_CELL)
&& matches!(st_typ, CompilerScope::Module | CompilerScope::Class)
{
// CPython keeps comprehension-only cells in
// module/class scopes as normal local/name
// bindings and uses DEF_COMP_CELL to allocate the
// synthetic cell slot. The spliced comp child
// should not force the outer name itself to CELL.
SymbolScope::Local
} else {
self.found_in_inner_scope(sub_tables, &symbol.name, st_typ)
.unwrap_or(SymbolScope::Local)
}
} else if let Some(scope) = class_entry
.and_then(|class_symbols| class_symbols.get(&symbol.name))
.and_then(|class_sym| {
if class_sym.flags.contains(SymbolFlags::GLOBAL) {
Some(SymbolScope::GlobalExplicit)
} else if class_sym.is_bound() && class_sym.scope != SymbolScope::Free {
// If name is bound in enclosing class, use GlobalImplicit
// so it can be accessed via __classdict__
Some(SymbolScope::GlobalImplicit)
} else {
None
}
})
}
SymbolScope::GlobalExplicit | SymbolScope::GlobalImplicit => {}
SymbolScope::Local | SymbolScope::Cell => {}
SymbolScope::Unknown => {
// Try hard to figure out what the scope of this symbol is.
let scope = if symbol.is_bound() {
if symbol.flags.contains(SymbolFlags::COMP_CELL)
&& matches!(st_typ, CompilerScope::Module | CompilerScope::Class)
{
scope
} else if let Some(scope) = self.found_in_outer_scope(
&symbol.name,
st_typ,
skip_enclosing_function_scope,
) {
// If found in enclosing scope (function/TypeParams), use that
scope
} else if self.tables.is_empty() {
// Don't make assumptions when we don't know.
SymbolScope::Unknown
// CPython keeps comprehension-only cells in
// module/class scopes as normal local/name
// bindings and uses DEF_COMP_CELL to allocate the
// synthetic cell slot. The spliced comp child
// should not force the outer name itself to CELL.
SymbolScope::Local
} else {
// If there are scopes above we assume global.
SymbolScope::GlobalImplicit
};
symbol.scope = scope;
}
self.found_in_inner_scope(sub_tables, &symbol.name, st_typ)
.unwrap_or(SymbolScope::Local)
}
} else if let Some(scope) = class_entry
.and_then(|class_symbols| class_symbols.get(&symbol.name))
.and_then(|class_sym| {
if class_sym.flags.contains(SymbolFlags::GLOBAL) {
Some(SymbolScope::GlobalExplicit)
} else if class_sym.is_bound() && class_sym.scope != SymbolScope::Free {
// If name is bound in enclosing class, use GlobalImplicit
// so it can be accessed via __classdict__
Some(SymbolScope::GlobalImplicit)
} else {
None
}
})
{
scope
} else if let Some(scope) =
self.found_in_outer_scope(&symbol.name, st_typ, skip_enclosing_function_scope)
{
// If found in enclosing scope (function/TypeParams), use that
scope
} else if self.tables.is_empty() {
// Don't make assumptions when we don't know.
SymbolScope::Unknown
} else {
// If there are scopes above we assume global.
SymbolScope::GlobalImplicit
};
symbol.scope = scope;
}
}
Ok(())
Expand Down Expand Up @@ -1023,106 +1006,6 @@ impl SymbolTableAnalyzer {
}
})
}

// Implements the symbol analysis and scope extension for names
// assigned by a named expression in a comprehension. See:
// https://github.com/python/cpython/blob/7b78e7f9fd77bb3280ee39fb74b86772a7d46a70/Python/symtable.c#L1435
fn analyze_symbol_comprehension(
&mut self,
symbol: &mut Symbol,
parent_offset: usize,
) -> SymbolTableResult {
// when this is called, we expect to be in the direct parent scope of the scope that contains 'symbol'
let last = self.tables.iter_mut().rev().nth(parent_offset).unwrap();
let symbols = &mut last.0;
let table_type = last.1;

// it is not allowed to use an iterator variable as assignee in a named expression
if symbol.flags.contains(SymbolFlags::ITER) {
return Err(SymbolTableError {
error: format!(
"assignment expression cannot rebind comprehension iteration variable {}",
symbol.name
),
// TODO: accurate location info, somehow
location: None,
});
}

match table_type {
CompilerScope::Module => {
symbol.scope = SymbolScope::GlobalImplicit;
}
CompilerScope::Class => {
// named expressions are forbidden in comprehensions on class scope
return Err(SymbolTableError {
error: "assignment expression within a comprehension cannot be used in a class body".to_string(),
// TODO: accurate location info, somehow
location: None,
});
}
CompilerScope::Function | CompilerScope::AsyncFunction | CompilerScope::Lambda => {
if let Some(parent_symbol) = symbols.get_mut(&symbol.name) {
if let SymbolScope::Unknown = parent_symbol.scope {
// this information is new, as the assignment is done in inner scope
parent_symbol.flags.insert(SymbolFlags::ASSIGNED);
}

symbol.scope = if parent_symbol.is_global() {
parent_symbol.scope
} else {
SymbolScope::Free
};
} else {
let mut cloned_sym = symbol.clone();
cloned_sym.scope = SymbolScope::Cell;
last.0.insert(cloned_sym.name.to_owned(), cloned_sym);
}
}
CompilerScope::Comprehension => {
// TODO check for conflicts - requires more context information about variables
match symbols.get_mut(&symbol.name) {
Some(parent_symbol) => {
// check if assignee is an iterator in top scope
if parent_symbol.flags.contains(SymbolFlags::ITER) {
return Err(SymbolTableError {
error: format!(
"assignment expression cannot rebind comprehension iteration variable {}",
symbol.name
),
location: None,
});
}

// we synthesize the assignment to the symbol from inner scope
parent_symbol.flags.insert(SymbolFlags::ASSIGNED); // more checks are required
}
None => {
// extend the scope of the inner symbol
// as we are in a nested comprehension, we expect that the symbol is needed
// outside, too, and set it therefore to non-local scope. I.e., we expect to
// find a definition on a higher level
let mut cloned_sym = symbol.clone();
cloned_sym.scope = SymbolScope::Free;
last.0.insert(cloned_sym.name.to_owned(), cloned_sym);
}
}

self.analyze_symbol_comprehension(symbol, parent_offset + 1)?;
}
CompilerScope::TypeParams => {
// Named expression in comprehension cannot be used in type params
return Err(SymbolTableError {
error: "assignment expression within a comprehension cannot be used within the definition of a generic".to_string(),
location: None,
});
}
CompilerScope::Annotation | CompilerScope::TypeAlias | CompilerScope::TypeVariable => {
self.analyze_symbol_comprehension(symbol, parent_offset + 1)?;
}
}
Ok(())
}
}

#[derive(Clone, Copy, Debug)]
Expand Down Expand Up @@ -3475,6 +3358,10 @@ impl SymbolTableBuilder {
table.symbols.entry(name.into_owned()).or_insert(symbol)
};

if matches!(role, SymbolUsage::Global | SymbolUsage::Nonlocal) {
symbol.location = location;
}
Comment on lines +3361 to +3363

@coderabbitai coderabbitai Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Store the per-name range for global/nonlocal declarations.

Line 3361 now persists the passed location, but the Stmt::Global/Stmt::Nonlocal callers pass statement.range() for every declared name. For nonlocal a, b, an error for b will point at the statement start instead of the offending identifier. Prefer calling register_ident(name, ...) or passing name.range.

Suggested fix 
- self.register_name(name.as_str(), SymbolUsage::Global, statement.range())?;
+ self.register_ident(name, SymbolUsage::Global)?;

- self.register_name(
-     name.as_str(),
-     SymbolUsage::Nonlocal,
-     statement.range(),
- )?;
+ self.register_ident(name, SymbolUsage::Nonlocal)?;
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/codegen/src/symboltable.rs` around lines 3361 - 3363, The
global/nonlocal handling in symbol registration is using the statement-wide
range instead of each identifier’s own range, so errors for later names point to
the wrong place. Update the `Stmt::Global` and `Stmt::Nonlocal` call sites in
`symboltable.rs` to pass each name’s range (or use `register_ident(name, ...)`
for per-name registration) rather than `statement.range()`. Keep the change
aligned with the `register_ident`/`symbol.location` flow so each declared name
gets its own location.


// Set proper scope and flags on symbol:
let flags = &mut symbol.flags;
match role {
Expand Down
3 changes: 2 additions & 1 deletion crates/vm/src/stdlib/_ast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2438,6 +2438,7 @@ pub(crate) fn compile(
}
};
opts.future_features |= codegen::preprocess::future_features(&ast);
let source = text.clone();
let source_file = SourceFileBuilder::new(filename, text).finish();
#[cfg(feature = "parser")]
let code = {
Expand Down Expand Up @@ -2491,7 +2492,7 @@ pub(crate) fn compile(
};
#[cfg(not(feature = "parser"))]
let code = codegen::compile::compile_top(ast, source_file, mode, opts);
let code = code.map_err(|err| vm.new_syntax_error(&err.into(), None))?; // FIXME source
let code = code.map_err(|err| vm.new_syntax_error(&err.into(), Some(source.as_str())))?;
Ok(crate::builtins::PyCode::new_ref_from_bytecode(vm, code).into())
}

Expand Down
Loading