diff --git a/examples/arrays_and_functions.nes b/examples/arrays_and_functions.nes index 71509e6..185c76d 100644 Binary files a/examples/arrays_and_functions.nes and b/examples/arrays_and_functions.nes differ diff --git a/examples/coin_cavern.nes b/examples/coin_cavern.nes index 3e72b7f..d0a9d0b 100644 Binary files a/examples/coin_cavern.nes and b/examples/coin_cavern.nes differ diff --git a/examples/function_chain.nes b/examples/function_chain.nes index 6471e48..6af9715 100644 Binary files a/examples/function_chain.nes and b/examples/function_chain.nes differ diff --git a/examples/inline_asm_demo.nes b/examples/inline_asm_demo.nes index caee039..34b6fc0 100644 Binary files a/examples/inline_asm_demo.nes and b/examples/inline_asm_demo.nes differ diff --git a/examples/loop_break_continue.nes b/examples/loop_break_continue.nes index a3792b9..87a7f16 100644 Binary files a/examples/loop_break_continue.nes and b/examples/loop_break_continue.nes differ diff --git a/examples/mmc1_banked.nes b/examples/mmc1_banked.nes index e5e1109..fbeb3ad 100644 Binary files a/examples/mmc1_banked.nes and b/examples/mmc1_banked.nes differ diff --git a/examples/platformer.nes b/examples/platformer.nes index ce8b799..a3bdec7 100644 Binary files a/examples/platformer.nes and b/examples/platformer.nes differ diff --git a/examples/war.nes b/examples/war.nes index daeda7e..bfdc2c1 100644 Binary files a/examples/war.nes and b/examples/war.nes differ diff --git a/examples/war/COMPILER_BUGS.md b/examples/war/COMPILER_BUGS.md index be072d4..4f6ab07 100644 --- a/examples/war/COMPILER_BUGS.md +++ b/examples/war/COMPILER_BUGS.md @@ -2,15 +2,30 @@ This document captures bugs and limitations discovered while building `examples/war.ne`. Each entry includes a minimal -reproduction, the symptom we observed, the root cause if known, -and a workaround we used in `examples/war/*.ne`. The intent is -to track these so they can be fixed in a future compiler pass — -once they are, the corresponding workarounds in `war/*.ne` -should be reverted to keep the example honest. +reproduction, the symptom we observed, the root cause, the +workaround originally used in `examples/war/*.ne`, and the +compiler fix that shipped (when shipped). + +## Status summary + +| # | Short name | Status | Fix commit | Regression test | +|---|---|---|---|---| +| 1 | `fun` with > 4 params silently drops the rest | **FIXED** (E0506 diagnostic) | `analyzer: reject functions with more than 4 parameters (E0506)` | `analyze_rejects_function_with_more_than_4_params`, `analyze_accepts_function_with_exactly_4_params` | +| 1b | Same-named params share VarIds across functions | **FIXED** (scope-qualified keys) | `analyzer/ir: scope function locals per function body` | `analyze_allows_same_param_name_in_two_functions` | +| 2 | Param transport slots $04-$07 clobbered by nested calls | **FIXED** (codegen prologue spill) | `codegen: spill parameters from $04-$07 into per-function RAM slots` | `codegen::ir_codegen::gen_function_prologue_spills_params_to_local_ram` | +| 3 | Function-local `var` declarations share one flat namespace | **FIXED** (scope-qualified keys) | `analyzer/ir: scope function locals per function body` | `analyze_allows_same_local_name_in_two_functions`, `analyze_allows_same_local_name_in_two_state_handlers`, `analyze_still_rejects_duplicate_local_in_same_function` | +| 4 | 8-sprites-per-scanline limit invisible to user code | Open (hardware limit; static analyzer hint could help) | — | — | +| 5 | `inline` keyword silently declined for short functions | Open | — | — | +| 6 | `wide_hi` IR map leaked between functions (u16→u8 aliasing) | **FIXED** (cleared per function) | `ir: clear wide_hi between functions to fix 16-bit op aliasing` | `ir::tests::wide_hi_does_not_leak_between_functions` | + +**Once a fix lands, revert the workaround in `examples/war/*.ne` +in the same commit** so the example keeps the game honest and +the PR diff visibly proves the fix works end-to-end. Bugs #1, +#1b, #2, #3, and #6 have had their workarounds reverted. --- -## 1. Functions with more than 4 parameters silently corrupt the 5th+ +## 1. Functions with more than 4 parameters silently corrupt the 5th+ *(FIXED)* ### Symptom @@ -94,7 +109,7 @@ Two reasonable options: --- -## 1b. Function parameters with the same name in different functions share a VarId, which collides their zero-page slot mapping +## 1b. Function parameters with the same name in different functions share a VarId, which collides their zero-page slot mapping *(FIXED)* ### Symptom @@ -171,24 +186,35 @@ function (see bug #3); we extended the same scheme to params: `pbb_card` / `dcf_card` snapshots from bug #2 stay because they also help with the bug-2 clobbering. -### Fix proposal +### Fix -Two layers to fix in: +Both the analyzer and the IR lowerer now qualify function-body +`var` / parameter declarations with the enclosing function name +(or state handler name) under an internal key +`"__local__{scope}__{name}"`. Each function's locals and +parameters therefore get **distinct** symbol-table entries and +VarIds even when the source names collide. -1. **IR lowering**: give every function its own `var_map` for - parameters and locals. The global `var_map` should only hold - top-level `var` / `const` / `enum` symbols. +Lookups inside a function body go through +`Analyzer::resolve_symbol` / `LoweringContext::scoped_key`, +which prefer the scope-qualified key over the bare one — so +a function-local `var x` correctly shadows a same-named global +(or another function's `var x`). -2. **Codegen**: even after the IR fix, the global `var_addrs` - `HashMap` should grow a per-function dimension (one map per - `IrFunction`) so two different functions can independently - assign their own VarIds to overlapping zero-page slots. +State-level locals (declared at `state Foo { var x: u8 }` +outside any handler) stay in the global namespace so every +handler in the state can read/write them across frames. -Either fix alone is probably enough; both together is robust. +See `src/analyzer/mod.rs::resolve_symbol` / `resolve_key` / +`scoped_name` and `src/ir/lowering.rs::scoped_key`. + +Together with fix #2 below, bugs #1b and #2 are completely +gone: the workaround-prefixed locals and params in `war/*.ne` +(the `dcf_`, `dwp_`, `pba_`, etc tags) are all reverted. --- -## 2. Function parameters share zero-page slots with nested calls — values clobbered across `JSR` +## 2. Function parameters share zero-page slots with nested calls — values clobbered across `JSR` *(FIXED)* ### Symptom @@ -231,24 +257,38 @@ throughout the body. See `war/render.ne::draw_card_face`, `war/render.ne::draw_flying_card`, `war/deck.ne::push_back_a`, `war/deck.ne::push_back_b`. -### Fix proposal +### Fix -1. **Spill on entry**: at the top of every function body that - makes a call, copy `$04..$07` into per-function RAM slots and - rewrite all parameter reads to load from the RAM copies. - Equivalent to what users are doing manually today. +`codegen::ir_codegen::IrCodeGen::new` now allocates every +function-local — including its parameters — into a dedicated +per-function RAM slot at `$0300+`. Parameters are still passed +via the zero-page transport slots `$04-$07` as the calling +convention, but `gen_function` now emits a 4-instruction +**prologue** at every function entry: -2. **Smarter scheduling**: only spill a parameter slot if it's - live across a call site (CFG-aware liveness pass on params). - Same effect, less RAM cost for short helpers that never read - their params after calling out. +``` +LDA $04 ; transport slot 0 +STA +LDA $05 ; transport slot 1 +STA +... etc ... +``` -Either fix would let users write straightforward function bodies -without having to remember the snapshot dance. +By the time the body runs, every parameter lives in the +function's dedicated RAM slot, so any nested call can freely +clobber `$04-$07` (passing its own arguments to _its_ callee) +without corrupting the caller's saved parameters. + +The cost is 4 LDA/STA pairs at every function entry (≈ 20 +bytes of ROM, 16 cycles). Worth it to make the calling +convention sound. + +See `codegen::ir_codegen::gen_function_prologue_spills_params_to_local_ram` +for the regression test. --- -## 3. Function-local variable names are in a flat global namespace +## 3. Function-local variable names are in a flat global namespace *(FIXED)* ### Symptom @@ -302,18 +342,23 @@ identifying its enclosing function (e.g. `dfa_card` in `dwp_px` in `draw_word_player`). This makes long files harder to read but is fully mechanical. -### Fix proposal +### Fix -Rework `register_var` to maintain a stack of scopes (one per -function body, one per nested block). Each `Statement::VarDecl` -inserts into the current scope. Lookup walks the stack from -innermost to outermost. The existing global symbol table is -unchanged for top-level globals / consts / fun names; only -function-locals shift to the scoped table. +Same as #1b: the analyzer and IR lowerer now internally +qualify function-body `var` declarations with the enclosing +scope's name, so `foo`'s `var i` and `bar`'s `var i` resolve +to `__local__foo__i` and `__local__bar__i` respectively. The +two entries coexist peacefully in the (still-flat) symbol +table. -A smaller intermediate fix: keep the flat table but qualify -each local's stored name as `::` so the global -table sees unique entries even when source names collide. +What *didn't* change: two `var i` declarations inside the +same function body still collide with E0501 (we scoped per +function body, not per nested block). That's a deliberate +trade-off — per-block scoping would require live-range +analysis to reuse RAM slots across blocks, which is a much +bigger change. The analyzer test +`analyze_still_rejects_duplicate_local_in_same_function` +pins this behaviour. --- diff --git a/examples/war/compare.ne b/examples/war/compare.ne index 319cc65..bd9f2fa 100644 --- a/examples/war/compare.ne +++ b/examples/war/compare.ne @@ -2,28 +2,24 @@ // // Cards are packed as `(rank << 4) | suit`; the extractors are // one shift or one mask each. -// -// Parameter names are unique across the entire program — see -// COMPILER_BUGS.md §1b for why same-named params in different -// functions silently corrupt each other through shared VarIds. -inline fun card_rank(crk_c: u8) -> u8 { - return crk_c >> 4 +inline fun card_rank(card: u8) -> u8 { + return card >> 4 } -inline fun card_suit(csu_c: u8) -> u8 { - return csu_c & 0x0F +inline fun card_suit(card: u8) -> u8 { + return card & 0x0F } // Compare two cards by rank. Returns: -// 1 if A wins, 2 if B wins, 0 if they tie -fun compare_cards(cmp_a: u8, cmp_b: u8) -> u8 { - var cmp_ra: u8 = card_rank(cmp_a) - var cmp_rb: u8 = card_rank(cmp_b) - if cmp_ra > cmp_rb { +// 1 if A wins, 2 if B wins, 0 if they tie. +fun compare_cards(a: u8, b: u8) -> u8 { + var ra: u8 = card_rank(a) + var rb: u8 = card_rank(b) + if ra > rb { return 1 } - if cmp_rb > cmp_ra { + if rb > ra { return 2 } return 0 diff --git a/examples/war/deal_state.ne b/examples/war/deal_state.ne index dd690d1..5e35486 100644 --- a/examples/war/deal_state.ne +++ b/examples/war/deal_state.ne @@ -47,36 +47,36 @@ state Deal { // deal_next counter controls how many of the 52 have // "landed", so the first half goes to A and the second // half to B — matching the actual split_decks() logic. - var dl_dealt_a: u8 = deal_next - var dl_dealt_b: u8 = 0 - if dl_dealt_a > HALF_DECK { - dl_dealt_b = dl_dealt_a - HALF_DECK - dl_dealt_a = HALF_DECK + var dealt_a: u8 = deal_next + var dealt_b: u8 = 0 + if dealt_a > HALF_DECK { + dealt_b = dealt_a - HALF_DECK + dealt_a = HALF_DECK } // Both decks drawn as card backs whenever they have at // least one card. Before that, skip the draw so the slot // is empty. - if dl_dealt_a > 0 { + if dealt_a > 0 { draw_card_back(DECK_A_X, DECK_Y) } - if dl_dealt_b > 0 { + if dealt_b > 0 { draw_card_back(DECK_B_X, DECK_Y) } - draw_count(COUNT_A_X, COUNT_Y, dl_dealt_a) - draw_count(COUNT_B_X, COUNT_Y, dl_dealt_b) + draw_count(COUNT_A_X, COUNT_Y, dealt_a) + draw_count(COUNT_B_X, COUNT_Y, dealt_b) // ── Flying card ────────────────────────────────── // A single face-down card bouncing between the centre // and each deck. The x position alternates based on the // low bit of deal_next (even → going to A, odd → B). if deal_next < DECK_SIZE { - var dl_fly_x: u8 = DECK_A_X + 32 + var fly_x_pos: u8 = DECK_A_X + 32 if (deal_next & 1) != 0 { - dl_fly_x = DECK_B_X - 32 + fly_x_pos = DECK_B_X - 32 } - draw_card_back(dl_fly_x, 96) + draw_card_back(fly_x_pos, 96) } // ── Transition ─────────────────────────────────── diff --git a/examples/war/deck.ne b/examples/war/deck.ne index 4af0891..7661a89 100644 --- a/examples/war/deck.ne +++ b/examples/war/deck.ne @@ -6,12 +6,6 @@ // the buffer. Everything wraps mod 52 — since 52 isn't a power // of two, we use `if idx >= 52 { idx -= 52 }` instead of `%` to // avoid the expensive software mod routine. -// -// NEScript v0.1 has a flat global symbol table for every `var` -// declaration (function-locals included), so each function's -// locals are prefixed with the function's short name to avoid -// E0501 collisions across the program. Function parameters ARE -// scoped per-function, so we can keep their names short. // ── Helpers shared by every deck ────────────────────────── // @@ -19,19 +13,11 @@ // needed inside push_back because `front` and `count` are both // ≤ 51 by construction, so the sum is at most 102 and one // subtraction is enough. -// -// Every parameter name in this file is unique across the entire -// program. NEScript v0.1's IR lowering uses a single global -// var_map for parameter names, so two functions both named -// `wrap52(v: u8)` and `another(v: u8)` end up sharing a VarId -// and the codegen routes their parameter reads to whichever -// zero-page slot the LAST function to be lowered claimed. See -// COMPILER_BUGS.md §1b for the gory details. -inline fun wrap52(w52_v: u8) -> u8 { - if w52_v >= DECK_SIZE { - return w52_v - DECK_SIZE +inline fun wrap52(v: u8) -> u8 { + if v >= DECK_SIZE { + return v - DECK_SIZE } - return w52_v + return v } // ── deck_a ──────────────────────────────────────────────── @@ -44,20 +30,15 @@ fun deck_a_empty() -> u8 { } fun draw_front_a() -> u8 { - var dfa_card: u8 = deck_a[deck_a_front] + var card: u8 = deck_a[deck_a_front] deck_a_front = wrap52(deck_a_front + 1) deck_a_count -= 1 - return dfa_card + return card } -fun push_back_a(pba_in: u8) { - // Snapshot `pba_in` into a local before calling wrap52, - // because NEScript v0.1's parameter-passing ABI uses fixed - // zero-page slots — wrap52's first param shares slot $04 - // with our `pba_in` and would silently clobber it. - var pba_card: u8 = pba_in - var pba_slot: u8 = wrap52(deck_a_front + deck_a_count) - deck_a[pba_slot] = pba_card +fun push_back_a(card: u8) { + var slot: u8 = wrap52(deck_a_front + deck_a_count) + deck_a[slot] = card deck_a_count += 1 } @@ -71,23 +52,22 @@ fun deck_b_empty() -> u8 { } fun draw_front_b() -> u8 { - var dfb_card: u8 = deck_b[deck_b_front] + var card: u8 = deck_b[deck_b_front] deck_b_front = wrap52(deck_b_front + 1) deck_b_count -= 1 - return dfb_card + return card } -fun push_back_b(pbb_in: u8) { - var pbb_card: u8 = pbb_in - var pbb_slot: u8 = wrap52(deck_b_front + deck_b_count) - deck_b[pbb_slot] = pbb_card +fun push_back_b(card: u8) { + var slot: u8 = wrap52(deck_b_front + deck_b_count) + deck_b[slot] = card deck_b_count += 1 } // ── pot ─────────────────────────────────────────────────── -fun push_back_pot(pbp_in: u8) { - pot[pot_count] = pbp_in +fun push_back_pot(card: u8) { + pot[pot_count] = card pot_count += 1 } @@ -98,19 +78,19 @@ fun clear_pot() { // Transfer every card currently in the pot into deck_a, in FIFO // order (so the face-up and face-down cards layer naturally). fun pot_to_a() { - var pta_i: u8 = 0 - while pta_i < pot_count { - push_back_a(pot[pta_i]) - pta_i += 1 + var i: u8 = 0 + while i < pot_count { + push_back_a(pot[i]) + i += 1 } pot_count = 0 } fun pot_to_b() { - var ptb_i: u8 = 0 - while ptb_i < pot_count { - push_back_b(pot[ptb_i]) - ptb_i += 1 + var i: u8 = 0 + while i < pot_count { + push_back_b(pot[i]) + i += 1 } pot_count = 0 } @@ -125,43 +105,42 @@ fun pot_to_b() { // half, and reset both queues' cursors. // // The random-swap shuffle is a bounded alternative to -// Fisher-Yates: it does N swaps between two random indices, where -// each index is rand() & 0x3F (0..63) and the swap is only done -// when both indices are < 52. 200 iterations on a 52-card deck is -// empirically well-mixed and uses only bitwise ops (no multiply, -// no divide, no W0101 warning). +// Fisher-Yates: it does 200 swaps between two random indices, +// where each index is rand() & 0x3F (0..63) and the swap is +// only done when both indices are < 52. Uses only bitwise ops +// (no multiply, no divide). fun build_master_deck() { - var bmd_r: u8 = 1 - var bmd_i: u8 = 0 - while bmd_r <= RANK_KING { - var bmd_s: u8 = 0 - while bmd_s < 4 { + var r: u8 = 1 + var i: u8 = 0 + while r <= RANK_KING { + var s: u8 = 0 + while s < 4 { // Pack rank into the high nibble, suit into the low. // rank fits in 4 bits (max 13) and suit fits in 2 // bits, so the shift-and-or is exact. - var bmd_shifted: u8 = bmd_r << 4 - deck_a[bmd_i] = bmd_shifted | bmd_s - bmd_i += 1 - bmd_s += 1 + var packed: u8 = r << 4 + deck_a[i] = packed | s + i += 1 + s += 1 } - bmd_r += 1 + r += 1 } } fun shuffle_deck_a() { - var shf_k: u8 = 0 - while shf_k < 200 { - var shf_i: u8 = rand_u8() & 0x3F - var shf_j: u8 = rand_u8() & 0x3F - if shf_i < DECK_SIZE { - if shf_j < DECK_SIZE { - var shf_tmp: u8 = deck_a[shf_i] - deck_a[shf_i] = deck_a[shf_j] - deck_a[shf_j] = shf_tmp + var k: u8 = 0 + while k < 200 { + var i: u8 = rand_u8() & 0x3F + var j: u8 = rand_u8() & 0x3F + if i < DECK_SIZE { + if j < DECK_SIZE { + var tmp: u8 = deck_a[i] + deck_a[i] = deck_a[j] + deck_a[j] = tmp } } - shf_k += 1 + k += 1 } } @@ -169,10 +148,10 @@ fun shuffle_deck_a() { // first 26 stay in deck_a, the second 26 move into deck_b. // Reset both queues' front/count cursors in the process. fun split_decks() { - var spd_i: u8 = 0 - while spd_i < HALF_DECK { - deck_b[spd_i] = deck_a[HALF_DECK + spd_i] - spd_i += 1 + var i: u8 = 0 + while i < HALF_DECK { + deck_b[i] = deck_a[HALF_DECK + i] + i += 1 } deck_a_front = 0 deck_a_count = HALF_DECK diff --git a/examples/war/play_state.ne b/examples/war/play_state.ne index 6b5f40c..2d80638 100644 --- a/examples/war/play_state.ne +++ b/examples/war/play_state.ne @@ -3,10 +3,6 @@ // `phase` cycles through the P_* constants defined in // war/constants.ne. `phase_timer` counts frames inside the // current phase and is reset to 0 whenever the phase changes. -// -// All function-local var names are prefixed with the function's -// short name (or with `pf_` for "play frame") so the global -// symbol table stays collision-free. // Set the current phase and zero the timer in one shot. Inlined // so each call site is just two stores. @@ -30,57 +26,46 @@ fun draw_table() { } // Bury helper for a war: move one card from the deck into the -// pot (face-down). Must be called with a non-empty deck. Two -// near-identical helpers (one per side) keep the locals -// uniquely-named. +// pot (face-down). Must be called with a non-empty deck. fun bury_from_a() { - var bfa_c: u8 = draw_front_a() - push_back_pot(bfa_c) + var c: u8 = draw_front_a() + push_back_pot(c) } fun bury_from_b() { - var bfb_c: u8 = draw_front_b() - push_back_pot(bfb_c) + var c: u8 = draw_front_b() + push_back_pot(c) } -// Draw the BIG WAR banner — three 16x16 metasprites at the -// centre of the screen. 12 sprites total, drawn in the centre -// row so they don't conflict with the deck stacks (rows 64-87) -// or the face-up cards (rows 128-151). -// -// All offsets stepped through locals to dodge the `x + N` -// parameter aliasing bug; see draw_word_player. +// Draw the BIG WAR banner — three 16×16 metasprite letters at +// the centre of the screen. 12 sprites total, drawn in the +// centre row so they don't conflict with the deck stacks (rows +// 64-87) or the face-up cards (rows 128-151). fun draw_big_war_banner(x: u8, y: u8) { - var bwb_y1: u8 = y + 8 - var bwb_x1: u8 = x + 8 - var bwb_x2: u8 = x + 20 - var bwb_x3: u8 = x + 28 - var bwb_x4: u8 = x + 40 - var bwb_x5: u8 = x + 48 // BIG W - draw Tileset at: (x, y) frame: TILE_BIG_W_TL - draw Tileset at: (bwb_x1, y) frame: TILE_BIG_W_TR - draw Tileset at: (x, bwb_y1) frame: TILE_BIG_W_BL - draw Tileset at: (bwb_x1, bwb_y1) frame: TILE_BIG_W_BR + draw Tileset at: (x, y) frame: TILE_BIG_W_TL + draw Tileset at: (x + 8, y) frame: TILE_BIG_W_TR + draw Tileset at: (x, y + 8) frame: TILE_BIG_W_BL + draw Tileset at: (x + 8, y + 8) frame: TILE_BIG_W_BR // BIG A - draw Tileset at: (bwb_x2, y) frame: TILE_BIG_A_TL - draw Tileset at: (bwb_x3, y) frame: TILE_BIG_A_TR - draw Tileset at: (bwb_x2, bwb_y1) frame: TILE_BIG_A_BL - draw Tileset at: (bwb_x3, bwb_y1) frame: TILE_BIG_A_BR + draw Tileset at: (x + 20, y) frame: TILE_BIG_A_TL + draw Tileset at: (x + 28, y) frame: TILE_BIG_A_TR + draw Tileset at: (x + 20, y + 8) frame: TILE_BIG_A_BL + draw Tileset at: (x + 28, y + 8) frame: TILE_BIG_A_BR // BIG R - draw Tileset at: (bwb_x4, y) frame: TILE_BIG_R_TL - draw Tileset at: (bwb_x5, y) frame: TILE_BIG_R_TR - draw Tileset at: (bwb_x4, bwb_y1) frame: TILE_BIG_R_BL - draw Tileset at: (bwb_x5, bwb_y1) frame: TILE_BIG_R_BR + draw Tileset at: (x + 40, y) frame: TILE_BIG_R_TL + draw Tileset at: (x + 48, y) frame: TILE_BIG_R_TR + draw Tileset at: (x + 40, y + 8) frame: TILE_BIG_R_BL + draw Tileset at: (x + 48, y + 8) frame: TILE_BIG_R_BR } // Begin the A-side draw animation: pull the top card off deck_a, // stash it as the face-up `card_a`, arm the fly state for the // deck → play slide, and play the click sfx. // -// fly_card / fly_face_up are stuffed directly into globals -// instead of being passed to arm_fly, because arm_fly only takes -// 4 params (the v0.1 ABI limit) and silently drops anything past -// the fourth. +// fly_card / fly_face_up are written directly to globals +// instead of being passed to arm_fly, because arm_fly already +// takes 4 parameters and the v0.1 ABI caps function signatures +// at 4 parameters (E0506). fun begin_draw_a() { if deck_a_count > 0 { card_a = draw_front_a() @@ -189,16 +174,16 @@ state Playing { // Both cards go into the pot regardless of outcome. push_back_pot(card_a) push_back_pot(card_b) - pf_result = compare_cards(card_a, card_b) - if pf_result == 1 { + var result: u8 = compare_cards(card_a, card_b) + if result == 1 { play CheerA set_phase(P_WIN_A) } - if pf_result == 2 { + if result == 2 { play CheerB set_phase(P_WIN_B) } - if pf_result == 0 { + if result == 0 { // It's a tie — but only enter the war flow if both // sides actually have cards left to bury. If a // player ran out of cards on this very tie, the diff --git a/examples/war/render.ne b/examples/war/render.ne index 26c3633..6729a3f 100644 --- a/examples/war/render.ne +++ b/examples/war/render.ne @@ -6,10 +6,6 @@ // a 16×24 card face burns 6 sprite slots, a single-character // letter burns 1, and so on. The caller is responsible for // sprite budgeting — see PLAN.md §3. -// -// Function-local var names are prefixed with the function's -// short name to avoid the global symbol-table collisions that -// E0501 would otherwise complain about. // ── Card face ────────────────────────────────────────────── // @@ -22,69 +18,46 @@ // // Every tile in the card has a fully-opaque white background // (palette index 2) so the felt green behind the card does not -// bleed through — see `assets.ne` for the art itself. The big -// centre pip is a single 16×16 shape split across the bottom -// two rows (4 tiles total); `rank` sits in the top-left corner -// and `small_suit` in the top-right. -fun draw_card_face(dcf_in_x: u8, dcf_in_y: u8, dcf_in_card: u8) { - // Snapshot every parameter into a fresh local *before* any - // nested function call. NEScript v0.1 passes parameters via - // fixed zero-page slots ($04, $05, $06), and any inner call - // overwrites those slots with its own parameter values — - // including the inline `card_rank` / `card_suit` calls below, - // which would otherwise leave `x` and `y` corrupted by the - // time the draw lines run. - var dcf_x: u8 = dcf_in_x - var dcf_y: u8 = dcf_in_y - var dcf_card: u8 = dcf_in_card - var dcf_rank: u8 = card_rank(dcf_card) - var dcf_suit: u8 = card_suit(dcf_card) - var dcf_rank_tile: u8 = TILE_RANK_BASE + dcf_rank - 1 - var dcf_small_tile: u8 = TILE_SUIT_SMALL_BASE + dcf_suit - var dcf_pip_tl: u8 = TILE_PIP_TL_BASE + dcf_suit - var dcf_pip_tr: u8 = TILE_PIP_TR_BASE + dcf_suit - var dcf_pip_bl: u8 = TILE_PIP_BL_BASE + dcf_suit - var dcf_pip_br: u8 = TILE_PIP_BR_BASE + dcf_suit - var dcf_x1: u8 = dcf_x + 8 - var dcf_y1: u8 = dcf_y + 8 - var dcf_y2: u8 = dcf_y + 16 +// bleed through — see `assets.ne` for the art itself. +fun draw_card_face(x: u8, y: u8, card: u8) { + var rank: u8 = card_rank(card) + var suit: u8 = card_suit(card) + var rank_tile: u8 = TILE_RANK_BASE + rank - 1 + var small_tile: u8 = TILE_SUIT_SMALL_BASE + suit + var pip_tl: u8 = TILE_PIP_TL_BASE + suit + var pip_tr: u8 = TILE_PIP_TR_BASE + suit + var pip_bl: u8 = TILE_PIP_BL_BASE + suit + var pip_br: u8 = TILE_PIP_BR_BASE + suit // Row 0 — rank corner + small suit - draw Tileset at: (dcf_x, dcf_y) frame: dcf_rank_tile - draw Tileset at: (dcf_x1, dcf_y) frame: dcf_small_tile + draw Tileset at: (x, y) frame: rank_tile + draw Tileset at: (x + 8, y) frame: small_tile // Row 1 — top half of the 16×16 big pip - draw Tileset at: (dcf_x, dcf_y1) frame: dcf_pip_tl - draw Tileset at: (dcf_x1, dcf_y1) frame: dcf_pip_tr + draw Tileset at: (x, y + 8) frame: pip_tl + draw Tileset at: (x + 8, y + 8) frame: pip_tr // Row 2 — bottom half of the 16×16 big pip - draw Tileset at: (dcf_x, dcf_y2) frame: dcf_pip_bl - draw Tileset at: (dcf_x1, dcf_y2) frame: dcf_pip_br + draw Tileset at: (x, y + 16) frame: pip_bl + draw Tileset at: (x + 8, y + 16) frame: pip_br } -// Draw the card-back lattice at (x, y). 6 sprites again. Rows -// 0 and 2 reuse the same top/bottom back tiles; row 1 uses the -// bottom row of the lattice as a filler so the pattern stays -// continuous. +// Draw the card-back checkerboard at (x, y). 6 sprites. The +// back tiles tile seamlessly so every cell of the 16×24 card +// body carries a 2-pixel square of the same black/white grid. fun draw_card_back(x: u8, y: u8) { - var dcb_x1: u8 = x + 8 - var dcb_y1: u8 = y + 8 - var dcb_y2: u8 = y + 16 - draw Tileset at: (x, y) frame: TILE_BACK_TL - draw Tileset at: (dcb_x1, y) frame: TILE_BACK_TR - draw Tileset at: (x, dcb_y1) frame: TILE_BACK_BL - draw Tileset at: (dcb_x1, dcb_y1) frame: TILE_BACK_BR - draw Tileset at: (x, dcb_y2) frame: TILE_BACK_TL - draw Tileset at: (dcb_x1, dcb_y2) frame: TILE_BACK_TR + draw Tileset at: (x, y) frame: TILE_BACK_TL + draw Tileset at: (x + 8, y) frame: TILE_BACK_TR + draw Tileset at: (x, y + 8) frame: TILE_BACK_BL + draw Tileset at: (x + 8, y + 8) frame: TILE_BACK_BR + draw Tileset at: (x, y + 16) frame: TILE_BACK_TL + draw Tileset at: (x + 8, y + 16) frame: TILE_BACK_TR } // Dispatch between front and back based on the fly_face_up flag -// stamped by the phase handlers. Snapshots params first because -// the inner call clobbers $04/$05. +// stamped by the phase handlers. fun draw_flying_card(x: u8, y: u8) { - var dfc_x: u8 = x - var dfc_y: u8 = y if fly_face_up == 0 { - draw_card_back(dfc_x, dfc_y) + draw_card_back(x, y) } else { - draw_card_face(dfc_x, dfc_y, fly_card) + draw_card_face(x, y, fly_card) } } @@ -95,91 +68,62 @@ fun draw_digit(x: u8, y: u8, d: u8) { draw Tileset at: (x, y) frame: TILE_DIGIT_BASE + d } -// Draw a two-digit decimal count (0..99) at (x, y). Leading zero -// is preserved so the HUD always renders two glyphs wide, which -// keeps the layout stable as the counts change. +// Draw a two-digit decimal count (0..99) at (x, y). Leading +// zero is preserved so the HUD always renders two glyphs wide, +// which keeps the layout stable as the counts change. fun draw_count(x: u8, y: u8, v: u8) { - var dct_tens: u8 = 0 - var dct_n: u8 = v + var tens: u8 = 0 + var n: u8 = v // Divide by 10 without the software divide: repeatedly // subtract 10 until n < 10. For v ≤ 52 this loops at most // 5 times, cheaper than calling `/` and `%`. - while dct_n >= 10 { - dct_n -= 10 - dct_tens += 1 + while n >= 10 { + n -= 10 + tens += 1 } - draw_digit(x, y, dct_tens) - draw_digit(x + 8, y, dct_n) + draw_digit(x, y, tens) + draw_digit(x + 8, y, n) } // Draw a single letter 'A'..'Z' using the sprite font. `ch` is -// the letter index (0 = A, 25 = Z). Deliberately NOT marked -// `inline`: when this was inlined the resulting code put each -// inlined `draw` at double the intended X step (the inliner -// appears to re-evaluate the (x + N) parameter expression in a -// way that compounds across consecutive draws). Keeping it as -// a real function call gives every draw_letter call its own -// argument-evaluation context and the spacing comes out right. +// the letter index (0 = A, 25 = Z). fun draw_letter(x: u8, y: u8, ch: u8) { draw Tileset at: (x, y) frame: TILE_LETTER_BASE + ch } // Short helper for drawing the word "PLAYER" at (x, y). 6 letters. // Used by the HUD and the victory banner. -// -// We accumulate the X position in a local instead of passing -// `x + N` as a call argument: the latter pattern miscompiles in -// NEScript v0.1 (consecutive `x + N` arguments to the same -// function appear to alias the parameter slot, leaving the second -// onward call site reading a stale offset). Stepping a local -// avoids the issue entirely. fun draw_word_player(x: u8, y: u8) { - var dwp_px: u8 = x - draw_letter(dwp_px, y, 15) // P - dwp_px += 8 - draw_letter(dwp_px, y, 11) // L - dwp_px += 8 - draw_letter(dwp_px, y, 0) // A - dwp_px += 8 - draw_letter(dwp_px, y, 24) // Y - dwp_px += 8 - draw_letter(dwp_px, y, 4) // E - dwp_px += 8 - draw_letter(dwp_px, y, 17) // R + draw_letter(x, y, 15) // P + draw_letter(x + 8, y, 11) // L + draw_letter(x + 16, y, 0) // A + draw_letter(x + 24, y, 24) // Y + draw_letter(x + 32, y, 4) // E + draw_letter(x + 40, y, 17) // R } // "WINS" — used on the victory screen. fun draw_word_wins(x: u8, y: u8) { - var dww_px: u8 = x - draw_letter(dww_px, y, 22) // W - dww_px += 8 - draw_letter(dww_px, y, 8) // I - dww_px += 8 - draw_letter(dww_px, y, 13) // N - dww_px += 8 - draw_letter(dww_px, y, 18) // S + draw_letter(x, y, 22) // W + draw_letter(x + 8, y, 8) // I + draw_letter(x + 16, y, 13) // N + draw_letter(x + 24, y, 18) // S } // "PRESS" — used on the title screen. fun draw_word_press(x: u8, y: u8) { - var dwr_px: u8 = x - draw_letter(dwr_px, y, 15) // P - dwr_px += 8 - draw_letter(dwr_px, y, 17) // R - dwr_px += 8 - draw_letter(dwr_px, y, 4) // E - dwr_px += 8 - draw_letter(dwr_px, y, 18) // S - dwr_px += 8 - draw_letter(dwr_px, y, 18) // S + draw_letter(x, y, 15) // P + draw_letter(x + 8, y, 17) // R + draw_letter(x + 16, y, 4) // E + draw_letter(x + 24, y, 18) // S + draw_letter(x + 32, y, 18) // S } // ── Fly animation driver ────────────────────────────────── // -// We avoid the software multiply (and the W0101 "expensive -// multiply" warning) by stepping the fly position by a fixed -// constant each frame instead of computing `start + dx * t / -// FRAMES`. The constant FLY_STEP is chosen so that +// Avoiding a software multiply: instead of computing +// `start + dx * t / FRAMES` we step the fly position by a +// fixed constant each frame. FLY_STEP is chosen so that // FRAMES_FLY * FLY_STEP equals the deck-to-play distance on // both axes: // @@ -192,8 +136,8 @@ const FLY_STEP: u8 = 4 // Step the global (fly_x, fly_y) by FLY_STEP in the directions // stored in fly_dx_sign / fly_dy_sign. Sign 0 = positive (move -// right / down), 1 = negative (move left / up). Called once per -// frame inside the relevant fly-phase handler. +// right / down), 1 = negative (move left / up). Called once +// per frame inside the relevant fly-phase handler. fun step_fly_pos() { if fly_dx_sign == 0 { fly_x += FLY_STEP @@ -208,11 +152,9 @@ fun step_fly_pos() { } // Initialise the fly state for a card slide from (sx, sy) using -// the given direction signs. NEScript v0.1 only allocates four -// zero-page slots ($04-$07) for function parameters, so callers -// must stash `fly_card` and `fly_face_up` directly into the -// globals before invoking arm_fly — passing them as the 5th and -// 6th params would silently drop the values on the floor. +// the given direction signs, card byte, and face-up flag. The +// caller is responsible for picking signs so the end position +// lands where it should after FRAMES_FLY frames. fun arm_fly(sx: u8, sy: u8, dxsign: u8, dysign: u8) { fly_x = sx fly_y = sy diff --git a/examples/war/state.ne b/examples/war/state.ne index 2428e12..ea9055c 100644 --- a/examples/war/state.ne +++ b/examples/war/state.ne @@ -40,7 +40,6 @@ var a_is_cpu: u8 = 1 // bool-ish var b_is_cpu: u8 = 1 var phase: u8 = 0 // one of the P_* constants var phase_timer: u8 = 0 // counts up during each phase -var pf_result: u8 = 0 // P_RESOLVE comparison result (1=A, 2=B, 0=tie) // ── Animation state ─────────────────────────────────────── // Shared by every card-fly phase. We step (fly_x, fly_y) by a diff --git a/examples/war/victory_state.ne b/examples/war/victory_state.ne index e8cbc6b..4e7d47b 100644 --- a/examples/war/victory_state.ne +++ b/examples/war/victory_state.ne @@ -34,17 +34,21 @@ state Victory { // deck is empty (shouldn't happen — we only arrive // here because the *loser* is empty) fall back to a // card back so the layout is stable. + // Pull the winner's top card into a single local. Only + // one `var top` declaration per on-frame body — NEScript + // scopes locals per function body, not per if-block. + var top: u8 = 0 if winner == 0 { if deck_a_count > 0 { - var vic_top_a: u8 = deck_a[deck_a_front] - draw_card_face(120, 128, vic_top_a) + top = deck_a[deck_a_front] + draw_card_face(120, 128, top) } else { draw_card_back(120, 128) } } else { if deck_b_count > 0 { - var vic_top_b: u8 = deck_b[deck_b_front] - draw_card_face(120, 128, vic_top_b) + top = deck_b[deck_b_front] + draw_card_face(120, 128, top) } else { draw_card_back(120, 128) } diff --git a/src/analyzer/mod.rs b/src/analyzer/mod.rs index f33717e..3fb54d7 100644 --- a/src/analyzer/mod.rs +++ b/src/analyzer/mod.rs @@ -119,6 +119,7 @@ pub fn analyze(program: &Program) -> AnalysisResult { function_return_types: HashMap::new(), current_return_type: None, in_function_body: false, + current_scope_prefix: None, struct_layouts: HashMap::new(), }; analyzer.analyze_program(program); @@ -180,6 +181,24 @@ struct Analyzer { /// distinguish "void function" from "state handler" when checking /// `return value` statements. in_function_body: bool, + /// Current local scope prefix used to qualify function-body + /// `var` declarations. `None` at the top level and during + /// state-level declaration registration; set to `Some("foo")` + /// while analyzing the body of `fun foo`, and to + /// `Some("Title::frame")` (etc.) while analyzing a state + /// handler body. When it is `Some(prefix)`, `register_var` + /// stores the declaration under a qualified key + /// `"__local__{prefix}__{name}"`, and `resolve_*` helpers + /// below fall back through the qualified key first and the + /// bare key second so identifier reads inside a body resolve + /// to the locally-scoped entry when one exists. + /// + /// This is how functions and handlers get their own local + /// namespaces without requiring a full nested-scope stack. + /// Top-level globals, consts, state-level vars, function + /// names, and enum variants still live at the bare name in + /// `self.symbols`. + current_scope_prefix: Option, /// Struct name to layout. Each field has an offset in bytes from /// the base address of the struct. struct_layouts: HashMap, @@ -524,16 +543,27 @@ impl Analyzer { )); } - // Type-check all state bodies + // Type-check all state bodies. Each handler body is its + // own local scope: a `var i` inside `Title::on frame` + // does not collide with a `var i` inside `Playing::on + // frame`. State-level `var`s (declared at `state Foo { var x + // }`) stay in the global scope so every handler in the + // state can read and write them across frames. for state in &program.states { if let Some(block) = &state.on_enter { + self.current_scope_prefix = Some(format!("{}__enter", state.name)); self.check_block(block, &state_names); + self.current_scope_prefix = None; } if let Some(block) = &state.on_exit { + self.current_scope_prefix = Some(format!("{}__exit", state.name)); self.check_block(block, &state_names); + self.current_scope_prefix = None; } if let Some(block) = &state.on_frame { + self.current_scope_prefix = Some(format!("{}__frame", state.name)); self.check_block(block, &state_names); + self.current_scope_prefix = None; } // `on scanline(N)` is only valid with mappers that have a // scanline-counting IRQ source (currently only MMC3). @@ -544,43 +574,64 @@ impl Analyzer { state.span, )); } - for (_, block) in &state.on_scanline { + for (line, block) in &state.on_scanline { + self.current_scope_prefix = Some(format!("{}__scanline_{}", state.name, line)); self.check_block(block, &state_names); + self.current_scope_prefix = None; } } - // Type-check function bodies. Parameters are registered as - // symbols for the duration of the body check so that identifier - // references (and the W0103 used-variable tracker) can resolve - // them. They are unregistered afterwards to avoid leaking into - // the global scope. Parameters are also pre-marked as "used" so - // we do not emit W0103 for unused function arguments (which are - // a common and deliberate pattern). + // Type-check function bodies. Each function body gets its + // own local scope by setting `current_scope_prefix` to + // the function's name. Parameters are registered as + // function-local symbols under the prefixed key so two + // different functions can both declare a parameter named + // `x` without the analyzer's duplicate-declaration check + // firing. Each parameter also gets its own dedicated RAM + // slot allocated via `allocate_ram` — the codegen emits a + // prologue that copies the transport slots `$04-$07` + // into these RAM slots at function entry, so the param + // values survive any nested calls the function body + // makes. Parameters are pre-marked as "used" so we do + // not emit W0103 for unused function arguments. for fun in &program.functions { - let mut added_params = Vec::new(); + self.current_scope_prefix = Some(fun.name.clone()); for param in &fun.params { - if !self.symbols.contains_key(¶m.name) { + let key = self.scoped_name(¶m.name); + let size = match param.param_type { + NesType::U8 | NesType::I8 | NesType::Bool => 1, + NesType::U16 => 2, + // Struct/array parameters are not supported + // in v0.1; the parser already rejects them, + // so defaulting to 1 byte here is just a + // fallback to keep the analyzer from + // crashing on malformed ASTs. + _ => 1, + }; + if let Some(address) = self.allocate_ram(size, fun.span) { self.symbols.insert( - param.name.clone(), + key.clone(), Symbol { - name: param.name.clone(), + name: key.clone(), sym_type: param.param_type.clone(), is_const: false, span: fun.span, }, ); - added_params.push(param.name.clone()); + self.var_allocations.push(VarAllocation { + name: key.clone(), + address, + size, + }); + self.used_vars.insert(key); } - self.mark_var_used(¶m.name); } self.current_return_type.clone_from(&fun.return_type); self.in_function_body = true; self.check_block(&fun.body, &state_names); self.current_return_type = None; self.in_function_body = false; - for name in &added_params { - self.symbols.remove(name); - } + self.current_scope_prefix = None; } // Build call graph @@ -629,18 +680,68 @@ impl Analyzer { self.check_unreachable_states(program); } + /// Qualify `name` under the current scope prefix. If no prefix + /// is active (top-level analysis, state-level declaration) the + /// name is returned unchanged. Inside a function body the + /// result is `"__local__{prefix}__{name}"` — the same key the + /// IR lowerer and codegen use when walking a function's locals. + fn scoped_name(&self, name: &str) -> String { + match &self.current_scope_prefix { + Some(prefix) => format!("__local__{prefix}__{name}"), + None => name.to_string(), + } + } + + /// Resolve a user-written identifier to the matching symbol + /// table key. Tries the current scope's qualified key first + /// (so function-local vars shadow same-named globals) and + /// falls back to the bare key for globals, consts, enum + /// variants, state-locals, and function names. + fn resolve_key(&self, name: &str) -> String { + if let Some(prefix) = &self.current_scope_prefix { + let qualified = format!("__local__{prefix}__{name}"); + if self.symbols.contains_key(&qualified) { + return qualified; + } + } + name.to_string() + } + + /// Look up a user-written identifier in the symbol table, + /// preferring the current scope's qualified entry (if any) + /// and falling back to the bare global entry. + fn resolve_symbol(&self, name: &str) -> Option<&Symbol> { + if let Some(prefix) = &self.current_scope_prefix { + let qualified = format!("__local__{prefix}__{name}"); + if let Some(sym) = self.symbols.get(&qualified) { + return Some(sym); + } + } + self.symbols.get(name) + } + + /// True if a symbol exists for `name` in the current scope or + /// in the global scope. + fn symbol_exists(&self, name: &str) -> bool { + self.resolve_symbol(name).is_some() + } + /// Mark a variable name as having been read somewhere in the program. - /// Also bumps the W0107 access counter. + /// Also bumps the W0107 access counter. Resolves the name through + /// the current scope so the right (qualified-or-bare) key is + /// recorded. fn mark_var_used(&mut self, name: &str) { - self.used_vars.insert(name.to_string()); - self.bump_var_access(name); + let key = self.resolve_key(name); + self.used_vars.insert(key.clone()); + *self.var_access_counts.entry(key).or_insert(0) += 1; } /// Increment the observed access count for `name`. Called for /// both reads (via [`Analyzer::mark_var_used`]) and writes (at /// `Statement::Assign` handling). Used for W0107. fn bump_var_access(&mut self, name: &str) { - *self.var_access_counts.entry(name.to_string()).or_insert(0) += 1; + let key = self.resolve_key(name); + *self.var_access_counts.entry(key).or_insert(0) += 1; } /// Emit W0107 if `var` is declared `fast` but sees fewer than @@ -712,7 +813,7 @@ impl Analyzer { fn walk_expr_reads(&mut self, expr: &Expr) { match expr { Expr::Ident(name, span) => { - if self.symbols.contains_key(name) { + if self.symbol_exists(name) { self.mark_var_used(name); } else { self.emit_undefined_var(name, *span); @@ -720,7 +821,7 @@ impl Analyzer { } Expr::ArrayIndex(name, idx, span) => { // Array base is a read; index may contain more reads. - if self.symbols.contains_key(name) { + if self.symbol_exists(name) { self.mark_var_used(name); } else { self.emit_undefined_var(name, *span); @@ -728,14 +829,15 @@ impl Analyzer { self.walk_expr_reads(idx); } Expr::FieldAccess(name, field, span) => { - // Resolve the struct variable and verify the field - // exists. Mark the synthetic `name.field` variable as - // used so W0103 doesn't fire. - let full_name = format!("{name}.{field}"); + // Resolve the struct variable through the scope + // stack and verify the field exists. Mark both the + // base and the synthetic `name.field` entry used. + let base_key = self.resolve_key(name); + let full_name = format!("{base_key}.{field}"); if self.symbols.contains_key(&full_name) { self.mark_var_used(name); - self.mark_var_used(&full_name); - } else if !self.symbols.contains_key(name) { + self.used_vars.insert(full_name); + } else if !self.symbols.contains_key(&base_key) { self.emit_undefined_var(name, *span); } else { self.diagnostics.push(Diagnostic::error( @@ -1134,7 +1236,19 @@ impl Analyzer { } fn register_var(&mut self, var: &VarDecl) { - if self.symbols.contains_key(&var.name) { + // Scope-qualified storage key. At the top level this is + // the bare `var.name`; inside a function or handler body + // it is `"__local__{prefix}__{name}"` so two different + // functions can each declare a local `var i` without + // colliding on the flat symbol table. + let key = self.scoped_name(&var.name); + + // Duplicate check runs against the qualified key so + // shadowing a global with a function-local var is fine + // (the local key is distinct). Two locals with the same + // name inside the same scope still collide and report + // E0501 correctly. + if self.symbols.contains_key(&key) { self.diagnostics.push(Diagnostic::error( ErrorCode::E0501, format!("duplicate declaration of '{}'", var.name), @@ -1194,9 +1308,9 @@ impl Analyzer { // symbol so that later references don't cascade into E0502, // but don't record a var_allocations entry. self.symbols.insert( - var.name.clone(), + key.clone(), Symbol { - name: var.name.clone(), + name: key, sym_type: var.var_type.clone(), is_const: false, span: var.span, @@ -1215,15 +1329,21 @@ impl Analyzer { // `p.pos.y` leaves. Array fields produce a single // synthetic with the array type so the existing // `Expr::ArrayIndex` lowering picks them up. + // + // Struct fields use the qualified key as the base so + // function-local struct instances don't collide with + // same-named globals. `flatten_struct_fields` builds + // `"{key}.{field}"` paths, which inherits the scope + // prefix automatically. if let NesType::Struct(sname) = &var.var_type { let layout = self.struct_layouts[sname].clone(); - self.flatten_struct_fields(&var.name, address, &layout, var.span); + self.flatten_struct_fields(&key, address, &layout, var.span); // Also register the struct variable itself (as a symbol // only — it doesn't have a single VarAllocation entry). self.symbols.insert( - var.name.clone(), + key.clone(), Symbol { - name: var.name.clone(), + name: key, sym_type: var.var_type.clone(), is_const: false, span: var.span, @@ -1233,9 +1353,9 @@ impl Analyzer { } self.symbols.insert( - var.name.clone(), + key.clone(), Symbol { - name: var.name.clone(), + name: key.clone(), sym_type: var.var_type.clone(), is_const: false, span: var.span, @@ -1243,7 +1363,7 @@ impl Analyzer { ); self.var_allocations.push(VarAllocation { - name: var.name.clone(), + name: key, address, size, }); @@ -1441,10 +1561,12 @@ impl Analyzer { } } Statement::Assign(lvalue, _, expr, span) => { - // Check if trying to assign to a constant + // Check if trying to assign to a constant. Lookups + // go through `resolve_symbol` so a function-local + // shadowing a global const/var is handled correctly. match lvalue { LValue::Var(name) => { - if let Some(sym) = self.symbols.get(name) { + if let Some(sym) = self.resolve_symbol(name) { if sym.is_const { self.diagnostics.push(Diagnostic::error( ErrorCode::E0203, @@ -1467,7 +1589,7 @@ impl Analyzer { } } LValue::ArrayIndex(name, idx) => { - if let Some(sym) = self.symbols.get(name) { + if let Some(sym) = self.resolve_symbol(name) { if sym.is_const { self.diagnostics.push(Diagnostic::error( ErrorCode::E0203, @@ -1484,13 +1606,20 @@ impl Analyzer { self.walk_expr_reads(idx); } LValue::Field(name, field) => { - let full_name = format!("{name}.{field}"); + // Struct instances are stored under the + // scope-qualified key (see register_var); + // the per-field synthetic keys inherit that + // prefix via `flatten_struct_fields`. Build + // the field path off whichever key actually + // exists — scoped first, bare second. + let base_key = self.resolve_key(name); + let full_name = format!("{base_key}.{field}"); if self.symbols.contains_key(&full_name) { // Assigning to a field is a mutation; don't // mark the struct variable as "read" just // because we wrote to one of its fields. - self.mark_var_used(&full_name); - } else if self.symbols.contains_key(name) { + self.used_vars.insert(full_name); + } else if self.symbols.contains_key(&base_key) { self.diagnostics.push(Diagnostic::error( ErrorCode::E0201, format!("'{name}' has no field '{field}'"), @@ -1558,11 +1687,16 @@ impl Analyzer { self.walk_expr_reads(end); self.check_expr_type(start, &NesType::U8); self.check_expr_type(end, &NesType::U8); - let was_shadowed = self.symbols.remove(var); + // Register the loop variable under the current + // scope's qualified key so two `for i in 0..n` + // loops in different functions get their own + // per-function RAM slot. + let loop_key = self.scoped_name(var); + let was_shadowed = self.symbols.remove(&loop_key); self.symbols.insert( - var.clone(), + loop_key.clone(), Symbol { - name: var.clone(), + name: loop_key.clone(), sym_type: NesType::U8, is_const: false, span: *span, @@ -1573,19 +1707,19 @@ impl Analyzer { // other u8 local. let loop_var_addr = self.allocate_ram(1, *span).unwrap_or(0x10); self.var_allocations.push(VarAllocation { - name: var.clone(), + name: loop_key.clone(), address: loop_var_addr, size: 1, }); // Loop variable is always "used" in the header. - self.mark_var_used(var); + self.used_vars.insert(loop_key.clone()); let was_in_loop = self.in_loop; self.in_loop = true; self.check_block(body, state_names); self.in_loop = was_in_loop; - self.symbols.remove(var); + self.symbols.remove(&loop_key); if let Some(old) = was_shadowed { - self.symbols.insert(var.clone(), old); + self.symbols.insert(loop_key, old); } } Statement::Loop(body, span) => { @@ -1780,15 +1914,17 @@ impl Analyzer { fn lvalue_type(&self, lvalue: &LValue, _span: Span) -> Option { match lvalue { - LValue::Var(name) => self.symbols.get(name).map(|s| s.sym_type.clone()), + LValue::Var(name) => self.resolve_symbol(name).map(|s| s.sym_type.clone()), LValue::ArrayIndex(name, _) => { - self.symbols.get(name).and_then(|sym| match &sym.sym_type { - NesType::Array(elem, _) => Some(elem.as_ref().clone()), - _ => None, - }) + self.resolve_symbol(name) + .and_then(|sym| match &sym.sym_type { + NesType::Array(elem, _) => Some(elem.as_ref().clone()), + _ => None, + }) } LValue::Field(name, field) => { - let full_name = format!("{name}.{field}"); + let base_key = self.resolve_key(name); + let full_name = format!("{base_key}.{field}"); self.symbols.get(&full_name).map(|s| s.sym_type.clone()) } } @@ -1873,7 +2009,7 @@ impl Analyzer { } } Expr::BoolLiteral(_, _) => Some(NesType::Bool), - Expr::Ident(name, _) => self.symbols.get(name).map(|s| s.sym_type.clone()), + Expr::Ident(name, _) => self.resolve_symbol(name).map(|s| s.sym_type.clone()), Expr::ButtonRead(_, _, _) => Some(NesType::Bool), Expr::BinaryOp(_, op, _, _) => match op { BinOp::Eq @@ -1890,13 +2026,14 @@ impl Analyzer { Expr::UnaryOp(_, _, _) => Some(NesType::U8), Expr::Call(_, _, _) => Some(NesType::U8), // Simplified for M1 Expr::ArrayIndex(name, _, _) => { - self.symbols.get(name).and_then(|s| match &s.sym_type { + self.resolve_symbol(name).and_then(|s| match &s.sym_type { NesType::Array(elem, _) => Some(elem.as_ref().clone()), _ => None, }) } Expr::FieldAccess(name, field, _) => { - let full_name = format!("{name}.{field}"); + let base_key = self.resolve_key(name); + let full_name = format!("{base_key}.{field}"); self.symbols.get(&full_name).map(|s| s.sym_type.clone()) } Expr::ArrayLiteral(_, _) => Some(NesType::U8), // element type inferred from context diff --git a/src/analyzer/tests.rs b/src/analyzer/tests.rs index 103e72b..c87c297 100644 --- a/src/analyzer/tests.rs +++ b/src/analyzer/tests.rs @@ -2100,3 +2100,103 @@ fn analyze_accepts_function_with_exactly_4_params() { "#, ); } + +#[test] +fn analyze_allows_same_local_name_in_two_functions() { + // Regression test for COMPILER_BUGS.md §3: function-body + // `var` declarations used to live in a flat global namespace, + // so two functions both declaring `var i` collided on E0501. + // They should now coexist in their own per-function scopes. + analyze_ok( + r#" + game "T" { mapper: NROM } + fun foo() -> u8 { + var i: u8 = 0 + while i < 5 { i += 1 } + return i + } + fun bar() -> u8 { + var i: u8 = 0 + while i < 10 { i += 1 } + return i + } + var total: u8 = 0 + on frame { + total = foo() + bar() + wait_frame + } + start Main + "#, + ); +} + +#[test] +fn analyze_allows_same_param_name_in_two_functions() { + // Regression test for COMPILER_BUGS.md §1b: parameters + // across different functions used to share VarIds because + // the IR lowerer's `var_map` was global. Both declaration + // (analyzer) and lowering (IR) should now give each + // function's parameters their own independent entries. + analyze_ok( + r#" + game "T" { mapper: NROM } + fun shift_left(x: u8) -> u8 { return x << 1 } + fun shift_right(x: u8) -> u8 { return x >> 1 } + var n: u8 = 0 + on frame { + n = shift_left(5) + shift_right(20) + wait_frame + } + start Main + "#, + ); +} + +#[test] +fn analyze_allows_same_local_name_in_two_state_handlers() { + // Each state handler gets its own local scope, so both + // `Title::on frame` and `Playing::on frame` can declare + // `var i` independently. + analyze_ok( + r#" + game "T" { mapper: NROM } + state Title { + on frame { + var i: u8 = 0 + while i < 3 { i += 1 } + if button.start { transition Playing } + } + } + state Playing { + on frame { + var i: u8 = 0 + while i < 7 { i += 1 } + } + } + start Title + "#, + ); +} + +#[test] +fn analyze_still_rejects_duplicate_local_in_same_function() { + // Two `var i` declarations inside the SAME function body + // should still trip E0501 — we scoped locals per function, + // not per statement. + let errors = analyze_errors( + r#" + game "T" { mapper: NROM } + fun foo() -> u8 { + var i: u8 = 0 + var i: u8 = 1 + return i + } + on frame { var r: u8 = foo() wait_frame } + start Main + "#, + ); + assert!( + errors.contains(&ErrorCode::E0501), + "expected E0501 for duplicate `var i` in same function, got: {errors:?}" + ); +} diff --git a/src/codegen/ir_codegen.rs b/src/codegen/ir_codegen.rs index 51d4950..1332f49 100644 --- a/src/codegen/ir_codegen.rs +++ b/src/codegen/ir_codegen.rs @@ -210,13 +210,27 @@ impl<'a> IrCodeGen<'a> { var_sizes.insert(global.var_id, alloc.size); } } - // Map each function's parameter VarIds to the zero-page - // parameter-passing slots $04-$07 (up to 4 params). Map the - // rest of each function's locals into main RAM starting at - // `$0300` (after the OAM buffer). Locals don't overlap with - // globals (which were placed by the analyzer) and are - // disjoint across functions so nested calls don't corrupt - // each other. + // Map every function-local — parameters AND body-declared + // vars — into a dedicated RAM slot at `$0300+`. Parameters + // are still passed via the zero-page transport slots + // `$04-$07` as the calling convention, but `gen_function` + // emits a prologue at function entry that copies those + // transport slots into these per-function RAM slots. That + // way, when a function makes a nested call, the nested + // call clobbers `$04-$07` (writing its own arguments into + // them) without disturbing the caller's saved parameters. + // + // Before this change, parameters lived in `$04-$07` for the + // duration of the function body, so any call nested inside + // a function's body silently corrupted the caller's + // parameters — see COMPILER_BUGS.md §2. The per-function + // RAM slots + prologue spill fix that class of bug at the + // cost of 4 LDA/STA pairs per function entry. + // + // Locals are laid out linearly across every function: + // NEScript forbids recursion (E0402) and enforces a + // bounded call depth (E0401), so lifetime overlap between + // functions is fine and we don't need to pack them. let mut local_ram_next: u16 = 0x0300; // Advance past any RAM global so locals don't clobber them. // Each global occupies `[address, address + size)` — for an @@ -236,17 +250,10 @@ impl<'a> IrCodeGen<'a> { local_ram_next = max_ram_global_end; } for func in &ir.functions { - for (i, local) in func.locals.iter().enumerate() { - if i < func.param_count { - if i < 4 { - var_addrs.insert(local.var_id, 0x04 + i as u16); - var_sizes.insert(local.var_id, local.size); - } - } else { - var_addrs.insert(local.var_id, local_ram_next); - var_sizes.insert(local.var_id, local.size); - local_ram_next += local.size.max(1); - } + for local in &func.locals { + var_addrs.insert(local.var_id, local_ram_next); + var_sizes.insert(local.var_id, local.size); + local_ram_next += local.size.max(1); } } let function_names = ir.functions.iter().map(|f| f.name.clone()).collect(); @@ -814,6 +821,36 @@ impl<'a> IrCodeGen<'a> { self.emit_label(&format!("__ir_fn_{}", func.name)); + // Prologue: spill the parameter-transport slots $04-$07 + // into the function's per-local RAM slots. The caller + // stages arguments into $04-$07 before the JSR; this + // copy makes sure that when the function body later + // makes a nested call (which clobbers $04-$07 again to + // pass its own arguments), the caller's parameters are + // still available in their dedicated RAM slots. + // + // Parameters are the first `param_count` entries of + // `func.locals`. The analyzer refuses functions with + // more than 4 parameters (E0506), so the `.take(4)` + // below is a defensive guard — it should never be hit + // in a well-formed program. + for (i, local) in func + .locals + .iter() + .take(func.param_count) + .take(4) + .enumerate() + { + if let Some(&addr) = self.var_addrs.get(&local.var_id) { + self.emit(LDA, AM::ZeroPage(0x04 + i as u8)); + if addr < 0x100 { + self.emit(STA, AM::ZeroPage(addr as u8)); + } else { + self.emit(STA, AM::Absolute(addr)); + } + } + } + // At the start of a frame handler that actually draws // sprites, clear the OAM shadow buffer so stale sprites from // the previous frame (or from a different state's handler) @@ -3598,3 +3635,73 @@ mod more_tests { ); } } + +#[test] +fn gen_function_prologue_spills_params_to_local_ram() { + // Regression test for COMPILER_BUGS.md §2: without the + // param-spill prologue, a function's parameters live only + // in the transport slots $04-$07. The first nested call + // inside the body would overwrite those slots with its + // own arguments, silently corrupting the caller's params. + // + // Compile a function that takes `x: u8`, calls `helper(x)`, + // then uses `x` again. Verify the callee reads `x` from a + // RAM slot (absolute addressing at $0300+) rather than + // directly from `$04`. + use crate::parser; + let src = r#" + game "Test" { mapper: NROM } + var out: u8 = 0 + fun helper(a: u8) -> u8 { return a } + fun caller(x: u8) -> u8 { + var tmp: u8 = helper(x) + return tmp + x + } + on frame { out = caller(42) } + start Main + "#; + let (prog, _) = parser::parse(src); + let prog = prog.unwrap(); + let analysis = crate::analyzer::analyze(&prog); + assert!( + analysis.diagnostics.iter().all(|d| !d.is_error()), + "unexpected analysis errors: {:?}", + analysis.diagnostics + ); + let ir = crate::ir::lower(&prog, &analysis); + let mut codegen = IrCodeGen::new(&analysis.var_allocations, &ir); + let insts = codegen.generate(&ir); + + // Find the __ir_fn_caller label. Immediately after it, look + // for the spill pattern: `LDA $04 / STA `. + let caller_idx = insts + .iter() + .position(|i| i.mode == AM::Label("__ir_fn_caller".into())) + .expect("caller function should be emitted"); + let mut saw_lda_zp4 = false; + let mut saw_sta_abs = false; + for inst in &insts[caller_idx + 1..] { + if let AM::Label(l) = &inst.mode { + if l.starts_with("__ir_fn_") && l != "__ir_fn_caller" { + break; + } + } + if inst.opcode == LDA && inst.mode == AM::ZeroPage(0x04) { + saw_lda_zp4 = true; + } + if saw_lda_zp4 && inst.opcode == STA { + if let AM::Absolute(a) = inst.mode { + if a >= 0x0300 { + saw_sta_abs = true; + break; + } + } + } + } + assert!( + saw_lda_zp4 && saw_sta_abs, + "caller function should open with `LDA $04 / STA ` \ + as the param-spill prologue — the fix for COMPILER_BUGS.md §2 \ + is not in effect" + ); +} diff --git a/src/ir/lowering.rs b/src/ir/lowering.rs index 768ae19..1ee9789 100644 --- a/src/ir/lowering.rs +++ b/src/ir/lowering.rs @@ -26,6 +26,14 @@ struct LoweringContext { /// symbol table). Used to decide between 8-bit and 16-bit IR /// ops for identifier reads/writes and binary operations. var_types: HashMap, + /// Current local scope prefix — mirrors the analyzer's field + /// of the same name. While lowering a function or handler + /// body this is `Some("")` (or `Some("State__frame")`, + /// etc), and `get_or_create_var` prepends + /// `"__local__{prefix}__"` to any bare identifier lookup so + /// function-local vars resolve to the scoped entry the + /// analyzer registered for them. `None` outside of any body. + current_scope_prefix: Option, next_var_id: u32, next_temp: u32, next_block: u32, @@ -98,6 +106,7 @@ impl LoweringContext { var_map, const_values: HashMap::new(), var_types, + current_scope_prefix: None, next_var_id, next_temp: 0, next_block: 0, @@ -113,13 +122,6 @@ impl LoweringContext { } } - /// Register a function parameter's type in the `var_types` map - /// so that identifier reads inside the function body know - /// whether to load as a byte or a word. - fn register_param_type(&mut self, name: &str, ty: &NesType) { - self.var_types.insert(name.to_string(), ty.clone()); - } - fn fresh_temp(&mut self) -> IrTemp { let t = IrTemp(self.next_temp); self.next_temp += 1; @@ -131,15 +133,30 @@ impl LoweringContext { format!("{prefix}_{}", self.next_block) } - fn get_or_create_var(&mut self, name: &str) -> VarId { - if let Some(&id) = self.var_map.get(name) { - id - } else { - let id = VarId(self.next_var_id); - self.next_var_id += 1; - self.var_map.insert(name.to_string(), id); - id + /// Resolve a user-written identifier to the scoped key used by + /// the symbol table. Mirrors `Analyzer::resolve_key`: tries the + /// current function/handler's qualified key first, falls back + /// to the bare key for globals / consts / enum variants / + /// state-level vars / function names. + fn scoped_key(&self, name: &str) -> String { + if let Some(prefix) = &self.current_scope_prefix { + let qualified = format!("__local__{prefix}__{name}"); + if self.var_map.contains_key(&qualified) || self.var_types.contains_key(&qualified) { + return qualified; + } } + name.to_string() + } + + fn get_or_create_var(&mut self, name: &str) -> VarId { + let key = self.scoped_key(name); + if let Some(&id) = self.var_map.get(&key) { + return id; + } + let id = VarId(self.next_var_id); + self.next_var_id += 1; + self.var_map.insert(key, id); + id } /// Recursively expand a struct-literal global initializer into @@ -408,8 +425,15 @@ impl LoweringContext { self.wide_hi.clear(); self.current_blocks = Vec::new(); self.current_locals = Vec::new(); + // Enter the function's local scope so all bare identifier + // lookups inside the body resolve against the analyzer's + // `__local__{function_name}__{name}` entries. + self.current_scope_prefix = Some(fun.name.clone()); - // Register parameters as locals + // Register parameters as locals. They're looked up via + // their bare name (which `get_or_create_var` now qualifies + // via `scoped_key`), so two different functions can each + // have a parameter named `x` without the VarIds colliding. for param in &fun.params { let var_id = self.get_or_create_var(¶m.name); self.current_locals.push(IrLocal { @@ -417,7 +441,10 @@ impl LoweringContext { name: param.name.clone(), size: type_size(¶m.param_type), }); - self.register_param_type(¶m.name, ¶m.param_type); + // Register the param type under the scoped key so + // `lower_expr` can decide 8-bit vs 16-bit loads. + let key = format!("__local__{}__{}", fun.name, param.name); + self.var_types.insert(key, param.param_type.clone()); } let entry = self.fresh_label(&format!("fn_{}_entry", fun.name)); @@ -443,21 +470,40 @@ impl LoweringContext { bank: fun.bank.clone(), source_span: fun.span, }); + self.current_scope_prefix = None; } fn lower_state(&mut self, state: &StateDecl, _is_start: bool) { - // Lower each event handler as a separate function + // Lower each event handler as a separate function. Each + // handler uses a distinct scope prefix so a `var i` in + // `Title::on frame` and one in `Playing::on frame` get + // different VarIds. if let Some(on_enter) = &state.on_enter { - self.lower_handler(&format!("{}_enter", state.name), on_enter, state); + self.lower_handler( + &format!("{}_enter", state.name), + &format!("{}__enter", state.name), + on_enter, + state, + ); } if let Some(on_exit) = &state.on_exit { - self.lower_handler(&format!("{}_exit", state.name), on_exit, state); + self.lower_handler( + &format!("{}_exit", state.name), + &format!("{}__exit", state.name), + on_exit, + state, + ); } if let Some(on_frame) = &state.on_frame { - self.lower_handler(&format!("{}_frame", state.name), on_frame, state); + self.lower_handler( + &format!("{}_frame", state.name), + &format!("{}__frame", state.name), + on_frame, + state, + ); } // Lower each scanline handler as a function named @@ -465,11 +511,12 @@ impl LoweringContext { // IRQ dispatch wrapper separately. for (line, block) in &state.on_scanline { let name = format!("{}_scanline_{line}", state.name); - self.lower_handler(&name, block, state); + let scope = format!("{}__scanline_{line}", state.name); + self.lower_handler(&name, &scope, block, state); } } - fn lower_handler(&mut self, name: &str, block: &Block, state: &StateDecl) { + fn lower_handler(&mut self, name: &str, scope_prefix: &str, block: &Block, state: &StateDecl) { self.next_temp = 0; // Same per-function reset as `lower_function`. See the // commentary there and COMPILER_BUGS.md §6 for why this is @@ -478,6 +525,7 @@ impl LoweringContext { // catastrophically wrong 16-bit IR ops. self.wide_hi.clear(); self.current_blocks = Vec::new(); + self.current_scope_prefix = Some(scope_prefix.to_string()); // Seed `current_locals` with the state's declared locals so any // `VarDecl` inside the handler body — tracked by // `lower_statement` via `current_locals` — is appended alongside @@ -488,6 +536,13 @@ impl LoweringContext { // addresses) would never see them. The result would be a // silent `LoadVar`/`StoreVar` emit-nothing bug that leaves the // temp slots uninitialized at runtime. + // + // State-level locals (declared at `state Foo { var i: u8 }` + // outside any handler) live in the GLOBAL scope so every + // handler in the state can read/write them across frames. + // `get_or_create_var` would try the scoped key first — + // which isn't registered for state-locals — then fall back + // to the bare key, which IS registered. self.current_locals = Vec::new(); for var in &state.locals { let var_id = self.get_or_create_var(&var.name); @@ -516,6 +571,7 @@ impl LoweringContext { bank: None, source_span: state.span, }); + self.current_scope_prefix = None; } fn lower_block(&mut self, block: &Block) { diff --git a/tests/emulator/goldens/arrays_and_functions.png b/tests/emulator/goldens/arrays_and_functions.png index 5032868..dfe577a 100644 Binary files a/tests/emulator/goldens/arrays_and_functions.png and b/tests/emulator/goldens/arrays_and_functions.png differ diff --git a/tests/emulator/goldens/platformer.audio.hash b/tests/emulator/goldens/platformer.audio.hash index f4619b8..b716f1b 100644 --- a/tests/emulator/goldens/platformer.audio.hash +++ b/tests/emulator/goldens/platformer.audio.hash @@ -1 +1 @@ -e8b3ea0b 132084 +f1b3c27b 132084 diff --git a/tests/emulator/goldens/war.audio.hash b/tests/emulator/goldens/war.audio.hash index e8f8ea6..7f338d6 100644 --- a/tests/emulator/goldens/war.audio.hash +++ b/tests/emulator/goldens/war.audio.hash @@ -1 +1 @@ -21f4a315 132084 +143660f 132084