New Instruction ToBool,PopJumpIfFalse#6112
Conversation
Sorry, something went wrong.
WalkthroughRenames conditional-jump variants to PopJumpIfTrue/PopJumpIfFalse, adds a unit Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant BC as Bytecode
participant Frame as ExecutingFrame
participant Stack
participant VM as VM/Runtime
rect rgba(230,240,255,0.6)
note over BC,Frame: ToBool execution
Frame->>Stack: pop value
Frame->>VM: try_to_bool(value)
VM-->>Frame: bool_obj
Frame->>Stack: push bool_obj
end
rect rgba(240,230,255,0.6)
note over BC,Frame: PopJumpIfTrue / PopJumpIfFalse execution
Frame->>Stack: pop value
Frame->>VM: try_to_bool(value)
VM-->>Frame: bool_result
alt pop result matches jump condition
Frame->>Frame: jump to target label
else
Frame->>Frame: continue (value discarded)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
✨ 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/Issue comments)Type Other keywords and placeholders
Status, 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: 1
🧹 Nitpick comments (2)
vm/src/frame.rs (2)
1240-1242: Avoid duplicating jump logic; reuse existing jump_if here.PopJumpIfFalse currently calls pop_jump_if, which is identical to jump_if. You can dispatch directly to jump_if to reduce duplication and keep all “pop-and-jump” semantics in one place.
Apply this diff to the match arm:
- bytecode::Instruction::PopJumpIfFalse { target } => { - self.pop_jump_if(vm, target.get(arg), false) - } + bytecode::Instruction::PopJumpIfFalse { target } => + self.jump_if(vm, target.get(arg), false),
1877-1893: pop_jump_if duplicates jump_if; consider removing or documenting divergence intent.The implementation is bit-for-bit identical to jump_if (pop TOS, to-bool, jump on flag). Unless you plan to diverge semantics later, remove it to prevent drift.
Apply this diff to remove the helper:
- #[inline] - fn pop_jump_if( - &mut self, - vm: &VirtualMachine, - target: bytecode::Label, - flag: bool, - ) -> FrameResult { - // PopJumpIf{True,False} - // Pop TOS, convert to bool, jump if matches flag - let obj = self.pop_value(); - let value = obj.try_to_bool(vm)?; - if value == flag { - self.jump(target); - } - // If not jumping, the value is already popped - Ok(None) - }If you prefer keeping both for readability, add a brief comment in pop_jump_if stating it intentionally mirrors jump_if today.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
compiler/core/src/bytecode.rs(7 hunks)vm/src/frame.rs(3 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:
vm/src/frame.rscompiler/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). (9)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests on wasm-wasi
🔇 Additional comments (7)
vm/src/frame.rs (1)
1086-1091: ToBool execution path looks correct (pop -> truthy -> push bool).Net stack effect is 0 and consistent with other truthiness sites (e.g., UnaryOperator::Not, jump_if). No issues spotted.
compiler/core/src/bytecode.rs (6)
528-528: Re-enabling ToBool in the enum is fine.Matches VM support and printing paths; no API fallout.
1264-1264: Including PopJumpIfFalse in label_arg is correct.Ensures disassembler/CFG find jump targets.
1338-1338: ToBool stack effect = 0 is consistent with VM.Good alignment with execution (pop + push).
1349-1349: PopJumpIfFalse stack effect = -1 matches VM’s single-pop semantics.If you adopt Option B (two-pop), remember to update this to -2.
1541-1541: Disassembly formatting for ToBool is wired correctly.No issues.
1557-1557: Disassembly formatting for PopJumpIfFalse is wired correctly.Will display target like other jumps.
Sorry, something went wrong.
youknowone
force-pushed
the
instruction
branch
from
847482e to
75a138a
Compare
August 26, 2025 06:44
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
compiler/core/src/bytecode.rs (1)
461-471: Ensure bytecode changes are accompanied by a marshal‐format bumpIt looks like you’ve inserted
ToBoolinto the middle of theInstructionenum without updating the marshal protocol version or assigning explicit discriminants. BecauseInstructionis#[repr(u8)]and we transmute incoming bytes up toLAST_INSTRUCTION, adding a new variant shifts all downstream opcodes, breaking any previously serialized bytecode.Please address this before landing:
• compiler/core/src/bytecode.rs
– You’ve added
rust ToBool,
mid‐enum, which shifts all subsequent discriminants. Verify thatLAST_INSTRUCTIONremains pointing at the true final variant (currentlyExtendedArg), and consider assigning an explicit value toToBool(or to all new variants) to preserve existing opcode numbers if backward‐compatibility across marshal versions is required.• compiler/core/src/marshal.rs
– The marshal format version is still
rust pub const FORMAT_VERSION: u32 = 4;
Bump this to a new value (e.g.5) so that the VM rejects older bytecode rather than mis‐decoding it.
Suggested diff:
diff --- a/compiler/core/src/marshal.rs +++ b/compiler/core/src/marshal.rs @@ -4,7 +4,7 @@ pub const FORMAT_VERSION: u32 = 4; #[derive(Debug)] pub enum MarshalError { - /// Invalid Bytecode + /// Invalid Bytecode
becomes
diff pub const FORMAT_VERSION: u32 = 5;These steps will prevent subtle, hard‐to‐debug mismatches when loading marshaled code across versions.
♻️ Duplicate comments (1)
compiler/core/src/bytecode.rs (1)
551-556: Docstrings for PopJumpIfTrue/PopJumpIfFalse now match one-pop behaviorThis aligns the documentation with the VM implementation and stack_effect (-1). Thanks for fixing the earlier inconsistency.
🧹 Nitpick comments (4)
jit/src/instructions.rs (1)
291-303: PopJumpIfTrue: mirrors semantics correctly
- Pops the condition, converts to boolean, true → target, false → fallthrough. Matches POP_JUMP_IF_TRUE behavior.
- Fallthrough is established by switching to else_block post-branch.
Apply the same variable rename for consistency:
- let then_block = self.get_or_create_block(target.get(arg)); + let target_block = self.get_or_create_block(target.get(arg)); let else_block = self.builder.create_block(); self.builder .ins() - .brif(val, then_block, &[], else_block, &[]); + .brif(val, target_block, &[], else_block, &[]);vm/src/frame.rs (2)
1086-1091: ToBool runtime semantics are correct; consider avoiding pop/push with replace_topNet stack effect is 0 and matches bytecode::Instruction::ToBool. Minor micro-optimization: replace TOS in-place to avoid an extra pop/push pair.
Apply this diff:
bytecode::Instruction::ToBool => { - let obj = self.pop_value(); - let bool_val = obj.try_to_bool(vm)?; - self.push_value(vm.ctx.new_bool(bool_val).into()); + let tos_bool = self.top_value().to_owned().try_to_bool(vm)?; + self.replace_top(vm.ctx.new_bool(tos_bool).into()); Ok(None) }
712-720: Remove stale commented-out ToBool match armThis dead, commented block now duplicates the live ToBool case and can confuse future edits. Please remove it.
Apply this diff:
- // bytecode::Instruction::ToBool => { - // dbg!("Shouldn't be called outside of match statements for now") - // let value = self.pop_value(); - // // call __bool__ - // let result = value.try_to_bool(vm)?; - // self.push_value(vm.ctx.new_bool(result).into()); - // Ok(None) - // }compiler/core/src/bytecode.rs (1)
528-528: New ToBool opcode: placement and naming look fine; add a brief doc commentThe unit variant is added consistently, and downstream code handles it. Consider a short doc comment to clarify that it converts TOS to a concrete bool object and leaves it on TOS (stack effect 0).
Apply this diff:
- ToBool, + /// Convert TOS to a boolean (via __bool__ or __len__), push the bool object back (net 0). + ToBool,
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (8)
compiler/codegen/src/snapshots/rustpython_codegen__compile__tests__if_ands.snapis excluded by!**/*.snapcompiler/codegen/src/snapshots/rustpython_codegen__compile__tests__if_mixed.snapis excluded by!**/*.snapcompiler/codegen/src/snapshots/rustpython_codegen__compile__tests__if_ors.snapis excluded by!**/*.snapcompiler/codegen/src/snapshots/rustpython_codegen__compile__tests__nested_double_async_with.snapis excluded by!**/*.snapcompiler/codegen/src/snapshots/rustpython_compiler_core__compile__tests__if_ands.snapis excluded by!**/*.snapcompiler/codegen/src/snapshots/rustpython_compiler_core__compile__tests__if_mixed.snapis excluded by!**/*.snapcompiler/codegen/src/snapshots/rustpython_compiler_core__compile__tests__if_ors.snapis excluded by!**/*.snapcompiler/codegen/src/snapshots/rustpython_compiler_core__compile__tests__nested_double_async_with.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
compiler/codegen/src/compile.rs(3 hunks)compiler/core/src/bytecode.rs(7 hunks)jit/src/instructions.rs(2 hunks)vm/src/frame.rs(3 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/codegen/src/compile.rsvm/src/frame.rsjit/src/instructions.rscompiler/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 on wasm-wasi
- GitHub Check: Check the WASM package and demo
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run tests under miri
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
🔇 Additional comments (10)
jit/src/instructions.rs (1)
278-290: PopJumpIfFalse logic is correct – no JIT‐side ToBool support neededVerified that the JIT compiler never actually emits
Instruction::ToBool(the sole reference incompiler/codegen/src/compile.rsis commented out), so adding a handler for it isn’t necessary at this time. The only remaining suggestion is the optional rename for clarity:• In
jit/src/instructions.rsat lines 278–290, renamethen_blocktotarget_block(and update its use in.brif) in both thePopJumpIfFalseandPopJumpIfTruearms:- let then_block = self.get_or_create_block(target.get(arg)); + let target_block = self.get_or_create_block(target.get(arg)); let else_block = self.builder.create_block(); self.builder - .ins() - .brif(val, else_block, &[], then_block, &[]); + .ins() + .brif(val, else_block, &[], target_block, &[]); …compiler/codegen/src/compile.rs (3)
2029-2033: try/except: switching to PopJumpIfFalse fixes stack disciplineUsing PopJumpIfFalse after ExceptionMatch ensures the temporary boolean is consumed on either path. This prevents a stray bool on the success path before binding “as name” or popping the exception. Good alignment with the new POP_JUMP* contract.
Please double-check that TestOperation::ExceptionMatch leaves only the original exception and a single boolean on stack before this jump; no other sites in this handler depend on the boolean staying on the stack.
3119-3123: Pattern engine: PopJumpIfFalse is the right choice for fail pathsAdopting PopJumpIfFalse in jump_to_fail_pop is correct: failure consumes the condition result before transferring to the cleanup label, which simplifies the required pops at the failure targets.
4399-4411: compile_jump_if: updated to PopJumpIfTrue/False—confirm no callers expect the test value to remainConverting both branches to POP_JUMP variants is the right move and matches the new bytecode. Since POP_JUMP* consumes the test, ensure all call sites (if/elif/while, assert, comp generators) do not rely on the test value remaining on the stack. A quick scan of the surrounding control-flow builders suggests they do not.
If you want a safety net, run snapshot tests around:
- if/elif/else nesting
- while with complex expressions
- assert with and without message
Existing insta snapshots (e.g., test_if_ors/ands/mixed) are good coverage; adding one for while would be helpful.
vm/src/frame.rs (2)
1850-1855: Helper pop_jump_if(): clear factoring; semantics match CPython truthinessGood consolidation. It pops once, converts via try_to_bool, and conditionally jumps—exactly what PopJumpIf{True,False} require. No issues spotted.
1236-1241: All staleJumpIfTrue/JumpIfFalsevariants have been removedVerification via ripgrep confirms zero occurrences of the old
JumpIfTrueorJumpIfFalsevariants in the repository. The newPopJumpIfTrue/PopJumpIfFalseinstructions are consistently wired across all relevant crates:
- vm/src/frame.rs: dispatch cases for
PopJumpIfTrueandPopJumpIfFalse(lines 1236–1241)- jit/src/instructions.rs: implementations for both variants (lines 278–293)
- compiler/codegen/src/compile.rs: emission and
jump_to_fail_popcalls usingPopJumpIfTrue/PopJumpIfFalsethroughout control‐flow compilation- compiler/core/src/bytecode.rs: enum definitions and matching arms exclusively reference
PopJumpIfTrue,PopJumpIfFalse,JumpIfTrueOrPop, andJumpIfFalseOrPop- Snapshot files under both
compiler/codegenandcompiler/coreconfirm test coverage uses only the new variantsNo residual references to the old
JumpIfTrue/JumpIfFalseremain.compiler/core/src/bytecode.rs (4)
1257-1260: Include PopJumpIf{True,False} in label_arg()Label extraction updated appropriately; keeps disassembly and control-flow analysis correct.
1334-1334: Stack effect for ToBool = 0 is correctRuntime pops and pushes a bool object back; net zero. Matches VM implementation.
1345-1345: Stack effect for PopJumpIf{True,False} = -1 is correctMatches single-pop branching semantics used by vm::frame::ExecutingFrame::pop_jump_if.
1536-1536: Disassembly formatting covers ToBool and PopJumpIf{True,False}fmt_dis outputs are wired; no issues.
Also applies to: 1549-1551
Sorry, something went wrong.
youknowone
merged commit 776cabb
into
RustPython:main
Aug 26, 2025
Summary by CodeRabbit
New Features
Chores