Align opcode names in `dis` by ShaharNaveh · Pull Request #6526 · RustPython/RustPython · GitHub

Also renamed Instruction:: Pop -> Instruction::PopTop

Summary by CodeRabbit

Refactor
- Unified and standardized stack-cleanup instruction across the compiler, VM, and JIT. Behavior remains the same; this is an internal refactor to make stack-pop operations consistent and improve consistency of generated bytecode and disassembly output.

✏️ Tip: You can customize this high-level summary in your review settings.

📝 Walkthrough

Walkthrough

Renamed the Pop opcode to PopTop across compiler-core, codegen, VM, and JIT so emitted bytecode and dispatch use PopTop while preserving previous pop semantics (remove top-of-stack).

Changes

Cohort / File(s)	Summary
Bytecode definition crates/compiler-core/src/bytecode.rs	Removed Pop enum variant; added PopTop. Updated stack-effect logic, opcode formatting/disassembly, and mappings to reference PopTop.
Code generation crates/codegen/src/compile.rs	Replaced emitted Pop instructions with PopTop across codegen paths (statements, patterns, matches, control-flow stack cleanup). Updated related comments.
VM dispatch / frame execution crates/vm/src/frame.rs, crates/vm/src/bytecode.rs	Removed Pop handling; added PopTop handling in ExecutingFrame::execute_instruction to pop and ignore top-of-stack (returns Ok(None)).
JIT instruction handlers crates/jit/src/instructions.rs	Removed Instruction::Pop handling; added Instruction::PopTop handling that pops the top stack value and returns Ok(()).
Manifests / metadata Cargo.toml	No API surface changes; manifests referenced/updated in diff context.

Sequence Diagram(s)

sequenceDiagram
  participant Codegen
  participant Bytecode as Compiler-core
  participant Runtime as VM/JIT
  rect rgb(240,255,240)
  Note over Codegen,Bytecode: Build-time phase
  Codegen->>Bytecode: emit PopTop
  end
  rect rgb(240,240,255)
  Note over Runtime: Runtime execution
  Runtime->>Runtime: fetch PopTop
  Runtime-->>Runtime: STACK.pop() (discard)
  end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Sort Instruction enum & related match arms #6322 — Modifies the Instruction enum and related match arms in compiler-core/src/bytecode.rs.

Suggested reviewers

youknowone

Poem

🐰 I nibbled code in a quiet spot,

Pop hopped up and became PopTop,
Stack crumbs brushed off with a gentle sweep,
Bytecode hums as the changes creep,
A rabbit claps — small hop, big drop!

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title references 'dis' and 'opcode names', which aligns with the PR's stated objective of aligning opcode names in the dis tooling. However, it is vague and doesn't clearly convey the main change: renaming the Pop opcode variant to PopTop.	Consider a more specific title like 'Rename Pop opcode to PopTop for dis alignment' to clearly communicate the primary change across the codebase.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Actionable comments posted: 0

🧹 Nitpick comments (1)

crates/codegen/src/compile.rs (1)

1104-1115: Guard the compile_block_expr assumption about trailing PopTop

compile_block_expr still unconditionally pops the last instruction, assuming it’s the PopTop emitted for a trailing Stmt::Expr. That was true before the rename, but it’s an implicit contract between compile_statement and compile_block_expr. Consider asserting the last instruction is Instruction::PopTop (or otherwise checking) before popping to make future refactors safer.

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 61ddd98 and 2e220f9.

⛔ Files ignored due to path filters (4)

crates/codegen/src/snapshots/rustpython_codegen__compile__tests__if_ands.snap is excluded by !**/*.snap
crates/codegen/src/snapshots/rustpython_codegen__compile__tests__if_mixed.snap is excluded by !**/*.snap
crates/codegen/src/snapshots/rustpython_codegen__compile__tests__if_ors.snap is excluded by !**/*.snap
crates/codegen/src/snapshots/rustpython_codegen__compile__tests__nested_double_async_with.snap is excluded by !**/*.snap

📒 Files selected for processing (3)

crates/codegen/src/compile.rs
crates/compiler-core/src/bytecode.rs
crates/vm/src/frame.rs

🧰 Additional context used 📓 Path-based instructions (1) **/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust

Files:

crates/codegen/src/compile.rs
crates/vm/src/frame.rs
crates/compiler-core/src/bytecode.rs

🧠 Learnings (1) 📚 Learning: 2025-09-03T13:15:23.436Z

Learnt from: youknowone
Repo: RustPython/RustPython PR: 6123
File: stdlib/src/contextvars.rs:128-134
Timestamp: 2025-09-03T13:15:23.436Z
Learning: Vec::pop_if was stabilized in Rust 1.86.0 (released April 3, 2025) and is available in standard library for conditionally removing the last element from a vector based on a predicate.

Applied to files:

crates/compiler-core/src/bytecode.rs

🧬 Code graph analysis (1) crates/compiler-core/src/bytecode.rs (4)

crates/vm/src/frame.rs (4)

arg (2458-2459)

func (2121-2122)

name (1231-1231)

name (2450-2450)

crates/codegen/src/compile.rs (2)

name (897-899)

varname (900-907)

crates/codegen/src/ir.rs (1)

idx (45-47)

crates/stdlib/src/re.rs (1)

end (378-385)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)

GitHub Check: Run snippets and cpython tests (windows-latest)
GitHub Check: Run snippets and cpython tests (macos-latest)
GitHub Check: Check the WASM package and demo
GitHub Check: Check Rust code with clippy
GitHub Check: Run rust tests (windows-latest)
GitHub Check: Run rust tests (macos-latest)
GitHub Check: Run snippets and cpython tests on wasm-wasi
GitHub Check: Run tests under miri
GitHub Check: Ensure compilation on various targets

🔇 Additional comments (4)

crates/vm/src/frame.rs (1)

1359-1363: LGTM! Clean alignment with CPython naming.

The PopTop instruction handler correctly pops and discards the top-of-stack value. The implementation is straightforward, the comment is clear, and the semantics are unchanged from the previous Pop variant.

crates/codegen/src/compile.rs (2)

1055-1093: PopTop replaces Pop cleanly for simple TOS discards

The new Instruction::PopTop usages here (REPL printing, import-from module pop, statement expr results, with without as, non-simple annotated assigns, and generator comprehensions) are all straightforward “drop unused result” sites and remain stack-correct with stack_effect(PopTop) == -1. No functional change beyond the opcode rename.

Also applies to: 1403-1412, 2050-2061, 2927-2957, 4190-4198, 4810-4833

3136-3710: Pattern-matching and control-flow cleanup PopTop sites remain stack-safe

All pattern/exception-related uses of PopTop (pattern failure cleanups, wildcard/star captures, mapping/sequence/class patterns, Match default/fallthrough handling, and chained-comparison early-exit cleanup) correctly discard subjects or intermediate values that are no longer needed. The PatternContext.on_top accounting plus stack_effect(PopTop) = -1 stays consistent, so control-flow and stack behavior are preserved across the rename.

Also applies to: 3793-3819, 3887-3894, 4125-4131

crates/compiler-core/src/bytecode.rs (1)

588-607: PopTop opcode semantics and disassembly alignment look correct

Instruction::PopTop is documented as a pure STACK.pop() and has stack_effect == -1, matching the old Pop behavior.

The disassembler now prints POP_TOP (and other opcodes in CPython-style ALL_CAPS), which aligns RustPython’s dis output with CPython without changing runtime behavior.

Enum ordering and LAST_INSTRUCTION = Instruction::YieldValue stay intact, so the numeric opcode for this slot is preserved; only the variant name and displayed opname changed. This is a public API rename for Instruction, but internally everything remains consistent.

Also applies to: 795-799, 1672-1797, 1867-1973

Actionable comments posted: 0

🧹 Nitpick comments (1)

crates/jit/src/instructions.rs (1)
580-583: LGTM! The opcode rename correctly aligns with CPython.

The PopTop instruction is properly implemented - it pops the top value from the stack and returns successfully, matching the expected semantics.

Consider adding stack underflow validation for consistency with other stack operations in this file (e.g., lines 279, 447, 555). Most other instructions that pop from the stack use .ok_or(JitCompileError::BadBytecode)? to detect empty stack conditions.
🔎 Optional defensive programming improvement
 Instruction::PopTop => {
-    self.stack.pop();
+    self.stack.pop().ok_or(JitCompileError::BadBytecode)?;
     Ok(())
 }

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2e220f9 and e5abd45.

📒 Files selected for processing (1)

crates/jit/src/instructions.rs

🧰 Additional context used 📓 Path-based instructions (1) **/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass, pymodule, pyfunction, etc.) when implementing Python functionality in Rust

Files:

crates/jit/src/instructions.rs

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)

GitHub Check: Run snippets and cpython tests (ubuntu-latest)
GitHub Check: Run snippets and cpython tests (windows-latest)
GitHub Check: Run rust tests (windows-latest)
GitHub Check: Run snippets and cpython tests (macos-latest)
GitHub Check: Run rust tests (ubuntu-latest)
GitHub Check: Run rust tests (macos-latest)
GitHub Check: Check the WASM package and demo
GitHub Check: Ensure compilation on various targets
GitHub Check: Run tests under miri
GitHub Check: Check Rust code with clippy

Is there reason to change the convention?

Yes. Some tests are checking for the presents/absent of certain opcodes by name like:

RustPython/Lib/test/test_compile.py

Lines 1073 to 1081 in 61ddd98

    
           def test_no_wraparound_jump(self): 
        
               # See https://bugs.python.org/issue46724 
        
               def while_not_chained(a, b, c): 
        
                   while not (a < b < c): 
        
                       pass 
        
               for instr in dis.Bytecode(while_not_chained): 
        
                   self.assertNotEqual(instr.opname, "EXTENDED_ARG")

And pretty much all tests under test_dis.py (which we have a VERY outdated version of ATM), aligning the names of the opcodes just increases our test coverage

👍

ShaharNaveh added 5 commits December 25, 2025 13:12

opcodes dis repr like cpython. POP-> POP_TOP

9b67d41

Adjust frame.rs

ea875f8

Fix codegen

5a816bb

snapshots

01dd81a

Add doc for PopTop

2e220f9

fix jit

e5abd45

coderabbitai Bot reviewed Dec 25, 2025

View reviewed changes

youknowone reviewed Dec 25, 2025

View reviewed changes

youknowone approved these changes Dec 25, 2025

View reviewed changes

youknowone merged commit bcdf37b into RustPython:main Dec 25, 2025
13 checks passed

coderabbitai Bot mentioned this pull request Dec 26, 2025

Reduce usage JumpIfTrueOrPop & JumpIfFalseOrPop opcdes. Refactor compile_compare #6524

Merged

Align opcode names in `dis` by ShaharNaveh · Pull Request #6526 · RustPython/RustPython · GitHub

Uh oh!

Align opcode names in dis - #6526

Align opcode names in dis#6526
youknowone merged 6 commits into
RustPython:mainfrom
ShaharNaveh:bytecode-align-names

ShaharNaveh commented Dec 25, 2025 •
edited by coderabbitai Bot
Loading

Uh oh!

Uh oh!

coderabbitai Bot commented Dec 25, 2025 •
edited
Loading

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot left a comment

Uh oh!

youknowone Dec 25, 2025

Uh oh!

ShaharNaveh Dec 25, 2025 •
edited
Loading

Uh oh!

Uh oh!

youknowone left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

                       match self {
-                          BeforeAsyncWith => w!(BeforeAsyncWith),
+                          BeforeAsyncWith => w!(BEFORE_ASYNC_WITH),

	def test_no_wraparound_jump(self):
	# See https://bugs.python.org/issue46724

	def while_not_chained(a, b, c):
	while not (a < b < c):
	pass

	for instr in dis.Bytecode(while_not_chained):
	self.assertNotEqual(instr.opname, "EXTENDED_ARG")

Align opcode names in `dis` by ShaharNaveh · Pull Request #6526 · RustPython/RustPython · GitHub

Uh oh!

Align opcode names in dis - #6526

Align opcode names in dis#6526youknowone merged 6 commits intoRustPython:mainRustPython/RustPython:mainfrom ShaharNaveh:bytecode-align-namesShaharNaveh/RustPython:bytecode-align-namesCopy head branch name to clipboard

Conversation

ShaharNaveh commented Dec 25, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Dec 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

youknowone Dec 25, 2025

Choose a reason for hiding this comment

Uh oh!

ShaharNaveh Dec 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Align opcode names in dis#6526
youknowone merged 6 commits into
RustPython:mainfrom
ShaharNaveh:bytecode-align-names

ShaharNaveh commented Dec 25, 2025 •
edited by coderabbitai Bot
Loading

coderabbitai Bot commented Dec 25, 2025 •
edited
Loading

ShaharNaveh Dec 25, 2025 •
edited
Loading