Compiler parity: docstring dedent, StopIteration wrapper, constant folding#7530
Compiler parity: docstring dedent, StopIteration wrapper, constant folding#7530youknowone merged 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCompiler and IR changes implement binary constant-folding, format-string %-to-fstring optimization, dead-code traversal via a Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Sorry, something went wrong.
19d8a3d to
15e2b2e
Compare
March 28, 2026 17:01
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/codegen/src/compile.rs`:
- Around line 8737-8740: The flag do_not_emit_bytecode only prevents emitting
instructions in _emit(), but helpers like emit_load_const call arg_constant()
(which interns constants) before _emit(), leaking metadata; modify emit helpers
(e.g., emit_load_const) and/or arg_constant() to check do_not_emit_bytecode and
skip interning when do_not_emit_bytecode > 0 so no constants/names are added
while bytecode emission is disabled, ensuring _emit, emit_load_const,
arg_constant (and any other helpers that mutate constant/name tables) return
early or no-op metadata mutations when the flag is set.
- Around line 3871-3885: The PEP 479 StopIteration wrapper currently wraps only
compile_function_body() and omits generator/coroutine code objects created in
compile_comprehension(), causing StopIteration to escape; modify
compile_comprehension() to mirror the same logic used where is_gen is computed
(create handler_block via new_block(), emit PseudoInstruction::SetupCleanup {
delta: handler_block }, and call push_fblock(FBlockType::StopIteration,
handler_block, handler_block) when the comprehension builds a
generator/coroutine) or refactor the wrapper into a small helper used by both
compile_function_body() and compile_comprehension(); also apply the same fix to
the duplicate site referenced around the other block (the section analogous to
3917-3929).
- Around line 8943-8963: The current expr_constant implementation incorrectly
returns Some(!elts.is_empty()) for ast::Expr::Tuple, which folds away element
evaluation (and side effects) for non-empty tuples; change this to only return
Some(false) for an empty tuple and for non-empty tuples return Some(true) only
if every element is a compile-time constant (call expr_constant recursively on
each element and if any returns None, return None), otherwise return None so
elements with possible side effects are preserved; update the ast::Expr::Tuple
branch in expr_constant accordingly (check elts, recurse into each elt, and only
yield Some(true) when all elts are constant).
- Around line 5217-5224: The current early-return for constant == Some(true)
skips compiling the elif/else tests, which bypasses async-await validation and
leaves nested scope sub_tables unconsumed; instead, keep the no-emission mode
but still compile each clause's test expressions (and any nested
lambdas/comprehensions) so errors and scope consumption occur. Concretely,
inside the constant == Some(true) branch where you call self.emit_nop() and
self.compile_statements(body) and adjust self.do_not_emit_bytecode, also iterate
over elif_else_clauses and call the same compiler path that validates/compiles
their test expressions (e.g. use the existing expression-compile routine used
for condition checks) with do_not_emit_bytecode incremented so no bytecode is
emitted but await-checks and sub_table consumption run; do not compile clause
bodies, only their tests/scopes. Ensure you reference self.compile_statements,
elif_else_clauses, and self.do_not_emit_bytecode when making the change.
- Around line 8992-8994: The early return when self.do_not_emit_bytecode > 0
skips loop/except* validation and lets dead branches (e.g., `if False: break`)
pass; instead, preserve the validation logic and only skip emission. Change the
code around the `if self.do_not_emit_bytecode > 0 { return Ok(()); }` guard so
that you still execute the loop/except* checks that follow (the
dead-branch/`break`/`continue` and `except*` validation code) but bypass only
the bytecode emission paths—e.g., wrap emission-specific calls with a check `if
self.do_not_emit_bytecode == 0 { ... }` or move the `return` to after the
validation block; keep references to `self.do_not_emit_bytecode`, the
loop/except* validation code paths, and emission sites to locate and update the
necessary conditionals.
In `@crates/codegen/src/ir.rs`:
- Around line 193-196: The binop-constant folding pass should run again after
UNARY_NEGATIVE has been folded into LOAD_CONST so expressions like `-1 + 2` and
`2 * -3` are caught; update the sequence around fold_binop_constants(),
fold_unary_negative(), and remove_nops() to call fold_binop_constants() once
more immediately after fold_unary_negative() (keeping the surrounding
remove_nops() calls as needed) so binary constant folding re-runs on the newly
produced constants.
- Around line 787-798: The BinOp::FloorDivide and BinOp::Remainder folding
currently uses Rust’s / and % which produce non-Python semantics for BigInt and
floats; change the BigInt cases to call malachite_bigint::BigInt methods
.div_floor() and .mod_floor() (used by the runtime) instead of l / r and l % r,
and for the float remainder branch replicate the Python floor-mod behavior by
applying the sign-correction logic from float_ops::mod_() (or the divmod helper)
so result sign follows the divisor (i.e., matches Python’s // and % semantics
for negative values).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: e2c39f50-5b6c-4884-a8a3-1bc68e3aad8f
⛔ Files ignored due to path filters (1)
crates/codegen/src/snapshots/rustpython_codegen__compile__tests__nested_double_async_with.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
crates/codegen/src/compile.rscrates/codegen/src/ir.rs
Sorry, something went wrong.
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: (module 'contextlib test_contextlib_async test_peepholer' not found) Legend:
|
Sorry, something went wrong.
There was a problem hiding this comment.
♻️ Duplicate comments (4)
crates/codegen/src/compile.rs (4)
8745-8748: ⚠️ Potential issue | 🟠 MajorStill open:
do_not_emit_bytecodeonly gates_emit().
emit_resume_for_scope()still writesResumedirectly, and helpers likearg_constant()/_name_inner()still mutate metadata before_emit()is consulted. A deaddef/classbranch can therefore leave partially formed nested code objects plus extra const/name entries in the live code object.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/codegen/src/compile.rs` around lines 8745 - 8748, The do_not_emit_bytecode flag currently only prevents bytecode writes in _emit(), but other helpers (emit_resume_for_scope, arg_constant, _name_inner, etc.) still mutate constants/names/metadata and directly write Resume, which leaves partially-formed nested code objects; update those helpers to consult the same do_not_emit_bytecode guard (or route all emission through _emit) so that emit_resume_for_scope uses _emit or checks do_not_emit_bytecode before writing Resume, and modify arg_constant() and _name_inner() to avoid mutating const/name tables when do_not_emit_bytecode > 0 (or defer metadata mutation until after the guard clears), ensuring no metadata changes occur unless _emit actually runs.
3871-3886: ⚠️ Potential issue | 🟠 MajorStill open: comprehension code objects bypass the new PEP 479 wrapper.
This only wraps
compile_function_body(). Non-inlined<genexpr>and async comprehension code objects are still built incompile_comprehension()without theSetupCleanup/FBlockType::StopIterationprologue and handler, soStopIterationcan still escape there.Also applies to: 3918-3933
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/codegen/src/compile.rs` around lines 3871 - 3886, The PEP 479 wrapper is only being applied around compile_function_body() so non-inlined comprehension code objects built in compile_comprehension() can still let StopIteration escape; update compile_comprehension() (and the same pattern around the other location noted) to create the same stop_iteration_block logic used above: detect is_gen (is_async || current_symbol_table().is_generator), emit a PseudoInstruction::SetupCleanup with the new handler_block, call set_no_location(), push_fblock(FBlockType::StopIteration, handler_block, handler_block) before compiling the comprehension body, and ensure you emit/clear the handler_block (stop_iteration_block) exactly like the compile_function_body() path so comprehensions get the same StopIteration handler.
5219-5228: ⚠️ Potential issue | 🔴 CriticalStill open: dead
eliftests are skipped entirely.Lines 5224-5227 only walk the dead clause bodies. That suppresses validation in the skipped tests and can consume
sub_tablesout of order whenever a deadeliftest contains a lambda or comprehension. Keepdo_not_emit_bytecodeenabled here, but compile each dead clause's test before its body.Minimal fix sketch
if constant == Some(true) { self.emit_nop(); self.compile_statements(body)?; self.do_not_emit_bytecode += 1; for clause in elif_else_clauses { + if let Some(test) = &clause.test { + self.compile_expression(test)?; + } self.compile_statements(&clause.body)?; } self.do_not_emit_bytecode -= 1; return Ok(()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/codegen/src/compile.rs` around lines 5219 - 5228, The current branch that handles constant true tests skips compiling the tests of dead elif/else clauses, which bypasses validation and can mis-order sub_tables; in the block where constant == Some(true) (referencing elif_else_clauses, do_not_emit_bytecode, and compile_statements), keep do_not_emit_bytecode incremented but invoke the compiler on each dead clause's test before compiling its body (e.g., call the method that compiles expressions/tests for each clause — the same routine used for live tests — while do_not_emit_bytecode is set), then compile_statements(&clause.body) as before so validation and sub_table consumption occur in the correct order.
8959-8979: ⚠️ Potential issue | 🔴 CriticalStill open: non-empty tuples are not automatically compile-time
true.
Some(!elts.is_empty())foldsif (side_effect(),):into an unconditional branch and drops element evaluation. Only()is safelyfalse; non-empty tuples should returnSome(true)only when every element is itself compile-time constant, otherwiseNone.Minimal fix sketch
- ast::Expr::Tuple(ast::ExprTuple { elts, .. }) => Some(!elts.is_empty()), + ast::Expr::Tuple(ast::ExprTuple { elts, .. }) => { + if elts.is_empty() { + Some(false) + } else if elts.iter().all(|elt| Self::expr_constant(elt).is_some()) { + Some(true) + } else { + None + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/codegen/src/compile.rs` around lines 8959 - 8979, The current expr_constant implementation treats any non-empty tuple as a compile-time true (Some(!elts.is_empty())), which can drop element evaluation and side effects; update the ast::Expr::Tuple arm in expr_constant so that: if elts.is_empty() return Some(false); otherwise iterate each element in elts and call expr_constant on it — if any element returns None then return None (preserve side effects), but if every element returns Some(_) then return Some(true). Reference: function expr_constant, match arm ast::Expr::Tuple and the elts iterator.
🧹 Nitpick comments (3)
crates/compiler-core/src/bytecode.rs (1)
140-142: Consider more idiomatic Vec initialization.Using
vec![false; num_units]is more concise and idiomatic thanVec::new()followed byresize().♻️ Suggested simplification
pub fn build_no_location_mask(linetable: &[u8], num_units: usize) -> Vec<bool> { - let mut mask = Vec::new(); - mask.resize(num_units, false); + let mut mask = vec![false; num_units]; let mut pos = 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/compiler-core/src/bytecode.rs` around lines 140 - 142, The function build_no_location_mask currently creates an empty Vec and then resizes it; replace that pattern by initializing the boolean vector idiomatically with vec![false; num_units]. Locate build_no_location_mask(linetable: &[u8], num_units: usize) and change the mask construction to use the vec![false; num_units] form so the vector is allocated and filled in one expression.crates/vm/src/frame.rs (1)
1793-1795: Extract the throw-handler restart path.The same
*self.prev_line = 0; self.run(vm)restart is repeated in all threeOk(None)branches. A tiny helper would keep this tracing-sensitive behavior from drifting across the throw/unwind paths.As per coding guidelines "When branches differ only in a value but share common logic, extract the differing value first, then call the common logic once to avoid duplicate code."
Also applies to: 1838-1840, 1909-1914
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/frame.rs` around lines 1793 - 1795, Extract the repeated restart logic used in the Ok(None) branches into a small helper (e.g., restart_after_throw or handle_throw_restart) that sets *self.prev_line = 0 and calls self.run(vm), then replace each Ok(None) branch (the ones calling *self.prev_line = 0; self.run(vm)) with a single call to that helper; update any throw/unwind paths that currently duplicate this pattern (including occurrences around the Ok(None) branches at the shown locations) so the differing value (the branch-specific context) is computed first and the common restart helper is invoked once.crates/codegen/src/ir.rs (1)
881-892: Consider pre-checking result size to avoid large allocations.The guard
n > 4096limits repetition count, but doesn't account for string length. A 1000-character string repeated 4000 times allocates ~4MB beforeconst_too_big()rejects it. Consider checking the product upfront:Suggested refinement
(ConstantData::Str { value: s }, ConstantData::Integer { value: n }) if matches!(op, BinOp::Multiply) => { let n: usize = n.try_into().ok()?; - if n > 4096 { + let result_len = s.len().checked_mul(n)?; + if result_len > 4096 { return None; } let result = s.to_string().repeat(n);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/codegen/src/ir.rs` around lines 881 - 892, In the ConstantData multiplication arm (matching ConstantData::Str { value: s } and ConstantData::Integer { value: n } when BinOp::Multiply), pre-check the resulting byte/char size before allocating: convert n safely (as currently done), compute s.len() and check s.len().checked_mul(n).filter(|&len| len <= MAX_ALLOWED_BYTES).ok_or_none()? (or return None) to avoid large allocations, then call repeat(n) only if the product is within the limit (replace the current n > 4096 guard with this product-based check); reference symbols: ConstantData::Str, ConstantData::Integer, BinOp::Multiply, and the local variable s and n.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/codegen/src/compile.rs`:
- Around line 8745-8748: The do_not_emit_bytecode flag currently only prevents
bytecode writes in _emit(), but other helpers (emit_resume_for_scope,
arg_constant, _name_inner, etc.) still mutate constants/names/metadata and
directly write Resume, which leaves partially-formed nested code objects; update
those helpers to consult the same do_not_emit_bytecode guard (or route all
emission through _emit) so that emit_resume_for_scope uses _emit or checks
do_not_emit_bytecode before writing Resume, and modify arg_constant() and
_name_inner() to avoid mutating const/name tables when do_not_emit_bytecode > 0
(or defer metadata mutation until after the guard clears), ensuring no metadata
changes occur unless _emit actually runs.
- Around line 3871-3886: The PEP 479 wrapper is only being applied around
compile_function_body() so non-inlined comprehension code objects built in
compile_comprehension() can still let StopIteration escape; update
compile_comprehension() (and the same pattern around the other location noted)
to create the same stop_iteration_block logic used above: detect is_gen
(is_async || current_symbol_table().is_generator), emit a
PseudoInstruction::SetupCleanup with the new handler_block, call
set_no_location(), push_fblock(FBlockType::StopIteration, handler_block,
handler_block) before compiling the comprehension body, and ensure you
emit/clear the handler_block (stop_iteration_block) exactly like the
compile_function_body() path so comprehensions get the same StopIteration
handler.
- Around line 5219-5228: The current branch that handles constant true tests
skips compiling the tests of dead elif/else clauses, which bypasses validation
and can mis-order sub_tables; in the block where constant == Some(true)
(referencing elif_else_clauses, do_not_emit_bytecode, and compile_statements),
keep do_not_emit_bytecode incremented but invoke the compiler on each dead
clause's test before compiling its body (e.g., call the method that compiles
expressions/tests for each clause — the same routine used for live tests — while
do_not_emit_bytecode is set), then compile_statements(&clause.body) as before so
validation and sub_table consumption occur in the correct order.
- Around line 8959-8979: The current expr_constant implementation treats any
non-empty tuple as a compile-time true (Some(!elts.is_empty())), which can drop
element evaluation and side effects; update the ast::Expr::Tuple arm in
expr_constant so that: if elts.is_empty() return Some(false); otherwise iterate
each element in elts and call expr_constant on it — if any element returns None
then return None (preserve side effects), but if every element returns Some(_)
then return Some(true). Reference: function expr_constant, match arm
ast::Expr::Tuple and the elts iterator.
---
Nitpick comments:
In `@crates/codegen/src/ir.rs`:
- Around line 881-892: In the ConstantData multiplication arm (matching
ConstantData::Str { value: s } and ConstantData::Integer { value: n } when
BinOp::Multiply), pre-check the resulting byte/char size before allocating:
convert n safely (as currently done), compute s.len() and check
s.len().checked_mul(n).filter(|&len| len <= MAX_ALLOWED_BYTES).ok_or_none()? (or
return None) to avoid large allocations, then call repeat(n) only if the product
is within the limit (replace the current n > 4096 guard with this product-based
check); reference symbols: ConstantData::Str, ConstantData::Integer,
BinOp::Multiply, and the local variable s and n.
In `@crates/compiler-core/src/bytecode.rs`:
- Around line 140-142: The function build_no_location_mask currently creates an
empty Vec and then resizes it; replace that pattern by initializing the boolean
vector idiomatically with vec![false; num_units]. Locate
build_no_location_mask(linetable: &[u8], num_units: usize) and change the mask
construction to use the vec![false; num_units] form so the vector is allocated
and filled in one expression.
In `@crates/vm/src/frame.rs`:
- Around line 1793-1795: Extract the repeated restart logic used in the Ok(None)
branches into a small helper (e.g., restart_after_throw or handle_throw_restart)
that sets *self.prev_line = 0 and calls self.run(vm), then replace each Ok(None)
branch (the ones calling *self.prev_line = 0; self.run(vm)) with a single call
to that helper; update any throw/unwind paths that currently duplicate this
pattern (including occurrences around the Ok(None) branches at the shown
locations) so the differing value (the branch-specific context) is computed
first and the common restart helper is invoked once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 8511859f-9767-4ee1-b1af-d792f7aff520
⛔ Files ignored due to path filters (2)
Lib/test/test_contextlib.pyis excluded by!Lib/**Lib/test/test_contextlib_async.pyis excluded by!Lib/**
📒 Files selected for processing (5)
crates/codegen/src/compile.rscrates/codegen/src/ir.rscrates/compiler-core/src/bytecode.rscrates/vm/src/frame.rscrates/vm/src/stdlib/sys/monitoring.rs
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (6)
crates/codegen/src/ir.rs (2)
788-799: ⚠️ Potential issue | 🔴 CriticalUse Python-compatible floor-division and modulo rules in constant folding.
These arms currently fold with raw Rust operators, so negative cases can compile to the wrong constant (
-3 // 2,-3 % 2,-1.0 % 3.0). This needs to mirror the runtime helpers rather thanl / randl % r.Run this read-only check to compare the fold path with the runtime helpers:
#!/bin/bash sed -n '788,846p' crates/codegen/src/ir.rs rg -n -C2 '\bdiv_floor\b|\bmod_floor\b' crates/vm/src/builtins/int.rs rg -n -C3 'pub fn (divmod|mod_)|floordiv' crates/common/src/float_ops.rsExpected: the runtime code uses Python-compatible floor/mod behavior, while this fold path still uses raw
/and%.Also applies to: 842-846
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/codegen/src/ir.rs` around lines 788 - 799, The constant-folding arms for BinOp::FloorDivide and BinOp::Remainder currently use Rust's raw / and % which produce incorrect results for negative operands; replace those raw operators with the same Python-compatible helpers used at runtime: call the integer helpers (div_floor and mod_floor from the int builtins) when folding integer operands, and the float helpers (floordiv and/or mod_ in crates/common/src/float_ops.rs) for floating-point operands, preserving the zero-checks and returning None on divide-by-zero; update the BinOp::FloorDivide and BinOp::Remainder match arms to dispatch to those helper functions instead of using l / r or l % r.
193-196: ⚠️ Potential issue | 🟡 MinorRun
fold_binop_constants()once more after unary folding.
-1 + 2and2 * -3still miss folding here because the binop pass runs beforeUNARY_NEGATIVEcollapses intoLOAD_CONST, and there is no second binop sweep afterward.Suggested change
self.fold_binop_constants(); self.remove_nops(); self.fold_unary_negative(); self.remove_nops(); // remove NOPs from unary folding so tuple/list/set see contiguous LOADs + self.fold_binop_constants(); + self.remove_nops(); self.fold_tuple_constants();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/codegen/src/ir.rs` around lines 193 - 196, The binop constant folding pass needs to run one more time after unary-negative folding so expressions like `-1 + 2` and `2 * -3` get folded; update the sequence in the IR pass to call fold_binop_constants() again after fold_unary_negative() (i.e., keep the existing initial fold_binop_constants(); remove_nops(); fold_unary_negative(); remove_nops(); then invoke fold_binop_constants() a second time) so the binop pass sees LOAD_CONST produced by fold_unary_negative().crates/codegen/src/compile.rs (4)
3884-3899: ⚠️ Potential issue | 🟠 MajorPEP 479 still misses comprehension-created code objects.
This wrapper only covers
compile_function_body(). Generator expressions and non-inlined async comprehensions still build their own generator/coroutine code objects incompile_comprehension(), soStopIterationcan still escape there without going throughStopIterationError.Also applies to: 3931-3946
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/codegen/src/compile.rs` around lines 3884 - 3899, The StopIteration wrapper is only applied around compile_function_body(), so generator expressions and async comprehensions created in compile_comprehension() can still leak StopIteration; update compile_comprehension() to detect generator/coroutine code objects (same condition used: is_async || self.current_symbol_table().is_generator) and wrap their body creation with the same SetupCleanup + push_fblock(FBlockType::StopIteration, handler_block, handler_block) pattern (creating handler_block via new_block(), emit PseudoInstruction::SetupCleanup with that block, call set_no_location(), and push_fblock), mirroring the stop_iteration_block logic used around compile_function_body() so StopIteration is always converted to StopIterationError for comprehension-created code objects (also apply the same change at the analogous location referenced around lines 3931-3946).
8831-8834: ⚠️ Potential issue | 🟠 MajorSuppressing
_emit()alone still leaks dead code into the code object.Dead paths still call metadata writers like
arg_constant()/_name_inner(), and direct emit sites likeemit_resume_for_scope()bypass this guard entirely. That meansco_consts,co_names, and even nested dead code objects can still change whiledo_not_emit_bytecode > 0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/codegen/src/compile.rs` around lines 8831 - 8834, Dead code suppression via do_not_emit_bytecode currently only stops _emit but still lets metadata writers and some emit sites (e.g., arg_constant(), _name_inner(), emit_resume_for_scope()) mutate co_consts/co_names and nested code objects; update those writers and bypassing sites to respect the same guard. Specifically, add an early-return check that consults do_not_emit_bytecode (or a new helper like should_emit_metadata()) at the start of arg_constant, _name_inner, emit_resume_for_scope and any other metadata-mutating helpers so they do not modify co_consts/co_names or create nested code objects when do_not_emit_bytecode > 0. Ensure the helper is used consistently across all metadata writers instead of only in _emit.
9064-9064: ⚠️ Potential issue | 🔴 CriticalDon't fold arbitrary non-empty tuples to unconditional
true.A non-empty tuple is truthy, but it is only safe to fold when every element is itself compile-time constant. As written,
if (side_effect(),):becomes unconditional and drops both the side effect and any nested-scope consumption.🔧 Minimal fix
- ast::Expr::Tuple(ast::ExprTuple { elts, .. }) => Some(!elts.is_empty()), + ast::Expr::Tuple(ast::ExprTuple { elts, .. }) => { + if elts.is_empty() { + Some(false) + } else if elts.iter().all(|elt| Self::expr_constant(elt).is_some()) { + Some(true) + } else { + None + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/codegen/src/compile.rs` at line 9064, The match arm handling ast::Expr::Tuple currently returns Some(!elts.is_empty()) which incorrectly treats any non-empty tuple as unconditionally truthy; change it to only fold to true when every element is a compile-time constant by replacing the unconditional check with a per-element constant check (e.g., elts.iter().all(|e| <existing_const_check>(e))). Update the ast::Expr::Tuple(ast::ExprTuple { elts, .. }) => branch to call the project’s const-fold/const-detect helper (such as expr_is_const or constant_foldable) on each elt and return Some(true) only if all elements are constant, otherwise return None.
5234-5241: ⚠️ Potential issue | 🔴 CriticalDead
eliftests still need to run under suppression.This skips every later clause test entirely. That hides validation like
await-outside-async and leaves nested scopes in those tests unconsumed; only the clause bodies should be skipped.🔧 Minimal fix
if constant == Some(true) { self.emit_nop(); self.compile_statements(body)?; self.do_not_emit_bytecode += 1; for clause in elif_else_clauses { + if let Some(test) = &clause.test { + self.compile_expression(test)?; + } self.compile_statements(&clause.body)?; } self.do_not_emit_bytecode -= 1; return Ok(()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/codegen/src/compile.rs` around lines 5234 - 5241, When constant == Some(true) the code currently skips compiling later clause tests entirely; instead ensure clause tests still run under bytecode suppression so validations (like await-outside-async) and scope consumption occur. Change the loop over elif_else_clauses so you increment do_not_emit_bytecode, compile each clause's test/condition (compile the test expression) and then compile the clause.body under suppression, then decrement do_not_emit_bytecode; i.e., move the suppression (do_not_emit_bytecode += 1 / -= 1) to cover both the clause test and body rather than skipping the test entirely while only skipping the body (refer to elif_else_clauses, compile_statements, and the do_not_emit_bytecode flag).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/codegen/src/compile.rs`:
- Around line 2110-2122: The NAME→FAST rewrite must be limited to when we are
compiling the inlined comprehension body; update the conditional that sets
op_type (the matches!(op_type, NameOp::Name) &&
self.current_code_info().metadata.fast_hidden.get(name.as_ref()).is_some_and(|&v|
v) branch) to also require a guard that we are currently inside the active
inlined comprehension (for example check an existing flag or counter like
self.inlining_comprehension/ self.inlined_comprehension_depth or a metadata
marker on current_code_info() such as
current_code_info().metadata.is_inlined_comprehension); i.e. only apply
NameOp::Fast when that “inlined comprehension active” predicate is true so the
fast_hidden entry on the enclosing code object does not leak to subsequent
top-level/class code.
In `@crates/codegen/src/ir.rs`:
- Around line 2245-2254: The cloned instructions t0/t1 are having their location
fields updated but not their lineno_override, so inlined LOAD_CONST/RETURN_VALUE
can retain the target block's override (e.g., -1); update the clones (load and
ret) to copy ins.lineno_override (and, if present, any related
end_lineno_override semantics) from the jump instruction before pushing into
replacements so the inlined return reports the jump site line number.
- Around line 1480-1511: The new combine_store_fast_load_fast optimization is
never run because finalize_code() doesn't call it; update finalize_code() to
invoke combine_store_fast_load_fast() (placed after optimize_load_fast_borrow()
to preserve the intended ordering so STORE_FAST + LOAD_FAST_BORROW pairs aren't
combined) so the STORE_FAST_LOAD_FAST opcode becomes live and affects emitted
bytecode.
In `@crates/codegen/src/symboltable.rs`:
- Around line 2172-2181: The comp inlining decision sets
self.tables.last_mut().unwrap().comp_inlined = true too early because it only
checks element_has_await from elt1/elt2; move all await checks into a single
pre-check before that assignment: use expr_contains_await over elt1/elt2, all
if-clauses (ifs), and every generator iterable expression (the same expressions
later scanned around the existing logic), combine them into a single boolean
(e.g., any_await_in_comprehension) and include it in the gate alongside
is_generator and has_async_gen; ensure the code references expr_contains_await,
elt1, elt2, the ifs and generator iterable expressions and only sets
comp_inlined when no await is found.
In `@scripts/dis_dump.py`:
- Around line 82-96: The current argrepr normalization mutates real string
literal reprs; update the logic around the substitutions that touch argrepr (the
two " + NULL" removals and the two re.sub calls that unescape \\u/\\U and the at
0x removal) to skip these rewrites when argrepr is itself a quoted Python
literal: detect quoted reprs with a regex like ^[rubfRUBF]*(['"])(.*)\1$ (or
similar) and if it matches, do not apply the address/NULL/unescape rewrites;
keep the existing _unescape helper and substitutions but only call them when
argrepr is not a string literal.
- Around line 294-307: The results dict in scripts/dis_dump.py uses non-unique
keys (relpath or basename) causing collisions when multiple targets are passed;
modify the keying logic around the loop that calls process_file so keys include
the input target context (e.g., prefix relpath with a unique identifier derived
from the target such as the target root name or absolute path, or use a tuple
key like (target, relpath)) and apply the same scheme for single-file branches
(replace os.path.basename(target) with the same unique-key format) so entries
from different input targets cannot overwrite each other.
---
Duplicate comments:
In `@crates/codegen/src/compile.rs`:
- Around line 3884-3899: The StopIteration wrapper is only applied around
compile_function_body(), so generator expressions and async comprehensions
created in compile_comprehension() can still leak StopIteration; update
compile_comprehension() to detect generator/coroutine code objects (same
condition used: is_async || self.current_symbol_table().is_generator) and wrap
their body creation with the same SetupCleanup +
push_fblock(FBlockType::StopIteration, handler_block, handler_block) pattern
(creating handler_block via new_block(), emit PseudoInstruction::SetupCleanup
with that block, call set_no_location(), and push_fblock), mirroring the
stop_iteration_block logic used around compile_function_body() so StopIteration
is always converted to StopIterationError for comprehension-created code objects
(also apply the same change at the analogous location referenced around lines
3931-3946).
- Around line 8831-8834: Dead code suppression via do_not_emit_bytecode
currently only stops _emit but still lets metadata writers and some emit sites
(e.g., arg_constant(), _name_inner(), emit_resume_for_scope()) mutate
co_consts/co_names and nested code objects; update those writers and bypassing
sites to respect the same guard. Specifically, add an early-return check that
consults do_not_emit_bytecode (or a new helper like should_emit_metadata()) at
the start of arg_constant, _name_inner, emit_resume_for_scope and any other
metadata-mutating helpers so they do not modify co_consts/co_names or create
nested code objects when do_not_emit_bytecode > 0. Ensure the helper is used
consistently across all metadata writers instead of only in _emit.
- Line 9064: The match arm handling ast::Expr::Tuple currently returns
Some(!elts.is_empty()) which incorrectly treats any non-empty tuple as
unconditionally truthy; change it to only fold to true when every element is a
compile-time constant by replacing the unconditional check with a per-element
constant check (e.g., elts.iter().all(|e| <existing_const_check>(e))). Update
the ast::Expr::Tuple(ast::ExprTuple { elts, .. }) => branch to call the
project’s const-fold/const-detect helper (such as expr_is_const or
constant_foldable) on each elt and return Some(true) only if all elements are
constant, otherwise return None.
- Around line 5234-5241: When constant == Some(true) the code currently skips
compiling later clause tests entirely; instead ensure clause tests still run
under bytecode suppression so validations (like await-outside-async) and scope
consumption occur. Change the loop over elif_else_clauses so you increment
do_not_emit_bytecode, compile each clause's test/condition (compile the test
expression) and then compile the clause.body under suppression, then decrement
do_not_emit_bytecode; i.e., move the suppression (do_not_emit_bytecode += 1 / -=
1) to cover both the clause test and body rather than skipping the test entirely
while only skipping the body (refer to elif_else_clauses, compile_statements,
and the do_not_emit_bytecode flag).
In `@crates/codegen/src/ir.rs`:
- Around line 788-799: The constant-folding arms for BinOp::FloorDivide and
BinOp::Remainder currently use Rust's raw / and % which produce incorrect
results for negative operands; replace those raw operators with the same
Python-compatible helpers used at runtime: call the integer helpers (div_floor
and mod_floor from the int builtins) when folding integer operands, and the
float helpers (floordiv and/or mod_ in crates/common/src/float_ops.rs) for
floating-point operands, preserving the zero-checks and returning None on
divide-by-zero; update the BinOp::FloorDivide and BinOp::Remainder match arms to
dispatch to those helper functions instead of using l / r or l % r.
- Around line 193-196: The binop constant folding pass needs to run one more
time after unary-negative folding so expressions like `-1 + 2` and `2 * -3` get
folded; update the sequence in the IR pass to call fold_binop_constants() again
after fold_unary_negative() (i.e., keep the existing initial
fold_binop_constants(); remove_nops(); fold_unary_negative(); remove_nops();
then invoke fold_binop_constants() a second time) so the binop pass sees
LOAD_CONST produced by fold_unary_negative().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 16603046-1c6b-42c3-91cf-cede05961179
⛔ Files ignored due to path filters (1)
Lib/test/test_peepholer.pyis excluded by!Lib/**
📒 Files selected for processing (5)
crates/codegen/src/compile.rscrates/codegen/src/ir.rscrates/codegen/src/symboltable.rscrates/vm/src/frame.rsscripts/dis_dump.py
✅ Files skipped from review due to trivial changes (1)
- crates/vm/src/frame.rs
Sorry, something went wrong.
d23e92b to
fc52da9
Compare
March 29, 2026 10:47
- Docstring dedent (clean_basic_doc): match indentation stripping - StopIteration wrapper for generators/coroutines (PEP 479) - Constant condition elimination: if False:/while True: optimization - do_not_emit_bytecode for dead code paths with sub_table consumption - POP_JUMP_IF_NONE/NOT_NONE for `x is None` patterns - CALL_KW threshold: nelts + nkwelts > 15 (STACK_USE_GUIDELINE/2) - Dict literal chunking: 17-pair chunks matching flowgraph.c - PEP 709: inlined comprehensions in function scopes, symboltable comp_inlined - Optimize %-format to f-string bytecode (optimize_format_str) - expr_constant: safe tuple folding, elif test walk for sub_table consumption
…malization - Binary op constant folding with Python floor-div/modulo semantics - Unary negative folding into LOAD_CONST, with binop re-run - Tuple/list/set/frozenset constant folding - STORE_FAST_LOAD_FAST combination (insert_superinstructions before optimize_load_fast) - JUMP → RETURN_VALUE inlining (normalize_jumps) - RESUME depth correction for StopIteration wrapper - DCE, unreachable block elimination, duplicate_end_returns
fc52da9 to
2e49c0a
Compare
March 29, 2026 11:51
- bytecode.rs: ConvertValue, FormatSimple, LoadFastAndClear, MakeCell instructions - frame.rs: StopIteration handling, RESUME depth support - _symtable.rs: filter inlined comprehension children - jit/instructions.rs: StoreFastLoadFast support - monitoring.rs: LINE event for non-function scopes - Remove expectedFailure markers for passing tests
2e49c0a to
702b388
Compare
March 29, 2026 12:20
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
crates/codegen/src/compile.rs (2)
5202-5288: ⚠️ Potential issue | 🟠 MajorDead branches still mutate live code-object metadata.
Walking dead
if/whilebodies underdo_not_emit_bytecodenow reuses the normal compilation paths, but_emit()is the only guard. Helpers likeemit_load_const(),_name_inner(), andmake_closure()still intern constants, names, and nested code objects before_emit()returns, so dead branches can still leak metadata into the live code object.Also applies to: 5298-5314, 8829-8832
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/codegen/src/compile.rs` around lines 5202 - 5288, Dead branches still mutate code-object metadata because helper routines (emit_load_const, _name_inner, make_closure) intern constants/names or register nested code objects even when do_not_emit_bytecode is set; change those helpers to early-return or take a no-op path when self.do_not_emit_bytecode > 0 so they do not mutate live metadata (e.g., in emit_load_const check self.do_not_emit_bytecode and skip interning/emission, in _name_inner avoid adding names to tables, and in make_closure avoid registering the nested code object), or alternatively make them append into the current sub_table only (not the live code object) when do_not_emit_bytecode is active; update compile_if/compile_while callers only if needed to ensure do_not_emit_bytecode is set before any of these helpers run.
3877-3939: ⚠️ Potential issue | 🟠 MajorWrap comprehension-generated generator/coroutine code objects too.
This only protects
compile_function_body().compile_comprehension()around Line 8323 still builds generator-expression and async-comprehension code objects without the sameSetupCleanup/StopIterationErrorepilogue, soStopIterationcan still escape there with pre-PEP-479 behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/codegen/src/compile.rs` around lines 3877 - 3939, The comprehension compiler (compile_comprehension) must apply the same PEP-479 generator/coroutine wrapping as compile_function_body: detect generator/async-comprehension, create a handler_block via self.new_block(), emit PseudoInstruction::SetupCleanup { delta: handler_block }, call self.set_no_location() and self.push_fblock(FBlockType::StopIteration, handler_block, handler_block) before compiling the comprehension body, and after compiling emit PseudoInstruction::PopBlock, self.set_no_location(), self.pop_fblock(FBlockType::StopIteration), self.switch_to_block(handler_block), then emit Instruction::CallIntrinsic1 { func: oparg::IntrinsicFunction1::StopIterationError } followed by Instruction::Reraise { depth: 1u32 } (with set_no_location() calls as in compile_function_body) so StopIteration cannot escape pre-PEP-479 in generator/async comprehensions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/codegen/src/compile.rs`:
- Around line 7484-7489: The fast-path for format-string optimization
incorrectly runs when the RHS tuple contains starred elements (e.g., "%s" %
(*xs,)), causing Starred to be compiled as a plain expression; update the match
that checks ast::Expr::Tuple in compile.rs so it only takes the fast-path when
the tuple's elements contain no Starred nodes (e.g., require elts.iter().all(|e|
!matches!(e, ast::Expr::Starred(_)))); keep the call to
try_optimize_format_str(...) and short-circuit the optimization when any starred
element is present.
In `@crates/vm/src/stdlib/_symtable.rs`:
- Around line 177-179: Filtering out items with comp_inlined currently discards
entire subtrees (hiding valid descendant symbol tables); instead, when building
the list (where you now do .filter(|t| !t.comp_inlined).map(|t|
to_py_symbol_table(t.clone()).into_pyobject(vm)).collect()), replace that
pipeline with a flat-mapping step that yields the node itself when comp_inlined
is false, but when comp_inlined is true yields its non-inlined descendants (or
recursively yields grandchildren) and then call to_py_symbol_table on those
actual nodes; in short, preserve/flatten children of nodes where comp_inlined ==
true rather than dropping the whole t so symtable.children includes nested
non-inlined scopes.
---
Duplicate comments:
In `@crates/codegen/src/compile.rs`:
- Around line 5202-5288: Dead branches still mutate code-object metadata because
helper routines (emit_load_const, _name_inner, make_closure) intern
constants/names or register nested code objects even when do_not_emit_bytecode
is set; change those helpers to early-return or take a no-op path when
self.do_not_emit_bytecode > 0 so they do not mutate live metadata (e.g., in
emit_load_const check self.do_not_emit_bytecode and skip interning/emission, in
_name_inner avoid adding names to tables, and in make_closure avoid registering
the nested code object), or alternatively make them append into the current
sub_table only (not the live code object) when do_not_emit_bytecode is active;
update compile_if/compile_while callers only if needed to ensure
do_not_emit_bytecode is set before any of these helpers run.
- Around line 3877-3939: The comprehension compiler (compile_comprehension) must
apply the same PEP-479 generator/coroutine wrapping as compile_function_body:
detect generator/async-comprehension, create a handler_block via
self.new_block(), emit PseudoInstruction::SetupCleanup { delta: handler_block },
call self.set_no_location() and self.push_fblock(FBlockType::StopIteration,
handler_block, handler_block) before compiling the comprehension body, and after
compiling emit PseudoInstruction::PopBlock, self.set_no_location(),
self.pop_fblock(FBlockType::StopIteration), self.switch_to_block(handler_block),
then emit Instruction::CallIntrinsic1 { func:
oparg::IntrinsicFunction1::StopIterationError } followed by Instruction::Reraise
{ depth: 1u32 } (with set_no_location() calls as in compile_function_body) so
StopIteration cannot escape pre-PEP-479 in generator/async comprehensions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 08d34965-5a39-4209-9d14-b94e80ab1d23
⛔ Files ignored due to path filters (4)
Lib/test/test_contextlib.pyis excluded by!Lib/**Lib/test/test_contextlib_async.pyis excluded by!Lib/**Lib/test/test_peepholer.pyis excluded by!Lib/**crates/codegen/src/snapshots/rustpython_codegen__compile__tests__nested_double_async_with.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
crates/codegen/src/compile.rscrates/codegen/src/ir.rscrates/codegen/src/symboltable.rscrates/compiler-core/src/bytecode.rscrates/jit/src/instructions.rscrates/vm/src/frame.rscrates/vm/src/stdlib/_symtable.rscrates/vm/src/stdlib/sys/monitoring.rs
✅ Files skipped from review due to trivial changes (2)
- crates/codegen/src/symboltable.rs
- crates/vm/src/stdlib/sys/monitoring.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/compiler-core/src/bytecode.rs
- crates/vm/src/frame.rs
- crates/codegen/src/ir.rs
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm/src/frame.rs`:
- Around line 9462-9464: The code creates a fresh RuntimeError via
vm.new_runtime_error and sets __cause__ from arg.downcast() but loses the
original traceback, so wrap of StopIteration will drop the originating frame;
change the construction to preserve the original exception's traceback by
extracting the traceback from the original exception (arg.downcast()) and
attaching it to the new error before returning (e.g., set the new error's
__traceback__ from the original exception's __traceback__), keeping the existing
err.set___cause__(...) logic so __cause__ and __traceback__ are both preserved;
refer to vm.new_runtime_error, err.set___cause__, arg.downcast(), and the
surrounding Reraise/Instruction::Reraise handling in ExecutingFrame::run when
making the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 011f36ad-c94a-465b-bc43-4b61ff9c9a6c
⛔ Files ignored due to path filters (3)
Lib/test/test_contextlib.pyis excluded by!Lib/**Lib/test/test_contextlib_async.pyis excluded by!Lib/**Lib/test/test_peepholer.pyis excluded by!Lib/**
📒 Files selected for processing (6)
crates/codegen/src/ir.rscrates/compiler-core/src/bytecode.rscrates/jit/src/instructions.rscrates/vm/src/frame.rscrates/vm/src/stdlib/_symtable.rscrates/vm/src/stdlib/sys/monitoring.rs
✅ Files skipped from review due to trivial changes (1)
- crates/codegen/src/ir.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/vm/src/stdlib/sys/monitoring.rs
- crates/compiler-core/src/bytecode.rs
Sorry, something went wrong.
- expr_constant: don't fold non-empty tuples (may have side effects) - %-format optimization: skip when tuple contains starred elements - _symtable children: flatten inlined comp children instead of dropping
2ebd702
into
RustPython:main
Mar 29, 2026
x is NonepatternsSummary by CodeRabbit
Bug Fixes
Performance
Improvements