From 667f565ddd1dd57a29b85c2cbf9445af761a0f9f Mon Sep 17 00:00:00 2001 From: Arena Agent Date: Tue, 28 Jul 2026 20:23:51 +0000 Subject: [PATCH 1/2] fix(cad): match arms bound new variables instead of matching KeyCode Went looking for a clippy ratchet and found that clippy had never run on this crate at all: two dependencies fail deny-by-default lints, so `cargo clippy -p nigig-build` aborted before linting nigig-build. Fixing those two unblocked the crate and immediately exposed a real defect class. THE BUG. A bare identifier in a match pattern that is not a known variant is parsed by Rust as a NEW BINDING that matches everything. Nine such names were used as KeyCode patterns: KeyEnter, KeyBackspace, BracketLeft, BracketRight (cad/viewport.rs) Digit0..Digit9, Equal, LeftBracket, RightBracket, Apostrophe (doc, invoice, pm) Real names are ReturnKey, Backspace, LBracket, RBracket, Key0..Key9, Equals, Quote. Consequences, in severity order: - cad/viewport.rs: `BracketLeft => { .. }` is UNGUARDED and sits above the numeric arms, so it swallowed every remaining key. All direct-distance-entry input -- digits, '.', '-', ',' -- was unreachable. The entire DDE feature was dead. - The guarded ones fired for any key satisfying the guard: pressing Q while drawing with a non-empty buffer committed the coordinate. - doc/invoice/project_management: `Digit0 => '0'` swallowed the rest of the keymap, so every digit and symbol typed produced '0'. This compiles cleanly and no test catches it. rustc's only signal is `unreachable_pattern` plus `unused variable: \`Capitalised\`` -- both buried in the 200+ warnings nobody could see, because clippy never ran. 85 unreachable-pattern warnings before, 1 after (a benign catch-all). UNBLOCKING CLIPPY. Two deny-level errors in dependencies: - nigig-uikit user_project_pill.rs: a `for` loop returning on its first iteration (never_loop). Rewritten as `.next()`. - spreadsheet-engine formula2.rs: CellRef had an inherent to_string shadowing Display (inherent_to_string_shadow_display). The naive fix is a trap: Display::fmt was `f.write_str(&self.to_string())`, which resolved to the inherent method -- delete it and the same call resolves to ToString::to_string, which calls Display::fmt, recursing until the stack overflows. Verified with a standalone repro before fixing. The body moved into Display; Range got the Display impl it never had. Tests: keycode_variant_tests names the four correct variants, so it stops compiling if any is renamed -- the point being that the old code compiled precisely because the names were wrong. Two formula2 tests pin that `x.to_string()` and `format!("{x}")` agree, which is what the shadowing lint exists to protect. CI gate added and negative-tested: greps for a capitalised `unused variable`, which is the signature of this bug. Restoring BracketLeft makes it fire. 625 lib + 154 integration + 225 spreadsheet-engine + 44 doc-engine, 0 failed. --- .forgejo/workflows/nigig-build.yml | 27 +++++++ .../pages/workspace/cad/ARCHITECTURE.md | 17 ++++ .../pages/workspace/cad/viewport.rs | 51 ++++++++++-- .../pages/workspace/doc/widgets/doc_widget.rs | 28 +++---- .../pages/workspace/invoice/mod.rs | 28 +++---- .../pages/workspace/project_management/mod.rs | 8 +- .../workspace/project_management/utils.rs | 8 +- .../spreadsheet-engine/src/formula2.rs | 78 ++++++++++++++----- .../src/shared/user_project_pill.rs | 10 ++- 9 files changed, 189 insertions(+), 66 deletions(-) diff --git a/.forgejo/workflows/nigig-build.yml b/.forgejo/workflows/nigig-build.yml index 5d82f40..754f0d4 100644 --- a/.forgejo/workflows/nigig-build.yml +++ b/.forgejo/workflows/nigig-build.yml @@ -176,6 +176,33 @@ jobs: cargo-deny --manifest-path crates/apps/nigig-build/Cargo.toml \ --all-features check --config "$PWD/deny-nigig-build.toml" + # A bare identifier in a match pattern that is NOT a known variant + # is parsed as a new binding that matches everything. Four such + # names (KeyEnter, KeyBackspace, BracketLeft, BracketRight) plus + # Digit0-9/Equal/LeftBracket/RightBracket/Apostrophe silently turned + # keyboard handlers into catch-alls: the whole direct-distance-entry + # feature was unreachable, and typing any digit produced '0'. + # + # It compiles, and no test catches it. rustc reports it as + # `unreachable_pattern` plus an `unused_variables` warning on a + # capitalised name -- which is the signature grepped for here. + - name: No match arms binding a non-existent enum variant + run: | + set -euo pipefail + # A capitalised "unused variable" is almost always a mistyped + # variant used as a pattern. + out=$(cargo build --locked -p nigig-build --lib --message-format=short 2>&1 \ + | grep -E 'unused variable: `[A-Z]' || true) + if [ -n "$out" ]; then + echo "$out" + echo + echo "ERROR: the pattern(s) above bind a new variable instead of" + echo "matching an enum variant -- check the spelling against the" + echo "enum definition. These silently swallow every input." + exit 1 + fi + echo "OK" + - name: Reject whitespace errors run: git diff --check diff --git a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/ARCHITECTURE.md b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/ARCHITECTURE.md index 48e2c35..a2faeb5 100644 --- a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/ARCHITECTURE.md +++ b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/ARCHITECTURE.md @@ -252,6 +252,23 @@ has to own a `Solid` just to get GPU buffers. --- +## Keyboard shortcuts + +`handle_tool_key_shortcut` matches on `KeyCode` with `use KeyCode::*` in +scope. **A misspelled variant does not fail to compile.** Rust reads an +unknown bare identifier in a pattern as a new binding that matches +everything, so `BracketLeft => { .. }` (the real name is `LBracket`) was +an unguarded catch-all sitting above the numeric arms — every +direct-distance-entry key was unreachable. + +The only signal is a `unused variable: \`Capitalised\`` warning plus +`unreachable_pattern`. CI greps for the first. When adding a shortcut, +check the variant name against the platform crate: it is `ReturnKey`, +`Backspace`, `LBracket`/`RBracket`, `Key0`–`Key9`, `Equals`, `Quote` — +not the DOM-style names. + +--- + ## Runtime paths and secrets Two rules, both enforced by CI (`.forgejo/workflows/nigig-build.yml`): diff --git a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/viewport.rs b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/viewport.rs index 85a4266..63cbea4 100644 --- a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/viewport.rs +++ b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/viewport.rs @@ -3841,22 +3841,22 @@ impl CadViewport { } } // DDE: Enter commits coordinate input when buffer is non-empty - KeyEnter if self.drawing.is_drawing && !self.drawing.dde_buffer.is_empty() => { + ReturnKey if self.drawing.is_drawing && !self.drawing.dde_buffer.is_empty() => { self.dde_live_preview(); self.drawing.dde_buffer.clear(); self.area.redraw(cx); cx.redraw_all(); true } - KeyEnter if self.drawing.is_drawing && self.drawing.tool == CadTool::Polyline => { + ReturnKey if self.drawing.is_drawing && self.drawing.tool == CadTool::Polyline => { self.finish_polyline(cx); true } - KeyEnter if self.drawing.is_drawing && self.drawing.tool == CadTool::Polygon => { + ReturnKey if self.drawing.is_drawing && self.drawing.tool == CadTool::Polygon => { self.finish_drawing(cx); true } - KeyEnter if self.drawing.is_drawing && self.drawing.tool == CadTool::Circle => { + ReturnKey if self.drawing.is_drawing && self.drawing.tool == CadTool::Circle => { // Commit circle with current radius (set via prompt or mouse) self.finish_drawing(cx); true @@ -3914,14 +3914,14 @@ impl CadViewport { true } // DDE: Backspace removes last char from buffer, or undoes polyline point - KeyBackspace if self.drawing.is_drawing && !self.drawing.dde_buffer.is_empty() => { + Backspace if self.drawing.is_drawing && !self.drawing.dde_buffer.is_empty() => { self.drawing.dde_buffer.pop(); self.area.redraw(cx); cx.redraw_all(); true } // Per-point undo during polyline drawing - KeyBackspace + Backspace if self.drawing.is_drawing && !self.drawing.polyline_points.is_empty() => { // `let else` rather than `.unwrap()`: the match guard above @@ -3997,13 +3997,13 @@ impl CadViewport { true } // Tolerance adjustment - BracketLeft => { + LBracket => { self.snap.snap_tolerance = (self.snap.snap_tolerance * 0.5).max(0.01); self.area.redraw(cx); cx.redraw_all(); true } - BracketRight => { + RBracket => { self.snap.snap_tolerance = (self.snap.snap_tolerance * 2.0).min(5.0); self.area.redraw(cx); cx.redraw_all(); @@ -7284,6 +7284,41 @@ fn part_model_matrix_cadnode(node: &CadNode) -> Mat4f { // in the wrong place. // =========================================================================== +#[cfg(test)] +mod keycode_variant_tests { + use makepad_widgets::makepad_platform::KeyCode; + + /// Four names used as `match` arms in `handle_key_shortcuts` are not + /// `KeyCode` variants at all: `KeyEnter`, `KeyBackspace`, + /// `BracketLeft`, `BracketRight`. Rust parses an unknown lowercase-or- + /// uppercase bare identifier in a pattern as a NEW BINDING that + /// matches everything, so each was an irrefutable catch-all rather + /// than a key test. + /// + /// `BracketLeft` was unguarded and sat above the numeric arms, so it + /// swallowed every remaining key: all direct-distance-entry input + /// (digits, `.`, `-`, `,`) was unreachable. The guarded ones + /// (`KeyEnter if ..`, `KeyBackspace if ..`) fired for *any* key that + /// satisfied the guard -- pressing `Q` while drawing with a non-empty + /// buffer committed the coordinate. + /// + /// This test names the real variants. It does not compile if any of + /// them is renamed upstream, which is the point: the previous code + /// compiled precisely because the names were wrong. + #[test] + fn the_real_keycode_variants_exist_under_their_correct_names() { + // Correct spellings, verified against the platform crate. + let _: KeyCode = KeyCode::ReturnKey; + let _: KeyCode = KeyCode::Backspace; + let _: KeyCode = KeyCode::LBracket; + let _: KeyCode = KeyCode::RBracket; + + // They are distinct, so a match on one cannot answer for another. + assert_ne!(KeyCode::ReturnKey, KeyCode::Backspace); + assert_ne!(KeyCode::LBracket, KeyCode::RBracket); + } +} + #[cfg(test)] mod parts_sync_tests { use crate::construction_frame::pages::workspace::cad::cad_scene::{ diff --git a/crates/apps/nigig-build/src/construction_frame/pages/workspace/doc/widgets/doc_widget.rs b/crates/apps/nigig-build/src/construction_frame/pages/workspace/doc/widgets/doc_widget.rs index 1542329..62bc9d4 100644 --- a/crates/apps/nigig-build/src/construction_frame/pages/workspace/doc/widgets/doc_widget.rs +++ b/crates/apps/nigig-build/src/construction_frame/pages/workspace/doc/widgets/doc_widget.rs @@ -2685,24 +2685,24 @@ fn key_to_char_doc(key_code: KeyCode, is_shift: bool) -> Option { KeyX => 'x', KeyY => 'y', KeyZ => 'z', - Digit0 => '0', - Digit1 => '1', - Digit2 => '2', - Digit3 => '3', - Digit4 => '4', - Digit5 => '5', - Digit6 => '6', - Digit7 => '7', - Digit8 => '8', - Digit9 => '9', + Key0 => '0', + Key1 => '1', + Key2 => '2', + Key3 => '3', + Key4 => '4', + Key5 => '5', + Key6 => '6', + Key7 => '7', + Key8 => '8', + Key9 => '9', Space => ' ', Minus => '-', - Equal => '=', - LeftBracket => '[', - RightBracket => ']', + Equals => '=', + LBracket => '[', + RBracket => ']', Backslash => '\\', Semicolon => ';', - Apostrophe => '\'', + Quote => '\'', Comma => ',', Period => '.', Slash => '/', diff --git a/crates/apps/nigig-build/src/construction_frame/pages/workspace/invoice/mod.rs b/crates/apps/nigig-build/src/construction_frame/pages/workspace/invoice/mod.rs index 7d1661e..9f2e613 100644 --- a/crates/apps/nigig-build/src/construction_frame/pages/workspace/invoice/mod.rs +++ b/crates/apps/nigig-build/src/construction_frame/pages/workspace/invoice/mod.rs @@ -1295,24 +1295,24 @@ fn key_to_char_invoice(key_code: KeyCode, is_shift: bool) -> Option { KeyX => 'x', KeyY => 'y', KeyZ => 'z', - Digit0 => '0', - Digit1 => '1', - Digit2 => '2', - Digit3 => '3', - Digit4 => '4', - Digit5 => '5', - Digit6 => '6', - Digit7 => '7', - Digit8 => '8', - Digit9 => '9', + Key0 => '0', + Key1 => '1', + Key2 => '2', + Key3 => '3', + Key4 => '4', + Key5 => '5', + Key6 => '6', + Key7 => '7', + Key8 => '8', + Key9 => '9', Space => ' ', Minus => '-', - Equal => '=', - LeftBracket => '[', - RightBracket => ']', + Equals => '=', + LBracket => '[', + RBracket => ']', Backslash => '\\', Semicolon => ';', - Apostrophe => '\'', + Quote => '\'', Comma => ',', Period => '.', Slash => '/', diff --git a/crates/apps/nigig-build/src/construction_frame/pages/workspace/project_management/mod.rs b/crates/apps/nigig-build/src/construction_frame/pages/workspace/project_management/mod.rs index 56789fb..8960794 100755 --- a/crates/apps/nigig-build/src/construction_frame/pages/workspace/project_management/mod.rs +++ b/crates/apps/nigig-build/src/construction_frame/pages/workspace/project_management/mod.rs @@ -2022,10 +2022,10 @@ pub fn key_to_char_gantt(key_code: KeyCode, is_shift: bool) -> Option { KeyM => 'm', KeyN => 'n', KeyO => 'o', KeyP => 'p', KeyQ => 'q', KeyR => 'r', KeyS => 's', KeyT => 't', KeyU => 'u', KeyV => 'v', KeyW => 'w', KeyX => 'x', KeyY => 'y', KeyZ => 'z', - Digit0 => '0', Digit1 => '1', Digit2 => '2', Digit3 => '3', Digit4 => '4', - Digit5 => '5', Digit6 => '6', Digit7 => '7', Digit8 => '8', Digit9 => '9', - Space => ' ', Minus => '-', Equal => '=', LeftBracket => '[', RightBracket => ']', - Backslash => '\\', Semicolon => ';', Apostrophe => '\'', Comma => ',', Period => '.', + Key0 => '0', Key1 => '1', Key2 => '2', Key3 => '3', Key4 => '4', + Key5 => '5', Key6 => '6', Key7 => '7', Key8 => '8', Key9 => '9', + Space => ' ', Minus => '-', Equals => '=', LBracket => '[', RBracket => ']', + Backslash => '\\', Semicolon => ';', Quote => '\'', Comma => ',', Period => '.', Slash => '/', _ => return None, }; if is_shift { diff --git a/crates/apps/nigig-build/src/construction_frame/pages/workspace/project_management/utils.rs b/crates/apps/nigig-build/src/construction_frame/pages/workspace/project_management/utils.rs index b202801..ec80b76 100644 --- a/crates/apps/nigig-build/src/construction_frame/pages/workspace/project_management/utils.rs +++ b/crates/apps/nigig-build/src/construction_frame/pages/workspace/project_management/utils.rs @@ -8,10 +8,10 @@ pub fn key_to_char_gantt(key_code: KeyCode, is_shift: bool) -> Option { KeyM => 'm', KeyN => 'n', KeyO => 'o', KeyP => 'p', KeyQ => 'q', KeyR => 'r', KeyS => 's', KeyT => 't', KeyU => 'u', KeyV => 'v', KeyW => 'w', KeyX => 'x', KeyY => 'y', KeyZ => 'z', - Digit0 => '0', Digit1 => '1', Digit2 => '2', Digit3 => '3', Digit4 => '4', - Digit5 => '5', Digit6 => '6', Digit7 => '7', Digit8 => '8', Digit9 => '9', - Space => ' ', Minus => '-', Equal => '=', LeftBracket => '[', RightBracket => ']', - Backslash => '\\', Semicolon => ';', Apostrophe => '\'', Comma => ',', Period => '.', + Key0 => '0', Key1 => '1', Key2 => '2', Key3 => '3', Key4 => '4', + Key5 => '5', Key6 => '6', Key7 => '7', Key8 => '8', Key9 => '9', + Space => ' ', Minus => '-', Equals => '=', LBracket => '[', RBracket => ']', + Backslash => '\\', Semicolon => ';', Quote => '\'', Comma => ',', Period => '.', Slash => '/', _ => return None, }; if is_shift { diff --git a/crates/apps/spreadsheet/spreadsheet-engine/src/formula2.rs b/crates/apps/spreadsheet/spreadsheet-engine/src/formula2.rs index f877b33..976c491 100644 --- a/crates/apps/spreadsheet/spreadsheet-engine/src/formula2.rs +++ b/crates/apps/spreadsheet/spreadsheet-engine/src/formula2.rs @@ -160,26 +160,28 @@ impl CellRef { }) } - /// Format back to A1 notation (e.g. `$A$1`, `B2`). - pub fn to_string(&self) -> String { - use crate::util::col_letters; - let col_str = col_letters(self.col); - let mut out = String::new(); - if self.abs_col { - out.push('$'); - } - out.push_str(&col_str); - if self.abs_row { - out.push('$'); - } - out.push_str(&(self.row + 1).to_string()); - out - } } +/// Format back to A1 notation (e.g. `$A$1`, `B2`). +/// +/// This is the only implementation. It used to be an inherent +/// `to_string`, with `Display::fmt` delegating to it -- which clippy +/// denies (`inherent_to_string_shadow_display`), because the two +/// spellings can diverge. Note that the delegation could not simply be +/// deleted: `f.write_str(&self.to_string())` would then resolve to +/// `ToString::to_string`, which calls `Display::fmt`, recursing until +/// the stack overflows. The body has to move here. impl fmt::Display for CellRef { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.write_str(&self.to_string()) + use crate::util::col_letters; + if self.abs_col { + f.write_str("$")?; + } + f.write_str(&col_letters(self.col))?; + if self.abs_row { + f.write_str("$")?; + } + write!(f, "{}", self.row + 1) } } @@ -208,8 +210,11 @@ impl Range { (min_r..=max_r).flat_map(move |r| (min_c..=max_c).map(move |c| (r, c))) } - pub fn to_string(&self) -> String { - format!("{}:{}", self.start, self.end) +} + +impl fmt::Display for Range { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}:{}", self.start, self.end) } } @@ -1216,6 +1221,43 @@ mod tests { use super::*; use std::cell::RefCell; + /// `CellRef` and `Range` carried an inherent `to_string` alongside a + /// `Display` impl that called it. Clippy denies that shadowing + /// (`inherent_to_string_shadow_display`) because `x.to_string()` and + /// `format!("{x}")` could silently diverge. + /// + /// The trap when removing the inherent method: `Display::fmt` was + /// written as `f.write_str(&self.to_string())`, which resolved to the + /// inherent method. Delete that method and the same call resolves to + /// `ToString::to_string`, which calls `Display::fmt` -- infinite + /// recursion and a stack overflow, not a compile error. + /// + /// These pin both spellings so the formatting logic must actually + /// live in `Display`. + #[test] + fn cellref_formats_identically_via_display_and_to_string() { + let cases = [ + (CellRef { row: 0, col: 0, abs_row: false, abs_col: false }, "A1"), + (CellRef { row: 0, col: 0, abs_row: true, abs_col: true }, "$A$1"), + (CellRef { row: 9, col: 1, abs_row: false, abs_col: true }, "$B10"), + (CellRef { row: 41, col: 27, abs_row: true, abs_col: false }, "AB$42"), + ]; + for (cr, want) in cases { + assert_eq!(format!("{cr}"), want, "Display"); + assert_eq!(cr.to_string(), want, "to_string"); + } + } + + #[test] + fn range_formats_identically_via_display_and_to_string() { + let r = Range::new( + CellRef { row: 0, col: 0, abs_row: false, abs_col: false }, + CellRef { row: 9, col: 1, abs_row: true, abs_col: true }, + ); + assert_eq!(format!("{r}"), "A1:$B$10"); + assert_eq!(r.to_string(), "A1:$B$10"); + } + /// A simple in-memory cell store for evaluation tests. struct MockCtx { cells: std::collections::HashMap<(u32, u32), String>, diff --git a/crates/nigig-uikit/src/shared/user_project_pill.rs b/crates/nigig-uikit/src/shared/user_project_pill.rs index c526b76..71b9217 100644 --- a/crates/nigig-uikit/src/shared/user_project_pill.rs +++ b/crates/nigig-uikit/src/shared/user_project_pill.rs @@ -447,9 +447,11 @@ impl UserProjectPillRef { } pub fn selected(&self, actions: &Actions) -> Option { - for action in actions.filter_widget_actions_cast::(self.widget_uid()) { - return Some(action); - } - None + // `.next()`, not a `for` loop that returns on its first iteration. + // Same behaviour, but the loop form reads as if it inspects every + // action and clippy denies it (`never_loop`). + actions + .filter_widget_actions_cast::(self.widget_uid()) + .next() } } From 915c338987c16617ba1f22239a21d28e41fd4fe3 Mon Sep 17 00:00:00 2001 From: Arena Agent Date: Tue, 28 Jul 2026 20:26:03 +0000 Subject: [PATCH 2/2] test(spreadsheet-ui): fix a wrong assertion in the new exchange test origin/main was red: navigation_and_lifecycle_are_headless asserted that after `exchange_sheet_data(0, &mut external)` the caller's buffer holds "active". Nothing in the test ever writes that string. `exchange_sheet_data` is a two-way `mem::swap`, so `external` comes back with sheet 0's contents. Sheet 0 is empty at that point: "adapter" was written while sheet 1 was active, and `remove_sheet(1)` then deleted the sheet holding it. Corrected to assert the empty string, and added the assertion the test was missing -- that the sheet now holds "grid". Checking only one side of a swap would pass for a function that merely cleared the buffer. Verified pre-existing: fails identically on origin/main without my commits. --- .../spreadsheet/spreadsheet-ui/src/model.rs | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/crates/apps/spreadsheet/spreadsheet-ui/src/model.rs b/crates/apps/spreadsheet/spreadsheet-ui/src/model.rs index 522b558..4048390 100644 --- a/crates/apps/spreadsheet/spreadsheet-ui/src/model.rs +++ b/crates/apps/spreadsheet/spreadsheet-ui/src/model.rs @@ -147,10 +147,27 @@ mod tests { let restored = WorkspaceModel::from_serialized(&model.serialize()).unwrap(); assert_eq!(restored.workbook().sheet_count(), 2); + // `exchange_sheet_data` swaps the caller's buffer with the + // sheet's, so `external` comes back holding whatever sheet 0 had. + // + // Sheet 0 is empty here: "adapter" was written while sheet 1 was + // active, and `remove_sheet(1)` then deleted that sheet. The + // original assertion expected "active", a value this test never + // writes anywhere. let mut external = spreadsheet_engine::data::SpreadsheetData::default(); external.set_cell(0, 0, "grid"); assert!(model.exchange_sheet_data(0, &mut external)); - assert_eq!(external.get_raw(0, 0), "active"); + assert_eq!( + external.get_raw(0, 0), + "", + "external must receive sheet 0's (empty) contents" + ); + // ...and the sheet must now hold what the caller passed in. + assert_eq!( + model.workbook().get_raw(0, 0), + "grid", + "the swap must be two-way, not a one-way read" + ); assert!(!model.exchange_sheet_data(99, &mut external)); } }