From e8d602c1bc6477b6ef21f3f7d9503f5212063741 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 15 Apr 2026 03:56:42 +0000 Subject: [PATCH] codereview: address six findings from a fresh review pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A focused review of the branch surfaced two correctness bugs and four important polish items in the new features. None of the existing example goldens shift — every fix is gated on conditions that don't fire in the committed examples. **debug.frame_overran() never reset between frames in implicit wait_frame programs.** The IR-level WaitFrame op cleared $07FE, but the implicit main-loop flag-clear that runs between dispatch iterations only cleared ZP_FRAME_FLAG. A program whose `on frame { ... }` body had no explicit `wait_frame` would latch $07FE to 1 on the first miss and never reset, breaking `debug.assert(not debug.frame_overran())` guards. The dispatch loop now also clears $07FE in debug builds, mirroring the WaitFrame path. New regression test asserts the main loop emits exactly one STA $07FE in a no-wait_frame debug build. **Metasprite base-tile resolution silently miscompiled for `@chr` / `@binary` sprites.** The IR lowering walks `program.sprites` to compute base tile indices but assumes 1 tile per non-Inline source, while the real asset resolver reads the file. The analyzer now hard-rejects the combination with a clear "use inline pixels" hint instead of letting it compile to a visual glitch. New analyzer test `analyze_metasprite_with_external_chr_sprite_errors` covers it. **next_sprite_tile capping silently allowed CHR overlap.** The pipeline used `.min(255)` which would let a background tile overwrite a sprite tile when the sprite range filled the pattern table. Now hard-errors via CompileError::AssetResolution when the sprite range >= 256 *and* the program declares any `@nametable(...)` background. Inline backgrounds aren't affected. **Linker silently truncated background CHR overflow.** The `if end <= chr.len()` guard at the CHR copy site dropped any auto-CHR bytes that would have run past the pattern table. Replaced with a debug assertion since the resolver should have caught it upstream — defense in depth. **Stale comment in nested_structs.ne** said struct literals "don't accept array fields yet" while the example itself demonstrates inline array fields working through `expand_struct_literal_init`. Comment updated. **Misleading sentinel comment in audio.rs** described the pitch envelope's trailing zero as a runtime sentinel; in practice the volume tick `JMP`s to `__audio_sfx_done` first and the pitch update block never reads the trailing byte. Rewrote the comment to clarify it's padding for predictable blob length. Also tidies up two minor items the reviewer flagged: - `flatten_struct_fields` rebuilt the `struct_sizes` HashMap on every leaf field; hoisted the snapshot to the function entry. - Integration tests called `resolve_backgrounds(..., 0)` (the new `next_sprite_tile` parameter); changed to `1` so a future PNG-nametable test fixture won't accidentally overwrite the runtime smiley at tile 0. https://claude.ai/code/session_01KEczoNUX3WmcFLfq6iAQxB --- examples/nested_structs.ne | 8 ++++-- src/analyzer/mod.rs | 58 ++++++++++++++++++++++++++++++++++---- src/analyzer/tests.rs | 29 +++++++++++++++++++ src/assets/audio.rs | 12 +++++--- src/assets/resolve.rs | 6 ++-- src/codegen/ir_codegen.rs | 42 ++++++++++++++++++++++++++- src/ir/tests.rs | 18 ++++++++++-- src/linker/mod.rs | 13 +++++++-- src/linker/tests.rs | 2 +- src/pipeline.rs | 22 +++++++++++++-- src/runtime/mod.rs | 9 ++++-- tests/integration_test.rs | 10 +++---- 12 files changed, 195 insertions(+), 34 deletions(-) diff --git a/examples/nested_structs.ne b/examples/nested_structs.ne index dc27b6b..57604c0 100644 --- a/examples/nested_structs.ne +++ b/examples/nested_structs.ne @@ -28,9 +28,11 @@ struct Hero { inv: u8[4], } -// Hero positions are initialized inline so the on-frame handler -// only has to step them. Inventory bytes are a separate `var` -// because struct literals don't accept array fields yet. +// Hero positions are initialized inline. Both the nested +// `Vec2 { ... }` and the inline `inv: [1, 2, 3, 4]` array +// initializers are unpacked into per-leaf-field IR globals by +// `expand_struct_literal_init` in src/ir/lowering.rs, so each +// leaf gets its own `LDA #imm; STA addr` pair at reset. var hero1: Hero = Hero { pos: Vec2 { x: 32, y: 96 }, hp: 100, inv: [1, 2, 3, 4] } var hero2: Hero = Hero { pos: Vec2 { x: 200, y: 128 }, hp: 100, inv: [10, 20, 30, 40] } diff --git a/src/analyzer/mod.rs b/src/analyzer/mod.rs index eae8acf..8192130 100644 --- a/src/analyzer/mod.rs +++ b/src/analyzer/mod.rs @@ -401,6 +401,19 @@ impl Analyzer { // must exist, and the metasprite name must be unique // (against other metasprites and against sprites — both // share the same `draw` lookup namespace). + // + // We also enforce a more restrictive rule for the v1 + // metasprite lowering: every sprite that PRECEDES a + // metasprite-targeted sprite in declaration order must use + // an inline `pixels:` body. The IR lowering at + // `ir/lowering.rs::lower_program` walks the sprite list to + // compute base tile indices for the metasprite's `frame:` + // resolution, but it can't read external `@chr(...)` / + // `@binary(...)` files at lowering time and falls back to + // a single-tile assumption — that would silently misalign + // the metasprite's frame indices. Reject those programs at + // analysis time so users get a clear error instead of a + // visual glitch at runtime. let mut seen_metasprites = HashSet::new(); let sprite_names: HashSet = program.sprites.iter().map(|s| s.name.clone()).collect(); @@ -422,7 +435,36 @@ impl Analyzer { ms.span, )); } - if !sprite_names.contains(&ms.sprite_name) { + if sprite_names.contains(&ms.sprite_name) { + // Check that every sprite up to *and including* + // the target uses an inline pixels source. Any + // earlier non-inline sprite would shift the base + // tile of the target by an unknown amount; the + // target itself also has to be inline so the + // lowering knows how many tiles to consume for it. + for sprite in &program.sprites { + let is_inline = matches!( + sprite.chr_source, + crate::parser::ast::AssetSource::Inline(_) + ); + if !is_inline { + self.diagnostics.push(Diagnostic::error( + ErrorCode::E0201, + format!( + "metasprite '{}' depends on sprite '{}' which uses an external `@chr` or `@binary` source; \ + the v1 metasprite lowering can't compute base tile indices for non-inline sprites — \ + inline the pixel art with a `pixels:` block, or remove the metasprite", + ms.name, sprite.name + ), + ms.span, + )); + break; + } + if sprite.name == ms.sprite_name { + break; + } + } + } else { self.diagnostics.push(Diagnostic::error( ErrorCode::E0201, format!( @@ -1038,6 +1080,15 @@ impl Analyzer { layout: &StructLayout, var_span: Span, ) { + // Snapshot the per-struct sizes once at the top of the + // recursion so deep struct trees don't rebuild the same + // map at every leaf — `type_size_with` is the only + // consumer and it just needs the size lookup. + let struct_sizes: HashMap = self + .struct_layouts + .iter() + .map(|(n, l)| (n.clone(), l.size)) + .collect(); for (field_name, field_type, offset) in &layout.fields { let full_name = format!("{base_name}.{field_name}"); let field_addr = base_addr + offset; @@ -1071,11 +1122,6 @@ impl Analyzer { span: var_span, }, ); - let struct_sizes: HashMap = self - .struct_layouts - .iter() - .map(|(n, l)| (n.clone(), l.size)) - .collect(); let field_size = type_size_with(field_type, &struct_sizes); self.var_allocations.push(VarAllocation { name: full_name, diff --git a/src/analyzer/tests.rs b/src/analyzer/tests.rs index 44e8d03..a89233b 100644 --- a/src/analyzer/tests.rs +++ b/src/analyzer/tests.rs @@ -544,6 +544,35 @@ fn analyze_metasprite_unknown_sprite_errors() { ); } +#[test] +fn analyze_metasprite_with_external_chr_sprite_errors() { + // The IR lowering walks `program.sprites` to compute base + // tile indices for the metasprite's `frame:` array, but it + // can't read external `@chr(...)` files at lowering time + // and would fall back to a 1-tile assumption. That would + // silently misalign the metasprite, so the analyzer rejects + // the combination upfront with a clear "use inline pixels" + // hint. + let errors = analyze_errors( + r#" + game "T" { mapper: NROM } + sprite Tileset @chr("art/sheet.png") + metasprite Hero { + sprite: Tileset + dx: [0, 8] + dy: [0, 0] + frame: [0, 1] + } + on frame { wait_frame } + start Main + "#, + ); + assert!( + errors.contains(&ErrorCode::E0201), + "metasprite over an external-CHR sprite should emit E0201, got: {errors:?}" + ); +} + #[test] fn analyze_metasprite_mismatched_array_lengths_errors() { let errors = analyze_errors( diff --git a/src/assets/audio.rs b/src/assets/audio.rs index e4f6824..8cd1381 100644 --- a/src/assets/audio.rs +++ b/src/assets/audio.rs @@ -376,10 +376,14 @@ fn compile_pulse_sfx(decl: &SfxDecl) -> SfxData { // it again" behaviour and emits no pitch blob, so existing // sfx ROMs are byte-identical. The pitch envelope is padded // (or truncated) to match the volume envelope length so the - // runtime can walk both pointers in lockstep without a - // separate length check; the trailing zero sentinel is added - // last so the pitch blob's last byte aligns with the volume - // sentinel and the runtime stops both walks at the same NMI. + // runtime can walk both pointers in lockstep. The trailing + // zero byte is *padding*, not a sentinel the runtime ever + // reads — the volume tick `JMP`s to `__audio_sfx_done` on + // its own zero sentinel before the pitch update block runs, + // so the pitch pointer never advances past the user's last + // byte. Keeping the trailing zero costs one ROM byte but + // makes the blob length predictable for the linker symbol + // dump and matches the `volume` envelope's shape. let pitch_envelope = build_pulse_pitch_envelope(&decl.pitch, decl.volume.len()); SfxData { name: decl.name.clone(), diff --git a/src/assets/resolve.rs b/src/assets/resolve.rs index b3eca45..67fc834 100644 --- a/src/assets/resolve.rs +++ b/src/assets/resolve.rs @@ -436,7 +436,7 @@ mod tests { png_source: None, span: Span::dummy(), }); - let resolved = resolve_backgrounds(&program, Path::new("."), 0).unwrap(); + let resolved = resolve_backgrounds(&program, Path::new("."), 1).unwrap(); assert_eq!(resolved.len(), 1); assert_eq!(resolved[0].name, "Stage"); assert_eq!(resolved[0].tiles.len(), 960); @@ -478,7 +478,7 @@ mod tests { png_source: Some(png_path.file_name().unwrap().to_string_lossy().to_string()), span: Span::dummy(), }); - let resolved = resolve_backgrounds(&program, &dir, 0).unwrap(); + let resolved = resolve_backgrounds(&program, &dir, 1).unwrap(); let _ = std::fs::remove_file(&png_path); assert_eq!(resolved.len(), 1); assert_eq!(resolved[0].tiles.len(), 960); @@ -515,7 +515,7 @@ mod tests { png_source: Some(png_path.file_name().unwrap().to_string_lossy().to_string()), span: Span::dummy(), }); - let err = resolve_backgrounds(&program, &dir, 0).unwrap_err(); + let err = resolve_backgrounds(&program, &dir, 1).unwrap_err(); let _ = std::fs::remove_file(&png_path); assert!( err.contains("background 'Oops' PNG source"), diff --git a/src/codegen/ir_codegen.rs b/src/codegen/ir_codegen.rs index a09e226..51d4950 100644 --- a/src/codegen/ir_codegen.rs +++ b/src/codegen/ir_codegen.rs @@ -687,9 +687,19 @@ impl<'a> IrCodeGen<'a> { self.emit_label(&wait_label); self.emit(LDA, AM::ZeroPage(ZP_FRAME_FLAG)); self.emit(BEQ, AM::LabelRelative(wait_label)); - // Clear the flag + // Clear the flag. self.emit(LDA, AM::Immediate(0)); self.emit(STA, AM::ZeroPage(ZP_FRAME_FLAG)); + // In debug mode, also clear the per-frame "did this frame + // overrun" sticky bit at $07FE so user code sees a fresh + // value next NMI even when the program has no explicit + // `wait_frame` inside its handler. The IR-level WaitFrame + // op clears it too, so explicit-wait programs already get + // this for free; mirroring it here makes the implicit + // main-loop path consistent. + if self.debug_mode { + self.emit(STA, AM::Absolute(0x07FE)); + } // Dispatch on current_state using CMP + BNE + JMP trampoline self.emit(LDA, AM::ZeroPage(ZP_CURRENT_STATE)); @@ -2747,6 +2757,36 @@ mod more_tests { ); } + #[test] + fn ir_codegen_main_loop_clears_overrun_flag_in_debug_mode() { + // The implicit main-loop wait at the top of the dispatch + // loop is the only thing that clears the frame-ready flag + // for programs whose `on frame { ... }` body has no + // explicit `wait_frame`. Without also clearing $07FE here + // the per-frame overrun sticky bit would latch to 1 on + // the first miss and never reset, breaking + // `debug.assert(not debug.frame_overran())` guards. We + // verify by counting STA $07FE writes in a debug build + // of a program with NO explicit wait_frame — it should + // appear exactly once (the main loop), not zero. + let insts = lower_and_gen_debug( + r#" + game "T" { mapper: NROM } + var x: u8 = 0 + on frame { x = 1 } + start Main + "#, + ); + let clear_count = insts + .iter() + .filter(|i| i.opcode == STA && i.mode == AM::Absolute(0x07FE)) + .count(); + assert_eq!( + clear_count, 1, + "main loop should emit exactly one STA $07FE in debug mode (no explicit wait_frame)" + ); + } + #[test] fn ir_codegen_wait_frame_release_does_not_touch_overrun_flag() { let insts = lower_and_gen( diff --git a/src/ir/tests.rs b/src/ir/tests.rs index 311c9c9..402e6a1 100644 --- a/src/ir/tests.rs +++ b/src/ir/tests.rs @@ -391,10 +391,9 @@ fn lower_metasprite_draw_expands_to_one_op_per_tile() { .iter() .find(|f| f.name.contains("frame")) .unwrap(); - let draws: Vec<_> = frame_fn - .blocks + let ops: Vec<&IrOp> = frame_fn.blocks.iter().flat_map(|b| &b.ops).collect(); + let draws: Vec<_> = ops .iter() - .flat_map(|b| &b.ops) .filter_map(|op| match op { IrOp::DrawSprite { sprite_name, .. } => Some(sprite_name.clone()), _ => None, @@ -411,6 +410,19 @@ fn lower_metasprite_draw_expands_to_one_op_per_tile() { "expanded ops should target the underlying sprite" ); } + // Each tile in the metasprite uses `frame: 0` relative to + // the underlying sprite. The single-sprite program has `Tile` + // at base index 1 (smiley occupies 0), so the resolved + // absolute frame index for every expanded DrawSprite is 1 — + // we should see at least one `LoadImm(_, 1)` matching that. + let load_imm_1_count = ops + .iter() + .filter(|op| matches!(op, IrOp::LoadImm(_, 1))) + .count(); + assert!( + load_imm_1_count >= 4, + "metasprite expansion should LoadImm(_, 1) at least once per tile (sprite Tile sits at base index 1, frame: [0,0,0,0]); got {load_imm_1_count}" + ); } #[test] diff --git a/src/linker/mod.rs b/src/linker/mod.rs index 0c78c04..7356ce4 100644 --- a/src/linker/mod.rs +++ b/src/linker/mod.rs @@ -702,9 +702,16 @@ impl Linker { } let offset = bg.chr_base_tile as usize * 16; let end = offset + bg.chr_bytes.len(); - if end <= chr.len() { - chr[offset..end].copy_from_slice(&bg.chr_bytes); - } + assert!( + end <= chr.len(), + "background '{}' auto-CHR ({} bytes at base tile {}) overflows the {}-byte CHR ROM; \ + the resolver should have caught this — file a bug", + bg.name, + bg.chr_bytes.len(), + bg.chr_base_tile, + chr.len() + ); + chr[offset..end].copy_from_slice(&bg.chr_bytes); } builder.set_chr(chr); diff --git a/src/linker/tests.rs b/src/linker/tests.rs index 503034e..6b28c10 100644 --- a/src/linker/tests.rs +++ b/src/linker/tests.rs @@ -141,7 +141,7 @@ fn link_with_background_chr_places_blob_after_sprites() { let linker = Linker::new(Mirroring::Horizontal); let user_code = vec![Instruction::implied(NOP)]; let sprite_bytes: Vec = vec![0xAA; 16]; // tile 1 - let bg_chr: Vec = (0..32).map(|i| i as u8).collect(); // tiles 5, 6 + let bg_chr: Vec = (0u8..32u8).collect(); // tiles 5, 6 let sprites = vec![SpriteData { name: "Player".into(), tile_index: 1, diff --git a/src/pipeline.rs b/src/pipeline.rs index 71cb11e..39fb520 100644 --- a/src/pipeline.rs +++ b/src/pipeline.rs @@ -164,14 +164,32 @@ pub fn compile_source( // tile is whatever sits past the last sprite. We derive it // from the resolved `SpriteData` rather than re-walking the // AST to keep the two sides honest. - let next_sprite_tile = sprites + // + // Hard-error if the sprite range already fills the 256-tile + // pattern table. A silent cap would let a background tile + // overwrite the last sprite tile — the kind of latent + // miscompile we'd rather catch at link time than at runtime. + // The check is lifted out of `resolve_backgrounds` so the + // diagnostic mentions the sprite count, not just the + // background that happened to trip the limit. + let next_sprite_tile_u16 = sprites .iter() .map(|s| { let count = s.chr_bytes.len().div_ceil(16) as u16; u16::from(s.tile_index) + count }) .max() - .map_or(1u8, |max| max.min(255) as u8); + .unwrap_or(1u16); + let has_png_background = program.backgrounds.iter().any(|b| b.png_source.is_some()); + if has_png_background && next_sprite_tile_u16 >= 256 { + return Err(CompileError::AssetResolution(format!( + "sprite tile range ends at index {next_sprite_tile_u16} which leaves no room for \ + background tiles in the 256-tile pattern table; remove or shrink a sprite, or \ + use an inline background body instead of `@nametable(...)`" + ))); + } + #[allow(clippy::cast_possible_truncation)] + let next_sprite_tile: u8 = next_sprite_tile_u16.min(255) as u8; let backgrounds = assets::resolve_backgrounds(&program, source_dir, next_sprite_tile) .map_err(|e| CompileError::AssetResolution(format!("backgrounds: {e}")))?; diff --git a/src/runtime/mod.rs b/src/runtime/mod.rs index e4dbe94..19b760b 100644 --- a/src/runtime/mod.rs +++ b/src/runtime/mod.rs @@ -99,10 +99,13 @@ pub const DEBUG_FRAME_OVERRUN_ADDR: u16 = 0x07FF; /// Debug-mode "did the previous frame overrun" sticky bit. Set /// to 1 by the NMI handler at the same time as it bumps -/// [`DEBUG_FRAME_OVERRUN_ADDR`], and cleared to 0 by `wait_frame` -/// once the main loop catches up. Exposed to user code as +/// [`DEBUG_FRAME_OVERRUN_ADDR`], and cleared to 0 by either an +/// explicit `wait_frame` IR op *or* the implicit main-loop +/// flag-clear that runs between every dispatch — so a program +/// whose `on frame { ... }` body has no explicit `wait_frame` +/// still sees a fresh value next NMI. Exposed to user code as /// `debug.frame_overran()` — a per-frame "did this frame finish -/// in time" predicate suited for `debug.assert(!debug.frame_overran())` +/// in time" predicate suited for `debug.assert(not debug.frame_overran())` /// guards. Lives one byte below the cumulative counter so the /// two can be inspected together in a Mesen memory viewer. pub const DEBUG_FRAME_OVERRUN_FLAG_ADDR: u16 = 0x07FE; diff --git a/tests/integration_test.rs b/tests/integration_test.rs index d5e83c7..c8bd8ac 100644 --- a/tests/integration_test.rs +++ b/tests/integration_test.rs @@ -1154,7 +1154,7 @@ fn compile_banked(source: &str) -> Vec { let music = assets::resolve_music(&program).expect("music resolution should succeed"); let palettes = assets::resolve_palettes(&program, Path::new(".")) .expect("palette resolution should succeed"); - let backgrounds = assets::resolve_backgrounds(&program, Path::new("."), 0) + let backgrounds = assets::resolve_backgrounds(&program, Path::new("."), 1) .expect("background resolution should succeed"); let mut codegen = IrCodeGen::new(&analysis.var_allocations, &ir_program) @@ -1932,7 +1932,7 @@ fn e2e_png_palette_source_compiles_and_splices_bytes_into_prg() { // relative PNG path lands on the fixture we just wrote. let palettes = assets::resolve_palettes(&program, &dir).expect("palette resolution should succeed"); - let backgrounds = assets::resolve_backgrounds(&program, &dir, 0).expect("bg ok"); + let backgrounds = assets::resolve_backgrounds(&program, &dir, 1).expect("bg ok"); assert_eq!(palettes.len(), 1); assert_eq!(palettes[0].name, "Main"); // First two bytes should map via `nearest_nes_color` to black @@ -2018,7 +2018,7 @@ fn compile_banked_with_opts(source: &str, optimize: bool) -> Vec { let music = assets::resolve_music(&program).expect("music resolution should succeed"); let palettes = assets::resolve_palettes(&program, Path::new(".")) .expect("palette resolution should succeed"); - let backgrounds = assets::resolve_backgrounds(&program, Path::new("."), 0) + let backgrounds = assets::resolve_backgrounds(&program, Path::new("."), 1) .expect("background resolution should succeed"); let mut codegen = IrCodeGen::new(&analysis.var_allocations, &ir_program) @@ -2261,7 +2261,7 @@ fn source_map_survives_aggressive_peephole_folding() { let sfx = assets::resolve_sfx(&program).unwrap(); let music = assets::resolve_music(&program).unwrap(); let palettes = assets::resolve_palettes(&program, Path::new(".")).unwrap(); - let backgrounds = assets::resolve_backgrounds(&program, Path::new("."), 0).unwrap(); + let backgrounds = assets::resolve_backgrounds(&program, Path::new("."), 1).unwrap(); let mut codegen = IrCodeGen::new(&analysis.var_allocations, &ir_program) .with_sprites(&sprites) @@ -2402,7 +2402,7 @@ fn debug_build_emits_bounds_check_halt_routine() { let music = assets::resolve_music(&program).unwrap(); let palettes = assets::resolve_palettes(&program, Path::new(".")) .expect("palette resolution should succeed"); - let backgrounds = assets::resolve_backgrounds(&program, Path::new("."), 0) + let backgrounds = assets::resolve_backgrounds(&program, Path::new("."), 1) .expect("background resolution should succeed"); let mut cg_debug = IrCodeGen::new(&analysis.var_allocations, &ir_program)