Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

EnsureBuildSlice oparg to be either 2 or 3#6313

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
youknowone merged 4 commits intoRustPython:mainfromShaharNaveh:build-slice-oparg
Nov 30, 2025

Conversation

@ShaharNaveh
Copy link
Collaborator

@ShaharNavehShaharNaveh commentedNov 29, 2025•
edited by coderabbitaibot
Loading

We are currently only checking if the argument count is 3, meaning that even if we pass it-1,0 or999 it will act as if2 was passed into it.

This change ensures that the oparg forBuildSlice can be either2 or3

Summary by CodeRabbit

  • Refactor
    • Slice construction now encodes whether a slice has two or three arguments (instead of a boolean "step" flag), unifying compile-time and runtime handling.
    • Bytecode and execution paths updated to use this explicit argument-count form; behavior is preserved while improving consistency and clarity.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitaibot commentedNov 29, 2025•
edited
Loading

Walkthrough

Replace the booleanstep BuildSlice argument with aBuildSliceArgCount enum (Two/Three) across bytecode, codegen, and VM, and update compile_slice/subscript emission and VM execution to use argc-based slice construction.

Changes

Cohort / File(s)Change Summary
Bytecode instruction definition
crates/compiler-core/src/bytecode.rs		Added publicBuildSliceArgCount enum (Two = 2,Three = 3), implementedOpArgType andargc() helper; replacedInstruction::BuildSlice fieldstep: Arg<bool> withargc: Arg<BuildSliceArgCount>; updated stack-effect, display/marshal, and op-arg handling to useargc.
Compiler codegen
crates/codegen/src/compile.rs		ImportedBuildSliceArgCount; changedfn compile_slice(...) return type toCompileResult<BuildSliceArgCount>; updated slice and subscript emission to compute and emitBuildSlice { argc: BuildSliceArgCount::Two/Three } instead of using a booleanstep.
VM execution / Frame
crates/vm/src/frame.rs		execute_build_slice now acceptsargc: BuildSliceArgCount; call sites passargc.get(arg); VM derives whether to pop a step value fromargc (Two → no step popped, Three → pop step) and adjusts pops accordingly when constructing slices.

Sequence Diagram(s)

mermaid
sequenceDiagram
participant Compiler as Compiler (codegen)
participant Bytecode as Bytecode (instruction stream)
participant VM as VM (frame executor)

Compiler->>Bytecode: emit BuildSlice { argc: Two/Three }
Note over Bytecode: argc encoded as BuildSliceArgCount
VM->>Bytecode: fetch Instruction::BuildSlice(argc)
VM->>VM: argc.get(arg) → determine pop count
alt argc == Three
VM->>VM: pop stop, pop step, pop start
else argc == Two
VM->>VM: pop stop, pop start
end
VM->>VM: construct slice object and push result

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Verify stack-effect calculation incrates/compiler-core/src/bytecode.rs usesargc.get(arg).argc().get() correctly for both variants.
  • Check all callers ofcompile_slice incrates/codegen/src/compile.rs to ensureBuildSliceArgCount values propagate and emission matches CPython ordering.
  • Confirmexecute_build_slice incrates/vm/src/frame.rs pops values in the correct order and handles optional step only whenThree.

Possibly related PRs

  • compile_subscript #6008 — modifies compile-side slice/subscript emission; strongly related to changing compile logic and BuildSlice emission.

Poem

🐰 Two or three I softly count,
I hop through code and bytes amount.
No boolean step to slow my pace,
I tuck argc in the slice's place.
A tiny leap — a cleaner trace.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check nameStatusExplanation
Description Check✅ PassedCheck skipped - CodeRabbit’s high-level summary is enabled.
Title check✅ PassedThe title directly and concisely describes the main change: restricting BuildSlice oparg values to 2 or 3, which aligns with the primary objective of the PR.
Docstring Coverage✅ PassedDocstring 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 usingCodeRabbit! 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.

Copy link
Contributor

@coderabbitaicoderabbitaibot left a 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)

451-477:Two-element slice fast path usesBuildSliceArgCount correctly, but return type could be tightened

Theis_two_element_slice guard pluscompile_slice guaranteesn is always2, soBuildSliceArgCount::from_op_arg(n).unwrap() is safe. If you want to make this more self-documenting and future-proof, you could instead:

  • Changecompile_slice to returnBuildSliceArgCount (and maybe not expose rawu32 here), or
  • InlineBuildSliceArgCount::Two at this callsite and treatcompile_slice as “emit two bounds, no step” for this fast path.

Not blocking, but would reduce reliance on the implicit 2/3 contract ofcompile_slice.

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between6782fa2 and29ee656.

📒 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 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/vm/src/frame.rs
  • crates/compiler-core/src/bytecode.rs
  • crates/codegen/src/compile.rs
🧠 Learnings (1)
📚 Learning: 2025-11-29T12:17:28.583Z
Learnt from: CRRepo: RustPython/RustPython PR: 0File: .github/copilot-instructions.md:0-0Timestamp: 2025-11-29T12:17:28.583ZLearning: 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 fromstep: bool toargc: BuildSliceArgCount strengthens 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 fromstep: bool toargc: BuildSliceArgCount makes 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 restrictBuildSlice oparg to exactly 2 or 3 arguments.

crates/codegen/src/compile.rs (1)

4585-4604:Expr::Slice emission now encodes argc explicitly and matches stack effect

The newargc computation based onstep presence 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:BuildSliceArgCount enum correctly constrains allowedBUILD_SLICE arg counts

DefiningBuildSliceArgCount with repr(u8) and values{ Two = 2, Three = 3 } viaop_arg_enum! ensures that only opargs 2 or 3 round-trip throughOpArgType andArg<T>. This is a clean way to encode the CPython-style 2/3 argument semantics forBUILD_SLICE and directly supports the PR’s goal of rejecting other arg counts.

762-764:Instruction::BuildSlice wiring (encoding, stack effect, display) is consistent; consider compatibility expectations

SwitchingBuildSlice toargc: Arg<BuildSliceArgCount> and computing the stack effect as1 - argc keeps 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., legacy0/1 encodings or corrupt bytecode) will now causeArg::get →BuildSliceArgCount::from_op_arg to returnNone and 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 encodeBUILD_SLICE with argument counts 2 or 3, or that such bytecode is rejected before execution.

Also applies to: 1565-1569, 1756-1756

Copy link
Contributor

@coderabbitaicoderabbitaibot left a 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)

396-427:compile_slice’sBuildSliceArgCount return type aligns withBuildSlice constraints

The function now compiles lower/upper bounds, conditionally compilesstep, and returnsBuildSliceArgCount::{Two,Three}, so emittedargc can 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 theThree arm 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

📥 Commits

Reviewing files that changed from the base of the PR and between29ee656 and759195a.

📒 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 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
🧠 Learnings (1)
📚 Learning: 2025-11-29T12:17:28.583Z
Learnt from: CRRepo: RustPython/RustPython PR: 0File: .github/copilot-instructions.md:0-0Timestamp: 2025-11-29T12:17:28.583ZLearning: 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:ImportingBuildSliceArgCount is consistent and necessary

The addedBuildSliceArgCount import matches its later usage inInstruction::BuildSlice and keeps the bytecode-related imports grouped coherently.

451-466:Two‑element subscript slice fast path correctly wiresargc intoBuildSlice

Derivingargc viacompile_slice and feeding it intoInstruction::BuildSlice { argc } keeps the stack shape as[value, slice] and guarantees the oparg is constrained to the enum (effectivelyTwo here, sinceis_two_element_slice enforcesstep.is_none()). The separation between Load/Store (using the fast path) and Del (falling back to genericSubscript/DeleteSubscript) remains intact.

4576-4596:Expr::Slice emission now usesBuildSliceArgCount with correct stack arity

Thecompile_bound helper ensures lower/upper each contribute exactly one value (orNone), the optionalstep adds one more when present, andargc is then chosen asTwo vsThree to match. This guaranteesInstruction::BuildSlice { argc } never sees an invalid argument count and satisfies the “only 2 or 3” oparg requirement.

@ShaharNavehShaharNaveh changed the titleForceBuildSlice oparg to be either 2 or 3EnsureBuildSlice oparg to be either 2 or 3Nov 29, 2025
Comment on lines 1158 to 1165
/// ```py
/// x[5:10]
/// ```
Two =2,
/// ```py
/// x[5:10:2]
/// ```
Three =3,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// ```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

implBuildSliceArgCount{constfnargc(&self) ->usize{        ....}}

Copy link
CollaboratorAuthor

Choose a reason for hiding this comment

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

Hmmm, but then how this be validated atstack_effect?

Copy link
Member

Choose a reason for hiding this comment

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

Not by usingas cast, but something likestep.get(arg).argc() which return 2 or 3.

ShaharNaveh reacted with thumbs up emoji
Copy link
Contributor

@coderabbitaicoderabbitaibot left a 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/compiler-core/src/bytecode.rs (1)

1153-1193:BuildSliceArgCount +OpArgType correctly restricts op_arg to {2, 3}

The enum plusOpArgType implementation ensures only2 and3 round‑trip:from_op_arg rejects other values, andto_op_arg always emits2 or3 viaargc(), 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 needNonZeroU8 or const‑fn semantics here, you could simplify and drop theunsafe by returning a plainu8 fromargc() and adjustingto_op_arg accordingly, 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 ensurecargo fmt andcargo clippy are clean after these changes.

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between759195a and5cfd018.

📒 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 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/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 usage

Addingnum::NonZeroU8 here is appropriate forBuildSliceArgCount::argc() and keeps the dependency localized to this module.

762-764:UsingArg<BuildSliceArgCount> forBuildSlice tightens the type contract

SwitchingBuildSlice to carryargc: Arg<BuildSliceArgCount> makes the opcode’s argument domain explicit and enforces2 or3 at the type level viaOpArgType, aligning with the PR’s goal of rejecting other values.

1591-1595:stack_effect forBuildSlice now derives from validated argc

1 - (argc.get(arg).argc().get() as i32) yields-1 forTwo (2‑arg slice) and-2 forThree (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 showsBuildSliceArgCount explicitly

PrintingBuildSlice { argc } with?argc will renderBuildSlice(Two)/BuildSlice(Three), which is clearer in disassembly than a raw integer or boolean and matches the new enum.

Copy link
Member

@youknowoneyouknowone left a comment

Choose a reason for hiding this comment

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

👍

@youknowoneyouknowone merged commit4051bec intoRustPython:mainNov 30, 2025
13 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@coderabbitaicoderabbitai[bot]coderabbitai[bot] left review comments

@youknowoneyouknowoneyouknowone approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ShaharNaveh@youknowone
[8]ページ先頭
©2009-2025 Movatter.jp