GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Split stack_effect into pushed & popped by ShaharNaveh · Pull Request #6893 · RustPython/RustPython Conversation
|
📝 WalkthroughWalkthroughRefactors stack-effect handling by introducing a new Changes
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
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ 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. Comment |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
crates/stdlib/src/_opcode.rs (1)
169-201: Add validation to reject negativeopargvalues.CPython's
dis.stack_effecttreatsopargas an unsigned bytecode argument and raisesValueError("invalid opcode or oparg")ifopargis negative. Your change fromu32toi32allows 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")); }
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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| /// 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; |
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
| } | ||
|
|
||
| pub trait InstructionMetadata { | ||
| pub trait StackEffect { |
There was a problem 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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| fn stack_effect_info(&self, oparg: u32) -> StackEffect; | ||
|
|
||
| /// Stack effect of [`Self::stack_effect_info`]. | ||
| fn stack_effect(&self, oparg: u32) -> i32 { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
👍
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.