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/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)); } } 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() } }