Conversation
📝 WalkthroughWalkthroughAdds PEP 649 conditional-annotation tracking (per-code-unit fields and enter/leave hooks), moves debug forbidden-name enforcement into the symbol-table with a varnames stack, bumps marshal format version 4→5 and adds slice type sentinel, and updates JIT tests/annotation extraction to use Wtf8 keys and forward-ref strings. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,230,255,0.5)
participant Parser
participant Compiler
participant CodeInfo
participant SymbolTable
end
Parser->>Compiler: emit AST node (If/For/With/Try/Match)
Compiler->>SymbolTable: enter_scope / enter_annotation_scope
SymbolTable->>SymbolTable: push varnames_stack, seed annotation varnames
Compiler->>CodeInfo: enter_conditional_block()
CodeInfo-->>Compiler: in_conditional_block++
Compiler->>Compiler: compile body (emit conditional annotations)
Compiler->>CodeInfo: allocate next_conditional_annotation_index when needed
Compiler->>CodeInfo: leave_conditional_block()
CodeInfo-->>Compiler: in_conditional_block--
Compiler->>SymbolTable: leave_annotation_scope / leave_scope
SymbolTable->>SymbolTable: pop varnames_stack, restore parent varnames
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
2e7dd40 to
54ba45f
Compare
54ba45f to
0dbf142
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/codegen/src/compile.rs (1)
5936-5955: Fix conditional annotation indexing for mixed class/module annotations.
next_conditional_annotation_indexonly increments whenis_conditionalis true. In class scopes that contain both unconditional and conditional annotations, this misaligns indices withcompile_module_annotate’s enumeration order, which can cause unconditional annotations to be skipped or the wrong entries to be included.Allocate an index for every simple annotation when
has_conditional_annotationsis true, and only gate insertion into__conditional_annotations__by the conditionality check. Also treat class scope like module scope for tracking purposes.🔧 Proposed fix
- let is_module = self.current_symbol_table().typ == CompilerScope::Module; - let in_conditional_block = self.current_code_info().in_conditional_block > 0; - let is_conditional = is_module || in_conditional_block; - - if is_conditional { - // Get the current annotation index and increment - let code_info = self.current_code_info(); - let annotation_index = code_info.next_conditional_annotation_index; - code_info.next_conditional_annotation_index += 1; + let scope_type = self.current_symbol_table().typ; + let in_conditional_block = self.current_code_info().in_conditional_block > 0; + let is_conditional = matches!(scope_type, CompilerScope::Module | CompilerScope::Class) + || in_conditional_block; + + // Allocate a stable index for every annotation in this scope. + let code_info = self.current_code_info(); + let annotation_index = code_info.next_conditional_annotation_index; + code_info.next_conditional_annotation_index += 1; + + if is_conditional { // Add index to __conditional_annotations__ set let cond_annotations_name = self.name("__conditional_annotations__"); emit!(self, Instruction::LoadName(cond_annotations_name)); self.emit_load_const(ConstantData::Integer { value: annotation_index.into(), }); emit!(self, Instruction::SetAdd { i: 0_u32 }); emit!(self, Instruction::PopTop); }
🤖 Fix all issues with AI agents
In `@crates/codegen/src/symboltable.rs`:
- Around line 991-995: The __debug__ binding check must be centralized so all
binding paths are validated: add a guard in register_name that rejects the
identifier "__debug__" (returning the same error check_name would) before any
registration occurs, and remove or avoid duplicate checks elsewhere; update
calls that currently call register_ident or register_name bypassing validation
(e.g., import alias handling and MatchAs capture registration) to call
register_name so imports, pattern-match captures, parameters, and comprehension
targets (including ExpressionContext::Iter) are validated; alternatively, if you
prefer not to centralize, treat ExpressionContext::Iter as
ExpressionContext::Store in the comprehension-target scanner and ensure
register_ident callers for imports and MatchAs invoke check_name first.
In `@crates/jit/tests/common.rs`:
- Around line 101-113: The current logic silently skips malformed annotation
values (when val_is_const is true but code.constants.get(val_idx) is not
ConstantData::Str, or when val_is_const is false but code.names.get(val_idx) is
None), causing later generic panics; update the branch around val_is_const /
val_idx resolution in crates/jit/tests/common.rs (the block that constructs
type_name and then calls annotations.insert(param_name,
StackValue::String(type_name))) to fail fast: when a const entry exists but is
not ConstantData::Str, or when a name index is missing, return/raise a clear
error or panic that includes param_name and val_idx (and ideally the offending
constant's debug info), rather than treating it as None, so malformed annotate
bytecode reports an explicit diagnostic.
crates/jit/tests/common.rs
Outdated
| // Value can be a name (type ref) or a const string (forward ref) | ||
| let type_name = if val_is_const { | ||
| match code.constants.get(val_idx) { | ||
| Some(ConstantData::Str { value }) => { | ||
| Some(value.to_string_lossy().into_owned()) | ||
| } | ||
| _ => None, | ||
| } | ||
| } else { | ||
| code.names.get(val_idx).cloned() | ||
| }; | ||
| if let Some(type_name) = type_name { | ||
| annotations.insert(param_name, StackValue::String(type_name)); |
There was a problem hiding this comment.
Don’t silently drop unsupported annotation values.
If a const isn’t a string (or the name index is missing), the annotation is skipped and later fails with a generic “Argument have annotation” panic. Prefer failing fast with a clear message so malformed annotate bytecode is obvious.
🔧 Proposed fix (fail fast with explicit diagnostics)
- let type_name = if val_is_const {
- match code.constants.get(val_idx) {
- Some(ConstantData::Str { value }) => {
- Some(value.to_string_lossy().into_owned())
- }
- _ => None,
- }
- } else {
- code.names.get(val_idx).cloned()
- };
- if let Some(type_name) = type_name {
- annotations.insert(param_name, StackValue::String(type_name));
- }
+ let type_name = if val_is_const {
+ match code.constants.get(val_idx) {
+ Some(ConstantData::Str { value }) => {
+ value.to_string_lossy().into_owned()
+ }
+ Some(other) => {
+ panic!("Unsupported annotation const: {:?}", other)
+ }
+ None => panic!("Annotation const idx out of range: {val_idx}"),
+ }
+ } else {
+ code.names
+ .get(val_idx)
+ .unwrap_or_else(|| {
+ panic!("Annotation name idx out of range: {val_idx}")
+ })
+ .clone()
+ };
+ annotations.insert(param_name, StackValue::String(type_name));📝 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.
| // Value can be a name (type ref) or a const string (forward ref) | |
| let type_name = if val_is_const { | |
| match code.constants.get(val_idx) { | |
| Some(ConstantData::Str { value }) => { | |
| Some(value.to_string_lossy().into_owned()) | |
| } | |
| _ => None, | |
| } | |
| } else { | |
| code.names.get(val_idx).cloned() | |
| }; | |
| if let Some(type_name) = type_name { | |
| annotations.insert(param_name, StackValue::String(type_name)); | |
| // Value can be a name (type ref) or a const string (forward ref) | |
| let type_name = if val_is_const { | |
| match code.constants.get(val_idx) { | |
| Some(ConstantData::Str { value }) => { | |
| value.to_string_lossy().into_owned() | |
| } | |
| Some(other) => { | |
| panic!("Unsupported annotation const: {:?}", other) | |
| } | |
| None => panic!("Annotation const idx out of range: {val_idx}"), | |
| } | |
| } else { | |
| code.names | |
| .get(val_idx) | |
| .unwrap_or_else(|| { | |
| panic!("Annotation name idx out of range: {val_idx}") | |
| }) | |
| .clone() | |
| }; | |
| annotations.insert(param_name, StackValue::String(type_name)); |
🤖 Prompt for AI Agents
In `@crates/jit/tests/common.rs` around lines 101 - 113, The current logic
silently skips malformed annotation values (when val_is_const is true but
code.constants.get(val_idx) is not ConstantData::Str, or when val_is_const is
false but code.names.get(val_idx) is None), causing later generic panics; update
the branch around val_is_const / val_idx resolution in
crates/jit/tests/common.rs (the block that constructs type_name and then calls
annotations.insert(param_name, StackValue::String(type_name))) to fail fast:
when a const entry exists but is not ConstantData::Str, or when a name index is
missing, return/raise a clear error or panic that includes param_name and
val_idx (and ideally the offending constant's debug info), rather than treating
it as None, so malformed annotate bytecode reports an explicit diagnostic.
0dbf142 to
a903083
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/codegen/src/compile.rs (1)
5934-5956: Class-scope unconditional annotations are omitted from__annotate__when any conditional annotation exists in the scope.Because
is_conditionalonly returnstruefor module-level annotations or when inside a conditional block, top-level class annotations are never added to__conditional_annotations__. However,compile_module_annotateuses set membership to determine which annotations to include, so an empty set results in all annotations being skipped.The suggested fix is correct: include
CompilerScope::Classin theis_conditionalcheck to ensure class-scope annotations are properly tracked.🛠️ Suggested fix for class-scope inclusion
- let is_module = self.current_symbol_table().typ == CompilerScope::Module; - let in_conditional_block = self.current_code_info().in_conditional_block > 0; - let is_conditional = is_module || in_conditional_block; + let scope_type = self.current_symbol_table().typ; + let in_conditional_block = self.current_code_info().in_conditional_block > 0; + let is_conditional = + matches!(scope_type, CompilerScope::Module | CompilerScope::Class) || in_conditional_block;A test case with a class containing both conditional and unconditional annotations would catch this regression.
♻️ Duplicate comments (2)
crates/jit/tests/common.rs (1)
94-113: Fail fast on unsupported annotation values (still silently dropped).This still skips invalid annotation values by returning
None, which leads to a later generic panic. Please fail fast with a targeted error to surface malformed annotate bytecode immediately.crates/codegen/src/symboltable.rs (1)
2181-2208: TreatExpressionContext::Iteras a binding for__debug__.
Comprehension/loop targets useIter; those should be rejected just likeStore.🛠️ Proposed fix
- match context { - ExpressionContext::Store => { + match context { + ExpressionContext::Store | ExpressionContext::Iter => { return Err(SymbolTableError { error: "cannot assign to __debug__".to_owned(), location, }); }Is "for __debug__ in iterable:" a SyntaxError in Python (i.e., is __debug__ forbidden as an iteration target)?
a903083 to
0d8a372
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/codegen/src/compile.rs (1)
5936-5959: Conditional-annotation indices can desync from__annotate__enumeration.
next_conditional_annotation_indexis incremented for every simple annotated assignment compiled (including those nested inif/for/while/try/match), butcompile_module_annotateenumerates only top-level annotations viacollect_simple_annotations. This can shift indices and cause executed annotations to be omitted from__annotate__(e.g., a conditional annotation before a top-level one).Consider aligning enumeration with compilation order by recursively collecting simple annotations from nested bodies (or, alternatively, only increment the index for annotations that
compile_module_annotatewill enumerate).🔧 Possible fix: recursively collect annotations in AST order
- fn collect_simple_annotations(body: &[Stmt]) -> Vec<(&str, &Expr)> { - let mut annotations = Vec::new(); - for stmt in body { - if let Stmt::AnnAssign(StmtAnnAssign { - target, - annotation, - simple, - .. - }) = stmt - && *simple - && let Expr::Name(ExprName { id, .. }) = target.as_ref() - { - annotations.push((id.as_str(), annotation.as_ref())); - } - } - annotations - } + fn collect_simple_annotations(body: &[Stmt]) -> Vec<(&str, &Expr)> { + fn walk<'a>(stmts: &'a [Stmt], out: &mut Vec<(&'a str, &'a Expr)>) { + for stmt in stmts { + match stmt { + Stmt::AnnAssign(StmtAnnAssign { target, annotation, simple, .. }) + if *simple && matches!(target.as_ref(), Expr::Name(_)) => + { + if let Expr::Name(ExprName { id, .. }) = target.as_ref() { + out.push((id.as_str(), annotation.as_ref())); + } + } + Stmt::If(StmtIf { body, elif_else_clauses, .. }) => { + walk(body, out); + for clause in elif_else_clauses { + walk(&clause.body, out); + } + } + Stmt::For(StmtFor { body, orelse, .. }) + | Stmt::While(StmtWhile { body, orelse, .. }) => { + walk(body, out); + walk(orelse, out); + } + Stmt::With(StmtWith { body, .. }) => walk(body, out), + Stmt::Try(StmtTry { body, handlers, orelse, finalbody, .. }) => { + walk(body, out); + for handler in handlers { + if let ExceptHandler::ExceptHandler(ExceptHandlerExceptHandler { body, .. }) = handler { + walk(body, out); + } + } + walk(orelse, out); + walk(finalbody, out); + } + Stmt::Match(StmtMatch { cases, .. }) => { + for case in cases { + walk(&case.body, out); + } + } + _ => {} + } + } + } + let mut annotations = Vec::new(); + walk(body, &mut annotations); + annotations + }
♻️ Duplicate comments (2)
crates/codegen/src/symboltable.rs (1)
2181-2225: debug can still slip through pattern-capture bindings.Now that
register_nameno longer enforces the forbidden-name rule, pattern captures (MatchAs,MatchStar, mappingrest) still callregister_identwithoutcheck_name. That allows__debug__to be bound via pattern matching, which should be rejected.Please add
check_name(..., ExpressionContext::Store, ...)before thoseregister_identcalls.🔧 Proposed fix
fn scan_pattern(&mut self, pattern: &Pattern) -> SymbolTableResult { use Pattern::*; match pattern { MatchMapping(PatternMatchMapping { keys, patterns, rest, .. }) => { self.scan_expressions(keys, ExpressionContext::Load)?; self.scan_patterns(patterns)?; if let Some(rest) = rest { + self.check_name(rest.as_str(), ExpressionContext::Store, rest.range)?; self.register_ident(rest, SymbolUsage::Assigned)?; } } MatchStar(PatternMatchStar { name, .. }) => { if let Some(name) = name { + self.check_name(name.as_str(), ExpressionContext::Store, name.range)?; self.register_ident(name, SymbolUsage::Assigned)?; } } MatchAs(PatternMatchAs { pattern, name, .. }) => { if let Some(pattern) = pattern { self.scan_pattern(pattern)?; } if let Some(name) = name { + self.check_name(name.as_str(), ExpressionContext::Store, name.range)?; self.register_ident(name, SymbolUsage::Assigned)?; } } MatchOr(PatternMatchOr { patterns, .. }) => self.scan_patterns(patterns)?, } Ok(()) }crates/jit/tests/common.rs (1)
95-144: Warn-and-skip still hides malformed annotations.The new warnings still drop malformed entries, which later causes a generic “Argument have annotation” panic. Prefer failing fast here with an explicit panic/error that includes the offending index/type.
🔧 Proposed fix
- match code.constants.get(val_idx) { - Some(ConstantData::Str { value }) => { - Some(value.as_str().map(|s| s.to_owned()).unwrap_or_else( - |_| value.to_string_lossy().into_owned(), - )) - } - Some(other) => { - eprintln!( - "Warning: Malformed annotation for '{:?}': expected string constant at index {}, got {:?}", - param_name, val_idx, other - ); - None - } - None => { - eprintln!( - "Warning: Malformed annotation for '{:?}': constant index {} out of bounds (len={})", - param_name, - val_idx, - code.constants.len() - ); - None - } - } + match code.constants.get(val_idx) { + Some(ConstantData::Str { value }) => Some( + value + .as_str() + .map(|s| s.to_owned()) + .unwrap_or_else(|_| value.to_string_lossy().into_owned()), + ), + Some(other) => panic!( + "Unsupported annotation const for '{:?}' at idx {}: {:?}", + param_name, val_idx, other + ), + None => panic!( + "Annotation const idx out of bounds for '{:?}': {} (len={})", + param_name, val_idx, code.constants.len() + ), + } } else { match code.names.get(val_idx) { - Some(name) => Some(name.clone()), - None => { - eprintln!( - "Warning: Malformed annotation for '{}': name index {} out of bounds (len={})", - param_name, - val_idx, - code.names.len() - ); - None - } + Some(name) => Some(name.clone()), + None => panic!( + "Annotation name idx out of bounds for '{:?}': {} (len={})", + param_name, val_idx, code.names.len() + ), } };
🧹 Nitpick comments (1)
crates/compiler-core/src/marshal.rs (1)
471-476: Consider a distinct error for unsupported Slice constants.Returning
BadTypehides the difference between “unknown marker” vs “known but unsupported.” A dedicated error improves diagnostics and feature‑gating.💡 Possible refactor
pub enum MarshalError { /// Unexpected End Of File Eof, /// Invalid Bytecode InvalidBytecode, /// Invalid utf8 in string InvalidUtf8, /// Invalid source location InvalidLocation, /// Bad type marker BadType, + /// Known type marker that RustPython does not yet support + UnsupportedType(u8), } impl core::fmt::Display for MarshalError { fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { match self { Self::Eof => f.write_str("unexpected end of data"), Self::InvalidBytecode => f.write_str("invalid bytecode"), Self::InvalidUtf8 => f.write_str("invalid utf8"), Self::InvalidLocation => f.write_str("invalid source location"), Self::BadType => f.write_str("bad type marker"), + Self::UnsupportedType(marker) => write!(f, "unsupported type marker: {marker:`#x`}"), } } } Type::Slice => { // Slice constants are not yet supported in RustPython // This would require adding a Slice variant to ConstantData enum // For now, return an error if we encounter a slice in marshal data - return Err(MarshalError::BadType); + return Err(MarshalError::UnsupportedType(Type::Slice as u8)); }
Apply reviews from #6718
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.