From d2cfce595b337bf1a1e8d0f91439e576d9c6ff51 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 14 Apr 2026 01:35:08 +0000 Subject: [PATCH] analyzer: add four new warning diagnostics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extends the analyzer with the warnings listed under "Error message polish" in docs/future-work.md: - W0102 (existing): now also fires for `while true { ... }` and `loop { if cond { continue } }`. `continue` is explicitly not counted as an exit — the loop still spins forever. - W0105 (new): palette declarations whose sub-palette first-bytes disagree. The NES mirrors $3F10/$3F14/$3F18/$3F1C onto $3F00/ $3F04/$3F08/$3F0C, so a 32-byte sequential write silently overwrites the background universal colour; the grouped `universal:` form auto-fixes this so only flat `colors: [...]` declarations can trip it. - W0106 (new): a call at statement position whose callee has a declared return type — the value is silently dropped. - W0107 (new): `fast` variables with fewer than three observed reads+writes, which don't justify holding a scarce zero-page slot. Leading-underscore names are exempt. All four are warnings only — no IR, codegen, or runtime changes, so every committed example ROM rebuilds byte-identical and no emulator goldens flip. Tests added in src/analyzer/tests.rs. One legitimate W0106 surfaces on examples/platformer.ne (the `resolve_enemy_hit` helper uses early-return for control flow but its return value is discarded by the caller); fixing it would shift the ROM and flip goldens, so it is left in place as an informational hint rather than a hard fix. https://claude.ai/code/session_01MaNVcDmK9gsspRkdxowQAM --- src/analyzer/mod.rs | 188 ++++++++++++++++++++- src/analyzer/tests.rs | 356 +++++++++++++++++++++++++++++++++++++++ src/errors/diagnostic.rs | 14 +- 3 files changed, 556 insertions(+), 2 deletions(-) diff --git a/src/analyzer/mod.rs b/src/analyzer/mod.rs index 427bef4..4328c48 100644 --- a/src/analyzer/mod.rs +++ b/src/analyzer/mod.rs @@ -45,6 +45,14 @@ const ZP_USER_CAP: u8 = 0x80; /// RAM at `$0000-$07FF`; the allocator uses up through `$07FF`. const RAM_END: u16 = 0x0800; +/// W0107 threshold: a `fast` variable with fewer than this many +/// observed accesses is flagged as wasting a zero-page slot. Writes +/// and reads both count; the initializer of a `VarDecl` counts as a +/// write. Three is a reasonable cutoff — anything below that barely +/// justifies holding a scarce slot that codegen could use for a +/// hotter variable. +const W0107_MIN_ACCESSES: u32 = 3; + /// Analyze a parsed program for semantic errors. pub fn analyze(program: &Program) -> AnalysisResult { // Pre-collect declared and builtin-matchable sfx/music names so @@ -87,7 +95,9 @@ pub fn analyze(program: &Program) -> AnalysisResult { stack_depth_limit: DEFAULT_STACK_DEPTH, in_loop: false, used_vars: HashSet::new(), + var_access_counts: HashMap::new(), function_signatures: HashMap::new(), + function_return_types: HashMap::new(), current_return_type: None, in_function_body: false, struct_layouts: HashMap::new(), @@ -129,9 +139,19 @@ struct Analyzer { /// Names of variables that have been read somewhere in the program. /// Used for the W0103 unused-variable warning. used_vars: HashSet, + /// Count of observed references (reads + writes) for each + /// variable. Used for the W0107 fast-variable-underuse warning + /// to detect `fast var` declarations that hog a zero-page slot + /// without justifying it. + var_access_counts: HashMap, /// Function name to parameter types (in order). Used to validate /// call arity and argument types. function_signatures: HashMap>, + /// Function name to declared return type. `None` here means the + /// function has no declared return type (i.e. "void"); functions + /// with a declared return type appear with `Some(ty)`. Used by + /// W0106 to detect implicit-drop of a return value. + function_return_types: HashMap>, /// Return type of the function currently being analyzed, or None /// when the function has no declared return type. Only meaningful /// when `in_function_body` is true. @@ -214,6 +234,49 @@ impl Analyzer { )); } } + // W0105: the NES mirrors $3F10/$3F14/$3F18/$3F1C onto + // $3F00/$3F04/$3F08/$3F0C, so the "first byte" of every + // sub-palette at indices 0, 4, 8, 12, 16, 20, 24, 28 is + // really a single shared universal background colour. + // Writing a 32-byte blob sequentially with inconsistent + // values at those offsets silently overwrites $3F00 with + // whatever the last sprite sub-palette's first byte is — + // a classic "my screen is suddenly black" bug. The + // grouped-form palette parser auto-fixes this via its + // `universal:` field, so only the flat-form `colors:` + // path can trip this warning in practice. + let universals: Vec = [0, 4, 8, 12, 16, 20, 24, 28] + .into_iter() + .filter_map(|i| palette.colors.get(i).copied()) + .collect(); + if universals.len() >= 2 { + let first = universals[0]; + if universals.iter().any(|&b| b != first) { + self.diagnostics.push( + Diagnostic::warning( + ErrorCode::W0105, + format!( + "palette '{}' has inconsistent universal-background bytes across \ + its sub-palettes", + palette.name, + ), + palette.span, + ) + .with_note( + "the NES PPU mirrors $3F10/$3F14/$3F18/$3F1C onto \ + $3F00/$3F04/$3F08/$3F0C, so a 32-byte sequential \ + palette write overwrites the background universal \ + colour with sub-palette sp3's first byte", + ) + .with_help( + "set every sub-palette's first byte (indices 0, 4, 8, 12, \ + 16, 20, 24, 28) to the same universal colour, or switch \ + to the grouped form (`universal:` + `bg0..sp3`) which \ + fixes the mirror automatically", + ), + ); + } + } } let mut seen_backgrounds = HashSet::new(); for bg in &program.backgrounds { @@ -359,13 +422,78 @@ impl Analyzer { } } + // Check for under-used `fast` variables (W0107). A `fast` + // declaration reserves one of the NES's scarce zero-page + // slots, so it should see frequent traffic; below the + // threshold the author is spending a precious slot on a + // cold variable. Globals and state-locals both count. + for var in &program.globals { + self.check_fast_var_usage(var); + } + for state in &program.states { + for var in &state.locals { + self.check_fast_var_usage(var); + } + } + // Check for unreachable states (W0104). self.check_unreachable_states(program); } /// Mark a variable name as having been read somewhere in the program. + /// Also bumps the W0107 access counter. fn mark_var_used(&mut self, name: &str) { self.used_vars.insert(name.to_string()); + self.bump_var_access(name); + } + + /// Increment the observed access count for `name`. Called for + /// both reads (via [`Analyzer::mark_var_used`]) and writes (at + /// `Statement::Assign` handling). Used for W0107. + fn bump_var_access(&mut self, name: &str) { + *self.var_access_counts.entry(name.to_string()).or_insert(0) += 1; + } + + /// Emit W0107 if `var` is declared `fast` but sees fewer than + /// [`W0107_MIN_ACCESSES`] observed reads + writes across the + /// whole program. Variables that are already unused (W0103 + /// fires) are skipped to avoid double-reporting; leading-`_` + /// names are also exempt by the same convention that W0103 uses. + /// Array-typed `fast` variables are skipped because they never + /// end up in zero page anyway (allocation kicks them to main + /// RAM), so the slot argument doesn't apply. + fn check_fast_var_usage(&mut self, var: &VarDecl) { + if var.placement != Placement::Fast { + return; + } + if var.name.starts_with('_') { + return; + } + if !self.used_vars.contains(&var.name) { + return; + } + if matches!(var.var_type, NesType::Array(_, _)) { + return; + } + let count = self.var_access_counts.get(&var.name).copied().unwrap_or(0); + if count < W0107_MIN_ACCESSES { + self.diagnostics.push( + Diagnostic::warning( + ErrorCode::W0107, + format!( + "`fast` variable '{}' is accessed {count} time{plural}; \ + it wastes a zero-page slot", + var.name, + plural = if count == 1 { "" } else { "s" }, + ), + var.span, + ) + .with_help( + "drop the `fast` qualifier so the variable lives in main \ + RAM and the zero-page slot can go to a hotter variable", + ), + ); + } } /// Emit W0103 if `var` is never read anywhere. Variables named @@ -789,6 +917,8 @@ impl Analyzer { let param_types: Vec = fun.params.iter().map(|p| p.param_type.clone()).collect(); self.function_signatures .insert(fun.name.clone(), param_types); + self.function_return_types + .insert(fun.name.clone(), fun.return_type.clone()); } /// Attempt to allocate `size` bytes of RAM for a variable declared @@ -925,6 +1055,11 @@ impl Analyzer { if let Some(init) = &var.init { self.walk_expr_reads(init); self.check_expr_type(init, &var.var_type); + // The initializer is a write to the variable; + // count it toward W0107's access tally without + // marking the variable "read" (W0103 still wants + // to fire on a declared-but-unread var). + self.bump_var_access(&var.name); } } Statement::Assign(lvalue, _, expr, span) => { @@ -939,6 +1074,13 @@ impl Analyzer { *span, )); } + // A plain scalar write doesn't count as a + // read for W0103 (an unused variable might + // only be written to, never read), but it + // is still an access of the variable's + // storage for W0107's "is this `fast` + // slot worth it?" check. + self.bump_var_access(name); } else { // Assigning to an undeclared name is an // error — the lowering would otherwise @@ -1000,13 +1142,29 @@ impl Analyzer { self.check_block(block, state_names); } } - Statement::While(cond, body, _) => { + Statement::While(cond, body, span) => { self.walk_expr_reads(cond); self.check_expr_type(cond, &NesType::Bool); let was_in_loop = self.in_loop; self.in_loop = true; self.check_block(body, state_names); self.in_loop = was_in_loop; + // W0102: a `while true { ... }` (or the rarely-written + // `while 1 { ... }`) that never breaks, returns, + // transitions, or waits for a frame is an infinite + // spin — same hazard as the bare `loop { ... }` below. + if is_always_true(cond) && !block_can_exit_or_yield(body) { + self.diagnostics.push( + Diagnostic::warning( + ErrorCode::W0102, + "infinite loop with no break, return, transition, or wait_frame", + *span, + ) + .with_help( + "add `wait_frame`, `break`, `return`, or `transition` somewhere in the body", + ), + ); + } } Statement::For { var, @@ -1109,6 +1267,23 @@ impl Analyzer { self.check_intrinsic_args(name, args, *span); } else if self.symbols.contains_key(name) { self.check_call_signature(name, args, *span); + // W0106: a call at statement position whose + // callee has a declared return type silently + // drops the value. Flag it so the author at + // least acknowledges the discard. + if matches!(self.function_return_types.get(name), Some(Some(_))) { + self.diagnostics.push( + Diagnostic::warning( + ErrorCode::W0106, + format!("return value of '{name}' is discarded"), + *span, + ) + .with_help( + "bind the result to a variable (e.g. `var _result: u8 = f()`), \ + or remove the return type from the function if the value isn't useful", + ), + ); + } } else { self.diagnostics.push(Diagnostic::error( ErrorCode::E0503, @@ -1561,6 +1736,17 @@ fn block_can_exit_or_yield(block: &Block) -> bool { block.statements.iter().any(stmt_can_exit_or_yield) } +/// True if `expr` is an always-true constant condition — i.e. the +/// literal `true`, or a non-zero integer literal. Used by W0102 so +/// `while true { ... }` gets the same treatment as bare `loop`. +fn is_always_true(expr: &Expr) -> bool { + match expr { + Expr::BoolLiteral(true, _) => true, + Expr::IntLiteral(v, _) => *v != 0, + _ => false, + } +} + fn stmt_can_exit_or_yield(stmt: &Statement) -> bool { match stmt { Statement::Break(_) diff --git a/src/analyzer/tests.rs b/src/analyzer/tests.rs index f29728c..5b9fb7c 100644 --- a/src/analyzer/tests.rs +++ b/src/analyzer/tests.rs @@ -1148,3 +1148,359 @@ fn analyze_does_not_reserve_zero_page_without_palette_or_bg() { .expect("x should be allocated"); assert_eq!(x.address, 0x10); } + +// ── W0102 extended: `while true` + continue-only loops ────── + +#[test] +fn analyze_while_true_without_exit_warns() { + // `while true { x = x + 1 }` — no break/return/wait_frame, + // so the same W0102 that fires on bare `loop { ... }` must + // also fire here. + let errors = analyze_errors( + r#" + game "T" { mapper: NROM } + var x: u8 = 0 + on frame { + while true { x = x + 1 } + } + start Main + "#, + ); + assert!( + errors.contains(&ErrorCode::W0102), + "`while true` with no exit should produce W0102, got: {errors:?}" + ); +} + +#[test] +fn analyze_while_true_with_wait_frame_ok() { + // `while true { wait_frame }` yields control to the NMI each + // iteration, so the NES actually makes progress — no warning. + let result = analyze_ok( + r#" + game "T" { mapper: NROM } + on frame { + while true { wait_frame } + } + start Main + "#, + ); + assert!( + !result + .diagnostics + .iter() + .any(|d| d.code == ErrorCode::W0102), + "`while true` + wait_frame should NOT warn, got: {:?}", + result.diagnostics + ); +} + +#[test] +fn analyze_while_true_with_break_ok() { + // A reachable `break` satisfies W0102 just like it does for + // bare `loop`. + let result = analyze_ok( + r#" + game "T" { mapper: NROM } + var x: u8 = 0 + on frame { + while true { + x = x + 1 + if x == 10 { break } + } + } + start Main + "#, + ); + assert!( + !result + .diagnostics + .iter() + .any(|d| d.code == ErrorCode::W0102), + "`while true` + break should NOT warn, got: {:?}", + result.diagnostics + ); +} + +#[test] +fn analyze_loop_with_only_continue_still_warns() { + // `continue` is *not* an exit — the loop still spins forever. + // W0102 must still fire here. + let errors = analyze_errors( + r#" + game "T" { mapper: NROM } + var x: u8 = 0 + on frame { + loop { + if x == 0 { continue } + } + } + start Main + "#, + ); + assert!( + errors.contains(&ErrorCode::W0102), + "`loop` whose only exit is `continue` should still produce W0102, got: {errors:?}" + ); +} + +// ── W0105: palette universal-byte consistency ─────────────── + +#[test] +fn analyze_palette_consistent_universals_ok() { + // Flat-form palette where every sub-palette's first byte is + // the same universal colour ($0F = black). No W0105. + let result = analyze_ok( + r#" + game "T" { mapper: NROM } + palette Consistent { + colors: [ + 0x0F, 0x11, 0x12, 0x13, + 0x0F, 0x21, 0x22, 0x23, + 0x0F, 0x31, 0x32, 0x33, + 0x0F, 0x01, 0x02, 0x03, + 0x0F, 0x05, 0x06, 0x07, + 0x0F, 0x15, 0x16, 0x17, + 0x0F, 0x25, 0x26, 0x27, + 0x0F, 0x35, 0x36, 0x37 + ] + } + on frame { wait_frame } + start Main + "#, + ); + assert!( + !result + .diagnostics + .iter() + .any(|d| d.code == ErrorCode::W0105), + "consistent palette should NOT warn, got: {:?}", + result.diagnostics + ); +} + +#[test] +fn analyze_palette_inconsistent_universals_warns() { + // Flat-form palette whose sub-palette first bytes disagree + // (index 0 = $0F, index 16 = $30, etc.) — the $3F10 mirror + // will overwrite the background universal colour at runtime. + let errors = analyze_errors( + r#" + game "T" { mapper: NROM } + palette Broken { + colors: [ + 0x0F, 0x11, 0x12, 0x13, + 0x0F, 0x21, 0x22, 0x23, + 0x0F, 0x31, 0x32, 0x33, + 0x0F, 0x01, 0x02, 0x03, + 0x30, 0x05, 0x06, 0x07, + 0x0F, 0x15, 0x16, 0x17, + 0x0F, 0x25, 0x26, 0x27, + 0x0F, 0x35, 0x36, 0x37 + ] + } + on frame { wait_frame } + start Main + "#, + ); + assert!( + errors.contains(&ErrorCode::W0105), + "inconsistent palette universals should produce W0105, got: {errors:?}" + ); +} + +#[test] +fn analyze_grouped_palette_is_always_consistent() { + // The grouped form uses the `universal:` field to drive + // every sub-palette's first byte, so it can never trip + // W0105 — even when the sub-palette bodies differ wildly. + let result = analyze_ok( + r#" + game "T" { mapper: NROM } + palette Grouped { + universal: 0x0F + bg0: [0x11, 0x12, 0x13] + bg1: [0x21, 0x22, 0x23] + bg2: [0x31, 0x32, 0x33] + bg3: [0x01, 0x02, 0x03] + sp0: [0x05, 0x06, 0x07] + sp1: [0x15, 0x16, 0x17] + sp2: [0x25, 0x26, 0x27] + sp3: [0x35, 0x36, 0x37] + } + on frame { wait_frame } + start Main + "#, + ); + assert!( + !result + .diagnostics + .iter() + .any(|d| d.code == ErrorCode::W0105), + "grouped palette should never trip W0105, got: {:?}", + result.diagnostics + ); +} + +// ── W0106: implicit drop of a function return value ───────── + +#[test] +fn analyze_discarded_non_void_return_warns() { + // `double(x)` returns u8 but the caller drops the result. + let errors = analyze_errors( + r#" + game "T" { mapper: NROM } + var x: u8 = 0 + fun double(n: u8) -> u8 { return n + n } + on frame { double(x) } + start Main + "#, + ); + assert!( + errors.contains(&ErrorCode::W0106), + "discarded non-void return should produce W0106, got: {errors:?}" + ); +} + +#[test] +fn analyze_discarded_void_call_ok() { + // Void function at statement position is the happy path — + // no discarded value, no warning. + let result = analyze_ok( + r#" + game "T" { mapper: NROM } + var x: u8 = 0 + fun bump() { x = x + 1 } + on frame { bump() } + start Main + "#, + ); + assert!( + !result + .diagnostics + .iter() + .any(|d| d.code == ErrorCode::W0106), + "void call should NOT produce W0106, got: {:?}", + result.diagnostics + ); +} + +#[test] +fn analyze_non_void_return_used_as_rhs_ok() { + // Same signature as the discarded case, but the return value + // is consumed by an assignment — no warning. + let result = analyze_ok( + r#" + game "T" { mapper: NROM } + var x: u8 = 0 + var y: u8 = 0 + fun double(n: u8) -> u8 { return n + n } + on frame { y = double(x) } + start Main + "#, + ); + assert!( + !result + .diagnostics + .iter() + .any(|d| d.code == ErrorCode::W0106), + "assigned return value should NOT produce W0106, got: {:?}", + result.diagnostics + ); +} + +// ── W0107: `fast` variable slot under-use ─────────────────── + +#[test] +fn analyze_fast_var_underused_warns() { + // `counter` is declared `fast` but only one read (in the + // `if` condition), so its access count is 1 — below the + // threshold of 3. W0107 should fire. + let errors = analyze_errors( + r#" + game "T" { mapper: NROM } + fast var counter: u8 = 0 + on frame { + if counter == 0 { wait_frame } + } + start Main + "#, + ); + assert!( + errors.contains(&ErrorCode::W0107), + "under-used `fast` var should produce W0107, got: {errors:?}" + ); +} + +#[test] +fn analyze_fast_var_heavy_use_ok() { + // Three-plus accesses (one init + one read + one write-back) + // is enough to justify the slot — no W0107. + let result = analyze_ok( + r#" + game "T" { mapper: NROM } + fast var counter: u8 = 0 + on frame { + counter = counter + 1 + if counter == 0 { wait_frame } + } + start Main + "#, + ); + assert!( + !result + .diagnostics + .iter() + .any(|d| d.code == ErrorCode::W0107), + "hot `fast` var should NOT warn, got: {:?}", + result.diagnostics + ); +} + +#[test] +fn analyze_non_fast_var_never_warns() { + // Only `fast` declarations are checked — a plain `var` with + // the same (light) access pattern must not fire W0107. + let result = analyze_ok( + r#" + game "T" { mapper: NROM } + var counter: u8 = 0 + on frame { + if counter == 0 { wait_frame } + } + start Main + "#, + ); + assert!( + !result + .diagnostics + .iter() + .any(|d| d.code == ErrorCode::W0107), + "plain `var` should never trip W0107, got: {:?}", + result.diagnostics + ); +} + +#[test] +fn analyze_fast_var_underscore_exempt() { + // Leading-underscore names are exempt from W0107, mirroring + // the W0103 convention for deliberately-unused variables. + let result = analyze_ok( + r#" + game "T" { mapper: NROM } + fast var _reserved: u8 = 0 + on frame { + if _reserved == 0 { wait_frame } + } + start Main + "#, + ); + assert!( + !result + .diagnostics + .iter() + .any(|d| d.code == ErrorCode::W0107), + "underscore-prefixed `fast` var should be exempt, got: {:?}", + result.diagnostics + ); +} diff --git a/src/errors/diagnostic.rs b/src/errors/diagnostic.rs index d4fae9f..3dc73a8 100644 --- a/src/errors/diagnostic.rs +++ b/src/errors/diagnostic.rs @@ -39,6 +39,9 @@ pub enum ErrorCode { W0102, // loop without break or wait_frame W0103, // unused variable W0104, // unreachable code after terminator, or unreachable state + W0105, // palette sub-palette universal mismatch (mirror collision) + W0106, // implicit drop of non-void function return value + W0107, // `fast` variable rarely accessed (wastes zero-page slot) } impl fmt::Display for ErrorCode { @@ -62,6 +65,9 @@ impl fmt::Display for ErrorCode { Self::W0102 => "W0102", Self::W0103 => "W0103", Self::W0104 => "W0104", + Self::W0105 => "W0105", + Self::W0106 => "W0106", + Self::W0107 => "W0107", }; write!(f, "{code}") } @@ -70,7 +76,13 @@ impl fmt::Display for ErrorCode { impl ErrorCode { pub fn level(self) -> Level { match self { - Self::W0101 | Self::W0102 | Self::W0103 | Self::W0104 => Level::Warning, + Self::W0101 + | Self::W0102 + | Self::W0103 + | Self::W0104 + | Self::W0105 + | Self::W0106 + | Self::W0107 => Level::Warning, _ => Level::Error, } }