Split stack_effect into pushed & popped#6893

ShaharNaveh

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

coderabbitai

📝 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()

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

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

coderabbitai

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.

coderabbitai

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.

Split stack_effect into pushed & popped

52b65a8

Fix typo

654609b

coderabbitai Bot reviewed Jan 29, 2026

View reviewed changes

youknowone enabled auto-merge (squash) January 29, 2026 13:36

youknowone requested changes Jan 29, 2026

View reviewed changes

ShaharNaveh added 6 commits January 30, 2026 15:18

Merge remote-tracking branch 'upstream/main' into opcodes-stack-effect

e407978

Fix merge

1ea2bd9

Make StructEffect a struct

97662d0

Fix more code

5951499

Merge remote-tracking branch 'upstream/main' into opcodes-stack-effect

b355782

Fix typo

bf92d5c

coderabbitai Bot reviewed Jan 30, 2026

View reviewed changes

ShaharNaveh added 4 commits January 30, 2026 19:19

Merge remote-tracking branch 'upstream/main' into opcodes-stack-effect

1f92b11

Simpify stack_effect_info

9f4d512

Fix code

b5587ac

Simplfy casting as checks

b4e8dea

ShaharNaveh requested a review from youknowone January 30, 2026 18:21

Merge remote-tracking branch 'upstream/main' into opcodes-stack-effect

2137406

youknowone reviewed Jan 30, 2026

View reviewed changes

youknowone approved these changes Jan 30, 2026

View reviewed changes

youknowone disabled auto-merge January 31, 2026 07:58

ShaharNaveh mentioned this pull request Jan 31, 2026

Use stack_effect_info for getting pop&push count #6913

Merged

coderabbitai Bot mentioned this pull request Feb 1, 2026

Rework exception compiler #6909

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split stack_effect into pushed & popped#6893