Repository navigation
Align nested code object bytecode parity - #7942
Merged
youknowone merged 9 commits intoMay 21, 2026
Merged
youknowone merged 9 commits into
youknowone merged 9 commits into
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
📝 Walkthrough
WalkthroughThis 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)
✏️ 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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
youknowone
marked this pull request as ready for review
May 21, 2026 00:37
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agentsVerify 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).
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
📥 CommitsReviewing files that changed from the base of the PR and between 948924a and b92d314.
📒 Files selected for processing (2)- crates/codegen/src/compile.rs
- crates/codegen/src/ir.rs
Sorry, something went wrong.
Comment thread
crates/codegen/src/ir.rs
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)crates/codegen/src/ir.rs (1)🤖 Prompt for all review comments with AI agents8757-8760: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Use the first non-empty chain block when checking the for-break cleanup guard.
Line 8759 still checks chain_start directly. If empty label blocks appear before the real PopTop; JUMP_* cleanup block, this guard is bypassed and the reorder can still fire.
Suggested fix🤖 Prompt for AI Agents- 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; }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.
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.
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
📥 CommitsReviewing files that changed from the base of the PR and between b92d314 and a91674d.
📒 Files selected for processing (3)- crates/codegen/src/compile.rs
- crates/codegen/src/ir.rs
- crates/codegen/src/symboltable.rs
Sorry, something went wrong.
Comment thread
crates/codegen/src/compile.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 | ||
| } |
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
❓ 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 AgentsVerify 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.
Sorry, something went wrong.
youknowone
force-pushed
the
bytecode-parity
branch
from
a91674d to
4d19a8c
Compare
May 21, 2026 16:43
Copy link
Copy Markdown
Contributor
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/pyclbr.py dependencies:
dependent tests: (1 tests)
Legend:
|
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
ℹ️ Review info ⚙️ Run configuration
crates/codegen/src/ir.rs (1)🤖 Prompt for all review comments with AI agentscrates/codegen/src/compile.rs (1)8796-8801: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Use the first non-empty chain block in this for-break guard.
chain_start is still the raw layout successor here. If there’s an empty label block before the real PopTop; JUMP_* cleanup block, this guard misses the for-break shape and the reorder still fires.
Suggested fix🤖 Prompt for AI Agentsif 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; }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.895-903: ⚠️ Potential issue | 🟡 Minor
Handle async generator expressions in this visitor too.
This still skips ast::Expr::Generator, so a match arm body containing an async genexpr can leave body_contains_async_comprehension false and take the plain Jump path instead of JumpNoInterrupt.
Proposed fix🤖 Prompt for AI Agentsmatch 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),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.
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
📥 CommitsReviewing files that changed from the base of the PR and between a91674d and 4d19a8c.
⛔ Files ignored due to path filters (1)- Lib/test/test_pyclbr.py is excluded by !Lib/**
- crates/codegen/src/compile.rs
- crates/codegen/src/ir.rs
- crates/codegen/src/symboltable.rs
- scripts/dis_dump.py
Sorry, something went wrong.
This was referenced Jun 20, 2026
Merged
2 tasks
youknowone
added a commit
that referenced
this pull request
Sep 16, 2026
* Align nested code object bytecode parity * Align CPython jump-back parity * Align match guard block parity with CPython * Align CPython block borrow parity * Align CPython load-fast barrier parity * Align CPython try-end label parity * Align CPython try-except end label barriers * Align more CPython bytecode edge cases * Align bare except finally bytecode layout
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
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