Ensure BuildSlice oparg to be either 2 or 3
#6313
Conversation
WalkthroughReplace the boolean Changes
Sequence Diagram(s)mermaid Compiler->>Bytecode: emit BuildSlice { argc: Two/Three } Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/codegen/src/compile.rs (1)
451-477: Two-element slice fast path usesBuildSliceArgCountcorrectly, but return type could be tightenedThe
is_two_element_sliceguard pluscompile_sliceguaranteesnis always2, soBuildSliceArgCount::from_op_arg(n).unwrap()is safe. If you want to make this more self-documenting and future-proof, you could instead:
- Change
compile_sliceto returnBuildSliceArgCount(and maybe not expose rawu32here), or- Inline
BuildSliceArgCount::Twoat this callsite and treatcompile_sliceas “emit two bounds, no step” for this fast path.Not blocking, but would reduce reliance on the implicit 2/3 contract of
compile_slice.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/codegen/src/compile.rs(3 hunks)crates/compiler-core/src/bytecode.rs(4 hunks)crates/vm/src/frame.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto 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/vm/src/frame.rscrates/compiler-core/src/bytecode.rscrates/codegen/src/compile.rs
🧠 Learnings (1)
📚 Learning: 2025-11-29T12:17:28.583Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.583Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/codegen/src/compile.rs
🧬 Code graph analysis (1)
crates/codegen/src/compile.rs (1)
crates/compiler-core/src/bytecode.rs (7)
from_op_arg(341-341)from_op_arg(348-350)from_op_arg(360-362)from_op_arg(463-465)from_op_arg(481-489)from_op_arg(914-916)from_op_arg(1177-1180)
⏰ 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). (11)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run tests under miri
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Check Rust code with clippy
🔇 Additional comments (5)
crates/vm/src/frame.rs (2)
837-839: LGTM! Type-safe instruction dispatch.The change from
step: booltoargc: BuildSliceArgCountstrengthens type safety by ensuring only valid argument counts (2 or 3) can be dispatched to the slice builder. The call site correctly passesargc.get(arg)to the execution function.
1780-1800: LGTM! Excellent type safety improvement.The refactoring from
step: booltoargc: BuildSliceArgCountmakes the function signature more explicit and type-safe:
- The exhaustive match on the enum clearly documents that only 2 or 3 arguments are valid
- The enum prevents invalid values (e.g.,
-1,0,999) from being misinterpreted- Logic is semantically equivalent to the previous implementation
- Stack operations remain correct
This change successfully enforces the PR objective to restrict
BuildSliceoparg to exactly 2 or 3 arguments.crates/codegen/src/compile.rs (1)
4585-4604:Expr::Sliceemission now encodes argc explicitly and matches stack effectThe new
argccomputation based onsteppresence andemit!(..., Instruction::BuildSlice { argc })keeps the stack behavior correct (2 or 3 inputs → 1 slice object) and aligns withBuildSliceArgCount’s restricted domain. This is exactly what the new bytecode representation expects.crates/compiler-core/src/bytecode.rs (2)
1153-1167:BuildSliceArgCountenum correctly constrains allowedBUILD_SLICEarg countsDefining
BuildSliceArgCountwith repr(u8) and values{ Two = 2, Three = 3 }viaop_arg_enum!ensures that only opargs 2 or 3 round-trip throughOpArgTypeandArg<T>. This is a clean way to encode the CPython-style 2/3 argument semantics forBUILD_SLICEand directly supports the PR’s goal of rejecting other arg counts.
762-764:Instruction::BuildSlicewiring (encoding, stack effect, display) is consistent; consider compatibility expectationsSwitching
BuildSlicetoargc: Arg<BuildSliceArgCount>and computing the stack effect as1 - argckeeps the net stack change correct for both 2- and 3-argument cases, and the disassembly now prints the enum (Two/Three), which remains informative.One behavioral change is that invalid encoded arg values (e.g., legacy
0/1encodings or corrupt bytecode) will now causeArg::get→BuildSliceArgCount::from_op_argto returnNoneand panic on unwrap, whereas previously any non-zero oparg was treated as “has step”. This matches how otherop_arg_enum!-based instructions already behave, but it does tighten the contract forBUILD_SLICE.If you rely on loading older or third-party bytecode streams, it’s worth double-checking that they already encode
BUILD_SLICEwith argument counts 2 or 3, or that such bytecode is rejected before execution.Also applies to: 1565-1569, 1756-1756
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/codegen/src/compile.rs (1)
396-427:compile_slice’sBuildSliceArgCountreturn type aligns withBuildSliceconstraintsThe function now compiles lower/upper bounds, conditionally compiles
step, and returnsBuildSliceArgCount::{Two,Three}, so emittedargccan only be 2 or 3 and always matches the number of pushed values. Currently this helper is only used from the two‑element slice fast path (so theThreearm is effectively unused but harmless). If you want to document that invariant, adebug_assert!(s.step.is_none())here would make it explicit, but it’s optional.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/codegen/src/compile.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto 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
🧠 Learnings (1)
📚 Learning: 2025-11-29T12:17:28.583Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.583Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/codegen/src/compile.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). (11)
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
🔇 Additional comments (3)
crates/codegen/src/compile.rs (3)
35-40: ImportingBuildSliceArgCountis consistent and necessaryThe added
BuildSliceArgCountimport matches its later usage inInstruction::BuildSliceand keeps the bytecode-related imports grouped coherently.
451-466: Two‑element subscript slice fast path correctly wiresargcintoBuildSliceDeriving
argcviacompile_sliceand feeding it intoInstruction::BuildSlice { argc }keeps the stack shape as[value, slice]and guarantees the oparg is constrained to the enum (effectivelyTwohere, sinceis_two_element_sliceenforcesstep.is_none()). The separation between Load/Store (using the fast path) and Del (falling back to genericSubscript/DeleteSubscript) remains intact.
4576-4596:Expr::Sliceemission now usesBuildSliceArgCountwith correct stack arityThe
compile_boundhelper ensures lower/upper each contribute exactly one value (orNone), the optionalstepadds one more when present, andargcis then chosen asTwovsThreeto match. This guaranteesInstruction::BuildSlice { argc }never sees an invalid argument count and satisfies the “only 2 or 3” oparg requirement.
ShaharNaveh
changed the title
BuildSlice oparg to be either 2 or 3BuildSlice oparg to be either 2 or 3
crates/compiler-core/src/bytecode.rs
Outdated
| /// ```py | ||
| /// x[5:10] | ||
| /// ``` | ||
| Two = 2, | ||
| /// ```py | ||
| /// x[5:10:2] | ||
| /// ``` | ||
| Three = 3, |
There was a problem hiding this comment.
| /// ```py | |
| /// x[5:10] | |
| /// ``` | |
| Two = 2, | |
| /// ```py | |
| /// x[5:10:2] | |
| /// ``` | |
| Three = 3, | |
| /// ```py | |
| /// x[5:10] | |
| /// ``` | |
| Two, | |
| /// ```py | |
| /// x[5:10:2] | |
| /// ``` | |
| Three, |
Removing value not to block optimization. Adding a method will be enough
impl BuildSliceArgCount {
const fn argc(&self) -> usize {
....
}
}There was a problem hiding this comment.
Hmmm, but then how this be validated at stack_effect?
There was a problem hiding this comment.
Not by using as cast, but something like step.get(arg).argc() which return 2 or 3.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/compiler-core/src/bytecode.rs (1)
1153-1193:BuildSliceArgCount+OpArgTypecorrectly restricts op_arg to {2, 3}The enum plus
OpArgTypeimplementation ensures only2and3round‑trip:from_op_argrejects other values, andto_op_argalways emits2or3viaargc(), so invalid bytecode can’t be represented as a validArg<BuildSliceArgCount>. The separateargc()helper also avoids relying on enum discriminant values in stack analysis / VM code.If you don’t specifically need
NonZeroU8or const‑fn semantics here, you could simplify and drop theunsafeby returning a plainu8fromargc()and adjustingto_op_argaccordingly, e.g.:- pub const fn argc(self) -> NonZeroU8 { - let inner = match self { - Self::Two => 2, - Self::Three => 3, - }; - // Safety: `inner` can be either 2 or 3. - unsafe { NonZeroU8::new_unchecked(inner) } - } + pub const fn argc(self) -> u8 { + match self { + Self::Two => 2, + Self::Three => 3, + } + }- fn to_op_arg(self) -> u32 { - u32::from(self.argc().get()) - } + fn to_op_arg(self) -> u32 { + self.argc().into() + }This is purely optional and doesn’t affect correctness. As per coding guidelines, please also ensure
cargo fmtandcargo clippyare clean after these changes.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/compiler-core/src/bytecode.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto 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/compiler-core/src/bytecode.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). (11)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run tests under miri
- 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 snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
🔇 Additional comments (4)
crates/compiler-core/src/bytecode.rs (4)
13-13: NonZeroU8 import matches new argc helper usageAdding
num::NonZeroU8here is appropriate forBuildSliceArgCount::argc()and keeps the dependency localized to this module.
762-764: UsingArg<BuildSliceArgCount>forBuildSlicetightens the type contractSwitching
BuildSliceto carryargc: Arg<BuildSliceArgCount>makes the opcode’s argument domain explicit and enforces2or3at the type level viaOpArgType, aligning with the PR’s goal of rejecting other values.
1591-1595:stack_effectforBuildSlicenow derives from validated argc
1 - (argc.get(arg).argc().get() as i32)yields-1forTwo(2‑arg slice) and-2forThree(3‑arg slice), preserving previous stack behavior while now depending on the enum‑validated argument count instead of a loosely interpreted value.
1782-1783: Disassembly showsBuildSliceArgCountexplicitlyPrinting
BuildSlice { argc }with?argcwill renderBuildSlice(Two)/BuildSlice(Three), which is clearer in disassembly than a raw integer or boolean and matches the new enum.
2 participants
We are currently only checking if the argument count is 3, meaning that even if we pass it
-1,0or999it will act as if2was passed into it.This change ensures that the oparg for
BuildSlicecan be either2or3Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.