Align bytecode codegen structure with CPython 3.14#7588
Align bytecode codegen structure with CPython 3.14#7588youknowone merged 8 commits intoRustPython:mainfrom
Conversation
📝 WalkthroughWalkthroughIntroduces a VM-scoped constant bag ( Changes
Sequence Diagram(s)sequenceDiagram
participant Importer
participant Marshal
participant VM
participant PyCode
Importer->>Marshal: read frozen bytes / marshal deserialize
Marshal->>VM: request ConstantBag (PyVmBag(vm))
VM->>PyCode: PyCode::new_ref_from_frozen(vm, frozen.code)
PyCode-->>VM: PyCode ref (with VM bag)
VM-->>Importer: import_code_obj(PyCode)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 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 |
999252a to
f1beb4e
Compare
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: (module 'compile test_grammar test_patma test_peepholer test_pep646_syntax test_sys_settrace' not found) Legend:
|
b3f1187 to
1dadb52
Compare
…tion alignment - Add BoolOp constant folding with short-circuit semantics in compile_expression - Add constant truthiness evaluation for assert statement optimization - Disable const collection/boolop folding in starred unpack and assignment contexts - Move annotation block generation after body with AnnotationsPlaceholder splicing - Reorder insert_superinstructions to run before push_cold_blocks (matching flowgraph.c) - Lower LOAD_CLOSURE after superinstructions to avoid false LOAD_FAST_LOAD_FAST - Add ToBool before PopJumpIf in comparisons and chained compare cleanup blocks - Unify annotation dict building to always use incremental BuildMap + StoreSubscr - Add TrueDivide constant folding for integer operands - Fold constant sets to Frozenset (not Tuple) in try_fold_constant_collection - Add PyVmBag for frozenset constant materialization in code objects - Add remove_redundant_const_pop_top_pairs pass and peephole const+branch folding - Emit Nop for skipped constant expressions and constant-true asserts - Preserve comprehension local ordering by source-order bound name collection - Simplify annotation scanning in symboltable (remove simple-name gate)
…ow fixes - Reorder comprehension symbol-table walk so the outermost iterator registers its sub_tables in the enclosing scope before the comp scope, and rescan elt/ifs in CPython's order. Codegen peeks past the outermost iterator's nested scopes to find the comprehension table. - For plain try/except, emit handler sub_tables before the else block so codegen's linear sub_table cursor stays aligned. - Rename `collect_simple_annotations` to `collect_annotations` and evaluate non-simple annotations during __annotate__ compilation to preserve source-order side effects while keeping the simple-name index stable. - Dedupe equivalent code constants in `arg_constant` and add a structural equality check on `CodeObject`. - Disable LOAD_FAST_BORROW for the tail end block when a try has a bare `except:` clause, and have `new_block` inherit the flag from the current block. - Remove `cfg!(debug_assertions)` guard around the `optimize_load_fast_borrow` start-depth check so mismatches are handled (return instead of assert) in release builds. - Collapse nop-only blocks that precede a return epilogue and hoist the prior line number into the next real instruction so the line table matches. - Unmark now-passing `test_consts_in_conditionals`, `test_load_fast_unknown_simple`, `test_load_fast_known_because_already_loaded`, and PEP 646 f3/f4 annotation checks.
- In `compile_try_except`, drop the leading Nop and set the end block's source range from the last orelse/body statement so line events after the try fall on the right line. - Recognise constant-false asserts as the direct-raise shape (no ToBool/PopJumpIfFalse) and flip the test assertion accordingly. - Extend `remove_redundant_nops_in_blocks` to also look through a trailing nop before a return-epilogue pair (LoadConst/ReturnValue or LoadSmallInt/ReturnValue) so the epilogue keeps the correct line number. - Rename `preds` to `predecessor_blocks` in the LOAD_FAST_BORROW disable pass and add a test-only `debug_late_cfg_trace` helper. - Regenerate the `nested_double_async_with` snapshot: the tail reference to `stop_exc` now emits LOAD_FAST instead of LOAD_FAST_BORROW.
5b7252c to
34b130d
Compare
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 `@scripts/dis_dump.py`:
- Around line 99-106: The frozenset branch only canonicalizes top-level
frozensets and leaves nested frozensets inside tuples/lists/dicts with
interpreter-dependent ordering; modify the logic in the frozenset handling (the
block that reads argrepr, calls ast.literal_eval into values, and returns
frozenset(...)) to recursively normalize the parsed literal structure before
re-rendering: write a small helper (or reuse _normalize_argrepr recursively)
that walks the evaluated value (handling frozenset, set, tuple, list, dict) and
returns a canonical Python structure where frozensets/sets are converted to
sorted lists or canonicalized representations, then use that normalized
structure to build the final string (the code that currently builds parts and
returns f"frozenset({{{...}}})"). Ensure the helper is applied to each element
in tuples/lists/dicts so nested frozensets are consistently ordered.
🪄 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: d31b5f9c-f0b9-472f-821f-6f65d8b118b5
⛔ Files ignored due to path filters (12)
Lib/test/test_compile.pyis excluded by!Lib/**Lib/test/test_grammar.pyis excluded by!Lib/**Lib/test/test_patma.pyis excluded by!Lib/**Lib/test/test_peepholer.pyis excluded by!Lib/**Lib/test/test_pep646_syntax.pyis excluded by!Lib/**Lib/test/test_sys_settrace.pyis excluded by!Lib/**crates/codegen/src/snapshots/rustpython_codegen__compile__tests__bare_function_annotations_check_attribute_and_subscript_expressions.snapis excluded by!**/*.snapcrates/codegen/src/snapshots/rustpython_codegen__compile__tests__constant_true_if_pass_keeps_line_anchor_nop.snapis excluded by!**/*.snapcrates/codegen/src/snapshots/rustpython_codegen__compile__tests__if_ands.snapis excluded by!**/*.snapcrates/codegen/src/snapshots/rustpython_codegen__compile__tests__if_mixed.snapis excluded by!**/*.snapcrates/codegen/src/snapshots/rustpython_codegen__compile__tests__if_ors.snapis excluded by!**/*.snapcrates/codegen/src/snapshots/rustpython_codegen__compile__tests__nested_double_async_with.snapis excluded by!**/*.snap
📒 Files selected for processing (14)
crates/codegen/src/compile.rscrates/codegen/src/ir.rscrates/codegen/src/symboltable.rscrates/compiler-core/src/bytecode/instruction.rscrates/vm/src/builtins/code.rscrates/vm/src/frame.rscrates/vm/src/import.rscrates/vm/src/stdlib/_ast.rscrates/vm/src/stdlib/_ctypes/simple.rscrates/vm/src/stdlib/_imp.rscrates/vm/src/stdlib/marshal.rscrates/vm/src/types/slot.rscrates/vm/src/vm/compile.rsscripts/dis_dump.py
- Fold a constant list iterable into a constant tuple in for-loop iterable position, matching the CPython optimizer, and strip a redundant LIST_TO_TUPLE immediately before GET_ITER in the IR peephole pass. - Emit a Nop at the break/continue source range before unwinding so line events land on the break/continue statement instead of the following instruction. - Drop `propagate_disable_load_fast_borrow`; the forward propagation was over-zealous and the per-block inheritance in `new_block` plus the bare-except marker are enough. - Relax `inline_small_or_no_lineno_blocks` so small exit blocks at the tail of a cold block are always inlined, not just return epilogues. - Add codegen tests covering the LIST_TO_TUPLE/GET_ITER peephole and the late-CFG trace helper for a for-loop list-literal iterable.
34b130d to
f983099
Compare
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 `@scripts/dis_dump.py`:
- Around line 114-120: The current filler logic for dis._common_constants
assumes the list initially has length 5 and uses (builtins.list,
builtins.set)[len(common_constants) - 5], which yields negative or wrong indices
when the initial length differs; change the loop to explicitly append the
correct constants for the missing slots (i.e., if len(common_constants) == 5
append builtins.list, if == 6 append builtins.set) until length reaches 7, then
assign back to dis._common_constants; reference symbols: _IS_RUSTPYTHON,
dis._common_constants, common_constants, builtins.list, builtins.set, and
LOAD_COMMON_CONSTANT.
🪄 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: 96719fbb-fbe3-4936-aad4-951698fa4c41
⛔ Files ignored due to path filters (10)
Lib/test/test_compile.pyis excluded by!Lib/**Lib/test/test_grammar.pyis excluded by!Lib/**Lib/test/test_patma.pyis excluded by!Lib/**Lib/test/test_peepholer.pyis excluded by!Lib/**Lib/test/test_sys_settrace.pyis excluded by!Lib/**crates/codegen/src/snapshots/rustpython_codegen__compile__tests__bare_function_annotations_check_attribute_and_subscript_expressions.snapis excluded by!**/*.snapcrates/codegen/src/snapshots/rustpython_codegen__compile__tests__constant_true_if_pass_keeps_line_anchor_nop.snapis excluded by!**/*.snapcrates/codegen/src/snapshots/rustpython_codegen__compile__tests__if_ands.snapis excluded by!**/*.snapcrates/codegen/src/snapshots/rustpython_codegen__compile__tests__if_mixed.snapis excluded by!**/*.snapcrates/codegen/src/snapshots/rustpython_codegen__compile__tests__nested_double_async_with.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
crates/codegen/src/compile.rscrates/codegen/src/ir.rscrates/codegen/src/symboltable.rscrates/vm/src/stdlib/_ctypes/simple.rscrates/vm/src/types/slot.rsscripts/dis_dump.py
✅ Files skipped from review due to trivial changes (2)
- crates/vm/src/stdlib/_ctypes/simple.rs
- crates/vm/src/types/slot.rs
| if _IS_RUSTPYTHON and hasattr(dis, "_common_constants"): | ||
| common_constants = list(dis._common_constants) | ||
| while len(common_constants) < 7: | ||
| common_constants.append( | ||
| (builtins.list, builtins.set)[len(common_constants) - 5] | ||
| ) | ||
| dis._common_constants = common_constants |
There was a problem hiding this comment.
Fragile assumption: _common_constants starts with exactly 5 entries.
The index len(common_constants) - 5 assumes the initial list length is exactly 5 (CPython's current size). If RustPython's dis._common_constants is ever shorter than 5, the first iteration uses a negative index (wrong constants appended silently); if it starts between 6 and 7, you may skip builtins.list and jump straight to builtins.set. Either case produces misleading disassembly for LOAD_COMMON_CONSTANT.
🛠️ Suggested hardening
-if _IS_RUSTPYTHON and hasattr(dis, "_common_constants"):
- common_constants = list(dis._common_constants)
- while len(common_constants) < 7:
- common_constants.append(
- (builtins.list, builtins.set)[len(common_constants) - 5]
- )
- dis._common_constants = common_constants
+if _IS_RUSTPYTHON and hasattr(dis, "_common_constants"):
+ common_constants = list(dis._common_constants)
+ # Ensure list/set occupy the RustPython-expected slots 5 and 6.
+ extras = [builtins.list, builtins.set]
+ for idx, value in enumerate(extras, start=5):
+ if len(common_constants) <= idx:
+ common_constants.append(value)
+ elif common_constants[idx] is not value:
+ common_constants[idx] = value
+ dis._common_constants = common_constants🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/dis_dump.py` around lines 114 - 120, The current filler logic for
dis._common_constants assumes the list initially has length 5 and uses
(builtins.list, builtins.set)[len(common_constants) - 5], which yields negative
or wrong indices when the initial length differs; change the loop to explicitly
append the correct constants for the missing slots (i.e., if
len(common_constants) == 5 append builtins.list, if == 6 append builtins.set)
until length reaches 7, then assign back to dis._common_constants; reference
symbols: _IS_RUSTPYTHON, dis._common_constants, common_constants, builtins.list,
builtins.set, and LOAD_COMMON_CONSTANT.
Summary by CodeRabbit
Bug Fixes
Refactor
Chores