From 548787ac8aa613e3d10608b541edf646f3f212bf Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 15 Apr 2026 23:19:07 +0000 Subject: [PATCH] W0110 inline fallback warning + docs refresh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit W0110: when a function marked `inline` has a body shape the IR lowerer can't splice (conditional early return, loops, nested control flow, empty void body), the analyzer now emits a warning at the declaration site so the declined hint is visible instead of silently falling back to a regular JSR. Implementation: - New `W0110` error code in `src/errors/diagnostic.rs` (warning level). - New `pub fn can_inline_fun(return_type, body) -> bool` in `src/ir/lowering.rs`, extracted from the existing capture logic so the analyzer and the IR lowerer share the same eligibility rules and can never drift. - New `check_inline_declinability` analyzer pass called from the tail of `analyze_program`, mirroring the existing `check_sprite_scanline_budget` / `check_unreachable_states` passes. Emits W0110 with help + note text pointing at the two accepted body shapes. - `capture_inline_bodies` now defers to `can_inline_fun` instead of duplicating the match pattern, so the two sides stay in lockstep by construction. Four regression tests in `src/analyzer/tests.rs` cover the conditional-return and while-loop declines plus the two accepted shapes (single-return expression, void sequence). Example source cleanups: `wrap52` in `examples/war/deck.ne` and `abs_diff` in both `examples/arrays_and_functions.ne` and `examples/loop_break_continue.ne` drop the `inline` keyword. All three were dead hints — the `inline` was being silently declined before this change, so removing it is source-only; the three ROMs are byte-identical, all 32 emulator goldens still match. Docs refresh - `docs/language-guide.md`: rewrote the Inline Functions section (real behaviour + W0110), added W0105/W0106/W0107/W0108/W0109/ W0110 to the warnings table, added the `debug.sprite_overflow*` builtins + sprite-per-scanline mitigations section to the Debug Mode docs, added a `cycle_sprites` statement entry and cross-referenced it from `draw`. - `docs/nes-reference.md`: fleshed out the "NEScript Memory Usage" block with the full ZP + high-RAM layout, including the new `$07EF` / `$07FC` / `$07FD` slots for sprite cycling and the debug sprite-overflow telemetry. - `docs/future-work.md`: documented all four debug query builtins in the "What ships today" block; updated the open "OAM allocation strategy" question to reference the shipped `cycle_sprites` path and ask about an automatic-flicker game attribute as a follow-up. - `docs/architecture.md`: updated the `ir/` and `optimizer/` module summaries to describe real inline splicing (now in lowering, not the optimizer). - `README.md`: reframed the `inline` bullet from "hint" to "real splicing for single-return / void-body shapes"; expanded the debug-support bullet to mention the four query builtins and their stripping in release builds; added a new bullet for the three-layer sprite-per-scanline mitigations; bumped the test count from 497 → 694; updated the war.ne entry to mention the seven compiler bugs are all fixed and point readers at `git log` (instead of the deleted COMPILER_BUGS.md). - `examples/README.md`: same `git log`-pointing rewrite for the war.ne entry. Deletions - `examples/war/COMPILER_BUGS.md` is removed. All seven catalogued bugs are fixed; the file's historical value lives in `git log` now. Every source-code comment and doc reference to the file has been updated to either point at `git log` or just describe the bug in place. Test count: 616 unit + 75 integration + 3 doctests = 694 total. Clippy / fmt clean. 32/32 emulator goldens match. https://claude.ai/code/session_0143dTgh3UeRrtfHgQwzcv5z --- README.md | 9 +- docs/architecture.md | 4 +- docs/future-work.md | 24 +- docs/language-guide.md | 149 +++++- docs/nes-reference.md | 19 +- examples/README.md | 2 +- examples/arrays_and_functions.ne | 6 +- examples/loop_break_continue.ne | 4 +- examples/war/COMPILER_BUGS.md | 769 ------------------------------- examples/war/PLAN.md | 23 +- examples/war/deck.ne | 7 +- src/analyzer/mod.rs | 50 ++ src/analyzer/tests.rs | 158 ++++++- src/codegen/ir_codegen.rs | 15 +- src/errors/diagnostic.rs | 5 +- src/ir/lowering.rs | 91 ++-- src/ir/mod.rs | 2 +- src/ir/tests.rs | 8 +- 18 files changed, 487 insertions(+), 858 deletions(-) delete mode 100644 examples/war/COMPILER_BUGS.md diff --git a/README.md b/README.md index 5cb1978..6d5905b 100644 --- a/README.md +++ b/README.md @@ -48,7 +48,7 @@ start Main - **Game-aware syntax** -- states, sprites, palettes, backgrounds, and input are first-class constructs - **Full type system** -- `u8`, `i8`, `u16`, `bool`, fixed-size arrays (`u8[N]`), `enum`, `struct` - **Rich control flow** -- `if`/`else`, `while`, `for i in 0..N`, `loop`, `match` -- **Functions** -- with parameters, return types, `inline` hint, recursion detection +- **Functions** -- with parameters, return types, real `inline fun` splicing for single-return and void-body shapes, recursion detection - **State machines** -- `state` with `on enter`, `on exit`, `on frame`, `on scanline(N)` handlers - **Compile-time safety** -- call depth limits, recursion detection, type checking, unused-var warnings - **IR-based optimizer** -- constant folding, dead code elimination, strength reduction (incl. div/mod by power-of-two), copy propagation, peephole passes including INC/DEC fold and live-range slot recycling @@ -59,7 +59,8 @@ start Main - **Asset pipeline** -- PNG-to-CHR conversion, inline tile data, sfx envelopes, music note streams - **Inline assembly** -- `asm { ... }` with `{var}` substitution, plus `raw asm { ... }` for verbatim blocks - **Hardware intrinsics** -- `poke(addr, value)` / `peek(addr)` for direct register access -- **Debug support** -- `--debug` flag enables `debug.log` / `debug.assert` writes to the emulator debug port +- **Debug support** -- `--debug` flag enables `debug.log` / `debug.assert`, runtime array bounds checks, frame-overrun and sprite-overflow counters, and the `debug.frame_overrun_count()` / `debug.frame_overran()` / `debug.sprite_overflow_count()` / `debug.sprite_overflow()` query builtins — all stripped entirely in release builds +- **Sprite-per-scanline mitigations** -- three layers of defense for the NES's 8-sprites-per-scanline hardware limit: compile-time `W0109` static check for literal layouts, runtime `cycle_sprites` flicker intrinsic for dynamic scenes, and debug-mode telemetry via `debug.sprite_overflow()` for playtest assertions - **Compile-time diagnostics** -- `--dump-ir`, `--memory-map`, `--call-graph` flags - **Single binary** -- no dependencies on ca65, Python, or any external tools @@ -95,7 +96,7 @@ start Main | [`metasprite_demo.ne`](examples/metasprite_demo.ne) | `metasprite Hero { sprite: ..., dx: [...], dy: [...], frame: [...] }` declarative multi-tile groups — `draw Hero at: (x, y)` expands to one OAM slot per tile so 16×16 sprites stop needing four hand-written `draw` statements | | [`sprite_flicker_demo.ne`](examples/sprite_flicker_demo.ne) | `cycle_sprites` — rotates the OAM DMA start offset one slot per frame so scenes with more than 8 sprites on a scanline drop a *different* one each frame. Turns the NES's permanent sprite-dropout hardware symptom into visible flicker, which the eye reconstructs from adjacent frames. Pairs with the compile-time `W0109` warning and the debug-mode `debug.sprite_overflow()` / `debug.sprite_overflow_count()` telemetry for a three-layer defense against the 8-sprites-per-scanline limit. | | [`platformer.ne`](examples/platformer.ne) | **End-to-end side-scroller** — custom CHR tileset, full background nametable, metasprite player with gravity/jump physics, wrap-around scrolling, stomp-or-die enemy collisions, live stomp-count HUD, pickup coins, user-declared SFX + music, and a Title → Playing → GameOver state machine with a proximity-based autopilot so the headless harness demonstrates the full gameplay loop (stomp, stomp, die, retry) inside six seconds | -| [`war.ne`](examples/war.ne) | **Production-quality card game** — a complete port of War split across `examples/war/*.ne`: title screen with a 0/1/2-player menu, animated deal, sliding face-up cards, deck-count HUD, "WAR!" tie-break with buried cards, victory screen with a fanfare, and a brisk 4/4 march on pulse 2. Pulls in nearly every NEScript subsystem (custom 88-tile sheet, felt nametable, 8-bit LFSR PRNG, queue-based decks, phase machine inside `Playing`, multiple sfx + music tracks). Building it surfaced five compiler bugs / limitations, all catalogued in [`examples/war/COMPILER_BUGS.md`](examples/war/COMPILER_BUGS.md) — two fixed in the same PR. | +| [`war.ne`](examples/war.ne) | **Production-quality card game** — a complete port of War split across `examples/war/*.ne`: title screen with a 0/1/2-player menu, animated deal, sliding face-up cards, deck-count HUD, "WAR!" tie-break with buried cards, victory screen with a fanfare, and a brisk 4/4 march on pulse 2. Pulls in nearly every NEScript subsystem (custom 88-tile sheet, felt nametable, 8-bit LFSR PRNG, queue-based decks, phase machine inside `Playing`, multiple sfx + music tracks). Building it surfaced seven compiler bugs, all fixed on the same branch — see `git log` for the details. | ## Compiler Commands @@ -136,7 +137,7 @@ NEScript implements all five planned milestones: | M4: Optimization | Done | Strength reduction, ZP promotion, type casting, asm-dump | | M5: Bank Switching | Done | MMC1/UxROM/MMC3, bank declarations, software mul/div | -497 tests across the lexer, parser, analyzer, IR, optimizer, codegen, assembler, linker, runtime, ROM, and asset modules, with CI running fmt, clippy, test, and example compilation on every push. +694 tests across the lexer, parser, analyzer, IR, optimizer, codegen, assembler, linker, runtime, ROM, and asset modules, plus a pixel- and audio-exact emulator harness that captures a golden framebuffer + audio hash for every example. CI runs fmt, clippy, test, example compilation, and the emulator harness on every push. ## License diff --git a/docs/architecture.md b/docs/architecture.md index a70651c..583b5a2 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -41,10 +41,10 @@ Each module has a `mod.rs` (implementation) and a co-located `tests.rs` with uni `mod.rs`, `tests.rs`. Performs semantic analysis on the AST: type checking, scope and symbol table management, call graph construction with depth analysis, state reachability, unused-variable detection, and dead-code-after-terminator warnings. Emits all user-facing diagnostics beyond lexer/parser syntax errors. ### `ir/` -`mod.rs` (types), `lowering.rs` (AST → IR), `tests.rs`. The IR is a flat, register-agnostic representation built from virtual temps and basic blocks. Lowering flattens nested expressions, expands 16-bit operations, desugars `for` into `while`, and resolves constant expressions early. +`mod.rs` (types), `lowering.rs` (AST → IR), `tests.rs`. The IR is a flat, register-agnostic representation built from virtual temps and basic blocks. Lowering flattens nested expressions, expands 16-bit operations, desugars `for` into `while`, resolves constant expressions early, and performs real `inline fun` splicing — functions marked `inline` whose bodies match one of the two splicable shapes (single `return ` or void statement sequence) are captured before lowering begins and substituted at every call site. Bodies that don't match (conditional returns, loops, nested control flow) fall back to regular out-of-line calls and the analyzer emits `W0110` at the declaration. ### `optimizer/` -`mod.rs`, `tests.rs`. Runs passes over the IR in order: strength reduction (mul/div by powers of two, mod by powers of two, ShiftVar → ShiftLeft/Right), function inlining (trivial-function elimination), constant folding (arithmetic + comparisons + shifts), and dead code elimination. +`mod.rs`, `tests.rs`. Runs passes over the IR in order: strength reduction (mul/div by powers of two, mod by powers of two, ShiftVar → ShiftLeft/Right), constant folding (arithmetic + comparisons + shifts), copy propagation, and dead code elimination. The `inline fun` splicing happens one phase earlier in `ir/lowering.rs` so the optimizer sees already-inlined call sites. ### `codegen/` `ir_codegen.rs`, `peephole.rs`, `mod.rs`. `ir_codegen.rs` walks the optimized IR and emits 6502 instructions; variables land in allocated addresses and IR temps land in a recycling zero-page slot pool. `peephole.rs` runs after codegen to clean up the temp-heavy output (dead-load elimination, branch folding, INC/DEC fold, copy propagation). diff --git a/docs/future-work.md b/docs/future-work.md index 46b2536..1774817 100644 --- a/docs/future-work.md +++ b/docs/future-work.md @@ -151,11 +151,16 @@ op and writes a plain-text map of ` ` entries for every lowered statement. Debug builds emit array bounds checks (CMP against size, BCC past a `JMP __debug_halt` wedge) and bump an overrun counter at `$07FF` in the NMI handler when the main loop didn't -reach `wait_frame` before the next vblank. **Plus** two new query -expressions: `debug.frame_overrun_count()` returns the cumulative counter -and `debug.frame_overran()` returns a per-frame sticky bit (cleared by -the next `wait_frame`) so user code can write -`debug.assert(not debug.frame_overran())` guards. +reach `wait_frame` before the next vblank. + +**Plus four query expressions** that mirror the counter/sticky pattern: +`debug.frame_overrun_count()` / `debug.frame_overran()` return the cumulative +overrun counter and a per-frame sticky bit so user code can write +`debug.assert(not debug.frame_overran())` guards, and +`debug.sprite_overflow_count()` / `debug.sprite_overflow()` do the same for +the NES PPU's sprite-per-scanline flag (`$2002` bit 5), which the NMI +handler samples once per frame in debug mode. All four sticky bits clear +on the next `wait_frame`. **Still TODO.** - **`debug.overlay(x, y, text)`** — needs the text/HUD subsystem (see @@ -204,8 +209,13 @@ semantics come back, add the codes at that point. would be cheap but would invalidate any copy-pasted ca65 fragments. 2. **Debug port address.** $4800 is conventional but not universal. Should we support multiple debug output methods? -3. **OAM allocation strategy.** Sequential allocation vs priority-based with - automatic sprite cycling for the 8-per-scanline limit? +3. **OAM allocation strategy.** Sequential allocation remains the default; + the `cycle_sprites` opt-in keyword rotates the DMA offset each frame so + scenes past the 8-per-scanline budget flicker instead of dropping the + same sprite every frame. Open question: should automatic cycling become + a `game` attribute (`sprite_flicker: true`) that emits the increment + without requiring a per-frame call, and/or add a `draw ... priority: + pinned` modifier for HUD sprites that must stay at low OAM slots? 4. **Error recovery granularity.** How aggressively should the parser recover? More recovery means more errors per compile but also risks cascading false errors. diff --git a/docs/language-guide.md b/docs/language-guide.md index 4717e0c..c840e63 100644 --- a/docs/language-guide.md +++ b/docs/language-guide.md @@ -256,18 +256,42 @@ fun reset_score() { ### Inline Functions -The `inline` keyword hints the compiler to inline the function at call sites: +The `inline` keyword marks a function for inlining at call sites. The IR +lowering pass captures the body up front and substitutes it wherever the +function is called, skipping the normal `JSR` entirely. Two body shapes +are accepted: + +**Single-return expression** — a function with a declared return type +whose body is exactly `{ return }`. The expression is re-lowered +in place of each call, with every parameter name substituted for the +caller's argument temps. ``` -inline fun clamp(val: u8, max: u8) -> u8 { - if val > max { - return max - } - return val +inline fun card_rank(card: u8) -> u8 { + return card >> 4 } ``` -`inline` is a hint -- the compiler may decline for large functions. +**Void multi-statement** — a function with no return type whose body is +a sequence of plain statements (assigns, calls, draws, scroll, +`set_palette`, `load_background`, `wait_frame`, `cycle_sprites`, inline +asm, or the `debug.*` builtins). Nested control flow, `return`, +`break`, `continue`, and `transition` are not allowed. + +``` +inline fun set_phase(p: u8) { + phase = p + phase_timer = 0 + cursor_x = 0 +} +``` + +Functions marked `inline` whose body doesn't match either shape (a +conditional early return, a `while` loop, nested `if`/`else`, etc.) +fall back to a regular out-of-line `JSR` call. The compiler emits a +`W0110` warning at the declaration site so the declined hint is +visible — rewrite the body to fit one of the two shapes, or drop the +`inline` keyword if the call overhead is acceptable. ### Calling Functions @@ -610,7 +634,11 @@ draw Player at: (player_x, player_y) draw Coin at: (COIN_X, COIN_Y) frame: anim_frame ``` -The `draw` statement writes to the OAM shadow buffer. The NES supports up to 64 sprites per frame. +The `draw` statement writes to the OAM shadow buffer. The NES supports +up to 64 sprites per frame, and the PPU can only render 8 sprites per +scanline — see the `cycle_sprites` statement below and the +[sprite-per-scanline mitigations](#sprite-per-scanline-mitigations) +section for how to handle scenes that exceed the 8-per-scanline budget. Syntax: `draw SpriteName at: (x_expr, y_expr) [frame: expr]` @@ -634,6 +662,38 @@ wait_frame() This triggers OAM DMA transfer and PPU updates before yielding. Inside `on frame`, a `wait_frame()` is implicit at the end of each frame. +### Cycle Sprites + +Rotate the runtime's sprite-cycling offset by one OAM slot (4 bytes), +naturally wrapping at 256 back to 0. When any statement in a program +emits `cycle_sprites`, the linker switches the NMI handler over to a +variant that writes the current offset byte (at `$07EF`) to `$2003` +before triggering the OAM DMA — so each frame's DMA lands in a +different slot of the PPU's OAM buffer. + +``` +on frame { + draw Enemy0 at: (e0x, e0y) + draw Enemy1 at: (e1x, e1y) + // ...lots of enemies... + cycle_sprites + wait_frame +} +``` + +The practical effect is the classic NES flicker: scenes with more than +8 sprites on a single scanline drop a *different* sprite on each +frame, and the eye reconstructs the missing pixels from frame +persistence. Permanent dropout becomes visible flicker, which reads as +a hardware limit rather than a game bug. + +`cycle_sprites` is opt-in by design. Programs that never call it emit +the original fixed-offset NMI path (byte-identical to every +pre-cycling ROM). See +[sprite-per-scanline mitigations](#sprite-per-scanline-mitigations) +for when to use it together with the compile-time `W0109` warning and +the debug-mode `debug.sprite_overflow*()` telemetry. + ### Scroll Set the PPU scroll position: @@ -1107,7 +1167,67 @@ In debug mode, the compiler inserts: - Array bounds checking on indexed access - Arithmetic overflow warnings - Stack depth monitoring at function entry -- Frame overrun detection (warns if frame handler exceeds vblank period) +- Frame overrun detection (bumps a counter at `$07FF` whenever the + frame handler runs past vblank) +- Sprite-per-scanline overflow detection (bumps a counter at `$07FD` + whenever the PPU's sprite overflow flag at `$2002` bit 5 was set + for the just-finished frame) + +### Debug Queries + +Four builtin expressions let user code inspect the debug counters and +sticky bits. All four return a `u8`, peek a fixed runtime address in +debug builds, and compile to a constant zero in release builds (so +`debug.assert(not debug.frame_overran())` guards disappear entirely +when you ship). + +``` +var n: u8 = debug.frame_overrun_count() // cumulative overruns since reset +debug.assert(not debug.frame_overran()) // sticky bit, cleared on next wait_frame + +var s: u8 = debug.sprite_overflow_count() // cumulative PPU sprite overflows +debug.assert(not debug.sprite_overflow()) // sticky bit, cleared on next wait_frame +``` + +The sprite overflow pair reads the NES hardware flag (`$2002` bit 5), +which has a few well-known quirks but is correct for the overwhelming +majority of cases. Use it together with the compile-time `W0109` static +check and the runtime `cycle_sprites` flicker mitigation — see the +sprite-per-scanline section below. + +### Sprite-per-scanline mitigations + +The NES PPU can only render 8 sprites per scanline. Anything past the +budget is silently dropped, and because sprites land in the shadow OAM +in draw order, the same sprite gets dropped every frame — a permanent +dropout that reads as a bug rather than a hardware limit. NEScript +ships three layers of mitigation: + +1. **Compile time** — the `W0109` warning fires on layouts with more + than 8 literal-coordinate sprites overlapping any scanline. Catches + static HUDs, text labels, and title screens. +2. **Runtime** — the `cycle_sprites` keyword statement bumps a + rotating offset byte at `$07EF`. A cycling variant of the NMI + handler writes that byte to `$2003` before the OAM DMA, so each + frame's DMA lands in a different slot of the PPU's OAM buffer. + Over N frames each of the N overlapping sprites gets dropped + approximately once, producing visible flicker the eye + reconstructs from frame persistence — the classic NES idiom + used by Gradius, Battletoads, and every shmup. +3. **Playtesting** — `debug.sprite_overflow()` / + `debug.sprite_overflow_count()` expose the PPU hardware flag as + debug queries so user code can assert the budget holds, or a + debug overlay can display the running count. + +``` +on frame { + // ... draw all your sprites ... + cycle_sprites // rotate one slot per frame + wait_frame +} +``` + +See `examples/sprite_flicker_demo.ne` for the end-to-end flow. --- @@ -1214,6 +1334,17 @@ reference NEScript variables. | W0102 | Loop without break or wait_frame | | W0103 | Unused variable | | W0104 | Unreachable code (after return/break/transition, or state unreachable from start) | +| W0105 | Palette sub-palette universal mismatch (mirror collision) | +| W0106 | Implicit drop of non-void function return value | +| W0107 | `fast` variable rarely accessed (wastes a zero-page slot) | +| W0108 | Array elements past byte 255 unreachable via 8-bit X index | +| W0109 | More than 8 literal-coordinate sprites overlap one scanline (NES hardware limit — see `cycle_sprites` and `debug.sprite_overflow()` for runtime mitigations) | +| W0110 | `inline fun` body shape cannot be inlined; falling back to a regular `JSR` call (rewrite as a single-return expression or a void statement sequence, or drop the `inline` keyword) | + +`nescript build` prints warnings in addition to errors on a successful +compile, so code-quality hints surface during normal development without +needing a separate `nescript check` pass. Errors still halt the build; +warnings never do. ### Example Error Output diff --git a/docs/nes-reference.md b/docs/nes-reference.md index 55df451..88d6198 100644 --- a/docs/nes-reference.md +++ b/docs/nes-reference.md @@ -53,10 +53,23 @@ The 6502's 3-register architecture drives the compiler's register allocator. The ### NEScript Memory Usage The compiler reserves the following: -- `$00`-`$0F`: System use (frame counter, temp registers, NMI flags) -- `$10`-`$FF`: Available for `fast` variables and compiler-promoted variables +- `$00`-`$0F`: System use (frame counter, input, OAM cursor, SFX/music pointers, mul/div scratch) +- `$10`: `ZP_BANK_CURRENT` (current switchable PRG bank index, only in banked programs) +- `$11`-`$17`: PPU update slots (palette/nametable flags and pending pointers, only when the program declares palette or background blocks) +- `$18`-`$7F`: Available for `fast` variables (user zero-page) +- `$80`-`$FF`: IR codegen temp slots (scratch for expression evaluation) - `$0200`-`$02FF`: OAM shadow buffer (DMA'd to PPU each frame) -- `$0300`-`$07FF`: General variables, state-local storage +- `$0300`-`$07EE`: General variables, per-function RAM (parameter spill + locals), state-local storage +- `$07EF`: `SPRITE_CYCLE_ADDR` — rotating offset byte used by `cycle_sprites` (only when the program emits a `cycle_sprites` statement) +- `$07F0`-`$07F7`: Audio channel state (noise/triangle/sfx-pitch pointers; only when the program declares a matching sfx) +- `$07FC`: `DEBUG_SPRITE_OVERFLOW_FLAG_ADDR` — per-frame sticky bit (debug builds only) +- `$07FD`: `DEBUG_SPRITE_OVERFLOW_COUNT_ADDR` — cumulative PPU sprite overflow counter (debug builds only) +- `$07FE`: `DEBUG_FRAME_OVERRUN_FLAG_ADDR` — per-frame sticky bit (debug builds only) +- `$07FF`: `DEBUG_FRAME_OVERRUN_ADDR` — cumulative frame overrun counter (debug builds only) + +Release-mode programs that don't opt into audio, banking, debug, or +sprite cycling leave the corresponding slots untouched and the +analyzer is free to allocate user globals over them. --- diff --git a/examples/README.md b/examples/README.md index 2808b2d..5dde9a0 100644 --- a/examples/README.md +++ b/examples/README.md @@ -37,7 +37,7 @@ Open any `.nes` file in an NES emulator ([Mesen](https://www.mesen.ca/), [FCEUX] | `nested_structs.ne` | nested struct fields, array struct fields, chained literals | Two `Hero` instances each carry a `Vec2` position and a `u8[4]` inventory. Exercises `hero.pos.x` chained access, `hero.inv[i]` array-field access, and chained struct-literal initializers (`Hero { pos: Vec2 { x: ..., y: ... }, inv: [...] }`). | | `platformer.ne` | **every subsystem** | End-to-end side-scrolling demo: custom CHR tileset, full 32×30 nametable with per-region attribute palettes, 2×2 metasprite hero with gravity/jump physics, wrap-around horizontal scrolling, stomp-or-die enemy collisions with a live stomp-count HUD, coin pickups, user-declared SFX + music, and a Title → Playing → GameOver state machine with a proximity-based autopilot so the headless harness cycles through stomp, stomp, die, and retry inside six seconds. Regenerate the tile art with `cargo run --bin gen_platformer_tiles`. | | `sprite_flicker_demo.ne` | `cycle_sprites`, 8-per-scanline hardware limit | Twelve sprites packed onto the same 4-pixel band — two more than the NES's 8-sprites-per-scanline hardware budget. The W0109 analyzer warning fires at compile time, and a `cycle_sprites` call at the end of `on frame` rotates the OAM DMA offset one slot per frame so the PPU drops a *different* sprite each frame. The permanent-dropout failure mode becomes visible flicker, which the eye reconstructs across frames. The classic NES technique used by Gradius, Battletoads, and every shmup that ever existed. | -| `war.ne` | **production-quality card game**, multi-file source layout | A complete port of the card game War, split across `examples/war/*.ne` files and pulled in via `include` directives. Title screen with a 0/1/2-player menu (cursor sprite, blinking PRESS A, brisk 4/4 march on pulse 2), a 50-frame deal animation, a deep `Playing` state with an inner phase machine (`P_WAIT_A`/`P_FLY_A`/.../`P_WAR_BANNER`/`P_WAR_BURY`/`P_CHECK`), card-conserving queue-based decks built on a 200-iteration random-swap shuffle, a "WAR!" tie-break that buries 3+1 face-down cards per player and plays a noise-channel thump per bury, and a victory screen with the builtin fanfare. The first NEScript example to use a top-level file as a thin shell that `include`s ~12 component files; building it surfaced and fixed two compiler bugs (E0506 too-many-params, and the IR-lowering `wide_hi` leak across functions). The remaining limitations and workarounds are catalogued in [`war/COMPILER_BUGS.md`](war/COMPILER_BUGS.md). | +| `war.ne` | **production-quality card game**, multi-file source layout | A complete port of the card game War, split across `examples/war/*.ne` files and pulled in via `include` directives. Title screen with a 0/1/2-player menu (cursor sprite, blinking PRESS A, brisk 4/4 march on pulse 2), a 50-frame deal animation, a deep `Playing` state with an inner phase machine (`P_WAIT_A`/`P_FLY_A`/.../`P_WAR_BANNER`/`P_WAR_BURY`/`P_CHECK`), card-conserving queue-based decks built on a 200-iteration random-swap shuffle, a "WAR!" tie-break that buries 3+1 face-down cards per player and plays a noise-channel thump per bury, and a victory screen with the builtin fanfare. The first NEScript example to use a top-level file as a thin shell that `include`s ~12 component files; building it surfaced seven compiler bugs across the analyzer, IR lowerer, and codegen that were all fixed on the same branch (see `git log` for details). | ## Emulator Controls diff --git a/examples/arrays_and_functions.ne b/examples/arrays_and_functions.ne index bd2163d..8086448 100644 --- a/examples/arrays_and_functions.ne +++ b/examples/arrays_and_functions.ne @@ -28,7 +28,11 @@ fun clamp(val: u8, max: u8) -> u8 { } // Inline function: absolute difference -inline fun abs_diff(a: u8, b: u8) -> u8 { +// Not marked `inline`: the conditional early return is one of +// the shapes the inliner declines (W0110). Living with the JSR +// is the correct call here since rewriting as a branchless max +// wouldn't fit in a single-return expression. +fun abs_diff(a: u8, b: u8) -> u8 { if a > b { return a - b } diff --git a/examples/loop_break_continue.ne b/examples/loop_break_continue.ne index fb9cebf..5d8f0ad 100644 --- a/examples/loop_break_continue.ne +++ b/examples/loop_break_continue.ne @@ -38,7 +38,9 @@ var hit: u8 = 0 // 1 if the player is touching any hazard var hit_idx: u8 = 0 // which slot was hit (only meaningful if hit == 1) // Small absolute-difference helper. Saves lines at each call site. -inline fun abs_diff(a: u8, b: u8) -> u8 { +// Not marked `inline`: the conditional early return is one of +// the shapes the inliner declines (W0110). +fun abs_diff(a: u8, b: u8) -> u8 { if a > b { return a - b } diff --git a/examples/war/COMPILER_BUGS.md b/examples/war/COMPILER_BUGS.md deleted file mode 100644 index 8c37f4e..0000000 --- a/examples/war/COMPILER_BUGS.md +++ /dev/null @@ -1,769 +0,0 @@ -# NEScript v0.1 — Compiler Bugs and Limitations Found While Building War - -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, 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 | **FIXED** (W0109 static analyzer warning) | `analyzer: add W0109 sprite-per-scanline budget check` | `analyze_sprite_scanline_budget_warns_over_eight`, `analyze_sprite_scanline_budget_ok_when_staggered`, `analyze_sprite_scanline_budget_skips_dynamic_coords`, `analyze_sprite_scanline_budget_expands_metasprites`, `analyze_sprite_scanline_budget_recurses_into_if` | -| 5 | `inline` keyword silently declined for short functions | **FIXED** (IR lowering now inlines expression and void bodies) | `ir: real inlining for single-return and void-body `inline fun`s` | `ir::tests::inline_fun_expression_body_emits_no_call_at_use_site`, `inline_fun_void_body_statements_are_spliced`, `inline_fun_with_conditional_return_compiles_as_regular_call`, `inline_fun_nested_inlines_substitute_correctly` | -| 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. All seven -catalogued bugs have now shipped their fixes; the example code -no longer carries any workaround comments. - ---- - -## 1. Functions with more than 4 parameters silently corrupt the 5th+ *(FIXED)* - -### Symptom - -Calling a function with 5 or 6 parameters compiles cleanly, with -no warning or error, but at runtime the 5th and 6th parameter -values are silently replaced by garbage (typically the value of -parameter 3 or 4). Animations and state writes that depend on -those parameters behave as if zero was passed. - -### Reproduction - -```nescript -fun arm_fly(sx: u8, sy: u8, dxsign: u8, dysign: u8, card: u8, fu: u8) { - fly_x = sx - fly_y = sy - fly_dx_sign = dxsign - fly_dy_sign = dysign - fly_card = card // gets the value of dxsign instead! - fly_face_up = fu // gets the value of dxsign instead! -} - -fun caller() { - arm_fly(32, 64, 0, 0, 147, 1) - // After this call: - // fly_x = 32, fly_y = 64, fly_dx_sign = 0, fly_dy_sign = 0 - // fly_card = 0 (NOT 147) - // fly_face_up = 0 (NOT 1) -} -``` - -### Root cause - -`src/codegen/ir_codegen.rs` (around line 240) iterates through -`func.locals` and assigns the first 4 entries to zero-page -parameter slots `$04`-`$07`: - -```rust -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); - ... - } - } else { - ... - } - } -} -``` - -The `if i < 4` guard silently drops the mapping for params 5+ -without inserting any RAM allocation for them. The corresponding -caller-side codegen for `Call` writes only the first four -arguments. Result: params 5 and 6 are never passed and the -callee reads stale memory from $04-$07 in their place. - -### Workaround used in `examples/war/` - -`arm_fly` is split: the four "arming" parameters stay in the -function signature, and `fly_card` / `fly_face_up` are written to -the global state directly at every call site instead. See -`war/play_state.ne` (`begin_draw_a` / `begin_draw_b`). - -### Fix proposal - -Two reasonable options: - -1. **Diagnose-only**: emit `E05XX too many parameters` when a - `fun` declaration has more than 4 params. This is the - smallest possible change and turns silent miscompiles into a - loud compile-time error. Should ship immediately even if - option 2 is also planned. - -2. **Spill to RAM**: extend the calling convention so params - beyond the first four are passed via dedicated RAM slots in - the callee's local frame. The caller-side `Call` codegen - would write those slots before `JSR`, the callee-side prologue - could leave them as-is. This grows the per-function RAM - footprint but lets users write any signature they like. - ---- - -## 1b. Function parameters with the same name in different functions share a VarId, which collides their zero-page slot mapping *(FIXED)* - -### Symptom - -Two unrelated functions whose parameters happen to be named the -same (e.g. both have a `card: u8` parameter, or both have an -`x: u8` parameter) end up reading parameters from the wrong -zero-page slot at runtime. One function reads `$04`, another -reads `$06`, a third reads `$05` — depending on the parameter's -*position* in whichever function is processed last by the -codegen. - -This is a much sneakier sibling of bug #1: rather than dropping -a parameter past the 4th slot, it silently reroutes parameter -reads to slots that hold completely unrelated values from the -caller. - -### Reproduction - -```nescript -// Function A: card is the 1st parameter, expected at $04 -fun push_back_a(card: u8) { - deck_a[deck_a_front] = card // reads from $06, not $04! - deck_a_count += 1 -} - -// Function B: card is the 3rd parameter, expected at $06 -fun draw_card_face(x: u8, y: u8, card: u8) { - // ... uses card normally ... -} -``` - -The IR lowering assigns `card` a single shared `VarId` because -its `var_map` is global across all functions. The codegen then -walks each function in turn, inserting `(VarId(card), $0X)` -mappings into a single global `var_addrs` `HashMap` — and -whichever function comes last in iteration order wins the -mapping. If `draw_card_face` is processed after `push_back_a`, -`VarId(card)` ends up mapped to `$06`, and `push_back_a` then -reads its `card` parameter from `$06` (which holds whatever the -caller was using as a third argument — typically junk). - -### Root cause - -`src/ir/lowering.rs::get_or_create_var` looks up names in -`self.var_map`, which is shared across the whole program: - -```rust -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 - } -} -``` - -`lower_function` calls `get_or_create_var(¶m.name)` for each -parameter, so two different functions both with a `card` -parameter resolve to the same `VarId`. Once that single `VarId` -flows into the codegen, the per-function "this is param index N -of function F" relationship is lost — there's only one global -mapping per `VarId`. - -### Workaround used in `examples/war/` - -Every parameter name in the war source is unique across the -entire program. Function-locals were already prefixed by -function (see bug #3); we extended the same scheme to params: -`push_back_a(pba_arg_card: u8)` instead of -`push_back_a(card: u8)`, etc. The wrapping `pba_card` / -`pbb_card` / `dcf_card` snapshots from bug #2 stay because they -also help with the bug-2 clobbering. - -### Fix - -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. - -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`). - -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. - -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` *(FIXED)* - -### Symptom - -A function that takes parameters and then calls another function -sees its own parameters silently replaced by the inner call's -arguments. Any code path that reads the original parameter -*after* the inner call gets the wrong value. - -### Reproduction - -```nescript -fun draw_card_face(x: u8, y: u8, card: u8) { - var rank: u8 = card_rank(card) // x at $04 is now `card` - var suit: u8 = card_suit(card) // x at $04 is still `card` - // x is supposed to be 120 here, but it's actually `card` - var x1: u8 = x + 8 // computes card + 8, not 120 + 8 - draw Tileset at: (x, y) frame: ... // draws at x = card, not 120 -} -``` - -Concretely, calling `draw_card_face(120, 128, 0x93)` puts the -card sprite at `(0x93, 128)` — completely wrong. - -### Root cause - -Same allocator as bug #1: `func.locals[0..param_count]` are -mapped to `$04`, `$05`, `$06`, `$07`. The caller writes its own -arguments into the same zero-page slots before `JSR`, so the -caller's parameters at those slots get clobbered by the callee's -arguments. There is no save/restore wrapper around `JSR` and no -spill/reload pass to refresh the caller's parameters from a -backing copy. - -### Workaround used in `examples/war/` - -Every helper that takes parameters AND makes any nested function -call snapshots its parameters into fresh local variables at the -top of the function, then references the locals exclusively -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 - -`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: - -``` -LDA $04 ; transport slot 0 -STA -LDA $05 ; transport slot 1 -STA -... etc ... -``` - -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 *(FIXED)* - -### Symptom - -Two different functions cannot declare locals with the same -name. The compiler emits `E0501 duplicate declaration of ''` -even though the locals are in disjoint scopes. - -### Reproduction - -```nescript -fun foo() { - var i: u8 = 0 - while i < 10 { i += 1 } -} - -fun bar() { - var i: u8 = 0 // E0501 duplicate declaration of 'i' - while i < 5 { i += 1 } -} -``` - -### Root cause - -`src/analyzer/mod.rs::register_var` inserts every `var` -declaration into a single `self.symbols` map keyed only on the -variable's name, with no qualification by function or block: - -```rust -fn register_var(&mut self, var: &VarDecl) { - if self.symbols.contains_key(&var.name) { - self.diagnostics.push(Diagnostic::error( - ErrorCode::E0501, - format!("duplicate declaration of '{}'", var.name), - var.span, - )); - return; - } - ... -} -``` - -`check_statement` calls `register_var` for every `Statement::VarDecl` -encountered while walking function bodies, so all locals across -all functions and all nested blocks land in the same namespace. - -### Workaround used in `examples/war/` - -Every function-local variable is prefixed with a short tag -identifying its enclosing function (e.g. `dfa_card` in -`draw_front_a`, `pba_slot` in `push_back_a`, -`dwp_px` in `draw_word_player`). This makes long files harder to -read but is fully mechanical. - -### Fix - -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. - -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. - ---- - -## 4. Per-frame sprite-per-scanline limit is invisible to user code *(FIXED)* - -### Symptom - -Drawing more than 8 sprites whose Y rectangles intersect a -single scanline causes the NES PPU to silently drop the excess -sprites past the 8th in OAM order. Letters or tiles just don't -render, and prior to this fix the compiler emitted no warning -even when the entire layout was a tree of literal coordinates -it could have checked. - -### Reproduction - -```nescript -// 9 letters all on the same Y row: -draw Letter at: (0, 100) -draw Letter at: (8, 100) -draw Letter at: (16, 100) -draw Letter at: (24, 100) -draw Letter at: (32, 100) -draw Letter at: (40, 100) -draw Letter at: (48, 100) -draw Letter at: (56, 100) -draw Letter at: (64, 100) // past budget — silently dropped -``` - -Pre-fix the compiler said nothing and the 9th letter never -showed up on hardware. Post-fix the analyzer emits: - -``` -warning[W0109]: state 'Main' draws 9 literal-coordinate sprites - overlapping scanline 100; the NES renders at - most 8 sprites per scanline - = help: stagger draws vertically by at least 8 pixels, reduce - the number of on-screen sprites, or split the draws across - `on_scanline` handlers - = note: the 9th and later sprites on a scanline are dropped - by the PPU, causing flicker or invisible objects on real - hardware -``` - -### Root cause - -The 8-sprites-per-scanline cap is a real NES hardware -constraint, not a compiler bug — but NEScript had no static -check to catch the cases where user code makes the problem -obvious at compile time, even though the draw allocator is -sequential and the literal coords it sees are trivially -checkable. - -### Workaround used in `examples/war/` - -We staggered text rows by hand. The title screen's "WAR / -CARD GAME / 0 PLAYER / 1 PLAYER / 2 PLAYER" layout sits each -row at a different y so no scanline carries more than 7 -sprites; the victory screen's "PLAYER X / WINS" wraps after -the player letter for the same reason. These layouts stay in -place post-fix — they now pass the analyzer cleanly because -they're under budget. - -### Fix - -`src/analyzer/mod.rs::check_sprite_scanline_budget` runs at -the end of `analyze_program`. For each state's `on_frame` -handler it walks the block tree (including nested -`if`/`while`/`for`/`loop`) collecting literal-coordinate -`draw` statements into a `Vec<(y, x, span)>`. Metasprite -draws expand into one tuple per tile via the metasprite's -`dx`/`dy` offset arrays, so a metasprite that covers four -tiles on the same y contributes four sprites to the overlap -count. Non-literal coordinates are skipped entirely because -the static analysis can't know where they land at runtime. - -With the tuples collected, the analyzer iterates every -scanline 0..240 and counts sprites whose `y <= scanline < -y+8`. The worst scanline is cached and, if the count exceeds -8, a `W0109` diagnostic is emitted with labels pointing at -every draw site that contributed (deduplicated so metasprite -expansions don't spam the message). - -Only `on_frame` is checked. `on_enter` / `on_exit` fire once -per transition and aren't the hot sprite path; checking them -would produce false positives on brief splash animations. -Conditional branches are unioned (conservative over-count) — -a sprite drawn inside an `if` counts for budget purposes even -if its runtime branch is exclusive with a sibling's. The -trade-off: the check stays local and simple, at the cost of -occasionally flagging hand-sliced layouts that the user knows -are actually safe. - -### Regression tests - -Five tests in `src/analyzer/tests.rs`: - -- `analyze_sprite_scanline_budget_warns_over_eight` — nine - literal draws on the same `y` trips W0109. -- `analyze_sprite_scanline_budget_ok_when_staggered` — nine - draws each on a different `y` row are silent. -- `analyze_sprite_scanline_budget_skips_dynamic_coords` — - draws with a `var`-backed `x` are skipped (no false - positive) because the analysis can't resolve them. -- `analyze_sprite_scanline_budget_expands_metasprites` — a - four-tile metasprite drawn three times trips W0109 because - the analyzer expands each draw into its per-tile offsets. -- `analyze_sprite_scanline_budget_recurses_into_if` — nine - draws inside an `if` block still trip W0109 (conservative - over-count). - -### Layer-2: runtime sprite cycling (`cycle_sprites`) - -W0109 only catches the literal-coordinate case — a game with ->8 dynamically-positioned sprites (enemies, projectiles, -animated NPCs) is invisible to it. The hardware will still -drop the 9th+ sprite on every frame, and because draw order -is stable frame-to-frame the *same* sprite goes missing every -frame, which reads to the developer as a game bug rather -than a hardware limit. - -The classic NES mitigation is sprite cycling: rotate the OAM -DMA start offset each frame so different sprites land in the -PPU's "first 8" on each successive frame. Over N frames (where -N is the number of overlapping sprites) each sprite gets -dropped exactly once, and the eye reconstructs the missing -pixels from frame persistence. Permanent dropout becomes -visible flicker — the failure mode every NES player -recognises, and vastly better UX than "my bullet disappeared." - -NEScript ships this as the opt-in `cycle_sprites` statement: - -```nescript -on frame { - draw Enemy0 at: (e0x, e0y) - draw Enemy1 at: (e1x, e1y) - // ...lots of enemies... - cycle_sprites // bump the rotating offset one slot - wait_frame -} -``` - -Each call adds 4 to a one-byte runtime counter at `$07EF` -(natural u8 wrap at 256 → 0) and emits a `__sprite_cycle_used` -marker label. The linker reads the marker and swaps the NMI -handler over to a variant that writes the counter to `$2003` -before triggering the OAM DMA, so each frame's DMA lands in -a different slot of the PPU's OAM buffer. Over 64 frames the -rotation completes a full cycle. - -Programs that don't call `cycle_sprites` emit no marker and -get the original fixed-offset NMI path, so every existing -golden ROM stays byte-identical. Opt-in by design — the -tradeoff is "cosmetic HUD elements you pinned to slot 0 lose -their pin" — so programs that manage OAM priority manually -can keep doing so. - -The [`examples/sprite_flicker_demo.ne`](../sprite_flicker_demo.ne) -example drives 12 sprites into a 4-pixel band to exercise -both W0109 at compile time and `cycle_sprites` at runtime; -the committed emulator golden captures a specific frame of -the cycling pattern. - -### Layer-3: debug-mode runtime overflow telemetry - -`debug.sprite_overflow_count()` and `debug.sprite_overflow()` -mirror the existing `debug.frame_overrun_count()` / -`debug.frame_overran()` pair. In debug builds the NMI handler -samples the PPU's sprite-overflow flag (`$2002` bit 5) at the -top of vblank — it reflects whether any scanline of the -just-finished frame had more than 8 sprites and fired the -hardware "give up" pathway. If the bit is set the handler -bumps a cumulative counter at `$07FD` and sets a per-frame -sticky bit at `$07FC`, which the next `wait_frame` clears. - -User code reads those bytes via the new builtins: - -```nescript -debug.assert(not debug.sprite_overflow()) -``` - -…or, in an overlay: - -```nescript -var ovf: u8 = 0 -ovf = debug.sprite_overflow_count() -draw Digit at: (8, 8) frame: ovf -``` - -The PPU hardware flag has well-known quirks (it occasionally -misses the 9th sprite or sets the flag when none actually -overflowed), but it's correct for the overwhelming majority -of cases and is essentially free to sample — one `LDA $2002; -AND #$20` at NMI top, ~15 cycles per frame. Release builds -never emit the check block, so the four bytes at `$07EF` / -`$07FC`-`$07FD` remain free for the analyzer to allocate. - -Combined, the three layers catch the sprite-per-scanline -limit at three different lifecycle stages: W0109 at compile -time for statically-knowable layouts, `debug.sprite_overflow*` -at playtest time for the dynamic cases W0109 can't see, and -`cycle_sprites` at runtime as a graceful fallback for the -cases the user knows are unavoidable. - ---- - -## 5. The `inline` keyword is a hint and is silently ignored for short functions *(FIXED)* - -### Symptom - -Marking a tiny function `inline fun` did not inline it. -The compiler still emitted a real `JSR` with full parameter -passing through `$04`-`$07`, which meant the declared-inline -helpers in War (`card_rank`, `card_suit`, `set_phase`) still -paid the calling-convention overhead and still fell foul of -the bug-2 clobbering until the param-spill prologue landed. - -### Reproduction - -```nescript -inline fun card_rank(card: u8) -> u8 { - return card >> 4 -} -``` - -Pre-fix, the asm dump showed `JSR __ir_fn_card_rank` at every -call site. Post-fix the body is spliced at each use and no -`JSR` is emitted at all. - -### Root cause - -The IR lowerer's old handling of `inline fun` was a no-op — -`is_inline` was read off the AST but the lowering path for -`Call` never branched on it. The optimizer passes also had -no inlining transform. So the keyword was parsed and then -dropped on the floor, producing regular out-of-line code. - -### Fix - -`src/ir/lowering.rs` now captures inline bodies up front in -`LoweringContext::capture_inline_bodies` and rewrites call -sites at lowering time. Two body shapes are supported: - -1. **Single-return expression** (e.g. `return card >> 4`) — - captured as `InlineBody::Expression(Expr)`. At the call - site, the lowerer evaluates each argument into a fresh - temp, pushes a substitution frame mapping parameter names - to those temps, and recursively lowers the expression in - place of a `Call` op. No IR `Call`/`Return` ops are - emitted; the caller ends up with the same IR it would - have had if the expression were written directly. - -2. **Void multi-statement body** — captured as - `InlineBody::Void(Vec)`, but only when every - statement passes `is_splicable_void_stmt` (plain - assignments, statement-level calls, draws, palette/ - background/scroll writes, `wait_frame`, inline asm, debug - builtins). Any control flow (`if`/`while`/`for`/`loop`/ - `return`/`break`/`continue`/`transition`) disqualifies - the function from being inlined, and the call stays a - regular `Call`. This mirrors War's `set_phase` (a - four-statement global assign) and `reset_flight` (a - similar pattern). - -Functions that are marked `inline` but have a body shape the -simple substitution machinery can't splice — notably ones -with conditional early returns like War's `wrap52` — fall -back to regular out-of-line calls with no diagnostic. That's -a deliberate trade-off: rather than refuse to compile the -program or emit a noisy warning, we degrade gracefully. The -`inline` keyword is now a best-effort hint whose "best -effort" is predictable and documented here. - -### Substitution stack - -Nested inline expansions push a fresh substitution frame so -an inline body calling another inline sees the inner -function's parameter substitutions, not its own. -`lookup_inline_sub` walks only the top of the stack because -inner bodies are lowered to completion before the stack is -popped, so an unambiguous "current" frame always exists. See -`LoweringContext::inline_subs_stack` and -`lower_expr::Expr::Ident` (which checks the substitution -stack before the global var table). - -### Regression tests - -Four tests in `src/ir/tests.rs`: - -- `inline_fun_expression_body_emits_no_call_at_use_site` — - a `return x * 2` inline emits no `Call`, just the multiply. -- `inline_fun_void_body_statements_are_spliced` — a void - three-statement inline compiles to three individual ops - at the caller, not a `Call`. -- `inline_fun_with_conditional_return_compiles_as_regular_call` - — a body with an `if ... return` pattern falls back to a - regular `Call` op. -- `inline_fun_nested_inlines_substitute_correctly` — inline A - calling inline B sees B's parameter substitutions, not A's. - ---- - -## 6. `wide_hi` IR-lowering map leaked between functions and corrupted 16-bit ops *(FIXED)* - -### Symptom - -A function whose body had no 16-bit values whatsoever would -nonetheless emit `CmpEq16` (and other `Op16` variants) where the -*destination* temp aliased one of the *source* temps. The -resulting comparison effectively became "is this byte equal to -some uninitialised stack memory?", which in War caused the -phase-machine `match phase { ... }` dispatcher to skip the -`P_WIN_B` arm forever once the game first reached it — the game -would freeze with both cards face-up and "PLAYER B WINS" never -firing. - -### Reproduction (pre-fix) - -A handful of `u16` `+= 1` operations early in a state handler -followed by a long `match` chain on a `u8` was enough to trip it. -The minimum repro is roughly: - -```nescript -var clock: u16 = 0 -var phase: u8 = 0 -on frame { - clock += 1 // wide op leaves wide_hi entries - match phase { // u8 match — should be 8-bit - 0 => { phase = 1 } - 1 => { phase = 2 } - 2 => { phase = 3 } - 3 => { phase = 4 } - 4 => { phase = 5 } - 5 => { phase = 6 } - 6 => { phase = 7 } - 7 => { /* corrupt — never matched */ } - _ => {} - } -} -``` - -The IR for the `phase == 7` arm came out as -`CmpEq16 { dest: T147, a_lo: T145, a_hi: T148, b_lo: T146, -b_hi: T147 }` — note `dest == b_hi`. The codegen happily emits -the corresponding 16-bit asm, but reads garbage for the `b_hi` -operand because it points at the same scratch slot the result -will be written to. - -### Root cause - -`src/ir/lowering.rs::IrLowerer` carries a `wide_hi: HashMap` -that records "this low temp's high byte lives at this other -temp" pairs whenever a 16-bit value is produced. `lower_function` -and `lower_handler` both reset `next_temp = 0` at the start of -each function — but they did *not* clear `wide_hi`. Stale entries -from earlier functions stuck around and matched against fresh -temp IDs in subsequent functions (which start counting from 0 -again), causing `is_wide(t)` and `widen(t)` to return spurious -"wide" results for what should have been narrow `u8` values. - -When that happens inside `lower_binop`'s `Eq` path, `widen(r)` -returns the stale `(r, hi_r)` pair where `hi_r` happens to be the -*next* temp ID `fresh_temp()` will hand out a moment later — so -the `dest` temp and `b_hi` end up identical. - -### Fix - -`src/ir/lowering.rs`: in both `lower_function` and `lower_handler`, -add `self.wide_hi.clear();` immediately after `self.next_temp = 0;`. -Done in this PR. - -### Why this didn't show up sooner - -Every prior example either declared no `u16` globals at all, or -declared one and used it sparingly enough that the temp IDs -the leaked entries claimed never collided with the rest of the -function. War is the first example that combines a `u16` -free-running counter with a deep state machine that does many -`u8` comparisons in the same `on frame` body, which is exactly -the shape the bug needs to manifest. - -### Regression test - -`src/ir/tests.rs::wide_hi_does_not_leak_between_functions` (added -in this PR) compiles a two-function program where function A -uses a `u16 += 1` (creating wide entries) and function B does -`u8 == const` comparisons in a match. Pre-fix, the IR would emit -`CmpEq16` with aliased dest/source; post-fix it emits the -expected 8-bit `CmpEq`. - ---- - -## Verification path after fixes - -Once any of the bugs above are fixed in the compiler, the -corresponding workarounds in `examples/war/*.ne` should be -reverted in the same PR so: - -- The example demonstrates idiomatic code, not workaround code. -- The PR's diff visibly proves the fix works end-to-end (the - workaround removal would otherwise be a silent regression). -- The committed `examples/war.nes` rebuilds byte-identically to - the reverted source, which the pre-commit hook enforces. - -The relevant workaround sites are catalogued in each bug's -"Workaround used" section above; grep for the prefix tags -(`dcf_`, `dfa_`, `pba_`, `dwp_`, …) to find them all. diff --git a/examples/war/PLAN.md b/examples/war/PLAN.md index 7bc0f97..dd141f7 100644 --- a/examples/war/PLAN.md +++ b/examples/war/PLAN.md @@ -216,10 +216,10 @@ parallel). - [x] Code review pass: read every file end-to-end, fix any mistakes found. -Two compiler bugs were discovered along the way and fixed in -the same PR (E0506 too-many-params + the wide_hi map leak in IR -lowering). Three more compiler limitations are documented in -`COMPILER_BUGS.md` with workarounds in the war source for now. +Seven compiler bugs were discovered along the way and all fixed +on this branch — see `git log` for the full list. Every +workaround that was originally in the war source has been +reverted now that the underlying bugs are fixed. --- @@ -229,16 +229,11 @@ A few things shifted from the approved plan: - **`arm_fly` is 4 params, not 6.** The 5th and 6th params (`fly_card`, `fly_face_up`) are written to globals at every - call site instead, because the v0.1 ABI silently drops params - past the 4th. The 4-param limit now produces a clean E0506 - diagnostic so future authors won't trip on it. -- **`card` parameter in `card_rank` / `card_suit` / `draw_card_face` - was renamed to per-function unique names** to dodge the IR - lowering's shared-VarId issue documented in - `COMPILER_BUGS.md` §1b. Same for `wrap52`'s `v` and the - `push_back_*` helpers. `x` and `y` are still shared because - they sit at the same parameter position in every helper that - takes them, so the routed slot mapping is consistent. + call site instead, because the v0.1 ABI only passes four + parameters via the zero-page transport slots. The 4-param + limit now produces a clean E0506 diagnostic so future authors + see the error up front instead of chasing silent + miscompiles. - **The `Playing` state's phase machine uses `match`, not a flat if-chain.** The if-chain shape allowed two phases to run in the same frame after a `set_phase` transition, which made diff --git a/examples/war/deck.ne b/examples/war/deck.ne index 7661a89..e950ec9 100644 --- a/examples/war/deck.ne +++ b/examples/war/deck.ne @@ -13,7 +13,12 @@ // 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. -inline fun wrap52(v: u8) -> u8 { +// +// Not marked `inline`: the conditional early return is one of +// the body shapes the inliner declines (W0110), and living +// with the JSR is cheaper than rewriting this as `v % DECK_SIZE` +// (which would pull in the software modulo routine). +fun wrap52(v: u8) -> u8 { if v >= DECK_SIZE { return v - DECK_SIZE } diff --git a/src/analyzer/mod.rs b/src/analyzer/mod.rs index 0205c63..0c46e9a 100644 --- a/src/analyzer/mod.rs +++ b/src/analyzer/mod.rs @@ -688,6 +688,15 @@ impl Analyzer { // coordinates are skipped because the static analysis // can't know where the sprite will land at runtime. self.check_sprite_scanline_budget(program); + + // Check every `inline fun` declaration against the + // IR lowerer's inline-eligibility rules. Functions + // whose body shape isn't splicable compile as regular + // out-of-line calls — which is correct, but silent: + // users who mark a helper `inline` to avoid call + // overhead might not realize the hint was declined. + // W0110 makes the fallback visible. + self.check_inline_declinability(program); } /// Qualify `name` under the current scope prefix. If no prefix @@ -1148,6 +1157,47 @@ impl Analyzer { } } + /// Walk every `inline fun` declaration and check whether the + /// IR lowerer will actually inline its body. Functions that + /// won't inline (conditional returns, loops, transitions, + /// empty void bodies, etc.) compile as regular out-of-line + /// calls — correct but silent. W0110 surfaces the fallback + /// at the declaration site with help text pointing at the + /// two body shapes the inliner does accept. + /// + /// Defers to [`crate::ir::lowering::can_inline_fun`] so the + /// two sides (warning + actual capture) can never drift. + fn check_inline_declinability(&mut self, program: &Program) { + for fun in &program.functions { + if !fun.is_inline { + continue; + } + if crate::ir::can_inline_fun(fun.return_type.as_ref(), &fun.body) { + continue; + } + self.diagnostics.push( + Diagnostic::warning( + ErrorCode::W0110, + format!( + "`inline fun {}` cannot be inlined; falling back to a regular call", + fun.name + ), + fun.span, + ) + .with_help( + "the inliner accepts two body shapes: a single `return ` (for \ + functions with a return type) or a sequence of plain statements with no \ + control flow (for void functions). Rewrite the body to fit one of those, \ + or remove the `inline` keyword if the JSR overhead is acceptable", + ) + .with_note( + "rejected body shapes include conditional early returns, if/while/for/loop \ + blocks, transitions, breaks, continues, and nested function definitions", + ), + ); + } + } + fn register_const(&mut self, c: &ConstDecl) { if self.symbols.contains_key(&c.name) { self.diagnostics.push(Diagnostic::error( diff --git a/src/analyzer/tests.rs b/src/analyzer/tests.rs index bea2d76..f615cd4 100644 --- a/src/analyzer/tests.rs +++ b/src/analyzer/tests.rs @@ -2103,10 +2103,11 @@ 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. + // Regression test for the function-local scope-qualification + // fix on the War bug-cleanup branch (see `git log`): function- + // body `var` declarations used to live in a flat global + // namespace, so two functions both declaring `var i` collided + // on E0501. They now coexist in per-function scopes. analyze_ok( r#" game "T" { mapper: NROM } @@ -2132,11 +2133,12 @@ fn analyze_allows_same_local_name_in_two_functions() { #[test] fn analyze_allows_same_param_name_in_two_functions() { - // Regression test for COMPILER_BUGS.md §1b: parameters + // Regression test for the scope-qualified parameter fix on + // the War bug-cleanup branch (see `git log`): 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. + // (analyzer) and lowering (IR) now give each function's + // parameters their own independent entries. analyze_ok( r#" game "T" { mapper: NROM } @@ -2477,3 +2479,145 @@ fn analyze_accepts_cycle_sprites_statement() { "#, ); } + +#[test] +fn analyze_inline_fun_with_conditional_return_trips_w0110() { + // A function marked `inline` whose body has an early + // conditional return can't be inlined by the simple + // substitution machinery — it compiles as a regular + // `JSR` call, and W0110 must fire at the declaration. + let (prog, diags) = parser::parse( + r#" + game "T" { mapper: NROM } + inline fun wrap(v: u8) -> u8 { + if v >= 52 { + return v - 52 + } + return v + } + var x: u8 = 0 + on frame { + x = wrap(60) + wait_frame + } + start Main + "#, + ); + assert!(diags.is_empty(), "parse errors: {diags:?}"); + let result = analyze(&prog.unwrap()); + let w0110: Vec<_> = result + .diagnostics + .iter() + .filter(|d| d.code == ErrorCode::W0110) + .collect(); + assert_eq!( + w0110.len(), + 1, + "expected exactly one W0110, got: {:?}", + result.diagnostics + ); + assert!( + w0110[0].message.contains("wrap"), + "W0110 should name the declined function, got: {}", + w0110[0].message + ); + assert!( + w0110[0].help.is_some() && w0110[0].note.is_some(), + "W0110 should carry both help and note text" + ); +} + +#[test] +fn analyze_inline_fun_with_single_return_expression_is_accepted() { + // A body that is exactly `return ` is the canonical + // inlinable shape — no W0110 should fire. + let (prog, diags) = parser::parse( + r#" + game "T" { mapper: NROM } + inline fun card_rank(card: u8) -> u8 { + return card >> 4 + } + var x: u8 = 0 + on frame { + x = card_rank(0x93) + wait_frame + } + start Main + "#, + ); + assert!(diags.is_empty(), "parse errors: {diags:?}"); + let result = analyze(&prog.unwrap()); + assert!( + !result + .diagnostics + .iter() + .any(|d| d.code == ErrorCode::W0110), + "did not expect W0110 for single-return inline, got: {:?}", + result.diagnostics + ); +} + +#[test] +fn analyze_inline_void_fun_is_accepted() { + // A void body with nothing but assigns is inlinable. + let (prog, diags) = parser::parse( + r#" + game "T" { mapper: NROM } + var a: u8 = 0 + var b: u8 = 0 + inline fun set_pair(x: u8, y: u8) { + a = x + b = y + } + on frame { + set_pair(5, 6) + wait_frame + } + start Main + "#, + ); + assert!(diags.is_empty(), "parse errors: {diags:?}"); + let result = analyze(&prog.unwrap()); + assert!( + !result + .diagnostics + .iter() + .any(|d| d.code == ErrorCode::W0110), + "did not expect W0110 for void inline, got: {:?}", + result.diagnostics + ); +} + +#[test] +fn analyze_inline_fun_with_loop_body_trips_w0110() { + // A `while` loop inside a marked-inline body is another + // disqualifying shape. + let (prog, diags) = parser::parse( + r#" + game "T" { mapper: NROM } + var sum: u8 = 0 + inline fun accumulate(n: u8) { + var i: u8 = 0 + while i < n { + sum += i + i += 1 + } + } + on frame { + accumulate(5) + wait_frame + } + start Main + "#, + ); + assert!(diags.is_empty(), "parse errors: {diags:?}"); + let result = analyze(&prog.unwrap()); + assert!( + result + .diagnostics + .iter() + .any(|d| d.code == ErrorCode::W0110), + "expected W0110 for loop-body inline, got: {:?}", + result.diagnostics + ); +} diff --git a/src/codegen/ir_codegen.rs b/src/codegen/ir_codegen.rs index 1238545..671a241 100644 --- a/src/codegen/ir_codegen.rs +++ b/src/codegen/ir_codegen.rs @@ -242,9 +242,11 @@ impl<'a> IrCodeGen<'a> { // 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. + // parameters (fixed on the War bug-cleanup branch; see + // `git log` for the original reproduction and root cause). + // 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 @@ -3906,7 +3908,8 @@ mod more_tests { #[test] fn gen_function_prologue_spills_params_to_local_ram() { - // Regression test for COMPILER_BUGS.md §2: without the + // Regression test for the param-slot clobbering bug fixed + // on the War bug-cleanup branch (see `git log`): 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 @@ -3969,7 +3972,7 @@ fn gen_function_prologue_spills_params_to_local_ram() { 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" + as the param-spill prologue — the param-clobbering fix is \ + not in effect" ); } diff --git a/src/errors/diagnostic.rs b/src/errors/diagnostic.rs index 6659081..11d3250 100644 --- a/src/errors/diagnostic.rs +++ b/src/errors/diagnostic.rs @@ -45,6 +45,7 @@ pub enum ErrorCode { W0107, // `fast` variable rarely accessed (wastes zero-page slot) W0108, // array elements past byte 255 unreachable via 8-bit X index W0109, // too many literal-coord sprite draws on one scanline (NES 8/scanline limit) + W0110, // `inline fun` declined — body shape not splicable, fell back to regular call } impl fmt::Display for ErrorCode { @@ -74,6 +75,7 @@ impl fmt::Display for ErrorCode { Self::W0107 => "W0107", Self::W0108 => "W0108", Self::W0109 => "W0109", + Self::W0110 => "W0110", }; write!(f, "{code}") } @@ -90,7 +92,8 @@ impl ErrorCode { | Self::W0106 | Self::W0107 | Self::W0108 - | Self::W0109 => Level::Warning, + | Self::W0109 + | Self::W0110 => Level::Warning, _ => Level::Error, } } diff --git a/src/ir/lowering.rs b/src/ir/lowering.rs index fff5624..289bdfb 100644 --- a/src/ir/lowering.rs +++ b/src/ir/lowering.rs @@ -43,7 +43,8 @@ struct LoweringContext { /// registered as substitutions for the parameter names, /// and the body is lowered into the caller's current block /// in place of a `Call` op. See `try_inline_call_expr` / - /// `try_inline_call_stmt` below and `COMPILER_BUGS.md` §5. + /// `try_inline_call_stmt` below; the feature was added on + /// the War bug-cleanup branch (see `git log`). inline_bodies: HashMap, /// Substitution stack for nested inline expansions. The top /// frame is the active substitution map — `Expr::Ident(name)` @@ -237,11 +238,18 @@ impl LoweringContext { if !fun.is_inline { continue; } + // Defer to the shared `can_inline_fun` helper so the + // analyzer's W0110 check and this capture pass agree + // on exactly which bodies are splicable — any drift + // between the two would either swallow real bugs + // (lower inlines a body the analyzer thinks it + // wouldn't) or emit false-positive warnings (analyzer + // warns on something the lowerer actually inlines). + if !can_inline_fun(fun.return_type.as_ref(), &fun.body) { + continue; + } // Single-return-expression shape. - if fun.return_type.is_some() - && fun.body.statements.len() == 1 - && matches!(fun.body.statements[0], Statement::Return(Some(_), _)) - { + if fun.return_type.is_some() && fun.body.statements.len() == 1 { if let Statement::Return(Some(expr), _) = &fun.body.statements[0] { self.inline_bodies.insert( fun.name.clone(), @@ -253,25 +261,16 @@ impl LoweringContext { continue; } } - // Void multi-statement shape: no return type, and - // every body statement must be a shape we know how - // to splice. Only assigns, statement-context calls, - // draws, scroll, set_palette, and load_background - // are accepted — anything with nested control flow - // is too complex to inline without a full CFG - // clone. - if fun.return_type.is_none() - && !fun.body.statements.is_empty() - && fun.body.statements.iter().all(is_splicable_void_stmt) - { - self.inline_bodies.insert( - fun.name.clone(), - CapturedInline { - params: fun.params.clone(), - body: InlineBody::Void(fun.body.statements.clone()), - }, - ); - } + // Void multi-statement shape: no return type, every + // body statement is splicable. The helper already + // checked both conditions so we just clone. + self.inline_bodies.insert( + fun.name.clone(), + CapturedInline { + params: fun.params.clone(), + body: InlineBody::Void(fun.body.statements.clone()), + }, + ); } } @@ -626,7 +625,9 @@ impl LoweringContext { // true and, worse, `widen(t)` returning a stale `hi` temp ID // that collides with a later `fresh_temp()` allocation — // producing 16-bit IR ops where the destination temp is - // *also* one of the source temps. See COMPILER_BUGS.md §6. + // *also* one of the source temps (the `wide_hi` leak bug + // fixed on the War cleanup branch; see `git log` for the + // full reproduction). self.wide_hi.clear(); self.current_blocks = Vec::new(); self.current_locals = Vec::new(); @@ -724,9 +725,9 @@ impl LoweringContext { 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 - // critical — without it, state-handler bodies pick up wide - // temp pairs left over from the previous function and emit + // commentary there for why this is critical — without it, + // state-handler bodies pick up wide temp pairs left over + // from the previous function and emit // catastrophically wrong 16-bit IR ops. self.wide_hi.clear(); self.current_blocks = Vec::new(); @@ -1837,6 +1838,40 @@ fn is_splicable_void_stmt(stmt: &Statement) -> bool { ) } +/// True if an `inline fun` with the given return type and body +/// matches one of the shapes [`LoweringContext::capture_inline_bodies`] +/// can splice into a caller. Two shapes are recognised: +/// +/// 1. **Single-return expression**: the function has a declared +/// return type and its body is exactly `{ return }`. +/// 2. **Void multi-statement**: the function has no return type +/// and every body statement passes [`is_splicable_void_stmt`]. +/// +/// Anything else (conditional early returns, loops, nested +/// control flow, multiple returns, an empty void body) falls back +/// to a regular `JSR` call at every site. The analyzer calls this +/// to emit `W0110` when a declared-inline function won't actually +/// be inlined, and the IR lowerer calls the same logic when it +/// decides which bodies to capture — keeping both sides in sync. +#[must_use] +pub fn can_inline_fun(return_type: Option<&NesType>, body: &Block) -> bool { + // Single-return expression shape. + if return_type.is_some() + && body.statements.len() == 1 + && matches!(body.statements[0], Statement::Return(Some(_), _)) + { + return true; + } + // Void multi-statement shape. + if return_type.is_none() + && !body.statements.is_empty() + && body.statements.iter().all(is_splicable_void_stmt) + { + return true; + } + false +} + fn type_size(t: &NesType) -> u16 { match t { NesType::U8 | NesType::I8 | NesType::Bool => 1, diff --git a/src/ir/mod.rs b/src/ir/mod.rs index cb18efc..2d2dedc 100644 --- a/src/ir/mod.rs +++ b/src/ir/mod.rs @@ -2,7 +2,7 @@ mod lowering; #[cfg(test)] mod tests; -pub use lowering::{lower, RAW_ASM_PREFIX}; +pub use lowering::{can_inline_fun, lower, RAW_ASM_PREFIX}; use crate::lexer::Span; use std::fmt; diff --git a/src/ir/tests.rs b/src/ir/tests.rs index 940236c..a69cfca 100644 --- a/src/ir/tests.rs +++ b/src/ir/tests.rs @@ -743,7 +743,8 @@ fn lower_modulo_emits_mod_op_not_load_imm_zero() { #[test] fn wide_hi_does_not_leak_between_functions() { - // Regression test for COMPILER_BUGS.md §6: the IR lowerer's + // Regression test for the `wide_hi` leak bug fixed on the + // War bug-cleanup branch (see `git log`): the IR lowerer's // `wide_hi` map used to persist across function boundaries // even though `next_temp` resets to 0 per function. A // function whose body had no u16 ops would inherit stale @@ -790,7 +791,7 @@ fn wide_hi_does_not_leak_between_functions() { { // The dest of a 16-bit compare must never alias one // of its operand high bytes — that's the symptom of - // bug #6 from war/COMPILER_BUGS.md. + // the `wide_hi` leak bug. if dest == b_hi || dest == a_hi { wide_eq_dest_aliases += 1; } @@ -804,7 +805,8 @@ fn wide_hi_does_not_leak_between_functions() { #[test] fn inline_fun_expression_body_emits_no_call_at_use_site() { - // Regression test for COMPILER_BUGS.md §5: `inline fun` + // Regression test for the real-inlining feature added on + // the War bug-cleanup branch (see `git log`): `inline fun` // with a single-return-expression body should be spliced // at every call site instead of emitting a Call op. The // lowered frame handler should contain zero Call ops