Renewd DictUpdate instruction #6085
Conversation
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
|
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.
| DictUpdate { | ||
| index: Arg<u32>, | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Clarify DictUpdate index semantics and verify call sites
Good move making DictUpdate carry index: Arg<u32>; naming aligns with CopyItem/Swap.
Please add a short doc comment clarifying:
- What
indexis relative to (TOS before pop vs. after popping the source mapping). - Whether it’s a 0-based offset and any bounds expectations.
Also, since this changes the bytecode argument shape, ensure marshaled bytecode compatibility is addressed (e.g., version bump/invalidation or explicit handling when index == 0 to preserve legacy behavior).
Run this to find any stale patterns still matching/constructing the unit variant form:
I can draft the doc comment and add unit tests asserting stack effect and disassembly output once you confirm the intended “before/after pop” convention.
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "All occurrences of DictUpdate with context (sanity check):"
rg -n --hidden --glob '!target' -A3 -B3 '\bDictUpdate\b'
echo
echo "Likely pattern matches not using struct form (manual review recommended):"
rg -n --hidden --glob '!target' -e '\bDictUpdate\s*=>' -e '\bInstruction::DictUpdate\s*[),]|,$'
echo
echo "Constructors not using struct form (look for missing braces):"
rg -n --hidden --glob '!target' -e '\bInstruction::DictUpdate\b(?!\s*\{)' -ULength of output: 319276
Add documentation for DictUpdate index semantics & ensure marshaling compatibility
Please update compiler/core/src/bytecode.rs around the DictUpdate variant (lines 667–672) as follows:
- Insert a doc comment explaining that
indexis a 0-based offset to the dict on the stack before popping the source mapping. For example:/// Pop a source mapping from the top of the stack and update /// the dict located `index` slots below it. `index` is a 0-based /// offset relative to the top-of-stack *before* the pop. DictUpdate { index: Arg<u32>, },
- Note in the doc that legacy bytecode (with no
indexfield) will decode withindex == 0; if you intend to break compatibility, bump the bytecode version or handleindex == 0explicitly in tests.
Next steps:
- No stale unit-variant constructors were found (grep output shows only struct-form usage), so no further code changes required there.
- Add unit tests in the bytecode test module asserting:
Instruction::DictUpdate { index }yieldsstack_effect() == -1fmt_dis()printsDictUpdate { index }
- Add a marshaling test that serializes an old-format
DictUpdate(without arg) and ensures it decodes toDictUpdate { index: 0 }.
🤖 Prompt for AI Agents
In compiler/core/src/bytecode.rs around lines 667 to 672, add a doc comment to
the DictUpdate variant explaining that the index field is a 0-based offset to
the dict on the stack before popping the source mapping, clarifying the
semantics as described. Also note in the doc that legacy bytecode without the
index field decodes with index == 0 and mention the need to handle compatibility
accordingly. Then, add unit tests in the bytecode test module to assert that
Instruction::DictUpdate { index } has a stack_effect() of -1 and that fmt_dis()
outputs the expected string. Finally, add a marshaling test that serializes an
old-format DictUpdate (without the index argument) and verifies it decodes to
DictUpdate { index: 0 }.
| if vm | ||
| .get_method(other.clone(), vm.ctx.intern_str("keys")) | ||
| .get_method(source.clone(), vm.ctx.intern_str("keys")) | ||
| .is_none() | ||
| { | ||
| return Err(vm.new_type_error(format!( | ||
| "'{}' object is not a mapping", | ||
| other.class().name() | ||
| source.class().name() | ||
| ))); |
There was a problem hiding this comment.
Don't mask attribute lookup errors when checking mapping protocol
vm.get_method(...) returns Option<PyResult<_>>; using is_none() ignores Some(Err(e)) and turns it into a TypeError, hiding the real exception. Propagate lookup errors instead.
- if vm
- .get_method(source.clone(), vm.ctx.intern_str("keys"))
- .is_none()
- {
- return Err(vm.new_type_error(format!(
- "'{}' object is not a mapping",
- source.class().name()
- )));
- }
+ match vm.get_method(source.clone(), vm.ctx.intern_str("keys")) {
+ Some(Ok(_)) => {}
+ Some(Err(e)) => return Err(e),
+ None => {
+ return Err(vm.new_type_error(format!(
+ "'{}' object is not a mapping",
+ source.class().name()
+ )));
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if vm | |
| .get_method(other.clone(), vm.ctx.intern_str("keys")) | |
| .get_method(source.clone(), vm.ctx.intern_str("keys")) | |
| .is_none() | |
| { | |
| return Err(vm.new_type_error(format!( | |
| "'{}' object is not a mapping", | |
| other.class().name() | |
| source.class().name() | |
| ))); | |
| match vm.get_method(source.clone(), vm.ctx.intern_str("keys")) { | |
| Some(Ok(_)) => {} | |
| Some(Err(e)) => return Err(e), | |
| None => { | |
| return Err(vm.new_type_error(format!( | |
| "'{}' object is not a mapping", | |
| source.class().name() | |
| ))); | |
| } | |
| } |
🤖 Prompt for AI Agents
In vm/src/frame.rs around lines 826 to 833, the code uses is_none() on the
result of vm.get_method(...) which returns Option<PyResult<_>>, causing it to
ignore Some(Err(e)) and incorrectly convert it to a TypeError. To fix this,
handle the Option and PyResult separately: first check if the Option is None to
return the TypeError, but if it is Some, propagate any Err by returning it
instead of masking it.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor