Merged
Conversation
Contributor
|
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 adds builtin-generator-call optimizations for tuple/list/set/all/any with a single generator, tightens constant-folding (tuples, ints, f-strings), changes module-level resolution for nested Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,230,201,0.5)
Compiler->>Compiler: detect builtin-generator-call pattern or constant-foldable expr
Compiler->>Bytecode: emit specialized bytecode (aggregate loops / short-circuit) or fallback CALL sequence
end
rect rgba(187,222,251,0.5)
Bytecode->>VM: execute LOAD_COMMON_CONSTANT(BuiltinList/BuiltinSet/...), CALL/ITER ops
VM->>Stack: push types/tuple/constants and iterate/accumulate results
end
rect rgba(255,224,178,0.5)
VM->>Runtime: perform call/iteration/return control flow as emitted
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 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🧪 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 |
youknowone
force-pushed
the
bytecode-parity
branch
from
31d88bf to
3f69ae4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/codegen/src/compile.rs (3)
2014-2022: Consider caching the nested-global lookup.
compile_name()will re-walk the nested symbol-table tree for every module-scope name op here. On large modules with lots of nested scopes, that turns this parity fix into repeated O(subtables) work. Precomputing the affected names once onSymbolTablewould keep this path cheap.Also applies to: 2110-2114
🤖 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 2014 - 2022, The nested-global lookup in module_name_declared_global_in_nested_scope is expensive because compile_name re-walks sub_tables for every module-scope name; add a cached collection on SymbolTable (e.g., nested_globals: HashSet<String> or a lazily-computed cached_nested_globals) that is populated once by walking sub_tables and noting names with SymbolScope::GlobalExplicit, then change module_name_declared_global_in_nested_scope (or its callers such as compile_name) to consult that cache instead of recursively iterating sub_tables; ensure the cache is updated/filled when the SymbolTable is built or mutated so subsequent calls are O(1).
3445-3599: Factor the shared handler emitter back out of the no-finally fast path.
compile_try_except_no_finally()now carries a second copy of the exception-match / alias-cleanup flow that already exists incompile_try_statement(). That makes future parity or exception-table fixes easy to miss in one branch. Please consider extracting the common handler emission and only specializing the exit targets.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."
🤖 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 3445 - 3599, The duplicate exception-match / alias-cleanup logic in compile_try_except_no_finally should be factored into a shared helper used by both compile_try_except_no_finally and compile_try_statement: extract the per-handler emission (the code that handles CheckExcMatch, PopTop/PopExcept, alias store/cleanup, JumpNoInterrupt to end_block and the cleanup-end path that Reraise) into a new method (e.g., emit_exception_handler or emit_handler_body) that accepts parameters for the handler AST node, the next_handler block, the end_block/cleanup_end targets, and any FBlockDatum needed; replace the duplicated blocks in compile_try_except_no_finally with calls to that helper and update compile_try_statement to call it too so only the differing targets are passed in and the shared logic lives in one place.
2574-2584: Use the fold result as the branch condition.This path classifies constants twice (
is_const_expression()andtry_fold_constant_expr()), and theexpect()turns any future drift between them into a compiler panic. Branching ontry_fold_constant_expr()directly keeps the optimization and removes the invariant.Refactor sketch
- let preserve_tos = !Self::is_const_expression(v); - if preserve_tos { - self.compile_expression(v)?; - } - self.unwind_fblock_stack(preserve_tos, false)?; - if !preserve_tos { - let constant = self - .try_fold_constant_expr(v)? - .expect("literal return should fold to a constant"); - self.emit_load_const(constant); - } + if let Some(constant) = self.try_fold_constant_expr(v)? { + self.unwind_fblock_stack(false, false)?; + self.emit_load_const(constant); + } else { + self.compile_expression(v)?; + self.unwind_fblock_stack(true, false)?; + }🤖 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 2574 - 2584, Call try_fold_constant_expr(v) once and branch based on its Option result instead of using is_const_expression() and expect(): let folded = self.try_fold_constant_expr(v)?; set preserve_tos = folded.is_none(); if preserve_tos then call self.compile_expression(v)?; then call self.unwind_fblock_stack(preserve_tos, false)?; finally, if let Some(constant) = folded { self.emit_load_const(constant); } This removes the double classification (is_const_expression vs try_fold_constant_expr), avoids the expect() panic, and preserves the original behavior of compile_expression, unwind_fblock_stack, and emit_load_const.
🤖 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/compiler-core/src/bytecode/oparg.rs`:
- Around line 648-651: The bytecode format changed by adding new CommonConstant
discriminants (BuiltinList and BuiltinSet), so update the PYC_MAGIC_NUMBER
constant to the next value (increment from 2997 to 2998) wherever
PYC_MAGIC_NUMBER is defined/used so .pyc headers reflect the new format; ensure
the change references the CommonConstant enum (BuiltinList = 5, BuiltinSet = 6)
so any tooling that reads/writes .pyc files uses the new magic number.
---
Nitpick comments:
In `@crates/codegen/src/compile.rs`:
- Around line 2014-2022: The nested-global lookup in
module_name_declared_global_in_nested_scope is expensive because compile_name
re-walks sub_tables for every module-scope name; add a cached collection on
SymbolTable (e.g., nested_globals: HashSet<String> or a lazily-computed
cached_nested_globals) that is populated once by walking sub_tables and noting
names with SymbolScope::GlobalExplicit, then change
module_name_declared_global_in_nested_scope (or its callers such as
compile_name) to consult that cache instead of recursively iterating sub_tables;
ensure the cache is updated/filled when the SymbolTable is built or mutated so
subsequent calls are O(1).
- Around line 3445-3599: The duplicate exception-match / alias-cleanup logic in
compile_try_except_no_finally should be factored into a shared helper used by
both compile_try_except_no_finally and compile_try_statement: extract the
per-handler emission (the code that handles CheckExcMatch, PopTop/PopExcept,
alias store/cleanup, JumpNoInterrupt to end_block and the cleanup-end path that
Reraise) into a new method (e.g., emit_exception_handler or emit_handler_body)
that accepts parameters for the handler AST node, the next_handler block, the
end_block/cleanup_end targets, and any FBlockDatum needed; replace the
duplicated blocks in compile_try_except_no_finally with calls to that helper and
update compile_try_statement to call it too so only the differing targets are
passed in and the shared logic lives in one place.
- Around line 2574-2584: Call try_fold_constant_expr(v) once and branch based on
its Option result instead of using is_const_expression() and expect(): let
folded = self.try_fold_constant_expr(v)?; set preserve_tos = folded.is_none();
if preserve_tos then call self.compile_expression(v)?; then call
self.unwind_fblock_stack(preserve_tos, false)?; finally, if let Some(constant) =
folded { self.emit_load_const(constant); } This removes the double
classification (is_const_expression vs try_fold_constant_expr), avoids the
expect() panic, and preserves the original behavior of compile_expression,
unwind_fblock_stack, and emit_load_const.
🪄 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: 530d1c2c-b85c-4605-bce9-ac3e930ac148
⛔ 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 (3)
crates/codegen/src/compile.rscrates/compiler-core/src/bytecode/oparg.rscrates/vm/src/frame.rs
Comment on lines
+648
to
+651
| /// Built-in `list` type | ||
| BuiltinList = 5, | ||
| /// Built-in `set` type | ||
| BuiltinSet = 6, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if PYC_MAGIC_NUMBER was modified in this PR
echo "=== Current PYC_MAGIC_NUMBER value ==="
rg -n "PYC_MAGIC_NUMBER:\s*u16\s*=" crates/vm/src/version.rs
echo ""
echo "=== Check git diff for version.rs changes ==="
git diff origin/main -- crates/vm/src/version.rs 2>/dev/null || echo "Unable to diff against main"
echo ""
echo "=== Full context around magic number ==="
rg -B2 -A5 "pub const PYC_MAGIC_NUMBER" crates/vm/src/version.rsRepository: RustPython/RustPython
Length of output: 780
🏁 Script executed:
#!/bin/bash
# Search for CommonConstant serialization/deserialization
echo "=== CommonConstant TryFrom/From implementations ==="
rg -B2 -A10 "impl.*TryFrom.*CommonConstant|impl.*From.*CommonConstant" crates/
echo ""
echo "=== Check if CommonConstant is stored in bytecode ==="
rg -B3 -A3 "CommonConstant" crates/compiler-core/src/bytecode/ | head -100
echo ""
echo "=== Check version.rs for any notes on format versioning ==="
cat crates/vm/src/version.rs | head -50Repository: RustPython/RustPython
Length of output: 5546
🏁 Script executed:
#!/bin/bash
# Check if BuiltinList and BuiltinSet are actually used in the compiler
echo "=== Usage of BuiltinList and BuiltinSet in compiler ==="
rg "BuiltinList|BuiltinSet" crates/compiler-core/src/ --with-filename
echo ""
echo "=== Check if CommonConstant is used in code generation ==="
rg "CommonConstant::" crates/compiler-core/src/ -B2 -A2 | head -80
echo ""
echo "=== Check the complete CommonConstant enum definition ==="
rg -B5 "pub enum CommonConstant" crates/compiler-core/src/bytecode/oparg.rs -A30Repository: RustPython/RustPython
Length of output: 1688
🏁 Script executed:
#!/bin/bash
# Check if there are any compiler passes or functions that generate LoadCommonConstant instructions
echo "=== Search for LoadCommonConstant instruction generation ==="
rg "LoadCommonConstant" crates/compiler-core/src/ -B3 -A3
echo ""
echo "=== Check compilation context for common constant usage ==="
rg "CommonConstant" crates/compiler-core/src/codegen/ -B2 -A2
echo ""
echo "=== Verify if these enum variants are actually referenced anywhere ==="
rg "BuiltinList|BuiltinSet" crates/ --type rust | grep -v "oparg.rs" | grep -v "\.pyc"Repository: RustPython/RustPython
Length of output: 2905
🏁 Script executed:
#!/bin/bash
# Check how LoadCommonConstant is encoded/decoded in bytecode
echo "=== Check bytecode marshaling/unmarshaling of CommonConstant ==="
rg "CommonConstant" crates/vm/src/ -B3 -A3
echo ""
echo "=== Check if there's a TryFrom u8 for CommonConstant ==="
rg -A20 "impl.*TryFrom.*u8.*CommonConstant|impl.*From.*u8.*CommonConstant" crates/
echo ""
echo "=== Check recent git history for magic number bumps ==="
git log --oneline crates/vm/src/version.rs | head -10
echo ""
echo "=== Look for any serialization logic ==="
rg "serialize|deserialize" crates/compiler-core/src/bytecode/oparg.rs | head -20Repository: RustPython/RustPython
Length of output: 1892
🏁 Script executed:
#!/bin/bash
# Check the current git status to understand what's being changed
echo "=== Check git status and recent changes ==="
git status
echo ""
echo "=== Check if BuiltinList/BuiltinSet references exist in current HEAD ==="
git show HEAD:crates/compiler-core/src/bytecode/oparg.rs | grep -c "BuiltinList\|BuiltinSet" || echo "Not found in HEAD"
echo ""
echo "=== Full diff for oparg.rs to see what was changed ==="
git diff HEAD -- crates/compiler-core/src/bytecode/oparg.rs | head -100
echo ""
echo "=== Check if frame.rs already had these references before ==="
git show HEAD:crates/vm/src/frame.rs | grep -A2 "BuiltinList\|BuiltinSet" | head -20Repository: RustPython/RustPython
Length of output: 612
🏁 Script executed:
#!/bin/bash
# Verify these variants are actively being generated in bytecode
echo "=== Check when BuiltinList/BuiltinSet were added to the enum ==="
git log -p --all -S "BuiltinList" -- crates/compiler-core/src/bytecode/oparg.rs | head -80
echo ""
echo "=== Check git log for PYC_MAGIC_NUMBER changes ==="
git log --all --oneline -- crates/vm/src/version.rs | head -20
echo ""
echo "=== Check if there are comments or TODOs about versioning ==="
rg "TODO|FIXME|bump.*magic|version.*bytecode" crates/vm/src/version.rs crates/compiler-core/src/bytecode/oparg.rsRepository: RustPython/RustPython
Length of output: 652
🏁 Script executed:
#!/bin/bash
# Check the complete diff context for the PR
echo "=== Check all diffs for oparg.rs ==="
git diff HEAD -- crates/compiler-core/src/bytecode/oparg.rs
echo ""
echo "=== Verify current state: do BuiltinList and BuiltinSet exist in oparg.rs? ==="
rg -n "BuiltinList|BuiltinSet" crates/compiler-core/src/bytecode/oparg.rs
echo ""
echo "=== Check compile.rs for these references ==="
git show HEAD:crates/codegen/src/compile.rs | grep -B3 -A3 "BuiltinList\|BuiltinSet" | head -30Repository: RustPython/RustPython
Length of output: 1345
🏁 Script executed:
#!/bin/bash
# Check if these variants are actually new or already existed
echo "=== Check commit history for when these were added ==="
git log --all --oneline -- crates/compiler-core/src/bytecode/oparg.rs | head -5
echo ""
echo "=== Check the specific commit that added BuiltinList/Set ==="
git log --all -p -- crates/compiler-core/src/bytecode/oparg.rs | grep -B20 "BuiltinList" | head -50
echo ""
echo "=== Determine current PR: check if these were just added or already existed ==="
# Try to find the most recent change to these specific lines
git blame crates/compiler-core/src/bytecode/oparg.rs | grep -A1 -B1 "BuiltinList\|BuiltinSet"Repository: RustPython/RustPython
Length of output: 2316
🏁 Script executed:
#!/bin/bash
# Check what commit 3f69ae47 actually changed
echo "=== Examine commit 3f69ae47 ==="
git show 3f69ae47 --stat | head -30
echo ""
echo "=== Check if magic number was changed in that commit ==="
git show 3f69ae47 -- crates/vm/src/version.rs | head -50
echo ""
echo "=== Full diff of version.rs in that commit ==="
git show 3f69ae47:crates/vm/src/version.rs | grep -B2 -A2 "PYC_MAGIC_NUMBER"Repository: RustPython/RustPython
Length of output: 3910
🏁 Script executed:
#!/bin/bash
# Check if BuiltinList/BuiltinSet existed before commit 3f69ae47
echo "=== Check parent commit of 3f69ae47 ==="
git show 3f69ae47^:crates/compiler-core/src/bytecode/oparg.rs 2>/dev/null | grep -A20 "pub enum CommonConstant" || echo "File did not exist in parent"
echo ""
echo "=== Verify: were these variants newly added or already present? ==="
git show 959b088d:crates/compiler-core/src/bytecode/oparg.rs 2>/dev/null | grep "BuiltinList\|BuiltinSet" || echo "Not found in 959b088d"
echo ""
echo "=== Check if CommonConstant enum structure changed ==="
git diff 959b088d 3f69ae47 -- crates/compiler-core/src/bytecode/oparg.rs | grep -B5 -A5 "BuiltinList\|BuiltinSet" || echo "No direct diff found"
echo ""
echo "=== Summary: does HEAD have BuiltinList/BuiltinSet implementation in frame.rs? ==="
git show HEAD:crates/vm/src/frame.rs | grep -B2 -A2 "BuiltinList\|BuiltinSet" | wc -lRepository: RustPython/RustPython
Length of output: 968
Bump PYC_MAGIC_NUMBER for bytecode format change.
Adding new CommonConstant discriminants (BuiltinList = 5, BuiltinSet = 6) extends the bytecode format. Although the compiler and VM implementations are coordinated in this commit, the PYC_MAGIC_NUMBER should be incremented from 2997 to prevent bytecode format confusion when .pyc files are distributed or cached. Old .pyc files with magic 2997 won't contain these new constant values, but new files will, creating a compatibility risk.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/compiler-core/src/bytecode/oparg.rs` around lines 648 - 651, The
bytecode format changed by adding new CommonConstant discriminants (BuiltinList
and BuiltinSet), so update the PYC_MAGIC_NUMBER constant to the next value
(increment from 2997 to 2998) wherever PYC_MAGIC_NUMBER is defined/used so .pyc
headers reflect the new format; ensure the change references the CommonConstant
enum (BuiltinList = 5, BuiltinSet = 6) so any tooling that reads/writes .pyc
files uses the new magic number.
3f69ae4 to
bdc7365
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/codegen/src/ir.rs (1)
1178-1186: Minor: Redundant bounds check.The condition
i + 1 < block.instructions.len()on line 1178 is redundant since the outerwhileloop (line 1137) already guarantees this invariant. The code is correct, just slightly verbose.♻️ Suggested simplification
- } else if i + 1 < block.instructions.len() - && matches!( + } else if matches!( block.instructions[i].instr.real(), Some(Instruction::BuildList { .. } | Instruction::BuildSet { .. }) )🤖 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 1178 - 1186, Remove the redundant bounds check `i + 1 < block.instructions.len()` from the compound `if` condition since the surrounding `while` loop already ensures `i + 1` is in range; keep the remaining `matches!` checks against `block.instructions[i].instr.real()` and `block.instructions[i + 1].instr.real()` (the `Instruction::BuildList | BuildSet` and `Instruction::GetIter` checks) unchanged so the logic is preserved.
🤖 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 2574-2584: Compute the constant fold result first (call
try_fold_constant_expr(v) and store the Option) and use its presence to decide
the fast-path instead of relying on is_const_expression(); set preserve_tos =
folded.is_none(), call compile_expression(v) only when preserve_tos is true,
call unwind_fblock_stack(preserve_tos, false) as before, and when folded is
Some(constant) call emit_load_const(constant) (avoiding the expect on
try_fold_constant_expr and using the fold result directly). Use the symbols
try_fold_constant_expr, is_const_expression, compile_expression,
unwind_fblock_stack, and emit_load_const to locate and update the logic.
- Around line 3525-3554: The current normal-exit alias cleanup keeps the outer
FBlockType::ExceptionHandler active across emit!(Instruction::PopExcept) and the
subsequent store_name/compile_name calls, which can cause PopExcept/Copy to run
against a stack missing prev_exc; to fix, pop the outer ExceptionHandler (call
self.pop_fblock(FBlockType::ExceptionHandler)) before emitting PopExcept and
performing the alias cleanup (the sequence around emit!(Instruction::PopExcept),
self.emit_load_const(ConstantData::None), self.store_name(alias.as_str()), and
self.compile_name(alias.as_str(), NameUsage::Delete)), and mirror the
non-finalbody teardown ordering where cleanup_end is switched-to/compiled first
and the handler is popped/re-pushed around normal-exit cleanup; adjust usage of
switch_to_block(cleanup_end), emit!(PseudoInstruction::JumpNoInterrupt { delta:
end_block }), and emit!(Instruction::Reraise { depth: 1 }) accordingly so the
outer handler is removed prior to name cleanup.
---
Nitpick comments:
In `@crates/codegen/src/ir.rs`:
- Around line 1178-1186: Remove the redundant bounds check `i + 1 <
block.instructions.len()` from the compound `if` condition since the surrounding
`while` loop already ensures `i + 1` is in range; keep the remaining `matches!`
checks against `block.instructions[i].instr.real()` and `block.instructions[i +
1].instr.real()` (the `Instruction::BuildList | BuildSet` and
`Instruction::GetIter` checks) unchanged so the logic is preserved.
🪄 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: dbd017b1-5bf6-4100-a5f7-e635015ed7a9
⛔ 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 (4)
crates/codegen/src/compile.rscrates/codegen/src/ir.rscrates/compiler-core/src/bytecode/oparg.rscrates/vm/src/frame.rs
✅ Files skipped from review due to trivial changes (1)
- crates/compiler-core/src/bytecode/oparg.rs
youknowone
force-pushed
the
bytecode-parity
branch
from
bdc7365 to
9334687
Compare
Contributor
There was a problem hiding this comment.
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/ir.rs (1)
1170-1177:⚠️ Potential issue | 🟠 MajorThe frozenset fast path still misses the common 3+ constant-set case.
Because
fold_set_constants()still triggers forset_size >= 3at Line 1260 and inserts a tuple constant at Lines 1302-1304, the newConstantData::Frozensetbranch here is only reachable for 1-2 element sets. Lines 1170-1177 then stripBUILD_SET/SET_UPDATEfrom the 3+ element path, leavingLOAD_CONST (tuple) + GET_ITER. That makes the iterator source depend on literal size and misses the parity goal; the earlier set fold needs to emitFrozensettoo before this fast path can elide the build.Also applies to: 1210-1216
🤖 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 1170 - 1177, The fast-path that replaces BUILD_SET/SET_UPDATE with NOPs only handles ConstantData::Frozenset for 1–2 element cases but fold_set_constants still emits a tuple constant for sets with size >= 3, so update the code path so the parity invariant holds: modify fold_set_constants to emit ConstantData::Frozenset for all folded literal sets (including size >= 3) instead of a tuple, or alternatively extend the BUILD_SET/SET_UPDATE fast-path in the block handling (the code that touches block.instructions[i], block.instructions[i+2], and sets Instruction::Nop) to also detect the LOAD_CONST(tuple) pattern and treat it equivalently to ConstantData::Frozenset; apply the same change to the sibling fast-path handling (the other block analogous to the one around i+40) so both locations handle the 3+ element case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/codegen/src/ir.rs`:
- Around line 1170-1177: The fast-path that replaces BUILD_SET/SET_UPDATE with
NOPs only handles ConstantData::Frozenset for 1–2 element cases but
fold_set_constants still emits a tuple constant for sets with size >= 3, so
update the code path so the parity invariant holds: modify fold_set_constants to
emit ConstantData::Frozenset for all folded literal sets (including size >= 3)
instead of a tuple, or alternatively extend the BUILD_SET/SET_UPDATE fast-path
in the block handling (the code that touches block.instructions[i],
block.instructions[i+2], and sets Instruction::Nop) to also detect the
LOAD_CONST(tuple) pattern and treat it equivalently to ConstantData::Frozenset;
apply the same change to the sibling fast-path handling (the other block
analogous to the one around i+40) so both locations handle the 3+ element case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 2f246cfa-9428-4106-9a96-a28ca59eea46
⛔ 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 (4)
crates/codegen/src/compile.rscrates/codegen/src/ir.rscrates/compiler-core/src/bytecode/oparg.rscrates/vm/src/frame.rs
✅ Files skipped from review due to trivial changes (2)
- crates/compiler-core/src/bytecode/oparg.rs
- crates/vm/src/frame.rs
👮 Files not reviewed due to content moderation or server errors (1)
- crates/codegen/src/compile.rs
youknowone
force-pushed
the
bytecode-parity
branch
from
9334687 to
7f7b29e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/codegen/src/ir.rs (1)
28-29: Use the new sequence threshold in the set-folding path too.
MIN_CONST_SEQUENCE_SIZEnow drives list folding, butfold_set_constantsstill hardcodes3. The thresholds will drift the next time this gets tuned.♻️ Suggested cleanup
- if set_size < 3 || i < set_size { + if set_size < MIN_CONST_SEQUENCE_SIZE || i < set_size {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: 1083-1085
🤖 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 28 - 29, Replace the hardcoded "3" used in the set-folding path with the MIN_CONST_SEQUENCE_SIZE constant and refactor the branching so the threshold is computed once and shared by both list and set folding logic; specifically update the fold_set_constants logic to reference MIN_CONST_SEQUENCE_SIZE (and/or assign it to a local variable like threshold) instead of literal 3 and call the common folding code/path so both branches use the same configurable threshold (also apply the same change to the nearby branches around fold_set_constants that currently duplicate the value).
🤖 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/ir.rs`:
- Around line 832-836: The code casts the exponent with "as usize" before
calling num_traits::pow::pow which truncates on 32-bit targets; replace the
unchecked cast with a checked conversion (e.g. use exp.try_into() or
usize::try_from(exp)) and return None if the exponent does not fit in usize so
we don't call pow with a truncated exponent; update the block around the
variables r/exp and the call to num_traits::pow::pow and keep the existing
overflow/size check against MAX_INT_SIZE_BITS.
---
Nitpick comments:
In `@crates/codegen/src/ir.rs`:
- Around line 28-29: Replace the hardcoded "3" used in the set-folding path with
the MIN_CONST_SEQUENCE_SIZE constant and refactor the branching so the threshold
is computed once and shared by both list and set folding logic; specifically
update the fold_set_constants logic to reference MIN_CONST_SEQUENCE_SIZE (and/or
assign it to a local variable like threshold) instead of literal 3 and call the
common folding code/path so both branches use the same configurable threshold
(also apply the same change to the nearby branches around fold_set_constants
that currently duplicate the value).
🪄 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: fdbc29b4-ca92-4e33-a4ff-3a1bee3f01c1
⛔ 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 (4)
crates/codegen/src/compile.rscrates/codegen/src/ir.rscrates/compiler-core/src/bytecode/oparg.rscrates/vm/src/frame.rs
✅ Files skipped from review due to trivial changes (1)
- crates/vm/src/frame.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/compiler-core/src/bytecode/oparg.rs
👮 Files not reviewed due to content moderation or server errors (1)
- crates/codegen/src/compile.rs
youknowone
force-pushed
the
bytecode-parity
branch
from
7f7b29e to
ee92b35
Compare
Contributor
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: (module 'monitoring test_scope' not found) Legend:
|
youknowone
force-pushed
the
bytecode-parity
branch
2 times, most recently
from
e150d64 to
8651fbb
Compare
- Add CFG block splitting, jump threading, backward jump normalization - Add genexpr StopIteration wrapper - Add ConstantData::Slice and constant slice folding - Add duplicate_exits_without_lineno and Block: Clone - Add builtin(genexpr) optimization for tuple/list/set/all/any - Add compile_try_except_no_finally for try-except without finally - Add module_name_declared_global_in_nested_scope - Add constant tuple folding in try_fold_constant_expr - Add fstring literal-only optimization and empty literal elision - Fix duplicate_exits_without_lineno: splice new blocks into linked list
youknowone
force-pushed
the
bytecode-parity
branch
from
8651fbb to
ca6e56a
Compare
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
New Features
Performance
Bug Fixes
Tests