Upgrade test_code and fix code bugs - #7440
Merged
youknowone merged 4 commits intoMar 17, 2026
Merged
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
📝 Walkthrough
WalkthroughThis 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)
✏️ 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. Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the reviews.tools.github-checks setting to adjust the time to wait for GitHub Checks to complete. |
Sorry, something went wrong.
Copy link
Copy Markdown
Contributor
📦 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
youknowone
force-pushed
the
pycode
branch
2 times, most recently
from
8aae51e to
6118492
Compare
March 16, 2026 06:52
youknowone
marked this pull request as ready for review
March 16, 2026 07:17
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)crates/codegen/src/ir.rs (1)🤖 Prompt for all review comments with AI agentscrates/codegen/src/compile.rs (1)783-789: Tightened LOAD_SMALL_INT range is correct; consider clarifying the nearby comment.
Lines 783-784 correctly constrain encoding to 0..=255 for 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 AgentsVerify 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).6906-6917: Avoid duplicate constant-literal classifiers
This helper duplicates ExprExt::is_constant (Lines 50-60). Keeping both implementations in sync is error-prone; prefer reusing the trait method.
Suggested refactor🤖 Prompt for AI Agents/// 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() }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.
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
📥 CommitsReviewing files that changed from the base of the PR and between f1ddb4f and 6118492.
⛔ Files ignored due to path filters (3)- Lib/test/_code_definitions.py is excluded by !Lib/**
- Lib/test/test_code.py is excluded by !Lib/**
- Lib/test/test_dis.py is excluded by !Lib/**
- crates/codegen/src/compile.rs
- crates/codegen/src/ir.rs
- crates/compiler-core/src/bytecode.rs
- crates/compiler-core/src/bytecode/instruction.rs
- crates/vm/src/builtins/code.rs
- crates/compiler-core/src/bytecode/instruction.rs
Sorry, something went wrong.
Comment thread
crates/codegen/src/compile.rs
Comment thread
crates/codegen/src/compile.rs
Comment on lines
+2193
to
+2209
| let dominated_by_interactive = | ||
| self.interactive && !self.ctx.in_func() && !self.ctx.in_class; | ||
| if !dominated_by_interactive && Self::is_const_expression(value) { | ||
| // Skip compilation entirely - the expression has no side effects | ||
| } else { | ||
| self.compile_expression(value)?; | ||
|
|
||
| if self.interactive && !self.ctx.in_func() && !self.ctx.in_class { | ||
| emit!( | ||
| self, | ||
| Instruction::CallIntrinsic1 { | ||
| func: bytecode::IntrinsicFunction1::Print | ||
| } | ||
| ); | ||
| } | ||
| if dominated_by_interactive { | ||
| emit!( | ||
| self, | ||
| Instruction::CallIntrinsic1 { | ||
| func: bytecode::IntrinsicFunction1::Print | ||
| } | ||
| ); | ||
| } | ||
|
|
||
| emit!(self, Instruction::PopTop); | ||
| emit!(self, Instruction::PopTop); |
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
Interactive printing currently leaks into nested blocks
The condition on Line 2194 is true for nested module-level blocks (if/try/with), so expression statements inside those blocks can get auto-printed. This should be restricted to top-level interactive statements only.
Suggested fix- let dominated_by_interactive =
- self.interactive && !self.ctx.in_func() && !self.ctx.in_class;
+ let dominated_by_interactive = self.interactive
+ && !self.ctx.in_func()
+ && !self.ctx.in_class
+ && self.current_code_info().in_conditional_block == 0;‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let dominated_by_interactive = | |
| self.interactive && !self.ctx.in_func() && !self.ctx.in_class; | |
| if !dominated_by_interactive && Self::is_const_expression(value) { | |
| // Skip compilation entirely - the expression has no side effects | |
| } else { | |
| self.compile_expression(value)?; | |
| if self.interactive && !self.ctx.in_func() && !self.ctx.in_class { | |
| emit!( | |
| self, | |
| Instruction::CallIntrinsic1 { | |
| func: bytecode::IntrinsicFunction1::Print | |
| } | |
| ); | |
| } | |
| if dominated_by_interactive { | |
| emit!( | |
| self, | |
| Instruction::CallIntrinsic1 { | |
| func: bytecode::IntrinsicFunction1::Print | |
| } | |
| ); | |
| } | |
| emit!(self, Instruction::PopTop); | |
| emit!(self, Instruction::PopTop); | |
| let dominated_by_interactive = self.interactive | |
| && !self.ctx.in_func() | |
| && !self.ctx.in_class | |
| && self.current_code_info().in_conditional_block == 0; | |
| if !dominated_by_interactive && Self::is_const_expression(value) { | |
| // Skip compilation entirely - the expression has no side effects | |
| } else { | |
| self.compile_expression(value)?; | |
| if dominated_by_interactive { | |
| emit!( | |
| self, | |
| Instruction::CallIntrinsic1 { | |
| func: bytecode::IntrinsicFunction1::Print | |
| } | |
| ); | |
| } | |
| emit!(self, Instruction::PopTop); |
Verify each finding against the current code and only fix it if needed.
In `@crates/codegen/src/compile.rs` around lines 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.
Sorry, something went wrong.
Comment thread
crates/vm/src/builtins/code.rs
Comment on lines
+796
to
+799
| // spell-checker: ignore lnotab | ||
| // co_lnotab is intentionally not implemented. | ||
| // It was deprecated since 3.12 and scheduled for removal in 3.14. | ||
| // Use co_lines() or co_linetable instead. |
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
⚠️ Potential issue | 🟡 Minor
🧩 Analysis chain🌐 Web query:
Python co_lnotab deprecation removal version
💡 Result:
- code.co_lnotab was deprecated in Python 3.10 by PEP 626 (use code.co_lines() instead). [1]
- It started emitting a proper DeprecationWarning in Python 3.12. [1]
- Although it was initially tracked as “scheduled for removal in Python 3.14” in early 3.12-era notes, that removal was postponed, and the current plan is removal in Python 3.15. [2]
So: expect co_lnotab to be removed in Python 3.15 (not 3.14). [2]
Sources: [1] (Python 3.12 “What’s New”), [2] (python.org core-dev discussion referencing the postponement PR).
Update comment with accurate deprecation timeline.
The comment states co_lnotab was "deprecated since 3.12 and scheduled for removal in 3.14." However, co_lnotab was actually deprecated in Python 3.10 (per PEP 626), with the DeprecationWarning added in Python 3.12. The scheduled removal date was also updated from Python 3.14 to Python 3.15. Consider updating the comment to reflect: "deprecated in Python 3.10, scheduled for removal in Python 3.15."
🤖 Prompt for AI AgentsVerify each finding against the current code and only fix it if needed. In `@crates/vm/src/builtins/code.rs` around lines 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.
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
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)crates/codegen/src/compile.rs (2)🧹 Nitpick comments (1)1084-1090: ⚠️ Potential issue | 🟠 Major
CodeFlags::NESTED is still computed too broadly.
self.code_stack.len() > 1 marks scopes under synthetic parents (e.g. <generic parameters of ...>) as nested, even when the effective parent is module scope.
Suggested fix🤖 Prompt for AI Agents- 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 };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 | 🟠 Major
Interactive 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🤖 Prompt for AI Agents- 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;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.
crates/codegen/src/compile.rs (1)🤖 Prompt for all review comments with AI agents6906-6917: Prefer reusing ExprExt::is_constant to avoid duplicated predicates.
This helper duplicates the literal-const check already defined on ast::Expr, which can drift over time.
Refactor suggestion🤖 Prompt for AI Agentsfn 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() }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.
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
📥 CommitsReviewing files that changed from the base of the PR and between 6118492 and c57f4de.
⛔ Files ignored due to path filters (4)- Lib/test/test_code.py is excluded by !Lib/**
- Lib/test/test_codeop.py is excluded by !Lib/**
- Lib/test/test_dis.py is excluded by !Lib/**
- Lib/test/test_marshal.py is excluded by !Lib/**
- crates/codegen/src/compile.rs
- crates/codegen/src/ir.rs
- crates/compiler-core/src/bytecode.rs
- crates/vm/src/builtins/code.rs
Sorry, something went wrong.
Comment thread
crates/vm/src/builtins/code.rs
Comment on lines
+450
to
+517
| impl Comparable for PyCode { | ||
| fn cmp( | ||
| zelf: &Py<Self>, | ||
| other: &PyObject, | ||
| op: crate::types::PyComparisonOp, | ||
| vm: &VirtualMachine, | ||
| ) -> PyResult<crate::function::PyComparisonValue> { | ||
| op.eq_only(|| { | ||
| let other = class_or_notimplemented!(Self, other); | ||
| let a = &zelf.code; | ||
| let b = &other.code; | ||
| let eq = a.obj_name == b.obj_name | ||
| && a.arg_count == b.arg_count | ||
| && a.posonlyarg_count == b.posonlyarg_count | ||
| && a.kwonlyarg_count == b.kwonlyarg_count | ||
| && a.flags == b.flags | ||
| && a.first_line_number == b.first_line_number | ||
| && a.instructions.original_bytes() == b.instructions.original_bytes() | ||
| && a.linetable == b.linetable | ||
| && a.exceptiontable == b.exceptiontable | ||
| && a.names == b.names | ||
| && a.varnames == b.varnames | ||
| && a.freevars == b.freevars | ||
| && a.cellvars == b.cellvars | ||
| && { | ||
| let a_consts: Vec<_> = a.constants.iter().map(|c| c.0.clone()).collect(); | ||
| let b_consts: Vec<_> = b.constants.iter().map(|c| c.0.clone()).collect(); | ||
| if a_consts.len() != b_consts.len() { | ||
| false | ||
| } else { | ||
| let mut eq = true; | ||
| for (ac, bc) in a_consts.iter().zip(b_consts.iter()) { | ||
| if !vm.bool_eq(ac, bc)? { | ||
| eq = false; | ||
| break; | ||
| } | ||
| } | ||
| eq | ||
| } | ||
| }; | ||
| Ok(eq.into()) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| impl Hashable for PyCode { | ||
| fn hash(zelf: &Py<Self>, vm: &VirtualMachine) -> PyResult<crate::common::hash::PyHash> { | ||
| let code = &zelf.code; | ||
| // Hash a tuple of key attributes, matching CPython's code_hash | ||
| let tuple = vm.ctx.new_tuple(vec![ | ||
| vm.ctx.new_str(code.obj_name.as_str()).into(), | ||
| vm.ctx.new_int(code.arg_count).into(), | ||
| vm.ctx.new_int(code.posonlyarg_count).into(), | ||
| vm.ctx.new_int(code.kwonlyarg_count).into(), | ||
| vm.ctx.new_int(code.varnames.len()).into(), | ||
| vm.ctx.new_int(code.flags.bits()).into(), | ||
| vm.ctx | ||
| .new_int(code.first_line_number.map_or(0, |n| n.get()) as i64) | ||
| .into(), | ||
| vm.ctx.new_bytes(code.instructions.original_bytes()).into(), | ||
| { | ||
| let consts: Vec<_> = code.constants.iter().map(|c| c.0.clone()).collect(); | ||
| vm.ctx.new_tuple(consts).into() | ||
| }, | ||
| ]); | ||
| tuple.as_object().hash(vm) | ||
| } | ||
| } |
Copy link
Copy Markdown
Contributor
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
PyCode equality currently ignores meaningful state fields.
cmp can return True even when source_path, qualname, or max_stackdepth differ. That makes distinct code objects compare equal after replace(...) changes to those fields.
Suggested fix impl Comparable for PyCode {
@@
- let eq = a.obj_name == b.obj_name
+ let eq = a.obj_name == b.obj_name
+ && a.source_path == b.source_path
+ && a.qualname == b.qualname
&& a.arg_count == b.arg_count
&& a.posonlyarg_count == b.posonlyarg_count
&& a.kwonlyarg_count == b.kwonlyarg_count
+ && a.max_stackdepth == b.max_stackdepth
&& a.flags == b.flags
&& a.first_line_number == b.first_line_number
&& a.instructions.original_bytes() == b.instructions.original_bytes()Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/builtins/code.rs` around lines 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.
Sorry, something went wrong.
Merged
youknowone
added a commit
to youknowone/RustPython
that referenced
this pull request
Mar 22, 2026
Upgrade test_code and fix code bugs
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
New Features
Performance Improvements
Compatibility