GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Split stack_effect into pushed & popped by ShaharNaveh · Pull Request #6893 · RustPython/RustPython

Conversation

@ShaharNaveh

@ShaharNaveh ShaharNaveh commented Jan 29, 2026 •
edited by coderabbitai Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Auto generated both num_popped and num_pushed, and then compared the result with the current stack_effect impl. This will make it easier to auto-generate this in the future, and gets us 1 step closer to #6746 as imo it's better to construct the instruction once instead of validate that the oparg is valid at every operation. Also, it's easier to see if we differences in our opcode impls from CPython (if the stack_effect does not match)

Summary by CodeRabbit

  • Refactor
    • Consolidated stack-effect handling into a structured push/pop abstraction and exposed it across instruction representations so callers can query pushed/popped counts or a combined effect.
  • Chores
    • Added bidirectional conversion for opcode argument values to simplify argument handling and adjusted internal call sites accordingly.
  • Impact
    • Improves consistency and reliability when analyzing and optimizing bytecode.

✏️ 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
Split stack_effect into pushed & popped
52b65a8
@coderabbitai

coderabbitai Bot commented Jan 29, 2026 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

Refactors stack-effect handling by introducing a new StackEffect type (pushed/popped + effect), extends InstructionMetadata with stack_effect_info (returning StackEffect) while keeping a default stack_effect delegating to it, adds From<OpArg> for u32, re-exports StackEffect, and updates call sites in codegen and stdlib to use the new API (plus a minor local import removal).

Changes

Cohort / File(s) Summary
Core trait & implementations
crates/compiler-core/src/bytecode/instruction.rs, crates/compiler-core/src/bytecode.rs
Introduce pub struct StackEffect { pushed, popped }; add stack_effect_info(&self, oparg: u32) -> StackEffect to InstructionMetadata with a default stack_effect that returns .effect(); implement for Instruction/PseudoInstruction/AnyInstruction; re-export StackEffect.
OpArg conversions
crates/compiler-core/src/bytecode/oparg.rs
Add impl From<OpArg> for u32 to enable converting OpArg -> u32.
Call-site updates
crates/codegen/src/ir.rs, crates/stdlib/src/_opcode.rs
Update call sites to pass numeric oparg (use .into() or pass u32) and to consume the new stack-effect API; remove a local import of InstructionMetadata in codegen/src/ir.rs; minor change in _opcode.rs to pass oparg directly.

Sequence Diagram(s)

sequenceDiagram
  participant Codegen as Codegen (crates/codegen)
  participant Stdlib as Stdlib (_opcode)
  participant Compiler as Compiler-core (AnyInstruction / Instruction)
  participant OpArg as OpArg (converter)
  Note over Codegen,Stdlib: Obtain numeric oparg and request stack effect
  Codegen->>OpArg: convert/forward oparg -> u32 (use `.into()` where needed)
  Stdlib->>OpArg: use oparg as u32 / forward as-is
  Codegen->>Compiler: call stack_effect_info(u32) or stack_effect(u32)
  Stdlib->>Compiler: call stack_effect(u32)
  Compiler-->>Codegen: return StackEffect (pushed,popped) / .effect()
  Compiler-->>Stdlib: return StackEffect (pushed,popped) / .effect()
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • Remove SUBSCRIPT, JUMP_IF_{TRUE,FALSE}_OR_POP #6810: Modifies instruction stack-effect API and relates to the InstructionMetadata / stack-effect changes.
  • instruction improvements #6829: Updates IR and call sites to the new StackEffect-based interface.
  • Align psuedo ops to CPython 3.14.2 #6846: Overlaps on stack-effect computation and oparg handling across compiler-core and stdlib.

Suggested reviewers

  • youknowone

Poem

🐇 I hop through bytes and stacky streams,
Counting pops and pushed-up dreams.
From OpArg bits to effect’s song,
I tally tidy, neat, and strong—
A rabbit’s cheer for code that gleams! 🥕

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Split stack_effect into pushed & popped' directly and accurately describes the main architectural change: refactoring stack_effect from a single i32 return into a structured StackEffect type with separate pushed and popped fields.
Docstring Coverage ✅ Passed Docstring coverage is 94.44% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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
  • X
  • Mastodon
  • Reddit
  • LinkedIn

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.

@ShaharNaveh
Fix typo
654609b
coderabbitai[bot]
coderabbitai Bot reviewed Jan 29, 2026
View reviewed changes

@coderabbitai 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: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/stdlib/src/_opcode.rs (1)

169-201: Add validation to reject negative oparg values.

CPython's dis.stack_effect treats oparg as an unsigned bytecode argument and raises ValueError("invalid opcode or oparg") if oparg is negative. Your change from u32 to i32 allows negatives to pass through unchecked, creating a compatibility divergence. Add a guard:

if oparg < 0 {
    return Err(vm.new_value_error("oparg must be non-negative"));
}
🤖 Fix all issues with AI agents
In `@crates/codegen/src/ir.rs`:
- Line 734: Replace the unchecked cast u32::from(info.arg) as i32 with a checked
conversion using i32::try_from(u32::from(info.arg)) and handle the Err case
instead of allowing silent wraparound; specifically, call
instr.stack_effect(i32::try_from(u32::from(info.arg)).expect("OpArg out of i32
range")) or propagate an error if appropriate for your context, and make the
same replacement for the other occurrence noted (the second use of
instr.stack_effect with info.arg).

In `@crates/compiler-core/src/bytecode/instruction.rs`:
- Around line 900-906: The match arm for the opcode BinaryOpInplaceAddUnicode
currently returns 0 pushes which contradicts the IR treatment (pop 2 / push 1)
and causes stack_effect = -2; update the implementation that computes pushes
(e.g., num_pushed or the match in instruction.rs where BinaryOpInplaceAddUnicode
is handled) to return 1 like the other binary ops
(BinaryOpAddFloat/Int/Unicode/Extend/Multiply) so the stack depth calculation
matches the IR semantics.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread crates/codegen/src/ir.rs Resolved

Uh oh!

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

Comment thread crates/compiler-core/src/bytecode/instruction.rs Resolved

Uh oh!

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

@youknowone
youknowone enabled auto-merge (squash) January 29, 2026 13:36
youknowone
youknowone requested changes Jan 29, 2026
View reviewed changes
Comment thread crates/compiler-core/src/bytecode/instruction.rs Outdated
Comment on lines +855 to +859
/// How many items this instruction is popping from the stack.
fn num_popped(&self, oparg: i32) -> i32;

/// How many items this instruction is pushing on the stack.
fn num_pushed(&self, oparg: i32) -> i32;

@youknowone youknowone Jan 29, 2026

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.

I like this idea. How do you think about stack_popped_and_pushed(&self, oparg: i32) -> (i32, i32) to avoid a whole instruction match twice

Sorry, something went wrong.

Uh oh!

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

All reactions

@ShaharNaveh ShaharNaveh Jan 29, 2026

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.

Sure, I can do it as a followup improvement if this is fine by you

Sorry, something went wrong.

Uh oh!

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

All reactions

@ShaharNaveh ShaharNaveh Jan 30, 2026

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.

I've made StackEffect into a struct, this will be clearer than returning an unnamed tuple

Sorry, something went wrong.

Uh oh!

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

All reactions

@youknowone youknowone Jan 30, 2026

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.

oh, i meant not about changing stack_effect return type but merging num_popped and num_pushed into single function returning a tuple (or StackEffect, that's looking better!) to avoid full instruciton match for each function.
if we are going to use stack_effect as a single number for most of places, keeping current -> i32 will be easier for user side.

Sorry, something went wrong.

Uh oh!

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

ShaharNaveh reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread crates/compiler-core/src/bytecode/instruction.rs Outdated
}

pub trait InstructionMetadata {
pub trait StackEffect {

@youknowone youknowone Jan 29, 2026

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.

What's good if we split this trait? It seems required to implement for the all types implementing InstructionMetadata

Sorry, something went wrong.

Uh oh!

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

All reactions

@ShaharNaveh ShaharNaveh Jan 29, 2026 •
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.

No need really, it made more sense for me to split it. but it can be part of InstructionMetadata. I'm fine with whatever you want:)

Sorry, something went wrong.

Uh oh!

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

All reactions

@youknowone youknowone Jan 29, 2026

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.

Splitting traits as grouping is looking attractive while defining and implementing traits, but it usually leads to import hell for user side. I'd like to keep a single trait if all InstructionMetadata and StackEffect has manual implementation requirements. It can be splitted when one has manual implementation but the other one has blanket implementation (or any kind of different implementation strategy)

Sorry, something went wrong.

Uh oh!

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

ShaharNaveh reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
ShaharNaveh added 6 commits January 30, 2026 15:18
@ShaharNaveh
Merge remote-tracking branch 'upstream/main' into opcodes-stack-effect
e407978
@ShaharNaveh
Fix merge
1ea2bd9
@ShaharNaveh
Make StructEffect a struct
97662d0
@ShaharNaveh
Fix more code
5951499
@ShaharNaveh
Merge remote-tracking branch 'upstream/main' into opcodes-stack-effect
b355782
@ShaharNaveh
Fix typo
bf92d5c
coderabbitai[bot]
coderabbitai Bot reviewed Jan 30, 2026
View reviewed changes

@coderabbitai 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: 1

🤖 Fix all issues with AI agents
In `@crates/compiler-core/src/bytecode/instruction.rs`:
- Around line 1396-1435: The From<StackEffect> for i32 impl uses unsafe
unwrap_unchecked on conversions of StackEffect.pushed() and .popped(), which can
UB if values exceed i32::MAX; replace the unsafe block by performing safe
conversions (use i32::try_from(...) and propagate or panic with expect including
the offending value), e.g. convert pushed and popped to i32 via try_from and
subtract normally, and update StackEffect::effect() to rely on this safe
conversion; also fix the "SAFTEY" typo to "SAFETY" in the comment to avoid
confusion.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread crates/compiler-core/src/bytecode/instruction.rs Resolved

Uh oh!

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

ShaharNaveh added 4 commits January 30, 2026 19:19
@ShaharNaveh
Merge remote-tracking branch 'upstream/main' into opcodes-stack-effect
1f92b11
@ShaharNaveh
Simpify stack_effect_info
9f4d512
@ShaharNaveh
Fix code
b5587ac
@ShaharNaveh
Simplfy casting as checks
b4e8dea
@ShaharNaveh
ShaharNaveh requested a review from youknowone January 30, 2026 18:21
@ShaharNaveh
Merge remote-tracking branch 'upstream/main' into opcodes-stack-effect
2137406
youknowone
youknowone reviewed Jan 30, 2026
View reviewed changes
Comment thread crates/compiler-core/src/bytecode/instruction.rs
fn stack_effect_info(&self, oparg: u32) -> StackEffect;

/// Stack effect of [`Self::stack_effect_info`].
fn stack_effect(&self, oparg: u32) -> i32 {

@youknowone youknowone Jan 30, 2026

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
youknowone approved these changes Jan 30, 2026
View reviewed changes
@youknowone
youknowone disabled auto-merge January 31, 2026 07:58
@youknowone
youknowone merged commit 743d6b8 into RustPython:main Jan 31, 2026
13 checks passed

Uh oh!

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

@ShaharNaveh ShaharNaveh mentioned this pull request Jan 31, 2026
Use stack_effect_info for getting pop&push count #6913
Merged
@coderabbitai coderabbitai Bot mentioned this pull request Feb 1, 2026
Rework exception compiler #6909
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments
@youknowone 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

@ShaharNaveh @youknowone
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.