Skip to content

Conversation

@ShaharNaveh
Copy link
Collaborator

@ShaharNaveh ShaharNaveh commented Jan 5, 2026

Summary by CodeRabbit

  • Refactor
    • Improved error handling for stack argument validation with more detailed error messages when type mismatches occur.
    • Simplified internal instruction validation logic for enhanced code maintainability.

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 5, 2026

📝 Walkthrough

Walkthrough

The opcode module's validity checks are simplified to rely on a unified validation approach, while expanding the set of recognized instructions in has_name and has_jump. Error handling for invalid stack arguments is improved with detailed type information, and boolean extraction is standardized.

Changes

Cohort / File(s) Change Summary
Opcode Validation & Instruction Set Expansion
crates/stdlib/src/opcode.rs
Refactored has_const, has_name, has_jump, has_free, and has_local to use unified Self::is_valid(opcode) checks. Expanded has_name to include ImportName, LoadAttr, LoadMethod, LoadNameAny, and StoreLocal. Expanded has_jump to cover JumpIfNotExcMatch, JumpIfTrueOrPop, and additional jump-like instructions. Improved stack_effect error handling to return detailed type error messages for non-integer oparg values. Replaced pattern-based boolean extraction with v.try_to_bool(vm). Removed unused PyBool and PyNone imports.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

🐰 A rabbit hops through opcodes clean,
Where jumps and names now widely seen,
With errors clear and checks so tight,
The code simplifies—what a sight!
No more redundant paths to chase,
Just unified validation's grace!

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Fix opcode args' is vague and does not clearly convey what specific issue is being fixed or what the main change accomplishes. Use a more descriptive title that explains the specific fix, such as 'Refactor opcode validation and improve stack effect error handling' or 'Simplify opcode checks and enhance type error messages'.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings

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.

Copy link
Contributor

@coderabbitai coderabbitai bot 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 (2)
crates/stdlib/src/opcode.rs (2)

56-132: Consider eliminating double conversion in has_* functions.

All the has_* functions (lines 56-132) call Self::is_valid(opcode) which internally performs Instruction::try_from(opcode as u8).is_ok(), then immediately call Instruction::try_from(opcode as u8) again in the matches! macro. This double conversion is inefficient.

Consider inlining the range check and performing a single conversion:

🔎 Example refactor for has_const
 pub fn has_const(opcode: i32) -> bool {
-    Self::is_valid(opcode)
-        && matches!(
-            Instruction::try_from(opcode as u8),
-            Ok(Instruction::LoadConst { .. } | Instruction::ReturnConst { .. })
-        )
+    (0..=255).contains(&opcode)
+        && matches!(
+            Instruction::try_from(opcode as u8),
+            Ok(Instruction::LoadConst { .. } | Instruction::ReturnConst { .. })
+        )
 }

Apply the same pattern to has_name, has_jump, has_free, and has_local.


158-176: Optional: Simplify redundant error handling for oparg.

After the fast_isinstance check on line 161 confirms the value is an int type, the downcast_ref on line 167 should always succeed. The ok_or_else on lines 168-173 provides defensive error handling with the same error message, but is likely redundant.

🔎 Simplified approach
 let oparg = args
     .oparg
     .map(|v| {
         if !v.fast_isinstance(vm.ctx.types.int_type) {
             return Err(vm.new_type_error(format!(
                 "'{}' object cannot be interpreted as an integer",
                 v.class().name()
             )));
         }
         v.downcast_ref::<PyInt>()
-            .ok_or_else(|| {
-                vm.new_type_error(format!(
-                    "'{}' object cannot be interpreted as an integer",
-                    v.class().name()
-                ))
-            })?
+            .expect("downcast should succeed after isinstance check")
             .try_to_primitive::<u32>(vm)
     })
     .unwrap_or(Ok(0))?;

Alternatively, keep the defensive check if there are VM edge cases where isinstance and downcast might diverge.

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 75838e5 and b2ca650.

📒 Files selected for processing (1)
  • crates/stdlib/src/opcode.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.rs: Follow the default rustfmt code style by running cargo fmt to format Rust code
Always run clippy to lint 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/stdlib/src/opcode.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 rust tests (macos-latest)
  • GitHub Check: Run rust tests (ubuntu-latest)
  • GitHub Check: Run snippets and cpython tests (ubuntu-latest)
  • GitHub Check: Check Rust code with clippy
  • GitHub Check: Run rust tests (windows-latest)
  • GitHub Check: Run snippets and cpython tests (windows-latest)
  • GitHub Check: Run snippets and cpython tests (macos-latest)
  • GitHub Check: Ensure compilation on various targets
  • GitHub Check: Check the WASM package and demo
  • GitHub Check: Run snippets and cpython tests on wasm-wasi
  • GitHub Check: Run tests under miri
🔇 Additional comments (3)
crates/stdlib/src/opcode.rs (3)

7-7: Import cleanup looks appropriate.

The removal of PyBool and PyNone aligns with the refactored boolean extraction logic in stack_effect that now uses try_to_bool.


178-185: Clean boolean extraction for jump parameter.

The use of try_to_bool(vm) with a clear error message is a clean, idiomatic approach that properly handles boolean conversion through the VM's standard method.


67-83: The expanded opcode lists in has_name and has_jump are correctly classified.

Verification against CPython 3.11 confirms:

  • has_name additions align with CPython's hasname opcodes: ImportName, LoadAttr, LoadMethod, LoadNameAny (LOAD_NAME), and StoreLocal (STORE_NAME) all use co_names in CPython.
  • has_jump additions are correctly classified: JumpIfTrueOrPop and Send match CPython's hasjrel opcodes, and JumpIfNotExcMatch is properly identified as a jump instruction.

All opcodes exist in the Instruction enum and are semantically aligned with their CPython counterparts despite different naming conventions.

Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

oops, thank you for catching

@youknowone youknowone merged commit 4af869b into RustPython:main Jan 5, 2026
13 checks passed
terryluan12 pushed a commit to terryluan12/RustPython that referenced this pull request Jan 5, 2026
@coderabbitai coderabbitai bot mentioned this pull request Jan 14, 2026
terryluan12 pushed a commit to terryluan12/RustPython that referenced this pull request Jan 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants