Align nested code object bytecode parity#7942
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds AST-tail predicates and tuple-nonfolding, wires those predicates into finally/try/orelse/while/for/match codegen preservation and borrow decisions, introduces a per-instruction for-loop break-cleanup flag and IR guards to prevent unsafe rewrites, refines jump sizing/duplication logic, and adds tests plus stable constant normalization for disassembly. ChangesTry/except control-flow and for-break pattern handling
IR-level flags and control-flow rewrite guards
Symbol table and tooling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@crates/codegen/src/ir.rs`:
- Around line 8625-8630: The current guard uses chain_start directly but must
check the first non-empty successor before calling is_for_break_cleanup_block to
avoid false negatives when empty label blocks sit between the conditional and
the real chain body; update the condition so that instead of
is_for_break_cleanup_block(blocks, chain_start) you compute the first non-empty
block by walking successors from chain_start (skipping empty/label-only blocks)
and pass that block to is_for_break_cleanup_block (keep the other checks:
is_generic_false_path_reorder and jump_targets_for_iter(blocks, jump_block)
unchanged).
🪄 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: 03818ab4-fb70-4ef1-a405-b00f6b23538e
📒 Files selected for processing (2)
crates/codegen/src/compile.rscrates/codegen/src/ir.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/codegen/src/ir.rs (1)
8757-8760:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the first non-empty chain block when checking the for-break cleanup guard.
Line 8759 still checks
chain_startdirectly. If empty label blocks appear before the realPopTop; JUMP_*cleanup block, this guard is bypassed and the reorder can still fire.Suggested fix
- if is_generic_false_path_reorder - && jump_targets_for_iter(blocks, jump_block) - && is_for_break_cleanup_block(blocks, chain_start) + if is_generic_false_path_reorder + && jump_targets_for_iter(blocks, jump_block) + && is_for_break_cleanup_block(blocks, next_nonempty_block(blocks, chain_start)) { current = next; continue; }🤖 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/ir.rs` around lines 8757 - 8760, The check uses chain_start directly which can be an empty/label block, letting the for-break-cleanup guard be bypassed; update the condition so that instead of passing chain_start into is_for_break_cleanup_block, walk the chain from chain_start to find the first non-empty block (skip empty/label-only blocks) and pass that block into is_for_break_cleanup_block while keeping the existing jump_targets_for_iter and is_generic_false_path_reorder checks intact; update references around is_generic_false_path_reorder, jump_targets_for_iter(blocks, jump_block), and is_for_break_cleanup_block(...) to use the located non-empty chain block.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@crates/codegen/src/compile.rs`:
- Around line 879-913: The visitor in statements_contain_async_comprehension
misses generator expressions: update AsyncComprehensionVisitor::visit_expr to
also match ast::Expr::Generator(ast::ExprGenerator { generators, .. }) and check
generators.iter().any(|g| g.is_async) so async generator expressions set
found=true; this ensures the predicate used by compile_match_inner (which
controls emitting PseudoInstruction::JumpNoInterrupt) detects async genexps the
same way as ListComp/SetComp/DictComp. If omission was intentional, add a
clarifying comment near statements_contain_async_comprehension explaining why
ast::Expr::Generator is excluded.
---
Duplicate comments:
In `@crates/codegen/src/ir.rs`:
- Around line 8757-8760: The check uses chain_start directly which can be an
empty/label block, letting the for-break-cleanup guard be bypassed; update the
condition so that instead of passing chain_start into
is_for_break_cleanup_block, walk the chain from chain_start to find the first
non-empty block (skip empty/label-only blocks) and pass that block into
is_for_break_cleanup_block while keeping the existing jump_targets_for_iter and
is_generic_false_path_reorder checks intact; update references around
is_generic_false_path_reorder, jump_targets_for_iter(blocks, jump_block), and
is_for_break_cleanup_block(...) to use the located non-empty chain block.
🪄 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: bf1f7bdc-885b-4ca7-b0f4-7f4967682d0f
📒 Files selected for processing (3)
crates/codegen/src/compile.rscrates/codegen/src/ir.rscrates/codegen/src/symboltable.rs
Comment on lines
+879
to
+913
| fn statements_contain_async_comprehension(body: &[ast::Stmt]) -> bool { | ||
| use ast::visitor::Visitor; | ||
|
|
||
| #[derive(Default)] | ||
| struct AsyncComprehensionVisitor { | ||
| found: bool, | ||
| } | ||
|
|
||
| impl ast::visitor::Visitor<'_> for AsyncComprehensionVisitor { | ||
| fn visit_expr(&mut self, expr: &ast::Expr) { | ||
| if self.found { | ||
| return; | ||
| } | ||
| match expr { | ||
| ast::Expr::ListComp(ast::ExprListComp { generators, .. }) | ||
| | ast::Expr::SetComp(ast::ExprSetComp { generators, .. }) | ||
| | ast::Expr::DictComp(ast::ExprDictComp { generators, .. }) | ||
| if generators.iter().any(|generator| generator.is_async) => | ||
| { | ||
| self.found = true; | ||
| } | ||
| _ => ast::visitor::walk_expr(self, expr), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| let mut visitor = AsyncComprehensionVisitor::default(); | ||
| for stmt in body { | ||
| visitor.visit_stmt(stmt); | ||
| if visitor.found { | ||
| return true; | ||
| } | ||
| } | ||
| false | ||
| } |
Contributor
There was a problem hiding this comment.
❓ Verification inconclusive
🏁 Script executed:
#!/bin/bash
# Confirm the AST variant name for generator expressions in the ruff_python_ast
# dependency used by RustPython's codegen.
rg -nP --type=rust -C2 '\bExpr::(Generator|GeneratorExp)\b' crates/codegen
rg -nP --type=rust -C2 '\bExprGenerator(Exp)?\b' crates/codegen
# And whether `is_async` is a field of its Comprehension entries.
rg -nP --type=rust -C3 'struct (ExprGenerator|ExprGeneratorExp)\b'Repository: RustPython/RustPython
Length of output: 4170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the exact predicate definition
rg -n "fn statements_contain_async_comprehension" crates/codegen/src/compile.rs
rg -n "statements_contain_async_comprehension\\(" crates/codegen/src/compile.rs
# Inspect the function body around the reported lines
sed -n '840,940p' crates/codegen/src/compile.rs
# Find where JumpNoInterrupt is gated by async-comprehension detection
rg -n "JumpNoInterrupt" crates/codegen/src/compile.rs
rg -n "async_comprehension" crates/codegen/src/compile.rs
# Also search for other “contain_async_*” helpers that might already handle Generator expressions
rg -n "contain_async" crates/codegen/src/compile.rs
# Confirm the AST variant + fields for generator expressions in this repo’s rust bindings usage
rg -n "Expr::Generator\\(ast::ExprGenerator" crates/codegen/src/compile.rsRepository: RustPython/RustPython
Length of output: 6941
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '9090,9170p' crates/codegen/src/compile.rs
# Inspect the generator-related test mentioned in search results
sed -n '28360,28510p' crates/codegen/src/compile.rs
# Inspect the match async comprehension iterator test for context
sed -n '38810,38890p' crates/codegen/src/compile.rsRepository: RustPython/RustPython
Length of output: 11293
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "statements_contain_async_comprehension" crates/codegen/src/compile.rs
# Inspect around the one call site (~9130-9150) more tightly
sed -n '9105,9155p' crates/codegen/src/compile.rs
# Find definition/usage context of PseudoInstruction::JumpNoInterrupt
rg -n "enum PseudoInstruction|JumpNoInterrupt" crates/codegen/src/compile.rsRepository: RustPython/RustPython
Length of output: 4733
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the existing Expr::Generator match arm usage (~11248-11252 per earlier search)
sed -n '11190,11320p' crates/codegen/src/compile.rs
# Search for any other async-comprehension / async-generator detection helpers
rg -n "statements_contain_async|async_comprehension|async generator|AsyncGen" crates/codegen/src/compile.rs
# Search for other predicates that might cover Expr::Generator with async generators
rg -n "Expr::Generator\\(ast::ExprGenerator \\{[^}]*generators" crates/codegen/src/compile.rs
# Look for any existing visitor/predicate that checks generator.is_async specifically for Expr::Generator
rg -n "Expr::Generator\\(ast::ExprGenerator\\b[\\s\\S]*is_async" crates/codegen/src/compile.rsRepository: RustPython/RustPython
Length of output: 5880
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all uses/mentions of `is_async` in the codebase (should be limited).
rg -n --hidden --no-ignore-vcs "\bis_async\b" crates/codegen/src/compile.rs crates/codegen/src -S
# Check if `statements_contain_async_comprehension` is the only place controlling JumpNoInterrupt
rg -n "statements_contain_async_comprehension|body_contains_async_comprehension|JumpNoInterrupt.*end" crates/codegen/src/compile.rs
# Inspect the generator-specific code around the compile.rs match arm (~11248)
sed -n '11230,11310p' crates/codegen/src/compile.rsRepository: RustPython/RustPython
Length of output: 18681
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show more surrounding context for the match compilation block that uses body_contains_async_comprehension
sed -n '9010,9180p' crates/codegen/src/compile.rs
# Find any match-related tests mentioning genexpr or generator expressions
rg -n "test_.*match.*(genexpr|genexpr|Generator|generator expr|async genexpr|AsyncGenWrap)" crates/codegen/src/compile.rs
# Specifically search for "match" and "genexpr" in close proximity
rg -n "match[\\s\\S]{0,300}genexpr|genexpr[\\s\\S]{0,300}match" crates/codegen/src/compile.rs
# Search for expected JumpNoInterrupt behavior in tests (esp around match tests)
rg -n "JumpNoInterrupt\\s*\\{[^}]*end\\s*\\}|body_contains_async_comprehension|statements_contain_async_comprehension" crates/codegen/src/compile.rsRepository: RustPython/RustPython
Length of output: 7128
Detect async generator expressions in statements_contain_async_comprehension
statements_contain_async_comprehension only checks ast::Expr::ListComp / ast::Expr::SetComp / ast::Expr::DictComp generators for generator.is_async, but async generator expressions are represented as ast::Expr::Generator(ast::ExprGenerator { generators, .. }) and can also have generators[*].is_async: true. Since this predicate gates whether compile_match_inner emits PseudoInstruction::JumpNoInterrupt for match-arm bodies, async genexps could be missed and make match codegen diverge from CPython. Extend the visitor to include ast::Expr::Generator (or add a clarifying comment if the omission is intentional).
🤖 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/compile.rs` around lines 879 - 913, The visitor in
statements_contain_async_comprehension misses generator expressions: update
AsyncComprehensionVisitor::visit_expr to also match
ast::Expr::Generator(ast::ExprGenerator { generators, .. }) and check
generators.iter().any(|g| g.is_async) so async generator expressions set
found=true; this ensures the predicate used by compile_match_inner (which
controls emitting PseudoInstruction::JumpNoInterrupt) detects async genexps the
same way as ListComp/SetComp/DictComp. If omission was intentional, add a
clarifying comment near statements_contain_async_comprehension explaining why
ast::Expr::Generator is excluded.
a91674d to
4d19a8c
Compare
Contributor
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/pyclbr.py dependencies:
dependent tests: (1 tests)
Legend:
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
crates/codegen/src/ir.rs (1)
8796-8801:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the first non-empty chain block in this for-break guard.
chain_startis still the raw layout successor here. If there’s an empty label block before the realPopTop; JUMP_*cleanup block, this guard misses the for-break shape and the reorder still fires.Suggested fix
if is_generic_false_path_reorder && jump_targets_for_iter(blocks, jump_block) - && is_for_break_cleanup_block(blocks, chain_start) + && is_for_break_cleanup_block( + blocks, + next_nonempty_block(blocks, chain_start), + ) { current = next; continue; }🤖 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/ir.rs` around lines 8796 - 8801, The guard uses chain_start (the raw layout successor) which can be an empty label block and thus misses the for-break cleanup pattern; change the check so you first walk from chain_start to the first non-empty chain block (skip empty/label-only blocks) and pass that resolved block into is_for_break_cleanup_block(blocks, resolved_chain_start) (keeping the surrounding conditions using is_generic_false_path_reorder, jump_targets_for_iter(blocks, jump_block), and the same current/next control flow), so the for-break shape is detected even when there's an intervening empty label block.crates/codegen/src/compile.rs (1)
895-903:⚠️ Potential issue | 🟡 MinorHandle async generator expressions in this visitor too.
This still skips
ast::Expr::Generator, so a match arm body containing an async genexpr can leavebody_contains_async_comprehensionfalse and take the plainJumppath instead ofJumpNoInterrupt.Proposed fix
match expr { ast::Expr::ListComp(ast::ExprListComp { generators, .. }) | ast::Expr::SetComp(ast::ExprSetComp { generators, .. }) | ast::Expr::DictComp(ast::ExprDictComp { generators, .. }) + | ast::Expr::Generator(ast::ExprGenerator { generators, .. }) if generators.iter().any(|generator| generator.is_async) => { self.found = true; } _ => ast::visitor::walk_expr(self, expr),🤖 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/compile.rs` around lines 895 - 903, The visitor currently marks async comprehensions when matching ListComp/SetComp/DictComp but misses ast::Expr::Generator, so add a match arm for ast::Expr::Generator (or include it in the existing pattern) and set self.found = true when its generator(s) are async; update the match in compile.rs that currently checks generators.iter().any(|generator| generator.is_async) to also handle ast::Expr::Generator so body_contains_async_comprehension becomes true and the code chooses JumpNoInterrupt instead of Jump.
🤖 Prompt for all review comments with 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.
Duplicate comments:
In `@crates/codegen/src/compile.rs`:
- Around line 895-903: The visitor currently marks async comprehensions when
matching ListComp/SetComp/DictComp but misses ast::Expr::Generator, so add a
match arm for ast::Expr::Generator (or include it in the existing pattern) and
set self.found = true when its generator(s) are async; update the match in
compile.rs that currently checks generators.iter().any(|generator|
generator.is_async) to also handle ast::Expr::Generator so
body_contains_async_comprehension becomes true and the code chooses
JumpNoInterrupt instead of Jump.
In `@crates/codegen/src/ir.rs`:
- Around line 8796-8801: The guard uses chain_start (the raw layout successor)
which can be an empty label block and thus misses the for-break cleanup pattern;
change the check so you first walk from chain_start to the first non-empty chain
block (skip empty/label-only blocks) and pass that resolved block into
is_for_break_cleanup_block(blocks, resolved_chain_start) (keeping the
surrounding conditions using is_generic_false_path_reorder,
jump_targets_for_iter(blocks, jump_block), and the same current/next control
flow), so the for-break shape is detected even when there's an intervening empty
label block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 45f2ac17-9cf9-4f7a-a1be-fa0cf60bf8ea
⛔ Files ignored due to path filters (1)
Lib/test/test_pyclbr.pyis excluded by!Lib/**
📒 Files selected for processing (4)
crates/codegen/src/compile.rscrates/codegen/src/ir.rscrates/codegen/src/symboltable.rsscripts/dis_dump.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Tests
Tooling
Chores