From 3e323bfa2ae3e19d2ce5fbaca6cbdd956c3a3e3f Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Sun, 4 Oct 2026 12:00:20 -0400 Subject: [PATCH] =?UTF-8?q?Robustness:=20VectorCraft=20never=20crashes=20?= =?UTF-8?q?=E2=80=94=20no=20panics=20in=20shipped=20code?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Shipped code no longer unwraps, expects or panics: every one of the 124 unwrap()/expect()/panic!/unreachable! sites outside tests now handles the failure (Result/Option, ?, let-else, defaults) and reports errors to the user. - Lints: workspace clippy denies unwrap_used, expect_used, panic, todo, unimplemented and unreachable; clippy.toml allows them in tests, and integration tests, examples and testkit opt out explicitly. - Safety net (vectorcraft_engine::guard): a panic in a top-level command rolls the document back and returns EngineError::Internal; a panicking tool is reset; the MCP server answers with an internal error and keeps serving; the UI frame and each control request are guarded. - APIs that hid failures now return them: Rendered::to_png/to_jpeg/to_webp return Result (JPEG/WebP errors used to write empty files), FontDb::face returns Option, Region::top returns Option. - Fuzzing: engine/tests/import_fuzz.rs feeds garbage, hostile and mutated SVG and PDF through import → render → export (30k cases locally, no panics). - Docs: AGENTS.md/CLAUDE.md rule, docs/development.md "Robustness" section, README pointer and a PR template checklist. Co-Authored-By: Claude Opus 5.5 --- .github/pull_request_template.md | 17 ++ AGENTS.md | 10 +- CLAUDE.md | 10 +- Cargo.toml | 9 +- README.md | 5 +- apps/vectorcraft-cli/src/perf.rs | 33 ++- apps/vectorcraft-cli/tests/cli.rs | 2 + apps/vectorcraft-cli/tests/mcp_e2e.rs | 2 + apps/vectorcraft/build.rs | 7 +- clippy.toml | 5 + crates/brush/src/calli.rs | 3 +- crates/brush/src/lib.rs | 6 +- crates/brush/src/tests.rs | 2 +- crates/brush/src/track.rs | 6 +- crates/color/src/gradient.rs | 6 +- crates/color/src/swatch.rs | 19 +- crates/doc/src/appearance.rs | 3 +- crates/doc/src/lib.rs | 10 +- crates/doc/src/live.rs | 37 ++-- crates/effects/src/group.rs | 7 +- crates/engine/src/cmd/brushsym.rs | 13 +- crates/engine/src/cmd/buildcmds.rs | 13 +- crates/engine/src/cmd/colorcmds.rs | 18 +- crates/engine/src/cmd/distortcmds.rs | 3 +- crates/engine/src/cmd/draw2.rs | 89 +++++--- crates/engine/src/cmd/fileio/encode.rs | 1 + crates/engine/src/cmd/flatten.rs | 46 ++-- crates/engine/src/cmd/fonts.rs | 5 +- crates/engine/src/cmd/live.rs | 6 +- crates/engine/src/cmd/menucmds.rs | 8 +- crates/engine/src/cmd/mod.rs | 8 +- crates/engine/src/cmd/object.rs | 4 +- crates/engine/src/cmd/path.rs | 29 ++- crates/engine/src/cmd/pathops.rs | 20 +- crates/engine/src/cmd/patterncmds.rs | 9 +- crates/engine/src/cmd/rasterfx.rs | 6 +- crates/engine/src/cmd/select.rs | 2 +- crates/engine/src/cmd/textedit.rs | 7 +- crates/engine/src/cmd/textstyles.rs | 17 +- crates/engine/src/cmd/threads.rs | 2 +- crates/engine/src/cmd/typecmd.rs | 2 +- crates/engine/src/guard.rs | 46 ++++ crates/engine/src/lib.rs | 44 +++- crates/engine/src/tests.rs | 29 +++ crates/engine/src/tooling.rs | 23 +- crates/engine/tests/command_sweep.rs | 2 + crates/engine/tests/import_fuzz.rs | 285 +++++++++++++++++++++++++ crates/engine/tests/known_bugs.rs | 2 + crates/engine/tests/model_based.rs | 2 + crates/format/tests/prop_format.rs | 2 + crates/geom/src/path.rs | 21 +- crates/geom/tests/prop_geom.rs | 4 +- crates/mcp/src/backend.rs | 4 +- crates/mcp/src/headless.rs | 2 +- crates/mcp/src/server.rs | 5 +- crates/mcp/src/tools.rs | 4 +- crates/mcp/tests/protocol_props.rs | 2 + crates/pathops/src/edit.rs | 53 +++-- crates/pathops/src/fit.rs | 3 +- crates/pathops/src/pathfinder.rs | 22 +- crates/pathops/tests/ops.rs | 2 + crates/pathops/tests/prop_pathops.rs | 2 + crates/pathops/tests/props.rs | 2 + crates/pdf/src/import.rs | 6 +- crates/render/examples/bench.rs | 2 + crates/render/src/lib.rs | 29 +-- crates/render/src/tests.rs | 2 +- crates/render/tests/golden.rs | 6 +- crates/svg/src/import.rs | 5 +- crates/svg/tests/prop_svg.rs | 2 + crates/svg/tests/roundtrip.rs | 2 + crates/testkit/src/lib.rs | 4 + crates/text/src/fontdb.rs | 14 +- crates/text/src/shape.rs | 11 +- crates/text/src/tests.rs | 12 +- crates/text/src/tests_typo.rs | 2 +- crates/tools/src/distort/arap.rs | 22 +- crates/tools/src/distort/liquify.rs | 5 +- crates/tools/src/draw2/anchor.rs | 14 +- crates/tools/src/text.rs | 3 +- crates/trace/src/lib.rs | 2 +- crates/ui-egui/src/canvas.rs | 2 +- crates/ui-egui/src/control.rs | 12 +- crates/ui-egui/src/lib.rs | 20 +- crates/ui-egui/src/menus.rs | 14 +- crates/ui-egui/src/panels/glyphs.rs | 5 +- crates/ui-egui/src/panels/layers.rs | 7 +- crates/ui-egui/src/panels/swatches.rs | 7 +- docs/development.md | 66 ++++++ 89 files changed, 1041 insertions(+), 303 deletions(-) create mode 100644 .github/pull_request_template.md create mode 100644 clippy.toml create mode 100644 crates/engine/src/guard.rs create mode 100644 crates/engine/tests/import_fuzz.rs diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md new file mode 100644 index 00000000..1ae855ad --- /dev/null +++ b/.github/pull_request_template.md @@ -0,0 +1,17 @@ +## What and why + + + +## How it was tested + + + +## Checklist + +- [ ] `cargo xtask ci` passes (fmt, clippy, tests, assets, layers, wasm). +- [ ] No panics in shipped code: no `unwrap()`, `expect()`, `panic!`, `unreachable!`, `todo!` or + `unimplemented!` outside tests, and no unchecked indexing on data from files, parameters or + messages. Errors are returned as `Result` and shown to the user + ([why and how](https://github.com/storytold/vectorcraft/blob/main/docs/development.md#robustness-vectorcraft-never-crashes)). +- [ ] Clean-room: nothing copied from Adobe products or GPL/AGPL code ([AGENTS.md](https://github.com/storytold/vectorcraft/blob/main/AGENTS.md)). +- [ ] Every new asset has a row in `ASSETS.md`. diff --git a/AGENTS.md b/AGENTS.md index d4e7cd6a..d8c591a7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -24,11 +24,19 @@ Formerly **DrawCraft** (renamed 2026-10-01): old `.drawcraft` files and `"format - Code and original assets are MIT OR Apache-2.0 (`LICENSE-MIT`, `LICENSE-APACHE`, `NOTICE`). The ArtCraft logos in `docs/brand/` are trademarks, not open source, under `docs/brand/LICENSE-brand.txt`: never modify them or use them outside VectorCraft. - Prefer art generated in code for defaults (swatches, brushes, symbols, patterns, cursors). If the provenance of an asset is unclear, don't add it. - Reference screenshots of Illustrator stay local under the gitignored `plan/` and are never committed, published or used as assets. +- **Never crash: no panics in shipped code.** A crash loses the user's unsaved work. Files, pasted data, MCP and control messages, and command params are all untrusted input. + - Fallible work returns `Result` (or `Option`); callers handle it or propagate it with `?`. Failures reach the user as an error message (`EngineError` for commands, the crate's own error type or `String` below the engine), never as an abort. + - Workspace clippy lints ban these outside tests, so `cargo xtask ci` fails on them: `unwrap()`, `expect()`, `panic!`, `unreachable!`, `todo!`, `unimplemented!`. Use instead: `?`, `.ok_or(…)?`, `let … else { return … }`, `if let`, `unwrap_or` / `unwrap_or_default` / `unwrap_or_else`, `.first()` / `.last()` / `.split_last()`, and a real error for a match arm you believe is impossible. + - Don't panic implicitly either: use `.get(i)` instead of `v[i]` or `&s[a..b]` when the index comes from data. Don't divide integers by something that can be zero, cap sizes and counts read from input (allocations, images, loops), and check that floats are finite before casting them or looping on them. + - Don't silence errors just to satisfy the lint: use `let _ =` or `.ok()` only where ignoring the failure is the right behaviour, with a comment saying why. + - Tests may unwrap and panic (`#[test]`, `#[cfg(test)]`, `tests/`, `testkit`). Integration-test files start with an `#![allow(…)]` for that. + - `vectorcraft_engine::guard` is the last line of defence. Every entry point (commands, tool events, MCP requests, the UI frame, control requests) catches a panic, rolls the document back and reports an internal error. It exists for bugs and doesn't replace `Result`: wasm aborts on panic, so the web app has no safety net. + - Untrusted input is fuzzed (`engine/tests/import_fuzz.rs`, `engine/tests/command_sweep.rs`, `format/tests/prop_format.rs`, `mcp/tests/protocol_props.rs`). Extend these when you add an importer, a parser or a protocol method. - **Everything is a command.** User-visible behaviour = a command in `crates/engine/src/cmd/*` (id, label, menu path, shortcut, params doc, `enabled`, `run`) + tests. Tools emit commands (Begin/Preview/Commit). UI-only commands live in `crates/ui-egui/src/menus.rs` (`UI_COMMANDS`). The control channel and MCP reach all of them. - **Layering** is enforced by `cargo xtask layers`. Nothing below L6 depends on egui/eframe/winit/rfd. - **The UI is thin**: panels read engine state and act through `app.run(id, params)`. Colours come from `theme::Tokens`. - **Rust only** (no handwritten JS/TS). **Never break wasm** (`cargo xtask wasm`). -- **Quality gates** before every commit: `cargo xtask ci` (fmt, clippy -D warnings, tests, layers, wasm). One task id per commit (`M2.1: pen tool`). +- **Quality gates** before every commit: `cargo xtask ci` (fmt, clippy -D warnings including the no-panic lints, tests, layers, wasm). One task id per commit (`M2.1: pen tool`). ## Running and looking at the app - `cargo run --release -p vectorcraft -- --control 7979 [file.svg|file.vectorcraft]` (sibling apps' agents use the same default port: if the log says it failed to bind, pick another port — otherwise your requests reach a different app). diff --git a/CLAUDE.md b/CLAUDE.md index d4e7cd6a..d8c591a7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -24,11 +24,19 @@ Formerly **DrawCraft** (renamed 2026-10-01): old `.drawcraft` files and `"format - Code and original assets are MIT OR Apache-2.0 (`LICENSE-MIT`, `LICENSE-APACHE`, `NOTICE`). The ArtCraft logos in `docs/brand/` are trademarks, not open source, under `docs/brand/LICENSE-brand.txt`: never modify them or use them outside VectorCraft. - Prefer art generated in code for defaults (swatches, brushes, symbols, patterns, cursors). If the provenance of an asset is unclear, don't add it. - Reference screenshots of Illustrator stay local under the gitignored `plan/` and are never committed, published or used as assets. +- **Never crash: no panics in shipped code.** A crash loses the user's unsaved work. Files, pasted data, MCP and control messages, and command params are all untrusted input. + - Fallible work returns `Result` (or `Option`); callers handle it or propagate it with `?`. Failures reach the user as an error message (`EngineError` for commands, the crate's own error type or `String` below the engine), never as an abort. + - Workspace clippy lints ban these outside tests, so `cargo xtask ci` fails on them: `unwrap()`, `expect()`, `panic!`, `unreachable!`, `todo!`, `unimplemented!`. Use instead: `?`, `.ok_or(…)?`, `let … else { return … }`, `if let`, `unwrap_or` / `unwrap_or_default` / `unwrap_or_else`, `.first()` / `.last()` / `.split_last()`, and a real error for a match arm you believe is impossible. + - Don't panic implicitly either: use `.get(i)` instead of `v[i]` or `&s[a..b]` when the index comes from data. Don't divide integers by something that can be zero, cap sizes and counts read from input (allocations, images, loops), and check that floats are finite before casting them or looping on them. + - Don't silence errors just to satisfy the lint: use `let _ =` or `.ok()` only where ignoring the failure is the right behaviour, with a comment saying why. + - Tests may unwrap and panic (`#[test]`, `#[cfg(test)]`, `tests/`, `testkit`). Integration-test files start with an `#![allow(…)]` for that. + - `vectorcraft_engine::guard` is the last line of defence. Every entry point (commands, tool events, MCP requests, the UI frame, control requests) catches a panic, rolls the document back and reports an internal error. It exists for bugs and doesn't replace `Result`: wasm aborts on panic, so the web app has no safety net. + - Untrusted input is fuzzed (`engine/tests/import_fuzz.rs`, `engine/tests/command_sweep.rs`, `format/tests/prop_format.rs`, `mcp/tests/protocol_props.rs`). Extend these when you add an importer, a parser or a protocol method. - **Everything is a command.** User-visible behaviour = a command in `crates/engine/src/cmd/*` (id, label, menu path, shortcut, params doc, `enabled`, `run`) + tests. Tools emit commands (Begin/Preview/Commit). UI-only commands live in `crates/ui-egui/src/menus.rs` (`UI_COMMANDS`). The control channel and MCP reach all of them. - **Layering** is enforced by `cargo xtask layers`. Nothing below L6 depends on egui/eframe/winit/rfd. - **The UI is thin**: panels read engine state and act through `app.run(id, params)`. Colours come from `theme::Tokens`. - **Rust only** (no handwritten JS/TS). **Never break wasm** (`cargo xtask wasm`). -- **Quality gates** before every commit: `cargo xtask ci` (fmt, clippy -D warnings, tests, layers, wasm). One task id per commit (`M2.1: pen tool`). +- **Quality gates** before every commit: `cargo xtask ci` (fmt, clippy -D warnings including the no-panic lints, tests, layers, wasm). One task id per commit (`M2.1: pen tool`). ## Running and looking at the app - `cargo run --release -p vectorcraft -- --control 7979 [file.svg|file.vectorcraft]` (sibling apps' agents use the same default port: if the log says it failed to bind, pick another port — otherwise your requests reach a different app). diff --git a/Cargo.toml b/Cargo.toml index 30a0ec15..2209c307 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -63,7 +63,14 @@ unsafe_code = "deny" [workspace.lints.clippy] dbg_macro = "warn" -todo = "warn" +# VectorCraft never crashes: shipped code returns `Result`/`Option` instead of panicking (see +# AGENTS.md › Robustness). Tests may unwrap (clippy.toml). +unwrap_used = "deny" +expect_used = "deny" +panic = "deny" +todo = "deny" +unimplemented = "deny" +unreachable = "deny" [profile.dev.package."*"] opt-level = 2 diff --git a/README.md b/README.md index 672e5030..023bd839 100644 --- a/README.md +++ b/README.md @@ -173,8 +173,9 @@ milestones, and honest time-to-parity estimates. and `apps/{vectorcraft, vectorcraft-cli, vectorcraft-web}`. The egui frontend is its own crate, so the UI can be swapped without touching the engine. -Agent and contributor rules (clean-room, the asset policy, quality gates) are in -[`AGENTS.md`](AGENTS.md). Every bundled asset is listed with its licence in [`ASSETS.md`](ASSETS.md). +Agent and contributor rules (clean-room, the asset policy, no panics in shipped code, quality gates) are in +[`AGENTS.md`](AGENTS.md); [`docs/development.md`](docs/development.md#robustness-vectorcraft-never-crashes) +explains how VectorCraft avoids crashing. Every bundled asset is listed with its licence in [`ASSETS.md`](ASSETS.md). ## The Crafting Apps diff --git a/apps/vectorcraft-cli/src/perf.rs b/apps/vectorcraft-cli/src/perf.rs index 3ab22a11..2f4e5c11 100644 --- a/apps/vectorcraft-cli/src/perf.rs +++ b/apps/vectorcraft-cli/src/perf.rs @@ -23,9 +23,9 @@ impl Rng { } /// `n` mixed paths (ellipses with strokes, translucent rectangles, stroked stars) on a 1600×1200 page. -pub fn synthetic(n: usize) -> Document { +pub fn synthetic(n: usize) -> Result { let mut d = Document::new(1600.0, 1200.0); - let l = d.layers[0].id; + let l = d.layers.first().map(|l| l.id); let mut r = Rng(42); for i in 0..n { let (x, y, s) = (r.next() * 1600.0, r.next() * 1200.0, 3.0 + r.next() * 22.0); @@ -43,9 +43,9 @@ pub fn synthetic(n: usize) -> Document { if i % 3 == 1 { node.opacity = 0.8; } - d.insert(Some(l), usize::MAX, node).expect("insert"); + d.insert(l, usize::MAX, node).map_err(|e| format!("building the synthetic document: {e}"))?; } - d + Ok(d) } fn median_ms(runs: usize, mut f: impl FnMut()) -> f64 { @@ -57,7 +57,7 @@ fn median_ms(runs: usize, mut f: impl FnMut()) -> f64 { }) .collect(); v.sort_by(f64::total_cmp); - v[v.len() / 2] + v.get(v.len() / 2).copied().unwrap_or(0.0) } /// 1-minute load average (macOS / Linux), if available. @@ -87,7 +87,7 @@ pub fn run(args: &[String]) -> Result<(), String> { println!("WARNING: the machine is busy; wall-clock timings below are not trustworthy."); } let t = Instant::now(); - let doc = synthetic(n); + let doc = synthetic(n)?; println!(" (built in {:.0} ms)", t.elapsed().as_secs_f64() * 1000.0); let mut rows: Vec<(&str, f64, f64)> = vec![]; @@ -123,18 +123,31 @@ pub fn run(args: &[String]) -> Result<(), String> { let mut bytes = vec![]; rows.push(("save .vectorcraft", median_ms(3, || bytes = vectorcraft_format::save_file(&doc)), 300.0)); - rows.push(("open .vectorcraft", median_ms(3, || drop(vectorcraft_format::load(&bytes).expect("load"))), 300.0)); + let mut failed = None; + let open = median_ms(3, || { + if let Err(e) = vectorcraft_format::load(&bytes) { + failed = Some(format!("open .vectorcraft: {e}")); + } + }); + rows.push(("open .vectorcraft", open, 300.0)); let svg_opts = vectorcraft_svg::ExportOptions::default(); rows.push(("export SVG", median_ms(3, || drop(vectorcraft_svg::export(&doc, &svg_opts))), 500.0)); // Pathfinder Unite on 1,000 overlapping paths. let unite = median_ms(3, || { let mut s = vectorcraft_engine::Session::new(); - s.add_document(synthetic(1000), None); - s.execute("select.all", &json!({})).expect("select"); - s.execute("object.pathfinder.unite", &json!({})).expect("unite"); + let r = synthetic(1000).and_then(|d| { + s.add_document(d, None); + s.execute("select.all", &json!({})).and_then(|_| s.execute("object.pathfinder.unite", &json!({}))).map_err(|e| e.to_string()) + }); + if let Err(e) = r { + failed = Some(format!("Pathfinder Unite: {e}")); + } }); rows.push(("Pathfinder Unite, 1,000 paths", unite, 150.0)); + if let Some(e) = failed { + return Err(e); + } let mut over = 0; println!(" {:<40} {:>10} {:>10}", "", "measured", "budget"); diff --git a/apps/vectorcraft-cli/tests/cli.rs b/apps/vectorcraft-cli/tests/cli.rs index 0bd7cafd..398f2880 100644 --- a/apps/vectorcraft-cli/tests/cli.rs +++ b/apps/vectorcraft-cli/tests/cli.rs @@ -1,3 +1,5 @@ +// Integration tests: unwrapping and panicking on failure is fine here, unlike in shipped code (AGENTS.md › Robustness). +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] use std::io::Write; use std::process::{Command, Stdio}; diff --git a/apps/vectorcraft-cli/tests/mcp_e2e.rs b/apps/vectorcraft-cli/tests/mcp_e2e.rs index 5a353070..e75ef094 100644 --- a/apps/vectorcraft-cli/tests/mcp_e2e.rs +++ b/apps/vectorcraft-cli/tests/mcp_e2e.rs @@ -1,4 +1,6 @@ //! End-to-end MCP over stdio against the built `vectorcraft-cli mcp --headless` binary. +// Integration tests: unwrapping and panicking on failure is fine here, unlike in shipped code (AGENTS.md › Robustness). +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] use std::io::{BufRead, BufReader, Write}; use std::process::{Child, ChildStdin, ChildStdout, Command, Stdio}; diff --git a/apps/vectorcraft/build.rs b/apps/vectorcraft/build.rs index 931c49e4..71e104fc 100644 --- a/apps/vectorcraft/build.rs +++ b/apps/vectorcraft/build.rs @@ -4,12 +4,12 @@ //! cross-compile from macOS or Linux still links, unless `VECTORCRAFT_REQUIRE_WINRES=1` turns it into //! an error (for release builds). -fn main() { +fn main() -> Result<(), String> { println!("cargo:rerun-if-changed=build.rs"); println!("cargo:rerun-if-changed=../../assets/app-icon/vectorcraft.ico"); println!("cargo:rerun-if-env-changed=VECTORCRAFT_REQUIRE_WINRES"); if std::env::var("CARGO_CFG_TARGET_OS").as_deref() != Ok("windows") { - return; + return Ok(()); } let mut res = winresource::WindowsResource::new(); res.set_icon("../../assets/app-icon/vectorcraft.ico") @@ -21,8 +21,9 @@ fn main() { .set("InternalName", "vectorcraft"); if let Err(e) = res.compile() { if std::env::var_os("VECTORCRAFT_REQUIRE_WINRES").is_some() { - panic!("embedding Windows resources failed: {e}"); + return Err(format!("embedding Windows resources failed: {e}")); } println!("cargo:warning=vectorcraft.exe built without icon/version resources: {e}"); } + Ok(()) } diff --git a/clippy.toml b/clippy.toml new file mode 100644 index 00000000..0369326b --- /dev/null +++ b/clippy.toml @@ -0,0 +1,5 @@ +# Tests may unwrap, expect and panic: a failing assertion is the point of a test. Shipped code may +# not (workspace lints in Cargo.toml; AGENTS.md › Robustness). +allow-unwrap-in-tests = true +allow-expect-in-tests = true +allow-panic-in-tests = true diff --git a/crates/brush/src/calli.rs b/crates/brush/src/calli.rs index a5163893..f73c4933 100644 --- a/crates/brush/src/calli.rs +++ b/crates/brush/src/calli.rs @@ -83,10 +83,9 @@ pub(crate) fn sweep(t: &Track, nib: &Nib) -> Vec { }; let half = std::f64::consts::FRAC_PI_2; let mut poly = left.clone(); - let (pe, te) = *s.last().unwrap(); + let (Some(&(pe, te)), Some(&(ps, ts))) = (s.last(), s.first()) else { return Vec::new() }; cap(pe, te, half, -half, &mut poly); poly.extend(right.iter().rev()); - let (ps, ts) = s[0]; cap(ps, ts, -half, -3.0 * half, &mut poly); vec![SubPath::polyline(&poly, true)] } diff --git a/crates/brush/src/lib.rs b/crates/brush/src/lib.rs index 183fadb5..08d86f15 100644 --- a/crates/brush/src/lib.rs +++ b/crates/brush/src/lib.rs @@ -325,7 +325,11 @@ pub fn unique_name(lib: &[Brush], base: &str) -> String { if !lib.iter().any(|b| b.name == base) { return base.to_string(); } - (2..).map(|i| format!("{base} {i}")).find(|n| !lib.iter().any(|b| &b.name == n)).unwrap() + // At most `lib.len()` names are taken, so one of the first `lib.len() + 1` candidates is free. + (2..=lib.len() + 2) + .map(|i| format!("{base} {i}")) + .find(|n| !lib.iter().any(|b| &b.name == n)) + .unwrap_or_else(|| format!("{base} {}", lib.len() + 2)) } // ---------- geometry ---------- diff --git a/crates/brush/src/tests.rs b/crates/brush/src/tests.rs index cc522ac8..2382d984 100644 --- a/crates/brush/src/tests.rs +++ b/crates/brush/src/tests.rs @@ -267,7 +267,7 @@ fn bristle_makes_translucent_strands() { #[test] fn tints_colorization_uses_stroke_colour() { let b = art_brush(rect_art(10.0, 4.0), ArtScale::Stretch); - let BrushKind::Art(mut a) = b.kind else { unreachable!() }; + let BrushKind::Art(mut a) = b.kind else { panic!("not an art brush") }; a.colorization = Colorization::Tints; let b = Brush { name: "t".into(), kind: BrushKind::Art(a) }; let out = stroke_pieces(&b, &line(0.0, 0.0, 50.0, 0.0), &stroke(1.0)); diff --git a/crates/brush/src/track.rs b/crates/brush/src/track.rs index ead92736..e02bcf48 100644 --- a/crates/brush/src/track.rs +++ b/crates/brush/src/track.rs @@ -76,11 +76,11 @@ impl Track { corner.push(a.dot(b) < cos_limit); tangents.push(unit(a + b)); } - (Some(a), None) | (None, Some(a)) => { + (a, b) => { + // An end of an open track (both `None` only for a track without segments). corner.push(false); - tangents.push(a); + tangents.push(a.or(b).unwrap_or(Vec2::new(1.0, 0.0))); } - (None, None) => unreachable!(), } } Some(Track { verts: pts, cum, closed, dirs, tangents, corner }) diff --git a/crates/color/src/gradient.rs b/crates/color/src/gradient.rs index f793a9d4..c28ac48f 100644 --- a/crates/color/src/gradient.rs +++ b/crates/color/src/gradient.rs @@ -110,8 +110,10 @@ impl Gradient { return (mix(&a.color, &b.color, v), a.opacity + (b.opacity - a.opacity) * v); } } - let l = stops.last().unwrap(); - (l.color, l.opacity) + match stops.last() { + Some(l) => (l.color, l.opacity), + None => (Color::BLACK, 1.0), + } } /// Stops expanded so that midpoints are represented as explicit stops (for renderers without midpoints). pub fn expanded_stops(&self) -> Vec<(f32, Color, f32)> { diff --git a/crates/color/src/swatch.rs b/crates/color/src/swatch.rs index 3cc6b463..21f9af98 100644 --- a/crates/color/src/swatch.rs +++ b/crates/color/src/swatch.rs @@ -48,7 +48,7 @@ pub struct SwatchGroup { } fn solid(name: &str, hex: &str) -> Swatch { - Swatch { name: name.into(), paint: Paint::solid(Color::from_hex(hex).unwrap()), global: false, spot: false } + Swatch { name: name.into(), paint: Paint::solid(Color::from_hex(hex).unwrap_or(Color::BLACK)), global: false, spot: false } } /// The default swatches of a document in colour `model`: [`base_swatches`] with their RGB colours @@ -125,7 +125,10 @@ fn base_swatches() -> (Vec, Vec) { name: name.into(), paint: Paint::Gradient(Box::new(crate::GradientPaint::new(Gradient { kind, - stops: vec![GradientStop::new(0.0, Color::from_hex(a).unwrap()), GradientStop::new(1.0, Color::from_hex(b).unwrap())], + stops: vec![ + GradientStop::new(0.0, Color::from_hex(a).unwrap_or(Color::WHITE)), + GradientStop::new(1.0, Color::from_hex(b).unwrap_or(Color::BLACK)), + ], }))), global: false, spot: false, @@ -180,4 +183,16 @@ mod tests { let black = s.iter().find(|w| w.name == "Black").and_then(|w| w.paint.color()); assert_eq!(black, Some(Color::cmyk(0.0, 0.0, 0.0, 1.0))); } + + #[test] + fn default_swatches_parse() { + // `solid`/`grad` fall back to black or white on a bad literal instead of panicking; make sure + // none falls back. + let src = include_str!("swatch.rs"); + let hexes: Vec<&str> = src.split('"').filter(|t| t.starts_with('#') && t.len() == 7).collect(); + assert!(hexes.len() > 30); + for h in hexes { + assert!(Color::from_hex(h).is_some(), "{h}"); + } + } } diff --git a/crates/doc/src/appearance.rs b/crates/doc/src/appearance.rs index 48e6d47f..c697aa81 100644 --- a/crates/doc/src/appearance.rs +++ b/crates/doc/src/appearance.rs @@ -241,8 +241,7 @@ impl WidthProfile { return (w[0].1 + (w[1].1 - w[0].1) * u, w[0].2 + (w[1].2 - w[0].2) * u); } } - let l = p.last().unwrap(); - (l.1, l.2) + p.last().map_or((1.0, 1.0), |l| (l.1, l.2)) } /// The built-in profiles, in menu order. "uniform" is the plain stroke (no profile). pub const PRESETS: [ProfilePreset; 7] = [ diff --git a/crates/doc/src/lib.rs b/crates/doc/src/lib.rs index 07d1dfed..194f294f 100644 --- a/crates/doc/src/lib.rs +++ b/crates/doc/src/lib.rs @@ -615,8 +615,11 @@ impl Document { pub fn remove(&mut self, id: NodeId) -> Result, DocError> { let parent = self.parent_of(id); let path = self.index_path(id).ok_or(DocError::NoNode(id))?; - let idx = *path.last().unwrap(); + let idx = *path.last().ok_or(DocError::NoNode(id))?; let ch = self.children_mut(parent)?; + if idx >= ch.len() { + return Err(DocError::NoNode(id)); + } Ok(ch.remove(idx)) } @@ -719,8 +722,7 @@ impl Document { n.id = self.alloc_id(); if let Some(ch) = n.children_mut() { let old: Vec> = std::mem::take(ch); - let fresh: Vec> = old.iter().map(|c| Arc::new(self.reid(c))).collect(); - *n.children_mut().unwrap() = fresh; + *ch = old.iter().map(|c| Arc::new(self.reid(c))).collect(); } n } @@ -781,7 +783,7 @@ impl Document { fn default_graphic_styles() -> Vec { use vectorcraft_color::{Color, Paint}; - let solid = |hex| Paint::solid(Color::from_hex(hex).unwrap()); + let solid = |hex| Paint::solid(Color::from_hex(hex).unwrap_or(Color::BLACK)); [ GraphicStyle::new(DEFAULT_GRAPHIC_STYLE, Appearance::default_art()), GraphicStyle::new("Black Outline", Appearance::basic(Paint::None, Paint::solid(Color::BLACK), 1.0)), diff --git a/crates/doc/src/live.rs b/crates/doc/src/live.rs index 92d7ee01..fe0ee33d 100644 --- a/crates/doc/src/live.rs +++ b/crates/doc/src/live.rs @@ -466,11 +466,7 @@ impl Spine { if pts.len() < 2 { return None; } - let mut cum = vec![0.0]; - for w in pts.windows(2) { - cum.push(cum.last().unwrap() + w[0].distance(w[1])); - } - Some(Self { pts, cum }) + Some(Self { cum: cumulative_lengths(&pts), pts }) } pub fn length(&self) -> f64 { *self.cum.last().unwrap_or(&0.0) @@ -639,6 +635,18 @@ fn poly_len(c: &CubicBez) -> f64 { c.p0.distance(c.p1) + c.p1.distance(c.p2) + c.p2.distance(c.p3) } +/// Running length along a polyline: `0, |p0p1|, |p0p1| + |p1p2|, …` (one entry per point). +fn cumulative_lengths(pts: &[Point]) -> Vec { + let mut total = 0.0; + let mut cum = Vec::with_capacity(pts.len()); + cum.push(0.0); + for w in pts.windows(2) { + total += w[0].distance(w[1]); + cum.push(total); + } + cum +} + /// Map every point of `path` through the non-linear function `f`. Segments are split into pieces /// no longer than about `max_piece` (up to 64 per segment) and their control points mapped, which /// approximates the image curve to O(h²). Closedness and subpath structure are preserved. @@ -662,7 +670,9 @@ pub fn map_nonlinear(path: &PathData, max_piece: f64, f: impl Fn(Point) -> Point let k = ((poly_len(&c) / max_piece).ceil() as usize).clamp(1, 64); for j in 0..k { let sub = c.subsegment((j as f64 / k as f64)..((j + 1) as f64 / k as f64)); - res.last_mut().unwrap().h_out = f(sub.p1); + if let Some(last) = res.last_mut() { + last.h_out = f(sub.p1); + } if j + 1 < k { res.push(Anchor { p: f(sub.p3), h_in: f(sub.p2), h_out: f(sub.p3), kind: Default::default() }); } else { @@ -673,8 +683,9 @@ pub fn map_nonlinear(path: &PathData, max_piece: f64, f: impl Fn(Point) -> Point } if let Some(h) = pending_in && sp.closed + && let Some(first) = res.first_mut() { - res[0].h_in = h; + first.h_in = h; } let anchors = res.into_iter().map(|a| Anchor::with_handles(a.p, a.h_in, a.h_out)).collect(); subs.push(SubPath::new(anchors, sp.closed)); @@ -704,11 +715,8 @@ fn resample(poly: &[Point], m: usize) -> Vec { if poly.len() == 1 { return vec![poly[0]; m]; } - let mut cum = vec![0.0]; - for w in poly.windows(2) { - cum.push(cum.last().unwrap() + w[0].distance(w[1])); - } - let total = *cum.last().unwrap(); + let cum = cumulative_lengths(poly); + let total = cum.last().copied().unwrap_or(0.0); let mut out = Vec::with_capacity(m); let mut i = 0; for k in 0..m { @@ -749,7 +757,10 @@ impl Coons { let r = path.bounds()?; let tol = (r.width() + r.height()).max(1e-6) / 2000.0; let mut poly = flatten_subpath(sp, tol); - if poly.len() > 1 && poly[0].distance(*poly.last().unwrap()) < 1e-9 { + if poly.len() > 1 + && let (Some(a), Some(b)) = (poly.first(), poly.last()) + && a.distance(*b) < 1e-9 + { poly.pop(); } if poly.len() < 3 { diff --git a/crates/effects/src/group.rs b/crates/effects/src/group.rs index 3b3978aa..1a45d4a8 100644 --- a/crates/effects/src/group.rs +++ b/crates/effects/src/group.rs @@ -112,8 +112,11 @@ pub fn pathfinder_children(group: &Node, hook: Option) -> Option 1 { let parts: Vec<(PathData, FillRule)> = lv.iter().filter_map(geometry).collect(); let refs: Vec<(&PathData, FillRule)> = parts.iter().map(|(p, r)| (p, *r)).collect(); - shapes.push(po::Shape::new(po::unite_all(&refs), FillRule::NonZero, sources.len() as u64)); - sources.push(lv.pop().expect("non-empty")); + let key = sources.len() as u64; + if let Some(top) = lv.pop() { + shapes.push(po::Shape::new(po::unite_all(&refs), FillRule::NonZero, key)); + sources.push(top); + } } else { for l in lv { if let Some((p, r)) = geometry(&l) { diff --git a/crates/engine/src/cmd/brushsym.rs b/crates/engine/src/cmd/brushsym.rs index b31afdf0..db379953 100644 --- a/crates/engine/src/cmd/brushsym.rs +++ b/crates/engine/src/cmd/brushsym.rs @@ -248,11 +248,12 @@ fn brush_apply(s: &mut Session, p: &Value) -> Result { if n.appearance.stroke().is_none_or(|st| st.paint.is_none()) { n.appearance.set_stroke(stroke.clone()); } - let st = n.appearance.stroke_mut().expect("stroke"); - if st.width <= 0.0 { - st.width = 1.0; + if let Some(st) = n.appearance.stroke_mut() { + if st.width <= 0.0 { + st.width = 1.0; + } + st.brush = Some(name.clone()); } - st.brush = Some(name.clone()); } Ok(()) })?; @@ -515,7 +516,7 @@ fn unique_symbol_name(d: &Document, base: &str) -> String { if !taken(base) { return base.to_string(); } - (2..).map(|i| format!("{base} {i}")).find(|n| !taken(n)).unwrap() + (2..).map(|i| format!("{base} {i}")).find(|n| !taken(n)).unwrap_or_else(|| base.to_string()) } fn instances_of(d: &Document, name: Option<&str>) -> Vec { @@ -676,7 +677,7 @@ fn symbol_edit(s: &mut Session, p: &Value) -> Result { let first = *ids.first().ok_or_else(|| EngineError::Other("select a symbol instance".into()))?; let name = match &s.doc()?.doc.node(first).map(|n| n.kind.clone()) { Some(NodeKind::SymbolInstance { symbol, .. }) => symbol.clone(), - _ => unreachable!(), + _ => return Err(EngineError::Other("select a symbol instance".into())), }; let out = s.edit("Edit Symbol", |d, sel| { let out = break_links(d, &[first])?; diff --git a/crates/engine/src/cmd/buildcmds.rs b/crates/engine/src/cmd/buildcmds.rs index afb2d91d..12c4f491 100644 --- a/crates/engine/src/cmd/buildcmds.rs +++ b/crates/engine/src/cmd/buildcmds.rs @@ -308,7 +308,7 @@ fn sb_merge(s: &mut Session, p: &Value) -> Result { new_nodes.push(shape_node(d, rest, Some(leaf))); } } - if !erase && i == style_src { + if !erase && Some(i) == style_src { let mut m = shape_node(d, union.clone(), Some(leaf)); if let Some(f) = &fill { m.appearance.set_fill(f.clone()); @@ -399,7 +399,7 @@ fn build_children(d: &mut Document, sources: Vec>) -> Result<(Vec Result { let label = if expand { "Image Trace (Make and Expand)" } else { "Image Trace" }; let id = s.edit(label, |d, sel| { let src = d.node(target).cloned().ok_or(EngineError::NoNode(target))?; - let mut image = if is_trace_group(&src) { (**src.children().unwrap().first().unwrap()).clone() } else { src.clone() }; + let mut image = if is_trace_group(&src) { + src.children() + .and_then(|c| c.first()) + .map(|c| (**c).clone()) + .ok_or_else(|| EngineError::Other("Image Trace: the traced image is missing".into()))? + } else { + src.clone() + }; image.visible = true; let mut children: Vec> = vec![]; if !expand { diff --git a/crates/engine/src/cmd/colorcmds.rs b/crates/engine/src/cmd/colorcmds.rs index 818ef40a..9536d00f 100644 --- a/crates/engine/src/cmd/colorcmds.rs +++ b/crates/engine/src/cmd/colorcmds.rs @@ -131,6 +131,15 @@ pub(crate) fn to_gray(c: Color) -> Color { c.in_model(Model::Gray) } +/// Grayscale ink amount (0 = white, 1 = black) of a colour. +fn gray_level(c: Color) -> f32 { + match to_gray(c) { + Color::Gray { k } => k, + // `in_model(Gray)` always yields Gray. + _ => 0.0, + } +} + /// What a recolouring pass applies to a colour that can link to a swatch (a solid colour or a /// gradient stop): `(colour, link, tint)`, true when it changed them. pub(crate) type LinkMap<'a> = dyn Fn(&mut Color, &mut Option, &mut f32) -> bool + 'a; @@ -416,10 +425,7 @@ fn adjust_balance(s: &mut Session, p: &Value) -> Result { } "gray" => { let dgray = g("gray"); - Box::new(move |c| { - let Color::Gray { k } = to_gray(c) else { unreachable!() }; - Color::gray(cl(k + dgray)) - }) + Box::new(move |c| Color::gray(cl(gray_level(c) + dgray))) } "global" => { // Each linked colour moves to the shifted tint of its global swatch's current colour. @@ -506,8 +512,8 @@ fn blend(s: &mut Session, order: BlendOrder) -> Result { BlendOrder::Horizontal => ids.sort_by(|a, b| center(a).x.total_cmp(¢er(b).x)), BlendOrder::Vertical => ids.sort_by(|a, b| center(a).y.total_cmp(¢er(b).y)), } - let first = d.node(ids[0]).and_then(fill_color).unwrap_or_default(); - let last = d.node(*ids.last().unwrap()).and_then(fill_color).unwrap_or_default(); + let first = ids.first().and_then(|id| d.node(*id)).and_then(fill_color).unwrap_or_default(); + let last = ids.last().and_then(|id| d.node(*id)).and_then(fill_color).unwrap_or_default(); // Ends in one model blend in it; mixed ones give colours in the document's model. let model = if first.model() == last.model() { first.model() } else { d.color_mode.model() }; let n = ids.len(); diff --git a/crates/engine/src/cmd/distortcmds.rs b/crates/engine/src/cmd/distortcmds.rs index 270cada2..faf9a1c5 100644 --- a/crates/engine/src/cmd/distortcmds.rs +++ b/crates/engine/src/cmd/distortcmds.rs @@ -356,7 +356,8 @@ fn liquify(s: &mut Session, p: &Value) -> Result { let doc = &s.doc()?.doc; let leaves = if roots.is_empty() { // Nothing selected: everything the brush sweeps over. - let sweep = dab_pts.iter().map(|c| prm.brush_bounds(*c)).reduce(|a, b| a.union(b)).unwrap(); + // No dabs sweep nothing. + let sweep = dab_pts.iter().map(|c| prm.brush_bounds(*c)).reduce(|a, b| a.union(b)).unwrap_or(vectorcraft_geom::Rect::ZERO); let mut all = vec![]; doc.walk(|n| { if matches!(n.kind, NodeKind::Path { guide: false, .. }) diff --git a/crates/engine/src/cmd/draw2.rs b/crates/engine/src/cmd/draw2.rs index fb12148c..75e48955 100644 --- a/crates/engine/src/cmd/draw2.rs +++ b/crates/engine/src/cmd/draw2.rs @@ -347,12 +347,13 @@ fn cut_subpath(sp: &SubPath, remove: &dyn Fn(Point) -> bool) -> Option= 2 { - let first_starts = runs[0][0].0 == 0 && runs[0][0].1 == 0.0; - let last = runs.last().unwrap(); - let last_ends = last[last.len() - 1].0 == nseg - 1 && last[last.len() - 1].2 == 1.0; + let first_starts = runs.first().and_then(|r| r.first()).is_some_and(|s| s.0 == 0 && s.1 == 0.0); + let last_ends = runs.last().and_then(|r| r.last()).is_some_and(|s| s.0 + 1 == nseg && s.2 == 1.0); if first_starts && last_ends { let first = runs.remove(0); - runs.last_mut().unwrap().extend(first); + if let Some(last) = runs.last_mut() { + last.extend(first); + } } } let out = runs @@ -363,10 +364,10 @@ fn cut_subpath(sp: &SubPath, remove: &dyn Fn(Point) -> bool) -> Option a.h_out = h1, + None => anchors.push(Anchor { h_out: h1, ..Anchor::corner(c.p0) }), } - anchors.last_mut().unwrap().h_out = h1; anchors.push(Anchor { p: c.p3, h_in: h2, h_out: c.p3, kind: AnchorKind::Corner }); } for a in &mut anchors { @@ -419,16 +420,20 @@ fn path_kind(path: PathData) -> NodeKind { /// corners) and simplify it to `tol`. fn fit_freehand(pts: &[Point], tol: f64, closed: bool) -> SubPath { let min = (tol * 0.3).max(0.05); - let mut kept: Vec = vec![pts[0]]; - for &p in &pts[1..] { - if kept.last().unwrap().distance(p) >= min { + let (Some(&p0), Some(&pn)) = (pts.first(), pts.last()) else { return SubPath::new(vec![], closed) }; + let mut kept: Vec = vec![p0]; + for &p in pts.iter().skip(1) { + if kept.last().is_none_or(|k| k.distance(p) >= min) { kept.push(p); } } - if let (Some(&l), true) = (pts.last(), kept.len() > 1) { - *kept.last_mut().unwrap() = l; - } else if kept.len() == 1 && pts.len() > 1 { - kept.push(pts[pts.len() - 1]); + // The last sample always ends the stroke. + if kept.len() > 1 { + if let Some(k) = kept.last_mut() { + *k = pn; + } + } else if pts.len() > 1 { + kept.push(pn); } let n = kept.len(); let marked: Vec<(Point, bool)> = (0..n) @@ -465,25 +470,34 @@ fn freehand(s: &mut Session, p: &Value) -> Result { if at_start { sp.reverse(); } - let end = sp.anchors.last().unwrap().p; - pts[0] = end; + let (Some(first), Some(end)) = (sp.anchors.first().map(|a| a.p), sp.anchors.last().map(|a| a.p)) else { + return Err(bad(C, "path has no open end")); + }; + let Some(p0) = pts.first_mut() else { return Err(bad(C, "needs points")) }; + *p0 = end; if pts.len() < 2 { return Ok(json!({ "id": id.0 })); } - let first = sp.anchors[0].p; let close_tol = (tol * 4.0).max(6.0); - let closing = sp.anchors.len() >= 2 && pts.last().unwrap().distance(first) <= close_tol; - if closing { - *pts.last_mut().unwrap() = first; + let closing = sp.anchors.len() >= 2 && pts.last().is_some_and(|p| p.distance(first) <= close_tol); + if closing && let Some(l) = pts.last_mut() { + *l = first; } let fit = fit_freehand(&pts, tol, false); let mut add = fit.anchors; - sp.anchors.last_mut().unwrap().h_out = add[0].h_out; - add.remove(0); + if let (Some(last), Some(a0)) = (sp.anchors.last_mut(), add.first()) { + last.h_out = a0.h_out; + } + if !add.is_empty() { + add.remove(0); + } sp.anchors.extend(add); - if closing && sp.anchors.len() > 2 { - let l = sp.anchors.pop().unwrap(); - sp.anchors[0].h_in = l.h_in; + if closing + && sp.anchors.len() > 2 + && let Some(l) = sp.anchors.pop() + && let Some(a0) = sp.anchors.first_mut() + { + a0.h_in = l.h_in; sp.closed = true; } for a in &mut sp.anchors { @@ -900,8 +914,10 @@ fn smooth_subpath(sp: &SubPath, near: &dyn Fn(Point) -> bool, tol: f64) -> Optio m.push(false); open.closed = false; let mut res = smooth_open(&open.anchors, &m, tol); - let l = res.pop().unwrap(); - res[0].h_in = l.h_in; + let l = res.pop()?; + if let Some(r0) = res.first_mut() { + r0.h_in = l.h_in; + } return Some(SubPath::new(res, true)); } Some(SubPath::new(smooth_open(&sp.anchors, &marked, tol), false)) @@ -1029,8 +1045,10 @@ fn join_scrub(s: &mut Session, p: &Value) -> Result { if ida == idb && sa == sb { let path = path_mut(d, ida)?; let sp = &mut path.subpaths[sa]; - if merge && sp.anchors.len() > 2 { - let l = sp.anchors.pop().unwrap(); + if merge + && sp.anchors.len() > 2 + && let Some(l) = sp.anchors.pop() + { let d0 = mid - sp.anchors[0].p; sp.anchors[0].translate(d0); sp.anchors[0].h_in = l.h_in + (mid - l.p); @@ -1048,10 +1066,12 @@ fn join_scrub(s: &mut Session, p: &Value) -> Result { if !startb { y.reverse(); } - if merge { + if merge + && !y.anchors.is_empty() + && let Some(last) = x.anchors.last_mut() + { let yf = y.anchors.remove(0); - let lp = x.anchors.last().unwrap().p; - let last = x.anchors.last_mut().unwrap(); + let lp = last.p; last.translate(mid - lp); last.h_out = yf.h_out + (mid - yf.p); *last = Anchor::with_handles(last.p, last.h_in, last.h_out); @@ -1117,8 +1137,9 @@ fn blob(s: &mut Session, p: &Value) -> Result { let mut inputs: Vec<(&PathData, FillRule)> = vec![(®ion, FillRule::NonZero)]; inputs.extend(merge_ids.iter().map(|(_, pd, r)| (pd, *r))); let merged = po::unite_all(&inputs); - let keep = merge_ids.last().unwrap().0; - let others: Vec = merge_ids[..merge_ids.len() - 1].iter().map(|m| m.0).collect(); + let Some(((keep, _, _), rest)) = merge_ids.split_last() else { return add_node(s, "Blob Brush", path_kind(region), ap, None) }; + let keep = *keep; + let others: Vec = rest.iter().map(|m| m.0).collect(); s.edit("Blob Brush", |d, sel| { *path_mut(d, keep)? = merged; for id in others { diff --git a/crates/engine/src/cmd/fileio/encode.rs b/crates/engine/src/cmd/fileio/encode.rs index 3de0d5f6..15666abc 100644 --- a/crates/engine/src/cmd/fileio/encode.rs +++ b/crates/engine/src/cmd/fileio/encode.rs @@ -130,6 +130,7 @@ pub fn encode(doc: &Document, format: &str, p: &Value) -> Result> { "webp" => img.to_webp(), _ => img.to_jpeg(o.quality.unwrap_or(90)), } + .map_err(EngineError::Other)? } _ => return Err(bad(C, format!("no encoder for {} yet", f.label))), }) diff --git a/crates/engine/src/cmd/flatten.rs b/crates/engine/src/cmd/flatten.rs index e2a8cf95..21789ae5 100644 --- a/crates/engine/src/cmd/flatten.rs +++ b/crates/engine/src/cmd/flatten.rs @@ -231,7 +231,7 @@ fn flatten(s: &mut Session, p: &Value) -> Result { return Err(EngineError::Other("Flatten Transparency: select objects to flatten".into())); } let src = s.doc()?.doc.clone(); - let plan = plan(&src, &roots, &o, true); + let plan = plan(&src, &roots, &o, true)?; let options = serde_json::to_value(&o).unwrap_or_default(); if plan.flat.is_empty() && !plan.kept.iter().any(|id| src.node(*id).is_some_and(|n| kept_work(n, &o))) { return Ok(json!({ "ids": roots.iter().map(|i| i.0).collect::>(), "rasterized": 0, "vector": 0, "options": options })); @@ -635,8 +635,8 @@ impl FlattenReport { } /// What flattening `roots` of `doc` with `o` would do. -pub fn report(doc: &Document, roots: &[NodeId], o: &FlattenOptions) -> FlattenReport { - let plan = plan(doc, roots, o, false); +pub fn report(doc: &Document, roots: &[NodeId], o: &FlattenOptions) -> Result { + let plan = plan(doc, roots, o, false)?; let mut r = FlattenReport { transparent: plan.transparent, ..Default::default() }; // Every object under `id` (but a compound path's own pieces), visible ones only. fn visit(n: &Node, f: &mut dyn FnMut(&Node)) { @@ -685,7 +685,7 @@ pub fn report(doc: &Document, roots: &[NodeId], o: &FlattenOptions) -> FlattenRe }); } r.affected.sort_by_key(|id| doc.index_path(*id)); - r + Ok(r) } impl Session { @@ -704,7 +704,7 @@ impl Session { let layers: Vec = self.doc()?.doc.layers.iter().map(|l| l.id.0).collect(); target_roots(self, &json!({ "ids": layers }))? }; - let r = report(&self.doc()?.doc, &roots, &o); + let r = report(&self.doc()?.doc, &roots, &o)?; Ok((o, r)) } } @@ -778,8 +778,8 @@ struct Raster { } /// The flattening of `roots` with `o`; `render` renders the images (the preview only needs where -/// they go). -fn plan(src: &Document, roots: &[NodeId], o: &FlattenOptions, render: bool) -> Plan { +/// they go). Fails only when an image can't be encoded. +fn plan(src: &Document, roots: &[NodeId], o: &FlattenOptions, render: bool) -> Result { let tmp = isolated_doc(src, roots.iter().filter_map(|id| src.node(*id).cloned()).collect()); // Geometry effects first: what is left on the art are raster effects. let baked = effects::bake_document(&tmp).unwrap_or(tmp); @@ -792,19 +792,24 @@ fn plan(src: &Document, roots: &[NodeId], o: &FlattenOptions, render: bool) -> P let mut out = Plan { flat: vec![], kept: vec![], transparent: roots.iter().zip(&see_through).filter(|(_, t)| **t).map(|(id, _)| *id).collect() }; for g in overlapping(&reaches) { let ids: Vec = g.iter().map(|&i| roots[i]).collect(); - let flat = g.iter().any(|&i| see_through[i]).then(|| { - let art: Vec = g.iter().map(|&i| art[i].clone()).collect(); + let group = g.iter().any(|&i| see_through[i]).then(|| { let plain: Vec<&Node> = g.iter().map(|&i| &plain[i]).collect(); - let (regions, area) = flatten_group(&plain, o, cmyk)?; - let raster = area.as_ref().filter(|_| render).map(|a| render_raster(&baked, &art, a.rect, a.ppi, a.clip.clone(), o)); - Some(Flat { roots: ids.clone(), regions, area, raster }) + flatten_group(&plain, o, cmyk) }); - match flat.flatten() { - Some(f) => out.flat.push(f), - None => out.kept.extend(ids), - } + let Some((regions, area)) = group.flatten() else { + out.kept.extend(ids); + continue; + }; + let raster = match area.as_ref().filter(|_| render) { + Some(a) => { + let art: Vec = g.iter().map(|&i| art[i].clone()).collect(); + Some(render_raster(&baked, &art, a.rect, a.ppi, a.clip.clone(), o)?) + } + None => None, + }; + out.flat.push(Flat { roots: ids, regions, area, raster }); } - out + Ok(out) } /// The regions and the image area replacing one group of overlapping objects (`plain`: the art as @@ -902,8 +907,8 @@ fn region_fill(elems: &[Elem], sources: &[usize], o: &FlattenOptions, cmyk: bool /// `art` rendered over `rect` at `ppi`: over white with the white the art doesn't cover taken out /// again (so blend modes see the white page), or over nothing with Preserve Alpha. The image and -/// its `clip` reach a pixel further. -fn render_raster(doc: &Document, art: &[Node], rect: Rect, ppi: f64, clip: Option, o: &FlattenOptions) -> Raster { +/// its `clip` reach a pixel further. Fails when the image can't be encoded. +fn render_raster(doc: &Document, art: &[Node], rect: Rect, ppi: f64, clip: Option, o: &FlattenOptions) -> Result { let mut scale = ppi / 72.0; scale = scale.min((MAX_PIXELS / (rect.width() * rect.height()).max(1.0)).sqrt()); let px = 1.0 / scale; @@ -952,7 +957,8 @@ fn render_raster(doc: &Document, art: &[Node], rect: Rect, ppi: f64, clip: Optio } } let xf = Affine::translate(region.origin().to_vec2()) * Affine::scale(1.0 / scale); - Raster { png: img.to_png(), width: img.width, height: img.height, xf, clip } + let png = img.to_png().map_err(|e| EngineError::Other(format!("Flatten Transparency: {e}")))?; + Ok(Raster { png, width: img.width, height: img.height, xf, clip }) } /// `n` at full opacity with Normal blending throughout (objects, fills and strokes). diff --git a/crates/engine/src/cmd/fonts.rs b/crates/engine/src/cmd/fonts.rs index 82a1365e..5403a914 100644 --- a/crates/engine/src/cmd/fonts.rs +++ b/crates/engine/src/cmd/fonts.rs @@ -126,7 +126,10 @@ fn replace(s: &mut Session, p: &Value) -> Result { for r in &mut t.runs { if matches(&r.style) { // An explicit style, else the closest the new family has to the old one. - let style = ts.clone().unwrap_or_else(|| db.face(&tf, &r.style.font_style).style.clone()); + let style = ts + .clone() + .or_else(|| db.face(&tf, &r.style.font_style).map(|f| f.style.clone())) + .unwrap_or_else(|| r.style.font_style.clone()); r.style.font_family = tf.clone(); r.style.font_style = style; changed = true; diff --git a/crates/engine/src/cmd/live.rs b/crates/engine/src/cmd/live.rs index 9a44adf2..591179c8 100644 --- a/crates/engine/src/cmd/live.rs +++ b/crates/engine/src/cmd/live.rs @@ -463,7 +463,7 @@ fn blend_replace_spine(s: &mut Session, _: &Value) -> Result { && !blends.iter().any(|b| st.doc.ancestry(*id).unwrap_or_default().contains(b)) }) .ok_or_else(|| EngineError::Other("Replace Spine: select a blend and a path".into()))?; - let b = *blends.first().unwrap(); + let Some(&b) = blends.first() else { return Err(EngineError::Other("Replace Spine: select a blend and a path".into())) }; s.edit("Replace Spine", |d, sel| { let spine = d.node(path).and_then(|n| n.path_data().cloned()).ok_or(EngineError::NoNode(path))?; d.remove(path)?; @@ -576,9 +576,9 @@ fn env_make_top(s: &mut Session, p: &Value) -> Result { if roots.len() < 2 { return Err(bad(C, "select the content and a path on top")); } - let top = *roots.last().unwrap(); + let Some((&top, content)) = roots.split_last() else { return Err(bad(C, "select the content and a path on top")) }; + let content = content.to_vec(); let path = s.doc()?.doc.node(top).and_then(outline_of).filter(|p| p.bounds().is_some()).ok_or_else(|| bad(C, "the top object must be a path"))?; - let content = roots[..roots.len() - 1].to_vec(); let id = s.edit("Make Envelope", |d, sel| { let id = wrap(d, sel, &content, |id, content| envelope(id, content, EnvelopeKind::TopObject { path }))?; d.remove(top)?; diff --git a/crates/engine/src/cmd/menucmds.rs b/crates/engine/src/cmd/menucmds.rs index 8b471661..d3fc727e 100644 --- a/crates/engine/src/cmd/menucmds.rs +++ b/crates/engine/src/cmd/menucmds.rs @@ -323,7 +323,7 @@ fn render_png(doc: &vectorcraft_doc::Document, region: Rect, scale: f64, white: } let mut r = vectorcraft_render::Renderer::new(); let img = r.render_region(doc, region, scale, white); - Ok((img.to_png(), img.width, img.height)) + Ok((img.to_png().map_err(EngineError::Other)?, img.width, img.height)) } fn rasterize(s: &mut Session, p: &Value) -> Result { @@ -370,7 +370,7 @@ fn rasterize(s: &mut Session, p: &Value) -> Result { fn has_image(s: &Session) -> std::result::Result<(), String> { has_selection(s)?; - let st = s.active().unwrap(); + let st = s.active().ok_or("no document open")?; if st.selection.objects.iter().any(|id| matches!(st.doc.node(*id).map(|n| &n.kind), Some(NodeKind::Image(_)))) { Ok(()) } else { @@ -389,7 +389,7 @@ fn object_mosaic(s: &mut Session, p: &Value) -> Result { .find(|id| matches!(st.doc.node(*id).map(|n| &n.kind), Some(NodeKind::Image(_)))) .ok_or_else(|| bad(C, "select an image"))?; let node = st.doc.node(id).cloned().ok_or(EngineError::NoNode(id))?; - let NodeKind::Image(im) = &node.kind else { unreachable!() }; + let NodeKind::Image(im) = &node.kind else { return Err(bad(C, "select an image")) }; let blob = st.doc.images.get(&im.key).filter(|b| !b.bytes.is_empty()).ok_or_else(|| bad(C, "the image has no embedded pixels"))?; let raster = vectorcraft_trace::Raster::decode(&blob.bytes).map_err(|e| bad(C, e.to_string()))?; let cols = p.get("columns").and_then(Value::as_u64).unwrap_or(10).clamp(1, 1000) as u32; @@ -464,7 +464,7 @@ fn crop_image(s: &mut Session, p: &Value) -> Result { .find(|id| matches!(st.doc.node(*id).map(|n| &n.kind), Some(NodeKind::Image(_)))) .ok_or_else(|| bad(C, "select an image"))?; let node = st.doc.node(id).cloned().ok_or(EngineError::NoNode(id))?; - let NodeKind::Image(im) = &node.kind else { unreachable!() }; + let NodeKind::Image(im) = &node.kind else { return Err(bad(C, "select an image")) }; let ib = node.geometric_bounds().ok_or_else(|| bad(C, "image has no bounds"))?; let rect = match p.get("rect").and_then(Value::as_array) { Some(a) if a.len() == 4 => { diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index 5992c65e..21adff72 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -120,12 +120,12 @@ pub fn has_doc(s: &Session) -> std::result::Result<(), String> { s.active().map(|_| ()).ok_or_else(|| "no document open".into()) } pub fn has_selection(s: &Session) -> std::result::Result<(), String> { - has_doc(s)?; - if s.active().unwrap().selection.is_empty() { Err("nothing selected".into()) } else { Ok(()) } + let st = s.active().ok_or("no document open")?; + if st.selection.is_empty() { Err("nothing selected".into()) } else { Ok(()) } } pub fn has_multi(s: &Session) -> std::result::Result<(), String> { - has_doc(s)?; - if s.active().unwrap().selection.len() < 2 { Err("select at least two objects".into()) } else { Ok(()) } + let st = s.active().ok_or("no document open")?; + if st.selection.len() < 2 { Err("select at least two objects".into()) } else { Ok(()) } } pub fn can_undo(s: &Session) -> std::result::Result<(), String> { s.active().filter(|d| !d.history.undo.is_empty()).map(|_| ()).ok_or_else(|| "nothing to undo".into()) diff --git a/crates/engine/src/cmd/object.rs b/crates/engine/src/cmd/object.rs index f8316d9c..1c08f1c1 100644 --- a/crates/engine/src/cmd/object.rs +++ b/crates/engine/src/cmd/object.rs @@ -511,7 +511,7 @@ fn compound_make(s: &mut Session, _: &Value) -> Result { for id in &ids { d.remove(*id)?; } - let ch = d.node_mut(cid).unwrap().children_mut().unwrap(); + let ch = d.node_mut(cid).and_then(|n| n.children_mut()).ok_or(EngineError::NoNode(cid))?; for mut p in paths { let pn = Arc::make_mut(&mut p); pn.appearance = Appearance::default(); @@ -586,7 +586,7 @@ pub(super) fn release_clip(d: &mut Document, id: NodeId) { fn clip_make(s: &mut Session, _: &Value) -> Result { let ids = selected_roots(s)?; - let top = *ids.last().unwrap(); + let Some(&top) = ids.last() else { return Err(EngineError::Other("nothing selected".into())) }; let gid = s.edit("Make Clipping Mask", |d, sel| { make_clipping_path(d, top)?; let (par, idx, _) = d.position(top).ok_or(EngineError::NoNode(top))?; diff --git a/crates/engine/src/cmd/path.rs b/crates/engine/src/cmd/path.rs index c0b663ee..e4faf3b9 100644 --- a/crates/engine/src/cmd/path.rs +++ b/crates/engine/src/cmd/path.rs @@ -283,6 +283,11 @@ fn reverse(s: &mut Session, p: &Value) -> Result { Ok(json!({ "changed": changed })) } +/// First and last anchor points of a subpath. +fn end_points(sp: &SubPath) -> Option<(Point, Point)> { + Some((sp.anchors.first()?.p, sp.anchors.last()?.p)) +} + fn join(s: &mut Session, _: &Value) -> Result { let ids = selected_paths(s)?; match ids.as_slice() { @@ -292,9 +297,12 @@ fn join(s: &mut Session, _: &Value) -> Result { let path = path_mut(d, id)?; let sp = path.subpaths.iter_mut().find(|s| !s.closed).ok_or_else(|| EngineError::Other("path is already closed".into()))?; // Merge coincident end points. - if sp.anchors.len() > 2 && sp.anchors[0].p.distance(sp.anchors.last().unwrap().p) < 1e-6 { - let last = sp.anchors.pop().unwrap(); - sp.anchors[0].h_in = last.h_in; + if sp.anchors.len() > 2 + && end_points(sp).is_some_and(|(f, l)| f.distance(l) < 1e-6) + && let Some(last) = sp.anchors.pop() + && let Some(first) = sp.anchors.first_mut() + { + first.h_in = last.h_in; } sp.closed = true; Ok(()) @@ -310,21 +318,26 @@ fn join(s: &mut Session, _: &Value) -> Result { }; let mut x = pa.subpaths[sa].clone(); let mut y = pb.subpaths[sb].clone(); - let (xf, xl) = (x.anchors[0].p, x.anchors.last().unwrap().p); - let (yf, yl) = (y.anchors[0].p, y.anchors.last().unwrap().p); + let (Some((xf, xl)), Some((yf, yl))) = (end_points(&x), end_points(&y)) else { + return Err(EngineError::Other("join needs open paths".into())); + }; // Pick the closest pair of ends. let cands = [(xl.distance(yf), false, false), (xl.distance(yl), false, true), (xf.distance(yf), true, false), (xf.distance(yl), true, true)]; - let (_, rx, ry) = cands.into_iter().min_by(|p, q| p.0.total_cmp(&q.0)).unwrap(); + let (_, rx, ry) = cands.into_iter().min_by(|p, q| p.0.total_cmp(&q.0)).unwrap_or((0.0, false, false)); if rx { x.reverse(); } if ry { y.reverse(); } - if x.anchors.last().unwrap().p.distance(y.anchors[0].p) < 1e-6 { + if let (Some(xe), Some(ys)) = (x.anchors.last().map(|a| a.p), y.anchors.first().map(|a| a.p)) + && xe.distance(ys) < 1e-6 + { let first = y.anchors.remove(0); - x.anchors.last_mut().unwrap().h_out = first.h_out; + if let Some(l) = x.anchors.last_mut() { + l.h_out = first.h_out; + } } x.anchors.extend(y.anchors); pa.subpaths[sa] = x; diff --git a/crates/engine/src/cmd/pathops.rs b/crates/engine/src/cmd/pathops.rs index 5a3623d8..03224caf 100644 --- a/crates/engine/src/cmd/pathops.rs +++ b/crates/engine/src/cmd/pathops.rs @@ -324,11 +324,14 @@ fn run_pf(s: &mut Session, op: PathfinderOp) -> Result { if lv.is_empty() { continue; } - if shape_mode && lv.len() > 1 { + if shape_mode + && lv.len() > 1 + && let Some(top) = lv.last() + { // A group acts as one shape (the union of its leaves) with its top leaf's style. let parts: Vec<(PathData, FillRule)> = lv.iter().filter_map(node_path).collect(); let refs: Vec<(&PathData, FillRule)> = parts.iter().map(|(p, r)| (p, *r)).collect(); - push(po::unite_all(&refs), FillRule::NonZero, style_of(lv.last().unwrap())); + push(po::unite_all(&refs), FillRule::NonZero, style_of(top)); } else { for l in &lv { if let Some((p, r)) = node_path(l) { @@ -349,8 +352,10 @@ fn run_pf(s: &mut Session, op: PathfinderOp) -> Result { } let (par, idx, _) = d.position(top).ok_or(EngineError::NoNode(top))?; let new_node = if shape_mode { - let r = results.into_iter().find(|r| !r.path.is_empty()).unwrap(); - shape_node_styled(d, r.path, &styles[r.key as usize]) + let r = + results.into_iter().find(|r| !r.path.is_empty()).ok_or_else(|| EngineError::Other(format!("{label} produced an empty result")))?; + let style = styles.get(r.key as usize).ok_or_else(|| EngineError::Other(format!("{label}: lost track of a shape's style")))?; + shape_node_styled(d, r.path, style) } else { let mut children = vec![]; for r in results { @@ -418,8 +423,11 @@ fn offset_path(s: &mut Session, p: &Value) -> Result { if made.is_empty() { continue; } - let new = if made.len() == 1 && !matches!(n.kind, NodeKind::Group { .. }) { - made.pop().unwrap() + let new = if made.len() == 1 + && !matches!(n.kind, NodeKind::Group { .. }) + && let Some(one) = made.pop() + { + one } else { let gid = d.alloc_id(); Node::group(gid, made.into_iter().map(Arc::new).collect()) diff --git a/crates/engine/src/cmd/patterncmds.rs b/crates/engine/src/cmd/patterncmds.rs index 17fd8f27..b9e5d9de 100644 --- a/crates/engine/src/cmd/patterncmds.rs +++ b/crates/engine/src/cmd/patterncmds.rs @@ -212,11 +212,11 @@ fn unique_pattern_name(d: &Document, base: &str) -> String { if d.pattern(base).is_none() && d.swatch(base).is_none() { return base.to_string(); } - (2..).map(|i| format!("{base} {i}")).find(|n| d.pattern(n).is_none() && d.swatch(n).is_none()).unwrap() + (2..).map(|i| format!("{base} {i}")).find(|n| d.pattern(n).is_none() && d.swatch(n).is_none()).unwrap_or_else(|| base.to_string()) } fn next_pattern_name(d: &Document) -> String { - (1..).map(|i| format!("New Pattern {i}")).find(|n| d.pattern(n).is_none() && d.swatch(n).is_none()).unwrap() + (1..).map(|i| format!("New Pattern {i}")).find(|n| d.pattern(n).is_none() && d.swatch(n).is_none()).unwrap_or_else(|| "New Pattern".to_string()) } /// Add pattern `def` and its swatch. @@ -633,11 +633,8 @@ fn make_repeat(s: &mut Session, p: &Value, label: &str, make: impl Fn(Vec> = roots.iter().filter_map(|id| d.node(*id).cloned()).map(Arc::new).collect(); let id = d.alloc_id(); diff --git a/crates/engine/src/cmd/rasterfx.rs b/crates/engine/src/cmd/rasterfx.rs index 0244e376..d1b9533d 100644 --- a/crates/engine/src/cmd/rasterfx.rs +++ b/crates/engine/src/cmd/rasterfx.rs @@ -94,19 +94,21 @@ pub(crate) fn effect_image(out: &mut Document, whole: &Node, knockout: Option<&N let mut img = render(out, vec![whole.clone()], region, scale); if let Some(bare) = knockout { let obj = render(out, vec![bare.clone()], region, scale); - for (px, o) in img.pixels.chunks_exact_mut(4).zip(obj.pixels.chunks_exact(4)) { + for (px, o) in img.pixels.as_chunks_mut::<4>().0.iter_mut().zip(obj.pixels.as_chunks::<4>().0) { let keep = 1.0 - o[3] as f32 / 255.0; for c in px.iter_mut() { *c = (*c as f32 * keep).round() as u8; } } } + // Can't encode: keep the object as it is (vector, effects ignored) rather than fail the export. + let png = img.to_png().ok()?; let id = out.alloc_id(); let mut key = format!("raster-effect-{}", id.0); while out.images.contains_key(&key) { key.push('+'); } - out.images.insert(key.clone(), ImageBlob { mime: "image/png".into(), bytes: Arc::new(img.to_png()) }); + out.images.insert(key.clone(), ImageBlob { mime: "image/png".into(), bytes: Arc::new(png) }); let xf = Affine::translate(region.origin().to_vec2()) * Affine::scale(1.0 / scale); let mut image = Node::new(id, NodeKind::Image(ImageObject { key, width: img.width, height: img.height, xf, link: None })); image.name = Some("Raster effect".into()); diff --git a/crates/engine/src/cmd/select.rs b/crates/engine/src/cmd/select.rs index 086bdfb0..2b797d20 100644 --- a/crates/engine/src/cmd/select.rs +++ b/crates/engine/src/cmd/select.rs @@ -155,7 +155,7 @@ fn step(s: &mut Session, dir: i64) -> Result { if j < 0 || j >= n as i64 { return ok(); } - let id = st.doc.children(par).unwrap()[j as usize].id; + let Some(id) = st.doc.children(par).and_then(|c| c.get(j as usize)).map(|n| n.id) else { return ok() }; s.select(|_, sel| sel.set([id]))?; ok() } diff --git a/crates/engine/src/cmd/textedit.rs b/crates/engine/src/cmd/textedit.rs index 82ef9a9b..e284fb2e 100644 --- a/crates/engine/src/cmd/textedit.rs +++ b/crates/engine/src/cmd/textedit.rs @@ -231,8 +231,11 @@ impl CharChange { // Keep the style when the new family has it, else its closest match. if self.style.is_none() { let styles = vectorcraft_text::FontDb::global().styles(f); - if !styles.is_empty() && !styles.iter().any(|s| s.eq_ignore_ascii_case(&st.font_style)) { - st.font_style = vectorcraft_text::FontDb::global().face(f, &st.font_style).style.clone(); + if !styles.is_empty() + && !styles.iter().any(|s| s.eq_ignore_ascii_case(&st.font_style)) + && let Some(face) = vectorcraft_text::FontDb::global().face(f, &st.font_style) + { + st.font_style = face.style.clone(); } } } diff --git a/crates/engine/src/cmd/textstyles.rs b/crates/engine/src/cmd/textstyles.rs index 549a9185..eef862d9 100644 --- a/crates/engine/src/cmd/textstyles.rs +++ b/crates/engine/src/cmd/textstyles.rs @@ -141,19 +141,16 @@ fn with_attrs(base: &T, attrs: &Map(cur: &T, old: &Map, new: &Map) -> T { - let mut m = match serde_json::to_value(cur) { - Ok(Value::Object(m)) => m, - _ => return serde_json::from_value(serde_json::to_value(cur).unwrap_or_default()).unwrap_or_else(|_| unreachable!()), - }; +/// value (or not set by it) take the new value; overrides stay. Unchanged if the style doesn't +/// round-trip through JSON. +fn restyle(cur: &T, old: &Map, new: &Map) -> T { + let Ok(Value::Object(mut m)) = serde_json::to_value(cur) else { return cur.clone() }; for (k, v) in new { if old.get(k).is_none_or(|o| m.get(k) == Some(o)) { m.insert(k.clone(), v.clone()); } } - serde_json::from_value(Value::Object(m.clone())) - .unwrap_or_else(|_| serde_json::from_value(serde_json::to_value(cur).unwrap_or_default()).expect("round trip")) + serde_json::from_value(Value::Object(m)).unwrap_or_else(|_| cur.clone()) } /// Every text object id in the document. @@ -276,7 +273,7 @@ fn new(s: &mut Session, p: &Value, kind: Kind) -> Result { let base = if kind == Kind::Char { "Character Style" } else { "Paragraph Style" }; let name = match str_param(p, "name") { Some(n) => n.to_string(), - None => (1..).map(|i| format!("{base} {i}")).find(|n| kind.attrs(d, n).is_none()).expect("free name"), + None => (1..).map(|i| format!("{base} {i}")).find(|n| kind.attrs(d, n).is_none()).unwrap_or_else(|| base.to_string()), }; if name.trim().is_empty() || kind.attrs(d, &name).is_some() { return Err(bad(&c, format!("a style named `{name}` already exists"))); @@ -387,7 +384,7 @@ fn duplicate(s: &mut Session, p: &Value, kind: Kind) -> Result { let copy = (1..) .map(|i| if i == 1 { format!("{name} copy") } else { format!("{name} copy {i}") }) .find(|n| kind.attrs(d, n).is_none()) - .expect("free name"); + .unwrap_or_else(|| format!("{name} copy")); new(s, &json!({ "name": copy, "attrs": attrs }), kind) } diff --git a/crates/engine/src/cmd/threads.rs b/crates/engine/src/cmd/threads.rs index 138f99e6..0b4d9615 100644 --- a/crates/engine/src/cmd/threads.rs +++ b/crates/engine/src/cmd/threads.rs @@ -47,7 +47,7 @@ pub fn specs() -> Vec { fn has_thread_selection(s: &Session) -> std::result::Result<(), String> { has_selection(s)?; - let st = s.active().unwrap(); + let st = s.active().ok_or("no document open")?; if st.selection.objects.iter().any(|id| st.doc.text_threads.iter().any(|t| t.contains(id))) { Ok(()) } else { diff --git a/crates/engine/src/cmd/typecmd.rs b/crates/engine/src/cmd/typecmd.rs index 8bc1a935..2dfeeff5 100644 --- a/crates/engine/src/cmd/typecmd.rs +++ b/crates/engine/src/cmd/typecmd.rs @@ -250,7 +250,7 @@ fn area_options(s: &mut Session, p: &Value) -> Result { let first = ids.first().ok_or_else(|| bad(C, "select area type (text in a frame)"))?; let current = match &s.doc()?.doc.node(*first).map(|n| &n.kind) { Some(NodeKind::Text(t)) => t.area.clone(), - _ => unreachable!(), + _ => return Err(bad(C, "select area type (text in a frame)")), }; let mut v = serde_json::to_value(¤t).map_err(|e| EngineError::Other(e.to_string()))?; let mut changed = false; diff --git a/crates/engine/src/guard.rs b/crates/engine/src/guard.rs new file mode 100644 index 00000000..ce759c77 --- /dev/null +++ b/crates/engine/src/guard.rs @@ -0,0 +1,46 @@ +//! Last line of defence against bugs: frontends never crash on a panic. +//! +//! Code must not panic (see `AGENTS.md` › Robustness); fallible work returns `Result`. Should a bug +//! panic anyway, the entry points ([`crate::Session::execute`], tool events, the MCP server, the UI +//! frame and the control channel) catch it with [`catch_panic`], roll back what they can and report +//! an error instead of taking the app (and the user's unsaved work) down. +//! +//! On wasm the panic strategy is `abort`, so this only helps native builds; the rule against +//! panicking code is what protects the web app. + +use std::any::Any; +use std::panic::{AssertUnwindSafe, catch_unwind}; + +/// Runs `f`, turning a panic into `Err(message)`. The default panic hook still prints the message +/// and location to stderr. Callers must leave their state consistent after an `Err` (roll back, or +/// rebuild the part `f` was working on). +pub fn catch_panic(f: impl FnOnce() -> T) -> Result { + catch_unwind(AssertUnwindSafe(f)).map_err(|payload| { + let msg = panic_message(payload.as_ref()); + log::error!("recovered from a panic: {msg}"); + msg + }) +} + +/// The message of a panic payload (`panic!("…")` carries a `&str` or a `String`). +pub fn panic_message(payload: &(dyn Any + Send)) -> String { + if let Some(s) = payload.downcast_ref::<&str>() { + (*s).to_string() + } else if let Some(s) = payload.downcast_ref::() { + s.clone() + } else { + "unknown panic".to_string() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn catch_panic_returns_value_or_message() { + assert_eq!(catch_panic(|| 7), Ok(7)); + assert_eq!(catch_panic(|| -> i32 { panic!("boom {}", 1) }), Err("boom 1".to_string())); + assert_eq!(catch_panic(|| -> i32 { std::panic::panic_any(5_u8) }), Err("unknown panic".to_string())); + } +} diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 6e2f76eb..e907bbdb 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -7,6 +7,7 @@ #![forbid(unsafe_code)] pub mod cmd; +pub mod guard; pub mod inspect; mod tooling; @@ -39,6 +40,9 @@ pub enum EngineError { NoNode(NodeId), #[error("{0}")] Other(String), + /// A bug: the command panicked. The document was rolled back to its state before the command. + #[error("internal error in `{cmd}`: {msg} (the document was left as it was before; please report this bug)")] + Internal { cmd: String, msg: String }, } impl From for EngineError { @@ -489,6 +493,8 @@ pub struct Session { pub(crate) tool: Box, pub(crate) last_view: ViewInfo, depth: u32, + /// Set when the active tool panicked (see [`guard`]); reported by the next tool event. + tool_panic: Option, /// Draw Normal / Behind / Inside (toolbar drawing modes). pub draw_mode: DrawMode, /// The path new art is drawn inside (Draw Inside). @@ -545,6 +551,7 @@ impl Session { tool: vectorcraft_tools::create("selection"), last_view: ViewInfo::default(), depth: 0, + tool_panic: None, draw_mode: DrawMode::Normal, draw_inside: None, untitled_counter: 0, @@ -654,9 +661,7 @@ impl Session { if self.depth == 0 { self.journal_note.clear(); } - self.depth += 1; - let r = (spec.run)(self, params); - self.depth -= 1; + let r = if self.depth == 0 { self.run_guarded(id, |s| (spec.run)(s, params)) } else { self.run_nested(|s| (spec.run)(s, params)) }; let r = r?; if self.depth == 0 { self.inherit_new_art(); @@ -690,6 +695,39 @@ impl Session { } } + fn run_nested(&mut self, f: impl FnOnce(&mut Self) -> Result) -> Result { + self.depth += 1; + let r = f(self); + self.depth -= 1; + r + } + + /// A top-level command: a panic (a bug) becomes [`EngineError::Internal`] and the active + /// document goes back to how it was before the command, instead of crashing the frontend. + fn run_guarded(&mut self, id: &str, f: impl FnOnce(&mut Self) -> Result) -> Result { + let snapshot = self.active().map(|d| (d.uid, d.doc.clone(), d.selection.clone(), d.interaction.clone())); + let active = self.active; + match guard::catch_panic(|| self.run_nested(f)) { + Ok(r) => r, + Err(msg) => { + self.depth = 0; + self.journal_note.clear(); + if let Some((uid, doc, selection, interaction)) = snapshot + && let Some(st) = self.docs.iter_mut().find(|d| d.uid == uid) + { + st.doc = doc; + st.selection = selection; + st.interaction = interaction; + st.revision += 1; + } + if active.is_some_and(|i| i < self.docs.len()) { + self.active = active; + } + Err(EngineError::Internal { cmd: id.to_string(), msg }) + } + } + } + /// Run `f` on a mutable copy of the active document. Outside an interaction this records one /// undo step labelled `label`; inside one it just mutates (the interaction commits later). pub fn edit(&mut self, label: &str, f: impl FnOnce(&mut Document, &mut Selection) -> Result) -> Result { diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 99ecd5f8..92a69775 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -481,3 +481,32 @@ fn shaper_turns_rough_strokes_into_live_shapes_and_scribbles_delete() { stroke(&mut s, &zig); assert!(s.doc().unwrap().doc.node(id).is_none()); } + +#[test] +fn a_panicking_command_rolls_back_instead_of_crashing() { + let mut s = session(); + let a = rect(&mut s, 10.0, 10.0, 100.0, 50.0); + let before = s.doc().unwrap().doc.clone(); + let undo_steps = s.doc().unwrap().history.undo.len(); + // A bug halfway through a command: the document was already changed in place, inside a + // nested command. + let r = s.run_guarded("test.panic", |s| { + let st = s.doc_mut()?; + Arc::make_mut(&mut st.doc).remove(a)?; + st.selection = Selection::default(); + s.run_nested(|_| panic!("boom")) + }); + match r { + Err(EngineError::Internal { cmd, msg }) => assert_eq!((cmd.as_str(), msg.as_str()), ("test.panic", "boom")), + other => panic!("expected an internal error, got {other:?}"), + } + let st = s.doc().unwrap(); + assert!(Arc::ptr_eq(&st.doc, &before), "document rolled back"); + assert_eq!(st.selection.objects, vec![a]); + assert_eq!(st.history.undo.len(), undo_steps); + assert_eq!(s.depth, 0); + // The session keeps working (and journaling top-level commands). + let b = rect(&mut s, 200.0, 10.0, 50.0, 50.0); + assert!(s.doc().unwrap().doc.node(b).is_some()); + assert_eq!(s.journal.last().map(|j| j.0.as_str()), Some("shape.rectangle")); +} diff --git a/crates/engine/src/tooling.rs b/crates/engine/src/tooling.rs index 8d5bff79..d07cdade 100644 --- a/crates/engine/src/tooling.rs +++ b/crates/engine/src/tooling.rs @@ -4,7 +4,7 @@ use serde_json::Value; use vectorcraft_geom::Point; use vectorcraft_tools::{Action, Cursor, Mods, Overlay, PointerEvent, ToolContext, ToolKey}; -use crate::{Result, Session}; +use crate::{EngineError, Result, Session}; /// View state the tools need from the frontend. #[derive(Clone, Copy, Debug)] @@ -86,24 +86,43 @@ impl Session { raster_sample: self.prefs.eyedropper.sample_size, preview_bounds: self.prefs.use_preview_bounds, }; - f(self.tool.as_mut(), &cx) + let tool = &mut self.tool; + match crate::guard::catch_panic(|| f(tool.as_mut(), &cx)) { + Ok(r) => r, + Err(msg) => { + // A bug in the tool: start it afresh and drop its half-done drag. + let id = self.tool.id(); + self.tool = vectorcraft_tools::create(id); + let _ = self.cancel_interaction(); + self.tool_panic = Some(EngineError::Internal { cmd: format!("tool `{id}`"), msg }); + R::default() + } + } + } + + /// The error of a tool that panicked in the last [`Self::with_tool_cx`] call, if any. + fn take_tool_panic(&mut self) -> Result<()> { + self.tool_panic.take().map_or(Ok(()), Err) } /// Feed a pointer event to the active tool. Returns requests for the UI. pub fn pointer(&mut self, ev: &PointerEvent, view: ViewInfo) -> Result> { self.last_view = view; let acts = self.with_tool_cx(view, |t, cx| t.pointer(cx, ev)); + self.take_tool_panic()?; self.apply_actions(acts) } pub fn tool_key(&mut self, key: ToolKey, mods: Mods, view: ViewInfo) -> Result> { let acts = self.with_tool_cx(view, |t, cx| t.key(cx, key, mods)); + self.take_tool_panic()?; self.apply_actions(acts) } /// Typed text for the active tool (Type tool). pub fn tool_text(&mut self, text: &str, view: ViewInfo) -> Result> { let acts = self.with_tool_cx(view, |t, cx| t.text_input(cx, text)); + self.take_tool_panic()?; self.apply_actions(acts) } diff --git a/crates/engine/tests/command_sweep.rs b/crates/engine/tests/command_sweep.rs index 331bd2a6..ef109335 100644 --- a/crates/engine/tests/command_sweep.rs +++ b/crates/engine/tests/command_sweep.rs @@ -1,6 +1,8 @@ //! Every-command sweep: each registered command, in three fixture states, with `{}` and with fuzzed //! junk params, must return Ok or Err (never panic), never leave an interaction open, and never //! corrupt the tree. +// Integration tests: unwrapping and panicking on failure is fine here, unlike in shipped code (AGENTS.md › Robustness). +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] use serde_json::{Value, json}; use vectorcraft_engine::{Session, command_specs}; diff --git a/crates/engine/tests/import_fuzz.rs b/crates/engine/tests/import_fuzz.rs new file mode 100644 index 00000000..94cf3ba6 --- /dev/null +++ b/crates/engine/tests/import_fuzz.rs @@ -0,0 +1,285 @@ +//! Untrusted files never crash the app: garbage, truncated, mutated and hostile SVG and PDF input +//! must import as an error or as a document that then renders and exports, without a panic. +//! +//! `PROPTEST_CASES=20000 cargo test -p vectorcraft-engine --test import_fuzz` runs a deeper search. +// Integration tests: unwrapping and panicking on failure is fine here, unlike in shipped code (AGENTS.md › Robustness). +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] + +use proptest::prelude::*; +use vectorcraft_doc::Document; +use vectorcraft_testkit::catch_quiet; +use vectorcraft_testkit::fixtures::rich_session; + +/// Import must not panic; whatever comes back must render and export without panicking either. +fn survive(what: &str, import: impl FnOnce() -> Option) -> Result<(), TestCaseError> { + let r = catch_quiet(|| { + if let Some(d) = import() { + let mut r = vectorcraft_render::Renderer::new(); + if let Some(ab) = d.artboards.first() { + let scale = (256.0 / ab.rect.width().max(ab.rect.height()).max(1.0)).min(1.0); + if vectorcraft_render::raster_size(ab.rect, scale).is_ok() { + let _ = r.render_region(&d, ab.rect, scale, true).to_png(); + } + } + let _ = vectorcraft_svg::export(&d, &vectorcraft_svg::ExportOptions::default()); + let _ = vectorcraft_engine::export_pdf(&d, &vectorcraft_pdf::PdfOptions::default()); + let _ = vectorcraft_format::save_file(&d); + } + }); + r.map_err(|msg| TestCaseError::fail(format!("{what}: panicked: {msg}"))) +} + +fn rich_doc() -> Document { + (*rich_session().doc().unwrap().doc).clone() +} + +fn rich_svg() -> String { + vectorcraft_svg::export(&rich_doc(), &vectorcraft_svg::ExportOptions::default()) +} + +fn rich_pdf() -> Vec { + vectorcraft_engine::export_pdf(&rich_doc(), &vectorcraft_pdf::PdfOptions { compress: false, ..Default::default() }).unwrap() +} + +/// Numbers that tend to break arithmetic: zero, negatives, huge, tiny, exponents, junk. +fn arb_num() -> impl Strategy { + prop_oneof![ + (-1000.0f64..1000.0).prop_map(|v| format!("{v:.2}")), + Just("0".to_string()), + Just("-0".to_string()), + Just("1e308".to_string()), + Just("-1e308".to_string()), + Just("1e-308".to_string()), + Just("1e15".to_string()), + Just("4294967296".to_string()), + Just("-2147483649".to_string()), + Just("NaN".to_string()), + Just("inf".to_string()), + Just("".to_string()), + Just("1e".to_string()), + Just("--1".to_string()), + ] +} + +// ---------- SVG ---------- + +fn arb_svg_attr() -> impl Strategy { + let names = prop::sample::select(vec![ + "x", + "y", + "width", + "height", + "rx", + "ry", + "r", + "cx", + "cy", + "x1", + "y1", + "x2", + "y2", + "dx", + "dy", + "font-size", + "stroke-width", + "stroke-dasharray", + "stroke-dashoffset", + "stroke-miterlimit", + "opacity", + "fill-opacity", + "offset", + "startOffset", + "letter-spacing", + "rotate", + "textLength", + "fx", + "fy", + "fr", + "points", + "viewBox", + "transform", + "d", + ]); + let value = prop_oneof![ + arb_num(), + prop::collection::vec(arb_num(), 0..7).prop_map(|v| v.join(" ")), + prop::collection::vec(arb_num(), 0..7).prop_map(|v| format!("matrix({})", v.join(" "))), + prop::collection::vec(arb_num(), 0..3).prop_map(|v| format!("rotate({}) scale({})", v.join(" "), v.join(","))), + prop::collection::vec((prop::sample::select(vec!["M", "L", "C", "Q", "A", "Z", "H", "V", "S", "T", "m", "a"]), arb_num()), 0..12) + .prop_map(|v| v.iter().map(|(c, n)| format!("{c}{n} {n}")).collect::>().join(" ")), + prop::collection::vec(arb_num(), 0..2).prop_map(|v| format!("{}%", v.join(""))), + ]; + (names, value).prop_map(|(n, v)| format!(" {n}=\"{v}\"")) +} + +fn arb_svg_element(depth: u32) -> BoxedStrategy { + let tag = prop::sample::select(vec![ + "rect", + "circle", + "ellipse", + "line", + "polyline", + "polygon", + "path", + "text", + "tspan", + "textPath", + "g", + "use", + "image", + "linearGradient", + "radialGradient", + "stop", + "pattern", + "clipPath", + "mask", + "symbol", + "defs", + "svg", + "marker", + ]); + let attrs = || prop::collection::vec(arb_svg_attr(), 0..6).prop_map(|v| v.concat()); + let leaf = (tag.clone(), attrs(), "[a-z ]{0,6}").prop_map(|(t, a, txt)| format!("<{t} id=\"e\"{a} href=\"#e\">{txt}")); + if depth == 0 { + return leaf.boxed(); + } + prop_oneof![ + leaf, + (tag, attrs(), prop::collection::vec(arb_svg_element(depth - 1), 0..4)).prop_map(|(t, a, ch)| format!("<{t}{a}>{}", ch.concat())) + ] + .boxed() +} + +/// A character-level edit that keeps the string valid UTF-8 (the importer takes `&str`). +fn mutate_text(src: &str, cut: usize, edits: &[(usize, char)]) -> String { + let mut chars: Vec = src.chars().collect(); + for &(i, c) in edits { + if !chars.is_empty() { + let n = chars.len(); + chars[i % n] = c; + } + } + chars.truncate(cut.min(chars.len())); + chars.into_iter().collect() +} + +proptest! { + #![proptest_config(ProptestConfig { cases: 64, failure_persistence: None, ..ProptestConfig::default() })] + + #[test] + fn svg_garbage_never_panics(s in ".{0,300}") { + survive("svg garbage", || vectorcraft_svg::import(&s).ok())?; + } + + #[test] + fn svg_hostile_markup_never_panics( + root in prop::collection::vec(arb_svg_attr(), 0..4), + body in prop::collection::vec(arb_svg_element(2), 0..6), + ) { + let svg = format!("{}", root.concat(), body.concat()); + survive(&svg, || vectorcraft_svg::import(&svg).ok())?; + } + + #[test] + fn svg_mutated_export_never_panics( + cut in 0usize..20_000, + edits in prop::collection::vec((0usize..20_000, prop::sample::select(vec!['<', '>', '"', '/', '-', '9', 'e', '.', ' ', '#', '%', '&', ';', 'x'])), 0..10), + ) { + let svg = mutate_text(&rich_svg(), cut, &edits); + survive("mutated svg", || vectorcraft_svg::import(&svg).ok())?; + } +} + +// ---------- PDF ---------- + +fn handmade_pdf(content: &str, resources: &str, media: &str) -> Vec { + let objs = [ + "<< /Type /Catalog /Pages 2 0 R >>".to_string(), + "<< /Type /Pages /Kids [3 0 R] /Count 1 >>".to_string(), + format!("<< /Type /Page /Parent 2 0 R /MediaBox {media} /Contents 4 0 R /Resources {resources} >>"), + format!("<< /Length {} >>\nstream\n{content}\nendstream", content.len() + 1), + ]; + let mut out = b"%PDF-1.7\n".to_vec(); + let mut offsets = vec![]; + for (i, o) in objs.iter().enumerate() { + offsets.push(out.len()); + out.extend_from_slice(format!("{} 0 obj\n{o}\nendobj\n", i + 1).as_bytes()); + } + let xref = out.len(); + out.extend_from_slice(format!("xref\n0 {}\n0000000000 65535 f \n", objs.len() + 1).as_bytes()); + for off in offsets { + out.extend_from_slice(format!("{off:010} 00000 n \n").as_bytes()); + } + out.extend_from_slice(format!("trailer\n<< /Size {} /Root 1 0 R >>\nstartxref\n{xref}\n%%EOF\n", objs.len() + 1).as_bytes()); + out +} + +/// One content-stream operation with junk operands. +fn arb_pdf_op() -> impl Strategy { + let op = prop::sample::select(vec![ + "m", "l", "c", "v", "y", "h", "re", "f", "f*", "F", "S", "s", "B", "B*", "b", "n", "W", "W*", "q", "Q", "cm", "w", "J", "j", "M", "d", "ri", + "i", "gs", "g", "G", "rg", "RG", "k", "K", "cs", "CS", "sc", "SC", "scn", "SCN", "sh", "Do", "BT", "ET", "Tf", "Td", "TD", "Tm", "T*", "Tc", + "Tw", "Tz", "TL", "Tr", "Ts", "Tj", "TJ", "'", "\"", "BMC", "BDC", "EMC", "d0", "d1", + ]); + let operand = prop_oneof![ + arb_num(), + prop::collection::vec(arb_num(), 0..5).prop_map(|v| format!("[{}]", v.join(" "))), + Just("/F1".to_string()), + Just("/Sh0".to_string()), + Just("/Im0".to_string()), + Just("/GS0".to_string()), + Just("/DeviceRGB".to_string()), + Just("/Pattern".to_string()), + Just("(Hi)".to_string()), + Just("<00ff>".to_string()), + Just("[(a) -250 (b)]".to_string()), + ]; + (prop::collection::vec(operand, 0..7), op).prop_map(|(args, op)| format!("{} {op}", args.join(" "))) +} + +fn arb_resources() -> impl Strategy { + prop::sample::select(vec![ + "<< >>".to_string(), + "<< /Font << /F1 << /Type /Font /Subtype /Type1 /BaseFont /Helvetica >> >> >>".to_string(), + "<< /ExtGState << /GS0 << /CA 0.5 /ca -3 /BM /Multiply /SMask /None >> >> >>".to_string(), + "<< /Shading << /Sh0 << /ShadingType 2 /ColorSpace /DeviceRGB /Coords [0 0 1e308 0] /Function << /FunctionType 2 /Domain [0 1] /C0 [1 0 0] /C1 [0 0 1] /N 1 >> >> >> >>".to_string(), + "<< /Shading << /Sh0 << /ShadingType 3 /ColorSpace /DeviceRGB /Coords [0 0 0 0 0 -5] /Function << /FunctionType 2 /Domain [0 1] /C0 [1] /C1 [] /N -1 >> >> >> >>".to_string(), + "<< /XObject << /Im0 << /Type /XObject /Subtype /Image /Width 4294967295 /Height 0 /BitsPerComponent 8 /ColorSpace /DeviceRGB /Length 3 >> >> >>".to_string(), + "<< /Pattern << /P0 << /PatternType 1 /PaintType 1 /TilingType 1 /BBox [0 0 0 0] /XStep 0 /YStep -0 /Resources << >> >> >> >>".to_string(), + ]) +} + +proptest! { + #![proptest_config(ProptestConfig { cases: 64, failure_persistence: None, ..ProptestConfig::default() })] + + #[test] + fn pdf_garbage_never_panics(bytes in prop::collection::vec(any::(), 0..600)) { + let mut b = b"%PDF-1.7\n".to_vec(); + b.extend(bytes); + survive("pdf garbage", || vectorcraft_pdf::import(&b).ok())?; + } + + #[test] + fn pdf_hostile_content_never_panics( + ops in prop::collection::vec(arb_pdf_op(), 0..40), + resources in arb_resources(), + media in prop::sample::select(vec!["[0 0 100 100]", "[0 0 0 0]", "[0 0 1e308 1e308]", "[100 100 0 0]", "[-5 -5 -1 -1]", "[0 0 14400 14400]", "[]", "[0 0 NaN 5]"]), + ) { + let pdf = handmade_pdf(&ops.join("\n"), &resources, media); + survive(&ops.join(" "), || vectorcraft_pdf::import(&pdf).ok())?; + } + + #[test] + fn pdf_mutated_export_never_panics( + cut in 0usize..40_000, + flips in prop::collection::vec((0usize..40_000, prop::sample::select(vec![b'0', b'9', b'-', b'.', b' ', b'[', b']', b'<', b'>', b'/', b'(', b'e', 0u8, 0xff])), 0..10), + ) { + let mut b = rich_pdf(); + for (i, c) in flips { + let n = b.len(); + b[i % n] = c; + } + b.truncate(cut.min(b.len()).max(9)); + survive("mutated pdf", || vectorcraft_pdf::import(&b).ok())?; + } +} diff --git a/crates/engine/tests/known_bugs.rs b/crates/engine/tests/known_bugs.rs index ea394663..61f1bf6d 100644 --- a/crates/engine/tests/known_bugs.rs +++ b/crates/engine/tests/known_bugs.rs @@ -1,5 +1,7 @@ //! Minimal reproductions of engine bugs found by the property tests. Each is `#[ignore]`d with a //! `BUG:` reason until fixed; remove the attribute when the fix lands. +// Integration tests: unwrapping and panicking on failure is fine here, unlike in shipped code (AGENTS.md › Robustness). +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] use serde_json::json; use vectorcraft_engine::Session; diff --git a/crates/engine/tests/model_based.rs b/crates/engine/tests/model_based.rs index 90578515..af91b249 100644 --- a/crates/engine/tests/model_based.rs +++ b/crates/engine/tests/model_based.rs @@ -1,5 +1,7 @@ //! Model-based / stateful property tests: random command sequences drawn from the registry, with //! structural invariants checked after every step and history/serialization invariants at the end. +// Integration tests: unwrapping and panicking on failure is fine here, unlike in shipped code (AGENTS.md › Robustness). +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] use proptest::prelude::*; use serde_json::json; diff --git a/crates/format/tests/prop_format.rs b/crates/format/tests/prop_format.rs index 2d2ba623..eea139d9 100644 --- a/crates/format/tests/prop_format.rs +++ b/crates/format/tests/prop_format.rs @@ -1,4 +1,6 @@ //! `.vectorcraft` format: round trips, robustness against garbage, versioning, unknown fields. +// Integration tests: unwrapping and panicking on failure is fine here, unlike in shipped code (AGENTS.md › Robustness). +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] use std::sync::Arc; diff --git a/crates/geom/src/path.rs b/crates/geom/src/path.rs index 22fc3a03..33942790 100644 --- a/crates/geom/src/path.rs +++ b/crates/geom/src/path.rs @@ -317,13 +317,13 @@ impl PathData { PathEl::ClosePath => { if let Some(mut sp) = cur.take() { // Merge a closing anchor that duplicates the start. - if sp.anchors.len() > 1 { - let first = sp.anchors[0].p; - let lastp = sp.anchors.last().unwrap().p; - if first.distance(lastp) < 1e-7 { - let l = sp.anchors.pop().unwrap(); - sp.anchors[0].h_in = l.h_in; - } + if sp.anchors.len() > 1 + && let (Some(first), Some(lastp)) = (sp.anchors.first().map(|a| a.p), sp.anchors.last().map(|a| a.p)) + && first.distance(lastp) < 1e-7 + && let Some(l) = sp.anchors.pop() + && let Some(a0) = sp.anchors.first_mut() + { + a0.h_in = l.h_in; } sp.closed = true; for a in &mut sp.anchors { @@ -351,9 +351,10 @@ impl PathData { return None; } let bp = self.to_bezpath(); - if self.anchor_count() == 1 { - let p = self.subpaths.iter().find_map(|s| s.anchors.first()).unwrap().p; - return Some(Rect::from_points(p, p)); + if self.anchor_count() == 1 + && let Some(a) = self.subpaths.iter().find_map(|s| s.anchors.first()) + { + return Some(Rect::from_points(a.p, a.p)); } Some(bp.bounding_box()) } diff --git a/crates/geom/tests/prop_geom.rs b/crates/geom/tests/prop_geom.rs index b3bf993b..905e57e8 100644 --- a/crates/geom/tests/prop_geom.rs +++ b/crates/geom/tests/prop_geom.rs @@ -1,4 +1,6 @@ //! Geometry property tests: PathData ↔ BezPath, bounds, hit testing, anchors and shapes. +// Integration tests: unwrapping and panicking on failure is fine here, unlike in shipped code (AGENTS.md › Robustness). +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] use proptest::prelude::*; use vectorcraft_geom::hit::{distance_to_outline, fill_contains, stroke_contains}; @@ -29,7 +31,7 @@ fn crossing_winding(bp: &BezPath, p: Point) -> i32 { PathEl::MoveTo(q) => pts.push(vec![q]), PathEl::LineTo(q) => pts.last_mut().unwrap().push(q), PathEl::ClosePath => {} - _ => unreachable!(), + other => panic!("unexpected {other:?}"), }); for poly in pts { for i in 0..poly.len() { diff --git a/crates/mcp/src/backend.rs b/crates/mcp/src/backend.rs index 57f9bbe6..e969e8a5 100644 --- a/crates/mcp/src/backend.rs +++ b/crates/mcp/src/backend.rs @@ -59,7 +59,9 @@ impl Remote { if self.conn.is_none() { self.reconnect()?; } - let (reader, writer) = self.conn.as_mut().expect("connected"); + let Some((reader, writer)) = self.conn.as_mut() else { + return Err(std::io::Error::new(std::io::ErrorKind::NotConnected, "not connected to the app")); + }; writer.write_all(line.as_bytes())?; writer.write_all(b"\n")?; writer.flush()?; diff --git a/crates/mcp/src/headless.rs b/crates/mcp/src/headless.rs index 3cb1c0dc..4735884b 100644 --- a/crates/mcp/src/headless.rs +++ b/crates/mcp/src/headless.rs @@ -201,7 +201,7 @@ impl Headless { let scale = p.get("scale").and_then(Value::as_f64).unwrap_or(1.0).clamp(0.01, 16.0); vectorcraft_render::raster_size(r, scale)?; let img = self.renderer.render_region(&doc, r, scale, true); - let png = img.to_png(); + let png = img.to_png()?; match s(p, "path") { Some(path) => { std::fs::write(path, &png).map_err(|e| format!("write {path}: {e}"))?; diff --git a/crates/mcp/src/server.rs b/crates/mcp/src/server.rs index 6b168481..c735c279 100644 --- a/crates/mcp/src/server.rs +++ b/crates/mcp/src/server.rs @@ -111,7 +111,10 @@ impl Server { if !(id.is_string() || id.is_number()) { return Some(error(Value::Null, INVALID_REQUEST, "`id` must be a string or number")); } - Some(match self.request(method, ¶ms) { + // A bug that panics fails this request only; the server keeps serving. + let r = vectorcraft_engine::guard::catch_panic(|| self.request(method, ¶ms)) + .unwrap_or_else(|msg| Err((INTERNAL_ERROR, format!("internal error in `{method}`: {msg} (please report this bug)")))); + Some(match r { Ok(r) => response(id, r), Err((code, m)) => error(id, code, m), }) diff --git a/crates/mcp/src/tools.rs b/crates/mcp/src/tools.rs index 5cd9b125..3749bb3a 100644 --- a/crates/mcp/src/tools.rs +++ b/crates/mcp/src/tools.rs @@ -516,7 +516,7 @@ fn add_text(b: &mut dyn Backend, a: &Args) -> Result { } let r = if let Some(path) = a.get("path") { let mut p = json!({"path": path, "mode": a.get("mode").cloned().unwrap_or(json!("onPath")), "text": text}); - for (k, v) in style.as_object().unwrap() { + for (k, v) in style.as_object().into_iter().flatten() { p[k.as_str()] = v.clone(); } exec(b, "text.createInPath", p)? @@ -524,7 +524,7 @@ fn add_text(b: &mut dyn Backend, a: &Args) -> Result { let x = a.get("x").and_then(Value::as_f64).ok_or("give `x` and `y` (or a `path`)")?; let y = a.get("y").and_then(Value::as_f64).ok_or("give `x` and `y` (or a `path`)")?; let mut p = json!({"x": x, "y": y, "text": text}); - for (k, v) in style.as_object().unwrap() { + for (k, v) in style.as_object().into_iter().flatten() { p[k.as_str()] = v.clone(); } if let Some(c) = a.get("color") { diff --git a/crates/mcp/tests/protocol_props.rs b/crates/mcp/tests/protocol_props.rs index 9cafa082..f355e5ea 100644 --- a/crates/mcp/tests/protocol_props.rs +++ b/crates/mcp/tests/protocol_props.rs @@ -1,6 +1,8 @@ //! MCP server properties (in-process, headless backend): tool schemas are valid, every tool //! survives junk arguments, JSON-RPC framing always answers correctly, and random editing through //! MCP keeps the document valid. +// Integration tests: unwrapping and panicking on failure is fine here, unlike in shipped code (AGENTS.md › Robustness). +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] use proptest::prelude::*; use serde_json::{Value, json}; diff --git a/crates/pathops/src/edit.rs b/crates/pathops/src/edit.rs index e3e4aba7..587a8247 100644 --- a/crates/pathops/src/edit.rs +++ b/crates/pathops/src/edit.rs @@ -188,8 +188,9 @@ pub fn remove_redundant_points(path: &PathData, tolerance: f64) -> PathData { segs.remove(i1); if sp.closed && i1 == 0 { // The merged segment now starts the loop. - let last = segs.pop().expect("non-empty"); - segs.insert(0, last); + if let Some(last) = segs.pop() { + segs.insert(0, last); + } } changed = true; break; @@ -287,18 +288,22 @@ pub fn join(paths: &[PathData], tolerance: f64) -> PathData { } } } - if open.len() == 1 { - let mut sp = open.pop().expect("one"); - let n = sp.anchors.len(); - if n > 2 && sp.anchors[0].p.distance(sp.anchors[n - 1].p) <= tolerance { - let last = sp.anchors.pop().expect("n > 2"); - let f = &mut sp.anchors[0]; + if open.len() == 1 + && let Some(mut sp) = open.pop() + { + let ends_meet = sp.anchors.len() > 2 && end_point(&sp, false).zip(end_point(&sp, true)).is_some_and(|(a, b)| a.distance(b) <= tolerance); + if ends_meet + && let Some(last) = sp.anchors.pop() + && let Some(f) = sp.anchors.first_mut() + { *f = Anchor::with_handles(f.p, last.h_in, f.h_out); } else { - let f = &mut sp.anchors[0]; - f.h_in = f.p; - let l = sp.anchors.last_mut().expect("n >= 2"); - l.h_out = l.p; + if let Some(f) = sp.anchors.first_mut() { + f.h_in = f.p; + } + if let Some(l) = sp.anchors.last_mut() { + l.h_out = l.p; + } } sp.closed = true; closed.push(sp); @@ -311,8 +316,7 @@ pub fn join(paths: &[PathData], tolerance: f64) -> PathData { for j in (i + 1)..open.len() { for ei in [false, true] { for ej in [false, true] { - let pi = if ei { open[i].anchors.last() } else { open[i].anchors.first() }.expect("len>=2").p; - let pj = if ej { open[j].anchors.last() } else { open[j].anchors.first() }.expect("len>=2").p; + let (Some(pi), Some(pj)) = (end_point(&open[i], ei), end_point(&open[j], ej)) else { continue }; let d = pi.distance(pj); if d < best.0 { best = (d, i, j, ei, ej); @@ -330,14 +334,18 @@ pub fn join(paths: &[PathData], tolerance: f64) -> PathData { if ej { b.reverse(); } - if d <= tolerance { + if d <= tolerance && !b.anchors.is_empty() { let first = b.anchors.remove(0); - let l = a.anchors.last_mut().expect("len>=2"); - *l = Anchor::with_handles(l.p, l.h_in, first.h_out); + if let Some(l) = a.anchors.last_mut() { + *l = Anchor::with_handles(l.p, l.h_in, first.h_out); + } } else { - let l = a.anchors.last_mut().expect("len>=2"); - l.h_out = l.p; - b.anchors[0].h_in = b.anchors[0].p; + if let Some(l) = a.anchors.last_mut() { + l.h_out = l.p; + } + if let Some(f) = b.anchors.first_mut() { + f.h_in = f.p; + } } a.anchors.extend(b.anchors); } @@ -369,3 +377,8 @@ pub fn split_into_grid(rect: Rect, rows: usize, cols: usize, gutter: f64) -> Vec } out } + +/// The first (`last == false`) or last anchor point of a subpath. +fn end_point(sp: &SubPath, last: bool) -> Option { + if last { sp.anchors.last() } else { sp.anchors.first() }.map(|a| a.p) +} diff --git a/crates/pathops/src/fit.rs b/crates/pathops/src/fit.rs index 83e10184..a9a91764 100644 --- a/crates/pathops/src/fit.rs +++ b/crates/pathops/src/fit.rs @@ -112,8 +112,7 @@ fn bern(t: f64) -> [f64; 4] { /// Solve the 2x2 normal equations for the handle lengths along the fixed tangents. fn generate(pts: &[Point], u: &[f64], t0: Vec2, t1: Vec2) -> CubicBez { - let p0 = pts[0]; - let p3 = *pts.last().unwrap(); + let (Some(&p0), Some(&p3)) = (pts.first(), pts.last()) else { return CubicBez::new(Point::ZERO, Point::ZERO, Point::ZERO, Point::ZERO) }; let dir_in = -t1; // handle at the end points backwards let (mut c00, mut c01, mut c11, mut x0, mut x1) = (0.0, 0.0, 0.0, 0.0, 0.0); for (p, &t) in pts.iter().zip(u) { diff --git a/crates/pathops/src/pathfinder.rs b/crates/pathops/src/pathfinder.rs index 9fa707db..3772c158 100644 --- a/crates/pathops/src/pathfinder.rs +++ b/crates/pathops/src/pathfinder.rs @@ -57,9 +57,9 @@ pub struct Region { } impl Region { - /// Front-most covering shape. - pub fn top(&self) -> usize { - *self.sources.last().expect("regions are never uncovered") + /// Front-most covering shape (`None` only for an uncovered face, which regions never are). + pub fn top(&self) -> Option { + self.sources.last().copied() } /// Does the face contain `p`? pub fn contains(&self, p: Point) -> bool { @@ -87,23 +87,27 @@ pub fn pathfinder(op: PathfinderOp, shapes: &[Shape]) -> Vec { return Vec::new(); } let front_key = shapes[n - 1].key; - if op == PathfinderOp::Unite { + let unite = || { let v: Vec<(&PathData, FillRule)> = shapes.iter().map(|s| (&s.path, s.rule)).collect(); - return one(unite_all(&v), front_key); + one(unite_all(&v), front_key) + }; + // Unite doesn't need the arrangement. + if op == PathfinderOp::Unite { + return unite(); } let Some(arr) = arrangement(shapes) else { return Vec::new() }; let p = &arr.tidy; match op { - PathfinderOp::Unite => unreachable!(), + PathfinderOp::Unite => unite(), PathfinderOp::MinusFront => one(all_contours_to_path(&arr.contours(|m| m[0] && !m[1..].iter().any(|&b| b)), p), shapes[0].key), PathfinderOp::MinusBack => one(all_contours_to_path(&arr.contours(|m| m[n - 1] && !m[..n - 1].iter().any(|&b| b)), p), front_key), PathfinderOp::Intersect => one(all_contours_to_path(&arr.contours(|m| m.iter().all(|&b| b)), p), front_key), PathfinderOp::Exclude => one(all_contours_to_path(&arr.contours(|m| m.iter().filter(|&&b| b).count() % 2 == 1), p), front_key), PathfinderOp::Divide => regions_of(&arr) .into_iter() - .map(|r| { - let key = shapes[r.top()].key; - Shape::new(r.path, FillRule::NonZero, key) + .filter_map(|r| { + let key = shapes.get(r.top()?)?.key; + Some(Shape::new(r.path, FillRule::NonZero, key)) }) .collect(), PathfinderOp::Trim => (0..n).flat_map(|i| one(all_contours_to_path(&arr.contours(|m| topmost(m) == Some(i)), p), shapes[i].key)).collect(), diff --git a/crates/pathops/tests/ops.rs b/crates/pathops/tests/ops.rs index 1bf29736..cd088491 100644 --- a/crates/pathops/tests/ops.rs +++ b/crates/pathops/tests/ops.rs @@ -1,3 +1,5 @@ +// Integration tests: unwrapping and panicking on failure is fine here, unlike in shipped code (AGENTS.md › Robustness). +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] use std::f64::consts::PI; use kurbo::{ParamCurve, Point, Rect, Shape as _}; diff --git a/crates/pathops/tests/prop_pathops.rs b/crates/pathops/tests/prop_pathops.rs index 5c3204d7..262e85e4 100644 --- a/crates/pathops/tests/prop_pathops.rs +++ b/crates/pathops/tests/prop_pathops.rs @@ -1,5 +1,7 @@ //! Path-operation properties on concave polygons, ellipses and random smooth curves (complements //! `props.rs`, which uses rectangles and circles). +// Integration tests: unwrapping and panicking on failure is fine here, unlike in shipped code (AGENTS.md › Robustness). +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] mod common; diff --git a/crates/pathops/tests/props.rs b/crates/pathops/tests/props.rs index 9637d190..f09e96f0 100644 --- a/crates/pathops/tests/props.rs +++ b/crates/pathops/tests/props.rs @@ -1,3 +1,5 @@ +// Integration tests: unwrapping and panicking on failure is fine here, unlike in shipped code (AGENTS.md › Robustness). +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] mod common; use std::f64::consts::PI; diff --git a/crates/pdf/src/import.rs b/crates/pdf/src/import.rs index acc53250..ab1e3cd4 100644 --- a/crates/pdf/src/import.rs +++ b/crates/pdf/src/import.rs @@ -252,7 +252,7 @@ impl Builder { if self.stack.len() <= 1 { return; } - let f = self.stack.pop().expect("frame"); + let Some(f) = self.stack.pop() else { return }; let node = match f.kind { FrameKind::Root => None, FrameKind::Skip => { @@ -386,7 +386,7 @@ fn shading_gradient(sp: &hayro_interpret::pattern::ShadingPattern) -> Option Device<'a> for Builder { let (w, h, sf) = (img.width(), img.height(), img.scale_factors()); let a = alpha.map(|a| resize_alpha(&a, w, h)).unwrap_or_else(|| vec![255; (w * h) as usize]); let rgba: Vec = match img { - ImageData::Rgb(d) => d.data.chunks_exact(3).zip(a).flat_map(|(c, a)| [c[0], c[1], c[2], a]).collect(), + ImageData::Rgb(d) => d.data.as_chunks::<3>().0.iter().zip(a).flat_map(|(c, a)| [c[0], c[1], c[2], a]).collect(), ImageData::Luma(d) => d.data.iter().zip(a).flat_map(|(g, a)| [*g, *g, *g, a]).collect(), }; decoded = Some((rgba, w, h, sf)); diff --git a/crates/render/examples/bench.rs b/crates/render/examples/bench.rs index 470f418b..e6bbe07b 100644 --- a/crates/render/examples/bench.rs +++ b/crates/render/examples/bench.rs @@ -1,4 +1,6 @@ //! `cargo run --release -p vectorcraft-render --example bench` — render timing on synthetic documents. +// Example (dev tool): unwrapping and panicking on failure is fine here, unlike in shipped code (AGENTS.md › Robustness). +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] use vectorcraft_color::{Color, Paint}; use vectorcraft_doc::{Appearance, Document, Node}; diff --git a/crates/render/src/lib.rs b/crates/render/src/lib.rs index 383404e1..3dd41cf5 100644 --- a/crates/render/src/lib.rs +++ b/crates/render/src/lib.rs @@ -118,7 +118,7 @@ impl Rendered { /// Un-premultiplied RGBA8 copy. pub fn to_straight(&self) -> Vec { let mut out = self.pixels.clone(); - for px in out.chunks_exact_mut(4) { + for px in out.as_chunks_mut::<4>().0 { let a = px[3] as u32; if a != 0 && a != 255 { for c in &mut px[..3] { @@ -129,17 +129,20 @@ impl Rendered { out } /// Encode as PNG. - pub fn to_png(&self) -> Vec { + pub fn to_png(&self) -> Result, String> { let mut buf = Vec::new(); - let img = image::RgbaImage::from_raw(self.width, self.height, self.to_straight()).expect("size"); - img.write_to(&mut std::io::Cursor::new(&mut buf), image::ImageFormat::Png).expect("png encode"); - buf + let img = image::RgbaImage::from_raw(self.width, self.height, self.to_straight()) + .ok_or("PNG encoding failed: pixel buffer doesn't match the image size")?; + img.write_to(&mut std::io::Cursor::new(&mut buf), image::ImageFormat::Png).map_err(|e| format!("PNG encoding failed: {e}"))?; + Ok(buf) } /// Encode as JPEG (flattened on white) at `quality` 1..=100. - pub fn to_jpeg(&self, quality: u8) -> Vec { + pub fn to_jpeg(&self, quality: u8) -> Result, String> { let rgba = self.to_straight(); let rgb: Vec = rgba - .chunks_exact(4) + .as_chunks::<4>() + .0 + .iter() .flat_map(|p| { let a = p[3] as u32; let mix = |c: u8| ((c as u32 * a + 255 * (255 - a)) / 255) as u8; @@ -148,15 +151,17 @@ impl Rendered { .collect(); let mut buf = Vec::new(); let enc = image::codecs::jpeg::JpegEncoder::new_with_quality(&mut buf, quality.clamp(1, 100)); - let _ = image::ImageEncoder::write_image(enc, &rgb, self.width, self.height, image::ExtendedColorType::Rgb8); - buf + image::ImageEncoder::write_image(enc, &rgb, self.width, self.height, image::ExtendedColorType::Rgb8) + .map_err(|e| format!("JPEG encoding failed: {e}"))?; + Ok(buf) } /// Encode as lossless WebP. - pub fn to_webp(&self) -> Vec { + pub fn to_webp(&self) -> Result, String> { let mut buf = Vec::new(); let enc = image::codecs::webp::WebPEncoder::new_lossless(&mut buf); - let _ = image::ImageEncoder::write_image(enc, &self.to_straight(), self.width, self.height, image::ExtendedColorType::Rgba8); - buf + image::ImageEncoder::write_image(enc, &self.to_straight(), self.width, self.height, image::ExtendedColorType::Rgba8) + .map_err(|e| format!("WebP encoding failed: {e}"))?; + Ok(buf) } /// Straight-alpha RGBA at (x, y). pub fn pixel(&self, x: u32, y: u32) -> [u8; 4] { diff --git a/crates/render/src/tests.rs b/crates/render/src/tests.rs index 4a8aff07..68ef1779 100644 --- a/crates/render/src/tests.rs +++ b/crates/render/src/tests.rs @@ -188,7 +188,7 @@ fn region_export_png() { d.insert(Some(l), 0, n).unwrap(); let r = Renderer::new().render_region(&d, d.artboards[0].rect, 2.0, true); assert_eq!((r.width, r.height), (200, 200)); - let png = r.to_png(); + let png = r.to_png().unwrap(); assert_eq!(&png[1..4], b"PNG"); } diff --git a/crates/render/tests/golden.rs b/crates/render/tests/golden.rs index 74f3066b..20955f17 100644 --- a/crates/render/tests/golden.rs +++ b/crates/render/tests/golden.rs @@ -1,5 +1,7 @@ //! Pixel-sample "golden" tests on small fixtures: gradients, blend modes, clip groups, stroke //! alignment/dashes/arrowheads, effects, text, and opacity folding vs layer compositing. +// Integration tests: unwrapping and panicking on failure is fine here, unlike in shipped code (AGENTS.md › Robustness). +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] use serde_json::json; use vectorcraft_color::{BlendMode, Color, Gradient, GradientKind, GradientPaint, GradientStop, Paint}; @@ -137,7 +139,7 @@ fn blend_ref(mode: BlendMode, b: f64, s: f64) -> f64 { b + (2.0 * s - 1.0) * (d - b) } } - _ => unreachable!(), + other => panic!("unexpected {other:?}"), } } @@ -496,7 +498,7 @@ fn png_encoding_roundtrips_pixels() { b.rect(Rect::new(10.0, 10.0, 60.0, 60.0), Color::rgb(0.2, 0.4, 0.6), |n| n.opacity = 0.5); let d = b.build(); let r = vectorcraft_render::Renderer::new().render_region(&d, Rect::new(0.0, 0.0, 100.0, 100.0), 1.0, false); - let decoded = Image::from_png(&r.to_png()).unwrap(); + let decoded = Image::from_png(&r.to_png().unwrap()).unwrap(); assert_eq!(decoded, Image::from_rendered(&r)); } diff --git a/crates/svg/src/import.rs b/crates/svg/src/import.rs index 1e146722..83ed4d8c 100644 --- a/crates/svg/src/import.rs +++ b/crates/svg/src/import.rs @@ -275,7 +275,7 @@ impl Importer { if !g.filters().is_empty() { self.warn(format!("filter on {label} ignored")); } - let children = self.children(g, ts); + let mut children = self.children(g, ts); let mut n = if let Some(cp) = g.clip_path() { if cp.clip_path().is_some() { self.warn(format!("nested clip path on {label} approximated by its outer clip")); @@ -294,8 +294,9 @@ impl Importer { && mask.is_none() && children.len() == 1 && (g.blend_mode() == usvg::BlendMode::Normal || children[0].blend == BlendMode::Normal) + && let Some(only) = children.pop() { - let mut c = Arc::unwrap_or_clone(children.into_iter().next().unwrap()); + let mut c = Arc::unwrap_or_clone(only); c.opacity *= g.opacity().get(); if g.blend_mode() != usvg::BlendMode::Normal { c.blend = blend(g.blend_mode()); diff --git a/crates/svg/tests/prop_svg.rs b/crates/svg/tests/prop_svg.rs index 4c95dfc8..2ce710e7 100644 --- a/crates/svg/tests/prop_svg.rs +++ b/crates/svg/tests/prop_svg.rs @@ -1,5 +1,7 @@ //! SVG export/import properties: never panics, always re-imports, and plain art survives the round //! trip visually (render comparison with a perceptual tolerance). +// Integration tests: unwrapping and panicking on failure is fine here, unlike in shipped code (AGENTS.md › Robustness). +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] use proptest::prelude::*; use vectorcraft_color::{Color, Paint}; diff --git a/crates/svg/tests/roundtrip.rs b/crates/svg/tests/roundtrip.rs index 907808c0..c865203a 100644 --- a/crates/svg/tests/roundtrip.rs +++ b/crates/svg/tests/roundtrip.rs @@ -1,3 +1,5 @@ +// Integration tests: unwrapping and panicking on failure is fine here, unlike in shipped code (AGENTS.md › Robustness). +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] use std::sync::Arc; use vectorcraft_color::{BlendMode, Color, Gradient, GradientGeom, GradientKind, GradientPaint, Paint}; diff --git a/crates/testkit/src/lib.rs b/crates/testkit/src/lib.rs index 5d339202..e5c15ac2 100644 --- a/crates/testkit/src/lib.rs +++ b/crates/testkit/src/lib.rs @@ -11,6 +11,10 @@ //! - [`geom`]: geometry assertions (approximate equality, curve sampling, Hausdorff distance). //! //! This crate may only be used as a dev-dependency (enforced by `cargo xtask layers`). +// Test support only (a dev-dependency of every crate that uses it): a failed setup or assertion +// must panic, like `assert!`, so the shipped-code ban on panicking (AGENTS.md › Robustness) doesn't +// apply here. +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] #![forbid(unsafe_code)] pub mod fixtures; diff --git a/crates/text/src/fontdb.rs b/crates/text/src/fontdb.rs index faf833c0..20762b37 100644 --- a/crates/text/src/fontdb.rs +++ b/crates/text/src/fontdb.rs @@ -400,21 +400,19 @@ impl FontDb { } /// Resolve a family + style to a face, falling back to the closest style of the family, then to - /// Source Sans 3 Regular. - pub fn face(&self, family: &str, style: &str) -> Arc { + /// Source Sans 3 Regular. `None` only if no font at all is loaded (the bundled fonts failed to + /// parse), in which case text has no glyphs. + pub fn face(&self, family: &str, style: &str) -> Option> { if let Some(f) = self.find(family, style) { - return f; + return Some(f); } #[cfg(not(target_arch = "wasm32"))] if self.load_cataloged(family) && let Some(f) = self.find(family, style) { - return f; + return Some(f); } - self.find(FALLBACK_FAMILY, style) - .or_else(|| self.find(FALLBACK_FAMILY, "Regular")) - .or_else(|| self.read_faces().first().cloned()) - .expect("bundled fonts are always present") + self.find(FALLBACK_FAMILY, style).or_else(|| self.find(FALLBACK_FAMILY, "Regular")).or_else(|| self.read_faces().first().cloned()) } /// Is `family` available (loaded)? diff --git a/crates/text/src/shape.rs b/crates/text/src/shape.rs index aab095aa..49c287d4 100644 --- a/crates/text/src/shape.rs +++ b/crates/text/src/shape.rs @@ -83,16 +83,17 @@ fn is_cjk(c: char) -> bool { /// Vertical metrics (points) of a style's resolved face: (ascent, descent, leading). pub(crate) fn style_metrics(db: &FontDb, st: &CharStyle) -> (f64, f64, f64) { - let face = db.face(&st.font_family, &st.font_style); - let k = st.size / face.upem; let vs = st.v_scale / 100.0; + let Some(face) = db.face(&st.font_family, &st.font_style) else { return (st.size * 0.8 * vs, st.size * 0.2 * vs, st.effective_leading()) }; + let k = st.size / face.upem; (face.ascent * k * vs, face.descent * k * vs, st.effective_leading()) } /// Cap height and x height (points) of a style's resolved face. pub(crate) fn cap_x_heights(db: &FontDb, st: &CharStyle) -> (f64, f64) { - let face = db.face(&st.font_family, &st.font_style); - let k = st.size / face.upem * st.v_scale / 100.0; + let vs = st.v_scale / 100.0; + let Some(face) = db.face(&st.font_family, &st.font_style) else { return (st.size * 0.7 * vs, st.size * 0.5 * vs) }; + let k = st.size / face.upem * vs; (face.cap_height * k, face.x_height * k) } @@ -111,7 +112,7 @@ pub(crate) fn shape_range( if a >= b { continue; } - let primary = db.face(&st.font_family, &st.font_style); + let Some(primary) = db.face(&st.font_family, &st.font_style) else { continue }; let pmap = primary.skrifa().map(|f| f.charmap()); // Split into segments by font coverage. let mut seg_start = a; diff --git a/crates/text/src/tests.rs b/crates/text/src/tests.rs index 1a62bfe7..e0bd49e9 100644 --- a/crates/text/src/tests.rs +++ b/crates/text/src/tests.rs @@ -47,15 +47,15 @@ fn bundled_families_and_styles() { #[test] fn style_lookup_and_fallback() { - let f = db().face("Inter", "Semibold"); + let f = db().face("Inter", "Semibold").unwrap(); assert_eq!((f.family.as_str(), f.style.as_str()), ("Inter", "SemiBold")); - let f = db().face("inter", "Bold"); + let f = db().face("inter", "Bold").unwrap(); assert_eq!(f.style, "SemiBold", "closest weight"); - let f = db().face("No Such Font", "Regular"); + let f = db().face("No Such Font", "Regular").unwrap(); assert_eq!((f.family.as_str(), f.style.as_str()), ("Source Sans 3", "Regular")); - let f = db().face("No Such Font", "Bold"); + let f = db().face("No Such Font", "Bold").unwrap(); assert_eq!(f.style, "Bold"); - assert_eq!(db().face("Source Sans 3", "Italic").style, "Italic"); + assert_eq!(db().face("Source Sans 3", "Italic").unwrap().style, "Italic"); } #[test] @@ -344,7 +344,7 @@ fn on_path_circle_and_overflow() { #[test] fn fallback_font_per_character() { - let primary = db().face("Source Sans 3", "Regular"); + let primary = db().face("Source Sans 3", "Regular").unwrap(); // Find a character the primary lacks but another bundled font has. let c = ['\u{2500}', '\u{2192}', '\u{25B6}', '\u{2588}', '\u{21E5}', '\u{2318}'] .into_iter() diff --git a/crates/text/src/tests_typo.rs b/crates/text/src/tests_typo.rs index 99335006..83194d8e 100644 --- a/crates/text/src/tests_typo.rs +++ b/crates/text/src/tests_typo.rs @@ -363,7 +363,7 @@ fn opentype_ligature_switches() { #[test] fn face_charmap_for_glyphs_panel() { - let f = db().face("Source Sans 3", "Regular"); + let f = db().face("Source Sans 3", "Regular").unwrap(); let chars = f.chars(); assert!(chars.len() > 200, "{}", chars.len()); assert!(chars.windows(2).all(|w| w[0].0 < w[1].0)); diff --git a/crates/tools/src/distort/arap.rs b/crates/tools/src/distort/arap.rs index fbe8bbf4..946ed52a 100644 --- a/crates/tools/src/distort/arap.rs +++ b/crates/tools/src/distort/arap.rs @@ -204,7 +204,7 @@ impl Mesh { /// Band width (in vertices) of the vertex adjacency. fn vertex_band(&self) -> usize { - self.tris.iter().map(|t| t.iter().max().unwrap() - t.iter().min().unwrap()).max().unwrap_or(0) + self.tris.iter().filter_map(|t| Some(t.iter().max()? - t.iter().min()?)).max().unwrap_or(0) } /// The triangle and barycentric coordinates of `p` (extrapolated from the nearest mesh cell @@ -448,18 +448,18 @@ pub fn auto_pins(mesh: &Mesh, count: usize) -> Vec { return vec![]; } let c = centres.iter().fold(Vec2::ZERO, |a, p| a + p.to_vec2()) / centres.len() as f64; - let first = *centres.iter().min_by(|a, b| a.distance_squared(c.to_point()).total_cmp(&b.distance_squared(c.to_point()))).unwrap(); + let Some(&first) = centres.iter().min_by(|a, b| a.distance_squared(c.to_point()).total_cmp(&b.distance_squared(c.to_point()))) else { + return vec![]; + }; let mut out = vec![first]; while out.len() < count.min(centres.len()) { - let next = centres - .iter() - .max_by(|a, b| { - let da = out.iter().map(|o| o.distance_squared(**a)).fold(f64::MAX, f64::min); - let db = out.iter().map(|o| o.distance_squared(**b)).fold(f64::MAX, f64::min); - da.total_cmp(&db) - }) - .copied() - .unwrap(); + let Some(&next) = centres.iter().max_by(|a, b| { + let da = out.iter().map(|o| o.distance_squared(**a)).fold(f64::MAX, f64::min); + let db = out.iter().map(|o| o.distance_squared(**b)).fold(f64::MAX, f64::min); + da.total_cmp(&db) + }) else { + break; + }; out.push(next); } out diff --git a/crates/tools/src/distort/liquify.rs b/crates/tools/src/distort/liquify.rs index a56d5f26..a02bf22f 100644 --- a/crates/tools/src/distort/liquify.rs +++ b/crates/tools/src/distort/liquify.rs @@ -218,8 +218,9 @@ pub fn dabs(points: &[Point], spacing: f64) -> Vec { break; } } - let end = *points.last().unwrap(); - if end.distance(last) > spacing * 0.25 { + if let Some(&end) = points.last() + && end.distance(last) > spacing * 0.25 + { out.push(end); } out diff --git a/crates/tools/src/draw2/anchor.rs b/crates/tools/src/draw2/anchor.rs index 5533b1df..7a623652 100644 --- a/crates/tools/src/draw2/anchor.rs +++ b/crates/tools/src/draw2/anchor.rs @@ -128,10 +128,12 @@ impl Tool for AnchorTool { *began = true; } } - out.push(match self.state { - State::Convert { id, si, ai, .. } => { - Action::Preview("path.convertAnchor".into(), json!({"id": id.0, "subpath": si, "anchor": ai, "to": "smooth", "x": p.x, "y": p.y})) - } + let preview = match self.state { + State::Idle => return out, + State::Convert { id, si, ai, .. } => Action::Preview( + "path.convertAnchor".into(), + json!({"id": id.0, "subpath": si, "anchor": ai, "to": "smooth", "x": p.x, "y": p.y}), + ), State::Handle { id, si, ai, out, .. } => Action::Preview( "path.setHandle".into(), json!({"id": id.0, "subpath": si, "anchor": ai, "which": if out { "out" } else { "in" }, "x": p.x, "y": p.y, "independent": true}), @@ -140,8 +142,8 @@ impl Tool for AnchorTool { "path.reshapeSegment".into(), json!({"id": id.0, "subpath": si, "segment": seg, "t": t, "dx": p.x - start.x, "dy": p.y - start.y}), ), - State::Idle => unreachable!(), - }); + }; + out.push(preview); out } ("anchorPoint", PointerKind::Up) => { diff --git a/crates/tools/src/text.rs b/crates/tools/src/text.rs index 8ef68732..c48ca408 100644 --- a/crates/tools/src/text.rs +++ b/crates/tools/src/text.rs @@ -137,9 +137,8 @@ impl TypeTool { } if self.typing.is_none() { out.push(Action::Begin("Typing".into())); - self.typing = Some(Typing { base: t.runs.clone(), cur: t.runs.clone() }); } - let ty = self.typing.as_mut().expect("typing session"); + let ty = self.typing.get_or_insert_with(|| Typing { base: t.runs.clone(), cur: t.runs.clone() }); let len = edit::runs_len(&ty.cur); let (a, b) = (self.caret.min(self.anchor).min(len), self.caret.max(self.anchor).min(len)); let caret = match &styled { diff --git a/crates/trace/src/lib.rs b/crates/trace/src/lib.rs index 7eabf0fe..8776d80f 100644 --- a/crates/trace/src/lib.rs +++ b/crates/trace/src/lib.rs @@ -229,7 +229,7 @@ pub fn preset(name: &str) -> Option { /// All built-in presets in panel order. pub fn presets() -> Vec<(&'static str, TraceParams)> { - PRESET_NAMES.iter().map(|n| (*n, preset(n).expect("built-in preset"))).collect() + PRESET_NAMES.iter().filter_map(|n| Some((*n, preset(n)?))).collect() } /// One traced shape: an outer contour plus its holes, filled with `color`. diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index e4aeb5c9..3f8f09b8 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -112,7 +112,7 @@ pub fn fit(app: &mut VectorcraftApp, how: &str) { _ => st.doc.artboards.first().map(|a| a.rect), }; let Some(target) = target else { return }; - let v = app.view_mut().unwrap(); + let Some(v) = app.view_mut() else { return }; v.center = target.center(); v.fitted = true; if how == "view.actualSize" { diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index 7057c4a9..91714ec4 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -52,7 +52,7 @@ pub enum Outcome { fn ok(v: Value) -> Outcome { Outcome::Done(json!({"ok": true, "result": v})) } -fn err(e: impl std::fmt::Display) -> Outcome { +pub(crate) fn err(e: impl std::fmt::Display) -> Outcome { Outcome::Done(json!({"ok": false, "error": e.to_string()})) } fn wrap(r: Result) -> Outcome { @@ -289,7 +289,10 @@ pub fn handle(app: &mut VectorcraftApp, ctx: &egui::Context, req: &ControlReques return err(e); } let img = app.canvas.renderer.render_region(&doc, r, scale, true); - let png = img.to_png(); + let png = match img.to_png() { + Ok(png) => png, + Err(e) => return err(e), + }; match s("path") { Some(path) => match app.services.write.as_mut() { Some(w) => wrap(w(path, &png).map(|_| json!({"path": path, "width": img.width, "height": img.height}))), @@ -320,7 +323,10 @@ pub fn save_screenshot(app: &mut VectorcraftApp, image: &egui::ColorImage, path: }; let rgba: Vec = image.pixels.iter().flat_map(|c| c.to_array()).collect(); let img = vectorcraft_render::Rendered { width: w as u32, height: h as u32, pixels: rgba }; - let png = img.to_png(); + let png = match img.to_png() { + Ok(png) => png, + Err(e) => return json!({"ok": false, "error": e}), + }; match app.services.write.as_mut() { Some(wr) => match wr(path, &png) { Ok(()) => json!({"ok": true, "result": {"path": path, "width": w, "height": h}}), diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 066c81fa..edcfc22a 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -320,7 +320,10 @@ impl VectorcraftApp { let Some(rx) = self.control_rx.take() else { return }; while let Ok(req) = rx.try_recv() { let reply = req.reply.clone(); - match control::handle(self, ctx, &req) { + // Guarded per request: a panic must not drop the channel (taken out of `self` above). + let outcome = vectorcraft_engine::guard::catch_panic(|| control::handle(self, ctx, &req)) + .unwrap_or_else(|msg| control::err(format!("internal error: {msg} (please report this bug)"))); + match outcome { control::Outcome::Done(v) => { let _ = reply.send(v); } @@ -419,8 +422,15 @@ pub fn now_ms() -> f64 { /// eframe isn't a dependency of this crate (the host owns the event loop); these entry points are /// called from the host's `eframe::App` impl. impl VectorcraftApp { - /// Per-frame logic before layout (control channel, shortcuts, inbox). + /// Per-frame logic before layout (control channel, shortcuts, inbox). A bug that panics costs + /// one frame and shows an error, instead of closing the app with unsaved work. pub fn logic(&mut self, ctx: &egui::Context) { + if let Err(msg) = vectorcraft_engine::guard::catch_panic(|| self.logic_frame(ctx)) { + self.status(format!("Internal error: {msg} (please report this bug)")); + } + } + + fn logic_frame(&mut self, ctx: &egui::Context) { if !self.styled { theme::install_fonts(ctx); theme::apply(ctx, self.ui.brightness); @@ -498,6 +508,12 @@ impl VectorcraftApp { /// Lay out the whole window. pub fn ui(&mut self, ui: &mut egui::Ui) { + if let Err(msg) = vectorcraft_engine::guard::catch_panic(|| self.ui_frame(ui)) { + self.status(format!("Internal error: {msg} (please report this bug)")); + } + } + + fn ui_frame(&mut self, ui: &mut egui::Ui) { let ctx = ui.ctx().clone(); if !self.fonts_ready { ctx.request_repaint(); diff --git a/crates/ui-egui/src/menus.rs b/crates/ui-egui/src/menus.rs index 2eed99a7..5ab5b43d 100644 --- a/crates/ui-egui/src/menus.rs +++ b/crates/ui-egui/src/menus.rs @@ -645,10 +645,10 @@ pub fn run_ui_command(app: &mut VectorcraftApp, id: &str, p: &Value) -> Option match p.get("index").and_then(Value::as_u64).map(|i| i as usize).or(app.session.active_index()) { - Some(i) => crate::unsaved::close(app, i), - None => return None, - }, + "file.close" => { + let i = p.get("index").and_then(Value::as_u64).map(|i| i as usize).or(app.session.active_index())?; + crate::unsaved::close(app, i) + } "file.closeAll" => crate::unsaved::close_all(app, "closeAll"), "app.quit" => crate::unsaved::close_all(app, "quit"), "ui.swatchOptions" => match s("name") { @@ -1742,7 +1742,11 @@ pub fn pretty_shortcut(s: &str) -> String { if cfg!(target_os = "macos") { let mut mods = String::new(); let parts: Vec<&str> = s.split('+').collect(); - let (key, ms) = if s.ends_with("++") { ("+", &parts[..parts.len() - 2]) } else { (*parts.last().unwrap(), &parts[..parts.len() - 1]) }; + let (key, ms): (&str, &[&str]) = if s.ends_with("++") { + ("+", parts.get(..parts.len().saturating_sub(2)).unwrap_or_default()) + } else { + parts.split_last().map_or(("", &[][..]), |(k, rest)| (*k, rest)) + }; for m in ["Ctrl", "Alt", "Shift", "Cmd"] { if ms.contains(&m) { mods.push_str(match m { diff --git a/crates/ui-egui/src/panels/glyphs.rs b/crates/ui-egui/src/panels/glyphs.rs index cd2cd80f..bf722e4e 100644 --- a/crates/ui-egui/src/panels/glyphs.rs +++ b/crates/ui-egui/src/panels/glyphs.rs @@ -141,7 +141,10 @@ pub fn show(app: &mut VectorcraftApp, ui: &mut Ui) { // The panel browses the current text's font unless another font was picked here. let picked: Option<(String, String)> = pstate(ui.ctx(), "gl-font"); let (family, style) = picked.or(current).unwrap_or_else(|| (vectorcraft_text::FALLBACK_FAMILY.to_string(), "Regular".to_string())); - let face = db.face(&family, &style); + let Some(face) = db.face(&family, &style) else { + ui.label(egui::RichText::new("No fonts are available.").color(t.text_dim)); + return; + }; let subset: usize = pstate(ui.ctx(), "gl-subset"); let (_, lo, hi) = SUBSETS[subset.min(SUBSETS.len() - 1)]; let w = ui.available_width(); diff --git a/crates/ui-egui/src/panels/layers.rs b/crates/ui-egui/src/panels/layers.rs index 3ceb682a..03ce91bb 100644 --- a/crates/ui-egui/src/panels/layers.rs +++ b/crates/ui-egui/src/panels/layers.rs @@ -287,8 +287,11 @@ fn row( } } - if has_children && expanded.contains(&n.id.0) { - for (i, c) in n.children().unwrap().iter().enumerate().rev() { + if has_children + && expanded.contains(&n.id.0) + && let Some(children) = n.children() + { + for (i, c) in children.iter().enumerate().rev() { row(ui, doc, c, depth + 1, i == 0 && n.clips(), sel, target, current, expanded, actions, t); } } diff --git a/crates/ui-egui/src/panels/swatches.rs b/crates/ui-egui/src/panels/swatches.rs index d846a293..9d74e8af 100644 --- a/crates/ui-egui/src/panels/swatches.rs +++ b/crates/ui-egui/src/panels/swatches.rs @@ -735,11 +735,10 @@ pub(crate) fn tile_grid<'a, T>(ui: &mut Ui, items: &'a [T], view: View, is_folde let per_row = ((w - 2.0) / pitch).floor().max(1.0) as usize; let mut rows: Vec> = vec![vec![]]; for e in items { - let len = rows.last().map_or(0, Vec::len); - if len == per_row || (len > 0 && is_folder(e)) { - rows.push(vec![]); + match rows.last_mut() { + Some(row) if row.len() < per_row && (row.is_empty() || !is_folder(e)) => row.push(e), + _ => rows.push(vec![e]), } - rows.last_mut().expect("a row").push(e); } let mut out = Vec::with_capacity(items.len()); for row in rows { diff --git a/docs/development.md b/docs/development.md index b8c5bca7..6787a85a 100644 --- a/docs/development.md +++ b/docs/development.md @@ -27,3 +27,69 @@ How the web shell (`apps/vectorcraft-web/src/web.rs`) differs from desktop: ## Vendor names gate `cargo xtask brands` (part of `cargo xtask ci`) fails when user-visible text names another vendor's products or company: string literals in Rust sources (command labels and params docs, menus, panels, MCP tool definitions), `Cargo.toml` descriptions and packaging files. Comments and test code are not checked. Say "the reference app" or name the feature itself. A line that must keep an old name, such as an alias that files or preferences from earlier versions still use, carries a `brand-ok` comment. + +## Robustness: VectorCraft never crashes + +A crash takes the user's unsaved work with it, and much of what the app reads is untrusted: SVG, +PDF, `.ai` and `.vectorcraft` files, pasted data, MCP and control-channel messages, command +parameters. Shipped code therefore never panics. Anything that can fail returns `Result` (or +`Option`), the caller handles or propagates it, and the user sees an error message. + +### Enforced by lints + +`Cargo.toml` denies these clippy lints for the whole workspace, and `cargo xtask ci` runs clippy with +`-D warnings`: + +| Banned in shipped code | Use instead | +|---|---| +| `x.unwrap()`, `x.expect("…")` | `x?`, `x.ok_or(err)?`, `let Some(v) = x else { return … }`, `if let`, `unwrap_or` / `unwrap_or_default` / `unwrap_or_else` | +| `v.last().unwrap()`, `v.pop().unwrap()` | `if let Some(l) = v.last_mut()`, `let Some((last, rest)) = v.split_last() else { … }` | +| `panic!`, `unreachable!()` in a match arm | return an error (`EngineError::Other`, `bad(C, "…")`) or a sensible default | +| `todo!`, `unimplemented!` | don't merge unfinished paths; return an error that says what isn't supported | + +`clippy.toml` allows `unwrap`, `expect` and `panic` inside `#[test]` and `#[cfg(test)]` code. +Integration-test files under `tests/` and `examples/`, and the `testkit` crate, opt out with +`#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)]`. Tests use `panic!("…")` +instead of `unreachable!()`. + +### Not caught by lints: avoid implicit panics too + +- **Indexing and slicing:** `v[i]` and `&s[a..b]` panic when out of range. When the index comes from + data (a file, a parameter, another document), use `v.get(i)` / `s.get(a..b)`. Slicing a `&str` also + panics off a UTF-8 boundary. +- **Arithmetic:** integer division or `%` by zero, `usize` subtraction that can go below zero (write + `i + 1 == n` instead of `i == n - 1`, or use `saturating_sub`), and `as` casts from non-finite floats. +- **Unbounded work:** cap counts, sizes and allocations read from input. Image dimensions, repeat + counts, line breaks and recursion depth all need a limit. +- **Encoders and decoders:** propagate their errors. Writing an empty file is a silent failure, not a fix. + +Don't silence the lint by discarding errors. `let _ = …` and `.ok()` are for failures that really +don't matter, with a comment saying why. + +### The safety net + +`vectorcraft_engine::guard::catch_panic` wraps every entry point: + +- `Session::execute` restores the active document, selection and interaction as they were before + the top-level command and returns `EngineError::Internal`. +- Tool events (`Session::pointer`, `tool_key`, `tool_text`) reset the tool and cancel its drag. +- The MCP server answers the request with a JSON-RPC internal error and keeps serving. +- `VectorcraftApp::logic` and `ui` lose one frame and show the error in the status bar; each + control-channel request is guarded on its own. + +The net exists for bugs, not as a substitute for `Result`. On wasm a panic aborts the app, so the +web build has no net at all. + +### Fuzzing + +Untrusted input has property tests that must never panic: + +- `crates/engine/tests/import_fuzz.rs`: garbage, hostile and mutated SVG and PDF, imported, then + rendered and exported. +- `crates/engine/tests/command_sweep.rs`: every command with junk parameters. +- `crates/format/tests/prop_format.rs`: garbage and mutated `.vectorcraft` files. +- `crates/mcp/tests/protocol_props.rs`: malformed MCP messages. + +CI runs a few dozen cases each. Before touching an importer, run a deeper search, e.g. +`PROPTEST_CASES=20000 cargo test --release -p vectorcraft-engine --test import_fuzz`. When it finds a +panic, fix the code and add the input as a regular test.