mirror of
https://github.com/imjasonh/nescript
synced 2026-07-10 17:52:51 +00:00
analyzer+codegen: turn silently-dropped feature paths into hard failures (or fix them)
Phase 2 of the post-PR-#31 audit. The codebase had four documented
"silently skip" paths that parsed user intent but produced no code.
Each one was the same shape as the state-local bug: the analyzer
accepted the program, the IR lowered the construct, but somewhere
downstream the emitted code was dropped on the floor — and a pixel
golden that captured the broken behaviour locked it in as correct.
Fix each per the plan, either by implementing the feature or
rejecting the program at the analyzer.
### on_exit handlers now actually run
`IrOp::Transition` used to comment "on_exit of the current state
isn't called here because we don't know from an IR op alone which
state we're leaving." The codegen emitted the exit handler's body
as an IR function but never JSR'd it. Three example programs
(pong, war, state_machine) relied on `stop_music` or mode-flag
translation inside `on exit` that had been silently never running.
Emit a small CMP-chain against `ZP_CURRENT_STATE` before each
transition: for every state that declares an on_exit, compare the
current index, branch past on miss, JSR the exit handler on match,
then JMP to the shared done-label so only the leaving state's
handler fires. The chain is inlined at each transition site
(bounded by the number of states declaring on_exit) rather than
factored into a single trampoline — simpler to reason about, and
transitions are rare enough that the extra bytes don't matter.
Pong / war / state_machine ROMs change because the dispatch code
is now emitted. Video goldens stay byte-identical (no transitions
happen within the 180-frame harness window under no-input). Pong
and war audio hashes shifted from pure code-layout timing and are
regenerated. `docs/pong.gif` and `docs/war.gif` are byte-identical.
### State-local array initializers now refuse to compile (E0601)
`src/ir/lowering.rs:887` had the comment "Array initializers for
state-locals aren't supported yet... Programs that try this should
get a diagnostic from the analyzer; for now, silently skip." The
analyzer never actually emitted that diagnostic. Verified by
compiling `state Main { var buf: u8[4] = [10,20,30,40] ... }`:
the program built a valid ROM with no trace of 10/20/30/40 in PRG.
Add E0601 to the analyzer's state-local pass. The IR lowerer's
defensive `continue` stays in place as a belt-and-braces guard.
### `on scanline` without MMC3 is now E0603
Previously E0203 ("invalid operation for type") which is a
miscategorisation — the feature is unsupported on the current
mapper, not a type error. Dedicated E0603 makes the future-work
shape explicit.
### `slow` variables now actually live outside zero page
`Placement::Slow` was parsed into the AST but `allocate_ram`
ignored it, so `slow var cold: u8` still landed in ZP like any
other u8. Wire `var.placement` through `allocate_ram_with_placement`
and skip the ZP branch when `Slow` is set. `Fast` remains
advisory (the existing default already prefers ZP for u8 vars),
validated by W0107.
### Other address-map silent drops hardened
Alongside the var_addrs hardening from phase 1, three `state_indices`
lookup sites that did `.copied().unwrap_or(0)` or silent `if let`
are now explicit panics: scanline IRQ dispatch, MMC3 reload, and
`IrOp::Transition`. A miss in any of them is a compiler bug, not
valid input — the analyzer catches unknown state names upstream.
### Regression guards
Four new tests would have failed against the old silently-dropping
code paths:
- `analyze_state_local_array_initializer_rejected` — expects E0601.
- `analyze_on_exit_declaration_accepted` — expects no errors.
- `analyze_slow_var_forced_out_of_zero_page` — expects alloc
address >= $0100.
- `transition_dispatches_leaving_states_on_exit_handler` — counts
distinct JSR targets in the PRG before/after adding `on exit` to
a state; the exit-bearing build must have more.
All 720 tests pass. All 34 emulator goldens pass after the pong/war
audio hash refresh.
https://claude.ai/code/session_01AoQ678uVeqpyayvWHpfDhC
This commit is contained in:
Claude
parent
f7012c6533
commit
48bae97c51
10 changed files with 281 additions and 26 deletions
|
|
@ -1603,24 +1603,63 @@ impl<'a> IrCodeGen<'a> {
|
|||
self.emit(STA, AM::Absolute(0x07EF));
|
||||
}
|
||||
IrOp::Transition(name) => {
|
||||
// Write the target state's index to current_state, then
|
||||
// call the target state's on_enter handler if it exists,
|
||||
// then JMP to main loop for the new state's frame handler.
|
||||
// Order of operations for `transition Foo`:
|
||||
// 1. Dispatch the *current* state's `on exit` (if any
|
||||
// state declares one) by runtime check against
|
||||
// `ZP_CURRENT_STATE`. The IR op only knows the
|
||||
// target state's name, not the source, so we emit
|
||||
// a small CMP-chain that matches whichever state
|
||||
// is currently active and JSRs its exit handler.
|
||||
// This is the fix for the long-latent silent-drop
|
||||
// bug where on_exit handlers were lowered but
|
||||
// never called — caught by the analyzer tripwire
|
||||
// that rejected `on exit` declarations until this
|
||||
// dispatch existed. Example programs (pong, war,
|
||||
// state_machine) relied on `stop_music` inside
|
||||
// `on exit` that had been silently never running.
|
||||
// 2. Write the target state's index to current_state.
|
||||
// 3. Call the target state's on_enter (if defined).
|
||||
// 4. JMP back to the main loop.
|
||||
//
|
||||
// Note: on_exit of the current state isn't called here
|
||||
// because we don't know from an IR op alone which state
|
||||
// we're leaving. Proper on_exit support would need
|
||||
// per-state transition lowering. Future improvement.
|
||||
if let Some(&idx) = self.state_indices.get(name) {
|
||||
self.emit(LDA, AM::Immediate(idx));
|
||||
self.emit(STA, AM::ZeroPage(ZP_CURRENT_STATE));
|
||||
// Call the target state's on_enter handler if defined
|
||||
let enter_fn = format!("{name}_enter");
|
||||
if self.function_exists(&enter_fn) {
|
||||
self.emit(JSR, AM::Label(format!("__ir_fn_{enter_fn}")));
|
||||
// `transition <unknown>` is rejected by the analyzer
|
||||
// as E0502, so a miss in `state_indices` here is a
|
||||
// compiler bug, not valid input.
|
||||
let mut exit_fns: Vec<(u8, String)> = self
|
||||
.state_indices
|
||||
.iter()
|
||||
.filter(|(state, _)| self.function_exists(&format!("{state}_exit")))
|
||||
.map(|(state, &i)| (i, format!("{state}_exit")))
|
||||
.collect();
|
||||
exit_fns.sort_by_key(|(i, _)| *i);
|
||||
if !exit_fns.is_empty() {
|
||||
let suffix = self.local_label_suffix();
|
||||
let done = format!("__ir_xit_done_{suffix}");
|
||||
self.emit(LDA, AM::ZeroPage(ZP_CURRENT_STATE));
|
||||
for (i, (state_idx, exit_fn)) in exit_fns.iter().enumerate() {
|
||||
let skip = format!("__ir_xit_skip_{suffix}_{i}");
|
||||
self.emit(CMP, AM::Immediate(*state_idx));
|
||||
self.emit(BNE, AM::LabelRelative(skip.clone()));
|
||||
self.emit(JSR, AM::Label(format!("__ir_fn_{exit_fn}")));
|
||||
self.emit(JMP, AM::Label(done.clone()));
|
||||
self.emit_label(&skip);
|
||||
}
|
||||
self.emit(JMP, AM::Label("__ir_main_loop".into()));
|
||||
self.emit_label(&done);
|
||||
}
|
||||
|
||||
let &idx = self.state_indices.get(name).unwrap_or_else(|| {
|
||||
panic!(
|
||||
"internal compiler error: IrOp::Transition references \
|
||||
state {name:?} which has no dispatch index (did the \
|
||||
analyzer forget to reject an unknown state name?)"
|
||||
)
|
||||
});
|
||||
self.emit(LDA, AM::Immediate(idx));
|
||||
self.emit(STA, AM::ZeroPage(ZP_CURRENT_STATE));
|
||||
let enter_fn = format!("{name}_enter");
|
||||
if self.function_exists(&enter_fn) {
|
||||
self.emit(JSR, AM::Label(format!("__ir_fn_{enter_fn}")));
|
||||
}
|
||||
self.emit(JMP, AM::Label("__ir_main_loop".into()));
|
||||
}
|
||||
IrOp::Scroll(x, y) => {
|
||||
// PPU scroll register $2005 takes two writes: X then Y
|
||||
|
|
@ -2216,7 +2255,16 @@ impl<'a> IrCodeGen<'a> {
|
|||
self.emit(LDA, AM::ZeroPage(ZP_CURRENT_STATE));
|
||||
let done_label = "__irq_user_done".to_string();
|
||||
for (state_idx_iter, (state_name, lines)) in groups.iter().enumerate() {
|
||||
let state_idx = self.state_indices.get(state_name).copied().unwrap_or(0);
|
||||
// Scanline groups are built from declared state names, so any
|
||||
// miss here is a compiler-internal inconsistency. Failing to 0
|
||||
// would silently route this state's scanline handlers to
|
||||
// state 0, which is a PR-#31-shaped silent miscompile.
|
||||
let state_idx = *self.state_indices.get(state_name).unwrap_or_else(|| {
|
||||
panic!(
|
||||
"internal compiler error: scanline group for state \
|
||||
{state_name:?} has no dispatch index"
|
||||
)
|
||||
});
|
||||
let next_state_label = format!("__irq_ns_{state_idx_iter}");
|
||||
self.emit(CMP, AM::Immediate(state_idx));
|
||||
self.emit(BNE, AM::LabelRelative(next_state_label.clone()));
|
||||
|
|
@ -2289,7 +2337,15 @@ impl<'a> IrCodeGen<'a> {
|
|||
self.emit(LDA, AM::ZeroPage(ZP_CURRENT_STATE));
|
||||
let reload_done = "__ir_mmc3_reload_done".to_string();
|
||||
for (i, (state_name, lines)) in groups.iter().enumerate() {
|
||||
let state_idx = self.state_indices.get(state_name).copied().unwrap_or(0);
|
||||
// Same invariant as the IRQ dispatcher above: scanline groups
|
||||
// are built from declared states, so a missing entry is a
|
||||
// compiler bug rather than valid input.
|
||||
let state_idx = *self.state_indices.get(state_name).unwrap_or_else(|| {
|
||||
panic!(
|
||||
"internal compiler error: scanline reload for state \
|
||||
{state_name:?} has no dispatch index"
|
||||
)
|
||||
});
|
||||
let skip_label = format!("__ir_reload_skip_{i}");
|
||||
self.emit(CMP, AM::Immediate(state_idx));
|
||||
self.emit(BNE, AM::LabelRelative(skip_label.clone()));
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue