Reviewing files that changed from the base of the PR and between e5aec9d and 1c6a0fc.

• crates/codegen/src/compile.rs (114 hunks)
• crates/compiler-core/src/bytecode.rs (4 hunks)
• crates/vm/src/frame.rs (8 hunks)

• GitHub Check: Run rust tests (windows-latest)
• GitHub Check: Check the WASM package and demo
• GitHub Check: Run snippets and cpython tests (windows-latest)
• GitHub Check: Run snippets and cpython tests (macos-latest)
• GitHub Check: Run tests under miri
• GitHub Check: Check Rust code with clippy
• GitHub Check: Run snippets and cpython tests on wasm-wasi
• GitHub Check: Ensure compilation on various targets

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

238-251: Tuple-style emit! macro, EmitArg, and const-load helpers look correct

The new emit! arm for Instruction::$op(arg) correctly routes through emit_arg, and the EmitArg impls for OpArgType and ir::BlockIdx ensure that:

• data-bearing opcodes get their OpArg from OpArgMarker::new(...), and
• label-based opcodes (e.g. Jump, Break, Continue) store the label in InstructionInfo::target with a marker created via OpArgMarker::marker().

emit_load_const / emit_return_const and the emit_return_value peephole now construct and rewrite LoadConst(idx) / ReturnConst(idx) using the marker form, reusing the existing encoded OpArg under the hood. This matches the new enum shape and keeps indices and labels type-safe. Based on the provided marker() definition, this use of OpArgMarker::marker() as a zero-sized type marker is consistent.

Also applies to: 5091-5120, 5454-5475

---

430-487: BuildSlice refactor keeps 2-/3-argument slice semantics intact

In compile_subscript and the standalone slice expression, Instruction::BuildSlice(n == 3) and Instruction::BuildSlice(step) move from struct fields to tuple-style args while still using a boolean to distinguish 2 vs 3 stack operands. That matches CPython’s BINARY_SLICE/BUILD_SLICE contract and preserves the existing control flow around 2-element vs stepped slices.

Also applies to: 4390-4407

---

497-538: Collection-building opcodes (Build*, Unpack*) are converted mechanically and stay consistent

Across starunpack_helper, default-argument handling, dict construction, lambda defaults, gather_elements, and f-string assembly you’ve systematically switched:

• BuildTuple{...}, BuildList{...}, BuildSet{...}, BuildMap{...}, BuildString{...} → BuildTuple(size), BuildList(size), BuildSet(size), BuildMap(size), BuildString(count), with size/count always a u32 derived from lengths (to_u32(), try_from), and
• UnpackSequence{...}, UnpackEx{...} → UnpackSequence(n), UnpackEx(args).

The stack shapes (how many elements are consumed/produced) and the computed counts are unchanged; only the enum constructor syntax and arg plumbing differ. No off-by-one or overflow risks are apparent in the new uses.

Also applies to: 2079-2122, 4305-4323, 4475-4501, 4839-4881, 5340-5341, 5447-5448

---

783-799: Resume opcodes now take raw u32 tags; usage is consistent across scopes and suspension points

All Resume sites (AtFuncStart, AfterYield, AfterAwait, AfterYieldFrom) now pass bytecode::ResumeType::* as u32, which matches the new tuple-style constructor and appears consistent across:

• function/module entry (enter_scope),
• async with / async for,
• yield, await, yield from,
• generator-based comprehensions.

Every Resume is still paired with the correct preceding Yield*/GetAwaitable/YieldFrom. As long as Resume’s oparg type in Instruction is indeed u32 (or a type alias thereof), this is semantically equivalent.

Also applies to: 2836-2845, 2886-2888, 2922-2929, 4417-4454, 4529-4539, 4993-5003, 5058-5064

---

1062-1075: CopyItem / Swap depth indices remain aligned with previous stack discipline

You’ve replaced legacy dup/rot instructions with tuple-style CopyItem(depth) and Swap(depth) in several stack-sensitive places:

• REPL last-expression printing and block expressions,
• multi-target assignments and named expressions,
• __classcell__ capture in class bodies,
• pattern-matching helpers (sequence/mapping/or patterns),
• chained comparisons and guard handling,
• augmented assignment for subscripts/attributes.

All these call sites preserve the old constant depths (1_u32, 2_u32, 3, 1 + remaining, etc.), and the surrounding comments/logic still match the expected stack shapes (e.g., copying the subject before pattern attempts, reordering container/slice/result in augassign). I don’t see any depth mismatches or regressions here.

Also applies to: 1087-1098, 1636-1644, 2626-2633, 3163-3175, 3230-3233, 3331-3337, 3492-3551, 3595-3603, 3838-3850, 3947-3954, 4128-4145, 4162-4170, 4648-4651

---

1305-1391: Import and attribute opcodes (Import*, Load*/Store*/DeleteAttr, LoadMethod) updated cleanly

The various import and attribute-related instructions now use tuple-style constructors:

• ImportName(idx), ImportFrom(idx), ImportNameless,
• LoadAttr(idx), StoreAttr(idx), DeleteAttr(idx),
• LoadMethod(idx).

Each idx is consistently obtained via self.name(...) (a NameIdx), and the surrounding control flow (from-import *, chained attribute loads, delete-attribute, method calls) is unchanged. This is a straightforward enum-shape refactor with no semantic change.

Also applies to: 1721-1735, 4041-4053, 4362-4366, 4742-4751

---

1408-1463: Control-flow opcodes with label operands correctly leverage the new Label plumbing

All structured control flow now routes label targets through emit!(..., Instruction::<Op>(block_idx)), relying on the new EmitArg<bytecode::Label> for ir::BlockIdx:

• Jump(...) in if/elif chains, while/for loops, try/except/else/finally, match, and comprehensions,
• loop control (Break(exit_block), Continue(loop_block), ForIter(else_block)),
• exception setup (SetupExcept(handler_block), SetupFinally(finally_block)),
• boolean / guard jumps (PopJumpIfTrue/False, JumpIfFalseOrPop, JumpIfTrueOrPop).

Targets are always the appropriate BlockIdx (e.g., after/else blocks, handler blocks, loop heads/tails), and there’s no sign of miswired labels. This matches the new label-based Instruction design without altering high-level control flow.

Also applies to: 1553-1607, 1958-2061, 2791-2816, 2896-2965, 2999-3015, 3812-3890, 4207-4277, 4281-4303, 4990-5003

---

1465-1479: Raise and comparison-related opcodes migrated to tuple-style with unchanged semantics

The refactors:

• Raise(kind) for the three RaiseKind cases,
• CompareOperation(ComparisonOperator::...) in mapping/sequence patterns and chained comparisons,
• ContainsOp(Invert::...), IsOp(Invert::...) in comparisons and pattern checks,

are all one-to-one replacements of the prior struct-style variants. The chosen ComparisonOperator values and Invert flags still align with the surrounding logic (>= for mapping-length checks, == for value patterns, ==/>= in chained comparisons, etc.). No behavioural differences are introduced.

Also applies to: 3421-3443, 3720-3739, 3756-3765, 3898-3973

---

1806-1955: Type-parameter and generic handling matches the new intrinsic and function-attribute opcodes

In the type-params pipeline:

• compile_type_param_bound_or_default uses UnpackSequence(1) only when allowed and wraps evaluation in a tiny function called via CallFunctionPositional(0), as before.
• compile_type_params now emits CallIntrinsic1(TypeVar/ParamSpec/TypeVarTuple) and CallIntrinsic2(TypeVarWithBound/Constraint/SetTypeparamDefault) with tuple-style args, plus CopyItem(1) + store_name to bind each type param. The counts passed to BuildTuple(...) are still derived from type_params.len().

For generic functions/classes:

• compile_function_def and compile_class_def use Swap(2) / Swap(num_typeparam_args + 1) and CallIntrinsic2(SetFunctionTypeParams) to attach type params, and later CallFunctionPositional(num_typeparam_args) to invoke the wrapper closure.
• In class compilation, CallIntrinsic1(SubscriptGeneric) and the subsequent CallFunctionKeyword/Positional for __build_class__ are updated mechanically to tuple-style with the same argument counts.

Across these sites the stack comments and arg counts still line up with the design; I don’t see any regressions in how generics are threaded through.

Also applies to: 2550-2640, 2656-2760

---

1958-2061: Try/except/finally, async-with, async-for, and async comprehensions remain structurally equivalent

The conversions to:

• SetupFinally(finally_block), SetupExcept(handler_block/after_block), Jump(else_block/finally_block), Raise(Reraise) in compile_try_statement,
• SetupAsyncWith(final_block) / SetupWith(final_block) plus Resume(AfterAwait as u32) in compile_with,
• async-for’s SetupExcept(else_block), Resume(AfterAwait as u32), and EndAsyncFor,
• async comprehensions’ SetupExcept(after_block), Resume(AfterAwait as u32), and EndAsyncFor,

all keep the same block structure and stack behaviour used previously, just through tuple-style variants. Exception handling, finally unwinding, and async control flow should behave identically.

Also applies to: 2818-2894, 2896-2965, 4990-5003

---

3096-3570: Pattern-matching opcodes (Unpack*, Match*, mapping/sequence/or-pattern helpers) are consistently updated

In the structural pattern-matching helpers you’ve migrated to tuple-style opcodes:

• UnpackEx(args) / UnpackSequence(n) in sequence unpacking and star patterns,
• MatchClass(nargs), MatchMapping, MatchSequence,
• mapping patterns’ BuildTuple(size) for keys, BuildMap(0), DictUpdate(2), UnpackSequence(size), and the CopyItem/Swap choreography for deleting keys into DeleteSubscript,
• BinaryOperation(BinaryOperator::Subtract) for index arithmetic,
• various CopyItem/Swap calls in or-pattern store alignment and sequence subscripting.

The updated instructions mirror the CPython algorithm and the prior Rust implementation; counts are still derived from elts.len(), nargs + n_attrs, or size checks that guard against overflow. I don’t see off-by-one issues or stack mismanagement introduced by the tuple-style transition here.

Also applies to: 3680-3769

---

3822-3890: match case dispatch and guards use the new jump opcodes correctly

For match statements:

• Each non-default case now uses CopyItem(1) to duplicate the subject (except the last checked case), compiles the pattern, then jumps with Jump(end) after the case body.
• Guards use PopJumpIfFalse(pattern_context.fail_pop[0]) to fall back to the failure label, and you still call emit_and_reset_fail_pop between cases.
• The final default case, when present, uses JumpIfFalseOrPop(end) for its guard.

The tuple-style Jump/PopJumpIfFalse/JumpIfFalseOrPop opcodes target the same blocks as before, and the subject clean-up (Pop) and store binding logic remain intact. Control flow through cases is unchanged.

---

3898-3973: Chained comparisons and boolean short-circuiting preserve previous control-flow structure

For chained comparisons:

• CompareOperation(Equal/Less/LessOrEqual/Greater/GreaterOrEqual) is now emitted via the tuple-style variant.
• The Swap(2) + CopyItem(2) pattern to retain the RHS for the next comparison, and JumpIfFalseOrPop(break_block) for early exit, are unchanged in logic.

For boolean expressions:

• compile_jump_if’s fallback path now uses PopJumpIfTrue/False(target_block),
• compile_bool_op uses JumpIfFalseOrPop / JumpIfTrueOrPop with the same block layout (after_block).

Only the constructor syntax changed; the jump conditions, labels, and stack effects remain as before.

Also applies to: 4207-4277, 4281-4303

---

4350-4366: Expression-level opcodes (unary, slice, yield/await, comprehensions, f-strings) are refactored mechanically

Key expression sites now use tuple-style variants:

• UnaryOperation(op) after unary exprs, with op unchanged.
• BuildSlice(step) in slice expressions (bool step flag).
• YieldValue + Resume(AfterYield/AfterYieldFrom) and GetAwaitable + YieldFrom + Resume(AfterAwait) for yield, yield from, await.
• Comprehensions’ initializers BuildList/BuildSet/BuildMap(OpArgMarker::marker()) and per-element ops ListAppend(len), SetAdd(len), MapAdd(len) in compile_comprehension.
• f-strings using FormatValue(conversion) and BuildString(count) for both nested and top-level formatting.

Given the counts and enums are unchanged, these are straightforward enum-shape updates without semantic changes.

Also applies to: 4406-4407, 4417-4454, 4538-4584, 4593-4612, 5340-5341, 5436-5448

---

4720-4834: Call and keyword handling (CallFunction* / CallMethod* / BuildMap*) align with the new API

In the call path:

• compile_call now uses LoadMethod(idx) for method calls, then dispatches through compile_method_call vs compile_normal_call.
• compile_normal_call and compile_method_call cleanly map CallType::{Positional,Keyword,Ex} to CallFunctionPositional/Keyword/Ex and their method counterparts.
• compile_call_inner uses BuildTupleFromTuples(size) vs BuildTuple(size) for positional args, and CallFunctionKeyword(count) when keyword names are packed into a const tuple.
• compile_keywords emits BuildMap(sub_size) for each group of named keywords and BuildMapForCall(size) when multiple mapping segments are to be merged.

All argument counts are derived from arguments.len(), size, or keyword group sizes as before, so the switch to tuple-style opcodes should not affect call semantics.

---

5130-5136: Const-return peephole still works with marker-based LoadConst / ReturnConst

emit_load_const / emit_return_const now create LoadConst(idx) / ReturnConst(idx) using emit_arg, and emit_return_value rewrites a terminal LoadConst(idx) into ReturnConst(idx) by pattern-matching on the new tuple-style variant.

Because the OpArg payload is untouched when you mutate inst.instr, the encoded constant index is preserved. This keeps the peephole optimization intact under the new Instruction representation.

Also applies to: 5140-5143

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

882-885: TODO placeholder always returns true.

The gen_is_coroutine closure always returns true, which may not correctly identify whether a generator is actually a coroutine. This could affect the behavior of async iteration. The TODO is acknowledged, but consider adding an issue reference to track this.

---

533-1543: The execute_instruction match arms are correctly updated.

The transition to tuple-style instruction variants with .get(arg) accessor pattern is consistently applied throughout the execution logic. The alphabetical ordering of match arms aligns with the bytecode definition (with the noted exception of EnterFinally/ExtendedArg order in bytecode.rs).

crates/compiler-core/src/bytecode.rs (1)

551-691: Tuple-style variant refactoring looks correct.

The conversion from named-field variants (e.g., Foo { index: Arg<T> }) to tuple-style variants (e.g., Foo(Arg<T>)) is consistently applied. The LAST_INSTRUCTION sentinel is correctly updated to YieldValue (the last variant alphabetically after fixing the EnterFinally/ExtendedArg order).

Reviewing files that changed from the base of the PR and between d917dbc and 6579723.

• crates/codegen/src/compile.rs (114 hunks)
• crates/compiler-core/src/bytecode.rs (5 hunks)
• crates/jit/src/instructions.rs (4 hunks)
• crates/jit/tests/common.rs (4 hunks)
• crates/vm/src/frame.rs (8 hunks)

• GitHub Check: Run tests under miri
• GitHub Check: Run snippets and cpython tests on wasm-wasi
• GitHub Check: Check Rust code with clippy
• GitHub Check: Check the WASM package and demo
• GitHub Check: Run snippets and cpython 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 rust tests (windows-latest)
• GitHub Check: Run rust tests (macos-latest)
• GitHub Check: Ensure compilation on various targets

crates/jit/tests/common.rs (1)

95-173: StackMachine opcode updates align with new compiler emission

The new handlers for BuildMap, ExtendedArg, LoadConst, LoadNameAny, ReturnConst, ReturnValue, SetFunctionAttribute, StoreAttr, and StoreLocal are consistent with how compile.rs now emits bytecode (especially for annotations, function creation, and locals). Stack shapes for annotations (LoadNameAny + BuildMap + SetFunctionAttribute(ANNOTATIONS)) and for storing functions into locals via StoreLocal look correct for the jit_function! test harness.

No blocking issues here from a JIT test perspective.

crates/jit/src/instructions.rs (4)

198-208: Unreachable-code tracking correctly extended to ReturnConst

Marking in_unreachable_code = true for both Instruction::ReturnValue and Instruction::ReturnConst(_) ensures no further IR is emitted in the same block after any return. This matches how ReturnConst now behaves and avoids malformed Cranelift blocks.

---

428-480: CompareOperation handling is type‑sound for int/bool/float combinations

The new Instruction::CompareOperation arm correctly:

• Pops b then a (rhs, lhs) and inspects their JitType.
• Widens Bool to I64 before integer comparisons, treating bools as 0/1.
• Maps all ComparisonOperator variants to the appropriate IntCC/FloatCC.
• Rejects unsupported type pairs with JitCompileError::NotSupported.

This keeps the JIT’s comparison semantics aligned with Python’s for ints, bools, and floats.

---

514-548: Branch lowering for PopJumpIfFalse/True matches bytecode semantics

The new PopJumpIfFalse / PopJumpIfTrue arms:

• Pop the condition, coerce it via boolean_val, and create a then_block for the jump target and a fresh fall‑through block.
• Use brif with the target and fall‑through blocks wired in the correct order (false → target for PopJumpIfFalse, true → target for PopJumpIfTrue), then switch the builder to the fall‑through block.

This matches the CPython POP_JUMP_IF_* behavior while integrating cleanly with the label‑based block mapping.

---

553-618: New control‑flow and stack ops (ExtendedArg, Jump, Load*, Pop, PopBlock, Resume, ReturnConst/Value, SetupLoop, StoreFast, UnpackSequence) look consistent

The added/updated cases in add_instruction for:

• ExtendedArg (no‑op),
• Jump,
• LoadConst / LoadFast / LoadGlobal,
• Pop / PopBlock,
• Resume (placeholder),
• ReturnConst / ReturnValue,
• SetupLoop,
• StoreFast,
• UnpackSequence,

all use reasonable stack discipline and error out with BadBytecode / NotSupported on malformed input. In particular:

• LoadGlobal only supports the function’s own name, pushing a FuncRef, which is a clear, safe restriction.
• UnpackSequence checks that the tuple length matches size.get(arg) before extending the stack in reverse, matching Python’s UNPACK_SEQUENCE behavior. (docs.python.org)

These additions are fine for the current, limited JIT surface.

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

441-461: Slice emission with BuildSlice(n == 3) matches 2/3‑arg slice semantics

compile_subscript now calls:

let n = self.compile_slice(s)?;
match ctx {
    ExprContext::Load => {
        emit!(self, Instruction::BuildSlice(n == 3));
        emit!(self, Instruction::Subscript);
    }
    ExprContext::Store => {
        emit!(self, Instruction::BuildSlice(n == 3));
        emit!(self, Instruction::StoreSubscript);
    }
    ...
}

With compile_slice returning 3 when a step is present and 2 otherwise, this lines up with Python 3.12’s BUILD_SLICE taking a boolean “has step” flag rather than an explicit count, while keeping the existing stack discipline.

---

150-217: Function annotations and SetFunctionAttribute(ANNOTATIONS) are wired correctly

The combination of:

• visit_annotations pushing name/annotation pairs and then emit!(self, Instruction::BuildMap(num_annotations)), and
• compile_function_def OR’ing MakeFunctionFlags::ANNOTATIONS into funcflags and passing it into compile_function_body / make_closure,

ensures that when make_closure runs, the annotations dict is already on the stack beneath the freshly created function, so SetFunctionAttribute(ANNOTATIONS) can consume [annotations_dict, func] and attach the annotations. This matches the new attribute‑based MAKE_FUNCTION design and the JIT test harness’ expectation that annotations live in a single map per function.

---

2091-2132: Default/kw‑default handling via BuildTuple and BuildMap is consistent

compile_default_arguments now:

• Compiles positional defaults and wraps them in BuildTuple(defaults.len().to_u32()), setting DEFAULTS in MakeFunctionFlags.
• Compiles keyword‑only defaults into key/value pairs and wraps them in BuildMap(kw_with_defaults.len().to_u32()), setting KW_ONLY_DEFAULTS.

These opcodes match Python’s BUILD_TUPLE / BUILD_MAP(count) semantics (tuple of defaults, dict of kwdefaults), and make_closure later consumes them via SetFunctionAttribute(DEFAULTS) and SetFunctionAttribute(KW_ONLY_DEFAULTS) in the right stack order.

---

5122-5140: emit_return_value peephole to ReturnConst is safe and reduces bytecode

The new emit_return_value:

fn emit_return_value(&mut self) {
    if let Some(inst) = self.current_block().instructions.last_mut()
        && let Instruction::LoadConst(idx) = inst.instr
    {
        inst.instr = Instruction::ReturnConst(idx);
        return;
    }
    emit!(self, Instruction::ReturnValue)
}

reuses the existing OpArg payload and only changes the opcode variant, so the op‑arg encoding stays correct while shrinking a LOAD_CONST; RETURN_VALUE pair into a single RETURN_CONST. This matches the new RETURN_CONST opcode and is a straightforward win.

---

4303-4314: Dict literal emission (BuildMap + DictUpdate) matches CPython 3.9+

compile_dict now:

• Emits key/value pairs for non‑unpacked entries followed by BuildMap(size).
• For each **mapping entry, evaluates the mapping expression then emits DictUpdate(1).

This mirrors CPython’s BUILD_MAP(count) followed by DICT_UPDATE(i) semantics for unpacked mappings, with size as the number of packed key/value pairs. (docs.python.org)

---

3033-3045: Based on my verification through web searches, I can confirm key claims from the review comment:

✓ Verified:

• CPython's SWAP(i) does use 1-based indexing: SWAP(i) swaps STACK[-i] with STACK[-1]
• SWAP(1) is indeed a no-op (swaps TOS with itself)
• RustPython implements SWAP matching CPython's semantics

✗ Cannot Verify (Repository access unavailable):

• The claimed off-by-one in the JIT implementation
• Whether pattern_helper_rotate actually works correctly with RustPython's implementation of Instruction::Swap
• The actual behavior of Instruction::CopyItem and its semantic alignment with COPY(i)

---

The review comment's core assumption about CPython-style 1-based depth semantics is correct; however, verification of the specific JIT off-by-one and correctness of pattern_helper_rotate requires repository access.

The reviewer's request to confirm that Instruction::Swap and Instruction::CopyItem use CPython's 1-based depth semantics is reasonable and well-founded, given that RustPython is documented to follow CPython's semantics. The suggestion to verify this alignment is sound, as semantic inconsistencies between VM components would indeed cause bugs. However, without access to the RustPython repository to inspect the actual JIT implementation and the claimed off-by-one error, the verification cannot be completed.

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

534-1543: LGTM! Mechanical conversion is correct and complete.

All instruction handlers have been correctly updated from struct-style to tuple-style syntax. The pattern is consistent throughout: each instruction variant uses Foo(marker) followed by marker.get(arg) to extract the actual argument value. The unsafe operations are appropriately documented and follow the established "trust compiler" pattern.

crates/compiler-core/src/bytecode.rs (1)

552-691: Enum refactoring looks good overall.

The Instruction enum variants have been successfully:

1. Converted from struct-style to tuple-style (e.g., Foo { index: Arg<T> } → Foo(Arg<T>))
2. Alphabetically sorted as intended
3. Properly reflected in LAST_INSTRUCTION constant

The conversion is complete and correct for the enum definition itself.