GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Align opcode names in `dis` by ShaharNaveh · Pull Request #6526 · RustPython/RustPython

Conversation

ShaharNaveh commented Dec 25, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

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.}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

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

coderabbitai Bot commented Dec 25, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

📝 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.}

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          fix jit

e5abd45

coderabbitai Bot reviewed

Dec 25, 2025

View reviewed changes

coderabbitai Bot left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

coderabbitai Bot reviewed

Dec 25, 2025

View reviewed changes

coderabbitai Bot left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

youknowone reviewed

Dec 25, 2025

View reviewed changes

Comment thread

crates/compiler-core/src/bytecode.rs


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

youknowone Dec 25, 2025

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there reason to change the convention?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ShaharNaveh Dec 25, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

youknowone approved these changes

Dec 25, 2025

View reviewed changes

youknowone left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

youknowone merged commit bcdf37b into RustPython:main

Dec 25, 2025

13 checks passed

Uh oh!

There was an error while loading. Please reload this page.

coderabbitai Bot mentioned this pull request

Dec 26, 2025

Reduce usage JumpIfTrueOrPop & JumpIfFalseOrPop opcdes. Refactor compile_compare #6524

Merged

Reviewers

coderabbitai[bot]

coderabbitai[bot] left review comments

youknowone

youknowone approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

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.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ShaharNaveh commented Dec 25, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

coderabbitai Bot commented Dec 25, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

ShaharNaveh Dec 25, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.