From 48bae97c51e2112f6e8b77d5b465bb778ace9136 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 18 Apr 2026 00:06:39 +0000 Subject: [PATCH] analyzer+codegen: turn silently-dropped feature paths into hard failures (or fix them) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- examples/pong.nes | Bin 24592 -> 24592 bytes examples/state_machine.nes | Bin 24592 -> 24592 bytes examples/war.nes | Bin 24592 -> 24592 bytes src/analyzer/mod.rs | 47 +++++++++++-- src/analyzer/tests.rs | 86 ++++++++++++++++++++++- src/codegen/ir_codegen.rs | 90 ++++++++++++++++++++----- src/errors/diagnostic.rs | 6 ++ tests/emulator/goldens/pong.audio.hash | 2 +- tests/emulator/goldens/war.audio.hash | 2 +- tests/integration_test.rs | 74 ++++++++++++++++++++ 10 files changed, 281 insertions(+), 26 deletions(-) diff --git a/examples/pong.nes b/examples/pong.nes index ba800df8178db561473c69f8e70caf64b2e31695..d4c8621f4ed8123fc117b17af53ddb02e2f0f1d7 100644 GIT binary patch delta 714 zcmZvZ&1(}u7{+&Yve~R@wz26}(0oj^HA%%#LGV(r;0IU+Zyu#s5D$9MgHW(kC1X)p ztSor!MMjvQPz>}INU@6BQ*^(?Qt?k%J?Kf|n~$cc2s^{P@4Ua~d3JZDfmYzTr`%++ z2%bN1w7{|DCZ?KKyNcA99%n-$UgPRUezz^$9k>FuF6k+Ojb`ZohcwWeDDQAm#tSLg@1*&M@}s5_cDL0qQwqyRJtEx zns(rj)Ff8nm9Wh7Ec2#-ODz^5w>LPprDl}GKGE~-7mAWUE}R5nm^^W4d4nIdPp)CQ z>yV;p5WXS&Ah3o?Lfrbl~1HgJ8 zSRVjRb7D*2-fT}3>b6}7tZXM~TTwD6uz4JdDpSQ=_s3YTN3V^~c>|xA>;+r!c7rW= z))6C9SoL`2{lhgyNR)d5g#)wo{%qZ|E*W`Q^Q~LP+1wT1ZiN?U(XVo~)L;?Vr)OuC yh&E+LX2i0!WlW|X`OrmSoGVP{Q8r9deoY-qBeYc;KQv#AJ`JSC)R&jZ8S)oI#wf%9 delta 606 zcmZ8dO=uHA6yDj{?RI0c{Ud1)O)^nx8>OYUdMMb7Ih0X)7ljCdS3QVep<46M(k`|P zl%5pSDNeLd%~?pWhVH@N-AxYxuU=OLJ;v82L@mrb=6&DyzW3&>Rmd8i1@jx|s=j{L zw_o_4{)zHeLHQD~x}E#bqv{Y>tL`sL)yp?*n>vea?A{?JmU|lHwzwMS7+=S~I_qtE zvqe90^3LNnwAFI?;Usly8yqmvvbTSOkTYUyVC<-IW8ie*|6cBIOEn=TN$yTW`G}8? zK3jvLEJnjJ3#sifnrni=u2&TS$}anSvMP^z6FZb&^pyrF$R3plF?e(Fr_a8>WnXsL zbWLWw8I(u9WN5Uzhem0v%y0-~+9TL@C}4#>cH#qj^08CJ@-?Haoi4fEx?w4xvFHIhK{!mV8#~?DjDAN;Yj#DrMaWj zJxNswEY)Z&RTznaV}<{$CnlB?ss5=@7T+!Hm`dZ*p(0UCyia6x2t|}QoB14i&FCC| z7M7%ytAK%VW^T?j>?zlnRa9I^o*LBah?3}-G{KQw(90i&<(1?JA^Gz2p)2EJez@3W N?Q4~n`%n93*&mo7{Ko(Q diff --git a/examples/state_machine.nes b/examples/state_machine.nes index ca4c77536f68813c1a277a36e6e6d435596d0850..cd69d764c6432e79ee18e49a577c45744c3726c3 100644 GIT binary patch delta 173 zcmbPmfN=s4)iBmitTpD0K6sMp0<#a_p~)W@oE#3C0|QghL4BVTV>S j0F|#f08uW0ti0$D#H!6cOii||^A9l`xVf3%fl&bf+(1I# delta 112 zcmbPmfN=s4)i63wtTpEJKX{Vq0<+JngA;d_v2(3tY-Lu^o$SO|DY@z3QUQrfu2zAS zOhB>62Yq@EEM?SG_`o&!4P%ak!J(B5K;bn9Ai@G4pjtO~Fg4juVzyyTKE!Zf?q+@m GMg;(T-YIqf diff --git a/examples/war.nes b/examples/war.nes index 05fc56df603f9b4dc462e14e0f21e4afcc1825d2..fe83362c8afc7339e68b6237d4fd4b0921e8be9e 100644 GIT binary patch delta 1842 zcmZuyeP|nH7|-41l1rMl(KcO|uI<}quFHmLw{>l)gT}%9W8R=R^bhrq3gQSua0sH< znaKL0g9cmAKU5}u@jBi@$JTZrlu5s~HHF+=KAKGE6j9ui%CZU-oKT4 zM*kgO8g@tGF5;2F93Tg9>*OWDzlE*n+&mFt7Tt!{R^-ER2w_7_X*f|13+S!5?42hE zV^3ZcmdO@2Z;&t^D4ax*B+n@9T)1K$NP zuWM8Nl zm=5U5U+y+90_7b#70b;cn2#qE&W*z7bvkG^^&*>BTuncTZrW)!`j4G%VJEfasiZf? zGQOhIE|c(OfiO`)u$u(dmVpG>ZItAmj-Fe9k?ktA^ln%$_Vlh!1?VZCX4Bkc5OyU{ z*8hkwm_2%HQtR#`8uTXF0|Wm>-mN_rBe; z38uQjD1osy3DrfCUoyt`kuv^RU4tu09pz3Ofcq%Ly3oSLYB*^Wf^EmCB=RmBS+d(p z%E05hIRoUeE0Fx^thjPqj!=N{vPx5aS+=iNLk85_3xzV+pw*J zSOl{T_1}^|Oe>3Zz84c|v#g70(l~L$BH%s)%a*^-SbkZ}v*8K6_GE6=?D1xrcqeXj z??B;Jm0*4HRC2Yj^CM&=he|85zFMsQ`YI?A(CiA^kVCNkgv-0oIESGXMYp delta 1514 zcmZ`(TWB0r7@nQk*;}`}lVmg5Y_sV}V$!U!>Bes2Wed%VHrf$`w6xTh3VkURP4UG- zF_HElK8$XA5T%Hh%0W&bHM)5aOCdMYOkij4DWw#VTG4886SO{v#Q)5$n~jUGu;TA@MH;#R5~eOX(jUm zb6r?Gs~k!hVn))An1%#@FXfY?HosfGh`nr;obVj1oMt?|&U@I0Wwx5UuMUKb>p?duTt70b!8Fj8I7g>4g-HJ^phD z53&%B!BfF{dNaikwzFX@qbrMau=4b`6hXeI5;Fl9qWuw@Awd5Ta@^PAI$$!9S54mY z#m5bkBH&2cHzw_hl#d^q%mtFmAc?M1TVw*@X%h~LTfpb&xsrs!E7}qei~!WNWb*41 zdD>~%Ao6z;O)lmby}JZmR3)Kv}@A@sF0dET#jPo%4<`^ag3(=+?h z)U}yRJDBE0{F_@l#;=>DoLbSwK)@80;*wAqp zL1A~(Os<7xfD0KK27lY6+m*sVB0bKxo z3Sh@T#AWT1UKSsP!9brZ@8sLz%UON%ef}8Np}q#MfWGboP43pAO;UHRmb8~lHMZ1qco&y8Ij6R8&T!6nT^uY;K|egGZl$t7(*Pu>o6HEt_Zhx~V*1}CoxWG&DX zauuw2*ai4hXz@aI_EGXnpjC_)h@&~z;Vo8Puk-YL$pgv~y1K%*a=@5@=5`lJx7s1T zS|lUte)rX)u~B+Td#6YeYO@@I5A!OSRC}6!EmCL1R@z2o?4u6s*Ul8l9kowvT}>)2 cPs?Jom^|9{RoBJvx?|*)Fnj;jXUZ`82cnE{cmMzZ 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