Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/compiler-core/opcode.toml`:
- Around line 269-270: The oparg type for
PseudoOpcode.opcodes.StoreFastMaybeNull is incorrect: change the var_num oparg
from oparg::NameIdx to oparg::VarNum so PseudoInstruction::StoreFastMaybeNull is
generated with var_num: Arg<oparg::VarNum> (matching other var_num uses like
StoreFast/LoadFast) to index localsplus and preserve the VarNum newtype's type
safety.
In `@crates/compiler-core/src/bytecode/instruction.rs`:
- Around line 49-58: The doc-comment promises that stack_effect_jump differs for
certain ops like Self::ForIter, but stack_effect_jump currently just delegates
to stack_effect(oparg), so ForIter's branch effect isn't handled; update
stack_effect_jump to match on instruction variants (e.g., match self {
Self::ForIter => -1, /* other special cases */ ... , _ =>
self.stack_effect(oparg) }) so branch (jump=true) effects are correct, or
alternatively remove the misleading ForIter example from the doc-comment if you
do not want to implement overrides now; ensure callers that rely on
stack_effect_jump (the call-site noted in the review) will receive correct
jump-edge counts.
- Around line 121-124: Instruction::stack_effect_jump incorrectly delegates to
Opcode::stack_effect instead of Opcode::stack_effect_jump; update the method to
call self.as_opcode().stack_effect_jump(oparg) so it mirrors
PseudoInstruction::stack_effect_jump and correctly uses the branch-specific
stack effect behavior for opcodes like ForIter (use as_opcode(),
stack_effect_jump, and Instruction::stack_effect_jump to locate the change).
---
Nitpick comments:
In `@crates/compiler-core/generate.py`:
- Around line 18-21: The except KeyError block re-raises a new ValueError
without exception chaining, triggering Ruff B904; update the except to capture
the original KeyError (e.g., except KeyError as e) and re-raise the ValueError
with explicit chaining using "from e" so the traceback context for CPYTHON_ROOT
resolution (pathlib.Path(os.environ["CPYTHON_ROOT"]).expanduser().resolve()) is
preserved.
- Around line 462-470: The walrus in fn_as_opcode assigns oparg but never uses
it, triggering Flake8 F841; replace the walrus check in the loop (where arms is
built for each instr/name) with a plain containment check against self.metadata
for "oparg" (e.g., use if "oparg" in self.metadata.get(name, {}) or if
self.metadata.get(name, {}).get("oparg") without assignment) so the branch that
appends " { .. }" runs without creating an unused binding.
In `@crates/compiler-core/src/bytecode/instruction.rs`:
- Around line 480-498: The AnyOpcode::deopt const fn can be simplified: inside
each arm (Self::Real(opcode) and Self::Pseudo(opcode)) replace the if-let
Some/else None pattern with a match on opcode.deopt() that returns
Some(Self::Real(op)) / Some(Self::Pseudo(op)) or None, preserving const fn and
behavior and matching the style used by Opcode::deoptimize and the nearby
accessors.
In `@crates/stdlib/src/_opcode.rs`:
- Around line 70-145: Add direct unit tests targeting the _opcode module: call
the Python-exposed functions stack_effect (with jump true/false/None), is_valid,
has_arg, has_const, has_name, has_jump, has_free, has_local, and has_exc to
assert expected behavior for (1) a known valid opcode (e.g., LOAD_CONST or
POP_TOP) returns correct stack effects and true for the corresponding has_*
helpers, (2) an invalid opcode integer causes is_valid False and stack_effect
raises/returns the ValueError behavior observed in the implementation, and (3) a
specialized opcode (one whose deopt() is Some) triggers the ValueError path in
stack_effect; keep tests focused and small so future opcode generator changes
are caught without relying only on dis.dis snapshots.