Reviewing files that changed from the base of the PR and between f0526b9 and f4b8be8.

• crates/codegen/src/compile.rs
• crates/compiler-core/src/bytecode.rs
• crates/vm/src/frame.rs

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

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

557-564: LGTM: Instruction rename is consistent.

The rename from BinarySubscript to BinarySubscr aligns with the PR's opcode standardization objective. The implementation logic remains unchanged.

---

791-803: LGTM: Semantic clarification with DeleteName.

The rename from DeleteLocal to DeleteName better reflects that this instruction deletes by name from the locals mapping (using ass_subscript), distinguishing it from DeleteFast which operates on the fastlocals array by index. This improves code clarity.

---

804-804: LGTM: Instruction rename is consistent.

The rename from DeleteSubscript to DeleteSubscr aligns with the opcode standardization.

---

1350-1358: LGTM: Instruction rename is consistent.

The rename from PopException to PopExcept follows the abbreviated naming convention used elsewhere in the bytecode.

---

1370-1370: LGTM: Instruction rename is consistent.

The rename from Raise to RaiseVarargs better communicates that this instruction handles variable arguments for raise statements (reraise, raise with cause, etc.).

---

1418-1418: LGTM: Instruction rename is consistent.

The rename from SetupAnnotation (singular) to SetupAnnotations (plural) is more accurate since the instruction sets up the __annotations__ dictionary which holds multiple annotations.

---

1487-1492: LGTM: Semantic clarification with StoreName.

The rename from StoreLocal to StoreName better reflects that this instruction stores by name in the locals mapping (using ass_subscript), distinguishing it from StoreFast which stores by index in the fastlocals array. This improves code clarity.

---

1493-1493: LGTM: Instruction rename is consistent.

The rename from StoreSubscript to StoreSubscr aligns with the opcode standardization and abbreviated naming convention.

---

530-1578: Ensure code passes cargo fmt and cargo clippy before merging.

Per the coding guidelines for Rust files, run cargo fmt to format the code and cargo clippy to check for lints. Fix any warnings introduced by these changes before final submission.

---

700-722: CheckExcMatch implementation is correct.

The implementation properly validates that exception types inherit from BaseException, handles both single types and tuples, and leverages is_instance which correctly delegates tuple matching to object_recursive_isinstance. The bytecode instruction is correctly emitted during exception handler compilation, and the logic accurately mirrors Python's exception matching behavior.

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

1696-1823: Stack effects for new/renamed opcodes match codegen usage

The updated stack_effect arms for BinarySubscr, Subscript, StoreSubscr, DeleteSubscr, StoreName/DeleteName, CheckEgMatch, CheckExcMatch, RaiseVarargs, EndAsyncFor, SetupAnnotations, etc., line up with how they’re emitted in compile.rs:

• StoreSubscr => -3 and DeleteSubscr => -2 match the [value, container, index] and [container, index] stack shapes produced by compile_subscript, compile_store, and compile_augassign.
• BinarySubscr => -1 is used where we duplicate the subject and then index into it in pattern helpers.
• RaiseVarargs’s -(kind.get(arg) as u8 as i32) matches the number of values the compiler pushes for each RaiseKind (0/1/2).
• CheckEgMatch => 0 (pop 2, push 2) is consistent with the [orig, list, rest, type] -> [orig, list, new_rest, match] comment in compile_try_star_except.
• CheckExcMatch(_) => -2 matches the CopyItem + exc_type pattern in compile_try_statement (3 stack entries down to the original exc).

I don’t see any inconsistency between these definitions and the codegen sites.

---

875-915: Confirm bytecode compatibility expectations after changing opcode values

By pinning explicit repr(u8) discriminants and updating Instruction::try_from accordingly, the binary encoding of bytecode has changed. Any previously serialized CodeUnit/CodeObject (e.g., cached .pyc-like artifacts) using the old opcode layout will now either fail to decode (InvalidBytecode) or, if produced by older code that also used transmute, could be misinterpreted if reused across versions. If RustPython ever persists bytecode across versions, it would be good to confirm that:

• Either the magic/marshal version is bumped so old caches are rejected explicitly, or
• You guarantee such caches are never reused across a change like this.

Otherwise, you may get hard‑to‑diagnose failures when loading stale compiled artifacts.

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

445-502: Subscript codegen matches new BinarySubscr/StoreSubscr/DeleteSubscr semantics

The various subscript paths are internally consistent with the new opcodes and their stack effects:

• compile_subscript:

  • Load: evaluates value, then slice, and calls Subscript, giving [obj, index] -> result as expected.
  • Store: when used from compile_store, we start with the assigned value on the stack; after compiling value and slice (and BuildSlice when optimizing 2‑element slices) we end up with [value, container, sub], which matches StoreSubscr => -3.
  • Delete: compiles value then slice and calls DeleteSubscr, with [container, sub] matching DeleteSubscr => -2.

• pattern_helper_sequence_subscr duplicates the subject and uses BinarySubscr with [subject, subject, idx] so the helper keeps the original subject while extracting an element, consistent with BinarySubscr => -1.

• compile_annotated_assign builds [annotation, __annotations__, name] before StoreSubscr, so __annotations__[name] = annotation is encoded correctly.

• compile_augassign’s subscript case carefully arranges the stack to [result, container, index] before StoreSubscr, again matching the [value, container, sub] convention.

Given the stack_effect definitions in bytecode.rs, all of these sites look correct.

Also applies to: 1801-1827, 3621-3622, 4484-4511, 4588-4648

---

1180-1302: NameOp::Name migration to LoadNameAny/StoreName/DeleteName is coherent

Routing NameOp::Name through LoadNameAny / StoreName / DeleteName is consistent with how indices are allocated:

• name() and get_global_name_index() both index into CodeInfo.metadata.names, which is exactly what these opcodes consume.
• Module/class bodies (compile_program, compile_program_single, compile_class_body) and class metadata setup (__module__, __qualname__, __doc__, __firstlineno__, __type_params__, __classcell__) all use name() and StoreName, so they’re aligned with the bytecode enum.

As long as the VM side now treats LOAD_NAME_ANY/STORE_NAME/DELETE_NAME as the generic name path (mirroring the old local/name opcodes), behavior should be unchanged.

Also applies to: 2959-3042

---

1501-1518: RaiseVarargs and exception-matching opcodes integrate cleanly in try/except and assert paths

The switch from Raise to RaiseVarargs and from JumpIfNotExcMatch to CheckExcMatch is wired consistently:

• Stmt::Raise:

  • RaiseKind::Raise is emitted after compiling one expression (stack_effect -1).
  • RaiseKind::RaiseCause is emitted after compiling two expressions (stack_effect -2).
  • Bare raise uses RaiseKind::Reraise (stack_effect 0), matching the absence of new stack values.

• In assert lowering, the explicit RaiseVarargs { kind: Raise } matches compiling exactly one exception instance.

• Classic try/except:

  • For typed handlers, the CopyItem + exc_type + CheckExcMatch(next_handler) pattern creates [exc, exc, type] so that CheckExcMatch can consume the duplicate and type and still leave the original exc for the bound name or PopTop.
  • Unhandled cases jump to the final next_handler block, where RaiseVarargs { kind: Reraise } is emitted, so unmatched exceptions are re‑raised.

• try/except*:

  • Uses CheckEgMatch with the documented [orig, list, rest, type] -> [orig, list, new_rest, match] shape.
  • SetExcInfo is used before running the handler body so a bare raise picks up the right match.
  • The “no more to reraise” path does PopTop (drop the None result) and PopExcept, while the re‑raise path deliberately omits PopExcept and just executes RaiseVarargs { kind: Raise }, leaving block unwinding to the VM.

These all line up with the stack_effect definitions in bytecode.rs; I don’t see a mismatch.

Also applies to: 1586-1591, 2053-2147, 2178-2420

---

2923-2957: SetupAnnotations usage remains consistent with annotation detection

The rename to Instruction::SetupAnnotations has been plumbed through all relevant sites:

• Module and REPL compilation (compile_program, compile_program_single) call find_ann and emit SetupAnnotations only when needed.
• Class bodies (compile_class_body) also call find_ann and emit SetupAnnotations once at the top when annotations are present.

Since compile_annotated_assign is already gated on !ctx.in_func() for runtime annotation storage, this preserves the prior behavior with the new opcode name.

Also applies to: 1034-1053, 1063-1065, 3019-3022

---

3821-4022: CompareOp/ComparisonOperator wiring for patterns and comparisons looks correct

The migration to ComparisonOperator + Instruction::CompareOp is coherent:

• Mapping in compile_addcompare:
  • CmpOp::Eq/NotEq/Lt/LtE/Gt/GtE map to Equal, NotEqual, Less, LessOrEqual, Greater, GreaterOrEqual respectively.
• Structural pattern matching:
  • Mapping patterns use CompareOp { op: GreaterOrEqual } for the len(subject) >= size and len(subject) >= size-1 checks.
  • Sequence patterns use CompareOp { op: Equal } for len(subject) == size and GreaterOrEqual where a *rest is present.
  • Value patterns use CompareOp { op: Equal } for subject == value.
• Other relational/inclusion ops (In/NotIn, Is/IsNot) delegate to ContainsOp and IsOp with the right Invert flag.

Given CompareOp has stack effect -1 and the surrounding code always leaves the subject underneath the comparison, these changes look correct.

Also applies to: 4132-4209, 4211-4223, 4361-4376

---

4484-4517: Annotation storage and augmented assignment now use StoreSubscr consistently

Two key sites now rely on StoreSubscr and are consistent with its stack contract:

• compile_annotated_assign:

  • For simple name: T outside functions, after compile_annotation the stack is [annotation], then LoadNameAny(__annotations__), then the mangled name string, yielding [annotation, __annotations__, name] before StoreSubscr, which matches the [value, container, index] convention.

• compile_augassign:

  • The subscript case builds [container, slice], duplicates both to get a copy for loading, uses Subscript to obtain the original value, then performs the inplace binary op with the RHS and finally reorders to [result, container, slice] just before StoreSubscr.

Both cases align with the StoreSubscr stack_effect and with how simple subscript stores are compiled via compile_subscript.

Also applies to: 4588-4648

---

5443-5630: Async for/comprehensions correctly use EndAsyncFor under the new opcode layout

In both explicit async for loops and comprehensions:

• compile_for emits EndAsyncFor on the async path after the else block, matching EndAsyncFor => -2 (it cleans up the async iterator state).
• compile_comprehension emits EndAsyncFor when any generator is async, and uses GetAIter/GetANext + Resume { AfterAwait }, consistent with the updated async for bytecodes.

Given the updated numeric assignment for EndAsyncFor and the unchanged stack_effect, the codegen here still looks correct.

Also applies to: 3331-3377

Reviewing files that changed from the base of the PR and between 153a03a and 5369dab.

• crates/compiler-core/src/bytecode.rs

• GitHub Check: Check Rust code with clippy
• GitHub Check: Run rust tests (windows-latest)
• GitHub Check: Run snippets and cpython tests (windows-latest)
• GitHub Check: Ensure compilation on various targets

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

679-712: Coordinate with PR #6638 before merging.

Based on the PR comments, PR #6638 will significantly change the instruction structure. Since this PR assigns explicit discriminants to the Instruction enum, merging both could create conflicts or require substantial rework.

Recommend coordinating with the author of PR #6638 to determine:

1. Which PR should merge first
2. Whether discriminant assignments should be deferred until after #6638
3. How to handle potential conflicts in the instruction enum structure

---

679-712: Cache (opcode 0) and Reserved3 (opcode 3) variants appear to be referenced but not defined in the Instruction enum.

The review comment correctly identifies that opcode 0 is missing from the visible enum definition (BeforeAsyncWith starts at 1). However, the code references Cache and Reserved3 in match statements within stack_effect() (line 1846) and the display logic (line 2084), but these variants cannot be located in the enum definition. If not properly defined, this would cause compilation errors.

This discrepancy needs clarification:

• If Cache and Reserved3 are intentionally omitted per CPython 3.13 spec (which removed CACHE as a separate opcode), they should be removed from match statements.
• If they should be included, they must be added to the enum with appropriate discriminants (Cache = 0, Reserved3 = 3).

Reviewing files that changed from the base of the PR and between 5369dab and 258f61c.

• Lib/_opcode_metadata.py is excluded by !Lib/**
• Lib/test/test__opcode.py is excluded by !Lib/**
• Lib/test/test_dis.py is excluded by !Lib/**

• crates/codegen/src/compile.rs
• crates/compiler-core/src/bytecode.rs
• crates/jit/src/instructions.rs
• crates/jit/tests/common.rs
• crates/stdlib/src/opcode.rs
• crates/vm/src/frame.rs
• scripts/generate_opcode_metadata.py

• GitHub Check: Run snippets and cpython tests (macos-latest)
• GitHub Check: Run snippets and cpython tests (windows-2025)
• GitHub Check: Run snippets and cpython tests (ubuntu-latest)

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

1763-1914: LGTM! Stack effect calculations properly updated.

The stack effect calculations have been correctly updated to use the renamed instruction variants (LoadName, StoreName, DeleteName, Call, CallKw, RaiseVarargs, etc.). The logic for each instruction's stack manipulation appears correct.

---

1982-2104: LGTM! Disassembly formatting correctly updated.

The fmt_dis method has been properly updated to display the renamed instruction variants with their appropriate names (e.g., BINARY_SUBSCR, DELETE_SUBSCR, LOAD_NAME, STORE_NAME, CALL, CALL_KW, RAISE_VARARGS, etc.).

scripts/generate_opcode_metadata.py (2)

55-81: LGTM! Generation logic correctly parses and outputs opcode metadata.

The script properly:

1. Reads the Rust source file
2. Extracts the Instruction enum body
3. Parses opcodes with their discriminants
4. Generates sorted output in CPython 3.13 compatible format

The dynamic parsing approach eliminates the need for manual updates when opcodes change.

---

32-37: Python version requirement is acceptable for this development script.

This is a development-only script located in the scripts/ directory used to generate opcode metadata. The script already indicates compatibility with CPython 3.13 in its header comment, and the project's whats_left.py explicitly requires Python 3.13+. Since itertools.batched was introduced in Python 3.12, its use is fully compatible with the project's stated Python version requirements.

No action needed.

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

99-105: LGTM! Test instruction handling correctly updated.

The test stack machine has been properly updated to handle the renamed instruction variants:

• LoadNameAny → LoadName
• StoreLocal → StoreName

The logic remains the same, only the variant names changed.

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

457-476: LGTM! JIT call instruction updated.

The JIT compiler correctly handles the renamed Call instruction (previously CallFunctionPositional). The implementation logic remains unchanged.

---

492-545: LGTM! Comparison instruction renamed.

The CompareOp instruction (previously CompareOperation) is correctly handled in the JIT with proper type conversions and condition code mappings for both integer and float comparisons.

crates/stdlib/src/opcode.rs (1)

66-83: LGTM! Opcode name classification correctly updated.

The has_name function has been properly updated to recognize the renamed instruction variants:

• DeleteLocal → DeleteName
• LoadNameAny → LoadName
• StoreLocal → StoreName

All three renamed variants correctly appear in the pattern match for instructions that use co_names.

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

394-394: LGTM! Instruction variant renames are consistent.

The renamed instruction variants align well with standard Python bytecode naming conventions:

• RaiseVarargs, BinarySubscr, CallKw, Call, CompareOp, DeleteName, DeleteSubscr, LoadName, PopExcept, SetupAnnotations, StoreName, and StoreSubscr are all clearer and more conventional names.

The logic remains unchanged—these are purely API surface updates.

Also applies to: 613-613, 714-718, 756-756, 825-825, 838-838, 1195-1195, 1431-1431, 1460-1460, 1562-1562, 1624-1624, 1630-1630

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

1818-1819: The kind mapping correctly covers all three raise forms (bare raise, raise exc, raise exc from cause), and the encoding aligns with the RaiseKind enum and opcode metadata. No issues found.

---

5222-5227: Annotation assignment, augassign, and normal call paths updated to new ops—monitor for stack effect regressions

All three locations (lines 5222–5227, 5363–5364, 6111–6115) correctly use the updated instructions (LoadName, StoreSubscr, Call, CallKw). Run the full test suite plus targeted cases (subscripting stores, __annotations__ in class/module, kwargs calls, exception handling) to verify stack effects before merge.

---

1345-1346: SetupAnnotations instruction verified: correctly gated and scoped

SetupAnnotations is properly emitted in all three entry points (module, function, class) using consistent gating with Self::find_ann(), which recursively detects annotated assignments. The runtime implementation correctly initializes the __annotations__ dict only when needed. No silent breakage risk identified—scopes align with Python semantics and the instruction behavior is sound.

---

1597-1602: LoadName/StoreName/DeleteName scope semantics are correct

LoadNameAny does not exist in the codebase. The current implementation correctly emits LoadName for NameOp::Name, which handles the proper LEGB lookup (Locals → Enclosing → Globals → Builtins): it first tries self.locals.mapping().subscript(name, vm), and on KeyError falls back to self.load_global_or_builtin(name, vm). This is the correct behavior for module and class scopes. StoreName and DeleteName similarly use locals.mapping() for storage/deletion, which is appropriate for these contexts. No behavior regression exists.

---

4338-4339: Pattern-matching opcode renames (BinarySubscr / CompareOp / DeleteSubscr) are properly defined and handled
All three opcodes are correctly defined in the Instruction enum (BinarySubscr = 5, DeleteSubscr = 9, CompareOp with op field), with proper VM dispatch handlers in frame.rs, correct stack effect tracking, and consistent bytecode serialization. All usages in compile.rs align with the defined opcode metadata.

---

497-510: Subscript opcodes are properly defined with documented stack semantics

The StoreSubscr (opcode 39, -3 stack effect), DeleteSubscr (opcode 9, -2 stack effect), and BinarySubscr (opcode 5, -1 stack effect) instructions are well-defined in crates/compiler-core/src/bytecode.rs with clear stack effect documentation. The usage patterns in compile.rs are correct: for store operations, the container and index are on the stack followed by the value; for delete operations, the container and index are consumed. No stack order issues detected.

---

3564-3596: Verify LoadName semantics for .type_params access in PEP 695 generic class contexts

While .type_params is registered with the TYPE_PARAM flag (making it a bound symbol), the code uses LoadName instructions to access it rather than LoadDeref. This appears to work because:

1. In builtins.rs (line 1040), .type_params is explicitly set in the class function's namespace dictionary before execution
2. The compile_name function has special handling for TypeParams scope that allows fallback to parent scope lookup
3. At runtime, LoadName succeeds by finding .type_params in the namespace dict

However, this relies on runtime namespace provisioning rather than the symbol scope mechanism. Confirm that:

• .type_params is guaranteed to be in the namespace dict in all PEP 695 execution paths
• No optimization or refactoring removes the .type_params namespace entry before the class code executes
• The fallback logic in compile_name consistently resolves .type_params correctly across all TypeParams scope access patterns (lines 3589-3590, 3679-3680, 3690, and others)