Renewd DictUpdate instruction#6085
Conversation
Sorry, something went wrong.
WalkthroughThe changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant Compiler
participant Bytecode
participant VM
participant Stack
participant PyDict
Compiler->>Bytecode: Emit DictUpdate { index }
Bytecode->>VM: Execute DictUpdate { index }
VM->>Stack: Pop source mapping
VM->>Stack: Access target dict at TOS-(index-1)
VM->>PyDict: Merge items from source mapping
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
compiler/codegen/src/compile.rs (1)
3588-3591: Stale comment references old DictUpdate APIThe commented-out
compile_pattern_mappingstub still showsDictUpdate { size: 2 }. When reviving this code, update it to the newindexform to match the VM semantics and the change made above.vm/src/frame.rs (2)
808-821: Simplify index-to-depth computation (avoid branch, clarify intent)Use saturating_sub to derive the depth after popping the source. This removes branching and cleanly supports idx==0 or 1 mapping to TOS.
- let idx = index.get(arg); - - // Pop the source from TOS - let source = self.pop_value(); - - // Get the dict to update (it's now at TOS-(i-1) after popping source) - let dict = if idx <= 1 { - // DICT_UPDATE 0 or 1: dict is at TOS (after popping source) - self.top_value() - } else { - // DICT_UPDATE n: dict is at TOS-(n-1) - self.nth_value(idx - 1) - }; + let idx = index.get(arg); + // Pop the source from TOS + let source = self.pop_value(); + // After popping, dict is at depth idx.saturating_sub(1): + // idx in {0,1} => depth 0 (TOS); idx >= 2 => depth (idx-1) + let depth = idx.saturating_sub(1); + let dict = self.nth_value(depth);
803-821: Document/align index semantics with bytecode specCurrent code treats idx==0 and idx==1 equivalently (dict at TOS after pop). If the bytecode spec is 1-based (compiler currently emits 1), consider:
- Explicitly documenting the valid range and meaning in the instruction definition, or
- Asserting expected range in debug builds for early detection of emitter/VM mismatches.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
compiler/codegen/src/compile.rs(1 hunks)compiler/core/src/bytecode.rs(3 hunks)vm/src/frame.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style (cargo fmtto format)
Always run clippy to lint code (cargo clippy) before completing tasks. Fix any warnings or lints that are introduced by your 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:
compiler/core/src/bytecode.rscompiler/codegen/src/compile.rsvm/src/frame.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). (10)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Check Rust code with rustfmt and 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 (5)
compiler/codegen/src/compile.rs (2)
4399-4402: DictUpdate index=1 matches the stack shape; LGTMAt this point TOS is the mapping (from
**expr) and the target dict is one below it. Usingindex: 1aligns with the updated bytecode and VM semantics.
4399-4402: No remaining oldDictUpdateusages foundRan ripgrep across the repo and only discovered a single commented example of the old
{ size: … }form (in compiler/codegen/src/compile.rs), so there are no active references to remove or replace.compiler/core/src/bytecode.rs (2)
1402-1402: Stack effect -1 looks correctPopping only the source mapping while keeping the target dict on the stack matches the described VM behavior. No changes needed.
1591-1592: Disassembly formatting is consistentPrinting
DictUpdate(index)matches formatting used by similar instructions (e.g.,CopyItem,Swap). Looks good.vm/src/frame.rs (1)
836-837: LGTM on merge semanticsUsing dict.merge_object(source, vm) delegates to dict’s merge logic and preserves mapping semantics.
Sorry, something went wrong.
youknowone
merged commit a9a9e3b
into
RustPython:main
Aug 8, 2025
Summary by CodeRabbit
New Features
Bug Fixes
Refactor