⚠️ Potential issue | 🟡 Minor

fmt_dis for PseudoInstruction panics unconditionally.

This unimplemented!() will panic if Instruction::Pseudo is ever formatted via the InstructionMetadata trait (e.g., during debug/disassembly output). Consider providing a minimal implementation or at least using todo!("PseudoInstruction display not yet implemented") for a clearer error message.

     fn fmt_dis(
         &self,
-        _arg: OpArg,
-        _f: &mut fmt::Formatter<'_>,
-        _ctx: &impl InstrDisplayContext,
+        arg: OpArg,
+        f: &mut fmt::Formatter<'_>,
+        _ctx: &impl InstrDisplayContext,
         _expand_code_objects: bool,
-        _pad: usize,
+        pad: usize,
         _level: usize,
     ) -> fmt::Result {
-        unimplemented!()
+        match self {
+            Self::Jump { target } => write!(f, "{:pad$}({})", "JUMP", target.get(arg)),
+            Self::JumpNoInterrupt { target } => write!(f, "{:pad$}({})", "JUMP_NO_INTERRUPT", target.get(arg)),
+            Self::LoadAttrMethod { idx } => write!(f, "{:pad$}({})", "LOAD_ATTR_METHOD", idx.get(arg)),
+            Self::LoadSuperMethod { idx } => write!(f, "{:pad$}({})", "LOAD_SUPER_METHOD", idx.get(arg)),
+            Self::LoadZeroSuperAttr { idx } => write!(f, "{:pad$}({})", "LOAD_ZERO_SUPER_ATTR", idx.get(arg)),
+            Self::LoadZeroSuperMethod { idx } => write!(f, "{:pad$}({})", "LOAD_ZERO_SUPER_METHOD", idx.get(arg)),
+            Self::PopBlock => write!(f, "POP_BLOCK"),
+            _ => write!(f, "PSEUDO_PLACEHOLDER"),
+        }
     }

In `@crates/compiler-core/src/bytecode/instruction.rs` around lines 893 - 904, The
fmt_dis implementation for PseudoInstruction currently calls unimplemented!()
and will panic if Instruction::Pseudo is ever formatted via InstructionMetadata;
replace the panic with a defensive implementation (or at minimum a clearer
todo!) by updating the fmt_dis method on the
PseudoInstruction/Instruction::Pseudo branch: either call
todo!("PseudoInstruction display not yet implemented") to provide a clearer
error or implement a minimal formatter that writes a placeholder string (e.g.,
the pseudo opcode name and any relevant fields) to the provided fmt::Formatter,
respecting the signature of fmt_dis and using the
_ctx/_expand_code_objects/_pad/_level params as no-ops if not needed. Ensure the
change is made in the fmt_dis function so formatting of Instruction::Pseudo no
longer panics.

Is this type fit in u16? Otherwise union could be better

Ain't sure what you mean, could you please explain?

And yes, this type fit in a u16 it can hold a RealInstruction (u8) or a PseudoInstruction (u16)

Please add #[repr(u16)] or static_assertions::assert_eq_size to ensure compiler actually recognize it or not.

I tried to add #[repr(u16)], but core::mem::size_of::<Instruction>() gives 4.
Why do we care about the the size of Instruction? It's not intended to be marshaled directly

Oh, you are right! I didn't catch that.
Still union looks more fit than Instruction because they are just separated types sharing a same space. Will that be too much painful to use union than enum for this?

Oh, you are right! I didn't catch that. Still union looks more fit than Instruction because they are just separated types sharing a same space. Will that be too much painful to use union than enum for this?

IMO yes it will. And we wouldn't gain a lot from it tbh as the instruction loop will only have RealInstruction anyway (which is what we had until now)

Do you want me to remove the #[repr(u16)] so the compiler can have some further optimizations?

hmm, if this is only used by compiler anyway, probably that could be better idea.

I was originally thinking

union Instruction {
    real: RealInstruction,
    pseudo: PseudoInstruction,
    raw: u16,
}

impl Instruction {
    fn real(&self) -> Option<RealInstruction> {
        if self.raw < ... { 
            Some(...)
        } else {
             ...
        }
    }
}

btw, if The pseudo instruction is only used for compiler, vm only will have RealInstruction. That looks a bit awkward. How about keeping the RealInstruction name as Instruction, and the wrapper as AnyInstruction?

enum AnyInstruction {
    Real(Instruction),
    Pseudo(PseudoInstruction),
}

hmm, if this is only used by compiler anyway, probably that could be better idea.

I was originally thinking

union Instruction {
    real: RealInstruction,
    pseudo: PseudoInstruction,
    raw: u16,
}

impl Instruction {
    fn real(&self) -> Option<RealInstruction> {
        if self.raw < ... { 
            Some(...)
        } else {
             ...
        }
    }
}

While it's cool (and doable) I don't think we will gain much from doing that.

btw, if The pseudo instruction is only used for compiler, vm only will have RealInstruction. That looks a bit awkward. How about keeping the RealInstruction name as Instruction, and the wrapper as AnyInstruction?

enum AnyInstruction {
    Real(Instruction),
    Pseudo(PseudoInstruction),
}

Sure, I can do that