Sort Instruction enum & related match arms
#6322
Conversation
WalkthroughThis PR performs a comprehensive restructuring of the bytecode instruction set across the compiler, JIT, and VM, reorganizing opcode variants alphabetically and updating instruction handlers to support an expanded set of operations including async/context-manager support, advanced data-structure construction, pattern matching, and improved control flow and exception handling. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
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 (3)
crates/jit/src/instructions.rs (1)
337-357: Incomplete handling of InplaceLshift and InplaceRshift in shift operations.The match pattern at line 337 only handles
BinaryOperator::Lshift | BinaryOperator::Rshift, but the inner conditional at line 350 checks forBinaryOperator::Lshift | BinaryOperator::InplaceLshift. This means ifInplaceLshiftorInplaceRshiftis passed, the outer match won't catch it, leading toNotSupportederror.Apply this diff to handle the inplace variants consistently:
( - BinaryOperator::Lshift | BinaryOperator::Rshift, + BinaryOperator::Lshift | BinaryOperator::InplaceLshift + | BinaryOperator::Rshift | BinaryOperator::InplaceRshift, JitValue::Int(a), JitValue::Int(b), ) => {crates/vm/src/frame.rs (1)
2087-2114: Unsafe pointer manipulation in execute_set_function_attribute.The unsafe block at lines 2108-2111 casts away const-ness and mutates the function payload. While the comment claims this is safe because
SetFunctionAttributealways followsMakeFunction, this pattern is fragile and could lead to UB if the invariant is violated.Consider documenting this invariant more prominently or using a safer pattern like
RefCellif the performance cost is acceptable.crates/compiler-core/src/bytecode.rs (1)
1727-1729: MatchKeys stack effect comment appears incorrect.The comment says "Pop 2 (subject, keys), push 3 (subject, keys_or_none, values_or_none)" but the actual effect is
1, which would mean net +1 to the stack. Looking at the VM implementation (frame.rs lines 1227-1293),MatchKeysdoesn't pop subject and keys - it reads them and pushes one value (either values tuple or None). The stack effect of1is correct, but the comment is misleading.Update the comment to accurately reflect the stack behavior:
- MatchKeys => 1, // Pop 2 (subject, keys), push 3 (subject, keys_or_none, values_or_none) + MatchKeys => 1, // Read subject and keys from stack (no pop), push values tuple or None
🧹 Nitpick comments (1)
crates/jit/src/instructions.rs (1)
586-589: Resume instruction is stubbed with TODO.The
Resumeinstruction handler is effectively a no-op. While acceptable for an initial implementation, this should be tracked for future completion.Would you like me to open an issue to track the completion of the
Resumeinstruction implementation in the JIT?
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/compiler-core/src/bytecode.rs(3 hunks)crates/jit/src/instructions.rs(2 hunks)crates/vm/src/frame.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint Rust 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/vm/src/frame.rscrates/jit/src/instructions.rscrates/compiler-core/src/bytecode.rs
🧬 Code graph analysis (1)
crates/vm/src/frame.rs (1)
crates/vm/src/stdlib/builtins.rs (1)
__build_class__(893-1062)
⏰ 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). (1)
- GitHub Check: Run snippets and cpython tests (windows-latest)
🔇 Additional comments (16)
crates/jit/src/instructions.rs (4)
556-559: PopBlock instruction is stubbed with TODO.Similar to Resume,
PopBlockneeds proper block management implementation for complete JIT support.
439-443: BuildTuple implementation looks correct.The tuple building correctly pops elements from the stack and creates a
JitValue::Tuple.
464-517: CompareOperation implementation handles int/bool/float comparisons correctly.The implementation properly extends bools to i64 for comparison and uses appropriate integer/float comparison conditions.
641-655: UnpackSequence correctly validates tuple size.The implementation appropriately checks that the tuple size matches the expected size before unpacking, and reverses the elements before extending the stack to maintain proper order.
crates/vm/src/frame.rs (6)
534-558: BeforeAsyncWith implementation looks correct.The async context manager protocol is properly implemented: retrieves
__aenter__, invokes it, then gets__aexit__and pushes both onto the stack in the correct order.
1106-1225: MatchClass implementation is comprehensive.The pattern matching for class patterns correctly handles:
- Instance checking
__match_args__extraction for positional argumentsMATCH_SELFbehavior for built-in types- Keyword attribute extraction
- Proper error handling for missing attributes
1227-1294: MatchKeys implementation correctly uses.get()method pattern.The implementation properly follows CPython's approach using
map.get(key, dummy)for atomic key checking, which handlescollections.defaultdictand similar types correctly without triggering__missing__.
1067-1081: LoadMethod pushes three values: target, is_method flag, and func.This correctly implements the method loading pattern where:
targetis the object for bound methods (or None for attributes)is_methodboolean distinguishes bound methods from plain attributesfuncis the actual callableThe corresponding
execute_method_callat lines 1876-1896 correctly handles both cases.
808-819: EndAsyncFor correctly handles StopAsyncIteration.The implementation properly:
- Pops the exception
- Pops the except handler block
- Validates block level matches stack size
- Pops the async iterator
- Checks for
StopAsyncIterationand clears the exception stack
1460-1476: SWAP instruction implementation is correct.The implementation correctly matches CPython 3.11+ semantics. For
SWAP(n), the code calculatesj = len - index_val, which swaps TOS (at indexlen - 1) with the element at distancenbelow it (at indexlen - n). This implements the specified semantics whereSTACK[-n]andSTACK[-1]are exchanged.crates/compiler-core/src/bytecode.rs (6)
551-800: Instruction enum is alphabetically sorted.The enum variants from
BeforeAsyncWiththroughYieldValueappear to be properly sorted alphabetically, fulfilling the PR objective.
803-805: LAST_INSTRUCTION correctly updated to YieldValue.Since
YieldValueis the last variant alphabetically, this constant is correctly set for marshaling bounds checking.
618-620: ContainsOp uses tuple-style variant as intended.The conversion from named struct-style (
ContainsOp { invert: Arg<Invert> }) to tuple-style (ContainsOp(Arg<Invert>)) aligns with the PR objectives for simplifying enum variants.
1550-1569: label_arg correctly includes all jump instructions.All instructions that contain
Labelarguments are properly listed in this match, including the newly addedBreakandContinuevariants.
1800-1907: fmt_dis match arms are alphabetically sorted.The display formatting match arms are properly sorted to match the instruction enum ordering, maintaining consistency throughout the codebase.
1674-1676: SetupAnnotation, SetupLoop, SetupFinally, EnterFinally, EndFinally all have stack effect 0.These control flow setup instructions correctly report zero stack effect as they manage block state rather than stack values.
closes #6312
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.