◐ Shell
clean mode source ↗

LoadClassDeref -> LoadFromDictOrDeref by ShaharNaveh · Pull Request #6692 · RustPython/RustPython

ShaharNaveh

📝 Walkthrough

Walkthrough

This PR removes the LoadClassDeref opcode instruction and replaces all occurrences with LoadFromDictOrDeref across the compiler and VM. The functionality remains unchanged; this is a consolidation replacing a specialized instruction variant with a more general one.

Changes

Cohort / File(s) Summary
Instruction Definition & Bytecode
crates/compiler-core/src/bytecode.rs		 Removed LoadClassDeref(Arg<NameIdx>) = 132 enum variant. Updated TryFrom decoding, stack effect handling, and disassembly formatting to reference LoadFromDictOrDeref instead.
Instruction Emission
crates/codegen/src/compile.rs		 Changed class-scope name loading to emit Instruction::LoadFromDictOrDeref instead of Instruction::LoadClassDeref.
Instruction Execution
crates/vm/src/frame.rs		 Updated ExecutingFrame instruction handler to match on LoadFromDictOrDeref instead of LoadClassDeref; internal logic unchanged.
Opcode Classification
crates/stdlib/src/opcode.rs		 Updated Opcode::has_free to check for LoadFromDictOrDeref instead of LoadClassDeref.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

Suggested reviewers

  • youknowone

Poem

🐰 A class deref bows out with grace,
LoadFromDict takes its place,
One instruction now, instead of two,
The bytecode's cleaner—hooray, it's true! ✨

🚥 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 directly and accurately describes the main change across all modified files: replacing the LoadClassDeref instruction variant with LoadFromDictOrDeref throughout the codebase.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% 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
📜 Recent 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 9e225f5 and 07e7f46.

⛔ Files ignored due to path filters (1)
  • Lib/_opcode_metadata.py is excluded by !Lib/**
📒 Files selected for processing (4)
  • crates/codegen/src/compile.rs
  • crates/compiler-core/src/bytecode.rs
  • crates/stdlib/src/opcode.rs
  • crates/vm/src/frame.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
  • crates/vm/src/frame.rs
  • crates/codegen/src/compile.rs
  • 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). (3)
  • GitHub Check: Run snippets and cpython tests (macos-latest)
  • GitHub Check: Run snippets and cpython tests (ubuntu-latest)
  • GitHub Check: Run snippets and cpython tests (windows-2025)
🔇 Additional comments (6)
crates/compiler-core/src/bytecode.rs (3)

845-846: Opcode/API change looks consistent; consider bytecode backwards-compat expectations.

Line 845: Adding Instruction::LoadFromDictOrDeref(Arg<NameIdx>) = 89 aligns with the CPython opcode space and matches the PR objective, but removing the old LoadClassDeref opcode means any previously-serialized RustPython bytecode containing that opcode will now fail to decode (InvalidBytecode). If RustPython ever persists bytecode between runs, consider a one-release decode shim mapping the old opcode value to LoadFromDictOrDeref during unmarshalling.

1806-1814: Stack effect update for LoadFromDictOrDeref is correct.

Line 1813: LoadFromDictOrDeref(_) => 1 matches “pushes one value” and keeps stack-depth computation consistent with the VM implementation.

2123-2124: Disassembly output updated correctly.

Line 2123: Printing as LOAD_FROM_DICT_OR_DEREF and using cell_name formatting matches the opcode’s cell/free indexing semantics.

crates/stdlib/src/opcode.rs (1)

104-116: has_free() correctly accounts for LoadFromDictOrDeref.

Line 111: Treating Instruction::LoadFromDictOrDeref(_) as using co_freevars/co_cellvars is consistent with the opcode’s operand being an index into the cell/free table.

crates/vm/src/frame.rs (1)

1115-1130: VM execution path matches dict-or-deref semantics; add/confirm a regression test for shadowing.

Lines 1115-1129: The implementation does “locals dict lookup first, then cell/free fallback”, which matches the intended behavior for class-scope name resolution. I’d add/confirm a test that covers shadowing (class namespace key present vs missing) and the unbound-cell path to ensure behavior didn’t subtly drift during the rename.

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

1560-1568: The replacement is a valid drop-in substitute; all concerns are verified.

Both LoadDeref and LoadFromDictOrDeref use identical operand type (Arg<NameIdx>) and stack effect (1). The VM handler correctly implements class scope semantics by checking the locals dict first via self.locals.mapping().subscript(name, vm), then falling back to the closure cell/free variable. No references to the old LoadClassDeref remain, and the new instruction is properly integrated in opcode tables and bytecode definitions across the codebase.

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.