Upgrade test_code and fix code bugs#7440
Conversation
📝 WalkthroughWalkthroughThis PR implements nested scope flag handling in code compilation, optimizes constant expression evaluation at module scope, adjusts small integer constant conversion logic, adds a NESTED code flag, removes RustPython-specific opcode gating for CPython 3.14 compatibility, and extends PyCode with comparison, hashing, and replacement capabilities. Changes
Sequence Diagram(s)(No sequence diagrams generated — changes are internal compiler/VM adjustments without a single new multi-component sequential flow that benefits from a diagram.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
Sorry, something went wrong.
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/code.py dependencies:
dependent tests: (1 tests) [x] lib: cpython/Lib/codeop.py dependencies:
dependent tests: (98 tests)
[x] lib: cpython/Lib/dis.py dependencies:
dependent tests: (70 tests)
[x] test: cpython/Lib/test/test_marshal.py (TODO: 25) dependencies: dependent tests: (21 tests)
[x] lib: cpython/Lib/types.py dependencies:
dependent tests: (52 tests)
Legend:
|
Sorry, something went wrong.
- Add Comparable and Hashable traits for PyCode - Compare by name, args, flags, bytecode, consts, names, vars, linetable - Hash by tuple of key attributes matching CPython's code_hash - Remove unused custom_ops slice in Instruction::try_from - Add co_lnotab intentional non-implementation comment
8aae51e to
6118492
Compare
March 16, 2026 06:52
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/codegen/src/ir.rs (1)
783-789: TightenedLOAD_SMALL_INTrange is correct; consider clarifying the nearby comment.Lines 783-784 correctly constrain encoding to
0..=255for unsigned oparg. Optional nit: the comment at Line 787 still references two’s-complement/i32 semantics, which can be read as supporting negative values.🤖 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 783 - 789, The comment next to the LOAD_SMALL_INT conversion is misleading about signed two's-complement semantics; update the comment near the block that checks value.to_i32() and sets instr.instr = Instruction::LoadSmallInt { i: Arg::marker() } and instr.arg = OpArg::new(small as u32) to explicitly state that the oparg is an unsigned u32 in the range 0..=255 and that the i32 is being used only to check range before casting to u32 (remove wording implying negative/two's-complement encoding).crates/codegen/src/compile.rs (1)
6906-6917: Avoid duplicate constant-literal classifiersThis helper duplicates
ExprExt::is_constant(Lines 50-60). Keeping both implementations in sync is error-prone; prefer reusing the trait method.Suggested refactor
/// Returns true if the expression is a constant with no side effects. fn is_const_expression(expr: &ast::Expr) -> bool { - matches!( - expr, - ast::Expr::StringLiteral(_) - | ast::Expr::BytesLiteral(_) - | ast::Expr::NumberLiteral(_) - | ast::Expr::BooleanLiteral(_) - | ast::Expr::NoneLiteral(_) - | ast::Expr::EllipsisLiteral(_) - ) + expr.is_constant() }🤖 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 6906 - 6917, The is_const_expression helper duplicates existing logic in the ExprExt trait; remove or replace its body to call the shared implementation instead of reimplementing the match. Specifically, in the is_const_expression function use ExprExt::is_constant (or call expr.is_constant() if the trait is in scope) so the code delegates to the trait method rather than repeating the literal-match logic; keep the function signature (fn is_const_expression(expr: &ast::Expr) -> bool) so callers need no changes.
🤖 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 2193-2209: The interactive-printing guard
(dominated_by_interactive) currently triggers inside nested module-level blocks;
change its definition to require a true top-level module context so only
top-level interactive expression statements auto-print. Concretely, update
dominated_by_interactive to: self.interactive && !self.ctx.in_func() &&
!self.ctx.in_class && self.ctx.is_top_level_module(), and add/implement a
Context method is_top_level_module() that returns true only when the current
scope is the module top-level (i.e., not inside any nested module-level blocks
like if/try/with). Ensure compile_expression and the subsequent emit of
Instruction::CallIntrinsic1 { Print } remains guarded by the revised
dominated_by_interactive.
In `@crates/vm/src/builtins/code.rs`:
- Around line 796-799: Update the inline comment about co_lnotab to reflect the
correct timeline: change the text that currently reads "deprecated since 3.12
and scheduled for removal in 3.14" to state that co_lnotab was deprecated in
Python 3.10 (per PEP 626), had DeprecationWarning added in Python 3.12, and is
scheduled for removal in Python 3.15; adjust the surrounding sentence to still
recommend using co_lines() or co_linetable and reference co_lnotab, co_lines,
and co_linetable so the intent remains clear.
---
Nitpick comments:
In `@crates/codegen/src/compile.rs`:
- Around line 6906-6917: The is_const_expression helper duplicates existing
logic in the ExprExt trait; remove or replace its body to call the shared
implementation instead of reimplementing the match. Specifically, in the
is_const_expression function use ExprExt::is_constant (or call
expr.is_constant() if the trait is in scope) so the code delegates to the trait
method rather than repeating the literal-match logic; keep the function
signature (fn is_const_expression(expr: &ast::Expr) -> bool) so callers need no
changes.
In `@crates/codegen/src/ir.rs`:
- Around line 783-789: The comment next to the LOAD_SMALL_INT conversion is
misleading about signed two's-complement semantics; update the comment near the
block that checks value.to_i32() and sets instr.instr =
Instruction::LoadSmallInt { i: Arg::marker() } and instr.arg = OpArg::new(small
as u32) to explicitly state that the oparg is an unsigned u32 in the range
0..=255 and that the i32 is being used only to check range before casting to u32
(remove wording implying negative/two's-complement encoding).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 5d992108-8f4a-4a61-964d-449d340b163e
⛔ Files ignored due to path filters (3)
Lib/test/_code_definitions.pyis excluded by!Lib/**Lib/test/test_code.pyis excluded by!Lib/**Lib/test/test_dis.pyis excluded by!Lib/**
📒 Files selected for processing (5)
crates/codegen/src/compile.rscrates/codegen/src/ir.rscrates/compiler-core/src/bytecode.rscrates/compiler-core/src/bytecode/instruction.rscrates/vm/src/builtins/code.rs
💤 Files with no reviewable changes (1)
- crates/compiler-core/src/bytecode/instruction.rs
Sorry, something went wrong.
- Add CO_NESTED flag (0x10) for nested function scopes - Emit LOAD_SMALL_INT for integers 0..=255 instead of LOAD_CONST - Eliminate dead constant expression statements (no side effects) - Ensure None in co_consts for functions with no other constants - Add code.__replace__() for copy.replace() support - Mark test_co_lnotab and test_invalid_bytecode as expectedFailure
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
crates/codegen/src/compile.rs (2)
1084-1090: ⚠️ Potential issue | 🟠 Major
CodeFlags::NESTEDis still computed too broadly.
self.code_stack.len() > 1marks scopes under synthetic parents (e.g.<generic parameters of ...>) as nested, even when the effective parent is module scope.Suggested fix
- let flags = if self.code_stack.len() > 1 { + let is_nested = self + .code_stack + .iter() + .rev() + .skip(1) + .find(|parent| !parent.metadata.name.starts_with("<generic parameters of ")) + .is_some_and(|parent| parent.metadata.name != "<module>"); + + let flags = if is_nested { flags | bytecode::CodeFlags::NESTED } else { flags };🤖 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 1084 - 1090, The NESTED flag is being set whenever self.code_stack.len() > 1 which incorrectly marks scopes whose immediate parent is a synthetic container (e.g., "<generic parameters of ...>") as nested; change the logic that sets bytecode::CodeFlags::NESTED to inspect the actual parent code object on self.code_stack (e.g., let parent = self.code_stack.last().unwrap()) and only set CodeFlags::NESTED when that parent represents a real non-module scope (i.e., not a module-level scope and not synthetic generic-params/placeholder parents); update the condition around flags |= bytecode::CodeFlags::NESTED to check parent.kind / parent.is_module() / parent.is_synthetic() (or equivalent fields on the stack item) so module-equivalent parents do not cause the NESTED flag to be set.
2193-2210: ⚠️ Potential issue | 🟠 MajorInteractive auto-print should only run at true module top level.
The current guard still triggers inside nested module-level blocks (
if/try/with) in REPL mode.Suggested fix
- let dominated_by_interactive = - self.interactive && !self.ctx.in_func() && !self.ctx.in_class; + let at_module_top_level = self.current_code_info().in_conditional_block == 0; + let dominated_by_interactive = self.interactive + && !self.ctx.in_func() + && !self.ctx.in_class + && at_module_top_level;🤖 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 2193 - 2210, The interactive auto-print gate currently uses dominated_by_interactive = self.interactive && !self.ctx.in_func() && !self.ctx.in_class, which still fires inside nested module-level blocks; change the condition to require true module top-level by also ensuring the parser's module-block nesting is zero (e.g. && self.ctx.block_nesting == 0 or an equivalent accessor like self.ctx.in_module_top_level()), so update the dominated_by_interactive calculation and, if needed, add/derive the ctx.block_nesting/in_module_top_level helper to make the check explicit before emitting the Print intrinsic in compile_expression handling.
🧹 Nitpick comments (1)
crates/codegen/src/compile.rs (1)
6906-6917: Prefer reusingExprExt::is_constantto avoid duplicated predicates.This helper duplicates the literal-const check already defined on
ast::Expr, which can drift over time.Refactor suggestion
fn is_const_expression(expr: &ast::Expr) -> bool { - matches!( - expr, - ast::Expr::StringLiteral(_) - | ast::Expr::BytesLiteral(_) - | ast::Expr::NumberLiteral(_) - | ast::Expr::BooleanLiteral(_) - | ast::Expr::NoneLiteral(_) - | ast::Expr::EllipsisLiteral(_) - ) + expr.is_constant() }🤖 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 6906 - 6917, The helper is_const_expression duplicates the literal-const predicate; replace its body to delegate to the existing extension method by calling ExprExt::is_constant (or expr.is_constant() if the trait is in scope) instead of matching literals, or remove the helper and update callers to use ExprExt::is_constant directly; target the function named is_const_expression and references to ast::Expr to make this change.
🤖 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/builtins/code.rs`:
- Around line 450-517: The PyCode equality and hashing omit meaningful fields
(source_path, qualname, max_stackdepth), so update Comparable::cmp (the impl for
PyCode::cmp) to also compare code.source_path, code.qualname, and
code.max_stackdepth alongside the existing checks (add these three comparisons
in the big `eq = ...` expression and include them inside the constants/iter
section as appropriate), and update Hashable::hash (the impl for PyCode::hash)
to include code.source_path, code.qualname, and code.max_stackdepth in the tuple
used for hashing (convert Option/source and qualname to the appropriate VM
objects like strings/None and include max_stackdepth as an int) so that object
equality and hashing remain consistent.
---
Duplicate comments:
In `@crates/codegen/src/compile.rs`:
- Around line 1084-1090: The NESTED flag is being set whenever
self.code_stack.len() > 1 which incorrectly marks scopes whose immediate parent
is a synthetic container (e.g., "<generic parameters of ...>") as nested; change
the logic that sets bytecode::CodeFlags::NESTED to inspect the actual parent
code object on self.code_stack (e.g., let parent =
self.code_stack.last().unwrap()) and only set CodeFlags::NESTED when that parent
represents a real non-module scope (i.e., not a module-level scope and not
synthetic generic-params/placeholder parents); update the condition around flags
|= bytecode::CodeFlags::NESTED to check parent.kind / parent.is_module() /
parent.is_synthetic() (or equivalent fields on the stack item) so
module-equivalent parents do not cause the NESTED flag to be set.
- Around line 2193-2210: The interactive auto-print gate currently uses
dominated_by_interactive = self.interactive && !self.ctx.in_func() &&
!self.ctx.in_class, which still fires inside nested module-level blocks; change
the condition to require true module top-level by also ensuring the parser's
module-block nesting is zero (e.g. && self.ctx.block_nesting == 0 or an
equivalent accessor like self.ctx.in_module_top_level()), so update the
dominated_by_interactive calculation and, if needed, add/derive the
ctx.block_nesting/in_module_top_level helper to make the check explicit before
emitting the Print intrinsic in compile_expression handling.
---
Nitpick comments:
In `@crates/codegen/src/compile.rs`:
- Around line 6906-6917: The helper is_const_expression duplicates the
literal-const predicate; replace its body to delegate to the existing extension
method by calling ExprExt::is_constant (or expr.is_constant() if the trait is in
scope) instead of matching literals, or remove the helper and update callers to
use ExprExt::is_constant directly; target the function named is_const_expression
and references to ast::Expr to make this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: af81529a-eb5b-4d02-b9c2-7c3ec41a96c8
⛔ Files ignored due to path filters (4)
Lib/test/test_code.pyis excluded by!Lib/**Lib/test/test_codeop.pyis excluded by!Lib/**Lib/test/test_dis.pyis excluded by!Lib/**Lib/test/test_marshal.pyis excluded by!Lib/**
📒 Files selected for processing (4)
crates/codegen/src/compile.rscrates/codegen/src/ir.rscrates/compiler-core/src/bytecode.rscrates/vm/src/builtins/code.rs
Sorry, something went wrong.
0768cf8
into
RustPython:main
Mar 17, 2026
Upgrade test_code and fix code bugs
Summary by CodeRabbit
New Features
Performance Improvements
Compatibility