Skip to content

CSE Machine: no clean way to add a new instruction type without hacky workarounds across two repos #4079

Description

@Akshay-2007-1

The problem

Adding a genuinely new instruction type to the CSE machine (say, from py-slang) is way harder than it should be, and the failure mode when you get it wrong is silent and confusing: it just shows up as the literal text "INSTRUCTION" with no error anywhere telling you what went wrong.

@AaravMalani ran into this directly while adding a MODULE_FUNCTION_CALL instruction type to py-slang. Here's the full chain of what's actually going on, since it took a fair bit of digging to trace:

  1. py-slang serializes control items over the Conductor channel with a metadata.instrType field, which is supposed to be one of js-slang's own InstrType enum string values (from js-slang/dist/cse-machine/types), via a PY_TO_JS_INSTR_TYPE lookup table in PyCseMachinePlugin.ts.
  2. The frontend's CseSnapshotAdapter.ts reconstructs the control item on the JS side using only instrType/symbol/numOfArgs/arity/srcNode from that metadata. The raw displayText string py-slang sent is thrown away entirely once metadata.instrType is set, so writing custom display text in py-slang's own instrDisplayText() does nothing for these.
  3. The actual rendered label comes from CseMachineUtils.ts's getControlItemComponent(), which is a switch (controlItem.instrType) over js-slang's own closed InstrType enum. If your instruction doesn't map to one of the ~20 existing js-slang values, it silently falls into default: return new ControlItemComponent('INSTRUCTION', 'INSTRUCTION', ...). No warning, no error, just a generic unhelpful label.
  4. If you want a real animation too, there's a second, separate switch on instrType in CseMachineAnimation.tsx's updateAnimation(). And a third (lower stakes, just drives a "stash item about to be popped" highlight hint) in isStashItemInDanger() back in CseMachineUtils.ts.
  5. Here's the kicker: since ControlItem/InstrType are imported directly from js-slang and are a closed TypeScript enum, you can't even add case 'YourNewType': to any of these switches without a type error, because TypeScript correctly says there's no overlap between the enum and an arbitrary string literal. The only way to add a genuinely new instruction type is to cast the switch discriminant to string at each of these three sites, which is an easy thing to not know you need to do and isn't documented anywhere.

So today you have two bad options for a new instruction:

  • Map it to an existing js-slang InstrType (e.g. Application) and lose any custom display text or distinct animation, and also remember that numOfArgs/other metadata population in serializeControlItem() is gated on py-slang's own instrType checks, not the js-slang string you mapped to, so that needs updating too if you want that data to actually reach the frontend.
  • Add a genuinely new type and have to know to touch three separate switch statements across two repos, plus work around the closed-enum typing issue at each one, with zero guidance or errors pointing you there when you get it wrong.

Why this matters

This isn't just a Python problem. Any language plugged into Conductor's CSE machine (there's already talk of migrating js-slang itself onto Conductor) will hit the exact same wall the moment it needs an instruction that doesn't map cleanly onto js-slang's original 20-ish types. Right now the "extension point" for new instruction types is an external closed enum that was never designed to be extended, which seems backwards for something that's explicitly meant to be a shared, language-agnostic visualization.

What might help

Not trying to prescribe the exact fix here since it probably needs some real design thought, but a few directions that seem worth considering:

  • A proper language-agnostic instruction type concept (maybe living in @sourceacademy/common-cse-machine alongside the rest of the shared protocol) instead of reusing js-slang's own internal enum as the de facto extension point.
  • Whatever that ends up being, it should fail loudly (a console warning at minimum) when an unrecognized instruction type shows up, instead of silently rendering "INSTRUCTION" with no trace of what actually happened.
  • Some documentation on what actually needs to change across both repos to add a new instruction type, since right now it's three separate switch statements you'd only find by grepping around, discovered this myself while helping debug the exact issue above.

Happy to help however's useful here, just wanted to get this written down properly before it's forgotten.

Metadata

Metadata

Labels

_infrastructureRelating to project infrastructure_refactornice-to-haveIssue is a non-critical, non-important

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions