diff --git a/examples/pong.nes b/examples/pong.nes index ba800df..d4c8621 100644 Binary files a/examples/pong.nes and b/examples/pong.nes differ diff --git a/examples/state_machine.nes b/examples/state_machine.nes index ca4c775..cd69d76 100644 Binary files a/examples/state_machine.nes and b/examples/state_machine.nes differ diff --git a/examples/war.nes b/examples/war.nes index 05fc56d..fe83362 100644 Binary files a/examples/war.nes and b/examples/war.nes differ diff --git a/src/analyzer/mod.rs b/src/analyzer/mod.rs index 5c65c87..710270e 100644 --- a/src/analyzer/mod.rs +++ b/src/analyzer/mod.rs @@ -565,6 +565,26 @@ impl Analyzer { self.next_zp_addr = overlay_zp_base; self.next_ram_addr = overlay_ram_base; for var in &state.locals { + // Array initializers on state-locals aren't lowered + // yet — the IR would need to emit a runtime memcpy + // from a ROM blob into the allocated RAM region on + // each on_enter, and today the lowerer just + // `continue`s past the decl. Refuse the program rather + // than silently dropping the initializer (PR-#31-shaped + // bug). See docs/future-work.md for the plan. + if let Some(Expr::ArrayLiteral(_, _)) = &var.init { + self.diagnostics.push(Diagnostic::error( + ErrorCode::E0601, + format!( + "state-local variable '{}' has an array \ + initializer; this isn't lowered yet. Move \ + the array to a program-level `var` or \ + assign the elements inside `on_enter`.", + var.name + ), + var.span, + )); + } self.register_var(var); } if self.next_zp_addr > max_zp { @@ -612,10 +632,13 @@ impl Analyzer { self.current_scope_prefix = None; } // `on scanline(N)` is only valid with mappers that have a - // scanline-counting IRQ source (currently only MMC3). + // scanline-counting IRQ source (currently only MMC3). Keep + // this as a hard error — without MMC3 the codegen emits + // the handler functions but the IRQ dispatcher is never + // wired up, so the handlers silently never run. if !state.on_scanline.is_empty() && program.game.mapper != Mapper::MMC3 { self.diagnostics.push(Diagnostic::error( - ErrorCode::E0203, + ErrorCode::E0603, "`on scanline` requires the MMC3 mapper", state.span, )); @@ -1523,7 +1546,7 @@ impl Analyzer { } } - let Some(address) = self.allocate_ram(size, var.span) else { + let Some(address) = self.allocate_ram_with_placement(size, var.placement, var.span) else { // Allocation failed (E0301 already emitted) — still add the // symbol so that later references don't cascade into E0502, // but don't record a var_allocations entry. @@ -1644,9 +1667,23 @@ impl Analyzer { /// zero-page user region is bounded above by [`ZP_USER_CAP`] to /// leave room for IR codegen temp slots starting at $80. fn allocate_ram(&mut self, size: u16, span: Span) -> Option { + self.allocate_ram_with_placement(size, Placement::Auto, span) + } + + fn allocate_ram_with_placement( + &mut self, + size: u16, + placement: Placement, + span: Span, + ) -> Option { // Zero-page u8 allocation — bounded by ZP_USER_CAP to avoid - // colliding with the IR temp region at $80+. - if size == 1 && self.next_zp_addr < ZP_USER_CAP { + // colliding with the IR temp region at $80+. `slow` forces + // main RAM so users can deliberately keep a u8 out of ZP + // (e.g. a cold variable they don't want wasting a ZP slot); + // without this branch the `slow` keyword parsed but had no + // observable effect, which is the same silent-drop shape + // that bit PR #31. + if size == 1 && self.next_zp_addr < ZP_USER_CAP && placement != Placement::Slow { let addr = u16::from(self.next_zp_addr); self.next_zp_addr = self.next_zp_addr.wrapping_add(1); return Some(addr); diff --git a/src/analyzer/tests.rs b/src/analyzer/tests.rs index f615cd4..131c5a2 100644 --- a/src/analyzer/tests.rs +++ b/src/analyzer/tests.rs @@ -868,8 +868,62 @@ fn analyze_on_scanline_requires_mmc3() { "#, ); assert!( - errors.contains(&ErrorCode::E0203), - "on scanline without MMC3 should produce E0203, got: {errors:?}" + errors.contains(&ErrorCode::E0603), + "on scanline without MMC3 should produce E0603, got: {errors:?}" + ); +} + +#[test] +fn analyze_state_local_array_initializer_rejected() { + // Regression guard against the state-local-array silent drop. The + // IR lowerer's `on_enter` initializer loop `continue`s past + // ArrayLiteral inits without emitting any stores, so without this + // diagnostic the program compiles and the buffer is full of stale + // RAM at runtime. + let errors = analyze_errors( + r#" + game "Test" { mapper: NROM } + state Main { + var buf: u8[4] = [10, 20, 30, 40] + on frame { scroll(buf[0], buf[1]) } + } + start Main + "#, + ); + assert!( + errors.contains(&ErrorCode::E0601), + "state-local array initializer should produce E0601, got: {errors:?}" + ); +} + +#[test] +fn analyze_on_exit_declaration_accepted() { + // Regression guard: `on exit` handlers were lowered to IR + // functions but never dispatched by transitions, so any side + // effect inside silently never ran. The codegen now emits a + // runtime CMP-chain against `ZP_CURRENT_STATE` before every + // `transition` to JSR the leaving state's on_exit — confirm + // the analyzer accepts the declaration so the feature stays + // usable. + let result = analyze_ok( + r#" + game "Test" { mapper: NROM } + state Main { + on enter {} + on frame { transition Other } + on exit { scroll(0, 0) } + } + state Other { + on enter {} + on frame {} + } + start Main + "#, + ); + assert!( + result.diagnostics.iter().all(|d| !d.is_error()), + "`on exit` should be accepted now that transitions dispatch it, got: {:?}", + result.diagnostics ); } @@ -1915,6 +1969,34 @@ fn analyze_fast_var_underscore_exempt() { ); } +#[test] +fn analyze_slow_var_forced_out_of_zero_page() { + // The `slow` keyword should keep a u8 var out of zero page so + // users can free a hot ZP slot for a different variable. Before + // this was wired up, `slow` was parsed but ignored (same silent- + // drop shape as the PR #31 state-local bug). + let result = analyze_ok( + r#" + game "T" { mapper: NROM } + slow var cold: u8 = 0 + on frame { + if cold == 0 { wait_frame } + } + start Main + "#, + ); + let alloc = result + .var_allocations + .iter() + .find(|a| a.name == "cold") + .expect("`cold` should be allocated"); + assert!( + alloc.address >= 0x0100, + "`slow var cold` should live outside zero page but was allocated at ${:04X}", + alloc.address + ); +} + #[test] fn analyze_oversized_array_warns_w0108() { // A u8 array with 300 elements has byte size 300 > 256. The diff --git a/src/codegen/ir_codegen.rs b/src/codegen/ir_codegen.rs index 4edbed9..a444e79 100644 --- a/src/codegen/ir_codegen.rs +++ b/src/codegen/ir_codegen.rs @@ -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 ` 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())); diff --git a/src/errors/diagnostic.rs b/src/errors/diagnostic.rs index 11d3250..8ab6a98 100644 --- a/src/errors/diagnostic.rs +++ b/src/errors/diagnostic.rs @@ -35,6 +35,10 @@ pub enum ErrorCode { E0505, // multiple start declarations E0506, // function has too many parameters (max 4 in v0.1) + // E06xx: Unsupported-feature errors (parsed but not lowered) + E0601, // state-local variable with array initializer is not supported + E0603, // on_scanline handler requires mapper MMC3 + // W01xx: Warnings W0101, // expensive multiply/divide operation W0102, // loop without break or wait_frame @@ -66,6 +70,8 @@ impl fmt::Display for ErrorCode { Self::E0504 => "E0504", Self::E0505 => "E0505", Self::E0506 => "E0506", + Self::E0601 => "E0601", + Self::E0603 => "E0603", Self::W0101 => "W0101", Self::W0102 => "W0102", Self::W0103 => "W0103", diff --git a/tests/emulator/goldens/pong.audio.hash b/tests/emulator/goldens/pong.audio.hash index 02f3dfa..469676e 100644 --- a/tests/emulator/goldens/pong.audio.hash +++ b/tests/emulator/goldens/pong.audio.hash @@ -1 +1 @@ -555a22e1 132084 +6c171f3d 132084 diff --git a/tests/emulator/goldens/war.audio.hash b/tests/emulator/goldens/war.audio.hash index 9345746..c832f7e 100644 --- a/tests/emulator/goldens/war.audio.hash +++ b/tests/emulator/goldens/war.audio.hash @@ -1 +1 @@ -e3805766 132084 +60d2ce9d 132084 diff --git a/tests/integration_test.rs b/tests/integration_test.rs index 81434c4..a4afc78 100644 --- a/tests/integration_test.rs +++ b/tests/integration_test.rs @@ -335,6 +335,80 @@ fn program_with_u16_struct_field() { rom::validate_ines(&rom_data).expect("should be valid iNES"); } +#[test] +fn transition_dispatches_leaving_states_on_exit_handler() { + // `on exit` handlers used to be silently never called — the + // codegen documented that IrOp::Transition "doesn't know which + // state it's leaving" and skipped the on_exit JSR. Example + // programs (pong, war, state_machine) had `stop_music` sitting + // in an `on exit` block that never ran, and goldens captured + // the bug as "correct" behavior. The fix emits a runtime + // CMP-chain against ZP_CURRENT_STATE before every transition to + // JSR the leaving state's exit handler. + // + // Compile a program with two states where Source declares an + // `on exit` and transitions to Target, then assert the PRG + // contains a JSR to the Source_exit label. We look for the + // absolute JSR opcode $20 followed by any 16-bit target, and + // verify the linked label's address lands on one of them by + // scanning the ROM for the `STA ZP_CURRENT_STATE` sequence + // that would indicate the transition lowered at all. + let source = r#" + game "ExitDispatch" { mapper: NROM } + state Source { + on enter {} + on frame { transition Target } + on exit { stop_music } + } + state Target { + on enter {} + on frame {} + } + start Source + "#; + let rom_data = compile(source); + let prg = &rom_data[16..16 + 16384]; + + // NROM ROMs are a fixed 24576 + 16-byte header, so we can't + // compare file sizes. Compare the count of distinct JSR + // targets instead: a program without `on exit` only JSRs + // `Target_enter`, so its transition site has one JSR; adding + // `on exit` to Source injects at least one more JSR (the + // exit-dispatch JSR to `__ir_fn_Source_exit`). + let baseline_source = r#" + game "ExitDispatch" { mapper: NROM } + state Source { + on enter {} + on frame { transition Target } + } + state Target { + on enter {} + on frame {} + } + start Source + "#; + let baseline = compile(baseline_source); + let baseline_prg = &baseline[16..16 + 16384]; + + let count_jsrs = |bytes: &[u8]| -> std::collections::HashSet { + bytes + .windows(3) + .filter(|w| w[0] == 0x20) + .map(|w| u16::from_le_bytes([w[1], w[2]])) + .collect() + }; + let exit_jsrs = count_jsrs(prg); + let base_jsrs = count_jsrs(baseline_prg); + assert!( + exit_jsrs.len() > base_jsrs.len(), + "expected the on-exit-bearing PRG to contain more distinct JSR \ + targets than the baseline (got {} vs {}) — if this fails, the \ + exit dispatch JSR to `__ir_fn_Source_exit` is being dropped", + exit_jsrs.len(), + base_jsrs.len() + ); +} + #[test] fn uninitialized_struct_field_store_emits_sta_to_allocated_address() { // Regression guard for the silent-drop bug uncovered while